Re: [libvirt] [PATCH] LXC: Detect fs support. Mount only supported fs in containers

-----Original Message----- From: cardoe(a)cardoe.com [mailto:cardoe@cardoe.com] On Behalf Of Doug Goldstein Sent: Wednesday, September 25, 2013 6:26 PM To: Purcareata Bogdan-B43198 Cc: libvir-list Subject: Re: [libvirt] [PATCH] LXC: Detect fs support. Mount only supported fs in containers On Wed, Sep 25, 2013 at 5:49 AM, Bogdan Purcareata <bogdan.purcareata(a)freescale.com&gt; wrote: > Some filesystems - specifically securityfs - are not supported in > all systems running libvirt containers. When starting a container, > mount only the filesystems that are supported on the host. Detection > of filesystem support is done at runtime. Might be worth noting this behavior is since 6807238d87fd93dee30 > > Signed-off-by: Bogdan Purcareata <bogdan.purcareata(a)freescale.com&gt; > --- > src/lxc/lxc_container.c | 61 +++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 61 insertions(+) > > diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c > index c60f5d8..eff9a24 100644 > --- a/src/lxc/lxc_container.c > +++ b/src/lxc/lxc_container.c > @@ -509,6 +509,61 @@ static int lxcContainerChildMountSort(const void *a, const void *b) > # define MS_SLAVE (1<<19) > #endif > > +/* > + * This function attempts to detect kernel support > + * for a specific filesystem type. This is done by > + * inspecting /proc/filesystems. > + */ > +static int lxcCheckFSSupport(const char *fs_type) > +{ > + FILE *fp = NULL; > + int ret = -1; So your error case here is -1, which is good which we typically use. > + const char *fslist = "/proc/filesystems"; > + char *line = NULL; > + const char *type; > + > + if (!fs_type) > + return 1; .... but here its 1. You appear to use 1 as the success case below so this conflicts.

> + > + VIR_DEBUG("Checking kernel support for %s", fs_type); > + > + VIR_DEBUG("Open %s", fslist); Can we combine these two debugs?

> + if (!(fp = fopen(fslist, "r"))) { > + if (errno == ENOENT) It seems like you had another line after this if check that went away since the line below is not indented properly. Since we always want to print the error message you probably want to drop this if check

> + > + virReportSystemError(errno, > + _("Unable to read %s"), > + fslist); > + goto cleanup; > + } > + > + while (!feof(fp)) { > + size_t n; > + VIR_FREE(line); > + if (getline(&line, &n, fp) <= 0) { > + if (feof(fp)) > + break; This could really be optimized with: while (getline(&line, &n, fp) > 0) { .....do work here.... } if (ferror(fp)) { .... handle error ... } And we wouldn't have to have nested error checking.

> + > + goto cleanup; > + } > + > + type = strstr(line, fs_type); So I dislike using strstr() here because it'll be a greedy match. e.g. should you check for "fuse", it will also match "fuseblk" and "fusectl". Or check for "nfs" and it'll match on "nfs4".

> + if (type) { > + ret = 1; So here's the success case that it was found and its set to 1. > + goto cleanup; > + } > + } > + > + VIR_DEBUG("No kernel support for %s", fs_type); > + > + ret = 0; > + > +cleanup: > + VIR_FREE(line); > + VIR_FORCE_FCLOSE(fp); > + return ret; > +} > + > static int lxcContainerGetSubtree(const char *prefix, > char ***mountsret, > size_t *nmountsret) > @@ -855,17 +910,23 @@ static int lxcContainerMountBasicFS(bool userns_enabled) > for (i = 0; i < ARRAY_CARDINALITY(lxcBasicMounts); i++) { > virLXCBasicMountInfo const *mnt = &lxcBasicMounts[i]; > const char *srcpath = NULL; > + const char *dstpath = NULL; > > VIR_DEBUG("Processing %s -> %s", > mnt->src, mnt->dst); > > srcpath = mnt->src; > + dstpath = mnt->dst; > > /* Skip if mount doesn't exist in source */ > if ((srcpath[0] == '/') && > (access(srcpath, R_OK) < 0)) > continue; > > + if ((access(dstpath, R_OK) < 0) || /* mount is not present on host */ So I understand you are doing this for securityfs. But there's other cases when the filesystem might not be mounted on the host but we want to mount it in the container so I could see this check wrecking havoc with that. I could also be wrong.

> + (!lxcCheckFSSupport(mnt->type))) /* no fs support in kernel */ > + continue; > + > #if WITH_SELINUX > if (STREQ(mnt->src, SELINUX_MOUNT) && > (!is_selinux_enabled() || userns_enabled)) > -- > 1.7.11.7 > > > -- > libvir-list mailing list > libvir-list(a)redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list Hopefully some of the comments above help. Thanks for fixing this since I too had a kernel running without securityfs and hit this.

-- Doug Goldstein