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

[Vladimir Sementsov-Ogievskiy]

14.08.2019 13:07, Vladimir Sementsov-Ogievskiy wrote:
> To get rid of implicit filters related workarounds in future let's
> deprecate them now.

Interesting, could we deprecate implicit filter without deprecation of 
unnecessity of
parameter? As actually, it's good when this parameter is not necessary, in most 
cases
user is not interested in node-name.

Obviously we can do the following:

1. In 4.2 we deprecate unnecessity, which implies deprecation of implicit 
filters
2. After some releases in 4.x we can drop deprecated functionality, so we drop 
it together with
implicit filters. And, in same release 4.x we return it back (as it's 
compatible change :)
but without implicit filters (so, if filter-node-name not specified, we just 
create
explicit filter with autogenerated node-name)

So, effectively we just drop "deprecation mark" together with implicit filters, 
which is nice
but actually confusing.

Instead, we may do
1. In 4.2 deprecate
2. In 4.x drop optionality together with implicit filters
3. In 4.y (y > x of course) return optionality back

It's a bit safer, but for users who miss releases [4.x, 4.y) it's no 
difference..

Or we just write in spec, that implicit filters are deprecated? But we have 
nothing about implicit
filters in spec. More over, we directly write that we have filter, and if 
parameter is omitted
it's node-name is autogenerated. So actually, the fact the filter is hidden 
when filter-node-name is
unspecified is _undocumented_.

So, finally, it looks like nothing to deprecated in specification, we can just 
drop implicit filters :)

What do you think?

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


-- 
Best regards,
Vladimir