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

[Vladimir Sementsov-Ogievskiy]

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 <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.

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