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

[John Snow]

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

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

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.

--js