On Mon, Oct 01, 2018 at 09:33 AM +0200, Peter Krempa <pkrempa@xxxxxxxxxx> 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@xxxxxxxxxxxxx>
>> > Reviewed-by: Boris Fiuczynski <fiuczy@xxxxxxxxxxxxx>
>> > ---
>> > src/qemu/qemu_process.c | 2 +-
>> > 1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
>> > index 44c63c42d618..6c5a6472d8cd 100644
>> > --- a/src/qemu/qemu_process.c
>> > +++ b/src/qemu/qemu_process.c
>> > @@ -5282,7 +5282,7 @@ qemuProcessInit(virQEMUDriverPtr driver,
>>
>> The comment just above here could use a tweak for grammar ;-):
>>
>> /* in case when the post parse callback failed we need to re-run it
>> on the
>> * old config prior we start the VM */
>>
>> > if (vm->def->postParseFailed) {
>> > VIR_DEBUG("re-running the post parse callback");
>> >
>> > - if (virDomainDefPostParse(vm->def, caps, 0, driver->xmlopt, NULL) < 0)
>> > + if (virDomainDefPostParse(vm->def, caps, 0, driver->xmlopt, priv->qemuCaps) < 0)
>>
>> Searching through history of this line finds Peter's original commit in
>> this area - 7726d158, which seems to indicate a very specific reason for
>> providing a NULL capabilities value here.
>>
>> I think from this patch on is something Peter has worked on a lot, so I
>> would prefer to defer to Peter on them since I'm sure he understands all
>> the various PostParse and Validate special conditions and various flags
>> usage better than I do.
>
> Thanks for notifying me.
First, thanks for your answer!
>
> In general, when parsing a "new" domain config as it's the case here,
> NULL should be passed in.
Wouldn’t it be better to invalidate the QEMU caps (priv->qemuCaps)
explicitly beforehand? (for example if the 'postParseFailed' flag is set
and we’re starting the domain).
>
> The qemu driver registers the 'domainPostParseDataAlloc' callback to
> qemuDomainPostParseDataAlloc. This callback is executed after the
> 'basic' post parse callback which fills in the emulator binary.
>
> Passing an explicit qemuCaps is meant only for special cases e.g.
> migration where we have a specific set of capabilities which need to be
> used rather than a new one.
I would do all the precondition stuff (e.g. the priv->qemuCaps isn’t
valid anymore) as early as possible in the overall “task/job” process.
>
> This patch does not seem to make sense with the justification provided
> here as the post-parse callbacks fill in the proper caps here and this
> part clearly uses a new domain configuration, so the default behaviour
> should be used.
>
> Changing this would also need that we change the normal parser path to
> achieve the same results which is impossible as you don't have access to
> priv->qemuCaps prior to creating the virDomainObj object.
Yep, that’s why I said in the cover letter “With this patch series the
behavior is still not (completely) fixed, but the performance has been
significantly improved.”.
IMHO, it’s a design flaw that the virDomainObjList class is responsible
for the creation of the virDomainObj… The responsibilities of the
classes are mixed and this enforces the lock order virDomainObjList ->
virDomainObj (what actually is a performance bottleneck). IMHO, we
should create the virDomainObj earlier and add it to the
virDomainObjList when it’s useful.
What’s your opinion about that?
>
--
Kind regards / Beste Grüße
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
