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

[Osier Yang]

On 2012年12月14日 21:23, Eric Blake wrote:
> On 12/13/2012 12:05 PM, 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
>
> You generally want both values in one go; calling stat() twice because
> you have two functions is not only a waste, but a racy window (if
> someone is modifying the pathname in the meantime).

Okay, agreed.

>
>
>> +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) {
>
> This is hard-coded to probe the actual kernel.  If you instead make it
> use a configurable prefix, then we could default to the kernel path, but
> also allow our testsuite to pass in a prefix from the testsuite, so that
> we can test this functionality even on kernels that don't support the
> feature (similar to how we have tests/nodeinfodata for faked cpu and
> node information).  I'm not yet sure whether we'll need to fake this
> information in any of our tests, but it's food for thought.
>> +int
>> +virSetDeviceUnprivSGIO(const char *path,
>> +                       int unpriv_sgio)
>
> Is this value only ever going to be 0 or 1?

Not sure, and as far as I get from the kernel patches thread,
I think it possibly could be other values too.

> If so, a bool might be more
> appropriate.

Returning int here doesn't here anway, so I'd keep it in case
of new values for unpriv_sgio.

>
>> +{
>> +    char *sysfs_path = NULL;
>> +    char *val = NULL;
>> +    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 (virAsprintf(&val, "%d", unpriv_sgio)<  0) {
>
> If indeed this is bool, then you could avoid the virAsprintf,
>
>> +        virReportOOMError();
>> +        goto cleanup;
>> +    }
>> +
>> +    if ((rc = virFileWriteStr(sysfs_path, val, 0))<  0) {
>
> and instead write a single '0' or '1' with less malloc pressure.
>
>> +        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)
>
> Again, to we expect the kernel to ever return more than 0 or 1?

Yes, I'd like keep it unchanged.

> Would
> this interface be any simpler as:
>
> /* -1 for error, 0 for off, 1 for on */
> int virGetDeviceUnprivSGIO(const char *path)
>