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

[Laszlo Ersek]

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 <address@hidden> 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" <address@hidden> 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/address@hidden/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 <address@hidden>
> ---
> 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