| Message ID | 20210630232749.21873-4-jziviani@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Improve module accelerator error message | expand |
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
On Wed, Jul 21, 2021 at 10:57:37AM +0100, Daniel P. Berrangé wrote: > 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 Thank you Gerd and Daniel, I'll improve it and send a v2. Thank you very much, Jose > > > 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 :| >
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;
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(+)