| Message ID | 20201029220246.472693-20-ehabkost@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Make qdev static property API usable by any QOM type | expand |
On Fri, Oct 30, 2020 at 2:17 AM Eduardo Habkost <ehabkost@redhat.com> wrote: > The array property registration code is hard to follow. Move the > two steps into separate functions that have clear > responsibilities. > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > --- > Cc: Paolo Bonzini <pbonzini@redhat.com> > Cc: "Daniel P. Berrangé" <berrange@redhat.com> > Cc: Eduardo Habkost <ehabkost@redhat.com> > Cc: qemu-devel@nongnu.org > --- > hw/core/qdev-properties.c | 60 ++++++++++++++++++++++++++------------- > 1 file changed, 41 insertions(+), 19 deletions(-) > > diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c > index 27c09255d7..1f06dfb5d5 100644 > --- a/hw/core/qdev-properties.c > +++ b/hw/core/qdev-properties.c > @@ -588,6 +588,32 @@ typedef struct { > ObjectPropertyRelease *release; > } ArrayElementProperty; > > +/** > + * Create ArrayElementProperty based on array length property > + * @array_len_prop (which was previously defined using > DEFINE_PROP_ARRAY()). > + */ > (some day we will have to clarify our API doc style, but not now ;) +static ArrayElementProperty *array_element_new(Object *obj, > + Property *array_len_prop, > + const char *arrayname, > + int index, > + void *eltptr) > +{ > + char *propname = g_strdup_printf("%s[%d]", arrayname, index); > + ArrayElementProperty *arrayprop = g_new0(ArrayElementProperty, 1); > + arrayprop->release = array_len_prop->arrayinfo->release; > + arrayprop->propname = propname; > + arrayprop->prop.info = array_len_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(qdev_get_prop_ptr(obj, &arrayprop->prop) == eltptr); > + return arrayprop; > +} > + > /* object property release callback for array element properties: > * we call the underlying element's property release hook, and > * then free the memory we allocated when we added the property. > @@ -602,6 +628,18 @@ static void array_element_release(Object *obj, const > char *name, void *opaque) > g_free(p); > } > > +static void object_property_add_array_element(Object *obj, > + Property *array_len_prop, > + ArrayElementProperty *prop) > +{ > + object_property_add(obj, prop->prop.name, > + prop->prop.info->name, > + static_prop_getter(prop->prop.info), > + static_prop_setter(prop->prop.info), > + array_element_release, > + prop); > +} > + > static void set_prop_arraylen(Object *obj, Visitor *v, const char *name, > void *opaque, Error **errp) > { > @@ -641,25 +679,9 @@ static void set_prop_arraylen(Object *obj, Visitor > *v, const char *name, > */ > *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(qdev_get_prop_ptr(obj, &arrayprop->prop) == eltptr); > - object_property_add(obj, propname, > - arrayprop->prop.info->name, > - static_prop_getter(arrayprop->prop.info), > - static_prop_setter(arrayprop->prop.info), > - array_element_release, > - arrayprop); > + ArrayElementProperty *elt_prop = array_element_new(obj, prop, > arrayname, > + i, eltptr); > + object_property_add_array_element(obj, prop, elt_prop); > } > } > > -- > 2.28.0 > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
On Fri, Oct 30, 2020 at 2:03 PM Marc-André Lureau < marcandre.lureau@gmail.com> wrote: > > > On Fri, Oct 30, 2020 at 2:17 AM Eduardo Habkost <ehabkost@redhat.com> > wrote: > >> The array property registration code is hard to follow. Move the >> two steps into separate functions that have clear >> responsibilities. >> >> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> >> --- >> Cc: Paolo Bonzini <pbonzini@redhat.com> >> Cc: "Daniel P. Berrangé" <berrange@redhat.com> >> Cc: Eduardo Habkost <ehabkost@redhat.com> >> Cc: qemu-devel@nongnu.org >> --- >> hw/core/qdev-properties.c | 60 ++++++++++++++++++++++++++------------- >> 1 file changed, 41 insertions(+), 19 deletions(-) >> >> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c >> index 27c09255d7..1f06dfb5d5 100644 >> --- a/hw/core/qdev-properties.c >> +++ b/hw/core/qdev-properties.c >> @@ -588,6 +588,32 @@ typedef struct { >> ObjectPropertyRelease *release; >> } ArrayElementProperty; >> >> +/** >> + * Create ArrayElementProperty based on array length property >> + * @array_len_prop (which was previously defined using >> DEFINE_PROP_ARRAY()). >> + */ >> > > (some day we will have to clarify our API doc style, but not now ;) > > Actually, I didn't realize but we do use kerneldoc in sphinx nowadays. Peter, shouldn't you have updated CODING_STYLE.rst to say explicitly that our C API should be documented with it? How do we enforce or check the comment style across the code base, or per-files (without necessarily including it in the generated manual/doc)? thanks
On Fri, Oct 30, 2020 at 02:10:26PM +0400, Marc-André Lureau wrote: > On Fri, Oct 30, 2020 at 2:03 PM Marc-André Lureau < > marcandre.lureau@gmail.com> wrote: > > > > > > > On Fri, Oct 30, 2020 at 2:17 AM Eduardo Habkost <ehabkost@redhat.com> > > wrote: > > > >> The array property registration code is hard to follow. Move the > >> two steps into separate functions that have clear > >> responsibilities. > >> > >> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > >> --- > >> Cc: Paolo Bonzini <pbonzini@redhat.com> > >> Cc: "Daniel P. Berrangé" <berrange@redhat.com> > >> Cc: Eduardo Habkost <ehabkost@redhat.com> > >> Cc: qemu-devel@nongnu.org > >> --- > >> hw/core/qdev-properties.c | 60 ++++++++++++++++++++++++++------------- > >> 1 file changed, 41 insertions(+), 19 deletions(-) > >> > >> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c > >> index 27c09255d7..1f06dfb5d5 100644 > >> --- a/hw/core/qdev-properties.c > >> +++ b/hw/core/qdev-properties.c > >> @@ -588,6 +588,32 @@ typedef struct { > >> ObjectPropertyRelease *release; > >> } ArrayElementProperty; > >> > >> +/** > >> + * Create ArrayElementProperty based on array length property > >> + * @array_len_prop (which was previously defined using > >> DEFINE_PROP_ARRAY()). > >> + */ > >> > > > > (some day we will have to clarify our API doc style, but not now ;) > > > > > Actually, I didn't realize but we do use kerneldoc in sphinx nowadays. > > Peter, shouldn't you have updated CODING_STYLE.rst to say explicitly that > our C API should be documented with it? > > How do we enforce or check the comment style across the code base, or > per-files (without necessarily including it in the generated manual/doc)? I'd say we should include it in the generated developer docs, and enforce whatever level of error checking is availble at build times. I'll happily update any API docs in code I'm subsys maintainer for, if we actually generate and validate at build time. Regards, Daniel
On Fri, Oct 30, 2020 at 02:03:07PM +0400, Marc-André Lureau wrote: > On Fri, Oct 30, 2020 at 2:17 AM Eduardo Habkost <ehabkost@redhat.com> wrote: > > > The array property registration code is hard to follow. Move the > > two steps into separate functions that have clear > > responsibilities. > > > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > > --- > > Cc: Paolo Bonzini <pbonzini@redhat.com> > > Cc: "Daniel P. Berrangé" <berrange@redhat.com> > > Cc: Eduardo Habkost <ehabkost@redhat.com> > > Cc: qemu-devel@nongnu.org > > --- > > hw/core/qdev-properties.c | 60 ++++++++++++++++++++++++++------------- > > 1 file changed, 41 insertions(+), 19 deletions(-) > > > > diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c > > index 27c09255d7..1f06dfb5d5 100644 > > --- a/hw/core/qdev-properties.c > > +++ b/hw/core/qdev-properties.c > > @@ -588,6 +588,32 @@ typedef struct { > > ObjectPropertyRelease *release; > > } ArrayElementProperty; > > > > +/** > > + * Create ArrayElementProperty based on array length property > > + * @array_len_prop (which was previously defined using > > DEFINE_PROP_ARRAY()). > > + */ > > > > (some day we will have to clarify our API doc style, but not now ;) In this specific case, this one was not supposed to be a real doc comment. My first version of this commit had a full doc comment, then I decided it was overkill for an internal static function and I made it a plain paragraph. The "/**" and "@array_len_prop" are leftovers from the old doc comment and I will remove them if respinning the series. > > +static ArrayElementProperty *array_element_new(Object *obj,
diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c index 27c09255d7..1f06dfb5d5 100644 --- a/hw/core/qdev-properties.c +++ b/hw/core/qdev-properties.c @@ -588,6 +588,32 @@ typedef struct { ObjectPropertyRelease *release; } ArrayElementProperty; +/** + * Create ArrayElementProperty based on array length property + * @array_len_prop (which was previously defined using DEFINE_PROP_ARRAY()). + */ +static ArrayElementProperty *array_element_new(Object *obj, + Property *array_len_prop, + const char *arrayname, + int index, + void *eltptr) +{ + char *propname = g_strdup_printf("%s[%d]", arrayname, index); + ArrayElementProperty *arrayprop = g_new0(ArrayElementProperty, 1); + arrayprop->release = array_len_prop->arrayinfo->release; + arrayprop->propname = propname; + arrayprop->prop.info = array_len_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(qdev_get_prop_ptr(obj, &arrayprop->prop) == eltptr); + return arrayprop; +} + /* object property release callback for array element properties: * we call the underlying element's property release hook, and * then free the memory we allocated when we added the property. @@ -602,6 +628,18 @@ static void array_element_release(Object *obj, const char *name, void *opaque) g_free(p); } +static void object_property_add_array_element(Object *obj, + Property *array_len_prop, + ArrayElementProperty *prop) +{ + object_property_add(obj, prop->prop.name, + prop->prop.info->name, + static_prop_getter(prop->prop.info), + static_prop_setter(prop->prop.info), + array_element_release, + prop); +} + static void set_prop_arraylen(Object *obj, Visitor *v, const char *name, void *opaque, Error **errp) { @@ -641,25 +679,9 @@ static void set_prop_arraylen(Object *obj, Visitor *v, const char *name, */ *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(qdev_get_prop_ptr(obj, &arrayprop->prop) == eltptr); - object_property_add(obj, propname, - arrayprop->prop.info->name, - static_prop_getter(arrayprop->prop.info), - static_prop_setter(arrayprop->prop.info), - array_element_release, - arrayprop); + ArrayElementProperty *elt_prop = array_element_new(obj, prop, arrayname, + i, eltptr); + object_property_add_array_element(obj, prop, elt_prop); } }
The array property registration code is hard to follow. Move the two steps into separate functions that have clear responsibilities. Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> --- Cc: Paolo Bonzini <pbonzini@redhat.com> Cc: "Daniel P. Berrangé" <berrange@redhat.com> Cc: Eduardo Habkost <ehabkost@redhat.com> Cc: qemu-devel@nongnu.org --- hw/core/qdev-properties.c | 60 ++++++++++++++++++++++++++------------- 1 file changed, 41 insertions(+), 19 deletions(-)