[Qemu-devel] [PATCH] net: Silence 'has no peer' messages in testing mode

[Qemu-devel] [PATCH] net: Silence 'has no peer' messages in testing mode

[Thomas Huth]

When running qtests with -nodefaults, we are not interested in
these 'XYZ has no peer' messages.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 net/net.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/net/net.c b/net/net.c
index 29f8398..58bf85e 100644
--- a/net/net.c
+++ b/net/net.c
@@ -1427,12 +1427,13 @@ void net_check_clients(void)
 
     net_hub_check_clients();
 
-    QTAILQ_FOREACH(nc, &net_clients, next) {
-        if (!nc->peer) {
-            warn_report("%s %s has no peer",
-                        nc->info->type == NET_CLIENT_DRIVER_NIC
-                        ? "nic" : "netdev",
-                        nc->name);
+    if (!qtest_enabled() || nd_table[0].used) {
+        QTAILQ_FOREACH(nc, &net_clients, next) {
+            if (!nc->peer) {
+                warn_report("%s %s has no peer",
+                            nc->info->type == NET_CLIENT_DRIVER_NIC
+                            ? "nic" : "netdev", nc->name);
+            }
         }
     }
 
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH] net: Silence 'has no peer' messages in testing mode

[Markus Armbruster]

Thomas Huth <thuth@redhat.com> writes:

> When running qtests with -nodefaults, we are not interested in
> these 'XYZ has no peer' messages.
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  net/net.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/net/net.c b/net/net.c
> index 29f8398..58bf85e 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -1427,12 +1427,13 @@ void net_check_clients(void)
>  
>      net_hub_check_clients();
>  
> -    QTAILQ_FOREACH(nc, &net_clients, next) {
> -        if (!nc->peer) {
> -            warn_report("%s %s has no peer",
> -                        nc->info->type == NET_CLIENT_DRIVER_NIC
> -                        ? "nic" : "netdev",
> -                        nc->name);
> +    if (!qtest_enabled() || nd_table[0].used) {

I understand the !qtest_enabled part, but not the nd_table[0].used
part.  Can you explain?

> +        QTAILQ_FOREACH(nc, &net_clients, next) {
> +            if (!nc->peer) {
> +                warn_report("%s %s has no peer",
> +                            nc->info->type == NET_CLIENT_DRIVER_NIC
> +                            ? "nic" : "netdev", nc->name);
> +            }
>          }
>      }

Re: [Qemu-devel] [PATCH] net: Silence 'has no peer' messages in testing mode

[Thomas Huth]

On 03.05.2018 13:47, Markus Armbruster wrote:
> Thomas Huth <thuth@redhat.com> writes:
> 
>> When running qtests with -nodefaults, we are not interested in
>> these 'XYZ has no peer' messages.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>  net/net.c | 13 +++++++------
>>  1 file changed, 7 insertions(+), 6 deletions(-)
>>
>> diff --git a/net/net.c b/net/net.c
>> index 29f8398..58bf85e 100644
>> --- a/net/net.c
>> +++ b/net/net.c
>> @@ -1427,12 +1427,13 @@ void net_check_clients(void)
>>  
>>      net_hub_check_clients();
>>  
>> -    QTAILQ_FOREACH(nc, &net_clients, next) {
>> -        if (!nc->peer) {
>> -            warn_report("%s %s has no peer",
>> -                        nc->info->type == NET_CLIENT_DRIVER_NIC
>> -                        ? "nic" : "netdev",
>> -                        nc->name);
>> +    if (!qtest_enabled() || nd_table[0].used) {
> 
> I understand the !qtest_enabled part, but not the nd_table[0].used
> part.  Can you explain?

Sure: I want to silence the message in qtest mode with -nodefaults.
qtest mode enabled means qtest_enabled() returns true.
-nodefaults enabled means nd_table[0].used is set to false.
So silence the message if qtest_enabled() && !nd_table[0].used.
Negates to: Print the message if !qtest_enabled || nd_table[0].used.

>> +        QTAILQ_FOREACH(nc, &net_clients, next) {
>> +            if (!nc->peer) {
>> +                warn_report("%s %s has no peer",
>> +                            nc->info->type == NET_CLIENT_DRIVER_NIC
>> +                            ? "nic" : "netdev", nc->name);
>> +            }
>>          }
>>      }

 Thomas

Re: [Qemu-devel] [PATCH] net: Silence 'has no peer' messages in testing mode

[Markus Armbruster]

Thomas Huth <thuth@redhat.com> writes:

> On 03.05.2018 13:47, Markus Armbruster wrote:
>> Thomas Huth <thuth@redhat.com> writes:
>> 
>>> When running qtests with -nodefaults, we are not interested in
>>> these 'XYZ has no peer' messages.
>>>
>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>> ---
>>>  net/net.c | 13 +++++++------
>>>  1 file changed, 7 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/net/net.c b/net/net.c
>>> index 29f8398..58bf85e 100644
>>> --- a/net/net.c
>>> +++ b/net/net.c
>>> @@ -1427,12 +1427,13 @@ void net_check_clients(void)
>>>  
>>>      net_hub_check_clients();
>>>  
>>> -    QTAILQ_FOREACH(nc, &net_clients, next) {
>>> -        if (!nc->peer) {
>>> -            warn_report("%s %s has no peer",
>>> -                        nc->info->type == NET_CLIENT_DRIVER_NIC
>>> -                        ? "nic" : "netdev",
>>> -                        nc->name);
>>> +    if (!qtest_enabled() || nd_table[0].used) {
>> 
>> I understand the !qtest_enabled part, but not the nd_table[0].used
>> part.  Can you explain?
>
> Sure: I want to silence the message in qtest mode with -nodefaults.
> qtest mode enabled means qtest_enabled() returns true.
> -nodefaults enabled means nd_table[0].used is set to false.

.used is initialized to false, and becomes true on successful
net_init_nic() / net_param_nic().

The connection to -nodefaults is this:

    if (default_net) {
        QemuOptsList *net = qemu_find_opts("net");
        qemu_opts_set(net, NULL, "type", "nic", &error_abort);
#ifdef CONFIG_SLIRP
        qemu_opts_set(net, NULL, "type", "user", &error_abort);
#endif
    }

When default_net is true, we effectively add -net nic".  It is true
unless the user specifies any of -nodefaults, -netdev, -nic, -net.  So,
with the default nic, nd_table[0].used is true.  It's also true with
non-default nics specified with convenience options.

> So silence the message if qtest_enabled() && !nd_table[0].used.

Thus, silence the message if qtesting and the user specified -nodefaults
and didn't add nics with convenience options.

Two (possibly confused) questions:

1. The user can add nics without convenience options:

    $ upstream-qemu -display none -nodefaults -device e1000
    upstream-qemu: warning: nic e1000.0 has no peer

   Shouldn't we silence the warning then, too?

2. We already have code to silence the warning:

    /* 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) {
        net_check_clients();
    }

   Is it a good idea to split the logic between net_check_clients() and
   its caller?

> Negates to: Print the message if !qtest_enabled || nd_table[0].used.
>
>>> +        QTAILQ_FOREACH(nc, &net_clients, next) {
>>> +            if (!nc->peer) {
>>> +                warn_report("%s %s has no peer",
>>> +                            nc->info->type == NET_CLIENT_DRIVER_NIC
>>> +                            ? "nic" : "netdev", nc->name);
>>> +            }
>>>          }
>>>      }
>
>  Thomas

Re: [Qemu-devel] [PATCH] net: Silence 'has no peer' messages in testing mode

[Thomas Huth]

On 07.05.2018 09:14, Markus Armbruster wrote:
[...]
> Two (possibly confused) questions:
> 
> 1. The user can add nics without convenience options:
> 
>     $ upstream-qemu -display none -nodefaults -device e1000
>     upstream-qemu: warning: nic e1000.0 has no peer
> 
>    Shouldn't we silence the warning then, too?

No, since that is certainly a mis-configuration in that case. Why would
a user want to add a NIC without host backend?

> 2. We already have code to silence the warning:
> 
>     /* 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) {
>         net_check_clients();
>     }
> 
>    Is it a good idea to split the logic between net_check_clients() and
>    its caller?

Hmm, it's likely nicer to keep everything in one place. Since
"default_net" is only available in vl.c, I think the checks should go
there... so I'll rework my patch accordingly.

 Thomas

Re: [Qemu-devel] [PATCH] net: Silence 'has no peer' messages in testing mode

[Markus Armbruster]

Thomas Huth <thuth@redhat.com> writes:

> On 07.05.2018 09:14, Markus Armbruster wrote:
> [...]
>> Two (possibly confused) questions:
>> 
>> 1. The user can add nics without convenience options:
>> 
>>     $ upstream-qemu -display none -nodefaults -device e1000
>>     upstream-qemu: warning: nic e1000.0 has no peer
>> 
>>    Shouldn't we silence the warning then, too?
>
> No, since that is certainly a mis-configuration in that case. Why would
> a user want to add a NIC without host backend?

Related:

    $ upstream-qemu -display none -nodefaults -net nic
    upstream-qemu: warning: vlan 0 is not connected to host network

The root of my confusion: I'm having difficulties making the connection
from the guard predicate (!qtest_enabled() || nd_table[0].used) to
"stupid default setup that isn't the test's fault".

In what scenarios exactly do we get unwanted "has no peer" messages?
Mandatory onboard NIC, perhaps?

Arguably, onboard NICs should default to a null backend that behaves
like "no carrier".  That's exactly what physical hardware does.  Could
even be useful outside tests once we support hot-plugging network
backends (we might already, I have no idea), just like physical hardware
supports plugging in network cables.

[...]