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

["Jason J. Herne" <jjherne@linux.ibm.com>]

On 3/6/19 10:27 AM, Cornelia Huck wrote:
> On Wed, 6 Mar 2019 09:55:40 -0500
> "Jason J. Herne" <jjherne@linux.ibm.com> wrote:
> 
>> On 3/4/19 8:40 AM, Cornelia Huck wrote:
>>> On Fri,  1 Mar 2019 13:59:21 -0500
>>> "Jason J. Herne" <jjherne@linux.ibm.com> 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 (jjherne@linux.ibm.com)