[PATCH 0/6] virXMLPropStringLimit removal follow-up
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
The helpers will update the passed boolean if the tristate's value is not _ABSENT. Signed-off-by: Peter Krempa <pkrempa@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
Reduce the extent of custom logcic and custom error messages by using virXMLPropTristateBool. Signed-off-by: Peter Krempa <pkrempa@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
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@redhat.com> --- src/conf/domain_conf.c | 41 +++++++++++++++++------------------------ 1 file changed, 17 insertions(+), 24 deletions(-)
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@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
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@redhat.com> --- src/internal.h | 9 +++++++++ 1 file changed, 9 insertions(+)
Use the new STRLIM macro and unify it with the empty string check. Signed-off-by: Peter Krempa <pkrempa@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
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@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
Signed-off-by: Peter Krempa <pkrempa@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
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@redhat.com> Jano
participants (2)
-
Ján Tomko -
Peter Krempa