Re: [Qemu-devel] [PATCH 15/16] qapi: convert netdev_add

[Laszlo Ersek <lersek@redhat.com>]

Comments in-line.

On 05/17/12 16:33, Luiz Capitulino wrote:
> This is not a full QAPI conversion, but an intermediate step.
> 
> In essence, do_netdev_add() is split into three functions:
> 
>  1. netdev_add(): performs the actual work. This function is fully
>     converted to Error (thus, it's "qapi-friendly")
> 
>  2. qmp_netdev_add(): the QMP front-end for netdev_add(). This is
>     coded by hand and not auto-generated (gen=no in the schema). The
>     reason for this it's a lot easier and simpler to with QemuOpts
>     this way
> 
>  3. hmp_netdev_add(): HMP front-end.
> 
> This design was suggested by Paolo Bonzini.
> 
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> ---
>  hmp-commands.hx  |    3 +--
>  hmp.c            |   21 +++++++++++++++++++++
>  hmp.h            |    1 +
>  net.c            |   41 +++++++++++++++++++++++++++--------------
>  net.h            |    3 ++-
>  qapi-schema.json |   28 ++++++++++++++++++++++++++++
>  qmp-commands.hx  |    5 +----
>  7 files changed, 81 insertions(+), 21 deletions(-)
> 
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 81723c8..d0ce6a5 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -1037,8 +1037,7 @@ ETEXI
>          .args_type  = "netdev:O",
>          .params     = "[user|tap|socket],id=str[,prop=value][,...]",
>          .help       = "add host network device",
> -        .user_print = monitor_user_noop,
> -        .mhandler.cmd_new = do_netdev_add,
> +        .mhandler.cmd = hmp_netdev_add,
>      },
>  
>  STEXI
> diff --git a/hmp.c b/hmp.c
> index 42ced2a..7a4e25f 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -14,6 +14,8 @@
>   */
>  
>  #include "hmp.h"
> +#include "net.h"
> +#include "qemu-option.h"
>  #include "qemu-timer.h"
>  #include "qmp-commands.h"
>  
> @@ -969,3 +971,22 @@ void hmp_dump_guest_memory(Monitor *mon, const QDict *qdict)
>                            &errp);
>      hmp_handle_error(mon, &errp);
>  }
> +
> +void hmp_netdev_add(Monitor *mon, const QDict *qdict)
> +{
> +    Error *err = NULL;
> +    QemuOpts *opts;
> +
> +    opts = qemu_opts_from_qdict(qemu_find_opts("netdev"), qdict, &err);

I note we trust qemu_find_opts("netdev") to succeed, as we did in
do_netdev_add() before.


> +    if (error_is_set(&err)) {
> +        goto out;
> +    }
> +
> +    netdev_add(opts, &err);

OK this takes on the task of net_client_init().

> +    if (error_is_set(&err)) {
> +        qemu_opts_del(opts);
> +    }
> +
> +out:
> +    hmp_handle_error(mon, &err);
> +}

Basically the HMP function does the same as do_netdev_add() did before. OK.


> diff --git a/hmp.h b/hmp.h
> index 5cf3241..017df87 100644
> --- a/hmp.h
> +++ b/hmp.h
> @@ -62,5 +62,6 @@ void hmp_block_job_cancel(Monitor *mon, const QDict *qdict);
>  void hmp_migrate(Monitor *mon, const QDict *qdict);
>  void hmp_device_del(Monitor *mon, const QDict *qdict);
>  void hmp_dump_guest_memory(Monitor *mon, const QDict *qdict);
> +void hmp_netdev_add(Monitor *mon, const QDict *qdict);
>  
>  #endif
> diff --git a/net.c b/net.c
> index 7e9d0c5..5f0c53c 100644
> --- a/net.c
> +++ b/net.c
> @@ -1234,27 +1234,39 @@ void net_host_device_remove(Monitor *mon, const QDict *qdict)
>      qemu_del_vlan_client(vc);
>  }
>  
> -int do_netdev_add(Monitor *mon, const QDict *qdict, QObject **ret_data)
> +void netdev_add(QemuOpts *opts, Error **errp)
> +{
> +    net_client_init(opts, 1, errp);
> +}
> +
> +int qmp_netdev_add(Monitor *mon, const QDict *qdict, QObject **ret)
>  {
>      Error *local_err = NULL;
> +    QemuOptsList *opts_list;
>      QemuOpts *opts;
> -    int res;
>  
> -    opts = qemu_opts_from_qdict(qemu_find_opts("netdev"), qdict, &local_err);
> -    if (!opts) {
> -        qerror_report_err(local_err);
> -        error_free(local_err);
> -        return -1;
> +    opts_list = qemu_find_opts_err("netdev", &local_err);

I think we're a bit too paranoid here (and a bit inconsistent with
hmp_netdev_add()), but that's just a superficial observation.


> +    if (error_is_set(&local_err)) {
> +        goto exit_err;
>      }
>  
> -    res = net_client_init(opts, 1, &local_err);
> -    if (res < 0) {
> -        qerror_report_err(local_err);
> -        error_free(local_err);
> +    opts = qemu_opts_from_qdict(opts_list, qdict, &local_err);
> +    if (error_is_set(&local_err)) {
> +        goto exit_err;
> +    }
> +
> +    netdev_add(opts, &local_err);
> +    if (error_is_set(&local_err)) {
>          qemu_opts_del(opts);
> +        goto exit_err;
>      }
>  
> -    return res;
> +    return 0;
> +
> +exit_err:
> +    qerror_report_err(local_err);
> +    error_free(local_err);
> +    return -1;
>  }

OK. We migrate to error_is_set() checks now, move the error reporting to
the end, into a common block. Also drop "res", since netdev_add()
ignores net_client_init()'s retval anyway.

>  
>  int do_netdev_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
> @@ -1447,15 +1459,16 @@ static int net_init_client(QemuOpts *opts, void *dummy)
>  static int net_init_netdev(QemuOpts *opts, void *dummy)
>  {
>      Error *local_err = NULL;
> +    int ret;
>  
> -    net_client_init(opts, 1, &local_err);
> +    ret = net_client_init(opts, 1, &local_err);
>      if (error_is_set(&local_err)) {
>          qerror_report_err(local_err);
>          error_free(local_err);
>          return -1;
>      }
>  
> -    return 0;
> +    return ret;
>  }

Hmmm. How is this modification related to this patch, and why is it
necessary in general?

net_client_init() can return -1 in eight places, and it propagates /
sets errors too in those places. There's another place in there where
the type-specific init function can return a positive value (and no
error is set or reported). Until now we seemed to handle that no
differently from 0.

This change will allow net_init_netdev() to pass this positive value to
its only caller, net_init_clients():

net_init_clients()
  qemu_opts_foreach() -- called with abort_on_failure == 1
    net_init_netdev() -- now returning positive values
      net_client_init()
        type-specific init

In effect a positive retval from the type-specific init function will
now abort the loop in qemu_opts_foreach(), but it won't cause
net_init_clients() itself to fail (= return -1 early), because rc will
be positive in qemu_opts_foreach().

What are we trying to achieve?

The rest seems fine.

Thanks,
Laszlo

>  
>  int net_init_clients(void)
> diff --git a/net.h b/net.h
> index 7ee97e9..1eb9280 100644
> --- a/net.h
> +++ b/net.h
> @@ -170,7 +170,8 @@ void net_check_clients(void);
>  void net_cleanup(void);
>  void net_host_device_add(Monitor *mon, const QDict *qdict);
>  void net_host_device_remove(Monitor *mon, const QDict *qdict);
> -int do_netdev_add(Monitor *mon, const QDict *qdict, QObject **ret_data);
> +void netdev_add(QemuOpts *opts, Error **errp);
> +int qmp_netdev_add(Monitor *mon, const QDict *qdict, QObject **ret);
>  int do_netdev_del(Monitor *mon, const QDict *qdict, QObject **ret_data);
>  
>  #define DEFAULT_NETWORK_SCRIPT "/etc/qemu-ifup"
> diff --git a/qapi-schema.json b/qapi-schema.json
> index dfa0e1f..69fcd8e 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -1798,3 +1798,31 @@
>  { 'command': 'dump-guest-memory',
>    'data': { 'paging': 'bool', 'protocol': 'str', '*begin': 'int',
>              '*length': 'int' } }
> +##
> +# @netdev_add:
> +#
> +# Add a network backend.
> +#
> +# @type: the type of network backend.  Current valid values are 'user', 'tap',
> +#        'vde', 'socket', 'dump' and 'bridge'
> +#
> +# @id: the name of the new network backend
> +#
> +# @props: #optional a list of properties to be passed to the backend in
> +#         the format 'name=value', like 'ifname=tap0,script=no'
> +#
> +# Notes: The semantics of @props is not well defined.  Future commands will be
> +#        introduced that provide stronger typing for backend creation.
> +#
> +# Since: 0.14.0
> +#
> +# Returns: Nothing on success
> +#          If @type is not a valid network backend, DeviceNotFound
> +#          If @id is not a valid identifier, InvalidParameterValue
> +#          if @id already exists, DuplicateId
> +#          If @props contains an invalid parameter for this backend,
> +#            InvalidParameter
> +##
> +{ 'command': 'netdev_add',
> +  'data': {'type': 'str', 'id': 'str', '*props': '**'},
> +  'gen': 'no' }
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 2aa64ad..f6550cb 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -642,10 +642,7 @@ EQMP
>      {
>          .name       = "netdev_add",
>          .args_type  = "netdev:O",
> -        .params     = "[user|tap|socket],id=str[,prop=value][,...]",
> -        .help       = "add host network device",
> -        .user_print = monitor_user_noop,
> -        .mhandler.cmd_new = do_netdev_add,
> +        .mhandler.cmd_new = qmp_netdev_add,
>      },
>  
>  SQMP