- Devel - libvirt List Archives

[libvirt] [PATCH for 5.7.0 0/2] virsh: Rename --precopy-bandwidth migration option
by Jiri Denemark 03 Sep '19

03 Sep '19

The (pre-copy) bandwidth was historically the only bandwidth we supported and thus it is called just "bandwidth" in all other places. E.g., virsh migrate-setspeed or in the migration typed parameter name. Let's make the new option for virsh migrate consistent. Jiri Denemark (2): virsh: Rename --precopy-bandwidth migration option news: Rename --precopy-bandwidth migration option docs/news.xml | 4 ++-- tools/virsh-domain.c | 6 +++--- tools/virsh.pod | 4 ++-- 3 files changed, 7 insertions(+), 7 deletions(-) -- 2.23.0

3 4

[libvirt] [PATCH] xenconfig: move contents to libxl driver and remove directory
by Jim Fehlig 02 Sep '19

02 Sep '19

Signed-off-by: Jim Fehlig <jfehlig(a)suse.com> --- cfg.mk | 2 +- configure.ac | 2 -- po/POTFILES | 6 ++--- src/Makefile.am | 1 - src/libvirt_xenconfig.syms | 12 ---------- src/libxl/Makefile.inc.am | 25 ++++++--------------- src/{xenconfig => libxl}/xen_common.c | 0 src/{xenconfig => libxl}/xen_common.h | 0 src/{xenconfig => libxl}/xen_xl.c | 0 src/{xenconfig => libxl}/xen_xl.h | 0 src/{xenconfig => libxl}/xen_xm.c | 0 src/{xenconfig => libxl}/xen_xm.h | 0 src/{xenconfig => libxl}/xenxs_private.h | 0 src/xenconfig/Makefile.inc.am | 28 ------------------------ tests/xlconfigtest.c | 2 +- tests/xmconfigtest.c | 2 +- 16 files changed, 13 insertions(+), 67 deletions(-) diff --git a/cfg.mk b/cfg.mk index c459ad443f..1f29729949 100644 --- a/cfg.mk +++ b/cfg.mk @@ -791,7 +791,7 @@ sc_prohibit_cross_inclusion: access/ | conf/) safe="($$dir|conf|util)";; \ cpu/| network/| node_device/| rpc/| security/| storage/) \ safe="($$dir|util|conf|storage)";; \ - xenapi/ | xenconfig/ ) safe="($$dir|util|conf|xen|cpu)";; \ + xenapi/) safe="($$dir|util|conf|xen|cpu)";; \ *) safe="($$dir|$(mid_dirs)|util)";; \ esac; \ in_vc_files="^src/$$dir" \ diff --git a/configure.ac b/configure.ac index a60543072d..890702a89d 100644 --- a/configure.ac +++ b/configure.ac @@ -465,8 +465,6 @@ LIBVIRT_DRIVER_CHECK_LIBVIRTD LIBVIRT_DRIVER_CHECK_NETWORK LIBVIRT_DRIVER_CHECK_INTERFACE -AM_CONDITIONAL([WITH_XENCONFIG], [test "$with_libxl" = "yes"]) - dnl dnl in case someone want to build static binaries diff --git a/po/POTFILES b/po/POTFILES index c62bc32bb2..e466e1bc55 100644 --- a/po/POTFILES +++ b/po/POTFILES @@ -88,6 +88,9 @@ src/libxl/libxl_conf.c src/libxl/libxl_domain.c src/libxl/libxl_driver.c src/libxl/libxl_migration.c +src/libxl/xen_common.c +src/libxl/xen_xl.c +src/libxl/xen_xm.c src/locking/lock_daemon.c src/locking/lock_daemon_dispatch.c src/locking/lock_driver_lockd.c @@ -291,9 +294,6 @@ src/vz/vz_utils.c src/vz/vz_utils.h src/xenapi/xenapi_driver.c src/xenapi/xenapi_utils.c -src/xenconfig/xen_common.c -src/xenconfig/xen_xl.c -src/xenconfig/xen_xm.c tests/virpolkittest.c tools/libvirt-guests.sh.in tools/virsh-checkpoint.c diff --git a/src/Makefile.am b/src/Makefile.am index adaf61350a..6626659113 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -118,7 +118,6 @@ include vbox/Makefile.inc.am include openvz/Makefile.inc.am include qemu/Makefile.inc.am include bhyve/Makefile.inc.am -include xenconfig/Makefile.inc.am include libxl/Makefile.inc.am include xenapi/Makefile.inc.am include vz/Makefile.inc.am diff --git a/src/libvirt_xenconfig.syms b/src/libvirt_xenconfig.syms deleted file mode 100644 index 6e2e578b15..0000000000 --- a/src/libvirt_xenconfig.syms +++ /dev/null @@ -1,12 +0,0 @@ -# -# These symbols are dependent upon --with-xen via WITH_XEN or --with-libxl via WITH_LIBXL. -# - -# xenconfig/xen_xm.h -xenFormatXM; -xenParseXM; - -# Let emacs know we want case-insensitive sorting -# Local Variables: -# sort-fold-case: t -# End: diff --git a/src/libxl/Makefile.inc.am b/src/libxl/Makefile.inc.am index 1587404586..560b214877 100644 --- a/src/libxl/Makefile.inc.am +++ b/src/libxl/Makefile.inc.am @@ -1,6 +1,13 @@ # vim: filetype=automake LIBXL_DRIVER_SOURCES = \ + libxl/xenxs_private.h \ + libxl/xen_common.c \ + libxl/xen_common.h \ + libxl/xen_xl.c \ + libxl/xen_xl.h \ + libxl/xen_xm.c \ + libxl/xen_xm.h \ libxl/libxl_conf.c \ libxl/libxl_conf.h \ libxl/libxl_capabilities.c \ @@ -15,29 +22,13 @@ LIBXL_DRIVER_SOURCES = \ libxl/libxl_migration.h \ $(NULL) -XENCONFIG_LIBXL_SOURCES = \ - $(XENCONFIG_SOURCES) \ - xenconfig/xen_xl.c \ - xenconfig/xen_xl.h \ - $(NULL) - DRIVER_SOURCE_FILES += $(LIBXL_DRIVER_SOURCES) STATEFUL_DRIVER_SOURCE_FILES += $(LIBXL_DRIVER_SOURCES) EXTRA_DIST += \ $(LIBXL_DRIVER_SOURCES) \ - $(XENCONFIG_LIBXL_SOURCES) \ $(NULL) if WITH_LIBXL -noinst_LTLIBRARIES += libvirt_xenconfig_libxl.la -libvirt_xenconfig_libxl_la_LIBADD = $(LIBXL_LIBS) -libvirt_xenconfig_libxl_la_CFLAGS = \ - -I$(srcdir)/conf \ - -I$(srcdir)/libxl \ - $(AM_CFLAGS) \ - $(NULL) -libvirt_xenconfig_libxl_la_SOURCES = $(XENCONFIG_LIBXL_SOURCES) - noinst_LTLIBRARIES += libvirt_driver_libxl_impl.la libvirt_driver_libxl_la_SOURCES = libvirt_driver_libxl_la_LIBADD = \ @@ -54,14 +45,12 @@ libvirt_driver_libxl_impl_la_CFLAGS = \ -I$(srcdir)/access \ -I$(srcdir)/conf \ -I$(srcdir)/secret \ - -I$(srcdir)/xenconfig \ $(AM_CFLAGS) \ $(NULL) libvirt_driver_libxl_impl_la_LDFLAGS = $(AM_LDFLAGS) libvirt_driver_libxl_impl_la_LIBADD = \ $(LIBXL_LIBS) \ $(LIBXML_LIBS) \ - libvirt_xenconfig_libxl.la \ $(NULL) libvirt_driver_libxl_impl_la_SOURCES = $(LIBXL_DRIVER_SOURCES) diff --git a/src/xenconfig/xen_common.c b/src/libxl/xen_common.c similarity index 100% rename from src/xenconfig/xen_common.c rename to src/libxl/xen_common.c diff --git a/src/xenconfig/xen_common.h b/src/libxl/xen_common.h similarity index 100% rename from src/xenconfig/xen_common.h rename to src/libxl/xen_common.h diff --git a/src/xenconfig/xen_xl.c b/src/libxl/xen_xl.c similarity index 100% rename from src/xenconfig/xen_xl.c rename to src/libxl/xen_xl.c diff --git a/src/xenconfig/xen_xl.h b/src/libxl/xen_xl.h similarity index 100% rename from src/xenconfig/xen_xl.h rename to src/libxl/xen_xl.h diff --git a/src/xenconfig/xen_xm.c b/src/libxl/xen_xm.c similarity index 100% rename from src/xenconfig/xen_xm.c rename to src/libxl/xen_xm.c diff --git a/src/xenconfig/xen_xm.h b/src/libxl/xen_xm.h similarity index 100% rename from src/xenconfig/xen_xm.h rename to src/libxl/xen_xm.h diff --git a/src/xenconfig/xenxs_private.h b/src/libxl/xenxs_private.h similarity index 100% rename from src/xenconfig/xenxs_private.h rename to src/libxl/xenxs_private.h diff --git a/src/xenconfig/Makefile.inc.am b/src/xenconfig/Makefile.inc.am deleted file mode 100644 index 5785b49268..0000000000 --- a/src/xenconfig/Makefile.inc.am +++ /dev/null @@ -1,28 +0,0 @@ -# vim: filetype=automake - -XENCONFIG_SOURCES = \ - xenconfig/xenxs_private.h \ - xenconfig/xen_common.c \ - xenconfig/xen_common.h \ - xenconfig/xen_xm.c \ - xenconfig/xen_xm.h \ - $(NULL) - -EXTRA_DIST += $(XENCONFIG_SOURCES) - -if WITH_XENCONFIG -noinst_LTLIBRARIES += libvirt_xenconfig.la -libvirt_la_BUILT_LIBADD += libvirt_xenconfig.la -libvirt_xenconfig_la_CFLAGS = \ - -I$(srcdir)/conf \ - $(AM_CFLAGS) \ - $(NULL) -libvirt_xenconfig_la_SOURCES = $(XENCONFIG_SOURCES) - -endif WITH_XENCONFIG - -if WITH_XENCONFIG -USED_SYM_FILES += $(srcdir)/libvirt_xenconfig.syms -else ! WITH_XENCONFIG -SYM_FILES += $(srcdir)/libvirt_xenconfig.syms -endif ! WITH_XENCONFIG diff --git a/tests/xlconfigtest.c b/tests/xlconfigtest.c index 59171606d1..ae0db71293 100644 --- a/tests/xlconfigtest.c +++ b/tests/xlconfigtest.c @@ -27,7 +27,7 @@ #include "internal.h" #include "datatypes.h" -#include "xenconfig/xen_xl.h" +#include "libxl/xen_xl.h" #include "viralloc.h" #include "virstring.h" #include "testutils.h" diff --git a/tests/xmconfigtest.c b/tests/xmconfigtest.c index 1de3c33f69..3137dc564c 100644 --- a/tests/xmconfigtest.c +++ b/tests/xmconfigtest.c @@ -25,7 +25,7 @@ #include "internal.h" #include "datatypes.h" -#include "xenconfig/xen_xm.h" +#include "libxl/xen_xm.h" #include "testutils.h" #include "testutilsxen.h" #include "viralloc.h" -- 2.22.0

6 7

[libvirt] [PATCH] qemu: domain: Fix potential NULL deref when parsing job private data
by Peter Krempa 02 Sep '19

02 Sep '19

A specially crafted XML which would reference a non-existing disk but request the mirror to be registered with the blockjob could potentially make the parser dereference NULL. Fix it by moving the code slightly and just treat it as a wrong job XML. Found by Coverity. Reported-by: John Ferlan <jferlan(a)redhat.com> Signed-off-by: Peter Krempa <pkrempa(a)redhat.com> --- src/qemu/qemu_domain.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 657f3ecfe4..c7eb0b5e9a 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3012,15 +3012,19 @@ qemuDomainObjPrivateXMLParseBlockjobData(virDomainObjPtr vm, invalidData = true; } + if (mirror) { + if (disk) + job->mirrorChain = virObjectRef(disk->mirror); + else + invalidData = true; + } + job->state = state; job->newstate = newstate; job->errmsg = virXPathString("string(./errmsg)", ctxt); job->invalidData = invalidData; job->disk = disk; - if (mirror) - job->mirrorChain = virObjectRef(job->disk->mirror); - qemuDomainObjPrivateXMLParseBlockjobDataSpecific(job, ctxt, xmlopt); if (qemuBlockJobRegister(job, vm, disk, false) < 0) -- 2.21.0

3 2

[libvirt] [PATCH] util: Set backing file name for LOOP_GET_STATUS64 queries.
by jcfaracco＠gmail.com 02 Sep '19

02 Sep '19

From: Julio Faracco <jcfaracco(a)gmail.com> This is an issue for LXC loop devices when you are trying to get loop devices info using `ioctl`. Modern apps uses `/sys/dev/block` to grab information about devices, but if you use the method mention you won't be able to retrive the associated file with that loop device. See example below from cryptsetup sources: static char *_ioctl_backing_file(const char *loop) { struct loop_info64 lo64 = {0}; int loop_fd; loop_fd = open(loop, O_RDONLY); if (loop_fd < 0) return NULL; if (ioctl(loop_fd, LOOP_GET_STATUS64, &lo64) < 0) { close(loop_fd); return NULL; } lo64.lo_file_name[LO_NAME_SIZE-2] = '*'; lo64.lo_file_name[LO_NAME_SIZE-1] = 0; close(loop_fd); return strdup((char*)lo64.lo_file_name); } It will return an empty string because lo_file_name was not set. Signed-off-by: Julio Faracco <jcfaracco(a)gmail.com> --- src/util/virfile.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/util/virfile.c b/src/util/virfile.c index 81a3c096eb..fe0aa54eb6 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -781,6 +781,16 @@ int virFileLoopDeviceAssociate(const char *file, memset(&lo, 0, sizeof(lo)); lo.lo_flags = LO_FLAGS_AUTOCLEAR; + /* Set backing file name for LOOP_GET_STATUS64 queries */ + if (virStrncpy((char *) lo.lo_file_name, file, + strlen(file), LO_NAME_SIZE) < 0) { + virReportSystemError(errno, + _("Unable to set backing file %s"), file); + goto cleanup; + } + lo.lo_file_name[LO_NAME_SIZE-2] = '*'; + lo.lo_file_name[LO_NAME_SIZE-1] = 0; + if ((fsfd = open(file, O_RDWR)) < 0) { virReportSystemError(errno, _("Unable to open %s"), file); -- 2.20.1

1 0

[libvirt] [RFC] On present using dummy hostdev usb device
by Nikolay Shirokovskiy 02 Sep '19

02 Sep '19

Hi, all! We use an interesting approach when starting/migrating/etc domain with usb hostdev with startupPolicy=optional. We add qemu usb-host device with missing hostaddr/hostbus parameters (dummy device). I guess there are 2 reasons why we do it. First without dummy device migration will fail as described in [1]. Second is an interesting property of dummy device that qemu starts to monitor for attaching of usb devices and binds the first attached to node to the dummy device. So one can start a domain with missing hostdev and attach it later or migrate a domain then detach hostdev on source and attach it on destination. But as qemu binds the first attached device this is not reliable, to say the least. And after all this does not work if domain uses distinct mount namespace which is default. So I question does it make sense to use dummy device at all? In case of migration/resume from suspend/revert to snapshot we can either fix qemu to ignore incoming missing hostdev data or add dummy device temporarily. The latter solution is worse as it brings dummy device behaviour even for a short period of time. However having a temporary dummy device is neccessary step towards the time when all supported versions of qemu do the mentioned ignoring. As to handling attaching of missing hostdev device to node it should be done in libvirt which can do necessary mount namespace actions. (Actually I developing such patches right now but some peculiarities of dummy device bring me here). Nikolay [1] https://www.redhat.com/archives/libvir-list/2012-October/msg00440.html

5 7

[libvirt] [rust PATCH] Add list_all_volumes method for storage_pool::StoragePool
by Sage Imel 02 Sep '19

02 Sep '19

From: Sage Imel <sage(a)sagenite.net> Always returns the full list of volumes, can't just ask it how many volumes are in the pool Signed-off-by: Sage Imel <sage(a)cat.pdx.edu> --- src/storage_pool.rs | 29 ++++++++++++++++++++++++++++- 1 file changed, 28 insertions(+), 1 deletion(-) diff --git a/src/storage_pool.rs b/src/storage_pool.rs index 38676c2..e8ed21c 100644 --- a/src/storage_pool.rs +++ b/src/storage_pool.rs @@ -18,7 +18,7 @@ extern crate libc; -use std::str; +use std::{str, ptr}; use connect::sys::virConnectPtr; use storage_vol::sys::virStorageVolPtr; @@ -57,6 +57,10 @@ extern "C" { xml: *const libc::c_char, flags: libc::c_uint) -> sys::virStoragePoolPtr; + fn virStoragePoolListAllVolumes(ptr: sys::virStoragePoolPtr, + vols: *mut *mut virStorageVolPtr, + flags:libc::c_uint) + -> libc::c_int; fn virStoragePoolLookupByID(c: virConnectPtr, id: libc::c_int) -> sys::virStoragePoolPtr; fn virStoragePoolLookupByName(c: virConnectPtr, id: *const libc::c_char) @@ -103,6 +107,8 @@ pub const STORAGE_POOL_CREATE_WITH_BUILD: StoragePoolCreateFlags = 1 << 0; pub const STORAGE_POOL_CREATE_WITH_BUILD_OVERWRITE: StoragePoolCreateFlags = 1 << 1; pub const STORAGE_POOL_CREATE_WITH_BUILD_NO_OVERWRITE: StoragePoolCreateFlags = 1 << 2; +pub type VirStoragePoolListAllVolumesFlags = self::libc::c_uint; + pub type StoragePoolState = self::libc::c_uint; pub const VIR_STORAGE_POOL_INACTIVE: StoragePoolState = 0; pub const VIR_STORAGE_POOL_BUILDING: StoragePoolState = 1; @@ -201,6 +207,27 @@ impl StoragePool { } } + pub fn list_all_volumes(&self, + flags: VirStoragePoolListAllVolumesFlags) + -> Result<Vec<StorageVol>, Error> { + unsafe { + let mut volumes: *mut virStorageVolPtr = ptr::null_mut(); + let size = + virStoragePoolListAllVolumes(self.as_ptr(), &mut volumes, flags as libc::c_uint); + if size == -1 { + return Err(Error::new()); + } + + let mut array: Vec<StorageVol> = Vec::new(); + for x in 0..size as isize { + array.push(StorageVol::new(*volumes.offset(x))); + } + libc::free(volumes as *mut libc::c_void); + + return Ok(array); + } + } + pub fn lookup_by_id(conn: &Connect, id: u32) -> Result<StoragePool, Error> { unsafe { let ptr = virStoragePoolLookupByID(conn.as_ptr(), id as libc::c_int); -- 2.17.1

2 2

[libvirt] [glib PATCH 0/2] Add gvir_config_domain_os_get_firmware() + _domain_os_get_machine() test
by Fabiano Fidêncio 02 Sep '19

02 Sep '19

This series adds a way to retrive the DomainOs' Firmware set (with tests) and, while touching that code, also adds one more check for gvir_config_domain_os_get_machine(). Fabiano Fidêncio (2): gconfig: Add _domain_os_get_firmware() tests,test-gconfig: Check _domain_os_get_machine() libvirt-gconfig/libvirt-gconfig-domain-os.c | 12 ++++++++++++ libvirt-gconfig/libvirt-gconfig-domain-os.h | 1 + libvirt-gconfig/libvirt-gconfig.sym | 1 + tests/test-gconfig.c | 3 +++ 4 files changed, 17 insertions(+) -- 2.21.0

2 4

[libvirt] Entering freeze for libvirt 5.7.0
by Daniel Veillard 02 Sep '19

02 Sep '19

I'm a day late but I just tagged it in git and pushed signed tarball and source rpm to the usual place: https://libvirt.org/sources/ Seems to work fine in my very minimal testing, there is a few red spots in CI https://ci.centos.org/view/libvirt/ but they seems mostly related to Debian 10 testing so maybe there was a hiccup on that arch in CI... I guess I will push RC2 over the week-end, and if things are fine push the final 5.7.0 release next Tuesday, in the meantime, please give it some testing, thanks, Daniel -- Daniel Veillard | Red Hat Developers Tools http://developer.redhat.com/ veillard(a)redhat.com | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | virtualization library http://libvirt.org/

3 3

[libvirt] [PATCH v4 1/4] lib: minor fixes to virDomainGetGuestInfo docs
by Jonathon Jongsma 30 Aug '19

30 Aug '19

Due to a typo, some of the field names didn't have closing quotes and the information about the hostname was omitted. Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com> --- src/libvirt-domain.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index cea4d2c11b..bc24090e2e 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -12237,10 +12237,10 @@ virDomainSetVcpu(virDomainPtr domain, * * "user.count" - the number of active users on this domain as an * unsigned int - * "user.<num>.name - username of the user as a string - * "user.<num>.domain - domain of the user as a string (may only be + * "user.<num>.name" - username of the user as a string + * "user.<num>.domain" - domain of the user as a string (may only be * present on certain guest types) - * "user.<num>.login-time - the login time of a user in milliseconds + * "user.<num>.login-time" - the login time of a user in milliseconds * since the epoch as unsigned long long * * VIR_DOMAIN_GUEST_INFO_OS: @@ -12288,6 +12288,12 @@ virDomainSetVcpu(virDomainPtr domain, * "fs.<num>.disk.<num>.serial" - the serial number of the disk * "fs.<num>.disk.<num>.device" - the device node of the disk * + * VIR_DOMAIN_GUEST_INFO_HOSTNAME: + * Returns information about the hostname of the domain. The typed + * parameter keys are in this format: + * + * "hostname" - the hostname of the domain + * * Using 0 for @types returns all information groups supported by the given * hypervisor. * -- 2.21.0

4 12

Re: [libvirt] [BUG] When PLUG a bridge interface to an active VM, the generated LIVE and CONFIG mac address are different
by wujing (O) 30 Aug '19

30 Aug '19

> On 8/28/19 11:42 AM, wujing (O) wrote: > >>>>> On 8/22/19 10:43 AM, Xu Yandong (Yandong Xu) wrote: > >>>> That possibly might be out of scope, but autofilling the mac > >>>> address as early as virDomainNetDefParseXML also is not ideal. > >>> > >>> We have pushed a patch bellow that can restore the situation to an > >>> older > >> state. > >>> > >>> Subject: [PATCH] qemu: use the same def when attaching device live > >>> and config > >>> > >>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index > >>> 617d7d5..eca54d0 100644 > >>> --- a/src/qemu/qemu_driver.c > >>> +++ b/src/qemu/qemu_driver.c > >>> @@ -8615,6 +8615,22 @@ > >> qemuDomainAttachDeviceLiveAndConfig(virDomainObjPtr vm, > >>> if (!(caps = virQEMUDriverGetCapabilities(driver, false))) > >>> goto cleanup; > >>> > >>> + if (flags & VIR_DOMAIN_AFFECT_CONFIG && > >>> + flags & VIR_DOMAIN_AFFECT_LIVE) { > >>> + /* If we are affecting both CONFIG and LIVE > >>> + * use the same xml of device preferentially > >>> + * to make the configuration consistent. > >>> + */ > >>> + devLive = virDomainDeviceDefParse(xml, vm->def, > >> > >> This use of vm->def prefers live XML. This means that for instance > >> PCI address are assigned based on current PCI layout in live XML > >> which in general is different to inactive XML. > >> > >> And it's not only PCI addresses, we autogenerate some out aspects of > >> <interface/> (e.g. model) and even more for other devices. Just take > >> a look at qemuDomainDeviceDefPostParse(). All functions there which > >> take domain def as an argument do so because they are basing > >> autogenerated value on domain definition. Plus there's more in parser > code. > >> > >> While your patch might work for your use case, it can break others. > >> > >> BTW: have you read the original commit that caused this? I'm failing > >> to see how we would not re-introduce the problem with your patch. If > >> you're using live XML to validate/generate device addresses, how can > >> we generate valid address for inactive XML? > >> > > > > This patch is a little different with the original commit, since we > > only prefer live XML when using LIVE AND CONFIG flags at the same time. > Still parse separate device def if attach with only one flag. > > So this patch is kinda a combination of 1e0534a7702 and 55ce6564634, just > to narrow down the problem influence. > > > > And I verified as the steps in > https://bugzilla.redhat.com/show_bug.cgi?id=1559867, work as expected. > > Well, then try to coldplug <hostdev mode='subsystem' type='scsi'/> (that is > only to inactive XML), and then try hotplug different <hostdev type'scsi'/> to > both live and inactive XMLs and the bug will reproduce. > Problem is, that inactive XML contains a device which is not contained in live > XML and thus when generating device address based solely on live XML a > conflicting address is generated. > > If we'd prefer inactive XML in your patch then the steps in my reproducer > need to be swapped. At any rate, the issue is still there. You are right, but I found a new issue on current solution. Test as steps below. 1. Having a running vm ## virsh list Id Name State ---------------------- │~ 1 rhel running 2. prepare 2 scsi hostdev xml using the same address ## cat h1.xml <hostdev mode='subsystem' type='scsi' managed='no'> <source> <adapter name='scsi_host2'/> <address bus='0' target='0' unit='0'/> </source> <address type='drive' controller='0' bus='0' target='0' unit='2'/> </hostdev> ## cat h2.xml <hostdev mode='subsystem' type='scsi' managed='no'> <source> <adapter name='scsi_host3'/> <address bus='0' target='0' unit='0'/> </source> <address type='drive' controller='0' bus='0' target='0' unit='2'/> </hostdev> 3. attach 2 hostdev to vm with 'attach-device --config' ## virsh attach-device rhel h1.xml --config Device attached successfully ## virsh attach-device rhel h2.xml --config Device attached successfully 4. Check the vm's xml with 'dumpxml --inactive' ## virsh dumpxml rhel --inactive ... <hostdev mode='subsystem' type='scsi' managed='no'> <source> <adapter name='scsi_host2'/> <address bus='0' target='0' unit='0'/> </source> <address type='drive' controller='0' bus='0' target='0' unit='2'/> </hostdev> <hostdev mode='subsystem' type='scsi' managed='no'> <source> <adapter name='scsi_host3'/> <address bus='0' target='0' unit='0'/> </source> <address type='drive' controller='0' bus='0' target='0' unit='2'/> </hostdev> ... a) Looks like libvirt can not verify if user configured the same address for different scsi hostdevs. Because virDomainHostdevDefPostParse only calls virDomainDriveAddressIsUsedByDisk to check if address is used by other disks, but no check if used by other hostdevs. And qemuDomainAttachDeviceConfig calls virDomainHostdevFind to check duplicate devices by src addr and other stuff, not including addr in vm. Should that be fixed? b) If we fixed a), then we can get a devConf that both fits to live and config xml by passing devLive and domainPersistentDef to virDomainDeviceDefCopy. diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 617d7d5..eca54d0 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8615,6 +8615,22 @@ qemuDomainAttachDeviceLiveAndConfig(virDomainObjPtr vm, if (!(caps = virQEMUDriverGetCapabilities(driver, false))) goto cleanup; + if (flags & VIR_DOMAIN_AFFECT_CONFIG && + flags & VIR_DOMAIN_AFFECT_LIVE) { + devLive = virDomainDeviceDefParse(xml, vm->def, + caps, driver->xmlopt, + parse_flags); + if (!devLive) + goto cleanup; + vmdef = virDomainObjCopyPersistentDef(vm, caps, driver->xmlopt); + if (!vmdef) + goto cleanup; + /* Use persistent vm def to get a device def copy + */ + devConf = virDomainDeviceDefCopy(devLive, vmdef, caps, driver->xmlopt); + if (!devConf) + goto cleanup; + } If devLive fits to vmdef, then we get a compatible devConf, or just fail and return. Wu Jing

3 2