Re: [Libguestfs] [PATCH v2 01/11] daemon: btrfs: add helper functions mount and umount

On Thu, Dec 11, 2014 at 02:11:29PM +0800, Hu Tao wrote: You shouldn't initialize fs_buf to NULL here, since it prevents the compiler from detecting uninitialized use of fs_buf. The reason it was initialized to NULL in the previous code was because it was a CLEANUP_FREE function (and those have to be initialized almost always). You have correctly removed CLEANUP_FREE, but not the initialization of NULL. There's a whole chunk of code missing in here, starting with: It seems like it is still necessary to me. > + } else { > + fs_buf = strdup ("/tmp/btrfs.XXXXXX"); > + if (fs_buf == NULL) { > + fprintf (stderr, "strdup\n"); > + return NULL; > + } > + > + if (mkdtemp (fs_buf) == NULL) { > + fprintf (stderr, "mkdtemp: %s\n", fs_buf); > + free (fs_buf); > + return NULL; > + } > + > + if (mount_vfs_nochroot ("", NULL, fs, fs_buf, "<internal>") == -1) { > + if (rmdir (fs_buf) == -1 && errno != ENOENT) > + fprintf (stderr, "rmdir: %m\n"); > + free (fs_buf); > + return NULL; > + } > + } > + > + return fs_buf; > +} > + > +static int > +umount (char *fs_buf, const mountable_t *fs) > +{ > + if (fs->type != MOUNTABLE_PATH) { > + CLEANUP_FREE char *err = NULL; > + if (command (NULL, &err, str_umount, fs_buf, NULL) == -1) { > + reply_with_error ("%s", err ? err : "malloc"); > + return -1; > + } > + > + if (rmdir (fs_buf) == -1 && errno != ENOENT) { > + reply_with_error ("rmdir: %m\n"); > + return -1; > + } > + } > + free (fs_buf); > + return 0; > +} > + > guestfs_int_btrfssubvolume_list * > do_btrfs_subvolume_list (const mountable_t *fs) > { > @@ -336,42 +386,10 @@ do_btrfs_subvolume_list (const mountable_t *fs) > > /* Execute 'btrfs subvolume list <fs>', and split the output into lines */ > { > - CLEANUP_FREE char *fs_buf = NULL; > - > - if (fs->type == MOUNTABLE_PATH) { > - fs_buf = sysroot_path (fs->device); > - if (fs->type != MOUNTABLE_PATH && fs_buf) { > - CLEANUP_FREE char *err = NULL; > - if (command (NULL, &err, str_umount, fs_buf, NULL) == -1) > - fprintf (stderr, "%s\n", err); > - > - if (rmdir (fs_buf) == -1 && errno != ENOENT) > - fprintf (stderr, "rmdir: %m\n"); > - } > - return NULL; > - } > - } > - > - else { > - fs_buf = strdup ("/tmp/btrfs.XXXXXX"); > - if (fs_buf == NULL) { > - reply_with_perror ("strdup"); > - goto cmderror; > - } > + char *fs_buf = mount (fs); > > - if (mkdtemp (fs_buf) == NULL) { > - reply_with_perror ("mkdtemp"); > - goto cmderror; > - } > - > - if (mount_vfs_nochroot ("", NULL, fs, fs_buf, "<internal>") == -1) { > - goto cmderror; > - } > - } > + if (!fs) Did you mean if (!fs_buf) ? Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into KVM guests. http://libguestfs.org/virt-v2v