12:44 p.m.
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
12:44 p.m.
New subject: [libvirt] [PATCH 01/11] qemu: Use VIR_STEAL_PTR macro
Signed-off-by: Marc Hartmayer <mhartmay(a)linux.ibm.com>
Reviewed-by: Bjoern Walk <bwalk(a)linux.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
Reviewed-by: Stefan Zimmermann <stzi(a)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
3:29 p.m.
New subject: [libvirt] [PATCH 01/11] qemu: Use VIR_STEAL_PTR macro
On 9/20/18 1:44 PM, Marc Hartmayer wrote:
Signed-off-by: Marc Hartmayer <mhartmay(a)linux.ibm.com>
Reviewed-by: Bjoern Walk <bwalk(a)linux.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
Reviewed-by: Stefan Zimmermann <stzi(a)linux.ibm.com>
---
src/qemu/qemu_domain.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
Trivial, but I'll wait until freeze of off before pushing...
John
12:44 p.m.
New subject: [libvirt] [PATCH 02/11] qemu: Introduce qemuDomainUpdateQEMUCaps()
This function updates the used QEMU capabilities of @vm by querying
the QEMU capabilities cache.
Signed-off-by: Marc Hartmayer <mhartmay(a)linux.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)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
3:32 p.m.
New subject: [libvirt] [PATCH 02/11] qemu: Introduce qemuDomainUpdateQEMUCaps()
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(a)linux.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)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(a)redhat.com>
(less trivial, but will wait for freeze to be over before pushing)
John
12:44 p.m.
New subject: [libvirt] [PATCH 03/11] qemu: Pass QEMUCaps to virDomainDefPostParse
...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(a)linux.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)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
9:02 p.m.
New subject: [libvirt] [PATCH 03/11] qemu: Pass QEMUCaps to virDomainDefPostParse
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(a)linux.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)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;
}
2:33 a.m.
New subject: [libvirt] [PATCH 03/11] qemu: Pass QEMUCaps to virDomainDefPostParse
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(a)linux.ibm.com>
> Reviewed-by: Boris Fiuczynski <fiuczy(a)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.
3:31 a.m.
New subject: [libvirt] [PATCH 03/11] qemu: Pass QEMUCaps to virDomainDefPostParse
On Mon, Oct 01, 2018 at 09:33 AM +0200, Peter Krempa <pkrempa(a)redhat.com> wrote:
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 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(a)linux.ibm.com > > Reviewed-by: Boris
Fiuczynski <fiuczy(a)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 -
5:36 a.m.
New subject: [libvirt] [PATCH 03/11] qemu: Pass QEMUCaps to virDomainDefPostParse
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(a)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(a)linux.ibm.com>
>> > Reviewed-by: Boris Fiuczynski <fiuczy(a)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.
7:58 a.m.
New subject: [libvirt] [PATCH 03/11] qemu: Pass QEMUCaps to virDomainDefPostParse
On Mon, Oct 01, 2018 at 12:36 PM +0200, Peter Krempa <pkrempa(a)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
8:04 a.m.
New subject: [libvirt] [PATCH 03/11] qemu: Pass QEMUCaps to virDomainDefPostParse
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(a)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.
12:44 p.m.
New subject: [libvirt] [PATCH 04/11] conf: Use getParseOpaque() in virDomainObjSetDefTransient
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(a)linux.ibm.com>
Reviewed-by: Stefan Zimmermann <stzi(a)linux.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)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
9:06 p.m.
New subject: [libvirt] [PATCH 04/11] conf: Use getParseOpaque() in virDomainObjSetDefTransient
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(a)linux.ibm.com>
Reviewed-by: Stefan Zimmermann <stzi(a)linux.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)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;
3:53 a.m.
New subject: [libvirt] [PATCH 04/11] conf: Use getParseOpaque() in virDomainObjSetDefTransient
On Sat, Sep 29, 2018 at 04:06 AM +0200, John Ferlan <jferlan(a)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(a)linux.ibm.com>
> Reviewed-by: Stefan Zimmermann <stzi(a)linux.ibm.com>
> Reviewed-by: Boris Fiuczynski <fiuczy(a)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
7:47 a.m.
New subject: [libvirt] [PATCH 04/11] conf: Use getParseOpaque() in virDomainObjSetDefTransient
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(a)linux.ibm.com>
Reviewed-by: Stefan Zimmermann <stzi(a)linux.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)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.
12:44 p.m.
New subject: [libvirt] [PATCH 05/11] conf: Add function description for virDomainDefPostParse
Signed-off-by: Marc Hartmayer <mhartmay(a)linux.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)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
9:08 p.m.
New subject: [libvirt] [PATCH 05/11] conf: Add function description for virDomainDefPostParse
On 9/20/18 1:44 PM, Marc Hartmayer wrote:
Signed-off-by: Marc Hartmayer <mhartmay(a)linux.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)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,
3:59 a.m.
New subject: [libvirt] [PATCH 05/11] conf: Add function description for virDomainDefPostParse
On Sat, Sep 29, 2018 at 04:08 AM +0200, John Ferlan <jferlan(a)redhat.com> wrote:
On 9/20/18 1:44 PM, Marc Hartmayer wrote:
> Signed-off-by: Marc Hartmayer <mhartmay(a)linux.ibm.com>
> Reviewed-by: Boris Fiuczynski <fiuczy(a)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
12:44 p.m.
New subject: [libvirt] [PATCH 06/11] conf: Get rid of virDomainDeviceDefPostParseOne
Move the domainPostParseDataAlloc/Free calls to
virDomainDeviceDefParse. As an consequence
virDomainDeviceDefPostParseOne is superfluous and can therefore be
removed.
Signed-off-by: Marc Hartmayer <mhartmay(a)linux.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)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
9:09 p.m.
New subject: [libvirt] [PATCH 06/11] conf: Get rid of virDomainDeviceDefPostParseOne
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(a)linux.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)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;
5:27 a.m.
New subject: [libvirt] [PATCH 06/11] conf: Get rid of virDomainDeviceDefPostParseOne
On Sat, Sep 29, 2018 at 04:09 AM +0200, John Ferlan <jferlan(a)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(a)linux.ibm.com>
> Reviewed-by: Boris Fiuczynski <fiuczy(a)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
5:41 a.m.
New subject: [libvirt] [PATCH 06/11] conf: Get rid of virDomainDeviceDefPostParseOne
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(a)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(a)linux.ibm.com>
>> Reviewed-by: Boris Fiuczynski <fiuczy(a)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.
7:38 a.m.
New subject: [libvirt] [PATCH 06/11] conf: Get rid of virDomainDeviceDefPostParseOne
On Mon, Oct 01, 2018 at 12:41 PM +0200, Peter Krempa <pkrempa(a)redhat.com> wrote:
--
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(a)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(a)linux.ibm.com >
>> Reviewed-by: Boris Fiuczynski <fiuczy(a)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.
7:50 a.m.
New subject: [libvirt] [PATCH 06/11] conf: Get rid of virDomainDeviceDefPostParseOne
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(a)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(a)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(a)linux.ibm.com>
>> >> Reviewed-by: Boris Fiuczynski <fiuczy(a)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.
8:08 a.m.
New subject: [libvirt] [PATCH 06/11] conf: Get rid of virDomainDeviceDefPostParseOne
On Mon, Oct 01, 2018 at 02:50 PM +0200, Peter Krempa <pkrempa(a)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(a)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(a)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(a)linux.ibm.com>
> >> >> Reviewed-by: Boris Fiuczynski <fiuczy(a)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
12:44 p.m.
New subject: [libvirt] [PATCH 07/11] conf: Extend virDomainDefValidate(Callback) for parseOpaque
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(a)linux.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)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
10:34 a.m.
New subject: [libvirt] [PATCH 07/11] conf: Extend virDomainDefValidate(Callback) for parseOpaque
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(a)linux.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)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;
5:30 a.m.
New subject: [libvirt] [PATCH 07/11] conf: Extend virDomainDefValidate(Callback) for parseOpaque
On Sat, Sep 29, 2018 at 05:34 PM +0200, John Ferlan <jferlan(a)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(a)linux.ibm.com>
> Reviewed-by: Boris Fiuczynski <fiuczy(a)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
12:44 p.m.
New subject: [libvirt] [PATCH 08/11] conf: Use domainPostParseData(Alloc|Free) in virDomainDefValidate
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(a)linux.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)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
10:35 a.m.
New subject: [libvirt] [PATCH 08/11] conf: Use domainPostParseData(Alloc|Free) in virDomainDefValidate
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(a)linux.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)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;
}
5:49 a.m.
New subject: [libvirt] [PATCH 08/11] conf: Use domainPostParseData(Alloc|Free) in virDomainDefValidate
On Sat, Sep 29, 2018 at 05:35 PM +0200, John Ferlan <jferlan(a)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(a)linux.ibm.com>
> Reviewed-by: Boris Fiuczynski <fiuczy(a)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
12:44 p.m.
New subject: [libvirt] [PATCH 09/11] qemu: Use @parseOpaque in qemuDomainDefValidate
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(a)linux.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)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
12:44 p.m.
New subject: [libvirt] [PATCH 10/11] conf: Extend virDomainDeviceDefValidate(Callback) for parseOpaque
Add parseOpaque parameter to virDomainDeviceDefValidate(Callback). But
don't use it for now.
Signed-off-by: Marc Hartmayer <mhartmay(a)linux.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)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
12:44 p.m.
New subject: [libvirt] [PATCH 11/11] qemu: Use @parseOpaque in qemuDomainDeviceDefValidate
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(a)linux.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)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
4:43 p.m.
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 :|
6:47 a.m.
Daniel P. Berrangé <berrange(a)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
12:32 p.m.
On Thu, Oct 25, 2018 at 01:47:26PM +0200, Bjoern Walk wrote:
Daniel P. Berrangé <berrange(a)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 :|
11:57 p.m.
Daniel P. Berrangé <berrange(a)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(a)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
4:33 a.m.
On Fri, Oct 26, 2018 at 06:57:54AM +0200, Bjoern Walk wrote:
Daniel P. Berrangé <berrange(a)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(a)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 :|
2:32 a.m.
On Wed, Oct 24, 2018 at 11:43 PM +0200, "Daniel P. Berrangé"
<berrange(a)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
2218
days inactive
2254
days old
40 comments
5 participants
participants (5)
-
Bjoern Walk -
Daniel P. Berrangé -
John Ferlan -
Marc Hartmayer -
Peter Krempa