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

ZeyadYasser · 2025-03-15T15:54:17Z

~~Note: This PR builds on top of #15172 to reuse and refactor common helpers.~~

This PR adds implementation for the export/unexport commands for the snap-gpio-chardev helper.

~~Relevant commits, until #15172 is merged:~~

~~Export command: 22bc4c4~~
~~Unexport command: 8de225e~~

For context, please check the SD129 spec.

cmd/snap-gpio-helper/main.go

interfaces/builtin/gpio_chardev.go

cmd/snap-gpio-helper/common.go

cmd/snap-gpio-helper/chip.go

cmd/snap-gpio-helper/cmd_export.go

cmd/snap-gpio-helper/chip.go

cmd/snap-gpio-helper/common.go

bboozzoo · 2025-03-17T12:14:14Z

cmd/snap-gpio-helper/common.go

+	stat := &unix.Stat_t{}
+	if err := unixStat(chip.Path(), stat); err != nil {


Suggested change

stat := &unix.Stat_t{}

if err := unixStat(chip.Path(), stat); err != nil {

var stat unix.Stat_t

if err := unixStat(chip.Path(), &stat); err != nil {

zyga · 2025-03-18T06:52:47Z

cmd/snap-gpio-helper/chip.go

+	"golang.org/x/sys/unix"
+)
+
+type GPIOChardev interface {


IMO this belongs somewhere in another package that is just imported here.

I was originally thinking of moving it to the gadget package, maybe it should go there?

Actually thinking again about it, This is not going to get used in other places (at least for now) and moving into another package would unnecessarily fragment it. What do you think?

I tend to avoid mixing "main" and complex implementation. Would it be a problem to move out (mocking/testing)?

strutil/range.go

zyga · 2025-03-19T15:19:25Z

I will park further reviews until the base branch is merged.

github-actions · 2025-03-19T16:17:45Z

Thu Apr 17 09:23:30 UTC 2025
The following results are from: https://github.com/canonical/snapd/actions/runs/14489069391

No spread failures reported

zyga · 2025-03-27T07:25:26Z

@ZeyadYasser this needs a rebase

pedronis · 2025-03-27T16:55:29Z

gadget/device/gpio_chardev.go

I wonder whether this and some/all of common.go shouldn't instead go into a new sandbox/gpio?

zyga

First full pass. Thanks for writing this!

I have some small comments but mainly:

Use File.SyscallConn.Control instead of file.Fd
I'd strongly suggest to use contexts around snap-gpio-helper so that interrupting the process behaves correctly.

cmd/snap-gpio-helper/cmd_export.go

cmd/snap-gpio-helper/cmd_unexport.go

cmd/snap-gpio-helper/common.go

gadget/device/gpio_chardev.go

pedronis

did a pass on the sandbox/gpio stuff mostly

pedronis · 2025-04-07T09:31:06Z

sandbox/gpio/common.go

+}
+
+func removeEphermalUdevTaggingRule(gadget, slot string) error {
+	// XXX: is rule reload/trigger nessacary


typo, also do we know the answer?

AFAICT, not really since the device is removed afterwards anyway

pedronis · 2025-04-07T09:37:52Z

sandbox/gpio/common.go

+	if err := os.MkdirAll(filepath.Dir(devPath), 0755); err != nil {
+		return err
+	}
+	if err := unixMknod(devPath, uint32(stat.Mode), int(stat.Rdev)); err != nil {


this line probably merits a comment about what it does

I was thinking about this some time and I wonder if we should symlink to the device that is already created by udev? I don't know what the design for this was but doing so would have the following consequences:

For administrators, looking at /dev/snap would be self-documenting

When the actual device goes away, the device node is removed and we have a stale symlink - an arguably better outcome.

I'm not pushing for this. I just want to know what you think.

I don't know what this is the state in the spec, of this but I think the original proposal was to use a symlink, I let Maciej and Zeyad comment on this

Only doing a symlink on the slot side might not be possible, because the generated apparmor rule on plug connection needs to know the underlying aggregator chip path (/dev/gpiochipX) in advance and this will be dynamically allocated on boot.

snapd/interfaces/builtin/gpio_chardev.go

Lines 184 to 192 in cdb451c

func (iface *gpioChardevInterface) AppArmorConnectedPlug(spec *apparmor.Specification, plug *interfaces.ConnectedPlug, slot *interfaces.ConnectedSlot) error {

slotSnapName := slot.Snap().InstanceName()

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())

// Allow access to plug-side symlink to exported virtual slot device.

snippet += fmt.Sprintf("/dev/snap/gpio-chardev/%s/{,*} r,\n", plugSnapName)

spec.AddSnippet(snippet)

but I think the original proposal was to use a symlink

The plug-side device is indeed a symlink, the helper command is responsible for the slot-side device.

snapd/interfaces/builtin/gpio_chardev.go

Line 176 in cdb451c

ExecStart: fmt.Sprintf("/bin/sh -c 'mkdir -p %q && ln -s %q %q'", filepath.Dir(symlink), target, symlink),

This is specified in SD129

create a character device node for the slot, with major/minor numbers corresponding to the newly created device

pedronis · 2025-04-07T09:38:46Z

sandbox/gpio/common.go

+	return filepath.Join(filepath.Join(dirs.GlobalRootDir, ephermalUdevRulesDir), fname)
+}
+
+func addEphermalUdevTaggingRule(ctx context.Context, chip *ChardevChip, instanceName, slotName string) error {


typo Ephemeral

sandbox/gpio/gpio_chardev.go

sandbox/gpio/common.go

zyga

Thanks! I have almost no reservations anymore.

I think my biggest question right now is the exact order of operations in the export path. See my comments inline for details.

Thanks for switching the FD handling over to Control and for the nice use of contexts!

dirs/dirs.go

sandbox/gpio/common.go

zyga · 2025-04-09T09:12:52Z

sandbox/gpio/common.go

+	if err := os.MkdirAll(filepath.Dir(devPath), 0755); err != nil {
+		return err
+	}
+	if err := unixMknod(devPath, uint32(stat.Mode), int(stat.Rdev)); err != nil {


I was thinking about this some time and I wonder if we should symlink to the device that is already created by udev? I don't know what the design for this was but doing so would have the following consequences:

For administrators, looking at /dev/snap would be self-documenting

When the actual device goes away, the device node is removed and we have a stale symlink - an arguably better outcome.

I'm not pushing for this. I just want to know what you think.

sandbox/gpio/common.go

sandbox/gpio/gpio_chardev.go

strutil/range.go

pedronis · 2025-04-09T16:25:04Z

@ZeyadYasser please work with @bboozzoo and @zyga to land this, no major objections from me at this point

osutil/stat.go

sandbox/gpio/common.go

zyga

I stopped reading around common.go.

Please have a look at the code around mknod (more important) and around timers (not much important).

cmd/snap-gpio-helper/cmd_export_test.go

osutil/stat.go

sandbox/gpio/common.go

zyga

LGTM with one extra nil check.

See my comment on the Range.String function as well, but don't implement anything more in this PR please.

zyga · 2025-04-15T11:40:43Z

sandbox/gpio/common.go

+}
+
+// This has to match the memory layout of `struct gpiochip_info` found
+// in /include/uapi/linux/gpio.h in the kernel.


Suggested change

// in /include/uapi/linux/gpio.h in the kernel.

// in /usr/include/uapi/linux/gpio.h in the kernel.

or without / if within the kernel source tree, depending on which one this is referring to

it is in the kernel tree headers https://github.com/torvalds/linux/blob/834a4a689699090a406d1662b03affa8b155d025/include/uapi/linux/gpio.h#L32-L36

zyga · 2025-04-15T11:42:02Z

sandbox/gpio/common.go

+	}
+
+	stat, ok := fi.Sys().(*syscall.Stat_t)
+	if !ok {


Suggested change

if !ok {

if !ok || stat == nil {

zyga · 2025-04-15T11:45:57Z

strutil/range.go

+	var commaSeparated strings.Builder
+	size := len(r)
+	for i, span := range r {
+		commaSeparated.WriteString(span.String())


FYI: there's the TextAppender interface for cases like this, so that we can avoid small bits of memory needed only to construct a buffer.

You can totally skip that but the API is interesting and worth looking at the rationale at golang/go#62384

bboozzoo

LGTM

bboozzoo · 2025-04-15T11:50:54Z

cmd/snap-gpio-helper/cmd_unexport_test.go

+	main "github.com/snapcore/snapd/cmd/snap-gpio-helper"
+	. "gopkg.in/check.v1"


Suggested change

main "github.com/snapcore/snapd/cmd/snap-gpio-helper"

. "gopkg.in/check.v1"

. "gopkg.in/check.v1"

main "github.com/snapcore/snapd/cmd/snap-gpio-helper"

and in other files

bboozzoo · 2025-04-15T11:51:09Z

cmd/snap-gpio-helper/cmd_export_test.go

+	main "github.com/snapcore/snapd/cmd/snap-gpio-helper"
+	"github.com/snapcore/snapd/strutil"
+	. "gopkg.in/check.v1"


Suggested change

main "github.com/snapcore/snapd/cmd/snap-gpio-helper"

"github.com/snapcore/snapd/strutil"

. "gopkg.in/check.v1"

. "gopkg.in/check.v1"

main "github.com/snapcore/snapd/cmd/snap-gpio-helper"

"github.com/snapcore/snapd/strutil"

bboozzoo · 2025-04-15T11:52:34Z

sandbox/gpio/common.go

+}
+
+// This has to match the memory layout of `struct gpiochip_info` found
+// in /include/uapi/linux/gpio.h in the kernel.


or without / if within the kernel source tree, depending on which one this is referring to

bboozzoo · 2025-04-15T12:02:03Z

sandbox/gpio/common.go

+		return false, err
+	}
+	stat, ok := finfo.Sys().(*syscall.Stat_t)
+	if !ok {


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

…g to be set Signed-off-by: Zeyad Gouda <[email protected]>

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

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

ZeyadYasser requested review from bboozzoo and pedronis March 15, 2025 15:54

ZeyadYasser force-pushed the gpiod-helper-commands branch from 8de225e to 96bc984 Compare March 15, 2025 15:56

ZeyadYasser changed the title ~~Gpiod helper commands~~ cmd/snap-gpio-helper: add gpio-chardev export/unexport commands Mar 15, 2025

ZeyadYasser force-pushed the gpiod-helper-commands branch from 96bc984 to 8d02191 Compare March 17, 2025 09:07

bboozzoo reviewed Mar 17, 2025

View reviewed changes

zyga reviewed Mar 18, 2025

View reviewed changes

strutil/range.go Outdated Show resolved Hide resolved

ZeyadYasser requested review from bboozzoo and zyga March 19, 2025 13:54

ZeyadYasser force-pushed the gpiod-helper-commands branch from 8b9d45d to acd4ae0 Compare March 27, 2025 14:54

pedronis reviewed Mar 27, 2025

View reviewed changes

zyga reviewed Mar 31, 2025

View reviewed changes

ZeyadYasser requested review from zyga and pedronis April 4, 2025 13:43

pedronis reviewed Apr 7, 2025

View reviewed changes

bboozzoo reviewed Apr 8, 2025

View reviewed changes

zyga reviewed Apr 9, 2025

View reviewed changes

ZeyadYasser requested review from bboozzoo and pedronis April 9, 2025 15:32

pedronis removed their request for review April 9, 2025 16:24

ZeyadYasser mentioned this pull request Apr 10, 2025

snap: add gpio-chardev cross-slot line validation #15307

Merged

ZeyadYasser requested a review from zyga April 10, 2025 19:51

ZeyadYasser force-pushed the gpiod-helper-commands branch 2 times, most recently from 30f8d3c to 5cfcd17 Compare April 10, 2025 19:58

bboozzoo reviewed Apr 15, 2025

View reviewed changes

osutil/stat.go Outdated Show resolved Hide resolved

sandbox/gpio/common.go Outdated Show resolved Hide resolved

zyga requested changes Apr 15, 2025

View reviewed changes

ZeyadYasser requested review from bboozzoo and zyga April 15, 2025 11:26

zyga approved these changes Apr 15, 2025

View reviewed changes

bboozzoo approved these changes Apr 15, 2025

View reviewed changes

ZeyadYasser added 21 commits April 16, 2025 10:25

many: refactor gpio chardev virtual device path

9422df2

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

cmd/snap-gpio-helper: add gpio-chardev export command

c99c29e

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

cmd/snap-gpio-helper: add gpio-chardev unexport command

33c2047

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

cmd/snap-gpio-helper: require experimental.gpio-chardev-interface fla…

046a348

…g to be set Signed-off-by: Zeyad Gouda <[email protected]>

fixup! cmd/snap-gpio-helper: address review comments

d9818b9

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

fixup! many: move gpio chip helpers into gadget/device package

ee46a1c

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

fixup! cmd/snap-gpio-helper: fix unit test race

c53bd22

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

fixup! many: refactor gpio helpers into separate sandbox/gpio package

774d8a5

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

fixup! many: address review comments

ff050fd

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

fixup! many: fix mac os build tests

8cca557

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

fixup! sandbox/gpio: fix typos

81c16cd

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

osutil: add device major/minor number helpers

406a0e4

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

fixup! sandbox/gpio: address review comments

561bbfd

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

fixup! many: address review comments

71e9ff6

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

fixup! osutil: fix failing mac os unit tests

422970e

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

fixup! osutil: remove dependency on CGO for device major/minor helpers

0f0c4d1

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

fixup! sandbox/gpio: fix mixing fs.FileMode and raw syscall file

172d333

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

fixup! sandbox/gpio: fix failing mac os tests

b3752c1

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

fixup! many: address review comments

168593e

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

fixup! sandbox/gpio: add stat nil check

200b180

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

fixup! many: address review comments

0d11203

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

ZeyadYasser force-pushed the gpiod-helper-commands branch from f1a3fa2 to 0d11203 Compare April 16, 2025 09:12

ZeyadYasser merged commit 3548b47 into canonical:master Apr 17, 2025
76 of 77 checks passed

ZeyadYasser mentioned this pull request May 15, 2025

many: remove the gpio-chardev-interface experimental flag #15449

Merged

		stat := &unix.Stat_t{}
		if err := unixStat(chip.Path(), stat); err != nil {

	func (iface gpioChardevInterface) AppArmorConnectedPlug(spec apparmor.Specification, plug interfaces.ConnectedPlug, slot interfaces.ConnectedSlot) error {
	slotSnapName := slot.Snap().InstanceName()
	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())
	// Allow access to plug-side symlink to exported virtual slot device.
	snippet += fmt.Sprintf("/dev/snap/gpio-chardev/%s/{,*} r,\n", plugSnapName)
	spec.AddSnippet(snippet)

	// in /include/uapi/linux/gpio.h in the kernel.
	// in /usr/include/uapi/linux/gpio.h in the kernel.

		main "github.com/snapcore/snapd/cmd/snap-gpio-helper"
		. "gopkg.in/check.v1"

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

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

Uh oh!

Conversation

ZeyadYasser commented Mar 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

zyga commented Mar 19, 2025

Uh oh!

github-actions bot commented Mar 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

No spread failures reported

Uh oh!

zyga commented Mar 27, 2025

Uh oh!

pedronis Mar 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zyga left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pedronis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pedronis Apr 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ZeyadYasser Apr 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zyga left a comment

ZeyadYasser commented Mar 15, 2025 •

edited

Loading

github-actions bot commented Mar 19, 2025 •

edited

Loading

pedronis Mar 27, 2025 •

edited

Loading

pedronis Apr 7, 2025 •

edited

Loading

ZeyadYasser Apr 10, 2025 •

edited

Loading

pedronis commented Apr 9, 2025 •

edited

Loading