[PATCH 0/7] Couple of miscellaneous cleanups
There's this request for us to parse NVDIMMs from .vmx files. Well, we're not there yet - firstly we'd need to deduce NUMA configuration so that we know to which guest NUMA node is the NVDIMM attached. But that is very dynamic and in fact computed on virtual machine startup [1]. Nevertheless, I've accumulated couple of cleanups that are independent of the feature and can be merged separately. 1: https://frankdenneman.nl/2016/08/22/numa-deep-dive-part-5-esxi-vmkernel-numa... Michal Prívozník (7): coding-style: Follow our own recommendation wrt spacing around commas docs: Fill missing docs on STRCASEPREFIX() and STRSKIP() internal: Introduce STRCASESKIP() vmx: Convert virVMXConfigScanResultsCollector() to use STRCASESKIP() vmx: Rework virVMXConfigScanResultsCollector slightly conf: Introduce virDomainMemoryDefNew() conf: Declare and use autoptr for virDomainMemoryDef docs/coding-style.rst | 42 ++++++++++++++++++++++++++++-------- src/conf/domain_conf.c | 48 ++++++++++++++++++++++++------------------ src/conf/domain_conf.h | 2 ++ src/internal.h | 2 ++ src/vmx/vmx.c | 11 +++++----- 5 files changed, 71 insertions(+), 34 deletions(-) -- 2.37.4
We require a space after a comma and even document this in our coding style document. However, our own rule is broken in the very same document when listing string comparison macros. Separate macro arguments properly. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- docs/coding-style.rst | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/docs/coding-style.rst b/docs/coding-style.rst index 81bd4474f1..d4c9d3b8ab 100644 --- a/docs/coding-style.rst +++ b/docs/coding-style.rst @@ -705,35 +705,35 @@ use one of the following semantically named macros :: - STREQ(a,b) - STRNEQ(a,b) + STREQ(a, b) + STRNEQ(a, b) - For case insensitive equality: :: - STRCASEEQ(a,b) - STRCASENEQ(a,b) + STRCASEEQ(a, b) + STRCASENEQ(a, b) - For strict equality of a substring: :: - STREQLEN(a,b,n) - STRNEQLEN(a,b,n) + STREQLEN(a, b, n) + STRNEQLEN(a, b, n) - For case insensitive equality of a substring: :: - STRCASEEQLEN(a,b,n) - STRCASENEQLEN(a,b,n) + STRCASEEQLEN(a, b, n) + STRCASENEQLEN(a, b, n) - For strict equality of a prefix: :: - STRPREFIX(a,b) + STRPREFIX(a, b) - To avoid having to check if a or b are NULL: -- 2.37.4
We document use of our STR*() macros, but somehow missed STRCASEPREFIX() and STRSKIP(). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- docs/coding-style.rst | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/docs/coding-style.rst b/docs/coding-style.rst index d4c9d3b8ab..1faaf681e4 100644 --- a/docs/coding-style.rst +++ b/docs/coding-style.rst @@ -735,6 +735,21 @@ use one of the following semantically named macros STRPREFIX(a, b) +- For case insensitive equality of a prefix: + + :: + + STRCASEPREFIX(a, b) + +- For skipping prefix: + + :: + + /* Instead of: + * STRPREFIX(a, b) ? a + strlen(b) : NULL + * use: */ + STRSKIP(a, b) + - To avoid having to check if a or b are NULL: :: -- 2.37.4
There is so far one case where STRCASEPREFIX(a, b) && a + strlen(b) combo is used (in virVMXConfigScanResultsCollector()), but there will be more. Do what we do usually: introduce a macro. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- docs/coding-style.rst | 9 +++++++++ src/internal.h | 2 ++ 2 files changed, 11 insertions(+) diff --git a/docs/coding-style.rst b/docs/coding-style.rst index 1faaf681e4..02d99330bf 100644 --- a/docs/coding-style.rst +++ b/docs/coding-style.rst @@ -750,6 +750,15 @@ use one of the following semantically named macros * use: */ STRSKIP(a, b) +- For skipping prefix case insensitively: + + :: + + /* Instead of: + * STRCASEPREFIX(a, b) ? a + strlen(b) : NULL + * use: */ + STRCASESKIP(a, b) + - To avoid having to check if a or b are NULL: :: diff --git a/src/internal.h b/src/internal.h index 1e8e2908bf..35cc22ee3d 100644 --- a/src/internal.h +++ b/src/internal.h @@ -87,6 +87,8 @@ #define STRPREFIX(a, b) (strncmp(a, b, strlen(b)) == 0) #define STRCASEPREFIX(a, b) (g_ascii_strncasecmp(a, b, strlen(b)) == 0) #define STRSKIP(a, b) (STRPREFIX(a, b) ? (a) + strlen(b) : NULL) +#define STRCASESKIP(a, b) (STRCASEPREFIX(a, b) ? (a) + strlen(b) : NULL) + /** * STRLIM * @str: pointer to a string (evaluated once) -- 2.37.4
Now that we have STRCASESKIP() there's no need to open code it. Convert virVMXConfigScanResultsCollector() so that it uses this new macro. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/vmx/vmx.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c index bf0dba17d8..d3e452e3ef 100644 --- a/src/vmx/vmx.c +++ b/src/vmx/vmx.c @@ -1321,12 +1321,13 @@ virVMXConfigScanResultsCollector(const char* name, void *opaque) { struct virVMXConfigScanResults *results = opaque; + const char *suffix = NULL; - if (STRCASEPREFIX(name, "ethernet")) { + if ((suffix = STRCASESKIP(name, "ethernet"))) { unsigned int idx; char *p; - if (virStrToLong_uip(name + 8, &p, 10, &idx) < 0 || + if (virStrToLong_uip(suffix, &p, 10, &idx) < 0 || *p != '.') { virReportError(VIR_ERR_INTERNAL_ERROR, _("failed to parse the index of the VMX key '%s'"), -- 2.37.4
The idea here is that virVMXConfigScanResultsCollector() sets the networks_max_index to the highest ethernet index seen. Well, the struct member is signed int, we parse just seen index into uint and then typecast to compare the two. This is not necessary, because the maximum number of NICs a vSphere domain can have is (<drumrolll/>): ten [1]. This will fit into signed int easily anywhere. 1: https://configmax.esp.vmware.com/guest?vmwareproduct=vSphere&release=vSphere%208.0&categories=1-0 Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/vmx/vmx.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c index d3e452e3ef..287a877b4a 100644 --- a/src/vmx/vmx.c +++ b/src/vmx/vmx.c @@ -1324,10 +1324,10 @@ virVMXConfigScanResultsCollector(const char* name, const char *suffix = NULL; if ((suffix = STRCASESKIP(name, "ethernet"))) { - unsigned int idx; + int idx; char *p; - if (virStrToLong_uip(suffix, &p, 10, &idx) < 0 || + if (virStrToLong_i(suffix, &p, 10, &idx) < 0 || *p != '.') { virReportError(VIR_ERR_INTERNAL_ERROR, _("failed to parse the index of the VMX key '%s'"), @@ -1335,8 +1335,8 @@ virVMXConfigScanResultsCollector(const char* name, return -1; } - if ((int)idx > results->networks_max_index) - results->networks_max_index = (int)idx; + if (idx > results->networks_max_index) + results->networks_max_index = idx; } return 0; -- 2.37.4
On Wed, Nov 16, 2022 at 10:11:43 +0100, Michal Privoznik wrote:
The idea here is that virVMXConfigScanResultsCollector() sets the networks_max_index to the highest ethernet index seen. Well, the struct member is signed int, we parse just seen index into uint and then typecast to compare the two. This is not necessary, because the maximum number of NICs a vSphere domain can have is (<drumrolll/>): ten [1]. This will fit into signed int easily anywhere.
1: https://configmax.esp.vmware.com/guest?vmwareproduct=vSphere&release=vSphere%208.0&categories=1-0
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/vmx/vmx.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c index d3e452e3ef..287a877b4a 100644 --- a/src/vmx/vmx.c +++ b/src/vmx/vmx.c @@ -1324,10 +1324,10 @@ virVMXConfigScanResultsCollector(const char* name, const char *suffix = NULL;
if ((suffix = STRCASESKIP(name, "ethernet"))) { - unsigned int idx; + int idx; char *p;
- if (virStrToLong_uip(suffix, &p, 10, &idx) < 0 || + if (virStrToLong_i(suffix, &p, 10, &idx) < 0 || *p != '.') { virReportError(VIR_ERR_INTERNAL_ERROR, _("failed to parse the index of the VMX key '%s'"),
Just a note, because it isn't obvious neiter from the context, nor from the commit message. 'virStrToLong_uip' validates that the parsed number is not negative. Switching to virStrToLong_i allows negative numbers to be considered, but in this case it won't matter too much. We'd just ignore the network device if the index for some reason was -1, and additionally we'd trust any massive positive number anyways.
On 11/16/22 12:35, Peter Krempa wrote:
On Wed, Nov 16, 2022 at 10:11:43 +0100, Michal Privoznik wrote:
The idea here is that virVMXConfigScanResultsCollector() sets the networks_max_index to the highest ethernet index seen. Well, the struct member is signed int, we parse just seen index into uint and then typecast to compare the two. This is not necessary, because the maximum number of NICs a vSphere domain can have is (<drumrolll/>): ten [1]. This will fit into signed int easily anywhere.
1: https://configmax.esp.vmware.com/guest?vmwareproduct=vSphere&release=vSphere%208.0&categories=1-0
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/vmx/vmx.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c index d3e452e3ef..287a877b4a 100644 --- a/src/vmx/vmx.c +++ b/src/vmx/vmx.c @@ -1324,10 +1324,10 @@ virVMXConfigScanResultsCollector(const char* name, const char *suffix = NULL;
if ((suffix = STRCASESKIP(name, "ethernet"))) { - unsigned int idx; + int idx; char *p;
- if (virStrToLong_uip(suffix, &p, 10, &idx) < 0 || + if (virStrToLong_i(suffix, &p, 10, &idx) < 0 || *p != '.') { virReportError(VIR_ERR_INTERNAL_ERROR, _("failed to parse the index of the VMX key '%s'"),
Just a note, because it isn't obvious neiter from the context, nor from the commit message.
'virStrToLong_uip' validates that the parsed number is not negative.
Switching to virStrToLong_i allows negative numbers to be considered, but in this case it won't matter too much. We'd just ignore the network device if the index for some reason was -1, and additionally we'd trust any massive positive number anyways.
Yeah, do you want me to put idx < 0 check in or mention this in a commit message? Michal
On Wed, Nov 16, 2022 at 12:37:30 +0100, Michal Prívozník wrote:
On 11/16/22 12:35, Peter Krempa wrote:
On Wed, Nov 16, 2022 at 10:11:43 +0100, Michal Privoznik wrote:
The idea here is that virVMXConfigScanResultsCollector() sets the networks_max_index to the highest ethernet index seen. Well, the struct member is signed int, we parse just seen index into uint and then typecast to compare the two. This is not necessary, because the maximum number of NICs a vSphere domain can have is (<drumrolll/>): ten [1]. This will fit into signed int easily anywhere.
1: https://configmax.esp.vmware.com/guest?vmwareproduct=vSphere&release=vSphere%208.0&categories=1-0
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/vmx/vmx.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c index d3e452e3ef..287a877b4a 100644 --- a/src/vmx/vmx.c +++ b/src/vmx/vmx.c @@ -1324,10 +1324,10 @@ virVMXConfigScanResultsCollector(const char* name, const char *suffix = NULL;
if ((suffix = STRCASESKIP(name, "ethernet"))) { - unsigned int idx; + int idx; char *p;
- if (virStrToLong_uip(suffix, &p, 10, &idx) < 0 || + if (virStrToLong_i(suffix, &p, 10, &idx) < 0 || *p != '.') { virReportError(VIR_ERR_INTERNAL_ERROR, _("failed to parse the index of the VMX key '%s'"),
Just a note, because it isn't obvious neiter from the context, nor from the commit message.
'virStrToLong_uip' validates that the parsed number is not negative.
Switching to virStrToLong_i allows negative numbers to be considered, but in this case it won't matter too much. We'd just ignore the network device if the index for some reason was -1, and additionally we'd trust any massive positive number anyways.
Yeah, do you want me to put idx < 0 check in or mention this in a commit message?
I don't think it will be a problem even without any change, but you can add the idx < 0 check to the existing *p != '.' to preserve the semantics completely.
This is new allocator for virDomainMemoryDef struct which also sets some default values: @model and @targetNode. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 26 +++++++++++++++++++------- src/conf/domain_conf.h | 1 + 2 files changed, 20 insertions(+), 7 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 622f83590f..d3b8ef3656 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -13088,9 +13088,6 @@ virDomainMemoryTargetDefParseXML(xmlNodePtr node, ctxt->node = node; - /* initialize to value which marks that the user didn't specify it */ - def->targetNode = -1; - if ((rv = virXPathInt("string(./node)", ctxt, &def->targetNode)) == -2 || (rv == 0 && def->targetNode < 0)) { virReportError(VIR_ERR_XML_ERROR, "%s", @@ -13226,6 +13223,20 @@ virDomainSecDefParseXML(xmlNodePtr lsecNode, } +virDomainMemoryDef * +virDomainMemoryDefNew(virDomainMemoryModel model) +{ + virDomainMemoryDef *def = NULL; + + def = g_new0(virDomainMemoryDef, 1); + def->model = model; + /* initialize to value which marks that the user didn't specify it */ + def->targetNode = -1; + + return def; +} + + static virDomainMemoryDef * virDomainMemoryDefParseXML(virDomainXMLOption *xmlopt, xmlNodePtr memdevNode, @@ -13234,18 +13245,19 @@ virDomainMemoryDefParseXML(virDomainXMLOption *xmlopt, { VIR_XPATH_NODE_AUTORESTORE(ctxt) xmlNodePtr node; - virDomainMemoryDef *def; + virDomainMemoryDef *def = NULL; + virDomainMemoryModel model; g_autofree char *tmp = NULL; - def = g_new0(virDomainMemoryDef, 1); - ctxt->node = memdevNode; if (virXMLPropEnum(memdevNode, "model", virDomainMemoryModelTypeFromString, VIR_XML_PROP_REQUIRED | VIR_XML_PROP_NONZERO, - &def->model) < 0) + &model) < 0) goto error; + def = virDomainMemoryDefNew(model); + if (virXMLPropEnum(memdevNode, "access", virDomainMemoryAccessTypeFromString, VIR_XML_PROP_NONZERO, &def->access) < 0) goto error; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index e16558988d..d29ebf8b8a 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2619,6 +2619,7 @@ struct _virDomainMemoryDef { virDomainDeviceInfo info; }; +virDomainMemoryDef *virDomainMemoryDefNew(virDomainMemoryModel model); void virDomainMemoryDefFree(virDomainMemoryDef *def); struct _virDomainIdMapEntry { -- 2.37.4
Register virDomainMemoryDefFree() to do the cleanup. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 24 ++++++++++-------------- src/conf/domain_conf.h | 1 + 2 files changed, 11 insertions(+), 14 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index d3b8ef3656..3790121cf7 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -13245,7 +13245,7 @@ virDomainMemoryDefParseXML(virDomainXMLOption *xmlopt, { VIR_XPATH_NODE_AUTORESTORE(ctxt) xmlNodePtr node; - virDomainMemoryDef *def = NULL; + g_autoptr(virDomainMemoryDef) def = NULL; virDomainMemoryModel model; g_autofree char *tmp = NULL; @@ -13254,17 +13254,17 @@ virDomainMemoryDefParseXML(virDomainXMLOption *xmlopt, if (virXMLPropEnum(memdevNode, "model", virDomainMemoryModelTypeFromString, VIR_XML_PROP_REQUIRED | VIR_XML_PROP_NONZERO, &model) < 0) - goto error; + return NULL; def = virDomainMemoryDefNew(model); if (virXMLPropEnum(memdevNode, "access", virDomainMemoryAccessTypeFromString, VIR_XML_PROP_NONZERO, &def->access) < 0) - goto error; + return NULL; if (virXMLPropTristateBool(memdevNode, "discard", VIR_XML_PROP_NONE, &def->discard) < 0) - goto error; + return NULL; /* Extract NVDIMM UUID. */ if (def->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM && @@ -13274,34 +13274,30 @@ virDomainMemoryDefParseXML(virDomainXMLOption *xmlopt, if (virUUIDParse(tmp, def->uuid) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("malformed uuid element")); - goto error; + return NULL; } } /* source */ if ((node = virXPathNode("./source", ctxt)) && virDomainMemorySourceDefParseXML(node, ctxt, def) < 0) - goto error; + return NULL; /* target */ if (!(node = virXPathNode("./target", ctxt))) { virReportError(VIR_ERR_XML_ERROR, "%s", _("missing <target> element for <memory> device")); - goto error; + return NULL; } if (virDomainMemoryTargetDefParseXML(node, ctxt, def) < 0) - goto error; + return NULL; if (virDomainDeviceInfoParseXML(xmlopt, memdevNode, ctxt, &def->info, flags) < 0) - goto error; + return NULL; - return def; - - error: - virDomainMemoryDefFree(def); - return NULL; + return g_steal_pointer(&def); } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index d29ebf8b8a..a9f7e8d977 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2621,6 +2621,7 @@ struct _virDomainMemoryDef { virDomainMemoryDef *virDomainMemoryDefNew(virDomainMemoryModel model); void virDomainMemoryDefFree(virDomainMemoryDef *def); +G_DEFINE_AUTOPTR_CLEANUP_FUNC(virDomainMemoryDef, virDomainMemoryDefFree); struct _virDomainIdMapEntry { unsigned int start; -- 2.37.4
On Wed, 2022-11-16 at 10:11 +0100, Michal Privoznik wrote:
There's this request for us to parse NVDIMMs from .vmx files. Well, we're not there yet - firstly we'd need to deduce NUMA configuration so that we know to which guest NUMA node is the NVDIMM attached. But that is very dynamic and in fact computed on virtual machine startup [1].
Nevertheless, I've accumulated couple of cleanups that are independent of the feature and can be merged separately.
1: https://frankdenneman.nl/2016/08/22/numa-deep-dive-part-5-esxi-vmkernel-numa...
Michal Prívozník (7): coding-style: Follow our own recommendation wrt spacing around commas docs: Fill missing docs on STRCASEPREFIX() and STRSKIP() internal: Introduce STRCASESKIP() vmx: Convert virVMXConfigScanResultsCollector() to use STRCASESKIP() vmx: Rework virVMXConfigScanResultsCollector slightly conf: Introduce virDomainMemoryDefNew() conf: Declare and use autoptr for virDomainMemoryDef
docs/coding-style.rst | 42 ++++++++++++++++++++++++++++-------- src/conf/domain_conf.c | 48 ++++++++++++++++++++++++---------------- -- src/conf/domain_conf.h | 2 ++ src/internal.h | 2 ++ src/vmx/vmx.c | 11 +++++----- 5 files changed, 71 insertions(+), 34 deletions(-)
Reviewed-by: Tim Wiederhake <twiederh@redhat.com>
participants (4)
-
Michal Privoznik -
Michal Prívozník
-
Peter Krempa -
Tim Wiederhake