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

[Vladimir Sementsov-Ogievskiy]

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(-)
>>
> 
>>   
>> +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