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

2017-09-28 Thread Paul Eggert 
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):


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

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

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

2017-09-28 Thread Nir Soffer 
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(-)

diff --git a/libvirt-override.c b/libvirt-override.c
index f5ff605..d746350 100644
--- a/libvirt-override.c
+++ b/libvirt-override.c
@@ -441,6 +441,7 @@ libvirt_virDomainGetSchedulerType(PyObject *self 
ATTRIBUTE_UNUSED,
 LIBVIRT_BEGIN_ALLOW_THREADS;
 c_retval = virDomainGetSchedulerType(domain, );
 LIBVIRT_END_ALLOW_THREADS;
+
 if (c_retval == NULL)
 return VIR_PY_NONE;
 
@@ -1221,6 +1222,7 @@ libvirt_virDomainGetVcpus(PyObject *self ATTRIBUTE_UNUSED,
 LIBVIRT_BEGIN_ALLOW_THREADS;
 i_retval = virDomainGetInfo(domain, );
 LIBVIRT_END_ALLOW_THREADS;
+
 if (i_retval < 0)
 return VIR_PY_NONE;
 
@@ -1239,6 +1241,7 @@ libvirt_virDomainGetVcpus(PyObject *self ATTRIBUTE_UNUSED,
  cpuinfo, dominfo.nrVirtCpu,
  cpumap, cpumaplen);
 LIBVIRT_END_ALLOW_THREADS;
+
 if (i_retval < 0) {
 error = VIR_PY_NONE;
 goto cleanup;
@@ -1402,6 +1405,7 @@ libvirt_virDomainGetVcpuPinInfo(PyObject *self 
ATTRIBUTE_UNUSED,
 LIBVIRT_BEGIN_ALLOW_THREADS;
 i_retval = virDomainGetInfo(domain, );
 LIBVIRT_END_ALLOW_THREADS;
+
 if (i_retval < 0)
 return VIR_PY_NONE;
 
@@ -1522,6 +1526,7 @@ libvirt_virDomainGetEmulatorPinInfo(PyObject *self 
ATTRIBUTE_UNUSED,
 LIBVIRT_BEGIN_ALLOW_THREADS;
 ret = virDomainGetEmulatorPinInfo(domain, cpumap, cpumaplen, flags);
 LIBVIRT_END_ALLOW_THREADS;
+
 if (ret < 0) {
 VIR_FREE(cpumap);
 return VIR_PY_NONE;
@@ -1660,6 +1665,7 @@ libvirt_virDomainPinIOThread(PyObject *self 
ATTRIBUTE_UNUSED,
 i_retval = virDomainPinIOThread(domain, iothread_val,
 cpumap, cpumaplen, flags);
 LIBVIRT_END_ALLOW_THREADS;
+
 if (i_retval < 0) {
 ret = VIR_PY_INT_FAIL;
 goto cleanup;
@@ -1728,6 +1734,7 @@ libvirt_virConnGetLastError(PyObject *self 
ATTRIBUTE_UNUSED,
 LIBVIRT_BEGIN_ALLOW_THREADS;
 err = virConnGetLastError(conn);
 LIBVIRT_END_ALLOW_THREADS;
+
 if (err == NULL)
 return VIR_PY_NONE;
 
@@ -1970,9 +1977,9 @@ libvirt_virConnectOpenAuth(PyObject *self 
ATTRIBUTE_UNUSED,
 auth.cbdata = pyauth;
 
 LIBVIRT_BEGIN_ALLOW_THREADS;
-
 c_retval = virConnectOpenAuth(name, , flags);
 LIBVIRT_END_ALLOW_THREADS;
+
 VIR_FREE(auth.credtype);
 py_retval = libvirt_virConnectPtrWrap((virConnectPtr) c_retval);
 return py_retval;
@@ -1997,12 +2004,10 @@ libvirt_virGetVersion(PyObject *self ATTRIBUTE_UNUSED,
 return NULL;
 
 LIBVIRT_BEGIN_ALLOW_THREADS;
-
 if (type == NULL)
 c_retval = virGetVersion(, NULL, NULL);
 else
 c_retval = virGetVersion(, type, );
-
 LIBVIRT_END_ALLOW_THREADS;
 
 if (c_retval == -1)
@@ -2029,9 +2034,7 @@ libvirt_virConnectGetVersion(PyObject *self 
ATTRIBUTE_UNUSED,
 conn = (virConnectPtr) PyvirConnect_Get(pyobj_conn);
 
 LIBVIRT_BEGIN_ALLOW_THREADS;
-
 c_retval = virConnectGetVersion(conn, );
-
 LIBVIRT_END_ALLOW_THREADS;
 
 if (c_retval == -1)
@@ -2059,9 +2062,7 @@ libvirt_virConnectGetCPUModelNames(PyObject *self 
ATTRIBUTE_UNUSED,
 conn = (virConnectPtr) PyvirConnect_Get(pyobj_conn);
 
 LIBVIRT_BEGIN_ALLOW_THREADS;
-
 c_retval = virConnectGetCPUModelNames(conn, arch, , flags);
-
 LIBVIRT_END_ALLOW_THREADS;
 
 if (c_retval == -1)
@@ -2103,9 +2104,7 @@ libvirt_virConnectGetLibVersion(PyObject *self 
ATTRIBUTE_UNUSED,
 conn = (virConnectPtr) PyvirConnect_Get(pyobj_conn);
 
 LIBVIRT_BEGIN_ALLOW_THREADS;
-
 c_retval = virConnectGetLibVersion(conn, );
-
 LIBVIRT_END_ALLOW_THREADS;
 
 if (c_retval == -1)
@@ -2132,6 +2131,7 @@ libvirt_virConnectListDomainsID(PyObject *self 
ATTRIBUTE_UNUSED,
 LIBVIRT_BEGIN_ALLOW_THREADS;
 c_retval = virConnectNumOfDomains(conn);
 LIBVIRT_END_ALLOW_THREADS;
+
 if (c_retval < 0)
 return VIR_PY_NONE;
 
@@ -2142,6 +2142,7 @@ libvirt_virConnectListDomainsID(PyObject *self 
ATTRIBUTE_UNUSED,
 LIBVIRT_BEGIN_ALLOW_THREADS;
 c_retval = virConnectListDomains(conn, ids, c_retval);
 LIBVIRT_END_ALLOW_THREADS;
+
 if (c_retval < 0) {
 py_retval = VIR_PY_NONE;
 goto cleanup;
@@ -2186,6 +2187,7 @@ libvirt_virConnectListAllDomains(PyObject *self 
ATTRIBUTE_UNUSED,
 LIBVIRT_BEGIN_ALLOW_THREADS;
 c_retval = virConnectListAllDomains(conn, , flags);
 LIBVIRT_END_ALLOW_THREADS;
+
 if (c_retval < 0)
 return VIR_PY_NONE;
 
@@ -2231,15 +2233,18 @@ libvirt_virConnectListDefinedDomains(PyObject *self

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

2017-09-28 Thread John Ferlan 
Silly me - touching the nwfilter code... Turns out the reverted patch
in 1/2 doesn't cover it as virNWFilterVarValueCreateSimple actually
steals @addr, so freeing it on successful return ends up being a very
bad thing as I found out with the nwfilter avocado-vt tests.

So revert that and just go with the only free on failure logic in patch2

John Ferlan (2):
  Revert "nwfilter: Fix possible segfault on sometimes consumed
variable"
  nwfilter: Only free inetaddr on virNWFilterIPAddrMapAddIPAddr failure

 src/conf/nwfilter_ipaddrmap.c   | 9 +
 src/nwfilter/nwfilter_learnipaddr.c | 5 -
 2 files changed, 5 insertions(+), 9 deletions(-)

-- 
2.13.5

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

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

2017-09-28 Thread 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

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

2017-09-28 Thread John Ferlan 
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;
 
 /* 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;
 }
 
 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);
-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

Re: [libvirt] Plans for next release

2017-09-28 Thread John Ferlan 


On 09/28/2017 01:01 PM, Daniel Veillard wrote:
> On Thu, Sep 28, 2017 at 03:15:10PM +0200, Peter Krempa wrote:
>> On Thu, Sep 28, 2017 at 12:17:41 +0200, Daniel Veillard wrote:
>>>  I'm a bit late, again ...
>>>
>>> I think we should enter freeze late today, then I can make an rc2 this week
>>> end and shoot for a release on the 3rd
>>>
>>>   I hope there is no issue with this,
>>
>> Please wait until John pushes the remainder of the VxHS series which I'm
>> currently reviewing, so that also the TLS parts make it in.
> 
>   Okay, ping me when you are done
> 
> Daniel
> 

Guess I assumed the ping would be ping me on internal IRC... In any
case, the VxHS series was pushed already - so freeze away.


John

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

[libvirt] [PATCH] Fix vxhs test to have stable certificate dir

2017-09-28 Thread Daniel P. Berrange 
The test suite has hardcoded /etc/pki/qemu as the cert dir, but this
only works if configure has --sysconfdir=/etc passed. We must set the
vxhs cert dir to a stable path in the test suite.

Signed-off-by: Daniel P. Berrange 
---

Pushed as a build fix

 .../qemuxml2argv-disk-drive-network-tlsx509-vxhs.args | 4 ++--
 tests/qemuxml2argvtest.c  | 3 +++
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git 
a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-tlsx509-vxhs.args 
b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-tlsx509-vxhs.args
index 572c9f36c..a75272454 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-tlsx509-vxhs.args
+++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-tlsx509-vxhs.args
@@ -20,7 +20,7 @@ server,nowait \
 -no-acpi \
 -boot c \
 -usb \
--object tls-creds-x509,id=objvirtio-disk0_tls0,dir=/etc/pki/qemu,\
+-object tls-creds-x509,id=objvirtio-disk0_tls0,dir=/etc/pki/libvirt-vxhs,\
 endpoint=client,verify-peer=yes \
 -drive file.driver=vxhs,file.tls-creds=objvirtio-disk0_tls0,\
 file.vdisk-id=eb90327c-8302-4725-9e1b-4e85ed4dc251,file.server.type=tcp,\
@@ -28,7 +28,7 @@ 
file.server.host=192.168.0.1,file.server.port=,format=raw,if=none,\
 id=drive-virtio-disk0,cache=none \
 -device virtio-blk-pci,bus=pci.0,addr=0x4,drive=drive-virtio-disk0,\
 id=virtio-disk0 \
--object tls-creds-x509,id=objvirtio-disk1_tls0,dir=/etc/pki/qemu,\
+-object tls-creds-x509,id=objvirtio-disk1_tls0,dir=/etc/pki/libvirt-vxhs,\
 endpoint=client,verify-peer=yes \
 -drive file.driver=vxhs,file.tls-creds=objvirtio-disk1_tls0,\
 file.vdisk-id=eb90327c-8302-4725-9e1b-4e85ed4dc252,file.server.type=tcp,\
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 1958ad428..7271ea07e 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -599,6 +599,9 @@ mymain(void)
 VIR_FREE(driver.config->chardevTLSx509certdir);
 if (VIR_STRDUP_QUIET(driver.config->chardevTLSx509certdir, 
"/etc/pki/libvirt-chardev") < 0)
 return EXIT_FAILURE;
+VIR_FREE(driver.config->vxhsTLSx509certdir);
+if (VIR_STRDUP_QUIET(driver.config->vxhsTLSx509certdir, 
"/etc/pki/libvirt-vxhs") < 0)
+return EXIT_FAILURE;
 
 VIR_FREE(driver.config->hugetlbfs);
 if (VIR_ALLOC_N(driver.config->hugetlbfs, 2) < 0)
-- 
2.13.5

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

Re: [libvirt] Plans for next release

2017-09-28 Thread Daniel Veillard 
On Thu, Sep 28, 2017 at 03:15:10PM +0200, Peter Krempa wrote:
> On Thu, Sep 28, 2017 at 12:17:41 +0200, Daniel Veillard wrote:
> >  I'm a bit late, again ...
> > 
> > I think we should enter freeze late today, then I can make an rc2 this week
> > end and shoot for a release on the 3rd
> > 
> >   I hope there is no issue with this,
> 
> Please wait until John pushes the remainder of the VxHS series which I'm
> currently reviewing, so that also the TLS parts make it in.

  Okay, ping me when you are done

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] [PATCH v10 0/5] Add support for Veritas HyperScale (VxHS) block device protocol

2017-09-28 Thread ashish mittal 
>
> The series is now pushed without any adjustment from the patch3 followup
> (Peter and I hashed it out over the much faster internal IRC channel -
> mail list delivery is still brutally slow/behind).
>
> Tks -
>
> John
>
>
Thank you, John! Couldn't have done it without your help!
And thanks to the reviewers for showing me how it's done :)

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

Re: [libvirt] [PATCH] docs, rng: Allow a pool name to be line domain name

2017-09-28 Thread Peter Krempa 
On Thu, Sep 28, 2017 at 10:03:55 -0400, John Ferlan wrote:
> 
> 
> On 09/28/2017 09:47 AM, Peter Krempa wrote:
> > On Wed, Sep 27, 2017 at 15:07:35 -0400, John Ferlan wrote:
> >> https://bugzilla.redhat.com/show_bug.cgi?id=1475250
> >>
> >> It's possible to define and start a pool with a '.' in the
> >> name; however, when trying to add a volume to a domain using
> >> the storage pool source with a name with a '.' in the name,
> >> the domain RNG validation fails because RNG uses 'genericName'
> >> which does not allow a '.' in the name. Pool definition has
> >> no similar call to virXMLValidateAgainstSchema. Pool name
> >> validation occurs in storagePoolDefineXML and only calls
> >> virXMLCheckIllegalChars using the same parameter "\n" as
> >> qemuDomainDefineXMLFlags would check after the RNG check
> >> could be succesful.
> >>
> >> So in order to resolve this, create a poolName definition
> >> in the RNG and allow the pool name and the volume source
> >> pool name to use that definition.
> >>
> >> Signed-off-by: John Ferlan 
> >> ---
> >>  docs/schemas/domaincommon.rng  | 2 +-
> >>  docs/schemas/storagecommon.rng | 8 
> >>  docs/schemas/storagepool.rng   | 4 ++--
> >>  3 files changed, 11 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> >> index 76852abb3..2cc8dcecf 100644
> >> --- a/docs/schemas/domaincommon.rng
> >> +++ b/docs/schemas/domaincommon.rng
> >> @@ -1669,7 +1669,7 @@
> >>  
> >>
> >>  
> >> -  
> >> +  
> >>  
> >>  
> >>
> >> diff --git a/docs/schemas/storagecommon.rng 
> >> b/docs/schemas/storagecommon.rng
> >> index 717f3c603..49578312e 100644
> >> --- a/docs/schemas/storagecommon.rng
> >> +++ b/docs/schemas/storagecommon.rng
> >> @@ -6,6 +6,14 @@
> >>
> >>  
> >> +  
> >> +
> >> +  
> >> +  [^
> >> +]+
> >> +
> >> +  
> >> +
> >>
> >>  
> >>
> >> diff --git a/docs/schemas/storagepool.rng b/docs/schemas/storagepool.rng
> >> index f0117bd69..52b2044be 100644
> >> --- a/docs/schemas/storagepool.rng
> >> +++ b/docs/schemas/storagepool.rng
> >> @@ -209,7 +209,7 @@
> >>  
> >>
> >>  
> >> -  
> >> +  
> > 
> > This means that a name starting with a dot is invalid according to the
> > schema, but the user ignored the schema and the code is not doing enough
> > validation.
> > 
> > I'm not convinced that this is the correct solution. VMs disallow dots
> > since the name is used for generating filenames and using '../' as
> > prefix will allow directory traversal exploits.
> > 
> > NACK, I think we should disallow pool names with a dot even in the code.
> > It will be slightly harder since there are no 'validate' callbacks for
> > them  and you can't disallow them in the parser.
> > 
> 
> As a test I defined a domain and a pool using ".test" and it worked.
> 
> # virsh list
>  IdName   State
> 
>  1 .f23   running
> 
> # virsh dumpxml .f23
> 
>   .f23
>   e28761fe-ea53-4682-924a-744a4028f146
>   2097152
> ...
> 
> 
> The problem with disallowing in code after the fact is how do you handle
> things now that we have allowed it?  I have another long standing bug
> where someone doesn't want a space in the name or even worse a space as
> the name.

Fair enough, in fact qemu driver (and others which rely on files) forbid
usage of '/' in the name. In such case we can allow a '.' or whatever
else, so that it's not considered as a directory.

In case of the storage driver I think we can ban '/' accross all drivers
and also in the parser, since storage pool containing an '/' probably
would vanish after VM restart and/or fail to be created for other
reasons than validation.

Anyways, you need to change the validation regex and add the check to
reject slashes.


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

Re: [libvirt] [PATCH v10 0/5] Add support for Veritas HyperScale (VxHS) block device protocol

2017-09-28 Thread John Ferlan 


On 09/27/2017 11:45 AM, John Ferlan wrote:
> v9: https://www.redhat.com/archives/libvir-list/2017-September/msg00641.html
> 
> Differences to v9:
> 
>  * Patch 1:
>   - Clean up the wording from code review
> 
>  * Patch 2: (NEW)
>   - Split out the formatting change for source/prototcol
> 
>  * Patch 3:
>   - Add the parsing of the tlsFromConfig for storage source - this
> just follows what chardev has done.
> 
>  * Patch 4:
>   - Remove the src->tlsListen
> 
>  * Patch 5:
>   - Remove the validation (from code review)
>   - Remove the comments in qemuDomainAddDiskSrcTLSObject
>   - Merge the -multi- test into the existing test - it was
> easier than removing the existing and the using -multi-
> 
> Ashish Mittal (3):
>   conf: Introduce TLS options for VxHS block device clients
>   util: Add TLS attributes to virStorageSource
>   qemu: Add TLS support for Veritas HyperScale (VxHS)
> 
> John Ferlan (2):
>   docs: Clean up the description for network disk protocol options
>   qemu: Introduce qemuDomainPrepareDiskSource
> 
>  docs/formatdomain.html.in  | 40 +---
>  docs/schemas/domaincommon.rng  |  5 ++
>  src/conf/domain_conf.c | 64 +++
>  src/conf/domain_conf.h |  3 +-
>  src/conf/snapshot_conf.c   |  7 ++-
>  src/qemu/libvirtd_qemu.aug |  4 ++
>  src/qemu/qemu.conf | 34 ++
>  src/qemu/qemu_block.c  |  2 +
>  src/qemu/qemu_command.c| 33 ++
>  src/qemu/qemu_conf.c   | 16 +
>  src/qemu/qemu_conf.h   |  3 +
>  src/qemu/qemu_domain.c | 73 
> ++
>  src/qemu/qemu_domain.h | 11 
>  src/qemu/qemu_hotplug.c| 73 
> ++
>  src/qemu/qemu_process.c|  4 ++
>  src/qemu/test_libvirtd_qemu.aug.in |  2 +
>  src/util/virstoragefile.c  | 10 ++-
>  src/util/virstoragefile.h  | 14 +
>  ...muxml2argv-disk-drive-network-tlsx509-vxhs.args | 43 +
>  ...emuxml2argv-disk-drive-network-tlsx509-vxhs.xml | 50 +++
>  tests/qemuxml2argvtest.c   |  5 ++
>  ...uxml2xmlout-disk-drive-network-tlsx509-vxhs.xml | 52 +++
>  tests/qemuxml2xmltest.c|  1 +
>  23 files changed, 523 insertions(+), 26 deletions(-)
>  create mode 100644 
> tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-tlsx509-vxhs.args
>  create mode 100644 
> tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-tlsx509-vxhs.xml
>  create mode 100644 
> tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-drive-network-tlsx509-vxhs.xml
> 

The series is now pushed without any adjustment from the patch3 followup
(Peter and I hashed it out over the much faster internal IRC channel -
mail list delivery is still brutally slow/behind).

Tks -

John

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

Re: [libvirt] [PATCH v10 5/5] qemu: Add TLS support for Veritas HyperScale (VxHS)

2017-09-28 Thread Peter Krempa 
On Wed, Sep 27, 2017 at 11:45:55 -0400, John Ferlan wrote:
> From: Ashish Mittal 
> 
> Alter qemu command line generation in order to possibly add TLS for
> a suitably configured domain.
> 
> Sample TLS args generated by libvirt -
> 
> -object tls-creds-x509,id=objvirtio-disk0_tls0,dir=/etc/pki/qemu,\
> endpoint=client,verify-peer=yes \
> -drive file.driver=vxhs,file.tls-creds=objvirtio-disk0_tls0,\
> file.vdisk-id=eb90327c-8302-4725-9e1b-4e85ed4dc251,\
> file.server.type=tcp,file.server.host=192.168.0.1,\
> file.server.port=,format=raw,if=none,\
> id=drive-virtio-disk0,cache=none \
> -device virtio-blk-pci,bus=pci.0,addr=0x4,drive=drive-virtio-disk0,\
> id=virtio-disk0
> 
> Update the qemuxml2argvtest with a couple of examples. One for a
> simple case and the other a bit more complex where multiple VxHS disks
> are added where at least one uses a VxHS that doesn't require TLS
> credentials and thus sets the domain disk source attribute "tls = 'no'".
> 
> Update the hotplug to be able to handle processing the tlsAlias whether
> it's to add the TLS object when hotplugging a disk or to remove the TLS
> object when hot unplugging a disk.  The hot plug/unplug code is largely
> generic, but the addition code does make the VXHS specific checks only
> because it needs to grab the correct config directory and generate the
> object as the command line would do.

ACK


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

Re: [libvirt] [PATCH] docs, rng: Allow a pool name to be line domain name

2017-09-28 Thread John Ferlan 


On 09/28/2017 09:47 AM, Peter Krempa wrote:
> On Wed, Sep 27, 2017 at 15:07:35 -0400, John Ferlan wrote:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1475250
>>
>> It's possible to define and start a pool with a '.' in the
>> name; however, when trying to add a volume to a domain using
>> the storage pool source with a name with a '.' in the name,
>> the domain RNG validation fails because RNG uses 'genericName'
>> which does not allow a '.' in the name. Pool definition has
>> no similar call to virXMLValidateAgainstSchema. Pool name
>> validation occurs in storagePoolDefineXML and only calls
>> virXMLCheckIllegalChars using the same parameter "\n" as
>> qemuDomainDefineXMLFlags would check after the RNG check
>> could be succesful.
>>
>> So in order to resolve this, create a poolName definition
>> in the RNG and allow the pool name and the volume source
>> pool name to use that definition.
>>
>> Signed-off-by: John Ferlan 
>> ---
>>  docs/schemas/domaincommon.rng  | 2 +-
>>  docs/schemas/storagecommon.rng | 8 
>>  docs/schemas/storagepool.rng   | 4 ++--
>>  3 files changed, 11 insertions(+), 3 deletions(-)
>>
>> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
>> index 76852abb3..2cc8dcecf 100644
>> --- a/docs/schemas/domaincommon.rng
>> +++ b/docs/schemas/domaincommon.rng
>> @@ -1669,7 +1669,7 @@
>>  
>>
>>  
>> -  
>> +  
>>  
>>  
>>
>> diff --git a/docs/schemas/storagecommon.rng b/docs/schemas/storagecommon.rng
>> index 717f3c603..49578312e 100644
>> --- a/docs/schemas/storagecommon.rng
>> +++ b/docs/schemas/storagecommon.rng
>> @@ -6,6 +6,14 @@
>>
>>  
>> +  
>> +
>> +  
>> +  [^
>> +]+
>> +
>> +  
>> +
>>
>>  
>>
>> diff --git a/docs/schemas/storagepool.rng b/docs/schemas/storagepool.rng
>> index f0117bd69..52b2044be 100644
>> --- a/docs/schemas/storagepool.rng
>> +++ b/docs/schemas/storagepool.rng
>> @@ -209,7 +209,7 @@
>>  
>>
>>  
>> -  
>> +  
> 
> This means that a name starting with a dot is invalid according to the
> schema, but the user ignored the schema and the code is not doing enough
> validation.
> 
> I'm not convinced that this is the correct solution. VMs disallow dots
> since the name is used for generating filenames and using '../' as
> prefix will allow directory traversal exploits.
> 
> NACK, I think we should disallow pool names with a dot even in the code.
> It will be slightly harder since there are no 'validate' callbacks for
> them  and you can't disallow them in the parser.
> 

As a test I defined a domain and a pool using ".test" and it worked.

# virsh list
 IdName   State

 1 .f23   running

# virsh dumpxml .f23

  .f23
  e28761fe-ea53-4682-924a-744a4028f146
  2097152
...


The problem with disallowing in code after the fact is how do you handle
things now that we have allowed it?  I have another long standing bug
where someone doesn't want a space in the name or even worse a space as
the name.

John

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

Re: [libvirt] [PATCH v10 3/5] util: Add TLS attributes to virStorageSource

2017-09-28 Thread John Ferlan 


On 09/28/2017 09:25 AM, Peter Krempa wrote:
> On Wed, Sep 27, 2017 at 11:45:53 -0400, John Ferlan wrote:
>> From: Ashish Mittal 
>>
>> Add an optional virTristateBool haveTLS to virStorageSource to
>> manage whether a storage source will be using TLS.
>>
>> Sample XML for a VxHS disk:
>>
>> 
>>   
>>   > tls='yes'>
>> 
>>   
>>   
>> 
>>
>> Additionally add a tlsFromConfig boolean to control whether the TLS
>> setting was due to domain configuration or qemu.conf global setting
>> in order to decide whether to Format the haveTLS setting for either
>> a live or saved domain configuration file.
> 
> I'm still unhappy that you've disregarded my comment and still format
> this as an integer. I don't really buy the argument that it should be
> this way because it's done the wrong way somewhere else.
> 
> Said this ... ACK regardless. 
> 

I guess I was going for consistency with the existing model.

I can format and parse as 'true' and 'false' though, would the
following suffice (sorry, I know you dislike cut-n-paste output,
but this was quicker)?

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 87192eb2d..2d8e573c9 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -8166,13 +8166,8 @@ virDomainDiskSourceParse(xmlNodePtr node,
 
 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;
+if (tlsCfg && STREQ(tlsCfg, "true"))
+src->tlsFromConfig = true;
 }
 
 /* for historical reasons the volume name for gluster volume is stored
@@ -21729,7 +21724,8 @@ virDomainDiskSourceFormatNetwork(virBufferPtr attrBuf,
 virBufferAsprintf(attrBuf, " tls='%s'",
   virTristateBoolTypeToString(src->haveTLS));
 if (flags & VIR_DOMAIN_DEF_FORMAT_STATUS)
-virBufferAsprintf(attrBuf, " tlsFromConfig='%d'", src->tlsFromConfig);
+virBufferAsprintf(attrBuf, " tlsFromConfig='%s'",
+  src->tlsFromConfig ? "true" : "false");
 
 for (n = 0; n < src->nhosts; n++) {
 virBufferAddLit(childBuf, "

Re: [libvirt] [PATCH v10 4/5] qemu: Introduce qemuDomainPrepareDiskSource

2017-09-28 Thread Peter Krempa 
On Wed, Sep 27, 2017 at 11:45:54 -0400, John Ferlan wrote:
> Introduce a function to setup any TLS needs for a disk source.
> 
> If there's a configuration or other error setting up the disk source
> for TLS, then cause the domain startup to fail.
> 
> For VxHS, follow the chardevTLS model where if the src->haveTLS hasn't
> been configured, then take the system/global cfg->haveTLS setting for
> the storage source *and* mark that we've done so via the tlsFromConfig
> setting in storage source.
> 
> Next, if we are using TLS, then generate an alias into a virStorageSource
> 'tlsAlias' field that will be used to create the TLS object and added to
> the disk object in order to link the two together for QEMU.
> 
> Signed-off-by: John Ferlan 
> ---
>  src/qemu/qemu_domain.c| 73 
> +++
>  src/qemu/qemu_domain.h| 11 +++
>  src/qemu/qemu_process.c   |  4 +++
>  src/util/virstoragefile.c |  8 +-
>  src/util/virstoragefile.h |  7 +
>  5 files changed, 102 insertions(+), 1 deletion(-)

ACK


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

Re: [libvirt] [PATCH v10 2/5] docs: Clean up the description for network disk protocol options

2017-09-28 Thread Peter Krempa 
On Wed, Sep 27, 2017 at 11:45:52 -0400, John Ferlan wrote:
> Clean up the description a bit to make it more readable and not
> appear as one long run-on paragraph.
> 
> Signed-off-by: John Ferlan 
> ---
>  docs/formatdomain.html.in | 25 +
>  1 file changed, 17 insertions(+), 8 deletions(-)

ACK


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

Re: [libvirt] [PATCH] docs, rng: Allow a pool name to be line domain name

2017-09-28 Thread Peter Krempa 
On Wed, Sep 27, 2017 at 15:07:35 -0400, John Ferlan wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1475250
> 
> It's possible to define and start a pool with a '.' in the
> name; however, when trying to add a volume to a domain using
> the storage pool source with a name with a '.' in the name,
> the domain RNG validation fails because RNG uses 'genericName'
> which does not allow a '.' in the name. Pool definition has
> no similar call to virXMLValidateAgainstSchema. Pool name
> validation occurs in storagePoolDefineXML and only calls
> virXMLCheckIllegalChars using the same parameter "\n" as
> qemuDomainDefineXMLFlags would check after the RNG check
> could be succesful.
> 
> So in order to resolve this, create a poolName definition
> in the RNG and allow the pool name and the volume source
> pool name to use that definition.
> 
> Signed-off-by: John Ferlan 
> ---
>  docs/schemas/domaincommon.rng  | 2 +-
>  docs/schemas/storagecommon.rng | 8 
>  docs/schemas/storagepool.rng   | 4 ++--
>  3 files changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 76852abb3..2cc8dcecf 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -1669,7 +1669,7 @@
>  
>
>  
> -  
> +  
>  
>  
>
> diff --git a/docs/schemas/storagecommon.rng b/docs/schemas/storagecommon.rng
> index 717f3c603..49578312e 100644
> --- a/docs/schemas/storagecommon.rng
> +++ b/docs/schemas/storagecommon.rng
> @@ -6,6 +6,14 @@
>
>  
> +  
> +
> +  
> +  [^
> +]+
> +
> +  
> +
>
>  
>
> diff --git a/docs/schemas/storagepool.rng b/docs/schemas/storagepool.rng
> index f0117bd69..52b2044be 100644
> --- a/docs/schemas/storagepool.rng
> +++ b/docs/schemas/storagepool.rng
> @@ -209,7 +209,7 @@
>  
>
>  
> -  
> +  

This means that a name starting with a dot is invalid according to the
schema, but the user ignored the schema and the code is not doing enough
validation.

I'm not convinced that this is the correct solution. VMs disallow dots
since the name is used for generating filenames and using '../' as
prefix will allow directory traversal exploits.

NACK, I think we should disallow pool names with a dot even in the code.
It will be slightly harder since there are no 'validate' callbacks for
them  and you can't disallow them in the parser.


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

Re: [libvirt] Plans for next release

2017-09-28 Thread Peter Krempa 
On Thu, Sep 28, 2017 at 12:17:41 +0200, Daniel Veillard wrote:
>  I'm a bit late, again ...
> 
> I think we should enter freeze late today, then I can make an rc2 this week
> end and shoot for a release on the 3rd
> 
>   I hope there is no issue with this,

Please wait until John pushes the remainder of the VxHS series which I'm
currently reviewing, so that also the TLS parts make it in.


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

Re: [libvirt] [PATCH v10 1/5] conf: Introduce TLS options for VxHS block device clients

2017-09-28 Thread Peter Krempa 
On Wed, Sep 27, 2017 at 11:45:51 -0400, John Ferlan wrote:
> From: Ashish Mittal 
> 
> Add a new TLS X.509 certificate type - "vxhs". This will handle the
> creation of a TLS certificate capability for properly configured
> VxHS network block device clients.
> 
> The following describes the behavior of TLS for VxHS block device:
> 
>   (1) Two new options have been added in /etc/libvirt/qemu.conf
>   to control TLS behavior with VxHS block devices
>   "vxhs_tls" and "vxhs_tls_x509_cert_dir".
>   (2) Setting "vxhs_tls=1" in /etc/libvirt/qemu.conf will enable
>   TLS for VxHS block devices.
>   (3) "vxhs_tls_x509_cert_dir" can be set to the full path where the
>   TLS CA certificate and the client certificate and keys are saved.
>   If this value is missing, the "default_tls_x509_cert_dir" will be
>   used instead. If the environment is not configured properly the
>   authentication to the VxHS server will fail.
> 
> Signed-off-by: Ashish Mittal 
> Signed-off-by: John Ferlan 
> ---
>  src/qemu/libvirtd_qemu.aug |  4 
>  src/qemu/qemu.conf | 34 ++
>  src/qemu/qemu_conf.c   | 16 
>  src/qemu/qemu_conf.h   |  3 +++
>  src/qemu/test_libvirtd_qemu.aug.in |  2 ++
>  5 files changed, 59 insertions(+)

ACK


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

Re: [libvirt] [PATCH v10 3/5] util: Add TLS attributes to virStorageSource

2017-09-28 Thread Peter Krempa 
On Wed, Sep 27, 2017 at 11:45:53 -0400, John Ferlan wrote:
> From: Ashish Mittal 
> 
> Add an optional virTristateBool haveTLS to virStorageSource to
> manage whether a storage source will be using TLS.
> 
> Sample XML for a VxHS disk:
> 
> 
>   
>tls='yes'>
> 
>   
>   
> 
> 
> Additionally add a tlsFromConfig boolean to control whether the TLS
> setting was due to domain configuration or qemu.conf global setting
> in order to decide whether to Format the haveTLS setting for either
> a live or saved domain configuration file.

I'm still unhappy that you've disregarded my comment and still format
this as an integer. I don't really buy the argument that it should be
this way because it's done the wrong way somewhere else.

Said this ... ACK regardless. 



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

Re: [libvirt] [Qemu-devel] [PATCH 1/2] vl: Eliminate defconfig variable

2017-09-28 Thread Eduardo Habkost 
(CCing libvir-list)

On Thu, Sep 28, 2017 at 12:42:02PM +0100, Daniel P. Berrange wrote:
> On Wed, Sep 27, 2017 at 05:32:24PM -0300, Eduardo Habkost wrote:
> > Both -nodefconfig and -no-user-config options do the same thing
> > today, we only need one variable to keep track of them.
> 
> What reason for picking -nodefconfig instead of -no-user-config to
> deprecate ?  -nodefconfig predates -no-user-config by a few years,
> and is what libvirt has historically used. So from libvirt POV
> we'd have a slight preference to deprecate -no-user-config instead
> and keep -nodefconfig, simply to avoid need to add conditional
> logic to libvirt to pick which to use.

libvirt already prefers -no-user-config, which is the right thing
for libvirt because it needs a mechanism to disable user-provided
configuration only, not QEMU-provided data files (in case they
exist).

In other words, we have an important use case for the
-no-user-config semantics in libvirt, and I am not aware of any
existing user of the -nodefconfig semantics.

(For the record, I think the existing documented -nodefconfig
semantics is awful and useless, but that's the one we got.  Old
discussions about this can be seen at:
https://www.mail-archive.com/libvir-list@redhat.com/msg53083.html )

> 
> > 
> > Suggested-by: Markus Armbruster 
> > Signed-off-by: Eduardo Habkost 
> > ---
> >  vl.c | 5 +
> >  1 file changed, 1 insertion(+), 4 deletions(-)
> > 
> > diff --git a/vl.c b/vl.c
> > index 4fd01fda91..b347a97a5b 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -3093,7 +3093,6 @@ int main(int argc, char **argv, char **envp)
> >  const char *qtest_log = NULL;
> >  const char *pid_file = NULL;
> >  const char *incoming = NULL;
> > -bool defconfig = true;
> >  bool userconfig = true;
> >  bool nographic = false;
> >  DisplayType display_type = DT_DEFAULT;
> > @@ -3194,8 +3193,6 @@ int main(int argc, char **argv, char **envp)
> >  popt = lookup_opt(argc, argv, , );
> >  switch (popt->index) {
> >  case QEMU_OPTION_nodefconfig:
> > -defconfig = false;
> > -break;
> >  case QEMU_OPTION_nouserconfig:
> >  userconfig = false;
> >  break;
> > @@ -3203,7 +3200,7 @@ int main(int argc, char **argv, char **envp)
> >  }
> >  }
> >  
> > -if (defconfig && userconfig) {
> > +if (userconfig) {
> >  if (qemu_read_default_config_file() < 0) {
> >  exit(1);
> >  }
> > -- 
> > 2.13.5
> > 
> > 
> 
> 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 :|

-- 
Eduardo

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

2017-09-28 Thread Daniel P. Berrange 
On Wed, Sep 27, 2017 at 11:36:20PM +0200, Christian Ehrhardt wrote:
> Hi,
> there seems to be an incompatibility to the last glibc due to [1].
> 
> Eventually this breaks gnulib unittests (and maybe more).

[snip]

We should have detected this a while ago sinc Fedora rawhide has 2.26, in
fact it has pre-release of 2.27. I've just found however that our Jenkins
job config had a mistake in it so it wasn't setting VIR_TEST_EXPENSIVE=1
and so the gnulib tests were being skipped. I've fixed jenkins now, so
presumably we'll shortly get a failure in jenkins :-)

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] gnulib tests in libvirt broken by newer glibc 2.26

2017-09-28 Thread Christian Ehrhardt 
On Thu, Sep 28, 2017 at 2:05 PM, Christian Ehrhardt <
christian.ehrha...@canonical.com> wrote:

>
>
> On Thu, Sep 28, 2017 at 12:25 AM, Eric Blake  wrote:
>
>> [adding gnulib]
>>
>
> [...]

>
>>
> then libvirt needs to pick up the
>> updated gnulib.
>
>
> I copied in current gnulib from git and it didn't resolve yet. But maybe
> it is just something that still is work in progress.
>

Until there is a final and better solution is available the following patch
avoids the issue without fully skipping the test for now.
Since the issue seems only to apply to getopt, but not getopt_long and the
code doesn't use that in anything except examples it might be a valid
interim helper until glibc/gnulib sorted out how that is handled more
correctly.


-- 
Christian Ehrhardt
Software Engineer, Ubuntu Server
Canonical Ltd
Description: fix gnulib unittests with glibc 2.26

Due to glibc and gnulib exchanging and syncing getopt implementations
it now uses the implementation form glibc but expects the former gnulib
behavior.

Until fully resolved override the test include to use gnulib function as it
used to be before.

It is somewhat ok to do so as the only real user of getopt so far is an example
at examples/admin/logging.c. All others use getopt_long which does not have
this function name collision between unistd.h and getopt.h.

Note: to be replaced by the final fix as soon as that is available.

Forwarded: yes https://www.redhat.com/archives/libvir-list/2017-September/msg01039.html
Author: Christian Ehrhardt 
Bug-Ubuntu: https://bugs.launchpad.net/bugs/1718668
Last-Update: 2017-09-28

 test-getopt-posix.c |7 +++
  1 file changed, 7 insertions(+)

--- a/gnulib/tests/test-getopt-posix.c
+++ b/gnulib/tests/test-getopt-posix.c
@@ -22,6 +22,13 @@
ftell link warning if we are not using the gnulib ftell module.  */
 #define _GL_NO_LARGE_FILES
 
+/*
+ * Glibc 2.26 does hard include bits/getopt_posix.h which causes the system
+ * to use glibc's getopt but the tests expect gnulib behavior. Until a better
+ * fix is available this avoids that mis-resolution.
+ */
+#include 
+
 /* POSIX and glibc provide the getopt() function in , see
http://pubs.opengroup.org/onlinepubs/9699919799/functions/getopt.html
https://www.gnu.org/software/libc/manual/html_node/Using-Getopt.html
--
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

2017-09-28 Thread Christian Ehrhardt 
On Thu, Sep 28, 2017 at 12:25 AM, Eric Blake  wrote:

> [adding gnulib]
>
> On 09/27/2017 04:36 PM, Christian Ehrhardt wrote:
> > Hi,
> > there seems to be an incompatibility to the last glibc due to [1].
>
> Gnulib needs to be updated to track the glibc changes (it looks like
> that is actually under way already),


Yeah that seems to be part of the effort I linked.


> then libvirt needs to pick up the
> updated gnulib.


I copied in current gnulib from git and it didn't resolve yet. But maybe it
is just something that still is work in progress.


> I'll leave the rest of your email intact in case it
> helps gnulib readers, but I'll probably help resolve the issue when I
> get a chance.
>



> I'm assuming you hit this failure on rawhide, as that is the platform
> most likely to be using bleeding-edge glibc with the getopt changes?


It was Ubuntu 17.10 (Artful) which is on 2.26

>
> > Eventually this breaks gnulib unittests (and maybe more).
> > Debugging went from an assert, to bidngin different symbols, to changed
> > function names to different header resolution.
> >
> > Because it expects it to behave "posixly" but arguments are changed
> > differently.
> > FAIL: test-getopt-posix
> > ===
> >
> > ../../../../gnulib/tests/test-getopt.h:754: assertion 'strcmp (argv[1],
> > "donald") == 0' failed
> >
> > # get and build latest libvirt to get the local gnulib
> > $ wget http://libvirt.org/sources/libvirt-3.7.0.tar.xz
> > $ tar xf libvirt-3.7.0.tar.xz
> > $ cd libvirt
> > $ apt build-dep libvirt
> > $ ./autogen.sh
> > $ make -j4
> >
> >
> > You can run the following simplified test derived from the unit test (it
> is
> > much shorter, so easier to debug e.g. in -E).
> >
> > $ cat << EOF >> test1.c
> > #include 
> > #include 
> >
> > #include 
> > #include 
> > #include 
> >
> > int main (void)
> > {
> > return 0;
> > }
> > EOF
> > $ gcc -I ./gnulib/lib -I . test1.c -H
> >
> > You can see in -H output already the difference in the paths of the
> headers
> > that it uses:
> >
> > Glibc <2.26
> > . ./config.h
> > .. ./config-post.h
> > . /usr/include/unistd.h
> > [...]
> > .. /usr/include/getopt.h
> >
> > Glibc >=2.26
> > . ./config.h
> > .. ./config-post.h
> > . ./gnulib/lib/unistd.h
> > [...]
> >
> > ... /usr/include/x86_64-linux-gnu/bits/getopt_posix.h
> >  /usr/include/x86_64-linux-gnu/bits/getopt_core.
> >
> >
> > If you build with -E you'll also see that it now uses getopt from glibc
> > instead of the prefixed rpl_getopt from gnulib.
> >
> > It behaves as if it would not find gnulib and instead fall back to glibc.
> > But it finds gnulib, instead due to glibc's changes its implementation is
> > fetched before and due to that pulling in glibc's behavior while the unit
> > test is verifying against the one it expects from gnulib.
> >
> >
> > Sorry, but I don't see the right fix here yet - I could easily silence
> the
> > test but that is no fix. Especially if there might be implications due to
> > handling the args (slightly) differently.
> >
> > I really wanted to come up with the same test against gnulib alone, but I
> > was lost in the build system for too long and could not get the example
> to
> > fail without libvirt (OTOH I'm sure it would).
> >
> > Therefore I'm reaching out to you for your help and experience on the
> build
> > system what could be done.
> >
> > [1]: https://sourceware.org/ml/libc-alpha/2017-04/msg00115.html
> >
> >
> >
> > --
> > libvir-list mailing list
> > libvir-list@redhat.com
> > https://www.redhat.com/mailman/listinfo/libvir-list
> >
>
> --
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.   +1-919-301-3266
> Virtualization:  qemu.org | libvirt.org
>
>


-- 
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 v4 7/7] nodedev: Work around the uevent race by hooking up virFileWaitForAccess

2017-09-28 Thread Erik Skultety 
On Wed, Sep 20, 2017 at 10:33:14AM -0400, John Ferlan wrote:
>
>
> On 09/18/2017 12:34 PM, Erik Skultety wrote:
> > If we find ourselves in the situation that the 'add' uevent has been
> > fired earlier than the sysfs tree for a device was created, we should
> > use the best-effort approach and give kernel some predetermined amount
> > of time, thus waiting for the attributes to be ready rather than
> > discarding the device from our device list forever. If those don't appear
> > in the given time frame, we need to move on, since libvirt can't wait
> > indefinitely.
> >
> > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1463285
> >
> > Signed-off-by: Erik Skultety 
> > ---
> >  src/node_device/node_device_udev.c | 16 +++-
> >  1 file changed, 15 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/node_device/node_device_udev.c 
> > b/src/node_device/node_device_udev.c
> > index 70e15ffb8..2f63256a3 100644
> > --- a/src/node_device/node_device_udev.c
> > +++ b/src/node_device/node_device_udev.c
> > @@ -1166,9 +1166,23 @@ udevProcessMediatedDevice(struct udev_device *dev,
> >  char *canonicalpath = NULL;
> >  virNodeDevCapMdevPtr data = >caps->data.mdev;
> >
> > -if (virAsprintf(, "%s/mdev_type", 
> > udev_device_get_syspath(dev)) < 0)
> > +/* Because of a kernel uevent race, we might get the 'add' event prior 
> > to
> > + * the sysfs tree being ready, so any attempt to access any sysfs 
> > attribute
> > + * would result in ENOENT and us dropping the device, so let's work 
> > around
> > + * it by waiting for the attributes to become available.
> > + */
> > +
> > +if (virAsprintf(, "%s/mdev_type",
> > +udev_device_get_syspath(dev)) < 0)
> >  goto cleanup;
> >
> > +if (virFileWaitForExists(linkpath, 1, 100) < 0) {
> > +virReportSystemError(errno,
> > + _("failed to wait for file '%s' to appear"),
> > + linkpath);
> > +goto cleanup;
> > +}
> > +
>
> So the linkpath gets created after the canonicalpath... and we don't

Well, I would assume so, I believe that the kernel wouldn't create symlinks
first and the canonical paths second, such a design would be sooo broken.

Thanks,
Erik

> have to check that right?
>
> Considering what I pointed out in my previous review - I wouldn't be
> able to use virFileWaitForExists, but a similar loop would be possible.
> For NPIV the file exists, it's the contents that are bogus momentarily.
>
> Other consumers waiting that I looked at usually wait on some sort of
> open() and usleep() when ENOENT is returned (there's multiple examples
> if you search on ulseep).  Wonder if any of those could utilize
> something like this...  I know patches welcome ;-)
>
> Reviewed-by: John Ferlan 
>
>
> >  if (virFileResolveLink(linkpath, ) < 0) {
> >  virReportSystemError(errno, _("failed to resolve '%s'"), linkpath);
> >  goto cleanup;
> >

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

[libvirt] Plans for next release

2017-09-28 Thread Daniel Veillard 
 I'm a bit late, again ...

I think we should enter freeze late today, then I can make an rc2 this week
end and shoot for a release on the 3rd

  I hope there is no issue with this,

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] [PATCH] nwfilter: Fix possible segfault on sometimes consumed variable

2017-09-28 Thread Erik Skultety 
On Wed, Sep 27, 2017 at 10:22:17AM -0400, John Ferlan wrote:
> The virNWFilterIPAddrMapAddIPAddr code can consume the @addr parameter
> on success when the @ifname is found in the ipAddressMap->hashTable
> hash table in the call to virNWFilterVarValueAddValue; however, if
> not found in the hash table, then @addr is formatted into a @val
> which is stored in the table and on return the caller would be
> expected to free @addr.
>
> Thus, the caller has no way to determine on success whether @addr was
> consumed, so in order to fix this create a @tmp variable which will
> be stored/consumed when virNWFilterVarValueAddValue succeeds. That way
> the caller can free @addr whether the function returns success or failure.
>
> Signed-off-by: John Ferlan 

Reviewed-by: Erik Skultety 

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

2017-09-28 Thread Erik Skultety 
[...]

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

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.

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?

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

This magic threshold approach still wouldn't work because you don't know
anything about the event at that point, you'd have to process them to actually
find out whether that's a legitimate event or it's an already noted event that
hasn't been processed yet, so it would still mess with your counters.

>
> IIRC from the previous trip down this rabbit hole (I did briefly go back
> and read the comments) that what you're trying to avoid is the following
> message spamming the error logs because of a scheduling mishap... Thus
> perhaps the following message could only be displayed if nevents > 1 and
> nothing was found knowing that we have this "timing problem" between
> udev, this thread, and the fd polling scanner of when a device should be
> "found"...  and then reset nevents == 0 with the appropriate lock.
>
> Curious on your thoughts before an R-B/ACK though.
>
> John
>
>
> >  if (!device) {
> >  virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> > _("udev_monitor_receive_device returned NULL"));
> > @@ -1750,10 +1755,12 @@ udevEventHandleCallback(int watch ATTRIBUTE_UNUSED,
> >  int events ATTRIBUTE_UNUSED,
> >  void *opaque)
> >  {
> > +udevPrivate *priv = NULL;
> >  struct udev_monitor *udev_monitor = NULL;
> >  udevEventThreadDataPtr threadData = opaque;
> >
> >  nodeDeviceLock();
> > +priv = driver->privateData;
> >  udev_monitor = DRV_STATE_UDEV_MONITOR(driver);
> >
> >  if

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

2017-09-28 Thread Chao Fan 
On Thu, Sep 28, 2017 at 04:32:26PM +0800, 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;
> 

Ping John Ferlan,

>+if (stats->ram_page_size && (!(stats->ram_pag_size > 0) ||
>+virTypedParamsAddULLong(, , ,

Thanks for your suggestion, and I wonder if here is OK.

Thanks,
Chao Fan
>+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",
>   
> >ram_dirty_rate));
>+ignore_value(virJSONValueObjectGetNumberUlong(ram, "page-size",
>+  >ram_page_size));
> ignore_value(virJSONValueObjectGetNumberUlong(ram, "dirty-sync-count",
>   >ram_iteration));
> 
>diff --git a/tools/virsh-domain.c

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

2017-09-28 Thread Chao Fan 
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(, , ,
+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",
   >ram_dirty_rate));
+ignore_value(virJSONValueObjectGetNumberUlong(ram, "page-size",
+  >ram_page_size));
 ignore_value(virJSONValueObjectGetNumberUlong(ram, "dirty-sync-count",
   >ram_iteration));
 
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index a3f3b7c7b..a50713d6e 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -6021,6 +6021,14 @@ cmdDomjobinfo(vshControl *ctl, const vshCmd *cmd)
 }
 
 if ((rc = virTypedParamsGetULLong(params, nparams,
+

Re: [libvirt] [RFC] docs: Discourage usage of cache mode=passthrough

2017-09-28 Thread Daniel P. Berrange 
On Thu, Sep 21, 2017 at 01:14:04PM -0400, Laine Stump wrote:
> On 09/19/2017 03:37 PM, Eduardo Habkost wrote:
> > Cache mode=passthrough can result in a broken cache topology if
> > the domain topology is not exactly the same as the host topology.
> > Warn about that in the documentation.
> >
> > Bug report for reference:
> > https://bugzilla.redhat.com/show_bug.cgi?id=1184125
> >
> > Signed-off-by: Eduardo Habkost 
> > ---
> >  docs/formatdomain.html.in | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> > index 57ec2ff34..9c21892f3 100644
> > --- a/docs/formatdomain.html.in
> > +++ b/docs/formatdomain.html.in
> > @@ -1478,7 +1478,9 @@
> >  
> >passthrough
> >The real CPU cache data reported by the host CPU will be
> > -passed through to the virtual CPU.
> > +passed through to the virtual CPU.  Using this mode is not
> > +recommended unless the domain CPU and NUMA topology is 
> > exactly
> > +the same as the host CPU and NUMA topology.
> 
> To me this sounds like it should be forbidden by libvirt, rather than
> just documented as "bad". (I haven't followed any previous discussion on
> the topic though, so maybe I'm over-reacting).

In high performance setups, people pin guest vCPUs to host pCPUs and
set the vCPU topology to match the host pCPU topology they've pinned
to. So ohaving a cache mode that matches this topology is just fine.
It simply isn't something you want as a default for the more typical
floating vCPUs scenarios.

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] iohelper: use saferead if later write with O_DIRECT

2017-09-28 Thread Nikolay Shirokovskiy 
One of the usecases of iohelper is to read from pipe and write
to file with O_DIRECT. As we read from pipe we can have partial
read and then we fail to write this data because output file
is open with O_DIRECT and buffer size is not aligned.
---
 src/util/iohelper.c | 16 ++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/src/util/iohelper.c b/src/util/iohelper.c
index 5416d45..bb8a8dd 100644
--- a/src/util/iohelper.c
+++ b/src/util/iohelper.c
@@ -109,9 +109,21 @@ runIO(const char *path, int fd, int oflags)
 while (1) {
 ssize_t got;
 
-if ((got = read(fdin, buf, buflen)) < 0) {
-if (errno == EINTR)
+/* If we read with O_DIRECT from file we can't use saferead as
+ * it can lead to unaligned read after reading last bytes.
+ * If we write with O_DIRECT use should use saferead so that
+ * writes will be aligned.
+ * In other cases using saferead reduces number of syscalls.
+ */
+if (fdin == fd && direct) {
+if ((got = read(fdin, buf, buflen)) < 0 &&
+errno == EINTR)
 continue;
+} else {
+got = saferead(fdin, buf, buflen);
+}
+
+if (got < 0) {
 virReportSystemError(errno, _("Unable to read %s"), fdinname);
 goto cleanup;
 }
-- 
1.8.3.1

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

[libvirt] [PATCH] spec: don't package product dirs

2017-09-28 Thread Nikolay Shirokovskiy 
Directories /var/{lib,cache}/libvirt/qemu/ are created by libvirtd on
start and their owner:group is changed according to the config. Thus
no need to include them in libvirt-daemon-driver-qemu package. Otherwise
we see noisy "directory changed" on rpm -V for the package.
---
 libvirt.spec.in | 2 --
 1 file changed, 2 deletions(-)

diff --git a/libvirt.spec.in b/libvirt.spec.in
index a3bd77f..e20f65c 100644
--- a/libvirt.spec.in
+++ b/libvirt.spec.in
@@ -1911,8 +1911,6 @@ exit 0
 %config(noreplace) %{_sysconfdir}/libvirt/qemu-lockd.conf
 %config(noreplace) %{_sysconfdir}/logrotate.d/libvirtd.qemu
 %ghost %dir %attr(0700, root, root) %{_localstatedir}/run/libvirt/qemu/
-%dir %attr(0751, %{qemu_user}, %{qemu_group}) 
%{_localstatedir}/lib/libvirt/qemu/
-%dir %attr(0750, %{qemu_user}, %{qemu_group}) 
%{_localstatedir}/cache/libvirt/qemu/
 %{_datadir}/augeas/lenses/libvirtd_qemu.aug
 %{_datadir}/augeas/lenses/tests/test_libvirtd_qemu.aug
 %{_libdir}/%{name}/connection-driver/libvirt_driver_qemu.so
-- 
1.8.3.1

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