Re: [PATCH 1/2] qemu: Fix migration with custom XML
On Tue, Apr 16, 2024 at 19:53:25 +0200, Jiri Denemark wrote:
> Ages ago origCPU in domain private data was introduced to provide
> backward compatibility when migrating to an old libvirt, which did not
> support fetching updated CPU definition from QEMU. Thus origCPU will
> contain the original CPU definition before such update. But only if the
> update actually changed anything. Let's always fill origCPU with the
> original definition when starting a domain so that we can rely on it
> being always set, even if it matches the updated definition.
>
> This fixes migration or save operations with custom domain XML after
> commit v10.1.0-88-g14d3517410, which expected origCPU to be always set
> to the CPU definition from inactive XML to check features explicitly
> requested by a user.
>
> https://issues.redhat.com/browse/RHEL-30622
>
> Signed-off-by: Jiri Denemark
> ---
> src/qemu/qemu_domain.c | 38 +-
> src/qemu/qemu_process.c | 14 ++
> 2 files changed, 23 insertions(+), 29 deletions(-)
>
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 6b869797a8..ca50d435d2 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -10995,12 +10995,13 @@ virSaveCookieCallbacks
> virQEMUDriverDomainSaveCookie = {
> * @vm: domain which is being started
> * @cpu: CPU updated when the domain was running previously (before
> migration,
> * snapshot, or save)
> - * @origCPU: where to store the original CPU from vm->def in case @cpu was
> - * used instead
> + * @origCPU: where to store the original CPU from vm->def
> *
> - * Replace the CPU definition with the updated one when QEMU is new enough to
> - * allow us to check extra features it is about to enable or disable when
> - * starting a domain. The original CPU is stored in @origCPU.
> + * Save the original CPU definition from inactive XML in @origCPU so that we
> + * can safely update it and still be able to check what exactly a user asked
> + * for. The domain definition will either contain a copy of the original CPU
> + * definition or a copy of @cpu in case the domain was already running and
> + * we're just restoring a saved state or preparing for incoming migration.
> *
> * Returns 0 on success, -1 on error.
> */
> @@ -11013,15 +11014,25 @@ qemuDomainUpdateCPU(virDomainObj *vm,
>
> *origCPU = NULL;
>
> -if (!cpu || !vm->def->cpu ||
> -!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_QUERY_CPU_MODEL_EXPANSION)
> ||
> -virCPUDefIsEqual(vm->def->cpu, cpu, false))
> +if (!vm->def->cpu)
> return 0;
>
> -if (!(cpu = virCPUDefCopy(cpu)))
> +if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_QUERY_CPU_MODEL_EXPANSION))
> +return 0;
> +
> +/* nothing to do if only topology part of CPU def is used */
> +if (vm->def->cpu->mode == VIR_CPU_MODE_CUSTOM && !vm->def->cpu->model)
> +return 0;
> +
> +if (cpu)
> +cpu = virCPUDefCopy(cpu);
> +else
> +cpu = virCPUDefCopy(vm->def->cpu);
> +
> +if (!cpu)
> return -1;
First two reads were very confusing as this is overwriting the argument
>
> -VIR_DEBUG("Replacing CPU def with the updated one");
> +VIR_DEBUG("Replacing CPU definition");
>
> *origCPU = vm->def->cpu;
> vm->def->cpu = cpu;
... just to store it here. At this point it's no longer necessary. You
can first move the old def into 'origCPU' and then directly assign into
the variable to avoid the overwrite for readability.
> @@ -11064,13 +11075,6 @@ qemuDomainFixupCPUs(virDomainObj *vm,
> !vm->def->cpu->model)
> return 0;
>
> -/* Missing origCPU means QEMU created exactly the same virtual CPU which
> - * we asked for or libvirt was too old to mess up the translation from
> - * host-model.
> - */
> -if (!*origCPU)
> -return 0;
This function is called from two places:
src/qemu/qemu_process.c=qemuProcessStartWithMemoryState(virConnectPtr conn,
src/qemu/qemu_process.c:qemuDomainFixupCPUs(vm, &cookie->cpu) < 0)
src/qemu/qemu_process.c=qemuProcessRefreshCPU(virQEMUDriver *driver,
src/qemu/qemu_process.c:if (qemuDomainFixupCPUs(vm, &priv->origCPU) < 0
'priv->origCPU' which is used in the latter is AFAIU filled only in
'qemuDomainUpdateCPU' called when starting up a qemu process and in
'qemuDomainObjPrivateXMLParse' when loading a status XML.
In both cases there are code paths which may leave 'priv->origCPU' to be
NULL.
The following code will dereference it unconditionally:
if (virCPUDefFindFeature(*origCPU, "cmt")) {
> -
> if (virCPUDefFindFeature(vm->def->cpu, "cmt")) {
> g_autoptr(virCPUDef) fixedCPU =
> virCPUDefCopyWithoutModel(vm->def->cpu);
>
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index e4bcb628cf..8165c28dbc 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -4437,8 +4437,6 @@ qemuProcessUp
Re: [PATCH 2/2] NEWS: Mention migration bug with custom XML
On Tue, Apr 16, 2024 at 19:53:26 +0200, Jiri Denemark wrote: > Signed-off-by: Jiri Denemark > --- > NEWS.rst | 8 > 1 file changed, 8 insertions(+) > > diff --git a/NEWS.rst b/NEWS.rst > index c108e66f46..852dadf532 100644 > --- a/NEWS.rst > +++ b/NEWS.rst > @@ -21,6 +21,14 @@ v10.3.0 (unreleased) > > * **Bug fixes** > > + * qemu: Fix migration with custom XML custom migration XML perhaps ? or custom definition for migration? Each VM xml is custom ;) > + > +Libvirt 10.2.0 would sometimes complain about incompatible CPU definition > +when trying to migrate or save a domain and passing a custom XML even > +though such XML was properly generated as migratable. Hitting this bug > +depends on the guest CPU definition and the host on which a particular > +domain was running. Reviewed-by: Peter Krempa ___ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-le...@lists.libvirt.org
Re: [PATCH 2/2] NEWS: Mention migration bug with custom XML
On Wed, Apr 17, 2024 at 09:17:31 +0200, Peter Krempa wrote: > On Tue, Apr 16, 2024 at 19:53:26 +0200, Jiri Denemark wrote: > > Signed-off-by: Jiri Denemark > > --- > > NEWS.rst | 8 > > 1 file changed, 8 insertions(+) > > > > diff --git a/NEWS.rst b/NEWS.rst > > index c108e66f46..852dadf532 100644 > > --- a/NEWS.rst > > +++ b/NEWS.rst > > @@ -21,6 +21,14 @@ v10.3.0 (unreleased) > > > > * **Bug fixes** > > > > + * qemu: Fix migration with custom XML > > custom migration XML perhaps ? or custom definition for migration? Each > VM xml is custom ;) Yeah, I didn't like too many "migration" words on a single line :-) Jirka ___ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-le...@lists.libvirt.org
Re: [PATCH v3 01/22] hw/i386/pc: Deprecate 2.4 to 2.7 pc-i440fx machines
On Tue, Apr 16, 2024 at 03:52:30PM +0200, Philippe Mathieu-Daudé wrote: > Date: Tue, 16 Apr 2024 15:52:30 +0200 > From: Philippe Mathieu-Daudé > Subject: [PATCH v3 01/22] hw/i386/pc: Deprecate 2.4 to 2.7 pc-i440fx > machines > X-Mailer: git-send-email 2.41.0 > > Similarly to the commit c7437f0ddb "docs/about: Mark the > old pc-i440fx-2.0 - 2.3 machine types as deprecated", > deprecate the 2.4 to 2.7 machines. > > Suggested-by: Thomas Huth > Signed-off-by: Philippe Mathieu-Daudé > --- > docs/about/deprecated.rst | 4 ++-- > hw/i386/pc_piix.c | 2 +- > 2 files changed, 3 insertions(+), 3 deletions(-) LGTM, Reviewed-by: Zhao Liu ___ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-le...@lists.libvirt.org
Re: [PATCH v3 20/22] hw/i386/pc: Remove deprecated pc-i440fx-2.3 machine
On Tue, Apr 16, 2024 at 03:52:49PM +0200, Philippe Mathieu-Daudé wrote: > Date: Tue, 16 Apr 2024 15:52:49 +0200 > From: Philippe Mathieu-Daudé > Subject: [PATCH v3 20/22] hw/i386/pc: Remove deprecated pc-i440fx-2.3 > machine > X-Mailer: git-send-email 2.41.0 > > The pc-i440fx-2.3 machine was deprecated for the 8.2 > release (see commit c7437f0ddb "docs/about: Mark the > old pc-i440fx-2.0 - 2.3 machine types as deprecated"), > time to remove it. > > Signed-off-by: Philippe Mathieu-Daudé > --- > docs/about/deprecated.rst | 4 ++-- > docs/about/removed-features.rst | 2 +- > hw/i386/pc.c| 25 - > hw/i386/pc_piix.c | 19 --- > 4 files changed, 3 insertions(+), 47 deletions(-) Reviewed-by: Zhao Liu ___ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-le...@lists.libvirt.org
[PATCH] qemu: Deliver shutoff reason with qemu hooks
For abnormal shutoff reasons, we can start guest again with qemu hooks. Signed-off-by: Sun Feng --- docs/hooks.rst | 3 ++- src/qemu/qemu_process.c | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/docs/hooks.rst b/docs/hooks.rst index 1dbc492bd4..45856e4ca4 100644 --- a/docs/hooks.rst +++ b/docs/hooks.rst @@ -204,7 +204,8 @@ operation. There is no specific operation to indicate a "restart" is occurring. - When a QEMU guest is stopped, the qemu hook script is called in two locations, to match the startup. First, :since:`since 0.8.0`, the hook is - called before libvirt restores any labels: + called before libvirt restores any labels, :since:`since 9.10.0`, shutoff + reason is delivered with **extra argument**: :: diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index e4bcb628cf..c42f5c9139 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -8564,7 +8564,8 @@ void qemuProcessStop(virQEMUDriver *driver, /* we can't stop the operation even if the script raised an error */ ignore_value(virHookCall(VIR_HOOK_DRIVER_QEMU, vm->def->name, VIR_HOOK_QEMU_OP_STOPPED, VIR_HOOK_SUBOP_END, - NULL, xml, NULL)); + virDomainShutoffReasonTypeToString(reason), + xml, NULL)); } /* Reset Security Labels unless caller don't want us to */ -- 2.34.1 ___ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-le...@lists.libvirt.org
Re: [PATCH] qemu: Deliver shutoff reason with qemu hooks
On Wed, Apr 17, 2024 at 13:58:06 +0800, Sun Feng wrote: > For abnormal shutoff reasons, we can start guest again with qemu hooks. Beware that: A hook script must not call back into libvirt, as the libvirt daemon is already waiting for the script to exit. A deadlock is likely to occur. https://libvirt.org/hooks.html#calling-libvirt-functions-from-within-a-hook-script Preferrably summarize what the patch is doing or suggest something that is not strongly discouraged by the docs. > Signed-off-by: Sun Feng > --- > docs/hooks.rst | 3 ++- > src/qemu/qemu_process.c | 3 ++- > 2 files changed, 4 insertions(+), 2 deletions(-) > > diff --git a/docs/hooks.rst b/docs/hooks.rst > index 1dbc492bd4..45856e4ca4 100644 > --- a/docs/hooks.rst > +++ b/docs/hooks.rst > @@ -204,7 +204,8 @@ operation. There is no specific operation to indicate a > "restart" is occurring. > > - When a QEMU guest is stopped, the qemu hook script is called in two > locations, to match the startup. First, :since:`since 0.8.0`, the hook is > - called before libvirt restores any labels: > + called before libvirt restores any labels, :since:`since 9.10.0`, shutoff > + reason is delivered with **extra argument**: Next release is going to be 10.3.0 > > :: > > diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c > index e4bcb628cf..c42f5c9139 100644 > --- a/src/qemu/qemu_process.c > +++ b/src/qemu/qemu_process.c > @@ -8564,7 +8564,8 @@ void qemuProcessStop(virQEMUDriver *driver, > /* we can't stop the operation even if the script raised an error */ > ignore_value(virHookCall(VIR_HOOK_DRIVER_QEMU, vm->def->name, > VIR_HOOK_QEMU_OP_STOPPED, > VIR_HOOK_SUBOP_END, > - NULL, xml, NULL)); > + virDomainShutoffReasonTypeToString(reason), > + xml, NULL)); To be honest I'm not exactly sold that this is needed as the reason for this hook's existence is to simply clear up any resources that would be set up by the start hook, thus it's usually irrelevant why that happened. In your example you need to setup the hook so that it spawns off a separate process and terminates thus you can also query the shutdown reason from libvirt in that call too. Said that I'm not strongly against it, I'm just pointing out that I don't really see a good reason based on the fact that the hooks must not call back into libvirt. I'll let others chime in with their opinion. ___ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-le...@lists.libvirt.org
Re: [PATCH] qemu: Deliver shutoff reason with qemu hooks
On Wed, Apr 17, 2024 at 10:08:24AM +0200, Peter Krempa wrote: > On Wed, Apr 17, 2024 at 13:58:06 +0800, Sun Feng wrote: > > For abnormal shutoff reasons, we can start guest again with qemu hooks. > > Beware that: > > A hook script must not call back into libvirt, as the libvirt daemon is > already waiting for the script to exit. > > A deadlock is likely to occur. > > https://libvirt.org/hooks.html#calling-libvirt-functions-from-within-a-hook-script > > Preferrably summarize what the patch is doing or suggest something that > is not strongly discouraged by the docs. > > > Signed-off-by: Sun Feng > > --- > > docs/hooks.rst | 3 ++- > > src/qemu/qemu_process.c | 3 ++- > > 2 files changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/docs/hooks.rst b/docs/hooks.rst > > index 1dbc492bd4..45856e4ca4 100644 > > --- a/docs/hooks.rst > > +++ b/docs/hooks.rst > > @@ -204,7 +204,8 @@ operation. There is no specific operation to indicate a > > "restart" is occurring. > > > > - When a QEMU guest is stopped, the qemu hook script is called in two > > locations, to match the startup. First, :since:`since 0.8.0`, the hook > > is > > - called before libvirt restores any labels: > > + called before libvirt restores any labels, :since:`since 9.10.0`, > > shutoff > > + reason is delivered with **extra argument**: > > Next release is going to be 10.3.0 > > > > > :: > > > > diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c > > index e4bcb628cf..c42f5c9139 100644 > > --- a/src/qemu/qemu_process.c > > +++ b/src/qemu/qemu_process.c > > @@ -8564,7 +8564,8 @@ void qemuProcessStop(virQEMUDriver *driver, > > /* we can't stop the operation even if the script raised an error > > */ > > ignore_value(virHookCall(VIR_HOOK_DRIVER_QEMU, vm->def->name, > > VIR_HOOK_QEMU_OP_STOPPED, > > VIR_HOOK_SUBOP_END, > > - NULL, xml, NULL)); > > + > > virDomainShutoffReasonTypeToString(reason), > > + xml, NULL)); > > To be honest I'm not exactly sold that this is needed as the reason for > this hook's existence is to simply clear up any resources that would be > set up by the start hook, thus it's usually irrelevant why that > happened. The thing that would convince me is that in outgoing migration there are some resources you might wish to skip cleanup of if they need to be preserved in some manner for the dst host. IIUC we don't currently have a way to indicate that the "stopped" hook is in response to an outgoing migration, and the "reason" would provide this. With that justification though, I would now query the asymmetry of only providing 'reason' to the 'stopped' hook. It should be provided to *all* the hooks - prepare, start, started, stop, stopped, release. With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| ___ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-le...@lists.libvirt.org
Re: [PATCH] network: ensure nparams is non-negative
On a Tuesday in 2024, Daniel P. Berrangé wrote:
On Tue, Apr 16, 2024 at 12:58:53PM +0200, Ján Tomko wrote:
On a Tuesday in 2024, Daniel P. Berrangé wrote:
> The typed parameter array must be either 0, or a positive
> number.
>
Does this matter?
The API documentation says:
* @nparams: pointer to received number of interface parameter
and it looks like we ignore the number as long as params is NULL.
This missing check is something I noticed when fixing the recent
CVE about RPC checking nparams. In all other APIs we have such
a virCheckNonNegativeArgGoto for '*nparams', this was the only
one that was missing.
I believe it is harmless in terms of risk to libvirt/libvirtd,
but it might lead to better detection/reporting of bugs in apps.
I was thinking someone initializing *nparams to -1 could be a valid use
according to the documentation (with params = NULL), but if we do it the
same for all other similar APIs, it's okay.
However, it seems 'nparams' being NULL is not valid whether it's
the user or libvirt allocating the array, so please also add:
virCheckNonNullArgGoto(nparams, error);
Reviewed-by: Ján Tomko
Jano
> Signed-off-by: Daniel P. Berrangé
> ---
> src/libvirt-network.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/src/libvirt-network.c b/src/libvirt-network.c
> index ef17a8a04d..e467716b6a 100644
> --- a/src/libvirt-network.c
> +++ b/src/libvirt-network.c
> @@ -1577,6 +1577,8 @@ virNetworkPortGetParameters(virNetworkPortPtr port,
> virCheckNetworkPortReturn(port, -1);
> conn = port->net->conn;
>
> +virCheckNonNegativeArgGoto(*nparams, error);
> +
> if (conn->networkDriver && conn->networkDriver->networkPortGetParameters)
{
> int ret;
> ret = conn->networkDriver->networkPortGetParameters(port, params,
nparams, flags);
> --
> 2.43.0
> ___
> Devel mailing list -- devel@lists.libvirt.org
> To unsubscribe send an email to devel-le...@lists.libvirt.org
With regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
signature.asc
Description: PGP signature
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org
Re: [PATCH] network: ensure nparams is non-negative
On Wed, Apr 17, 2024 at 02:03:38PM +0200, Ján Tomko wrote: > On a Tuesday in 2024, Daniel P. Berrangé wrote: > > On Tue, Apr 16, 2024 at 12:58:53PM +0200, Ján Tomko wrote: > > > On a Tuesday in 2024, Daniel P. Berrangé wrote: > > > > The typed parameter array must be either 0, or a positive > > > > number. > > > > > > > > > > Does this matter? > > > > > > The API documentation says: > > > * @nparams: pointer to received number of interface parameter > > > > > > and it looks like we ignore the number as long as params is NULL. > > > > This missing check is something I noticed when fixing the recent > > CVE about RPC checking nparams. In all other APIs we have such > > a virCheckNonNegativeArgGoto for '*nparams', this was the only > > one that was missing. > > > > I believe it is harmless in terms of risk to libvirt/libvirtd, > > but it might lead to better detection/reporting of bugs in apps. > > > > I was thinking someone initializing *nparams to -1 could be a valid use > according to the documentation (with params = NULL), but if we do it the > same for all other similar APIs, it's okay. > > However, it seems 'nparams' being NULL is not valid whether it's > the user or libvirt allocating the array, so please also add: > > virCheckNonNullArgGoto(nparams, error); Opps yes, all the other APIs have this check, so should have it here too. > > Reviewed-by: Ján Tomko > With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| ___ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-le...@lists.libvirt.org
Re: [PATCH 1/2] qemu: Fix migration with custom XML
On Wed, Apr 17, 2024 at 09:15:39 +0200, Peter Krempa wrote:
> On Tue, Apr 16, 2024 at 19:53:25 +0200, Jiri Denemark wrote:
> > Ages ago origCPU in domain private data was introduced to provide
> > backward compatibility when migrating to an old libvirt, which did not
> > support fetching updated CPU definition from QEMU. Thus origCPU will
> > contain the original CPU definition before such update. But only if the
> > update actually changed anything. Let's always fill origCPU with the
> > original definition when starting a domain so that we can rely on it
> > being always set, even if it matches the updated definition.
> >
> > This fixes migration or save operations with custom domain XML after
> > commit v10.1.0-88-g14d3517410, which expected origCPU to be always set
> > to the CPU definition from inactive XML to check features explicitly
> > requested by a user.
> >
> > https://issues.redhat.com/browse/RHEL-30622
> >
> > Signed-off-by: Jiri Denemark
> > ---
> > src/qemu/qemu_domain.c | 38 +-
> > src/qemu/qemu_process.c | 14 ++
> > 2 files changed, 23 insertions(+), 29 deletions(-)
> >
> > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> > index 6b869797a8..ca50d435d2 100644
> > --- a/src/qemu/qemu_domain.c
> > +++ b/src/qemu/qemu_domain.c
> > @@ -11013,15 +11014,25 @@ qemuDomainUpdateCPU(virDomainObj *vm,
> >
> > *origCPU = NULL;
> >
> > -if (!cpu || !vm->def->cpu ||
> > -!virQEMUCapsGet(priv->qemuCaps,
> > QEMU_CAPS_QUERY_CPU_MODEL_EXPANSION) ||
> > -virCPUDefIsEqual(vm->def->cpu, cpu, false))
> > +if (!vm->def->cpu)
> > return 0;
> >
> > -if (!(cpu = virCPUDefCopy(cpu)))
> > +if (!virQEMUCapsGet(priv->qemuCaps,
> > QEMU_CAPS_QUERY_CPU_MODEL_EXPANSION))
> > +return 0;
> > +
> > +/* nothing to do if only topology part of CPU def is used */
> > +if (vm->def->cpu->mode == VIR_CPU_MODE_CUSTOM && !vm->def->cpu->model)
> > +return 0;
> > +
> > +if (cpu)
> > +cpu = virCPUDefCopy(cpu);
> > +else
> > +cpu = virCPUDefCopy(vm->def->cpu);
> > +
> > +if (!cpu)
> > return -1;
>
>
> First two reads were very confusing as this is overwriting the argument
>
>
> >
> > -VIR_DEBUG("Replacing CPU def with the updated one");
> > +VIR_DEBUG("Replacing CPU definition");
> >
> > *origCPU = vm->def->cpu;
> > vm->def->cpu = cpu;
>
> ... just to store it here. At this point it's no longer necessary. You
> can first move the old def into 'origCPU' and then directly assign into
> the variable to avoid the overwrite for readability.
Oh right, virCPUDefCopy can never return NULL.
>
> > @@ -11064,13 +11075,6 @@ qemuDomainFixupCPUs(virDomainObj *vm,
> > !vm->def->cpu->model)
> > return 0;
> >
> > -/* Missing origCPU means QEMU created exactly the same virtual CPU
> > which
> > - * we asked for or libvirt was too old to mess up the translation from
> > - * host-model.
> > - */
> > -if (!*origCPU)
> > -return 0;
>
> This function is called from two places:
>
> src/qemu/qemu_process.c=qemuProcessStartWithMemoryState(virConnectPtr conn,
> src/qemu/qemu_process.c:qemuDomainFixupCPUs(vm, &cookie->cpu) < 0)
Oops, nice catch. Can I pretend I added this hunk to check if reviewers
pay enough attention? :-) The call is guarded by cookie != NULL, but no
check for cookie->cpu is done. I'll drop this hunk.
> src/qemu/qemu_process.c=qemuProcessRefreshCPU(virQEMUDriver *driver,
> src/qemu/qemu_process.c:if (qemuDomainFixupCPUs(vm, &priv->origCPU) < > 0
>
> 'priv->origCPU' which is used in the latter is AFAIU filled only in
> 'qemuDomainUpdateCPU' called when starting up a qemu process and in
> 'qemuDomainObjPrivateXMLParse' when loading a status XML.
>
> In both cases there are code paths which may leave 'priv->origCPU' to be
> NULL.
>
> The following code will dereference it unconditionally:
>
> if (virCPUDefFindFeature(*origCPU, "cmt")) {
This specific issue would not exist if I didn't forget to update one
more place... see below.
> > diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> > index e4bcb628cf..8165c28dbc 100644
> > --- a/src/qemu/qemu_process.c
> > +++ b/src/qemu/qemu_process.c
> > @@ -4449,19 +4447,11 @@ qemuProcessUpdateLiveGuestCPU(virDomainObj *vm,
> > !def->cpu->model))
> > return 0;
> >
> > -orig = virCPUDefCopy(def->cpu);
> > -
> > -if ((rc = virCPUUpdateLive(def->os.arch, def->cpu, enabled, disabled))
> > < 0) {
> > +if ((rc = virCPUUpdateLive(def->os.arch, def->cpu, enabled, disabled))
> > < 0)
> > return -1;
> > -} else if (rc == 0) {
> > -/* Store the original CPU in priv if QEMU changed it and we didn't
> > - * get the original CPU via migration, restore, or snapshot revert.
> > - */
> > -if (!priv->origCPU && !virCPUDefIsEqual(def->cpu, orig
[PATCH v2 0/4] qemu: Fix migration with custom XML
Version 2: - see patch 1/4 for more details - added two cleanups Jiri Denemark (4): qemu: Fix migration with custom XML NEWS: Mention migration bug with custom XML qemu: Change return type of qemuDomainUpdateCPU to void qemu: Change return type of qemuDomainFixupCPUs to void NEWS.rst| 8 +++ src/qemu/qemu_domain.c | 51 - src/qemu/qemu_domain.h | 4 ++-- src/qemu/qemu_process.c | 29 --- 4 files changed, 45 insertions(+), 47 deletions(-) -- 2.44.0 ___ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-le...@lists.libvirt.org
[PATCH v2 1/4] qemu: Fix migration with custom XML
Ages ago origCPU in domain private data was introduced to provide
backward compatibility when migrating to an old libvirt, which did not
support fetching updated CPU definition from QEMU. Thus origCPU will
contain the original CPU definition before such update. But only if the
update actually changed anything. Let's always fill origCPU with the
original definition when starting a domain so that we can rely on it
being always set, even if it matches the updated definition.
This fixes migration or save operations with custom domain XML after
commit v10.1.0-88-g14d3517410, which expected origCPU to be always set
to the CPU definition from inactive XML to check features explicitly
requested by a user.
https://issues.redhat.com/browse/RHEL-30622
Signed-off-by: Jiri Denemark
Tested-by: Han Han
---
Notes:
Version 2:
- virCPUDefCopy never returns NULL
- do not remove !*origCPU check in qemuDomainFixupCPUs
- make sure origCPU is set when reconnecting to running domains
src/qemu/qemu_domain.c | 33 -
src/qemu/qemu_process.c | 19 +++
2 files changed, 27 insertions(+), 25 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 6b869797a8..d91c7d478b 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -10995,12 +10995,13 @@ virSaveCookieCallbacks virQEMUDriverDomainSaveCookie
= {
* @vm: domain which is being started
* @cpu: CPU updated when the domain was running previously (before migration,
* snapshot, or save)
- * @origCPU: where to store the original CPU from vm->def in case @cpu was
- * used instead
+ * @origCPU: where to store the original CPU from vm->def
*
- * Replace the CPU definition with the updated one when QEMU is new enough to
- * allow us to check extra features it is about to enable or disable when
- * starting a domain. The original CPU is stored in @origCPU.
+ * Save the original CPU definition from inactive XML in @origCPU so that we
+ * can safely update it and still be able to check what exactly a user asked
+ * for. The domain definition will either contain a copy of the original CPU
+ * definition or a copy of @cpu in case the domain was already running and
+ * we're just restoring a saved state or preparing for incoming migration.
*
* Returns 0 on success, -1 on error.
*/
@@ -11013,18 +11014,24 @@ qemuDomainUpdateCPU(virDomainObj *vm,
*origCPU = NULL;
-if (!cpu || !vm->def->cpu ||
-!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_QUERY_CPU_MODEL_EXPANSION) ||
-virCPUDefIsEqual(vm->def->cpu, cpu, false))
+if (!vm->def->cpu)
return 0;
-if (!(cpu = virCPUDefCopy(cpu)))
-return -1;
+if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_QUERY_CPU_MODEL_EXPANSION))
+return 0;
+
+/* nothing to do if only topology part of CPU def is used */
+if (vm->def->cpu->mode == VIR_CPU_MODE_CUSTOM && !vm->def->cpu->model)
+return 0;
+
+VIR_DEBUG("Replacing CPU definition");
-VIR_DEBUG("Replacing CPU def with the updated one");
+*origCPU = g_steal_pointer(&vm->def->cpu);
-*origCPU = vm->def->cpu;
-vm->def->cpu = cpu;
+if (cpu)
+vm->def->cpu = virCPUDefCopy(cpu);
+else
+vm->def->cpu = virCPUDefCopy(*origCPU);
return 0;
}
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index e4bcb628cf..f3e295ccf0 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -4437,8 +4437,6 @@ qemuProcessUpdateLiveGuestCPU(virDomainObj *vm,
virCPUData *disabled)
{
virDomainDef *def = vm->def;
-qemuDomainObjPrivate *priv = vm->privateData;
-g_autoptr(virCPUDef) orig = NULL;
int rc;
if (!enabled)
@@ -4449,19 +4447,11 @@ qemuProcessUpdateLiveGuestCPU(virDomainObj *vm,
!def->cpu->model))
return 0;
-orig = virCPUDefCopy(def->cpu);
-
-if ((rc = virCPUUpdateLive(def->os.arch, def->cpu, enabled, disabled)) <
0) {
+if ((rc = virCPUUpdateLive(def->os.arch, def->cpu, enabled, disabled)) < 0)
return -1;
-} else if (rc == 0) {
-/* Store the original CPU in priv if QEMU changed it and we didn't
- * get the original CPU via migration, restore, or snapshot revert.
- */
-if (!priv->origCPU && !virCPUDefIsEqual(def->cpu, orig, false))
-priv->origCPU = g_steal_pointer(&orig);
+if (rc == 0)
def->cpu->check = VIR_CPU_CHECK_FULL;
-}
return 0;
}
@@ -9147,6 +9137,11 @@ qemuProcessReconnect(void *opaque)
qemuDomainVcpuPersistOrder(obj->def);
+/* Make sure priv->origCPU is always set. */
+if (!priv->origCPU &&
+qemuDomainUpdateCPU(obj, NULL, &priv->origCPU) < 0)
+goto error;
+
if (qemuProcessRefreshCPU(driver, obj) < 0)
goto error;
--
2.44.0
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe
[PATCH v2 2/4] NEWS: Mention migration bug with custom XML
Signed-off-by: Jiri Denemark Reviewed-by: Peter Krempa --- NEWS.rst | 8 1 file changed, 8 insertions(+) diff --git a/NEWS.rst b/NEWS.rst index c108e66f46..852dadf532 100644 --- a/NEWS.rst +++ b/NEWS.rst @@ -21,6 +21,14 @@ v10.3.0 (unreleased) * **Bug fixes** + * qemu: Fix migration with custom XML + +Libvirt 10.2.0 would sometimes complain about incompatible CPU definition +when trying to migrate or save a domain and passing a custom XML even +though such XML was properly generated as migratable. Hitting this bug +depends on the guest CPU definition and the host on which a particular +domain was running. + v10.2.0 (2024-04-02) -- 2.44.0 ___ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-le...@lists.libvirt.org
[PATCH v2 3/4] qemu: Change return type of qemuDomainUpdateCPU to void
The function never fails.
Signed-off-by: Jiri Denemark
---
src/qemu/qemu_domain.c | 12
src/qemu/qemu_domain.h | 2 +-
src/qemu/qemu_process.c | 8 +++-
3 files changed, 8 insertions(+), 14 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index d91c7d478b..3dfabfda2f 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -11002,10 +11002,8 @@ virSaveCookieCallbacks virQEMUDriverDomainSaveCookie =
{
* for. The domain definition will either contain a copy of the original CPU
* definition or a copy of @cpu in case the domain was already running and
* we're just restoring a saved state or preparing for incoming migration.
- *
- * Returns 0 on success, -1 on error.
*/
-int
+void
qemuDomainUpdateCPU(virDomainObj *vm,
virCPUDef *cpu,
virCPUDef **origCPU)
@@ -11015,14 +11013,14 @@ qemuDomainUpdateCPU(virDomainObj *vm,
*origCPU = NULL;
if (!vm->def->cpu)
-return 0;
+return;
if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_QUERY_CPU_MODEL_EXPANSION))
-return 0;
+return;
/* nothing to do if only topology part of CPU def is used */
if (vm->def->cpu->mode == VIR_CPU_MODE_CUSTOM && !vm->def->cpu->model)
-return 0;
+return;
VIR_DEBUG("Replacing CPU definition");
@@ -11032,8 +11030,6 @@ qemuDomainUpdateCPU(virDomainObj *vm,
vm->def->cpu = virCPUDefCopy(cpu);
else
vm->def->cpu = virCPUDefCopy(*origCPU);
-
-return 0;
}
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index bd37cb245a..264817eef9 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -980,7 +980,7 @@ virStorageSource *qemuDomainGetStorageSourceByDevstr(const
char *devstr,
virDomainDef *def,
virDomainBackupDef
*backupdef);
-int
+void
qemuDomainUpdateCPU(virDomainObj *vm,
virCPUDef *cpu,
virCPUDef **origCPU);
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index f3e295ccf0..7fdb9ac23a 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -5674,8 +5674,7 @@ qemuProcessInit(virQEMUDriver *driver,
if (qemuProcessPrepareQEMUCaps(vm, driver->qemuCapsCache) < 0)
return -1;
-if (qemuDomainUpdateCPU(vm, updatedCPU, &origCPU) < 0)
-return -1;
+qemuDomainUpdateCPU(vm, updatedCPU, &origCPU);
if (qemuProcessStartValidate(driver, vm, priv->qemuCaps, flags) < 0)
return -1;
@@ -9138,9 +9137,8 @@ qemuProcessReconnect(void *opaque)
qemuDomainVcpuPersistOrder(obj->def);
/* Make sure priv->origCPU is always set. */
-if (!priv->origCPU &&
-qemuDomainUpdateCPU(obj, NULL, &priv->origCPU) < 0)
-goto error;
+if (!priv->origCPU)
+qemuDomainUpdateCPU(obj, NULL, &priv->origCPU);
if (qemuProcessRefreshCPU(driver, obj) < 0)
goto error;
--
2.44.0
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org
[PATCH v2 4/4] qemu: Change return type of qemuDomainFixupCPUs to void
The function never fails.
Signed-off-by: Jiri Denemark
---
Notes:
Version 2:
- virCPUDefCopy never returns NULL
- do not remove !*origCPU check in qemuDomainFixupCPUs
- make sure origCPU is set when reconnecting to running domains
src/qemu/qemu_domain.c | 12
src/qemu/qemu_domain.h | 2 +-
src/qemu/qemu_process.c | 8 +++-
3 files changed, 8 insertions(+), 14 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 3dfabfda2f..3469f0d40c 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -11050,29 +11050,27 @@ qemuDomainUpdateCPU(virDomainObj *vm,
*
* This function can only be used on an active domain or when restoring a
* domain which was running.
- *
- * Returns 0 on success, -1 on error.
*/
-int
+void
qemuDomainFixupCPUs(virDomainObj *vm,
virCPUDef **origCPU)
{
virArch arch = vm->def->os.arch;
if (!ARCH_IS_X86(arch))
-return 0;
+return;
if (!vm->def->cpu ||
vm->def->cpu->mode != VIR_CPU_MODE_CUSTOM ||
!vm->def->cpu->model)
-return 0;
+return;
/* Missing origCPU means QEMU created exactly the same virtual CPU which
* we asked for or libvirt was too old to mess up the translation from
* host-model.
*/
if (!*origCPU)
-return 0;
+return;
if (virCPUDefFindFeature(vm->def->cpu, "cmt")) {
g_autoptr(virCPUDef) fixedCPU =
virCPUDefCopyWithoutModel(vm->def->cpu);
@@ -11093,8 +11091,6 @@ qemuDomainFixupCPUs(virDomainObj *vm,
virCPUDefFree(*origCPU);
*origCPU = g_steal_pointer(&fixedOrig);
}
-
-return 0;
}
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index 264817eef9..a3089ea449 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -985,7 +985,7 @@ qemuDomainUpdateCPU(virDomainObj *vm,
virCPUDef *cpu,
virCPUDef **origCPU);
-int
+void
qemuDomainFixupCPUs(virDomainObj *vm,
virCPUDef **origCPU);
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 7fdb9ac23a..686f6ed6c1 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -8278,9 +8278,8 @@ qemuProcessStartWithMemoryState(virConnectPtr conn,
/* No cookie means libvirt which saved the domain was too old to mess up
* the CPU definitions.
*/
-if (cookie &&
-qemuDomainFixupCPUs(vm, &cookie->cpu) < 0)
-return -1;
+if (cookie)
+qemuDomainFixupCPUs(vm, &cookie->cpu);
if (cookie && !cookie->slirpHelper)
priv->disableSlirp = true;
@@ -8926,8 +8925,7 @@ qemuProcessRefreshCPU(virQEMUDriver *driver,
* case the host-model is known to not contain features which QEMU
* doesn't know about.
*/
-if (qemuDomainFixupCPUs(vm, &priv->origCPU) < 0)
-return -1;
+qemuDomainFixupCPUs(vm, &priv->origCPU);
}
return 0;
--
2.44.0
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org
Re: [PATCH v2 1/4] qemu: Fix migration with custom XML
On Wed, Apr 17, 2024 at 14:36:08 +0200, Jiri Denemark wrote: > Ages ago origCPU in domain private data was introduced to provide > backward compatibility when migrating to an old libvirt, which did not > support fetching updated CPU definition from QEMU. Thus origCPU will > contain the original CPU definition before such update. But only if the > update actually changed anything. Let's always fill origCPU with the > original definition when starting a domain so that we can rely on it > being always set, even if it matches the updated definition. > > This fixes migration or save operations with custom domain XML after > commit v10.1.0-88-g14d3517410, which expected origCPU to be always set > to the CPU definition from inactive XML to check features explicitly > requested by a user. > > https://issues.redhat.com/browse/RHEL-30622 > > Signed-off-by: Jiri Denemark > Tested-by: Han Han > --- [...] > @@ -9147,6 +9137,11 @@ qemuProcessReconnect(void *opaque) > > qemuDomainVcpuPersistOrder(obj->def); > > +/* Make sure priv->origCPU is always set. */ Comment is misleading because there are situations when it still will be NULL. > +if (!priv->origCPU && > +qemuDomainUpdateCPU(obj, NULL, &priv->origCPU) < 0) > +goto error; > + > if (qemuProcessRefreshCPU(driver, obj) < 0) > goto error; With the comment fixed: Reviewed-by: Peter Krempa ___ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-le...@lists.libvirt.org
Re: [PATCH v2 3/4] qemu: Change return type of qemuDomainUpdateCPU to void
On Wed, Apr 17, 2024 at 14:36:10 +0200, Jiri Denemark wrote: > The function never fails. > > Signed-off-by: Jiri Denemark > --- > src/qemu/qemu_domain.c | 12 > src/qemu/qemu_domain.h | 2 +- > src/qemu/qemu_process.c | 8 +++- > 3 files changed, 8 insertions(+), 14 deletions(-) Reviewed-by: Peter Krempa ___ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-le...@lists.libvirt.org
Re: [PATCH v2 4/4] qemu: Change return type of qemuDomainFixupCPUs to void
On Wed, Apr 17, 2024 at 14:36:11 +0200, Jiri Denemark wrote: > The function never fails. > > Signed-off-by: Jiri Denemark > --- Reviewed-by: Peter Krempa ___ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-le...@lists.libvirt.org
[PATCH v2 0/5] qemu: Introduce shared_filesystems configuration option
The need to have something like this in the first place is driven by KubeVirt (see [1] and [2]). A draft version of this series has been integrated into KubeVirt and it has been confirmed that it was effective in removing the need to use LD_PRELOAD hacks in the storage provider. Changes from [v1]: * documented more explicitly that the newly introduced option is intended for very specific scenarios and not general usage; as part of this, the NEWS update has been dropped too; * made a few tweaks and addressed a few oversight based on review feedback; * several preparatory cleanup patches have been pushed. Changes from [v0]: * reworked approach. CC'ing Stefan so he can have a look at the TPM part and shout if I've gotten anything wrong :) [v1] https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/XEISMPGRJHFRT4LZ3MJ3L3XR7OPOQKPM/ [v0] https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/MMKVR54LD3SDG5CMSXUECV7I57LMJJTH/ [1] https://issues.redhat.com/browse/CNV-34322 [2] https://issues.redhat.com/browse/CNV-39370 Andrea Bolognani (5): security: Fix alignment qemu: Introduce shared_filesystems configuration option qemu: Propagate shared_filesystems utils: Use overrides in virFileIsSharedFS() qemu: Always set labels for TPM state src/lxc/lxc_controller.c | 3 +- src/lxc/lxc_driver.c | 2 +- src/lxc/lxc_process.c | 4 +- src/qemu/libvirtd_qemu.aug | 3 ++ src/qemu/qemu.conf.in | 23 src/qemu/qemu_conf.c | 17 ++ src/qemu/qemu_conf.h | 2 + src/qemu/qemu_domain.c | 7 ++- src/qemu/qemu_extdevice.c | 2 +- src/qemu/qemu_migration.c | 23 src/qemu/qemu_security.c | 85 +++--- src/qemu/qemu_tpm.c| 38 +++-- src/qemu/qemu_tpm.h| 10 ++-- src/qemu/test_libvirtd_qemu.aug.in | 5 ++ src/security/security_apparmor.c | 2 + src/security/security_dac.c| 47 + src/security/security_driver.h | 8 ++- src/security/security_manager.c| 33 +--- src/security/security_manager.h| 9 +++- src/security/security_nop.c| 5 ++ src/security/security_selinux.c| 56 +++- src/security/security_stack.c | 32 --- src/util/virfile.c | 53 +-- src/util/virfile.h | 3 +- tests/securityselinuxlabeltest.c | 2 +- tests/virfiletest.c| 2 +- 26 files changed, 370 insertions(+), 106 deletions(-) -- 2.44.0 ___ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-le...@lists.libvirt.org
[PATCH v2 1/5] security: Fix alignment
Signed-off-by: Andrea Bolognani
---
src/security/security_selinux.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index aaec34ff8b..a4915dbc89 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -1983,9 +1983,9 @@
virSecuritySELinuxSetImageLabelInternal(virSecurityManager *mgr,
static int
virSecuritySELinuxSetImageLabel(virSecurityManager *mgr,
- virDomainDef *def,
- virStorageSource *src,
- virSecurityDomainImageLabelFlags flags)
+virDomainDef *def,
+virStorageSource *src,
+virSecurityDomainImageLabelFlags flags)
{
virStorageSource *parent = src;
virStorageSource *n;
--
2.44.0
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org
[PATCH v2 4/5] utils: Use overrides in virFileIsSharedFS()
If the local admin has explicitly declared that a certain
filesystem is to be considered shared, we should treat it as
such.
Signed-off-by: Andrea Bolognani
---
src/util/virfile.c | 42 +-
1 file changed, 41 insertions(+), 1 deletion(-)
diff --git a/src/util/virfile.c b/src/util/virfile.c
index 3268866f8b..45815919d6 100644
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
@@ -3801,9 +3801,49 @@ virFileGetDefaultHugepage(virHugeTLBFS *fs,
return NULL;
}
+static bool
+virFileIsSharedFSOverride(const char *path,
+ char *const *overrides)
+{
+g_autofree char *dirpath = NULL;
+char *p = NULL;
+
+if (!path || path[0] != '/' || !overrides)
+return false;
+
+if (g_strv_contains((const char *const *) overrides, path))
+return true;
+
+dirpath = g_strdup(path);
+
+/* Continue until we've scanned the entire path */
+while (p != dirpath) {
+
+/* Find the last slash */
+if ((p = strrchr(dirpath, '/')) == NULL)
+break;
+
+/* Truncate the path by overwriting the slash that we've just
+ * found with a null byte. If it is the very first slash in
+ * the path, we need to handle things slightly differently */
+if (p == dirpath)
+*(p+1) = '\0';
+else
+*p = '\0';
+
+if (g_strv_contains((const char *const *) overrides, dirpath))
+return true;
+}
+
+return false;
+}
+
int virFileIsSharedFS(const char *path,
- char *const *overrides G_GNUC_UNUSED)
+ char *const *overrides)
{
+if (virFileIsSharedFSOverride(path, overrides))
+return 1;
+
return virFileIsSharedFSType(path,
VIR_FILE_SHFS_NFS |
VIR_FILE_SHFS_GFS2 |
--
2.44.0
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org
[PATCH v2 2/5] qemu: Introduce shared_filesystems configuration option
As explained in the comment, this can help in scenarios where
a shared filesystem can't be detected as such by libvirt, by
giving the admin the opportunity to provide this information
manually.
Signed-off-by: Andrea Bolognani
---
src/qemu/libvirtd_qemu.aug | 3 +++
src/qemu/qemu.conf.in | 23 +++
src/qemu/qemu_conf.c | 17 +
src/qemu/qemu_conf.h | 2 ++
src/qemu/test_libvirtd_qemu.aug.in | 5 +
5 files changed, 50 insertions(+)
diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug
index 2b6526538f..1377fd89cc 100644
--- a/src/qemu/libvirtd_qemu.aug
+++ b/src/qemu/libvirtd_qemu.aug
@@ -143,6 +143,8 @@ module Libvirtd_qemu =
let storage_entry = bool_entry "storage_use_nbdkit"
+ let filesystem_entry = str_array_entry "shared_filesystems"
+
(* Entries that used to exist in the config which are now
* deleted. We keep on parsing them so we don't break
* ability to parse old configs after upgrade
@@ -173,6 +175,7 @@ module Libvirtd_qemu =
| swtpm_entry
| capability_filters_entry
| storage_entry
+ | filesystem_entry
| obsolete_entry
let comment = [ label "#comment" . del /#[ \t]*/ "# " . store /([^
\t\n][^\n]*)?/ . del /\n/ "\n" ]
diff --git a/src/qemu/qemu.conf.in b/src/qemu/qemu.conf.in
index f406df8749..c543a0a55b 100644
--- a/src/qemu/qemu.conf.in
+++ b/src/qemu/qemu.conf.in
@@ -986,3 +986,26 @@
# note that the default might change in future releases.
#
#storage_use_nbdkit = @USE_NBDKIT_DEFAULT@
+
+# libvirt will normally prevent migration if the storage backing the VM is not
+# on a shared filesystems. Sometimes, however, the storage *is* shared despite
+# not being detected as such: for example, this is the case when one of the
+# hosts involved in the migration is exporting its local storage to the other
+# one via NFS.
+#
+# Any directory listed here will be assumed to live on a shared filesystem,
+# making migration possible in scenarios such as the one described above.
+#
+# Due to how label remembering is implemented, it will generally be necessary
+# to set remember_owner=0 *on both sides of the migration* as well.
+#
+# NOTE: this option is intended to help in very specific scenarios that are
+# rarely encountered. If you find yourself reaching for this option, consider
+# reworking your environment so that it follows a more common architecture
+# rather than using it.
+#
+#shared_filesystems = [
+# "/path/to/images",
+# "/path/to/nvram",
+# "/path/to/swtpm"
+#]
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 4050a82341..01c6bcc793 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -374,6 +374,8 @@ static void virQEMUDriverConfigDispose(void *obj)
g_strfreev(cfg->capabilityfilters);
+g_strfreev(cfg->sharedFilesystems);
+
g_free(cfg->deprecationBehavior);
}
@@ -1084,6 +1086,18 @@ virQEMUDriverConfigLoadStorageEntry(virQEMUDriverConfig
*cfg,
}
+static int
+virQEMUDriverConfigLoadFilesystemEntry(virQEMUDriverConfig *cfg,
+ virConf *conf)
+{
+if (virConfGetValueStringList(conf, "shared_filesystems", false,
+ &cfg->sharedFilesystems) < 0)
+return -1;
+
+return 0;
+}
+
+
int virQEMUDriverConfigLoadFile(virQEMUDriverConfig *cfg,
const char *filename,
bool privileged)
@@ -1158,6 +1172,9 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfig *cfg,
if (virQEMUDriverConfigLoadStorageEntry(cfg, conf) < 0)
return -1;
+if (virQEMUDriverConfigLoadFilesystemEntry(cfg, conf) < 0)
+return -1;
+
return 0;
}
diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
index 36049b4bfa..b53d56be02 100644
--- a/src/qemu/qemu_conf.h
+++ b/src/qemu/qemu_conf.h
@@ -233,6 +233,8 @@ struct _virQEMUDriverConfig {
bool storageUseNbdkit;
virQEMUSchedCore schedCore;
+
+char **sharedFilesystems;
};
G_DEFINE_AUTOPTR_CLEANUP_FUNC(virQEMUDriverConfig, virObjectUnref);
diff --git a/src/qemu/test_libvirtd_qemu.aug.in
b/src/qemu/test_libvirtd_qemu.aug.in
index b97e6de11e..69fdae215a 100644
--- a/src/qemu/test_libvirtd_qemu.aug.in
+++ b/src/qemu/test_libvirtd_qemu.aug.in
@@ -119,3 +119,8 @@ module Test_libvirtd_qemu =
{ "deprecation_behavior" = "none" }
{ "sched_core" = "none" }
{ "storage_use_nbdkit" = "@USE_NBDKIT_DEFAULT@" }
+{ "shared_filesystems"
+{ "1" = "/path/to/images" }
+{ "2" = "/path/to/nvram" }
+{ "3" = "/path/to/swtpm" }
+}
--
2.44.0
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org
[PATCH v2 5/5] qemu: Always set labels for TPM state
Up until this point, we have avoided setting labels for
incoming migration when the TPM state is stored on a shared
filesystem. This seems to make sense, because since the
underlying storage is shared surely the labels will be as
well.
There's one problem, though: when a guest is migrated, the
SELinux context for the destination process is different from
the one of the source process.
We haven't hit any issues with the current approach so far
because NFS doesn't support SELinux, so effectively it doesn't
matter whether relabeling happens or not: even if the SELinux
contexts of the source and target processes are different,
both will be able to access the storage.
Now that it's possible for the local admin to manually mark
exported directories as shared filesystems, however, things
can get problematic.
Consider the case in which one host (mig-one) exports its
local filesystem /srv/nfs/libvirt/swtpm via NFS, and at the
same time bind-mounts it to /var/lib/libvirt/swtpm; another
host (mig-two) mounts the same filesystem to the same
location, this time via NFS. Additionally, in order to
allow migration in both directions, on mig-one the
/var/lib/libvirt/swtpm directory is listed in the
shared_filesystems qemu.conf option.
When migrating from mig-one to mig-two, things work just fine;
going in the opposite direction, however, results in an error:
# virsh migrate cirros qemu+ssh://mig-one/system
error: internal error: QEMU unexpectedly closed the monitor (vm='cirros'):
qemu-system-x86_64: tpm-emulator: Setting the stateblob (type 1) failed with
a TPM error 0x1f
qemu-system-x86_64: error while loading state for instance 0x0 of device
'tpm-emulator'
qemu-system-x86_64: load of migration failed: Input/output error
This is because the directory on mig-one is considered a
shared filesystem and thus labeling is skipped, resulting in
a SELinux denial.
The solution is quite simple: remove the check and always
relabel. We know that it's okay to do so not just because it
makes the error seen above go away, but also because no such
check currently exists for disks and other types of persistent
storage such as NVRAM files, which always get relabeled.
Signed-off-by: Andrea Bolognani
---
src/qemu/qemu_tpm.c | 11 ++-
1 file changed, 2 insertions(+), 9 deletions(-)
diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c
index cdf4bfbad2..e5a9fb8b16 100644
--- a/src/qemu/qemu_tpm.c
+++ b/src/qemu/qemu_tpm.c
@@ -929,7 +929,6 @@ qemuTPMEmulatorStart(virQEMUDriver *driver,
g_autofree char *pidfile = NULL;
virTimeBackOffVar timebackoff;
const unsigned long long timeout = 1000; /* ms */
-bool setTPMStateLabel = true;
pid_t pid = -1;
cfg = virQEMUDriverGetConfig(driver);
@@ -956,13 +955,7 @@ qemuTPMEmulatorStart(virQEMUDriver *driver,
virCommandSetPidFile(cmd, pidfile);
virCommandSetErrorFD(cmd, &errfd);
-if (incomingMigration &&
-virFileIsSharedFS(tpm->data.emulator.storagepath,
cfg->sharedFilesystems) == 1) {
-/* security labels must have been set up on source already */
-setTPMStateLabel = false;
-}
-
-if (qemuSecuritySetTPMLabels(driver, vm, setTPMStateLabel) < 0)
+if (qemuSecuritySetTPMLabels(driver, vm, true) < 0)
return -1;
if (qemuSecurityCommandRun(driver, vm, cmd, cfg->swtpm_user,
@@ -1011,7 +1004,7 @@ qemuTPMEmulatorStart(virQEMUDriver *driver,
virProcessKillPainfully(pid, true);
if (pidfile)
unlink(pidfile);
-qemuSecurityRestoreTPMLabels(driver, vm, setTPMStateLabel);
+qemuSecurityRestoreTPMLabels(driver, vm, true);
return -1;
}
--
2.44.0
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org
[PATCH v2 3/5] qemu: Propagate shared_filesystems
virFileIsSharedFS() is the function that ultimately decides
whether a filesystem should be considered shared, but the list
of manually configured shared filesystems is part of the QEMU
driver's configuration, so we need to pass the information
through several layers in order to make use of it.
Note that with this change the list is propagated all the way
through, but its contents are still ignored, so the behavior
remains the same for now.
Signed-off-by: Andrea Bolognani
---
src/lxc/lxc_controller.c | 3 +-
src/lxc/lxc_driver.c | 2 +-
src/lxc/lxc_process.c| 4 +-
src/qemu/qemu_domain.c | 7 ++-
src/qemu/qemu_extdevice.c| 2 +-
src/qemu/qemu_migration.c| 23 -
src/qemu/qemu_security.c | 85
src/qemu/qemu_tpm.c | 29 +++
src/qemu/qemu_tpm.h | 10 ++--
src/security/security_apparmor.c | 2 +
src/security/security_dac.c | 47 ++
src/security/security_driver.h | 8 ++-
src/security/security_manager.c | 33 ++---
src/security/security_manager.h | 9 +++-
src/security/security_nop.c | 5 ++
src/security/security_selinux.c | 50 ++-
src/security/security_stack.c| 32 +---
src/util/virfile.c | 13 +++--
src/util/virfile.h | 3 +-
tests/securityselinuxlabeltest.c | 2 +-
tests/virfiletest.c | 2 +-
21 files changed, 276 insertions(+), 95 deletions(-)
diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c
index 505b71d05e..7b432a1160 100644
--- a/src/lxc/lxc_controller.c
+++ b/src/lxc/lxc_controller.c
@@ -1919,7 +1919,8 @@ static int virLXCControllerSetupDisk(virLXCController
*ctrl,
/* Labelling normally operates on src, but we need
* to actually label the dst here, so hack the config */
def->src->path = dst;
-if (virSecurityManagerSetImageLabel(securityDriver, ctrl->def, def->src,
+if (virSecurityManagerSetImageLabel(securityDriver,
+NULL, ctrl->def, def->src,
VIR_SECURITY_DOMAIN_IMAGE_LABEL_BACKING_CHAIN) < 0)
goto cleanup;
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
index 1842ae8f0e..1fda848b0c 100644
--- a/src/lxc/lxc_driver.c
+++ b/src/lxc/lxc_driver.c
@@ -3261,7 +3261,7 @@ lxcDomainAttachDeviceMknodHelper(pid_t pid G_GNUC_UNUSED,
char *tmpsrc = def->src->path;
def->src->path = data->file;
if (virSecurityManagerSetImageLabel(data->driver->securityManager,
-data->vm->def, def->src,
+NULL, data->vm->def, def->src,
VIR_SECURITY_DOMAIN_IMAGE_LABEL_BACKING_CHAIN) < 0) {
def->src->path = tmpsrc;
goto cleanup;
diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c
index 30ff4eb3d0..a7e8e2c28e 100644
--- a/src/lxc/lxc_process.c
+++ b/src/lxc/lxc_process.c
@@ -170,7 +170,7 @@ static void virLXCProcessCleanup(virLXCDriver *driver,
}
if (flags & VIR_LXC_PROCESS_CLEANUP_RESTORE_SECLABEL) {
-virSecurityManagerRestoreAllLabel(driver->securityManager,
+virSecurityManagerRestoreAllLabel(driver->securityManager, NULL,
vm->def, false, false);
}
@@ -1320,7 +1320,7 @@ int virLXCProcessStart(virLXCDriver * driver,
stopFlags |= VIR_LXC_PROCESS_CLEANUP_RELEASE_SECLABEL;
VIR_DEBUG("Setting domain security labels");
-if (virSecurityManagerSetAllLabel(driver->securityManager,
+if (virSecurityManagerSetAllLabel(driver->securityManager, NULL,
vm->def, NULL, false, false) < 0)
goto cleanup;
stopFlags |= VIR_LXC_PROCESS_CLEANUP_RESTORE_SECLABEL;
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 6b869797a8..59ed8f4107 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -11913,7 +11913,12 @@ virQEMUFileOpenAs(uid_t fallback_uid,
bool need_unlink = false;
unsigned int vfoflags = 0;
int fd = -1;
-int path_shared = virFileIsSharedFS(path);
+/* Note that it would be pointless to pass
+ * virQEMUDriverConfig.sharedFilesystems here, since those
+ * listed there are by definition paths that can be accessed
+ * as local from the current host. Thus, a second attempt at
+ * opening the file would not make a difference */
+int path_shared = virFileIsSharedFS(path, NULL);
uid_t uid = geteuid();
gid_t gid = getegid();
diff --git a/src/qemu/qemu_extdevice.c b/src/qemu/qemu_extdevice.c
index ed5976d1f7..dc1bb56237 100644
--- a/src/qemu/qemu_extdevice.c
+++ b/src/qemu/qemu_extdevice.c
@@ -165,7 +165,7 @@ qemuExtDevicesCleanupHost(virQEMUDriver *driver,
virDomainTPMDef *tpm = def->tpms[i];
if (tpm->type == VIR_DOMAIN_T
Re: [PATCH 06/10] qemu: Introduce shared_filesystems configuration option
On Tue, Mar 26, 2024 at 08:54:03AM -0700, Andrea Bolognani wrote: > On Wed, Mar 20, 2024 at 08:43:24AM -0700, Andrea Bolognani wrote: > > On Wed, Mar 20, 2024 at 12:37:37PM +0100, Peter Krempa wrote: > > > On Wed, Mar 20, 2024 at 10:19:11 +0100, Andrea Bolognani wrote: > > > > +# libvirt will normally prevent migration if the storage backing the > > > > VM is not > > > > +# on a shared filesystems. Sometimes, however, the storage *is* shared > > > > despite > > > > +# not being detected as such: for example, this is the case when one > > > > of the > > > > +# hosts involved in the migration is exporting its local storage to > > > > the other > > > > +# one via NFS. > > > > +# > > > > +# Any directory listed here will be assumed to live on a shared > > > > filesystem, > > > > +# making migration possible in scenarios such as the one described > > > > above. > > > > +# > > > > +# If you need this feature, you probably want to set remember_owner=0 > > > > too. > > > > > > Could you please elaborate why you'd want to disable owner remembering? > > > With remote filesystems this works so I expect that if this makes > > > certain paths behave as shared filesystems, they should behave such > > > without any additional tweaks > > > > To be quite honest I don't remember exactly why I've added that, but > > I can confirm that if remember_owner=0 is not used on the destination > > host then migration will stall for a bit and then fail with > > > > error: unable to lock /var/lib/libvirt/swtpm/.../tpm2/.lock for > > metadata change: Resource temporarily unavailable > > > > Things work fine if swtpm is not involved. I'm going to dig deeper, > > but my guess is that, just like the situation addressed by the last > > patch, having an additional process complicates things compared to > > when we need to worry about QEMU only. > > I've managed to track this down, and I wasn't far off. > > The issue is that, when remember_owner is enabled, we perform a bunch > of additional locking on files around the actual relabel operation; > specifically, we call virSecurityManagerTransactionCommit() with the > last argument set to true. > > This doesn't seem to cause any issues in general, *except* when it > comes to swtpm's storage lock. I take this back. Further testing has confirmed that there are other scenarios in which this is problematic, for example NVRAM files. Even with the TPM locking issue out of the way, we still have to deal with an underlying issue: label remembering is implemented by setting custom XATTRs on the files involved, and NFS doesn't support XATTRs. So what will happen when remember_owner=1 and the VM getting started on the host that has local access to the filesystem is, that these XATTRs will get set and not cleared up on migration; the VM will make it safely to the other host. Then, when you attempt to migrate it back, libvirt will report Setting different SELinux label on /var/lib/libvirt/qemu/nvram/test_VARS.fd which is already in use and abort the operation. This is caused by # getfattr -dm- /var/lib/libvirt/qemu/nvram/test_VARS.fd security.selinux="system_u:object_r:svirt_image_t:s0:c704,c774" trusted.libvirt.security.dac="+0:+0" trusted.libvirt.security.ref_dac="1" trusted.libvirt.security.ref_selinux="1" trusted.libvirt.security.selinux="system_u:object_r:svirt_image_t:s0" trusted.libvirt.security.timestamp_dac="1713347549" trusted.libvirt.security.timestamp_selinux="1713347549" In particular, the non-zero reference counts are what convinces libvirt to bail. I don't think we can handle this nicely without opening other cans of worms. The principle of "don't touch anything on the source once the VM is running on the destination" is a good one to follow, since ignoring it might result in the destination VM suddenly being prevented from performing I/O. And in case the migration fails, which as you both pointed out earlier is a possibility that we need to take into account, retaining the existing labels instead of clearing them is actually a good thing, as it allows resuming execution on the source host without running into permission issues. In other words, I've come to the conclusion that remember_owner=0 is the least bad option here. In the v2 that I've just posted, I've updated the comment to stress further the fact that this option comes with caveats and is not to be used lightly. -- Andrea Bolognani / Red Hat / Virtualization ___ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-le...@lists.libvirt.org
Re: [PATCH v3 5/5] conf: nodedev: Fill active_config at XML parse time
On 4/9/24 11:20 AM, Boris Fiuczynski wrote: > On 4/9/24 16:56, Cole Robinson wrote: >> Commit v10.0.0-265-ge67bca23e4 added a `active_config` and >> `defined_config` to nodedev mdev internal XML handling. >> `defined_config` can be filled at XML parse time, but `active_config` >> must be filled in by nodedev driver. This wasn't implemented for the >> test driver however, which caused virt-manager test suite regressions. >> >> Example before: >> >> ``` >> $ virsh --connect >> test:///home/crobinso/src/virt-manager/tests/data/testdriver/testdriver.xml >> nodedev-dumpxml mdev_8e37ee90_2b51_45e3_9b25_bf8283c03110 >> >> mdev_8e37ee90_2b51_45e3_9b25_bf8283c03110 >> >> /sys/devices/css0/0.0.0023/8e37ee90-2b51-45e3-9b25-bf8283c03110 >> css_0_0_0023 >> >> >> >> >> >> ``` >> >> Example after: >> >> ``` >> $ virsh --connect >> test:///home/crobinso/src/virt-manager/tests/data/testdriver/testdriver.xml >> nodedev-dumpxml mdev_8e37ee90_2b51_45e3_9b25_bf8283c03110 >> >> mdev_8e37ee90_2b51_45e3_9b25_bf8283c03110 >> >> /sys/devices/css0/0.0.0023/8e37ee90-2b51-45e3-9b25-bf8283c03110 >> css_0_0_0023 >> >> >> >> >> ``` >> >> Simplest solution is to fill in `active_config` at XML define time >> as well. The real node_device driver already takes care to free any >> `active_config` when it live updates this info, so we are safe there. > > I do not think that it is a good idea to hack the general code which > creates in the real environments fake data. > I can't tell how this affects real environments. Is there a place in the udev driver where XML is parsed, and then active_config isn't explicitly updated? If not, do you object to me pushing this with Michal's ACK? This is causing some pain with virt-manager dev workflow > If your tests require active mdevs than the mocking code should set > these active and also do the config copy. > Obviously this can work but IMO it sets a bad precedent. If this active/inactive_config structure is extended in the future, the test driver syncing needs to be extended to match, or we need more plumbing to share more of this Personally I would have rather seen this implemented using ->newDef pattern used by domain, network, storage. That's the closest thing to an internal standard for handling differences between inactive config and runtime config. ->def is copied to ->newDef at object startup time, and the drivers change newDef as needed to reflect runtime state of the object. Thanks, Cole ___ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-le...@lists.libvirt.org
Re: [PATCH v3 4/5] test: make nodedevs active by default
On 4/9/24 11:19 AM, Boris Fiuczynski wrote: > On 4/9/24 16:56, Cole Robinson wrote: >> This was the implied default before nodedevs gained a notion of >> being inactive, and matches how we handle parsing other objects >> >> Signed-off-by: Cole Robinson >> --- >> src/test/test_driver.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/src/test/test_driver.c b/src/test/test_driver.c >> index 41828f86b6..9db7a44035 100644 >> --- a/src/test/test_driver.c >> +++ b/src/test/test_driver.c >> @@ -1269,6 +1269,7 @@ testParseNodedevs(testDriver *privconn, >> return -1; >> } >> + virNodeDeviceObjSetActive(obj, true); > > This will actually render the mdev object to be transient which is an > active mdev not having a persistent definition. > The data using virNodeDeviceDefParseXML is stored in the mdevs > defined_config only therefore the data is showing up when you use "virsh > nodedev-dumpxml" as it defaults to the "current state" of the nodedev. > > For data consistency of the node you should instead do > virNodeDeviceObjSetPersistent(obj, true); > > Now the mdev is inactive and persistent and the virsh command should be > correct. > > I guess this would also resolve the requirement for the next patch > unless you have a requirement to mock dumping transient mdevs. Oops I replied to patch 5 before seeing this one. Let me explore this a bit. Thanks, Cole ___ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-le...@lists.libvirt.org
Re: [PATCH v3 5/5] conf: nodedev: Fill active_config at XML parse time
On Wed, Apr 17, 2024 at 09:58:10AM -0400, Cole Robinson wrote: > On 4/9/24 11:20 AM, Boris Fiuczynski wrote: > > On 4/9/24 16:56, Cole Robinson wrote: > >> Commit v10.0.0-265-ge67bca23e4 added a `active_config` and > >> `defined_config` to nodedev mdev internal XML handling. > >> `defined_config` can be filled at XML parse time, but `active_config` > >> must be filled in by nodedev driver. This wasn't implemented for the > >> test driver however, which caused virt-manager test suite regressions. > >> > >> Example before: > >> > >> ``` > >> $ virsh --connect > >> test:///home/crobinso/src/virt-manager/tests/data/testdriver/testdriver.xml > >> nodedev-dumpxml mdev_8e37ee90_2b51_45e3_9b25_bf8283c03110 > >> > >> mdev_8e37ee90_2b51_45e3_9b25_bf8283c03110 > >> > >> /sys/devices/css0/0.0.0023/8e37ee90-2b51-45e3-9b25-bf8283c03110 > >> css_0_0_0023 > >> > >> > >> > >> > >> > >> ``` > >> > >> Example after: > >> > >> ``` > >> $ virsh --connect > >> test:///home/crobinso/src/virt-manager/tests/data/testdriver/testdriver.xml > >> nodedev-dumpxml mdev_8e37ee90_2b51_45e3_9b25_bf8283c03110 > >> > >> mdev_8e37ee90_2b51_45e3_9b25_bf8283c03110 > >> > >> /sys/devices/css0/0.0.0023/8e37ee90-2b51-45e3-9b25-bf8283c03110 > >> css_0_0_0023 > >> > >> > >> > >> > >> ``` > >> > >> Simplest solution is to fill in `active_config` at XML define time > >> as well. The real node_device driver already takes care to free any > >> `active_config` when it live updates this info, so we are safe there. > > > > I do not think that it is a good idea to hack the general code which > > creates in the real environments fake data. > > > > I can't tell how this affects real environments. Is there a place in the > udev driver where XML is parsed, and then active_config isn't explicitly > updated? > > If not, do you object to me pushing this with Michal's ACK? This is > causing some pain with virt-manager dev workflow > > > If your tests require active mdevs than the mocking code should set > > these active and also do the config copy. > > > > Obviously this can work but IMO it sets a bad precedent. If this > active/inactive_config structure is extended in the future, the test > driver syncing needs to be extended to match, or we need more plumbing > to share more of this > > > Personally I would have rather seen this implemented using ->newDef > pattern used by domain, network, storage. That's the closest thing to an > internal standard for handling differences between inactive config and > runtime config. ->def is copied to ->newDef at object startup time, and > the drivers change newDef as needed to reflect runtime state of the object. Yes, if we need to store both active and inactive config information then I would strongly prefer to have the normal def+newDef pattern, so we can expose this difference in the APIs with an "INACTIVE" flag and --inactive virsh opt. With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| ___ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-le...@lists.libvirt.org
Re: [PATCH v3 4/5] test: make nodedevs active by default
On 4/17/24 10:04 AM, Cole Robinson wrote: > On 4/9/24 11:19 AM, Boris Fiuczynski wrote: >> On 4/9/24 16:56, Cole Robinson wrote: >>> This was the implied default before nodedevs gained a notion of >>> being inactive, and matches how we handle parsing other objects >>> >>> Signed-off-by: Cole Robinson >>> --- >>> src/test/test_driver.c | 1 + >>> 1 file changed, 1 insertion(+) >>> >>> diff --git a/src/test/test_driver.c b/src/test/test_driver.c >>> index 41828f86b6..9db7a44035 100644 >>> --- a/src/test/test_driver.c >>> +++ b/src/test/test_driver.c >>> @@ -1269,6 +1269,7 @@ testParseNodedevs(testDriver *privconn, >>> return -1; >>> } >>> + virNodeDeviceObjSetActive(obj, true); >> >> This will actually render the mdev object to be transient which is an >> active mdev not having a persistent definition. >> The data using virNodeDeviceDefParseXML is stored in the mdevs >> defined_config only therefore the data is showing up when you use "virsh >> nodedev-dumpxml" as it defaults to the "current state" of the nodedev. >> >> For data consistency of the node you should instead do >> virNodeDeviceObjSetPersistent(obj, true); >> Good catch that this does not make the devices persistent, I missed that detail. Ok so there's a bit more context needed here. For test driver XML parsing, all other objects are considered persistent by default. Transient has to be opted in via testdriver specific XML namespace. So yeah makes sense for consistency to make object persistent as well. But same logic applies for making the object 'active' by default. test driver assumes any read in domain/interface/network/storage object is default active and persistent. Makes sense to do the same for nodedev IMO. Notable we need this so `virsh nodedev-list` works in the way it historically did, before nodedev devices could be inactive. >> Now the mdev is inactive and persistent and the virsh command should be >> correct. >> virt-manager/virt-install isn't using virsh, we are using direct API via python bindings. We are calling virNodeDeviceGetXMLDesc with flags=0, the only historically supported value. But even still, this doesn't fix `virsh nodedev-dumpxml $mdev` for test driver, because GetXMLDesc test driver implementation doesn't force INACTIVE XML flag when the object is inactive, like the real node_device_driver.c does. So flags=0 tries to dump 'active' XML of an inactive object. So that's another bit to fix. But still IMO that is not sufficient, since nodedevs parsed via test driver input XML should be considered active by default. I will push patch 1-3 which you and michal reviewed. I will send another version of this series with the 'persistent' bit fixed, and an additional patch to fix nodedev GetXML semantics to match node_device driver. But that still leaves something like patch 5 as a requirement. Thanks, Cole ___ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-le...@lists.libvirt.org
[PATCH v4 0/3] test: fix nodedev mdev XML regression
See last patch for explanation. First two patches are related bugfixes/improvements v4 changes: - 3 patches pushed - mark parsed devices as persistent too - add GetXML fix patch Cole Robinson (3): test: make parsed nodedevs active and persistent test: Sync GetXML INACTIVE behavior with live driver conf: nodedev: Fill active_config at XML parse time src/conf/node_device_conf.c | 5 - src/test/test_driver.c | 19 ++- tests/nodedevxml2xmltest.c | 15 --- 3 files changed, 22 insertions(+), 17 deletions(-) -- 2.44.0 ___ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-le...@lists.libvirt.org
[PATCH v4 2/3] test: Sync GetXML INACTIVE behavior with live driver
- Error if INACTIVE requested for transient object
- Force dumping INACTIVE XML when object is inactive
Signed-off-by: Cole Robinson
---
src/test/test_driver.c | 17 -
1 file changed, 16 insertions(+), 1 deletion(-)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index 153ab7cdc2..e7d2b6c866 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -7514,15 +7514,30 @@ testNodeDeviceGetXMLDesc(virNodeDevicePtr dev,
{
testDriver *driver = dev->conn->privateData;
virNodeDeviceObj *obj;
+virNodeDeviceDef *def;
char *ret = NULL;
virCheckFlags(VIR_NODE_DEVICE_XML_INACTIVE, NULL);
if (!(obj = testNodeDeviceObjFindByName(driver, dev->name)))
return NULL;
+def = virNodeDeviceObjGetDef(obj);
-ret = virNodeDeviceDefFormat(virNodeDeviceObjGetDef(obj), flags);
+if (flags & VIR_NODE_DEVICE_XML_INACTIVE) {
+if (!virNodeDeviceObjIsPersistent(obj)) {
+virReportError(VIR_ERR_OPERATION_INVALID,
+ _("node device '%1$s' is not persistent"),
+ def->name);
+goto cleanup;
+}
+} else {
+if (!virNodeDeviceObjIsActive(obj))
+flags |= VIR_NODE_DEVICE_XML_INACTIVE;
+}
+ret = virNodeDeviceDefFormat(def, flags);
+
+ cleanup:
virNodeDeviceObjEndAPI(&obj);
return ret;
}
--
2.44.0
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org
[PATCH v4 1/3] test: make parsed nodedevs active and persistent
This was the implied default before nodedevs gained a notion of being inactive and transient. It also matches the implied default when parsing other object types Signed-off-by: Cole Robinson --- src/test/test_driver.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 41828f86b6..153ab7cdc2 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -1269,6 +1269,8 @@ testParseNodedevs(testDriver *privconn, return -1; } +virNodeDeviceObjSetPersistent(obj, true); +virNodeDeviceObjSetActive(obj, true); virNodeDeviceObjSetSkipUpdateCaps(obj, true); virNodeDeviceObjEndAPI(&obj); } -- 2.44.0 ___ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-le...@lists.libvirt.org
[PATCH v4 3/3] conf: nodedev: Fill active_config at XML parse time
Commit v10.0.0-265-ge67bca23e4 added a `active_config` and
`defined_config` to nodedev mdev internal XML handling.
`defined_config` can be filled at XML parse time, but `active_config`
must be filled in by nodedev driver. This wasn't implemented for the
test driver however, which caused virt-manager test suite regressions.
Example before:
```
$ virsh --connect
test:///home/crobinso/src/virt-manager/tests/data/testdriver/testdriver.xml
nodedev-dumpxml mdev_8e37ee90_2b51_45e3_9b25_bf8283c03110
mdev_8e37ee90_2b51_45e3_9b25_bf8283c03110
/sys/devices/css0/0.0.0023/8e37ee90-2b51-45e3-9b25-bf8283c03110
css_0_0_0023
```
Example after:
```
$ virsh --connect
test:///home/crobinso/src/virt-manager/tests/data/testdriver/testdriver.xml
nodedev-dumpxml mdev_8e37ee90_2b51_45e3_9b25_bf8283c03110
mdev_8e37ee90_2b51_45e3_9b25_bf8283c03110
/sys/devices/css0/0.0.0023/8e37ee90-2b51-45e3-9b25-bf8283c03110
css_0_0_0023
```
Simplest solution is to fill in `active_config` at XML define time
as well. The real node_device driver already takes care to free any
`active_config` when it live updates this info, so we are safe there.
This also lets us drop the test suite logic to duplicate this data.
Signed-off-by: Cole Robinson
---
src/conf/node_device_conf.c | 5 -
tests/nodedevxml2xmltest.c | 15 ---
2 files changed, 4 insertions(+), 16 deletions(-)
diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c
index 5cfbd6a7eb..f381ea128c 100644
--- a/src/conf/node_device_conf.c
+++ b/src/conf/node_device_conf.c
@@ -,6 +,7 @@ virNodeDevCapMdevParseXML(xmlXPathContextPtr ctxt,
_("missing type id attribute for '%1$s'"), def->name);
return -1;
}
+mdev->active_config.type = g_strdup(mdev->defined_config.type);
if ((uuidstr = virXPathString("string(./uuid[1])", ctxt))) {
unsigned char uuidbuf[VIR_UUID_BUFLEN];
@@ -2248,8 +2249,10 @@ virNodeDevCapMdevParseXML(xmlXPathContextPtr ctxt,
if ((nattrs = virXPathNodeSet("./attr", ctxt, &attrs)) < 0)
return -1;
-for (i = 0; i < nattrs; i++)
+for (i = 0; i < nattrs; i++) {
virNodeDevCapMdevAttributeParseXML(ctxt, attrs[i],
&mdev->defined_config);
+virNodeDevCapMdevAttributeParseXML(ctxt, attrs[i],
&mdev->active_config);
+}
return 0;
}
diff --git a/tests/nodedevxml2xmltest.c b/tests/nodedevxml2xmltest.c
index e918922672..d2663a8d68 100644
--- a/tests/nodedevxml2xmltest.c
+++ b/tests/nodedevxml2xmltest.c
@@ -24,7 +24,6 @@ testCompareXMLToXMLFiles(const char *xml, const char
*outfile, unsigned int flag
int ret = -1;
virNodeDeviceDef *dev = NULL;
virNodeDevCapsDef *caps;
-size_t i;
if (virTestLoadFile(xml, &xmlData) < 0)
goto fail;
@@ -52,20 +51,6 @@ testCompareXMLToXMLFiles(const char *xml, const char
*outfile, unsigned int flag
data->storage.logical_block_size;
}
}
-
-if (caps->data.type == VIR_NODE_DEV_CAP_MDEV &&
-!(flags & VIR_NODE_DEVICE_XML_INACTIVE)) {
-data->mdev.active_config.type =
g_strdup(data->mdev.defined_config.type);
-for (i = 0; i < data->mdev.defined_config.nattributes; i++) {
-g_autoptr(virMediatedDeviceAttr) attr =
g_new0(virMediatedDeviceAttr, 1);
-
-attr->name =
g_strdup(data->mdev.defined_config.attributes[i]->name);
-attr->value =
g_strdup(data->mdev.defined_config.attributes[i]->value);
-VIR_APPEND_ELEMENT(data->mdev.active_config.attributes,
- data->mdev.active_config.nattributes,
- attr);
-}
-}
}
if (!(actual = virNodeDeviceDefFormat(dev, flags)))
--
2.44.0
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org
Re: [PATCH 09/10] qemu: Always set labels for TPM state
On Wed, Mar 20, 2024 at 09:10:48AM -0700, Andrea Bolognani wrote:
> On Wed, Mar 20, 2024 at 10:18:39AM -0400, Stefan Berger wrote:
> > On 3/20/24 08:23, Peter Krempa wrote:
> > > Did you consider the case when the migration fails and the VM will be
> > > restored to run on the source host again? In such case doin the
> > > relabelling might break the source host.
> >
> > Right. I seem to remember testing such scenarios. I had to put an exit() (or
> > something like it) into swtpm on the destination side to trigger the
> > fallback to the source side. The swtpm on the source side had closed file
> > access and wants to open them (lockfile) again and so the files needed to be
> > labeled correctly if the storage on the source side is
> > on the disk and exported via NFS from there (iirc). If the storage is
> > NFS-exported from a 3rd host it probably would not require the labels.
>
> I didn't really consider the failure scenario, so thank you for
> bringing that up.
>
> I think it would be still fine. If the source has NFS storage, then
> access will keep working regardless of what relabeling the
> destination has been up to in the meantime. And if the source has
> local storage, then the relabeling on the destination (via NFS) will
> not actually have touched the SELinux labels.
>
> The only concern I have is that, when going from local to NFS, labels
> might have been restored on the source side. But I assume that
> restoring only happens once the migration has been confirmed as
> successful? I'll check.
>
> Once again, as far as I can tell (please let me know if I'm wrong!)
> there is no special casing when it comes to disks and other types of
> persistent storage, so if this approach was problematic I would have
> expected many issues to have been reported by now.
I've tested this to confirm. My trick to simulating a migration
failure was to add
to the migration XML, where the VM uses a different machine type in
its configuration. This results in something like
process exited while connecting to monitor: qemu-system-x86_64:
-device {"driver":"pcie-root-port","port":8,"chassis":1,"id":"pci.1",\
"bus":"pcie.0","multifunction":true,"addr":"0x1"}: Bus 'pcie.0' not found
which should be a decent enough proxy for the kind of "we went as far
as attempting to start the QEMU process on the destination host, then
things went sideways" failure that we'd experience as a consequence
of a permission error. Suggestions on how to improve the test
methodology are very much appreciated :)
Anyway, based on what I've seen I think I can confirm my initial
intuition as reported above. Things work the way you expect, in that
upon migration failure the VM keeps happily chugging along on the
source host.
--
Andrea Bolognani / Red Hat / Virtualization
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org
Re: [PATCH v2 1/5] security: Fix alignment
On 4/17/24 09:29, Andrea Bolognani wrote:
Signed-off-by: Andrea Bolognani
Reviewed-by: Stefan Berger
---
src/security/security_selinux.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index aaec34ff8b..a4915dbc89 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -1983,9 +1983,9 @@
virSecuritySELinuxSetImageLabelInternal(virSecurityManager *mgr,
static int
virSecuritySELinuxSetImageLabel(virSecurityManager *mgr,
- virDomainDef *def,
- virStorageSource *src,
- virSecurityDomainImageLabelFlags flags)
+virDomainDef *def,
+virStorageSource *src,
+virSecurityDomainImageLabelFlags flags)
{
virStorageSource *parent = src;
virStorageSource *n;
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org
Re: [PATCH v2 2/5] qemu: Introduce shared_filesystems configuration option
On 4/17/24 09:29, Andrea Bolognani wrote:
As explained in the comment, this can help in scenarios where
a shared filesystem can't be detected as such by libvirt, by
giving the admin the opportunity to provide this information
manually.
Signed-off-by: Andrea Bolognani
---
src/qemu/libvirtd_qemu.aug | 3 +++
src/qemu/qemu.conf.in | 23 +++
src/qemu/qemu_conf.c | 17 +
src/qemu/qemu_conf.h | 2 ++
src/qemu/test_libvirtd_qemu.aug.in | 5 +
5 files changed, 50 insertions(+)
diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug
index 2b6526538f..1377fd89cc 100644
--- a/src/qemu/libvirtd_qemu.aug
+++ b/src/qemu/libvirtd_qemu.aug
@@ -143,6 +143,8 @@ module Libvirtd_qemu =
let storage_entry = bool_entry "storage_use_nbdkit"
+ let filesystem_entry = str_array_entry "shared_filesystems"
+
(* Entries that used to exist in the config which are now
* deleted. We keep on parsing them so we don't break
* ability to parse old configs after upgrade
@@ -173,6 +175,7 @@ module Libvirtd_qemu =
| swtpm_entry
| capability_filters_entry
| storage_entry
+ | filesystem_entry
| obsolete_entry
let comment = [ label "#comment" . del /#[ \t]*/ "# " . store /([^ \t\n][^\n]*)?/ . del /\n/ "\n" ]
diff --git a/src/qemu/qemu.conf.in b/src/qemu/qemu.conf.in
index f406df8749..c543a0a55b 100644
--- a/src/qemu/qemu.conf.in
+++ b/src/qemu/qemu.conf.in
@@ -986,3 +986,26 @@
# note that the default might change in future releases.
#
#storage_use_nbdkit = @USE_NBDKIT_DEFAULT@
+
+# libvirt will normally prevent migration if the storage backing the VM is not
+# on a shared filesystems. Sometimes, however, the storage *is* shared despite
+# not being detected as such: for example, this is the case when one of the
+# hosts involved in the migration is exporting its local storage to the other
+# one via NFS.
+#
+# Any directory listed here will be assumed to live on a shared filesystem,
+# making migration possible in scenarios such as the one described above.
+#
+# Due to how label remembering is implemented, it will generally be necessary
+# to set remember_owner=0 *on both sides of the migration* as well.
+#
+# NOTE: this option is intended to help in very specific scenarios that are
+# rarely encountered. If you find yourself reaching for this option, consider
+# reworking your environment so that it follows a more common architecture
+# rather than using it.
+#
+#shared_filesystems = [
+# "/path/to/images",
+# "/path/to/nvram",
+# "/path/to/swtpm"
+#]
May be worth considering: Would it ever be useful or necessary for
libvirt to know what type of filesystem is used for the sharing, so that
users should write nfs:/path/to/swtpm for example? I don't have much
experience with shared filesystems (other than NFS) but maybe sometimes
behavior has to be adjusted depending on what type is being used.
Otherwise LGTM.
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 4050a82341..01c6bcc793 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -374,6 +374,8 @@ static void virQEMUDriverConfigDispose(void *obj)
g_strfreev(cfg->capabilityfilters);
+g_strfreev(cfg->sharedFilesystems);
+
g_free(cfg->deprecationBehavior);
}
@@ -1084,6 +1086,18 @@ virQEMUDriverConfigLoadStorageEntry(virQEMUDriverConfig *cfg,
}
+static int
+virQEMUDriverConfigLoadFilesystemEntry(virQEMUDriverConfig *cfg,
+ virConf *conf)
+{
+if (virConfGetValueStringList(conf, "shared_filesystems", false,
+ &cfg->sharedFilesystems) < 0)
+return -1;
+
+return 0;
+}
+
+
int virQEMUDriverConfigLoadFile(virQEMUDriverConfig *cfg,
const char *filename,
bool privileged)
@@ -1158,6 +1172,9 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfig *cfg,
if (virQEMUDriverConfigLoadStorageEntry(cfg, conf) < 0)
return -1;
+if (virQEMUDriverConfigLoadFilesystemEntry(cfg, conf) < 0)
+return -1;
+
return 0;
}
diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
index 36049b4bfa..b53d56be02 100644
--- a/src/qemu/qemu_conf.h
+++ b/src/qemu/qemu_conf.h
@@ -233,6 +233,8 @@ struct _virQEMUDriverConfig {
bool storageUseNbdkit;
virQEMUSchedCore schedCore;
+
+char **sharedFilesystems;
};
G_DEFINE_AUTOPTR_CLEANUP_FUNC(virQEMUDriverConfig, virObjectUnref);
diff --git a/src/qemu/test_libvirtd_qemu.aug.in
b/src/qemu/test_libvirtd_qemu.aug.in
index b97e6de11e..69fdae215a 100644
--- a/src/qemu/test_libvirtd_qemu.aug.in
+++ b/src/qemu/test_libvirtd_qemu.aug.in
@@ -119,3 +119,8 @@ module Test_libvirtd_qemu =
{ "deprecation_behavior" = "none" }
{ "sched_core" = "none" }
{ "storage_use_nbdkit"
Revisiting parallel save/restore
Hi All, While Fabiano has been working on improving save/restore performance in qemu, I've been tinkering with the same in libvirt. The end goal is to introduce a new VIR_DOMAIN_SAVE_PARALLEL flag for save/restore, along with a VIR_DOMAIN_SAVE_PARAM_PARALLEL_CONNECTIONS parameter to specify the number of concurrent channels used for the save/restore. Recall Claudio previously posted a patch series implementing parallel save/restore completely in libvirt, using qemu's multifd functionality [1]. A good starting point on this journey is supporting the new mapped-ram capability in qemu 9.0 [2]. Since mapped-ram is a new on-disk format, I assume we'll need a new QEMU_SAVE_VERSION 3 when using it? Otherwise I'm not sure how to detect if a saved image is in mapped-ram format vs the existing, sequential stream format. IIUC, mapped-ram cannot be used with the exiting 'fd:' migration URI and instead must use 'file:'. Does qemu advertise support for that? I couldn't find it. If not, 'file:' (available in qemu 8.2) predates mapped-ram, so in theory we could live without the advertisement. It's also not clear when we want to enable the mapped-ram capability. Should it always be enabled if supported by the underlying qemu? One motivation for creating the mapped-ram was to support direct-io of the migration stream in qemu, in which case it could be tied to VIR_DOMAIN_SAVE_BYPASS_CACHE. E.g. the mapped-ram capability is enabled when user specifies VIR_DOMAIN_SAVE_BYPASS_CACHE && user-provided path results in a seekable fd && qemu supports mapped-ram? Looking ahead, should the mapped-ram capability be required for supporting the VIR_DOMAIN_SAVE_PARALLEL flag? As I understand, parallel save/restore was another motivation for creating the mapped-ram feature. It allows multifd threads to write exclusively to the offsets provided by mapped-ram. Can multiple multifd threads concurrently write to an fd without mapped-ram? Regards, Jim [1] https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/3Y5GMS6A4QS4IXWDKFFV3A2FO5YMCFES/ [2] https://gitlab.com/qemu-project/qemu/-/blob/master/docs/devel/migration/mapped-ram.rst?ref_type=heads ___ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-le...@lists.libvirt.org
