Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
interfaces: add consoles interface #3051
Conversation
femdom
added some commits
Mar 17, 2017
Some checks were not successful
ogra1
self-requested a review
Mar 20, 2017
|
i dont think this will fly as is, most ubuntu-core installs will actually use a different console= option to run their IoT devices with serial console etc ... the apparmor managing code should better parse /proc/cmdline and grab the console= arg from there instead of hardcoding tty0 |
niemeyer
requested changes
Mar 20, 2017
We'll need to change at least the interface name to the singular ("console") as usual in most (all?) of the interfaces.
Also, can you please provide some more background so we can understand the demand for this interface, can take the appropriate action regarding what @ogra1 is pointing out?
Note that we can most likely be exactly precise, and open up access just the one correct console device.
We also need to be very careful with this interface, as it hands off access to what is being typed. We'll need a review from @jdstrand on those details.
Our software tries to access /dev/tty0 or /dev/console or /dev/tty qlinuxfbscreen.cpp |
stolowski
added some commits
Mar 27, 2017
All checks have passed
|
@femdom FYI, I've updated this PR for the new API changes in master. |
|
@stolowski, thank you! |
| + return "consoles" | ||
| +} | ||
| + | ||
| +func (iface *ConsolesInterface) String() string { |
| + | ||
| +func (iface *ConsolesInterface) UdevConnectedPlug(spec *udev.Specification, plug *interfaces.Plug, slot *interfaces.Slot) error { | ||
| + for appName := range plug.Apps { | ||
| + tag := udevSnapSecurityName(plug.Snap.Name(), appName) |
| + | ||
| + // Creation of the slot of this type | ||
| + // is allowed only by a gadget or os snap | ||
| + if !(slot.Snap.Type == "os") { |
|
FYI, this interface is on my list for review but it will take a little while to get through everything. |
|
Should I fix addressed issues? |
|
Please feel free to address any review feedback, yes. It is still on my list to review and still will take quite a bit of time to get through everything. It has unfortunately fallen behind a number of other items, but we'll get there. |
|
@jdstrand Any news here? We're making a push to get the queue clean this week: |
|
@niemeyer - I'd like to study the tty subsystem for this PR to better understand it. I mentioned yesterday that this PR has fallen behind other work-- that work is mostly items from https://forum.snapcraft.io/t/review-sprint-1/377 (atm, bash completion, but several others after that). This PR won't be reviewed this week at this point. |
| + | ||
| + // Creation of the slot of this type | ||
| + // is allowed only by a gadget or os snap | ||
| + if !(slot.Snap.Type == "os") { |
zyga
Apr 28, 2017
Contributor
Nitpick: can this please say if slot.Snap.Type != snap.TypeOS { ... } please?
|
I'll work on this first thing tomorrow. I'll review it again, apply all the feedback and get it ready for another review. |
zyga
added
the
Blocked
label
May 9, 2017
|
After discussing with @jdstrand I'm marking this interface as blocked. It requires additional research into the kernel tty layers to determine if the security is sensible. |
Some checks were not successful
zyga
added some commits
May 11, 2017
All checks have passed
|
What's the state of this PR? |
chipaca
added
the
Decaying
label
Jul 10, 2017
|
@chipaca - still blocked behind other higher priority work, but it is in the queue and not lost. |
|
OK. It'll need some serious de-conflicting I fear. |
|
can we please also have the sys_tty_config capability, something taking over the console might want to reconfigure the tty as well... |
Some checks were not successful
|
I just de-conflicted it, I will iterate some more to add modern meta-data and simplify a few things. |
|
Since this continues to wait on me, I'm happy to deconflict it as needed (though I see @zyga did this already). For now I'm going to close this PR but will reopen when it is at the top of the queue. |
jdstrand
closed this
Jul 11, 2017
zyga
added some commits
Jul 11, 2017
Some checks were not successful
|
I'll re-open this briefly to push my changes :) |
zyga
reopened this
Jul 11, 2017
|
Pushed, closing again :) |
femdom commentedMar 17, 2017
To allow access to the /dev/tty0 and /dev/console