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

["Daniel P. Berrangé" <berrange@redhat.com>]

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
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



> 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.


> 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

> @@ -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

> 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', 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
-- 
|: 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 :|