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

[Jason J. Herne]

On 3/6/19 10:27 AM, Cornelia Huck wrote:
> 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.
Ahh, makes sense now. Thanks for clarifying. I can do it that way, but it does seem a bit 
redundant to allow s390_gen_initial_iplb to be called either with, or without the device 
type data. In the case where it is called without, it just has to make the call to 
s390_get_ccw_device anyway. So, to me, it seems like added lines of code for very little 
benefit. Why not either always call s390_get_ccw_device from inside s390_gen_initial_iplb, 
or always require the parameters to be valid?

--
-- Jason J. Herne (address@hidden)