[libvirt] [PATCH] LXC: Detect fs support. Mount only supported fs in containers
Purcareata Bogdan-B43198
B43198 at freescale.com
Tue Oct 1 11:01:54 UTC 2013
> -----Original Message-----
> From: cardoe at cardoe.com [mailto:cardoe at 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 at freescale.com> 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 at freescale.com>
> > ---
> > 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.
I want to consider this a success case as well - there shouldn't be a problem trying to mount an entry with type=NULL. Since the filesystem type is NULL, the kernel should support the mount. Is this judgement acceptable?
>
> > +
> > + VIR_DEBUG("Checking kernel support for %s", fs_type);
> > +
> > + VIR_DEBUG("Open %s", fslist);
>
> Can we combine these two debugs?
Yes. Will do.
>
> > + 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
Yes, copy/paste error. Will properly handle the error case.
>
> > +
> > + 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.
Thank you, will update.
>
> > +
> > + 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".
After the strstr(), I can also do a strncmp(type, fs_type, strlen(fs_type)), and that will make sure there is a complete match. Sorry I haven't thought of this caveat.
>
> > + 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.
Initially, I thought of this as well. But there seems to be a problem with virFileMakePath, since it is called either way, and it fails to make /sys/kernel/securityfs. I also tried to make it by hand, without any luck:
root at p4080ds:/sys/kernel# mkdir securityfs
mkdir: cannot create directory 'securityfs': No such file or directory
So I assumed it is best to mount it only if it's already present. How do you think I should handle this?
>
> > + (!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 at 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.
Thank you very much for your comments! Sorry for the delay, I missed the patch comments when I initially read this mail.
Bogdan P.
>
> --
> Doug Goldstein
More information about the libvir-list
mailing list