Re: [Qemu-devel] [PATCH 00/16] introduce OptsVisitor, rebase -net/-netdev parsing

[Michael Roth]

On Wed, Jun 06, 2012 at 05:30:03PM +0200, Laszlo Ersek wrote:
> On 06/06/12 17:16, Michael Roth wrote:
> > On Wed, Jun 06, 2012 at 04:10:44PM +0200, Paolo Bonzini wrote:
> 
> >> The uintXX visitors do not fail if you pass a negative value.  I'm fine
> >> with including the patch with the small bug and fixing it as a
> >> follow-up, there's plenty of time before 1.2.
> > 
> > How would we implement such a check?
> > 
> > In the case of uint64_t, the field we're visiting is passed in as a
> > uint64_t*, so -1 is indistinguishable from the unsigned interpretation
> > of the field, which is within the valid range. 
> > 
> > For uintXX_t where XX < 64, a negative value would exceed the UINTXX_MAX
> > check, so those cases are already handled.
> > 
> > Or am I missing something?
> 
> I found three instances of the patch on the list:
> 
>   http://lists.nongnu.org/archive/html/qemu-devel/2012-04/msg00333.html
>   http://lists.nongnu.org/archive/html/qemu-devel/2012-04/msg01292.html
>   http://lists.nongnu.org/archive/html/qemu-devel/2012-04/msg04068.html
> 
> looking at the third one, all of
> 
> - visit_type_uint8()
> - visit_type_uint16()
> - visit_type_uint32()
> - visit_type_uint64()
> 
> seem to define "value" as an int64_t. Thus when we fall back to
> (*v->type_int)(), the comparison is still done against an int64_t. Since
> "int" is equivalent to "int32_t" on the platforms I can think of, and
> "int64_t" to "long long", the comparisons are evaluated as follows:
> 
>   value > UINT8_MAX
>   value > UINT16_MAX
> 
> First the right hand sides are promoted to "int" (with unchanged value),
> and then "int" is converted to "long long" (both signed, different
> conversion rank).
> 
>   value > UINT32_MAX
> 
> The right hand side is directly converted to "long long" (signed vs.
> unsigned, signed has greater rank and can represent all values of the
> lower-rank unsigned type).
> 
> I propose
> 
>   value < 0 || value > UINT8_MAX
>   value < 0 || value > UINT16_MAX
>   value < 0 || value > UINT32_MAX
>   value < 0

Thanks, this does indeed seem warranted. I'm fine either of the options
Andreas' suggested (a fix-up to squash into into qom-next version or a
seperate patch to apply to qom-next). Only thing I'd like to avoid is
having a modified/squashed patch floating around.

> 
> Laszlo
>