[Libguestfs] [PATCH 06/12] mount: Add mount_vfs_internal and umount_internal

[Richard W.M. Jones]

On Thu, Feb 07, 2013 at 03:57:52PM +0000, Matthew Booth wrote:
> These 2 internal functions allow mounting and unmounting of mountables
> outside /sysroot.

There seems to be some variable movement in this patch which confuses
things.  Variable placement should be left alone in unrelated patches.

>  daemon/daemon.h |   8 +++++
>  daemon/mount.c  | 103 +++++++++++++++++++++++++++++++++++++++-----------------
>  2 files changed, 81 insertions(+), 30 deletions(-)
> 
> diff --git a/daemon/daemon.h b/daemon/daemon.h
> index a94c338..78591a0 100644
> --- a/daemon/daemon.h
> +++ b/daemon/daemon.h
> @@ -65,6 +65,14 @@ extern int xread (int sock, void *buf, size_t len)
>  
>  extern char *mountable_to_string (const mountable_t *mountable);
>  
> +/*-- in mount.c --*/
> +
> +extern int mount_vfs_internal (const char *options, const char *vfstype,
> +                               const mountable_t *mountable,
> +                               const char *mp, const char *user_mp, char **err);
> +extern int umount_internal (const char *pathordevice,
> +                            int force, int lazyunmount, char **err);
> +
>  /* Growable strings buffer. */
>  struct stringsbuf {
>    char **argv;
> diff --git a/daemon/mount.c b/daemon/mount.c
> index 484e45c..34510bd 100644
> --- a/daemon/mount.c
> +++ b/daemon/mount.c
> @@ -126,31 +126,44 @@ int
>  do_mount_vfs (const char *options, const char *vfstype,
>                const mountable_t *mountable, const char *mountpoint)
>  {
> -  int r;
> -  CLEANUP_FREE char *mp = NULL;
> -  CLEANUP_FREE char *error = NULL;
> -  struct stat statbuf;
> -
>    ABS_PATH (mountpoint, , return -1);
>  
> -  mp = sysroot_path (mountpoint);
> +  CLEANUP_FREE char *mp = sysroot_path (mountpoint);
>    if (!mp) {
>      reply_with_perror ("malloc");
>      return -1;
>    }
>  
>    /* Check the mountpoint exists and is a directory. */
> +  struct stat statbuf;
>    if (stat (mp, &statbuf) == -1) {
>      reply_with_perror ("mount: %s", mountpoint);
>      return -1;
>    }
> +
>    if (!S_ISDIR (statbuf.st_mode)) {
>      reply_with_perror ("mount: %s: mount point is not a directory", mountpoint);
>      return -1;
>    }
>  
> +  CLEANUP_FREE char *err = NULL;
> +  int r = mount_vfs_internal (options, vfstype, mountable,
> +                              mp, mountpoint, &err);
> +
> +  if (r == -1) {
> +    reply_with_error ("%s", err ? err : "malloc");
> +  }
> +
> +  return r;
> +}
> +
> +int
> +mount_vfs_internal (const char *options, const char *vfstype,
> +                    const mountable_t *mountable,
> +                    const char *mp, const char *user_mp,
> +                    char **err)
> +{
>    CLEANUP_FREE char *options_plus = NULL;
> -  const char *device = mountable->device;
>    switch (mountable->type) {
>      case MOUNTABLE_DEVICE:
>        break;
> @@ -160,24 +173,34 @@ do_mount_vfs (const char *options, const char *vfstype,
>          if (asprintf (&options_plus, "subvol=%s,%s",
>                        mountable->volume, options) == -1)
>          {
> -          reply_with_perror ("asprintf");
> +          /* No point trying to allocate more memory for an error message */
> +          fprintf (stderr, "asprintf: %m\n");
>            return -1;

This may or may not be a good thing, but it shouldn't be
a part of this patch.

On a separate point, just because asprintf fails doesn't mean that
reply_with_perror would fail.  asprintf might have failed because
'options' was somehow an unterminated string (ie. it requested a huge
amount of memory), not because there's no memory left.

>          }
>        }
>        
>        else {
>          if (asprintf (&options_plus, "subvol=%s", mountable->volume) == -1) {
> -          reply_with_perror ("asprintf");
> +          /* No point trying to allocate more memory for an error message */
> +          fprintf (stderr, "asprintf: %m\n");
>            return -1;

Ditto.

>        }
>        break;
>  
>      default:
> -      reply_with_error ("Invalid mountable type: %i", mountable->type);
> +      if (asprintf (err, "Invalid mountable type: %i", mountable->type) == -1)
> +      {
> +          /* No point trying to allocate more memory for an error message */
> +          fprintf (stderr, "Invalid mountable type: %i", mountable->type);
> +          fprintf (stderr, "asprintf: %m\n");
> +      }
>        return -1;
>    }

Ditto.

I didn't review further.  I think this patch needs reworking.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top