[libvirt] [PATCH 00/16] Add ability fill driver specific defaults when parsing the XML

This monster series cleans up a ton of stuff and then adds the ability to fill driver specific defaults by means of a callback. The usage is demonstrated on automaticaly filling default NIC type with qemu. The long term aim is to move all validations and default settings out of the parser. Peter Krempa (16): conf: Improve core dump config error message qemu: Refactor error paths in virQEMUDriverCreateCapabilities conf: Ensure that new devices are added to conf copy function conf: Fix label naming in virDomainDefFormatInternal conf: Reformat many function headers in domain_conf.c conf: Refactor cpumask handling conf: whitespace cleanups and refactors with no semantic impact conf: Refactor ABI stability checking and break long lines conf: Make virDomainDeviceInfoIterate usable without os type conf: Add separate defaults addition and validation for XML parsing qemu: Implement the device parse callback and use it for interfaces tests: add to the tests and fix fallout qemu_command: Clean up default model passing conf: Don't add default controllers in the XML parser conf: Simplify cputune parameter retrieval conf: Move validation of domain title src/conf/capabilities.h | 6 + src/conf/domain_conf.c | 1177 ++++++++++---------- src/conf/domain_conf.h | 4 +- src/qemu/qemu_command.c | 14 +- src/qemu/qemu_conf.c | 18 +- src/qemu/qemu_domain.c | 23 + src/qemu/qemu_domain.h | 1 + .../qemuxml2argv-net-bandwidth.xml | 1 + .../qemuxml2argvdata/qemuxml2argv-net-client.args | 6 +- .../qemuxml2argv-net-eth-ifname.args | 6 +- .../qemuxml2argv-net-eth-ifname.xml | 1 + .../qemuxml2argv-net-eth-names.args | 8 +- tests/qemuxml2argvdata/qemuxml2argv-net-eth.args | 6 +- tests/qemuxml2argvdata/qemuxml2argv-net-eth.xml | 1 + .../qemuxml2argvdata/qemuxml2argv-net-hostdev.xml | 1 + tests/qemuxml2argvdata/qemuxml2argv-net-mcast.args | 6 +- .../qemuxml2argv-net-openvswitch.xml | 1 + .../qemuxml2argvdata/qemuxml2argv-net-server.args | 6 +- tests/qemuxml2argvdata/qemuxml2argv-net-user.args | 5 +- tests/qemuxml2argvdata/qemuxml2argv-net-user.xml | 1 + .../qemuxml2argv-net-virtio-network-portgroup.xml | 2 + .../qemuxml2xmlout-graphics-spice-timeout.xml | 1 + tests/testutilsqemu.c | 1 + 23 files changed, 663 insertions(+), 633 deletions(-) -- 1.8.1.1

The message didn't seem to be much helpful --- src/conf/domain_conf.c | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index f2887c6..bd1ea25 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -9265,16 +9265,13 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, goto error; /* and info about it */ - tmp = virXPathString("string(./memory[1]/@dumpCore)", ctxt); - if (tmp) { - def->mem.dump_core = virDomainMemDumpTypeFromString(tmp); - - if (def->mem.dump_core <= 0) { - virReportError(VIR_ERR_XML_ERROR, _("Bad value '%s'"), tmp); - goto error; - } - VIR_FREE(tmp); + if ((tmp = virXPathString("string(./memory[1]/@dumpCore)", ctxt)) && + (def->mem.dump_core = virDomainMemDumpTypeFromString(tmp)) <= 0) { + virReportError(VIR_ERR_XML_ERROR, + _("Invalid memory core dump attribute value '%s'"), tmp); + goto error; } + VIR_FREE(tmp); if (def->mem.cur_balloon > def->mem.max_balloon) { /* Older libvirt could get into this situation due to -- 1.8.1.1

On 02/20/2013 10:06 AM, Peter Krempa wrote:
The message didn't seem to be much helpful --- src/conf/domain_conf.c | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-)
ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

Change the error label to "error" and simplify some error paths. --- src/qemu/qemu_conf.c | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 8299b79..a2a05d4 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -543,11 +543,8 @@ virCapsPtr virQEMUDriverCreateCapabilities(virQEMUDriverPtr driver) virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); /* Basic host arch / guest machine capabilities */ - if (!(caps = virQEMUCapsInit(driver->qemuCapsCache))) { - virReportOOMError(); - virObjectUnref(cfg); - return NULL; - } + if (!(caps = virQEMUCapsInit(driver->qemuCapsCache))) + goto no_memory; if (cfg->allowDiskFormatProbing) { caps->defaultDiskDriverName = NULL; @@ -563,14 +560,12 @@ virCapsPtr virQEMUDriverCreateCapabilities(virQEMUDriverPtr driver) if (virGetHostUUID(caps->host.host_uuid)) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("cannot get the host uuid")); - goto err_exit; + goto error; } /* access sec drivers and create a sec model for each one */ - sec_managers = virSecurityManagerGetNested(driver->securityManager); - if (sec_managers == NULL) { - goto err_exit; - } + if (!(sec_managers = virSecurityManagerGetNested(driver->securityManager))) + goto error; /* calculate length */ for (i = 0; sec_managers[i]; i++) @@ -597,7 +592,7 @@ virCapsPtr virQEMUDriverCreateCapabilities(virQEMUDriverPtr driver) no_memory: virReportOOMError(); -err_exit: +error: VIR_FREE(sec_managers); virObjectUnref(caps); virObjectUnref(cfg); -- 1.8.1.1

On 02/20/2013 10:06 AM, Peter Krempa wrote:
Change the error label to "error" and simplify some error paths. --- src/qemu/qemu_conf.c | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-)
ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

Use the correct type and get rid of "default" label in switch to make the compiler complain if a new device type is added. --- src/conf/domain_conf.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index bd1ea25..218a28b 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -16030,7 +16030,7 @@ virDomainDeviceDefCopy(virCapsPtr caps, char *xmlStr = NULL; int rc = -1; - switch (src->type) { + switch ((virDomainDeviceType) src->type) { case VIR_DOMAIN_DEVICE_DISK: rc = virDomainDiskDefFormat(&buf, src->data.disk, flags); break; @@ -16070,7 +16070,11 @@ virDomainDeviceDefCopy(virCapsPtr caps, case VIR_DOMAIN_DEVICE_REDIRDEV: rc = virDomainRedirdevDefFormat(&buf, src->data.redirdev, flags); break; - default: + case VIR_DOMAIN_DEVICE_NONE: + case VIR_DOMAIN_DEVICE_SMARTCARD: + case VIR_DOMAIN_DEVICE_CHR: + case VIR_DOMAIN_DEVICE_MEMBALLOON: + case VIR_DOMAIN_DEVICE_LAST: virReportError(VIR_ERR_INTERNAL_ERROR, _("Copying definition of '%d' type " "is not implemented yet."), -- 1.8.1.1

On 02/20/2013 10:06 AM, Peter Krempa wrote:
Use the correct type and get rid of "default" label in switch to make the compiler complain if a new device type is added. --- src/conf/domain_conf.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

The label named "cleanup" was used in error cases only. Change it to "error". --- src/conf/domain_conf.c | 72 +++++++++++++++++++++++++------------------------- 1 file changed, 36 insertions(+), 36 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 218a28b..e6b1f7c 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -14267,7 +14267,7 @@ virDomainDefFormatInternal(virDomainDefPtr def, if (!(type = virDomainVirtTypeToString(def->virtType))) { virReportError(VIR_ERR_INTERNAL_ERROR, _("unexpected domain type %d"), def->virtType); - goto cleanup; + goto error; } if (def->id == -1) @@ -14307,7 +14307,7 @@ virDomainDefFormatInternal(virDomainDefPtr def, virBufferGetIndent(buf, false) / 2 + 1, 1) < 0) { xmlBufferFree(xmlbuf); xmlIndentTreeOutput = oldIndentTreeOutput; - goto cleanup; + goto error; } virBufferAsprintf(buf, " %s\n", (char *) xmlBufferContent(xmlbuf)); xmlBufferFree(xmlbuf); @@ -14395,7 +14395,7 @@ virDomainDefFormatInternal(virDomainDefPtr def, if (def->cpumask && !virBitmapIsAllSet(def->cpumask)) { char *cpumask = NULL; if ((cpumask = virBitmapFormat(def->cpumask)) == NULL) - goto cleanup; + goto error; virBufferAsprintf(buf, " cpuset='%s'", cpumask); VIR_FREE(cpumask); } @@ -14448,7 +14448,7 @@ virDomainDefFormatInternal(virDomainDefPtr def, if (cpumask == NULL) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("failed to format cpuset for vcpupin")); - goto cleanup; + goto error; } virBufferAsprintf(buf, "cpuset='%s'/>\n", cpumask); @@ -14463,7 +14463,7 @@ virDomainDefFormatInternal(virDomainDefPtr def, if (cpumask == NULL) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("failed to format cpuset for emulator")); - goto cleanup; + goto error; } virBufferAsprintf(buf, "cpuset='%s'/>\n", cpumask); @@ -14493,7 +14493,7 @@ virDomainDefFormatInternal(virDomainDefPtr def, virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("failed to format nodeset for " "NUMA memory tuning")); - goto cleanup; + goto error; } virBufferAsprintf(buf, "nodeset='%s'/>\n", nodemask); VIR_FREE(nodemask); @@ -14557,7 +14557,7 @@ virDomainDefFormatInternal(virDomainDefPtr def, virReportError(VIR_ERR_INTERNAL_ERROR, _("unexpected boot device type %d"), def->os.bootDevs[n]); - goto cleanup; + goto error; } virBufferAsprintf(buf, " <boot dev='%s'/>\n", boottype); } @@ -14590,7 +14590,7 @@ virDomainDefFormatInternal(virDomainDefPtr def, if (mode == NULL) { virReportError(VIR_ERR_INTERNAL_ERROR, _("unexpected smbios mode %d"), def->os.smbios_mode); - goto cleanup; + goto error; } virBufferAsprintf(buf, " <smbios mode='%s'/>\n", mode); } @@ -14605,7 +14605,7 @@ virDomainDefFormatInternal(virDomainDefPtr def, if (!name) { virReportError(VIR_ERR_INTERNAL_ERROR, _("unexpected feature %d"), i); - goto cleanup; + goto error; } virBufferAsprintf(buf, " <%s", name); if (i == VIR_DOMAIN_FEATURE_APIC && def->apic_eoi) { @@ -14640,7 +14640,7 @@ virDomainDefFormatInternal(virDomainDefPtr def, virBufferAdjustIndent(buf, 2); if (virCPUDefFormatBufFull(buf, def->cpu, flags) < 0) - goto cleanup; + goto error; virBufferAdjustIndent(buf, -2); virBufferAsprintf(buf, " <clock offset='%s'", @@ -14666,7 +14666,7 @@ virDomainDefFormatInternal(virDomainDefPtr def, virBufferAddLit(buf, ">\n"); for (n = 0; n < def->clock.ntimers; n++) { if (virDomainTimerDefFormat(buf, def->clock.timers[n]) < 0) - goto cleanup; + goto error; } virBufferAddLit(buf, " </clock>\n"); } @@ -14674,20 +14674,20 @@ virDomainDefFormatInternal(virDomainDefPtr def, if (virDomainEventActionDefFormat(buf, def->onPoweroff, "on_poweroff", virDomainLifecycleTypeToString) < 0) - goto cleanup; + goto error; if (virDomainEventActionDefFormat(buf, def->onReboot, "on_reboot", virDomainLifecycleTypeToString) < 0) - goto cleanup; + goto error; if (virDomainEventActionDefFormat(buf, def->onCrash, "on_crash", virDomainLifecycleCrashTypeToString) < 0) - goto cleanup; + goto error; if (def->onLockFailure != VIR_DOMAIN_LOCK_FAILURE_DEFAULT && virDomainEventActionDefFormat(buf, def->onLockFailure, "on_lockfailure", virDomainLockFailureTypeToString) < 0) - goto cleanup; + goto error; if (def->pm.s3 || def->pm.s4) { virBufferAddLit(buf, " <pm>\n"); @@ -14709,36 +14709,36 @@ virDomainDefFormatInternal(virDomainDefPtr def, for (n = 0 ; n < def->ndisks ; n++) if (virDomainDiskDefFormat(buf, def->disks[n], flags) < 0) - goto cleanup; + goto error; for (n = 0 ; n < def->ncontrollers ; n++) if (virDomainControllerDefFormat(buf, def->controllers[n], flags) < 0) - goto cleanup; + goto error; for (n = 0 ; n < def->nleases ; n++) if (virDomainLeaseDefFormat(buf, def->leases[n]) < 0) - goto cleanup; + goto error; for (n = 0 ; n < def->nfss ; n++) if (virDomainFSDefFormat(buf, def->fss[n], flags) < 0) - goto cleanup; + goto error; for (n = 0 ; n < def->nnets ; n++) if (virDomainNetDefFormat(buf, def->nets[n], flags) < 0) - goto cleanup; + goto error; for (n = 0 ; n < def->nsmartcards ; n++) if (virDomainSmartcardDefFormat(buf, def->smartcards[n], flags) < 0) - goto cleanup; + goto error; for (n = 0 ; n < def->nserials ; n++) if (virDomainChrDefFormat(buf, def->serials[n], flags) < 0) - goto cleanup; + goto error; for (n = 0 ; n < def->nparallels ; n++) if (virDomainChrDefFormat(buf, def->parallels[n], flags) < 0) - goto cleanup; + goto error; for (n = 0 ; n < def->nconsoles ; n++) { virDomainChrDef console; @@ -14755,7 +14755,7 @@ virDomainDefFormatInternal(virDomainDefPtr def, memcpy(&console, def->consoles[n], sizeof(console)); } if (virDomainChrDefFormat(buf, &console, flags) < 0) - goto cleanup; + goto error; } if (STREQ(def->os.type, "hvm") && def->nconsoles == 0 && @@ -14764,17 +14764,17 @@ virDomainDefFormatInternal(virDomainDefPtr def, memcpy(&console, def->serials[n], sizeof(console)); console.deviceType = VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE; if (virDomainChrDefFormat(buf, &console, flags) < 0) - goto cleanup; + goto error; } for (n = 0 ; n < def->nchannels ; n++) if (virDomainChrDefFormat(buf, def->channels[n], flags) < 0) - goto cleanup; + goto error; for (n = 0 ; n < def->ninputs ; n++) if (def->inputs[n]->bus == VIR_DOMAIN_INPUT_BUS_USB && virDomainInputDefFormat(buf, def->inputs[n], flags) < 0) - goto cleanup; + goto error; if (def->ngraphics > 0) { /* If graphics is enabled, add the implicit mouse */ @@ -14786,20 +14786,20 @@ virDomainDefFormatInternal(virDomainDefPtr def, }; if (virDomainInputDefFormat(buf, &autoInput, flags) < 0) - goto cleanup; + goto error; for (n = 0 ; n < def->ngraphics ; n++) if (virDomainGraphicsDefFormat(buf, def->graphics[n], flags) < 0) - goto cleanup; + goto error; } for (n = 0 ; n < def->nsounds ; n++) if (virDomainSoundDefFormat(buf, def->sounds[n], flags) < 0) - goto cleanup; + goto error; for (n = 0 ; n < def->nvideos ; n++) if (virDomainVideoDefFormat(buf, def->videos[n], flags) < 0) - goto cleanup; + goto error; for (n = 0 ; n < def->nhostdevs ; n++) { /* If parent.type != NONE, this is just a pointer to the @@ -14808,20 +14808,20 @@ virDomainDefFormatInternal(virDomainDefPtr def, */ if (def->hostdevs[n]->parent.type == VIR_DOMAIN_DEVICE_NONE && virDomainHostdevDefFormat(buf, def->hostdevs[n], flags) < 0) { - goto cleanup; + goto error; } } for (n = 0 ; n < def->nredirdevs ; n++) if (virDomainRedirdevDefFormat(buf, def->redirdevs[n], flags) < 0) - goto cleanup; + goto error; if (def->redirfilter) virDomainRedirFilterDefFormat(buf, def->redirfilter); for (n = 0 ; n < def->nhubs ; n++) if (virDomainHubDefFormat(buf, def->hubs[n], flags) < 0) - goto cleanup; + goto error; if (def->watchdog) virDomainWatchdogDefFormat(buf, def->watchdog, flags); @@ -14838,7 +14838,7 @@ virDomainDefFormatInternal(virDomainDefPtr def, if (def->namespaceData && def->ns.format) { if ((def->ns.format)(buf, def->namespaceData) < 0) - goto cleanup; + goto error; } virBufferAddLit(buf, "</domain>\n"); @@ -14850,7 +14850,7 @@ virDomainDefFormatInternal(virDomainDefPtr def, no_memory: virReportOOMError(); - cleanup: + error: virBufferFreeAndReset(buf); return -1; } -- 1.8.1.1

On 02/20/2013 12:06 PM, Peter Krempa wrote:
The label named "cleanup" was used in error cases only. Change it to "error". --- src/conf/domain_conf.c | 72 +++++++++++++++++++++++++------------------------- 1 file changed, 36 insertions(+), 36 deletions(-)
Mechanical. ACK.

Many of the headers were using the old style and even overflowing the 80 column mark. --- src/conf/domain_conf.c | 341 +++++++++++++++++++++++++++++-------------------- 1 file changed, 201 insertions(+), 140 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index e6b1f7c..fad293b 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -8093,12 +8093,13 @@ error: return NULL; } -static int virDomainEventActionParseXML(xmlXPathContextPtr ctxt, - const char *name, - const char *xpath, - int *val, - int defaultVal, - virEventActionFromStringFunc convFunc) +static int +virDomainEventActionParseXML(xmlXPathContextPtr ctxt, + const char *name, + const char *xpath, + int *val, + int defaultVal, + virEventActionFromStringFunc convFunc) { char *tmp = virXPathString(xpath, ctxt); if (tmp == NULL) { @@ -8138,10 +8139,11 @@ virDomainPMStateParseXML(xmlXPathContextPtr ctxt, return ret; } -virDomainDeviceDefPtr virDomainDeviceDefParse(virCapsPtr caps, - virDomainDefPtr def, - const char *xmlStr, - unsigned int flags) +virDomainDeviceDefPtr +virDomainDeviceDefParse(virCapsPtr caps, + virDomainDefPtr def, + const char *xmlStr, + unsigned int flags) { xmlDocPtr xml; xmlNodePtr node; @@ -9129,12 +9131,13 @@ cleanup: } -static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, - xmlDocPtr xml, - xmlNodePtr root, - xmlXPathContextPtr ctxt, - unsigned int expectedVirtTypes, - unsigned int flags) +static virDomainDefPtr +virDomainDefParseXML(virCapsPtr caps, + xmlDocPtr xml, + xmlNodePtr root, + xmlXPathContextPtr ctxt, + unsigned int expectedVirtTypes, + unsigned int flags) { xmlNodePtr *nodes = NULL, node = NULL; char *tmp = NULL; @@ -10739,11 +10742,12 @@ no_memory: } -static virDomainObjPtr virDomainObjParseXML(virCapsPtr caps, - xmlDocPtr xml, - xmlXPathContextPtr ctxt, - unsigned int expectedVirtTypes, - unsigned int flags) +static virDomainObjPtr +virDomainObjParseXML(virCapsPtr caps, + xmlDocPtr xml, + xmlXPathContextPtr ctxt, + unsigned int expectedVirtTypes, + unsigned int flags) { char *tmp = NULL; long val; @@ -10857,28 +10861,31 @@ virDomainDefParse(const char *xmlStr, return def; } -virDomainDefPtr virDomainDefParseString(virCapsPtr caps, - const char *xmlStr, - unsigned int expectedVirtTypes, - unsigned int flags) +virDomainDefPtr +virDomainDefParseString(virCapsPtr caps, + const char *xmlStr, + unsigned int expectedVirtTypes, + unsigned int flags) { return virDomainDefParse(xmlStr, NULL, caps, expectedVirtTypes, flags); } -virDomainDefPtr virDomainDefParseFile(virCapsPtr caps, - const char *filename, - unsigned int expectedVirtTypes, - unsigned int flags) +virDomainDefPtr +virDomainDefParseFile(virCapsPtr caps, + const char *filename, + unsigned int expectedVirtTypes, + unsigned int flags) { return virDomainDefParse(NULL, filename, caps, expectedVirtTypes, flags); } -virDomainDefPtr virDomainDefParseNode(virCapsPtr caps, - xmlDocPtr xml, - xmlNodePtr root, - unsigned int expectedVirtTypes, - unsigned int flags) +virDomainDefPtr +virDomainDefParseNode(virCapsPtr caps, + xmlDocPtr xml, + xmlNodePtr root, + unsigned int expectedVirtTypes, + unsigned int flags) { xmlXPathContextPtr ctxt = NULL; virDomainDefPtr def = NULL; @@ -10961,8 +10968,9 @@ virDomainObjParseFile(virCapsPtr caps, } -static bool virDomainTimerDefCheckABIStability(virDomainTimerDefPtr src, - virDomainTimerDefPtr dst) +static bool +virDomainTimerDefCheckABIStability(virDomainTimerDefPtr src, + virDomainTimerDefPtr dst) { bool identical = false; @@ -11005,8 +11013,9 @@ cleanup: } -static bool virDomainDeviceInfoCheckABIStability(virDomainDeviceInfoPtr src, - virDomainDeviceInfoPtr dst) +static bool +virDomainDeviceInfoCheckABIStability(virDomainDeviceInfoPtr src, + virDomainDeviceInfoPtr dst) { bool identical = false; @@ -11084,8 +11093,9 @@ cleanup: } -static bool virDomainDiskDefCheckABIStability(virDomainDiskDefPtr src, - virDomainDiskDefPtr dst) +static bool +virDomainDiskDefCheckABIStability(virDomainDiskDefPtr src, + virDomainDiskDefPtr dst) { bool identical = false; @@ -11135,8 +11145,9 @@ cleanup: } -static bool virDomainControllerDefCheckABIStability(virDomainControllerDefPtr src, - virDomainControllerDefPtr dst) +static bool +virDomainControllerDefCheckABIStability(virDomainControllerDefPtr src, + virDomainControllerDefPtr dst) { bool identical = false; @@ -11188,8 +11199,9 @@ cleanup: } -static bool virDomainFsDefCheckABIStability(virDomainFSDefPtr src, - virDomainFSDefPtr dst) +static bool +virDomainFsDefCheckABIStability(virDomainFSDefPtr src, + virDomainFSDefPtr dst) { bool identical = false; @@ -11216,8 +11228,9 @@ cleanup: } -static bool virDomainNetDefCheckABIStability(virDomainNetDefPtr src, - virDomainNetDefPtr dst) +static bool +virDomainNetDefCheckABIStability(virDomainNetDefPtr src, + virDomainNetDefPtr dst) { bool identical = false; @@ -11249,8 +11262,9 @@ cleanup: } -static bool virDomainInputDefCheckABIStability(virDomainInputDefPtr src, - virDomainInputDefPtr dst) +static bool +virDomainInputDefCheckABIStability(virDomainInputDefPtr src, + virDomainInputDefPtr dst) { bool identical = false; @@ -11280,8 +11294,9 @@ cleanup: } -static bool virDomainSoundDefCheckABIStability(virDomainSoundDefPtr src, - virDomainSoundDefPtr dst) +static bool +virDomainSoundDefCheckABIStability(virDomainSoundDefPtr src, + virDomainSoundDefPtr dst) { bool identical = false; @@ -11303,8 +11318,9 @@ cleanup: } -static bool virDomainVideoDefCheckABIStability(virDomainVideoDefPtr src, - virDomainVideoDefPtr dst) +static bool +virDomainVideoDefCheckABIStability(virDomainVideoDefPtr src, + virDomainVideoDefPtr dst) { bool identical = false; @@ -11363,8 +11379,9 @@ cleanup: } -static bool virDomainHostdevDefCheckABIStability(virDomainHostdevDefPtr src, - virDomainHostdevDefPtr dst) +static bool +virDomainHostdevDefCheckABIStability(virDomainHostdevDefPtr src, + virDomainHostdevDefPtr dst) { bool identical = false; @@ -11396,8 +11413,9 @@ cleanup: } -static bool virDomainSmartcardDefCheckABIStability(virDomainSmartcardDefPtr src, - virDomainSmartcardDefPtr dst) +static bool +virDomainSmartcardDefCheckABIStability(virDomainSmartcardDefPtr src, + virDomainSmartcardDefPtr dst) { bool identical = false; @@ -11411,8 +11429,9 @@ cleanup: } -static bool virDomainSerialDefCheckABIStability(virDomainChrDefPtr src, - virDomainChrDefPtr dst) +static bool +virDomainSerialDefCheckABIStability(virDomainChrDefPtr src, + virDomainChrDefPtr dst) { bool identical = false; @@ -11433,8 +11452,9 @@ cleanup: } -static bool virDomainParallelDefCheckABIStability(virDomainChrDefPtr src, - virDomainChrDefPtr dst) +static bool +virDomainParallelDefCheckABIStability(virDomainChrDefPtr src, + virDomainChrDefPtr dst) { bool identical = false; @@ -11455,8 +11475,9 @@ cleanup: } -static bool virDomainChannelDefCheckABIStability(virDomainChrDefPtr src, - virDomainChrDefPtr dst) +static bool +virDomainChannelDefCheckABIStability(virDomainChrDefPtr src, + virDomainChrDefPtr dst) { bool identical = false; @@ -11511,8 +11532,9 @@ cleanup: } -static bool virDomainConsoleDefCheckABIStability(virDomainChrDefPtr src, - virDomainChrDefPtr dst) +static bool +virDomainConsoleDefCheckABIStability(virDomainChrDefPtr src, + virDomainChrDefPtr dst) { bool identical = false; @@ -11534,8 +11556,9 @@ cleanup: } -static bool virDomainWatchdogDefCheckABIStability(virDomainWatchdogDefPtr src, - virDomainWatchdogDefPtr dst) +static bool +virDomainWatchdogDefCheckABIStability(virDomainWatchdogDefPtr src, + virDomainWatchdogDefPtr dst) { bool identical = false; @@ -11557,8 +11580,9 @@ cleanup: } -static bool virDomainMemballoonDefCheckABIStability(virDomainMemballoonDefPtr src, - virDomainMemballoonDefPtr dst) +static bool +virDomainMemballoonDefCheckABIStability(virDomainMemballoonDefPtr src, + virDomainMemballoonDefPtr dst) { bool identical = false; @@ -11580,8 +11604,9 @@ cleanup: } -static bool virDomainHubDefCheckABIStability(virDomainHubDefPtr src, - virDomainHubDefPtr dst) +static bool +virDomainHubDefCheckABIStability(virDomainHubDefPtr src, + virDomainHubDefPtr dst) { bool identical = false; @@ -11662,8 +11687,9 @@ cleanup: * which will affect the guest ABI. This is primarily to allow * validation of custom XML config passed in during migration */ -bool virDomainDefCheckABIStability(virDomainDefPtr src, - virDomainDefPtr dst) +bool +virDomainDefCheckABIStability(virDomainDefPtr src, + virDomainDefPtr dst) { bool identical = false; int i; @@ -11969,9 +11995,10 @@ cleanup: } -static int virDomainDefAddDiskControllersForType(virDomainDefPtr def, - int controllerType, - int diskBus) +static int +virDomainDefAddDiskControllersForType(virDomainDefPtr def, + int controllerType, + int diskBus) { int i; int maxController = -1; @@ -11996,7 +12023,8 @@ static int virDomainDefAddDiskControllersForType(virDomainDefPtr def, } -static int virDomainDefMaybeAddVirtioSerialController(virDomainDefPtr def) +static int +virDomainDefMaybeAddVirtioSerialController(virDomainDefPtr def) { /* Look for any virtio serial or virtio console devs */ int i; @@ -12079,7 +12107,8 @@ virDomainDefMaybeAddSmartcardController(virDomainDefPtr def) * in the XML. This is for compat with existing apps which will * not know/care about <controller> info in the XML */ -int virDomainDefAddImplicitControllers(virDomainDefPtr def) +int +virDomainDefAddImplicitControllers(virDomainDefPtr def) { if (virDomainDefAddDiskControllersForType(def, VIR_DOMAIN_CONTROLLER_TYPE_SCSI, @@ -12148,11 +12177,12 @@ virDomainVcpuPinFindByVcpu(virDomainVcpuPinDefPtr *def, return NULL; } -int virDomainVcpuPinAdd(virDomainVcpuPinDefPtr **vcpupin_list, - int *nvcpupin, - unsigned char *cpumap, - int maplen, - int vcpu) +int +virDomainVcpuPinAdd(virDomainVcpuPinDefPtr **vcpupin_list, + int *nvcpupin, + unsigned char *cpumap, + int maplen, + int vcpu) { virDomainVcpuPinDefPtr vcpupin = NULL; @@ -12388,8 +12418,9 @@ virDomainLeaseDefFormat(virBufferPtr buf, return 0; } -static void virDomainDiskGeometryDefFormat(virBufferPtr buf, - virDomainDiskDefPtr def) +static void +virDomainDiskGeometryDefFormat(virBufferPtr buf, + virDomainDiskDefPtr def) { const char *trans = virDomainDiskGeometryTransTypeToString(def->geometry.trans); @@ -12409,8 +12440,9 @@ static void virDomainDiskGeometryDefFormat(virBufferPtr buf, virBufferAddLit(buf, "/>\n"); } } -static void virDomainDiskBlockIoDefFormat(virBufferPtr buf, - virDomainDiskDefPtr def) +static void +virDomainDiskBlockIoDefFormat(virBufferPtr buf, + virDomainDiskDefPtr def) { if (def->blockio.logical_block_size > 0 || def->blockio.physical_block_size > 0) { @@ -14868,9 +14900,10 @@ virDomainDefFormat(virDomainDefPtr def, unsigned int flags) } -static char *virDomainObjFormat(virCapsPtr caps, - virDomainObjPtr obj, - unsigned int flags) +static char * +virDomainObjFormat(virCapsPtr caps, + virDomainObjPtr obj, + unsigned int flags) { virBuffer buf = VIR_BUFFER_INITIALIZER; int state; @@ -14964,9 +14997,10 @@ virDomainDefCompatibleDevice(virDomainDefPtr def, return 0; } -int virDomainSaveXML(const char *configDir, - virDomainDefPtr def, - const char *xml) +int +virDomainSaveXML(const char *configDir, + virDomainDefPtr def, + const char *xml) { char uuidstr[VIR_UUID_STRING_BUFLEN]; char *configFile = NULL; @@ -14992,8 +15026,9 @@ int virDomainSaveXML(const char *configDir, return ret; } -int virDomainSaveConfig(const char *configDir, - virDomainDefPtr def) +int +virDomainSaveConfig(const char *configDir, + virDomainDefPtr def) { int ret = -1; char *xml; @@ -15011,9 +15046,10 @@ cleanup: return ret; } -int virDomainSaveStatus(virCapsPtr caps, - const char *statusDir, - virDomainObjPtr obj) +int +virDomainSaveStatus(virCapsPtr caps, + const char *statusDir, + virDomainObjPtr obj) { unsigned int flags = (VIR_DOMAIN_XML_SECURE | VIR_DOMAIN_XML_INTERNAL_STATUS | @@ -15130,14 +15166,15 @@ error: return NULL; } -int virDomainObjListLoadAllConfigs(virDomainObjListPtr doms, - virCapsPtr caps, - const char *configDir, - const char *autostartDir, - int liveStatus, - unsigned int expectedVirtTypes, - virDomainLoadConfigNotify notify, - void *opaque) +int +virDomainObjListLoadAllConfigs(virDomainObjListPtr doms, + virCapsPtr caps, + const char *configDir, + const char *autostartDir, + int liveStatus, + unsigned int expectedVirtTypes, + virDomainLoadConfigNotify notify, + void *opaque) { DIR *dir; struct dirent *entry; @@ -15196,9 +15233,10 @@ int virDomainObjListLoadAllConfigs(virDomainObjListPtr doms, return 0; } -int virDomainDeleteConfig(const char *configDir, - const char *autostartDir, - virDomainObjPtr dom) +int +virDomainDeleteConfig(const char *configDir, + const char *autostartDir, + virDomainObjPtr dom) { char *configFile = NULL, *autostartLink = NULL; int ret = -1; @@ -15227,8 +15265,9 @@ cleanup: return ret; } -char *virDomainConfigFile(const char *dir, - const char *name) +char +*virDomainConfigFile(const char *dir, + const char *name) { char *ret = NULL; @@ -15248,9 +15287,10 @@ char *virDomainConfigFile(const char *dir, * @param devIdx parsed device number * @return 0 on success, -1 on failure */ -int virDiskNameToBusDeviceIndex(const virDomainDiskDefPtr disk, - int *busIdx, - int *devIdx) { +int +virDiskNameToBusDeviceIndex(const virDomainDiskDefPtr disk, + int *busIdx, + int *devIdx) { int idx = virDiskNameToIndex(disk->dst); if (idx < 0) @@ -15278,7 +15318,8 @@ int virDiskNameToBusDeviceIndex(const virDomainDiskDefPtr disk, return 0; } -virDomainFSDefPtr virDomainGetRootFilesystem(virDomainDefPtr def) +virDomainFSDefPtr +virDomainGetRootFilesystem(virDomainDefPtr def) { int i; @@ -15294,7 +15335,10 @@ virDomainFSDefPtr virDomainGetRootFilesystem(virDomainDefPtr def) } -static void virDomainObjListCountActive(void *payload, const void *name ATTRIBUTE_UNUSED, void *data) +static void +virDomainObjListCountActive(void *payload, + const void *name ATTRIBUTE_UNUSED, + void *data) { virDomainObjPtr obj = payload; int *count = data; @@ -15304,7 +15348,10 @@ static void virDomainObjListCountActive(void *payload, const void *name ATTRIBUT virObjectUnlock(obj); } -static void virDomainObjListCountInactive(void *payload, const void *name ATTRIBUTE_UNUSED, void *data) +static void +virDomainObjListCountInactive(void *payload, + const void *name ATTRIBUTE_UNUSED, + void *data) { virDomainObjPtr obj = payload; int *count = data; @@ -15314,7 +15361,9 @@ static void virDomainObjListCountInactive(void *payload, const void *name ATTRIB virObjectUnlock(obj); } -int virDomainObjListNumOfDomains(virDomainObjListPtr doms, int active) +int +virDomainObjListNumOfDomains(virDomainObjListPtr doms, + int active) { int count = 0; virObjectLock(doms); @@ -15332,7 +15381,10 @@ struct virDomainIDData { int *ids; }; -static void virDomainObjListCopyActiveIDs(void *payload, const void *name ATTRIBUTE_UNUSED, void *opaque) +static void +virDomainObjListCopyActiveIDs(void *payload, + const void *name ATTRIBUTE_UNUSED, + void *opaque) { virDomainObjPtr obj = payload; struct virDomainIDData *data = opaque; @@ -15342,9 +15394,10 @@ static void virDomainObjListCopyActiveIDs(void *payload, const void *name ATTRIB virObjectUnlock(obj); } -int virDomainObjListGetActiveIDs(virDomainObjListPtr doms, - int *ids, - int maxids) +int +virDomainObjListGetActiveIDs(virDomainObjListPtr doms, + int *ids, + int maxids) { struct virDomainIDData data = { 0, maxids, ids }; virObjectLock(doms); @@ -15360,7 +15413,10 @@ struct virDomainNameData { char **const names; }; -static void virDomainObjListCopyInactiveNames(void *payload, const void *name ATTRIBUTE_UNUSED, void *opaque) +static void +virDomainObjListCopyInactiveNames(void *payload, + const void *name ATTRIBUTE_UNUSED, + void *opaque) { virDomainObjPtr obj = payload; struct virDomainNameData *data = opaque; @@ -15379,9 +15435,10 @@ static void virDomainObjListCopyInactiveNames(void *payload, const void *name AT } -int virDomainObjListGetInactiveNames(virDomainObjListPtr doms, - char **const names, - int maxnames) +int +virDomainObjListGetInactiveNames(virDomainObjListPtr doms, + char **const names, + int maxnames) { struct virDomainNameData data = { 0, 0, maxnames, names }; int i; @@ -15419,9 +15476,10 @@ virDomainObjListHelper(void *payload, data->ret = -1; } -int virDomainObjListForEach(virDomainObjListPtr doms, - virDomainObjListIterator callback, - void *opaque) +int +virDomainObjListForEach(virDomainObjListPtr doms, + virDomainObjListIterator callback, + void *opaque) { struct virDomainListIterData data = { callback, opaque, 0, @@ -15433,10 +15491,11 @@ int virDomainObjListForEach(virDomainObjListPtr doms, } -int virDomainChrDefForeach(virDomainDefPtr def, - bool abortOnError, - virDomainChrDefIterator iter, - void *opaque) +int +virDomainChrDefForeach(virDomainDefPtr def, + bool abortOnError, + virDomainChrDefIterator iter, + void *opaque) { int i; int rc = 0; @@ -15485,10 +15544,11 @@ done: } -int virDomainSmartcardDefForeach(virDomainDefPtr def, - bool abortOnError, - virDomainSmartcardDefIterator iter, - void *opaque) +int +virDomainSmartcardDefForeach(virDomainDefPtr def, + bool abortOnError, + virDomainSmartcardDefIterator iter, + void *opaque) { int i; int rc = 0; @@ -15513,10 +15573,11 @@ done: * ignoreOpenFailure determines whether to warn about a chain that * mentions a backing file without also having metadata on that * file. */ -int virDomainDiskDefForeachPath(virDomainDiskDefPtr disk, - bool ignoreOpenFailure, - virDomainDiskDefPathIterator iter, - void *opaque) +int +virDomainDiskDefForeachPath(virDomainDiskDefPtr disk, + bool ignoreOpenFailure, + virDomainDiskDefPathIterator iter, + void *opaque) { int ret = -1; size_t depth = 0; -- 1.8.1.1

On 02/20/2013 12:06 PM, Peter Krempa wrote:
Many of the headers were using the old style and even overflowing the 80 column mark. --- src/conf/domain_conf.c | 341 +++++++++++++++++++++++++++++-------------------- 1 file changed, 201 insertions(+), 140 deletions(-)
Also mechanical. ACK.

Declare local variables at the start of the block and fix trivial formatting issues. --- src/conf/domain_conf.c | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index fad293b..18df1bd 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -14463,9 +14463,8 @@ virDomainDefFormatInternal(virDomainDefPtr def, def->cputune.emulator_quota); for (i = 0; i < def->cputune.nvcpupin; i++) { - /* Ignore the vcpupin which inherit from "cpuset" - * of "<vcpu>." - */ + char *cpumask; + /* Ignore the vcpupin which inherit from "cpuset of "<vcpu>." */ if (def->cpumask && virBitmapEqual(def->cpumask, def->cputune.vcpupin[i]->cpumask)) @@ -14474,10 +14473,7 @@ virDomainDefFormatInternal(virDomainDefPtr def, virBufferAsprintf(buf, " <vcpupin vcpu='%u' ", def->cputune.vcpupin[i]->vcpuid); - char *cpumask = NULL; - cpumask = virBitmapFormat(def->cputune.vcpupin[i]->cpumask); - - if (cpumask == NULL) { + if (!(cpumask = virBitmapFormat(def->cputune.vcpupin[i]->cpumask))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("failed to format cpuset for vcpupin")); goto error; @@ -14488,11 +14484,10 @@ virDomainDefFormatInternal(virDomainDefPtr def, } if (def->cputune.emulatorpin) { + char *cpumask; virBufferAsprintf(buf, " <emulatorpin "); - char *cpumask = NULL; - cpumask = virBitmapFormat(def->cputune.emulatorpin->cpumask); - if (cpumask == NULL) { + if (!(cpumask = virBitmapFormat(def->cputune.emulatorpin->cpumask))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("failed to format cpuset for emulator")); goto error; -- 1.8.1.1

On 02/20/2013 10:06 AM, Peter Krempa wrote:
Declare local variables at the start of the block and fix trivial formatting issues. --- src/conf/domain_conf.c | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-)
ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

This patch changes many unrelated places to simplify the code or update code style. This patch should not have any semantic impact on the code. --- src/conf/domain_conf.c | 137 +++++++++++++++++++++---------------------------- src/conf/domain_conf.h | 4 +- 2 files changed, 60 insertions(+), 81 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 18df1bd..b4340f3 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -8150,9 +8150,9 @@ virDomainDeviceDefParse(virCapsPtr caps, xmlXPathContextPtr ctxt = NULL; virDomainDeviceDefPtr dev = NULL; - if (!(xml = virXMLParseStringCtxt(xmlStr, _("(device_definition)"), &ctxt))) { + if (!(xml = virXMLParseStringCtxt(xmlStr, _("(device_definition)"), &ctxt))) goto error; - } + node = ctxt->node; if (VIR_ALLOC(dev) < 0) { @@ -8219,20 +8219,18 @@ virDomainDeviceDefParse(virCapsPtr caps, if (!(dev->data.redirdev = virDomainRedirdevDefParseXML(node, NULL, flags))) goto error; } else { - virReportError(VIR_ERR_XML_ERROR, - "%s", _("unknown device type")); + virReportError(VIR_ERR_XML_ERROR, "%s", _("unknown device type")); goto error; } +cleanup: xmlFreeDoc(xml); xmlXPathFreeContext(ctxt); return dev; - error: - xmlFreeDoc(xml); - xmlXPathFreeContext(ctxt); +error: VIR_FREE(dev); - return NULL; + goto cleanup; } @@ -9296,8 +9294,7 @@ virDomainDefParseXML(virCapsPtr caps, def->mem.cur_balloon = def->mem.max_balloon; } - node = virXPathNode("./memoryBacking/hugepages", ctxt); - if (node) + if ((node = virXPathNode("./memoryBacking/hugepages", ctxt))) def->mem.hugepage_backed = true; /* Extract blkio cgroup tunables */ @@ -9652,36 +9649,34 @@ virDomainDefParseXML(virCapsPtr caps, } VIR_FREE(nodes); - n = virXPathNodeSet("./features/*", ctxt, &nodes); - if (n < 0) + if ((n = virXPathNodeSet("./features/*", ctxt, &nodes)) < 0) goto error; - if (n) { - for (i = 0 ; i < n ; i++) { - int val = virDomainFeatureTypeFromString((const char *)nodes[i]->name); - if (val < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unexpected feature %s"), - nodes[i]->name); - goto error; - } - def->features |= (1 << val); - if (val == VIR_DOMAIN_FEATURE_APIC) { - tmp = virXPathString("string(./features/apic/@eoi)", ctxt); - if (tmp) { - int eoi; - if ((eoi = virDomainFeatureStateTypeFromString(tmp)) <= 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unknown value for attribute eoi: %s"), - tmp); - goto error; - } - def->apic_eoi = eoi; - VIR_FREE(tmp); + + for (i = 0 ; i < n ; i++) { + int val = virDomainFeatureTypeFromString((const char *)nodes[i]->name); + if (val < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unexpected feature %s"), + nodes[i]->name); + goto error; + } + def->features |= (1 << val); + if (val == VIR_DOMAIN_FEATURE_APIC) { + tmp = virXPathString("string(./features/apic/@eoi)", ctxt); + if (tmp) { + int eoi; + if ((eoi = virDomainFeatureStateTypeFromString(tmp)) <= 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown value for attribute eoi: %s"), + tmp); + goto error; } + def->apic_eoi = eoi; + VIR_FREE(tmp); } } - VIR_FREE(nodes); } + VIR_FREE(nodes); if (def->features & (1 << VIR_DOMAIN_FEATURE_HYPERV)) { int feature; @@ -9769,17 +9764,14 @@ virDomainDefParseXML(virCapsPtr caps, &def->pm.s4) < 0) goto error; - tmp = virXPathString("string(./clock/@offset)", ctxt); - if (tmp) { - if ((def->clock.offset = virDomainClockOffsetTypeFromString(tmp)) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unknown clock offset '%s'"), tmp); - goto error; - } - VIR_FREE(tmp); - } else { - def->clock.offset = VIR_DOMAIN_CLOCK_OFFSET_UTC; + if ((tmp = virXPathString("string(./clock/@offset)", ctxt)) && + (def->clock.offset = virDomainClockOffsetTypeFromString(tmp)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unknown clock offset '%s'"), tmp); + goto error; } + VIR_FREE(tmp); + switch (def->clock.offset) { case VIR_DOMAIN_CLOCK_OFFSET_LOCALTIME: case VIR_DOMAIN_CLOCK_OFFSET_UTC: @@ -9838,11 +9830,12 @@ virDomainDefParseXML(virCapsPtr caps, break; } - if ((n = virXPathNodeSet("./clock/timer", ctxt, &nodes)) < 0) { + if ((n = virXPathNodeSet("./clock/timer", ctxt, &nodes)) < 0) goto error; - } + if (n && VIR_ALLOC_N(def->clock.timers, n) < 0) goto no_memory; + for (i = 0 ; i < n ; i++) { virDomainTimerDefPtr timer = virDomainTimerDefParseXML(nodes[i], ctxt); @@ -10690,8 +10683,8 @@ virDomainDefParseXML(virCapsPtr caps, } } } - tmp = virXPathString("string(./os/smbios/@mode)", ctxt); - if (tmp) { + + if ((tmp = virXPathString("string(./os/smbios/@mode)", ctxt))) { int mode; if ((mode = virDomainSmbiosModeTypeFromString(tmp)) < 0) { @@ -10701,27 +10694,22 @@ virDomainDefParseXML(virCapsPtr caps, } def->os.smbios_mode = mode; VIR_FREE(tmp); - } else { - def->os.smbios_mode = VIR_DOMAIN_SMBIOS_NONE; /* not present */ } /* Extract custom metadata */ - if ((node = virXPathNode("./metadata[1]", ctxt)) != NULL) { + if ((node = virXPathNode("./metadata[1]", ctxt)) != NULL) def->metadata = xmlCopyNode(node, 1); - } /* we have to make a copy of all of the callback pointers here since * we won't have the virCaps structure available during free */ def->ns = caps->ns; - if (def->ns.parse) { - if ((def->ns.parse)(xml, root, ctxt, &def->namespaceData) < 0) - goto error; - } + if (def->ns.parse && + (def->ns.parse)(xml, root, ctxt, &def->namespaceData) < 0) + goto error; - /* Auto-add any implied controllers which aren't present - */ + /* Auto-add any implied controllers which aren't present */ if (virDomainDefAddImplicitControllers(def) < 0) goto error; @@ -10731,9 +10719,7 @@ virDomainDefParseXML(virCapsPtr caps, no_memory: virReportOOMError(); - /* fallthrough */ - - error: +error: VIR_FREE(tmp); VIR_FREE(nodes); virBitmapFree(bootMap); @@ -10898,8 +10884,7 @@ virDomainDefParseNode(virCapsPtr caps, goto cleanup; } - ctxt = xmlXPathNewContext(xml); - if (ctxt == NULL) { + if (!(ctxt = xmlXPathNewContext(xml))) { virReportOOMError(); goto cleanup; } @@ -15028,8 +15013,7 @@ virDomainSaveConfig(const char *configDir, int ret = -1; char *xml; - if (!(xml = virDomainDefFormat(def, - VIR_DOMAIN_XML_WRITE_FLAGS))) + if (!(xml = virDomainDefFormat(def, VIR_DOMAIN_XML_WRITE_FLAGS))) goto cleanup; if (virDomainSaveXML(configDir, def, xml)) @@ -15238,7 +15222,8 @@ virDomainDeleteConfig(const char *configDir, if ((configFile = virDomainConfigFile(configDir, dom->def->name)) == NULL) goto cleanup; - if ((autostartLink = virDomainConfigFile(autostartDir, dom->def->name)) == NULL) + if ((autostartLink = virDomainConfigFile(autostartDir, + dom->def->name)) == NULL) goto cleanup; /* Not fatal if this doesn't work */ @@ -15264,12 +15249,10 @@ char *virDomainConfigFile(const char *dir, const char *name) { - char *ret = NULL; + char *ret; - if (virAsprintf(&ret, "%s/%s.xml", dir, name) < 0) { + if (virAsprintf(&ret, "%s/%s.xml", dir, name) < 0) virReportOOMError(); - return NULL; - } return ret; } @@ -15441,16 +15424,13 @@ virDomainObjListGetInactiveNames(virDomainObjListPtr doms, virHashForEach(doms->objs, virDomainObjListCopyInactiveNames, &data); virObjectUnlock(doms); if (data.oom) { + for (i = 0 ; i < data.numnames ; i++) + VIR_FREE(data.names[i]); virReportOOMError(); - goto cleanup; + return -1; } return data.numnames; - -cleanup: - for (i = 0 ; i < data.numnames ; i++) - VIR_FREE(data.names[i]); - return -1; } @@ -15620,8 +15600,7 @@ virDomainDefCopy(virCapsPtr caps, virDomainDefPtr src, bool migratable) write_flags |= VIR_DOMAIN_XML_INACTIVE | VIR_DOMAIN_XML_MIGRATABLE; /* Easiest to clone via a round-trip through XML. */ - xml = virDomainDefFormat(src, write_flags); - if (!xml) + if (!(xml = virDomainDefFormat(src, write_flags))) return NULL; ret = virDomainDefParseString(caps, xml, -1, read_flags); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 4ffa4aa..cce53ca 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -783,7 +783,7 @@ struct _virDomainFSDef { }; -/* 5 different types of networking config */ +/* network config types */ enum virDomainNetType { VIR_DOMAIN_NET_TYPE_USER, VIR_DOMAIN_NET_TYPE_ETHERNET, @@ -1420,7 +1420,7 @@ struct _virDomainMemballoonDef { enum virDomainSmbiosMode { - VIR_DOMAIN_SMBIOS_NONE, + VIR_DOMAIN_SMBIOS_NONE = 0, VIR_DOMAIN_SMBIOS_EMULATE, VIR_DOMAIN_SMBIOS_HOST, VIR_DOMAIN_SMBIOS_SYSINFO, -- 1.8.1.1

On 02/20/2013 12:06 PM, Peter Krempa wrote:
This patch changes many unrelated places to simplify the code or update code style. This patch should not have any semantic impact on the code. --- src/conf/domain_conf.c | 137 +++++++++++++++++++++---------------------------- src/conf/domain_conf.h | 4 +- 2 files changed, 60 insertions(+), 81 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 18df1bd..b4340f3 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -8150,9 +8150,9 @@ virDomainDeviceDefParse(virCapsPtr caps, xmlXPathContextPtr ctxt = NULL; virDomainDeviceDefPtr dev = NULL;
- if (!(xml = virXMLParseStringCtxt(xmlStr, _("(device_definition)"), &ctxt))) { + if (!(xml = virXMLParseStringCtxt(xmlStr, _("(device_definition)"), &ctxt))) goto error; - } + node = ctxt->node;
if (VIR_ALLOC(dev) < 0) { @@ -8219,20 +8219,18 @@ virDomainDeviceDefParse(virCapsPtr caps, if (!(dev->data.redirdev = virDomainRedirdevDefParseXML(node, NULL, flags))) goto error; } else { - virReportError(VIR_ERR_XML_ERROR, - "%s", _("unknown device type")); + virReportError(VIR_ERR_XML_ERROR, "%s", _("unknown device type")); goto error; }
+cleanup: xmlFreeDoc(xml); xmlXPathFreeContext(ctxt); return dev;
- error: - xmlFreeDoc(xml); - xmlXPathFreeContext(ctxt); +error: VIR_FREE(dev); - return NULL; + goto cleanup; }
@@ -9296,8 +9294,7 @@ virDomainDefParseXML(virCapsPtr caps, def->mem.cur_balloon = def->mem.max_balloon; }
- node = virXPathNode("./memoryBacking/hugepages", ctxt); - if (node) + if ((node = virXPathNode("./memoryBacking/hugepages", ctxt))) def->mem.hugepage_backed = true;
/* Extract blkio cgroup tunables */ @@ -9652,36 +9649,34 @@ virDomainDefParseXML(virCapsPtr caps, } VIR_FREE(nodes);
- n = virXPathNodeSet("./features/*", ctxt, &nodes); - if (n < 0) + if ((n = virXPathNodeSet("./features/*", ctxt, &nodes)) < 0) goto error; - if (n) { - for (i = 0 ; i < n ; i++) { - int val = virDomainFeatureTypeFromString((const char *)nodes[i]->name); - if (val < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unexpected feature %s"), - nodes[i]->name); - goto error; - } - def->features |= (1 << val); - if (val == VIR_DOMAIN_FEATURE_APIC) { - tmp = virXPathString("string(./features/apic/@eoi)", ctxt); - if (tmp) { - int eoi; - if ((eoi = virDomainFeatureStateTypeFromString(tmp)) <= 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unknown value for attribute eoi: %s"), - tmp); - goto error; - } - def->apic_eoi = eoi; - VIR_FREE(tmp); + + for (i = 0 ; i < n ; i++) { + int val = virDomainFeatureTypeFromString((const char *)nodes[i]->name); + if (val < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unexpected feature %s"), + nodes[i]->name); + goto error; + } + def->features |= (1 << val); + if (val == VIR_DOMAIN_FEATURE_APIC) { + tmp = virXPathString("string(./features/apic/@eoi)", ctxt); + if (tmp) { + int eoi; + if ((eoi = virDomainFeatureStateTypeFromString(tmp)) <= 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown value for attribute eoi: %s"), + tmp); + goto error; } + def->apic_eoi = eoi; + VIR_FREE(tmp); } } - VIR_FREE(nodes); } + VIR_FREE(nodes);
if (def->features & (1 << VIR_DOMAIN_FEATURE_HYPERV)) { int feature; @@ -9769,17 +9764,14 @@ virDomainDefParseXML(virCapsPtr caps, &def->pm.s4) < 0) goto error;
- tmp = virXPathString("string(./clock/@offset)", ctxt); - if (tmp) { - if ((def->clock.offset = virDomainClockOffsetTypeFromString(tmp)) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unknown clock offset '%s'"), tmp); - goto error; - } - VIR_FREE(tmp); - } else { - def->clock.offset = VIR_DOMAIN_CLOCK_OFFSET_UTC; + if ((tmp = virXPathString("string(./clock/@offset)", ctxt)) && + (def->clock.offset = virDomainClockOffsetTypeFromString(tmp)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unknown clock offset '%s'"), tmp); + goto error; } + VIR_FREE(tmp); + switch (def->clock.offset) { case VIR_DOMAIN_CLOCK_OFFSET_LOCALTIME: case VIR_DOMAIN_CLOCK_OFFSET_UTC: @@ -9838,11 +9830,12 @@ virDomainDefParseXML(virCapsPtr caps, break; }
- if ((n = virXPathNodeSet("./clock/timer", ctxt, &nodes)) < 0) { + if ((n = virXPathNodeSet("./clock/timer", ctxt, &nodes)) < 0) goto error; - } + if (n && VIR_ALLOC_N(def->clock.timers, n) < 0) goto no_memory; + for (i = 0 ; i < n ; i++) { virDomainTimerDefPtr timer = virDomainTimerDefParseXML(nodes[i], ctxt); @@ -10690,8 +10683,8 @@ virDomainDefParseXML(virCapsPtr caps, } } } - tmp = virXPathString("string(./os/smbios/@mode)", ctxt); - if (tmp) { + + if ((tmp = virXPathString("string(./os/smbios/@mode)", ctxt))) { int mode;
if ((mode = virDomainSmbiosModeTypeFromString(tmp)) < 0) { @@ -10701,27 +10694,22 @@ virDomainDefParseXML(virCapsPtr caps, } def->os.smbios_mode = mode; VIR_FREE(tmp); - } else { - def->os.smbios_mode = VIR_DOMAIN_SMBIOS_NONE; /* not present */ }
/* Extract custom metadata */ - if ((node = virXPathNode("./metadata[1]", ctxt)) != NULL) { + if ((node = virXPathNode("./metadata[1]", ctxt)) != NULL) def->metadata = xmlCopyNode(node, 1); - }
/* we have to make a copy of all of the callback pointers here since * we won't have the virCaps structure available during free */ def->ns = caps->ns;
- if (def->ns.parse) { - if ((def->ns.parse)(xml, root, ctxt, &def->namespaceData) < 0) - goto error; - } + if (def->ns.parse && + (def->ns.parse)(xml, root, ctxt, &def->namespaceData) < 0) + goto error;
I prefer adding braces when the conditional of a compound statement has multiple lines (yeah, I know this isn't in the official guidelines).
- /* Auto-add any implied controllers which aren't present - */ + /* Auto-add any implied controllers which aren't present */ if (virDomainDefAddImplicitControllers(def) < 0) goto error;
@@ -10731,9 +10719,7 @@ virDomainDefParseXML(virCapsPtr caps,
no_memory: virReportOOMError(); - /* fallthrough */ - - error: +error: VIR_FREE(tmp); VIR_FREE(nodes); virBitmapFree(bootMap); @@ -10898,8 +10884,7 @@ virDomainDefParseNode(virCapsPtr caps, goto cleanup; }
- ctxt = xmlXPathNewContext(xml); - if (ctxt == NULL) { + if (!(ctxt = xmlXPathNewContext(xml))) { virReportOOMError(); goto cleanup; } @@ -15028,8 +15013,7 @@ virDomainSaveConfig(const char *configDir, int ret = -1; char *xml;
- if (!(xml = virDomainDefFormat(def, - VIR_DOMAIN_XML_WRITE_FLAGS))) + if (!(xml = virDomainDefFormat(def, VIR_DOMAIN_XML_WRITE_FLAGS))) goto cleanup;
if (virDomainSaveXML(configDir, def, xml)) @@ -15238,7 +15222,8 @@ virDomainDeleteConfig(const char *configDir,
if ((configFile = virDomainConfigFile(configDir, dom->def->name)) == NULL) goto cleanup; - if ((autostartLink = virDomainConfigFile(autostartDir, dom->def->name)) == NULL) + if ((autostartLink = virDomainConfigFile(autostartDir, + dom->def->name)) == NULL) goto cleanup;
Same comment about multi-line conditions vs. braces.
/* Not fatal if this doesn't work */ @@ -15264,12 +15249,10 @@ char *virDomainConfigFile(const char *dir, const char *name) { - char *ret = NULL; + char *ret;
- if (virAsprintf(&ret, "%s/%s.xml", dir, name) < 0) { + if (virAsprintf(&ret, "%s/%s.xml", dir, name) < 0) virReportOOMError(); - return NULL; - }
return ret; } @@ -15441,16 +15424,13 @@ virDomainObjListGetInactiveNames(virDomainObjListPtr doms, virHashForEach(doms->objs, virDomainObjListCopyInactiveNames, &data); virObjectUnlock(doms); if (data.oom) { + for (i = 0 ; i < data.numnames ; i++) + VIR_FREE(data.names[i]); virReportOOMError(); - goto cleanup; + return -1; }
return data.numnames; - -cleanup: - for (i = 0 ; i < data.numnames ; i++) - VIR_FREE(data.names[i]); - return -1; }
@@ -15620,8 +15600,7 @@ virDomainDefCopy(virCapsPtr caps, virDomainDefPtr src, bool migratable) write_flags |= VIR_DOMAIN_XML_INACTIVE | VIR_DOMAIN_XML_MIGRATABLE;
/* Easiest to clone via a round-trip through XML. */ - xml = virDomainDefFormat(src, write_flags); - if (!xml) + if (!(xml = virDomainDefFormat(src, write_flags))) return NULL;
ret = virDomainDefParseString(caps, xml, -1, read_flags); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 4ffa4aa..cce53ca 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -783,7 +783,7 @@ struct _virDomainFSDef { };
-/* 5 different types of networking config */ +/* network config types */ enum virDomainNetType { VIR_DOMAIN_NET_TYPE_USER, VIR_DOMAIN_NET_TYPE_ETHERNET, @@ -1420,7 +1420,7 @@ struct _virDomainMemballoonDef {
enum virDomainSmbiosMode { - VIR_DOMAIN_SMBIOS_NONE, + VIR_DOMAIN_SMBIOS_NONE = 0, VIR_DOMAIN_SMBIOS_EMULATE, VIR_DOMAIN_SMBIOS_HOST, VIR_DOMAIN_SMBIOS_SYSINFO,
ACK (you can add the braces where I indicated or not - since it's not in the code style guidelines, it's a matter of taste :-)

Get rid of the "identical" variable in the ABI stability checks in favor of return statements and break or refactor very long lines where possible. --- src/conf/domain_conf.c | 469 +++++++++++++++++++++---------------------------- 1 file changed, 200 insertions(+), 269 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index b4340f3..8024bff 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -10957,21 +10957,19 @@ static bool virDomainTimerDefCheckABIStability(virDomainTimerDefPtr src, virDomainTimerDefPtr dst) { - bool identical = false; - if (src->name != dst->name) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Target timer %s does not match source %s"), virDomainTimerNameTypeToString(dst->name), virDomainTimerNameTypeToString(src->name)); - goto cleanup; + return false; } if (src->present != dst->present) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Target timer presence %d does not match source %d"), dst->present, src->present); - goto cleanup; + return false; } if (src->name == VIR_DOMAIN_TIMER_NAME_TSC) { @@ -10979,7 +10977,7 @@ virDomainTimerDefCheckABIStability(virDomainTimerDefPtr src, virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Target TSC frequency %lu does not match source %lu"), dst->frequency, src->frequency); - goto cleanup; + return false; } if (src->mode != dst->mode) { @@ -10987,14 +10985,11 @@ virDomainTimerDefCheckABIStability(virDomainTimerDefPtr src, _("Target TSC mode %s does not match source %s"), virDomainTimerModeTypeToString(dst->mode), virDomainTimerModeTypeToString(src->mode)); - goto cleanup; + return false; } } - identical = true; - -cleanup: - return identical; + return true; } @@ -11002,14 +10997,12 @@ static bool virDomainDeviceInfoCheckABIStability(virDomainDeviceInfoPtr src, virDomainDeviceInfoPtr dst) { - bool identical = false; - if (src->type != dst->type) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Target device address type %s does not match source %s"), virDomainDeviceAddressTypeToString(dst->type), virDomainDeviceAddressTypeToString(src->type)); - goto cleanup; + return false; } switch (src->type) { @@ -11025,7 +11018,7 @@ virDomainDeviceInfoCheckABIStability(virDomainDeviceInfoPtr src, dst->addr.pci.slot, dst->addr.pci.function, src->addr.pci.domain, src->addr.pci.bus, src->addr.pci.slot, src->addr.pci.function); - goto cleanup; + return false; } break; @@ -11034,12 +11027,13 @@ virDomainDeviceInfoCheckABIStability(virDomainDeviceInfoPtr src, src->addr.drive.bus != dst->addr.drive.bus || src->addr.drive.unit != dst->addr.drive.unit) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Target device drive address %d:%d:%d does not match source %d:%d:%d"), + _("Target device drive address %d:%d:%d " + "does not match source %d:%d:%d"), dst->addr.drive.controller, dst->addr.drive.bus, dst->addr.drive.unit, src->addr.drive.controller, src->addr.drive.bus, src->addr.drive.unit); - goto cleanup; + return false; } break; @@ -11048,12 +11042,13 @@ virDomainDeviceInfoCheckABIStability(virDomainDeviceInfoPtr src, src->addr.vioserial.bus != dst->addr.vioserial.bus || src->addr.vioserial.port != dst->addr.vioserial.port) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Target device virtio serial address %d:%d:%d does not match source %d:%d:%d"), + _("Target device virtio serial address %d:%d:%d " + "does not match source %d:%d:%d"), dst->addr.vioserial.controller, dst->addr.vioserial.bus, dst->addr.vioserial.port, src->addr.vioserial.controller, src->addr.vioserial.bus, src->addr.vioserial.port); - goto cleanup; + return false; } break; @@ -11061,20 +11056,18 @@ virDomainDeviceInfoCheckABIStability(virDomainDeviceInfoPtr src, if (src->addr.ccid.controller != dst->addr.ccid.controller || src->addr.ccid.slot != dst->addr.ccid.slot) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Target device ccid address %d:%d does not match source %d:%d"), + _("Target device ccid address %d:%d " + "does not match source %d:%d"), dst->addr.ccid.controller, dst->addr.ccid.slot, src->addr.ccid.controller, src->addr.ccid.slot); - goto cleanup; + return false; } break; } - identical = true; - -cleanup: - return identical; + return true; } @@ -11082,14 +11075,12 @@ static bool virDomainDiskDefCheckABIStability(virDomainDiskDefPtr src, virDomainDiskDefPtr dst) { - bool identical = false; - if (src->device != dst->device) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Target disk device %s does not match source %s"), virDomainDiskDeviceTypeToString(dst->device), virDomainDiskDeviceTypeToString(src->device)); - goto cleanup; + return false; } if (src->bus != dst->bus) { @@ -11097,36 +11088,33 @@ virDomainDiskDefCheckABIStability(virDomainDiskDefPtr src, _("Target disk bus %s does not match source %s"), virDomainDiskBusTypeToString(dst->bus), virDomainDiskBusTypeToString(src->bus)); - goto cleanup; + return false; } if (STRNEQ(src->dst, dst->dst)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Target disk %s does not match source %s"), dst->dst, src->dst); - goto cleanup; + return false; } if (STRNEQ_NULLABLE(src->serial, dst->serial)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Target disk serial %s does not match source %s"), NULLSTR(dst->serial), NULLSTR(src->serial)); - goto cleanup; + return false; } if (src->readonly != dst->readonly || src->shared != dst->shared) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("Target disk access mode does not match source")); - goto cleanup; + return false; } if (!virDomainDeviceInfoCheckABIStability(&src->info, &dst->info)) - goto cleanup; - - identical = true; + return false; -cleanup: - return identical; + return true; } @@ -11134,28 +11122,26 @@ static bool virDomainControllerDefCheckABIStability(virDomainControllerDefPtr src, virDomainControllerDefPtr dst) { - bool identical = false; - if (src->type != dst->type) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Target controller type %s does not match source %s"), virDomainControllerTypeToString(dst->type), virDomainControllerTypeToString(src->type)); - goto cleanup; + return false; } if (src->idx != dst->idx) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Target controller index %d does not match source %d"), dst->idx, src->idx); - goto cleanup; + return false; } if (src->model != dst->model) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Target controller model %d does not match source %d"), dst->model, src->model); - goto cleanup; + return false; } if (src->type == VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL) { @@ -11163,24 +11149,21 @@ virDomainControllerDefCheckABIStability(virDomainControllerDefPtr src, virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Target controller ports %d does not match source %d"), dst->opts.vioserial.ports, src->opts.vioserial.ports); - goto cleanup; + return false; } if (src->opts.vioserial.vectors != dst->opts.vioserial.vectors) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Target controller vectors %d does not match source %d"), dst->opts.vioserial.vectors, src->opts.vioserial.vectors); - goto cleanup; + return false; } } if (!virDomainDeviceInfoCheckABIStability(&src->info, &dst->info)) - goto cleanup; - - identical = true; + return false; -cleanup: - return identical; + return true; } @@ -11188,28 +11171,23 @@ static bool virDomainFsDefCheckABIStability(virDomainFSDefPtr src, virDomainFSDefPtr dst) { - bool identical = false; - if (STRNEQ(src->dst, dst->dst)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Target filesystem guest target %s does not match source %s"), dst->dst, src->dst); - goto cleanup; + return false; } if (src->readonly != dst->readonly) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("Target filesystem access mode does not match source")); - goto cleanup; + return false; } if (!virDomainDeviceInfoCheckABIStability(&src->info, &dst->info)) - goto cleanup; - - identical = true; + return false; -cleanup: - return identical; + return true; } @@ -11217,8 +11195,6 @@ static bool virDomainNetDefCheckABIStability(virDomainNetDefPtr src, virDomainNetDefPtr dst) { - bool identical = false; - if (virMacAddrCmp(&src->mac, &dst->mac) != 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Target network card mac %02x:%02x:%02x:%02x:%02x:%02x" @@ -11227,23 +11203,20 @@ virDomainNetDefCheckABIStability(virDomainNetDefPtr src, dst->mac.addr[3], dst->mac.addr[4], dst->mac.addr[5], src->mac.addr[0], src->mac.addr[1], src->mac.addr[2], src->mac.addr[3], src->mac.addr[4], src->mac.addr[5]); - goto cleanup; + return false; } if (STRNEQ_NULLABLE(src->model, dst->model)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Target network card model %s does not match source %s"), NULLSTR(dst->model), NULLSTR(src->model)); - goto cleanup; + return false; } if (!virDomainDeviceInfoCheckABIStability(&src->info, &dst->info)) - goto cleanup; - - identical = true; + return false; -cleanup: - return identical; + return true; } @@ -11251,14 +11224,12 @@ static bool virDomainInputDefCheckABIStability(virDomainInputDefPtr src, virDomainInputDefPtr dst) { - bool identical = false; - if (src->type != dst->type) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Target input device type %s does not match source %s"), virDomainInputTypeToString(dst->type), virDomainInputTypeToString(src->type)); - goto cleanup; + return false; } if (src->bus != dst->bus) { @@ -11266,16 +11237,13 @@ virDomainInputDefCheckABIStability(virDomainInputDefPtr src, _("Target input device bus %s does not match source %s"), virDomainInputBusTypeToString(dst->bus), virDomainInputBusTypeToString(src->bus)); - goto cleanup; + return false; } if (!virDomainDeviceInfoCheckABIStability(&src->info, &dst->info)) - goto cleanup; - - identical = true; + return false; -cleanup: - return identical; + return true; } @@ -11283,23 +11251,18 @@ static bool virDomainSoundDefCheckABIStability(virDomainSoundDefPtr src, virDomainSoundDefPtr dst) { - bool identical = false; - if (src->model != dst->model) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Target sound card model %s does not match source %s"), virDomainSoundModelTypeToString(dst->model), virDomainSoundModelTypeToString(src->model)); - goto cleanup; + return false; } if (!virDomainDeviceInfoCheckABIStability(&src->info, &dst->info)) - goto cleanup; - - identical = true; + return false; -cleanup: - return identical; + return true; } @@ -11307,35 +11270,33 @@ static bool virDomainVideoDefCheckABIStability(virDomainVideoDefPtr src, virDomainVideoDefPtr dst) { - bool identical = false; - if (src->type != dst->type) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Target video card model %s does not match source %s"), virDomainVideoTypeToString(dst->type), virDomainVideoTypeToString(src->type)); - goto cleanup; + return false; } if (src->vram != dst->vram) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Target video card vram %u does not match source %u"), dst->vram, src->vram); - goto cleanup; + return false; } if (src->heads != dst->heads) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Target video card heads %u does not match source %u"), dst->heads, src->heads); - goto cleanup; + return false; } if ((src->accel && !dst->accel) || (!src->accel && dst->accel)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("Target video card acceleration does not match source")); - goto cleanup; + return false; } if (src->accel) { @@ -11343,24 +11304,21 @@ virDomainVideoDefCheckABIStability(virDomainVideoDefPtr src, virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Target video card 2d accel %u does not match source %u"), dst->accel->support2d, src->accel->support2d); - goto cleanup; + return false; } if (src->accel->support3d != dst->accel->support3d) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Target video card 3d accel %u does not match source %u"), dst->accel->support3d, src->accel->support3d); - goto cleanup; + return false; } } if (!virDomainDeviceInfoCheckABIStability(&src->info, &dst->info)) - goto cleanup; - - identical = true; + return false; -cleanup: - return identical; + return true; } @@ -11368,33 +11326,27 @@ static bool virDomainHostdevDefCheckABIStability(virDomainHostdevDefPtr src, virDomainHostdevDefPtr dst) { - bool identical = false; - if (src->mode != dst->mode) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Target host device mode %s does not match source %s"), virDomainHostdevModeTypeToString(dst->mode), virDomainHostdevModeTypeToString(src->mode)); - goto cleanup; + return false; } - if (src->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) { - if (src->source.subsys.type != dst->source.subsys.type) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Target host device subsystem %s does not match source %s"), - virDomainHostdevSubsysTypeToString(dst->source.subsys.type), - virDomainHostdevSubsysTypeToString(src->source.subsys.type)); - goto cleanup; - } + if (src->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS && + src->source.subsys.type != dst->source.subsys.type) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Target host device subsystem %s does not match source %s"), + virDomainHostdevSubsysTypeToString(dst->source.subsys.type), + virDomainHostdevSubsysTypeToString(src->source.subsys.type)); + return false; } if (!virDomainDeviceInfoCheckABIStability(src->info, dst->info)) - goto cleanup; - - identical = true; + return false; -cleanup: - return identical; + return true; } @@ -11402,15 +11354,10 @@ static bool virDomainSmartcardDefCheckABIStability(virDomainSmartcardDefPtr src, virDomainSmartcardDefPtr dst) { - bool identical = false; - if (!virDomainDeviceInfoCheckABIStability(&src->info, &dst->info)) - goto cleanup; - - identical = true; + return false; -cleanup: - return identical; + return true; } @@ -11418,22 +11365,17 @@ static bool virDomainSerialDefCheckABIStability(virDomainChrDefPtr src, virDomainChrDefPtr dst) { - bool identical = false; - if (src->target.port != dst->target.port) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Target serial port %d does not match source %d"), dst->target.port, src->target.port); - goto cleanup; + return false; } if (!virDomainDeviceInfoCheckABIStability(&src->info, &dst->info)) - goto cleanup; - - identical = true; + return false; -cleanup: - return identical; + return true; } @@ -11441,22 +11383,17 @@ static bool virDomainParallelDefCheckABIStability(virDomainChrDefPtr src, virDomainChrDefPtr dst) { - bool identical = false; - if (src->target.port != dst->target.port) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Target serial port %d does not match source %d"), dst->target.port, src->target.port); - goto cleanup; + return false; } if (!virDomainDeviceInfoCheckABIStability(&src->info, &dst->info)) - goto cleanup; - - identical = true; + return false; -cleanup: - return identical; + return true; } @@ -11464,14 +11401,12 @@ static bool virDomainChannelDefCheckABIStability(virDomainChrDefPtr src, virDomainChrDefPtr dst) { - bool identical = false; - if (src->targetType != dst->targetType) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Target channel type %s does not match source %s"), virDomainChrChannelTargetTypeToString(dst->targetType), virDomainChrChannelTargetTypeToString(src->targetType)); - goto cleanup; + return false; } switch (src->targetType) { @@ -11480,7 +11415,7 @@ virDomainChannelDefCheckABIStability(virDomainChrDefPtr src, virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Target channel name %s does not match source %s"), NULLSTR(dst->target.name), NULLSTR(src->target.name)); - goto cleanup; + return false; } if (src->source.type != dst->source.type && (src->source.type == VIR_DOMAIN_CHR_TYPE_SPICEVMC || @@ -11489,7 +11424,7 @@ virDomainChannelDefCheckABIStability(virDomainChrDefPtr src, virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("Changing device type to/from spicevmc would" " change default target channel name")); - goto cleanup; + return false; } break; case VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_GUESTFWD: @@ -11502,18 +11437,15 @@ virDomainChannelDefCheckABIStability(virDomainChrDefPtr src, NULLSTR(daddr), NULLSTR(saddr)); VIR_FREE(saddr); VIR_FREE(daddr); - goto cleanup; + return false; } break; } if (!virDomainDeviceInfoCheckABIStability(&src->info, &dst->info)) - goto cleanup; - - identical = true; + return false; -cleanup: - return identical; + return true; } @@ -11521,23 +11453,18 @@ static bool virDomainConsoleDefCheckABIStability(virDomainChrDefPtr src, virDomainChrDefPtr dst) { - bool identical = false; - if (src->targetType != dst->targetType) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Target console type %s does not match source %s"), virDomainChrConsoleTargetTypeToString(dst->targetType), virDomainChrConsoleTargetTypeToString(src->targetType)); - goto cleanup; + return false; } if (!virDomainDeviceInfoCheckABIStability(&src->info, &dst->info)) - goto cleanup; - - identical = true; + return false; -cleanup: - return identical; + return true; } @@ -11545,23 +11472,18 @@ static bool virDomainWatchdogDefCheckABIStability(virDomainWatchdogDefPtr src, virDomainWatchdogDefPtr dst) { - bool identical = false; - if (src->model != dst->model) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Target watchdog model %s does not match source %s"), virDomainWatchdogModelTypeToString(dst->model), virDomainWatchdogModelTypeToString(src->model)); - goto cleanup; + return false; } if (!virDomainDeviceInfoCheckABIStability(&src->info, &dst->info)) - goto cleanup; - - identical = true; + return false; -cleanup: - return identical; + return true; } @@ -11569,23 +11491,18 @@ static bool virDomainMemballoonDefCheckABIStability(virDomainMemballoonDefPtr src, virDomainMemballoonDefPtr dst) { - bool identical = false; - if (src->model != dst->model) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Target balloon model %s does not match source %s"), virDomainMemballoonModelTypeToString(dst->model), virDomainMemballoonModelTypeToString(src->model)); - goto cleanup; + return false; } if (!virDomainDeviceInfoCheckABIStability(&src->info, &dst->info)) - goto cleanup; - - identical = true; + return false; -cleanup: - return identical; + return true; } @@ -11593,23 +11510,18 @@ static bool virDomainHubDefCheckABIStability(virDomainHubDefPtr src, virDomainHubDefPtr dst) { - bool identical = false; - if (src->type != dst->type) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Target hub device type %s does not match source %s"), virDomainHubTypeToString(dst->type), virDomainHubTypeToString(src->type)); - goto cleanup; + return false; } if (!virDomainDeviceInfoCheckABIStability(&src->info, &dst->info)) - goto cleanup; - - identical = true; + return false; -cleanup: - return identical; + return true; } static bool @@ -11617,14 +11529,13 @@ virDomainRedirFilterDefCheckABIStability(virDomainRedirFilterDefPtr src, virDomainRedirFilterDefPtr dst) { int i; - bool identical = false; if (src->nusbdevs != dst->nusbdevs) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Target USB redirection filter rule " "count %zu does not match source %zu"), dst->nusbdevs, src->nusbdevs); - goto cleanup; + return false; } for (i = 0; i < src->nusbdevs; i++) { @@ -11633,25 +11544,25 @@ virDomainRedirFilterDefCheckABIStability(virDomainRedirFilterDefPtr src, if (srcUsbDev->usbClass != dstUsbDev->usbClass) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("Target USB Class code does not match source")); - goto cleanup; + return false; } if (srcUsbDev->vendor != dstUsbDev->vendor) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("Target USB vendor ID does not match source")); - goto cleanup; + return false; } if (srcUsbDev->product != dstUsbDev->product) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("Target USB product ID does not match source")); - goto cleanup; + return false; } if (srcUsbDev->version != dstUsbDev->version) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("Target USB version does not match source")); - goto cleanup; + return false; } if (srcUsbDev->allow != dstUsbDev->allow) { @@ -11659,13 +11570,11 @@ virDomainRedirFilterDefCheckABIStability(virDomainRedirFilterDefPtr src, _("Target USB allow '%s' does not match source '%s'"), dstUsbDev->allow ? "yes" : "no", srcUsbDev->allow ? "yes" : "no"); - goto cleanup; + return false; } } - identical = true; -cleanup: - return identical; + return true; } /* This compares two configurations and looks for any differences @@ -11676,7 +11585,6 @@ bool virDomainDefCheckABIStability(virDomainDefPtr src, virDomainDefPtr dst) { - bool identical = false; int i; if (src->virtType != dst->virtType) { @@ -11684,7 +11592,7 @@ virDomainDefCheckABIStability(virDomainDefPtr src, _("Target domain virt type %s does not match source %s"), virDomainVirtTypeToString(dst->virtType), virDomainVirtTypeToString(src->virtType)); - goto cleanup; + return false; } if (memcmp(src->uuid, dst->uuid, VIR_UUID_BUFLEN) != 0) { @@ -11695,60 +11603,60 @@ virDomainDefCheckABIStability(virDomainDefPtr src, virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Target domain uuid %s does not match source %s"), uuiddst, uuidsrc); - goto cleanup; + return false; } if (src->mem.max_balloon != dst->mem.max_balloon) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Target domain max memory %lld does not match source %lld"), dst->mem.max_balloon, src->mem.max_balloon); - goto cleanup; + return false; } if (src->mem.cur_balloon != dst->mem.cur_balloon) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Target domain current memory %lld does not match source %lld"), dst->mem.cur_balloon, src->mem.cur_balloon); - goto cleanup; + return false; } if (src->mem.hugepage_backed != dst->mem.hugepage_backed) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Target domain huge page backing %d does not match source %d"), dst->mem.hugepage_backed, src->mem.hugepage_backed); - goto cleanup; + return false; } if (src->vcpus != dst->vcpus) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Target domain vpu count %d does not match source %d"), dst->vcpus, src->vcpus); - goto cleanup; + return false; } if (src->maxvcpus != dst->maxvcpus) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Target domain vpu max %d does not match source %d"), dst->maxvcpus, src->maxvcpus); - goto cleanup; + return false; } if (STRNEQ(src->os.type, dst->os.type)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Target domain OS type %s does not match source %s"), dst->os.type, src->os.type); - goto cleanup; + return false; } if (src->os.arch != dst->os.arch){ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Target domain architecture %s does not match source %s"), virArchToString(dst->os.arch), virArchToString(src->os.arch)); - goto cleanup; + return false; } if (STRNEQ(src->os.machine, dst->os.machine)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Target domain OS type %s does not match source %s"), dst->os.machine, src->os.machine); - goto cleanup; + return false; } if (src->os.smbios_mode != dst->os.smbios_mode) { @@ -11756,227 +11664,250 @@ virDomainDefCheckABIStability(virDomainDefPtr src, _("Target domain SMBIOS mode %s does not match source %s"), virDomainSmbiosModeTypeToString(dst->os.smbios_mode), virDomainSmbiosModeTypeToString(src->os.smbios_mode)); - goto cleanup; + return false; } if (src->features != dst->features) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Target domain features %d does not match source %d"), dst->features, src->features); - goto cleanup; + return false; } if (src->clock.ntimers != dst->clock.ntimers) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("Target domain timers do not match source")); - goto cleanup; + return false; } for (i = 0 ; i < src->clock.ntimers ; i++) { - if (!virDomainTimerDefCheckABIStability(src->clock.timers[i], dst->clock.timers[i])) - goto cleanup; + if (!virDomainTimerDefCheckABIStability(src->clock.timers[i], + dst->clock.timers[i])) + return false; } if (!virCPUDefIsEqual(src->cpu, dst->cpu)) - goto cleanup; + return false; if (!virSysinfoIsEqual(src->sysinfo, dst->sysinfo)) - goto cleanup; + return false; if (src->ndisks != dst->ndisks) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Target domain disk count %zu does not match source %zu"), dst->ndisks, src->ndisks); - goto cleanup; + return false; } for (i = 0 ; i < src->ndisks ; i++) if (!virDomainDiskDefCheckABIStability(src->disks[i], dst->disks[i])) - goto cleanup; + return false; if (src->ncontrollers != dst->ncontrollers) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Target domain disk controller count %zu does not match source %zu"), + _("Target domain disk controller count %zu " + "does not match source %zu"), dst->ncontrollers, src->ncontrollers); - goto cleanup; + return false; } for (i = 0 ; i < src->ncontrollers ; i++) - if (!virDomainControllerDefCheckABIStability(src->controllers[i], dst->controllers[i])) - goto cleanup; + if (!virDomainControllerDefCheckABIStability(src->controllers[i], + dst->controllers[i])) + return false; if (src->nfss != dst->nfss) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Target domain filesystem count %zu does not match source %zu"), + _("Target domain filesystem count %zu " + "does not match source %zu"), dst->nfss, src->nfss); - goto cleanup; + return false; } for (i = 0 ; i < src->nfss ; i++) if (!virDomainFsDefCheckABIStability(src->fss[i], dst->fss[i])) - goto cleanup; + return false; if (src->nnets != dst->nnets) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Target domain net card count %zu does not match source %zu"), + _("Target domain net card count %zu " + "does not match source %zu"), dst->nnets, src->nnets); - goto cleanup; + return false; } for (i = 0 ; i < src->nnets ; i++) if (!virDomainNetDefCheckABIStability(src->nets[i], dst->nets[i])) - goto cleanup; + return false; if (src->ninputs != dst->ninputs) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Target domain input device count %zu does not match source %zu"), + _("Target domain input device count %zu " + "does not match source %zu"), dst->ninputs, src->ninputs); - goto cleanup; + return false; } for (i = 0 ; i < src->ninputs ; i++) if (!virDomainInputDefCheckABIStability(src->inputs[i], dst->inputs[i])) - goto cleanup; + return false; if (src->nsounds != dst->nsounds) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Target domain sound card count %zu does not match source %zu"), + _("Target domain sound card count %zu " + "does not match source %zu"), dst->nsounds, src->nsounds); - goto cleanup; + return false; } for (i = 0 ; i < src->nsounds ; i++) if (!virDomainSoundDefCheckABIStability(src->sounds[i], dst->sounds[i])) - goto cleanup; + return false; if (src->nvideos != dst->nvideos) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Target domain video card count %zu does not match source %zu"), + _("Target domain video card count %zu " + "does not match source %zu"), dst->nvideos, src->nvideos); - goto cleanup; + return false; } for (i = 0 ; i < src->nvideos ; i++) if (!virDomainVideoDefCheckABIStability(src->videos[i], dst->videos[i])) - goto cleanup; + return false; if (src->nhostdevs != dst->nhostdevs) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Target domain host device count %zu does not match source %zu"), + _("Target domain host device count %zu " + "does not match source %zu"), dst->nhostdevs, src->nhostdevs); - goto cleanup; + return false; } for (i = 0 ; i < src->nhostdevs ; i++) - if (!virDomainHostdevDefCheckABIStability(src->hostdevs[i], dst->hostdevs[i])) - goto cleanup; + if (!virDomainHostdevDefCheckABIStability(src->hostdevs[i], + dst->hostdevs[i])) + return false; if (src->nsmartcards != dst->nsmartcards) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Target domain smartcard count %zu does not match source %zu"), + _("Target domain smartcard count %zu " + "does not match source %zu"), dst->nsmartcards, src->nsmartcards); - goto cleanup; + return false; } for (i = 0 ; i < src->nsmartcards ; i++) - if (!virDomainSmartcardDefCheckABIStability(src->smartcards[i], dst->smartcards[i])) - goto cleanup; + if (!virDomainSmartcardDefCheckABIStability(src->smartcards[i], + dst->smartcards[i])) + return false; if (src->nserials != dst->nserials) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Target domain serial port count %zu does not match source %zu"), + _("Target domain serial port count %zu " + "does not match source %zu"), dst->nserials, src->nserials); - goto cleanup; + return false; } for (i = 0 ; i < src->nserials ; i++) - if (!virDomainSerialDefCheckABIStability(src->serials[i], dst->serials[i])) - goto cleanup; + if (!virDomainSerialDefCheckABIStability(src->serials[i], + dst->serials[i])) + return false; if (src->nparallels != dst->nparallels) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Target domain parallel port count %zu does not match source %zu"), + _("Target domain parallel port count %zu " + "does not match source %zu"), dst->nparallels, src->nparallels); - goto cleanup; + return false; } for (i = 0 ; i < src->nparallels ; i++) - if (!virDomainParallelDefCheckABIStability(src->parallels[i], dst->parallels[i])) - goto cleanup; + if (!virDomainParallelDefCheckABIStability(src->parallels[i], + dst->parallels[i])) + return false; if (src->nchannels != dst->nchannels) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Target domain channel count %zu does not match source %zu"), + _("Target domain channel count %zu " + "does not match source %zu"), dst->nchannels, src->nchannels); - goto cleanup; + return false; } for (i = 0 ; i < src->nchannels ; i++) - if (!virDomainChannelDefCheckABIStability(src->channels[i], dst->channels[i])) - goto cleanup; + if (!virDomainChannelDefCheckABIStability(src->channels[i], + dst->channels[i])) + return false; if (src->nconsoles != dst->nconsoles) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Target domain console count %zu does not match source %zu"), + _("Target domain console count %zu " + "does not match source %zu"), dst->nconsoles, src->nconsoles); - goto cleanup; + return false; } for (i = 0 ; i < src->nconsoles ; i++) - if (!virDomainConsoleDefCheckABIStability(src->consoles[i], dst->consoles[i])) - goto cleanup; + if (!virDomainConsoleDefCheckABIStability(src->consoles[i], + dst->consoles[i])) + return false; if (src->nhubs != dst->nhubs) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Target domain hub device count %zu does not match source %zu"), + _("Target domain hub device count %zu " + "does not match source %zu"), dst->nhubs, src->nhubs); - goto cleanup; + return false; } for (i = 0 ; i < src->nhubs ; i++) if (!virDomainHubDefCheckABIStability(src->hubs[i], dst->hubs[i])) - goto cleanup; + return false; if ((!src->redirfilter && dst->redirfilter) || (src->redirfilter && !dst->redirfilter)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Target domain USB redirection filter count %d does not match source %d"), + _("Target domain USB redirection filter count %d " + "does not match source %d"), dst->redirfilter ? 1 : 0, src->redirfilter ? 1 : 0); - goto cleanup; + return false; } if (src->redirfilter && - !virDomainRedirFilterDefCheckABIStability(src->redirfilter, dst->redirfilter)) - goto cleanup; + !virDomainRedirFilterDefCheckABIStability(src->redirfilter, + dst->redirfilter)) + return false; if ((!src->watchdog && dst->watchdog) || (src->watchdog && !dst->watchdog)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Target domain watchdog count %d does not match source %d"), + _("Target domain watchdog count %d " + "does not match source %d"), dst->watchdog ? 1 : 0, src->watchdog ? 1 : 0); - goto cleanup; + return false; } if (src->watchdog && !virDomainWatchdogDefCheckABIStability(src->watchdog, dst->watchdog)) - goto cleanup; + return false; if ((!src->memballoon && dst->memballoon) || (src->memballoon && !dst->memballoon)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Target domain memory balloon count %d does not match source %d"), + _("Target domain memory balloon count %d " + "does not match source %d"), dst->memballoon ? 1 : 0, src->memballoon ? 1 : 0); - goto cleanup; + return false; } if (src->memballoon && - !virDomainMemballoonDefCheckABIStability(src->memballoon, dst->memballoon)) - goto cleanup; - - identical = true; + !virDomainMemballoonDefCheckABIStability(src->memballoon, + dst->memballoon)) + return false; -cleanup: - return identical; + return true; } -- 1.8.1.1

On 02/20/2013 12:06 PM, Peter Krempa wrote:
Get rid of the "identical" variable in the ABI stability checks in favor of return statements and break or refactor very long lines where possible. --- src/conf/domain_conf.c | 469 +++++++++++++++++++++---------------------------- 1 file changed, 200 insertions(+), 269 deletions(-)
ACK (although in general I have some misgivings about cosmetic changes like this, as they obscure the history of the modified lines, making it more tedious to sift through the past commit logs to understand why certain bits of code exist / what changed, etc).

On 02/20/13 18:44, Laine Stump wrote:
On 02/20/2013 12:06 PM, Peter Krempa wrote:
Get rid of the "identical" variable in the ABI stability checks in favor of return statements and break or refactor very long lines where possible. --- src/conf/domain_conf.c | 469 +++++++++++++++++++++---------------------------- 1 file changed, 200 insertions(+), 269 deletions(-)
ACK (although in general I have some misgivings about cosmetic changes like this, as they obscure the history of the modified lines, making it more tedious to sift through the past commit logs to understand why certain bits of code exist / what changed, etc).
Thankfully there's a script for vim (called fugitive) and for sure for other editors too that allows to browse the blame history. Thanks for the reviews, I pushed patches 1-6 and 8. Peter

On 02/21/13 15:27, Peter Krempa wrote:
On 02/20/13 18:44, Laine Stump wrote:
On 02/20/2013 12:06 PM, Peter Krempa wrote:
Get rid of the "identical" variable in the ABI stability checks in favor of return statements and break or refactor very long lines where possible. --- src/conf/domain_conf.c | 469 +++++++++++++++++++++---------------------------- 1 file changed, 200 insertions(+), 269 deletions(-)
ACK (although in general I have some misgivings about cosmetic changes like this, as they obscure the history of the modified lines, making it more tedious to sift through the past commit logs to understand why certain bits of code exist / what changed, etc).
Thankfully there's a script for vim (called fugitive) and for sure for other editors too that allows to browse the blame history.
Thanks for the reviews, I pushed patches 1-6 and 8.
Peter
Ping? Could anyone please have a look at the rest of the series. Peter

Make the iterator function usable in the next patches. Also refactor some parts to avoid strcmp if not necessary. --- src/conf/domain_conf.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 8024bff..421492f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2311,8 +2311,9 @@ int virDomainDeviceInfoIterate(virDomainDefPtr def, return -1; } for (i = 0; i < def->nconsoles ; i++) { - if ((STREQ(def->os.type, "hvm")) && i == 0 && - def->consoles[i]->targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL) + if (i == 0 && + def->consoles[i]->targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL && + STREQ_NULLABLE(def->os.type, "hvm")) continue; device.data.chr = def->consoles[i]; if (cb(def, &device, &def->consoles[i]->info, opaque) < 0) -- 1.8.1.1

On 02/20/2013 12:06 PM, Peter Krempa wrote:
Make the iterator function usable in the next patches. Also refactor some parts to avoid strcmp if not necessary. --- src/conf/domain_conf.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 8024bff..421492f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2311,8 +2311,9 @@ int virDomainDeviceInfoIterate(virDomainDefPtr def, return -1; } for (i = 0; i < def->nconsoles ; i++) { - if ((STREQ(def->os.type, "hvm")) && i == 0 && - def->consoles[i]->targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL) + if (i == 0 && + def->consoles[i]->targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL && + STREQ_NULLABLE(def->os.type, "hvm")) continue; device.data.chr = def->consoles[i]; if (cb(def, &device, &def->consoles[i]->info, opaque) < 0)
Since this bit of code seems so odd and aberrant, and your change is going to mask the original commit id that made it (and thus make it more difficult for someone in the future to learn *why* the code is so strange), I think you should mention the original commit (and maybe even a bit of its reasoning) in this patch's commit message. (The original patch is babe7d, and this was done because (just repeating the info in the commit message) the first console device for hvm domains is always an alias of a separately defined <serial> device, so this prevents the callback being called twice on the same device. If that information is incorrect, then maybe this code is also incorrect. (Certainly it would be good to eliminate it if we could, although I can't claim to have a concrete idea how to do that). (Interestingly, This same thing can also be true for <hostdev> and <interface>. Every <interface type='hostdev'> has a corresponding entry in the hostdev array (with the hostdev's virDeviceInfo *pointing* back at the interface's virDeviceInfo. However, in that case I took the strategy of checking for this duality during the body of any callback that cared, rather than polluting a general purpose iterator function with details of the items being iterated. So maybe that's a start on how to eliminate it) At any rate, ACK to the change; the re-ordering won't have any negative side effects, and using STREQ_NULLABLE seems like a good idea.

This patch adds instrumentation that will ultimately allow to split out filling of defaults and input validation from the XML parser to separate functions. With this patch, after the XML is parsed, a callback to the driver is issued requesing to fill and validate driver specific details of the configuration. This allows to use sensible defaults and checks on a per driver basis at the time the XML is parsed. Two callback pointers are exposed in the virCaps object: * virDriverDeviceDefCallback - called for a single device parsed and for every single device in a domain config. A virDomainDeviceDefPtr is passed. * virDriverDomainDefCallback - called if a domain definition is parsed device specific callbacks were called. A virDomainDefPtr is passed. Errors may be reported in those callbacks resulting in a XML parsing failure. Additionally two internal filler functions are added: virDomainDeviceDefUpdateDefaultsInternal and virDomainDefUpdateDefaultsInternal that are meant to be used for separating the validation and defaults assignment code from the parser function. --- src/conf/capabilities.h | 6 +++++ src/conf/domain_conf.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 78 insertions(+) diff --git a/src/conf/capabilities.h b/src/conf/capabilities.h index cc01765..56cd09f 100644 --- a/src/conf/capabilities.h +++ b/src/conf/capabilities.h @@ -171,6 +171,12 @@ struct _virCaps { bool hasWideScsiBus; const char *defaultInitPath; + + /* these callbacks are called after a XML definition of a device or domain + * is parsed. They are meant to validate and fill driver-specific values. */ + int (*virDriverDeviceDefCallback)(void *); /* virDomainDeviceDefPtr is passed */ + int (*virDriverDomainDefCallback)(void *); /* virDomainDefPtr is passed */ + virDomainXMLNamespace ns; }; diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 421492f..d881983 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2353,6 +2353,70 @@ int virDomainDeviceInfoIterate(virDomainDefPtr def, } +static int +virDomainDeviceDefUpdateDefaultsInternal(virDomainDeviceDefPtr dev ATTRIBUTE_UNUSED) +{ + /* not in use yet */ + return 0; +} + + +static int +virDomainDefUpdateDefaultsInternal(virDomainDefPtr def ATTRIBUTE_UNUSED, + virCapsPtr caps ATTRIBUTE_UNUSED) +{ + /* not in use yet */ + return 0; +} + + +static int +virDomainDeviceDefUpdateDefaults(virDomainDeviceDefPtr dev, + virCapsPtr caps) +{ + int ret; + /* at first call the driver callback to update the device */ + if (caps->virDriverDeviceDefCallback && + (ret = (caps->virDriverDeviceDefCallback)(dev)) < 0) + return ret; + + /* then fill the parse defaults and do the checks */ + return virDomainDeviceDefUpdateDefaultsInternal(dev); +} + + +static int +virDomainDefUpdateDefaultsDeviceIterator(virDomainDefPtr def ATTRIBUTE_UNUSED, + virDomainDeviceDefPtr dev, + virDomainDeviceInfoPtr info ATTRIBUTE_UNUSED, + void *opaque) +{ + virCapsPtr caps = opaque; + return virDomainDeviceDefUpdateDefaults(dev, caps); +} + + +static int +virDomainDefUpdateDefaults(virDomainDefPtr def, + virCapsPtr caps) +{ + int ret; + + /* at first update all the devices , unfortunately they are separate */ + if ((ret = virDomainDeviceInfoIterate(def, + virDomainDefUpdateDefaultsDeviceIterator, + caps)) < 0) + return ret; + + /* call the driver callback to update the rest of the definition */ + if (caps->virDriverDomainDefCallback && + (ret = (caps->virDriverDomainDefCallback)(def)) < 0) + return ret; + + return virDomainDefUpdateDefaultsInternal(def, caps); +} + + void virDomainDefClearPCIAddresses(virDomainDefPtr def) { virDomainDeviceInfoIterate(def, virDomainDeviceInfoClearPCIAddress, NULL); @@ -8224,6 +8288,10 @@ virDomainDeviceDefParse(virCapsPtr caps, goto error; } + /* callback to fill driver specific device aspects */ + if (virDomainDeviceDefUpdateDefaults(dev, caps) < 0) + goto error; + cleanup: xmlFreeDoc(xml); xmlXPathFreeContext(ctxt); @@ -10714,6 +10782,10 @@ virDomainDefParseXML(virCapsPtr caps, if (virDomainDefAddImplicitControllers(def) < 0) goto error; + /* callback to fill driver specific domain aspects */ + if (virDomainDefUpdateDefaults(def, caps) < 0) + goto error; + virBitmapFree(bootMap); return def; -- 1.8.1.1

On 02/20/2013 12:06 PM, Peter Krempa wrote:
This patch adds instrumentation that will ultimately allow to split out filling of defaults and input validation from the XML parser to separate functions.
With this patch, after the XML is parsed, a callback to the driver is issued requesing to fill and validate driver specific details of the configuration. This allows to use sensible defaults and checks on a per driver basis at the time the XML is parsed.
Two callback pointers are exposed in the virCaps object: * virDriverDeviceDefCallback - called for a single device parsed and for every single device in a domain config. A virDomainDeviceDefPtr is passed. * virDriverDomainDefCallback - called if a domain definition is parsed device specific callbacks were called. A virDomainDefPtr is passed.
Errors may be reported in those callbacks resulting in a XML parsing failure.
Additionally two internal filler functions are added: virDomainDeviceDefUpdateDefaultsInternal and virDomainDefUpdateDefaultsInternal that are meant to be used for separating the validation and defaults assignment code from the parser function. --- src/conf/capabilities.h | 6 +++++ src/conf/domain_conf.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 78 insertions(+)
diff --git a/src/conf/capabilities.h b/src/conf/capabilities.h index cc01765..56cd09f 100644 --- a/src/conf/capabilities.h +++ b/src/conf/capabilities.h @@ -171,6 +171,12 @@ struct _virCaps { bool hasWideScsiBus; const char *defaultInitPath;
+ + /* these callbacks are called after a XML definition of a device or domain + * is parsed. They are meant to validate and fill driver-specific values. */ + int (*virDriverDeviceDefCallback)(void *); /* virDomainDeviceDefPtr is passed */ + int (*virDriverDomainDefCallback)(void *); /* virDomainDefPtr is passed */
If you know that virDriverDeviceDefCallback is always given a virDomainDeviceDefPtr, why prototype it as void*? Same question for virDriverDomainDefCallback. Additionally, in the callback for a device, we will need to have the virDomainDefPtr (e.g. so that we can see what machinetype was selected for the domain). And in both of these callbacks we will need to get the virCapsPtr so that (for example), we can have access to information about which devices are available for which machine types, the default pci addresses of integrated devices, etc. So, I think the callbacks should be like this: int (*virDriverDeviceDefCallback) (virCapsPtr caps, virDomainDefPtr *domdef, virDomainDeviceDefPtr *devdef); int (*virDriverDomainDefCallback) (virCapsPtr caps, virDomainDefPtr *domdef);
+ virDomainXMLNamespace ns; };
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 421492f..d881983 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2353,6 +2353,70 @@ int virDomainDeviceInfoIterate(virDomainDefPtr def, }
+static int +virDomainDeviceDefUpdateDefaultsInternal(virDomainDeviceDefPtr dev ATTRIBUTE_UNUSED)
I don't think I agree with calling these functions "UpdateDefaults", because they could be used for any number of things, e.g. adding extra devices, creating bridges for extra buses, validating addresses, etc. Maybe they could be called virDomain*AdjustConfig*() or virDomain*PostProcess*().
+{ + /* not in use yet */ + return 0; +} + + +static int +virDomainDefUpdateDefaultsInternal(virDomainDefPtr def ATTRIBUTE_UNUSED, + virCapsPtr caps ATTRIBUTE_UNUSED) +{ + /* not in use yet */ + return 0; +} + + +static int +virDomainDeviceDefUpdateDefaults(virDomainDeviceDefPtr dev, + virCapsPtr caps) +{ + int ret; + /* at first call the driver callback to update the device */ + if (caps->virDriverDeviceDefCallback && + (ret = (caps->virDriverDeviceDefCallback)(dev)) < 0) + return ret; + + /* then fill the parse defaults and do the checks */ + return virDomainDeviceDefUpdateDefaultsInternal(dev); +} + + +static int +virDomainDefUpdateDefaultsDeviceIterator(virDomainDefPtr def ATTRIBUTE_UNUSED, + virDomainDeviceDefPtr dev, + virDomainDeviceInfoPtr info ATTRIBUTE_UNUSED, + void *opaque) +{ + virCapsPtr caps = opaque; + return virDomainDeviceDefUpdateDefaults(dev, caps); +} + + +static int +virDomainDefUpdateDefaults(virDomainDefPtr def, + virCapsPtr caps) +{ + int ret; + + /* at first update all the devices , unfortunately they are separate */ + if ((ret = virDomainDeviceInfoIterate(def, + virDomainDefUpdateDefaultsDeviceIterator, + caps)) < 0) + return ret; + + /* call the driver callback to update the rest of the definition */ + if (caps->virDriverDomainDefCallback && + (ret = (caps->virDriverDomainDefCallback)(def)) < 0) + return ret;
I find myself wondering if we're going to want to adjust the domain as a whole first, then the devices, or the individual devices, then the domain. I'm not sure which would be more useful. However, it does seem like the device callback is more likely to look at info for the domain (which we would probably want to be already adjusted at that time) than for the domain callback to want to look at individual devices. I could be wrong though...
+ + return virDomainDefUpdateDefaultsInternal(def, caps); +} + + void virDomainDefClearPCIAddresses(virDomainDefPtr def) { virDomainDeviceInfoIterate(def, virDomainDeviceInfoClearPCIAddress, NULL); @@ -8224,6 +8288,10 @@ virDomainDeviceDefParse(virCapsPtr caps, goto error; }
+ /* callback to fill driver specific device aspects */ + if (virDomainDeviceDefUpdateDefaults(dev, caps) < 0) + goto error; + cleanup: xmlFreeDoc(xml); xmlXPathFreeContext(ctxt); @@ -10714,6 +10782,10 @@ virDomainDefParseXML(virCapsPtr caps, if (virDomainDefAddImplicitControllers(def) < 0) goto error;
+ /* callback to fill driver specific domain aspects */ + if (virDomainDefUpdateDefaults(def, caps) < 0) + goto error; + virBitmapFree(bootMap);
return def;

On 03/01/2013 02:10 PM, Laine Stump wrote:
On 02/20/2013 12:06 PM, Peter Krempa wrote:
This patch adds instrumentation that will ultimately allow to split out filling of defaults and input validation from the XML parser to separate functions.
With this patch, after the XML is parsed, a callback to the driver is issued requesing to fill and validate driver specific details of the configuration. This allows to use sensible defaults and checks on a per driver basis at the time the XML is parsed.
Two callback pointers are exposed in the virCaps object: * virDriverDeviceDefCallback - called for a single device parsed and for every single device in a domain config. A virDomainDeviceDefPtr is passed. * virDriverDomainDefCallback - called if a domain definition is parsed device specific callbacks were called. A virDomainDefPtr is passed.
Errors may be reported in those callbacks resulting in a XML parsing failure.
Additionally two internal filler functions are added: virDomainDeviceDefUpdateDefaultsInternal and virDomainDefUpdateDefaultsInternal that are meant to be used for separating the validation and defaults assignment code from the parser function. --- src/conf/capabilities.h | 6 +++++ src/conf/domain_conf.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 78 insertions(+)
diff --git a/src/conf/capabilities.h b/src/conf/capabilities.h index cc01765..56cd09f 100644 --- a/src/conf/capabilities.h +++ b/src/conf/capabilities.h @@ -171,6 +171,12 @@ struct _virCaps { bool hasWideScsiBus; const char *defaultInitPath;
+ + /* these callbacks are called after a XML definition of a device or domain + * is parsed. They are meant to validate and fill driver-specific values. */ + int (*virDriverDeviceDefCallback)(void *); /* virDomainDeviceDefPtr is passed */ + int (*virDriverDomainDefCallback)(void *); /* virDomainDefPtr is passed */ If you know that virDriverDeviceDefCallback is always given a virDomainDeviceDefPtr, why prototype it as void*? Same question for virDriverDomainDefCallback.
Additionally, in the callback for a device, we will need to have the virDomainDefPtr (e.g. so that we can see what machinetype was selected for the domain). And in both of these callbacks we will need to get the virCapsPtr so that (for example), we can have access to information about which devices are available for which machine types, the default pci addresses of integrated devices, etc.
So, I think the callbacks should be like this:
int (*virDriverDeviceDefCallback) (virCapsPtr caps, virDomainDefPtr *domdef, virDomainDeviceDefPtr *devdef); int (*virDriverDomainDefCallback) (virCapsPtr caps, virDomainDefPtr *domdef);
+ virDomainXMLNamespace ns; };
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 421492f..d881983 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2353,6 +2353,70 @@ int virDomainDeviceInfoIterate(virDomainDefPtr def, }
+static int +virDomainDeviceDefUpdateDefaultsInternal(virDomainDeviceDefPtr dev ATTRIBUTE_UNUSED) I don't think I agree with calling these functions "UpdateDefaults", because they could be used for any number of things, e.g. adding extra devices, creating bridges for extra buses, validating addresses, etc. Maybe they could be called virDomain*AdjustConfig*() or virDomain*PostProcess*().
+{ + /* not in use yet */ + return 0; +} + + +static int +virDomainDefUpdateDefaultsInternal(virDomainDefPtr def ATTRIBUTE_UNUSED, + virCapsPtr caps ATTRIBUTE_UNUSED) +{ + /* not in use yet */ + return 0; +} + + +static int +virDomainDeviceDefUpdateDefaults(virDomainDeviceDefPtr dev, + virCapsPtr caps) +{ + int ret; + /* at first call the driver callback to update the device */ + if (caps->virDriverDeviceDefCallback && + (ret = (caps->virDriverDeviceDefCallback)(dev)) < 0) + return ret; + + /* then fill the parse defaults and do the checks */ + return virDomainDeviceDefUpdateDefaultsInternal(dev); +} + + +static int +virDomainDefUpdateDefaultsDeviceIterator(virDomainDefPtr def ATTRIBUTE_UNUSED, + virDomainDeviceDefPtr dev, + virDomainDeviceInfoPtr info ATTRIBUTE_UNUSED, + void *opaque) +{ + virCapsPtr caps = opaque; + return virDomainDeviceDefUpdateDefaults(dev, caps); +} + + +static int +virDomainDefUpdateDefaults(virDomainDefPtr def, + virCapsPtr caps) +{ + int ret; + + /* at first update all the devices , unfortunately they are separate */ + if ((ret = virDomainDeviceInfoIterate(def, + virDomainDefUpdateDefaultsDeviceIterator, + caps)) < 0) + return ret; + + /* call the driver callback to update the rest of the definition */ + if (caps->virDriverDomainDefCallback && + (ret = (caps->virDriverDomainDefCallback)(def)) < 0) + return ret; I find myself wondering if we're going to want to adjust the domain as a whole first, then the devices, or the individual devices, then the domain. I'm not sure which would be more useful. However, it does seem like the device callback is more likely to look at info for the domain (which we would probably want to be already adjusted at that time) than for the domain callback to want to look at individual devices. I could be wrong though...
And I've already found a counter-example. We will want the PCI addresses of the devices (the ones which don't have an explicitly confugured address anyway) to be assigned by the device callback, and it's true that that will need the domain info to be properly setup (at least machinetype). However, we'll want to go through and auto-create all the required extra bridges during the domain callback, but knowing which bridges we need to create requires that we have already assigned PCI addresses to all of the devices. :-/
+ + return virDomainDefUpdateDefaultsInternal(def, caps); +} + + void virDomainDefClearPCIAddresses(virDomainDefPtr def) { virDomainDeviceInfoIterate(def, virDomainDeviceInfoClearPCIAddress, NULL); @@ -8224,6 +8288,10 @@ virDomainDeviceDefParse(virCapsPtr caps, goto error; }
+ /* callback to fill driver specific device aspects */ + if (virDomainDeviceDefUpdateDefaults(dev, caps) < 0) + goto error; + cleanup: xmlFreeDoc(xml); xmlXPathFreeContext(ctxt); @@ -10714,6 +10782,10 @@ virDomainDefParseXML(virCapsPtr caps, if (virDomainDefAddImplicitControllers(def) < 0) goto error;
+ /* callback to fill driver specific domain aspects */ + if (virDomainDefUpdateDefaults(def, caps) < 0) + goto error; + virBitmapFree(bootMap);
return def; -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

On 03/01/13 20:10, Laine Stump wrote:
On 02/20/2013 12:06 PM, Peter Krempa wrote:
This patch adds instrumentation that will ultimately allow to split out filling of defaults and input validation from the XML parser to separate functions.
With this patch, after the XML is parsed, a callback to the driver is issued requesing to fill and validate driver specific details of the configuration. This allows to use sensible defaults and checks on a per driver basis at the time the XML is parsed.
Two callback pointers are exposed in the virCaps object: * virDriverDeviceDefCallback - called for a single device parsed and for every single device in a domain config. A virDomainDeviceDefPtr is passed. * virDriverDomainDefCallback - called if a domain definition is parsed device specific callbacks were called. A virDomainDefPtr is passed.
Errors may be reported in those callbacks resulting in a XML parsing failure.
Additionally two internal filler functions are added: virDomainDeviceDefUpdateDefaultsInternal and virDomainDefUpdateDefaultsInternal that are meant to be used for separating the validation and defaults assignment code from the parser function. --- src/conf/capabilities.h | 6 +++++ src/conf/domain_conf.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 78 insertions(+)
diff --git a/src/conf/capabilities.h b/src/conf/capabilities.h index cc01765..56cd09f 100644 --- a/src/conf/capabilities.h +++ b/src/conf/capabilities.h @@ -171,6 +171,12 @@ struct _virCaps { bool hasWideScsiBus; const char *defaultInitPath;
+ + /* these callbacks are called after a XML definition of a device or domain + * is parsed. They are meant to validate and fill driver-specific values. */ + int (*virDriverDeviceDefCallback)(void *); /* virDomainDeviceDefPtr is passed */ + int (*virDriverDomainDefCallback)(void *); /* virDomainDefPtr is passed */
If you know that virDriverDeviceDefCallback is always given a virDomainDeviceDefPtr, why prototype it as void*? Same question for virDriverDomainDefCallback.
Those two types are defined in conf/domain_conf.h. conf/domain_conf.h in return needs conf/capabilities.h. This creates a circular dependency chain if I try to include domain_conf.h in capabilities.h. I tried to to come up with a different solution than void * but none of the others were nicer than that. The options were: 1) use a separate header file for one of the types 2) use a extern declaration 3) include domain_conf.h into capabilities.h after all needed types were declared. I'll welcome any tips how to solve this problem. (cc-d Dan and Eric).
Additionally, in the callback for a device, we will need to have the virDomainDefPtr (e.g. so that we can see what machinetype was selected for the domain). And in both of these callbacks we will need to get the virCapsPtr so that (for example), we can have access to information about which devices are available for which machine types, the default pci addresses of integrated devices, etc.
So, I think the callbacks should be like this:
int (*virDriverDeviceDefCallback) (virCapsPtr caps, virDomainDefPtr *domdef, virDomainDeviceDefPtr *devdef); int (*virDriverDomainDefCallback) (virCapsPtr caps, virDomainDefPtr *domdef);
Yep, seems reasonable to me. Peter

On Mon, Mar 04, 2013 at 04:04:43PM +0100, Peter Krempa wrote:
On 03/01/13 20:10, Laine Stump wrote:
On 02/20/2013 12:06 PM, Peter Krempa wrote:
This patch adds instrumentation that will ultimately allow to split out filling of defaults and input validation from the XML parser to separate functions.
With this patch, after the XML is parsed, a callback to the driver is issued requesing to fill and validate driver specific details of the configuration. This allows to use sensible defaults and checks on a per driver basis at the time the XML is parsed.
Two callback pointers are exposed in the virCaps object: * virDriverDeviceDefCallback - called for a single device parsed and for every single device in a domain config. A virDomainDeviceDefPtr is passed. * virDriverDomainDefCallback - called if a domain definition is parsed device specific callbacks were called. A virDomainDefPtr is passed.
Errors may be reported in those callbacks resulting in a XML parsing failure.
Additionally two internal filler functions are added: virDomainDeviceDefUpdateDefaultsInternal and virDomainDefUpdateDefaultsInternal that are meant to be used for separating the validation and defaults assignment code from the parser function. --- src/conf/capabilities.h | 6 +++++ src/conf/domain_conf.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 78 insertions(+)
diff --git a/src/conf/capabilities.h b/src/conf/capabilities.h index cc01765..56cd09f 100644 --- a/src/conf/capabilities.h +++ b/src/conf/capabilities.h @@ -171,6 +171,12 @@ struct _virCaps { bool hasWideScsiBus; const char *defaultInitPath;
+ + /* these callbacks are called after a XML definition of a device or domain + * is parsed. They are meant to validate and fill driver-specific values. */ + int (*virDriverDeviceDefCallback)(void *); /* virDomainDeviceDefPtr is passed */ + int (*virDriverDomainDefCallback)(void *); /* virDomainDefPtr is passed */
If you know that virDriverDeviceDefCallback is always given a virDomainDeviceDefPtr, why prototype it as void*? Same question for virDriverDomainDefCallback.
Those two types are defined in conf/domain_conf.h. conf/domain_conf.h in return needs conf/capabilities.h. This creates a circular dependency chain if I try to include domain_conf.h in capabilities.h. I tried to to come up with a different solution than void * but none of the others were nicer than that. The options were:
1) use a separate header file for one of the types 2) use a extern declaration 3) include domain_conf.h into capabilities.h after all needed types were declared.
I'll welcome any tips how to solve this problem. (cc-d Dan and Eric).
This says to me that capabilities.h is not the right place for this data. Originally virCapabilities was just used to hold information about the guests/machine types / etc supported by a driver. Then we extended the domain XML parsers to require this object to be passed in. Then we added namespace hooks to it. Now we're adding yet more stuff. IMHO it is time to undo this mess. We need a virCapabilities object that just contains the data associated with the capabilities XML schema, nothing more. Then we need a virDomainParserConfigPtr struct that contains the information required by the virDomainDef parsers. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

This patch implements the callback that is used to fill the qemu default network card into the XML if none is provided in the definition. --- src/qemu/qemu_conf.c | 1 + src/qemu/qemu_domain.c | 23 +++++++++++++++++++++++ src/qemu/qemu_domain.h | 1 + 3 files changed, 25 insertions(+) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index a2a05d4..cc6a738 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -556,6 +556,7 @@ virCapsPtr virQEMUDriverCreateCapabilities(virQEMUDriverPtr driver) qemuDomainSetPrivateDataHooks(caps); qemuDomainSetNamespaceHooks(caps); + qemuDomainSetDefHooks(caps); if (virGetHostUUID(caps->host.host_uuid)) { virReportError(VIR_ERR_INTERNAL_ERROR, diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 482f64a..7c8768b 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -633,6 +633,24 @@ qemuDomainDefNamespaceFormatXML(virBufferPtr buf, return 0; } + +static int +qemuDomainDeviceDefCallback(void *device) +{ + virDomainDeviceDefPtr dev = device; + + if (dev->type == VIR_DOMAIN_DEVICE_NET) { + if (!dev->data.net->model) { + if (!(dev->data.net->model = strdup("rtl8139"))) { + virReportOOMError(); + return -1; + } + } + } + return 0; +} + + static const char * qemuDomainDefNamespaceHref(void) { @@ -659,6 +677,11 @@ void qemuDomainSetNamespaceHooks(virCapsPtr caps) caps->ns.href = qemuDomainDefNamespaceHref; } +void qemuDomainSetDefHooks(virCapsPtr caps) +{ + caps->virDriverDeviceDefCallback = qemuDomainDeviceDefCallback; +} + static void qemuDomainObjSaveJob(virQEMUDriverPtr driver, virDomainObjPtr obj) { diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 905b099..210cffd 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -180,6 +180,7 @@ void qemuDomainEventQueue(virQEMUDriverPtr driver, void qemuDomainSetPrivateDataHooks(virCapsPtr caps); void qemuDomainSetNamespaceHooks(virCapsPtr caps); +void qemuDomainSetDefHooks(virCapsPtr caps); int qemuDomainObjBeginJob(virQEMUDriverPtr driver, virDomainObjPtr obj, -- 1.8.1.1

On 02/20/2013 12:06 PM, Peter Krempa wrote:
This patch implements the callback that is used to fill the qemu default network card into the XML if none is provided in the definition. --- src/qemu/qemu_conf.c | 1 + src/qemu/qemu_domain.c | 23 +++++++++++++++++++++++ src/qemu/qemu_domain.h | 1 + 3 files changed, 25 insertions(+)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index a2a05d4..cc6a738 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -556,6 +556,7 @@ virCapsPtr virQEMUDriverCreateCapabilities(virQEMUDriverPtr driver)
qemuDomainSetPrivateDataHooks(caps); qemuDomainSetNamespaceHooks(caps); + qemuDomainSetDefHooks(caps);
if (virGetHostUUID(caps->host.host_uuid)) { virReportError(VIR_ERR_INTERNAL_ERROR, diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 482f64a..7c8768b 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -633,6 +633,24 @@ qemuDomainDefNamespaceFormatXML(virBufferPtr buf, return 0; }
+ +static int +qemuDomainDeviceDefCallback(void *device) +{ + virDomainDeviceDefPtr dev = device; + + if (dev->type == VIR_DOMAIN_DEVICE_NET) { + if (!dev->data.net->model) { + if (!(dev->data.net->model = strdup("rtl8139"))) { + virReportOOMError(); + return -1; + } + } + } + return 0; +}
This is a good example of why the domain pointer and caps pointer are needed in the callback. The default netdev model cuold very well change depending on which hypervisor is used, the version/capabilities of the hypervisor, and the machinetype.
+ + static const char * qemuDomainDefNamespaceHref(void) { @@ -659,6 +677,11 @@ void qemuDomainSetNamespaceHooks(virCapsPtr caps) caps->ns.href = qemuDomainDefNamespaceHref; }
+void qemuDomainSetDefHooks(virCapsPtr caps) +{ + caps->virDriverDeviceDefCallback = qemuDomainDeviceDefCallback; +} + static void qemuDomainObjSaveJob(virQEMUDriverPtr driver, virDomainObjPtr obj) { diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 905b099..210cffd 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -180,6 +180,7 @@ void qemuDomainEventQueue(virQEMUDriverPtr driver,
void qemuDomainSetPrivateDataHooks(virCapsPtr caps); void qemuDomainSetNamespaceHooks(virCapsPtr caps); +void qemuDomainSetDefHooks(virCapsPtr caps);
int qemuDomainObjBeginJob(virQEMUDriverPtr driver, virDomainObjPtr obj,

On 03/01/2013 02:13 PM, Laine Stump wrote:
On 02/20/2013 12:06 PM, Peter Krempa wrote:
This patch implements the callback that is used to fill the qemu default network card into the XML if none is provided in the definition. --- src/qemu/qemu_conf.c | 1 + src/qemu/qemu_domain.c | 23 +++++++++++++++++++++++ src/qemu/qemu_domain.h | 1 + 3 files changed, 25 insertions(+)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index a2a05d4..cc6a738 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -556,6 +556,7 @@ virCapsPtr virQEMUDriverCreateCapabilities(virQEMUDriverPtr driver)
qemuDomainSetPrivateDataHooks(caps); qemuDomainSetNamespaceHooks(caps); + qemuDomainSetDefHooks(caps);
if (virGetHostUUID(caps->host.host_uuid)) { virReportError(VIR_ERR_INTERNAL_ERROR, diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 482f64a..7c8768b 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -633,6 +633,24 @@ qemuDomainDefNamespaceFormatXML(virBufferPtr buf, return 0; }
+ +static int +qemuDomainDeviceDefCallback(void *device) +{ + virDomainDeviceDefPtr dev = device; + + if (dev->type == VIR_DOMAIN_DEVICE_NET) { + if (!dev->data.net->model) { + if (!(dev->data.net->model = strdup("rtl8139"))) { + virReportOOMError(); + return -1;
Also, if the net->type is hostdev, then model should not be set at all (although it is harmlessly ignored, it could be confusing).
+ } + } + } + return 0; +} This is a good example of why the domain pointer and caps pointer are needed in the callback. The default netdev model cuold very well change depending on which hypervisor is used, the version/capabilities of the hypervisor, and the machinetype.
+ + static const char * qemuDomainDefNamespaceHref(void) { @@ -659,6 +677,11 @@ void qemuDomainSetNamespaceHooks(virCapsPtr caps) caps->ns.href = qemuDomainDefNamespaceHref; }
+void qemuDomainSetDefHooks(virCapsPtr caps) +{ + caps->virDriverDeviceDefCallback = qemuDomainDeviceDefCallback; +} + static void qemuDomainObjSaveJob(virQEMUDriverPtr driver, virDomainObjPtr obj) { diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 905b099..210cffd 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -180,6 +180,7 @@ void qemuDomainEventQueue(virQEMUDriverPtr driver,
void qemuDomainSetPrivateDataHooks(virCapsPtr caps); void qemuDomainSetNamespaceHooks(virCapsPtr caps); +void qemuDomainSetDefHooks(virCapsPtr caps);
int qemuDomainObjBeginJob(virQEMUDriverPtr driver, virDomainObjPtr obj, -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

--- tests/qemuxml2argvdata/qemuxml2argv-net-bandwidth.xml | 1 + tests/qemuxml2argvdata/qemuxml2argv-net-client.args | 6 +----- tests/qemuxml2argvdata/qemuxml2argv-net-eth-ifname.args | 6 +----- tests/qemuxml2argvdata/qemuxml2argv-net-eth-ifname.xml | 1 + tests/qemuxml2argvdata/qemuxml2argv-net-eth-names.args | 8 +------- tests/qemuxml2argvdata/qemuxml2argv-net-eth.args | 6 +----- tests/qemuxml2argvdata/qemuxml2argv-net-eth.xml | 1 + tests/qemuxml2argvdata/qemuxml2argv-net-hostdev.xml | 1 + tests/qemuxml2argvdata/qemuxml2argv-net-mcast.args | 6 +----- tests/qemuxml2argvdata/qemuxml2argv-net-openvswitch.xml | 1 + tests/qemuxml2argvdata/qemuxml2argv-net-server.args | 6 +----- tests/qemuxml2argvdata/qemuxml2argv-net-user.args | 5 +---- tests/qemuxml2argvdata/qemuxml2argv-net-user.xml | 1 + .../qemuxml2argv-net-virtio-network-portgroup.xml | 2 ++ .../qemuxml2xmloutdata/qemuxml2xmlout-graphics-spice-timeout.xml | 1 + tests/testutilsqemu.c | 1 + 16 files changed, 17 insertions(+), 36 deletions(-) diff --git a/tests/qemuxml2argvdata/qemuxml2argv-net-bandwidth.xml b/tests/qemuxml2argvdata/qemuxml2argv-net-bandwidth.xml index bf7dde5..885e854 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-net-bandwidth.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-net-bandwidth.xml @@ -44,6 +44,7 @@ <interface type='network'> <mac address='52:54:00:24:a5:9f'/> <source network='default'/> + <model type='rtl8139'/> <bandwidth> <inbound average='1000' peak='4000' burst='1024'/> <outbound average='128' peak='256' burst='32768'/> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-net-client.args b/tests/qemuxml2argvdata/qemuxml2argv-net-client.args index 7974f2e..64022d9 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-net-client.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-net-client.args @@ -1,5 +1 @@ -LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M \ -pc -m 214 -smp 1 -nographic -monitor unix:/tmp/test-monitor,server,nowait \ --no-acpi -boot c -usb -hda /dev/HostVG/QEMUGuest1 -net nic,\ -macaddr=52:54:00:8c:b9:05,vlan=0 -net socket,connect=192.168.0.1:5558,vlan=0 \ --serial none -parallel none +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -usb -hda /dev/HostVG/QEMUGuest1 -net nic,macaddr=52:54:00:8c:b9:05,vlan=0,model=rtl8139 -net socket,connect=192.168.0.1:5558,vlan=0 -serial none -parallel none diff --git a/tests/qemuxml2argvdata/qemuxml2argv-net-eth-ifname.args b/tests/qemuxml2argvdata/qemuxml2argv-net-eth-ifname.args index cced5d5..67bf045 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-net-eth-ifname.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-net-eth-ifname.args @@ -1,5 +1 @@ -LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M \ -pc -m 214 -smp 1 -nographic -monitor unix:/tmp/test-monitor,server,nowait \ --no-acpi -boot c -usb -hda /dev/HostVG/QEMUGuest1 -net nic,\ -macaddr=00:11:22:33:44:55,vlan=0 -net tap,ifname=nic02,script=/etc/qemu-ifup,\ -vlan=0 -serial none -parallel none +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -usb -hda /dev/HostVG/QEMUGuest1 -net nic,macaddr=00:11:22:33:44:55,vlan=0,model=rtl8139 -net tap,ifname=nic02,script=/etc/qemu-ifup,vlan=0 -serial none -parallel none diff --git a/tests/qemuxml2argvdata/qemuxml2argv-net-eth-ifname.xml b/tests/qemuxml2argvdata/qemuxml2argv-net-eth-ifname.xml index 04a4ca4..b150371 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-net-eth-ifname.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-net-eth-ifname.xml @@ -25,6 +25,7 @@ <mac address='00:11:22:33:44:55'/> <script path='/etc/qemu-ifup'/> <target dev='nic02'/> + <model type='rtl8139'/> </interface> <memballoon model='virtio'/> </devices> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-net-eth-names.args b/tests/qemuxml2argvdata/qemuxml2argv-net-eth-names.args index dc15f63..73e5d5e 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-net-eth-names.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-net-eth-names.args @@ -1,7 +1 @@ -LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M \ -pc -m 214 -smp 1 -nographic -monitor unix:/tmp/test-monitor,server,nowait \ --no-acpi -boot c -usb -hda /dev/HostVG/QEMUGuest1 -net nic,\ -macaddr=00:11:22:33:44:55,vlan=0,name=net0 -net tap,script=/etc/qemu-ifup,\ -vlan=0,name=hostnet0 -net nic,macaddr=00:11:22:33:44:56,vlan=1,model=e1000,\ -name=net1 -net tap,script=/etc/qemu-ifup,vlan=1,name=hostnet1 -serial none \ --parallel none +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -usb -hda /dev/HostVG/QEMUGuest1 -net nic,macaddr=00:11:22:33:44:55,vlan=0,model=rtl8139,name=net0 -net tap,script=/etc/qemu-ifup,vlan=0,name=hostnet0 -net nic,macaddr=00:11:22:33:44:56,vlan=1,model=e1000,name=net1 -net tap,script=/etc/qemu-ifup,vlan=1,name=hostnet1 -serial none -parallel none diff --git a/tests/qemuxml2argvdata/qemuxml2argv-net-eth.args b/tests/qemuxml2argvdata/qemuxml2argv-net-eth.args index a482193..2bb556e 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-net-eth.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-net-eth.args @@ -1,5 +1 @@ -LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M \ -pc -m 214 -smp 1 -nographic -monitor unix:/tmp/test-monitor,server,nowait \ --no-acpi -boot c -usb -hda /dev/HostVG/QEMUGuest1 -net nic,\ -macaddr=00:11:22:33:44:55,vlan=0 -net tap,script=/etc/qemu-ifup,vlan=0 -serial \ -none -parallel none +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -usb -hda /dev/HostVG/QEMUGuest1 -net nic,macaddr=00:11:22:33:44:55,vlan=0,model=rtl8139 -net tap,script=/etc/qemu-ifup,vlan=0 -serial none -parallel none diff --git a/tests/qemuxml2argvdata/qemuxml2argv-net-eth.xml b/tests/qemuxml2argvdata/qemuxml2argv-net-eth.xml index 87dd65f..eca5da5 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-net-eth.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-net-eth.xml @@ -24,6 +24,7 @@ <interface type='ethernet'> <mac address='00:11:22:33:44:55'/> <script path='/etc/qemu-ifup'/> + <model type='rtl8139'/> </interface> <memballoon model='virtio'/> </devices> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-net-hostdev.xml b/tests/qemuxml2argvdata/qemuxml2argv-net-hostdev.xml index 81f70d0..9be0d2d 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-net-hostdev.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-net-hostdev.xml @@ -32,6 +32,7 @@ <virtualport type='802.1Qbg'> <parameters managerid='11' typeid='1193047' typeidversion='2' instanceid='09b11c53-8b5c-4eeb-8f00-d84eaa0aaa4f'/> </virtualport> + <model type='rtl8139'/> </interface> <memballoon model='virtio'/> </devices> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-net-mcast.args b/tests/qemuxml2argvdata/qemuxml2argv-net-mcast.args index fc2091b..6a406b5 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-net-mcast.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-net-mcast.args @@ -1,5 +1 @@ -LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M \ -pc -m 214 -smp 1 -nographic -monitor unix:/tmp/test-monitor,server,nowait \ --no-acpi -boot c -usb -hda /dev/HostVG/QEMUGuest1 -net nic,\ -macaddr=52:54:00:8c:b9:05,vlan=0 -net socket,mcast=192.0.0.1:5558,vlan=0 \ --serial none -parallel none +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -usb -hda /dev/HostVG/QEMUGuest1 -net nic,macaddr=52:54:00:8c:b9:05,vlan=0,model=rtl8139 -net socket,mcast=192.0.0.1:5558,vlan=0 -serial none -parallel none diff --git a/tests/qemuxml2argvdata/qemuxml2argv-net-openvswitch.xml b/tests/qemuxml2argvdata/qemuxml2argv-net-openvswitch.xml index ff09844..9c2c5dc 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-net-openvswitch.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-net-openvswitch.xml @@ -32,6 +32,7 @@ <virtualport type='openvswitch'> <parameters interfaceid='09b11c53-8b5c-4eeb-8f00-d84eaa0aaa4f' profileid='bob'/> </virtualport> + <model type='rtl8139'/> </interface> <memballoon model='virtio'/> </devices> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-net-server.args b/tests/qemuxml2argvdata/qemuxml2argv-net-server.args index 7c9d735..6b0c6d0 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-net-server.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-net-server.args @@ -1,5 +1 @@ -LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M \ -pc -m 214 -smp 1 -nographic -monitor unix:/tmp/test-monitor,server,nowait \ --no-acpi -boot c -usb -hda /dev/HostVG/QEMUGuest1 -net nic,\ -macaddr=52:54:00:8c:b9:05,vlan=0 -net socket,listen=192.168.0.1:5558,vlan=0 \ --serial none -parallel none +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -usb -hda /dev/HostVG/QEMUGuest1 -net nic,macaddr=52:54:00:8c:b9:05,vlan=0,model=rtl8139 -net socket,listen=192.168.0.1:5558,vlan=0 -serial none -parallel none diff --git a/tests/qemuxml2argvdata/qemuxml2argv-net-user.args b/tests/qemuxml2argvdata/qemuxml2argv-net-user.args index 7364281..23f286d 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-net-user.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-net-user.args @@ -1,4 +1 @@ -LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M \ -pc -m 214 -smp 1 -nographic -monitor unix:/tmp/test-monitor,server,nowait \ --no-acpi -boot c -usb -hda /dev/HostVG/QEMUGuest1 -net nic,\ -macaddr=00:11:22:33:44:55,vlan=0 -net user,vlan=0 -serial none -parallel none +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -usb -hda /dev/HostVG/QEMUGuest1 -net nic,macaddr=00:11:22:33:44:55,vlan=0,model=rtl8139 -net user,vlan=0 -serial none -parallel none diff --git a/tests/qemuxml2argvdata/qemuxml2argv-net-user.xml b/tests/qemuxml2argvdata/qemuxml2argv-net-user.xml index 37e5edf..fe3a271 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-net-user.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-net-user.xml @@ -23,6 +23,7 @@ <controller type='ide' index='0'/> <interface type='user'> <mac address='00:11:22:33:44:55'/> + <model type='rtl8139'/> </interface> <memballoon model='virtio'/> </devices> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-network-portgroup.xml b/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-network-portgroup.xml index c84ed3f..0fb9b2c 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-network-portgroup.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-network-portgroup.xml @@ -38,6 +38,7 @@ <virtualport> <parameters instanceid='09b11c53-8b5c-4eeb-8f00-d84eaa0aaa4f' interfaceid='09b11c53-8b5c-4eeb-8f00-d84eaa0aaa4f'/> </virtualport> + <model type='rtl8139'/> </interface> <interface type='network'> <mac address='22:11:22:33:44:55'/> @@ -45,6 +46,7 @@ <virtualport type='802.1Qbh'> <parameters profileid='testhis99'/> </virtualport> + <model type='rtl8139'/> </interface> <memballoon model='virtio'/> </devices> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-graphics-spice-timeout.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-graphics-spice-timeout.xml index cd19b64..54b68fa 100644 --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-graphics-spice-timeout.xml +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-graphics-spice-timeout.xml @@ -62,6 +62,7 @@ <interface type='ethernet'> <mac address='52:54:00:71:70:89'/> <script path='/etc/qemu-ifup'/> + <model type='rtl8139'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x07' function='0x0'/> </interface> <serial type='pty'> diff --git a/tests/testutilsqemu.c b/tests/testutilsqemu.c index 966527c..0d9fb1d 100644 --- a/tests/testutilsqemu.c +++ b/tests/testutilsqemu.c @@ -175,6 +175,7 @@ virCapsPtr testQemuCapsInit(void) { goto cleanup; qemuDomainSetNamespaceHooks(caps); + qemuDomainSetDefHooks(caps); if ((guest = virCapabilitiesAddGuest(caps, "hvm", VIR_ARCH_I686, "/usr/bin/qemu", NULL, -- 1.8.1.1

After the previous commit, the assignment of the default model is no longer needed. Get rid of it and simplify the code. --- src/qemu/qemu_command.c | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index dee493f..82e9426 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3155,21 +3155,17 @@ qemuBuildNicDevStr(virDomainNetDefPtr net, virQEMUCapsPtr qemuCaps) { virBuffer buf = VIR_BUFFER_INITIALIZER; - const char *nic; + const char *nic = net->model; bool usingVirtio = false; - if (!net->model) { - nic = "rtl8139"; - } else if (STREQ(net->model, "virtio")) { + if (STREQ(net->model, "virtio")) { if (net->info.type == - VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390) { + VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390) nic = "virtio-net-s390"; - } else { + else nic = "virtio-net-pci"; - } + usingVirtio = true; - } else { - nic = net->model; } virBufferAdd(&buf, nic, strlen(nic)); -- 1.8.1.1

Move this to the after-parse updater. --- src/conf/domain_conf.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index d881983..3c3172d 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2362,10 +2362,13 @@ virDomainDeviceDefUpdateDefaultsInternal(virDomainDeviceDefPtr dev ATTRIBUTE_UNU static int -virDomainDefUpdateDefaultsInternal(virDomainDefPtr def ATTRIBUTE_UNUSED, +virDomainDefUpdateDefaultsInternal(virDomainDefPtr def, virCapsPtr caps ATTRIBUTE_UNUSED) { - /* not in use yet */ + /* Auto-add any implied controllers which aren't present */ + if (virDomainDefAddImplicitControllers(def) < 0) + return -1; + return 0; } @@ -10778,10 +10781,6 @@ virDomainDefParseXML(virCapsPtr caps, (def->ns.parse)(xml, root, ctxt, &def->namespaceData) < 0) goto error; - /* Auto-add any implied controllers which aren't present */ - if (virDomainDefAddImplicitControllers(def) < 0) - goto error; - /* callback to fill driver specific domain aspects */ if (virDomainDefUpdateDefaults(def, caps) < 0) goto error; -- 1.8.1.1

On 02/20/2013 12:06 PM, Peter Krempa wrote:
Move this to the after-parse updater.
It really should happen in a callback provided by qemu (what will that do to xen? Do we need to provide the same callback for the xen driver?).
--- src/conf/domain_conf.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index d881983..3c3172d 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2362,10 +2362,13 @@ virDomainDeviceDefUpdateDefaultsInternal(virDomainDeviceDefPtr dev ATTRIBUTE_UNU
static int -virDomainDefUpdateDefaultsInternal(virDomainDefPtr def ATTRIBUTE_UNUSED, +virDomainDefUpdateDefaultsInternal(virDomainDefPtr def, virCapsPtr caps ATTRIBUTE_UNUSED) { - /* not in use yet */ + /* Auto-add any implied controllers which aren't present */ + if (virDomainDefAddImplicitControllers(def) < 0) + return -1; + return 0; }
@@ -10778,10 +10781,6 @@ virDomainDefParseXML(virCapsPtr caps, (def->ns.parse)(xml, root, ctxt, &def->namespaceData) < 0) goto error;
- /* Auto-add any implied controllers which aren't present */ - if (virDomainDefAddImplicitControllers(def) < 0) - goto error; - /* callback to fill driver specific domain aspects */ if (virDomainDefUpdateDefaults(def, caps) < 0) goto error;

The parameters are initialized already so no need to do it again. --- src/conf/domain_conf.c | 29 ++++++++--------------------- 1 file changed, 8 insertions(+), 21 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 3c3172d..da04b7b 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -9485,29 +9485,16 @@ virDomainDefParseXML(virCapsPtr caps, } /* Extract cpu tunables. */ - if (virXPathULong("string(./cputune/shares[1])", ctxt, - &def->cputune.shares) < 0) - def->cputune.shares = 0; + virXPathULong("string(./cputune/shares[1])", ctxt, &def->cputune.shares); + virXPathULongLong("string(./cputune/period[1])", ctxt, &def->cputune.period); + virXPathLongLong("string(./cputune/quota[1])", ctxt, &def->cputune.quota); + virXPathULongLong("string(./cputune/emulator_period[1])", ctxt, + &def->cputune.emulator_period); + virXPathLongLong("string(./cputune/emulator_quota[1])", ctxt, + &def->cputune.emulator_quota); - if (virXPathULongLong("string(./cputune/period[1])", ctxt, - &def->cputune.period) < 0) - def->cputune.period = 0; - - if (virXPathLongLong("string(./cputune/quota[1])", ctxt, - &def->cputune.quota) < 0) - def->cputune.quota = 0; - - if (virXPathULongLong("string(./cputune/emulator_period[1])", ctxt, - &def->cputune.emulator_period) < 0) - def->cputune.emulator_period = 0; - - if (virXPathLongLong("string(./cputune/emulator_quota[1])", ctxt, - &def->cputune.emulator_quota) < 0) - def->cputune.emulator_quota = 0; - - if ((n = virXPathNodeSet("./cputune/vcpupin", ctxt, &nodes)) < 0) { + if ((n = virXPathNodeSet("./cputune/vcpupin", ctxt, &nodes)) < 0) goto error; - } if (n && VIR_ALLOC_N(def->cputune.vcpupin, n) < 0) goto no_memory; -- 1.8.1.1

On 02/20/2013 12:06 PM, Peter Krempa wrote:
The parameters are initialized already so no need to do it again. --- src/conf/domain_conf.c | 29 ++++++++--------------------- 1 file changed, 8 insertions(+), 21 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 3c3172d..da04b7b 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -9485,29 +9485,16 @@ virDomainDefParseXML(virCapsPtr caps, }
/* Extract cpu tunables. */ - if (virXPathULong("string(./cputune/shares[1])", ctxt, - &def->cputune.shares) < 0) - def->cputune.shares = 0; + virXPathULong("string(./cputune/shares[1])", ctxt, &def->cputune.shares); + virXPathULongLong("string(./cputune/period[1])", ctxt, &def->cputune.period); + virXPathLongLong("string(./cputune/quota[1])", ctxt, &def->cputune.quota); + virXPathULongLong("string(./cputune/emulator_period[1])", ctxt, + &def->cputune.emulator_period); + virXPathLongLong("string(./cputune/emulator_quota[1])", ctxt, + &def->cputune.emulator_quota);
You're not doing anything to check for invalid data here. If somebody sets one of them to "blorg", you'll just set it to 0.
- if (virXPathULongLong("string(./cputune/period[1])", ctxt, - &def->cputune.period) < 0) - def->cputune.period = 0; - - if (virXPathLongLong("string(./cputune/quota[1])", ctxt, - &def->cputune.quota) < 0) - def->cputune.quota = 0; - - if (virXPathULongLong("string(./cputune/emulator_period[1])", ctxt, - &def->cputune.emulator_period) < 0) - def->cputune.emulator_period = 0; - - if (virXPathLongLong("string(./cputune/emulator_quota[1])", ctxt, - &def->cputune.emulator_quota) < 0) - def->cputune.emulator_quota = 0; - - if ((n = virXPathNodeSet("./cputune/vcpupin", ctxt, &nodes)) < 0) { + if ((n = virXPathNodeSet("./cputune/vcpupin", ctxt, &nodes)) < 0) goto error; - }
if (n && VIR_ALLOC_N(def->cputune.vcpupin, n) < 0) goto no_memory;

On 03/01/13 20:29, Laine Stump wrote:
On 02/20/2013 12:06 PM, Peter Krempa wrote:
The parameters are initialized already so no need to do it again. --- src/conf/domain_conf.c | 29 ++++++++--------------------- 1 file changed, 8 insertions(+), 21 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 3c3172d..da04b7b 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -9485,29 +9485,16 @@ virDomainDefParseXML(virCapsPtr caps, }
/* Extract cpu tunables. */ - if (virXPathULong("string(./cputune/shares[1])", ctxt, - &def->cputune.shares) < 0) - def->cputune.shares = 0; + virXPathULong("string(./cputune/shares[1])", ctxt, &def->cputune.shares); + virXPathULongLong("string(./cputune/period[1])", ctxt, &def->cputune.period); + virXPathLongLong("string(./cputune/quota[1])", ctxt, &def->cputune.quota); + virXPathULongLong("string(./cputune/emulator_period[1])", ctxt, + &def->cputune.emulator_period); + virXPathLongLong("string(./cputune/emulator_quota[1])", ctxt, + &def->cputune.emulator_quota);
You're not doing anything to check for invalid data here. If somebody sets one of them to "blorg", you'll just set it to 0.
For my defense, it used to be this way and I just simplified the code. I agree we should actually change this to provide meaningful errors. v2 coming soon.
- if (virXPathULongLong("string(./cputune/period[1])", ctxt, - &def->cputune.period) < 0) - def->cputune.period = 0; - - if (virXPathLongLong("string(./cputune/quota[1])", ctxt, - &def->cputune.quota) < 0) - def->cputune.quota = 0; - - if (virXPathULongLong("string(./cputune/emulator_period[1])", ctxt, - &def->cputune.emulator_period) < 0) - def->cputune.emulator_period = 0; - - if (virXPathLongLong("string(./cputune/emulator_quota[1])", ctxt, - &def->cputune.emulator_quota) < 0) - def->cputune.emulator_quota = 0; - - if ((n = virXPathNodeSet("./cputune/vcpupin", ctxt, &nodes)) < 0) { + if ((n = virXPathNodeSet("./cputune/vcpupin", ctxt, &nodes)) < 0) goto error; - }
if (n && VIR_ALLOC_N(def->cputune.vcpupin, n) < 0) goto no_memory;
Peter

Use the helper to avoid doing this in the parser. --- src/conf/domain_conf.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index da04b7b..0689eff 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2365,6 +2365,13 @@ static int virDomainDefUpdateDefaultsInternal(virDomainDefPtr def, virCapsPtr caps ATTRIBUTE_UNUSED) { + /* verify the domain title */ + if (def->title && strchr(def->title, '\n')) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Domain title can't contain newlines")); + return -1; + } + /* Auto-add any implied controllers which aren't present */ if (virDomainDefAddImplicitControllers(def) < 0) return -1; @@ -9314,12 +9321,6 @@ virDomainDefParseXML(virCapsPtr caps, /* Extract short description of domain (title) */ def->title = virXPathString("string(./title[1])", ctxt); - if (def->title && strchr(def->title, '\n')) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("Domain title can't contain newlines")); - goto error; - } - /* Extract documentation if present */ def->description = virXPathString("string(./description[1])", ctxt); -- 1.8.1.1

On 02/20/2013 12:06 PM, Peter Krempa wrote:
Use the helper to avoid doing this in the parser. --- src/conf/domain_conf.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index da04b7b..0689eff 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2365,6 +2365,13 @@ static int virDomainDefUpdateDefaultsInternal(virDomainDefPtr def, virCapsPtr caps ATTRIBUTE_UNUSED) { + /* verify the domain title */ + if (def->title && strchr(def->title, '\n')) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Domain title can't contain newlines")); + return -1; + } +
This is a good example of something that isn't "updating defaults". (I think your implication here is that this validation should be hypervisor-specific, and so should end up being moved out into the hypervisor's parse callback, which is true, but it doesn't fit with the current named purpose of the function.)
/* Auto-add any implied controllers which aren't present */ if (virDomainDefAddImplicitControllers(def) < 0) return -1; @@ -9314,12 +9321,6 @@ virDomainDefParseXML(virCapsPtr caps,
/* Extract short description of domain (title) */ def->title = virXPathString("string(./title[1])", ctxt); - if (def->title && strchr(def->title, '\n')) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("Domain title can't contain newlines")); - goto error; - } - /* Extract documentation if present */ def->description = virXPathString("string(./description[1])", ctxt);

On 02/20/13 18:06, Peter Krempa wrote:
This monster series cleans up a ton of stuff and then adds the ability to fill driver specific defaults by means of a callback.
The usage is demonstrated on automaticaly filling default NIC type with qemu.
The long term aim is to move all validations and default settings out of the parser.
I pushed the rest of the ACKed cleanup patches from this series and I'm now working on a v2 of the callback stuff after the input from Laine and Dan. Thanks for the reviews. Peter
participants (4)
-
Daniel P. Berrange
-
Eric Blake
-
Laine Stump
-
Peter Krempa