Re: [Qemu-devel] [PATCH RFC 1/7] qom: allow properties to be registered against classes

[Daniel P. Berrange]

On Mon, Sep 07, 2015 at 03:11:56PM +0200, Andreas Färber wrote:
> Am 07.09.2015 um 10:46 schrieb Daniel P. Berrange:
> > On Fri, Sep 04, 2015 at 11:38:06PM +0200, Marc-André Lureau wrote:
> >> On Wed, Aug 26, 2015 at 2:03 PM, Daniel P. Berrange <address@hidden> wrote:
> >>> +ObjectProperty *
> >>> +object_class_property_add(ObjectClass *klass,
> >>> +                          const char *name,
> >>> +                          const char *type,
> >>> +                          ObjectPropertyAccessor *get,
> >>> +                          ObjectPropertyAccessor *set,
> >>> +                          ObjectPropertyRelease *release,
> >>> +                          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);
> >>> +
> >>
> >> I question the need for dynamic/array property name registered in
> >> classes. What would be more useful is an array property instead. It
> >> would help to introspect classes for dynamic "children[*]" case.
> >> object_property_add_child() could verify/check against the class
> >> declaration, and grow the instance properties list (like it does now,
> >> but it would be only for instances of children[] items). On
> >> introspection of classes, the class "children[*]" property would be
> >> visible, but would be hidden when introspecting the instance, and you
> >> wouldn't be able to lookup that "array" property.
> >>
> >> It seems relatively straightforward to deal with the link<> case, by
> >> storing the offset of the "child" pointer. This seems fine if limited
> >> to a single link<> (it should probably check the prop is not of the
> >> name[*] style already), ex:
> >> https://gist.github.com/elmarco/905241b683fb9c5f2a08
> >>
> >> Your patches looks good  to me in general but object_property_del()
> >> should be fixed, since the prop find may belong to the class.
> > 
> > Actually I skipped object_property_del() intentionally. Classes should
> > be immutable once defined, so deleting a property from a class would
> > not be appropriate.
> 
> Agreed, I don't see a use case either.
> 
> Can you propose a sentence to amend the commit message with? Then I
> would apply this patch to my upcoming qom-next pull, then the unreviewed
> rest could go through their respective maintainers.

"Supporting for deletion of properties registered on classes is
 omitted, since class definitions must be immutable once created"

Before you queue it though, I was going to repost with the support
for magic "[*]" property expansion removed, unless you think that
is really needed. It doesn't do anything you can't do explicitly
so I figure its cleaner to not add this magic to the class imp
too, as its known to suffer poor scalability.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|