Kevin Wolf <kw...@redhat.com> writes:
> Am 14.10.2023 um 08:36 hat Markus Armbruster geschrieben:
>> Kevin Wolf <kw...@redhat.com> writes:
>>
>> > Am 22.09.2023 um 17:05 hat Markus Armbruster geschrieben:
>> >> Kevin Wolf <kw...@redhat.com> writes:
>> >>
>> >> > Until now, array properties are actually implemented with a hack that
>> >> > uses multiple properties on the QOM level: a static "foo-len" property
>> >> > and after it is set, dynamically created "foo[i]" properties.
>> >> >
>> >> > In external interfaces (-device on the command line and device_add in
>> >> > QMP), this interface was broken by commit f3558b1b ('qdev: Base object
>> >> > creation on QDict rather than QemuOpts') because QDicts are unordered
>> >> > and therefore it could happen that QEMU tried to set the indexed
>> >> > properties before setting the length, which fails and effectively makes
>> >> > array properties inaccessible. In particular, this affects the 'ports'
>> >> > property of the 'rocker' device.
>> >> >
>> >> > This patch reworks the external interface so that instead of using a
>> >> > separate top-level property for the length and for each element, we use
>> >> > a single true array property that accepts a list value. In the external
>> >> > interfaces, this is naturally expressed as a JSON list and makes array
>> >> > properties accessible again.
>> >> >
>> >> > Creating an array property on the command line without using JSON format
>> >> > is currently not possible. This could be fixed by switching from
>> >> > QemuOpts to a keyval parser, which however requires consideration of the
>> >> > compatibility implications.
>> >> >
>> >> > All internal users of devices with array properties go through
>> >> > qdev_prop_set_array() at this point, so updating it takes care of all of
>> >> > them.
>> >> >
>> >> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1090
>> >> > Fixes: f3558b1b763683bb877f7dd5b282469cdadc65c3
>> >> > Signed-off-by: Kevin Wolf <kw...@redhat.com>
>
>> >> > +{
>> >> > + return (Property) {
>> >> > + .info = parent_prop->arrayinfo,
>> >> > + .name = name,
>> >> > + /*
>> >> > + * This ugly piece of pointer arithmetic sets up the offset so
>> >> > + * that when the underlying release hook calls
>> >> > qdev_get_prop_ptr
>> >> > + * they get the right answer despite the array element not
>> >> > actually
>> >> > + * being inside the device struct.
>> >> > + */
>> >> > + .offset = elem - (char *) obj,
>> >>
>> >> Isn't this is undefined behavior?
>> >
>> > It should be at least less illegal than the old version of it, which did
>> > the calculation on void * and still worked in practice...
>> >
>> > But yes, strictly speaking, it's probably undefined behaviour. I can
>> > calculate on uintptr_t instead, and then it should be defined here.
>> >
>> > The QOM counterpart object_field_prop_ptr() is probably still undefined
>> > because it calculates on a pointer and I think the spec allows casting
>> > back to a pointer only after we've applied the offset so that we stay in
>> > the same object with pointer arithmetics.
>>
>> We should not have to waste time on worrying about compilers using UB
>> fine print against us, but sadly we do.
>>
>> I'm not objecting to your code, I'm merely pointing out a potential time
>> bomb. In a programming environment that has embraced time bombing with
>> gusto.
>
> While I'm touching this part, I'll change it to uintptr_t. I'll leave
> the counterpart for later.
>
>> >> Delete the space between (char *) and obj.
>> >>
>> >> > + };
>> >> > +}
>> >> > +
>> >> > +/*
>> >> > + * Object property release callback for array properties: We call the
>> >> > underlying
>> >> > + * element's property release hook for each element.
>> >> > + *
>> >> > + * Note that it is the responsibility of the individual device's
>> >> > deinit to free
>> >> > + * the array proper.
>> >>
>> >> What is a device's "deinit"? Is it the unrealize() method? The
>> >> instance_finalize() method?
>> >
>> > Who knows? I only moved this comment.
>>
>> Opportunity to improve it. Not a demand.
>
> If you have a better version of it? My guess is only a guess, so I'd
> avoid putting it in the code without understanding if there are reasons
> why it has to be a specific place, or why a specific place doesn't work.
>
> Leaving it vague is probably better than being specific, but potentially
> wrong.
Fair.
>> > My guess is that it doesn't really matter as long as _something_ frees
>> > the array when unplugging the device.
>> >
>> >> > */
>> >> > -static void array_element_release(Object *obj, const char *name, void
>> >> > *opaque)
>> >> > +static void release_prop_array(Object *obj, const char *name, void
>> >> > *opaque)
>> >> > {
>> >> > - ArrayElementProperty *p = opaque;
>> >> > - if (p->release) {
>> >> > - p->release(obj, name, opaque);
>> >> > + Property *prop = opaque;
>> >> > + uint32_t *alenptr = object_field_prop_ptr(obj, prop);
>> >> > + void **arrayptr = (void *)obj + prop->arrayoffset;
>> >>
>> >> I'd call these @plen and @pelts, but that's clearly a matter of taste.
>> >
>> > I just kept the old names in set_prop_array() and used the same names in
>> > new functions to stay consistent. But to be honest, @plen and @pelts
>> > would be equally confusing to me.
>> >
>> > My own choice would probably have been something like array_len and
>> > array_data (if you want to know that it's a pointer, look at its type).
>> >
>> >> > + char *elem = *arrayptr;
>> >> > + int i;
>> >> > +
>> >> > + for (i = 0; i < *alenptr; i++) {
>> >> > + Property elem_prop = array_elem_prop(obj, prop, name, elem);
>> >> > + prop->arrayinfo->release(obj, NULL, &elem_prop);
>> >> > + elem += prop->arrayfieldsize;
>> >> > }
>> >> > - g_free(p->propname);
>> >> > - g_free(p);
>> >> > }
>> >> >
>> >> > -static void set_prop_arraylen(Object *obj, Visitor *v, const char
>> >> > *name,
>> >> > - void *opaque, Error **errp)
>> >> > +/*
>> >> > + * Setter for an array property. This sets both the array length
>> >> > (which is
>> >> > + * technically the property field in the object) and the array itself
>> >> > (a pointer
>> >> > + * to which is stored in the additional field described by
>> >> > prop->arrayoffset).
>> >> > + */
>> >> > +static void set_prop_array(Object *obj, Visitor *v, const char *name,
>> >> > + void *opaque, Error **errp)
>> >> > {
>> >> > - /* Setter for the property which defines the length of a
>> >> > - * variable-sized property array. As well as actually setting the
>> >> > - * array-length field in the device struct, we have to create the
>> >> > - * array itself and dynamically add the corresponding properties.
>> >> > - */
>> >> > + ERRP_GUARD();
>> >> > +
>> >>
>> >> Drop the blank line.
>> >>
>> >> > Property *prop = opaque;
>> >> > uint32_t *alenptr = object_field_prop_ptr(obj, prop);
>> >> > void **arrayptr = (void *)obj + prop->arrayoffset;
>> >> > - void *eltptr;
>> >> > - const char *arrayname;
>> >> > - int i;
>> >> > + GenericList *list, *elem, *next;
>> >> > + const size_t list_elem_size = sizeof(*list) + prop->arrayfieldsize;
>> >>
>> >> This can be smaller than the size of the QAPI-generated list type, since
>> >> the compiler may add padding. Does it matter?
>> >
>> > If it happens in practice, it does matter. Do we have any cleaner way to
>> > get the element size without knowing the content of the list?
>> >
>> > I expect that because GenericList only contains a single pointer, the
>> > rest should have natural alignment
>>
>> Yes, GenericList's size and alignment should match a pointer's:
>>
>> typedef struct GenericList {
>> struct GenericList *next;
>> char padding[];
>> } GenericList;
>>
>> > and therefore the compiler shouldn't
>> > have any reason to insert padding.
>>
>> The actual list will look like
>>
>> struct FOOList {
>> FOOList *next;
>> FOOTYPE value;
>> }
>>
>> where FOOTYPE is some QAPI-generated type. No padding as long as
>> FOOTYPE's alignment divides the pointer size. I figure that's true for
>> our current targets and generated QAPI types (currently pointers,
>> double, bool, or integers up to 64 bits).
>
> I'm quite confused about the whole alignment stuff at the moment, but
> after looking some more at the code, I think I just realised one
> important thing: We're actually not dealing with QAPI-generated types
> here, but with custom visitors in QOM property setters.
Hmm.
The patch is about qdev array properties, which are special QOM
properties.
A QOM property encapsulates arbitray C data within a QOM object. Its
get() method enables getting this data with an output visitor, and its
set() method enables setting it with an input visitor.
When the C data is a QAPI type T, set() and get() simply call
visit_type_T(). Example: property_get_uint8_ptr(),
property_set_uint8_ptr().
When it isn't, set() and get() do what qapi/visitor.h calls a "virtual
walk". Example: the array property we're discussing here.
However, there's a catch: set_prop_array() doesn't visit the property's
C data (a heap-allocated array of ELEMENT-TYPE) directly. Instead, it
visits a heap-allocated GenericList of ELEMENT-TYPE, which it later
copies to the array.
Here's the visit:
/* Read the whole input into a temporary list */
elem = list;
while (elem) {
Property elem_prop = array_elem_prop(obj, prop, name, elem->padding);
---> prop->arrayinfo->set(obj, v, NULL, &elem_prop, errp);
if (*errp) {
ok = false;
goto out_obj;
}
(*alenptr)++;
elem = visit_next_list(v, elem, list_elem_size);
}
The call marked ---> visits an element at address &elem->padding.
This address is pointer-aligned. What if the element type requires more
alignment? Unusual, but certainly not impossible. For instance, SSE
and AVX vectors require 16 or even 32 byte alignment.
Or am I confused?
> The netdev one
> specifically (which is used in the array property of the 'rocker'
> device) uses NICPeers (not a pointer to NICPeers!), which is much
> larger.
>
> But now I'm wondering, FOOList doesn't even exist as a type. We're
> calculating list_elem_size only to pass it to visit_start_list(), which
> doesn't have FOOList either. What does the visitor do with it?
>
> It seems it doesn't do anything with it except using it as the size to
> malloc() elements. And then we (the array property code in this patch)
> use &elem->padding as the element pointer, directly from GenericList,
> not any specific FOOList. So I think this just means that we never
> insert any padding and what the compiler would do with a hypothetical
> FOOList, which doesn't even have to exist as a type, doesn't matter.
>
> Am I missing something or does this mean that the code is actually fine?
>
>> > If you think this is not enough and there is no other way to get the
>> > size of the list elements, we might have to generate packed structs for
>> > the QAPI list types (which are really only two pointers, so not much to
>> > lose when we do that).
>>
>> Could we assert the element type's alignment divides GenericList's size?
>> Not here, obviously, but in DEFINE_PROP_ARRAY(), where we can use
>> __alignof__(_arraytype).
>
> A bit tricky because we're in the middle of a literal, but I suppose
> with the GCC extension we could change one random element of the struct
> to something like:
>
> ({ QEMU_BUILD_BUG_ON(...); real_value; })
>
> Of course, if my thoughts above were right, this might not actually be
> needed.
>
>> We could also play with attribute aligned to ensure GenericList's size is
>> safe, but I doubt that's worthwhile.
>>
>> >> > + char *elemptr;
>> >> > + bool ok = true;
>> >> >
>> >> > if (*alenptr) {
>> >> > error_setg(errp, "array size property %s may not be set more
>> >> > than once",
>> >> > name);
>> >> > return;
>> >> > }
>> >> > - if (!visit_type_uint32(v, name, alenptr, errp)) {
>> >> > +
>> >> > + if (!visit_start_list(v, name, &list, list_elem_size, errp)) {
>> >> > return;
>> >> > }
>> >> > - if (!*alenptr) {
>> >> > +
>> >> > + /* Read the whole input into a temporary list */
>> >> > + elem = list;
>> >> > + while (elem) {
>> >> > + Property elem_prop = array_elem_prop(obj, prop, name,
>> >> > elem->padding);
>> >> > + prop->arrayinfo->set(obj, v, NULL, &elem_prop, errp);
>> >> > + if (*errp) {
>> >> > + ok = false;
>> >> > + goto out_obj;
>> >> > + }
>> >> > + (*alenptr)++;
>> >> > + elem = visit_next_list(v, elem, list_elem_size);
>> >> > + }
>> >> > +
>> >> > + ok = visit_check_list(v, errp);
>> >> > +out_obj:
>> >> > + visit_end_list(v, (void**) &list);
>> >> > +
>> >> > + if (!ok) {
>> >> > + for (elem = list; elem; elem = next) {
>> >> > + next = elem->next;
>> >> > + g_free(elem);
>> >> > + }
>> >>
>> >> We consume the list even on error. It's too late in my day for me to
>> >> see why that's proper.
>> >
>> > Who else would free it otherwise?
>> >
>> > This is pretty much the same as the generated list visitors do.
>>
>> Help me out: point me to the precedence you have in mind.
>
> Any QAPI generated list visiting function should do. As an example, I'm
> looking at visit_type_Qcow2BitmapInfoList():
>
> ...
> ok = visit_check_list(v, errp);
> out_obj:
> visit_end_list(v, (void **)obj);
> if (!ok && visit_is_input(v)) {
> qapi_free_Qcow2BitmapInfoList(*obj);
> *obj = NULL;
> }
> return ok;
> }
>
> On failure, it calls qapi_free_Qcow2BitmapInfoList().
>
> What's probably wrong in my code is that to be a full equivalent, it
> also needs to free things behind pointers that are owned by elem, i.e.
> call prop->arrayinfo->release().
>
> In practice, it doesn't currently make a difference because none of the
> types used with array properties actually have a release callback.
> Should still be fixed, of course.
Glad I got sufficiently confused to ask ;)
>> >> > return;
>> >> > }
>> >> >
>> >> > - /* DEFINE_PROP_ARRAY guarantees that name should start with this
>> >> > prefix;
>> >> > - * strip it off so we can get the name of the array itself.
>> >> > + /*
>> >> > + * Now that we know how big the array has to be, move the data
>> >> > over to a
>> >> > + * linear array and free the temporary list.
>> >> > */
>> >> > - assert(strncmp(name, PROP_ARRAY_LEN_PREFIX,
>> >> > - strlen(PROP_ARRAY_LEN_PREFIX)) == 0);
>> >> > - arrayname = name + strlen(PROP_ARRAY_LEN_PREFIX);
>> >> > + *arrayptr = g_malloc_n(*alenptr, prop->arrayfieldsize);
>> >> > + elemptr = *arrayptr;
>> >> > + for (elem = list; elem; elem = next) {
>> >> > + memcpy(elemptr, elem->padding, prop->arrayfieldsize);
>> >> > + elemptr += prop->arrayfieldsize;
>> >> > + next = elem->next;
>> >> > + g_free(elem);
>> >> > + }
>> >> > +}
>> >> >
>> >> > - /* Note that it is the responsibility of the individual device's
>> >> > deinit
>> >> > - * to free the array proper.
>> >> > - */
>> >> > - *arrayptr = eltptr = g_malloc0(*alenptr * prop->arrayfieldsize);
>> >> > - for (i = 0; i < *alenptr; i++, eltptr += prop->arrayfieldsize) {
>> >> > - char *propname = g_strdup_printf("%s[%d]", arrayname, i);
>> >> > - ArrayElementProperty *arrayprop = g_new0(ArrayElementProperty,
>> >> > 1);
>> >> > - arrayprop->release = prop->arrayinfo->release;
>> >> > - arrayprop->propname = propname;
>> >> > - arrayprop->prop.info = prop->arrayinfo;
>> >> > - arrayprop->prop.name = propname;
>> >> > - /* This ugly piece of pointer arithmetic sets up the offset so
>> >> > - * that when the underlying get/set hooks call
>> >> > qdev_get_prop_ptr
>> >> > - * they get the right answer despite the array element not
>> >> > actually
>> >> > - * being inside the device struct.
>> >> > - */
>> >> > - arrayprop->prop.offset = eltptr - (void *)obj;
>> >> > - assert(object_field_prop_ptr(obj, &arrayprop->prop) == eltptr);
>> >> > - object_property_add(obj, propname,
>> >> > - arrayprop->prop.info->name,
>> >> > - field_prop_getter(arrayprop->prop.info),
>> >> > - field_prop_setter(arrayprop->prop.info),
>> >> > - array_element_release,
>> >> > - arrayprop);
>> >> > +static void get_prop_array(Object *obj, Visitor *v, const char *name,
>> >> > + void *opaque, Error **errp)
>> >> > +{
>> >> > + ERRP_GUARD();
>> >> > +
>> >>
>> >> Drop the blank line.
>> >>
>> >> > + Property *prop = opaque;
>> >> > + uint32_t *alenptr = object_field_prop_ptr(obj, prop);
>> >> > + void **arrayptr = (void *)obj + prop->arrayoffset;
>> >> > + char *elem = *arrayptr;
>> >> > + GenericList *list;
>> >> > + const size_t list_elem_size = sizeof(*list) + prop->arrayfieldsize;
>> >> > + int i;
>> >> > +
>> >> > + if (!visit_start_list(v, name, &list, list_elem_size, errp)) {
>> >> > + return;
>> >> > }
>> >> > +
>> >> > + for (i = 0; i < *alenptr; i++) {
>> >> > + Property elem_prop = array_elem_prop(obj, prop, name, elem);
>> >> > + prop->arrayinfo->get(obj, v, NULL, &elem_prop, errp);
>> >> > + if (*errp) {
>> >> > + goto out_obj;
>> >> > + }
>> >> > + elem += prop->arrayfieldsize;
>> >> > + }
>> >> > +
>> >>
>> >> You neglect to call visit_check_list().
>> >
>> > It is documented to be intended for input visitors only. Do we need it
>> > with an output visitor?
>>
>> Help me out: where is that documented?
>
> In struct Visitor:
>
> /* Optional; intended for input visitors */
> bool (*check_list)(Visitor *v, Error **errp);
I think this comment tries to say "this is intended to be used by input
visitors", which doesn't quite imply "only input visitors use this".
> And indeed, the existing output vistors don't implement it.
It's implemented by all the input visitors.
Fine print: the forward visitor is the same kind of visitor as the one
it wraps, either input or output. It implements check_list()
unconditionally.
> Admittedly, the public visit_check_list() is less clear:
>
> /*
> * Prepare for completing a list visit.
> *
> * On failure, store an error through @errp. Can happen only when @v
> * is an input visitor.
> *
> * Return true on success, false on failure.
> *
> * Should be called prior to visit_end_list() if all other
> * intermediate visit steps were successful, to allow the visitor one
> * last chance to report errors. May be skipped on a cleanup path,
> * where there is no need to check for further errors.
> */
> bool visit_check_list(Visitor *v, Error **errp);
>
> If you prefer, I can add it here just to be sure. Instead of having real
> error handling, assert(ok) should be enough.
We either tighten the public visitor contract to make calling
visit_check_list() clearly optional when you know you're not using an
input visitor.
Or we call it here, too. Probably simpler.
>> >> > +out_obj:
>> >> > + visit_end_list(v, (void**) &list);
>> >> > }
>
> Kevin
