Re: [libvirt] [PATCH 1/2] util: introduce virDirRead wrapper for readdir

2014-04-22 Thread Natanael Copa
On Mon, 21 Apr 2014 16:05:28 -0600
Eric Blake ebl...@redhat.com wrote:

 On 04/20/2014 05:53 AM, Natanael Copa wrote:
...
  +/* return 0 = success, 1 = end-of-dir and -1 = error */
 
 This logic is a bit odd to reason about.  I think 1 = success (returned
 1 entry), 0 = end-of-dir (successfully returned 0 entries) may be a bit
 easier to reason about.
 
 With your construct, the loop looks like:
 
 while (!(value = virReadDir(dir, ent, name))) {
 ...
 }
 if (value  0)
 error
 
 with my proposed return values, the loop looks like:
 
 while ((value = virReadDir(dir, ent, name))  0) {
 ...
 }
 if (value  0)
 error
 
 Mine is slightly more typing, but seems a bit more intuitive.  Anyone
 else with an opinion?

It was to be consistent with the current virDirCreate which returns 0
on success, like many of the posix functions. But its your code, and
you that will have to live with it. I don't have any strong opinion.

  +int virDirRead(DIR *dirp, struct dirent **ent, const char *dirname)
 
 Many of our wrappers are named resembling what they wrap, as in
 virReaddir().  On the other hand, we already have the virDir... prefix
 for other actions, so I can live with this name.

The idea was to group it in the same category as virDirCreate. Might be
you want virDirOpen, virDirClose, virDirRewind in future. And maybe
even move it out from virfile to virdir.

 
  +{
  +errno = 0;
  +*ent = readdir(dirp);
 
 Due to this statement, both ent and dirp must be non-NULL pointers...
 
  @@ -211,6 +212,8 @@ enum {
   };
   int virDirCreate(const char *path, mode_t mode, uid_t uid, gid_t gid,
unsigned int flags) ATTRIBUTE_RETURN_CHECK;
  +int virDirRead(DIR *dirp, struct dirent **ent, const char *dirname);
 
 ...so I'd add:
 
 ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK.

I'm not familiar with those. I'll lave that to you.

 I like how you made error reporting optional - pass a name to report the
 error, pass NULL to suppress it while still getting an error indication.
 
 There's also the point I raised about whether we should make the wrapper
 function automatically skip . and ..; this would best be done by adding
 a flags argument, where the default of 0 returns all entries, but
 passing a non-zero flag can do filtering.  I guess it still remains to
 be seen how many callers would benefit from this.

I'll leave that to you. It now runs on musl libc so I'm done scratching
my itch ;)

 Also, your series
 only touched one file to use the new paradigm; but ideally we'd add a
 syntax check that enforces its use everywhere. 

I was hoping you would take care of it from here. You now know what the
problem is and you know your own code better than I do.

Thanks for the feedback.

-nc


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

Re: [libvirt] libvirt-libxl driver defaulting to tap disk and not working (ubuntu 12.10 + xen 4.5 + libvirt 1.2.3 + openstack)

2014-04-22 Thread Stefan Bader
On 21.04.2014 23:53, Jim Fehlig wrote:
 Tian, Shuangtai wrote:
 Hi, Jim
 The blktap seems not a module in xen 4.5, when I tried the load it , can not 
 find the module, is there something wrong I did?
   
 
 It would be provided by your dom0 kernel, not Xen.  The Ubuntu Xen
 kernel doesn't provide a blktap module?

There is a blktap source package which can be installed separately. Though I
must admit I never tried to use it. There is some code for blktap(2) in the Xen
source but I had to disable that because it would otherwise clash with the
external package.
Tian, you may try to sudo apt-get install blktap-utils which should pull in
the other pieces (libraryies/dkms module).

-Stefan

 
 Regards,
 Jim
 
 BTW ,I compiled the xen4.5 by myself.
 Thanks ! 

 -Original Message-
 From: Jim Fehlig [mailto:jfeh...@suse.com] 
 Sent: Friday, April 18, 2014 11:52 PM
 To: Tian, Shuangtai
 Cc: libvir-list@redhat.com
 Subject: Re: [libvirt] libvirt-libxl driver defaulting to tap disk and not 
 working (ubuntu 12.10 + xen 4.5 + libvirt 1.2.3 + openstack)

 Tian, Shuangtai wrote:
   
 HI

 I am an openstacker, when I used the latest libvirt and xen code to 
 run the openstack. Can not create the vm.

 there is an error in libxl log, you can see the log:

 Os : Ubuntu 12.10

  

 Compiled against library: libvirt 1.2.3

 Using library: libvirt 1.2.3

 Using API: Xen 1.2.3

 Running hypervisor: Xen 4.5.0

  

 libxl: debug: libxl_create.c:1356:do_domain_create: ao 0x7f7894002810:
 create: how=(nil) callback=(nil) poller=0x7f7894001bb0

 libxl: debug: libxl_device.c:251:libxl__device_disk_set_backend: Disk 
 vdev=xvda spec.backend=tap

 libxl: debug: libxl_device.c:210:disk_try_backend: Disk vdev=xvda, 
 backend tap unsuitable because blktap not available

 

 In addition to John's suggestions, try loading a blktap module in dom0. 
 It should work if a blktap driver is available.

 Regards,
 Jim

   
 libxl: error: libxl_device.c:289:libxl__device_disk_set_backend: no 
 suitable backend for disk xvda

 libxl: debug: libxl_event.c:1739:libxl__ao_complete: ao
 0x7f7894002810: complete, rc=-3

 libxl: debug: libxl_create.c:1370:do_domain_create: ao 0x7f7894002810:
 inprogress: poller=0x7f7894001bb0, flags=ic

 libxl: debug: libxl_event.c:1711:libxl__ao__destroy: ao
 0x7f7894002810: destroy

  

 The blktap does work, and I also find the same error someone has 
 posted,
 (http://www.redhat.com/archives/libvir-list/2013-February/msg01124.htm
 l)

 When I change the type to “phy”, it also doesnot work. And try to 
 change the type to other options, also does not work.

 Can someone give me some suggestions?

 Thanks !

  

 The XML from the openstack is :

 domain type=xen

   uuid9e9ed86c-8892-40da-acd1-31ec6303abfe/uuid

   nameinstance-0001/name

   memory524288/memory

   vcpu1/vcpu

   os

 typexen/type


 kernel/opt/stack/data/nova/instances/9e9ed86c-8892-40da-acd1-31ec630
 3abfe/kernel/kernel


 initrd/opt/stack/data/nova/instances/9e9ed86c-8892-40da-acd1-31ec630
 3abfe/ramdisk/initrd

 cmdlinero root=/dev/xvda/cmdline

   /os

   features

acpi/

 apic/

   /features

   clock offset=utc/

   devices

 disk type=file device=disk

   driver name=tap2 type=raw cache=none/

   source
 file=/opt/stack/data/nova/instances/9e9ed86c-8892-40da-acd1-31ec6303a
 bfe/disk/

   target bus=xen dev=xvda/

 /disk

 disk type=file device=cdrom

   driver name=tap2 type=raw cache=none/

   source
 file=/opt/stack/data/nova/instances/9e9ed86c-8892-40da-acd1-31ec6303a
 bfe/disk.config/

   target bus=ide dev=xvdd/

 /disk

 interface type=bridge

   mac address=fa:16:3e:d8:c3:c0/

   source bridge=br100/

   filterref 
 filter=nova-instance-instance-0001-fa163ed8c3c0/

 /interface

 console type=pty/

 graphics type=vnc autoport=yes keymap=en-us
 listen=127.0.0.1/

 video

   model type=xen/

 /video

   /devices

 /domain

  

  

  

 Best regards,

 Tian, Shuangtai

  

 --
 --

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




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

Re: [libvirt] [PATCH] qemu: don't check for backing chains for formats w/o snapshot support

2014-04-22 Thread Martin Kletzander

On Mon, Apr 21, 2014 at 03:22:43PM -0600, Eric Blake wrote:

On 04/17/2014 04:20 AM, Martin Kletzander wrote:

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

Signed-off-by: Martin Kletzander mklet...@redhat.com
---
 src/libvirt_private.syms  |  2 +-
 src/qemu/qemu_domain.c|  3 +++
 src/util/virstoragefile.c | 15 +++
 src/util/virstoragefile.h |  2 ++
 4 files changed, 21 insertions(+), 1 deletion(-)




@@ -1850,7 +1851,6 @@ virStorageSourcePoolModeTypeToString;
 virStorageTypeFromString;
 virStorageTypeToString;

-
 # util/virstring.h


Spurious whitespace change.



+
+bool
+virStorageFormatMaySupportSnapshots(enum virStorageFileFormat format)
+{
+if (format == VIR_STORAGE_FILE_AUTO ||
+format == VIR_STORAGE_FILE_AUTO_SAFE)
+return true;
+
+/* Better safe than sorry */
+if (format = VIR_STORAGE_FILE_NONE ||
+format = VIR_STORAGE_FILE_LAST)
+return false;
+
+return !!fileTypeInfo[format].getBackingStore;


Hmm, how does this compare with the recent commit db7d7c0e which added
the VIR_STORAGE_FILE_BACKING marker?  I made that separation in order to
state that all formats less than the marker do not have a
getBackingStore callback (well, other than the exceptions of the _AUTO
and _AUTO_SAFE formats which are less than 0), and all formats = that
marker DO have a potential for a backing store.  Should this code be
using that new constant instead of probing the existence of
getBackingStore?  Or conversely, should the domain_conf.c code that I
touched in that patch instead be using this new function instead of
relying on the VIR_STORAGE_FILE_BACKING marker?



TBH, I haven't noticed the change, this function just works.  I first
tried to enumerate all the VIR_STORAGE_FILE_* formats, but that looked
terrible and it haven't met the goal I had.

The aim in commit db7d7c0e differs a bit, I guess.  My point was that
we are opening files just to close them in a while, because we have no
getBackingStore() function (the bug is opened about /dev/sr0 without
media in the drive, which should just work and it doesn't).

Since all formats = VIR_STORAGE_FILE_BACKING have .getBackingStore
specified, this function and the approach in db7d7c0e currently
differs only for VIR_STORAGE_FILE_AUTO(_SAFE), but those can't be
specified in the backing chain, right?

One more thing in which this differs is a format change in future, but
IIUC, the order of the elements in the enum can be changed, so if new
format without backing support is added or backing support is
implemented for QED, re-ordering of those elements will solve it.

Having said all that, I think changing either approach to the
different one is fine, but changing my patch to the following one will
probably be the cleanest way.

Martin

diff --git i/src/qemu/qemu_domain.c w/src/qemu/qemu_domain.c
index cdd4601..5bde917 100644
--- i/src/qemu/qemu_domain.c
+++ w/src/qemu/qemu_domain.c
@@ -2246,11 +2246,14 @@ qemuDomainCheckDiskPresence(virQEMUDriverPtr driver,
VIR_DEBUG(Checking for disk presence);
for (i = vm-def-ndisks; i  0; i--) {
disk = vm-def-disks[i - 1];
+enum virStorageFileFormat format = virDomainDiskGetFormat(disk);

if (!virDomainDiskGetSource(disk))
continue;

-if (qemuDomainDetermineDiskChain(driver, vm, disk, false) = 0 
+if ((format  VIR_STORAGE_FILE_NONE ||
+ format = VIR_STORAGE_FILE_BACKING) 
+qemuDomainDetermineDiskChain(driver, vm, disk, false) = 0 
qemuDiskChainCheckBroken(disk) = 0)
continue;

--


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

Re: [libvirt] libvirt-libxl driver defaulting to tap disk and not working (ubuntu 12.10 + xen 4.5 + libvirt 1.2.3 + openstack)

2014-04-22 Thread Tian, Shuangtai
Jim and Stefan

Thanks , I will check my environment again.

-Original Message-
From: Stefan Bader [mailto:stefan.ba...@canonical.com] 
Sent: Tuesday, April 22, 2014 4:20 PM
To: Jim Fehlig; Tian, Shuangtai
Cc: libvir-list@redhat.com
Subject: Re: [libvirt] libvirt-libxl driver defaulting to tap disk and not 
working (ubuntu 12.10 + xen 4.5 + libvirt 1.2.3 + openstack)

On 21.04.2014 23:53, Jim Fehlig wrote:
 Tian, Shuangtai wrote:
 Hi, Jim
 The blktap seems not a module in xen 4.5, when I tried the load it , can not 
 find the module, is there something wrong I did?
   
 
 It would be provided by your dom0 kernel, not Xen.  The Ubuntu Xen 
 kernel doesn't provide a blktap module?

There is a blktap source package which can be installed separately. Though I 
must admit I never tried to use it. There is some code for blktap(2) in the Xen 
source but I had to disable that because it would otherwise clash with the 
external package.
Tian, you may try to sudo apt-get install blktap-utils which should pull in 
the other pieces (libraryies/dkms module).

-Stefan

 
 Regards,
 Jim
 
 BTW ,I compiled the xen4.5 by myself.
 Thanks ! 

 -Original Message-
 From: Jim Fehlig [mailto:jfeh...@suse.com]
 Sent: Friday, April 18, 2014 11:52 PM
 To: Tian, Shuangtai
 Cc: libvir-list@redhat.com
 Subject: Re: [libvirt] libvirt-libxl driver defaulting to tap disk 
 and not working (ubuntu 12.10 + xen 4.5 + libvirt 1.2.3 + openstack)

 Tian, Shuangtai wrote:
   
 HI

 I am an openstacker, when I used the latest libvirt and xen code to 
 run the openstack. Can not create the vm.

 there is an error in libxl log, you can see the log:

 Os : Ubuntu 12.10

  

 Compiled against library: libvirt 1.2.3

 Using library: libvirt 1.2.3

 Using API: Xen 1.2.3

 Running hypervisor: Xen 4.5.0

  

 libxl: debug: libxl_create.c:1356:do_domain_create: ao 0x7f7894002810:
 create: how=(nil) callback=(nil) poller=0x7f7894001bb0

 libxl: debug: libxl_device.c:251:libxl__device_disk_set_backend: 
 Disk vdev=xvda spec.backend=tap

 libxl: debug: libxl_device.c:210:disk_try_backend: Disk vdev=xvda, 
 backend tap unsuitable because blktap not available

 

 In addition to John's suggestions, try loading a blktap module in dom0. 
 It should work if a blktap driver is available.

 Regards,
 Jim

   
 libxl: error: libxl_device.c:289:libxl__device_disk_set_backend: no 
 suitable backend for disk xvda

 libxl: debug: libxl_event.c:1739:libxl__ao_complete: ao
 0x7f7894002810: complete, rc=-3

 libxl: debug: libxl_create.c:1370:do_domain_create: ao 0x7f7894002810:
 inprogress: poller=0x7f7894001bb0, flags=ic

 libxl: debug: libxl_event.c:1711:libxl__ao__destroy: ao
 0x7f7894002810: destroy

  

 The blktap does work, and I also find the same error someone has 
 posted, 
 (http://www.redhat.com/archives/libvir-list/2013-February/msg01124.h
 tm
 l)

 When I change the type to “phy”, it also doesnot work. And try to 
 change the type to other options, also does not work.

 Can someone give me some suggestions?

 Thanks !

  

 The XML from the openstack is :

 domain type=xen

   uuid9e9ed86c-8892-40da-acd1-31ec6303abfe/uuid

   nameinstance-0001/name

   memory524288/memory

   vcpu1/vcpu

   os

 typexen/type


 kernel/opt/stack/data/nova/instances/9e9ed86c-8892-40da-acd1-31ec6
 30
 3abfe/kernel/kernel


 initrd/opt/stack/data/nova/instances/9e9ed86c-8892-40da-acd1-31ec6
 30
 3abfe/ramdisk/initrd

 cmdlinero root=/dev/xvda/cmdline

   /os

   features

acpi/

 apic/

   /features

   clock offset=utc/

   devices

 disk type=file device=disk

   driver name=tap2 type=raw cache=none/

   source
 file=/opt/stack/data/nova/instances/9e9ed86c-8892-40da-acd1-31ec630
 3a
 bfe/disk/

   target bus=xen dev=xvda/

 /disk

 disk type=file device=cdrom

   driver name=tap2 type=raw cache=none/

   source
 file=/opt/stack/data/nova/instances/9e9ed86c-8892-40da-acd1-31ec630
 3a
 bfe/disk.config/

   target bus=ide dev=xvdd/

 /disk

 interface type=bridge

   mac address=fa:16:3e:d8:c3:c0/

   source bridge=br100/

   filterref
 filter=nova-instance-instance-0001-fa163ed8c3c0/

 /interface

 console type=pty/

 graphics type=vnc autoport=yes keymap=en-us
 listen=127.0.0.1/

 video

   model type=xen/

 /video

   /devices

 /domain

  

  

  

 Best regards,

 Tian, Shuangtai

  

 
 --
 --

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



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

[libvirt] [test-API][PATCH V2 1/2] Add new cases for testing setSchedulerParametersFlags related API

2014-04-22 Thread Xuesong Zhang
---
 repos/domain/sched_params_flag.py | 155 ++
 1 file changed, 155 insertions(+)
 create mode 100755 repos/domain/sched_params_flag.py

diff --git a/repos/domain/sched_params_flag.py 
b/repos/domain/sched_params_flag.py
new file mode 100755
index 000..f7a5e44
--- /dev/null
+++ b/repos/domain/sched_params_flag.py
@@ -0,0 +1,155 @@
+#!/usr/bin/evn python
+# Set and show scheduler parameters with flag, such as --current, --live
+# and --config
+
+import libvirt
+from libvirt import libvirtError
+
+from src import sharedmod
+from utils import utils
+from xml.dom import minidom
+import os
+
+required_params = ('guestname', 'vcpuquota', 'vcpuperiod', 'emulatorperiod',
+   'emulatorquota', 'cpushares', 'flag',)
+optional_params = {}
+
+def check_sched_params_flag(guestname, domobj, sched_params_after, domstate,
+flags_value):
+Check scheduler parameters validity after setting
+
+
+if (domstate == 1) and ((flags_value == 0) or (flags_value == 1)):
+While the domain is running and the flag is --live or --current,
+   the value can be checked with the cgroup value
+   As for the other condition, the value can be checked with the domain
+   config xml
+
+
+if os.path.exists(/cgroup):
+ Add the judgment method, since the cgroup path is different on
+rhel6 and rhel7.
+if the folder cgroup is existed, it means the host os is rhel6,
+if not existed, it means the the host of is rhel7
+
+
+cgroup_path = cat /cgroup/cpu/libvirt/qemu/%s/ % guestname
+else:
+cgroup_path = cat /sys/fs/cgroup/cpu\,cpuacct/machine.slice/ \
+  machine-qemux2d%s.scope/ % guestname
+
+sched_dicts = {'vcpu_quota': 'vcpu0/cpu.cfs_quota_us',
+   'vcpu_period': 'vcpu0/cpu.cfs_period_us',
+   'emulator_period': 'emulator/cpu.cfs_period_us',
+   'emulator_quota': 'emulator/cpu.cfs_quota_us',
+   'cpu_shares': 'cpu.shares'}
+
+for sched_key in sched_dicts:
+cmd = cgroup_path + sched_dicts[sched_key]
+status, cmd_value = utils.exec_cmd(cmd, shell=True)
+if status:
+logger.error(failed to get ***%s*** value % sched_key)
+return 1
+sched_dicts[sched_key] = int(cmd_value[0])
+if sched_dicts[sched_key] != sched_params_after[sched_key]:
+logger.error(set scheduler parameters failed)
+return 1
+
+else:
+guestxml = domobj.XMLDesc(libvirt.VIR_DOMAIN_XML_INACTIVE)
+logger.debug(domain %s config xml :\n%s % (domobj.name(), guestxml))
+
+xmlrootnode = minidom.parseString(guestxml)
+
+sched_dicts = {'vcpu_quota': 'quota', 'vcpu_period': 'period',
+   'emulator_period': 'emulator_period',
+   'emulator_quota': 'emulator_quota',
+   'cpu_shares': 'shares'}
+
+for sched_key in sched_dicts:
+node = xmlrootnode.getElementsByTagName(sched_dicts[sched_key])[0]
+sched_dicts[sched_key] = int(node.childNodes[0].data)
+if sched_dicts[sched_key] != sched_params_after[sched_key]:
+logger.error(set scheduler parameters failed)
+return 1
+
+logger.info(set scheduler parameters success)
+return 0
+
+def sched_params_flag(params):
+ Change and get the scheduler parameters
+
+
+global logger
+logger = params['logger']
+guestname = params['guestname']
+dicts = {'vcpu_quota': int(params['vcpuquota']),
+ 'vcpu_period': int(params['vcpuperiod']),
+ 'emulator_period': int(params['emulatorperiod']),
+ 'emulator_quota': int(params['emulatorquota']),
+ 'cpu_shares': int(params['cpushares'])}
+flags = params['flag']
+
+try:
+conn = sharedmod.libvirtobj['conn']
+domobj = conn.lookupByName(guestname)
+
+domstate = domobj.state(0)[0]
+virDomainState
+   VIR_DOMAIN_NOSTATE = 0
+   VIR_DOMAIN_RUNNING = 1
+   VIR_DOMAIN_BLOCKED = 2
+   VIR_DOMAIN_PAUSED = 3
+   VIR_DOMAIN_SHUTDOWN = 4
+   VIR_DOMAIN_SHUTOFF = 5
+   VIR_DOMAIN_CRASHED = 6
+   VIR_DOMAIN_PMSUSPENDED = 7
+
+   please see the following reference link:
+   http://libvirt.org/html/libvirt-libvirt.html#virDomainState
+
+if domstate == libvirt.VIR_DOMAIN_RUNNING:
+logger.info(the state of virtual machine is ***running***)
+elif domstate == libvirt.VIR_DOMAIN_SHUTOFF:
+logger.info(the state of virtual machine is ***shutoff***)
+else:
+logger.error(the state of virtual machine is not running or  \
+ 

[libvirt] [test-API][PATCH V2 2/2] Add scenario conf file for testing setSchedulerParametersFlags related API

2014-04-22 Thread Xuesong Zhang
---
 cases/sched_params.conf | 106 
 1 file changed, 106 insertions(+)
 create mode 100755 cases/sched_params.conf

diff --git a/cases/sched_params.conf b/cases/sched_params.conf
new file mode 100755
index 000..21b6c04
--- /dev/null
+++ b/cases/sched_params.conf
@@ -0,0 +1,106 @@
+domain:install_linux_cdrom
+guestname
+$defaultname
+guestos
+$defaultos
+guestarch
+$defaultarch
+vcpu
+$defaultvcpu
+memory
+$defaultmem
+hddriver
+$defaulthd
+nicdriver
+$defaultnic
+imageformat
+qcow2
+
+domain:sched_params_flag
+guestname
+$defaultname
+vcpuquota
+1001
+vcpuperiod
+21
+emulatorperiod
+31
+emulatorquota
+4001
+cpushares
+5001
+flag
+current
+
+domain:sched_params_flag
+guestname
+$defaultname
+vcpuquota
+1002
+vcpuperiod
+22
+emulatorperiod
+32
+emulatorquota
+4002
+cpushares
+5002
+flag
+live
+
+domain:sched_params_flag
+guestname
+$defaultname
+vcpuquota
+1003
+vcpuperiod
+23
+emulatorperiod
+33
+emulatorquota
+4003
+cpushares
+5003
+flag
+config
+
+domain:destroy
+guestname
+$defaultname
+
+domain:sched_params_flag
+guestname
+$defaultname
+vcpuquota
+1004
+vcpuperiod
+24
+emulatorperiod
+34
+emulatorquota
+4004
+cpushares
+5004
+flag
+current
+
+domain:sched_params_flag
+guestname
+$defaultname
+vcpuquota
+1005
+vcpuperiod
+25
+emulatorperiod
+35
+emulatorquota
+4005
+cpushares
+5005
+flag
+config
+
+domain:undefine
+guestname
+$defaultname
+
-- 
1.8.3.1

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


[libvirt] [test-API][PATCH V2 0/2] Add case for setSchedulerParametersFlag API

2014-04-22 Thread Xuesong Zhang
V1-V2:
Delete the symbol '\' at the end of lines.
Update the domian stats to libvirt.VIR_DOMAIN_RUNNING and
 libvirt.VIR_DOMAIN_SHUTOFF, instead of the
 static value.
Delete the scenario without flags in file shed_parms.conf

V1:
Add test case for setSchedulerParametersFlags related API
Add conf file of setSchedulerParametersFlags test


Xuesong Zhang (2):
  Add new cases for testing setSchedulerParametersFlags related API
  Add scenario conf file for testing setSchedulerParametersFlags related
API

 cases/sched_params.conf   | 106 ++
 repos/domain/sched_params_flag.py | 155 ++
 2 files changed, 261 insertions(+)
 create mode 100755 cases/sched_params.conf
 create mode 100755 repos/domain/sched_params_flag.py

-- 
1.8.3.1

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


Re: [libvirt] [PATCH v2] Add support for migration URI configuration

2014-04-22 Thread chen.fan.f...@cn.fujitsu.com
Ping...

On Mon, 2014-04-21 at 17:15 +0800, Chen Fan wrote: 
 For now, we set the migration URI from command line '--migrate_uri' or
 construct the URI by looking up the dest host's hostname which could be
 solved by DNS automatically.
 
 But in cases the dest host have two or more NICs to reach, we may need to
 send the migration data over a specific NIC which is different from the
 automatically resloved one for some reason like performance, security, etc.
 thus we must explicitly specify the migrateuri in command line everytime,
 but it is too troublesome if there are many such hosts(and don't forget
 virt-manager).
 
 This patch adds a configuration file option on dest host to save the
 default migrate uri which explicitly configure which of this host's
 addresses should be used to transfer, thus user doesn't boring to specify
 it in command line everytime.
 
 Signed-off-by: Chen Fan chen.fan.f...@cn.fujitsu.com
 ---
  src/qemu/qemu.conf| 5 -
  src/qemu/qemu_conf.c  | 2 ++
  src/qemu/qemu_conf.h  | 2 ++
  src/qemu/qemu_migration.c | 5 +
  4 files changed, 13 insertions(+), 1 deletion(-)
 
 diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf
 index f0e802f..2973631 100644
 --- a/src/qemu/qemu.conf
 +++ b/src/qemu/qemu.conf
 @@ -449,7 +449,10 @@
  #
  #seccomp_sandbox = 1
  
 -
 +# Override the URI used for any specific migration URI to be sent.
 +# Defaults to NULL, will be set to as tcp://hostIP[:port].
 +#
 +#migrate_uri = tcp://hostIP:port
  
  # Override the listen address for all incoming migrations. Defaults to
  # 0.0.0.0, or :: if both host and qemu are capable of IPv6.
 diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
 index 198ee2f..0bd943d 100644
 --- a/src/qemu/qemu_conf.c
 +++ b/src/qemu/qemu_conf.c
 @@ -576,6 +576,8 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr 
 cfg,
  
  GET_VALUE_STR(migration_address, cfg-migrationAddress);
  
 +GET_VALUE_STR(migrate_uri, cfg-migrateUri);
 +
  ret = 0;
  
   cleanup:
 diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
 index a36ea63..2e45421 100644
 --- a/src/qemu/qemu_conf.h
 +++ b/src/qemu/qemu_conf.h
 @@ -167,6 +167,8 @@ struct _virQEMUDriverConfig {
  char *migrationAddress;
  int migrationPortMin;
  int migrationPortMax;
 +
 +char *migrateUri;
  };
  
  /* Main driver state */
 diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
 index 593d2d3..ab64c58 100644
 --- a/src/qemu/qemu_migration.c
 +++ b/src/qemu/qemu_migration.c
 @@ -2639,6 +2639,7 @@ qemuMigrationPrepareDirect(virQEMUDriverPtr driver,
  int ret = -1;
  virURIPtr uri = NULL;
  bool well_formed_uri = true;
 +virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
  
  VIR_DEBUG(driver=%p, dconn=%p, cookiein=%s, cookieinlen=%d, 
cookieout=%p, cookieoutlen=%p, uri_in=%s, uri_out=%p, 
 @@ -2649,6 +2650,10 @@ qemuMigrationPrepareDirect(virQEMUDriverPtr driver,
  
  *uri_out = NULL;
  
 +if (uri_in == NULL  cfg-migrateUri) {
 +uri_in = cfg-migrateUri;
 +}
 +
  /* The URI passed in may be NULL or a string tcp://somehostname:port.
   *
   * If the URI passed in is NULL then we allocate a port number


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


Re: [libvirt] Proper behavior when creating a new transient domain with same name/uuid as existing persistent domain

2014-04-22 Thread Daniel P. Berrange
On Mon, Apr 21, 2014 at 09:43:21AM -0600, Eric Blake wrote:
 On 04/21/2014 05:13 AM, Laine Stump wrote:
  Playing around with the network driver I found that if I net-define a
  network, then net-create using a slightly modified version of the same
  XML file (e.g. different bridge name, but same name/uuid) the following
  will happen:
  
  1) a network of that name is started, it is persistent, and it has the
  properties from the modified XML file (the one given to net-create).
  
  2) both net-dumpxml and net-dumpxml --inactive will show the
  modified attributes.
  
  3) when I net-destroy the network, the network will still exist (since
  it is still persistent), but will have all of the modified attributes
  rather than the originals.
  
  Wanting the network driver to behave consistently with the qemu driver,
  I did the same test with a qemu domain. I found exactly the same behavior.
  
  I would have expected that when I tried to create a domain (or network)
  with the same name  UUID as an existing persistentinactive domain (or
  network) that one of the following two things would happen:
  
  1) the operation would fail because a persistent domain/network already
  existed
 
 No, we explicitly document that attempting to 'create' something that
 already exists as an inactive persistent object is merely longhand for
 'start'ing the existing object.
 
  
  2) the domain/network would be started with the modified attributes, but
  the persistent definition would still show the original attributes,
  which would remain after destroying the transient
 
 Well, it's really only one object.  But yes, I'd expect the persistent
 definition to remain unchanged, and that the temporary modified
 definition passed through 'create' is lost once the object is no longer
 active.

Yes, that is what /used/ to happen. If you boot from a transient XML
config, that should not impact the persistent config once the guest
is shutoff again.


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

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


Re: [libvirt] [PATCH perl-Sys-Virt] Don't die if snapshots are unsupported

2014-04-22 Thread Daniel P. Berrange
On Mon, Apr 21, 2014 at 02:19:12PM -0600, Mike Latimer wrote:
 Many libvirt-tck tests do not explicitly cleanup test domains before exiting.
 In this case Sys::Virt::TCK-cleanup is triggered, and the environment is
 reset automatically. During this reset, any existing snapshots are found for
 deletion through Sys::Virt::Domain-list_snapshots. If the underlying
 connection driver does not support snapshot functions (such as lxc or libxl),
 the process dies with the following error:
 
 libvirt error code: 3, message: this function is not supported by the
  connection driver: virDomainSnapshotListNames
 
 The best resolution would be to add snapshot support to the driver(s), but as
 this requires a long term effort, it is better to catch the error and return
 an empty list of snapshots to the calling function. In the case of
 libvirt-tck, this allows the cleanup to succeed and subsequent testing to
 continue normally.
 
 ---
  Domain.pm |5 -
  1 file changed, 4 insertions(+), 1 deletion(-)
 
 --- a/lib/Sys/Virt/Domain.pm  2014-04-07 04:35:31.0 -0600
 +++ b/lib/Sys/Virt/Domain.pm  2014-04-21 13:48:22.868956838 -0600
 @@ -1416,7 +1416,10 @@ recommended as a more efficient alternat
  sub list_snapshots {
  my $self = shift;
  
 -my $nnames = $self-num_of_snapshots();
 +# If snapshots are not supported by the driver (lxc, libxl...), catch the
 +# function not supported error and simply return as there are no 
 snapshots
 +my $nnames = eval { $self-num_of_snapshots() };
 +return if($@ =~ /function is not supported/);
  my @names = $self-list_snapshot_names($nnames);

Unfortunately this is a change in API behaviour that affects more
than just the TCK. You need todo the equivalent change to the TCK
code instead really.


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

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


Re: [libvirt] [PATCH 1/4] conf: Output disk backing store details in domain XML

2014-04-22 Thread Peter Krempa
On 04/21/14 10:32, Jiri Denemark wrote:
 The XML for quite a longish backing chain is shown below:
 
   disk type='network' device='disk'
 driver name='qemu' type='qcow2'/
 source protocol='nbd' name='bar'
   host transport='unix' socket='/var/run/nbdsock'/
 /source
 backingStore type='block' index='1'
   format type='qcow2'/
   source dev='/dev/HostVG/QEMUGuest1'/
   backingStore type='file' index='2'
 format type='qcow2'/
 source file='/tmp/image2.qcow'/
 backingStore type='file' index='3'
   format type='qcow2'/
   source file='/tmp/image3.qcow'/
   backingStore type='file' index='4'
 format type='qcow2'/
 source file='/tmp/image4.qcow'/
 backingStore type='file' index='5'
   format type='qcow2'/
   source file='/tmp/image5.qcow'/
   backingStore type='file' index='6'
 format type='raw'/
 source file='/tmp/Fedora-17-x86_64-Live-KDE.iso'/
 backingStore/
   /backingStore
 /backingStore
   /backingStore
 /backingStore
   /backingStore
 /backingStore
 target dev='vdb' bus='virtio'/
   /disk
 
 Various disk types and formats can be mixed in one chain. The
 backingStore/ empty element marks the end of the backing chain and it
 is there mostly for future support of parsing the chain provided by a
 user. If it's missing, we are supposed to probe for the rest of the
 chain ourselves, otherwise complete chain was provided by the user. The
 index attributes of backingStore elements can be used to unambiguously
 identify a specific part of the image chain.
 
 Signed-off-by: Jiri Denemark jdene...@redhat.com
 ---
  docs/formatdomain.html.in | 59 +++
  docs/schemas/domaincommon.rng | 48 ++--
  tests/domainschemadata/backing-chains.xml | 94 
 +++
  3 files changed, 195 insertions(+), 6 deletions(-)
  create mode 100644 tests/domainschemadata/backing-chains.xml
 
 diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
 index e851f85..a6fccd9 100644
 --- a/docs/formatdomain.html.in
 +++ b/docs/formatdomain.html.in

...

 @@ -1814,6 +1828,51 @@
  This feature doesn't support migration currently.
  /p
  /dd
 +  dtcodebackingStore/code/dt
 +  dd
 +This element describes the backing store used by the disk specified 
 by
 +sibling codesource/code element. span class=sinceSince
 +1.2.4/span. An empty codebackingStore/code element means the
 +sibling source is self-contained and is not based on any backing 
 store.
 +The following attributes and sub-elements are supported in
 +codebackingStore/code:
 +dl
 +  dtcodetype/code attribute/dt
 +  dd
 +The codetype/code attribute represents the type of disk used
 +by the backing store, see disk type attribute above for more
 +details and possible values.
 +  /dd
 +  dtcodeindex/code attribute/dt
 +  dd
 +This attribute is only valid in output (and ignored on input) and
 +it can be used to refer to a specific part of the disk chain when
 +doing block operations (such as via the
 +codevirDomainBlockRebase/code API). For example,
 +codevda[2]/code refers to the backing store with
 +codeindex='2'/code of the disk with codevda/code target.
 +  /dd
 +  dtcodeformat/code sub-element/dt
 +  dd
 +The codeformat/code element contains codetype/code
 +attribute which specifies the internal format of the backing
 +store, such as coderaw/code or codeqcow2/code.
 +  /dd
 +  dtcodesource/code sub-element/dt
 +  dd
 +This element has the same structure as the codesource/code
 +element in codedriver/code. It specifies which file, device,

s/driver/disk/ ?

 +or network location contains the data of the described backing
 +store.
 +  /dd
 +  dtcodebackingStore/code sub-element/dt
 +  dd
 +If the backing store is not self-contained, the next element
 +in the chain is described by nested codebackingStore/code
 +element.
 +  /dd
 +/dl
 +  /dd
dtcodemirror/code/dt
dd
  This element is present if the hypervisor has started a block


Maybe we should also document that user-specified backing chains aren't
currently supported.

ACK with the two issues above addressed.

Peter



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

Re: [libvirt] Proper behavior when creating a new transient domain with same name/uuid as existing persistent domain

2014-04-22 Thread Laine Stump
On 04/21/2014 06:43 PM, Eric Blake wrote:
 On 04/21/2014 05:13 AM, Laine Stump wrote:
 Playing around with the network driver I found that if I net-define a
 network, then net-create using a slightly modified version of the same
 XML file (e.g. different bridge name, but same name/uuid) the following
 will happen:

 1) a network of that name is started, it is persistent, and it has the
 properties from the modified XML file (the one given to net-create).

 2) both net-dumpxml and net-dumpxml --inactive will show the
 modified attributes.

 3) when I net-destroy the network, the network will still exist (since
 it is still persistent), but will have all of the modified attributes
 rather than the originals.

 Wanting the network driver to behave consistently with the qemu driver,
 I did the same test with a qemu domain. I found exactly the same behavior.

 I would have expected that when I tried to create a domain (or network)
 with the same name  UUID as an existing persistentinactive domain (or
 network) that one of the following two things would happen:

 1) the operation would fail because a persistent domain/network already
 existed
 No, we explicitly document that attempting to 'create' something that
 already exists as an inactive persistent object is merely longhand for
 'start'ing the existing object.

[citation missing] :-)


 2) the domain/network would be started with the modified attributes, but
 the persistent definition would still show the original attributes,
 which would remain after destroying the transient
 Well, it's really only one object.  But yes, I'd expect the persistent
 definition to remain unchanged, and that the temporary modified
 definition passed through 'create' is lost once the object is no longer
 active.

 3) upon creating the transient version, the domain/network would have
 its persistent flag cleared, and would thus cease to exist when destroyed.
 No, we explicitly document the following possible transitions:

 no object - persistent inactive ('define')
 no object - transient running ('create')
 persistent inactive - persistent running ('start')
 persistent inactive - no object ('undefine')
 transient running - persistent running ('define')
 transient running - no object ('destroy')
 persistent running - persistent inactive ('destroy')
 persistent running - transient running ('undefine')

I was looking for a table like that and didn't find it in the places I
looked in the docs directory, the autogenerated API docs, or on the
wiki. The closest I found was the diagram here:

http://wiki.libvirt.org/page/VM_lifecycle#Transient_guest_domains_vs_Persistent_guest_domains

but that diagram has some extra transitions/states not mentioned above
(due to the addition of Paused and Saved) and is missing others
(because it doesn't attempt to show a difference between
persistent+running and transient+running, among other things).

So where did your table come from?

(I also notice that the table you give above itself makes no mention of
the fact that create can be used to transition from
persistent/inactive to persistent/running :-))


 Was the current behavior consciously arrived at, or is it just an
 accident of circumstances? If the latter, then what should the proper
 behavior be? (I would vote for (2))
 I also vote for behavior (2)


And since Dan says that it previously worked this way, I will make the
network driver (which have *never* gotten transient objects quite right)
work this way, and unless someone else beats me to it I'll fix the
regression in the qemu driver.

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


Re: [libvirt] [PATCH 2/4] conf: Format and parse backing chains in domain XML

2014-04-22 Thread Peter Krempa
On 04/21/14 10:32, Jiri Denemark wrote:
 This patch implements formating and parsing code for the backing store
 schema defined and documented by the previous patch.
 
 This patch does not aim at providing full persistent storage of disk
 backing chains yet. The formatter is supposed to provide the backing
 chain detected when starting a domain and thus it is not formatted into
 an inactive domain XML. The parser is implemented mainly for the purpose
 of testing the XML generated by the formatter and thus it does not
 distinguish between no backingStore element and an empty backingStore
 element. This will have to change once we fully implement support for
 user-supplied backing chains.
 
 Signed-off-by: Jiri Denemark jdene...@redhat.com
 ---
  src/conf/domain_conf.c | 128 
 +

...

  54 files changed, 211 insertions(+)
 
 diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
 index 99b57ee..9dbe0af 100644
 --- a/src/conf/domain_conf.c
 +++ b/src/conf/domain_conf.c

...

 @@ -5842,6 +5910,9 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
   virDomainDiskDefAssignAddress(xmlopt, def)  0)
  goto error;
  
 +if (virDomainDiskBackingStoreParse(ctxt, def-src)  0)
 +goto error;
 +
   cleanup:
  VIR_FREE(bus);
  VIR_FREE(type);

Hmmm, I think we should notify the user somehow that the parsed input
will be ignored, but that can be done later as we still have time until
the release.

ACK.

Peter




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

Re: [libvirt] [PATCH 3/4] tests: More output options for xml2xml tests

2014-04-22 Thread Peter Krempa
On 04/21/14 10:32, Jiri Denemark wrote:
 So far, qemuxml2xml test was only able to check if the result matches
 the original or the appropriate XML in qemuxml2xmloutdata regardless on
 flags used to format the XML. Since the result can be different
 depending on VIR_DOMAIN_XML_INACTIVE flag being used or not, this patch
 adds support for qemuxml2xmlout-%s-active.xml and
 qemuxml2xmlout-%s-inactive.xml output files. If the file specific to the
 flag used exists, it is used in preference to the generic
 qemuxml2xmlout-%s.xml file when reading the expected output.
 
 Signed-off-by: Jiri Denemark jdene...@redhat.com
 ---
  tests/qemuxml2xmltest.c | 44 ++--
  1 file changed, 34 insertions(+), 10 deletions(-)
 

ACK.

Peter




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

Re: [libvirt] [PATCH 4/4] tests: Test backing store XML formatting and parsing

2014-04-22 Thread Peter Krempa
On 04/21/14 10:32, Jiri Denemark wrote:
 Signed-off-by: Jiri Denemark jdene...@redhat.com
 ---
  .../qemuxml2argv-disk-backing-chains.xml   | 94 +
  .../qemuxml2xmlout-disk-backing-chains-active.xml  | 96 
 ++
  ...qemuxml2xmlout-disk-backing-chains-inactive.xml | 59 +
  tests/qemuxml2xmltest.c|  2 +
  4 files changed, 251 insertions(+)
  create mode 100644 
 tests/qemuxml2argvdata/qemuxml2argv-disk-backing-chains.xml
  create mode 100644 
 tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-backing-chains-active.xml
  create mode 100644 
 tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-backing-chains-inactive.xml
 

ACK.

Peter




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

[libvirt] [PATCH 2/4] qemuDomainBlockCommit: Track virStorageSourcePtr for base

2014-04-22 Thread Jiri Denemark
virStorageFileChainLookup is able to give use virStorageSourcePtr which
contains the pointer to its canonical path. Let's use a more general
virStorageSourcePtr instead of just canonical path.

Former base_canon maps to baseSource-path.

Signed-off-by: Jiri Denemark jdene...@redhat.com
---
 src/qemu/qemu_driver.c | 18 +++---
 1 file changed, 7 insertions(+), 11 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index a557d75..35ab2f0 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -15297,8 +15297,8 @@ qemuDomainBlockCommit(virDomainPtr dom, const char 
*path, const char *base,
 int idx;
 virDomainDiskDefPtr disk = NULL;
 virStorageSourcePtr topSource;
+virStorageSourcePtr baseSource;
 const char *top_parent = NULL;
-const char *base_canon = NULL;
 bool clean_access = false;
 
 virCheckFlags(VIR_DOMAIN_BLOCK_COMMIT_SHALLOW, -1);
@@ -15353,16 +15353,12 @@ qemuDomainBlockCommit(virDomainPtr dom, const char 
*path, const char *base,
 }
 
 if (!base  (flags  VIR_DOMAIN_BLOCK_COMMIT_SHALLOW))
-base_canon = topSource-backingStore-path;
-else if (!(base_canon = virStorageFileChainLookup(topSource,
-  base, NULL, NULL)))
+baseSource = topSource-backingStore;
+else if (!(virStorageFileChainLookup(topSource, base, baseSource, NULL)))
 goto endjob;
 
-/* Note that this code exploits the fact that
- * virStorageFileChainLookup guarantees a simple pointer
- * comparison will work, rather than needing full-blown STREQ.  */
 if ((flags  VIR_DOMAIN_BLOCK_COMMIT_SHALLOW) 
-base_canon != topSource-backingStore-path) {
+baseSource != topSource-backingStore) {
 virReportError(VIR_ERR_INVALID_ARG,
_(base '%s' is not immediately below '%s' in chain 
  for '%s'),
@@ -15378,7 +15374,7 @@ qemuDomainBlockCommit(virDomainPtr dom, const char 
*path, const char *base,
  * operation succeeds, but doing that requires tracking the
  * operation in XML across libvirtd restarts.  */
 clean_access = true;
-if (qemuDomainPrepareDiskChainElement(driver, vm, disk, base_canon,
+if (qemuDomainPrepareDiskChainElement(driver, vm, disk, baseSource-path,
   VIR_DISK_CHAIN_READ_WRITE)  0 ||
 (top_parent  top_parent != disk-src.path 
  qemuDomainPrepareDiskChainElement(driver, vm, disk,
@@ -15394,13 +15390,13 @@ qemuDomainBlockCommit(virDomainPtr dom, const char 
*path, const char *base,
 qemuDomainObjEnterMonitor(driver, vm);
 ret = qemuMonitorBlockCommit(priv-mon, device,
  top ? top : topSource-path,
- base ? base : base_canon, bandwidth);
+ base ? base : baseSource-path, bandwidth);
 qemuDomainObjExitMonitor(driver, vm);
 
  endjob:
 if (ret  0  clean_access) {
 /* Revert access to read-only, if possible.  */
-qemuDomainPrepareDiskChainElement(driver, vm, disk, base_canon,
+qemuDomainPrepareDiskChainElement(driver, vm, disk, baseSource-path,
   VIR_DISK_CHAIN_READ_ONLY);
 if (top_parent  top_parent != disk-src.path)
 qemuDomainPrepareDiskChainElement(driver, vm, disk,
-- 
1.9.2

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


[libvirt] [PATCH 0/4] Add support for addressing backing stores by index

2014-04-22 Thread Jiri Denemark
Jiri Denemark (4):
  qemuDomainBlockCommit: Don't track top_canon path separately
  qemuDomainBlockCommit: Track virStorageSourcePtr for base
  virStorageFileChainLookup: Return virStorageSourcePtr
  Add support for addressing backing stores by index

 src/libvirt.c | 13 ++-
 src/libvirt_private.syms  |  1 +
 src/qemu/qemu_driver.c| 57 +++-
 src/util/virstoragefile.c | 97 +--
 src/util/virstoragefile.h | 14 +--
 tests/virstoragetest.c| 86 ++---
 6 files changed, 193 insertions(+), 75 deletions(-)

-- 
1.9.2

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


[libvirt] [PATCH 3/4] virStorageFileChainLookup: Return virStorageSourcePtr

2014-04-22 Thread Jiri Denemark
Returning both virStorageSourcePtr and its path member does not make a
lot of sense.

Signed-off-by: Jiri Denemark jdene...@redhat.com
---
 src/qemu/qemu_driver.c|  7 +++
 src/util/virstoragefile.c | 16 +++-
 src/util/virstoragefile.h |  7 +++
 tests/virstoragetest.c| 39 +++
 4 files changed, 36 insertions(+), 33 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 35ab2f0..0283d99 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -15340,9 +15340,8 @@ qemuDomainBlockCommit(virDomainPtr dom, const char 
*path, const char *base,
 
 if (!top) {
 topSource = disk-src;
-} else if (!(virStorageFileChainLookup(disk-src,
-   top, topSource,
-   top_parent))) {
+} else if (!(topSource = virStorageFileChainLookup(disk-src.backingStore,
+   top, top_parent))) {
 goto endjob;
 }
 if (!topSource-backingStore) {
@@ -15354,7 +15353,7 @@ qemuDomainBlockCommit(virDomainPtr dom, const char 
*path, const char *base,
 
 if (!base  (flags  VIR_DOMAIN_BLOCK_COMMIT_SHALLOW))
 baseSource = topSource-backingStore;
-else if (!(virStorageFileChainLookup(topSource, base, baseSource, NULL)))
+else if (!(baseSource = virStorageFileChainLookup(topSource, base, NULL)))
 goto endjob;
 
 if ((flags  VIR_DOMAIN_BLOCK_COMMIT_SHALLOW) 
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index b2d8f62..4fdbb8b 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -1510,9 +1510,9 @@ int virStorageFileGetSCSIKey(const char *path,
  * Since the results point within CHAIN, they must not be
  * independently freed.  Reports an error and returns NULL if NAME is
  * not found.  */
-const char *
+virStorageSourcePtr
 virStorageFileChainLookup(virStorageSourcePtr chain,
-  const char *name, virStorageSourcePtr *meta,
+  const char *name,
   const char **parent)
 {
 const char *start = chain-path;
@@ -1545,24 +1545,22 @@ virStorageFileChainLookup(virStorageSourcePtr chain,
 parentDir = chain-relDir;
 chain = chain-backingStore;
 }
+
 if (!chain)
 goto error;
-if (meta)
-*meta = chain;
-return chain-path;
+return chain;
 
  error:
-if (name)
+if (name) {
 virReportError(VIR_ERR_INVALID_ARG,
_(could not find image '%s' in chain for '%s'),
name, start);
-else
+} else {
 virReportError(VIR_ERR_INVALID_ARG,
_(could not find base image in chain for '%s'),
start);
+}
 *parent = NULL;
-if (meta)
-*meta = NULL;
 return NULL;
 }
 
diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
index a0adb9b..82198e5 100644
--- a/src/util/virstoragefile.h
+++ b/src/util/virstoragefile.h
@@ -281,10 +281,9 @@ virStorageSourcePtr virStorageFileGetMetadataFromBuf(const 
char *path,
 int virStorageFileChainGetBroken(virStorageSourcePtr chain,
  char **broken_file);
 
-const char *virStorageFileChainLookup(virStorageSourcePtr chain,
-  const char *name,
-  virStorageSourcePtr *meta,
-  const char **parent)
+virStorageSourcePtr virStorageFileChainLookup(virStorageSourcePtr chain,
+  const char *name,
+  const char **parent)
 ATTRIBUTE_NONNULL(1);
 
 int virStorageFileResize(const char *path,
diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c
index 5320c78..edab03a 100644
--- a/tests/virstoragetest.c
+++ b/tests/virstoragetest.c
@@ -424,16 +424,11 @@ testStorageLookup(const void *args)
 {
 const struct testLookupData *data = args;
 int ret = 0;
-const char *actualResult;
-virStorageSourcePtr actualMeta;
+virStorageSourcePtr result;
 const char *actualParent;
 
-/* This function is documented as giving results within chain, but
- * as the same string may be duplicated into more than one field,
- * we rely on STREQ rather than pointer equality.  Test twice to
- * ensure optional parameters don't cause NULL deref.  */
-actualResult = virStorageFileChainLookup(data-chain, data-name,
- NULL, NULL);
+ /* Test twice to ensure optional parameter doesn't cause NULL deref. */
+result = virStorageFileChainLookup(data-chain, data-name, NULL);
 
 if (!data-expResult) {
 if (!virGetLastError()) {
@@ -448,24 +443,36 @@ testStorageLookup(const void *args)
 }
 }
 
-if (STRNEQ_NULLABLE(data-expResult, 

[libvirt] [PATCH 1/4] qemuDomainBlockCommit: Don't track top_canon path separately

2014-04-22 Thread Jiri Denemark
virStorageFileChainLookup is able to give use virStorageSourcePtr which
contains the pointer to its canonical path. There's no need for the
caller to store both of them.

Former top_meta maps to topSource and top_canon maps to topSource-path.

Signed-off-by: Jiri Denemark jdene...@redhat.com
---
 src/qemu/qemu_driver.c | 27 +--
 1 file changed, 13 insertions(+), 14 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index c8f3f6f..a557d75 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -15296,8 +15296,7 @@ qemuDomainBlockCommit(virDomainPtr dom, const char 
*path, const char *base,
 int ret = -1;
 int idx;
 virDomainDiskDefPtr disk = NULL;
-const char *top_canon = NULL;
-virStorageSourcePtr top_meta = NULL;
+virStorageSourcePtr topSource;
 const char *top_parent = NULL;
 const char *base_canon = NULL;
 bool clean_access = false;
@@ -15340,22 +15339,22 @@ qemuDomainBlockCommit(virDomainPtr dom, const char 
*path, const char *base,
 goto endjob;
 
 if (!top) {
-top_canon = disk-src.path;
-top_meta = disk-src;
-} else if (!(top_canon = virStorageFileChainLookup(disk-src,
-   top, top_meta,
-   top_parent))) {
+topSource = disk-src;
+} else if (!(virStorageFileChainLookup(disk-src,
+   top, topSource,
+   top_parent))) {
 goto endjob;
 }
-if (!top_meta || !top_meta-backingStore) {
+if (!topSource-backingStore) {
 virReportError(VIR_ERR_INVALID_ARG,
_(top '%s' in chain for '%s' has no backing file),
-   top_canon, path);
+   topSource-path, path);
 goto endjob;
 }
+
 if (!base  (flags  VIR_DOMAIN_BLOCK_COMMIT_SHALLOW))
-base_canon = top_meta-backingStore-path;
-else if (!(base_canon = virStorageFileChainLookup(top_meta,
+base_canon = topSource-backingStore-path;
+else if (!(base_canon = virStorageFileChainLookup(topSource,
   base, NULL, NULL)))
 goto endjob;
 
@@ -15363,11 +15362,11 @@ qemuDomainBlockCommit(virDomainPtr dom, const char 
*path, const char *base,
  * virStorageFileChainLookup guarantees a simple pointer
  * comparison will work, rather than needing full-blown STREQ.  */
 if ((flags  VIR_DOMAIN_BLOCK_COMMIT_SHALLOW) 
-base_canon != top_meta-backingStore-path) {
+base_canon != topSource-backingStore-path) {
 virReportError(VIR_ERR_INVALID_ARG,
_(base '%s' is not immediately below '%s' in chain 
  for '%s'),
-   base, top_canon, path);
+   base, topSource-path, path);
 goto endjob;
 }
 
@@ -15394,7 +15393,7 @@ qemuDomainBlockCommit(virDomainPtr dom, const char 
*path, const char *base,
  * thing if the user specified a relative name). */
 qemuDomainObjEnterMonitor(driver, vm);
 ret = qemuMonitorBlockCommit(priv-mon, device,
- top ? top : top_canon,
+ top ? top : topSource-path,
  base ? base : base_canon, bandwidth);
 qemuDomainObjExitMonitor(driver, vm);
 
-- 
1.9.2

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


[libvirt] [PATCH 4/4] Add support for addressing backing stores by index

2014-04-22 Thread Jiri Denemark
Each backing store of a given disk is associated with a unique index
(which is also formated in domain XML) for easier addressing of any
particular backing store. With this patch, any backing store can be
addressed by its disk target and the index. For example, vdc[4]
addresses the backing store with index equal to 4 of the disk identified
by vdc target. Such shorthand can be used in any API in place for a
backing file path:

virsh blockcommit domain vda --base vda[3] --top vda[2]

Signed-off-by: Jiri Denemark jdene...@redhat.com
---
 src/libvirt.c | 13 ++--
 src/libvirt_private.syms  |  1 +
 src/qemu/qemu_driver.c| 29 -
 src/util/virstoragefile.c | 83 ---
 src/util/virstoragefile.h |  7 
 tests/virstoragetest.c| 51 -
 6 files changed, 152 insertions(+), 32 deletions(-)

diff --git a/src/libvirt.c b/src/libvirt.c
index 4454829..a9f56c3 100644
--- a/src/libvirt.c
+++ b/src/libvirt.c
@@ -19787,9 +19787,10 @@ virDomainBlockRebase(virDomainPtr dom, const char 
*disk,
  * virDomainBlockCommit:
  * @dom: pointer to domain object
  * @disk: path to the block device, or device shorthand
- * @base: path to backing file to merge into, or NULL for default
+ * @base: path to backing file to merge into, or device shorthand,
+ *or NULL for default
  * @top: path to file within backing chain that contains data to be merged,
- *   or NULL to merge all possible data
+ *   or device shorthand, or NULL to merge all possible data
  * @bandwidth: (optional) specify commit bandwidth limit in MiB/s
  * @flags: bitwise-OR of virDomainBlockCommitFlags
  *
@@ -19839,6 +19840,14 @@ virDomainBlockRebase(virDomainPtr dom, const char 
*disk,
  * can be found by calling virDomainGetXMLDesc() and inspecting
  * elements within //domain/devices/disk.
  *
+ * The @base and @top parameters can be either paths to files within the
+ * backing chain, or the device target shorthand (the target dev='...'/
+ * sub-element, such as vda) followed by an index to the backing chain
+ * enclosed in square brackets. Backing chain indexes can be found by
+ * inspecting //disk//backingStore/@index in the domain XML. Thus, for
+ * example, vda[3] refers to the backing store with index equal to 3
+ * in the chain of disk vda.
+ *
  * The maximum bandwidth (in MiB/s) that will be used to do the commit can be
  * specified with the bandwidth parameter.  If set to 0, libvirt will choose a
  * suitable default.  Some hypervisors do not support this feature and will
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index fe8a810..b77050d 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1830,6 +1830,7 @@ virStorageFileGetMetadataFromBuf;
 virStorageFileGetMetadataFromFD;
 virStorageFileGetSCSIKey;
 virStorageFileIsClusterFS;
+virStorageFileParseChainIndex;
 virStorageFileProbeFormat;
 virStorageFileProbeFormatFromBuf;
 virStorageFileResize;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 0283d99..4ed1123 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -15285,8 +15285,11 @@ qemuDomainBlockPull(virDomainPtr dom, const char 
*path, unsigned long bandwidth,
 
 
 static int
-qemuDomainBlockCommit(virDomainPtr dom, const char *path, const char *base,
-  const char *top, unsigned long bandwidth,
+qemuDomainBlockCommit(virDomainPtr dom,
+  const char *path,
+  const char *base,
+  const char *top,
+  unsigned long bandwidth,
   unsigned int flags)
 {
 virQEMUDriverPtr driver = dom-conn-privateData;
@@ -15297,7 +15300,9 @@ qemuDomainBlockCommit(virDomainPtr dom, const char 
*path, const char *base,
 int idx;
 virDomainDiskDefPtr disk = NULL;
 virStorageSourcePtr topSource;
+unsigned int topIndex = 0;
 virStorageSourcePtr baseSource;
+unsigned int baseIndex = 0;
 const char *top_parent = NULL;
 bool clean_access = false;
 
@@ -15338,12 +15343,15 @@ qemuDomainBlockCommit(virDomainPtr dom, const char 
*path, const char *base,
 if (qemuDomainDetermineDiskChain(driver, vm, disk, false)  0)
 goto endjob;
 
-if (!top) {
+if (!top)
 topSource = disk-src;
-} else if (!(topSource = virStorageFileChainLookup(disk-src.backingStore,
-   top, top_parent))) {
+else if (virStorageFileParseChainIndex(disk-dst, top, topIndex)  0 ||
+ !(topSource = virStorageFileChainLookup(disk-src,
+ disk-src.backingStore,
+ top, topIndex,
+ top_parent)))
 goto endjob;
-}
+
 if (!topSource-backingStore) {
 virReportError(VIR_ERR_INVALID_ARG,

[libvirt] [PATCH 3/3] Fix error for out of range vcpu in qemuDomainPinVcpuFlags

2014-04-22 Thread Ján Tomko
Changes:
error: invalid argument: vcpu number out of range 2  2
to slightly less confusing:
error: invalid argument: vcpu number out of range 2  1
---
 src/qemu/qemu_driver.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 6996b80..c3ac6d4 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4355,7 +4355,7 @@ qemuDomainPinVcpuFlags(virDomainPtr dom,
 if (vcpu  (priv-nvcpupids-1)) {
 virReportError(VIR_ERR_INVALID_ARG,
_(vcpu number out of range %d  %d),
-   vcpu, priv-nvcpupids);
+   vcpu, priv-nvcpupids - 1);
 goto cleanup;
 }
 
-- 
1.8.3.2

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


[libvirt] [PATCH 0/3] vcpu fixes

2014-04-22 Thread Ján Tomko
Ján Tomko (3):
  Properly free vcpupin info for unplugged CPUs
  Make virDomainVcpuPinDel return void
  Fix error for out of range vcpu in qemuDomainPinVcpuFlags

 src/conf/domain_conf.c   | 31 ++-
 src/conf/domain_conf.h   |  5 +
 src/libvirt_private.syms |  1 -
 src/libxl/libxl_driver.c |  7 +--
 src/qemu/qemu_driver.c   | 22 --
 5 files changed, 8 insertions(+), 58 deletions(-)

-- 
1.8.3.2

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

[libvirt] [PATCH 1/3] Properly free vcpupin info for unplugged CPUs

2014-04-22 Thread Ján Tomko
Remove the pointer from def-cputune.vcpupin after unplugging
the CPU and also free the bitmap contained in the structure
by calling virDomainVcpuPinDel instead of VIR_FREE.

Introduced by commit 0df1a79.

This makes virDomainLookupVcpuPin redundant.

https://bugzilla.redhat.com/show_bug.cgi?id=1088165
---
 src/conf/domain_conf.c   | 20 
 src/conf/domain_conf.h   |  3 ---
 src/libvirt_private.syms |  1 -
 src/qemu/qemu_driver.c   |  6 +-
 4 files changed, 1 insertion(+), 29 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 05fa3f9..cb0df3d 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -10966,26 +10966,6 @@ virDomainVcpuPinDefParseXML(xmlNodePtr node,
 goto cleanup;
 }
 
-/*
- * Return the vcpupin related with the vcpu id on SUCCESS, or
- * NULL on failure.
- */
-virDomainVcpuPinDefPtr
-virDomainLookupVcpuPin(virDomainDefPtr def,
-   int vcpuid)
-{
-size_t i;
-
-if (!def-cputune.vcpupin)
-return NULL;
-
-for (i = 0; i  def-cputune.nvcpupin; i++) {
-if (def-cputune.vcpupin[i]-vcpuid == vcpuid)
-return def-cputune.vcpupin[i];
-}
-
-return NULL;
-}
 
 int
 virDomainDefMaybeAddController(virDomainDefPtr def,
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 2ff0bc4..3426c48 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2671,9 +2671,6 @@ int virDomainObjListExport(virDomainObjListPtr doms,
virDomainObjListFilter filter,
unsigned int flags);
 
-virDomainVcpuPinDefPtr virDomainLookupVcpuPin(virDomainDefPtr def,
-  int vcpuid);
-
 int
 virDomainDefMaybeAddController(virDomainDefPtr def,
int type,
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 0e81f2f..972b184 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -292,7 +292,6 @@ virDomainLifecycleTypeToString;
 virDomainLiveConfigHelperMethod;
 virDomainLockFailureTypeFromString;
 virDomainLockFailureTypeToString;
-virDomainLookupVcpuPin;
 virDomainMemballoonModelTypeFromString;
 virDomainMemballoonModelTypeToString;
 virDomainMemDumpTypeFromString;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 5970585..3fbaa62 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4120,8 +4120,6 @@ static int qemuDomainHotplugVcpus(virQEMUDriverPtr driver,
 }
 } else {
 for (i = oldvcpus - 1; i = nvcpus; i--) {
-virDomainVcpuPinDefPtr vcpupin = NULL;
-
 if (priv-cgroup) {
 if (virCgroupNewVcpu(priv-cgroup, i, false, cgroup_vcpu)  0)
 goto cleanup;
@@ -4132,9 +4130,7 @@ static int qemuDomainHotplugVcpus(virQEMUDriverPtr driver,
 }
 
 /* Free vcpupin setting */
-if ((vcpupin = virDomainLookupVcpuPin(vm-def, i))) {
-VIR_FREE(vcpupin);
-}
+ignore_value(virDomainVcpuPinDel(vm-def, i));
 }
 }
 
-- 
1.8.3.2

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


[libvirt] [PATCH 2/3] Make virDomainVcpuPinDel return void

2014-04-22 Thread Ján Tomko
Before, it only returned -1 on failure to shrink the array.
Since the switch to VIR_DELETE_ELEMENT in commit 2133441,
it returns either 0 or 0.
---
 src/conf/domain_conf.c   | 11 ++-
 src/conf/domain_conf.h   |  2 +-
 src/libxl/libxl_driver.c |  7 +--
 src/qemu/qemu_driver.c   | 16 +++-
 4 files changed, 7 insertions(+), 29 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index cb0df3d..57eb215 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -14510,27 +14510,20 @@ virDomainVcpuPinAdd(virDomainVcpuPinDefPtr 
**vcpupin_list,
 return -1;
 }
 
-int
+void
 virDomainVcpuPinDel(virDomainDefPtr def, int vcpu)
 {
 int n;
 virDomainVcpuPinDefPtr *vcpupin_list = def-cputune.vcpupin;
 
-/* No vcpupin exists yet */
-if (!def-cputune.nvcpupin) {
-return 0;
-}
-
 for (n = 0; n  def-cputune.nvcpupin; n++) {
 if (vcpupin_list[n]-vcpuid == vcpu) {
 virBitmapFree(vcpupin_list[n]-cpumask);
 VIR_FREE(vcpupin_list[n]);
 VIR_DELETE_ELEMENT(vcpupin_list, n, def-cputune.nvcpupin);
-break;
+return;
 }
 }
-
-return 0;
 }
 
 int
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 3426c48..c1cc854 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2300,7 +2300,7 @@ int virDomainVcpuPinAdd(virDomainVcpuPinDefPtr 
**vcpupin_list,
 int maplen,
 int vcpu);
 
-int virDomainVcpuPinDel(virDomainDefPtr def, int vcpu);
+void virDomainVcpuPinDel(virDomainDefPtr def, int vcpu);
 
 int virDomainEmulatorPinAdd(virDomainDefPtr def,
   unsigned char *cpumap,
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index b3f8df6..a6ae8a1 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -1945,12 +1945,7 @@ libxlDomainPinVcpuFlags(virDomainPtr dom, unsigned int 
vcpu,
 
 /* full bitmap means reset the settings (if any). */
 if (virBitmapIsAllSet(pcpumap)) {
-if (virDomainVcpuPinDel(targetDef, vcpu)  0) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
-   _(Failed to delete vcpupin xml for vcpu '%d'),
-   vcpu);
-goto endjob;
-}
+virDomainVcpuPinDel(targetDef, vcpu);
 goto done;
 }
 
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 3fbaa62..6996b80 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4130,7 +4130,7 @@ static int qemuDomainHotplugVcpus(virQEMUDriverPtr driver,
 }
 
 /* Free vcpupin setting */
-ignore_value(virDomainVcpuPinDel(vm-def, i));
+virDomainVcpuPinDel(vm-def, i);
 }
 }
 
@@ -4423,12 +4423,7 @@ qemuDomainPinVcpuFlags(virDomainPtr dom,
 }
 
 if (doReset) {
-if (virDomainVcpuPinDel(vm-def, vcpu)  0) {
-virReportError(VIR_ERR_INTERNAL_ERROR, %s,
-   _(failed to delete vcpupin xml of 
- a running domain));
-goto cleanup;
-}
+virDomainVcpuPinDel(vm-def, vcpu);
 } else {
 if (vm-def-cputune.vcpupin)
 virDomainVcpuPinDefArrayFree(vm-def-cputune.vcpupin, 
vm-def-cputune.nvcpupin);
@@ -4448,12 +4443,7 @@ qemuDomainPinVcpuFlags(virDomainPtr dom,
 if (flags  VIR_DOMAIN_AFFECT_CONFIG) {
 
 if (doReset) {
-if (virDomainVcpuPinDel(persistentDef, vcpu)  0) {
-virReportError(VIR_ERR_INTERNAL_ERROR, %s,
-   _(failed to delete vcpupin xml of 
- a persistent domain));
-goto cleanup;
-}
+virDomainVcpuPinDel(persistentDef, vcpu);
 } else {
 if (!persistentDef-cputune.vcpupin) {
 if (VIR_ALLOC(persistentDef-cputune.vcpupin)  0)
-- 
1.8.3.2

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


Re: [libvirt] [PATCH 01/18] qemu: unexport qemuDiskChainCheckBroken

2014-04-22 Thread Eric Blake
On 04/20/2014 04:13 PM, Peter Krempa wrote:
 The function isn't used in any other source file. Move it so that it
 doesn't need a declaration.
 ---
  src/qemu/qemu_domain.c | 44 ++--
  src/qemu/qemu_domain.h |  2 --
  2 files changed, 22 insertions(+), 24 deletions(-)

ACK.

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



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

Re: [libvirt] [PATCH 02/18] util: storagefile: Always store raw backing name in the metadata

2014-04-22 Thread Eric Blake
On 04/20/2014 04:13 PM, Peter Krempa wrote:
 Don't use the backingStoreRaw as a indication of broken chains. Fill it
 always and tweak the broken image chain detector to avoid changing the
 semantics.
 
 The new semantics to detect a broken chain is the presence of string in
 backingStoreRaw but the lack of the backing chain metadata structure in
 the chain.

Yay - you were able to take this where I had been aiming to go before I
got interrupted by real life (TM) last week :)

 
 Now that the raw backing store name is always filled there's no need to
 pass the raw name variable separately to fill in case the backing is not
 a file. Tweak the function so that it can handle a NULL in that case.
 ---
  src/util/virstoragefile.c | 51 
 +++
  1 file changed, 21 insertions(+), 30 deletions(-)
 

ACK

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



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

Re: [libvirt] [PATCH 03/18] util: storage: Don't force path canonicalization when loading metadata

2014-04-22 Thread Eric Blake
On 04/20/2014 04:13 PM, Peter Krempa wrote:
 Avoid breaking gluster volumes that don't have local representation. Use
 the provided name when canonicalization fails. Broken by commit 79f11b35
 
 Fixes:
  $ virsh pool-start glusterpool
  error: Failed to start pool glusterpool
  error: unable to resolve 'asdf': No such file or directory
 ---
  src/util/virstoragefile.c | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)

I'd feel a bit better if we had testsuite coverage for this (since my
broken commit still managed to pass 'make check', it shows our testsuite
has a hole).

 
 diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
 index 5fbb6e7..c707200 100644
 --- a/src/util/virstoragefile.c
 +++ b/src/util/virstoragefile.c
 @@ -1012,10 +1012,12 @@ virStorageFileGetMetadataFromBuf(const char *path,
  virStorageFileMetadataPtr ret = NULL;
  char *canonPath;
 
 -if (!(canonPath = canonicalize_file_name(path))) {
 +if (!(canonPath = canonicalize_file_name(path)) 
 +VIR_STRDUP(canonPath, path)  0) {
  virReportSystemError(errno, _(unable to resolve '%s'), path);
  return NULL;
  }

I'm not sure if I like this.  We are blindly trying to resolve 'path' as
if it were a local file name, and then if it failed, use 'path' as-is in
case it was a remote name.  I think what we should really be doing is:

if (virStorageIsFile(path)) {
if (!(canonPath = canonicalize_file_name(path))) {
error;
}
} else {
if (VIR_STRDUP(canonPath, path)  0) {
error;
}
}

In other words, the bug is that we are trying to canonicalize a non-file
name (which has a chance for false positives if the file system contains
an odd file name), rather than reserving the canonicalization to happen
ONLY when we know it is not a network protocol name.

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



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

Re: [libvirt] [PATCH 2/3] Make virDomainVcpuPinDel return void

2014-04-22 Thread Peter Krempa
On 04/22/14 14:52, Ján Tomko wrote:
 Before, it only returned -1 on failure to shrink the array.
 Since the switch to VIR_DELETE_ELEMENT in commit 2133441,
 it returns either 0 or 0.
 ---
  src/conf/domain_conf.c   | 11 ++-
  src/conf/domain_conf.h   |  2 +-
  src/libxl/libxl_driver.c |  7 +--
  src/qemu/qemu_driver.c   | 16 +++-
  4 files changed, 7 insertions(+), 29 deletions(-)
 

ACK,

Peter




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

Re: [libvirt] [PATCH 3/3] Fix error for out of range vcpu in qemuDomainPinVcpuFlags

2014-04-22 Thread Peter Krempa
On 04/22/14 14:52, Ján Tomko wrote:
 Changes:
 error: invalid argument: vcpu number out of range 2  2
 to slightly less confusing:
 error: invalid argument: vcpu number out of range 2  1
 ---
  src/qemu/qemu_driver.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
 

ACK,

Peter




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

Re: [libvirt] [PATCH 1/3] Properly free vcpupin info for unplugged CPUs

2014-04-22 Thread Peter Krempa
On 04/22/14 14:52, Ján Tomko wrote:
 Remove the pointer from def-cputune.vcpupin after unplugging
 the CPU and also free the bitmap contained in the structure
 by calling virDomainVcpuPinDel instead of VIR_FREE.
 
 Introduced by commit 0df1a79.
 
 This makes virDomainLookupVcpuPin redundant.
 
 https://bugzilla.redhat.com/show_bug.cgi?id=1088165
 ---
  src/conf/domain_conf.c   | 20 
  src/conf/domain_conf.h   |  3 ---
  src/libvirt_private.syms |  1 -
  src/qemu/qemu_driver.c   |  6 +-
  4 files changed, 1 insertion(+), 29 deletions(-)
 

ACK,

Peter




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

Re: [libvirt] [PATCH] nodeinfo: fix detection of physical memory on uclibc/musl libc

2014-04-22 Thread Daniel P. Berrange
On Tue, Apr 15, 2014 at 04:50:08PM +0200, Natanael Copa wrote:
 On Tue, 15 Apr 2014 12:30:47 +0100
 Daniel P. Berrange berra...@redhat.com wrote:
  
  The whole point of physmem_total is that libvirt can avoid having to
  have a bunch of different implementations. Any kind of fix that libvirt
  could do, can equally be done in the physmem_total, so there's no reason
  to not rely on physmem_total. So we only need to fix physmem_total, and
  the musl libc.
  
  The only reason to fix libvirt, is if there's some problem that would
  prevent us getting the fix into gnulib in an acceptable timeframe.
 
 Fair enough.
 
 I posted a bug report and a patch to bugs-gnulib.

Thanks for making the effort / taking the time to fix gnulib.

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

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


Re: [libvirt] [PATCH perl-Sys-Virt] Don't die if snapshots are unsupported

2014-04-22 Thread Mike Latimer
On Tuesday, April 22, 2014 11:35:31 AM Daniel P. Berrange wrote:
 Unfortunately this is a change in API behaviour that affects more
 than just the TCK. You need todo the equivalent change to the TCK
 code instead really.

Ok. I didn't really view this as a change to the API, but I suppose it is. 
I'll implement this at the TCK layer and submit a new patch.

Thanks,
Mike

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


Re: [libvirt] [PATCH v5 1/5] Introduce virDomainFSFreeze() and virDomainFSThaw() public API

2014-04-22 Thread Daniel P. Berrange
On Thu, Apr 03, 2014 at 11:39:29AM -0400, Tomoki Sekiyama wrote:
 +
 +/**
 + * virDomainFSFreeze:
 + * @dom: a domain object
 + * @disks: list of disk names to be frozen
 + * @ndisks: the number of disks specified in @disks

I realize this current patch series doesn't use the @disks parameter
at all, but what exactly are we expecting to be passed here ? Is
this a list of filesystem paths from the guest OS pov, or is it a
list of disks targets from libvirt's POV ? I'm guessing the former
since this is expected to be passed to the guest agent.

 + * @flags: extra flags, not used yet, so callers should always pass 0
 + *
 + * Freeze filesystems on the specified disks within the guest (hence guest
 + * agent may be required depending on hypervisor used). If @ndisks is 0,
 + * every mounted filesystem on the guest is frozen.
 + * Freeze can be nested. When it is called on the same disk multiple times,
 + * the disk will not be thawed until virDomainFSThaw is called the same 
 times.

I'm not entirely convinced we should allow nesting. I think it would
be better to report an error if the user tries to freeze a disk that
is already frozen, or tries to thaw a disks that is not frozen. Nesting
makes it harder for applications to know just what state the guest is
in.

eg, consider that they need to run a command in the guest agent that
requires the guest FS to be un-thawed. If we allow nesting, then after
the app runs virDomainFSThaw, the app can't tell whether or not all
thes path are un-thawed or not. 

IMHO we should forbid nesting.

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

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


[libvirt] [PATCH] libxl: support ACPI shutdown flag

2014-04-22 Thread Jim Fehlig
Add support for VIR_DOMAIN_SHUTDOWN_ACPI_POWER_BTN flag in
libxlDomainShutdownFlags().  Inspired by similar functionality
in the Xen xl client.

Signed-off-by: Jim Fehlig jfeh...@suse.com
---

I considered invoking libxl_send_trigger() immediately when
VIR_DOMAIN_SHUTDOWN_ACPI_POWER_BTN flag is specified, but in the
end decided to only honor the flag if a normal shutdown request
failed.  This behavior is similar to xl and conforms to the
virDomainShutdownFlags() docs

If @flags is set to zero, then the hypervisor will choose the method
of shutdown it considers best. To have greater control pass one or
more of the virDomainShutdownFlagValues. The order in which the
hypervisor tries each shutdown method is undefined, and a hypervisor
is not required to support all methods.

I'm certainly receptive to only invoking libxl_send_trigger() when
VIR_DOMAIN_SHUTDOWN_ACPI_POWER_BTN is specified if folks think that
is a better approach.

 src/libxl/libxl_driver.c | 26 ++
 1 file changed, 22 insertions(+), 4 deletions(-)

diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index b3f8df6..2b30c08 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -868,7 +868,7 @@ libxlDomainShutdownFlags(virDomainPtr dom, unsigned int 
flags)
 int ret = -1;
 libxlDomainObjPrivatePtr priv;
 
-virCheckFlags(0, -1);
+virCheckFlags(VIR_DOMAIN_SHUTDOWN_ACPI_POWER_BTN, -1);
 
 if (!(vm = libxlDomObjFromDomain(dom)))
 goto cleanup;
@@ -883,17 +883,35 @@ libxlDomainShutdownFlags(virDomainPtr dom, unsigned int 
flags)
 }
 
 priv = vm-privateData;
-if (libxl_domain_shutdown(priv-ctx, vm-def-id) != 0) {
+ret = libxl_domain_shutdown(priv-ctx, vm-def-id);
+if (ret == ERROR_NOPARAVIRT) {
+if (flags  VIR_DOMAIN_SHUTDOWN_ACPI_POWER_BTN) {
+VIR_DEBUG(PV control interface not available, 
+  sending ACPI power button event.);
+ret = libxl_send_trigger(priv-ctx, vm-def-id,
+ LIBXL_TRIGGER_POWER, 0);
+} else {
+virReportError(VIR_ERR_OPERATION_INVALID,
+   %s, _(PV control interface not available, 
+   preventing external graceful shutdown. 
+   Consider using an ACPI power event.));
+ret = -1;
+goto cleanup;
+}
+}
+
+if (ret != 0) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
_(Failed to shutdown domain '%d' with libxenlight),
vm-def-id);
+ret = -1;
 goto cleanup;
 }
 
-/* vm is marked shutoff (or removed from domains list if not persistent)
+/* ret == 0 == successful shutdown request
+ * vm is marked shutoff (or removed from domains list if not persistent)
  * in shutdown event handler.
  */
-ret = 0;
 
  cleanup:
 if (vm)
-- 
1.8.1.4

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


[libvirt] [PATCH] bhyve: implement domainGetCPUStats

2014-04-22 Thread Roman Bogorodskiy
For per CPU stats, implement virBhyveGetDomainPercpuStats() that
uses bhyvectl tool to obtain the guest's vcpu stats.

For total CPU stats, add virBhyveGetDomainTotalCpuStats() that
gets the hypervisor process CPU stats using kvm (kernel
memory interface).
---
 configure.ac  |   7 +++
 src/bhyve/bhyve_driver.c  |  38 +
 src/bhyve/bhyve_process.c | 136 ++
 src/bhyve/bhyve_process.h |  10 
 4 files changed, 191 insertions(+)

diff --git a/configure.ac b/configure.ac
index ea85851..dbfada2 100644
--- a/configure.ac
+++ b/configure.ac
@@ -2678,6 +2678,13 @@ AC_CHECK_DECLS([cpuset_getaffinity],
 #include sys/cpuset.h
])
 
+# Check for BSD kvm (kernel memory interface)
+if test $with_freebsd = yes; then
+ AC_CHECK_LIB([kvm], [kvm_getprocs], [],
+  [AC_MSG_ERROR([BSD kernel memory interface library is 
required to build on FreeBSD])]
+ )
+fi
+
 # Check if we need to look for ifconfig
 if test $want_ifconfig = yes; then
  AC_PATH_PROG([IFCONFIG_PATH], [ifconfig])
diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c
index 0cafe4c..25b0dce 100644
--- a/src/bhyve/bhyve_driver.c
+++ b/src/bhyve/bhyve_driver.c
@@ -930,6 +930,43 @@ bhyveDomainGetMetadata(virDomainPtr dom,
 }
 
 static int
+bhyveDomainGetCPUStats(virDomainPtr dom,
+   virTypedParameterPtr params ATTRIBUTE_UNUSED,
+   unsigned int nparams,
+   int start_cpu,
+   unsigned int ncpus,
+   unsigned int flags)
+{
+virDomainObjPtr vm = NULL;
+int ret = -1;
+
+virCheckFlags(VIR_TYPED_PARAM_STRING_OKAY, -1);
+
+if (!(vm = bhyveDomObjFromDomain(dom)))
+return ret;
+
+if (virDomainGetCPUStatsEnsureACL(dom-conn, vm-def)  0)
+goto cleanup;
+
+if (!virDomainObjIsActive(vm)) {
+virReportError(VIR_ERR_OPERATION_INVALID, %s,
+   _(domain is not running));
+goto cleanup;
+}
+
+if (start_cpu == -1)
+ret = virBhyveGetDomainTotalCpuStats(vm, params, nparams);
+else
+ret = virBhyveGetDomainPercpuStats(vm, params, nparams,
+   start_cpu, ncpus);
+
+ cleanup:
+if (vm)
+virObjectUnlock(vm);
+return ret;
+}
+
+static int
 bhyveNodeGetCPUStats(virConnectPtr conn,
  int cpuNum,
  virNodeCPUStatsPtr params,
@@ -1198,6 +1235,7 @@ static virDriver bhyveDriver = {
 .domainOpenConsole = bhyveDomainOpenConsole, /* 1.2.4 */
 .domainSetMetadata = bhyveDomainSetMetadata, /* 1.2.4 */
 .domainGetMetadata = bhyveDomainGetMetadata, /* 1.2.4 */
+.domainGetCPUStats = bhyveDomainGetCPUStats, /* 1.2.4 */
 .nodeGetCPUStats = bhyveNodeGetCPUStats, /* 1.2.2 */
 .nodeGetMemoryStats = bhyveNodeGetMemoryStats, /* 1.2.2 */
 .nodeGetInfo = bhyveNodeGetInfo, /* 1.2.3 */
diff --git a/src/bhyve/bhyve_process.c b/src/bhyve/bhyve_process.c
index a557bc5..e1f4324 100644
--- a/src/bhyve/bhyve_process.c
+++ b/src/bhyve/bhyve_process.c
@@ -22,7 +22,11 @@
 #include config.h
 
 #include fcntl.h
+#include kvm.h
+#include sys/param.h
 #include sys/types.h
+#include sys/sysctl.h
+#include sys/user.h
 #include sys/ioctl.h
 #include net/if.h
 #include net/if_tap.h
@@ -41,11 +45,15 @@
 #include virnetdev.h
 #include virnetdevbridge.h
 #include virnetdevtap.h
+#include virtypedparam.h
 
 #define VIR_FROM_THIS  VIR_FROM_BHYVE
 
 VIR_LOG_INIT(bhyve.bhyve_process);
 
+#define BHYVE_TOTAL_CPU_STAT_PARAM 1
+#define BHYVE_PER_CPU_STAT_PARAM 1
+
 static virDomainObjPtr
 bhyveProcessAutoDestroy(virDomainObjPtr vm,
 virConnectPtr conn ATTRIBUTE_UNUSED,
@@ -246,3 +254,131 @@ virBhyveProcessStop(bhyveConnPtr driver,
 virCommandFree(cmd);
 return ret;
 }
+
+static int
+virBhyveExtractCpuTotal(char **const groups,
+void *opaque)
+{
+return virStrToLong_ull(groups[0], NULL, 10, opaque);
+}
+
+int
+virBhyveGetDomainTotalCpuStats(virDomainObjPtr vm,
+   virTypedParameterPtr params ATTRIBUTE_UNUSED,
+   unsigned int nparams)
+{
+struct kinfo_proc* kp;
+kvm_t* kd;
+char errbuf[_POSIX2_LINE_MAX];
+int nprocs;
+int ret = -1;
+
+if (nparams == 0)
+return BHYVE_TOTAL_CPU_STAT_PARAM;
+
+if ((kd = kvm_openfiles(NULL, NULL, NULL, O_RDONLY, errbuf)) == NULL) {
+virReportError(VIR_ERR_SYSTEM_ERROR,
+   _(Unable to get kvm descriptor: %s),
+   errbuf);
+return -1;
+
+}
+
+kp = kvm_getprocs(kd, KERN_PROC_PID, vm-pid, nprocs);
+if (kp == NULL || nprocs != 1) {
+virReportError(VIR_ERR_SYSTEM_ERROR,
+   _(Unable to obtain information about pid: %d),
+   (int)vm-pid);
+goto cleanup;
+}
+
+if 

Re: [libvirt] [PATCH] bhyve: implement domainGetCPUStats

2014-04-22 Thread Roman Bogorodskiy
  Roman Bogorodskiy wrote:

 For per CPU stats, implement virBhyveGetDomainPercpuStats() that
 uses bhyvectl tool to obtain the guest's vcpu stats.
 
 For total CPU stats, add virBhyveGetDomainTotalCpuStats() that
 gets the hypervisor process CPU stats using kvm (kernel
 memory interface).
 ---
  configure.ac  |   7 +++
  src/bhyve/bhyve_driver.c  |  38 +
  src/bhyve/bhyve_process.c | 136 
 ++
  src/bhyve/bhyve_process.h |  10 
  4 files changed, 191 insertions(+)
 diff --git a/src/bhyve/bhyve_process.c b/src/bhyve/bhyve_process.c
 index a557bc5..e1f4324 100644
 --- a/src/bhyve/bhyve_process.c
 +++ b/src/bhyve/bhyve_process.c

...

 +int
 +virBhyveGetDomainTotalCpuStats(virDomainObjPtr vm,
 +   virTypedParameterPtr params ATTRIBUTE_UNUSED,
 +   unsigned int nparams)
 +{
 +struct kinfo_proc* kp;
 +kvm_t* kd;

Just noticed that it should be:

struct kinfo_proc *kp;
kvm_t *kd;

I will not re-send the patch just because of that and will include the
fix in the next version.

 +char errbuf[_POSIX2_LINE_MAX];
 +int nprocs;
 +int ret = -1;

Roman Bogorodskiy


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

Re: [libvirt] [PATCH v5 1/5] Introduce virDomainFSFreeze() and virDomainFSThaw() public API

2014-04-22 Thread Tomoki Sekiyama
Hi Daniel,
thanks for your comment.

On 4/22/14 11:39 , Daniel P. Berrange berra...@redhat.com wrote:
On Thu, Apr 03, 2014 at 11:39:29AM -0400, Tomoki Sekiyama wrote:
 +
 +/**
 + * virDomainFSFreeze:
 + * @dom: a domain object
 + * @disks: list of disk names to be frozen
 + * @ndisks: the number of disks specified in @disks

I realize this current patch series doesn't use the @disks parameter
at all, but what exactly are we expecting to be passed here ? Is
this a list of filesystem paths from the guest OS pov, or is it a
list of disks targets from libvirt's POV ? I'm guessing the former
since this is expected to be passed to the guest agent.

My intention for 'disks' is latter, a list of disks targets from
libvirt's POV.
Currently it is just a placeholder of API. It would be passed to the
agent after it is converted into a list of device addresses (e.g. a pair
of drive address and controller's PCI address) so that the agent can
look up filesystems on the specified disks.


 + * @flags: extra flags, not used yet, so callers should always pass 0
 + *
 + * Freeze filesystems on the specified disks within the guest (hence
guest
 + * agent may be required depending on hypervisor used). If @ndisks is
0,
 + * every mounted filesystem on the guest is frozen.
 + * Freeze can be nested. When it is called on the same disk multiple
times,
 + * the disk will not be thawed until virDomainFSThaw is called the
same times.

I'm not entirely convinced we should allow nesting. I think it would
be better to report an error if the user tries to freeze a disk that
is already frozen, or tries to thaw a disks that is not frozen. Nesting
makes it harder for applications to know just what state the guest is
in.

eg, consider that they need to run a command in the guest agent that
requires the guest FS to be un-thawed. If we allow nesting, then after
the app runs virDomainFSThaw, the app can't tell whether or not all
thes path are un-thawed or not.

IMHO we should forbid nesting.

Nesting was originally advised by Eric for the reason of scalability.
But if it is not actually wanted by apps, I think we can remove nesting
from API design. (I personally don't know apps that run parallel
FSFreeze operations for multiple disks.)

From the implementation view point, nesting may be difficult to
supported in qemu guest agent, because some guest operating systems such
as Windows cannot start new FSFreeze while some of filesystems are frozen.

Regards,
Tomoki Sekiyama


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


Re: [libvirt] [PATCH 04/18] util: storage: Remove obsolete argument virStorageFileGetMetadataInternal

2014-04-22 Thread Eric Blake
On 04/20/2014 04:13 PM, Peter Krempa wrote:
 As we already pass the whole structure down the call path there's no
 need to return some stuff in a separate argument. Remove the argument
 and call tweakers to avoid breaking semantics.

s/call tweakers/tweak callers/

 
 virStorageFileGetMetadataFromBuf will be refactored later along with the
 storage driver.
 ---
  src/util/virstoragefile.c | 13 ++---
  1 file changed, 6 insertions(+), 7 deletions(-)
 

ACK

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



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

Re: [libvirt] [PATCH 03/18] util: storage: Don't force path canonicalization when loading metadata

2014-04-22 Thread Eric Blake
On 04/22/2014 07:34 AM, Eric Blake wrote:
 On 04/20/2014 04:13 PM, Peter Krempa wrote:
 Avoid breaking gluster volumes that don't have local representation. Use
 the provided name when canonicalization fails. Broken by commit 79f11b35

 Fixes:
  $ virsh pool-start glusterpool
  error: Failed to start pool glusterpool
  error: unable to resolve 'asdf': No such file or directory
 ---
  src/util/virstoragefile.c | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)
 
 I'd feel a bit better if we had testsuite coverage for this (since my
 broken commit still managed to pass 'make check', it shows our testsuite
 has a hole).
 

 -if (!(canonPath = canonicalize_file_name(path))) {
 +if (!(canonPath = canonicalize_file_name(path)) 
 +VIR_STRDUP(canonPath, path)  0) {
  virReportSystemError(errno, _(unable to resolve '%s'), path);
  return NULL;
  }
 
 I'm not sure if I like this.  We are blindly trying to resolve 'path' as
 if it were a local file name, and then if it failed, use 'path' as-is in
 case it was a remote name.  I think what we should really be doing is:

I'd still like the test improvements, but those can be separate patches
as long as they are in by the time gluster interaction is fully
integrated.  So for this patch, I can live with ACK if you squash this in:

diff --git i/src/util/virstoragefile.c w/src/util/virstoragefile.c
index c707200..6ea45a4 100644
--- i/src/util/virstoragefile.c
+++ w/src/util/virstoragefile.c
@@ -1012,9 +1012,12 @@ virStorageFileGetMetadataFromBuf(const char *path,
 virStorageFileMetadataPtr ret = NULL;
 char *canonPath;

-if (!(canonPath = canonicalize_file_name(path)) 
-VIR_STRDUP(canonPath, path)  0) {
-virReportSystemError(errno, _(unable to resolve '%s'), path);
+if (virStorageIsFile(path)) {
+if (!(canonPath = canonicalize_file_name(path))) {
+virReportSystemError(errno, _(unable to resolve '%s'), path);
+return NULL;
+}
+} else if (VIR_STRDUP(canonPath, path)  0) {
 return NULL;
 }

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



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

Re: [libvirt] [PATCH 04/18] util: storage: Remove obsolete argument virStorageFileGetMetadataInternal

2014-04-22 Thread Eric Blake
On 04/22/2014 02:42 PM, Eric Blake wrote:
 On 04/20/2014 04:13 PM, Peter Krempa wrote:
 As we already pass the whole structure down the call path there's no
 need to return some stuff in a separate argument. Remove the argument
 and call tweakers to avoid breaking semantics.
 
 s/call tweakers/tweak callers/
 

 virStorageFileGetMetadataFromBuf will be refactored later along with the
 storage driver.
 ---
  src/util/virstoragefile.c | 13 ++---
  1 file changed, 6 insertions(+), 7 deletions(-)

 
 ACK

Oops, spoke too early: please squash this in:

diff --git i/src/util/virstoragefile.c w/src/util/virstoragefile.c
index 8a5bb01..cb22255 100644
--- i/src/util/virstoragefile.c
+++ w/src/util/virstoragefile.c
@@ -795,7 +795,7 @@ qcow2GetFeatures(virBitmapPtr *features,
  * information about the file and its backing store.  */
 static int ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2)
 ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4) ATTRIBUTE_NONNULL(7)
-ATTRIBUTE_NONNULL(8) ATTRIBUTE_NONNULL(9)
+ATTRIBUTE_NONNULL(8)
 virStorageFileGetMetadataInternal(const char *path,
   const char *canonPath,
   const char *directory,


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



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

Re: [libvirt] [PATCH 05/18] util: storage: Move checking of the actual backing image to the worker

2014-04-22 Thread Eric Blake
On 04/20/2014 04:13 PM, Peter Krempa wrote:
 Move the code checking the presence of the backing file to the recursive
 worker function instead of the metadata parser. The recursive worker
 will later be changed to parse more than just local files and this
 change will help the separation.
 ---
  src/util/virstoragefile.c | 63 
 ---
  1 file changed, 27 insertions(+), 36 deletions(-)
 
 diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
 index 513f15d..a005e00 100644
 --- a/src/util/virstoragefile.c
 +++ b/src/util/virstoragefile.c
 @@ -795,7 +795,7 @@ qcow2GetFeatures(virBitmapPtr *features,
   * information about the file and its backing store.  */
  static int ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2)
  ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4) ATTRIBUTE_NONNULL(7)
 -ATTRIBUTE_NONNULL(8) ATTRIBUTE_NONNULL(9)
 +ATTRIBUTE_NONNULL(8)

This hunk belongs in 4/18.


 @@ -1192,7 +1164,26 @@ virStorageFileGetMetadataRecurse(const char *path, 
 const char *canonPath,
  ret = 0;
  }
 
 -if (ret == 0  meta-backingStore) {
 +if (ret == 0  meta-backingStoreRaw) {
 +if (virStorageIsFile(meta-backingStoreRaw)) {
 +if (virFindBackingFile(directory,
 +   meta-backingStoreRaw,
 +   backingDirectory,
 +   meta-backingStore)  0) {
 +/* the backing file is (currently) unavailable, treat this
 + * file as standalone:
 + * backingStoreRaw is kept to mark broken image chains */
 +VIR_WARN(Backing file '%s' of image '%s' is missing.,
 + meta-backingStoreRaw, path);
 +
 +return 0;
 +}
 +} else {
 +if (VIR_STRDUP(meta-backingStore, meta-backingStoreRaw)  0)
 +return -1;
 +}
 +
 +
  virStorageFileMetadataPtr backing;

This puts a declaration after statement; if you want, you could squash
this in.  Either way, ACK.

diff --git i/src/util/virstoragefile.c w/src/util/virstoragefile.c
index 6f0fb79..82b5c65 100644
--- i/src/util/virstoragefile.c
+++ w/src/util/virstoragefile.c
@@ -1168,6 +1168,8 @@ virStorageFileGetMetadataRecurse(const char *path,
const char *canonPath,
 }

 if (ret == 0  meta-backingStoreRaw) {
+virStorageFileMetadataPtr backing;
+
 if (virStorageIsFile(meta-backingStoreRaw)) {
 if (virFindBackingFile(directory,
meta-backingStoreRaw,
@@ -1186,9 +1188,6 @@ virStorageFileGetMetadataRecurse(const char *path,
const char *canonPath,
 return -1;
 }

-
-virStorageFileMetadataPtr backing;
-
 if (backingFormat == VIR_STORAGE_FILE_AUTO  !allow_probe)
 backingFormat = VIR_STORAGE_FILE_RAW;
 else if (backingFormat == VIR_STORAGE_FILE_AUTO_SAFE)


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



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

Re: [libvirt] [PATCH 06/18] storage: util: Clean up arguments of virStorageFileGetMetadataInternal

2014-04-22 Thread Eric Blake
On 04/20/2014 04:13 PM, Peter Krempa wrote:
 Avoid passing lot of arguments into guts of metadata retrieval to fill
 the actual structure. Temporarily fill the structure before passing it
 down to the actual metadata extractor.
 
 This will later help the inversion of the steps taken to extract the
 metadata so that this function can be fully converted to
 virStorageSource as the data struct.
 ---
  src/util/virstoragefile.c | 164 
 +++---
  1 file changed, 81 insertions(+), 83 deletions(-)
 

 -/* Given a header in BUF with length LEN, as parsed from the file with
 - * user-provided name PATH and opened from CANONPATH, and where any
 - * relative backing file will be opened from DIRECTORY, and assuming
 - * it has the given FORMAT, populate the newly-allocated META with
 - * information about the file and its backing store.  */
 +/* Given a header in BUF with length LEN, as parsed from the storage file
 + * assuming it has the given FORMAT, populate information into META
 + * with information about the file and its backing store. Return format
 + * of the backing store as BACKING_FORMAT. PATH and FORMAT have to be
 + * pre-populated in META*/

Cosmetic - I usually stick space before */

  {
  int ret = -1;
 
 -VIR_DEBUG(path=%s, canonPath=%s, dir=%s, buf=%p, len=%zu, format=%d,
 -  path, canonPath, directory, buf, len, format);
 +VIR_DEBUG(path=%s, buf=%p, len=%zu, meta-format=%d,
 +  meta-path, buf, len, meta-format);

The diff would be slightly smaller if you do:

int format = meta-format;

up front, then keep the original use of format everywhere else...

 
 -if (VIR_STRDUP(meta-path, path)  0)
 -goto cleanup;
 -if (VIR_STRDUP(meta-canonPath, canonPath)  0)
 -goto cleanup;
 -if (VIR_STRDUP(meta-relDir, directory)  0)
 -goto cleanup;
 +if (meta-format == VIR_STORAGE_FILE_AUTO)
 +meta-format = virStorageFileProbeFormatFromBuf(meta-path, buf, 
 len);

...well, right here, you'd have to do 'format = meta-format =
virStorage...();', but the rest of the function has fewer changes.  But
it doesn't matter to me; I'm fine keeping it as you wrote it.

 +static virStorageFileMetadataPtr
 +virStorageFileMetadataNew(const char *path,
 +  int format)
 +{
 +virStorageFileMetadataPtr ret = NULL;
 +
 +if (VIR_ALLOC(ret)  0)
 +return NULL;
 +
 +ret-format = format;
 +ret-type = VIR_STORAGE_TYPE_FILE;
 +
 +if (VIR_STRDUP(ret-path, path)  0)
 +goto error;
 +
 +if (!(ret-canonPath = canonicalize_file_name(path))) {

Same complaint as in 3/18 - you should only call
canonicalize_file_name() if you KNOW that path should be interpreted as
a file name.

 @@ -979,19 +994,11 @@ virStorageFileGetMetadataFromBuf(const char *path,
   int *backingFormat)
  {
  virStorageFileMetadataPtr ret = NULL;
 -char *canonPath;
 
 -if (!(canonPath = canonicalize_file_name(path)) 
 -VIR_STRDUP(canonPath, path)  0) {
 -virReportSystemError(errno, _(unable to resolve '%s'), path);
 +if (!(ret = virStorageFileMetadataNew(path, format)))
  return NULL;
 -}

Minor merge conflicts with my suggested resolution to 3/18 - but should
be obvious resolution.

 @@ -1071,7 +1072,6 @@ virStorageFileGetMetadataFromFDInternal(const char 
 *path,
  return ret;
  }
 
 -
  /**

Spurious whitespace change.

ACK with this squashed in (and any additional trivial cleanups you want
to optionally do based on my comments above):

diff --git i/src/util/virstoragefile.c w/src/util/virstoragefile.c
index b0fe2ae..ff8de43 100644
--- i/src/util/virstoragefile.c
+++ w/src/util/virstoragefile.c
@@ -948,8 +948,12 @@ virStorageFileMetadataNew(const char *path,
 if (VIR_STRDUP(ret-path, path)  0)
 goto error;

-if (!(ret-canonPath = canonicalize_file_name(path))) {
-virReportSystemError(errno, _(unable to resolve '%s'), path);
+if (virStorageIsFile(path)) {
+if (!(ret-canonPath = canonicalize_file_name(path))) {
+virReportSystemError(errno, _(unable to resolve '%s'), path);
+goto error;
+}
+} else if (VIR_STRDUP(ret-canonPath, path)  0) {
 goto error;
 }

@@ -1072,6 +1076,7 @@
virStorageFileGetMetadataFromFDInternal(virStorageFileMetadataPtr meta,
 return ret;
 }

+
 /**
  * virStorageFileGetMetadataFromFD:
  *


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



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

Re: [libvirt] [PATCH 07/18] util: storage: Rename path to relPath in virStorageFileMetadata

2014-04-22 Thread Eric Blake
On 04/20/2014 04:13 PM, Peter Krempa wrote:
 To allow future change of virStorageFileMetadata to virStorageSource we
 need to store a full path in the path variable as rest of the code
 expects it to be a full path. Rename the path field to relPath to
 keep tracking the info but allowing a real path field.
 ---
  src/util/virstoragefile.c | 22 +++---
  src/util/virstoragefile.h |  2 +-
  tests/virstoragetest.c|  6 +++---
  3 files changed, 15 insertions(+), 15 deletions(-)

 @@ -945,7 +945,7 @@ virStorageFileMetadataNew(const char *path,
  ret-format = format;
  ret-type = VIR_STORAGE_TYPE_FILE;
 
 -if (VIR_STRDUP(ret-path, path)  0)
 +if (VIR_STRDUP(ret-relPath, path)  0)

Obvious merge conflict resolution here with my proposed fixup to 6/18.

Mechanical; ACK.

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



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

Re: [libvirt] [PATCH 08/18] util: storagefile: Rename canonPath to path in virStorageFileMetadata

2014-04-22 Thread Eric Blake
On 04/20/2014 04:13 PM, Peter Krempa wrote:
 As for the previous patch, this change is needed to achieve
 compatibility with all the existing code, where we expect a fully
 qualified path of local files to be present.
 ---
  src/util/virstoragefile.c | 18 +++
  src/util/virstoragefile.h |  2 +-
  tests/virstoragetest.c| 56 
 +++
  3 files changed, 38 insertions(+), 38 deletions(-)

 +++ b/src/util/virstoragefile.c
 @@ -948,7 +948,7 @@ virStorageFileMetadataNew(const char *path,
  if (VIR_STRDUP(ret-relPath, path)  0)
  goto error;
 
 -if (!(ret-canonPath = canonicalize_file_name(path))) {
 +if (!(ret-path = canonicalize_file_name(path))) {

Another merge conflict with my earlier squash-in requests.  Should be an
obvious fix.

Mechanical; ACK.


 +++ b/tests/virstoragetest.c
 @@ -237,7 +237,7 @@ struct _testFileData
  bool expEncrypted;
  const char *pathRel;
  const char *pathAbs;
 -const char *canonPath;
 +const char *path;

I'm not sure I would have renamed this field.  But I guess it doesn't hurt.

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



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

Re: [libvirt] [PATCH 09/18] util: virstoragefile: Don't use backingStore directly

2014-04-22 Thread Eric Blake
On 04/20/2014 04:13 PM, Peter Krempa wrote:
 As a temporary step to allow killing of the backingStore field of
 struct virStorageFileMetadata the recursive metadata retrieval function
 will be converted not to use the field in the lookup process.
 ---
  src/util/virstoragefile.c | 11 ---
  1 file changed, 8 insertions(+), 3 deletions(-)
 

 @@ -1177,10 +1178,12 @@ virStorageFileGetMetadataRecurse(const char *path, 
 const char *canonPath,
  return 0;
  }
  } else {
 -if (VIR_STRDUP(meta-backingStore, meta-backingStoreRaw)  0)
 +if (VIR_STRDUP(backingPath, meta-backingStoreRaw)  0)
  return -1;
  }
 
 +if (VIR_STRDUP(meta-backingStore, backingPath)  0)
 +return -1;
 
  virStorageFileMetadataPtr backing;

Merge conflict with my suggested changes to 4/18, should be easy enough
to fix.

ACK.

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



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

Re: [libvirt] [PATCH 10/18] virstoragefile: Kill backingStore field from virStorageFileMetadata

2014-04-22 Thread Eric Blake
On 04/20/2014 04:13 PM, Peter Krempa wrote:
 Remove the obsolete field replaced by data in path.
 
 The testsuite requires tweaking as the name of the backing file is now
 stored one layer deeper in the backking chain linked list.

s/backking/backing/

 ---
  src/conf/domain_conf.c| 13 ++--
  src/qemu/qemu_driver.c|  8 
  src/util/virstoragefile.c |  5 -
  src/util/virstoragefile.h |  5 -
  tests/virstoragetest.c| 50 
 +++
  5 files changed, 36 insertions(+), 45 deletions(-)
 
 diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
 index 05fa3f9..006aa96 100644
 --- a/src/conf/domain_conf.c
 +++ b/src/conf/domain_conf.c
 @@ -18565,16 +18565,17 @@ virDomainDiskDefForeachPath(virDomainDiskDefPtr 
 disk,
 
  if (iter(disk, path, 0, opaque)  0)
  goto cleanup;
 -
 -tmp = disk-backingChain;
 -while (tmp  virStorageIsFile(tmp-backingStore)) {
 -if (!ignoreOpenFailure  !tmp-backingMeta) {
 +/* XXX: temporarily we need to select the second element of the backing
 + * chain to start as the first is the copy of the disk itself. */
 +tmp = disk-backingChain ? disk-backingChain-backingMeta : NULL;
 +while (tmp  virStorageIsFile(tmp-path)) {

For that matter, if a future patch allows a file to be the backing store
of a network path, we don't want to stop the iteration just because
there was nothing to label on the intermediate network resource.  So
this isn't the last time we will be touching this function :)

 +++ b/src/util/virstoragefile.h
 @@ -148,11 +148,6 @@ struct _virStorageFileMetadata {
  unsigned long long capacity;
  virBitmapPtr features; /* bits described by enum virStorageFileFeature */
  char *compat;
 -
 -/* Fields I'm trying to delete, because it is confusing to have to
 - * query the parent metadata for details about the backing
 - * store.  */
 -char *backingStore; /* Canonical name (absolute file, or protocol). 
 Should be same as backingMeta-canonPath */
  };

Yay - you finished the efforts I started.  The fact that you were able
to pick up where I left off is reassuring - it means the design is
indeed progressing to something a bit more reasonable to understand, and
that I explained my goals fairly well.

ACK.

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



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

Re: [libvirt] [PATCH 11/18] util: storagefile: Add function to free a virStorageSourcePrt

2014-04-22 Thread Eric Blake
On 04/20/2014 04:13 PM, Peter Krempa wrote:

s/Prt/Ptr/ in the subject

 Add a free function as some parts of the code will allocate the
 structure.
 ---
  src/libvirt_private.syms  |  1 +
  src/util/virstoragefile.c | 11 +++
  src/util/virstoragefile.h |  1 +
  3 files changed, 13 insertions(+)

ACK with that fix.

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



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

Re: [libvirt] [PATCH 12/18] util: storagefile: Add fields from virStorageMetadata to virStorageSource

2014-04-22 Thread Eric Blake
On 04/20/2014 04:13 PM, Peter Krempa wrote:
 Add the required fields that are missing from the new structure that
 will allow us to switch the storage file metadata code entirely to the
 new structure.
 
 Add relPath and relDir and the raw backing store name. Also allow
 creating linked lists of virStorageSourcePtrs to express backing chains.
 ---
  src/util/virstoragefile.c |  7 +++
  src/util/virstoragefile.h | 14 ++
  2 files changed, 21 insertions(+)

ACK.

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



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

Re: [libvirt] [PATCH 13/18] maint: Switch over from struct virStorageFileMetadata to virStorageSource

2014-04-22 Thread Eric Blake
On 04/20/2014 04:13 PM, Peter Krempa wrote:
 Replace the old structure with the new one. This change is a trivial
 name change operation (along with change of the freeing function).
 ---
  src/conf/domain_conf.c|  4 +--
  src/conf/domain_conf.h|  2 +-
  src/qemu/qemu_domain.c|  2 +-
  src/qemu/qemu_driver.c| 16 +--
  src/storage/storage_backend_fs.c  |  4 +--
  src/storage/storage_backend_gluster.c |  4 +--
  src/util/virstoragefile.c | 54 
 +--
  src/util/virstoragefile.h | 32 ++---
  tests/virstoragetest.c| 20 ++---
  9 files changed, 69 insertions(+), 69 deletions(-)
 

 @@ -1182,7 +1182,7 @@ virStorageFileGetMetadataRecurse(const char *path, 
 const char *canonPath,
  return -1;
  }
 
 -virStorageFileMetadataPtr backing;
 +virStorageSourcePtr backing;
 

Merge conflict with my suggestions for 5/18; obvious resolution.

Mechanical; ACK.

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



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

Re: [libvirt] [PATCH 14/18] util: virstorage: Kill struct virStorageFileMetadata

2014-04-22 Thread Eric Blake
On 04/20/2014 04:13 PM, Peter Krempa wrote:
 Remove the now unused pieces of the structure.
 ---
  src/libvirt_private.syms  |  1 -
  src/util/virstoragefile.c | 23 ---
  src/util/virstoragefile.h | 37 -
  3 files changed, 61 deletions(-)

ACK.

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



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

Re: [libvirt] [PATCH 15/18] util: virstoragefile: Rename backingMeta to backingStore

2014-04-22 Thread Eric Blake
On 04/20/2014 04:13 PM, Peter Krempa wrote:
 To conform with the naming of the planed XML output rename the metadata

s/planed/planned/

 variable name.
 
 s/backingMeta/backingStore/g
 ---
  src/conf/domain_conf.c|  6 +++---
  src/qemu/qemu_driver.c|  8 
  src/util/virstoragefile.c | 12 +--
  src/util/virstoragefile.h |  2 +-
  tests/virstoragetest.c| 52 
 +++
  5 files changed, 40 insertions(+), 40 deletions(-)

Mechanical; ACK.

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



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

Re: [libvirt] [PATCH 16/18] storage: Move disk-backingChain to the recursive disk-src.backingStore

2014-04-22 Thread Eric Blake
On 04/20/2014 04:13 PM, Peter Krempa wrote:
 Switch over to storing of the backing chain as a recursive
 virStorageSource structure.
 
 This is a string based move. Currently the first element will be present
 twice in the backing chain as currently the retrieval function stores
 the parent in the newly detected chain. This will be fixed later.
 ---
  src/conf/domain_conf.c  |  5 ++---
  src/conf/domain_conf.h  |  1 -
  src/qemu/qemu_domain.c  | 20 ++--
  src/qemu/qemu_driver.c  | 30 +++---
  src/security/security_selinux.c |  2 +-
  src/security/virt-aa-helper.c   |  8 
  6 files changed, 32 insertions(+), 34 deletions(-)
 

 @@ -12730,13 +12730,13 @@ 
 qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver,
  if (virAsprintf(device, drive-%s, disk-info.alias)  0)
  goto cleanup;
 
 -/* XXX Here, we know we are about to alter disk-backingChain if
 +/* XXX Here, we know we are about to alter disk-src.backingStore if
   * successful, so we nuke the existing chain so that future commands will
   * recompute it.  Better would be storing the chain ourselves rather than
   * reprobing, but this requires modifying domain_conf and our XML to 
 fully
   * track the chain across libvirtd restarts.  */

Nice to know we are getting closer to fixing this comment.

Indeed mechanical, and I agree with your choice for the awkward
double-storage of the top of the chain until a later patch.

ACK.

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



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

Re: [libvirt] [PATCH 17/18] util: virstoragefile: Don't mangle data stored about directories

2014-04-22 Thread Eric Blake
On 04/20/2014 04:13 PM, Peter Krempa wrote:
 Don't remove detected metadata about directory based storage volumes.
 ---
  src/util/virstoragefile.c | 7 ++-
  tests/virstoragetest.c| 3 +++
  2 files changed, 5 insertions(+), 5 deletions(-)

ACK.

 +++ b/tests/virstoragetest.c
 @@ -709,6 +709,9 @@ mymain(void)
  testFileData dir = {
  .pathRel = dir,
  .pathAbs = absdir,
 +.path = canondir,
 +.relDirRel = .,
 +.relDirAbs = datadir,
  .type = VIR_STORAGE_TYPE_DIR,
  .format = VIR_STORAGE_FILE_DIR,
  };

Ah, I'm glad I spent time improving the testsuite.  This entire series
has been much easier to justify and easier to work with because of the
tests.

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



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

Re: [libvirt] [PATCH 18/18] util: storage: Invert the way recursive metadata retrieval works

2014-04-22 Thread Eric Blake
On 04/20/2014 04:13 PM, Peter Krempa wrote:
 To avoid having the root of a backing chain present twice in the list we
 need to invert the working of virStorageFileGetMetadataRecurse.
 
 Until now the recursive worker created a new backing chain element from
 the name and other information passed as arguments. This required us to
 pass the data of the parend in a deconstructed way and the worker

s/parend/parent/

 created a new entry for the parent.
 
 This patch converts this function so that it just fills in metadata
 about the parent and creates a backing chain element from those. This
 removes the duplication of the first element.
 
 To avoid breaking the test suite, virstoragetest now calls a wrapper
 that creates the parent structure explicitly and pre-fills it with the
 test data with same function signature as previously used.
 ---
  src/conf/domain_conf.c|   5 +-
  src/qemu/qemu_domain.c|  12 ++-
  src/qemu/qemu_driver.c|   6 +-
  src/security/virt-aa-helper.c |   7 +-
  src/util/virstoragefile.c | 193 
 ++
  src/util/virstoragefile.h |   7 +-
  tests/virstoragetest.c|  47 +-
  7 files changed, 158 insertions(+), 119 deletions(-)
 


 -}
 +/* check wether we need to go deeper */

s/wether/whether/


  }
 +} else {
 +/* TODO: To satisfy the test case, copy the network URI as path. T
 + * his will be removed later */

s/T.*his/This/


 +
 +if (virStorageFileGetMetadataRecurse(backingStore,
 + backingStore-path,
 + uid, gid, allow_probe,
 + cycle)  0) {
 +/* if we fail somewhere midway, just accept the and return a
 + * broken chain */

s/the and/and/ ?


 @@ -1220,51 +1227,51 @@ virStorageFileGetMetadataRecurse(const char *path, 
 const char *canonPath,
   *
   * Caller MUST free result after use via virStorageSourceFree.
   */
 -virStorageSourcePtr
 -virStorageFileGetMetadata(const char *path, int format,
 +int
 +virStorageFileGetMetadata(virStorageSourcePtr src,
uid_t uid, gid_t gid,
bool allow_probe)

Yep, quite an inversion; but looks reasonable.


 +
 +static virStorageSourcePtr
 +testStorageFileGetMetadata(const char *path,
 +   int format,
 +   uid_t uid, gid_t gid,
 +   bool allow_probe)

Nice wrapper.


 
 -meta = virStorageFileGetMetadata(data-start, data-format, -1, -1,
 +meta = testStorageFileGetMetadata(data-start, data-format, -1, -1,
   (data-flags  ALLOW_PROBE) != 0);

Indentation of second line is now off; here and in many other hunks this
file.

It's late for me, and this one's big enough that I'd like to revisit it
in my morning to make sure I'm not overlooking anything else.  It will
probably be ack with nits fixed, but can't hurt to be careful...


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



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

[libvirt] Local storage-migration plus network disks (Ceph RBD)

2014-04-22 Thread Blair Bethwaite
Hi there,

We have a production OpenStack cloud, currently on Qemu 1.0  1.5 with
Libivrt 1.1.1. We're using local storage with storage-migration when
we need to move machines around.

Since we built this thing we've noticed that with network storage
attached (have seen this with iSCSI and Ceph RBD targets, mainly
interested in the latter) the migration moves all of the network disk
contents as well, which for any non-toy disk sizes pretty much renders
it useless as the migration is then bounded not by the guest memory
size and activity but also by the block storage size. E.g., an idle
guest with 8GB RAM and a 300GB Ceph RBD attached takes ~3 hours to
migrate (over a 20GE network) and we observe balanced TX/RX on both
the source and destination, as the source reads blocks from the Ceph
cluster, streams them to the destination, and the destination in turn
writes them back to the Ceph cluster.

Looking in Libvirt's qemu_migration.c code I see
qemuMigrationDriveMirror skips shared, RO and source-less disks. But
I'm not sure why network disks in general, and in particular RBDs,
wouldn't be considered shared for these purposes. From my naive
reading the checks in qemuMigrationIsSafe (which explicitly pass
NETWORK+RBD) seem to confirm this, at least for RBDs. Is this a bug?

-- 
Cheers,
~Blairo

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