[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 07/16] conf: whitespace cleanups and refactors with no semantic impact



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 :-)


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]