6:59 a.m.
Tim pointed out that some of the code could be simplified a bit more by
using virTristateBool parsing functions and also that strnlen could be
used instead of strlen.
This series does those cleanups as well as one further refactor of
character verification in virDomainDeviceLoadparmIsValid.
Peter Krempa (6):
util: enum: Add helpers for converting virTristate* to a plain bool
conf: seclabel: Parse booleans using virXMLPropTristateBool instead of
virStringParseYesNo
internal: Add STRLIM macro for checking string length using strnlen()
virDomainDeviceLoadparmIsValid: Simplify value lenght check
virDomainDeviceLoadparmIsValid: Use 'strspn' instead of a loop
conf: domain: Convert all string length checks to STRLIM
src/conf/domain_conf.c | 83 +++++++------------
src/internal.h | 9 ++
src/libvirt_private.syms | 2 +
src/util/virenum.c | 54 ++++++++++++
src/util/virenum.h | 2 +
.../machine-loadparm-s390-char-invalid.err | 2 +-
.../machine-loadparm-s390-len-invalid.err | 2 +-
7 files changed, 99 insertions(+), 55 deletions(-)
--
2.31.1
6:59 a.m.
New subject: [PATCH 1/6] util: enum: Add helpers for converting virTristate* to a plain bool
The helpers will update the passed boolean if the tristate's value is
not _ABSENT.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/libvirt_private.syms | 2 ++
src/util/virenum.c | 54 ++++++++++++++++++++++++++++++++++++++++
src/util/virenum.h | 2 ++
3 files changed, 58 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index b98cb0f66d..49795e5b81 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2147,9 +2147,11 @@ ebtablesRemoveForwardAllowIn;
virEnumFromString;
virEnumToString;
virTristateBoolFromBool;
+virTristateBoolToBool;
virTristateBoolTypeFromString;
virTristateBoolTypeToString;
virTristateSwitchFromBool;
+virTristateSwitchToBool;
virTristateSwitchTypeFromString;
virTristateSwitchTypeToString;
diff --git a/src/util/virenum.c b/src/util/virenum.c
index 26093bd795..103f00b524 100644
--- a/src/util/virenum.c
+++ b/src/util/virenum.c
@@ -47,6 +47,33 @@ virTristateBoolFromBool(bool val)
}
+/**
+ * virTristateBoolToBool:
+ * @t: a virTristateBool value
+ * @b: pointer to a boolean to be updated according to the value of @t
+ *
+ * The value pointed to by @b is updated if the tristate value @t is not absent.
+ */
+void
+virTristateBoolToBool(virTristateBool t,
+ bool *b)
+{
+ switch (t) {
+ case VIR_TRISTATE_BOOL_YES:
+ *b = true;
+ break;
+
+ case VIR_TRISTATE_BOOL_NO:
+ *b = false;
+ break;
+
+ case VIR_TRISTATE_BOOL_ABSENT:
+ case VIR_TRISTATE_BOOL_LAST:
+ break;
+ }
+}
+
+
virTristateSwitch
virTristateSwitchFromBool(bool val)
{
@@ -57,6 +84,33 @@ virTristateSwitchFromBool(bool val)
}
+/**
+ * virTristateSwitchToBool:
+ * @t: a virTristateSwitch value
+ * @b: pointer to a boolean to be updated according to the value of @t
+ *
+ * The value pointed to by @b is updated if the tristate value @t is not absent.
+ */
+void
+virTristateSwitchToBool(virTristateSwitch t,
+ bool *b)
+{
+ switch (t) {
+ case VIR_TRISTATE_SWITCH_ON:
+ *b = true;
+ break;
+
+ case VIR_TRISTATE_SWITCH_OFF:
+ *b = false;
+ break;
+
+ case VIR_TRISTATE_SWITCH_ABSENT:
+ case VIR_TRISTATE_SWITCH_LAST:
+ break;
+ }
+}
+
+
int
virEnumFromString(const char * const *types,
unsigned int ntypes,
diff --git a/src/util/virenum.h b/src/util/virenum.h
index d74af35530..98f01d574d 100644
--- a/src/util/virenum.h
+++ b/src/util/virenum.h
@@ -68,7 +68,9 @@ VIR_ENUM_DECL(virTristateBool);
VIR_ENUM_DECL(virTristateSwitch);
virTristateBool virTristateBoolFromBool(bool val);
+void virTristateBoolToBool(virTristateBool t, bool *b);
virTristateSwitch virTristateSwitchFromBool(bool val);
+void virTristateSwitchToBool(virTristateSwitch t, bool *b);
/* the two enums must be in sync to be able to use helpers interchangeably in
* some special cases */
--
2.31.1
6:59 a.m.
New subject: [PATCH 2/6] conf: seclabel: Parse booleans using virXMLPropTristateBool instead of virStringParseYesNo
Reduce the extent of custom logcic and custom error messages by using
virXMLPropTristateBool.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/conf/domain_conf.c | 41 +++++++++++++++++------------------------
1 file changed, 17 insertions(+), 24 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index f88405ab02..0203d17e9d 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -7801,7 +7801,7 @@ virSecurityLabelDefParseXML(xmlXPathContextPtr ctxt,
unsigned int flags)
{
g_autofree char *model = NULL;
- g_autofree char *relabel = NULL;
+ virTristateBool relabel = VIR_TRISTATE_BOOL_ABSENT;
g_autoptr(virSecurityLabelDef) seclabel = NULL;
if ((model = virXMLPropString(ctxt->node, "model")) &&
@@ -7824,13 +7824,10 @@ virSecurityLabelDefParseXML(xmlXPathContextPtr ctxt,
seclabel->type == VIR_DOMAIN_SECLABEL_NONE)
seclabel->relabel = false;
- if ((relabel = virXMLPropString(ctxt->node, "relabel"))) {
- if (virStringParseYesNo(relabel, &seclabel->relabel) < 0) {
- virReportError(VIR_ERR_XML_ERROR,
- _("invalid security relabel value '%s'"),
relabel);
- return NULL;
- }
- }
+ if (virXMLPropTristateBool(ctxt->node, "relabel", VIR_XML_PROP_NONZERO,
&relabel) < 0)
+ return NULL;
+
+ virTristateBoolToBool(relabel, &seclabel->relabel);
if (seclabel->type == VIR_DOMAIN_SECLABEL_DYNAMIC &&
!seclabel->relabel) {
@@ -8029,9 +8026,8 @@ virSecurityDeviceLabelDefParseXML(virSecurityDeviceLabelDef
***seclabels_rtn,
for (i = 0; i < n; i++) {
g_autofree char *model = NULL;
- g_autofree char *relabel = NULL;
g_autofree char *label = NULL;
- g_autofree char *labelskip = NULL;
+ virTristateBool t;
/* get model associated to this override */
model = virXMLPropString(list[i], "model");
@@ -8047,23 +8043,20 @@ virSecurityDeviceLabelDefParseXML(virSecurityDeviceLabelDef
***seclabels_rtn,
seclabels[i]->model = g_steal_pointer(&model);
}
- relabel = virXMLPropString(list[i], "relabel");
- if (relabel != NULL) {
- if (virStringParseYesNo(relabel, &seclabels[i]->relabel) < 0) {
- virReportError(VIR_ERR_XML_ERROR,
- _("invalid security relabel value %s"),
- relabel);
- goto error;
- }
- } else {
- seclabels[i]->relabel = true;
- }
+ if (virXMLPropTristateBool(list[i], "relabel", VIR_XML_PROP_NONZERO,
&t) < 0)
+ goto error;
+
+ seclabels[i]->relabel = true;
+ virTristateBoolToBool(t, &seclabels[i]->relabel);
/* labelskip is only parsed on live images */
- labelskip = virXMLPropString(list[i], "labelskip");
seclabels[i]->labelskip = false;
- if (labelskip && !(flags & VIR_DOMAIN_DEF_PARSE_INACTIVE))
- ignore_value(virStringParseYesNo(labelskip,
&seclabels[i]->labelskip));
+ if (!(flags & VIR_DOMAIN_DEF_PARSE_INACTIVE)) {
+ if (virXMLPropTristateBool(list[i], "labelskip",
VIR_XML_PROP_NONZERO, &t) < 0)
+ goto error;
+
+ virTristateBoolToBool(t, &seclabels[i]->labelskip);
+ }
ctxt->node = list[i];
label = virXPathString("string(./label)", ctxt);
--
2.31.1
7:57 a.m.
New subject: [PATCH 2/6] conf: seclabel: Parse booleans using virXMLPropTristateBool instead of virStringParseYesNo
On a Thursday in 2021, Peter Krempa wrote:
Reduce the extent of custom logcic and custom error messages by using
logic
virXMLPropTristateBool.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/conf/domain_conf.c | 41 +++++++++++++++++------------------------
1 file changed, 17 insertions(+), 24 deletions(-)
6:59 a.m.
New subject: [PATCH 3/6] internal: Add STRLIM macro for checking string length using strnlen()
As a microoprimization when checking whether length of a string fits
into a limit we don't necessarily need to calculate the full lenght but
can use strnlen to check only LIMIT+1 chars. Add a macro which will
simplify the expressions.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/internal.h | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/src/internal.h b/src/internal.h
index d3809bf057..2e404cd705 100644
--- a/src/internal.h
+++ b/src/internal.h
@@ -87,6 +87,15 @@
#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)
+/**
+ * STRLIM
+ * @str: pointer to a string (evaluated once)
+ * @lim: length limit (evaluated twice)
+ *
+ * Evaluates as true if length of @str doesn't exceed the limit @lim. Note
+ * that @lim + 1 characters may be accessed.
+ */
+#define STRLIM(str, lim) (strnlen((str), (lim) + 1) <= (lim))
#define STREQ_NULLABLE(a, b) (g_strcmp0(a, b) == 0)
#define STRNEQ_NULLABLE(a, b) (g_strcmp0(a, b) != 0)
--
2.31.1
7:57 a.m.
New subject: [PATCH 3/6] internal: Add STRLIM macro for checking string length using strnlen()
On a Thursday in 2021, Peter Krempa wrote:
As a microoprimization when checking whether length of a string fits
into a limit we don't necessarily need to calculate the full lenght but
length
can use strnlen to check only LIMIT+1 chars. Add a macro which will
simplify the expressions.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/internal.h | 9 +++++++++
1 file changed, 9 insertions(+)
6:59 a.m.
New subject: [PATCH 4/6] virDomainDeviceLoadparmIsValid: Simplify value lenght check
Use the new STRLIM macro and unify it with the empty string check.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/conf/domain_conf.c | 11 +++--------
.../machine-loadparm-s390-len-invalid.err | 2 +-
2 files changed, 4 insertions(+), 9 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 0203d17e9d..c542782750 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -6300,15 +6300,10 @@ virDomainDeviceLoadparmIsValid(const char *loadparm)
{
size_t i;
- if (virStringIsEmpty(loadparm)) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("loadparm cannot be an empty string"));
- return false;
- }
-
- if (strlen(loadparm) > 8) {
+ if (virStringIsEmpty(loadparm) || !STRLIM(loadparm, 8)) {
virReportError(VIR_ERR_INTERNAL_ERROR,
- _("loadparm '%s' exceeds 8 characters"),
loadparm);
+ _("loadparm value '%s' must be between 1 and 8
characters"),
+ loadparm);
return false;
}
diff --git a/tests/qemuxml2argvdata/machine-loadparm-s390-len-invalid.err
b/tests/qemuxml2argvdata/machine-loadparm-s390-len-invalid.err
index 9afaa68ae2..9fd0425f20 100644
--- a/tests/qemuxml2argvdata/machine-loadparm-s390-len-invalid.err
+++ b/tests/qemuxml2argvdata/machine-loadparm-s390-len-invalid.err
@@ -1 +1 @@
-internal error: loadparm 'LOADPARM1' exceeds 8 characters
+internal error: loadparm value 'LOADPARM1' must be between 1 and 8 characters
--
2.31.1
6:59 a.m.
New subject: [PATCH 5/6] virDomainDeviceLoadparmIsValid: Use 'strspn' instead of a loop
In other places we use strspn to validate a character subset. Convert
the in-place loop and simplify the error message.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/conf/domain_conf.c | 19 +++++--------------
.../machine-loadparm-s390-char-invalid.err | 2 +-
2 files changed, 6 insertions(+), 15 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index c542782750..b1cc229d97 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -6298,8 +6298,6 @@ virDomainObjCheckActive(virDomainObj *dom)
static bool
virDomainDeviceLoadparmIsValid(const char *loadparm)
{
- size_t i;
-
if (virStringIsEmpty(loadparm) || !STRLIM(loadparm, 8)) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("loadparm value '%s' must be between 1 and 8
characters"),
@@ -6307,18 +6305,11 @@ virDomainDeviceLoadparmIsValid(const char *loadparm)
return false;
}
- for (i = 0; i < strlen(loadparm); i++) {
- uint8_t c = loadparm[i];
-
- if (('A' <= c && c <= 'Z') || ('0' <= c
&& c <= '9') ||
- (c == '.') || (c == ' ')) {
- continue;
- } else {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("invalid loadparm char '%c', expecting
chars"
- " in set of [a-zA-Z0-9.] and blank spaces"), c);
- return false;
- }
+ if (strspn(loadparm,
"ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789. ") !=
strlen(loadparm)) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("invalid loadparm value '%s', expecting chars in
set of [a-zA-Z0-9.] and blank spaces"),
+ loadparm);
+ return false;
}
return true;
diff --git a/tests/qemuxml2argvdata/machine-loadparm-s390-char-invalid.err
b/tests/qemuxml2argvdata/machine-loadparm-s390-char-invalid.err
index c3eb455594..0283db9f48 100644
--- a/tests/qemuxml2argvdata/machine-loadparm-s390-char-invalid.err
+++ b/tests/qemuxml2argvdata/machine-loadparm-s390-char-invalid.err
@@ -1 +1 @@
-internal error: invalid loadparm char '?', expecting chars in set of [a-zA-Z0-9.]
and blank spaces
+internal error: invalid loadparm value 'SYS1?', expecting chars in set of
[a-zA-Z0-9.] and blank spaces
--
2.31.1
6:59 a.m.
New subject: [PATCH 6/6] conf: domain: Convert all string length checks to STRLIM
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/conf/domain_conf.c | 14 ++++++--------
1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index b1cc229d97..2e88526eb4 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -7791,7 +7791,7 @@ virSecurityLabelDefParseXML(xmlXPathContextPtr ctxt,
g_autoptr(virSecurityLabelDef) seclabel = NULL;
if ((model = virXMLPropString(ctxt->node, "model")) &&
- strlen(model) >= VIR_SECURITY_MODEL_BUFLEN - 1)
+ !STRLIM(model, VIR_SECURITY_MODEL_BUFLEN - 1))
g_clear_pointer(&model, g_free);
if (!(seclabel = virSecurityLabelDefNew(model)))
@@ -7856,8 +7856,7 @@ virSecurityLabelDefParseXML(xmlXPathContextPtr ctxt,
(!(flags & VIR_DOMAIN_DEF_PARSE_INACTIVE) &&
seclabel->type != VIR_DOMAIN_SECLABEL_NONE)) {
seclabel->label = virXPathString("string(./label[1])", ctxt);
- if (!seclabel->label ||
- strlen(seclabel->label) >= VIR_SECURITY_LABEL_BUFLEN - 1) {
+ if (!seclabel->label || !STRLIM(seclabel->label, VIR_SECURITY_LABEL_BUFLEN
- 1)) {
virReportError(VIR_ERR_XML_ERROR,
"%s", _("security label is missing"));
return NULL;
@@ -7870,8 +7869,7 @@ virSecurityLabelDefParseXML(xmlXPathContextPtr ctxt,
seclabel->type != VIR_DOMAIN_SECLABEL_NONE)) {
seclabel->imagelabel = virXPathString("string(./imagelabel[1])",
ctxt);
- if (!seclabel->imagelabel ||
- strlen(seclabel->imagelabel) >= VIR_SECURITY_LABEL_BUFLEN - 1) {
+ if (!seclabel->imagelabel || !STRLIM(seclabel->imagelabel,
VIR_SECURITY_LABEL_BUFLEN - 1)) {
virReportError(VIR_ERR_XML_ERROR,
"%s", _("security imagelabel is
missing"));
return NULL;
@@ -7883,7 +7881,7 @@ virSecurityLabelDefParseXML(xmlXPathContextPtr ctxt,
seclabel->baselabel = virXPathString("string(./baselabel[1])",
ctxt);
if (seclabel->baselabel &&
- strlen(seclabel->baselabel) >= VIR_SECURITY_LABEL_BUFLEN - 1)
+ !STRLIM(seclabel->baselabel, VIR_SECURITY_LABEL_BUFLEN - 1))
g_clear_pointer(&seclabel->baselabel, g_free);
}
@@ -8047,7 +8045,7 @@ virSecurityDeviceLabelDefParseXML(virSecurityDeviceLabelDef
***seclabels_rtn,
ctxt->node = list[i];
label = virXPathString("string(./label)", ctxt);
- if (label && strlen(label) < VIR_SECURITY_LABEL_BUFLEN)
+ if (label && STRLIM(label, VIR_SECURITY_LABEL_BUFLEN - 1))
seclabels[i]->label = g_steal_pointer(&label);
if (seclabels[i]->label && !seclabels[i]->relabel) {
@@ -17508,7 +17506,7 @@ virDomainFeaturesHyperVDefParse(virDomainDef *def,
return -1;
}
- if (strlen(def->hyperv_vendor_id) > VIR_DOMAIN_HYPERV_VENDOR_ID_MAX) {
+ if (!STRLIM(def->hyperv_vendor_id, VIR_DOMAIN_HYPERV_VENDOR_ID_MAX)) {
virReportError(VIR_ERR_XML_ERROR,
_("HyperV vendor_id value must not be more than %d
characters."),
VIR_DOMAIN_HYPERV_VENDOR_ID_MAX);
--
2.31.1
7:58 a.m.
On a Thursday in 2021, Peter Krempa wrote:
Tim pointed out that some of the code could be simplified a bit more
by
using virTristateBool parsing functions and also that strnlen could be
used instead of strlen.
This series does those cleanups as well as one further refactor of
character verification in virDomainDeviceLoadparmIsValid.
Peter Krempa (6):
util: enum: Add helpers for converting virTristate* to a plain bool
conf: seclabel: Parse booleans using virXMLPropTristateBool instead of
virStringParseYesNo
internal: Add STRLIM macro for checking string length using strnlen()
virDomainDeviceLoadparmIsValid: Simplify value lenght check
virDomainDeviceLoadparmIsValid: Use 'strspn' instead of a loop
conf: domain: Convert all string length checks to STRLIM
src/conf/domain_conf.c | 83 +++++++------------
src/internal.h | 9 ++
src/libvirt_private.syms | 2 +
src/util/virenum.c | 54 ++++++++++++
src/util/virenum.h | 2 +
.../machine-loadparm-s390-char-invalid.err | 2 +-
.../machine-loadparm-s390-len-invalid.err | 2 +-
7 files changed, 99 insertions(+), 55 deletions(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
1092
days inactive
1092
days old
9 comments
2 participants
participants (2)
-
Ján Tomko -
Peter Krempa