[PATCH 0/3] qemu: Refactor typed parameter list construction in SEV apis
Peter Krempa (3): qemuDomainGetLaunchSecurityInfo: Don't forget unlock VM object on (impossible) error qemuDomainGetLaunchSecurityInfo: Use virTypedParamList to construct return value qemuNodeGetSEVInfo: Use virTypedParamList to construct return value src/qemu/qemu_driver.c | 107 ++++++++++++----------------------------- 1 file changed, 31 insertions(+), 76 deletions(-) -- 2.48.1
If 'vm->def->sec->sectype' would be invalid; which is currently not possible; we'd not unlock the domain object. Fix the logic even when the bug currently can't happen. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index a5122d0cd6..45bb3c09aa 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -19056,7 +19056,7 @@ qemuDomainGetLaunchSecurityInfo(virDomainPtr domain, case VIR_DOMAIN_LAUNCH_SECURITY_NONE: case VIR_DOMAIN_LAUNCH_SECURITY_LAST: virReportEnumRangeError(virDomainLaunchSecurity, vm->def->sec->sectype); - return -1; + goto cleanup; } ret = 0; -- 2.48.1
On 3/11/25 12:59, Peter Krempa wrote:
If 'vm->def->sec->sectype' would be invalid; which is currently not possible; we'd not unlock the domain object. Fix the logic even when the bug currently can't happen.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index a5122d0cd6..45bb3c09aa 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -19056,7 +19056,7 @@ qemuDomainGetLaunchSecurityInfo(virDomainPtr domain, case VIR_DOMAIN_LAUNCH_SECURITY_NONE: case VIR_DOMAIN_LAUNCH_SECURITY_LAST: virReportEnumRangeError(virDomainLaunchSecurity, vm->def->sec->sectype); - return -1; + goto cleanup;
Ooops, looking at the commit that introduced this (no need to run git blame O;-)), there's another problem like this in virSecurityDACRestoreAllLabel(). But that function has some other issues too, so I'll post a patch shortly. Michal
Simplify the code by using the modern helpers. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 42 ++++++++++++++---------------------------- 1 file changed, 14 insertions(+), 28 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 45bb3c09aa..a40db1dd2e 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -18948,15 +18948,13 @@ qemuNodeGetSEVInfo(virConnectPtr conn, static int qemuDomainGetSEVInfo(virDomainObj *vm, - virTypedParameterPtr *params, - int *nparams, + virTypedParamList *list, unsigned int flags) { int ret = -1; int rv; g_autofree char *tmp = NULL; qemuMonitorSEVInfo info = { }; - int maxpar = 0; virCheckFlags(VIR_TYPED_PARAM_STRING_OKAY, -1); @@ -18981,36 +18979,20 @@ qemuDomainGetSEVInfo(virDomainObj *vm, if (rv < 0) goto endjob; - if (virTypedParamsAddString(params, nparams, &maxpar, - VIR_DOMAIN_LAUNCH_SECURITY_SEV_MEASUREMENT, - tmp) < 0) - goto endjob; - if (virTypedParamsAddUInt(params, nparams, &maxpar, - VIR_DOMAIN_LAUNCH_SECURITY_SEV_API_MAJOR, - info.apiMajor) < 0) - goto endjob; - if (virTypedParamsAddUInt(params, nparams, &maxpar, - VIR_DOMAIN_LAUNCH_SECURITY_SEV_API_MINOR, - info.apiMinor) < 0) - goto endjob; - if (virTypedParamsAddUInt(params, nparams, &maxpar, - VIR_DOMAIN_LAUNCH_SECURITY_SEV_BUILD_ID, - info.buildID) < 0) - goto endjob; + virTypedParamListAddString(list, tmp, VIR_DOMAIN_LAUNCH_SECURITY_SEV_MEASUREMENT); + virTypedParamListAddUInt(list, info.apiMajor, VIR_DOMAIN_LAUNCH_SECURITY_SEV_API_MAJOR); + virTypedParamListAddUInt(list, info.apiMinor, VIR_DOMAIN_LAUNCH_SECURITY_SEV_API_MINOR); + virTypedParamListAddUInt(list, info.buildID, VIR_DOMAIN_LAUNCH_SECURITY_SEV_BUILD_ID); switch (info.type) { case QEMU_MONITOR_SEV_GUEST_TYPE_SEV: - if (virTypedParamsAddUInt(params, nparams, &maxpar, - VIR_DOMAIN_LAUNCH_SECURITY_SEV_POLICY, - info.data.sev.policy) < 0) - goto endjob; + virTypedParamListAddUInt(list, info.data.sev.policy, + VIR_DOMAIN_LAUNCH_SECURITY_SEV_POLICY); break; case QEMU_MONITOR_SEV_GUEST_TYPE_SEV_SNP: - if (virTypedParamsAddULLong(params, nparams, &maxpar, - VIR_DOMAIN_LAUNCH_SECURITY_SEV_SNP_POLICY, - info.data.sev_snp.snp_policy) < 0) - goto endjob; + virTypedParamListAddULLong(list, info.data.sev_snp.snp_policy, + VIR_DOMAIN_LAUNCH_SECURITY_SEV_SNP_POLICY); break; case QEMU_MONITOR_SEV_GUEST_TYPE_LAST: @@ -19031,6 +19013,7 @@ qemuDomainGetLaunchSecurityInfo(virDomainPtr domain, int *nparams, unsigned int flags) { + g_autoptr(virTypedParamList) list = virTypedParamListNew(); virDomainObj *vm; int ret = -1; @@ -19048,7 +19031,7 @@ qemuDomainGetLaunchSecurityInfo(virDomainPtr domain, switch (vm->def->sec->sectype) { case VIR_DOMAIN_LAUNCH_SECURITY_SEV: case VIR_DOMAIN_LAUNCH_SECURITY_SEV_SNP: - if (qemuDomainGetSEVInfo(vm, params, nparams, flags) < 0) + if (qemuDomainGetSEVInfo(vm, list, flags) < 0) goto cleanup; break; case VIR_DOMAIN_LAUNCH_SECURITY_PV: @@ -19059,6 +19042,9 @@ qemuDomainGetLaunchSecurityInfo(virDomainPtr domain, goto cleanup; } + if (virTypedParamListSteal(list, params, nparams) < 0) + goto cleanup; + ret = 0; cleanup: -- 2.48.1
Simplify the code by using the modern helpers. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 63 +++++++++++------------------------------- 1 file changed, 16 insertions(+), 47 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index a40db1dd2e..2cabc7ee2f 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -18859,58 +18859,22 @@ qemuDomainSetLifecycleAction(virDomainPtr dom, } -static int +static void qemuGetSEVInfoToParams(virQEMUCaps *qemuCaps, - virTypedParameterPtr *params, - int *nparams, - unsigned int flags) + virTypedParamList *list) { - int maxpar = 0; - int n = 0; virSEVCapability *sev = virQEMUCapsGetSEVCapabilities(qemuCaps); - virTypedParameterPtr sevParams = NULL; - - virCheckFlags(VIR_TYPED_PARAM_STRING_OKAY, -1); - - if (virTypedParamsAddString(&sevParams, &n, &maxpar, - VIR_NODE_SEV_PDH, sev->pdh) < 0) - return -1; - - if (virTypedParamsAddString(&sevParams, &n, &maxpar, - VIR_NODE_SEV_CERT_CHAIN, sev->cert_chain) < 0) - goto cleanup; - - if ((sev->cpu0_id != NULL) && - (virTypedParamsAddString(&sevParams, &n, &maxpar, - VIR_NODE_SEV_CPU0_ID, sev->cpu0_id) < 0)) - goto cleanup; - if (virTypedParamsAddUInt(&sevParams, &n, &maxpar, - VIR_NODE_SEV_CBITPOS, sev->cbitpos) < 0) - goto cleanup; - - if (virTypedParamsAddUInt(&sevParams, &n, &maxpar, - VIR_NODE_SEV_REDUCED_PHYS_BITS, - sev->reduced_phys_bits) < 0) - goto cleanup; - - if (virTypedParamsAddUInt(&sevParams, &n, &maxpar, - VIR_NODE_SEV_MAX_GUESTS, - sev->max_guests) < 0) - goto cleanup; - - if (virTypedParamsAddUInt(&sevParams, &n, &maxpar, - VIR_NODE_SEV_MAX_ES_GUESTS, - sev->max_es_guests) < 0) - goto cleanup; + virTypedParamListAddString(list, sev->pdh, VIR_NODE_SEV_PDH); + virTypedParamListAddString(list, sev->cert_chain, VIR_NODE_SEV_CERT_CHAIN); - *params = g_steal_pointer(&sevParams); - *nparams = n; - return 0; + if (sev->cpu0_id != NULL) + virTypedParamListAddString(list, sev->cpu0_id, VIR_NODE_SEV_CPU0_ID); - cleanup: - virTypedParamsFree(sevParams, n); - return -1; + virTypedParamListAddUInt(list, sev->cbitpos, VIR_NODE_SEV_CBITPOS); + virTypedParamListAddUInt(list, sev->reduced_phys_bits, VIR_NODE_SEV_REDUCED_PHYS_BITS); + virTypedParamListAddUInt(list, sev->max_guests, VIR_NODE_SEV_MAX_GUESTS); + virTypedParamListAddUInt(list, sev->max_es_guests, VIR_NODE_SEV_MAX_ES_GUESTS); } @@ -18920,9 +18884,12 @@ qemuNodeGetSEVInfo(virConnectPtr conn, int *nparams, unsigned int flags) { + g_autoptr(virTypedParamList) list = virTypedParamListNew(); virQEMUDriver *driver = conn->privateData; g_autoptr(virQEMUCaps) qemucaps = NULL; + virCheckFlags(VIR_TYPED_PARAM_STRING_OKAY, -1); + if (virNodeGetSevInfoEnsureACL(conn) < 0) return -1; @@ -18939,7 +18906,9 @@ qemuNodeGetSEVInfo(virConnectPtr conn, return -1; } - if (qemuGetSEVInfoToParams(qemucaps, params, nparams, flags) < 0) + qemuGetSEVInfoToParams(qemucaps, list); + + if (virTypedParamListSteal(list, params, nparams) < 0) return -1; return 0; -- 2.48.1
On 3/11/25 12:59, Peter Krempa wrote:
Peter Krempa (3): qemuDomainGetLaunchSecurityInfo: Don't forget unlock VM object on (impossible) error qemuDomainGetLaunchSecurityInfo: Use virTypedParamList to construct return value qemuNodeGetSEVInfo: Use virTypedParamList to construct return value
src/qemu/qemu_driver.c | 107 ++++++++++++----------------------------- 1 file changed, 31 insertions(+), 76 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
participants (2)
-
Michal Prívozník -
Peter Krempa