On Tue, Oct 06, 2020 at 03:57:30PM -0700, Sui Chen wrote:
> On Fri, Oct 2, 2020 at 1:54 PM Vijay Khemka <vijaykhemka@fb.com> wrote:
> > If I understand correctly, protocol buffer will be used by daemon who
> > Is responding to the IPMI request and connecting to this daemon via
> > library call, then it is completely restricted for the use of protocol buffer.
> > If you are passing protocol buffer to this daemon then we have to define
> > some policy here.
> 
> The Protocol buffer is only for serializing the data to be sent
> outside of the BMC. It is not used for communication inside
> phosphor-health-monitor and will not be passed to the daemon.

Why isn't this part done from within an existing IPMI provider (ideally
to me a google-ipmi-* repository at this time)?  I'm not especially keen
on these details leaking out into other non-IPMI repositories.

> >
> >     Other than these two things I think adding new metrics to
> >     phosphor-health-monitor should be manageable. I can start by trying to
> >     add the IPMI blob handler to phosphor-health-monitor; my first attempt
> >     might not look very elegant, but if we find answers to the two
> >     questions above, the merged result will look a lot better. Hopefully
> >     we can find a solution that works well for everyone.
> >
> > I am looking forward to your patches
> 
> Please check out this WIP:
> https://gerrit.openbmc-project.xyz/c/openbmc/phosphor-health-monitor/+/37092
> 
> This WIP currently just adds the IPMI blob-based code to
> phosphor-health-monitor almost as-is.
> It also shows what we already have now.
> 
> There will be some work to merge the daemon and the blob handler in an
> organic way, and I am open to discussion with you how to do that. The
> first step I think I can do is to put the code for extracting the
> metrics (metrics.cpp, blob/metric.cpp) into a single file and share
> that between the daemon and the IPMI blob handler.
> 
> Another issue I found is I am not using the latest sdbusplus so I have
> to comment out the usage of ValueIface::Unit::Percent for now.
> 
> To build this requires 1) adding a pkgconfig file to
> phosphor-ipmi-blobs (before
> https://gerrit.openbmc-project.xyz/c/openbmc/phosphor-ipmi-blobs/+/37133
> gets merged) and 2) adding phosphor-ipmi-blobs and protobuf to DEPENDS
> in phosphor-health-monitor's Bitbake recipe.
> 
> Hope this WIP change illustrates our intention clearly.
> 
> Thanks!

-- 
Patrick Williams