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

[Halil Pasic]

On Wed, 3 Mar 2021 08:07:50 +0100
Gerd Hoffmann <kraxel@redhat.com> wrote:

>   Hi,
> 
> > The only approaches I can think of to make type_register_mayfail()
> > "work" involve adding a dependency check in type_register_internal()
> > before the call to type_table_add() is made. This can "work" for modules,
> > because for types loaded from we can hope, that all dependencies are
> > already, as modules are loaded relatively late.  
> 
> Yes, for modules the lazy binding should not be needed and we should be
> able to check for the parent at registration time.  module.c keeps track
> of whenever phase1 init for builtin qom objects did happen already, so
> we can use that instead of passing mayfail through a bunch of function
> calls.  Quick & dirty test hack below.

Hi Gerd! Thank you very much for your patience. I've sent out a v3 yesterday,
which takes a similar, yet slightly different approach. I will comment
on the proposed diff below. Could you please have a look at my v3? I
would prefer if we can go forward with that solution, but I am more than
willing to prepare a v4 based on this proposal.


> 
> BTW: "qemu-system-x86_64 -device help" tries to load all modules and is
> a nice test case ;)

Yes, I've reported the problem in 
Message-ID: <20210219035206.730f145e.pasic@linux.ibm.com>
using that, for the trace I took -device virtio-gpu-ccw because
the trace looked nicer to me. ;)

> 
> HTH,
>   Gerd
> 
> commit 75ca3012e626318b562bcb51ecdc34400e25f2d0
> Author: Gerd Hoffmann <kraxel@redhat.com>
> Date:   Tue Mar 2 16:26:39 2021 +0100
> 
>     [hack] modular type init check
> 
> diff --git a/qom/object.c b/qom/object.c
> index 491823db4a2d..01785e73f495 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -135,11 +135,22 @@ static TypeImpl *type_new(const TypeInfo *info)
>      return ti;
>  }
>  
> +/* HACK: util/module.c */
> +extern bool modules_init_done[MODULE_INIT_MAX];
> +static TypeImpl *type_get_by_name(const char *name);
> +
>  static TypeImpl *type_register_internal(const TypeInfo *info)
>  {
>      TypeImpl *ti;
>      ti = type_new(info);
>  
> +    if (modules_init_done[MODULE_INIT_QOM]) {
> +        if (ti->parent && !type_get_by_name(ti->parent)) {
> +            g_free(ti);

The function type_new() has some g_strdup() action. We would need a
type_delete() in order to clean those up properly if we are taking
this approach. In v3 I decided to check the info, and avoid instantiating
a ti so I don't have to clean it up.

> +            return NULL;
> +        }
> +    }
> +

This would change the postcondition of the type_register*() familiy
of functions. It effectively  makes type_register_*() a 'mayfail'
depending on a global state, which is
modules_init_done[MODULE_INIT_QOM].  It affects all modules. IMHO we should 
also update the documentation if we decide to move forward with this approach.

I will give this a spin later.

Please have a look at my v3 and let us decide how to move forward based
on that.

Regards,
Halil

[..]