is_method_error versus call() errors

[William Kennington]

Personally, I'd be more inclined to create helpers that wrap those
common paradigms expressively. I wrote a quick patch series that adds
these helpers. Let me know what you think.
https://gerrit.openbmc-project.xyz/#/c/openbmc/sdbusplus/+/12670/On
Mon, Sep 10, 2018 at 5:14 PM Kun Yi <kunyi at google.com> wrote:
>
> I think correspondingly we should start adding unit tests that checks our code are exception safe.
>
> Meanwhile, in some cases we don't really care from which level the error originates. Does it make sense to have a macro like:
>
> #define IS_ERROR_OR_EXCEPTION(dbus_call) \
>   try { \
>     return (dbus_call).is_method_error(); \
>   } catch (const sdbusplus::exception::SdBusError& ex) { \
>     return true; \
>   }
>
> On Mon, Sep 10, 2018 at 2:50 PM William Kennington <wak at google.com> wrote:
>>
>> This is something that is a little confusing about the semantics of
>> sdbus. When using sd_bus_call(), it will only populate the returned
>> message pointer if the dbus rpc succeeds. In the case where something
>> fails at the dbus level, the sd_bus_call() method fails and the
>> message is never populated. If we don't check for errors on
>> sd_bus_call() but use sd_bus_message_is_method_error on the NULL
>> message, we get an -EINVAL and treat that as an error. This behavior
>> works fine if you only care that you had an error, but don't need to
>> interpret the error and perform special case handling depending on the
>> type. This is why I added the exceptions to sd_bus_call() so that we
>> can parse out specific errors like -EBUSY or -EINTR, but handle the
>> rest differently.
>>
>> sd_bus_message_is_method_error() is supposed to be used for checking
>> if the server we are talking to sent an error response, not if the
>> dbus layer had some kind of error. These errors are application
>> specific and can be parsed out of a valid message if they exist.
>> sd_bus_call() will still return success even if the server method sent
>> a METHOD_ERROR type of message. You can't technically rely on
>> sd_bus_message_read returning an error when reading from a
>> METHOD_ERROR type of message. Practically though if your METHOD_ERROR
>> message never have any content the read will fail since the message
>> won't contain the expected data types. Unfortunately this means we
>> will should keep all of the is_method_error checks and make sure c++
>> exception handling is in place for all of the daemons.On Mon, Sep 10,
>> 2018 at 9:18 AM Patrick Venture <venture at google.com> wrote:
>> >
>> > Given:
>> >
>> > _public_ int sd_bus_message_is_method_error(sd_bus_message *m, const
>> > char *name) {
>> >         assert_return(m, -EINVAL);
>> >         if (m->header->type != SD_BUS_MESSAGE_METHOD_ERROR)
>> >                 return 0;
>> >         if (name && (!m->error.name || !streq(m->error.name, name)))
>> >                 return 0;
>> >         return 1;
>> > }
>> >
>> > The failures from call() don't always set the reply (maybe don't ever,
>> > but I didn't read it closely).  Which means, is_method_error() can
>> > happen when the bus call itself was fine, so we need to check both IFF
>> > we're going to read the response.
>> >
>> > I'm working on eliminating the new crashes from phosphor-host-ipmid
>> > and other repositories, now that sdbusplus can except, which is not
>> > caught even though there's an error case for is_method_error()
>> > immediately after the call.
>> >
>> > In phosphor-pid-control, I dropped the is_method_error() check, but I
>> > think it's still valid before trying to read() a message -- so I will
>> > likely throw a patch adding that check.
>> >
>> > Patrick
>
>
>
> --
> Regards,
> Kun