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

[Cornelia Huck]

On Wed, 6 Mar 2019 11:28:37 -0500
"Jason J. Herne" <address@hidden> wrote:

> 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?
> 

My main goal was to avoid needing to do the casting twice. If that
makes the code too ugly, we can also go back to making
s390_get_ccw_device accept a NULL devtype (which I'd find less ugly
than needing to pass in an unused variable.)