Re: [PATCH v5 3/4] module: add Error arguments to module_load and module_load_qom

[Claudio Fontana]

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