A few assorted fixes and cleanups

martingkelly commented 8 years ago

Hi, thanks for the great project! These are just a few things I noticed while running the flash utility.

StefanScherer commented 8 years ago

I also encountered a problem that the flash script just exits if no SD card could be found. Can you reproduce it with your PR? Without the PR I can see that the flash script is waiting for me to plug in a SD card. Can you please check and update the PR?

martingkelly commented 8 years ago

Ah, I see what you're referring to. This is caused by set -e, which is causing flash to fail when df | grep /media returns non-0. Although we could fix up this check, there's a deeper issue here.

On my system, waiting for an SD card doesn't work properly, for the same reason (my system doesn't do auto-mounting).

On my system:

martin@columbia:~/flash$ Linux/flash --hostname NAME hypriot-rpi-20160306-192317.img 
No SD card found. Please insert SD card, I'll wait for it...

When you insert an SD card, it still doesn't continue. The issue is caused by the code assuming that an inserted SD card will automatically get mounted into /media, which is not necessarily true:

      echo "No SD card found. Please insert SD card, I'll wait for it..."
      while [ "${disk}" == "" ]; do
        sleep 1
        disk=$(df | grep /media | cut -f1 -d " " | sed -e 's/[0-9]//'|sed -e 's/p.*$//' | sort | uniq)
      done

Unfortunately, the fix for this isn't so simple, as it's not particularly easy to detect an inserted SD card; it would be easy to detect an inserted drive (monitor /sys/block/*), but determining if it's an SD card vs some other drive would not be so easy. Personally, I would be in favor of removing the code for handling this and just erroring out rather than risk a false positive (which could erase data you care about), but there are other ways to address it too.

What do you think? I could fix this up in a variety of ways, but I'll defer to your judgment regarding what you prefer.

StefanScherer commented 8 years ago

@martingkelly Thanks for the details. So it worked on Linux systems with automounting SD cards etc. But if this does not work for all Linux users and checking all these details would overcomplicate the script I prefer removing this feature on Linux.

martingkelly commented 8 years ago

OK, I added a patch to remove the check for SD card insertion. With this patch, the behavior is as follows:

If you specify -d, it will use the device what you give but still prompt you to make sure.
If you don't specify -d, it will show you your devices and let you pick one; if you leave off /dev (e.g. just sdb), it will also helpfully expand it to a full device path (e.g. /dev/sdb). It will still prompt you for safety.

What do you think about this method?

See below output with the patch:

martin@columbia:~/flash$ ~/flash/Linux/flash --hostname d -d /dev/sdb hypriot-rpi-20160306-192317.img
NAME                        SIZE TYPE  MOUNTPOINT
sda                       223.6G disk  
├─sda1                      243M part  /boot
├─sda2                        1K part  
└─sda5                    223.3G part  
  └─sda5_crypt            223.3G crypt 
    ├─columbia--vg-root   214.3G lvm   /
    └─columbia--vg-swap_1   9.1G lvm   [SWAP]

Is /dev/sdb correct? 
martin@columbia:~/flash$ ~/flash/Linux/flash --hostname d hypriot-rpi-20160306-192317.img
NAME                        SIZE TYPE  MOUNTPOINT
sda                       223.6G disk  
├─sda1                      243M part  /boot
├─sda2                        1K part  
└─sda5                    223.3G part  
  └─sda5_crypt            223.3G crypt 
    ├─columbia--vg-root   214.3G lvm   /
    └─columbia--vg-swap_1   9.1G lvm   [SWAP]
Please pick your device: sdb

Is /dev/sdb correct?

StefanScherer commented 8 years ago

OK, I'm willing to merge. Let's see what other Linux users of the flash script tell about the new behaviour. Could you help me a little bit with the Linux version in the future?

StefanScherer commented 8 years ago

With this PR I wanted to try out the new squash on merge feature of GitHub. Haven't done this before.

martingkelly commented 8 years ago

Heh, "squash on merge" is kind of amusing, as it turns all the commits into one. This has good and bad things about it, depending on how you want the tree to look :). Anyway, thanks for the merge. I'm happy to help out more in the future; my email is martin@martingkelly.com if you need to reach me.

hypriot / flash

A few assorted fixes and cleanups #54