On Mon, Sep 10, 2018 at 2:50 P= M William Kennington <wak@google.com> wrote:

This is something tha= t 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@google.com> wrote:
>
> Given:
>
> _public_ int sd_bus_message_is_method_error(sd_bus_message *m, const > char *name) {
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0assert_return(m, -EINVAL);
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (m->header->type !=3D SD_BUS= _MESSAGE_METHOD_ERROR)
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return 0;=
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (name && (!m->error.name = || !streq(m->error.name, name)))
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return 0;=
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return 1;
> }
>
> The failures from call() don't always set the reply (maybe don'= ;t ever,
> but I didn't read it closely).=C2=A0 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-ipmi= d
> 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<= br> > think it's still valid before trying to read() a message -- so I w= ill
> likely throw a patch adding that check.
>
> Patrick