Re: [Qemu-devel] [PATCH 1/2] block: enhance QEMUIOVector structure

[Vladimir Sementsov-Ogievskiy]

30.01.2019 17:55, Eric Blake wrote:
> On 1/30/19 3:14 AM, Vladimir Sementsov-Ogievskiy wrote:
>> 30.01.2019 0:35, Eric Blake wrote:
>>> On 1/29/19 8:37 AM, Vladimir Sementsov-Ogievskiy wrote:
>>>> Add a possibility of embedded iovec, for cases when we need only one
>>>> local iov.
>>>>
>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
>>>> ---
>>>>    include/qemu/iov.h | 41 ++++++++++++++++++++++++++++++++++++++++-
>>>>    1 file changed, 40 insertions(+), 1 deletion(-)
> 
>>> POSIX does not guarantee that iov_base occurs prior to iov_len in struct
>>> iovec; your code is therefore rather fragile and would fail to compile
>>> if we encounter a libc where iov_len is positioned differently than in
>>> glibc, tripping the first assertion.  For an offset other than 0, you
>>> could declare:
>>>     char [offsetof(struct iovec, iov_len)] __pad;
>>> to be more robust; but since C doesn't like 0-length array declarations,
>>> I'm not sure how you would cater to a libc where iov_len is at offset 0,
>>
>> Hmm, tested a bit, and the only complain I can get is a warning about 0-sized
>> array when option -pedantic is set. So, is it a real problem?
> 
> Even if we require the use of gcc extensions elsewhere, it doesn't hurt
> to avoid them where it is easy.
> 
>>
>> And we already have a lot of zero-sized arrays in Qemu, just look at
>>    git grep '\w\+\s\+\w\+\[0\];' | grep -v return
> 
> And how many of those are the last entry in a struct?  A 0-size array at
> the end is a common idiom for a flex-sized struct (without relying on
> C99 VLA); a 0-size array in the middle of a struct is unusual.

Reasonable, so ...

> 
>>
>>
>> hm, or we can do like this:
>>
>> typedef struct QEMUIOVector {
>>       struct iovec *iov;
>>       int niov;
>>       union {
>>           struct {
>>               int nalloc;
>>               struct iovec local_iov;
>>           };
>>           struct {
>>               char __pad[sizeof(int) + offsetof(struct iovec, iov_len)];
>>               size_t size;
>>           };
>>       };
>> } QEMUIOVector;
> 
> Yes, that's a reasonable way to do it.

... will try this in v2, if no more comments.

> 
>>
>>
>>> short of a configure probe and #ifdeffery, which feels like a bit much
>>> to manage (really, the point of the assert is that we don't have to
>>> worry about an unusual libc having a different offset UNLESS the build
>>> fails, so no need to address a problem we aren't hitting yet).
>>
>> However, char __pad[offsetof(struct iovec, iov_len)]; seems useful and even
>> more self-documented, so I'll use it in v2, if nobody minds.
>>
> 
> We'll see how it looks, then.
> 
> 
>>>> +        .iov_base = (void *)(buf),          \
>>>
>>> Why is the cast necessary?  Are we casting away const?
> 
> Answered in 2/2 - yes. But a comment about /* cast away const */ is useful.
> 
>>>   Since C already
>>> allows assignment of any other (non-const) pointer to void* without a
>>> cast, it looks superfluous.
>>>
>>>> +        .iov_len = len                      \
>>>
>>> Missing () around len.
>>
>> For style? What the thing len should be to break it without ()?
> 
> Although we are unlikely to hit this given our coding conventions,
> remember that '=' has higher precedence than ',', for this contorted
> example (and yes, it's contorted because you can't pass the comma
> operator through to a macro without either supplying your own (), which
> defeats the demonstration, or relying on a helper macro such as
> raw_pair() here):
> 
> $ cat foo.c
> struct s {
>      int i;
> };
> #define raw_pair(a, b) a, b
> #define A(arg) { .i = (arg) }
> #define B(arg) { .i = arg }
> 
> int
> main(int argc, char **argv)
> {
>      struct s s1 = A(raw_pair(1, 2));
>      struct s s2 = B(raw_pair(4, 8));
>      return s1.i + s2.i;
> }
> $ gcc -o foo foo.c
> foo.c: In function ‘main’:
> foo.c:12:33: warning: excess elements in struct initializer
>       struct s s2 = B(raw_pair(4, 8));
>                                   ^
> foo.c:6:23: note: in definition of macro ‘B’
>   #define B(arg) { .i = arg }
>                         ^~~
> foo.c:12:21: note: in expansion of macro ‘raw_pair’
>       struct s s2 = B(raw_pair(4, 8));
>                       ^~~~~~~~
> foo.c:12:33: note: (near initialization for ‘s2’)
>       struct s s2 = B(raw_pair(4, 8));
>                                   ^
> foo.c:6:23: note: in definition of macro ‘B’
>   #define B(arg) { .i = arg }
>                         ^~~
> foo.c:12:21: note: in expansion of macro ‘raw_pair’
>       struct s s2 = B(raw_pair(4, 8));
>                       ^~~~~~~~
> $ ./foo; echo $?
> 6
> 
> which says that for A() using the (), there were no warnings and the
> second value was assigned (-Wall changes that, complaining about unused
> value of the left side of the comma operator - but that's okay); but for
> B() without (), there was a diagnostic even without -Wall about too many
> initializers, and the first value was assigned.

Oh, that's cool!

> 
>>> Why both a macro and a function that uses the macro?
> 
> Also answered in 2/2 - the macro is for variable declarations; the
> function is for runtime code such as for-loops that start with an
> uninitialized declaration and have to reinitialize things. They also
> differ in that one takes a struct, the other takes a pointer to a struct.
> 
> 


-- 
Best regards,
Vladimir