Re: [libvirt] [Qemu-devel] [PATCH 2/2] qapi: deprecate implicit filters
> On 22 Nov 2019, at 09:41, Markus Armbruster wrote: > > Reviving this old thread, because I'd like to connect it to more recent > discussions. > > Christophe de Dinechin mailto:dinec...@redhat.com>> > writes: > >> Markus Armbruster writes: >> >>> Peter Krempa writes: >>> >> [...] From my experience users report non-fatal messages mostly only if it is spamming the system log. One of instances are very unlikely to be noticed. In my experience it's better to notify us in libvirt of such change and we will try our best to fix it. >>> >>> How to best alert the layers above QEMU was one of the topic of the KVM >>> Forum 2018 BoF on deprecating stuff. Minutes: >>> >>>Message-ID: <87mur0ls8o@dusky.pond.sub.org> >>>https://lists.nongnu.org/archive/html/qemu-devel/2018-10/msg05828.html >>> >>> Relevant part: >>> >>> * We need to communicate "you're using something that is deprecated". >>> How? Right now, we print a deprecation message. Okay when humans use >>> QEMU directly in a shell. However, when QEMU sits at the bottom of a >>> software stack, the message will likely end up in a log file that is >>> effectively write-only. >>> >>> - The one way to get people read log files is crashing their >>>application. A command line option --future could make QEMU crash >>>right after printing a deprecation message. This could help with >>>finding use of deprecated features in a testing environment. >>> >>> - A less destructive way to grab people's attention is to make things >>>run really, really slow: have QEMU go to sleep for a while after >>>printing a deprecation message. >>> >>> - We can also pass the buck to the next layer up: emit a QMP event. >>> >>>Sadly, by the time the next layer connects to QMP, plenty of stuff >>>already happened. We'd have to buffer deprecation events somehow. >>> >>>What would libvirt do with such an event? Log it, taint the domain, >>>emit a (libvirt) event to pass it on to the next layer up. >>> >>> - A completely different idea is to have a configuratin linter. To >>>support doing this at the libvirt level, QEMU could expose "is >>>deprecated" in interface introspection. Feels feasible for QMP, >>>where we already have sufficiently expressive introspection. For >>>CLI, we'd first have to provide that (but we want that anyway). >>> >>> - We might also want to dispay deprecation messages in QEMU's GUI >>>somehow, or on serial consoles. >> >> 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, >> 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()? > > "Grepability" alone would make that worthwhile, I think. > >> 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? > > This is the difficult part. See my "Exposing feature deprecation to > machine clients (was: [Qemu-devel] [PATCH 2/2] qapi: deprecate implicit > filters)" in this thread. Quote: > >Propagating errors is painful. It has caused massive churn all over the >place. > >I don't think we can hitch deprecation info to the existing error >propagation, since we need to take the success path back to the QMP >core, not an error path. > >Propagating a second object for warnings... thanks, but no thanks. > >The QMP core could provide a function for recording deprecation info for >the currently executing QMP command. This is how we used to record >errors in QMP commands, until Anthony rammed through what we have now. >The commit messages (e.g. d5ec4f27c38) provide no justification. I >dimly recall adamant (oral?) claims that recording errors in the Monitor >object cannot work for us. > >I smell a swamp. This looks
Re: [libvirt] [Qemu-devel] [PATCH 2/2] qapi: deprecate implicit filters
Reviving this old thread, because I'd like to connect it to more recent discussions. Christophe de Dinechin writes: > Markus Armbruster writes: > >> Peter Krempa writes: >> > [...] >>> From my experience users report non-fatal messages mostly only if it is >>> spamming the system log. One of instances are very unlikely to be >>> noticed. >>> >>> In my experience it's better to notify us in libvirt of such change and >>> we will try our best to fix it. >> >> How to best alert the layers above QEMU was one of the topic of the KVM >> Forum 2018 BoF on deprecating stuff. Minutes: >> >> Message-ID: <87mur0ls8o@dusky.pond.sub.org> >> https://lists.nongnu.org/archive/html/qemu-devel/2018-10/msg05828.html >> >> Relevant part: >> >> * We need to communicate "you're using something that is deprecated". >> How? Right now, we print a deprecation message. Okay when humans use >> QEMU directly in a shell. However, when QEMU sits at the bottom of a >> software stack, the message will likely end up in a log file that is >> effectively write-only. >> >> - The one way to get people read log files is crashing their >> application. A command line option --future could make QEMU crash >> right after printing a deprecation message. This could help with >> finding use of deprecated features in a testing environment. >> >> - A less destructive way to grab people's attention is to make things >> run really, really slow: have QEMU go to sleep for a while after >> printing a deprecation message. >> >> - We can also pass the buck to the next layer up: emit a QMP event. >> >> Sadly, by the time the next layer connects to QMP, plenty of stuff >> already happened. We'd have to buffer deprecation events somehow. >> >> What would libvirt do with such an event? Log it, taint the domain, >> emit a (libvirt) event to pass it on to the next layer up. >> >> - A completely different idea is to have a configuratin linter. To >> support doing this at the libvirt level, QEMU could expose "is >> deprecated" in interface introspection. Feels feasible for QMP, >> where we already have sufficiently expressive introspection. For >> CLI, we'd first have to provide that (but we want that anyway). >> >> - We might also want to dispay deprecation messages in QEMU's GUI >> somehow, or on serial consoles. > > 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, > 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()? "Grepability" alone would make that worthwhile, I think. > 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? This is the difficult part. See my "Exposing feature deprecation to machine clients (was: [Qemu-devel] [PATCH 2/2] qapi: deprecate implicit filters)" in this thread. Quote: Propagating errors is painful. It has caused massive churn all over the place. I don't think we can hitch deprecation info to the existing error propagation, since we need to take the success path back to the QMP core, not an error path. Propagating a second object for warnings... thanks, but no thanks. The QMP core could provide a function for recording deprecation info for the currently executing QMP command. This is how we used to record errors in QMP commands, until Anthony rammed through what we have now. The commit messages (e.g. d5ec4f27c38) provide no justification. I dimly recall adamant (oral?) claims that recording errors in the Monitor object cannot work for us. I smell a swamp. Can we avoid plumbing deprecation info from command code to QMP core? Only if the QMP core itself can recognize deprecated interfaces. I believe it can for the cases we can expose in introspecion. Let me explain.
Re: [libvirt] [Qemu-devel] [PATCH 2/2] qapi: deprecate implicit filters
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
--
libvir-list mailing list
libvir-list@redhat.com
Re: [libvirt] [Qemu-devel] [PATCH 2/2] qapi: deprecate implicit filters
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"
Re: [libvirt] [Qemu-devel] [PATCH 2/2] qapi: deprecate implicit filters
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. 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. 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? 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. 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. > > 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) -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] [PATCH 2/2] qapi: deprecate implicit filters
On 8/29/19 12:45 PM, Christophe de Dinechin wrote: > > Markus Armbruster writes: > >> Peter Krempa writes: >> > [...] >>> From my experience users report non-fatal messages mostly only if it is >>> spamming the system log. One of instances are very unlikely to be >>> noticed. >>> >>> In my experience it's better to notify us in libvirt of such change and >>> we will try our best to fix it. >> >> How to best alert the layers above QEMU was one of the topic of the KVM >> Forum 2018 BoF on deprecating stuff. Minutes: >> >> Message-ID: <87mur0ls8o@dusky.pond.sub.org> >> https://lists.nongnu.org/archive/html/qemu-devel/2018-10/msg05828.html >> >> Relevant part: >> >> * We need to communicate "you're using something that is deprecated". >> How? Right now, we print a deprecation message. Okay when humans use >> QEMU directly in a shell. However, when QEMU sits at the bottom of a >> software stack, the message will likely end up in a log file that is >> effectively write-only. >> >> - The one way to get people read log files is crashing their >> application. A command line option --future could make QEMU crash >> right after printing a deprecation message. This could help with >> finding use of deprecated features in a testing environment. >> >> - A less destructive way to grab people's attention is to make things >> run really, really slow: have QEMU go to sleep for a while after >> printing a deprecation message. >> >> - We can also pass the buck to the next layer up: emit a QMP event. >> >> Sadly, by the time the next layer connects to QMP, plenty of stuff >> already happened. We'd have to buffer deprecation events somehow. >> >> What would libvirt do with such an event? Log it, taint the domain, >> emit a (libvirt) event to pass it on to the next layer up. >> >> - A completely different idea is to have a configuratin linter. To >> support doing this at the libvirt level, QEMU could expose "is >> deprecated" in interface introspection. Feels feasible for QMP, >> where we already have sufficiently expressive introspection. For >> CLI, we'd first have to provide that (but we want that anyway). >> >> - We might also want to dispay deprecation messages in QEMU's GUI >> somehow, or on serial consoles. > > 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. > 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? > 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? > 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. 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. 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. > > [1] https://lists.nongnu.org/archive/html/qemu-devel/2018-10/msg06131.html > > > -- > Cheers, > Christophe de Dinechin (IRC c3d) > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] [PATCH 2/2] qapi: deprecate implicit filters
Markus Armbruster writes: > Peter Krempa writes: > [...] >> From my experience users report non-fatal messages mostly only if it is >> spamming the system log. One of instances are very unlikely to be >> noticed. >> >> In my experience it's better to notify us in libvirt of such change and >> we will try our best to fix it. > > How to best alert the layers above QEMU was one of the topic of the KVM > Forum 2018 BoF on deprecating stuff. Minutes: > > Message-ID: <87mur0ls8o@dusky.pond.sub.org> > https://lists.nongnu.org/archive/html/qemu-devel/2018-10/msg05828.html > > Relevant part: > > * We need to communicate "you're using something that is deprecated". > How? Right now, we print a deprecation message. Okay when humans use > QEMU directly in a shell. However, when QEMU sits at the bottom of a > software stack, the message will likely end up in a log file that is > effectively write-only. > > - The one way to get people read log files is crashing their > application. A command line option --future could make QEMU crash > right after printing a deprecation message. This could help with > finding use of deprecated features in a testing environment. > > - A less destructive way to grab people's attention is to make things > run really, really slow: have QEMU go to sleep for a while after > printing a deprecation message. > > - We can also pass the buck to the next layer up: emit a QMP event. > > Sadly, by the time the next layer connects to QMP, plenty of stuff > already happened. We'd have to buffer deprecation events somehow. > > What would libvirt do with such an event? Log it, taint the domain, > emit a (libvirt) event to pass it on to the next layer up. > > - A completely different idea is to have a configuratin linter. To > support doing this at the libvirt level, QEMU could expose "is > deprecated" in interface introspection. Feels feasible for QMP, > where we already have sufficiently expressive introspection. For > CLI, we'd first have to provide that (but we want that anyway). > > - We might also want to dispay deprecation messages in QEMU's GUI > somehow, or on serial consoles. 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, 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()? 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? 3. Teach libvirt how to consume that new tag and pass it along? [1] https://lists.nongnu.org/archive/html/qemu-devel/2018-10/msg06131.html -- Cheers, Christophe de Dinechin (IRC c3d) -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] [PATCH 2/2] qapi: deprecate implicit filters
John Snow writes:
[...]
>
> This might be OK to do right away, though.
>
> I asked Markus this not too long ago; do we want to amend the QAPI
> schema specification to allow commands to return with "Warning" strings,
> or "Deprecated" stings to allow in-band deprecation notices for cases
> like these?
>
> example:
>
> { "return": {},
> "deprecated": True,
> "warning": "Omitting filter-node-name parameter is deprecated, it will
> be required in the future"
> }
>
> There's no "error" key, so this should be recognized as success by
> compatible clients, but they'll definitely see the extra information.
>
> Part of my motivation is to facilitate a more aggressive deprecation of
> legacy features by ensuring that we are able to rigorously notify users
> through any means that they need to adjust their scripts.
I like this approach even if there is no consumer today. It does not
hurt, and it is indeed a motivation to develop consumers that care.
I personally find this much easier to swallow than any kind of crash on
deprecation, which already at the BoF seemed like a really big hammer to
kill a fly.
CC'ing Andrea as well, because we discussed recently about how to deal
with error checking in general, and if a new error checking framework is
being put in place, adding deprecation to the thinking could be a good
idea.
--
Cheers,
Christophe de Dinechin (IRC c3d)
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] [PATCH 2/2] qapi: deprecate implicit filters
16.08.2019 15:33, Markus Armbruster wrote:
> Kevin Wolf writes:
>
>> Am 15.08.2019 um 21:24 hat Markus Armbruster geschrieben:
> [...]
>>> Let's assume all libvirt ever does with deprecation notices is logging
>>> them. Would that solve the problem of reliably alerting libvirt
>>> developers to deprecation issues? Nope. But it could help
>>> occasionally.
>>
>> I'm not saying that deprecation notices would hurt, just that they
>> probably won't solve problem alone.
>
> No argument.
>
>> Crashing if --future is given and logging otherwise seems reasonable
>> enough to me. Whether we need to wire up a new deprecation mechanism in
>> QMP for the logging or if we can just keep printing to stderr is
>> debatable. stderr already ends up in a log file, a QMP extension would
>> require new libvirt code. If libvirt would log deprecation notices more
>> prominently, or use the information for tainting or any other kind of
>> processing, a dedicated QMP mechanism could be justified.
>
> I'd like to start with two tasks:
>
> * A CLI option to configure what to do on use of a deprecated feature.
>
>We currently warn. We want to be able to crash instead. Silencing
>the warnings might be useful. Turning them into errors might be
>useful.
>
>The existing ad hoc warnings need to be replaced by a call of a common
>function that implements the configurable behavior.
>
> * QAPI feature flag "deprecated", for introspectable deprecation, and
>without ad hoc code.
>
> Then see whether our users need more.
>
Crashing is useful for libvirt developers, it's obvious, just enable
crash-on-deprecated
on all testing environments and most probably we will not miss such a case.
For qapi I doubt is it really needed. Implementing code in libvirt which will
check for command
(or it's parameter, or it's parameter "optionality" is deprecated) ? It's hard
and what libvirt
should report to final user? It becomes a kind of synthetic error in libvirt
code, like
...
log_error("We are going to divide by zero. It's a bug, please report it to
developers!");
x = a / 0;
...
It's simpler to fix second line than implement special mechanism including
protocol specification
to report such a case.
I exaggerate of course with this example, but I doubt that implementing a
special protocol
for it worth doing. And I think notifying libvirt by email (as Peter said) and
providing option
"crash-on-deprecated" in Qemu are enough for libvirt developers to prevent and
to fix using
deprecated things.
In other words, I don't see why reporting deprecated feature usage is better in
libvirt than in
Qemu (by warning, error or crash), and in Qemu it's much more simple and don't
need QAPI protocol
extension.
(I'm sorry if I'm repeating already written arguments, I've not read the whole
thread)
--
Best regards,
Vladimir
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] [PATCH 2/2] qapi: deprecate implicit filters
Kevin Wolf writes: > Am 15.08.2019 um 21:24 hat Markus Armbruster geschrieben: [...] >> Let's assume all libvirt ever does with deprecation notices is logging >> them. Would that solve the problem of reliably alerting libvirt >> developers to deprecation issues? Nope. But it could help >> occasionally. > > I'm not saying that deprecation notices would hurt, just that they > probably won't solve problem alone. No argument. > Crashing if --future is given and logging otherwise seems reasonable > enough to me. Whether we need to wire up a new deprecation mechanism in > QMP for the logging or if we can just keep printing to stderr is > debatable. stderr already ends up in a log file, a QMP extension would > require new libvirt code. If libvirt would log deprecation notices more > prominently, or use the information for tainting or any other kind of > processing, a dedicated QMP mechanism could be justified. I'd like to start with two tasks: * A CLI option to configure what to do on use of a deprecated feature. We currently warn. We want to be able to crash instead. Silencing the warnings might be useful. Turning them into errors might be useful. The existing ad hoc warnings need to be replaced by a call of a common function that implements the configurable behavior. * QAPI feature flag "deprecated", for introspectable deprecation, and without ad hoc code. Then see whether our users need more. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] [PATCH 2/2] qapi: deprecate implicit filters
Am 15.08.2019 um 21:24 hat Markus Armbruster geschrieben:
> Kevin Wolf writes:
>
> > Am 15.08.2019 um 18:07 hat John Snow geschrieben:
> >>
> >>
> >> On 8/15/19 6:49 AM, Kevin Wolf wrote:
> >> > Am 14.08.2019 um 21:27 hat John Snow geschrieben:
> >> >>
> >> >>
> >> >> On 8/14/19 6:07 AM, Vladimir Sementsov-Ogievskiy wrote:
> >> >>> To get rid of implicit filters related workarounds in future let's
> >> >>> deprecate them now.
> >> >>>
> >> >>> Signed-off-by: Vladimir Sementsov-Ogievskiy
> >> >>> ---
> >> >>> qemu-deprecated.texi | 7 +++
> >> >>> qapi/block-core.json | 6 --
> >> >>> include/block/block_int.h | 10 +-
> >> >>> blockdev.c| 10 ++
> >> >>> 4 files changed, 30 insertions(+), 3 deletions(-)
> >> >>>
> >> >>> diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
> >> >>> index 2753fafd0b..8222440148 100644
> >> >>> --- a/qemu-deprecated.texi
> >> >>> +++ b/qemu-deprecated.texi
> >> >>> @@ -183,6 +183,13 @@ the 'wait' field, which is only applicable to
> >> >>> sockets in server mode
> >> >>>
> >> >>> Use blockdev-mirror and blockdev-backup instead.
> >> >>>
> >> >>> +@subsection implicit filters (since 4.2)
> >> >>> +
> >> >>> +Mirror and commit jobs inserts filters, which becomes implicit if user
> >> >>> +omitted filter-node-name parameter. So omitting it is deprecated, set
> >> >>> it
> >> >>> +always. Note, that drive-mirror don't have this parameter, so it will
> >> >>> +create implicit filter anyway, but drive-mirror is deprecated itself
> >> >>> too.
> >> >>> +
> >> >>> @section Human Monitor Protocol (HMP) commands
> >> >>>
> >> >>> @subsection The hub_id parameter of 'hostfwd_add' / 'hostfwd_remove'
> >> >>> (since 3.1)
> >> >>> diff --git a/qapi/block-core.json b/qapi/block-core.json
> >> >>> index 4e35526634..0505ac9d8b 100644
> >> >>> --- a/qapi/block-core.json
> >> >>> +++ b/qapi/block-core.json
> >> >>> @@ -1596,7 +1596,8 @@
> >> >>> # @filter-node-name: the node name that should be assigned to the
> >> >>> #filter driver that the commit job inserts into
> >> >>> the graph
> >> >>> #above @top. If this option is not given, a node
> >> >>> name is
> >> >>> -#autogenerated. (Since: 2.9)
> >> >>> +#autogenerated. Omitting this option is
> >> >>> deprecated, it will
> >> >>> +#be required in future. (Since: 2.9)
> >> >>> #
> >> >>> # @auto-finalize: When false, this job will wait in a PENDING state
> >> >>> after it has
> >> >>> # finished its work, waiting for @block-job-finalize
> >> >>> before
> >> >>> @@ -2249,7 +2250,8 @@
> >> >>> # @filter-node-name: the node name that should be assigned to the
> >> >>> #filter driver that the mirror job inserts into
> >> >>> the graph
> >> >>> #above @device. If this option is not given, a
> >> >>> node name is
> >> >>> -#autogenerated. (Since: 2.9)
> >> >>> +#autogenerated. Omitting this option is
> >> >>> deprecated, it will
> >> >>> +#be required in future. (Since: 2.9)
> >> >>> #
> >> >>> # @copy-mode: when to copy data to the destination; defaults to
> >> >>> 'background'
> >> >>> # (Since: 3.0)
> >> >>> diff --git a/include/block/block_int.h b/include/block/block_int.h
> >> >>> index 3aa1e832a8..624da0b4a2 100644
> >> >>> --- a/include/block/block_int.h
> >> >>> +++ b/include/block/block_int.h
> >> >>> @@ -762,7 +762,15 @@ struct BlockDriverState {
> >> >>> bool sg;/* if true, the device is a /dev/sg* */
> >> >>> bool probed;/* if true, format was probed rather than
> >> >>> specified */
> >> >>> bool force_share; /* if true, always allow all shared permissions
> >> >>> */
> >> >>> -bool implicit; /* if true, this filter node was automatically
> >> >>> inserted */
> >> >>> +
> >> >>> +/*
> >> >>> + * @implicit field is deprecated, don't set it to true for new
> >> >>> filters.
> >> >>> + * If true, this filter node was automatically inserted and user
> >> >>> don't
> >> >>> + * know about it and unprepared for any effects of it. So,
> >> >>> implicit
> >> >>> + * filters are workarounded and skipped in many places of the
> >> >>> block
> >> >>> + * layer code.
> >> >>> + */
> >> >>> +bool implicit;
> >> >>>
> >> >>> BlockDriver *drv; /* NULL means no media */
> >> >>> void *opaque;
> >> >>> diff --git a/blockdev.c b/blockdev.c
> >> >>> index 36e9368e01..b3cfaccce1 100644
> >> >>> --- a/blockdev.c
> >> >>> +++ b/blockdev.c
> >> >>> @@ -3292,6 +3292,11 @@ void qmp_block_commit(bool has_job_id, const
> >> >>> char *job_id, const char *device,
> >> >>> BlockdevOnError on_error = BLOCKDEV_ON_ERROR_REPORT;
> >> >>> int job_flags = JOB_DEFAULT;
> >> >>>
> >> >>> +if (!has_filter_node_name) {
> >> >>> +
Re: [libvirt] [Qemu-devel] [PATCH 2/2] qapi: deprecate implicit filters
Kevin Wolf writes:
> Am 15.08.2019 um 18:07 hat John Snow geschrieben:
>>
>>
>> On 8/15/19 6:49 AM, Kevin Wolf wrote:
>> > Am 14.08.2019 um 21:27 hat John Snow geschrieben:
>> >>
>> >>
>> >> On 8/14/19 6:07 AM, Vladimir Sementsov-Ogievskiy wrote:
>> >>> To get rid of implicit filters related workarounds in future let's
>> >>> deprecate them now.
>> >>>
>> >>> Signed-off-by: Vladimir Sementsov-Ogievskiy
>> >>> ---
>> >>> qemu-deprecated.texi | 7 +++
>> >>> qapi/block-core.json | 6 --
>> >>> include/block/block_int.h | 10 +-
>> >>> blockdev.c| 10 ++
>> >>> 4 files changed, 30 insertions(+), 3 deletions(-)
>> >>>
>> >>> diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
>> >>> index 2753fafd0b..8222440148 100644
>> >>> --- a/qemu-deprecated.texi
>> >>> +++ b/qemu-deprecated.texi
>> >>> @@ -183,6 +183,13 @@ the 'wait' field, which is only applicable to
>> >>> sockets in server mode
>> >>>
>> >>> Use blockdev-mirror and blockdev-backup instead.
>> >>>
>> >>> +@subsection implicit filters (since 4.2)
>> >>> +
>> >>> +Mirror and commit jobs inserts filters, which becomes implicit if user
>> >>> +omitted filter-node-name parameter. So omitting it is deprecated, set it
>> >>> +always. Note, that drive-mirror don't have this parameter, so it will
>> >>> +create implicit filter anyway, but drive-mirror is deprecated itself
>> >>> too.
>> >>> +
>> >>> @section Human Monitor Protocol (HMP) commands
>> >>>
>> >>> @subsection The hub_id parameter of 'hostfwd_add' / 'hostfwd_remove'
>> >>> (since 3.1)
>> >>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> >>> index 4e35526634..0505ac9d8b 100644
>> >>> --- a/qapi/block-core.json
>> >>> +++ b/qapi/block-core.json
>> >>> @@ -1596,7 +1596,8 @@
>> >>> # @filter-node-name: the node name that should be assigned to the
>> >>> #filter driver that the commit job inserts into the
>> >>> graph
>> >>> #above @top. If this option is not given, a node
>> >>> name is
>> >>> -#autogenerated. (Since: 2.9)
>> >>> +#autogenerated. Omitting this option is deprecated,
>> >>> it will
>> >>> +#be required in future. (Since: 2.9)
>> >>> #
>> >>> # @auto-finalize: When false, this job will wait in a PENDING state
>> >>> after it has
>> >>> # finished its work, waiting for @block-job-finalize
>> >>> before
>> >>> @@ -2249,7 +2250,8 @@
>> >>> # @filter-node-name: the node name that should be assigned to the
>> >>> #filter driver that the mirror job inserts into the
>> >>> graph
>> >>> #above @device. If this option is not given, a node
>> >>> name is
>> >>> -#autogenerated. (Since: 2.9)
>> >>> +#autogenerated. Omitting this option is deprecated,
>> >>> it will
>> >>> +#be required in future. (Since: 2.9)
>> >>> #
>> >>> # @copy-mode: when to copy data to the destination; defaults to
>> >>> 'background'
>> >>> # (Since: 3.0)
>> >>> diff --git a/include/block/block_int.h b/include/block/block_int.h
>> >>> index 3aa1e832a8..624da0b4a2 100644
>> >>> --- a/include/block/block_int.h
>> >>> +++ b/include/block/block_int.h
>> >>> @@ -762,7 +762,15 @@ struct BlockDriverState {
>> >>> bool sg;/* if true, the device is a /dev/sg* */
>> >>> bool probed;/* if true, format was probed rather than specified
>> >>> */
>> >>> bool force_share; /* if true, always allow all shared permissions */
>> >>> -bool implicit; /* if true, this filter node was automatically
>> >>> inserted */
>> >>> +
>> >>> +/*
>> >>> + * @implicit field is deprecated, don't set it to true for new
>> >>> filters.
>> >>> + * If true, this filter node was automatically inserted and user
>> >>> don't
>> >>> + * know about it and unprepared for any effects of it. So, implicit
>> >>> + * filters are workarounded and skipped in many places of the block
>> >>> + * layer code.
>> >>> + */
>> >>> +bool implicit;
>> >>>
>> >>> BlockDriver *drv; /* NULL means no media */
>> >>> void *opaque;
>> >>> diff --git a/blockdev.c b/blockdev.c
>> >>> index 36e9368e01..b3cfaccce1 100644
>> >>> --- a/blockdev.c
>> >>> +++ b/blockdev.c
>> >>> @@ -3292,6 +3292,11 @@ void qmp_block_commit(bool has_job_id, const char
>> >>> *job_id, const char *device,
>> >>> BlockdevOnError on_error = BLOCKDEV_ON_ERROR_REPORT;
>> >>> int job_flags = JOB_DEFAULT;
>> >>>
>> >>> +if (!has_filter_node_name) {
>> >>> +warn_report("Omitting filter-node-name parameter is deprecated,
>> >>> it "
>> >>> +"will be required in future");
>> >>> +}
>> >>> +
>> >>> if (!has_speed) {
>> >>> speed = 0;
>> >>> }
>> >>> @@ -3990,6 +3995,11 @@ void qmp_blockdev_mirror(bool has_job_id, const
>> >>> char *job_id,
>> >>>
Re: [libvirt] [Qemu-devel] [PATCH 2/2] qapi: deprecate implicit filters
On 8/15/19 12:48 PM, Kevin Wolf wrote:
> Am 15.08.2019 um 18:07 hat John Snow geschrieben:
>> On 8/15/19 6:49 AM, Kevin Wolf wrote:
>>> Am 14.08.2019 um 21:27 hat John Snow geschrieben:
This might be OK to do right away, though.
I asked Markus this not too long ago; do we want to amend the QAPI
schema specification to allow commands to return with "Warning" strings,
or "Deprecated" stings to allow in-band deprecation notices for cases
like these?
example:
{ "return": {},
"deprecated": True,
"warning": "Omitting filter-node-name parameter is deprecated, it will
be required in the future"
}
There's no "error" key, so this should be recognized as success by
compatible clients, but they'll definitely see the extra information.
Part of my motivation is to facilitate a more aggressive deprecation of
legacy features by ensuring that we are able to rigorously notify users
through any means that they need to adjust their scripts.
>>>
>>> Who would read this, though? In the best case it ends up deep in a
>>> libvirt log that nobody will look at because there was no error. In the
>>> more common case, the debug level is configured so that QMP traffic
>>> isn't even logged.
>>>
>>> Kevin
>>>
>>
>> I believe you are right, but I also can't shake the feeling that this
>> attitude ensures that we'll never find a way to expose this information
>> to the end-user. Is this not too defeatist?
>
> I think the discussed approach that seemed most likely to me to succeed
> was adding a command line option that makes QEMU just crash if you use a
> deprecated feature, and enable that in libvirt test cases (or possibly
> even any non-release builds, though maybe it's a bit harsh there).
>
>> I think deprecation notices in the QMP stream has two benefits:
>>
>> 1) Any direct usages via qmp-shell or manual JSON connection are likely
>> to see this message in development or testing. I feel the usage of QEMU
>> directly is more likely to increase with time as other stacks seek to
>> work around libvirt.
>>
>> [Whether or not they should is another question, but I believe the
>> current reality to be that people are trying to.]
>
> I don't know about other people, but as a human user, I don't care about
> deprecation notices. As long as something works, I use it, and once I
> get an error message back, I'll use something else.
>
> If I manually enter drive_mirror and get a warning back, that doesn't
> tell me that libvirt still does the same thing and needs to be fixed. It
> just tells me that in the future I might need to change the commands
> that I use manually.
>
That the message we return needs to be *useful* doesn't sound like a
count against it.
> I guess this would still prevent adding new libvirt features that build
> on deprecated QEMU features because some manual testing will be involved
> there. But was this ever a problem?
>
No, because until recently we didn't deprecate anything.
>> 2) Programmatic deprecation notices can't be presented to a user at all
>> if we don't send them; at least this way it becomes libvirt's problem
>> over what to do with them. Perhaps even just in testing and regression
>> suites libvirt can assert that it sees no deprecation warnings (or
>> whitelist certain ones it knows about.)
>>
>> In the case of libvirt, it's not even necessarily about making sure the
>> end user sees it, because it isn't even necessarily the user's fault --
>> it's libvirt's. This is a sure-fire programmatic way to communicate
>> compatibility changes to libvirt.
>
> If libvirt uses this to make test cases fail, it could work.
>
Yeah, I think there's solid use there. I'll continue along in Markus's
thread.
> Kevin
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] [PATCH 2/2] qapi: deprecate implicit filters
Am 15.08.2019 um 18:07 hat John Snow geschrieben:
>
>
> On 8/15/19 6:49 AM, Kevin Wolf wrote:
> > Am 14.08.2019 um 21:27 hat John Snow geschrieben:
> >>
> >>
> >> On 8/14/19 6:07 AM, Vladimir Sementsov-Ogievskiy wrote:
> >>> To get rid of implicit filters related workarounds in future let's
> >>> deprecate them now.
> >>>
> >>> Signed-off-by: Vladimir Sementsov-Ogievskiy
> >>> ---
> >>> qemu-deprecated.texi | 7 +++
> >>> qapi/block-core.json | 6 --
> >>> include/block/block_int.h | 10 +-
> >>> blockdev.c| 10 ++
> >>> 4 files changed, 30 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
> >>> index 2753fafd0b..8222440148 100644
> >>> --- a/qemu-deprecated.texi
> >>> +++ b/qemu-deprecated.texi
> >>> @@ -183,6 +183,13 @@ the 'wait' field, which is only applicable to
> >>> sockets in server mode
> >>>
> >>> Use blockdev-mirror and blockdev-backup instead.
> >>>
> >>> +@subsection implicit filters (since 4.2)
> >>> +
> >>> +Mirror and commit jobs inserts filters, which becomes implicit if user
> >>> +omitted filter-node-name parameter. So omitting it is deprecated, set it
> >>> +always. Note, that drive-mirror don't have this parameter, so it will
> >>> +create implicit filter anyway, but drive-mirror is deprecated itself too.
> >>> +
> >>> @section Human Monitor Protocol (HMP) commands
> >>>
> >>> @subsection The hub_id parameter of 'hostfwd_add' / 'hostfwd_remove'
> >>> (since 3.1)
> >>> diff --git a/qapi/block-core.json b/qapi/block-core.json
> >>> index 4e35526634..0505ac9d8b 100644
> >>> --- a/qapi/block-core.json
> >>> +++ b/qapi/block-core.json
> >>> @@ -1596,7 +1596,8 @@
> >>> # @filter-node-name: the node name that should be assigned to the
> >>> #filter driver that the commit job inserts into the
> >>> graph
> >>> #above @top. If this option is not given, a node
> >>> name is
> >>> -#autogenerated. (Since: 2.9)
> >>> +#autogenerated. Omitting this option is deprecated,
> >>> it will
> >>> +#be required in future. (Since: 2.9)
> >>> #
> >>> # @auto-finalize: When false, this job will wait in a PENDING state
> >>> after it has
> >>> # finished its work, waiting for @block-job-finalize
> >>> before
> >>> @@ -2249,7 +2250,8 @@
> >>> # @filter-node-name: the node name that should be assigned to the
> >>> #filter driver that the mirror job inserts into the
> >>> graph
> >>> #above @device. If this option is not given, a node
> >>> name is
> >>> -#autogenerated. (Since: 2.9)
> >>> +#autogenerated. Omitting this option is deprecated,
> >>> it will
> >>> +#be required in future. (Since: 2.9)
> >>> #
> >>> # @copy-mode: when to copy data to the destination; defaults to
> >>> 'background'
> >>> # (Since: 3.0)
> >>> diff --git a/include/block/block_int.h b/include/block/block_int.h
> >>> index 3aa1e832a8..624da0b4a2 100644
> >>> --- a/include/block/block_int.h
> >>> +++ b/include/block/block_int.h
> >>> @@ -762,7 +762,15 @@ struct BlockDriverState {
> >>> bool sg;/* if true, the device is a /dev/sg* */
> >>> bool probed;/* if true, format was probed rather than specified
> >>> */
> >>> bool force_share; /* if true, always allow all shared permissions */
> >>> -bool implicit; /* if true, this filter node was automatically
> >>> inserted */
> >>> +
> >>> +/*
> >>> + * @implicit field is deprecated, don't set it to true for new
> >>> filters.
> >>> + * If true, this filter node was automatically inserted and user
> >>> don't
> >>> + * know about it and unprepared for any effects of it. So, implicit
> >>> + * filters are workarounded and skipped in many places of the block
> >>> + * layer code.
> >>> + */
> >>> +bool implicit;
> >>>
> >>> BlockDriver *drv; /* NULL means no media */
> >>> void *opaque;
> >>> diff --git a/blockdev.c b/blockdev.c
> >>> index 36e9368e01..b3cfaccce1 100644
> >>> --- a/blockdev.c
> >>> +++ b/blockdev.c
> >>> @@ -3292,6 +3292,11 @@ void qmp_block_commit(bool has_job_id, const char
> >>> *job_id, const char *device,
> >>> BlockdevOnError on_error = BLOCKDEV_ON_ERROR_REPORT;
> >>> int job_flags = JOB_DEFAULT;
> >>>
> >>> +if (!has_filter_node_name) {
> >>> +warn_report("Omitting filter-node-name parameter is deprecated,
> >>> it "
> >>> +"will be required in future");
> >>> +}
> >>> +
> >>> if (!has_speed) {
> >>> speed = 0;
> >>> }
> >>> @@ -3990,6 +3995,11 @@ void qmp_blockdev_mirror(bool has_job_id, const
> >>> char *job_id,
> >>> Error *local_err = NULL;
> >>> int ret;
> >>>
> >>> +if (!has_filter_node_name) {
> >>> +warn_report("Omitting filter-node-name
Re: [libvirt] [Qemu-devel] [PATCH 2/2] qapi: deprecate implicit filters
On 8/15/19 6:49 AM, Kevin Wolf wrote:
> Am 14.08.2019 um 21:27 hat John Snow geschrieben:
>>
>>
>> On 8/14/19 6:07 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> To get rid of implicit filters related workarounds in future let's
>>> deprecate them now.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy
>>> ---
>>> qemu-deprecated.texi | 7 +++
>>> qapi/block-core.json | 6 --
>>> include/block/block_int.h | 10 +-
>>> blockdev.c| 10 ++
>>> 4 files changed, 30 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
>>> index 2753fafd0b..8222440148 100644
>>> --- a/qemu-deprecated.texi
>>> +++ b/qemu-deprecated.texi
>>> @@ -183,6 +183,13 @@ the 'wait' field, which is only applicable to sockets
>>> in server mode
>>>
>>> Use blockdev-mirror and blockdev-backup instead.
>>>
>>> +@subsection implicit filters (since 4.2)
>>> +
>>> +Mirror and commit jobs inserts filters, which becomes implicit if user
>>> +omitted filter-node-name parameter. So omitting it is deprecated, set it
>>> +always. Note, that drive-mirror don't have this parameter, so it will
>>> +create implicit filter anyway, but drive-mirror is deprecated itself too.
>>> +
>>> @section Human Monitor Protocol (HMP) commands
>>>
>>> @subsection The hub_id parameter of 'hostfwd_add' / 'hostfwd_remove'
>>> (since 3.1)
>>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>>> index 4e35526634..0505ac9d8b 100644
>>> --- a/qapi/block-core.json
>>> +++ b/qapi/block-core.json
>>> @@ -1596,7 +1596,8 @@
>>> # @filter-node-name: the node name that should be assigned to the
>>> #filter driver that the commit job inserts into the
>>> graph
>>> #above @top. If this option is not given, a node name
>>> is
>>> -#autogenerated. (Since: 2.9)
>>> +#autogenerated. Omitting this option is deprecated, it
>>> will
>>> +#be required in future. (Since: 2.9)
>>> #
>>> # @auto-finalize: When false, this job will wait in a PENDING state after
>>> it has
>>> # finished its work, waiting for @block-job-finalize before
>>> @@ -2249,7 +2250,8 @@
>>> # @filter-node-name: the node name that should be assigned to the
>>> #filter driver that the mirror job inserts into the
>>> graph
>>> #above @device. If this option is not given, a node
>>> name is
>>> -#autogenerated. (Since: 2.9)
>>> +#autogenerated. Omitting this option is deprecated, it
>>> will
>>> +#be required in future. (Since: 2.9)
>>> #
>>> # @copy-mode: when to copy data to the destination; defaults to
>>> 'background'
>>> # (Since: 3.0)
>>> diff --git a/include/block/block_int.h b/include/block/block_int.h
>>> index 3aa1e832a8..624da0b4a2 100644
>>> --- a/include/block/block_int.h
>>> +++ b/include/block/block_int.h
>>> @@ -762,7 +762,15 @@ struct BlockDriverState {
>>> bool sg;/* if true, the device is a /dev/sg* */
>>> bool probed;/* if true, format was probed rather than specified */
>>> bool force_share; /* if true, always allow all shared permissions */
>>> -bool implicit; /* if true, this filter node was automatically
>>> inserted */
>>> +
>>> +/*
>>> + * @implicit field is deprecated, don't set it to true for new filters.
>>> + * If true, this filter node was automatically inserted and user don't
>>> + * know about it and unprepared for any effects of it. So, implicit
>>> + * filters are workarounded and skipped in many places of the block
>>> + * layer code.
>>> + */
>>> +bool implicit;
>>>
>>> BlockDriver *drv; /* NULL means no media */
>>> void *opaque;
>>> diff --git a/blockdev.c b/blockdev.c
>>> index 36e9368e01..b3cfaccce1 100644
>>> --- a/blockdev.c
>>> +++ b/blockdev.c
>>> @@ -3292,6 +3292,11 @@ void qmp_block_commit(bool has_job_id, const char
>>> *job_id, const char *device,
>>> BlockdevOnError on_error = BLOCKDEV_ON_ERROR_REPORT;
>>> int job_flags = JOB_DEFAULT;
>>>
>>> +if (!has_filter_node_name) {
>>> +warn_report("Omitting filter-node-name parameter is deprecated, it
>>> "
>>> +"will be required in future");
>>> +}
>>> +
>>> if (!has_speed) {
>>> speed = 0;
>>> }
>>> @@ -3990,6 +3995,11 @@ void qmp_blockdev_mirror(bool has_job_id, const char
>>> *job_id,
>>> Error *local_err = NULL;
>>> int ret;
>>>
>>> +if (!has_filter_node_name) {
>>> +warn_report("Omitting filter-node-name parameter is deprecated, it
>>> "
>>> +"will be required in future");
>>> +}
>>> +
>>> bs = qmp_get_root_bs(device, errp);
>>> if (!bs) {
>>> return;
>>>
>>
>> This might be OK to do right away, though.
>>
>> I asked Markus this not too long ago; do we want to amend
Re: [libvirt] [Qemu-devel] [PATCH 2/2] qapi: deprecate implicit filters
Peter Krempa writes:
> On Thu, Aug 15, 2019 at 12:49:28 +0200, Kevin Wolf wrote:
>> Am 14.08.2019 um 21:27 hat John Snow geschrieben:
>
> [...]
>
>> > example:
>> >
>> > { "return": {},
>> > "deprecated": True,
>> > "warning": "Omitting filter-node-name parameter is deprecated, it will
>> > be required in the future"
>> > }
>> >
>> > There's no "error" key, so this should be recognized as success by
>> > compatible clients, but they'll definitely see the extra information.
>> >
>> > Part of my motivation is to facilitate a more aggressive deprecation of
>> > legacy features by ensuring that we are able to rigorously notify users
>> > through any means that they need to adjust their scripts.
>>
>> Who would read this, though? In the best case it ends up deep in a
>> libvirt log that nobody will look at because there was no error. In the
>> more common case, the debug level is configured so that QMP traffic
>> isn't even logged.
>
> The best we could do here is to log a warning. Thankfully we have one
> central function which always checks the returned JSON from qemu so we
> could do that universally.
>
> The would end up in the system log and alternatively also in the VM
> log file. I agree with Kevin that the possibility of it being noticed
> is rather small.
>
> From my experience users report non-fatal messages mostly only if it is
> spamming the system log. One of instances are very unlikely to be
> noticed.
>
> In my experience it's better to notify us in libvirt of such change and
> we will try our best to fix it.
How to best alert the layers above QEMU was one of the topic of the KVM
Forum 2018 BoF on deprecating stuff. Minutes:
Message-ID: <87mur0ls8o@dusky.pond.sub.org>
https://lists.nongnu.org/archive/html/qemu-devel/2018-10/msg05828.html
Relevant part:
* We need to communicate "you're using something that is deprecated".
How? Right now, we print a deprecation message. Okay when humans use
QEMU directly in a shell. However, when QEMU sits at the bottom of a
software stack, the message will likely end up in a log file that is
effectively write-only.
- The one way to get people read log files is crashing their
application. A command line option --future could make QEMU crash
right after printing a deprecation message. This could help with
finding use of deprecated features in a testing environment.
- A less destructive way to grab people's attention is to make things
run really, really slow: have QEMU go to sleep for a while after
printing a deprecation message.
- We can also pass the buck to the next layer up: emit a QMP event.
Sadly, by the time the next layer connects to QMP, plenty of stuff
already happened. We'd have to buffer deprecation events somehow.
What would libvirt do with such an event? Log it, taint the domain,
emit a (libvirt) event to pass it on to the next layer up.
- A completely different idea is to have a configuratin linter. To
support doing this at the libvirt level, QEMU could expose "is
deprecated" in interface introspection. Feels feasible for QMP,
where we already have sufficiently expressive introspection. For
CLI, we'd first have to provide that (but we want that anyway).
- We might also want to dispay deprecation messages in QEMU's GUI
somehow, or on serial consoles.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] [PATCH 2/2] qapi: deprecate implicit filters
On Thu, Aug 15, 2019 at 12:49:28 +0200, Kevin Wolf wrote:
> Am 14.08.2019 um 21:27 hat John Snow geschrieben:
[...]
> > example:
> >
> > { "return": {},
> > "deprecated": True,
> > "warning": "Omitting filter-node-name parameter is deprecated, it will
> > be required in the future"
> > }
> >
> > There's no "error" key, so this should be recognized as success by
> > compatible clients, but they'll definitely see the extra information.
> >
> > Part of my motivation is to facilitate a more aggressive deprecation of
> > legacy features by ensuring that we are able to rigorously notify users
> > through any means that they need to adjust their scripts.
>
> Who would read this, though? In the best case it ends up deep in a
> libvirt log that nobody will look at because there was no error. In the
> more common case, the debug level is configured so that QMP traffic
> isn't even logged.
The best we could do here is to log a warning. Thankfully we have one
central function which always checks the returned JSON from qemu so we
could do that universally.
The would end up in the system log and alternatively also in the VM
log file. I agree with Kevin that the possibility of it being noticed
is rather small.
From my experience users report non-fatal messages mostly only if it is
spamming the system log. One of instances are very unlikely to be
noticed.
In my experience it's better to notify us in libvirt of such change and
we will try our best to fix it.
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] [PATCH 2/2] qapi: deprecate implicit filters
Am 14.08.2019 um 21:27 hat John Snow geschrieben:
>
>
> On 8/14/19 6:07 AM, Vladimir Sementsov-Ogievskiy wrote:
> > To get rid of implicit filters related workarounds in future let's
> > deprecate them now.
> >
> > Signed-off-by: Vladimir Sementsov-Ogievskiy
> > ---
> > qemu-deprecated.texi | 7 +++
> > qapi/block-core.json | 6 --
> > include/block/block_int.h | 10 +-
> > blockdev.c| 10 ++
> > 4 files changed, 30 insertions(+), 3 deletions(-)
> >
> > diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
> > index 2753fafd0b..8222440148 100644
> > --- a/qemu-deprecated.texi
> > +++ b/qemu-deprecated.texi
> > @@ -183,6 +183,13 @@ the 'wait' field, which is only applicable to sockets
> > in server mode
> >
> > Use blockdev-mirror and blockdev-backup instead.
> >
> > +@subsection implicit filters (since 4.2)
> > +
> > +Mirror and commit jobs inserts filters, which becomes implicit if user
> > +omitted filter-node-name parameter. So omitting it is deprecated, set it
> > +always. Note, that drive-mirror don't have this parameter, so it will
> > +create implicit filter anyway, but drive-mirror is deprecated itself too.
> > +
> > @section Human Monitor Protocol (HMP) commands
> >
> > @subsection The hub_id parameter of 'hostfwd_add' / 'hostfwd_remove'
> > (since 3.1)
> > diff --git a/qapi/block-core.json b/qapi/block-core.json
> > index 4e35526634..0505ac9d8b 100644
> > --- a/qapi/block-core.json
> > +++ b/qapi/block-core.json
> > @@ -1596,7 +1596,8 @@
> > # @filter-node-name: the node name that should be assigned to the
> > #filter driver that the commit job inserts into the
> > graph
> > #above @top. If this option is not given, a node name
> > is
> > -#autogenerated. (Since: 2.9)
> > +#autogenerated. Omitting this option is deprecated, it
> > will
> > +#be required in future. (Since: 2.9)
> > #
> > # @auto-finalize: When false, this job will wait in a PENDING state after
> > it has
> > # finished its work, waiting for @block-job-finalize before
> > @@ -2249,7 +2250,8 @@
> > # @filter-node-name: the node name that should be assigned to the
> > #filter driver that the mirror job inserts into the
> > graph
> > #above @device. If this option is not given, a node
> > name is
> > -#autogenerated. (Since: 2.9)
> > +#autogenerated. Omitting this option is deprecated, it
> > will
> > +#be required in future. (Since: 2.9)
> > #
> > # @copy-mode: when to copy data to the destination; defaults to
> > 'background'
> > # (Since: 3.0)
> > diff --git a/include/block/block_int.h b/include/block/block_int.h
> > index 3aa1e832a8..624da0b4a2 100644
> > --- a/include/block/block_int.h
> > +++ b/include/block/block_int.h
> > @@ -762,7 +762,15 @@ struct BlockDriverState {
> > bool sg;/* if true, the device is a /dev/sg* */
> > bool probed;/* if true, format was probed rather than specified */
> > bool force_share; /* if true, always allow all shared permissions */
> > -bool implicit; /* if true, this filter node was automatically
> > inserted */
> > +
> > +/*
> > + * @implicit field is deprecated, don't set it to true for new filters.
> > + * If true, this filter node was automatically inserted and user don't
> > + * know about it and unprepared for any effects of it. So, implicit
> > + * filters are workarounded and skipped in many places of the block
> > + * layer code.
> > + */
> > +bool implicit;
> >
> > BlockDriver *drv; /* NULL means no media */
> > void *opaque;
> > diff --git a/blockdev.c b/blockdev.c
> > index 36e9368e01..b3cfaccce1 100644
> > --- a/blockdev.c
> > +++ b/blockdev.c
> > @@ -3292,6 +3292,11 @@ void qmp_block_commit(bool has_job_id, const char
> > *job_id, const char *device,
> > BlockdevOnError on_error = BLOCKDEV_ON_ERROR_REPORT;
> > int job_flags = JOB_DEFAULT;
> >
> > +if (!has_filter_node_name) {
> > +warn_report("Omitting filter-node-name parameter is deprecated, it
> > "
> > +"will be required in future");
> > +}
> > +
> > if (!has_speed) {
> > speed = 0;
> > }
> > @@ -3990,6 +3995,11 @@ void qmp_blockdev_mirror(bool has_job_id, const char
> > *job_id,
> > Error *local_err = NULL;
> > int ret;
> >
> > +if (!has_filter_node_name) {
> > +warn_report("Omitting filter-node-name parameter is deprecated, it
> > "
> > +"will be required in future");
> > +}
> > +
> > bs = qmp_get_root_bs(device, errp);
> > if (!bs) {
> > return;
> >
>
> This might be OK to do right away, though.
>
> I asked Markus this not too long ago; do we want to amend the QAPI
> schema specification to allow
Re: [libvirt] [Qemu-devel] [PATCH 2/2] qapi: deprecate implicit filters
On 8/14/19 6:07 AM, Vladimir Sementsov-Ogievskiy wrote:
> To get rid of implicit filters related workarounds in future let's
> deprecate them now.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy
> ---
> qemu-deprecated.texi | 7 +++
> qapi/block-core.json | 6 --
> include/block/block_int.h | 10 +-
> blockdev.c| 10 ++
> 4 files changed, 30 insertions(+), 3 deletions(-)
>
> diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
> index 2753fafd0b..8222440148 100644
> --- a/qemu-deprecated.texi
> +++ b/qemu-deprecated.texi
> @@ -183,6 +183,13 @@ the 'wait' field, which is only applicable to sockets in
> server mode
>
> Use blockdev-mirror and blockdev-backup instead.
>
> +@subsection implicit filters (since 4.2)
> +
> +Mirror and commit jobs inserts filters, which becomes implicit if user
> +omitted filter-node-name parameter. So omitting it is deprecated, set it
> +always. Note, that drive-mirror don't have this parameter, so it will
> +create implicit filter anyway, but drive-mirror is deprecated itself too.
> +
> @section Human Monitor Protocol (HMP) commands
>
> @subsection The hub_id parameter of 'hostfwd_add' / 'hostfwd_remove' (since
> 3.1)
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 4e35526634..0505ac9d8b 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1596,7 +1596,8 @@
> # @filter-node-name: the node name that should be assigned to the
> #filter driver that the commit job inserts into the graph
> #above @top. If this option is not given, a node name is
> -#autogenerated. (Since: 2.9)
> +#autogenerated. Omitting this option is deprecated, it
> will
> +#be required in future. (Since: 2.9)
> #
> # @auto-finalize: When false, this job will wait in a PENDING state after it
> has
> # finished its work, waiting for @block-job-finalize before
> @@ -2249,7 +2250,8 @@
> # @filter-node-name: the node name that should be assigned to the
> #filter driver that the mirror job inserts into the graph
> #above @device. If this option is not given, a node name
> is
> -#autogenerated. (Since: 2.9)
> +#autogenerated. Omitting this option is deprecated, it
> will
> +#be required in future. (Since: 2.9)
> #
> # @copy-mode: when to copy data to the destination; defaults to 'background'
> # (Since: 3.0)
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 3aa1e832a8..624da0b4a2 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -762,7 +762,15 @@ struct BlockDriverState {
> bool sg;/* if true, the device is a /dev/sg* */
> bool probed;/* if true, format was probed rather than specified */
> bool force_share; /* if true, always allow all shared permissions */
> -bool implicit; /* if true, this filter node was automatically inserted
> */
> +
> +/*
> + * @implicit field is deprecated, don't set it to true for new filters.
> + * If true, this filter node was automatically inserted and user don't
> + * know about it and unprepared for any effects of it. So, implicit
> + * filters are workarounded and skipped in many places of the block
> + * layer code.
> + */
> +bool implicit;
>
> BlockDriver *drv; /* NULL means no media */
> void *opaque;
> diff --git a/blockdev.c b/blockdev.c
> index 36e9368e01..b3cfaccce1 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -3292,6 +3292,11 @@ void qmp_block_commit(bool has_job_id, const char
> *job_id, const char *device,
> BlockdevOnError on_error = BLOCKDEV_ON_ERROR_REPORT;
> int job_flags = JOB_DEFAULT;
>
> +if (!has_filter_node_name) {
> +warn_report("Omitting filter-node-name parameter is deprecated, it "
> +"will be required in future");
> +}
> +
> if (!has_speed) {
> speed = 0;
> }
> @@ -3990,6 +3995,11 @@ void qmp_blockdev_mirror(bool has_job_id, const char
> *job_id,
> Error *local_err = NULL;
> int ret;
>
> +if (!has_filter_node_name) {
> +warn_report("Omitting filter-node-name parameter is deprecated, it "
> +"will be required in future");
> +}
> +
> bs = qmp_get_root_bs(device, errp);
> if (!bs) {
> return;
>
This might be OK to do right away, though.
I asked Markus this not too long ago; do we want to amend the QAPI
schema specification to allow commands to return with "Warning" strings,
or "Deprecated" stings to allow in-band deprecation notices for cases
like these?
example:
{ "return": {},
"deprecated": True,
"warning": "Omitting filter-node-name parameter is deprecated, it will
be required in the future"
}
There's no "error" key, so this should be
