[copy_to_esp] Limit search to target disk only #110
Conversation
|
Build $6636 |
part2/stages/Functions/install-main
Outdated
| @@ -17,6 +17,9 @@ | |||
| # Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA | |||
| # | |||
|
|
|||
| . ${DISK_CONF} | |||
| # ^^^ defines TARGET_DISK | |||
There was a problem hiding this comment.
Nit: please put your comments above the code you are explaining.
There was a problem hiding this comment.
Normally I would...but there are a number of files in installer.git that have this same "^^^ defines" structure, so I went with that for consistency.
There was a problem hiding this comment.
So it is used in 17 places where as the more traditional style is used more often. With that said, regardless if I disagree with that format, you are correct there is precedent for its usage.
There was a problem hiding this comment.
Well it's used specifically where configuration files are sourced. I'm not sure where that originated but I'm not a fan of it either. Just going with what I see.
part2/stages/Functions/install-main
Outdated
| @@ -591,7 +594,7 @@ create_efi_boot_entries() | |||
|
|
|||
| copy_to_esp() | |||
| { | |||
| local ESP=$(sfdisk -l -q | grep "EFI System" | awk '{ print $1 }') | |||
| local ESP=$(sfdisk -l -q "/dev/${TARGET_DISK}" | grep "EFI System" | awk '{ print $1 }') | |||
There was a problem hiding this comment.
Is TARGET_DISK defined during an OTA/upgrade?
There was a problem hiding this comment.
No it's not, good point. Working on a fix.
part2/stages/Functions/install-main
Outdated
| # TARGET_DISK not defined in upgrades; locate our | ||
| # installation disk manually. | ||
| if [ -z "${TARGET_DISK}" ]; then | ||
| local part="$(vgs xenclient --noheadings -o pv_name | sed 's/\s*//g' | sed 's|^/dev/||g')" |
There was a problem hiding this comment.
A since sed instance can run multiple commands like this: sed -e 's/\s*//g' -e 's|^/dev/||'
part2/stages/Functions/install-main
Outdated
| local TARGET_DISK="$(readlink /sys/class/block/$part | awk -F/ '{ print $(NF-1) }')" | ||
| fi | ||
|
|
||
| local ESP=$(sfdisk -l -q "/dev/${TARGET_DISK}" | grep "EFI System" | awk '{ print $1 }') |
There was a problem hiding this comment.
You could elminate grep with sfdisk -l -q "/dev/${TARGET_DISK}" | awk '/EFI System/{ print $1 }'
There was a problem hiding this comment.
I'm going to leave the grep call in there for the sole reason of the operation being much clearer to someone who doesn't fully know the awk language (like myself).
part2/stages/Functions/install-main
Outdated
|
|
||
| # get disk from partition (e.g. sda2 --> sda). using | ||
| # readlink instead of simply stripping off trailing | ||
| # numbers to cover other disk formats (e.g. nvme) |
There was a problem hiding this comment.
The comment makes it sound like it's readlink resolving to the disk, but it's really the sysfs directory hierarchy. Maybe say "Use readlink to get the /sys/device entry and then use the parent directory as the disk name"
|
Need this in stable-9 too, please submit a PR when the changes are approved for master. |
|
Install failed with: |
part2/stages/Functions/install-main
Outdated
| # work for other disk formats (e.g. nvme). this method | ||
| # will get the /sys/device entry and use the parent dir | ||
| # as the disk name. | ||
| local TARGET_DISK="$(readlink /sys/class/block/$part | awk -F/ '{ print $(NF-1) }')" |
There was a problem hiding this comment.
The installer already has helpers to handle this kind of things:
local devnode="$(vgs xenclient --noheadings -o pv_name)"
local disk=$(get_devnode_disk "${devnode}")
local part=$(get_devnode_part "${devnode}")
|
@jean-edouard ah I see what happened here...when I tested this I modified the installer while it was live, and I believe I waited until the installation was found before doing so. Let me do another round of testing on this. |
OXT-1632 Signed-off-by: Nicholas Tsirakis <tsirakisn@ainfosec.com>
|
Ready for test again. |
|
Build $6645 |
|
LGTM, merging soon. |
@dpsmith tagging for visibility
OXT-1632
Signed-off-by: Nicholas Tsirakis tsirakisn@ainfosec.com