Eric Blake <ebl...@redhat.com> writes:
> On 11/24/20 7:36 AM, Markus Armbruster wrote:
>> Markus Armbruster <arm...@redhat.com> writes:
>>
>>> Yuri Benditovich <yuri.benditov...@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?
