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

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)