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: > Signed-off-by: Hu Tao <hutao(a)cn.fujitsu.com&gt; > --- > daemon/btrfs.c | 104 +++++++++++++++++++++++++++++++-------------------------- > 1 file changed, 56 insertions(+), 48 deletions(-) > > diff --git a/daemon/btrfs.c b/daemon/btrfs.c > index 754fdcd..2e9859d 100644 > --- a/daemon/btrfs.c > +++ b/daemon/btrfs.c > @@ -326,6 +326,56 @@ do_btrfs_subvolume_create (const char *dest, const char *qgroupid) > return 0; > } > > +static char > +*mount (const mountable_t *fs) > +{ > + char *fs_buf = NULL; 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. > + if (fs->type == MOUNTABLE_PATH) { > + return sysroot_path (fs->device); There's a whole chunk of code missing in here, starting with:

> - if (fs_buf == NULL) { > - reply_with_perror ("malloc"); > - > - cmderror: 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_buf == NULL) { > - reply_with_perror ("malloc"); > - > - cmderror: > - 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. > + return NULL; > > ADD_ARG (argv, i, str_btrfs); > ADD_ARG (argv, i, "subvolume"); > @@ -382,18 +400,8 @@ do_btrfs_subvolume_list (const mountable_t *fs) > CLEANUP_FREE char *out = NULL, *errout = NULL; > int r = commandv (&out, &errout, argv); > > - 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"); > - goto cmderror; > - } > - > - if (rmdir (fs_buf) == -1 && errno != ENOENT) { > - reply_with_error ("rmdir: %m\n"); > - goto cmderror; > - } > - } > + if (umount (fs_buf, fs) != 0) > + return NULL; > > if (r == -1) { > CLEANUP_FREE char *fs_desc = mountable_to_string (fs); > @@ -401,7 +409,7 @@ do_btrfs_subvolume_list (const mountable_t *fs) > fprintf (stderr, "malloc: %m"); > } > reply_with_error ("%s: %s", fs_desc ? fs_desc : "malloc", errout); > - goto cmderror; > + return NULL; > } > > lines = split_lines (out); > -- > 1.9.3 > > _______________________________________________ > Libguestfs mailing list > Libguestfs(a)redhat.com > https://www.redhat.com/mailman/listinfo/libguestfs -- 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