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 <vsement...@virtuozzo.com>
>>>> ---
>>>> 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
