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

[Eric Blake]

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.

> 
> 
> 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.

> 
> 
>> 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.

>> 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.


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

signature.asc
Description: OpenPGP digital signature