geerlingguy / rpi-clone

A shell script to clone a booted disk on a Raspberry Pi.
https://rpi-clone.jeffgeerling.com/
BSD 3-Clause "New" or "Revised" License
172 stars 10 forks source link

Don't convert partuuids for nonexistent source partitions #8

Closed aaronkollasch closed 4 months ago

aaronkollasch commented 4 months ago

Copying this issue from https://github.com/billw2/rpi-clone/pull/149 as that repo is abandoned.

Issue

When converting to partuuids, if there is an empty element in ${src_partition[@]}, then /etc/fstab will have /dev/ replaced with PARTUUID= without actually changing the device name to a PARTUUID.

I don't have the exact logs when this happened to me, but it looked something like this:

$ sudo rpi-clone -l sda --convert-fstab-to-partuuid

This will change your /etc/fstab, are you sure?  (yes/no): yes

Converting /etc/fstab from device names to PARTUUID
lsblk: /dev/: not a block device
  Editing /etc/fstab, changing /dev/ to
...

and I ended up with a broken fstab:

proc            /proc           proc    defaults          0       0
PARTUUID=mmcblk0p6  /boot           vfat    defaults          0       2
PARTUUID=mmcblk0p7  /               ext4    defaults,noatime  0       1
# a swapfile is not a swap partition, no line here
#   use  dphys-swapfile swap[on|off]  for that

I had previously run sudo rpi-clone -l sda so perhaps that is why there was an empty element in src_partition, I'm not sure.

Changes

Check that ${src_partition[p]} is not empty before editing /etc/fstab.

Then, the command I used will instead exit with an error, which is much safer:

Could not find any sda partition names in /etc/fstab, nothing changed.
geerlingguy commented 4 months ago

Since I don't personally use this feature, if someone else can give the thumbs up on the change, I'd be happy to merge!

framps commented 4 months ago

Wouldn't it make sense to make sure there is no empty element in src_partition? Would be interesting to know you partitioning which causes this issue :wink:

aaronkollasch commented 4 months ago

Wouldn't it make sense to make sure there is no empty element in src_partition? Would be interesting to know you partitioning which causes this issue 😉

My disk has primary partitions sda1 and sda2, and then logical partitions sda5+, so there will be indices in between without partitions, whereas the code iterates through indices 1-n. If you look at where src_partition is created, there is actually a check for empty lines: https://github.com/geerlingguy/rpi-clone/blob/423470cf452e85b69435ad1019db66bb3a8f5deb/rpi-clone#L677-L694

and there's also a check later on in the main code path: https://github.com/geerlingguy/rpi-clone/blob/423470cf452e85b69435ad1019db66bb3a8f5deb/rpi-clone#L1001-L1006

But it doesn't seem to have made it to the part where partuuids get converted: https://github.com/geerlingguy/rpi-clone/blob/423470cf452e85b69435ad1019db66bb3a8f5deb/rpi-clone#L958-L967

(actually I might change my code to match L1003-L1006 and check src_exists[p])

framps commented 4 months ago

I didn't detect Bill already used an array to mark a partition as not to exist :-(. Frankly I would just delete this partition in the list instead of mark it as non existing. But then you can't use a simple array with partition indices any more and will need much more complicated logic :-(

I agree to use src_exists[p] is a much more clean way to handle this exception.