[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 2/2] net: Drop the NetLegacy structure, always use Netdev
From: |
Thomas Huth |
Subject: |
Re: [PATCH v2 2/2] net: Drop the NetLegacy structure, always use Netdev instead |
Date: |
Tue, 12 May 2020 17:13:42 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.9.0 |
On 12/05/2020 16.32, Eric Blake wrote:
> On 5/12/20 7:31 AM, Thomas Huth wrote:
>> Now that the "name" parameter is gone, there is hardly any difference
>> between NetLegacy and Netdev anymore. Drop NetLegacy and always use
>> Netdev to simplify the code quite a bit.
>>
>> Signed-off-by: Thomas Huth <address@hidden>
>> ---
>
>> +++ b/net/net.c
>> @@ -967,13 +967,14 @@ static int (* const
>> net_client_init_fun[NET_CLIENT_DRIVER__MAX])(
>> static int net_client_init1(const void *object, bool is_netdev,
>> Error **errp)
>
> Why do we still need the 'is_netdev' parameter?
Yes. See net_init_client(), it uses "false" here.
> If all callers are
> passing in a netdev, then either this parameter needs to be renamed to
> capture the actual difference between callers, or it can be dropped
> altogether.
Don't think of the "Netdev" structure when you're looking at
"is_netdev", rather think about the "-netdev" comand line parameter.
"is_netdev" is used to distinguish between "-netdev" and "-net".
>> {
>> - Netdev legacy = {0};
>> - const Netdev *netdev;
>> + const Netdev *netdev = object;
>> NetClientState *peer = NULL;
>> if (is_netdev) {
>> - netdev = object;
>> -
>> + if (!netdev->has_id) {
>> + error_setg(errp, QERR_MISSING_PARAMETER, "id");
>> + return -1;
>> + }
>
> You wouldn't need this if 'id' remained mandatory.
It's still required for "-net" - see my reply to patch 1/2.
>> @@ -981,56 +982,11 @@ static int net_client_init1(const void *object,
>> bool is_netdev, Error **errp)
>> return -1;
>> }
>> } else {
>> - const NetLegacy *net = object;
>> - const NetLegacyOptions *opts = net->opts;
>> - legacy.id = net->id;
>> - netdev = &legacy;
>> -
>> - /* Map the old options to the new flat type */
>> - switch (opts->type) {
>> - case NET_LEGACY_OPTIONS_TYPE_NONE:
>> + if (netdev->type == NET_CLIENT_DRIVER_NONE) {
>> return 0; /* nothing to do */
>
>
>> -
>> - if (!net_client_init_fun[netdev->type]) {
>> + if (netdev->type == NET_CLIENT_DRIVER_HUBPORT ||
>> + !net_client_init_fun[netdev->type]) {
>> error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "type",
>> "a net backend type (maybe it is not compiled "
>> "into this binary)");
>
> So maybe we still want this legacy-handling code, but renaming
> 'is_netdev' to 'legacy_handling' may make more sense.
Hmm, maybe renaming would be good indeed to avoid confusion here... I'll
think about it.
>> @@ -1039,7 +995,7 @@ static int net_client_init1(const void *object,
>> bool is_netdev, Error **errp)
>> /* Do not add to a hub if it's a nic with a netdev=
>> parameter. */
>> if (netdev->type != NET_CLIENT_DRIVER_NIC ||
>> - !opts->u.nic.has_netdev) {
>> + !netdev->u.nic.has_netdev) {
>> peer = net_hub_add_port(0, NULL, NULL);
>> }
>> }
>> @@ -1143,21 +1099,13 @@ static int net_client_init(QemuOpts *opts,
>> bool is_netdev, Error **errp)
>> }
>> }
>> - if (is_netdev) {
>> - visit_type_Netdev(v, NULL, (Netdev **)&object, &err);
>> - } else {
>> - visit_type_NetLegacy(v, NULL, (NetLegacy **)&object, &err);
>> - }
>> + visit_type_Netdev(v, NULL, (Netdev **)&object, &err);
>
> Why do we need to cast? If all callers are passing a Netdev, can't we
> just give 'object' the correct type to begin with?
Agreed, it should be possible to use Netdev* instead of void* for
"object" now. I'll change that in v3.
>> +++ b/qapi/net.json
>> @@ -453,7 +453,7 @@
>> # 'l2tpv3' - since 2.1
>> ##
>> { 'union': 'Netdev',
>> - 'base': { 'id': 'str', 'type': 'NetClientDriver' },
>> + 'base': { '*id': 'str', 'type': 'NetClientDriver' },
>
> I don't think we need to make 'id' optional.
It's required for "-net" now.
>> 'discriminator': 'type',
>> 'data': {
>> 'nic': 'NetLegacyNicOptions',
>> @@ -467,52 +467,6 @@
>> 'netmap': 'NetdevNetmapOptions',
>> 'vhost-user': 'NetdevVhostUserOptions' } }
>> -##
>> -# @NetLegacy:
>> -#
>> -# Captures the configuration of a network device; legacy.
>> -#
>> -# @id: identifier for monitor commands
>> -#
>> -# @opts: device type specific properties (legacy)
>> -#
>> -# Since: 1.2
>> -##
>> -{ 'struct': 'NetLegacy',
>> - 'data': {
>> - '*id': 'str',
>> - 'opts': 'NetLegacyOptions' } }
>> -
>> -##
>> -# @NetLegacyOptionsType:
>> -#
>> -# Since: 1.2
>> -##
>> -{ 'enum': 'NetLegacyOptionsType',
>> - 'data': ['none', 'nic', 'user', 'tap', 'l2tpv3', 'socket', 'vde',
>> - 'bridge', 'netmap', 'vhost-user'] }
>
> NetLegacyOptionsType differs from NetClientDriver only by missing
> 'hubport', which your code above special-cased, so on that front, the
> simplification makes sense.
Thanks for your review!
Thomas