06.02.2019 20:25, Eric Blake wrote:
> 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 <vsement...@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.
Assertion is a bit simpler to understand than structure layout. And it exactly
asserts what the comment say about @size...
>
>> +
>> +#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).
I think, yes. So, we'll be able to use both macro and function for const buffers
in the same way
>
>> +{
>> + *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);
>>
>
--
Best regards,
Vladimir
