Re: [PATCH] hmp: Changed hmp_netdev_add() using qmp_marshal_netdev_add()

[Markus Armbruster <armbru@redhat.com>]

Eric Blake <eblake@redhat.com> writes:

> On 11/24/20 7:36 AM, Markus Armbruster wrote:
>> Markus Armbruster <armbru@redhat.com> writes:
>> 
>>> Yuri Benditovich <yuri.benditovich@daynix.com> writes:
>>>
>>>> Please confirm that this patch is intended to solve only the problem with
>>>> hmp (and disallow duplicated ids)
>>>
>>> The intent is to reject duplicate ID and to accept non-duplicate ID, no
>>> matter how the device is created (CLI, HMP, QMP) or a prior instance was
>>> deleted (HMP, QMP).
>>>
>>>> With it the netdev that was added from qemu's command line and was deleted
>>>> (for example by hmp) still can't be created, correct?
>>>
>>> Yet another case; back to the drawing board...
>> 
>> Next try.  Hope this is one holds water :)
>> 
>> 
>> diff --git a/net/net.c b/net/net.c
>> index 794c652282..c1dc75fc37 100644
>> --- a/net/net.c
>> +++ b/net/net.c
>> @@ -978,6 +978,7 @@ static int (* const net_client_init_fun[NET_CLIENT_DRIVER__MAX])(
>>  static int net_client_init1(const Netdev *netdev, bool is_netdev, Error **errp)
>>  {
>>      NetClientState *peer = NULL;
>> +    NetClientState *nc;
>>  
>>      if (is_netdev) {
>>          if (netdev->type == NET_CLIENT_DRIVER_NIC ||
>> @@ -1005,6 +1006,12 @@ static int net_client_init1(const Netdev *netdev, bool is_netdev, Error **errp)
>>          }
>>      }
>>  
>> +    nc = qemu_find_netdev(netdev->id);
>> +    if (nc) {
>> +        error_setg(errp, "Duplicate ID '%s'", netdev->id);
>> +        return -1;
>> +    }
>
> Here, we fail if qemu_find_netdev() succeeded, regardless of whether
> is_netdev was set...
>
>> +
>>      if (net_client_init_fun[netdev->type](netdev, netdev->id, peer, errp) < 0) {
>>          /* FIXME drop when all init functions store an Error */
>>          if (errp && !*errp) {
>> @@ -1015,8 +1022,6 @@ static int net_client_init1(const Netdev *netdev, bool is_netdev, Error **errp)
>>      }
>>  
>>      if (is_netdev) {
>> -        NetClientState *nc;
>> -
>>          nc = qemu_find_netdev(netdev->id);
>>          assert(nc);
>
> and here, when is_netdev is set, we expect qemu_find_netdev() to
> succeed.  Does the first hunk need to be 'if (nc && !is_netdev)' ?

My head hurts.

I suspect splitting the function into one function for is_netdev=false
and another one for is_netdev=true would result in more readable code.
Same for net_client_init().

That said, a duplicate ID is to be rejected regardless of is_netdev,
isn't it?

Let's examine how we can get here with is_netdev=false.

Callers:

* net_client_init(): passes its own is_netdev argumet

  Callers:

  - netdev_add(): passes true

  - net_init_client(): passes false

    Caller: net_init_clients() for each -net.  The IDs are all distinct.
    The error check I add in this patch redundant in this case.  It is
    not wrong.

  - net_init_netdev(): passes true

  - net_param_nic(): passes true

* qmp_netdev_add(): passes true

Okay?