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

[Thomas Huth]

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.

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