make_ext4fs: init at unstable-2017-05-21 #67744

samueldr · 2019-08-30T03:09:23Z

Motivation for this change

The upstream tools for ext4 does not create reproducible images. make_ext4fs, from the OpenWRT project (initially from the now merged LEDE project) is recommended by Reproducible Builds for the purpose of reproducibly producing ext4fs filesystems.

This is needed if we want sd_image to be reproducible. This is also work needed towards mobile-nixos.

Things to do

Have other contributors review the patches.

I have written a patch adding support to specify an UUID. I do not think I should be trusted with bare C. I want both stylistic review (though desire it to be good for use upstream) and a security review. Though, I figure the security issues are likely of extremely low severity, if any. I may have done a off-by-one or two in the code, though from testing if there are they are benign.

This is the only reason this PR is a draft. The quality of the patches are still unknown. Once the patches have been reviewed I will do the process with upstream to get them upstreamed.

Things done

☑️ Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
Built on platform(s)
- ☑️ NixOS
- 🔲 macOS
- 🔲 other Linux distributions
🔲 Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
☑️ Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
☑️ Tested execution of all binary files (usually in ./result/bin/)
☑️ Determined the impact on package closure size (by running nix path-info -S before and after)
☑️ Ensured that relevant documentation is up to date
☑️ Fits CONTRIBUTING.md.

Of note: I have a test harness in the image-builder work for mobile-nixos which tests software. It will continue testing it while I expand the scope of the image building. Additionally, that image-builder infra is intended to be upstreamed in NixOS proper.

stale · 2020-06-01T11:03:27Z

Thank you for your contributions.
This has been automatically marked as stale because it has had no activity for 180 days.
If this is still important to you, we ask that you leave a comment below. Your comment can be as simple as "still important to me". This lets people see that at least one person still cares about this. Someone will have to do this at most twice a year if there is no other activity.
Here are suggestions that might help resolve this more quickly:

Search for maintainers and people that previously touched the
related code and @ mention them in a comment.
Ask on the NixOS Discourse. 3. Ask on the #nixos channel on
irc.freenode.net.

ajs124

I'm really not a C expert, but this mostly seems fine to me, except for some style notes and possible changes. Have you asked upstream what they think of this?

ajs124 · 2020-06-01T11:28:14Z

pkgs/tools/filesystems/make_ext4fs/0001-uuid-add-facilities-to-parse-and-print-UUIDs.patch

+{
+	/* Index in uuid */
+	int u = 0;
+	for (int i = 0; i < UUID_STR_LEN-1; i++) {


The rest of this codebase seems to declare int i before the loop. Apparently doing it the way you do here is a C99 feature, which they might be trying to avoid.

ajs124 · 2020-06-01T11:28:19Z

pkgs/tools/filesystems/make_ext4fs/0001-uuid-add-facilities-to-parse-and-print-UUIDs.patch

+		if (i == 8 || i == 13 || i == 18 || i == 23) {
+			strcpy(result+i, "-");
+		}
+		else {


They seem to be using } else { instead of putting it on a separate line.

ajs124 · 2020-06-01T11:30:34Z

pkgs/tools/filesystems/make_ext4fs/0001-uuid-add-facilities-to-parse-and-print-UUIDs.patch

+	}
+	for (int i = 0; i < UUID_STR_LEN - 1; i++) {
+		if (i == 8 || i == 13 || i == 18 || i == 23) {
+			/* Skip hyphens */


Instead of this "empty if -> else", you could just be doing an if !(). Not sure what's better style, though.

ajs124 · 2020-06-01T11:34:04Z

pkgs/tools/filesystems/make_ext4fs/0002-make_ext4fs-allow-setting-a-specific-UUID.patch

+ 		printf("    Inode size: %d\n", info.inode_size);
+ 		printf("    Label: %s\n", info.label);
+		if (info.with_uuid) {
+			char uuid[UUID_STR_LEN] = "";


You could use char uuid[UUID_STR_LEN] = { 0 }; to initialize char arrays.

ajs124 · 2020-06-01T11:35:30Z

pkgs/tools/filesystems/make_ext4fs/0001-uuid-add-facilities-to-parse-and-print-UUIDs.patch

+			strcpy(result+i, "-");
+		}
+		else {
+			/* This writes \0 at the end of every invocation */


It also writes the actual value there? Why this comment?

I think this was me being confused at needing to use 3 as the length of the write.

samueldr · 2020-06-01T20:17:28Z

Oh, forgot about this.

At the moment I tried getting it touch with upstream on their recommended and got literally no response. Well partly false, I didn't post on the wrong mailing list, but maybe I should have to be told where to post instead. It's a project under the OpenWRT organization, but it is unclear how to contribute.

samueldr · 2020-06-01T20:54:07Z

Thanks for the review! I have addressed all comments I believe.

See NixOS/mobile-nixos#156 for a more understandable diff of the patches, since the force push updated quite a bunch since I rebased on top of current unstable.

flokli · 2020-06-21T20:20:50Z

This seems to suggest it appeared in AOSP before being imported into OpenWRT.

Did you get some way to contact the current developers? If there's no mailing list, maybe just send them an email ;-)

samueldr · 2020-06-21T20:55:28Z

Yes, it was made for AOSP before being made a part of LEDE, and now OpenWRT.

(I still don't like mailing lists, so I've been masterfully procrastinating.)

stale · 2020-12-19T04:31:56Z

I marked this as stale due to inactivity. → More info

flokli · 2020-12-20T12:38:31Z

I still don't think we should ship these patches in-tree, but send them upstream and fetchurl from there. They're generically useful, not only to NixOS.

@samueldr can you push them upstream and then update the expression, so this can be merged?

stale · 2021-06-18T13:30:12Z

I marked this as stale due to inactivity. → More info

RaitoBezarius · 2022-12-04T10:52:22Z

Can I do something to help this PR getting merged, it would be nice to have increased reproducibility for make-disk-image!

samueldr · 2022-12-04T22:52:44Z

look and see who is the upstream, nowadays
update the package if needed
plan to sent the changes to upstream

OpenWRT went through a weird phase in the past few years with LEDE forking off, and then merging back.

Then, as far as making things more reproducible, that is another goal: we probably want to invest in better expressions to make disk images, rather than just update the multiple messy ones we have. Though that is another goal for another time.

RaitoBezarius · 2022-12-04T22:59:07Z

look and see who is the upstream, nowadays

update the package if needed

plan to sent the changes to upstream

OpenWRT went through a weird phase in the past few years with LEDE forking off, and then merging back.

Then, as far as making things more reproducible, that is another goal: we probably want to invest in better expressions to make disk images, rather than just update the multiple messy ones we have. Though that is another goal for another time.

This is part of #203641 effort (unifying make-disk-image slowly) :)

ofborg bot added 8.has: package (new) 11.by: package-maintainer 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 labels Aug 30, 2019

stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 1, 2020

ajs124 reviewed Jun 1, 2020

View reviewed changes

stale bot removed 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md labels Jun 1, 2020

samueldr added 2 commits June 1, 2020 16:41

make_ext4fs: init at unstable-2017-05-21

5f3a32b

make_ext4fs: Adds support for specifying UUID

b9f161c

samueldr force-pushed the pkg/make_ext4fs/init branch from 19a57c4 to b9f161c Compare June 1, 2020 20:52

ofborg bot added the 10.rebuild-linux: 1 label Jun 1, 2020

samueldr mentioned this pull request Jun 21, 2020

nixos/make-ext4-fs: increase fudge factor from 1.03 to 1.10 #91214

Merged

10 tasks

stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Dec 19, 2020

stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Dec 20, 2020

stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 18, 2021

stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Dec 4, 2022

stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 18, 2023

wegank added the 2.status: merge conflict label Mar 20, 2024

stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Mar 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

make_ext4fs: init at unstable-2017-05-21 #67744

make_ext4fs: init at unstable-2017-05-21 #67744

samueldr commented Aug 30, 2019

stale bot commented Jun 1, 2020

ajs124 left a comment

ajs124 Jun 1, 2020

ajs124 Jun 1, 2020

ajs124 Jun 1, 2020

ajs124 Jun 1, 2020

ajs124 Jun 1, 2020

samueldr Jun 1, 2020

samueldr commented Jun 1, 2020

samueldr commented Jun 1, 2020 •

edited

flokli commented Jun 21, 2020

samueldr commented Jun 21, 2020

stale bot commented Dec 19, 2020

flokli commented Dec 20, 2020

stale bot commented Jun 18, 2021

RaitoBezarius commented Dec 4, 2022

samueldr commented Dec 4, 2022

RaitoBezarius commented Dec 4, 2022

make_ext4fs: init at unstable-2017-05-21 #67744

Are you sure you want to change the base?

make_ext4fs: init at unstable-2017-05-21 #67744

Conversation

samueldr commented Aug 30, 2019

Motivation for this change

Things to do

Things done

stale bot commented Jun 1, 2020

ajs124 left a comment

Choose a reason for hiding this comment

ajs124 Jun 1, 2020

Choose a reason for hiding this comment

ajs124 Jun 1, 2020

Choose a reason for hiding this comment

ajs124 Jun 1, 2020

Choose a reason for hiding this comment

ajs124 Jun 1, 2020

Choose a reason for hiding this comment

ajs124 Jun 1, 2020

Choose a reason for hiding this comment

samueldr Jun 1, 2020

Choose a reason for hiding this comment

samueldr commented Jun 1, 2020

samueldr commented Jun 1, 2020 • edited

flokli commented Jun 21, 2020

samueldr commented Jun 21, 2020

stale bot commented Dec 19, 2020

flokli commented Dec 20, 2020

stale bot commented Jun 18, 2021

RaitoBezarius commented Dec 4, 2022

samueldr commented Dec 4, 2022

RaitoBezarius commented Dec 4, 2022

samueldr commented Jun 1, 2020 •

edited