[Maintainers] Call for maintenance

[Maintainers] Call for maintenance

[Brad Bishop]

I wanted to highlight a recent sdbusplus patch:

https://gerrit.openbmc-project.xyz/#/c/10566/

The nutshell is proper error handling of errors from libsystemd's
sd_bus_call has been added and sdbusplus::bus::call may now throw
SdBusError.

This note is an open call to all maintainers to scrub your applications
for use of sdbusplus::bus::call and handle SdBusError as appropriate.

I want to point out that catching SdBusError in your wrapper function
around sdbusplus::bus::call and simply eating it (or logging it, tracing
it, etc) is not really “handling” this new exception.  Nor is wrapping
your entire program in a try block “handling” it either.  Instead,
traverse the stack until you find the code with the logic that knows
whether or not the error is anticipated or not, and handle it there.  

For example:

https://gerrit.openbmc-project.xyz/#/c/11423/3/src/propertywatchimpl.hpp

thx - brad

Re: [Maintainers] Call for maintenance

[Tom Joseph]

Hello,

I am scrubbing the phosphor-host-ipmid to handle the SdBusError exception.

Even though sdbusplus::exception::SdBusError is inherited from 
std::exception,
my observation is that catch statement of std::exception does not catch
sdbusplus::exception::SdBusError exception.

try
{
     // Statement throwing sdbusplus::exception::SdBusError exception.
}
catch (const std::exception& e) // Does not catch 
sdbusplus::exception::SdBusError
{

}

I think the multiple inheritance from std::exception is causing this.

Regards,
Tom

On Thursday 12 July 2018 11:22 PM, Brad Bishop wrote:
> I wanted to highlight a recent sdbusplus patch:
>
> https://gerrit.openbmc-project.xyz/#/c/10566/
>
> The nutshell is proper error handling of errors from libsystemd's
> sd_bus_call has been added and sdbusplus::bus::call may now throw
> SdBusError.
>
> This note is an open call to all maintainers to scrub your applications
> for use of sdbusplus::bus::call and handle SdBusError as appropriate.
>
> I want to point out that catching SdBusError in your wrapper function
> around sdbusplus::bus::call and simply eating it (or logging it, tracing
> it, etc) is not really “handling” this new exception.  Nor is wrapping
> your entire program in a try block “handling” it either.  Instead,
> traverse the stack until you find the code with the logic that knows
> whether or not the error is anticipated or not, and handle it there.
>
> For example:
>
> https://gerrit.openbmc-project.xyz/#/c/11423/3/src/propertywatchimpl.hpp
>
> thx - brad

Re: [Maintainers] Call for maintenance

[Lei YU]

On Tue, Jul 31, 2018 at 10:21 PM Tom Joseph <tomjose@linux.vnet.ibm.com> wrote:
>
> Hello,
>
> I am scrubbing the phosphor-host-ipmid to handle the SdBusError exception.
>
> Even though sdbusplus::exception::SdBusError is inherited from
> std::exception,
> my observation is that catch statement of std::exception does not catch
> sdbusplus::exception::SdBusError exception.
>
> try
> {
>      // Statement throwing sdbusplus::exception::SdBusError exception.
> }
> catch (const std::exception& e) // Does not catch
> sdbusplus::exception::SdBusError
> {
>
> }
>
> I think the multiple inheritance from std::exception is causing this.
>

Yup, I meet the same issue.
The root cause is multiple inheritance of std::exception:

    struct exception : public std::exception;
    struct internal_exception : public exception;
    class SdBusError final : public internal_exception, public
std::system_error;

SdBusError inherits both internal_exception and std::system_error,
which both inherit std::exception.
We can not use virtual inheritance here because std::system_error
already non-virtually inherits std::exception.

So probably we should not use diamond inheritance here...

Re: [Maintainers] Call for maintenance

[Vernon Mauery]

On 01-Aug-2018 10:13 AM, Lei YU wrote:
>On Tue, Jul 31, 2018 at 10:21 PM Tom Joseph <tomjose@linux.vnet.ibm.com> wrote:
>>
>> Hello,
>>
>> I am scrubbing the phosphor-host-ipmid to handle the SdBusError exception.
>>
>> Even though sdbusplus::exception::SdBusError is inherited from
>> std::exception,
>> my observation is that catch statement of std::exception does not catch
>> sdbusplus::exception::SdBusError exception.
>>
>> try
>> {
>>      // Statement throwing sdbusplus::exception::SdBusError exception.
>> }
>> catch (const std::exception& e) // Does not catch
>> sdbusplus::exception::SdBusError
>> {
>>
>> }
>>
>> I think the multiple inheritance from std::exception is causing this.
>>
>
>Yup, I meet the same issue.
>The root cause is multiple inheritance of std::exception:
>
>    struct exception : public std::exception;
>    struct internal_exception : public exception;
>    class SdBusError final : public internal_exception, public
>std::system_error;
>
>SdBusError inherits both internal_exception and std::system_error,
>which both inherit std::exception.
>We can not use virtual inheritance here because std::system_error
>already non-virtually inherits std::exception.
>
>So probably we should not use diamond inheritance here...

I just submitted a patch for review. This allows SdBusError to be caught 
via std::exception:

https://gerrit.openbmc-project.xyz/#/c/openbmc/sdbusplus/+/11726/

There are still packages that may not even be handling std::exceptions 
around sdbusplus calls, but that is a different problem. :)

--Vernon