[libvirt] [PATCH v2] libxl: support serial list

[Bob Liu]

Add support for multi serial devices, after this patch virsh can be used to
connect different serial devices of running domains. E.g.
vish # console  --devname serial

Note:
This depends on a xen/libxl bug fix to have libxl_console_get_tty(...) correctly
returning the tty path (as opposed to always returning the first one).
[0] https://lists.xen.org/archives/html/xen-devel/2016-08/msg00438.html

Signed-off-by: Bob Liu 
---
v2: Add #ifdef LIBXL_HAVE_BUILDINFO_SERIAL_LIST
---
 src/libxl/libxl_conf.c   | 24 +---
 src/libxl/libxl_domain.c | 23 +++
 src/libxl/libxl_driver.c | 17 +
 3 files changed, 53 insertions(+), 11 deletions(-)

diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index 146e08a..26c704d 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -431,14 +431,32 @@ libxlMakeDomBuildInfo(virDomainDefPtr def,
 }
 
 if (def->nserials) {
-if (def->nserials > 1) {
+if (def->nserials == 1) {
+if (libxlMakeChrdevStr(def->serials[0], _info->u.hvm.serial) 
<
+0)
+return -1;
+} else {
+#ifdef LIBXL_HAVE_BUILDINFO_SERIAL_LIST
+if (VIR_ALLOC_N(b_info->u.hvm.serial_list, def->nserials + 1) <
+0)
+return -1;
+
+for (i = 0; i < def->nserials; i++) {
+if (libxlMakeChrdevStr(def->serials[i],
+   _info->u.hvm.serial_list[i]) < 0)
+{
+libxl_string_list_dispose(_info->u.hvm.serial_list);
+return -1;
+}
+}
+b_info->u.hvm.serial_list[i] = NULL;
+#else
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
"%s",
_("Only one serial device is supported by 
libxl"));
 return -1;
+#endif
 }
-if (libxlMakeChrdevStr(def->serials[0], _info->u.hvm.serial) < 0)
-return -1;
 }
 
 if (def->nparallels) {
diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
index 0e26b91..5d8c9c1 100644
--- a/src/libxl/libxl_domain.c
+++ b/src/libxl/libxl_domain.c
@@ -989,6 +989,29 @@ libxlConsoleCallback(libxl_ctx *ctx, libxl_event *ev, void 
*for_callback)
 VIR_FREE(console);
 }
 }
+for (i = 0; i < vm->def->nserials; i++) {
+virDomainChrDefPtr chr = vm->def->serials[i];
+char *console = NULL;
+int ret;
+
+ignore_value(virAsprintf(>info.alias, "serial%zd", i));
+if (chr->source.type == VIR_DOMAIN_CHR_TYPE_PTY) {
+if (chr->source.data.file.path)
+continue;
+ret = libxl_console_get_tty(ctx, ev->domid,
+chr->target.port,
+LIBXL_CONSOLE_TYPE_SERIAL,
+);
+if (!ret) {
+VIR_FREE(chr->source.data.file.path);
+if (console && console[0] != '\0') {
+ignore_value(VIR_STRDUP(chr->source.data.file.path,
+console));
+}
+}
+VIR_FREE(console);
+}
+}
 virObjectUnlock(vm);
 libxl_event_free(ctx, ev);
 }
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index f153f69..a34eb02 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -4450,13 +4450,6 @@ libxlDomainOpenConsole(virDomainPtr dom,
 
 virCheckFlags(VIR_DOMAIN_CONSOLE_FORCE, -1);
 
-if (dev_name) {
-/* XXX support device aliases in future */
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
-   _("Named device aliases are not supported"));
-goto cleanup;
-}
-
 if (!(vm = libxlDomObjFromDomain(dom)))
 goto cleanup;
 
@@ -4472,8 +4465,16 @@ libxlDomainOpenConsole(virDomainPtr dom,
 }
 
 priv = vm->privateData;
+if (dev_name) {
+size_t i;
 
-if (vm->def->nconsoles) {
+for (i = 0; !chr && i < vm->def->nserials; i++) {
+if (STREQ(dev_name, vm->def->serials[i]->info.alias)) {
+chr = vm->def->serials[i];
+break;
+}
+}
+} else if (vm->def->nconsoles) {
 chr = vm->def->consoles[0];
 if (chr->targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL)
 chr = vm->def->serials[0];
-- 
2.6.5

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

Re: [libvirt] [PATCH v2 2/8] conf: add node_device_event handling

[Cole Robinson]

On 08/03/2016 06:39 PM, John Ferlan wrote:
> 
> 
> On 08/03/2016 06:30 PM, Cole Robinson wrote:
>> On 08/03/2016 05:57 PM, John Ferlan wrote:
>>>
>>>
>>> On 08/03/2016 05:37 PM, Cole Robinson wrote:
 On 08/03/2016 09:40 AM, John Ferlan wrote:
>
>
> On 07/28/2016 08:02 AM, Jovanka Gulicoska wrote:
>> Add node device event handling infrastructure to node_device_event.[ch]
>> ---
>>  src/Makefile.am  |   5 +
>>  src/conf/node_device_event.c | 234 
>> +++
>>  src/conf/node_device_event.h |  59 +++
>>  src/libvirt_private.syms |   5 +
>>  4 files changed, 303 insertions(+)
>>  create mode 100644 src/conf/node_device_event.c
>>  create mode 100644 src/conf/node_device_event.h
>>
>
> [...]
>
>> diff --git a/src/conf/node_device_event.c b/src/conf/node_device_event.c
>> new file mode 100644
>> index 000..61bc912
>
> [...]
>
>> +
>> +/**
>> + * virNodeDeviceEventLifecycleNew:
>> + * @name: name of the node device object the event describes
>> + * @type: type of lifecycle event
>> + * @detail: more details about @type
>> + *
>> + * Create a new node device lifecycle event.
>> + */
>> +virObjectEventPtr
>> +virNodeDeviceEventLifecycleNew(const char *name,
>> +   int type,
>> +   int detail)
>> +{
>> +virNodeDeviceEventLifecyclePtr event;
>> +
>> +if (virNodeDeviceEventsInitialize() < 0)
>> +return NULL;
>> +
>> +if (!(event = virObjectEventNew(virNodeDeviceEventLifecycleClass,
>> +
>> virNodeDeviceEventDispatchDefaultFunc,
>> +VIR_NODE_DEVICE_EVENT_ID_LIFECYCLE,
>> +0, name, NULL, name)))
> 
>
> This has caused a Coverity build failure since the prototype has:
>
> ATTRIBUTE_NONNULL(6)
>

 I think just dropping it is fine? The code was updated to handle uuid=NULL

 diff --git a/src/conf/object_event_private.h 
 b/src/conf/object_event_private.h
 index 92c25d4..27b461f 100644
 --- a/src/conf/object_event_private.h
 +++ b/src/conf/object_event_private.h
 @@ -106,6 +106,6 @@ virObjectEventNew(virClassPtr klass,
const unsigned char *uuid,
const char *key)
  ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(5)
 -ATTRIBUTE_NONNULL(6) ATTRIBUTE_NONNULL(7);
 +ATTRIBUTE_NONNULL(7);

  #endif

> It gets even worse in the function and needs to be resolved before the
> "next" release.
>

 I can't parse this sentence... are there additional issues?

>>> virObjectEventNew(...,const unsigned char *uuid,...)
>>> ...
>>> memcpy(event->meta.uuid, uuid, VIR_UUID_BUFLEN);
>>> ...
>>>
>>> e.g.
>>> memcpy(event->meta.uuid, NULL, VIR_UUID_BUFLEN);
>>>
>>
>> But the full code from virObjectEventNew handles that case by checking for
>> non-NULL uuid first:
>>
>> if (uuid)
>> memcpy(event->meta.uuid, uuid, VIR_UUID_BUFLEN);
>>
> 
> Hmm strange, my coverity environment AFU'd  Ahhh I see why.
> 
> OK, removing the ATTRIBUTE_NONNULL(6) works just fine
> 
> ACK for that
> 
> John
> 
> Long story short - recent update to f24 has for some reason caused
> compilation failures for "-Wnonnull-compare"'s within the coverity build
> environment, so I made some local changes to get past it while I figured
> out what the issue was. That was one of the many changes.
> 

Got it. I pushed that change:

commit e584615b81b5cabb252b1866171bde25b4f06d05
Author: Cole Robinson 
Date:   Wed Aug 3 18:45:50 2016 -0400

conf: events: Fix coverity warning

Since 2bfa75134 virObjectEventNew can be passed a NULL 'uuid' value,
so drop the ATTRIBUTE_NONNULL annotation

Thanks,
Cole

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

Re: [libvirt] [PATCH v2 2/8] conf: add node_device_event handling

[John Ferlan]

On 08/03/2016 06:30 PM, Cole Robinson wrote:
> On 08/03/2016 05:57 PM, John Ferlan wrote:
>>
>>
>> On 08/03/2016 05:37 PM, Cole Robinson wrote:
>>> On 08/03/2016 09:40 AM, John Ferlan wrote:


 On 07/28/2016 08:02 AM, Jovanka Gulicoska wrote:
> Add node device event handling infrastructure to node_device_event.[ch]
> ---
>  src/Makefile.am  |   5 +
>  src/conf/node_device_event.c | 234 
> +++
>  src/conf/node_device_event.h |  59 +++
>  src/libvirt_private.syms |   5 +
>  4 files changed, 303 insertions(+)
>  create mode 100644 src/conf/node_device_event.c
>  create mode 100644 src/conf/node_device_event.h
>

 [...]

> diff --git a/src/conf/node_device_event.c b/src/conf/node_device_event.c
> new file mode 100644
> index 000..61bc912

 [...]

> +
> +/**
> + * virNodeDeviceEventLifecycleNew:
> + * @name: name of the node device object the event describes
> + * @type: type of lifecycle event
> + * @detail: more details about @type
> + *
> + * Create a new node device lifecycle event.
> + */
> +virObjectEventPtr
> +virNodeDeviceEventLifecycleNew(const char *name,
> +   int type,
> +   int detail)
> +{
> +virNodeDeviceEventLifecyclePtr event;
> +
> +if (virNodeDeviceEventsInitialize() < 0)
> +return NULL;
> +
> +if (!(event = virObjectEventNew(virNodeDeviceEventLifecycleClass,
> +
> virNodeDeviceEventDispatchDefaultFunc,
> +VIR_NODE_DEVICE_EVENT_ID_LIFECYCLE,
> +0, name, NULL, name)))
 

 This has caused a Coverity build failure since the prototype has:

 ATTRIBUTE_NONNULL(6)

>>>
>>> I think just dropping it is fine? The code was updated to handle uuid=NULL
>>>
>>> diff --git a/src/conf/object_event_private.h 
>>> b/src/conf/object_event_private.h
>>> index 92c25d4..27b461f 100644
>>> --- a/src/conf/object_event_private.h
>>> +++ b/src/conf/object_event_private.h
>>> @@ -106,6 +106,6 @@ virObjectEventNew(virClassPtr klass,
>>>const unsigned char *uuid,
>>>const char *key)
>>>  ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(5)
>>> -ATTRIBUTE_NONNULL(6) ATTRIBUTE_NONNULL(7);
>>> +ATTRIBUTE_NONNULL(7);
>>>
>>>  #endif
>>>
 It gets even worse in the function and needs to be resolved before the
 "next" release.

>>>
>>> I can't parse this sentence... are there additional issues?
>>>
>> virObjectEventNew(...,const unsigned char *uuid,...)
>> ...
>> memcpy(event->meta.uuid, uuid, VIR_UUID_BUFLEN);
>> ...
>>
>> e.g.
>> memcpy(event->meta.uuid, NULL, VIR_UUID_BUFLEN);
>>
> 
> But the full code from virObjectEventNew handles that case by checking for
> non-NULL uuid first:
> 
> if (uuid)
> memcpy(event->meta.uuid, uuid, VIR_UUID_BUFLEN);
> 

Hmm strange, my coverity environment AFU'd  Ahhh I see why.

OK, removing the ATTRIBUTE_NONNULL(6) works just fine

ACK for that

John

Long story short - recent update to f24 has for some reason caused
compilation failures for "-Wnonnull-compare"'s within the coverity build
environment, so I made some local changes to get past it while I figured
out what the issue was. That was one of the many changes.

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

[libvirt] [PATCH v5 0/8] perf: add more perf events support

[John Ferlan]

v4: http://www.redhat.com/archives/libvir-list/2016-July/msg00607.html

Reworked/reworded the series slightly.  The end result is mostly the
same as the original, but with the suggested tweaks.


John Ferlan (3):
  virsh: Add a forward reference to perf command from domstats --perf
  virsh: Rework the perf event names into a table.
  util: Move virPerfNew and virPerfFree

Qiaowei Ren (5):
  perf: rename qemuDomainGetStatsPerfRdt()
  perf: Remove the switch from qemuDomainGetStatsPerf
  util: Add some comment details for virPerfEventType
  perf: Adjust the perf initialization
  perf: add more perf events support

 docs/formatdomain.html.in   |  24 +++
 docs/schemas/domaincommon.rng   |   4 +
 include/libvirt/libvirt-domain.h|  39 +
 src/libvirt-domain.c|   9 ++
 src/qemu/qemu_driver.c  |  23 ++-
 src/util/virperf.c  | 230 +---
 src/util/virperf.h  |  14 +-
 tests/genericxml2xmlindata/generic-perf.xml |   4 +
 tools/virsh.pod |  40 +++--
 9 files changed, 275 insertions(+), 112 deletions(-)

-- 
2.7.4

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

Re: [libvirt] [PATCH v2 2/8] conf: add node_device_event handling

[Cole Robinson]

On 08/03/2016 05:57 PM, John Ferlan wrote:
> 
> 
> On 08/03/2016 05:37 PM, Cole Robinson wrote:
>> On 08/03/2016 09:40 AM, John Ferlan wrote:
>>>
>>>
>>> On 07/28/2016 08:02 AM, Jovanka Gulicoska wrote:
 Add node device event handling infrastructure to node_device_event.[ch]
 ---
  src/Makefile.am  |   5 +
  src/conf/node_device_event.c | 234 
 +++
  src/conf/node_device_event.h |  59 +++
  src/libvirt_private.syms |   5 +
  4 files changed, 303 insertions(+)
  create mode 100644 src/conf/node_device_event.c
  create mode 100644 src/conf/node_device_event.h

>>>
>>> [...]
>>>
 diff --git a/src/conf/node_device_event.c b/src/conf/node_device_event.c
 new file mode 100644
 index 000..61bc912
>>>
>>> [...]
>>>
 +
 +/**
 + * virNodeDeviceEventLifecycleNew:
 + * @name: name of the node device object the event describes
 + * @type: type of lifecycle event
 + * @detail: more details about @type
 + *
 + * Create a new node device lifecycle event.
 + */
 +virObjectEventPtr
 +virNodeDeviceEventLifecycleNew(const char *name,
 +   int type,
 +   int detail)
 +{
 +virNodeDeviceEventLifecyclePtr event;
 +
 +if (virNodeDeviceEventsInitialize() < 0)
 +return NULL;
 +
 +if (!(event = virObjectEventNew(virNodeDeviceEventLifecycleClass,
 +virNodeDeviceEventDispatchDefaultFunc,
 +VIR_NODE_DEVICE_EVENT_ID_LIFECYCLE,
 +0, name, NULL, name)))
>>> 
>>>
>>> This has caused a Coverity build failure since the prototype has:
>>>
>>> ATTRIBUTE_NONNULL(6)
>>>
>>
>> I think just dropping it is fine? The code was updated to handle uuid=NULL
>>
>> diff --git a/src/conf/object_event_private.h 
>> b/src/conf/object_event_private.h
>> index 92c25d4..27b461f 100644
>> --- a/src/conf/object_event_private.h
>> +++ b/src/conf/object_event_private.h
>> @@ -106,6 +106,6 @@ virObjectEventNew(virClassPtr klass,
>>const unsigned char *uuid,
>>const char *key)
>>  ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(5)
>> -ATTRIBUTE_NONNULL(6) ATTRIBUTE_NONNULL(7);
>> +ATTRIBUTE_NONNULL(7);
>>
>>  #endif
>>
>>> It gets even worse in the function and needs to be resolved before the
>>> "next" release.
>>>
>>
>> I can't parse this sentence... are there additional issues?
>>
> virObjectEventNew(...,const unsigned char *uuid,...)
> ...
> memcpy(event->meta.uuid, uuid, VIR_UUID_BUFLEN);
> ...
> 
> e.g.
> memcpy(event->meta.uuid, NULL, VIR_UUID_BUFLEN);
> 

But the full code from virObjectEventNew handles that case by checking for
non-NULL uuid first:

if (uuid)
memcpy(event->meta.uuid, uuid, VIR_UUID_BUFLEN);

- Cole

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

[libvirt] [PATCH v5 2/8] perf: Remove the switch from qemuDomainGetStatsPerf

[John Ferlan]

From: Qiaowei Ren 

Remove the unnecessary switch since all VIR_PERF_EVENT* values are fetched

Signed-off-by: John Ferlan 
---
 src/qemu/qemu_driver.c | 12 +++-
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index c45207e..b41eaa3 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -19172,15 +19172,9 @@ qemuDomainGetStatsPerf(virQEMUDriverPtr driver 
ATTRIBUTE_UNUSED,
 if (!virPerfEventIsEnabled(priv->perf, i))
  continue;
 
-switch (i) {
-case VIR_PERF_EVENT_CMT:
-case VIR_PERF_EVENT_MBMT:
-case VIR_PERF_EVENT_MBML:
-if (qemuDomainGetStatsPerfOneEvent(priv->perf, i, record,
-   maxparams) < 0)
-goto cleanup;
-break;
-}
+if (qemuDomainGetStatsPerfOneEvent(priv->perf, i,
+   record, maxparams) < 0)
+goto cleanup;
 }
 
 ret = 0;
-- 
2.7.4

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

[libvirt] [PATCH v5 6/8] util: Move virPerfNew and virPerfFree

[John Ferlan]

Move them to the bottom under the #ifdef code.

Signed-off-by: John Ferlan 
---
 src/util/virperf.c | 68 +++---
 1 file changed, 34 insertions(+), 34 deletions(-)

diff --git a/src/util/virperf.c b/src/util/virperf.c
index 4661ba3..627b2ec 100644
--- a/src/util/virperf.c
+++ b/src/util/virperf.c
@@ -57,40 +57,6 @@ struct virPerf {
 struct virPerfEvent events[VIR_PERF_EVENT_LAST];
 };
 
-virPerfPtr
-virPerfNew(void)
-{
-size_t i;
-virPerfPtr perf;
-
-if (VIR_ALLOC(perf) < 0)
-return NULL;
-
-for (i = 0; i < VIR_PERF_EVENT_LAST; i++) {
-perf->events[i].type = i;
-perf->events[i].fd = -1;
-perf->events[i].enabled = false;
-}
-
-return perf;
-}
-
-void
-virPerfFree(virPerfPtr perf)
-{
-size_t i;
-
-if (perf == NULL)
-return;
-
-for (i = 0; i < VIR_PERF_EVENT_LAST; i++) {
-if (perf->events[i].enabled)
-virPerfEventDisable(perf, i);
-}
-
-VIR_FREE(perf);
-}
-
 #if defined(__linux__) && defined(HAVE_SYS_SYSCALL_H)
 
 # include 
@@ -303,3 +269,37 @@ virPerfReadEvent(virPerfPtr perf ATTRIBUTE_UNUSED,
 }
 
 #endif
+
+virPerfPtr
+virPerfNew(void)
+{
+size_t i;
+virPerfPtr perf;
+
+if (VIR_ALLOC(perf) < 0)
+return NULL;
+
+for (i = 0; i < VIR_PERF_EVENT_LAST; i++) {
+perf->events[i].type = i;
+perf->events[i].fd = -1;
+perf->events[i].enabled = false;
+}
+
+return perf;
+}
+
+void
+virPerfFree(virPerfPtr perf)
+{
+size_t i;
+
+if (perf == NULL)
+return;
+
+for (i = 0; i < VIR_PERF_EVENT_LAST; i++) {
+if (perf->events[i].enabled)
+virPerfEventDisable(perf, i);
+}
+
+VIR_FREE(perf);
+}
-- 
2.7.4

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

[libvirt] [PATCH v5 1/8] perf: rename qemuDomainGetStatsPerfRdt()

[John Ferlan]

From: Qiaowei Ren 

This patch rename qemuDomainGetStatsPerfRdt() to
qemuDomainGetStatsPerfOneEvent()

Signed-off-by: Qiaowei Ren 
Signed-off-by: John Ferlan 
---
 src/qemu/qemu_driver.c | 11 ++-
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index dbdacba..c45207e 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -19133,10 +19133,10 @@ qemuDomainGetStatsBlock(virQEMUDriverPtr driver,
 #undef QEMU_ADD_COUNT_PARAM
 
 static int
-qemuDomainGetStatsPerfRdt(virPerfPtr perf,
-  virPerfEventType type,
-  virDomainStatsRecordPtr record,
-  int *maxparams)
+qemuDomainGetStatsPerfOneEvent(virPerfPtr perf,
+   virPerfEventType type,
+   virDomainStatsRecordPtr record,
+   int *maxparams)
 {
 char param_name[VIR_TYPED_PARAM_FIELD_LENGTH];
 uint64_t value = 0;
@@ -19176,7 +19176,8 @@ qemuDomainGetStatsPerf(virQEMUDriverPtr driver 
ATTRIBUTE_UNUSED,
 case VIR_PERF_EVENT_CMT:
 case VIR_PERF_EVENT_MBMT:
 case VIR_PERF_EVENT_MBML:
-if (qemuDomainGetStatsPerfRdt(priv->perf, i, record, maxparams) < 
0)
+if (qemuDomainGetStatsPerfOneEvent(priv->perf, i, record,
+   maxparams) < 0)
 goto cleanup;
 break;
 }
-- 
2.7.4

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

[libvirt] [PATCH v5 8/8] perf: add more perf events support

[John Ferlan]

From: Qiaowei Ren 

With current perf framework, this patch adds support and documentation
for more perf events, including cache misses, cache references, cpu cycles,
and instructions.

Signed-off-by: Qiaowei Ren 
Signed-off-by: John Ferlan 
---
 docs/formatdomain.html.in   | 24 ++
 docs/schemas/domaincommon.rng   |  4 +++
 include/libvirt/libvirt-domain.h| 39 +
 src/libvirt-domain.c|  9 +++
 src/qemu/qemu_driver.c  |  4 +++
 src/util/virperf.c  | 16 +++-
 src/util/virperf.h  |  5 
 tests/genericxml2xmlindata/generic-perf.xml |  4 +++
 tools/virsh.pod | 13 ++
 9 files changed, 117 insertions(+), 1 deletion(-)

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index fa88839..f7c5d3b 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -1867,6 +1867,10 @@
 event name='cmt' enabled='yes'/
 event name='mbmt' enabled='no'/
 event name='mbml' enabled='yes'/
+event name='cpu_cycles' enabled='no'/
+event name='instructions' enabled='yes'/
+event name='cache_references' enabled='no'/
+event name='cache_misses' enabled='no'/
   /perf
   ...
 
@@ -1892,6 +1896,26 @@
   bandwidth of memory traffic for a memory controller
   perf.mbml
 
+
+  cpu_cycles
+  the number of cpu cycles one instruction needs
+  perf.cpu_cycles
+
+
+  instructions
+  the count of instructions by applications running on the 
platform
+  perf.instructions
+
+
+  cache_references
+  the count of cache hits by applications running on the platform
+  perf.cache_references
+
+
+  cache_misses
+  the count of cache misses by applications running on the 
platform
+  perf.cache_misses
+
   
 
 Devices
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index cb9f134..715ccc5 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -414,6 +414,10 @@
   cmt
   mbmt
   mbml
+  cpu_cycles
+  instructions
+  cache_references
+  cache_misses
 
   
   
diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
index 42a2bea..6e0e7fb 100644
--- a/include/libvirt/libvirt-domain.h
+++ b/include/libvirt/libvirt-domain.h
@@ -1972,6 +1972,45 @@ void 
virDomainStatsRecordListFree(virDomainStatsRecordPtr *stats);
  */
 # define VIR_PERF_PARAM_MBML "mbml"
 
+/**
+ * VIR_PERF_PARAM_CACHE_MISSES:
+ *
+ * Macro for typed parameter name that represents cache_misses perf
+ * event which can be used to measure the count of cache misses by
+ * applications running on the platform. It corresponds to the
+ * "perf.cache_misses" field in the *Stats APIs.
+ */
+# define VIR_PERF_PARAM_CACHE_MISSES "cache_misses"
+
+/**
+ * VIR_PERF_PARAM_CACHE_REFERENCES:
+ *
+ * Macro for typed parameter name that represents cache_references
+ * perf event which can be used to measure the count of cache hits
+ * by applications running on the platform. It corresponds to the
+ * "perf.cache_references" field in the *Stats APIs.
+ */
+# define VIR_PERF_PARAM_CACHE_REFERENCES "cache_references"
+
+/**
+ * VIR_PERF_PARAM_INSTRUCTIONS:
+ *
+ * Macro for typed parameter name that represents instructions perf
+ * event which can be used to measure the count of instructions
+ * by applications running on the platform. It corresponds to the
+ * "perf.instructions" field in the *Stats APIs.
+ */
+# define VIR_PERF_PARAM_INSTRUCTIONS "instructions"
+
+/**
+ * VIR_PERF_PARAM_CPU_CYCLES:
+ *
+ * Macro for typed parameter name that represents cpu_cycles perf event
+ * which can be used to measure how many cpu cycles one instruction needs.
+ * It corresponds to the "perf.cpu_cycles" field in the *Stats APIs.
+ */
+# define VIR_PERF_PARAM_CPU_CYCLES "cpu_cycles"
+
 int virDomainGetPerfEvents(virDomainPtr dom,
virTypedParameterPtr *params,
int *nparams,
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
index 4f08349..46f0318 100644
--- a/src/libvirt-domain.c
+++ b/src/libvirt-domain.c
@@ -11457,6 +11457,15 @@ virConnectGetDomainCapabilities(virConnectPtr conn,
  * "perf.mbml" - the amount of data (bytes/s) sent through the memory 
controller
  *   on the socket as unsigned long long. It is produced by mbml
  *   perf event.
+ * "perf.cache_misses" - the count of cache misses as unsigned long long.
+ *   It is produced by cache_misses perf event.
+ * "perf.cache_references" - the count of cache hits as unsigned long long.
+ *   It is produced by 

[libvirt] [PATCH v5 4/8] virsh: Add a forward reference to perf command from domstats --perf

[John Ferlan]

Keep the details in one place...

Signed-off-by: John Ferlan 
---
 tools/virsh.pod | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/virsh.pod b/tools/virsh.pod
index 97d16c5..00b5d51 100644
--- a/tools/virsh.pod
+++ b/tools/virsh.pod
@@ -935,6 +935,8 @@ I<--perf> returns the statistics of all enabled perf events:
 "perf.mbmt" - total system bandwidth from one level of cache
 "perf.mbml" - bandwidth of memory traffic for a memory controller
 
+See the B command for more details about each event.
+
 I<--block> returns information about disks associated with each
 domain.  Using the I<--backing> flag extends this information to
 cover all resources in the backing chain, rather than the default
-- 
2.7.4

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

[libvirt] [PATCH v5 3/8] util: Add some comment details for virPerfEventType

[John Ferlan]

From: Qiaowei Ren 

Add to some details for the existing enum

Signed-off-by: John Ferlan 
---
 src/util/virperf.h | 9 ++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/src/util/virperf.h b/src/util/virperf.h
index 7163410..bdafe03 100644
--- a/src/util/virperf.h
+++ b/src/util/virperf.h
@@ -24,10 +24,13 @@
 
 # include "virutil.h"
 
+/* Some Intel processor families introduced some RDT (Resource Director
+ * Technology) features to monitor or control shared resource based on
+ * the perf framework in the linux kernel. */
 typedef enum {
-VIR_PERF_EVENT_CMT,
-VIR_PERF_EVENT_MBMT,
-VIR_PERF_EVENT_MBML,
+VIR_PERF_EVENT_CMT,/* Cache Monitoring Technology */
+VIR_PERF_EVENT_MBMT,   /* Memory Bandwidth Monitoring Total */
+VIR_PERF_EVENT_MBML,   /* Memory Bandwidth Monitor Limit for controller */
 
 VIR_PERF_EVENT_LAST
 } virPerfEventType;
-- 
2.7.4

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

[libvirt] [PATCH v5 7/8] perf: Adjust the perf initialization

[John Ferlan]

From: Qiaowei Ren 

Introduce a static attr table and refactor virPerfEventEnable() for
general purpose usage.

This patch creates a static table/matrix that converts the VIR_PERF_EVENT_*
events into their respective "attr.type" and "attr.config" so that
virPerfEventEnable doesn't have the switch the calling function passes
by value the 'type'.

Signed-off-by: Qiaowei Ren 
Signed-off-by: John Ferlan 
---
 src/util/virperf.c | 164 +
 1 file changed, 103 insertions(+), 61 deletions(-)

diff --git a/src/util/virperf.c b/src/util/virperf.c
index 627b2ec..018000a 100644
--- a/src/util/virperf.c
+++ b/src/util/virperf.c
@@ -61,13 +61,24 @@ struct virPerf {
 
 # include 
 
-static virPerfEventPtr
-virPerfGetEvent(virPerfPtr perf,
-virPerfEventType type)
-{
-if (!perf)
-return NULL;
+struct virPerfEventAttr {
+int type;
+unsigned int attrType;
+unsigned long long attrConfig;
+};
+
+static struct virPerfEventAttr attrs[] = {
+{.type = VIR_PERF_EVENT_CMT, .attrType = 0, .attrConfig = 1},
+{.type = VIR_PERF_EVENT_MBMT, .attrType = 0, .attrConfig = 2},
+{.type = VIR_PERF_EVENT_MBML, .attrType = 0, .attrConfig = 3},
+};
+typedef struct virPerfEventAttr *virPerfEventAttrPtr;
+
 
+static virPerfEventAttrPtr
+virPerfGetEventAttr(virPerfEventType type)
+{
+size_t i;
 if (type >= VIR_PERF_EVENT_LAST) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
_("Event '%d' is not supported"),
@@ -75,67 +86,113 @@ virPerfGetEvent(virPerfPtr perf,
 return NULL;
 }
 
-return perf->events + type;
+for (i = 0; i < VIR_PERF_EVENT_LAST; i++) {
+if (i == type)
+return attrs + i;
+}
+
+return NULL;
 }
 
+
 static int
-virPerfRdtEnable(virPerfEventPtr event,
- pid_t pid)
+virPerfRdtAttrInit(void)
 {
-struct perf_event_attr rdt_attr;
 char *buf = NULL;
 char *tmp = NULL;
-unsigned int event_type, scale;
+unsigned int attr_type = 0;
 
-if (virFileReadAll("/sys/devices/intel_cqm/type",
-   10, ) < 0)
+if (virFileReadAll("/sys/devices/intel_cqm/type", 10, ) < 0)
 goto error;
 
 if ((tmp = strchr(buf, '\n')))
 *tmp = '\0';
 
-if (virStrToLong_ui(buf, NULL, 10, _type) < 0) {
+if (virStrToLong_ui(buf, NULL, 10, _type) < 0) {
 virReportSystemError(errno, "%s",
  _("failed to get rdt event type"));
 goto error;
 }
 VIR_FREE(buf);
 
-memset(_attr, 0, sizeof(rdt_attr));
-rdt_attr.size = sizeof(rdt_attr);
-rdt_attr.type = event_type;
-rdt_attr.inherit = 1;
-rdt_attr.disabled = 1;
-rdt_attr.enable_on_exec = 0;
+attrs[VIR_PERF_EVENT_CMT].attrType = attr_type;
+attrs[VIR_PERF_EVENT_MBMT].attrType = attr_type;
+attrs[VIR_PERF_EVENT_MBML].attrType = attr_type;
+
+return 0;
+
+ error:
+VIR_FREE(buf);
+return -1;
+}
+
+
+static virPerfEventPtr
+virPerfGetEvent(virPerfPtr perf,
+virPerfEventType type)
+{
+if (!perf)
+return NULL;
+
+if (type >= VIR_PERF_EVENT_LAST) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("Event '%d' is not supported"),
+   type);
+return NULL;
+}
+
+return perf->events + type;
+}
+
+int
+virPerfEventEnable(virPerfPtr perf,
+   virPerfEventType type,
+   pid_t pid)
+{
+char *buf = NULL;
+struct perf_event_attr attr;
+virPerfEventPtr event = virPerfGetEvent(perf, type);
+virPerfEventAttrPtr event_attr = virPerfGetEventAttr(type);
+
+if (!event || !event_attr)
+return -1;
 
-switch (event->type) {
-case VIR_PERF_EVENT_CMT:
+if (event_attr->attrType == 0 && (type == VIR_PERF_EVENT_CMT ||
+   type == VIR_PERF_EVENT_MBMT ||
+   type == VIR_PERF_EVENT_MBML)) {
+virReportSystemError(errno,
+ _("Unable to open perf event for %s"),
+ virPerfEventTypeToString(event->type));
+return -1;
+}
+
+if (type == VIR_PERF_EVENT_CMT) {
 if (virFileReadAll("/sys/devices/intel_cqm/events/llc_occupancy.scale",
10, ) < 0)
 goto error;
 
-if (virStrToLong_ui(buf, NULL, 10, ) < 0) {
+if (virStrToLong_i(buf, NULL, 10, >efields.cmt.scale) < 0) {
 virReportSystemError(errno, "%s",
  _("failed to get cmt scaling factor"));
 goto error;
 }
-event->efields.cmt.scale = scale;
-
-rdt_attr.config = 1;
-break;
-case VIR_PERF_EVENT_MBMT:
-rdt_attr.config = 2;
-break;
-case VIR_PERF_EVENT_MBML:
-rdt_attr.config = 3;
-break;
+
+

[libvirt] [PATCH v5 5/8] virsh: Rework the perf event names into a table.

[John Ferlan]

Should be easier to read

Signed-off-by: John Ferlan 
---
 tools/virsh.pod | 25 +++--
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/tools/virsh.pod b/tools/virsh.pod
index 00b5d51..7ce498b 100644
--- a/tools/virsh.pod
+++ b/tools/virsh.pod
@@ -2225,16 +2225,21 @@ from different sources can be supported by perf.
 Currently only QEMU/KVM supports this command. The I<--enable> and I<--disable>
 option combined with B can be used to enabled or disable specific
 performance event. B is a string list of one or more events
-separated by commas. Valid event names are "cmt", "mbmt", "mbml".
-CMT is a PQos (Platform Qos) feature to monitor the usage of cache by
-applications running on the platform.
-MBM (Memory Bandwidth Monitoring) provides a way to monitor the Total
-system memory bandwidth between one level of cache and another (mbmt)
-and the amount of data (bytes/s) sent through the memory controller on
-the socket (mbml).
-
-The statistics can be retrieved using the B command using the
-I<--perf> flag.
+separated by commas. Valid event names are as follows:
+
+B
+  cmt  - A PQos (Platform Qos) feature to monitor the
+ usage of cache by applications running on the
+ platform.
+  mbmt - Provides a way to monitor the total system
+ memory bandwidth between one level of cache
+ and another.
+  mbml - Provides a way to limit the amount of data
+ (bytes/s) send through the memory controller
+ on the socket.
+
+B: The statistics can be retrieved using the B command using
+the I<--perf> flag.
 
 If I<--live> is specified, affect a running guest.
 If I<--config> is specified, affect the next boot of a persistent guest.
-- 
2.7.4

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

Re: [libvirt] [PATCH v4 4/4] perf: add description for new events in virsh.pod

[John Ferlan]

On 07/16/2016 04:15 AM, Qiaowei Ren wrote:
> This patch add 'domstats --perf' description for new events in
> virsh.pod.
> 
> Signed-off-by: Qiaowei Ren 
> ---
>  tools/virsh.pod | 4 
>  1 file changed, 4 insertions(+)
> 

This would need to be merged with the previous patch since that's where
the fields are introduced.


> diff --git a/tools/virsh.pod b/tools/virsh.pod
> index d7cd10e..da7f24f 100644
> --- a/tools/virsh.pod
> +++ b/tools/virsh.pod
> @@ -909,6 +909,10 @@ I<--perf> returns the statistics of all enabled perf 
> events:
>  "perf.cmt" - the cache usage in Byte currently used
>  "perf.mbmt" - total system bandwidth from one level of cache
>  "perf.mbml" - bandwidth of memory traffic for a memory controller
> +"perf.cpu_cycles" - the number of cpu cycles one instruction needs
> +"perf.instructions" - the count of instructions
> +"perf.cache_references" - the count of cache hits
> +"perf.cache_misses" - the count of caches misses
>  

As previously stated - it'd be nice to add a reference to the "perf"
section from the domstats --perf section, e.g.:

See the B command for more details about each event.


>  I<--block> returns information about disks associated with each
>  domain.  Using the I<--backing> flag extends this information to
> 


The update to the perf section is missing...  You'll see my adjustments
shortly - just cleaning up the patches a bit...

John

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

Re: [libvirt] [PATCH v4 3/4] perf: add more perf events support

[John Ferlan]

On 07/16/2016 04:15 AM, Qiaowei Ren wrote:
> With current perf framework, this patch adds support to more perf
> events, including cache misses, cache references, cpu cycles,
> instrctions, etc..
> 
> Signed-off-by: Qiaowei Ren 
> ---
>  docs/formatdomain.html.in   | 24 ++
>  docs/schemas/domaincommon.rng   |  4 +++
>  include/libvirt/libvirt-domain.h| 39 
> +
>  src/libvirt-domain.c|  8 ++
>  src/qemu/qemu_driver.c  |  4 +++
>  src/util/virperf.c  | 16 +++-
>  src/util/virperf.h  |  5 
>  tests/genericxml2xmlindata/generic-perf.xml |  4 +++
>  8 files changed, 103 insertions(+), 1 deletion(-)
> 
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 59a8bb9..53a0809 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -1842,6 +1842,10 @@
>  event name='cmt' enabled='yes'/
>  event name='mbmt' enabled='no'/
>  event name='mbml' enabled='yes'/
> +event name='cpu_cycles' enabled='no'/
> +event name='instructions' enabled='yes'/
> +event name='cache_references' enabled='no'/
> +event name='cache_misses' enabled='no'/
>/perf
>...
>  
> @@ -1867,6 +1871,26 @@
>bandwidth of memory traffic for a memory controller
>perf.mbml
>  
> +
> +  cpu_cycles
> +  the number of cpu cycles one instruction needs
> +  perf.cpu_cycles
> +
> +
> +  instructions
> +  the count of instructions by applications running on the 
> platform
> +  perf.instructions
> +
> +
> +  cache_references
> +  the count of cache hits by applications running on the 
> platform
> +  perf.cache_references
> +
> +
> +  cache_misses
> +  the count of cache misses by applications running on the 
> platform
> +  perf.cache_misses
> +
>
>  
>  Devices
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 348dbfe..be8ebb5 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -414,6 +414,10 @@
>cmt
>mbmt
>mbml
> +  cpu_cycles
> +  instructions
> +  cache_references
> +  cache_misses
>  
>
>
> diff --git a/include/libvirt/libvirt-domain.h 
> b/include/libvirt/libvirt-domain.h
> index 7ea93aa..0002d99 100644
> --- a/include/libvirt/libvirt-domain.h
> +++ b/include/libvirt/libvirt-domain.h
> @@ -1947,6 +1947,45 @@ void 
> virDomainStatsRecordListFree(virDomainStatsRecordPtr *stats);
>   */
>  # define VIR_PERF_PARAM_MBML "mbml"
>  
> +/**
> + * VIR_PERF_PARAM_CACHE_MISSES:
> + *
> + * Macro for typed parameter name that represents cache_misses perf
> + * event which can be used to measure the count of cache misses by
> + * applications running on the platform. It corresponds to the
> + * "perf.cache_misses" field in the *Stats APIs.
> + */
> +# define VIR_PERF_PARAM_CACHE_MISSES "cache_misses"
> +
> +/**
> + * VIR_PERF_PARAM_CACHE_REFERENCES:
> + *
> + * Macro for typed parameter name that represents cache_references
> + * perf event which can be used to measure the count of cache hits
> + * by applications running on the platform. It corresponds to the
> + * "perf.cache_references" field in the *Stats APIs.
> + */
> +# define VIR_PERF_PARAM_CACHE_REFERENCES "cache_references"
> +
> +/**
> + * VIR_PERF_PARAM_INSTRUCTIONS:
> + *
> + * Macro for typed parameter name that represents instructions perf
> + * event which can be used to measure the count of instructions
> + * by applications running on the platform. It corresponds to the
> + * "perf.instructions" field in the *Stats APIs.
> + */
> +# define VIR_PERF_PARAM_INSTRUCTIONS "instructions"
> +
> +/**
> + * VIR_PERF_PARAM_CPU_CYCLES:
> + *
> + * Macro for typed parameter name that represents cpu_cycles perf event
> + * which can be used to measure how many cpu cycles one instruction needs.
> + * It corresponds to the "perf.cpu_cycles" field in the *Stats APIs.
> + */
> +# define VIR_PERF_PARAM_CPU_CYCLES "cpu_cycles"
> +
>  int virDomainGetPerfEvents(virDomainPtr dom,
> virTypedParameterPtr *params,
> int *nparams,
> diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
> index 4e71a94..b383cbb 100644
> --- a/src/libvirt-domain.c
> +++ b/src/libvirt-domain.c
> @@ -11452,6 +11452,14 @@ virConnectGetDomainCapabilities(virConnectPtr conn,
>   * "perf.mbml" - the amount of data (bytes/s) sent through the memory 
> controller
>   *   on the socket as unsigned long long. It is produced by mbml
>   *   perf event.
> + * "perf.cache_misses" - the count of cache misses as unsigned long long.
> + * It is produced by cache_misses perf event.
> 

Re: [libvirt] [PATCH v4 2/4] perf: introduce a static attr table and refactor virPerfEventEnable() for general purpose

[John Ferlan]

Need to keep those commit messages a lot shorter...

On 07/16/2016 04:15 AM, Qiaowei Ren wrote:
> This patch creates some sort of static table/matrix that would be able to
> convert the VIR_PERF_EVENT_* into their respective "attr.type" and
> "attr.config", so that virPerfEventEnable() doesn't have the switch the
> calling function passes by value the 'type'. This change is for general
> purpose in future.
> 
> Signed-off-by: Qiaowei Ren 
> ---
>  src/util/virperf.c | 160 
> ++---
>  src/util/virperf.h |   6 ++
>  2 files changed, 97 insertions(+), 69 deletions(-)
> 

Another patch which could use some splitting...  Rather than go back and
forth more on this, I'll generate a sequence of patches based on your
code with some tweaks - most of which I'll suggest throughout here.

If you git am those patches and git diff them to your current patches
you can see the differences... But I'd also expect you could test and
validate my adjustments work...


> diff --git a/src/util/virperf.c b/src/util/virperf.c
> index 4661ba3..01c7c70 100644
> --- a/src/util/virperf.c
> +++ b/src/util/virperf.c
> @@ -57,6 +57,8 @@ struct virPerf {
>  struct virPerfEvent events[VIR_PERF_EVENT_LAST];
>  };
>  
> +static void virPerfRdtAttrInit(void);
> +

Avoid forward decls... I think what would be best to do is move
virPerfNew and virPerfFree to the bottom of the function after the
#endif for "if defined(__linux__) && defined(HAVE_SYS_SYSCALL_H)"

That would be it's own separate patch...  Then we can start modifying
the code to add the new call...

>  virPerfPtr
>  virPerfNew(void)
>  {

In my changes virPerfNew is at the bottom now so we won't need the
forward decl.

> @@ -72,6 +74,8 @@ virPerfNew(void)
>  perf->events[i].enabled = false;
>  }
>  
> +virPerfRdtAttrInit();
> +

This should return a value, as follows:

if (virPerfRdtAttrInit() < 0)
virResetLastError();

For the #else code, we just return 0 - so it's a no-op. When the
virPerfEventEnable is called an error is returned.

The "real" code inside the #if getting a failure would need to be made
"as failure proof" as the existing code which would have returned a
failure from virPerfEventEnable and for the most part ignore it.  Once
we call virPerfEventEnable it'll find the attrType == 0 for the RDT
events and cause a failure.

Alternatively, we could fail virPerfNew if AttrInit fails, but that's a
bit different than the existing code.

>  return perf;
>  }
>  
> @@ -95,12 +99,21 @@ virPerfFree(virPerfPtr perf)
>  
>  # include 
>  
> -static virPerfEventPtr
> -virPerfGetEvent(virPerfPtr perf,
> -virPerfEventType type)
> +static struct virPerfEventAttr {
> +int type;
> +unsigned int attrType;
> +unsigned long long attrConfig;
> +} attrs[] = {
> +{.type = VIR_PERF_EVENT_CMT, .attrType = 0, .attrConfig = 1},
> +{.type = VIR_PERF_EVENT_MBMT, .attrType = 0, .attrConfig = 2},
> +{.type = VIR_PERF_EVENT_MBML, .attrType = 0, .attrConfig = 3},
> +};
> +typedef struct virPerfEventAttr *virPerfEventAttrPtr;
> +
> +static virPerfEventAttrPtr
> +virPerfGetEventAttr(virPerfEventType type)
>  {
> -if (!perf)
> -return NULL;
> +size_t i;
>  
>  if (type >= VIR_PERF_EVENT_LAST) {
>  virReportError(VIR_ERR_INTERNAL_ERROR,
> @@ -109,17 +122,20 @@ virPerfGetEvent(virPerfPtr perf,
>  return NULL;
>  }
>  
> -return perf->events + type;
> +for (i = 0; i < VIR_PERF_EVENT_LAST; i++) {
> +if (i == type)
> +return attrs + i;
> +}
> +
> +return NULL;
>  }
>  
> -static int
> -virPerfRdtEnable(virPerfEventPtr event,
> - pid_t pid)
> +static void virPerfRdtAttrInit(void)

static void
virPerfRdtAttrInit(void)

is the normal way of doing this.

However, this is not a void function... If there's any errors here, then
we're not failing and returning any error...


>  {
> -struct perf_event_attr rdt_attr;
>  char *buf = NULL;
>  char *tmp = NULL;
> -unsigned int event_type, scale;
> +size_t i;
> +unsigned int attr_type = 0;
>  
>  if (virFileReadAll("/sys/devices/intel_cqm/type",
> 10, ) < 0)

This will go to error with some error message set, but then returns a
void status.

> @@ -128,48 +144,74 @@ virPerfRdtEnable(virPerfEventPtr event,
>  if ((tmp = strchr(buf, '\n')))
>  *tmp = '\0';
>  
> -if (virStrToLong_ui(buf, NULL, 10, _type) < 0) {
> +if (virStrToLong_ui(buf, NULL, 10, _type) < 0) {
>  virReportSystemError(errno, "%s",
>   _("failed to get rdt event type"));
>  goto error;

This goes to error with some error message set, but then returns a void
status.

>  }
>  VIR_FREE(buf);

Currently unnecessary since everything falls through into the error:
label...

>  
> -memset(_attr, 0, sizeof(rdt_attr));
> -rdt_attr.size = 

Re: [libvirt] [PATCH v4 1/4] perf: rename qemuDomainGetStatsPerfRdt()

[John Ferlan]

On 07/16/2016 04:15 AM, Qiaowei Ren wrote:
> This patch rename qemuDomainGetStatsPerfRdt() to
> qemuDomainGetStatsPerfOneEvent() and update qemuDomainGetStatsPerf()
> based on this change for multiple/general purpose.
> 
> Signed-off-by: Qiaowei Ren 
> ---
>  src/qemu/qemu_driver.c | 19 +++
>  1 file changed, 7 insertions(+), 12 deletions(-)
> 

This really should be two patches - one to rename the function and one
to remove the switch.

John
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index cda85f6..1fdb7b8 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -18938,10 +18938,10 @@ qemuDomainGetStatsBlock(virQEMUDriverPtr driver,
>  #undef QEMU_ADD_COUNT_PARAM
>  
>  static int
> -qemuDomainGetStatsPerfRdt(virPerfPtr perf,
> -  virPerfEventType type,
> -  virDomainStatsRecordPtr record,
> -  int *maxparams)
> +qemuDomainGetStatsPerfOneEvent(virPerfPtr perf,
> +   virPerfEventType type,
> +   virDomainStatsRecordPtr record,
> +   int *maxparams)
>  {
>  char param_name[VIR_TYPED_PARAM_FIELD_LENGTH];
>  uint64_t value = 0;
> @@ -18977,14 +18977,9 @@ qemuDomainGetStatsPerf(virQEMUDriverPtr driver 
> ATTRIBUTE_UNUSED,
>  if (!virPerfEventIsEnabled(priv->perf, i))
>   continue;
>  
> -switch (i) {
> -case VIR_PERF_EVENT_CMT:
> -case VIR_PERF_EVENT_MBMT:
> -case VIR_PERF_EVENT_MBML:
> -if (qemuDomainGetStatsPerfRdt(priv->perf, i, record, maxparams) 
> < 0)
> -goto cleanup;
> -break;
> -}
> +if (qemuDomainGetStatsPerfOneEvent(priv->perf, i,
> +   record, maxparams) < 0)
> +goto cleanup;
>  }
>  
>  ret = 0;
> 

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

Re: [libvirt] [PATCH v2 2/8] conf: add node_device_event handling

[John Ferlan]

On 08/03/2016 05:37 PM, Cole Robinson wrote:
> On 08/03/2016 09:40 AM, John Ferlan wrote:
>>
>>
>> On 07/28/2016 08:02 AM, Jovanka Gulicoska wrote:
>>> Add node device event handling infrastructure to node_device_event.[ch]
>>> ---
>>>  src/Makefile.am  |   5 +
>>>  src/conf/node_device_event.c | 234 
>>> +++
>>>  src/conf/node_device_event.h |  59 +++
>>>  src/libvirt_private.syms |   5 +
>>>  4 files changed, 303 insertions(+)
>>>  create mode 100644 src/conf/node_device_event.c
>>>  create mode 100644 src/conf/node_device_event.h
>>>
>>
>> [...]
>>
>>> diff --git a/src/conf/node_device_event.c b/src/conf/node_device_event.c
>>> new file mode 100644
>>> index 000..61bc912
>>
>> [...]
>>
>>> +
>>> +/**
>>> + * virNodeDeviceEventLifecycleNew:
>>> + * @name: name of the node device object the event describes
>>> + * @type: type of lifecycle event
>>> + * @detail: more details about @type
>>> + *
>>> + * Create a new node device lifecycle event.
>>> + */
>>> +virObjectEventPtr
>>> +virNodeDeviceEventLifecycleNew(const char *name,
>>> +   int type,
>>> +   int detail)
>>> +{
>>> +virNodeDeviceEventLifecyclePtr event;
>>> +
>>> +if (virNodeDeviceEventsInitialize() < 0)
>>> +return NULL;
>>> +
>>> +if (!(event = virObjectEventNew(virNodeDeviceEventLifecycleClass,
>>> +virNodeDeviceEventDispatchDefaultFunc,
>>> +VIR_NODE_DEVICE_EVENT_ID_LIFECYCLE,
>>> +0, name, NULL, name)))
>> 
>>
>> This has caused a Coverity build failure since the prototype has:
>>
>> ATTRIBUTE_NONNULL(6)
>>
> 
> I think just dropping it is fine? The code was updated to handle uuid=NULL
> 
> diff --git a/src/conf/object_event_private.h b/src/conf/object_event_private.h
> index 92c25d4..27b461f 100644
> --- a/src/conf/object_event_private.h
> +++ b/src/conf/object_event_private.h
> @@ -106,6 +106,6 @@ virObjectEventNew(virClassPtr klass,
>const unsigned char *uuid,
>const char *key)
>  ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(5)
> -ATTRIBUTE_NONNULL(6) ATTRIBUTE_NONNULL(7);
> +ATTRIBUTE_NONNULL(7);
> 
>  #endif
> 
>> It gets even worse in the function and needs to be resolved before the
>> "next" release.
>>
> 
> I can't parse this sentence... are there additional issues?
> 
virObjectEventNew(...,const unsigned char *uuid,...)
...
memcpy(event->meta.uuid, uuid, VIR_UUID_BUFLEN);
...

e.g.
memcpy(event->meta.uuid, NULL, VIR_UUID_BUFLEN);


John

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

Re: [libvirt] Examples of QEMU machine-type-specific code in libvirt?

[Laine Stump]

On 08/03/2016 04:49 PM, Eduardo Habkost wrote:

Hi,

I am collecting some material for my KVM Forum talk, and I am
interested in hearing from libvirt developers about instances
where machine-type-specific information is needed by libvirt, and
the info is not provided by QEMU.

I found some instances where machine-type capabilities are
hardcoded in libvirt itself (see below[1]), but I would like to
know if you have other examples. They may be about things that
are already done by libvirt today, or about things that you would
like to do in the future.


(I'm guessing this first one may be what put you onto the topic in the 
first place, but for completeness' sake...)


Without actually instantiating a virtual machine of a particular type, 
It's not possible to know whether that machinetype has a legacy-pci root 
bus, or a PCIe root complex (or neither). This means we have to guess, 
and it has big implications. In some cases our guess is 100% correct 
(i440fx and q35 machinetypes inqemu-system-x86_64) but in other cases we 
may or may not be right (the aarch64 "virt" machinetype, which as I 
understand may or may not have a PCIe root complex, depending on the 
qemu version?)


This in turn leads to differences in which PCI controllers are available 
for a machinetype (e.g., even though the ioh3420 (pcie-root-port) device 
is advertised as available in qemu-system_x86_64, it's only actually 
usable for q35-based machinetypes; same for pxb-pcie, and the xio3130 
upstream/downstream ports.


Same thing goes for some other integrated devices - Q35-based 
machinetypes include an AHCI controller while i440fx machinetypes (as 
well as "sun4u" and "g3beige") include an IDE controller, and these are 
at fixed addresses (and can't be removed).


And then there is the Q35 machinetype vs. fdc. Yeah, sure nobody in this 
age should be using a floppy disk (even emulated), but even within 
different versions of the q35 machinetype, a floppy disk controller may 
or may not be present (see qemuDominMachineNeedsFDC()).


You may say that libvirt could solve this problem when probing qemu 
capabilities by simply instantiating a virtual machine and then 
examining its devices, but (I've been told) this is too expensive 
(especially since we'd need to do it for every machinetype that was 
used, although admittedly we've been caching the results for quite 
awhile now, so the impact may not be as great as it once was).




[1] Examples where machine-type names are hardcoded in libvirt:


Keep in mind that some of those places we're checking os.machine are 
actually utility functions whose only job is to check the machinetype 
and return true/false, and they're called from several other places. For 
example qemuDomainMachineIsQ35(), qemuDomainMachineIs440FX(), and 
qeuDomainMachineIsVirt() (which actually checks if the arch is armv7l or 
aarch64 *and* machinetype is "virt*").


Which points out - it's a slightly different issue (since in these cases 
at least it's a different qemu binary), but you may also want to look at 
places where we examine "os.arch" and make a decision based on that.




src/qemu/qemu_capabilities.c:STREQ(def->os.machine, "ppce500"))
src/qemu/qemu_capabilities.c:STREQ(def->os.machine, "prep"))
src/qemu/qemu_capabilities.c:STREQ(def->os.machine, "bamboo"))
src/qemu/qemu_capabilities.c:if (STREQ(def->os.machine, "mpc8544ds"))
src/qemu/qemu_capabilities.c:if (STREQ(machines[i]->name, "none"))
src/qemu/qemu_capabilities.c:STREQ(def->os.machine, "isapc");
src/qemu/qemu_capabilities.c:(STRNEQ(machine, "pseries") && !STRPREFIX(machine, 
"pseries-")))
src/qemu/qemu_capabilities.c:(STRNEQ(machine, "pseries") && !STRPREFIX(machine, 
"pseries-")))
src/qemu/qemu_capabilities.c:if (STRNEQ(domCaps->machine, "virt") &&
src/qemu/qemu_capabilities.c:!STRPREFIX(domCaps->machine, "virt-"))
src/qemu/qemu_command.c:if (STRPREFIX(def->os.machine, "s390-virtio") &&
src/qemu/qemu_domain.c:if (STREQ(def->os.machine, "isapc")) {
src/qemu/qemu_domain.c:if (STREQ(def->os.machine, "versatilepb"))
src/qemu/qemu_domain.c:return (STRPREFIX(def->os.machine, "pc-q35") ||
src/qemu/qemu_domain.c:STREQ(def->os.machine, "q35"));
src/qemu/qemu_domain.c:return (STREQ(def->os.machine, "pc") ||
src/qemu/qemu_domain.c:STRPREFIX(def->os.machine, "pc-0.") ||
src/qemu/qemu_domain.c:STRPREFIX(def->os.machine, "pc-1.") ||
src/qemu/qemu_domain.c:STRPREFIX(def->os.machine, "pc-i440") ||
src/qemu/qemu_domain.c:STRPREFIX(def->os.machine, "rhel"));
src/qemu/qemu_domain.c:char *p = STRSKIP(def->os.machine, "pc-q35-");
src/qemu/qemu_domain.c:return STRPREFIX(def->os.machine, "s390-ccw");
src/qemu/qemu_domain.c:if (STRNEQ(def->os.machine, "virt") &&
src/qemu/qemu_domain.c:!STRPREFIX(def->os.machine, "virt-"))
src/qemu/qemu_domain.c:if (STRNEQ(def->os.machine, 

Re: [libvirt] [PATCH v2 2/8] conf: add node_device_event handling

[Cole Robinson]

On 08/03/2016 09:40 AM, John Ferlan wrote:
> 
> 
> On 07/28/2016 08:02 AM, Jovanka Gulicoska wrote:
>> Add node device event handling infrastructure to node_device_event.[ch]
>> ---
>>  src/Makefile.am  |   5 +
>>  src/conf/node_device_event.c | 234 
>> +++
>>  src/conf/node_device_event.h |  59 +++
>>  src/libvirt_private.syms |   5 +
>>  4 files changed, 303 insertions(+)
>>  create mode 100644 src/conf/node_device_event.c
>>  create mode 100644 src/conf/node_device_event.h
>>
> 
> [...]
> 
>> diff --git a/src/conf/node_device_event.c b/src/conf/node_device_event.c
>> new file mode 100644
>> index 000..61bc912
> 
> [...]
> 
>> +
>> +/**
>> + * virNodeDeviceEventLifecycleNew:
>> + * @name: name of the node device object the event describes
>> + * @type: type of lifecycle event
>> + * @detail: more details about @type
>> + *
>> + * Create a new node device lifecycle event.
>> + */
>> +virObjectEventPtr
>> +virNodeDeviceEventLifecycleNew(const char *name,
>> +   int type,
>> +   int detail)
>> +{
>> +virNodeDeviceEventLifecyclePtr event;
>> +
>> +if (virNodeDeviceEventsInitialize() < 0)
>> +return NULL;
>> +
>> +if (!(event = virObjectEventNew(virNodeDeviceEventLifecycleClass,
>> +virNodeDeviceEventDispatchDefaultFunc,
>> +VIR_NODE_DEVICE_EVENT_ID_LIFECYCLE,
>> +0, name, NULL, name)))
> 
> 
> This has caused a Coverity build failure since the prototype has:
> 
> ATTRIBUTE_NONNULL(6)
> 

I think just dropping it is fine? The code was updated to handle uuid=NULL

diff --git a/src/conf/object_event_private.h b/src/conf/object_event_private.h
index 92c25d4..27b461f 100644
--- a/src/conf/object_event_private.h
+++ b/src/conf/object_event_private.h
@@ -106,6 +106,6 @@ virObjectEventNew(virClassPtr klass,
   const unsigned char *uuid,
   const char *key)
 ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(5)
-ATTRIBUTE_NONNULL(6) ATTRIBUTE_NONNULL(7);
+ATTRIBUTE_NONNULL(7);

 #endif

> It gets even worse in the function and needs to be resolved before the
> "next" release.
> 

I can't parse this sentence... are there additional issues?

Thanks,
Cole

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

[libvirt] Examples of QEMU machine-type-specific code in libvirt?

[Eduardo Habkost]

Hi,

I am collecting some material for my KVM Forum talk, and I am
interested in hearing from libvirt developers about instances
where machine-type-specific information is needed by libvirt, and
the info is not provided by QEMU.

I found some instances where machine-type capabilities are
hardcoded in libvirt itself (see below[1]), but I would like to
know if you have other examples. They may be about things that
are already done by libvirt today, or about things that you would
like to do in the future.

[1] Examples where machine-type names are hardcoded in libvirt:

src/qemu/qemu_capabilities.c:STREQ(def->os.machine, "ppce500"))
src/qemu/qemu_capabilities.c:STREQ(def->os.machine, "prep"))
src/qemu/qemu_capabilities.c:STREQ(def->os.machine, "bamboo"))
src/qemu/qemu_capabilities.c:if (STREQ(def->os.machine, "mpc8544ds"))
src/qemu/qemu_capabilities.c:if (STREQ(machines[i]->name, "none"))
src/qemu/qemu_capabilities.c:STREQ(def->os.machine, "isapc");
src/qemu/qemu_capabilities.c:(STRNEQ(machine, "pseries") && 
!STRPREFIX(machine, "pseries-")))
src/qemu/qemu_capabilities.c:(STRNEQ(machine, "pseries") && 
!STRPREFIX(machine, "pseries-")))
src/qemu/qemu_capabilities.c:if (STRNEQ(domCaps->machine, "virt") &&
src/qemu/qemu_capabilities.c:!STRPREFIX(domCaps->machine, "virt-"))
src/qemu/qemu_command.c:if (STRPREFIX(def->os.machine, "s390-virtio") &&
src/qemu/qemu_domain.c:if (STREQ(def->os.machine, "isapc")) {
src/qemu/qemu_domain.c:if (STREQ(def->os.machine, "versatilepb"))
src/qemu/qemu_domain.c:return (STRPREFIX(def->os.machine, "pc-q35") ||
src/qemu/qemu_domain.c:STREQ(def->os.machine, "q35"));
src/qemu/qemu_domain.c:return (STREQ(def->os.machine, "pc") ||
src/qemu/qemu_domain.c:STRPREFIX(def->os.machine, "pc-0.") ||
src/qemu/qemu_domain.c:STRPREFIX(def->os.machine, "pc-1.") ||
src/qemu/qemu_domain.c:STRPREFIX(def->os.machine, "pc-i440") ||
src/qemu/qemu_domain.c:STRPREFIX(def->os.machine, "rhel"));
src/qemu/qemu_domain.c:char *p = STRSKIP(def->os.machine, "pc-q35-");
src/qemu/qemu_domain.c:return STRPREFIX(def->os.machine, "s390-ccw");
src/qemu/qemu_domain.c:if (STRNEQ(def->os.machine, "virt") &&
src/qemu/qemu_domain.c:!STRPREFIX(def->os.machine, "virt-"))
src/qemu/qemu_domain.c:if (STRNEQ(def->os.machine, "pseries") &&
src/qemu/qemu_domain.c:!STRPREFIX(def->os.machine, "pseries-"))
src/qemu/qemu_domain.c:STREQ(def->os.machine, "malta") ||
src/qemu/qemu_domain.c:STREQ(def->os.machine, "sun4u") ||
src/qemu/qemu_domain.c:STREQ(def->os.machine, "g3beige");
src/qemu/qemu_domain_address.c:if (!(STRPREFIX(def->os.machine, 
"vexpress-") ||
src/qemu/qemu_domain_address.c:if (STREQ(def->os.machine, "versatilepb"))


-- 
Eduardo

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

Re: [libvirt] [PATCH] libxl: allow libxl to calculate shadow mem requirements

[Jim Fehlig]

On 08/03/2016 03:51 AM, Joao Martins wrote:
> On 08/03/2016 12:49 AM, Jim Fehlig wrote:
>> Long, long ago before libxl_get_required_shadow_memory() was
>> made publicly available, its code was copied to the libxl driver
>> for calculating shadow memory requirements of HVM domains.
>>
>> Long ago, libxl_get_required_shadow_memory() was exported in
>> libxl_utils.h and included in xen-devel packages everywhere.
>>
>> Remove the copied code, which has become stale, and let libxl
>> provode a proper shadow memory value.
>>
>> Signed-off-by: Jim Fehlig 
>> ---
>>
>> I ensured libxl_get_required_shadow_memory() was available
>> as far back as Xen 4.4, which is the minimum version supported
>> by the libxl driver.
> Cool, and FWIW, Looks good to me:
>
> Acked-by: Joao Martins 

Thanks. It's an obvious improvement that should have been done long ago.

>
> (Also tested an HVM guest in which codepath goes through):

Same here. And I ensured the calculation was the same compared to a VM started
with xl.

I've pushed the patch.

Regards,
Jim

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

Re: [libvirt] [PATCH 10/10] qemu: monitor: Return struct from qemuMonitor(Text|Json)QueryCPUs

[John Ferlan]

On 08/03/2016 01:00 PM, Peter Krempa wrote:
> On Wed, Aug 03, 2016 at 12:04:02 -0400, John Ferlan wrote:
>>
>>
>> On 08/03/2016 04:11 AM, Peter Krempa wrote:
>>> Prepare to extract more data by returning a array of structs rather than
>>> just an array of thread ids. Additionally report fatal errors separately
>>> from qemu not being able to produce data.
>>> ---
>>>  src/qemu/qemu_monitor.c  | 31 ---
>>>  src/qemu/qemu_monitor.h  |  6 
>>>  src/qemu/qemu_monitor_json.c | 71 
>>> ++--
>>>  src/qemu/qemu_monitor_json.h |  2 +-
>>>  src/qemu/qemu_monitor_text.c | 37 +++
>>>  src/qemu/qemu_monitor_text.h |  2 +-
>>>  tests/qemumonitorjsontest.c  | 31 ++-
>>>  7 files changed, 104 insertions(+), 76 deletions(-)
>>>
>>> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
>>> index 0011ceb..578b078 100644
>>> --- a/src/qemu/qemu_monitor.c
>>> +++ b/src/qemu/qemu_monitor.c
>>> @@ -1666,6 +1666,16 @@ qemuMonitorCPUInfoFree(qemuMonitorCPUInfoPtr cpus,
>>>  VIR_FREE(cpus);
>>>  }
>>>
>>> +void
>>> +qemuMonitorQueryCpusFree(struct qemuMonitorQueryCpusEntry *entries,
>>> + size_t nentries ATTRIBUTE_UNUSED)
>>> +{
>>> +if (!entries)
>>> +return;
>>
>> [1] Maybe this should be a 'int' parameter and a <= 0 check...
> 
> What?! That's a freeing function. That does not make any sense.
> 

oh right - my eyes read nentries...  and I even noted in the caller that
nentries is not used Anyway, I suppose part of me was trying to
forward think why you would add nentries and that passing a -2 or -1
here may not end quickly...

John

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

Re: [libvirt] [PATCH 3/3] docs: drop todo.html

[Cole Robinson]

On 08/03/2016 12:30 PM, Ján Tomko wrote:
> There is little information value in:
>   Todo list unavailable: no config file
> 
> Drop the file completely along with the script generating it.
> ---

I had a similar patch before:

http://www.redhat.com/archives/libvir-list/2016-May/msg01618.html

The only real difference is that I changed the website to point to
http://wiki.libvirt.org/page/Todo which grabs a list of RFE bugs from
bugzilla. I'm fine with this approach of dropping the website sidebar 'todo'
entry entirely though... I opted to keep it just to avoid a potential source
of complaints for my patch

- Cole

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

Re: [libvirt] [PATCH 10/10] qemu: monitor: Return struct from qemuMonitor(Text|Json)QueryCPUs

[Peter Krempa]

On Wed, Aug 03, 2016 at 12:04:02 -0400, John Ferlan wrote:
> 
> 
> On 08/03/2016 04:11 AM, Peter Krempa wrote:
> > Prepare to extract more data by returning a array of structs rather than
> > just an array of thread ids. Additionally report fatal errors separately
> > from qemu not being able to produce data.
> > ---
> >  src/qemu/qemu_monitor.c  | 31 ---
> >  src/qemu/qemu_monitor.h  |  6 
> >  src/qemu/qemu_monitor_json.c | 71 
> > ++--
> >  src/qemu/qemu_monitor_json.h |  2 +-
> >  src/qemu/qemu_monitor_text.c | 37 +++
> >  src/qemu/qemu_monitor_text.h |  2 +-
> >  tests/qemumonitorjsontest.c  | 31 ++-
> >  7 files changed, 104 insertions(+), 76 deletions(-)
> > 
> > diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
> > index 0011ceb..578b078 100644
> > --- a/src/qemu/qemu_monitor.c
> > +++ b/src/qemu/qemu_monitor.c
> > @@ -1666,6 +1666,16 @@ qemuMonitorCPUInfoFree(qemuMonitorCPUInfoPtr cpus,
> >  VIR_FREE(cpus);
> >  }
> > 
> > +void
> > +qemuMonitorQueryCpusFree(struct qemuMonitorQueryCpusEntry *entries,
> > + size_t nentries ATTRIBUTE_UNUSED)
> > +{
> > +if (!entries)
> > +return;
> 
> [1] Maybe this should be a 'int' parameter and a <= 0 check...

What?! That's a freeing function. That does not make any sense.

> 
> > +
> > +VIR_FREE(entries);
> > +}
> > +

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

[libvirt] [PATCH 0/3] docs/Makefile.am cleanups

[Ján Tomko]

The effect of 2/3 will be more apparent after apibuild.py speedups
(to be written).

Ján Tomko (3):
  docs/Makefile.am: remove redundant variables
  docs/Makefile.am: build hvsupport.html earlier
  docs: drop todo.html

 .gitignore|   1 -
 docs/Makefile.am  |  44 +++---
 docs/sitemap.html.in  |   4 --
 docs/todo.cfg-example |  26 ---
 docs/todo.pl  | 125 --
 5 files changed, 7 insertions(+), 193 deletions(-)
 delete mode 100644 docs/todo.cfg-example
 delete mode 100755 docs/todo.pl

-- 
2.7.3

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

[libvirt] [PATCH 2/3] docs/Makefile.am: build hvsupport.html earlier

[Ján Tomko]

This file requires three steps instead of two.
Move it earlier in the list of targets to avoid waiting for it.
---
 docs/Makefile.am | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/docs/Makefile.am b/docs/Makefile.am
index f266117..d6e975d 100644
--- a/docs/Makefile.am
+++ b/docs/Makefile.am
@@ -96,9 +96,10 @@ internals_html = $(internals_html_in:%.html.in=%.html)
 # For all other files, since we ship pre-built html in the
 # tarball, we must also ship the sources, even when those
 # sources are themselves generated.
-dot_html_in = $(notdir $(wildcard $(srcdir)/*.html.in)) \
+dot_html_in = \
+  hvsupport.html.in \
   todo.html.in \
-  hvsupport.html.in
+  $(notdir $(wildcard $(srcdir)/*.html.in))
 dot_html = $(dot_html_in:%.html.in=%.html)
 
 dot_php_in = $(notdir $(wildcard $(srcdir)/*.php.in))
-- 
2.7.3

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

[libvirt] [PATCH 1/3] docs/Makefile.am: remove redundant variables

[Ján Tomko]

Remove DOC_SOURCE_DIR, introduced by and
unused since commit b325d74.

PERL is already detected in configure.ac.
---
 docs/Makefile.am | 5 -
 1 file changed, 5 deletions(-)

diff --git a/docs/Makefile.am b/docs/Makefile.am
index 206ef3b..f266117 100644
--- a/docs/Makefile.am
+++ b/docs/Makefile.am
@@ -16,11 +16,6 @@
 ## License along with this library.  If not, see
 ## .
 
-PERL = perl
-
-# The directory containing the source code (if it contains documentation).
-DOC_SOURCE_DIR=../src
-
 DEVHELP_DIR=$(datadir)/gtk-doc/html/libvirt
 
 apihtml =  \
-- 
2.7.3

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

[libvirt] [PATCH 3/3] docs: drop todo.html

[Ján Tomko]

There is little information value in:
  Todo list unavailable: no config file

Drop the file completely along with the script generating it.
---
 .gitignore|   1 -
 docs/Makefile.am  |  34 ++
 docs/sitemap.html.in  |   4 --
 docs/todo.cfg-example |  26 ---
 docs/todo.pl  | 125 --
 5 files changed, 4 insertions(+), 186 deletions(-)
 delete mode 100644 docs/todo.cfg-example
 delete mode 100755 docs/todo.pl

diff --git a/.gitignore b/.gitignore
index e87c085..96347d8 100644
--- a/.gitignore
+++ b/.gitignore
@@ -73,7 +73,6 @@
 /docs/libvirt-qemu-*.xml
 /docs/libvirt-refs.xml
 /docs/search.php
-/docs/todo.html.in
 /examples/admin/client_close
 /examples/admin/client_info
 /examples/admin/client_limits
diff --git a/docs/Makefile.am b/docs/Makefile.am
index d6e975d..4665050 100644
--- a/docs/Makefile.am
+++ b/docs/Makefile.am
@@ -90,15 +90,10 @@ internals_html_in = \
   $(patsubst $(srcdir)/%,%,$(wildcard $(srcdir)/internals/*.html.in))
 internals_html = $(internals_html_in:%.html.in=%.html)
 
-# todo.html is special - it is shipped in the tarball, but we
-# have a dedicated 'todo' target to rebuild it from a proper
-# config file, all other users are able to build it locally.
-# For all other files, since we ship pre-built html in the
-# tarball, we must also ship the sources, even when those
-# sources are themselves generated.
+# Since we ship pre-built html in the tarball, we must also ship
+# the sources, even when those sources are themselves generated.
 dot_html_in = \
   hvsupport.html.in \
-  todo.html.in \
   $(notdir $(wildcard $(srcdir)/*.html.in))
 dot_html = $(dot_html_in:%.html.in=%.html)
 
@@ -156,7 +151,7 @@ EXTRA_DIST= \
   $(patches) $(dot_php_in) $(dot_php_code_in) $(dot_php)\
   $(internals_html_in) $(internals_html) \
   sitemap.html.in aclperms.htmlinc \
-  todo.pl hvsupport.pl todo.cfg-example \
+  hvsupport.pl \
   $(schema_DATA)
 
 acl_generated = aclperms.htmlinc
@@ -183,24 +178,6 @@ admin_api: $(srcdir)/libvirt-admin-api.xml 
$(srcdir)/libvirt-admin-refs.xml
 web: $(dot_html) $(internals_html) html/index.html devhelp/index.html \
   $(dot_php)
 
-todo.html.in: todo.pl
-   if [ -f  todo.cfg ]; then \
-   echo "Generating $@"; \
-   $(PERL) $< > $@ \
-   || { rm $@ && exit 1; }; \
-   else \
-   echo "Stubbing $@"; \
-   printf "%s\n" \
- "http://www.w3.org/1999/xhtml\;>" \
- "" \
- "Todo list unavailable: no config file" \
- "" > $@ ; \
-   fi
-
-todo:
-   rm -f todo.html.in
-   $(MAKE) todo.html
-
 hvsupport.html: $(srcdir)/hvsupport.html.in
 
 $(srcdir)/hvsupport.html.in: $(srcdir)/hvsupport.pl $(api_DATA) \
@@ -210,8 +187,6 @@ $(srcdir)/hvsupport.html.in: $(srcdir)/hvsupport.pl 
$(api_DATA) \
$(AM_V_GEN)$(PERL) $(srcdir)/hvsupport.pl $(top_srcdir)/src > $@ \
|| { rm $@ && exit 1; }
 
-.PHONY: todo
-
 %.png: %.fig
convert -rotate 90 $< $@
 
@@ -333,8 +308,7 @@ clean-local:
rm -f *~ *.bak *.hierarchy *.signals *-unused.txt *.html
 
 maintainer-clean-local: clean-local
-   rm -rf $(srcdir)/libvirt-api.xml $(srcdir)/libvirt-refs.xml \
-   todo.html.in
+   rm -rf $(srcdir)/libvirt-api.xml $(srcdir)/libvirt-refs.xml
rm -rf $(srcdir)/libvirt-qemu-api.xml $(srcdir)/libvirt-qemu-refs.xml
rm -rf $(srcdir)/libvirt-lxc-api.xml $(srcdir)/libvirt-lxc-refs.xml
rm -rf $(srcdir)/libvirt-admin-api.xml $(srcdir)/libvirt-admin-refs.xml
diff --git a/docs/sitemap.html.in b/docs/sitemap.html.in
index 757d5aa..fd80d76 100644
--- a/docs/sitemap.html.in
+++ b/docs/sitemap.html.in
@@ -449,10 +449,6 @@
 Security bug reporting and resolution process
   
   
-Todo list
-Main feature request list
-  
-  
 Pending patches
 Pending patches awaiting reviews and integration
   
diff --git a/docs/todo.cfg-example b/docs/todo.cfg-example
deleted file mode 100644
index a99c61a..000
--- a/docs/todo.cfg-example
+++ /dev/null
@@ -1,26 +0,0 @@
-bugzilla = {
-#username = ...some email addr...
-#password = ...some bz password...
-server = https://bugzilla.redhat.com
-}
-query = {
-  product = Virtualization Tools
-  alias = libvirtTodo
-}
-
-output = {
-   title = Todo list
-   blurb = 

[libvirt] [PATCH 6/7] cfg.mk: drop redundant sc_size_of_brackets

[Ján Tomko]

This check forbids a space after sizeof, which is already done
by check-spacing.pl.
---
 build-aux/check-spacing.pl | 2 +-
 cfg.mk | 6 --
 2 files changed, 1 insertion(+), 7 deletions(-)

diff --git a/build-aux/check-spacing.pl b/build-aux/check-spacing.pl
index 448acf2..afc7758 100755
--- a/build-aux/check-spacing.pl
+++ b/build-aux/check-spacing.pl
@@ -94,7 +94,7 @@ foreach my $file (@ARGV) {
 if ($kw =~ /^(?:if|for|while|switch|return)$/) {
 $tmpdata =~ s/(?:$kw\s\()/XXX(/;
 } else {
-print "Whitespace after non-keyword:\n";
+print "Whitespace after non-keyword or sizeof:\n";
 print "$file:$.: $line";
 $ret = 1;
 last;
diff --git a/cfg.mk b/cfg.mk
index 9f236c2..3405c9a 100644
--- a/cfg.mk
+++ b/cfg.mk
@@ -476,12 +476,6 @@ sc_forbid_const_pointer_typedef:
halt='use correct style and type for Ptr typedefs'  \
  $(_sc_search_regexp)
 
-# Forbid sizeof foo or sizeof (foo), require sizeof(foo)
-sc_size_of_brackets:
-   @prohibit='sizeof\s'\
-   halt='use sizeof(foo), not sizeof (foo) or sizeof foo'  \
- $(_sc_search_regexp)
-
 # Ensure that no C source file, docs, or rng schema uses TABs for
 # indentation.  Also match *.h.in files, to get libvirt.h.in.  Exclude
 # files in gnulib, since they're imported.
-- 
2.7.3

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

[libvirt] [PATCH 2/7] configure.ac: drop checks for ETH flags

[Ján Tomko]

No choices are made at configure time based on these checks.

Drop them and use #ifdefs in virnetdev.c.
---
 configure.ac |  5 -
 src/util/virnetdev.c | 20 ++--
 2 files changed, 10 insertions(+), 15 deletions(-)

diff --git a/configure.ac b/configure.ac
index 8d7d63e..db42173 100644
--- a/configure.ac
+++ b/configure.ac
@@ -402,11 +402,6 @@ AC_CHECK_TYPE([struct sockpeercred],
   [], [[#include 
   ]])
 
-AC_CHECK_DECLS([ETH_FLAG_TXVLAN, ETH_FLAG_NTUPLE, ETH_FLAG_RXHASH, 
ETH_FLAG_LRO,
-ETHTOOL_GGSO, ETHTOOL_GGRO, ETHTOOL_GFLAGS, ETHTOOL_GFEATURES],
-  [], [], [[#include 
-  ]])
-
 dnl Our only use of libtasn1.h is in the testsuite, and can be skipped
 dnl if the header is not present.  Assume -ltasn1 is present if the
 dnl header could be found.
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index fa695d4..c3a35bb 100644
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -85,7 +85,7 @@ VIR_LOG_INIT("util.netdev");
 #endif
 
 #define RESOURCE_FILE_LEN 4096
-#if HAVE_DECL_ETHTOOL_GFEATURES
+#ifdef ETHTOOL_GFEATURES
 # define TX_UDP_TNL 25
 # define GFEATURES_SIZE 2
 # define FEATURE_WORD(blocks, index, field)  ((blocks)[(index) / 32U].field)
@@ -2446,28 +2446,28 @@ virNetDevGetEthtoolFeatures(virBitmapPtr bitmap,
 {ETHTOOL_GTXCSUM, VIR_NET_DEV_FEAT_GTXCSUM},
 {ETHTOOL_GSG, VIR_NET_DEV_FEAT_GSG},
 {ETHTOOL_GTSO, VIR_NET_DEV_FEAT_GTSO},
-# if HAVE_DECL_ETHTOOL_GGSO
+# ifdef ETHTOOL_GGSO
 {ETHTOOL_GGSO, VIR_NET_DEV_FEAT_GGSO},
 # endif
-# if HAVE_DECL_ETHTOOL_GGRO
+# ifdef ETHTOOL_GGRO
 {ETHTOOL_GGRO, VIR_NET_DEV_FEAT_GGRO},
 # endif
 };
 
-# if HAVE_DECL_ETHTOOL_GFLAGS
+# ifdef ETHTOOL_GFLAGS
 /* ethtool masks */
 struct virNetDevEthtoolFeatureCmd flags[] = {
-#  if HAVE_DECL_ETH_FLAG_LRO
+#  ifdef ETH_FLAG_LRO
 {ETH_FLAG_LRO, VIR_NET_DEV_FEAT_LRO},
 #  endif
-#  if HAVE_DECL_ETH_FLAG_TXVLAN
+#  ifdef ETH_FLAG_TXVLAN
 {ETH_FLAG_RXVLAN, VIR_NET_DEV_FEAT_RXVLAN},
 {ETH_FLAG_TXVLAN, VIR_NET_DEV_FEAT_TXVLAN},
 #  endif
-#  if HAVE_DECL_ETH_FLAG_NTUBLE
+#  ifdef ETH_FLAG_NTUPLE
 {ETH_FLAG_NTUPLE, VIR_NET_DEV_FEAT_NTUPLE},
 #  endif
-#  if HAVE_DECL_ETH_FLAG_RXHASH
+#  ifdef ETH_FLAG_RXHASH
 {ETH_FLAG_RXHASH, VIR_NET_DEV_FEAT_RXHASH},
 #  endif
 };
@@ -2479,7 +2479,7 @@ virNetDevGetEthtoolFeatures(virBitmapPtr bitmap,
 ignore_value(virBitmapSetBit(bitmap, ethtool_cmds[i].feat));
 }
 
-# if HAVE_DECL_ETHTOOL_GFLAGS
+# ifdef ETHTOOL_GFLAGS
 cmd.cmd = ETHTOOL_GFLAGS;
 if (virNetDevFeatureAvailable(fd, ifr, )) {
 for (i = 0; i < ARRAY_CARDINALITY(flags); i++) {
@@ -2491,7 +2491,7 @@ virNetDevGetEthtoolFeatures(virBitmapPtr bitmap,
 }
 
 
-# if HAVE_DECL_ETHTOOL_GFEATURES
+# ifdef ETHTOOL_GFEATURES
 /**
  * virNetDevGFeatureAvailable
  * This function checks for the availability of a network device gfeature
-- 
2.7.3

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

[libvirt] [PATCH 5/7] cfg.mk: drop redundant sc_prohibit_gethostby

[Ján Tomko]

Both gethostbyaddr and gethostbyname* are already checked
by sc_prohibit_nonreentrant.
---
 cfg.mk | 9 -
 1 file changed, 9 deletions(-)

diff --git a/cfg.mk b/cfg.mk
index 0604d69..9f236c2 100644
--- a/cfg.mk
+++ b/cfg.mk
@@ -520,13 +520,6 @@ sc_forbid_manual_xml_indent:
halt='use virBufferAdjustIndent instead of spaces when indenting xml' \
  $(_sc_search_regexp)
 
-# Not only do they fail to deal well with ipv6, but the gethostby*
-# functions are also not thread-safe.
-sc_prohibit_gethostby:
-   @prohibit='\

[libvirt] [PATCH 3/7] tests: fix the return value of test-wrap-argv

[Ján Tomko]

The script was returning success unless it failed on the last file.
This went unnoticed because sc_prohibit_long_lines forbids lines
longer than 90 characters in .arg[sv] files.
---
 tests/test-wrap-argv.pl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/test-wrap-argv.pl b/tests/test-wrap-argv.pl
index 2898156..f0d3c0b 100755
--- a/tests/test-wrap-argv.pl
+++ b/tests/test-wrap-argv.pl
@@ -39,8 +39,8 @@ if (@ARGV[0] eq "--in-place") {
 shift @ARGV;
 }
 
+$ret = 0;
 foreach my $file (@ARGV) {
-$ret = 0;
 if (($file) < 0) {
 $ret = 1;
 }
-- 
2.7.3

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

[libvirt] [PATCH 0/7] More syntax-check cleanups

[Ján Tomko]

The first patch pulls the latest gnulibs to fetch the newest syntax-check
speedups.

Patch 2 is included since a gnulib update will trigger configure anyway.

The rest are mostly cleanups, the speed-ups they (might) provide are barely
measurable.

Ján Tomko (7):
  maint: update to latest gnulib
  configure.ac: drop checks for ETH flags
  tests: fix the return value of test-wrap-argv
  cfg.mk: use subst instead of tr
  cfg.mk: drop redundant sc_prohibit_gethostby
  cfg.mk: drop redundant sc_size_of_brackets
  cfg.mk: join not_streq and not_strneq tests

 .gnulib|  2 +-
 build-aux/check-spacing.pl |  2 +-
 cfg.mk | 35 +--
 configure.ac   |  5 -
 src/util/virnetdev.c   | 20 ++--
 tests/test-wrap-argv.pl|  2 +-
 tests/virstoragetest.c |  2 +-
 7 files changed, 19 insertions(+), 49 deletions(-)

-- 
2.7.3

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

[libvirt] [PATCH 7/7] cfg.mk: join not_streq and not_strneq tests

[Ján Tomko]

The marginally nicer error message is not worth the extra lines in
cfg.mk.

Also drop the excludes since there was only one offender in the tests.
---
 cfg.mk | 15 ++-
 tests/virstoragetest.c |  2 +-
 2 files changed, 3 insertions(+), 14 deletions(-)

diff --git a/cfg.mk b/cfg.mk
index 3405c9a..42123c3 100644
--- a/cfg.mk
+++ b/cfg.mk
@@ -980,13 +980,8 @@ sc_prohibit_pthread_create:
  $(_sc_search_regexp)
 
 sc_prohibit_not_streq:
-   @prohibit='! *STREQ *\(.*\)'\
-   halt='Use STRNEQ instead of !STREQ' \
- $(_sc_search_regexp)
-
-sc_prohibit_not_strneq:
-   @prohibit='! *STRNEQ *\(.*\)'   \
-   halt='Use STREQ instead of !STRNEQ' \
+   @prohibit='! *STRN?EQ *\(.*\)'  \
+   halt='Use STRNEQ instead of !STREQ and STREQ instead of !STRNEQ'
\
  $(_sc_search_regexp)
 
 sc_prohibit_verbose_strcat:
@@ -1240,12 +1235,6 @@ exclude_file_name_regexp--sc_prohibit_sysconf_pagesize = 
\
 exclude_file_name_regexp--sc_prohibit_pthread_create = \
   ^(cfg\.mk|src/util/virthread\.c|tests/.*)$$
 
-exclude_file_name_regexp--sc_prohibit_not_streq = \
-  ^tests/.*\.[ch]$$
-
-exclude_file_name_regexp--sc_prohibit_not_strneq = \
-  ^tests/.*\.[ch]$$
-
 exclude_file_name_regexp--sc_prohibit_dt_without_code = \
   ^docs/(newapi\.xsl|(apps|contact)\.html\.in)$$
 
diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c
index 3b19f59..184b936 100644
--- a/tests/virstoragetest.c
+++ b/tests/virstoragetest.c
@@ -698,7 +698,7 @@ testBackingParse(const void *args)
 goto cleanup;
 }
 
-if (!STREQ(xml, data->expect)) {
+if (STRNEQ(xml, data->expect)) {
 fprintf(stderr, "\n backing store string '%s'\n"
 "expected storage source xml:\n%s\n"
 "actual storage source xml:\n%s\n",
-- 
2.7.3

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

[libvirt] [PATCH 4/7] cfg.mk: use subst instead of tr

[Ján Tomko]

GNU make is able to replace characters, no need to call tr.
---
 cfg.mk | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/cfg.mk b/cfg.mk
index cf47d4c..0604d69 100644
--- a/cfg.mk
+++ b/cfg.mk
@@ -614,8 +614,9 @@ msg_gen_function += xenapiSessionErrorHandler
 # msg_gen_function += vshPrint
 # msg_gen_function += vshError
 
-func_or := $(shell echo $(msg_gen_function)|tr -s ' ' '|')
-func_re := ($(func_or))
+space =
+space +=
+func_re= ($(subst $(space),|,$(msg_gen_function)))
 
 # Look for diagnostics that aren't marked for translation.
 # This won't find any for which error's format string is on a separate line.
-- 
2.7.3

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

[libvirt] [PATCH 1/7] maint: update to latest gnulib

[Ján Tomko]

Pick up the new syntax-check speedups.
---
 .gnulib | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/.gnulib b/.gnulib
index 68b6ade..0fe8b3c 16
--- a/.gnulib
+++ b/.gnulib
@@ -1 +1 @@
-Subproject commit 68b6adebef05670a312fb92b05e7bd089d2ed43a
+Subproject commit 0fe8b3c8116168e3b8b058df63ddedd45c3fcab0
-- 
2.7.3

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

Re: [libvirt] [PATCH 10/10] qemu: monitor: Return struct from qemuMonitor(Text|Json)QueryCPUs

[John Ferlan]

On 08/03/2016 04:11 AM, Peter Krempa wrote:
> Prepare to extract more data by returning a array of structs rather than
> just an array of thread ids. Additionally report fatal errors separately
> from qemu not being able to produce data.
> ---
>  src/qemu/qemu_monitor.c  | 31 ---
>  src/qemu/qemu_monitor.h  |  6 
>  src/qemu/qemu_monitor_json.c | 71 
> ++--
>  src/qemu/qemu_monitor_json.h |  2 +-
>  src/qemu/qemu_monitor_text.c | 37 +++
>  src/qemu/qemu_monitor_text.h |  2 +-
>  tests/qemumonitorjsontest.c  | 31 ++-
>  7 files changed, 104 insertions(+), 76 deletions(-)
> 
> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
> index 0011ceb..578b078 100644
> --- a/src/qemu/qemu_monitor.c
> +++ b/src/qemu/qemu_monitor.c
> @@ -1666,6 +1666,16 @@ qemuMonitorCPUInfoFree(qemuMonitorCPUInfoPtr cpus,
>  VIR_FREE(cpus);
>  }
> 
> +void
> +qemuMonitorQueryCpusFree(struct qemuMonitorQueryCpusEntry *entries,
> + size_t nentries ATTRIBUTE_UNUSED)
> +{
> +if (!entries)
> +return;

[1] Maybe this should be a 'int' parameter and a <= 0 check...

> +
> +VIR_FREE(entries);
> +}
> +
> 
>  /**
>   * qemuMonitorGetCPUInfo:
> @@ -1686,10 +1696,10 @@ qemuMonitorGetCPUInfo(qemuMonitorPtr mon,
>size_t maxvcpus)
>  {
>  qemuMonitorCPUInfoPtr info = NULL;
> -int *pids = NULL;
> +struct qemuMonitorQueryCpusEntry *cpuentries = NULL;
>  size_t i;
>  int ret = -1;
> -int rc;
> +int ncpuentries;
> 
>  QEMU_CHECK_MONITOR(mon);
> 
> @@ -1697,25 +1707,26 @@ qemuMonitorGetCPUInfo(qemuMonitorPtr mon,
>  return -1;
> 
>  if (mon->json)
> -rc = qemuMonitorJSONQueryCPUs(mon, );
> +ncpuentries = qemuMonitorJSONQueryCPUs(mon, );
>  else
> -rc = qemuMonitorTextQueryCPUs(mon, );
> +ncpuentries = qemuMonitorTextQueryCPUs(mon, );
> +
> +if (ncpuentries < 0) {
> +if (ncpuentries == -2)
> +ret = 0;

Similar to previous patch w/r/t "ret = 0" and returning to caller

> 
> -if (rc < 0) {
> -virResetLastError();
> -ret = 0;
>  goto cleanup;
>  }
> 
> -for (i = 0; i < rc; i++)
> -info[i].tid = pids[i];
> +for (i = 0; i < ncpuentries; i++)
> +info[i].tid = cpuentries[i].tid;
> 
>  VIR_STEAL(*vcpus, info);
>  ret = 0;
> 
>   cleanup:
>  qemuMonitorCPUInfoFree(info, maxvcpus);
> -VIR_FREE(pids);
> +qemuMonitorQueryCpusFree(cpuentries, ncpuentries);

[1]Although not "currently" used, the ncpuentries is an int being passed
to a function that wants size_t

And yes, that's from a Coverity warning

>  return ret;
>  }
> 
> diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
> index 1ef002a..6022b9d 100644
> --- a/src/qemu/qemu_monitor.h
> +++ b/src/qemu/qemu_monitor.h
> @@ -390,6 +390,12 @@ int qemuMonitorGetStatus(qemuMonitorPtr mon,
>  int qemuMonitorSystemReset(qemuMonitorPtr mon);
>  int qemuMonitorSystemPowerdown(qemuMonitorPtr mon);
> 
> +struct qemuMonitorQueryCpusEntry {
> +pid_t tid;
> +};
> +void qemuMonitorQueryCpusFree(struct qemuMonitorQueryCpusEntry *entries,
> +  size_t nentries);
> +
> 
>  struct _qemuMonitorCPUInfo {
>  pid_t tid;
> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> index 715e1c7..8018860 100644
> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c
> @@ -1323,69 +1323,65 @@ int qemuMonitorJSONSystemReset(qemuMonitorPtr mon)
>   *   { "CPU": 1, "current": false, "halted": true, "pc": 7108165 } ]
>   */
>  static int
> -qemuMonitorJSONExtractCPUInfo(virJSONValuePtr reply,
> -  int **pids)
> +qemuMonitorJSONExtractCPUInfo(virJSONValuePtr data,
> +  struct qemuMonitorQueryCpusEntry **entries)
>  {
> -virJSONValuePtr data;
> +struct qemuMonitorQueryCpusEntry *cpus = NULL;
>  int ret = -1;
>  size_t i;
> -int *threads = NULL;
>  ssize_t ncpus;
> 
> -if (!(data = virJSONValueObjectGetArray(reply, "return"))) {
> -virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> -   _("cpu reply was missing return data"));
> -goto cleanup;
> -}
> -
> -if ((ncpus = virJSONValueArraySize(data)) <= 0) {
> -virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> -   _("cpu information was empty"));
> -goto cleanup;
> -}
> +if ((ncpus = virJSONValueArraySize(data)) <= 0)
> +return -2;
> 
> -if (VIR_ALLOC_N(threads, ncpus) < 0)
> +if (VIR_ALLOC_N(cpus, ncpus) < 0)
>  goto cleanup;
> 
>  for (i = 0; i < ncpus; i++) {
>  virJSONValuePtr entry = virJSONValueArrayGet(data, i);
> -int thread;
> +int thread = 0;
>  if (!entry) {
> -virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> -

[libvirt] [PATCH 1/2] qemu: add a max_core setting to qemu.conf for core dump size

[Daniel P. Berrange]

Currently the QEMU processes inherit their core dump rlimit
from libvirtd, which is really suboptimal. This change allows
their limit to be directly controlled from qemu.conf instead.
---
 src/libvirt_private.syms   |  2 ++
 src/qemu/libvirtd_qemu.aug |  4 
 src/qemu/qemu.conf | 23 ++-
 src/qemu/qemu_conf.c   | 17 +
 src/qemu/qemu_conf.h   |  1 +
 src/qemu/qemu_process.c|  1 +
 src/qemu/test_libvirtd_qemu.aug.in |  1 +
 src/util/vircommand.c  | 14 ++
 src/util/vircommand.h  |  1 +
 src/util/virprocess.c  | 36 
 src/util/virprocess.h  |  1 +
 11 files changed, 100 insertions(+), 1 deletion(-)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 419c33d..6dc2b23 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1389,6 +1389,7 @@ virCommandSetErrorFD;
 virCommandSetGID;
 virCommandSetInputBuffer;
 virCommandSetInputFD;
+virCommandSetMaxCoreSize;
 virCommandSetMaxFiles;
 virCommandSetMaxMemLock;
 virCommandSetMaxProcesses;
@@ -2199,6 +2200,7 @@ virProcessRunInMountNamespace;
 virProcessSchedPolicyTypeFromString;
 virProcessSchedPolicyTypeToString;
 virProcessSetAffinity;
+virProcessSetMaxCoreSize;
 virProcessSetMaxFiles;
 virProcessSetMaxMemLock;
 virProcessSetMaxProcesses;
diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug
index 8bc23ba..9ec8250 100644
--- a/src/qemu/libvirtd_qemu.aug
+++ b/src/qemu/libvirtd_qemu.aug
@@ -22,6 +22,9 @@ module Libvirtd_qemu =
let int_entry   (kw:string) = [ key kw . value_sep . int_val ]
let str_array_entry (kw:string) = [ key kw . value_sep . str_array_val ]
 
+   let unlimited_val =  del /\"/ "\"" . store /unlimited/ . del /\"/ "\""
+   let limits_entry (kw:string) = [ key kw . value_sep . unlimited_val ] |  [ 
key kw . value_sep . int_val ]
+
 
(* Config entry grouped by function - same order as example config *)
let vnc_entry = str_entry "vnc_listen"
@@ -72,6 +75,7 @@ module Libvirtd_qemu =
  | bool_entry "set_process_name"
  | int_entry "max_processes"
  | int_entry "max_files"
+ | limits_entry "max_core"
  | str_entry "stdio_handler"
 
let device_entry = bool_entry "mac_filter"
diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf
index 7964273..abf9938 100644
--- a/src/qemu/qemu.conf
+++ b/src/qemu/qemu.conf
@@ -401,7 +401,28 @@
 #max_processes = 0
 #max_files = 0
 
-
+# If max_core is set to a positive integer, then QEMU will be
+# permitted to create core dumps when it crashes, provided its
+# RAM size is smaller than the limit set.
+#
+# Be warned that the core dump will include a full copy of the
+# guest RAM, unless it has been disabled via the guest XML by
+# setting:
+#
+#   ...guest ram...
+#
+# If guest RAM is to be included, ensure the max_core limit
+# is set to at least the size of the largest expected guest
+# plus another 1GB for any QEMU host side memory mappings.
+#
+# As a special case it can be set to the string "unlimited" to
+# to allow arbitrarily sized core dumps.
+#
+# By default the core dump size is set to 0 disabling all dumps
+#
+# Size is in bytes or string "unlimited"
+#
+#max_core = "unlimited"
 
 # mac_filter enables MAC addressed based filtering on bridge ports.
 # This currently requires ebtables to be installed.
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index fa9d65e..45d039c 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -393,6 +393,7 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg,
 char **controllers = NULL;
 char **hugetlbfs = NULL;
 char **nvram = NULL;
+char *corestr = NULL;
 
 /* Just check the file is readable before opening it, otherwise
  * libvirt emits an error.
@@ -633,6 +634,21 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg,
 if (virConfGetValueUInt(conf, "max_files", >maxFiles) < 0)
 goto cleanup;
 
+if (virConfGetValueType(conf, "max_core") == VIR_CONF_STRING) {
+if (virConfGetValueString(conf, "max_core", ) < 0)
+goto cleanup;
+if (STREQ(corestr, "unlimited")) {
+cfg->maxCore = ULLONG_MAX;
+} else {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("Unknown core size '%s'"),
+   corestr);
+goto cleanup;
+}
+} else if (virConfGetValueULLong(conf, "max_core", >maxCore) < 0) {
+goto cleanup;
+}
+
 if (virConfGetValueString(conf, "lock_manager", >lockManagerName) < 0)
 goto cleanup;
 if (virConfGetValueString(conf, "stdio_handler", ) < 0)
@@ -715,6 +731,7 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg,
 virStringFreeList(controllers);
 virStringFreeList(hugetlbfs);
 

[libvirt] [PATCH 0/2] Improve handling of QEMU core dumping

[Daniel P. Berrange]

Daniel P. Berrange (2):
  qemu: add a max_core setting to qemu.conf for core dump size
  qemu: allow turning off QEMU guest RAM dump globally

 src/libvirt_private.syms   |  2 ++
 src/qemu/libvirtd_qemu.aug |  5 +
 src/qemu/qemu.conf | 31 +++
 src/qemu/qemu_command.c| 18 --
 src/qemu/qemu_conf.c   | 20 
 src/qemu/qemu_conf.h   |  2 ++
 src/qemu/qemu_process.c|  1 +
 src/qemu/test_libvirtd_qemu.aug.in |  2 ++
 src/util/vircommand.c  | 14 ++
 src/util/vircommand.h  |  1 +
 src/util/virprocess.c  | 36 
 src/util/virprocess.h  |  1 +
 tests/qemuxml2argvtest.c   |  4 
 13 files changed, 131 insertions(+), 6 deletions(-)

-- 
2.7.4

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

[libvirt] [PATCH 2/2] qemu: allow turning off QEMU guest RAM dump globally

[Daniel P. Berrange]

We already have the ability to turn off dumping of guest
RAM via the domain XML. This is not particularly useful
though, as it is under control of the management application.
What is needed is a way for the sysadmin to turn off guest
RAM defaults globally, regardless of whether the mgmt app
provides its own way to set this in the domain XML.

So this adds a 'dump_guest_core' option in /etc/libvirt/qemu.conf
which defaults to false. ie guest RAM will never be included in
the QEMU core dumps by default. This default is different from
historical practice, but is considered to be more suitable as
a default because

 a) guest RAM can be huge and so inflicts a DOS on the host
I/O subsystem when dumping core for QEMU crashes

 b) guest RAM can contain alot of sensitive data belonging
to the VM owner. This should not generally be copied
around inside QEMU core dumps submitted to vendors for
debugging

 c) guest RAM contents are rarely useful in diagnosing
QEMU crashes

Signed-off-by: Daniel P. Berrange 
---
 src/qemu/libvirtd_qemu.aug |  1 +
 src/qemu/qemu.conf | 16 +---
 src/qemu/qemu_command.c| 18 --
 src/qemu/qemu_conf.c   |  3 +++
 src/qemu/qemu_conf.h   |  1 +
 src/qemu/test_libvirtd_qemu.aug.in |  1 +
 tests/qemuxml2argvtest.c   |  4 
 7 files changed, 35 insertions(+), 9 deletions(-)

diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug
index 9ec8250..c4ca77e 100644
--- a/src/qemu/libvirtd_qemu.aug
+++ b/src/qemu/libvirtd_qemu.aug
@@ -76,6 +76,7 @@ module Libvirtd_qemu =
  | int_entry "max_processes"
  | int_entry "max_files"
  | limits_entry "max_core"
+| bool_entry "dump_guest_core"
  | str_entry "stdio_handler"
 
let device_entry = bool_entry "mac_filter"
diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf
index abf9938..67ab215 100644
--- a/src/qemu/qemu.conf
+++ b/src/qemu/qemu.conf
@@ -406,10 +406,10 @@
 # RAM size is smaller than the limit set.
 #
 # Be warned that the core dump will include a full copy of the
-# guest RAM, unless it has been disabled via the guest XML by
-# setting:
+# guest RAM, if the 'dump_guest_core' setting has been enabled,
+# or if the guest XML contains
 #
-#   ...guest ram...
+#   ...guest ram...
 #
 # If guest RAM is to be included, ensure the max_core limit
 # is set to at least the size of the largest expected guest
@@ -424,6 +424,16 @@
 #
 #max_core = "unlimited"
 
+# Determine if guest RAM is included in QEMU core dumps. By
+# default guest RAM will be excluded if a new enough QEMU is
+# present. Setting this to '1' will force guest RAM to always
+# be included in QEMU core dumps.
+#
+# This setting will be ignored if the guest XML has set the
+# dumpcore attribute on the  element.
+#
+#dump_guest_core = 1
+
 # mac_filter enables MAC addressed based filtering on bridge ports.
 # This currently requires ebtables to be installed.
 #
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 197537f..944e0b1 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -6927,6 +6927,7 @@ qemuBuildNameCommandLine(virCommandPtr cmd,
 
 static int
 qemuBuildMachineCommandLine(virCommandPtr cmd,
+virQEMUDriverConfigPtr cfg,
 const virDomainDef *def,
 virQEMUCapsPtr qemuCaps)
 {
@@ -6999,17 +7000,22 @@ qemuBuildMachineCommandLine(virCommandPtr cmd,
   virTristateSwitchTypeToString(vmport));
 }
 
-if (def->mem.dump_core) {
-if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DUMP_GUEST_CORE)) {
+if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DUMP_GUEST_CORE)) {
+if (def->mem.dump_core) {
+virBufferAsprintf(, ",dump-guest-core=%s",
+  
virTristateSwitchTypeToString(def->mem.dump_core));
+} else if (cfg->dumpGuestCore != true) {
+virBufferAsprintf(, ",dump-guest-core=%s",
+  cfg->dumpGuestCore ? "on" : "off");
+}
+} else {
+if (def->mem.dump_core) {
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("dump-guest-core is not available "
  "with this QEMU binary"));
 virBufferFreeAndReset();
 return -1;
 }
-
-virBufferAsprintf(, ",dump-guest-core=%s",
-  
virTristateSwitchTypeToString(def->mem.dump_core));
 }
 
 if (def->mem.nosharepages) {
@@ -9365,7 +9371,7 @@ qemuBuildCommandLine(virQEMUDriverPtr driver,
 if (enableFips)
 virCommandAddArg(cmd, "-enable-fips");
 
-if (qemuBuildMachineCommandLine(cmd, def, qemuCaps) < 0)
+if 

Re: [libvirt] [gconfig v2 4/4] config: Add spice host setter

[Christophe Fergeau]

Same minor comments as the previous patch,
Adding some test case to tests/test-gconfig.c would be good to have imo.

Acked-by: Christophe Fergeau 

Christophe

On Mon, Aug 01, 2016 at 11:52:40PM +0300, Visarion Alexandru wrote:
> From: Visarion Alexandru 
> 
> Learn to set the address that spice is listening on.
> 
> We first remove the 'listen' attribute to avoid inconsistencies
> checks between the 'listen' attribute and the 'address'
> attribute of the 'listen' node.
> ---
>  .../libvirt-gconfig-domain-graphics-spice.c| 28 
> ++
>  .../libvirt-gconfig-domain-graphics-spice.h|  3 +++
>  libvirt-gconfig/libvirt-gconfig.sym|  1 +
>  3 files changed, 32 insertions(+)
> 
> diff --git a/libvirt-gconfig/libvirt-gconfig-domain-graphics-spice.c 
> b/libvirt-gconfig/libvirt-gconfig-domain-graphics-spice.c
> index 079ea27..4219ff0 100644
> --- a/libvirt-gconfig/libvirt-gconfig-domain-graphics-spice.c
> +++ b/libvirt-gconfig/libvirt-gconfig-domain-graphics-spice.c
> @@ -157,6 +157,34 @@ void 
> gvir_config_domain_graphics_spice_set_image_compression
>  compression);
>  }
>  
> +/**
> + * gvir_config_domain_graphics_spice_set_listen_address:
> + * @graphics: a #GVirConfigDomainGraphicsSpice
> + * @listens: (in) (element-type LibvirtGConfig.DomainGraphicsListen):
> + *
> + * This method is used to set the various listen nodes a 
> #GVirConfigDomainGraphicsSpice
> + * device can handle.
> +*/
> +void 
> gvir_config_domain_graphics_spice_set_listen_address(GVirConfigDomainGraphicsSpice
>  *graphics,
> +  GList *listens)
> +{
> +GList *it;
> +
> +g_return_if_fail(GVIR_CONFIG_IS_DOMAIN_GRAPHICS_SPICE(graphics));
> +
> +gvir_config_object_remove_attribute (GVIR_CONFIG_OBJECT(graphics),
> + "listen");
> +gvir_config_object_delete_children (GVIR_CONFIG_OBJECT (graphics),
> +"listen", NULL);
> +
> +for (it = listens; it != NULL; it = it->next) {
> +g_return_if_fail(GVIR_CONFIG_IS_DOMAIN_GRAPHICS_LISTEN(it->data));
> +
> +gvir_config_object_attach_add(GVIR_CONFIG_OBJECT(graphics),
> +  GVIR_CONFIG_OBJECT(it->data));
> +}
> +}
> +
>  void gvir_config_domain_graphics_spice_set_gl(GVirConfigDomainGraphicsSpice 
> *graphics,
>gboolean gl)
>  {
> diff --git a/libvirt-gconfig/libvirt-gconfig-domain-graphics-spice.h 
> b/libvirt-gconfig/libvirt-gconfig-domain-graphics-spice.h
> index 25c132e..03abe5b 100644
> --- a/libvirt-gconfig/libvirt-gconfig-domain-graphics-spice.h
> +++ b/libvirt-gconfig/libvirt-gconfig-domain-graphics-spice.h
> @@ -95,6 +95,9 @@ gvir_config_domain_graphics_spice_get_image_compression
>  void gvir_config_domain_graphics_spice_set_gl(GVirConfigDomainGraphicsSpice 
> *graphics,
>gboolean gl);
>  
> +void 
> gvir_config_domain_graphics_spice_set_listen_address(GVirConfigDomainGraphicsSpice
>  *graphics,
> +GList *listens);
> +
>  G_END_DECLS
>  
>  #endif /* __LIBVIRT_GCONFIG_DOMAIN_GRAPHICS_SPICE_H__ */
> diff --git a/libvirt-gconfig/libvirt-gconfig.sym 
> b/libvirt-gconfig/libvirt-gconfig.sym
> index 117a648..73fd977 100644
> --- a/libvirt-gconfig/libvirt-gconfig.sym
> +++ b/libvirt-gconfig/libvirt-gconfig.sym
> @@ -769,6 +769,7 @@ global:
>   gvir_config_domain_graphics_listen_address_new;
>   gvir_config_domain_graphics_listen_address_new_from_xml;
>   gvir_config_domain_graphics_vnc_set_listen_address;
> + gvir_config_domain_graphics_spice_set_listen_address;
>  } LIBVIRT_GCONFIG_0.2.2;
>  
>  #  define new API here using predicted next version number 
> -- 
> 2.7.4
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list


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

Re: [libvirt] [gconfig v2 3/4] config: Add vnc host setter

[Christophe Fergeau]

On Mon, Aug 01, 2016 at 11:52:39PM +0300, Visarion Alexandru wrote:
> From: Visarion Alexandru 
> 
> Learn to set the address that vnc is listening on.
> 
> We first remove the 'listen' attribute to avoid inconsistencies
> checks between the 'listen' attribute and the 'address'
> attribute of the 'listen' node.
> ---
>  .../libvirt-gconfig-domain-graphics-vnc.c  | 28 
> ++
>  .../libvirt-gconfig-domain-graphics-vnc.h  |  3 +++
>  libvirt-gconfig/libvirt-gconfig.sym|  1 +
>  3 files changed, 32 insertions(+)
> 
> diff --git a/libvirt-gconfig/libvirt-gconfig-domain-graphics-vnc.c 
> b/libvirt-gconfig/libvirt-gconfig-domain-graphics-vnc.c
> index fc26bb9..dc7641b 100644
> --- a/libvirt-gconfig/libvirt-gconfig-domain-graphics-vnc.c
> +++ b/libvirt-gconfig/libvirt-gconfig-domain-graphics-vnc.c
> @@ -120,6 +120,34 @@ void 
> gvir_config_domain_graphics_vnc_set_port(GVirConfigDomainGraphicsVnc *graph
> NULL);
>  }
>  
> +/**
> + * gvir_config_domain_graphics_vnc_set_listen_address:
> + * @graphics: a #GVirConfigDomainGraphicsVnc
> + * @listens: (in) (element-type LibvirtGConfig.DomainGraphicsListen):
> + *
> + * This method is used to set the various listen nodes a 
> #GVirConfigDomainGraphicsVnc
> + * device can handle.
> +*/
> +void 
> gvir_config_domain_graphics_vnc_set_listen_address(GVirConfigDomainGraphicsVnc
>  *graphics,
> +GList *listens)
> +{
> +GList *it;
> +
> +g_return_if_fail(GVIR_CONFIG_IS_DOMAIN_GRAPHICS_VNC(graphics));
> +
> +gvir_config_object_remove_attribute (GVIR_CONFIG_OBJECT(graphics),
> +"listen");
> +gvir_config_object_delete_children (GVIR_CONFIG_OBJECT (graphics),
> +"listen", NULL);
> +
> +for (it = listens; it != NULL; it = it->next) {
> +g_return_if_fail(GVIR_CONFIG_IS_DOMAIN_GRAPHICS_LISTEN(it->data));
> +

gvir_config_domain_set_devices() warns but continues trying the other
devices, we probably can do the same here:

if (!GVIR_CONFIG_IS_DOMAIN_GRAPHICS_LISTEN(it->data)) {
g_warn_if_reached();
continue;
}

> +gvir_config_object_attach_add(GVIR_CONFIG_OBJECT(graphics),
> +  GVIR_CONFIG_OBJECT(it->data));
> +}
> +}
> +
>  void 
> gvir_config_domain_graphics_vnc_set_password(GVirConfigDomainGraphicsVnc 
> *graphics,
>const char *password)
>  {
> diff --git a/libvirt-gconfig/libvirt-gconfig-domain-graphics-vnc.h 
> b/libvirt-gconfig/libvirt-gconfig-domain-graphics-vnc.h
> index fe78621..f524dcc 100644
> --- a/libvirt-gconfig/libvirt-gconfig-domain-graphics-vnc.h
> +++ b/libvirt-gconfig/libvirt-gconfig-domain-graphics-vnc.h
> @@ -73,6 +73,9 @@ int 
> gvir_config_domain_graphics_vnc_get_port(GVirConfigDomainGraphicsVnc *graphi
>  void gvir_config_domain_graphics_vnc_set_port(GVirConfigDomainGraphicsVnc 
> *graphics,
>int port);
>  
> +void 
> gvir_config_domain_graphics_vnc_set_listen_address(GVirConfigDomainGraphicsVnc
>  *graphics,
> +GList *listens);

Indentation is off here, you need 4 more spaces.

> +
>  void 
> gvir_config_domain_graphics_vnc_set_password(GVirConfigDomainGraphicsVnc 
> *graphics,
>const char *password);
>  
> diff --git a/libvirt-gconfig/libvirt-gconfig.sym 
> b/libvirt-gconfig/libvirt-gconfig.sym
> index fab6059..117a648 100644
> --- a/libvirt-gconfig/libvirt-gconfig.sym
> +++ b/libvirt-gconfig/libvirt-gconfig.sym
> @@ -768,6 +768,7 @@ global:
>   gvir_config_domain_graphics_listen_address_get_type;
>   gvir_config_domain_graphics_listen_address_new;
>   gvir_config_domain_graphics_listen_address_new_from_xml;
> + gvir_config_domain_graphics_vnc_set_listen_address;
>  } LIBVIRT_GCONFIG_0.2.2;

Be careful to the alphabetical sorting as in the other patches.

Acked-by: Christophe Fergeau 

Christophe


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

Re: [libvirt] [gconfig v2 2/4] Introduce libvirt-gconfig-domain-graphics-listen-address

[Christophe Fergeau]

On Mon, Aug 01, 2016 at 11:52:38PM +0300, Visarion Alexandru wrote:
> From: Visarion Alexandru 
> 
> This is needed to be able to change the address a graphics
> device is listening on.
> ---
>  libvirt-gconfig/Makefile.am|  2 +
>  ...ibvirt-gconfig-domain-graphics-listen-address.c | 79 
> ++
>  ...ibvirt-gconfig-domain-graphics-listen-address.h | 67 ++
>  libvirt-gconfig/libvirt-gconfig.h  |  1 +
>  libvirt-gconfig/libvirt-gconfig.sym|  3 +
>  5 files changed, 152 insertions(+)
>  create mode 100644 
> libvirt-gconfig/libvirt-gconfig-domain-graphics-listen-address.c
>  create mode 100644 
> libvirt-gconfig/libvirt-gconfig-domain-graphics-listen-address.h
> 
> diff --git a/libvirt-gconfig/Makefile.am b/libvirt-gconfig/Makefile.am
> index 27c6df1..6be860b 100644
> --- a/libvirt-gconfig/Makefile.am
> +++ b/libvirt-gconfig/Makefile.am
> @@ -46,6 +46,7 @@ GCONFIG_HEADER_FILES = \
>   libvirt-gconfig-domain-filesys.h \
>   libvirt-gconfig-domain-graphics.h \
>   libvirt-gconfig-domain-graphics-listen.h\
> + libvirt-gconfig-domain-graphics-listen-address.h\
>   libvirt-gconfig-domain-graphics-desktop.h \
>   libvirt-gconfig-domain-graphics-rdp.h \
>   libvirt-gconfig-domain-graphics-sdl.h \
> @@ -140,6 +141,7 @@ GCONFIG_SOURCE_FILES = \
>   libvirt-gconfig-domain-filesys.c \
>   libvirt-gconfig-domain-graphics.c \
>   libvirt-gconfig-domain-graphics-listen.c\
> + libvirt-gconfig-domain-graphics-listen-address.c\
>   libvirt-gconfig-domain-graphics-desktop.c \
>   libvirt-gconfig-domain-graphics-rdp.c \
>   libvirt-gconfig-domain-graphics-sdl.c \
> diff --git a/libvirt-gconfig/libvirt-gconfig-domain-graphics-listen-address.c 
> b/libvirt-gconfig/libvirt-gconfig-domain-graphics-listen-address.c
> new file mode 100644
> index 000..460ca23
> --- /dev/null
> +++ b/libvirt-gconfig/libvirt-gconfig-domain-graphics-listen-address.c
> @@ -0,0 +1,79 @@
> +/*
> + * libvirt-gconfig-domain-graphics-listen-address.c: libvirt domain graphics 
> listen address configuration
> + *
> + * Copyright (C) 2016 Red Hat, Inc.

Same comment about the copyright, this should contain your name.

> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library. If not, see
> + * .
> + *
> + * Author: Visarion Alexandru 
> + */
> +
> +#include 
> +
> +#include "libvirt-gconfig/libvirt-gconfig.h"
> +#include "libvirt-gconfig/libvirt-gconfig-private.h"
> +
> +#define GVIR_CONFIG_DOMAIN_GRAPHICS_LISTEN_ADDRESS_GET_PRIVATE(obj)  
>\
> +(G_TYPE_INSTANCE_GET_PRIVATE((obj), 
> GVIR_CONFIG_TYPE_DOMAIN_GRAPHICS_LISTEN_ADDRESS, 
> GVirConfigDomainGraphicsListenAddressPrivate))
> +
> +struct _GVirConfigDomainGraphicsListenAddressPrivate
> +{
> +gboolean unused;
> +};
> +
> +G_DEFINE_TYPE(GVirConfigDomainGraphicsListenAddress, 
> gvir_config_domain_graphics_listen_address, 
> GVIR_CONFIG_TYPE_DOMAIN_GRAPHICS_LISTEN);
> +
> +
> +static void 
> gvir_config_domain_graphics_listen_address_class_init(GVirConfigDomainGraphicsListenAddressClass
>  *klass)
> +{
> +g_type_class_add_private(klass, 
> sizeof(GVirConfigDomainGraphicsListenAddressPrivate));
> +}
> +
> +
> +static void 
> gvir_config_domain_graphics_listen_address_init(GVirConfigDomainGraphicsListenAddress
>  *address)
> +{
> +address->priv = 
> GVIR_CONFIG_DOMAIN_GRAPHICS_LISTEN_ADDRESS_GET_PRIVATE(address);
> +}
> +
> +
> +GVirConfigDomainGraphicsListenAddress 
> *gvir_config_domain_graphics_listen_address_new(const char *address)
> +{
> +GVirConfigObject *object;
> +
> +object = 
> gvir_config_object_new(GVIR_CONFIG_TYPE_DOMAIN_GRAPHICS_LISTEN_ADDRESS, 
> "listen", NULL);
> +gvir_config_object_set_attribute(object,
> + "type", "address",
> + NULL);
> +gvir_config_object_set_attribute(object,
> + "address", address,
> + NULL);
> +
> +return 

Re: [libvirt] [gconfig v2 1/4] Introduce libvirt-domain-graphics-listen

[Christophe Fergeau]

Hey,

On Mon, Aug 01, 2016 at 11:52:37PM +0300, Visarion Alexandru wrote:
> From: Visarion Alexandru 
> 
> Abstract class which represents a listen child node
> of the graphics device.
> ---
>  libvirt-gconfig/Makefile.am|  2 +
>  .../libvirt-gconfig-domain-graphics-listen.c   | 48 
>  .../libvirt-gconfig-domain-graphics-listen.h   | 64 
> ++
>  libvirt-gconfig/libvirt-gconfig.h  |  1 +
>  libvirt-gconfig/libvirt-gconfig.sym|  1 +
>  5 files changed, 116 insertions(+)
>  create mode 100644 libvirt-gconfig/libvirt-gconfig-domain-graphics-listen.c
>  create mode 100644 libvirt-gconfig/libvirt-gconfig-domain-graphics-listen.h
> 
> diff --git a/libvirt-gconfig/Makefile.am b/libvirt-gconfig/Makefile.am
> index 0400343..27c6df1 100644
> --- a/libvirt-gconfig/Makefile.am
> +++ b/libvirt-gconfig/Makefile.am
> @@ -45,6 +45,7 @@ GCONFIG_HEADER_FILES = \
>   libvirt-gconfig-domain-disk-driver.h \
>   libvirt-gconfig-domain-filesys.h \
>   libvirt-gconfig-domain-graphics.h \
> + libvirt-gconfig-domain-graphics-listen.h\
>   libvirt-gconfig-domain-graphics-desktop.h \
>   libvirt-gconfig-domain-graphics-rdp.h \
>   libvirt-gconfig-domain-graphics-sdl.h \
> @@ -138,6 +139,7 @@ GCONFIG_SOURCE_FILES = \
>   libvirt-gconfig-domain-disk-driver.c \
>   libvirt-gconfig-domain-filesys.c \
>   libvirt-gconfig-domain-graphics.c \
> + libvirt-gconfig-domain-graphics-listen.c\
>   libvirt-gconfig-domain-graphics-desktop.c \
>   libvirt-gconfig-domain-graphics-rdp.c \
>   libvirt-gconfig-domain-graphics-sdl.c \
> diff --git a/libvirt-gconfig/libvirt-gconfig-domain-graphics-listen.c 
> b/libvirt-gconfig/libvirt-gconfig-domain-graphics-listen.c
> new file mode 100644
> index 000..c89f126
> --- /dev/null
> +++ b/libvirt-gconfig/libvirt-gconfig-domain-graphics-listen.c
> @@ -0,0 +1,48 @@
> +/*
> + * libvirt-gconfig-domain-graphics-listen.c: libvirt domain graphics listen 
> base class
> + *


Since you write this code and not Red Hat, this should read:

> + * Copyright (C) 2016 Visarion Alexandru 

If you copied the code from another file and want to keep the copyright
history, just keep the previous (C) line, and add another one with your
name on it.


> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library. If not, see
> + * .
> + *
> + * Author: Visarion Alexandru 
> + */
> +
> +#include 
> +
> +#include "libvirt-gconfig/libvirt-gconfig.h"
> +#include "libvirt-gconfig/libvirt-gconfig-private.h"
> +
> +#define GVIR_CONFIG_DOMAIN_GRAPHICS_LISTEN_GET_PRIVATE(obj)  
>\
> +(G_TYPE_INSTANCE_GET_PRIVATE((obj), 
> GVIR_CONFIG_TYPE_DOMAIN_GRAPHICS_LISTEN, 
> GVirConfigDomainGraphicsListenPrivate))
> +
> +struct _GVirConfigDomainGraphicsListenPrivate
> +{
> +gboolean unused;
> +};
> +
> +G_DEFINE_ABSTRACT_TYPE(GVirConfigDomainGraphicsListen, 
> gvir_config_domain_graphics_listen, GVIR_CONFIG_TYPE_OBJECT);
> +
> +
> +static void 
> gvir_config_domain_graphics_listen_class_init(GVirConfigDomainGraphicsListenClass
>  *klass)
> +{
> +g_type_class_add_private(klass, 
> sizeof(GVirConfigDomainGraphicsListenPrivate));
> +}
> +
> +
> +static void 
> gvir_config_domain_graphics_listen_init(GVirConfigDomainGraphicsListen 
> *listen)
> +{
> +listen->priv = GVIR_CONFIG_DOMAIN_GRAPHICS_LISTEN_GET_PRIVATE(listen);
> +}
> diff --git a/libvirt-gconfig/libvirt-gconfig-domain-graphics-listen.h 
> b/libvirt-gconfig/libvirt-gconfig-domain-graphics-listen.h
> new file mode 100644
> index 000..dba3811
> --- /dev/null
> +++ b/libvirt-gconfig/libvirt-gconfig-domain-graphics-listen.h
> @@ -0,0 +1,64 @@
> +/*
> + * libvirt-gconfig-domain-graphics-listen.h: libvirt domain graphics listen 
> base class
> + *
> + * Copyright (C) 2016 Red Hat, Inc.
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software 

Re: [libvirt] [PATCH 08/10] qemu: domain: Simplify return values of qemuDomainRefreshVcpuInfo

[Pavel Hrdina]

On Wed, Aug 03, 2016 at 09:03:50AM -0400, John Ferlan wrote:
> 
> 
> On 08/03/2016 04:11 AM, Peter Krempa wrote:
> > Call the vcpu thread info validation separately to decrease complexity
> > of returned values by qemuDomainRefreshVcpuInfo.
> > 
> > This function now returns 0 on success and -1 on error. Certain
> > failures of qemu to report data are still considered as success. Any
> > error reported now is fatal.
> > ---
> >  src/qemu/qemu_domain.c  | 16 +---
> >  src/qemu/qemu_driver.c  | 20 ++--
> >  src/qemu/qemu_process.c |  6 ++
> >  3 files changed, 21 insertions(+), 21 deletions(-)
> > 
> > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> > index f27f2f7..4cdd012 100644
> > --- a/src/qemu/qemu_domain.c
> > +++ b/src/qemu/qemu_domain.c
> > @@ -5671,10 +5671,10 @@ qemuDomainValidateVcpuInfo(virDomainObjPtr vm)
> >   * @vm: domain object
> >   * @asyncJob: current asynchronous job type
> >   *
> > - * Updates vCPU information private data of @vm.
> > + * Updates vCPU information private data of @vm. Due to historical reasons 
> > this
> > + * function returns success even if some data were not reported by qemu.
> >   *
> > - * Returns number of detected vCPU threads on success, -1 on error and 
> > reports
> > - * an appropriate error, -2 if the domain doesn't exist any more.
> > + * Returns 0 on success and -1 on fatal error.
> >   */
> >  int
> >  qemuDomainRefreshVcpuInfo(virQEMUDriverPtr driver,
> > @@ -5722,10 +5722,7 @@ qemuDomainRefreshVcpuInfo(virQEMUDriverPtr driver,
> >  if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
> >  return -1;
> >  ncpupids = qemuMonitorGetCPUInfo(qemuDomainGetMonitor(vm), );
> > -if (qemuDomainObjExitMonitor(driver, vm) < 0) {
> > -ret = -2;
> > -goto cleanup;
> > -}
> > +if (qemuDomainObjExitMonitor(driver, vm) < 0)
> 
> This is missing a "goto cleanup" which is added in the next patch, but
> needs to be here!
> 
> > 
> >  /* failure to get the VCPU <-> PID mapping or to execute the query
> >   * command will not be treated fatal as some versions of qemu don't
> > @@ -5745,10 +5742,7 @@ qemuDomainRefreshVcpuInfo(virQEMUDriverPtr driver,
> >  QEMU_DOMAIN_VCPU_PRIVATE(vcpu)->tid = 0;
> >  }
> > 
> > -if (qemuDomainValidateVcpuInfo(vm) < 0)
> > -goto cleanup;
> > -
> > -ret = ncpupids;
> > +ret = 0;
> > 
> >   cleanup:
> >  VIR_FREE(cpupids);
> > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> > index 453572b..2199258 100644
> > --- a/src/qemu/qemu_driver.c
> > +++ b/src/qemu/qemu_driver.c
> > @@ -4615,6 +4615,7 @@ qemuDomainHotplugAddVcpu(virQEMUDriverPtr driver,
> >  {
> >  qemuDomainObjPrivatePtr priv = vm->privateData;
> >  virDomainVcpuDefPtr vcpuinfo = virDomainDefGetVcpu(vm->def, vcpu);
> > +qemuDomainVcpuPrivatePtr vcpupriv = QEMU_DOMAIN_VCPU_PRIVATE(vcpuinfo);
> >  int ret = -1;
> >  int rc;
> >  int oldvcpus = virDomainDefGetVcpus(vm->def);
> > @@ -4639,15 +4640,14 @@ qemuDomainHotplugAddVcpu(virQEMUDriverPtr driver,
> > 
> >  vcpuinfo->online = true;
> > 
> > -if ((rc = qemuDomainRefreshVcpuInfo(driver, vm, QEMU_ASYNC_JOB_NONE)) 
> > <= 0) {
> > -/* vcpu pids were not detected, skip setting of affinity */
> > -if (rc == 0)
> > -ret = 0;
> > +if (qemuDomainRefreshVcpuInfo(driver, vm, QEMU_ASYNC_JOB_NONE) < 0)
> > +goto cleanup;
> > 
> > +if (qemuDomainValidateVcpuInfo(vm) < 0)
> >  goto cleanup;
> > -}
> > 
> > -if (qemuProcessSetupVcpu(vm, vcpu) < 0)
> > +if (vcpupriv->tid > 0 &&
> > +qemuProcessSetupVcpu(vm, vcpu) < 0)
> >  goto cleanup;
> > 
> >  ret = 0;
> > @@ -4689,11 +4689,11 @@ qemuDomainHotplugDelVcpu(virQEMUDriverPtr driver,
> >  goto cleanup;
> >  }
> > 
> > -if ((rc = qemuDomainRefreshVcpuInfo(driver, vm, QEMU_ASYNC_JOB_NONE)) 
> > < 0) {
> > -/* rollback only if domain didn't exit */
> > -if (rc == -2)
> > -goto cleanup;
> > +if (qemuDomainRefreshVcpuInfo(driver, vm, QEMU_ASYNC_JOB_NONE) < 0)
> > +goto cleanup;
> 
> Typing what I'm thinking - just to be sure it matches your expectations
> of this code. So we no longer care to distinguish between failure and
> failure due to domain exit. This just seems to mean we'll get that audit
> after a possible domain exit and reset the vcpuinfo->online (won't
> really matter if the domain exited).

The logic is still the same as before this patch.  Function
qemuDomainRefreshVcpuInfo fails with error only in case the domain exited.
It's safe to rollback only if qemuDomainValidateVcpuInfo fails, this is what
was extracted from qemuDomainRefreshVcpuInfo.

> 
> ACK - with the goto cleanup added at least.
> 
> John
> > 
> > +if (qemuDomainValidateVcpuInfo(vm) < 0) {
> > +/* rollback vcpu count if the setting has failed */
> >  virDomainAuditVcpu(vm, 

Re: [libvirt] [PATCH v2 2/8] conf: add node_device_event handling

[John Ferlan]

On 07/28/2016 08:02 AM, Jovanka Gulicoska wrote:
> Add node device event handling infrastructure to node_device_event.[ch]
> ---
>  src/Makefile.am  |   5 +
>  src/conf/node_device_event.c | 234 
> +++
>  src/conf/node_device_event.h |  59 +++
>  src/libvirt_private.syms |   5 +
>  4 files changed, 303 insertions(+)
>  create mode 100644 src/conf/node_device_event.c
>  create mode 100644 src/conf/node_device_event.h
> 

[...]

> diff --git a/src/conf/node_device_event.c b/src/conf/node_device_event.c
> new file mode 100644
> index 000..61bc912

[...]

> +
> +/**
> + * virNodeDeviceEventLifecycleNew:
> + * @name: name of the node device object the event describes
> + * @type: type of lifecycle event
> + * @detail: more details about @type
> + *
> + * Create a new node device lifecycle event.
> + */
> +virObjectEventPtr
> +virNodeDeviceEventLifecycleNew(const char *name,
> +   int type,
> +   int detail)
> +{
> +virNodeDeviceEventLifecyclePtr event;
> +
> +if (virNodeDeviceEventsInitialize() < 0)
> +return NULL;
> +
> +if (!(event = virObjectEventNew(virNodeDeviceEventLifecycleClass,
> +virNodeDeviceEventDispatchDefaultFunc,
> +VIR_NODE_DEVICE_EVENT_ID_LIFECYCLE,
> +0, name, NULL, name)))


This has caused a Coverity build failure since the prototype has:

ATTRIBUTE_NONNULL(6)

It gets even worse in the function and needs to be resolved before the
"next" release.

John

> +return NULL;
> +
> +event->type = type;
> +event->detail = detail;
> +
> +return (virObjectEventPtr)event;
> +}

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

Re: [libvirt] [PATCH 09/10] qemu: monitor: Return structures from qemuMonitorGetCPUInfo

[John Ferlan]

On 08/03/2016 04:11 AM, Peter Krempa wrote:
> The function will gradually add more returned data. Return a struct for
> every vCPU containing the data.
> ---
>  src/qemu/qemu_domain.c  | 26 ++-
>  src/qemu/qemu_monitor.c | 56 
> ++---
>  src/qemu/qemu_monitor.h | 13 +++-
>  3 files changed, 72 insertions(+), 23 deletions(-)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 4cdd012..9d389f7 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -5682,10 +5682,11 @@ qemuDomainRefreshVcpuInfo(virQEMUDriverPtr driver,
>int asyncJob)
>  {
>  virDomainVcpuDefPtr vcpu;
> +qemuDomainVcpuPrivatePtr vcpupriv;
> +qemuMonitorCPUInfoPtr info = NULL;
>  size_t maxvcpus = virDomainDefGetVcpusMax(vm->def);
> -pid_t *cpupids = NULL;
> -int ncpupids;
>  size_t i;
> +int rc;
>  int ret = -1;
> 
>  /*
> @@ -5721,31 +5722,26 @@ qemuDomainRefreshVcpuInfo(virQEMUDriverPtr driver,
> 
>  if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
>  return -1;
> -ncpupids = qemuMonitorGetCPUInfo(qemuDomainGetMonitor(vm), );
> +
> +rc = qemuMonitorGetCPUInfo(qemuDomainGetMonitor(vm), , maxvcpus);
> +
>  if (qemuDomainObjExitMonitor(driver, vm) < 0)
> +goto cleanup;
> 
> -/* failure to get the VCPU <-> PID mapping or to execute the query
> - * command will not be treated fatal as some versions of qemu don't
> - * support this command */
> -if (ncpupids <= 0) {
> -virResetLastError();
> -ret = 0;
> +if (rc < 0)
>  goto cleanup;
> -}
> 
>  for (i = 0; i < maxvcpus; i++) {
>  vcpu = virDomainDefGetVcpu(vm->def, i);
> +vcpupriv = QEMU_DOMAIN_VCPU_PRIVATE(vcpu);
> 
> -if (i < ncpupids)
> -QEMU_DOMAIN_VCPU_PRIVATE(vcpu)->tid = cpupids[i];
> -else
> -QEMU_DOMAIN_VCPU_PRIVATE(vcpu)->tid = 0;
> +vcpupriv->tid = info[i].tid;
>  }

FWIW:
Once we reach this point the VIR_DOMAIN_VIRT_QEMU check is the only way
qemuDomainHasVcpuPids can return 0 in qemuDomainValidateVcpuInfo... So I
wonder - would it be useful to alter that function too so that we're
sure things match up properly vis-a-vis the 'tid' and online checks?
And my alter, I was thinking along the lines of a similar check for
VIR_DOMAIN_VIRT_QEMU...

> 
>  ret = 0;
> 
>   cleanup:
> -VIR_FREE(cpupids);
> +qemuMonitorCPUInfoFree(info, maxvcpus);
>  return ret;
>  }
> 
> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
> index 4403bdd..0011ceb 100644
> --- a/src/qemu/qemu_monitor.c
> +++ b/src/qemu/qemu_monitor.c
> @@ -1656,25 +1656,67 @@ qemuMonitorSystemReset(qemuMonitorPtr mon)
>  }
> 
> 
> +void
> +qemuMonitorCPUInfoFree(qemuMonitorCPUInfoPtr cpus,
> +   size_t ncpus ATTRIBUTE_UNUSED)
> +{
> +if (!cpus)
> +return;
> +
> +VIR_FREE(cpus);
> +}
> +
> +
>  /**
>   * qemuMonitorGetCPUInfo:
>   * @mon: monitor
> - * @pids: returned array of thread ids corresponding to the vCPUs
> + * @cpus: pointer filled by array of qemuMonitorCPUInfo structures
> + * @maxvcpus: total possible number of vcpus
> + *
> + * Detects VCPU information. If qemu doesn't support or fails reporting
> + * information this function will return success as other parts of libvirt
> + * are able to cope with that.
>   *
> - * Detects the vCPU thread ids. Returns count of detected vCPUs on success,
> - * 0 if qemu didn't report thread ids (does not report libvirt error),
> - * -1 on error (reports libvirt error).
> + * Returns 0 on success (including if qemu didn't report any data) and
> + *  -1 on error (reports libvirt error).
>   */
>  int
>  qemuMonitorGetCPUInfo(qemuMonitorPtr mon,
> -  int **pids)
> +  qemuMonitorCPUInfoPtr *vcpus,
> +  size_t maxvcpus)
>  {
> +qemuMonitorCPUInfoPtr info = NULL;
> +int *pids = NULL;
> +size_t i;
> +int ret = -1;
> +int rc;
> +
>  QEMU_CHECK_MONITOR(mon);
> 
> +if (VIR_ALLOC_N(info, maxvcpus) < 0)
> +return -1;
> +
>  if (mon->json)
> -return qemuMonitorJSONQueryCPUs(mon, pids);
> +rc = qemuMonitorJSONQueryCPUs(mon, );
>  else
> -return qemuMonitorTextQueryCPUs(mon, pids);
> +rc = qemuMonitorTextQueryCPUs(mon, );
> +
> +if (rc < 0) {

If qemuMonitorJSONExtractCPUInfo() finds ncpus == 0, then it returns an
error which won't be "cleaned up" unless rc <= 0

> +virResetLastError();
> +ret = 0;
> +goto cleanup;

So, ret == 0, we go to cleanup, it cleans up 'info', then our caller
finds rc == 0, so it can fall into that for maxvcpus loop looking 'info'
and then of course freeing 'info' again.

> +}
> +
> +for (i = 0; i < rc; i++)
> +info[i].tid = pids[i];

Still no "gaps" in our "internal" pids list which is fine, 

Re: [libvirt] [PATCH 10/10] qemu: monitor: Return struct from qemuMonitor(Text|Json)QueryCPUs

[Jiri Denemark]

On Wed, Aug 03, 2016 at 10:11:02 +0200, Peter Krempa wrote:
> Prepare to extract more data by returning a array of structs rather than

s/a array/an array/

Jirka

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

Re: [libvirt] [PATCH 01/10] qemu: monitor: Add monitor API for device_add supporting JSON objects

[Ján Tomko]

On Wed, Aug 03, 2016 at 10:10:53AM +0200, Peter Krempa wrote:

Rather than formating a string and splitting it back to a JSON object


s/formating/formatting/

Jan


add API that will take a JSON object directly.
---
src/qemu/qemu_monitor.c  | 18 ++
src/qemu/qemu_monitor.h  |  2 ++
src/qemu/qemu_monitor_json.c | 29 +++--
src/qemu/qemu_monitor_json.h |  2 ++
4 files changed, 41 insertions(+), 10 deletions(-)



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

Re: [libvirt] [PATCH 08/10] qemu: domain: Simplify return values of qemuDomainRefreshVcpuInfo

[John Ferlan]

On 08/03/2016 04:11 AM, Peter Krempa wrote:
> Call the vcpu thread info validation separately to decrease complexity
> of returned values by qemuDomainRefreshVcpuInfo.
> 
> This function now returns 0 on success and -1 on error. Certain
> failures of qemu to report data are still considered as success. Any
> error reported now is fatal.
> ---
>  src/qemu/qemu_domain.c  | 16 +---
>  src/qemu/qemu_driver.c  | 20 ++--
>  src/qemu/qemu_process.c |  6 ++
>  3 files changed, 21 insertions(+), 21 deletions(-)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index f27f2f7..4cdd012 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -5671,10 +5671,10 @@ qemuDomainValidateVcpuInfo(virDomainObjPtr vm)
>   * @vm: domain object
>   * @asyncJob: current asynchronous job type
>   *
> - * Updates vCPU information private data of @vm.
> + * Updates vCPU information private data of @vm. Due to historical reasons 
> this
> + * function returns success even if some data were not reported by qemu.
>   *
> - * Returns number of detected vCPU threads on success, -1 on error and 
> reports
> - * an appropriate error, -2 if the domain doesn't exist any more.
> + * Returns 0 on success and -1 on fatal error.
>   */
>  int
>  qemuDomainRefreshVcpuInfo(virQEMUDriverPtr driver,
> @@ -5722,10 +5722,7 @@ qemuDomainRefreshVcpuInfo(virQEMUDriverPtr driver,
>  if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
>  return -1;
>  ncpupids = qemuMonitorGetCPUInfo(qemuDomainGetMonitor(vm), );
> -if (qemuDomainObjExitMonitor(driver, vm) < 0) {
> -ret = -2;
> -goto cleanup;
> -}
> +if (qemuDomainObjExitMonitor(driver, vm) < 0)

This is missing a "goto cleanup" which is added in the next patch, but
needs to be here!

> 
>  /* failure to get the VCPU <-> PID mapping or to execute the query
>   * command will not be treated fatal as some versions of qemu don't
> @@ -5745,10 +5742,7 @@ qemuDomainRefreshVcpuInfo(virQEMUDriverPtr driver,
>  QEMU_DOMAIN_VCPU_PRIVATE(vcpu)->tid = 0;
>  }
> 
> -if (qemuDomainValidateVcpuInfo(vm) < 0)
> -goto cleanup;
> -
> -ret = ncpupids;
> +ret = 0;
> 
>   cleanup:
>  VIR_FREE(cpupids);
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 453572b..2199258 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -4615,6 +4615,7 @@ qemuDomainHotplugAddVcpu(virQEMUDriverPtr driver,
>  {
>  qemuDomainObjPrivatePtr priv = vm->privateData;
>  virDomainVcpuDefPtr vcpuinfo = virDomainDefGetVcpu(vm->def, vcpu);
> +qemuDomainVcpuPrivatePtr vcpupriv = QEMU_DOMAIN_VCPU_PRIVATE(vcpuinfo);
>  int ret = -1;
>  int rc;
>  int oldvcpus = virDomainDefGetVcpus(vm->def);
> @@ -4639,15 +4640,14 @@ qemuDomainHotplugAddVcpu(virQEMUDriverPtr driver,
> 
>  vcpuinfo->online = true;
> 
> -if ((rc = qemuDomainRefreshVcpuInfo(driver, vm, QEMU_ASYNC_JOB_NONE)) <= 
> 0) {
> -/* vcpu pids were not detected, skip setting of affinity */
> -if (rc == 0)
> -ret = 0;
> +if (qemuDomainRefreshVcpuInfo(driver, vm, QEMU_ASYNC_JOB_NONE) < 0)
> +goto cleanup;
> 
> +if (qemuDomainValidateVcpuInfo(vm) < 0)
>  goto cleanup;
> -}
> 
> -if (qemuProcessSetupVcpu(vm, vcpu) < 0)
> +if (vcpupriv->tid > 0 &&
> +qemuProcessSetupVcpu(vm, vcpu) < 0)
>  goto cleanup;
> 
>  ret = 0;
> @@ -4689,11 +4689,11 @@ qemuDomainHotplugDelVcpu(virQEMUDriverPtr driver,
>  goto cleanup;
>  }
> 
> -if ((rc = qemuDomainRefreshVcpuInfo(driver, vm, QEMU_ASYNC_JOB_NONE)) < 
> 0) {
> -/* rollback only if domain didn't exit */
> -if (rc == -2)
> -goto cleanup;
> +if (qemuDomainRefreshVcpuInfo(driver, vm, QEMU_ASYNC_JOB_NONE) < 0)
> +goto cleanup;

Typing what I'm thinking - just to be sure it matches your expectations
of this code. So we no longer care to distinguish between failure and
failure due to domain exit. This just seems to mean we'll get that audit
after a possible domain exit and reset the vcpuinfo->online (won't
really matter if the domain exited).

ACK - with the goto cleanup added at least.

John
> 
> +if (qemuDomainValidateVcpuInfo(vm) < 0) {
> +/* rollback vcpu count if the setting has failed */
>  virDomainAuditVcpu(vm, oldvcpus, oldvcpus - 1, "update", false);
>  vcpuinfo->online = true;
>  goto cleanup;
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 9136ba5..2e405af 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -5193,6 +5193,9 @@ qemuProcessLaunch(virConnectPtr conn,
>  if (qemuDomainRefreshVcpuInfo(driver, vm, asyncJob) < 0)
>  goto cleanup;
> 
> +if (qemuDomainValidateVcpuInfo(vm) < 0)
> +goto cleanup;
> +
>  VIR_DEBUG("Detecting IOThread PIDs");
>  if 

Re: [libvirt] [PATCH 07/10] qemu: domain: Improve vCPU data checking in qemuDomainDetectVcpuPids

[Pavel Hrdina]

On Wed, Aug 03, 2016 at 10:10:59AM +0200, Peter Krempa wrote:

s/qemuDomainDetectVcpuPids/qemuDomainRefreshVcpu/ in $subject

> Validate the presence of the thread id according to state of the vCPU
> rather than just checking the vCPU count. Additionally put the new
> validation code into a separate function so that the information
> retrieval can be split from the validation.
> ---
>  src/qemu/qemu_domain.c | 49 +++--
>  src/qemu/qemu_domain.h |  1 +
>  2 files changed, 44 insertions(+), 6 deletions(-)
> 

ACK

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

Re: [libvirt] [PATCH 07/10] qemu: domain: Improve vCPU data checking in qemuDomainDetectVcpuPids

[John Ferlan]

$SUBJ
s/qemuDomainDetectVcpuPids/qemuDomainRefreshVcpuInfo/

On 08/03/2016 04:10 AM, Peter Krempa wrote:
> Validate the presence of the thread id according to state of the vCPU
> rather than just checking the vCPU count. Additionally put the new
> validation code into a separate function so that the information
> retrieval can be split from the validation.
> ---
>  src/qemu/qemu_domain.c | 49 +++--
>  src/qemu/qemu_domain.h |  1 +
>  2 files changed, 44 insertions(+), 6 deletions(-)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 5cde841..f27f2f7 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -5624,6 +5624,48 @@ qemuDomainGetVcpuPid(virDomainObjPtr vm,
> 
> 
>  /**
> + * qemuDomainValidateVcpuInfo:
> + *
> + * Validates vcpu thread information. If vcpu thread IDs are reported by 
> qemu,
> + * this function validates that online vcpus have thread info present and
> + * offline vcpus don't.
> + *
> + * Returns 0 on success -1 on error.
> + */
> +int
> +qemuDomainValidateVcpuInfo(virDomainObjPtr vm)
> +{
> +size_t maxvcpus = virDomainDefGetVcpusMax(vm->def);
> +virDomainVcpuDefPtr vcpu;
> +qemuDomainVcpuPrivatePtr vcpupriv;
> +size_t i;
> +
> +if (!qemuDomainHasVcpuPids(vm))
> +return 0;

This is checking priv->nvcpupids > 0, which we know as a truth since a
few lines before calling this function there's a check:

 /* failure to get the VCPU <-> PID mapping or to execute the query
  * command will not be treated fatal as some versions of qemu don't
  * support this command */
 if (ncpupids <= 0) {

Of course future patches "adjust" where this function is called to split
the Refresh and Validate into two parts and by patch 9 those lines
disappear. I guess it's just duplicitous for a few patches.

Still, by the time we reach patch 9 other than the VIR_DOMAIN_VIRT_QEMU
check - I wonder if we may want to make the subsequent checks since
there'd be no other way to get here since if the Refresh function
returns failure, this function wouldn't be called.

ACK to what's here with the adjustment for the commit message... maybe
I'm thinking too hard for this early in the morning ;-)


John


> +
> +for (i = 0; i < maxvcpus; i++) {
> +vcpu = virDomainDefGetVcpu(vm->def, i);
> +vcpupriv = QEMU_DOMAIN_VCPU_PRIVATE(vcpu);
> +
> +if (vcpu->online && vcpupriv->tid == 0) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> +   _("qemu didn't report thread id for vcpu '%zu'"), 
> i);
> +return -1;
> +}
> +
> +if (!vcpu->online && vcpupriv->tid != 0) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> +   _("qemu reported thread if for inactive vcpu 
> '%zu'"),
> +   i);
> +return -1;
> +}
> +}
> +
> +return 0;
> +}
> +
> +
> +/**
>   * qemuDomainRefreshVcpuInfo:
>   * @driver: qemu driver data
>   * @vm: domain object
> @@ -5703,13 +5745,8 @@ qemuDomainRefreshVcpuInfo(virQEMUDriverPtr driver,
>  QEMU_DOMAIN_VCPU_PRIVATE(vcpu)->tid = 0;
>  }
> 
> -if (ncpupids != virDomainDefGetVcpus(vm->def)) {
> -virReportError(VIR_ERR_INTERNAL_ERROR,
> -   _("got wrong number of vCPU pids from QEMU monitor. "
> - "got %d, wanted %d"),
> -   ncpupids, virDomainDefGetVcpus(vm->def));
> +if (qemuDomainValidateVcpuInfo(vm) < 0)
>  goto cleanup;
> -}
> 
>  ret = ncpupids;
> 
> diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
> index 3193427..0613093 100644
> --- a/src/qemu/qemu_domain.h
> +++ b/src/qemu/qemu_domain.h
> @@ -647,6 +647,7 @@ int qemuDomainDefValidateMemoryHotplug(const virDomainDef 
> *def,
> 
>  bool qemuDomainHasVcpuPids(virDomainObjPtr vm);
>  pid_t qemuDomainGetVcpuPid(virDomainObjPtr vm, unsigned int vcpuid);
> +int qemuDomainValidateVcpuInfo(virDomainObjPtr vm);
>  int qemuDomainRefreshVcpuInfo(virQEMUDriverPtr driver,
>virDomainObjPtr vm,
>int asyncJob);
> 

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

Re: [libvirt] [PATCH 02/10] internal: Introduce macro for stealing pointers

[John Ferlan]

On 08/03/2016 04:10 AM, Peter Krempa wrote:
> VIR_STEAL copies the second argument into the first and then sets it to
> NULL. This is useful for stealing pointers.
> ---
>  src/internal.h | 12 
>  1 file changed, 12 insertions(+)
> 
> diff --git a/src/internal.h b/src/internal.h
> index 0dc34c7..c633ee6 100644
> --- a/src/internal.h
> +++ b/src/internal.h
> @@ -307,6 +307,18 @@
>  } while (0)
> 
>  /**
> + * VIR_STEAL:
> + *
> + * Steals pointer passed as second argument into the first argument. Second
> + * argument must not have side effects.
> + */
> +# define VIR_STEAL(a, b)  \
> +do { \
> +(a) = (b); \
> +(b) = NULL;\
> +} while (0)
> +
> +/**
>   * virCheckFlags:
>   * @supported: an OR'ed set of supported flags
>   * @retval: return value in case unsupported flags were passed
> 

While one can look at the code and deduce what it does - the name is not
that descriptive.  VIR_ASSIGN_PTR or VIR_ASSIGN_POINTER?  Just a thought...

It's also not used until patch 9 - perhaps move it a bit closer to that.

John

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

Re: [libvirt] [PATCH 06/10] qemu: monitor: Rename qemuMonitor(JSON|Text)GetCPUInfo

[Pavel Hrdina]

On Wed, Aug 03, 2016 at 10:10:58AM +0200, Peter Krempa wrote:
> Use a name that contains the command used to get the information.
> ---
>  src/qemu/qemu_monitor.c  | 4 ++--
>  src/qemu/qemu_monitor_json.c | 5 +++--
>  src/qemu/qemu_monitor_json.h | 4 ++--
>  src/qemu/qemu_monitor_text.c | 5 +++--
>  src/qemu/qemu_monitor_text.h | 4 ++--
>  tests/qemumonitorjsontest.c  | 6 +++---
>  6 files changed, 15 insertions(+), 13 deletions(-)
> 

ACK

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

Re: [libvirt] [PATCH 01/10] qemu: monitor: Add monitor API for device_add supporting JSON objects

[Pavel Hrdina]

On Wed, Aug 03, 2016 at 12:43:48PM +0200, Pavel Hrdina wrote:
> On Wed, Aug 03, 2016 at 10:10:53AM +0200, Peter Krempa wrote:
> > Rather than formating a string and splitting it back to a JSON object
> > add API that will take a JSON object directly.
> > ---
> >  src/qemu/qemu_monitor.c  | 18 ++
> >  src/qemu/qemu_monitor.h  |  2 ++
> >  src/qemu/qemu_monitor_json.c | 29 +++--
> >  src/qemu/qemu_monitor_json.h |  2 ++
> >  4 files changed, 41 insertions(+), 10 deletions(-)
> > 
> > diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
> > index 58c04d5..b58c412 100644
> > --- a/src/qemu/qemu_monitor.c
> > +++ b/src/qemu/qemu_monitor.c
> > @@ -2771,6 +2771,24 @@ qemuMonitorAddDevice(qemuMonitorPtr mon,
> > 
> > 
> >  /**
> > + * qemuMonitorAddDeviceArgs:
> > + * @mon: monitor object
> > + * @args: arguments for device add, consumed on success or failure
> > + *
> > + * Adds a device described by @args. Requires JSON monitor.
> > + * Returns 0 on success -1 on error.
> > + */
> > +int
> > +qemuMonitorAddDeviceArgs(qemuMonitorPtr mon,
> > + virJSONValuePtr args)
> > +{
> > +QEMU_CHECK_MONITOR_JSON(mon);
> > +
> > +return qemuMonitorJSONAddDeviceArgs(mon, args);
> > +}
> > +
> > +
> > +/**
> >   * qemuMonitorAddObject:
> >   * @mon: Pointer to monitor object
> >   * @type: Type name of object to add
> > diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
> > index ae0954d..805656b 100644
> > --- a/src/qemu/qemu_monitor.h
> > +++ b/src/qemu/qemu_monitor.h
> > @@ -685,6 +685,8 @@ int qemuMonitorAttachPCIDiskController(qemuMonitorPtr 
> > mon,
> > const char *bus,
> > virPCIDeviceAddress *guestAddr);
> > 
> > +int qemuMonitorAddDeviceArgs(qemuMonitorPtr mon,
> > + virJSONValuePtr args);

I've missed this one, wrong indentation.

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

Re: [libvirt] [PATCH 05/10] qemu: domain: Rename qemuDomainDetectVcpuPids to qemuDomainRefreshVcpuInfo

[Pavel Hrdina]

On Wed, Aug 03, 2016 at 10:10:57AM +0200, Peter Krempa wrote:
> The function will eventually do more useful stuff than just detection of
> thread ids.
> ---
>  src/qemu/qemu_domain.c  | 10 +-
>  src/qemu/qemu_domain.h  |  5 +++--
>  src/qemu/qemu_driver.c  |  4 ++--
>  src/qemu/qemu_process.c |  6 +++---
>  4 files changed, 13 insertions(+), 12 deletions(-)
> 

ACK

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

Re: [libvirt] [PATCH 04/10] qemu: Improve error message in virDomainGetVcpus

[Pavel Hrdina]

On Wed, Aug 03, 2016 at 10:10:56AM +0200, Peter Krempa wrote:
> If the VM is offline we can't retrieve the runtime statistical
> information. Pinning could be retrieved but there are separate APIs for
> that.
> ---
>  src/qemu/qemu_driver.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 

ACK

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

Re: [libvirt] [PATCH 03/10] qemu: monitor: Add do-while block to QEMU_CHECK_MONITOR_FULL

[Pavel Hrdina]

On Wed, Aug 03, 2016 at 10:10:55AM +0200, Peter Krempa wrote:
> Assure that it's just one statement to avoid problems when used with
> conditions.
> ---
>  src/qemu/qemu_monitor.c | 25 ++---
>  1 file changed, 14 insertions(+), 11 deletions(-)
> 

ACK

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

Re: [libvirt] [PATCH 02/10] internal: Introduce macro for stealing pointers

[Pavel Hrdina]

On Wed, Aug 03, 2016 at 10:10:54AM +0200, Peter Krempa wrote:
> VIR_STEAL copies the second argument into the first and then sets it to
> NULL. This is useful for stealing pointers.
> ---
>  src/internal.h | 12 
>  1 file changed, 12 insertions(+)
> 
> diff --git a/src/internal.h b/src/internal.h
> index 0dc34c7..c633ee6 100644
> --- a/src/internal.h
> +++ b/src/internal.h
> @@ -307,6 +307,18 @@
>  } while (0)
> 
>  /**
> + * VIR_STEAL:
> + *
> + * Steals pointer passed as second argument into the first argument. Second
> + * argument must not have side effects.
> + */
> +# define VIR_STEAL(a, b)  \
> +do { \
> +(a) = (b); \
> +(b) = NULL;\
> +} while (0)

I guess that there is no harm having this macro even though it's only two lines
of code.

I would align the backslash to the same column as we tend to do in libvirt code.

ACK

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

Re: [libvirt] [PATCH 01/10] qemu: monitor: Add monitor API for device_add supporting JSON objects

[Pavel Hrdina]

On Wed, Aug 03, 2016 at 10:10:53AM +0200, Peter Krempa wrote:
> Rather than formating a string and splitting it back to a JSON object
> add API that will take a JSON object directly.
> ---
>  src/qemu/qemu_monitor.c  | 18 ++
>  src/qemu/qemu_monitor.h  |  2 ++
>  src/qemu/qemu_monitor_json.c | 29 +++--
>  src/qemu/qemu_monitor_json.h |  2 ++
>  4 files changed, 41 insertions(+), 10 deletions(-)
> 
> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
> index 58c04d5..b58c412 100644
> --- a/src/qemu/qemu_monitor.c
> +++ b/src/qemu/qemu_monitor.c
> @@ -2771,6 +2771,24 @@ qemuMonitorAddDevice(qemuMonitorPtr mon,
> 
> 
>  /**
> + * qemuMonitorAddDeviceArgs:
> + * @mon: monitor object
> + * @args: arguments for device add, consumed on success or failure
> + *
> + * Adds a device described by @args. Requires JSON monitor.
> + * Returns 0 on success -1 on error.
> + */
> +int
> +qemuMonitorAddDeviceArgs(qemuMonitorPtr mon,
> + virJSONValuePtr args)
> +{
> +QEMU_CHECK_MONITOR_JSON(mon);
> +
> +return qemuMonitorJSONAddDeviceArgs(mon, args);
> +}
> +
> +
> +/**
>   * qemuMonitorAddObject:
>   * @mon: Pointer to monitor object
>   * @type: Type name of object to add
> diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
> index ae0954d..805656b 100644
> --- a/src/qemu/qemu_monitor.h
> +++ b/src/qemu/qemu_monitor.h
> @@ -685,6 +685,8 @@ int qemuMonitorAttachPCIDiskController(qemuMonitorPtr mon,
> const char *bus,
> virPCIDeviceAddress *guestAddr);
> 
> +int qemuMonitorAddDeviceArgs(qemuMonitorPtr mon,
> + virJSONValuePtr args);
>  int qemuMonitorAddDevice(qemuMonitorPtr mon,
>   const char *devicestr);
> 
> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> index 5283024..cf55a61 100644
> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c
> @@ -3583,20 +3583,15 @@ int qemuMonitorJSONDelDevice(qemuMonitorPtr mon,
>  }
> 
> 
> -int qemuMonitorJSONAddDevice(qemuMonitorPtr mon,
> - const char *devicestr)
> +int
> +qemuMonitorJSONAddDeviceArgs(qemuMonitorPtr mon,
> + virJSONValuePtr args)
>  {
>  int ret = -1;
> -virJSONValuePtr cmd;
> +virJSONValuePtr cmd = NULL;
>  virJSONValuePtr reply = NULL;
> -virJSONValuePtr args;
> 
> -cmd = qemuMonitorJSONMakeCommand("device_add", NULL);
> -if (!cmd)
> -return -1;
> -
> -args = qemuMonitorJSONKeywordStringToJSON(devicestr, "driver");
> -if (!args)
> +if (!(cmd = qemuMonitorJSONMakeCommand("device_add", NULL)))
>  goto cleanup;
> 
>  if (virJSONValueObjectAppend(cmd, "arguments", args) < 0)
> @@ -3618,6 +3613,20 @@ int qemuMonitorJSONAddDevice(qemuMonitorPtr mon,
>  }
> 
> 
> +int
> +qemuMonitorJSONAddDevice(qemuMonitorPtr mon,
> + const char *devicestr)
> +{
> +virJSONValuePtr args;
> +
> +if (!(args = qemuMonitorJSONKeywordStringToJSON(devicestr, "driver")))
> +return -1;
> +
> +/* qemuMonitorJSONAddDeviceArgs always consumes @args */

This comment is redundant, the same information is in the function doc.

> +return qemuMonitorJSONAddDeviceArgs(mon, args);
> +}
> +
> +
>  int qemuMonitorJSONAddObject(qemuMonitorPtr mon,
>   const char *type,
>   const char *objalias,
> diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h
> index 0b3d898..7f3222a 100644
> --- a/src/qemu/qemu_monitor_json.h
> +++ b/src/qemu/qemu_monitor_json.h
> @@ -215,6 +215,8 @@ int qemuMonitorJSONAttachPCIDiskController(qemuMonitorPtr 
> mon,
> const char *bus,
> virPCIDeviceAddress *guestAddr);
> 
> +int qemuMonitorJSONAddDeviceArgs(qemuMonitorPtr mon,
> + virJSONValuePtr args);
>  int qemuMonitorJSONAddDevice(qemuMonitorPtr mon,
>   const char *devicestr);
> 

ACK

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

[libvirt] [PATCH] storage: Fix a NULL ptr dereference in virStorageBackendCreateQemuImg

[Erik Skultety]

There was a missing check for vol->target.encryption being NULL
at one particular place (modified by commit a48c71411) which caused a crash
when user attempted to create a raw volume using a non-raw file volume as
source.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1363636

Signed-off-by: Erik Skultety 
---
 src/storage/storage_backend.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
index 1f33181..d4334dc 100644
--- a/src/storage/storage_backend.c
+++ b/src/storage/storage_backend.c
@@ -1459,6 +1459,7 @@ virStorageBackendCreateQemuImg(virConnectPtr conn,
 goto cleanup;
 
 if (vol->target.format == VIR_STORAGE_FILE_RAW &&
+vol->target.encryption &&
 vol->target.encryption->format == VIR_STORAGE_ENCRYPTION_FORMAT_LUKS) {
 if (!(secretPath =
   virStorageBackendCreateQemuImgSecretPath(conn, pool, vol)))
-- 
2.5.5

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

Re: [libvirt] [PATCH] libxl: add serial list support

[Joao Martins]

[Adding Jim as there was past discussion about serials/consoles]

On 08/03/2016 02:29 AM, Bob Liu wrote:
> Add support for multi serial devices, after this patch virsh can be used to
> connect different serial devices of running domains. E.g.
> vish # console  --devname serial
> 
> Signed-off-by: Bob Liu 
> ---
Through some internal review it slipped that serial list is only supported from 
Xen
>= 4.5 (see below). Also for folks seeing this patch, it should be noted that 
>this
depends on a bug fix to have libxl_console_get_tty(...) correctly returning the 
tty
path (as opposed to always returning the first one).

[0] https://lists.xen.org/archives/html/xen-devel/2016-08/msg00438.html

>  src/libxl/libxl_conf.c   | 25 ++---
>  src/libxl/libxl_domain.c | 23 +++
>  src/libxl/libxl_driver.c | 17 +
>  3 files changed, 50 insertions(+), 15 deletions(-)
> 
> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
> index 146e08a..685a5ad 100644
> --- a/src/libxl/libxl_conf.c
> +++ b/src/libxl/libxl_conf.c
> @@ -431,14 +431,25 @@ libxlMakeDomBuildInfo(virDomainDefPtr def,
>  }
>  
>  if (def->nserials) {
> -if (def->nserials > 1) {
> -virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> -   "%s",
> -   _("Only one serial device is supported by 
> libxl"));
> -return -1;
> +if (def->nserials == 1) {
> +if (libxlMakeChrdevStr(def->serials[0], 
> _info->u.hvm.serial) <
> +0)
> +return -1;
> +} else {
> +if (VIR_ALLOC_N(b_info->u.hvm.serial_list, def->nserials + 
> 1) <
> +0)
> +return -1;
> +
> +for (i = 0; i < def->nserials; i++) {
> +if (libxlMakeChrdevStr(def->serials[i],
> +   _info->u.hvm.serial_list[i]) < 
> 0)
> +{
> +
> libxl_string_list_dispose(_info->u.hvm.serial_list);
> +return -1;
> +}
> +}
> +b_info->u.hvm.serial_list[i] = NULL;
This particular chunk needs to be surrounded with 
LIBXL_HAVE_BUILDINFO_SERIAL_LIST
(otherwise it would break compilation) and then warn the user that it's not 
supported
for Xen 4.4. Something like:

#ifdef LIBXL_HAVE_BUILDINFO_SERIAL_LIST
if (VIR_ALLOC_N(b_info->u.hvm.serial_list, def->nserials + 1) <
0)
return -1;

for (i = 0; i < def->nserials; i++) {
if (libxlMakeChrdevStr(def->serials[i],
   _info->u.hvm.serial_list[i]) < 0)
{
libxl_string_list_dispose(_info->u.hvm.serial_list);
return -1;
}
}
b_info->u.hvm.serial_list[i] = NULL;
#else
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
   "%s",
   _("Only one serial device is supported by 
libxl"));
return -1;  
#endif

>  }
> -if (libxlMakeChrdevStr(def->serials[0], _info->u.hvm.serial) < 
> 0)
> -return -1;
>  }
>  
>  if (def->nparallels) {
> diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
> index 0e26b91..82fe9e8 100644
> --- a/src/libxl/libxl_domain.c
> +++ b/src/libxl/libxl_domain.c
> @@ -989,6 +989,29 @@ libxlConsoleCallback(libxl_ctx *ctx, libxl_event *ev, 
> void *for_callback)
>  VIR_FREE(console);
>  }
>  }
> +for (i = 0; i < vm->def->nserials; i++) {
> +virDomainChrDefPtr chr = vm->def->serials[i];
> +char *console = NULL;
> +int ret;
> +
> +if (chr->source.type == VIR_DOMAIN_CHR_TYPE_PTY) {
> +ignore_value(virAsprintf(>info.alias, "serial%zd", i));
It's probably worth setting alias before this if, since it's general to all 
serial types.

> +if (chr->source.data.file.path)
> +continue;
> +ret = libxl_console_get_tty(ctx, ev->domid,
> +chr->target.port,
> +LIBXL_CONSOLE_TYPE_SERIAL,
> +);
> +if (!ret) {
> +VIR_FREE(chr->source.data.file.path);
> +if (console && console[0] != '\0') {
> +ignore_value(VIR_STRDUP(chr->source.data.file.path,
> +console));
> +}
> +}
> +VIR_FREE(console);
> +}
> +}
>  virObjectUnlock(vm);
>  libxl_event_free(ctx, ev);
>  }
> diff --git a/src/libxl/libxl_driver.c 

Re: [libvirt] [PATCH] libxl: allow libxl to calculate shadow mem requirements

[Joao Martins]

On 08/03/2016 12:49 AM, Jim Fehlig wrote:
> Long, long ago before libxl_get_required_shadow_memory() was
> made publicly available, its code was copied to the libxl driver
> for calculating shadow memory requirements of HVM domains.
> 
> Long ago, libxl_get_required_shadow_memory() was exported in
> libxl_utils.h and included in xen-devel packages everywhere.
> 
> Remove the copied code, which has become stale, and let libxl
> provode a proper shadow memory value.
> 
> Signed-off-by: Jim Fehlig 
> ---
> 
> I ensured libxl_get_required_shadow_memory() was available
> as far back as Xen 4.4, which is the minimum version supported
> by the libxl driver.

Cool, and FWIW, Looks good to me:

Acked-by: Joao Martins 

(Also tested an HVM guest in which codepath goes through):

>  src/libxl/libxl_conf.c | 13 -
>  1 file changed, 4 insertions(+), 9 deletions(-)
> 
> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
> index 1344604..5202ca1 100644
> --- a/src/libxl/libxl_conf.c
> +++ b/src/libxl/libxl_conf.c
> @@ -493,15 +493,10 @@ libxlMakeDomBuildInfo(virDomainDefPtr def,
>  }
>  }
>  
> -/*
> - * The following comment and calculation were taken directly from
> - * libxenlight's internal function 
> libxl_get_required_shadow_memory():
> - *
> - * 256 pages (1MB) per vcpu, plus 1 page per MiB of RAM for the P2M 
> map,
> - * plus 1 page per MiB of RAM to shadow the resident processes.
> - */
> -b_info->shadow_memkb = 4 * (256 * 
> libxl_bitmap_count_set(_info->avail_vcpus) +
> -2 * (b_info->max_memkb / 1024));
> +/* Allow libxl to calculate shadow memory requirements */
> +b_info->shadow_memkb =
> +libxl_get_required_shadow_memory(b_info->max_memkb,
> + b_info->max_vcpus);
>  } else {
>  /*
>   * For compatibility with the legacy xen toolstack, default to pygrub
> 

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

[libvirt] [PATCH 07/10] qemu: domain: Improve vCPU data checking in qemuDomainDetectVcpuPids

[Peter Krempa]

Validate the presence of the thread id according to state of the vCPU
rather than just checking the vCPU count. Additionally put the new
validation code into a separate function so that the information
retrieval can be split from the validation.
---
 src/qemu/qemu_domain.c | 49 +++--
 src/qemu/qemu_domain.h |  1 +
 2 files changed, 44 insertions(+), 6 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 5cde841..f27f2f7 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -5624,6 +5624,48 @@ qemuDomainGetVcpuPid(virDomainObjPtr vm,


 /**
+ * qemuDomainValidateVcpuInfo:
+ *
+ * Validates vcpu thread information. If vcpu thread IDs are reported by qemu,
+ * this function validates that online vcpus have thread info present and
+ * offline vcpus don't.
+ *
+ * Returns 0 on success -1 on error.
+ */
+int
+qemuDomainValidateVcpuInfo(virDomainObjPtr vm)
+{
+size_t maxvcpus = virDomainDefGetVcpusMax(vm->def);
+virDomainVcpuDefPtr vcpu;
+qemuDomainVcpuPrivatePtr vcpupriv;
+size_t i;
+
+if (!qemuDomainHasVcpuPids(vm))
+return 0;
+
+for (i = 0; i < maxvcpus; i++) {
+vcpu = virDomainDefGetVcpu(vm->def, i);
+vcpupriv = QEMU_DOMAIN_VCPU_PRIVATE(vcpu);
+
+if (vcpu->online && vcpupriv->tid == 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("qemu didn't report thread id for vcpu '%zu'"), 
i);
+return -1;
+}
+
+if (!vcpu->online && vcpupriv->tid != 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("qemu reported thread if for inactive vcpu 
'%zu'"),
+   i);
+return -1;
+}
+}
+
+return 0;
+}
+
+
+/**
  * qemuDomainRefreshVcpuInfo:
  * @driver: qemu driver data
  * @vm: domain object
@@ -5703,13 +5745,8 @@ qemuDomainRefreshVcpuInfo(virQEMUDriverPtr driver,
 QEMU_DOMAIN_VCPU_PRIVATE(vcpu)->tid = 0;
 }

-if (ncpupids != virDomainDefGetVcpus(vm->def)) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
-   _("got wrong number of vCPU pids from QEMU monitor. "
- "got %d, wanted %d"),
-   ncpupids, virDomainDefGetVcpus(vm->def));
+if (qemuDomainValidateVcpuInfo(vm) < 0)
 goto cleanup;
-}

 ret = ncpupids;

diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index 3193427..0613093 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -647,6 +647,7 @@ int qemuDomainDefValidateMemoryHotplug(const virDomainDef 
*def,

 bool qemuDomainHasVcpuPids(virDomainObjPtr vm);
 pid_t qemuDomainGetVcpuPid(virDomainObjPtr vm, unsigned int vcpuid);
+int qemuDomainValidateVcpuInfo(virDomainObjPtr vm);
 int qemuDomainRefreshVcpuInfo(virQEMUDriverPtr driver,
   virDomainObjPtr vm,
   int asyncJob);
-- 
2.9.2

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

[libvirt] [PATCH 01/10] qemu: monitor: Add monitor API for device_add supporting JSON objects

[Peter Krempa]

Rather than formating a string and splitting it back to a JSON object
add API that will take a JSON object directly.
---
 src/qemu/qemu_monitor.c  | 18 ++
 src/qemu/qemu_monitor.h  |  2 ++
 src/qemu/qemu_monitor_json.c | 29 +++--
 src/qemu/qemu_monitor_json.h |  2 ++
 4 files changed, 41 insertions(+), 10 deletions(-)

diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index 58c04d5..b58c412 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -2771,6 +2771,24 @@ qemuMonitorAddDevice(qemuMonitorPtr mon,


 /**
+ * qemuMonitorAddDeviceArgs:
+ * @mon: monitor object
+ * @args: arguments for device add, consumed on success or failure
+ *
+ * Adds a device described by @args. Requires JSON monitor.
+ * Returns 0 on success -1 on error.
+ */
+int
+qemuMonitorAddDeviceArgs(qemuMonitorPtr mon,
+ virJSONValuePtr args)
+{
+QEMU_CHECK_MONITOR_JSON(mon);
+
+return qemuMonitorJSONAddDeviceArgs(mon, args);
+}
+
+
+/**
  * qemuMonitorAddObject:
  * @mon: Pointer to monitor object
  * @type: Type name of object to add
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
index ae0954d..805656b 100644
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -685,6 +685,8 @@ int qemuMonitorAttachPCIDiskController(qemuMonitorPtr mon,
const char *bus,
virPCIDeviceAddress *guestAddr);

+int qemuMonitorAddDeviceArgs(qemuMonitorPtr mon,
+ virJSONValuePtr args);
 int qemuMonitorAddDevice(qemuMonitorPtr mon,
  const char *devicestr);

diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 5283024..cf55a61 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -3583,20 +3583,15 @@ int qemuMonitorJSONDelDevice(qemuMonitorPtr mon,
 }


-int qemuMonitorJSONAddDevice(qemuMonitorPtr mon,
- const char *devicestr)
+int
+qemuMonitorJSONAddDeviceArgs(qemuMonitorPtr mon,
+ virJSONValuePtr args)
 {
 int ret = -1;
-virJSONValuePtr cmd;
+virJSONValuePtr cmd = NULL;
 virJSONValuePtr reply = NULL;
-virJSONValuePtr args;

-cmd = qemuMonitorJSONMakeCommand("device_add", NULL);
-if (!cmd)
-return -1;
-
-args = qemuMonitorJSONKeywordStringToJSON(devicestr, "driver");
-if (!args)
+if (!(cmd = qemuMonitorJSONMakeCommand("device_add", NULL)))
 goto cleanup;

 if (virJSONValueObjectAppend(cmd, "arguments", args) < 0)
@@ -3618,6 +3613,20 @@ int qemuMonitorJSONAddDevice(qemuMonitorPtr mon,
 }


+int
+qemuMonitorJSONAddDevice(qemuMonitorPtr mon,
+ const char *devicestr)
+{
+virJSONValuePtr args;
+
+if (!(args = qemuMonitorJSONKeywordStringToJSON(devicestr, "driver")))
+return -1;
+
+/* qemuMonitorJSONAddDeviceArgs always consumes @args */
+return qemuMonitorJSONAddDeviceArgs(mon, args);
+}
+
+
 int qemuMonitorJSONAddObject(qemuMonitorPtr mon,
  const char *type,
  const char *objalias,
diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h
index 0b3d898..7f3222a 100644
--- a/src/qemu/qemu_monitor_json.h
+++ b/src/qemu/qemu_monitor_json.h
@@ -215,6 +215,8 @@ int qemuMonitorJSONAttachPCIDiskController(qemuMonitorPtr 
mon,
const char *bus,
virPCIDeviceAddress *guestAddr);

+int qemuMonitorJSONAddDeviceArgs(qemuMonitorPtr mon,
+ virJSONValuePtr args);
 int qemuMonitorJSONAddDevice(qemuMonitorPtr mon,
  const char *devicestr);

-- 
2.9.2

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

[libvirt] [PATCH 06/10] qemu: monitor: Rename qemuMonitor(JSON|Text)GetCPUInfo

[Peter Krempa]

Use a name that contains the command used to get the information.
---
 src/qemu/qemu_monitor.c  | 4 ++--
 src/qemu/qemu_monitor_json.c | 5 +++--
 src/qemu/qemu_monitor_json.h | 4 ++--
 src/qemu/qemu_monitor_text.c | 5 +++--
 src/qemu/qemu_monitor_text.h | 4 ++--
 tests/qemumonitorjsontest.c  | 6 +++---
 6 files changed, 15 insertions(+), 13 deletions(-)

diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index 1ef18dd..4403bdd 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -1672,9 +1672,9 @@ qemuMonitorGetCPUInfo(qemuMonitorPtr mon,
 QEMU_CHECK_MONITOR(mon);

 if (mon->json)
-return qemuMonitorJSONGetCPUInfo(mon, pids);
+return qemuMonitorJSONQueryCPUs(mon, pids);
 else
-return qemuMonitorTextGetCPUInfo(mon, pids);
+return qemuMonitorTextQueryCPUs(mon, pids);
 }


diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index cf55a61..715e1c7 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -1376,8 +1376,9 @@ qemuMonitorJSONExtractCPUInfo(virJSONValuePtr reply,
 }


-int qemuMonitorJSONGetCPUInfo(qemuMonitorPtr mon,
-  int **pids)
+int
+qemuMonitorJSONQueryCPUs(qemuMonitorPtr mon,
+ int **pids)
 {
 int ret = -1;
 virJSONValuePtr cmd = qemuMonitorJSONMakeCommand("query-cpus",
diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h
index 7f3222a..174f0ef 100644
--- a/src/qemu/qemu_monitor_json.h
+++ b/src/qemu/qemu_monitor_json.h
@@ -58,8 +58,8 @@ int qemuMonitorJSONGetStatus(qemuMonitorPtr mon,
 int qemuMonitorJSONSystemPowerdown(qemuMonitorPtr mon);
 int qemuMonitorJSONSystemReset(qemuMonitorPtr mon);

-int qemuMonitorJSONGetCPUInfo(qemuMonitorPtr mon,
-  int **pids);
+int qemuMonitorJSONQueryCPUs(qemuMonitorPtr mon,
+ int **pids);
 int qemuMonitorJSONGetVirtType(qemuMonitorPtr mon,
virDomainVirtType *virtType);
 int qemuMonitorJSONUpdateVideoMemorySize(qemuMonitorPtr mon,
diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c
index 2c31b52..ca04965 100644
--- a/src/qemu/qemu_monitor_text.c
+++ b/src/qemu/qemu_monitor_text.c
@@ -500,8 +500,9 @@ int qemuMonitorTextSystemReset(qemuMonitorPtr mon)
 }


-int qemuMonitorTextGetCPUInfo(qemuMonitorPtr mon,
-  int **pids)
+int
+qemuMonitorTextQueryCPUs(qemuMonitorPtr mon,
+ int **pids)
 {
 char *qemucpus = NULL;
 char *line;
diff --git a/src/qemu/qemu_monitor_text.h b/src/qemu/qemu_monitor_text.h
index eeaca52..b7f0cab 100644
--- a/src/qemu/qemu_monitor_text.h
+++ b/src/qemu/qemu_monitor_text.h
@@ -49,8 +49,8 @@ int qemuMonitorTextGetStatus(qemuMonitorPtr mon,
 int qemuMonitorTextSystemPowerdown(qemuMonitorPtr mon);
 int qemuMonitorTextSystemReset(qemuMonitorPtr mon);

-int qemuMonitorTextGetCPUInfo(qemuMonitorPtr mon,
-  int **pids);
+int qemuMonitorTextQueryCPUs(qemuMonitorPtr mon,
+ int **pids);
 int qemuMonitorTextGetVirtType(qemuMonitorPtr mon,
virDomainVirtType *virtType);
 int qemuMonitorTextGetBalloonInfo(qemuMonitorPtr mon,
diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c
index e8946c2..544b569 100644
--- a/tests/qemumonitorjsontest.c
+++ b/tests/qemumonitorjsontest.c
@@ -1203,7 +1203,7 @@ GEN_TEST_FUNC(qemuMonitorJSONDetachCharDev, "serial1")


 static int
-testQemuMonitorJSONqemuMonitorJSONGetCPUInfo(const void *data)
+testQemuMonitorJSONqemuMonitorJSONQueryCPUs(const void *data)
 {
 virDomainXMLOptionPtr xmlopt = (virDomainXMLOptionPtr)data;
 qemuMonitorTestPtr test = qemuMonitorTestNewSimple(true, xmlopt);
@@ -1252,7 +1252,7 @@ testQemuMonitorJSONqemuMonitorJSONGetCPUInfo(const void 
*data)
"}") < 0)
 goto cleanup;

-ncpupids = qemuMonitorJSONGetCPUInfo(qemuMonitorTestGetMonitor(test), 
);
+ncpupids = qemuMonitorJSONQueryCPUs(qemuMonitorTestGetMonitor(test), 
);

 if (ncpupids != 4) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -2420,7 +2420,7 @@ mymain(void)
 DO_TEST(qemuMonitorJSONSetBlockIoThrottle);
 DO_TEST(qemuMonitorJSONGetTargetArch);
 DO_TEST(qemuMonitorJSONGetMigrationCapability);
-DO_TEST(qemuMonitorJSONGetCPUInfo);
+DO_TEST(qemuMonitorJSONQueryCPUs);
 DO_TEST(qemuMonitorJSONGetVirtType);
 DO_TEST(qemuMonitorJSONSendKey);
 DO_TEST(qemuMonitorJSONGetDumpGuestMemoryCapability);
-- 
2.9.2

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

[libvirt] [PATCH 03/10] qemu: monitor: Add do-while block to QEMU_CHECK_MONITOR_FULL

[Peter Krempa]

Assure that it's just one statement to avoid problems when used with
conditions.
---
 src/qemu/qemu_monitor.c | 25 ++---
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index b58c412..1ef18dd 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -112,17 +112,20 @@ struct _qemuMonitor {
  * monitor.
  */
 #define QEMU_CHECK_MONITOR_FULL(mon, force_json, exit) 
\
-if (!mon) {
\
-virReportError(VIR_ERR_INVALID_ARG, "%s",  
\
-   _("monitor must not be NULL")); 
\
-exit;  
\
-}  
\
-VIR_DEBUG("mon:%p vm:%p json:%d fd:%d", mon, mon->vm, mon->json, mon->fd); 
\
-if (force_json && !mon->json) {
\
-virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
\
-   _("JSON monitor is required")); 
\
-exit;  
\
-}
+do {   
\
+if (!mon) {
\
+virReportError(VIR_ERR_INVALID_ARG, "%s",  
\
+   _("monitor must not be NULL")); 
\
+exit;  
\
+}  
\
+VIR_DEBUG("mon:%p vm:%p json:%d fd:%d",
\
+  mon, mon->vm, mon->json, mon->fd);   
\
+if (force_json && !mon->json) {
\
+virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
\
+   _("JSON monitor is required")); 
\
+exit;  
\
+}  
\
+} while (0)

 /* Check monitor and return NULL on error */
 #define QEMU_CHECK_MONITOR_NULL(mon) \
-- 
2.9.2

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

[libvirt] [PATCH 05/10] qemu: domain: Rename qemuDomainDetectVcpuPids to qemuDomainRefreshVcpuInfo

[Peter Krempa]

The function will eventually do more useful stuff than just detection of
thread ids.
---
 src/qemu/qemu_domain.c  | 10 +-
 src/qemu/qemu_domain.h  |  5 +++--
 src/qemu/qemu_driver.c  |  4 ++--
 src/qemu/qemu_process.c |  6 +++---
 4 files changed, 13 insertions(+), 12 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index efc1fb5..5cde841 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -5624,20 +5624,20 @@ qemuDomainGetVcpuPid(virDomainObjPtr vm,


 /**
- * qemuDomainDetectVcpuPids:
+ * qemuDomainRefreshVcpuInfo:
  * @driver: qemu driver data
  * @vm: domain object
  * @asyncJob: current asynchronous job type
  *
- * Updates vCPU thread ids in the private data of @vm.
+ * Updates vCPU information private data of @vm.
  *
  * Returns number of detected vCPU threads on success, -1 on error and reports
  * an appropriate error, -2 if the domain doesn't exist any more.
  */
 int
-qemuDomainDetectVcpuPids(virQEMUDriverPtr driver,
- virDomainObjPtr vm,
- int asyncJob)
+qemuDomainRefreshVcpuInfo(virQEMUDriverPtr driver,
+  virDomainObjPtr vm,
+  int asyncJob)
 {
 virDomainVcpuDefPtr vcpu;
 size_t maxvcpus = virDomainDefGetVcpusMax(vm->def);
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index 9e10ae4..3193427 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -647,8 +647,9 @@ int qemuDomainDefValidateMemoryHotplug(const virDomainDef 
*def,

 bool qemuDomainHasVcpuPids(virDomainObjPtr vm);
 pid_t qemuDomainGetVcpuPid(virDomainObjPtr vm, unsigned int vcpuid);
-int qemuDomainDetectVcpuPids(virQEMUDriverPtr driver, virDomainObjPtr vm,
- int asyncJob);
+int qemuDomainRefreshVcpuInfo(virQEMUDriverPtr driver,
+  virDomainObjPtr vm,
+  int asyncJob);

 bool qemuDomainSupportsNicdev(virDomainDefPtr def,
   virDomainNetDefPtr net);
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 061c16f..453572b 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4639,7 +4639,7 @@ qemuDomainHotplugAddVcpu(virQEMUDriverPtr driver,

 vcpuinfo->online = true;

-if ((rc = qemuDomainDetectVcpuPids(driver, vm, QEMU_ASYNC_JOB_NONE)) <= 0) 
{
+if ((rc = qemuDomainRefreshVcpuInfo(driver, vm, QEMU_ASYNC_JOB_NONE)) <= 
0) {
 /* vcpu pids were not detected, skip setting of affinity */
 if (rc == 0)
 ret = 0;
@@ -4689,7 +4689,7 @@ qemuDomainHotplugDelVcpu(virQEMUDriverPtr driver,
 goto cleanup;
 }

-if ((rc = qemuDomainDetectVcpuPids(driver, vm, QEMU_ASYNC_JOB_NONE)) < 0) {
+if ((rc = qemuDomainRefreshVcpuInfo(driver, vm, QEMU_ASYNC_JOB_NONE)) < 0) 
{
 /* rollback only if domain didn't exit */
 if (rc == -2)
 goto cleanup;
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index a4935d2..9136ba5 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -5189,8 +5189,8 @@ qemuProcessLaunch(virConnectPtr conn,
 if (qemuSetupCpusetMems(vm) < 0)
 goto cleanup;

-VIR_DEBUG("Detecting VCPU PIDs");
-if (qemuDomainDetectVcpuPids(driver, vm, asyncJob) < 0)
+VIR_DEBUG("Refreshing VCPU info");
+if (qemuDomainRefreshVcpuInfo(driver, vm, asyncJob) < 0)
 goto cleanup;

 VIR_DEBUG("Detecting IOThread PIDs");
@@ -5982,7 +5982,7 @@ int qemuProcessAttach(virConnectPtr conn ATTRIBUTE_UNUSED,
 }

 VIR_DEBUG("Detecting VCPU PIDs");
-if (qemuDomainDetectVcpuPids(driver, vm, QEMU_ASYNC_JOB_NONE) < 0)
+if (qemuDomainRefreshVcpuInfo(driver, vm, QEMU_ASYNC_JOB_NONE) < 0)
 goto error;

 VIR_DEBUG("Detecting IOThread PIDs");
-- 
2.9.2

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

[libvirt] [PATCH 04/10] qemu: Improve error message in virDomainGetVcpus

[Peter Krempa]

If the VM is offline we can't retrieve the runtime statistical
information. Pinning could be retrieved but there are separate APIs for
that.
---
 src/qemu/qemu_driver.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 11db9a5..061c16f 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -5284,9 +5284,8 @@ qemuDomainGetVcpus(virDomainPtr dom,
 goto cleanup;

 if (!virDomainObjIsActive(vm)) {
-virReportError(VIR_ERR_OPERATION_INVALID,
-   "%s",
-   _("cannot list vcpu pinning for an inactive domain"));
+virReportError(VIR_ERR_OPERATION_INVALID, "%s",
+   _("cannot retrieve vcpu information for inactive 
domain"));
 goto cleanup;
 }

-- 
2.9.2

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

[libvirt] [PATCH 09/10] qemu: monitor: Return structures from qemuMonitorGetCPUInfo

[Peter Krempa]

The function will gradually add more returned data. Return a struct for
every vCPU containing the data.
---
 src/qemu/qemu_domain.c  | 26 ++-
 src/qemu/qemu_monitor.c | 56 ++---
 src/qemu/qemu_monitor.h | 13 +++-
 3 files changed, 72 insertions(+), 23 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 4cdd012..9d389f7 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -5682,10 +5682,11 @@ qemuDomainRefreshVcpuInfo(virQEMUDriverPtr driver,
   int asyncJob)
 {
 virDomainVcpuDefPtr vcpu;
+qemuDomainVcpuPrivatePtr vcpupriv;
+qemuMonitorCPUInfoPtr info = NULL;
 size_t maxvcpus = virDomainDefGetVcpusMax(vm->def);
-pid_t *cpupids = NULL;
-int ncpupids;
 size_t i;
+int rc;
 int ret = -1;

 /*
@@ -5721,31 +5722,26 @@ qemuDomainRefreshVcpuInfo(virQEMUDriverPtr driver,

 if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
 return -1;
-ncpupids = qemuMonitorGetCPUInfo(qemuDomainGetMonitor(vm), );
+
+rc = qemuMonitorGetCPUInfo(qemuDomainGetMonitor(vm), , maxvcpus);
+
 if (qemuDomainObjExitMonitor(driver, vm) < 0)
+goto cleanup;

-/* failure to get the VCPU <-> PID mapping or to execute the query
- * command will not be treated fatal as some versions of qemu don't
- * support this command */
-if (ncpupids <= 0) {
-virResetLastError();
-ret = 0;
+if (rc < 0)
 goto cleanup;
-}

 for (i = 0; i < maxvcpus; i++) {
 vcpu = virDomainDefGetVcpu(vm->def, i);
+vcpupriv = QEMU_DOMAIN_VCPU_PRIVATE(vcpu);

-if (i < ncpupids)
-QEMU_DOMAIN_VCPU_PRIVATE(vcpu)->tid = cpupids[i];
-else
-QEMU_DOMAIN_VCPU_PRIVATE(vcpu)->tid = 0;
+vcpupriv->tid = info[i].tid;
 }

 ret = 0;

  cleanup:
-VIR_FREE(cpupids);
+qemuMonitorCPUInfoFree(info, maxvcpus);
 return ret;
 }

diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index 4403bdd..0011ceb 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -1656,25 +1656,67 @@ qemuMonitorSystemReset(qemuMonitorPtr mon)
 }


+void
+qemuMonitorCPUInfoFree(qemuMonitorCPUInfoPtr cpus,
+   size_t ncpus ATTRIBUTE_UNUSED)
+{
+if (!cpus)
+return;
+
+VIR_FREE(cpus);
+}
+
+
 /**
  * qemuMonitorGetCPUInfo:
  * @mon: monitor
- * @pids: returned array of thread ids corresponding to the vCPUs
+ * @cpus: pointer filled by array of qemuMonitorCPUInfo structures
+ * @maxvcpus: total possible number of vcpus
+ *
+ * Detects VCPU information. If qemu doesn't support or fails reporting
+ * information this function will return success as other parts of libvirt
+ * are able to cope with that.
  *
- * Detects the vCPU thread ids. Returns count of detected vCPUs on success,
- * 0 if qemu didn't report thread ids (does not report libvirt error),
- * -1 on error (reports libvirt error).
+ * Returns 0 on success (including if qemu didn't report any data) and
+ *  -1 on error (reports libvirt error).
  */
 int
 qemuMonitorGetCPUInfo(qemuMonitorPtr mon,
-  int **pids)
+  qemuMonitorCPUInfoPtr *vcpus,
+  size_t maxvcpus)
 {
+qemuMonitorCPUInfoPtr info = NULL;
+int *pids = NULL;
+size_t i;
+int ret = -1;
+int rc;
+
 QEMU_CHECK_MONITOR(mon);

+if (VIR_ALLOC_N(info, maxvcpus) < 0)
+return -1;
+
 if (mon->json)
-return qemuMonitorJSONQueryCPUs(mon, pids);
+rc = qemuMonitorJSONQueryCPUs(mon, );
 else
-return qemuMonitorTextQueryCPUs(mon, pids);
+rc = qemuMonitorTextQueryCPUs(mon, );
+
+if (rc < 0) {
+virResetLastError();
+ret = 0;
+goto cleanup;
+}
+
+for (i = 0; i < rc; i++)
+info[i].tid = pids[i];
+
+VIR_STEAL(*vcpus, info);
+ret = 0;
+
+ cleanup:
+qemuMonitorCPUInfoFree(info, maxvcpus);
+VIR_FREE(pids);
+return ret;
 }


diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
index 805656b..1ef002a 100644
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -390,8 +390,19 @@ int qemuMonitorGetStatus(qemuMonitorPtr mon,
 int qemuMonitorSystemReset(qemuMonitorPtr mon);
 int qemuMonitorSystemPowerdown(qemuMonitorPtr mon);

+
+struct _qemuMonitorCPUInfo {
+pid_t tid;
+};
+typedef struct _qemuMonitorCPUInfo qemuMonitorCPUInfo;
+typedef qemuMonitorCPUInfo *qemuMonitorCPUInfoPtr;
+
+void qemuMonitorCPUInfoFree(qemuMonitorCPUInfoPtr list,
+size_t nitems);
 int qemuMonitorGetCPUInfo(qemuMonitorPtr mon,
-  int **pids);
+  qemuMonitorCPUInfoPtr *vcpus,
+  size_t maxvcpus);
+
 int qemuMonitorGetVirtType(qemuMonitorPtr mon,
virDomainVirtType *virtType);
 int 

[libvirt] [PATCH 00/10] New vCPU hotplug prequel

[Peter Krempa]

A few patches that are standalone enough which originated from my work in
progress on new way to do vcpu hotplug.

Peter Krempa (10):
  qemu: monitor: Add monitor API for device_add supporting JSON objects
  internal: Introduce macro for stealing pointers
  qemu: monitor: Add do-while block to QEMU_CHECK_MONITOR_FULL
  qemu: Improve error message in virDomainGetVcpus
  qemu: domain: Rename qemuDomainDetectVcpuPids to
qemuDomainRefreshVcpuInfo
  qemu: monitor: Rename qemuMonitor(JSON|Text)GetCPUInfo
  qemu: domain: Improve vCPU data checking in qemuDomainDetectVcpuPids
  qemu: domain: Simplify return values of qemuDomainRefreshVcpuInfo
  qemu: monitor: Return structures from qemuMonitorGetCPUInfo
  qemu: monitor: Return struct from qemuMonitor(Text|Json)QueryCPUs

 src/internal.h   |  12 +
 src/qemu/qemu_domain.c   |  95 -
 src/qemu/qemu_domain.h   |   6 ++-
 src/qemu/qemu_driver.c   |  25 +-
 src/qemu/qemu_monitor.c  | 110 ---
 src/qemu/qemu_monitor.h  |  21 -
 src/qemu/qemu_monitor_json.c | 103 ++--
 src/qemu/qemu_monitor_json.h |   6 ++-
 src/qemu/qemu_monitor_text.c |  40 
 src/qemu/qemu_monitor_text.h |   4 +-
 src/qemu/qemu_process.c  |  12 +++--
 tests/qemumonitorjsontest.c  |  35 ++
 12 files changed, 316 insertions(+), 153 deletions(-)

-- 
2.9.2

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

[libvirt] [PATCH 08/10] qemu: domain: Simplify return values of qemuDomainRefreshVcpuInfo

[Peter Krempa]

Call the vcpu thread info validation separately to decrease complexity
of returned values by qemuDomainRefreshVcpuInfo.

This function now returns 0 on success and -1 on error. Certain
failures of qemu to report data are still considered as success. Any
error reported now is fatal.
---
 src/qemu/qemu_domain.c  | 16 +---
 src/qemu/qemu_driver.c  | 20 ++--
 src/qemu/qemu_process.c |  6 ++
 3 files changed, 21 insertions(+), 21 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index f27f2f7..4cdd012 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -5671,10 +5671,10 @@ qemuDomainValidateVcpuInfo(virDomainObjPtr vm)
  * @vm: domain object
  * @asyncJob: current asynchronous job type
  *
- * Updates vCPU information private data of @vm.
+ * Updates vCPU information private data of @vm. Due to historical reasons this
+ * function returns success even if some data were not reported by qemu.
  *
- * Returns number of detected vCPU threads on success, -1 on error and reports
- * an appropriate error, -2 if the domain doesn't exist any more.
+ * Returns 0 on success and -1 on fatal error.
  */
 int
 qemuDomainRefreshVcpuInfo(virQEMUDriverPtr driver,
@@ -5722,10 +5722,7 @@ qemuDomainRefreshVcpuInfo(virQEMUDriverPtr driver,
 if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
 return -1;
 ncpupids = qemuMonitorGetCPUInfo(qemuDomainGetMonitor(vm), );
-if (qemuDomainObjExitMonitor(driver, vm) < 0) {
-ret = -2;
-goto cleanup;
-}
+if (qemuDomainObjExitMonitor(driver, vm) < 0)

 /* failure to get the VCPU <-> PID mapping or to execute the query
  * command will not be treated fatal as some versions of qemu don't
@@ -5745,10 +5742,7 @@ qemuDomainRefreshVcpuInfo(virQEMUDriverPtr driver,
 QEMU_DOMAIN_VCPU_PRIVATE(vcpu)->tid = 0;
 }

-if (qemuDomainValidateVcpuInfo(vm) < 0)
-goto cleanup;
-
-ret = ncpupids;
+ret = 0;

  cleanup:
 VIR_FREE(cpupids);
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 453572b..2199258 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4615,6 +4615,7 @@ qemuDomainHotplugAddVcpu(virQEMUDriverPtr driver,
 {
 qemuDomainObjPrivatePtr priv = vm->privateData;
 virDomainVcpuDefPtr vcpuinfo = virDomainDefGetVcpu(vm->def, vcpu);
+qemuDomainVcpuPrivatePtr vcpupriv = QEMU_DOMAIN_VCPU_PRIVATE(vcpuinfo);
 int ret = -1;
 int rc;
 int oldvcpus = virDomainDefGetVcpus(vm->def);
@@ -4639,15 +4640,14 @@ qemuDomainHotplugAddVcpu(virQEMUDriverPtr driver,

 vcpuinfo->online = true;

-if ((rc = qemuDomainRefreshVcpuInfo(driver, vm, QEMU_ASYNC_JOB_NONE)) <= 
0) {
-/* vcpu pids were not detected, skip setting of affinity */
-if (rc == 0)
-ret = 0;
+if (qemuDomainRefreshVcpuInfo(driver, vm, QEMU_ASYNC_JOB_NONE) < 0)
+goto cleanup;

+if (qemuDomainValidateVcpuInfo(vm) < 0)
 goto cleanup;
-}

-if (qemuProcessSetupVcpu(vm, vcpu) < 0)
+if (vcpupriv->tid > 0 &&
+qemuProcessSetupVcpu(vm, vcpu) < 0)
 goto cleanup;

 ret = 0;
@@ -4689,11 +4689,11 @@ qemuDomainHotplugDelVcpu(virQEMUDriverPtr driver,
 goto cleanup;
 }

-if ((rc = qemuDomainRefreshVcpuInfo(driver, vm, QEMU_ASYNC_JOB_NONE)) < 0) 
{
-/* rollback only if domain didn't exit */
-if (rc == -2)
-goto cleanup;
+if (qemuDomainRefreshVcpuInfo(driver, vm, QEMU_ASYNC_JOB_NONE) < 0)
+goto cleanup;

+if (qemuDomainValidateVcpuInfo(vm) < 0) {
+/* rollback vcpu count if the setting has failed */
 virDomainAuditVcpu(vm, oldvcpus, oldvcpus - 1, "update", false);
 vcpuinfo->online = true;
 goto cleanup;
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 9136ba5..2e405af 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -5193,6 +5193,9 @@ qemuProcessLaunch(virConnectPtr conn,
 if (qemuDomainRefreshVcpuInfo(driver, vm, asyncJob) < 0)
 goto cleanup;

+if (qemuDomainValidateVcpuInfo(vm) < 0)
+goto cleanup;
+
 VIR_DEBUG("Detecting IOThread PIDs");
 if (qemuProcessDetectIOThreadPIDs(driver, vm, asyncJob) < 0)
 goto cleanup;
@@ -5985,6 +5988,9 @@ int qemuProcessAttach(virConnectPtr conn ATTRIBUTE_UNUSED,
 if (qemuDomainRefreshVcpuInfo(driver, vm, QEMU_ASYNC_JOB_NONE) < 0)
 goto error;

+if (qemuDomainValidateVcpuInfo(vm) < 0)
+goto error;
+
 VIR_DEBUG("Detecting IOThread PIDs");
 if (qemuProcessDetectIOThreadPIDs(driver, vm, QEMU_ASYNC_JOB_NONE) < 0)
 goto error;
-- 
2.9.2

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

[libvirt] [PATCH 10/10] qemu: monitor: Return struct from qemuMonitor(Text|Json)QueryCPUs

[Peter Krempa]

Prepare to extract more data by returning a array of structs rather than
just an array of thread ids. Additionally report fatal errors separately
from qemu not being able to produce data.
---
 src/qemu/qemu_monitor.c  | 31 ---
 src/qemu/qemu_monitor.h  |  6 
 src/qemu/qemu_monitor_json.c | 71 ++--
 src/qemu/qemu_monitor_json.h |  2 +-
 src/qemu/qemu_monitor_text.c | 37 +++
 src/qemu/qemu_monitor_text.h |  2 +-
 tests/qemumonitorjsontest.c  | 31 ++-
 7 files changed, 104 insertions(+), 76 deletions(-)

diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index 0011ceb..578b078 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -1666,6 +1666,16 @@ qemuMonitorCPUInfoFree(qemuMonitorCPUInfoPtr cpus,
 VIR_FREE(cpus);
 }

+void
+qemuMonitorQueryCpusFree(struct qemuMonitorQueryCpusEntry *entries,
+ size_t nentries ATTRIBUTE_UNUSED)
+{
+if (!entries)
+return;
+
+VIR_FREE(entries);
+}
+

 /**
  * qemuMonitorGetCPUInfo:
@@ -1686,10 +1696,10 @@ qemuMonitorGetCPUInfo(qemuMonitorPtr mon,
   size_t maxvcpus)
 {
 qemuMonitorCPUInfoPtr info = NULL;
-int *pids = NULL;
+struct qemuMonitorQueryCpusEntry *cpuentries = NULL;
 size_t i;
 int ret = -1;
-int rc;
+int ncpuentries;

 QEMU_CHECK_MONITOR(mon);

@@ -1697,25 +1707,26 @@ qemuMonitorGetCPUInfo(qemuMonitorPtr mon,
 return -1;

 if (mon->json)
-rc = qemuMonitorJSONQueryCPUs(mon, );
+ncpuentries = qemuMonitorJSONQueryCPUs(mon, );
 else
-rc = qemuMonitorTextQueryCPUs(mon, );
+ncpuentries = qemuMonitorTextQueryCPUs(mon, );
+
+if (ncpuentries < 0) {
+if (ncpuentries == -2)
+ret = 0;

-if (rc < 0) {
-virResetLastError();
-ret = 0;
 goto cleanup;
 }

-for (i = 0; i < rc; i++)
-info[i].tid = pids[i];
+for (i = 0; i < ncpuentries; i++)
+info[i].tid = cpuentries[i].tid;

 VIR_STEAL(*vcpus, info);
 ret = 0;

  cleanup:
 qemuMonitorCPUInfoFree(info, maxvcpus);
-VIR_FREE(pids);
+qemuMonitorQueryCpusFree(cpuentries, ncpuentries);
 return ret;
 }

diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
index 1ef002a..6022b9d 100644
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -390,6 +390,12 @@ int qemuMonitorGetStatus(qemuMonitorPtr mon,
 int qemuMonitorSystemReset(qemuMonitorPtr mon);
 int qemuMonitorSystemPowerdown(qemuMonitorPtr mon);

+struct qemuMonitorQueryCpusEntry {
+pid_t tid;
+};
+void qemuMonitorQueryCpusFree(struct qemuMonitorQueryCpusEntry *entries,
+  size_t nentries);
+

 struct _qemuMonitorCPUInfo {
 pid_t tid;
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 715e1c7..8018860 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -1323,69 +1323,65 @@ int qemuMonitorJSONSystemReset(qemuMonitorPtr mon)
  *   { "CPU": 1, "current": false, "halted": true, "pc": 7108165 } ]
  */
 static int
-qemuMonitorJSONExtractCPUInfo(virJSONValuePtr reply,
-  int **pids)
+qemuMonitorJSONExtractCPUInfo(virJSONValuePtr data,
+  struct qemuMonitorQueryCpusEntry **entries)
 {
-virJSONValuePtr data;
+struct qemuMonitorQueryCpusEntry *cpus = NULL;
 int ret = -1;
 size_t i;
-int *threads = NULL;
 ssize_t ncpus;

-if (!(data = virJSONValueObjectGetArray(reply, "return"))) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-   _("cpu reply was missing return data"));
-goto cleanup;
-}
-
-if ((ncpus = virJSONValueArraySize(data)) <= 0) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-   _("cpu information was empty"));
-goto cleanup;
-}
+if ((ncpus = virJSONValueArraySize(data)) <= 0)
+return -2;

-if (VIR_ALLOC_N(threads, ncpus) < 0)
+if (VIR_ALLOC_N(cpus, ncpus) < 0)
 goto cleanup;

 for (i = 0; i < ncpus; i++) {
 virJSONValuePtr entry = virJSONValueArrayGet(data, i);
-int thread;
+int thread = 0;
 if (!entry) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-   _("cpu information was missing an array element"));
+ret = -2;
 goto cleanup;
 }

-if (virJSONValueObjectGetNumberInt(entry, "thread_id", ) < 0) {
-/* Some older qemu versions don't report the thread_id,
- * so treat this as non-fatal, simply returning no data */
-ret = 0;
-goto cleanup;
-}
+/* Some older qemu versions don't report the thread_id so treat this as
+ * non-fatal, simply returning no data */
+ignore_value(virJSONValueObjectGetNumberInt(entry, "thread_id", 
));

[libvirt] [PATCH 02/10] internal: Introduce macro for stealing pointers

[Peter Krempa]

VIR_STEAL copies the second argument into the first and then sets it to
NULL. This is useful for stealing pointers.
---
 src/internal.h | 12 
 1 file changed, 12 insertions(+)

diff --git a/src/internal.h b/src/internal.h
index 0dc34c7..c633ee6 100644
--- a/src/internal.h
+++ b/src/internal.h
@@ -307,6 +307,18 @@
 } while (0)

 /**
+ * VIR_STEAL:
+ *
+ * Steals pointer passed as second argument into the first argument. Second
+ * argument must not have side effects.
+ */
+# define VIR_STEAL(a, b)  \
+do { \
+(a) = (b); \
+(b) = NULL;\
+} while (0)
+
+/**
  * virCheckFlags:
  * @supported: an OR'ed set of supported flags
  * @retval: return value in case unsupported flags were passed
-- 
2.9.2

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

Re: [libvirt] Release of libvirt-2.1.0

[Daniel Veillard]

On Tue, Aug 02, 2016 at 02:46:34PM +0100, Justin Clift wrote:
> On 2 Aug 2016, at 09:57, Daniel Veillard  wrote:
> >   Sorry I didn't push an rc2 on Friday, finishing my move, but not seeing 
> > any
> > issue raised by rc1, I though it was better to push the final release now 
> > and free
> > up the tree for pending development. As a result 2.1.0 is tagged in git and 
> > I
> > pushed signed tarbal and rpms to the usual place:
> > 
> >ftp://libvirt.org/libvirt/
> 
> It's working for OSX too. (whew!) :)
> 
> Libvirt 2.1.0 is now available through OSX Homebrew.

  Cool, thanks Justin !

Daniel

> + Justin
> 
> --
> "My grandfather once told me that there are two kinds of people: those
> who work and those who take the credit. He told me to try to be in the
> first group; there was less competition there."
> - Indira Gandhi

-- 
Daniel Veillard  | Open Source and Standards, Red Hat
veill...@redhat.com  | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
http://veillard.com/ | virtualization library  http://libvirt.org/

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