Re: [PATCH v3 1/4] qapi: net: Add query-netdevs command

From: Alexey Kirillov <lekiravi@yandex-team.ru>
To: Markus Armbruster <armbru@redhat.com>
Cc: Laurent Vivier <lvivier@redhat.com>,
	Thomas Huth <thuth@redhat.com>,
	"Michael S. Tsirkin" <mst@redhat.com>,
	Stefan Weil <sw@weilnetz.de>, Jason Wang <jasowang@redhat.com>,
	"qemu-devel@nongnu.org" <qemu-devel@nongnu.org>,
	Vincenzo Maffione <v.maffione@gmail.com>,
	"yc-core@yandex-team.ru" <yc-core@yandex-team.ru>,
	Samuel Thibault <samuel.thibault@ens-lyon.org>,
	Paolo Bonzini <pbonzini@redhat.com>,
	Giuseppe Lettieri <g.lettieri@iet.unipi.it>,
	Luigi Rizzo <rizzo@iet.unipi.it>
Subject: Re: [PATCH v3 1/4] qapi: net: Add query-netdevs command
Date: Wed, 09 Sep 2020 15:45:08 +0300	[thread overview]
Message-ID: <35731599654153@mail.yandex-team.ru> (raw)
In-Reply-To: <87k0x3xjmv.fsf@dusky.pond.sub.org>

09.09.2020, 14:41, "Markus Armbruster" <armbru@redhat.com>:
> Alexey Kirillov <lekiravi@yandex-team.ru> writes:
>
>>  07.09.2020, 15:40, "Markus Armbruster" <armbru@redhat.com>:
>>>  Alexey Kirillov <lekiravi@yandex-team.ru> writes:
>>>
>>>>   Hi!
>>>>
>>>>   02.09.2020, 14:23, "Markus Armbruster" <armbru@redhat.com>:
>>>>>   Alexey Kirillov <lekiravi@yandex-team.ru> writes:
>>>>>
>>>>>>    Add a qmp command that provides information about currently attached
>>>>>>    backend network devices and their configuration.
>>>>>>
>>>>>>    Signed-off-by: Alexey Kirillov <lekiravi@yandex-team.ru>
>>>>>
>>>>>   [...]
>>>>>>    diff --git a/qapi/net.json b/qapi/net.json
>>>>>>    index ddb113e5e5..c09322bb42 100644
>>>>>>    --- a/qapi/net.json
>>>>>>    +++ b/qapi/net.json
>>>>>>    @@ -714,3 +714,71 @@
>>>>>>     ##
>>>>>>     { 'event': 'FAILOVER_NEGOTIATED',
>>>>>>       'data': {'device-id': 'str'} }
>>>>>>    +
>>>>>>    +##
>>>>>>    +# @NetdevInfo:
>>>>>>    +#
>>>>>>    +# Configuration of a network backend device (netdev).
>>>>>>    +#
>>>>>>    +# @id: Device identifier.
>>>>>>    +#
>>>>>>    +# @type: Specify the driver used for interpreting remaining arguments.
>>>>>>    +#
>>>>>>    +# @peer-id: Connected frontend network device identifier.
>>>>>>    +#
>>>>>>    +# Since: 5.2
>>>>>>    +##
>>>>>>    +{ 'union': 'NetdevInfo',
>>>>>>    + 'base': { 'id': 'str',
>>>>>>    + 'type': 'NetClientDriver',
>>>>>>    + '*peer-id': 'str' },
>>>>>>    + 'discriminator': 'type',
>>>>>>    + 'data': {
>>>>>>    + 'user': 'NetdevUserOptions',
>>>>>>    + 'tap': 'NetdevTapOptions',
>>>>>>    + 'l2tpv3': 'NetdevL2TPv3Options',
>>>>>>    + 'socket': 'NetdevSocketOptions',
>>>>>>    + 'vde': 'NetdevVdeOptions',
>>>>>>    + 'bridge': 'NetdevBridgeOptions',
>>>>>>    + 'netmap': 'NetdevNetmapOptions',
>>>>>>    + 'vhost-user': 'NetdevVhostUserOptions',
>>>>>>    + 'vhost-vdpa': 'NetdevVhostVDPAOptions' } }
>>>>>
>>>>>   This is union Netdev plus a common member @peer-id, less the variant
>>>>>   members for NetClientDriver values 'nic' and 'hubport'.
>>>>>
>>>>>   Can 'type: 'nic' and 'type': 'hubport' occur?
>>>>
>>>>   No, it can't. We don't support NIC/hubport in query-netdevs, so we neither create this
>>>>   structure for them, nor store config.
>>>
>>>  Same for 'none', I guess.
>>>
>>>  As defined, NetdevInfo allows types 'none', 'nic', and 'hubport', it
>>>  just has no variant members for them. The fact that they can't occur is
>>>  not coded into the type, and therefore not visible in introspection.
>>>
>>>  To make introspection more precise, we'd have to define a new enum type.
>>>  How much that would complicate the C code is unclear.
>>>
>>>  Do we need it to be more precise? Eric, got an opinion?
>>>
>>>  Existing type Netdev has the same issue for 'none' and 'nic'. I guess
>>>  this is so we can reuse the type for -net. Not sure that's a good idea,
>>>  but not this patch's problem.
>>>
>>>>>   What's the use case for @peer-id?
>>>>
>>>>   Main reason is to provide information "is this backend connected to frontend device".
>>>>   User also may want to know which backend used for frontend device.
>>>>
>>>>>   Assuming we want @peer-id: its documentation is too vague. Cases:
>>>>>
>>>>>   * Not connected to a frontend: I guess @peer-id is absent then.
>>>>
>>>>   Absolutely correct.
>>>>
>>>>>   * Connected to a frontend
>>>>>
>>>>>     - that has a qdev ID (the thing you set with -device id=...): I guess
>>>>>       it's the qdev ID then.
>>>>
>>>>   Correct.
>>>>
>>>>>     - that doesn't have a qdev ID: anyone's guess.
>>>>
>>>>   We use field "name" of structure NetClientState, so if there is no direct ID setting,
>>>>   there must be generated name (in format "model.id").
>>>
>>>  Perhaps:
>>>
>>>        # @peer-id: the connected network backend's name (absent if no
>>>        # backend is connected)
>>
>>  Maybe you mean:
>>
>>  # @peer-id: The connected frontend network device name (absent if no frontend
>>  # is connected).
>
> Yes, I wrote backend, but meant frontend.
>
> Aside: our naming is undisciplined: we use both "id" and "name" in QMP
> for NetClientState member name. Unfortunate.

Well, I was looking at netdev_add/netdev_del when choosing member's name.
Looks like "name" more often used for NICs when "id" is for common netdevs.

It seems to me that the "id" is more suitable for network devices, since
it is uniquely define them when calling QMP commands.

>>  In any case, thanks for pointing. I'll add this in the next patch version.
>
> [...]

-- 
Alexey Kirillov
Yandex.Cloud