qemu-devel
[Top][All Lists]
Advanced

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

Re: [Qemu-devel] [PATCH for-2.11? v7 0/6] block: Don't compare strings i


From: Eric Blake
Subject: Re: [Qemu-devel] [PATCH for-2.11? v7 0/6] block: Don't compare strings in bdrv_reopen_prepare()
Date: Tue, 14 Nov 2017 13:31:41 -0600
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0

On 11/14/2017 12:01 PM, Max Reitz wrote:
> bdrv_reopen_prepare() assumes that all BDS options are strings, which is
> not necessarily correct. This series introduces a new qobject_is_equal()
> function which can be used to test whether any options have changed,
> independently of their type.
> 
> 
> v7:
> - Patch 6: Fix a clang warning:
>     tests/check-qobject.c:39:24: error: passing an object that undergoes
>                                  default argument promotion to
>                                  'va_start' has undefined behavior
>   TIL: You cannot use va_start(ap, foo) if @foo is a bool.  An int
>        works, however.

I knew that va_arg(ap, bool) was undefined behavior, but didn't realize
va_start(ap, bool_param) was also undefined.  But sure enough, reading
C99 7.15.1.4:

4 The parameter parmN is the identifier of the rightmost parameter in
the variable
parameter list in the function definition (the one just before the ,
...). If the parameter
parmN is declared with the register storage class, with a function or
array type, or
with a type that is not compatible with the type that results after
application of the default
argument promotions, the behavior is undefined.

>        Feel free to explain the long version to me, because I don't
>        think I have fully understood the issue, but it's something like
>        "Using bools for variadic arguments results in their promotion to
>        an int, but you have to use a type that is promoted to itself
>        (like int)."

So it must be that C99 is trying to cater to platforms that have special
ABI for passing multiple bool on the stack, by stating that the moment
argument promotion is in effect, va_list adjacent to a bool may cause
problems with that special packing in the ABI.  Weird.

> 
> 
> git-backport-diff against v6:
> 
> Key:
> [----] : patches are identical
> [####] : number of functional differences between upstream/downstream patch
> [down] : patch is downstream-only
> The flags [FC] indicate (F)unctional and (C)ontextual differences, 
> respectively
> 
> 001/6:[----] [--] 'qapi/qnull: Add own header'
> 002/6:[----] [--] 'qapi/qlist: Add qlist_append_null() macro'
> 003/6:[----] [--] 'qapi: Add qobject_is_equal()'
> 004/6:[----] [--] 'block: qobject_is_equal() in bdrv_reopen_prepare()'
> 005/6:[----] [--] 'iotests: Add test for non-string option reopening'
> 006/6:[0011] [FC] 'tests: Add check-qobject for equality tests'

I still think this is 2.11 material.  Once you fix the typo I point out
separately on 6/6, the changes since v6 look reasonable, so series:
Reviewed-by: Eric Blake <address@hidden>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature


reply via email to

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