On Wed, 6 Mar 2019 11:28:37 -0500
"Jason J. Herne" <jjhe...@linux.ibm.com> wrote:
> On 3/6/19 10:27 AM, Cornelia Huck wrote:
> > On Wed, 6 Mar 2019 09:55:40 -0500
> > "Jason J. Herne" <jjhe...@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" <jjhe...@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?
>
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.)
