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