Re: [Qemu-devel] [PULL 22/27] vl: Create block backends before setting machine properties

[Markus Armbruster]

I apologize for the delay, got distracted.

Michal Privoznik <address@hidden> writes:

> On 5/16/19 1:43 PM, Markus Armbruster wrote:
> <snip/>
>
>>> Actually, there is more problems with this. Trying to run a guest with
>>> persistent reservations fails after this patch is applied (git bisect
>>> points me to this commit). My command line is:
>>>
>>> qemu.git $ ./x86_64-softmmu/qemu-system-x86_64 \
>>> -monitor stdio \
>>> -object pr-manager-helper,id=pr-helper0,path=/tmp/pr-helper0.sock
>>> -drive
>>> file=/dev/mapper/crypt,file.pr-manager=pr-helper0,format=raw,if=none,id=drive-scsi0-0-0-2
>>> \
>>> -device
>>> scsi-block,bus=scsi0.0,channel=0,scsi-id=0,lun=2,drive=drive-scsi0-0-0-2,id=scsi0-0-0-2
>>>
>>> Honestly, I have no idea how to fix it, so I'm just raising this issue
>>> here. Do you want me to open a bug or something?
>>
>> Let's skip the bug filing bureaucracy and go straight to debugging.
>
> Agreed.
>
>>
>> Actual and expected behavior of your reproducer, please :)
>>
>
> Actual is that qemu fails to parse cmd line:
>
>
> qemu-system-x86_64: -drive
> file=/dev/mapper/crypt,file.pr-manager=pr-helper0,format=raw,if=none,id=drive-scsi0-0-0-2:
> No persistent reservation manager with id 'pr-helper0'
>
>
> Which obviously is not correct, because pr-helper0 is specified.
> Expected result is that qemu suceeds in parsing the cmd line and
> starts the guest. To test it you don't need /dev/mapper/* really, I
> mean, if qemu fails with a different error message (e.g. it can't open
> the disk or EPERM or whatever), it's a sign it got past the cmd line
> parsing successfuly.

Reproduced, thanks!

Here's what happens.  Our general problem is that qemu-system-FOO's
main() acts on command line arguments in its own idiosyncratic order.
There's not much method to its madness.  Whenever we find a case where
one kind of command line argument needs to refer to something created
for another kind later, we rejigger the order.

Some time back, Dan Berrangé ran into an "impossible" instance of this
general problem: some kinds of -object get referenced by certain
character devices (therefore, -object must be acted on before character
devices), but other kinds of -object reference other character devices
(therefore, -object must be acted on after character devices).  He
solved the problem by sorting the -object into two buckets (commit
f08f9271bfe):

* Normal ones are created pretty early, so they can be referenced by
  (most) other things.

* Delayed ones are created pretty late, so they can reference (most)
  other things.

The pr-manager-helper object is a delayed one (commit 7c9e527659c).

Worked because block backends got created even after delayed objects:

    qemu_opts_foreach(qemu_find_opts("object"),
                      user_creatable_add_opts_foreach,
                      object_create_initial, &error_fatal);
    [...]
    qemu_opts_foreach(qemu_find_opts("object"),
                      user_creatable_add_opts_foreach,
                      object_create_delayed, &error_fatal);
    [...]
    configure_blockdev(&bdo_queue, machine_class, snapshot);

Commit cda4aa9a5a0 moved the configure_blockdev() up:

    qemu_opts_foreach(qemu_find_opts("object"),
                      user_creatable_add_opts_foreach,
                      object_create_initial, &error_fatal);
    [...]
    /*
     * Note: we need to create block backends before
     * machine_set_property(), so machine properties can refer to
     * them.
     */
    configure_blockdev(&bdo_queue, machine_class, snapshot);
    [...]
    qemu_opts_foreach(qemu_find_opts("object"),
                      user_creatable_add_opts_foreach,
                      object_create_delayed, &error_fatal);

Now file-posix property "pr-manager" can no longer reference a
pr-manager-helper object.  Regression.

If I make pr-manager-helper a normal object, it again can reference it.

Paolo, why is pr-manager-helper a delayed object?  Why this hunk of
commit 7c9e527659c:

    diff --git a/vl.c b/vl.c
    index 9bb5058c3a..a121a65731 100644
    --- a/vl.c
    +++ b/vl.c
    @@ -2893,7 +2893,8 @@ static int machine_set_property(void *opaque,
      */
     static bool object_create_initial(const char *type)
     {
    -    if (g_str_equal(type, "rng-egd")) {
    +    if (g_str_equal(type, "rng-egd") ||
    +        g_str_has_prefix(type, "pr-manager-")) {
             return false;
         }