Re: [libvirt] [PATCH v2 09/31] qemu: Change return type of virQEMUCapsFetchCPUDefinitions

2019-10-16 Thread Ján Tomko

On Tue, Oct 15, 2019 at 05:34:45PM +0200, Jiri Denemark wrote:

The function would return a valid virDomainCapsCPUModelsPtr with empty
CPU models list if query-cpu-definitions exists in QEMU, but returns
GenericError meaning it's not in fact implemented. This behaviour is a
bit strange especially after such virDomainCapsCPUModels structure is
stored in capabilities XML and parsed back, which will result in NULL
virDomainCapsCPUModelsPtr rather than a structure containing nothing.

Let's just keep virDomainCapsCPUModelsPtr NULL if the QMP command is not
implemented and change the return value to int so that callers can
easily check for failure or success.

Signed-off-by: Jiri Denemark 
---

Notes:
   Version 2:
   - new patch

src/qemu/qemu_capabilities.c | 34 +-
src/qemu/qemu_capabilities.h |  5 +++--
src/qemu/qemu_process.c  | 17 ++---
3 files changed, 34 insertions(+), 22 deletions(-)



Reviewed-by: Ján Tomko 

Your call whether you consider squashing in the following, since
VIR_STEAL_PTR still hasn't been eliminated:

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index c8b3b65aad..8e0fae54d7 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -2510,7 +2510,7 @@ virQEMUCapsFetchCPUDefinitions(qemuMonitorPtr mon,
goto cleanup;
}

-VIR_STEAL_PTR(*cpuModels, models);
+*cpuModels = g_steal_pointer();
ret = 0;

 cleanup:



signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v2 09/31] qemu: Change return type of virQEMUCapsFetchCPUDefinitions

2019-10-15 Thread Jiri Denemark
The function would return a valid virDomainCapsCPUModelsPtr with empty
CPU models list if query-cpu-definitions exists in QEMU, but returns
GenericError meaning it's not in fact implemented. This behaviour is a
bit strange especially after such virDomainCapsCPUModels structure is
stored in capabilities XML and parsed back, which will result in NULL
virDomainCapsCPUModelsPtr rather than a structure containing nothing.

Let's just keep virDomainCapsCPUModelsPtr NULL if the QMP command is not
implemented and change the return value to int so that callers can
easily check for failure or success.

Signed-off-by: Jiri Denemark 
---

Notes:
Version 2:
- new patch

 src/qemu/qemu_capabilities.c | 34 +-
 src/qemu/qemu_capabilities.h |  5 +++--
 src/qemu/qemu_process.c  | 17 ++---
 3 files changed, 34 insertions(+), 22 deletions(-)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index b608eb1a43..6fa5e06edb 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -2452,17 +2452,26 @@ virQEMUCapsProbeQMPMachineProps(virQEMUCapsPtr qemuCaps,
 }
 
 
-virDomainCapsCPUModelsPtr
+int
 virQEMUCapsFetchCPUDefinitions(qemuMonitorPtr mon,
-   virArch arch)
+   virArch arch,
+   virDomainCapsCPUModelsPtr *cpuModels)
 {
 virDomainCapsCPUModelsPtr models = NULL;
 qemuMonitorCPUDefInfoPtr *cpus = NULL;
 int ncpus = 0;
 size_t i;
+int ret = -1;
+
+*cpuModels = NULL;
 
 if ((ncpus = qemuMonitorGetCPUDefinitions(mon, )) < 0)
-return NULL;
+return -1;
+
+if (ncpus == 0) {
+ret = 0;
+goto cleanup;
+}
 
 /* QEMU 2.11 for Power renamed all CPU models to lower case, we need to
  * translate them back to libvirt's upper case model names. */
@@ -2471,7 +2480,7 @@ virQEMUCapsFetchCPUDefinitions(qemuMonitorPtr mon,
 char **name;
 
 if (virCPUGetModels(arch, ) < 0)
-goto error;
+goto cleanup;
 
 for (name = libvirtModels; name && *name; name++) {
 for (i = 0; i < ncpus; i++) {
@@ -2480,13 +2489,13 @@ virQEMUCapsFetchCPUDefinitions(qemuMonitorPtr mon,
 
 VIR_FREE(cpus[i]->name);
 if (VIR_STRDUP(cpus[i]->name, *name) < 0)
-goto error;
+goto cleanup;
 }
 }
 }
 
 if (!(models = virDomainCapsCPUModelsNew(ncpus)))
-goto error;
+goto cleanup;
 
 for (i = 0; i < ncpus; i++) {
 virDomainCapsCPUUsable usable = VIR_DOMCAPS_CPU_USABLE_UNKNOWN;
@@ -2498,19 +2507,18 @@ virQEMUCapsFetchCPUDefinitions(qemuMonitorPtr mon,
 
 if (virDomainCapsCPUModelsAddSteal(models, [i]->name, usable,
[i]->blockers) < 0)
-goto error;
+goto cleanup;
 }
 
+VIR_STEAL_PTR(*cpuModels, models);
+ret = 0;
+
  cleanup:
 for (i = 0; i < ncpus; i++)
 qemuMonitorCPUDefInfoFree(cpus[i]);
 VIR_FREE(cpus);
-return models;
-
- error:
 virObjectUnref(models);
-models = NULL;
-goto cleanup;
+return ret;
 }
 
 
@@ -2524,7 +2532,7 @@ virQEMUCapsProbeQMPCPUDefinitions(virQEMUCapsPtr qemuCaps,
 if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_QUERY_CPU_DEFINITIONS))
 return 0;
 
-if (!(models = virQEMUCapsFetchCPUDefinitions(mon, qemuCaps->arch)))
+if (virQEMUCapsFetchCPUDefinitions(mon, qemuCaps->arch, ) < 0)
 return -1;
 
 if (tcg || !virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM))
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index 67dccc522e..ba3fe3d2b6 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -573,8 +573,9 @@ virDomainCapsCPUModelsPtr 
virQEMUCapsGetCPUDefinitions(virQEMUCapsPtr qemuCaps,
virDomainVirtType type,
const char 
**modelWhitelist,
const char 
**modelBlacklist);
-virDomainCapsCPUModelsPtr virQEMUCapsFetchCPUDefinitions(qemuMonitorPtr mon,
- virArch arch);
+int virQEMUCapsFetchCPUDefinitions(qemuMonitorPtr mon,
+   virArch arch,
+   virDomainCapsCPUModelsPtr *cpuModels);
 
 typedef enum {
 /* Host CPU definition reported in domain capabilities. */
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index f37acab9e4..93fe994f8e 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -4353,27 +4353,30 @@ qemuProcessUpdateAndVerifyCPU(virQEMUDriverPtr driver,
 }
 
 
-static virDomainCapsCPUModelsPtr
+static int
 qemuProcessFetchCPUDefinitions(virQEMUDriverPtr driver,
virDomainObjPtr vm,
-