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 :-)
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list