[Qemu-devel] Re: [PATCH v6 08/12] pcie/aer: helper functions for pcie aer capability

["Michael S. Tsirkin" <mst@redhat.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