On 04/08/2013 09:12 AM, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange" <berrange@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> ---
> 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
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list