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 <lcapitul...@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
