Re: [PATCH for-5.2 4/6] pc-bios/s390-ccw: Do not bail out early if not finding a SCSI disk

[Cornelia Huck]

On Tue, 28 Jul 2020 20:37:32 +0200
Thomas Huth <thuth@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 <thuth@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;