[libvirt] [PATCH 00/11] Avoid numerous calls of virQEMUCapsCacheLookup
For a domain definition there are numerous calls of virQEMUCapsCacheLookup (the same applies to the domain start). This slows down the process since virQEMUCapsCacheLookup validates that the QEMU capabilitites are still valid (among other things, a fork is done for this if the user for the QEMU processes is 'qemu'). Therefore let's reduce the number of virQEMUCapsCacheLookup calls whenever possible and reasonable. In addition to the speed up, there is the risk that virQEMUCapsCacheLookup returns different QEMU capabilities during a task if, for example, the QEMU binary has changed during the task. The correct way would be: - get the QEMU capabilities only once per task via virQEMUCapsCacheLookup - do the task with these QEMU capabilities or - abort the task after a cache invalidation Note: With this patch series the behavior is still not (completely) fixed, but the performance has been significantly improved. In a quick test this gave a speed up of factor 4 for a simple define/undefine loop. In general, the more devices a domain has, the more drastic the overhead becomes (because a cache validation is performed for each device). Marc Hartmayer (11): qemu: Use VIR_STEAL_PTR macro qemu: Introduce qemuDomainUpdateQEMUCaps() qemu: Pass QEMUCaps to virDomainDefPostParse conf: Use getParseOpaque() in virDomainObjSetDefTransient conf: Add function description for virDomainDefPostParse conf: Get rid of virDomainDeviceDefPostParseOne conf: Extend virDomainDefValidate(Callback) for parseOpaque conf: Use domainPostParseData(Alloc|Free) in virDomainDefValidate qemu: Use @parseOpaque in qemuDomainDefValidate conf: Extend virDomainDeviceDefValidate(Callback) for parseOpaque qemu: Use @parseOpaque in qemuDomainDeviceDefValidate src/conf/domain_conf.c | 98 ++++++++++++++++++++++++++--------------- src/conf/domain_conf.h | 9 ++-- src/qemu/qemu_domain.c | 82 +++++++++++++++++++--------------- src/qemu/qemu_domain.h | 4 ++ src/qemu/qemu_process.c | 18 +++----- src/vz/vz_driver.c | 6 ++- 6 files changed, 128 insertions(+), 89 deletions(-) -- 2.17.0
Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> Reviewed-by: Stefan Zimmermann <stzi@linux.ibm.com> --- src/qemu/qemu_domain.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 2fd8a2a268cd..06aa1fac5d0b 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2921,8 +2921,7 @@ qemuDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt, } } - priv->qemuCaps = qemuCaps; - qemuCaps = NULL; + VIR_STEAL_PTR(priv->qemuCaps, qemuCaps); } VIR_FREE(nodes); -- 2.17.0
On 9/20/18 1:44 PM, Marc Hartmayer wrote:
Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> Reviewed-by: Stefan Zimmermann <stzi@linux.ibm.com> --- src/qemu/qemu_domain.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
Reviewed-by: John Ferlan <jferlan@redhat.com> Trivial, but I'll wait until freeze of off before pushing... John
This function updates the used QEMU capabilities of @vm by querying the QEMU capabilities cache. Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> --- src/qemu/qemu_domain.c | 25 +++++++++++++++++++++++++ src/qemu/qemu_domain.h | 4 ++++ src/qemu/qemu_process.c | 14 +++----------- 3 files changed, 32 insertions(+), 11 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 06aa1fac5d0b..eb80711597cb 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -13071,6 +13071,31 @@ 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->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 914c9a6a8d14..080cac76e2b1 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -1030,6 +1030,10 @@ 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 f9a01daee78a..44c63c42d618 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5287,10 +5287,7 @@ qemuProcessInit(virQEMUDriverPtr driver, } VIR_DEBUG("Determining emulator version"); - virObjectUnref(priv->qemuCaps); - if (!(priv->qemuCaps = virQEMUCapsCacheLookupCopy(driver->qemuCapsCache, - vm->def->emulator, - vm->def->os.machine))) + if (qemuDomainUpdateQEMUCaps(vm, driver->qemuCapsCache) < 0) goto cleanup; if (flags & VIR_QEMU_PROCESS_START_STANDALONE) @@ -7391,10 +7388,7 @@ int qemuProcessAttach(virConnectPtr conn ATTRIBUTE_UNUSED, goto error; VIR_DEBUG("Determining emulator version"); - virObjectUnref(priv->qemuCaps); - if (!(priv->qemuCaps = virQEMUCapsCacheLookupCopy(driver->qemuCapsCache, - vm->def->emulator, - vm->def->os.machine))) + if (qemuDomainUpdateQEMUCaps(vm, driver->qemuCapsCache) < 0) goto error; VIR_DEBUG("Preparing monitor state"); @@ -7857,9 +7851,7 @@ qemuProcessReconnect(void *opaque) * caps in the domain status, so re-query them */ if (!priv->qemuCaps && - !(priv->qemuCaps = virQEMUCapsCacheLookupCopy(driver->qemuCapsCache, - obj->def->emulator, - obj->def->os.machine))) + (qemuDomainUpdateQEMUCaps(obj, driver->qemuCapsCache) < 0)) goto error; /* In case the domain shutdown while we were not running, -- 2.17.0
On 9/20/18 1:44 PM, Marc Hartmayer wrote:
This function updates the used QEMU capabilities of @vm by querying the QEMU capabilities cache.
Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> --- src/qemu/qemu_domain.c | 25 +++++++++++++++++++++++++ src/qemu/qemu_domain.h | 4 ++++ src/qemu/qemu_process.c | 14 +++----------- 3 files changed, 32 insertions(+), 11 deletions(-)
Reviewed-by: John Ferlan <jferlan@redhat.com> (less trivial, but will wait for freeze to be over before pushing) John
...although priv->qemuCaps will be NULL in almost every case when the post parse callback has failed. That may change in the future. Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> --- src/qemu/qemu_process.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 44c63c42d618..6c5a6472d8cd 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5282,7 +5282,7 @@ qemuProcessInit(virQEMUDriverPtr driver, if (vm->def->postParseFailed) { VIR_DEBUG("re-running the post parse callback"); - if (virDomainDefPostParse(vm->def, caps, 0, driver->xmlopt, NULL) < 0) + if (virDomainDefPostParse(vm->def, caps, 0, driver->xmlopt, priv->qemuCaps) < 0) goto cleanup; } -- 2.17.0
On 9/20/18 1:44 PM, Marc Hartmayer wrote:
...although priv->qemuCaps will be NULL in almost every case when the post parse callback has failed. That may change in the future.
Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> --- src/qemu/qemu_process.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 44c63c42d618..6c5a6472d8cd 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5282,7 +5282,7 @@ qemuProcessInit(virQEMUDriverPtr driver,
The comment just above here could use a tweak for grammar ;-): /* in case when the post parse callback failed we need to re-run it on the * old config prior we start the VM */
if (vm->def->postParseFailed) { VIR_DEBUG("re-running the post parse callback");
- if (virDomainDefPostParse(vm->def, caps, 0, driver->xmlopt, NULL) < 0) + if (virDomainDefPostParse(vm->def, caps, 0, driver->xmlopt, priv->qemuCaps) < 0)
Searching through history of this line finds Peter's original commit in this area - 7726d158, which seems to indicate a very specific reason for providing a NULL capabilities value here. I think from this patch on is something Peter has worked on a lot, so I would prefer to defer to Peter on them since I'm sure he understands all the various PostParse and Validate special conditions and various flags usage better than I do. I'll still provide a few comments along the way. John I did CC Peter just to bring his attention to the series...
goto cleanup; }
On Fri, Sep 28, 2018 at 22:02:59 -0400, John Ferlan wrote:
On 9/20/18 1:44 PM, Marc Hartmayer wrote:
...although priv->qemuCaps will be NULL in almost every case when the post parse callback has failed. That may change in the future.
Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> --- src/qemu/qemu_process.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 44c63c42d618..6c5a6472d8cd 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5282,7 +5282,7 @@ qemuProcessInit(virQEMUDriverPtr driver,
The comment just above here could use a tweak for grammar ;-):
/* in case when the post parse callback failed we need to re-run it on the * old config prior we start the VM */
if (vm->def->postParseFailed) { VIR_DEBUG("re-running the post parse callback");
- if (virDomainDefPostParse(vm->def, caps, 0, driver->xmlopt, NULL) < 0) + if (virDomainDefPostParse(vm->def, caps, 0, driver->xmlopt, priv->qemuCaps) < 0)
Searching through history of this line finds Peter's original commit in this area - 7726d158, which seems to indicate a very specific reason for providing a NULL capabilities value here.
I think from this patch on is something Peter has worked on a lot, so I would prefer to defer to Peter on them since I'm sure he understands all the various PostParse and Validate special conditions and various flags usage better than I do.
Thanks for notifying me. In general, when parsing a "new" domain config as it's the case here, NULL should be passed in. The qemu driver registers the 'domainPostParseDataAlloc' callback to qemuDomainPostParseDataAlloc. This callback is executed after the 'basic' post parse callback which fills in the emulator binary. Passing an explicit qemuCaps is meant only for special cases e.g. migration where we have a specific set of capabilities which need to be used rather than a new one. This patch does not seem to make sense with the justification provided here as the post-parse callbacks fill in the proper caps here and this part clearly uses a new domain configuration, so the default behaviour should be used. Changing this would also need that we change the normal parser path to achieve the same results which is impossible as you don't have access to priv->qemuCaps prior to creating the virDomainObj object.
On Mon, Oct 01, 2018 at 09:33 AM +0200, Peter Krempa <pkrempa@redhat.com> wrote:
On Fri, Sep 28, 2018 at 22:02:59 -0400, John Ferlan wrote:
On 9/20/18 1:44 PM, Marc Hartmayer wrote:
...although priv->qemuCaps will be NULL in almost every case when the post parse callback has failed. That may change in the future.
Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> --- src/qemu/qemu_process.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 44c63c42d618..6c5a6472d8cd 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5282,7 +5282,7 @@ qemuProcessInit(virQEMUDriverPtr driver,
The comment just above here could use a tweak for grammar ;-):
/* in case when the post parse callback failed we need to re-run it on the * old config prior we start the VM */
if (vm->def->postParseFailed) { VIR_DEBUG("re-running the post parse callback");
- if (virDomainDefPostParse(vm->def, caps, 0, driver->xmlopt, NULL) < 0) + if (virDomainDefPostParse(vm->def, caps, 0, driver->xmlopt, priv->qemuCaps) < 0)
Searching through history of this line finds Peter's original commit in this area - 7726d158, which seems to indicate a very specific reason for providing a NULL capabilities value here.
I think from this patch on is something Peter has worked on a lot, so I would prefer to defer to Peter on them since I'm sure he understands all the various PostParse and Validate special conditions and various flags usage better than I do.
Thanks for notifying me.
First, thanks for your answer!
In general, when parsing a "new" domain config as it's the case here, NULL should be passed in.
Wouldn’t it be better to invalidate the QEMU caps (priv->qemuCaps) explicitly beforehand? (for example if the 'postParseFailed' flag is set and we’re starting the domain).
The qemu driver registers the 'domainPostParseDataAlloc' callback to qemuDomainPostParseDataAlloc. This callback is executed after the 'basic' post parse callback which fills in the emulator binary.
Passing an explicit qemuCaps is meant only for special cases e.g. migration where we have a specific set of capabilities which need to be used rather than a new one.
I would do all the precondition stuff (e.g. the priv->qemuCaps isn’t valid anymore) as early as possible in the overall “task/job” process.
This patch does not seem to make sense with the justification provided here as the post-parse callbacks fill in the proper caps here and this part clearly uses a new domain configuration, so the default behaviour should be used.
Changing this would also need that we change the normal parser path to achieve the same results which is impossible as you don't have access to priv->qemuCaps prior to creating the virDomainObj object.
Yep, that’s why I said in the cover letter “With this patch series the behavior is still not (completely) fixed, but the performance has been significantly improved.”. IMHO, it’s a design flaw that the virDomainObjList class is responsible for the creation of the virDomainObj… The responsibilities of the classes are mixed and this enforces the lock order virDomainObjList -> virDomainObj (what actually is a performance bottleneck). IMHO, we should create the virDomainObj earlier and add it to the virDomainObjList when it’s useful. What’s your opinion about that?
-- Kind regards / Beste Grüße Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzende des Aufsichtsrats: Martina Koederitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
On Mon, Oct 01, 2018 at 10:31:36 +0200, Marc Hartmayer wrote:
On Mon, Oct 01, 2018 at 09:33 AM +0200, Peter Krempa <pkrempa@redhat.com> wrote:
On Fri, Sep 28, 2018 at 22:02:59 -0400, John Ferlan wrote:
On 9/20/18 1:44 PM, Marc Hartmayer wrote:
...although priv->qemuCaps will be NULL in almost every case when the post parse callback has failed. That may change in the future.
Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> --- src/qemu/qemu_process.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 44c63c42d618..6c5a6472d8cd 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5282,7 +5282,7 @@ qemuProcessInit(virQEMUDriverPtr driver,
The comment just above here could use a tweak for grammar ;-):
/* in case when the post parse callback failed we need to re-run it on the * old config prior we start the VM */
if (vm->def->postParseFailed) { VIR_DEBUG("re-running the post parse callback");
- if (virDomainDefPostParse(vm->def, caps, 0, driver->xmlopt, NULL) < 0) + if (virDomainDefPostParse(vm->def, caps, 0, driver->xmlopt, priv->qemuCaps) < 0)
Searching through history of this line finds Peter's original commit in this area - 7726d158, which seems to indicate a very specific reason for providing a NULL capabilities value here.
I think from this patch on is something Peter has worked on a lot, so I would prefer to defer to Peter on them since I'm sure he understands all the various PostParse and Validate special conditions and various flags usage better than I do.
Thanks for notifying me.
First, thanks for your answer!
In general, when parsing a "new" domain config as it's the case here, NULL should be passed in.
Wouldn’t it be better to invalidate the QEMU caps (priv->qemuCaps) explicitly beforehand? (for example if the 'postParseFailed' flag is set and we’re starting the domain).
As stated below, in the normal code-flow at the point when the XML is parsed there is no virDomainObj and thus no priv. Even when you create a virDomainObj prior to that, the priv->qemuCaps is based on the binary which in turn is based on defaults added by the post parse callback as the binary is a qemu-driver-specific.
The qemu driver registers the 'domainPostParseDataAlloc' callback to qemuDomainPostParseDataAlloc. This callback is executed after the 'basic' post parse callback which fills in the emulator binary.
Passing an explicit qemuCaps is meant only for special cases e.g. migration where we have a specific set of capabilities which need to be used rather than a new one.
I would do all the precondition stuff (e.g. the priv->qemuCaps isn’t valid anymore) as early as possible in the overall “task/job” process.
Well, technically priv->qemuCaps is invalid and unneeded when the VM is not running and valid and immutable if it is running. If the VM is not running, there is no qemu process associated to it so it does not make sense to store random qemuCaps along with it, since the qemu binary might change at the point when we attempt to start it. I was not a fan of needing qemuCaps in the post parse infrastructure of qemu, but at this point we can't do much about it especially as we want to keep the config stable after defined.
This patch does not seem to make sense with the justification provided here as the post-parse callbacks fill in the proper caps here and this part clearly uses a new domain configuration, so the default behaviour should be used.
Changing this would also need that we change the normal parser path to achieve the same results which is impossible as you don't have access to priv->qemuCaps prior to creating the virDomainObj object.
Yep, that’s why I said in the cover letter “With this patch series the behavior is still not (completely) fixed, but the performance has been significantly improved.”.
I don't see what should be fixed here, but mostly as I did not read the series.
IMHO, it’s a design flaw that the virDomainObjList class is responsible for the creation of the virDomainObj… The responsibilities of the classes are mixed and this enforces the lock order virDomainObjList -> virDomainObj (what actually is a performance bottleneck). IMHO, we should create the virDomainObj earlier and add it to the virDomainObjList when it’s useful.
I think that is orthogonal from the post parse infrastructure. Post parse applies on virDomainDef object, while the domain list is of virDomainObj type. That is thre reason to have private copy of the caps in the post parse code flow. You don't have to associate a config with a domain all the time.
What’s your opinion about that?
I did not touch the domain obj list for quite some time so I don't have any guidance on whether its possible/feasible to untangle it.
On Mon, Oct 01, 2018 at 12:36 PM +0200, Peter Krempa <pkrempa@redhat.com> wrote: […snip…]
I would do all the precondition stuff (e.g. the priv->qemuCaps isn’t valid anymore) as early as possible in the overall “task/job” process.
Well, technically priv->qemuCaps is invalid and unneeded when the VM is not running and valid and immutable if it is running.
If the VM is not running, there is no qemu process associated to it so it does not make sense to store random qemuCaps along with it, since the qemu binary might change at the point when we attempt to start it.
Yep, that makes sense.
I was not a fan of needing qemuCaps in the post parse infrastructure of qemu, but at this point we can't do much about it especially as we want to keep the config stable after defined.
This patch does not seem to make sense with the justification provided here as the post-parse callbacks fill in the proper caps here and this part clearly uses a new domain configuration, so the default behaviour should be used.
Changing this would also need that we change the normal parser path to achieve the same results which is impossible as you don't have access to priv->qemuCaps prior to creating the virDomainObj object.
Yep, that’s why I said in the cover letter “With this patch series the behavior is still not (completely) fixed, but the performance has been significantly improved.”.
I don't see what should be fixed here, but mostly as I did not read the series.
The problem is that we currently don’t take into account that the QEMU binary can change during a task (e.g. define a domain). Therefore, it’s possible that two calls of virQEMUCapsCacheLookup return different QEMU capabilities. […snip] -- Kind regards / Beste Grüße Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzende des Aufsichtsrats: Martina Koederitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
On Mon, Oct 01, 2018 at 14:58:11 +0200, Marc Hartmayer wrote:
On Mon, Oct 01, 2018 at 12:36 PM +0200, Peter Krempa <pkrempa@redhat.com> wrote:
[...]
Yep, that’s why I said in the cover letter “With this patch series the behavior is still not (completely) fixed, but the performance has been significantly improved.”.
I don't see what should be fixed here, but mostly as I did not read the series.
The problem is that we currently don’t take into account that the QEMU binary can change during a task (e.g. define a domain). Therefore, it’s possible that two calls of virQEMUCapsCacheLookup return different QEMU capabilities.
I don't think that is too big of a problem in the light that the binary can change between the final call to virQEMUCapsCacheLookup which is used by the code generating the commandline and actual exec of the qemu binary once the commandline is generated. Solving that won't be easy at all. Since virDomainDefValidate is called with the final capabilities instance when starting the qemu process the possibility that it would change and the problems it might create are negligible when compared to actually execing something else.
This allows the QEMU driver to use the originally used QEMU capabilities for copying the domain. It avoids the usage of a possible changed QEMU capability as virQEMUCapsCacheLookup might return a different QEMU capability than used before (for example when during the job the QEMU binary has been replaced virQEMUCapsCacheLookupCopy will invalidate the originally used QEMU capability). For other drivers @parseOpqaue will still be NULL, because xmlopt->privateData.getParseOpaque is currently only implemented for the QEMU driver. Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> Reviewed-by: Stefan Zimmermann <stzi@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> --- src/conf/domain_conf.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 1ee43950ae2d..a3f2fcb0a001 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3366,6 +3366,7 @@ virDomainObjSetDefTransient(virCapsPtr caps, virDomainObjPtr domain) { int ret = -1; + void *parseOpaque = NULL; if (!domain->persistent) return 0; @@ -3373,7 +3374,10 @@ virDomainObjSetDefTransient(virCapsPtr caps, if (domain->newDef) return 0; - if (!(domain->newDef = virDomainDefCopy(domain->def, caps, xmlopt, NULL, false))) + if (xmlopt->privateData.getParseOpaque) + parseOpaque = xmlopt->privateData.getParseOpaque(domain); + + if (!(domain->newDef = virDomainDefCopy(domain->def, caps, xmlopt, parseOpaque, false))) goto out; ret = 0; -- 2.17.0
On 9/20/18 1:44 PM, Marc Hartmayer wrote:
This allows the QEMU driver to use the originally used QEMU capabilities for copying the domain. It avoids the usage of a possible changed QEMU capability as virQEMUCapsCacheLookup might return a different QEMU capability than used before (for example when during the job the QEMU binary has been replaced virQEMUCapsCacheLookupCopy will invalidate the originally used QEMU capability).
For other drivers @parseOpqaue will still be NULL, because xmlopt->privateData.getParseOpaque is currently only implemented for the QEMU driver.
Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> Reviewed-by: Stefan Zimmermann <stzi@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> --- src/conf/domain_conf.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
This one I'm not so sure about. One would think the domain def that's being copied would have already gone through PostParse, et. al. The SetDefTransient is where we're copying the recently Parsed and Validated @def, so why would it need the qemuCaps especially since @flags would be PARSE_INACTIVE | PARSE_SKIP_VALIDATE in *DefCopy. John
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 1ee43950ae2d..a3f2fcb0a001 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3366,6 +3366,7 @@ virDomainObjSetDefTransient(virCapsPtr caps, virDomainObjPtr domain) { int ret = -1; + void *parseOpaque = NULL;
if (!domain->persistent) return 0; @@ -3373,7 +3374,10 @@ virDomainObjSetDefTransient(virCapsPtr caps, if (domain->newDef) return 0;
- if (!(domain->newDef = virDomainDefCopy(domain->def, caps, xmlopt, NULL, false))) + if (xmlopt->privateData.getParseOpaque) + parseOpaque = xmlopt->privateData.getParseOpaque(domain); + + if (!(domain->newDef = virDomainDefCopy(domain->def, caps, xmlopt, parseOpaque, false))) goto out;
ret = 0;
On Sat, Sep 29, 2018 at 04:06 AM +0200, John Ferlan <jferlan@redhat.com> wrote:
On 9/20/18 1:44 PM, Marc Hartmayer wrote:
This allows the QEMU driver to use the originally used QEMU capabilities for copying the domain. It avoids the usage of a possible changed QEMU capability as virQEMUCapsCacheLookup might return a different QEMU capability than used before (for example when during the job the QEMU binary has been replaced virQEMUCapsCacheLookupCopy will invalidate the originally used QEMU capability).
For other drivers @parseOpqaue will still be NULL, because xmlopt->privateData.getParseOpaque is currently only implemented for the QEMU driver.
Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> Reviewed-by: Stefan Zimmermann <stzi@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> --- src/conf/domain_conf.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
This one I'm not so sure about. One would think the domain def that's being copied would have already gone through PostParse, et. al. The SetDefTransient is where we're copying the recently Parsed and Validated @def, so why would it need the qemuCaps especially since @flags would be PARSE_INACTIVE | PARSE_SKIP_VALIDATE in *DefCopy.
As far as I can see virDomainDefParseNode doesn’t skip the virDomainDefPostParse part (there are the caps needed). Can you please double check this? An interesting scenario/test would be to see what happens if the QEMU binary (and therefore the capabilities) has changed before the virDomainObjSetDefTransient call.
John
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 1ee43950ae2d..a3f2fcb0a001 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3366,6 +3366,7 @@ virDomainObjSetDefTransient(virCapsPtr caps, virDomainObjPtr domain) { int ret = -1; + void *parseOpaque = NULL;
if (!domain->persistent) return 0; @@ -3373,7 +3374,10 @@ virDomainObjSetDefTransient(virCapsPtr caps, if (domain->newDef) return 0;
- if (!(domain->newDef = virDomainDefCopy(domain->def, caps, xmlopt, NULL, false))) + if (xmlopt->privateData.getParseOpaque) + parseOpaque = xmlopt->privateData.getParseOpaque(domain); + + if (!(domain->newDef = virDomainDefCopy(domain->def, caps, xmlopt, parseOpaque, false))) goto out;
ret = 0;
-- Kind regards / Beste Grüße Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzende des Aufsichtsrats: Martina Koederitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
On Thu, Sep 20, 2018 at 19:44:50 +0200, Marc Hartmayer wrote:
This allows the QEMU driver to use the originally used QEMU capabilities for copying the domain. It avoids the usage of a possible changed QEMU capability as virQEMUCapsCacheLookup might return a different QEMU capability than used before (for example when during the job the QEMU binary has been replaced virQEMUCapsCacheLookupCopy will invalidate the originally used QEMU capability).
For other drivers @parseOpqaue will still be NULL, because xmlopt->privateData.getParseOpaque is currently only implemented for the QEMU driver.
Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> Reviewed-by: Stefan Zimmermann <stzi@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> --- src/conf/domain_conf.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 1ee43950ae2d..a3f2fcb0a001 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3366,6 +3366,7 @@ virDomainObjSetDefTransient(virCapsPtr caps, virDomainObjPtr domain) { int ret = -1; + void *parseOpaque = NULL;
if (!domain->persistent) return 0; @@ -3373,7 +3374,10 @@ virDomainObjSetDefTransient(virCapsPtr caps, if (domain->newDef) return 0;
- if (!(domain->newDef = virDomainDefCopy(domain->def, caps, xmlopt, NULL, false))) + if (xmlopt->privateData.getParseOpaque) + parseOpaque = xmlopt->privateData.getParseOpaque(domain);
I'd prefer if the caller passes this in explicitly since virDomainDefCopy uses it explicitly. Additionally the caller will be aware where the pointer is taken. The callback is really used only in the place where we are restoring the existing domain object from the status XML and thus we really need the specific caps they were used. Since the code is common the callback needs to be used. Ideally priv->qemuCaps will be cleared when the VM is NOT running so at the point above you'd get NULL anyways in most cases.
Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> --- src/conf/domain_conf.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index a3f2fcb0a001..3c307d325a89 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5215,6 +5215,19 @@ virDomainDefPostParseCheckFailure(virDomainDefPtr def, } +/** + * virDomainDefPostParse: + * @def: domain definition + * @caps: driver capabilities object + * @parseFlags: virDomainDefParseFlags + * @xmlopt: XML parser option object + * @parseOpaque: opaque data and it might be NULL (for QEMU driver it's qemuCaps) + * + * This function does various common and hypervisor specific auto + * generation, verification, and validation. + * + * Returns 0 on success, -1 on error + */ int virDomainDefPostParse(virDomainDefPtr def, virCapsPtr caps, -- 2.17.0
On 9/20/18 1:44 PM, Marc Hartmayer wrote:
Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> --- src/conf/domain_conf.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index a3f2fcb0a001..3c307d325a89 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5215,6 +5215,19 @@ virDomainDefPostParseCheckFailure(virDomainDefPtr def, }
+/** + * virDomainDefPostParse: + * @def: domain definition + * @caps: driver capabilities object + * @parseFlags: virDomainDefParseFlags + * @xmlopt: XML parser option object + * @parseOpaque: opaque data and it might be NULL (for QEMU driver it's qemuCaps) + * + * This function does various common and hypervisor specific auto + * generation, verification, and validation.
I think this is a good start. I'd really like to see what the "expectations" are for various @parseFlags. By far one of the more confusing things. Also while we're at it - @parseOpaque details and/or assumptions w/r/t whether it should be provided or not. I also note a few callers use the comment "/* callback to fill driver specific domain aspects */", so that maybe good to pull in... John
+ * + * Returns 0 on success, -1 on error + */ int virDomainDefPostParse(virDomainDefPtr def, virCapsPtr caps,
On Sat, Sep 29, 2018 at 04:08 AM +0200, John Ferlan <jferlan@redhat.com> wrote:
On 9/20/18 1:44 PM, Marc Hartmayer wrote:
Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> --- src/conf/domain_conf.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index a3f2fcb0a001..3c307d325a89 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5215,6 +5215,19 @@ virDomainDefPostParseCheckFailure(virDomainDefPtr def, }
+/** + * virDomainDefPostParse: + * @def: domain definition + * @caps: driver capabilities object + * @parseFlags: virDomainDefParseFlags + * @xmlopt: XML parser option object + * @parseOpaque: opaque data and it might be NULL (for QEMU driver it's qemuCaps) + * + * This function does various common and hypervisor specific auto + * generation, verification, and validation.
I think this is a good start. I'd really like to see what the "expectations" are for various @parseFlags. By far one of the more confusing things. Also while we're at it - @parseOpaque details and/or assumptions w/r/t whether it should be provided or not.
I really hope this function will be divided into separate functions in some time… Currently, it does way _too_ many things.
I also note a few callers use the comment "/* callback to fill driver specific domain aspects */", so that maybe good to pull in...
Okay.
John
+ * + * Returns 0 on success, -1 on error + */ int virDomainDefPostParse(virDomainDefPtr def, virCapsPtr caps,
-- Kind regards / Beste Grüße Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzende des Aufsichtsrats: Martina Koederitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
Move the domainPostParseDataAlloc/Free calls to virDomainDeviceDefParse. As an consequence virDomainDeviceDefPostParseOne is superfluous and can therefore be removed. Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> --- src/conf/domain_conf.c | 37 +++++++++++-------------------------- 1 file changed, 11 insertions(+), 26 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 3c307d325a89..e61f04ea2271 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4900,31 +4900,6 @@ virDomainDeviceDefPostParse(virDomainDeviceDefPtr dev, return 0; } -static int -virDomainDeviceDefPostParseOne(virDomainDeviceDefPtr dev, - const virDomainDef *def, - virCapsPtr caps, - unsigned int flags, - virDomainXMLOptionPtr xmlopt) -{ - void *parseOpaque = NULL; - int ret; - - if (xmlopt->config.domainPostParseDataAlloc) { - if (xmlopt->config.domainPostParseDataAlloc(def, caps, flags, - xmlopt->config.priv, - &parseOpaque) < 0) - return -1; - } - - ret = virDomainDeviceDefPostParse(dev, def, caps, flags, xmlopt, parseOpaque); - - if (parseOpaque && xmlopt->config.domainPostParseDataFree) - xmlopt->config.domainPostParseDataFree(parseOpaque); - - return ret; -} - struct virDomainDefPostParseDeviceIteratorData { virCapsPtr caps; @@ -16066,6 +16041,7 @@ virDomainDeviceDefParse(const char *xmlStr, { xmlDocPtr xml; xmlNodePtr node; + void *parseOpaque = NULL; xmlXPathContextPtr ctxt = NULL; virDomainDeviceDefPtr dev = NULL; char *netprefix; @@ -16222,8 +16198,15 @@ virDomainDeviceDefParse(const char *xmlStr, break; } + if (xmlopt->config.domainPostParseDataAlloc) { + if (xmlopt->config.domainPostParseDataAlloc(def, caps, flags, + xmlopt->config.priv, + &parseOpaque) < 0) + goto error; + } + /* callback to fill driver specific device aspects */ - if (virDomainDeviceDefPostParseOne(dev, def, caps, flags, xmlopt) < 0) + if (virDomainDeviceDefPostParse(dev, def, caps, flags, xmlopt, parseOpaque) < 0) goto error; /* validate the configuration */ @@ -16231,6 +16214,8 @@ virDomainDeviceDefParse(const char *xmlStr, goto error; cleanup: + if (parseOpaque && xmlopt->config.domainPostParseDataFree) + xmlopt->config.domainPostParseDataFree(parseOpaque); xmlFreeDoc(xml); xmlXPathFreeContext(ctxt); return dev; -- 2.17.0
On 9/20/18 1:44 PM, Marc Hartmayer wrote:
Move the domainPostParseDataAlloc/Free calls to virDomainDeviceDefParse. As an consequence virDomainDeviceDefPostParseOne is superfluous and can therefore be removed.
Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> --- src/conf/domain_conf.c | 37 +++++++++++-------------------------- 1 file changed, 11 insertions(+), 26 deletions(-)
I'm not against this per se; however, I should not that the code was specifically extracted in commit e168bc8a. John
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 3c307d325a89..e61f04ea2271 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4900,31 +4900,6 @@ virDomainDeviceDefPostParse(virDomainDeviceDefPtr dev, return 0; }
-static int -virDomainDeviceDefPostParseOne(virDomainDeviceDefPtr dev, - const virDomainDef *def, - virCapsPtr caps, - unsigned int flags, - virDomainXMLOptionPtr xmlopt) -{ - void *parseOpaque = NULL; - int ret; - - if (xmlopt->config.domainPostParseDataAlloc) { - if (xmlopt->config.domainPostParseDataAlloc(def, caps, flags, - xmlopt->config.priv, - &parseOpaque) < 0) - return -1; - } - - ret = virDomainDeviceDefPostParse(dev, def, caps, flags, xmlopt, parseOpaque); - - if (parseOpaque && xmlopt->config.domainPostParseDataFree) - xmlopt->config.domainPostParseDataFree(parseOpaque); - - return ret; -} -
struct virDomainDefPostParseDeviceIteratorData { virCapsPtr caps; @@ -16066,6 +16041,7 @@ virDomainDeviceDefParse(const char *xmlStr, { xmlDocPtr xml; xmlNodePtr node; + void *parseOpaque = NULL; xmlXPathContextPtr ctxt = NULL; virDomainDeviceDefPtr dev = NULL; char *netprefix; @@ -16222,8 +16198,15 @@ virDomainDeviceDefParse(const char *xmlStr, break; }
+ if (xmlopt->config.domainPostParseDataAlloc) { + if (xmlopt->config.domainPostParseDataAlloc(def, caps, flags, + xmlopt->config.priv, + &parseOpaque) < 0) + goto error; + } + /* callback to fill driver specific device aspects */ - if (virDomainDeviceDefPostParseOne(dev, def, caps, flags, xmlopt) < 0) + if (virDomainDeviceDefPostParse(dev, def, caps, flags, xmlopt, parseOpaque) < 0) goto error;
/* validate the configuration */ @@ -16231,6 +16214,8 @@ virDomainDeviceDefParse(const char *xmlStr, goto error;
cleanup: + if (parseOpaque && xmlopt->config.domainPostParseDataFree) + xmlopt->config.domainPostParseDataFree(parseOpaque); xmlFreeDoc(xml); xmlXPathFreeContext(ctxt); return dev;
On Sat, Sep 29, 2018 at 04:09 AM +0200, John Ferlan <jferlan@redhat.com> wrote:
On 9/20/18 1:44 PM, Marc Hartmayer wrote:
Move the domainPostParseDataAlloc/Free calls to virDomainDeviceDefParse. As an consequence virDomainDeviceDefPostParseOne is superfluous and can therefore be removed.
Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> --- src/conf/domain_conf.c | 37 +++++++++++-------------------------- 1 file changed, 11 insertions(+), 26 deletions(-)
I'm not against this per se; however, I should not that the code was specifically extracted in commit e168bc8a.
There are the following three functions: virDomainDeviceDefParse virDomainDeviceDefPostParse virDomainDeviceDefPostParseOne Peter introduced the function “virDomainDeviceDefPostParseOne” to avoid the allocation of private data across the callbacks. This is absolutely fine. What I’ve done is, I moved the domainPostParseDataAlloc/Free calls to virDomainDeviceDefParse instead of having an extra wrapper function (virDomainDeviceDefPostParse/One) for that. With this change I can reuse the QEMU caps for the virDomainDeviceDefValidate call in virDomainDeviceDefParse as well. For safety I added Peter to CC.
John
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 3c307d325a89..e61f04ea2271 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4900,31 +4900,6 @@ virDomainDeviceDefPostParse(virDomainDeviceDefPtr dev, return 0; }
-static int -virDomainDeviceDefPostParseOne(virDomainDeviceDefPtr dev, - const virDomainDef *def, - virCapsPtr caps, - unsigned int flags, - virDomainXMLOptionPtr xmlopt) -{ - void *parseOpaque = NULL; - int ret; - - if (xmlopt->config.domainPostParseDataAlloc) { - if (xmlopt->config.domainPostParseDataAlloc(def, caps, flags, - xmlopt->config.priv, - &parseOpaque) < 0) - return -1; - } - - ret = virDomainDeviceDefPostParse(dev, def, caps, flags, xmlopt, parseOpaque); - - if (parseOpaque && xmlopt->config.domainPostParseDataFree) - xmlopt->config.domainPostParseDataFree(parseOpaque); - - return ret; -} -
struct virDomainDefPostParseDeviceIteratorData { virCapsPtr caps; @@ -16066,6 +16041,7 @@ virDomainDeviceDefParse(const char *xmlStr, { xmlDocPtr xml; xmlNodePtr node; + void *parseOpaque = NULL; xmlXPathContextPtr ctxt = NULL; virDomainDeviceDefPtr dev = NULL; char *netprefix; @@ -16222,8 +16198,15 @@ virDomainDeviceDefParse(const char *xmlStr, break; }
+ if (xmlopt->config.domainPostParseDataAlloc) { + if (xmlopt->config.domainPostParseDataAlloc(def, caps, flags, + xmlopt->config.priv, + &parseOpaque) < 0) + goto error; + } + /* callback to fill driver specific device aspects */ - if (virDomainDeviceDefPostParseOne(dev, def, caps, flags, xmlopt) < 0) + if (virDomainDeviceDefPostParse(dev, def, caps, flags, xmlopt, parseOpaque) < 0) goto error;
/* validate the configuration */ @@ -16231,6 +16214,8 @@ virDomainDeviceDefParse(const char *xmlStr, goto error;
cleanup: + if (parseOpaque && xmlopt->config.domainPostParseDataFree) + xmlopt->config.domainPostParseDataFree(parseOpaque); xmlFreeDoc(xml); xmlXPathFreeContext(ctxt); return dev;
-- Kind regards / Beste Grüße Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzende des Aufsichtsrats: Martina Koederitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
On Mon, Oct 01, 2018 at 12:27:41 +0200, Marc Hartmayer wrote:
On Sat, Sep 29, 2018 at 04:09 AM +0200, John Ferlan <jferlan@redhat.com> wrote:
On 9/20/18 1:44 PM, Marc Hartmayer wrote:
Move the domainPostParseDataAlloc/Free calls to virDomainDeviceDefParse. As an consequence virDomainDeviceDefPostParseOne is superfluous and can therefore be removed.
Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> --- src/conf/domain_conf.c | 37 +++++++++++-------------------------- 1 file changed, 11 insertions(+), 26 deletions(-)
I'm not against this per se; however, I should not that the code was specifically extracted in commit e168bc8a.
There are the following three functions:
virDomainDeviceDefParse virDomainDeviceDefPostParse virDomainDeviceDefPostParseOne
Peter introduced the function “virDomainDeviceDefPostParseOne” to avoid the allocation of private data across the callbacks. This is absolutely fine.
What I’ve done is, I moved the domainPostParseDataAlloc/Free calls to virDomainDeviceDefParse instead of having an extra wrapper function (virDomainDeviceDefPostParse/One) for that. With this change I can reuse the QEMU caps for the virDomainDeviceDefValidate call in virDomainDeviceDefParse as well.
For the above it's not necessary to open-code what virDomainDeviceDefPostParseOne in a rather massive function. You can pass the opaque in if you want. Please don't remove the wrapper.
On Mon, Oct 01, 2018 at 12:41 PM +0200, Peter Krempa <pkrempa@redhat.com> wrote:
On Mon, Oct 01, 2018 at 12:27:41 +0200, Marc Hartmayer wrote:
On Sat, Sep 29, 2018 at 04:09 AM +0200, John Ferlan <jferlan@redhat.com> wrote:
On 9/20/18 1:44 PM, Marc Hartmayer wrote:
Move the domainPostParseDataAlloc/Free calls to virDomainDeviceDefParse. As an consequence virDomainDeviceDefPostParseOne is superfluous and can therefore be removed.
Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> --- src/conf/domain_conf.c | 37 +++++++++++-------------------------- 1 file changed, 11 insertions(+), 26 deletions(-)
I'm not against this per se; however, I should not that the code was specifically extracted in commit e168bc8a.
There are the following three functions:
virDomainDeviceDefParse virDomainDeviceDefPostParse virDomainDeviceDefPostParseOne
Peter introduced the function “virDomainDeviceDefPostParseOne” to avoid the allocation of private data across the callbacks. This is absolutely fine.
What I’ve done is, I moved the domainPostParseDataAlloc/Free calls to virDomainDeviceDefParse instead of having an extra wrapper function (virDomainDeviceDefPostParse/One) for that. With this change I can reuse the QEMU caps for the virDomainDeviceDefValidate call in virDomainDeviceDefParse as well.
For the above it's not necessary to open-code what virDomainDeviceDefPostParseOne in a rather massive function. You can pass the opaque in if you want.
I don’t get it. Where can I pass the opaque? At the end, we must use the same qemuCaps in virDomainDeviceDefValidate that we already used for virDomainDeviceDefPostParse(One).
Please don't remove the wrapper.
-- Kind regards / Beste Grüße Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzende des Aufsichtsrats: Martina Koederitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
On Mon, Oct 01, 2018 at 14:38:40 +0200, Marc Hartmayer wrote:
On Mon, Oct 01, 2018 at 12:41 PM +0200, Peter Krempa <pkrempa@redhat.com> wrote:
On Mon, Oct 01, 2018 at 12:27:41 +0200, Marc Hartmayer wrote:
On Sat, Sep 29, 2018 at 04:09 AM +0200, John Ferlan <jferlan@redhat.com> wrote:
On 9/20/18 1:44 PM, Marc Hartmayer wrote:
Move the domainPostParseDataAlloc/Free calls to virDomainDeviceDefParse. As an consequence virDomainDeviceDefPostParseOne is superfluous and can therefore be removed.
Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> --- src/conf/domain_conf.c | 37 +++++++++++-------------------------- 1 file changed, 11 insertions(+), 26 deletions(-)
I'm not against this per se; however, I should not that the code was specifically extracted in commit e168bc8a.
There are the following three functions:
virDomainDeviceDefParse virDomainDeviceDefPostParse virDomainDeviceDefPostParseOne
Peter introduced the function “virDomainDeviceDefPostParseOne” to avoid the allocation of private data across the callbacks. This is absolutely fine.
What I’ve done is, I moved the domainPostParseDataAlloc/Free calls to virDomainDeviceDefParse instead of having an extra wrapper function (virDomainDeviceDefPostParse/One) for that. With this change I can reuse the QEMU caps for the virDomainDeviceDefValidate call in virDomainDeviceDefParse as well.
For the above it's not necessary to open-code what virDomainDeviceDefPostParseOne in a rather massive function. You can pass the opaque in if you want.
I don’t get it. Where can I pass the opaque?
virDomainDeviceDefParse is a big spaghettified function and adding other code to it is unwanted. If you need to unify the allocation of private data, move the validation function call to the helper (with appropriate rename) rather than blatantly pasting the code back.
On Mon, Oct 01, 2018 at 02:50 PM +0200, Peter Krempa <pkrempa@redhat.com> wrote:
On Mon, Oct 01, 2018 at 14:38:40 +0200, Marc Hartmayer wrote:
On Mon, Oct 01, 2018 at 12:41 PM +0200, Peter Krempa <pkrempa@redhat.com> wrote:
On Mon, Oct 01, 2018 at 12:27:41 +0200, Marc Hartmayer wrote:
On Sat, Sep 29, 2018 at 04:09 AM +0200, John Ferlan <jferlan@redhat.com> wrote:
On 9/20/18 1:44 PM, Marc Hartmayer wrote:
Move the domainPostParseDataAlloc/Free calls to virDomainDeviceDefParse. As an consequence virDomainDeviceDefPostParseOne is superfluous and can therefore be removed.
Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> --- src/conf/domain_conf.c | 37 +++++++++++-------------------------- 1 file changed, 11 insertions(+), 26 deletions(-)
I'm not against this per se; however, I should not that the code was specifically extracted in commit e168bc8a.
There are the following three functions:
virDomainDeviceDefParse virDomainDeviceDefPostParse virDomainDeviceDefPostParseOne
Peter introduced the function “virDomainDeviceDefPostParseOne” to avoid the allocation of private data across the callbacks. This is absolutely fine.
What I’ve done is, I moved the domainPostParseDataAlloc/Free calls to virDomainDeviceDefParse instead of having an extra wrapper function (virDomainDeviceDefPostParse/One) for that. With this change I can reuse the QEMU caps for the virDomainDeviceDefValidate call in virDomainDeviceDefParse as well.
For the above it's not necessary to open-code what virDomainDeviceDefPostParseOne in a rather massive function. You can pass the opaque in if you want.
I don’t get it. Where can I pass the opaque?
virDomainDeviceDefParse is a big spaghettified function and adding other code to it is unwanted. If you need to unify the allocation of private data, move the validation function call to the helper (with appropriate rename) rather than blatantly pasting the code back.
Ahhh okay. Any ideas for the name? Thanks. Side note: What has always surprised me is that the "virDomainDeviceDefPostParse" function is called right out of the context of the function "virDomainDeviceDefParse". Shouldn’t it be called directly _after_ virDomainDeviceDefParse? -- Kind regards / Beste Grüße Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzende des Aufsichtsrats: Martina Koederitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
Add @parseOpaque argument to virDomainDefValidate and virDomainDefValidateCallback, but don't use it for now since it's not ensured that it's always a non-NULL value. Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> --- src/conf/domain_conf.c | 11 +++++++---- src/conf/domain_conf.h | 6 ++++-- src/qemu/qemu_domain.c | 3 ++- src/qemu/qemu_process.c | 2 +- src/vz/vz_driver.c | 3 ++- 5 files changed, 16 insertions(+), 9 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index e61f04ea2271..ae7f3ed95faf 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -6271,6 +6271,7 @@ virDomainDefValidateInternal(const virDomainDef *def) * @caps: driver capabilities object * @parseFlags: virDomainDefParseFlags * @xmlopt: XML parser option object + * @parseOpaque: opaque data and it might be NULL (for QEMU driver it's qemuCaps) * * This validation function is designed to take checks of globally invalid * configurations that the parser needs to accept so that VMs don't vanish upon @@ -6284,12 +6285,14 @@ int virDomainDefValidate(virDomainDefPtr def, virCapsPtr caps, unsigned int parseFlags, - virDomainXMLOptionPtr xmlopt) + virDomainXMLOptionPtr xmlopt, + void *parseOpaque) { struct virDomainDefPostParseDeviceIteratorData data = { .caps = caps, .xmlopt = xmlopt, .parseFlags = parseFlags, + .parseOpaque = parseOpaque, }; /* validate configuration only in certain places */ @@ -6298,7 +6301,7 @@ virDomainDefValidate(virDomainDefPtr def, /* call the domain config callback */ if (xmlopt->config.domainValidateCallback && - xmlopt->config.domainValidateCallback(def, caps, xmlopt->config.priv) < 0) + xmlopt->config.domainValidateCallback(def, caps, xmlopt->config.priv, data.parseOpaque) < 0) return -1; /* iterate the devices */ @@ -21063,7 +21066,7 @@ virDomainObjParseXML(xmlDocPtr xml, goto error; /* valdiate configuration */ - if (virDomainDefValidate(obj->def, caps, flags, xmlopt) < 0) + if (virDomainDefValidate(obj->def, caps, flags, xmlopt, parseOpaque) < 0) goto error; return obj; @@ -21154,7 +21157,7 @@ virDomainDefParseNode(xmlDocPtr xml, goto cleanup; /* valdiate configuration */ - if (virDomainDefValidate(def, caps, flags, xmlopt) < 0) + if (virDomainDefValidate(def, caps, flags, xmlopt, parseOpaque) < 0) goto cleanup; VIR_STEAL_PTR(ret, def); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index e30a4b2fe7b9..3642d5eb6cba 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2696,7 +2696,8 @@ typedef void (*virDomainDefPostParseDataFree)(void *parseOpaque); * config. */ typedef int (*virDomainDefValidateCallback)(const virDomainDef *def, virCapsPtr caps, - void *opaque); + void *opaque, + void *domainOpaque); /* Called once per device, for adjusting per-device settings while * leaving the overall domain otherwise unchanged. */ @@ -2812,7 +2813,8 @@ bool virDomainDeviceAliasIsUserAlias(const char *aliasStr); int virDomainDefValidate(virDomainDefPtr def, virCapsPtr caps, unsigned int parseFlags, - virDomainXMLOptionPtr xmlopt); + virDomainXMLOptionPtr xmlopt, + void *parseOpaque); static inline bool virDomainObjIsActive(virDomainObjPtr dom) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index eb80711597cb..52d2aa435a36 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3990,7 +3990,8 @@ qemuDomainDefValidateMemory(const virDomainDef *def) static int qemuDomainDefValidate(const virDomainDef *def, virCapsPtr caps ATTRIBUTE_UNUSED, - void *opaque) + void *opaque, + void *parseOpaque ATTRIBUTE_UNUSED) { virQEMUDriverPtr driver = opaque; virQEMUCapsPtr qemuCaps = NULL; diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 6c5a6472d8cd..fed2c0f545e3 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5161,7 +5161,7 @@ qemuProcessStartValidateXML(virQEMUDriverPtr driver, * VM that was running before (migration, snapshots, save). It's more * important to start such VM than keep the configuration clean */ if ((flags & VIR_QEMU_PROCESS_START_NEW) && - virDomainDefValidate(vm->def, caps, 0, driver->xmlopt) < 0) + virDomainDefValidate(vm->def, caps, 0, driver->xmlopt, qemuCaps) < 0) return -1; return 0; diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c index bedc6316db8d..16c44c2f2215 100644 --- a/src/vz/vz_driver.c +++ b/src/vz/vz_driver.c @@ -250,7 +250,8 @@ vzDomainDefPostParse(virDomainDefPtr def, static int vzDomainDefValidate(const virDomainDef *def, virCapsPtr caps ATTRIBUTE_UNUSED, - void *opaque) + void *opaque, + void *parserOpaque ATTRIBUTE_UNUSED) { if (vzCheckUnsupportedControllers(def, opaque) < 0) return -1; -- 2.17.0
On 9/20/18 1:44 PM, Marc Hartmayer wrote:
Add @parseOpaque argument to virDomainDefValidate and virDomainDefValidateCallback, but don't use it for now since it's not ensured that it's always a non-NULL value.
Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> --- src/conf/domain_conf.c | 11 +++++++---- src/conf/domain_conf.h | 6 ++++-- src/qemu/qemu_domain.c | 3 ++- src/qemu/qemu_process.c | 2 +- src/vz/vz_driver.c | 3 ++- 5 files changed, 16 insertions(+), 9 deletions(-)
I like this idea especially since the Validate paths are the ones where qemuCaps seem to be most useful. Couple of nits below John
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index e61f04ea2271..ae7f3ed95faf 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -6271,6 +6271,7 @@ virDomainDefValidateInternal(const virDomainDef *def) * @caps: driver capabilities object * @parseFlags: virDomainDefParseFlags * @xmlopt: XML parser option object + * @parseOpaque: opaque data and it might be NULL (for QEMU driver it's qemuCaps) * * This validation function is designed to take checks of globally invalid * configurations that the parser needs to accept so that VMs don't vanish upon @@ -6284,12 +6285,14 @@ int virDomainDefValidate(virDomainDefPtr def, virCapsPtr caps, unsigned int parseFlags, - virDomainXMLOptionPtr xmlopt) + virDomainXMLOptionPtr xmlopt, + void *parseOpaque) { struct virDomainDefPostParseDeviceIteratorData data = { .caps = caps, .xmlopt = xmlopt, .parseFlags = parseFlags, + .parseOpaque = parseOpaque, };
/* validate configuration only in certain places */ @@ -6298,7 +6301,7 @@ virDomainDefValidate(virDomainDefPtr def,
/* call the domain config callback */ if (xmlopt->config.domainValidateCallback && - xmlopt->config.domainValidateCallback(def, caps, xmlopt->config.priv) < 0) + xmlopt->config.domainValidateCallback(def, caps, xmlopt->config.priv, data.parseOpaque) < 0) return -1;
/* iterate the devices */ @@ -21063,7 +21066,7 @@ virDomainObjParseXML(xmlDocPtr xml, goto error;
/* valdiate configuration */
May as well fix the typo above *validate
- if (virDomainDefValidate(obj->def, caps, flags, xmlopt) < 0) + if (virDomainDefValidate(obj->def, caps, flags, xmlopt, parseOpaque) < 0) goto error;
return obj; @@ -21154,7 +21157,7 @@ virDomainDefParseNode(xmlDocPtr xml, goto cleanup;
/* valdiate configuration */
Consistency is the key ;-)
- if (virDomainDefValidate(def, caps, flags, xmlopt) < 0) + if (virDomainDefValidate(def, caps, flags, xmlopt, parseOpaque) < 0) goto cleanup;
VIR_STEAL_PTR(ret, def); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index e30a4b2fe7b9..3642d5eb6cba 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2696,7 +2696,8 @@ typedef void (*virDomainDefPostParseDataFree)(void *parseOpaque); * config. */ typedef int (*virDomainDefValidateCallback)(const virDomainDef *def, virCapsPtr caps, - void *opaque); + void *opaque, + void *domainOpaque);
/* Called once per device, for adjusting per-device settings while * leaving the overall domain otherwise unchanged. */ @@ -2812,7 +2813,8 @@ bool virDomainDeviceAliasIsUserAlias(const char *aliasStr); int virDomainDefValidate(virDomainDefPtr def, virCapsPtr caps, unsigned int parseFlags, - virDomainXMLOptionPtr xmlopt); + virDomainXMLOptionPtr xmlopt, + void *parseOpaque);
static inline bool virDomainObjIsActive(virDomainObjPtr dom) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index eb80711597cb..52d2aa435a36 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3990,7 +3990,8 @@ qemuDomainDefValidateMemory(const virDomainDef *def) static int qemuDomainDefValidate(const virDomainDef *def, virCapsPtr caps ATTRIBUTE_UNUSED, - void *opaque) + void *opaque, + void *parseOpaque ATTRIBUTE_UNUSED) { virQEMUDriverPtr driver = opaque; virQEMUCapsPtr qemuCaps = NULL; diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 6c5a6472d8cd..fed2c0f545e3 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5161,7 +5161,7 @@ qemuProcessStartValidateXML(virQEMUDriverPtr driver, * VM that was running before (migration, snapshots, save). It's more * important to start such VM than keep the configuration clean */ if ((flags & VIR_QEMU_PROCESS_START_NEW) && - virDomainDefValidate(vm->def, caps, 0, driver->xmlopt) < 0) + virDomainDefValidate(vm->def, caps, 0, driver->xmlopt, qemuCaps) < 0) return -1;
return 0; diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c index bedc6316db8d..16c44c2f2215 100644 --- a/src/vz/vz_driver.c +++ b/src/vz/vz_driver.c @@ -250,7 +250,8 @@ vzDomainDefPostParse(virDomainDefPtr def, static int vzDomainDefValidate(const virDomainDef *def, virCapsPtr caps ATTRIBUTE_UNUSED, - void *opaque) + void *opaque, + void *parserOpaque ATTRIBUTE_UNUSED)
nit: @parseOpaque
{ if (vzCheckUnsupportedControllers(def, opaque) < 0) return -1;
On Sat, Sep 29, 2018 at 05:34 PM +0200, John Ferlan <jferlan@redhat.com> wrote:
On 9/20/18 1:44 PM, Marc Hartmayer wrote:
Add @parseOpaque argument to virDomainDefValidate and virDomainDefValidateCallback, but don't use it for now since it's not ensured that it's always a non-NULL value.
Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> --- src/conf/domain_conf.c | 11 +++++++---- src/conf/domain_conf.h | 6 ++++-- src/qemu/qemu_domain.c | 3 ++- src/qemu/qemu_process.c | 2 +- src/vz/vz_driver.c | 3 ++- 5 files changed, 16 insertions(+), 9 deletions(-)
I like this idea especially since the Validate paths are the ones where qemuCaps seem to be most useful.
Couple of nits below
John
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index e61f04ea2271..ae7f3ed95faf 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -6271,6 +6271,7 @@ virDomainDefValidateInternal(const virDomainDef *def) * @caps: driver capabilities object * @parseFlags: virDomainDefParseFlags * @xmlopt: XML parser option object + * @parseOpaque: opaque data and it might be NULL (for QEMU driver it's qemuCaps) * * This validation function is designed to take checks of globally invalid * configurations that the parser needs to accept so that VMs don't vanish upon @@ -6284,12 +6285,14 @@ int virDomainDefValidate(virDomainDefPtr def, virCapsPtr caps, unsigned int parseFlags, - virDomainXMLOptionPtr xmlopt) + virDomainXMLOptionPtr xmlopt, + void *parseOpaque) { struct virDomainDefPostParseDeviceIteratorData data = { .caps = caps, .xmlopt = xmlopt, .parseFlags = parseFlags, + .parseOpaque = parseOpaque, };
/* validate configuration only in certain places */ @@ -6298,7 +6301,7 @@ virDomainDefValidate(virDomainDefPtr def,
/* call the domain config callback */ if (xmlopt->config.domainValidateCallback && - xmlopt->config.domainValidateCallback(def, caps, xmlopt->config.priv) < 0) + xmlopt->config.domainValidateCallback(def, caps, xmlopt->config.priv, data.parseOpaque) < 0) return -1;
/* iterate the devices */ @@ -21063,7 +21066,7 @@ virDomainObjParseXML(xmlDocPtr xml, goto error;
/* valdiate configuration */
May as well fix the typo above *validate
Will change.
- if (virDomainDefValidate(obj->def, caps, flags, xmlopt) < 0) + if (virDomainDefValidate(obj->def, caps, flags, xmlopt, parseOpaque) < 0) goto error;
return obj; @@ -21154,7 +21157,7 @@ virDomainDefParseNode(xmlDocPtr xml, goto cleanup;
/* valdiate configuration */
Consistency is the key ;-)
Yep :D
- if (virDomainDefValidate(def, caps, flags, xmlopt) < 0) + if (virDomainDefValidate(def, caps, flags, xmlopt, parseOpaque) < 0) goto cleanup;
[…snip…]
static int vzDomainDefValidate(const virDomainDef *def, virCapsPtr caps ATTRIBUTE_UNUSED, - void *opaque) + void *opaque, + void *parserOpaque ATTRIBUTE_UNUSED)
nit: @parseOpaque
Okay.
{ if (vzCheckUnsupportedControllers(def, opaque) < 0) return -1;
Thanks for the review! -- Kind regards / Beste Grüße Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzende des Aufsichtsrats: Martina Koederitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
For the usage of the parameter @parseOpqaue within virDomainDefValidate it must be ensured that the parameter @parseOpaque is not NULL. But since there are code paths where virDomainDefValidate is called with @parseOpaque == NULL (e.g. the function call of virDomainDefParseNode with @parseOpqaue == NULL leads to this). To prevent this, domainPostParseDataAlloc is called within virDomainDefValidate to ensure that @parseOpaque is always set. The usage of domainPostParseDataAlloc within virDomainDefValidate is allowed since virDomainDefValidate is only called after virDomainDefPostParse. Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> --- src/conf/domain_conf.c | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index ae7f3ed95faf..4f1c569b5733 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -6288,6 +6288,9 @@ virDomainDefValidate(virDomainDefPtr def, virDomainXMLOptionPtr xmlopt, void *parseOpaque) { + int ret = -1; + bool localParseOpaque = false; + struct virDomainDefPostParseDeviceIteratorData data = { .caps = caps, .xmlopt = xmlopt, @@ -6299,6 +6302,17 @@ virDomainDefValidate(virDomainDefPtr def, if (parseFlags & VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE) return 0; + if (!data.parseOpaque && + xmlopt->config.domainPostParseDataAlloc) { + ret = xmlopt->config.domainPostParseDataAlloc(def, caps, parseFlags, + xmlopt->config.priv, + &data.parseOpaque); + + if (virDomainDefPostParseCheckFailure(def, parseFlags, ret) < 0) + goto cleanup; + localParseOpaque = true; + } + /* call the domain config callback */ if (xmlopt->config.domainValidateCallback && xmlopt->config.domainValidateCallback(def, caps, xmlopt->config.priv, data.parseOpaque) < 0) @@ -6313,6 +6327,13 @@ virDomainDefValidate(virDomainDefPtr def, if (virDomainDefValidateInternal(def) < 0) return -1; + cleanup: + if (localParseOpaque && xmlopt->config.domainPostParseDataFree) + xmlopt->config.domainPostParseDataFree(data.parseOpaque); + + if (ret == 1) + return -1; + return 0; } -- 2.17.0
On 9/20/18 1:44 PM, Marc Hartmayer wrote:
For the usage of the parameter @parseOpqaue within virDomainDefValidate it must be ensured that the parameter @parseOpaque is not NULL. But since there are code paths where virDomainDefValidate is called with @parseOpaque == NULL (e.g. the function call of virDomainDefParseNode with @parseOpqaue == NULL leads to this).
To prevent this, domainPostParseDataAlloc is called within virDomainDefValidate to ensure that @parseOpaque is always set. The usage of domainPostParseDataAlloc within virDomainDefValidate is allowed since virDomainDefValidate is only called after virDomainDefPostParse.
Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> --- src/conf/domain_conf.c | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+)
Thus is similar to commit e168bc8a did for virDomainDefPostParse...
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index ae7f3ed95faf..4f1c569b5733 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -6288,6 +6288,9 @@ virDomainDefValidate(virDomainDefPtr def, virDomainXMLOptionPtr xmlopt, void *parseOpaque) { + int ret = -1; + bool localParseOpaque = false; + struct virDomainDefPostParseDeviceIteratorData data = { .caps = caps, .xmlopt = xmlopt, @@ -6299,6 +6302,17 @@ virDomainDefValidate(virDomainDefPtr def, if (parseFlags & VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE) return 0;
+ if (!data.parseOpaque && + xmlopt->config.domainPostParseDataAlloc) { + ret = xmlopt->config.domainPostParseDataAlloc(def, caps, parseFlags, + xmlopt->config.priv, + &data.parseOpaque);
So ret = 1 if virQEMUCapsCacheLookup failed in qemuDomainPostParseDataAlloc; otherwise, ret = 0; Just looks strange below with the ret == 1 check.
+ + if (virDomainDefPostParseCheckFailure(def, parseFlags, ret) < 0)
This though has/uses specific @parseFlag options and sets def->postParseFailed when it may not be true if for some reason of course the post parse processing never got the parseOpaque value. Wouldn't it perhaps be problematic in the patch3 scenario where you'd be passing the priv->qemuCaps when @postParseFailed? John
+ goto cleanup; + localParseOpaque = true; + } + /* call the domain config callback */ if (xmlopt->config.domainValidateCallback && xmlopt->config.domainValidateCallback(def, caps, xmlopt->config.priv, data.parseOpaque) < 0) @@ -6313,6 +6327,13 @@ virDomainDefValidate(virDomainDefPtr def, if (virDomainDefValidateInternal(def) < 0) return -1;
+ cleanup: + if (localParseOpaque && xmlopt->config.domainPostParseDataFree) + xmlopt->config.domainPostParseDataFree(data.parseOpaque); + + if (ret == 1) + return -1; + return 0; }
On Sat, Sep 29, 2018 at 05:35 PM +0200, John Ferlan <jferlan@redhat.com> wrote:
On 9/20/18 1:44 PM, Marc Hartmayer wrote:
For the usage of the parameter @parseOpqaue within virDomainDefValidate it must be ensured that the parameter @parseOpaque is not NULL. But since there are code paths where virDomainDefValidate is called with @parseOpaque == NULL (e.g. the function call of virDomainDefParseNode with @parseOpqaue == NULL leads to this).
To prevent this, domainPostParseDataAlloc is called within virDomainDefValidate to ensure that @parseOpaque is always set. The usage of domainPostParseDataAlloc within virDomainDefValidate is allowed since virDomainDefValidate is only called after virDomainDefPostParse.
Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> --- src/conf/domain_conf.c | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+)
Thus is similar to commit e168bc8a did for virDomainDefPostParse...
Yes. That was actually my “inspiration”.
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index ae7f3ed95faf..4f1c569b5733 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -6288,6 +6288,9 @@ virDomainDefValidate(virDomainDefPtr def, virDomainXMLOptionPtr xmlopt, void *parseOpaque) { + int ret = -1; + bool localParseOpaque = false; + struct virDomainDefPostParseDeviceIteratorData data = { .caps = caps, .xmlopt = xmlopt, @@ -6299,6 +6302,17 @@ virDomainDefValidate(virDomainDefPtr def, if (parseFlags & VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE) return 0;
+ if (!data.parseOpaque && + xmlopt->config.domainPostParseDataAlloc) { + ret = xmlopt->config.domainPostParseDataAlloc(def, caps, parseFlags, + xmlopt->config.priv, + &data.parseOpaque);
So ret = 1 if virQEMUCapsCacheLookup failed in qemuDomainPostParseDataAlloc; otherwise, ret = 0; Just looks strange below with the ret == 1 check.
Yes, do you have something better in mind? Maybe we can add an error label. While fixing it we can also adapt virDomainDefPostParse (were it’s, in my opinion, even more confusing since @ret is used multiple times).
+ + if (virDomainDefPostParseCheckFailure(def, parseFlags, ret) < 0)
This though has/uses specific @parseFlag options and sets def->postParseFailed when it may not be true if for some reason of course the post parse processing never got the parseOpaque value. Wouldn't it perhaps be problematic in the patch3 scenario where you'd be passing the priv->qemuCaps when @postParseFailed?
See my answer for patch 3.
John
+ goto cleanup; + localParseOpaque = true; + } + /* call the domain config callback */ if (xmlopt->config.domainValidateCallback && xmlopt->config.domainValidateCallback(def, caps, xmlopt->config.priv, data.parseOpaque) < 0) @@ -6313,6 +6327,13 @@ virDomainDefValidate(virDomainDefPtr def, if (virDomainDefValidateInternal(def) < 0) return -1;
+ cleanup: + if (localParseOpaque && xmlopt->config.domainPostParseDataFree) + xmlopt->config.domainPostParseDataFree(data.parseOpaque); + + if (ret == 1) + return -1; + return 0; }
-- Kind regards / Beste Grüße Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzende des Aufsichtsrats: Martina Koederitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
Since the previous commit made sure that @parseOpaque is always set to a non-NULL value we can use it now. This avoids the usage of a possible different QEMU capability than before, since virQEMUCapsCacheLookup could return a different QEMU capability than used before (e.g. if during the job the used QEMU binary has been replaced and thus the QEMU capability is invalidated by the file cache) Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> --- src/qemu/qemu_domain.c | 44 ++++++++++++++++-------------------------- 1 file changed, 17 insertions(+), 27 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 52d2aa435a36..aacb54a72f72 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3990,21 +3990,15 @@ qemuDomainDefValidateMemory(const virDomainDef *def) static int qemuDomainDefValidate(const virDomainDef *def, virCapsPtr caps ATTRIBUTE_UNUSED, - void *opaque, - void *parseOpaque ATTRIBUTE_UNUSED) + void *opaque ATTRIBUTE_UNUSED, + void *parseOpaque) { - virQEMUDriverPtr driver = opaque; - virQEMUCapsPtr qemuCaps = NULL; - int ret = -1; - - if (!(qemuCaps = virQEMUCapsCacheLookup(driver->qemuCapsCache, - def->emulator))) - goto cleanup; + virQEMUCapsPtr qemuCaps = parseOpaque; if (def->mem.min_guarantee) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("Parameter 'min_guarantee' not supported by QEMU.")); - goto cleanup; + return -1; } /* On x86, UEFI requires ACPI */ @@ -4014,7 +4008,7 @@ qemuDomainDefValidate(const virDomainDef *def, def->features[VIR_DOMAIN_FEATURE_ACPI] != VIR_TRISTATE_SWITCH_ON) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("UEFI requires ACPI on this architecture")); - goto cleanup; + return -1; } /* On aarch64, ACPI requires UEFI */ @@ -4024,7 +4018,7 @@ qemuDomainDefValidate(const virDomainDef *def, def->os.loader->type != VIR_DOMAIN_LOADER_TYPE_PFLASH)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("ACPI requires UEFI on this architecture")); - goto cleanup; + return -1; } if (def->os.loader && @@ -4035,7 +4029,7 @@ qemuDomainDefValidate(const virDomainDef *def, if (!qemuDomainIsQ35(def)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("Secure boot is supported with q35 machine types only")); - goto cleanup; + return -1; } /* Now, technically it is possible to have secure boot on @@ -4044,13 +4038,13 @@ qemuDomainDefValidate(const virDomainDef *def, if (def->os.arch != VIR_ARCH_X86_64) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("Secure boot is supported for x86_64 architecture only")); - goto cleanup; + return -1; } if (def->features[VIR_DOMAIN_FEATURE_SMM] != VIR_TRISTATE_SWITCH_ON) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("Secure boot requires SMM feature enabled")); - goto cleanup; + return -1; } } @@ -4068,7 +4062,7 @@ qemuDomainDefValidate(const virDomainDef *def, topologycpus != virDomainDefGetVcpusMax(def)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("CPU topology doesn't match maximum vcpu count")); - goto cleanup; + return -1; } /* vCPU hotplug granularity must be respected */ @@ -4078,7 +4072,7 @@ qemuDomainDefValidate(const virDomainDef *def, _("vCPUs count must be a multiple of the vCPU " "hotplug granularity (%u)"), granularity); - goto cleanup; + return -1; } } @@ -4089,14 +4083,14 @@ qemuDomainDefValidate(const virDomainDef *def, _("more than %d vCPUs are only supported on " "q35-based machine types"), QEMU_MAX_VCPUS_WITHOUT_EIM); - goto cleanup; + return -1; } if (!def->iommu || def->iommu->eim != VIR_TRISTATE_SWITCH_ON) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("more than %d vCPUs require extended interrupt " "mode enabled on the iommu device"), QEMU_MAX_VCPUS_WITHOUT_EIM); - goto cleanup; + return -1; } } @@ -4104,20 +4098,16 @@ qemuDomainDefValidate(const virDomainDef *def, def->virtType != VIR_DOMAIN_VIRT_KVM) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("cachetune is only supported for KVM domains")); - goto cleanup; + return -1; } if (qemuDomainDefValidateFeatures(def, qemuCaps) < 0) - goto cleanup; + return -1; if (qemuDomainDefValidateMemory(def) < 0) - goto cleanup; + return -1; - ret = 0; - - cleanup: - virObjectUnref(qemuCaps); - return ret; + return 0; } -- 2.17.0
Add parseOpaque parameter to virDomainDeviceDefValidate(Callback). But don't use it for now. Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> --- src/conf/domain_conf.c | 10 ++++++---- src/conf/domain_conf.h | 3 ++- src/qemu/qemu_domain.c | 3 ++- src/vz/vz_driver.c | 3 ++- 4 files changed, 12 insertions(+), 7 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 4f1c569b5733..5b2277defbdf 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5961,18 +5961,20 @@ virDomainDeviceValidateAliasForHotplug(virDomainObjPtr vm, } +/* parseOpaque: must not be NULL */ static int virDomainDeviceDefValidate(const virDomainDeviceDef *dev, const virDomainDef *def, unsigned int parseFlags, - virDomainXMLOptionPtr xmlopt) + virDomainXMLOptionPtr xmlopt, + void *parseOpaque) { /* validate configuration only in certain places */ if (parseFlags & VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE) return 0; if (xmlopt->config.deviceValidateCallback && - xmlopt->config.deviceValidateCallback(dev, def, xmlopt->config.priv)) + xmlopt->config.deviceValidateCallback(dev, def, xmlopt->config.priv, parseOpaque)) return -1; if (virDomainDeviceDefValidateInternal(dev, def) < 0) @@ -5990,7 +5992,7 @@ virDomainDefValidateDeviceIterator(virDomainDefPtr def, { struct virDomainDefPostParseDeviceIteratorData *data = opaque; return virDomainDeviceDefValidate(dev, def, - data->parseFlags, data->xmlopt); + data->parseFlags, data->xmlopt, data->parseOpaque); } @@ -16234,7 +16236,7 @@ virDomainDeviceDefParse(const char *xmlStr, goto error; /* validate the configuration */ - if (virDomainDeviceDefValidate(dev, def, flags, xmlopt) < 0) + if (virDomainDeviceDefValidate(dev, def, flags, xmlopt, parseOpaque) < 0) goto error; cleanup: diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 3642d5eb6cba..1dcfabf97e53 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2703,7 +2703,8 @@ typedef int (*virDomainDefValidateCallback)(const virDomainDef *def, * leaving the overall domain otherwise unchanged. */ typedef int (*virDomainDeviceDefValidateCallback)(const virDomainDeviceDef *dev, const virDomainDef *def, - void *opaque); + void *opaque, + void *parseOpaque); typedef struct _virDomainDefParserConfig virDomainDefParserConfig; typedef virDomainDefParserConfig *virDomainDefParserConfigPtr; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index aacb54a72f72..2096a2a39ac5 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -5790,7 +5790,8 @@ qemuDomainDeviceDefValidateInput(const virDomainInputDef *input, static int qemuDomainDeviceDefValidate(const virDomainDeviceDef *dev, const virDomainDef *def, - void *opaque) + void *opaque, + void *parseOpaque ATTRIBUTE_UNUSED) { int ret = 0; virQEMUDriverPtr driver = opaque; diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c index 16c44c2f2215..502e34aba43c 100644 --- a/src/vz/vz_driver.c +++ b/src/vz/vz_driver.c @@ -281,7 +281,8 @@ vzDomainDeviceDefPostParse(virDomainDeviceDefPtr dev, static int vzDomainDeviceDefValidate(const virDomainDeviceDef *dev, const virDomainDef *def, - void *opaque ATTRIBUTE_UNUSED) + void *opaque ATTRIBUTE_UNUSED, + void *parseOpaque ATTRIBUTE_UNUSED) { if (dev->type == VIR_DOMAIN_DEVICE_DISK) return vzCheckUnsupportedDisk(def, dev->data.disk, opaque); -- 2.17.0
Since each code path ensures that @parseOpaque cannot be NULL, we can use the argument instead of using virQEMUCapsCacheLookup where the QEMU capabilities may have been changed during the libvirt job. Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> --- src/qemu/qemu_domain.c | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 2096a2a39ac5..05641b3dceb8 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -5790,16 +5790,11 @@ qemuDomainDeviceDefValidateInput(const virDomainInputDef *input, static int qemuDomainDeviceDefValidate(const virDomainDeviceDef *dev, const virDomainDef *def, - void *opaque, - void *parseOpaque ATTRIBUTE_UNUSED) + void *opaque ATTRIBUTE_UNUSED, + void *parseOpaque) { int ret = 0; - virQEMUDriverPtr driver = opaque; - virQEMUCapsPtr qemuCaps = NULL; - - if (!(qemuCaps = virQEMUCapsCacheLookup(driver->qemuCapsCache, - def->emulator))) - return -1; + virQEMUCapsPtr qemuCaps = parseOpaque; switch ((virDomainDeviceType)dev->type) { case VIR_DOMAIN_DEVICE_NET: @@ -5876,7 +5871,6 @@ qemuDomainDeviceDefValidate(const virDomainDeviceDef *dev, break; } - virObjectUnref(qemuCaps); return ret; } -- 2.17.0
On Thu, Sep 20, 2018 at 07:44:46PM +0200, Marc Hartmayer wrote:
For a domain definition there are numerous calls of virQEMUCapsCacheLookup (the same applies to the domain start). This slows down the process since virQEMUCapsCacheLookup validates that the QEMU capabilitites are still valid (among other things, a fork is done for this if the user for the QEMU processes is 'qemu'). Therefore let's reduce the number of virQEMUCapsCacheLookup calls whenever possible and reasonable.
In addition to the speed up, there is the risk that virQEMUCapsCacheLookup returns different QEMU capabilities during a task if, for example, the QEMU binary has changed during the task.
The correct way would be:
- get the QEMU capabilities only once per task via virQEMUCapsCacheLookup - do the task with these QEMU capabilities
or
- abort the task after a cache invalidation
Note: With this patch series the behavior is still not (completely) fixed, but the performance has been significantly improved. In a quick test this gave a speed up of factor 4 for a simple define/undefine loop.
In general, the more devices a domain has, the more drastic the overhead becomes (because a cache validation is performed for each device).
IIUC from your KVM Forum presentation, the overhead of the cache lookup is almost entirely coming from the fork() call triggered by the single call kvmUsable = virFileAccessibleAs("/dev/kvm", R_OK | W_OK, priv->runUid, priv->runGid) == 0; Rather than the major refactor of the way the parse callbacks work, we could instead just optimize this call to avoid the repeated forks. Even if we reduced the number of calls to the cache, it is still somewhat overkill to be checking /dev/kvm via fork() every time. eg even if you reduced it to just a single cache lookup, if you run virDomainDefine for 100 domains in parallel it is still going to do 100 forks. We could optimize this by jcalling virFileAccessibleAs once and storing the result in a global. Then just do a plain stat() call in process to check the st_ctime field for changes. We only need re-run the heavy virFileAccessibleAs check if st_ctime has changed (indicating a owner/group/acl change). 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 :|
Daniel P. Berrangé <berrange@redhat.com> [2018-10-24, 10:43PM +0100]:
We could optimize this by jcalling virFileAccessibleAs once and storing the result in a global. Then just do a plain stat() call in process to check the st_ctime field for changes. We only need re-run the heavy virFileAccessibleAs check if st_ctime has changed (indicating a owner/group/acl change).
But can't access permission change outside of changing the actual device file (e.g. cgroups or other OS capabilities)? Isn't that the whole purpose of the virFileAccessibleAs gymnastics? -- IBM Systems Linux on Z & Virtualization Development -------------------------------------------------- IBM Deutschland Research & Development GmbH Schönaicher Str. 220, 71032 Böblingen Phone: +49 7031 16 1819 -------------------------------------------------- Vorsitzende des Aufsichtsrats: Martina Koederitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
On Thu, Oct 25, 2018 at 01:47:26PM +0200, Bjoern Walk wrote:
Daniel P. Berrangé <berrange@redhat.com> [2018-10-24, 10:43PM +0100]:
We could optimize this by jcalling virFileAccessibleAs once and storing the result in a global. Then just do a plain stat() call in process to check the st_ctime field for changes. We only need re-run the heavy virFileAccessibleAs check if st_ctime has changed (indicating a owner/group/acl change).
But can't access permission change outside of changing the actual device file (e.g. cgroups or other OS capabilities)? Isn't that the whole purpose of the virFileAccessibleAs gymnastics?
Yes, cgroups could restrict access to /dev/kvm, but virFileAccessibleAs does not use cgroups, it only cares about using the correct user + group membership. 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 :|
Daniel P. Berrangé <berrange@redhat.com> [2018-10-25, 06:32PM +0100]:
On Thu, Oct 25, 2018 at 01:47:26PM +0200, Bjoern Walk wrote:
Daniel P. Berrangé <berrange@redhat.com> [2018-10-24, 10:43PM +0100]:
We could optimize this by jcalling virFileAccessibleAs once and storing the result in a global. Then just do a plain stat() call in process to check the st_ctime field for changes. We only need re-run the heavy virFileAccessibleAs check if st_ctime has changed (indicating a owner/group/acl change).
But can't access permission change outside of changing the actual device file (e.g. cgroups or other OS capabilities)? Isn't that the whole purpose of the virFileAccessibleAs gymnastics?
Yes, cgroups could restrict access to /dev/kvm, but virFileAccessibleAs does not use cgroups, it only cares about using the correct user + group membership.
Sorry, but then I don't understand the purpose of this function at all. Why would you EVER check permissions like that? A simple stat(2) call should give you the exact same information, no? -- IBM Systems Linux on Z & Virtualization Development -------------------------------------------------- IBM Deutschland Research & Development GmbH Schönaicher Str. 220, 71032 Böblingen Phone: +49 7031 16 1819 -------------------------------------------------- Vorsitzende des Aufsichtsrats: Martina Koederitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
On Fri, Oct 26, 2018 at 06:57:54AM +0200, Bjoern Walk wrote:
Daniel P. Berrangé <berrange@redhat.com> [2018-10-25, 06:32PM +0100]:
On Thu, Oct 25, 2018 at 01:47:26PM +0200, Bjoern Walk wrote:
Daniel P. Berrangé <berrange@redhat.com> [2018-10-24, 10:43PM +0100]:
We could optimize this by jcalling virFileAccessibleAs once and storing the result in a global. Then just do a plain stat() call in process to check the st_ctime field for changes. We only need re-run the heavy virFileAccessibleAs check if st_ctime has changed (indicating a owner/group/acl change).
But can't access permission change outside of changing the actual device file (e.g. cgroups or other OS capabilities)? Isn't that the whole purpose of the virFileAccessibleAs gymnastics?
Yes, cgroups could restrict access to /dev/kvm, but virFileAccessibleAs does not use cgroups, it only cares about using the correct user + group membership.
Sorry, but then I don't understand the purpose of this function at all. Why would you EVER check permissions like that? A simple stat(2) call should give you the exact same information, no?
The capabilities probing runs as the 'qemu' user and 'qemu' group. If libvirtd stat()s the /dev/kvm device, it will merely find out if the *root* user can access it. We need to chcek if the qemu/qemu user/group can access it, and if you try todo via a stat() then you have to reinvent the kernel logic for checking supplementary group memberships and file ACL membership. The only sensible way is for the process checking to actually be running as the qemu/qemu user and group, hence we fork and change to this user/group pair. 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 Wed, Oct 24, 2018 at 11:43 PM +0200, "Daniel P. Berrangé" <berrange@redhat.com> wrote:
On Thu, Sep 20, 2018 at 07:44:46PM +0200, Marc Hartmayer wrote:
For a domain definition there are numerous calls of virQEMUCapsCacheLookup (the same applies to the domain start). This slows down the process since virQEMUCapsCacheLookup validates that the QEMU capabilitites are still valid (among other things, a fork is done for this if the user for the QEMU processes is 'qemu'). Therefore let's reduce the number of virQEMUCapsCacheLookup calls whenever possible and reasonable.
In addition to the speed up, there is the risk that virQEMUCapsCacheLookup returns different QEMU capabilities during a task if, for example, the QEMU binary has changed during the task.
The correct way would be:
- get the QEMU capabilities only once per task via virQEMUCapsCacheLookup - do the task with these QEMU capabilities
or
- abort the task after a cache invalidation
Note: With this patch series the behavior is still not (completely) fixed, but the performance has been significantly improved. In a quick test this gave a speed up of factor 4 for a simple define/undefine loop.
In general, the more devices a domain has, the more drastic the overhead becomes (because a cache validation is performed for each device).
IIUC from your KVM Forum presentation, the overhead of the cache lookup is almost entirely coming from the fork() call triggered by the single call
kvmUsable = virFileAccessibleAs("/dev/kvm", R_OK | W_OK, priv->runUid, priv->runGid) == 0;
Rather than the major refactor of the way the parse callbacks work, we could instead just optimize this call to avoid the repeated forks.
Even if we reduced the number of calls to the cache, it is still somewhat overkill to be checking /dev/kvm via fork() every time. eg even if you reduced it to just a single cache lookup, if you run virDomainDefine for 100 domains in parallel it is still going to do 100 forks.
We could optimize this by jcalling virFileAccessibleAs once and storing the result in a global. Then just do a plain stat() call in process to check the st_ctime field for changes. We only need re-run the heavy virFileAccessibleAs check if st_ctime has changed (indicating a owner/group/acl change).
Okay, if that’s fine I’ll add this as an additional patch to this series. Because the various virQEMUCapsCacheLookup calls still seem to be wrong to me. Marc
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 :|
-- Kind regards / Beste Grüße Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzende des Aufsichtsrats: Martina Koederitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
participants (5)
-
Bjoern Walk -
Daniel P. Berrangé -
John Ferlan -
Marc Hartmayer -
Peter Krempa