Re: Re: [RFC 07/21] conf: Replace virNetworkDNSTxtDefParseXML(hardcoded) with namesake(generated)

On Wed, Jun 10, 2020 at 09:20:35AM +0800, Shi Lei wrote: > Signed-off-by: Shi Lei <shi_lei(a)massclouds.com&gt; > --- >  po/POTFILES.in           |  1 + >  src/conf/Makefile.inc.am |  2 ++ >  src/conf/network_conf.c  | 47 ++++++---------------------------------- >  src/conf/network_conf.h  |  8 ++++--- >  4 files changed, 15 insertions(+), 43 deletions(-) > diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c > index 47aaef3..964a8a7 100644 > --- a/src/conf/network_conf.c > +++ b/src/conf/network_conf.c > @@ -174,14 +174,6 @@ virNetworkIPDefClear(virNetworkIPDefPtr def) >  } >  >  > -static void > -virNetworkDNSTxtDefClear(virNetworkDNSTxtDefPtr def) > -{ > -    VIR_FREE(def->name); > -    VIR_FREE(def->value); > -} > - > - >  static void >  virNetworkDNSHostDefClear(virNetworkDNSHostDefPtr def) >  { > @@ -903,7 +895,7 @@ virNetworkDNSSrvDefParseXML(const char *networkName, >  } >  >  > -static int > +int >  virNetworkDNSTxtDefParseXMLHook(xmlNodePtr node G_GNUC_UNUSED, >                                  virNetworkDNSTxtDefPtr def, >                                  const char *networkName, > @@ -943,33 +935,6 @@ virNetworkDNSTxtDefParseXMLHook(xmlNodePtr node G_GNUC_UNUSED, >  } >  >  > -static int > -virNetworkDNSTxtDefParseXML(const char *networkName, > -                            xmlNodePtr node, > -                            virNetworkDNSTxtDefPtr def, > -                            bool partialOkay) > -{ > -    if (!(def->name = virXMLPropString(node, "name"))) { > -        virReportError(VIR_ERR_XML_DETAIL, > -                       _("missing required name attribute in DNS TXT record " > -                         "of network %s"), networkName); > -        goto error; > -    } > - > -    def->value = virXMLPropString(node, "value"); > - > -    if (virNetworkDNSTxtDefParseXMLHook(node, def, networkName, > -                                        &partialOkay) < 0) > -        goto error; > - > -    return 0; > - > - error: > -    virNetworkDNSTxtDefClear(def); > -    return -1; > -} Comparing this old code to the new generated code: int virNetworkDNSTxtDefParseXML(xmlNodePtr node,                             virNetworkDNSTxtDefPtr def,                             const char *instname,                             void *opaque) {     g_autofree char *nameStr = NULL;     g_autofree char *valueStr = NULL;     VIR_USED(instname);     VIR_USED(opaque);     if (!def)         goto error;     nameStr = virXMLPropString(node, "name");     if (nameStr == NULL) {         virReportError(VIR_ERR_XML_ERROR,                        _("Missing 'name' setting in '%s'"),                        instname);         goto error;     }     def->name = g_strdup(nameStr);     valueStr = virXMLPropString(node, "value");     if (valueStr)         def->value = g_strdup(valueStr);     if (virNetworkDNSTxtDefParseXMLHook(node, def, instname, opaque) < 0)         goto error;     return 0; error:         virNetworkDNSTxtDefClear(def);     return -1; } The main changes I'd suggest are *  Change           virReportError(VIR_ERR_XML_ERROR,                        _("Missing 'name' setting in '%s'"),                        instname);    To           virReportError(VIR_ERR_XML_ERROR,                        _("Missing '%s' attribute in '%s'"),                        "name", instname);   so that it reduces the number of translatable strings * Remove the extra g_strdup's In an earlier patch I complained about your use of Clear() functions. Now I see this patch though, I understand why you were using Clear() - this pointer is just a member of an array, so we can't directly Free() it.  So I withdraw my objection on the earlier patch about use of Clear()

Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|