is_method_error versus call() errors

[Kun Yi]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ozlabs.org/pipermail/openbmc/attachments/20180910/06e69f4b/attachment.html>