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

[Gao feng]

On 10/07/2013 11:44 AM, Gao feng wrote:
> 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.
> 

So I think you can remove the check of access(dstpath, R_OK). if securityfs is supported,
virFileMakePath must be successful.

the reason fail to create directory securityfs is creating files is unsupported by sysfs.