[phosphor-logging] About the "Stop emitting Entry propChanged before ifacesAdded" change reason

[Bills, Jason M]

On 11/4/2021 3:19 PM, Patrick Williams wrote:
> On Thu, Nov 04, 2021 at 12:39:05PM -0700, Ed Tanous wrote:
>> On Wed, Oct 20, 2021 at 10:18 AM Patrick Williams <patrick at stwcx.xyz> wrote:
>>>
>>> On Wed, Oct 20, 2021 at 10:13:06AM -0500, Matt Spinler wrote:
>>>> values, and then explicitly emits the IA signal.   Others can chime in,
>>>> but I didn't see it as proper D-Bus behavior to emit propertiesChanged
>>>> before InterfacesAdded, since in fact no property is changing after the
>>>> interface was added.
>>>
>>> Correct.  PropertiesChanged signals should not show up before InterfacesAdded.
>>
>> But they should still show up eventually (after the InterfacesAdded),
>> right?
> 
> I'm not positive what you're asking.  Does it happen or should it be done?  What
> I tried to describe is what correct behavior would look like.
> 
> My understanding is that if you don't have a service name, no signals will be
> emitted.  I don't recall where I've seen that in code before.
> 
> If you have a service name, but the object does not have an object manager in
> the path, then no InterfacesAdded signals will be emitted.  Many processes put
> this into the root, so this isn't an issue.
> 
> PropertiesChanged and InterfacesAdded are independent from a sd_bus perspective
> (and they belong to two different Interfaces anyhow).  If you call
> sd_bus_emit_properties_changed before calling sd_bus_emit_interfaces_added or
> sd_bus_emit_object_added you will still get signals emitted for the properties
> changed (I confirmed this in the systemd repo).
> 
> There is no queueing or deferring of the PropertiesChanged signals until after
> the InterfacesAdded.
> 
> To me, it does not make any sense to emit signals for PropertiesChanged prior to
> actually informing the world that the interface exists via InterfacesAdded.
> You'll end up sending out signals for properties which nobody even knows they
> exist anyhow.
> 
> It is _also_ a bad idea to send out InterfacesAdded signals before you have
> fully constructed and initialized your object.  This can cause other processes
> to act on the property information they received via the InterfacesAdded signal
> but with half-constructed / invalid property state.
> 
>> The patchset here seems to be under the impression that
>> PropertyChanged is never emitted when the object is added, which seems
>> incorrect, or at the very least is a breaking behavioral change.
>> https://gerrit.openbmc-project.xyz/c/openbmc/bmcweb/+/48231
> 
> The PropertyChanged signal is only emitted if the property changes *after* the
> object was fully formed (by sending out its InterfacesAdded).  The
> PropertyChanged signals are not queued up from the time the object is first
> added until after InterfacesAdded is emitted, which is what it sort of sounded
> like you were implying might happen.
> 
> I'm not sure why you are saying it is breaking behavioral change, other than it
> might have use to work this way, but that way it use to work didn't make any
> sense from a dbus perspective.  This proposed change in bmcweb seems
> conceptually reasonable.
> 
I'm not sure if it's what we're talking about here with the behavioral 
change, but in sdbusplus when a new interface is initialized, by default 
it will also send a PropertiesChanged signal for newly added properties.

There is a 'skipPropertyChangedSignal' parameter that can be set to 
'true' to skip the ProperitesChanged and only send InterfacesAdded:
bool initialize(const bool skipPropertyChangedSignal = false)

https://github.com/openbmc/sdbusplus/blob/master/include/sdbusplus/asio/object_server.hpp#L740

I think there are some components that depend on the default behavior 
and only watch for PropertiesChanged rather than InterfacesAdded.