Re: [PATCH v4 2/3] module: add Error arguments to module_load_one and module_load_qom_one

[Claudio Fontana]

On 9/16/22 16:26, Markus Armbruster wrote:
> Claudio Fontana <cfontana@suse.de> writes:
> 
>> On 9/16/22 11:27, Markus Armbruster wrote:
>>> Claudio Fontana <cfontana@suse.de> writes:
>>>
>>>> improve error handling during module load, by changing:
>>>>
>>>> bool module_load_one(const char *prefix, const char *lib_name);
>>>> void module_load_qom_one(const char *type);
>>>>
>>>> to:
>>>>
>>>> bool module_load_one(const char *prefix, const char *name, Error **errp);
>>>> bool module_load_qom_one(const char *type, Error **errp);
>>>>
>>>> module_load_qom_one has been introduced in:
>>>>
>>>> commit 28457744c345 ("module: qom module support"), which built on top of
>>>> module_load_one, but discarded the bool return value. Restore it.
>>>>
>>>> Adapt all callers to emit errors, or ignore them, or fail hard,
>>>> as appropriate in each context.
>>>
>>> How exactly does behavior change?  The commit message is mum on the
>>> behavior before the patch, and vague on the behavior afterwards.
>>
>> The current behavior is difficult to describe as it is basically devoid of 
>> any error handling for QOM modules.
>> What error handling facilities were potentially available before were 
>> rendered moot by commit 28457744c345.
> 
> *Sigh*
> 
>> the behavior changes in this way:
>>
>> audio: when attempting to load an audio module, now actually report module 
>> load errors.
>> Previous behavior: say nothing, and leave it to the user to figure out what 
>> is wrong by failures later on during execution.
>>
>> block: when attempting to load a block module, now actually report module 
>> load errors.
>> Previous behavior: say nothing, and leave it to the user to figure out what 
>> is wrong by failures later on during execution.
>>
>> block/dmg: specifically for the dmg module, which contains submodules 
>> dmg-bz2 and dmg-lzfse, now also warn that if the submodules are not present, 
>> the corresponding decompression will not be available.
>> Previous behavior: say nothing, and leave it to the user to figure out what 
>> is wrong by failures later on during execution.
>>
>> qdev: when creating a new qdev Device object (DeviceState), actually report 
>> module load errors. If a module cannot be loaded to create that device, 
>> abort execution as intended.
>> QEMU users of qdev assume that qdev_new() returns non-NULL. There is a 
>> separate qdev_try_new() function whose only difference from qdev_new() is 
>> that it can return NULL rather than asserting.
>> (see include/hw/qdev-core.h :: qdev_try_new)
>>
>> Previous behavior: say nothing, and leave it to the user to figure out what 
>> is wrong by failures later on during execution (likely, just segfault).
>>
>> qom/object.c : when initializing a QOM object, actually report module load 
>> errors.
>> Previous behavior: say nothing, and leave it to the user to figure out what 
>> is wrong by failures later on during execution.
>>
>> qom/object.c : when looking up an ObjectClass by name 
>> (module_object_class_by_name), actually report module load errors.
>> Previous behavior: say nothing, and leave it to the user to figure out what 
>> is wrong by failures later on during execution.
>>
>> qtest: when processing the "module_load" qtest command, if there is an 
>> actual error in the load of the module, report it in addition to sending a 
>> "FAIL" response.
>>
>> console: when attempting to load a display module, now actually report 
>> module load errors.
>> Previous behavior: say nothing, and leave it to the user to figure out what 
>> is wrong by failures later on during execution.
>>
>> util/module: provide a boolean return value and an Error parameter to 
>> module_load_qom_one and provide an Error parameter to module_load_one what 
>> was missing it.
>> Report an error using the Error facilities when module is present, but fails 
>> to be loaded due to an error occurring.
>> Also report an error if multiple modules are loadable and try to provide the 
>> same type.
>>
>> Previous behavior:
>> difficult to describe, a really confused implementation.
>>
>> The module_load_file part of the code seemed to assume being called 
>> separately, and was checking the caller arguments as though it would be a 
>> self-contained API,
>> rechecking the suffix of the string for a valid ".so" (CONFIG_HOST_DSOSUF), 
>> when it is called that way explicitly and only by module_load_one as a 
>> static function.
>>
>> It was printing an error with fprintf specifically only in the case where 
>> g_module_open failed, and never for any other error condition.
>> It only printed this error if the boolean "mayfail" was set to false, but 
>> the argument was always passed as false, in all execution paths.
>> It was not reporting any error if multiple modules are present all providing 
>> the same type.
> 
> What a mess.
> 
>> Let me know if you have further questions, what I described is a summary of 
>> all the changes contained. I can add to the commit message if necessary.
> 
> Yes, please.

Will do.

> 
>>>> Signed-off-by: Claudio Fontana <cfontana@suse.de>
>>>> ---
>>>>  audio/audio.c         |   9 ++-
>>>>  block.c               |  15 ++++-
>>>>  block/dmg.c           |  18 +++++-
>>>>  hw/core/qdev.c        |  10 ++-
>>>>  include/qemu/module.h |  38 ++++++++++--
>>>>  qom/object.c          |  18 +++++-
>>>>  softmmu/qtest.c       |   6 +-
>>>>  ui/console.c          |  18 +++++-
>>>>  util/module.c         | 140 ++++++++++++++++++++++++------------------
>>>>  9 files changed, 194 insertions(+), 78 deletions(-)
>>>>
>>>> diff --git a/audio/audio.c b/audio/audio.c
>>>> index 76b8735b44..cff7464c07 100644
>>>> --- a/audio/audio.c
>>>> +++ b/audio/audio.c
>>>> @@ -72,6 +72,7 @@ void audio_driver_register(audio_driver *drv)
>>>>  audio_driver *audio_driver_lookup(const char *name)
>>>>  {
>>>>      struct audio_driver *d;
>>>> +    Error *local_err = NULL;
>>>>  
>>>>      QLIST_FOREACH(d, &audio_drivers, next) {
>>>>          if (strcmp(name, d->name) == 0) {
>>>> @@ -79,7 +80,13 @@ audio_driver *audio_driver_lookup(const char *name)
>>>>          }
>>>>      }
>>>>  
>>>> -    audio_module_load_one(name);
>>>> +    if (!audio_module_load_one(name, &local_err)) {
>>>> +        if (local_err) {
>>>> +            error_report_err(local_err);
>>>> +        }
>>>> +        return NULL;
>>>> +    }
>>>> +
>>>>      QLIST_FOREACH(d, &audio_drivers, next) {
>>>>          if (strcmp(name, d->name) == 0) {
>>>>              return d;
>>>> diff --git a/block.c b/block.c
>>>> index bc85f46eed..8b610c6d95 100644
>>>> --- a/block.c
>>>> +++ b/block.c
>>>> @@ -464,7 +464,14 @@ 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_one(block_driver_modules[i].library_name);
>>>> +            Error *local_err = NULL;
>>>> +            if 
>>>> (!block_module_load_one(block_driver_modules[i].library_name,
>>>> +                                       &local_err)) {
>>>> +                if (local_err) {
>>>> +                    error_report_err(local_err);
>>>> +                }
>>>> +                return NULL;
>>>> +            }
>>>>              break;
>>>>          }
>>>>      }
>>>
>>> Before the patch, bdrv_find_format() fails silently[*].
>>>
>>> Afterwards, it reports an error on some failures, but not on others.
>>> Sure this is what we want?
>>
>> Yes, I am sure. It only reports an error if an error exists.
>> When a module is not present, no error should be printed in this context.
> 
> Callers commonly look like this (this one is in bdrv_fill_options()):
> 
>         drv = bdrv_find_format(drvname);
>         if (!drv) {
>             error_setg(errp, "Unknown driver '%s'", drvname);
>             return -ENOENT;
>         }
> 
> where @errp is a parameter.
> 
> The patch changes bdrv_find_format() to report some failures with
> error_report_err().  This is effectively the same anti-pattern I pointed
> out below, starting with "Uh-oh": use of error_report() or equivalent
> within a function with an Error ** parameter.
> 
> If the "Unknown driver" Error object is ultimately passed to
> error_report_err(), we report two errors if module loading fails, else
> one.
> 
> Emitting two error messages for the same problem, a useful one and a
> useless one, is bad UI, but it's arguably still better than just a
> useless one.

I'll see if it makes sense to merge them.
I wanted to avoid removing the existing Unknown driver / or Unknown Protocol 
errors
which people might be relying on.

> 
> But what if a caller wants to handle errors without reporting them?
> This is a legitimate use of the Error API.  No go, we spam stderr
> regardless.

I suppose the Error API can be directed to something else than stderr?

> 
> What if a caller wants to report the error in some other way, say write
> it to a log file?  No go, we still spam stderr.
> 
> The patch breaks on of the Error API's design maxims.  error.h's big
> comment:
> 
>  * - Separation of concerns: the function is responsible for detecting
>  *   errors and failing cleanly; handling the error is its caller's
>  *   job.  [...]
> 
> The patch changes bdrv_find_options() to no longer satisfy this rule: it
> doesn't leave handling entirely to the caller anymore.
> 
> You might argue that no existing caller does such things.  Fine.  Then
> I'll ask you to document that bdrv_fill_options() may only be used in
> certain ways, unlike other functions using the Error API.  Same for all
> the other affected functions.  And then Kevin will probably NAK the
> whole thing :)
> 
>>>> @@ -976,7 +983,11 @@ 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_one(block_driver_modules[i].library_name);
>>>> +            Error *local_err = NULL;
>>>> +            if 
>>>> (!block_module_load_one(block_driver_modules[i].library_name,
>>>> +                                       &local_err) && local_err) {
>>>
>>> Break the line before && local_err, please, like you do elsewhere.
>>
>> This is to avoid hitting the 80 chars on one line, as warned by checkpatch. 
>> Should I disregard checkpatch, is it not used anymore?
> 
> Elsewhere, you format like this:
> 
>                  if 
> (!block_module_load_one(block_driver_modules[i].library_name,
>                                             &local_err)
>                      && local_err) {
> 
> Better, isn't it?

I would not say so, I have no preference, but I can change it.

> 
>>
>>>
>>>> +                error_report_err(local_err);
>>>> +            }
>>>>              break;
>>>>          }
>>>>      }
>>>
>>> Uh-oh: error_report() or equivalent in a function with an Error **
>>> parameter.  This is almost always wrong.  Shouldn't you pass the error
>>> to the caller?
>>
>> I guess this is the "almost" case. No I should not pass anything back.
>>
>> The Error **errp parameter of bdrv_find_protocol, and the Error local_err 
>> parameter inside it are different errors.
>>
>> The first is about whether a protocol has been found or not.
>> The second is about whether there was an error during the load of a module.
>>
>> No, local_err should not be passed back to the caller in my view. The error 
>> passed back to the caller is errp and is already set correctly.
> 
> Part of bdrv_find_protocol()'s (unstated) contract is "either succeed
> and return non-null, or fail, return null, and set an error."
> 
> This is pretty much ubiquitous among Error-using functions.  error.h's
> big comment:
> 
>  * - On success, the function should not touch *errp.  On failure, it
>  *   should set a new error, e.g. with error_setg(errp, ...), or
>  *   propagate an existing one, e.g. with error_propagate(errp, ...).
> 
> Note there is no third case "except on some failures, don't set an
> error".
> 
> Of course, bdrv_find_protocol() might be an exception, i.e. I still owe
> you proof of my claim about its contract.  So let's examine callers:
> 
> * print_block_option_help() in qemu-img.c:
> 
>             proto_drv = bdrv_find_protocol(filename, true, &local_err);
>             if (!proto_drv) {
>                 error_report_err(local_err);
>                 qemu_opts_free(create_opts);
>                 return 1;
>             }
> 
>   If bdrv_find_protocol() returns null without setting @local_err,
>   error_report_err() will crash.

I don't see how this patch changes anything here, or why it relates to this 
code at all.

No, this patch does not change bdrv_find_protocol() unwritten claim about the 
Error parameter.

I think you should re-read the code honestly.

> 
>   Two more instances elsewhere in qemu-img.c
> 
> * bdrv_create_file() in block.c:
> 
>         drv = bdrv_find_protocol(filename, true, errp);
>         if (drv == NULL) {
>             return -ENOENT;
>         }
> 
>   If bdrv_find_protocol() returns null without setting @local_err, then
>   bdrv_create_file() returns -ENOENT without setting an error.

Again, I do not see how this is an argument pertinent to this patch.

> 
>   bdrv_create_file() is called by a bunch of BlockDriver
>   .bdrv_co_create_opts() methods, e.g. raw_co_create_opts():
> 
>         return bdrv_create_file(filename, opts, errp);
> 
>   The method is called by bdrv_create_co_entry():
> 
>         ret = cco->drv->bdrv_co_create_opts(cco->drv,
>                                             cco->filename, cco->opts, 
> &local_err);
>         error_propagate(&cco->err, local_err);
>         cco->ret = ret;
> 
>        If bdrv_find_protocol() returns null without setting an error, then
>        we set cco->ret = -ENOENT and cco->err = NULL.
> 
>        One of its users is bdrv_create():
> 
>         if (qemu_in_coroutine()) {
>             /* Fast-path if already in coroutine context */
>             bdrv_create_co_entry(&cco);
>         } else {
>             co = qemu_coroutine_create(bdrv_create_co_entry, &cco);
>             qemu_coroutine_enter(co);
>             while (cco.ret == NOT_DONE) {
>                 aio_poll(qemu_get_aio_context(), true);
>             }
>         }
> 
>         ret = cco.ret;
>         if (ret < 0) {
>             if (cco.err) {
>                 error_propagate(errp, cco.err);
>             } else {
>                 error_setg_errno(errp, -ret, "Could not create image");
>             }
>         }
> 
>     out:
>         g_free(cco.filename);
>         return ret;
> 
>   If bdrv_find_protocol() returns null without setting an error, then
>   bdrv_create() returns -ENOENT without setting an error.
> 
>   One of its callers is bdrv_append_temp_snapshot():
> 
>     ret = bdrv_create(&bdrv_qcow2, tmp_filename, opts, errp);
>     qemu_opts_del(opts);
>     if (ret < 0) {
>         error_prepend(errp, "Could not create temporary overlay '%s': ",
>                       tmp_filename);
>         goto out;
>     }
> 
>   Crash.
> 
> Adding failure modes that use @errp differently than existing ones is
> playing with fire :)


Except this patch has nothing to do with the material presented here.

> 
>>> Please check all uses of your FOO_load_one() for this issue.

Will do again, but to me it looks unrelated.

>>
>> I have checked all instances multiple times. If there are issues please 
>> present them so I can address them.
> 
> I humbly suggest that instances in functions that set an error on
> failures before this patch need re-checking.

Will go over it again.

> 
>>>> diff --git a/block/dmg.c b/block/dmg.c
>>>> index 98db18d82a..11d184d39c 100644
>>>> --- a/block/dmg.c
>>>> +++ b/block/dmg.c
>>>> @@ -434,6 +434,7 @@ static int dmg_open(BlockDriverState *bs, QDict 
>>>> *options, int flags,
>>>>      uint64_t plist_xml_offset, plist_xml_length;
>>>>      int64_t offset;
>>>>      int ret;
>>>> +    Error *local_err = NULL;
>>>>  
>>>>      ret = bdrv_apply_auto_read_only(bs, NULL, errp);
>>>>      if (ret < 0) {
>>>> @@ -446,8 +447,21 @@ static int dmg_open(BlockDriverState *bs, QDict 
>>>> *options, int flags,
>>>>          return -EINVAL;
>>>>      }
>>>>  
>>>> -    block_module_load_one("dmg-bz2");
>>>> -    block_module_load_one("dmg-lzfse");
>>>> +    if (!block_module_load_one("dmg-bz2", &local_err)) {
>>>> +        if (local_err) {
>>>> +            error_report_err(local_err);
>>>> +            return -EINVAL;
>>>> +        }
>>>> +        warn_report("dmg-bz2 module not present, bz2 decomp unavailable");
>>>> +    }
>>>> +    local_err = NULL;
>>>> +    if (!block_module_load_one("dmg-lzfse", &local_err)) {
>>>> +        if (local_err) {
>>>> +            error_report_err(local_err);
>>>> +            return -EINVAL;
>>>> +        }
>>>> +        warn_report("dmg-lzfse module not present, lzfse decomp 
>>>> unavailable");
>>>> +    }
>>>>  
>>>>      s->n_chunks = 0;
>>>>      s->offsets = s->lengths = s->sectors = s->sectorcounts = NULL;
>>>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
>>>> index 0806d8fcaa..5902c59c94 100644
>>>> --- a/hw/core/qdev.c
>>>> +++ b/hw/core/qdev.c
>>>> @@ -148,7 +148,15 @@ bool qdev_set_parent_bus(DeviceState *dev, BusState 
>>>> *bus, Error **errp)
>>>>  DeviceState *qdev_new(const char *name)
>>>>  {
>>>>      if (!object_class_by_name(name)) {
>>>> -        module_load_qom_one(name);
>>>> +        Error *local_err = NULL;
>>>> +        if (!module_load_qom_one(name, &local_err)) {
>>>> +            if (local_err) {
>>>> +                error_report_err(local_err);
>>>> +            } else {
>>>> +                error_report("could not find a module for type '%s'", 
>>>> name);
>>>> +            }
>>>> +            abort();
>>>
>>> Why is this a programming error?
>>
>> As described before in your request for previous/after, this is a 
>> programming error because qdev_new is not expected to fail.
>> All QEMU users of qdev_new expect the API to return non-NULL and immediately 
>> use and dereference the pointer returned.
>> There is a separate API, qdev_try_new, for creating a device and allowing a 
>> NULL return value.
> 
> Well, then the programming error is "adding loadable device modules
> broke qdev_new()".
> 
> This is qdev_new() before commit 7ab6e7fcce "qdev: device module
> support":
> 
>     DeviceState *qdev_new(const char *name)
>     {
>         return DEVICE(object_new(name));
>     }
> 
> Precondition: @name is a valid device type name.  If it isn't, we fail
> an assertion.
> 
> If you got a @name that may not be valid, you must use qdev_try_new()
> instead:
> 
>     DeviceState *qdev_try_new(const char *name)
>     {
>         if (!object_class_by_name(name)) {
>             return NULL;
>         }
> 
>         return DEVICE(object_new(name));
>     }
> 
> The guard ensures we call object_new() only when the precondition holds.
> 
> Except it doesn't: when @name is a valid type name, but not a *device*
> type name, we still crash.  Bug.
> 
> Commit 7ab6e7fcce changed qdev_new() to
> 
>     DeviceState *qdev_new(const char *name)
>     {
>         if (!object_class_by_name(name)) {
>             module_load_qom_one(name);
>         }
>         return DEVICE(object_new(name));
>     }
> 
> Now the (enforced!) precondition is @name is a valid device type name
> and the device is either compiled in or it will succeed to load.
> 
> "Will succeed to load" is an inappropriate precondition.
> 
> Devices compiled as modules must be instantiated with qdev_try_new().
> Loadable module support should be dropped from qdev_new().

This should be a separate series / patch in my view, with bigger impact on the 
whole code.

> 
> Not your patch's fault, of course.  By the way, welcome to the swamp!
> Do not mind the crocodiles, they just want to play.
> 
>>>> +        }
>>>>      }
>>>>      return DEVICE(object_new(name));
>>>>  }
>>>> diff --git a/include/qemu/module.h b/include/qemu/module.h
>>>> index 8c012bbe03..78d4c4de96 100644
>>>> --- a/include/qemu/module.h
>>>> +++ b/include/qemu/module.h
>>>> @@ -61,16 +61,44 @@ typedef enum {
>>>>  #define fuzz_target_init(function) module_init(function, \
>>>>                                                 MODULE_INIT_FUZZ_TARGET)
>>>>  #define migration_init(function) module_init(function, 
>>>> MODULE_INIT_MIGRATION)
>>>> -#define block_module_load_one(lib) module_load_one("block-", lib)
>>>> -#define ui_module_load_one(lib) module_load_one("ui-", lib)
>>>> -#define audio_module_load_one(lib) module_load_one("audio-", lib)
>>>> +#define block_module_load_one(lib, errp) module_load_one("block-", lib, 
>>>> errp)
>>>> +#define ui_module_load_one(lib, errp) module_load_one("ui-", lib, errp)
>>>> +#define audio_module_load_one(lib, errp) module_load_one("audio-", lib, 
>>>> errp)
>>>>  
>>>>  void register_module_init(void (*fn)(void), module_init_type type);
>>>>  void register_dso_module_init(void (*fn)(void), module_init_type type);
>>>>  
>>>>  void module_call_init(module_init_type type);
>>>> -bool module_load_one(const char *prefix, const char *lib_name);
>>>> -void module_load_qom_one(const char *type);
>>>> +
>>>> +/*
>>>> + * module_load_one: attempt to load a module from a set of directories
>>>> + *
>>>> + * directories searched are:
>>>> + * - getenv("QEMU_MODULE_DIR")
>>>> + * - get_relocated_path(CONFIG_QEMU_MODDIR);
>>>> + * - /var/run/qemu/${version_dir}
>>>> + *
>>>> + * prefix:         a subsystem prefix, or the empty string ("audio-", 
>>>> ..., "")
>>>> + * name:           name of the module
>>>> + * errp:           error to set in case the module is found, but load 
>>>> failed.
>>>> + *
>>>> + * Return value:   true on success (found and loaded);
>>>> + *                 if module if found, but load failed, errp will be set.
>>>> + *                 if module is not found, errp will not be set.
>>>
>>> I understand you need to distingush two failure modes "found, but load
>>> failed" and "not found".
>>
>> That is correct.
>>
>>>
>>> Functions that set an error on some failures only tend to be awkward: in
>>> addition to checking the return value for failure, you have to check
>>> @errp for special failures.  This is particularly cumbersome when it
>>> requires a @local_err and an error_propagate() just for that.  I
>>> generally prefer to return an error code and always set an error.
>>>
>>> That said, the patch doesn't look bad.  Perhaps it will be once the
>>> issues I pointed out above have been addressed.  Let's not worry about
>>> it right now.
>>>
>>>> + */
>>>> +bool module_load_one(const char *prefix, const char *name, Error **errp);
>>>> +
>>>> +/*
>>>> + * module_load_qom_one: attempt to load a module to provide a QOM type
>>>> + *
>>>> + * type:           the type to be provided
>>>> + * errp:           error to set.
>>>> + *
>>>> + * Return value:   true on success (found and loaded), false on failure.
>>>> + *                 If a module is simply not found for the type,
>>>> + *                 errp will not be set.
>>>> + */
>>>> +bool module_load_qom_one(const char *type, Error **errp);
>>>>  void module_load_qom_all(void);
>>>>  void module_allow_arch(const char *arch);
>>>>  
>>>
>>> [...]
>>>
>>>
>>> [*] Except it prints "Module is not supported by system\n" to stderr
>>> when !g_module_supported(), which doesn't look right to me.
>>>
>> I don't know what this comment relates to.
> 
> It's a footnote attached to "Before the patch, bdrv_find_format() fails
> silently[*]" above.
> 
> 
> I think the root of the problems with this patch is that you convert the
> FOO_module_load_one() functions to the Error API without propagating the
> error far enough.  It really needs to go all the way up.
> 

I respecfully disagree. Sometimes I think we need to be practical; there is for 
sure further improvement possible,
a redesign of the Error API to be more consistent with return values,
rethinking about use of qdev_new vs try_qdev_new() etc,

but in my view the series is complete and self-consistent
(and btw fixes an actual bug in your CentOS docker / kubevirt).

I'll review all the changes and see if merging some errors make sense in some 
cases, but generally I would recommend against it.

Further improvements can and should be done but in my view should not be part 
of this series,
I cannot honestly take at the moment that kind of work item.

Thanks,

Claudio