On Thu, Dec 11, 2014 at 02:45:18PM +0000, Richard W.M. Jones wrote:
On Thu, Dec 11, 2014 at 02:11:29PM +0800, Hu Tao wrote:
Signed-off-by: Hu Tao <hutao@cn.fujitsu.com> --- 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.
Okay.
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:
Agreed.
- 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");
Meanwhile, I changed fprintf (stderr,...) to reply_error().
+ 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) ?
Yes! Thank you for review! Regards, Hu
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@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