Damien Hedde <damien.he...@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.
