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

[Hu Tao]

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 at 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 at 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