Re: RFC: QMP configuration - allocating/setting qdev array properties?

[Markus Armbruster <armbru@redhat.com>]

Damien Hedde <damien.hedde@greensocs.com> writes:

> Hi Mirela,
>
> On 1/11/22 17:54, Mirela Grujic wrote:
>> Hi,
>> 
>> While working on a prototype and configuring a whole machine using
>> QMP we run into the following scenario.
>> 
>> Some device models use array properties.

A gift that keeps on giving...

>>                                          The array is allocated when
>> len-<arrayname> property is set, then, individual elements of the
>> array can be set as any other property (see description above the 
>> DEFINE_PROP_ARRAY definition in qdev-properties.h for more
>> details). We need to do both (allocate the array and set its
>> elements) before the device can be realized. Attempting to set
>> len-<arrayname> and array elements in a single device_add command
>> does not work because the order of setting properties is not
>> guaranteed, i.e. we're likely attempting to set an element of the
>> array that's not yet allocated.
>
> It happens because device options are stored in an optdict. When this
> optdict is traversed to set the qdev-properties, no specific order is
> used.

To be precise: it's stored in a QDict[*]

qdev_device_add_from_qdict() sets properties with
object_set_properties_from_qdict(), which iterates over the QDict in
unspecified order.

> Better json format support would probably solve this issue in the
> long-term. But right now, we are stuck with the optdict in the middle 
> which do not support advanced structure like lists or dictionaries.

I figure you mean actual array-valued properties, like

    'foo': [ 1, 2, 3 ]

instead of

    'len-foo': 3, 'len[0]': 1, 'len[1]': 2, 'len[2]': 3

> We could solve this by being more "smart" in when setting the
> properties. I'm not sure we can be really smart here and detect which 
> options is an array length but we could at least have some heuristic
> and for example: set first "len-xxx" properties so that array will be 
> allocated before being filled.

Ugh!

Another stop gap solution could be making QDict iterate in insertion
order, like Python dict does since 3.6.

>> Allowing the device initialize and realize phases to be split would 
>> solve this problem. For example, the device_add would be issued with
>> 'realized=false', we can set the len-<arrayname> in the same
>> device_add command or a following qom-set command, then we would use
>> a sequence of qom-set commands to set array elements, and at the
>> end, we would realize the device by issuing qom-set path=<device_id>
>> property=realized value=true. This is what we currently do in our
>> prototype.
>
> I think that is a bad idea. Because then the user would have access to
> a "not-realized" device (which is really a not-constructed object).
> It could then do anything with the object (which might work or not
> might). And at the end, maybe realize will fail and that would leave 
> qemu in a inconsistent state: the object will be used somewhere and at
> the same time it will be a state where it is not usable.

I don't regard a not-realized device as not-constructed object.
Construction is qdev_new().  We then configure by setting properties.
Realization makes the device accessible to the guest.

-device / device_add fuse all this into one operation: create device,
set the properties specified by the user, realize.

C code need not fuse like this.  There are places where we create
devices, then abandon them, i.e. destroy them without realizing.

I share your concern that providing the user the basic operations
unfused might expose more bugs.

In today's usage, a fused operation is all we need.  But when wiring up
complex composite devices in configuration rather than C code, we may
get to the point where we need the basic operations unfused.


[*] -device / device_add with a non-JSON argument go to QDict via
QemuOpts.  Doesn't matter here.