Re: [PATCH v2 2/2] net: Drop the NetLegacy structure, always use Netdev instead

[Thomas Huth <thuth@redhat.com>]

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 <thuth@redhat.com>
>> ---
> 
>> +++ 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