[libvirt] [PATCH 0/4] Cleanup and fix parsing of vendor_id in the XML cpu definition
This patchset contains Ken ICHIKAWA's patch to fix parsing of vendor_id (with tweaked commit message) and my cleanups to pre-existing problems in the code which I found while reviewing Ken's patch. Ken ICHIKAWA (1): conf: cpu: Fix parsing of vendor_id Peter Krempa (3): conf: cpu: Fix memory leak when specifying cpu vendor_id manually conf: cpu: Refactor parsing of vendor_id and fallback attributes conf: cpu: Break some long lines src/conf/cpu_conf.c | 82 +++++++++++++++++++++++++---------------------------- 1 file changed, 38 insertions(+), 44 deletions(-) -- 1.8.0.2
From: Ken ICHIKAWA <ichikawa.ken@jp.fujitsu.com> This patch fixes a problem that vendor_id attribute can not be defined when fallback attribute is not defined. If I define domain xml like below: <domain> <cpu> <model vendor_id='aaaabbbbcccc'>core2duo</model> </cpu> </domain> In dumpxml, vendor_id is not reflected: <domain> <cpu mode='custom' match='exact'> <model fallback='allow'>core2duo</model> </cpu> </domain> The expected output is: <domain> <cpu mode='custom' match='exact'> <model fallback='allow' vendor_id='aaaabbbbcccc'>core2duo</model> </cpu> </domain> If the fallback attribute and vendor_id attribute is defined at the same time, it's reflected as expected. Signed-off-by: Ken ICHIKAWA <ichikawa.ken@jp.fujitsu.com> --- src/conf/cpu_conf.c | 42 +++++++++++++++++++++--------------------- 1 file changed, 21 insertions(+), 21 deletions(-) diff --git a/src/conf/cpu_conf.c b/src/conf/cpu_conf.c index 8cb54a3..6157ed7 100644 --- a/src/conf/cpu_conf.c +++ b/src/conf/cpu_conf.c @@ -300,32 +300,32 @@ virCPUDefParseXML(const xmlNodePtr node, goto error; } } + } - if (virXPathBoolean("boolean(./model[1]/@vendor_id)", ctxt)) { - char *vendor_id; - - vendor_id = virXPathString("string(./model[1]/@vendor_id)", - ctxt); - if (!vendor_id || - strlen(vendor_id) != VIR_CPU_VENDOR_ID_LENGTH) { - virReportError(VIR_ERR_XML_ERROR, - _("vendor_id must be exactly" - " %d characters long"), - VIR_CPU_VENDOR_ID_LENGTH); + if (virXPathBoolean("boolean(./model[1]/@vendor_id)", ctxt)) { + char *vendor_id; + + vendor_id = virXPathString("string(./model[1]/@vendor_id)", + ctxt); + if (!vendor_id || + strlen(vendor_id) != VIR_CPU_VENDOR_ID_LENGTH) { + virReportError(VIR_ERR_XML_ERROR, + _("vendor_id must be exactly" + " %d characters long"), + VIR_CPU_VENDOR_ID_LENGTH); + VIR_FREE(vendor_id); + goto error; + } + /* ensure that the string can be passed to qemu*/ + for (i = 0; i < strlen(vendor_id); i++) { + if (vendor_id[i]==',') { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("vendor id is invalid")); VIR_FREE(vendor_id); goto error; } - /* ensure that the string can be passed to qemu*/ - for (i = 0; i < strlen(vendor_id); i++) { - if (vendor_id[i]==',') { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("vendor id is invalid")); - VIR_FREE(vendor_id); - goto error; - } - } - def->vendor_id = vendor_id; } + def->vendor_id = vendor_id; } } -- 1.8.0.2
On 12/17/2012 12:22 PM, Peter Krempa wrote:
From: Ken ICHIKAWA <ichikawa.ken@jp.fujitsu.com>
This patch fixes a problem that vendor_id attribute can not be defined when fallback attribute is not defined.
If I define domain xml like below: <domain> <cpu> <model vendor_id='aaaabbbbcccc'>core2duo</model> </cpu> </domain>
In dumpxml, vendor_id is not reflected: <domain> <cpu mode='custom' match='exact'> <model fallback='allow'>core2duo</model> </cpu> </domain>
The expected output is: <domain> <cpu mode='custom' match='exact'> <model fallback='allow' vendor_id='aaaabbbbcccc'>core2duo</model> </cpu> </domain>
If the fallback attribute and vendor_id attribute is defined at the same time, it's reflected as expected.
Signed-off-by: Ken ICHIKAWA <ichikawa.ken@jp.fujitsu.com> --- src/conf/cpu_conf.c | 42 +++++++++++++++++++++--------------------- 1 file changed, 21 insertions(+), 21 deletions(-)
diff --git a/src/conf/cpu_conf.c b/src/conf/cpu_conf.c index 8cb54a3..6157ed7 100644 --- a/src/conf/cpu_conf.c +++ b/src/conf/cpu_conf.c @@ -300,32 +300,32 @@ virCPUDefParseXML(const xmlNodePtr node, goto error; } } + }
- if (virXPathBoolean("boolean(./model[1]/@vendor_id)", ctxt)) { - char *vendor_id; - - vendor_id = virXPathString("string(./model[1]/@vendor_id)", - ctxt); - if (!vendor_id || - strlen(vendor_id) != VIR_CPU_VENDOR_ID_LENGTH) { - virReportError(VIR_ERR_XML_ERROR, - _("vendor_id must be exactly" - " %d characters long"), - VIR_CPU_VENDOR_ID_LENGTH); + if (virXPathBoolean("boolean(./model[1]/@vendor_id)", ctxt)) { + char *vendor_id; + + vendor_id = virXPathString("string(./model[1]/@vendor_id)", + ctxt); + if (!vendor_id || + strlen(vendor_id) != VIR_CPU_VENDOR_ID_LENGTH) { + virReportError(VIR_ERR_XML_ERROR, + _("vendor_id must be exactly" + " %d characters long"), + VIR_CPU_VENDOR_ID_LENGTH); + VIR_FREE(vendor_id); + goto error; + } + /* ensure that the string can be passed to qemu*/ + for (i = 0; i < strlen(vendor_id); i++) { + if (vendor_id[i]==',') { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("vendor id is invalid")); VIR_FREE(vendor_id); goto error; } - /* ensure that the string can be passed to qemu*/ - for (i = 0; i < strlen(vendor_id); i++) { - if (vendor_id[i]==',') { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("vendor id is invalid")); - VIR_FREE(vendor_id); - goto error; - } - } - def->vendor_id = vendor_id; } + def->vendor_id = vendor_id; } }
ACK thanks to the fact that you are cleaning it up in next patches. Martin
The field was not freed from the cpu definition. --- src/conf/cpu_conf.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/conf/cpu_conf.c b/src/conf/cpu_conf.c index 6157ed7..7528980 100644 --- a/src/conf/cpu_conf.c +++ b/src/conf/cpu_conf.c @@ -87,6 +87,7 @@ virCPUDefFree(virCPUDefPtr def) VIR_FREE(def->cells[i].cpustr); } VIR_FREE(def->cells); + VIR_FREE(def->vendor_id); VIR_FREE(def); } -- 1.8.0.2
On 12/17/2012 12:22 PM, Peter Krempa wrote:
The field was not freed from the cpu definition. --- src/conf/cpu_conf.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/src/conf/cpu_conf.c b/src/conf/cpu_conf.c index 6157ed7..7528980 100644 --- a/src/conf/cpu_conf.c +++ b/src/conf/cpu_conf.c @@ -87,6 +87,7 @@ virCPUDefFree(virCPUDefPtr def) VIR_FREE(def->cells[i].cpustr); } VIR_FREE(def->cells); + VIR_FREE(def->vendor_id);
VIR_FREE(def); }
ACK, Martin
This patch simplifies the code that parses the fallback and vendor_id attributes from the domain xml cpu definition. Changes done: - free temp variables in the cleanup section instead of local use - remove checking for presence of the attribute to directly getting the value (saving call to virXPathBoolean) - remove loop used to check for ',' in the vendor_id string with strchr --- src/conf/cpu_conf.c | 43 +++++++++++++++++-------------------------- 1 file changed, 17 insertions(+), 26 deletions(-) diff --git a/src/conf/cpu_conf.c b/src/conf/cpu_conf.c index 7528980..e5695f4 100644 --- a/src/conf/cpu_conf.c +++ b/src/conf/cpu_conf.c @@ -195,6 +195,8 @@ virCPUDefParseXML(const xmlNodePtr node, int n; unsigned int i; char *cpuMode; + char *fallback = NULL; + char *vendor_id = NULL; if (!xmlStrEqual(node->name, BAD_CAST "cpu")) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -288,45 +290,32 @@ virCPUDefParseXML(const xmlNodePtr node, if (def->type == VIR_CPU_TYPE_GUEST && def->mode != VIR_CPU_MODE_HOST_PASSTHROUGH) { - if (virXPathBoolean("boolean(./model[1]/@fallback)", ctxt)) { - const char *fallback; - - fallback = virXPathString("string(./model[1]/@fallback)", ctxt); - if (fallback) { - def->fallback = virCPUFallbackTypeFromString(fallback); - VIR_FREE(fallback); - if (def->fallback < 0) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("Invalid fallback attribute")); - goto error; - } + if ((fallback = virXPathString("string(./model[1]/@fallback)", ctxt))) { + if ((def->fallback = virCPUFallbackTypeFromString(fallback)) < 0) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Invalid fallback attribute")); + goto error; } } - if (virXPathBoolean("boolean(./model[1]/@vendor_id)", ctxt)) { - char *vendor_id; - - vendor_id = virXPathString("string(./model[1]/@vendor_id)", - ctxt); - if (!vendor_id || - strlen(vendor_id) != VIR_CPU_VENDOR_ID_LENGTH) { + if ((vendor_id = virXPathString("string(./model[1]/@vendor_id)", + ctxt))) { + if (strlen(vendor_id) != VIR_CPU_VENDOR_ID_LENGTH) { virReportError(VIR_ERR_XML_ERROR, - _("vendor_id must be exactly" - " %d characters long"), + _("vendor_id must be exactly %d characters long"), VIR_CPU_VENDOR_ID_LENGTH); - VIR_FREE(vendor_id); goto error; } + /* ensure that the string can be passed to qemu*/ - for (i = 0; i < strlen(vendor_id); i++) { - if (vendor_id[i]==',') { + if (strchr(vendor_id, ',')) { virReportError(VIR_ERR_XML_ERROR, "%s", _("vendor id is invalid")); - VIR_FREE(vendor_id); goto error; - } } + def->vendor_id = vendor_id; + vendor_id = NULL; } } @@ -490,6 +479,8 @@ virCPUDefParseXML(const xmlNodePtr node, } cleanup: + VIR_FREE(fallback); + VIR_FREE(vendor_id); VIR_FREE(nodes); return def; -- 1.8.0.2
On 12/17/2012 12:22 PM, Peter Krempa wrote:
This patch simplifies the code that parses the fallback and vendor_id attributes from the domain xml cpu definition.
Changes done: - free temp variables in the cleanup section instead of local use - remove checking for presence of the attribute to directly getting the value (saving call to virXPathBoolean) - remove loop used to check for ',' in the vendor_id string with strchr
s/remove/replace/ ACK, Martin
--- src/conf/cpu_conf.c | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/src/conf/cpu_conf.c b/src/conf/cpu_conf.c index e5695f4..41c5535 100644 --- a/src/conf/cpu_conf.c +++ b/src/conf/cpu_conf.c @@ -264,8 +264,9 @@ virCPUDefParseXML(const xmlNodePtr node, VIR_FREE(match); if (def->match < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("Invalid match attribute for CPU specification")); + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Invalid match attribute for CPU " + "specification")); goto error; } } @@ -333,8 +334,8 @@ virCPUDefParseXML(const xmlNodePtr node, ret = virXPathULong("string(./topology[1]/@sockets)", ctxt, &ul); if (ret < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("Missing 'sockets' attribute in CPU topology")); + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Missing 'sockets' attribute in CPU topology")); goto error; } def->sockets = (unsigned int) ul; @@ -342,8 +343,8 @@ virCPUDefParseXML(const xmlNodePtr node, ret = virXPathULong("string(./topology[1]/@cores)", ctxt, &ul); if (ret < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("Missing 'cores' attribute in CPU topology")); + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Missing 'cores' attribute in CPU topology")); goto error; } def->cores = (unsigned int) ul; @@ -351,8 +352,8 @@ virCPUDefParseXML(const xmlNodePtr node, ret = virXPathULong("string(./topology[1]/@threads)", ctxt, &ul); if (ret < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("Missing 'threads' attribute in CPU topology")); + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Missing 'threads' attribute in CPU topology")); goto error; } def->threads = (unsigned int) ul; @@ -370,8 +371,9 @@ virCPUDefParseXML(const xmlNodePtr node, if (n > 0) { if (!def->model) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("Non-empty feature list specified without CPU model")); + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Non-empty feature list specified without " + "CPU model")); goto error; } -- 1.8.0.2
participants (2)
-
Martin Kletzander -
Peter Krempa