Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Various improvements #6

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Various improvements #6

wants to merge 4 commits into from

Conversation

glebm
Copy link
Contributor

@glebm glebm commented Mar 29, 2020

Various improvements:

  1. App names can now have spaces
  2. Partition is unmounted before resizing (RG350 16 GiB card resize: 3m45s -> 3m10s).
  3. Creating the data image no longer requires root.

@glebm glebm force-pushed the no-root branch 5 times, most recently from cd06ad8 to 8d01931 Compare March 30, 2020 13:45
Copy link
Collaborator

@mthuurne mthuurne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea behind this PR: being able to build without requiring commands as root on the host.

Most of the comments are about changes that probably shouldn't be in this commit.

The only fundamental concern I have is about file ownership in the created file system: there doesn't seem to be any mechanism yet that ensures the pre-installed applications are going to be owned by the user on the device.

create_data_image.sh Outdated Show resolved Hide resolved
create_data_image.sh Outdated Show resolved Hide resolved
create_data_image.sh Outdated Show resolved Hide resolved
create_data_image.sh Outdated Show resolved Hide resolved
echo "Creating data partition of ${IMAGE_SIZE} MB..."
mkdir -p images
dd if=/dev/zero of=images/data.bin bs=1M count=${IMAGE_SIZE}
MKE2FS_CONFIG=mke2fs.conf /sbin/mkfs.ext4 -b4096 -I128 -m3 \
-E lazy_itable_init=0,lazy_journal_init=0,resize=268435456 \
-E lazy_itable_init=0,lazy_journal_init=0,resize=268435456,root_owner=0:0 \
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What will the owner inside the created file system be for files other than the root directory? Some files should be owned by root, while for example the pre-installed applications should be owned by the user, since the user must be allowed to remove or upgrade them.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On my RG350, the owner of the appears to be default (uid 1000).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess that's because the user IDs on your PC are the same as on OpenDingux: 1000:1000. If you run the script as a different user it probably would not work. An easy fix is to chown 1000:1000 though, as this is guaranteed to be the "od" user's IDs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please push the fix, since you have more understanding of what's going on here?
Note that this is now configured via mke2fs.conf.

@glebm glebm marked this pull request as ready for review April 3, 2020 00:50
@glebm
Copy link
Contributor Author

glebm commented Apr 3, 2020

Re-based on top of #8

The only fundamental concern I have is about file ownership in the created file system: there doesn't seem to be any mechanism yet that ensures the pre-installed applications are going to be owned by the user on the device.

It does work (I don't know exactly why). The files are owned by user default.

Copy link
Collaborator

@pcercuei pcercuei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly I'd just keep the actual code with online resizing, if the difference is this small. It's not like it will be run at each boot. Thoughts?

@glebm
Copy link
Contributor Author

glebm commented Dec 15, 2022

Honestly I'd just keep the actual code with online resizing, if the difference is this small. It's not like it will be run at each boot. Thoughts?

I wouldn't call 30 seconds on a 16 GiB card small!

@glebm
Copy link
Contributor Author

glebm commented Dec 15, 2022

I'll squash the unsigned "Review comments"-type commits once all the concerns have been addressed

@glebm glebm changed the title ./create_data_image.sh: Do not require root Various improvements Dec 15, 2022
@pcercuei
Copy link
Collaborator

Honestly I'd just keep the actual code with online resizing, if the difference is this small. It's not like it will be run at each boot. Thoughts?

I wouldn't call 30 seconds on a 16 GiB card small!

That's a 15% difference, it's not that much. My point was more that this is executed only once after flashing, so even if the difference is x2, it still wouldn't matter much, since users would almost never experience it.

My gripe with the offline resizing code is the mount_data_partition() function, which does not follow what S03mountdata.sh is doing.
I see two solutions:

  • S03mountdata.sh could define the mount_data_partition() function, and since all .sh init scripts are sourced by rcS, it should be visible in your resize script;
  • Alternatively just reboot once the resizing is done. Then you don't have to care about mounting the data partition again.

@glebm
Copy link
Contributor Author

glebm commented Dec 15, 2022

That's a 15% difference, it's not that much. My point was more that this is executed only once after flashing, so even if the difference is x2, it still wouldn't matter much, since users would almost never experience it.

I'd say the initial boot setup experience also matters, especially since it's not difficult for us to implement this

@glebm glebm force-pushed the no-root branch 3 times, most recently from 44f94bb to d68dac0 Compare December 15, 2022 12:34
@glebm
Copy link
Contributor Author

glebm commented Dec 15, 2022

S03mountdata.sh could define the mount_data_partition() function, and since all .sh init scripts are sourced by rcS, it should be visible in your resize script;

Changed to simply call S03mountdata.sh

This results in a faster resize as resize2fs can perform an "offline"
resize instead of an "online" one.

Timings for me on RG350 with a stock 16G SD card are:
online: 3m45s
offline: 3m10s

Signed-off-by: Gleb Mazovetskiy <[email protected]>
Unused because root is no longer required.

Signed-off-by: Gleb Mazovetskiy <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants