tool: smarter systemd-boot install #76

2023-01-23T18:11:36Z

nikstur commented

2023-01-23 18:11:36 +00:00

(Migrated from github.com)

The process of installing systemd-boot is "smarter" because it now
considers a a few conditions instead of doing nothing if there is a file
at the deistination path. systemd-boot is now forcibly installed (i.e.
overwriting any file at the destination) if (1) there is no file at the
destination, OR (2) a newer version of systemd-boot is available, OR (3)
the signature of the file at the destination could not be verified.

Fixes part of #39

Will produce a small and easy to fix merge conflict with #75

The process of installing systemd-boot is "smarter" because it now considers a a few conditions instead of doing nothing if there is a file at the deistination path. systemd-boot is now forcibly installed (i.e. overwriting any file at the destination) if (1) there is no file at the destination, OR (2) a newer version of systemd-boot is available, OR (3) the signature of the file at the destination could not be verified. Fixes part of #39 Will produce a small and easy to fix merge conflict with #75

RaitoBezarius (Migrated from github.com) reviewed

2023-01-23 18:11:36 +00:00

nikstur commented

2023-01-23 18:18:28 +00:00

(Migrated from github.com)

This still needs a test or two I've realised. ^^

blitz (Migrated from github.com) reviewed

2023-01-24 23:54:29 +00:00

rust/tool/src/install.rs

blitz (Migrated from github.com) commented

2023-01-24 23:54:29 +00:00

nit: It is a bit of a hack though. ;)

blitz (Migrated from github.com) reviewed

2023-01-25 00:00:46 +00:00

rust/tool/src/os_release.rs

					
				@ -39,0 +55,4 @@

				            let v = kv

				                .get(1)

				                .map(|s| s.trim_matches('"'))

				                .with_context(|| format!("Failed to get second element from {kv:?}"))?;

blitz (Migrated from github.com) commented

2023-01-25 00:00:43 +00:00

This doesn't handle entries with \" correctly. (See also the spec.)

Maybe something like this would help: https://crates.io/crates/os-release

This doesn't handle entries with `\"` correctly. (See also the [spec](https://www.freedesktop.org/software/systemd/man/os-release.html).) Maybe something like this would help: https://crates.io/crates/os-release

nikstur (Migrated from github.com) reviewed

2023-01-25 00:01:15 +00:00

rust/tool/src/install.rs

nikstur (Migrated from github.com) commented

2023-01-25 00:01:14 +00:00

Oh yes big time. Do you have a better solution? Or should I just make this clear in the comment as well?

blitz (Migrated from github.com) reviewed

2023-01-25 00:04:48 +00:00

rust/tool/src/pe.rs

blitz (Migrated from github.com) commented

2023-01-25 00:04:48 +00:00

But you don't provide a path as parameter. The comment seems stale.

blitz (Migrated from github.com) requested changes

2023-01-25 00:06:54 +00:00

blitz (Migrated from github.com) left a comment

This code was really easy to follow thanks to the useful comments. I love it.

My only concern here is the yolo parsing/printing of the os-release file. If we can use one of the existing parsing libraries that also handles the quoting correctly, I would sleep more calmly. :)

This code was really easy to follow thanks to the useful comments. I love it. My only concern here is the yolo parsing/printing of the os-release file. If we can use one of the existing parsing libraries that also handles the quoting correctly, I would sleep more calmly. :)

nikstur (Migrated from github.com) reviewed

2023-01-25 00:46:28 +00:00

rust/tool/src/os_release.rs

					
				@ -39,0 +55,4 @@

				            let v = kv

				                .get(1)

				                .map(|s| s.trim_matches('"'))

				                .with_context(|| format!("Failed to get second element from {kv:?}"))?;

nikstur (Migrated from github.com) commented

2023-01-25 00:46:28 +00:00

I looked at the os-release crate because I had the same instinct of avoiding yolo parsing, but sadly it's (1) unmaintained since 2018 and (2) actually performs the same hack (just slightly less elegantly): Compare mine with theirs

We actually never write out a file we have previously read. So I'm not too worried about yolo parsing from that perspective.
I should add a comment, however, that writing out a previously read file might not produce the same representation.

I looked at the os-release crate because I had the same instinct of avoiding yolo parsing, but sadly it's (1) unmaintained since 2018 and (2) actually performs the same hack (just slightly less elegantly): Compare [mine](https://github.com/nix-community/lanzaboote/blob/smart-systemd-boot-install/rust/tool/src/os_release.rs#L53) with [theirs](https://github.com/pop-os/os-release/blob/master/src/lib.rs#L37) We actually never write out a file we have previously read. So I'm not too worried about yolo parsing from that perspective. I should add a comment, however, that writing out a previously read file might not produce the same representation.

blitz (Migrated from github.com) reviewed

2023-01-25 10:58:57 +00:00

rust/tool/src/os_release.rs

					
				@ -39,0 +55,4 @@

				            let v = kv

				                .get(1)

				                .map(|s| s.trim_matches('"'))

				                .with_context(|| format!("Failed to get second element from {kv:?}"))?;

blitz (Migrated from github.com) commented

2023-01-25 10:58:56 +00:00

Fair point. I've opened #77 to fix this later.

blitz (Migrated from github.com) reviewed

2023-01-25 11:00:09 +00:00

rust/tool/src/install.rs

blitz (Migrated from github.com) commented

2023-01-25 11:00:08 +00:00

If you make it clear in the comments this is fine for me. It does the job.

If someone wants to they can just add a small parser from String to (u32, u32).

If you make it clear in the comments this is fine for me. It does the job. If someone wants to they can just add a small parser from `String` to `(u32, u32)`.