Re: [libvirt] [PATCH v4 2/2] add interface for blkio.weight_device

On 10/12/2011 03:26 AM, Hu Tao wrote: I had to tweak this one to apply on top of my proposed changes to the other one; I'll repost the total changes as a v5. This should be squashed into 1/2. We should document what format that string takes. Yes, virsh should also document it, but not all clients go through virsh, so just copy the virsh listing here: /device/path,weight;/device/path,weight. Which means we should probably _also_ do some input validation, and reject /device/path names that contain ','. I don't like this pattern of creating vir*Free functions with multiple arguments. Either we should be freeing just one struct (single path/weight pair, caller iterates over the array to free it one struct at a time), or encapsulating the entire array of path/weight pairs, and the number of those pairs, into a single struct (caller frees a single struct with one parameter). And yes, I know we did it with virTypedParameterFree, at my suggestion. This is making me think all the more that we should update the name of that function, and push the free back to the caller. This should check for buffer error. The caller can't issue a good error message - it doesn't know if the failure was due to stat() failure or to OOM. You only have two callers, which both reported VIR_ERR_INTERNAL_ERROR, but I'm wondering if that should be improved - if the error is user-visible, we should try harder to give a more accurate message. But I didn't change that. This won't validate without edits to docs/schemas/domaincommon.rng. Also, we have to document the new XML in docs/formatdomain.html.in. We should split this patch into (at least) two parts - the public API and clients that react to that API (docs, include, virsh), and the qemu-specific implementation of the new public interface. Here, I'm thinking its better to have a for loop, freeing each weight_devices[i]. The for loop on ndevices renders this NULL check redundant. virBufferAddLit is faster, when it works. Must be escaped, in case it contains characters that would interfere with xml parsing. size_t for array length Typically, if we use 'name' for an array, then we use 'nname' for its length; meaning this should either be ndevices/devices, or nweight_devices/weight_devices. I prefer the short name 'device', not to mention that 'device_weight' reads better than 'weight_device' if we were to go with a longer name. 'make syntax-check' rejected this, since you didn't use any of the functions in that header. Make temp const char*, or remove const from weightDeviceStr, instead of casting away const. Memory leak - if you parse the first path, but the second strndup fails, then you leak result[0].path. Unused variable. I've run out of time to finish this review today, but so far, I'm squashing in at least this: diff --git i/src/conf/domain_conf.c w/src/conf/domain_conf.c index 215a46f..4a5aefc 100644 --- i/src/conf/domain_conf.c +++ w/src/conf/domain_conf.c @@ -629,6 +629,9 @@ int virBlkioWeightDeviceToStr(virBlkioWeightDevicePtr weightdevices, weightdevices[i].weight); } + if (virBufferError(&buf)) + return -1; + *result = virBufferContentAndReset(&buf); return 0; } @@ -6677,7 +6680,8 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, } for (i = 0; i < n; i++) { - virDomainBlkioWeightDeviceParseXML(nodes[i], &def->blkio.weight_devices[i]); + virDomainBlkioWeightDeviceParseXML(nodes[i], + &def->blkio.weight_devices[i]); } def->blkio.ndevices = n; VIR_FREE(nodes); @@ -10763,17 +10767,13 @@ virDomainDefFormatInternal(virDomainDefPtr def, virBufferAsprintf(buf, " <weight>%u</weight>\n", def->blkio.weight); - if (def->blkio.weight_devices) { - int i; - - for (i = 0; i < def->blkio.ndevices; i++) { - virBufferAsprintf(buf, " <device>\n"); - virBufferAsprintf(buf, " <path>%s</path>\n", - def->blkio.weight_devices[i].path); - virBufferAsprintf(buf, " <weight>%d</weight>\n", - def->blkio.weight_devices[i].weight); - virBufferAsprintf(buf, " </device>\n"); - } + for (n = 0; n < def->blkio.ndevices; n++) { + virBufferAddLit(buf, " <device>\n"); + virBufferEscapeString(buf, " <path>%s</path>\n", + def->blkio.weight_devices[n].path); + virBufferAsprintf(buf, " <weight>%d</weight>\n", + def->blkio.weight_devices[n].weight); + virBufferAddLit(buf, " </device>\n"); } virBufferAsprintf(buf, " </blkiotune>\n"); diff --git i/src/qemu/qemu_driver.c w/src/qemu/qemu_driver.c index 7060111..caf0615 100644 --- i/src/qemu/qemu_driver.c +++ w/src/qemu/qemu_driver.c @@ -89,7 +89,6 @@ #include "locking/lock_manager.h" #include "locking/domain_lock.h" #include "virkeycode.h" -#include "c-ctype.h" #define VIR_FROM_THIS VIR_FROM_QEMU @@ -5895,14 +5894,16 @@ cleanup: /* weightDeviceStr in the form of /device/path,weight;/device/path,weight * for example, /dev/disk/by-path/pci-0000:00:1f.2-scsi-0:0:0:0,800 */ -static int parseBlkioWeightDeviceStr(const char *weightDeviceStr, virBlkioWeightDevicePtr *dw, int *size) +static int +parseBlkioWeightDeviceStr(char *weightDeviceStr, + virBlkioWeightDevicePtr *dw, int *size) { char *temp; int nDevice = 0; int i; virBlkioWeightDevicePtr result = NULL; - temp = (char *)weightDeviceStr; + temp = weightDeviceStr; while (temp) { temp = strchr(temp, ';'); if (temp) { @@ -5920,7 +5921,7 @@ static int parseBlkioWeightDeviceStr(const char *weightDeviceStr, virBlkioWeight } i = 0; - temp = (char *)weightDeviceStr; + temp = weightDeviceStr; while (temp && i < nDevice) { char *p = temp; @@ -5988,8 +5989,8 @@ static int qemuDomainSetBlkioParameters(virDomainPtr dom, isActive = virDomainObjIsActive(vm); - typed_string = (flags & VIR_DOMAIN_TYPED_STRING_OKAY) == VIR_DOMAIN_TYPED_STRING_OKAY; - flags &= ~VIR_DOMAIN_TYPED_STRING_OKAY; + typed_string = (flags & VIR_TYPED_PARAM_STRING_OKAY) != 0; + flags &= ~VIR_TYPED_PARAM_STRING_OKAY; if (flags == VIR_DOMAIN_AFFECT_CURRENT) { if (isActive) flags = VIR_DOMAIN_AFFECT_LIVE; @@ -6187,7 +6188,7 @@ static int qemuDomainGetBlkioParameters(virDomainPtr dom, if ((*nparams) == 0) { /* Current number of blkio parameters supported by cgroups */ - if (flags & VIR_DOMAIN_TYPED_STRING_OKAY) + if (flags & VIR_TYPED_PARAM_STRING_OKAY) *nparams = QEMU_NB_BLKIO_PARAM; else *nparams = QEMU_NB_BLKIO_PARAM - 1; @@ -6195,7 +6196,7 @@ static int qemuDomainGetBlkioParameters(virDomainPtr dom, goto cleanup; } - if (flags & VIR_DOMAIN_TYPED_STRING_OKAY) { + if (flags & VIR_TYPED_PARAM_STRING_OKAY) { if ((*nparams) != QEMU_NB_BLKIO_PARAM) { qemuReportError(VIR_ERR_INVALID_ARG, "%s", _("Invalid parameter count")); @@ -6211,8 +6212,8 @@ static int qemuDomainGetBlkioParameters(virDomainPtr dom, isActive = virDomainObjIsActive(vm); - typed_string = (flags & VIR_DOMAIN_TYPED_STRING_OKAY) == VIR_DOMAIN_TYPED_STRING_OKAY; - flags &= ~VIR_DOMAIN_TYPED_STRING_OKAY; + typed_string = (flags & VIR_TYPED_PARAM_STRING_OKAY) != 0; + flags &= ~VIR_TYPED_PARAM_STRING_OKAY; if (flags == VIR_DOMAIN_AFFECT_CURRENT) { if (isActive) flags = VIR_DOMAIN_AFFECT_LIVE; diff --git i/tools/virsh.c w/tools/virsh.c index 58e7894..7197105 100644 --- i/tools/virsh.c +++ w/tools/virsh.c @@ -4717,13 +4717,13 @@ cmdBlkiotune(vshControl * ctl, const vshCmd * cmd) if (nparams == 0) { /* get the number of blkio parameters */ - /* old libvirtd doesn't understand VIR_DOMAIN_TYPED_STRING_OKAY, we + /* old libvirtd doesn't understand VIR_TYPED_PARAM_STRING_OKAY, we * give it a second try with this flag disabled in the case of an * old libvirtd. */ - flags |= VIR_DOMAIN_TYPED_STRING_OKAY; + flags |= VIR_TYPED_PARAM_STRING_OKAY; if (virDomainGetBlkioParameters(dom, NULL, &nparams, flags) != 0) { if (last_error->code == VIR_ERR_INVALID_ARG) { - flags &= ~VIR_DOMAIN_TYPED_STRING_OKAY; + flags &= ~VIR_TYPED_PARAM_STRING_OKAY; nparams = 0; if (virDomainGetBlkioParameters(dom, NULL, &nparams, flags) != 0) { vshError(ctl, "%s", @@ -4810,10 +4810,10 @@ cmdBlkiotune(vshControl * ctl, const vshCmd * cmd) } ret = true; if (weight_device) { - flags |= VIR_DOMAIN_TYPED_STRING_OKAY; + flags |= VIR_TYPED_PARAM_STRING_OKAY; if (virDomainSetBlkioParameters(dom, params, nparams, flags) != 0) { if (last_error->code == VIR_ERR_INVALID_ARG) { - flags &= ~VIR_DOMAIN_TYPED_STRING_OKAY; + flags &= ~VIR_TYPED_PARAM_STRING_OKAY; if (virDomainSetBlkioParameters(dom, params, nparams, flags) != 0) { vshError(ctl, "%s", _("Unable to change blkio parameters")); ret = false; -- Eric Blake eblake(a)redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org