Re: [PATCH v1 COLO Pre 03/12] tools/libxc: export xc_bitops.h

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 02/06/15 10:26, Yang Hongyang wrote:
> When we are under COLO, we will send dirty page bitmap info from
> secondary to primary at every checkpoint. So we need to get/test
> the dirty page bitmap. We just expose xc_bitops.h for libxl use.
>
> NOTE:
>   Need to make clean and rerun configure to get it compiled.
>
> Signed-off-by: Yang Hongyang <yanghy@cn.fujitsu.com>

I like this change, but lets take the opportunity to fix some of the
issues in it.

> ---
>  tools/libxc/include/xc_bitops.h | 76 +++++++++++++++++++++++++++++++++++++++++
>  tools/libxc/xc_bitops.h         | 76 -----------------------------------------
>  2 files changed, 76 insertions(+), 76 deletions(-)
>  create mode 100644 tools/libxc/include/xc_bitops.h
>  delete mode 100644 tools/libxc/xc_bitops.h
>
> diff --git a/tools/libxc/include/xc_bitops.h b/tools/libxc/include/xc_bitops.h
> new file mode 100644
> index 0000000..cd749f4
> --- /dev/null
> +++ b/tools/libxc/include/xc_bitops.h
> @@ -0,0 +1,76 @@
> +#ifndef XC_BITOPS_H
> +#define XC_BITOPS_H 1

No need for a 1 here

> +
> +/* bitmap operations for single threaded access */
> +
> +#include <stdlib.h>
> +#include <string.h>
> +
> +#define BITS_PER_LONG (sizeof(unsigned long) * 8)

All defines like this need XC_ prefixes, and CHAR_BIT should be used in
preference to 8.

> +#define ORDER_LONG (sizeof(unsigned long) == 4 ? 5 : 6)

This name is misleading, as it is in terms of bits not bytes. 
XC_BITMAP_SHIFT perhaps?

> +
> +#define BITMAP_ENTRY(_nr,_bmap) ((_bmap))[(_nr)/BITS_PER_LONG]
> +#define BITMAP_SHIFT(_nr) ((_nr) % BITS_PER_LONG)

I would recommend dropping these and open coding the few cases below. 
It would be far more clear.

> +
> +/* calculate required space for number of longs needed to hold nr_bits */
> +static inline int bitmap_size(int nr_bits)

"int" has been inappropriate everywhere in this file.  unsigned long
please (or settle on unsigned int everywhere)

> +{
> +    int nr_long, nr_bytes;
> +    nr_long = (nr_bits + BITS_PER_LONG - 1) >> ORDER_LONG;

This calculation can overflow.

(nr_bits >> ORDER_LONG) + !!(nr_bits % BITS_PER_LONG)

> +    nr_bytes = nr_long * sizeof(unsigned long);
> +    return nr_bytes;
> +}
> +
> +static inline unsigned long *bitmap_alloc(int nr_bits)
> +{
> +    return calloc(1, bitmap_size(nr_bits));
> +}
> +
> +static inline void bitmap_set(unsigned long *addr, int nr_bits)
> +{
> +    memset(addr, 0xff, bitmap_size(nr_bits));
> +}
> +
> +static inline void bitmap_clear(unsigned long *addr, int nr_bits)
> +{
> +    memset(addr, 0, bitmap_size(nr_bits));
> +}
> +
> +static inline int test_bit(int nr, unsigned long *addr)

const *addr, as this is a read-only operation.

> +{
> +    return (BITMAP_ENTRY(nr, addr) >> BITMAP_SHIFT(nr)) & 1;
> +}
> +
> +static inline void clear_bit(int nr, unsigned long *addr)
> +{
> +    BITMAP_ENTRY(nr, addr) &= ~(1UL << BITMAP_SHIFT(nr));
> +}
> +
> +static inline void set_bit(int nr, unsigned long *addr)
> +{
> +    BITMAP_ENTRY(nr, addr) |= (1UL << BITMAP_SHIFT(nr));
> +}

It would be nice to be consistent on whether the bitmap pointer or the
bit is the first parameter.  Perhaps a second cleanup patch which makes
this consistent and adjusts all current callers.

~Andrew

> +
> +static inline int test_and_clear_bit(int nr, unsigned long *addr)
> +{
> +    int oldbit = test_bit(nr, addr);
> +    clear_bit(nr, addr);
> +    return oldbit;
> +}
> +
> +static inline int test_and_set_bit(int nr, unsigned long *addr)
> +{
> +    int oldbit = test_bit(nr, addr);
> +    set_bit(nr, addr);
> +    return oldbit;
> +}
> +
> +static inline void bitmap_or(unsigned long *dst, const unsigned long *other,
> +                             int nr_bits)
> +{
> +    int i, nr_longs = (bitmap_size(nr_bits) / sizeof(unsigned long));
> +    for ( i = 0; i < nr_longs; ++i )
> +        dst[i] |= other[i];
> +}
> +
> +#endif  /* XC_BITOPS_H */
> diff --git a/tools/libxc/xc_bitops.h b/tools/libxc/xc_bitops.h
> deleted file mode 100644
> index cd749f4..0000000
> --- a/tools/libxc/xc_bitops.h
> +++ /dev/null
> @@ -1,76 +0,0 @@
> -#ifndef XC_BITOPS_H
> -#define XC_BITOPS_H 1
> -
> -/* bitmap operations for single threaded access */
> -
> -#include <stdlib.h>
> -#include <string.h>
> -
> -#define BITS_PER_LONG (sizeof(unsigned long) * 8)
> -#define ORDER_LONG (sizeof(unsigned long) == 4 ? 5 : 6)
> -
> -#define BITMAP_ENTRY(_nr,_bmap) ((_bmap))[(_nr)/BITS_PER_LONG]
> -#define BITMAP_SHIFT(_nr) ((_nr) % BITS_PER_LONG)
> -
> -/* calculate required space for number of longs needed to hold nr_bits */
> -static inline int bitmap_size(int nr_bits)
> -{
> -    int nr_long, nr_bytes;
> -    nr_long = (nr_bits + BITS_PER_LONG - 1) >> ORDER_LONG;
> -    nr_bytes = nr_long * sizeof(unsigned long);
> -    return nr_bytes;
> -}
> -
> -static inline unsigned long *bitmap_alloc(int nr_bits)
> -{
> -    return calloc(1, bitmap_size(nr_bits));
> -}
> -
> -static inline void bitmap_set(unsigned long *addr, int nr_bits)
> -{
> -    memset(addr, 0xff, bitmap_size(nr_bits));
> -}
> -
> -static inline void bitmap_clear(unsigned long *addr, int nr_bits)
> -{
> -    memset(addr, 0, bitmap_size(nr_bits));
> -}
> -
> -static inline int test_bit(int nr, unsigned long *addr)
> -{
> -    return (BITMAP_ENTRY(nr, addr) >> BITMAP_SHIFT(nr)) & 1;
> -}
> -
> -static inline void clear_bit(int nr, unsigned long *addr)
> -{
> -    BITMAP_ENTRY(nr, addr) &= ~(1UL << BITMAP_SHIFT(nr));
> -}
> -
> -static inline void set_bit(int nr, unsigned long *addr)
> -{
> -    BITMAP_ENTRY(nr, addr) |= (1UL << BITMAP_SHIFT(nr));
> -}
> -
> -static inline int test_and_clear_bit(int nr, unsigned long *addr)
> -{
> -    int oldbit = test_bit(nr, addr);
> -    clear_bit(nr, addr);
> -    return oldbit;
> -}
> -
> -static inline int test_and_set_bit(int nr, unsigned long *addr)
> -{
> -    int oldbit = test_bit(nr, addr);
> -    set_bit(nr, addr);
> -    return oldbit;
> -}
> -
> -static inline void bitmap_or(unsigned long *dst, const unsigned long *other,
> -                             int nr_bits)
> -{
> -    int i, nr_longs = (bitmap_size(nr_bits) / sizeof(unsigned long));
> -    for ( i = 0; i < nr_longs; ++i )
> -        dst[i] |= other[i];
> -}
> -
> -#endif  /* XC_BITOPS_H */