[Qemu-devel] [PATCH 0/3] net: reimplement -net nic diagnostic fix

[Qemu-devel] [PATCH 0/3] net: reimplement -net nic diagnostic fix

[Peter Maydell]

This patchset reverts commit f68b9d672, which was triggering
spuriously for NICs created via -device rather than -net nic.
It then reimplements the improved diagnostics with a different
approach which only applies to '-net nic'. (It's only -net nic
devices that can be ignored (ie not instantiated); -device
nic devices are always instantiated. Checking for -device user
errors like "this device wasn't plugged into any bus" is
a separate issue not addressed here.)

Apologies for the regression.

Peter Maydell (3):
  Revert "net: Improve the warnings for dubious command line option
    combinations"
  net: Don't warn about the default network setup
  net: Warn about "-net nic" options which were ignored

 hw/qdev.c |    1 +
 net.c     |   42 +++++++++++++++++++-----------------------
 net.h     |    3 ++-
 3 files changed, 22 insertions(+), 24 deletions(-)

[Qemu-devel] [PATCH 1/3] Revert "net: Improve the warnings for dubious command line option combinations"

[Peter Maydell]

This reverts commit f68b9d672b90dedc79aeb9b44607f484dbe46a6b.
That attempt at diagnosing unused -net nic options failed to account
for NICs created via -device; back it out cleanly in preparation
for implementing in a different manner.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 net.c |   31 -------------------------------
 1 files changed, 0 insertions(+), 31 deletions(-)

diff --git a/net.c b/net.c
index 4f777c3..1b521a9 100644
--- a/net.c
+++ b/net.c
@@ -1304,22 +1304,6 @@ void net_check_clients(void)
 {
     VLANState *vlan;
     VLANClientState *vc;
-    int seen_nics = 0;
-
-    /* Don't warn about the default network setup that you get if
-     * no command line -net options are specified. There are two
-     * cases that we would otherwise complain about:
-     * (1) board doesn't support a NIC but the implicit "-net nic"
-     * requested one; we'd otherwise complain about more NICs being
-     * specified than we support, and also that the vlan set up by
-     * the implicit "-net user" didn't have any NICs connected to it
-     * (2) CONFIG_SLIRP not set: we'd otherwise complain about the
-     * implicit "-net nic" setting up a nic that wasn't connected to
-     * anything.
-     */
-    if (default_net) {
-        return;
-    }
 
     QTAILQ_FOREACH(vlan, &vlans, next) {
         int has_nic = 0, has_host_dev = 0;
@@ -1327,7 +1311,6 @@ void net_check_clients(void)
         QTAILQ_FOREACH(vc, &vlan->clients, next) {
             switch (vc->info->type) {
             case NET_CLIENT_TYPE_NIC:
-                seen_nics++;
                 has_nic = 1;
                 break;
             case NET_CLIENT_TYPE_SLIRP:
@@ -1347,26 +1330,12 @@ void net_check_clients(void)
                     vlan->id);
     }
     QTAILQ_FOREACH(vc, &non_vlan_clients, next) {
-        if (vc->info->type == NET_CLIENT_TYPE_NIC) {
-            seen_nics++;
-        }
         if (!vc->peer) {
             fprintf(stderr, "Warning: %s %s has no peer\n",
                     vc->info->type == NET_CLIENT_TYPE_NIC ? "nic" : "netdev",
                     vc->name);
         }
     }
-    if (seen_nics != nb_nics) {
-        /* Number of NICs requested by user on command line doesn't match
-         * the number the model actually registered with us.
-         * This will generally only happen for models of embedded boards
-         * with no PCI bus or similar. PCI based machines can instantiate
-         * all requested NICs as PCI devices but usually embedded boards
-         * only have a single NIC.
-         */
-        fprintf(stderr, "Warning: more nics requested than this machine "
-                "supports; some have been ignored\n");
-    }
 }
 
 static int net_init_client(QemuOpts *opts, void *dummy)
-- 
1.7.1

[Qemu-devel] [PATCH 2/3] net: Don't warn about the default network setup

[Peter Maydell]

Don't warn about the default network setup that you get if no command line
-net options are specified.  There are two cases that we would otherwise
complain about:

(1) board doesn't support a NIC but the implicit "-net nic" requested one
(2) CONFIG_SLIRP not set, so the implicit "-net nic" sets up a nic that
isn't connected to anything

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 net.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/net.c b/net.c
index 1b521a9..68c2840 100644
--- a/net.c
+++ b/net.c
@@ -1305,6 +1305,18 @@ void net_check_clients(void)
     VLANState *vlan;
     VLANClientState *vc;
 
+    /* Don't warn about the default network setup that you get if
+     * no command line -net or -netdev options are specified. There
+     * are two cases that we would otherwise complain about:
+     * (1) board doesn't support a NIC but the implicit "-net nic"
+     * requested one
+     * (2) CONFIG_SLIRP not set, in which case the implicit "-net nic"
+     * sets up a nic that isn't connected to anything.
+     */
+    if (default_net) {
+        return;
+    }
+
     QTAILQ_FOREACH(vlan, &vlans, next) {
         int has_nic = 0, has_host_dev = 0;
 
-- 
1.7.1

[Qemu-devel] [PATCH 3/3] net: Warn about "-net nic" options which were ignored

[Peter Maydell]

Diagnose the case where the user asked for a NIC via "-net nic"
but the board didn't instantiate that NIC (for example where the
user asked for two NICs but the board only supports one). Note
that this diagnostic doesn't apply to NICs created through -device,
because those are always instantiated.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/qdev.c |    1 +
 net.c     |   15 +++++++++++++++
 net.h     |    3 ++-
 3 files changed, 18 insertions(+), 1 deletions(-)

diff --git a/hw/qdev.c b/hw/qdev.c
index 9519f5d..2987901 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -468,6 +468,7 @@ void qdev_set_nic_properties(DeviceState *dev, NICInfo *nd)
         qdev_prop_exists(dev, "vectors")) {
         qdev_prop_set_uint32(dev, "vectors", nd->nvectors);
     }
+    nd->instantiated = 1;
 }
 
 BusState *qdev_get_child_bus(DeviceState *dev, const char *name)
diff --git a/net.c b/net.c
index 68c2840..a104976 100644
--- a/net.c
+++ b/net.c
@@ -1304,6 +1304,7 @@ void net_check_clients(void)
 {
     VLANState *vlan;
     VLANClientState *vc;
+    int i;
 
     /* Don't warn about the default network setup that you get if
      * no command line -net or -netdev options are specified. There
@@ -1348,6 +1349,20 @@ void net_check_clients(void)
                     vc->name);
         }
     }
+
+    /* Check that all NICs requested via -net nic actually got created.
+     * NICs created via -device don't need to be checked here because
+     * they are always instantiated.
+     */
+    for (i = 0; i < MAX_NICS; i++) {
+        NICInfo *nd = &nd_table[i];
+        if (nd->used && !nd->instantiated) {
+            fprintf(stderr, "Warning: requested NIC (%s, model %s) "
+                    "was not created (not supported by this machine?)\n",
+                    nd->name ? nd->name : "anonymous",
+                    nd->model ? nd->model : "unspecified");
+        }
+    }
 }
 
 static int net_init_client(QemuOpts *opts, void *dummy)
diff --git a/net.h b/net.h
index 6ceca50..5b883a9 100644
--- a/net.h
+++ b/net.h
@@ -133,7 +133,8 @@ struct NICInfo {
     char *devaddr;
     VLANState *vlan;
     VLANClientState *netdev;
-    int used;
+    int used;         /* is this slot in nd_table[] being used? */
+    int instantiated; /* does this NICInfo correspond to an instantiated NIC? */
     int nvectors;
 };
 
-- 
1.7.1

Re: [Qemu-devel] [PATCH 0/3] net: reimplement -net nic diagnostic fix

[Jan Kiszka]

On 2011-05-20 17:49, Peter Maydell wrote:
> This patchset reverts commit f68b9d672, which was triggering
> spuriously for NICs created via -device rather than -net nic.
> It then reimplements the improved diagnostics with a different
> approach which only applies to '-net nic'. (It's only -net nic
> devices that can be ignored (ie not instantiated); -device
> nic devices are always instantiated. Checking for -device user
> errors like "this device wasn't plugged into any bus" is
> a separate issue not addressed here.)

But we still have the problem that qemu_new_nic does not call
net_init_nic. That means e.g. that NICs instantiated via -device don't
participate in automatic MAC assignment. Maybe fixing that makes this
series obsolete.

Jan

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

Re: [Qemu-devel] [PATCH 0/3] net: reimplement -net nic diagnostic fix

[Peter Maydell]

On 20 May 2011 16:55, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> On 2011-05-20 17:49, Peter Maydell wrote:
>> This patchset reverts commit f68b9d672, which was triggering
>> spuriously for NICs created via -device rather than -net nic.
>> It then reimplements the improved diagnostics with a different
>> approach which only applies to '-net nic'. (It's only -net nic
>> devices that can be ignored (ie not instantiated); -device
>> nic devices are always instantiated. Checking for -device user
>> errors like "this device wasn't plugged into any bus" is
>> a separate issue not addressed here.)
>
> But we still have the problem that qemu_new_nic does not call
> net_init_nic. That means e.g. that NICs instantiated via -device don't
> participate in automatic MAC assignment. Maybe fixing that makes this
> series obsolete.

I think that avoiding MAC address clashes should be done by
having net_init_nic() call qemu_macaddr_default_if_unset()
rather than doing its own MAC assignment.

Having qemu_new_nic() call net_init_nic() is a bit tricky
because (a) as you said earlier you need to avoid doing
it for NICs which were created from nd_table entries in the
first place and (b) it imposes a new limit on the number
of NICs you can create via -device which wasn't there before
(because nd_table is fixed size).

I think I agree with Paul Brook's comment on IRC that the
real long term solution is for nd_table[] to go away completely,
(ie not for the new qdev option code to add entries in nd_table[]
by calling net_init_nic().)

-- PMM

Re: [Qemu-devel] [PATCH 0/3] net: reimplement -net nic diagnostic fix

[Jan Kiszka]

On 2011-05-20 18:04, Peter Maydell wrote:
> On 20 May 2011 16:55, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>> On 2011-05-20 17:49, Peter Maydell wrote:
>>> This patchset reverts commit f68b9d672, which was triggering
>>> spuriously for NICs created via -device rather than -net nic.
>>> It then reimplements the improved diagnostics with a different
>>> approach which only applies to '-net nic'. (It's only -net nic
>>> devices that can be ignored (ie not instantiated); -device
>>> nic devices are always instantiated. Checking for -device user
>>> errors like "this device wasn't plugged into any bus" is
>>> a separate issue not addressed here.)
>>
>> But we still have the problem that qemu_new_nic does not call
>> net_init_nic. That means e.g. that NICs instantiated via -device don't
>> participate in automatic MAC assignment. Maybe fixing that makes this
>> series obsolete.
> 
> I think that avoiding MAC address clashes should be done by
> having net_init_nic() call qemu_macaddr_default_if_unset()

You mean qemu_new_nic? Otherwise it wouldn't help. Will have a look to
move that over.

> rather than doing its own MAC assignment.
> 
> Having qemu_new_nic() call net_init_nic() is a bit tricky
> because (a) as you said earlier you need to avoid doing
> it for NICs which were created from nd_table entries in the
> first place and (b) it imposes a new limit on the number
> of NICs you can create via -device which wasn't there before
> (because nd_table is fixed size).
> 
> I think I agree with Paul Brook's comment on IRC that the
> real long term solution is for nd_table[] to go away completely,
> (ie not for the new qdev option code to add entries in nd_table[]
> by calling net_init_nic().)

I don't disagree that nd_table is better removed. Then let's address all
those issues separately.

Jan

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

Re: [Qemu-devel] [PATCH 0/3] net: reimplement -net nic diagnostic fix

[Peter Maydell]

On 20 May 2011 17:07, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> On 2011-05-20 18:04, Peter Maydell wrote:
>> I think that avoiding MAC address clashes should be done by
>> having net_init_nic() call qemu_macaddr_default_if_unset()
>
> You mean qemu_new_nic? Otherwise it wouldn't help. Will have a look to
> move that over.

No, I mean net_init_nic(). At the moment there are two bits of
code which generate a fresh MAC address:
 1. qemu_macaddr_default_if_unset() -- init routines for qdev network
  devices call this
 2. net_init_nic() -- devices created via -net nic get their MAC
  address assigned here; if the underlying device is qdev then
  by the time the qdev init routine is called the macaddr is
  already set so it isn't overridden

So if the code in 2. calls qemu_macaddr_default_if_unset instead then
we only have one function with a static variable which doles out
autogenerated MAC addresses.

-- PMM

Re: [Qemu-devel] [PATCH 0/3] net: reimplement -net nic diagnostic fix

[Jan Kiszka]

On 2011-05-20 18:12, Peter Maydell wrote:
> On 20 May 2011 17:07, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>> On 2011-05-20 18:04, Peter Maydell wrote:
>>> I think that avoiding MAC address clashes should be done by
>>> having net_init_nic() call qemu_macaddr_default_if_unset()
>>
>> You mean qemu_new_nic? Otherwise it wouldn't help. Will have a look to
>> move that over.
> 
> No, I mean net_init_nic(). At the moment there are two bits of
> code which generate a fresh MAC address:
>  1. qemu_macaddr_default_if_unset() -- init routines for qdev network
>   devices call this
>  2. net_init_nic() -- devices created via -net nic get their MAC
>   address assigned here; if the underlying device is qdev then
>   by the time the qdev init routine is called the macaddr is
>   already set so it isn't overridden
> 
> So if the code in 2. calls qemu_macaddr_default_if_unset instead then
> we only have one function with a static variable which doles out
> autogenerated MAC addresses.

Ah, I see. I thought qdev would always assign the same MAC. Then it's fine.

Jan

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

Re: [Qemu-devel] [PATCH 2/3] net: Don't warn about the default network setup

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> Don't warn about the default network setup that you get if no command line
> -net options are specified.  There are two cases that we would otherwise
> complain about:
>
> (1) board doesn't support a NIC but the implicit "-net nic" requested one

Maybe such board should declare no_nic, similar to no_parallel &
friends.  But that's out of this patch's scope.

This complaint is only added in the next patch, isn't it?  I don't mind.

> (2) CONFIG_SLIRP not set, so the implicit "-net nic" sets up a nic that
> isn't connected to anything

This one's already there, I think.

Re: [Qemu-devel] [PATCH 3/3] net: Warn about "-net nic" options which were ignored

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> Diagnose the case where the user asked for a NIC via "-net nic"
> but the board didn't instantiate that NIC (for example where the
> user asked for two NICs but the board only supports one). Note
> that this diagnostic doesn't apply to NICs created through -device,
> because those are always instantiated.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/qdev.c |    1 +
>  net.c     |   15 +++++++++++++++
>  net.h     |    3 ++-
>  3 files changed, 18 insertions(+), 1 deletions(-)
>
> diff --git a/hw/qdev.c b/hw/qdev.c
> index 9519f5d..2987901 100644
> --- a/hw/qdev.c
> +++ b/hw/qdev.c
> @@ -468,6 +468,7 @@ void qdev_set_nic_properties(DeviceState *dev, NICInfo *nd)
>          qdev_prop_exists(dev, "vectors")) {
>          qdev_prop_set_uint32(dev, "vectors", nd->nvectors);
>      }
> +    nd->instantiated = 1;
>  }
>  
>  BusState *qdev_get_child_bus(DeviceState *dev, const char *name)
> diff --git a/net.c b/net.c
> index 68c2840..a104976 100644
> --- a/net.c
> +++ b/net.c
> @@ -1304,6 +1304,7 @@ void net_check_clients(void)
>  {
>      VLANState *vlan;
>      VLANClientState *vc;
> +    int i;
>  
>      /* Don't warn about the default network setup that you get if
>       * no command line -net or -netdev options are specified. There
> @@ -1348,6 +1349,20 @@ void net_check_clients(void)
>                      vc->name);
>          }
>      }
> +
> +    /* Check that all NICs requested via -net nic actually got created.
> +     * NICs created via -device don't need to be checked here because
> +     * they are always instantiated.
> +     */
> +    for (i = 0; i < MAX_NICS; i++) {
> +        NICInfo *nd = &nd_table[i];
> +        if (nd->used && !nd->instantiated) {
> +            fprintf(stderr, "Warning: requested NIC (%s, model %s) "
> +                    "was not created (not supported by this machine?)\n",
> +                    nd->name ? nd->name : "anonymous",
> +                    nd->model ? nd->model : "unspecified");
> +        }
> +    }

Nicer message would be possible if NICInfo had a Location for
error_print_loc().  Out of this patch's scope.

>  }
>  
>  static int net_init_client(QemuOpts *opts, void *dummy)
> diff --git a/net.h b/net.h
> index 6ceca50..5b883a9 100644
> --- a/net.h
> +++ b/net.h
> @@ -133,7 +133,8 @@ struct NICInfo {
>      char *devaddr;
>      VLANState *vlan;
>      VLANClientState *netdev;
> -    int used;
> +    int used;         /* is this slot in nd_table[] being used? */
> +    int instantiated; /* does this NICInfo correspond to an instantiated NIC? */
>      int nvectors;
>  };

Re: [Qemu-devel] [PATCH 0/3] net: reimplement -net nic diagnostic fix

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> This patchset reverts commit f68b9d672, which was triggering
> spuriously for NICs created via -device rather than -net nic.
> It then reimplements the improved diagnostics with a different
> approach which only applies to '-net nic'. (It's only -net nic
> devices that can be ignored (ie not instantiated); -device
> nic devices are always instantiated. Checking for -device user
> errors like "this device wasn't plugged into any bus" is
> a separate issue not addressed here.)
>
> Apologies for the regression.

Looks good to me.

Re: [Qemu-devel] [PATCH 2/3] net: Don't warn about the default network setup

[Peter Maydell]

On 23 May 2011 09:30, Markus Armbruster <armbru@redhat.com> wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
>
>> Don't warn about the default network setup that you get if no command line
>> -net options are specified.  There are two cases that we would otherwise
>> complain about:
>>
>> (1) board doesn't support a NIC but the implicit "-net nic" requested one
>
> Maybe such board should declare no_nic, similar to no_parallel &
> friends.  But that's out of this patch's scope.

I think in the brave new qdev world this kind of thing should be
handled generically by warning about any -device requested devices
which couldn't be plugged into anything sensible.

> This complaint is only added in the next patch, isn't it?  I don't mind.

It won't explicitly say "you asked for a NIC and didn't get one",
but it will make the obscurer complaint "Warning: vlan 0 with no
nics", because the implicit '-net user' is then not connected to
anything. I was deliberately slightly vague with the comment text
so it would be true both before and after the following patch :-)

>> (2) CONFIG_SLIRP not set, so the implicit "-net nic" sets up a nic that
>> isn't connected to anything
>
> This one's already there, I think.

No, without this patch qemu will complain
"Warning: vlan 0 is not connected to host network" if you configured
with --disable-slirp.

(--disable-slirp seems to have been left out of configure --help;
I assume that's an oversight.)

-- PMM

Re: [Qemu-devel] [PATCH 0/3] net: reimplement -net nic diagnostic fix

[Peter Maydell]

Combination 'ping' and 'oops I forgot to cc the net/ maintainers'...

-- PMM

On 20 May 2011 16:49, Peter Maydell <peter.maydell@linaro.org> wrote:
> This patchset reverts commit f68b9d672, which was triggering
> spuriously for NICs created via -device rather than -net nic.
> It then reimplements the improved diagnostics with a different
> approach which only applies to '-net nic'. (It's only -net nic
> devices that can be ignored (ie not instantiated); -device
> nic devices are always instantiated. Checking for -device user
> errors like "this device wasn't plugged into any bus" is
> a separate issue not addressed here.)
>
> Apologies for the regression.
>
> Peter Maydell (3):
>  Revert "net: Improve the warnings for dubious command line option
>    combinations"
>  net: Don't warn about the default network setup
>  net: Warn about "-net nic" options which were ignored
>
>  hw/qdev.c |    1 +
>  net.c     |   42 +++++++++++++++++++-----------------------
>  net.h     |    3 ++-
>  3 files changed, 22 insertions(+), 24 deletions(-)

Re: [Qemu-devel] [PATCH 0/3] net: reimplement -net nic diagnostic fix

[Peter Maydell]

Ping^2 ?

On 5 June 2011 14:12, Peter Maydell <peter.maydell@linaro.org> wrote:
> Combination 'ping' and 'oops I forgot to cc the net/ maintainers'...
>
> -- PMM
>
> On 20 May 2011 16:49, Peter Maydell <peter.maydell@linaro.org> wrote:
>> This patchset reverts commit f68b9d672, which was triggering
>> spuriously for NICs created via -device rather than -net nic.
>> It then reimplements the improved diagnostics with a different
>> approach which only applies to '-net nic'. (It's only -net nic
>> devices that can be ignored (ie not instantiated); -device
>> nic devices are always instantiated. Checking for -device user
>> errors like "this device wasn't plugged into any bus" is
>> a separate issue not addressed here.)
>>
>> Apologies for the regression.
>>
>> Peter Maydell (3):
>>  Revert "net: Improve the warnings for dubious command line option
>>    combinations"
>>  net: Don't warn about the default network setup
>>  net: Warn about "-net nic" options which were ignored
>>
>>  hw/qdev.c |    1 +
>>  net.c     |   42 +++++++++++++++++++-----------------------
>>  net.h     |    3 ++-
>>  3 files changed, 22 insertions(+), 24 deletions(-)

Re: [Qemu-devel] [PATCH 0/3] net: reimplement -net nic diagnostic fix

[Anthony Liguori]

On 05/20/2011 10:49 AM, Peter Maydell wrote:
> This patchset reverts commit f68b9d672, which was triggering
> spuriously for NICs created via -device rather than -net nic.
> It then reimplements the improved diagnostics with a different
> approach which only applies to '-net nic'. (It's only -net nic
> devices that can be ignored (ie not instantiated); -device
> nic devices are always instantiated. Checking for -device user
> errors like "this device wasn't plugged into any bus" is
> a separate issue not addressed here.)
>
> Apologies for the regression.

Applied all.  Thanks.

Regards,

Anthony Liguori

>
> Peter Maydell (3):
>    Revert "net: Improve the warnings for dubious command line option
>      combinations"
>    net: Don't warn about the default network setup
>    net: Warn about "-net nic" options which were ignored
>
>   hw/qdev.c |    1 +
>   net.c     |   42 +++++++++++++++++++-----------------------
>   net.h     |    3 ++-
>   3 files changed, 22 insertions(+), 24 deletions(-)
>
>
>