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

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3499 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]