Re: [RFC PATCH v3 03/11] qapi: net: introduce a way to bypass qemu_opts_parse_noisily()

[Laurent Vivier <lvivier@redhat.com>]

On 20/06/2022 14:43, Markus Armbruster wrote:
> Laurent Vivier <lvivier@redhat.com> writes:
> 
>> As qemu_opts_parse_noisily() flattens the QAPI structures ("type" field
>> of Netdev structure can collides with "type" field of SocketAddress),
>> we introduce a way to bypass qemu_opts_parse_noisily() and use directly
>> visit_type_Netdev() to parse the backend parameters.
>>
>> More details from Markus:
>>
>> qemu_init() passes the argument of -netdev, -nic, and -net to
>> net_client_parse().
>>
>> net_client_parse() parses with qemu_opts_parse_noisily(), passing
>> QemuOptsList qemu_netdev_opts for -netdev, qemu_nic_opts for -nic, and
>> qemu_net_opts for -net.  Their desc[] are all empty, which means any
>> keys are accepted.  The result of the parse (a QemuOpts) is stored in
>> the QemuOptsList.
>>
>> Note that QemuOpts is flat by design.  In some places, we layer non-flat
>> on top using dotted keys convention, but not here.
>>
>> net_init_clients() iterates over the stored QemuOpts, and passes them to
>> net_init_netdev(), net_param_nic(), or net_init_client(), respectively.
>>
>> These functions pass the QemuOpts to net_client_init().  They also do
>> other things with the QemuOpts, which we can ignore here.
>>
>> net_client_init() uses the opts visitor to convert the (flat) QemOpts to
>> a (non-flat) QAPI object Netdev.  Netdev is also the argument of QMP
>> command netdev_add.
>>
>> The opts visitor was an early attempt to support QAPI in
>> (QemuOpts-based) CLI.  It restricts QAPI types to a certain shape; see
>> commit eb7ee2cbeb "qapi: introduce OptsVisitor".
>>
>> A more modern way to support QAPI is qobject_input_visitor_new_str().
>> It uses keyval_parse() instead of QemuOpts for KEY=VALUE,... syntax, and
>> it also supports JSON syntax.  The former isn't quite as expressive as
>> JSON, but it's a lot closer than QemuOpts + opts visitor.
>>
>> This commit paves the way to use of the modern way instead.
>>
>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
>> ---
>>   include/net/net.h |  1 +
>>   net/net.c         | 60 +++++++++++++++++++++++++++++++++++++++++++++++
>>   softmmu/vl.c      |  3 ++-
>>   3 files changed, 63 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/net/net.h b/include/net/net.h
>> index c53c64ac18c4..4ae8ed480f73 100644
>> --- a/include/net/net.h
>> +++ b/include/net/net.h
>> @@ -214,6 +214,7 @@ extern NICInfo nd_table[MAX_NICS];
>>   extern const char *host_net_devices[];
>>   
>>   /* from net.c */
>> +int netdev_parse_modern(const char *optarg);
>>   int net_client_parse(QemuOptsList *opts_list, const char *str);
>>   void show_netdevs(void);
>>   void net_init_clients(void);
>> diff --git a/net/net.c b/net/net.c
>> index 15958f881776..c337d3d753fe 100644
>> --- a/net/net.c
>> +++ b/net/net.c
>> @@ -54,6 +54,7 @@
>>   #include "net/colo-compare.h"
>>   #include "net/filter.h"
>>   #include "qapi/string-output-visitor.h"
>> +#include "qapi/qobject-input-visitor.h"
>>   
>>   /* Net bridge is currently not supported for W32. */
>>   #if !defined(_WIN32)
>> @@ -63,6 +64,16 @@
>>   static VMChangeStateEntry *net_change_state_entry;
>>   static QTAILQ_HEAD(, NetClientState) net_clients;
>>   
>> +typedef struct NetdevQueueEntry {
>> +    Netdev *nd;
>> +    Location loc;
>> +    QSIMPLEQ_ENTRY(NetdevQueueEntry) entry;
>> +} NetdevQueueEntry;
>> +
>> +typedef QSIMPLEQ_HEAD(, NetdevQueueEntry) NetdevQueue;
>> +
>> +static NetdevQueue nd_queue = QSIMPLEQ_HEAD_INITIALIZER(nd_queue);
>> +
>>   /***********************************************************/
>>   /* network device redirectors */
>>   
>> @@ -1562,6 +1573,20 @@ out:
>>       return ret;
>>   }
>>   
>> +static void netdev_init_modern(void)
>> +{
>> +    while (!QSIMPLEQ_EMPTY(&nd_queue)) {
>> +        NetdevQueueEntry *nd = QSIMPLEQ_FIRST(&nd_queue);
>> +
>> +        QSIMPLEQ_REMOVE_HEAD(&nd_queue, entry);
>> +        loc_push_restore(&nd->loc);
>> +        net_client_init1(nd->nd, true, &error_fatal);
>> +        loc_pop(&nd->loc);
>> +        qapi_free_Netdev(nd->nd);
>> +        g_free(nd);
>> +    }
>> +}
>> +
>>   void net_init_clients(void)
>>   {
>>       net_change_state_entry =
>> @@ -1569,6 +1594,8 @@ void net_init_clients(void)
>>   
>>       QTAILQ_INIT(&net_clients);
>>   
>> +    netdev_init_modern();
>> +
>>       qemu_opts_foreach(qemu_find_opts("netdev"), net_init_netdev, NULL,
>>                         &error_fatal);
>>   
>> @@ -1579,6 +1606,39 @@ void net_init_clients(void)
>>                         &error_fatal);
>>   }
>>   
>> +/*
>> + * netdev_is_modern() returns true when the backend needs to bypass
>> + * qemu_opts_parse_noisily()
>> + */
>> +static bool netdev_is_modern(const char *optarg)
>> +{
>> +    return false;
>> +}
>> +
>> +/*
>> + * netdev_parse_modern() uses modern, more expressive syntax than
>> + * net_client_parse(), supports only the netdev option.
>> + */
>> +int netdev_parse_modern(const char *optarg)
>> +{
>> +    Visitor *v;
>> +    NetdevQueueEntry *nd;
>> +
>> +    if (!netdev_is_modern(optarg)) {
>> +        return -1;
>> +    }
>> +
>> +    v = qobject_input_visitor_new_str(optarg, "type", &error_fatal);
>> +    nd = g_new(NetdevQueueEntry, 1);
>> +    visit_type_Netdev(v, NULL, &nd->nd, &error_fatal);
>> +    visit_free(v);
>> +    loc_save(&nd->loc);
>> +
>> +    QSIMPLEQ_INSERT_TAIL(&nd_queue, nd, entry);
>> +
>> +    return 0;
>> +}
>> +
>>   int net_client_parse(QemuOptsList *opts_list, const char *optarg)
>>   {
>>       if (!qemu_opts_parse_noisily(opts_list, optarg, true)) {
>> diff --git a/softmmu/vl.c b/softmmu/vl.c
>> index 8eed0f31c073..838f5b48c447 100644
>> --- a/softmmu/vl.c
>> +++ b/softmmu/vl.c
>> @@ -2839,7 +2839,8 @@ void qemu_init(int argc, char **argv, char **envp)
>>                   break;
>>               case QEMU_OPTION_netdev:
>>                   default_net = 0;
>> -                if (net_client_parse(qemu_find_opts("netdev"), optarg) == -1) {
>> +                if (netdev_parse_modern(optarg) == -1 &&
>> +                    net_client_parse(qemu_find_opts("netdev"), optarg) == -1) {
>>                       exit(1);
>>                   }
>>                   break;
> 
> To make this work, netdev_parse_modern() must
> 
> * either succeeed, or
> 
> * fail without reporting an error, or
> 
> * report an error and exit()
> 
> Recommend to spell that out in its function comment.
> 
> Alternatively:
> 
>                     if (netdev_is_modern(optarg)) {
>                         netdev_parse_modern(optarg);
>                     } else {
>                         if (net_client_parse(qemu_find_opts("netdev"), optarg) == -1) {
>                             exit(1);
>                         }
>                     }
> 
> netdev_is_modern() needs external linkage, and netdev_parse_modern()
> loses its return value.
> 
> Note that all callers net_client_parse() handle failure exactly the same
> way.  If we let net_client_parse() exit(), then this becomes
> 
>                     if (netdev_is_modern(optarg)) {
>                         netdev_parse_modern(optarg);
>                     } else {
>                         net_client_parse(qemu_find_opts("netdev"), optarg);
>                     }
> 
> I like this one best.  What do you think?
> 

Me too.

I wanted to have only entry point in net.c it's why I didn't export netdev_is_modern() but 
I think it's better to export it not to mix error return (parse has failed) and checking 
result (is modern or not).

And I agree it's more consistent to have both parse functions behaving in the same way 
(exit or not exit...).

I update the patch in that way.

Thanks,
Laurent