i/builtin: add gpio-chardev interface #15172

ZeyadYasser · 2025-03-05T18:04:17Z

This implements the gpio-chardev interface according to the SD129 spec which is currently hidden behind an experimental feature flag until kernel improvements to the gpio-aggreagtor lands.

Also, I had to extend the systemd backend to support specifying Wants/WantedBy/After/Before directives 44562bb.

codecov · 2025-03-05T18:24:14Z

Codecov Report

Attention: Patch coverage is 95.13514% with 9 lines in your changes missing coverage. Please review.

Project coverage is 78.14%. Comparing base (d6d95f0) to head (510fc2f).
Report is 34 commits behind head on master.

Files with missing lines	Patch %	Lines
interfaces/builtin/gpio_chardev.go	95.04%	4 Missing and 2 partials ⚠️
strutil/range.go	94.23%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #15172      +/-   ##
==========================================
+ Coverage   78.09%   78.14%   +0.04%     
==========================================
  Files        1190     1192       +2     
  Lines      158458   159239     +781     
==========================================
+ Hits       123746   124431     +685     
- Misses      27017    27076      +59     
- Partials     7695     7732      +37

Flag	Coverage Δ
unittests	`78.14% <95.13%> (+0.04%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

github-actions · 2025-03-05T19:57:26Z

Fri Mar 21 12:43:06 UTC 2025
The following results are from: https://github.com/canonical/snapd/actions/runs/13990024789

Failures:

Preparing:

google-distro-1:debian-11-64:tests/main/snap-handle-link
google:ubuntu-24.10-64:tests/main/interfaces-pulseaudio
google:ubuntu-24.10-64:tests/main/apparmor-prompting-integration-tests:create_multiple_allow

Executing:

google-distro-1:debian-11-64:tests/main/snap-user-service-restart-on-upgrade
google-distro-1:debian-11-64:tests/main/interfaces-personal-files
google-distro-1:debian-11-64:tests/main/interfaces-desktop
google-distro-1:debian-11-64:tests/main/prepare-image-uboot
google-distro-1:debian-11-64:tests/main/interfaces-mount-observe
google-distro-1:debian-11-64:tests/main/snapshot-cross-revno
google-distro-1:debian-11-64:tests/main/snap-debug-stacktrace
google-distro-1:debian-11-64:tests/main/install-errors:withreexec
google-core:ubuntu-core-20-64:tests/main/interfaces-browser-support:disallow
google-core:ubuntu-core-20-64:tests/main/interfaces-mount-observe
google-core:ubuntu-core-20-64:tests/main/interfaces-browser-support:allow
google-core:ubuntu-core-20-64:tests/smoke/sandbox
google-core:ubuntu-core-20-64:tests/smoke/install
google:ubuntu-20.04-64:tests/main/lxd:snapd_cgroup_just_inside
google:ubuntu-20.04-64:tests/main/progress
google:ubuntu-20.04-64:tests/main/install-errors:withreexec
google:ubuntu-20.04-64:tests/main/cgroup-tracking:test
google:ubuntu-20.04-64:tests/main/desktop-portal-screenshot
google:ubuntu-20.04-64:tests/main/non-home:var
google:ubuntu-20.04-64:tests/main/interfaces-libvirt
google:ubuntu-20.04-64:tests/main/interfaces-polkit
google:ubuntu-20.04-64:tests/main/dbus-activation-session
google:ubuntu-20.04-64:tests/main/interfaces-network-status-classic
google:ubuntu-24.10-64:tests/main/snap-debug-stacktrace
google:ubuntu-24.10-64:tests/main/non-home:home2
google:ubuntu-24.10-64:tests/main/interfaces-shared-memory-private
google:ubuntu-24.10-64:tests/main/interfaces-polkit
google:ubuntu-24.10-64:tests/main/snap-user-service-start-on-install
google:ubuntu-24.10-64:tests/main/snapshot-users
google:ubuntu-24.10-64:tests/main/refresh-app-awareness:classic
google:ubuntu-24.10-64:tests/main/interfaces-requests-activates-handlers
google:ubuntu-24.10-64:tests/main/snap-confine-undesired-mode-group
google:ubuntu-24.10-64:tests/main/install-errors:withreexec
google:ubuntu-24.04-64:tests/main/try

Restoring:

google-distro-1:debian-11-64:tests/main/snap-handle-link
google:ubuntu-24.10-64:tests/main/apparmor-prompting-integration-tests:create_multiple_allow

bboozzoo · 2025-03-06T12:02:16Z

interfaces/builtin/gpio_chardev.go

+	return lines, nil
+}
+
+func validateLines(linesAttr string) error {


this could be made a helper in strutil, e.g.:

// RangeSpan represents a span of numbers inside a range. A span with // equal Start and Stop describes a span of a single number. type RangeSpan struct { Start, Stop uint } // Range of discrete numbers represented as set of non overlapping spans. type Range struct { // Spans within the range, ordered by Start. Spans []RangeSpan } // ParseRange parses a range represented as a string. The entries are // joining them with a comma: n[,m] or as a range: n-m or a combination // of both, assuming the ranges do not overlap, e.g.: n,m,x-y. func ParseRange(in str, expectedMax uint) (Range, error) { ... }

and then you can reuse it in the helper. Separately I'd add something to the gadget package, or a subpackage there to convert range to a sequence of lines, which should be quite trivial at this point.

Thanks, this now could easily be shared with the helper command, about converting a range into a sequence of lines, I don't think we really need this as the kernel in fact accepts this exact format.

interfaces/builtin/gpio_chardev.go

strutil/range.go

strutil/range_test.go

pedronis

this looks reasonable to me

pedronis · 2025-03-11T17:28:31Z

strutil/range.go

+}
+
+func parseRangeSpan(in string) (RangeSpan, error) {
+	hasNegativeStart := strings.HasPrefix(in, "-")


do we need the negative range support?

not at all, I discussed dropping it with @ZeyadYasser

Switched to using uints

interfaces/builtin/gpio_chardev.go

strutil/range.go

strutil/range_test.go

interfaces/builtin/gpio_chardev.go

interfaces/builtin/gpio_chardev_test.go

bboozzoo

LGTM

pedronis

thanks, couple of small things, question for @bboozzoo

pedronis · 2025-03-12T13:56:22Z

strutil/range.go

+// ParseRange parses a range represented as a string. The entries are joining
+// them with a comma: n[,m] or as a range: n-m or a combination of both, assuming
+// the ranges are non-negative and do not overlap, e.g.: n,m,x-y.
+func ParseRange(in string) (Range, error) {


nitpick: s/in/input/ or s/in/s/

pedronis · 2025-03-12T13:57:01Z

strutil/range.go

+	// Parse range e.g. 2-5
+	tokens := strings.SplitN(in, "-", 2)
+	if len(tokens) != 2 {
+		return RangeSpan{}, fmt.Errorf("invalid range span %q", in)


this error is not tested

This check is not reachable since input that doesn't contain - will have already been parsed as a single number, and doing SplitN on a string that has - will always have exactly two tokens.

The check is redundant, but good to have in case SplitN was swapped to Split for any reason or by mistake.

pedronis · 2025-03-12T14:00:43Z

interfaces/builtin/gpio_chardev.go

+	return nil
+}
+
+// XXX: What should be the limit on max range.


@bboozzoo any suggestion here? is there a limit in the kernel? otherwise we should just remove the XXX

we can set it to 512, which would still be very high and unlikely to ever be reached in practice.

Signed-off-by: Zeyad Gouda <[email protected]>

This implements the gpio-chardev interface (spec SD129) which is currently hidden behind an experimental feature flag until kernel improvements to the gpio-aggreagtor lands. Signed-off-by: Zeyad Gouda <[email protected]>

Signed-off-by: Zeyad Gouda <[email protected]>

- strutil: fix range helper typo s/Intersets/Intersects - strutil: only support positive numbers in ParseRange - strutil: sort range spans in ParseRange - i/builtin/gpio-chardev: error message improvements - i/builtin/gpio-chardev: remove unnesaccery non-negative validation Signed-off-by: Zeyad Gouda <[email protected]>

Signed-off-by: Zeyad Gouda <[email protected]>

ZeyadYasser · 2025-03-13T12:47:38Z

I had to rebase to resolve conflicts

pedronis

thanks

strutil/range.go

Signed-off-by: Zeyad Gouda <[email protected]>

zyga

Thanks for the improvements. I did a longer pass.

I have some small fixes/improvements to the range code and some questions and comments to the interface code. Please see inline.

zyga · 2025-03-19T13:25:16Z

strutil/range.go

+	return false
+}
+
+func (r Range) Size() (size int) {


Can you document this and explain (and check that this is indeed expected) that overlapping range spans are counted multiple times.

for Range, overlapping range spans are documented as unexpected, we could relax this later if we need to. This is also enforced through the ParseRange helper.

// Range of discrete numbers represented as a set of non overlapping RangeSpan(s). type Range []RangeSpan

zyga · 2025-03-19T13:28:27Z

strutil/range.go

+		}
+		r = append(r, s)
+	}
+	sort.Sort(spansByStart(r))


This sort of sorting is discouraged now. Perhaps you can use sort.Slice:

Suggested change

sort.Sort(spansByStart(r))

sort.Slice(r, func(i, j int) bool { return r[i].Start < r[j].Start })

zyga · 2025-03-19T13:29:01Z

strutil/range.go

+	return r, nil
+}
+
+func parseRangeSpan(in string) (RangeSpan, error) {


Could you please move this next to RangeSpan

zyga · 2025-03-19T13:29:59Z

strutil/range.go

+	"strings"
+)
+
+// RangeSpan represents a span of numbers inside a range. A span with


Nitpick. I tried to make sense of the naming of RangeSpan and Range but I keep feeling those are off. Could this be just Span and the other one Spans? Feel free to ignore.

I don't have strong opinions on this, @bboozzoo @pedronis what do you think?

zyga · 2025-03-19T13:30:44Z

strutil/range.go

+	}
+	start, err := strconv.ParseUint(tokens[0], 10, 32)
+	if err != nil {
+		return RangeSpan{}, fmt.Errorf("invalid range span %q: %w", in, err)


Small working tweak to make errors clearer:

Suggested change

return RangeSpan{}, fmt.Errorf("invalid range span %q: %w", in, err)

return RangeSpan{}, fmt.Errorf("invalid start of span %q: %w", in, err)

zyga · 2025-03-19T13:37:02Z

interfaces/builtin/gpio_chardev.go

+	plugName := slot.Name()
+	plugSnapName := plug.Snap().InstanceName()
+
+	target := fmt.Sprintf("/dev/snap/gpio-chardev/%s/%s", slotSnapName, slotName)


I'd really use filepath.Join here.

zyga · 2025-03-19T13:37:31Z

interfaces/builtin/gpio_chardev.go

+		Type:            "oneshot",
+		RemainAfterExit: true,
+		ExecStart:       fmt.Sprintf("/bin/sh -c 'mkdir -p %q && ln -s %q %q'", filepath.Dir(symlink), target, symlink),
+		ExecStop:        fmt.Sprintf("/bin/sh -c 'rm -f %q'", symlink),


Do we need to run this via the shell?

not really, I was keeping it consistent with ExecStart, I can switch to /bin/rm

zyga · 2025-03-19T13:38:00Z

interfaces/builtin/gpio_chardev.go

+	plugSnapName := plug.Snap().InstanceName()
+	snippet := "# Allow access to exported gpio chardev lines\n"
+	// Allow access to exported virtual slot device.
+	snippet += fmt.Sprintf("/dev/snap/gpio-chardev/%s/%s rwk,\n", slotSnapName, slot.Name())


We are computing this many times in the file. Can you use a helper that has the path exactly once.

Good point, I already did that in the helper command PR as a follow up

https://github.com/canonical/snapd/pull/15212/files#diff-10f4c2a34c9c6fe86ebdebad743920f2190b6af510ee25458ec839a7369d9e05

zyga · 2025-03-19T13:38:29Z

interfaces/builtin/gpio_chardev.go

+	// Allow access to exported virtual slot device.
+	snippet += fmt.Sprintf("/dev/snap/gpio-chardev/%s/%s rwk,\n", slotSnapName, slot.Name())
+	// Allow access to plug-side symlink to exported virtual slot device.
+	snippet += fmt.Sprintf("/dev/snap/gpio-chardev/%s/ r,\n", plugSnapName)


I think we can fold this to /{,*} r

zyga · 2025-03-19T13:39:26Z

interfaces/systemd/service.go

@@ -53,6 +57,18 @@ func (s *Service) String() string {
 	if s.ExecStop != "" {
 		fmt.Fprintf(&buf, "ExecStop=%s\n", s.ExecStop)
 	}
+	if s.Wants != "" {
+		fmt.Fprintf(&buf, "Wants=%s\n", s.Wants)


Buf should have been a strings.Builder not bytes.Buffer

Good point, I think this should go into a followup PR, This one is already huge

Signed-off-by: Zeyad Gouda <[email protected]>

ZeyadYasser added the Needs Samuele review Needs a review from Samuele before it can land label Mar 5, 2025

ZeyadYasser requested review from bboozzoo and pedronis March 5, 2025 18:04

bboozzoo reviewed Mar 6, 2025

View reviewed changes

ZeyadYasser requested a review from bboozzoo March 7, 2025 10:22

ZeyadYasser added the Needs security review Can only be merged once security gave a :+1: label Mar 10, 2025

bboozzoo reviewed Mar 11, 2025

View reviewed changes

strutil/range.go Outdated Show resolved Hide resolved

strutil/range.go Outdated Show resolved Hide resolved

strutil/range.go Show resolved Hide resolved

strutil/range_test.go Outdated Show resolved Hide resolved

pedronis reviewed Mar 11, 2025

View reviewed changes

ZeyadYasser requested review from bboozzoo and pedronis March 12, 2025 08:28

bboozzoo reviewed Mar 12, 2025

View reviewed changes

ZeyadYasser requested a review from bboozzoo March 12, 2025 11:07

bboozzoo approved these changes Mar 12, 2025

View reviewed changes

pedronis reviewed Mar 12, 2025

View reviewed changes

ZeyadYasser added 11 commits March 13, 2025 14:46

i/systemd: support Wants/WantedBy/After/Before directives

a6b1a8d

Signed-off-by: Zeyad Gouda <[email protected]>

i/builtin: add gpio-chardev interface

00f4444

This implements the gpio-chardev interface (spec SD129) which is currently hidden behind an experimental feature flag until kernel improvements to the gpio-aggreagtor lands. Signed-off-by: Zeyad Gouda <[email protected]>

strutil: add range parsing helper

f7d4f31

Signed-off-by: Zeyad Gouda <[email protected]>

fixup! i/builtin: use strutil.ParseRange helper

05a8d23

Signed-off-by: Zeyad Gouda <[email protected]>

fixup! i/builtin: add source-chip validation

dcd15e8

Signed-off-by: Zeyad Gouda <[email protected]>

fixup! strutil: typo fix and test coverage improvement

63447a0

Signed-off-by: Zeyad Gouda <[email protected]>

fixup! i/builtin: use dirs.DistroLibExecDir instead of fixed path

6fa76de

Signed-off-by: Zeyad Gouda <[email protected]>

fixup! i/builtin: fix release info mocking order

2af84cf

Signed-off-by: Zeyad Gouda <[email protected]>

tests: exclude gpio-chardev interface from document-interfaces-url

8e86b7f

Signed-off-by: Zeyad Gouda <[email protected]>

fixup! address review comments

7c6db0b

Signed-off-by: Zeyad Gouda <[email protected]>

ZeyadYasser force-pushed the gpiod-chardev-interface branch from 71b8a46 to 7c6db0b Compare March 13, 2025 12:47

ZeyadYasser requested a review from pedronis March 13, 2025 12:47

pedronis approved these changes Mar 13, 2025

View reviewed changes

ZeyadYasser mentioned this pull request Mar 15, 2025

cmd/snap-gpio-helper: add gpio-chardev export/unexport commands #15212

Open

bboozzoo requested a review from jslarraz March 18, 2025 10:11

zyga reviewed Mar 18, 2025

View reviewed changes

strutil/range.go Outdated Show resolved Hide resolved

fixup! strutil: simplify Range into a slice of RangeSpan

7d2e192

Signed-off-by: Zeyad Gouda <[email protected]>

ZeyadYasser requested a review from zyga March 19, 2025 12:00

zyga reviewed Mar 19, 2025

View reviewed changes

fixup! many: address review comments

510fc2f

Signed-off-by: Zeyad Gouda <[email protected]>

ZeyadYasser requested a review from zyga March 21, 2025 10:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

i/builtin: add gpio-chardev interface #15172

i/builtin: add gpio-chardev interface #15172

ZeyadYasser commented Mar 5, 2025

codecov bot commented Mar 5, 2025 •

edited

Loading

github-actions bot commented Mar 5, 2025 •

edited

Loading

bboozzoo Mar 6, 2025

ZeyadYasser Mar 7, 2025

pedronis left a comment

pedronis Mar 11, 2025

bboozzoo Mar 11, 2025

ZeyadYasser Mar 12, 2025

bboozzoo left a comment

pedronis left a comment

pedronis Mar 12, 2025

pedronis Mar 12, 2025

ZeyadYasser Mar 13, 2025

pedronis Mar 12, 2025

bboozzoo Mar 12, 2025

ZeyadYasser commented Mar 13, 2025

pedronis left a comment

zyga left a comment

zyga Mar 19, 2025

ZeyadYasser Mar 21, 2025

zyga Mar 19, 2025

zyga Mar 19, 2025

zyga Mar 19, 2025

ZeyadYasser Mar 21, 2025

zyga Mar 19, 2025

zyga Mar 19, 2025

zyga Mar 19, 2025

ZeyadYasser Mar 21, 2025

zyga Mar 19, 2025

ZeyadYasser Mar 21, 2025

zyga Mar 19, 2025

zyga Mar 19, 2025

ZeyadYasser Mar 21, 2025

	sort.Sort(spansByStart(r))
	sort.Slice(r, func(i, j int) bool { return r[i].Start < r[j].Start })

	return RangeSpan{}, fmt.Errorf("invalid range span %q: %w", in, err)
	return RangeSpan{}, fmt.Errorf("invalid start of span %q: %w", in, err)

i/builtin: add gpio-chardev interface #15172

Are you sure you want to change the base?

i/builtin: add gpio-chardev interface #15172

Conversation

ZeyadYasser commented Mar 5, 2025

codecov bot commented Mar 5, 2025 • edited Loading

Codecov Report

github-actions bot commented Mar 5, 2025 • edited Loading

Failures:

Preparing:

Executing:

Restoring:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedronis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bboozzoo left a comment

Choose a reason for hiding this comment

pedronis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZeyadYasser commented Mar 13, 2025

pedronis left a comment

Choose a reason for hiding this comment

zyga left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Mar 5, 2025 •

edited

Loading

github-actions bot commented Mar 5, 2025 •

edited

Loading