openbmc.lists.ozlabs.org archive mirror
 help / color / mirror / Atom feed
From: Patrick Williams <patrick@stwcx.xyz>
To: OpenBMC List <openbmc@lists.ozlabs.org>
Subject: sdbusplus exception type SdBusError
Date: Thu, 2 Sep 2021 10:36:28 -0500	[thread overview]
Message-ID: <YTDvfIn4Z05mGdCx@heinlein> (raw)

[-- 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 --]

                 reply	other threads:[~2021-09-02 15:37 UTC|newest]

Thread overview: [no followups] expand[flat|nested]  mbox.gz  Atom feed

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YTDvfIn4Z05mGdCx@heinlein \
    --to=patrick@stwcx.xyz \
    --cc=openbmc@lists.ozlabs.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).