qemu-block
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-block] [PATCH] osdep: Fix ROUND_UP(64-bit, 32-bit)


From: Laszlo Ersek
Subject: Re: [Qemu-block] [PATCH] osdep: Fix ROUND_UP(64-bit, 32-bit)
Date: Thu, 14 Sep 2017 10:44:10 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0

On 09/13/17 23:03, Eric Blake wrote:
> When using bit-wise operations that exploit the power-of-two
> nature of the second argument of ROUND_UP(), we still need to
> ensure that the mask is as wide as the first argument (done
> by using addition of 0 to force proper arithmetic promotion).
> Unpatched, ROUND_UP(2ULL*1024*1024*1024*1024, 512) produces 0,
> instead of the intended 2TiB.
> 
> Broken since its introduction in commit 292c8e50 (v1.5.0).
> 
> CC: address@hidden
> Signed-off-by: Eric Blake <address@hidden>
> 
> ---
> I did not audit to see how many potential users of ROUND_UP
> are actually passing in different sized types where the first
> argument can be larger than UINT32_MAX; I stumbled across the
> problem when iotests 190 started failing on a patch where I
> added a new use.  We can either be conservative and put this
> on qemu-stable no matter what, or go through the effort of an
> audit to see what might be broken (many callers in the block
> layer, but not just there).
> ---
>  include/qemu/osdep.h | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> index 6855b94bbf..7a3000efc5 100644
> --- a/include/qemu/osdep.h
> +++ b/include/qemu/osdep.h
> @@ -189,13 +189,13 @@ extern int daemon(int, int);
> 
>  /* Round number up to multiple. Requires that d be a power of 2 (see
>   * QEMU_ALIGN_UP for a safer but slower version on arbitrary
> - * numbers) */
> + * numbers); works even if d is a smaller type than n.  */
>  #ifndef ROUND_UP
> -#define ROUND_UP(n,d) (((n) + (d) - 1) & -(d))
> +#define ROUND_UP(n, d) (((n) + (d) - 1) & -((n) - (n) + (d)))
>  #endif

Another way to widen the mask as necessary would be:

  (((n) + (d) - 1) & -(0 ? (n) : (d)))

>From the C standard,

6.5.15 Conditional operator
5 If both the second and third operands have arithmetic type, the result
  type that would be determined by the usual arithmetic conversions,
  were they applied to those two operands, is the type of the result.
  [...]

Using the conditional operator is *perhaps* better than addition and
subtraction, because it would keep the previous trait of ROUND_UP that
"n" is evaluated only once:

6.5.15 Conditional operator
4 The first operand is evaluated; there is a sequence point after its
  evaluation. The second operand is evaluated only if the first compares
  unequal to 0; the third operand is evaluated only if the first
  compares equal to 0; the result is the value of the second or third
  operand (whichever is evaluated), converted to the type described
  below. [...]

Although, I admit that invoking ROUND_UP with any side effects in the
arguments would be crazy to begin with...

> 
>  #ifndef DIV_ROUND_UP
> -#define DIV_ROUND_UP(n,d) (((n) + (d) - 1) / (d))
> +#define DIV_ROUND_UP(n, d) (((n) + (d) - 1) / (d))
>  #endif

This looks like an independent whitespace fix; should it be in this patch?

Thanks
Laszlo



reply via email to

[Prev in Thread] Current Thread [Next in Thread]