From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([140.186.70.92]:43826)
	by lists.gnu.org with esmtp (Exim 4.71) (envelope-from <hch@lst.de>)
	id 1QNS5Y-0002d5-Qx
	for qemu-devel@nongnu.org; Fri, 20 May 2011 11:58:09 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <hch@lst.de>) id 1QNS5X-0002RD-TO
	for qemu-devel@nongnu.org; Fri, 20 May 2011 11:58:08 -0400
Received: from verein.lst.de ([213.95.11.211]:50185 helo=newverein.lst.de)
	by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from <hch@lst.de>)
	id 1QNS5X-0002R8-NI
	for qemu-devel@nongnu.org; Fri, 20 May 2011 11:58:07 -0400
Date: Fri, 20 May 2011 17:58:07 +0200
From: Christoph Hellwig <hch@lst.de>
Message-ID: <20110520155807.GE4466@lst.de>
References: <1305630067-2119-1-git-send-email-pbonzini@redhat.com>
	<1305630067-2119-6-git-send-email-pbonzini@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1305630067-2119-6-git-send-email-pbonzini@redhat.com>
Subject: Re: [Qemu-devel] [PATCH v3 05/21] scsi: reference-count requests
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Paolo Bonzini <pbonzini@redhat.com>
Cc: qemu-devel@nongnu.org

> --- 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.