Re: [Qemu-block] [RFC PATCH] block: local qiov helper

[Vladimir Sementsov-Ogievskiy]

29.01.2019 6:31, Stefan Hajnoczi wrote:
> On Fri, Jan 25, 2019 at 07:46:01PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> Hi all.
>>
>> What about such a simple helper for a very often patter around
>> qemu_iovec_init_external ?
> 
> Sounds good, qemu_iovec_init() has 55 references vs
> qemu_iovec_init_external() with 51.  It's worth making
> qemu_iovec_init_external() nicer to use.
> 
>> If we like it, I'll update other callers of qemu_iovec_init_external.
>>
>> Possible interface change would be
>> LOCAL_QIOV(lc, buf, len);
>> instead of
>> LocalQiov lc = LOCAL_QIOV(lc, buf, len);
>>
>> or, may be, someone has a better idea?
> 
> Bike-shedding territory, but I prefer LocalQiov lc = LOCAL_QIOV(lc, buf,
> len) because it reveals the type.  This makes the code easier to read
> than just LOCAL_QIOV(lc, buf, len) by itself - the reader is forced to
> look up the macro definition to figure out what magic happens.
> 
>> diff --git a/block/io.c b/block/io.c
>> index bd9d688f8b..c7d7b199c1 100644
>> --- a/block/io.c
>> +++ b/block/io.c
>> @@ -949,18 +949,13 @@ int bdrv_preadv(BdrvChild *child, int64_t offset, 
>> QEMUIOVector *qiov)
>>   
>>   int bdrv_pread(BdrvChild *child, int64_t offset, void *buf, int bytes)
>>   {
>> -    QEMUIOVector qiov;
>> -    struct iovec iov = {
>> -        .iov_base = (void *)buf,
>> -        .iov_len = bytes,
>> -    };
>> +    LocalQiov lq = LOCAL_QIOV(lq, buf, bytes);
>>   
>>       if (bytes < 0) {
>>           return -EINVAL;
>>       }
>>   
>> -    qemu_iovec_init_external(&qiov, &iov, 1);
>> -    return bdrv_preadv(child, offset, &qiov);
>> +    return bdrv_preadv(child, offset, &lq.qiov);
> 
> I think it's unfortunate that LocalQiov is necessary since the caller
> only needs the qiov.  Can we afford to embed the struct iovec into
> QEMUIOVector?
> 
> That way callers don't need a separate LocalQiov type:
> 
>    QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, buf, bytes);
>    ...
>    return bdrv_preadv(child, offset, &qiov);
> 


Hmm. In this case we definitely will have tiny extra memory usage, but we gain 
beautiful
readability.

So, like this:

diff --git a/include/qemu/iov.h b/include/qemu/iov.h
index 5f433c7768..53de1b38bb 100644
--- a/include/qemu/iov.h
+++ b/include/qemu/iov.h
@@ -134,9 +134,31 @@ typedef struct QEMUIOVector {
      struct iovec *iov;
      int niov;
      int nalloc;
-    size_t size;
+    union {
+        struct {
+            void *__unused_iov_base;
+            size_t size;
+        };
+        struct iovec local_iov;
+    };
  } QEMUIOVector;

+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));
+
+#define QEMU_IOVEC_INIT_BUF(self, buf, len) \
+{                                           \
+    .iov = &self.local_iov,                 \
+    .niov = 1,                              \
+    .nalloc = -1,                           \
+    .local_iov = {                          \
+        .iov_base = (void *)(buf),          \
+        .iov_len = 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);
diff --git a/block/io.c b/block/io.c
index bd9d688f8b..39a1a848af 100644
--- a/block/io.c
+++ b/block/io.c
@@ -949,17 +949,12 @@ int bdrv_preadv(BdrvChild *child, int64_t offset, 
QEMUIOVector *qiov)

  int bdrv_pread(BdrvChild *child, int64_t offset, void *buf, int bytes)
  {
-    QEMUIOVector qiov;
-    struct iovec iov = {
-        .iov_base = (void *)buf,
-        .iov_len = bytes,
-    };
+    QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, buf, bytes);

      if (bytes < 0) {
          return -EINVAL;
      }

-    qemu_iovec_init_external(&qiov, &iov, 1);
      return bdrv_preadv(child, offset, &qiov);
  }



Ok?


It's also possible to unite nalloc and local_iov.iov_base, to save 4 bytes, and 
we'll have to add

  void qemu_iovec_add(QEMUIOVector *qiov, void *base, size_t len)
  {
-    assert(qiov->nalloc != -1);
+    assert(qiov->iov != &qiov->local_iov && qiov->nalloc != -1);

But I think it's not worth it.

-- 
Best regards,
Vladimir