[Qemu-devel] [PATCH 0/2] netdev fixes

[Qemu-devel] [PATCH 0/2] netdev fixes

[Markus Armbruster]

Markus Armbruster (2):
  Fix automatically assigned network names for netdev
  Fix netdev name lookup in -device, device_add, netdev_del

 net.c |   19 +++++++++++++++----
 1 files changed, 15 insertions(+), 4 deletions(-)

-- 
1.7.2.3

[Qemu-devel] [PATCH 1/2] Fix automatically assigned network names for netdev

[Markus Armbruster]

If a network client doesn't have a name, we make one up, with
assign_name().  assign_name() creates a name MODEL.NUM, where MODEL is
the client's model, and NUM is the number of MODELs that already
exist.

Bug: it misses clients that are not on a VLAN, i.e. netdevs and the
NICs using them:

    $ qemu-system-x86_64 -nodefaults -vnc :0 -S -monitor stdio -netdev user,id=hostnet0 -net nic,netdev=hostnet0 -netdev user,id=hostnet1 -net nic,netdev=hostnet1
    QEMU 0.14.50 monitor - type 'help' for more information
    (qemu) info network
    Devices not on any VLAN:
      hostnet0: net=10.0.2.0, restricted=n peer=e1000.0
      hostnet1: net=10.0.2.0, restricted=n peer=e1000.0
      e1000.0: model=e1000,macaddr=52:54:00:12:34:56 peer=hostnet0
      e1000.0: model=e1000,macaddr=52:54:00:12:34:57 peer=hostnet1

Fix that.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 net.c |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/net.c b/net.c
index 4f777c3..8754627 100644
--- a/net.c
+++ b/net.c
@@ -150,12 +150,11 @@ void qemu_macaddr_default_if_unset(MACAddr *macaddr)
 static char *assign_name(VLANClientState *vc1, const char *model)
 {
     VLANState *vlan;
+    VLANClientState *vc;
     char buf[256];
     int id = 0;
 
     QTAILQ_FOREACH(vlan, &vlans, next) {
-        VLANClientState *vc;
-
         QTAILQ_FOREACH(vc, &vlan->clients, next) {
             if (vc != vc1 && strcmp(vc->model, model) == 0) {
                 id++;
@@ -163,6 +162,12 @@ static char *assign_name(VLANClientState *vc1, const char *model)
         }
     }
 
+    QTAILQ_FOREACH(vc, &non_vlan_clients, next) {
+        if (vc != vc1 && strcmp(vc->model, model) == 0) {
+            id++;
+        }
+    }
+
     snprintf(buf, sizeof(buf), "%s.%d", model, id);
 
     return qemu_strdup(buf);
-- 
1.7.2.3

[Qemu-devel] [PATCH 2/2] Fix netdev name lookup in -device, device_add, netdev_del

[Markus Armbruster]

qemu_find_netdev() looks up members of non_vlan_clients by name.  It
happily returns the first match.  Trouble is the names need not be
unique.

non_vlan_clients contains host parts (netdevs) and guest parts (NICs).

Netdevs have unique names: a netdev's name is a (mandatory)
qemu_netdev_opts ID, and these are unique.

NIC names are not unique.  If a NIC has a qdev ID (which is unique),
that's its name.  Else, we make up a name.  The made-up names are
unique, but they can clash with qdev IDs.  Even if NICs had unique
names, they could still clash with netdev names.

Callers of qemu_find_netdev():

* net_init_nic() wants a netdev.  It happens to work because it runs
  before NICs get added to non_vlan_clients.

* do_netdev_del() wants a netdev.  If it gets a NIC, it complains and
  fails.  Bug: a netdev with the same name that comes later in
  non_vlan_clients can't be deleted:

    $ qemu-system-x86_64 -nodefaults -vnc :0 -S -monitor stdio -netdev user,id=hostnet0 -device virtio-net-pci,netdev=hostnet0,id=virtio1
    [...]
    (qemu) netdev_add user,id=virtio1
    (qemu) info network
    Devices not on any VLAN:
      hostnet0: net=10.0.2.0, restricted=n peer=virtio1
      virtio1: model=virtio-net-pci,macaddr=52:54:00:12:34:56 peer=hostnet0
      virtio1: net=10.0.2.0, restricted=n
    (qemu) netdev_del virtio1
    Device 'virtio1' not found

* parse_netdev() wants a netdev.  If it gets a NIC, it gets confused.
  With the test setup above:

    (qemu) device_add virtio-net-pci,netdev=virtio1
    Property 'virtio-net-pci.netdev' can't take value 'virtio1', it's in use

  You can even connect two NICs to each other:

    $ qemu-system-x86_64 -nodefaults -vnc :0 -S -monitor stdio -device virtio-net-pci,id=virtio1 -device e1000,netdev=virtio1
    [...]
    Devices not on any VLAN:
      virtio1: model=virtio-net-pci,macaddr=52:54:00:12:34:56 peer=e1000.0
      e1000.0: model=e1000,macaddr=52:54:00:12:34:57 peer=virtio1
    (qemu) q
    Segmentation fault (core dumped)

* do_set_link() works fine for both netdevs and NICs.  Whether it
  really makes sense for netdevs is debatable, but that's outside this
  patch's scope.

Change qemu_find_netdev() to return only netdevs.  This fixes the
netdev_del and device_add/-device bugs demonstrated above.

To avoid changing set_link, make do_set_link() search non_vlan_clients
by hand instead of calling qemu_find_netdev().

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 net.c |   10 ++++++++--
 1 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/net.c b/net.c
index 8754627..c5c4b6b 100644
--- a/net.c
+++ b/net.c
@@ -658,6 +658,8 @@ VLANClientState *qemu_find_netdev(const char *id)
     VLANClientState *vc;
 
     QTAILQ_FOREACH(vc, &non_vlan_clients, next) {
+        if (vc->info->type == NET_CLIENT_TYPE_NIC)
+            continue;
         if (!strcmp(vc->name, id)) {
             return vc;
         }
@@ -1217,7 +1219,7 @@ int do_netdev_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
     VLANClientState *vc;
 
     vc = qemu_find_netdev(id);
-    if (!vc || vc->info->type == NET_CLIENT_TYPE_NIC) {
+    if (!vc) {
         qerror_report(QERR_DEVICE_NOT_FOUND, id);
         return -1;
     }
@@ -1262,7 +1264,11 @@ int do_set_link(Monitor *mon, const QDict *qdict, QObject **ret_data)
             }
         }
     }
-    vc = qemu_find_netdev(name);
+    QTAILQ_FOREACH(vc, &non_vlan_clients, next) {
+        if (!strcmp(vc->name, name)) {
+            goto done;
+        }
+    }
 done:
 
     if (!vc) {
-- 
1.7.2.3

Re: [Qemu-devel] [PATCH 0/2] netdev fixes

[Markus Armbruster]

Ping?

Re: [Qemu-devel] [PATCH 0/2] netdev fixes

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> Ping?

Could someone please take pity and review or commit this?  Thanks.

Re: [Qemu-devel] [PATCH 0/2] netdev fixes

[Michael S. Tsirkin]

On Thu, Jun 16, 2011 at 06:45:35PM +0200, Markus Armbruster wrote:
> Markus Armbruster (2):
>   Fix automatically assigned network names for netdev
>   Fix netdev name lookup in -device, device_add, netdev_del
> 
>  net.c |   19 +++++++++++++++----
>  1 files changed, 15 insertions(+), 4 deletions(-)

Thanks, applied.
I think going forward we should bring more order into ways we assign
IDs.

> -- 
> 1.7.2.3
> 

Re: [Qemu-devel] [PATCH 0/2] netdev fixes

[Jan Kiszka]

On 2011-07-19 14:10, Michael S. Tsirkin wrote:
> On Thu, Jun 16, 2011 at 06:45:35PM +0200, Markus Armbruster wrote:
>> Markus Armbruster (2):
>>   Fix automatically assigned network names for netdev
>>   Fix netdev name lookup in -device, device_add, netdev_del
>>
>>  net.c |   19 +++++++++++++++----
>>  1 files changed, 15 insertions(+), 4 deletions(-)
> 
> Thanks, applied.
> I think going forward we should bring more order into ways we assign
> IDs.

Did you take over net subsystem integration? Or who should initially
receive related patches now?

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH 0/2] netdev fixes

[Michael S. Tsirkin]

On Tue, Jul 19, 2011 at 07:52:39PM +0200, Jan Kiszka wrote:
> On 2011-07-19 14:10, Michael S. Tsirkin wrote:
> > On Thu, Jun 16, 2011 at 06:45:35PM +0200, Markus Armbruster wrote:
> >> Markus Armbruster (2):
> >>   Fix automatically assigned network names for netdev
> >>   Fix netdev name lookup in -device, device_add, netdev_del
> >>
> >>  net.c |   19 +++++++++++++++----
> >>  1 files changed, 15 insertions(+), 4 deletions(-)
> > 
> > Thanks, applied.
> > I think going forward we should bring more order into ways we assign
> > IDs.
> 
> Did you take over net subsystem integration?

No, Markus just asked me to look at his patches, and I took interest.
I applied the patches to my tree, from there pull requests go to Anthony
who is a maintainer for net/ so I think that's all right.

Also, it's just my tree, pull request won't go out before
Thursday at the earliest, so if someone objects
(or wants to merge them himself), there's plenty of time.

> Or who should initially receive related patches now?
> 
> Jan

I think running ./scripts/get_maintainer.pl is generally
a good idea. For net you'd get:

[qemu]$ ./scripts/get_maintainer.pl -f net.c net/
Anthony Liguori <aliguori@us.ibm.com> (maintainer:Network device layer,commit_signer:4/16=25%)
Mark McLoughlin <markmc@redhat.com> (maintainer:Network device layer)
Aurelien Jarno <aurelien@aurel32.net> (commit_signer:6/16=38%)
"Michael S. Tsirkin" <mst@redhat.com> (commit_signer:5/16=31%)
Jason Wang <jasowang@redhat.com> (commit_signer:4/16=25%)
Peter Maydell <peter.maydell@linaro.org> (commit_signer:4/16=25%)


> -- 
> Siemens AG, Corporate Technology, CT T DE IT 1
> Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH 0/2] netdev fixes

[Mark McLoughlin]

On Wed, 2011-07-20 at 11:19 +0300, Michael S. Tsirkin wrote:
> On Tue, Jul 19, 2011 at 07:52:39PM +0200, Jan Kiszka wrote:
> > On 2011-07-19 14:10, Michael S. Tsirkin wrote:
> > > On Thu, Jun 16, 2011 at 06:45:35PM +0200, Markus Armbruster wrote:
> > >> Markus Armbruster (2):
> > >>   Fix automatically assigned network names for netdev
> > >>   Fix netdev name lookup in -device, device_add, netdev_del
> > >>
> > >>  net.c |   19 +++++++++++++++----
> > >>  1 files changed, 15 insertions(+), 4 deletions(-)
> > > 
> > > Thanks, applied.
> > > I think going forward we should bring more order into ways we assign
> > > IDs.
> > 
> > Did you take over net subsystem integration?
> 
> No, Markus just asked me to look at his patches, and I took interest.
> I applied the patches to my tree, from there pull requests go to Anthony
> who is a maintainer for net/ so I think that's all right.
> 
> Also, it's just my tree, pull request won't go out before
> Thursday at the earliest, so if someone objects
> (or wants to merge them himself), there's plenty of time.
> 
> > Or who should initially receive related patches now?
> > 
> > Jan
> 
> I think running ./scripts/get_maintainer.pl is generally
> a good idea. For net you'd get:
> 
> [qemu]$ ./scripts/get_maintainer.pl -f net.c net/
> Anthony Liguori <aliguori@us.ibm.com> (maintainer:Network device layer,commit_signer:4/16=25%)
> Mark McLoughlin <markmc@redhat.com> (maintainer:Network device layer)

This is clearly out of date - feel free to remove it

Cheers,
Mark.