it will help to remove code duplication in places
that currently open code registration of static
type[s] and remove necessity to declare function
for type_init() to call, when only static types
need to be registered.
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
I'm going to use it for CPU types in followup patches
CC: ehabkost@redhat.com
---
include/qemu/module.h | 10 ++++++++++
include/qom/object.h | 10 ++++++++++
qom/object.c | 9 +++++++++
3 files changed, 29 insertions(+)
diff --git a/include/qemu/module.h b/include/qemu/module.h
index 56dd218..60dd632 100644
--- a/include/qemu/module.h
+++ b/include/qemu/module.h
@@ -52,6 +52,16 @@ typedef enum {
#define type_init(function) module_init(function, MODULE_INIT_QOM)
#define trace_init(function) module_init(function, MODULE_INIT_TRACE)
+#define REGISTER_STATIC_TYPES(t, nr) \
+static void do_qemu_init_ ## t(void) \
+{ \
+ type_register_static_array(t, nr); \
+} \
+type_init(do_qemu_init_ ## t)
+
+#define REGISTER_STATIC_TYPE(t) \
+ REGISTER_STATIC_TYPES(t, 1)
+
#define block_module_load_one(lib) module_load_one("block-", lib)
void register_module_init(void (*fn)(void), module_init_type type);
diff --git a/include/qom/object.h b/include/qom/object.h
index f3e5cff..17fcadd 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -789,6 +789,16 @@ Type type_register_static(const TypeInfo *info);
Type type_register(const TypeInfo *info);
/**
+ * type_register_static_array:
+ * @infos: The array of the new type #TypeInfo structures.
+ * @nr_infos: number of entries in @infos
+ *
+ * @infos and all of the strings it points to should exist for the life time
+ * that the type is registered.
+ */
+void type_register_static_array(const TypeInfo *infos, int nr_infos);
+
+/**
* object_class_dynamic_cast_assert:
* @klass: The #ObjectClass to attempt to cast.
* @typename: The QOM typename of the class to cast to.
diff --git a/qom/object.c b/qom/object.c
index 3e18537..40b1729 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -151,6 +151,15 @@ TypeImpl *type_register_static(const TypeInfo *info)
return type_register(info);
}
+void type_register_static_array(const TypeInfo *infos, int nr_infos)
+{
+ int i;
+
+ for (i = 0; i < nr_infos; i++) {
+ assert(type_register_static(&infos[i]));
+ }
+}
+
static TypeImpl *type_get_by_name(const char *name)
{
if (name == NULL) {
--
2.7.4
Hi Igor,
On 10/03/2017 09:14 AM, Igor Mammedov wrote:
> it will help to remove code duplication in places
> that currently open code registration of static
> type[s] and remove necessity to declare function
> for type_init() to call, when only static types
> need to be registered.
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> I'm going to use it for CPU types in followup patches
>
> CC: ehabkost@redhat.com
> ---
> include/qemu/module.h | 10 ++++++++++
> include/qom/object.h | 10 ++++++++++
> qom/object.c | 9 +++++++++
> 3 files changed, 29 insertions(+)
>
> diff --git a/include/qemu/module.h b/include/qemu/module.h
> index 56dd218..60dd632 100644
> --- a/include/qemu/module.h
> +++ b/include/qemu/module.h
> @@ -52,6 +52,16 @@ typedef enum {
> #define type_init(function) module_init(function, MODULE_INIT_QOM)
> #define trace_init(function) module_init(function, MODULE_INIT_TRACE)
>
> +#define REGISTER_STATIC_TYPES(t, nr) \
ok
> +static void do_qemu_init_ ## t(void) \
> +{ \
> + type_register_static_array(t, nr); \
> +} \
> +type_init(do_qemu_init_ ## t)
> +
> +#define REGISTER_STATIC_TYPE(t) \
eh why not...
What's your plan for your "generalize parsing of cpu_model (2)" series?
re-post the whole? If you just change this macro you can keep my R-b for it.
> + REGISTER_STATIC_TYPES(t, 1)
> +
> #define block_module_load_one(lib) module_load_one("block-", lib)
>
> void register_module_init(void (*fn)(void), module_init_type type);
> diff --git a/include/qom/object.h b/include/qom/object.h
> index f3e5cff..17fcadd 100644
> --- a/include/qom/object.h
> +++ b/include/qom/object.h
> @@ -789,6 +789,16 @@ Type type_register_static(const TypeInfo *info);
> Type type_register(const TypeInfo *info);
>
> /**
> + * type_register_static_array:
> + * @infos: The array of the new type #TypeInfo structures.
> + * @nr_infos: number of entries in @infos
> + *
> + * @infos and all of the strings it points to should exist for the life time
> + * that the type is registered.
> + */
> +void type_register_static_array(const TypeInfo *infos, int nr_infos);
> +
> +/**
> * object_class_dynamic_cast_assert:
> * @klass: The #ObjectClass to attempt to cast.
> * @typename: The QOM typename of the class to cast to.
> diff --git a/qom/object.c b/qom/object.c
> index 3e18537..40b1729 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -151,6 +151,15 @@ TypeImpl *type_register_static(const TypeInfo *info)
> return type_register(info);
> }
>
> +void type_register_static_array(const TypeInfo *infos, int nr_infos)
> +{
> + int i;
> +
> + for (i = 0; i < nr_infos; i++) {
> + assert(type_register_static(&infos[i]));
> + }
> +}
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> +
> static TypeImpl *type_get_by_name(const char *name)
> {
> if (name == NULL) {
>
On Tue, 3 Oct 2017 10:29:22 -0300
Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Hi Igor,
>
> On 10/03/2017 09:14 AM, Igor Mammedov wrote:
> > it will help to remove code duplication in places
> > that currently open code registration of static
> > type[s] and remove necessity to declare function
> > for type_init() to call, when only static types
> > need to be registered.
> >
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> > I'm going to use it for CPU types in followup patches
> >
> > CC: ehabkost@redhat.com
> > ---
> > include/qemu/module.h | 10 ++++++++++
> > include/qom/object.h | 10 ++++++++++
> > qom/object.c | 9 +++++++++
> > 3 files changed, 29 insertions(+)
> >
> > diff --git a/include/qemu/module.h b/include/qemu/module.h
> > index 56dd218..60dd632 100644
> > --- a/include/qemu/module.h
> > +++ b/include/qemu/module.h
> > @@ -52,6 +52,16 @@ typedef enum {
> > #define type_init(function) module_init(function, MODULE_INIT_QOM)
> > #define trace_init(function) module_init(function, MODULE_INIT_TRACE)
> >
> > +#define REGISTER_STATIC_TYPES(t, nr) \
>
> ok
>
> > +static void do_qemu_init_ ## t(void) \
> > +{ \
> > + type_register_static_array(t, nr); \
> > +} \
> > +type_init(do_qemu_init_ ## t)
> > +
> > +#define REGISTER_STATIC_TYPE(t) \
>
> eh why not...
>
> What's your plan for your "generalize parsing of cpu_model (2)" series?
> re-post the whole? If you just change this macro you can keep my R-b for it.
It looks like I'll have to amend and repost series.
I'd prefer to keep type_init_from_array() patch for now as it does what's need
and in line with current type_init().
But it seems Eduardo doesn't like it and want's a more generalized way
to handle typeinfo array or single typeinfo.
So most likely I'll end-up keeping *_cpu_register_types() functions and call
type_register_static_array() from there replacing only loops.
>
> > + REGISTER_STATIC_TYPES(t, 1)
> > +
> > #define block_module_load_one(lib) module_load_one("block-", lib)
> >
> > void register_module_init(void (*fn)(void), module_init_type type);
> > diff --git a/include/qom/object.h b/include/qom/object.h
> > index f3e5cff..17fcadd 100644
> > --- a/include/qom/object.h
> > +++ b/include/qom/object.h
> > @@ -789,6 +789,16 @@ Type type_register_static(const TypeInfo *info);
> > Type type_register(const TypeInfo *info);
> >
> > /**
> > + * type_register_static_array:
> > + * @infos: The array of the new type #TypeInfo structures.
> > + * @nr_infos: number of entries in @infos
> > + *
> > + * @infos and all of the strings it points to should exist for the life time
> > + * that the type is registered.
> > + */
> > +void type_register_static_array(const TypeInfo *infos, int nr_infos);
> > +
> > +/**
> > * object_class_dynamic_cast_assert:
> > * @klass: The #ObjectClass to attempt to cast.
> > * @typename: The QOM typename of the class to cast to.
> > diff --git a/qom/object.c b/qom/object.c
> > index 3e18537..40b1729 100644
> > --- a/qom/object.c
> > +++ b/qom/object.c
> > @@ -151,6 +151,15 @@ TypeImpl *type_register_static(const TypeInfo *info)
> > return type_register(info);
> > }
> >
> > +void type_register_static_array(const TypeInfo *infos, int nr_infos)
> > +{
> > + int i;
> > +
> > + for (i = 0; i < nr_infos; i++) {
> > + assert(type_register_static(&infos[i]));
> > + }
> > +}
>
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>
> > +
> > static TypeImpl *type_get_by_name(const char *name)
> > {
> > if (name == NULL) {
> >
>
On Tue, Oct 03, 2017 at 02:14:46PM +0200, Igor Mammedov wrote:
> it will help to remove code duplication in places
> that currently open code registration of static
> type[s] and remove necessity to declare function
> for type_init() to call, when only static types
> need to be registered.
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> I'm going to use it for CPU types in followup patches
>
> CC: ehabkost@redhat.com
> ---
> include/qemu/module.h | 10 ++++++++++
> include/qom/object.h | 10 ++++++++++
> qom/object.c | 9 +++++++++
> 3 files changed, 29 insertions(+)
>
> diff --git a/include/qemu/module.h b/include/qemu/module.h
> index 56dd218..60dd632 100644
> --- a/include/qemu/module.h
> +++ b/include/qemu/module.h
> @@ -52,6 +52,16 @@ typedef enum {
> #define type_init(function) module_init(function, MODULE_INIT_QOM)
> #define trace_init(function) module_init(function, MODULE_INIT_TRACE)
>
> +#define REGISTER_STATIC_TYPES(t, nr) \
I'm unsure about the name. We already have a declarative way to
register machine types (DEFINE_MACHINE), maybe use "DEFINE_*" for
consistency?
Also, this function won't work if t isn't a static variable, will
it? The "STATIC" part looks redundant.
In other words, how about "DEFINE_TYPE" and "DEFINE_TYPES"?
> +static void do_qemu_init_ ## t(void) \
> +{ \
> + type_register_static_array(t, nr); \
qemu/module.h doesn't include qom/object.h. I think this should
go to either qom/object.h or a new header file.
> +} \
> +type_init(do_qemu_init_ ## t)
> +
> +#define REGISTER_STATIC_TYPE(t) \
> + REGISTER_STATIC_TYPES(t, 1)
> +
> #define block_module_load_one(lib) module_load_one("block-", lib)
>
> void register_module_init(void (*fn)(void), module_init_type type);
> diff --git a/include/qom/object.h b/include/qom/object.h
> index f3e5cff..17fcadd 100644
> --- a/include/qom/object.h
> +++ b/include/qom/object.h
> @@ -789,6 +789,16 @@ Type type_register_static(const TypeInfo *info);
> Type type_register(const TypeInfo *info);
>
> /**
> + * type_register_static_array:
> + * @infos: The array of the new type #TypeInfo structures.
> + * @nr_infos: number of entries in @infos
> + *
> + * @infos and all of the strings it points to should exist for the life time
> + * that the type is registered.
> + */
> +void type_register_static_array(const TypeInfo *infos, int nr_infos);
This new function is probably less controversial than the new
module.h-based code. Maybe worth moving to a separate patch so
it can be merged earlier?
(If I were you, I would just use type_register_static_array()
inside the existing *_cpu_register_types() functions by now, so
this won't block the whole series, and worry about
REGISTER_STATIC_TYPE*/DEFINE_TYPE* later.)
> +
> +/**
> * object_class_dynamic_cast_assert:
> * @klass: The #ObjectClass to attempt to cast.
> * @typename: The QOM typename of the class to cast to.
> diff --git a/qom/object.c b/qom/object.c
> index 3e18537..40b1729 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -151,6 +151,15 @@ TypeImpl *type_register_static(const TypeInfo *info)
> return type_register(info);
> }
>
> +void type_register_static_array(const TypeInfo *infos, int nr_infos)
> +{
> + int i;
> +
> + for (i = 0; i < nr_infos; i++) {
> + assert(type_register_static(&infos[i]));
Why the assert()? type_register_static() is already guaranteed
to never return NULL.
This will break if compiled with -DNDEBUG: assert() won't even
evaluate the expression if NDEBUG is defined.
> + }
> +}
> +
> static TypeImpl *type_get_by_name(const char *name)
> {
> if (name == NULL) {
> --
> 2.7.4
>
--
Eduardo
On Tue, 3 Oct 2017 11:25:01 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:
> On Tue, Oct 03, 2017 at 02:14:46PM +0200, Igor Mammedov wrote:
> > it will help to remove code duplication in places
> > that currently open code registration of static
> > type[s] and remove necessity to declare function
> > for type_init() to call, when only static types
> > need to be registered.
> >
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> > I'm going to use it for CPU types in followup patches
> >
> > CC: ehabkost@redhat.com
> > ---
> > include/qemu/module.h | 10 ++++++++++
> > include/qom/object.h | 10 ++++++++++
> > qom/object.c | 9 +++++++++
> > 3 files changed, 29 insertions(+)
> >
> > diff --git a/include/qemu/module.h b/include/qemu/module.h
> > index 56dd218..60dd632 100644
> > --- a/include/qemu/module.h
> > +++ b/include/qemu/module.h
> > @@ -52,6 +52,16 @@ typedef enum {
> > #define type_init(function) module_init(function, MODULE_INIT_QOM)
> > #define trace_init(function) module_init(function, MODULE_INIT_TRACE)
> >
> > +#define REGISTER_STATIC_TYPES(t, nr) \
>
> I'm unsure about the name. We already have a declarative way to
> register machine types (DEFINE_MACHINE), maybe use "DEFINE_*" for
> consistency?
>
> Also, this function won't work if t isn't a static variable, will
> it? The "STATIC" part looks redundant.
>
> In other words, how about "DEFINE_TYPE" and "DEFINE_TYPES"?
'define' word remind me more of declaration but here we actually
register types that's why 'register' looks better to me as it says
exactly what's going here.
>
>
> > +static void do_qemu_init_ ## t(void) \
> > +{ \
> > + type_register_static_array(t, nr); \
>
> qemu/module.h doesn't include qom/object.h. I think this should
> go to either qom/object.h or a new header file.
I could move it to qom/object.h
>
> > +} \
> > +type_init(do_qemu_init_ ## t)
> > +
> > +#define REGISTER_STATIC_TYPE(t) \
> > + REGISTER_STATIC_TYPES(t, 1)
> > +
> > #define block_module_load_one(lib) module_load_one("block-", lib)
> >
> > void register_module_init(void (*fn)(void), module_init_type type);
> > diff --git a/include/qom/object.h b/include/qom/object.h
> > index f3e5cff..17fcadd 100644
> > --- a/include/qom/object.h
> > +++ b/include/qom/object.h
> > @@ -789,6 +789,16 @@ Type type_register_static(const TypeInfo *info);
> > Type type_register(const TypeInfo *info);
> >
> > /**
> > + * type_register_static_array:
> > + * @infos: The array of the new type #TypeInfo structures.
> > + * @nr_infos: number of entries in @infos
> > + *
> > + * @infos and all of the strings it points to should exist for the life time
> > + * that the type is registered.
> > + */
> > +void type_register_static_array(const TypeInfo *infos, int nr_infos);
>
> This new function is probably less controversial than the new
> module.h-based code. Maybe worth moving to a separate patch so
> it can be merged earlier?
>
> (If I were you, I would just use type_register_static_array()
> inside the existing *_cpu_register_types() functions by now, so
> this won't block the whole series, and worry about
> REGISTER_STATIC_TYPE*/DEFINE_TYPE* later.)
it would be nice to have both in one series so that
we won't have to touch/rewrite more than once code where
static cpu types are registered.
If you prefer I can do what you suggest and keep
*_cpu_register_types() functions
but I can't promise to return back to cleaning them up
or to REGISTER_STATIC_TYPE*/DEFINE_TYPE, so probably
someone else will have to pickup that work.
> > +
> > +/**
> > * object_class_dynamic_cast_assert:
> > * @klass: The #ObjectClass to attempt to cast.
> > * @typename: The QOM typename of the class to cast to.
> > diff --git a/qom/object.c b/qom/object.c
> > index 3e18537..40b1729 100644
> > --- a/qom/object.c
> > +++ b/qom/object.c
> > @@ -151,6 +151,15 @@ TypeImpl *type_register_static(const TypeInfo *info)
> > return type_register(info);
> > }
> >
> > +void type_register_static_array(const TypeInfo *infos, int nr_infos)
> > +{
> > + int i;
> > +
> > + for (i = 0; i < nr_infos; i++) {
> > + assert(type_register_static(&infos[i]));
>
> Why the assert()? type_register_static() is already guaranteed
> to never return NULL.
according to doc comment type_register_static might return 0 on failure
so qemu should die in that case.
>
> This will break if compiled with -DNDEBUG: assert() won't even
> evaluate the expression if NDEBUG is defined.
>
> > + }
> > +}
> > +
> > static TypeImpl *type_get_by_name(const char *name)
> > {
> > if (name == NULL) {
> > --
> > 2.7.4
> >
>
On Tue, Oct 03, 2017 at 05:01:39PM +0200, Igor Mammedov wrote:
> On Tue, 3 Oct 2017 11:25:01 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
>
> > On Tue, Oct 03, 2017 at 02:14:46PM +0200, Igor Mammedov wrote:
> > > it will help to remove code duplication in places
> > > that currently open code registration of static
> > > type[s] and remove necessity to declare function
> > > for type_init() to call, when only static types
> > > need to be registered.
> > >
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > ---
> > > I'm going to use it for CPU types in followup patches
> > >
> > > CC: ehabkost@redhat.com
> > > ---
> > > include/qemu/module.h | 10 ++++++++++
> > > include/qom/object.h | 10 ++++++++++
> > > qom/object.c | 9 +++++++++
> > > 3 files changed, 29 insertions(+)
> > >
> > > diff --git a/include/qemu/module.h b/include/qemu/module.h
> > > index 56dd218..60dd632 100644
> > > --- a/include/qemu/module.h
> > > +++ b/include/qemu/module.h
> > > @@ -52,6 +52,16 @@ typedef enum {
> > > #define type_init(function) module_init(function, MODULE_INIT_QOM)
> > > #define trace_init(function) module_init(function, MODULE_INIT_TRACE)
> > >
> > > +#define REGISTER_STATIC_TYPES(t, nr) \
> >
> > I'm unsure about the name. We already have a declarative way to
> > register machine types (DEFINE_MACHINE), maybe use "DEFINE_*" for
> > consistency?
> >
> > Also, this function won't work if t isn't a static variable, will
> > it? The "STATIC" part looks redundant.
> >
> > In other words, how about "DEFINE_TYPE" and "DEFINE_TYPES"?
> 'define' word remind me more of declaration but here we actually
> register types that's why 'register' looks better to me as it says
> exactly what's going here.
Making it look like a declaration and not an action like
"register" is on purpose, as it's not a function call anymore:
it's a declaration that will make the type get registered
automatically.
>
> >
> >
> > > +static void do_qemu_init_ ## t(void) \
> > > +{ \
> > > + type_register_static_array(t, nr); \
> >
> > qemu/module.h doesn't include qom/object.h. I think this should
> > go to either qom/object.h or a new header file.
> I could move it to qom/object.h
>
> >
> > > +} \
> > > +type_init(do_qemu_init_ ## t)
> > > +
> > > +#define REGISTER_STATIC_TYPE(t) \
> > > + REGISTER_STATIC_TYPES(t, 1)
> > > +
> > > #define block_module_load_one(lib) module_load_one("block-", lib)
> > >
> > > void register_module_init(void (*fn)(void), module_init_type type);
> > > diff --git a/include/qom/object.h b/include/qom/object.h
> > > index f3e5cff..17fcadd 100644
> > > --- a/include/qom/object.h
> > > +++ b/include/qom/object.h
> > > @@ -789,6 +789,16 @@ Type type_register_static(const TypeInfo *info);
> > > Type type_register(const TypeInfo *info);
> > >
> > > /**
> > > + * type_register_static_array:
> > > + * @infos: The array of the new type #TypeInfo structures.
> > > + * @nr_infos: number of entries in @infos
> > > + *
> > > + * @infos and all of the strings it points to should exist for the life time
> > > + * that the type is registered.
> > > + */
> > > +void type_register_static_array(const TypeInfo *infos, int nr_infos);
> >
> > This new function is probably less controversial than the new
> > module.h-based code. Maybe worth moving to a separate patch so
> > it can be merged earlier?
> >
> > (If I were you, I would just use type_register_static_array()
> > inside the existing *_cpu_register_types() functions by now, so
> > this won't block the whole series, and worry about
> > REGISTER_STATIC_TYPE*/DEFINE_TYPE* later.)
> it would be nice to have both in one series so that
> we won't have to touch/rewrite more than once code where
> static cpu types are registered.
>
> If you prefer I can do what you suggest and keep
> *_cpu_register_types() functions
> but I can't promise to return back to cleaning them up
> or to REGISTER_STATIC_TYPE*/DEFINE_TYPE, so probably
> someone else will have to pickup that work.
No problem to me, if this is left for later. The macro saves
only 3 lines of code.
I think REGISTER_STATIC_TYPE*/DEFINE_TYPE* is a good idea, but
considering that QOM types are not the only kind of entity we
register at module_init(), we need to pick a style that can be
consistently used by other subsystems (e.g. for QemuOpts,
BlockDrivers, and TraceEvents). This means this could take a
little longer to be discussed, and I guess you wouldn't like the
discussion to block the cleanups in this series.
>
> > > +
> > > +/**
> > > * object_class_dynamic_cast_assert:
> > > * @klass: The #ObjectClass to attempt to cast.
> > > * @typename: The QOM typename of the class to cast to.
> > > diff --git a/qom/object.c b/qom/object.c
> > > index 3e18537..40b1729 100644
> > > --- a/qom/object.c
> > > +++ b/qom/object.c
> > > @@ -151,6 +151,15 @@ TypeImpl *type_register_static(const TypeInfo *info)
> > > return type_register(info);
> > > }
> > >
> > > +void type_register_static_array(const TypeInfo *infos, int nr_infos)
> > > +{
> > > + int i;
> > > +
> > > + for (i = 0; i < nr_infos; i++) {
> > > + assert(type_register_static(&infos[i]));
> >
> > Why the assert()? type_register_static() is already guaranteed
> > to never return NULL.
> according to doc comment type_register_static might return 0 on failure
> so qemu should die in that case.
We have absolutely no callers of type_register*() that check its
return value, and the existing implementation guarantees it will
never return NULL. Instead of making
type_register_static_array() more commplex, we could just update
the documentation.
>
> >
> > This will break if compiled with -DNDEBUG: assert() won't even
> > evaluate the expression if NDEBUG is defined.
> >
> > > + }
> > > +}
> > > +
> > > static TypeImpl *type_get_by_name(const char *name)
> > > {
> > > if (name == NULL) {
> > > --
> > > 2.7.4
> > >
> >
>
--
Eduardo
On Tue, 3 Oct 2017 12:37:22 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:
> On Tue, Oct 03, 2017 at 05:01:39PM +0200, Igor Mammedov wrote:
> > On Tue, 3 Oct 2017 11:25:01 -0300
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> >
> > > On Tue, Oct 03, 2017 at 02:14:46PM +0200, Igor Mammedov wrote:
> > > > it will help to remove code duplication in places
> > > > that currently open code registration of static
> > > > type[s] and remove necessity to declare function
> > > > for type_init() to call, when only static types
> > > > need to be registered.
> > > >
> > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > > ---
> > > > I'm going to use it for CPU types in followup patches
> > > >
> > > > CC: ehabkost@redhat.com
> > > > ---
> > > > include/qemu/module.h | 10 ++++++++++
> > > > include/qom/object.h | 10 ++++++++++
> > > > qom/object.c | 9 +++++++++
> > > > 3 files changed, 29 insertions(+)
> > > >
> > > > diff --git a/include/qemu/module.h b/include/qemu/module.h
> > > > index 56dd218..60dd632 100644
> > > > --- a/include/qemu/module.h
> > > > +++ b/include/qemu/module.h
> > > > @@ -52,6 +52,16 @@ typedef enum {
> > > > #define type_init(function) module_init(function, MODULE_INIT_QOM)
> > > > #define trace_init(function) module_init(function, MODULE_INIT_TRACE)
> > > >
> > > > +#define REGISTER_STATIC_TYPES(t, nr) \
> > >
> > > I'm unsure about the name. We already have a declarative way to
> > > register machine types (DEFINE_MACHINE), maybe use "DEFINE_*" for
> > > consistency?
> > >
> > > Also, this function won't work if t isn't a static variable, will
> > > it? The "STATIC" part looks redundant.
> > >
> > > In other words, how about "DEFINE_TYPE" and "DEFINE_TYPES"?
> > 'define' word remind me more of declaration but here we actually
> > register types that's why 'register' looks better to me as it says
> > exactly what's going here.
>
> Making it look like a declaration and not an action like
> "register" is on purpose, as it's not a function call anymore:
> it's a declaration that will make the type get registered
> automatically.
so if there is no objections I'd respin patch with
"DEFINE_TYPE" and "DEFINE_TYPES"
that Eduardo suggested and put these into object.h instead of module.h
I also would prefer to drop/postpone "DEFINE_TYPE" till someone takes
on a quest to actually use it.
>
> >
> > >
> > >
> > > > +static void do_qemu_init_ ## t(void) \
> > > > +{ \
> > > > + type_register_static_array(t, nr); \
> > >
> > > qemu/module.h doesn't include qom/object.h. I think this should
> > > go to either qom/object.h or a new header file.
> > I could move it to qom/object.h
> >
> > >
> > > > +} \
> > > > +type_init(do_qemu_init_ ## t)
> > > > +
> > > > +#define REGISTER_STATIC_TYPE(t) \
> > > > + REGISTER_STATIC_TYPES(t, 1)
> > > > +
> > > > #define block_module_load_one(lib) module_load_one("block-", lib)
> > > >
> > > > void register_module_init(void (*fn)(void), module_init_type type);
> > > > diff --git a/include/qom/object.h b/include/qom/object.h
> > > > index f3e5cff..17fcadd 100644
> > > > --- a/include/qom/object.h
> > > > +++ b/include/qom/object.h
> > > > @@ -789,6 +789,16 @@ Type type_register_static(const TypeInfo *info);
> > > > Type type_register(const TypeInfo *info);
> > > >
> > > > /**
> > > > + * type_register_static_array:
> > > > + * @infos: The array of the new type #TypeInfo structures.
> > > > + * @nr_infos: number of entries in @infos
> > > > + *
> > > > + * @infos and all of the strings it points to should exist for the life time
> > > > + * that the type is registered.
> > > > + */
> > > > +void type_register_static_array(const TypeInfo *infos, int nr_infos);
> > >
> > > This new function is probably less controversial than the new
> > > module.h-based code. Maybe worth moving to a separate patch so
> > > it can be merged earlier?
> > >
> > > (If I were you, I would just use type_register_static_array()
> > > inside the existing *_cpu_register_types() functions by now, so
> > > this won't block the whole series, and worry about
> > > REGISTER_STATIC_TYPE*/DEFINE_TYPE* later.)
> > it would be nice to have both in one series so that
> > we won't have to touch/rewrite more than once code where
> > static cpu types are registered.
> >
> > If you prefer I can do what you suggest and keep
> > *_cpu_register_types() functions
> > but I can't promise to return back to cleaning them up
> > or to REGISTER_STATIC_TYPE*/DEFINE_TYPE, so probably
> > someone else will have to pickup that work.
>
> No problem to me, if this is left for later. The macro saves
> only 3 lines of code.
>
> I think REGISTER_STATIC_TYPE*/DEFINE_TYPE* is a good idea, but
> considering that QOM types are not the only kind of entity we
> register at module_init(), we need to pick a style that can be
> consistently used by other subsystems (e.g. for QemuOpts,
> BlockDrivers, and TraceEvents). This means this could take a
> little longer to be discussed, and I guess you wouldn't like the
> discussion to block the cleanups in this series.
>
> >
> > > > +
> > > > +/**
> > > > * object_class_dynamic_cast_assert:
> > > > * @klass: The #ObjectClass to attempt to cast.
> > > > * @typename: The QOM typename of the class to cast to.
> > > > diff --git a/qom/object.c b/qom/object.c
> > > > index 3e18537..40b1729 100644
> > > > --- a/qom/object.c
> > > > +++ b/qom/object.c
> > > > @@ -151,6 +151,15 @@ TypeImpl *type_register_static(const TypeInfo *info)
> > > > return type_register(info);
> > > > }
> > > >
> > > > +void type_register_static_array(const TypeInfo *infos, int nr_infos)
> > > > +{
> > > > + int i;
> > > > +
> > > > + for (i = 0; i < nr_infos; i++) {
> > > > + assert(type_register_static(&infos[i]));
> > >
> > > Why the assert()? type_register_static() is already guaranteed
> > > to never return NULL.
> > according to doc comment type_register_static might return 0 on failure
> > so qemu should die in that case.
>
> We have absolutely no callers of type_register*() that check its
> return value, and the existing implementation guarantees it will
> never return NULL. Instead of making
> type_register_static_array() more commplex, we could just update
> the documentation.
>
> >
> > >
> > > This will break if compiled with -DNDEBUG: assert() won't even
> > > evaluate the expression if NDEBUG is defined.
> > >
> > > > + }
> > > > +}
> > > > +
> > > > static TypeImpl *type_get_by_name(const char *name)
> > > > {
> > > > if (name == NULL) {
> > > > --
> > > > 2.7.4
> > > >
> > >
> >
>
On Tue, Oct 03, 2017 at 06:37:37PM +0200, Igor Mammedov wrote:
> On Tue, 3 Oct 2017 12:37:22 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
>
> > On Tue, Oct 03, 2017 at 05:01:39PM +0200, Igor Mammedov wrote:
> > > On Tue, 3 Oct 2017 11:25:01 -0300
> > > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > >
> > > > On Tue, Oct 03, 2017 at 02:14:46PM +0200, Igor Mammedov wrote:
> > > > > it will help to remove code duplication in places
> > > > > that currently open code registration of static
> > > > > type[s] and remove necessity to declare function
> > > > > for type_init() to call, when only static types
> > > > > need to be registered.
> > > > >
> > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > > > ---
> > > > > I'm going to use it for CPU types in followup patches
> > > > >
> > > > > CC: ehabkost@redhat.com
> > > > > ---
> > > > > include/qemu/module.h | 10 ++++++++++
> > > > > include/qom/object.h | 10 ++++++++++
> > > > > qom/object.c | 9 +++++++++
> > > > > 3 files changed, 29 insertions(+)
> > > > >
> > > > > diff --git a/include/qemu/module.h b/include/qemu/module.h
> > > > > index 56dd218..60dd632 100644
> > > > > --- a/include/qemu/module.h
> > > > > +++ b/include/qemu/module.h
> > > > > @@ -52,6 +52,16 @@ typedef enum {
> > > > > #define type_init(function) module_init(function, MODULE_INIT_QOM)
> > > > > #define trace_init(function) module_init(function, MODULE_INIT_TRACE)
> > > > >
> > > > > +#define REGISTER_STATIC_TYPES(t, nr) \
> > > >
> > > > I'm unsure about the name. We already have a declarative way to
> > > > register machine types (DEFINE_MACHINE), maybe use "DEFINE_*" for
> > > > consistency?
> > > >
> > > > Also, this function won't work if t isn't a static variable, will
> > > > it? The "STATIC" part looks redundant.
> > > >
> > > > In other words, how about "DEFINE_TYPE" and "DEFINE_TYPES"?
> > > 'define' word remind me more of declaration but here we actually
> > > register types that's why 'register' looks better to me as it says
> > > exactly what's going here.
> >
> > Making it look like a declaration and not an action like
> > "register" is on purpose, as it's not a function call anymore:
> > it's a declaration that will make the type get registered
> > automatically.
>
> so if there is no objections I'd respin patch with
> "DEFINE_TYPE" and "DEFINE_TYPES"
> that Eduardo suggested and put these into object.h instead of module.h
>
> I also would prefer to drop/postpone "DEFINE_TYPE" till someone takes
> on a quest to actually use it.
No problem to me. Missing type_register_static_array() was a
problem, but missing DEFINE_TYPE is not a big deal.
--
Eduardo
On 3 October 2017 at 13:14, Igor Mammedov <imammedo@redhat.com> wrote:
> it will help to remove code duplication in places
> that currently open code registration of static
> type[s] and remove necessity to declare function
> for type_init() to call, when only static types
> need to be registered.
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> I'm going to use it for CPU types in followup patches
>
> CC: ehabkost@redhat.com
> ---
> include/qemu/module.h | 10 ++++++++++
> include/qom/object.h | 10 ++++++++++
> qom/object.c | 9 +++++++++
> 3 files changed, 29 insertions(+)
>
> diff --git a/include/qemu/module.h b/include/qemu/module.h
> index 56dd218..60dd632 100644
> --- a/include/qemu/module.h
> +++ b/include/qemu/module.h
> @@ -52,6 +52,16 @@ typedef enum {
> #define type_init(function) module_init(function, MODULE_INIT_QOM)
> #define trace_init(function) module_init(function, MODULE_INIT_TRACE)
>
> +#define REGISTER_STATIC_TYPES(t, nr) \
> +static void do_qemu_init_ ## t(void) \
> +{ \
> + type_register_static_array(t, nr); \
> +} \
> +type_init(do_qemu_init_ ## t)
Could we use
type_register_static_array(t, ARRAY_SIZE(t));
here to save the caller having to pass the array size,
or is that too magic?
thanks
-- PMM
On Tue, 3 Oct 2017 16:44:28 +0100
Peter Maydell <peter.maydell@linaro.org> wrote:
> On 3 October 2017 at 13:14, Igor Mammedov <imammedo@redhat.com> wrote:
> > it will help to remove code duplication in places
> > that currently open code registration of static
> > type[s] and remove necessity to declare function
> > for type_init() to call, when only static types
> > need to be registered.
> >
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> > I'm going to use it for CPU types in followup patches
> >
> > CC: ehabkost@redhat.com
> > ---
> > include/qemu/module.h | 10 ++++++++++
> > include/qom/object.h | 10 ++++++++++
> > qom/object.c | 9 +++++++++
> > 3 files changed, 29 insertions(+)
> >
> > diff --git a/include/qemu/module.h b/include/qemu/module.h
> > index 56dd218..60dd632 100644
> > --- a/include/qemu/module.h
> > +++ b/include/qemu/module.h
> > @@ -52,6 +52,16 @@ typedef enum {
> > #define type_init(function) module_init(function, MODULE_INIT_QOM)
> > #define trace_init(function) module_init(function, MODULE_INIT_TRACE)
> >
> > +#define REGISTER_STATIC_TYPES(t, nr) \
> > +static void do_qemu_init_ ## t(void) \
> > +{ \
> > + type_register_static_array(t, nr); \
> > +} \
> > +type_init(do_qemu_init_ ## t)
>
> Could we use
> type_register_static_array(t, ARRAY_SIZE(t));
> here to save the caller having to pass the array size,
> or is that too magic?
+1
this way user will not have to type ARRAY_SIZE(t) and make mistake by chance
© 2016 - 2026 Red Hat, Inc.