- Devel - libvirt List Archives

[libvirt] [PATCH 0/3] qemu: make runtime validation of NetDefs more consistent
by Laine Stump 16 Sep '19

16 Sep '19

There is some validation of NetDefs (<interface>) that can't be done until runtime, due to not knowing the exact configuration until that time. This needs to happen in 3 places: 1) in the qemu commandline when a new guest is started, 2) when an <interface> is hotplugged, and 3) when an <interface> is updated. Until now, there some (but not all) validation copy-pasted between (1) and (2), and none of that was present for (3). These patches move all the validation from (1) (which is the most complete) into a separate function, and then call that function from all three places, so the exact same validation is done in all cases. (These patches are a followup to a patch I sent *long* ago in a naive attempt to fix https://bugzilla.redhat.com/1502754 - my original patch did fix the problem when starting a guest with an invalid <interface> config, but not when hotplugging or updating such an interface.) NB: I noticed that if someone tries to specify <bandwidth> for an interface type that doesn't support it, we only give a warning, not an error, due to a fear that there are existing management apps that add <bandwidth> to all types of interfaces, and we don't want them to suddenly fail to run (I got this info from the BZ associated with the commit that added the warning). This seems to me to be going a bit too far - I think that (at least upstream) we should turn this into an error, and let the regression testing of said management apps catch the behavior change so they can fix their code. (insert Kermit-drinking-coffee meme here) Laine Stump (3): conf: make arg to virDomainNetGetActualVirtPortProfile() a const qemu: move runtime netdev validation into a separate function qemu: call common NetDef validation for hotplug and device update src/conf/domain_conf.c | 2 +- src/conf/domain_conf.h | 2 +- src/qemu/qemu_command.c | 52 +-------------------------- src/qemu/qemu_domain.c | 80 +++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_domain.h | 4 +++ src/qemu/qemu_hotplug.c | 32 +++++------------ 6 files changed, 95 insertions(+), 77 deletions(-) -- 2.21.0

2 8

[libvirt] Potential bug: spice port collision on different addresses
by Michael Edlinger 14 Sep '19

14 Sep '19

Hi all, I think I've found a bug in either libvirt or spice. I'll give a short description here, but I have a stack exchange question that also has information here: https://unix.stackexchange.com/questions/541127/how-to-listen-on-same-port-… Basically, have spice for VM1 listen to port 5900 on 10.0.0.2 and spice for VM2 listen to port 5900 on 10.0.0.3, and you'll get an error message saying the ports collide, even though they shouldn't be (they are listening on different addresses). I've tried going to the #virt irc channel with very limited responses, and would appreciate hearing what you all have to say about this issue. Thanks! - Michael

2 2

[libvirt] [PATCH] qemu_blockjob: Don't leak old job when registering a new one
by Michal Privoznik 14 Sep '19

14 Sep '19

If a disk that a block job is registered with already has a job (stored in private data) it needs to be unrefed to keep the refcounter in balance (and properly free the object): 2,577 (104 direct, 2,473 indirect) bytes in 1 blocks are definitely lost in loss record 168 of 174 at 0x4838B86: calloc (vg_replace_malloc.c:752) by 0x4A0AD42: virAllocVar (viralloc.c:346) by 0x4A84D12: virObjectNew (virobject.c:243) by 0x21EE13: qemuBlockJobDataNew (qemu_blockjob.c:111) by 0x184EBC: qemuDomainObjPrivateXMLParseBlockjobData (qemu_domain.c:3120) by 0x1854B5: qemuDomainObjPrivateXMLParseBlockjobs (qemu_domain.c:3191) by 0x186D7D: qemuDomainObjPrivateXMLParse (qemu_domain.c:3622) by 0x4B198CC: virDomainObjParseXML (domain_conf.c:21490) by 0x4B19DDF: virDomainObjParseNode (domain_conf.c:21630) by 0x4B19E7D: virDomainObjParseFile (domain_conf.c:21649) by 0x13FDD5: testCompareStatusXMLToXMLFiles (qemuxml2xmltest.c:61) by 0x16F7D2: virTestRun (testutils.c:115) Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com> --- src/qemu/qemu_blockjob.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c index a991309ee7..67b1f677f8 100644 --- a/src/qemu/qemu_blockjob.c +++ b/src/qemu/qemu_blockjob.c @@ -151,6 +151,7 @@ qemuBlockJobRegister(qemuBlockJobDataPtr job, if (disk) { job->disk = disk; job->chain = virObjectRef(disk->src); + virObjectUnref(QEMU_DOMAIN_DISK_PRIVATE(disk)->blockjob); QEMU_DOMAIN_DISK_PRIVATE(disk)->blockjob = virObjectRef(job); } -- 2.21.0

3 2

[libvirt] [PATCH] network: fix connection usage counts after restart
by Daniel P. Berrangé 13 Sep '19

13 Sep '19

Since the introduction of the virNetworkPort object, the network driver has a persistent record of ports that have been created against the networks. Thus the hypervisor drivers no longer communicate to the network driver during libvirtd restart. This change, however, meant that the connection usage counts were no longer re-initialized during a libvirtd restart. To deal with this we must iterate over all virNetworkPortDefPtr objects we have and invoke the notify callback to record the connection usage count. Signed-off-by: Daniel P. Berrangé <berrange(a)redhat.com> --- src/conf/virnetworkobj.c | 33 +++++++++++++++++++++++++++++++++ src/conf/virnetworkobj.h | 9 +++++++++ src/libvirt_private.syms | 1 + src/network/bridge_driver.c | 17 +++++++++++++++++ 4 files changed, 60 insertions(+) diff --git a/src/conf/virnetworkobj.c b/src/conf/virnetworkobj.c index d63ead7fac..ca1d598cf9 100644 --- a/src/conf/virnetworkobj.c +++ b/src/conf/virnetworkobj.c @@ -1844,6 +1844,39 @@ virNetworkObjPortListExport(virNetworkPtr net, } +typedef struct _virNetworkObjPortListForEachData virNetworkObjPortListForEachData; +struct _virNetworkObjPortListForEachData { + virNetworkPortListIter iter; + void *opaque; + bool err; +}; + +static int +virNetworkObjPortForEachCallback(void *payload, + const void *name ATTRIBUTE_UNUSED, + void *opaque) +{ + virNetworkObjPortListForEachData *data = opaque; + + if (!data->iter(payload, data->opaque)) + data->err = true; + + return 0; +} + +int +virNetworkObjPortForEach(virNetworkObjPtr obj, + virNetworkPortListIter iter, + void *opaque) +{ + virNetworkObjPortListForEachData data = { iter, opaque, false }; + virHashForEach(obj->ports, virNetworkObjPortForEachCallback, &data); + if (data.err) + return -1; + return 0; +} + + static int virNetworkObjLoadAllPorts(virNetworkObjPtr net, const char *stateDir) diff --git a/src/conf/virnetworkobj.h b/src/conf/virnetworkobj.h index 1c28f0888c..a91b4304c6 100644 --- a/src/conf/virnetworkobj.h +++ b/src/conf/virnetworkobj.h @@ -190,6 +190,15 @@ virNetworkObjPortListExport(virNetworkPtr net, virNetworkPortPtr **ports, virNetworkPortListFilter filter); +typedef bool +(*virNetworkPortListIter)(virNetworkPortDefPtr portdef, + void *opaque); + +int +virNetworkObjPortForEach(virNetworkObjPtr obj, + virNetworkPortListIter iter, + void *opaque); + int virNetworkObjSaveStatus(const char *statusDir, virNetworkObjPtr net, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 7fe10d2286..37afb07e21 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1097,6 +1097,7 @@ virNetworkObjLookupPort; virNetworkObjMacMgrAdd; virNetworkObjMacMgrDel; virNetworkObjNew; +virNetworkObjPortForEach; virNetworkObjPortListExport; virNetworkObjRemoveInactive; virNetworkObjReplacePersistentDef; diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 7b44184616..0fee153cb8 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -527,6 +527,21 @@ networkBridgeDummyNicName(const char *brname) } +static int +networkNotifyPort(virNetworkObjPtr obj, + virNetworkPortDefPtr port); + +static bool +networkUpdatePort(virNetworkPortDefPtr port, + void *opaque) +{ + virNetworkObjPtr obj = opaque; + + networkNotifyPort(obj, port); + + return false; +} + static int networkUpdateState(virNetworkObjPtr obj, void *opaque) @@ -591,6 +606,8 @@ networkUpdateState(virNetworkObjPtr obj, goto cleanup; } + virNetworkObjPortForEach(obj, networkUpdatePort, obj); + /* Try and read dnsmasq/radvd pids of active networks */ if (virNetworkObjIsActive(obj) && def->ips && (def->nips > 0)) { pid_t radvdPid; -- 2.21.0

2 3

[libvirt] [PATCH] network: apply bandwidth settings for forward mode=bridge
by Daniel P. Berrangé 13 Sep '19

13 Sep '19

We previously allowed bandwidth settings when attaching NICs to networks with forward mode=bridge: commit 42a92ee93d5432ebd9ebfd409903b5287fc7d7ff Author: Daniel P. Berrangé <berrange(a)redhat.com> Date: Tue Nov 20 11:30:05 2018 +0000 network: add missing bandwidth limits for bridge forward type In the case of a network with forward=bridge, which has a bridge device listed, we are capable of setting bandwidth limits but fail to call the function to register them. Reviewed-by: Cole Robinson <crobinso(a)redhat.com> Signed-off-by: Daniel P. Berrangé <berrange(a)redhat.com> Unfortunately the wrong version of this patch was posted and reviewed and thus it lacked the code to actually apply the bandwidth settings to the bridge itself. Signed-off-by: Daniel P. Berrangé <berrange(a)redhat.com> --- src/network/bridge_driver.c | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 0fee153cb8..6a97bb17e2 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -2743,21 +2743,39 @@ networkShutdownNetworkVirtual(virNetworkDriverStatePtr driver, static int networkStartNetworkBridge(virNetworkObjPtr obj) { + virNetworkDefPtr def = virNetworkObjGetDef(obj); + /* put anything here that needs to be done each time a network of * type BRIDGE, is started. On failure, undo anything you've done, * and return -1. On success return 0. */ - return networkStartHandleMACTableManagerMode(obj, NULL); + if (virNetDevBandwidthSet(def->bridge, def->bandwidth, true, true) < 0) + goto error; + + if (networkStartHandleMACTableManagerMode(obj, NULL) < 0) + goto error; + + return 0; + + error: + if (def->bandwidth) + virNetDevBandwidthClear(def->bridge); + return -1; } static int networkShutdownNetworkBridge(virNetworkObjPtr obj ATTRIBUTE_UNUSED) { + virNetworkDefPtr def = virNetworkObjGetDef(obj); + /* put anything here that needs to be done each time a network of * type BRIDGE is shutdown. On failure, undo anything you've done, * and return -1. On success return 0. */ + if (def->bandwidth) + virNetDevBandwidthClear(def->bridge); + return 0; } -- 2.21.0

2 1

[libvirt] [PATCH] network: add debug when bandwidth settings are not applied
by Daniel P. Berrangé 13 Sep '19

13 Sep '19

To aid in troubleshooting add some debug messages wrt bandwidth settings and networks. Signed-off-by: Daniel P. Berrangé <berrange(a)redhat.com> --- src/network/bridge_driver.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 6a97bb17e2..c54be96407 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -5192,9 +5192,14 @@ networkCheckBandwidth(virNetworkObjPtr obj, return -1; } + if (!netBand || !netBand->in) { + VIR_DEBUG("No network bandwidth controls present"); + /* no QoS required, claim success */ + return 1; + } if (((!ifaceBand || !ifaceBand->in || !ifaceBand->in->floor) && - (!oldBandwidth || !oldBandwidth->in || !oldBandwidth->in->floor)) || - !netBand || !netBand->in) { + (!oldBandwidth || !oldBandwidth->in || !oldBandwidth->in->floor))) { + VIR_DEBUG("No old/new interface bandwidth floor"); /* no QoS required, claim success */ return 1; } -- 2.21.0

2 1

[libvirt] [PATCH 0/9] Pass identity information between daemons
by Daniel P. Berrangé 13 Sep '19

13 Sep '19

This was previously posted as part of the daemon split series: https://www.redhat.com/archives/libvir-list/2019-July/msg01754.html The patches were not merged at the time since they needed more work which is now done by this series. This is all about enabling the polkit fine grain auth checks to work correctly with the split daemons A mgmt app running non-root will connect to virtqemud. virtqemud sees the client identity as the non-root user. virtqemud in turn may connect to virtnetworkd, forwarding API calls that the mgmt app makes. virtnetworkd sees the client identity as the root user. This series allows virtqemud to pass on the non-root identity of the mgmt app to virtnetworkd, so that polkit checks are done against the correct identity. Daniel P. Berrangé (9): api: introduce virConnectSetIdentity for passing uid, gid, selinux info util: change identity class attribute names tests: fix debug messages wrt selinux context when test fails util: make generic identity accessors private util: removed unused virIdentityIsEqual method util: sanitize return values for virIdentity getters util: store identity attrs as virTypedParameter internally util: allow identity to be imported/exported as typed parameters remote: pass identity across to newly opened daemons include/libvirt/libvirt-host.h | 74 +++++ src/access/viraccessdriverpolkit.c | 22 +- src/admin/admin_server.c | 52 +-- src/driver-hypervisor.h | 7 + src/libvirt-host.c | 51 +++ src/libvirt_private.syms | 21 +- src/libvirt_public.syms | 4 + src/libvirt_remote.syms | 1 + src/remote/remote_daemon_dispatch.c | 112 ++++++- src/remote/remote_driver.c | 1 + src/remote/remote_protocol.x | 16 +- src/remote_protocol-structs | 8 + src/rpc/virnetserverclient.c | 20 +- src/rpc/virnetserverclient.h | 2 + src/util/viridentity.c | 488 ++++++++++++++++------------ src/util/viridentity.h | 71 ++-- tests/viridentitytest.c | 115 ++----- tests/virnetserverclienttest.c | 40 +-- 18 files changed, 672 insertions(+), 433 deletions(-) -- 2.21.0

2 12

[libvirt] [PATCH] tests: remove use of virTestOOMActive from bhyve testsuite
by Daniel P. Berrangé 13 Sep '19

13 Sep '19

The virTestOOMActive method was deleted in commit 2c52ecd96086b4643b99b4570b5823d40ce2787b Author: Daniel P. Berrangé <berrange(a)redhat.com> Date: Thu Aug 29 13:04:07 2019 +0100 util: purge all code for testing OOM handling Signed-off-by: Daniel P. Berrangé <berrange(a)redhat.com> --- Pushed as a CI build fix for FreeBSD tests/bhyveargv2xmltest.c | 44 +++++++++++++++++++-------------------- 1 file changed, 21 insertions(+), 23 deletions(-) diff --git a/tests/bhyveargv2xmltest.c b/tests/bhyveargv2xmltest.c index 1684f03355..d108366682 100644 --- a/tests/bhyveargv2xmltest.c +++ b/tests/bhyveargv2xmltest.c @@ -39,38 +39,36 @@ testCompareXMLToArgvFiles(const char *xmlfile, if (!(vmdef = bhyveParseCommandLineString(cmd, driver.bhyvecaps, driver.xmlopt))) { - if ((flags & FLAG_EXPECT_FAILURE) && !virTestOOMActive()) { - VIR_TEST_DEBUG("Got expected failure from " - "bhyveParseCommandLineString."); + if ((flags & FLAG_EXPECT_FAILURE)) { + VIR_TEST_DEBUG("Got expected failure from " + "bhyveParseCommandLineString."); } else { goto fail; } - } else if ((flags & FLAG_EXPECT_FAILURE) && !virTestOOMActive()) { + } else if ((flags & FLAG_EXPECT_FAILURE)) { VIR_TEST_DEBUG("Did not get expected failure from " "bhyveParseCommandLineString."); goto fail; } - if (!virTestOOMActive()) { - if ((log = virTestLogContentAndReset()) == NULL) + if ((log = virTestLogContentAndReset()) == NULL) + goto fail; + if (flags & FLAG_EXPECT_WARNING) { + if (*log) { + VIR_TEST_DEBUG("Got expected warning from " + "bhyveParseCommandLineString:\n%s", + log); + } else { + VIR_TEST_DEBUG("bhyveParseCommandLineString " + "should have logged a warning"); + goto fail; + } + } else { /* didn't expect a warning */ + if (*log) { + VIR_TEST_DEBUG("Got unexpected warning from " + "bhyveParseCommandLineString:\n%s", + log); goto fail; - if (flags & FLAG_EXPECT_WARNING) { - if (*log) { - VIR_TEST_DEBUG("Got expected warning from " - "bhyveParseCommandLineString:\n%s", - log); - } else { - VIR_TEST_DEBUG("bhyveParseCommandLineString " - "should have logged a warning"); - goto fail; - } - } else { /* didn't expect a warning */ - if (*log) { - VIR_TEST_DEBUG("Got unexpected warning from " - "bhyveParseCommandLineString:\n%s", - log); - goto fail; - } } } -- 2.21.0

1 0

[libvirt] [PATCH] apparmor: avoid copying empty profile name
by Jim Fehlig 13 Sep '19

13 Sep '19

AppArmorGetSecurityProcessLabel copies the VM's profile name to the label member of virSecurityLabel struct. If the profile is not loaded, the name is set empty before calling virStrcpy to copy it. However, virStrcpy will fail if src is empty (0 length), causing AppArmorGetSecurityProcessLabel to needlessly fail. Simple operations that report security driver information will subsequently fail virsh dominfo test Id: 248 Name: test ... Security model: apparmor Security DOI: 0 error: internal error: error copying profile name Avoid copying an empty profile name when the profile is not loaded. Signed-off-by: Jim Fehlig <jfehlig(a)suse.com> --- src/security/security_apparmor.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c index 6d16b15c65..77eee9410c 100644 --- a/src/security/security_apparmor.c +++ b/src/security/security_apparmor.c @@ -525,14 +525,13 @@ AppArmorGetSecurityProcessLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, "%s", _("error getting profile status")); goto cleanup; } else if (status == -1) { - profile_name[0] = '\0'; - } - - if (virStrcpy(sec->label, profile_name, - VIR_SECURITY_LABEL_BUFLEN) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("error copying profile name")); - goto cleanup; + sec->label[0] = '\0'; + } else { + if (virStrcpy(sec->label, profile_name, VIR_SECURITY_LABEL_BUFLEN) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("error copying profile name")); + goto cleanup; + } } sec->enforcing = status == 1; -- 2.23.0

2 2

[libvirt] [PATCH] qemu: fix detach of hostdev based network interface
by Daniel P. Berrangé 13 Sep '19

13 Sep '19

This fixes bug in commit bbe2aa627f621e6749af374b22856184d1f351dc Author: Daniel P. Berrangé <berrange(a)redhat.com> Date: Thu Jul 26 17:24:30 2018 +0100 conf: simplify link from hostdev back to network device hostdevs have a link back to the original network device. This is fairly generic accepting any type of device, however, we don't intend to make use of this approach in future. It can thus be specialized to network devices. Reviewed-by: Cole Robinson <crobinso(a)redhat.com> Signed-off-by: Daniel P. Berrangé <berrange(a)redhat.com> which mistakenly deleted the assignment to the 'net' variable, which meant we never invoked the network driver release callback Signed-off-by: Daniel P. Berrangé <berrange(a)redhat.com> --- src/qemu/qemu_hotplug.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index bd8868b0f7..16070f2a57 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -4561,6 +4561,7 @@ qemuDomainRemoveHostDevice(virQEMUDriverPtr driver, } if (hostdev->parentnet) { + net = hostdev->parentnet; for (i = 0; i < vm->def->nnets; i++) { if (vm->def->nets[i] == hostdev->parentnet) { virDomainNetRemove(vm->def, i); -- 2.21.0

2 1