sbin/hammer: Partly revert 8b640662 in 2014
authorTomohiro Kusumi <kusumi.tomohiro@gmail.com>
Wed, 21 Sep 2016 22:04:18 +0000 (07:04 +0900)
committerTomohiro Kusumi <kusumi.tomohiro@gmail.com>
Sun, 9 Oct 2016 01:21:20 +0000 (10:21 +0900)
I guess it makes sense that 8b640662 silently ignores trailing / and
do destructive operations to a filesystem, since there isn't really
anything one can do by specifying a root inode (directory) of a PFS,
but it shouldn't silently modify (not ignore) the user input.

A command like pfs-status uses that input after calling getpfs(),
so the program should not silently modify the string and print it
as if that was the user input, because it's not. It should also avoid
modifying argv[i] even if it's technically possible (make it shorter).

It's also better to do it based on lstat(2) result than unconditionally
checking a trailing / based on assumption that a symlink that ends
with / is a directory (which is right if it actually exists).

The only case behaves differently from 8b640662 is when there is a
slave PFS that hasn't been synced with a master PFS yet after created.
In this case it fails to run commands using path arg with trailing /,
because lstat(2) mentioned above in scanpfsid() fails due to lack of
the root inode. This occurs by design of HAMMER, but not a tool issue.

Above won't be a problem either because the scenario mentioned in
8b640662 is mostly about auto completion by a shell, while in this
case the shell just recognizes it as a broken link. The tool doesn't
need to assist everything that users would potentially do based on
assumption that may be right or wrong.
Also see http://lists.dragonflybsd.org/pipermail/users/2015-April/228012.html

-----
Actually I even suspect reports by users and 8b640662 were wrong
in the first place, because one would still get the same PFS id from
both "symlink" and "symlink/" when "symlink" is a symlink to PFS.
"symlink" can extract the PFS id directly from "@@.." format, but
"symlink/" eventually results the same via hammer_pfs_autodetect().
In fact I could pfs-upgrade/pfs-downgrade by reverting whole 8b640662.

Having said that, it still needs to strip trailing / on pfs-destroy
otherwise it won't be able to remove a symlink to PFS after it gets
destroyed.

sbin/hammer/cmd_mirror.c
sbin/hammer/cmd_pfs.c
sbin/hammer/hammer.h

index 9077188..b01ccea 100644 (file)
@@ -92,7 +92,7 @@ hammer_cmd_mirror_read(char **av, int ac, int streaming)
        hammer_ioc_mrecord_any_t mrec;
        hammer_tid_t sync_tid;
        histogram_t histogram_ary;
-       char *filesystem;
+       const char *filesystem;
        char *buf = malloc(SERIALBUF_SIZE);
        int interrupted = 0;
        int error;
@@ -766,7 +766,7 @@ void
 hammer_cmd_mirror_write(char **av, int ac)
 {
        struct hammer_ioc_mirror_rw mirror;
-       char *filesystem;
+       const char *filesystem;
        char *buf = malloc(SERIALBUF_SIZE);
        struct hammer_ioc_pseudofs_rw pfs;
        struct hammer_ioc_mrecord_head pickup;
index 824ec89..16b75f4 100644 (file)
@@ -91,28 +91,47 @@ getdir(const char *path)
        return(dirpath);
 }
 
+/*
+ * If path is a symlink, return strdup'd path.
+ * If it's a directory via symlink, strip trailing /
+ * from strdup'd path and return the symlink.
+ */
+static char*
+getlink(const char *path)
+{
+       int i;
+       char *linkpath;
+       struct stat st;
+
+       if (lstat(path, &st))
+               return(NULL);
+       linkpath = strdup(path);
+
+       if (S_ISDIR(st.st_mode)) {
+               i = strlen(linkpath) - 1;
+               while (i > 0 && linkpath[i] == '/')
+                       linkpath[i--] = 0;
+               lstat(linkpath, &st);
+       }
+       if (S_ISLNK(st.st_mode)) {
+               return(linkpath);
+       }
+
+       free(linkpath);
+       return(NULL);
+}
+
 /*
  * Calculate the PFS id given a path to a file/directory or
  * a @@%llx:%d softlink.
  */
 int
-getpfs(struct hammer_ioc_pseudofs_rw *pfs, char *path)
+getpfs(struct hammer_ioc_pseudofs_rw *pfs, const char *path)
 {
        int fd;
-       char *p;
 
        clrpfs(pfs, NULL, -1);
 
-       /*
-        * Trailing '/' must be removed so that upon pfs-destroy
-        * the symlink can be deleted without problems.
-        * Root directory (/) must be excluded from this.
-        */
-       p = path + (int)strlen(path) - 1;
-       assert(p >= path);
-       while (p != path && *p == '/')
-               *p-- = 0;
-
        fd = scanpfsid(pfs, path);
        if (fd < 0) {
                /*
@@ -150,7 +169,8 @@ static int
 scanpfsid(struct hammer_ioc_pseudofs_rw *pfs, const char *path)
 {
        int fd = -1;
-       const char *p;
+       const char *p = path;
+       char *linkpath;
        char buf[64];
        uintmax_t dummy_tid;
        struct stat st;
@@ -163,16 +183,18 @@ scanpfsid(struct hammer_ioc_pseudofs_rw *pfs, const char *path)
                return(-1);  /* neither */
        }
 
-       /*
-        * If the path is a link read the link.
-        */
-       if (lstat(path, &st) == 0 && S_ISLNK(st.st_mode)) {
+       linkpath = getlink(path);
+       if (linkpath) {
+               /*
+                * Read the symlink assuming it's a link to PFS.
+                */
                bzero(buf, sizeof(buf));
-               if (readlink(path, buf, sizeof(buf) - 1) < 0)
+               if (readlink(linkpath, buf, sizeof(buf) - 1) < 0) {
+                       free(linkpath);
                        return(-1);
+               }
+               free(linkpath);
                p = buf;
-       } else {
-               p = path;
        }
 
        /*
@@ -354,7 +376,7 @@ void
 hammer_cmd_pseudofs_destroy(char **av, int ac)
 {
        struct hammer_ioc_pseudofs_rw pfs;
-       struct stat st;
+       char *linkpath;
        int fd;
        int i;
 
@@ -405,13 +427,15 @@ hammer_cmd_pseudofs_destroy(char **av, int ac)
         */
        if (ioctl(fd, HAMMERIOC_RMR_PSEUDOFS, &pfs) == 0) {
                printf("pfs-destroy of PFS#%d succeeded!\n", pfs.pfs_id);
-               if (lstat(av[0], &st) == 0 && S_ISLNK(st.st_mode)) {
-                       if (remove(av[0]) < 0) {
-                               fprintf(stderr, "Unable to remove softlink: %s "
-                                       "(but the PFS has been destroyed)\n",
-                                       av[0]);
-                               /* exit status 0 anyway */
+               linkpath = getlink(av[0]);
+               if (linkpath) {
+                       if (remove(linkpath) < 0) {
+                               fprintf(stderr,
+                                       "Unable to remove softlink %s: %s\n",
+                                       linkpath, strerror(errno));
+                                       /* exit status 0 anyway */
                        }
+                       free(linkpath);
                }
        } else {
                printf("pfs-destroy of PFS#%d failed: %s\n",
index ef4ce77..a9ad333 100644 (file)
@@ -144,7 +144,7 @@ void hammer_reset_cycle(void);
 
 void clrpfs(struct hammer_ioc_pseudofs_rw *pfs, hammer_pseudofs_data_t pfsd,
        int pfs_id);
-int getpfs(struct hammer_ioc_pseudofs_rw *pfs, char *path);
+int getpfs(struct hammer_ioc_pseudofs_rw *pfs, const char *path);
 void relpfs(int fd, struct hammer_ioc_pseudofs_rw *pfs);
 void dump_pfsd(hammer_pseudofs_data_t, int);
 int hammer_softprune_testdir(const char *dirname);