Re: [Qemu-devel] [PATCH 01/38] qom: add helper type_init_from_array()

[Igor Mammedov]

On Mon, 2 Oct 2017 16:24:04 -0300
Eduardo Habkost <address@hidden> wrote:

> On Mon, Oct 02, 2017 at 11:07:43AM +0200, Igor Mammedov wrote:
> > it will help to remove code duplication in places
> > that currently open code registration of several
> > types.
> > 
> > Signed-off-by: Igor Mammedov <address@hidden>
> > ---
> > I'm going to use it for CPU types in followup patches
> > 
> > CC: address@hidden
> > ---
> >  include/qemu/module.h | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 
> > diff --git a/include/qemu/module.h b/include/qemu/module.h
> > index 56dd218..29f9089 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 type_init_from_array(array)                                        
> > \  
> 
> So we're moving from a imperative way to register types to a
> declarative way.  Sounds nice.
> 
> You are also adding a way to register a TypeInfo array easily.
> That's nice, too.
> 
> But, why do we need to address both at the same time?  I think
> this adds some confusing gaps to the module/QOM APIs:
> 
> * If you want to register a type at runtime, there are only
>   functions to register a single type (type_register(),
>   type_register_static().  What if I want to register a TypeInfo
>   array from an existing type_init() function?
> * If you want to declare types to be registered automatically at
>   module_call_init(MODULE_INIT_QOM), you are adding a helper that
>   avoids manual type_register*() calls, but only for arrays.
>   What if I want to register a single type?  (There are 440+
>   functions containing a single type_register*() call in the
>   tree)
just array is sufficient for code touched in this series.
I'll add helper for single type as well, but I'd leave for someone
else to use it (i.e. do cleanup).

> I have two suggestions:
> 
> * Adding a type_register_static_array() helper first, call it
>   from a type_init() function, and think about a module.h-based
>   helper for arrays later.
> 
> * (For later) adding a module_init() helper that works for
>   registering a single type too. So, this very common pattern:
> 
>       static const TypeInfo FOO_type = {
>           /* ... */
>       };
>       
>       static void register_FOO_type(void)
>       {
>           type_register_static(&FOO_type);
>       }
>       
>       QOM_TYPE(register_FOO_type);
> 
>   could be written as:
> 
>       static const TypeInfo FOO_type = {
>           /* ... */
>       };
>       
>       QOM_TYPE(FOO_type);
> 
> (I'm suggesting QOM_TYPE as the macro name, but I'm not sure
> about it.  But I think type_init_*() is confusing because it
> doesn't work like the other *_init() helpers in module.h)
> 
> QOM_TYPE(T) could be just a shortcut to:
> QOM_TYPES_PTR(&T, 1).
QOM here seems redundant, how about
REGISTER_STATIC_TYPE/REGISTER_STATIC_TYPES

> 
> Your type_init_from_array(A) helper could be just a shortcut to:
> QOM_TYPES_PTR(A, ARRAY_SIZE(A)).
> 
> > +static void do_qemu_init_ ## array(void)                                   
> > \
> > +{                                                                          
> > \
> > +    int i;                                                                 
> > \
> > +    for (i = 0; i < ARRAY_SIZE(array); i++) {                              
> > \
> > +        type_register_static(&array[i]);                                   
> > \  
> 
> type_register_static() is in qom/object.[ch], and module.[ch] has
> no dependency on QOM at all.  I believe this belongs in
> qom/object.h, not module.h.
ok

> 
> > +    }                                                                      
> > \
> > +}                                                                          
> > \
> > +module_init(do_qemu_init_ ## array, MODULE_INIT_QOM)  
> 
> Why not use type_init(do_qemu_init_ ## array)?
I'll will use it in v2

> 
> > +  
> 
> Also for later: this is not the first time we generate function
> bodies on the fly for module_init().  Maybe module_init could
> support a void* data argument?
> 
> >  #define block_module_load_one(lib) module_load_one("block-", lib)
> >  
> >  void register_module_init(void (*fn)(void), module_init_type type);
> > -- 
> > 2.7.4
> >   
>