Re: [Xen-devel] [PATCH 1 of 5] xentrace: fix t_info_pages calculation for the default case

[George Dunlap <george.dunlap@xxxxxxxxxxxxx>]

Actually, I think the source of this bug was the re-definition of what
the variable meant in the middle.  I think the Right Thing, to avoid
bugs like this in the future, would be to assign separate variables,
thus:

t_info_bytes= [calculation]
t_info_pages = t_info_bytes / PAGE_SIZE
if(t_info_bytes % PAGE_SIZE)
 t_info_pages++;

The compiler should optimize away unused stuff, and even if not, a byte
here is small cost to make the logic more readable.

 -George

On Wed, 2011-03-23 at 12:46 +0000, Olaf Hering wrote:
> On Wed, Mar 23, Keir Fraser wrote:
> 
> > On 23/03/2011 11:20, "Olaf Hering" <olaf@xxxxxxxxx> wrote:
> > 
> > >>>>      t_info_pages /= PAGE_SIZE;
> > >>>> -    if ( t_info_pages % PAGE_SIZE )
> > >>>> +    if ( t_info_pages % PAGE_SIZE || t_info_pages == 0 )
> > >>> 
> > >>> While certainly not having a significant effect, to the unsuspecting
> > >>> reader this looks like a bug - is it really meant to be a remainder
> > >>> operation on the *result* of a division (rather than on the original
> > >>> dividend)? Couldn't you just (ab)use PFN_UP() here?
> > >> 
> > >> By which you mean to replace the division and subsequent if statement 
> > >> with
> > >> t_info_pages = PFN_UP(t_info_pages).
> > > 
> > > I did not know about PFN_UP() until now, using it would work as well.
> > 
> > As opposed to the existing code (even including your latest patch) which
> > doesn't work properly. You need to respin at least your patch 1/5.
> 
> I will send the bugfix now.
> 
> Olaf



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel