On Fri, Jul 07, 2017 at 04:30:29PM -0300, Eduardo Habkost wrote:
> On Fri, Jul 07, 2017 at 08:18:20PM +0200, Igor Mammedov wrote:
> > On Fri, 7 Jul 2017 12:09:56 -0300
> > Eduardo Habkost <ehabk...@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" <ehabk...@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.
I could reproduce it with the following test case.
Signed-off-by: Eduardo Habkost <ehabk...@redhat.com>
---
diff --git a/tests/check-qom-proplist.c b/tests/check-qom-proplist.c
index 8e432e9..c320cff 100644
--- a/tests/check-qom-proplist.c
+++ b/tests/check-qom-proplist.c
@@ -568,6 +568,29 @@ static void test_dummy_delchild(void)
object_unparent(OBJECT(dev));
}
+static void test_qom_partial_path(void)
+{
+ Object *root = object_get_objects_root();
+ Object *o = object_new(TYPE_DUMMY);
+ Object *o1_1 = object_new(TYPE_DUMMY);
+ Object *o1_2 = object_new(TYPE_DUMMY);
+ Object *o2 = object_new(TYPE_DUMMY);
+
+ object_property_add_child(root, "o", o, &error_abort);
+ object_property_add_child(o, "o1", o1_1, &error_abort);
+ object_property_add_child(o, "o2", o1_2, &error_abort);
+ object_property_add_child(root, "o2", o2, &error_abort);
+
+ g_assert(!object_resolve_path_type("", TYPE_DUMMY, NULL));
+ g_assert(!object_resolve_path("o2", NULL));
+ g_assert(object_resolve_path("o1", NULL) == o1_1);
+
+ object_unref(o);
+ object_unref(o2);
+ object_unref(o1_1);
+ object_unref(o1_2);
+}
+
int main(int argc, char **argv)
{
g_test_init(&argc, &argv, NULL);
@@ -585,6 +608,7 @@ int main(int argc, char **argv)
g_test_add_func("/qom/proplist/getenum", test_dummy_getenum);
g_test_add_func("/qom/proplist/iterator", test_dummy_iterator);
g_test_add_func("/qom/proplist/delchild", test_dummy_delchild);
+ g_test_add_func("/qom/path/resolve/partial", test_qom_partial_path);
return g_test_run();
}
--
Eduardo
