All of lore.kernel.org
 help / color / mirror / Atom feed
* 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.