Re: [PATCH v6 16/19] qapi: add interface for backend-transfer virtio-net/tap migration

[Markus Armbruster <armbru@redhat.com>]

Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:

> On 06.10.25 16:23, Markus Armbruster wrote:
>> Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
>> 
>>> To migrate virtio-net TAP device backend (including open fds) locally,
>>> user should simply set migration parameter
>>>
>>>     backend-transfer = ["virtio-net-tap"]
>>>
>>> Why not simple boolean? To simplify migration to further versions,
>>> when more devices will support backend-transfer migration.
>>>
>>> Alternatively, we may add per-device option to disable backend-transfer
>>> migration, but still:
>>>
>>> 1. It's more comfortable to set same capabilities/parameters on both
>>> source and target QEMU, than care about each device.
>>>
>>> 2. To not break the design, that machine-type + device options +
>>> migration capabilities and parameters are fully define the resulting
>>> migration stream. We'll break this if add in future more
>>> backend-transfer support in devices under same backend-transfer=true
>>> parameter.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

[...]

>>> diff --git a/migration/options.h b/migration/options.h
>>> index 82d839709e..55c0345433 100644
>>> --- a/migration/options.h
>>> +++ b/migration/options.h
>>> @@ -87,6 +87,8 @@ const char *migrate_tls_hostname(void);
>>>  uint64_t migrate_xbzrle_cache_size(void);
>>>  ZeroPageDetection migrate_zero_page_detection(void);
>>>   
>>> +bool migrate_virtio_net_tap(void);
>>> +
>>>  /* parameters helpers */
>>>   
>>>   bool migrate_params_check(MigrationParameters *params, Error **errp);
>>> diff --git a/qapi/migration.json b/qapi/migration.json
>>> index 2387c21e9c..e39785dc07 100644
>>> --- a/qapi/migration.json
>>> +++ b/qapi/migration.json
>>> @@ -747,6 +747,18 @@
>>>        '*transform': 'BitmapMigrationBitmapAliasTransform'
>>>    } }
>>>   
>>> +##
>>> +# @BackendTransfer:
>>> +#
>>> +# @virtio-net-tap: Enable backend-transfer migration for virtio-net/tap. When
>>> +#     enabled, TAP fds and all related state is passed to target QEMU through
>>> +#     migration channel (which should be unix socket).
>> 
>> Suggest "are passed to the destination in the migration channel" and
>> "should be a UNIX domain socket".
>> 
>> docs/devel/qapi-code-gen.rst:
>> 
>>      For legibility, wrap text paragraphs so every line is at most 70
>>      characters long.
>> 
>>      Separate sentences with two spaces.
>
> Ok. We do lack this check in checkpatch

Would be nice, yes.

>>> +#
>>> +# Since: 10.2
>>> +##
>>> +{ 'enum': 'BackendTransfer',
>>> +  'data': [ 'virtio-net-tap' ] }
>>> +
>>>  ##
>>>  # @BitmapMigrationNodeAlias:
>>>  #
>>> @@ -924,10 +936,14 @@
>>>  #     only has effect if the @mapped-ram capability is enabled.
>>>  #     (Since 9.1)
>>>  #
>>> +# @backend-transfer: List of targets to enable backend-transfer
>>> +#     migration for. This requires migration channel to be a unix
>>> +#     socket (to pass fds through). (Since 10.2)
>> 
>> Elsewhere, we describe the same restriction like this:
>> 
>>                                           This CPR channel must support
>>    #     file descriptor transfer with SCM_RIGHTS, i.e. it must be a
>>    #     UNIX domain socket.
>> 
>
> Thanks, I'll copy this phrasing to be consistent.
>
>>> +#
>>>  # Features:
>>>  #
>>> -# @unstable: Members @x-checkpoint-delay and
>>> -#     @x-vcpu-dirty-limit-period are experimental.
>>> +# @unstable: Members @x-checkpoint-delay,
>>> +#     @x-vcpu-dirty-limit-period and @backend-transfer are experimental.
>> 
>> List members in alphabetical order, please.
>> 
>>>  #
>>>  # Since: 2.4
>>>  ##
>>> @@ -950,7 +966,8 @@
>>>              'vcpu-dirty-limit',
>>>              'mode',
>>>              'zero-page-detection',
>>> -           'direct-io'] }
>>> +           'direct-io',
>>> +           'backend-transfer' ] }
>> 
>> Forgot feature 'unstable'?
>
> Opps. Interesting, how it compiles? Usually, inconsistencies between
> QAPI comments and definitions are hardly checked.

You found a gap in the checking.

[...]