10:11 a.m.
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-n...
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
10:11 a.m.
New subject: [PATCH 1/7] coding-style: Follow our own recommendation wrt spacing around commas
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(a)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
10:11 a.m.
New subject: [PATCH 2/7] docs: Fill missing docs on STRCASEPREFIX() and STRSKIP()
We document use of our STR*() macros, but somehow missed
STRCASEPREFIX() and STRSKIP().
Signed-off-by: Michal Privoznik <mprivozn(a)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
10:11 a.m.
New subject: [PATCH 3/7] internal: Introduce STRCASESKIP()
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(a)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
10:11 a.m.
New subject: [PATCH 4/7] vmx: Convert virVMXConfigScanResultsCollector() to use STRCASESKIP()
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(a)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
10:11 a.m.
New subject: [PATCH 5/7] vmx: Rework virVMXConfigScanResultsCollector slightly
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=...
Signed-off-by: Michal Privoznik <mprivozn(a)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
12:35 p.m.
New subject: [PATCH 5/7] vmx: Rework virVMXConfigScanResultsCollector slightly
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=...
Signed-off-by: Michal Privoznik <mprivozn(a)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.
12:37 p.m.
New subject: [PATCH 5/7] vmx: Rework virVMXConfigScanResultsCollector slightly
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=...
>
> Signed-off-by: Michal Privoznik <mprivozn(a)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
12:43 p.m.
New subject: [PATCH 5/7] vmx: Rework virVMXConfigScanResultsCollector slightly
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=...
>>
>> Signed-off-by: Michal Privoznik <mprivozn(a)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.
10:11 a.m.
New subject: [PATCH 6/7] conf: Introduce virDomainMemoryDefNew()
This is new allocator for virDomainMemoryDef struct which also
sets some default values: @model and @targetNode.
Signed-off-by: Michal Privoznik <mprivozn(a)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
10:11 a.m.
New subject: [PATCH 7/7] conf: Declare and use autoptr for virDomainMemoryDef
Register virDomainMemoryDefFree() to do the cleanup.
Signed-off-by: Michal Privoznik <mprivozn(a)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
11:03 a.m.
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-n...
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(a)redhat.com>
675
days inactive
675
days old
11 comments
4 participants
participants (4)
-
Michal Privoznik -
Michal Prívozník
-
Peter Krempa -
Tim Wiederhake