From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=39345 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PLXXI-0000GC-FU for qemu-devel@nongnu.org; Thu, 25 Nov 2010 03:50:38 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1PLXXG-0001lX-WC for qemu-devel@nongnu.org; Thu, 25 Nov 2010 03:50:36 -0500 Received: from cantor2.suse.de ([195.135.220.15]:57956 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1PLXXG-0001kd-NQ for qemu-devel@nongnu.org; Thu, 25 Nov 2010 03:50:34 -0500 Message-ID: <4CEE2405.3080906@suse.de> Date: Thu, 25 Nov 2010 09:53:25 +0100 From: Hannes Reinecke MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH 13/15] scsi: Implement alloc_req_iov callback References: <1290597370-21365-1-git-send-email-hare@suse.de> <1290597370-21365-14-git-send-email-hare@suse.de> <20101124165250.GE31124@lst.de> In-Reply-To: <20101124165250.GE31124@lst.de> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Christoph Hellwig Cc: stefanha@gmail.com, qemu-devel@nongnu.org, nab@linux-iscsi.org, kraxel@redhat.com On 11/24/2010 05:52 PM, Christoph Hellwig wrote: > On Wed, Nov 24, 2010 at 12:16:08PM +0100, Hannes Reinecke wrote: >> Add callback to create a request with a predefined iovec. >> This is required for drivers which can use the iovec >> of a command directly. >=20 > What happend to my comment that the iov and non-iov case should share > code? Also what happened to the other comment about not naming the > method implementation different from the method name. >=20 Looked into it. Sure I could be doing it for scsi-disk; for scsi-generic it won't work, though. And it's not much of a code-share to have from it; you'll end up with something like: static SCSIRequest *scsi_new_request(SCSIDevice *d, uint32_t tag, uint32_t lun) { struct iovec *iov; uint8_t *buf; SCSIDiskState *s =3D DO_UPCAST(SCSIDiskState, qdev, d); SCSIRequest *req; SCSIDiskReq *r; buf =3D qemu_blockalign(s->bs, SCSI_DMA_BUF_SIZE); iov =3D qemu_mallocz(sizeof(struct iovec)); iov[0].iov_base =3D buf; req =3D scsi_new_request_iovec(d, tag, lun, iov, 1); r =3D DO_UPCAST(SCSIDiskReq, req, req); r->iov_buf =3D buf; return req; } which doesn't look better than the original: static SCSIRequest *scsi_new_request(SCSIDevice *d, uint32_t tag, uint32_t lun) { SCSIDiskState *s =3D DO_UPCAST(SCSIDiskState, qdev, d); SCSIRequest *req; SCSIDiskReq *r; req =3D scsi_req_alloc(sizeof(SCSIDiskReq), d, tag, lun); r =3D DO_UPCAST(SCSIDiskReq, req, req); r->iov_buf =3D qemu_blockalign(s->bs, SCSI_DMA_BUF_SIZE); r->iov =3D qemu_mallocz(sizeof(struct iovec)); r->iov[0].iov_base =3D r->iov_buf; r->iov_num =3D 1; return req; } But I'm open to suggestions. And for the naming: The SCSI stack is using 'req' for every function accepting SCSIRequest as an argument: hw/scsi.h: SCSIRequest *scsi_req_alloc(size_t size, SCSIDevice *d, uint32_t tag, uint32_t lun); void scsi_req_free(SCSIRequest *req); int scsi_req_parse(SCSIRequest *req, uint8_t *buf); void scsi_req_print(SCSIRequest *req); void scsi_req_complete(SCSIRequest *req); So using 'alloc_req' and 'free_req' is in line with this naming scheme. Using 'alloc_request' and 'free_request' really looked odd in the light of the general usage. Hence I didn't do it. But again, I'm open to suggestions here. Cheers, Hannes --=20 Dr. Hannes Reinecke zSeries & Storage hare@suse.de +49 911 74053 688 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=FCrnberg GF: Markus Rex, HRB 16746 (AG N=FCrnberg)