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(-)
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(-) -- 2.26.2
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(-) >
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(-) >> > >
Am 27.10.2022 um 16:52 hat Claudio Fontana geschrieben: > 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. What is the right tree to take this one? get_maintainers.pl doesn't show anything for module.[ch], which might be why nobody feels responsible for merging this. Kevin
On 10/27/22 20:34, Kevin Wolf wrote: > Am 27.10.2022 um 16:52 hat Claudio Fontana geschrieben: >> 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. > > What is the right tree to take this one? > > get_maintainers.pl doesn't show anything for module.[ch], which might be > why nobody feels responsible for merging this. > > Kevin Ping, there is no util/* catch-all, so indeed it seems an unmaintained section of QEMU, Richard and Markus took an interest to review the patches, but it seems that the project needs a maintainer / queue for the util/module.[ch].. Peter could you help with this? Claudio
On Fri, 4 Nov 2022 at 09:05, Claudio Fontana <cfontana@suse.de> wrote: > > On 10/27/22 20:34, Kevin Wolf wrote: > > Am 27.10.2022 um 16:52 hat Claudio Fontana geschrieben: > >> 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. > > > > What is the right tree to take this one? > > > > get_maintainers.pl doesn't show anything for module.[ch], which might be > > why nobody feels responsible for merging this. > > > > Kevin > > Ping, > > there is no util/* catch-all, so indeed it seems an unmaintained section of QEMU, > Richard and Markus took an interest to review the patches, > > but it seems that the project needs a maintainer / queue for the util/module.[ch].. > > Peter could you help with this? I asked on IRC, and Paolo said he'd take this series. thanks -- PMM
© 2016 - 2024 Red Hat, Inc.