[libvirt PATCH 0/6] More XML parsing boiler code refactoring
In the spirit of https://listman.redhat.com/archives/libvir-list/2021-May/msg00550.html. Tim Wiederhake (6): conf: Add AUTOPTR_CLEANUP_FUNC for virDomainSEVDef conf: virDomainSEVDef: Change type of "sectype" to virDomainLaunchSecurity virDomainSEVDefParseXML: Use virXMLPropEnum virDomainSEVDefParseXML: Use automatic memory management virDomainSEVDefParseXML: Remove superfluous `goto`s virDomainSEVDefParseXML: Remove superfluous variable initialization src/conf/domain_conf.c | 41 +++++++++++------------------------------ src/conf/domain_conf.h | 4 +++- 2 files changed, 14 insertions(+), 31 deletions(-) -- 2.31.1
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/domain_conf.c | 2 +- src/conf/domain_conf.h | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 04c10df0a9..c1438d85f4 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3490,7 +3490,7 @@ virDomainResctrlDefFree(virDomainResctrlDef *resctrl) } -static void +void virDomainSEVDefFree(virDomainSEVDef *def) { if (!def) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 4d9d499b16..a14a2090f8 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2661,6 +2661,8 @@ struct _virDomainSEVDef { unsigned int reduced_phys_bits; }; +void virDomainSEVDefFree(virDomainSEVDef *def); +G_DEFINE_AUTOPTR_CLEANUP_FUNC(virDomainSEVDef, virDomainSEVDefFree); typedef enum { VIR_DOMAIN_IOMMU_MODEL_INTEL, -- 2.31.1
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/domain_conf.c | 6 ++++-- src/conf/domain_conf.h | 2 +- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index c1438d85f4..68ab18f3ab 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -14722,6 +14722,7 @@ virDomainSEVDefParseXML(xmlNodePtr sevNode, virDomainSEVDef *def; unsigned long policy; g_autofree char *type = NULL; + int sectype; int rc = -1; def = g_new0(virDomainSEVDef, 1); @@ -14734,8 +14735,8 @@ virDomainSEVDefParseXML(xmlNodePtr sevNode, goto error; } - def->sectype = virDomainLaunchSecurityTypeFromString(type); - switch ((virDomainLaunchSecurity) def->sectype) { + sectype = virDomainLaunchSecurityTypeFromString(type); + switch ((virDomainLaunchSecurity) sectype) { case VIR_DOMAIN_LAUNCH_SECURITY_SEV: break; case VIR_DOMAIN_LAUNCH_SECURITY_NONE: @@ -14746,6 +14747,7 @@ virDomainSEVDefParseXML(xmlNodePtr sevNode, type); goto error; } + def->sectype = sectype; if (virXPathULongHex("string(./policy)", ctxt, &policy) < 0) { virReportError(VIR_ERR_XML_ERROR, "%s", diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index a14a2090f8..c31531c93b 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2651,7 +2651,7 @@ typedef enum { struct _virDomainSEVDef { - int sectype; /* enum virDomainLaunchSecurity */ + virDomainLaunchSecurity sectype; char *dh_cert; char *session; unsigned int policy; -- 2.31.1
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/domain_conf.c | 23 +++-------------------- 1 file changed, 3 insertions(+), 20 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 68ab18f3ab..dda615a8ba 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -14721,33 +14721,16 @@ virDomainSEVDefParseXML(xmlNodePtr sevNode, VIR_XPATH_NODE_AUTORESTORE(ctxt) virDomainSEVDef *def; unsigned long policy; - g_autofree char *type = NULL; - int sectype; int rc = -1; def = g_new0(virDomainSEVDef, 1); ctxt->node = sevNode; - if (!(type = virXMLPropString(sevNode, "type"))) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("missing launch security type")); - goto error; - } - - sectype = virDomainLaunchSecurityTypeFromString(type); - switch ((virDomainLaunchSecurity) sectype) { - case VIR_DOMAIN_LAUNCH_SECURITY_SEV: - break; - case VIR_DOMAIN_LAUNCH_SECURITY_NONE: - case VIR_DOMAIN_LAUNCH_SECURITY_LAST: - default: - virReportError(VIR_ERR_XML_ERROR, - _("unsupported launch security type '%s'"), - type); + if (virXMLPropEnum(sevNode, "type", virDomainLaunchSecurityTypeFromString, + VIR_XML_PROP_NONZERO | VIR_XML_PROP_REQUIRED, + &def->sectype) < 0) goto error; - } - def->sectype = sectype; if (virXPathULongHex("string(./policy)", ctxt, &policy) < 0) { virReportError(VIR_ERR_XML_ERROR, "%s", -- 2.31.1
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/domain_conf.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index dda615a8ba..dd803e6df5 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -14719,7 +14719,7 @@ virDomainSEVDefParseXML(xmlNodePtr sevNode, xmlXPathContextPtr ctxt) { VIR_XPATH_NODE_AUTORESTORE(ctxt) - virDomainSEVDef *def; + g_autoptr(virDomainSEVDef) def = NULL; unsigned long policy; int rc = -1; @@ -14765,10 +14765,9 @@ virDomainSEVDefParseXML(xmlNodePtr sevNode, def->dh_cert = virXPathString("string(./dhCert)", ctxt); def->session = virXPathString("string(./session)", ctxt); - return def; + return g_steal_pointer(&def); error: - virDomainSEVDefFree(def); return NULL; } -- 2.31.1
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/domain_conf.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index dd803e6df5..db8ec23d70 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -14730,12 +14730,12 @@ virDomainSEVDefParseXML(xmlNodePtr sevNode, if (virXMLPropEnum(sevNode, "type", virDomainLaunchSecurityTypeFromString, VIR_XML_PROP_NONZERO | VIR_XML_PROP_REQUIRED, &def->sectype) < 0) - goto error; + return NULL; if (virXPathULongHex("string(./policy)", ctxt, &policy) < 0) { virReportError(VIR_ERR_XML_ERROR, "%s", _("failed to get launch security policy")); - goto error; + return NULL; } /* the following attributes are platform dependent and if missing, we can @@ -14747,7 +14747,7 @@ virDomainSEVDefParseXML(xmlNodePtr sevNode, } else if (rc == -2) { virReportError(VIR_ERR_XML_ERROR, "%s", _("Invalid format for launch security cbitpos")); - goto error; + return NULL; } rc = virXPathUInt("string(./reducedPhysBits)", ctxt, @@ -14758,7 +14758,7 @@ virDomainSEVDefParseXML(xmlNodePtr sevNode, virReportError(VIR_ERR_XML_ERROR, "%s", _("Invalid format for launch security " "reduced-phys-bits")); - goto error; + return NULL; } def->policy = policy; @@ -14766,9 +14766,6 @@ virDomainSEVDefParseXML(xmlNodePtr sevNode, def->session = virXPathString("string(./session)", ctxt); return g_steal_pointer(&def); - - error: - return NULL; } -- 2.31.1
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/domain_conf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index db8ec23d70..2d8ae7e860 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -14721,7 +14721,7 @@ virDomainSEVDefParseXML(xmlNodePtr sevNode, VIR_XPATH_NODE_AUTORESTORE(ctxt) g_autoptr(virDomainSEVDef) def = NULL; unsigned long policy; - int rc = -1; + int rc; def = g_new0(virDomainSEVDef, 1); -- 2.31.1
On Mon, Jul 05, 2021 at 12:46:54PM +0200, Tim Wiederhake wrote:
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/domain_conf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index db8ec23d70..2d8ae7e860 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -14721,7 +14721,7 @@ virDomainSEVDefParseXML(xmlNodePtr sevNode, VIR_XPATH_NODE_AUTORESTORE(ctxt) g_autoptr(virDomainSEVDef) def = NULL; unsigned long policy; - int rc = -1; + int rc;
This is just a pointless nitpick (given that this has already been pushed), but initializing a variable at the time of its definition is actually a good practice, so there was really no point in this patch, the only bad thing about the code segment is that we're inconsistent. Regards, Erik
On a %A in %Y, Tim Wiederhake wrote:
In the spirit of https://listman.redhat.com/archives/libvir-list/2021-May/msg00550.html.
Tim Wiederhake (6): conf: Add AUTOPTR_CLEANUP_FUNC for virDomainSEVDef conf: virDomainSEVDef: Change type of "sectype" to virDomainLaunchSecurity virDomainSEVDefParseXML: Use virXMLPropEnum virDomainSEVDefParseXML: Use automatic memory management virDomainSEVDefParseXML: Remove superfluous `goto`s virDomainSEVDefParseXML: Remove superfluous variable initialization
src/conf/domain_conf.c | 41 +++++++++++------------------------------ src/conf/domain_conf.h | 4 +++- 2 files changed, 14 insertions(+), 31 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> and pushed Jano
participants (3)
-
Erik Skultety -
Jano Tomko -
Tim Wiederhake