Re: [libvirt] [PATCH 1/2] util: introduce virDirRead wrapper for readdir
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)
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
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)
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
--- 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
--- 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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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)
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