openbmc.lists.ozlabs.org archive mirror
 help / color / mirror / Atom feed
* sdbusplus exception type SdBusError
@ 2021-09-02 15:36 Patrick Williams
  0 siblings, 0 replies; only message in thread
From: Patrick Williams @ 2021-09-02 15:36 UTC (permalink / raw)
  To: OpenBMC List

[-- Attachment #1: Type: text/plain, Size: 2400 bytes --]

Hello,

I am going to start on some changes to sdbusplus which make it so that some
errors which previously threw an 'SdBusError' now throw a more specialized type.

In the process of getting started on this I'm observing a few existing issues
across the codebase.

   1. Many repositories were catching 'SdBusError'.  This type was always
      intended to be internal to sdbusplus (in fact, it inherits from an error
      called 'internal_error').  Quite likely, by catching this exception, you
      are missing other exceptions that sdbusplus can throw.  Certainly you are
      missing exceptions that sdbusplus will start throwing with my upcoming
      changes.

   2. I see a handful of repositories *throwing* SdBusErrors.  Again, this was
      intended to be an internal exception to sdbusplus itself and not a generic
      exception type that any arbitrary string could be wrapped with.  I'm going
      to leave this alone for the time being, but in the future it is quite
      likely that I'll force a compile break with this.  If you need to throw
      something that is of type sdbusplus::exception::exception and can't figure
      out what is better, please reach out to me with some context.

   3. Along the lines of #2, *some* of the cases where an SdBusError is thrown
      is entirely invalid error path handling and your application is _GOING_
      _TO_ _CRASH_ whenever you have a dbus client that pushes you down these
      paths!  Almost all of these I see are using the ASIO bindings.  In many
      cases the ASIO bindings *DO NOT* catch any exceptions and magically turn
      them into sd_bus_error's.  By throwing an exception, you're not making an
      error return to the calling client, but instead blowing through all of the
      sd_bus C code with your C++ exception and putting your application into an
      invalid state.  At a minimum you are leaking memory.

*(3): the phosphor-dbus-interface bindings *DO* turn exceptions into
      sd_bus_error responses but typically this requires you to throw a PDI
      error type.

I have put up a large number of commits to fix #1, so if all you are doing is
catching SdBusError you probably have no change necessary.  If you see a 
`catch (SdBusError&)` being added to the codebase, know it is probably wrong and
should be fixed in review.

-- 
Patrick Williams

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] only message in thread

only message in thread, other threads:[~2021-09-02 15:37 UTC | newest]

Thread overview: (only message) (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-02 15:36 sdbusplus exception type SdBusError Patrick Williams

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).