Re: [Xen-devel] [PATCH] xen: clamp bitmaps to correct number of bits

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 06.09.12 at 13:56, Ian Campbell <ian.campbell@xxxxxxxxxx> wrote:
> The principal of least surprise suggests that these bits ought not to
> be set and this is not a hot path so fix this at the hypervisor layer
> by clamping the bits in the returned bitmap to the correct limit.

Hmm, I see your point, but without looking at the description
above (i.e. if I just saw the code in its post-patch form) I'd be
immediately tempted to rip this all out again, the more that
the inverse functions don't do the same. So perhaps extending
the comment before the newly added function would be useful
to prevent such desires from coming up?

> @@ -38,6 +38,17 @@
>   * for the best explanations of this ordering.
>   */
>  
> +/* Ensure that the last byte is zero from nbits onwards. */
> +static void clamp_last_byte(uint8_t *bp, int nbits)
> +{
> +     int lastbyte = nbits/8;
> +     int remainder = nbits % 8;
> +     int mask = ((1U<<remainder)-1)&0xff;

While I realize the callers use plain int, I'd be very much in favor
of not continuing this bad practice (the more that you use 1U
already) - simply make the parameter and all locals (assuming
you really think they're useful; I would have omitted all but
"remainder", given they're being used just once) unsigned.

And once at it, please insert spaces consistently and drop the
bogus "&0xff".

Jan

> +
> +     if (remainder)
> +             bp[lastbyte] &= mask;
> +}
> +
>  int __bitmap_empty(const unsigned long *bitmap, int bits)
>  {
>       int k, lim = bits/BITS_PER_LONG;



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel