[15/16] qapi: convert netdev_add

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(-)

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

On Fri, 18 May 2012 17:51:01 +0200
Laszlo Ersek <lersek@redhat.com> wrote:

> > +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.

Yes, as far as I can see a failure is impossible. We could add an assert(),
but this patch doesn't change the current code in that aspect.

> >  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?

I think you found two problems here. The first one is that this hunk doesn't
pertain to this patch, it should be in the previous one.

The other problem is this:

> 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().

Your theory is right, but this is not related to this series, as the current
code does it too. But more importantly, it seems that that condition can
never happen in practice.

The only type init function that returns a positive value is net_init_nic().
However, net_init_netdev() calls net_client_init() with is_netdev=1, meaning
that it wants to initialize a backend. This also means that net_init_nic()
will never be called in this context.

We have two options here:

 1. Just maintain that behavior. This was my original intention, but then
    the hunk changing net_init_netdev() should be moved to the previous patch

 2. The value returned by net_init_nic() doesn't seem to be used outside
    net_init_nic(), so I could add a patch to this series dropping it

Michael, Markus, any suggestion?

Other than that I think we should have a better separation between front-ends
and back-ends in that code (we seem to call everything a "type").

Il 18/05/2012 19:41, Luiz Capitulino ha scritto:
> Other than that I think we should have a better separation between front-ends
> and back-ends in that code (we seem to call everything a "type").

That makes sense if you consider the way -net originally worked with
VLANs...

Paolo