On 27/09/2024 19.15, Jared Rossi wrote:
On 9/27/24 11:02 AM, Thomas Huth wrote:
On 27/09/2024 02.51, jro...@linux.ibm.com wrote:
From: Jared Rossi <jro...@linux.ibm.com>
Remove panic-on-error from IPL ISO El Torito specific functions so that
error
recovery may be possible in the future.
Functions that would previously panic now provide a return code.
Signed-off-by: Jared Rossi <jro...@linux.ibm.com>
---
pc-bios/s390-ccw/bootmap.h | 17 +++++++---
pc-bios/s390-ccw/s390-ccw.h | 1 +
pc-bios/s390-ccw/bootmap.c | 64 ++++++++++++++++++++++++-------------
3 files changed, 55 insertions(+), 27 deletions(-)
diff --git a/pc-bios/s390-ccw/bootmap.h b/pc-bios/s390-ccw/bootmap.h
index bbe2c132aa..cb5346829b 100644
--- a/pc-bios/s390-ccw/bootmap.h
+++ b/pc-bios/s390-ccw/bootmap.h
@@ -385,17 +385,24 @@ static inline uint32_t iso_733_to_u32(uint64_t x)
#define ISO_PRIMARY_VD_SECTOR 16
-static inline void read_iso_sector(uint32_t block_offset, void *buf,
+static inline int read_iso_sector(uint32_t block_offset, void *buf,
const char *errmsg)
{
- IPL_assert(virtio_read_many(block_offset, buf, 1) == 0, errmsg);
This IPL_assert() made sure that virtio_read_many() returned 0 (for success)...
+ if (virtio_read(block_offset, buf)) {
... so the new code here checks that virtio_read() (which returns the same
error code as virtio_read_many()) does *not* return 0 to signal that there
was an error...
+ puts(errmsg);
+ return 1;
+ }
+ return 0;
}
-static inline void read_iso_boot_image(uint32_t block_offset, void
*load_addr,
+static inline int read_iso_boot_image(uint32_t block_offset, void
*load_addr,
uint32_t blks_to_load)
{
- IPL_assert(virtio_read_many(block_offset, load_addr, blks_to_load)
== 0,
- "Failed to read boot image!");
... and this IPL_assert() also checks that virtio_read_many() returns 0 for
success...
+ if (!virtio_read_many(block_offset, load_addr, blks_to_load)) {
... but this code here checks that virtio_read_many() now returns 0 to
signal that there is an error...
Either I need more coffee or one of the two if-conditions is wrong...?
That "!" looks wrong here? Or do I misunderstood the original IPL_assert()
condition?
Basically all of the IPL_assert() conditions become logically flipped, but
it is
intended. IPL_assert() panics if success condition is NOT met, but in the new
version an error code is returned if an failure condition IS met, so we are
branching on the inverse condition.
Why is one of the two if-statements using a "!" and why is one without it?
+ puts("Failed to read boot image!");
+ return 1;
+ }
+ return 0;
}
#define ISO9660_MAX_DIR_DEPTH 8
...
@@ -706,14 +708,18 @@ static inline uint32_t iso_get_file_size(uint32_t
load_rba)
sec_offset[0] = 0;
while (level >= 0) {
- IPL_assert(sec_offset[level] <= ISO_SECTOR_SIZE,
- "Directory tree structure violation");
+ if (sec_offset[level] > ISO_SECTOR_SIZE) {
+ puts("Directory tree structure violation");
+ return -EIO;
+ }
cur_record = (IsoDirHdr *)(temp + sec_offset[level]);
if (sec_offset[level] == 0) {
- read_iso_sector(sec_loc[level], temp,
- "Failed to read ISO directory");
+ if (virtio_read(sec_loc[level], temp)) {
+ puts("Failed to read ISO directory");
+ return -EIO;
+ }
Any reasons for switching from read_iso_sector() directly to virtio_read()
here?
I think this is just an oversight on my part. I had thought to remove the
read_iso_sector() function entirely since it is just a wrapper for
virtio_read() that becomes redundant once the panic is removed, but it looks
like I wasn't consistent with where I removed it. In my opinion we can remove
read_iso_sector() and just call virtio_read(), but either way it should be
consistent, so I will standardize the calls. I don't see any compelling reason
to keep the read_iso_sector() function since all it is doing is checking the
RC of virtio_read(), which we will want to check anyway to determine if we need
to abort the IPL here.
I agree, I also don't see a compelling reason for keeping read_iso_sector()
anymore.
Thomas
