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

[Eric Blake]

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


> +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?  If so, a bool might be more
appropriate.

> +{
> +    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?  Would
this interface be any simpler as:

/* -1 for error, 0 for off, 1 for on */
int virGetDeviceUnprivSGIO(const char *path)

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 619 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20121214/c8cf4c25/attachment-0001.sig>