Re: [libvirt] [RFC PATCH 03/12] domain_conf: introduce cpu def helpers
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(a)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