module_object_class_by_name() calls module_load_qom_one if the object
is provided by a dynamically linked library. Such library might not be
available at this moment - for instance, it can be a package not yet
installed. Thus, instead of assert error messages, this patch outputs
more friendly messages.

Current error messages:
$ ./qemu-system-x86_64 -machine q35 -accel tcg -kernel /boot/vmlinuz
...
ERROR:../accel/accel-softmmu.c:82:accel_init_ops_interfaces: assertion failed: (ops != NULL)
Bail out! ERROR:../accel/accel-softmmu.c:82:accel_init_ops_interfaces: assertion failed: (ops != NULL)
[1]    31964 IOT instruction (core dumped)  ./qemu-system-x86_64 ...

New error message:
$ ./qemu-system-x86_64 -machine q35 -accel tcg -kernel /boot/vmlinuz
accel-tcg-x86_64 module is missing, install the package or config the library path correctly.

$ make check
...
Running test qtest-x86_64/test-filter-mirror
Running test qtest-x86_64/endianness-test
accel-qtest-x86_64 module is missing, install the package or config the library path correctly.
accel-qtest-x86_64 module is missing, install the package or config the library path correctly.
accel-qtest-x86_64 module is missing, install the package or config the library path correctly.
accel-qtest-x86_64 module is missing, install the package or config the library path correctly.
accel-qtest-x86_64 module is missing, install the package or config the library path correctly.
accel-tcg-x86_64 module is missing, install the package or config the library path correctly.
...

Signed-off-by: Jose R. Ziviani <jziviani@suse.de>
---
 qom/object.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/qom/object.c b/qom/object.c
index 6a01d56546..2d40245af9 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -1024,6 +1024,24 @@ ObjectClass *object_class_by_name(const char *typename)
     return type->class;
 }
 
+char *get_accel_module_name(const char *ac_name);
+
+char *get_accel_module_name(const char *ac_name)
+{
+    size_t len = strlen(ac_name);
+    char *module_name = NULL;
+
+    if (strncmp(ac_name, "tcg-accel-ops", len) == 0) {
+#ifdef CONFIG_TCG_MODULAR
+        module_name = g_strdup_printf("%s%s", "accel-tcg-", "x86_64");
+#endif
+    } else if (strncmp(ac_name, "qtest-accel-ops", len) == 0) {
+        module_name = g_strdup_printf("%s%s", "accel-qtest-", "x86_64");
+    }
+
+    return module_name;
+}
+
 ObjectClass *module_object_class_by_name(const char *typename)
 {
     ObjectClass *oc;
@@ -1031,8 +1049,20 @@ ObjectClass *module_object_class_by_name(const char *typename)
     oc = object_class_by_name(typename);
 #ifdef CONFIG_MODULES
     if (!oc) {
+        char *module_name;
         module_load_qom_one(typename);
         oc = object_class_by_name(typename);
+        module_name = get_accel_module_name(typename);
+        if (module_name) {
+            if (!module_is_loaded(module_name)) {
+                fprintf(stderr, "%s module is missing, install the "
+                                "package or config the library path "
+                                "correctly.\n", module_name);
+                g_free(module_name);
+                exit(1);
+            }
+            g_free(module_name);
+        }
     }
 #endif
     return oc;
-- 
2.32.0

On 7/1/21 1:27 AM, Jose R. Ziviani wrote:
> module_object_class_by_name() calls module_load_qom_one if the object
> is provided by a dynamically linked library. Such library might not be
> available at this moment - for instance, it can be a package not yet
> installed. Thus, instead of assert error messages, this patch outputs
> more friendly messages.
> 
> Current error messages:
> $ ./qemu-system-x86_64 -machine q35 -accel tcg -kernel /boot/vmlinuz
> ...
> ERROR:../accel/accel-softmmu.c:82:accel_init_ops_interfaces: assertion failed: (ops != NULL)
> Bail out! ERROR:../accel/accel-softmmu.c:82:accel_init_ops_interfaces: assertion failed: (ops != NULL)
> [1]    31964 IOT instruction (core dumped)  ./qemu-system-x86_64 ...
> 
> New error message:
> $ ./qemu-system-x86_64 -machine q35 -accel tcg -kernel /boot/vmlinuz
> accel-tcg-x86_64 module is missing, install the package or config the library path correctly.
> 
> $ make check
> ...
> Running test qtest-x86_64/test-filter-mirror
> Running test qtest-x86_64/endianness-test
> accel-qtest-x86_64 module is missing, install the package or config the library path correctly.
> accel-qtest-x86_64 module is missing, install the package or config the library path correctly.
> accel-qtest-x86_64 module is missing, install the package or config the library path correctly.
> accel-qtest-x86_64 module is missing, install the package or config the library path correctly.
> accel-qtest-x86_64 module is missing, install the package or config the library path correctly.
> accel-tcg-x86_64 module is missing, install the package or config the library path correctly.
> ...
> 
> Signed-off-by: Jose R. Ziviani <jziviani@suse.de>
> ---
>  qom/object.c | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
> 
> diff --git a/qom/object.c b/qom/object.c
> index 6a01d56546..2d40245af9 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -1024,6 +1024,24 @@ ObjectClass *object_class_by_name(const char *typename)
>      return type->class;
>  }
>  
> +char *get_accel_module_name(const char *ac_name);
> +
> +char *get_accel_module_name(const char *ac_name)
> +{
> +    size_t len = strlen(ac_name);
> +    char *module_name = NULL;
> +
> +    if (strncmp(ac_name, "tcg-accel-ops", len) == 0) {
> +#ifdef CONFIG_TCG_MODULAR
> +        module_name = g_strdup_printf("%s%s", "accel-tcg-", "x86_64");
> +#endif
> +    } else if (strncmp(ac_name, "qtest-accel-ops", len) == 0) {
> +        module_name = g_strdup_printf("%s%s", "accel-qtest-", "x86_64");
> +    }
> +
> +    return module_name;
> +}
> +
>  ObjectClass *module_object_class_by_name(const char *typename)
>  {
>      ObjectClass *oc;
> @@ -1031,8 +1049,20 @@ ObjectClass *module_object_class_by_name(const char *typename)
>      oc = object_class_by_name(typename);
>  #ifdef CONFIG_MODULES
>      if (!oc) {
> +        char *module_name;
>          module_load_qom_one(typename);
>          oc = object_class_by_name(typename);
> +        module_name = get_accel_module_name(typename);
> +        if (module_name) {
> +            if (!module_is_loaded(module_name)) {
> +                fprintf(stderr, "%s module is missing, install the "
> +                                "package or config the library path "
> +                                "correctly.\n", module_name);
> +                g_free(module_name);
> +                exit(1);
> +            }
> +            g_free(module_name);
> +        }
>      }
>  #endif
>      return oc;
> 

Hi Jose,

this inserts accel logic into module_object_class_by_name,
maybe some other place would be better to insert this check,
like when trying to select an accelerator?

Thanks,

CLaudio

On Mon, Jul 19, 2021 at 05:29:49PM +0200, Claudio Fontana wrote:
> On 7/1/21 1:27 AM, Jose R. Ziviani wrote:
> > module_object_class_by_name() calls module_load_qom_one if the object
> > is provided by a dynamically linked library. Such library might not be
> > available at this moment - for instance, it can be a package not yet
> > installed. Thus, instead of assert error messages, this patch outputs
> > more friendly messages.
> > 
> > Current error messages:
> > $ ./qemu-system-x86_64 -machine q35 -accel tcg -kernel /boot/vmlinuz
> > ...
> > ERROR:../accel/accel-softmmu.c:82:accel_init_ops_interfaces: assertion failed: (ops != NULL)
> > Bail out! ERROR:../accel/accel-softmmu.c:82:accel_init_ops_interfaces: assertion failed: (ops != NULL)
> > [1]    31964 IOT instruction (core dumped)  ./qemu-system-x86_64 ...
> > 
> > New error message:
> > $ ./qemu-system-x86_64 -machine q35 -accel tcg -kernel /boot/vmlinuz
> > accel-tcg-x86_64 module is missing, install the package or config the library path correctly.
> > 
> > $ make check
> > ...
> > Running test qtest-x86_64/test-filter-mirror
> > Running test qtest-x86_64/endianness-test
> > accel-qtest-x86_64 module is missing, install the package or config the library path correctly.
> > accel-qtest-x86_64 module is missing, install the package or config the library path correctly.
> > accel-qtest-x86_64 module is missing, install the package or config the library path correctly.
> > accel-qtest-x86_64 module is missing, install the package or config the library path correctly.
> > accel-qtest-x86_64 module is missing, install the package or config the library path correctly.
> > accel-tcg-x86_64 module is missing, install the package or config the library path correctly.
> > ...
> > 
> > Signed-off-by: Jose R. Ziviani <jziviani@suse.de>
> > ---
> >  qom/object.c | 30 ++++++++++++++++++++++++++++++
> >  1 file changed, 30 insertions(+)
> > 
> > diff --git a/qom/object.c b/qom/object.c
> > index 6a01d56546..2d40245af9 100644
> > --- a/qom/object.c
> > +++ b/qom/object.c
> > @@ -1024,6 +1024,24 @@ ObjectClass *object_class_by_name(const char *typename)
> >      return type->class;
> >  }
> >  
> > +char *get_accel_module_name(const char *ac_name);
> > +
> > +char *get_accel_module_name(const char *ac_name)
> > +{
> > +    size_t len = strlen(ac_name);
> > +    char *module_name = NULL;
> > +
> > +    if (strncmp(ac_name, "tcg-accel-ops", len) == 0) {
> > +#ifdef CONFIG_TCG_MODULAR
> > +        module_name = g_strdup_printf("%s%s", "accel-tcg-", "x86_64");
> > +#endif
> > +    } else if (strncmp(ac_name, "qtest-accel-ops", len) == 0) {
> > +        module_name = g_strdup_printf("%s%s", "accel-qtest-", "x86_64");
> > +    }
> > +
> > +    return module_name;
> > +}
> > +
> >  ObjectClass *module_object_class_by_name(const char *typename)
> >  {
> >      ObjectClass *oc;
> > @@ -1031,8 +1049,20 @@ ObjectClass *module_object_class_by_name(const char *typename)
> >      oc = object_class_by_name(typename);
> >  #ifdef CONFIG_MODULES
> >      if (!oc) {
> > +        char *module_name;
> >          module_load_qom_one(typename);
> >          oc = object_class_by_name(typename);
> > +        module_name = get_accel_module_name(typename);
> > +        if (module_name) {
> > +            if (!module_is_loaded(module_name)) {
> > +                fprintf(stderr, "%s module is missing, install the "
> > +                                "package or config the library path "
> > +                                "correctly.\n", module_name);
> > +                g_free(module_name);
> > +                exit(1);
> > +            }
> > +            g_free(module_name);
> > +        }
> >      }
> >  #endif
> >      return oc;
> > 
> 
> Hi Jose,
> 
> this inserts accel logic into module_object_class_by_name,
> maybe some other place would be better to insert this check,
> like when trying to select an accelerator?


Hello Claudio,

Yes, I think that 'get_accel_module_name()' may be a more generic
'get_module_name()' to handle any module failure (not only
accelerators).

I'll improve it and send a v2. Thank you for reviewing it.

> 
> Thanks,
> 
> CLaudio

On 7/20/21 3:26 AM, Jose R. Ziviani wrote:
> On Mon, Jul 19, 2021 at 05:29:49PM +0200, Claudio Fontana wrote:
>> On 7/1/21 1:27 AM, Jose R. Ziviani wrote:
>>> module_object_class_by_name() calls module_load_qom_one if the object
>>> is provided by a dynamically linked library. Such library might not be
>>> available at this moment - for instance, it can be a package not yet
>>> installed. Thus, instead of assert error messages, this patch outputs
>>> more friendly messages.
>>>
>>> Current error messages:
>>> $ ./qemu-system-x86_64 -machine q35 -accel tcg -kernel /boot/vmlinuz
>>> ...
>>> ERROR:../accel/accel-softmmu.c:82:accel_init_ops_interfaces: assertion failed: (ops != NULL)
>>> Bail out! ERROR:../accel/accel-softmmu.c:82:accel_init_ops_interfaces: assertion failed: (ops != NULL)
>>> [1]    31964 IOT instruction (core dumped)  ./qemu-system-x86_64 ...
>>>
>>> New error message:
>>> $ ./qemu-system-x86_64 -machine q35 -accel tcg -kernel /boot/vmlinuz
>>> accel-tcg-x86_64 module is missing, install the package or config the library path correctly.
>>>
>>> $ make check
>>> ...
>>> Running test qtest-x86_64/test-filter-mirror
>>> Running test qtest-x86_64/endianness-test
>>> accel-qtest-x86_64 module is missing, install the package or config the library path correctly.
>>> accel-qtest-x86_64 module is missing, install the package or config the library path correctly.
>>> accel-qtest-x86_64 module is missing, install the package or config the library path correctly.
>>> accel-qtest-x86_64 module is missing, install the package or config the library path correctly.
>>> accel-qtest-x86_64 module is missing, install the package or config the library path correctly.
>>> accel-tcg-x86_64 module is missing, install the package or config the library path correctly.
>>> ...
>>>
>>> Signed-off-by: Jose R. Ziviani <jziviani@suse.de>
>>> ---
>>>  qom/object.c | 30 ++++++++++++++++++++++++++++++
>>>  1 file changed, 30 insertions(+)
>>>
>>> diff --git a/qom/object.c b/qom/object.c
>>> index 6a01d56546..2d40245af9 100644
>>> --- a/qom/object.c
>>> +++ b/qom/object.c
>>> @@ -1024,6 +1024,24 @@ ObjectClass *object_class_by_name(const char *typename)
>>>      return type->class;
>>>  }
>>>  
>>> +char *get_accel_module_name(const char *ac_name);
>>> +
>>> +char *get_accel_module_name(const char *ac_name)
>>> +{
>>> +    size_t len = strlen(ac_name);
>>> +    char *module_name = NULL;
>>> +
>>> +    if (strncmp(ac_name, "tcg-accel-ops", len) == 0) {
>>> +#ifdef CONFIG_TCG_MODULAR
>>> +        module_name = g_strdup_printf("%s%s", "accel-tcg-", "x86_64");
>>> +#endif
>>> +    } else if (strncmp(ac_name, "qtest-accel-ops", len) == 0) {
>>> +        module_name = g_strdup_printf("%s%s", "accel-qtest-", "x86_64");
>>> +    }
>>> +
>>> +    return module_name;
>>> +}
>>> +
>>>  ObjectClass *module_object_class_by_name(const char *typename)
>>>  {
>>>      ObjectClass *oc;
>>> @@ -1031,8 +1049,20 @@ ObjectClass *module_object_class_by_name(const char *typename)
>>>      oc = object_class_by_name(typename);
>>>  #ifdef CONFIG_MODULES
>>>      if (!oc) {
>>> +        char *module_name;
>>>          module_load_qom_one(typename);
>>>          oc = object_class_by_name(typename);
>>> +        module_name = get_accel_module_name(typename);
>>> +        if (module_name) {
>>> +            if (!module_is_loaded(module_name)) {
>>> +                fprintf(stderr, "%s module is missing, install the "
>>> +                                "package or config the library path "
>>> +                                "correctly.\n", module_name);
>>> +                g_free(module_name);
>>> +                exit(1);
>>> +            }
>>> +            g_free(module_name);
>>> +        }
>>>      }
>>>  #endif
>>>      return oc;
>>>
>>
>> Hi Jose,
>>
>> this inserts accel logic into module_object_class_by_name,
>> maybe some other place would be better to insert this check,
>> like when trying to select an accelerator?
> 
> 
> Hello Claudio,
> 
> Yes, I think that 'get_accel_module_name()' may be a more generic
> 'get_module_name()' to handle any module failure (not only
> accelerators).
> 
> I'll improve it and send a v2. Thank you for reviewing it.
> 

I also wonder, and @Gerd , should "tcg_enabled()", tcg_allowed etc as a mechanism be rethought,
in light of modular TCG?

Probably would be easier to do if we had a general mechanism for CONFIG_TCG=m CONFIG_KVM=m ... which can separately be set as modules.

Thanks,

Claudio

>  ObjectClass *module_object_class_by_name(const char *typename)
>  {
>      ObjectClass *oc;
> @@ -1031,8 +1049,20 @@ ObjectClass *module_object_class_by_name(const char *typename)
>      oc = object_class_by_name(typename);
>  #ifdef CONFIG_MODULES
>      if (!oc) {
> +        char *module_name;
>          module_load_qom_one(typename);
>          oc = object_class_by_name(typename);
> +        module_name = get_accel_module_name(typename);
> +        if (module_name) {
> +            if (!module_is_loaded(module_name)) {
> +                fprintf(stderr, "%s module is missing, install the "
> +                                "package or config the library path "
> +                                "correctly.\n", module_name);
> +                g_free(module_name);
> +                exit(1);
> +            }
> +            g_free(module_name);
> +        }

This error logging should IMHO be moved to util/module.c.  Either have a
helper function to print the error message, or have
module_load_qom_one() print it.

There is also no need to hard-code the module names.  We have the module
database and module_load_qom_one() uses it to figure which module must
be loaded for a specific qom object.  We can likewise use the database
for printing the error message.

take care,
  Gerd

On Wed, Jul 21, 2021 at 11:54:45AM +0200, Gerd Hoffmann wrote:
> >  ObjectClass *module_object_class_by_name(const char *typename)
> >  {
> >      ObjectClass *oc;
> > @@ -1031,8 +1049,20 @@ ObjectClass *module_object_class_by_name(const char *typename)
> >      oc = object_class_by_name(typename);
> >  #ifdef CONFIG_MODULES
> >      if (!oc) {
> > +        char *module_name;
> >          module_load_qom_one(typename);
> >          oc = object_class_by_name(typename);
> > +        module_name = get_accel_module_name(typename);
> > +        if (module_name) {
> > +            if (!module_is_loaded(module_name)) {
> > +                fprintf(stderr, "%s module is missing, install the "
> > +                                "package or config the library path "
> > +                                "correctly.\n", module_name);
> > +                g_free(module_name);
> > +                exit(1);
> > +            }
> > +            g_free(module_name);
> > +        }
> 
> This error logging should IMHO be moved to util/module.c.  Either have a
> helper function to print the error message, or have
> module_load_qom_one() print it.
> 
> There is also no need to hard-code the module names.  We have the module
> database and module_load_qom_one() uses it to figure which module must
> be loaded for a specific qom object.  We can likewise use the database
> for printing the error message.

IIUC, module loading can be triggered from hotplug of backends/devices,
If so, we really shouldn't be printing to stderr, but using Error *errp
throughout, so that QMP can give back useful error messages


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|