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