Re: [Qemu-devel] [PATCH 08/10] migration: s/uint64_t/int64_t the definitions of it_age

[Eric Blake]

On 03/11/2014 06:53 AM, address@hidden wrote:
> From: ChenLiang <address@hidden>
> 

The subject line says what, but there is no WHY.  The commit message
should explain why you want a signed type.

> Signed-off-by: ChenLiang <address@hidden>
> Signed-off-by: Gonglei <address@hidden>
> ---
>  arch_init.c                    | 4 ++--
>  include/migration/page_cache.h | 4 ++--
>  page_cache.c                   | 6 +++---
>  3 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/arch_init.c b/arch_init.c
> index 461a10a..1c1488a 100644
> --- a/arch_init.c
> +++ b/arch_init.c
> @@ -109,7 +109,7 @@ const uint32_t arch_type = QEMU_ARCH;
>  static bool mig_throttle_on;
>  static void check_guest_throttling(void);
>  
> -static uint64_t bitmap_sync_cnt;
> +static int64_t bitmap_sync_cnt;

Wait.  Patch 2/10 introduced this variable.  Why not introduce it with
the correct type to begin with?  Please reorder your series so that you
aren't churning on new code.


> -static inline uint64_t get_bitmap_sync_cnt(void)
> +static inline int64_t get_bitmap_sync_cnt(void)
>  {
>      return bitmap_sync_cnt;

Not to mention that my argument in patch 2 still holds - unless you are
going to export this function, it is pointless compared to just using
the variable directly.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature