[PATCH 0/3] Migration deadcode removal

[PATCH 0/3] Migration deadcode removal

[dave]

From: "Dr. David Alan Gilbert" <dave@treblig.org>

Hi,
  This is a set of deadcode removal around migration
found by looking for unused symbols.

  Note this does remove the old zero-blocks
capability, but it's been meaningless anyway since block
migration went.

Dave

Dr. David Alan Gilbert (3):
  migration: Remove migrate_cap_set
  migration: Remove unused zero-blocks capability
  migration: Remove unused socket_send_channel_create_sync

 migration/options.c | 28 ----------------------------
 migration/options.h |  2 --
 migration/socket.c  | 18 ------------------
 migration/socket.h  |  1 -
 qapi/migration.json | 10 +---------
 5 files changed, 1 insertion(+), 58 deletions(-)

-- 
2.46.0

[PATCH 1/3] migration: Remove migrate_cap_set

[dave]

From: "Dr. David Alan Gilbert" <dave@treblig.org>

migrate_cap_set has been unused since
  18d154f575 ("migration: Remove 'blk/-b' option from migrate commands")

Remove it.

Signed-off-by: Dr. David Alan Gilbert <dave@treblig.org>
---
 migration/options.c | 20 --------------------
 migration/options.h |  1 -
 2 files changed, 21 deletions(-)

diff --git a/migration/options.c b/migration/options.c
index 147cd2b8fd..9460c5dee9 100644
--- a/migration/options.c
+++ b/migration/options.c
@@ -605,26 +605,6 @@ bool migrate_caps_check(bool *old_caps, bool *new_caps, Error **errp)
     return true;
 }
 
-bool migrate_cap_set(int cap, bool value, Error **errp)
-{
-    MigrationState *s = migrate_get_current();
-    bool new_caps[MIGRATION_CAPABILITY__MAX];
-
-    if (migration_is_running()) {
-        error_setg(errp, "There's a migration process in progress");
-        return false;
-    }
-
-    memcpy(new_caps, s->capabilities, sizeof(new_caps));
-    new_caps[cap] = value;
-
-    if (!migrate_caps_check(s->capabilities, new_caps, errp)) {
-        return false;
-    }
-    s->capabilities[cap] = value;
-    return true;
-}
-
 MigrationCapabilityStatusList *qmp_query_migrate_capabilities(Error **errp)
 {
     MigrationCapabilityStatusList *head = NULL, **tail = &head;
diff --git a/migration/options.h b/migration/options.h
index a0bd6edc06..36e7b3723f 100644
--- a/migration/options.h
+++ b/migration/options.h
@@ -58,7 +58,6 @@ bool migrate_tls(void);
 /* capabilities helpers */
 
 bool migrate_caps_check(bool *old_caps, bool *new_caps, Error **errp);
-bool migrate_cap_set(int cap, bool value, Error **errp);
 
 /* parameters */
 
-- 
2.46.0

[PATCH 2/3] migration: Remove unused zero-blocks capability

[dave]

From: "Dr. David Alan Gilbert" <dave@treblig.org>

migrate_zero_blocks is unused since
  eef0bae3a7 ("migration: Remove block migration")

Remove it.
That whole zero-blocks capability was just for old-school
block migration anyway.

Remove the capability as well.

Signed-off-by: Dr. David Alan Gilbert <dave@treblig.org>
---
 migration/options.c |  8 --------
 migration/options.h |  1 -
 qapi/migration.json | 10 +---------
 3 files changed, 1 insertion(+), 18 deletions(-)

diff --git a/migration/options.c b/migration/options.c
index 9460c5dee9..997e060612 100644
--- a/migration/options.c
+++ b/migration/options.c
@@ -177,7 +177,6 @@ Property migration_properties[] = {
     DEFINE_PROP_MIG_CAP("x-xbzrle", MIGRATION_CAPABILITY_XBZRLE),
     DEFINE_PROP_MIG_CAP("x-rdma-pin-all", MIGRATION_CAPABILITY_RDMA_PIN_ALL),
     DEFINE_PROP_MIG_CAP("x-auto-converge", MIGRATION_CAPABILITY_AUTO_CONVERGE),
-    DEFINE_PROP_MIG_CAP("x-zero-blocks", MIGRATION_CAPABILITY_ZERO_BLOCKS),
     DEFINE_PROP_MIG_CAP("x-events", MIGRATION_CAPABILITY_EVENTS),
     DEFINE_PROP_MIG_CAP("x-postcopy-ram", MIGRATION_CAPABILITY_POSTCOPY_RAM),
     DEFINE_PROP_MIG_CAP("x-postcopy-preempt",
@@ -339,13 +338,6 @@ bool migrate_xbzrle(void)
     return s->capabilities[MIGRATION_CAPABILITY_XBZRLE];
 }
 
-bool migrate_zero_blocks(void)
-{
-    MigrationState *s = migrate_get_current();
-
-    return s->capabilities[MIGRATION_CAPABILITY_ZERO_BLOCKS];
-}
-
 bool migrate_zero_copy_send(void)
 {
     MigrationState *s = migrate_get_current();
diff --git a/migration/options.h b/migration/options.h
index 36e7b3723f..79084eed0d 100644
--- a/migration/options.h
+++ b/migration/options.h
@@ -40,7 +40,6 @@ bool migrate_release_ram(void);
 bool migrate_return_path(void);
 bool migrate_validate_uuid(void);
 bool migrate_xbzrle(void);
-bool migrate_zero_blocks(void);
 bool migrate_zero_copy_send(void);
 
 /*
diff --git a/qapi/migration.json b/qapi/migration.json
index b66cccf107..82d0fc962e 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -389,13 +389,6 @@
 #     footprint is mlock()'d on demand or all at once.  Refer to
 #     docs/rdma.txt for usage.  Disabled by default.  (since 2.0)
 #
-# @zero-blocks: During storage migration encode blocks of zeroes
-#     efficiently.  This essentially saves 1MB of zeroes per block on
-#     the wire.  Enabling requires source and target VM to support
-#     this feature.  To enable it is sufficient to enable the
-#     capability on the source VM.  The feature is disabled by
-#     default.  (since 1.6)
-#
 # @events: generate events for each migration state change (since 2.4)
 #
 # @auto-converge: If enabled, QEMU will automatically throttle down
@@ -483,7 +476,7 @@
 # Since: 1.2
 ##
 { 'enum': 'MigrationCapability',
-  'data': ['xbzrle', 'rdma-pin-all', 'auto-converge', 'zero-blocks',
+  'data': ['xbzrle', 'rdma-pin-all', 'auto-converge',
            'events', 'postcopy-ram',
            { 'name': 'x-colo', 'features': [ 'unstable' ] },
            'release-ram',
@@ -542,7 +535,6 @@
 #           {"state": false, "capability": "xbzrle"},
 #           {"state": false, "capability": "rdma-pin-all"},
 #           {"state": false, "capability": "auto-converge"},
-#           {"state": false, "capability": "zero-blocks"},
 #           {"state": true, "capability": "events"},
 #           {"state": false, "capability": "postcopy-ram"},
 #           {"state": false, "capability": "x-colo"}
-- 
2.46.0

[PATCH 3/3] migration: Remove unused socket_send_channel_create_sync

[dave]

From: "Dr. David Alan Gilbert" <dave@treblig.org>

socket_send_channel_create_sync only use was removed by
  d0edb8a173 ("migration: Create the postcopy preempt channel asynchronously")

Remove it.

Signed-off-by: Dr. David Alan Gilbert <dave@treblig.org>
---
 migration/socket.c | 18 ------------------
 migration/socket.h |  1 -
 2 files changed, 19 deletions(-)

diff --git a/migration/socket.c b/migration/socket.c
index 9ab89b1e08..5ec65b8c03 100644
--- a/migration/socket.c
+++ b/migration/socket.c
@@ -42,24 +42,6 @@ void socket_send_channel_create(QIOTaskFunc f, void *data)
                                      f, data, NULL, NULL);
 }
 
-QIOChannel *socket_send_channel_create_sync(Error **errp)
-{
-    QIOChannelSocket *sioc = qio_channel_socket_new();
-
-    if (!outgoing_args.saddr) {
-        object_unref(OBJECT(sioc));
-        error_setg(errp, "Initial sock address not set!");
-        return NULL;
-    }
-
-    if (qio_channel_socket_connect_sync(sioc, outgoing_args.saddr, errp) < 0) {
-        object_unref(OBJECT(sioc));
-        return NULL;
-    }
-
-    return QIO_CHANNEL(sioc);
-}
-
 struct SocketConnectData {
     MigrationState *s;
     char *hostname;
diff --git a/migration/socket.h b/migration/socket.h
index 46c233ecd2..04ebbe95a1 100644
--- a/migration/socket.h
+++ b/migration/socket.h
@@ -22,7 +22,6 @@
 #include "qemu/sockets.h"
 
 void socket_send_channel_create(QIOTaskFunc f, void *data);
-QIOChannel *socket_send_channel_create_sync(Error **errp);
 
 void socket_start_incoming_migration(SocketAddress *saddr, Error **errp);
 
-- 
2.46.0

Re: [PATCH 2/3] migration: Remove unused zero-blocks capability

[Markus Armbruster]

dave@treblig.org writes:

> From: "Dr. David Alan Gilbert" <dave@treblig.org>
>
> migrate_zero_blocks is unused since
>   eef0bae3a7 ("migration: Remove block migration")
>
> Remove it.
> That whole zero-blocks capability was just for old-school
> block migration anyway.
>
> Remove the capability as well.
>
> Signed-off-by: Dr. David Alan Gilbert <dave@treblig.org>
> ---
>  migration/options.c |  8 --------
>  migration/options.h |  1 -
>  qapi/migration.json | 10 +---------
>  3 files changed, 1 insertion(+), 18 deletions(-)
>
> diff --git a/migration/options.c b/migration/options.c
> index 9460c5dee9..997e060612 100644
> --- a/migration/options.c
> +++ b/migration/options.c
> @@ -177,7 +177,6 @@ Property migration_properties[] = {
>      DEFINE_PROP_MIG_CAP("x-xbzrle", MIGRATION_CAPABILITY_XBZRLE),
>      DEFINE_PROP_MIG_CAP("x-rdma-pin-all", MIGRATION_CAPABILITY_RDMA_PIN_ALL),
>      DEFINE_PROP_MIG_CAP("x-auto-converge", MIGRATION_CAPABILITY_AUTO_CONVERGE),
> -    DEFINE_PROP_MIG_CAP("x-zero-blocks", MIGRATION_CAPABILITY_ZERO_BLOCKS),
>      DEFINE_PROP_MIG_CAP("x-events", MIGRATION_CAPABILITY_EVENTS),
>      DEFINE_PROP_MIG_CAP("x-postcopy-ram", MIGRATION_CAPABILITY_POSTCOPY_RAM),
>      DEFINE_PROP_MIG_CAP("x-postcopy-preempt",

Property of (pseudo-)device "migration".  The "x-" prefix suggests we
expect management software not to rely on it.  Okay.

[...]

> diff --git a/qapi/migration.json b/qapi/migration.json
> index b66cccf107..82d0fc962e 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -389,13 +389,6 @@
>  #     footprint is mlock()'d on demand or all at once.  Refer to
>  #     docs/rdma.txt for usage.  Disabled by default.  (since 2.0)
>  #
> -# @zero-blocks: During storage migration encode blocks of zeroes
> -#     efficiently.  This essentially saves 1MB of zeroes per block on
> -#     the wire.  Enabling requires source and target VM to support
> -#     this feature.  To enable it is sufficient to enable the
> -#     capability on the source VM.  The feature is disabled by
> -#     default.  (since 1.6)
> -#
>  # @events: generate events for each migration state change (since 2.4)
>  #
>  # @auto-converge: If enabled, QEMU will automatically throttle down
> @@ -483,7 +476,7 @@
>  # Since: 1.2
>  ##
>  { 'enum': 'MigrationCapability',
> -  'data': ['xbzrle', 'rdma-pin-all', 'auto-converge', 'zero-blocks',
> +  'data': ['xbzrle', 'rdma-pin-all', 'auto-converge',
>             'events', 'postcopy-ram',
>             { 'name': 'x-colo', 'features': [ 'unstable' ] },
>             'release-ram',

This is used by migrate-set-capabilities and query-migrate-capabilities,
via ['MigrationCapabilityStatus'].

query-migrate-capabilities is unaffected: it couldn't return zero-blocks
anymore even before the patch.

migrate-set-capabilities changes incompatibly, I'm afraid.  Before the
patch:

    {"execute": "migrate-set-capabilities", "arguments": {"capabilities": [{"capability": "zero-blocks", "state": true}]}}
    {"return": {}}

Afterwards:

    {"error": {"class": "GenericError", "desc": "Parameter 'capability' does not accept value 'zero-blocks'"}}

If we had somehow rejected the capability when it made no sense,
removing it now it never makes sense would be obviously fine.

The straight & narrow path is to deprecate now, remove later.

If we believe nothing relies on it, we can bend the rules and remove
right away.  Missing then: update to docs/about/removed-features.rst.

> @@ -542,7 +535,6 @@
>  #           {"state": false, "capability": "xbzrle"},
>  #           {"state": false, "capability": "rdma-pin-all"},
>  #           {"state": false, "capability": "auto-converge"},
> -#           {"state": false, "capability": "zero-blocks"},
>  #           {"state": true, "capability": "events"},
>  #           {"state": false, "capability": "postcopy-ram"},
>  #           {"state": false, "capability": "x-colo"}

Example for query-migrate-capabilities.  Good.

Re: [PATCH 2/3] migration: Remove unused zero-blocks capability

[Peter Xu]

On Wed, Sep 18, 2024 at 07:52:56AM +0200, Markus Armbruster wrote:
> dave@treblig.org writes:
> 
> > From: "Dr. David Alan Gilbert" <dave@treblig.org>
> >
> > migrate_zero_blocks is unused since
> >   eef0bae3a7 ("migration: Remove block migration")
> >
> > Remove it.
> > That whole zero-blocks capability was just for old-school
> > block migration anyway.
> >
> > Remove the capability as well.
> >
> > Signed-off-by: Dr. David Alan Gilbert <dave@treblig.org>
> > ---
> >  migration/options.c |  8 --------
> >  migration/options.h |  1 -
> >  qapi/migration.json | 10 +---------
> >  3 files changed, 1 insertion(+), 18 deletions(-)
> >
> > diff --git a/migration/options.c b/migration/options.c
> > index 9460c5dee9..997e060612 100644
> > --- a/migration/options.c
> > +++ b/migration/options.c
> > @@ -177,7 +177,6 @@ Property migration_properties[] = {
> >      DEFINE_PROP_MIG_CAP("x-xbzrle", MIGRATION_CAPABILITY_XBZRLE),
> >      DEFINE_PROP_MIG_CAP("x-rdma-pin-all", MIGRATION_CAPABILITY_RDMA_PIN_ALL),
> >      DEFINE_PROP_MIG_CAP("x-auto-converge", MIGRATION_CAPABILITY_AUTO_CONVERGE),
> > -    DEFINE_PROP_MIG_CAP("x-zero-blocks", MIGRATION_CAPABILITY_ZERO_BLOCKS),
> >      DEFINE_PROP_MIG_CAP("x-events", MIGRATION_CAPABILITY_EVENTS),
> >      DEFINE_PROP_MIG_CAP("x-postcopy-ram", MIGRATION_CAPABILITY_POSTCOPY_RAM),
> >      DEFINE_PROP_MIG_CAP("x-postcopy-preempt",
> 
> Property of (pseudo-)device "migration".  The "x-" prefix suggests we
> expect management software not to rely on it.  Okay.
> 
> [...]
> 
> > diff --git a/qapi/migration.json b/qapi/migration.json
> > index b66cccf107..82d0fc962e 100644
> > --- a/qapi/migration.json
> > +++ b/qapi/migration.json
> > @@ -389,13 +389,6 @@
> >  #     footprint is mlock()'d on demand or all at once.  Refer to
> >  #     docs/rdma.txt for usage.  Disabled by default.  (since 2.0)
> >  #
> > -# @zero-blocks: During storage migration encode blocks of zeroes
> > -#     efficiently.  This essentially saves 1MB of zeroes per block on
> > -#     the wire.  Enabling requires source and target VM to support
> > -#     this feature.  To enable it is sufficient to enable the
> > -#     capability on the source VM.  The feature is disabled by
> > -#     default.  (since 1.6)
> > -#
> >  # @events: generate events for each migration state change (since 2.4)
> >  #
> >  # @auto-converge: If enabled, QEMU will automatically throttle down
> > @@ -483,7 +476,7 @@
> >  # Since: 1.2
> >  ##
> >  { 'enum': 'MigrationCapability',
> > -  'data': ['xbzrle', 'rdma-pin-all', 'auto-converge', 'zero-blocks',
> > +  'data': ['xbzrle', 'rdma-pin-all', 'auto-converge',
> >             'events', 'postcopy-ram',
> >             { 'name': 'x-colo', 'features': [ 'unstable' ] },
> >             'release-ram',
> 
> This is used by migrate-set-capabilities and query-migrate-capabilities,
> via ['MigrationCapabilityStatus'].
> 
> query-migrate-capabilities is unaffected: it couldn't return zero-blocks
> anymore even before the patch.
> 
> migrate-set-capabilities changes incompatibly, I'm afraid.  Before the
> patch:
> 
>     {"execute": "migrate-set-capabilities", "arguments": {"capabilities": [{"capability": "zero-blocks", "state": true}]}}
>     {"return": {}}
> 
> Afterwards:
> 
>     {"error": {"class": "GenericError", "desc": "Parameter 'capability' does not accept value 'zero-blocks'"}}
> 
> If we had somehow rejected the capability when it made no sense,
> removing it now it never makes sense would be obviously fine.
> 
> The straight & narrow path is to deprecate now, remove later.

I wonder whether we can make this one simpler, as IIUC this cap depends on
the block migration feature, which properly went through the deprecation
process and got removed in the previous release.

IOW, currently QEMU behaves the same with this cap on/off, ignoring it
completely.  I think it means the deprecation message (even if we provide
some for two extra releases..) wouldn't be anything helpful as anyone who
uses this feature already got affected before this patch.. this feature,
together with block migration, are simply all gone already?

> 
> If we believe nothing relies on it, we can bend the rules and remove
> right away.  Missing then: update to docs/about/removed-features.rst.

Indeed, we could mention there.

Thanks,

> 
> > @@ -542,7 +535,6 @@
> >  #           {"state": false, "capability": "xbzrle"},
> >  #           {"state": false, "capability": "rdma-pin-all"},
> >  #           {"state": false, "capability": "auto-converge"},
> > -#           {"state": false, "capability": "zero-blocks"},
> >  #           {"state": true, "capability": "events"},
> >  #           {"state": false, "capability": "postcopy-ram"},
> >  #           {"state": false, "capability": "x-colo"}
> 
> Example for query-migrate-capabilities.  Good.
> 

-- 
Peter Xu

Re: [PATCH 2/3] migration: Remove unused zero-blocks capability

[Markus Armbruster]

Peter Xu <peterx@redhat.com> writes:

> On Wed, Sep 18, 2024 at 07:52:56AM +0200, Markus Armbruster wrote:
>> dave@treblig.org writes:
>> 
>> > From: "Dr. David Alan Gilbert" <dave@treblig.org>
>> >
>> > migrate_zero_blocks is unused since
>> >   eef0bae3a7 ("migration: Remove block migration")
>> >
>> > Remove it.
>> > That whole zero-blocks capability was just for old-school
>> > block migration anyway.
>> >
>> > Remove the capability as well.
>> >
>> > Signed-off-by: Dr. David Alan Gilbert <dave@treblig.org>
>> > ---
>> >  migration/options.c |  8 --------
>> >  migration/options.h |  1 -
>> >  qapi/migration.json | 10 +---------
>> >  3 files changed, 1 insertion(+), 18 deletions(-)
>> >
>> > diff --git a/migration/options.c b/migration/options.c
>> > index 9460c5dee9..997e060612 100644
>> > --- a/migration/options.c
>> > +++ b/migration/options.c
>> > @@ -177,7 +177,6 @@ Property migration_properties[] = {
>> >      DEFINE_PROP_MIG_CAP("x-xbzrle", MIGRATION_CAPABILITY_XBZRLE),
>> >      DEFINE_PROP_MIG_CAP("x-rdma-pin-all", MIGRATION_CAPABILITY_RDMA_PIN_ALL),
>> >      DEFINE_PROP_MIG_CAP("x-auto-converge", MIGRATION_CAPABILITY_AUTO_CONVERGE),
>> > -    DEFINE_PROP_MIG_CAP("x-zero-blocks", MIGRATION_CAPABILITY_ZERO_BLOCKS),
>> >      DEFINE_PROP_MIG_CAP("x-events", MIGRATION_CAPABILITY_EVENTS),
>> >      DEFINE_PROP_MIG_CAP("x-postcopy-ram", MIGRATION_CAPABILITY_POSTCOPY_RAM),
>> >      DEFINE_PROP_MIG_CAP("x-postcopy-preempt",
>> 
>> Property of (pseudo-)device "migration".  The "x-" prefix suggests we
>> expect management software not to rely on it.  Okay.
>> 
>> [...]
>> 
>> > diff --git a/qapi/migration.json b/qapi/migration.json
>> > index b66cccf107..82d0fc962e 100644
>> > --- a/qapi/migration.json
>> > +++ b/qapi/migration.json
>> > @@ -389,13 +389,6 @@
>> >  #     footprint is mlock()'d on demand or all at once.  Refer to
>> >  #     docs/rdma.txt for usage.  Disabled by default.  (since 2.0)
>> >  #
>> > -# @zero-blocks: During storage migration encode blocks of zeroes
>> > -#     efficiently.  This essentially saves 1MB of zeroes per block on
>> > -#     the wire.  Enabling requires source and target VM to support
>> > -#     this feature.  To enable it is sufficient to enable the
>> > -#     capability on the source VM.  The feature is disabled by
>> > -#     default.  (since 1.6)
>> > -#
>> >  # @events: generate events for each migration state change (since 2.4)
>> >  #
>> >  # @auto-converge: If enabled, QEMU will automatically throttle down
>> > @@ -483,7 +476,7 @@
>> >  # Since: 1.2
>> >  ##
>> >  { 'enum': 'MigrationCapability',
>> > -  'data': ['xbzrle', 'rdma-pin-all', 'auto-converge', 'zero-blocks',
>> > +  'data': ['xbzrle', 'rdma-pin-all', 'auto-converge',
>> >             'events', 'postcopy-ram',
>> >             { 'name': 'x-colo', 'features': [ 'unstable' ] },
>> >             'release-ram',
>> 
>> This is used by migrate-set-capabilities and query-migrate-capabilities,
>> via ['MigrationCapabilityStatus'].
>> 
>> query-migrate-capabilities is unaffected: it couldn't return zero-blocks
>> anymore even before the patch.
>> 
>> migrate-set-capabilities changes incompatibly, I'm afraid.  Before the
>> patch:
>> 
>>     {"execute": "migrate-set-capabilities", "arguments": {"capabilities": [{"capability": "zero-blocks", "state": true}]}}
>>     {"return": {}}
>> 
>> Afterwards:
>> 
>>     {"error": {"class": "GenericError", "desc": "Parameter 'capability' does not accept value 'zero-blocks'"}}
>> 
>> If we had somehow rejected the capability when it made no sense,
>> removing it now it never makes sense would be obviously fine.
>> 
>> The straight & narrow path is to deprecate now, remove later.
>
> I wonder whether we can make this one simpler, as IIUC this cap depends on
> the block migration feature, which properly went through the deprecation
> process and got removed in the previous release.
>
> IOW, currently QEMU behaves the same with this cap on/off, ignoring it
> completely.  I think it means the deprecation message (even if we provide
> some for two extra releases..) wouldn't be anything helpful as anyone who
> uses this feature already got affected before this patch.. this feature,
> together with block migration, are simply all gone already?

We break compatibility for users who supply capability @zero-blocks even
though they are not using block migration.

Before this patch, the capability is silently ignored.

Afterwards, we reject it.

This harmless misuse was *not* affected by our prior removal of block
migration.

It *is* affected by the proposed removal of the capability.

We either treat this in struct accordance to our rules: deprecate now,
remove later.  Or we bend our them:

>> If we believe nothing relies on it, we can bend the rules and remove
>> right away.

Not for me to decide.

>>              Missing then: update to docs/about/removed-features.rst.
>
> Indeed, we could mention there.
>
> Thanks,
>
>> 
>> > @@ -542,7 +535,6 @@
>> >  #           {"state": false, "capability": "xbzrle"},
>> >  #           {"state": false, "capability": "rdma-pin-all"},
>> >  #           {"state": false, "capability": "auto-converge"},
>> > -#           {"state": false, "capability": "zero-blocks"},
>> >  #           {"state": true, "capability": "events"},
>> >  #           {"state": false, "capability": "postcopy-ram"},
>> >  #           {"state": false, "capability": "x-colo"}
>> 
>> Example for query-migrate-capabilities.  Good.

Re: [PATCH 1/3] migration: Remove migrate_cap_set

[Fabiano Rosas]

dave@treblig.org writes:

> From: "Dr. David Alan Gilbert" <dave@treblig.org>
>
> migrate_cap_set has been unused since
>   18d154f575 ("migration: Remove 'blk/-b' option from migrate commands")
>
> Remove it.
>
> Signed-off-by: Dr. David Alan Gilbert <dave@treblig.org>

Reviewed-by: Fabiano Rosas <farosas@suse.de>

Re: [PATCH 3/3] migration: Remove unused socket_send_channel_create_sync

[Fabiano Rosas]

dave@treblig.org writes:

> From: "Dr. David Alan Gilbert" <dave@treblig.org>
>
> socket_send_channel_create_sync only use was removed by
>   d0edb8a173 ("migration: Create the postcopy preempt channel asynchronously")
>
> Remove it.
>
> Signed-off-by: Dr. David Alan Gilbert <dave@treblig.org>

Reviewed-by: Fabiano Rosas <farosas@suse.de>

Re: [PATCH 2/3] migration: Remove unused zero-blocks capability

[Fabiano Rosas]

Markus Armbruster <armbru@redhat.com> writes:

> Peter Xu <peterx@redhat.com> writes:
>
>> On Wed, Sep 18, 2024 at 07:52:56AM +0200, Markus Armbruster wrote:
>>> dave@treblig.org writes:
>>> 
>>> > From: "Dr. David Alan Gilbert" <dave@treblig.org>
>>> >
>>> > migrate_zero_blocks is unused since
>>> >   eef0bae3a7 ("migration: Remove block migration")
>>> >
>>> > Remove it.
>>> > That whole zero-blocks capability was just for old-school
>>> > block migration anyway.
>>> >
>>> > Remove the capability as well.
>>> >
>>> > Signed-off-by: Dr. David Alan Gilbert <dave@treblig.org>
>>> > ---
>>> >  migration/options.c |  8 --------
>>> >  migration/options.h |  1 -
>>> >  qapi/migration.json | 10 +---------
>>> >  3 files changed, 1 insertion(+), 18 deletions(-)
>>> >
>>> > diff --git a/migration/options.c b/migration/options.c
>>> > index 9460c5dee9..997e060612 100644
>>> > --- a/migration/options.c
>>> > +++ b/migration/options.c
>>> > @@ -177,7 +177,6 @@ Property migration_properties[] = {
>>> >      DEFINE_PROP_MIG_CAP("x-xbzrle", MIGRATION_CAPABILITY_XBZRLE),
>>> >      DEFINE_PROP_MIG_CAP("x-rdma-pin-all", MIGRATION_CAPABILITY_RDMA_PIN_ALL),
>>> >      DEFINE_PROP_MIG_CAP("x-auto-converge", MIGRATION_CAPABILITY_AUTO_CONVERGE),
>>> > -    DEFINE_PROP_MIG_CAP("x-zero-blocks", MIGRATION_CAPABILITY_ZERO_BLOCKS),
>>> >      DEFINE_PROP_MIG_CAP("x-events", MIGRATION_CAPABILITY_EVENTS),
>>> >      DEFINE_PROP_MIG_CAP("x-postcopy-ram", MIGRATION_CAPABILITY_POSTCOPY_RAM),
>>> >      DEFINE_PROP_MIG_CAP("x-postcopy-preempt",
>>> 
>>> Property of (pseudo-)device "migration".  The "x-" prefix suggests we
>>> expect management software not to rely on it.  Okay.
>>> 
>>> [...]
>>> 
>>> > diff --git a/qapi/migration.json b/qapi/migration.json
>>> > index b66cccf107..82d0fc962e 100644
>>> > --- a/qapi/migration.json
>>> > +++ b/qapi/migration.json
>>> > @@ -389,13 +389,6 @@
>>> >  #     footprint is mlock()'d on demand or all at once.  Refer to
>>> >  #     docs/rdma.txt for usage.  Disabled by default.  (since 2.0)
>>> >  #
>>> > -# @zero-blocks: During storage migration encode blocks of zeroes
>>> > -#     efficiently.  This essentially saves 1MB of zeroes per block on
>>> > -#     the wire.  Enabling requires source and target VM to support
>>> > -#     this feature.  To enable it is sufficient to enable the
>>> > -#     capability on the source VM.  The feature is disabled by
>>> > -#     default.  (since 1.6)
>>> > -#
>>> >  # @events: generate events for each migration state change (since 2.4)
>>> >  #
>>> >  # @auto-converge: If enabled, QEMU will automatically throttle down
>>> > @@ -483,7 +476,7 @@
>>> >  # Since: 1.2
>>> >  ##
>>> >  { 'enum': 'MigrationCapability',
>>> > -  'data': ['xbzrle', 'rdma-pin-all', 'auto-converge', 'zero-blocks',
>>> > +  'data': ['xbzrle', 'rdma-pin-all', 'auto-converge',
>>> >             'events', 'postcopy-ram',
>>> >             { 'name': 'x-colo', 'features': [ 'unstable' ] },
>>> >             'release-ram',
>>> 
>>> This is used by migrate-set-capabilities and query-migrate-capabilities,
>>> via ['MigrationCapabilityStatus'].
>>> 
>>> query-migrate-capabilities is unaffected: it couldn't return zero-blocks
>>> anymore even before the patch.
>>> 
>>> migrate-set-capabilities changes incompatibly, I'm afraid.  Before the
>>> patch:
>>> 
>>>     {"execute": "migrate-set-capabilities", "arguments": {"capabilities": [{"capability": "zero-blocks", "state": true}]}}
>>>     {"return": {}}
>>> 
>>> Afterwards:
>>> 
>>>     {"error": {"class": "GenericError", "desc": "Parameter 'capability' does not accept value 'zero-blocks'"}}
>>> 
>>> If we had somehow rejected the capability when it made no sense,
>>> removing it now it never makes sense would be obviously fine.
>>> 
>>> The straight & narrow path is to deprecate now, remove later.
>>
>> I wonder whether we can make this one simpler, as IIUC this cap depends on
>> the block migration feature, which properly went through the deprecation
>> process and got removed in the previous release.
>>
>> IOW, currently QEMU behaves the same with this cap on/off, ignoring it
>> completely.  I think it means the deprecation message (even if we provide
>> some for two extra releases..) wouldn't be anything helpful as anyone who
>> uses this feature already got affected before this patch.. this feature,
>> together with block migration, are simply all gone already?
>
> We break compatibility for users who supply capability @zero-blocks even
> though they are not using block migration.
>
> Before this patch, the capability is silently ignored.
>
> Afterwards, we reject it.
>
> This harmless misuse was *not* affected by our prior removal of block
> migration.
>
> It *is* affected by the proposed removal of the capability.

How does this policy_skip thing works? Could we automatically warn
whenever a capability has the 'deprecated' feature in migration.json?

Also, some of the incompatibility errors in migrate_caps_check() could
be simplified with something like a new:
'features': [ 'conflicts': [ 'cap1', 'cap2' ] ]
to indicate which caps are incompatible between themselves.

>
> We either treat this in struct accordance to our rules: deprecate now,
> remove later.  Or we bend our them:
>
>>> If we believe nothing relies on it, we can bend the rules and remove
>>> right away.
>
> Not for me to decide.
>

I'm fine either way, but in any case:

-- >8 --
From 3ff313a52e37b8cb407c900d7a1aa266560aebb7 Mon Sep 17 00:00:00 2001
From: Fabiano Rosas <farosas@suse.de>
Date: Thu, 19 Sep 2024 09:49:44 -0300
Subject: [PATCH] migration: Deprecate zero-blocks capability

The zero-blocks capability was meant to be used along with the block
migration, which has been removed already in commit eef0bae3a7
("migration: Remove block migration").

Setting zero-blocks is currently a noop, but the outright removal of
the capability would cause and error in case some users are still
setting it. Put the capability through the deprecation process.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 docs/about/deprecated.rst | 6 ++++++
 migration/options.c       | 4 ++++
 qapi/migration.json       | 5 ++++-
 3 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
index ed31d4b0b2..47cabb6fcc 100644
--- a/docs/about/deprecated.rst
+++ b/docs/about/deprecated.rst
@@ -476,3 +476,9 @@ usage of providing a file descriptor to a plain file has been
 deprecated in favor of explicitly using the ``file:`` URI with the
 file descriptor being passed as an ``fdset``. Refer to the ``add-fd``
 command documentation for details on the ``fdset`` usage.
+
+``zero-blocks`` capability (since 9.2)
+''''''''''''''''''''''''''''''''''''''
+
+The ``zero-blocks`` capability was part of the block migration which
+doesn't exist anymore since it was removed in QEMU v9.1.
diff --git a/migration/options.c b/migration/options.c
index 147cd2b8fd..b828bad0d9 100644
--- a/migration/options.c
+++ b/migration/options.c
@@ -457,6 +457,10 @@ bool migrate_caps_check(bool *old_caps, bool *new_caps, Error **errp)
     ERRP_GUARD();
     MigrationIncomingState *mis = migration_incoming_get_current();
 
+    if (new_caps[MIGRATION_CAPABILITY_ZERO_BLOCKS]) {
+        warn_report("zero-blocks capability is deprecated");
+    }
+
 #ifndef CONFIG_REPLICATION
     if (new_caps[MIGRATION_CAPABILITY_X_COLO]) {
         error_setg(errp, "QEMU compiled without replication module"
diff --git a/qapi/migration.json b/qapi/migration.json
index b66cccf107..3af6aa1740 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -479,11 +479,14 @@
 # Features:
 #
 # @unstable: Members @x-colo and @x-ignore-shared are experimental.
+# @deprecated: Member @zero-blocks is deprecated as being part of
+#     block migration which was already removed.
 #
 # Since: 1.2
 ##
 { 'enum': 'MigrationCapability',
-  'data': ['xbzrle', 'rdma-pin-all', 'auto-converge', 'zero-blocks',
+  'data': ['xbzrle', 'rdma-pin-all', 'auto-converge',
+           { 'name': 'zero-blocks', 'features': [ 'deprecated' ] },
            'events', 'postcopy-ram',
            { 'name': 'x-colo', 'features': [ 'unstable' ] },
            'release-ram',
-- 
2.35.3

Re: [PATCH 2/3] migration: Remove unused zero-blocks capability

[Dr. David Alan Gilbert]

* Fabiano Rosas (farosas@suse.de) wrote:
> Markus Armbruster <armbru@redhat.com> writes:
> 
> > Peter Xu <peterx@redhat.com> writes:
> >
> >> On Wed, Sep 18, 2024 at 07:52:56AM +0200, Markus Armbruster wrote:
> >>> dave@treblig.org writes:
> >>> 
> >>> > From: "Dr. David Alan Gilbert" <dave@treblig.org>
> >>> >
> >>> > migrate_zero_blocks is unused since
> >>> >   eef0bae3a7 ("migration: Remove block migration")
> >>> >
> >>> > Remove it.
> >>> > That whole zero-blocks capability was just for old-school
> >>> > block migration anyway.
> >>> >
> >>> > Remove the capability as well.
> >>> >
> >>> > Signed-off-by: Dr. David Alan Gilbert <dave@treblig.org>
> >>> > ---
> >>> >  migration/options.c |  8 --------
> >>> >  migration/options.h |  1 -
> >>> >  qapi/migration.json | 10 +---------
> >>> >  3 files changed, 1 insertion(+), 18 deletions(-)
> >>> >
> >>> > diff --git a/migration/options.c b/migration/options.c
> >>> > index 9460c5dee9..997e060612 100644
> >>> > --- a/migration/options.c
> >>> > +++ b/migration/options.c
> >>> > @@ -177,7 +177,6 @@ Property migration_properties[] = {
> >>> >      DEFINE_PROP_MIG_CAP("x-xbzrle", MIGRATION_CAPABILITY_XBZRLE),
> >>> >      DEFINE_PROP_MIG_CAP("x-rdma-pin-all", MIGRATION_CAPABILITY_RDMA_PIN_ALL),
> >>> >      DEFINE_PROP_MIG_CAP("x-auto-converge", MIGRATION_CAPABILITY_AUTO_CONVERGE),
> >>> > -    DEFINE_PROP_MIG_CAP("x-zero-blocks", MIGRATION_CAPABILITY_ZERO_BLOCKS),
> >>> >      DEFINE_PROP_MIG_CAP("x-events", MIGRATION_CAPABILITY_EVENTS),
> >>> >      DEFINE_PROP_MIG_CAP("x-postcopy-ram", MIGRATION_CAPABILITY_POSTCOPY_RAM),
> >>> >      DEFINE_PROP_MIG_CAP("x-postcopy-preempt",
> >>> 
> >>> Property of (pseudo-)device "migration".  The "x-" prefix suggests we
> >>> expect management software not to rely on it.  Okay.
> >>> 
> >>> [...]
> >>> 
> >>> > diff --git a/qapi/migration.json b/qapi/migration.json
> >>> > index b66cccf107..82d0fc962e 100644
> >>> > --- a/qapi/migration.json
> >>> > +++ b/qapi/migration.json
> >>> > @@ -389,13 +389,6 @@
> >>> >  #     footprint is mlock()'d on demand or all at once.  Refer to
> >>> >  #     docs/rdma.txt for usage.  Disabled by default.  (since 2.0)
> >>> >  #
> >>> > -# @zero-blocks: During storage migration encode blocks of zeroes
> >>> > -#     efficiently.  This essentially saves 1MB of zeroes per block on
> >>> > -#     the wire.  Enabling requires source and target VM to support
> >>> > -#     this feature.  To enable it is sufficient to enable the
> >>> > -#     capability on the source VM.  The feature is disabled by
> >>> > -#     default.  (since 1.6)
> >>> > -#
> >>> >  # @events: generate events for each migration state change (since 2.4)
> >>> >  #
> >>> >  # @auto-converge: If enabled, QEMU will automatically throttle down
> >>> > @@ -483,7 +476,7 @@
> >>> >  # Since: 1.2
> >>> >  ##
> >>> >  { 'enum': 'MigrationCapability',
> >>> > -  'data': ['xbzrle', 'rdma-pin-all', 'auto-converge', 'zero-blocks',
> >>> > +  'data': ['xbzrle', 'rdma-pin-all', 'auto-converge',
> >>> >             'events', 'postcopy-ram',
> >>> >             { 'name': 'x-colo', 'features': [ 'unstable' ] },
> >>> >             'release-ram',
> >>> 
> >>> This is used by migrate-set-capabilities and query-migrate-capabilities,
> >>> via ['MigrationCapabilityStatus'].
> >>> 
> >>> query-migrate-capabilities is unaffected: it couldn't return zero-blocks
> >>> anymore even before the patch.
> >>> 
> >>> migrate-set-capabilities changes incompatibly, I'm afraid.  Before the
> >>> patch:
> >>> 
> >>>     {"execute": "migrate-set-capabilities", "arguments": {"capabilities": [{"capability": "zero-blocks", "state": true}]}}
> >>>     {"return": {}}
> >>> 
> >>> Afterwards:
> >>> 
> >>>     {"error": {"class": "GenericError", "desc": "Parameter 'capability' does not accept value 'zero-blocks'"}}
> >>> 
> >>> If we had somehow rejected the capability when it made no sense,
> >>> removing it now it never makes sense would be obviously fine.
> >>> 
> >>> The straight & narrow path is to deprecate now, remove later.
> >>
> >> I wonder whether we can make this one simpler, as IIUC this cap depends on
> >> the block migration feature, which properly went through the deprecation
> >> process and got removed in the previous release.
> >>
> >> IOW, currently QEMU behaves the same with this cap on/off, ignoring it
> >> completely.  I think it means the deprecation message (even if we provide
> >> some for two extra releases..) wouldn't be anything helpful as anyone who
> >> uses this feature already got affected before this patch.. this feature,
> >> together with block migration, are simply all gone already?
> >
> > We break compatibility for users who supply capability @zero-blocks even
> > though they are not using block migration.
> >
> > Before this patch, the capability is silently ignored.
> >
> > Afterwards, we reject it.
> >
> > This harmless misuse was *not* affected by our prior removal of block
> > migration.
> >
> > It *is* affected by the proposed removal of the capability.
> 
> How does this policy_skip thing works? Could we automatically warn
> whenever a capability has the 'deprecated' feature in migration.json?
> 
> Also, some of the incompatibility errors in migrate_caps_check() could
> be simplified with something like a new:
> 'features': [ 'conflicts': [ 'cap1', 'cap2' ] ]
> to indicate which caps are incompatible between themselves.
> 
> >
> > We either treat this in struct accordance to our rules: deprecate now,
> > remove later.  Or we bend our them:
> >
> >>> If we believe nothing relies on it, we can bend the rules and remove
> >>> right away.
> >
> > Not for me to decide.
> >
> 
> I'm fine either way, but in any case:

OK, so I'll split my code to just remove the dead function rather than the
actual capability; Thanks for Fabiano for doing the Deprecation stuff!

Dave

> -- >8 --
> >From 3ff313a52e37b8cb407c900d7a1aa266560aebb7 Mon Sep 17 00:00:00 2001
> From: Fabiano Rosas <farosas@suse.de>
> Date: Thu, 19 Sep 2024 09:49:44 -0300
> Subject: [PATCH] migration: Deprecate zero-blocks capability
> 
> The zero-blocks capability was meant to be used along with the block
> migration, which has been removed already in commit eef0bae3a7
> ("migration: Remove block migration").
> 
> Setting zero-blocks is currently a noop, but the outright removal of
> the capability would cause and error in case some users are still
> setting it. Put the capability through the deprecation process.
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>  docs/about/deprecated.rst | 6 ++++++
>  migration/options.c       | 4 ++++
>  qapi/migration.json       | 5 ++++-
>  3 files changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
> index ed31d4b0b2..47cabb6fcc 100644
> --- a/docs/about/deprecated.rst
> +++ b/docs/about/deprecated.rst
> @@ -476,3 +476,9 @@ usage of providing a file descriptor to a plain file has been
>  deprecated in favor of explicitly using the ``file:`` URI with the
>  file descriptor being passed as an ``fdset``. Refer to the ``add-fd``
>  command documentation for details on the ``fdset`` usage.
> +
> +``zero-blocks`` capability (since 9.2)
> +''''''''''''''''''''''''''''''''''''''
> +
> +The ``zero-blocks`` capability was part of the block migration which
> +doesn't exist anymore since it was removed in QEMU v9.1.
> diff --git a/migration/options.c b/migration/options.c
> index 147cd2b8fd..b828bad0d9 100644
> --- a/migration/options.c
> +++ b/migration/options.c
> @@ -457,6 +457,10 @@ bool migrate_caps_check(bool *old_caps, bool *new_caps, Error **errp)
>      ERRP_GUARD();
>      MigrationIncomingState *mis = migration_incoming_get_current();
>  
> +    if (new_caps[MIGRATION_CAPABILITY_ZERO_BLOCKS]) {
> +        warn_report("zero-blocks capability is deprecated");
> +    }
> +
>  #ifndef CONFIG_REPLICATION
>      if (new_caps[MIGRATION_CAPABILITY_X_COLO]) {
>          error_setg(errp, "QEMU compiled without replication module"
> diff --git a/qapi/migration.json b/qapi/migration.json
> index b66cccf107..3af6aa1740 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -479,11 +479,14 @@
>  # Features:
>  #
>  # @unstable: Members @x-colo and @x-ignore-shared are experimental.
> +# @deprecated: Member @zero-blocks is deprecated as being part of
> +#     block migration which was already removed.
>  #
>  # Since: 1.2
>  ##
>  { 'enum': 'MigrationCapability',
> -  'data': ['xbzrle', 'rdma-pin-all', 'auto-converge', 'zero-blocks',
> +  'data': ['xbzrle', 'rdma-pin-all', 'auto-converge',
> +           { 'name': 'zero-blocks', 'features': [ 'deprecated' ] },
>             'events', 'postcopy-ram',
>             { 'name': 'x-colo', 'features': [ 'unstable' ] },
>             'release-ram',
> -- 
> 2.35.3
> 
-- 
 -----Open up your eyes, open up your mind, open up your code -------   
/ Dr. David Alan Gilbert    |       Running GNU/Linux       | Happy  \ 
\        dave @ treblig.org |                               | In Hex /
 \ _________________________|_____ http://www.treblig.org   |_______/

Re: [PATCH 2/3] migration: Remove unused zero-blocks capability

[Peter Xu]

On Thu, Sep 19, 2024 at 01:00:34PM +0000, Dr. David Alan Gilbert wrote:
> * Fabiano Rosas (farosas@suse.de) wrote:
> > Markus Armbruster <armbru@redhat.com> writes:
> > 
> > > Peter Xu <peterx@redhat.com> writes:
> > >
> > >> On Wed, Sep 18, 2024 at 07:52:56AM +0200, Markus Armbruster wrote:
> > >>> dave@treblig.org writes:
> > >>> 
> > >>> > From: "Dr. David Alan Gilbert" <dave@treblig.org>
> > >>> >
> > >>> > migrate_zero_blocks is unused since
> > >>> >   eef0bae3a7 ("migration: Remove block migration")
> > >>> >
> > >>> > Remove it.
> > >>> > That whole zero-blocks capability was just for old-school
> > >>> > block migration anyway.
> > >>> >
> > >>> > Remove the capability as well.
> > >>> >
> > >>> > Signed-off-by: Dr. David Alan Gilbert <dave@treblig.org>
> > >>> > ---
> > >>> >  migration/options.c |  8 --------
> > >>> >  migration/options.h |  1 -
> > >>> >  qapi/migration.json | 10 +---------
> > >>> >  3 files changed, 1 insertion(+), 18 deletions(-)
> > >>> >
> > >>> > diff --git a/migration/options.c b/migration/options.c
> > >>> > index 9460c5dee9..997e060612 100644
> > >>> > --- a/migration/options.c
> > >>> > +++ b/migration/options.c
> > >>> > @@ -177,7 +177,6 @@ Property migration_properties[] = {
> > >>> >      DEFINE_PROP_MIG_CAP("x-xbzrle", MIGRATION_CAPABILITY_XBZRLE),
> > >>> >      DEFINE_PROP_MIG_CAP("x-rdma-pin-all", MIGRATION_CAPABILITY_RDMA_PIN_ALL),
> > >>> >      DEFINE_PROP_MIG_CAP("x-auto-converge", MIGRATION_CAPABILITY_AUTO_CONVERGE),
> > >>> > -    DEFINE_PROP_MIG_CAP("x-zero-blocks", MIGRATION_CAPABILITY_ZERO_BLOCKS),
> > >>> >      DEFINE_PROP_MIG_CAP("x-events", MIGRATION_CAPABILITY_EVENTS),
> > >>> >      DEFINE_PROP_MIG_CAP("x-postcopy-ram", MIGRATION_CAPABILITY_POSTCOPY_RAM),
> > >>> >      DEFINE_PROP_MIG_CAP("x-postcopy-preempt",
> > >>> 
> > >>> Property of (pseudo-)device "migration".  The "x-" prefix suggests we
> > >>> expect management software not to rely on it.  Okay.
> > >>> 
> > >>> [...]
> > >>> 
> > >>> > diff --git a/qapi/migration.json b/qapi/migration.json
> > >>> > index b66cccf107..82d0fc962e 100644
> > >>> > --- a/qapi/migration.json
> > >>> > +++ b/qapi/migration.json
> > >>> > @@ -389,13 +389,6 @@
> > >>> >  #     footprint is mlock()'d on demand or all at once.  Refer to
> > >>> >  #     docs/rdma.txt for usage.  Disabled by default.  (since 2.0)
> > >>> >  #
> > >>> > -# @zero-blocks: During storage migration encode blocks of zeroes
> > >>> > -#     efficiently.  This essentially saves 1MB of zeroes per block on
> > >>> > -#     the wire.  Enabling requires source and target VM to support
> > >>> > -#     this feature.  To enable it is sufficient to enable the
> > >>> > -#     capability on the source VM.  The feature is disabled by
> > >>> > -#     default.  (since 1.6)
> > >>> > -#
> > >>> >  # @events: generate events for each migration state change (since 2.4)
> > >>> >  #
> > >>> >  # @auto-converge: If enabled, QEMU will automatically throttle down
> > >>> > @@ -483,7 +476,7 @@
> > >>> >  # Since: 1.2
> > >>> >  ##
> > >>> >  { 'enum': 'MigrationCapability',
> > >>> > -  'data': ['xbzrle', 'rdma-pin-all', 'auto-converge', 'zero-blocks',
> > >>> > +  'data': ['xbzrle', 'rdma-pin-all', 'auto-converge',
> > >>> >             'events', 'postcopy-ram',
> > >>> >             { 'name': 'x-colo', 'features': [ 'unstable' ] },
> > >>> >             'release-ram',
> > >>> 
> > >>> This is used by migrate-set-capabilities and query-migrate-capabilities,
> > >>> via ['MigrationCapabilityStatus'].
> > >>> 
> > >>> query-migrate-capabilities is unaffected: it couldn't return zero-blocks
> > >>> anymore even before the patch.
> > >>> 
> > >>> migrate-set-capabilities changes incompatibly, I'm afraid.  Before the
> > >>> patch:
> > >>> 
> > >>>     {"execute": "migrate-set-capabilities", "arguments": {"capabilities": [{"capability": "zero-blocks", "state": true}]}}
> > >>>     {"return": {}}
> > >>> 
> > >>> Afterwards:
> > >>> 
> > >>>     {"error": {"class": "GenericError", "desc": "Parameter 'capability' does not accept value 'zero-blocks'"}}
> > >>> 
> > >>> If we had somehow rejected the capability when it made no sense,
> > >>> removing it now it never makes sense would be obviously fine.
> > >>> 
> > >>> The straight & narrow path is to deprecate now, remove later.
> > >>
> > >> I wonder whether we can make this one simpler, as IIUC this cap depends on
> > >> the block migration feature, which properly went through the deprecation
> > >> process and got removed in the previous release.
> > >>
> > >> IOW, currently QEMU behaves the same with this cap on/off, ignoring it
> > >> completely.  I think it means the deprecation message (even if we provide
> > >> some for two extra releases..) wouldn't be anything helpful as anyone who
> > >> uses this feature already got affected before this patch.. this feature,
> > >> together with block migration, are simply all gone already?
> > >
> > > We break compatibility for users who supply capability @zero-blocks even
> > > though they are not using block migration.
> > >
> > > Before this patch, the capability is silently ignored.
> > >
> > > Afterwards, we reject it.
> > >
> > > This harmless misuse was *not* affected by our prior removal of block
> > > migration.
> > >
> > > It *is* affected by the proposed removal of the capability.
> > 
> > How does this policy_skip thing works? Could we automatically warn
> > whenever a capability has the 'deprecated' feature in migration.json?
> > 
> > Also, some of the incompatibility errors in migrate_caps_check() could
> > be simplified with something like a new:
> > 'features': [ 'conflicts': [ 'cap1', 'cap2' ] ]
> > to indicate which caps are incompatible between themselves.
> > 
> > >
> > > We either treat this in struct accordance to our rules: deprecate now,
> > > remove later.  Or we bend our them:
> > >
> > >>> If we believe nothing relies on it, we can bend the rules and remove
> > >>> right away.
> > >
> > > Not for me to decide.
> > >
> > 
> > I'm fine either way, but in any case:
> 
> OK, so I'll split my code to just remove the dead function rather than the
> actual capability; Thanks for Fabiano for doing the Deprecation stuff!

I'll wait for a final ACK from Markus, then I can queue below patch
directly into migration-next.

Thanks all!

> 
> Dave
> 
> > -- >8 --
> > >From 3ff313a52e37b8cb407c900d7a1aa266560aebb7 Mon Sep 17 00:00:00 2001
> > From: Fabiano Rosas <farosas@suse.de>
> > Date: Thu, 19 Sep 2024 09:49:44 -0300
> > Subject: [PATCH] migration: Deprecate zero-blocks capability
> > 
> > The zero-blocks capability was meant to be used along with the block
> > migration, which has been removed already in commit eef0bae3a7
> > ("migration: Remove block migration").
> > 
> > Setting zero-blocks is currently a noop, but the outright removal of
> > the capability would cause and error in case some users are still
> > setting it. Put the capability through the deprecation process.
> > 
> > Signed-off-by: Fabiano Rosas <farosas@suse.de>
> > ---
> >  docs/about/deprecated.rst | 6 ++++++
> >  migration/options.c       | 4 ++++
> >  qapi/migration.json       | 5 ++++-
> >  3 files changed, 14 insertions(+), 1 deletion(-)
> > 
> > diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
> > index ed31d4b0b2..47cabb6fcc 100644
> > --- a/docs/about/deprecated.rst
> > +++ b/docs/about/deprecated.rst
> > @@ -476,3 +476,9 @@ usage of providing a file descriptor to a plain file has been
> >  deprecated in favor of explicitly using the ``file:`` URI with the
> >  file descriptor being passed as an ``fdset``. Refer to the ``add-fd``
> >  command documentation for details on the ``fdset`` usage.
> > +
> > +``zero-blocks`` capability (since 9.2)
> > +''''''''''''''''''''''''''''''''''''''
> > +
> > +The ``zero-blocks`` capability was part of the block migration which
> > +doesn't exist anymore since it was removed in QEMU v9.1.
> > diff --git a/migration/options.c b/migration/options.c
> > index 147cd2b8fd..b828bad0d9 100644
> > --- a/migration/options.c
> > +++ b/migration/options.c
> > @@ -457,6 +457,10 @@ bool migrate_caps_check(bool *old_caps, bool *new_caps, Error **errp)
> >      ERRP_GUARD();
> >      MigrationIncomingState *mis = migration_incoming_get_current();
> >  
> > +    if (new_caps[MIGRATION_CAPABILITY_ZERO_BLOCKS]) {
> > +        warn_report("zero-blocks capability is deprecated");
> > +    }
> > +
> >  #ifndef CONFIG_REPLICATION
> >      if (new_caps[MIGRATION_CAPABILITY_X_COLO]) {
> >          error_setg(errp, "QEMU compiled without replication module"
> > diff --git a/qapi/migration.json b/qapi/migration.json
> > index b66cccf107..3af6aa1740 100644
> > --- a/qapi/migration.json
> > +++ b/qapi/migration.json
> > @@ -479,11 +479,14 @@
> >  # Features:
> >  #
> >  # @unstable: Members @x-colo and @x-ignore-shared are experimental.
> > +# @deprecated: Member @zero-blocks is deprecated as being part of
> > +#     block migration which was already removed.
> >  #
> >  # Since: 1.2
> >  ##
> >  { 'enum': 'MigrationCapability',
> > -  'data': ['xbzrle', 'rdma-pin-all', 'auto-converge', 'zero-blocks',
> > +  'data': ['xbzrle', 'rdma-pin-all', 'auto-converge',
> > +           { 'name': 'zero-blocks', 'features': [ 'deprecated' ] },
> >             'events', 'postcopy-ram',
> >             { 'name': 'x-colo', 'features': [ 'unstable' ] },
> >             'release-ram',
> > -- 
> > 2.35.3
> > 
> -- 
>  -----Open up your eyes, open up your mind, open up your code -------   
> / Dr. David Alan Gilbert    |       Running GNU/Linux       | Happy  \ 
> \        dave @ treblig.org |                               | In Hex /
>  \ _________________________|_____ http://www.treblig.org   |_______/
> 

-- 
Peter Xu