Re: [PATCH v2 06/21] migration: Add Error** argument to .save_setup() handler

[Vladimir Sementsov-Ogievskiy]

On 29.02.24 16:21, Markus Armbruster wrote:
> Thomas Huth <thuth@redhat.com> writes:
>
> > On 29/02/2024 08.20, Vladimir Sementsov-Ogievskiy wrote:
> > > On 29.02.24 09:32, Markus Armbruster wrote:
>
> [...]
>
> > > > Anti-pattern: fail without setting an error.  There might be more
> > > > elsewhere in the series.
> > > >
> > > > qapi/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, ...).
> > > >    *
> > > >    * - Whenever practical, also return a value that indicates success /
> > > >    *   failure.  This can make the error checking more concise, and can
> > > >    *   avoid useless error object creation and destruction.  Note that
> > > >    *   we still have many functions returning void.  We recommend
> > > >    *   • bool-valued functions return true on success / false on failure,
> > > >    *   • pointer-valued functions return non-null / null pointer, and
> > > >    *   • integer-valued functions return non-negative / negative.
> > > >
> > > >          }
> > > >          qemu_put_be64(f, STATTR_FLAG_EOS);
> > > >          return 0;
> > > >      }
> > > >
> > > > When adding Error **errp to a function, you must also add code to set an
> > > > error on failure to every failure path.  Adding it in a later patch in
> > > > the same series can be okay,
> > >
> > > Personally, I'd prefer not doing so. Creating wrong commits and fixing them in 
> > > same series - better to merge all fixes into bad commit:)
> >
> > I agree - that might create issues with bisecting later. Please fix it in this 
> > patch here already!
>
> Depends on the wrongness, really.

In my understanding, unset errp on failure path is wrong enough.

For example, simple pattern

Error *local_err = NULL;

int ret = foo(&local_err);
if (ret < 0) {
    error_report_err(local_err);
    return;
}

will just crash.

When I write code, I expect that "errp is set" === "ret < 0", for all functions 
returning negative integer on failure.

Also, we have enough code that relying on errp for failure checking:
$ git grep 'if (local_err)' | wc -l
373

Of course, most of this should be checking failure of functions that return 
void, but who knows.

> We don't want broken intermediate states, no argument.
>
> But intermediate states that are merely unclean can be acceptable.
>
> For instance, my commit a30ecde6e79 (net: Permit incremental conversion
> of init functions to Error) added such Error ** parameters to a somewhat
> tangled nest of functions, along with FIXME comments where errors
> weren't set.  The next few commits fixed most, but not all of them.
> Later commits fixed some more.  The one in tap-win32.c is still there
> today.
>
> This was acceptable, because it improved things from "bad error
> reporting" to "okay error reporting in most cases, unclean and bad error
> reporting in a few cases marked FIXME", with "a few" over time going
> down to the one I can't test, and nobody else seems to care about.

--
Best regards,
Vladimir