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
