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

[Laszlo Ersek]

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