many: move notify.SysPath to apparmor.NotifySocketPath #15131
Conversation
github-actions
bot
added
the
Needs Documentation -auto-
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #15131 +/- ##
==========================================
- Coverage 78.07% 78.06% -0.02%
==========================================
Files 1182 1185 +3
Lines 157743 158255 +512
==========================================
+ Hits 123154 123537 +383
- Misses 26943 27037 +94
- Partials 7646 7681 +35
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| @@ -41,7 +41,11 @@ var ( | |||
| // on the notify socket with that version, in which case we'll need to try | |||
| // the next version in the list. | |||
| versionLikelySupportedChecks = map[ProtocolVersion]func() bool{ | |||
| 3: SupportAvailable, | |||
| 3: func() bool { | |||
There was a problem hiding this comment.
Is this intentional?
I was asking about timing/staging this change in the patch.
There was a problem hiding this comment.
Yes, since SupportAvailable is going away, we can't call it here anymore. This was only ever really a placeholder anyway, since any caller interested in protocol version support is being called because prompting itself is supported, and prompting support first checks that the notify socket already exists. So this check was not necessary, just something we could do to make extra sure, until we introduce more interesting checks (e.g. in #15089)
There was a problem hiding this comment.
This patch removes SupportAvailable (since notify.SysPath is being replaced by apparmor.NotifySocketPath), so I remove the call to it here as well, and in #15089 replace it with a proper check.
| @@ -305,6 +304,14 @@ const ( | |||
| Full | |||
| ) | |||
|
|
|||
| var ( | |||
There was a problem hiding this comment.
Could those carry documentation (follow up is fine)
There was a problem hiding this comment.
Probably should. The other ones already existed, but I moved their declaration up to before they're set, since that made more sense to me. Just the last one is new, and it's the path to the socket over which communication can occur with kernel apparmor.
|
Fri Feb 28 19:27:56 UTC 2025 Failures:Preparing:
Executing:
Restoring:
|
olivercalder
force-pushed
the
prompting-notify-socket-change-package
branch
from
7ac2cb2 to
3fc325b
Compare
olivercalder
force-pushed
the
prompting-notify-socket-change-package
branch
from
3fc325b to
5bb38f8
Compare
|
Github CI was stuck uploading spread results for hours, and cancellation wasn't working, so I closed and reopened to get it to actually restart. |
Move the definition of the notify socket path from the notify package into the apparmor package. This means that the apparmor package no longer needs to import the notify package, so the notify package can import the apparmor package in the future without a circular import. Signed-off-by: Oliver Calder <[email protected]>
Signed-off-by: Oliver Calder <[email protected]>
olivercalder
force-pushed
the
prompting-notify-socket-change-package
branch
from
5bb38f8 to
ba6dff8
Compare
Move the definition of the notify socket path from the notify package into the apparmor package. This means that the apparmor package no longer needs to import the notify package, so the notify package can import the apparmor package in the future without a circular import.
This is required for #15089 so that it can check the supported AppArmor kernel features by importing the
apparmorpackage.