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

[Jiri Denemark]

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.

> +#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.

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

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

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

Jirka