Re: [qemu-s390x] [PATCH v3 01/16] s390 vfio-ccw: Add bootindex property and IPLB data

[Cornelia Huck]

On Wed, 6 Mar 2019 09:55:40 -0500
"Jason J. Herne" <address@hidden> wrote:

> On 3/4/19 8:40 AM, Cornelia Huck wrote:
> > On Fri,  1 Mar 2019 13:59:21 -0500
> > "Jason J. Herne" <address@hidden> wrote:

> >> @@ -532,7 +559,7 @@ void s390_ipl_reset_request(CPUState *cs, enum 
> >> s390_reset reset_type)
> >>           !ipl->netboot &&
> >>           ipl->iplb.pbt == S390_IPL_TYPE_CCW &&
> >>           is_virtio_scsi_device(&ipl->iplb)) {
> >> -        CcwDevice *ccw_dev = s390_get_ccw_device(get_boot_device(0));
> >> +        CcwDevice *ccw_dev = s390_get_ccw_device(get_boot_device(0), 
> >> &devtype);  
> > 
> > It feels wrong to have a variable that is not used later... what about
> > either
> > - making s390_get_ccw_device() capable of handling a NULL second
> >    parameter, or
> > - (what I think would be nicer) passing in the devtype as an optional
> >    parameter to gen_initial_iplb() so you don't need to do the casting
> >    dance twice?
> >   
> 
> I'm with you on everything suggested for this patch except this last item. 
> I'm not sure 
> what you are suggesting here. I can easily visualize passing NULL for devtype 
> when we want 
> to ignore it but I'm not sure what you mean by 'optional parameter'

Hm, actually you'd need the device as well :) Basically,

static bool s390_gen_initial_iplb(S390IPLState *ipl, CcwDevice *ccw_dev, int 
devtype)
{
(...)
    if (!ccw_dev) {
        //get ccw_dev, which also gives us the devtype
    }

    if (ccw_dev) {
        //as before; devtype is valid here
        (...)
        return true;
    }
    return false;
}

So you can call it with s390_gen_initial_iplb(ipl, NULL, 0) if you
don't have the values already.