[libvirt] [PATCH v2] LXC: Detect fs support. Mount only supported filesystems

[Gao feng]

On 10/04/2013 07:33 PM, Purcareata Bogdan-B43198 wrote:

>>> +/*
>>> + * 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;
>>> +    const char *fslist = "/proc/filesystems";
>>> +    char *line = NULL;
>>> +    char *type;
>>> +    size_t n;
>>> +
>>> +    /* there should be no problem mounting an entry
>>> +     * with NULL fs type, hence NULL fs types are
>>> +     * supported */
>>> +    if (!fs_type) {
>>> +	ret = 1;
>>> +	goto out;
>>> +    }
>>> +
>>> +    VIR_DEBUG("Checking kernel support for %s in %s", fs_type, fslist);
>>> +
>>> +    if (!(fp = fopen(fslist, "r"))) {
>>
>> I don't know if we can open /proc/filesystems successfully here if container
>> shares
>> root directory with host, since the /proc filesystem has been unmounted in
>> lxcContainerUnmountForSharedRoot.
> 
> Right. I just noticed the search for "proc" fails, since /proc/filesystem requires procfs to be mounted. (Un)fortunately, my handling of lxcCheckFSSupport() bypassed this error, and mounted procfs anyways. I will update the code with a proper handle for the error code. I just don't see how I can handle all filesystem entries in an uniform manner, since each one is so special.
> 
>>

So save the supported filesystem list before we unmount the proc filesystem, and in lxcCheckFSSupport
use this list to check if the filesystem is supported by kernel.

btw it's better to return the error of fopen to user.

>>> +        virReportSystemError(errno,
>>> +                             _("Unable to read %s"),
>>> +                             fslist);
>>> +        goto out;
>>> +    }
>>> +
>>> +    while(getline(&line, &n, fp) > 0) {
>>> +	type = strstr(line, fs_type);
>>> +
>>> +	if (!type)
>>> +		continue;
>>> +
>>> +	if (!strncmp(type, fs_type, strlen(type))) {
>>
>> The strncmp() function compares the only first (at most) n bytes of s1 and s2.
>> please use STREQ here.
> 
> Thanks, I will update.
> 
>>
>>> +		ret = 1;
>>> +		goto cleanup;
>>> +	}
>>> +    }
>>> +
>>> +    if (ferror(fp)) {
>>> +	virReportSystemError(errno,
>>> +                             _("Error reading line from %s"),
>>> +                             fslist);
>>> +        goto cleanup;
>>> +    }
>>> +
>>> +    VIR_DEBUG("No kernel support for %s", fs_type);
>>> +
>>> +    ret = 0;
>>> +
>>
>> You set ret to 0 here, so the return value 0 means this filesystem
>> is unsupported by kernel, right? what the meaning of return value -1?
>>
>> you return -1 when ferror(fp) is true.
> 
> So I thought it would be like this:
> - -1 - error encountered
> - 0 - no error, no kernel support for the filesystem
> - 1 - no error, kernel support present
> 
>>
>>> +cleanup:
>>> +    VIR_FREE(line);
>>> +    VIR_FORCE_FCLOSE(fp);
>>> +out:
>>> +    return ret;
>>> +}
>>> +
>>>  static int lxcContainerGetSubtree(const char *prefix,
>>>                                    char ***mountsret,
>>>                                    size_t *nmountsret)
>>> @@ -789,17 +850,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 */
>>> +	    (!lxcCheckFSSupport(mnt->type))) /* no fs support in kernel */
>>> +		continue;
>>> +
>>
>> The access is in the incorrect place, it should be called after we create mnt-
>>> dst.
>> so Move this check after virFileMakePath(mnt->dst).
> 
> My specific problem was that mounting security failed even before reaching the actual mount syscall. 
> 
> It failed when doing virFileMakePath("/sys/kernel/securityfs"), because /sys is previously mounted read only (I realized this just now).
> 
> root at p4080ds:/sys/kernel# mkdir securityfs
> mkdir: cannot create directory 'securityfs': No such file or directory
> 

I don't know how this occurred, since the directory securityfs is created when you mount sysfs.
Actually virFileMakePath will not create securityfs directory since it already exists.

Thanks