Re: [PATCH v2 1/1] hw/s390x: modularize virtio-gpu-ccw

[Halil Pasic <pasic@linux.ibm.com>]

On Wed, 24 Feb 2021 12:36:17 +0100
Gerd Hoffmann <kraxel@redhat.com> wrote:

> >  static void virtio_ccw_gpu_register(void)
> >  {
> > +#ifdef CONFIG_MODULES
> > +    type_register_static_mayfail(&virtio_ccw_gpu);
> > +#else
> >      type_register_static(&virtio_ccw_gpu);
> > +#endif  
> 
> Move the ifdef to type_register_static_mayfail, so this is not
> duplicated for every module which might need this?

I am concerned about a cluttered API. Having the documentation say:

/**
 * type_register_static_mayfail:
 * @info: The #TypeInfo of the new type.
 *
 * @info and all of the strings it points to should exist for the life time
 * that the type is registered.
 * 
 * If missing a parent type and if qom/object.c is built with CONFIG_MODULES
 * type_register_static_mayfail() differs from type_register_static only in not
 * printing an error and aborting but returning NULL. If qom/object.c is
 * built without CONFIG_MODULES type_register_static_mayfail() is same as
 * type_register_static() 
 * Returns: the new #Type or NULL if missing a parent type.
 */
Type type_register_static_mayfail(const TypeInfo *info);

does not feel right. Indeed modules seems to be the only
circumstance under which a failed type registration does not imply
a programming error. So I'm absolutely against shoving this logic
down into object.c. But I find the variant I posted nicer to document
and nicer to read: looking at virtio_ccw_gpu_register() one sees
immediately that if built as a module, it is OK if the registration
fails, and if built-in it is expected to work.

> 
> > --- a/include/hw/s390x/css.h
> > +++ b/include/hw/s390x/css.h  
> 
> Move this to a separate patch?
> The "add type_register_mayfail" and "modularize virtio-gpu-ccw" changes
> should be separate patches too.
> 
> > -static TypeImpl *type_register_internal(const TypeInfo *info)
> > +static TypeImpl *type_register_internal(const TypeInfo *info, bool mayfail)
> >  {
> >      TypeImpl *ti;
> >      ti = type_new(info);  
> 
> Hmm, type_register_internal seems to not look at the new mayfail flag.
> Patch looks incomplete ...

It definitely is. I messed up my smoke test (used the wrong executable)
so I did not notice.

> 
> take care,
>   Gerd
> 
>