> -----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