* is_method_error versus call() errors
@ 2018-09-10 16:17 Patrick Venture
2018-09-10 21:49 ` William Kennington
0 siblings, 1 reply; 6+ messages in thread
From: Patrick Venture @ 2018-09-10 16:17 UTC (permalink / raw)
To: OpenBMC Maillist
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: is_method_error versus call() errors 2018-09-10 16:17 is_method_error versus call() errors Patrick Venture @ 2018-09-10 21:49 ` William Kennington 2018-09-11 0:13 ` Kun Yi 2018-09-11 9:51 ` Thomaiyar, Richard Marian 0 siblings, 2 replies; 6+ messages in thread From: William Kennington @ 2018-09-10 21:49 UTC (permalink / raw) To: Patrick Venture; +Cc: openbmc 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@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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: is_method_error versus call() errors 2018-09-10 21:49 ` William Kennington @ 2018-09-11 0:13 ` Kun Yi 2018-09-11 7:44 ` William Kennington 2018-09-11 9:51 ` Thomaiyar, Richard Marian 1 sibling, 1 reply; 6+ messages in thread From: Kun Yi @ 2018-09-11 0:13 UTC (permalink / raw) To: William Kennington; +Cc: Patrick Venture, OpenBMC Maillist [-- Attachment #1: Type: text/plain, Size: 3272 bytes --] 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@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@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 [-- Attachment #2: Type: text/html, Size: 4323 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: is_method_error versus call() errors 2018-09-11 0:13 ` Kun Yi @ 2018-09-11 7:44 ` William Kennington 0 siblings, 0 replies; 6+ messages in thread From: William Kennington @ 2018-09-11 7:44 UTC (permalink / raw) To: Kun Yi; +Cc: Patrick Venture, openbmc 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@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@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@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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: is_method_error versus call() errors 2018-09-10 21:49 ` William Kennington 2018-09-11 0:13 ` Kun Yi @ 2018-09-11 9:51 ` Thomaiyar, Richard Marian 2018-09-11 21:40 ` William Kennington 1 sibling, 1 reply; 6+ messages in thread From: Thomaiyar, Richard Marian @ 2018-09-11 9:51 UTC (permalink / raw) To: William Kennington, Patrick Venture; +Cc: openbmc In sdbus++ generated code we end up setting error_const for any issue (i.e. in the D-Bus daemon / server end). catch(sdbusplus::xyz::openbmc_project::Common::Error::InvalidArgument& e) { sd_bus_error_set_const(error, e.name(), e.description()); return -EINVAL; } and in our sdbusplus (bus.hpp.in), the code is updated to throw the error auto call(message::message& m, uint64_t timeout_us = 0) { sd_bus_error error = SD_BUS_ERROR_NULL; sd_bus_message* reply = nullptr; int r = _intf->sd_bus_call(_bus.get(), m.get(), timeout_us, &error, &reply); if (r < 0) { throw exception::SdBusError(&error, "sd_bus_call"); } return message::message(reply, _intf, std::false_type()); } i.e. for any error thrown in the D-Bus daemon(server), the client application call() will just throw SdBusError, and not the message object. Hence there is no way to perform is_method_error() on the message right? Also, i believe even before throwing SdBusError(), sd_bus_message_unref(reply) has to be called, to make sure that messages are unref even for errors thrown from D-Bus daemon. Let me know if i misunderstand the logic in current code. Regards, Richard On 9/11/2018 3:19 AM, William Kennington 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@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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: is_method_error versus call() errors 2018-09-11 9:51 ` Thomaiyar, Richard Marian @ 2018-09-11 21:40 ` William Kennington 0 siblings, 0 replies; 6+ messages in thread From: William Kennington @ 2018-09-11 21:40 UTC (permalink / raw) To: richard.marian.thomaiyar; +Cc: Patrick Venture, openbmc I should have looked at the code prior to misremembering how sd_bus handles method errors during sd_bus_call(). If you look at the code, it does in fact return sd_bus_errors for method errors automaticaly. https://github.com/systemd/systemd/blob/v237/src/libsystemd/sd-bus/sd-bus.c#L2130 You can ignore the previous patchset I sent adding the method_error checks as they are superfluous. It looks like the only missing convenience is the ignore errors variant of the call_noreply_noexcept. On Tue, Sep 11, 2018 at 2:52 AM Thomaiyar, Richard Marian <richard.marian.thomaiyar@linux.intel.com> wrote: > > In sdbus++ generated code we end up setting error_const for any issue > (i.e. in the D-Bus daemon / server end). > > catch(sdbusplus::xyz::openbmc_project::Common::Error::InvalidArgument& e) > { > sd_bus_error_set_const(error, e.name(), e.description()); > return -EINVAL; > } > > and in our sdbusplus (bus.hpp.in), the code is updated to throw the error > > auto call(message::message& m, uint64_t timeout_us = 0) > { > sd_bus_error error = SD_BUS_ERROR_NULL; > sd_bus_message* reply = nullptr; > int r = _intf->sd_bus_call(_bus.get(), m.get(), timeout_us, &error, > &reply); > if (r < 0) > { > throw exception::SdBusError(&error, "sd_bus_call"); > } > > return message::message(reply, _intf, std::false_type()); > } > > i.e. for any error thrown in the D-Bus daemon(server), the client > application call() will just throw SdBusError, and not the message > object. Hence there is no way to perform is_method_error() on the > message right? > > Also, i believe even before throwing SdBusError(), > sd_bus_message_unref(reply) has to be called, to make sure that messages > are unref even for errors thrown from D-Bus daemon. > > Let me know if i misunderstand the logic in current code. > > Regards, > > Richard > > > On 9/11/2018 3:19 AM, William Kennington 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@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 > ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-09-11 21:41 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-09-10 16:17 is_method_error versus call() errors Patrick Venture 2018-09-10 21:49 ` William Kennington 2018-09-11 0:13 ` Kun Yi 2018-09-11 7:44 ` William Kennington 2018-09-11 9:51 ` Thomaiyar, Richard Marian 2018-09-11 21:40 ` William Kennington
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.