[libvirt] [PATCH 0/9] qemu: clean up capability handling during startup (incremental backup prequels)
For libvirt to use incremental backup we will require that blockdev is enabled. Clean up how we handle caps during startup since blockdev may be masked out there and add infrastructure to interlock flags. Peter Krempa (9): qemu: process: Make it obvious that virDomainDefPostParse is called with NULL opaque qemu: process: Don't try to redetect missing qemuCaps on reconnect qemu: Move and rename qemuDomainUpdateQEMUCaps qemu: process: Move clearing of the BLOCKDEV capability to qemuProcessPrepareQEMUCaps qemu: process: Move clearing of QEMU_CAPS_CHARDEV_FD_PASS to qemuProcessPrepareQEMUCaps qemu: process: Move handling of qemu capability overrides qemu: caps: Don't check capability before clearing it qemu: capabilities: Lock out incremental backup capability without blockdev qemu: process: Re-process qemu capability lockout in qemuProcessPrepareQEMUCaps src/qemu/qemu_capabilities.c | 20 +++++++++- src/qemu/qemu_capabilities.h | 2 + src/qemu/qemu_domain.c | 26 ------------ src/qemu/qemu_domain.h | 4 -- src/qemu/qemu_process.c | 76 ++++++++++++++++++++++++------------ 5 files changed, 72 insertions(+), 56 deletions(-) -- 2.23.0
Commit c90fb5a828a added explicit use of the private copy of the qemu capabilities to various places. The change to qemuProcessInit was bogus though as at the point where we re-initiate the post parse callbacks priv->qemuCaps is still NULL as we clear it after shutdown of the VM and don't initiate it until a later point. Using the value from priv->qemuCaps might mislead readers of the code into thinking that something useful is being passed at that point so go with an explicit NULL instead. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_process.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 161b3910e5..1b88c471f4 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5531,7 +5531,8 @@ qemuProcessInit(virQEMUDriverPtr driver, if (vm->def->postParseFailed) { VIR_DEBUG("re-running the post parse callback"); - if (virDomainDefPostParse(vm->def, caps, 0, driver->xmlopt, priv->qemuCaps) < 0) + /* we don't have the private copy of qemuCaps at this point */ + if (virDomainDefPostParse(vm->def, caps, 0, driver->xmlopt, NULL) < 0) goto cleanup; } -- 2.23.0
On Mon, Nov 25, 2019 at 01:53:46PM +0100, Peter Krempa wrote:
Commit c90fb5a828a added explicit use of the private copy of the qemu capabilities to various places. The change to qemuProcessInit was bogus though as at the point where we re-initiate the post parse callbacks priv->qemuCaps is still NULL as we clear it after shutdown of the VM and don't initiate it until a later point.
Using the value from priv->qemuCaps might mislead readers of the code into thinking that something useful is being passed at that point so go with an explicit NULL instead.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_process.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
The redetection was originally added in 43c01d3838 as a way to recover from libvirtd upgrade from the time when we didn't persist the qemu capabilities in the status XML. Also this the oldest supported qemu by more than two years. Even if somebody would have a running VM running at least qemu 1.5 with such an old libvirt we certainly wouldn't do the right thing by redetecting the capabilities and then trying to communicate with qemu. For now it will be the best to just stop considering this scenario any more. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_process.c | 7 ------- 1 file changed, 7 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 1b88c471f4..a76a8da841 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -8085,13 +8085,6 @@ qemuProcessReconnect(void *opaque) goto error; } - /* If upgrading from old libvirtd we won't have found any - * caps in the domain status, so re-query them - */ - if (!priv->qemuCaps && - (qemuDomainUpdateQEMUCaps(obj, driver->qemuCapsCache) < 0)) - goto error; - /* In case the domain shutdown while we were not running, * we need to finish the shutdown process. And we need to do it after * we have virQEMUCaps filled in. -- 2.23.0
On Mon, Nov 25, 2019 at 01:53:47PM +0100, Peter Krempa wrote:
The redetection was originally added in 43c01d3838 as a way to recover from libvirtd upgrade from the time when we didn't persist the qemu capabilities in the status XML. Also this the oldest supported qemu by more than two years.
Even if somebody would have a running VM running at least qemu 1.5 with such an old libvirt we certainly wouldn't do the right thing by redetecting the capabilities and then trying to communicate with qemu.
For now it will be the best to just stop considering this scenario any more.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_process.c | 7 ------- 1 file changed, 7 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 1b88c471f4..a76a8da841 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -8085,13 +8085,6 @@ qemuProcessReconnect(void *opaque) goto error; }
- /* If upgrading from old libvirtd we won't have found any - * caps in the domain status, so re-query them - */ - if (!priv->qemuCaps && - (qemuDomainUpdateQEMUCaps(obj, driver->qemuCapsCache) < 0)) - goto error;
Shouldn't we be making missing qemuCaps into a fatal error when loading that VM. 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 :|
On Mon, Nov 25, 2019 at 12:59:00 +0000, Daniel Berrange wrote:
On Mon, Nov 25, 2019 at 01:53:47PM +0100, Peter Krempa wrote:
The redetection was originally added in 43c01d3838 as a way to recover from libvirtd upgrade from the time when we didn't persist the qemu capabilities in the status XML. Also this the oldest supported qemu by more than two years.
Even if somebody would have a running VM running at least qemu 1.5 with such an old libvirt we certainly wouldn't do the right thing by redetecting the capabilities and then trying to communicate with qemu.
For now it will be the best to just stop considering this scenario any more.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_process.c | 7 ------- 1 file changed, 7 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 1b88c471f4..a76a8da841 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -8085,13 +8085,6 @@ qemuProcessReconnect(void *opaque) goto error; }
- /* If upgrading from old libvirtd we won't have found any - * caps in the domain status, so re-query them - */ - if (!priv->qemuCaps && - (qemuDomainUpdateQEMUCaps(obj, driver->qemuCapsCache) < 0)) - goto error;
Shouldn't we be making missing qemuCaps into a fatal error when loading that VM.
I thought about it for a bit. If we make it an error the VM will vanish after upgrade (and continue running). If we don't make an error it will not vanish and you'll be able to at least destroy it. Given that such situation is super-unlikely, I opted to remove it. Nobody will probably run into this situation and if yes then the VM will not vanish at least. I can add an error though, but to me it seems it's not worth.
On Mon, Nov 25, 2019 at 02:07:13PM +0100, Peter Krempa wrote:
On Mon, Nov 25, 2019 at 12:59:00 +0000, Daniel Berrange wrote:
On Mon, Nov 25, 2019 at 01:53:47PM +0100, Peter Krempa wrote:
The redetection was originally added in 43c01d3838 as a way to recover from libvirtd upgrade from the time when we didn't persist the qemu capabilities in the status XML. Also this the oldest supported qemu by more than two years.
Even if somebody would have a running VM running at least qemu 1.5 with such an old libvirt we certainly wouldn't do the right thing by redetecting the capabilities and then trying to communicate with qemu.
For now it will be the best to just stop considering this scenario any more.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_process.c | 7 ------- 1 file changed, 7 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 1b88c471f4..a76a8da841 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -8085,13 +8085,6 @@ qemuProcessReconnect(void *opaque) goto error; }
- /* If upgrading from old libvirtd we won't have found any - * caps in the domain status, so re-query them - */ - if (!priv->qemuCaps && - (qemuDomainUpdateQEMUCaps(obj, driver->qemuCapsCache) < 0)) - goto error;
Shouldn't we be making missing qemuCaps into a fatal error when loading that VM.
I thought about it for a bit. If we make it an error the VM will vanish after upgrade (and continue running). If we don't make an error it will not vanish and you'll be able to at least destroy it.
Given that such situation is super-unlikely, I opted to remove it. Nobody will probably run into this situation and if yes then the VM will not vanish at least.
I can add an error though, but to me it seems it's not worth.
My concern is that pretty much all our later code will assume that priv->qemuCaps is non-NULL. So by ignoring the error, the VM might not vanish initially, but libvirtd may well crash shortly after. 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 :|
On Mon, Nov 25, 2019 at 13:11:10 +0000, Daniel Berrange wrote:
On Mon, Nov 25, 2019 at 02:07:13PM +0100, Peter Krempa wrote:
On Mon, Nov 25, 2019 at 12:59:00 +0000, Daniel Berrange wrote:
On Mon, Nov 25, 2019 at 01:53:47PM +0100, Peter Krempa wrote:
The redetection was originally added in 43c01d3838 as a way to recover from libvirtd upgrade from the time when we didn't persist the qemu capabilities in the status XML. Also this the oldest supported qemu by more than two years.
Even if somebody would have a running VM running at least qemu 1.5 with such an old libvirt we certainly wouldn't do the right thing by redetecting the capabilities and then trying to communicate with qemu.
For now it will be the best to just stop considering this scenario any more.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_process.c | 7 ------- 1 file changed, 7 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 1b88c471f4..a76a8da841 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -8085,13 +8085,6 @@ qemuProcessReconnect(void *opaque) goto error; }
- /* If upgrading from old libvirtd we won't have found any - * caps in the domain status, so re-query them - */ - if (!priv->qemuCaps && - (qemuDomainUpdateQEMUCaps(obj, driver->qemuCapsCache) < 0)) - goto error;
Shouldn't we be making missing qemuCaps into a fatal error when loading that VM.
I thought about it for a bit. If we make it an error the VM will vanish after upgrade (and continue running). If we don't make an error it will not vanish and you'll be able to at least destroy it.
Given that such situation is super-unlikely, I opted to remove it. Nobody will probably run into this situation and if yes then the VM will not vanish at least.
I can add an error though, but to me it seems it's not worth.
My concern is that pretty much all our later code will assume that priv->qemuCaps is non-NULL. So by ignoring the error, the VM might not vanish initially, but libvirtd may well crash shortly after.
Good point. The caps are parsed in qemuDomainObjPrivateXMLParse How about we either change the condition to allocate priv->qemuCaps even if 0 elements are found in the XML so that priv->qemuCaps is not NULL (but empty). Or potentially if we really want to make it an error then I'll add something like: if (n == 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("missing qemu capabilities in status XML")); goto error; } to the appropriate place in qemuDomainObjPrivateXMLParse
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 :|
On Mon, Nov 25, 2019 at 02:18:54PM +0100, Peter Krempa wrote:
On Mon, Nov 25, 2019 at 13:11:10 +0000, Daniel Berrange wrote:
On Mon, Nov 25, 2019 at 02:07:13PM +0100, Peter Krempa wrote:
On Mon, Nov 25, 2019 at 12:59:00 +0000, Daniel Berrange wrote:
On Mon, Nov 25, 2019 at 01:53:47PM +0100, Peter Krempa wrote:
The redetection was originally added in 43c01d3838 as a way to recover from libvirtd upgrade from the time when we didn't persist the qemu capabilities in the status XML. Also this the oldest supported qemu by more than two years.
Even if somebody would have a running VM running at least qemu 1.5 with such an old libvirt we certainly wouldn't do the right thing by redetecting the capabilities and then trying to communicate with qemu.
For now it will be the best to just stop considering this scenario any more.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_process.c | 7 ------- 1 file changed, 7 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 1b88c471f4..a76a8da841 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -8085,13 +8085,6 @@ qemuProcessReconnect(void *opaque) goto error; }
- /* If upgrading from old libvirtd we won't have found any - * caps in the domain status, so re-query them - */ - if (!priv->qemuCaps && - (qemuDomainUpdateQEMUCaps(obj, driver->qemuCapsCache) < 0)) - goto error;
Shouldn't we be making missing qemuCaps into a fatal error when loading that VM.
I thought about it for a bit. If we make it an error the VM will vanish after upgrade (and continue running). If we don't make an error it will not vanish and you'll be able to at least destroy it.
Given that such situation is super-unlikely, I opted to remove it. Nobody will probably run into this situation and if yes then the VM will not vanish at least.
I can add an error though, but to me it seems it's not worth.
My concern is that pretty much all our later code will assume that priv->qemuCaps is non-NULL. So by ignoring the error, the VM might not vanish initially, but libvirtd may well crash shortly after.
Good point.
The caps are parsed in qemuDomainObjPrivateXMLParse
How about we either change the condition to allocate priv->qemuCaps even if 0 elements are found in the XML so that priv->qemuCaps is not NULL (but empty).
This is fine, especially if we mark the VM tainted when we reconnect.
Or potentially if we really want to make it an error then I'll add something like:
if (n == 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("missing qemu capabilities in status XML")); goto error; }
to the appropriate place in qemuDomainObjPrivateXMLParse
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 :|
On Mon, Nov 25, 2019 at 13:47:15 +0000, Daniel Berrange wrote:
On Mon, Nov 25, 2019 at 02:18:54PM +0100, Peter Krempa wrote:
On Mon, Nov 25, 2019 at 13:11:10 +0000, Daniel Berrange wrote:
On Mon, Nov 25, 2019 at 02:07:13PM +0100, Peter Krempa wrote:
On Mon, Nov 25, 2019 at 12:59:00 +0000, Daniel Berrange wrote:
On Mon, Nov 25, 2019 at 01:53:47PM +0100, Peter Krempa wrote:
The redetection was originally added in 43c01d3838 as a way to recover from libvirtd upgrade from the time when we didn't persist the qemu capabilities in the status XML. Also this the oldest supported qemu by more than two years.
Even if somebody would have a running VM running at least qemu 1.5 with such an old libvirt we certainly wouldn't do the right thing by redetecting the capabilities and then trying to communicate with qemu.
For now it will be the best to just stop considering this scenario any more.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_process.c | 7 ------- 1 file changed, 7 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 1b88c471f4..a76a8da841 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -8085,13 +8085,6 @@ qemuProcessReconnect(void *opaque) goto error; }
- /* If upgrading from old libvirtd we won't have found any - * caps in the domain status, so re-query them - */ - if (!priv->qemuCaps && - (qemuDomainUpdateQEMUCaps(obj, driver->qemuCapsCache) < 0)) - goto error;
Shouldn't we be making missing qemuCaps into a fatal error when loading that VM.
I thought about it for a bit. If we make it an error the VM will vanish after upgrade (and continue running). If we don't make an error it will not vanish and you'll be able to at least destroy it.
Given that such situation is super-unlikely, I opted to remove it. Nobody will probably run into this situation and if yes then the VM will not vanish at least.
I can add an error though, but to me it seems it's not worth.
My concern is that pretty much all our later code will assume that priv->qemuCaps is non-NULL. So by ignoring the error, the VM might not vanish initially, but libvirtd may well crash shortly after.
Good point.
The caps are parsed in qemuDomainObjPrivateXMLParse
How about we either change the condition to allocate priv->qemuCaps even if 0 elements are found in the XML so that priv->qemuCaps is not NULL (but empty).
This is fine, especially if we mark the VM tainted when we reconnect.
Eh, I think it's so unlikely for that situation to happend that I'll preferrably add an explicit error and make the VM disappear rather than trying to add a taint for this scenario.
The redetection was originally added in 43c01d3838 as a way to recover from libvirtd upgrade from the time when we didn't persist the qemu capabilities in the status XML. Also this the oldest supported qemu by more than two years. Even if somebody would have a running VM running at least qemu 1.5 with such an old libvirt we certainly wouldn't do the right thing by redetecting the capabilities and then trying to communicate with qemu. For now it will be the best to just stop considering this scenario any more and error out for such VM. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- - report an error rather than doing nothing src/qemu/qemu_process.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 3083d0b538..1c8ad1c279 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -8085,12 +8085,12 @@ qemuProcessReconnect(void *opaque) goto error; } - /* If upgrading from old libvirtd we won't have found any - * caps in the domain status, so re-query them - */ - if (!priv->qemuCaps && - (qemuDomainUpdateQEMUCaps(obj, driver->qemuCapsCache) < 0)) + if (!priv->qemuCaps) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("domain '%s' has no capabilities recorded"), + obj->def->name); goto error; + } /* In case the domain shutdown while we were not running, * we need to finish the shutdown process. And we need to do it after -- 2.23.0
On Mon, Dec 02, 2019 at 04:35:36PM +0100, Peter Krempa wrote:
The redetection was originally added in 43c01d3838 as a way to recover from libvirtd upgrade from the time when we didn't persist the qemu capabilities in the status XML. Also this the oldest supported qemu by more than two years.
Even if somebody would have a running VM running at least qemu 1.5 with such an old libvirt we certainly wouldn't do the right thing by redetecting the capabilities and then trying to communicate with qemu.
For now it will be the best to just stop considering this scenario any more and error out for such VM.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- - report an error rather than doing nothing
src/qemu/qemu_process.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 3083d0b538..1c8ad1c279 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -8085,12 +8085,12 @@ qemuProcessReconnect(void *opaque) goto error; }
- /* If upgrading from old libvirtd we won't have found any - * caps in the domain status, so re-query them - */ - if (!priv->qemuCaps && - (qemuDomainUpdateQEMUCaps(obj, driver->qemuCapsCache) < 0)) + if (!priv->qemuCaps) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("domain '%s' has no capabilities recorded"), + obj->def->name); goto error; + }
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
The function is now used only in qemu_process.c so move it there and name it 'qemuProcessPrepareQEMUCaps' which is more appropriate to what it's doing. The reworded comment now mentions that it will also post-process the caps for VM startup. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 26 -------------------------- src/qemu/qemu_domain.h | 4 ---- src/qemu/qemu_process.c | 30 +++++++++++++++++++++++++++++- 3 files changed, 29 insertions(+), 31 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 168e7464a9..abe1396e3a 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -15022,32 +15022,6 @@ qemuDomainFixupCPUs(virDomainObjPtr vm, } -/** - * qemuDomainUpdateQEMUCaps: - * @vm: domain object - * @qemuCapsCache: cache of QEMU capabilities - * - * This function updates the used QEMU capabilities of @vm by querying - * the QEMU capabilities cache. - * - * Returns 0 on success, -1 on error. - */ -int -qemuDomainUpdateQEMUCaps(virDomainObjPtr vm, - virFileCachePtr qemuCapsCache) -{ - qemuDomainObjPrivatePtr priv = vm->privateData; - - virObjectUnref(priv->qemuCaps); - if (!(priv->qemuCaps = virQEMUCapsCacheLookupCopy(qemuCapsCache, - vm->def->virtType, - vm->def->emulator, - vm->def->os.machine))) - return -1; - return 0; -} - - char * qemuDomainGetMachineName(virDomainObjPtr vm) { diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index db45a932dc..f626d3a54c 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -1155,10 +1155,6 @@ int qemuDomainFixupCPUs(virDomainObjPtr vm, virCPUDefPtr *origCPU); -int -qemuDomainUpdateQEMUCaps(virDomainObjPtr vm, - virFileCachePtr qemuCapsCache); - char * qemuDomainGetMachineName(virDomainObjPtr vm); diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index a76a8da841..ad3129c7bd 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5486,6 +5486,34 @@ qemuProcessStartUpdateCustomCaps(virDomainObjPtr vm) } +/** + * qemuProcessPrepareQEMUCaps: + * @vm: domain object + * @qemuCapsCache: cache of QEMU capabilities + * + * Prepare the capabilities of a QEMU process for startup. This includes + * copying the caps to a static cache and potential post-processing depending + * on the configuration of the VM and startup process. + * + * Returns 0 on success, -1 on error. + */ +static int +qemuProcessPrepareQEMUCaps(virDomainObjPtr vm, + virFileCachePtr qemuCapsCache) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + + virObjectUnref(priv->qemuCaps); + if (!(priv->qemuCaps = virQEMUCapsCacheLookupCopy(qemuCapsCache, + vm->def->virtType, + vm->def->emulator, + vm->def->os.machine))) + return -1; + + return 0; +} + + /** * qemuProcessInit: * @@ -5537,7 +5565,7 @@ qemuProcessInit(virQEMUDriverPtr driver, } VIR_DEBUG("Determining emulator version"); - if (qemuDomainUpdateQEMUCaps(vm, driver->qemuCapsCache) < 0) + if (qemuProcessPrepareQEMUCaps(vm, driver->qemuCapsCache) < 0) goto cleanup; if (flags & VIR_QEMU_PROCESS_START_STANDALONE) -- 2.23.0
On Mon, Nov 25, 2019 at 01:53:48PM +0100, Peter Krempa wrote:
The function is now used only in qemu_process.c so move it there and name it 'qemuProcessPrepareQEMUCaps' which is more appropriate to what it's doing.
The reworded comment now mentions that it will also post-process the caps for VM startup.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 26 -------------------------- src/qemu/qemu_domain.h | 4 ---- src/qemu/qemu_process.c | 30 +++++++++++++++++++++++++++++- 3 files changed, 29 insertions(+), 31 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Start aggregating all capability post-processing code in one place. The comment was modified while moving it as it was mentioning floppies which are no longer clearing the blockdev capability. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_process.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index ad3129c7bd..f05c1d637f 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5502,6 +5502,7 @@ qemuProcessPrepareQEMUCaps(virDomainObjPtr vm, virFileCachePtr qemuCapsCache) { qemuDomainObjPrivatePtr priv = vm->privateData; + size_t i; virObjectUnref(priv->qemuCaps); if (!(priv->qemuCaps = virQEMUCapsCacheLookupCopy(qemuCapsCache, @@ -5510,6 +5511,14 @@ qemuProcessPrepareQEMUCaps(virDomainObjPtr vm, vm->def->os.machine))) return -1; + /* clear the 'blockdev' capability for VMs which have disks that need -drive */ + for (i = 0; i < vm->def->ndisks; i++) { + if (qemuDiskBusNeedsDriveArg(vm->def->disks[i]->bus)) { + virQEMUCapsClear(priv->qemuCaps, QEMU_CAPS_BLOCKDEV); + break; + } + } + return 0; } @@ -6273,14 +6282,6 @@ qemuProcessPrepareDomain(virQEMUDriverPtr driver, qemuProcessPrepareAllowReboot(vm); - /* clear the 'blockdev' capability for VMs which have disks that need - * -drive or which have floppies where we can't reliably get the QOM path */ - for (i = 0; i < vm->def->ndisks; i++) { - if (qemuDiskBusNeedsDriveArg(vm->def->disks[i]->bus)) { - virQEMUCapsClear(priv->qemuCaps, QEMU_CAPS_BLOCKDEV); - break; - } - } /* * Normally PCI addresses are assigned in the virDomainCreate -- 2.23.0
On Mon, Nov 25, 2019 at 01:53:49PM +0100, Peter Krempa wrote:
Start aggregating all capability post-processing code in one place.
The comment was modified while moving it as it was mentioning floppies which are no longer clearing the blockdev capability.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_process.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
On 11/25/19 7:53 AM, Peter Krempa wrote:
Start aggregating all capability post-processing code in one place.
The comment was modified while moving it as it was mentioning floppies which are no longer clearing the blockdev capability.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_process.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index ad3129c7bd..f05c1d637f 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5502,6 +5502,7 @@ qemuProcessPrepareQEMUCaps(virDomainObjPtr vm, virFileCachePtr qemuCapsCache) { qemuDomainObjPrivatePtr priv = vm->privateData; + size_t i;
virObjectUnref(priv->qemuCaps); if (!(priv->qemuCaps = virQEMUCapsCacheLookupCopy(qemuCapsCache, @@ -5510,6 +5511,14 @@ qemuProcessPrepareQEMUCaps(virDomainObjPtr vm, vm->def->os.machine))) return -1;
+ /* clear the 'blockdev' capability for VMs which have disks that need -drive */ + for (i = 0; i < vm->def->ndisks; i++) { + if (qemuDiskBusNeedsDriveArg(vm->def->disks[i]->bus)) { + virQEMUCapsClear(priv->qemuCaps, QEMU_CAPS_BLOCKDEV); + break; + } + } + return 0; }
@@ -6273,14 +6282,6 @@ qemuProcessPrepareDomain(virQEMUDriverPtr driver,
qemuProcessPrepareAllowReboot(vm);
- /* clear the 'blockdev' capability for VMs which have disks that need - * -drive or which have floppies where we can't reliably get the QOM path */ - for (i = 0; i < vm->def->ndisks; i++) { - if (qemuDiskBusNeedsDriveArg(vm->def->disks[i]->bus)) { - virQEMUCapsClear(priv->qemuCaps, QEMU_CAPS_BLOCKDEV); - break; - } - }
/* * Normally PCI addresses are assigned in the virDomainCreate
This last hunk should remove a newline too Thanks, Cole
Move the post-processing of the QEMU_CAPS_CHARDEV_FD_PASS flag to the new function. The clearing of the capability is based on the presence of VIR_QEMU_PROCESS_START_STANDALONE so we must also pass in the process start flags. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_process.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index f05c1d637f..f36d03b0a6 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5490,6 +5490,7 @@ qemuProcessStartUpdateCustomCaps(virDomainObjPtr vm) * qemuProcessPrepareQEMUCaps: * @vm: domain object * @qemuCapsCache: cache of QEMU capabilities + * @processStartFlags: flags based on the VIR_QEMU_PROCESS_START_* enum * * Prepare the capabilities of a QEMU process for startup. This includes * copying the caps to a static cache and potential post-processing depending @@ -5499,7 +5500,8 @@ qemuProcessStartUpdateCustomCaps(virDomainObjPtr vm) */ static int qemuProcessPrepareQEMUCaps(virDomainObjPtr vm, - virFileCachePtr qemuCapsCache) + virFileCachePtr qemuCapsCache, + unsigned int processStartFlags) { qemuDomainObjPrivatePtr priv = vm->privateData; size_t i; @@ -5519,6 +5521,9 @@ qemuProcessPrepareQEMUCaps(virDomainObjPtr vm, } } + if (processStartFlags & VIR_QEMU_PROCESS_START_STANDALONE) + virQEMUCapsClear(priv->qemuCaps, QEMU_CAPS_CHARDEV_FD_PASS); + return 0; } @@ -5574,12 +5579,9 @@ qemuProcessInit(virQEMUDriverPtr driver, } VIR_DEBUG("Determining emulator version"); - if (qemuProcessPrepareQEMUCaps(vm, driver->qemuCapsCache) < 0) + if (qemuProcessPrepareQEMUCaps(vm, driver->qemuCapsCache, flags) < 0) goto cleanup; - if (flags & VIR_QEMU_PROCESS_START_STANDALONE) - virQEMUCapsClear(priv->qemuCaps, QEMU_CAPS_CHARDEV_FD_PASS); - if (qemuDomainUpdateCPU(vm, updatedCPU, &origCPU) < 0) goto cleanup; -- 2.23.0
On 11/25/19 7:53 AM, Peter Krempa wrote:
Move the post-processing of the QEMU_CAPS_CHARDEV_FD_PASS flag to the new function.
The clearing of the capability is based on the presence of VIR_QEMU_PROCESS_START_STANDALONE so we must also pass in the process start flags.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_process.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)
Reviewed-by: Cole Robinson <crobinso@redhat.com> - Cole
Do all post-processing of capabilities in qemuProcessPrepareQEMUCaps. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_process.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index f36d03b0a6..f170554198 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5524,6 +5524,10 @@ qemuProcessPrepareQEMUCaps(virDomainObjPtr vm, if (processStartFlags & VIR_QEMU_PROCESS_START_STANDALONE) virQEMUCapsClear(priv->qemuCaps, QEMU_CAPS_CHARDEV_FD_PASS); + /* Update qemu capabilities according to lists passed in via namespace */ + if (qemuProcessStartUpdateCustomCaps(vm) < 0) + return -1; + return 0; } @@ -5596,10 +5600,6 @@ qemuProcessInit(virQEMUDriverPtr driver, if (virDomainObjSetDefTransient(caps, driver->xmlopt, vm, priv->qemuCaps) < 0) goto cleanup; - /* Update qemu capabilities according to lists passed in via namespace */ - if (qemuProcessStartUpdateCustomCaps(vm) < 0) - goto cleanup; - if (flags & VIR_QEMU_PROCESS_START_PRETEND) { if (qemuDomainSetPrivatePaths(driver, vm) < 0) { virDomainObjRemoveTransientDef(vm); -- 2.23.0
On 11/25/19 7:53 AM, Peter Krempa wrote:
Do all post-processing of capabilities in qemuProcessPrepareQEMUCaps.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_process.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
Reviewed-by: Cole Robinson <crobinso@redhat.com> - Cole
Checking whether a qemu capability set right before clearing it without any other logic doesn't make sense. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_capabilities.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index e5f19ddcaa..7c0533aef5 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -4602,8 +4602,7 @@ virQEMUCapsInitProcessCaps(virQEMUCapsPtr qemuCaps) /* Prealloc on NVDIMMs is broken on older QEMUs leading to * user data corruption. If we are dealing with such version * of QEMU pretend we don't know how to NVDIMM. */ - if (qemuCaps->version < 2009000 && - virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_NVDIMM)) + if (qemuCaps->version < 2009000) virQEMUCapsClear(qemuCaps, QEMU_CAPS_DEVICE_NVDIMM); if (ARCH_IS_X86(qemuCaps->arch) && -- 2.23.0
On 11/25/19 7:53 AM, Peter Krempa wrote:
Checking whether a qemu capability set right before clearing it without any other logic doesn't make sense.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_capabilities.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
Reviewed-by: Cole Robinson <crobinso@redhat.com> - Cole
Blockdev is required to do incremental backups properly. Add a helper function for locking out capabilities and export it to allow re-doing the processing if a different code path modifies capabilities. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_capabilities.c | 17 +++++++++++++++++ src/qemu/qemu_capabilities.h | 2 ++ 2 files changed, 19 insertions(+) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 7c0533aef5..124619b733 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -4577,6 +4577,21 @@ virQEMUCapsInitQMPVersionCaps(virQEMUCapsPtr qemuCaps) } +/** + * virQEMUCapsInitProcessCapsInterlock: + * @qemuCaps: QEMU capabilities + * + * A capability which requires a different capability being present in order + * for libvirt to be able to drive it properly should be processed here. + */ +void +virQEMUCapsInitProcessCapsInterlock(virQEMUCapsPtr qemuCaps) +{ + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_BLOCKDEV)) + virQEMUCapsClear(qemuCaps, QEMU_CAPS_INCREMENTAL_BACKUP); +} + + /** * virQEMUCapsInitProcessCaps: * @qemuCaps: QEMU capabilities @@ -4627,6 +4642,8 @@ virQEMUCapsInitProcessCaps(virQEMUCapsPtr qemuCaps) virQEMUCapsGet(qemuCaps, QEMU_CAPS_SCSI_DISK_DEVICE_ID) && virQEMUCapsGet(qemuCaps, QEMU_CAPS_SAVEVM_MONITOR_NODES)) virQEMUCapsSet(qemuCaps, QEMU_CAPS_BLOCKDEV); + + virQEMUCapsInitProcessCapsInterlock(qemuCaps); } diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index 3fd8bebe79..4d7d836e8c 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -555,6 +555,8 @@ void virQEMUCapsClear(virQEMUCapsPtr qemuCaps, bool virQEMUCapsGet(virQEMUCapsPtr qemuCaps, virQEMUCapsFlags flag); +void virQEMUCapsInitProcessCapsInterlock(virQEMUCapsPtr qemuCaps); + bool virQEMUCapsHasPCIMultiBus(virQEMUCapsPtr qemuCaps, const virDomainDef *def); -- 2.23.0
On 11/25/19 7:53 AM, Peter Krempa wrote:
Blockdev is required to do incremental backups properly. Add a helper function for locking out capabilities and export it to allow re-doing the processing if a different code path modifies capabilities.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_capabilities.c | 17 +++++++++++++++++ src/qemu/qemu_capabilities.h | 2 ++ 2 files changed, 19 insertions(+)
Reviewed-by: Cole Robinson <crobinso@redhat.com> this made me go look at the case that _BLOCKDEV is disabled. bus='sd', hmm that's unfortunate. Do you know if there's qemu plans to make it -blockdev compatible? Maybe -machine option like was done with -pflash? Or are we stuck with that indefinitely? Thanks, Cole
On Tue, Nov 26, 2019 at 14:57:25 -0500, Cole Robinson wrote:
On 11/25/19 7:53 AM, Peter Krempa wrote:
Blockdev is required to do incremental backups properly. Add a helper function for locking out capabilities and export it to allow re-doing the processing if a different code path modifies capabilities.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_capabilities.c | 17 +++++++++++++++++ src/qemu/qemu_capabilities.h | 2 ++ 2 files changed, 19 insertions(+)
Reviewed-by: Cole Robinson <crobinso@redhat.com>
this made me go look at the case that _BLOCKDEV is disabled. bus='sd', hmm that's unfortunate. Do you know if there's qemu plans to make it -blockdev compatible? Maybe -machine option like was done with -pflash? Or are we stuck with that indefinitely?
There are IIRC 3 board models which don't support sd via -device. Also there isn't much documentation that would guide us how to use -device instead of -drive for this. Said that, most boards are already converted and have e.g. explicit SD controllers, so many cases would be fixable, but that requires some deep knowledge of qemu.
We clear some capabilities here so the lockouts need to be re-evaluated. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_process.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index f170554198..bc97ec3bb4 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5528,6 +5528,9 @@ qemuProcessPrepareQEMUCaps(virDomainObjPtr vm, if (qemuProcessStartUpdateCustomCaps(vm) < 0) return -1; + /* re-process capability lockouts since we might have removed capabilities */ + virQEMUCapsInitProcessCapsInterlock(priv->qemuCaps); + return 0; } -- 2.23.0
On 11/25/19 7:53 AM, Peter Krempa wrote:
We clear some capabilities here so the lockouts need to be re-evaluated.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_process.c | 3 +++ 1 file changed, 3 insertions(+)
Reviewed-by: Cole Robinson <crobinso@redhat.com> - Cole
participants (4)
-
Cole Robinson -
Daniel P. Berrangé -
Ján Tomko -
Peter Krempa