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

[John Snow]

On 8/30/19 6:07 AM, Christophe de Dinechin wrote:
> 
> John Snow writes:
> 
>> On 8/29/19 12:45 PM, Christophe de Dinechin wrote:
>>>
> [...]
> 
>>> Sorry for catching up late, this mail thread happened during my PTO.
>>>
>>> I remember bringing up at the time [1] that the correct solution needs
>>> to take into account usage models that vary from
>>>
>>> - a workstation case, where displaying an error box is easy and
>>>   convenient,
>>>
>>> - to local headless VMs where system-level notification would do the job
>>>   better, allowing us to leverage things like system-wide email 
>>> notifications
>>>
>>> - to large-scale collections of VMs managed by some layered product,
>>>   where the correct reporting would be through something like Insights,
>>>   i.e. you don't scan individual logs, you want something like "913 VMs
>>>   are using deprecated X"
>>>
>>> To me, that implies that we need to have a clear division of roles, with
>>> a standard way to
>>>
>>> a) produce the errors,
>>> b) propagate them,
>>
>> I started replying to this thread to the other mail you sent; I think
>> this is going to be fairly involved. I wouldn't mind being proven wrong
>> though.
> 
> Yes, I think it does look involved, but mostly for historical reasons.
> In other words, what is complicated is preserving the historical
> behaviors so as to not break existing consumers.
> 
>>
>>> c) consume them (at least up to libvirt)
>>>
>>> Notice that this work has already been done for "real" errors,
>>> i.e. there is a real QAPI notion of "errors". AFAICT, warn_report does
>>> not connect to it, though, it goes through error_vprintf which is really
>>> just basic logging.
>>>
>>> So would it make sense to:
>>>
>>> 1. Add a deprecation_report() alongside warn_report()?
>>>
>>
>> Where's that get routed to? just an error_vprintf style situation?
> 
> Yes, but see below.
> 
>>
>>> 2. Connect warn_report() and all the error_vprintf output to QAPI,
>>>    e.g. using John's suggestion of adding the messages using some
>>>    "warning" or "deprecated" tag?
>>>
>>
>> How do you correlate them?
> 
> 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.

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:

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.


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.



(* I'm not proposing all three in a concrete way, but just referencing
the general class of semantic non-error information we may want to
propagate, however we end up deciding to model it.)

> 2. replace the current "link + if" switching for error_vprintf with some
>    actual notification mechanism, with one option routine to
>    monitor_vprintf, one to stderr, one to log file, and then an
>    additional path that would post a newly minted qapi warning.
> 
>>
>>> 3. Teach libvirt how to consume that new tag and pass it along?
>>>
>>
>> I think it's not libvirt's job to pass it along, exactly -- libvirt made
>> the decision for which features to engage in QEMU, not the end user.
> 
> First, by "pass along", I meant to possible layered products or
> management software. We don't necessarily need a new virErrorLevel,
> deprecation could be a warning with some special domain,
> e.g. VIR_FROM_DEPRECATION.
> 

SHOULD it pass it along? Libvirt knows what QMP is invoking, that should
be opaque to upper layers. a "DEPRECATION" log stream might give the
wrong idea -- that it's not the libvirt feature that's deprecated, but
something in one of the many drivers it has.

I think it's fine to relay this information via generic logging to the
higher levels (so that debugging can be facilitated from that level) but
I don't think it needs special access to these notices because they're
not useful to RHV developers or RHV users.

> There may be a need to add some API here. Looking at the code, it's not
> obvious to me that libvirt has any notion of error priority. In other
> words, if you raise an error then a warning, you get the warning as the
> last error, right?
> 

(Don't know.)

> 
> 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.

> This point of view also seems to require that libvirt or the user should
> know ahead of time it's about to engage a deprecated feature. To me, the
> problem is precisely that neither libvirt nor the user knows, which is
> why we are discussing how to best make it known.

Yeah, sure. I'm still a fan of the end result of having QMP return
messages including that information. I just don't think it makes sense
for libvirt to then relay it to the user or up the stack. I don't think
that's the same as proposing that it hides it, either.

> 
>>
>> If the user upgrades QEMU but not libvirt, it's not really anything they
>> have control over and they shouldn't be pestered with such things.
>>
>> However, if libvirt accidentally released a version that engages
>> deprecated behavior (and were unaware of it), it'd be nice to get user
>> reports, surely?
>>
>> Logging messages for libvirt might be the best that can be done there in
>> that case.
> 
> I personally would treat that like any warning.
> 
>>
>>
>> In contrast, power user tools like QMP libraries, qmp-shell and others
>> allow more direct and meaningful access to QMP, so those should report
>> deprecation messages to the user.
> 
> Agreed.
> 
>>
>>>
>>> [1] https://lists.nongnu.org/archive/html/qemu-devel/2018-10/msg06131.html
>>>
> 
> --
> Thanks,
> Christophe de Dinechin (IRC c3d)
>