It's hard and not necessary to maintain outdated versions of these commands. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov(a)virtuozzo.com&gt; --- qemu-deprecated.texi | 4 ++++ qapi/block-core.json | 4 ++++ qapi/transaction.json | 2 +- blockdev.c | 10 ++++++++++ 4 files changed, 19 insertions(+), 1 deletion(-) diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi index fff07bb2a3..2753fafd0b 100644 --- a/qemu-deprecated.texi +++ b/qemu-deprecated.texi @@ -179,6 +179,10 @@ and accurate ``query-qmp-schema'' command. Character devices creating sockets in client mode should not specify the 'wait' field, which is only applicable to sockets in server mode +@subsection drive-mirror, drive-backup and drive-backup transaction action (since 4.2) + +Use blockdev-mirror and blockdev-backup instead. + @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 0d43d4f37c..4e35526634 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -1635,6 +1635,8 @@ ## # @drive-backup: # +# Command is deprecated, use blockdev-mirror instead. +# # Start a point-in-time copy of a block device to a new destination. The # status of ongoing drive-backup operations can be checked with # query-block-jobs where the BlockJobInfo.type field has the value 'backup'. @@ -1855,6 +1857,8 @@ ## # @drive-mirror: # +# Command is deprecated, use blockdev-mirror instead. +# # Start mirroring a block device's writes to a new destination. target # specifies the target of the new image. If the file exists, or if it # is a device, it will be used as the new destination for writes. If diff --git a/qapi/transaction.json b/qapi/transaction.json index 95edb78227..a16a9ff8a6 100644 --- a/qapi/transaction.json +++ b/qapi/transaction.json @@ -53,7 +53,7 @@ # - @blockdev-snapshot: since 2.5 # - @blockdev-snapshot-internal-sync: since 1.7 # - @blockdev-snapshot-sync: since 1.1 -# - @drive-backup: since 1.6 +# - @drive-backup: deprecated action, since 1.6 # # Since: 1.1 ## diff --git a/blockdev.c b/blockdev.c index 4d141e9a1f..36e9368e01 100644 --- a/blockdev.c +++ b/blockdev.c @@ -1771,6 +1771,9 @@ static void drive_backup_prepare(BlkActionState *common, Error **errp) AioContext *aio_context; Error *local_err = NULL; + warn_report("drive-backup transaction action is deprecated and will " + "disappear in future. Use blockdev-backup action instead"); + assert(common->action->type == TRANSACTION_ACTION_KIND_DRIVE_BACKUP); backup = common->action->u.drive_backup.data; @@ -3591,6 +3594,10 @@ void qmp_drive_backup(DriveBackup *arg, Error **errp) { BlockJob *job; + + warn_report("drive-backup command is deprecated and will disappear in " + "future. Use blockdev-backup instead"); + job = do_drive_backup(arg, NULL, errp); if (job) { job_start(&job->job); @@ -3831,6 +3838,9 @@ void qmp_drive_mirror(DriveMirror *arg, Error **errp) const char *format = arg->format; int ret; + warn_report("drive-mirror command is deprecated and will disappear in " + "future. Use blockdev-mirror instead"); + bs = qmp_get_root_bs(arg->device, errp); if (!bs) { return;

On Wed, Aug 14, 2019 at 15:22:15 -0400, John Snow wrote: > > > On 8/14/19 6:07 AM, Vladimir Sementsov-Ogievskiy wrote: >> It's hard and not necessary to maintain outdated versions of these >> commands. >> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov(a)virtuozzo.com&gt; >> --- >> qemu-deprecated.texi | 4 ++++ >> qapi/block-core.json | 4 ++++ >> qapi/transaction.json | 2 +- >> blockdev.c | 10 ++++++++++ >> 4 files changed, 19 insertions(+), 1 deletion(-) >> >> diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi >> index fff07bb2a3..2753fafd0b 100644 >> --- a/qemu-deprecated.texi >> +++ b/qemu-deprecated.texi >> @@ -179,6 +179,10 @@ and accurate ``query-qmp-schema'' command. >> Character devices creating sockets in client mode should not specify >> the 'wait' field, which is only applicable to sockets in server mode >> >> +@subsection drive-mirror, drive-backup and drive-backup transaction action (since 4.2) >> + >> +Use blockdev-mirror and blockdev-backup instead. >> + >> @section Human Monitor Protocol (HMP) commands >> >> @subsection The hub_id parameter of 'hostfwd_add' / 'hostfwd_remove' (since 3.1) [...] >> @@ -3831,6 +3838,9 @@ void qmp_drive_mirror(DriveMirror *arg, Error **errp) >> const char *format = arg->format; >> int ret; >> >> + warn_report("drive-mirror command is deprecated and will disappear in " >> + "future. Use blockdev-mirror instead"); >> + >> bs = qmp_get_root_bs(arg->device, errp); >> if (!bs) { >> return; >> > > Hm! > > I wonder if this is ever-so-slightly too soon for our friends over at > the libvirt project. > > I don't think they have fully moved away from the non-blockdev > interfaces *just yet*, and I might encourage seeing the first full > libvirt release that does support and use it before we start the > deprecation clock. > > (Juuuust in case.) > > That's just me being very, very cautious though. > > Peter Krempa, how do you feel about this? Thanks for the heads up!

Currently libvirt does not use 'drive-backup' at all so that one can be deprecated immediately. In case of 'drive-mirror' the situation is a bit more complex: Libvirt uses 'drive-mirror' currently in the following places 1) virDomainBlockCopy API With blockdev integration enabled this will go away. Pathces are being reviewed: https://www.redhat.com/archives/libvir-list/2019-August/msg00295.html 2) VM migration with non-shared storage Currently uses 'drive-mirror' in most cases but there is pre-existing integration for blockdev-mirror for nbd+tls. I need to make sure that the blockdev version will be used unconditionally once the integration is enabled. This is a TODO. There is also one gotcha. In case when an 'sd' card device is used for the VM, libvirt disables all of blockdev, because SD cards can't be expressed with blockdev. There's too many code paths which would need checking to be worth it. To be fair, I'm not even sure when a sd card can be emulated by qemu as all of my basic tests failed and I did not care more. For libvirt to enable blockdev there's one more part missing and that's snapshot integration. I'm currently testing patches to integrate it with external snapshots, which should be posted soon. I also found a bug in qemu, which prevents creation of internal snapshots when -blockdev is used: When savevm HMP command is used (via QMP->HMP bridge) qemu invokes save_snapshot(), which calls bdrv_all_can_snapshot(). That function uses bdrv_next() to iterate all nodes which correspond to a block backend first, but then also iterates any other node which is monitor-owned. Since with blockdev all nodes including the ones for the 'file' protocol are monitor-owned, and 'file' does not support snapshots that check fails. A simple hack of skipping the second part in bdrv_next() allows to do a snapshot actually. Kevin told me that the idea is that also non-attached nodes should be considered for internal snapshod which is okay in my opinion, but given how the snapshot works for the files attached to backeds (and also in pre-blockdev use) only the top level of a chain should ever be considered for snapshot. So the summary is, that I'm pretty hopeful that we should be able to get rid of all reasonable uses of drive-mirror very soon after I finish snapshot integration. The only question is how much we care about SD card users being able to do a drive-mirror in the future.

To get rid of implicit filters related workarounds in future let's deprecate them now. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov(a)virtuozzo.com&gt; --- 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;

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(a)virtuozzo.com&gt; > --- > 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.

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(a)virtuozzo.com&gt; >> --- >> 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

Kevin Wolf <kwolf(a)redhat.com&gt; 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(a)virtuozzo.com&gt; >> >>> --- >> >>> 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 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.

John Snow <jsnow(a)redhat.com&gt; writes: > 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(a)virtuozzo.com&gt; >> --- [...] >> 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. This is a compatible evolution of the QMP protocol. > 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. Yes, we should help libvirt etc. with detecting use of deprecated features. We discussed this at the KVM Forum 2018 BoF on deprecating stuff. Minutes: Message-ID: <87mur0ls8o.fsf(a)dusky.pond.sub.org&gt; https://lists.nongnu.org/archive/html/qemu-devel/2018-10/msg05828.html Last item is relevant here. Adding deprecation information to QMP's success response belongs to "We can also pass the buck to the next layer up", next to "emit a QMP event". Let's compare the two, i.e. "deprecation info in success response" vs. "deprecation event". 1. Possible triggers Anything we put in the success response should only ever apply to the (successful) command. So this one's limited to QMP commands. A QMP event is not limited to QMP commands. For instance, it could be emitted for deprecated CLI features (long after the fact, in addition to human-readable warnings on stderr), or when we detect use of a deprecated feature only after we sent the success response, say in a job. Neither use case is particularly convincing. Reporting use of deprecated CLI in QMP feels like a work-around for the CLI's machine-unfriendliness. Job-like commands should really check their arguments upfront. 2. Connection to trigger Connecting responses to commands is the QMP protocol's responsibility. Transmitting deprecation information in the response trivially ties it to the offending command. The QMP protocol doesn't tie events to anything. Thus, a deprecation event needs an event-specific tie to its trigger. The obvious way to tie it to a command mirrors how the QMP protocol ties responses to commands: by command ID. The event either has to be sent just to the offending monitor (currently, all events are broadcast to all monitors), or include a suitable monitor ID. For non-command triggers, we'd have to invent some other tie. 3. Interface complexity Tying the event to some arbitrary trigger adds complexity. Do we need non-command triggers, and badly enough to justify the additional complexity? 4. Implementation complexity Emitting an event could be as simple as qapi_event_send_deprecated(qmp_command_id(), "Omitting 'filter-node-name'"); where qmp_command_id() returns the ID of the currently executing command. Making qmp_command_id() work is up to the QMP core. Simple enough as long as each QMP command runs to completion before its monitor starts the next one. The event is "fire and forget". There is no warning object propagated up the call chain into the QMP core like errors objects are. "Fire and forget" is ideal for letting arbitrary code decide "this is deprecated". Note the QAPI schema remains untouched. Unlike an event, which can be emitted anywhere, the success response gets built in the QMP core. To have the core add deprecation info to it, we need to get the info to the core. If deprecation info originates in command code, like errors do, we need to propagate it up the call chain into the QMP core like errors. 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. Kevin recently added "features" to the QAPI schema language. The implementation is incomplete, but that's detail. The idea is to tack a "deprecated" feature to deprecated stuff in the QAPI schema.

Commands and arguments need to support features for that. Implementation should be relatively straightforward. Deprecating an argument's optionalness may require a "optional-deprecated" feature. I've seen more elegant designs, but I've also seen plenty of uglier ones. Note that features are tied to schema syntax. To express semantically conditional deprecation like "if you specify argument FOO, then not specifying argument BAR is deprecated", we'd have to add a language for these conditions. Uh, not now, maybe never. The primary use of having deprecation defined in the QAPI schema is introspection. The BoF minutes mention this, too. A secondary use could be detecting use of deprecated features right in the QMP core. No need for ad hoc code in commands, no need for plumbing information from there to the QMP core. I'd like to pursue this idea, then see how well it suits our deprecation needs.

To get rid of implicit filters related workarounds in future let's deprecate them now.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov(a)virtuozzo.com&gt; --- 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;