[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCHv7 5/6] fw_cfg: move qdev_init_nofail() from fw_c
From: |
Eduardo Habkost |
Subject: |
Re: [Qemu-devel] [PATCHv7 5/6] fw_cfg: move qdev_init_nofail() from fw_cfg_init1() to callers |
Date: |
Mon, 10 Jul 2017 14:38:21 -0300 |
User-agent: |
Mutt/1.8.0 (2017-02-23) |
On Mon, Jul 10, 2017 at 05:23:36PM +0200, Igor Mammedov wrote:
> On Mon, 10 Jul 2017 11:53:31 -0300
> Eduardo Habkost <address@hidden> wrote:
>
> > On Mon, Jul 10, 2017 at 10:01:47AM +0200, Igor Mammedov wrote:
> > > On Fri, 7 Jul 2017 17:20:25 +0100
> > > Mark Cave-Ayland <address@hidden> wrote:
> > >
> > > > On 07/07/17 16:07, Eduardo Habkost wrote:
> > > >
> > > > >> looks fine,
> > > > >>
> > > > >> so what I'd do is:
> > > > >> * drop 4/6
> > > >
> > > > Yes.
> > > >
> > > > > Agreed on this point. But:
> > > > >
> > > > >> * make fw_cfg_find() use ambiguous argument and error_abort if
> > > > >> ambiguous == true
> > > >
> > > > During my latest tests I've found that everything works fine without the
> > > > ambiguous argument.
> > > >
> > > > Do we still want to keep it? And I don't think error_abort() is the
> > > > right thing to do here, I'd much rather return NULL and add a suitable
> > > > comment.
> > > I'd still use ambiguous argument and since you prefer not to assert
> > > I'd add errp argument to fw_cfg_find() and handle error at callsites.
> > >
> > > Just returning NULL isn't sufficient if you need to distinguish
> > > 'not found' vs 'duplicate' usecases, additionally 'not found'
> > > in most cases isn't even error but 'duplicate' definitely is.
> > >
> > > Aborting on diplicate in fw_cfg_find() is fine and would
> > > help to avoid touching current callers if you wish to limit
> > > patches scope, but you can go with proper error propagating
> > > route if you wish.
> >
> > Just making realize refuse to create two devices sounds much
> > simpler to me. No need to make fw_cfg_find() more complex (if we
> > add errp argument to it) or less useful (if we add
> > assert(!ambiguous) to it).
> the problem here was a error message to print if fw_cfg_find()
> returns NULL for missing or duplicate, if we need to print
> precise error we would need proper error handling.
I don't see where we would need a precise error message, except
for realizefn (where the only case fw_cfg_find() would return
NULL is for duplicate devices).
>
> Considering to fw_cfg is builtin device I'd prefer just
> assert in fw_cfg_find() on duplicate (all the callers consider it as error)
> and let developer to deal with assert if it is triggered.
Except that it would make it more difficult for realizefn to
return a proper error message.
Anyway, I am not completely against adding assert(!ambiguous) to
fw_cfg_find() if Mark wants to follow your advice. I just think
it's not necessary. I will only continue discussing this if I
see issues in the next version of the series.
--
Eduardo
- Re: [Qemu-devel] [PATCHv7 5/6] fw_cfg: move qdev_init_nofail() from fw_cfg_init1() to callers, (continued)
- Re: [Qemu-devel] [PATCHv7 5/6] fw_cfg: move qdev_init_nofail() from fw_cfg_init1() to callers, Igor Mammedov, 2017/07/07
- Re: [Qemu-devel] [PATCHv7 5/6] fw_cfg: move qdev_init_nofail() from fw_cfg_init1() to callers, Eduardo Habkost, 2017/07/07
- Re: [Qemu-devel] [PATCHv7 5/6] fw_cfg: move qdev_init_nofail() from fw_cfg_init1() to callers, Eduardo Habkost, 2017/07/07
- Re: [Qemu-devel] [PATCHv7 5/6] fw_cfg: move qdev_init_nofail() from fw_cfg_init1() to callers, Mark Cave-Ayland, 2017/07/07
- Re: [Qemu-devel] [PATCHv7 5/6] fw_cfg: move qdev_init_nofail() from fw_cfg_init1() to callers, Igor Mammedov, 2017/07/10
- Re: [Qemu-devel] [PATCHv7 5/6] fw_cfg: move qdev_init_nofail() from fw_cfg_init1() to callers, Eduardo Habkost, 2017/07/10
- Re: [Qemu-devel] [PATCHv7 5/6] fw_cfg: move qdev_init_nofail() from fw_cfg_init1() to callers, Igor Mammedov, 2017/07/10
- Re: [Qemu-devel] [PATCHv7 5/6] fw_cfg: move qdev_init_nofail() from fw_cfg_init1() to callers,
Eduardo Habkost <=
- Re: [Qemu-devel] [PATCHv7 5/6] fw_cfg: move qdev_init_nofail() from fw_cfg_init1() to callers, Mark Cave-Ayland, 2017/07/11
- Re: [Qemu-devel] [PATCHv7 5/6] fw_cfg: move qdev_init_nofail() from fw_cfg_init1() to callers, Igor Mammedov, 2017/07/10
- Re: [Qemu-devel] [PATCHv7 5/6] fw_cfg: move qdev_init_nofail() from fw_cfg_init1() to callers, Eduardo Habkost, 2017/07/10
- Re: [Qemu-devel] [PATCHv7 5/6] fw_cfg: move qdev_init_nofail() from fw_cfg_init1() to callers, Eduardo Habkost, 2017/07/07
- Re: [Qemu-devel] [PATCHv7 5/6] fw_cfg: move qdev_init_nofail() from fw_cfg_init1() to callers, Igor Mammedov, 2017/07/07
- Re: [Qemu-devel] [PATCHv7 5/6] fw_cfg: move qdev_init_nofail() from fw_cfg_init1() to callers, Eduardo Habkost, 2017/07/07
- Re: [Qemu-devel] [PATCHv7 5/6] fw_cfg: move qdev_init_nofail() from fw_cfg_init1() to callers, Igor Mammedov, 2017/07/07
- Re: [Qemu-devel] [PATCHv7 5/6] fw_cfg: move qdev_init_nofail() from fw_cfg_init1() to callers, Eduardo Habkost, 2017/07/07
- Re: [Qemu-devel] [PATCHv7 5/6] fw_cfg: move qdev_init_nofail() from fw_cfg_init1() to callers, Eduardo Habkost, 2017/07/07
- Re: [Qemu-devel] [PATCHv7 5/6] fw_cfg: move qdev_init_nofail() from fw_cfg_init1() to callers, Laszlo Ersek, 2017/07/07