Re: [PATCH v9 0/5] improve error handling for module load

[Claudio Fontana]

ping, can this series move on?

Thanks,

Claudio


On 9/29/22 11:30, Claudio Fontana wrote:
> CHANGELOG:
> 
> v8 -> v9:
> 
> * add Signed-off-by tag for Kevin's commit
> * fully reviewed, added tags.
> 
> v7 -> v8:
> 
> * fix a problem in module_load, where the module_name in v7 was mistakenly 
> freed
>   via g_free() also in the success code path, and instead module_name memory
>   is owned by g_hash_table afer g_hash_table_add.
> 
> * add more text to the commit message to indicate areas of further 
> improvements,
>   and more details about changes.
> 
> * in PATCH 5/5, change the commit message to align with the change in v7,
>   ie, we exit(), we do not abort().
> 
> v6 -> v7:
> 
> * changed instances of abort() to exit(1), for the CONFIG_MODULES case 
> (Philippe).
> 
> * dmg: do not use a separate local error, use the existing errp (Kevin)
> 
> * block: do not use a separate local error, use the existing errp for
>   bdrv_find_protocol (Markus)
> 
> v5 -> v6:
> 
> * added a patch by Kevin to handle the dmg warning about missing
>   decompression submodules. (Kevin)
> 
> * added more verbose comments about all the affected callers of module_load
>   and module_load_qom (Markus)
> 
> (OPEN ISSUE): change abort() to exit() when type not present even after 
> loading module?
> 
> (Philippe)
> 
> v4 -> v5:
> 
> * added a patch to rename module_load_one and friends to module_load
> 
> * qdev_new: just reuse module_object_class_by_name, to avoid duplicating code
> 
> * changed return value of module_load to an int:
>   -1 error (Error **errp set).
>    0 module or dependencies not installed,
>    1 loaded
>    2 already loaded (or built-in)
> 
>    Adapted all callers.
> 
> * module_load: fixed some pre-existing memory leaks, used an out: label
>   to do the cleanup.
> 
> v3 -> v4: (Richard)
> 
> * module_object_class_by_name: return NULL immediately on load error.
> * audio_driver_lookup: same.
> * bdrv_find_format: same.
> 
> * dmg_open: handle optional compression submodules better: f.e.,
>   if "dmg-bz2" is not present, continue but offer a warning.
>   If "dmg-bz2" load fails with error, error out and return.
> 
> * module_load_dso: add newline to error_append_hint.
> 
> v2 -> v3:
> 
> * take the file existence check outside of module_load_file,
>   rename module_load_file to module_load_dso, will be called only on
>   an existing file. This will simplify the return value. (Richard)
> 
> * move exported function documentation into header files (Richard)
> 
> v1 -> v2:
> 
> * do not treat the display help text any differently and do report
>   module load _errors_. If the module does not exist (ENOENT, ENOTDIR),
>   no error will be produced.
> 
> DESCRIPTION:
> 
> while investigating a permission issue in accel, where accel-tcg-x86_64.so
> was not accessible, I noticed that no errors were produced regarding the
> module load failure.
> 
> This series attempts to improve module_load_one and module_load_qom_one
> to handle the error cases better and produce some errors.
> 
> Patch 1 is already reviewed and is about removing an unused existing
> argument "mayfail" from the call stack.
> 
> Patch 2 is the real meat, and that one I would say is RFC.
> Will follow up with comments on the specific questions I have.
> 
> Patch 3 finally adds a simple check in accel/, aborting if a module
> is not found, but relying on the existing error report from
> module_load_qom_one.
> 
> Claudio Fontana (4):
>   module: removed unused function argument "mayfail"
>   module: rename module_load_one to module_load
>   module: add Error arguments to module_load and module_load_qom
>   accel: abort if we fail to load the accelerator plugin
> 
> Kevin Wolf (1):
>   dmg: warn when opening dmg images containing blocks of unknown type
> 
>  accel/accel-softmmu.c |   8 +-
>  audio/audio.c         |  16 ++--
>  block.c               |  20 +++-
>  block/dmg.c           |  33 ++++++-
>  hw/core/qdev.c        |  17 +++-
>  include/qemu/module.h |  37 +++++++-
>  qom/object.c          |  18 +++-
>  softmmu/qtest.c       |   8 +-
>  ui/console.c          |  18 +++-
>  util/module.c         | 211 +++++++++++++++++++++++-------------------
>  10 files changed, 260 insertions(+), 126 deletions(-)
>