Comments

With the next patch, a device may hold SCSIRequest for an indefinite
time. Split a rather big patch, and protect against access errors,
by reference counting them. One such access error in fact exists (it
is visible by testing the lsi driver with MALLOC_PERTURB_), and this
patch provides the infrastructure to fix it later.
There is some ugliness in scsi_send_command implementation due to
the need to unref the request when it fails. This will go away
with the next patches, which move the unref'ing to the devices.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
hw/scsi-bus.c | 22 ++++++++++++++++++++--
hw/scsi-disk.c | 20 +++++++++++++-------
hw/scsi-generic.c | 23 ++++++++++++++++-------
hw/scsi.h | 5 +++++
4 files changed, 54 insertions(+), 16 deletions(-)

> --- a/hw/scsi-bus.c> +++ b/hw/scsi-bus.c> @@ -136,6 +136,7 @@ SCSIRequest *scsi_req_alloc(size_t size, SCSIDevice *d, uint32_t tag, uint32_t l> SCSIRequest *req;> > req = qemu_mallocz(size);> + req->refcount = 2;> req->bus = scsi_bus_from_device(d);> req->dev = d;> req->tag = tag;
A little comment explaining why we start out with a reference count of 2
would be useful here. Might be worth making that a top of the function
block comment explaning the function a bit more while you're at it.
> void scsi_req_free(SCSIRequest *req)> {> - scsi_req_dequeue(req);> + assert(req->refcount == 0);> qemu_free(req);> }
Is there any reason to keep a free function? The pattern should be
that people just call the function to decrement the reference count,
and that frees the structure when it hits zero. In the current model
that would mean moving the freeing out of ->free_req into scsi_req_unref,
but that seems pretty sensible anyway.

On 05/20/2011 05:58 PM, Christoph Hellwig wrote:
>> > void scsi_req_free(SCSIRequest *req)>> > {>> > - scsi_req_dequeue(req);>> > + assert(req->refcount == 0);>> > qemu_free(req);>> > }> Is there any reason to keep a free function?
It's internal for SCSIDevice implementation, kind of a "base
implementation" for free_req
> The pattern should be> that people just call the function to decrement the reference count,> and that frees the structure when it hits zero. In the current model> that would mean moving the freeing out of ->free_req into scsi_req_unref,> but that seems pretty sensible anyway.
free_req is still needed, because it takes care of freeing the bounce
buffers or any other allocated data.
Paolo

On 05/20/2011 07:48 PM, Paolo Bonzini wrote:
>> Is there any reason to keep a free function?>> It's internal for SCSIDevice implementation, kind of a "base> implementation" for free_req>>> The pattern should be>> that people just call the function to decrement the reference count,>> and that frees the structure when it hits zero. In the current model>> that would mean moving the freeing out of ->free_req into scsi_req_unref,>> but that seems pretty sensible anyway.>> free_req is still needed, because it takes care of freeing the bounce> buffers or any other allocated data.
Nevermind, I see what you mean. Will do.
Paolo