Reg sdbusplus - async handlers - D-Bus error not getting reflected

Reg sdbusplus - async handlers - D-Bus error not getting reflected

[Thomaiyar, Richard Marian]

Hi,

In sdbusplus code, async_send_handler callback() uses the 
sdbusplus::message::message.get_errorno() 
https://github.com/openbmc/sdbusplus/blob/master/sdbusplus/asio/detail/async_send_handler.hpp#L66 
function to instantiate the boost::system::error_code. Unfortunately, 
none of our D-Bus exception throwing functionality sets the error_no to 
a proper one, but instead updates only the error_message field 
https://github.com/openbmc/sdbusplus/blob/master/tools/sdbusplus/templates/method.mako.prototype.hpp.in#L171 
causing the error_code instance to always return a generic error code 
value, instead of a proper one. Because of this applications which uses 
the asio logic will not be able to differentiate between exceptions 
thrown / errors from D-Bus.

Planning to fix the same in 2 step,

1. To make all D-Bus exception to set the errorno properly using 
sd_bus_error_set_errno and getting the error from the yaml if available 
else return generic one (so that no change in existing daemon will be 
required).

2. make change in async_send_handler_callaback, such that derived class 
of boost::system::error_code is returned, which will hold the error 
message too. Any daemon which uses asio logic, can depend on ec.value() 
as primary exception identifier or ec.message() for any detailed 
exception thrown.

Let me know your thoughts , and if agree, will start implementing the same.

Regards,

Richard

Re: Reg sdbusplus - async handlers - D-Bus error not getting reflected

[Lei YU]

On Wed, Jun 26, 2019 at 12:19 AM Thomaiyar, Richard Marian
<richard.marian.thomaiyar@linux.intel.com> wrote:
>
> Hi,
>
> In sdbusplus code, async_send_handler callback() uses the
> sdbusplus::message::message.get_errorno()
> https://github.com/openbmc/sdbusplus/blob/master/sdbusplus/asio/detail/async_send_handler.hpp#L66
> function to instantiate the boost::system::error_code. Unfortunately,
> none of our D-Bus exception throwing functionality sets the error_no to
> a proper one, but instead updates only the error_message field
> https://github.com/openbmc/sdbusplus/blob/master/tools/sdbusplus/templates/method.mako.prototype.hpp.in#L171
> causing the error_code instance to always return a generic error code
> value, instead of a proper one. Because of this applications which uses
> the asio logic will not be able to differentiate between exceptions
> thrown / errors from D-Bus.
>
> Planning to fix the same in 2 step,
>
> 1. To make all D-Bus exception to set the errorno properly using
> sd_bus_error_set_errno and getting the error from the yaml if available
> else return generic one (so that no change in existing daemon will be
> required).
>
> 2. make change in async_send_handler_callaback, such that derived class
> of boost::system::error_code is returned, which will hold the error
> message too. Any daemon which uses asio logic, can depend on ec.value()
> as primary exception identifier or ec.message() for any detailed
> exception thrown.
>
> Let me know your thoughts , and if agree, will start implementing the same.
>
> Regards,
>
> Richard

I am glad that you feel the same way!
The patch https://gerrit.openbmc-project.xyz/c/openbmc/sdbusplus/+/21611 was
submitted trying to resolve the issue.
And during review, it's noticed that there is a better way to achieve the goal,
that the DBus error is returned by the message.

So we have https://gerrit.openbmc-project.xyz/c/openbmc/sdbusplus/+/22481

Now in sdbusplus, message::get_error() is added, and the caller could get the
sd_bus_error* from the returned message, and get the DBus exception details
from the sd_bus_error.

See examples at
https://github.com/openbmc/bmcweb/blob/e4a4b9a95622b8e1c1bae93718699ad19f4882ac/include/openbmc_dbus_rest.hpp#L1385-L1403
https://github.com/openbmc/bmcweb/blob/e4a4b9a95622b8e1c1bae93718699ad19f4882ac/include/openbmc_dbus_rest.hpp#L1882-L1893

Re: Reg sdbusplus - async handlers - D-Bus error not getting reflected

[Thomaiyar, Richard Marian]

Hi Lei,

https://gerrit.openbmc-project.xyz/c/openbmc/sdbusplus/+/21611  is exactly what i had in mind for #2 but still looking for #1, so that error_no can be propagated, at this point of time, it is not set from D-Bus daemon.

https://gerrit.openbmc-project.xyz/c/openbmc/sdbusplus/+/22481 - is merged, and it solves async_send, but doesn't work for async_method_call(). Any situation in which you have used in async_method_call() ?

regards,
Richard

On 6/26/2019 12:40 PM, Lei YU wrote:
> On Wed, Jun 26, 2019 at 12:19 AM Thomaiyar, Richard Marian
> <richard.marian.thomaiyar@linux.intel.com> wrote:
>> Hi,
>>
>> In sdbusplus code, async_send_handler callback() uses the
>> sdbusplus::message::message.get_errorno()
>> https://github.com/openbmc/sdbusplus/blob/master/sdbusplus/asio/detail/async_send_handler.hpp#L66
>> function to instantiate the boost::system::error_code. Unfortunately,
>> none of our D-Bus exception throwing functionality sets the error_no to
>> a proper one, but instead updates only the error_message field
>> https://github.com/openbmc/sdbusplus/blob/master/tools/sdbusplus/templates/method.mako.prototype.hpp.in#L171
>> causing the error_code instance to always return a generic error code
>> value, instead of a proper one. Because of this applications which uses
>> the asio logic will not be able to differentiate between exceptions
>> thrown / errors from D-Bus.
>>
>> Planning to fix the same in 2 step,
>>
>> 1. To make all D-Bus exception to set the errorno properly using
>> sd_bus_error_set_errno and getting the error from the yaml if available
>> else return generic one (so that no change in existing daemon will be
>> required).
>>
>> 2. make change in async_send_handler_callaback, such that derived class
>> of boost::system::error_code is returned, which will hold the error
>> message too. Any daemon which uses asio logic, can depend on ec.value()
>> as primary exception identifier or ec.message() for any detailed
>> exception thrown.
>>
>> Let me know your thoughts , and if agree, will start implementing the same.
>>
>> Regards,
>>
>> Richard
> I am glad that you feel the same way!
> The patch https://gerrit.openbmc-project.xyz/c/openbmc/sdbusplus/+/21611 was
> submitted trying to resolve the issue.
> And during review, it's noticed that there is a better way to achieve the goal,
> that the DBus error is returned by the message.
>
> So we have https://gerrit.openbmc-project.xyz/c/openbmc/sdbusplus/+/22481
>
> Now in sdbusplus, message::get_error() is added, and the caller could get the
> sd_bus_error* from the returned message, and get the DBus exception details
> from the sd_bus_error.
>
> See examples at
> https://github.com/openbmc/bmcweb/blob/e4a4b9a95622b8e1c1bae93718699ad19f4882ac/include/openbmc_dbus_rest.hpp#L1385-L1403
> https://github.com/openbmc/bmcweb/blob/e4a4b9a95622b8e1c1bae93718699ad19f4882ac/include/openbmc_dbus_rest.hpp#L1882-L1893

Re: Reg sdbusplus - async handlers - D-Bus error not getting reflected

[Lei YU]

On Wed, Jun 26, 2019 at 11:22 PM Thomaiyar, Richard Marian
<richard.marian.thomaiyar@linux.intel.com> wrote:
>
> Hi Lei,
>
> https://gerrit.openbmc-project.xyz/c/openbmc/sdbusplus/+/21611  is exactly what i had in mind for #2 but still looking for #1, so that error_no can be propagated, at this point of time, it is not set from D-Bus daemon.
>
> https://gerrit.openbmc-project.xyz/c/openbmc/sdbusplus/+/22481 - is merged, and it solves async_send, but doesn't work for async_method_call(). Any situation in which you have used in async_method_call() ?

Right, it won't work for async_method_call() due to it only returns
boost::system::error_code and data, no sdbusplus::message at all.
I am not a usual user of async_method_call() so do not meet a situation needing
to get the dbus error.
But it's true that it could be better for async_method_call() to get a proper
dbus error.

It could be achieved either by adding a custom boost::system::error_code (but
it's tricky and not well tested), or possibly adding sdbusplus::message or
sd_bus_error in the returned values (but it breaks the current interface).

@Ed @Vernon Do you have any suggestions?