Re: [libvirt] Paradox cpu topology in capabilities outputs

[Qiao, Liyong]

Okay, I got some answer myself.



This indicate the topology in one of a numa node. But what if I want to get the 
node’s physical socket number?


Best Regards

Eli Qiao(乔立勇）OpenStack Core team OTC Intel.
--


From: "Qiao, Liyong" 
Date: Wednesday, 11 January 2017 at 2:52 PM
To: "libvir-list@redhat.com" 
Cc: "Daniel P. Berrange" 
Subject: Paradox cpu topology in capabilities outputs

Hi,

I observe that virsh capabilities give wrong cpu topology on a multiple sockets 
host

taget@jfz1r04h13:~/libvirt$ lscpu
Architecture:  x86_64
CPU op-mode(s):32-bit, 64-bit
Byte Order:Little Endian
CPU(s):72
On-line CPU(s) list:   0-71
Thread(s) per core:2
Core(s) per socket:18
Socket(s): 2 <
NUMA node(s):  2
Vendor ID: GenuineIntel
CPU family:6
Model: 63
Model name:Intel(R) Xeon(R) CPU E5-2699 v3 @ 2.30GHz
Stepping:  2
CPU MHz:   1201.660
CPU max MHz:   3600.
CPU min MHz:   1200.
BogoMIPS:  4590.78
Virtualization:VT-x
L1d cache: 32K
L1i cache: 32K
L2 cache:  256K
L3 cache:  46080K
NUMA node0 CPU(s): 0-17,36-53
NUMA node1 CPU(s): 18-35,54-71

But output of virsh capabilities only gives.



looking into code and got this:
https://libvirt.org/git/?p=libvirt.git;a=blob;f=src/util/virhostcpu.c;h=f29f3122acee018b9fd7dca06fd7ae1fc118b210;hb=HEAD#l703

should we change it into

704
 *sockets  += nodesockets;


This also affect nodeinfo.sockets.

Attached file is the full output of capabilities of the host

Best Regards

Eli Qiao(乔立勇）OpenStack Core team OTC Intel.
--

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

[libvirt] Paradox cpu topology in capabilities outputs

[Qiao, Liyong]

Hi,

I observe that virsh capabilities give wrong cpu topology on a multiple sockets 
host

taget@jfz1r04h13:~/libvirt$ lscpu
Architecture:  x86_64
CPU op-mode(s):32-bit, 64-bit
Byte Order:Little Endian
CPU(s):72
On-line CPU(s) list:   0-71
Thread(s) per core:2
Core(s) per socket:18
Socket(s): 2 <
NUMA node(s):  2
Vendor ID: GenuineIntel
CPU family:6
Model: 63
Model name:Intel(R) Xeon(R) CPU E5-2699 v3 @ 2.30GHz
Stepping:  2
CPU MHz:   1201.660
CPU max MHz:   3600.
CPU min MHz:   1200.
BogoMIPS:  4590.78
Virtualization:VT-x
L1d cache: 32K
L1i cache: 32K
L2 cache:  256K
L3 cache:  46080K
NUMA node0 CPU(s): 0-17,36-53
NUMA node1 CPU(s): 18-35,54-71

But output of virsh capabilities only gives.



looking into code and got this:
https://libvirt.org/git/?p=libvirt.git;a=blob;f=src/util/virhostcpu.c;h=f29f3122acee018b9fd7dca06fd7ae1fc118b210;hb=HEAD#l703

should we change it into

704
 *sockets  += nodesockets;


This also affect nodeinfo.sockets.

Attached file is the full output of capabilities of the host

Best Regards

Eli Qiao(乔立勇）OpenStack Core team OTC Intel.
--



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

Re: [libvirt] Do different Virgil3D users running have the same CANVAS fingerprint?

[bancfc]

ACK :)

On 2017-01-09 11:17, Martin Kletzander wrote:

On Sun, Jan 08, 2017 at 07:17:07PM +0100, ban...@openmailbox.org wrote:

On 2016-12-27 04:05, ban...@openmailbox.org wrote:

Background:
Canvas fingerprinting is a technique to track users based on their
GPUs. https://en.wikipedia.org/wiki/Canvas_fingerprinting

Someone did some testing and the good news is the fingerprint is
common for all users of the same OS across all KVM users because it
presents the same virtual GPU/drivers for everybody.

What I need your help to find out is whether this holds for Virgil3D
users that enable graphics acceleration. (I am running on Debian and
the libvirt version in the repos is too old to have this included 
also


$ ./configure && make && make install # O:-)


I don't have multiple machines)


Oh, not even nested VMs won't hep here :(




To test this you need:

To run plain FireFox on a virtualized distro (with GL acceleration
enabled in libvirt) of your choice and visit
https://browserleaks.com/canvas . Then repeat the same steps with 
that

same distro (in a VM) on another physical machine and compare values.

A good result is if you see the same numbers for both machines. That
means the virtual device  fingerprint is uniform for all virtual
users.


Bumping. I realized I post this in holiday season when it was easily
missed.



Not only were even patches discussed during that time, but people will
catch up on things after the break, don't worry.


Since CANVAS is about graphics drivers version the question can be
simplified: Is the virtio-gpu driver version uniform across KVM
versions?



I think the drivers and versions will not be the same nor different 
*all

the time*.  Based on what the method says, the main difference is done
by font selection, hinting and antialiasing settings.  I don't think
that will be dependent on the graphics hardware.  Although the hardware
description might look the same.  The details about that should 
probably
be asked somewhere else than libvirt, e.g. whoever does virglrenderer 
or

qemu or...

I don't have another machine handy and my graphics guests are borked 
for

some reason (and I spent a lot of time trying to fix those already), so
I won't help much there.  I tried it with some live media, but then
realized it's not going to help.  I'd have to try the same VM (not only
distro), I guess.



--
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] storage_backend_rbd: always call rados_conf_read_file when connect a rbd pool

[Chen Hanxiao]

At 2017-01-11 02:23:54, "John Ferlan"  wrote:
>
>
>On 12/30/2016 03:39 AM, Chen Hanxiao wrote:
>> From: Chen Hanxiao 
>> 
>> This patch fix a dead lock when try to read a rbd image
>> 
>> When trying to connect a rbd server
>> (ceph-0.94.7-1.el7.centos.x86_64),
>> 
>> rbd_list/rbd_open enter a dead lock state.
>> 
>> Backtrace:
>> Thread 30 (Thread 0x7fdb342d0700 (LWP 12105)):
>> #0  0x7fdb40b16705 in pthread_cond_wait@@GLIBC_2.3.2 () from 
>> /lib64/libpthread.so.0
>> #1  0x7fdb294273f1 in librados::IoCtxImpl::operate_read(object_t const&, 
>> ObjectOperation*, ceph::buffer::list*, int) () from /lib64/librados.so.2
>> #2  0x7fdb29429fcc in librados::IoCtxImpl::read(object_t const&, 
>> ceph::buffer::list&, unsigned long, unsigned long) () from 
>> /lib64/librados.so.2
>> #3  0x7fdb293e850c in librados::IoCtx::read(std::string const&, 
>> ceph::buffer::list&, unsigned long, unsigned long) () from 
>> /lib64/librados.so.2
>> #4  0x7fdb2b9dd15e in librbd::list(librados::IoCtx&, 
>> std::vector >&) () from 
>> /lib64/librbd.so.1
>> #5  0x7fdb2b98c089 in rbd_list () from /lib64/librbd.so.1
>> #6  0x7fdb2e1a8052 in virStorageBackendRBDRefreshPool (conn=> out>, pool=0x7fdafc002d50) at storage/storage_backend_rbd.c:366
>> #7  0x7fdb2e193833 in storagePoolCreate (obj=0x7fdb1c1fd5a0, 
>> flags=) at storage/storage_driver.c:876
>> #8  0x7fdb43790ea1 in virStoragePoolCreate 
>> (pool=pool@entry=0x7fdb1c1fd5a0, flags=0) at libvirt-storage.c:695
>> #9  0x7fdb443becdf in remoteDispatchStoragePoolCreate 
>> (server=0x7fdb45fb2ab0, msg=0x7fdb45fb3db0, args=0x7fdb1c0037d0, 
>> rerr=0x7fdb342cfc30, client=) at remote_dispatch.h:14383
>> #10 remoteDispatchStoragePoolCreateHelper (server=0x7fdb45fb2ab0, 
>> client=, msg=0x7fdb45fb3db0, rerr=0x7fdb342cfc30, 
>> args=0x7fdb1c0037d0, ret=0x7fdb1c1b3260) at remote_dispatch.h:14359
>> #11 0x7fdb437d9c42 in virNetServerProgramDispatchCall 
>> (msg=0x7fdb45fb3db0, client=0x7fdb45fd1a80, server=0x7fdb45fb2ab0, 
>> prog=0x7fdb45fcd670) at rpc/virnetserverprogram.c:437
>> #12 virNetServerProgramDispatch (prog=0x7fdb45fcd670, 
>> server=server@entry=0x7fdb45fb2ab0, client=0x7fdb45fd1a80, 
>> msg=0x7fdb45fb3db0) at rpc/virnetserverprogram.c:307
>> #13 0x7fdb437d4ebd in virNetServerProcessMsg (msg=, 
>> prog=, client=, srv=0x7fdb45fb2ab0) at 
>> rpc/virnetserver.c:135
>> #14 virNetServerHandleJob (jobOpaque=, opaque=0x7fdb45fb2ab0) 
>> at rpc/virnetserver.c:156
>> #15 0x7fdb436cfb35 in virThreadPoolWorker 
>> (opaque=opaque@entry=0x7fdb45fa7650) at util/virthreadpool.c:145
>> #16 0x7fdb436cf058 in virThreadHelper (data=) at 
>> util/virthread.c:206
>> #17 0x7fdb40b12df5 in start_thread () from /lib64/libpthread.so.0
>> #18 0x7fdb408401ad in clone () from /lib64/libc.so.6
>> 
>> 366 len = rbd_list(ptr.ioctx, names, &max_size);
>> (gdb) n
>> [New Thread 0x7fdb20758700 (LWP 22458)]
>> [New Thread 0x7fdb20556700 (LWP 22459)]
>> [Thread 0x7fdb20758700 (LWP 22458) exited]
>> [New Thread 0x7fdb20455700 (LWP 22460)]
>> [Thread 0x7fdb20556700 (LWP 22459) exited]
>> [New Thread 0x7fdb20556700 (LWP 22461)]
>> 
>> infinite loop...
>> 
>> Signed-off-by: Chen Hanxiao 
>> ---
>>  src/storage/storage_backend_rbd.c | 7 +++
>>  1 file changed, 7 insertions(+)
>> 
>
>Could you provide a bit more context...
>
>Why does calling rados_conf_read_file with a NULL resolve the issue?
>
>Is this something "new" or "expected"? And if expected, why are we only
>seeing it now?
>
>What is the other thread that "has" the lock doing?

It seams that the server side of ceph does not response our request.

So when libvirt call rbd_open/rbd_list, etc, it never return.

But qemu works fine.
So I take qemu's code as a reference.
https://github.com/qemu/qemu/blob/master/block/rbd.c#L365

rados_conf_read_file with a NULL will try to get ceph conf file from 
/etc/ceph and other default paths.

Althougth we  rados_conf_set in the following code,
w/o rados_conf_read_file,
ceph-0.94.7-1.el7 does not answer our rbd_open.

Some elder or newer ceph server does not have this issue.
I think this may be a ceph server bug of ceph-0.94.7-1.el7.

Doing rados_conf_read_file(cluster, NULL)
will make our code more robust.

Regards,
- Chen

>
>>From my cursory/quick read of :
>
>http://docs.ceph.com/docs/master/rados/api/librados/
>
>...
>"Then you configure your rados_t to connect to your cluster, either by
>setting individual values (rados_conf_set()), using a configuration file
>(rados_conf_read_file()), using command line options
>(rados_conf_parse_argv()), or an environment variable
>(rados_conf_parse_env()):"
>
>Since we use rados_conf_set, that would seem to indicate we're OK. It's
>not clear from just what's posted why calling eventually calling
>rbd_list is causing a hang.
>
>I don't have the cycles or environment to do the research right now and
>it really isn't clear why a read_file would resolve the issue.
>
>John
>> diff --git a/src/storage/storage_bac

Re: [libvirt] [PATCH] Extend l3 cache to nodeinfo

[Qiao, Liyong]

Best Regards

Eli Qiao(乔立勇）OpenStack Core team OTC Intel.
-- 


On 10/01/2017, 8:32 PM, "Martin Kletzander"  wrote:

On Tue, Jan 10, 2017 at 04:11:03PM +0800, Eli Qiao wrote:
>This patch extends l3 cache infomation to nodeinfo output.
>

This doesn't make sense, in case there are multiple sockets with
different L3 cache sizes, it can't be represented in nodeinfo.  If there
can be multiple values, you need to extend virsh capabilities instead.

   
I don’t think it will work if we plug 2 different type of socket, if they can 
work together, they should have same size of L3 cache size, anyway, Daniel 
don’t agree to extend nodeinfo, I am trying to consider it as another way.


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

Re: [libvirt] [PATCH] Extend l3 cache to nodeinfo

[Qiao, Liyong]

>Also, why only l3 cache. Why not expose full info about
 >   the CPU cache hierarchy. It feels wrong to expose only
 >  L3 cache and ignore other levels of cache.
   
Okay, I’ll think how to expose there into capabilities. This is related to 
enable cache tune support in [1]
The status in kernel is that only L3 cache can be tuned(by cat_l3 support in 
kernel) for now.

Could you help to give some input for the RFC of cache tune?

[1]https://www.redhat.com/archives/libvir-list/2017-January/msg00354.html

Thanks Eli.


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

Re: [libvirt] RFC: Use __attribute__ ((cleanup) in libvirt ?

[Richard W.M. Jones]

On Tue, Jan 10, 2017 at 10:00:31AM +, Daniel P. Berrange wrote:
> If we mandate use of gcc / clang, then we wouldn't need to hide it
> behind a macro - we'd be able to use it inline. That said, using a
> macro makes it smaller and gives a bit of standardization. eg with
> libguestfs style:
> 
>   #define CLEANUP_FREE __attribute__((cleanup(free)))
>   #define CLEANUP_OBJECT_UNREF __attribute__((cleanup(virObjectUnref)))
> 
>   CLEANUP_FREE char *str;
>   CLEANUP_OBJECT_UNREF virDomainPtr dom;
> 
> vs full inline style:
> 
>   __attribute__((cleanup(free))) char *str;
>   __attribute__((cleanup(virObjectUnref))) virDomainPtr dom;
> 
> That said I see systemd took a halfway house
> 
>   #define _cleanup_(x) __attribute__((cleanup(x)))
> 
>   _cleanup(free) char *str;
>   _cleanup(virObjectUnref) virDomainPtr dom;

I think it's not quite as simple as that because GCC passes
the pointer to the pointer.  libguestfs uses:

#define CLEANUP_FREE __attribute__((cleanup(guestfs_int_cleanup_free)))

...

void
guestfs_int_cleanup_free (void *ptr)
{
  free (* (void **) ptr);
}

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html

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

Re: [libvirt] [PATCH] libxl: fix usb inputs loop error

[Jim Fehlig]

Cédric Bosdonnat wrote:
> List indexes where mixed up in the code looping over the USB
> input devices.
> ---
>  src/libxl/libxl_conf.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
> index ac83b51c7..1053e60a1 100644
> --- a/src/libxl/libxl_conf.c
> +++ b/src/libxl/libxl_conf.c
> @@ -479,7 +479,7 @@ libxlMakeDomBuildInfo(virDomainDefPtr def,
>  if (VIR_EXPAND_N(b_info->u.hvm.usbdevice_list, nusbdevice, 1) < 
> 0)
>  return -1;
>  #else
> -if (i > 1) {
> +if (nusbdevice > 1) {

IIUC, when LIBXL_HAVE_BUILDINFO_USBDEVICE_LIST is not defined nusbdevice will
always be 0. I think we need to increment it before the > 1 check.

Regards,
Jim

>  virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>  _("libxenlight supports only one input device"));
>  return -1;
> @@ -487,7 +487,7 @@ libxlMakeDomBuildInfo(virDomainDefPtr def,
>  #endif
>  
>  #ifdef LIBXL_HAVE_BUILDINFO_USBDEVICE_LIST
> -usbdevice = &b_info->u.hvm.usbdevice_list[i];
> +usbdevice = &b_info->u.hvm.usbdevice_list[nusbdevice - 1];
>  #else
>  usbdevice = &b_info->u.hvm.usbdevice;
>  #endif

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

Re: [libvirt] [PATCH v3 8/8] conf: aggregate multiple pcie-root-ports onto a single slot

[Laine Stump]

On 12/19/2016 10:23 AM, Laine Stump wrote:

Set the VIR_PCI_CONNECT_AGGREGATE_SLOT flag for pcie-root-ports so
that they will be assigned to all the functions on a slot.

Some qemu test case outputs had to be adjusted due to the
pcie-root-ports now being put on multiple functions.
---


ARGH!

In my final rebase before pushing, I pulled in Andrea's patches that 
switch aarch64/virt to using PCI by default, and the test case for that 
resulted in a make check failure:


564) QEMU XML-2-ARGV aarch64-virtio-pci-default ... libvirt: QEMU Driver 
error : unsupported configuration: 'multifunction=on' is not supported 
with this QEMU binary FAILED


Is it really true that the aarch64 qemu doesn't support multifunction 
devices? If so, that really needs to be fixed. In the meantime, this 
means I still can't push my patches, because doing so will break 
aarch64. I'll try to come up with a patch to conditionalize 
AGGREGATE_SLOT on support for multifunction (which I suppose I should 
have done to begin with, but I wouldn't have expected that a platform 
that supports PCIe doesn't support multifunction devices :-/)



  src/conf/domain_addr.c |  2 +-
  .../qemuxml2argv-pcie-root-port.args   |  5 +-
  .../qemuxml2argv-pcie-switch-upstream-port.args|  5 +-
  .../qemuxml2argv-q35-default-devices-only.args |  7 +--
  .../qemuxml2argv-q35-multifunction.args| 12 +
  .../qemuxml2argv-q35-multifunction.xml | 11 +
  .../qemuxml2argv-q35-pcie-autoadd.args | 30 ++--
  tests/qemuxml2argvdata/qemuxml2argv-q35-pcie.args  | 28 ++-
  .../qemuxml2argv-q35-virt-manager-basic.args   | 13 ++---
  .../qemuxml2argv-q35-virtio-pci.args   | 28 ++-
  tests/qemuxml2argvtest.c   |  2 +
  .../qemuxml2xmlout-pcie-root-port.xml  |  2 +-
  .../qemuxml2xmlout-pcie-switch-upstream-port.xml   |  4 +-
  .../qemuxml2xmlout-q35-default-devices-only.xml|  8 ++--
  .../qemuxml2xmlout-q35-multifunction.xml   | 55 ++
  .../qemuxml2xmlout-q35-pcie-autoadd.xml| 52 ++--
  .../qemuxml2xmloutdata/qemuxml2xmlout-q35-pcie.xml | 48 +--
  .../qemuxml2xmlout-q35-virt-manager-basic.xml  | 20 
  .../qemuxml2xmlout-q35-virtio-pci.xml  | 48 +--
  tests/qemuxml2xmltest.c|  2 +
  20 files changed, 237 insertions(+), 145 deletions(-)

diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c
index 18421e0..d60b1d9 100644
--- a/src/conf/domain_addr.c
+++ b/src/conf/domain_addr.c
@@ -63,7 +63,7 @@ 
virDomainPCIControllerModelToConnectType(virDomainControllerModelPCI model)
  return VIR_PCI_CONNECT_TYPE_DMI_TO_PCI_BRIDGE;
  
  case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT:

-return VIR_PCI_CONNECT_TYPE_PCIE_ROOT_PORT;
+return VIR_PCI_CONNECT_TYPE_PCIE_ROOT_PORT | 
VIR_PCI_CONNECT_AGGREGATE_SLOT;
  
  case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT:

  return VIR_PCI_CONNECT_TYPE_PCIE_SWITCH_UPSTREAM_PORT;
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pcie-root-port.args 
b/tests/qemuxml2argvdata/qemuxml2argv-pcie-root-port.args
index 27d5164..9a71281 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-pcie-root-port.args
+++ b/tests/qemuxml2argvdata/qemuxml2argv-pcie-root-port.args
@@ -18,8 +18,9 @@ QEMU_AUDIO_DRV=none \
  -boot c \
  -device i82801b11-bridge,id=pci.1,bus=pcie.0,addr=0x1e \
  -device pci-bridge,chassis_nr=2,id=pci.2,bus=pci.1,addr=0x0 \
--device ioh3420,port=0x10,chassis=3,id=pci.3,bus=pcie.0,addr=0x2 \
--device ioh3420,port=0x1a,chassis=40,id=pci.4,bus=pcie.0,addr=0x3 \
+-device ioh3420,port=0x10,chassis=3,id=pci.3,bus=pcie.0,multifunction=on,\
+addr=0x2 \
+-device ioh3420,port=0x1a,chassis=40,id=pci.4,bus=pcie.0,addr=0x2.0x1 \
  -drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-sata0-0-0 \
  -device ide-drive,bus=ide.0,drive=drive-sata0-0-0,id=sata0-0-0 \
  -device qxl-vga,id=video0,ram_size=67108864,vram_size=33554432,bus=pcie.0,\
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pcie-switch-upstream-port.args 
b/tests/qemuxml2argvdata/qemuxml2argv-pcie-switch-upstream-port.args
index 93d16b8..10aedd5 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-pcie-switch-upstream-port.args
+++ b/tests/qemuxml2argvdata/qemuxml2argv-pcie-switch-upstream-port.args
@@ -18,8 +18,9 @@ QEMU_AUDIO_DRV=none \
  -boot c \
  -device i82801b11-bridge,id=pci.1,bus=pcie.0,addr=0x1e \
  -device pci-bridge,chassis_nr=2,id=pci.2,bus=pci.1,addr=0x0 \
--device ioh3420,port=0x10,chassis=3,id=pci.3,bus=pcie.0,addr=0x2 \
--device ioh3420,port=0x18,chassis=4,id=pci.4,bus=pcie.0,addr=0x3 \
+-device ioh3420,port=0x10,chassis=3,id=pci.3,bus=pcie.0,multifunction=on,\
+addr=0x2 \
+-device ioh3420,port=0x11,chassis=4,id=pci.4,bus=pcie.0,addr=0x2.0x1 \
  -device x3130-upstream,id=pci.5,bus=pci.3,addr=0x0 \
  -device x3130

Re: [libvirt] [PATCH] virsh: pool-info: introduce option --bytes

[John Ferlan]

On 01/03/2017 08:01 PM, Chen Hanxiao wrote:
> From: Chen Hanxiao 
> 
> By default, pool-info will convert sizes to human friendly
> units.
> This patch will introduce option [--bytes].
> If specified, the raw sizes will be in the output.
> 
> Signed-off-by: Chen Hanxiao 
> ---
>  tools/virsh-pool.c | 25 +++--
>  tools/virsh.pod|  5 +++--
>  2 files changed, 22 insertions(+), 8 deletions(-)
> 

ACK - I will also merge in a change to news.xml before pushing

John

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

Re: [libvirt] [PATCH 00/12] vbox: remove support for vbox 3.x and older.

[Dawid Zamirski]

On Tue, 2017-01-10 at 11:50 -0500, John Ferlan wrote:
> 
> Seems reasonable - it builds for me ;-).  I did take a cursory scan
> through the changes - they all seem valid.
> 
> ACK series and can push everything before the release once/if the
> news
> patches series on list can come to a resolution (otherwise, I'll use
> news.html.in as this one does). Doubtful anyone has reservations
> about
> removing all the cruft, but I will let the ACK sit for the rest of
> today
> to see if anyone gripes ;-).
> 
> 
> John
> 
> FWIW: I had to cleanup a few make syntax-check warnings in
> vbox_impl.c
> due to not needing extra spaces on preprocessor indents (#define
> instead
> of # define):
> 

Whoops indeed - I've forgot to run make syntax-check before final
submission. Those were the result of removing wrapping #if..#endif
preprocessor directives that were used for old vbox versions which
reduced nesting level.

Regards,
Dawid

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

Re: [libvirt] [PATCH] NEWS: Add trailing periods to all sentences

[Laine Stump]

ACK. I never liked the idea of multiple sentences where everything 
except the final sentence ends with a period (but it wasn't worth 
complaining about).


On 01/10/2017 01:57 PM, Andrea Bolognani wrote:

Suggested-by: John Ferlan 
---
  docs/news.xml | 30 +++---
  1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/docs/news.xml b/docs/news.xml
index 909b4ca..672e988 100644
--- a/docs/news.xml
+++ b/docs/news.xml
@@ -24,7 +24,7 @@
  
Add the capability to allow group I/O throttling via a new
domain   subelement "group_name"
-  to allow sharing I/O throttling quota between multiple drives
+  to allow sharing I/O throttling quota between multiple drives.
  


@@ -33,7 +33,7 @@
  
  
New libvirt-guest nss module that translates libvirt
-  guest names into IP addresses
+  guest names into IP addresses.
  


@@ -43,7 +43,7 @@
  
Logging-related settings like log outputs and filters can now be
adjusted during runtime using the admin interface without the
-  necessity of the daemon's restart
+  necessity of the daemon's restart.
  


@@ -55,7 +55,7 @@
flag in order to return the host physical size in bytes
of the image container in the allocation field of the
_virStorageVolInfo structure. The --physical flag has been
-  added to the virsh vol-info command to access the data
+  added to the virsh vol-info command to access the data.
  


@@ -73,7 +73,7 @@
Add support to get the count of branch instructions
executed, branch misses, bus cycles, stalled frontend
cpu cycles, stalled backend cpu cycles, and ref cpu
-  cycles by applications running on the platform
+  cycles by applications running on the platform.
  


@@ -82,7 +82,7 @@
  
  
Add a display of the  size of a disk
-  volume in the output of the volume XML
+  volume in the output of the volume XML.
  


@@ -93,7 +93,7 @@
virtio-pci provides several advantages over virtio-mmio, such
as the ability to hotplug devices and improved performance.
While opting in to virtio-pci has been possible for a while,
-  newly-defined guests will now use it automatically
+  newly-defined guests will now use it automatically.
  

  
@@ -106,7 +106,7 @@
For an active domain, correct the physical value provided for
a raw sparse file backed storage and the allocation value provided
for a qcow2 file backed storage that hasn't yet been opened on
-  the domain
+  the domain.
  


@@ -115,7 +115,7 @@
  
  
The chardev detection code has been improved and can now handle this
-  configuration properly
+  configuration properly.
  


@@ -138,7 +138,7 @@
  
  
The shmem device can now utilize QEMU's ivshmem-plain and
-  ivshmem-doorbell, more modern versions of ivshmem
+  ivshmem-doorbell, more modern versions of ivshmem.
  


@@ -153,7 +153,7 @@
  
The new libssh transport allows one to connect to a running
libvirtd via SSH, using the libssh library; for example:
-  qemu+libssh://server/system
+  qemu+libssh://server/system.
  


@@ -162,7 +162,7 @@
  
  
Add the capability to pass through a scsi_host HBA and the
-  associated LUNs to the guest
+  associated LUNs to the guest.
  


@@ -190,7 +190,7 @@
e1000e, nec-xhci, vfio assigned devices) will be placed on
an Express controller (i.e. a pcie-root-port) instead of a
legacy PCI controller (i.e. pci-bridge) with the root ports
-  added as needed
+  added as needed.
  

  
@@ -222,7 +222,7 @@
  
  
List user-visible changes instead of single commits for a better
-  high-level overview of differences between libvirt releases
+  high-level overview of differences between libvirt releases.
  


@@ -232,7 +232,7 @@
  
The libvirt website looked very cluttered and outdated; it has now
been completely overhauled, resulting in a design that's better
-  organized and more pleasant to look at
+  organized and more pleasant to look at.
  

  



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

[libvirt] [PATCH] NEWS: Add trailing periods to all sentences

[Andrea Bolognani]

Suggested-by: John Ferlan 
---
 docs/news.xml | 30 +++---
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/docs/news.xml b/docs/news.xml
index 909b4ca..672e988 100644
--- a/docs/news.xml
+++ b/docs/news.xml
@@ -24,7 +24,7 @@
 
   Add the capability to allow group I/O throttling via a new
   domain   subelement "group_name"
-  to allow sharing I/O throttling quota between multiple drives
+  to allow sharing I/O throttling quota between multiple drives.
 
   
   
@@ -33,7 +33,7 @@
 
 
   New libvirt-guest nss module that translates libvirt
-  guest names into IP addresses
+  guest names into IP addresses.
 
   
   
@@ -43,7 +43,7 @@
 
   Logging-related settings like log outputs and filters can now be
   adjusted during runtime using the admin interface without the
-  necessity of the daemon's restart
+  necessity of the daemon's restart.
 
   
   
@@ -55,7 +55,7 @@
   flag in order to return the host physical size in bytes
   of the image container in the allocation field of the
   _virStorageVolInfo structure. The --physical flag has been
-  added to the virsh vol-info command to access the data
+  added to the virsh vol-info command to access the data.
 
   
   
@@ -73,7 +73,7 @@
   Add support to get the count of branch instructions
   executed, branch misses, bus cycles, stalled frontend
   cpu cycles, stalled backend cpu cycles, and ref cpu
-  cycles by applications running on the platform
+  cycles by applications running on the platform.
 
   
   
@@ -82,7 +82,7 @@
 
 
   Add a display of the  size of a disk
-  volume in the output of the volume XML
+  volume in the output of the volume XML.
 
   
   
@@ -93,7 +93,7 @@
   virtio-pci provides several advantages over virtio-mmio, such
   as the ability to hotplug devices and improved performance.
   While opting in to virtio-pci has been possible for a while,
-  newly-defined guests will now use it automatically
+  newly-defined guests will now use it automatically.
 
   
 
@@ -106,7 +106,7 @@
   For an active domain, correct the physical value provided for
   a raw sparse file backed storage and the allocation value provided
   for a qcow2 file backed storage that hasn't yet been opened on
-  the domain
+  the domain.
 
   
   
@@ -115,7 +115,7 @@
 
 
   The chardev detection code has been improved and can now handle this
-  configuration properly
+  configuration properly.
 
   
   
@@ -138,7 +138,7 @@
 
 
   The shmem device can now utilize QEMU's ivshmem-plain and
-  ivshmem-doorbell, more modern versions of ivshmem
+  ivshmem-doorbell, more modern versions of ivshmem.
 
   
   
@@ -153,7 +153,7 @@
 
   The new libssh transport allows one to connect to a running
   libvirtd via SSH, using the libssh library; for example:
-  qemu+libssh://server/system
+  qemu+libssh://server/system.
 
   
   
@@ -162,7 +162,7 @@
 
 
   Add the capability to pass through a scsi_host HBA and the
-  associated LUNs to the guest
+  associated LUNs to the guest.
 
   
   
@@ -190,7 +190,7 @@
   e1000e, nec-xhci, vfio assigned devices) will be placed on
   an Express controller (i.e. a pcie-root-port) instead of a
   legacy PCI controller (i.e. pci-bridge) with the root ports
-  added as needed
+  added as needed.
 
   
 
@@ -222,7 +222,7 @@
 
 
   List user-visible changes instead of single commits for a better
-  high-level overview of differences between libvirt releases
+  high-level overview of differences between libvirt releases.
 
   
   
@@ -232,7 +232,7 @@
 
   The libvirt website looked very cluttered and outdated; it has now
   been completely overhauled, resulting in a design that's better
-  organized and more pleasant to look at
+  organized and more pleasant to look at.
 
   
 
-- 
2.7.4

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

Re: [libvirt] [PATCH 2/3] remote: fix logic for known_hosts and keyfile checks

[Peter Krempa]

On Tue, Jan 10, 2017 at 19:43:19 +0100, Pino Toscano wrote:
> If any of them is specified for the libssh and libssh2 drivers, there is
> no need to depend on chec ks based on other paths: in particular, a

s/chec ks/checks/

> specified path for known_hosts was ignored if the local config directory
> could not be determined, and the path for keyfile was ignored if the
> home could not be determined.
> 
> Instead, lazily determine and use these two paths only in case they are
> needed.
> ---
>  src/rpc/virnetclient.c | 52 
> +++---
>  1 file changed, 28 insertions(+), 24 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 3/3] remote: do not check for an existing config dir

[Peter Krempa]

On Tue, Jan 10, 2017 at 19:43:20 +0100, Pino Toscano wrote:
> When composing the path to the default known_hosts file (for the libssh
> and libssh2 drivers), do not check whether the configuration directory
> (determined by virGetUserConfigDirectory()) exists: both the drivers can
> handle non-existing files, and are able to create them (and their
> directories) in that case.
> 
> This adds a small behaviour change: before, the key for an unknown host,
> and manually accepted, was saved only if the configuration directory
> existed -- a bit incoherent behaviour though.
> ---

ACK


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

[libvirt] [PATCH] maint: update to latest gnulib

[Eric Blake]

Among other recent changes, this includes a workaround to avoid
Clang compiler bug https://llvm.org/bugs/show_bug.cgi?id=16404
having spurious link failures.

* .gnulib: Update.
* bootstrap: Synchronize to upstream.

Signed-off-by: Eric Blake 
---

I'm debating about re-enabling test-lock (right now, we explicitly
pass --avoid=lock-tests to gnulib-tool via our bootstrap.conf), now
that upstream is finally starting to fix the bugs in the test that
were causing extremely slow completion or outright hangs on
multi-processor setups.  But that's probably something better done
right after a libvirt release, not right before a release candidate.

Pushing under the gnulib maintenance rule; contains these gnulib patches:

* .gnulib e210a3c...94386a1 (38):
  > maint.mk: enforce spelling of "timestamp" (i.e., no space)
  > dfa: minor simplification with emptyset
  > dfa: shrink constraints from 4 bits to 3
  > dfa: omit unnecessary ptrdiff_t check
  > dfa: omit unnecessary allocation
  > dfa: omit unused local
  > maint: time stamp -> timestamp
  > maint: remove stray .texi files
  > getprogname: fix port to IRIX
  > dfa: melt down dfastate into build_state
  > dfa: simplify transition table allocation
  > dfa: fix reallocation bug when matching newlines
  > Avoid -Wundef warning about undefined WINDOWS_SOCKETS.
  > Avoid -Wundef warning about undefined __USE_FILE_OFFSET64.
  > stdioext: Port to Minix 3.2 and newer.
  > getprogname: port to IRIX
  > localename-tests: port to NetBSD 7
  > glob, intprops, xalloc: work around Clang bug
  > dfa: fix 'return' typo
  > lock tests: Prefer semaphore over mutex.
  > parse-datetime: fix generated paths for coverage files
  > maint.mk: support parallel execution of coverage
  > lock: Provide guarantee to avoid writer starvation for rwlocks.
  > thread: Fix pth port.
  > parse-datetime: fix debug message on lone year number
  > parse-datetime: fix local timezone debug messages
  > parse-datetime: add debug warning about DST changes
  > parse-datetime: add debug warning about date arithmetic
  > parse-datetime: fix debug message of relative part after timezone
  > parse-datetime: fix incorrect debug message on lone number
  > exec
  > build-aux/mdate-sh
  > doc: fix typo in previous change
  > Revert copyright-year change to synced files
  > doc: modernize for C11 etc.
  > dfa: prefer functions to FETCH_WC macro
  > dfa: narrow more local var scopes
  > dfa: remove duplicate assignment

 .gnulib   | 2 +-
 bootstrap | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/.gnulib b/.gnulib
index e210a3c..94386a1 16
--- a/.gnulib
+++ b/.gnulib
@@ -1 +1 @@
-Subproject commit e210a3cbaec0ee82a67ff8fc427e21bdd64dba1b
+Subproject commit 94386a13667c645fd42544a7fd302c39fcdf
diff --git a/bootstrap b/bootstrap
index 26066b2..932ff85 100755
--- a/bootstrap
+++ b/bootstrap
@@ -1,6 +1,6 @@
 #! /bin/sh
 # Print a version string.
-scriptversion=2016-11-03.18; # UTC
+scriptversion=2017-01-09.19; # UTC

 # Bootstrap this package from checked-out sources.

@@ -790,7 +790,7 @@ symlink_to_dir()
   # Leave any existing symlink alone, if it already points to the source,
   # so that broken build tools that care about symlink times
   # aren't confused into doing unnecessary builds.  Conversely, if the
-  # existing symlink's time stamp is older than the source, make it afresh,
+  # existing symlink's timestamp is older than the source, make it afresh,
   # so that broken tools aren't confused into skipping needed builds.  See
   # .
   test -h "$dst" &&
-- 
2.9.3

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

Re: [libvirt] [PATCH 1/3] rpc: libssh: allow a NULL known_hosts file

[Peter Krempa]

On Tue, Jan 10, 2017 at 19:43:18 +0100, Pino Toscano wrote:
> Make sure that virNetLibsshSessionSetHostKeyVerification accepts a NULL
> value for the path to the known_hosts file:
> - call ssh_options_set(SSH_OPTIONS_KNOWNHOSTS) anyway, using /dev/null,
>   otherwise libssh will use its default path
> - do not call ssh_write_knownhost when no known hosts file was set
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1406457
> ---
>  src/rpc/virnetlibsshsession.c | 37 +++--
>  1 file changed, 23 insertions(+), 14 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 v4] libxl: define a per-domain logger.

[Jim Fehlig]

Cédric Bosdonnat wrote:
> libxl doesn't provide a way to write one log for each domain. Thus
> we need to demux the messages. If our logger doesn't know to which
> domain to attribute a message, then it will write it to the default
> log file.
> 
> Starting with Xen 4.9 (commit f9858025 and following), libxl will
> write the domain ID in an easy to grab manner. The logger introduced
> by this commit will use it to demux the libxl log messages.
> 
> Thanks to the default log file, this logger will also work with older
> versions of Xen.
> ---
>  * v4: delay the log file opening to the first message write.

Hmm, in hindsight I shouldn't have mentioned this in V3 since we'll always have
at least the json config written to the file correct? If so, there's really no
reason to delay opening it. Sorry for the confusion.

I tend to prefer V3, but with the nits I pointed out there fixed :-).

Regards,
Jim

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

Re: [libvirt] [PATCH v3 0/3] Improve release notes process

[Andrea Bolognani]

On Tue, 2017-01-10 at 13:28 -0500, John Ferlan wrote:
> > So, will you ACK this series if I promise to follow up with
> > a patch that adds leading periods to all s?
> 
> Sure, ACK with the caveat that I barely know how to spell xlst.

Is the misspelling intentional? If so, good one! If not,
you've definitely proven your point ;)

> Let's see what we can break before the freeze ;-)

I've pushed it now, so we'll know soon enough.


Now, if you'll excuse me, there's a follow-up patch that
I need to be working on... ;)

-- 
Andrea Bolognani / Red Hat / Virtualization

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

[libvirt] [PATCH 0/4] gluster: cleanup and fix code for pool discovery

[Peter Krempa]

Peter Krempa (4):
  storage: Fix error reporting when looking up storage pool sources
  storage: gluster: Report error if no volumes were found in pool lookup
  storage: gluster: Remove build-time dependency on the 'gluster' cli
tool
  spec: Depend on the gluster command line tool

 configure.ac  |  2 +-
 libvirt.spec.in   |  4 +++
 src/storage/storage_backend.c | 48 ---
 src/storage/storage_backend.h |  3 ++-
 src/storage/storage_backend_fs.c  | 22 ++--
 src/storage/storage_backend_gluster.c | 14 +++---
 6 files changed, 60 insertions(+), 33 deletions(-)

-- 
2.11.0

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

[libvirt] [PATCH 3/4] storage: gluster: Remove build-time dependency on the 'gluster' cli tool

[Peter Krempa]

The tool is used for pool discovery. Since we call an external binary we
don't really need to compile out the code that uses it. We can check
whether it exists at runtime.
---
 configure.ac  |  2 +-
 src/storage/storage_backend.c | 29 -
 src/storage/storage_backend.h |  3 ++-
 src/storage/storage_backend_fs.c  |  4 +---
 src/storage/storage_backend_gluster.c |  2 +-
 5 files changed, 21 insertions(+), 19 deletions(-)

diff --git a/configure.ac b/configure.ac
index c67ba79c7..9e41f8983 100644
--- a/configure.ac
+++ b/configure.ac
@@ -605,7 +605,7 @@ LIBVIRT_STORAGE_CHECK_ZFS

 if test "$with_storage_fs" = "yes" ||
test "$with_storage_gluster" = "yes"; then
-  AC_PATH_PROG([GLUSTER_CLI], [gluster], [], [$LIBVIRT_SBIN_PATH])
+  AC_PATH_PROG([GLUSTER_CLI], [gluster], [gluster], [$LIBVIRT_SBIN_PATH])
   if test "x$GLUSTER_CLI" != "x"; then
   AC_DEFINE_UNQUOTED([GLUSTER_CLI], ["$GLUSTER_CLI"],
 [Location or name of the gluster command line tool])
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
index 6c9706897..65a893679 100644
--- a/src/storage/storage_backend.c
+++ b/src/storage/storage_backend.c
@@ -2548,12 +2548,12 @@ virStorageBackendVolWipeLocal(virConnectPtr conn 
ATTRIBUTE_UNUSED,
 }


-#ifdef GLUSTER_CLI
 /**
  * virStorageBackendFindGlusterPoolSources:
  * @host: host to detect volumes on
  * @pooltype: src->format is set to this value
  * @list: list of storage pool sources to be filled
+ * @report: report error if the 'gluster' cli tool is missing
  *
  * Looks up gluster volumes on @host and fills them to @list.
  *
@@ -2562,8 +2562,10 @@ virStorageBackendVolWipeLocal(virConnectPtr conn 
ATTRIBUTE_UNUSED,
 int
 virStorageBackendFindGlusterPoolSources(const char *host,
 int pooltype,
-virStoragePoolSourceListPtr list)
+virStoragePoolSourceListPtr list,
+bool report)
 {
+char *glusterpath = NULL;
 char *outbuf = NULL;
 virCommandPtr cmd = NULL;
 xmlDocPtr doc = NULL;
@@ -2576,7 +2578,17 @@ virStorageBackendFindGlusterPoolSources(const char *host,

 int ret = -1;

-cmd = virCommandNewArgList(GLUSTER_CLI,
+if (!(glusterpath = virFindFileInPath(GLUSTER_CLI))) {
+if (report) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("'gluster' command line tool not found"));
+return -1;
+} else {
+return 0;
+}
+}
+
+cmd = virCommandNewArgList(glusterpath,
"--xml",
"--log-file=/dev/null",
"volume", "info", "all", NULL);
@@ -2629,18 +2641,9 @@ virStorageBackendFindGlusterPoolSources(const char *host,
 xmlFreeDoc(doc);
 VIR_FREE(outbuf);
 virCommandFree(cmd);
+VIR_FREE(glusterpath);
 return ret;
 }
-#else /* #ifdef GLUSTER_CLI */
-int
-virStorageBackendFindGlusterPoolSources(const char *host ATTRIBUTE_UNUSED,
-int pooltype ATTRIBUTE_UNUSED,
-virStoragePoolSourceListPtr list 
ATTRIBUTE_UNUSED)
-{
-VIR_INFO("gluster cli tool not installed");
-return 0;
-}
-#endif /* #ifdef GLUSTER_CLI */


 #if WITH_BLKID
diff --git a/src/storage/storage_backend.h b/src/storage/storage_backend.h
index 7ae8d58db..3f0403907 100644
--- a/src/storage/storage_backend.h
+++ b/src/storage/storage_backend.h
@@ -135,7 +135,8 @@ 
virStorageBackendGetBuildVolFromFunction(virStorageVolDefPtr vol,

 int virStorageBackendFindGlusterPoolSources(const char *host,
 int pooltype,
-virStoragePoolSourceListPtr list);
+virStoragePoolSourceListPtr list,
+bool report);

 int virStorageBackendVolUploadLocal(virConnectPtr conn,
 virStoragePoolObjPtr pool,
diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c
index b915fd58b..53c34335f 100644
--- a/src/storage/storage_backend_fs.c
+++ b/src/storage/storage_backend_fs.c
@@ -306,15 +306,13 @@ 
virStorageBackendFileSystemNetFindPoolSources(virConnectPtr conn ATTRIBUTE_UNUSE

 retNFS = virStorageBackendFileSystemNetFindNFSPoolSources(&state);

-# ifdef GLUSTER_CLI
 retGluster = virStorageBackendFindGlusterPoolSources(state.host,
  
VIR_STORAGE_POOL_NETFS_GLUSTERFS,
- &state.list);
+ &state.list, false);

 if (retGluster < 0)
 goto cleanup;

-# endif
 /* If both fail, then we won't return an empty list

[libvirt] [PATCH 2/4] storage: gluster: Report error if no volumes were found in pool lookup

[Peter Krempa]

Similarly to the 'netfs' pool, return an error if the host does not have
any volumes.
---
 src/storage/storage_backend_gluster.c | 14 +++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/src/storage/storage_backend_gluster.c 
b/src/storage/storage_backend_gluster.c
index ae0611543..0bd40f742 100644
--- a/src/storage/storage_backend_gluster.c
+++ b/src/storage/storage_backend_gluster.c
@@ -490,6 +490,7 @@ virStorageBackendGlusterFindPoolSources(virConnectPtr conn 
ATTRIBUTE_UNUSED,
 };
 virStoragePoolSourcePtr source = NULL;
 char *ret = NULL;
+int rc;
 size_t i;

 virCheckFlags(0, NULL);
@@ -510,11 +511,18 @@ virStorageBackendGlusterFindPoolSources(virConnectPtr 
conn ATTRIBUTE_UNUSED,
 goto cleanup;
 }

-if (virStorageBackendFindGlusterPoolSources(source->hosts[0].name,
-0, /* currently ignored */
-&list) < 0)
+if ((rc = virStorageBackendFindGlusterPoolSources(source->hosts[0].name,
+  0, /* currently ignored 
*/
+  &list)) < 0)
 goto cleanup;

+if (rc == 0) {
+virReportError(VIR_ERR_OPERATION_FAILED,
+   _("no storage pools were found on host '%s'"),
+   source->hosts[0].name);
+goto cleanup;
+}
+
 if (!(ret = virStoragePoolSourceListFormat(&list)))
 goto cleanup;

-- 
2.11.0

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

[libvirt] [PATCH 4/4] spec: Depend on the gluster command line tool

[Peter Krempa]

The gluster command line tool that is used to lookup storage pool
sources moved from the gluster-client package to gluster-cli.

Unfortunately the location differs between fedora and rhel so depend
on the package rather than binary.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1349441
---
 libvirt.spec.in | 4 
 1 file changed, 4 insertions(+)

diff --git a/libvirt.spec.in b/libvirt.spec.in
index 51ffbb55c..a76f6a9cf 100644
--- a/libvirt.spec.in
+++ b/libvirt.spec.in
@@ -595,6 +595,10 @@ Requires: util-linux
 %if 0%{?fedora}
 Requires: glusterfs-client >= 2.0.1
 %endif
+# gluster cli tool for pool discovery
+%if (0%{?fedora} || 0%{?with_storage_gluster})
+Requires: gluster-cli
+%endif
 # For LVM drivers
 Requires: lvm2
 # For ISCSI driver
-- 
2.11.0

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

[libvirt] [PATCH 1/4] storage: Fix error reporting when looking up storage pool sources

[Peter Krempa]

In commit 4090e15399 we went back from reporting no errors if no storage
pools were found on a given host to reporting a bad error. And only in
cases when gluster was not installed.

Report a less bad error in case there are no volumes. Also report the
error when gluster is installed but no volumes were found, since
virStorageBackendFindGlusterPoolSources would return success in that
case.
---
 src/storage/storage_backend.c| 19 ---
 src/storage/storage_backend_fs.c | 20 ++--
 2 files changed, 26 insertions(+), 13 deletions(-)

diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
index 18433e9c7..6c9706897 100644
--- a/src/storage/storage_backend.c
+++ b/src/storage/storage_backend.c
@@ -2549,6 +2549,16 @@ virStorageBackendVolWipeLocal(virConnectPtr conn 
ATTRIBUTE_UNUSED,


 #ifdef GLUSTER_CLI
+/**
+ * virStorageBackendFindGlusterPoolSources:
+ * @host: host to detect volumes on
+ * @pooltype: src->format is set to this value
+ * @list: list of storage pool sources to be filled
+ *
+ * Looks up gluster volumes on @host and fills them to @list.
+ *
+ * Returns number of volumes on the host on success, or -1 on error.
+ */
 int
 virStorageBackendFindGlusterPoolSources(const char *host,
 int pooltype,
@@ -2578,8 +2588,6 @@ virStorageBackendFindGlusterPoolSources(const char *host,
 goto cleanup;

 if (rc != 0) {
-VIR_INFO("failed to query host '%s' for gluster volumes: %s",
- host, outbuf);
 ret = 0;
 goto cleanup;
 }
@@ -2588,11 +2596,8 @@ virStorageBackendFindGlusterPoolSources(const char *host,
   &ctxt)))
 goto cleanup;

-if ((nnodes = virXPathNodeSet("//volumes/volume", ctxt, &nodes)) <= 0) {
-VIR_INFO("no gluster volumes available on '%s'", host);
-ret = 0;
+if ((nnodes = virXPathNodeSet("//volumes/volume", ctxt, &nodes)) < 0)
 goto cleanup;
-}

 for (i = 0; i < nnodes; i++) {
 ctxt->node = nodes[i];
@@ -2616,7 +2621,7 @@ virStorageBackendFindGlusterPoolSources(const char *host,
 src->format = pooltype;
 }

-ret = 0;
+ret = nnodes;

  cleanup:
 VIR_FREE(nodes);
diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c
index f4341f32c..b915fd58b 100644
--- a/src/storage/storage_backend_fs.c
+++ b/src/storage/storage_backend_fs.c
@@ -281,7 +281,8 @@ virStorageBackendFileSystemNetFindPoolSources(virConnectPtr 
conn ATTRIBUTE_UNUSE
 virStoragePoolSourcePtr source = NULL;
 char *ret = NULL;
 size_t i;
-int retNFS = -1, retGluster = -1;
+int retNFS = -1;
+int retGluster = 0;

 virCheckFlags(0, NULL);

@@ -306,14 +307,21 @@ 
virStorageBackendFileSystemNetFindPoolSources(virConnectPtr conn ATTRIBUTE_UNUSE
 retNFS = virStorageBackendFileSystemNetFindNFSPoolSources(&state);

 # ifdef GLUSTER_CLI
-retGluster =
-virStorageBackendFindGlusterPoolSources(state.host,
-
VIR_STORAGE_POOL_NETFS_GLUSTERFS,
-&state.list);
+retGluster = virStorageBackendFindGlusterPoolSources(state.host,
+ 
VIR_STORAGE_POOL_NETFS_GLUSTERFS,
+ &state.list);
+
+if (retGluster < 0)
+goto cleanup;
+
 # endif
 /* If both fail, then we won't return an empty list - return an error */
-if (retNFS < 0 && retGluster < 0)
+if (retNFS < 0 && retGluster == 0) {
+virReportError(VIR_ERR_OPERATION_FAILED,
+   _("no storage pools were found on host '%s'"),
+   state.host);
 goto cleanup;
+}

 if (!(ret = virStoragePoolSourceListFormat(&state.list)))
 goto cleanup;
-- 
2.11.0

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

[libvirt] [PATCH 2/3] remote: fix logic for known_hosts and keyfile checks

[Pino Toscano]

If any of them is specified for the libssh and libssh2 drivers, there is
no need to depend on chec ks based on other paths: in particular, a
specified path for known_hosts was ignored if the local config directory
could not be determined, and the path for keyfile was ignored if the
home could not be determined.

Instead, lazily determine and use these two paths only in case they are
needed.
---
 src/rpc/virnetclient.c | 52 +++---
 1 file changed, 28 insertions(+), 24 deletions(-)

diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c
index 34475d9..9c781e3 100644
--- a/src/rpc/virnetclient.c
+++ b/src/rpc/virnetclient.c
@@ -450,32 +450,34 @@ virNetClientPtr virNetClientNewLibSSH2(const char *host,
 char *nc = NULL;
 char *command = NULL;
 
-char *homedir = virGetUserDirectory();
-char *confdir = virGetUserConfigDirectory();
+char *homedir = NULL;
+char *confdir = NULL;
 char *knownhosts = NULL;
 char *privkey = NULL;
 
 /* Use default paths for known hosts an public keys if not provided */
-if (confdir) {
-if (!knownHostsPath) {
+if (knownHostsPath) {
+if (VIR_STRDUP(knownhosts, knownHostsPath) < 0)
+goto cleanup;
+} else {
+confdir = virGetUserConfigDirectory();
+if (confdir) {
 if (virFileExists(confdir)) {
 virBufferAsprintf(&buf, "%s/known_hosts", confdir);
 if (!(knownhosts = virBufferContentAndReset(&buf)))
 goto no_memory;
 }
-} else {
-if (VIR_STRDUP(knownhosts, knownHostsPath) < 0)
-goto cleanup;
 }
 }
 
-if (homedir) {
-if (!privkeyPath) {
+if (privkeyPath) {
+if (VIR_STRDUP(privkey, privkeyPath) < 0)
+goto cleanup;
+} else {
+homedir = virGetUserDirectory();
+if (homedir) {
 if (virNetClientFindDefaultSshKey(homedir, &privkey) < 0)
 goto no_memory;
-} else {
-if (VIR_STRDUP(privkey, privkeyPath) < 0)
-goto cleanup;
 }
 }
 
@@ -559,31 +561,33 @@ virNetClientPtr virNetClientNewLibssh(const char *host,
 char *nc = NULL;
 char *command = NULL;
 
-char *homedir = virGetUserDirectory();
-char *confdir = virGetUserConfigDirectory();
+char *homedir = NULL;
+char *confdir = NULL;
 char *knownhosts = NULL;
 char *privkey = NULL;
 
 /* Use default paths for known hosts an public keys if not provided */
-if (confdir) {
-if (!knownHostsPath) {
+if (knownHostsPath) {
+if (VIR_STRDUP(knownhosts, knownHostsPath) < 0)
+goto cleanup;
+} else {
+confdir = virGetUserConfigDirectory();
+if (confdir) {
 if (virFileExists(confdir)) {
 if (virAsprintf(&knownhosts, "%s/known_hosts", confdir) < 0)
 goto cleanup;
 }
-} else {
-if (VIR_STRDUP(knownhosts, knownHostsPath) < 0)
-goto cleanup;
 }
 }
 
-if (homedir) {
-if (!privkeyPath) {
+if (privkeyPath) {
+if (VIR_STRDUP(privkey, privkeyPath) < 0)
+goto cleanup;
+} else {
+homedir = virGetUserDirectory();
+if (homedir) {
 if (virNetClientFindDefaultSshKey(homedir, &privkey) < 0)
 goto no_memory;
-} else {
-if (VIR_STRDUP(privkey, privkeyPath) < 0)
-goto cleanup;
 }
 }
 
-- 
2.7.4

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

[libvirt] [PATCH 1/3] rpc: libssh: allow a NULL known_hosts file

[Pino Toscano]

Make sure that virNetLibsshSessionSetHostKeyVerification accepts a NULL
value for the path to the known_hosts file:
- call ssh_options_set(SSH_OPTIONS_KNOWNHOSTS) anyway, using /dev/null,
  otherwise libssh will use its default path
- do not call ssh_write_knownhost when no known hosts file was set

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1406457
---
 src/rpc/virnetlibsshsession.c | 37 +++--
 1 file changed, 23 insertions(+), 14 deletions(-)

diff --git a/src/rpc/virnetlibsshsession.c b/src/rpc/virnetlibsshsession.c
index 5de6629..25f93ce 100644
--- a/src/rpc/virnetlibsshsession.c
+++ b/src/rpc/virnetlibsshsession.c
@@ -382,14 +382,16 @@ virNetLibsshCheckHostKey(virNetLibsshSessionPtr sess)
 VIR_FREE(askKey.result);
 }
 
-/* write the host key file */
-if (ssh_write_knownhost(sess->session) < 0) {
-errmsg = ssh_get_error(sess->session);
-virReportError(VIR_ERR_LIBSSH,
-   _("failed to write known_host file '%s': %s"),
-   sess->knownHostsFile,
-   errmsg);
-return -1;
+/* write the host key file, if specified */
+if (sess->knownHostsFile) {
+if (ssh_write_knownhost(sess->session) < 0) {
+errmsg = ssh_get_error(sess->session);
+virReportError(VIR_ERR_LIBSSH,
+   _("failed to write known_host file '%s': %s"),
+   sess->knownHostsFile,
+   errmsg);
+return -1;
+}
 }
 /* key was accepted and added */
 return 0;
@@ -1172,13 +1174,20 @@ 
virNetLibsshSessionSetHostKeyVerification(virNetLibsshSessionPtr sess,
 goto error;
 }
 
-/* set the known hosts file */
-if (ssh_options_set(sess->session, SSH_OPTIONS_KNOWNHOSTS, hostsfile) < 0)
-goto error;
+/* set the known hosts file, if specified */
+if (hostsfile) {
+if (ssh_options_set(sess->session, SSH_OPTIONS_KNOWNHOSTS, hostsfile) 
< 0)
+goto error;
 
-VIR_FREE(sess->knownHostsFile);
-if (VIR_STRDUP(sess->knownHostsFile, hostsfile) < 0)
-goto error;
+VIR_FREE(sess->knownHostsFile);
+if (VIR_STRDUP(sess->knownHostsFile, hostsfile) < 0)
+goto error;
+} else {
+/* libssh does not support trying no known_host file at all:
+ * hence use /dev/null here, without storing it as file */
+if (ssh_options_set(sess->session, SSH_OPTIONS_KNOWNHOSTS, 
"/dev/null") < 0)
+goto error;
+}
 
 virObjectUnlock(sess);
 return 0;
-- 
2.7.4

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

[libvirt] [PATCH 3/3] remote: do not check for an existing config dir

[Pino Toscano]

When composing the path to the default known_hosts file (for the libssh
and libssh2 drivers), do not check whether the configuration directory
(determined by virGetUserConfigDirectory()) exists: both the drivers can
handle non-existing files, and are able to create them (and their
directories) in that case.

This adds a small behaviour change: before, the key for an unknown host,
and manually accepted, was saved only if the configuration directory
existed -- a bit incoherent behaviour though.
---
 src/rpc/virnetclient.c | 14 +-
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c
index 9c781e3..5174614 100644
--- a/src/rpc/virnetclient.c
+++ b/src/rpc/virnetclient.c
@@ -462,11 +462,9 @@ virNetClientPtr virNetClientNewLibSSH2(const char *host,
 } else {
 confdir = virGetUserConfigDirectory();
 if (confdir) {
-if (virFileExists(confdir)) {
-virBufferAsprintf(&buf, "%s/known_hosts", confdir);
-if (!(knownhosts = virBufferContentAndReset(&buf)))
-goto no_memory;
-}
+virBufferAsprintf(&buf, "%s/known_hosts", confdir);
+if (!(knownhosts = virBufferContentAndReset(&buf)))
+goto no_memory;
 }
 }
 
@@ -573,10 +571,8 @@ virNetClientPtr virNetClientNewLibssh(const char *host,
 } else {
 confdir = virGetUserConfigDirectory();
 if (confdir) {
-if (virFileExists(confdir)) {
-if (virAsprintf(&knownhosts, "%s/known_hosts", confdir) < 0)
-goto cleanup;
-}
+if (virAsprintf(&knownhosts, "%s/known_hosts", confdir) < 0)
+goto cleanup;
 }
 }
 
-- 
2.7.4

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

Re: [libvirt] [PATCH] storage_backend_rbd: always call rados_conf_read_file when connect a rbd pool

[John Ferlan]

On 12/30/2016 03:39 AM, Chen Hanxiao wrote:
> From: Chen Hanxiao 
> 
> This patch fix a dead lock when try to read a rbd image
> 
> When trying to connect a rbd server
> (ceph-0.94.7-1.el7.centos.x86_64),
> 
> rbd_list/rbd_open enter a dead lock state.
> 
> Backtrace:
> Thread 30 (Thread 0x7fdb342d0700 (LWP 12105)):
> #0  0x7fdb40b16705 in pthread_cond_wait@@GLIBC_2.3.2 () from 
> /lib64/libpthread.so.0
> #1  0x7fdb294273f1 in librados::IoCtxImpl::operate_read(object_t const&, 
> ObjectOperation*, ceph::buffer::list*, int) () from /lib64/librados.so.2
> #2  0x7fdb29429fcc in librados::IoCtxImpl::read(object_t const&, 
> ceph::buffer::list&, unsigned long, unsigned long) () from 
> /lib64/librados.so.2
> #3  0x7fdb293e850c in librados::IoCtx::read(std::string const&, 
> ceph::buffer::list&, unsigned long, unsigned long) () from 
> /lib64/librados.so.2
> #4  0x7fdb2b9dd15e in librbd::list(librados::IoCtx&, 
> std::vector >&) () from 
> /lib64/librbd.so.1
> #5  0x7fdb2b98c089 in rbd_list () from /lib64/librbd.so.1
> #6  0x7fdb2e1a8052 in virStorageBackendRBDRefreshPool (conn= out>, pool=0x7fdafc002d50) at storage/storage_backend_rbd.c:366
> #7  0x7fdb2e193833 in storagePoolCreate (obj=0x7fdb1c1fd5a0, 
> flags=) at storage/storage_driver.c:876
> #8  0x7fdb43790ea1 in virStoragePoolCreate 
> (pool=pool@entry=0x7fdb1c1fd5a0, flags=0) at libvirt-storage.c:695
> #9  0x7fdb443becdf in remoteDispatchStoragePoolCreate 
> (server=0x7fdb45fb2ab0, msg=0x7fdb45fb3db0, args=0x7fdb1c0037d0, 
> rerr=0x7fdb342cfc30, client=) at remote_dispatch.h:14383
> #10 remoteDispatchStoragePoolCreateHelper (server=0x7fdb45fb2ab0, 
> client=, msg=0x7fdb45fb3db0, rerr=0x7fdb342cfc30, 
> args=0x7fdb1c0037d0, ret=0x7fdb1c1b3260) at remote_dispatch.h:14359
> #11 0x7fdb437d9c42 in virNetServerProgramDispatchCall 
> (msg=0x7fdb45fb3db0, client=0x7fdb45fd1a80, server=0x7fdb45fb2ab0, 
> prog=0x7fdb45fcd670) at rpc/virnetserverprogram.c:437
> #12 virNetServerProgramDispatch (prog=0x7fdb45fcd670, 
> server=server@entry=0x7fdb45fb2ab0, client=0x7fdb45fd1a80, 
> msg=0x7fdb45fb3db0) at rpc/virnetserverprogram.c:307
> #13 0x7fdb437d4ebd in virNetServerProcessMsg (msg=, 
> prog=, client=, srv=0x7fdb45fb2ab0) at 
> rpc/virnetserver.c:135
> #14 virNetServerHandleJob (jobOpaque=, opaque=0x7fdb45fb2ab0) 
> at rpc/virnetserver.c:156
> #15 0x7fdb436cfb35 in virThreadPoolWorker 
> (opaque=opaque@entry=0x7fdb45fa7650) at util/virthreadpool.c:145
> #16 0x7fdb436cf058 in virThreadHelper (data=) at 
> util/virthread.c:206
> #17 0x7fdb40b12df5 in start_thread () from /lib64/libpthread.so.0
> #18 0x7fdb408401ad in clone () from /lib64/libc.so.6
> 
> 366 len = rbd_list(ptr.ioctx, names, &max_size);
> (gdb) n
> [New Thread 0x7fdb20758700 (LWP 22458)]
> [New Thread 0x7fdb20556700 (LWP 22459)]
> [Thread 0x7fdb20758700 (LWP 22458) exited]
> [New Thread 0x7fdb20455700 (LWP 22460)]
> [Thread 0x7fdb20556700 (LWP 22459) exited]
> [New Thread 0x7fdb20556700 (LWP 22461)]
> 
> infinite loop...
> 
> Signed-off-by: Chen Hanxiao 
> ---
>  src/storage/storage_backend_rbd.c | 7 +++
>  1 file changed, 7 insertions(+)
> 

Could you provide a bit more context...

Why does calling rados_conf_read_file with a NULL resolve the issue?

Is this something "new" or "expected"? And if expected, why are we only
seeing it now?

What is the other thread that "has" the lock doing?

>From my cursory/quick read of :

http://docs.ceph.com/docs/master/rados/api/librados/

...
"Then you configure your rados_t to connect to your cluster, either by
setting individual values (rados_conf_set()), using a configuration file
(rados_conf_read_file()), using command line options
(rados_conf_parse_argv()), or an environment variable
(rados_conf_parse_env()):"

Since we use rados_conf_set, that would seem to indicate we're OK. It's
not clear from just what's posted why calling eventually calling
rbd_list is causing a hang.

I don't have the cycles or environment to do the research right now and
it really isn't clear why a read_file would resolve the issue.

John
> diff --git a/src/storage/storage_backend_rbd.c 
> b/src/storage/storage_backend_rbd.c
> index b1c51ab..233737b 100644
> --- a/src/storage/storage_backend_rbd.c
> +++ b/src/storage/storage_backend_rbd.c
> @@ -95,6 +95,9 @@ 
> virStorageBackendRBDOpenRADOSConn(virStorageBackendRBDStatePtr ptr,
>  goto cleanup;
>  }
>  
> +/* try default location, but ignore failure */
> +rados_conf_read_file(ptr->cluster, NULL);
> +
>  if (!conn) {
>  virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> _("'ceph' authentication not supported "
> @@ -124,6 +127,10 @@ 
> virStorageBackendRBDOpenRADOSConn(virStorageBackendRBDStatePtr ptr,
> _("failed to create the RADOS cluster"));
>  goto cleanup;
>  }
> +
> +/* try default location, but ignore failure */
> +  

Re: [libvirt] [PATCH v3 0/3] Improve release notes process

[John Ferlan]

On 01/10/2017 11:55 AM, Andrea Bolognani wrote:
> On Tue, 2017-01-10 at 11:23 -0500, John Ferlan wrote:
>> I'm "mostly" fine with the changes - would like resolution sooner than
>> later since I have a couple of things to add to news, but I didn't want
>> to cause you extra merge cycles ;-)
> 
> Yeah, same here :)
> 
>> The *one* thing I'm really having a hard time with is not ending
>> sentences with a period for 's.
> 
> What about ?
> 
>> Especially when there's
>> two or more sentences within a  where the first one ends
>> with a period, but the second one doesn't
>>  
>> Not ending the previous paragraph without a period was really difficult
>> for my fingers.  I wouldn't hold up a ACK though for having or not
>> having a period.
> 
> Patch 1/3 translates the current entries directly to the new
> format, without any kind of editing.
> 
> As I've said before, I have zero problems having leading
> periods for all sentences in the release notes, especially
> if you feel so strongly about it :) We will have to change
> them all, though: an inconsistent style just won't do.
> 
> So, will you ACK this series if I promise to follow up with
> a patch that adds leading periods to all s?

Sure, ACK with the caveat that I barely know how to spell xlst.

Let's see what we can break before the freeze ;-)

John

> 
> -- 
> Andrea Bolognani / Red Hat / Virtualization
> 

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

Re: [libvirt] [PATCH 0/2] Couple of build fixes

[Andrea Bolognani]

On Tue, 2017-01-10 at 16:33 +0100, Michal Privoznik wrote:
> Spotted by our CI.
> 
> Michal Privoznik (2):
>   qemu_domain: Move qemuDomainGetPreservedMounts
>   virSecuritySELinuxSetFileconHelper: Fix build with broken selinux.h
> 
>  src/qemu/qemu_domain.c  | 140 
>
>  src/security/security_selinux.c |   2 +-
>  2 files changed, 71 insertions(+), 71 deletions(-)

ACK series (and pushed).

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [PATCH] libxl: implement virDomainGetMaxVcpus

[Jim Fehlig]

Pavel Hrdina wrote:
> On Fri, Jan 06, 2017 at 12:35:21PM -0700, Jim Fehlig wrote:
>> The libxl driver already supports getting maximum vcpu count via
>> libxlDomainGetVcpusFlags, allowing to trivially implement
>> virDomainGetMaxVcpus.
>>
>> Signed-off-by: Jim Fehlig 
>> ---
>>  docs/news.html.in| 2 ++
>>  src/libxl/libxl_driver.c | 8 
>>  2 files changed, 10 insertions(+)
> 
> ACK

Thanks! Pushed now.

Regards,
Jim

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

Re: [libvirt] [PATCH 2/2] qemu: use virDomainPCIAddressSetAllMulti() to set multi when needed

[Andrea Bolognani]

On Tue, 2017-01-10 at 03:23 -0500, Laine Stump wrote:
> If there are multiple devices assigned to the different functions of a
> single PCI slot, they will not work properly if the device at function
> 0 doesn't have its "multi" attribute turned on, so it makes sense for
> libvirt to turn it on during PCI address assignment. Just in case a
> future change in the configuration eliminates the devices on all non-0
> functions, and in case some guest OS would be confused by having the
> multi flag of function 0 modified to "no" at the next boot of the
> guest (it may seem like an inconsequential guest ABI change, but it
> *is* a guest ABI change).
> 
> We are set multi during PCI address assignment (which happens in the
> domain post-parse callback) rather than at qemu runtime so that the
> setting will be stored in config and persist even in the case that the
> devices in the non-0 functions of the slot are later removed from the
> config - this is done to prevent a "silent" guest ABI change (although
> it can only happen after shutting down and restarting the guest, and
> although it may seem like an inconsequential guest ABI change, it *is*
> a guest ABI change to turn off the multi bit).

The commit messages seems to contain editing artifacts that
I trust you'll get rid of before pushing.

[...]
> +/* set multi attribute for devices at function 0 of
> + * any slot that has multiple functions in use
> + */
> +virDomainPCIAddressSetAllMulti(def, addrs);

This will of course have to be changed now that
virDomainPCIAddressSetAllMulti() takes a single argument.

ACK

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [PATCH v2 1/2] conf: new function virDomainPCIAddressSetAllMulti()

[Andrea Bolognani]

On Tue, 2017-01-10 at 11:55 -0500, Laine Stump wrote:
> This utility function iterates through all devices looking for any
> with a PCI address that has function != 0 (which implies that multiple
> functions are in use on that slot), then uses an inner iterator to
> find the device that's on function 0 of that same slot and sets the
> "multi" in its virDomainDeviceInfo (as long as it hasn't already been
> set explicitly by someone who presumably has better information than
> we do).
> 
> It isn't yet called from anywhere, so will have no functional effect.

[...]
> +static int
> +virDomainPCIAddressSetMultiIter(virDomainDefPtr def ATTRIBUTE_UNUSED,
> +virDomainDeviceDefPtr dev ATTRIBUTE_UNUSED,
> +virDomainDeviceInfoPtr info,
> +void *data)

virDomainPCIAddressSetAllMultiIterInner()?
Or maybe virDomainPCIAddressSetAllMultiInnerIter()?

Just kidding ;)

[...]
> +static int
> +virDomainPCIAddressSetAllMultiIter(virDomainDefPtr def,
> +   virDomainDeviceDefPtr dev 
> ATTRIBUTE_UNUSED,
> +   virDomainDeviceInfoPtr info,
> +   void *data ATTRIBUTE_UNUSED)
> +{
> +virPCIDeviceAddressPtr testAddr;
> +
> +if (!info || info->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI)
> +   return 0;
> +
> +testAddr = &info->addr.pci;

Empty line here.

> +if (testAddr->function != 0) {
> +ignore_value(virDomainDeviceInfoIterate(def,
> +
> virDomainPCIAddressSetMultiIter,
> +testAddr));
> +}
> +
> +return 0;
> +}

ACK

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [PATCH v3 0/3] Improve release notes process

[Andrea Bolognani]

On Tue, 2017-01-10 at 11:23 -0500, John Ferlan wrote:
> I'm "mostly" fine with the changes - would like resolution sooner than
> later since I have a couple of things to add to news, but I didn't want
> to cause you extra merge cycles ;-)

Yeah, same here :)

> The *one* thing I'm really having a hard time with is not ending
> sentences with a period for 's.

What about ?

> Especially when there's
> two or more sentences within a  where the first one ends
> with a period, but the second one doesn't
> 
> Not ending the previous paragraph without a period was really difficult
> for my fingers.  I wouldn't hold up a ACK though for having or not
> having a period.

Patch 1/3 translates the current entries directly to the new
format, without any kind of editing.

As I've said before, I have zero problems having leading
periods for all sentences in the release notes, especially
if you feel so strongly about it :) We will have to change
them all, though: an inconsistent style just won't do.

So, will you ACK this series if I promise to follow up with
a patch that adds leading periods to all s?

-- 
Andrea Bolognani / Red Hat / Virtualization

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

[libvirt] [PATCH v2 1/2] conf: new function virDomainPCIAddressSetAllMulti()

[Laine Stump]

This utility function iterates through all devices looking for any
with a PCI address that has function != 0 (which implies that multiple
functions are in use on that slot), then uses an inner iterator to
find the device that's on function 0 of that same slot and sets the
"multi" in its virDomainDeviceInfo (as long as it hasn't already been
set explicitly by someone who presumably has better information than
we do).

It isn't yet called from anywhere, so will have no functional effect.
---

Change from V1: implement as two nested deviceinfo iterators as
suggested by Andrea.

 src/conf/domain_addr.c   | 82 
 src/conf/domain_addr.h   |  3 ++
 src/libvirt_private.syms |  1 +
 3 files changed, 86 insertions(+)

diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c
index 4d14ac4..453f287 100644
--- a/src/conf/domain_addr.c
+++ b/src/conf/domain_addr.c
@@ -867,6 +867,88 @@ 
virDomainPCIAddressReserveNextSlot(virDomainPCIAddressSetPtr addrs,
 }
 
 
+static int
+virDomainPCIAddressSetMultiIter(virDomainDefPtr def ATTRIBUTE_UNUSED,
+virDomainDeviceDefPtr dev ATTRIBUTE_UNUSED,
+virDomainDeviceInfoPtr info,
+void *data)
+{
+virPCIDeviceAddressPtr testAddr = data;
+virPCIDeviceAddressPtr thisAddr;
+
+if (!info || info->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI)
+   return 0;
+
+thisAddr = &info->addr.pci;
+
+if (thisAddr->domain == testAddr->domain &&
+thisAddr->bus == testAddr->bus &&
+thisAddr->slot == testAddr->slot &&
+thisAddr->function == 0) {
+
+/* only set to ON if it wasn't previously set
+ * (assuming that the user must have better information
+ * than us if they explicitly set it OFF)
+ */
+if (thisAddr->multi == VIR_TRISTATE_SWITCH_ABSENT)
+thisAddr->multi = VIR_TRISTATE_SWITCH_ON;
+
+return -1; /* finish early, *NOT* an error */
+}
+
+return 0;
+}
+
+
+static int
+virDomainPCIAddressSetAllMultiIter(virDomainDefPtr def,
+   virDomainDeviceDefPtr dev ATTRIBUTE_UNUSED,
+   virDomainDeviceInfoPtr info,
+   void *data ATTRIBUTE_UNUSED)
+{
+virPCIDeviceAddressPtr testAddr;
+
+if (!info || info->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI)
+   return 0;
+
+testAddr = &info->addr.pci;
+if (testAddr->function != 0) {
+ignore_value(virDomainDeviceInfoIterate(def,
+
virDomainPCIAddressSetMultiIter,
+testAddr));
+}
+
+return 0;
+}
+
+
+/**
+ * virDomainPCIAddressSetAllMulti():
+ *
+ * @def: the domain definition whose devices may need adjusting
+ * @addrs: address set keeping track of all addresses in use.
+ *
+ * Look for any PCI slots that have multiple functions assigned, and
+ * set multi to ON in the address for the device at function 0
+ * (unless it has been explicitly set to OFF).
+ *
+ * No return code, since there is no possibility of failure.
+ */
+void
+virDomainPCIAddressSetAllMulti(virDomainDefPtr def)
+{
+/* Use nested iterators over all the devices - the outer iterator
+ * scans through all the devices looking for those whose address
+ * has a non-0 function; when one is found, the inner iterator looks
+ * for the device that uses function 0 on the same slot and marks
+ * it as multi = ON
+ */
+ignore_value(virDomainDeviceInfoIterate(def,
+virDomainPCIAddressSetAllMultiIter,
+NULL));
+}
+
+
 static char*
 virDomainCCWAddressAsString(virDomainDeviceCCWAddressPtr addr)
 {
diff --git a/src/conf/domain_addr.h b/src/conf/domain_addr.h
index 5f0924e..c728c00 100644
--- a/src/conf/domain_addr.h
+++ b/src/conf/domain_addr.h
@@ -173,6 +173,9 @@ int 
virDomainPCIAddressReserveNextSlot(virDomainPCIAddressSetPtr addrs,
virDomainPCIConnectFlags flags)
 ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
 
+void virDomainPCIAddressSetAllMulti(virDomainDefPtr def)
+ATTRIBUTE_NONNULL(1);
+
 struct _virDomainCCWAddressSet {
 virHashTablePtr defined;
 virDomainDeviceCCWAddress next;
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 82adf7e..bdc0272 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -104,6 +104,7 @@ virDomainPCIAddressReserveAddr;
 virDomainPCIAddressReserveNextAddr;
 virDomainPCIAddressReserveNextSlot;
 virDomainPCIAddressReserveSlot;
+virDomainPCIAddressSetAllMulti;
 virDomainPCIAddressSetAlloc;
 virDomainPCIAddressSetFree;
 virDomainPCIAddressSetGrow;
-- 
2.7.4

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

Re: [libvirt] [PATCH 00/12] vbox: remove support for vbox 3.x and older.

[John Ferlan]

On 12/29/2016 03:34 PM, Dawid Zamirski wrote:
> Hello,
> 
> This series removes support for legacy VirtualBox versions (3.x and
> older) that did not have upstream support for a while and won't even
> work on any relatively recent distro. The idea for doing this was
> first mentioned here [1] and this makes the driver code cleaner as
> the APIs for those old versions were quite different from v4.0+ and
> required abstractions to handle the differences.
> 
> Please note that the first patch in the series is sent to ML as a
> compressed attachment because it deletes VirtualBox SDK header files
> for those old verions which are large and the patch would not pass ML
> message size limit.
> 
> [1] https://www.redhat.com/archives/libvir-list/2016-November/msg01133.html
> 
> Regards,
> Dawid
> 
> Dawid Zamirski (12):
>   vbox: remove SDK header files for vbox 3 and older.
>   vbox: remove calls to *InstallUniformedAPI macros.
>   vbox: remove code for old API versions.
>   vbox: remove _vboxAttachDrivesOld
>   vbox: do not use IHardDisk anymore.
>   vbox: remove getMachineForSession flag.
>   vbox: remove domain events support.
>   vbox: remove code dealing with oldMediumInterface
>   vbox: IVRDxServer to IVRDEServer.
>   vbox: fix _displayTakeScreenShotPNGToArray
>   vbox: consolidate vbox IID structures.
>   docs: add news entry in improvements section.
> 
>  docs/news.html.in |8 +
>  src/Makefile.am   |4 -
>  src/vbox/vbox_CAPI_v2_2.h | 4869 ---
>  src/vbox/vbox_CAPI_v3_0.h | 5396 ---
>  src/vbox/vbox_CAPI_v3_1.h | 5297 --
>  src/vbox/vbox_CAPI_v3_2.h | 5625 
> -
>  src/vbox/vbox_V2_2.c  |   37 -
>  src/vbox/vbox_V3_0.c  |   37 -
>  src/vbox/vbox_V3_1.c  |   37 -
>  src/vbox/vbox_V3_2.c  |   37 -
>  src/vbox/vbox_XPCOMCGlue.h|2 +-
>  src/vbox/vbox_common.c|  402 +--
>  src/vbox/vbox_common.h|   13 +-
>  src/vbox/vbox_network.c   |   12 +-
>  src/vbox/vbox_storage.c   |   76 +-
>  src/vbox/vbox_tmpl.c  | 3006 ++
>  src/vbox/vbox_uniformed_api.h |  171 +-
>  17 files changed, 438 insertions(+), 24591 deletions(-)
>  delete mode 100644 src/vbox/vbox_CAPI_v2_2.h
>  delete mode 100644 src/vbox/vbox_CAPI_v3_0.h
>  delete mode 100644 src/vbox/vbox_CAPI_v3_1.h
>  delete mode 100644 src/vbox/vbox_CAPI_v3_2.h
>  delete mode 100644 src/vbox/vbox_V2_2.c
>  delete mode 100644 src/vbox/vbox_V3_0.c
>  delete mode 100644 src/vbox/vbox_V3_1.c
>  delete mode 100644 src/vbox/vbox_V3_2.c
> 

Seems reasonable - it builds for me ;-).  I did take a cursory scan
through the changes - they all seem valid.

ACK series and can push everything before the release once/if the news
patches series on list can come to a resolution (otherwise, I'll use
news.html.in as this one does). Doubtful anyone has reservations about
removing all the cruft, but I will let the ACK sit for the rest of today
to see if anyone gripes ;-).


John

FWIW: I had to cleanup a few make syntax-check warnings in vbox_impl.c
due to not needing extra spaces on preprocessor indents (#define instead
of # define):

cppi: src/vbox/vbox_tmpl.c: line 159: not properly indented
cppi: src/vbox/vbox_tmpl.c: line 160: not properly indented
cppi: src/vbox/vbox_tmpl.c: line 260: not properly indented
cppi: src/vbox/vbox_tmpl.c: line 261: not properly indented
cppi: src/vbox/vbox_tmpl.c: line 262: not properly indented
cppi: src/vbox/vbox_tmpl.c: line 263: not properly indented
cppi: src/vbox/vbox_tmpl.c: line 264: not properly indented
cppi: src/vbox/vbox_tmpl.c: line 266: not properly indented
cppi: src/vbox/vbox_tmpl.c: line 817: not properly indented
cppi: src/vbox/vbox_tmpl.c: line 819: not properly indented
cppi: src/vbox/vbox_tmpl.c: line 852: not properly indented
cppi: src/vbox/vbox_tmpl.c: line 855: not properly indented
cppi: src/vbox/vbox_tmpl.c: line 863: not properly indented
cppi: src/vbox/vbox_tmpl.c: line 865: not properly indented
cppi: src/vbox/vbox_tmpl.c: line 867: not properly indented
cppi: src/vbox/vbox_tmpl.c: line 893: not properly indented
cppi: src/vbox/vbox_tmpl.c: line 895: not properly indented
cppi: src/vbox/vbox_tmpl.c: line 1581: not properly indented
cppi: src/vbox/vbox_tmpl.c: line 1587: not properly indented
cppi: src/vbox/vbox_tmpl.c: line 1589: not properly indented
cppi: src/vbox/vbox_tmpl.c: line 1591: not properly indented
cppi: src/vbox/vbox_tmpl.c: line 1592: not properly indented
cppi: src/vbox/vbox_tmpl.c: line 1596: not properly indented
cppi: src/vbox/vbox_tmpl.c: line 1598: not properly indented
cppi: src/vbox/vbox_tmpl.c: line 1600: not properly indented
cppi: src/vbox/vbox_tmpl.c: line 1601: not properly indented

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

Re: [libvirt] TSC frequency configuration & invtsc migration (was Re: [PATCH 4/4] kvm: Allow migration with invtsc)

[Paolo Bonzini]

On 05/01/2017 14:00, Daniel P. Berrange wrote:
>> If you do this unconditionally, you have another problem: if
>> tsc-frequency is set explicitly, migration is only possible if
>> TSC frequency of the destination matches[1], or if TSC scaling is
>> supported by the destination. It's a good idea to set a TSC
>> frequency only if invtsc is enabled explicitly in the config.
> 
> If we don't set tsc-frequency and the TSC frequency doesn't
> match, does that mean the guest migration succeed, but suddenly
> sees different TSC frequency ?

That's the reason why kvmclock exists (or more precisely, the reason why
kvmclock is still useful even when hosts have invtsc).

Paolo

> I guess we we allowed that historically we can't break that
> now, so setting it only if invtsc is set seems reasonable.

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

Re: [libvirt] TSC frequency configuration & invtsc migration (was Re: [PATCH 4/4] kvm: Allow migration with invtsc)

[Paolo Bonzini]

On 05/01/2017 11:48, Marcelo Tosatti wrote:
>> Host A has TSC scaling, host B doesn't have TSC scaling. We want
>> to be able to start the VM on host A, and migrate to B. In this
>> case, the only possible solution is to use B's frequency when
>> starting the VM. The QEMU process doesn't have enough information
>> to make that decision.
> That is a good point. But again, its a special case and 
> should be supported by -cpu xxx,tsc-frequency=.

I don't think this is a scenario that can work reliably.  The computed
TSC frequency may vary by 0.5% or so on every boot (e.g. you may get
2497000 kHz or 2511000 kHz for a 2.5 GHz TSC).  You can start the VM on
host A, reboot host B, and then you'll be unable to migrate.

Paolo

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

Re: [libvirt] qemu-kvm blocked for more than 120 seconds when "libvirt-guests" is enabled

[Andrea Bolognani]

On Tue, 2017-01-10 at 14:32 +0100, Oscar Segarra wrote:
> :_

You've sent four pings to this thread over the course of a
mere six days. That's *way* too many.

I realize this issue might be very pressing for you, but that
doesn't exempt you from list etiquette. Moreover, as you can
probably imagine, most of the people frequenting the list
are just now returning to their computers after end-of-year
break and will have quite a backlog to work through.

Someone will get around to replying to your message in due
time; if a couple of weeks have passed and you still haven't
gotten any replies, *at that point* it's perfectly fine to
ping the list.

One final note: libvir-list is for the development of libvirt
itself, questions such as this one are better asked on the
libvirt-users list.

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [PATCH v3 0/3] Improve release notes process

[John Ferlan]

On 01/10/2017 10:25 AM, Andrea Bolognani wrote:
> Dan raised some concerns, but it doesn't look like they're
> critical enough for him to NACK the series; moreover, it's
> all stuff that we can easily address later on, so I'd like
> to move forward with this before the freeze.
> 
> Details of changes can be found in the single patches.
> 
> 
> Andrea Bolognani (3):
>   NEWS: Improve building pipeline
>   NEWS: Reformat at generation time
>   docs: Document the release notes process for contributors
> 
>  .gitignore|   1 +
>  HACKING   |   7 ++
>  Makefile.am   |  26 --
>  docs/Makefile.am  |  28 +-
>  docs/hacking.html.in  |  10 ++
>  docs/news-ascii.xsl   |  68 ++
>  docs/news-html.xsl|  96 +++
>  docs/news.html.in | 164 
>  docs/news.xml | 252 
> ++
>  docs/news.xsl |  50 --
>  docs/reformat-news.py | 104 +
>  11 files changed, 583 insertions(+), 223 deletions(-)
>  create mode 100644 docs/news-ascii.xsl
>  create mode 100644 docs/news-html.xsl
>  delete mode 100644 docs/news.html.in
>  create mode 100644 docs/news.xml
>  delete mode 100644 docs/news.xsl
>  create mode 100755 docs/reformat-news.py
> 

I'm "mostly" fine with the changes - would like resolution sooner than
later since I have a couple of things to add to news, but I didn't want
to cause you extra merge cycles ;-)

The *one* thing I'm really having a hard time with is not ending
sentences with a period for 's.  Especially when there's
two or more sentences within a  where the first one ends
with a period, but the second one doesn't

Not ending the previous paragraph without a period was really difficult
for my fingers.  I wouldn't hold up a ACK though for having or not
having a period.

John

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

Re: [libvirt] RFC: Use __attribute__ ((cleanup) in libvirt ?

[Daniel P. Berrange]

On Tue, Jan 10, 2017 at 05:05:21PM +0100, Michal Privoznik wrote:
> On 01/09/2017 05:58 PM, Daniel P. Berrange wrote:
> > 
> > I'm wondering what people think of making use of this in libvirt ?
> 
> Apart from what have been said (I share the same concerns) I like the
> idea. All the "limitations" - we already have them. You have to
> initialize variables even when using 'goto cleanup'. But with this
> feature we can save few lines of code in a lot of functions. Therefore
> I'm up for it. BTW: do you know when was this introduced? Whether this
> feature is available on all prehistoric^Wstill supported systems like RHEL6.

The docs are non-existant on when this appeared, but I just checked
out gcc 4.1.2 and found plenty of references to attribute cleanup.
This covers RHEL-5 vintage, so it looks fine.

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

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

[libvirt] OpenStack/libvirt CAT interface

[Marcelo Tosatti]

There have been queries about the OpenStack interface 
for CAT:

http://bugzilla.redhat.com/show_bug.cgi?id=1299678

Comment 2 says:
Sahid Ferdjaoui 2016-01-19 10:58:48 EST
A spec will have to be addressed, after a first look this feature needs
some work in several components of Nova to maintain/schedule/consume
host's cache. I can work on that spec and implementation it when libvirt
will provides information about cache and feature to use it for guests.

I could add a comment about parameters to resctrltool, but since
this depends on the libvirt interface, it would be good to know
what the libvirt interface exposes first.

I believe it should be essentially similar to OpenStack's
"reserved_host_memory_mb":

Set the reserved_host_memory_mb to reserve RAM for host
processes. For
the purposes of testing I am going to use the default of 512 MB:
reserved_host_memory_mb=512

But rather use:

rdt_cat_cache_reservation=type=code/data/both,size=10mb,cacheid=2;
  type=code/data/both,size=2mb,cacheid=1;...

(per-vcpu).

Where cache-id is optional.

What is cache-id (from Documentation/x86/intel_rdt_ui.txt on recent
kernel sources):
Cache IDs
-
On current generation systems there is one L3 cache per socket and L2
caches are generally just shared by the hyperthreads on a core, but this
isn't an architectural requirement. We could have multiple separate L3
caches on a socket, multiple cores could share an L2 cache. So instead
of using "socket" or "core" to define the set of logical cpus sharing
a resource we use a "Cache ID". At a given cache level this will be a
unique number across the whole system (but it isn't guaranteed to be a
contiguous sequence, there may be gaps).  To find the ID for each
logical
CPU look in /sys/devices/system/cpu/cpu*/cache/index*/id


WHAT THE USER NEEDS TO SPECIFY FOR VIRTUALIZATION (KVM-RT)
==

For virtualization the following scenario is desired,
on a given socket:

* VM-A with VCPUs VM-A.vcpu-1, VM-A.vcpu-2.
* VM-B with VCPUs VM-B.vcpu-1, VM-B.vcpu-2.

With one realtime workload on each vcpu-2.

Assume VM-A.vcpu-2 on pcpu 3.
Assume VM-B.vcpu-2 on pcpu 5.

Assume pcpus 0-5 on cacheid 0.

We want VM-A.vcpu-2 to have a certain region of cache reserved,
and VM-B.vcpu-2 as well. vcpu-1 for both VMs can use the default group
(that is not have reserved L3 cache).

This translates to the following resctrltool-style reservations:

res.vm-a.vcpu-2

type=both,size=VM-A-RESSIZE,cache-id=0

res.vm-b.vcpu-2

type=both,size=VM-B-RESSIZE,cache-id=0

Which translate to the following in resctrlfs:

res.vm-a.vcpu-2

type=both,size=VM-A-RESSIZE,cache-id=0
type=both,size=default-size,cache-id=1
...

res.vm-b.vcpu-2

type=both,size=VM-B-RESSIZE,cache-id=0
type=both,size=default-size,cache-id=1
...

Which is what we want, since the VCPUs are pinned.


res.vm-a.vcpu-1 and res.vm-b.vcpu-1 don't need to
be assigned to any reservation, which means they'll
remain on the default group.

RESTRICTIONS TO THE SYNTAX ABOVE


Rules for the parameters:
* type=code must be paired with type=data entry.

ABOUT THE LIST INTERFACE


About an interface for listing the reservations
of the system to OpenStack.

I think that what OpenStack needs is to check, before
starting a guest on a given host, that there is sufficient
space available for the reservation.

To do that, it can:

1) resctrltool list (the end of the output mentions
   how much free space available there is), or
   via resctrlfs directly (have to lock the filesystem,
   read each directory, AND each schemata, and count
   number of zero bits).
2) Via libvirt

Should fix resctrltool/API to list amount of contiguous free space
BTW.

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

Re: [libvirt] [PATCH] storage: Fix storage_backend probing when PARTED not installed.

[Peter Krempa]

On Tue, Jan 10, 2017 at 10:20:11 -0500, John Ferlan wrote:
> Commit id 'a48c674f' caused problems for systems without PARTED installed.
> 
> So move the PARTED probing code back to storage_backend_disk.c and create
> a shim within storage_backend.c to call it if WITH_STORAGE_DISK is true;
> otherwise, just return -1 with the error.
> 
> Signed-off-by: John Ferlan 
> ---
> 
>  Reported to me internally via Peter Krempa...  Not pushing as a build
>  breaker - I'll let Peter review and ACK.

I don't really see a reason why this should be a build time dependency,
but this fixes the build for me now so ACK.

Peter

P.S: I'll post a patch that removes a build-time dependency on the
gluster cli tool soon, since it doesn't make much sense.


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

[libvirt] [PATCH] libxl: fix usb inputs loop error

[Cédric Bosdonnat]

List indexes where mixed up in the code looping over the USB
input devices.
---
 src/libxl/libxl_conf.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index ac83b51c7..1053e60a1 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -479,7 +479,7 @@ libxlMakeDomBuildInfo(virDomainDefPtr def,
 if (VIR_EXPAND_N(b_info->u.hvm.usbdevice_list, nusbdevice, 1) < 0)
 return -1;
 #else
-if (i > 1) {
+if (nusbdevice > 1) {
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
 _("libxenlight supports only one input device"));
 return -1;
@@ -487,7 +487,7 @@ libxlMakeDomBuildInfo(virDomainDefPtr def,
 #endif
 
 #ifdef LIBXL_HAVE_BUILDINFO_USBDEVICE_LIST
-usbdevice = &b_info->u.hvm.usbdevice_list[i];
+usbdevice = &b_info->u.hvm.usbdevice_list[nusbdevice - 1];
 #else
 usbdevice = &b_info->u.hvm.usbdevice;
 #endif
-- 
2.11.0

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

Re: [libvirt] RFC: Use __attribute__ ((cleanup) in libvirt ?

[Michal Privoznik]

On 01/09/2017 05:58 PM, Daniel P. Berrange wrote:
> 
> I'm wondering what people think of making use of this in libvirt ?

Apart from what have been said (I share the same concerns) I like the
idea. All the "limitations" - we already have them. You have to
initialize variables even when using 'goto cleanup'. But with this
feature we can save few lines of code in a lot of functions. Therefore
I'm up for it. BTW: do you know when was this introduced? Whether this
feature is available on all prehistoric^Wstill supported systems like RHEL6.

BTW: I've added this idea onto our GSoC page:

http://wiki.libvirt.org/page/Google_Summer_of_Code_2017#Automatic_freeing_of_memory

Sounds like perfect job for a student to me. Unless there's some
deadline to meet.

Michal

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

Re: [libvirt] [PATCH] Extend l3 cache to nodeinfo

[Daniel P. Berrange]

On Tue, Jan 10, 2017 at 04:11:03PM +0800, Eli Qiao wrote:
> This patch extends l3 cache infomation to nodeinfo output.

Also, why only l3 cache. Why not expose full info about
the CPU cache hierarchy. It feels wrong to expose only
L3 cache and ignore other levels of cache.

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

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

Re: [libvirt] qemu-kvm blocked for more than 120 seconds when "libvirt-guests" is enabled

[Andrea Bolognani]

On Sat, 2016-12-31 at 17:25 +0100, Oscar Segarra wrote:
[...]
> [root@vdicnode01 ~]# service libvirt-guests status
> Redirecting to /bin/systemctl status  libvirt-guests.service
> ● libvirt-guests.service - Suspend Active Libvirt Guests
>Loaded: loaded (/usr/lib/systemd/system/libvirt-guests.service; disabled; 
>vendor preset: disabled)

This is weird, libvirt-guests.service is disabled yet is being
stopped at shutdown, according to your screenshots.

>Active: active (exited) since Sat 2016-12-31 17:13:34 CET; 5s ago
>  Docs: man:libvirtd(8)
>http://libvirt.org
>   Process: 6619 ExecStart=/usr/libexec/libvirt-guests.sh start (code=exited, 
>status=0/SUCCESS)
>  Main PID: 6619 (code=exited, status=0/SUCCESS)
> 
> Dec 31 17:13:34 vdicnode01.vdicube.com systemd[1]: Starting Suspend Active 
> Libvirt Guests...
> Dec 31 17:13:34 vdicnode01.vdicube.com libvirt-guests.sh[6619]: 
> libvirt-guests is configured not to start any guests on boot
> Dec 31 17:13:34 vdicnode01.vdicube.com systemd[1]: Started Suspend Active 
> Libvirt Guests.
> [root@vdicnode01 ~]#
> 
> If I stop locally libvirt-guests (as root) Looks work perfectly:
> 
> [root@vdicnode01 ~]# virsh list
>  IdName   State
> 
> 
> [root@vdicnode01 ~]#

What does 'systemctl status libvirt-guests' tell you
at this point?

My only guess is that your guests might be taking too
long to shut down cleanly, which is not a problem when
you're stopping the service while the host is running
but causes a timeout during shutdown.

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [PATCH] rpc: libssh: don't crash when known_hosts file path is not provided

[Peter Krempa]

On Tue, Jan 10, 2017 at 16:08:03 +0100, Pino Toscano wrote:
> On Monday, 9 January 2017 15:16:46 CET Peter Krempa wrote:
> > When connecting as root, the "hostsfile" variable would be NULL due to
> > the code leading to this point. This would result into a crash when
> > attempting to set the known hosts file path.
> 
> Almost: the issue happens generally when known_hosts does not exist.
> Considering the following code in src/rpc/virnetclient.c (both
> virNetClientNewLibSSH2 and virNetClientNewLibssh have it):
> 
> if (confdir) {
> if (!knownHostsPath) {
> if (virFileExists(confdir)) {
> virBufferAsprintf(&buf, "%s/known_hosts", confdir);
> if (!(knownhosts = virBufferContentAndReset(&buf)))
> goto no_memory;
> }
> } else {
> if (VIR_STRDUP(knownhosts, knownHostsPath) < 0)
> goto cleanup;
> }
> }

There is one more bug here. If 'knownHostsPath' is provided it should be
set regardless of 'confdir' so the logic is broken there.

> 
> knownHostsPath is left NULL if known_hosts is not passed as parameter,
> and either one of:
> a) confdir is NULL: reading virGetUserConfigDirectory (actually,
>virGetXDGDirectory) this happens only when XDG_CONFIG_HOME is not
>set, and neither is HOME
> b) confdir is not NULL, but it does not exist

So this is the root problem here, with the logic above the code would
crash if 'confdir' would not be set if you specify the path manually. It
does not crash if you do it, so 'confdir' indeed is not NULL'

> Regarding (a), there is no other "fix" doable within virNetClient
> itself, since all the ways to determine the user home failed (and thus
> no path can be determined).

I agree.

> 
> Regarding (b), IMHO virNetClient could drop the virFileExists check,
> letting the libssh and libssh2 drivers to handle:
> - libssh seems to handle a non-existing file fine, creating its
>   directories and the file itself on its own when saving
> - the libssh2 already checks whether the file exists, using it only in
>   that case

That seems like a reasonable thing to do. Along with fixing the logic.

> 
> > To avoid deviating from the approach taken in the libssh2 driver set the
> > file to /dev/null so that all entries are discarded unless explicitly
> > specified.
> 
> While this works (ssh_write_knownhost should handle /dev as existing
> already), IMHO it's better to make the libssh driver behave more close
> to the libssh2, i.e. setting knownHostsFile only when considered valid.
> What about the attached patch instead?
> 
> -- 
> Pino Toscano

> From=20a97d1baeefd32990730f4ec32501bf5fdb7b675b Mon Sep 17 00:00:00 2001
> From: Pino Toscano 
> Date: Tue, 10 Jan 2017 16:04:03 +0100
> Subject: [PATCH] rpc: libssh: allow a NULL known_hosts file
> 
> Make sure that virNetLibsshSessionSetHostKeyVerification accepts a NULL
> value for the path to the known_hosts file:
> - call ssh_options_set(SSH_OPTIONS_KNOWNHOSTS) anyway, using /dev/null,
>   otherwise libssh will use its default path
> - do not call ssh_write_knownhost when no known hosts file was set
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1406457
> ---
>  src/rpc/virnetlibsshsession.c | 41 +++--
>  1 file changed, 27 insertions(+), 14 deletions(-)
> 
> diff --git a/src/rpc/virnetlibsshsession.c b/src/rpc/virnetlibsshsession.c
> index 5de6629..3ffe158 100644
> --- a/src/rpc/virnetlibsshsession.c
> +++ b/src/rpc/virnetlibsshsession.c
> @@ -303,6 +303,10 @@ virNetLibsshCheckHostKey(virNetLibsshSessionPtr sess)
>  if (!keyhashstr)
>  return -1;
>  
> +/* If a key mismatch happens, then surely there was an existing
> + * known_host file (with the different key). */
> +sa_assert(sess->knownHostsFile);

While this assertion is true, it's pointless to have it here.

> +
>  /* host key verification failed */
>  virReportError(VIR_ERR_AUTH_FAILED,
> _("!!! SSH HOST KEY VERIFICATION FAILED !!!: "
> @@ -382,14 +386,16 @@ virNetLibsshCheckHostKey(virNetLibsshSessionPtr sess)
>  VIR_FREE(askKey.result);
>  }
>  
> -/* write the host key file */
> -if (ssh_write_knownhost(sess->session) < 0) {
> -errmsg = ssh_get_error(sess->session);
> -virReportError(VIR_ERR_LIBSSH,
> -   _("failed to write known_host file '%s': %s"),
> -   sess->knownHostsFile,
> -   errmsg);
> -return -1;
> +/* write the host key file, if specified */
> +if (sess->knownHostsFile) {
> +if (ssh_write_knownhost(sess->session) < 0) {
> +errmsg = ssh_get_error(sess->session);
> +virReportError(VIR_ERR_LIBSSH,
> +   _("failed to write known_host file '%s': %s"),
> + 

[libvirt] [PATCH 2/2] virSecuritySELinuxSetFileconHelper: Fix build with broken selinux.h

[Michal Privoznik]

There are still some systems out there that have broken
setfilecon*() prototypes. Instead of taking 'const char *tcon' it
is taking 'char *tcon'. The function should just set the context,
not modify it.

We had been bitten with this problem before which resulted in
292d3f2d and subsequently b109c09765. However, with one my latest
commits (4674fc6afd6d) I've changed the type of @tcon variable to
'const char *' which results in build failure on the systems from
above.

Signed-off-by: Michal Privoznik 
---
 src/security/security_selinux.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index 92267e8fc..f229b5139 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -1138,7 +1138,7 @@ virSecuritySELinuxSetFileconHelper(const char *path, 
const char *tcon,
 
 VIR_INFO("Setting SELinux context on '%s' to '%s'", path, tcon);
 
-if (setfilecon_raw(path, tcon) < 0) {
+if (setfilecon_raw(path, (VIR_SELINUX_CTX_CONST char *) tcon) < 0) {
 int setfilecon_errno = errno;
 
 if (getfilecon_raw(path, &econ) >= 0) {
-- 
2.11.0

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

[libvirt] [PATCH 0/2] Couple of build fixes

[Michal Privoznik]

Spotted by our CI.

Michal Privoznik (2):
  qemu_domain: Move qemuDomainGetPreservedMounts
  virSecuritySELinuxSetFileconHelper: Fix build with broken selinux.h

 src/qemu/qemu_domain.c  | 140 
 src/security/security_selinux.c |   2 +-
 2 files changed, 71 insertions(+), 71 deletions(-)

-- 
2.11.0

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

[libvirt] [PATCH 1/2] qemu_domain: Move qemuDomainGetPreservedMounts

[Michal Privoznik]

This function is used only from code compiled on Linux. Therefore
on non-Linux platforms it triggers compilation error:

../../src/qemu/qemu_domain.c:209:1: error: unused function 
'qemuDomainGetPreservedMounts' [-Werror,-Wunused-function]

Signed-off-by: Michal Privoznik 
---
 src/qemu/qemu_domain.c | 140 -
 1 file changed, 70 insertions(+), 70 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 01765dc28..b26c02bda 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -194,76 +194,6 @@ qemuDomainEnableNamespace(virDomainObjPtr vm,
 }
 
 
-/**
- * qemuDomainGetPreservedMounts:
- *
- * Process list of mounted filesystems and:
- * a) save all FSs mounted under /dev to @devPath
- * b) generate backup path for all the entries in a)
- *
- * Any of the return pointers can be NULL.
- *
- * Returns 0 on success, -1 otherwise (with error reported)
- */
-static int
-qemuDomainGetPreservedMounts(virQEMUDriverPtr driver,
- virDomainObjPtr vm,
- char ***devPath,
- char ***devSavePath,
- size_t *ndevPath)
-{
-virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
-char **paths = NULL, **mounts = NULL;
-size_t i, nmounts;
-
-if (virFileGetMountSubtree(PROC_MOUNTS, "/dev",
-   &mounts, &nmounts) < 0)
-goto error;
-
-if (!nmounts) {
-if (ndevPath)
-*ndevPath = 0;
-return 0;
-}
-
-if (VIR_ALLOC_N(paths, nmounts) < 0)
-goto error;
-
-for (i = 0; i < nmounts; i++) {
-const char *suffix = mounts[i] + strlen(DEVPREFIX);
-
-if (STREQ(mounts[i], "/dev"))
-suffix = "dev";
-
-if (virAsprintf(&paths[i], "%s/%s.%s",
-cfg->stateDir, vm->def->name, suffix) < 0)
-goto error;
-}
-
-if (devPath)
-*devPath = mounts;
-else
-virStringListFreeCount(mounts, nmounts);
-
-if (devSavePath)
-*devSavePath = paths;
-else
-virStringListFreeCount(paths, nmounts);
-
-if (ndevPath)
-*ndevPath = nmounts;
-
-virObjectUnref(cfg);
-return 0;
-
- error:
-virStringListFreeCount(mounts, nmounts);
-virStringListFreeCount(paths, nmounts);
-virObjectUnref(cfg);
-return -1;
-}
-
-
 void qemuDomainEventQueue(virQEMUDriverPtr driver,
   virObjectEventPtr event)
 {
@@ -6950,6 +6880,76 @@ qemuDomainGetHostdevPath(virDomainHostdevDefPtr dev,
 
 
 #if defined(__linux__)
+/**
+ * qemuDomainGetPreservedMounts:
+ *
+ * Process list of mounted filesystems and:
+ * a) save all FSs mounted under /dev to @devPath
+ * b) generate backup path for all the entries in a)
+ *
+ * Any of the return pointers can be NULL.
+ *
+ * Returns 0 on success, -1 otherwise (with error reported)
+ */
+static int
+qemuDomainGetPreservedMounts(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ char ***devPath,
+ char ***devSavePath,
+ size_t *ndevPath)
+{
+virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
+char **paths = NULL, **mounts = NULL;
+size_t i, nmounts;
+
+if (virFileGetMountSubtree(PROC_MOUNTS, "/dev",
+   &mounts, &nmounts) < 0)
+goto error;
+
+if (!nmounts) {
+if (ndevPath)
+*ndevPath = 0;
+return 0;
+}
+
+if (VIR_ALLOC_N(paths, nmounts) < 0)
+goto error;
+
+for (i = 0; i < nmounts; i++) {
+const char *suffix = mounts[i] + strlen(DEVPREFIX);
+
+if (STREQ(mounts[i], "/dev"))
+suffix = "dev";
+
+if (virAsprintf(&paths[i], "%s/%s.%s",
+cfg->stateDir, vm->def->name, suffix) < 0)
+goto error;
+}
+
+if (devPath)
+*devPath = mounts;
+else
+virStringListFreeCount(mounts, nmounts);
+
+if (devSavePath)
+*devSavePath = paths;
+else
+virStringListFreeCount(paths, nmounts);
+
+if (ndevPath)
+*ndevPath = nmounts;
+
+virObjectUnref(cfg);
+return 0;
+
+ error:
+virStringListFreeCount(mounts, nmounts);
+virStringListFreeCount(paths, nmounts);
+virObjectUnref(cfg);
+return -1;
+}
+
+
 static int
 qemuDomainCreateDevice(const char *device,
const char *path,
-- 
2.11.0

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

Re: [libvirt] [PATCH 5/6] qemu: snapshot: Properly handle image locking

[Peter Krempa]

On Sat, Jan 07, 2017 at 13:37:48 -0500, John Ferlan wrote:
> 
> 
> On 12/16/2016 11:24 AM, Peter Krempa wrote:
> > Images that became the backing chain of the current image due to the
> > snapshot need to be unlocked in the lock manager. Also if qemu was
> > paused during the snapshot the current top level images need to be
> > released until qemu is resumed so that they can be acquired properly.
> > 
> > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1191901
> > ---
> >  src/qemu/qemu_driver.c | 17 +
> >  1 file changed, 17 insertions(+)
> > 
> > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> > index 742b6ceda..13da035c2 100644
> > --- a/src/qemu/qemu_driver.c
> > +++ b/src/qemu/qemu_driver.c
> > @@ -14452,6 +14452,23 @@ 
> > qemuDomainSnapshotCreateDiskActive(virQEMUDriverPtr driver,
> >  virStorageFileUnlink(diskdata[i].src) < 0)
> >  VIR_WARN("Unable to remove just-created %s", 
> > diskdata[i].src->path);
> >  }
> > +} else {
> > +/* on successful snapshot we need to remove locks from the now-old
> > + * disks and if the VM is paused release locks on the images since 
> > qemu
> > + * stopped using them*/
> > +bool paused = virDomainObjGetState(vm, NULL) != VIR_DOMAIN_RUNNING;
> > +
> > +for (i = 0; i < snap->def->ndisks; i++) {
> > +if (!diskdata[i].disk)
> 
> Should this be disk.src?  Or handle the _NONE case? since src only gets
> filled in for that.

The entries in the 'diskdata' array are filled only if they are not
_NONE.  !diskdata[i].disk->src would crash on the missing entries since
.disk would be NULL. !diskdata[i].disk || !diskdata[i].disk->src is
redundant since the src member is mandatory for a disk object, thus
always present if .disk is not NULL.

Peter


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

Re: [libvirt] [PATCH 4/6] qemu: snapshot: Refactor snapshot rollback on failure

[Peter Krempa]

On Sat, Jan 07, 2017 at 13:37:12 -0500, John Ferlan wrote:
> 
> 
> On 12/16/2016 11:24 AM, Peter Krempa wrote:
> > The code at first changed the definition and then rolled it back in case
> > of failure. This was ridiculous. Refactor the code so that the image in
> > the definition is changed only when the snapshot is successful.
> > 
> > The refactor will also simplify further fix of image locking when doing
> > snapshots.
> > ---
> >  src/qemu/qemu_driver.c | 352 
> > -
> >  1 file changed, 203 insertions(+), 149 deletions(-)

[...]

> > +if (diskdata[i].prepared)
> > +qemuDomainDiskChainElementRevoke(driver, vm, 
> > diskdata[i].src);
> > +
> > +if (diskdata[i].created &&
> > +virStorageFileUnlink(diskdata[i].src) < 0)
> > +VIR_WARN("Unable to remove just-created %s", 
> > diskdata[i].src->path);
> >  }
> >  }
> > 
> > -if (ret == 0 || !virQEMUCapsGet(priv->qemuCaps, 
> > QEMU_CAPS_TRANSACTION)) {
> > +if (ret == 0 || !actions) {

Note that the above change does not change logic in any way. It just
saves a call to virQEMUCapsGet since it's checked earlier when 'actions'
is allocated. 'actions' is allocated only if the capability is present.

> 
> Wouldn't we want to save the status based on transactions? Should this
> be "if ((!actions && do_transaction) || (actions && ret == 0))".

Not sure if this is relevant for this patch. This refactor tries to
remove the rollback of the disk image data. I don't think this piece of
code should be touched in this patch.

> 
> /* Either a partial update has happened without transaction
>  * support or a successful usage of actions to perform all
>  * updates occurred, thus we need to save our state/config */
> if (do_transaction && (!actions || (actions && ret == 0))) {
> }
> 
> Trying to avoid a possible write of the file if no transactions were
> completed that could cause an erroneous failure since we did nothing.

That is a very unlikely corner case. You are right that at this point
only the memory snapshot was taken which currently does not modify the
status or config XML in any way so it would be valid at this point to do
the suggested change.

I just don't think it's worth at all.

> 
> >  if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm, 
> > driver->caps) < 0 ||
> >  (persist && virDomainSaveConfig(cfg->configDir, driver->caps,
> >  vm->newDef) < 0))
> >  ret = -1;
> 
> I know this just follows the previous code, but at this point there's no
> more undoing and returning -1 would indicate something went wrong.
> Nothing quick comes to mind on the best way to handle that, so we can
> continue with this. Still we have an inconsistency between actual,
> Status, and possibly Config.  Which is worse?

Well, losing the status information will result into libvirt thinking
that the backing chain is different than qemu thinks and thus block jobs
will fail to work properly.

When we lose config changes you are in for a data loss. A new
start of the VM will basically revert the snapshot disk images back to
the state at the snapshot and you lose all data since the snapshot.

Both cases happen only if libvirtd is restarted since otherwise it does
not re-read the files.

I think that both should be fatal errors anyways due to somewhat
catastrophic implications.
> 
> ACK with at least the leak fixed... It'd be good to handle saving things
> better.  Whether there's anything than be done if one of the save's fail
> - is perhaps a challenge for another day.
> 
> John

Peter


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

[libvirt] [PATCH v3 1/3] NEWS: Improve building pipeline

[Andrea Bolognani]

Currently, building the NEWS file involves using a XSLT stylesheet
to extract information from the same HTML file that's used on the
libvirt website.

The process works, but it's quite fiddly in that it requires the
source HTML to be formatted in a very precise way, and a single
missing newline can mess up the resulting plain text considerably.

Moreover, the XSLT stylesheet itself encodes a lot of the details
of converting to plain text in a way that's not necessarily easy
to understand, tweak or fix.

To improve the process, move all existing entries to a new XML
file that contains exactly the information we care about in a
simple structured format, and start generating both the HTML and
plain text versions of the release notes using XSLT stylesheets
that can now afford to be almost trivial.
---
Changes from v2:

  * Address review comments
- Update "bottom text" for both ASCII and HTML
  versions

  * XSLT stylesheet (ASCII version)
- Prefix contents of the  element with
  a special marker in the output

  * Build system
- Make sure all files are distributed
- Tweak formatting

  * Release notes
- Sync to current master

Changes from v1:

  * Address review comments
- Rename XSLT stylesheets to news-{ascii,html}.xsl
- Add "do not edit" note to news.html.in
- Add "older releases" note to NEWS

  * Rename XML elements
-  instead of 
-  instead of 

 .gitignore  |   1 +
 Makefile.am |  11 ++-
 docs/Makefile.am|  28 +-
 docs/news-ascii.xsl |  68 ++
 docs/news-html.xsl  |  96 
 docs/news.html.in   | 164 --
 docs/news.xml   | 252 
 docs/news.xsl   |  50 ---
 8 files changed, 451 insertions(+), 219 deletions(-)
 create mode 100644 docs/news-ascii.xsl
 create mode 100644 docs/news-html.xsl
 delete mode 100644 docs/news.html.in
 create mode 100644 docs/news.xml
 delete mode 100644 docs/news.xsl

diff --git a/.gitignore b/.gitignore
index 984ad07..7b71bd1 100644
--- a/.gitignore
+++ b/.gitignore
@@ -72,6 +72,7 @@
 /docs/libvirt-lxc-*.xml
 /docs/libvirt-qemu-*.xml
 /docs/libvirt-refs.xml
+/docs/news.html.in
 /docs/search.php
 /docs/todo.html.in
 /examples/admin/client_close
diff --git a/Makefile.am b/Makefile.am
index 50c358c..fa45388 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -46,13 +46,18 @@ EXTRA_DIST = \
 pkgconfigdir = $(libdir)/pkgconfig
 pkgconfig_DATA = libvirt.pc libvirt-qemu.pc libvirt-lxc.pc libvirt-admin.pc
 
-NEWS: $(top_srcdir)/docs/news.xsl $(top_srcdir)/docs/news.html.in
+NEWS: \
+ $(srcdir)/docs/news.xml \
+ $(srcdir)/docs/news-ascii.xsl
$(AM_V_GEN)if [ -x $(XSLTPROC) ] ; then \
- $(XSLTPROC) --nonet $(top_srcdir)/docs/news.xsl   \
-$(top_srcdir)/docs/news.html.in\
+ $(XSLTPROC) --nonet $(srcdir)/docs/news-ascii.xsl \
+$(srcdir)/docs/news.xml\
   | perl -0777 -pe 's/\n\n+$$/\n/' \
   | perl -pe 's/[ \t]+$$//'\
   > $@-t && mv $@-t $@ ; fi
+EXTRA_DIST += \
+   $(srcdir)/docs/news.xml \
+   $(srcdir)/docs/news-ascii.xsl
 
 $(top_srcdir)/HACKING: $(top_srcdir)/docs/hacking1.xsl \
$(top_srcdir)/docs/hacking2.xsl \
diff --git a/docs/Makefile.am b/docs/Makefile.am
index 790c0a2..62e069e 100644
--- a/docs/Makefile.am
+++ b/docs/Makefile.am
@@ -106,9 +106,10 @@ internals_html = $(internals_html_in:%.html.in=%.html)
 # Since we ship pre-built html in the tarball, we must also
 # ship the sources, even when those sources are themselves
 # generated.
-# Generate hvsupport.html first, since it takes one extra step.
+# Generate hvsupport.html and news.html first, since they take one extra step.
 dot_html_in = \
   hvsupport.html.in \
+  news.html.in \
   $(notdir $(wildcard $(srcdir)/*.html.in))
 dot_html = $(dot_html_in:%.html.in=%.html)
 
@@ -156,7 +157,7 @@ schema_DATA = $(wildcard $(srcdir)/schemas/*.rng)
 
 EXTRA_DIST=\
   apibuild.py genaclperms.pl \
-  site.xsl subsite.xsl newapi.xsl news.xsl page.xsl \
+  site.xsl subsite.xsl newapi.xsl page.xsl \
   hacking1.xsl hacking2.xsl wrapstring.xsl \
   $(dot_html) $(dot_html_in) $(gif) $(apihtml) $(apipng) \
   $(devhelphtml) $(devhelppng) $(devhelpcss) $(devhelpxsl) \
@@ -200,6 +201,29 @@ $(srcdir)/hvsupport.html.in: $(srcdir)/hvsupport.pl 
$(api_DATA) \
$(AM_V_GEN)$(PERL) $(srcdir)/hvsupport.pl $(top_srcdir)/src > $@ \
|| { rm $@ && exit 1; }
 
+# xsltproc seems to add the xmlns="" attribute to random output elements:
+# use sed to strip it out, as leaving it there triggers XML errors during
+# further transformation steps
+news.html.in: \
+ $(srcdir)/news.xml \
+ $(srcdir)/news-html.xsl
+   $(AM_V_GEN) \
+   if [ -x $(XSLTPROC) ]; then

[libvirt] [PATCH v3 2/3] NEWS: Reformat at generation time

[Andrea Bolognani]

Instead of encoding formatting information inside the
corresponding XSLT stylesheet, use a Python script to reformat
the text appropriately based on a few simple markers.

Splitting the task between the XSLT stylesheet and the Python
script allows us to keep both parts very simple.
---
Changes from v2:

  * Python script
- Consider all unmarked text as verbatim
- Support new marker for description
- Remove redundancy
- Use print() instead of sys.stdout.write()

Changes from v1:

  * Address review comments
- Avoid breaking 'make syntax-check'

 Makefile.am   |  23 +++
 docs/reformat-news.py | 104 ++
 2 files changed, 119 insertions(+), 8 deletions(-)
 create mode 100755 docs/reformat-news.py

diff --git a/Makefile.am b/Makefile.am
index fa45388..c6324f5 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -48,16 +48,23 @@ pkgconfig_DATA = libvirt.pc libvirt-qemu.pc libvirt-lxc.pc 
libvirt-admin.pc
 
 NEWS: \
  $(srcdir)/docs/news.xml \
- $(srcdir)/docs/news-ascii.xsl
-   $(AM_V_GEN)if [ -x $(XSLTPROC) ] ; then \
- $(XSLTPROC) --nonet $(srcdir)/docs/news-ascii.xsl \
-$(srcdir)/docs/news.xml\
-  | perl -0777 -pe 's/\n\n+$$/\n/' \
-  | perl -pe 's/[ \t]+$$//'\
-  > $@-t && mv $@-t $@ ; fi
+ $(srcdir)/docs/news-ascii.xsl \
+ $(srcdir)/docs/reformat-news.py
+   $(AM_V_GEN) \
+   if [ -x $(XSLTPROC) ]; then \
+ $(XSLTPROC) --nonet \
+   $(srcdir)/docs/news-ascii.xsl \
+   $(srcdir)/docs/news.xml \
+ >$@-tmp \
+   || { rm -f $@-tmp; exit 1; }; \
+ $(srcdir)/docs/reformat-news.py $@-tmp >$@ \
+   || { rm -f $@-tmp; exit 1; }; \
+ rm -f $@-tmp; \
+   fi
 EXTRA_DIST += \
$(srcdir)/docs/news.xml \
-   $(srcdir)/docs/news-ascii.xsl
+   $(srcdir)/docs/news-ascii.xsl \
+   $(srcdir)/docs/reformat-news.py
 
 $(top_srcdir)/HACKING: $(top_srcdir)/docs/hacking1.xsl \
$(top_srcdir)/docs/hacking2.xsl \
diff --git a/docs/reformat-news.py b/docs/reformat-news.py
new file mode 100755
index 000..39499d9
--- /dev/null
+++ b/docs/reformat-news.py
@@ -0,0 +1,104 @@
+#!/usr/bin/env python
+
+# reformat-news.py: Reformat the NEWS file properly
+#
+# Copyright (C) 2017 Red Hat, Inc.
+#
+# This library is free software; you can redistribute it and/or
+# modify it under the terms of the GNU Lesser General Public
+# License as published by the Free Software Foundation; either
+# version 2.1 of the License, or (at your option) any later version.
+#
+# This library is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+# Lesser General Public License for more details.
+#
+# You should have received a copy of the GNU Lesser General Public
+# License along with this library.  If not, see
+# .
+#
+# Authors:
+# Andrea Bolognani 
+
+import sys
+
+COLUMNS = 80
+
+def reformat_with_indent(text, initial_indent, indent):
+
+res = ""
+line = initial_indent
+
+for word in text.split():
+
+# If adding one more word (plus a whitespace, plus a newline)
+# to the current line would push us over the desired number
+# of columns we start a new line instead
+if len(line) + len(word) > (COLUMNS - 2):
+res = res + line + "\n"
+line = indent
+
+# We need to take care when we've just started a  new line,
+# as we don't want to add any additional leading whitespace
+# in that case
+if line == indent or line == initial_indent:
+line = line + word
+else:
+line = line + " " + word
+
+# Append whatever's left
+res = res + line
+
+return res
+
+
+def reformat(line):
+
+# Empty lines don't need to be reformatted or even inspected
+if len(line) == 0:
+return line
+
+# For all non-empty lines, we decide the indentation level based
+# on the first character
+marker = line[0]
+
+# Release
+if marker == '#':
+initial_indent = 0
+indent = 2
+# Section
+elif marker == '*':
+initial_indent = 2
+indent = 4
+# Change summary
+elif marker == '-':
+initial_indent = 4
+indent = 6
+# Change description
+elif marker == '|':
+initial_indent = 8
+indent = 8
+# In this one case, the marker should not ultimately show
+# up in the output file, so we strip it before moving on
+line = line[1:]
+# Anything else should be left as-is
+else:
+return line
+
+return reformat_with_indent(line, " " * initial_indent, " " * indent)
+
+
+def main(args):
+
+i

[libvirt] [PATCH v3 3/3] docs: Document the release notes process for contributors

[Andrea Bolognani]

Now that we have built a fairly solid process for dealing with
release notes, we should start pushing for contributors to
provide the relevant information along with their code:
documenting the process is clearly a requirement for this to
happen.
---
Changes from v2:

  * Address review comments
- Point to the source file in user documentation
- Phrasing

Changes from v1:

  * Address review comments
- Remove ambiguity between merely documenting changes
  and adding entries to release notes
- Move from "should" to "must"
- Move new information to a separate 
- Regenerate HACKING file

 HACKING  |  7 +++
 docs/hacking.html.in | 10 ++
 2 files changed, 17 insertions(+)

diff --git a/HACKING b/HACKING
index c2cb037..ff545a8 100644
--- a/HACKING
+++ b/HACKING
@@ -226,6 +226,13 @@ to "tests/.valgrind.supp" in order to suppress the warning:
 (10) Update tests and/or documentation, particularly if you are adding a new
 feature or changing the output of a program.
 
+(11) Don't forget to update the release notes  by changing
+"docs/news.xml" if your changes are significant. All user-visible changes,
+such as adding new XML elements or fixing all but the most obscure bugs, must
+be (briefly) described in a release notes entry; changes that are only
+relevant to other libvirt developers, such as code refactoring, don't belong
+in the release notes.
+
 There is more on this subject, including lots of links to background reading
 on the subject, on Richard Jones' guide to working with open source projects
 .
diff --git a/docs/hacking.html.in b/docs/hacking.html.in
index 47475a2..c7bbcbd 100644
--- a/docs/hacking.html.in
+++ b/docs/hacking.html.in
@@ -294,6 +294,16 @@
 Update tests and/or documentation, particularly if you are adding
 a new feature or changing the output of a program.
   
+
+  
+Don't forget to update the release notes
+by changing docs/news.xml if your changes are
+significant. All user-visible changes, such as adding new XML elements
+or fixing all but the most obscure bugs, must be (briefly) described
+in a release notes entry; changes that are only relevant to other
+libvirt developers, such as code refactoring, don't belong in the
+release notes.
+  
 
 
 
-- 
2.7.4

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

[libvirt] [PATCH v3 0/3] Improve release notes process

[Andrea Bolognani]

Dan raised some concerns, but it doesn't look like they're
critical enough for him to NACK the series; moreover, it's
all stuff that we can easily address later on, so I'd like
to move forward with this before the freeze.

Details of changes can be found in the single patches.


Andrea Bolognani (3):
  NEWS: Improve building pipeline
  NEWS: Reformat at generation time
  docs: Document the release notes process for contributors

 .gitignore|   1 +
 HACKING   |   7 ++
 Makefile.am   |  26 --
 docs/Makefile.am  |  28 +-
 docs/hacking.html.in  |  10 ++
 docs/news-ascii.xsl   |  68 ++
 docs/news-html.xsl|  96 +++
 docs/news.html.in | 164 
 docs/news.xml | 252 ++
 docs/news.xsl |  50 --
 docs/reformat-news.py | 104 +
 11 files changed, 583 insertions(+), 223 deletions(-)
 create mode 100644 docs/news-ascii.xsl
 create mode 100644 docs/news-html.xsl
 delete mode 100644 docs/news.html.in
 create mode 100644 docs/news.xml
 delete mode 100644 docs/news.xsl
 create mode 100755 docs/reformat-news.py

-- 
2.7.4

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

[libvirt] [PATCH] storage: Fix storage_backend probing when PARTED not installed.

[John Ferlan]

Commit id 'a48c674f' caused problems for systems without PARTED installed.

So move the PARTED probing code back to storage_backend_disk.c and create
a shim within storage_backend.c to call it if WITH_STORAGE_DISK is true;
otherwise, just return -1 with the error.

Signed-off-by: John Ferlan 
---

 Reported to me internally via Peter Krempa...  Not pushing as a build
 breaker - I'll let Peter review and ACK.


 src/storage/storage_backend.c  | 155 -
 src/storage/storage_backend_disk.c | 152 
 src/storage/storage_backend_disk.h |   3 +
 3 files changed, 169 insertions(+), 141 deletions(-)

diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
index 060b1c0..18433e9 100644
--- a/src/storage/storage_backend.c
+++ b/src/storage/storage_backend.c
@@ -2851,157 +2851,30 @@ virStorageBackendBLKIDFindEmpty(const char *device 
ATTRIBUTE_UNUSED,
 #endif /* #if WITH_BLKID */
 
 
-typedef enum {
-VIR_STORAGE_PARTED_ERROR = -1,
-VIR_STORAGE_PARTED_MATCH,   /* Valid label found and matches format */
-VIR_STORAGE_PARTED_DIFFERENT,   /* Valid label found but not match format 
*/
-VIR_STORAGE_PARTED_UNKNOWN, /* No or unrecognized label */
-VIR_STORAGE_PARTED_NOPTTYPE,/* Did not find the Partition Table type */
-VIR_STORAGE_PARTED_PTTYPE_UNK,  /* Partition Table type unknown*/
-} virStorageBackendPARTEDResult;
-
-/**
- * Check for a valid disk label (partition table) on device using
- * the PARTED command
- *
- * returns virStorageBackendPARTEDResult
- */
-static virStorageBackendPARTEDResult
-virStorageBackendPARTEDFindLabel(const char *device,
- const char *format)
-{
-const char *const args[] = {
-device, "print", "--script", NULL,
-};
-virCommandPtr cmd = virCommandNew(PARTED);
-char *output = NULL;
-char *error = NULL;
-char *start, *end;
-int ret = VIR_STORAGE_PARTED_ERROR;
-
-virCommandAddArgSet(cmd, args);
-virCommandAddEnvString(cmd, "LC_ALL=C");
-virCommandSetOutputBuffer(cmd, &output);
-virCommandSetErrorBuffer(cmd, &error);
-
-/* if parted succeeds we have a valid partition table */
-ret = virCommandRun(cmd, NULL);
-if (ret < 0) {
-if ((output && strstr(output, "unrecognised disk label")) ||
-(error && strstr(error, "unrecognised disk label"))) {
-ret = VIR_STORAGE_PARTED_UNKNOWN;
-}
-goto cleanup;
-}
-
-/* Search for "Partition Table:" in the output. If not present,
- * then we cannot validate the partition table type.
- */
-if (!(start = strstr(output, "Partition Table: ")) ||
-!(end = strstr(start, "\n"))) {
-VIR_DEBUG("Unable to find tag in output: %s", output);
-ret = VIR_STORAGE_PARTED_NOPTTYPE;
-goto cleanup;
-}
-start += strlen("Partition Table: ");
-*end = '\0';
-
-/* on disk it's "msdos", but we document/use "dos" so deal with it here */
-if (STREQ(start, "msdos"))
-start += 2;
-
-/* Make sure we know about this type */
-if (virStoragePoolFormatDiskTypeFromString(start) < 0) {
-ret = VIR_STORAGE_PARTED_PTTYPE_UNK;
-goto cleanup;
-}
-
-/*  Does the on disk match what the pool desired? */
-if (STREQ(start, format))
-ret = VIR_STORAGE_PARTED_MATCH;
-
-ret = VIR_STORAGE_PARTED_DIFFERENT;
-
- cleanup:
-virCommandFree(cmd);
-VIR_FREE(output);
-VIR_FREE(error);
-return ret;
-}
-
+#if WITH_STORAGE_DISK
 
-/**
- * Determine whether the label on the disk is valid or in a known format
- * for the purpose of rewriting the label during build or being able to
- * start a pool on a device.
- *
- * When 'writelabel' is true, if we find a valid disk label on the device,
- * then we shouldn't be attempting to write as the volume may contain
- * data. Force the usage of the overwrite flag to the build command in
- * order to be certain. When the disk label is unrecognized, then it
- * should be safe to write.
- *
- * When 'writelabel' is false, only if we find a valid disk label on the
- * device should we allow the start since for this path we won't be
- * rewriting the label.
- *
- * Return: 0 if it's OK
- * -1 if something's wrong
- */
 static int
 virStorageBackendPARTEDValidLabel(const char *device,
   const char *format,
   bool writelabel)
 {
-int ret = -1;
-virStorageBackendPARTEDResult check;
-
-check = virStorageBackendPARTEDFindLabel(device, format);
-switch (check) {
-case VIR_STORAGE_PARTED_ERROR:
-virReportError(VIR_ERR_OPERATION_FAILED, "%s",
-   _("Error checking for disk label, failed to get "
- "disk partition information"));
-break;
-
-case VIR_STORAGE_PARTED_MATCH:
-if (writelabel)
-virReportError(VIR_ERR_OPERATION_I

Re: [libvirt] [PATCH] rpc: libssh: don't crash when known_hosts file path is not provided

[Pino Toscano]

On Monday, 9 January 2017 15:16:46 CET Peter Krempa wrote:
> When connecting as root, the "hostsfile" variable would be NULL due to
> the code leading to this point. This would result into a crash when
> attempting to set the known hosts file path.

Almost: the issue happens generally when known_hosts does not exist.
Considering the following code in src/rpc/virnetclient.c (both
virNetClientNewLibSSH2 and virNetClientNewLibssh have it):

if (confdir) {
if (!knownHostsPath) {
if (virFileExists(confdir)) {
virBufferAsprintf(&buf, "%s/known_hosts", confdir);
if (!(knownhosts = virBufferContentAndReset(&buf)))
goto no_memory;
}
} else {
if (VIR_STRDUP(knownhosts, knownHostsPath) < 0)
goto cleanup;
}
}

knownHostsPath is left NULL if known_hosts is not passed as parameter,
and either one of:
a) confdir is NULL: reading virGetUserConfigDirectory (actually,
   virGetXDGDirectory) this happens only when XDG_CONFIG_HOME is not
   set, and neither is HOME
b) confdir is not NULL, but it does not exist

Regarding (a), there is no other "fix" doable within virNetClient
itself, since all the ways to determine the user home failed (and thus
no path can be determined).

Regarding (b), IMHO virNetClient could drop the virFileExists check,
letting the libssh and libssh2 drivers to handle:
- libssh seems to handle a non-existing file fine, creating its
  directories and the file itself on its own when saving
- the libssh2 already checks whether the file exists, using it only in
  that case

> To avoid deviating from the approach taken in the libssh2 driver set the
> file to /dev/null so that all entries are discarded unless explicitly
> specified.

While this works (ssh_write_knownhost should handle /dev as existing
already), IMHO it's better to make the libssh driver behave more close
to the libssh2, i.e. setting knownHostsFile only when considered valid.
What about the attached patch instead?

--
Pino ToscanoFrom a97d1baeefd32990730f4ec32501bf5fdb7b675b Mon Sep 17 00:00:00 2001
From: Pino Toscano 
Date: Tue, 10 Jan 2017 16:04:03 +0100
Subject: [PATCH] rpc: libssh: allow a NULL known_hosts file

Make sure that virNetLibsshSessionSetHostKeyVerification accepts a NULL
value for the path to the known_hosts file:
- call ssh_options_set(SSH_OPTIONS_KNOWNHOSTS) anyway, using /dev/null,
  otherwise libssh will use its default path
- do not call ssh_write_knownhost when no known hosts file was set

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id06457
---
 src/rpc/virnetlibsshsession.c | 41 +++--
 1 file changed, 27 insertions(+), 14 deletions(-)

diff --git a/src/rpc/virnetlibsshsession.c b/src/rpc/virnetlibsshsession.c
index 5de6629..3ffe158 100644
--- a/src/rpc/virnetlibsshsession.c
+++ b/src/rpc/virnetlibsshsession.c
@@ -303,6 +303,10 @@ virNetLibsshCheckHostKey(virNetLibsshSessionPtr sess)
 if (!keyhashstr)
 return -1;

+/* If a key mismatch happens, then surely there was an existing
+ * known_host file (with the different key). */
+sa_assert(sess->knownHostsFile);
+
 /* host key verification failed */
 virReportError(VIR_ERR_AUTH_FAILED,
_("!!! SSH HOST KEY VERIFICATION FAILED !!!: "
@@ -382,14 +386,16 @@ virNetLibsshCheckHostKey(virNetLibsshSessionPtr sess)
 VIR_FREE(askKey.result);
 }

-/* write the host key file */
-if (ssh_write_knownhost(sess->session) < 0) {
-errmsg = ssh_get_error(sess->session);
-virReportError(VIR_ERR_LIBSSH,
-   _("failed to write known_host file '%s': %s"),
-   sess->knownHostsFile,
-   errmsg);
-return -1;
+/* write the host key file, if specified */
+if (sess->knownHostsFile) {
+if (ssh_write_knownhost(sess->session) < 0) {
+errmsg = ssh_get_error(sess->session);
+virReportError(VIR_ERR_LIBSSH,
+   _("failed to write known_host file '%s': %s"),
+   sess->knownHostsFile,
+   errmsg);
+return -1;
+}
 }
 /* key was accepted and added */
 return 0;
@@ -1172,13 +1178,20 @@ virNetLibsshSessionSetHostKeyVerification(virNetLibsshSessionPtr sess,
 goto error;
 }

-/* set the known hosts file */
-if (ssh_options_set(sess->session, SSH_OPTIONS_KNOWNHOSTS, hostsfile) < 0)
-goto error;
+/* set the known hosts file, if specified */
+if (hostsfile) {
+if (ssh_options_set(sess->session, SSH_OPTIONS_KNOWNHOSTS, hostsfile) < 0)
+goto error;

-VIR_FREE(sess->knownHostsFile);
-if (VIR_STRDUP(sess->knownHostsFile, hostsfile) < 0)
-goto error;
+

[libvirt] Regarding Migration Statistics

[Anubhav Guleria]

Greetings,

I am writing a code using libvirt API to migrate VM between two physical
hosts *(QEMU/KVM) , *say some *n *number of times.

*1)* I am using right now* virDomainPtr virDomainMigrate (...) *and to
calculate the total migration time I am using something like this:

*clock_gettime(CLOCK_MONOTONIC_RAW,&begin);  *
 *migrate*(domainToMigrate,nodeToMigrate);

*clock_gettime(CLOCK_MONOTONIC_RAW,&end);*



*Total Migration Time = end.tv_sec-begin.tv_sec*

Is this correct way to calculate total migration time. And is there some
way to calculate the downtime (not how to set it)?

*2) *I am interested in identifying in particular other statistics of
migration like :
*Number of iterations in Pre Copy*, *Memory transferred in each iteration*
etc.
I was going through the API and found* virDomainJobInfo
 and
virDomainGetJobStats

*functions.But
how to use them is not very clear. Can anyone point me to right place to
achieve this objective?

Thanks in advance.
And sorry if that was too silly to ask.

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

Re: [libvirt] RFC: Use __attribute__ ((cleanup) in libvirt ?

[Martin Kletzander]

On Tue, Jan 10, 2017 at 02:50:40PM +0100, Erik Skultety wrote:

On Tue, Jan 10, 2017 at 02:17:00PM +0100, Martin Kletzander wrote:

On Tue, Jan 10, 2017 at 10:00:31AM +, Daniel P. Berrange wrote:
> On Tue, Jan 10, 2017 at 10:48:47AM +0100, Martin Kletzander wrote:
> > On Tue, Jan 10, 2017 at 02:58:21AM -0500, Laine Stump wrote:
> > > On 01/09/2017 08:09 PM, Richard W.M. Jones wrote:
> > > > On Mon, Jan 09, 2017 at 04:58:49PM +, Daniel P. Berrange wrote:
> > > > > For those who don't already know, GCC and CLang both implement a C 
language
> > > > > extension that enables automatic free'ing of resources when variables 
go
> > > > > out of scope. This is done by annotating the variable with the 
"cleanup"
> > > > > attribute, pointing to a function the compiler will wire up a call to 
when
> > > > > unwinding the stack. Since the annotation points to an arbitrary user
> > > > > defined function, you're not limited to simple free() like semantics. 
The
> > > > > cleanup function could unlock a mutex, or decrement a reference 
count, etc
> > > > >
> > > > > This annotation is used extensively by systemd, and libguestfs, 
amongst
> > > > > other projects. This obviously doesn't bring full garbage collection 
to
> > > > > C, but it does enable the code to be simplified. By removing the need 
to
> > > > > put in many free() (or equiv) calls to cleanup state, the 
"interesting"
> > > > > logic in the code stands out more, not being obscured by cleanup calls
> > > > > and goto jumps.
> > > > >
> > > > > I'm wondering what people think of making use of this in libvirt ?
> > > > >
> > > > > To my mind the only real reason to *not* use it, would be to maintain
> > > > > code portability to non-GCC/non-CLang compilers. OS-X, *BSD and *Linux
> > > > > all use GCC or CLang or both, so its a non-issue there. So the only 
place
> > > > > this could cause pain is people building libvirt on Win32, who are 
using
> > > > > the Microsoft compilers instead og GCC.
> > > > >
> >
> > Only reason I see for not using it is the "temporary" mess it will
> > cause.  Yes, we can change to that incrementally, but it will take some
> > time and effort and it will never be all of the code that uses it.
> > Don't get me wrong, I would love using more builtin compiler features
> > and shortening the code here and there.  I'm just worried this
> > particular one might be more disrupting than useful.  Most of us are
> > pretty used to the code flow we already have and there's nothing you
> > can't achieve without the cleanup attribute.
> >
> > And yes, I used quotation marks around the word temporary intentionally.
>
> Yes, that's why I thought of it as something that would make for a GSoc
> project - have someone do a full conversion of particular areas of code.
> eg convert all of util/ or convert the domain XML parser, etc. Basically
> if we did it, I think we'd want to have entire files converted at once.
> Only converting individual methods ad-hoc would be quite messy.
>

Yes, I know, but that still doesn't mean all will be converted,
unfortunately.



Why not? I mean, the GSoC project doesn't need to be for just 1 student, if
we're granted the slots you could pick multiple students who would work on it in
parallel. Also, there are always means how we could keep track of it, an idea


You need:

 1) To be accepted as an organization
 2) Slots
 3) Students that are interested in rewriting cleanup all over the
code.

You cannot depend on any of these.


that first crossed my mind without giving any more thinking to it is that you
can track the modules still waiting to be switched to the new convention within
the GSoC section of our page. The 'virsh auto-completion' project has been in
the 'ongoing' state for at least 2 years so I personally don't see an issue
here, since it's a bigger task.



It's also been mentioned by some developers as "really messy" and it's
not something that changes how people read code in the whole codebase.

Martin


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

Re: [libvirt] [PATCH v4] libxl: define a per-domain logger.

[Cedric Bosdonnat]

On Tue, 2017-01-10 at 12:21 +, Joao Martins wrote:
> Hey Cedric,
> 
> On 01/10/2017 09:03 AM, Cédric Bosdonnat wrote:
> > libxl doesn't provide a way to write one log for each domain. Thus
> > we need to demux the messages. If our logger doesn't know to which
> > domain to attribute a message, then it will write it to the default
> > log file.
> > 
> > Starting with Xen 4.9 (commit f9858025 and following), libxl will
> > write the domain ID in an easy to grab manner. The logger introduced
> > by this commit will use it to demux the libxl log messages.
> > 
> > Thanks to the default log file, this logger will also work with older
> > versions of Xen.
> 
> This is great stuff!
> 
> BTW, while applying the patch in my local repo, I noticed some issues (and
> confirmed make syntax-check is failing with them as well). But I assume those
> could be fixed on commit.

Indeed, fixed it locally. Thanks for the heads up.
--
Cedric

> Joao
> 
> > ---
> >  * v4: delay the log file opening to the first message write.
> > 
> >  * v3: add JSON-formatted libxl_domain_config to the newly opened
> >    file when creating a guest.
> > 
> >  * v2: addressed Jim's comments
> > 
> >  src/Makefile.am  |   1 +
> >  src/libxl/libxl_conf.c   |  38 +--
> >  src/libxl/libxl_conf.h   |   4 +-
> >  src/libxl/libxl_domain.c |   7 ++
> >  src/libxl/libxl_driver.c |   2 +
> >  src/libxl/libxl_logger.c | 257 
> > +++
> >  src/libxl/libxl_logger.h |  40 
> >  7 files changed, 312 insertions(+), 37 deletions(-)
> >  create mode 100644 src/libxl/libxl_logger.c
> >  create mode 100644 src/libxl/libxl_logger.h
> > 
> > diff --git a/src/Makefile.am b/src/Makefile.am
> > index 0dd2faaf6..58e3108a6 100644
> > --- a/src/Makefile.am
> > +++ b/src/Makefile.am
> > @@ -851,6 +851,7 @@ LIBXL_DRIVER_SOURCES =  
> > \
> >     libxl/libxl_capabilities.c libxl/libxl_capabilities.h   \
> >     libxl/libxl_domain.c libxl/libxl_domain.h   \
> >     libxl/libxl_driver.c libxl/libxl_driver.h   \
> > +   libxl/libxl_logger.c libxl/libxl_logger.h   \
> >     libxl/libxl_migration.c libxl/libxl_migration.h
> >  
> >  UML_DRIVER_SOURCES =   \
> > diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
> > index b569ddad8..ac83b51c7 100644
> > --- a/src/libxl/libxl_conf.c
> > +++ b/src/libxl/libxl_conf.c
> > @@ -76,9 +76,7 @@ libxlDriverConfigDispose(void *obj)
> >  
> >  virObjectUnref(cfg->caps);
> >  libxl_ctx_free(cfg->ctx);
> > -xtl_logger_destroy(cfg->logger);
> > -if (cfg->logger_file)
> > -VIR_FORCE_FCLOSE(cfg->logger_file);
> > +libxlLoggerFree(cfg->logger);
> >  
> >  VIR_FREE(cfg->configDir);
> >  VIR_FREE(cfg->autostartDir);
> > @@ -1356,8 +1354,6 @@ libxlDriverConfigPtr
> >  libxlDriverConfigNew(void)
> >  {
> >  libxlDriverConfigPtr cfg;
> > -char *log_file = NULL;
> > -xentoollog_level log_level = XTL_DEBUG;
> >  char ebuf[1024];
> >  unsigned int free_mem;
> >  
> > @@ -1386,9 +1382,6 @@ libxlDriverConfigNew(void)
> >  if (VIR_STRDUP(cfg->channelDir, LIBXL_CHANNEL_DIR) < 0)
> >  goto error;
> >  
> > -if (virAsprintf(&log_file, "%s/libxl-driver.log", cfg->logDir) < 0)
> > -goto error;
> > -
> >  if (virFileMakePath(cfg->logDir) < 0) {
> >  virReportError(VIR_ERR_INTERNAL_ERROR,
> > _("failed to create log dir '%s': %s"),
> > @@ -1397,37 +1390,13 @@ libxlDriverConfigNew(void)
> >  goto error;
> >  }
> >  
> > -if ((cfg->logger_file = fopen(log_file, "a")) == NULL)  {
> > -VIR_ERROR(_("Failed to create log file '%s': %s"),
> > -  log_file, virStrerror(errno, ebuf, sizeof(ebuf)));
> > -goto error;
> > -}
> > -VIR_FREE(log_file);
> > -
> > -switch (virLogGetDefaultPriority()) {
> > -case VIR_LOG_DEBUG:
> > -log_level = XTL_DEBUG;
> > -break;
> > -case VIR_LOG_INFO:
> > -log_level = XTL_INFO;
> > -break;
> > -case VIR_LOG_WARN:
> > -log_level = XTL_WARN;
> > -break;
> > -case VIR_LOG_ERROR:
> > -log_level = XTL_ERROR;
> > -break;
> > -}
> > -
> > -cfg->logger =
> > -(xentoollog_logger *)xtl_createlogger_stdiostream(cfg->logger_file,
> > -  log_level, 
> > XTL_STDIOSTREAM_SHOW_DATE);
> > +cfg->logger = libxlLoggerNew(cfg->logDir, virLogGetDefaultPriority());
> >  if (!cfg->logger) {
> >  VIR_ERROR(_("cannot create logger for libxenlight, disabling 
> > driver"));
> >  goto error;
> >  }
> >  
> > -if (libxl_ctx_alloc(&cfg->ctx, LIBXL_VERSION, 0, cfg->logger)) {
> > +if (libxl_ctx_alloc(&cfg->ctx, LIBXL_VERSION, 0, (xentoollog_logger 
> > *)cfg->logger)) {
> >  VIR_ERROR(_("can

Re: [libvirt] RFC: Use __attribute__ ((cleanup) in libvirt ?

[Erik Skultety]

On Tue, Jan 10, 2017 at 01:55:02PM +, Daniel P. Berrange wrote:
> On Tue, Jan 10, 2017 at 02:17:00PM +0100, Martin Kletzander wrote:
> > On Tue, Jan 10, 2017 at 10:00:31AM +, Daniel P. Berrange wrote:
> > > On Tue, Jan 10, 2017 at 10:48:47AM +0100, Martin Kletzander wrote:
> > > > On Tue, Jan 10, 2017 at 02:58:21AM -0500, Laine Stump wrote:
> > > > > On 01/09/2017 08:09 PM, Richard W.M. Jones wrote:
> > > > > > On Mon, Jan 09, 2017 at 04:58:49PM +, Daniel P. Berrange wrote:
> > > > > > > For those who don't already know, GCC and CLang both implement a 
> > > > > > > C language
> > > > > > > extension that enables automatic free'ing of resources when 
> > > > > > > variables go
> > > > > > > out of scope. This is done by annotating the variable with the 
> > > > > > > "cleanup"
> > > > > > > attribute, pointing to a function the compiler will wire up a 
> > > > > > > call to when
> > > > > > > unwinding the stack. Since the annotation points to an arbitrary 
> > > > > > > user
> > > > > > > defined function, you're not limited to simple free() like 
> > > > > > > semantics. The
> > > > > > > cleanup function could unlock a mutex, or decrement a reference 
> > > > > > > count, etc
> > > > > > >
> > > > > > > This annotation is used extensively by systemd, and libguestfs, 
> > > > > > > amongst
> > > > > > > other projects. This obviously doesn't bring full garbage 
> > > > > > > collection to
> > > > > > > C, but it does enable the code to be simplified. By removing the 
> > > > > > > need to
> > > > > > > put in many free() (or equiv) calls to cleanup state, the 
> > > > > > > "interesting"
> > > > > > > logic in the code stands out more, not being obscured by cleanup 
> > > > > > > calls
> > > > > > > and goto jumps.
> > > > > > >
> > > > > > > I'm wondering what people think of making use of this in libvirt ?
> > > > > > >
> > > > > > > To my mind the only real reason to *not* use it, would be to 
> > > > > > > maintain
> > > > > > > code portability to non-GCC/non-CLang compilers. OS-X, *BSD and 
> > > > > > > *Linux
> > > > > > > all use GCC or CLang or both, so its a non-issue there. So the 
> > > > > > > only place
> > > > > > > this could cause pain is people building libvirt on Win32, who 
> > > > > > > are using
> > > > > > > the Microsoft compilers instead og GCC.
> > > > > > >
> > > > 
> > > > Only reason I see for not using it is the "temporary" mess it will
> > > > cause.  Yes, we can change to that incrementally, but it will take some
> > > > time and effort and it will never be all of the code that uses it.
> > > > Don't get me wrong, I would love using more builtin compiler features
> > > > and shortening the code here and there.  I'm just worried this
> > > > particular one might be more disrupting than useful.  Most of us are
> > > > pretty used to the code flow we already have and there's nothing you
> > > > can't achieve without the cleanup attribute.
> > > > 
> > > > And yes, I used quotation marks around the word temporary intentionally.
> > > 
> > > Yes, that's why I thought of it as something that would make for a GSoc
> > > project - have someone do a full conversion of particular areas of code.
> > > eg convert all of util/ or convert the domain XML parser, etc. Basically
> > > if we did it, I think we'd want to have entire files converted at once.
> > > Only converting individual methods ad-hoc would be quite messy.
> > > 
> > 
> > Yes, I know, but that still doesn't mean all will be converted,
> > unfortunately.
> > 
> > > > > > > IMHO, it is perfectly valid for us to declare that MSVC is 
> > > > > > > unsupported
> > > > > > > with Libvirt and users must use GCC to build on Windows, either 
> > > > > > > natively
> > > > > > > via cygwin, or cross-build from Linux hosts.
> > > > 
> > > > I would love to know if anyone actually tried doing that lately.  Given
> > > > how often we are broken with mingw and we only foind out thanks to our
> > > > test suite (and sometomes the fixing takes more than a release cycle), I
> > > > think nobody does that and from what I know, it might not even work.
> > > 
> > > We have mingw in the CI system for a while now and its generally fixed
> > > as quickly as native arch builds are fixed these days.
> > > 
> > 
> > Yes.  Now.  But there was a build-breaker for several months that nobody
> > cared about.  Pity the builds are truncated so I can't track it back
> > properly.  My point is that I don't remember anyone asking about it
> > during the whole time, just us trying to come up with fixes.
> > 
> > > 
> > > > > > (2) You must not write code like:
> > > > > >
> > > > > >fn ()
> > > > > >{
> > > > > >  CLEANUP_FREE char *v; // uninitialized
> > > > > >
> > > > > >  if (some error condition) {
> > > > > >return -1;
> > > > > >  }
> > > > > >  ...
> > > > > >}
> > > > > >
> > > > > > because that will call free (v) on the uninitialized variable.
> > > > > > Sometimes GCC can spot this.  In libguest

Re: [libvirt] RFC: Use __attribute__ ((cleanup) in libvirt ?

[Daniel P. Berrange]

On Tue, Jan 10, 2017 at 02:17:00PM +0100, Martin Kletzander wrote:
> On Tue, Jan 10, 2017 at 10:00:31AM +, Daniel P. Berrange wrote:
> > On Tue, Jan 10, 2017 at 10:48:47AM +0100, Martin Kletzander wrote:
> > > On Tue, Jan 10, 2017 at 02:58:21AM -0500, Laine Stump wrote:
> > > > On 01/09/2017 08:09 PM, Richard W.M. Jones wrote:
> > > > > On Mon, Jan 09, 2017 at 04:58:49PM +, Daniel P. Berrange wrote:
> > > > > > For those who don't already know, GCC and CLang both implement a C 
> > > > > > language
> > > > > > extension that enables automatic free'ing of resources when 
> > > > > > variables go
> > > > > > out of scope. This is done by annotating the variable with the 
> > > > > > "cleanup"
> > > > > > attribute, pointing to a function the compiler will wire up a call 
> > > > > > to when
> > > > > > unwinding the stack. Since the annotation points to an arbitrary 
> > > > > > user
> > > > > > defined function, you're not limited to simple free() like 
> > > > > > semantics. The
> > > > > > cleanup function could unlock a mutex, or decrement a reference 
> > > > > > count, etc
> > > > > >
> > > > > > This annotation is used extensively by systemd, and libguestfs, 
> > > > > > amongst
> > > > > > other projects. This obviously doesn't bring full garbage 
> > > > > > collection to
> > > > > > C, but it does enable the code to be simplified. By removing the 
> > > > > > need to
> > > > > > put in many free() (or equiv) calls to cleanup state, the 
> > > > > > "interesting"
> > > > > > logic in the code stands out more, not being obscured by cleanup 
> > > > > > calls
> > > > > > and goto jumps.
> > > > > >
> > > > > > I'm wondering what people think of making use of this in libvirt ?
> > > > > >
> > > > > > To my mind the only real reason to *not* use it, would be to 
> > > > > > maintain
> > > > > > code portability to non-GCC/non-CLang compilers. OS-X, *BSD and 
> > > > > > *Linux
> > > > > > all use GCC or CLang or both, so its a non-issue there. So the only 
> > > > > > place
> > > > > > this could cause pain is people building libvirt on Win32, who are 
> > > > > > using
> > > > > > the Microsoft compilers instead og GCC.
> > > > > >
> > > 
> > > Only reason I see for not using it is the "temporary" mess it will
> > > cause.  Yes, we can change to that incrementally, but it will take some
> > > time and effort and it will never be all of the code that uses it.
> > > Don't get me wrong, I would love using more builtin compiler features
> > > and shortening the code here and there.  I'm just worried this
> > > particular one might be more disrupting than useful.  Most of us are
> > > pretty used to the code flow we already have and there's nothing you
> > > can't achieve without the cleanup attribute.
> > > 
> > > And yes, I used quotation marks around the word temporary intentionally.
> > 
> > Yes, that's why I thought of it as something that would make for a GSoc
> > project - have someone do a full conversion of particular areas of code.
> > eg convert all of util/ or convert the domain XML parser, etc. Basically
> > if we did it, I think we'd want to have entire files converted at once.
> > Only converting individual methods ad-hoc would be quite messy.
> > 
> 
> Yes, I know, but that still doesn't mean all will be converted,
> unfortunately.
> 
> > > > > > IMHO, it is perfectly valid for us to declare that MSVC is 
> > > > > > unsupported
> > > > > > with Libvirt and users must use GCC to build on Windows, either 
> > > > > > natively
> > > > > > via cygwin, or cross-build from Linux hosts.
> > > 
> > > I would love to know if anyone actually tried doing that lately.  Given
> > > how often we are broken with mingw and we only foind out thanks to our
> > > test suite (and sometomes the fixing takes more than a release cycle), I
> > > think nobody does that and from what I know, it might not even work.
> > 
> > We have mingw in the CI system for a while now and its generally fixed
> > as quickly as native arch builds are fixed these days.
> > 
> 
> Yes.  Now.  But there was a build-breaker for several months that nobody
> cared about.  Pity the builds are truncated so I can't track it back
> properly.  My point is that I don't remember anyone asking about it
> during the whole time, just us trying to come up with fixes.
> 
> > 
> > > > > (2) You must not write code like:
> > > > >
> > > > >fn ()
> > > > >{
> > > > >  CLEANUP_FREE char *v; // uninitialized
> > > > >
> > > > >  if (some error condition) {
> > > > >return -1;
> > > > >  }
> > > > >  ...
> > > > >}
> > > > >
> > > > > because that will call free (v) on the uninitialized variable.
> > > > > Sometimes GCC can spot this.  In libguestfs we tend to initialize
> > > > > every CLEANUP_* variable to either an explicit value or NULL.  GCC
> > > > > optimizes away calls to free (NULL).
> > > >
> > > 
> > > I'm trying to initialize all variables, always, so I don't see this as a
> > > problem, but there are 

Re: [libvirt] RFC: Use __attribute__ ((cleanup) in libvirt ?

[Erik Skultety]

On Tue, Jan 10, 2017 at 02:17:00PM +0100, Martin Kletzander wrote:
> On Tue, Jan 10, 2017 at 10:00:31AM +, Daniel P. Berrange wrote:
> > On Tue, Jan 10, 2017 at 10:48:47AM +0100, Martin Kletzander wrote:
> > > On Tue, Jan 10, 2017 at 02:58:21AM -0500, Laine Stump wrote:
> > > > On 01/09/2017 08:09 PM, Richard W.M. Jones wrote:
> > > > > On Mon, Jan 09, 2017 at 04:58:49PM +, Daniel P. Berrange wrote:
> > > > > > For those who don't already know, GCC and CLang both implement a C 
> > > > > > language
> > > > > > extension that enables automatic free'ing of resources when 
> > > > > > variables go
> > > > > > out of scope. This is done by annotating the variable with the 
> > > > > > "cleanup"
> > > > > > attribute, pointing to a function the compiler will wire up a call 
> > > > > > to when
> > > > > > unwinding the stack. Since the annotation points to an arbitrary 
> > > > > > user
> > > > > > defined function, you're not limited to simple free() like 
> > > > > > semantics. The
> > > > > > cleanup function could unlock a mutex, or decrement a reference 
> > > > > > count, etc
> > > > > >
> > > > > > This annotation is used extensively by systemd, and libguestfs, 
> > > > > > amongst
> > > > > > other projects. This obviously doesn't bring full garbage 
> > > > > > collection to
> > > > > > C, but it does enable the code to be simplified. By removing the 
> > > > > > need to
> > > > > > put in many free() (or equiv) calls to cleanup state, the 
> > > > > > "interesting"
> > > > > > logic in the code stands out more, not being obscured by cleanup 
> > > > > > calls
> > > > > > and goto jumps.
> > > > > >
> > > > > > I'm wondering what people think of making use of this in libvirt ?
> > > > > >
> > > > > > To my mind the only real reason to *not* use it, would be to 
> > > > > > maintain
> > > > > > code portability to non-GCC/non-CLang compilers. OS-X, *BSD and 
> > > > > > *Linux
> > > > > > all use GCC or CLang or both, so its a non-issue there. So the only 
> > > > > > place
> > > > > > this could cause pain is people building libvirt on Win32, who are 
> > > > > > using
> > > > > > the Microsoft compilers instead og GCC.
> > > > > >
> > > 
> > > Only reason I see for not using it is the "temporary" mess it will
> > > cause.  Yes, we can change to that incrementally, but it will take some
> > > time and effort and it will never be all of the code that uses it.
> > > Don't get me wrong, I would love using more builtin compiler features
> > > and shortening the code here and there.  I'm just worried this
> > > particular one might be more disrupting than useful.  Most of us are
> > > pretty used to the code flow we already have and there's nothing you
> > > can't achieve without the cleanup attribute.
> > > 
> > > And yes, I used quotation marks around the word temporary intentionally.
> > 
> > Yes, that's why I thought of it as something that would make for a GSoc
> > project - have someone do a full conversion of particular areas of code.
> > eg convert all of util/ or convert the domain XML parser, etc. Basically
> > if we did it, I think we'd want to have entire files converted at once.
> > Only converting individual methods ad-hoc would be quite messy.
> > 
> 
> Yes, I know, but that still doesn't mean all will be converted,
> unfortunately.
> 

Why not? I mean, the GSoC project doesn't need to be for just 1 student, if
we're granted the slots you could pick multiple students who would work on it in
parallel. Also, there are always means how we could keep track of it, an idea
that first crossed my mind without giving any more thinking to it is that you
can track the modules still waiting to be switched to the new convention within
the GSoC section of our page. The 'virsh auto-completion' project has been in
the 'ongoing' state for at least 2 years so I personally don't see an issue
here, since it's a bigger task.

Erik

> > > > > > IMHO, it is perfectly valid for us to declare that MSVC is 
> > > > > > unsupported
> > > > > > with Libvirt and users must use GCC to build on Windows, either 
> > > > > > natively
> > > > > > via cygwin, or cross-build from Linux hosts.
> > > 
> > > I would love to know if anyone actually tried doing that lately.  Given
> > > how often we are broken with mingw and we only foind out thanks to our
> > > test suite (and sometomes the fixing takes more than a release cycle), I
> > > think nobody does that and from what I know, it might not even work.
> > 
> > We have mingw in the CI system for a while now and its generally fixed
> > as quickly as native arch builds are fixed these days.
> > 
> 
> Yes.  Now.  But there was a build-breaker for several months that nobody
> cared about.  Pity the builds are truncated so I can't track it back
> properly.  My point is that I don't remember anyone asking about it
> during the whole time, just us trying to come up with fixes.
> 
> > 
> > > > > (2) You must not write code like:
> > > > >
> > > > >fn ()
> > > > >{
> > >

Re: [libvirt] [PATCH go-xml] Added Storage Pool and Storage Volume XML schemes.

[Daniel P. Berrange]

On Mon, Jan 09, 2017 at 08:15:21AM +0100, Alexey Slaykovsky wrote:
> Signed-off-by: Alexey Slaykovsky 
> ---
>  storage_encryption.go |  50 ++
>  storage_pool.go   | 121 +
>  storage_pool_test.go  | 240 
>  storage_vol.go|  83 +
>  storage_vol_test.go   | 246 
> ++
>  5 files changed, 740 insertions(+)
>  create mode 100644 storage_encryption.go
>  create mode 100644 storage_pool.go
>  create mode 100644 storage_pool_test.go
>  create mode 100644 storage_vol.go
>  create mode 100644 storage_vol_test.go
> 


> +type StoragePool struct {
> + XMLNamexml.Name   `xml:"pool"`
> + Type   string `xml:"type,attr"`
> + Name   string `xml:"name"`
> + UUID   string `xml:"uuid,omitempty"`
> + Allocation uint64 `xml:"allocation,omitempty"`
> + Capacity   uint64 `xml:"capacity,omitempty"`
> + Available  uint64 `xml:"available,omitempty"`

These should support units too, as with storage vols.

> + Target *StoragePoolTarget `xml:"target"`
> + Source *StoragePoolSource `xml:"source"`
> +}

> +
> +type StorageVolumeMemory struct {

s/Memory/Size/

> + Unit  string `xml:"unit,attr,omitempty"`
> + Value uint64 `xml:",chardata"`
> +}


I've pushed this with the changes mentioned above.

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

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

Re: [libvirt] qemu-kvm blocked for more than 120 seconds when "libvirt-guests" is enabled

[Oscar Segarra]

:_

2016-12-31 17:25 GMT+01:00 Oscar Segarra :

> Hi,
>
> I just have two virtual machines in my environment, I want them to
> gracefully stop when host is powered off gracefully.
>
> Nevertheless, system hangs on shutdown:
>
> [root@vdicnode01 ~]# virsh list
>  IdName   State
> 
>  1 vdicdb01   running
>  2 vdicone01  running
>
> The configuration:
>
> /usr/libexec/libvirt-guests.sh
> URIS=default
> ON_BOOT=ignore
> ON_SHUTDOWN=shutdown
> SHUTDOWN_TIMEOUT=60
> PARALLEL_SHUTDOWN=5
> START_DELAY=0
> BYPASS_CACHE=0
> CONNECT_RETRIES=10
> RETRIES_SLEEP=1
> SYNC_TIME=0
>
> [root@vdicnode01 ~]# cat /etc/libvirt/qemu.conf
> user  = "oneadmin"
> group = "oneadmin"
> dynamic_ownership = 0
> spice_tls = 0
>
> With libvirt-guests service started:
> [root@vdicnode01 ~]# service libvirt-guests status
> Redirecting to /bin/systemctl status  libvirt-guests.service
> ● libvirt-guests.service - Suspend Active Libvirt Guests
>Loaded: loaded (/usr/lib/systemd/system/libvirt-guests.service;
> disabled; vendor preset: disabled)
>Active: active (exited) since Sat 2016-12-31 17:13:34 CET; 5s ago
>  Docs: man:libvirtd(8)
>http://libvirt.org
>   Process: 6619 ExecStart=/usr/libexec/libvirt-guests.sh start
> (code=exited, status=0/SUCCESS)
>  Main PID: 6619 (code=exited, status=0/SUCCESS)
>
> Dec 31 17:13:34 vdicnode01.vdicube.com systemd[1]: Starting Suspend
> Active Libvirt Guests...
> Dec 31 17:13:34 vdicnode01.vdicube.com libvirt-guests.sh[6619]:
> libvirt-guests is configured not to start any guests on boot
> Dec 31 17:13:34 vdicnode01.vdicube.com systemd[1]: Started Suspend Active
> Libvirt Guests.
> [root@vdicnode01 ~]#
>
> If I stop locally libvirt-guests (as root) Looks work perfectly:
>
> [root@vdicnode01 ~]# virsh list
>  IdName   State
> 
>
> [root@vdicnode01 ~]#
>
> --> Now I start again the virtual machines and the libvirt-guests service
> <--
> [root@vdicnode01 ~]# service libvirtd restart
> Redirecting to /bin/systemctl restart  libvirtd.service
> [root@vdicnode01 ~]# service libvirt-guests start
> Redirecting to /bin/systemctl start  libvirt-guests.service
> [root@vdicnode01 ~]# virsh list
>  IdName   State
> 
>  1 vdicone01  running
>  2 vdicdb01   running
>
> [root@vdicnode01 ~]#
>
> But If I shutdown the host it look not work properly (I attack screenshots
> of the shutdown process).
>
> Any help will be welcome.
>
> Thanks a lot.
>
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] libxl: implement virDomainGetMaxVcpus

[Pavel Hrdina]

On Fri, Jan 06, 2017 at 12:35:21PM -0700, Jim Fehlig wrote:
> The libxl driver already supports getting maximum vcpu count via
> libxlDomainGetVcpusFlags, allowing to trivially implement
> virDomainGetMaxVcpus.
> 
> Signed-off-by: Jim Fehlig 
> ---
>  docs/news.html.in| 2 ++
>  src/libxl/libxl_driver.c | 8 
>  2 files changed, 10 insertions(+)

ACK

Pavel


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

Re: [libvirt] RFC: Use __attribute__ ((cleanup) in libvirt ?

[Martin Kletzander]

On Tue, Jan 10, 2017 at 10:00:31AM +, Daniel P. Berrange wrote:

On Tue, Jan 10, 2017 at 10:48:47AM +0100, Martin Kletzander wrote:

On Tue, Jan 10, 2017 at 02:58:21AM -0500, Laine Stump wrote:
> On 01/09/2017 08:09 PM, Richard W.M. Jones wrote:
> > On Mon, Jan 09, 2017 at 04:58:49PM +, Daniel P. Berrange wrote:
> > > For those who don't already know, GCC and CLang both implement a C 
language
> > > extension that enables automatic free'ing of resources when variables go
> > > out of scope. This is done by annotating the variable with the "cleanup"
> > > attribute, pointing to a function the compiler will wire up a call to when
> > > unwinding the stack. Since the annotation points to an arbitrary user
> > > defined function, you're not limited to simple free() like semantics. The
> > > cleanup function could unlock a mutex, or decrement a reference count, etc
> > >
> > > This annotation is used extensively by systemd, and libguestfs, amongst
> > > other projects. This obviously doesn't bring full garbage collection to
> > > C, but it does enable the code to be simplified. By removing the need to
> > > put in many free() (or equiv) calls to cleanup state, the "interesting"
> > > logic in the code stands out more, not being obscured by cleanup calls
> > > and goto jumps.
> > >
> > > I'm wondering what people think of making use of this in libvirt ?
> > >
> > > To my mind the only real reason to *not* use it, would be to maintain
> > > code portability to non-GCC/non-CLang compilers. OS-X, *BSD and *Linux
> > > all use GCC or CLang or both, so its a non-issue there. So the only place
> > > this could cause pain is people building libvirt on Win32, who are using
> > > the Microsoft compilers instead og GCC.
> > >

Only reason I see for not using it is the "temporary" mess it will
cause.  Yes, we can change to that incrementally, but it will take some
time and effort and it will never be all of the code that uses it.
Don't get me wrong, I would love using more builtin compiler features
and shortening the code here and there.  I'm just worried this
particular one might be more disrupting than useful.  Most of us are
pretty used to the code flow we already have and there's nothing you
can't achieve without the cleanup attribute.

And yes, I used quotation marks around the word temporary intentionally.


Yes, that's why I thought of it as something that would make for a GSoc
project - have someone do a full conversion of particular areas of code.
eg convert all of util/ or convert the domain XML parser, etc. Basically
if we did it, I think we'd want to have entire files converted at once.
Only converting individual methods ad-hoc would be quite messy.



Yes, I know, but that still doesn't mean all will be converted,
unfortunately.


> > > IMHO, it is perfectly valid for us to declare that MSVC is unsupported
> > > with Libvirt and users must use GCC to build on Windows, either natively
> > > via cygwin, or cross-build from Linux hosts.

I would love to know if anyone actually tried doing that lately.  Given
how often we are broken with mingw and we only foind out thanks to our
test suite (and sometomes the fixing takes more than a release cycle), I
think nobody does that and from what I know, it might not even work.


We have mingw in the CI system for a while now and its generally fixed
as quickly as native arch builds are fixed these days.



Yes.  Now.  But there was a build-breaker for several months that nobody
cared about.  Pity the builds are truncated so I can't track it back
properly.  My point is that I don't remember anyone asking about it
during the whole time, just us trying to come up with fixes.




> > (2) You must not write code like:
> >
> >fn ()
> >{
> >  CLEANUP_FREE char *v; // uninitialized
> >
> >  if (some error condition) {
> >return -1;
> >  }
> >  ...
> >}
> >
> > because that will call free (v) on the uninitialized variable.
> > Sometimes GCC can spot this.  In libguestfs we tend to initialize
> > every CLEANUP_* variable to either an explicit value or NULL.  GCC
> > optimizes away calls to free (NULL).
>

I'm trying to initialize all variables, always, so I don't see this as a
problem, but there are some of us that (I have the feeling) are trying
to initialize as few as possible, so this (although it's a different
story) might still be a problem for someone.


We pretty much have the same problem already with 'goto cleanup' - you
have to make sure everything is initialized sanely before the first
"goto cleanup". So I think we're safe in this respect already and
the cleanup attributes wouldn't make it any more complex.



Yeah, but with __attribute__((cleanup)), you need to make sure
everything is properly initialized immediatelly as opposed to before
first cleanup.  I know it sounds easy, and it is.  And I love doing that
even without __attribute__((cleanup)), I just see the potential for
error.  Hopefully we'd be able to do a syntax-check

Re: [libvirt] [PATCH 0/2] Attempt, and fail, to fix build with Clang 3.9 / Linux

[Andrea Bolognani]

On Fri, 2017-01-06 at 15:48 -0600, Eric Blake wrote:
> > The last one I was unable to work around in any sensible
> > manner: when linking, clang fails with
> > 
> >   ../gnulib/lib/.libs/libgnu.a(mgetgroups.o):
> >   In function `realloc_groupbuf':
> >   .../libvirt/gnulib/lib/mgetgroups.c:45:
> >   undefined reference to `__muloti4'
> > 
> > This seems to be a well-known issue[1] with no solution in
> > sight; passing CFLAGS='-rtlib=compiler-rt' to configure
> > allows the compilation to succeed but, yeah, way gross.
> 
> And gnulib just fixed things to avoid tickling that clang bug:
> 
> http://git.savannah.gnu.org/cgit/gnulib.git/commit/?id=04441fd8
> 
> Work around LLVM bug 16404, which is still not fixed.
> https://llvm.org/bugs/show_bug.cgi?id=16404
> Problem reported by Nelson H. F. Beebe.

Wow, that's great!

> Shall I go ahead and update the gnulib submodule to the latest, and see
> if you can get further with your clang tests as a result?

I might not be able to play with Clang again in the immediate
future, but I don't see why that should prevent us from paving
the way for other parties who might be interested. Please go
right ahead :)

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [PATCHv2 02/11] storage: Fix implementation of no-overwrite for file system backend

[Michal Privoznik]

On 01/10/2017 01:36 PM, John Ferlan wrote:
> 
> [...]
> 
>>
>> Yeah, I think if this function was called MatchFS(), CheckFS() or
>> something among those lines I'm okay with this patch. But ProbeFS()
>> makes me think that the function is just probing for existing FSs on
>> given device. Sorry for the bikeshedding :-).
> 
> No problem - I'd rather get the name right now instead of having a
> followup because someone didn't like the name.
> 
> How about FindFS() - the function is looking to "find" if a FS exists on
> the disk/partition.
> 
> That would mean ProbeEmpty could be FindNoFS or FindEmpty (and the
> fallout in the remaining patches to adjust the names).

Looks reasonable to me.

Michal

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

Re: [libvirt] [PATCHv2 02/11] storage: Fix implementation of no-overwrite for file system backend

[John Ferlan]

[...]

> 
> Yeah, I think if this function was called MatchFS(), CheckFS() or
> something among those lines I'm okay with this patch. But ProbeFS()
> makes me think that the function is just probing for existing FSs on
> given device. Sorry for the bikeshedding :-).

No problem - I'd rather get the name right now instead of having a
followup because someone didn't like the name.

How about FindFS() - the function is looking to "find" if a FS exists on
the disk/partition.

That would mean ProbeEmpty could be FindNoFS or FindEmpty (and the
fallout in the remaining patches to adjust the names).

John

> 
> ACK if you make the function name reflect what the function actually does.
> 
> Michal
> 

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

Re: [libvirt] [PATCH] Extend l3 cache to nodeinfo

[Martin Kletzander]

On Tue, Jan 10, 2017 at 04:11:03PM +0800, Eli Qiao wrote:

This patch extends l3 cache infomation to nodeinfo output.



This doesn't make sense, in case there are multiple sockets with
different L3 cache sizes, it can't be represented in nodeinfo.  If there
can be multiple values, you need to extend virsh capabilities instead.


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

Re: [libvirt] [PATCHv2 02/11] storage: Fix implementation of no-overwrite for file system backend

[Michal Privoznik]

On 01/10/2017 01:12 AM, John Ferlan wrote:
> 
> 
> On 01/09/2017 10:32 AM, Michal Privoznik wrote:
>> On 12/15/2016 10:42 PM, John Ferlan wrote:
>>> https://bugzilla.redhat.com/show_bug.cgi?id=1363586
>>>
>>> Commit id '27758859' introduced the "NO_OVERWRITE" flag check for
>>> file system backends; however, the implementation, documentation,
>>> and algorithm was inconsistent. For the "flag" description for the
>>> API the flag was described as "Do not overwrite existing pool";
>>> however, within the storage backend code the flag is described
>>> as "it probes to determine if filesystem already exists on the
>>> target device, renurning an error if exists".
>>>
>>> The code itself was implemented using the paradigm to set up the
>>> superblock probe by creating a filter that would cause the code
>>> to only search for the provided format type. If that type wasn't
>>> found, then the algorithm would return success allowing the caller
>>> to format the device. If the format type already existed on the
>>> device, then the code would fail indicating that the a filesystem
>>> of the same type existed on the device.
>>>
>>> The result is that if someone had a file system of one type on the
>>> device, it was possible to overwrite it if a different format type
>>> was specified in updated XML effectively trashing whatever was on
>>> the device already.
>>>
>>> This patch alters what NO_OVERWRITE does for a file system backend
>>> to be more realistic and consistent with what should be expected when
>>> the caller requests to not overwrite the data on the disk.
>>>
>>> Rather than filter results based on the expected format type, the
>>> code will allow success/failure be determined solely on whether the
>>> blkid_do_probe calls finds some known format on the device. This
>>> adjustment also allows removal of the virStoragePoolProbeResult
>>> enum that was under utilized.
>>>
>>> If it does find a formatted file system different errors will be
>>> generated indicating a file system of a specific type already exists
>>> or a file system of some other type already exists.
>>>
>>> In the original virsh support commit id 'ddcd5674', the description
>>> for '--no-overwrite' within the 'pool-build' command help output
>>> has an ambiguous "of this type" included in the short description.
>>> Compared to the longer description within the "Build a given pool."
>>> section of the virsh.pod file it's more apparent that the meaning
>>> of this flag would cause failure if a probe of the target already
>>> has a filesystem.
>>>
>>> So this patch also modifies the short description to just be the
>>> antecedent of the 'overwrite' flag, which matches the API description.
>>> This patch also modifies the grammar in virsh.pod for no-overwrite
>>> as well as reworking the paragraph formats to make it easier to read.
>>>
>>> Signed-off-by: John Ferlan 
>>> ---
>>>
>>>   v1: http://www.redhat.com/archives/libvir-list/2016-November/msg00784.html
>>>
>>>   Changes aplenty since v1 - the subsequent patches are a result of the
>>>   review comments from jtomko vis-a-vis using blkid for the partitions
>>>
>>>  src/storage/storage_backend.c| 68 
>>> ++--
>>>  src/storage/storage_backend_fs.c | 15 +
>>>  src/storage/storage_backend_fs.h |  5 ---
>>>  tools/virsh-pool.c   |  2 +-
>>>  tools/virsh.pod  | 32 ++-
>>>  5 files changed, 58 insertions(+), 64 deletions(-)
>>>
>>> diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
>>> index 2432b54..108f2b9 100644
>>> --- a/src/storage/storage_backend.c
>>> +++ b/src/storage/storage_backend.c
>>> @@ -2646,87 +2646,80 @@ virStorageBackendFindGlusterPoolSources(const char 
>>> *host ATTRIBUTE_UNUSED,
>>>   * Use the blkid_ APIs in order to get details regarding whether a file
>>>   * system exists on the disk already.
>>>   *
>>> - * Returns @virStoragePoolProbeResult value, where any error will also
>>> - * set the error message.
>>> + * Returns:
>>> + *   -1: An error was encountered, with error message set
>>> + *0: No file system found
>>
>> Let me just say I find this very confusing. Why would ProbeFS() function
>> make decisions whether it is possible to rewrite FS or not? I  mean, I'd
>> expect ProbeFS() function to fetch me the FS on @device so that I,
>> caller, can make the decision myself.
> 
>> Was this something that was requested by Jan?
> 
> History? I just shortened the "FileSystem" to "FS" and inserted that
> BLKID to indicate the tool being used to do the probe. AFAIK, BLKID and
> PARTED are the existing options - all I'm looking to do is combine them
> which was something Jan hinted at IIRC.
> 
>>
>> Maybe it's a naming problem. MatchFS() sounds more like reflecting what
>> this function actually does after this change.
>>
> 
> Changing a name is easy, but after reading your comment in 3, I think
> inserting a step between 1 and 2 would at least remove the conce

Re: [libvirt] [PATCH v4] libxl: define a per-domain logger.

[Joao Martins]

Hey Cedric,

On 01/10/2017 09:03 AM, Cédric Bosdonnat wrote:
> libxl doesn't provide a way to write one log for each domain. Thus
> we need to demux the messages. If our logger doesn't know to which
> domain to attribute a message, then it will write it to the default
> log file.
> 
> Starting with Xen 4.9 (commit f9858025 and following), libxl will
> write the domain ID in an easy to grab manner. The logger introduced
> by this commit will use it to demux the libxl log messages.
> 
> Thanks to the default log file, this logger will also work with older
> versions of Xen.
This is great stuff!

BTW, while applying the patch in my local repo, I noticed some issues (and
confirmed make syntax-check is failing with them as well). But I assume those
could be fixed on commit.

Joao

> ---
>  * v4: delay the log file opening to the first message write.
> 
>  * v3: add JSON-formatted libxl_domain_config to the newly opened
>file when creating a guest.
> 
>  * v2: addressed Jim's comments
> 
>  src/Makefile.am  |   1 +
>  src/libxl/libxl_conf.c   |  38 +--
>  src/libxl/libxl_conf.h   |   4 +-
>  src/libxl/libxl_domain.c |   7 ++
>  src/libxl/libxl_driver.c |   2 +
>  src/libxl/libxl_logger.c | 257 
> +++
>  src/libxl/libxl_logger.h |  40 
>  7 files changed, 312 insertions(+), 37 deletions(-)
>  create mode 100644 src/libxl/libxl_logger.c
>  create mode 100644 src/libxl/libxl_logger.h
> 
> diff --git a/src/Makefile.am b/src/Makefile.am
> index 0dd2faaf6..58e3108a6 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -851,6 +851,7 @@ LIBXL_DRIVER_SOURCES =
> \
>   libxl/libxl_capabilities.c libxl/libxl_capabilities.h   \
>   libxl/libxl_domain.c libxl/libxl_domain.h   \
>   libxl/libxl_driver.c libxl/libxl_driver.h   \
> + libxl/libxl_logger.c libxl/libxl_logger.h   \
>   libxl/libxl_migration.c libxl/libxl_migration.h
>  
>  UML_DRIVER_SOURCES = \
> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
> index b569ddad8..ac83b51c7 100644
> --- a/src/libxl/libxl_conf.c
> +++ b/src/libxl/libxl_conf.c
> @@ -76,9 +76,7 @@ libxlDriverConfigDispose(void *obj)
>  
>  virObjectUnref(cfg->caps);
>  libxl_ctx_free(cfg->ctx);
> -xtl_logger_destroy(cfg->logger);
> -if (cfg->logger_file)
> -VIR_FORCE_FCLOSE(cfg->logger_file);
> +libxlLoggerFree(cfg->logger);
>  
>  VIR_FREE(cfg->configDir);
>  VIR_FREE(cfg->autostartDir);
> @@ -1356,8 +1354,6 @@ libxlDriverConfigPtr
>  libxlDriverConfigNew(void)
>  {
>  libxlDriverConfigPtr cfg;
> -char *log_file = NULL;
> -xentoollog_level log_level = XTL_DEBUG;
>  char ebuf[1024];
>  unsigned int free_mem;
>  
> @@ -1386,9 +1382,6 @@ libxlDriverConfigNew(void)
>  if (VIR_STRDUP(cfg->channelDir, LIBXL_CHANNEL_DIR) < 0)
>  goto error;
>  
> -if (virAsprintf(&log_file, "%s/libxl-driver.log", cfg->logDir) < 0)
> -goto error;
> -
>  if (virFileMakePath(cfg->logDir) < 0) {
>  virReportError(VIR_ERR_INTERNAL_ERROR,
> _("failed to create log dir '%s': %s"),
> @@ -1397,37 +1390,13 @@ libxlDriverConfigNew(void)
>  goto error;
>  }
>  
> -if ((cfg->logger_file = fopen(log_file, "a")) == NULL)  {
> -VIR_ERROR(_("Failed to create log file '%s': %s"),
> -  log_file, virStrerror(errno, ebuf, sizeof(ebuf)));
> -goto error;
> -}
> -VIR_FREE(log_file);
> -
> -switch (virLogGetDefaultPriority()) {
> -case VIR_LOG_DEBUG:
> -log_level = XTL_DEBUG;
> -break;
> -case VIR_LOG_INFO:
> -log_level = XTL_INFO;
> -break;
> -case VIR_LOG_WARN:
> -log_level = XTL_WARN;
> -break;
> -case VIR_LOG_ERROR:
> -log_level = XTL_ERROR;
> -break;
> -}
> -
> -cfg->logger =
> -(xentoollog_logger *)xtl_createlogger_stdiostream(cfg->logger_file,
> -  log_level, XTL_STDIOSTREAM_SHOW_DATE);
> +cfg->logger = libxlLoggerNew(cfg->logDir, virLogGetDefaultPriority());
>  if (!cfg->logger) {
>  VIR_ERROR(_("cannot create logger for libxenlight, disabling 
> driver"));
>  goto error;
>  }
>  
> -if (libxl_ctx_alloc(&cfg->ctx, LIBXL_VERSION, 0, cfg->logger)) {
> +if (libxl_ctx_alloc(&cfg->ctx, LIBXL_VERSION, 0, (xentoollog_logger 
> *)cfg->logger)) {
>  VIR_ERROR(_("cannot initialize libxenlight context, probably not "
>  "running in a Xen Dom0, disabling driver"));
>  goto error;
> @@ -1478,7 +1447,6 @@ libxlDriverConfigNew(void)
>  return cfg;
>  
>   error:
> -VIR_FREE(log_file);
>  virObjectUnref(cfg);
>  return NULL;
>  }
> diff --git a/src/libxl/libxl_conf.h b/src/libxl/libxl_conf.h
> ind

Re: [libvirt] [PATCH v2 5/5] qemu: Use transactions from security driver

[Michal Privoznik]

On 01/09/2017 11:18 PM, John Ferlan wrote:
> 
> 
> On 01/09/2017 07:58 AM, Michal Privoznik wrote:
>> So far if qemu is spawned under separate mount namespace in order
>> to relabel everything it needs an access to the security driver
>> is run in that namespace too. This has a very nasty down side -
> 
> s/is/to/
> 
>> it is being run in a separate process, so any internal state
>> transition is NOT reflected in the dameon. This can lead to many
> 
> s/dameon/daemon
> 
>> sleepless nights. Therefore, use the transaction APIs so that
>> libvirt developers can sleep tight again.
> 
> Having trouble sleeping lately? ;-)
> 
> 
>>
>> Signed-off-by: Michal Privoznik 
>> ---
>>  src/qemu/qemu_security.c | 100 
>> ++-
>>  1 file changed, 30 insertions(+), 70 deletions(-)
>>
>> diff --git a/src/qemu/qemu_security.c b/src/qemu/qemu_security.c
>> index 9ab91e9f2..544feeb4a 100644
>> --- a/src/qemu/qemu_security.c
>> +++ b/src/qemu/qemu_security.c
>> @@ -40,66 +40,31 @@ struct qemuSecuritySetRestoreAllLabelData {
>>  };
>>  
>>  
>> -static int
>> -qemuSecuritySetRestoreAllLabelHelper(pid_t pid,
>> - void *opaque)
>> -{
>> -struct qemuSecuritySetRestoreAllLabelData *data = opaque;
>> -
>> -virSecurityManagerPostFork(data->driver->securityManager);
>> -
>> -if (data->set) {
>> -VIR_DEBUG("Setting up security labels inside namespace pid=%lld",
>> -  (long long) pid);
>> -if (virSecurityManagerSetAllLabel(data->driver->securityManager,
>> -  data->vm->def,
>> -  data->stdin_path) < 0)
>> -return -1;
>> -} else {
>> -VIR_DEBUG("Restoring security labels inside namespace pid=%lld",
>> -  (long long) pid);
>> -if (virSecurityManagerRestoreAllLabel(data->driver->securityManager,
>> -  data->vm->def,
>> -  data->migrated) < 0)
>> -return -1;
>> -}
>> -
>> -return 0;
>> -}
>> -
>> -
>>  int
>>  qemuSecuritySetAllLabel(virQEMUDriverPtr driver,
>>  virDomainObjPtr vm,
>>  const char *stdin_path)
>>  {
>> -struct qemuSecuritySetRestoreAllLabelData data;
>> +int ret = -1;
>>  
>> -memset(&data, 0, sizeof(data));
>> +if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT) &&
>> +virSecurityManagerTransactionStart(driver->securityManager) < 0)
>> +goto cleanup;
>>  
>> -data.set = true;
>> -data.driver = driver;
>> -data.vm = vm;
>> -data.stdin_path = stdin_path;
>> +if (virSecurityManagerSetAllLabel(driver->securityManager,
>> +  vm->def,
>> +  stdin_path) < 0)
>> +goto cleanup;
>>  
>> -if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) {
>> -if (virSecurityManagerPreFork(driver->securityManager) < 0)
>> -return -1;
> 
> Both paths have removed the PreFork/PostFork processing... Is that then
> no longer required?  This is/was the only PreFork caller I think.


Yes, it is no longer required. There is no fork() happening in
virSecurityManagerSetAllLabel() anymore.

Michal

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

Re: [libvirt] [PATCH] Extend l3 cache to nodeinfo

[Qiao, Liyong]

Hi Daniel,

Thanks for your comments.

I will not change public ABI nor break RPC.

I wonder if you would agree with extend virHostCPUGetInfoPopulateLinux to get 
l3 cache size ?
Such as:
virHostCPUGetInfoPopulateLinux(FILE *cpuinfo,
   virArch arch,
   unsigned int *cpus,
   unsigned int *mhz,
   unsigned int *nodes,
   unsigned int *sockets,
   unsigned int *cores,
   unsigned int *threads,
   unsigned int *l3_cache)  ←-

virHostCPUGetInfoPopulateLinux is doing parsing /proc/cpuinfo , so I think it’s 
the best way to get l3 cache size.

cat /proc/cpuinfo

…
cpu MHz : 2821.435
cache size  : 56320 KB
…


Best Regards

Eli Qiao(乔立勇）OpenStack Core team OTC Intel.
-- 


On 10/01/2017, 5:44 PM, "Daniel P. Berrange"  wrote:

On Tue, Jan 10, 2017 at 04:11:03PM +0800, Eli Qiao wrote:
> This patch extends l3 cache infomation to nodeinfo output.
> 
> Signed-off-by: Eli Qiao 
> ---
>  include/libvirt/libvirt-host.h |  1 +
>  src/nodeinfo.c |  3 ++-
>  src/remote/remote_protocol.x   |  1 +
>  src/test/test_driver.c |  1 +
>  src/util/virhostcpu.c  | 29 +
>  src/util/virhostcpu.h  |  3 ++-
>  src/util/virhostcpupriv.h  |  3 ++-
>  tests/virhostcputest.c |  3 ++-
>  tools/virsh-host.c |  1 +
>  9 files changed, 37 insertions(+), 8 deletions(-)
> 
> diff --git a/include/libvirt/libvirt-host.h 
b/include/libvirt/libvirt-host.h
> index 07b5d15..ba926df 100644
> --- a/include/libvirt/libvirt-host.h
> +++ b/include/libvirt/libvirt-host.h
> @@ -167,6 +167,7 @@ struct _virNodeInfo {
>   processors in case of unusual NUMA 
topology*/
>  unsigned int threads; /* number of threads per core, 1 in case of
>   unusual numa topology */
> +unsigned int l3_cache; /* l3 cache in kilobytes */
>  };

NACK, it is *forbidden* to change public structs as this breaks
ABI compatibility.

Okay, get it.

> diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x
> index b846ef2..6a16b4e 100644
> --- a/src/remote/remote_protocol.x
> +++ b/src/remote/remote_protocol.x
> @@ -489,6 +489,7 @@ struct remote_node_get_info_ret { /* insert@1 */
>  int sockets;
>  int cores;
>  int threads;
> +int l3_cache;
>  };

Likewise this breaks RPC compatibility.

  This info wil need to be reported in the capabilities XML instead.

Sure.

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



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

Re: [libvirt] [PATCH] Extend l3 cache to nodeinfo

[Daniel P. Berrange]

On Tue, Jan 10, 2017 at 12:05:00PM +, Qiao, Liyong wrote:
> Hi Daniel,
> 
> Thanks for your comments.
> 
> I will not change public ABI nor break RPC.
> 
> I wonder if you would agree with extend virHostCPUGetInfoPopulateLinux to get 
> l3 cache size ?
> Such as:
> virHostCPUGetInfoPopulateLinux(FILE *cpuinfo,
>virArch arch,
>unsigned int *cpus,
>unsigned int *mhz,
>unsigned int *nodes,
>unsigned int *sockets,
>unsigned int *cores,
>unsigned int *threads,
>unsigned int *l3_cache)  ←-
> 
> virHostCPUGetInfoPopulateLinux is doing parsing /proc/cpuinfo , so I think 
> it’s the best way to get l3 cache size.
> 
> cat /proc/cpuinfo

/proc/cpuinfo is a pretty awful file because its contents are completely
different on every linux architecture. It would be preferrable to get
the cache sizes from /sys/devices/system/cpu/* if possible.


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

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

Re: [libvirt] [PATCH v3 1/3] qemu: Use virtio-pci by default for mach-virt guests

[Andrea Bolognani]

On Tue, 2017-01-10 at 03:38 -0500, Laine Stump wrote:
[...]
> > -if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIRTIO_MMIO)) {
> > -qemuDomainPrimeVirtioDeviceAddresses(
> > -def, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO);
> > +/* We use virtio-mmio by default on mach-virt guests only if they 
> > already
> > + * have at least one virtio-mmio device: in all other cases, we prefer
> > + * virtio-pci */
> > +if (qemuDomainMachineHasPCIeRoot(def) &&
> > +!qemuDomainHasVirtioMMIODevices(def)) {
> > +qemuDomainPrimeVirtioDeviceAddresses(def,
> > + 
> > VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI);
> 
> I'm having trouble remembering, and it's too early in the morning to go
> looking in the code - it seems like priming the addresses with PCI might
> not be necessary - as long as we simply *don't* prime them with
> virtio-mmio. I could be misremembering though. ACK either way.

It looks like that's indeed the case from changing the code
accordingly and running the test suite.

However, a cursory look (it's morning here as well ;) at the
code didn't give me enough confidence that it's working by
design rather than pure chance, and with the freeze coming
up shortly I decided to push the possibly slightly suboptimal
code instead. I will go back to this and remove it if it
turns out to be safe to do so after 3.0.0 is out.

> (ACK is based on the assumption that all concerned ARM parties have
> agreed that it's time to do this. I *think* that is the case :-)

It's been agreed a while ago that the only thing stopping us
from changing the default was poor guest support; as mentioned
in the commit message, that's no longer a concern, so onwards
to the future we go! :)

Thanks for the review.

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [PATCH v3 2/3] docs: Document virtio-mmio by default for mach-virt guests

[Andrea Bolognani]

On Tue, 2017-01-10 at 03:38 -0500, Laine Stump wrote:
> Don't you want the subject to say "Document virtio-pci by default"?

Of course I did :) Good catch!

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [PATCH 0/4] qemu: Don't leave temp mount dirs around

[Ján Tomko]

On Fri, Jan 06, 2017 at 11:30:39AM +0100, Michal Privoznik wrote:

After yesterday's patches [1] and fixing BZ [2] with them, the comment 4 got me
thinking: why leave /var/run/libvirt/$domain.pts and similar temporary
directories around? We need them just for a very short time. After that we
might as well remove them.

1: https://www.redhat.com/archives/libvir-list/2017-January/msg00073.html
2: https://bugzilla.redhat.com/show_bug.cgi?id=1406837

Michal Privoznik (4):
 qemuDomainCreateNamespace: s/unlink/rmdir/
 qemuDomainGetPreservedMounts: Do not special case /dev
 qemuDomainCreateNamespace: move mkdir to qemuDomainBuildNamespace
 qemu: Drop qemuDomainDeleteNamespace

src/qemu/qemu_domain.c  | 126 +---
src/qemu/qemu_domain.h  |   3 --
src/qemu/qemu_process.c |   2 -
3 files changed, 34 insertions(+), 97 deletions(-)


ACK series.

Jan


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

[libvirt] [PATCH go] Add support for perf cache_l1d event

[Daniel P. Berrange]

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

Pushed as a build fix

 connect.go  | 6 ++
 domain.go   | 6 ++
 domain_compat.h | 4 
 3 files changed, 16 insertions(+)

diff --git a/connect.go b/connect.go
index 35eb7fd..41c54b1 100644
--- a/connect.go
+++ b/connect.go
@@ -2253,6 +2253,8 @@ type DomainStatsPerf struct {
StalledCyclesBackend uint64
RefCpuCyclesSet  bool
RefCpuCycles uint64
+   CacheL1DSet  bool
+   CacheL1D uint64
 }
 
 func getDomainStatsPerfFieldInfo(params *DomainStatsPerf) 
map[string]typedParamsFieldInfo {
@@ -2309,6 +2311,10 @@ func getDomainStatsPerfFieldInfo(params 
*DomainStatsPerf) map[string]typedParams
set: ¶ms.RefCpuCyclesSet,
ul:  ¶ms.RefCpuCycles,
},
+   "perf.cache_l1d": typedParamsFieldInfo{
+   set: ¶ms.CacheL1DSet,
+   ul:  ¶ms.CacheL1D,
+   },
}
 }
 
diff --git a/domain.go b/domain.go
index 515138a..3e66dab 100644
--- a/domain.go
+++ b/domain.go
@@ -3183,6 +3183,8 @@ type DomainPerfEvents struct {
StalledCyclesBackend bool
RefCpuCyclesSet  bool
RefCpuCycles bool
+   CacheL1DSet  bool
+   CacheL1D bool
 }
 
 /* Remember to also update DomainStatsPerf in connect.go when adding to the 
stuct above */
@@ -3241,6 +3243,10 @@ func getDomainPerfEventsFieldInfo(params 
*DomainPerfEvents) map[string]typedPara
set: ¶ms.RefCpuCyclesSet,
b:   ¶ms.RefCpuCycles,
},
+   C.VIR_PERF_PARAM_CACHE_L1D: typedParamsFieldInfo{
+   set: ¶ms.CacheL1DSet,
+   b:   ¶ms.CacheL1D,
+   },
}
 }
 
diff --git a/domain_compat.h b/domain_compat.h
index 14072d0..4da562b 100644
--- a/domain_compat.h
+++ b/domain_compat.h
@@ -45,6 +45,10 @@
 #define VIR_PERF_PARAM_REF_CPU_CYCLES "ref_cpu_cycles"
 #endif
 
+#ifndef VIR_PERF_PARAM_CACHE_L1D
+#define VIR_PERF_PARAM_CACHE_L1D "cache_l1d"
+#endif
+
 #ifndef VIR_DOMAIN_EVENT_ID_METADATA_CHANGE
 #define VIR_DOMAIN_EVENT_ID_METADATA_CHANGE 23
 #endif
-- 
2.9.3

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

Re: [libvirt] [PATCH 1/2] conf: new function virDomainPCIAddressSetAllMulti()

[Andrea Bolognani]

On Tue, 2017-01-10 at 03:23 -0500, Laine Stump wrote:
> This utility function looks through a virPCIAddressSet for any slot of
> any bus that has multiple functions in use, and sets the "multi" flag
> in the virDomainDeviceInfo for the device that is assigned to function
> 0 of that slot (as long as it hasn't already been set explicitly by
> someone who presumably has better information than we do).
> 
> It isn't yet called from anywhere, so will have no functional effect.

[...]
> +static int
> +virDomainPCIAddressSetMultiIter(virDomainDefPtr def ATTRIBUTE_UNUSED,
> +virDomainDeviceDefPtr dev ATTRIBUTE_UNUSED,
> +virDomainDeviceInfoPtr info,
> +void *data)

virDomainPCIAddressSetAllMultiIter()?

Or not :)

[...]
> +/**
> + * virDomainPCIAddressSetAllMulti():
> + *
> + * @def: the domain definition whose devices may need adjusting
> + * @addrs: address set keeping track of all addresses in use.
> + *
> + * Look for any PCI slots that have multiple functions assigned, and
> + * set multi to YES in the address for the device at function 0

s/YES/ON/

> + * (unless it has been explicitly set to NO).

s/NO/OFF/

> + *
> + * No return code, since there is no possibility of failure.
> + */
> +void
> +virDomainPCIAddressSetAllMulti(virDomainDefPtr def,
> +   virDomainPCIAddressSetPtr addrs)

I also just realized that virDomainPCIAddressSetAllMulti()
might be mistaken for vir::DomainPCIAddressSet::AllMulti()
instead of vir::Domain::PCIAddressSetAllMulti().

The joys of pretending to use an object-oriented language
when actually all you have is plain old C :)

> +{
> +/* Scan through all the slots in @addrs looking for any that have
> + * more than just function 0 marked as in use, then use an
> + * iterator to find the DeviceInfo that uses function 0 on that
> + * slot and mark it as multi = YES

s/YES/ON/

> + */
> +size_t busIdx, slotIdx;
> +
> +for (busIdx = 0; busIdx < addrs->nbuses; busIdx++) {
> +virDomainPCIAddressBusPtr bus = &addrs->buses[busIdx];
> +
> +for (slotIdx = bus->minSlot; slotIdx <= bus->maxSlot; slotIdx++) {
> +if (bus->slot[slotIdx].functions > 1) {
> +virPCIDeviceAddress addr = { .domain = 0,

This doesn't look right. What happens if the multifunction
device is at eg. 0001:00:04.0?

...

Oh, never mind, apparently virDomainPCIAddressSet has no
concept of PCI domains anyways... So that's an existing
limitation - one that I assume we're going to have to
address, especially now that ppc64 guests are going to
want to use *lots* of separate PCI domains.


On the other hand, at the point where you're going to call
this function haven't PCI addresses already been assigned
and stored in @def? Why not iterate over it twice to make
the comparison future-proof? The computational cost would
be the same AFAICT.

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [libvirt-python][PATCH] examples: Update event-test.py

[Daniel P. Berrange]

On Tue, Jan 10, 2017 at 11:25:20AM +0100, Michal Privoznik wrote:
> With recent changes there are new events known to libvirt.
> Reflect those changes in our event-test.py example script.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  examples/event-test.py | 24 
>  1 file changed, 24 insertions(+)

ACK


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

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

Re: [libvirt] [PATCH python 1/2] Add support for secret event APIs

[Daniel P. Berrange]

On Tue, Jan 10, 2017 at 11:25:27AM +0100, Michal Privoznik wrote:
> On 01/09/2017 07:10 PM, Daniel P. Berrange wrote:
> > Signed-off-by: Daniel P. Berrange 
> > ---
> >  generator.py   |   2 +
> >  libvirt-override-virConnect.py |  43 +
> >  libvirt-override.c | 203 
> > +
> >  sanitytest.py  |   3 +
> >  4 files changed, 251 insertions(+)
> > 
> > diff --git a/generator.py b/generator.py
> > index afb1d34..e9be8b1 100755
> > --- a/generator.py
> > +++ b/generator.py
> > @@ -528,6 +528,8 @@ skip_function = (
> >  'virConnectStoragePoolEventDeregisterAny', # overridden in 
> > virConnect.py
> >  'virConnectNodeDeviceEventRegisterAny',   # overridden in virConnect.py
> >  'virConnectNodeDeviceEventDeregisterAny', # overridden in virConnect.py
> > +'virConnectSecretEventRegisterAny',   # overridden in virConnect.py
> > +'virConnectSecretEventDeregisterAny', # overridden in virConnect.py
> >  'virSaveLastError', # We have our own python error wrapper
> >  'virFreeError', # Only needed if we use virSaveLastError
> >  'virConnectListAllDomains', # overridden in virConnect.py
> > diff --git a/libvirt-override-virConnect.py b/libvirt-override-virConnect.py
> > index fb3d476..d26b480 100644
> > --- a/libvirt-override-virConnect.py
> > +++ b/libvirt-override-virConnect.py
> > @@ -392,6 +392,49 @@
> >  self.nodeDeviceEventCallbackID[ret] = opaque
> >  return ret
> >  
> > +def _dispatchSecretEventLifecycleCallback(self, net, event, detail, 
> > cbData):
> > +"""Dispatches events to python user secret lifecycle event 
> > callbacks
> > +"""
> > +cb = cbData["cb"]
> > +opaque = cbData["opaque"]
> > +
> > +cb(self, virSecret(self, _obj=net), event, detail, opaque)
> > +return 0
> > +
> > +def _dispatchSecretEventGEnericCallback(self, net, cbData):
> 
> s/GE/Ge/
> 
> And also probably just a leftover from copy-paste from network code, but
> s/net/secret/g

I'd already pushed, so added these fixes in a followup patch

> 
> > +"""Dispatches events to python user secret generic event callbacks
> > +"""
> > +cb = cbData["cb"]
> > +opaque = cbData["opaque"]
> > +
> > +cb(self, virSecret(self, _obj=net), opaque)
> > +return 0
> > +
> 
> Michal

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

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

Re: [libvirt] [PATCH python 1/2] Add support for secret event APIs

[Michal Privoznik]

On 01/09/2017 07:10 PM, Daniel P. Berrange wrote:
> Signed-off-by: Daniel P. Berrange 
> ---
>  generator.py   |   2 +
>  libvirt-override-virConnect.py |  43 +
>  libvirt-override.c | 203 
> +
>  sanitytest.py  |   3 +
>  4 files changed, 251 insertions(+)
> 
> diff --git a/generator.py b/generator.py
> index afb1d34..e9be8b1 100755
> --- a/generator.py
> +++ b/generator.py
> @@ -528,6 +528,8 @@ skip_function = (
>  'virConnectStoragePoolEventDeregisterAny', # overridden in virConnect.py
>  'virConnectNodeDeviceEventRegisterAny',   # overridden in virConnect.py
>  'virConnectNodeDeviceEventDeregisterAny', # overridden in virConnect.py
> +'virConnectSecretEventRegisterAny',   # overridden in virConnect.py
> +'virConnectSecretEventDeregisterAny', # overridden in virConnect.py
>  'virSaveLastError', # We have our own python error wrapper
>  'virFreeError', # Only needed if we use virSaveLastError
>  'virConnectListAllDomains', # overridden in virConnect.py
> diff --git a/libvirt-override-virConnect.py b/libvirt-override-virConnect.py
> index fb3d476..d26b480 100644
> --- a/libvirt-override-virConnect.py
> +++ b/libvirt-override-virConnect.py
> @@ -392,6 +392,49 @@
>  self.nodeDeviceEventCallbackID[ret] = opaque
>  return ret
>  
> +def _dispatchSecretEventLifecycleCallback(self, net, event, detail, 
> cbData):
> +"""Dispatches events to python user secret lifecycle event callbacks
> +"""
> +cb = cbData["cb"]
> +opaque = cbData["opaque"]
> +
> +cb(self, virSecret(self, _obj=net), event, detail, opaque)
> +return 0
> +
> +def _dispatchSecretEventGEnericCallback(self, net, cbData):

s/GE/Ge/

And also probably just a leftover from copy-paste from network code, but
s/net/secret/g

> +"""Dispatches events to python user secret generic event callbacks
> +"""
> +cb = cbData["cb"]
> +opaque = cbData["opaque"]
> +
> +cb(self, virSecret(self, _obj=net), opaque)
> +return 0
> +

Michal

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

[libvirt] [libvirt-python][PATCH] examples: Update event-test.py

[Michal Privoznik]

With recent changes there are new events known to libvirt.
Reflect those changes in our event-test.py example script.

Signed-off-by: Michal Privoznik 
---
 examples/event-test.py | 24 
 1 file changed, 24 insertions(+)

diff --git a/examples/event-test.py b/examples/event-test.py
index e43a2f8..d5af33c 100755
--- a/examples/event-test.py
+++ b/examples/event-test.py
@@ -538,6 +538,9 @@ def myDomainEventJobCompletedCallback(conn, dom, params, 
opaque):
 def myDomainEventDeviceRemovalFailedCallback(conn, dom, dev, opaque):
 print("myDomainEventDeviceRemovalFailedCallback: Domain %s(%s) failed to 
remove device: %s" % (
 dom.name(), dom.ID(), dev))
+def myDomainEventMetadataChangeCallback(conn, dom, mtype, nsuri, opaque):
+print("myDomainEventMetadataChangeCallback: Domain %s(%s) changed metadata 
mtype=%d nsuri=%s" % (
+dom.name(), dom.ID(), mtype, nsuri))
 
 ##
 # Network events
@@ -600,6 +603,23 @@ def myNodeDeviceEventLifecycleCallback(conn, dev, event, 
detail, opaque):
 def myNodeDeviceEventUpdateCallback(conn, dev, opaque):
 print("myNodeDeviceEventUpdateCallback: Node device %s" % dev.name())
 
+##
+# Secret events
+##
+def secretEventToString(event):
+secretEventStrings = ( "Defined",
+   "Undefined",
+)
+return secretEventStrings[event]
+
+def mySecretEventLifecycleCallback(conn, secret, event, detail, opaque):
+print("mySecretEventLifecycleCallback: Secret %s %s %d" % 
(secret.UUIDString(),
+   
secretEventToString(event),
+   detail))
+
+def mySecretEventValueChanged(conn, secret, opaque):
+print("mySecretEventValueChanged: Secret %s" % secret.UUIDString())
+
 ##
 # Set up and run the program
 ##
@@ -689,6 +709,7 @@ def main():
 vc.domainEventRegisterAny(None, 
libvirt.VIR_DOMAIN_EVENT_ID_MIGRATION_ITERATION, 
myDomainEventMigrationIteration, None)
 vc.domainEventRegisterAny(None, libvirt.VIR_DOMAIN_EVENT_ID_JOB_COMPLETED, 
myDomainEventJobCompletedCallback, None)
 vc.domainEventRegisterAny(None, 
libvirt.VIR_DOMAIN_EVENT_ID_DEVICE_REMOVAL_FAILED, 
myDomainEventDeviceRemovalFailedCallback, None)
+vc.domainEventRegisterAny(None, 
libvirt.VIR_DOMAIN_EVENT_ID_METADATA_CHANGE, 
myDomainEventMetadataChangeCallback, None)
 
 vc.networkEventRegisterAny(None, libvirt.VIR_NETWORK_EVENT_ID_LIFECYCLE, 
myNetworkEventLifecycleCallback, None)
 
@@ -698,6 +719,9 @@ def main():
 vc.nodeDeviceEventRegisterAny(None, 
libvirt.VIR_NODE_DEVICE_EVENT_ID_LIFECYCLE, myNodeDeviceEventLifecycleCallback, 
None)
 vc.nodeDeviceEventRegisterAny(None, 
libvirt.VIR_NODE_DEVICE_EVENT_ID_UPDATE, myNodeDeviceEventUpdateCallback, None)
 
+vc.secretEventRegisterAny(None, libvirt.VIR_SECRET_EVENT_ID_LIFECYCLE, 
mySecretEventLifecycleCallback, None)
+vc.secretEventRegisterAny(None, libvirt.VIR_SECRET_EVENT_ID_VALUE_CHANGED, 
mySecretEventValueChanged, None)
+
 vc.setKeepAlive(5, 3)
 
 # The rest of your app would go here normally, but for sake
-- 
2.11.0

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

Re: [libvirt] [libvirt-perl PATCH] Add perf cache_l1d event

[Daniel P. Berrange]

On Mon, Jan 09, 2017 at 06:47:04PM -0500, John Ferlan wrote:
> Signed-off-by: John Ferlan 
> ---
>  Daniel - feel free to add to the pile of changes you posted if it's easier.
> 
>  Changes| 1 +
>  Virt.xs| 1 +
>  lib/Sys/Virt/Domain.pm | 6 ++
>  3 files changed, 8 insertions(+)

ACK, I rebased onto my changes & pushed, to fix the build.


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

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

Re: [libvirt] RFC: Use __attribute__ ((cleanup) in libvirt ?

[Daniel P. Berrange]

On Tue, Jan 10, 2017 at 10:48:47AM +0100, Martin Kletzander wrote:
> On Tue, Jan 10, 2017 at 02:58:21AM -0500, Laine Stump wrote:
> > On 01/09/2017 08:09 PM, Richard W.M. Jones wrote:
> > > On Mon, Jan 09, 2017 at 04:58:49PM +, Daniel P. Berrange wrote:
> > > > For those who don't already know, GCC and CLang both implement a C 
> > > > language
> > > > extension that enables automatic free'ing of resources when variables go
> > > > out of scope. This is done by annotating the variable with the "cleanup"
> > > > attribute, pointing to a function the compiler will wire up a call to 
> > > > when
> > > > unwinding the stack. Since the annotation points to an arbitrary user
> > > > defined function, you're not limited to simple free() like semantics. 
> > > > The
> > > > cleanup function could unlock a mutex, or decrement a reference count, 
> > > > etc
> > > > 
> > > > This annotation is used extensively by systemd, and libguestfs, amongst
> > > > other projects. This obviously doesn't bring full garbage collection to
> > > > C, but it does enable the code to be simplified. By removing the need to
> > > > put in many free() (or equiv) calls to cleanup state, the "interesting"
> > > > logic in the code stands out more, not being obscured by cleanup calls
> > > > and goto jumps.
> > > > 
> > > > I'm wondering what people think of making use of this in libvirt ?
> > > > 
> > > > To my mind the only real reason to *not* use it, would be to maintain
> > > > code portability to non-GCC/non-CLang compilers. OS-X, *BSD and *Linux
> > > > all use GCC or CLang or both, so its a non-issue there. So the only 
> > > > place
> > > > this could cause pain is people building libvirt on Win32, who are using
> > > > the Microsoft compilers instead og GCC.
> > > > 
> 
> Only reason I see for not using it is the "temporary" mess it will
> cause.  Yes, we can change to that incrementally, but it will take some
> time and effort and it will never be all of the code that uses it.
> Don't get me wrong, I would love using more builtin compiler features
> and shortening the code here and there.  I'm just worried this
> particular one might be more disrupting than useful.  Most of us are
> pretty used to the code flow we already have and there's nothing you
> can't achieve without the cleanup attribute.
> 
> And yes, I used quotation marks around the word temporary intentionally.

Yes, that's why I thought of it as something that would make for a GSoc
project - have someone do a full conversion of particular areas of code.
eg convert all of util/ or convert the domain XML parser, etc. Basically
if we did it, I think we'd want to have entire files converted at once.
Only converting individual methods ad-hoc would be quite messy.

> > > > IMHO, it is perfectly valid for us to declare that MSVC is unsupported
> > > > with Libvirt and users must use GCC to build on Windows, either natively
> > > > via cygwin, or cross-build from Linux hosts.
> 
> I would love to know if anyone actually tried doing that lately.  Given
> how often we are broken with mingw and we only foind out thanks to our
> test suite (and sometomes the fixing takes more than a release cycle), I
> think nobody does that and from what I know, it might not even work.

We have mingw in the CI system for a while now and its generally fixed
as quickly as native arch builds are fixed these days.


> > > (2) You must not write code like:
> > > 
> > >fn ()
> > >{
> > >  CLEANUP_FREE char *v; // uninitialized
> > > 
> > >  if (some error condition) {
> > >return -1;
> > >  }
> > >  ...
> > >}
> > > 
> > > because that will call free (v) on the uninitialized variable.
> > > Sometimes GCC can spot this.  In libguestfs we tend to initialize
> > > every CLEANUP_* variable to either an explicit value or NULL.  GCC
> > > optimizes away calls to free (NULL).
> > 
> 
> I'm trying to initialize all variables, always, so I don't see this as a
> problem, but there are some of us that (I have the feeling) are trying
> to initialize as few as possible, so this (although it's a different
> story) might still be a problem for someone.

We pretty much have the same problem already with 'goto cleanup' - you
have to make sure everything is initialized sanely before the first
"goto cleanup". So I think we're safe in this respect already and
the cleanup attributes wouldn't make it any more complex.

> > You've covered one of the worries that I had about it (accidentally
> > marking for CLEANUP a pointer whose value gets returned, and the fact
> > that you can't use it for the cleanup of objects that would have
> > normally been returned, in the case that the function encounters an
> > error and has to dump everything). And since the nice cleanup isn't
> > happening for *everything*, people will have to be paying attention to
> > which objects are auto-cleaned up and which aren't, which will
> > inevitably lead to incorrect classification and/or accidentally addi

Re: [libvirt] RFC: Use __attribute__ ((cleanup) in libvirt ?

[Martin Kletzander]

On Tue, Jan 10, 2017 at 02:58:21AM -0500, Laine Stump wrote:

On 01/09/2017 08:09 PM, Richard W.M. Jones wrote:

On Mon, Jan 09, 2017 at 04:58:49PM +, Daniel P. Berrange wrote:

For those who don't already know, GCC and CLang both implement a C language
extension that enables automatic free'ing of resources when variables go
out of scope. This is done by annotating the variable with the "cleanup"
attribute, pointing to a function the compiler will wire up a call to when
unwinding the stack. Since the annotation points to an arbitrary user
defined function, you're not limited to simple free() like semantics. The
cleanup function could unlock a mutex, or decrement a reference count, etc

This annotation is used extensively by systemd, and libguestfs, amongst
other projects. This obviously doesn't bring full garbage collection to
C, but it does enable the code to be simplified. By removing the need to
put in many free() (or equiv) calls to cleanup state, the "interesting"
logic in the code stands out more, not being obscured by cleanup calls
and goto jumps.

I'm wondering what people think of making use of this in libvirt ?

To my mind the only real reason to *not* use it, would be to maintain
code portability to non-GCC/non-CLang compilers. OS-X, *BSD and *Linux
all use GCC or CLang or both, so its a non-issue there. So the only place
this could cause pain is people building libvirt on Win32, who are using
the Microsoft compilers instead og GCC.



Only reason I see for not using it is the "temporary" mess it will
cause.  Yes, we can change to that incrementally, but it will take some
time and effort and it will never be all of the code that uses it.
Don't get me wrong, I would love using more builtin compiler features
and shortening the code here and there.  I'm just worried this
particular one might be more disrupting than useful.  Most of us are
pretty used to the code flow we already have and there's nothing you
can't achieve without the cleanup attribute.

And yes, I used quotation marks around the word temporary intentionally.


IMHO, it is perfectly valid for us to declare that MSVC is unsupported
with Libvirt and users must use GCC to build on Windows, either natively
via cygwin, or cross-build from Linux hosts.


I would love to know if anyone actually tried doing that lately.  Given
how often we are broken with mingw and we only foind out thanks to our
test suite (and sometomes the fixing takes more than a release cycle), I
think nobody does that and from what I know, it might not even work.


From the libguestfs POV it's absolutely been a great thing.  Of course
we also don't care about MSVC, but that is a possible concern for
libvirt as you say.  I wonder if MSVC offers some non-standard support
for this?  As it's really a C++ compiler, maybe it's possible to use
that?

There are many many examples of how this simplifies code in
libguestfs.  Even very complex functions can often be written with no
"exit path" at all.  I'll just point to a few:

https://github.com/libguestfs/libguestfs/blob/master/src/inspect-fs.c#L343-L368
https://github.com/libguestfs/libguestfs/blob/master/src/launch-libvirt.c#L724-L815
https://github.com/libguestfs/libguestfs/blob/master/src/appliance-kcmdline.c#L98
https://github.com/libguestfs/libguestfs/blob/master/src/filearch.c#L131-L179

BTW you can use it for a lot more than just free().  We also have
macros for deleting temporary files and cleaning up complex objects.

Now it's also worth saying there are some catches:

(1) You must not CLEANUP_FREE any objects which you will return from
the function.  This often means you still need to explicitly free such
objects on error return paths.



You can go around that like this:

func()
{
 char *ret = NULL;
 CLEANUP_FREE char *tmp = NULL;
 ...
 if (error)
return ret; // or NULL rather
 ...
 ret = tmp;
 tmp = NULL;
 return ret;
}


(2) You must not write code like:

   fn ()
   {
 CLEANUP_FREE char *v; // uninitialized

 if (some error condition) {
   return -1;
 }
 ...
   }

because that will call free (v) on the uninitialized variable.
Sometimes GCC can spot this.  In libguestfs we tend to initialize
every CLEANUP_* variable to either an explicit value or NULL.  GCC
optimizes away calls to free (NULL).




I'm trying to initialize all variables, always, so I don't see this as a
problem, but there are some of us that (I have the feeling) are trying
to initialize as few as possible, so this (although it's a different
story) might still be a problem for someone.


You've covered one of the worries that I had about it (accidentally
marking for CLEANUP a pointer whose value gets returned, and the fact
that you can't use it for the cleanup of objects that would have
normally been returned, in the case that the function encounters an
error and has to dump everything). And since the nice cleanup isn't
happening for *everything*, people will have to be paying attention to
which objects are auto-cleaned 

Re: [libvirt] [PATCH] Extend l3 cache to nodeinfo

[Daniel P. Berrange]

On Tue, Jan 10, 2017 at 04:11:03PM +0800, Eli Qiao wrote:
> This patch extends l3 cache infomation to nodeinfo output.
> 
> Signed-off-by: Eli Qiao 
> ---
>  include/libvirt/libvirt-host.h |  1 +
>  src/nodeinfo.c |  3 ++-
>  src/remote/remote_protocol.x   |  1 +
>  src/test/test_driver.c |  1 +
>  src/util/virhostcpu.c  | 29 +
>  src/util/virhostcpu.h  |  3 ++-
>  src/util/virhostcpupriv.h  |  3 ++-
>  tests/virhostcputest.c |  3 ++-
>  tools/virsh-host.c |  1 +
>  9 files changed, 37 insertions(+), 8 deletions(-)
> 
> diff --git a/include/libvirt/libvirt-host.h b/include/libvirt/libvirt-host.h
> index 07b5d15..ba926df 100644
> --- a/include/libvirt/libvirt-host.h
> +++ b/include/libvirt/libvirt-host.h
> @@ -167,6 +167,7 @@ struct _virNodeInfo {
>   processors in case of unusual NUMA topology*/
>  unsigned int threads; /* number of threads per core, 1 in case of
>   unusual numa topology */
> +unsigned int l3_cache; /* l3 cache in kilobytes */
>  };

NACK, it is *forbidden* to change public structs as this breaks
ABI compatibility.


> diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x
> index b846ef2..6a16b4e 100644
> --- a/src/remote/remote_protocol.x
> +++ b/src/remote/remote_protocol.x
> @@ -489,6 +489,7 @@ struct remote_node_get_info_ret { /* insert@1 */
>  int sockets;
>  int cores;
>  int threads;
> +int l3_cache;
>  };

Likewise this breaks RPC compatibility.


This info wil need to be reported in the capabilities XML instead.

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

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

Re: [libvirt] [PATCH python 0/2] Add support for new events

[Michal Privoznik]

On 01/09/2017 07:10 PM, Daniel P. Berrange wrote:
> Daniel P. Berrange (2):
>   Add support for secret event APIs
>   Add support for domain metadata change event
> 
>  generator.py   |   2 +
>  libvirt-override-virConnect.py |  52 +
>  libvirt-override.c | 260 
> +
>  sanitytest.py  |   3 +
>  4 files changed, 317 insertions(+)
> 

Not a show stopper, but it would be nice to adjust
examples/event-test.py too. But that can be saved to a follow up patch.

ACK to both patches.

Michal

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

Re: [libvirt] Issue with libvirtd

[Michal Privoznik]

On 01/10/2017 07:48 AM, Faysal Ali wrote:
> Hi Michal,

[It is usually good idea to keep the list CCed - it may help others
finding a solution to their problems]

> 
> Well I have created my little python/libivrt app to manage my virtual
> machines. I am using python socket to check libivrt port availability, and
> the error End of file while reading data: Input/output error* only happens
> whenever that python socket script is trigger.
> 
> Here is the script of python socket
> 
> import libvirt, socket, sys
> 
> hostname='kvm09'
> port = 16509
> 
> try:
>   socket_host = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
>   socket_host.settimeout(1)
>   socket_host.connect((hostname, port))
>   socket_host.close()
>   print 'true'
> except Exception as err:
>   print err

Ah, I haven't expected this. Of course your are seeing the error
message. Libvirt has its own protocol on the top of TCP and since you
are connecting and dying immediately - without sending any valid libvirt
packet, the daemon logs an error. This is perfectly expected.

Also, this is *not* how you connect to libvirt. You want to open a
libvirt connection:

conn = libvirt.open(uri)
dom = conn.lookupByName(name)
...

Michal

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

[libvirt] [PATCH v4] libxl: define a per-domain logger.

[Cédric Bosdonnat]

libxl doesn't provide a way to write one log for each domain. Thus
we need to demux the messages. If our logger doesn't know to which
domain to attribute a message, then it will write it to the default
log file.

Starting with Xen 4.9 (commit f9858025 and following), libxl will
write the domain ID in an easy to grab manner. The logger introduced
by this commit will use it to demux the libxl log messages.

Thanks to the default log file, this logger will also work with older
versions of Xen.
---
 * v4: delay the log file opening to the first message write.

 * v3: add JSON-formatted libxl_domain_config to the newly opened
   file when creating a guest.

 * v2: addressed Jim's comments

 src/Makefile.am  |   1 +
 src/libxl/libxl_conf.c   |  38 +--
 src/libxl/libxl_conf.h   |   4 +-
 src/libxl/libxl_domain.c |   7 ++
 src/libxl/libxl_driver.c |   2 +
 src/libxl/libxl_logger.c | 257 +++
 src/libxl/libxl_logger.h |  40 
 7 files changed, 312 insertions(+), 37 deletions(-)
 create mode 100644 src/libxl/libxl_logger.c
 create mode 100644 src/libxl/libxl_logger.h

diff --git a/src/Makefile.am b/src/Makefile.am
index 0dd2faaf6..58e3108a6 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -851,6 +851,7 @@ LIBXL_DRIVER_SOURCES =  
\
libxl/libxl_capabilities.c libxl/libxl_capabilities.h   \
libxl/libxl_domain.c libxl/libxl_domain.h   \
libxl/libxl_driver.c libxl/libxl_driver.h   \
+   libxl/libxl_logger.c libxl/libxl_logger.h   \
libxl/libxl_migration.c libxl/libxl_migration.h
 
 UML_DRIVER_SOURCES =   \
diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index b569ddad8..ac83b51c7 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -76,9 +76,7 @@ libxlDriverConfigDispose(void *obj)
 
 virObjectUnref(cfg->caps);
 libxl_ctx_free(cfg->ctx);
-xtl_logger_destroy(cfg->logger);
-if (cfg->logger_file)
-VIR_FORCE_FCLOSE(cfg->logger_file);
+libxlLoggerFree(cfg->logger);
 
 VIR_FREE(cfg->configDir);
 VIR_FREE(cfg->autostartDir);
@@ -1356,8 +1354,6 @@ libxlDriverConfigPtr
 libxlDriverConfigNew(void)
 {
 libxlDriverConfigPtr cfg;
-char *log_file = NULL;
-xentoollog_level log_level = XTL_DEBUG;
 char ebuf[1024];
 unsigned int free_mem;
 
@@ -1386,9 +1382,6 @@ libxlDriverConfigNew(void)
 if (VIR_STRDUP(cfg->channelDir, LIBXL_CHANNEL_DIR) < 0)
 goto error;
 
-if (virAsprintf(&log_file, "%s/libxl-driver.log", cfg->logDir) < 0)
-goto error;
-
 if (virFileMakePath(cfg->logDir) < 0) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
_("failed to create log dir '%s': %s"),
@@ -1397,37 +1390,13 @@ libxlDriverConfigNew(void)
 goto error;
 }
 
-if ((cfg->logger_file = fopen(log_file, "a")) == NULL)  {
-VIR_ERROR(_("Failed to create log file '%s': %s"),
-  log_file, virStrerror(errno, ebuf, sizeof(ebuf)));
-goto error;
-}
-VIR_FREE(log_file);
-
-switch (virLogGetDefaultPriority()) {
-case VIR_LOG_DEBUG:
-log_level = XTL_DEBUG;
-break;
-case VIR_LOG_INFO:
-log_level = XTL_INFO;
-break;
-case VIR_LOG_WARN:
-log_level = XTL_WARN;
-break;
-case VIR_LOG_ERROR:
-log_level = XTL_ERROR;
-break;
-}
-
-cfg->logger =
-(xentoollog_logger *)xtl_createlogger_stdiostream(cfg->logger_file,
-  log_level, XTL_STDIOSTREAM_SHOW_DATE);
+cfg->logger = libxlLoggerNew(cfg->logDir, virLogGetDefaultPriority());
 if (!cfg->logger) {
 VIR_ERROR(_("cannot create logger for libxenlight, disabling driver"));
 goto error;
 }
 
-if (libxl_ctx_alloc(&cfg->ctx, LIBXL_VERSION, 0, cfg->logger)) {
+if (libxl_ctx_alloc(&cfg->ctx, LIBXL_VERSION, 0, (xentoollog_logger 
*)cfg->logger)) {
 VIR_ERROR(_("cannot initialize libxenlight context, probably not "
 "running in a Xen Dom0, disabling driver"));
 goto error;
@@ -1478,7 +1447,6 @@ libxlDriverConfigNew(void)
 return cfg;
 
  error:
-VIR_FREE(log_file);
 virObjectUnref(cfg);
 return NULL;
 }
diff --git a/src/libxl/libxl_conf.h b/src/libxl/libxl_conf.h
index 851f3afb4..69d78851a 100644
--- a/src/libxl/libxl_conf.h
+++ b/src/libxl/libxl_conf.h
@@ -41,6 +41,7 @@
 # include "locking/lock_manager.h"
 # include "virfirmware.h"
 # include "libxl_capabilities.h"
+# include "libxl_logger.h"
 
 # define LIBXL_DRIVER_NAME "xenlight"
 # define LIBXL_VNC_PORT_MIN  5900
@@ -74,8 +75,7 @@ struct _libxlDriverConfig {
 unsigned int version;
 
 /* log stream for driver-wide libxl ctx */
-FILE *logger_file;
-xentoollog_logger *logger;
+libxlLoggerPtr logger;
  

Re: [libvirt] [PATCH v3 2/3] docs: Document virtio-mmio by default for mach-virt guests

[Laine Stump]

Don't you want the subject to say "Document virtio-pci by default"?

ACK.

On 12/23/2016 01:54 PM, Andrea Bolognani wrote:

---
  docs/formatdomain.html.in | 8 +++-
  1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index f7bef51..01dd2b2 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -3255,7 +3255,13 @@
  currently only available for some armv7l and
  aarch64 virtual machines. virtio-mmio addresses
  do not have any additional attributes.
-Since 1.1.3
+Since 1.1.3
+If the guest architecture is aarch64 and the machine
+type is virt, libvirt will automatically assign PCI
+addresses to devices; however, the presence of a single device
+with virtio-mmio address in the guest configuration will cause
+libvirt to assign virtio-mmio addresses to all further devices.
+Since 3.0.0

isa
ISA addresses have the following additional



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

Re: [libvirt] [PATCH v3 3/3] NEWS: Update for virtio-pci by default for mach-virt guests

[Laine Stump]

On 12/23/2016 01:54 PM, Andrea Bolognani wrote:

---
  docs/news.html.in | 6 ++
  1 file changed, 6 insertions(+)

diff --git a/docs/news.html.in b/docs/news.html.in
index 36c7d3d..e5b9513 100644
--- a/docs/news.html.in
+++ b/docs/news.html.in
@@ -53,6 +53,12 @@
Add a display of the  size of a disk
volume in the output of the volume XML

+  qemu: Use virtio-pci by default for aarch64 mach-virt guests
+  virtio-pci provides several advantages over virtio-mmio, such
+  as the ability to hotplug devices and improved performance.
+  While opting in to virtio-pci has been possible for a while,
+  newly-defined guests will now use it automatically
+  
  

Bug fixes



ACK

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

Re: [libvirt] [PATCH v3 1/3] qemu: Use virtio-pci by default for mach-virt guests

[Laine Stump]

On 12/23/2016 01:54 PM, Andrea Bolognani wrote:

virtio-pci is the way forward for aarch64 guests: it's faster
and less alien to people coming from other architectures.
Now that guest support is finally getting there (Fedora 24,
CentOS 7.3, Ubuntu 16.04 and Debian testing all support
virtio-pci out of the box), we'd like to start using it by
default instead of virtio-mmio.

Users and applications can already opt-in by explicitly using

   

inside the relevant elements, but that's kind of cumbersome and
requires all users and management applications to adapt, which
we'd really like to avoid.

What we can do instead is use virtio-mmio only if the guest
already has at least one virtio-mmio device, and use virtio-pci
in all other situations.

That means existing virtio-mmio guests will keep using the old
addressing scheme, and new guests will automatically be created
using virtio-pci instead. Users can still override the default
in either direction.

Existing tests such as aarch64-aavmf-virtio-mmio and
aarch64-virtio-pci-default already cover all possible
scenarios, so no additions to the test suites are necessary.
---
  src/qemu/qemu_domain_address.c | 51 --
  ...l2argv-aarch64-virt-2.6-virtio-pci-default.args | 14 +++---
  .../qemuxml2argv-aarch64-virtio-pci-default.args   | 17 +---
  .../qemuxml2argv-aarch64-virtio-pci-default.xml|  3 --
  tests/qemuxml2argvtest.c   |  1 +
  .../qemuxml2xmlout-aarch64-virtio-pci-default.xml  | 40 ++---
  tests/qemuxml2xmltest.c|  1 +
  7 files changed, 106 insertions(+), 21 deletions(-)

diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
index be4ed23..d2f7953 100644
--- a/src/qemu/qemu_domain_address.c
+++ b/src/qemu/qemu_domain_address.c
@@ -398,6 +398,44 @@ qemuDomainAssignS390Addresses(virDomainDefPtr def,
  }
  
  
+static int

+qemuDomainHasVirtioMMIODevicesCallback(virDomainDefPtr def ATTRIBUTE_UNUSED,
+   virDomainDeviceDefPtr dev 
ATTRIBUTE_UNUSED,
+   virDomainDeviceInfoPtr info,
+   void *opaque)
+{
+if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO) {
+/* We can stop iterating as soon as we find the first
+ * virtio-mmio device */
+*((bool *) opaque) = true;
+return -1;
+}
+
+return 0;
+}
+
+
+/**
+ * qemuDomainHasVirtioMMIODevices:
+ * @def: domain definition
+ *
+ * Scan @def looking for devices with a virtio-mmio address.
+ *
+ * Returns: true if there are any, false otherwise
+ */
+static bool
+qemuDomainHasVirtioMMIODevices(virDomainDefPtr def)
+{
+bool result = false;
+
+virDomainDeviceInfoIterate(def,
+   qemuDomainHasVirtioMMIODevicesCallback,
+   &result);
+
+return result;
+}
+
+
  static void
  qemuDomainAssignARMVirtioMMIOAddresses(virDomainDefPtr def,
 virQEMUCapsPtr qemuCaps)
@@ -410,9 +448,16 @@ qemuDomainAssignARMVirtioMMIOAddresses(virDomainDefPtr def,
qemuDomainMachineIsVirt(def)))
  return;
  
-if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIRTIO_MMIO)) {

-qemuDomainPrimeVirtioDeviceAddresses(
-def, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO);
+/* We use virtio-mmio by default on mach-virt guests only if they already
+ * have at least one virtio-mmio device: in all other cases, we prefer
+ * virtio-pci */
+if (qemuDomainMachineHasPCIeRoot(def) &&
+!qemuDomainHasVirtioMMIODevices(def)) {
+qemuDomainPrimeVirtioDeviceAddresses(def,
+ 
VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI);



I'm having trouble remembering, and it's too early in the morning to go 
looking in the code - it seems like priming the addresses with PCI might 
not be necessary - as long as we simply *don't* prime them with 
virtio-mmio. I could be misremembering though. ACK either way.


(ACK is based on the assumption that all concerned ARM parties have 
agreed that it's time to do this. I *think* that is the case :-)




+} else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIRTIO_MMIO)) {
+qemuDomainPrimeVirtioDeviceAddresses(def,
+ 
VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO);
  }
  }
  
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-aarch64-virt-2.6-virtio-pci-default.args b/tests/qemuxml2argvdata/qemuxml2argv-aarch64-virt-2.6-virtio-pci-default.args

index 75db1a4..df03c6e 100644
--- 
a/tests/qemuxml2argvdata/qemuxml2argv-aarch64-virt-2.6-virtio-pci-default.args
+++ 
b/tests/qemuxml2argvdata/qemuxml2argv-aarch64-virt-2.6-virtio-pci-default.args
@@ -21,14 +21,18 @@ QEMU_AUDIO_DRV=none \
  -initrd /aarch64.initrd \
  -append 'earlyprintk console=ttyAMA0,115200n8 rw root=/dev/vda rootwait' \
  -dtb /aar

[libvirt] [PATCH 2/2] qemu: use virDomainPCIAddressSetAllMulti() to set multi when needed

[Laine Stump]

If there are multiple devices assigned to the different functions of a
single PCI slot, they will not work properly if the device at function
0 doesn't have its "multi" attribute turned on, so it makes sense for
libvirt to turn it on during PCI address assignment. Just in case a
future change in the configuration eliminates the devices on all non-0
functions, and in case some guest OS would be confused by having the
multi flag of function 0 modified to "no" at the next boot of the
guest (it may seem like an inconsequential guest ABI change, but it
*is* a guest ABI change).

We are set multi during PCI address assignment (which happens in the
domain post-parse callback) rather than at qemu runtime so that the
setting will be stored in config and persist even in the case that the
devices in the non-0 functions of the slot are later removed from the
config - this is done to prevent a "silent" guest ABI change (although
it can only happen after shutting down and restarting the guest, and
although it may seem like an inconsequential guest ABI change, it *is*
a guest ABI change to turn off the multi bit).
---
 src/qemu/qemu_domain_address.c |   5 +
 .../qemuxml2argv-q35-multifunction.args|  42 
 .../qemuxml2argv-q35-multifunction.xml |  51 +
 tests/qemuxml2argvtest.c   |  23 
 .../qemuxml2xmlout-q35-multifunction.xml   | 120 +
 tests/qemuxml2xmltest.c|  23 
 6 files changed, 264 insertions(+)
 create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-q35-multifunction.args
 create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-q35-multifunction.xml
 create mode 100644 
tests/qemuxml2xmloutdata/qemuxml2xmlout-q35-multifunction.xml

diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
index 4a2a489..d350921 100644
--- a/src/qemu/qemu_domain_address.c
+++ b/src/qemu/qemu_domain_address.c
@@ -2088,6 +2088,11 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def,
 if (qemuDomainAssignDevicePCISlots(def, qemuCaps, addrs) < 0)
 goto cleanup;
 
+/* set multi attribute for devices at function 0 of
+ * any slot that has multiple functions in use
+ */
+virDomainPCIAddressSetAllMulti(def, addrs);
+
 for (i = 0; i < def->ncontrollers; i++) {
 virDomainControllerDefPtr cont = def->controllers[i];
 int idx = cont->idx;
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-q35-multifunction.args 
b/tests/qemuxml2argvdata/qemuxml2argv-q35-multifunction.args
new file mode 100644
index 000..6c4e9b2
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-q35-multifunction.args
@@ -0,0 +1,42 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/home/test \
+USER=test \
+LOGNAME=test \
+QEMU_AUDIO_DRV=none \
+/usr/libexec/qemu-kvm \
+-name q35-test \
+-S \
+-M q35 \
+-m 2048 \
+-smp 2,sockets=2,cores=1,threads=1 \
+-uuid 11dbdcdd-4c3b-482b-8903-9bdb8c0a2774 \
+-nographic \
+-nodefaults \
+-monitor unix:/tmp/lib/domain--1-q35-test/monitor.sock,server,nowait \
+-no-acpi \
+-boot c \
+-device ioh3420,port=0x10,chassis=1,id=pci.1,bus=pcie.0,multifunction=on,\
+addr=0x2 \
+-device ioh3420,port=0x11,chassis=2,id=pci.2,bus=pcie.0,addr=0x2.0x1 \
+-device ioh3420,port=0x12,chassis=3,id=pci.3,bus=pcie.0,addr=0x2.0x2 \
+-device ioh3420,port=0x18,chassis=4,id=pci.4,bus=pcie.0,multifunction=on,\
+addr=0x3 \
+-device ioh3420,port=0x19,chassis=5,id=pci.5,bus=pcie.0,multifunction=on,\
+addr=0x3.0x1 \
+-device ioh3420,port=0x20,chassis=6,id=pci.6,bus=pcie.0,multifunction=off,\
+addr=0x4 \
+-device ioh3420,port=0x21,chassis=7,id=pci.7,bus=pcie.0,addr=0x4.0x1 \
+-device ioh3420,port=0x8,chassis=8,id=pci.8,bus=pcie.0,addr=0x1 \
+-device ioh3420,port=0x28,chassis=9,id=pci.9,bus=pcie.0,addr=0x5 \
+-device ioh3420,port=0x30,chassis=10,id=pci.10,bus=pcie.0,addr=0x6 \
+-device ioh3420,port=0x38,chassis=11,id=pci.11,bus=pcie.0,addr=0x7 \
+-device ioh3420,port=0x40,chassis=12,id=pci.12,bus=pcie.0,addr=0x8 \
+-device ioh3420,port=0x48,chassis=13,id=pci.13,bus=pcie.0,addr=0x9 \
+-device ioh3420,port=0x50,chassis=14,id=pci.14,bus=pcie.0,addr=0xa \
+-device ioh3420,port=0x58,chassis=15,id=pci.15,bus=pcie.0,addr=0xb \
+-device ioh3420,port=0x60,chassis=16,id=pci.16,bus=pcie.0,addr=0xc \
+-device ioh3420,port=0x68,chassis=17,id=pci.17,bus=pcie.0,addr=0xd \
+-device ioh3420,port=0x70,chassis=18,id=pci.18,bus=pcie.0,addr=0xe \
+-device nec-usb-xhci,id=usb,bus=pci.1,addr=0x0 \
+-device virtio-balloon-pci,id=balloon0,bus=pci.2,addr=0x0
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-q35-multifunction.xml 
b/tests/qemuxml2argvdata/qemuxml2argv-q35-multifunction.xml
new file mode 100644
index 000..c1edca1
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-q35-multifunction.xml
@@ -0,0 +1,51 @@
+
+  q35-test
+  11dbdcdd-4c3b-482b-8903-9bdb8c0a2774
+  2097152
+  2097152
+  2
+  
+hvm
+
+  
+  
+  destroy
+  restart
+  destroy
+ 

[libvirt] [PATCH 0/2] replacement for Patches 5/8 & 6/8 of "aggregate multiple pcie-root-ports onto a single slot"

[Laine Stump]

During review of the aforementioned series, Andrea expressed
misgivings about patch 6, which automatically set the multifunction=on
option transiently at the time the qemu commandline was generated. Dan
agreed. These two patches replace patches 5 and 6 of that series with
two patches that will set multi='on' in the domain's config rather
than just temporarily in the commandline. This way it will remain set
even if all the devices on the non-0 function are later removed
(unless it's explicitly changed by the user/management app, that is).

The rest of the series is ACKed and ready to push, which I'd like to
do before the freeze on Wednesday; these two patche are all that's
holding it up.

Laine Stump (2):
  conf: new function virDomainPCIAddressSetAllMulti()
  qemu: use virDomainPCIAddressSetAllMulti() to set multi when needed

 src/conf/domain_addr.c |  75 +
 src/conf/domain_addr.h |   4 +
 src/libvirt_private.syms   |   1 +
 src/qemu/qemu_domain_address.c |   5 +
 .../qemuxml2argv-q35-multifunction.args|  42 
 .../qemuxml2argv-q35-multifunction.xml |  51 +
 tests/qemuxml2argvtest.c   |  23 
 .../qemuxml2xmlout-q35-multifunction.xml   | 120 +
 tests/qemuxml2xmltest.c|  23 
 9 files changed, 344 insertions(+)
 create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-q35-multifunction.args
 create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-q35-multifunction.xml
 create mode 100644 
tests/qemuxml2xmloutdata/qemuxml2xmlout-q35-multifunction.xml

-- 
2.7.4

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

[libvirt] [PATCH 1/2] conf: new function virDomainPCIAddressSetAllMulti()

[Laine Stump]

This utility function looks through a virPCIAddressSet for any slot of
any bus that has multiple functions in use, and sets the "multi" flag
in the virDomainDeviceInfo for the device that is assigned to function
0 of that slot (as long as it hasn't already been set explicitly by
someone who presumably has better information than we do).

It isn't yet called from anywhere, so will have no functional effect.
---
 src/conf/domain_addr.c   | 75 
 src/conf/domain_addr.h   |  4 +++
 src/libvirt_private.syms |  1 +
 3 files changed, 80 insertions(+)

diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c
index 4d14ac4..0364e95 100644
--- a/src/conf/domain_addr.c
+++ b/src/conf/domain_addr.c
@@ -867,6 +867,81 @@ 
virDomainPCIAddressReserveNextSlot(virDomainPCIAddressSetPtr addrs,
 }
 
 
+static int
+virDomainPCIAddressSetMultiIter(virDomainDefPtr def ATTRIBUTE_UNUSED,
+virDomainDeviceDefPtr dev ATTRIBUTE_UNUSED,
+virDomainDeviceInfoPtr info,
+void *data)
+{
+virPCIDeviceAddressPtr testAddr = data;
+virPCIDeviceAddressPtr thisAddr;
+
+if (!info || info->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI)
+   return 0;
+
+thisAddr = &info->addr.pci;
+
+if (thisAddr->domain == testAddr->domain &&
+thisAddr->bus == testAddr->bus &&
+thisAddr->slot == testAddr->slot &&
+thisAddr->function == 0) {
+
+/* only set to ON if it wasn't previously set
+ * (assuming that the user must have better information
+ * than us if they explicitly set it OFF)
+ */
+if (thisAddr->multi == VIR_TRISTATE_SWITCH_ABSENT)
+thisAddr->multi = VIR_TRISTATE_SWITCH_ON;
+
+return -1; /* finish early, *NOT* an error */
+}
+
+return 0;
+}
+
+
+/**
+ * virDomainPCIAddressSetAllMulti():
+ *
+ * @def: the domain definition whose devices may need adjusting
+ * @addrs: address set keeping track of all addresses in use.
+ *
+ * Look for any PCI slots that have multiple functions assigned, and
+ * set multi to YES in the address for the device at function 0
+ * (unless it has been explicitly set to NO).
+ *
+ * No return code, since there is no possibility of failure.
+ */
+void
+virDomainPCIAddressSetAllMulti(virDomainDefPtr def,
+   virDomainPCIAddressSetPtr addrs)
+{
+/* Scan through all the slots in @addrs looking for any that have
+ * more than just function 0 marked as in use, then use an
+ * iterator to find the DeviceInfo that uses function 0 on that
+ * slot and mark it as multi = YES
+ */
+size_t busIdx, slotIdx;
+
+for (busIdx = 0; busIdx < addrs->nbuses; busIdx++) {
+virDomainPCIAddressBusPtr bus = &addrs->buses[busIdx];
+
+for (slotIdx = bus->minSlot; slotIdx <= bus->maxSlot; slotIdx++) {
+if (bus->slot[slotIdx].functions > 1) {
+virPCIDeviceAddress addr = { .domain = 0,
+ .bus = busIdx,
+ .slot = slotIdx,
+ .function = 0 };
+
+ignore_value(virDomainDeviceInfoIterate(def,
+
virDomainPCIAddressSetMultiIter,
+&addr));
+}
+}
+}
+}
+
+
 static char*
 virDomainCCWAddressAsString(virDomainDeviceCCWAddressPtr addr)
 {
diff --git a/src/conf/domain_addr.h b/src/conf/domain_addr.h
index 5f0924e..2263114 100644
--- a/src/conf/domain_addr.h
+++ b/src/conf/domain_addr.h
@@ -173,6 +173,10 @@ int 
virDomainPCIAddressReserveNextSlot(virDomainPCIAddressSetPtr addrs,
virDomainPCIConnectFlags flags)
 ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
 
+void virDomainPCIAddressSetAllMulti(virDomainDefPtr def,
+virDomainPCIAddressSetPtr addrs)
+ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
+
 struct _virDomainCCWAddressSet {
 virHashTablePtr defined;
 virDomainDeviceCCWAddress next;
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index a928e90..a0db583 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -104,6 +104,7 @@ virDomainPCIAddressReserveAddr;
 virDomainPCIAddressReserveNextAddr;
 virDomainPCIAddressReserveNextSlot;
 virDomainPCIAddressReserveSlot;
+virDomainPCIAddressSetAllMulti;
 virDomainPCIAddressSetAlloc;
 virDomainPCIAddressSetFree;
 virDomainPCIAddressSetGrow;
-- 
2.7.4

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