Re: [PATCH v2 08/13] rust: cleanup module_init!, use it from #[derive(Object)]

[Zhao Liu]

On Mon, Oct 21, 2024 at 06:35:33PM +0200, Paolo Bonzini wrote:
> Date: Mon, 21 Oct 2024 18:35:33 +0200
> From: Paolo Bonzini <pbonz...@redhat.com>
> Subject: [PATCH v2 08/13] rust: cleanup module_init!, use it from
>  #[derive(Object)]
> X-Mailer: git-send-email 2.46.2
> 
> Remove the duplicate code by using the module_init! macro; at the same time,
> simplify how module_init! is used, by taking inspiration from the 
> implementation
> of #[derive(Object)].
> 
> Signed-off-by: Paolo Bonzini <pbonz...@redhat.com>
> ---
>  rust/qemu-api-macros/src/lib.rs  | 33 +++-------------
>  rust/qemu-api/src/definitions.rs | 66 ++++++++++++++------------------
>  2 files changed, 33 insertions(+), 66 deletions(-)

LGTM (with some questions related type_init usage inline)

Reviewed-by: Zhao Liu <zhao1.liu@intel>

> diff --git a/rust/qemu-api-macros/src/lib.rs b/rust/qemu-api-macros/src/lib.rs
> index 70e3f920460..a4bc5d01ee8 100644
> --- a/rust/qemu-api-macros/src/lib.rs
> +++ b/rust/qemu-api-macros/src/lib.rs
> @@ -3,43 +3,20 @@
>  // SPDX-License-Identifier: GPL-2.0-or-later
>  #[proc_macro_derive(Object)]
>  pub fn derive_object(input: TokenStream) -> TokenStream {

[snip]

> +            MODULE_INIT_QOM => unsafe {
> +                ::qemu_api::bindings::type_register_static(&<#name as 
> ::qemu_api::definitions::ObjectImpl>::TYPE_INFO);
>              }

I want to see how general this macro could be, so I checked current
type_init() cases for TypeInfo. In most cases, only type_register_static()
is called directly in the init_fn() callback.

There are only two exceptions:

1. Some init_fn callbacks contain more complex validation or register
logic.

  For example, in backends/hostmem-epc.c, sgx_epc_backed_info involves
  extra check before type_register_static().

  static void register_types(void)
  {
      int fd = qemu_open_old("/dev/sgx_vepc", O_RDWR);
      if (fd >= 0) {
          close(fd);

          type_register_static(&sgx_epc_backed_info);
      }
  }


  And in hw/audio/intel-hda.c, there's extra pci_register_soundhw afer
  type_register_static():

  static void intel_hda_register_types(void)
  {
      type_register_static(&hda_codec_bus_info);
      type_register_static(&intel_hda_info);
      type_register_static(&intel_hda_info_ich6);
      type_register_static(&intel_hda_info_ich9);
      type_register_static(&hda_codec_device_type_info);
      pci_register_soundhw("hda", "Intel HD Audio", intel_hda_and_codec_init);
  }

  The device can define a custom init_fn() for TypeInfo based on
  module_init!, but I wonder if the examples above are valid. Is it
  allowed to include other logic in init_fn()?


2. Some init_fn callbacks use type_register() instead of
type_register_static().

  TypeImpl *type_register_static(const TypeInfo *info)
  {
      return type_register(info);
  }

  It seems that type_register() and type_register_static() are the same.
  I guess I could clean up one of them, right? (type_register() was added
  by your earlie commit 049cb3cfdac1 :-) ).

Thanks,
Zhao