Skip to content
This repository was archived by the owner on Sep 1, 2025. It is now read-only.

Conversation

brgl
Copy link
Owner

@brgl brgl commented Jul 18, 2024

This is a pull-request mirroring the patch series sent to linux-gpio created for the convenience of reviewers preferring github.

Big thanks to Philip Withnall <philip@tecnocode.co.uk> for his thorough review
of this series. I think I addressed most of the issues pointed out.

This series introduces the D-Bus API definition and its implementation in the
form of a GPIO manager daemon and a companion command-line client as well as
GLib bindings to libgpiod which form the base on which the former are built.

While I split the GLib and D-Bus code into several commits for easier review,
I intend to apply all changes to bindings/glib/ and dbus/ as two big commits
in the end as otherwise the split commits are not buildable until all of them
are applied.

The main point of interest is the D-Bus interface definition XML at
dbus/lib/io.gpiod1.xml as it is what defines the actual D-Bus API. Everything
else can be considered as implementation details as it's easier to change
later than the API that's supposed to be stable once released.

The first two patches expose the test infrastructure we use for the core
library and tools to the GLib bindings and dbus code. Next we add the GLib
bindings themselves. Not much to discuss here, they cover the entire libgpiod
API but wrap it in GObject abstractions and plug into the GLib event loop.

Finally we add the D-Bus code that's split into the daemon and command-line
client. I added some examples to the README and documented the behavior in
the help text of the programs as well as documented the interface file with
XML comments that gdbus-codegen can parse and use to generate docbook output.

For D-Bus, most of the testing happens in the command-line client bash tests.
It has a very good coverage of the daemon's code and also allows to run the
daemon through valgrind and verify there are no memory leaks and invalid
accesses. I still intend to extend the C test-suite for D-Bus with some corner
cases but didn't not have enough time for it.

Changes in v3:

  • make gpio-manager run as its own user in the systemd service file and add
    udev rules that automate the group assignment for gpiochips
  • add sandboxing options to the service file for an overall exposure score
    from systemd-analyze of 2.3
  • enable introspection for GLib bindings
  • set the minimum required GLib version for gdbus-codegen
  • fix time units in dbus docs
  • change the D-Bus type for Chip's path to byte-array
  • change the naming convention in strings: s/DBus/D-Bus/g
  • add the "unknown" value to the EventClock property and document how to
    interpret other unrecognized values
  • various doc updates
  • don't set environment variables from the daemon code, use the provided
    g_log_writer_default_set_debug_domains() helper
  • use g_build_filename() where appropriate
  • use g_steal_pointer() to improve error propagation
  • use G_PARAM_STATIC_STRINGS across all properties
  • use G_GNUC_PRINTF() in g_gpiod_set_error_from_errno()
  • change the library's namespace to Gpiodglib/GPIODGLIB/gpiodglib_
  • remove the "handle" properties in favor of passing the core libgpiod pointers
    to GObjects directly after they're constructed
  • add typedefs to property enums for better build-time safety
  • don't use g_value_set_static_string() for strings that are not really static
    across the entire lifetime of the program
  • rework the code for internal property setting and getting
  • add Requires.private: libgpiod to gpiod-glib pkgconfig file
  • Link to v2: https://lore.kernel.org/r/20240628-dbus-v2-0-c1331ac17cb8@linaro.org

Changes in v2:

  • fixed most segfaults I noticed (or was made aware of by others) in RFC
  • improve the code in GLib examples
  • make command-line tests pass shellckeck
  • fix build issue resulting in implicit pointer-to-int casting on some
    platforms
  • many small tweaks, fixes and improvements all over the place but without
    changing the API
  • fix a bunch of memory leaks reported by valgrind
  • Link to v1: https://lore.kernel.org/linux-gpio/20240412122804.109323-1-brgl@bgdev.pl/

@brgl brgl force-pushed the b4/dbus branch 2 times, most recently from ba0ded6 to f949597 Compare July 19, 2024 12:03
Bartosz Golaszewski added 3 commits August 1, 2024 11:42
- EDITME: describe what is new in this series revision.
- EDITME: use bulletpoints and terse descriptions.
- Link to v5: https://lore.kernel.org/r/20240812-dbus-v5-0-ead288509217@linaro.org

dbus: add GLib-based D-Bus daemon and command-line client

I'm resending it once more but with commits squashed into how they'll appear
in git once applied upstream. I think the code is in good enough shape that
it can now go into the master branch and any further development can happen
from there.

Big thanks to Philip Withnall <philip@tecnocode.co.uk> for his thorough review
of this series. I think I addressed most of the issues pointed out.

This series introduces the D-Bus API definition and its implementation in the
form of a GPIO manager daemon and a companion command-line client as well as
GLib bindings to libgpiod which form the base on which the former are built.

While I split the GLib and D-Bus code into several commits for easier review,
I intend to apply all changes to bindings/glib/ and dbus/ as two big commits
in the end as otherwise the split commits are not buildable until all of them
are applied.

The main point of interest is the D-Bus interface definition XML at
dbus/lib/io.gpiod1.xml as it is what defines the actual D-Bus API. Everything
else can be considered as implementation details as it's easier to change
later than the API that's supposed to be stable once released.

The first two patches expose the test infrastructure we use for the core
library and tools to the GLib bindings and dbus code. Next we add the GLib
bindings themselves. Not much to discuss here, they cover the entire libgpiod
API but wrap it in GObject abstractions and plug into the GLib event loop.

Finally we add the D-Bus code that's split into the daemon and command-line
client. I added some examples to the README and documented the behavior in
the help text of the programs as well as documented the interface file with
XML comments that gdbus-codegen can parse and use to generate docbook output.

For D-Bus, most of the testing happens in the command-line client bash tests.
It has a very good coverage of the daemon's code and also allows to run the
daemon through valgrind and verify there are no memory leaks and invalid
accesses.

Changes in v5:
- squash GLib bindings and D-Bus commits into two big commits
- Link to v4: https://lore.kernel.org/r/20240807-dbus-v4-0-64ea80169e51@linaro.org

Changes in v4:
- fix generating GObject introspection data
- use GLib doc blocks with introspection annotations suitable for generating
  docs with gi-docgen
- various comment and doc tweaks
- Link to v3: https://lore.kernel.org/r/20240718-dbus-v3-0-c9ea2604f082@linaro.org

Changes in v3:
- make gpio-manager run as its own user in the systemd service file and add
  udev rules that automate the group assignment for gpiochips
- add sandboxing options to the service file for an overall exposure score
  from systemd-analyze of 2.3
- enable introspection for GLib bindings
- set the minimum required GLib version for gdbus-codegen
- fix time units in dbus docs
- change the D-Bus type for Chip's path to byte-array
- change the naming convention in strings: s/DBus/D-Bus/g
- add the "unknown" value to the EventClock property and document how to
  interpret other unrecognized values
- various doc updates
- don't set environment variables from the daemon code, use the provided
  g_log_writer_default_set_debug_domains() helper
- use g_build_filename() where appropriate
- use g_steal_pointer() to improve error propagation
- use G_PARAM_STATIC_STRINGS across all properties
- use G_GNUC_PRINTF() in g_gpiod_set_error_from_errno()
- change the library's namespace to Gpiodglib/GPIODGLIB/gpiodglib_
- remove the "handle" properties in favor of passing the core libgpiod pointers
  to GObjects directly after they're constructed
- add typedefs to property enums for better build-time safety
- don't use g_value_set_static_string() for strings that are not really static
  across the entire lifetime of the program
- rework the code for internal property setting and getting
- add Requires.private: libgpiod to gpiod-glib pkgconfig file
- Link to v2: https://lore.kernel.org/r/20240628-dbus-v2-0-c1331ac17cb8@linaro.org

Changes in v2:
- fixed most segfaults I noticed (or was made aware of by others) in RFC
- improve the code in GLib examples
- make command-line tests pass shellckeck
- fix build issue resulting in implicit pointer-to-int casting on some
  platforms
- many small tweaks, fixes and improvements all over the place but without
  changing the API
- fix a bunch of memory leaks reported by valgrind
- Link to v1: https://lore.kernel.org/linux-gpio/20240412122804.109323-1-brgl@bgdev.pl/

--- b4-submit-tracking ---
# This section is used internally by b4 prep for tracking purposes.
{
  "series": {
    "revision": 6,
    "change-id": "20240527-dbus-820e9f7463d0",
    "prefixes": [
      "libgpiod"
    ],
    "history": {
      "v2": [
        "20240628-dbus-v2-0-e42336efe2d3@linaro.org",
        "20240628-dbus-v2-0-c1331ac17cb8@linaro.org"
      ],
      "v3": [
        "20240718-dbus-v3-0-c9ea2604f082@linaro.org"
      ],
      "v4": [
        "20240807-dbus-v4-0-64ea80169e51@linaro.org"
      ],
      "v5": [
        "20240812-dbus-v5-0-ead288509217@linaro.org"
      ]
    }
  }
}
In order to allow the upcoming GLib and DBus bindings to reuse the test
code, let's put all common elements into reusable libtool objects and
export the relevant symbols in internal headers.

Tested-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
In order to allow the upcoming DBus command-line client tests to reuse the
existing bash test harness, let's put the common code into an importable
file and rename run_tool to run_prog to reflect that it now can run any
program.

Tested-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
@brgl brgl force-pushed the b4/dbus branch 6 times, most recently from 994d378 to 43d7bfc Compare August 7, 2024 09:19
Bartosz Golaszewski added 2 commits August 8, 2024 11:00
Implement GObject-based GLib bindings for libgpiod. Include generating
GObject introspection data, tests and examples.

Tested-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Add the D-Bus API definition and its implementation in the form of a GPIO
manager daemon and a companion command-line client as well as some
additional configuration and data files (systemd service, example udev
configuration, etc.) and test suites.

Tested-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
@brgl brgl force-pushed the b4/dbus branch 2 times, most recently from 4a09b15 to e5a4ae0 Compare August 12, 2024 08:29
@brgl brgl closed this Aug 13, 2024
@brgl brgl deleted the b4/dbus branch August 13, 2024 08:50
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant