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 <berra...@redhat.com>
> >> 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 :|
