[libvirt] [PATCH] Unmount existing filesystems under user specified mounts in LXC

[Eric Blake]

On 04/08/2013 09:12 AM, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange" <berrange at redhat.com>
> 
> If the user requests a mount for /run, this may hide any existing
> mounts that are lower down in /run. The result is that the
> container still sees the mounts in /proc/mounts, but cannot
> access them
> 
> sh-4.2# df
> df: '/run/user/501/gvfs': No such file or directory
> df: '/run/media/berrange/LIVE': No such file or directory
> df: '/run/media/berrange/SecureDiskA1': No such file or directory
> df: '/run/libvirt/lxc/sandbox': No such file or directory
> Filesystem                      1K-blocks      Used Available Use% Mounted on
> /dev/mapper/vg_t500wlan-lv_root 151476396 135390200   8384900  95% /
> tmpfs                             1970888      3204   1967684   1% /run
> /dev/sda1                          194241    155940     28061  85% /boot
> devfs                                  64         0        64   0% /dev
> tmpfs                                  64         0        64   0% /sys/fs/cgroup
> tmpfs                             1970888      1200   1969688   1% /etc/libvirt-sandbox/scratch
> 
> Before mounting any filesystem at a particular location, we
> must recursively unmount anything at or below the target mount
> point

Makes sense.

> 
> Signed-off-by: Daniel P. Berrange <berrange at redhat.com>
> ---
>  src/lxc/lxc_container.c | 218 ++++++++++++++++++++++++------------------------
>  1 file changed, 111 insertions(+), 107 deletions(-)
> 
>  
> +static int lxcContainerGetSubtree(const char *prefix,
> +                                  char ***mountsret,
> +                                  size_t *nmountsret)

We aren't very consistent on the style:

static int
lxcContainerGetSubtree(...

> +{
> +    FILE *procmnt;
> +    struct mntent mntent;
> +    char mntbuf[1024];

fixed-width buffer...

> +    int ret = -1;
> +    char **mounts = NULL;
> +    size_t nmounts = 0;
> +
> +    VIR_DEBUG("prefix=%s", prefix);
> +
> +    *mountsret = NULL;
> +    *nmountsret = 0;
> +
> +    if (!(procmnt = setmntent("/proc/mounts", "r"))) {
> +        virReportSystemError(errno, "%s",
> +                             _("Failed to read /proc/mounts"));
> +        return -1;
> +    }
> +
> +    while (getmntent_r(procmnt, &mntent, mntbuf, sizeof(mntbuf)) != NULL) {

...and getmntent_r can return ERANGE if it is too small (where it is
definitely feasible that the user could supply a mount point larger than
1024) [well, I assume an ERANGE failure, even though the man page is
buggy for failing to mention errno values on failure, based on
similarity to other functions that fail with ERANGE when given a
too-small buffer]...

> +        VIR_DEBUG("Got %s", mntent.mnt_dir);
> +        if (!STRPREFIX(mntent.mnt_dir, prefix))
> +            continue;
> +
> +        if (VIR_REALLOC_N(mounts, nmounts+1) < 0) {

Is VIR_EXPAND or VIR_RESIZE better than VIR_REALLOC_N?

> +            virReportOOMError();

Another place that will conflict with OOM overhaul :)

> +            goto cleanup;
> +        }
> +        if (!(mounts[nmounts] = strdup(mntent.mnt_dir))) {
> +            virReportOOMError();
> +            goto cleanup;
> +        }
> +        nmounts++;
> +        VIR_DEBUG("Grabbed %s", mntent.mnt_dir);
> +    }

...but you aren't checking errno or allowing for the possibility of
needing to enlarge the buffer.

> +
> +    if (mounts)
> +        qsort(mounts, nmounts, sizeof(mounts[0]),
> +              lxcContainerChildMountSort);
> +
> +    ret = 0;
> +cleanup:
> +    *mountsret = mounts;
> +    *nmountsret = nmounts;
> +    endmntent(procmnt);
> +    return ret;
> +}
> +
> -static int lxcContainerUnmountSubtree(const char *prefix,
> -                                      bool isOldRootFS)
> -{

Looks like this patch is a bit of code shuffling coupled with some
tweaks; would it be better if it were split into two parts so that the
code shuffling is done with no semantic change, making it easier for the
second patch to show only what changed?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 621 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20130408/7d5e8308/attachment-0001.sig>