On Tue, 28 Jul 2020 20:37:32 +0200
Thomas Huth <th...@redhat.com> wrote:
> In case the user did not specify a boot device, we want to continue
> looking for other devices if there are no valid SCSI disks on a virtio-
> scsi controller. As a first step, do not panic in this case and let
> the control flow carry the error to the upper functions instead.
>
> Signed-off-by: Thomas Huth <th...@redhat.com>
> ---
> pc-bios/s390-ccw/main.c | 13 +++++++++----
> pc-bios/s390-ccw/s390-ccw.h | 2 +-
> pc-bios/s390-ccw/virtio-blkdev.c | 7 ++++---
> pc-bios/s390-ccw/virtio-scsi.c | 25 ++++++++++++++++++-------
> pc-bios/s390-ccw/virtio-scsi.h | 2 +-
> 5 files changed, 33 insertions(+), 16 deletions(-)
>
> diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
> index 9477313188..3cd01cd80f 100644
> --- a/pc-bios/s390-ccw/main.c
> +++ b/pc-bios/s390-ccw/main.c
> @@ -218,7 +218,7 @@ static void find_boot_device(void)
> IPL_assert(found, "Boot device not found\n");
> }
>
> -static void virtio_setup(void)
> +static bool virtio_setup(void)
Hm... I'm always wondering what to make of a function returning bool if
it is not of the "check something" variety. For a function called
virtio_setup(), I'd expect it to setup something, but would be unsure
what it meant if it returned true or false. Maybe better make it return
0 or a negative error?
(also applies to the other setup functions in this patch)
> {
> VDev *vdev = virtio_get_device();
> QemuIplParameters *early_qipl = (QemuIplParameters *)QIPL_ADDRESS;
(...)
> @@ -288,9 +288,10 @@ void virtio_blk_setup_device(SubChannelId schid)
> "Config: CDB size mismatch");
>
> sclp_print("Using virtio-scsi.\n");
> - virtio_scsi_setup(vdev);
> - break;
> + return virtio_scsi_setup(vdev);
You now have one case with a direct return, one that does not return,
and one that just continues. Can we make that a bit more consistent?
> default:
> panic("\n! No IPL device available !\n");
> }
> +
> + return true;
> }
> diff --git a/pc-bios/s390-ccw/virtio-scsi.c b/pc-bios/s390-ccw/virtio-scsi.c
> index eddfb8a7ad..4d05b02ed0 100644
> --- a/pc-bios/s390-ccw/virtio-scsi.c
> +++ b/pc-bios/s390-ccw/virtio-scsi.c
> @@ -194,7 +194,12 @@ static bool scsi_read_capacity(VDev *vdev,
>
> /* virtio-scsi routines */
>
> -static void virtio_scsi_locate_device(VDev *vdev)
> +/*
> + * Tries to locate a SCSI device and adds that information to the
> + * vdev->scsi_device structure.
"and adds the information for the found device" ?
> + * Returns true if SCSI device could be located, false otherwise
> + */
> +static bool virtio_scsi_locate_device(VDev *vdev)
Here, I think a bool is fine. 0/-ENODEV would also make sense.
> {
> const uint16_t channel = 0; /* again, it's what QEMU does */
> uint16_t target;
