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

[Markus Armbruster <armbru@redhat.com>]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 22.09.2022 um 14:42 hat Markus Armbruster geschrieben:

[...]

>> If you have callers that need to distinguish between not found, found
>> but bad, found and good, then return three distinct values.
>> 
>> I proposed to return -1 for found but bad (error), 0 for not found (no
>> error), and 1 for loaded (no error).
>
> My intuition with this one was that "not found" is still an error case,
> but it's an error case that we need to distinguish from other error
> cases.
>
> Is this one of the rare use cases for error classes?

If I remember correctly, "not found" is not actually an error for most
callers.  If we make it one, these callers have to error_free().  Minor
annoyance, especially when you have to add an else just for that.

Even if we decide to make it an error, I would not create an error class
for it.  I like

    rc = module_load_one(..., errp);
    if (rc == -ENOENT) {
        error_free(*errp);
    } else if (rc < 0) {
        return;
    }

better than

    Error *err = NULL;

    module_load_one(..., &err);
    if (err && err->class == ERROR_CLASS_NOT_FOUND) {
        error_free(err);
    } else if (err) {
        error_propagate(errp, err);
        return;
    }

I respect your intuition.  Would it still say "error case" when the
function is called module_load_if_exists()?

Hmm, another thought... a need to distinguish error cases can be a
symptom of trying to do too much in one function.  We could split this
into "look up module" and "actually load it".