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

[Claudio Fontana <cfontana@suse.de>]

A ping on this one, is there anything more that needs to be urgently addressed before it can be queued for inclusion?
This is currently creating problems for upstream kubevirt, due to the error handling not properly reporting permissions errors on module file access.

Thanks,

Claudio

On 10/21/22 10:24, Claudio Fontana wrote:
> 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(-)
>>
> 
>