Re: [Qemu-devel] [PATCH 2/3] scsi: replace 'tag' with 'hba_private' pointer

[Paolo Bonzini <pbonzini@redhat.com>]

On 07/01/2011 09:42 AM, Hannes Reinecke wrote:
> 'tag' is just an abstraction to identify the command
> from the driver. So we should make that explicit by
> replacing 'tag' with a driver-defined pointer 'hba_private'.
> This saves the lookup for driver handling several commands
> in parallel.

This makes tracing a bit harder to follow.  Perhaps you can keep the 
transport tag (a uint64_t) in the SCSIRequest for debugging purposes?

> Signed-off-by: Hannes Reinecke<hare@suse.de>
> ---
>   hw/esp.c          |    2 +-
>   hw/lsi53c895a.c   |   17 ++++++++---------
>   hw/scsi-bus.c     |   22 +++++++++++-----------
>   hw/scsi-disk.c    |    5 ++---
>   hw/scsi-generic.c |    4 ++--
>   hw/scsi.h         |    8 ++++----
>   hw/spapr_vscsi.c  |   41 ++++++++++++-----------------------------
>   hw/usb-msd.c      |   10 +++++-----
>   trace-events      |   14 +++++++-------
>   9 files changed, 52 insertions(+), 71 deletions(-)
>
> diff --git a/hw/esp.c b/hw/esp.c
> index 6d3f5d2..912ff89 100644
> --- a/hw/esp.c
> +++ b/hw/esp.c
> @@ -244,7 +244,7 @@ static void do_busid_cmd(ESPState *s, uint8_t *buf, uint8_t busid)
>
>       DPRINTF("do_busid_cmd: busid 0x%x\n", busid);
>       lun = busid&  7;
> -    s->current_req = scsi_req_new(s->current_dev, 0, lun);
> +    s->current_req = scsi_req_new(s->current_dev, lun, s);

Might as well pass NULL here.  The hba_private value is basically 
unnecessary when the adapter doesn't support tagged command queuing.

> diff --git a/hw/usb-msd.c b/hw/usb-msd.c
> index 86582cc..4e2ea03 100644
> --- a/hw/usb-msd.c
> +++ b/hw/usb-msd.c
> @@ -216,8 +216,8 @@ static void usb_msd_transfer_data(SCSIRequest *req, uint32_t len)
>       MSDState *s = DO_UPCAST(MSDState, dev.qdev, req->bus->qbus.parent);
>       USBPacket *p = s->packet;
>
> -    if (req->tag != s->tag) {
> -        fprintf(stderr, "usb-msd: Unexpected SCSI Tag 0x%x\n", req->tag);
> +    if (req->hba_private != s) {
> +        fprintf(stderr, "usb-msd: Unexpected SCSI command 0x%p\n", req);
>       }

Same here, just pass NULL and remove these ifs.

Otherwise looks like a very good idea.

Paolo