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