Re: [libvirt] [PATCH 03/41] qemu: Use virDomainCapsCPUModels for cpuDefinitions

2016-09-13 Thread Jiri Denemark 
On Mon, Aug 29, 2016 at 12:32:34 -0400, John Ferlan wrote:
...
> > -size_t virQEMUCapsGetCPUDefinitions(virQEMUCapsPtr qemuCaps,
> > -char ***names)
> > +int
> > +virQEMUCapsGetCPUDefinitions(virQEMUCapsPtr qemuCaps,
> > + char ***names,
> > + size_t *count)
> >  {
> > +size_t i;
> > +char **models = NULL;
> > +
> > +*count = 0;
> >  if (names)
> > -*names = qemuCaps->cpuDefinitions;
> > -return qemuCaps->ncpuDefinitions;
> > +*names = NULL;
> > +
> > +if (!qemuCaps->cpuDefinitions)
> > +return 0;
> > +
> > +if (names && VIR_ALLOC_N(models, qemuCaps->cpuDefinitions->count) < 0)
> > +return -1;
> > +
> 
> So if we don't have names, we don't get models and the following loop
> will only add to models if we've allocated it because we have names, right?
> 
> Thus could there be an optimization to set/return ->count if !names
> prior to this check? e.g.

It could, but only for a limited time. In the future the for loop will
do a bit of filtering and thus we will have to go through it to compute
@count.

> if (!names) {
> *count = qemuCaps->cpuDefinitions->count;
> return 0;
> }
> 
> This works, but it's tough to read.

> > +for (i = 0; i < qemuCaps->cpuDefinitions->count; i++) {
> > +virDomainCapsCPUModelPtr cpu = qemuCaps->cpuDefinitions->models + 
> > i;
> > +if (models && VIR_STRDUP(models[i], cpu->name) < 0)
> > +goto error;
> > +}
> > +
> > +if (names)
> > +*names = models;
> > +*count = qemuCaps->cpuDefinitions->count;
> > +return 0;
> > +
> > + error:
> > +virStringFreeListCount(models, i);
> > +return -1;
> >  }
...
> > @@ -4921,7 +4927,11 @@ int qemuMonitorJSONGetCPUDefinitions(qemuMonitorPtr 
> > mon,
> >  cpulist = NULL;
> >  
> >   cleanup:
> > -virStringFreeList(cpulist);
> > +if (ret < 0 && cpulist) {
> 
> I think "ret < 0" is superfluous... Coverity whines too

Right. Fixed.

Jirka

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 03/41] qemu: Use virDomainCapsCPUModels for cpuDefinitions

2016-08-29 Thread John Ferlan 


On 08/12/2016 09:32 AM, Jiri Denemark wrote:
> The list of supported CPU models in domain capabilities is stored in
> virDomainCapsCPUModels. Let's use the same object for storing CPU models
> in QEMU capabilities.
> 
> Signed-off-by: Jiri Denemark 
> ---
>  src/qemu/qemu_capabilities.c | 162 
> +++
>  src/qemu/qemu_capabilities.h |  10 +--
>  src/qemu/qemu_command.c  |   8 ++-
>  src/qemu/qemu_monitor.c  |  12 +++-
>  src/qemu/qemu_monitor.h  |  10 ++-
>  src/qemu/qemu_monitor_json.c |  24 +--
>  src/qemu/qemu_monitor_json.h |   2 +-
>  tests/qemumonitorjsontest.c  |   8 +--
>  tests/qemuxml2argvtest.c |  18 ++---
>  9 files changed, 164 insertions(+), 90 deletions(-)
> 

[..]

>  
>  
> -size_t virQEMUCapsGetCPUDefinitions(virQEMUCapsPtr qemuCaps,
> -char ***names)
> +int
> +virQEMUCapsGetCPUDefinitions(virQEMUCapsPtr qemuCaps,
> + char ***names,
> + size_t *count)
>  {
> +size_t i;
> +char **models = NULL;
> +
> +*count = 0;
>  if (names)
> -*names = qemuCaps->cpuDefinitions;
> -return qemuCaps->ncpuDefinitions;
> +*names = NULL;
> +
> +if (!qemuCaps->cpuDefinitions)
> +return 0;
> +
> +if (names && VIR_ALLOC_N(models, qemuCaps->cpuDefinitions->count) < 0)
> +return -1;
> +

So if we don't have names, we don't get models and the following loop
will only add to models if we've allocated it because we have names, right?

Thus could there be an optimization to set/return ->count if !names
prior to this check? e.g.

if (!names) {
*count = qemuCaps->cpuDefinitions->count;
return 0;
}

This works, but it's tough to read.

> +for (i = 0; i < qemuCaps->cpuDefinitions->count; i++) {
> +virDomainCapsCPUModelPtr cpu = qemuCaps->cpuDefinitions->models + i;
> +if (models && VIR_STRDUP(models[i], cpu->name) < 0)
> +goto error;
> +}
> +
> +if (names)
> +*names = models;
> +*count = qemuCaps->cpuDefinitions->count;
> +return 0;
> +
> + error:
> +virStringFreeListCount(models, i);
> +return -1;
>  }
>  

[...]

> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c
> @@ -4855,14 +4855,15 @@ int qemuMonitorJSONGetMachines(qemuMonitorPtr mon,
>  }
>  
>  
> -int qemuMonitorJSONGetCPUDefinitions(qemuMonitorPtr mon,
> - char ***cpus)
> +int
> +qemuMonitorJSONGetCPUDefinitions(qemuMonitorPtr mon,
> + qemuMonitorCPUDefInfoPtr **cpus)
>  {
>  int ret = -1;
>  virJSONValuePtr cmd;
>  virJSONValuePtr reply = NULL;
>  virJSONValuePtr data;
> -char **cpulist = NULL;
> +qemuMonitorCPUDefInfoPtr *cpulist = NULL;
>  int n = 0;
>  size_t i;
>  
> @@ -4898,13 +4899,18 @@ int qemuMonitorJSONGetCPUDefinitions(qemuMonitorPtr 
> mon,
>  goto cleanup;
>  }
>  
> -/* null-terminated list */
> -if (VIR_ALLOC_N(cpulist, n + 1) < 0)
> +if (VIR_ALLOC_N(cpulist, n) < 0)
>  goto cleanup;
>  
>  for (i = 0; i < n; i++) {
>  virJSONValuePtr child = virJSONValueArrayGet(data, i);
>  const char *tmp;
> +qemuMonitorCPUDefInfoPtr cpu;
> +
> +if (VIR_ALLOC(cpu) < 0)
> +goto cleanup;
> +
> +cpulist[i] = cpu;
>  
>  if (!(tmp = virJSONValueObjectGetString(child, "name"))) {
>  virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> @@ -4912,7 +4918,7 @@ int qemuMonitorJSONGetCPUDefinitions(qemuMonitorPtr mon,
>  goto cleanup;
>  }
>  
> -if (VIR_STRDUP(cpulist[i], tmp) < 0)
> +if (VIR_STRDUP(cpu->name, tmp) < 0)
>  goto cleanup;
>  }
>  
> @@ -4921,7 +4927,11 @@ int qemuMonitorJSONGetCPUDefinitions(qemuMonitorPtr 
> mon,
>  cpulist = NULL;
>  
>   cleanup:
> -virStringFreeList(cpulist);
> +if (ret < 0 && cpulist) {

I think "ret < 0" is superfluous... Coverity whines too

> +for (i = 0; i < n; i++)
> +qemuMonitorCPUDefInfoFree(cpulist[i]);
> +VIR_FREE(cpulist);
> +}
>  virJSONValueFree(cmd);
>  virJSONValueFree(reply);
>  return ret;

[...]

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 03/41] qemu: Use virDomainCapsCPUModels for cpuDefinitions

2016-08-12 Thread Jiri Denemark 
The list of supported CPU models in domain capabilities is stored in
virDomainCapsCPUModels. Let's use the same object for storing CPU models
in QEMU capabilities.

Signed-off-by: Jiri Denemark 
---
 src/qemu/qemu_capabilities.c | 162 +++
 src/qemu/qemu_capabilities.h |  10 +--
 src/qemu/qemu_command.c  |   8 ++-
 src/qemu/qemu_monitor.c  |  12 +++-
 src/qemu/qemu_monitor.h  |  10 ++-
 src/qemu/qemu_monitor_json.c |  24 +--
 src/qemu/qemu_monitor_json.h |   2 +-
 tests/qemumonitorjsontest.c  |   8 +--
 tests/qemuxml2argvtest.c |  18 ++---
 9 files changed, 164 insertions(+), 90 deletions(-)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 43e3ea7..1dcc970 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -373,8 +373,7 @@ struct _virQEMUCaps {
 
 virArch arch;
 
-size_t ncpuDefinitions;
-char **cpuDefinitions;
+virDomainCapsCPUModelsPtr cpuDefinitions;
 
 size_t nmachineTypes;
 struct virQEMUCapsMachineType *machineTypes;
@@ -612,7 +611,10 @@ virQEMUCapsParseX86Models(const char *output,
 {
 const char *p = output;
 const char *next;
-int ret = -1;
+virDomainCapsCPUModelsPtr cpus;
+
+if (!(cpus = virDomainCapsCPUModelsNew(0)))
+return -1;
 
 do {
 const char *t;
@@ -634,9 +636,6 @@ virQEMUCapsParseX86Models(const char *output,
 if (*p == '\0' || *p == '\n')
 continue;
 
-if (VIR_EXPAND_N(qemuCaps->cpuDefinitions, qemuCaps->ncpuDefinitions, 
1) < 0)
-goto cleanup;
-
 if (next)
 len = next - p - 1;
 else
@@ -647,14 +646,16 @@ virQEMUCapsParseX86Models(const char *output,
 len -= 2;
 }
 
-if (VIR_STRNDUP(qemuCaps->cpuDefinitions[qemuCaps->ncpuDefinitions - 
1], p, len) < 0)
-goto cleanup;
+if (virDomainCapsCPUModelsAdd(cpus, p, len) < 0)
+goto error;
 } while ((p = next));
 
-ret = 0;
+qemuCaps->cpuDefinitions = cpus;
+return 0;
 
- cleanup:
-return ret;
+ error:
+virObjectUnref(cpus);
+return -1;
 }
 
 /* ppc64 parser.
@@ -666,11 +667,13 @@ virQEMUCapsParsePPCModels(const char *output,
 {
 const char *p = output;
 const char *next;
-int ret = -1;
+virDomainCapsCPUModelsPtr cpus;
+
+if (!(cpus = virDomainCapsCPUModelsNew(0)))
+return -1;
 
 do {
 const char *t;
-size_t len;
 
 if ((next = strchr(p, '\n')))
 next++;
@@ -691,19 +694,16 @@ virQEMUCapsParsePPCModels(const char *output,
 if (*p == '\n')
 continue;
 
-if (VIR_EXPAND_N(qemuCaps->cpuDefinitions, qemuCaps->ncpuDefinitions, 
1) < 0)
-goto cleanup;
-
-len = t - p - 1;
-
-if (VIR_STRNDUP(qemuCaps->cpuDefinitions[qemuCaps->ncpuDefinitions - 
1], p, len) < 0)
-goto cleanup;
+if (virDomainCapsCPUModelsAdd(cpus, p, t - p - 1) < 0)
+goto error;
 } while ((p = next));
 
-ret = 0;
+qemuCaps->cpuDefinitions = cpus;
+return 0;
 
- cleanup:
-return ret;
+ error:
+virObjectUnref(cpus);
+return -1;
 }
 
 static int
@@ -2033,11 +2033,9 @@ virQEMUCapsPtr virQEMUCapsNewCopy(virQEMUCapsPtr 
qemuCaps)
 
 ret->arch = qemuCaps->arch;
 
-if (VIR_ALLOC_N(ret->cpuDefinitions, qemuCaps->ncpuDefinitions) < 0)
-goto error;
-ret->ncpuDefinitions = qemuCaps->ncpuDefinitions;
-for (i = 0; i < qemuCaps->ncpuDefinitions; i++) {
-if (VIR_STRDUP(ret->cpuDefinitions[i], qemuCaps->cpuDefinitions[i]) < 
0)
+if (qemuCaps->cpuDefinitions) {
+ret->cpuDefinitions = 
virDomainCapsCPUModelsCopy(qemuCaps->cpuDefinitions);
+if (!ret->cpuDefinitions)
 goto error;
 }
 
@@ -2076,9 +2074,7 @@ void virQEMUCapsDispose(void *obj)
 }
 VIR_FREE(qemuCaps->machineTypes);
 
-for (i = 0; i < qemuCaps->ncpuDefinitions; i++)
-VIR_FREE(qemuCaps->cpuDefinitions[i]);
-VIR_FREE(qemuCaps->cpuDefinitions);
+virObjectUnref(qemuCaps->cpuDefinitions);
 
 virBitmapFree(qemuCaps->flags);
 
@@ -2216,28 +2212,58 @@ const char *virQEMUCapsGetPackage(virQEMUCapsPtr 
qemuCaps)
 }
 
 
-int virQEMUCapsAddCPUDefinition(virQEMUCapsPtr qemuCaps,
-const char *name)
+int
+virQEMUCapsAddCPUDefinitions(virQEMUCapsPtr qemuCaps,
+ const char **name,
+ size_t count)
 {
-char *tmp;
+size_t i;
 
-if (VIR_STRDUP(tmp, name) < 0)
-return -1;
-if (VIR_EXPAND_N(qemuCaps->cpuDefinitions, qemuCaps->ncpuDefinitions, 1) < 
0) {
-VIR_FREE(tmp);
+if (!qemuCaps->cpuDefinitions &&
+!(qemuCaps->cpuDefinitions = virDomainCapsCPUModelsNew(count)))
 return -1;
+
+for (i = 0; i < count; i++) {
+if (virDomainCapsCPUModelsAdd(qemuCaps->cpuDefinitions, name[i], -1) < 
0)
+