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

[Luiz Capitulino <lcapitulino@redhat.com>]

On Fri, 18 May 2012 17:51:01 +0200
Laszlo Ersek <lersek@redhat.com> 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").