Add a possibility of embedded iovec, for cases when we need only one
local iov.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
include/qemu/iov.h | 41 ++++++++++++++++++++++++++++++++++++++++-
1 file changed, 40 insertions(+), 1 deletion(-)
diff --git a/include/qemu/iov.h b/include/qemu/iov.h
index 5f433c7768..f739cff97d 100644
--- a/include/qemu/iov.h
+++ b/include/qemu/iov.h
@@ -134,9 +134,48 @@ typedef struct QEMUIOVector {
struct iovec *iov;
int niov;
int nalloc;
- size_t size;
+
+ /*
+ * For external @iov (qemu_iovec_init_external()) or allocated @iov
+ * (qemu_iovec_init()) @size is the cumulative size of iovecs and
+ * @local_iov is invalid and unused.
+ *
+ * For embedded @iov (QEMU_IOVEC_INIT_BUF() or qemu_iovec_init_buf()),
+ * @iov is equal to &@local_iov, and @size is valid, as it has same
+ * offset and type as @local_iov.iov_len, which is guaranteed by
+ * static assertions below.
+ */
+ union {
+ struct {
+ void *__unused_iov_base;
+ size_t size;
+ };
+ struct iovec local_iov;
+ };
} QEMUIOVector;
+G_STATIC_ASSERT(offsetof(QEMUIOVector, size) ==
+ offsetof(QEMUIOVector, local_iov.iov_len));
+G_STATIC_ASSERT(sizeof(((QEMUIOVector *)NULL)->size) ==
+ sizeof(((QEMUIOVector *)NULL)->local_iov.iov_len));
+
+#define QEMU_IOVEC_INIT_BUF(self, buf, len) \
+{ \
+ .iov = &(self).local_iov, \
+ .niov = 1, \
+ .nalloc = -1, \
+ .local_iov = { \
+ .iov_base = (void *)(buf), \
+ .iov_len = len \
+ } \
+}
+
+static inline void qemu_iovec_init_buf(QEMUIOVector *qiov,
+ void *buf, size_t len)
+{
+ *qiov = (QEMUIOVector) QEMU_IOVEC_INIT_BUF(*qiov, buf, len);
+}
+
void qemu_iovec_init(QEMUIOVector *qiov, int alloc_hint);
void qemu_iovec_init_external(QEMUIOVector *qiov, struct iovec *iov, int niov);
void qemu_iovec_add(QEMUIOVector *qiov, void *base, size_t len);
--
2.18.0
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 <vsementsov@virtuozzo.com>
> ---
> include/qemu/iov.h | 41 ++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 40 insertions(+), 1 deletion(-)
>
>
> +G_STATIC_ASSERT(offsetof(QEMUIOVector, size) ==
> + offsetof(QEMUIOVector, local_iov.iov_len));
> +G_STATIC_ASSERT(sizeof(((QEMUIOVector *)NULL)->size) ==
> + sizeof(((QEMUIOVector *)NULL)->local_iov.iov_len));
We don't use G_STATIC_ASSERT() elsewhere (slirp excepted, as that is
trying to split off to a separate project); so these should be:
QEMU_BUILD_BUG_ON(offsetof(QEMUIOVector, size) !=
offsetof(QEMUIOVector, local_iov.iov_len));
and so on.
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,
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).
The second assertion (about sizeof being identical) is redundant, since
POSIX requires size_t iov_len, and we used size_t size (while a libc
might reorder the fields of struct iovec, they shouldn't be using the
wrong types); but perhaps you could use:
typeof(struct iovec.iov_len) size;
in the declaration to avoid even that possibility (I'm not sure it is
worth it, though).
> +
> +#define QEMU_IOVEC_INIT_BUF(self, buf, len) \
> +{ \
> + .iov = &(self).local_iov, \
> + .niov = 1, \
> + .nalloc = -1, \
> + .local_iov = { \
> + .iov_base = (void *)(buf), \
Why is the cast necessary? Are we casting away const? 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. I might also have used a trailing comma (I find
it easier to always use trailing comma; while we're unlikely to add more
members here, it does make for less churn in other places where a struct
may grow).
> + } \
> +}
> +
> +static inline void qemu_iovec_init_buf(QEMUIOVector *qiov,
> + void *buf, size_t len)
> +{
> + *qiov = (QEMUIOVector) QEMU_IOVEC_INIT_BUF(*qiov, buf, len);
> +}
Why both a macro and a function that uses the macro? Do you expect any
other code to actually use the macro, or will the function always be
sufficient, in which case you could inline the initializer without the
use of a macro.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
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 <vsementsov@virtuozzo.com>
>> ---
>> include/qemu/iov.h | 41 ++++++++++++++++++++++++++++++++++++++++-
>> 1 file changed, 40 insertions(+), 1 deletion(-)
>>
>
>>
>> +G_STATIC_ASSERT(offsetof(QEMUIOVector, size) ==
>> + offsetof(QEMUIOVector, local_iov.iov_len));
>> +G_STATIC_ASSERT(sizeof(((QEMUIOVector *)NULL)->size) ==
>> + sizeof(((QEMUIOVector *)NULL)->local_iov.iov_len));
>
> We don't use G_STATIC_ASSERT() elsewhere (slirp excepted, as that is
> trying to split off to a separate project); so these should be:
>
> QEMU_BUILD_BUG_ON(offsetof(QEMUIOVector, size) !=
> offsetof(QEMUIOVector, local_iov.iov_len));
>
> and so on.
>
> 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?
And we already have a lot of zero-sized arrays in Qemu, just look at
git grep '\w\+\s\+\w\+\[0\];' | grep -v return
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;
> 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.
>
> The second assertion (about sizeof being identical) is redundant
Ok.
> since
> POSIX requires size_t iov_len, and we used size_t size (while a libc
> might reorder the fields of struct iovec, they shouldn't be using the
> wrong types); but perhaps you could use:
> typeof(struct iovec.iov_len) size;
> in the declaration to avoid even that possibility (I'm not sure it is
> worth it, though).
>
>> +
>> +#define QEMU_IOVEC_INIT_BUF(self, buf, len) \
>> +{ \
>> + .iov = &(self).local_iov, \
>> + .niov = 1, \
>> + .nalloc = -1, \
>> + .local_iov = { \
>> + .iov_base = (void *)(buf), \
>
> Why is the cast necessary? Are we casting away const? 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 ()?
> I might also have used a trailing comma (I find
> it easier to always use trailing comma; while we're unlikely to add more
> members here, it does make for less churn in other places where a struct
> may grow).
>
>> + } \
>> +}
>> +
>> +static inline void qemu_iovec_init_buf(QEMUIOVector *qiov,
>> + void *buf, size_t len)
>> +{
>> + *qiov = (QEMUIOVector) QEMU_IOVEC_INIT_BUF(*qiov, buf, len);
>> +}
>
> Why both a macro and a function that uses the macro? Do you expect any
> other code to actually use the macro, or will the function always be
> sufficient, in which case you could inline the initializer without the
> use of a macro.
>
--
Best regards,
Vladimir
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 <vsementsov@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
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 <vsementsov@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
© 2016 - 2025 Red Hat, Inc.