[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 15/16] qapi: convert netdev_add
From: |
Luiz Capitulino |
Subject: |
Re: [Qemu-devel] [PATCH 15/16] qapi: convert netdev_add |
Date: |
Fri, 18 May 2012 14:41:36 -0300 |
On Fri, 18 May 2012 17:51:01 +0200
Laszlo Ersek <address@hidden> wrote:
> > +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.
Yes, as far as I can see a failure is impossible. We could add an assert(),
but this patch doesn't change the current code in that aspect.
> > 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?
I think you found two problems here. The first one is that this hunk doesn't
pertain to this patch, it should be in the previous one.
The other problem is this:
> 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().
Your theory is right, but this is not related to this series, as the current
code does it too. But more importantly, it seems that that condition can
never happen in practice.
The only type init function that returns a positive value is net_init_nic().
However, net_init_netdev() calls net_client_init() with is_netdev=1, meaning
that it wants to initialize a backend. This also means that net_init_nic()
will never be called in this context.
We have two options here:
1. Just maintain that behavior. This was my original intention, but then
the hunk changing net_init_netdev() should be moved to the previous patch
2. The value returned by net_init_nic() doesn't seem to be used outside
net_init_nic(), so I could add a patch to this series dropping it
Michael, Markus, any suggestion?
Other than that I think we should have a better separation between front-ends
and back-ends in that code (we seem to call everything a "type").
- [Qemu-devel] [PATCH 03/16] qemu-option: parse_option_bool(): use error_set(), (continued)
- [Qemu-devel] [PATCH 03/16] qemu-option: parse_option_bool(): use error_set(), Luiz Capitulino, 2012/05/17
- [Qemu-devel] [PATCH 08/16] qemu-option: introduce qemu_opt_set_err(), Luiz Capitulino, 2012/05/17
- [Qemu-devel] [PATCH 04/16] qemu-option: parse_option_size(): use error_set(), Luiz Capitulino, 2012/05/17
- [Qemu-devel] [PATCH 13/16] net: purge the monitor object from all init functions, Luiz Capitulino, 2012/05/17
- [Qemu-devel] [PATCH 11/16] qemu-config: find_list(): use error_set(), Luiz Capitulino, 2012/05/17
- [Qemu-devel] [PATCH 15/16] qapi: convert netdev_add, Luiz Capitulino, 2012/05/17
- [Qemu-devel] [PATCH 16/16] qapi: convert netdev_del, Luiz Capitulino, 2012/05/17
- [Qemu-devel] [PATCH 12/16] qemu-config: introduce qemu_find_opts_err(), Luiz Capitulino, 2012/05/17
- [Qemu-devel] [PATCH 06/16] qemu-option: qemu_opts_validate(): use error_set(), Luiz Capitulino, 2012/05/17
- [Qemu-devel] [PATCH 14/16] net: net_client_init(): use error_set(), Luiz Capitulino, 2012/05/17