On Tue, May 26, 2020 at 06:26:27PM +0530, Deepak Kodihalli wrote:
> On 19/05/20 9:10 am, Deepak Kodihalli wrote:
> To do a) above, there are 3 options:
> 
> 1) Propose a D-Bus interface in phosphor-dbus-interfaces. This was [2] 
> in my original email above. The concern raised by Patrick here is that 
> this interface is very specific to a protocol (PLDM in this case), 
> whereas the phosphor D-Bus interfaces are mostly abstract and protocol 
> agnostic.
> 
> In my opinion, PLDM is also a data model, so PLDM specific D-Bus 
> interfaces can enable two apps that are PLDM aware (for eg a PLDM 
> requester app talking to the PLDM daemon) to talk to each other. I do 
> see other protocol specific D-Bus interfaces as well (for eg related to 
> SMBIOS). So I don't really understand the concern. The protocol specific 
> interfaces do not preclude other abstract interfaces.

After thinking about it for a bit, I think this is my preference with
the design caveat that these are only consumed by processes which are
"FRU-to-Inventory" transformers.  I would suggest putting these
interfaces under the 'Inventory/' namespace somewhere to hopefully make
this clearer.

We have two implementations of these "FRU-to-Inventory" transformers: EM
and PIM.  Both of them have a form of dbus backdoor in order to get the
information they need to create the Inventory objects they host.  PIM uses
`Inventory/Manager`[1] and EM uses an undocumented `FruDevice` interface
between it and 'fru-device'.  Both of these implementations do
processing on the data they get (though, very minimal in the case of
PIM) and create `Inventory/Item`s as a result.

What I am worried about, and Richard seconded in his response, is the
details of PLDM (or any other protocol) starting to leak into other
processes.  We don't want people to notice that there is some piece of
information that isn't currently exposed via Inventory but happens to be
available in PLDM and start coding towards consuming it.  Hence, my
request that the design document the caveat I listed above.  We want to
limit the scope of applications that need to understand specific
protocols.

My suggestion would be to put these new proposed PLDM interfaces under
`Inventory/Source/PLDM`.  Anything under `Source` becomes one of these
"FRU-to-Inventory" transformational interfaces.

> 2) Propose a generic/abstract 'FRU properties' kind of D-Bus interface. 
> This is something that both the PLDM daemon and FRU device daemon could 
> use to host FRU information on D-Bus, and to provide the same as an 
> intermediate FRU format data to apps like EM. The suggestion on the docs 
> commit above [2] was to have an interface like {Enum[Protocol], 
> array[byte]}. I think instead this could be a dict[string, 
> variant[string, int64]], for a FRU property to value mapping.

I don't like arbitrary string-based properties like this.  They aren't
documented and effectively become a private API between the processes.
In order for someone to make a new implementation they have to reverse
engineer the code.

> Thanks,
> Deepak

1. https://github.com/openbmc/phosphor-dbus-interfaces/blob/master/xyz/openbmc_project/Inventory/Manager.interface.yaml

-- 
Patrick Williams