[Qemu-devel] [PATCH 0/2] Deprecate implicit filters

[Qemu-devel] [PATCH 0/2] Deprecate implicit filters

[Vladimir Sementsov-Ogievskiy]

Hi all!

Max's series to fix some problems around filters consists of 42 patches.
I'm sure that we didn't find all bugs around filters, and that filters
would be a constant source of bugs in future, as during developing new
feature nobody will consider all possible cases of dealing with filters
(OK, somebody will, but it's hard).

So, I'm thinking about starting some deprecations which will help us to
simplify all the picture at least in not far future. So, I'd really want
to deprecate implicit filters, ->file child based filters (move all
filters to use backing child), filename based interfaces (use node-names).

Most simple thing is implicit filters, so let's start from them.
drive-mirror don't support filter-node-name, so I propose to deprecate
it at all, together with drive-backup, instead of adding support of
filter-node-name, what do you think?

Vladimir Sementsov-Ogievskiy (2):
  qapi: deprecate drive-mirror and drive-backup
  qapi: deprecate implicit filters

 qemu-deprecated.texi      | 11 +++++++++++
 qapi/block-core.json      | 10 ++++++++--
 qapi/transaction.json     |  2 +-
 include/block/block_int.h | 10 +++++++++-
 blockdev.c                | 20 ++++++++++++++++++++
 5 files changed, 49 insertions(+), 4 deletions(-)

-- 
2.18.0

[Qemu-devel] [PATCH 1/2] qapi: deprecate drive-mirror and drive-backup

[Vladimir Sementsov-Ogievskiy]

It's hard and not necessary to maintain outdated versions of these
commands.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 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;
-- 
2.18.0

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

[Vladimir Sementsov-Ogievskiy]

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

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@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.
 
+@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;
-- 
2.18.0

Re: [Qemu-devel] [PATCH 1/2] qapi: deprecate drive-mirror and drive-backup

[John Snow]

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@virtuozzo.com>
> ---
>  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;
> 

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?

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

[John Snow]

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

--js

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

[Maxim Levitsky]

On Wed, 2019-08-14 at 15:27 -0400, John Snow wrote:
> 
> 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@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.
> >  
> > +@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.
> 
> --js
> 
This is a very good idea IMHO.

Best regards,
	Maxim Levitsky

Re: [Qemu-devel] [libvirt] [PATCH 1/2] qapi: deprecate drive-mirror and drive-backup

[Peter Krempa]

[-- Attachment #1: Type: text/plain, Size: 4615 bytes --]

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@virtuozzo.com>
> > ---
> >  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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Kevin Wolf]

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

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

[Peter Krempa]

[-- Attachment #1: Type: text/plain, Size: 1519 bytes --]

On Thu, Aug 15, 2019 at 12:49:28 +0200, Kevin Wolf wrote:
> Am 14.08.2019 um 21:27 hat John Snow geschrieben:

[...]

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

The best we could do here is to log a warning. Thankfully we have one
central function which always checks the returned JSON from qemu so we
could do that universally.

The would end up in the system log and alternatively also in the VM
log file. I agree with Kevin that the possibility of it being noticed
is rather small.

From my experience users report non-fatal messages mostly only if it is
spamming the system log. One of instances are very unlikely to be
noticed.

In my experience it's better to notify us in libvirt of such change and
we will try our best to fix it.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Markus Armbruster]

Peter Krempa <pkrempa@redhat.com> writes:

> On Thu, Aug 15, 2019 at 12:49:28 +0200, Kevin Wolf wrote:
>> Am 14.08.2019 um 21:27 hat John Snow geschrieben:
>
> [...]
>
>> > 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.
>
> The best we could do here is to log a warning. Thankfully we have one
> central function which always checks the returned JSON from qemu so we
> could do that universally.
>
> The would end up in the system log and alternatively also in the VM
> log file. I agree with Kevin that the possibility of it being noticed
> is rather small.
>
> From my experience users report non-fatal messages mostly only if it is
> spamming the system log. One of instances are very unlikely to be
> noticed.
>
> In my experience it's better to notify us in libvirt of such change and
> we will try our best to fix it.

How to best alert the layers above QEMU was one of the topic of the KVM
Forum 2018 BoF on deprecating stuff.  Minutes:

    Message-ID: <87mur0ls8o.fsf@dusky.pond.sub.org>
    https://lists.nongnu.org/archive/html/qemu-devel/2018-10/msg05828.html

Relevant part:

* We need to communicate "you're using something that is deprecated".
  How?  Right now, we print a deprecation message.  Okay when humans use
  QEMU directly in a shell.  However, when QEMU sits at the bottom of a
  software stack, the message will likely end up in a log file that is
  effectively write-only.
 
  - The one way to get people read log files is crashing their
    application.  A command line option --future could make QEMU crash
    right after printing a deprecation message.  This could help with
    finding use of deprecated features in a testing environment.

  - A less destructive way to grab people's attention is to make things
    run really, really slow: have QEMU go to sleep for a while after
    printing a deprecation message.
    
  - We can also pass the buck to the next layer up: emit a QMP event.

    Sadly, by the time the next layer connects to QMP, plenty of stuff
    already happened.  We'd have to buffer deprecation events somehow.

    What would libvirt do with such an event?  Log it, taint the domain,
    emit a (libvirt) event to pass it on to the next layer up.

  - A completely different idea is to have a configuratin linter.  To
    support doing this at the libvirt level, QEMU could expose "is
    deprecated" in interface introspection.  Feels feasible for QMP,
    where we already have sufficiently expressive introspection.  For
    CLI, we'd first have to provide that (but we want that anyway).

  - We might also want to dispay deprecation messages in QEMU's GUI
    somehow, or on serial consoles.

[Qemu-devel] Exposing feature deprecation to machine clients (was: [PATCH 2/2] qapi: deprecate implicit filters)

[Markus Armbruster]

John Snow <jsnow@redhat.com> 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@virtuozzo.com>
>> ---
[...]
>> 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@dusky.pond.sub.org>
    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.

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 <vsementsov@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.
>>>  
>>> +@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

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

[Kevin Wolf]

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@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.
> >>>  
> >>> +@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).

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

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

Kevin

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

[John Snow]

On 8/15/19 12:48 PM, Kevin Wolf wrote:
> 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:
>>>>
>>>> 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).
> 
>> 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.
> 

That the message we return needs to be *useful* doesn't sound like a
count against it.

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

No, because until recently we didn't deprecate anything.

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

Yeah, I think there's solid use there. I'll continue along in Markus's
thread.

> Kevin
> 

Re: [Qemu-devel] Exposing feature deprecation to machine clients (was: [PATCH 2/2] qapi: deprecate implicit filters)

[John Snow]

On 8/15/19 10:16 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> 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@virtuozzo.com>
>>> ---
> [...]
>>> 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@dusky.pond.sub.org>
>     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.
> 

Probably the best argument against it. Fire-and-forget avoids the
problem. Events might work just fine, but the "tie" bit seems like a yak
in need of a shave.

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

That's a good idea too; but the semantics of exactly *what* was
deprecated may be hard to capture.

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

I should clearly remember to talk to you before thinking about QMP in
public, because you've thought about it much more.

--js

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

[Markus Armbruster]

Kevin Wolf <kwolf@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@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.
>> >>>  
>> >>> +@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.

Re: [Qemu-devel] [libvirt] [PATCH 1/2] qapi: deprecate drive-mirror and drive-backup

[John Snow]

On 8/15/19 3:44 AM, Peter Krempa wrote:
> 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@virtuozzo.com>
>>> ---
>>>  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!
> 

You're welcome!

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

OK. It sounds like we should hold off on deprecating these for now
because it's not certain which libvirt release will no longer need them,
but it sounds like it's hopefully not far off.

--js

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

[Kevin Wolf]

Am 15.08.2019 um 21:24 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@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@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.
> >> >>>  
> >> >>> +@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.

I'm not saying that deprecation notices would hurt, just that they
probably won't solve problem alone.

Crashing if --future is given and logging otherwise seems reasonable
enough to me. Whether we need to wire up a new deprecation mechanism in
QMP for the logging or if we can just keep printing to stderr is
debatable. stderr already ends up in a log file, a QMP extension would
require new libvirt code. If libvirt would log deprecation notices more
prominently, or use the information for tainting or any other kind of
processing, a dedicated QMP mechanism could be justified.

Kevin

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

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 15.08.2019 um 21:24 hat Markus Armbruster geschrieben:
[...]
>> 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.
>
> I'm not saying that deprecation notices would hurt, just that they
> probably won't solve problem alone.

No argument.

> Crashing if --future is given and logging otherwise seems reasonable
> enough to me. Whether we need to wire up a new deprecation mechanism in
> QMP for the logging or if we can just keep printing to stderr is
> debatable. stderr already ends up in a log file, a QMP extension would
> require new libvirt code. If libvirt would log deprecation notices more
> prominently, or use the information for tainting or any other kind of
> processing, a dedicated QMP mechanism could be justified.

I'd like to start with two tasks:

* A CLI option to configure what to do on use of a deprecated feature.

  We currently warn.  We want to be able to crash instead.  Silencing
  the warnings might be useful.  Turning them into errors might be
  useful.

  The existing ad hoc warnings need to be replaced by a call of a common
  function that implements the configurable behavior.

* QAPI feature flag "deprecated", for introspectable deprecation, and
  without ad hoc code.

Then see whether our users need more.

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

[Vladimir Sementsov-Ogievskiy]

16.08.2019 15:33, Markus Armbruster wrote:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
>> Am 15.08.2019 um 21:24 hat Markus Armbruster geschrieben:
> [...]
>>> 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.
>>
>> I'm not saying that deprecation notices would hurt, just that they
>> probably won't solve problem alone.
> 
> No argument.
> 
>> Crashing if --future is given and logging otherwise seems reasonable
>> enough to me. Whether we need to wire up a new deprecation mechanism in
>> QMP for the logging or if we can just keep printing to stderr is
>> debatable. stderr already ends up in a log file, a QMP extension would
>> require new libvirt code. If libvirt would log deprecation notices more
>> prominently, or use the information for tainting or any other kind of
>> processing, a dedicated QMP mechanism could be justified.
> 
> I'd like to start with two tasks:
> 
> * A CLI option to configure what to do on use of a deprecated feature.
> 
>    We currently warn.  We want to be able to crash instead.  Silencing
>    the warnings might be useful.  Turning them into errors might be
>    useful.
> 
>    The existing ad hoc warnings need to be replaced by a call of a common
>    function that implements the configurable behavior.
> 
> * QAPI feature flag "deprecated", for introspectable deprecation, and
>    without ad hoc code.
> 
> Then see whether our users need more.
> 

Crashing is useful for libvirt developers, it's obvious, just enable crash-on-deprecated
on all testing environments and most probably we will not miss such a case.

For qapi I doubt is it really needed. Implementing code in libvirt which will check for command
(or it's parameter, or it's parameter "optionality" is deprecated) ? It's hard and what libvirt
should report to final user? It becomes a kind of synthetic error in libvirt code, like

...
log_error("We are going to divide by zero. It's a bug, please report it to developers!");
x = a / 0;
...

It's simpler to fix second line than implement special mechanism including protocol specification
to report such a case.

I exaggerate of course with this example, but I doubt that implementing a special protocol
for it worth doing. And I think notifying libvirt by email (as Peter said) and providing option
"crash-on-deprecated" in Qemu are enough for libvirt developers to prevent and to fix using
deprecated things.

In other words, I don't see why reporting deprecated feature usage is better in libvirt than in
Qemu (by warning, error or crash), and in Qemu it's much more simple and don't need QAPI protocol
extension.

(I'm sorry if I'm repeating already written arguments, I've not read the whole thread)

-- 
Best regards,
Vladimir

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 <vsementsov@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.
>   
> +@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

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

[John Snow]

On 8/23/19 5:22 AM, Vladimir Sementsov-Ogievskiy wrote:
> 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.
> 

https://en.wiktionary.org/wiki/unnecessity -- I am surprised to learn
that this a real word in the language I speak. :)

I assume you're referring to making the optional argument mandatory.

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

Ah, I see what you're digging at here now...

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

What exactly _IS_ an implicit filter? How does it differ today from an
explicit filter? I assumed the only difference was if it was named or
not; but I think I must be mistaken now if you're proposing leaving the
interface alone entirely.

Are they instantiated differently?

--js

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

[Vladimir Sementsov-Ogievskiy]

27.08.2019 23:12, John Snow wrote:
> 
> 
> On 8/23/19 5:22 AM, Vladimir Sementsov-Ogievskiy wrote:
>> 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.
>>
> 
> https://en.wiktionary.org/wiki/unnecessity -- I am surprised to learn
> that this a real word in the language I speak. :)
> 
> I assume you're referring to making the optional argument mandatory.

exactly, it's my a bit "google-translate-driven" English)

> 
>> 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
>>
> 
> Ah, I see what you're digging at here now...
> 
>> 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?
>>
> 
> What exactly _IS_ an implicit filter? How does it differ today from an
> explicit filter? I assumed the only difference was if it was named or
> not; but I think I must be mistaken now if you're proposing leaving the
> interface alone entirely.
> 
> Are they instantiated differently?
> 

As I understand, the only difference is their BlockDriverState.impicit field, and several places in code
where we skip implicit filter when trying to find something in a chain starting from a device.

Hmm, OK, let's see:

1. the only implicit filters are commit_top and mirror_top if user don't specify filter-node-name.

Where it make sense, i.e., where implicit field used?

2. bdrv_query_info, bdrv_query_bds_stats, bdrv_block_device_info(only when called from bdrv_query_info), they'll
report filter as top node if we don't mark it implicit.

3. bdrv_refresh_filename, bdrv_reopen_parse_backing, bdrv_drop_intermediate:
   I think it's not a problem, just drop special case for implicit fitlers

So, seems the only real change is query-block and query-blockstats output when mirror or commit is started
without specifying filter-node-name (filter would be on top)

So, how should we deprecate this, or can we just change it?

-- 
Best regards,
Vladimir

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

[John Snow]

(Peter: search for "pkrempa" down below.)

On 8/28/19 5:20 AM, Vladimir Sementsov-Ogievskiy wrote:
> 27.08.2019 23:12, John Snow wrote:
>>
>>
>> On 8/23/19 5:22 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> 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.
>>>
>>
>> https://en.wiktionary.org/wiki/unnecessity -- I am surprised to learn
>> that this a real word in the language I speak. :)
>>
>> I assume you're referring to making the optional argument mandatory.
> 
> exactly, it's my a bit "google-translate-driven" English)
> 

It teaches me some fun words!

>>
>>> 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
>>>
>>
>> Ah, I see what you're digging at here now...
>>
>>> 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?
>>>
>>
>> What exactly _IS_ an implicit filter? How does it differ today from an
>> explicit filter? I assumed the only difference was if it was named or
>> not; but I think I must be mistaken now if you're proposing leaving the
>> interface alone entirely.
>>
>> Are they instantiated differently?
>>
> 
> As I understand, the only difference is their BlockDriverState.impicit field, and several places in code
> where we skip implicit filter when trying to find something in a chain starting from a device.
> 

Oh, oh, yes. I see.

> Hmm, OK, let's see:
> 
> 1. the only implicit filters are commit_top and mirror_top if user don't specify filter-node-name.
> 
> Where it make sense, i.e., where implicit field used?
> 

`git grep -E '(->|\.)implicit'` is what I used to find usages.

> 2. bdrv_query_info, bdrv_query_bds_stats, bdrv_block_device_info(only when called from bdrv_query_info), they'll
> report filter as top node if we don't mark it implicit.
> 

So that's a bit of a change, but only visually. The "reality" is still
the same, we just report it more "accurately." libvirt MIGHT need a
heads up here. I'm looping pkrempa back in for comment.

<pkrempa>
Would libvirt be negatively impacted by the revelation of formerly
internal ("implicit") nodes created by mirror and commit via query block
commands? At the moment, QEMU hides them from you if you do not name them.
</pkrempa>

> 3. bdrv_refresh_filename, bdrv_reopen_parse_backing, bdrv_drop_intermediate:
>    I think it's not a problem, just drop special case for implicit fitlers
>

I'm much less certain about what the impact of this would be and would
need to audit it (and don't have the time to, personally.)

Do you have a POC or RFC patch that demonstrates dropping these special
cases? It might be nice to see as proof that it's safe to deprecate.

> So, seems the only real change is query-block and query-blockstats output when mirror or commit is started
> without specifying filter-node-name (filter would be on top)
> 
> So, how should we deprecate this, or can we just change it?
> 

I'm not sure if it's worth it yet, what does dropping the implicit field
buy us? Conceptually I understand that it's simpler without the notion
of implicit fields, but I imagine there's some cleanup in particular
that motivated this.

I'd say to just change the behavior, we should:

- Give a standard three-release warning that the behavior will change in
an incompatible way
- Demonstrate with an RFC patch that special cases around ->implicit in
block.c can be removed and do not make the code more complex,
- Get blessings from Peter Krempa.

As always: Libvirt is not the end-all be-all of QEMU management, but if
libvirt is capable of working around design changes then I believe any
project out there today also could, so it's a good litmus test.

--js

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

[Peter Krempa]

[-- Attachment #1: Type: text/plain, Size: 2917 bytes --]

On Wed, Aug 28, 2019 at 13:48:10 -0400, John Snow wrote:
> (Peter: search for "pkrempa" down below.)
> 
> On 8/28/19 5:20 AM, Vladimir Sementsov-Ogievskiy wrote:

[....]


> So that's a bit of a change, but only visually. The "reality" is still
> the same, we just report it more "accurately." libvirt MIGHT need a
> heads up here. I'm looping pkrempa back in for comment.
> 
> <pkrempa>
> Would libvirt be negatively impacted by the revelation of formerly
> internal ("implicit") nodes created by mirror and commit via query block
> commands? At the moment, QEMU hides them from you if you do not name them.

Currently we would not be able to handle that properly at least
definitely in the pre-blockdev case. In blockdev case I must make sure
that it will work.

The thing is that I didn't really want to touch the pre-blockdev case
code any more, but if you decide that we should do it I'm willing to
investigate this case also for the old commands.

> </pkrempa>
> 
> > 3. bdrv_refresh_filename, bdrv_reopen_parse_backing, bdrv_drop_intermediate:
> >    I think it's not a problem, just drop special case for implicit fitlers
> >
> 
> I'm much less certain about what the impact of this would be and would
> need to audit it (and don't have the time to, personally.)
> 
> Do you have a POC or RFC patch that demonstrates dropping these special
> cases? It might be nice to see as proof that it's safe to deprecate.
> 
> > So, seems the only real change is query-block and query-blockstats output when mirror or commit is started
> > without specifying filter-node-name (filter would be on top)
> > 
> > So, how should we deprecate this, or can we just change it?
> > 
> 
> I'm not sure if it's worth it yet, what does dropping the implicit field
> buy us? Conceptually I understand that it's simpler without the notion
> of implicit fields, but I imagine there's some cleanup in particular
> that motivated this.
> 
> I'd say to just change the behavior, we should:
> 
> - Give a standard three-release warning that the behavior will change in
> an incompatible way
> - Demonstrate with an RFC patch that special cases around ->implicit in
> block.c can be removed and do not make the code more complex,
> - Get blessings from Peter Krempa.
> 
> As always: Libvirt is not the end-all be-all of QEMU management, but if
> libvirt is capable of working around design changes then I believe any
> project out there today also could, so it's a good litmus test.

For libvirt we really care more whether a node is format/protocol
related or not rather than whether it's implicit or not.

In this case we could filter it by the known protocol and format driver
types and filter out the rest in cases when we e.g. detect the node
names for the pre-blockdev era cases.

(Note that even with new qemu, if an SD card is used blockdev will be
disabled).


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Vladimir Sementsov-Ogievskiy]

28.08.2019 20:48, John Snow wrote:
> (Peter: search for "pkrempa" down below.)
> 
> On 8/28/19 5:20 AM, Vladimir Sementsov-Ogievskiy wrote:
>> 27.08.2019 23:12, John Snow wrote:
>>>
>>>
>>> On 8/23/19 5:22 AM, Vladimir Sementsov-Ogievskiy wrote:
>>>> 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.
>>>>
>>>
>>> https://en.wiktionary.org/wiki/unnecessity -- I am surprised to learn
>>> that this a real word in the language I speak. :)
>>>
>>> I assume you're referring to making the optional argument mandatory.
>>
>> exactly, it's my a bit "google-translate-driven" English)
>>
> 
> It teaches me some fun words!
> 
>>>
>>>> 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
>>>>
>>>
>>> Ah, I see what you're digging at here now...
>>>
>>>> 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?
>>>>
>>>
>>> What exactly _IS_ an implicit filter? How does it differ today from an
>>> explicit filter? I assumed the only difference was if it was named or
>>> not; but I think I must be mistaken now if you're proposing leaving the
>>> interface alone entirely.
>>>
>>> Are they instantiated differently?
>>>
>>
>> As I understand, the only difference is their BlockDriverState.impicit field, and several places in code
>> where we skip implicit filter when trying to find something in a chain starting from a device.
>>
> 
> Oh, oh, yes. I see.
> 
>> Hmm, OK, let's see:
>>
>> 1. the only implicit filters are commit_top and mirror_top if user don't specify filter-node-name.
>>
>> Where it make sense, i.e., where implicit field used?
>>
> 
> `git grep -E '(->|\.)implicit'` is what I used to find usages.
> 
>> 2. bdrv_query_info, bdrv_query_bds_stats, bdrv_block_device_info(only when called from bdrv_query_info), they'll
>> report filter as top node if we don't mark it implicit.
>>
> 
> So that's a bit of a change, but only visually. The "reality" is still
> the same, we just report it more "accurately." libvirt MIGHT need a
> heads up here. I'm looping pkrempa back in for comment.
> 
> <pkrempa>
> Would libvirt be negatively impacted by the revelation of formerly
> internal ("implicit") nodes created by mirror and commit via query block
> commands? At the moment, QEMU hides them from you if you do not name them.
> </pkrempa>
> 
>> 3. bdrv_refresh_filename, bdrv_reopen_parse_backing, bdrv_drop_intermediate:
>>     I think it's not a problem, just drop special case for implicit fitlers
>>
> 
> I'm much less certain about what the impact of this would be and would
> need to audit it (and don't have the time to, personally.)
> 
> Do you have a POC or RFC patch that demonstrates dropping these special
> cases? It might be nice to see as proof that it's safe to deprecate.
> 
>> So, seems the only real change is query-block and query-blockstats output when mirror or commit is started
>> without specifying filter-node-name (filter would be on top)
>>
>> So, how should we deprecate this, or can we just change it?
>>
> 
> I'm not sure if it's worth it yet, what does dropping the implicit field
> buy us? Conceptually I understand that it's simpler without the notion
> of implicit fields, but I imagine there's some cleanup in particular
> that motivated this.

Reviewing Max's "block: Deal with filters" series motivated me.

> 
> I'd say to just change the behavior, we should:
> 
> - Give a standard three-release warning that the behavior will change in
> an incompatible way
> - Demonstrate with an RFC patch that special cases around ->implicit in
> block.c can be removed and do not make the code more complex,
> - Get blessings from Peter Krempa.
> 
> As always: Libvirt is not the end-all be-all of QEMU management, but if
> libvirt is capable of working around design changes then I believe any
> project out there today also could, so it's a good litmus test.
> 
> --js
> 


-- 
Best regards,
Vladimir

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

[Vladimir Sementsov-Ogievskiy]

29.08.2019 18:00, Vladimir Sementsov-Ogievskiy wrote:
> 28.08.2019 20:48, John Snow wrote:
>> (Peter: search for "pkrempa" down below.)
>>
>> On 8/28/19 5:20 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> 27.08.2019 23:12, John Snow wrote:
>>>>
>>>>
>>>> On 8/23/19 5:22 AM, Vladimir Sementsov-Ogievskiy wrote:
>>>>> 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.
>>>>>
>>>>
>>>> https://en.wiktionary.org/wiki/unnecessity -- I am surprised to learn
>>>> that this a real word in the language I speak. :)
>>>>
>>>> I assume you're referring to making the optional argument mandatory.
>>>
>>> exactly, it's my a bit "google-translate-driven" English)
>>>
>>
>> It teaches me some fun words!
>>
>>>>
>>>>> 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
>>>>>
>>>>
>>>> Ah, I see what you're digging at here now...
>>>>
>>>>> 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?
>>>>>
>>>>
>>>> What exactly _IS_ an implicit filter? How does it differ today from an
>>>> explicit filter? I assumed the only difference was if it was named or
>>>> not; but I think I must be mistaken now if you're proposing leaving the
>>>> interface alone entirely.
>>>>
>>>> Are they instantiated differently?
>>>>
>>>
>>> As I understand, the only difference is their BlockDriverState.impicit field, and several places in code
>>> where we skip implicit filter when trying to find something in a chain starting from a device.
>>>
>>
>> Oh, oh, yes. I see.
>>
>>> Hmm, OK, let's see:
>>>
>>> 1. the only implicit filters are commit_top and mirror_top if user don't specify filter-node-name.
>>>
>>> Where it make sense, i.e., where implicit field used?
>>>
>>
>> `git grep -E '(->|\.)implicit'` is what I used to find usages.
>>
>>> 2. bdrv_query_info, bdrv_query_bds_stats, bdrv_block_device_info(only when called from bdrv_query_info), they'll
>>> report filter as top node if we don't mark it implicit.
>>>
>>
>> So that's a bit of a change, but only visually. The "reality" is still
>> the same, we just report it more "accurately." libvirt MIGHT need a
>> heads up here. I'm looping pkrempa back in for comment.
>>
>> <pkrempa>
>> Would libvirt be negatively impacted by the revelation of formerly
>> internal ("implicit") nodes created by mirror and commit via query block
>> commands? At the moment, QEMU hides them from you if you do not name them.
>> </pkrempa>
>>
>>> 3. bdrv_refresh_filename, bdrv_reopen_parse_backing, bdrv_drop_intermediate:
>>>     I think it's not a problem, just drop special case for implicit fitlers
>>>
>>
>> I'm much less certain about what the impact of this would be and would
>> need to audit it (and don't have the time to, personally.)
>>
>> Do you have a POC or RFC patch that demonstrates dropping these special
>> cases? It might be nice to see as proof that it's safe to deprecate.

I faced a problem with some iotest (it's not a surprise of course), but I think, I'll
come back with and RFC of course.

>>
>>> So, seems the only real change is query-block and query-blockstats output when mirror or commit is started
>>> without specifying filter-node-name (filter would be on top)
>>>
>>> So, how should we deprecate this, or can we just change it?
>>>
>>
>> I'm not sure if it's worth it yet, what does dropping the implicit field
>> buy us? Conceptually I understand that it's simpler without the notion
>> of implicit fields, but I imagine there's some cleanup in particular
>> that motivated this.
> 
> Reviewing Max's "block: Deal with filters" series motivated me.


Currently, we just mostly don't care about implicit filters.

your command on master:
# git grep -E '(->|\.)implicit'
block.c:    while (backing_bs(overlay_bs) && backing_bs(overlay_bs)->implicit) {
block.c:        assert(!old_backing_bs || !old_backing_bs->implicit);
block.c:    while (explicit_top && explicit_top->implicit) {
block.c:    if (bs->implicit) {
block/commit.c:        commit_top_bs->implicit = true;
block/mirror.c:        mirror_top_bs->implicit = true;
block/qapi.c:        while (blk && bs0->drv && bs0->implicit) {
block/qapi.c:    while (bs && bs->drv && bs->implicit) {
block/qapi.c:    while (blk_level && bs->drv && bs->implicit) {


on Max's series:

# git grep -E '(->|\.)implicit'
block.c:           bdrv_filtered_bs(overlay_bs)->implicit)
block.c:        assert(!old_backing_bs || !old_backing_bs->implicit);
block.c:    if (bs->implicit) {
block.c:    while (!(stop_on_explicit_filter && !bs->implicit)) {
block/commit.c:        commit_top_bs->implicit = true;
block/mirror.c:        mirror_top_bs->implicit = true;

seems better, but actually, we get new function bdrv_skip_implicit_filters:
# git grep bdrv_skip_implicit_filters
block.c:    explicit_top = bdrv_skip_implicit_filters(explicit_top);
block.c:BlockDriverState *bdrv_skip_implicit_filters(BlockDriverState *bs)
block/block-backend.c:            non_filter = bdrv_skip_implicit_filters(blk->root->bs);
block/qapi.c:            bs0 = bdrv_skip_implicit_filters(bs0);
block/qapi.c:        bs = bdrv_skip_implicit_filters(bs);
block/qapi.c:        bs = bdrv_skip_implicit_filters(bs);
blockdev.c:        bs = bdrv_skip_implicit_filters(blk_bs(blk));
blockdev.c:                bdrv_skip_implicit_filters(source);
blockdev.c:    unfiltered_bs = bdrv_skip_implicit_filters(bs);
blockdev.c:        BlockDriverState *explicit_backing = bdrv_skip_implicit_filters(source);
blockdev.c:    unfiltered_bs = bdrv_skip_implicit_filters(bs);
include/block/block_int.h:BlockDriverState *bdrv_skip_implicit_filters(BlockDriverState *bs);


So, if we are going to really support implicit filters, we have to support them everywhere.
I'm not sure that Max's series covered all cases. We don't have strong definition of implicit
filters, or about have implicit and explicit filters should be interleaved on appending.

That's why I think it's better to drop them earlier, as it seems that we are going to increase
filter usage in block-layer.

> 
>>
>> I'd say to just change the behavior, we should:
>>
>> - Give a standard three-release warning that the behavior will change in
>> an incompatible way
>> - Demonstrate with an RFC patch that special cases around ->implicit in
>> block.c can be removed and do not make the code more complex,
>> - Get blessings from Peter Krempa.
>>
>> As always: Libvirt is not the end-all be-all of QEMU management, but if
>> libvirt is capable of working around design changes then I believe any
>> project out there today also could, so it's a good litmus test.
>>
>> --js
>>
> 
> 


-- 
Best regards,
Vladimir

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

[Vladimir Sementsov-Ogievskiy]

29.08.2019 17:44, Peter Krempa wrote:
> On Wed, Aug 28, 2019 at 13:48:10 -0400, John Snow wrote:
>> (Peter: search for "pkrempa" down below.)
>>
>> On 8/28/19 5:20 AM, Vladimir Sementsov-Ogievskiy wrote:
> 
> [....]
> 
> 
>> So that's a bit of a change, but only visually. The "reality" is still
>> the same, we just report it more "accurately." libvirt MIGHT need a
>> heads up here. I'm looping pkrempa back in for comment.
>>
>> <pkrempa>
>> Would libvirt be negatively impacted by the revelation of formerly
>> internal ("implicit") nodes created by mirror and commit via query block
>> commands? At the moment, QEMU hides them from you if you do not name them.
> 
> Currently we would not be able to handle that properly at least
> definitely in the pre-blockdev case. In blockdev case I must make sure
> that it will work.
> 
> The thing is that I didn't really want to touch the pre-blockdev case
> code any more,

Aren't you going to deprecate and drop it at some moment?

  but if you decide that we should do it I'm willing to
> investigate this case also for the old commands.
> 
>> </pkrempa>
>>
>>> 3. bdrv_refresh_filename, bdrv_reopen_parse_backing, bdrv_drop_intermediate:
>>>     I think it's not a problem, just drop special case for implicit fitlers
>>>
>>
>> I'm much less certain about what the impact of this would be and would
>> need to audit it (and don't have the time to, personally.)
>>
>> Do you have a POC or RFC patch that demonstrates dropping these special
>> cases? It might be nice to see as proof that it's safe to deprecate.
>>
>>> So, seems the only real change is query-block and query-blockstats output when mirror or commit is started
>>> without specifying filter-node-name (filter would be on top)
>>>
>>> So, how should we deprecate this, or can we just change it?
>>>
>>
>> I'm not sure if it's worth it yet, what does dropping the implicit field
>> buy us? Conceptually I understand that it's simpler without the notion
>> of implicit fields, but I imagine there's some cleanup in particular
>> that motivated this.
>>
>> I'd say to just change the behavior, we should:
>>
>> - Give a standard three-release warning that the behavior will change in
>> an incompatible way
>> - Demonstrate with an RFC patch that special cases around ->implicit in
>> block.c can be removed and do not make the code more complex,
>> - Get blessings from Peter Krempa.
>>
>> As always: Libvirt is not the end-all be-all of QEMU management, but if
>> libvirt is capable of working around design changes then I believe any
>> project out there today also could, so it's a good litmus test.
> 
> For libvirt we really care more whether a node is format/protocol
> related or not rather than whether it's implicit or not.
> 
> In this case we could filter it by the known protocol and format driver
> types and filter out the rest in cases when we e.g. detect the node
> names for the pre-blockdev era cases.
> 
> (Note that even with new qemu, if an SD card is used blockdev will be
> disabled).
> 


-- 
Best regards,
Vladimir

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

[Christophe de Dinechin]

John Snow writes:
[...]
>
> 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.

I like this approach even if there is no consumer today. It does not
hurt, and it is indeed a motivation to develop consumers that care.

I personally find this much easier to swallow than any kind of crash on
deprecation, which already at the BoF seemed like a really big hammer to
kill a fly.

CC'ing Andrea as well, because we discussed recently about how to deal
with error checking in general, and if a new error checking framework is
being put in place, adding deprecation to the thinking could be a good
idea.

--
Cheers,
Christophe de Dinechin (IRC c3d)

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

[Christophe de Dinechin]

Markus Armbruster writes:

> Peter Krempa <pkrempa@redhat.com> writes:
>
[...]
>> From my experience users report non-fatal messages mostly only if it is
>> spamming the system log. One of instances are very unlikely to be
>> noticed.
>>
>> In my experience it's better to notify us in libvirt of such change and
>> we will try our best to fix it.
>
> How to best alert the layers above QEMU was one of the topic of the KVM
> Forum 2018 BoF on deprecating stuff.  Minutes:
>
>     Message-ID: <87mur0ls8o.fsf@dusky.pond.sub.org>
>     https://lists.nongnu.org/archive/html/qemu-devel/2018-10/msg05828.html
>
> Relevant part:
>
> * We need to communicate "you're using something that is deprecated".
>   How?  Right now, we print a deprecation message.  Okay when humans use
>   QEMU directly in a shell.  However, when QEMU sits at the bottom of a
>   software stack, the message will likely end up in a log file that is
>   effectively write-only.
>
>   - The one way to get people read log files is crashing their
>     application.  A command line option --future could make QEMU crash
>     right after printing a deprecation message.  This could help with
>     finding use of deprecated features in a testing environment.
>
>   - A less destructive way to grab people's attention is to make things
>     run really, really slow: have QEMU go to sleep for a while after
>     printing a deprecation message.
>
>   - We can also pass the buck to the next layer up: emit a QMP event.
>
>     Sadly, by the time the next layer connects to QMP, plenty of stuff
>     already happened.  We'd have to buffer deprecation events somehow.
>
>     What would libvirt do with such an event?  Log it, taint the domain,
>     emit a (libvirt) event to pass it on to the next layer up.
>
>   - A completely different idea is to have a configuratin linter.  To
>     support doing this at the libvirt level, QEMU could expose "is
>     deprecated" in interface introspection.  Feels feasible for QMP,
>     where we already have sufficiently expressive introspection.  For
>     CLI, we'd first have to provide that (but we want that anyway).
>
>   - We might also want to dispay deprecation messages in QEMU's GUI
>     somehow, or on serial consoles.

Sorry for catching up late, this mail thread happened during my PTO.

I remember bringing up at the time [1] that the correct solution needs
to take into account usage models that vary from

- a workstation case, where displaying an error box is easy and
  convenient,

- to local headless VMs where system-level notification would do the job
  better, allowing us to leverage things like system-wide email notifications

- to large-scale collections of VMs managed by some layered product,
  where the correct reporting would be through something like Insights,
  i.e. you don't scan individual logs, you want something like "913 VMs
  are using deprecated X"

To me, that implies that we need to have a clear division of roles, with
a standard way to

a) produce the errors,
b) propagate them,
c) consume them (at least up to libvirt)

Notice that this work has already been done for "real" errors,
i.e. there is a real QAPI notion of "errors". AFAICT, warn_report does
not connect to it, though, it goes through error_vprintf which is really
just basic logging.

So would it make sense to:

1. Add a deprecation_report() alongside warn_report()?

2. Connect warn_report() and all the error_vprintf output to QAPI,
   e.g. using John's suggestion of adding the messages using some
   "warning" or "deprecated" tag?

3. Teach libvirt how to consume that new tag and pass it along?


[1] https://lists.nongnu.org/archive/html/qemu-devel/2018-10/msg06131.html


--
Cheers,
Christophe de Dinechin (IRC c3d)

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

[John Snow]

On 8/29/19 11:59 AM, Christophe de Dinechin wrote:
> 
> John Snow writes:
> [...]
>>
>> 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.
> 
> I like this approach even if there is no consumer today. It does not
> hurt, and it is indeed a motivation to develop consumers that care.
> 
> I personally find this much easier to swallow than any kind of crash on
> deprecation, which already at the BoF seemed like a really big hammer to
> kill a fly.
> 
> CC'ing Andrea as well, because we discussed recently about how to deal
> with error checking in general, and if a new error checking framework is
> being put in place, adding deprecation to the thinking could be a good
> idea.

The most convincing argument against deprecation notices like this is
not that they won't be consumed, but that they are difficult to plumb
through the C infrastructure.

Sadly, I think I have to agree there -- we can't even really model it
like hints, because these are cases where there was no /error/ but
instead a success -- but our error propagation doesn't work on those
terms generally and we'd need a rather extensive audit to allow warnings.

We could always fudge it with a kind of global warning log: clear the
log at the beginning of a QMP interaction and if the log is non-empty
when we return, amend the return with that information.

That's not really the nicest thing to do in a multi-process,
multi-threaded, multi-stacked application, though, so...

--js

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

[John Snow]

On 8/29/19 11:17 AM, Vladimir Sementsov-Ogievskiy wrote:
> Aren't you going to deprecate and drop it at some moment?

Libvirt's going to keep supporting older versions of QEMU in perpetuity.
Apparently, there are still some barriers (SD cards?) to using only
blockdev API for new versions, too:

>> "Note that even with new qemu, if an SD card is used blockdev will be
>> disabled."

It sounds like we need to facilitate libvirt's transfer to an
all-blockdev API for modern QEMU instances. Once we do that, we can
likely add an introspectable feature flag to commands that create
formerly-implicit nodes to tip off libvirt as to what behavior it can
expect.

(In practice, if libvirt sees the new flag, it knows it needs to rely
exclusively on blockdev API and that (likely) it should always provide a
node-name for the filter.)

I think it is reasonably clear that deprecating and re-implementing is
not something that will be compatible with libvirt's feature detection,
so we shouldn't do it.

I'm still not sold that this is worth the effort, but you and Max would
know best right now. I'll leave it to you two to sort out.

--js

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

[John Snow]

On 8/29/19 12:45 PM, Christophe de Dinechin wrote:
> 
> Markus Armbruster writes:
> 
>> Peter Krempa <pkrempa@redhat.com> writes:
>>
> [...]
>>> From my experience users report non-fatal messages mostly only if it is
>>> spamming the system log. One of instances are very unlikely to be
>>> noticed.
>>>
>>> In my experience it's better to notify us in libvirt of such change and
>>> we will try our best to fix it.
>>
>> How to best alert the layers above QEMU was one of the topic of the KVM
>> Forum 2018 BoF on deprecating stuff.  Minutes:
>>
>>     Message-ID: <87mur0ls8o.fsf@dusky.pond.sub.org>
>>     https://lists.nongnu.org/archive/html/qemu-devel/2018-10/msg05828.html
>>
>> Relevant part:
>>
>> * We need to communicate "you're using something that is deprecated".
>>   How?  Right now, we print a deprecation message.  Okay when humans use
>>   QEMU directly in a shell.  However, when QEMU sits at the bottom of a
>>   software stack, the message will likely end up in a log file that is
>>   effectively write-only.
>>
>>   - The one way to get people read log files is crashing their
>>     application.  A command line option --future could make QEMU crash
>>     right after printing a deprecation message.  This could help with
>>     finding use of deprecated features in a testing environment.
>>
>>   - A less destructive way to grab people's attention is to make things
>>     run really, really slow: have QEMU go to sleep for a while after
>>     printing a deprecation message.
>>
>>   - We can also pass the buck to the next layer up: emit a QMP event.
>>
>>     Sadly, by the time the next layer connects to QMP, plenty of stuff
>>     already happened.  We'd have to buffer deprecation events somehow.
>>
>>     What would libvirt do with such an event?  Log it, taint the domain,
>>     emit a (libvirt) event to pass it on to the next layer up.
>>
>>   - A completely different idea is to have a configuratin linter.  To
>>     support doing this at the libvirt level, QEMU could expose "is
>>     deprecated" in interface introspection.  Feels feasible for QMP,
>>     where we already have sufficiently expressive introspection.  For
>>     CLI, we'd first have to provide that (but we want that anyway).
>>
>>   - We might also want to dispay deprecation messages in QEMU's GUI
>>     somehow, or on serial consoles.
> 
> Sorry for catching up late, this mail thread happened during my PTO.
> 
> I remember bringing up at the time [1] that the correct solution needs
> to take into account usage models that vary from
> 
> - a workstation case, where displaying an error box is easy and
>   convenient,
> 
> - to local headless VMs where system-level notification would do the job
>   better, allowing us to leverage things like system-wide email notifications
> 
> - to large-scale collections of VMs managed by some layered product,
>   where the correct reporting would be through something like Insights,
>   i.e. you don't scan individual logs, you want something like "913 VMs
>   are using deprecated X"
> 
> To me, that implies that we need to have a clear division of roles, with
> a standard way to
> 
> a) produce the errors,
> b) propagate them,

I started replying to this thread to the other mail you sent; I think
this is going to be fairly involved. I wouldn't mind being proven wrong
though.

> c) consume them (at least up to libvirt)
> 
> Notice that this work has already been done for "real" errors,
> i.e. there is a real QAPI notion of "errors". AFAICT, warn_report does
> not connect to it, though, it goes through error_vprintf which is really
> just basic logging.
> 
> So would it make sense to:
> 
> 1. Add a deprecation_report() alongside warn_report()?
> 

Where's that get routed to? just an error_vprintf style situation?

> 2. Connect warn_report() and all the error_vprintf output to QAPI,
>    e.g. using John's suggestion of adding the messages using some
>    "warning" or "deprecated" tag?
> 

How do you correlate them?

> 3. Teach libvirt how to consume that new tag and pass it along?
> 

I think it's not libvirt's job to pass it along, exactly -- libvirt made
the decision for which features to engage in QEMU, not the end user.

If the user upgrades QEMU but not libvirt, it's not really anything they
have control over and they shouldn't be pestered with such things.

However, if libvirt accidentally released a version that engages
deprecated behavior (and were unaware of it), it'd be nice to get user
reports, surely?

Logging messages for libvirt might be the best that can be done there in
that case.


In contrast, power user tools like QMP libraries, qmp-shell and others
allow more direct and meaningful access to QMP, so those should report
deprecation messages to the user.

> 
> [1] https://lists.nongnu.org/archive/html/qemu-devel/2018-10/msg06131.html
> 
> 
> --
> Cheers,
> Christophe de Dinechin (IRC c3d)
> 

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

[Christophe de Dinechin]

John Snow writes:

> On 8/29/19 12:45 PM, Christophe de Dinechin wrote:
>>
[...]

>> Sorry for catching up late, this mail thread happened during my PTO.
>>
>> I remember bringing up at the time [1] that the correct solution needs
>> to take into account usage models that vary from
>>
>> - a workstation case, where displaying an error box is easy and
>>   convenient,
>>
>> - to local headless VMs where system-level notification would do the job
>>   better, allowing us to leverage things like system-wide email notifications
>>
>> - to large-scale collections of VMs managed by some layered product,
>>   where the correct reporting would be through something like Insights,
>>   i.e. you don't scan individual logs, you want something like "913 VMs
>>   are using deprecated X"
>>
>> To me, that implies that we need to have a clear division of roles, with
>> a standard way to
>>
>> a) produce the errors,
>> b) propagate them,
>
> I started replying to this thread to the other mail you sent; I think
> this is going to be fairly involved. I wouldn't mind being proven wrong
> though.

Yes, I think it does look involved, but mostly for historical reasons.
In other words, what is complicated is preserving the historical
behaviors so as to not break existing consumers.

>
>> c) consume them (at least up to libvirt)
>>
>> Notice that this work has already been done for "real" errors,
>> i.e. there is a real QAPI notion of "errors". AFAICT, warn_report does
>> not connect to it, though, it goes through error_vprintf which is really
>> just basic logging.
>>
>> So would it make sense to:
>>
>> 1. Add a deprecation_report() alongside warn_report()?
>>
>
> Where's that get routed to? just an error_vprintf style situation?

Yes, but see below.

>
>> 2. Connect warn_report() and all the error_vprintf output to QAPI,
>>    e.g. using John's suggestion of adding the messages using some
>>    "warning" or "deprecated" tag?
>>
>
> How do you correlate them?

Without having looked at the code much, I think I would

1. extend the existing QAPI error to support warnings, deprecations and
   info messages. The first problem I see is that there is no error, so
   we may sometimes need to create one when there was none before. And
   of course make sure that this does not ultimately show as an error,
   but as a success with additional annotations.

2. replace the current "link + if" switching for error_vprintf with some
   actual notification mechanism, with one option routine to
   monitor_vprintf, one to stderr, one to log file, and then an
   additional path that would post a newly minted qapi warning.

>
>> 3. Teach libvirt how to consume that new tag and pass it along?
>>
>
> I think it's not libvirt's job to pass it along, exactly -- libvirt made
> the decision for which features to engage in QEMU, not the end user.

First, by "pass along", I meant to possible layered products or
management software. We don't necessarily need a new virErrorLevel,
deprecation could be a warning with some special domain,
e.g. VIR_FROM_DEPRECATION.

There may be a need to add some API here. Looking at the code, it's not
obvious to me that libvirt has any notion of error priority. In other
words, if you raise an error then a warning, you get the warning as the
last error, right?


Second, why not report the use of deprecated features? I don't fully buy
the rationale that libvirt engages the features, because it does not do
it on its own, it does it because the user made some specific request.
This point of view also seems to require that libvirt or the user should
know ahead of time it's about to engage a deprecated feature. To me, the
problem is precisely that neither libvirt nor the user knows, which is
why we are discussing how to best make it known.

>
> If the user upgrades QEMU but not libvirt, it's not really anything they
> have control over and they shouldn't be pestered with such things.
>
> However, if libvirt accidentally released a version that engages
> deprecated behavior (and were unaware of it), it'd be nice to get user
> reports, surely?
>
> Logging messages for libvirt might be the best that can be done there in
> that case.

I personally would treat that like any warning.

>
>
> In contrast, power user tools like QMP libraries, qmp-shell and others
> allow more direct and meaningful access to QMP, so those should report
> deprecation messages to the user.

Agreed.

>
>>
>> [1] https://lists.nongnu.org/archive/html/qemu-devel/2018-10/msg06131.html
>>

--
Thanks,
Christophe de Dinechin (IRC c3d)

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

[John Snow]

On 8/30/19 6:07 AM, Christophe de Dinechin wrote:
> 
> John Snow writes:
> 
>> On 8/29/19 12:45 PM, Christophe de Dinechin wrote:
>>>
> [...]
> 
>>> Sorry for catching up late, this mail thread happened during my PTO.
>>>
>>> I remember bringing up at the time [1] that the correct solution needs
>>> to take into account usage models that vary from
>>>
>>> - a workstation case, where displaying an error box is easy and
>>>   convenient,
>>>
>>> - to local headless VMs where system-level notification would do the job
>>>   better, allowing us to leverage things like system-wide email notifications
>>>
>>> - to large-scale collections of VMs managed by some layered product,
>>>   where the correct reporting would be through something like Insights,
>>>   i.e. you don't scan individual logs, you want something like "913 VMs
>>>   are using deprecated X"
>>>
>>> To me, that implies that we need to have a clear division of roles, with
>>> a standard way to
>>>
>>> a) produce the errors,
>>> b) propagate them,
>>
>> I started replying to this thread to the other mail you sent; I think
>> this is going to be fairly involved. I wouldn't mind being proven wrong
>> though.
> 
> Yes, I think it does look involved, but mostly for historical reasons.
> In other words, what is complicated is preserving the historical
> behaviors so as to not break existing consumers.
> 
>>
>>> c) consume them (at least up to libvirt)
>>>
>>> Notice that this work has already been done for "real" errors,
>>> i.e. there is a real QAPI notion of "errors". AFAICT, warn_report does
>>> not connect to it, though, it goes through error_vprintf which is really
>>> just basic logging.
>>>
>>> So would it make sense to:
>>>
>>> 1. Add a deprecation_report() alongside warn_report()?
>>>
>>
>> Where's that get routed to? just an error_vprintf style situation?
> 
> Yes, but see below.
> 
>>
>>> 2. Connect warn_report() and all the error_vprintf output to QAPI,
>>>    e.g. using John's suggestion of adding the messages using some
>>>    "warning" or "deprecated" tag?
>>>
>>
>> How do you correlate them?
> 
> Without having looked at the code much, I think I would
> 
> 1. extend the existing QAPI error to support warnings, deprecations and
>    info messages. The first problem I see is that there is no error, so
>    we may sometimes need to create one when there was none before. And
>    of course make sure that this does not ultimately show as an error,
>    but as a success with additional annotations.
> 

I assume this might be a chance to consolidate all of the methodologies
we use for actually checking if there was an error or not. There have
been many and I am sure Markus can give us a history lesson if it's
warranted.

Generally, there's a few paradigms I see a lot:

1. Rely on an error return code being produced by the called function.
The caller trusts that errp was set. This is one of my favorite methods,
because it has the least scaffolding.

2. Pass errp directly to the called function, and check for null after
return. I don't like this method very much, because of confusion with:

3. Create a local error object; check THAT for null, and propagate the
error to the common error object. I think Markus has explained why we
have this code 50 times, and I forget again minutes later.


If we want to expand the concept of the error object into something that
encompasses hints, warnings and deprecations*, checking for null is no
longer appropriate. It might be a good chance to make our error
propagation story more consistent, too.

We could unify with a helper like this, I think, if I'm not forgetting
some crucial usage detail:

subroutine(foo, bar, errp);
if (failure(errp)) {
    error_append_hint(errp, "Lorem ipsum, ...");
    cleanup();
    return;
}

We would then always use this pattern that operates directly on the
caller's errp instead of creating local error objects to allow hints and
warnings to accumulate.



(* I'm not proposing all three in a concrete way, but just referencing
the general class of semantic non-error information we may want to
propagate, however we end up deciding to model it.)

> 2. replace the current "link + if" switching for error_vprintf with some
>    actual notification mechanism, with one option routine to
>    monitor_vprintf, one to stderr, one to log file, and then an
>    additional path that would post a newly minted qapi warning.
> 
>>
>>> 3. Teach libvirt how to consume that new tag and pass it along?
>>>
>>
>> I think it's not libvirt's job to pass it along, exactly -- libvirt made
>> the decision for which features to engage in QEMU, not the end user.
> 
> First, by "pass along", I meant to possible layered products or
> management software. We don't necessarily need a new virErrorLevel,
> deprecation could be a warning with some special domain,
> e.g. VIR_FROM_DEPRECATION.
> 

SHOULD it pass it along? Libvirt knows what QMP is invoking, that should
be opaque to upper layers. a "DEPRECATION" log stream might give the
wrong idea -- that it's not the libvirt feature that's deprecated, but
something in one of the many drivers it has.

I think it's fine to relay this information via generic logging to the
higher levels (so that debugging can be facilitated from that level) but
I don't think it needs special access to these notices because they're
not useful to RHV developers or RHV users.

> There may be a need to add some API here. Looking at the code, it's not
> obvious to me that libvirt has any notion of error priority. In other
> words, if you raise an error then a warning, you get the warning as the
> last error, right?
> 

(Don't know.)

> 
> Second, why not report the use of deprecated features? I don't fully buy
> the rationale that libvirt engages the features, because it does not do
> it on its own, it does it because the user made some specific request.

Because the user didn't request those specific QMP features, they asked
for the VM to start, or to stop, or they asked for a backup, or a snapshot.

On my desktop, I am not really too interested in knowing if XFCE is
using deprecated features of xorg or wayland. I didn't tell it to use
them and I have no real power or control over that. It's nice if I'm a
developer, but as a user, it's noise.

So a development log seems right for these, but not user-visible
interruptions.

> This point of view also seems to require that libvirt or the user should
> know ahead of time it's about to engage a deprecated feature. To me, the
> problem is precisely that neither libvirt nor the user knows, which is
> why we are discussing how to best make it known.

Yeah, sure. I'm still a fan of the end result of having QMP return
messages including that information. I just don't think it makes sense
for libvirt to then relay it to the user or up the stack. I don't think
that's the same as proposing that it hides it, either.

> 
>>
>> If the user upgrades QEMU but not libvirt, it's not really anything they
>> have control over and they shouldn't be pestered with such things.
>>
>> However, if libvirt accidentally released a version that engages
>> deprecated behavior (and were unaware of it), it'd be nice to get user
>> reports, surely?
>>
>> Logging messages for libvirt might be the best that can be done there in
>> that case.
> 
> I personally would treat that like any warning.
> 
>>
>>
>> In contrast, power user tools like QMP libraries, qmp-shell and others
>> allow more direct and meaningful access to QMP, so those should report
>> deprecation messages to the user.
> 
> Agreed.
> 
>>
>>>
>>> [1] https://lists.nongnu.org/archive/html/qemu-devel/2018-10/msg06131.html
>>>
> 
> --
> Thanks,
> Christophe de Dinechin (IRC c3d)
> 

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

[Kevin Wolf]

Am 30.08.2019 um 20:11 hat John Snow geschrieben:
> 
> 
> On 8/30/19 6:07 AM, Christophe de Dinechin wrote:
> > Without having looked at the code much, I think I would
> > 
> > 1. extend the existing QAPI error to support warnings, deprecations and
> >    info messages. The first problem I see is that there is no error, so
> >    we may sometimes need to create one when there was none before. And
> >    of course make sure that this does not ultimately show as an error,
> >    but as a success with additional annotations.
> > 
> 
> I assume this might be a chance to consolidate all of the methodologies
> we use for actually checking if there was an error or not. There have
> been many and I am sure Markus can give us a history lesson if it's
> warranted.
> 
> Generally, there's a few paradigms I see a lot:
> 
> 1. Rely on an error return code being produced by the called function.
> The caller trusts that errp was set. This is one of my favorite methods,
> because it has the least scaffolding.

This one is convenient to use, but the problem is that nobody enforces
that errp is always set if ret < 0, and that it's not set for ret == 0.
So in a way it is error-prone because it allows inconsistencies.

> 2. Pass errp directly to the called function, and check for null after
> return. I don't like this method very much, because of confusion with:

I mainly don't like this very much because it's simply wrong.

Callers can pass errp = NULL if they aren't interested in error
information. If you directly pass errp, you can't check *errp because
errp could be NULL.

So directly passing errp makes the code simpler, but only use it in
functions where you don't intend to check whether an error is set.

> 3. Create a local error object; check THAT for null, and propagate the
> error to the common error object. I think Markus has explained why we
> have this code 50 times, and I forget again minutes later.

local_err exists for those cases where you need to check the error
object before passing it to the caller. (And obviously for those cases
where you don't want to pass it to the caller, but do something like
error_report_err().)

> If we want to expand the concept of the error object into something that
> encompasses hints, warnings and deprecations*, checking for null is no
> longer appropriate. It might be a good chance to make our error
> propagation story more consistent, too.
> 
> We could unify with a helper like this, I think, if I'm not forgetting
> some crucial usage detail:
> 
> subroutine(foo, bar, errp);
> if (failure(errp)) {
>     error_append_hint(errp, "Lorem ipsum, ...");
>     cleanup();
>     return;
> }
> 
> We would then always use this pattern that operates directly on the
> caller's errp instead of creating local error objects to allow hints and
> warnings to accumulate.

There are two parts to the change that you imply:

1. Forbid passing errp = NULL to any function so that *errp can always
   be checked. This gets rid of local_err in the intermediate function,
   but may require the introduction of new local_err variables in
   top-level callers which ignore the error information.

2. Introduce failure(errp) to replace errp != NULL because we want Error
   to contain warnings and notes, too. Currently, it can contain only
   exactly one error, so this would be a major change.

   Note that the change wouldn't make the existing 'if (errp)' checks
   build failures, so getting confident that we found and replaced all
   of them is going to be hard.

Essentially, you'd probably want to replace Error with a new type so
that the compiler will at least be able to tell which places have been
converted and which haven't.

And then, you'd have to touch every single function that does something
with errors. This is a huge change across the whole source tree.

I doubt it's worth the effort.

> > Second, why not report the use of deprecated features? I don't fully buy
> > the rationale that libvirt engages the features, because it does not do
> > it on its own, it does it because the user made some specific request.
> 
> Because the user didn't request those specific QMP features, they asked
> for the VM to start, or to stop, or they asked for a backup, or a snapshot.
> 
> On my desktop, I am not really too interested in knowing if XFCE is
> using deprecated features of xorg or wayland. I didn't tell it to use
> them and I have no real power or control over that. It's nice if I'm a
> developer, but as a user, it's noise.
> 
> So a development log seems right for these, but not user-visible
> interruptions.

I agree, it's not the high-level operation the user requested that is
deprecated, but just the specific implementation libvirt uses to perform
the operation on a QEMU VM.

The expected response to a deprecation notice is that a libvirt update
makes it go away by using more modern interfaces, not that the user
changes their workflow.

Kevin

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

[Kevin Wolf]

Am 23.08.2019 um 11:22 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 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.

A modern client is supposed to be interested in node-names. We basically
expect it to manage the whole block graph at a node level, so it should
certainly make sure to know the node-name of every node. It will need it
to take snapshots, insert filter drivers or anything like this on top of
the implicit node.

So once we make the option mandatory (which means returning an error for
legacy clients that don't do node-level management), I don't see a good
reason to ever make it optional again.

Kevin

Re: [Qemu-devel] Exposing feature deprecation to machine clients

[Philippe Mathieu-Daudé]

Hi Markus,

On 8/15/19 7:40 PM, John Snow wrote:
> On 8/15/19 10:16 AM, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> writes:
[...]
>>> 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@dusky.pond.sub.org>
>>      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.
>>
> 
> Probably the best argument against it. Fire-and-forget avoids the
> problem. Events might work just fine, but the "tie" bit seems like a yak
> in need of a shave.
> 
>> 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.
>>
> 
> That's a good idea too; but the semantics of exactly *what* was
> deprecated may be hard to capture.
> 
>> 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.
>>
> 
> I should clearly remember to talk to you before thinking about QMP in
> public, because you've thought about it much more.

Pre-release period, time to deprecate some stuffs :)

How should we proceed? Do you have something in mind?

There are older threads about this. Should we start a new thread? Gather 
the different ideas on the Wiki?

(Obviously you are not the one responsible of this topic, you just 
happen to be the last one worried about it on the list).

Regards,

Phil.

Re: [Qemu-devel] Exposing feature deprecation to machine clients

[Vladimir Sementsov-Ogievskiy]

07.11.2019 21:52, Philippe Mathieu-Daudé wrote:
> Hi Markus,
> 
> On 8/15/19 7:40 PM, John Snow wrote:
>> On 8/15/19 10:16 AM, Markus Armbruster wrote:
>>> John Snow <jsnow@redhat.com> writes:
> [...]
>>>> 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@dusky.pond.sub.org>
>>>      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.
>>>
>>
>> Probably the best argument against it. Fire-and-forget avoids the
>> problem. Events might work just fine, but the "tie" bit seems like a yak
>> in need of a shave.
>>
>>> 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.
>>>
>>
>> That's a good idea too; but the semantics of exactly *what* was
>> deprecated may be hard to capture.
>>
>>> 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.
>>>
>>
>> I should clearly remember to talk to you before thinking about QMP in
>> public, because you've thought about it much more.
> 
> Pre-release period, time to deprecate some stuffs :)
> 
> How should we proceed? Do you have something in mind?
> 
> There are older threads about this. Should we start a new thread? Gather the different ideas on the Wiki?
> 
> (Obviously you are not the one responsible of this topic, you just happen to be the last one worried about it on the list).
> 
> Regards,
> 
> Phil.

Hi!

I wanted to resend, but faced some problems, and understand that I can't do it in time before soft-freeze..
But you say, that we can deprecate something even after hard-freeze?

Ok, the problem that I faced, is that deprecation warnings breaks some iotests.. What can we do?

1. Update iotests...
   1.1 Just update iotests outputs to show warnings. Then, in next release cycle, update iotests, to not use deprecated things
   or
   1.2 Update iotests to not use deprecated things.. Not appropriate for hard freeze.
or
2. Commit deprecations without warnings.. But how do people find out about this?

Next, what exactly to deprecate? As I understand, we can't deprecate drive-mirror now?
So I propose to:

1. deprecate drive-backup
2. add optional filter-node-name parameter to drive-mirror, to correspond to commit and mirror
3. deprecate that filter-node-name is optional for commit and mirror.

-- 
Best regards,
Vladimir

Deprecating stuff for 4.2 (was: [Qemu-devel] Exposing feature deprecation to machine clients)

[Markus Armbruster]

Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:

> 07.11.2019 21:52, Philippe Mathieu-Daudé wrote:
[...]
>> Pre-release period, time to deprecate some stuffs :)
>> 
>> How should we proceed? Do you have something in mind?
>> 
>> There are older threads about this. Should we start a new thread? Gather the different ideas on the Wiki?
>> 
>> (Obviously you are not the one responsible of this topic, you just happen to be the last one worried about it on the list).
>> 
>> Regards,
>> 
>> Phil.

4.2.0-rc0 has been tagged, i.e. we're in hard freeze already.  Only bug
fixes are accepted during hard freeze.  We've occasionally bent this
rule after -rc0 for borderline cases, e.g. to tweak a new external
interface before the release calcifies it.  Making a case for bending
the rules becomes harder with each -rc.

Ideally, we'd double-check new interfaces for gaffes before a release,
and whether old interfaces that have been replaced now should be
deprecated.  There's rarely time for that, and pretty much never for
releases right after KVM Forum.

So no, I don't have anything in mind for 4.2.

We intend to tag -rc1 next Tuesday.  To make that deadline, we'd need
patches, not just ideas.

> Hi!
>
> I wanted to resend, but faced some problems, and understand that I can't do it in time before soft-freeze..
> But you say, that we can deprecate something even after hard-freeze?

See above.

> Ok, the problem that I faced, is that deprecation warnings breaks some iotests.. What can we do?
>
> 1. Update iotests...
>    1.1 Just update iotests outputs to show warnings. Then, in next release cycle, update iotests, to not use deprecated things

Sounds workable to me, but I'm not the maintainer.

>    or
>    1.2 Update iotests to not use deprecated things.. Not appropriate for hard freeze.

Unnecessarily risky compared to 1.1.

> or
> 2. Commit deprecations without warnings.. But how do people find out about this?

Not nice.

We do it for QMP, but only because we still lack the means to warn
there.

> Next, what exactly to deprecate? As I understand, we can't deprecate drive-mirror now?
> So I propose to:
>
> 1. deprecate drive-backup
> 2. add optional filter-node-name parameter to drive-mirror, to correspond to commit and mirror
> 3. deprecate that filter-node-name is optional for commit and mirror.

To have a chance there, we need patches a.s.a.p.

Re: [Qemu-devel] Exposing feature deprecation to machine clients

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 9084 bytes --]

On 07.11.19 20:13, Vladimir Sementsov-Ogievskiy wrote:
> 07.11.2019 21:52, Philippe Mathieu-Daudé wrote:
>> Hi Markus,
>>
>> On 8/15/19 7:40 PM, John Snow wrote:
>>> On 8/15/19 10:16 AM, Markus Armbruster wrote:
>>>> John Snow <jsnow@redhat.com> writes:
>> [...]
>>>>> 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@dusky.pond.sub.org>
>>>>      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.
>>>>
>>>
>>> Probably the best argument against it. Fire-and-forget avoids the
>>> problem. Events might work just fine, but the "tie" bit seems like a yak
>>> in need of a shave.
>>>
>>>> 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.
>>>>
>>>
>>> That's a good idea too; but the semantics of exactly *what* was
>>> deprecated may be hard to capture.
>>>
>>>> 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.
>>>>
>>>
>>> I should clearly remember to talk to you before thinking about QMP in
>>> public, because you've thought about it much more.
>>
>> Pre-release period, time to deprecate some stuffs :)
>>
>> How should we proceed? Do you have something in mind?
>>
>> There are older threads about this. Should we start a new thread? Gather the different ideas on the Wiki?
>>
>> (Obviously you are not the one responsible of this topic, you just happen to be the last one worried about it on the list).
>>
>> Regards,
>>
>> Phil.
> 
> Hi!
> 
> I wanted to resend, but faced some problems, and understand that I can't do it in time before soft-freeze..
> But you say, that we can deprecate something even after hard-freeze?
> 
> Ok, the problem that I faced, is that deprecation warnings breaks some iotests.. What can we do?
> 
> 1. Update iotests...
>    1.1 Just update iotests outputs to show warnings. Then, in next release cycle, update iotests, to not use deprecated things
>    or
>    1.2 Update iotests to not use deprecated things.. Not appropriate for hard freeze.

I personally don’t have a problem with any test patches during freeze,
but maybe I should be more careful with auto-grouped patches.

> or
> 2. Commit deprecations without warnings.. But how do people find out about this?
> 
> Next, what exactly to deprecate? As I understand, we can't deprecate drive-mirror now?
> So I propose to:
> 
> 1. deprecate drive-backup

I suspect I missed something at KVM Forum, but what’s the hurry here?

Max

> 2. add optional filter-node-name parameter to drive-mirror, to correspond to commit and mirror
> 3. deprecate that filter-node-name is optional for commit and mirror.
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: Deprecating stuff for 4.2

[Vladimir Sementsov-Ogievskiy]

08.11.2019 9:41, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
> 
>> 07.11.2019 21:52, Philippe Mathieu-Daudé wrote:
> [...]
>>> Pre-release period, time to deprecate some stuffs :)
>>>
>>> How should we proceed? Do you have something in mind?
>>>
>>> There are older threads about this. Should we start a new thread? Gather the different ideas on the Wiki?
>>>
>>> (Obviously you are not the one responsible of this topic, you just happen to be the last one worried about it on the list).
>>>
>>> Regards,
>>>
>>> Phil.
> 
> 4.2.0-rc0 has been tagged, i.e. we're in hard freeze already.  Only bug
> fixes are accepted during hard freeze.  We've occasionally bent this
> rule after -rc0 for borderline cases, e.g. to tweak a new external
> interface before the release calcifies it.  Making a case for bending
> the rules becomes harder with each -rc.
> 
> Ideally, we'd double-check new interfaces for gaffes before a release,
> and whether old interfaces that have been replaced now should be
> deprecated.  There's rarely time for that, and pretty much never for
> releases right after KVM Forum.
> 
> So no, I don't have anything in mind for 4.2.
> 
> We intend to tag -rc1 next Tuesday.  To make that deadline, we'd need
> patches, not just ideas.
> 
>> Hi!
>>
>> I wanted to resend, but faced some problems, and understand that I can't do it in time before soft-freeze..
>> But you say, that we can deprecate something even after hard-freeze?
> 
> See above.
> 
>> Ok, the problem that I faced, is that deprecation warnings breaks some iotests.. What can we do?
>>
>> 1. Update iotests...
>>     1.1 Just update iotests outputs to show warnings. Then, in next release cycle, update iotests, to not use deprecated things
> 
> Sounds workable to me, but I'm not the maintainer.
> 
>>     or
>>     1.2 Update iotests to not use deprecated things.. Not appropriate for hard freeze.
> 
> Unnecessarily risky compared to 1.1.
> 
>> or
>> 2. Commit deprecations without warnings.. But how do people find out about this?
> 
> Not nice.
> 
> We do it for QMP, but only because we still lack the means to warn
> there.
> 
>> Next, what exactly to deprecate? As I understand, we can't deprecate drive-mirror now?
>> So I propose to:
>>
>> 1. deprecate drive-backup
>> 2. add optional filter-node-name parameter to drive-mirror, to correspond to commit and mirror
>> 3. deprecate that filter-node-name is optional for commit and mirror.
> 
> To have a chance there, we need patches a.s.a.p.
> 

OK, I'll send today and we'll see, what to do with it.

-- 
Best regards,
Vladimir

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

[Markus Armbruster]

Reviving this old thread, because I'd like to connect it to more recent
discussions.

Christophe de Dinechin <dinechin@redhat.com> writes:

> Markus Armbruster writes:
>
>> Peter Krempa <pkrempa@redhat.com> writes:
>>
> [...]
>>> From my experience users report non-fatal messages mostly only if it is
>>> spamming the system log. One of instances are very unlikely to be
>>> noticed.
>>>
>>> In my experience it's better to notify us in libvirt of such change and
>>> we will try our best to fix it.
>>
>> How to best alert the layers above QEMU was one of the topic of the KVM
>> Forum 2018 BoF on deprecating stuff.  Minutes:
>>
>>     Message-ID: <87mur0ls8o.fsf@dusky.pond.sub.org>
>>     https://lists.nongnu.org/archive/html/qemu-devel/2018-10/msg05828.html
>>
>> Relevant part:
>>
>> * We need to communicate "you're using something that is deprecated".
>>   How?  Right now, we print a deprecation message.  Okay when humans use
>>   QEMU directly in a shell.  However, when QEMU sits at the bottom of a
>>   software stack, the message will likely end up in a log file that is
>>   effectively write-only.
>>
>>   - The one way to get people read log files is crashing their
>>     application.  A command line option --future could make QEMU crash
>>     right after printing a deprecation message.  This could help with
>>     finding use of deprecated features in a testing environment.
>>
>>   - A less destructive way to grab people's attention is to make things
>>     run really, really slow: have QEMU go to sleep for a while after
>>     printing a deprecation message.
>>
>>   - We can also pass the buck to the next layer up: emit a QMP event.
>>
>>     Sadly, by the time the next layer connects to QMP, plenty of stuff
>>     already happened.  We'd have to buffer deprecation events somehow.
>>
>>     What would libvirt do with such an event?  Log it, taint the domain,
>>     emit a (libvirt) event to pass it on to the next layer up.
>>
>>   - A completely different idea is to have a configuratin linter.  To
>>     support doing this at the libvirt level, QEMU could expose "is
>>     deprecated" in interface introspection.  Feels feasible for QMP,
>>     where we already have sufficiently expressive introspection.  For
>>     CLI, we'd first have to provide that (but we want that anyway).
>>
>>   - We might also want to dispay deprecation messages in QEMU's GUI
>>     somehow, or on serial consoles.
>
> Sorry for catching up late, this mail thread happened during my PTO.
>
> I remember bringing up at the time [1] that the correct solution needs
> to take into account usage models that vary from
>
> - a workstation case, where displaying an error box is easy and
>   convenient,
>
> - to local headless VMs where system-level notification would do the job
>   better, allowing us to leverage things like system-wide email notifications
>
> - to large-scale collections of VMs managed by some layered product,
>   where the correct reporting would be through something like Insights,
>   i.e. you don't scan individual logs, you want something like "913 VMs
>   are using deprecated X"
>
> To me, that implies that we need to have a clear division of roles, with
> a standard way to
>
> a) produce the errors,
> b) propagate them,
> c) consume them (at least up to libvirt)
>
> Notice that this work has already been done for "real" errors,
> i.e. there is a real QAPI notion of "errors". AFAICT, warn_report does
> not connect to it, though, it goes through error_vprintf which is really
> just basic logging.
>
> So would it make sense to:
>
> 1. Add a deprecation_report() alongside warn_report()?

"Grepability" alone would make that worthwhile, I think.

> 2. Connect warn_report() and all the error_vprintf output to QAPI,
>    e.g. using John's suggestion of adding the messages using some
>    "warning" or "deprecated" tag?

This is the difficult part.  See my "Exposing feature deprecation to
machine clients (was: [Qemu-devel] [PATCH 2/2] qapi: deprecate implicit
filters)" in this thread.  Quote:

    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.

I've since explored this idea in

    [RFC PATCH 00/19] Configurable policy for handling deprecated interfaces
    Message-Id: <20191024123458.13505-1-armbru@redhat.com>
    https://lists.gnu.org/archive/html/qemu-devel/2019-10/msg06534.html    

> 3. Teach libvirt how to consume that new tag and pass it along?

Pertinent:
Message-ID: <20191024140146.GC8381@redhat.com>
https://lists.gnu.org/archive/html/qemu-devel/2019-10/msg06604.html

Quoting Dan's conclusion:

    In summary, it is probably reasonable to include this info in the QMP
    command reply, but don't expect much to be done with it beyond possibly
    writing it to a log file.

> [1] https://lists.nongnu.org/archive/html/qemu-devel/2018-10/msg06131.html
>
>
> --
> Cheers,
> Christophe de Dinechin (IRC c3d)

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

[Christophe de Dinechin]

[-- Attachment #1: Type: text/plain, Size: 10270 bytes --]



> On 22 Nov 2019, at 09:41, Markus Armbruster <armbru@redhat.com> wrote:
> 
> Reviving this old thread, because I'd like to connect it to more recent
> discussions.
> 
> Christophe de Dinechin <dinechin@redhat.com <mailto:dinechin@redhat.com>> writes:
> 
>> Markus Armbruster writes:
>> 
>>> Peter Krempa <pkrempa@redhat.com> writes:
>>> 
>> [...]
>>>> From my experience users report non-fatal messages mostly only if it is
>>>> spamming the system log. One of instances are very unlikely to be
>>>> noticed.
>>>> 
>>>> In my experience it's better to notify us in libvirt of such change and
>>>> we will try our best to fix it.
>>> 
>>> How to best alert the layers above QEMU was one of the topic of the KVM
>>> Forum 2018 BoF on deprecating stuff.  Minutes:
>>> 
>>>    Message-ID: <87mur0ls8o.fsf@dusky.pond.sub.org>
>>>    https://lists.nongnu.org/archive/html/qemu-devel/2018-10/msg05828.html
>>> 
>>> Relevant part:
>>> 
>>> * We need to communicate "you're using something that is deprecated".
>>>  How?  Right now, we print a deprecation message.  Okay when humans use
>>>  QEMU directly in a shell.  However, when QEMU sits at the bottom of a
>>>  software stack, the message will likely end up in a log file that is
>>>  effectively write-only.
>>> 
>>>  - The one way to get people read log files is crashing their
>>>    application.  A command line option --future could make QEMU crash
>>>    right after printing a deprecation message.  This could help with
>>>    finding use of deprecated features in a testing environment.
>>> 
>>>  - A less destructive way to grab people's attention is to make things
>>>    run really, really slow: have QEMU go to sleep for a while after
>>>    printing a deprecation message.
>>> 
>>>  - We can also pass the buck to the next layer up: emit a QMP event.
>>> 
>>>    Sadly, by the time the next layer connects to QMP, plenty of stuff
>>>    already happened.  We'd have to buffer deprecation events somehow.
>>> 
>>>    What would libvirt do with such an event?  Log it, taint the domain,
>>>    emit a (libvirt) event to pass it on to the next layer up.
>>> 
>>>  - A completely different idea is to have a configuratin linter.  To
>>>    support doing this at the libvirt level, QEMU could expose "is
>>>    deprecated" in interface introspection.  Feels feasible for QMP,
>>>    where we already have sufficiently expressive introspection.  For
>>>    CLI, we'd first have to provide that (but we want that anyway).
>>> 
>>>  - We might also want to dispay deprecation messages in QEMU's GUI
>>>    somehow, or on serial consoles.
>> 
>> Sorry for catching up late, this mail thread happened during my PTO.
>> 
>> I remember bringing up at the time [1] that the correct solution needs
>> to take into account usage models that vary from
>> 
>> - a workstation case, where displaying an error box is easy and
>>  convenient,
>> 
>> - to local headless VMs where system-level notification would do the job
>>  better, allowing us to leverage things like system-wide email notifications
>> 
>> - to large-scale collections of VMs managed by some layered product,
>>  where the correct reporting would be through something like Insights,
>>  i.e. you don't scan individual logs, you want something like "913 VMs
>>  are using deprecated X"
>> 
>> To me, that implies that we need to have a clear division of roles, with
>> a standard way to
>> 
>> a) produce the errors,
>> b) propagate them,
>> c) consume them (at least up to libvirt)
>> 
>> Notice that this work has already been done for "real" errors,
>> i.e. there is a real QAPI notion of "errors". AFAICT, warn_report does
>> not connect to it, though, it goes through error_vprintf which is really
>> just basic logging.
>> 
>> So would it make sense to:
>> 
>> 1. Add a deprecation_report() alongside warn_report()?
> 
> "Grepability" alone would make that worthwhile, I think.
> 
>> 2. Connect warn_report() and all the error_vprintf output to QAPI,
>>   e.g. using John's suggestion of adding the messages using some
>>   "warning" or "deprecated" tag?
> 
> This is the difficult part.  See my "Exposing feature deprecation to
> machine clients (was: [Qemu-devel] [PATCH 2/2] qapi: deprecate implicit
> filters)" in this thread.  Quote:
> 
>    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.

This looks swampy the way you describe it, but this make me think that
the recorder library may provide a somewhat elegant solution to this.

First, my presupposition here is that part of the issues you are describing
is because there is no “one size fits all” reporting of errors, warnings, etc.
Essentially, it’s a policy that is defined outside of libvirt or qemu, and not
all consumers will want the same policy.

The way the recorder library addresses that is to have as many recorders
as you need, and allowing the behavior for each recorder to be configured
individually at run-time, e.g. through env variables.

If you couple that with a naming convention for the recorders, you can
set the policy externally in a rather flexible way. For example, I will typically
have recorders that end with _error or _warning or _info or _verbose.
We could have a _deprecated convention just the same.

With a simple syntax, you can state that you want tracing on
all warnings and errors by activating a regexp like “.*_(warning|error)”,
or disable anything related to deprecation with “.*_deprecated.*=0”.
It wouldn’t be much of a stretch to have similar policies for “create a
qapi error object” or even “abort the process”.

This approach means that we would get to define the default policy
for the various deprecation cases, but that the user would ultimately
be able to override, maybe even individually, e.g.:

	// I don’t care about deprecation at all
	.*_deprecated=0

	// Log all deprecation messages, do nothing else with it
	.*_deprecated=<log>

	// Log most deprecation, but ignore “blink_tag” deprecation
	// messages and abort on “serial_uart” deprecation message
	.*_deprecated=<log>
	blink_tag_deprecated=0
	serial_uart_deprecated=<abort>


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

The recorder above (as it exists today) only deals with logging and reporting
messages. It provides no help whatsoever for introspection, which I assume
you would use to warn “ahead of time” that something is deprecated, right?

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

For these cases, we can still fallback to C code, no? As long as the
“report_deprecated()” being used is somewhat consistent between
the two, the fact that there is a “short, common case” for optiions
and a “complicated, less common case” would be invisible to users.

> 
>    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.
> 
> I've since explored this idea in
> 
>    [RFC PATCH 00/19] Configurable policy for handling deprecated interfaces
>    Message-Id: <20191024123458.13505-1-armbru@redhat.com <mailto:20191024123458.13505-1-armbru@redhat.com>>
>    https://lists.gnu.org/archive/html/qemu-devel/2019-10/msg06534.html <https://lists.gnu.org/archive/html/qemu-devel/2019-10/msg06534.html>    

I had not paid attention to this one. I’ll take a look and get back to you.



> 
>> 3. Teach libvirt how to consume that new tag and pass it along?
> 
> Pertinent:
> Message-ID: <20191024140146.GC8381@redhat.com <mailto:20191024140146.GC8381@redhat.com>>
> https://lists.gnu.org/archive/html/qemu-devel/2019-10/msg06604.html <https://lists.gnu.org/archive/html/qemu-devel/2019-10/msg06604.html>
> 
> Quoting Dan's conclusion:
> 
>    In summary, it is probably reasonable to include this info in the QMP
>    command reply, but don't expect much to be done with it beyond possibly
>    writing it to a log file.
> 
>> [1] https://lists.nongnu.org/archive/html/qemu-devel/2018-10/msg06131.html <https://lists.nongnu.org/archive/html/qemu-devel/2018-10/msg06131.html>


Thanks
Christophe

>> 
>> 
>> --
>> Cheers,
>> Christophe de Dinechin (IRC c3d)


[-- Attachment #2: Type: text/html, Size: 65971 bytes --]