Markus Armbruster <[email protected]> writes: > Jonathan Cameron <[email protected]> writes: > >> On Fri, 8 Aug 2025 10:08:14 +0200 >> Markus Armbruster <[email protected]> wrote: >> >>> Functions that use an Error **errp parameter to return errors should >>> not also report them to the user, because reporting is the caller's >>> job. When the caller does, the error is reported twice. When it >>> doesn't (because it recovered from the error), there is no error to >>> report, i.e. the report is bogus. >>> >>> cxl_fmws_link_targets() violates this principle: it calls >>> error_setg(&error_fatal, ...) via cxl_fmws_link(). Goes back to >>> commit 584f722eb3ab (hw/cxl: Make the CXL fixed memory windows >>> devices.) Currently harmless, because cxl_fmws_link_targets()'s >>> callers always pass &error_fatal. Clean this up by converting >>> cxl_fmws_link() to Error. >> >> Patch is definitely an improvement though I'm no sure how >> it is really a violation of the above principle given >> it has no effect on being called twice for example. > > Note I wrote "Clean this up", not "fix this" :) > > This is actually a canned commit message I've been using with suitable > adjustments for similar patches: commit b765d21e4ab, 35b1561e3ec, > e6696d3ee9b, 07d5b946539, ... > >>> Cc: Jonathan Cameron <[email protected]> >>> Signed-off-by: Markus Armbruster <[email protected]> >> >> The -1 return is perhaps unrelated to the main thing here, >> but does make more sense than return 1 so fair enough. > > Accident, will back it out.
Changed my mind: I'm keeping it, with rationale in the commit message. >> None of the above comments I've raised are that important to me though. >> >> Reviewed-by: Jonathan Cameron <[email protected]> > > Thanks!
