[16/16] qapi: convert netdev_del

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 hmp-commands.hx  |    3 +--
 hmp.c            |    9 +++++++++
 hmp.h            |    1 +
 net.c            |   11 +++++------
 net.h            |    1 -
 qapi-schema.json |   14 ++++++++++++++
 qmp-commands.hx  |    5 +----
 7 files changed, 31 insertions(+), 13 deletions(-)

On 05/17/12 16:33, Luiz Capitulino wrote:

> diff --git a/hmp.c b/hmp.c
> index 7a4e25f..2ce8cb9 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -990,3 +990,12 @@ void hmp_netdev_add(Monitor *mon, const QDict *qdict)
>  out:
>      hmp_handle_error(mon, &err);
>  }
> +
> +void hmp_netdev_del(Monitor *mon, const QDict *qdict)
> +{
> +    const char *id = qdict_get_str(qdict, "id");
> +    Error *err = NULL;
> +
> +    qmp_netdev_del(id, &err);
> +    hmp_handle_error(mon, &err);
> +}

I'm not sure what invariants "qdict" satisfies on entry to this
function... Does it certainly contain "id", and is it a string? If not,
even qdict_get_str() (or something below it) would crash, so I'll assume
we do set "id" to non-NULL here -- hmp_device_del() does the same.


> diff --git a/hmp.h b/hmp.h
> index 017df87..79d138d 100644
> --- a/hmp.h
> +++ b/hmp.h
> @@ -63,5 +63,6 @@ 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);
> +void hmp_netdev_del(Monitor *mon, const QDict *qdict);
>  
>  #endif
> diff --git a/net.c b/net.c
> index 5f0c53c..4aa416c 100644
> --- a/net.c
> +++ b/net.c
> @@ -1269,19 +1269,18 @@ exit_err:
>      return -1;
>  }
>  
> -int do_netdev_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
> +void qmp_netdev_del(const char *id, Error **errp)
>  {
> -    const char *id = qdict_get_str(qdict, "id");
>      VLANClientState *vc;
>  
>      vc = qemu_find_netdev(id);

OK, "id" must be non-NULL either way.

>      if (!vc) {
> -        qerror_report(QERR_DEVICE_NOT_FOUND, id);
> -        return -1;
> +        error_set(errp, QERR_DEVICE_NOT_FOUND, id);
> +        return;
>      }
> +
>      qemu_del_vlan_client(vc);
> -    qemu_opts_del(qemu_opts_find(qemu_find_opts("netdev"), id));
> -    return 0;
> +    qemu_opts_del(qemu_opts_find(qemu_find_opts_err("netdev", errp), id));
>  }

I think this last change is both unnecessary and ineffective.
qemu_find_opts("netdev") should always succeed.

If not, then -- even though qemu_find_opts_err() -> find_list() sets the
error -- we call qemu_opts_find(NULL, id), which I think crashes.

Anyway it seems harmless.

>  
>  static void print_net_client(Monitor *mon, VLANClientState *vc)
> diff --git a/net.h b/net.h
> index 1eb9280..bdc2a06 100644
> --- a/net.h
> +++ b/net.h
> @@ -172,7 +172,6 @@ void net_host_device_add(Monitor *mon, const QDict *qdict);
>  void net_host_device_remove(Monitor *mon, const QDict *qdict);
>  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"
>  #define DEFAULT_NETWORK_DOWN_SCRIPT "/etc/qemu-ifdown"
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 69fcd8e..bb1f806 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -1826,3 +1826,17 @@
>  { 'command': 'netdev_add',
>    'data': {'type': 'str', 'id': 'str', '*props': '**'},

Yep, "id" is mandatory, and I think it ensures that "id"'s value is at
worst an empty string in qmp_netdev_del(), not a NULL pointer.
(qmp_input_type_str() calls g_strdup(), if I'm not lost.)

>    'gen': 'no' }
> +
> +##
> +# @netdev_del:
> +#
> +# Remove a network backend.
> +#
> +# @id: the name of the network backend to remove
> +#
> +# Returns: Nothing on success
> +#          If @id is not a valid network backend, DeviceNotFound
> +#
> +# Since: 0.14.0
> +##
> +{ 'command': 'netdev_del', 'data': {'id': 'str'} }
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index f6550cb..57ea803 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -671,10 +671,7 @@ EQMP
>      {
>          .name       = "netdev_del",
>          .args_type  = "id:s",
> -        .params     = "id",
> -        .help       = "remove host network device",
> -        .user_print = monitor_user_noop,
> -        .mhandler.cmd_new = do_netdev_del,
> +        .mhandler.cmd_new = qmp_marshal_input_netdev_del,
>      },
>  
>  SQMP

I'm ready to ACK the series, but I'm intrigued by the net_init_netdev()
change in 15/16... :)

Thanks!
Laszlo

On Fri, 18 May 2012 18:15:34 +0200
Laszlo Ersek <lersek@redhat.com> wrote:

> On 05/17/12 16:33, Luiz Capitulino wrote:
> 
> > diff --git a/hmp.c b/hmp.c
> > index 7a4e25f..2ce8cb9 100644
> > --- a/hmp.c
> > +++ b/hmp.c
> > @@ -990,3 +990,12 @@ void hmp_netdev_add(Monitor *mon, const QDict *qdict)
> >  out:
> >      hmp_handle_error(mon, &err);
> >  }
> > +
> > +void hmp_netdev_del(Monitor *mon, const QDict *qdict)
> > +{
> > +    const char *id = qdict_get_str(qdict, "id");
> > +    Error *err = NULL;
> > +
> > +    qmp_netdev_del(id, &err);
> > +    hmp_handle_error(mon, &err);
> > +}
> 
> I'm not sure what invariants "qdict" satisfies on entry to this
> function... Does it certainly contain "id", and is it a string? If not,
> even qdict_get_str() (or something below it) would crash, so I'll assume
> we do set "id" to non-NULL here -- hmp_device_del() does the same.

qdict is a dictionary containing all options passed by the user. In this
context, "id" is an required option so it must exist at this point (if the
user doesn't pass it HMP (the human monitor) will fail before getting here).

> I'm ready to ACK the series, but I'm intrigued by the net_init_netdev()
> change in 15/16... :)

Thanks a lot for the review!