diff mbox

net: synchronize net_host_device_remove with host_net_remove_completion

Message ID 1419353600-30519-2-git-send-email-pbonzini@redhat.com
State New
Headers show

Commit Message

Paolo Bonzini Dec. 23, 2014, 4:53 p.m. UTC 
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(-)

Comments

Stefan Hajnoczi Jan. 2, 2015, 2:06 p.m. UTC | #1 
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
Paolo Bonzini Jan. 2, 2015, 4:20 p.m. UTC | #2 
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
Paolo Bonzini Jan. 19, 2015, 11:27 a.m. UTC | #3 
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
Paolo Bonzini Jan. 28, 2015, 9:50 a.m. UTC | #4 
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
Jason Wang Jan. 29, 2015, 10:15 a.m. UTC | #5 
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>
Stefan Hajnoczi Feb. 6, 2015, 1:54 p.m. UTC | #6 
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
Paolo Bonzini Feb. 6, 2015, 2:46 p.m. UTC | #7 
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
Stefan Hajnoczi Feb. 6, 2015, 4:51 p.m. UTC | #8 
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
diff mbox

Patch

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;
     }