[libvirt] [PATCH v3 RESEND] vhost-user: add support reconnect for vhost-user ports

[ZhiPeng Lu]

For vhost-user ports, Open vSwitch acts as the server and QEMU the client.
When OVS crashes or restarts, the QEMU process should be reconnected to
OVS.

Signed-off-by: ZhiPeng Lu 
---
 docs/schemas/domaincommon.rng  | 26 --
 src/conf/domain_conf.c | 40 ++
 src/conf/domain_conf.h | 10 +++---
 src/qemu/qemu_command.c|  2 +-
 src/qemu/qemu_domain.c |  2 +-
 src/qemu/qemu_monitor_json.c   |  2 +-
 .../qemuxml2argv-net-vhostuser-multiq.args |  4 +--
 .../qemuxml2argv-net-vhostuser-multiq.xml  |  8 +++--
 8 files changed, 65 insertions(+), 29 deletions(-)

diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 76852ab..3f4ed82 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -2327,6 +2327,18 @@
   
 
   
+  
+
+  
+
+  
+  
+
+  
+
+  
+
+  
 
   

Re: [libvirt] gnulib tests in libvirt broken by newer glibc 2.26

[Bruno Haible]

Hi,

Christian Ehrhardt wrote:
> > Fedora 26 only has glibc 2.25 - you need to have Fedora rawhide to get
> > the broken behaviour, as that has glibc 2.26.90
> As Daniel said at least glibc 2.26 as in Fedora rawhide or Ubuntu Artful.

This tip is not helpful: I spent two hours trying Fedora Rawhide, and the
version of today is not functional: the live ISO shows a desktop in which
all you can do is to reboot, and the installation ISO does an installation
but the installed system does not boot.

So we are back to the support mode where we ask for pieces of information
for analysis by us (gnulib developers).

> ./configure gl_cv_func_getopt_posix=no ac_cv_header_getopt_h=no

This mode of configuration is not the usual one. (Why, if you are on a glibc
system, would you want to avoid the system's  header and getopt()
function?) But anyway, it should be supported, and the result should be
a 'test-getopt-posix' program that uses gnulib's replacement:

$ nm test-getopt-posix | grep getopt
0060a160 b getopt_data
00405b90 T _getopt_internal_r
00400f90 t getopt_loop.constprop.0
00406170 T rpl_getopt
00406110 T rpl_getopt_internal
00401130 t test_getopt

> [1]: http://paste.ubuntu.com/25638847/

Please provide info as mail attachments (compressed if necessary) in the
future. This site provides a "download as text" button which requests the
user's Launchpad credentials and then attempts to transmit them without
encryption (at least that's what I understand from the Firefox warning).

> Here [1] a log of your commands on such a system showing the issue.
> I added a gdb to show the assert

Thanks. The essential lines are:

checking for getopt.h... (cached) no
checking whether getopt is POSIX compatible... (cached) no
...
{ echo '/* DO NOT EDIT! GENERATED AUTOMATICALLY! */'; \
  sed -e 's|@''GUARD_PREFIX''@|GL|g' \
  -e 's|@''HAVE_GETOPT_H''@|0|g' \
  -e 's|@''INCLUDE_NEXT''@|include_next|g' \
  -e 's|@''PRAGMA_SYSTEM_HEADER''@|#pragma GCC system_header|g' \
  -e 's|@''PRAGMA_COLUMNS''@||g' \
  -e 's|@''NEXT_GETOPT_H''@||g' \
  -e '/definition of _GL_ARG_NONNULL/r ./arg-nonnull.h' \
  < ./getopt.in.h; \
} > getopt.h-t && \
...
gcc -DHAVE_CONFIG_H -I. -I..  -DGNULIB_STRICT_CHECKING=1   -g -O2 -MT getopt.o 
-MD -MP -MF .deps/getopt.Tpo -c -o getopt.o getopt.c
...
gcc -DHAVE_CONFIG_H -I. -I..  -DGNULIB_STRICT_CHECKING=1   -g -O2 -MT getopt1.o 
-MD -MP -MF .deps/getopt1.Tpo -c -o getopt1.o getopt1.c

So, the build attempts to use the gnulib override of getopt.

More essential lines:

Program received signal SIGABRT, Aborted.
#0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:51
#1  0x77a2df5d in __GI_abort () at abort.c:90
#2  0x8d23 in test_getopt () at test-getopt.h:754
#3  0x4b24 in main () at test-getopt-main.h:65

So, the test is executing test_getopt () with the variable POSIXLY_CORRECT
unset; but test_getopt () executes code that is conditional on 'posixly' being
true. Which means that __GETOPT_PREFIX must be defined (presumably to 'rpl_').

Can you show three things, please?

  1) The output of
 $ nm test-getopt-posix | grep getopt

  2) The output of
 $ gcc -DHAVE_CONFIG_H -DGNULIB_STRICT_CHECKING=1 -I. -I.. -I../gllib -g 
-O2 -E test-getopt-posix.c

  3) The output of
 $ gcc -DHAVE_CONFIG_H -DGNULIB_STRICT_CHECKING=1 -I. -I.. -I../gllib -g 
-O2 -E -dM test-getopt-posix.c

Thanks.

Bruno

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

Re: [libvirt] [Qemu-devel] libvirt/QEMU/SEV interaction

[Laszlo Ersek]

On 09/29/17 23:16, Michael S. Tsirkin wrote:
> On Fri, Sep 29, 2017 at 02:48:45PM -0500, Richard Relph wrote:
>> On 9/29/17 2:34 PM, Michael S. Tsirkin wrote:
>>> On Wed, Sep 27, 2017 at 02:06:10PM -0500, Richard Relph wrote:
 Whether the "BIOS" is a "static shim" as Michael suggests, or a full BIOS,
 or even a BIOS+kernel+initrd is really not too significant. What is
 significant is that the GO has a basis for trusting all code that is
 imported in to their VM by the CP. And that NONE of the code provided by 
 the
 CP is "unknown" and unauditable by the GO. If the CP has a way to inject
 code unknown to the GO in to the guest VM, the trust model is broken and
 both GO and CP suffer the consequences.
>>>
>>> Absolutely.
>>>
 When the CP needs to update the BIOS image, they will have to inform the GO
 and allow the GO to establish trust in the CP's new BIOS image somehow.
>>>
>>> This GO update on every BIOS change is imho is not a workable model. You
>>> want something like checking the BIOS signature instead. And since
>>> hardware is all hash based, you need the shim to do it in software.
>>
>> A BIOS "signed" by the CP doesn't meet the security requirement. It is code
>> that is "unknown" to the GO.
> 
> There is a misunderstanding here.
> 
> BIOS would not be signed by a CP. It would be signed by a trusted
> software vendor e.g. by Red Hat.
> 
>> The (legitimate) CP does NOT want to be in that position of trust. If they
>> are, then some government somewhere is going to insist that they sign a BIOS
>> that allows the government to spy on the GO's VMs, and steal secrets from
>> it. Or some hacker admin will do it "for fun".
>>
>> How often do large public CPs really change their BIOSes? My sense is that
>> large public CPs prefer stability over "latest and greatest".
> 
> CPs just do dnf update. Software vendors change BIOSes.
> 
> And we do change them. Look at number of revisions for seabios in e.g.
> Fedora. More importantly we might need to change them quickly e.g.
> because of a security issue. Adding the need to coordinate with all GOs
> is not going to work. Neither can QEMU support booting old BIOS
> versions on new machine types indefinitely.
> 
>> And, perhaps more importantly, if a CP are able to sell a "more secure" VM,
>> one that justifies a higher price per vCPU hour, wouldn't that warrant some
>> changes in the "insecure" model being used today?
>>
>> Richard
> 
> Absolutely. CPs have no business signing images. But it is just not
> really feasible for software vendors to distribute hashes.

Can this be helped by "reproducible builds"? Like,

- guest firmware vendor publishes the source package,
- GO asynchronously audits the source package (most likely
incrementally, reviewing the new, broken-out patches in the source package),
- GO builds the binary package,
- GO verifies that it bit-wise matches the guest fw vendor's binary package,
- GO adds the fw binary's hash to their own whitelist.

By virtue of releasing the source package of the guest firmware (and by
announcing it via another erratum), the guest fw vendor implicitly
notifies all GOs. It is then up to the individual GOs to evaluate the
changes on their own time, and to switch to the new fw binary.

It would even be OK if the GO built their own fw binary from the source
package, and submitted that to the CP, as part of the guest payload. The
guest firmware vendor would still be able to support this build, since
it would match the vendor's own binary 100%.

Thanks
Laszlo

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

Re: [libvirt] [Qemu-devel] libvirt/QEMU/SEV interaction

[Laszlo Ersek]

Side topic; sorry if it has been mentioned elsewhere:

On 09/27/17 21:06, Richard Relph wrote:

> Whether the "BIOS" is a "static shim" as Michael suggests, or a full
> BIOS, or even a BIOS+kernel+initrd is really not too significant. What
> is significant is that the GO has a basis for trusting all code that is
> imported in to their VM by the CP. And that NONE of the code provided by
> the CP is "unknown" and unauditable by the GO. If the CP has a way to
> inject code unknown to the GO in to the guest VM, the trust model is
> broken and both GO and CP suffer the consequences.

The expansion ROMs (containing UEFI drivers) of emulated PCI devices,
and the same of assigned physical PCI devices, constitute another
channel through which code enters the guest from the outside (i.e., from
the Cloud Provider). The ROM BARs from which the guest firmware reads
the UEFI binaries are not guest RAM, they are MMIO. (For execution, the
drivers are copied into encrypted guest RAM.)

If the guest has Secure Boot enabled, then the oproms are verified[*]
(and not launched if verification fails), but this is slightly different
from what I understand under audit-by-GO. It means the GO wouldn't get a
measurement of the oproms for one-by-one clearing, when about to
green-light a guest startup. Instead the GO would ensure that Secure
Boot be enabled with the right certificates (and/or executable hashes)
enrolled off the bat, and then implicitly trust all oprom drivers
accepted by those certs / hashes. It's another layer of indirection.

This is likely nothing new qualitatively, but "the devil is in the
details", so I thought it was worth raising.

[*] For edk2 / OvmfPkg specifics, I'll mention

  gEfiSecurityPkgTokenSpaceGuid.PcdOptionRomImageVerificationPolicy

The SecurityPkg default is 0x04 ("Deny execution when there is security
violation"). However, OVMF sets it to 0x00 ("Always trust the image").
Please see the following commit for the reasons:

  https://github.com/tianocore/edk2/commit/1fea9ddb4e3fd

Brijesh, for SEV guests, we likely want to flip this PCD to 0x04, in the
AmdSevInitialize() function, in "OvmfPkg/PlatformPei/AmdSev.c". For that
we'll also have to change the PCD from fixed-at-build to dynamic, but
that in turn will require a change to "SecurityPkg.dec" itself
(currently it only allows fixed-at-build or patchable, not dynamic). Do
you want me to file a BZ in the TianoCore tracker for this, and assign
it to you? If you don't have time for writing the patch, I'm glad to do
it too, but then the review could be slower; both other OvmfPkg
co-maintainers are busy with other things.)

Thanks!
Laszlo

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

Re: [libvirt] [Qemu-devel] libvirt/QEMU/SEV interaction

[Michael S. Tsirkin]

On Fri, Sep 29, 2017 at 02:48:45PM -0500, Richard Relph wrote:
> On 9/29/17 2:34 PM, Michael S. Tsirkin wrote:
> > On Wed, Sep 27, 2017 at 02:06:10PM -0500, Richard Relph wrote:
> > > Whether the "BIOS" is a "static shim" as Michael suggests, or a full BIOS,
> > > or even a BIOS+kernel+initrd is really not too significant. What is
> > > significant is that the GO has a basis for trusting all code that is
> > > imported in to their VM by the CP. And that NONE of the code provided by 
> > > the
> > > CP is "unknown" and unauditable by the GO. If the CP has a way to inject
> > > code unknown to the GO in to the guest VM, the trust model is broken and
> > > both GO and CP suffer the consequences.
> > 
> > Absolutely.
> > 
> > > When the CP needs to update the BIOS image, they will have to inform the 
> > > GO
> > > and allow the GO to establish trust in the CP's new BIOS image somehow.
> > 
> > This GO update on every BIOS change is imho is not a workable model. You
> > want something like checking the BIOS signature instead. And since
> > hardware is all hash based, you need the shim to do it in software.
> 
> A BIOS "signed" by the CP doesn't meet the security requirement. It is code
> that is "unknown" to the GO.

There is a misunderstanding here.

BIOS would not be signed by a CP. It would be signed by a trusted
software vendor e.g. by Red Hat.

> The (legitimate) CP does NOT want to be in that position of trust. If they
> are, then some government somewhere is going to insist that they sign a BIOS
> that allows the government to spy on the GO's VMs, and steal secrets from
> it. Or some hacker admin will do it "for fun".
> 
> How often do large public CPs really change their BIOSes? My sense is that
> large public CPs prefer stability over "latest and greatest".

CPs just do dnf update. Software vendors change BIOSes.

And we do change them. Look at number of revisions for seabios in e.g.
Fedora. More importantly we might need to change them quickly e.g.
because of a security issue. Adding the need to coordinate with all GOs
is not going to work. Neither can QEMU support booting old BIOS
versions on new machine types indefinitely.

> And, perhaps more importantly, if a CP are able to sell a "more secure" VM,
> one that justifies a higher price per vCPU hour, wouldn't that warrant some
> changes in the "insecure" model being used today?
> 
> Richard

Absolutely. CPs have no business signing images. But it is just not
really feasible for software vendors to distribute hashes.

-- 
MST

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

Re: [libvirt] [Qemu-devel] libvirt/QEMU/SEV interaction

[Michael S. Tsirkin]

On Fri, Sep 29, 2017 at 03:07:40PM -0500, Richard Relph wrote:
> On 9/29/17 2:48 PM, Richard Relph wrote:
> > On 9/29/17 2:34 PM, Michael S. Tsirkin wrote:
> > > On Wed, Sep 27, 2017 at 02:06:10PM -0500, Richard Relph wrote:
> > > > Whether the "BIOS" is a "static shim" as Michael suggests, or a
> > > > full BIOS,
> > > > or even a BIOS+kernel+initrd is really not too significant. What is
> > > > significant is that the GO has a basis for trusting all code that is
> > > > imported in to their VM by the CP. And that NONE of the code
> > > > provided by the
> > > > CP is "unknown" and unauditable by the GO. If the CP has a way to inject
> > > > code unknown to the GO in to the guest VM, the trust model is broken and
> > > > both GO and CP suffer the consequences.
> > > 
> > > Absolutely.
> > > 
> > > > When the CP needs to update the BIOS image, they will have to
> > > > inform the GO
> > > > and allow the GO to establish trust in the CP's new BIOS image somehow.
> > > 
> > > This GO update on every BIOS change is imho is not a workable model. You
> > > want something like checking the BIOS signature instead. And since
> > > hardware is all hash based, you need the shim to do it in software.
> > 
> > A BIOS "signed" by the CP doesn't meet the security requirement. It is
> > code that is "unknown" to the GO.
> > 
> > The (legitimate) CP does NOT want to be in that position of trust. If
> > they are, then some government somewhere is going to insist that they
> > sign a BIOS that allows the government to spy on the GO's VMs, and steal
> > secrets from it. Or some hacker admin will do it "for fun".
> > 
> > How often do large public CPs really change their BIOSes? My sense is
> > that large public CPs prefer stability over "latest and greatest".
> > 
> > And, perhaps more importantly, if a CP are able to sell a "more secure"
> > VM, one that justifies a higher price per vCPU hour, wouldn't that
> > warrant some changes in the "insecure" model being used today?
> 
> Ultimately, I think both approaches are "doable". It will be a CP and GO
> decision. If the GO trusts the CP, the shim+signed BIOS will work fine.

I think there's a misunderstanding. A trusted software vendor would
sign the BIOS. GO would verify it. Trusting the CP is not required.

> If
> GO requires a more secure VM and the CP wants to offer it, the CP will
> figure out a way to satisfy the GO's "trust issue" that the BIOS can't be
> used to circumvent SEV's protections. Depending on your level of paranoia,
> that may require advance notice of BIOS changes, or even allowing the GO to
> provide the BIOS themselves, written to a spec supported by the CP's HV,
> and/or based on BIOS code provided by the CP.

We are discussing this on a qemu mailing list, aren't we? And from QEMU
point of view,  I think it won't be able to support a requirement
to boot ancient bios versions on new machine types indefinitely
with good performance and also fix security issues in them
in a timely manner somehow.


> 
> It's a business decision and I think SEV can support both. That said, AMD
> currently has no plans to write a shim that can verify the signature on a
> CP-provided BIOS image.
> 
> Richard

Someone else will have to work on a supportable solution for QEMU then.

> > 
> > Richard

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

[libvirt] [PATCH v2 0/2] Clean up the nwfilter mess I created

[John Ferlan]

v1: https://www.redhat.com/archives/libvir-list/2017-September/msg01072.html

Changes:

 * Patch1: No change, ACK'd, but not safe to push yet either..

 * Patch2: Rather than have virNWFilterIPAddrMapAddIPAddr consume
   the input @addr, let's make a copy of the input parameter
   and manage it within that code so that it wouldn't be
   consumed on virNWFilterHashTablePut failure after
   virNWFilterVarValueCreateSimple success.

John Ferlan (2):
  Revert "nwfilter: Fix possible segfault on sometimes consumed
variable"
  nwfilter: Don't have virNWFilterIPAddrMapAddIPAddr consume input

 src/conf/nwfilter_ipaddrmap.c | 16 +---
 src/nwfilter/nwfilter_dhcpsnoop.c |  3 ---
 2 files changed, 9 insertions(+), 10 deletions(-)

-- 
2.13.5

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

Re: [libvirt] [Qemu-devel] libvirt/QEMU/SEV interaction

[Richard Relph]

On 9/29/17 2:34 PM, Michael S. Tsirkin wrote:

On Wed, Sep 27, 2017 at 02:06:10PM -0500, Richard Relph wrote:

Whether the "BIOS" is a "static shim" as Michael suggests, or a full BIOS,
or even a BIOS+kernel+initrd is really not too significant. What is
significant is that the GO has a basis for trusting all code that is
imported in to their VM by the CP. And that NONE of the code provided by the
CP is "unknown" and unauditable by the GO. If the CP has a way to inject
code unknown to the GO in to the guest VM, the trust model is broken and
both GO and CP suffer the consequences.


Absolutely.


When the CP needs to update the BIOS image, they will have to inform the GO
and allow the GO to establish trust in the CP's new BIOS image somehow.


This GO update on every BIOS change is imho is not a workable model. You
want something like checking the BIOS signature instead. And since
hardware is all hash based, you need the shim to do it in software.


A BIOS "signed" by the CP doesn't meet the security requirement. It is 
code that is "unknown" to the GO.


The (legitimate) CP does NOT want to be in that position of trust. If 
they are, then some government somewhere is going to insist that they 
sign a BIOS that allows the government to spy on the GO's VMs, and steal 
secrets from it. Or some hacker admin will do it "for fun".


How often do large public CPs really change their BIOSes? My sense is 
that large public CPs prefer stability over "latest and greatest".


And, perhaps more importantly, if a CP are able to sell a "more secure" 
VM, one that justifies a higher price per vCPU hour, wouldn't that 
warrant some changes in the "insecure" model being used today?


Richard

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

[libvirt] [PATCH v2 1/2] Revert "nwfilter: Fix possible segfault on sometimes consumed variable"

[John Ferlan]

This reverts commit 6209bb32e5b6d8c15d55422bb4716b3b31c1c7b2.

This turns out to be the wrong adjustment

Signed-off-by: John Ferlan 
---
 src/conf/nwfilter_ipaddrmap.c | 9 +
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/src/conf/nwfilter_ipaddrmap.c b/src/conf/nwfilter_ipaddrmap.c
index 5668f366d..9c8584ce2 100644
--- a/src/conf/nwfilter_ipaddrmap.c
+++ b/src/conf/nwfilter_ipaddrmap.c
@@ -26,9 +26,7 @@
 
 #include "internal.h"
 
-#include "viralloc.h"
 #include "virerror.h"
-#include "virstring.h"
 #include "datatypes.h"
 #include "nwfilter_params.h"
 #include "nwfilter_ipaddrmap.h"
@@ -54,7 +52,6 @@ virNWFilterIPAddrMapAddIPAddr(const char *ifname, char *addr)
 {
 int ret = -1;
 virNWFilterVarValuePtr val;
-char *tmp = NULL;
 
 virMutexLock();
 
@@ -68,18 +65,14 @@ virNWFilterIPAddrMapAddIPAddr(const char *ifname, char 
*addr)
 virNWFilterVarValueFree(val);
 goto cleanup;
 } else {
-if (VIR_STRDUP(tmp, addr) < 0)
+if (virNWFilterVarValueAddValue(val, addr) < 0)
 goto cleanup;
-if (virNWFilterVarValueAddValue(val, tmp) < 0)
-goto cleanup;
-tmp = NULL;
 }
 
 ret = 0;
 
  cleanup:
 virMutexUnlock();
-VIR_FREE(tmp);
 
 return ret;
 }
-- 
2.13.5

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

Re: [libvirt] [Qemu-devel] libvirt/QEMU/SEV interaction

[Richard Relph]

On 9/29/17 2:48 PM, Richard Relph wrote:

On 9/29/17 2:34 PM, Michael S. Tsirkin wrote:

On Wed, Sep 27, 2017 at 02:06:10PM -0500, Richard Relph wrote:
Whether the "BIOS" is a "static shim" as Michael suggests, or a full 
BIOS,

or even a BIOS+kernel+initrd is really not too significant. What is
significant is that the GO has a basis for trusting all code that is
imported in to their VM by the CP. And that NONE of the code provided 
by the

CP is "unknown" and unauditable by the GO. If the CP has a way to inject
code unknown to the GO in to the guest VM, the trust model is broken and
both GO and CP suffer the consequences.


Absolutely.

When the CP needs to update the BIOS image, they will have to inform 
the GO

and allow the GO to establish trust in the CP's new BIOS image somehow.


This GO update on every BIOS change is imho is not a workable model. You
want something like checking the BIOS signature instead. And since
hardware is all hash based, you need the shim to do it in software.


A BIOS "signed" by the CP doesn't meet the security requirement. It is 
code that is "unknown" to the GO.


The (legitimate) CP does NOT want to be in that position of trust. If 
they are, then some government somewhere is going to insist that they 
sign a BIOS that allows the government to spy on the GO's VMs, and steal 
secrets from it. Or some hacker admin will do it "for fun".


How often do large public CPs really change their BIOSes? My sense is 
that large public CPs prefer stability over "latest and greatest".


And, perhaps more importantly, if a CP are able to sell a "more secure" 
VM, one that justifies a higher price per vCPU hour, wouldn't that 
warrant some changes in the "insecure" model being used today?


Ultimately, I think both approaches are "doable". It will be a CP and GO 
decision. If the GO trusts the CP, the shim+signed BIOS will work fine. 
If GO requires a more secure VM and the CP wants to offer it, the CP 
will figure out a way to satisfy the GO's "trust issue" that the BIOS 
can't be used to circumvent SEV's protections. Depending on your level 
of paranoia, that may require advance notice of BIOS changes, or even 
allowing the GO to provide the BIOS themselves, written to a spec 
supported by the CP's HV, and/or based on BIOS code provided by the CP.


It's a business decision and I think SEV can support both. That said, 
AMD currently has no plans to write a shim that can verify the signature 
on a CP-provided BIOS image.


Richard



Richard


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

[libvirt] [PATCH v2 2/2] nwfilter: Don't have virNWFilterIPAddrMapAddIPAddr consume input

[John Ferlan]

On pure success paths, virNWFilterIPAddrMapAddIPAddr was validly
consuming the input @addr; however, on failure paths it was possible
that virNWFilterVarValueCreateSimple succeed, but virNWFilterHashTablePut
failed resulting in virNWFilterVarValueFree being called to clean
up @val which also cleaned up the input @addr. Thus the caller had
no way to determine on failure whether it too should clean up the
passed parameter.

Instead, let's create a copy of the input @addr, then handle that
properly in the API allowing/forcing the caller to free it's own
copy of the input parameter.

Signed-off-by: John Ferlan 
---
 src/conf/nwfilter_ipaddrmap.c | 13 +++--
 src/nwfilter/nwfilter_dhcpsnoop.c |  3 ---
 2 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/src/conf/nwfilter_ipaddrmap.c b/src/conf/nwfilter_ipaddrmap.c
index 9c8584ce2..54e6d0f0f 100644
--- a/src/conf/nwfilter_ipaddrmap.c
+++ b/src/conf/nwfilter_ipaddrmap.c
@@ -26,7 +26,9 @@
 
 #include "internal.h"
 
+#include "viralloc.h"
 #include "virerror.h"
+#include "virstring.h"
 #include "datatypes.h"
 #include "nwfilter_params.h"
 #include "nwfilter_ipaddrmap.h"
@@ -51,28 +53,35 @@ int
 virNWFilterIPAddrMapAddIPAddr(const char *ifname, char *addr)
 {
 int ret = -1;
+char *addrCopy;
 virNWFilterVarValuePtr val;
 
+if (VIR_STRDUP(addrCopy, addr) < 0)
+return -1;
+
 virMutexLock();
 
 val = virHashLookup(ipAddressMap->hashTable, ifname);
 if (!val) {
-val = virNWFilterVarValueCreateSimple(addr);
+val = virNWFilterVarValueCreateSimple(addrCopy);
 if (!val)
 goto cleanup;
+addrCopy = NULL;
 ret = virNWFilterHashTablePut(ipAddressMap, ifname, val);
 if (ret < 0)
 virNWFilterVarValueFree(val);
 goto cleanup;
 } else {
-if (virNWFilterVarValueAddValue(val, addr) < 0)
+if (virNWFilterVarValueAddValue(val, addrCopy) < 0)
 goto cleanup;
+addrCopy = NULL;
 }
 
 ret = 0;
 
  cleanup:
 virMutexUnlock();
+VIR_FREE(addrCopy);
 
 return ret;
 }
diff --git a/src/nwfilter/nwfilter_dhcpsnoop.c 
b/src/nwfilter/nwfilter_dhcpsnoop.c
index 4436e396f..743136277 100644
--- a/src/nwfilter/nwfilter_dhcpsnoop.c
+++ b/src/nwfilter/nwfilter_dhcpsnoop.c
@@ -476,9 +476,6 @@ 
virNWFilterSnoopIPLeaseInstallRule(virNWFilterSnoopIPLeasePtr ipl,
 if (virNWFilterIPAddrMapAddIPAddr(req->ifname, ipaddr) < 0)
 goto exit_snooprequnlock;
 
-/* ipaddr now belongs to the map */
-ipaddr = NULL;
-
 if (!instantiate) {
 rc = 0;
 goto exit_snooprequnlock;
-- 
2.13.5

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

Re: [libvirt] gnulib tests in libvirt broken by newer glibc 2.26

[Paul Eggert]

On 09/29/2017 05:02 AM, Christian Ehrhardt wrote:

Here [1] a log of your commands on such a system showing the issue.


Thanks, but I still don't understand what the bug is. With those 
commands, the test programs use Gnulib-supplied getopt, not the glibc 
getopt. So why would any change in glibc 2.26 getopt affect things?


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

Re: [libvirt] [Qemu-devel] libvirt/QEMU/SEV interaction

[Michael S. Tsirkin]

On Wed, Sep 27, 2017 at 02:06:10PM -0500, Richard Relph wrote:
> Whether the "BIOS" is a "static shim" as Michael suggests, or a full BIOS,
> or even a BIOS+kernel+initrd is really not too significant. What is
> significant is that the GO has a basis for trusting all code that is
> imported in to their VM by the CP. And that NONE of the code provided by the
> CP is "unknown" and unauditable by the GO. If the CP has a way to inject
> code unknown to the GO in to the guest VM, the trust model is broken and
> both GO and CP suffer the consequences.

Absolutely.

> When the CP needs to update the BIOS image, they will have to inform the GO
> and allow the GO to establish trust in the CP's new BIOS image somehow.

This GO update on every BIOS change is imho is not a workable model. You
want something like checking the BIOS signature instead. And since
hardware is all hash based, you need the shim to do it in software.

-- 
MST

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

Re: [libvirt] [PATCH 2/2] nwfilter: Only free inetaddr on virNWFilterIPAddrMapAddIPAddr failure

[John Ferlan]

[...]

> 
> This assumes that virNWFilterIPAddrMapAddIPAddr consumes inetaddr only
> if it returns zero. However in a fraction of the unlikely cases when
> this function call can fail (all of them on OOM), it can consume it even
> though it returns -1 -- if virNWFilterVarValueCreateSimple succeeds,
> but virNWFilterHashTablePut fails.
> 

If virNWFilterHashTablePut fails in virNWFilterIPAddrMapAddIPAddr, then
it calls virNWFilterVarValueFree which will VIR_FREE the value that was
stored in @val which is returned from virNWFilterVarValueCreateSimple.

I still hate the nwfilter code.

John

>>     }
>>
>>     ret = virNWFilterInstantiateFilterLate(req->driver,
>> @@ -637,7 +639,8 @@ learnIPAddressThread(void *arg)
>>    req->filterparams);
>>     VIR_DEBUG("Result from applying firewall rules on "
>>   "%s with IP addr %s : %d", req->ifname,
>> inetaddr, ret);

[...]

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

Re: [libvirt] [PATCH 2/2] nwfilter: Only free inetaddr on virNWFilterIPAddrMapAddIPAddr failure

[John Ferlan]

On 09/29/2017 11:44 AM, Ján Tomko wrote:
> On Thu, Sep 28, 2017 at 03:26:49PM -0400, John Ferlan wrote:
>> Flag when virNWFilterIPAddrMapAddIPAddr to allow deletion - keep
>> @inetaddr around to message after virNWFilterInstantiateFilterLate
>>
>> Signed-off-by: John Ferlan 
>> ---
>> src/nwfilter/nwfilter_learnipaddr.c | 5 -
>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/nwfilter/nwfilter_learnipaddr.c
>> b/src/nwfilter/nwfilter_learnipaddr.c
>> index 5b95f0e61..567897221 100644
>> --- a/src/nwfilter/nwfilter_learnipaddr.c
>> +++ b/src/nwfilter/nwfilter_learnipaddr.c
>> @@ -610,6 +610,7 @@ learnIPAddressThread(void *arg)
>>     sa.data.inet4.sin_family = AF_INET;
>>     sa.data.inet4.sin_addr.s_addr = vmaddr;
>>     char *inetaddr;
>> +    bool failmap = false;
> 
> The name looks confusing. I'd prefer the name to document either what
> happened
> (adding_failed, address_consumed) or what should happen (free_addr). Or
> rather dropping the bool completely, log the IP address before the
> AddIpAddr call and remove it from the other VIR_DEBUG message.

to quote Michal from a recent series "Naming is hard"

I like the idea of logging the IP before...  That resolves some of the
oddities.

>>
>>     /* It is necessary to unlock interface here to avoid
>> updateMutex and
>>  * interface ordering deadlocks. Otherwise we are going to
>> @@ -625,6 +626,7 @@ learnIPAddressThread(void *arg)
>>     if (virNWFilterIPAddrMapAddIPAddr(req->ifname, inetaddr) <
>> 0) {
>>     VIR_ERROR(_("Failed to add IP address %s to IP address "
>>   "cache for interface %s"), inetaddr,
>> req->ifname);
>> +    failmap = true;
> 
> This assumes that virNWFilterIPAddrMapAddIPAddr consumes inetaddr only
> if it returns zero. However in a fraction of the unlikely cases when
> this function call can fail (all of them on OOM), it can consume it even
> though it returns -1 -- if virNWFilterVarValueCreateSimple succeeds,
> but virNWFilterHashTablePut fails.
> 

Ugh...  I hate this code.

>>     }
>>
>>     ret = virNWFilterInstantiateFilterLate(req->driver,
>> @@ -637,7 +639,8 @@ learnIPAddressThread(void *arg)
>>    req->filterparams);
>>     VIR_DEBUG("Result from applying firewall rules on "
>>   "%s with IP addr %s : %d", req->ifname,
>> inetaddr, ret);
> 
> At this point, inetaddr belongs to the hash table protected by the lock
> that AddIPAddr already released. Can we safely access it here?

We have been - it's a VIR_DEBUG though...

I'll work on a followup.

John

> 
> Jan
> 
>> -    VIR_FREE(inetaddr);
>> +    if (failmap)
>> +    VIR_FREE(inetaddr);
>>     }
>>     } else {
>>     if (showError)
>> -- 
>> 2.13.5
>>
>> -- 
>> libvir-list mailing list
>> libvir-list@redhat.com
>> https://www.redhat.com/mailman/listinfo/libvir-list

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

Re: [libvirt] [PATCH 1/2] Revert "nwfilter: Fix possible segfault on sometimes consumed variable"

[Ján Tomko]

On Thu, Sep 28, 2017 at 03:26:48PM -0400, John Ferlan wrote:

This reverts commit 6209bb32e5b6d8c15d55422bb4716b3b31c1c7b2.

This turns out to be the wrong adjustment

Signed-off-by: John Ferlan 
---
src/conf/nwfilter_ipaddrmap.c | 9 +
1 file changed, 1 insertion(+), 8 deletions(-)



ACK

Jan


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

Re: [libvirt] [PATCH 2/2] nwfilter: Only free inetaddr on virNWFilterIPAddrMapAddIPAddr failure

[Ján Tomko]

On Thu, Sep 28, 2017 at 03:26:49PM -0400, John Ferlan wrote:

Flag when virNWFilterIPAddrMapAddIPAddr to allow deletion - keep
@inetaddr around to message after virNWFilterInstantiateFilterLate

Signed-off-by: John Ferlan 
---
src/nwfilter/nwfilter_learnipaddr.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/src/nwfilter/nwfilter_learnipaddr.c 
b/src/nwfilter/nwfilter_learnipaddr.c
index 5b95f0e61..567897221 100644
--- a/src/nwfilter/nwfilter_learnipaddr.c
+++ b/src/nwfilter/nwfilter_learnipaddr.c
@@ -610,6 +610,7 @@ learnIPAddressThread(void *arg)
sa.data.inet4.sin_family = AF_INET;
sa.data.inet4.sin_addr.s_addr = vmaddr;
char *inetaddr;
+bool failmap = false;


The name looks confusing. I'd prefer the name to document either what happened
(adding_failed, address_consumed) or what should happen (free_addr). Or
rather dropping the bool completely, log the IP address before the
AddIpAddr call and remove it from the other VIR_DEBUG message.


/* It is necessary to unlock interface here to avoid updateMutex and
 * interface ordering deadlocks. Otherwise we are going to
@@ -625,6 +626,7 @@ learnIPAddressThread(void *arg)
if (virNWFilterIPAddrMapAddIPAddr(req->ifname, inetaddr) < 0) {
VIR_ERROR(_("Failed to add IP address %s to IP address "
  "cache for interface %s"), inetaddr, req->ifname);
+failmap = true;


This assumes that virNWFilterIPAddrMapAddIPAddr consumes inetaddr only
if it returns zero. However in a fraction of the unlikely cases when
this function call can fail (all of them on OOM), it can consume it even
though it returns -1 -- if virNWFilterVarValueCreateSimple succeeds,
but virNWFilterHashTablePut fails.


}

ret = virNWFilterInstantiateFilterLate(req->driver,
@@ -637,7 +639,8 @@ learnIPAddressThread(void *arg)
   req->filterparams);
VIR_DEBUG("Result from applying firewall rules on "
  "%s with IP addr %s : %d", req->ifname, inetaddr, ret);


At this point, inetaddr belongs to the hash table protected by the lock
that AddIPAddr already released. Can we safely access it here?

Jan


-VIR_FREE(inetaddr);
+if (failmap)
+VIR_FREE(inetaddr);
}
} else {
if (showError)
--
2.13.5

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


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

Re: [libvirt] [PATCH] conf: Split out parsing of network disk source XML elements

[Ján Tomko]

On Fri, Sep 29, 2017 at 09:12:34AM -0400, John Ferlan wrote:



On 09/29/2017 05:02 AM, Peter Krempa wrote:

virDomainDiskSourceParse got to the point of being an ugly spaghetti
mess by adding more and more stuff into it. Split out parsing of network
disk information into a separate function so that it stays contained.
---
I've had this patch on a branch that makes virDomainDiskSourceParse
uglier, but with the recent VxHS patches I've got a merge conflict, thus
I'm sending it now.

Note: this patch was generated with the "patience" diff algorithm


 src/conf/domain_conf.c | 183 ++---
 1 file changed, 99 insertions(+), 84 deletions(-)



Reviewed-by: John Ferlan 

Seems "safe" to me too.



Please don't push refactors or even pure code movement during the freeze.

Jan


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

Re: [libvirt] [PATCH 1/2] qemucapstest: Update test data for 'num-queues' property of virtio-blk

[Ján Tomko]

On Fri, Sep 29, 2017 at 09:54:22PM +0800, Lin Ma wrote:

Signed-off-by: Lin Ma 
---
tests/qemucapabilitiesdata/caps_2.10.0.s390x.xml  | 1 +
tests/qemucapabilitiesdata/caps_2.10.0.x86_64.xml | 1 +
tests/qemucapabilitiesdata/caps_2.7.0.s390x.xml   | 1 +
tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml  | 1 +
tests/qemucapabilitiesdata/caps_2.8.0.s390x.xml   | 1 +
tests/qemucapabilitiesdata/caps_2.8.0.x86_64.xml  | 1 +
tests/qemucapabilitiesdata/caps_2.9.0.ppc64le.xml | 1 +
tests/qemucapabilitiesdata/caps_2.9.0.s390x.xml   | 1 +
tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml  | 1 +
9 files changed, 9 insertions(+)



This commit on its own fails to pass make check.
The qemu_capabilities.{ch} changes should be a part of this commit.

Jan


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

Re: [libvirt] [PATCH 1/4] virt-aa-helper: fix paths for usb hostdevs

[Michal Privoznik]

On 09/20/2017 04:59 PM, Christian Ehrhardt wrote:
> If users only specified vendor (the common case) then parsing
> the xml via virDomainHostdevSubsysUSBDefParseXML would only set these.
> Bus and Device would much later be added when the devices are prepared
> to be added.
> 
> Due to that a hot-add of a usb hostdev works as the device is prepared
> and virt-aa-helper processes the new internal xml. But on an initial
> guest start at the time virt-aa-helper renders the apparmor rules the
> bus/device id's are not set yet:
> 
> p ctl->def->hostdevs[0]->source.subsys.u.usb
> $12 = {autoAddress = false, bus = 0, device = 0, vendor = 1921, product
> = 21888}
> 
> That causes rules to be wrong:
>   "/dev/bus/usb/000/000" rw,
> 
> The fix calls virHostdevFindUSBDevice after reading the XML from
> irt-aa-helper to only add apparmor rules for devices that could be found
> and now are fully known to be able to write the rule correctly.
> 
> It uncondtionally sets virHostdevFindUSBDevice mandatory attribute as
> adding an apparmor rule for a device not found makes no sense no matter
> what startup policy it has set.
> 
> Signed-off-by: Christian Ehrhardt 
> ---
>  src/security/virt-aa-helper.c | 4 
>  1 file changed, 4 insertions(+)
> 
> diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
> index 7944dc1..d1518ea 100644
> --- a/src/security/virt-aa-helper.c
> +++ b/src/security/virt-aa-helper.c
> @@ -55,6 +55,7 @@
>  #include "virrandom.h"
>  #include "virstring.h"
>  #include "virgettext.h"
> +#include "virhostdev.h"
>  
>  #include "storage/storage_source.h"
>  
> @@ -1069,6 +1070,9 @@ get_files(vahControl * ctl)
>  if (usb == NULL)
>  continue;
>  
> +if (virHostdevFindUSBDevice(dev, true, ) < 0)
> +continue;
> +

Shouldn't we rather fail in this case? Or, what happens if startupPolicy
of the device is set to 'optional'? I think we need to error out here
(although, we've probably errored out earlier in the process).

ACK to the rest of the patches (after some typo clean up, esp. in the
commit messages).

Michal

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

Re: [libvirt] [PATCH 2/2] qemu: Support multiqueue virtio-blk

[Ján Tomko]

On Fri, Sep 29, 2017 at 09:54:23PM +0800, Lin Ma wrote:

@@ -3053,6 +3053,10 @@
bus and "pci" or "ccw" address types.
Since 1.2.8 (QEMU 2.1)
  
+  
+The optional queues attribute specifies the number of
+virt queues for virtio-blk. (Since 
3.8.0)


3.9.0, now that 3.8.0 is frozen


+  
  
  For virtio disks,
  Virtio-specific options can also be



diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 87192eb2d..90572d51a 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -8761,6 +8761,15 @@ virDomainDiskDefDriverParseXML(virDomainDiskDefPtr def,
}
VIR_FREE(tmp);

+if ((tmp = virXMLPropString(cur, "queues")) &&
+virStrToLong_ui(tmp, NULL, 10, >queues) < 0) {


virStrToLong_ui allows specifying a negative number and wraps it into a
positive number (e.g. -1 is a shortcut for UINT_MAX)
Please use virStrToLong_uip instead.


+virReportError(VIR_ERR_XML_ERROR,
+   _("'queues' attribute must be positive number: %s"),
+   tmp);
+goto cleanup;
+}
+VIR_FREE(tmp);
+
ret = 0;

 cleanup:
@@ -21996,6 +22005,16 @@ virDomainDiskDefFormat(virBufferPtr buf,
virBufferAsprintf(, " iothread='%u'", def->iothread);
if (def->detect_zeroes)
virBufferAsprintf(, " detect_zeroes='%s'", detect_zeroes);
+if (def->queues) {
+if (def->bus == VIR_DOMAIN_DISK_BUS_VIRTIO)
+virBufferAsprintf(, " queues='%u'", def->queues);
+else {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+   _("queues attribute in disk driver element is only "
+ "supported by virtio-blk"));
+return -1;


This check does not belong in the formatter. If we parsed it, we should
be able to format it back.

Either only parse the attribute if the bus is DISK_BUS_VIRTIO, or add the check
to qemuDomain*DefValidate.


+}
+}

virDomainVirtioOptionsFormat(, def->virtio);




diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 4f141e0ac..4d2787d8f 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -2082,6 +2082,10 @@ qemuBuildDriveDevStr(const virDomainDef *def,
  (disk->device == VIR_DOMAIN_DISK_DEVICE_LUN)
  ? "on" : "off");
}
+if (disk->queues &&
+virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_BLK_NUM_QUEUES)) {
+virBufferAsprintf(, ",num-queues=%u", disk->queues);


If QEMU does not have the capability, it would be nice to report an
error to the user instead of quietly doing nothing with the attribute.
(The check also probably belongs in qemuDomain*DefValidate)


+}

if (qemuBuildVirtioOptionsStr(, disk->virtio, qemuCaps) < 0)
goto error;


Jan


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

[libvirt] [PATCH] docs: Add some changes to news.xml for this release

[Martin Kletzander]

Signed-off-by: Martin Kletzander 
---
 docs/news.xml | 81 +++
 1 file changed, 81 insertions(+)

diff --git a/docs/news.xml b/docs/news.xml
index aab812b259cf..c9e7e4b9d1e4 100644
--- a/docs/news.xml
+++ b/docs/news.xml
@@ -35,6 +35,26 @@
 
   
 
+  
+
+  qemu: Added support for cold-(un)plug of watchdog devices
+
+  
+  
+
+  qemu: Added support for setting IP address os usernet interfaces
+
+  
+  
+
+  qemu: Added support for Veritas Hyperscale (VxHS) block devices
+
+  
+  
+
+  storage: Added new events for pool-build and pool-delete
+
+  
 
 
   
@@ -59,8 +79,69 @@
 kernel-forward-plane-offload).
 
   
+  
+
+  New CPU models for AMD and Intel
+
+
+  AMD EPYC and Intel Skylake-Server CPU models were added together with
+  their features
+
+  
+  
+
+  Improve long waiting when saving a domain
+
+
+  While waiting for a write to disk to be finished, e.g. during save,
+  even simple operations like virsh list would be blocking
+  due to domain lock.  This is now resolved by unlocking the domain
+  in places where it is not needed.
+
+  
 
 
+  
+
+  Proper units are now used in virsh manpage for dom(mem)stats
+
+
+  Previously the documentation used multiples of 1000, but now it is
+  fixe to use multiples of 1024.
+
+  
+  
+
+  qemu: Fix error reporting when disk attachment fails
+
+
+  There was a possibility for the actual error to be overridden or
+  cleared during the rollback.
+
+  
+  
+
+  qemu: Fix assignment of graphics ports after daemon restart
+
+
+  This could be seen with newer kernels that have bug regarding
+  SO_REUSEADDR.  After libvirtd was restarted it could assign already
+  used address to new guests which would make them fail to start.  This
+  is fixed by marking used ports unavailable when reconnecting to
+  running QEMU domains.
+
+  
+  
+
+  Fix message decoding which was caused very strange bug
+
+
+  When parsing an RPC message with file descriptors was interrupted and
+  had to restart, the offset of the payload was calculated badly 
causing
+  strange issues like not being able to find a domain that was not
+  requested.
+
+  
 
   
   
-- 
2.14.2

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

Re: [libvirt] [PATCH] Revert "vhost-user: add support reconnect for vhost-user ports"

[Michal Privoznik]

On 09/20/2017 04:01 PM, Pavel Hrdina wrote:
> This reverts commit edaf4ebe95a5995585c8ab7bc5b92887286d4431.
> 
> This uses "reconnect" as attribute for  element, but we already
> have a  element for  element for chardev devices.
> 
> Since this is the same feature for different device it should be
> presented in XML the same way.
> 
> Signed-off-by: Pavel Hrdina 
> ---
>  docs/formatdomain.html.in  |  5 +---
>  docs/schemas/domaincommon.rng  |  5 
>  src/conf/domain_conf.c | 28 
> ++
>  .../qemuxml2argv-net-vhostuser-multiq.args |  2 +-
>  .../qemuxml2argv-net-vhostuser-multiq.xml  |  2 +-
>  5 files changed, 5 insertions(+), 37 deletions(-)

ACK and safe for the freeze.

Michal

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

Re: [libvirt] [PATCH alt] conf: Allow user define their own alias

[Daniel P. Berrange]

On Fri, Sep 29, 2017 at 03:56:53PM +0200, Michal Privoznik wrote:
> On 09/29/2017 01:52 PM, Daniel P. Berrange wrote:
> > On Fri, Sep 29, 2017 at 12:09:52PM +0100, Daniel P. Berrange wrote:
> >> On Fri, Sep 29, 2017 at 01:04:34PM +0200, Michal Privoznik wrote:
> >>> On 09/29/2017 10:49 AM, Daniel P. Berrange wrote:
>  On Fri, Sep 29, 2017 at 09:06:01AM +0200, Michal Privoznik wrote:
> > https://bugzilla.redhat.com/show_bug.cgi?id=1434451
> >
> > It comes handy for management application to be able to have a
> > per-device label so that it can uniquely identify devices it
> > cares about. The advantage of this approach is that we don't have
> > to generate aliases at define time (non trivial amount of work
> > and problems). The only thing we do is parse the user supplied
> > tag and format it back. For instance:
> >
> > 
> >   
> >   
> >   
> >   
> 
>  I really do not like this - having two arbitrary string alias names is
>  just crazy. 
> >>>
> >>> Why is that? We have plenty of elements that do not match to anything at
> >>> hypervisor level. Firstly, there's metadata. Secondly, aliases in lxc
> >>> driver don't match anything either. And one can argue that the link in
> >>> qemu can be broken too. I mean, it's just for now that the aliases we
> >>> report happen to be device IDs we put onto the qemu's cmd line. Not to
> >>> mention devices that we put there and not report in the domain XML => no
> >>> IDs visible there.
> >>
>  If we want to add a second attribute to uniquely identify
>  devices, then it should be a UUID IMHO, so it at least has some tangible
>  benefit instead of just duplicating the existing id format
> >>>
> >>> I'm failing to see why UUID is better than any arbitrary string. You
> >>> mean we would generate the UUIDs if not supplied by user?
> >>
> >> Some hypervisors could map UUIDs to individual devices, so it is a more
> >> generally useful concept.
> > 
> > Also if we have APIs that accept an 'alias' string, we cannot extend them
> > to support the user's own 'alias' unless we guarantee the user supplied
> > alias is different from the alias we give to QEMU.  
> 
> We can if we document that libvirt generated aliases take precedence
> over user ones. That way we can keep the backward compatibility.

No, that's fragile. If libvirt were ever to change its alias name for
something there is a risk it might clash with an app name, which previously
did not clash.

> > If we used UUID format,
> > however, we don't have any ambiguity between a string that's an alias and
> > a string that's a UUID.
> 
> So IIUC users would be able to specify UUID for devices they want and
> for the rest libvirt will generate new one? That'll make XMLs a lot
> bigger. If we will not generate UUIDs, but just store ones provided by
> user this also makes sense then. Although, dealing with UUIDs is very
> user unfriendly, but that ship sailed a long time ago :-P

I would not generate UUIDs - only have them if the app asked for them, or
if the hypervisor we're using has assigned them itself.

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

Re: [libvirt] [PATCH alt] conf: Allow user define their own alias

[Michal Privoznik]

On 09/29/2017 01:16 PM, Peter Krempa wrote:
> On Fri, Sep 29, 2017 at 12:57:29 +0200, Michal Privoznik wrote:
>> On 09/29/2017 09:52 AM, Peter Krempa wrote:
>>> On Fri, Sep 29, 2017 at 09:06:01 +0200, Michal Privoznik wrote:
 https://bugzilla.redhat.com/show_bug.cgi?id=1434451

 It comes handy for management application to be able to have a
 per-device label so that it can uniquely identify devices it
 cares about. The advantage of this approach is that we don't have
 to generate aliases at define time (non trivial amount of work
 and problems). The only thing we do is parse the user supplied
 tag and format it back. For instance:

 
   
   
   
   
   
 

 Signed-off-by: Michal Privoznik 
 ---

 An alternative approach to:

 https://www.redhat.com/archives/libvir-list/2017-September/msg00765.html

 Honestly, I prefer this one as it's simpler and we don't have to care about
 devices changing their aliases on cold plug. I mean, on cold (un-)plug we'd
 have to regenerate the aliases so it might be hard to track certain device.
 But with this approach, it's no problem.

 Also, I'm not completely convinced about the name of @user attribute. So 
 I'm
 open for suggestions.
>>>
>>> With this proposed design you need to make sure to document that the
>>> user alias is _not_ guaranteed to be unique and also that it can't be
>>> used to match the device in any way.
>>>
>>
>> Sure. I'll just add it at the end of the paragraph that describes
>> . Err, hold on. There's none! But okay, I think I can find a
>> place to add it there.
>>
>> Even though my patch doesn't implement it (simply because I wanted to
>> have ACK on the design so I've saved it for follow up patches), I don't
>> quite see why we can't match user supplied aliases?
> 
> I think it will introduce more problems than solve. You will need to
> make sure that it's unique even across hotplug and add lookup code for
> everything. Additionally you can't add that as a mandatory field when
> looking up, since some device classes allow lookup only with partial XML
> descriptions (for unplug/update).

It can't be mandatory, that's the whole point. Sure. Well, in
DomainPostParse I can check for uniqueness pretty easily: just create an
empty list and start adding all strings provided by user. If the string
we're trying to add is already in the list we just error out. Sure, I'll
have to iterate over all devices, but that should be pretty easy since
we have virDomainDeviceInfoIterate(). All that's needed is to write the
callback function (= a few lines of code). Then, on hotplug goto 10.
IOW, if user alias is provided just check for its uniqueness.

> 
>> virsh domif-setlink fedora myNet down
>>
>> This has the great advantage of being ordering agnostic. You don't have
>> to check for the alias of the device you want to modify (as aliases can
>> change across different startups, right?). True, for that we would have
> 
> Well, you've used a bad example for this. 'domif-setlink' uses target and
> mac address, both of which are stable and don't rely on ordering on
> startup. Same thing applies to disks.

Oh right. domiftune is more like it.

> 
> The matching in virsh in this particular command is done by looking for
> the full element and then using that with the UpdateDevice() API, so the
> lookup is basically syntax sugar.
> 
>> to make sure that the supplied aliases are unique per domain (which is
>> trivial to achieve). But apart from that I don't quite see why we
>> shouldn't/can't do it.
> 
> Well, I don't think it's trivial. It's simpler than providing the alias
> which would be used with qemu, but you still have a zillion hotplug code
> paths which would need to check this.

Well, it might be slightly tricky yes. But in general, the
virDomain*Find() would try to match the user alias first (if provided)
else continue with current behaviour. I'm not quite sure what you mean
by zillion code paths.

> 
>>> I think that users which know semantics of the current alias may be very
>>> confused by putting user data with different semantics into the same
>>> field.
>>>
>>
>> Yep. As I say, I'm not happy with the name either. Any suggestion is
>> welcome. So a separate element then? Naming is hard.
> 
> I'm voting for separate element in case there's consensus that this
> route is a good idea.
> 

Yeah, it may look a bit better. Any idea on the element name?

Michal

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

[libvirt] libvirt-python can't be installed in wondows

[Zhu, Fei (NSB - CN/Nanjing)]

Hi, all

   I'm interesting in libvirt-python and ready to install this package. It 
seems it only can be installed in Linux and can't be installed in Windows. Is 
it true? thanks
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH alt] conf: Allow user define their own alias

[Michal Privoznik]

On 09/29/2017 01:52 PM, Daniel P. Berrange wrote:
> On Fri, Sep 29, 2017 at 12:09:52PM +0100, Daniel P. Berrange wrote:
>> On Fri, Sep 29, 2017 at 01:04:34PM +0200, Michal Privoznik wrote:
>>> On 09/29/2017 10:49 AM, Daniel P. Berrange wrote:
 On Fri, Sep 29, 2017 at 09:06:01AM +0200, Michal Privoznik wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1434451
>
> It comes handy for management application to be able to have a
> per-device label so that it can uniquely identify devices it
> cares about. The advantage of this approach is that we don't have
> to generate aliases at define time (non trivial amount of work
> and problems). The only thing we do is parse the user supplied
> tag and format it back. For instance:
>
> 
>   
>   
>   
>   

 I really do not like this - having two arbitrary string alias names is
 just crazy. 
>>>
>>> Why is that? We have plenty of elements that do not match to anything at
>>> hypervisor level. Firstly, there's metadata. Secondly, aliases in lxc
>>> driver don't match anything either. And one can argue that the link in
>>> qemu can be broken too. I mean, it's just for now that the aliases we
>>> report happen to be device IDs we put onto the qemu's cmd line. Not to
>>> mention devices that we put there and not report in the domain XML => no
>>> IDs visible there.
>>
 If we want to add a second attribute to uniquely identify
 devices, then it should be a UUID IMHO, so it at least has some tangible
 benefit instead of just duplicating the existing id format
>>>
>>> I'm failing to see why UUID is better than any arbitrary string. You
>>> mean we would generate the UUIDs if not supplied by user?
>>
>> Some hypervisors could map UUIDs to individual devices, so it is a more
>> generally useful concept.
> 
> Also if we have APIs that accept an 'alias' string, we cannot extend them
> to support the user's own 'alias' unless we guarantee the user supplied
> alias is different from the alias we give to QEMU.  

We can if we document that libvirt generated aliases take precedence
over user ones. That way we can keep the backward compatibility.

> If we used UUID format,
> however, we don't have any ambiguity between a string that's an alias and
> a string that's a UUID.

So IIUC users would be able to specify UUID for devices they want and
for the rest libvirt will generate new one? That'll make XMLs a lot
bigger. If we will not generate UUIDs, but just store ones provided by
user this also makes sense then. Although, dealing with UUIDs is very
user unfriendly, but that ship sailed a long time ago :-P

Michal

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

[libvirt] [PATCH 2/2] qemu: Support multiqueue virtio-blk

[Lin Ma]

qemu 2.7.0 introduces multiqueue virtio-blk(commit 2f27059).
This patch introduces a new attribute "queues". An example of
the XML:


  

The corresponding QEMU command line:

-device virtio-blk-pci,scsi=off,num-queues=4,id=virtio-disk0

Signed-off-by: Lin Ma 
---
 docs/formatdomain.html.in  |  6 +++-
 docs/schemas/domaincommon.rng  |  5 
 src/conf/domain_conf.c | 19 
 src/conf/domain_conf.h |  1 +
 src/qemu/qemu_capabilities.c   |  2 ++
 src/qemu/qemu_capabilities.h   |  1 +
 src/qemu/qemu_command.c|  4 +++
 .../qemuxml2argv-disk-virtio-drive-queues.args | 24 +++
 .../qemuxml2argv-disk-virtio-drive-queues.xml  | 34 ++
 tests/qemuxml2argvtest.c   |  2 ++
 .../qemuxml2xmlout-disk-virtio-drive-queues.xml|  1 +
 tests/qemuxml2xmltest.c|  1 +
 12 files changed, 99 insertions(+), 1 deletion(-)
 create mode 100644 
tests/qemuxml2argvdata/qemuxml2argv-disk-virtio-drive-queues.args
 create mode 100644 
tests/qemuxml2argvdata/qemuxml2argv-disk-virtio-drive-queues.xml
 create mode 12 
tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-virtio-drive-queues.xml

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 5bdcb569c..2b4eac1f8 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -2393,7 +2393,7 @@
 target dev='vdc' bus='virtio'/
   /disk
   disk type='file' device='disk'
-driver name='qemu' type='qcow2'/
+driver name='qemu' type='qcow2' queues='4'/
 source file='/var/lib/libvirt/images/domain.qcow'/
 backingStore type='file'
   format type='qcow2'/
@@ -3053,6 +3053,10 @@
 bus and "pci" or "ccw" address types.
 Since 1.2.8 (QEMU 2.1)
   
+  
+The optional queues attribute specifies the number of
+virt queues for virtio-blk. (Since 
3.8.0)
+  
   
   For virtio disks,
   Virtio-specific options can also be
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index bac371ea3..66b3d70c6 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -1811,6 +1811,11 @@
   
 
   
+  
+
+  
+
+  
   
   
 
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 87192eb2d..90572d51a 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -8761,6 +8761,15 @@ virDomainDiskDefDriverParseXML(virDomainDiskDefPtr def,
 }
 VIR_FREE(tmp);
 
+if ((tmp = virXMLPropString(cur, "queues")) &&
+virStrToLong_ui(tmp, NULL, 10, >queues) < 0) {
+virReportError(VIR_ERR_XML_ERROR,
+   _("'queues' attribute must be positive number: %s"),
+   tmp);
+goto cleanup;
+}
+VIR_FREE(tmp);
+
 ret = 0;
 
  cleanup:
@@ -21996,6 +22005,16 @@ virDomainDiskDefFormat(virBufferPtr buf,
 virBufferAsprintf(, " iothread='%u'", def->iothread);
 if (def->detect_zeroes)
 virBufferAsprintf(, " detect_zeroes='%s'", detect_zeroes);
+if (def->queues) {
+if (def->bus == VIR_DOMAIN_DISK_BUS_VIRTIO)
+virBufferAsprintf(, " queues='%u'", def->queues);
+else {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+   _("queues attribute in disk driver element is only "
+ "supported by virtio-blk"));
+return -1;
+}
+}
 
 virDomainVirtioOptionsFormat(, def->virtio);
 
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 05a035a16..28abe2b0b 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -668,6 +668,7 @@ struct _virDomainDiskDef {
 unsigned int iothread; /* unused = 0, > 0 specific thread # */
 int detect_zeroes; /* enum virDomainDiskDetectZeroes */
 char *domain_name; /* backend domain name */
+unsigned int queues;
 virDomainVirtioOptionsPtr virtio;
 };
 
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 085910dd4..f9028157f 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -442,6 +442,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
 
   /* 270 */
   "vxhs",
+  "virtio-blk.num-queues",
 );
 
 
@@ -1692,6 +1693,7 @@ static struct virQEMUCapsStringFlags 
virQEMUCapsObjectPropsVirtioBlk[] = {
 { "event_idx", QEMU_CAPS_VIRTIO_BLK_EVENT_IDX },
 { "scsi", QEMU_CAPS_VIRTIO_BLK_SCSI },
 { "logical_block_size", QEMU_CAPS_BLOCKIO },
+{ "num-queues", QEMU_CAPS_VIRTIO_BLK_NUM_QUEUES },
 };
 
 static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsVirtioNet[] = {
diff --git a/src/qemu/qemu_capabilities.h 

[libvirt] [PATCH 1/2] qemucapstest: Update test data for 'num-queues' property of virtio-blk

[Lin Ma]

Signed-off-by: Lin Ma 
---
 tests/qemucapabilitiesdata/caps_2.10.0.s390x.xml  | 1 +
 tests/qemucapabilitiesdata/caps_2.10.0.x86_64.xml | 1 +
 tests/qemucapabilitiesdata/caps_2.7.0.s390x.xml   | 1 +
 tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml  | 1 +
 tests/qemucapabilitiesdata/caps_2.8.0.s390x.xml   | 1 +
 tests/qemucapabilitiesdata/caps_2.8.0.x86_64.xml  | 1 +
 tests/qemucapabilitiesdata/caps_2.9.0.ppc64le.xml | 1 +
 tests/qemucapabilitiesdata/caps_2.9.0.s390x.xml   | 1 +
 tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml  | 1 +
 9 files changed, 9 insertions(+)

diff --git a/tests/qemucapabilitiesdata/caps_2.10.0.s390x.xml 
b/tests/qemucapabilitiesdata/caps_2.10.0.s390x.xml
index 2806345b9..2546ebdd9 100644
--- a/tests/qemucapabilitiesdata/caps_2.10.0.s390x.xml
+++ b/tests/qemucapabilitiesdata/caps_2.10.0.s390x.xml
@@ -140,6 +140,7 @@
   
   
   
+  
   201
   0
   
diff --git a/tests/qemucapabilitiesdata/caps_2.10.0.x86_64.xml 
b/tests/qemucapabilitiesdata/caps_2.10.0.x86_64.xml
index 8a31431c0..10a182e18 100644
--- a/tests/qemucapabilitiesdata/caps_2.10.0.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_2.10.0.x86_64.xml
@@ -223,6 +223,7 @@
   
   
   
+  
   201
   0
(v2.10.0)
diff --git a/tests/qemucapabilitiesdata/caps_2.7.0.s390x.xml 
b/tests/qemucapabilitiesdata/caps_2.7.0.s390x.xml
index fe7bca93b..c5dfa2a9d 100644
--- a/tests/qemucapabilitiesdata/caps_2.7.0.s390x.xml
+++ b/tests/qemucapabilitiesdata/caps_2.7.0.s390x.xml
@@ -134,6 +134,7 @@
   
   
   
+  
   2007000
   0
   
diff --git a/tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml 
b/tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml
index 3fd28f09f..59adff6c9 100644
--- a/tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml
@@ -207,6 +207,7 @@
   
   
   
+  
   2007000
   0
(v2.7.0)
diff --git a/tests/qemucapabilitiesdata/caps_2.8.0.s390x.xml 
b/tests/qemucapabilitiesdata/caps_2.8.0.s390x.xml
index 21bbb820d..6d26896ef 100644
--- a/tests/qemucapabilitiesdata/caps_2.8.0.s390x.xml
+++ b/tests/qemucapabilitiesdata/caps_2.8.0.s390x.xml
@@ -136,6 +136,7 @@
   
   
   
+  
   2007093
   0
   
diff --git a/tests/qemucapabilitiesdata/caps_2.8.0.x86_64.xml 
b/tests/qemucapabilitiesdata/caps_2.8.0.x86_64.xml
index 761f9d141..88029c04d 100644
--- a/tests/qemucapabilitiesdata/caps_2.8.0.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_2.8.0.x86_64.xml
@@ -209,6 +209,7 @@
   
   
   
+  
   2008000
   0
(v2.8.0)
diff --git a/tests/qemucapabilitiesdata/caps_2.9.0.ppc64le.xml 
b/tests/qemucapabilitiesdata/caps_2.9.0.ppc64le.xml
index a373a6db6..786cea8ea 100644
--- a/tests/qemucapabilitiesdata/caps_2.9.0.ppc64le.xml
+++ b/tests/qemucapabilitiesdata/caps_2.9.0.ppc64le.xml
@@ -172,6 +172,7 @@
   
   
   
+  
   2009000
   0
(v2.9.0)
diff --git a/tests/qemucapabilitiesdata/caps_2.9.0.s390x.xml 
b/tests/qemucapabilitiesdata/caps_2.9.0.s390x.xml
index e80782cfb..896ed503c 100644
--- a/tests/qemucapabilitiesdata/caps_2.9.0.s390x.xml
+++ b/tests/qemucapabilitiesdata/caps_2.9.0.s390x.xml
@@ -137,6 +137,7 @@
   
   
   
+  
   2009000
   0
   
diff --git a/tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml 
b/tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml
index 3641d0332..e3ff12727 100644
--- a/tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml
@@ -220,6 +220,7 @@
   
   
   
+  
   2009000
   0
(v2.9.0)
-- 
2.14.0

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

[libvirt] [PATCH 0/2] Add multiqueue support for virtio-blk

[Lin Ma]

The multiqueue for virtio-blk was introduced since qemu 2.7.0.
These patches supported it and update test data for it.

Lin Ma (2):
  qemucapstest: Update test data for 'num-queues' property of virtio-blk
  qemu: Support multiqueue virtio-blk

 docs/formatdomain.html.in  |  6 +++-
 docs/schemas/domaincommon.rng  |  5 
 src/conf/domain_conf.c | 19 
 src/conf/domain_conf.h |  1 +
 src/qemu/qemu_capabilities.c   |  2 ++
 src/qemu/qemu_capabilities.h   |  1 +
 src/qemu/qemu_command.c|  4 +++
 tests/qemucapabilitiesdata/caps_2.10.0.s390x.xml   |  1 +
 tests/qemucapabilitiesdata/caps_2.10.0.x86_64.xml  |  1 +
 tests/qemucapabilitiesdata/caps_2.7.0.s390x.xml|  1 +
 tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml   |  1 +
 tests/qemucapabilitiesdata/caps_2.8.0.s390x.xml|  1 +
 tests/qemucapabilitiesdata/caps_2.8.0.x86_64.xml   |  1 +
 tests/qemucapabilitiesdata/caps_2.9.0.ppc64le.xml  |  1 +
 tests/qemucapabilitiesdata/caps_2.9.0.s390x.xml|  1 +
 tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml   |  1 +
 .../qemuxml2argv-disk-virtio-drive-queues.args | 24 +++
 .../qemuxml2argv-disk-virtio-drive-queues.xml  | 34 ++
 tests/qemuxml2argvtest.c   |  2 ++
 .../qemuxml2xmlout-disk-virtio-drive-queues.xml|  1 +
 tests/qemuxml2xmltest.c|  1 +
 21 files changed, 108 insertions(+), 1 deletion(-)
 create mode 100644 
tests/qemuxml2argvdata/qemuxml2argv-disk-virtio-drive-queues.args
 create mode 100644 
tests/qemuxml2argvdata/qemuxml2argv-disk-virtio-drive-queues.xml
 create mode 12 
tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-virtio-drive-queues.xml

-- 
2.14.0

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

Re: [libvirt] [PATCH v4 5/7] nodedev: Disable/re-enable polling on the udev fd

[John Ferlan]

On 09/28/2017 06:00 AM, Erik Skultety wrote:
> [...]
> 
>>>
>>>  nodeDeviceLock();
>>> +priv = driver->privateData;
>>>  udev_monitor = DRV_STATE_UDEV_MONITOR(driver);
>>>
>>>  if (!udevEventCheckMonitorFD(udev_monitor, 
>>> privateData->monitor_fd)) {
>>> @@ -1725,6 +1727,9 @@ udevEventHandleThread(void *opaque)
>>>  device = udev_monitor_receive_device(udev_monitor);
>>>  nodeDeviceUnlock();
>>>
>>> +/* Re-enable polling for new events on the @udev_monitor */
>>> +virEventUpdateHandle(priv->watch, VIR_EVENT_HANDLE_READABLE);
>>> +
>>
>> I think this should only be done when privateData->nevents == 0?  If we
>> have multiple events to read, then calling virEventPollUpdateHandle,
>> (eventually) for every pass through the loop seems like a bit of
>> overkill especially if udevEventHandleCallback turns right around and
>> disables it again.
>>
>> Also fortunately there isn't more than one udev thread sending the
>> events since you access the priv->watch without the driver lock...
>>
>> Conversely perhaps we only disable if events > 1... Then again, how does
>> one get to 2 events queued if we're disabling as soon as we increment
> 
> Very good point, technically events would still get queued, we just wouldn't
> check and yes, we would process 1 event at a time. Not optimal, but if you 
> look
> at the original code and compare it with this one performance-wise (and I hope
> I haven't missed anything that would render everything I write next a complete
> rubbish), the time complexity hasn't changed, the space complexity hasn't
> changed, what changed is code complexity which makes the code a bit slower due
> to the excessive locking and toggling the FD polling back and forth. So
> essentially you've got the same thing as you had before..but asynchronous.
> However, yes, the usage of @nevents is completely useless now (haven't 
> realized
> that immediately, thanks) and a simple signalling should suffice.

Having it "slower" is necessarily bad ;-)  That gives some of the other
slower buggers a chance to fill in the details we need. Throwing the
control back to udev quicker could aid in that too.

> 
> So how could we make it faster though? I thought more about the idea you 
> shared
> in one of the previous reviews, letting the thread actually pull all the data
> from the monitor, to which I IIRC replied something in the sense that the 
> event
> counting mechanism wouldn't allow that and it would break. Okay, let's drop 
> the
> event counting. What if we now let both the udev handler thread and the event
> loop "poll" the file descriptor, IOW let the event loop polling the monitor 
> fd,
> thus invoking udevHandleCallback which would in turn keep signalling the 
> handler
> thread that there are some data. The difference now in the handler thread 
> would
> be that it wouldn't blindly trust the callback about the data, because of the
> scheduling issue, it would keep poking the monitor itself until it gets either
> EAGAIN or EWOULDBLOCK from recvmsg() called in libudev,  which would then be 
> the
> signal to start waiting on the condition. The next an event appears, a signal
> from udevHandleCallback would finally have a meaning and wouldn't be ignored.

Is making it faster really a goal?  It's preferable that it works
consistently I think. The various errno possibilities and the desire to
avoid the "udev_monitor_receive_device returned NULL" message processing
because we had too many "cooks in the kitchen" trying to determine
whether a new device was really available or was this just another
notification for something we're already processing.

Also, not that I expect the udev code to change, but if a new errno is
added then we may have to keep up... Always a fear especially if we're
using the errno to dictate our algorithm.

> 
> This way, it's actually the handler thread who's 'the boss', as most of the
> signals from udevHandleCallback would be lost/NOPs.
> 
> Would you agree to such an approach?

At some point we get too fancy and think too hard about a problem
letting it consume us.  I think when I presented my idea - I wasn't
really understanding the details of how we consume the udev data. Now
that I have a bit more of a clue - perhaps the original idea wasn't so
good. If we receive multiple notifications that a device is ready to be
processed even though we could be processing it - how are we to truly
know whether we missed one or we really got it and udev was just
reminding us again.

I'm not against looking at a different mechanism - the question then
becomes from your perspective is it worth it?

John

> 
> Erik
> 
>> nevents? Wouldn't this totally obviate the need for nevents?
>>
>> I would think it would be overkill to disable/enable for just 1 event.
>> If multiple are queued, then yeah we're starting to get behind... Of
>> course that could effect the re-enable when events == 1 (or some
>> 

[libvirt] [PATCH 2/2] nwfilter: Fix memory leak and error path

[John Ferlan]

Found by Coverity. If virNWFilterHashTablePut, then the 3rd arg @val
must be free'd since it would be leaked.

This also fixes potential problem on the error path where the caller
could assume the virNWFilterHashTablePut was successful when in fact
it failed leading to other issues.

Signed-off-by: John Ferlan 
---
 src/nwfilter/nwfilter_gentech_driver.c | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/src/nwfilter/nwfilter_gentech_driver.c 
b/src/nwfilter/nwfilter_gentech_driver.c
index 7a3d115ba..500197bae 100644
--- a/src/nwfilter/nwfilter_gentech_driver.c
+++ b/src/nwfilter/nwfilter_gentech_driver.c
@@ -525,9 +525,12 @@ virNWFilterDetermineMissingVarsRec(virNWFilterDefPtr 
filter,
 }
 
 varAccess = virBufferContentAndReset();
-virNWFilterHashTablePut(missing_vars, varAccess,
-val);
+rc = virNWFilterHashTablePut(missing_vars, varAccess, val);
 VIR_FREE(varAccess);
+if (rc < 0) {
+virNWFilterVarValueFree(val);
+return -1;
+}
 }
 }
 } else if (inc) {
-- 
2.13.5

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

[libvirt] [PATCH 0/2] Fix another possible memory leak in nwfilter

[John Ferlan]

Oh lucky me - why am I stuck in nwfilter land

The first patch just cleans up the code and the second one resolves the
memory leak problem as well as a general error path problem.

Coverity noted that the error path wasn't checked - this is because
other recent changes in the module caused Coverity to decide to rethink
about this one noticing the lack of an error check. Of course upon more
visual inspection, the memory leak was obvious too. 

John Ferlan (2):
  nwfilter: Clean up virNWFilterDetermineMissingVarsRec returns
  nwfilter: Fix memory leak and error path

 src/nwfilter/nwfilter_gentech_driver.c | 30 +-
 1 file changed, 13 insertions(+), 17 deletions(-)

-- 
2.13.5

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

[libvirt] [PATCH 1/2] nwfilter: Clean up virNWFilterDetermineMissingVarsRec returns

[John Ferlan]

Rather than using loop break;'s in order to force a return
of rc = -1, let's just return -1 immediately on the various
error paths and then return 0 on the success path.

Signed-off-by: John Ferlan 
---
 src/nwfilter/nwfilter_gentech_driver.c | 23 ---
 1 file changed, 8 insertions(+), 15 deletions(-)

diff --git a/src/nwfilter/nwfilter_gentech_driver.c 
b/src/nwfilter/nwfilter_gentech_driver.c
index 6758200b5..7a3d115ba 100644
--- a/src/nwfilter/nwfilter_gentech_driver.c
+++ b/src/nwfilter/nwfilter_gentech_driver.c
@@ -494,7 +494,7 @@ virNWFilterDetermineMissingVarsRec(virNWFilterDefPtr filter,
virNWFilterDriverStatePtr driver)
 {
 virNWFilterObjPtr obj;
-int rc = 0;
+int rc;
 size_t i, j;
 virNWFilterDefPtr next_filter;
 virNWFilterDefPtr newNext_filter;
@@ -515,15 +515,13 @@ virNWFilterDetermineMissingVarsRec(virNWFilterDefPtr 
filter,
 virNWFilterVarAccessPrint(rule->varAccess[j], );
 if (virBufferError()) {
 virReportOOMError();
-rc = -1;
-break;
+return -1;
 }
 
 val = virNWFilterVarValueCreateSimpleCopyValue("1");
 if (!val) {
 virBufferFreeAndReset();
-rc = -1;
-break;
+return -1;
 }
 
 varAccess = virBufferContentAndReset();
@@ -532,21 +530,16 @@ virNWFilterDetermineMissingVarsRec(virNWFilterDefPtr 
filter,
 VIR_FREE(varAccess);
 }
 }
-if (rc)
-break;
 } else if (inc) {
 VIR_DEBUG("Following filter %s", inc->filterref);
 if (!(obj = 
virNWFilterObjListFindInstantiateFilter(driver->nwfilters,
-
inc->filterref))) {
-rc = -1;
-break;
-}
+
inc->filterref)))
+return -1;
 
 /* create a temporary hashmap for depth-first tree traversal */
 if (!(tmpvars = virNWFilterCreateVarsFrom(inc->params, vars))) {
-rc = -1;
 virNWFilterObjUnlock(obj);
-break;
+return -1;
 }
 
 next_filter = virNWFilterObjGetDef(obj);
@@ -571,10 +564,10 @@ virNWFilterDetermineMissingVarsRec(virNWFilterDefPtr 
filter,
 
 virNWFilterObjUnlock(obj);
 if (rc < 0)
-break;
+return -1;
 }
 }
-return rc;
+return 0;
 }
 
 
-- 
2.13.5

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

Re: [libvirt] [PATCH] conf: Split out parsing of network disk source XML elements

[John Ferlan]

On 09/29/2017 05:02 AM, Peter Krempa wrote:
> virDomainDiskSourceParse got to the point of being an ugly spaghetti
> mess by adding more and more stuff into it. Split out parsing of network
> disk information into a separate function so that it stays contained.
> ---
> I've had this patch on a branch that makes virDomainDiskSourceParse
> uglier, but with the recent VxHS patches I've got a merge conflict, thus
> I'm sending it now.
> 
> Note: this patch was generated with the "patience" diff algorithm
> 
> 
>  src/conf/domain_conf.c | 183 
> ++---
>  1 file changed, 99 insertions(+), 84 deletions(-)
> 

Reviewed-by: John Ferlan 

Seems "safe" to me too.

John

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

[libvirt] [PATCH] docs: Document the real behaviour of suspend-to-{mem, disk}

[Martin Kletzander]

We get a question every now and then about why hibernation works when
suspend-to-disk is disabled and similar.  Let's hope that, by documenting the
obvious more blatantly, people will get more informed.

Signed-off-by: Martin Kletzander 
---
 docs/formatdomain.html.in | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 5bdcb569c4c0..5dcf2fedb01c 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -1664,7 +1664,11 @@
   These elements enable ('yes') or disable ('no') BIOS support
 for S3 (suspend-to-mem) and S4 (suspend-to-disk) ACPI sleep
 states. If nothing is specified, then the hypervisor will be
-left with its default value.
+left with its default value.
+Note: This setting cannot prevent the guest OS from performing
+a suspend as the guest OS itself can choose to circumvent the
+unavailability of the sleep states (e.g. S4 by turning off
+completely).
 
 
 Hypervisor features
-- 
2.14.2

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

Re: [libvirt] [PATCH v2] qemu: add the print of page size in cmd domjobinfo

[John Ferlan]

On 09/28/2017 04:32 AM, Chao Fan wrote:
> The command "info migrate" of qemu outputs the dirty-pages-rate during
> migration, but page size is different in different architectures. So
> page size should be output to calculate dirty pages in bytes.
> 
> Page size is already implemented with commit
> 030ce1f8612215fcbe9d353dfeaeb2937f8e3f94 in qemu.
> Now Implement the counter-part in libvirt.
> 
> Signed-off-by: Chao Fan 
> Signed-off-by: Li Zhijian 
> ---
> v1 -> v2:
>   Follow the suggestion of John Ferlan:
>   1. Drop the fix for unrelated coding style problem.
>   2. Fix typo.
>   3. Improve a judgment logic when failing to get page size.
> ---
>  include/libvirt/libvirt-domain.h | 7 +++
>  src/qemu/qemu_domain.c   | 6 ++
>  src/qemu/qemu_migration_cookie.c | 7 +++
>  src/qemu/qemu_monitor.h  | 1 +
>  src/qemu/qemu_monitor_json.c | 2 ++
>  tools/virsh-domain.c | 8 
>  6 files changed, 31 insertions(+)
> 
> diff --git a/include/libvirt/libvirt-domain.h 
> b/include/libvirt/libvirt-domain.h
> index 030a62c43..1f4ddcf66 100644
> --- a/include/libvirt/libvirt-domain.h
> +++ b/include/libvirt/libvirt-domain.h
> @@ -3336,6 +3336,13 @@ typedef enum {
>  # define VIR_DOMAIN_JOB_MEMORY_DIRTY_RATE"memory_dirty_rate"
>  
>  /**
> + * VIR_DOMAIN_JOB_MEMORY_PAGE_SIZE:
> + *
> + * virDomainGetJobStats field: page size of the memory in this domain
> + */
> +# define VIR_DOMAIN_JOB_MEMORY_PAGE_SIZE"page_size"
> +
> +/**
>   * VIR_DOMAIN_JOB_MEMORY_ITERATION:
>   *
>   * virDomainGetJobStats field: current iteration over domain's memory
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index cb371f1e8..ce342b670 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -570,6 +570,12 @@ qemuDomainJobInfoToParams(qemuDomainJobInfoPtr jobInfo,
>  stats->ram_iteration) < 0)
>  goto error;
>  
> +if (stats->ram_page_size && (!(stats->ram_pag_size > 0) ||
> +virTypedParamsAddULLong(, , ,

Any reason to not just be:

if (stats->ram_page_size > 0 &&
virTypedParamsAddULLong(, , ,

?

That "(!(stats->ram_pag_size > 0) ||" is a bit harsh to read

Things look reasonable to me otherwise though.

This won't make 3.8.0 since we're in freeze; however, it looks
reasonable for 3.9.0. I can also add a brief docs/news.xml article too -
unless you want to respin with that.  Your call.

John

> +VIR_DOMAIN_JOB_MEMORY_PAGE_SIZE,
> +stats->ram_page_size) < 0))
> +goto error;
> +
>  if (virTypedParamsAddULLong(, , ,
>  VIR_DOMAIN_JOB_DISK_TOTAL,
>  stats->disk_total +
> diff --git a/src/qemu/qemu_migration_cookie.c 
> b/src/qemu/qemu_migration_cookie.c
> index eef40a6cd..bc6a8dc55 100644
> --- a/src/qemu/qemu_migration_cookie.c
> +++ b/src/qemu/qemu_migration_cookie.c
> @@ -654,6 +654,10 @@ qemuMigrationCookieStatisticsXMLFormat(virBufferPtr buf,
>stats->ram_iteration);
>  
>  virBufferAsprintf(buf, "<%1$s>%2$llu\n",
> +  VIR_DOMAIN_JOB_MEMORY_PAGE_SIZE,
> +  stats->ram_page_size);
> +
> +virBufferAsprintf(buf, "<%1$s>%2$llu\n",
>VIR_DOMAIN_JOB_DISK_TOTAL,
>stats->disk_total);
>  virBufferAsprintf(buf, "<%1$s>%2$llu\n",
> @@ -1014,6 +1018,9 @@ 
> qemuMigrationCookieStatisticsXMLParse(xmlXPathContextPtr ctxt)
>  virXPathULongLong("string(./" VIR_DOMAIN_JOB_MEMORY_ITERATION "[1])",
>ctxt, >ram_iteration);
>  
> +virXPathULongLong("string(./" VIR_DOMAIN_JOB_MEMORY_PAGE_SIZE "[1])",
> +  ctxt, >ram_page_size);
> +
>  virXPathULongLong("string(./" VIR_DOMAIN_JOB_DISK_TOTAL "[1])",
>ctxt, >disk_total);
>  virXPathULongLong("string(./" VIR_DOMAIN_JOB_DISK_PROCESSED "[1])",
> diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
> index 6414d2483..1e3322433 100644
> --- a/src/qemu/qemu_monitor.h
> +++ b/src/qemu/qemu_monitor.h
> @@ -677,6 +677,7 @@ struct _qemuMonitorMigrationStats {
>  unsigned long long ram_normal;
>  unsigned long long ram_normal_bytes;
>  unsigned long long ram_dirty_rate;
> +unsigned long long ram_page_size;
>  unsigned long long ram_iteration;
>  
>  unsigned long long disk_transferred;
> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> index 63b855920..625cbc134 100644
> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c
> @@ -2892,6 +2892,8 @@ qemuMonitorJSONGetMigrationStatsReply(virJSONValuePtr 
> reply,
>
> >ram_normal_bytes));
>  ignore_value(virJSONValueObjectGetNumberUlong(ram, 
> "dirty-pages-rate",
>

Re: [libvirt] gnulib tests in libvirt broken by newer glibc 2.26

[Christian Ehrhardt]

On Fri, Sep 29, 2017 at 10:45 AM, Daniel P. Berrange 
wrote:

> On Thu, Sep 28, 2017 at 04:41:37PM -0700, Paul Eggert wrote:
> > That patch essentially negates the point of the test, which is that
> getopt
> > should be visible from unistd.h. I'd rather fix the problem than nuke the
> > test.
> >
> > Could you explain what the Gnulib problem is here? I can't really see it
> in
> > your email. A self-contained example would help.
> >
> > For what it's worth, I could not reproduce the problem on Fedora 26 by
> doing
> > this in Gnulib (this tells 'configure' to use Gnulib-supplied getopt.h
> and
> > getopt.c):
>
> Fedora 26 only has glibc 2.25 - you need to have Fedora rawhide to get
> the broken behaviour, as that has glibc 2.26.90
>
> > ./gnulib-tool --create-testdir --dir foo getopt-posix
> > cd foo
> > ./configure gl_cv_func_getopt_posix=no ac_cv_header_getopt_h=no
> > make
> > make check
>

As Daniel said at least glibc 2.26 as in Fedora rawhide or Ubuntu Artful.
But thanks for these commands that way I could reproduce without needing
any of the libvirt build env.

Here [1] a log of your commands on such a system showing the issue.
I added a gdb to show the assert and an LD_DEBUG so you can see that getopt
is not taken from gnulib as the test assumes when verifying behavior.

[1]: http://paste.ubuntu.com/25638847/


> Regards,
> Daniel
> --
> |: https://berrange.com  -o-https://www.flickr.com/photos/
> dberrange :|
> |: https://libvirt.org -o-
> https://fstop138.berrange.com :|
> |: https://entangle-photo.org-o-https://www.instagram.com/
> dberrange :|
>



-- 
Christian Ehrhardt
Software Engineer, Ubuntu Server
Canonical Ltd
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH alt] conf: Allow user define their own alias

[Daniel P. Berrange]

On Fri, Sep 29, 2017 at 12:09:52PM +0100, Daniel P. Berrange wrote:
> On Fri, Sep 29, 2017 at 01:04:34PM +0200, Michal Privoznik wrote:
> > On 09/29/2017 10:49 AM, Daniel P. Berrange wrote:
> > > On Fri, Sep 29, 2017 at 09:06:01AM +0200, Michal Privoznik wrote:
> > >> https://bugzilla.redhat.com/show_bug.cgi?id=1434451
> > >>
> > >> It comes handy for management application to be able to have a
> > >> per-device label so that it can uniquely identify devices it
> > >> cares about. The advantage of this approach is that we don't have
> > >> to generate aliases at define time (non trivial amount of work
> > >> and problems). The only thing we do is parse the user supplied
> > >> tag and format it back. For instance:
> > >>
> > >> 
> > >>   
> > >>   
> > >>   
> > >>   
> > > 
> > > I really do not like this - having two arbitrary string alias names is
> > > just crazy. 
> > 
> > Why is that? We have plenty of elements that do not match to anything at
> > hypervisor level. Firstly, there's metadata. Secondly, aliases in lxc
> > driver don't match anything either. And one can argue that the link in
> > qemu can be broken too. I mean, it's just for now that the aliases we
> > report happen to be device IDs we put onto the qemu's cmd line. Not to
> > mention devices that we put there and not report in the domain XML => no
> > IDs visible there.
> 
> > > If we want to add a second attribute to uniquely identify
> > > devices, then it should be a UUID IMHO, so it at least has some tangible
> > > benefit instead of just duplicating the existing id format
> > 
> > I'm failing to see why UUID is better than any arbitrary string. You
> > mean we would generate the UUIDs if not supplied by user?
> 
> Some hypervisors could map UUIDs to individual devices, so it is a more
> generally useful concept.

Also if we have APIs that accept an 'alias' string, we cannot extend them
to support the user's own 'alias' unless we guarantee the user supplied
alias is different from the alias we give to QEMU.  If we used UUID format,
however, we don't have any ambiguity between a string that's an alias and
a string that's a UUID.

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

Re: [libvirt] [PATCH alt] conf: Allow user define their own alias

[Daniel P. Berrange]

On Fri, Sep 29, 2017 at 01:04:34PM +0200, Michal Privoznik wrote:
> On 09/29/2017 10:49 AM, Daniel P. Berrange wrote:
> > On Fri, Sep 29, 2017 at 09:06:01AM +0200, Michal Privoznik wrote:
> >> https://bugzilla.redhat.com/show_bug.cgi?id=1434451
> >>
> >> It comes handy for management application to be able to have a
> >> per-device label so that it can uniquely identify devices it
> >> cares about. The advantage of this approach is that we don't have
> >> to generate aliases at define time (non trivial amount of work
> >> and problems). The only thing we do is parse the user supplied
> >> tag and format it back. For instance:
> >>
> >> 
> >>   
> >>   
> >>   
> >>   
> > 
> > I really do not like this - having two arbitrary string alias names is
> > just crazy. 
> 
> Why is that? We have plenty of elements that do not match to anything at
> hypervisor level. Firstly, there's metadata. Secondly, aliases in lxc
> driver don't match anything either. And one can argue that the link in
> qemu can be broken too. I mean, it's just for now that the aliases we
> report happen to be device IDs we put onto the qemu's cmd line. Not to
> mention devices that we put there and not report in the domain XML => no
> IDs visible there.

> > If we want to add a second attribute to uniquely identify
> > devices, then it should be a UUID IMHO, so it at least has some tangible
> > benefit instead of just duplicating the existing id format
> 
> I'm failing to see why UUID is better than any arbitrary string. You
> mean we would generate the UUIDs if not supplied by user?

Some hypervisors could map UUIDs to individual devices, so it is a more
generally useful concept.

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

Re: [libvirt] [PATCH alt] conf: Allow user define their own alias

[Peter Krempa]

On Fri, Sep 29, 2017 at 12:57:29 +0200, Michal Privoznik wrote:
> On 09/29/2017 09:52 AM, Peter Krempa wrote:
> > On Fri, Sep 29, 2017 at 09:06:01 +0200, Michal Privoznik wrote:
> >> https://bugzilla.redhat.com/show_bug.cgi?id=1434451
> >>
> >> It comes handy for management application to be able to have a
> >> per-device label so that it can uniquely identify devices it
> >> cares about. The advantage of this approach is that we don't have
> >> to generate aliases at define time (non trivial amount of work
> >> and problems). The only thing we do is parse the user supplied
> >> tag and format it back. For instance:
> >>
> >> 
> >>   
> >>   
> >>   
> >>   
> >>   
> >> 
> >>
> >> Signed-off-by: Michal Privoznik 
> >> ---
> >>
> >> An alternative approach to:
> >>
> >> https://www.redhat.com/archives/libvir-list/2017-September/msg00765.html
> >>
> >> Honestly, I prefer this one as it's simpler and we don't have to care about
> >> devices changing their aliases on cold plug. I mean, on cold (un-)plug we'd
> >> have to regenerate the aliases so it might be hard to track certain device.
> >> But with this approach, it's no problem.
> >>
> >> Also, I'm not completely convinced about the name of @user attribute. So 
> >> I'm
> >> open for suggestions.
> > 
> > With this proposed design you need to make sure to document that the
> > user alias is _not_ guaranteed to be unique and also that it can't be
> > used to match the device in any way.
> > 
> 
> Sure. I'll just add it at the end of the paragraph that describes
> . Err, hold on. There's none! But okay, I think I can find a
> place to add it there.
> 
> Even though my patch doesn't implement it (simply because I wanted to
> have ACK on the design so I've saved it for follow up patches), I don't
> quite see why we can't match user supplied aliases?

I think it will introduce more problems than solve. You will need to
make sure that it's unique even across hotplug and add lookup code for
everything. Additionally you can't add that as a mandatory field when
looking up, since some device classes allow lookup only with partial XML
descriptions (for unplug/update).

> virsh domif-setlink fedora myNet down
> 
> This has the great advantage of being ordering agnostic. You don't have
> to check for the alias of the device you want to modify (as aliases can
> change across different startups, right?). True, for that we would have

Well, you've used a bad example for this. 'domif-setlink' uses target and
mac address, both of which are stable and don't rely on ordering on
startup. Same thing applies to disks.

The matching in virsh in this particular command is done by looking for
the full element and then using that with the UpdateDevice() API, so the
lookup is basically syntax sugar.

> to make sure that the supplied aliases are unique per domain (which is
> trivial to achieve). But apart from that I don't quite see why we
> shouldn't/can't do it.

Well, I don't think it's trivial. It's simpler than providing the alias
which would be used with qemu, but you still have a zillion hotplug code
paths which would need to check this.

> > I think that users which know semantics of the current alias may be very
> > confused by putting user data with different semantics into the same
> > field.
> > 
> 
> Yep. As I say, I'm not happy with the name either. Any suggestion is
> welcome. So a separate element then? Naming is hard.

I'm voting for separate element in case there's consensus that this
route is a good idea.


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

Re: [libvirt] [PATCH alt] conf: Allow user define their own alias

[Michal Privoznik]

On 09/29/2017 10:49 AM, Daniel P. Berrange wrote:
> On Fri, Sep 29, 2017 at 09:06:01AM +0200, Michal Privoznik wrote:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1434451
>>
>> It comes handy for management application to be able to have a
>> per-device label so that it can uniquely identify devices it
>> cares about. The advantage of this approach is that we don't have
>> to generate aliases at define time (non trivial amount of work
>> and problems). The only thing we do is parse the user supplied
>> tag and format it back. For instance:
>>
>> 
>>   
>>   
>>   
>>   
> 
> I really do not like this - having two arbitrary string alias names is
> just crazy. 

Why is that? We have plenty of elements that do not match to anything at
hypervisor level. Firstly, there's metadata. Secondly, aliases in lxc
driver don't match anything either. And one can argue that the link in
qemu can be broken too. I mean, it's just for now that the aliases we
report happen to be device IDs we put onto the qemu's cmd line. Not to
mention devices that we put there and not report in the domain XML => no
IDs visible there.

> If we want to add a second attribute to uniquely identify
> devices, then it should be a UUID IMHO, so it at least has some tangible
> benefit instead of just duplicating the existing id format

I'm failing to see why UUID is better than any arbitrary string. You
mean we would generate the UUIDs if not supplied by user?

Michal

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

Re: [libvirt] [PATCH alt] conf: Allow user define their own alias

[Michal Privoznik]

On 09/29/2017 09:52 AM, Peter Krempa wrote:
> On Fri, Sep 29, 2017 at 09:06:01 +0200, Michal Privoznik wrote:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1434451
>>
>> It comes handy for management application to be able to have a
>> per-device label so that it can uniquely identify devices it
>> cares about. The advantage of this approach is that we don't have
>> to generate aliases at define time (non trivial amount of work
>> and problems). The only thing we do is parse the user supplied
>> tag and format it back. For instance:
>>
>> 
>>   
>>   
>>   
>>   
>>   
>> 
>>
>> Signed-off-by: Michal Privoznik 
>> ---
>>
>> An alternative approach to:
>>
>> https://www.redhat.com/archives/libvir-list/2017-September/msg00765.html
>>
>> Honestly, I prefer this one as it's simpler and we don't have to care about
>> devices changing their aliases on cold plug. I mean, on cold (un-)plug we'd
>> have to regenerate the aliases so it might be hard to track certain device.
>> But with this approach, it's no problem.
>>
>> Also, I'm not completely convinced about the name of @user attribute. So I'm
>> open for suggestions.
> 
> With this proposed design you need to make sure to document that the
> user alias is _not_ guaranteed to be unique and also that it can't be
> used to match the device in any way.
> 

Sure. I'll just add it at the end of the paragraph that describes
. Err, hold on. There's none! But okay, I think I can find a
place to add it there.

Even though my patch doesn't implement it (simply because I wanted to
have ACK on the design so I've saved it for follow up patches), I don't
quite see why we can't match user supplied aliases?

virsh domif-setlink fedora myNet down

This has the great advantage of being ordering agnostic. You don't have
to check for the alias of the device you want to modify (as aliases can
change across different startups, right?). True, for that we would have
to make sure that the supplied aliases are unique per domain (which is
trivial to achieve). But apart from that I don't quite see why we
shouldn't/can't do it.

> I think that users which know semantics of the current alias may be very
> confused by putting user data with different semantics into the same
> field.
> 

Yep. As I say, I'm not happy with the name either. Any suggestion is
welcome. So a separate element then? Naming is hard.

Michal

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

Re: [libvirt] [PATCH python] Unify whitespace around *_ALLOW_THREADS macros

[Daniel P. Berrange]

On Fri, Sep 29, 2017 at 01:28:20AM +0300, Nir Soffer wrote:
> Most of the code treats libvirt API calls as separate block, keeping one
> blank line before the LIBVIRT_BEGIN_ALLOW_THREAD, and one blank line
> after LIBVIRT_END_ALLOW_THREADS. Unify the whitespace so all calls
> wrapped with these macros are treated as a separate block.
> ---
>  libvirt-override.c | 115 
> -
>  1 file changed, 87 insertions(+), 28 deletions(-)

Reviewed-by: Daniel P. Berrange 

and pushed to git.

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

Re: [libvirt] [PATCH alt] conf: Allow user define their own alias

[Daniel P. Berrange]

On Fri, Sep 29, 2017 at 09:06:01AM +0200, Michal Privoznik wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1434451
> 
> It comes handy for management application to be able to have a
> per-device label so that it can uniquely identify devices it
> cares about. The advantage of this approach is that we don't have
> to generate aliases at define time (non trivial amount of work
> and problems). The only thing we do is parse the user supplied
> tag and format it back. For instance:
> 
> 
>   
>   
>   
>   

I really do not like this - having two arbitrary string alias names is
just crazy.  If we want to add a second attribute to uniquely identify
devices, then it should be a UUID IMHO, so it at least has some tangible
benefit instead of just duplicating the existing id format


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

[libvirt] [PATCH] conf: Split out parsing of network disk source XML elements

[Peter Krempa]

virDomainDiskSourceParse got to the point of being an ugly spaghetti
mess by adding more and more stuff into it. Split out parsing of network
disk information into a separate function so that it stays contained.
---
I've had this patch on a branch that makes virDomainDiskSourceParse
uglier, but with the recent VxHS patches I've got a merge conflict, thus
I'm sending it now.

Note: this patch was generated with the "patience" diff algorithm


 src/conf/domain_conf.c | 183 ++---
 1 file changed, 99 insertions(+), 84 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 87192eb2d..98f5666ef 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -8106,18 +8106,109 @@ virDomainDiskSourcePoolDefParse(xmlNodePtr node,
 }


-int
-virDomainDiskSourceParse(xmlNodePtr node,
- xmlXPathContextPtr ctxt,
- virStorageSourcePtr src,
- unsigned int flags)
+static int
+virDomainDiskSourceNetworkParse(xmlNodePtr node,
+xmlXPathContextPtr ctxt,
+virStorageSourcePtr src,
+unsigned int flags)
 {
-int ret = -1;
 char *protocol = NULL;
-xmlNodePtr saveNode = ctxt->node;
 char *haveTLS = NULL;
 char *tlsCfg = NULL;
 int tlsCfgVal;
+int ret = -1;
+
+if (!(protocol = virXMLPropString(node, "protocol"))) {
+virReportError(VIR_ERR_XML_ERROR, "%s",
+   _("missing network source protocol type"));
+goto cleanup;
+}
+
+if ((src->protocol = virStorageNetProtocolTypeFromString(protocol)) <= 0) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("unknown protocol type '%s'"), protocol);
+goto cleanup;
+}
+
+if (!(src->path = virXMLPropString(node, "name")) &&
+src->protocol != VIR_STORAGE_NET_PROTOCOL_NBD) {
+virReportError(VIR_ERR_XML_ERROR, "%s",
+   _("missing name for disk source"));
+goto cleanup;
+}
+
+/* Check tls=yes|no domain setting for the block device
+ * At present only VxHS. Other block devices may be added later */
+if (src->protocol == VIR_STORAGE_NET_PROTOCOL_VXHS &&
+(haveTLS = virXMLPropString(node, "tls"))) {
+if ((src->haveTLS =
+virTristateBoolTypeFromString(haveTLS)) <= 0) {
+virReportError(VIR_ERR_XML_ERROR,
+   _("unknown disk source 'tls' setting '%s'"),
+   haveTLS);
+goto cleanup;
+}
+}
+
+if ((flags & VIR_DOMAIN_DEF_PARSE_STATUS) &&
+(tlsCfg = virXMLPropString(node, "tlsFromConfig"))) {
+if (virStrToLong_i(tlsCfg, NULL, 10, ) < 0) {
+virReportError(VIR_ERR_XML_ERROR,
+   _("Invalid tlsFromConfig value: %s"),
+   tlsCfg);
+goto cleanup;
+}
+src->tlsFromConfig = !!tlsCfgVal;
+}
+
+/* for historical reasons the volume name for gluster volume is stored
+ * as a part of the path. This is hard to work with when dealing with
+ * relative names. Split out the volume into a separate variable */
+if (src->path && src->protocol == VIR_STORAGE_NET_PROTOCOL_GLUSTER) {
+char *tmp;
+if (!(tmp = strchr(src->path, '/')) ||
+tmp == src->path) {
+virReportError(VIR_ERR_XML_ERROR,
+   _("missing volume name or file name in "
+ "gluster source path '%s'"), src->path);
+goto cleanup;
+}
+
+src->volume = src->path;
+
+if (VIR_STRDUP(src->path, tmp) < 0)
+goto cleanup;
+
+tmp[0] = '\0';
+}
+
+/* snapshot currently works only for remote disks */
+src->snapshot = virXPathString("string(./snapshot/@name)", ctxt);
+
+/* config file currently only works with remote disks */
+src->configFile = virXPathString("string(./config/@file)", ctxt);
+
+if (virDomainStorageNetworkParseHosts(node, >hosts, >nhosts) < 0)
+goto cleanup;
+
+virStorageSourceNetworkAssignDefaultPorts(src);
+
+ret = 0;
+
+ cleanup:
+VIR_FREE(protocol);
+return ret;
+}
+
+
+int
+virDomainDiskSourceParse(xmlNodePtr node,
+ xmlXPathContextPtr ctxt,
+ virStorageSourcePtr src,
+ unsigned int flags)
+{
+int ret = -1;
+xmlNodePtr saveNode = ctxt->node;

 ctxt->node = node;

@@ -8132,81 +8223,8 @@ virDomainDiskSourceParse(xmlNodePtr node,
 src->path = virXMLPropString(node, "dir");
 break;
 case VIR_STORAGE_TYPE_NETWORK:
-if (!(protocol = virXMLPropString(node, "protocol"))) {
-virReportError(VIR_ERR_XML_ERROR, "%s",
-   _("missing network source protocol type"));
+if 

Re: [libvirt] RFC: Detaching interface from guest fails with improper error message if no mac given in xml

[Michal Privoznik]

On 09/29/2017 09:15 AM, Madhu Pavan wrote:
> This RFC is regarding https://bugzilla.redhat.com/show_bug.cgi?id=1497054
> 
> Let's say we have a network interface
>     
>   
>   
>   
>    function='0x0'/>
>     
> 
> and we are trying to detach the above interface with the following
> xml(vfpool.xml):
>     
>   
>   
>    function='0x0'/>
>     
> Detaching the interface returns error:
> # virsh detach-device vffuest vfpool.xml
> error: Failed to detach device from vfpool.xml
> error: operation failed: no device matching mac address
> 52:54:00:54:f6:61 found
> 
> Here the mac address is auto-generated as we haven't given in the
> vfpool.xml.
> And virDomainNetFindIdx will try to match the auto-generated mac address
> with
> the domain xml. It fails as there will be no match and the error message
> says
> "no device matching mac address 52:54:00:54:f6:61 found".
> 
> Here in this scenario I see two possible improvements.
> 1. As virDomainNetFindIdx search according to mac address and guest side
>    PCI address (if specified), we can search for PCI address and avoid
>    mac address search if mac is not given in the xml. As the PCI address
>    is unique we don't compromise in performance.

Well, since I had discussion about (hot) unplug API recently, I still
remember that it's documented that users are required to pass the full
device XML and not just a portion of it.

> 2. Improve error message by saying mac address is missing in the device
> xml.

But this is an actual bug. We can do better here.

Michal

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

Re: [libvirt] RFC: Detaching interface from guest fails with improper error message if no mac given in xml

[Peter Krempa]

On Fri, Sep 29, 2017 at 14:18:52 +0530, Madhu Pavan wrote:
> 
> 
> On 09/29/2017 01:27 PM, Peter Krempa wrote:
> > On Fri, Sep 29, 2017 at 12:45:30 +0530, Madhu Pavan wrote:
> > > This RFC is regarding https://bugzilla.redhat.com/show_bug.cgi?id=1497054
> > > 
> > > Let's say we have a network interface
> > >      
> > >    
> > >    
> > >    
> > >     > > function='0x0'/>
> > >      
> > > 
> > > and we are trying to detach the above interface with the following
> > > xml(vfpool.xml):
> > >      
> > >    
> > >    
> > >     > > function='0x0'/>
> > >      
> > > Detaching the interface returns error:
> > > # virsh detach-device vffuest vfpool.xml
> > > error: Failed to detach device from vfpool.xml
> > > error: operation failed: no device matching mac address 52:54:00:54:f6:61
> > > found
> > > 
> > > Here the mac address is auto-generated as we haven't given in the
> > > vfpool.xml.
> > > And virDomainNetFindIdx will try to match the auto-generated mac address
> > > with
> > > the domain xml. It fails as there will be no match and the error message
> > > says
> > > "no device matching mac address 52:54:00:54:f6:61 found".
> > > 
> > > Here in this scenario I see two possible improvements.
> > > 1. As virDomainNetFindIdx search according to mac address and guest side
> > >     PCI address (if specified), we can search for PCI address and avoid
> > >     mac address search if mac is not given in the xml. As the PCI address
> > >     is unique we don't compromise in performance.
> > > 2. Improve error message by saying mac address is missing in the device 
> > > xml.
> > I think the problem is that the mac address is always generated in the
> > device XML parser. Auto assignment of the mac address is necessary when
> > the device is added to the VM. When removing, generating the mac address
> > is bogus.
> > 
> > I think we need to make sure that no unique ids are added to the parser
> > at least in the cases when the XML is parsed for the unplug code path.
> I have a patch ready to search according to PCI address in the absence
> of mac address. I can make changes to it so that mac address is not
> auto-generated. Is this good to proceed in this direction?

That patch probably will be necessary as well. The lookup of network
devices tries to compare the mac address as the first thing. Since we've
never had the option for the mac address to be missing you'll need to
fix it.

The problem is that you need to not generate the address since you won't
be able to tell in the qemu code whether the address was present or not.

Also there's the problem that the mac address is always present in the
definition object so you are looking for a substantial refactoring job
to make sure that it can be missing in some cases. Or at least remember
that it was autogenerated.


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

Re: [libvirt] RFC: Detaching interface from guest fails with improper error message if no mac given in xml

[Madhu Pavan]

On 09/29/2017 01:27 PM, Peter Krempa wrote:

On Fri, Sep 29, 2017 at 12:45:30 +0530, Madhu Pavan wrote:

This RFC is regarding https://bugzilla.redhat.com/show_bug.cgi?id=1497054

Let's say we have a network interface
     
   
   
   
   
     

and we are trying to detach the above interface with the following
xml(vfpool.xml):
     
   
   
   
     
Detaching the interface returns error:
# virsh detach-device vffuest vfpool.xml
error: Failed to detach device from vfpool.xml
error: operation failed: no device matching mac address 52:54:00:54:f6:61
found

Here the mac address is auto-generated as we haven't given in the
vfpool.xml.
And virDomainNetFindIdx will try to match the auto-generated mac address
with
the domain xml. It fails as there will be no match and the error message
says
"no device matching mac address 52:54:00:54:f6:61 found".

Here in this scenario I see two possible improvements.
1. As virDomainNetFindIdx search according to mac address and guest side
    PCI address (if specified), we can search for PCI address and avoid
    mac address search if mac is not given in the xml. As the PCI address
    is unique we don't compromise in performance.
2. Improve error message by saying mac address is missing in the device xml.

I think the problem is that the mac address is always generated in the
device XML parser. Auto assignment of the mac address is necessary when
the device is added to the VM. When removing, generating the mac address
is bogus.

I think we need to make sure that no unique ids are added to the parser
at least in the cases when the XML is parsed for the unplug code path.

I have a patch ready to search according to PCI address in the absence
of mac address. I can make changes to it so that mac address is not
auto-generated. Is this good to proceed in this direction?

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

Re: [libvirt] gnulib tests in libvirt broken by newer glibc 2.26

[Daniel P. Berrange]

On Thu, Sep 28, 2017 at 04:41:37PM -0700, Paul Eggert wrote:
> That patch essentially negates the point of the test, which is that getopt
> should be visible from unistd.h. I'd rather fix the problem than nuke the
> test.
> 
> Could you explain what the Gnulib problem is here? I can't really see it in
> your email. A self-contained example would help.
> 
> For what it's worth, I could not reproduce the problem on Fedora 26 by doing
> this in Gnulib (this tells 'configure' to use Gnulib-supplied getopt.h and
> getopt.c):

Fedora 26 only has glibc 2.25 - you need to have Fedora rawhide to get
the broken behaviour, as that has glibc 2.26.90

> ./gnulib-tool --create-testdir --dir foo getopt-posix
> cd foo
> ./configure gl_cv_func_getopt_posix=no ac_cv_header_getopt_h=no
> make
> make check

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

Re: [libvirt] RFC: Detaching interface from guest fails with improper error message if no mac given in xml

[Peter Krempa]

On Fri, Sep 29, 2017 at 12:45:30 +0530, Madhu Pavan wrote:
> This RFC is regarding https://bugzilla.redhat.com/show_bug.cgi?id=1497054
> 
> Let's say we have a network interface
>     
>   
>   
>   
>    function='0x0'/>
>     
> 
> and we are trying to detach the above interface with the following
> xml(vfpool.xml):
>     
>   
>   
>    function='0x0'/>
>     
> Detaching the interface returns error:
> # virsh detach-device vffuest vfpool.xml
> error: Failed to detach device from vfpool.xml
> error: operation failed: no device matching mac address 52:54:00:54:f6:61
> found
> 
> Here the mac address is auto-generated as we haven't given in the
> vfpool.xml.
> And virDomainNetFindIdx will try to match the auto-generated mac address
> with
> the domain xml. It fails as there will be no match and the error message
> says
> "no device matching mac address 52:54:00:54:f6:61 found".
> 
> Here in this scenario I see two possible improvements.
> 1. As virDomainNetFindIdx search according to mac address and guest side
>    PCI address (if specified), we can search for PCI address and avoid
>    mac address search if mac is not given in the xml. As the PCI address
>    is unique we don't compromise in performance.
> 2. Improve error message by saying mac address is missing in the device xml.

I think the problem is that the mac address is always generated in the
device XML parser. Auto assignment of the mac address is necessary when
the device is added to the VM. When removing, generating the mac address
is bogus.

I think we need to make sure that no unique ids are added to the parser
at least in the cases when the XML is parsed for the unplug code path.


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

[libvirt] Entering freeze for 3.8.0

[Daniel Veillard]

  Done, I have tagged RC1 in git, pushed signed tarball and rpms at the usual
location:

   ftp://libvirt.org/libvirt/


  Seems to work fine in my limited testing, I had a keyboard issue in my XP
guest but that's most likely unrelated. https://ci.centos.org/view/libvirt/
is mostly green with somehow libvirt-master-test failing recently, so this
seems good on that side too.
  Please give it more testing however, especially on different OS or
systems.

  Plan is to have an RC2 possibly over the week-end, and if things looks fine
a release on Tuesday,

   thanks,

Daniel

-- 
Daniel Veillard  | Red Hat Developers Tools http://developer.redhat.com/
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] Exposing mem-path in domain XML

[Michal Privoznik]

On 09/26/2017 12:00 AM, Zack Cornelius wrote:
> - Original Message -
>> From: "Michal Privoznik" 
>> To: "Zack Cornelius" 
>> Cc: "Daniel P. Berrange" , "libvir-list" 
>> 
>> Sent: Monday, September 25, 2017 9:17:10 AM
>> Subject: Re: [libvirt] Exposing mem-path in domain XML
> 
>> On 09/15/2017 03:49 PM, Zack Cornelius wrote:
>>>
> 
> Kove would only be using our integration with domains using the file
> memorybacking via the following XML, which I think simplifies the
> cases where the memory-backend-file gets used.
> 
>  
>

Re: [libvirt] [PATCH alt] conf: Allow user define their own alias

[Peter Krempa]

On Fri, Sep 29, 2017 at 09:06:01 +0200, Michal Privoznik wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1434451
> 
> It comes handy for management application to be able to have a
> per-device label so that it can uniquely identify devices it
> cares about. The advantage of this approach is that we don't have
> to generate aliases at define time (non trivial amount of work
> and problems). The only thing we do is parse the user supplied
> tag and format it back. For instance:
> 
> 
>   
>   
>   
>   
>   
> 
> 
> Signed-off-by: Michal Privoznik 
> ---
> 
> An alternative approach to:
> 
> https://www.redhat.com/archives/libvir-list/2017-September/msg00765.html
> 
> Honestly, I prefer this one as it's simpler and we don't have to care about
> devices changing their aliases on cold plug. I mean, on cold (un-)plug we'd
> have to regenerate the aliases so it might be hard to track certain device.
> But with this approach, it's no problem.
> 
> Also, I'm not completely convinced about the name of @user attribute. So I'm
> open for suggestions.

With this proposed design you need to make sure to document that the
user alias is _not_ guaranteed to be unique and also that it can't be
used to match the device in any way.

I think that users which know semantics of the current alias may be very
confused by putting user data with different semantics into the same
field.



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

[libvirt] [PATCH go-xml] Add support for memory device element

[zhenwei.pi]

Support model, access and target.
Add Marshal/Unmarshal mothed for memory device.
Add test code for device list in full domain.

Signed-off-by: zhenwei.pi 
---
 domain.go  | 29 +
 domain_test.go | 45 +
 2 files changed, 74 insertions(+)

diff --git a/domain.go b/domain.go
index 8c2cc1b..bacab11 100644
--- a/domain.go
+++ b/domain.go
@@ -436,6 +436,22 @@ type DomainHostdev struct {
Address *DomainAddress   `xml:"address"`
 }
 
+type DomainMemorydevTargetNode struct {
+   Value uint `xml:",chardata"`
+}
+
+type DomainMemorydevTarget struct {
+   Size *DomainMemory  `xml:"size"`
+   Node *DomainMemorydevTargetNode `xml:"node"`
+}
+
+type DomainMemorydev struct {
+   XMLName xml.Name   `xml:"memory"`
+   Model   string `xml:"model,attr"`
+   Access  string `xml:"access,attr"`
+   Target  *DomainMemorydevTarget `xml:"target"`
+}
+
 type DomainDeviceList struct {
Emulatorstring `xml:"emulator,omitempty"`
Controllers []DomainController `xml:"controller"`
@@ -452,6 +468,7 @@ type DomainDeviceList struct {
Sounds  []DomainSound  `xml:"sound"`
RNGs[]DomainRNG`xml:"rng"`
Hostdevs[]DomainHostdev`xml:"hostdev"`
+   Memorydevs  []DomainMemorydev  `xml:"memory"`
 }
 
 type DomainMemory struct {
@@ -915,6 +932,18 @@ func (d *DomainHostdev) Marshal() (string, error) {
return string(doc), nil
 }
 
+func (d *DomainMemorydev) Unmarshal(doc string) error {
+   return xml.Unmarshal([]byte(doc), d)
+}
+
+func (d *DomainMemorydev) Marshal() (string, error) {
+   doc, err := xml.MarshalIndent(d, "", "  ")
+   if err != nil {
+   return "", err
+   }
+   return string(doc), nil
+}
+
 type HexUint uint
 
 func (h *HexUint) UnmarshalXMLAttr(attr xml.Attr) error {
diff --git a/domain_test.go b/domain_test.go
index 4fe6bfe..dbebe42 100644
--- a/domain_test.go
+++ b/domain_test.go
@@ -372,6 +372,21 @@ var domainTestData = []struct {
},
},
},
+   Memorydevs: []DomainMemorydev{
+   DomainMemorydev{
+   Model:  "dimm",
+   Access: "private",
+   Target: {
+   Size: {
+   Value: 1,
+   Unit:  "GiB",
+   },
+   Node: 
{
+   Value: 0,
+   },
+   },
+   },
+   },
},
},
Expected: []string{
@@ -414,6 +429,12 @@ var domainTestData = []struct {
``,
`  `,
``,
+   ``,
+   `  `,
+   `1`,
+   `0`,
+   `  `,
+   ``,
`  `,
``,
},
@@ -1630,6 +1651,30 @@ var domainTestData = []struct {
``,
},
},
+   {
+   Object: {
+   Model:  "dimm",
+   Access: "private",
+   Target: {
+   Size: {
+   Value: 1,
+   Unit:  "GiB",
+   },
+   Node: {
+   Value: 0,
+   },
+   },
+   },
+
+   Expected: []string{
+   ``,
+   `  `,
+   `1`,
+   `0`,
+   `  `,
+   ``,
+   },
+   },
 }
 
 func TestDomain(t *testing.T) {
-- 
2.7.4

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

[libvirt] RFC: Detaching interface from guest fails with improper error message if no mac given in xml

[Madhu Pavan]

This RFC is regarding https://bugzilla.redhat.com/show_bug.cgi?id=1497054

Let's say we have a network interface
    
  
  
  
  function='0x0'/>

    

and we are trying to detach the above interface with the following 
xml(vfpool.xml):

    
  
  
  function='0x0'/>

    
Detaching the interface returns error:
# virsh detach-device vffuest vfpool.xml
error: Failed to detach device from vfpool.xml
error: operation failed: no device matching mac address 
52:54:00:54:f6:61 found


Here the mac address is auto-generated as we haven't given in the 
vfpool.xml.
And virDomainNetFindIdx will try to match the auto-generated mac address 
with
the domain xml. It fails as there will be no match and the error message 
says

"no device matching mac address 52:54:00:54:f6:61 found".

Here in this scenario I see two possible improvements.
1. As virDomainNetFindIdx search according to mac address and guest side
   PCI address (if specified), we can search for PCI address and avoid
   mac address search if mac is not given in the xml. As the PCI address
   is unique we don't compromise in performance.
2. Improve error message by saying mac address is missing in the device xml.

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

[libvirt] [PATCH alt] conf: Allow user define their own alias

[Michal Privoznik]

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

It comes handy for management application to be able to have a
per-device label so that it can uniquely identify devices it
cares about. The advantage of this approach is that we don't have
to generate aliases at define time (non trivial amount of work
and problems). The only thing we do is parse the user supplied
tag and format it back. For instance:


  
  
  
  
  


Signed-off-by: Michal Privoznik 
---

An alternative approach to:

https://www.redhat.com/archives/libvir-list/2017-September/msg00765.html

Honestly, I prefer this one as it's simpler and we don't have to care about
devices changing their aliases on cold plug. I mean, on cold (un-)plug we'd
have to regenerate the aliases so it might be hard to track certain device.
But with this approach, it's no problem.

Also, I'm not completely convinced about the name of @user attribute. So I'm
open for suggestions.

 docs/schemas/domaincommon.rng | 13 +++---
 src/conf/device_conf.c|  1 +
 src/conf/device_conf.h|  1 +
 src/conf/domain_conf.c| 20 ++-
 tests/genericxml2xmlindata/generic-user-alias.xml | 31 +++
 tests/genericxml2xmltest.c|  2 ++
 6 files changed, 59 insertions(+), 9 deletions(-)
 create mode 100644 tests/genericxml2xmlindata/generic-user-alias.xml

diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index bac371ea3..69c121ce9 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -5864,9 +5864,16 @@
   
   
 
-  
-
-  
+  
+
+  
+
+  
+  
+
+  
+
+  
 
 
   
diff --git a/src/conf/device_conf.c b/src/conf/device_conf.c
index d69f94fad..ced5db123 100644
--- a/src/conf/device_conf.c
+++ b/src/conf/device_conf.c
@@ -57,6 +57,7 @@ void
 virDomainDeviceInfoClear(virDomainDeviceInfoPtr info)
 {
 VIR_FREE(info->alias);
+VIR_FREE(info->user);
 memset(>addr, 0, sizeof(info->addr));
 info->type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE;
 VIR_FREE(info->romfile);
diff --git a/src/conf/device_conf.h b/src/conf/device_conf.h
index f87d6f1fc..08a9e57e3 100644
--- a/src/conf/device_conf.h
+++ b/src/conf/device_conf.h
@@ -135,6 +135,7 @@ typedef struct _virDomainDeviceInfo virDomainDeviceInfo;
 typedef virDomainDeviceInfo *virDomainDeviceInfoPtr;
 struct _virDomainDeviceInfo {
 char *alias;
+char *user; /* user defined ID for the device */
 int type; /* virDomainDeviceAddressType */
 union {
 virPCIDeviceAddress pci;
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 87192eb2d..885825226 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -5756,6 +5756,8 @@ virDomainDeviceInfoFormat(virBufferPtr buf,
   virDomainDeviceInfoPtr info,
   unsigned int flags)
 {
+bool formatAlias = info->alias && !(flags & 
VIR_DOMAIN_DEF_FORMAT_INACTIVE);
+
 if ((flags & VIR_DOMAIN_DEF_FORMAT_ALLOW_BOOT) && info->bootIndex) {
 virBufferAsprintf(buf, "bootIndex);
 
@@ -5764,9 +5766,13 @@ virDomainDeviceInfoFormat(virBufferPtr buf,
 
 virBufferAddLit(buf, "/>\n");
 }
-if (info->alias &&
-!(flags & VIR_DOMAIN_DEF_FORMAT_INACTIVE)) {
-virBufferAsprintf(buf, "\n", info->alias);
+if (formatAlias || info->user) {
+virBufferAddLit(buf, "alias);
+if (info->user)
+virBufferAsprintf(buf, " user='%s'", info->user);
+virBufferAddLit(buf, "/>\n");
 }
 
 if (info->mastertype == VIR_DOMAIN_CONTROLLER_MASTER_USB) {
@@ -6327,7 +6333,6 @@ virDomainDeviceInfoParseXML(xmlNodePtr node,
 while (cur != NULL) {
 if (cur->type == XML_ELEMENT_NODE) {
 if (alias == NULL &&
-!(flags & VIR_DOMAIN_DEF_PARSE_INACTIVE) &&
 virXMLNodeNameEqual(cur, "alias")) {
 alias = cur;
 } else if (address == NULL &&
@@ -6349,8 +6354,11 @@ virDomainDeviceInfoParseXML(xmlNodePtr node,
 cur = cur->next;
 }
 
-if (alias)
-info->alias = virXMLPropString(alias, "name");
+if (alias) {
+if (!(flags & VIR_DOMAIN_DEF_PARSE_INACTIVE))
+info->alias = virXMLPropString(alias, "name");
+info->user = virXMLPropString(alias, "user");
+}
 
 if (master) {
 info->mastertype = VIR_DOMAIN_CONTROLLER_MASTER_USB;
diff --git a/tests/genericxml2xmlindata/generic-user-alias.xml 
b/tests/genericxml2xmlindata/generic-user-alias.xml
new file mode 100644
index 0..025924442
--- /dev/null
+++ b/tests/genericxml2xmlindata/generic-user-alias.xml
@@ -0,0 +1,31 @@
+
+  QEMUGuest1
+