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