From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([209.51.188.92]:52808) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1h1aOc-0000fJ-Kf for qemu-devel@nongnu.org; Wed, 06 Mar 2019 12:31:59 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1h1aOZ-0006Ai-1v for qemu-devel@nongnu.org; Wed, 06 Mar 2019 12:31:56 -0500 Date: Wed, 6 Mar 2019 18:31:48 +0100 From: Cornelia Huck Message-ID: <20190306183148.665ef286.cohuck@redhat.com> In-Reply-To: <75da1e7f-3be4-ccb9-719e-c40c39f93f79@linux.ibm.com> References: <1551466776-29123-1-git-send-email-jjherne@linux.ibm.com> <1551466776-29123-2-git-send-email-jjherne@linux.ibm.com> <20190304144010.109e5ee1.cohuck@redhat.com> <20190306162729.0698ccb5.cohuck@redhat.com> <75da1e7f-3be4-ccb9-719e-c40c39f93f79@linux.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v3 01/16] s390 vfio-ccw: Add bootindex property and IPLB data List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Jason J. Herne" Cc: qemu-devel@nongnu.org, qemu-s390x@nongnu.org, pasic@linux.ibm.com, alifm@linux.ibm.com, borntraeger@de.ibm.com On Wed, 6 Mar 2019 11:28:37 -0500 "Jason J. Herne" wrote: > On 3/6/19 10:27 AM, Cornelia Huck wrote: > > On Wed, 6 Mar 2019 09:55:40 -0500 > > "Jason J. Herne" wrote: > > > >> On 3/4/19 8:40 AM, Cornelia Huck wrote: > >>> On Fri, 1 Mar 2019 13:59:21 -0500 > >>> "Jason J. Herne" 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.)