From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from [140.186.70.92] (port=43633 helo=eggs.gnu.org)
	by lists.gnu.org with esmtp (Exim 4.43) id 1P8qHI-0002L8-48
	for qemu-devel@nongnu.org; Thu, 21 Oct 2010 04:13:37 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <mst@redhat.com>) id 1P8qHH-0002rD-2g
	for qemu-devel@nongnu.org; Thu, 21 Oct 2010 04:13:36 -0400
Received: from mx1.redhat.com ([209.132.183.28]:9729)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <mst@redhat.com>) id 1P8qHG-0002r8-S9
	for qemu-devel@nongnu.org; Thu, 21 Oct 2010 04:13:35 -0400
Date: Thu, 21 Oct 2010 10:07:01 +0200
From: "Michael S. Tsirkin" <mst@redhat.com>
Message-ID: <20101021080701.GB27985@redhat.com>
References: <cover.1287562197.git.yamahata@valinux.co.jp>
	<b3216e946c77598d47c76c9b8cdb6d46dbeeea73.1287562197.git.yamahata@valinux.co.jp>
	<20101020095616.GD10783@redhat.com>
	<20101021051524.GB31309@valinux.co.jp>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20101021051524.GB31309@valinux.co.jp>
Subject: [Qemu-devel] Re: [PATCH v6 08/12] pcie/aer: helper functions for
 pcie aer capability
List-Id: qemu-devel.nongnu.org
List-Unsubscribe: <http://lists.nongnu.org/mailman/listinfo/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: <http://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Isaku Yamahata <yamahata@valinux.co.jp>
Cc: skandasa@cisco.com, adnan@khaleel.us, etmartin@cisco.com, qemu-devel@nongnu.org, wexu2@cisco.com

On Thu, Oct 21, 2010 at 02:15:24PM +0900, Isaku Yamahata wrote:
> Thank you for detailed review.
> 
> On Wed, Oct 20, 2010 at 11:56:16AM +0200, Michael S. Tsirkin wrote:
> > > +static uint32_t aer_log_del(PCIEAERLog *aer_log)
> > > +{
> > > +    uint32_t i = aer_log->consumer;
> > > +    aer_log->consumer = aer_log_next(aer_log->consumer, aer_log->log_max);
> > > +    return i;
> > > +}
> > 
> > 
> > Please just use 'int' or 'unsigned' instead of uint32_t if you simply
> > want to say 'a number'.  Using specific length makes it impossible to
> > say where you *really* want a value.
> 
> PCIEAERLog is saved/loaded. So explicit sized number is chosen.

I didn't notice. But consumer/producer are not guest visible, are they?
If yes I think it's a mistake to save/load them, tying us to
a specific implementation: just calculate and save the # of
valid entries in the log.

Also I put the comment here but it is a general one: pls go over the
code, and just take a look at what types you use all over and think
whether size really matters. In most places it does not, it just must be
big enough, so use int or unsigned there.  It will be much harder for
others to do so later.

> -- 
> yamahata