Re: [PATCH 09/18] pc-bios/s390-ccw: Remove panics from SCSI IPL path

[Thomas Huth <thuth@redhat.com>]

On 27/09/2024 02.51, jrossi@linux.ibm.com wrote:
> From: Jared Rossi <jrossi@linux.ibm.com>
> 
> Remove panic-on-error from virtio-scsi IPL 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 <jrossi@linux.ibm.com>
> 
> ---
...
> @@ -572,23 +575,37 @@ static void zipl_load_segment(ComponentEntry *entry)
>               }
>               address = virtio_load_direct(cur_desc[0], cur_desc[1], 0,
>                                            (void *)address);
> -            IPL_assert(address != -1, "zIPL load segment failed");
> +            if (!address) {

Shouldn't that be "if (address == -1)" or "if (address < 0)" instead?

Hmm, virtio_load_direct() seems to return an "unsigned long", so maybe that 
one rather needs to be fixed, too?

> +                puts("zIPL load segment failed");
> +                return -EIO;
> +            }
>           }
>       } while (blockno);
> +
> +    return 0;
>   }
...
> @@ -78,24 +84,30 @@ static void prepare_request(VDev *vdev, const void *cdb, int cdb_size,
>       }
>   }
>   
> -static inline void vs_io_assert(bool term, const char *msg)
> +static inline bool vs_io_assert(bool term, const char *msg)
>   {
> -    if (!term) {
> -        virtio_scsi_verify_response(&resp, msg);
> +    if (!term && !virtio_scsi_verify_response(&resp, msg)) {

Should that be "||" instead of "&&" ?

> +        return false;
>       }
> +
> +    return true;
>   }
>   
> -static void vs_run(const char *title, VirtioCmd *cmd, VDev *vdev,
> +static int vs_run(const char *title, VirtioCmd *cmd, VDev *vdev,
>                      const void *cdb, int cdb_size,
>                      void *data, uint32_t data_size)
>   {
>       prepare_request(vdev, cdb, cdb_size, data, data_size);
> -    vs_io_assert(virtio_run(vdev, VR_REQUEST, cmd) == 0, title);
> +    if (!vs_io_assert(virtio_run(vdev, VR_REQUEST, cmd) == 0, title)) {
> +        puts(title);

Shouldn't there be a "return -1;" or something similar here?

> +    }
> +
> +    return 0;
>   }

  Thomas