Re: [Qemu-devel] [PATCH qom v2 1/2] qom: object_property_add: Add automatic arrayification

[Peter Crosthwaite]

On Tue, Aug 12, 2014 at 9:36 PM, Andreas Färber <address@hidden> wrote:
> Am 04.08.2014 07:08, schrieb Peter Crosthwaite:
>> If "[*]" is given as the last part of a QOM property name, treat that
>> as an array property. The added property is given the first available
>> name, replacing the * with a decimal number counting from 0.
>>
>> First add with name "foo[*]" will be "foo[0]". Second "foo[1]" and so
>> on.
>
> I think it's worth mentioning here that the caller learns about which
> one has been created through the ObjectProperty return value - which is
> not immediately obvious from the diff.
>

TBH I wasn't thinking about this at all. All usages I've had for this
the caller does not care about the enumeration. The usual usage
pattern is

for (i = 0, i < n; ++i) {
    object_property_add(obj, "name[*]", get, set, release, opaque[i], err);
}

Someone else later is responsible for knowing how many or simply
querying the device for existence of the nth property.

That said. What you are saying should work. So I'll add the comment as
you suggest for completness.

>>
>> Signed-off-by: Peter Crosthwaite <address@hidden>
>> ---
>> changed since v1 (Paolo review):
>> Cache strlen result in variable
>> Use memcmp instead of strncmp
>>
>> Suggest by Paolo and first pass discussion on list about the feature
>> here:
>>
>> https://lists.nongnu.org/archive/html/qemu-devel/2014-06/msg03794.html
>>
>>  qom/object.c | 23 +++++++++++++++++++++++
>>  1 file changed, 23 insertions(+)
>>
>> diff --git a/qom/object.c b/qom/object.c
>> index 0e8267b..4484330 100644
>> --- a/qom/object.c
>> +++ b/qom/object.c
>> @@ -738,6 +738,29 @@ object_property_add(Object *obj, const char *name, 
>> const char *type,
>>                      void *opaque, Error **errp)
>>  {
>>      ObjectProperty *prop;
>> +    size_t name_len = strlen(name);
>> +
>> +    if (name_len >= 3 && !memcmp(name + name_len - 3, "[*]", 4)) {
>> +        int i;
>> +        ObjectProperty *ret;
>> +        char *name_no_array = g_strdup(name);
>> +
>> +        name_no_array[name_len - 3] = '\0';
>> +        for (i = 0; ; ++i) {
>> +            char *full_name = g_strdup_printf("%s[%d]", name_no_array, i);
>> +            Error *local_err = NULL;
>
> White line missing here...
>

Fixed

>> +            ret = object_property_add(obj, full_name, type, get, set,
>> +                                      release, opaque, &local_err);
>> +
>
> ... but present here.
>

Fixed

>> +            g_free(full_name);
>> +            if (!local_err) {
>> +                break;
>> +            }
>> +            error_free(local_err);
>
> Can't we do this without creating and throwing away an Error by
> comparing the list of obj->properties against full_name like below?
>

Well, I don't really like it, because O(1) operation becomes O(n). And
my preference is to use errp for error detection when possible rather
than inspecting the operation side effects (Markus any thoughts?).
That said, I think we could actually use the return code from the
function directly. Object_property_add will return NULL in cases where
it throws an error, so it may be trivially solvable with:

if (!ret) {
    break;
}

Paolo wrote this code this way in the first place though (i translate
across to object.c) and I do like the local_err approach more.

Regards,
Peter

>> +        }
>> +        g_free(name_no_array);
>> +        return ret;
>> +    }
>>
>>      QTAILQ_FOREACH(prop, &obj->properties, node) {
>>          if (strcmp(prop->name, name) == 0) {
>
> Regards,
> Andreas
>
> --
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
>