On 9/23/22 17:51, Daniel P. Berrangé wrote:
> On Fri, Sep 23, 2022 at 04:51:30PM +0200, Claudio Fontana wrote:
>> improve error handling during module load, by changing:
>>
>> bool module_load(const char *prefix, const char *lib_name);
>> void module_load_qom(const char *type);
>>
>> to:
>>
>> int module_load(const char *prefix, const char *name, Error **errp);
>> int module_load_qom(const char *type, Error **errp);
>>
>> where the return value is:
>>
>> -1 on module load error, and errp is set with the error
>> 0 on module or one of its dependencies are not installed
>> 1 on module load success
>> 2 on module load success (module already loaded or built-in)
>
>
>
>
>> diff --git a/audio/audio.c b/audio/audio.c
>> index 0a682336a0..ea51793843 100644
>> --- a/audio/audio.c
>> +++ b/audio/audio.c
>> @@ -72,20 +72,24 @@ void audio_driver_register(audio_driver *drv)
>> audio_driver *audio_driver_lookup(const char *name)
>> {
>> struct audio_driver *d;
>> + Error *local_err = NULL;
>> + int rv;
>>
>> QLIST_FOREACH(d, &audio_drivers, next) {
>> if (strcmp(name, d->name) == 0) {
>> return d;
>> }
>> }
>> -
>> - audio_module_load(name);
>> - QLIST_FOREACH(d, &audio_drivers, next) {
>> - if (strcmp(name, d->name) == 0) {
>> - return d;
>> + rv = audio_module_load(name, &local_err);
>> + if (rv > 0) {
>> + QLIST_FOREACH(d, &audio_drivers, next) {
>> + if (strcmp(name, d->name) == 0) {
>> + return d;
>> + }
>> }
>> + } else if (rv < 0) {
>> + error_report_err(local_err);
>> }
>
> The rv == 0 case could be treated the same as rv > 0
Not really, there is no need to do the loop again if rv == 0.
I mean it doesn't hurt, but Richard explicitly asked to remove redundant
execution in these cases.
> meaning the diff merely needs to be
>
> audio_module_load(name, &local_err)
> if (rv < 0) {
> error_report_err(local_err);
> return NULL;.
> }
>
>> return NULL;
>> }
>>
>> diff --git a/block.c b/block.c
>> index 72c7f6d47d..0390ece056 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -464,12 +464,18 @@ BlockDriver *bdrv_find_format(const char *format_name)
>> /* The driver isn't registered, maybe we need to load a module */
>> for (i = 0; i < (int)ARRAY_SIZE(block_driver_modules); ++i) {
>> if (!strcmp(block_driver_modules[i].format_name, format_name)) {
>> - block_module_load(block_driver_modules[i].library_name);
>> + Error *local_err = NULL;
>> + int rv = block_module_load(block_driver_modules[i].library_name,
>> + &local_err);
>> + if (rv > 0) {
>> + return bdrv_do_find_format(format_name);
>> + } else if (rv < 0) {
>> + error_report_err(local_err);
>> + }
>
> Again, rv ==0 can be handled the same as rv > 0
>
>
>> @@ -976,12 +982,17 @@ BlockDriver *bdrv_find_protocol(const char *filename,
>> for (i = 0; i < (int)ARRAY_SIZE(block_driver_modules); ++i) {
>> if (block_driver_modules[i].protocol_name &&
>> !strcmp(block_driver_modules[i].protocol_name, protocol)) {
>> - block_module_load(block_driver_modules[i].library_name);
>> + Error *local_err = NULL;
>> + int rv =
>> block_module_load(block_driver_modules[i].library_name, &local_err);
>> + if (rv > 0) {
>> + drv1 = bdrv_do_find_protocol(protocol);
>> + } else if (rv < 0) {
>> + error_report_err(local_err);
>> + }
>
> Likewise rv == 0 vs rv > 0
same here, this approach was there around v2, but I changed this, to avoid
calling bdrv_do_find_protocol() again
if we did not load the module successfully, as we already know it will fail.
>
>
>
>> diff --git a/block/dmg.c b/block/dmg.c
>> index 007b8d9996..e84a7a44a3 100644
>> --- a/block/dmg.c
>> +++ b/block/dmg.c
>> @@ -434,6 +434,8 @@ static int dmg_open(BlockDriverState *bs, QDict
>> *options, int flags,
>> uint64_t plist_xml_offset, plist_xml_length;
>> int64_t offset;
>> int ret;
>> + int module_rv;
>> + Error *local_err = NULL;
>>
>> ret = bdrv_apply_auto_read_only(bs, NULL, errp);
>> if (ret < 0) {
>> @@ -446,8 +448,21 @@ static int dmg_open(BlockDriverState *bs, QDict
>> *options, int flags,
>> return -EINVAL;
>> }
>>
>> - block_module_load("dmg-bz2");
>> - block_module_load("dmg-lzfse");
>> + module_rv = block_module_load("dmg-bz2", &local_err);
>> + if (module_rv < 0) {
>> + error_report_err(local_err);
>> + return -EINVAL;
>> + } else if (module_rv == 0) {
>> + warn_report("dmg-bz2 module not present, bz2 decomp unavailable");
>> + }
>> + local_err = NULL;
>> + module_rv = block_module_load("dmg-lzfse", &local_err);
>> + if (module_rv < 0) {
>> + error_report_err(local_err);
>> + return -EINVAL;
>> + } else if (module_rv == 0) {
>> + warn_report("dmg-lzfse module not present, lzfse decomp
>> unavailable");
>> + }
>
> THis is the wrong place for these warnings, it'll spam
> stdout, even if the file opened doesn't use bz2/lzfse.
>
> The real problem is the later code which appears to
> silently ignore data blocks if the lzfse/bz2 modules
> are not loaded, instead of using error_report.
Yes, the right place for the warning is a bit unknown to me though in the block
code
as I mentioned elsewhere in the thread, Kevin seems to understand what the
right handling should be,
maybe he can do an after-patch I could attach or merge in the series..
>
>
>> diff --git a/qom/object.c b/qom/object.c
>> index 4f834f3bf6..35ced55282 100644
>> --- a/qom/object.c
>> +++ b/qom/object.c
>> @@ -526,8 +526,13 @@ void object_initialize(void *data, size_t size, const
>> char *typename)
>>
>> #ifdef CONFIG_MODULES
>> if (!type) {
>> - module_load_qom(typename);
>> - type = type_get_by_name(typename);
>> + Error *local_err = NULL;
>> + int rv = module_load_qom(typename, &local_err);
>> + if (rv > 0) {
>> + type = type_get_by_name(typename);
>> + } else if (rv < 0) {
>> + error_report_err(local_err);
>> + }
>
> Again no need to distinguish rv == 0 from rv > 0
same answer here. I don't feel strongly, just saying, I changed this as a
result of the review process.
>
>> @@ -1033,8 +1038,13 @@ ObjectClass *module_object_class_by_name(const char
>> *typename)
>> oc = object_class_by_name(typename);
>> #ifdef CONFIG_MODULES
>> if (!oc) {
>> - module_load_qom(typename);
>> - oc = object_class_by_name(typename);
>> + Error *local_err = NULL;
>> + int rv = module_load_qom(typename, &local_err);
>> + if (rv > 0) {
>> + oc = object_class_by_name(typename);
>> + } else if (rv < 0) {
>> + error_report_err(local_err);
>> + }
>
> Same comment
same
>
>> diff --git a/softmmu/qtest.c b/softmmu/qtest.c
>> index fc5b733c63..36e28609ff 100644
>> --- a/softmmu/qtest.c
>> +++ b/softmmu/qtest.c
>> @@ -753,12 +753,18 @@ static void qtest_process_command(CharBackend *chr,
>> gchar **words)
>> qtest_sendf(chr, "OK %"PRIi64"\n",
>> (int64_t)qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
>> } else if (strcmp(words[0], "module_load") == 0) {
>> + Error *local_err = NULL;
>> + int rv;
>> g_assert(words[1] && words[2]);
>>
>> qtest_send_prefix(chr);
>> - if (module_load(words[1], words[2])) {
>> + rv = module_load(words[1], words[2], &local_err);
>> + if (rv > 0) {
>> qtest_sendf(chr, "OK\n");
>> } else {
>> + if (rv < 0) {
>> + error_report_err(local_err);
>> + }
>> qtest_sendf(chr, "FAIL\n");
>> }
>
> This change means the 'module_load' command is totally silent
If rv == 0, qtest will do
qtest_sendf(chr, "FAIL\n");
but will not report an error message.
Should we?
> if 'rv == 0', but the code appears to try to read a response
> line which will now never arrive AFAICT.
>
> In the context of 'modules-test.c' I think it is fine to treat
> rv == 0 the same as rv > 0 and thus print 'OK'.
>
>
>
> Perhaps I've overlooked something, but I'm not seeing a reason
> we need module_load() to return 4 different return values. It
> looks like every caller would work with a boolean success/fail
>
>
> With regards,
> Daniel
