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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> -----Original Message-----
> From: cardoe@xxxxxxxxxx [mailto:cardoe@xxxxxxxxxx] On Behalf Of Doug Goldstein
> Sent: Wednesday, September 25, 2013 6:26 PM
> To: Purcareata Bogdan-B43198
> Cc: libvir-list
> Subject: Re:  [PATCH] LXC: Detect fs support. Mount only supported fs in containers
> 
> On Wed, Sep 25, 2013 at 5:49 AM, Bogdan Purcareata
> <bogdan.purcareata@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
> > ---
> >  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@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@xxxxxxxxxx
> > 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


--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]