Re: [qemu-s390x] [PULL 01/19] s390 vfio-ccw: Add bootindex property and IPLB data

[Peter Maydell]

On Thu, 25 Apr 2019 at 14:21, Cornelia Huck <address@hidden> wrote:
>
> From: "Jason J. Herne" <address@hidden>
>
> Add bootindex property and iplb data for vfio-ccw devices. This allows us to
> forward boot information into the bios for vfio-ccw devices.
>
> Refactor s390_get_ccw_device() to return device type. This prevents us from
> having to use messy casting logic in several places.
>
> Signed-off-by: Jason J. Herne <address@hidden>
> Acked-by: Halil Pasic <address@hidden>
> Reviewed-by: Cornelia Huck <address@hidden>
> Reviewed-by: Thomas Huth <address@hidden>
> Message-Id: <address@hidden>
> [thuth: fixed "typedef struct VFIOCCWDevice" build failure with clang]
> Signed-off-by: Thomas Huth <address@hidden>

Hi; Coverity has a complaint (CID 1401098) about the use of
object_dynamic_cast() in this function. It looks like it's just
the result of code motion making it forget we'd dismissed the
warning before, but maybe we can avoid it entirely...

> @@ -335,20 +360,22 @@ static bool s390_gen_initial_iplb(S390IPLState *ipl)
>  {
>      DeviceState *dev_st;
>      CcwDevice *ccw_dev = NULL;
> +    SCSIDevice *sd;
> +    int devtype;
>
>      dev_st = get_boot_device(0);
>      if (dev_st) {
> -        ccw_dev = s390_get_ccw_device(dev_st);
> +        ccw_dev = s390_get_ccw_device(dev_st, &devtype);
>      }
>
>      /*
>       * Currently allow IPL only from CCW devices.
>       */
>      if (ccw_dev) {
> -        SCSIDevice *sd = (SCSIDevice *) object_dynamic_cast(OBJECT(dev_st),
> -                                                            
> TYPE_SCSI_DEVICE);
> -
> -        if (sd) {
> +        switch (devtype) {
> +        case CCW_DEVTYPE_SCSI:
> +            sd = (SCSIDevice *) object_dynamic_cast(OBJECT(dev_st),
> +                                                           TYPE_SCSI_DEVICE);

Coverity doesn't like the use of object_dynamic_cast() without a
check that the return value isn't NULL before we dereference
it a few lines further down.

I think that if we know this cast must always succeed, we
could instead just write
  SCSIDevice *sd = SCSI_DEVICE(dev_st);

On the other hand if the cast might not succeed because dev_st
isn't necessarily of the right type, then we should check it
for NULL and handle that appropriately.

thanks
-- PMM