[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Halil Pasic
Subject: Re: [PATCH v2 1/1] hw/s390x: modularize virtio-gpu-ccw
Date: Wed, 24 Feb 2021 17:46:34 +0100

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

reply via email to

[Prev in Thread] Current Thread [Next in Thread]