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

[Isaku Yamahata <yamahata@valinux.co.jp>]

On Thu, Oct 21, 2010 at 10:07:01AM +0200, Michael S. Tsirkin wrote:
> 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.

ACIEAERLog and PCIEAERErr themselves are the implementation specific
internal states which is not visible to guest.
So there is no point of arguing that consumer/producer are 
specific to implementation.


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

Will do.
-- 
yamahata