Re: [libvirt PATCH] qemu: fix crash in qemuDomainSetBlkioParameters without cgroups
On a Tuesday in 2020, Pavel Hrdina wrote: If we don't have cgroups available and user tries to update blkio parameters for running VM it will crash. It should have been protected by the virCgroupHasController() check but it was never called if the API was executed without any flags. We call virDomainObjGetDefs() which sets `def` and `persistentDef` based on the flags and these two variables should be used to figure out if we need to update LIVE, CONFIG or both states. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1808293 Signed-off-by: Pavel Hrdina --- src/qemu/qemu_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature
Re: [GSoC][PATCH 3/7] qemu_domainjob: add `saveDomainStatus` as a callback function to jobs
On Tue, Aug 04, 2020 at 08:06:45PM +0530, Prathamesh Chavan wrote:
> The function `qemuDomainObjSaveStatus` required an access to
> `virQEMUDriverPtr`.
> To make jobs hypervisor-agnostic we remove this funciton
> and replace it with a callback function from `qemuDomainJob`
>
> Removal of `virQEMUDriverPtr` as parameter resulted in
> removal of the same from function, where it was pass.
> All of such references were removed as the variable
> was no longer required.
>
> Signed-off-by: Prathamesh Chavan
> ---
> src/qemu/qemu_backup.c | 41 +-
> src/qemu/qemu_backup.h | 3 +-
> src/qemu/qemu_block.c| 45 +-
> src/qemu/qemu_block.h| 6 +-
> src/qemu/qemu_blockjob.c | 45 +-
> src/qemu/qemu_blockjob.h | 3 +-
> src/qemu/qemu_checkpoint.c | 29 +-
> src/qemu/qemu_domain.c | 78 ++-
> src/qemu/qemu_domain.h | 24 +-
> src/qemu/qemu_domainjob.c| 50 +-
> src/qemu/qemu_domainjob.h| 29 +-
> src/qemu/qemu_driver.c | 848 ++-
> src/qemu/qemu_hotplug.c | 319 ++--
> src/qemu/qemu_hotplug.h | 30 +-
> src/qemu/qemu_migration.c| 315 +---
> src/qemu/qemu_migration.h| 12 +-
> src/qemu/qemu_migration_cookie.c | 7 +-
> src/qemu/qemu_migration_params.c | 48 +-
> src/qemu/qemu_migration_params.h | 15 +-
> src/qemu/qemu_process.c | 258 +-
> src/qemu/qemu_process.h | 15 +-
> tests/qemuhotplugtest.c | 2 +-
> 22 files changed, 986 insertions(+), 1236 deletions(-)
>
Hi, I'm sorry for the delay, but I spent a while thinking about other
approaches to achieve the same what I'm commenting on below. I had to verify
every single idea by debugging libvirt so that I would not propose something
that was impossible to do and by doing that, I realized a very interesting
circular data reference we have:
(QEMU)driver->xmlopt->config.priv->(QEMU)driver
...
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 677fa7ea91..d7a944a886 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -634,6 +634,7 @@ static qemuDomainObjPrivateJobCallbacks
> qemuPrivateJobCallbacks = {
> .allocJobPrivate = qemuJobAllocPrivate,
> .freeJobPrivate = qemuJobFreePrivate,
> .resetJobPrivate = qemuJobResetPrivate,
> +.saveStatus = qemuDomainSaveStatus,
> .formatJob = qemuDomainFormatJobPrivate,
> .parseJob = qemuDomainParseJobPrivate,
> .setJobInfoOperation =qemuDomainJobInfoSetOperation,
Okay, ^this does the job, it works, but I would call it the easy way out.
The qemuPrivateJobCallbacks structure hints that it contains callbacks specific
to job handling to which qemuDomainSaveStatus is simply not related at all.
It just so happens, that we have to save the domain status basically every time
we're doing something to the VM.
Structurally, I see 2 ways to achieve the same code extraction properly.
First, having another structure for callbacks which would nest the existing
qemuPrivateJobCallbacks, IOW:
struct _qemuDomainObjPrivateCallbacks {
/* generic callbacks that we can't really categorize */
qemuDomainObjPrivateSaveStatus saveStatus;
/* Job related callbacks */
qemuDomainObjPrivateJobCallbacks jobcb;
}
We'd then pass ^this structure instead of the qemuDomainObjPrivateJobCallbacks
one at the relevant places.
I don't like the ^this solution that much either, but I wanted to mention it.
I think what we need to do instead is to look what qemuDomainSaveStatus or
qemuDomainObjSaveStatus really need. They need to access the driver and its
config, that's it. In that perspective it relates to the virDomainObj's private
data.
Specifically for qemuDomainObjSaveStatus:
...
if (virDomainObjIsActive(obj))
...
^This check can easily be extracted to the virDomainObjSave function, there's
not reason why it should be specific to QEMU only.
...
if (virDomainObjSave(obj, driver->xmlopt, cfg->stateDir) < 0)
...
^This is the thing we need to call from the hypervisor-agnostic code, except we
don't have @driver (note that for example libxl doesn't have @driver as
part of the virDomainObj's private data).
Considering the above, we need a generic wrapper over virDomainObjSave, let's
call it virDomainDriverObjSave:
void virDomainDriverObjSave(virDomainObjPtr obj) {
return obj->privateDataCallbacks.saveStatus(obj);
}
struct _virDomainObj {
...
void *privateData;
void (*privateDataFreeFunc) (void *);
virDomainObjSaveStatusCallbackPtr saveStatus; <
...
};
The saveStatus callback would then have to live inside xmlopt callbacks and
be copied over in virDomainObjNew (just like we copy the free callback).
This is far from ideal, as it involves @xmlopt as we should not be interacting
with it, but we're already abusing the @xmlopt on so many places that it's such
an integral part of libvirt that refactorin
Re: [GSoC][PATCH 4/7] qemu_domain: funciton declarations moved to correct file
On Tue, Aug 04, 2020 at 08:06:46PM +0530, Prathamesh Chavan wrote: > Functions `qemuDomainRemoveInactiveJob` and > `qemuDomainRemoveInactiveJobLocked` had their declaration > mispalced in `qemu_domainjob` and were moved to s/mispalced/misplaced > `qemu_domain`. > > Signed-off-by: Prathamesh Chavan > --- Reviewed-by: Erik Skultety
Re: [GSoC][PATCH 5/7] qemu_domainjob: added `getDomainXMLOptionPtr` callback function
On Tue, Aug 04, 2020 at 08:06:47PM +0530, Prathamesh Chavan wrote:
> To remove dependency of funcitons to access the `privateData` of
> qemu-domain, we introduce this callback funciton so that funcitons
> get exactly what they need.
>
> Signed-off-by: Prathamesh Chavan
> ---
> src/qemu/qemu_domain.c| 8
> src/qemu/qemu_domainjob.c | 4 ++--
> src/qemu/qemu_domainjob.h | 2 ++
> 3 files changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index d7a944a886..2e16c8e5fe 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -629,12 +629,20 @@ qemuDomainParseJobPrivate(xmlXPathContextPtr ctxt,
> return 0;
> }
>
> +static virDomainXMLOptionPtr
> +qemuGetDomainXMLOptionPtr(virDomainObjPtr vm)
> +{
> +qemuDomainObjPrivatePtr priv = vm->privateData;
> +return priv->driver->xmlopt;
> +
> +}
>
> static qemuDomainObjPrivateJobCallbacks qemuPrivateJobCallbacks = {
> .allocJobPrivate = qemuJobAllocPrivate,
> .freeJobPrivate = qemuJobFreePrivate,
> .resetJobPrivate = qemuJobResetPrivate,
> .saveStatus = qemuDomainSaveStatus,
> +.getDomainXMLOptionPtr = qemuGetDomainXMLOptionPtr,
> .formatJob = qemuDomainFormatJobPrivate,
> .parseJob = qemuDomainParseJobPrivate,
> .setJobInfoOperation = qemuDomainJobInfoSetOperation,
> diff --git a/src/qemu/qemu_domainjob.c b/src/qemu/qemu_domainjob.c
> index 19c847dffc..3eff45fd17 100644
> --- a/src/qemu/qemu_domainjob.c
> +++ b/src/qemu/qemu_domainjob.c
> @@ -765,7 +765,7 @@ qemuDomainObjPrivateXMLFormatNBDMigration(virBufferPtr
> buf,
> if (diskPriv->migrSource &&
> qemuDomainObjPrivateXMLFormatNBDMigrationSource(&childBuf,
>
> diskPriv->migrSource,
> -
> priv->driver->xmlopt) < 0)
> +
> priv->job.cb->getDomainXMLOptionPtr(vm)) < 0)
NBD migration is very much QEMU specific at the moment, so if you move
qemuDomainObjPrivateXMLFormatNBDMigrationSource and functions alike out of the
qemu_domainjob module which we're planning on using as a base for the
hypervisor-agnostic job handling module, you won't need this patch.
Erik
Re: [GSoC][PATCH 7/7] virmigraiton: `qemuMigrationJobPhase` transformed for more generic use
On Tue, Aug 04, 2020 at 08:06:49PM +0530, Prathamesh Chavan wrote: > `qemuMigrationJobPhase` was transformed into `virMigrationJobPhase` > and a common util file `virmigration` was created to store its > defination. > > Signed-off-by: Prathamesh Chavan > --- You basically just moved the QEMU migration enums which as a patch on its own doesn't make much sense, other migration bits that can be made agnostic would have to either follow or be part of the same patch. Erik
Re: [GSoC][PATCH 6/7] qemu_domainjob: removed reference to `qemuDomainObjPrivatePtr`
On Tue, Aug 04, 2020 at 08:06:48PM +0530, Prathamesh Chavan wrote: > References to `qemuDomainObjPrivatePtr` in qemu_domainjob > were removed as it is a qemu-hypervisor specific pointer. > > Signed-off-by: Prathamesh Chavan > --- This patch looks good. Erik
Re: [PATCH] network: allow accept_ra == 0 when enabling ipv6 forwarding
Yay! A user who follows up their conversation on the libvirt-users list
with a patch! :-)
On 8/11/20 8:14 PM, Ian Wienand wrote:
The checks modified here were added with
00d28a78b5d1f6eaf79f06ac59e31c568af9da37 to avoid losing routes on
hosts.
I'm Cc'ing the author of that patch, Cédric Bosdonnat, to make sure that
whatever we end up doing doesn't upset his use case.
However, tools such as systemd-networking and NetworkManager manage
RA's in userspace and thus IPv6 may be up and working on an interface
even with accept_ra == 0.
This modifies the check to only error if an interface's accept_ra is
already set to "1"; as noted inline this seems to when it is likely
that enabling forwarding may change the RA acceptance behaviour of the
interface.
Unfortunately, on my Fedora 32 machine that has NetworkManager enabled,
not all the interfaces have accept_ra=0 by default. One of the bridge
devices (created by NetworkManager in response to an ifcfg file in
/etc/sysconfig/network-scripts) has accept_ra = 1. (there are several
other interfaces that have accept_ra=1, but those interfaces are either
not online, or they don't have an IPv6 address.
So this doesn't work for all cases. I think we need to get more
information on how to most easily, generically, and reliably determine
if the kernel accept_ra setting can be ignored. Possibly the
NetworkManager people can help us out here.
(or alternately we could punt and just make a configuration setting,
although that is taking the easy road, and not a good precedent to set)
I have noticed this because I am using the IPv6 NAT features enabled
with 927acaedec7effbe67a154d8bfa0e67f7d08e6c7. I am using this on my
laptop which switches between wired and wireless connections; both of
which are configured in an unremarkable way by Fedora's NetworkManager
and get configured for IPv6 via SLAAC and whatever NetworkManager
magic it does. With this I can define and start a libvirt network
with and and it seems to "just work"
for guests.
Signed-off-by: Ian Wienand
---
src/util/virnetdevip.c | 41 +++--
1 file changed, 27 insertions(+), 14 deletions(-)
diff --git a/src/util/virnetdevip.c b/src/util/virnetdevip.c
index 409f062c5c..de27cacfc9 100644
--- a/src/util/virnetdevip.c
+++ b/src/util/virnetdevip.c
@@ -496,7 +496,7 @@ virNetDevIPGetAcceptRA(const char *ifname)
}
struct virNetDevIPCheckIPv6ForwardingData {
-bool hasRARoutes;
+bool hasKernelRARoutes;
/* Devices with conflicting accept_ra */
char **devices;
@@ -552,15 +552,26 @@ virNetDevIPCheckIPv6ForwardingCallback(struct nlmsghdr
*resp,
if (!ifname)
return -1;
-accept_ra = virNetDevIPGetAcceptRA(ifname);
-
VIR_DEBUG("Checking route for device %s (%d), accept_ra: %d",
ifname, ifindex, accept_ra);
-if (accept_ra != 2 && virNetDevIPCheckIPv6ForwardingAddIF(data, &ifname) < 0)
+accept_ra = virNetDevIPGetAcceptRA(ifname);
+/* 0 = do no accept RA
+ * 1 = accept if forwarding disabled
+ * 2 = ovveride and accept RA when forwarding enabled
+ *
+ * When RA is managed by userspace (systemd-networkd or
+ * NetworkManager) accept_ra is unset and we don't need to
+ * worry about it. If it is 1, enabling forwarding might
+ * change the behaviour so the user needs to be warned.
+ */
+if (accept_ra == 0)
+return 0;
+
+if (accept_ra == 1 && virNetDevIPCheckIPv6ForwardingAddIF(data, &ifname)
< 0)
return -1;
-data->hasRARoutes = true;
+data->hasKernelRARoutes = true;
return 0;
}
@@ -590,11 +601,13 @@ virNetDevIPCheckIPv6ForwardingCallback(struct nlmsghdr *resp,
VIR_DEBUG("Checking multipath route nexthop device %s (%d), accept_ra:
%d",
ifname, nh->rtnh_ifindex, accept_ra);
-if (accept_ra != 2 && virNetDevIPCheckIPv6ForwardingAddIF(data, &ifname) < 0)
-return -1;
+if (accept_ra == 1) {
+if (virNetDevIPCheckIPv6ForwardingAddIF(data, &ifname) < 0)
+return -1;
+data->hasKernelRARoutes = true;
+}
VIR_FREE(ifname);
-data->hasRARoutes = true;
len -= NLMSG_ALIGN(nh->rtnh_len);
VIR_WARNINGS_NO_CAST_ALIGN
@@ -613,7 +626,7 @@ virNetDevIPCheckIPv6Forwarding(void)
struct rtgenmsg genmsg;
size_t i;
struct virNetDevIPCheckIPv6ForwardingData data = {
-.hasRARoutes = false,
+.hasKernelRARoutes = false,
.devices = NULL,
.ndevices = 0
};
@@ -644,11 +657,11 @@ virNetDevIPCheckIPv6Forwarding(void)
goto cleanup;
}
-valid = !data.hasRARoutes || data.ndevices == 0;
+valid = !data.hasKernelRARoutes || data.ndevices == 0;
/*
Re: [PATCH] network: allow accept_ra == 0 when enabling ipv6 forwarding
On Wed, Aug 12, 2020 at 07:21:14PM -0400, Laine Stump wrote: > Yay! A user who follows up their conversation on the libvirt-users list with > a patch! :-) Heh, ipv6 in my work VM is my white whale, so, call me Ishmael :) > Unfortunately, on my Fedora 32 machine that has NetworkManager enabled, not > all the interfaces have accept_ra=0 by default. One of the bridge devices > (created by NetworkManager in response to an ifcfg file in > /etc/sysconfig/network-scripts) has accept_ra = 1. (there are several other > interfaces that have accept_ra=1, but those interfaces are either not > online, or they don't have an IPv6 address. Interesting, from a cursory search the only place NetworkManager seems to set accept_ra to 1 is [1] for ip tokens (TIL ...) and then that seems to turn if off again when the interface is enabled. But you know, that's also 18,000+ lines of logic in there ... so ... >From my memory of looking at the ifcfg-* files path it's a plugin for NM that reads those files and basically passes them as config into NM "core". So it doesn't do anything particularly magic. However I'm reminded of a bug that took us a very long time to diagnose with "glean", a tool to configure interfaces from config-drive data in OpenStack clouds. Some clouds don't do DHCP, so it is used to read the config-drive data set by the cloud and write out the network config. It had a long history, before systemd udev activation was a thing, an was calling "ip link set dev up" on the interface and then seeing if it had a carrier. This would enable accept_ra == 1 and the kernel would configure an IPv6 address; however since the interface then looked configured, and NetworkManager would ignore it, and thus not apply the IPv4 configuration we had written out. Hosts came up with an IPv6 but no IPv4; it was all very confusing till we put it together. My long winded point being that *maybe* something else is getting in the way ... > So this doesn't work for all cases. So the intent at least is to do this warning iff the existing interface is already set to "1". So if you did put that bridge into the VM network and we enabled forwarding on it, that would seem to change it's behaviour and possibly break it? > I think we need to get more information on how to most easily, > generically, and reliably determine if the kernel accept_ra setting > can be ignored. Possibly the NetworkManager people can help us out > here. ++ to that, if the correct response is to error, or automatically set it to "2", or ignore it are all open questions :) -i [1] https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/blob/52af5e901e4e5e7727ae83db18a37730b5f898fe/src/devices/nm-device.c#L1553 [2] https://review.opendev.org/#/c/688031/
[PATCH v2] virnetserver: fix some memory leaks in virNetTLSContextReloadForServer
These leaks were introduced in commit 15d280fa97b0, use g_autofree for all
cert_path pointers.
Signed-off-by: Jin Yan
---
src/rpc/virnettlscontext.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/rpc/virnettlscontext.c b/src/rpc/virnettlscontext.c
index 168f3010ae..37564db14e 100644
--- a/src/rpc/virnettlscontext.c
+++ b/src/rpc/virnettlscontext.c
@@ -921,10 +921,10 @@ int virNetTLSContextReloadForServer(virNetTLSContextPtr
ctxt,
{
gnutls_certificate_credentials_t x509credBak;
int err;
-char *cacert = NULL;
-char *cacrl = NULL;
-char *cert = NULL;
-char *key = NULL;
+g_autofree char *cacert = NULL;
+g_autofree char *cacrl = NULL;
+g_autofree char *cert = NULL;
+g_autofree char *key = NULL;
x509credBak = ctxt->x509cred;
ctxt->x509cred = NULL;
--
2.23.0
Re: device compatibility interface for live migration with assigned devices
On 2020/8/10 下午3:46, Yan Zhao wrote: driver is it handled by? It looks that the devlink is for network device specific, and in devlink.h, it says include/uapi/linux/devlink.h - Network physical device Netlink interface, Actually not, I think there used to have some discussion last year and the conclusion is to remove this comment. It supports IB and probably vDPA in the future. I feel like it's not very appropriate for a GPU driver to use this interface. Is that right? I think not though most of the users are switch or ethernet devices. It doesn't prevent you from inventing new abstractions. Note that devlink is based on netlink, netlink has been widely used by various subsystems other than networking. Thanks Thanks Yan
