[libvirt] [RFC PATCH 03/12] domain_conf: introduce cpu def helpers

Zhu Guihua zhugh.fnst at cn.fujitsu.com
Thu Jan 22 08:48:19 UTC 2015


On Wed, 2015-01-21 at 10:16 +0100, Peter Krempa wrote:
> On Wed, Jan 21, 2015 at 16:00:55 +0800, Zhu Guihua wrote:
> > virDomainCPUDefFree - free memory allocated
> > virDomainCPUDefParseXML - parse job type
> > virDomainCPUDefFormat - output job type
> 
> This patch lacks addition to the RNG schemas that would describe the
> elements that are added and the correct values.
> 
> Also lacks change to the docs/formatdomain.html.in
> 
> 
> > 
> > Signed-off-by: Zhu Guihua <zhugh.fnst at cn.fujitsu.com>
> > ---
> >  src/conf/domain_conf.c | 100 +++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 100 insertions(+)
> > 
> > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> > index e036d75..1f05056 100644
> > --- a/src/conf/domain_conf.c
> > +++ b/src/conf/domain_conf.c
> 
> ...
> 
> > +
> > +static virDomainCPUDefPtr
> > +virDomainCPUDefParseXML(xmlNodePtr node, virDomainDefPtr def)
> > +{
> > +    char *driver = NULL;
> > +    char *apic_id = NULL;
> > +    virDomainCPUDefPtr dev;
> > +
> > +    if (!(dev = virDomainCPUDefNew()))
> > +        return NULL;
> > +
> > +    driver = virXMLPropString(node, "driver");
> 
> The driver should rather be an enum value that is parsed from the string
> rather than stroing an inline string. This limits the namespace of
> devices libvirt supports but simplifies all matching of the driver later
> on.
> 

Yes, it will be an enum value in next version, and it will also support
more driver.

> 
> > +    if (driver == NULL) {
> > +        virReportError(VIR_ERR_XML_ERROR, "%s",
> > +                       _("missing cpu device driver"));
> > +        goto error;
> > +    }
> > +    dev->driver = driver;
> > +
> > +    apic_id = virXMLPropString(node, "apic_id");
> > +
> > +    if (!apic_id)
> > +        dev->apic_id = virDomainCPUGetFreeApicID(def);
> > +    else
> > +        dev->apic_id = atoi (apic_id);
> 
> Atoi is not allowed, use virStrToLong* instead. Also spaces after
> function name is forbidden.
> 

I will change it.

> > +
> > +    driver = NULL;
> > +    apic_id = NULL;
> > +
> > + cleanup:
> > +    VIR_FREE(driver);
> > +    VIR_FREE(apic_id);
> > +
> > +    return dev;
> > +
> > + error:
> > +    virDomainCPUDefFree(dev);
> > +    dev = NULL;
> > +    goto cleanup;
> > +}
> > +
> >  virDomainDeviceDefPtr
> >  virDomainDeviceDefParse(const char *xmlStr,
> >                          const virDomainDef *def,
> > @@ -11187,6 +11253,9 @@ virDomainDeviceDefParse(const char *xmlStr,
> >              goto error;
> >          break;
> >      case VIR_DOMAIN_DEVICE_CPU:
> > +        if (!(dev->data.cpu = virDomainCPUDefParseXML(node, (virDomainDefPtr)def)))
> > +            goto error;
> > +        break;
> >      case VIR_DOMAIN_DEVICE_NONE:
> >      case VIR_DOMAIN_DEVICE_LAST:
> >          break;
> > @@ -18142,6 +18211,31 @@ virDomainChrDefFormat(virBufferPtr buf,
> >  }
> >  
> >  static int
> > +virDomainCPUDefFormat(virBufferPtr buf,
> > +                      virDomainCPUDefPtr def,
> > +                      unsigned int flags)
> > +{
> > +    char *apic_id = NULL;
> > +
> > +    ignore_value(virAsprintf(&apic_id, "%d", def->apic_id));
> > +
> > +    virBufferAsprintf(buf, "<cpu driver='%s'", def->driver);
> > +
> > +    virBufferEscapeString(buf, " apic_id='%s'", apic_id);
> 
> Um? Why not virBufferAsprintf(buf, " apic_id='%d', apic_id)?
> 
> You won't need the intermediate string. Also it leaks the apic_id
> string.
> 

Got it.

> 
> > +
> > +    virBufferAddLit(buf, ">\n");
> > +    virBufferAdjustIndent(buf, 2);
> > +
> > +    if (virDomainDeviceInfoFormat(buf, &def->info, flags) < 0)
> > +        return -1;
> > +
> > +    virBufferAdjustIndent(buf, -2);
> > +    virBufferAddLit(buf, "</cpu>\n");
> > +
> > +    return 0;
> > +}
> > +
> > +static int
> 
> Peter

Regards,
Zhu





More information about the libvir-list mailing list