7:04 a.m.
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