On 9/22/22 14:33, Daniel P. Berrangé wrote: > On Thu, Sep 22, 2022 at 02:30:35PM +0200, Claudio Fontana wrote: >> On 9/22/22 12:37, Daniel P. Berrangé wrote: >>> On Thu, Sep 22, 2022 at 11:34:22AM +0200, Claudio Fontana wrote: >>>> On 9/22/22 11:31, Daniel P. Berrangé wrote: >>>>> On Thu, Sep 22, 2022 at 11:20:07AM +0200, Claudio Fontana wrote: >>>>>> On 9/22/22 10:28, Daniel P. Berrangé wrote: >>>>>>> >>>>>>>> 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). >>>>> >>>>> I suggested "try" as in the g_malloc vs g_try_malloc API naming pattern, >>>>> where the latter ignores the OOM error condition. >>>>> >>>>> So in this case 'try' means try to load the module, but don't fail if >>>>> the module is missing on disk. >>>> >>>> I understand what you mean, but this is wrong in this case. >>>> >>>> We _do not fail_ in module_load_one, whether an error is present >>>> or not, whether a module is found or not. >>> >>> Looking at the callers though, AFAIK there are only two patterns >>> that we need. All callers should report a fatal error if the module >>> exists and loading it failed eg module is from mis-matched build. >>> >>> Some callers also want a failure if the module doesn't exist on >> >> Some callers also want to behave differently if the module is not installed. >> It is not a "failure" in the same sense that what an Error returns. >> >> For example, the block dmg module tries to also load other submodules for >> decompression. >> >> If dmg does not find any such submodules, it will be able to support basic >> dmg just fine, >> but won't be able to open compressed dmg. > > The dmg case looks like it works fine with module_try_load_one(). I > know your patch does a "warn" when the module is missing currently, > but IMHO that's the wrong place todo this. Any problems with use > of compressed dmg should be reported at the time the feature is > actually used, rather than when trying to load the module.
Right, this was the input from Kevin as well, I planned to change that. We should warn only when we actually encounter/decode a compressed dmg. Thanks, Claudio