Re: [Qemu-devel] [PATCH 13/15] scsi: Implement alloc_req_iov callback

[Hannes Reinecke]

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.
> 
> 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.
> 
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 = DO_UPCAST(SCSIDiskState, qdev, d);
    SCSIRequest *req;
    SCSIDiskReq *r;

    buf = qemu_blockalign(s->bs, SCSI_DMA_BUF_SIZE);
    iov = qemu_mallocz(sizeof(struct iovec));
    iov[0].iov_base = buf;
    req = scsi_new_request_iovec(d, tag, lun, iov, 1);
    r = DO_UPCAST(SCSIDiskReq, req, req);
    r->iov_buf = 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 = DO_UPCAST(SCSIDiskState, qdev, d);
    SCSIRequest *req;
    SCSIDiskReq *r;

    req = scsi_req_alloc(sizeof(SCSIDiskReq), d, tag, lun);
    r = DO_UPCAST(SCSIDiskReq, req, req);
    r->iov_buf = qemu_blockalign(s->bs, SCSI_DMA_BUF_SIZE);
    r->iov = qemu_mallocz(sizeof(struct iovec));
    r->iov[0].iov_base = r->iov_buf;
    r->iov_num = 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
-- 
Dr. Hannes Reinecke                   zSeries & Storage
address@hidden                        +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Markus Rex, HRB 16746 (AG Nürnberg)