Re: [Qemu-devel] [PATCHv7 5/6] fw_cfg: move qdev_init_nofail() from fw_cfg_init1() to callers

[Eduardo Habkost <ehabkost@redhat.com>]

On Fri, Jul 07, 2017 at 08:18:20PM +0200, Igor Mammedov wrote:
> On Fri, 7 Jul 2017 12:09:56 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Fri, Jul 07, 2017 at 04:58:17PM +0200, Igor Mammedov wrote:
> > > On Fri, 7 Jul 2017 10:13:00 -0300
> > > "Eduardo Habkost" <ehabkost@redhat.com> wrote:
> > [...]
> > > > I don't disagree with adding the assert(), but it looks like
> > > > making fw_cfg_find() return NULL if there are multiple devices
> > > > can be useful for realize.
> > > > 
> > > > In this case, it looks like Mark is relying on that in
> > > > fw_cfg_common_realize(): if multiple devices are created,
> > > > fw_cfg_find() will return NULL, and realize will fail.  This
> > > > sounds like a more graceful way to handle multiple-device
> > > > creation than crashing on fw_cfg_find().  This is the solution
> > > > used by find_vmgenid_dev()/vmgenid_realize(), BTW.
> > > 
> > > I suspect that find_vmgenid_dev() works by luck as it could be
> > > placed only as /machine/peripheral-anon/foo1 or /machine/peripheral/foo2
> > >   object_resolve_partial_path() : machine
> > >            object_resolve_partial_path() : peripheral-anon => foo1
> > >            object_resolve_partial_path() : peripheral => foo2
> > >            if (found /* foo2 */) {
> > >                if (obj /* foo1 */) {
> > >                    return NULL;
> > 
> > I don't think this is luck: object_resolve_partial_path() is
> > explicitly documented to always return NULL if multiple matches
> > are found, and I don't see any bug in its implementation that
> > would break that functionality.
> 
> Maybe I'm reading it wrong, but consider following:
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg460692.html
> 
> it looks to me that using ambiguous argument is necessary for
> duplicate detection to work correctly.

Oh, good catch, I think I see the bug now.  We need to write a
test case and fix that.

-- 
Eduardo