interfaces/builtin: add the spi interface

[tokurz]

Hey,

this is my first push to GitHub, if I did anything wrong, please tell me :)
I had add the SPI Dev interface in the Snapd to work on that with Ubunutu Core on a RPi3 where we need to read data from Sensors.

When its give any chance to merge this into the "master" branch so may I can test and verify it is working.
Also the RPi3 Gadget needs also to be modified to exponate the interfaces for this.

It should be used as the same as the i2c interface.

I had run locally tests on --devmode snaps if the spi stuff is in general working.

in the boot.config I had add/modify follow lines:

dtparam=spi=on

also i run: sudo modprobe spi-bcm2835 (Maybe it's not neccessary to load it manually)

Cheers

[ogra1]

the pi3 gadget is at https://github.com/snapcore/pi3-gadget ... happy to review a PR for the config.txt and snapcraft.yaml changes :)

[tokurz]

Hey, thanks for the fast reply, can you help me to make the test successfully?

I ran "make fmt" and gaves no errors on my side, maybe something is in the go files wrong, golang is a complete new language for me, normal I develop on python and java.

Cheers

[tokurz]

Hey Guys,

I get some strange errors, maybe you can help me with that:

2017-06-22 16:39:58 Discarding linode:debian-unstable-64 (Spread-04), cannot connect: cannot connect to linode:debian-unstable-64 (Spread-04): ssh: handshake failed: ssh: unable to authenticate, attempted methods [none password], no supported methods remain

its full in the Travis CI log of that.

Cheers

[fgimenez]

@tokurz thanks a lot for your contribution and for pointing out the test error! :) Don't worry about it, we have been hitting some authentication errors lately and are actively looking into them, will be fixed shortly.

Not sure if you are aware of them but there are some unit test errors on the autopktest executions that seem related to your changes, take a look for instance at https://objectstorage.prodstack4-5.canonical.com/v1/AUTH_77e2ada1e7a84929a74ba3b87153c0ac/autopkgtest-xenial-snappy-dev-image/xenial/amd64/s/snapd/20170623_082242_16006@/log.gz

----------------------------------------------------------------------
FAIL: basedeclaration_test.go:523: baseDeclSuite.TestSlotInstallation

basedeclaration_test.go:552:
    c.Check(sanitizeErr, IsNil, comm)
... value *errors.errorString = &errors.errorString{s:"spi slot must have a path attribute"} ("spi slot must have a path attribute")
... spi by core snap

basedeclaration_test.go:547:
    c.Check(err, NotNil, comm)
... value = nil
... spi by gadget snap

basedeclaration_test.go:552:
    c.Check(sanitizeErr, IsNil, comm)
... value *errors.errorString = &errors.errorString{s:"spi slot must have a path attribute"} ("spi slot must have a path attribute")
... spi by gadget snap

OOPS: 44 passed, 1 FAILED
--- FAIL: TestPolicy (0.06s)

Cheers!

[tokurz]

Thanks for pointing me to that, I will look into it.

Cheers

[codecov-io]

Codecov Report

Merging #3499 into master will increase coverage by 0.15%.
The diff coverage is 86.95%.

@@            Coverage Diff             @@
##           master    #3499      +/-   ##
==========================================
+ Coverage    75.2%   75.36%   +0.15%     
==========================================
  Files         387      391       +4     
  Lines       33452    33730     +278     
==========================================
+ Hits        25157    25420     +263     
- Misses       6482     6490       +8     
- Partials     1813     1820       +7

Impacted Files	Coverage Δ
interfaces/builtin/spi.go	86.95% <86.95%> (ø)
daemon/response.go	81.29% <0%> (-7.82%)	⬇️
client/apps.go	96.61% <0%> (-3.39%)	⬇️
systemd/systemd.go	81.81% <0%> (-3.32%)	⬇️
daemon/daemon.go	61.6% <0%> (-1.01%)	⬇️
overlord/devicestate/handlers.go	61.97% <0%> (-0.64%)	⬇️
cmd/snap/cmd_services.go	5.76% <0%> (-0.49%)	⬇️
store/store.go	79.7% <0%> (-0.28%)	⬇️
release/release.go	89.18% <0%> (-0.15%)	⬇️
interfaces/builtin/avahi_observe.go	100% <0%> (ø)	⬆️
... and 18 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3df03f9...0c8efd4. Read the comment docs.

[tokurz]

looks great so far :)

[tokurz]

Hey, can someone merge it into the snap repository, I would like to work with that.

Cheers

[chipaca]

@tokurz while you look at it, could you look at signing the CLA?

[zyga]

I'll review and improve this slightly. I just merged the refactoring of the base policy and I would like to update your branch and review it as well.

[tokurz]

Hy @zyga sure feel free :), let me know when I should do any steps for help.

Cheers

[zyga]

Hey, I updated the branch with a few patches and merged master, resolving the conflict I introduced. I didn't review it fully but I think the basics look OK. I need to have a moment to check everything in detail before I can approve it.

Can you please help me out and complete the contributor license agreement, we cannot merge it without that.

[tokurz]

hey @zyga yes will do it today. Cheers

[tokurz]

Hey @zyga the contributor license agreement forms I have filled out and submitted.

Cheers

[chipaca]

CLA has been signed

Thank you!

[tokurz]

Hey @zyga the Travis CI had some trouble in timeout here the last statement from the output

2017-06-29 14:22:44 Removing disks from linode:ubuntu-core-16-64 (Spread-32)...

2017-06-29 14:23:22 Rebooting linode:ubuntu-core-16-64 (Spread-11) as requested...

The job exceeded the maximum time limit for jobs, and has been terminated.

Can you take a look at it?

Cheers

[zyga]

Some more comments. I'll try to review this again next week.

[zyga]

There are constants for this in the snap package, namely TypeGadget and TypeOS

[zyga]

I'd re-word this to use the slot name but I'd have to check how the error is used by the callers. In the end I'd like a message like slot $snapName:$slotName must have a path attribute

[zyga]

Same here

[zyga]

I have some helpers for this. I'll update this part.

[zyga]

I think we want to panic at this stage. This code can assume sanitize works OK

[zyga]

I'll improve and land this PR.

[tokurz]

sounds great , thank you

[zyga]

I updated this to match the new APIs and some coding style tweaks. This is ready for review by security CC @jdstrand

[zyga]

LGTM after security review

[jdstrand]

Thanks for submitting this, it looks quite good. Can you address the inline comments?

[jdstrand]

In other interfaces (eg i2c) we return nil here since panicking can cause trouble. This means the interface will connect but simply not have the added rules. This helps the user experience when dealing with a poorly named device.

[tokurz]

I had copy the interfaces from the i2c i dont know maybe something has changed in the meanwhile, but you can correct it, if you want. I'm new on the golang and not really aware with the error handling.

[jdstrand]

That's fine. Can you change:

panic("slot is not sanitized")

to:

return nil

Here and the other place I mentioned?

[zyga]

I think the panic is actually what we want. Interfaces should be valid (sanitize is to check that). We should not have a state where we can get to snippet methods and the plug/slot is still invalid. We panic in other places.

[jdstrand]

Ok-- I was looking at other interfaces but that was before various refactorings.

[jdstrand]

Is 'spi0' correct here or does it change depending on number of devices, etc? I wonder if this would be better:

spec.AddSnippet(fmt.Sprintf("/sys/devices/platform/**/**.spi/**/%s/** rw,", strings.TrimPrefix(path, "/dev/")))

Can you give an example path that you are wanting to allow?

[tokurz]

Hey the path is:

/sys/devices/platform/soc/3f204000.spi/spi_master/spi0/

which gives

spi0.0/
spi0.1/

in /dev/ are the interfaces:

/dev/spidev0.0
/dev/spidev0.1

Hope that helps

[jdstrand]

It does. Please use the rule I recommended which will help with future-proofing.

[jdstrand]

Same here wrt panic.

[tokurz]

See above comment

[jdstrand]

The test failures look unrelated:

2017-08-03 20:16:28 Failed tasks: 3
    - linode:ubuntu-16.04-64:tests/unit/c-unit-tests
    - linode:ubuntu-16.04-64:tests/unit/gccgo
    - linode:ubuntu-16.04-64:tests/unit/go

[tokurz]

Will took tomorrow look at it current on the way home

[tokurz]

I didnt understand this error message, maybe someone can help me?

spi_test.go:169:

c.Assert(spec.SnippetForTag("snap.consumer.app"), Equals, "/dev/spidev0.0 rw,/sys/devices/platform/**/**.spi/**/spidev0.0/** rw,")

obtained string = "" +
"/dev/spidev0.0 rw,\n" +
"/sys/devices/platform/**/**.spi/**/spidev0.0/** rw,"

expected string = "/dev/spidev0.0 rw,/sys/devices/platform/**/**.spi/**/spidev0.0/** rw,"

OOPS: 631 passed, 1 FAILED

--- FAIL: Test (0.19s)

the function:

func (s *spiInterfaceSuite) TestAppArmorSpec(c *C) {
        spec := &apparmor.Specification{}
        c.Assert(spec.AddConnectedPlug(s.iface, s.plug1, nil, s.slotGadget1, nil), IsNil)
        c.Assert(spec.SecurityTags(), DeepEquals, []string{"snap.consumer.app"})
        c.Assert(spec.SnippetForTag("snap.consumer.app"), Equals, "/dev/spidev0.0 rw,/sys/devices/platform/**/**.spi/**/spidev0.0/** rw,")
}

[jdstrand]

You are missing a newline here. This test is saying that you expect:

/dev/spidev0.0 rw,/sys/devices/platform/**/**.spi/**/spidev0.0/** rw,

but you get:

/dev/spidev0.0 rw,
/sys/devices/platform/**/**.spi/**/spidev0.0/** rw,

Change it too:

c.Assert(spec.SnippetForTag("snap.consumer.app"), Equals, "/dev/spidev0.0 rw\n,/sys/devices/platform/**/**.spi/**/spidev0.0/** rw,")

and this test should pass.

[zyga]

I fixed this

[jdstrand]

FYI, when developing interfaces, I like to run the unit tests locally like so:

$ go build -v github.com/snapcore/snapd/... && go test -v github.com/snapcore/snapd/interfaces/builtin

Once I'm satisfied with my interface tests, I will run all the non-spread tests:

$ ./run-checks

Then I commit and let travis run (whether I run spread tests next locally or not depends on the patch).

[tokurz]

Hey @jdstrand , thanks for the info, but what I dont understand is this error message:

c.Assert(spec.SnippetForTag("snap.consumer.app"), Equals, "/dev/spidev0.0 rw\n ,/sys/devices/platform/**/**.spi/**/spidev0.0/** rw,")
obtained string = "" +
"/dev/spidev0.0 rw,\n" +
"/sys/devices/platform/**/**.spi/**/spidev0.0/** rw,"

expected string = "" +
"/dev/spidev0.0 rw\n" +
",/sys/devices/platform/**/**.spi/**/spidev0.0/** rw,"

For me it looks correct with the , in the line, what doing I'm wrong..?

[tokurz]

Another thing, when I run it on my local machine:

$ go build -v github.com/snapcore/snapd/interfaces/builtin && go test -v github.com/snapcore/snapd/interfaces/builtin

I get those errors, but when i look into it, they are visible..

Do I need to rewrite from SanitizeSlot to SanitizePlug or did I need to fill out the construct?

type spiInterface struct{}

func (iface *spiInterface) Name() string {
        return "spi"
}

func (iface *spiInterface) StaticInfo() interfaces.StaticInfo {
        return interfaces.StaticInfo{
                Summary:              spiSummary,
                BaseDeclarationSlots: spiBaseDeclarationSlots,
        }
}

func (iface *spiInterface) SanitizeSlot(slot *interfaces.Slot) error {
        if err := sanitizeSlotReservedForOSOrGadget(iface, slot); err != nil {
                return err
        }
        _, err := iface.path(slot)
        return sanitizeSlotReservedForOSOrGadget(iface, slot)
}

github.com/snapcore/snapd/interfaces/builtin/spi.go:50: undefined: interfaces.StaticInfo
github.com/snapcore/snapd/interfaces/builtin/spi.go:51: undefined: interfaces.StaticInfo
github.com/snapcore/snapd/interfaces/builtin/spi.go:76: undefined: sanitizeSlotReservedForOSOrGadget
github.com/snapcore/snapd/interfaces/builtin/spi.go:107: cannot use spiInterface literal (type *spiInterface) as type interfaces.Interface in argument to registerIface:
        *spiInterface does not implement interfaces.Interface (missing SanitizePlug method)

[zyga]

@jdstrand added a comment about why I think panic is correct

[jdstrand]

In other interfaces (eg i2c) we return nil here since panicking can cause trouble. This means the interface will connect but simply not have the added rules. This helps the user experience when dealing with a poorly named device.

[tokurz]

I had copy the interfaces from the i2c i dont know maybe something has changed in the meanwhile, but you can correct it, if you want. I'm new on the golang and not really aware with the error handling.

[jdstrand]

That's fine. Can you change:

panic("slot is not sanitized")

to:

return nil

Here and the other place I mentioned?

[zyga]

I think the panic is actually what we want. Interfaces should be valid (sanitize is to check that). We should not have a state where we can get to snippet methods and the plug/slot is still invalid. We panic in other places.

[jdstrand]

Ok-- I was looking at other interfaces but that was before various refactorings.

[tokurz]

Hey @zyga thanks for this fix, will see if all running well. Greets TK Von: Zygmunt Krynicki [mailto:notifications@github.com] Gesendet: Mittwoch, 9. August 2017 13:58 An: snapcore/snapd <snapd@noreply.github.com> Cc: Tony Kurz <tony.kurz@ml-pa.com>; Mention <mention@noreply.github.com> Betreff: Re: [snapcore/snapd] interfaces/builtin: add the spi interface (#3499) @zyga commented on this pull request.

________________________________ In interfaces/builtin/spi_test.go<#3499 (comment)>:

+ c.Assert(slot.Sanitize(s.iface), ErrorMatches,

+ "spi slots are reserved for the core and gadget snaps") +} + +func (s *spiInterfaceSuite) TestUDevSpec(c *C) { + spec := &udev.Specification{} + c.Assert(spec.AddConnectedPlug(s.iface, s.plug1, nil, s.slotGadget1, nil), IsNil) + c.Assert(spec.Snippets(), HasLen, 1) + c.Assert(spec.Snippets()[0], Equals, `KERNEL=="spidev0.0", TAG+="snap_consumer_app"`) +} + +func (s *spiInterfaceSuite) TestAppArmorSpec(c *C) { + spec := &apparmor.Specification{} + c.Assert(spec.AddConnectedPlug(s.iface, s.plug1, nil, s.slotGadget1, nil), IsNil) + c.Assert(spec.SecurityTags(), DeepEquals, []string{"snap.consumer.app"}) + c.Assert(spec.SnippetForTag("snap.consumer.app"), Equals, "/dev/spidev0.0 rw,/sys/devices/platform/**/**.spi/**/spidev0.0/** rw,") I fixed this — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#3499 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AcM7OkGlBt07JN8GZkhZfs-eU7ja5oMCks5sWZ7cgaJpZM4N_qB_>.

[jdstrand]

Thanks for making the requested changes. LGTM

[tokurz]

Hey @jdstrand , follow test are running fail:

017-08-09 13:50:08 Failed tasks: 1

    - linode:ubuntu-16.04-32:tests/main/interfaces-cups-control

2017-08-09 13:50:08 Failed suite prepare: 1

    - linode:ubuntu-core-16-64:tests/regression/

2017-08-09 13:50:08 Failed suite restore: 2

    - linode:ubuntu-core-16-64:tests/regression/

    - linode:ubuntu-core-16-64:tests/regression/

[zyga]

The test failures are unrelated (flaky/racy tests). EDIT: I restarted the tests now.

[tokurz]

@zyga so the Interface can be merged into the snapcore sources?