[libvirt] [PATCH 1/8] util: Prepare helpers for unpriv_sgio setting

[Osier Yang]

On 2012年12月14日 21:13, Jiri Denemark wrote:
> On Fri, Dec 14, 2012 at 03:05:36 +0800, Osier Yang wrote:
>> "virGetDevice{Major,Minor}" could be used across the sources,
>> but it doesn't relate with this series, and could be done later.
>>
>> * src/util/util.h: (Declare virGetDevice{Major,Minor}, and
>>                      vir{Get,Set}DeviceUnprivSGIO)
>> * src/util/util.c: (New internal helper virCanonicalizeDiskPath to
>>                      canonicalize the disk path; Implement
>>                      virGetDevice{Major,Minor} and
>>                      vir{Get,Set}DeviceUnprivSGIO)
>> * src/libvirt_private.syms: Export private symbols of upper helpers
>> ---
>>   src/libvirt_private.syms |    4 +
>>   src/util/util.c          |  180 ++++++++++++++++++++++++++++++++++++++++++++++
>>   src/util/util.h          |    8 ++
>>   3 files changed, 192 insertions(+), 0 deletions(-)
> ...
>> diff --git a/src/util/util.c b/src/util/util.c
>> index 05e7ca7..8c6d43c 100644
>> --- a/src/util/util.c
>> +++ b/src/util/util.c
>> @@ -3129,3 +3129,183 @@ virStrIsPrint(const char *str)
>>
>>       return true;
>>   }
>> +
>> +static char *
>> +virCanonicalizeDiskPath(const char *path)
>> +{
>> +    char *canonical_path = NULL;
>> +
>> +    if (STRPREFIX(path, "/dev/disk/by-path") ||
>> +        STRPREFIX(path, "/dev/disk/by-uuid") ||
>> +        STRPREFIX(path, "/dev/disk/by-id")) {
>> +        if (virFileResolveLink(path,&canonical_path)<  0)
>> +            return NULL;
>> +    } else {
>> +        canonical_path = strdup(path);
>> +    }
>> +
>> +    return canonical_path;
>> +}
>
> There's no reason to restrict virFileResolveLink only to disk paths that
> start with /dev/disk/by-*. Symlinks to devices can be anywhere; just
> call it unconditionally.
>
>> +
>> +#if defined(major)&&  defined(minor)
>> +int
>> +virGetDeviceMajor(const char *path)
>> +{
>> +    struct stat sb;
>> +    char *canonical_path = NULL;
>> +
>> +    if (!(canonical_path = virCanonicalizeDiskPath(path)))
>> +        return -errno;
>> +
>> +    if (stat(canonical_path,&sb)<  0) {
>> +        VIR_FREE(canonical_path);
>> +        return -errno;
>> +    }
>> +
>> +    if (!S_ISBLK(sb.st_mode)) {
>> +        VIR_FREE(canonical_path);
>> +        return -EINVAL;
>> +    }
>> +
>> +    VIR_FREE(canonical_path);
>> +    return major(sb.st_rdev);
>> +}
>> +
>> +int
>> +virGetDeviceMinor(const char *path)
>> +{
>> +    struct stat sb;
>> +    char *canonical_path = NULL;
>> +
>> +    if (!(canonical_path = virCanonicalizeDiskPath(path)))
>> +        return -errno;
>> +
>> +    if (stat(canonical_path,&sb)<  0) {
>> +        VIR_FREE(canonical_path);
>> +        return -errno;
>> +    }
>> +
>> +    if (!S_ISBLK(sb.st_mode)) {
>> +        VIR_FREE(canonical_path);
>> +        return -EINVAL;
>> +    }
>> +
>> +    VIR_FREE(canonical_path);
>> +    return minor(sb.st_rdev);
>> +}
>
> Oh, this is horrible. Why did you create two functions for getting major
> and minor numbers? Creating
>
>      int virGetDeviceID(const char *path, int *major, int *minor)
>
> that would extract both at one go (since you want to get both of them
> anyway and if not, you can allow NULL to be passed for either) seems to
> be a better approach. And while at it, you can just remove the
> virCanonicalizeDiskPath helper and call virFileResolveLink directly in
> virGetDeviceID.

Okay, it's indeed for getting only one of them purpose. It won't
be only used by this patch set. But yes, I agreed with using
one function, for the potential race pointed by Eric.

>
>> +#else
>> +int
>> +virGetDeviceMajor(const char *path)
>> +{
>> +    return -ENOSYS;
>> +}
>> +
>> +int
>> +virGetDeviceMinor(const char *path)
>> +{
>> +    return -ENOSYS;
>> +}
>> +#endif
>> +
>> +static char *
>> +virGetUnprivSGIOSysfsPath(const char *path)
>> +{
>> +    int major, minor;
>> +    char *sysfs_path = NULL;
>> +
>> +    if ((major = virGetDeviceMajor(path))<  0) {
>> +        virReportSystemError(-major,
>> +                             _("Unable to get major number of device '%s'"),
>> +                             path);
>> +        return NULL;
>> +    }
>> +
>> +    if ((minor = virGetDeviceMinor(path))<  0) {
>> +        virReportSystemError(-minor,
>> +                             _("Unable to get minor number of device '%s'"),
>> +                             path);
>> +        return NULL;
>> +    }
>> +
>> +    if (virAsprintf(&sysfs_path, "/sys/dev/block/%d:%d/queue/unpriv_sgio",
>> +                    major, minor)<  0) {
>> +        virReportOOMError();
>> +        return NULL;
>> +    }
>> +
>> +    return sysfs_path;
>> +}
>> +
>> +int
>> +virSetDeviceUnprivSGIO(const char *path,
>> +                       int unpriv_sgio)
>> +{
>> +    char *sysfs_path = NULL;
>> +    char *val = NULL;
>> +    int ret = -1;
>> +    int rc = -1;
>
> Nit: no need to initalize rc.

Okay.

>
>> +
>> +    if (!(sysfs_path = virGetUnprivSGIOSysfsPath(path)))
>> +        return -1;
>> +
>> +    if (!virFileExists(sysfs_path)) {
>> +        virReportError(VIR_ERR_OPERATION_INVALID, "%s",
>> +                       _("unpriv_sgio is not supported by this kernel"));
>> +        goto cleanup;
>> +    }
>> +
>> +    if (virAsprintf(&val, "%d", unpriv_sgio)<  0) {
>> +        virReportOOMError();
>> +        goto cleanup;
>> +    }
>> +
>> +    if ((rc = virFileWriteStr(sysfs_path, val, 0))<  0) {
>> +        virReportSystemError(-rc, _("failed to set %s"), sysfs_path);
>> +        goto cleanup;
>> +    }
>> +
>> +    ret = 0;
>> +cleanup:
>> +    VIR_FREE(sysfs_path);
>> +    VIR_FREE(val);
>> +    return ret;
>> +}
>> +
>> +int
>> +virGetDeviceUnprivSGIO(const char *path,
>> +                       int *unpriv_sgio)
>> +{
>> +    char *sysfs_path = NULL;
>> +    char *buf = NULL;
>> +    char *tmp = NULL;
>
> Nit: sysfs_path and tmp do not really need be initialized.

That's just personal habit.

>
>> +    int ret = -1;
>> +    int rc = -1;
>> +
>> +    if (!(sysfs_path = virGetUnprivSGIOSysfsPath(path)))
>> +        return -1;
>> +
>> +    if (!virFileExists(sysfs_path)) {
>> +        virReportError(VIR_ERR_OPERATION_INVALID, "%s",
>> +                       _("unpriv_sgio is not supported by this kernel"));
>> +        goto cleanup;
>> +    }
>> +
>> +    if (virFileReadAll(sysfs_path, 1024,&buf)<  0)
>> +        goto cleanup;
>> +
>> +    if ((tmp = strchr(buf, '\n')))
>> +        *tmp = '\0';
>> +
>> +    if ((rc = virStrToLong_i(buf, NULL, 10,
>> +                             unpriv_sgio))<  0) {
>> +        virReportError(VIR_ERR_INTERNAL_ERROR,
>> +                       _("failed to parse value of %s"), sysfs_path);
>> +        goto cleanup;
>> +    }
>
> rc is completely useless in this function.

Okay.

>
>> +    ret = 0;
>> +cleanup:
>> +    VIR_FREE(sysfs_path);
>> +    VIR_FREE(buf);
>> +    return ret;
>> +}
>
> Jirka