Re: [Qemu-devel] [libvirt] [PATCH 2/2] qapi: deprecate implicit filters

[Kevin Wolf <kwolf@redhat.com>]

Am 30.08.2019 um 20:11 hat John Snow geschrieben:
> 
> 
> On 8/30/19 6:07 AM, Christophe de Dinechin wrote:
> > Without having looked at the code much, I think I would
> > 
> > 1. extend the existing QAPI error to support warnings, deprecations and
> >    info messages. The first problem I see is that there is no error, so
> >    we may sometimes need to create one when there was none before. And
> >    of course make sure that this does not ultimately show as an error,
> >    but as a success with additional annotations.
> > 
> 
> I assume this might be a chance to consolidate all of the methodologies
> we use for actually checking if there was an error or not. There have
> been many and I am sure Markus can give us a history lesson if it's
> warranted.
> 
> Generally, there's a few paradigms I see a lot:
> 
> 1. Rely on an error return code being produced by the called function.
> The caller trusts that errp was set. This is one of my favorite methods,
> because it has the least scaffolding.

This one is convenient to use, but the problem is that nobody enforces
that errp is always set if ret < 0, and that it's not set for ret == 0.
So in a way it is error-prone because it allows inconsistencies.

> 2. Pass errp directly to the called function, and check for null after
> return. I don't like this method very much, because of confusion with:

I mainly don't like this very much because it's simply wrong.

Callers can pass errp = NULL if they aren't interested in error
information. If you directly pass errp, you can't check *errp because
errp could be NULL.

So directly passing errp makes the code simpler, but only use it in
functions where you don't intend to check whether an error is set.

> 3. Create a local error object; check THAT for null, and propagate the
> error to the common error object. I think Markus has explained why we
> have this code 50 times, and I forget again minutes later.

local_err exists for those cases where you need to check the error
object before passing it to the caller. (And obviously for those cases
where you don't want to pass it to the caller, but do something like
error_report_err().)

> If we want to expand the concept of the error object into something that
> encompasses hints, warnings and deprecations*, checking for null is no
> longer appropriate. It might be a good chance to make our error
> propagation story more consistent, too.
> 
> We could unify with a helper like this, I think, if I'm not forgetting
> some crucial usage detail:
> 
> subroutine(foo, bar, errp);
> if (failure(errp)) {
>     error_append_hint(errp, "Lorem ipsum, ...");
>     cleanup();
>     return;
> }
> 
> We would then always use this pattern that operates directly on the
> caller's errp instead of creating local error objects to allow hints and
> warnings to accumulate.

There are two parts to the change that you imply:

1. Forbid passing errp = NULL to any function so that *errp can always
   be checked. This gets rid of local_err in the intermediate function,
   but may require the introduction of new local_err variables in
   top-level callers which ignore the error information.

2. Introduce failure(errp) to replace errp != NULL because we want Error
   to contain warnings and notes, too. Currently, it can contain only
   exactly one error, so this would be a major change.

   Note that the change wouldn't make the existing 'if (errp)' checks
   build failures, so getting confident that we found and replaced all
   of them is going to be hard.

Essentially, you'd probably want to replace Error with a new type so
that the compiler will at least be able to tell which places have been
converted and which haven't.

And then, you'd have to touch every single function that does something
with errors. This is a huge change across the whole source tree.

I doubt it's worth the effort.

> > Second, why not report the use of deprecated features? I don't fully buy
> > the rationale that libvirt engages the features, because it does not do
> > it on its own, it does it because the user made some specific request.
> 
> Because the user didn't request those specific QMP features, they asked
> for the VM to start, or to stop, or they asked for a backup, or a snapshot.
> 
> On my desktop, I am not really too interested in knowing if XFCE is
> using deprecated features of xorg or wayland. I didn't tell it to use
> them and I have no real power or control over that. It's nice if I'm a
> developer, but as a user, it's noise.
> 
> So a development log seems right for these, but not user-visible
> interruptions.

I agree, it's not the high-level operation the user requested that is
deprecated, but just the specific implementation libvirt uses to perform
the operation on a QEMU VM.

The expected response to a deprecation notice is that a libvirt update
makes it go away by using more modern interfaces, not that the user
changes their workflow.

Kevin