From: Eric Blake <eblake@redhat.com>
To: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>,
qemu-devel@nongnu.org, qemu-block@nongnu.org
Cc: mreitz@redhat.com, kwolf@redhat.com, fam@euphon.net, stefanha@redhat.com
Subject: Re: [Qemu-devel] [PATCH v2 1/2] block: enhance QEMUIOVector structure
Date: Wed, 6 Feb 2019 11:25:48 -0600 [thread overview]
Message-ID: <cb3e35a3-fd92-0b36-52c1-44312c5c11c8@redhat.com> (raw)
In-Reply-To: <20190206165342.219192-2-vsementsov@virtuozzo.com>
[-- Attachment #1: Type: text/plain, Size: 4151 bytes --]
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 <vsementsov@virtuozzo.com>
> ---
> 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
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
next prev parent reply other threads:[~2019-02-06 17:26 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-02-06 16:53 [Qemu-devel] [PATCH v2 0/2] block: local qiov helper: part I Vladimir Sementsov-Ogievskiy
2019-02-06 16:53 ` [Qemu-devel] [PATCH v2 1/2] block: enhance QEMUIOVector structure Vladimir Sementsov-Ogievskiy
2019-02-06 17:25 ` Eric Blake [this message]
2019-02-06 17:50 ` Vladimir Sementsov-Ogievskiy
2019-02-06 16:53 ` [Qemu-devel] [PATCH v2 2/2] block/io: use qemu_iovec_init_buf Vladimir Sementsov-Ogievskiy
2019-02-06 17:32 ` Eric Blake
2019-02-06 18:09 ` Vladimir Sementsov-Ogievskiy
2019-02-06 18:14 ` Eric Blake
2019-02-06 18:26 ` Vladimir Sementsov-Ogievskiy
2019-02-06 18:33 ` Eric Blake
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=cb3e35a3-fd92-0b36-52c1-44312c5c11c8@redhat.com \
--to=eblake@redhat.com \
--cc=fam@euphon.net \
--cc=kwolf@redhat.com \
--cc=mreitz@redhat.com \
--cc=qemu-block@nongnu.org \
--cc=qemu-devel@nongnu.org \
--cc=stefanha@redhat.com \
--cc=vsementsov@virtuozzo.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).