qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Kevin Wolf
Subject: Re: [Qemu-devel] [PATCH 2/2] qapi: deprecate implicit filters
Date: Thu, 15 Aug 2019 12:49:28 +0200
User-agent: Mutt/1.11.3 (2019-02-01)

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 <address@hidden>
> > ---
> >  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 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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]