On Thu, May 19, 2022 at 8:14 PM Lei Y= u <yulei.sh@bytedance.com&= gt; wrote:

This = email is to describe an issue in mapper that the
GetSubTree/GetSubTreePaths could return incomplete results when it's doing introspect.

Steps to reproduce the issue:
1. Configure phosphor-logging to get max 1000 entries. (with
-Derror_info_cap=3D1000 meson option)
2. Create 1000 logging entries.
3. Call GetSubTreePaths and make sure it gets the correct 1000 entries:
=C2=A0 =C2=A0 # busctl call "xyz.openbmc_project.ObjectMapper" "/xyz/openbmc_project/object_mapper"
"xyz.openbmc_project.ObjectMapper" GetSubTreePaths sias
/xyz/openbmc_project/logging/entry 0 1
xyz.openbmc_project.Logging.Entry | awk '{print $2;}'
=C2=A0 =C2=A0 1000
4. Restart logging service
=C2=A0 =C2=A0 # systemctl restart xyz.openbmc_project.Logging.service
5. After the service is restarted, call GetSubTreePaths for multiple
times in the short time (e.g. within 10 seconds)
=C2=A0 =C2=A0 # busctl call "xyz.openbmc_project.ObjectMapper" "/xyz/openbmc_project/object_mapper"
"xyz.openbmc_project.ObjectMapper" GetSubTreePaths sias
/xyz/openbmc_project/loggiz.openbmc_project.Logging.Entry | awk
'{print $2;}'
=C2=A0 =C2=A0 47
=C2=A0 =C2=A0 # busctl call "xyz.openbmc_project.ObjectMapper" "/xyz/openbmc_project/object_mapper"
"xyz.openbmc_project.ObjectMapper" GetSubTreePaths sias
/xyz/openbmc_project/loggiz.openbmc_project.Logging.Entry | awk
'{print $2;}'
=C2=A0 =C2=A0 375
=C2=A0 =C2=A0 # busctl call "xyz.openbmc_project.ObjectMapper" "/xyz/openbmc_project/object_mapper"
"xyz.openbmc_project.ObjectMapper" GetSubTreePaths sias
/xyz/openbmc_project/loggiz.openbmc_project.Logging.Entry | awk
'{print $2;}'
=C2=A0 =C2=A0 851
=C2=A0 =C2=A0 # busctl call "xyz.openbmc_project.ObjectMapper" "/xyz/openbmc_project/object_mapper"
"xyz.openbmc_project.ObjectMapper" GetSubTreePaths sias
/xyz/openbmc_project/loggiz.openbmc_project.Logging.Entry | awk
'{print $2;}'
=C2=A0 =C2=A0 1000

We can see that the result of GetSubTreePaths is increasing until it gets 1= 000.
This actually happens when mapper is doing introspect of the logging
service, and getting more and more objects.

The above "incomplete" behavior will impact the logic that depend= s on
the result of GetSubTreePaths.
E.g. in ipmid, the "cached SEL" feature depends on the reliable r= esult
of GetSubTreePath, to get the number of current logging entries.

I'm not really following why this is a problem, a= nd sounds like a bug in the cached SEL feature.=C2=A0 Dbus objects can get = added or removed at any time, including logging objects, if the cached sel = feature isn't handling all the added/removed signals as it should, that= really should be fixed.=C2=A0 Can you go into more detail about what this = actually causes?=C2=A0 Is it that logging objects are getting added out of = order?
=C2=A0
If
it's not correct, ipmid will not know the "missed" entries.

What is a "missed" entry in th= is context?
=C2=A0

The question is, should we make sure mapper returns the "stable"<= br> result in the above case?

I don't t= hink so, at least, when it was built, it wasn't designed to return atom= ically created results, but given that ANY object on dbus can be added or r= emoved at any time, it's not clear why this is a problem in the mapper = itself.
=C2=A0
When it's doing introspect of a service (e.g. nameOwnerChanged), it
could throw if the service is not fully introspected, and only return
the "correct" result after the service is fully introspected.=C2= =A0

If mapper could not guarantee the stable result, the service calling
mapper will have to add more complex logic to make sure it gets the
"full and correct" result.

I left comments on your code review as well, but please don't do thi= s.=C2=A0 This is going to force retry code into EVERY service that relies o= n the mapper, at EVERY mapper call site.=C2=A0 If the current behavior is t= ruly a problem, I would much rather the mapper support multi-versioned atom= ically create copies of the dbus-map, and have the mapper expose the "= reliable" interface, rather than spreading that throughout the system.= =C2=A0 For context, bmcweb alone has dozens of call sites to the mapper tha= t would need to be fixed if it were made to be possible for it to fail in t= his way.

=C2=A0

--
BRs,
Lei YU