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