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

[Eric Blake]

On 2/6/19 10:53 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 | 47 ++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 45 insertions(+), 2 deletions(-)
> 
> diff --git a/include/qemu/iov.h b/include/qemu/iov.h
> index 5f433c7768..3753b63558 100644
> --- a/include/qemu/iov.h
> +++ b/include/qemu/iov.h
> @@ -133,10 +133,53 @@ size_t iov_discard_back(struct iovec *iov, unsigned int 
> *iov_cnt,
>  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

s/ @size/, @size/

> +     * @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.

Only one static assertion below (s/assertions/assertion/), but even that
one could perhaps be dropped and this wording changed to "which is
guaranteed by the layout below"; or you could restore the assertion in
the earlier patch that sizeof(size) and sizeof(struct iovec.iov_len) are
equal) to make the plural correct.

> +     *
> +     * @nalloc is valid always and is -1 both for embedded and external

s/valid always/always valid/

> +     * cases. It included into union only to make appropriate padding for
> +     * @size field avoiding creation of 0-length array in the worst case.

It is included in the union only to ensure the padding prior to the
@size field will not result in a 0-length array.

> +     */
> +    union {
> +        struct {
> +            int nalloc;
> +            struct iovec local_iov;
> +        };
> +        struct {
> +            char __pad[sizeof(int) + offsetof(struct iovec, iov_len)];
> +            size_t size;
> +        };
> +    };
>  } QEMUIOVector;
>  
> +QEMU_BUILD_BUG_ON(offsetof(QEMUIOVector, size) !=
> +                  offsetof(QEMUIOVector, local_iov.iov_len));

I'm not sure this assertion adds any value; I don't see any leeway on
how a compiler could lay out the struct based on the declaration of the
padding.  However, I'm not opposed to keeping it in the patch if someone
else finds it useful.

> +
> +#define QEMU_IOVEC_INIT_BUF(self, buf, len)              \
> +{                                                        \
> +    .iov = &(self).local_iov,                            \
> +    .niov = 1,                                           \
> +    .nalloc = -1,                                        \
> +    .local_iov = {                                       \
> +        .iov_base = (void *)(buf), /* cast away const */ \
> +        .iov_len = (len),                                \
> +    },                                                   \
> +}
> +
> +static inline void qemu_iovec_init_buf(QEMUIOVector *qiov,
> +                                       void *buf, size_t len)

Should this be 'const void *buf', to make it easier to initialize a qiov
used for writes from an incoming const pointer?  That, and having const
here would make the 'cast away const' comment above all the more obvious
(I know it is necessary based on code in patch 2, but having it be
worthwhile in patch 1 makes it all the more obvious as a standalone patch).

> +{
> +    *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);
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

signature.asc
Description: OpenPGP digital signature