snap: add support for kernel-gpu-2404 #20

Saviq · 2024-10-12T13:29:42Z

No description provided.

Add a component monitor that will manage content in the target path based on the contents of a sentinel file under the source path.

Using hooks and the component monitor, ensure the content is managed in ${SNAP_DATA}/kernel-gpu-2404

Check for sentinel existence in the second content location and run through the wrapper within.

Since the component is likely to be more appropriate for the hardware, make it go first.

TheSignPainter98

This is looking nice! I think this PR could use a little extra error handling, for which I provide some suggestions :)

Edit: apparently my suggestions on error handling got dropped from my review... thanks, GitHub! Anyway, my suggestion would be to use anyhow

TheSignPainter98 · 2024-10-14T11:43:30Z

component-monitor/Cargo.toml

+
+[dependencies]
+clap = { version = "4", default-features = false, features = ["derive", "env", "help", "std", "usage"] }
+exitcode = { version = "1", default-features = false }


I think this dependency can be safely dropped, it looks like it's just being used to get exitcode::OK, which I think can be equivalently expressed using std::process::ExitCode::SUCCESS

TheSignPainter98 · 2024-10-14T11:48:15Z

component-monitor/src/main.rs

+        for _sig in signals.forever() {
+            std::process::exit(exitcode::OK);
+        }


The use of a loop here feels slightly odd as this as signals.forever() is actually being used as a fence and the body of the loop is only expected to be run once. Perhaps something like this could be clearer?

signals.forever().next(); // Await signal. process::exit(ExitCode::SUCCESS);

TheSignPainter98 · 2024-10-14T20:52:44Z

component-monitor/src/main.rs

+    };
+
+    let sentinel_tgt = target.join(sentinel);
+    if sentinel_data == fs::read(&sentinel_tgt).unwrap_or_default() {


The use of .unwrap_or_default() feels a little unclean here as it's being used to ignore the error case and create a new string. This check also relies on the condition that valid sentinel_data != "" in order to work, and although this may always be the case, it's an extra precondition which must be kept in mind. I think instead, it could be clearer to use something like this to explicitly ignore the error and then check the content if successfully read

fs::read(&sentinel_tgt).is_ok_and(|content| content == sentinel_data)

TheSignPainter98 · 2024-10-14T20:59:06Z

component-monitor/Cargo.toml

+edition = "2021"
+
+[dependencies]
+clap = { version = "4", default-features = false, features = ["derive", "env", "help", "std", "usage"] }


I'm a little surprised that the color, error-context and suggestions features are absent, is this for compatibility? Personally I'm a sucker for pretty terminal applications :D (have you also considered the wrap_help feature?) I see the rest of the dependencies deliberately don't have their default features enabled so I suspect this is all intentional

Really it's just for keeping it tight. It's not really a CLI application, just an internal service.

I dropped default_features across the board b/c I dislike things creeping up on me, is all ;)

And Innes warned me about nix being heavy, so that's probably why ;)

Ah, this sounds really good, thanks for the clarification!

TheSignPainter98 · 2024-10-14T21:13:27Z

component-monitor/src/main.rs

+    #[arg(env = "COMPONENT_SOURCE")]
+    source: PathBuf,
+    /// The sentinel file name
+    #[arg(env = "COMPONENT_SENTINEL", value_parser = clap::builder::NonEmptyStringValueParser::new())]


I think there are many ways the user can get a sentinel incorrect and given that it's quite difficult to pass an empty string here, I think it's probably okay not to check this explicitly.

However, if there is a reasonable finite number of inotify events we wish to support here, we could use an enum and implicitly get better validation:

use clap::ValueEnum; #[derive(ValueEnum)] // #[value(rename_all = "SCREAMING_SNAKE_CASE")] // Default is kebab-case, not sure // if this is more appropriate? enum Sentinel { InAccess, InModify, .. }

The sentinel here is actually a file name that we're monitoring for… I'm not reading in inotify events as arguments?

Or did I get my streams crossed here?

Oooooh, right, when I first read this, I thought sentinel here related to the inotify events! I think my confusion came from the use of the String type here---it looked like this specificially isn't a file given that target has type PathBuf below. I think using PathBuf for both would clean this up nicely.

For reference, String should be avoided for buffers as Rust's String and &str types both require valid UTF-8, so to drop this requrement, PathBuf is more idiomatic

I suppose I thought that Path* would not be appropriate for just a file name… but of course a file name is still a path…

Ah, but the string could still contain a / making it more like a path :) I think the intention is to use this with the file system, so signalling that could be beneficial (hence Path*).

This actually makes me wonder, if sentinel is expected not to contain any path separators, rather than having separate target and sentinel parameters, could we take one and extract the name of the directory? Otherwise, I think sentinel feels more like a path to me

TheSignPainter98 · 2024-10-14T21:27:10Z

component-monitor/src/main.rs

+            Err(e) => {
+                error!(target: "files", "{e}");
+            }
+        }


This section is quite deeply nested, I think it would be cleaner to flatten this with by assigning out of the outermost match, adding a few continues and then cleaning things up a little with if-let where we only care about the data in one branch of the match (this last change is a little more subjective than the others). How about something like this?

for entry in ... { let entry = match entry { Ok(e) => e, Err(err) => { error!(target: "files", "{err}"); continue; }, }; let entry_path = entry.path(); if entry_path.is_dir() { if let Err(err) = fs::remove_dir_all(&entry_path) { error!(..., "failed to remove ..."); continue; } debug!(..., "removed ... recursively"); continue; } if let Err(err) = fs::remove_file(&path) { error!(..., "failed to remove ..."); continue; } debug!(..., "removed ...") }

TheSignPainter98 · 2024-10-14T21:40:31Z

component-monitor/src/main.rs

+
+/// Check if the sentinel file is current, else clean the target directory and populate again,
+/// sentinel file being last
+fn populate(source: &PathBuf, sentinel: &String, target: &PathBuf) {


The types here are slightly non-idiomatic, specifically, we're passing immutable references to the owned versions of each of these types (the owned versions have enough information to perform mutation). This is a little awkward as if, for example, we wanted to pass a string literal here---it would force that &'static str to be copied into a String before it could be used! Instead, it's better practice to use to take references to the non-owned versions of each of these, i.e. &Path and &str. The compiler will automatically coerce these &Owned types into their &Borrowed equivalents, so this change will make this signature simpler without any real cost

TheSignPainter98 · 2024-10-18T11:04:58Z

component-monitor/src/main.rs

+                InotifyEvent {
+                    mask: AddWatchFlags::IN_DELETE_SELF | AddWatchFlags::IN_MOVE_SELF,
+                    ..
+                } => panic!("ERROR: Monitored folder disappeared"),


This seems like quite an easy panic to hit! Panicking is a very aggressive action and there is no guarantee that resources are nicely cleaned up (if this program were compiled with panic = "abort" set in Cargo.toml e.g. to reduce binary size). I think it would be nicer just to log an error and return here

paul-rodriguez · 2024-10-18T15:09:13Z

component-monitor/src/main.rs

+                if path == source {
+                    continue;
+                }
+                if *(relative_path.to_str().unwrap()) == **sentinel {


There should be a way to get this to read relative_path == sentinel.into() and then compare them as PathBuf

paul-rodriguez · 2024-10-18T15:14:45Z

component-monitor/src/main.rs

+        match entry {
+            Ok(e) => {
+                let path = e.path();
+                let relative_path = path.strip_prefix(source).unwrap();


You should try to avoid unwrap and prefer ? instead

Saviq

Thanks all for the comments!

Will reply for now to just the ones I have any excuse for :X

Saviq

Hmm how did these not get in…

Saviq · 2024-10-18T15:27:59Z

component-monitor/src/main.rs

+    #[arg(env = "COMPONENT_SOURCE")]
+    source: PathBuf,
+    /// The sentinel file name
+    #[arg(env = "COMPONENT_SENTINEL", value_parser = clap::builder::NonEmptyStringValueParser::new())]


The sentinel here is actually a file name that we're monitoring for… I'm not reading in inotify events as arguments?

Or did I get my streams crossed here?

Saviq · 2024-10-18T15:28:04Z

component-monitor/Cargo.toml

+edition = "2021"
+
+[dependencies]
+clap = { version = "4", default-features = false, features = ["derive", "env", "help", "std", "usage"] }


Really it's just for keeping it tight. It's not really a CLI application, just an internal service.

I dropped default_features across the board b/c I dislike things creeping up on me, is all ;)

And Innes warned me about nix being heavy, so that's probably why ;)

nadzyah · 2024-10-18T14:57:06Z

component-monitor/src/main.rs

@@ -0,0 +1,238 @@
+use log::{debug, error, info, warn, LevelFilter};
+use nix::sys::inotify::{AddWatchFlags, InitFlags, Inotify, InotifyEvent};
+use signal_hook::{consts::SIGINT, consts::SIGTERM, iterator::Signals};


just a suggestion

Suggested change

use signal_hook::{consts::SIGINT, consts::SIGTERM, iterator::Signals};

use signal_hook::{consts::{SIGINT, SIGTERM}, iterator::Signals};

nadzyah · 2024-10-18T15:13:21Z

component-monitor/src/main.rs

+    info!(target: "files", "copying from {source:?} to {target:?}");
+    for entry in WalkDir::new(&source) {
+        match entry {
+            Ok(e) => {


(from @TheSignPainter98) Instead of having the whole logic nested, we can use here something like this:

for entry in ... { let entry = match entry { Ok(e) => e, Err(err) => { error!(target: "files", "{err}"); continue; }, }; let entry_path = entry.path(); if entry_path.is_dir() { if let Err(err) = fs::remove_dir_all(&entry_path) { error!(..., "failed to remove ..."); continue; } debug!(..., "removed ... recursively"); continue; } if let Err(err) = fs::remove_file(&path) { error!(..., "failed to remove ..."); continue; } debug!(..., "removed ...") }

Then you can use the value in the code, avoiding additional nesting

Oh weird, apparently my comment above did make it into the review?! Amazing, thanks for making sure this is seen anyway Nadzeya!

nadzyah · 2024-10-18T15:18:28Z

component-monitor/src/main.rs

+
+    cleanup(target, sentinel);
+    info!(target: "files", "copying from {source:?} to {target:?}");
+    for entry in WalkDir::new(&source) {


Suggestion from cargo clippy: change this to: source

nadzyah · 2024-10-18T15:27:05Z

component-monitor/src/main.rs

+        for event in events {
+            debug!(
+                target: "inotify",
+                "handling {0:?} event for {1:?}",
+                event.mask,
+                event.name
+            );
+            match event {
+                InotifyEvent {
+                    name: Some(name),
+                    mask,
+                    ..
+                } if *name == *args.sentinel => match mask {


(from @TheSignPainter98 and @sminez)
Since InotifyEvent is not an enum but a struct, using match here is unexpected. It's better to extract the values and match them separately:

let (event_name, mask) = match event { InotifyEvent { name, mask .. } => (name, mask), _ => panic!("..."), };

Moreover, since it seems that you need only the fields from this struct, you can simplify it the following way:

for InotifyEvent { name, mask, .. } in events { ... }

Saviq added 2 commits October 12, 2024 13:42

new: add component-monitor

4f03764

Add a component monitor that will manage content in the target path based on the contents of a sentinel file under the source path.

snap: add kernel-gpu-2404 content support

3d98ad4

Using hooks and the component monitor, ensure the content is managed in ${SNAP_DATA}/kernel-gpu-2404

Saviq requested a review from a team as a code owner October 12, 2024 13:29

Saviq added the no-merge Not to be merged label Oct 12, 2024

snap: use the kernel component wrapper

e35f73c

Check for sentinel existence in the second content location and run through the wrapper within.

Saviq force-pushed the kernel-components branch from 7155cbf to e35f73c Compare October 12, 2024 13:35

kernel-gpu: make the component precede Mesa

f557a7b

Since the component is likely to be more appropriate for the hardware, make it go first.

TheSignPainter98 suggested changes Oct 18, 2024

View reviewed changes

paul-rodriguez reviewed Oct 18, 2024

View reviewed changes

Saviq commented Oct 18, 2024

View reviewed changes

nadzyah reviewed Oct 18, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

snap: add support for kernel-gpu-2404 #20

snap: add support for kernel-gpu-2404 #20

Saviq commented Oct 12, 2024

TheSignPainter98 left a comment •

edited

Loading

TheSignPainter98 Oct 14, 2024

TheSignPainter98 Oct 14, 2024

TheSignPainter98 Oct 14, 2024

TheSignPainter98 Oct 14, 2024

Saviq Oct 18, 2024

TheSignPainter98 Oct 18, 2024

TheSignPainter98 Oct 14, 2024

Saviq Oct 18, 2024

TheSignPainter98 Oct 18, 2024

Saviq Oct 18, 2024

TheSignPainter98 Oct 18, 2024 •

edited

Loading

TheSignPainter98 Oct 14, 2024

TheSignPainter98 Oct 14, 2024

TheSignPainter98 Oct 18, 2024

paul-rodriguez Oct 18, 2024

paul-rodriguez Oct 18, 2024

Saviq left a comment

Saviq left a comment

Saviq Oct 18, 2024

Saviq Oct 18, 2024

nadzyah Oct 18, 2024

nadzyah Oct 18, 2024

TheSignPainter98 Oct 18, 2024

nadzyah Oct 18, 2024

nadzyah Oct 18, 2024

	use signal_hook::{consts::SIGINT, consts::SIGTERM, iterator::Signals};
	use signal_hook::{consts::{SIGINT, SIGTERM}, iterator::Signals};

snap: add support for kernel-gpu-2404 #20

Are you sure you want to change the base?

snap: add support for kernel-gpu-2404 #20

Conversation

Saviq commented Oct 12, 2024

TheSignPainter98 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheSignPainter98 Oct 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Saviq left a comment

Choose a reason for hiding this comment

Saviq left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheSignPainter98 left a comment •

edited

Loading

TheSignPainter98 Oct 18, 2024 •

edited

Loading