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

Dietpi-Imager | FIx formula to get starting sector of root partition #3119

Merged
merged 2 commits into from
Sep 23, 2019
Merged

Dietpi-Imager | FIx formula to get starting sector of root partition #3119

merged 2 commits into from
Sep 23, 2019

Conversation

sal666
Copy link
Contributor

@sal666 sal666 commented Sep 20, 2019

Current method of obtaining starting sector of root partition is making some assumptions that are not always met. When running fdisk you can't assume that the following is always true:

  • root partition is the last partition of the disk
  • fdisk will not display any warning (like 'Partition table entries are not in disk order.')

Current method of obtaining starting sector of root partition is making some assumptions that are not always met. When running fdisk you can't assume that the following is always true:
- root partition is the last partition of the disk
- fdisk will not display any warning (like 'Partition table entries are not in disk order.')
@MichaIng MichaIng added this to the v6.26 milestone Sep 21, 2019
@MichaIng MichaIng added Enhancement 💨 META Everything that is not code related, e.g. GitHub, Wiki, website, community labels Sep 21, 2019
@MichaIng
Copy link
Owner

MichaIng commented Sep 21, 2019

@sal666
Many thanks for this. Indeed I was already thinking to simply expect root partition as last partition throughout the script and skip the related input dialog as well. At least all pre-images we use would work with this. However, if we already allow to choose root partition currently, your change simply makes sense and it is always better to be more compatible.

I made a small failsafe addition, so the root devices can only match again line start, and not inside the fdisk output header somewhere.

But as a general though about having root partition not the last partition:

  • If this is the case, the whole partition + fs shrinking does not make any sense.
  • The whole reason for this is to create as small as possible image files, to minimise download and extraction effort and most importantly allow to flash the image onto very small drives/cards.
  • But with any partition behind root partition, the image size will stay the same, while shrinking the root partition then only creates a "hole". In such case, the partitions need to be reordered, e.g. moved by tools like gparted. But having a root partition surrounded by other ones then again breaks any expansion on first boot, which makes the image unusable.
  • So actually, to avoid any unnecessary use, partitioning and confusion, it makes sense to simply ask the user if the root partition is the last one (printing the partition table, so one can see easily), and if "no" is selected, ship all the partition handling and going straight forward the dd image file creation, 7z archiving etc. The image file will then be as large as the hole drive, this should be as well mentioned within the dialog.
  • Or indeed because this is really no wise partition table at all, we could then indeed simply exit the script and advise user to remove or merge partitions that way that root partition is really the last one. What you think?

Copy link
Contributor Author

@sal666 sal666 left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@sal666
Copy link
Contributor Author

sal666 commented Sep 23, 2019

@MichaIng
It all depends on the intended use of the script. Is it for general use or just to prepare the distribution of self made DietPi images? For general use we couldn't make all these assumptions. In fact, proper Linux installations would imply specific partitions for /var, /home, swap -although that's not common on SBCs. On the other hand, if the real use case of the script is to prepare images of DietPi after having followed the procedure described in https://github.com/MichaIng/DietPi/wiki/How-to-create-a-DietPi-image-for-x86_64-PCs-(UEFI), then is right to assume that the root partition is the second partition and that there is no other partition after it. Even more, root partition will be 1.5GB in size, so maybe there is no need to reduce it as we don't expect the OS to be usable on smaller disks.
One test that I feel is missing is if the target disk has any partition already mounted. Maybe when asking the user to select the disk to create the image from the list of disks should be limited to disks with no mounted partitions.

@MichaIng
Copy link
Owner

@sal666
IMO it is quite important to have the image size as small as possible. This also reduces the dd writes to the SDcard initially and of course the writes to the host system + extraction etc. In many cases software producers do not take care much about this, but there are many people with weak non-durable drives and SDcards out there, producing a large amount of bug reports when those start to die, so we should do everything to reduce any disk I/O as much as possible, especially if it is this simple to do via script 😉.

So yeah, I will merge this PR for now. Since we allow choosing the root partition in the first place currently, which does not make any sense if we later expect it to be the last partition.
We can do other fine tuning later, e.g. skip all partition and FS resizing when it's not the last partition and advice user to do manually if required. I am just not sure if the auto image file creation and 7zing is a good thing then, e.g. a 64 GiB file would be produced from a 64 GiB SDcard/drive... Finally I still think, without any partition resizing, no image file should be produced, thus the script should exit in the first place.

@MichaIng MichaIng merged commit 716e885 into MichaIng:dev Sep 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement 💨 META Everything that is not code related, e.g. GitHub, Wiki, website, community
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants