Re: [libvirt] [PATCH 0/2] Couple more Coverity issues

[John Ferlan]

On 08/28/2014 03:28 PM, John Ferlan wrote:
> After perusing the pile of 70 or so warnings - these two stuck out as
> ones that were low hanging fruit and not false positives.
> 
> Many of the remaining "issues" are false positives or perhaps even
> bugs in Coverity, but I understand why they're being flagged. Freeing
> memory from counted lists where the incoming count must be zero based
> on code path - for some reason Coverity flags them because the incoming
> list memory is NULL and the for loop deref would be bad. The issue
> is Coverity doesn't seem to dig deep enough to determine that the
> count and list pointer are linked, sigh (yes, a lot of those).
> 
> John Ferlan (2):
>   virnetserverservice: Resolve Coverity ARRAY_VS_SINGLETON
>   qemu_driver: Resolve Coverity FORWARD_NULL
> 
>  src/qemu/qemu_driver.c| 3 +--
>  src/rpc/virnetserverservice.c | 2 +-
>  2 files changed, 2 insertions(+), 3 deletions(-)
> 

Both are now pushed.

John

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

Re: [libvirt] [PATCH 1/2] virnetserverservice: Resolve Coverity ARRAY_VS_SINGLETON

[Eric Blake]

On 08/28/2014 04:04 PM, John Ferlan wrote:

>> ACK.
>>
> 
> For the benefit of others ;-) as Eric and I have been IRC'g over this...
> This was introduced in this release cycle (commit id '9805256d')...  So
> not in the wild yet (fairly recent commit of 8/22)
> 
> Although I know you ACK'd what I have... How about the attached instead?
> It avoids the auto incremented pointer arithmetic (but of course still
> has an assumption regarding fd's being sequential).  It looks like more
> changed only because of the formatting to use "ret = " instead of
> "return" - just needed one more character in my variable name to avoid
> lines of single space adjustment.

No, the short version of (*cur_fd)++ is just fine.

Basically, any time * and ++ (or --) appear in the same statement, it's
usually sufficient to just add () to make it explicit which precedence
you were intending.

A more invasive solution might be to rewrite the coordination between
daemon/libvirtd.c and virnetserverservice.c; right now, the semantics
are loosely:

int cur_fd = STDERR_FILENO + 1;
read-write:
virNetServerServiceNewFDOrUnix(..., &cur_fd);
read-only:
virNetServerServiceNewFDOrUnix(..., &cur_fd);

where the idea is that the code registers two services, and those
services are either attached to unix sockets (no change to cur_fd,
because it is not consuming fds) or to incoming file descriptors passed
in on the command line to main (and we have control over the program
exec'ing this, so we KNOW that fd 3 is the read-write and fd 4 is the
read-only incoming fd).  That is, virNetServerServiceNewFDrUinx is
either incrementing cur_fd on behalf of libvirtd.c, so that the second
call starts at the next fd.

But that feels magical enough that if you would just have:

int cur_fd = STDERR_FILENO + 1;
bool consumed;
read-write:
virNetServerServiceNewFDOrUnix(..., cur_fd, &consumed);
if (consumed)
cur_fd++;
read-only:
virNetServerServiceNewFDOrUnix(..., cur_fd, &consumed);

then this code wouldn't be incrementing the contents of a pointer that
will be used in the next call, so much as assigning into a boolean
pointer; and the outermost caller is then responsible for tracking how
fast it is moving through the nfds inherited during exec.

But for the sake of getting the release ready, simpler feels better, so
pushing your v1 patch is just fine.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



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

Re: [libvirt] [PATCH 1/2] virnetserverservice: Resolve Coverity ARRAY_VS_SINGLETON

[John Ferlan]

On 08/28/2014 04:57 PM, Eric Blake wrote:
> On 08/28/2014 01:28 PM, John Ferlan wrote:
>> Coverity complained about the following:
>>
>> (3) Event ptr_arith:
>>Performing pointer arithmetic on "cur_fd" in expression "cur_fd++".
>> 130 return virNetServerServiceNewFD(*cur_fd++,
>>
>> It seems the belief is their is pointer arithmetic taking place.  By using
>> (*cur_fd)++ we avoid this possible issue
> 
> Not just their belief, but the actual C language.
> 

So yes, I was being a little facetious too. I cut off part of an
original "comment" regarding auto increment pointer arithmetic... It is
almost always prime for "issues" - one of those language constructs
useful for so many things, but oh so dangerous.

>>
>> Signed-off-by: John Ferlan 
>> ---
>>  src/rpc/virnetserverservice.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/src/rpc/virnetserverservice.c b/src/rpc/virnetserverservice.c
>> index fea05c3..486f93e 100644
>> --- a/src/rpc/virnetserverservice.c
>> +++ b/src/rpc/virnetserverservice.c
>> @@ -127,7 +127,7 @@ virNetServerServiceNewFDOrUNIX(const char *path,
>>   * There's still enough file descriptors.  In this case we'll
>>   * use the current one and increment it afterwards.
>>   */
>> -return virNetServerServiceNewFD(*cur_fd++,
> 
> In this function, cur_fd is int* (four bytes wide).  Suppose cur_fd
> starts life as 0x1000, and we start with memory contents of 3 at 0x1000,
> and 4 at 0x1004.
> 
> C precedence says this is equivalent to *(cur_fd++) - take the pointer
> cur_fd, do a pointer post-increment, then dereference the memory at the
> location before the increment.  We end up calling
> virNetServerServiceNewFD(3) (the contents of cur_fd before the deref),
> the memory at 0x1000 remains at 3, and any further uses of cur_fd would
> be at the next location 0x1004 - but we just returned so cur_fd is no
> longer in scope, so who cares about that change - we could have just
> written '*cur_fd' and been done with it.
> 
>> +return virNetServerServiceNewFD((*cur_fd)++,
> 
> While this says dereference cur_fd, then post-increment that location.
> We still end up calling virNetServerServiceNewFD(3) (the contents of the
> memory location before post-increment), but now cur_fd remains at
> 0x1000, whose contents are now 4.
> 
> The caller, daemon/libvirtd.c:daemonSetupNetworking(), calls this
> function twice in a row.  Without your patch, it ends up calling
> virNetServerServiceNewFD(3) for regular AND read-only sockets.  With
> your patch, it does the intended registration of fd 3 and 4.  I hope
> that's not a security bug.  Can you trace when this was introduced?
> 
> ACK.
> 

For the benefit of others ;-) as Eric and I have been IRC'g over this...
This was introduced in this release cycle (commit id '9805256d')...  So
not in the wild yet (fairly recent commit of 8/22)

Although I know you ACK'd what I have... How about the attached instead?
It avoids the auto incremented pointer arithmetic (but of course still
has an assumption regarding fd's being sequential).  It looks like more
changed only because of the formatting to use "ret = " instead of
"return" - just needed one more character in my variable name to avoid
lines of single space adjustment.

John

>From 438ee86a8bd2f060ce87601d60636f92a88932d1 Mon Sep 17 00:00:00 2001
From: John Ferlan 
Date: Thu, 28 Aug 2014 09:03:47 -0400
Subject: [PATCH 1/2] virnetserverservice: Resolve Coverity ARRAY_VS_SINGLETON

Coverity complained about the following:

(3) Event ptr_arith:
   Performing pointer arithmetic on "cur_fd" in expression "cur_fd++".
130 return virNetServerServiceNewFD(*cur_fd++,

The complain is that pointer arithmetic taking place instead of the
expected auto increment of the variable...  Rework the code to be a bit
more careful and void auto increment arithmetic on a pointer.

Signed-off-by: John Ferlan 
---
 src/rpc/virnetserverservice.c | 35 +--
 1 file changed, 21 insertions(+), 14 deletions(-)

diff --git a/src/rpc/virnetserverservice.c b/src/rpc/virnetserverservice.c
index fea05c3..d6b4024 100644
--- a/src/rpc/virnetserverservice.c
+++ b/src/rpc/virnetserverservice.c
@@ -106,36 +106,43 @@ virNetServerServiceNewFDOrUNIX(const char *path,
unsigned int nfds,
unsigned int *cur_fd)
 {
+virNetServerServicePtr ret;
 if (*cur_fd - STDERR_FILENO > nfds) {
 /*
  * There are no more file descriptors to use, so we have to
  * fallback to UNIX socket.
  */
-return virNetServerServiceNewUNIX(path,
-  mask,
-  grp,
-  auth,
+ret = virNetServerServiceNewUNIX(path,
+ mask,
+ grp,
+ 

Re: [libvirt] [PATCH v2 0/4] Introduce support for virtio-blk-pci iothreads

[John Ferlan]

On 08/27/2014 05:35 PM, John Ferlan wrote:
> 
> 
> On 08/26/2014 06:15 PM, John Ferlan wrote:
>> v1:
>> http://www.redhat.com/archives/libvir-list/2014-August/msg01155.html
>>
>> Changes since v1
>>
>> Patches 1-3 - purely from code review
>> Patch 4 - rework the checking of the to be added disk that has the iothread
>>  property set to be done during qemuBuildDriveDevStr() after the config
>>  check. This way the same checks are done for both start and hotplug.
>>
>>  Only set the "inuse" bit after qemuBuildDriveDevStr() returns 
>> successfully
>>  for both start and hotplug. This also enforces only setting for this 
>> path
>>
>>  Since the only way a disk with the property can be added is if the 
>> current
>>  emulator supports the feature, the calls to set/clear the bit if 
>> iothread
>>  is set should be safe from not needing to also ensure iothreadmap 
>> exists.
>>
>> John Ferlan (4):
>>   domain_conf: Introduce iothreads XML
>>   qemu: Add support for iothreads
>>   domain_conf: Add support for iothreads in disk definition
>>   qemu: Allow use of iothreads for disk definitions
>>
>>  docs/formatdomain.html.in  | 34 
>>  docs/schemas/domaincommon.rng  | 14 +
>>  src/conf/domain_conf.c | 47 +++-
>>  src/conf/domain_conf.h |  4 ++
>>  src/qemu/qemu_capabilities.c   |  2 +
>>  src/qemu/qemu_capabilities.h   |  1 +
>>  src/qemu/qemu_command.c| 64 
>> ++
>>  src/qemu/qemu_hotplug.c|  6 ++
>>  .../qemuxml2argv-iothreads-disk.args   | 17 ++
>>  .../qemuxml2argv-iothreads-disk.xml| 40 ++
>>  tests/qemuxml2argvdata/qemuxml2argv-iothreads.args |  8 +++
>>  tests/qemuxml2argvdata/qemuxml2argv-iothreads.xml  | 29 ++
>>  tests/qemuxml2argvtest.c   |  4 ++
>>  tests/qemuxml2xmltest.c|  2 +
>>  14 files changed, 271 insertions(+), 1 deletion(-)
>>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-iothreads-disk.args
>>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-iothreads-disk.xml
>>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-iothreads.args
>>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-iothreads.xml
>>
> 
> Given Stephan's comment about allowing more than 1 disk per IOThread - I
> have removed the iothreadmap (and other remnants including docs).
> 
> If folks want to see the changes before pushing that's fine, but since I
> figure I'm just removing code/checks - it'd still be "OK" for push.  I
> will "wait" for an answer knowing the sleep/work cycles of the various
> interested parties are not the same as mine :-)
> 
>

I pushed the changes without the iothreadmap

John

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

Re: [libvirt] [PATCH 2/2] qemu_driver: Resolve Coverity FORWARD_NULL

[Eric Blake]

On 08/28/2014 01:28 PM, John Ferlan wrote:
> In qemuDomainSnapshotCreateDiskActive() if we jumped to cleanup from a
> failed actions = virJSONValueNewArray(), then 'cfg' would be NULL.
> 
> So just return -1, which in turn removes the need for cleanup:
> 
> Signed-off-by: John Ferlan 
> ---
>  src/qemu/qemu_driver.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)

ACK.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



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

Re: [libvirt] [PATCH 1/2] virnetserverservice: Resolve Coverity ARRAY_VS_SINGLETON

[Eric Blake]

On 08/28/2014 01:28 PM, John Ferlan wrote:
> Coverity complained about the following:
> 
> (3) Event ptr_arith:
>Performing pointer arithmetic on "cur_fd" in expression "cur_fd++".
> 130 return virNetServerServiceNewFD(*cur_fd++,
> 
> It seems the belief is their is pointer arithmetic taking place.  By using
> (*cur_fd)++ we avoid this possible issue

Not just their belief, but the actual C language.

> 
> Signed-off-by: John Ferlan 
> ---
>  src/rpc/virnetserverservice.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/rpc/virnetserverservice.c b/src/rpc/virnetserverservice.c
> index fea05c3..486f93e 100644
> --- a/src/rpc/virnetserverservice.c
> +++ b/src/rpc/virnetserverservice.c
> @@ -127,7 +127,7 @@ virNetServerServiceNewFDOrUNIX(const char *path,
>   * There's still enough file descriptors.  In this case we'll
>   * use the current one and increment it afterwards.
>   */
> -return virNetServerServiceNewFD(*cur_fd++,

In this function, cur_fd is int* (four bytes wide).  Suppose cur_fd
starts life as 0x1000, and we start with memory contents of 3 at 0x1000,
and 4 at 0x1004.

C precedence says this is equivalent to *(cur_fd++) - take the pointer
cur_fd, do a pointer post-increment, then dereference the memory at the
location before the increment.  We end up calling
virNetServerServiceNewFD(3) (the contents of cur_fd before the deref),
the memory at 0x1000 remains at 3, and any further uses of cur_fd would
be at the next location 0x1004 - but we just returned so cur_fd is no
longer in scope, so who cares about that change - we could have just
written '*cur_fd' and been done with it.

> +return virNetServerServiceNewFD((*cur_fd)++,

While this says dereference cur_fd, then post-increment that location.
We still end up calling virNetServerServiceNewFD(3) (the contents of the
memory location before post-increment), but now cur_fd remains at
0x1000, whose contents are now 4.

The caller, daemon/libvirtd.c:daemonSetupNetworking(), calls this
function twice in a row.  Without your patch, it ends up calling
virNetServerServiceNewFD(3) for regular AND read-only sockets.  With
your patch, it does the intended registration of fd 3 and 4.  I hope
that's not a security bug.  Can you trace when this was introduced?

ACK.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



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

Re: [libvirt] [PATCH v2 2/2] network: Bring netdevs online later

[Matthew Rosato]

On 08/28/2014 09:56 AM, Martin Kletzander wrote:
> On Wed, Aug 27, 2014 at 10:34:14AM -0400, Matthew Rosato wrote:
>> Currently, MAC registration occurs during device creation, which is
>> early enough that, during live migration, you end up with duplicate
>> MAC addresses on still-running source and target devices, even though
>> the target device isn't actually being used yet.
>> This patch proposes to defer MAC registration until right before
>> the guest can actually use the device -- In other words, right
>> before starting guest CPUs.
>>
>> Signed-off-by: Matthew Rosato 
>> ---
>> src/Makefile.am |3 +-
>> src/conf/domain_conf.h  |2 ++
>> src/lxc/lxc_process.c   |4 ++-
>> src/qemu/qemu_command.c |6 +++-
>> src/qemu/qemu_hotplug.c |7 
>> src/qemu/qemu_interface.c   |   78
>> +++
>> src/qemu/qemu_interface.h   |   32 ++
>> src/qemu/qemu_process.c |4 +++
>> src/util/virnetdevmacvlan.c |8 +++--
>> src/util/virnetdevmacvlan.h |2 ++
>> 10 files changed, 140 insertions(+), 6 deletions(-)
>> create mode 100644 src/qemu/qemu_interface.c
>> create mode 100644 src/qemu/qemu_interface.h
>>
>> diff --git a/src/Makefile.am b/src/Makefile.am
>> index 46e411e..842573f 100644
>> --- a/src/Makefile.am
>> +++ b/src/Makefile.am
>> @@ -703,7 +703,8 @@ QEMU_DRIVER_SOURCES =\
>> qemu/qemu_monitor_text.h\
>> qemu/qemu_monitor_json.c\
>> qemu/qemu_monitor_json.h\
>> -qemu/qemu_driver.c qemu/qemu_driver.h
>> +qemu/qemu_driver.c qemu/qemu_driver.h   \
> 
> I don't know if this got mixed by the mail client or not, but I'm
> adjusting it (the backslash) to match the others.
> 
>> +qemu/qemu_interface.c qemu/qemu_interface.h
>>
> 
> I still don't see anything qemu-specific inthose files, but for now
> separate files are fine, we can move the code around any time later if
> it's needed.

I mentioned this in the cover letter comments, but you may have missed it:

" * I left the contents of qemu_interface as-is, rather than collapsing
them into non-qemu-specific functions, in order to keep Makefile linkage
consistent & happy (needs to be part of QEMU_DRIVER_SOURCES).  Instead,
incorporated copyright suggestions from previous comments.  "

Basically, you're right in that there is nothing inherently
qemu-specific in this code, just that it's currently only being written
with qemu in mind, and it sticks with the current Makefile convention
(everything in QEMU_DRIVER_SOURCES is part of src/qemu/qemu_*).

> 
>> XENAPI_DRIVER_SOURCES =\
>> xenapi/xenapi_driver.c xenapi/xenapi_driver.h\
>> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
>> index aead903..6268690 100644
>> --- a/src/conf/domain_conf.h
>> +++ b/src/conf/domain_conf.h
>> @@ -950,6 +950,8 @@ struct _virDomainNetDef {
>> virNetDevBandwidthPtr bandwidth;
>> virNetDevVlan vlan;
>> int linkstate;
>> +/* vmOp value saved if deferring interface start */
>> +virNetDevVPortProfileOp vmOp;
>> };
>>
>> /* Used for prefix of ifname of any network name generated dynamically
>> diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c
>> index ed30c37..b2256c0 100644
>> --- a/src/lxc/lxc_process.c
>> +++ b/src/lxc/lxc_process.c
>> @@ -300,6 +300,7 @@ char
>> *virLXCProcessSetupInterfaceDirect(virConnectPtr conn,
>> virNetDevBandwidthPtr bw;
>> virNetDevVPortProfilePtr prof;
>> virLXCDriverConfigPtr cfg = virLXCDriverGetConfig(driver);
>> +unsigned int macvlan_create_flags = VIR_NETDEV_MACVLAN_CREATE_IFUP;
>>
>> /* XXX how todo bandwidth controls ?
>>  * Since the 'net-ifname' is about to be moved to a different
>> @@ -336,7 +337,8 @@ char
>> *virLXCProcessSetupInterfaceDirect(virConnectPtr conn,
>> &res_ifname,
>> VIR_NETDEV_VPORT_PROFILE_OP_CREATE,
>> cfg->stateDir,
>> -virDomainNetGetActualBandwidth(net), 0) < 0)
>> +virDomainNetGetActualBandwidth(net),
>> +macvlan_create_flags) < 0)
>> goto cleanup;
>>
>> ret = res_ifname;
>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>> index 1f71fa3..43a8b63 100644
>> --- a/src/qemu/qemu_command.c
>> +++ b/src/qemu/qemu_command.c
>> @@ -199,6 +199,9 @@ qemuPhysIfaceConnect(virDomainDefPtr def,
>> net->ifname = res_ifname;
>> }
>>
>> +/* Save vport profile op for later */
>> +net->vmOp = vmop;
>> +
>> virObjectUnref(cfg);
>> return rc;
>> }
>> @@ -286,7 +289,7 @@ qemuNetworkIfaceConnect(virDomainDefPtr def,
>> {
>> char *brname = NULL;
>> int ret = -1;
>> -unsigned int tap_create_flags = VIR_NETDEV_TAP_CREATE_IFUP;
>> +unsigned int tap_create_flags = 0;
>> bool template_ifname = false;
>> int actualType = virDomainNetGetActualType(net);
>> virQEMUDriverConfigPt

[libvirt] [PATCH 2/2] qemu_driver: Resolve Coverity FORWARD_NULL

[John Ferlan]

In qemuDomainSnapshotCreateDiskActive() if we jumped to cleanup from a
failed actions = virJSONValueNewArray(), then 'cfg' would be NULL.

So just return -1, which in turn removes the need for cleanup:

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

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 5d21080..239a300 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -13023,7 +13023,7 @@ qemuDomainSnapshotCreateDiskActive(virQEMUDriverPtr 
driver,
 
 if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_TRANSACTION)) {
 if (!(actions = virJSONValueNewArray()))
-goto cleanup;
+return -1;
 } else if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DISK_SNAPSHOT)) {
 virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
_("live disk snapshot not supported with this "
@@ -13106,7 +13106,6 @@ qemuDomainSnapshotCreateDiskActive(virQEMUDriverPtr 
driver,
 }
 }
 
- cleanup:
 /* recheck backing chains of all disks involved in the snapshot */
 orig_err = virSaveLastError();
 for (i = 0; i < snap->def->ndisks; i++) {
-- 
1.9.3

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

[libvirt] [PATCH 1/2] virnetserverservice: Resolve Coverity ARRAY_VS_SINGLETON

[John Ferlan]

Coverity complained about the following:

(3) Event ptr_arith:
   Performing pointer arithmetic on "cur_fd" in expression "cur_fd++".
130 return virNetServerServiceNewFD(*cur_fd++,

It seems the belief is their is pointer arithmetic taking place.  By using
(*cur_fd)++ we avoid this possible issue

Signed-off-by: John Ferlan 
---
 src/rpc/virnetserverservice.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/rpc/virnetserverservice.c b/src/rpc/virnetserverservice.c
index fea05c3..486f93e 100644
--- a/src/rpc/virnetserverservice.c
+++ b/src/rpc/virnetserverservice.c
@@ -127,7 +127,7 @@ virNetServerServiceNewFDOrUNIX(const char *path,
  * There's still enough file descriptors.  In this case we'll
  * use the current one and increment it afterwards.
  */
-return virNetServerServiceNewFD(*cur_fd++,
+return virNetServerServiceNewFD((*cur_fd)++,
 auth,
 #if WITH_GNUTLS
 tls,
-- 
1.9.3

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

[libvirt] [PATCH 0/2] Couple more Coverity issues

[John Ferlan]

After perusing the pile of 70 or so warnings - these two stuck out as
ones that were low hanging fruit and not false positives.

Many of the remaining "issues" are false positives or perhaps even
bugs in Coverity, but I understand why they're being flagged. Freeing
memory from counted lists where the incoming count must be zero based
on code path - for some reason Coverity flags them because the incoming
list memory is NULL and the for loop deref would be bad. The issue
is Coverity doesn't seem to dig deep enough to determine that the
count and list pointer are linked, sigh (yes, a lot of those).

John Ferlan (2):
  virnetserverservice: Resolve Coverity ARRAY_VS_SINGLETON
  qemu_driver: Resolve Coverity FORWARD_NULL

 src/qemu/qemu_driver.c| 3 +--
 src/rpc/virnetserverservice.c | 2 +-
 2 files changed, 2 insertions(+), 3 deletions(-)

-- 
1.9.3

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

Re: [libvirt] [PATCH 01/19] libxl_migration: Resolve Coverity NULL_RETURNS

[John Ferlan]

On 08/28/2014 11:20 AM, Jim Fehlig wrote:
> John Ferlan wrote:

>>> 
>>
>> I am going to remove this one from my series and let you handle it.
>>   
> 
> After looking at the code again, I think it is safest to go with your patch.
> 
>> I would seem perhaps that the code was there to ensure that if either of
>> the calls to virDomainObjSetState() ended up resulting in 'dom' not
>> returning something from the virGetDomain(), then like perhaps the qemu
>> migration code, the "best choice" was to remove it.
> 
> Yep, agreed.  I haven't convinced myself that it is impossible for
> virGetDomain() to return a NULL domainPtr, but in the event it does I
> agree it is best to remove the domain.
> 
> Regards,
> Jim
> 

Close enough to an ACK for me ;-)

I've pushed the change

Thanks

John

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

Re: [libvirt] [PATCH 1.2.8] storage: zfs: fix double listing of new volumes

[John Ferlan]

On 08/27/2014 05:02 AM, Roman Bogorodskiy wrote:
> Currently, after calling commands to create a new volumes,
> virStorageBackendZFSCreateVol calls virStorageBackendZFSFindVols that
> calls virStorageBackendZFSParseVol.
> 
> virStorageBackendZFSParseVol checks if a volume already exists by
> trying to get it using virStorageVolDefFindByName.
> 
> For a just created volume it returns NULL, so volume is reported as
> new and appended to pool->volumes. This causes a volume to be listed
> twice as storageVolCreateXML appends this new volume to the list as
> well.
> 
> Fix that by passing a new volume definition to
> virStorageBackendZFSParseVol so it could determine if it needs to add
> this volume to the list.
> ---
>  src/storage/storage_backend_zfs.c | 45 
> ++-
>  1 file changed, 26 insertions(+), 19 deletions(-)
> 

ACK

Although it seems the "main" reason the create backend called the
FindVols was to ascertain if the volume was successfully created via the
virCommandRun call.

I believe this is safe for 1.2.8

John

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

[libvirt] [PATCH 3/4] add an example how to use cputune event

[Pavel Hrdina]

Signed-off-by: Pavel Hrdina 
---
 examples/object-events/event-test.c | 39 -
 1 file changed, 38 insertions(+), 1 deletion(-)

diff --git a/examples/object-events/event-test.c 
b/examples/object-events/event-test.c
index d6cfe46..afdc7f1 100644
--- a/examples/object-events/event-test.c
+++ b/examples/object-events/event-test.c
@@ -464,6 +464,35 @@ static int myNetworkEventCallback(virConnectPtr conn 
ATTRIBUTE_UNUSED,
 return 0;
 }
 
+static int
+myDomainEventCputuneCallback(virConnectPtr conn ATTRIBUTE_UNUSED,
+ virDomainPtr dom,
+ virDomainCputuneInfoPtr cputune,
+ void *opaque ATTRIBUTE_UNUSED)
+{
+printf("%s EVENT: Domain %s(%d) cputune updated:\n",
+   __func__, virDomainGetName(dom), virDomainGetID(dom));
+if (cputune->sharesSpecified) {
+printf("\tshares: %llu\n", cputune->shares);
+} else {
+printf("\tshares: not specified\n");
+}
+printf("\tperiod: %llu\n\tquota: %lld\n", cputune->period, cputune->quota);
+printf("\temulator_period: %llu\n\temulator_quota: %lld\n",
+   cputune->emulatorPeriod, cputune->emulatorQuota);
+if (cputune->emulatorpin.map) {
+printf("\temulatorpin: %x\n", *cputune->emulatorpin.map);
+}
+if (cputune->vcpupin) {
+size_t i;
+for (i = 0; i < cputune->nvcpupin; i++) {
+printf("\tvcpupin (vcpuid: %d): %x\n",
+   cputune->vcpupin[i].vcpuid,
+   *cputune->vcpupin[i].cpumask.map);
+}
+}
+return 0;
+}
 
 static void myFreeFunc(void *opaque)
 {
@@ -506,6 +535,7 @@ int main(int argc, char **argv)
 int callback14ret = -1;
 int callback15ret = -1;
 int callback16ret = -1;
+int callback17ret = -1;
 struct sigaction action_stop;
 
 memset(&action_stop, 0, sizeof(action_stop));
@@ -624,6 +654,11 @@ int main(int argc, char **argv)
   
VIR_NETWORK_EVENT_ID_LIFECYCLE,
   
VIR_NETWORK_EVENT_CALLBACK(myNetworkEventCallback),
   strdup("net callback"), 
myFreeFunc);
+callback17ret = virConnectDomainEventRegisterAny(dconn,
+ NULL,
+ 
VIR_DOMAIN_EVENT_ID_CPUTUNE,
+ 
VIR_DOMAIN_EVENT_CALLBACK(myDomainEventCputuneCallback),
+ strdup("cputune"), 
myFreeFunc);
 
 if ((callback1ret != -1) &&
 (callback2ret != -1) &&
@@ -639,7 +674,8 @@ int main(int argc, char **argv)
 (callback13ret != -1) &&
 (callback14ret != -1) &&
 (callback15ret != -1) &&
-(callback16ret != -1)) {
+(callback16ret != -1) &&
+(callback17ret != -1)) {
 if (virConnectSetKeepAlive(dconn, 5, 3) < 0) {
 virErrorPtr err = virGetLastError();
 fprintf(stderr, "Failed to start keepalive protocol: %s\n",
@@ -671,6 +707,7 @@ int main(int argc, char **argv)
 virConnectDomainEventDeregisterAny(dconn, callback14ret);
 virConnectDomainEventDeregisterAny(dconn, callback15ret);
 virConnectNetworkEventDeregisterAny(dconn, callback16ret);
+virConnectDomainEventDeregisterAny(dconn, callback17ret);
 if (callback8ret != -1)
 virConnectDomainEventDeregisterAny(dconn, callback8ret);
 }
-- 
1.8.5.5

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

[libvirt] [PATCH 2/4] event: introduce new event for cputune

[Pavel Hrdina]

Signed-off-by: Pavel Hrdina 
---
 daemon/remote.c  |  87 +++
 include/libvirt/libvirt.h.in |  62 ++
 src/conf/domain_event.c  | 120 +++
 src/conf/domain_event.h  |   7 +++
 src/libvirt_private.syms |   2 +
 src/remote/remote_driver.c   | 110 +++
 src/remote/remote_protocol.x |  39 +-
 src/remote_protocol-structs  |  32 
 tools/virsh-domain.c |  49 ++
 9 files changed, 507 insertions(+), 1 deletion(-)

diff --git a/daemon/remote.c b/daemon/remote.c
index 89714ca..ae42c4d 100644
--- a/daemon/remote.c
+++ b/daemon/remote.c
@@ -969,6 +969,92 @@ remoteRelayDomainEventBlockJob2(virConnectPtr conn,
 }
 
 
+static int
+remoteRelayDomainEventCputune(virConnectPtr conn,
+  virDomainPtr dom,
+  virDomainCputuneInfoPtr cputune,
+  void *opaque)
+{
+daemonClientEventCallbackPtr callback = opaque;
+remote_domain_event_cputune_msg data;
+size_t i;
+
+if (callback->callbackID < 0 ||
+!remoteRelayDomainEventCheckACL(callback->client, conn, dom))
+return -1;
+
+VIR_DEBUG("Relaying domain cputune event %s %d, callback %d",
+  dom->name, dom->id, callback->callbackID);
+
+/* build return data */
+memset(&data, 0, sizeof(data));
+make_nonnull_domain(&data.dom, dom);
+
+data.shares = cputune->shares;
+data.sharesSpecified = cputune->sharesSpecified;
+data.period = cputune->period;
+data.quota = cputune->quota;
+data.emulatorPeriod = cputune->emulatorPeriod;
+data.emulatorQuota = cputune->emulatorQuota;
+data.nvcpupin = cputune->nvcpupin;
+
+if (cputune->emulatorpin.map) {
+if (VIR_ALLOC_N(data.emulatorpin.map.map_val,
+cputune->emulatorpin.mapLen) < 0)
+goto error;
+memcpy(data.emulatorpin.map.map_val, cputune->emulatorpin.map,
+   cputune->emulatorpin.mapLen);
+data.emulatorpin.map.map_len = cputune->emulatorpin.mapLen;
+data.emulatorpin.mapLen = cputune->emulatorpin.mapLen;
+}
+if (cputune->vcpupin) {
+if (VIR_ALLOC_N(data.vcpupin.vcpupin_val, data.nvcpupin) < 0)
+goto error;
+data.vcpupin.vcpupin_len = data.nvcpupin;
+
+for (i = 0; i < data.nvcpupin; i++) {
+data.vcpupin.vcpupin_val[i].vcpuid = cputune->vcpupin[i].vcpuid;
+if (VIR_ALLOC_N(data.vcpupin.vcpupin_val[i].cpumask.map.map_val,
+cputune->vcpupin[i].cpumask.mapLen) < 0)
+goto error;
+memcpy(data.vcpupin.vcpupin_val[i].cpumask.map.map_val,
+   cputune->vcpupin[i].cpumask.map,
+   cputune->vcpupin[i].cpumask.mapLen);
+data.vcpupin.vcpupin_val[i].cpumask.map.map_len =
+cputune->vcpupin[i].cpumask.mapLen;
+data.vcpupin.vcpupin_val[i].cpumask.mapLen =
+cputune->vcpupin[i].cpumask.mapLen;
+}
+}
+
+if (callback->legacy) {
+remoteDispatchObjectEventSend(callback->client, remoteProgram,
+  REMOTE_PROC_DOMAIN_EVENT_CPUTUNE,
+  
(xdrproc_t)xdr_remote_domain_event_cputune_msg,
+  &data);
+} else {
+remote_domain_event_callback_cputune_msg msg = { callback->callbackID,
+ data };
+
+remoteDispatchObjectEventSend(callback->client, remoteProgram,
+  
REMOTE_PROC_DOMAIN_EVENT_CALLBACK_CPUTUNE,
+  
(xdrproc_t)xdr_remote_domain_event_callback_cputune_msg,
+  &msg);
+}
+
+return 0;
+
+ error:
+VIR_FREE(data.emulatorpin.map.map_val);
+if (data.vcpupin.vcpupin_val) {
+for (i = 0; i < data.nvcpupin; i++)
+VIR_FREE(data.vcpupin.vcpupin_val[i].cpumask.map.map_val);
+VIR_FREE(data.vcpupin.vcpupin_val);
+}
+return -1;
+}
+
+
 static virConnectDomainEventGenericCallback domainEventCallbacks[] = {
 VIR_DOMAIN_EVENT_CALLBACK(remoteRelayDomainEventLifecycle),
 VIR_DOMAIN_EVENT_CALLBACK(remoteRelayDomainEventReboot),
@@ -987,6 +1073,7 @@ static virConnectDomainEventGenericCallback 
domainEventCallbacks[] = {
 VIR_DOMAIN_EVENT_CALLBACK(remoteRelayDomainEventPMSuspendDisk),
 VIR_DOMAIN_EVENT_CALLBACK(remoteRelayDomainEventDeviceRemoved),
 VIR_DOMAIN_EVENT_CALLBACK(remoteRelayDomainEventBlockJob2),
+VIR_DOMAIN_EVENT_CALLBACK(remoteRelayDomainEventCputune),
 };
 
 verify(ARRAY_CARDINALITY(domainEventCallbacks) == VIR_DOMAIN_EVENT_ID_LAST);
diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
index 9358314..636b89b 100644
--- a/include/libvirt/libvirt.h.in
+++ b/i

[libvirt] [PATCH 4/4] cputune_event: queue the event for cputune updates

[Pavel Hrdina]

Signed-off-by: Pavel Hrdina 
---
 src/qemu/qemu_cgroup.c |  6 ++
 src/qemu/qemu_driver.c | 27 +++
 2 files changed, 33 insertions(+)

diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
index 43d14d4..95cc4d4 100644
--- a/src/qemu/qemu_cgroup.c
+++ b/src/qemu/qemu_cgroup.c
@@ -676,6 +676,7 @@ static int
 qemuSetupCpuCgroup(virDomainObjPtr vm)
 {
 qemuDomainObjPrivatePtr priv = vm->privateData;
+virObjectEventPtr event;
 
 if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPU)) {
if (vm->def->cputune.sharesSpecified) {
@@ -695,6 +696,11 @@ qemuSetupCpuCgroup(virDomainObjPtr vm)
 if (virCgroupGetCpuShares(priv->cgroup, &val) < 0)
 return -1;
 vm->def->cputune.shares = val;
+
+event = virDomainEventCputuneNewFromObj(vm, vm->def->cputune);
+
+if (event)
+qemuDomainEventQueue(vm->privateData, event);
 }
 
 return 0;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 5d21080..e2fedaf 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4461,6 +4461,7 @@ qemuDomainPinVcpuFlags(virDomainPtr dom,
 virBitmapPtr pcpumap = NULL;
 virQEMUDriverConfigPtr cfg = NULL;
 virCapsPtr caps = NULL;
+virObjectEventPtr event = NULL;
 
 virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
   VIR_DOMAIN_AFFECT_CONFIG, -1);
@@ -4568,6 +4569,8 @@ qemuDomainPinVcpuFlags(virDomainPtr dom,
 
 if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0)
 goto cleanup;
+
+event = virDomainEventCputuneNewFromDom(dom, vm->def->cputune);
 }
 
 if (flags & VIR_DOMAIN_AFFECT_CONFIG) {
@@ -4603,6 +4606,8 @@ qemuDomainPinVcpuFlags(virDomainPtr dom,
 virCgroupFree(&cgroup_vcpu);
 if (vm)
 virObjectUnlock(vm);
+if (event)
+qemuDomainEventQueue(driver, event);
 virBitmapFree(pcpumap);
 virObjectUnref(caps);
 virObjectUnref(cfg);
@@ -4727,6 +4732,7 @@ qemuDomainPinEmulator(virDomainPtr dom,
 virBitmapPtr pcpumap = NULL;
 virQEMUDriverConfigPtr cfg = NULL;
 virCapsPtr caps = NULL;
+virObjectEventPtr event = NULL;
 
 virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
   VIR_DOMAIN_AFFECT_CONFIG, -1);
@@ -4832,6 +4838,8 @@ qemuDomainPinEmulator(virDomainPtr dom,
 
 if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0)
 goto cleanup;
+
+event = virDomainEventCputuneNewFromDom(dom, vm->def->cputune);
 }
 
 if (flags & VIR_DOMAIN_AFFECT_CONFIG) {
@@ -4861,6 +4869,8 @@ qemuDomainPinEmulator(virDomainPtr dom,
  cleanup:
 if (cgroup_emulator)
 virCgroupFree(&cgroup_emulator);
+if (event)
+qemuDomainEventQueue(driver, event);
 virBitmapFree(pcpumap);
 virObjectUnref(caps);
 if (vm)
@@ -9053,6 +9063,8 @@ qemuDomainSetSchedulerParametersFlags(virDomainPtr dom,
 virQEMUDriverConfigPtr cfg = NULL;
 virCapsPtr caps = NULL;
 qemuDomainObjPrivatePtr priv;
+virObjectEventPtr event = NULL;
+bool emitEvent = false;
 
 virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
   VIR_DOMAIN_AFFECT_CONFIG, -1);
@@ -9123,6 +9135,8 @@ qemuDomainSetSchedulerParametersFlags(virDomainPtr dom,
 
 vm->def->cputune.shares = val;
 vm->def->cputune.sharesSpecified = true;
+
+emitEvent = true;
 }
 
 if (flags & VIR_DOMAIN_AFFECT_CONFIG) {
@@ -9140,6 +9154,8 @@ qemuDomainSetSchedulerParametersFlags(virDomainPtr dom,
 goto cleanup;
 
 vm->def->cputune.period = value_ul;
+
+emitEvent = true;
 }
 
 if (flags & VIR_DOMAIN_AFFECT_CONFIG)
@@ -9154,6 +9170,8 @@ qemuDomainSetSchedulerParametersFlags(virDomainPtr dom,
 goto cleanup;
 
 vm->def->cputune.quota = value_l;
+
+emitEvent = true;
 }
 
 if (flags & VIR_DOMAIN_AFFECT_CONFIG)
@@ -9169,6 +9187,8 @@ qemuDomainSetSchedulerParametersFlags(virDomainPtr dom,
 goto cleanup;
 
 vm->def->cputune.emulator_period = value_ul;
+
+emitEvent = true;
 }
 
 if (flags & VIR_DOMAIN_AFFECT_CONFIG)
@@ -9184,6 +9204,8 @@ qemuDomainSetSchedulerParametersFlags(virDomainPtr dom,
 goto cleanup;
 
 vm->def->cputune.emulator_quota = value_l;
+
+emitEvent = true;
 }
 
 if (flags & VIR_DOMAIN_AFFECT_CONFIG)
@@ -9204,12 +9226,17 @@ qemuDomainSetSchedulerParametersFlags(virDomainPtr dom,
 vmdef = NULL;
 }
 
+
 ret = 0;
 
  cleanup:
 virDomainDefFree(vmdef);
 if (vm)
 virObjectUnlock(vm);
+if (emitEvent)
+event = virDomainEventCputuneNewFromDom(dom, vm->def->cputune);
+if (event)
+qemuDomainEventQueue(driver, event);
 virObjectUnref(caps);
 virO

[libvirt] [PATCH 1/4] domain_conf: separate cputune struct from virDomainDef

[Pavel Hrdina]

Signed-off-by: Pavel Hrdina 
---
 src/conf/domain_conf.h | 27 ---
 1 file changed, 16 insertions(+), 11 deletions(-)

diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index a05254a..d9b0cfa 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -1872,6 +1872,21 @@ struct _virDomaiHugePage {
 unsigned long long size;/* hugepage size in KiB */
 };
 
+typedef struct _virDomainCputune virDomainCputune;
+typedef virDomainCputune *virDomainCputunePtr;
+
+struct _virDomainCputune {
+unsigned long shares;
+bool sharesSpecified;
+unsigned long long period;
+long long quota;
+unsigned long long emulator_period;
+long long emulator_quota;
+size_t nvcpupin;
+virDomainVcpuPinDefPtr *vcpupin;
+virDomainVcpuPinDefPtr emulatorpin;
+};
+
 /*
  * Guest VM main configuration
  *
@@ -1915,17 +1930,7 @@ struct _virDomainDef {
 int placement_mode;
 virBitmapPtr cpumask;
 
-struct {
-unsigned long shares;
-bool sharesSpecified;
-unsigned long long period;
-long long quota;
-unsigned long long emulator_period;
-long long emulator_quota;
-size_t nvcpupin;
-virDomainVcpuPinDefPtr *vcpupin;
-virDomainVcpuPinDefPtr emulatorpin;
-} cputune;
+virDomainCputune cputune;
 
 virDomainNumatunePtr numatune;
 virDomainResourceDefPtr resource;
-- 
1.8.5.5

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

[libvirt] [PATCH 0/4] Introduce new cputune event

[Pavel Hrdina]

This patch series introduces new cputune event to inform
management applications about every change of cputune values
for running domains.

Pavel Hrdina (4):
  domain_conf: separate cputune struct from virDomainDef
  event: introduce new event for cputune
  add an example how to use cputune event
  cputune_event: queue the event for cputune updates

 daemon/remote.c |  87 ++
 examples/object-events/event-test.c |  39 +++-
 include/libvirt/libvirt.h.in|  62 +++
 src/conf/domain_conf.h  |  27 
 src/conf/domain_event.c | 120 
 src/conf/domain_event.h |   7 +++
 src/libvirt_private.syms|   2 +
 src/qemu/qemu_cgroup.c  |   6 ++
 src/qemu/qemu_driver.c  |  27 
 src/remote/remote_driver.c  | 110 +
 src/remote/remote_protocol.x|  39 +++-
 src/remote_protocol-structs |  32 ++
 tools/virsh-domain.c|  49 +++
 13 files changed, 594 insertions(+), 13 deletions(-)

-- 
1.8.5.5

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

Re: [libvirt] Automatically affinitize hostdev interrupts to vm vCpus (qemu/kvm)

[Mooney, Sean K]

Hi sorry for the delay in responding.

Regarding the intel email footer,  I understand that It might deter people from 
responding.
it is automatically added if I email an external address by our exchange server.
I have asked for my account to be added to the exception whitelist 
so this should be removed soon. If not I will follow up with my personal  email 
account.

Regarding abstracting the implementation to a separate file, if the changes are 
small enough 
Perhaps the new code can be added util/virpci and util/virhostdev instead of 
creating util/virinterupt  
I can refactor the implementation to reflect whichever is preferred during code 
review.

I would prefer to use auto as the default also.  
If  the consensus is that it would be better not to enable this feature by 
default, then I am happy to  do that too.  
>From my initial investigation I have just looked at affinitizing interrupts 
>when the vm starts.
I agree that the  final implementation should also handle changes in the vm 
configuration (vpus, hostdevs plug/unplugged) 
and changes In the vm power state also.

Regards
Sean


-Original Message-
From: Martin Kletzander [mailto:mklet...@redhat.com] 
Sent: Monday, August 25, 2014 11:35 AM
To: Mooney, Sean K
Cc: libvir-list@redhat.com
Subject: Re: [libvirt] Automatically affinitize hostdev interrupts to vm vCpus 
(qemu/kvm)

On Mon, Aug 18, 2014 at 07:01:40PM +, Mooney, Sean K wrote:
>Hi
>I would like to ask for comments and propose a possible new libvirt feature.
>

Hi, could you convince your e-mail agent to wrap long lines?

>Problem statement:
>At present when you boot a vm via libvirt it is possible  to pin vm vCPUs to 
>host CPUs to improve performance in the  guest under certain conditions.
>If hostdev interrupts are not pined to a specific core/cpuset suboptimal 
>processing of irqs may occur reducing the performance of both guest and host.
>I would like to propose extending libvirt to automatically pin interrupts for 
>hostdev devices if they are present in the guest.
>
>By affinitizing interrupts, cache line sharing between the specified interrupt 
>and guest can be achieved.
>If CPU affinity for the guest, as set by the cpuset parameter, is 
>intelligently chosen to place the guest on the same numa node as the hostdev, 
>cross socket traffic can be mitigated.
>As a result, latency which would be introduce if the interrupt processing was 
>scheduled to a non-local numa node CPU can be reduced via interrupt pinning.
>
>Proposed change:
>
>* util/virpci and util/virhostdev will be extended to retrieve IRQ and 
>msi_interupt information from sysfs.
>
>* util/virinterupt  will be created
>
>* util/virinterupt  will implement managing interrupt affinity via 
>/proc/irq/x/smp_affinity
>

Nice that this will be abstracted out in a separate file, although if it's not 
huge, it can be part of something else.

>* qemuProcessInitCpuAffinity will be extended to conditionally 
>affinitize hostdev interrupts to vm vCpus when hostdevs are present in the vm 
>definition.

So it will only affect hostdevs, OK, that means there should be less (or no) 
disadvantages).  Although beware that hostdevs can be plugged/unplugged, number 
of vCPUs can be changed and, most importantly, the affinities (either set with 
sched_set_affinity or using cgroups) can be changed during the lifetime of the 
VM, and the smp_affinity of each IRQ should track such changes.  Needless to 
say the affinity should be restored after the machine dies/is turned off, etc., 
not just on hostdev unplug.

>Alternative implementation:
>
>In addition to the above changes the hostdev element could be extended to 
>include a cpuset attribute:
>
>* if the cpuset is auto: the interrupts would be pinned to the same 
>cpuset as the vms vCPU
>
>* if a cpuset is specified: the interrupts would be affinitized as per 
>the set cpuset
>
>* if the cpuset is native: the interrupts will not be pinned and the 
>current behaviour will not be changed.
>
>* If the cpuset attribute is not present either the behaviour of auto 
>or native could be used as a default.
>
>o   Using auto as the default would allow transparent use of the feature.
>

I like defaulting to auto also because the transparent use will have effect 
only in existing deployments with vCPU pinning.

>o   Using native as the default would allow no changes to any existing 
>deployments unless the feature is requested.

Although this version might be preferred by others.  This can, however, be 
discussed (and changed) during reviews.

>Any feedback is welcomed.
>Regards
>Sean.
>--
>Intel Shannon Limited
>Registered in Ireland
>Registered Office: Collinstown Industrial Park, Leixlip, County Kildare 
>Registered Number: 308263 Business address: Dromore House, East Park, 
>Shannon, Co. Clare
>
>This e-mail and any attachments may contain confidential material 

Re: [libvirt] [python PATCH 2/5] API: Skip 'virDomainStatsRecordListFree'

[Eric Blake]

On 08/28/2014 10:32 AM, Peter Krempa wrote:
> The new API function doesn't make sense to be exported in python. The
> bindings will return native types instead of the struct array.
> ---
>  generator.py | 1 +
>  1 file changed, 1 insertion(+)
> 

ACK.

> diff --git a/generator.py b/generator.py
> index 9c497be..cfc016e 100755
> --- a/generator.py
> +++ b/generator.py
> @@ -571,6 +571,7 @@ skip_function = (
>  "virTypedParamsGetULLong",
> 
>  'virNetworkDHCPLeaseFree', # only useful in C, python code uses list
> +'virDomainStatsRecordListFree', # only useful in C, python uses dict
>  )
> 
>  lxc_skip_function = (
> 

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



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

Re: [libvirt] [python PATCH 1/5] generator: enum: Don't sort enums by names

[Eric Blake]

On 08/28/2014 10:32 AM, Peter Krempa wrote:
> Setting OCDs aside, sorting enums breaks if the definition contains
> links to other enums defined in the libvirt header before. Let the
> generator generate it in the natural order.
> ---
>  generator.py | 2 --
>  1 file changed, 2 deletions(-)

ACK. But I _also_ think we need to fix libvirt to recursively resolve
enums defined by reference to other names, so that the API xml file it
generates is fully numeric rather than making clients chase down
symbolic resolutions themselves.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



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

[libvirt] [python PATCH 5/5] DO NOT APPLY: Fix build with missing virDomainBlockCopy API

[Peter Krempa]

---
 generator.py | 1 +
 1 file changed, 1 insertion(+)

diff --git a/generator.py b/generator.py
index 3642838..a88c146 100755
--- a/generator.py
+++ b/generator.py
@@ -551,6 +551,7 @@ skip_function = (
 "virStorageVolGetConnect",
 "virDomainSnapshotGetConnect",
 "virDomainSnapshotGetDomain",
+'virDomainBlockCopy',

 # only useful in C code, python code uses dict for typed parameters
 "virTypedParamsAddBoolean",
-- 
2.0.2

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

[libvirt] [python PATCH 0/5] Implement bindings for bulk stats API

[Peter Krempa]

The last patch is to ease review to be able to build the series.

Peter Krempa (5):
  generator: enum: Don't sort enums by names
  API: Skip 'virDomainStatsRecordListFree'
  API: Implement bindings for virConnectGetAllDomainStats
  API: Implement bindings for virDomainListGetStats
  DO NOT APPLY: Fix build with missing virDomainBlockCopy API

 generator.py   |   6 +-
 libvirt-override-virConnect.py | 100 
 libvirt-override.c | 144 +
 3 files changed, 248 insertions(+), 2 deletions(-)

-- 
2.0.2

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

[libvirt] [python PATCH 3/5] API: Implement bindings for virConnectGetAllDomainStats

[Peter Krempa]

Implement the function by returning a list of tuples instead the array
of virDomainStatsRecords and store the typed parameters as dict.
---
 generator.py   |  1 +
 libvirt-override-virConnect.py | 53 
 libvirt-override.c | 94 ++
 3 files changed, 148 insertions(+)

diff --git a/generator.py b/generator.py
index cfc016e..9addb89 100755
--- a/generator.py
+++ b/generator.py
@@ -507,6 +507,7 @@ skip_function = (
 'virConnectListAllNodeDevices', # overridden in virConnect.py
 'virConnectListAllNWFilters', # overridden in virConnect.py
 'virConnectListAllSecrets', # overridden in virConnect.py
+'virConnectGetAllDomainStats', # overridden in virConnect.py

 'virStreamRecvAll', # Pure python libvirt-override-virStream.py
 'virStreamSendAll', # Pure python libvirt-override-virStream.py
diff --git a/libvirt-override-virConnect.py b/libvirt-override-virConnect.py
index 31d71a3..c4c400a 100644
--- a/libvirt-override-virConnect.py
+++ b/libvirt-override-virConnect.py
@@ -383,3 +383,56 @@
 if ret is None:raise libvirtError('virDomainCreateXMLWithFiles() 
failed', conn=self)
 __tmp = virDomain(self,_obj=ret)
 return __tmp
+
+def getAllDomainStats(self, stats = 0, flags=0):
+"""Query statistics for all domains on a given connection.
+
+Report statistics of various parameters for a running VM according to 
@stats
+field. The statistics are returned as an array of structures for each 
queried
+domain. The structure contains an array of typed parameters containing 
the
+individual statistics. The typed parameter name for each statistic 
field
+consists of a dot-separated string containing name of the requested 
group
+followed by a group specific description of the statistic value.
+
+The statistic groups are enabled using the @stats parameter which is a
+binary-OR of enum virDomainStatsTypes. The following groups are 
available
+(although not necessarily implemented for each hypervisor):
+
+VIR_DOMAIN_STATS_STATE: Return domain state and reason for entering 
that
+state. The typed parameter keys are in this format:
+"state.state" - state of the VM, returned as int from virDomainState 
enum
+"state.reason" - reason for entering given state, returned as int from
+ virDomain*Reason enum corresponding to given state.
+
+Using 0 for @stats returns all stats groups supported by the given
+hypervisor.
+
+Specifying VIR_CONNECT_GET_ALL_DOMAINS_STATS_ENFORCE_STATS as @flags 
makes
+the function return error in case some of the stat types in @stats were
+not recognized by the daemon.
+
+Similarly to virConnectListAllDomains, @flags can contain various 
flags to
+filter the list of domains to provide stats for.
+
+VIR_CONNECT_GET_ALL_DOMAINS_STATS_ACTIVE selects online domains while
+VIR_CONNECT_GET_ALL_DOMAINS_STATS_INACTIVE selects offline ones.
+
+VIR_CONNECT_GET_ALL_DOMAINS_STATS_PERSISTENT and
+VIR_CONNECT_GET_ALL_DOMAINS_STATS_TRANSIENT allow to filter the list
+according to their persistence.
+
+To filter the list of VMs by domain state @flags can contain
+VIR_CONNECT_GET_ALL_DOMAINS_STATS_RUNNING,
+VIR_CONNECT_GET_ALL_DOMAINS_STATS_PAUSED,
+VIR_CONNECT_GET_ALL_DOMAINS_STATS_SHUTOFF and/or
+VIR_CONNECT_GET_ALL_DOMAINS_STATS_OTHER for all other states. """
+ret = libvirtmod.virConnectGetAllDomainStats(self._o, stats, flags)
+if ret is None:
+raise libvirtError("virConnectGetAllDomainStats() failed", 
conn=self)
+
+retlist = list()
+for elem in ret:
+record = (virDomain(self, _obj=elem[0]) , elem[1])
+retlist.append(record)
+
+return retlist
diff --git a/libvirt-override.c b/libvirt-override.c
index b2271ae..df4f15b 100644
--- a/libvirt-override.c
+++ b/libvirt-override.c
@@ -4964,6 +4964,97 @@ cleanup:
 return py_retval;
 }

+#if LIBVIR_CHECK_VERSION(1, 2, 8)
+static PyObject *
+convertDomainStatsRecord(virDomainStatsRecordPtr *records,
+ int nrecords)
+{
+PyObject *py_retval;
+PyObject *py_record;
+PyObject *py_record_domain;
+PyObject *py_record_stats;
+size_t i;
+
+if (!(py_retval = PyList_New(nrecords)))
+return NULL;
+
+for (i = 0; i < nrecords; i++) {
+if (!(py_record = PyTuple_New(2)))
+goto error;
+
+/* libvirt_virDomainPtrWrap steals the object */
+virDomainRef(records[i]->dom);
+if (!(py_record_domain = libvirt_virDomainPtrWrap(records[i]->dom))) {
+virDomainFree(records[i]->dom);
+goto error;
+}
+
+if (!(py_record_stats = getPyVirTypedParameter(records[i]->params,
+

[libvirt] [python PATCH 2/5] API: Skip 'virDomainStatsRecordListFree'

[Peter Krempa]

The new API function doesn't make sense to be exported in python. The
bindings will return native types instead of the struct array.
---
 generator.py | 1 +
 1 file changed, 1 insertion(+)

diff --git a/generator.py b/generator.py
index 9c497be..cfc016e 100755
--- a/generator.py
+++ b/generator.py
@@ -571,6 +571,7 @@ skip_function = (
 "virTypedParamsGetULLong",

 'virNetworkDHCPLeaseFree', # only useful in C, python code uses list
+'virDomainStatsRecordListFree', # only useful in C, python uses dict
 )

 lxc_skip_function = (
-- 
2.0.2

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

[libvirt] [python PATCH 1/5] generator: enum: Don't sort enums by names

[Peter Krempa]

Setting OCDs aside, sorting enums breaks if the definition contains
links to other enums defined in the libvirt header before. Let the
generator generate it in the natural order.
---
 generator.py | 2 --
 1 file changed, 2 deletions(-)

diff --git a/generator.py b/generator.py
index a12c52b..9c497be 100755
--- a/generator.py
+++ b/generator.py
@@ -1786,8 +1786,6 @@ def buildWrappers(module):
 return value

 enumvals = list(enums.items())
-if enumvals is not None:
-enumvals.sort(key=lambda x: x[0])
 for type,enum in enumvals:
 classes.write("# %s\n" % type)
 items = list(enum.items())
-- 
2.0.2

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

[libvirt] [python PATCH 4/5] API: Implement bindings for virDomainListGetStats

[Peter Krempa]

Implement the function by returning a list of tuples instead the array
of virDomainStatsRecords and store the typed parameters as dict.
---
 generator.py   |  1 +
 libvirt-override-virConnect.py | 47 +++
 libvirt-override.c | 50 ++
 3 files changed, 98 insertions(+)

diff --git a/generator.py b/generator.py
index 9addb89..3642838 100755
--- a/generator.py
+++ b/generator.py
@@ -508,6 +508,7 @@ skip_function = (
 'virConnectListAllNWFilters', # overridden in virConnect.py
 'virConnectListAllSecrets', # overridden in virConnect.py
 'virConnectGetAllDomainStats', # overridden in virConnect.py
+'virDomainListGetStats', # overriden in virConnect.py

 'virStreamRecvAll', # Pure python libvirt-override-virStream.py
 'virStreamSendAll', # Pure python libvirt-override-virStream.py
diff --git a/libvirt-override-virConnect.py b/libvirt-override-virConnect.py
index c4c400a..218f266 100644
--- a/libvirt-override-virConnect.py
+++ b/libvirt-override-virConnect.py
@@ -436,3 +436,50 @@
 retlist.append(record)

 return retlist
+
+def domainListGetStats(self, doms, stats=0, flags=0):
+""" Query statistics for given domains.
+
+Report statistics of various parameters for a running VM according to 
@stats
+field. The statistics are returned as an array of structures for each 
queried
+domain. The structure contains an array of typed parameters containing 
the
+individual statistics. The typed parameter name for each statistic 
field
+consists of a dot-separated string containing name of the requested 
group
+followed by a group specific description of the statistic value.
+
+The statistic groups are enabled using the @stats parameter which is a
+binary-OR of enum virDomainStatsTypes. The following groups are 
available
+(although not necessarily implemented for each hypervisor):
+
+VIR_DOMAIN_STATS_STATE: Return domain state and reason for entering 
that
+state. The typed parameter keys are in this format:
+"state.state" - state of the VM, returned as int from virDomainState 
enum
+"state.reason" - reason for entering given state, returned as int from
+ virDomain*Reason enum corresponding to given state.
+
+Using 0 for @stats returns all stats groups supported by the given
+hypervisor.
+
+Specifying VIR_CONNECT_GET_ALL_DOMAINS_STATS_ENFORCE_STATS as @flags 
makes
+the function return error in case some of the stat types in @stats were
+not recognized by the daemon.
+
+Get statistics about domains provided as a list in @doms. @stats is
+a bit field selecting requested statistics types."""
+domlist = list()
+for dom in doms:
+if not isinstance(dom, virDomain):
+raise libvirtError("domain list contains non-domain elements", 
conn=self)
+
+domlist.append(dom._o)
+
+ret = libvirtmod.virDomainListGetStats(self._o, domlist, stats, flags)
+if ret is None:
+raise libvirtError("virDomainListGetStats() failed", conn=self)
+
+retlist = list()
+for elem in ret:
+record = (virDomain(self, _obj=elem[0]) , elem[1])
+retlist.append(record)
+
+return retlist
diff --git a/libvirt-override.c b/libvirt-override.c
index df4f15b..7e9f570 100644
--- a/libvirt-override.c
+++ b/libvirt-override.c
@@ -5052,6 +5052,55 @@ libvirt_virConnectGetAllDomainStats(PyObject *self 
ATTRIBUTE_UNUSED,

 return py_retval;
 }
+
+
+static PyObject *
+libvirt_virDomainListGetStats(PyObject *self ATTRIBUTE_UNUSED,
+  PyObject *args)
+{
+PyObject *pyobj_conn;
+PyObject *py_retval;
+PyObject *py_domlist;
+virConnectPtr conn;
+virDomainStatsRecordPtr *records;
+virDomainPtr *doms = NULL;
+int nrecords;
+int ndoms;
+size_t i;
+unsigned int flags;
+unsigned int stats;
+
+if (!PyArg_ParseTuple(args, (char *)"OOii:virDomainListGetStats",
+  &pyobj_conn, &py_domlist, &stats, &flags))
+return NULL;
+conn = (virConnectPtr) PyvirConnect_Get(pyobj_conn);
+
+if (PyList_Check(py_domlist)) {
+ndoms = PyList_Size(py_domlist);
+
+if (VIR_ALLOC_N(doms, ndoms + 1) < 0)
+return PyErr_NoMemory();
+
+for (i = 0; i < ndoms; i++)
+doms[i] = PyvirDomain_Get(PyList_GetItem(py_domlist, i));
+}
+
+LIBVIRT_BEGIN_ALLOW_THREADS;
+nrecords = virDomainListGetStats(doms, stats, &records, flags);
+LIBVIRT_END_ALLOW_THREADS;
+
+if (nrecords < 0)
+return VIR_PY_NONE;
+
+if (!(py_retval = convertDomainStatsRecord(records, nrecords)))
+py_retval = VIR_PY_NONE;
+
+ cleanup:
+virDomainStatsRecordListFree(records);
+VIR_FREE(doms);

Re: [libvirt] [PATCH 01/19] libxl_migration: Resolve Coverity NULL_RETURNS

[Jim Fehlig]

John Ferlan wrote:
> On 08/27/2014 05:29 PM, Jim Fehlig wrote:
>   
>> John Ferlan wrote:
>> 
>>> Coverity noted that all callers to libxlDomainEventQueue() could ensure
>>> the second parameter (event) was true before calling except this case.
>>> As I look at the code and how events are used - it seems that prior to
>>> generating an event for the dom == NULL condition, the resume/suspend
>>> event should be queue'd after the virDomainSaveStatus() call which will
>>> goto cleanup and queue the saved event anyway.
>>>
>>> Signed-off-by: John Ferlan 
>>> ---
>>>  src/libxl/libxl_migration.c | 6 +-
>>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c
>>> index dbb5a8f..53ae63a 100644
>>> --- a/src/libxl/libxl_migration.c
>>> +++ b/src/libxl/libxl_migration.c
>>> @@ -512,6 +512,11 @@ libxlDomainMigrationFinish(virConnectPtr dconn,
>>>  if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0)
>>>  goto cleanup;
>>>  
>>> +if (event) {
>>> +libxlDomainEventQueue(driver, event);
>>> +event = NULL;
>>> +}
>>> +
>>>  dom = virGetDomain(dconn, vm->def->name, vm->def->uuid);
>>>  
>>>  if (dom == NULL) {
>>> @@ -519,7 +524,6 @@ libxlDomainMigrationFinish(virConnectPtr dconn,
>>>  libxlDomainCleanup(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED);
>>>  event = virDomainEventLifecycleNewFromObj(vm, 
>>> VIR_DOMAIN_EVENT_STOPPED,
>>>   VIR_DOMAIN_EVENT_STOPPED_FAILED);
>>> -libxlDomainEventQueue(driver, event);
>>>  }
>>>   
>>>   
>> See my question in your first series about whether the dom == NULL check
>> is even needed.  If not, I can send a patch to remove the check, in
>> which case this patch wouldn't be needed.
>>
>> https://www.redhat.com/archives/libvir-list/2014-August/msg01399.html
>>
>>
>> 
>
> I am going to remove this one from my series and let you handle it.
>   

After looking at the code again, I think it is safest to go with your patch.

> I would seem perhaps that the code was there to ensure that if either of
> the calls to virDomainObjSetState() ended up resulting in 'dom' not
> returning something from the virGetDomain(), then like perhaps the qemu
> migration code, the "best choice" was to remove it.

Yep, agreed.  I haven't convinced myself that it is impossible for
virGetDomain() to return a NULL domainPtr, but in the event it does I
agree it is best to remove the domain.

Regards,
Jim

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

Re: [libvirt] Proposition for the implementation of new features for Hyper-V driver

[Daniel P. Berrange]

On Thu, Aug 28, 2014 at 12:38:17PM +, Adrien Kantcheff wrote:
> Dear libvirt developers,
> 
> During my final student training in the French company Bull, with a
> previous student (Simon RASTELLO), we developed new features for
> Hyper-V driver.  For a project called OpenCloudware, our work was
> to bring new functionalities using libvirt API in order to do basic
> actions on virtual machines hosted on Hyper-V.
> 
> You may be interested in pushing our developments in the official
> release.

It is great to hear that someone still has interest in developing
and working on the Hyper-V driver for libvirt !
 
> The libvirt driver already provides functions to enumerate and get
> WMI classes by sending WQL requests to WMI provider and also to
> call WMI class methods. For that last kind of communication, a
> method requires input parameters. There are two kinds of argument: 
> basic (integer, string...) and complex (objects, end point references,
> embedded instances). Actually, just the first argument passing mode
> is available with libvirt driver. But the second one is very useful
> because there is many WMI methods with complex types parameters,
> and for the moment it constraints developers to call WMI methods
> only with basic types parameters. So in order to expand WMI methods
> calls, we have implemented the second argument passing mode.
> 
> Thanks to this new argument passing mode, we are available  to set
> new functionalities.

[snip]

> Attach files contain sources and an pdf of our contributions.
> 
> I'm ending my training this week but I'm available to answer any
> questions you may have. You can use my personal email:
> adrien.kantch...@gmail.com
> 
> I also put in copy of this email my tutors Yves VINTER and Christian
> BOURGEOIS. Feel free to contact them as well.

I appreciate that your finishing your assignment shortly, but could I
perhaps ask you or a co-worker at Bull to create a short patch series
out of your changes and submit it for review with 'git send-email' ?

I would love to see the code improvements you've done included in libvirt
but we can't effectively review them when they are just provided as a
complete new source file, instead of as a patch series.

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

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

Re: [libvirt] [PATCH v1 6/6] ivshmem: add start param to server attribute

[Martin Kletzander]

On Thu, Aug 28, 2014 at 03:46:47PM +0200, David Marchand wrote:


On 08/28/2014 02:26 PM, David Marchand wrote:



I'm not sure, though, what to do with the first point (race between
libvirt creating the socket to see that it exists and ivshmem
disconnecting).  Maybe libvirt could do this (if QEMU would support
it):

1: try to create the socket (if it exists, continue with 4)

2: connect to the socket

3: if connecting fails, goto 1;

4: if libvirt was the one who created the socket, start the server
and pass the FD of the socket to it

5: start qemu and pass the socket to it (qemu already supports that
with other devices like netdevs, etc.

This would take care of all three points.  No race, no permission
issues, nothing.

What do you think?


- Mmm, I had felt that there could be a race in the socket check, yes.
The LISTEN_FDS support in the server is not that big of a change.
I think I can take care of that.


- Did not think of the other points.
I think there is still some problem with your proposition, I need more
time to think about it (may be some prototyping to be sure).


I had misunderstood something about listen()/connect().
Ok, your proposition looks good to me.

At least for the server, this should be transparent as long as the
server handles LISTEN_FDS env variable or an option to tell it which fd
he should listen on.



Parameter is fine, too, I was probably just thinking about the
LISTEN_FDS stuff too much due to having some trouble implementing it
in libvirtd.


For the ivshmem part in QEMU itself, I think adding a property to
ivshmem pci class should do the trick, will see if I (or Maxime) can do
this.



Great.  One more minor thing, though.  In libvirt, we need to have the
option to know whether QEMU supports that newly added option.  We are
introspecting such things using QMP, so if it's a device property, we
should be able to get that.


Are there any other points concerning the server ?



Not that I know of (yet).  Feel free to Cc me on the patches for the
ivshmem stuff in qemu-devel.

Martin


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

Re: [libvirt] run qemu-agent-command via binding (ruby/python/php)

[Vasiliy Tolstov]

2014-08-28 17:50 GMT+04:00 Chris Lalancette :
> The ruby bindings support virDomainQemuAgentCommand() since 0.5.2.  I
> haven't really tested it, though, so your mileage may vary.  I
> definitely accept bug reports and patches.


Thanks! Very nice. Does it possible to request this feature from php
libvirt binding maintainer?

-- 
Vasiliy Tolstov,
e-mail: v.tols...@selfip.ru
jabber: v...@selfip.ru

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

Re: [libvirt] [PATCH v2 2/2] network: Bring netdevs online later

[Martin Kletzander]

On Wed, Aug 27, 2014 at 10:34:14AM -0400, Matthew Rosato wrote:

Currently, MAC registration occurs during device creation, which is
early enough that, during live migration, you end up with duplicate
MAC addresses on still-running source and target devices, even though
the target device isn't actually being used yet.
This patch proposes to defer MAC registration until right before
the guest can actually use the device -- In other words, right
before starting guest CPUs.

Signed-off-by: Matthew Rosato 
---
src/Makefile.am |3 +-
src/conf/domain_conf.h  |2 ++
src/lxc/lxc_process.c   |4 ++-
src/qemu/qemu_command.c |6 +++-
src/qemu/qemu_hotplug.c |7 
src/qemu/qemu_interface.c   |   78 +++
src/qemu/qemu_interface.h   |   32 ++
src/qemu/qemu_process.c |4 +++
src/util/virnetdevmacvlan.c |8 +++--
src/util/virnetdevmacvlan.h |2 ++
10 files changed, 140 insertions(+), 6 deletions(-)
create mode 100644 src/qemu/qemu_interface.c
create mode 100644 src/qemu/qemu_interface.h

diff --git a/src/Makefile.am b/src/Makefile.am
index 46e411e..842573f 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -703,7 +703,8 @@ QEMU_DRIVER_SOURCES =   
\
qemu/qemu_monitor_text.h\
qemu/qemu_monitor_json.c\
qemu/qemu_monitor_json.h\
-   qemu/qemu_driver.c qemu/qemu_driver.h
+   qemu/qemu_driver.c qemu/qemu_driver.h   \


I don't know if this got mixed by the mail client or not, but I'm
adjusting it (the backslash) to match the others.


+   qemu/qemu_interface.c qemu/qemu_interface.h



I still don't see anything qemu-specific inthose files, but for now
separate files are fine, we can move the code around any time later if
it's needed.


XENAPI_DRIVER_SOURCES = \
xenapi/xenapi_driver.c xenapi/xenapi_driver.h   \
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index aead903..6268690 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -950,6 +950,8 @@ struct _virDomainNetDef {
virNetDevBandwidthPtr bandwidth;
virNetDevVlan vlan;
int linkstate;
+/* vmOp value saved if deferring interface start */
+virNetDevVPortProfileOp vmOp;
};

/* Used for prefix of ifname of any network name generated dynamically
diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c
index ed30c37..b2256c0 100644
--- a/src/lxc/lxc_process.c
+++ b/src/lxc/lxc_process.c
@@ -300,6 +300,7 @@ char *virLXCProcessSetupInterfaceDirect(virConnectPtr conn,
virNetDevBandwidthPtr bw;
virNetDevVPortProfilePtr prof;
virLXCDriverConfigPtr cfg = virLXCDriverGetConfig(driver);
+unsigned int macvlan_create_flags = VIR_NETDEV_MACVLAN_CREATE_IFUP;

/* XXX how todo bandwidth controls ?
 * Since the 'net-ifname' is about to be moved to a different
@@ -336,7 +337,8 @@ char *virLXCProcessSetupInterfaceDirect(virConnectPtr conn,
&res_ifname,
VIR_NETDEV_VPORT_PROFILE_OP_CREATE,
cfg->stateDir,
-virDomainNetGetActualBandwidth(net), 0) < 0)
+virDomainNetGetActualBandwidth(net),
+macvlan_create_flags) < 0)
goto cleanup;

ret = res_ifname;
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 1f71fa3..43a8b63 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -199,6 +199,9 @@ qemuPhysIfaceConnect(virDomainDefPtr def,
net->ifname = res_ifname;
}

+/* Save vport profile op for later */
+net->vmOp = vmop;
+
virObjectUnref(cfg);
return rc;
}
@@ -286,7 +289,7 @@ qemuNetworkIfaceConnect(virDomainDefPtr def,
{
char *brname = NULL;
int ret = -1;
-unsigned int tap_create_flags = VIR_NETDEV_TAP_CREATE_IFUP;
+unsigned int tap_create_flags = 0;
bool template_ifname = false;
int actualType = virDomainNetGetActualType(net);
virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
@@ -346,6 +349,7 @@ qemuNetworkIfaceConnect(virDomainDefPtr def,
goto cleanup;
}
} else {
+tap_create_flags |= VIR_NETDEV_TAP_CREATE_IFUP;


But I don't quite understand why this is different for system and
session daemon.  Maybe it's my head just giving up on me.


if (qemuCreateInBridgePortWithHelper(cfg, brname,
 &net->ifname,
 tapfd, tap_create_flags) < 0) {
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index a364c52..633eda5 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -30,6 +30,7 @@
#include "qemu_domain.h"
#include "qemu_command.h"
#include "qemu_hostdev.h"
+#include "qemu_interface.h"
#include "domain_audit

Re: [libvirt] [PATCH v1 6/6] ivshmem: add start param to server attribute

[David Marchand]

On 08/28/2014 02:26 PM, David Marchand wrote:



I'm not sure, though, what to do with the first point (race between
libvirt creating the socket to see that it exists and ivshmem
disconnecting).  Maybe libvirt could do this (if QEMU would support
it):

1: try to create the socket (if it exists, continue with 4)

2: connect to the socket

3: if connecting fails, goto 1;

4: if libvirt was the one who created the socket, start the server
and pass the FD of the socket to it

5: start qemu and pass the socket to it (qemu already supports that
with other devices like netdevs, etc.

This would take care of all three points.  No race, no permission
issues, nothing.

What do you think?


- Mmm, I had felt that there could be a race in the socket check, yes.
The LISTEN_FDS support in the server is not that big of a change.
I think I can take care of that.


- Did not think of the other points.
I think there is still some problem with your proposition, I need more
time to think about it (may be some prototyping to be sure).


I had misunderstood something about listen()/connect().
Ok, your proposition looks good to me.

At least for the server, this should be transparent as long as the 
server handles LISTEN_FDS env variable or an option to tell it which fd 
he should listen on.


For the ivshmem part in QEMU itself, I think adding a property to 
ivshmem pci class should do the trick, will see if I (or Maxime) can do 
this.


Are there any other points concerning the server ?


--
David Marchand

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

Re: [libvirt] run qemu-agent-command via binding (ruby/python/php)

[Chris Lalancette]

On Thu, Aug 28, 2014 at 9:34 AM, Eric Blake  wrote:
> On 08/28/2014 07:12 AM, Vasiliy Tolstov wrote:
>> Hi! Does it possible(featured, planned) to run some qemu-agent-command
>> via libvirt binding (i'm interesting on ruby and php).?
>
> I'm not sure the time-schedule of the ruby and php bindings maintainers,
> but ideally, ALL libvirt API should eventually have exposure in each
> language binding.

The ruby bindings support virDomainQemuAgentCommand() since 0.5.2.  I
haven't really tested it, though, so your mileage may vary.  I
definitely accept bug reports and patches.

Chris

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

Re: [libvirt] [PATCH v2 1/2] util: Introduce flags field for macvtap creation

[Matthew Rosato]

On 08/28/2014 08:45 AM, Martin Kletzander wrote:
> On Wed, Aug 27, 2014 at 10:34:13AM -0400, Matthew Rosato wrote:
>> Currently, there is one flag passed in during macvtap creation
>> (withTap) -- Let's convert this field to an unsigned int flag
>> field for future expansion.
>>
>> Signed-off-by: Matthew Rosato 
>> ---
>> src/lxc/lxc_process.c   |4 ++--
>> src/qemu/qemu_command.c |6 --
>> src/util/virnetdevmacvlan.c |   28 +---
>> src/util/virnetdevmacvlan.h |   14 ++
>> 4 files changed, 33 insertions(+), 19 deletions(-)
>>
>> diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c
>> index 3353dc1..ed30c37 100644
>> --- a/src/lxc/lxc_process.c
>> +++ b/src/lxc/lxc_process.c
>> @@ -331,12 +331,12 @@ char
>> *virLXCProcessSetupInterfaceDirect(virConnectPtr conn,
>> net->ifname, &net->mac,
>> virDomainNetGetActualDirectDev(net),
>> virDomainNetGetActualDirectMode(net),
>> -false, false, def->uuid,
>> +false, def->uuid,
>> virDomainNetGetActualVirtPortProfile(net),
>> &res_ifname,
>> VIR_NETDEV_VPORT_PROFILE_OP_CREATE,
>> cfg->stateDir,
>> -virDomainNetGetActualBandwidth(net)) < 0)
>> +virDomainNetGetActualBandwidth(net), 0) < 0)
>> goto cleanup;
>>
>> ret = res_ifname;
>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>> index 9241f57..1f71fa3 100644
>> --- a/src/qemu/qemu_command.c
>> +++ b/src/qemu/qemu_command.c
>> @@ -177,6 +177,7 @@ qemuPhysIfaceConnect(virDomainDefPtr def,
>> char *res_ifname = NULL;
>> int vnet_hdr = 0;
>> virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
>> +unsigned int macvlan_create_flags =
>> VIR_NETDEV_MACVLAN_CREATE_WITH_TAP;
>>
>> if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_VNET_HDR) &&
>> net->model && STREQ(net->model, "virtio"))
>> @@ -186,11 +187,12 @@ qemuPhysIfaceConnect(virDomainDefPtr def,
>> net->ifname, &net->mac,
>> virDomainNetGetActualDirectDev(net),
>> virDomainNetGetActualDirectMode(net),
>> -true, vnet_hdr, def->uuid,
>> +vnet_hdr, def->uuid,
>> virDomainNetGetActualVirtPortProfile(net),
>> &res_ifname,
>> vmop, cfg->stateDir,
>> -virDomainNetGetActualBandwidth(net));
>> +virDomainNetGetActualBandwidth(net),
>> +macvlan_create_flags);
>> if (rc >= 0) {
>> virDomainAuditNetDevice(def, net, res_ifname, true);
>> VIR_FREE(net->ifname);
>> diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c
>> index 054194b..9ddeca4 100644
>> --- a/src/util/virnetdevmacvlan.c
>> +++ b/src/util/virnetdevmacvlan.c
>> @@ -790,6 +790,7 @@ virNetDevMacVLanVPortProfileRegisterCallback(const
>> char *ifname,
>>  * @macaddress: The MAC address for the macvtap device
>>  * @linkdev: The interface name of the NIC to connect to the external
>> bridge
>>  * @mode: int describing the mode for 'bridge', 'vepa', 'private' or
>> 'passthru'.
>> + * @flags: OR of virNetDevMacVLanCreateFlags.
> 
> I took the liberty of moving this as a last parameter as well.
> 
>>  * @vnet_hdr: 1 to enable IFF_VNET_HDR, 0 to disable it
>>  * @vmuuid: The UUID of the VM the macvtap belongs to
>>  * @virtPortProfile: pointer to object holding the virtual port
>> profile data
>> @@ -797,25 +798,29 @@
>> virNetDevMacVLanVPortProfileRegisterCallback(const char *ifname,
>>  * interface will be stored into if everything succeeded. It is up
>>  * to the caller to free the string.
>>  *
>> - * Returns file descriptor of the tap device in case of success with
>> @withTap,
>> - * otherwise returns 0; returns -1 on error.
>> + * Returns file descriptor of the tap device in case of success with
>> + * @flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP, otherwise returns 0;
>> returns
>> + * -1 on error.
>>  */
>> int virNetDevMacVLanCreateWithVPortProfile(const char *tgifname,
>>const virMacAddr *macaddress,
>>const char *linkdev,
>>virNetDevMacVLanMode mode,
>> -   bool withTap,
>>int vnet_hdr,
>>const unsigned char *vmuuid,
>>virNetDevVPortProfilePtr
>> virtPortProfile,
>>char **res_ifname,
>>virNetDevVPortProfileOp vmOp,
>>char *stateDir,
>> -   virNetDevBandwidthPtr
>> bandwidth)
>> +   virNetDevBandwidthPtr
>> bandwidth,
>> +   unsigned int flags)
>> {
>> -const char *type = withTap ? "macvtap" : "mac

Re: [libvirt] run qemu-agent-command via binding (ruby/python/php)

[Vasiliy Tolstov]

2014-08-28 17:34 GMT+04:00 Eric Blake :
> On 08/28/2014 07:12 AM, Vasiliy Tolstov wrote:
>> Hi! Does it possible(featured, planned) to run some qemu-agent-command
>> via libvirt binding (i'm interesting on ruby and php).?
>
> I'm not sure the time-schedule of the ruby and php bindings maintainers,
> but ideally, ALL libvirt API should eventually have exposure in each
> language binding.
>
>> I'm understand that i can connect via socket and run it, but it very
>> usable to get this ability inside binding.
>
> In the meantime, if you can fork out to a shell, you can use 'virsh'
> from within your language of choice to drive the bindings.
>
> Also, it might help to say WHAT you hope to do with qemu-agent-command -
> that is an explicitly unsupported interface, and if you find yourself
> having to use it, it means we have a hole in libvirt proper, where we
> should expose a supported API to get at the same task without going
> through the backdoor.  (As past examples, we added virDomainSetTime
> because people were previously having to use the agent's back door.)

Thanks. I want to use backdoor =) Now i'm need to get memory stats
from guest (free memory, used swap) and rewrite 50% of qemu-ga in
golang.
And feature plans is add ability to get,set,del ip addresses and
routes inside virtual machine.
In very long feature may be i want to create some control panel
backend that works via virtio-serial.
Why not add ability to send some json via libvirt api function and get results ?



-- 
Vasiliy Tolstov,
e-mail: v.tols...@selfip.ru
jabber: v...@selfip.ru

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

Re: [libvirt] [PATCHv4] qemu: Implement bulk stats API and one of the stats groups to return

[Peter Krempa]

On 08/28/14 14:48, Eric Blake wrote:
> On 08/28/2014 06:18 AM, Peter Krempa wrote:
>> Implement the API function for virDomainListGetStats and
>> virConnectGetAllDomainStats in a modular way and implement the
>> VIR_DOMAIN_STATS_STATE group of statistics.
>>
>> Although it may look like the function looks universal I'd rather not
>> expose it to other drivers as the coming stats groups are likely to do
>> qemu specific stuff to obtain the stats.
>> ---
>>
>> Notes:
>> Version 4:
>> - fixed handling and error checking of @stats
>> - domain filtering flags are now rejected when passing in a domain list
> 
> Looks nicer.  Thanks for putting up with me.
> 
> 
>> +static int
>> +qemuDomainGetStats(virConnectPtr conn,
>> +   virDomainObjPtr dom,
> 
>> +
>> +for (i = 0; qemuDomainGetStatsWorkers[i].func; i++) {
>> +if (stats & qemuDomainGetStatsWorkers[i].stats) {
>> +if (qemuDomainGetStatsWorkers[i].func(dom, tmp, &maxparams,
> 
> These two if's could be combined into one with &&, for one less level of
> indentation (but I'm also okay if you leave it alone).
> 
>> +  flags) < 0)
>> +goto cleanup;
>> +}
>> +}
>> +
>> +if (!(tmp->dom = virGetDomain(conn, dom->def->name, dom->def->uuid)))
> 
> Earlier versions had a comment about doing dom last for a reason.  Not
> sure if you want to reinstate it; I can live without it.

I've added the comment before I found a way to non-error-destructively
free the domain object so now it isn't entirely true.

> 
> 
>> +}
>> +
>> +
>> +
>> +static int
>> +qemuConnectGetAllDomainStats(virConnectPtr conn,
> 
> 3 blank lines is unusual.
> 

Also not-easy-to spot when trying to find it :D.

> ACK.
> 

Thanks pushed.

Peter



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

Re: [libvirt] run qemu-agent-command via binding (ruby/python/php)

[Eric Blake]

On 08/28/2014 07:12 AM, Vasiliy Tolstov wrote:
> Hi! Does it possible(featured, planned) to run some qemu-agent-command
> via libvirt binding (i'm interesting on ruby and php).?

I'm not sure the time-schedule of the ruby and php bindings maintainers,
but ideally, ALL libvirt API should eventually have exposure in each
language binding.

> I'm understand that i can connect via socket and run it, but it very
> usable to get this ability inside binding.

In the meantime, if you can fork out to a shell, you can use 'virsh'
from within your language of choice to drive the bindings.

Also, it might help to say WHAT you hope to do with qemu-agent-command -
that is an explicitly unsupported interface, and if you find yourself
having to use it, it means we have a hole in libvirt proper, where we
should expose a supported API to get at the same task without going
through the backdoor.  (As past examples, we added virDomainSetTime
because people were previously having to use the agent's back door.)

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



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

[libvirt] run qemu-agent-command via binding (ruby/python/php)

[Vasiliy Tolstov]

Hi! Does it possible(featured, planned) to run some qemu-agent-command
via libvirt binding (i'm interesting on ruby and php).?
I'm understand that i can connect via socket and run it, but it very
usable to get this ability inside binding.

-- 
Vasiliy Tolstov,
e-mail: v.tols...@selfip.ru
jabber: v...@selfip.ru

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

Re: [libvirt] [PATCHv4] qemu: Implement bulk stats API and one of the stats groups to return

[Eric Blake]

On 08/28/2014 06:18 AM, Peter Krempa wrote:
> Implement the API function for virDomainListGetStats and
> virConnectGetAllDomainStats in a modular way and implement the
> VIR_DOMAIN_STATS_STATE group of statistics.
> 
> Although it may look like the function looks universal I'd rather not
> expose it to other drivers as the coming stats groups are likely to do
> qemu specific stuff to obtain the stats.
> ---
> 
> Notes:
> Version 4:
> - fixed handling and error checking of @stats
> - domain filtering flags are now rejected when passing in a domain list

Looks nicer.  Thanks for putting up with me.


> +static int
> +qemuDomainGetStats(virConnectPtr conn,
> +   virDomainObjPtr dom,

> +
> +for (i = 0; qemuDomainGetStatsWorkers[i].func; i++) {
> +if (stats & qemuDomainGetStatsWorkers[i].stats) {
> +if (qemuDomainGetStatsWorkers[i].func(dom, tmp, &maxparams,

These two if's could be combined into one with &&, for one less level of
indentation (but I'm also okay if you leave it alone).

> +  flags) < 0)
> +goto cleanup;
> +}
> +}
> +
> +if (!(tmp->dom = virGetDomain(conn, dom->def->name, dom->def->uuid)))

Earlier versions had a comment about doing dom last for a reason.  Not
sure if you want to reinstate it; I can live without it.


> +}
> +
> +
> +
> +static int
> +qemuConnectGetAllDomainStats(virConnectPtr conn,

3 blank lines is unusual.

ACK.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



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

Re: [libvirt] [PATCH v2 1/2] util: Introduce flags field for macvtap creation

[Martin Kletzander]

On Wed, Aug 27, 2014 at 10:34:13AM -0400, Matthew Rosato wrote:

Currently, there is one flag passed in during macvtap creation
(withTap) -- Let's convert this field to an unsigned int flag
field for future expansion.

Signed-off-by: Matthew Rosato 
---
src/lxc/lxc_process.c   |4 ++--
src/qemu/qemu_command.c |6 --
src/util/virnetdevmacvlan.c |   28 +---
src/util/virnetdevmacvlan.h |   14 ++
4 files changed, 33 insertions(+), 19 deletions(-)

diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c
index 3353dc1..ed30c37 100644
--- a/src/lxc/lxc_process.c
+++ b/src/lxc/lxc_process.c
@@ -331,12 +331,12 @@ char *virLXCProcessSetupInterfaceDirect(virConnectPtr 
conn,
net->ifname, &net->mac,
virDomainNetGetActualDirectDev(net),
virDomainNetGetActualDirectMode(net),
-false, false, def->uuid,
+false, def->uuid,
virDomainNetGetActualVirtPortProfile(net),
&res_ifname,
VIR_NETDEV_VPORT_PROFILE_OP_CREATE,
cfg->stateDir,
-virDomainNetGetActualBandwidth(net)) < 0)
+virDomainNetGetActualBandwidth(net), 0) < 0)
goto cleanup;

ret = res_ifname;
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 9241f57..1f71fa3 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -177,6 +177,7 @@ qemuPhysIfaceConnect(virDomainDefPtr def,
char *res_ifname = NULL;
int vnet_hdr = 0;
virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
+unsigned int macvlan_create_flags = VIR_NETDEV_MACVLAN_CREATE_WITH_TAP;

if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_VNET_HDR) &&
net->model && STREQ(net->model, "virtio"))
@@ -186,11 +187,12 @@ qemuPhysIfaceConnect(virDomainDefPtr def,
net->ifname, &net->mac,
virDomainNetGetActualDirectDev(net),
virDomainNetGetActualDirectMode(net),
-true, vnet_hdr, def->uuid,
+vnet_hdr, def->uuid,
virDomainNetGetActualVirtPortProfile(net),
&res_ifname,
vmop, cfg->stateDir,
-virDomainNetGetActualBandwidth(net));
+virDomainNetGetActualBandwidth(net),
+macvlan_create_flags);
if (rc >= 0) {
virDomainAuditNetDevice(def, net, res_ifname, true);
VIR_FREE(net->ifname);
diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c
index 054194b..9ddeca4 100644
--- a/src/util/virnetdevmacvlan.c
+++ b/src/util/virnetdevmacvlan.c
@@ -790,6 +790,7 @@ virNetDevMacVLanVPortProfileRegisterCallback(const char 
*ifname,
 * @macaddress: The MAC address for the macvtap device
 * @linkdev: The interface name of the NIC to connect to the external bridge
 * @mode: int describing the mode for 'bridge', 'vepa', 'private' or 'passthru'.
+ * @flags: OR of virNetDevMacVLanCreateFlags.


I took the liberty of moving this as a last parameter as well.


 * @vnet_hdr: 1 to enable IFF_VNET_HDR, 0 to disable it
 * @vmuuid: The UUID of the VM the macvtap belongs to
 * @virtPortProfile: pointer to object holding the virtual port profile data
@@ -797,25 +798,29 @@ virNetDevMacVLanVPortProfileRegisterCallback(const char 
*ifname,
 * interface will be stored into if everything succeeded. It is up
 * to the caller to free the string.
 *
- * Returns file descriptor of the tap device in case of success with @withTap,
- * otherwise returns 0; returns -1 on error.
+ * Returns file descriptor of the tap device in case of success with
+ * @flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP, otherwise returns 0; returns
+ * -1 on error.
 */
int virNetDevMacVLanCreateWithVPortProfile(const char *tgifname,
   const virMacAddr *macaddress,
   const char *linkdev,
   virNetDevMacVLanMode mode,
-   bool withTap,
   int vnet_hdr,
   const unsigned char *vmuuid,
   virNetDevVPortProfilePtr 
virtPortProfile,
   char **res_ifname,
   virNetDevVPortProfileOp vmOp,
   char *stateDir,
-   virNetDevBandwidthPtr bandwidth)
+   virNetDevBandwidthPtr bandwidth,
+   unsigned int flags)
{
-const char *type = withTap ? "macvtap" : "macvlan";
-const char *prefix = withTap ? MACVTAP_NAME_PREFIX : MACVLAN_NAME_PREFIX;
-const char *pattern = withTap ? MACVTAP_NAME_PATTERN : 
MACVLAN_NAME_PATTERN;
+const char *type = (flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) ?
+"macvtap" : "macvlan";
+const char *prefix = (flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) ?
+MACVTAP_NAME_PR

Re: [libvirt] [PATCH 00/19] More Coverity patches

[John Ferlan]

On 08/27/2014 04:54 PM, John Ferlan wrote:
> I almost didn't want to do this due to the sheer volume, but figured
> at the very least the bulk of these are resource leaks found by the
> much pickier new coverity scanner.
> 
> After this there are "only" 70 issues found... 
> 
> John Ferlan (19):
>   libxl_migration: Resolve Coverity NULL_RETURNS
>   daemon: Resolve Coverity NEGATIVE_RETURNS
>   domain_conf: Resolve Coverity RESOURCE_LEAK
>   cpu_x86: Resolve Coverity RESOURCE_LEAK
>   qemu_command: Resolve Coverity RESOURCE_LEAK
>   qemu_agent: Resolve Coverity RESOURCE_LEAK
>   libxl_domain: Resolve Coverity RESOURCE_LEAK
>   qemu_capabilities: Resolve Coverity RESOURCE_LEAK
>   network_conf: Resolve Coverity RESOURCE_LEAK
>   virsh-network: Resolve Coverity RESOURCE_LEAK
>   bridge_driver: Resolve Coverity RESOURCE_LEAK
>   libxl_migration: Resolve Coverity RESOURCE_LEAK
>   phyp_driver: Resolve Coverity RESOURCE_LEAK
>   qemu_driver: Resolve Coverity RESOURCE_LEAK
>   storage_conf: Resolve Coverity RESOURCE_LEAK
>   qemu_monitor: Resolve Coverity NESTING_INDENT_MISMATCH
>   domain_conf: Resolve Coverity DEADCODE
>   qemu_driver: Resolve Coverity DEADCODE
>   qemu_command: Resolve Coverity DEADCODE
> 
>  daemon/remote.c  | 24 
>  src/conf/domain_conf.c   | 28 
>  src/conf/network_conf.c  |  2 ++
>  src/conf/storage_conf.c  |  2 ++
>  src/cpu/cpu_x86.c| 15 ++-
>  src/libxl/libxl_domain.c |  4 +++-
>  src/libxl/libxl_migration.c  | 11 +--
>  src/network/bridge_driver.c  |  1 +
>  src/phyp/phyp_driver.c   |  1 +
>  src/qemu/qemu_agent.c|  6 --
>  src/qemu/qemu_capabilities.c |  2 +-
>  src/qemu/qemu_command.c  |  9 +
>  src/qemu/qemu_driver.c   |  8 
>  src/qemu/qemu_monitor.c  |  3 ++-
>  tools/virsh-network.c|  2 +-
>  15 files changed, 85 insertions(+), 33 deletions(-)
> 

I removed patch 1 (letting Jim handle it)

I modified patch 3 (or now 2) to use virDomainVcpuPinDefFree()

I have pushed the series.

Thanks for the quick review -

John

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

Re: [libvirt] [PATCH v1 6/6] ivshmem: add start param to server attribute

[David Marchand]

On 08/28/2014 12:34 PM, Martin Kletzander wrote:


Great!  Really, and it's even easier than what I thought of.  We just
need to kill the server if we fail to start QEMU after starting the
server.

There is one caveat, though.  There is a race between libvirt checking
whether the socket exists and last domain disconnecting.  We also need
to be sure that the server is running and accepts the connection from
QEMU.  And we need to be able to specify the SELinux context of the
socket before it is created.  Adding SELinux support into
ivshmem-server would be too much, however.

Second and third points could be completely eliminated by the server
accepting LISTEN_FDS environment variable which would say that libvirt
is passing an FD with the socket already opened (libvirt would be sure
QEMU is already connected to the socket, and it would take care of the
permissions).  To see how this works you can have a looks at our
daemon code for libvirtd or virtlock, but even easier would probably
be to check systemd's documentation on socket activation (even though
this has nothing to do with systemd).  I coded it up without systemd
based on:

http://www.freedesktop.org/software/systemd/man/sd_listen_fds.html

and some other pages.  Trying to understand it from libvirt sources
might be an overkill.

I'm not sure, though, what to do with the first point (race between
libvirt creating the socket to see that it exists and ivshmem
disconnecting).  Maybe libvirt could do this (if QEMU would support
it):

1: try to create the socket (if it exists, continue with 4)

2: connect to the socket

3: if connecting fails, goto 1;

4: if libvirt was the one who created the socket, start the server
and pass the FD of the socket to it

5: start qemu and pass the socket to it (qemu already supports that
with other devices like netdevs, etc.

This would take care of all three points.  No race, no permission
issues, nothing.

What do you think?


- Mmm, I had felt that there could be a race in the socket check, yes.
The LISTEN_FDS support in the server is not that big of a change.
I think I can take care of that.


- Did not think of the other points.
I think there is still some problem with your proposition, I need more 
time to think about it (may be some prototyping to be sure).







[snip]


+/**
+ * virStopIvshmemServer:
+ * @shm_name: the name of the shared memory
+ *
+ * Stop an Ivshmem Server
+ *
+ * Returns 0 in case of success or -1 in case of failure.
+ */
+int
+virStopIvshmemServer(const char *shm_name)
+{
+char *ivshmserver_pidbase = NULL;
+pid_t ivshmserver_pid;
+int ret = -1;
+
+/* get pid of the ivshmem server */
+if (!(ivshmserver_pidbase =
virIvshmemServerPidFileBasename(shm_name)))
+goto cleanup;
+if (virPidFileRead(IVSHMEM_STATE_DIR, ivshmserver_pidbase,
+&ivshmserver_pid))
+goto cleanup;
+
+virProcessKill(ivshmserver_pid, SIGTERM);
+virPidFileDelete(IVSHMEM_STATE_DIR, ivshmserver_pidbase);


The pidfile support could be added to the server too, maybe...


I am not sure I understand the point.

The only thing this code does here is retrieve the current pid for the
ivshmem-server, it kills the associated pid then remove the pidfile.

So what do you mean by "pidfile support" ?
Do you suggest that, on exit, the ivshmem-server should remove the
pidfile it first created ?



Disregard this comment, our above idea takes care of anything I might
have meant in here :)


Yes, I thought so, no problem.


Thanks Martin.

--
David Marchand

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

Re: [libvirt] [PATCH 00/19] More Coverity patches

[John Ferlan]

On 08/27/2014 09:38 PM, Wang Rui wrote:
> On 2014/8/28 4:54, John Ferlan wrote:
>> I almost didn't want to do this due to the sheer volume, but figured
>> at the very least the bulk of these are resource leaks found by the
>> much pickier new coverity scanner.
>>
>> After this there are "only" 70 issues found... 
> 
> Nice. I did coverity scan yesterday by coincidence. There are 1400+
> total errors on my scan environment. I began to analyze 300+ RESOURCE LEAK
> errors. You said there were more than 140 on your side. So when you have
> finished fixing errors on your side, I'm glad to check if some other
> RESOURCE LEAK errors left and fix them
> 

I used "Coverity Static Analysis version 7.5.0 on Linux
3.15.8-200.fc20.x86_64 x86_64" that we have as a shared license of sorts
inside Red Hat.

I generally just take the defaults for the analysis "strength" (eg, the
--aggressiveness-level is the default of low rather than medium or high,
which I know from experience are much more intolerant).

My 141 count covered the gamut of issues and will be reduced to 72 with
these patches applied.  Some of the remaining are sourced from the same
problem - some are false positives.  It just takes time to go through
and figure that out and what the "best solution" may be.

Perhaps you are running with a different version or a different
aggressiveness especially to have 300+ resource leaks.  That's fine -
the more found the better off we are.

John

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

[libvirt] Cosmetic bug on libvirt.org

[Christophe Fergeau]

Hey,

I just noticed that clicking on "FAQ" in the left sidebar on libvirt.org
main page highlights the "Wiki" cell instead of highlighting "FAQ".
I have no idea how the website works, so I'm just reporting it here.

Cheers,

Christophe


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

[libvirt] [PATCHv4] qemu: Implement bulk stats API and one of the stats groups to return

[Peter Krempa]

Implement the API function for virDomainListGetStats and
virConnectGetAllDomainStats in a modular way and implement the
VIR_DOMAIN_STATS_STATE group of statistics.

Although it may look like the function looks universal I'd rather not
expose it to other drivers as the coming stats groups are likely to do
qemu specific stuff to obtain the stats.
---

Notes:
Version 4:
- fixed handling and error checking of @stats
- domain filtering flags are now rejected when passing in a domain list

 src/qemu/qemu_driver.c | 198 +
 1 file changed, 198 insertions(+)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 73959da..45a080b 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -17190,6 +17190,203 @@ qemuConnectGetDomainCapabilities(virConnectPtr conn,
 }


+static int
+qemuDomainGetStatsState(virDomainObjPtr dom,
+virDomainStatsRecordPtr record,
+int *maxparams,
+unsigned int privflags ATTRIBUTE_UNUSED)
+{
+if (virTypedParamsAddInt(&record->params,
+ &record->nparams,
+ maxparams,
+ "state.state",
+ dom->state.state) < 0)
+return -1;
+
+if (virTypedParamsAddInt(&record->params,
+ &record->nparams,
+ maxparams,
+ "state.reason",
+ dom->state.reason) < 0)
+return -1;
+
+return 0;
+}
+
+
+typedef int
+(*qemuDomainGetStatsFunc)(virDomainObjPtr dom,
+  virDomainStatsRecordPtr record,
+  int *maxparams,
+  unsigned int flags);
+
+struct qemuDomainGetStatsWorker {
+qemuDomainGetStatsFunc func;
+unsigned int stats;
+};
+
+static struct qemuDomainGetStatsWorker qemuDomainGetStatsWorkers[] = {
+{ qemuDomainGetStatsState, VIR_DOMAIN_STATS_STATE},
+{ NULL, 0 }
+};
+
+
+static int
+qemuDomainGetStatsCheckSupport(unsigned int *stats,
+   bool enforce)
+{
+unsigned int supportedstats = 0;
+size_t i;
+
+for (i = 0; qemuDomainGetStatsWorkers[i].func; i++)
+supportedstats |= qemuDomainGetStatsWorkers[i].stats;
+
+if (*stats == 0) {
+*stats = supportedstats;
+return 0;
+}
+
+if (enforce &&
+*stats & ~supportedstats) {
+virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED,
+   _("Stats types bits 0x%x are not supported by this 
daemon"),
+   *stats & ~supportedstats);
+return -1;
+}
+
+*stats &= supportedstats;
+return 0;
+}
+
+
+static int
+qemuDomainGetStats(virConnectPtr conn,
+   virDomainObjPtr dom,
+   unsigned int stats,
+   virDomainStatsRecordPtr *record,
+   unsigned int flags)
+{
+int maxparams = 0;
+virDomainStatsRecordPtr tmp;
+size_t i;
+int ret = -1;
+
+if (VIR_ALLOC(tmp) < 0)
+goto cleanup;
+
+for (i = 0; qemuDomainGetStatsWorkers[i].func; i++) {
+if (stats & qemuDomainGetStatsWorkers[i].stats) {
+if (qemuDomainGetStatsWorkers[i].func(dom, tmp, &maxparams,
+  flags) < 0)
+goto cleanup;
+}
+}
+
+if (!(tmp->dom = virGetDomain(conn, dom->def->name, dom->def->uuid)))
+goto cleanup;
+
+*record = tmp;
+tmp = NULL;
+ret = 0;
+
+ cleanup:
+if (tmp) {
+virTypedParamsFree(tmp->params, tmp->nparams);
+VIR_FREE(tmp);
+}
+
+return ret;
+}
+
+
+
+static int
+qemuConnectGetAllDomainStats(virConnectPtr conn,
+ virDomainPtr *doms,
+ unsigned int ndoms,
+ unsigned int stats,
+ virDomainStatsRecordPtr **retStats,
+ unsigned int flags)
+{
+virQEMUDriverPtr driver = conn->privateData;
+virDomainPtr *domlist = NULL;
+virDomainObjPtr dom = NULL;
+virDomainStatsRecordPtr *tmpstats = NULL;
+bool enforce = !!(flags & VIR_CONNECT_GET_ALL_DOMAINS_STATS_ENFORCE_STATS);
+int ntempdoms;
+int nstats = 0;
+size_t i;
+int ret = -1;
+
+if (ndoms)
+virCheckFlags(VIR_CONNECT_GET_ALL_DOMAINS_STATS_ENFORCE_STATS, -1);
+else
+virCheckFlags(VIR_CONNECT_LIST_DOMAINS_FILTERS_ACTIVE |
+  VIR_CONNECT_LIST_DOMAINS_FILTERS_PERSISTENT |
+  VIR_CONNECT_LIST_DOMAINS_FILTERS_STATE |
+  VIR_CONNECT_GET_ALL_DOMAINS_STATS_ENFORCE_STATS, -1);
+
+if (virConnectGetAllDomainStatsEnsureACL(conn) < 0)
+return -1;
+
+if (qemuDomainGetStatsCheckSupport(&stats, enforce) < 0)
+return -1;
+
+if (!ndoms) {
+unsigned int

Re: [libvirt] [PATCH 01/19] libxl_migration: Resolve Coverity NULL_RETURNS

[John Ferlan]

On 08/27/2014 05:29 PM, Jim Fehlig wrote:
> John Ferlan wrote:
>> Coverity noted that all callers to libxlDomainEventQueue() could ensure
>> the second parameter (event) was true before calling except this case.
>> As I look at the code and how events are used - it seems that prior to
>> generating an event for the dom == NULL condition, the resume/suspend
>> event should be queue'd after the virDomainSaveStatus() call which will
>> goto cleanup and queue the saved event anyway.
>>
>> Signed-off-by: John Ferlan 
>> ---
>>  src/libxl/libxl_migration.c | 6 +-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c
>> index dbb5a8f..53ae63a 100644
>> --- a/src/libxl/libxl_migration.c
>> +++ b/src/libxl/libxl_migration.c
>> @@ -512,6 +512,11 @@ libxlDomainMigrationFinish(virConnectPtr dconn,
>>  if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0)
>>  goto cleanup;
>>  
>> +if (event) {
>> +libxlDomainEventQueue(driver, event);
>> +event = NULL;
>> +}
>> +
>>  dom = virGetDomain(dconn, vm->def->name, vm->def->uuid);
>>  
>>  if (dom == NULL) {
>> @@ -519,7 +524,6 @@ libxlDomainMigrationFinish(virConnectPtr dconn,
>>  libxlDomainCleanup(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED);
>>  event = virDomainEventLifecycleNewFromObj(vm, 
>> VIR_DOMAIN_EVENT_STOPPED,
>>   VIR_DOMAIN_EVENT_STOPPED_FAILED);
>> -libxlDomainEventQueue(driver, event);
>>  }
>>   
> 
> See my question in your first series about whether the dom == NULL check
> is even needed.  If not, I can send a patch to remove the check, in
> which case this patch wouldn't be needed.
> 
> https://www.redhat.com/archives/libvir-list/2014-August/msg01399.html
> 
> 

I am going to remove this one from my series and let you handle it.

I would seem perhaps that the code was there to ensure that if either of
the calls to virDomainObjSetState() ended up resulting in 'dom' not
returning something from the virGetDomain(), then like perhaps the qemu
migration code, the "best choice" was to remove it. In particular I
looked at the second VIR_DOMAIN_SHUTOFF_FAILED in qemuMigrationFinish()
for a "comparison".  I'm by far no libvirt migration processing expert
though...

Tks -

John

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

Re: [libvirt] [PATCHv3 5/5] virsh: Implement command to excercise the bulk stats APIs

[Peter Krempa]

On 08/28/14 00:15, Eric Blake wrote:
> On 08/27/2014 12:25 PM, Peter Krempa wrote:
>> Add "domstats" command that excercises both of the new APIs depending if
> 
> s/excercises/exercises/

Hmmm, now that I've pushed this patch I noticed this typo :/ ... Oh
well. Everybody knows that my English spelling is terrible :)

> 
>> you specify a domain list or not. The output is printed as a key=value
>> list of the returned parameters.
>> ---
>>  tools/virsh-domain-monitor.c | 191 
>> +++
>>  tools/virsh.pod  |  34 
>>  2 files changed, 225 insertions(+)
>>

...

> 
> ACK with the tweaks above.
> 

I didn't forget to resolve the comments above before pushing.

Thanks.

peter



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

[libvirt] [PATCH RFC] qemu: add some synchronizations for snapshot

[Jincheng Miao]

Currently it lacks synchronization to modify domain's snapshot object list,
that race condition causes unsafe access to some freed snapshot objects.
Therefore, this patch wraps all access of snapshot object list in vm job lock.

Only the qemuDomainSnapshotCreateXML is not synchronized, it is related to
QEMU_ASYNC_JOB_SNAPSHOT async job for --disk-only snapshot. I am not sure
if it's ok to remove it, so need your ideas for qemuDomainSnapshotCreateXML.

Signed-off-by: Jincheng Miao 
---
 src/qemu/qemu_driver.c |  137 +++-
 1 files changed, 112 insertions(+), 25 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 73959da..7329aa9 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -13609,6 +13609,7 @@ static int qemuDomainSnapshotListNames(virDomainPtr 
domain, char **names,
 {
 virDomainObjPtr vm = NULL;
 int n = -1;
+virQEMUDriverPtr driver = domain->conn->privateData;
 
 virCheckFlags(VIR_DOMAIN_SNAPSHOT_LIST_ROOTS |
   VIR_DOMAIN_SNAPSHOT_FILTERS_ALL, -1);
@@ -13619,8 +13620,12 @@ static int qemuDomainSnapshotListNames(virDomainPtr 
domain, char **names,
 if (virDomainSnapshotListNamesEnsureACL(domain->conn, vm->def) < 0)
 goto cleanup;
 
+if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0)
+goto cleanup;
+
 n = virDomainSnapshotObjListGetNames(vm->snapshots, NULL, names, nameslen,
  flags);
+ignore_value(qemuDomainObjEndJob(driver, vm));
 
  cleanup:
 if (vm)
@@ -13633,6 +13638,7 @@ static int qemuDomainSnapshotNum(virDomainPtr domain,
 {
 virDomainObjPtr vm = NULL;
 int n = -1;
+virQEMUDriverPtr driver = domain->conn->privateData;
 
 virCheckFlags(VIR_DOMAIN_SNAPSHOT_LIST_ROOTS |
   VIR_DOMAIN_SNAPSHOT_FILTERS_ALL, -1);
@@ -13643,8 +13649,13 @@ static int qemuDomainSnapshotNum(virDomainPtr domain,
 if (virDomainSnapshotNumEnsureACL(domain->conn, vm->def) < 0)
 goto cleanup;
 
+if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0)
+goto cleanup;
+
 n = virDomainSnapshotObjListNum(vm->snapshots, NULL, flags);
 
+ignore_value(qemuDomainObjEndJob(driver, vm));
+
  cleanup:
 if (vm)
 virObjectUnlock(vm);
@@ -13657,6 +13668,7 @@ qemuDomainListAllSnapshots(virDomainPtr domain, 
virDomainSnapshotPtr **snaps,
 {
 virDomainObjPtr vm = NULL;
 int n = -1;
+virQEMUDriverPtr driver = domain->conn->privateData;
 
 virCheckFlags(VIR_DOMAIN_SNAPSHOT_LIST_ROOTS |
   VIR_DOMAIN_SNAPSHOT_FILTERS_ALL, -1);
@@ -13667,8 +13679,13 @@ qemuDomainListAllSnapshots(virDomainPtr domain, 
virDomainSnapshotPtr **snaps,
 if (virDomainListAllSnapshotsEnsureACL(domain->conn, vm->def) < 0)
 goto cleanup;
 
+if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0)
+goto cleanup;
+
 n = virDomainListSnapshots(vm->snapshots, NULL, domain, snaps, flags);
 
+ignore_value(qemuDomainObjEndJob(driver, vm));
+
  cleanup:
 if (vm)
 virObjectUnlock(vm);
@@ -13684,6 +13701,7 @@ 
qemuDomainSnapshotListChildrenNames(virDomainSnapshotPtr snapshot,
 virDomainObjPtr vm = NULL;
 virDomainSnapshotObjPtr snap = NULL;
 int n = -1;
+virQEMUDriverPtr driver = snapshot->domain->conn->privateData;
 
 virCheckFlags(VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS |
   VIR_DOMAIN_SNAPSHOT_FILTERS_ALL, -1);
@@ -13694,12 +13712,18 @@ 
qemuDomainSnapshotListChildrenNames(virDomainSnapshotPtr snapshot,
 if (virDomainSnapshotListChildrenNamesEnsureACL(snapshot->domain->conn, 
vm->def) < 0)
 goto cleanup;
 
-if (!(snap = qemuSnapObjFromSnapshot(vm, snapshot)))
+if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0)
 goto cleanup;
 
+if (!(snap = qemuSnapObjFromSnapshot(vm, snapshot)))
+goto endjob;
+
 n = virDomainSnapshotObjListGetNames(vm->snapshots, snap, names, nameslen,
  flags);
 
+ endjob:
+ignore_value(qemuDomainObjEndJob(driver, vm));
+
  cleanup:
 if (vm)
 virObjectUnlock(vm);
@@ -13713,6 +13737,7 @@ qemuDomainSnapshotNumChildren(virDomainSnapshotPtr 
snapshot,
 virDomainObjPtr vm = NULL;
 virDomainSnapshotObjPtr snap = NULL;
 int n = -1;
+virQEMUDriverPtr driver = snapshot->domain->conn->privateData;
 
 virCheckFlags(VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS |
   VIR_DOMAIN_SNAPSHOT_FILTERS_ALL, -1);
@@ -13723,11 +13748,17 @@ qemuDomainSnapshotNumChildren(virDomainSnapshotPtr 
snapshot,
 if (virDomainSnapshotNumChildrenEnsureACL(snapshot->domain->conn, vm->def) 
< 0)
 goto cleanup;
 
-if (!(snap = qemuSnapObjFromSnapshot(vm, snapshot)))
+if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0)
 goto cleanup;
 
+if (!(snap = qemuSnapObjFromSnapshot(vm, snapshot)))
+goto endjob;
+
 n = virDomainSnapshotOb

Re: [libvirt] [PATCH 0/3] Coverity patches to resolve RESOURCE_LEAK

[Ján Tomko]

On 08/28/2014 12:20 PM, Wang Rui wrote:
> I did coverity scan for libvirt-1.2.8 as John Ferlan did.

Technically, libvirt-1.2.8 is not yet released, just -rc1 :)

> He has sent many patches about RESOURCE_LEAK. I picked 
> the other errors left to fix. There are also many errors
> to analyze and fix in the future.
> 
> Wang Rui (3):
>   util: Resolve Coverity RESOURCE_LEAK
>   tests: Resolve Coverity RESOURCE_LEAK
>   qemu_capabilities: Resolve Coverity RESOURCE_LEAK
> 
>  src/qemu/qemu_capabilities.c | 4 +++-
>  src/util/virpci.c| 1 +
>  tests/shunloadtest.c | 1 +
>  3 files changed, 5 insertions(+), 1 deletion(-)
> 

ACK series; now pushed.

Jan



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

Re: [libvirt] [PATCH v1 6/6] ivshmem: add start param to server attribute

[Martin Kletzander]

On Thu, Aug 28, 2014 at 10:34:11AM +0200, David Marchand wrote:

Hello Martin,


On 08/26/2014 11:33 AM, Martin Kletzander wrote:

[Cc'ing David due to some questions/ideas about the ivshmem server]


@@ -5120,6 +5121,12 @@ qemuBuildIvshmemCommandLine(virCommandPtr cmd,
return -1;
virCommandAddArg(cmd, devstr);
VIR_FREE(devstr);
+
+if (ivshmem->server.start == VIR_TRISTATE_BOOL_YES) {
+if (virStartIvshmemServer(dev->name,
ivshmem->server.path,
+  ivshmem->size,
ivshmem->msi.vectors))
+return -1;
+}
}


What if the server is already running?  Either 2 domains have server
start='yes' or the user started it already.  We should not fail in
these cases, I guess.



[snip]



It would be great if the domain with server start='yes' didn't have to
be started first and killed last.  Or if we could just signal the
server (let's say SIGUSR1) that it should unlink the shmem and quit if
no domains are connected.  That way we could just send a signal to the
server whenever any domain is disconnecting (maybe some check that no
domain is being started might be good too ;) ).


Why not.

If we want to automagically start/stop the server, we don't need the
'start' field, but we need a way to find if a server is running for this
shared mem instance.

We can try to bind the unix socket and check if a EADDRINUSE is returned.
  If EADDRINUSE, server is running, nothing to do.
  Else libvirt starts the server with the option "-q" (new option I can
  add which means "quit on last client disconnect")

On stop, libvirt does nothing about the ivshmem-server.

With this, if the server had been started with "-q" (by libvirt or by
the user), then the server will quit on the last disconnect.
If the user did not start the server with -q, it is his reponsibility to
stop it.

The 'start' field can disappear.
No need to send any signal to ivshmem-server.


What do you think of this ?



Great!  Really, and it's even easier than what I thought of.  We just
need to kill the server if we fail to start QEMU after starting the
server.

There is one caveat, though.  There is a race between libvirt checking
whether the socket exists and last domain disconnecting.  We also need
to be sure that the server is running and accepts the connection from
QEMU.  And we need to be able to specify the SELinux context of the
socket before it is created.  Adding SELinux support into
ivshmem-server would be too much, however.

Second and third points could be completely eliminated by the server
accepting LISTEN_FDS environment variable which would say that libvirt
is passing an FD with the socket already opened (libvirt would be sure
QEMU is already connected to the socket, and it would take care of the
permissions).  To see how this works you can have a looks at our
daemon code for libvirtd or virtlock, but even easier would probably
be to check systemd's documentation on socket activation (even though
this has nothing to do with systemd).  I coded it up without systemd
based on:

http://www.freedesktop.org/software/systemd/man/sd_listen_fds.html

and some other pages.  Trying to understand it from libvirt sources
might be an overkill.

I'm not sure, though, what to do with the first point (race between
libvirt creating the socket to see that it exists and ivshmem
disconnecting).  Maybe libvirt could do this (if QEMU would support
it):

1: try to create the socket (if it exists, continue with 4)

2: connect to the socket

3: if connecting fails, goto 1;

4: if libvirt was the one who created the socket, start the server
   and pass the FD of the socket to it

5: start qemu and pass the socket to it (qemu already supports that
   with other devices like netdevs, etc.

This would take care of all three points.  No race, no permission
issues, nothing.

What do you think?



[snip]


+/**
+ * virStopIvshmemServer:
+ * @shm_name: the name of the shared memory
+ *
+ * Stop an Ivshmem Server
+ *
+ * Returns 0 in case of success or -1 in case of failure.
+ */
+int
+virStopIvshmemServer(const char *shm_name)
+{
+char *ivshmserver_pidbase = NULL;
+pid_t ivshmserver_pid;
+int ret = -1;
+
+/* get pid of the ivshmem server */
+if (!(ivshmserver_pidbase =
virIvshmemServerPidFileBasename(shm_name)))
+goto cleanup;
+if (virPidFileRead(IVSHMEM_STATE_DIR, ivshmserver_pidbase,
+&ivshmserver_pid))
+goto cleanup;
+
+virProcessKill(ivshmserver_pid, SIGTERM);
+virPidFileDelete(IVSHMEM_STATE_DIR, ivshmserver_pidbase);


The pidfile support could be added to the server too, maybe...


I am not sure I understand the point.

The only thing this code does here is retrieve the current pid for the
ivshmem-server, it kills the associated pid then remove the pidfile.

So what do you mean by "pidfile support" ?
Do you suggest that, on exit, the ivshmem-server should remove the
pid

Re: [libvirt] [PATCH 06/19] qemu_agent: Resolve Coverity RESOURCE_LEAK

[Wang Rui]

On 2014/8/28 17:03, Ján Tomko wrote:
> On 08/28/2014 04:40 AM, Wang Rui wrote:
>> On 2014/8/28 4:54, John Ferlan wrote:
>>> Coverity found that on error paths, the 'arg' value wasn't be cleaned
>>> up. Followed the example in qemuAgentSetVCPUs() where upon successful call
>>> to qemuAgentCommand() the 'cpus' is set to NULL; otherwise, when cleanup
>>> occurs the free the memory for 'arg'
>>>
>>> Signed-off-by: John Ferlan 
>>> ---
>>>  src/qemu/qemu_agent.c | 6 --
>>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>>
[...]
>> Setting arg to NULL can also lead to memory leak.
>> It makes virJSONValueFree(arg) below invalid.
> 
> If qemuAgentMakeCommand succeeds, the 'arg' array is now owned by 'cmd' and we
> need to set it to NULL here to prevent double free.

Oh, I got it. Thanks.



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

[libvirt] [PATCH 1/3] util: Resolve Coverity RESOURCE_LEAK

[Wang Rui]

Coverity determined that 'conflict' would be leaked.

Signed-off-by: Wang Rui 
---
 src/util/virpci.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/util/virpci.c b/src/util/virpci.c
index 0098d6c..f1d4499 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -801,6 +801,7 @@ virPCIDeviceTrySecondaryBusReset(virPCIDevicePtr dev,
 virReportError(VIR_ERR_INTERNAL_ERROR,
_("Active %s devices on bus with %s, not doing bus 
reset"),
conflict->name, dev->name);
+virPCIDeviceFree(conflict);
 return -1;
 }
 
-- 
1.7.12.4


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

[libvirt] [PATCH 0/3] Coverity patches to resolve RESOURCE_LEAK

[Wang Rui]

I did coverity scan for libvirt-1.2.8 as John Ferlan did.
He has sent many patches about RESOURCE_LEAK. I picked 
the other errors left to fix. There are also many errors
to analyze and fix in the future.

Wang Rui (3):
  util: Resolve Coverity RESOURCE_LEAK
  tests: Resolve Coverity RESOURCE_LEAK
  qemu_capabilities: Resolve Coverity RESOURCE_LEAK

 src/qemu/qemu_capabilities.c | 4 +++-
 src/util/virpci.c| 1 +
 tests/shunloadtest.c | 1 +
 3 files changed, 5 insertions(+), 1 deletion(-)

-- 
1.7.12.4


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

[libvirt] [PATCH 2/3] tests: Resolve Coverity RESOURCE_LEAK

[Wang Rui]

The 'lib' handle will be leaked if 'dlsym' condition fails.
So close the handle before return.

Signed-off-by: Wang Rui 
---
 tests/shunloadtest.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/shunloadtest.c b/tests/shunloadtest.c
index 499b1be..80f5351 100644
--- a/tests/shunloadtest.c
+++ b/tests/shunloadtest.c
@@ -114,6 +114,7 @@ int main(int argc ATTRIBUTE_UNUSED, char **argv)
 }
 if (!(startup = dlsym(lib, "shunloadStart"))) {
 fprintf(stderr, "Cannot find shunloadStart %s\n", dlerror());
+dlclose(lib);
 return 1;
 }
 
-- 
1.7.12.4


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

[libvirt] [PATCH 3/3] qemu_capabilities: Resolve Coverity RESOURCE_LEAK

[Wang Rui]

In function virQEMUCapsParseMachineTypesStr, VIR_STRNDUP allocates
memory for 'name' in {do,while} loop. If 'name' isn't freed before
'continue', its memory will be allocated again in the next loop.
In this case the memory allocated for 'name' in privious loop is
useless and not freed. Free it before continue this loop to fix that.

Signed-off-by: Wang Rui 
---
 src/qemu/qemu_capabilities.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index ce899f2..4a540ee 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -433,8 +433,10 @@ virQEMUCapsParseMachineTypesStr(const char *output,
 
 if ((t = strstr(p, "(alias of ")) && (!next || t < next)) {
 p = t + strlen("(alias of ");
-if (!(t = strchr(p, ')')) || (next && t >= next))
+if (!(t = strchr(p, ')')) || (next && t >= next)) {
+VIR_FREE(name);
 continue;
+}
 
 if (VIR_STRNDUP(canonical, p, t - p) < 0) {
 VIR_FREE(name);
-- 
1.7.12.4


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

Re: [libvirt] [PATCH v1 6/6] ivshmem: add start param to server attribute

[Martin Kletzander]

On Thu, Aug 28, 2014 at 02:35:10PM +0800, Wang Rui wrote:

On 2014/8/28 5:20, Maxime Leroy wrote:

On Tue, Aug 26, 2014 at 11:58 AM, Wang Rui  wrote:

On 2014/8/22 18:47, Maxime Leroy wrote:


+# util/virivshmemserver.h
+virStartIvshmemServer;
+virStopIvshmemServer;


I think function name virIvshmemStartServer is better.
So is the stop function.



What about virIvshmemServerStart ?


It looks fine.



Yes, it looks better.


@@ -5120,6 +5121,12 @@ qemuBuildIvshmemCommandLine(virCommandPtr cmd,
 return -1;
 virCommandAddArg(cmd, devstr);
 VIR_FREE(devstr);
+
+if (ivshmem->server.start == VIR_TRISTATE_BOOL_YES) {
+if (virStartIvshmemServer(dev->name, ivshmem->server.path,
+  ivshmem->size, ivshmem->msi.vectors))
+return -1;
+}
 }


I'm not sure that calling virStartIvshmemServer in qemuBuildIvshmemCommandLine
is the best way. Maybe qemuBuild*CommandLine() usually only build commandline.



Calling virStartIvshmemServer in qemuProcessStart should be better ?


Looks better, too. But we'd better to wait for other opinions.



It should be started as late as possible and stopped if no client
connects there.  This will need more changes I think, we'll discuss it
with David in the other thread.

Martin


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

Re: [libvirt] [PATCH v1 2/6] conf: Parse and format shmem device XML

[Martin Kletzander]

On Wed, Aug 27, 2014 at 10:52:02PM +0200, Maxime Leroy wrote:

On Tue, Aug 26, 2014 at 10:42 AM, Martin Kletzander  wrote:

On Fri, Aug 22, 2014 at 12:47:01PM +0200, Maxime Leroy wrote:


This patch adds configuration support for the shmem device
as described in the schema in the previous patch.


[...]


This parsing using loop over children does not readably (and in this
case neither reliably) check whether required options are present.


For example, I could use: servernode = virXPathNode("./server", ctxt))
instead of having a loop ?



Yes, sure.  I think that the parsing parts using the loops are
some very old code leftovers.  Maybe from some SAX parsing, but I
don't remember that, just guessing.


Currently it parses  as valid, but not specifying
the size is probably not what you want on qemu command-line ... [1]


Size is an optional parameter of ivshmem in qemu. By default, the size is 4MB.
Thus  is valid.



Oh, I didn't know that.  That is OK, but it should be mentioned in the
documentation, and tests for that should be included as well.


[...]

+if (vectors &&
+virStrToLong_ui(vectors, NULL, 10, &def->msi.vectors)
< 0) {



Use *_uip() if you don't want to accept negative values.



Ok. Thanks.

[...]

+static int virDomainIvshmemDefFormat(virBufferPtr buf,
+ virDomainIvshmemDefPtr def)
+{
+if (def->server.enabled)
+virBufferAsprintf(buf, "\n",
+  def->server.path);



One general idea while looking through the code; could we make the
path optional or even better, leave the "server" out somehow and
generalize it even more.  The path could be for example
"/var/run/libvirt/ivshmem--sock" since we are starting it
anyway, and permissions on that would be easier to set then (whole
path is prepared already).  The name would then be enough to get
either the shmem name or the server socket path.


If libvirt starts the ivshmem server, then we can use a default path.

If libvirt did not start the server, then we should still let the path
as an optional field for cases where the user wants to start the
server himself with a custom path.

We can have an optional path field to handle both cases.



I agree.
...
[coming back after few minutes]
...
Maybe you were right the first time.  Since we're not starting the
server, the path should be specified all the time.  After we add the
support for the server, we can relax the checks for the XML parsing.
That's always possible, but impossible the other way around (i.e. we
cannot make the checks more strict in the future).  So I'd say keep
the path mandatory and relax it in thelast patch that adds the support
for start='yes'.

One more suggestion that would help us get the patches earlier (before
the ivshmem-server is packaged in qemu) would be to check whether the
start='' attribute is in the XML and forbid such configuration.  And
also relax that check after the support for ivshmem-server is added.
Would you be OK with that?  What's you opinion?


Question is whether
we can get the information whether server needs to be started from
somewhere else.  E.g. does server make sense with no msi vectors and
without ioeventfd?


The server handles eventfds distribution across ivshmem clients.

These eventfds are used to trigger interrupts (with or without msi) in
the "guests" clients.

So you can imagine starting a server without msi to only trigger
interrupts in the guests.

I would say that the server can be seen as a "support interrupt"
property of the shmem object but calling it server is more explicit.



That explicit naming is what I wanted to get rid of, originally (why I
asked the question above).  I'm still thinking about the
generalization of "shared memory" instead of exactly matching one
particular technology+hypervisor (qemu+ivshmem).




+if (def->size)
+virBufferAsprintf(buf, "%llu\n",
+  def->size  / (1024 * 1024));
+



If anyone specifies size < 1M, this won't be properly formatted
(another idea for a test case).


Ivshmem in qemu only accepts size in MB or GB.



Well, that should be somehow written in the documentation, then.  But
if I'm reading David's patch properly (not that I'm qemu-experienced
or anything), it might be smaller after his patches:

https://lists.gnu.org/archive/html/qemu-devel/2014-08/msg01243.html

We should somehow be able to parse any size in src/conf/domain_conf.c
(global parsing code), but PostParse() functions for QEMU
(src/qemu/qemu_domain.c) should allow only what's possible in QEMU.
Or even better, if we can introspect how qemu parses the size (whether
size=1 means 1 B or 1 MB; or whether it supports size=1K for example),
we can forbid invalid values in qemuBuildCommandLine() based on the
qemu that we'll be running.

Martin


To prevent this error, we should not accept size under 1MB, in
virDomainIvshmemDefParseXML:

if (virDomainParseScaledValue("./size[1]", ctxt,
-

Re: [libvirt] [PATCHv3 3/5] remote: Implement bulk domain stats APIs in the remote driver

[Peter Krempa]

On 08/27/14 23:02, Eric Blake wrote:
> On 08/27/2014 12:25 PM, Peter Krempa wrote:
>> Implement the remote driver support for shuffling the domain stats
>> around.
>> ---
> 
>> +static int
>> +remoteDispatchConnectGetAllDomainStats(virNetServerPtr server 
>> ATTRIBUTE_UNUSED,
>> +   virNetServerClientPtr client,
>> +   virNetMessagePtr msg 
>> ATTRIBUTE_UNUSED,
>> +   virNetMessageErrorPtr rerr,
>> +   
>> remote_connect_get_all_domain_stats_args *args,
>> +   
>> remote_connect_get_all_domain_stats_ret *ret)
>> +{
>> +int rv = -1;
> 
>> +if (nrecords) {
>> +ret->retStats.retStats_len = nrecords;
>> +
>> +if (VIR_ALLOC_N(ret->retStats.retStats_val, nrecords) < 0)
>> +goto cleanup;
>> +
> 
> I'd switch these for safety (I don't know if the rpc cleanup code will
> try to dereference a NULL retStats.retStats_val if retStats_len is
> non-zero; while setting the length after the allocation should always be
> safe).
> 
>> +for (i = 0; i < nrecords; i++) {
>> +remote_domain_stats_record *dst = ret->retStats.retStats_val + 
>> i;
>> +
>> +make_nonnull_domain(&dst->dom, retStats[i]->dom);
> 
> [Not your fault, and certainly not for this patch, but we REALLY ought
> to fix make_nonnull_domain and friends to properly return errors on OOM,
> instead of silently breaking things]
> 
>> +++ b/src/remote/remote_driver.c
>> @@ -7717,6 +7717,89 @@ remoteNetworkGetDHCPLeases(virNetworkPtr net,
>>  }
>>
>>
>> +static int
>> +remoteConnectGetAllDomainStats(virConnectPtr conn,
>> +   virDomainPtr *doms,
>> +   unsigned int ndoms,
>> +   unsigned int stats,
>> +   virDomainStatsRecordPtr **retStats,
>> +   unsigned int flags)
>> +{
>> +struct private_data *priv = conn->networkPrivateData;
>> +int rv = -1;
>> +size_t i;
>> +remote_connect_get_all_domain_stats_args args;
>> +remote_connect_get_all_domain_stats_ret ret;
>> +
>> +virDomainStatsRecordPtr *tmpret = NULL;
>> +
>> +if (ndoms) {
>> +if (VIR_ALLOC_N(args.doms.doms_val, ndoms) < 0)
>> +goto cleanup;
>> +
>> +for (i = 0; i < ndoms; i++)
>> +make_nonnull_domain(args.doms.doms_val + i, doms[i]);
> 
> [same complaint here about silent OOM bugs]
> 
> ACK with the statement reorder.
> 

I've fixed and pushed patches 1-3 and will follow up with v4 of the rest.

Peter



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

Re: [libvirt] [PATCHv3 2/5] lib: Add few flags for the bulk stats APIs

[Peter Krempa]

On 08/27/14 22:50, Eric Blake wrote:
> On 08/27/2014 12:25 PM, Peter Krempa wrote:
>> Add domain list filtering functions and a flag to enforce checking
>> whether the remote daemon supports the requested stats groups.
>> ---
>>  include/libvirt/libvirt.h.in | 15 +++
>>  src/libvirt.c| 29 -
>>  2 files changed, 43 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
>> index e79c9ad..9358314 100644
>> --- a/include/libvirt/libvirt.h.in
>> +++ b/include/libvirt/libvirt.h.in
>> @@ -2513,6 +2513,21 @@ typedef enum {
>>  VIR_DOMAIN_STATS_STATE = (1 << 0), /* return domain state */
>>  } virDomainStatsTypes;
>>
>> +typedef enum {
>> +VIR_CONNECT_GET_ALL_DOMAINS_STATS_ACTIVE = 
>> VIR_CONNECT_LIST_DOMAINS_ACTIVE,
>> +VIR_CONNECT_GET_ALL_DOMAINS_STATS_INACTIVE = 
>> VIR_CONNECT_LIST_DOMAINS_INACTIVE,
>> +
>> +VIR_CONNECT_GET_ALL_DOMAINS_STATS_PERSISTENT = 
>> VIR_CONNECT_LIST_DOMAINS_PERSISTENT,
>> +VIR_CONNECT_GET_ALL_DOMAINS_STATS_TRANSIENT = 
>> VIR_CONNECT_LIST_DOMAINS_TRANSIENT,
>> +
>> +VIR_CONNECT_GET_ALL_DOMAINS_STATS_RUNNING = 
>> VIR_CONNECT_LIST_DOMAINS_RUNNING,
>> +VIR_CONNECT_GET_ALL_DOMAINS_STATS_PAUSED = 
>> VIR_CONNECT_LIST_DOMAINS_PAUSED,
>> +VIR_CONNECT_GET_ALL_DOMAINS_STATS_SHUTOFF = 
>> VIR_CONNECT_LIST_DOMAINS_SHUTOFF,
>> +VIR_CONNECT_GET_ALL_DOMAINS_STATS_OTHER = 
>> VIR_CONNECT_LIST_DOMAINS_OTHER,
> 
> Is this going to confuse the python bindings?  (I know in other places
> where we have set up one enum to match another that it didn't seem to
> work).  But fixing that would be a separate patch; it may be that
> docs/apibuild.py needs to learn how to do symbolic name dereferencing
> before generating the xml that feeds the python bindings (see also
> commit 9b291bb).
> 
> 
>> + *
>> + * Similarly to virConnectListAllDomains @flags can contain various flags to
> 
> s/ListAllDomains/ListAllDomains,/
> 
>> @@ -21590,7 +21610,7 @@ virConnectGetAllDomainStats(virConnectPtr conn,
>>   * @doms: NULL terminated array of domains
>>   * @stats: stats to return, binary-OR of virDomainStatsTypes
>>   * @retStats: Pointer that will be filled with the array of returned stats.
>> - * @flags: extra flags; not used yet, so callers should always pass 0
>> + * @flags: extra flags; binary-OR of virConnectGetAllDomainStatsFlags;
> 
> Why the trailing semicolon?
> 
>> + *
>> + * Note that specifying any of the domain list filtering flags in @flags
>> + * doesn't have effect on this function.
> 
> Fair enough, I suppose.  But I'd rather we error out if filtering flags
> were requested, instead of silently ignoring them, so that if we later
> change our mind, we can do in-place filtering.  I'll probably have more
> to say on this on a later patch.

I changed this sentence to:

 * Note that any of the domain list filtering flags in @flags will be rejected
 * by this function.
 *

> 
> Looks reasonable, so ACK with the grammar fixes
> 

And all the other tweaks you've suggested. 

Peter





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

Re: [libvirt] [PATCH v1 5/6] tests: Add tests for ivshmem device handling

[Martin Kletzander]

On Wed, Aug 27, 2014 at 11:02:36PM +0200, Maxime Leroy wrote:

On Tue, Aug 26, 2014 at 11:02 AM, Martin Kletzander  wrote:

On Fri, Aug 22, 2014 at 12:47:04PM +0200, Maxime Leroy wrote:



[...]

+
+  
+  
+
+
+  
+  32
+  



The qemuxml2xmltest didn't faile with this xml, which means we are not
allocating addresses for shmem devices.  We have to do that and put it
in the XML so (a) the address won't change and (b) that we know which
address is occupied by that in case we'll want to attach something to
the VM.



Ok. Thanks.


And xml2xml (and PARSE_ERROR xml2argv) tests can be squashed into the patch
with documentation and parsing.  Other xml2argv tests can be squashed
into the patch where qemu formats the command-line.



Ok.

I also should merge the documentation and parsing commit ?



That's a common practice, it ensures that if anyone back-ports any
commits anywhere, the documentation will match the parsing and tests
will be included as well.  It's not super-strict, but it's nice to
have and it also helps review sometimes.

Martin


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

Re: [libvirt] [PATCH 00/19] More Coverity patches

[Ján Tomko]

On 08/27/2014 10:54 PM, John Ferlan wrote:
> I almost didn't want to do this due to the sheer volume, but figured
> at the very least the bulk of these are resource leaks found by the
> much pickier new coverity scanner.
> 
> After this there are "only" 70 issues found... 
> 
> John Ferlan (19):
>   libxl_migration: Resolve Coverity NULL_RETURNS

I still don't know how to answer this one.

ACK to the rest, see my reply to 3/19 for a minor tweak.

Jan

>   daemon: Resolve Coverity NEGATIVE_RETURNS
>   domain_conf: Resolve Coverity RESOURCE_LEAK
>   cpu_x86: Resolve Coverity RESOURCE_LEAK
>   qemu_command: Resolve Coverity RESOURCE_LEAK
>   qemu_agent: Resolve Coverity RESOURCE_LEAK
>   libxl_domain: Resolve Coverity RESOURCE_LEAK
>   qemu_capabilities: Resolve Coverity RESOURCE_LEAK
>   network_conf: Resolve Coverity RESOURCE_LEAK
>   virsh-network: Resolve Coverity RESOURCE_LEAK
>   bridge_driver: Resolve Coverity RESOURCE_LEAK
>   libxl_migration: Resolve Coverity RESOURCE_LEAK
>   phyp_driver: Resolve Coverity RESOURCE_LEAK
>   qemu_driver: Resolve Coverity RESOURCE_LEAK
>   storage_conf: Resolve Coverity RESOURCE_LEAK
>   qemu_monitor: Resolve Coverity NESTING_INDENT_MISMATCH
>   domain_conf: Resolve Coverity DEADCODE
>   qemu_driver: Resolve Coverity DEADCODE
>   qemu_command: Resolve Coverity DEADCODE




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

Re: [libvirt] [PATCH 3/4] domain_conf: Add support for iothreads in disk definition

[Martin Kletzander]

On Wed, Aug 27, 2014 at 05:21:39PM +0100, Stefan Hajnoczi wrote:

On Tue, Aug 26, 2014 at 1:38 AM, John Ferlan  wrote:

+value. Each device must use a unique IOThread and threads will
+be numbered from 1 to the domain iothreads value.


It is reasonable for multiple devices to share the same IOThread.

For example, if you define just 1 IOThread and bind all virtio-blk
devices to it you have a similar setup as the QEMU (no IOThreads) with
the big exception that the QEMU global mutex is not needed for
virtio-blk I/O request processing.  This can already result in a
performance improvement and may make sense where there are many disks
but few host CPUs.



This makes sense.  And it's good that John was strict about this
because we will have no problem to relax the checking.  One question,
though.  Is it also OK to hotplug a disk that is about to share the
iothread with some other, aleady plugged, disk?  I assume it is.

Martin


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

Re: [libvirt] [PATCH 06/19] qemu_agent: Resolve Coverity RESOURCE_LEAK

[Ján Tomko]

On 08/28/2014 04:40 AM, Wang Rui wrote:
> On 2014/8/28 4:54, John Ferlan wrote:
>> Coverity found that on error paths, the 'arg' value wasn't be cleaned
>> up. Followed the example in qemuAgentSetVCPUs() where upon successful call
>> to qemuAgentCommand() the 'cpus' is set to NULL; otherwise, when cleanup
>> occurs the free the memory for 'arg'
>>
>> Signed-off-by: John Ferlan 
>> ---
>>  src/qemu/qemu_agent.c | 6 --
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
>> index a10954a..fe38f6d 100644
>> --- a/src/qemu/qemu_agent.c
>> +++ b/src/qemu/qemu_agent.c
>> @@ -1328,7 +1328,7 @@ int qemuAgentFSFreeze(qemuAgentPtr mon, const char 
>> **mountpoints,
>>unsigned int nmountpoints)
>>  {
>>  int ret = -1;
>> -virJSONValuePtr cmd, arg;
>> +virJSONValuePtr cmd, arg = NULL;
>>  virJSONValuePtr reply = NULL;
>>  
>>  if (mountpoints && nmountpoints) {
>> @@ -1343,7 +1343,8 @@ int qemuAgentFSFreeze(qemuAgentPtr mon, const char 
>> **mountpoints,
>>  }
>>  
>>  if (!cmd)
>> -return -1;
>> +goto cleanup;
>> +arg = NULL;
> 
> Setting arg to NULL can also lead to memory leak.
> It makes virJSONValueFree(arg) below invalid.

If qemuAgentMakeCommand succeeds, the 'arg' array is now owned by 'cmd' and we
need to set it to NULL here to prevent double free.

ACK to the patch as-is.

Jan



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

Re: [libvirt] [PATCH 03/19] domain_conf: Resolve Coverity RESOURCE_LEAK

[Ján Tomko]

On 08/27/2014 10:54 PM, John Ferlan wrote:
> Resolve a few RESOURCE_LEAK's identified by Coverity
> 
> Signed-off-by: John Ferlan 
> ---
>  src/conf/domain_conf.c | 15 +++
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 48afb8c..c7dbc73 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -5654,7 +5654,8 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
>  }
>  VIR_FREE(ready);
>  }
> -} else if (xmlStrEqual(cur->name, BAD_CAST "auth")) {
> +} else if (!authdef &&
> +   xmlStrEqual(cur->name, BAD_CAST "auth")) {
>  if (!(authdef = virStorageAuthDefParse(node->doc, cur)))
>  goto error;
>  if ((auth_secret_usage =
> @@ -12069,15 +12070,17 @@ virDomainDefParseXML(xmlDocPtr xml,
>  goto error;
>  }
>  
> -if (vcpupin->vcpuid >= def->vcpus)
> +if (vcpupin->vcpuid >= def->vcpus) {
>  /* To avoid the regression when daemon loading
>   * domain confs, we can't simply error out if
>   *  nodes greater than current vcpus,
>   * ignoring them instead.
>   */
>  VIR_WARN("Ignore vcpupin for not onlined vcpus");
> -else
> +VIR_FREE(vcpupin);

virDomainVcpuPinDefFree should be used here as well as in the 'duplicate
vcpupin' case a few lines above.

ACK with that fixed.

Jan



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

Re: [libvirt] [PATCH V3 0/3] Some improvements for video model

[Wang Rui]

On 2014/8/28 16:05, Gerd Hoffmann wrote:
> On Do, 2014-08-14 at 20:43 +0800, Wang Rui wrote:
>> From: Zeng Junliang 
>>
>> http://www.redhat.com/archives/libvir-list/2014-July/msg00644.html
>>
>> diff to v2:
>> - hide vram attribute silently instead of reporting an error.
>> - introduce three new capabilities for vga.vgamem_mb, vmvga.vgamem_mb and 
>> qxl.vgamem_mb.
>> - fix some error reported by building libvirt.
> 
> Series looks good to me from qemu developers point of view.
> Can't review the libvirt internals though.
> 
> cheers,
>   Gerd

Thanks. :-)


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

Re: [libvirt] [PATCH v1 6/6] ivshmem: add start param to server attribute

[David Marchand]

Hello Martin,


On 08/26/2014 11:33 AM, Martin Kletzander wrote:

[Cc'ing David due to some questions/ideas about the ivshmem server]


@@ -5120,6 +5121,12 @@ qemuBuildIvshmemCommandLine(virCommandPtr cmd,
return -1;
virCommandAddArg(cmd, devstr);
VIR_FREE(devstr);
+
+if (ivshmem->server.start == VIR_TRISTATE_BOOL_YES) {
+if (virStartIvshmemServer(dev->name,
ivshmem->server.path,
+  ivshmem->size,
ivshmem->msi.vectors))
+return -1;
+}
}


What if the server is already running?  Either 2 domains have server
start='yes' or the user started it already.  We should not fail in
these cases, I guess.



[snip]



It would be great if the domain with server start='yes' didn't have to
be started first and killed last.  Or if we could just signal the
server (let's say SIGUSR1) that it should unlink the shmem and quit if
no domains are connected.  That way we could just send a signal to the
server whenever any domain is disconnecting (maybe some check that no
domain is being started might be good too ;) ).


Why not.

If we want to automagically start/stop the server, we don't need the 
'start' field, but we need a way to find if a server is running for this 
shared mem instance.


We can try to bind the unix socket and check if a EADDRINUSE is returned.
  If EADDRINUSE, server is running, nothing to do.
  Else libvirt starts the server with the option "-q" (new option I can
  add which means "quit on last client disconnect")

On stop, libvirt does nothing about the ivshmem-server.

With this, if the server had been started with "-q" (by libvirt or by 
the user), then the server will quit on the last disconnect.
If the user did not start the server with -q, it is his reponsibility to 
stop it.


The 'start' field can disappear.
No need to send any signal to ivshmem-server.


What do you think of this ?


[snip]


+/**
+ * virStopIvshmemServer:
+ * @shm_name: the name of the shared memory
+ *
+ * Stop an Ivshmem Server
+ *
+ * Returns 0 in case of success or -1 in case of failure.
+ */
+int
+virStopIvshmemServer(const char *shm_name)
+{
+char *ivshmserver_pidbase = NULL;
+pid_t ivshmserver_pid;
+int ret = -1;
+
+/* get pid of the ivshmem server */
+if (!(ivshmserver_pidbase =
virIvshmemServerPidFileBasename(shm_name)))
+goto cleanup;
+if (virPidFileRead(IVSHMEM_STATE_DIR, ivshmserver_pidbase,
+&ivshmserver_pid))
+goto cleanup;
+
+virProcessKill(ivshmserver_pid, SIGTERM);
+virPidFileDelete(IVSHMEM_STATE_DIR, ivshmserver_pidbase);


The pidfile support could be added to the server too, maybe...


I am not sure I understand the point.

The only thing this code does here is retrieve the current pid for the 
ivshmem-server, it kills the associated pid then remove the pidfile.


So what do you mean by "pidfile support" ?
Do you suggest that, on exit, the ivshmem-server should remove the 
pidfile it first created ?





--
David Marchand

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

Re: [libvirt] Entering freeze for libvirt-1.2.8

[Daniel Veillard]

On Thu, Aug 28, 2014 at 09:25:22AM +0200, Richard Weinberger wrote:
> Am 28.08.2014 09:14, schrieb Daniel Veillard:
> > On Wed, Aug 27, 2014 at 08:45:29PM +0200, Richard Weinberger wrote:
> >> On Wed, Aug 27, 2014 at 9:18 AM, Daniel Veillard  
> >> wrote:
> >>>   So I tagged 1.2.8-rc1 in git and made tarball and signed rpms
> >>
> >> Can you please sign the tarball too?
> > 
> >   Well, the source rpm is signed, you can check it and it contains the
> > tarball, so technically there is already a signed source out there.
> > Signing a tarballl means putting out an additional file and keeping
> > it forever, I could do that but hum 
> 
> So everyone how wants to build libvirt from source and cares about data
> integrity has to unpack/verify the rpm?

  Assuming you already loaded my key with rpm --import from what I make
available on http://veillard.com/

  one download, and 2 automated rpm commands
wget ftp://libvirt.org/libvirt/libvirt-x.y.x-1.*.src.rpm

 even if you got DNS poisoning here, the following step would fail
that key wasn't 
rpm -K libvirt-x.y.x-1.*.src.rpm
rpm -i libvirt-x.y.x-1.*.src.rpm

  use the tar.gz in confidence

> Signing tarballs is nothing new nor rocket science.
> In times where the NSA tries to f*ck everyone at least some basic
> cryptographic arrangements should be applied.

  Give me a mechanism where one can do that checking as fast and in
a completely automated way and I  implement it :-)

> I know other projects are sloppy regarding signed releases too, this does
> not mean that libvirt should follow their bad example.

  I have not been sloppy, I have signed all the sources rpms from day 0
I also sign the corresponing git commits. The main issue is having a
clear, simple and failure proof process of checking a chunk of data
produced by the release. rpm has provided that for 15+ years. All the
alternatives I know require some human checking either by comparing
long strings of data or else.

> Come on... :-)

  I would return that TBH, come on people didn't provide something
completely automatable and human error proof to do this outside of
rpm. I'm willing to be educated if it's there, and add this to my
own process.

  I'm serious, I'm ready to add extra steps if I believe they are
automatable and human-error proof ! Show me the way :-)

Daniel

-- 
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

Re: [libvirt] [PATCH V3 0/3] Some improvements for video model

[Gerd Hoffmann]

On Do, 2014-08-14 at 20:43 +0800, Wang Rui wrote:
> From: Zeng Junliang 
> 
> http://www.redhat.com/archives/libvir-list/2014-July/msg00644.html
> 
> diff to v2:
> - hide vram attribute silently instead of reporting an error.
> - introduce three new capabilities for vga.vgamem_mb, vmvga.vgamem_mb and 
> qxl.vgamem_mb.
> - fix some error reported by building libvirt.

Series looks good to me from qemu developers point of view.
Can't review the libvirt internals though.

cheers,
  Gerd


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

Re: [libvirt] Entering freeze for libvirt-1.2.8

[Richard Weinberger]

Am 28.08.2014 09:14, schrieb Daniel Veillard:
> On Wed, Aug 27, 2014 at 08:45:29PM +0200, Richard Weinberger wrote:
>> On Wed, Aug 27, 2014 at 9:18 AM, Daniel Veillard  wrote:
>>>   So I tagged 1.2.8-rc1 in git and made tarball and signed rpms
>>
>> Can you please sign the tarball too?
> 
>   Well, the source rpm is signed, you can check it and it contains the
> tarball, so technically there is already a signed source out there.
> Signing a tarballl means putting out an additional file and keeping
> it forever, I could do that but hum 

So everyone how wants to build libvirt from source and cares about data
integrity has to unpack/verify the rpm?
Come on... :-)

Signing tarballs is nothing new nor rocket science.
In times where the NSA tries to f*ck everyone at least some basic
cryptographic arrangements should be applied.

I know other projects are sloppy regarding signed releases too, this does
not mean that libvirt should follow their bad example.

Thanks,
//richard

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

Re: [libvirt] Entering freeze for libvirt-1.2.8

[Daniel Veillard]

On Wed, Aug 27, 2014 at 08:45:29PM +0200, Richard Weinberger wrote:
> On Wed, Aug 27, 2014 at 9:18 AM, Daniel Veillard  wrote:
> >   So I tagged 1.2.8-rc1 in git and made tarball and signed rpms
> 
> Can you please sign the tarball too?

  Well, the source rpm is signed, you can check it and it contains the
tarball, so technically there is already a signed source out there.
Signing a tarballl means putting out an additional file and keeping
it forever, I could do that but hum 

Daniel

-- 
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