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

[Claudio Fontana]

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'd say:

- show_netdevs
_ get nic models
- show nic models

instead of:

- get nic models
- show netdevs
- show nic models


 > and as long as the declaration 
> of the variable has to stay at the top of the block (according to QEMU's 
> coding style), I think I'd also prefer to keep the initialization there.
> 

This conflict can easily be solved by putting the nic_models on its own line,
as I have shown in my hunk above.

Ciao,

Claudio