-----Original Message----- From: cardoe@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@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@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@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@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