[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v4 2/3] module: add Error arguments to module_load_one and mo
From: |
Claudio Fontana |
Subject: |
Re: [PATCH v4 2/3] module: add Error arguments to module_load_one and module_load_qom_one |
Date: |
Thu, 22 Sep 2022 11:21:56 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.4.0 |
On 9/22/22 11:20, Claudio Fontana wrote:
> On 9/22/22 10:28, Daniel P. Berrangé wrote:
>> On Thu, Sep 22, 2022 at 08:07:43AM +0200, Markus Armbruster wrote:
>>> Ease of use matters, too. When sticking to the rule leads to awkward
>>> code, we should stop and think. Should we deviate from the rule? Or
>>> can we avoid that by tweaking the interface?
>>>
>>> Philippe's proposed interface sticks to the rule.
>>
>> The cost is that when you see a function dosomething(true|false) as
>> a reader you often have no idea what the effect of true vs false is
>> on the behaviour of that function. You resort to looking at the
>> API docs and/or code. This is where C would really benefit from
>> having named parameters like as dosomething(ignore_errors=true|false)
>> is totally obvious. Anyway, I digress.
>
> The confusion here I think stems from the fact that not finding a module is
> _NORMAL BEHAVIOR_.
>
> We can configure the qemu package once including configuration for all
> modules,
> and then have the packager (or user) install the modules needed.
>
> We should break away from the easy-to-lean-to mindset that
>
> not finding a module => error path
>
> Because this is not the case. This is what is being confused in this
> discussion.
>
> Distinguishing the normal execution path from the error path (exception, in
> C++ parlance),
>
> we are just hindered by the fact that C can only have one return value.
>
>
>>
>>> Another interface that does: return -1 for error, 0 for module not found
>>> (no error), and 1 for loaded.
>>
>> IMHO this pattern is generally easier to understand when looking at
>> the callers, as the fatal error scenario is always clear.
>>
>> That said I would suggest neither approach as the public facing
>> API. Rather stop trying to overload 3 states onto an error reporting
>> pattern that inherantly wants to be 2 states. Instead just have
>> distinct methods
>>
>> bool module_load_one(const char *prefix, const char *name, Error *errp)
>> bool module_try_load_one(const char *prefix, const char *name, Error *errp)
>
>
> Here we are murking again the normal behavior and the error path.
>
> What is the meaning of try? It's not as though we would error out inside the
> function module_load_one,
> it's the _caller_ that needs to decide how to treat a return value of
> found/not found, and the exception (Error).
>
> If this makes it clearer, lets keep the existing Error API pattern of using
> both the return value and the Error parameter for the error (exception),
> and put the NORMAL BEHAVIOR error value in an argument using a pointer.
>
> We do not pass a "bool ignore_errors" , because that is again confusing the
> fact that it is not module_load_one that handles the errors,
> module_load_one should neither handle nor ignore the errors,
> it should generate an error in the error case, and a return value in the
> normal case.
>
> What about:
>
> /*
>
> * 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 CONDITION): errp will be set on module load error.
> * found: (output): set to true if a module with this name has been
> found, false if no such module is present.
> *
>
> * Return value: true if no error encountered (module loaded or not
> present).
> * false if an error has been generated, and errp will be set
> with the Error.
> */
>
Now with the missing prototype:
bool module_load_one(const char *prefix, const char *name, Error *errp, bool
*found);
> Thanks,
>
> C
>
>
>>
>> other names are available for the second, eg module_load_one_optional()
>>
>> Internally, both would call into a common helper following either
>> Philippe's idea, or the -1/0/1 int return value. Either is fine,
>> as they won't be exposed to any caller.
>>
>> With regards,
>> Daniel
>
- Re: [PATCH v4 2/3] module: add Error arguments to module_load_one and module_load_qom_one, (continued)
- Re: [PATCH v4 2/3] module: add Error arguments to module_load_one and module_load_qom_one, Philippe Mathieu-Daudé, 2022/09/19
- Re: [PATCH v4 2/3] module: add Error arguments to module_load_one and module_load_qom_one, Markus Armbruster, 2022/09/21
- Re: [PATCH v4 2/3] module: add Error arguments to module_load_one and module_load_qom_one, Claudio Fontana, 2022/09/21
- Re: [PATCH v4 2/3] module: add Error arguments to module_load_one and module_load_qom_one, Markus Armbruster, 2022/09/22
- Re: [PATCH v4 2/3] module: add Error arguments to module_load_one and module_load_qom_one, Daniel P . Berrangé, 2022/09/22
- Re: [PATCH v4 2/3] module: add Error arguments to module_load_one and module_load_qom_one, Claudio Fontana, 2022/09/22
- Re: [PATCH v4 2/3] module: add Error arguments to module_load_one and module_load_qom_one,
Claudio Fontana <=
- Re: [PATCH v4 2/3] module: add Error arguments to module_load_one and module_load_qom_one, Claudio Fontana, 2022/09/22
- Re: [PATCH v4 2/3] module: add Error arguments to module_load_one and module_load_qom_one, Daniel P . Berrangé, 2022/09/22
- Re: [PATCH v4 2/3] module: add Error arguments to module_load_one and module_load_qom_one, Claudio Fontana, 2022/09/22
- Re: [PATCH v4 2/3] module: add Error arguments to module_load_one and module_load_qom_one, Daniel P . Berrangé, 2022/09/22
- Re: [PATCH v4 2/3] module: add Error arguments to module_load_one and module_load_qom_one, Claudio Fontana, 2022/09/22
- Re: [PATCH v4 2/3] module: add Error arguments to module_load_one and module_load_qom_one, Daniel P . Berrangé, 2022/09/22
- Re: [PATCH v4 2/3] module: add Error arguments to module_load_one and module_load_qom_one, Claudio Fontana, 2022/09/22
- Re: [PATCH v4 2/3] module: add Error arguments to module_load_one and module_load_qom_one, Markus Armbruster, 2022/09/22
- Re: [PATCH v4 2/3] module: add Error arguments to module_load_one and module_load_qom_one, Claudio Fontana, 2022/09/22
- Re: [PATCH v4 2/3] module: add Error arguments to module_load_one and module_load_qom_one, Markus Armbruster, 2022/09/22