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

Markus Armbruster armbru at redhat.com
Thu Aug 15 19:24:34 UTC 2019


Kevin Wolf <kwolf at redhat.com> 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 <vsementsov at virtuozzo.com>
>> >>> ---
>> >>>  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.
>> >>>  
>> >>> + at 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 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).

Yup.  BoF minutes: "The one way to get people read log files is crashing
their application."

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

You're right in that relying on *humans* to read the machine-readable
deprecation notice probaly won't work for old client code trying to use
newly deprecated QMP.  It should work for new client code trying to use
already deprecated QMP.

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

Yes.

However, ensuring tests fail whenever libvirt receives a deprecation
notice via QMP seems harder than having them pass --future to QEMU to
make it crash instead of sending such a notice.

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.




More information about the libvir-list mailing list