[libvirt PATCH v2 01/20] cpu_x86: Simplify x86ParseCPUID

[Tim Wiederhake]

On Fri, 2021-11-05 at 16:30 +0100, Michal Prívozník wrote:
> On 11/4/21 5:27 PM, Tim Wiederhake wrote:
> > ... by using virXMLProp*() helpers. These only require a xmlNodePtr
> > and
> > do not need a xmlXPathContextPtr. Reflect that in the function
> > signature.
> > 
> > Signed-off-by: Tim Wiederhake <twiederh at redhat.com>
> > ---
> >  src/cpu/cpu_x86.c | 43 +++++++++++++++++--------------------------
> >  1 file changed, 17 insertions(+), 26 deletions(-)
> > 
> > diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c
> > index 1b829e5658..b0f6fa8f34 100644
> > --- a/src/cpu/cpu_x86.c
> > +++ b/src/cpu/cpu_x86.c
> > @@ -1021,37 +1021,28 @@ x86FeatureNames(virCPUx86Map *map,
> >  
> >  
> >  static int
> > -x86ParseCPUID(xmlXPathContextPtr ctxt,
> > +x86ParseCPUID(xmlNodePtr node,
> >                virCPUx86DataItem *item)
> >  {
> > -    virCPUx86CPUID *cpuid;
> > -    unsigned long eax_in, ecx_in;
> > -    unsigned long eax, ebx, ecx, edx;
> > -    int ret_eax_in, ret_ecx_in, ret_eax, ret_ebx, ret_ecx,
> > ret_edx;
> > -
> > -    memset(item, 0, sizeof(*item));
> > +    virCPUx86CPUID cpuid;
> >  
> > -    eax_in = ecx_in = 0;
> > -    eax = ebx = ecx = edx = 0;
> > -    ret_eax_in = virXPathULongHex("string(@eax_in)", ctxt,
> > &eax_in);
> > -    ret_ecx_in = virXPathULongHex("string(@ecx_in)", ctxt,
> > &ecx_in);
> > -    ret_eax = virXPathULongHex("string(@eax)", ctxt, &eax);
> > -    ret_ebx = virXPathULongHex("string(@ebx)", ctxt, &ebx);
> > -    ret_ecx = virXPathULongHex("string(@ecx)", ctxt, &ecx);
> > -    ret_edx = virXPathULongHex("string(@edx)", ctxt, &edx);
> > +    memset(&cpuid, 0, sizeof(cpuid));
> 
> Alternatively, let compiler clear the variable out in variable
> declaration:
> 
>     virCPUx86CPUID cpuid = { 0 };

You are right, that is cleaner. Will change before pushing. Thanks!

> > 
> > -    if (ret_eax_in < 0 || ret_ecx_in == -2 ||
> > -        ret_eax == -2 || ret_ebx == -2 || ret_ecx == -2 || ret_edx
> > == -2)
> > +    if (virXMLPropUInt(node, "eax_in", 0, VIR_XML_PROP_REQUIRED,
> > &cpuid.eax_in) < 0)
> > +        return -1;
> > +    if (virXMLPropUInt(node, "ecx_in", 0, VIR_XML_PROP_NONE,
> > &cpuid.ecx_in) < 0)
> > +        return -1;
> > +    if (virXMLPropUInt(node, "eax", 0, VIR_XML_PROP_NONE,
> > &cpuid.eax) < 0)
> > +        return -1;
> > +    if (virXMLPropUInt(node, "ebx", 0, VIR_XML_PROP_NONE,
> > &cpuid.ebx) < 0)
> > +        return -1;
> > +    if (virXMLPropUInt(node, "ecx", 0, VIR_XML_PROP_NONE,
> > &cpuid.ecx) < 0)
> > +        return -1;
> > +    if (virXMLPropUInt(node, "edx", 0, VIR_XML_PROP_NONE,
> > &cpuid.edx) < 0)
> >          return -1;
> >  
> >      item->type = VIR_CPU_X86_DATA_CPUID;
> > -    cpuid = &item->data.cpuid;
> > -    cpuid->eax_in = eax_in;
> > -    cpuid->ecx_in = ecx_in;
> > -    cpuid->eax = eax;
> > -    cpuid->ebx = ebx;
> > -    cpuid->ecx = ecx;
> > -    cpuid->edx = edx;
> > +    item->data.cpuid = cpuid;
> >      return 0;
> >  }
> >  
> > @@ -1122,7 +1113,7 @@ x86FeatureParse(xmlXPathContextPtr ctxt,
> >      for (i = 0; i < n; i++) {
> >          ctxt->node = nodes[i];
> >          if (virXMLNodeNameEqual(nodes[i], "cpuid")) {
> > -            if (x86ParseCPUID(ctxt, &item) < 0) {
> > +            if (x86ParseCPUID(ctxt->node, &item) < 0) {
> 
> I believe nodes[i] can be passed directly. Because in the next patch
> you rework the else branch and if that used nodes[i] too then 'ctxt-
> >node = nodes[i]' line could be dropped.
> 
> >                  virReportError(VIR_ERR_INTERNAL_ERROR,
> >                                 _("Invalid cpuid[%zu] in %s
> > feature"),
> >                                 i, feature->name);
> > @@ -1812,7 +1803,7 @@ virCPUx86DataParse(xmlXPathContextPtr ctxt)
> >      for (i = 0; i < n; i++) {
> >          ctxt->node = nodes[i];
> >          if (virXMLNodeNameEqual(nodes[i], "cpuid")) {
> > -            if (x86ParseCPUID(ctxt, &item) < 0) {
> > +            if (x86ParseCPUID(ctxt->node, &item) < 0) {
> >                  virReportError(VIR_ERR_INTERNAL_ERROR,
> >                                 _("failed to parse cpuid[%zu]"),
> > i);
> >                  return NULL;
> > 
> 
> Same here. But looking more into the future, doesn't matter really,
> because in pach 03/20 you replace all of this with a function and use
> xmlNode directly.
> 
> Michal
>