Re: [PATCH 2/3] net: Restore printing of the help text with "-nic help"

[Claudio Fontana]

On 11/10/22 13:23, Thomas Huth wrote:
> On 10/11/2022 13.05, Claudio Fontana wrote:
>> On 11/10/22 12:42, Thomas Huth wrote:
>>> On 08/11/2022 10.49, Claudio Fontana wrote:
>>>> On 11/4/22 13:57, Thomas Huth wrote:
>>>>> Running QEMU with "-nic help" used to work in QEMU 5.2 and earlier 
>>>>> versions
>>>>> (it showed the available netdev backends), but this feature got broken 
>>>>> during
>>>>> some refactoring in version 6.0. Let's restore the old behavior, and while
>>>>> we're at it, let's also print the available NIC models here now since this
>>>>> option can be used to configure both, netdev backend and model in one go.
>>>>>
>>>>> Fixes: ad6f932fe8 ("net: do not exit on "netdev_add help" monitor 
>>>>> command")
>>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>>> ---
>>>>>    net/net.c | 14 ++++++++++++--
>>>>>    1 file changed, 12 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/net/net.c b/net/net.c
>>>>> index c0516a8067..b4b8f2a9cc 100644
>>>>> --- a/net/net.c
>>>>> +++ b/net/net.c
>>>>> @@ -1571,8 +1571,18 @@ static int net_param_nic(void *dummy, QemuOpts 
>>>>> *opts, Error **errp)
>>>>>        const char *type;
>>>>>    
>>>>>        type = qemu_opt_get(opts, "type");
>>>>> -    if (type && g_str_equal(type, "none")) {
>>>>> -        return 0;    /* Nothing to do, default_net is cleared in vl.c */
>>>>> +    if (type) {
>>>>> +        if (g_str_equal(type, "none")) {
>>>>> +            return 0;    /* Nothing to do, default_net is cleared in 
>>>>> vl.c */
>>>>> +        }
>>>>> +        if (is_help_option(type)) {
>>>>> +            GPtrArray *nic_models = qemu_get_nic_models(TYPE_DEVICE);
>>>>> +            show_netdevs();
>>>>> +            printf("\n");
>>>>> +            qemu_show_nic_models(type, (const char **)nic_models->pdata);
>>>>> +            g_ptr_array_free(nic_models, true);
>>>>
>>>> nit: would not the order:
>>>>
>>>>> +            GPtrArray *nic_models;
>>>>> +            show_netdevs();
>>>>> +            printf("\n");
>>>>> +            nic_models = qemu_get_nic_models(TYPE_DEVICE);
>>>>> +            qemu_show_nic_models(type, (const char **)nic_models->pdata);
>>>>> +            g_ptr_array_free(nic_models, true);
>>>>
>>>> flow more logically?
>>>
>>> I think that's mostly a matter of taste ...
>>
>> To some extent, but for the reader it would make more sense not to intermix 
>> unrelated code?
> 
> I'm pretty sure that as soon as I change it, another reviewer
> shows up and asks me to put everything into one line again
> since they prefer more compact code ;-)
> 
>> I'd say:
>>
>> - show_netdevs
>> _ get nic models
>> - show nic models
>>
>> instead of:
>>
>> - get nic models
>> - show netdevs
>> - show nic models
> 
> I get your point, and I would immediately agree with you if we
> were allowed to do:
> 
>          show_netdevs();
>          printf("\n");
>          GPtrArray *nic_models = qemu_get_nic_models(TYPE_DEVICE);
>          qemu_show_nic_models(type, (const char **)nic_models->pdata);
>          g_ptr_array_free(nic_models, true);
> 
> Although this is possible nowadays (since we're using
> not C89 anymore), it's against the QEMU coding style.

It seems you haven't seen the next message I sent. You can write exactly the 
code above,
with valid C89 syntax,

which is:

show_netdevs();
printf("\n");
{
   GPtrArray *nic_models = qemu_get_nic_models(TYPE_DEVICE);
   qemu_show_nic_models(type, (const char **)nic_models->pdata);
   g_ptr_array_free(nic_models, true);
}

> 
> So it's a trade-off now - use two lines of code and have some more
> chronological code flow, or use one line of more compact code.
> 
> I'm in favor of the more compact code. So please let's stop
> bike-shedding now.
> 
>   Thanks,
>    Thomas
> 

Ah, you don't like it, I see. Well.

Thanks,

Claudio