=== chihchun is now known as chihchun_afk === chihchun_afk is now known as chihchun === chihchun is now known as chihchun_afk === chihchun_afk is now known as chihchun === chihchun is now known as chihchun_afk [05:10] Good morning (perhaps) [05:15] morning [05:21] mborzecki: hey [05:22] How was Easter? [05:22] zyga: too short :) [05:23] Today will be full of stuff :/ [05:23] In the bad sense I suspect [05:24] zyga: hmm https://forum.snapcraft.io/t/disabling-seccomp-sandbox-where-a-buggy-golang-seccomp-is-used/11054 [05:24] Yeah, that is one [05:25] zyga: do you know if the version in 29 is ok? [05:26] I didn’t check yet [05:27] I will be in the office at around 8 [05:29] brb, got to reboot, new kernel [05:35] zyga: has the mystery of broken seed.yaml been explained? [05:45] zyga: seed.yaml from that alpha image has a slightly weird format like so https://paste.ubuntu.com/p/vM2MZjZN2t/ but the test i added to check that in the code seems to work fine, no nils === chihchun_afk is now known as chihchun [06:07] No, we don’t know anything more yet === chihchun is now known as chihchun_afk [06:11] zyga: i did a clean install with the alpha iso, no issues [06:12] We need to talk to foundations to ask who can help with the installer [06:12] We don’t know how the seed is made [06:15] zyga: definitely the format is slightly awkward, so for instance this variant causes a nil https://paste.ubuntu.com/p/9N7CXZRtwv/ [06:17] Yeah [06:17] This is regular yaml [06:17] Nothing special [06:17] zyga: fwiw, we could have used SeedSnap instead of a pointer [06:17] Yes [06:18] Not sure why we did === chihchun_afk is now known as chihchun [06:21] i'll check f29 golang seccomp next, according to bodhi it was updated recent'y, but i'm not sure whether that's 0.9 tag exactly or some other commit [06:23] didn't know you could embed lua in rpm macros [06:24] Thanks [06:24] I will look at a content bug [06:24] zyga: something new? [06:30] Yes [06:30] Reported in response to a theme bug [06:35] mborzecki: ok, in the office now [06:35] obviously kids didn't want to go with the dog so I had to [06:36] mborzecki: nothing in recent memory has made them more lazy than the teacher's strike [06:39] mborzecki: is mvo working today? === chihchun is now known as chihchun_afk [06:42] zyga: idk [06:43] zyga: fedora's updated libseccomp-gois exactly 0.9 release, so the commit with the fix is not included [06:58] zyga: filed a bug in fedora https://bugzilla.redhat.com/show_bug.cgi?id=1702169 [06:58] ack [06:58] on centos we used vendored packages [07:03] zyga: so i'm thinking, fedora & debian are the only 2 distros that do not bundle golang deps, once they are fixed, we may not need to disable seccomp dynamically at all [07:04] mborzecki: jdstrand will join our standup today [07:04] mborzecki: perhaps discuss with him then [07:04] zyga: and a silly thought, since we already do the snap-seccomp version-info magic, maybe we could extend this to cover bpf output geenrated by the lib too? [07:04] mborzecki: eh :) [07:04] that's one rabbit hole IMOI [07:06] zyga: i mean the problem is that even if they update snap-seccomp bindings, we may not renerate the profiles, because lib version and supported syscalls could stay the same [07:06] yes, I was thinking about this earlier, all the effort we did [07:06] it is really insufficient [07:07] zyga: or we could get a pass from fesco or other body and just bundle that dep? :) [07:07] mborzecki: good luck to get that approved in the next 60 days ;) [07:08] anyways, golang deps are still arguably silly and don't fit the disto world (or iow the distro world did not catch up with golang/rust/node/python package handling yet) [07:08] mborzecki: it cannot catch up [07:08] mborzecki: but that's a separate conversation === pstolowski|afk is now known as pstolowski [07:10] mornings! [07:10] Hi! I opened that one https://forum.snapcraft.io/t/auto-connection-request-for-gpio-interface/11013 -- is that under the current store rules possible ? [07:11] zyga: better had with a glass of beer :P (we should make a list of beer-topics fwiw) [07:11] pstolowski: hey [07:11] mborzecki: haha, a trello lane :) [07:11] pstolowski: good morning, how are you doing? [07:11] pstolowski: had enough mazurki over weekend? :-) [07:12] om26er: yes [07:12] zyga: i'm fine, and satiated :) [07:13] how are you guys? [07:13] pstolowski: depressed a little [07:13] pstolowski: floodgates of bugs [07:13] pstolowski: I want holidays that last 4 days :) [07:14] oh... i need to catch up [07:15] mborzecki: can you take a look at https://github.com/snapcore/snapd/pull/6728? that could land [07:19] pstolowski: sure [07:22] mmmmm [07:22] hmmm [07:22] bugs, bugs everywhere [07:22] there is something wrong with slot attrs [07:29] * zyga wants snap plugs and snap slots [07:29] it's infuriating to debug [07:35] zyga: shouldn't mei rule be /dev/mei[0-9]* ? [07:35] mborzecki: why? [07:35] zyga: because i don't see + in the manpage :) [07:35] mborzecki: ? [07:35] mborzecki: in apparmor re? [07:36] zyga: yeah, the manpage is not too useful, but i don't see anything that uses + [07:36] mborzecki: grep for + in interfaces [07:36] perhaps it's a common bug [07:36] apparmor RE sucks a little for being custom [07:37] zyga@fyke:~/Documents/go/src/github.com/snapcore/snapd/cmd/snapd$ sudo ./snapd [07:37] AppArmor status: apparmor is enabled and all features are available [07:37] cannot run daemon: cannot obtain snap-seccomp version information: error: unsupported argument "version-info" [07:37] mborzecki: how do I run snapd now? [07:37] zyga: build snap-seccomp and put it in the same dir [07:37] ok [07:37] a bit meh [07:39] eh [07:40] not a great day [07:40] pstolowski: we have a serious bug in interfaces [07:41] zyga: uh, what is it? [07:41] pstolowski: hold on, let me prepare everything first [07:41] zyga: heh, looks like a bug in patterns, we have /dev/sr[0-9]* /dev/rtc[0-9]* /dev/input/event[0-9]* but only /dev/mei[0-9]+ [07:41] mborzecki: I disagree [07:41] mborzecki: zyga@fyke:~/Documents/go/src/github.com/snapcore/snapd/interfaces/builtin$ git grep ']+' [07:42] for me it shows much more than one item [07:42] ah, sorry [07:42] we use go REs as well [07:42] those are from go [07:42] mborzecki: sanity check, apparmor, use of + is a bug? [07:43] zyga: for all we know the parser does not complain :) [07:43] mborzecki: the parser sucks [07:43] zyga: fwiw, all other patterns use * [07:43] i'll open a PR [07:43] meh [07:43] mborzecki: thanks, assign the bug to yourself too [07:51] https://github.com/snapcore/snapd/pull/6762 [07:52] zyga: ^^ [07:52] back to pstolowski's review [07:52] pstolowski: can you HO? [07:52] pstolowski: need your eyes on something [07:53] I'm in standup [07:53] mborzecki: wanna see? === chihchun_afk is now known as chihchun [07:59] diddledan: in case you didn't notice, https://github.com/diddlesnaps/gog-galaxy-wine/blob/master/snap/snapcraft.yaml has conflict markers in it and is thus failing to parse as YAML [08:28] mborzecki: I don't know AARE; does * there mean "0 or more"? (the manpage is not clear to me -- it says "any number" which might include 0 or not) [08:28] https://github.com/snapcore/snapd/compare/master...zyga:fix/lp-1825883?expand=1 [08:29] Chipaca: -1 [08:29] ;) [08:29] Chipaca: hey [08:29] 'sup [08:32] Chipaca: i think it's more like glob for file paths, so dev /dev/mei[0-9] matches mei0-mei9 followed by whatever that's not / [08:33] Chipaca: fun day continues https://bugs.launchpad.net/snapd/+bug/1825883/comments/4 [08:33] Chipaca: unforunately apparmor.d manpage is most useless explaining AARE :/ [08:34] thanks cjwatson [08:34] Chipaca, mborzecki: my personal experience is that the documentation is only partial there [08:34] the only way to learn more is to look at what the kernel reads [08:34] which is non-trivial [08:34] Chipaca: for reference, we allow `/dev/tty[0-9]* rw,` for x11 and other rules also follow [0-9]* patterns [08:35] mborzecki: /dev/tty is a thing tho [08:35] I failed mid way on that because of the representation of AARE is only documented in the C implementation as imperative code, there's no comment as to what the representation is [08:35] * zyga wrote a partial apparmor decompiler at one point in the past [08:36] Chipaca: fair enough, /dev/ttyUSB is not :) /dev/tty{USB,ACM}[0-9]* [08:36] windy dat [08:36] *day [08:38] * zyga wants a set of debug commands that just display the data from the state [08:38] zyga: hm maybe we'll have libseccomp-golang updated in fedora sooner than expected -> https://bugzilla.redhat.com/show_bug.cgi?id=1702169 [08:39] which would be great imo :) [08:41] mborzecki: report a debian bug too please [08:44] pstolowski: so [08:44] pstolowski: the problem is that snap connect is storing information that is never updated after the fact [08:45] sore & forget [08:45] store & forget [08:49] zyga: right. we never call doConnect again, and with the new logic of storing static attrs in the state we don't refresh them, in the old implementation setup-profiles would pick the new attributes right? [08:49] yes [08:49] damn [08:49] that's bad [08:49] yes :) [08:49] looking at what may be the best way forward now [08:49] but first I want to add a system [08:49] that records a trace of revision in the repo [08:50] so repo will know what the revision of each snap is [08:50] and will panic if those disagree [08:52] zyga: we need to think, perhaps we should re-run all the interface hooks, that means disconnecting first and then re-connecting [08:53] pstolowski: I think we need to take a step back and write some invariants [08:53] pstolowski: hooks are both niche and complex [08:53] pstolowski: I'd like to support them but going through the hook door first might just confuse us more [08:57] zyga: maybe we should take step back and reconsider storing static attrs in state, and only use that for hotplug slots [08:58] pstolowski: maybe, I will add consistency checks to the repo and see what we get [09:11] zyga: i think setupProfilesForSnap needs fixing. it already does repo.Remove(snap) and Re-add(snap), and then reloadConnection which picks outdated attrs from state. [09:13] zyga: if _, err := m.repo.Connect(connRef, conn.StaticPlugAttrs, conn.DynamicPlugAttrs, conn.StaticSlotAttrs, conn.DynamicSlotAttrs, nil); err != nil { ... in reloadConnections [09:14] pstolowski: I agree this is where the error is triggered [09:14] pstolowski: I'm not yet sure what is the best way to fix it [09:14] https://bugs.launchpad.net/snapd/+bug/1825956 [09:14] This was reported to me by multiple people over the weekend [09:14] Feels somewhat critical (if it's not already known [09:14] popey: this is a duplicate of another bug [09:14] ok [09:14] it's biting a lot of people because 19.04 just came out and lots of people are installing systems with no snaps pre-installed [09:15] https://bugs.launchpad.net/snapd/+bug/1819318 [09:15] marked as such now [09:15] popey: I agree [09:15] popey: just ETOOMANYBUGS [09:16] bugs.reduce((status) => status.ignore()); [09:16] :-p [09:17] that should be a map really [09:17] I suck at jokes [09:18] nerd jokes are best jokes [09:18] (for nerds) [09:18] :-) [10:05] pstolowski: hmm [10:06] never mind === chihchun is now known as chihchun_afk [10:15] pstolowski: question, open interface manager's handlers.go and go to doConnect [10:16] pstolowski: inside that function look at the call to repo.Connect [10:16] pstolowski: it takes plugDynamicAttrs and slotDynamicAttrs, not taking static attrs (not sure why) [10:17] zyga: looking [10:17] pstolowski: now the questsion is, a few lines below that, we create connState and store plug and slot static and dynamic attrs using conn.{Plug,Slot}.{Dynamic,Static}Attrs() instead of what we passed to Connect [10:17] why is that? [10:18] aah [10:18] * zyga is dumb [10:18] sorry, this is correct [10:18] we save what the connection gave us [10:18] pstolowski: sorry for wasting your time [10:18] zyga: no worries. NewConnectedPlug and NewConnectedSlot are special-cased for nils and take static attrs from plug/slot info [10:22] zyga: i'm thinking a fix i suggested above for setupprofiles/reload connections is a potential solution, i'm happy to work on this, just let me know [10:23] pstolowski: I'm still working on this, I'm not convinced that is enough (it would fix it but I want to be sure we've fixed all cases of this issue) [10:23] ok [10:25] zyga: if you're making repo rev-aware, then i think it would be great to keep it separate from actual bugfix (if possible), i think it's a bit orthogonal problem [10:25] pstolowski: no worries :) [10:25] pstolowski: the repo is not the problem [10:26] pstolowski: I made that but id doesn't pick up the problem (obviously) [10:26] pstolowski: I'm making connState rev aware [10:46] pstolowski: we run the refresh hooks right? [10:46] pstolowski: but on snap refresh we don't run interface hooks, do we? [10:48] zyga: correct, except for autoconnects, those can run hooks [10:48] hmm? [10:48] how do you mean? [10:50] pstolowski: so, I think we have two bugs now [10:50] zyga: as part of install/refresh we run "autoconnect" to connect new interfaces (if applicable); it will run hooks [10:50] I see [10:50] the first bug is that the static attributes are stale, that's plain and obvious [10:51] the dynamic attributes are equally out of date, if you setup a content connection hook you will most likely be able to see the revision in the path you observe [10:51] if you refresh stuff those will be stale equally (so snaps will not see the accurate paths if they use hooks to update them) [10:51] pstolowski: I will break now, take a walk and think about this [10:52] pstolowski: my gut feeling is that we need to rethink refresh and interfaces [10:52] pstolowski: we can "hack" to fix this issue [10:52] pstolowski: but the core issue is more complex and I think what we do today is simply inconsistent with itself [10:52] pstolowski: we need to find something that is correct and consistent [10:52] pstolowski: and also not annoying or difficult to use for users [10:53] pstolowski: (e.g. do you see a disconnect hook and a connect hook when your peer refreshes?) [10:53] pstolowski: it's extra tricky for content because the snap will synchronously see the old and the new state and nothing in beteween [10:53] pstolowski: so the hook is really "hey buddy, it changed" without a way to see the prior state [10:54] pstolowski: I changed stuff enough in my branch to detect when the connection state is inconsistent with the repository [10:56] zyga: for autoconnect we do plugStatic, slotStatic, err := initialConnectAttributes(st, plugSnapInfo, plugSnap, plugName, slotSnapInfo, slotSnap, slotName), that's what the hooks see, and that is from CurrentInfo() after link-snap [10:56] zyga: that's in ifacestate.connect(..) that creates interface hook tasks [10:57] pstolowski: forget autoconnect [10:57] pstolowski: when you refresh a snap with an existing connection, the peers of that snap *never know about the refresh* [10:57] pstolowski: and attributes encode the revision [10:57] pstolowski: so if you use hooks to update internal snap config you are SOL [11:00] pstolowski: I pushed some patches to https://github.com/snapcore/snapd/compare/master...zyga:fix/lp-1825883?expand=1 [11:00] zyga: i'll show you something [11:00] zyga: https://github.com/snapcore/snapd/pull/5200/ [11:00] zyga: almost one year ago ;) [11:00] zyga: see // on refresh re-connect existing connections and run their interface hooks (if applicable) [11:01] zyga: there is even a spread test [11:01] pstolowski: that's good in itself [11:01] pstolowski: does this update connection state as well? [11:02] pstolowski: https://github.com/snapcore/snapd/commit/fe3498a40ac38857142525f781ea808447b19c5e this errors whenever we would take a decision based on old data [11:02] pstolowski: I'm happy to chat over TG but I need a walk now [11:04] zyga: yes that old branch updates connstate because it disconnectes and then reconnects, so it runs disconnect hooks and then connect hooks [11:04] zyga: it was parked back then :( [11:06] pstolowski: I’ll run my branch on all current tests to see if something is already affected [11:20] pstolowski: https://twitter.com/ismonkeyuser/status/1120568699168133120 [11:21] ;-) [11:21] :D [11:49] mborzecki: can you edit https://github.com/snapcore/snapd/pull/6762 to reference the bug report please [11:50] zyga: was there one? [11:50] mborzecki: I think so, no? was that just on the forum> [11:51] mborzecki: then at least a Forum: ... entry next to signed-off-by [11:51] zyga: i've only seen the forum post [11:51] zyga: so you mean the commit message? [11:54] mborzecki: yeah, or the PR merge message [11:57] pstolowski: I patched my branch to only report an error when the static attributes actually differ [11:57] pstolowski: we still have tests failing :/ [11:58] pstolowski: so we currently may depend on incorrect behivior [11:58] *behaviour [12:01] uhm [12:04] i'm afraid the only correct way of dealing with this is to really re-run (reconnect) everything (like in my old PR, modulo comments from pedronis - namely the comment re hooks not knowning if it's initial connect vs reconnect). we need to discuss when he is back [12:06] * pstolowski is kicking himself for not realizing significance of landing or not landing that old PR [12:08] mborzecki: fyi, apparmor.d *does* have AARE: "AARE = ?*[]{}^"\nSee below for meanings" [12:09] jdstrand: right, but the 'see below for meanings' is so informative [12:11] mborzecki: the bit under Globbing? [12:12] jdstrand: right, that one [12:12] huh. I find that clear... [12:12] how would you like to see it changed? I can fix it upstream [12:13] jdstrand: in particular, it says «* can substitute for any number of characters, excepting '/'», but doesn't say whether "any number" includes 0 [12:13] jdstrand: i.e. does /dev/foo[0-9]* match /dev/foo [12:13] it's a glob [12:14] so, 'no' [12:14] but 'yes', I see what you mean [12:14] :-) [12:15] jdstrand: also it says globbing, vs me expecting something mentioning AARE (because that's how it's named above) [12:15] I mean it does say 'File resources may be specified with a globbing syntax similar to that used by popular shells, such as csh(1), bash(1), zsh(1)." but it isn't as clear as it be [12:15] mborzecki: yes. up above it says See below without referencing Globbing, and down below it talks about globbing but not AARE [12:17] mborzecki: you probably saw that I approved and merged the pr [12:17] mborzecki: thanks. I don't know how I missed that :\ [12:17] jdstrand: mhm, thanks! [12:21] jdstrand: hey [12:21] jdstrand: fun day === jospoortvliet_ is now known as jospoortvliet === chihchun_afk is now known as chihchun [13:16] jdstrand: eoan pronuciation guide: https://www.youtube.com/watch?v=iQXMdR0XDT8 [13:18] Chipaca: hahaha [13:45] mborzecki, how close are we to 2.39 release? [13:45] Son_Goku: idk, i'd say not far, zyga do you know? [13:45] Son_Goku: hard to say [13:45] Son_Goku: I'd say we will postpone because of bugs [13:46] but mvo is out so I cannot say with certainty [13:46] okay [13:46] Son_Goku: it's been a week of bugs [13:48] 🐞🐜🐛 [13:50] roadmr: lol :) [13:50] * zyga runs an errand, see you later [14:03] * cachio afk [15:26] Hi! Regarding requests for auto-connection of snap interfaces, how long does it generally take and also what number of +1's does it take for a request to go live ? [15:27] case in point https://forum.snapcraft.io/t/please-allow-auto-connect-of-display-control-interface-for-deskconn/10831 [15:31] om26er: https://forum.snapcraft.io/t/process-for-aliases-auto-connections-and-tracks/455 === om26er89 is now known as om26er [15:33] thanks Chipaca [15:33] om26er: i've pinged @reviwers on that one as it's high time they looked at it [15:33] om26er: but that topic explains # of votes, timeline, etc [15:38] Chipaca cool, there is already a new +1 on that one ;-) [15:55] I wonder if today's xkcd is about libseccomp [15:56] heh [16:34] Re [16:34] * zyga looks at xkcd === pstolowski is now known as pstolowski|afk [17:26] I guess I should EOD === chihchun is now known as chihchun_afk