[Qemu-devel] [PATCH] net: synchronize net_host_device_remove with host_net_remove_completion

[Qemu-devel] [PATCH] net: synchronize net_host_device_remove with host_net_remove_completion

[Paolo Bonzini]

Using net_host_check_device is unnecessary.  qemu_del_net_client asserts
for the non-peer case that it can only process NIC type NetClientStates,
and that assertion is valid for the peered case as well, so move it and
use the same check in net_host_device_remove.  host_net_remove_completion
is already checking the type.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 net/net.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/net.c b/net/net.c
index 7acc162..1da612f 100644
--- a/net/net.c
+++ b/net/net.c
@@ -324,6 +324,8 @@ void qemu_del_net_client(NetClientState *nc)
     NetClientState *ncs[MAX_QUEUE_NUM];
     int queues, i;
 
+    assert(nc->info->type != NET_CLIENT_OPTIONS_KIND_NIC);
+
     /* If the NetClientState belongs to a multiqueue backend, we will change all
      * other NetClientStates also.
      */
@@ -355,8 +357,6 @@ void qemu_del_net_client(NetClientState *nc)
         return;
     }
 
-    assert(nc->info->type != NET_CLIENT_OPTIONS_KIND_NIC);
-
     for (i = 0; i < queues; i++) {
         qemu_cleanup_net_client(ncs[i]);
         qemu_free_net_client(ncs[i]);
@@ -992,7 +992,7 @@ void net_host_device_remove(Monitor *mon, const QDict *qdict)
                      device, vlan_id);
         return;
     }
-    if (!net_host_check_device(nc->model)) {
+    if (nc->info->type == NET_CLIENT_OPTIONS_KIND_NIC) {
         error_report("invalid host network device '%s'", device);
         return;
     }
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH] net: synchronize net_host_device_remove with host_net_remove_completion

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1052 bytes --]

On Tue, Dec 23, 2014 at 05:53:20PM +0100, Paolo Bonzini wrote:
> @@ -324,6 +324,8 @@ void qemu_del_net_client(NetClientState *nc)
>      NetClientState *ncs[MAX_QUEUE_NUM];
>      int queues, i;
>  
> +    assert(nc->info->type != NET_CLIENT_OPTIONS_KIND_NIC);
> +
>      /* If the NetClientState belongs to a multiqueue backend, we will change all
>       * other NetClientStates also.
>       */
> @@ -355,8 +357,6 @@ void qemu_del_net_client(NetClientState *nc)
>          return;
>      }
>  
> -    assert(nc->info->type != NET_CLIENT_OPTIONS_KIND_NIC);
> -
>      for (i = 0; i < queues; i++) {
>          qemu_cleanup_net_client(ncs[i]);
>          qemu_free_net_client(ncs[i]);

The assert can be dropped completely since the code already has an
equivalent assert:

  queues = qemu_find_net_clients_except(nc->name, ncs,
                                        NET_CLIENT_OPTIONS_KIND_NIC,
                                        MAX_QUEUE_NUM);
  assert(queues != 0); <-- fail if type == NET_CLIENT_OPTIONS_KIND_NIC

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH] net: synchronize net_host_device_remove with host_net_remove_completion

[Paolo Bonzini]

On 02/01/2015 15:06, Stefan Hajnoczi wrote:
> On Tue, Dec 23, 2014 at 05:53:20PM +0100, Paolo Bonzini wrote:
>> @@ -324,6 +324,8 @@ void qemu_del_net_client(NetClientState *nc)
>>      NetClientState *ncs[MAX_QUEUE_NUM];
>>      int queues, i;
>>  
>> +    assert(nc->info->type != NET_CLIENT_OPTIONS_KIND_NIC);
>> +
>>      /* If the NetClientState belongs to a multiqueue backend, we will change all
>>       * other NetClientStates also.
>>       */
>> @@ -355,8 +357,6 @@ void qemu_del_net_client(NetClientState *nc)
>>          return;
>>      }
>>  
>> -    assert(nc->info->type != NET_CLIENT_OPTIONS_KIND_NIC);
>> -
>>      for (i = 0; i < queues; i++) {
>>          qemu_cleanup_net_client(ncs[i]);
>>          qemu_free_net_client(ncs[i]);
> 
> The assert can be dropped completely since the code already has an
> equivalent assert:
> 
>   queues = qemu_find_net_clients_except(nc->name, ncs,
>                                         NET_CLIENT_OPTIONS_KIND_NIC,
>                                         MAX_QUEUE_NUM);
>   assert(queues != 0); <-- fail if type == NET_CLIENT_OPTIONS_KIND_NIC

I left it on purpose for documentation, but I'll send v2 next week that
removes it.

Paolo

Re: [Qemu-devel] [PATCH] net: synchronize net_host_device_remove with host_net_remove_completion

[Paolo Bonzini]

On 02/01/2015 17:20, Paolo Bonzini wrote:
>> > 
>> > The assert can be dropped completely since the code already has an
>> > equivalent assert:
>> > 
>> >   queues = qemu_find_net_clients_except(nc->name, ncs,
>> >                                         NET_CLIENT_OPTIONS_KIND_NIC,
>> >                                         MAX_QUEUE_NUM);
>> >   assert(queues != 0); <-- fail if type == NET_CLIENT_OPTIONS_KIND_NIC
> I left it on purpose for documentation, but I'll send v2 next week that
> removes it.

Actually it's not the same.  If you have "-netdev user,id=e1000 -device
e1000,netdev=e1000" you will be able to call qemu_del_net_client on the
NIC, and it will _not_ fail if the assertion is removed.

Paolo

Re: [Qemu-devel] [PATCH] net: synchronize net_host_device_remove with host_net_remove_completion

[Paolo Bonzini]

On 19/01/2015 12:27, Paolo Bonzini wrote:
> 
> 
> On 02/01/2015 17:20, Paolo Bonzini wrote:
>>>>
>>>> The assert can be dropped completely since the code already has an
>>>> equivalent assert:
>>>>
>>>>   queues = qemu_find_net_clients_except(nc->name, ncs,
>>>>                                         NET_CLIENT_OPTIONS_KIND_NIC,
>>>>                                         MAX_QUEUE_NUM);
>>>>   assert(queues != 0); <-- fail if type == NET_CLIENT_OPTIONS_KIND_NIC
>> I left it on purpose for documentation, but I'll send v2 next week that
>> removes it.
> 
> Actually it's not the same.  If you have "-netdev user,id=e1000 -device
> e1000,netdev=e1000" you will be able to call qemu_del_net_client on the
> NIC, and it will _not_ fail if the assertion is removed.

Ping?

Paolo

Re: [Qemu-devel] [PATCH] net: synchronize net_host_device_remove with host_net_remove_completion

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1080 bytes --]

On Mon, Jan 19, 2015 at 12:27:11PM +0100, Paolo Bonzini wrote:
> On 02/01/2015 17:20, Paolo Bonzini wrote:
> >> > 
> >> > The assert can be dropped completely since the code already has an
> >> > equivalent assert:
> >> > 
> >> >   queues = qemu_find_net_clients_except(nc->name, ncs,
> >> >                                         NET_CLIENT_OPTIONS_KIND_NIC,
> >> >                                         MAX_QUEUE_NUM);
> >> >   assert(queues != 0); <-- fail if type == NET_CLIENT_OPTIONS_KIND_NIC
> > I left it on purpose for documentation, but I'll send v2 next week that
> > removes it.
> 
> Actually it's not the same.  If you have "-netdev user,id=e1000 -device
> e1000,netdev=e1000" you will be able to call qemu_del_net_client on the
> NIC, and it will _not_ fail if the assertion is removed.

I don't follow.

If you call qemu_del_net_client(e1000_nic) then
qemu_find_net_clients_except(nc->name, ncs, NET_CLIENT_OPTIONS_KIND_NIC,
MAX_QUEUE_NUM) returns 0.  This causes the assert(queues != 0) to fail.

Can you explain the scenario?

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH] net: synchronize net_host_device_remove with host_net_remove_completion

[Paolo Bonzini]

On 06/02/2015 14:54, Stefan Hajnoczi wrote:
> On Mon, Jan 19, 2015 at 12:27:11PM +0100, Paolo Bonzini wrote:
>> On 02/01/2015 17:20, Paolo Bonzini wrote:
>>>>>
>>>>> The assert can be dropped completely since the code already has an
>>>>> equivalent assert:
>>>>>
>>>>>   queues = qemu_find_net_clients_except(nc->name, ncs,
>>>>>                                         NET_CLIENT_OPTIONS_KIND_NIC,
>>>>>                                         MAX_QUEUE_NUM);
>>>>>   assert(queues != 0); <-- fail if type == NET_CLIENT_OPTIONS_KIND_NIC
>>> I left it on purpose for documentation, but I'll send v2 next week that
>>> removes it.
>>
>> Actually it's not the same.  If you have "-netdev user,id=e1000 -device
>> e1000,netdev=e1000" you will be able to call qemu_del_net_client on the
>> NIC, and it will _not_ fail if the assertion is removed.
> 
> I don't follow.
> 
> If you call qemu_del_net_client(e1000_nic) then
> qemu_find_net_clients_except(nc->name, ncs, NET_CLIENT_OPTIONS_KIND_NIC,
> MAX_QUEUE_NUM) returns 0.  This causes the assert(queues != 0) to fail.

NICs and other clients are in separate namespaces.  So if you do

   -netdev user,id=e1000
   -device e1000,netdev=e1000,id=e1000

you have two NetClients named "e1000".  If you call (by mistake)
qemu_del_net_client(e1000_nic), qemu_find_net_clients_except will return
the SLIRP client and the assertion will not fail.

So you need a separate assertion.

Paolo

Re: [Qemu-devel] [PATCH] net: synchronize net_host_device_remove with host_net_remove_completion

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1654 bytes --]

On Fri, Feb 06, 2015 at 03:46:42PM +0100, Paolo Bonzini wrote:
> 
> 
> On 06/02/2015 14:54, Stefan Hajnoczi wrote:
> > On Mon, Jan 19, 2015 at 12:27:11PM +0100, Paolo Bonzini wrote:
> >> On 02/01/2015 17:20, Paolo Bonzini wrote:
> >>>>>
> >>>>> The assert can be dropped completely since the code already has an
> >>>>> equivalent assert:
> >>>>>
> >>>>>   queues = qemu_find_net_clients_except(nc->name, ncs,
> >>>>>                                         NET_CLIENT_OPTIONS_KIND_NIC,
> >>>>>                                         MAX_QUEUE_NUM);
> >>>>>   assert(queues != 0); <-- fail if type == NET_CLIENT_OPTIONS_KIND_NIC
> >>> I left it on purpose for documentation, but I'll send v2 next week that
> >>> removes it.
> >>
> >> Actually it's not the same.  If you have "-netdev user,id=e1000 -device
> >> e1000,netdev=e1000" you will be able to call qemu_del_net_client on the
> >> NIC, and it will _not_ fail if the assertion is removed.
> > 
> > I don't follow.
> > 
> > If you call qemu_del_net_client(e1000_nic) then
> > qemu_find_net_clients_except(nc->name, ncs, NET_CLIENT_OPTIONS_KIND_NIC,
> > MAX_QUEUE_NUM) returns 0.  This causes the assert(queues != 0) to fail.
> 
> NICs and other clients are in separate namespaces.  So if you do
> 
>    -netdev user,id=e1000
>    -device e1000,netdev=e1000,id=e1000
> 
> you have two NetClients named "e1000".  If you call (by mistake)
> qemu_del_net_client(e1000_nic), qemu_find_net_clients_except will return
> the SLIRP client and the assertion will not fail.

Thanks for explaining.

Applied to my net tree:
https://github.com/stefanha/qemu/commits/net

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH] net: synchronize net_host_device_remove with host_net_remove_completion

[Jason Wang]

On 12/24/2014 12:53 AM, Paolo Bonzini wrote:
> Using net_host_check_device is unnecessary.  qemu_del_net_client asserts
> for the non-peer case that it can only process NIC type NetClientStates,
> and that assertion is valid for the peered case as well, so move it and
> use the same check in net_host_device_remove.  host_net_remove_completion
> is already checking the type.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  net/net.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/net/net.c b/net/net.c
> index 7acc162..1da612f 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -324,6 +324,8 @@ void qemu_del_net_client(NetClientState *nc)
>      NetClientState *ncs[MAX_QUEUE_NUM];
>      int queues, i;
>  
> +    assert(nc->info->type != NET_CLIENT_OPTIONS_KIND_NIC);
> +
>      /* If the NetClientState belongs to a multiqueue backend, we will change all
>       * other NetClientStates also.
>       */
> @@ -355,8 +357,6 @@ void qemu_del_net_client(NetClientState *nc)
>          return;
>      }
>  
> -    assert(nc->info->type != NET_CLIENT_OPTIONS_KIND_NIC);
> -
>      for (i = 0; i < queues; i++) {
>          qemu_cleanup_net_client(ncs[i]);
>          qemu_free_net_client(ncs[i]);
> @@ -992,7 +992,7 @@ void net_host_device_remove(Monitor *mon, const QDict *qdict)
>                       device, vlan_id);
>          return;
>      }
> -    if (!net_host_check_device(nc->model)) {
> +    if (nc->info->type == NET_CLIENT_OPTIONS_KIND_NIC) {
>          error_report("invalid host network device '%s'", device);
>          return;
>      }

Reviewed-by: Jason Wang <jasowang@redhat.com>