| Message ID | 20170707195416.GK10776@localhost.localdomain |
|---|---|
| State | New |
| Headers | show |
On 07/07/17 21:54, Eduardo Habkost wrote: > 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 <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. > > I could reproduce it with the following test case. > > Signed-off-by: Eduardo Habkost <ehabkost@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(); > } > Meta comment: variable names like "o" and "l" are terrible (they look like 0 and 1, dependent on your font). I suggest "obj". Thanks Laszlo
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(); }