[Qemu-devel] [PATCH 0/2] block: local qiov helper: part I

[Qemu-devel] [PATCH 0/2] block: local qiov helper: part I

[Vladimir Sementsov-Ogievskiy]

Hi all!

Here is a proposal for a new simple helper for a very often patter
around qemu_iovec_init_external, when we need simple qiov with only
one iov, initialized from external buffer.

Here only block/io.c updated to use new helper, I'll update other
things on top of this separately.

Vladimir Sementsov-Ogievskiy (2):
  block: enhance QEMUIOVector structure
  block/io: use qemu_iovec_init_buf

 include/qemu/iov.h | 41 ++++++++++++++++++++-
 block/io.c         | 89 +++++++++++-----------------------------------
 2 files changed, 60 insertions(+), 70 deletions(-)

-- 
2.18.0

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

[Vladimir Sementsov-Ogievskiy]

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 | 41 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 40 insertions(+), 1 deletion(-)

diff --git a/include/qemu/iov.h b/include/qemu/iov.h
index 5f433c7768..f739cff97d 100644
--- a/include/qemu/iov.h
+++ b/include/qemu/iov.h
@@ -134,9 +134,48 @@ 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
+     * @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.
+     */
+    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                      \
+    }                                       \
+}
+
+static inline void qemu_iovec_init_buf(QEMUIOVector *qiov,
+                                       void *buf, size_t len)
+{
+    *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);
-- 
2.18.0

[Qemu-devel] [PATCH 2/2] block/io: use qemu_iovec_init_buf

[Vladimir Sementsov-Ogievskiy]

Use new qemu_iovec_init_buf() instead of
qemu_iovec_init_external( ... , 1), which simplifies the code.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/io.c | 89 ++++++++++++------------------------------------------
 1 file changed, 20 insertions(+), 69 deletions(-)

diff --git a/block/io.c b/block/io.c
index bd9d688f8b..80961910a6 100644
--- a/block/io.c
+++ b/block/io.c
@@ -842,17 +842,13 @@ static int bdrv_prwv_co(BdrvChild *child, int64_t offset,
 static int bdrv_rw_co(BdrvChild *child, int64_t sector_num, uint8_t *buf,
                       int nb_sectors, bool is_write, BdrvRequestFlags flags)
 {
-    QEMUIOVector qiov;
-    struct iovec iov = {
-        .iov_base = (void *)buf,
-        .iov_len = nb_sectors * BDRV_SECTOR_SIZE,
-    };
+    QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, buf,
+                                            nb_sectors * BDRV_SECTOR_SIZE);
 
     if (nb_sectors < 0 || nb_sectors > BDRV_REQUEST_MAX_SECTORS) {
         return -EINVAL;
     }
 
-    qemu_iovec_init_external(&qiov, &iov, 1);
     return bdrv_prwv_co(child, sector_num << BDRV_SECTOR_BITS,
                         &qiov, is_write, flags);
 }
@@ -879,13 +875,8 @@ int bdrv_write(BdrvChild *child, int64_t sector_num,
 int bdrv_pwrite_zeroes(BdrvChild *child, int64_t offset,
                        int bytes, BdrvRequestFlags flags)
 {
-    QEMUIOVector qiov;
-    struct iovec iov = {
-        .iov_base = NULL,
-        .iov_len = bytes,
-    };
+    QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, NULL, bytes);
 
-    qemu_iovec_init_external(&qiov, &iov, 1);
     return bdrv_prwv_co(child, offset, &qiov, true,
                         BDRV_REQ_ZERO_WRITE | flags);
 }
@@ -949,17 +940,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);
 }
 
@@ -977,17 +963,12 @@ int bdrv_pwritev(BdrvChild *child, int64_t offset, QEMUIOVector *qiov)
 
 int bdrv_pwrite(BdrvChild *child, int64_t offset, const void *buf, int bytes)
 {
-    QEMUIOVector qiov;
-    struct iovec iov = {
-        .iov_base   = (void *) buf,
-        .iov_len    = bytes,
-    };
+    QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, NULL, bytes);
 
     if (bytes < 0) {
         return -EINVAL;
     }
 
-    qemu_iovec_init_external(&qiov, &iov, 1);
     return bdrv_pwritev(child, offset, &qiov);
 }
 
@@ -1164,7 +1145,6 @@ static int coroutine_fn bdrv_co_do_copy_on_readv(BdrvChild *child,
     void *bounce_buffer;
 
     BlockDriver *drv = bs->drv;
-    struct iovec iov;
     QEMUIOVector local_qiov;
     int64_t cluster_offset;
     int64_t cluster_bytes;
@@ -1229,9 +1209,7 @@ static int coroutine_fn bdrv_co_do_copy_on_readv(BdrvChild *child,
 
         if (ret <= 0) {
             /* Must copy-on-read; use the bounce buffer */
-            iov.iov_base = bounce_buffer;
-            iov.iov_len = pnum = MIN(pnum, MAX_BOUNCE_BUFFER);
-            qemu_iovec_init_external(&local_qiov, &iov, 1);
+            qemu_iovec_init_buf(&local_qiov, bounce_buffer, pnum);
 
             ret = bdrv_driver_preadv(bs, cluster_offset, pnum,
                                      &local_qiov, 0);
@@ -1476,7 +1454,7 @@ static int coroutine_fn bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
 {
     BlockDriver *drv = bs->drv;
     QEMUIOVector qiov;
-    struct iovec iov = {0};
+    void *buf = NULL;
     int ret = 0;
     bool need_flush = false;
     int head = 0;
@@ -1546,16 +1524,15 @@ static int coroutine_fn bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
                 need_flush = true;
             }
             num = MIN(num, max_transfer);
-            iov.iov_len = num;
-            if (iov.iov_base == NULL) {
-                iov.iov_base = qemu_try_blockalign(bs, num);
-                if (iov.iov_base == NULL) {
+            if (buf == NULL) {
+                buf = qemu_try_blockalign(bs, num);
+                if (buf == NULL) {
                     ret = -ENOMEM;
                     goto fail;
                 }
-                memset(iov.iov_base, 0, num);
+                memset(buf, 0, num);
             }
-            qemu_iovec_init_external(&qiov, &iov, 1);
+            qemu_iovec_init_buf(&qiov, buf, num);
 
             ret = bdrv_driver_pwritev(bs, offset, num, &qiov, write_flags);
 
@@ -1563,8 +1540,8 @@ static int coroutine_fn bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
              * all future requests.
              */
             if (num < max_transfer) {
-                qemu_vfree(iov.iov_base);
-                iov.iov_base = NULL;
+                qemu_vfree(buf);
+                buf = NULL;
             }
         }
 
@@ -1576,7 +1553,7 @@ fail:
     if (ret == 0 && need_flush) {
         ret = bdrv_co_flush(bs);
     }
-    qemu_vfree(iov.iov_base);
+    qemu_vfree(buf);
     return ret;
 }
 
@@ -1762,7 +1739,6 @@ static int coroutine_fn bdrv_co_do_zero_pwritev(BdrvChild *child,
     BlockDriverState *bs = child->bs;
     uint8_t *buf = NULL;
     QEMUIOVector local_qiov;
-    struct iovec iov;
     uint64_t align = bs->bl.request_alignment;
     unsigned int head_padding_bytes, tail_padding_bytes;
     int ret = 0;
@@ -1774,11 +1750,7 @@ static int coroutine_fn bdrv_co_do_zero_pwritev(BdrvChild *child,
     assert(flags & BDRV_REQ_ZERO_WRITE);
     if (head_padding_bytes || tail_padding_bytes) {
         buf = qemu_blockalign(bs, align);
-        iov = (struct iovec) {
-            .iov_base   = buf,
-            .iov_len    = align,
-        };
-        qemu_iovec_init_external(&local_qiov, &iov, 1);
+        qemu_iovec_init_buf(&local_qiov, buf, align);
     }
     if (head_padding_bytes) {
         uint64_t zero_bytes = MIN(bytes, align - head_padding_bytes);
@@ -1884,17 +1856,12 @@ int coroutine_fn bdrv_co_pwritev(BdrvChild *child,
 
     if (offset & (align - 1)) {
         QEMUIOVector head_qiov;
-        struct iovec head_iov;
 
         mark_request_serialising(&req, align);
         wait_serialising_requests(&req);
 
         head_buf = qemu_blockalign(bs, align);
-        head_iov = (struct iovec) {
-            .iov_base   = head_buf,
-            .iov_len    = align,
-        };
-        qemu_iovec_init_external(&head_qiov, &head_iov, 1);
+        qemu_iovec_init_buf(&head_qiov, head_buf, align);
 
         bdrv_debug_event(bs, BLKDBG_PWRITEV_RMW_HEAD);
         ret = bdrv_aligned_preadv(child, &req, offset & ~(align - 1), align,
@@ -1923,7 +1890,6 @@ int coroutine_fn bdrv_co_pwritev(BdrvChild *child,
 
     if ((offset + bytes) & (align - 1)) {
         QEMUIOVector tail_qiov;
-        struct iovec tail_iov;
         size_t tail_bytes;
         bool waited;
 
@@ -1932,11 +1898,7 @@ int coroutine_fn bdrv_co_pwritev(BdrvChild *child,
         assert(!waited || !use_local_qiov);
 
         tail_buf = qemu_blockalign(bs, align);
-        tail_iov = (struct iovec) {
-            .iov_base   = tail_buf,
-            .iov_len    = align,
-        };
-        qemu_iovec_init_external(&tail_qiov, &tail_iov, 1);
+        qemu_iovec_init_buf(&tail_qiov, tail_buf, align);
 
         bdrv_debug_event(bs, BLKDBG_PWRITEV_RMW_TAIL);
         ret = bdrv_aligned_preadv(child, &req, (offset + bytes) & ~(align - 1),
@@ -2465,15 +2427,9 @@ bdrv_rw_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos,
 int bdrv_save_vmstate(BlockDriverState *bs, const uint8_t *buf,
                       int64_t pos, int size)
 {
-    QEMUIOVector qiov;
-    struct iovec iov = {
-        .iov_base   = (void *) buf,
-        .iov_len    = size,
-    };
+    QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, buf, size);
     int ret;
 
-    qemu_iovec_init_external(&qiov, &iov, 1);
-
     ret = bdrv_writev_vmstate(bs, &qiov, pos);
     if (ret < 0) {
         return ret;
@@ -2490,14 +2446,9 @@ int bdrv_writev_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos)
 int bdrv_load_vmstate(BlockDriverState *bs, uint8_t *buf,
                       int64_t pos, int size)
 {
-    QEMUIOVector qiov;
-    struct iovec iov = {
-        .iov_base   = buf,
-        .iov_len    = size,
-    };
+    QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, buf, size);
     int ret;
 
-    qemu_iovec_init_external(&qiov, &iov, 1);
     ret = bdrv_readv_vmstate(bs, &qiov, pos);
     if (ret < 0) {
         return ret;
-- 
2.18.0

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 3499 bytes --]

On 1/29/19 8:37 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 | 41 ++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 40 insertions(+), 1 deletion(-)
> 

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

We don't use G_STATIC_ASSERT() elsewhere (slirp excepted, as that is
trying to split off to a separate project); so these should be:

QEMU_BUILD_BUG_ON(offsetof(QEMUIOVector, size) !=
                  offsetof(QEMUIOVector, local_iov.iov_len));

and so on.

POSIX does not guarantee that iov_base occurs prior to iov_len in struct
iovec; your code is therefore rather fragile and would fail to compile
if we encounter a libc where iov_len is positioned differently than in
glibc, tripping the first assertion.  For an offset other than 0, you
could declare:
  char [offsetof(struct iovec, iov_len)] __pad;
to be more robust; but since C doesn't like 0-length array declarations,
I'm not sure how you would cater to a libc where iov_len is at offset 0,
short of a configure probe and #ifdeffery, which feels like a bit much
to manage (really, the point of the assert is that we don't have to
worry about an unusual libc having a different offset UNLESS the build
fails, so no need to address a problem we aren't hitting yet).

The second assertion (about sizeof being identical) is redundant, since
POSIX requires size_t iov_len, and we used size_t size (while a libc
might reorder the fields of struct iovec, they shouldn't be using the
wrong types); but perhaps you could use:
 typeof(struct iovec.iov_len) size;
in the declaration to avoid even that possibility (I'm not sure it is
worth it, though).

> +
> +#define QEMU_IOVEC_INIT_BUF(self, buf, len) \
> +{                                           \
> +    .iov = &(self).local_iov,               \
> +    .niov = 1,                              \
> +    .nalloc = -1,                           \
> +    .local_iov = {                          \
> +        .iov_base = (void *)(buf),          \

Why is the cast necessary?  Are we casting away const?  Since C already
allows assignment of any other (non-const) pointer to void* without a
cast, it looks superfluous.

> +        .iov_len = len                      \

Missing () around len. I might also have used a trailing comma (I find
it easier to always use trailing comma; while we're unlikely to add more
members here, it does make for less churn in other places where a struct
may grow).

> +    }                                       \
> +}
> +
> +static inline void qemu_iovec_init_buf(QEMUIOVector *qiov,
> +                                       void *buf, size_t len)
> +{
> +    *qiov = (QEMUIOVector) QEMU_IOVEC_INIT_BUF(*qiov, buf, len);
> +}

Why both a macro and a function that uses the macro? Do you expect any
other code to actually use the macro, or will the function always be
sufficient, in which case you could inline the initializer without the
use of a macro.

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

Re: [Qemu-devel] [PATCH 2/2] block/io: use qemu_iovec_init_buf

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2281 bytes --]

On 1/29/19 8:38 AM, Vladimir Sementsov-Ogievskiy wrote:
> Use new qemu_iovec_init_buf() instead of
> qemu_iovec_init_external( ... , 1), which simplifies the code.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/io.c | 89 ++++++++++++------------------------------------------
>  1 file changed, 20 insertions(+), 69 deletions(-)
> 
> diff --git a/block/io.c b/block/io.c
> index bd9d688f8b..80961910a6 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -842,17 +842,13 @@ static int bdrv_prwv_co(BdrvChild *child, int64_t offset,
>  static int bdrv_rw_co(BdrvChild *child, int64_t sector_num, uint8_t *buf,
>                        int nb_sectors, bool is_write, BdrvRequestFlags flags)
>  {
> -    QEMUIOVector qiov;
> -    struct iovec iov = {
> -        .iov_base = (void *)buf,
> -        .iov_len = nb_sectors * BDRV_SECTOR_SIZE,
> -    };
> +    QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, buf,
> +                                            nb_sectors * BDRV_SECTOR_SIZE);

Okay, so you ARE using both the macro...

> @@ -977,17 +963,12 @@ int bdrv_pwritev(BdrvChild *child, int64_t offset, QEMUIOVector *qiov)
>  
>  int bdrv_pwrite(BdrvChild *child, int64_t offset, const void *buf, int bytes)
>  {
> -    QEMUIOVector qiov;
> -    struct iovec iov = {
> -        .iov_base   = (void *) buf,
> -        .iov_len    = bytes,
> -    };
> +    QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, NULL, bytes);

Ouch - why are you passing NULL instead of buf?  But this answers why
the macro had to cast - it is indeed casting away const.

> @@ -1229,9 +1209,7 @@ static int coroutine_fn bdrv_co_do_copy_on_readv(BdrvChild *child,
>  
>          if (ret <= 0) {
>              /* Must copy-on-read; use the bounce buffer */
> -            iov.iov_base = bounce_buffer;
> -            iov.iov_len = pnum = MIN(pnum, MAX_BOUNCE_BUFFER);
> -            qemu_iovec_init_external(&local_qiov, &iov, 1);
> +            qemu_iovec_init_buf(&local_qiov, bounce_buffer, pnum);

...and the function.

Ouch - why did you lose the assignment of pnum = MIN() here?

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

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

[Vladimir Sementsov-Ogievskiy]

30.01.2019 0:35, Eric Blake wrote:
> On 1/29/19 8:37 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 | 41 ++++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 40 insertions(+), 1 deletion(-)
>>
> 
>>   
>> +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));
> 
> We don't use G_STATIC_ASSERT() elsewhere (slirp excepted, as that is
> trying to split off to a separate project); so these should be:
> 
> QEMU_BUILD_BUG_ON(offsetof(QEMUIOVector, size) !=
>                    offsetof(QEMUIOVector, local_iov.iov_len));
> 
> and so on.
> 
> POSIX does not guarantee that iov_base occurs prior to iov_len in struct
> iovec; your code is therefore rather fragile and would fail to compile
> if we encounter a libc where iov_len is positioned differently than in
> glibc, tripping the first assertion.  For an offset other than 0, you
> could declare:
>    char [offsetof(struct iovec, iov_len)] __pad;
> to be more robust; but since C doesn't like 0-length array declarations,
> I'm not sure how you would cater to a libc where iov_len is at offset 0,

Hmm, tested a bit, and the only complain I can get is a warning about 0-sized
array when option -pedantic is set. So, is it a real problem?

And we already have a lot of zero-sized arrays in Qemu, just look at
  git grep '\w\+\s\+\w\+\[0\];' | grep -v return


hm, or we can do like this:

typedef struct QEMUIOVector {
     struct iovec *iov;
     int niov;
     union {
         struct {
             int nalloc;
             struct iovec local_iov;
         };
         struct {
             char __pad[sizeof(int) + offsetof(struct iovec, iov_len)];
             size_t size;
         };
     };
} QEMUIOVector;


> short of a configure probe and #ifdeffery, which feels like a bit much
> to manage (really, the point of the assert is that we don't have to
> worry about an unusual libc having a different offset UNLESS the build
> fails, so no need to address a problem we aren't hitting yet).

However, char __pad[offsetof(struct iovec, iov_len)]; seems useful and even
more self-documented, so I'll use it in v2, if nobody minds.

> 
> The second assertion (about sizeof being identical) is redundant

Ok.

> since
> POSIX requires size_t iov_len, and we used size_t size (while a libc
> might reorder the fields of struct iovec, they shouldn't be using the
> wrong types); but perhaps you could use:
>   typeof(struct iovec.iov_len) size;
> in the declaration to avoid even that possibility (I'm not sure it is
> worth it, though).
> 
>> +
>> +#define QEMU_IOVEC_INIT_BUF(self, buf, len) \
>> +{                                           \
>> +    .iov = &(self).local_iov,               \
>> +    .niov = 1,                              \
>> +    .nalloc = -1,                           \
>> +    .local_iov = {                          \
>> +        .iov_base = (void *)(buf),          \
> 
> Why is the cast necessary?  Are we casting away const?  Since C already
> allows assignment of any other (non-const) pointer to void* without a
> cast, it looks superfluous.
> 
>> +        .iov_len = len                      \
> 
> Missing () around len.

For style? What the thing len should be to break it without ()?

> I might also have used a trailing comma (I find
> it easier to always use trailing comma; while we're unlikely to add more
> members here, it does make for less churn in other places where a struct
> may grow).
> 
>> +    }                                       \
>> +}
>> +
>> +static inline void qemu_iovec_init_buf(QEMUIOVector *qiov,
>> +                                       void *buf, size_t len)
>> +{
>> +    *qiov = (QEMUIOVector) QEMU_IOVEC_INIT_BUF(*qiov, buf, len);
>> +}
> 
> Why both a macro and a function that uses the macro? Do you expect any
> other code to actually use the macro, or will the function always be
> sufficient, in which case you could inline the initializer without the
> use of a macro.
> 


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 2/2] block/io: use qemu_iovec_init_buf

[Vladimir Sementsov-Ogievskiy]

30.01.2019 0:41, Eric Blake wrote:
> On 1/29/19 8:38 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Use new qemu_iovec_init_buf() instead of
>> qemu_iovec_init_external( ... , 1), which simplifies the code.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   block/io.c | 89 ++++++++++++------------------------------------------
>>   1 file changed, 20 insertions(+), 69 deletions(-)
>>
>> diff --git a/block/io.c b/block/io.c
>> index bd9d688f8b..80961910a6 100644
>> --- a/block/io.c
>> +++ b/block/io.c
>> @@ -842,17 +842,13 @@ static int bdrv_prwv_co(BdrvChild *child, int64_t offset,
>>   static int bdrv_rw_co(BdrvChild *child, int64_t sector_num, uint8_t *buf,
>>                         int nb_sectors, bool is_write, BdrvRequestFlags flags)
>>   {
>> -    QEMUIOVector qiov;
>> -    struct iovec iov = {
>> -        .iov_base = (void *)buf,
>> -        .iov_len = nb_sectors * BDRV_SECTOR_SIZE,
>> -    };
>> +    QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, buf,
>> +                                            nb_sectors * BDRV_SECTOR_SIZE);
> 
> Okay, so you ARE using both the macro...
> 
>> @@ -977,17 +963,12 @@ int bdrv_pwritev(BdrvChild *child, int64_t offset, QEMUIOVector *qiov)
>>   
>>   int bdrv_pwrite(BdrvChild *child, int64_t offset, const void *buf, int bytes)
>>   {
>> -    QEMUIOVector qiov;
>> -    struct iovec iov = {
>> -        .iov_base   = (void *) buf,
>> -        .iov_len    = bytes,
>> -    };
>> +    QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, NULL, bytes);
> 
> Ouch - why are you passing NULL instead of buf?  But this answers why
> the macro had to cast - it is indeed casting away const.
> 
>> @@ -1229,9 +1209,7 @@ static int coroutine_fn bdrv_co_do_copy_on_readv(BdrvChild *child,
>>   
>>           if (ret <= 0) {
>>               /* Must copy-on-read; use the bounce buffer */
>> -            iov.iov_base = bounce_buffer;
>> -            iov.iov_len = pnum = MIN(pnum, MAX_BOUNCE_BUFFER);
>> -            qemu_iovec_init_external(&local_qiov, &iov, 1);
>> +            qemu_iovec_init_buf(&local_qiov, bounce_buffer, pnum);
> 
> ...and the function.
> 
> Ouch - why did you lose the assignment of pnum = MIN() here?
> 

Thank you, both hit the mark, I was inattentive.


-- 
Best regards,
Vladimir

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 5279 bytes --]

On 1/30/19 3:14 AM, Vladimir Sementsov-Ogievskiy wrote:
> 30.01.2019 0:35, Eric Blake wrote:
>> On 1/29/19 8:37 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 | 41 ++++++++++++++++++++++++++++++++++++++++-
>>>   1 file changed, 40 insertions(+), 1 deletion(-)

>> POSIX does not guarantee that iov_base occurs prior to iov_len in struct
>> iovec; your code is therefore rather fragile and would fail to compile
>> if we encounter a libc where iov_len is positioned differently than in
>> glibc, tripping the first assertion.  For an offset other than 0, you
>> could declare:
>>    char [offsetof(struct iovec, iov_len)] __pad;
>> to be more robust; but since C doesn't like 0-length array declarations,
>> I'm not sure how you would cater to a libc where iov_len is at offset 0,
> 
> Hmm, tested a bit, and the only complain I can get is a warning about 0-sized
> array when option -pedantic is set. So, is it a real problem?

Even if we require the use of gcc extensions elsewhere, it doesn't hurt
to avoid them where it is easy.

> 
> And we already have a lot of zero-sized arrays in Qemu, just look at
>   git grep '\w\+\s\+\w\+\[0\];' | grep -v return

And how many of those are the last entry in a struct?  A 0-size array at
the end is a common idiom for a flex-sized struct (without relying on
C99 VLA); a 0-size array in the middle of a struct is unusual.

> 
> 
> hm, or we can do like this:
> 
> typedef struct QEMUIOVector {
>      struct iovec *iov;
>      int niov;
>      union {
>          struct {
>              int nalloc;
>              struct iovec local_iov;
>          };
>          struct {
>              char __pad[sizeof(int) + offsetof(struct iovec, iov_len)];
>              size_t size;
>          };
>      };
> } QEMUIOVector;

Yes, that's a reasonable way to do it.

> 
> 
>> short of a configure probe and #ifdeffery, which feels like a bit much
>> to manage (really, the point of the assert is that we don't have to
>> worry about an unusual libc having a different offset UNLESS the build
>> fails, so no need to address a problem we aren't hitting yet).
> 
> However, char __pad[offsetof(struct iovec, iov_len)]; seems useful and even
> more self-documented, so I'll use it in v2, if nobody minds.
>

We'll see how it looks, then.


>>> +        .iov_base = (void *)(buf),          \
>>
>> Why is the cast necessary?  Are we casting away const?

Answered in 2/2 - yes. But a comment about /* cast away const */ is useful.

>>  Since C already
>> allows assignment of any other (non-const) pointer to void* without a
>> cast, it looks superfluous.
>>
>>> +        .iov_len = len                      \
>>
>> Missing () around len.
> 
> For style? What the thing len should be to break it without ()?

Although we are unlikely to hit this given our coding conventions,
remember that '=' has higher precedence than ',', for this contorted
example (and yes, it's contorted because you can't pass the comma
operator through to a macro without either supplying your own (), which
defeats the demonstration, or relying on a helper macro such as
raw_pair() here):

$ cat foo.c
struct s {
    int i;
};
#define raw_pair(a, b) a, b
#define A(arg) { .i = (arg) }
#define B(arg) { .i = arg }

int
main(int argc, char **argv)
{
    struct s s1 = A(raw_pair(1, 2));
    struct s s2 = B(raw_pair(4, 8));
    return s1.i + s2.i;
}
$ gcc -o foo foo.c
foo.c: In function ‘main’:
foo.c:12:33: warning: excess elements in struct initializer
     struct s s2 = B(raw_pair(4, 8));
                                 ^
foo.c:6:23: note: in definition of macro ‘B’
 #define B(arg) { .i = arg }
                       ^~~
foo.c:12:21: note: in expansion of macro ‘raw_pair’
     struct s s2 = B(raw_pair(4, 8));
                     ^~~~~~~~
foo.c:12:33: note: (near initialization for ‘s2’)
     struct s s2 = B(raw_pair(4, 8));
                                 ^
foo.c:6:23: note: in definition of macro ‘B’
 #define B(arg) { .i = arg }
                       ^~~
foo.c:12:21: note: in expansion of macro ‘raw_pair’
     struct s s2 = B(raw_pair(4, 8));
                     ^~~~~~~~
$ ./foo; echo $?
6

which says that for A() using the (), there were no warnings and the
second value was assigned (-Wall changes that, complaining about unused
value of the left side of the comma operator - but that's okay); but for
B() without (), there was a diagnostic even without -Wall about too many
initializers, and the first value was assigned.

>> Why both a macro and a function that uses the macro? 

Also answered in 2/2 - the macro is for variable declarations; the
function is for runtime code such as for-loops that start with an
uninitialized declaration and have to reinitialize things. They also
differ in that one takes a struct, the other takes a pointer to a struct.


-- 
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: 484 bytes --]

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

[Vladimir Sementsov-Ogievskiy]

30.01.2019 17:55, Eric Blake wrote:
> On 1/30/19 3:14 AM, Vladimir Sementsov-Ogievskiy wrote:
>> 30.01.2019 0:35, Eric Blake wrote:
>>> On 1/29/19 8:37 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 | 41 ++++++++++++++++++++++++++++++++++++++++-
>>>>    1 file changed, 40 insertions(+), 1 deletion(-)
> 
>>> POSIX does not guarantee that iov_base occurs prior to iov_len in struct
>>> iovec; your code is therefore rather fragile and would fail to compile
>>> if we encounter a libc where iov_len is positioned differently than in
>>> glibc, tripping the first assertion.  For an offset other than 0, you
>>> could declare:
>>>     char [offsetof(struct iovec, iov_len)] __pad;
>>> to be more robust; but since C doesn't like 0-length array declarations,
>>> I'm not sure how you would cater to a libc where iov_len is at offset 0,
>>
>> Hmm, tested a bit, and the only complain I can get is a warning about 0-sized
>> array when option -pedantic is set. So, is it a real problem?
> 
> Even if we require the use of gcc extensions elsewhere, it doesn't hurt
> to avoid them where it is easy.
> 
>>
>> And we already have a lot of zero-sized arrays in Qemu, just look at
>>    git grep '\w\+\s\+\w\+\[0\];' | grep -v return
> 
> And how many of those are the last entry in a struct?  A 0-size array at
> the end is a common idiom for a flex-sized struct (without relying on
> C99 VLA); a 0-size array in the middle of a struct is unusual.

Reasonable, so ...

> 
>>
>>
>> hm, or we can do like this:
>>
>> typedef struct QEMUIOVector {
>>       struct iovec *iov;
>>       int niov;
>>       union {
>>           struct {
>>               int nalloc;
>>               struct iovec local_iov;
>>           };
>>           struct {
>>               char __pad[sizeof(int) + offsetof(struct iovec, iov_len)];
>>               size_t size;
>>           };
>>       };
>> } QEMUIOVector;
> 
> Yes, that's a reasonable way to do it.

... will try this in v2, if no more comments.

> 
>>
>>
>>> short of a configure probe and #ifdeffery, which feels like a bit much
>>> to manage (really, the point of the assert is that we don't have to
>>> worry about an unusual libc having a different offset UNLESS the build
>>> fails, so no need to address a problem we aren't hitting yet).
>>
>> However, char __pad[offsetof(struct iovec, iov_len)]; seems useful and even
>> more self-documented, so I'll use it in v2, if nobody minds.
>>
> 
> We'll see how it looks, then.
> 
> 
>>>> +        .iov_base = (void *)(buf),          \
>>>
>>> Why is the cast necessary?  Are we casting away const?
> 
> Answered in 2/2 - yes. But a comment about /* cast away const */ is useful.
> 
>>>   Since C already
>>> allows assignment of any other (non-const) pointer to void* without a
>>> cast, it looks superfluous.
>>>
>>>> +        .iov_len = len                      \
>>>
>>> Missing () around len.
>>
>> For style? What the thing len should be to break it without ()?
> 
> Although we are unlikely to hit this given our coding conventions,
> remember that '=' has higher precedence than ',', for this contorted
> example (and yes, it's contorted because you can't pass the comma
> operator through to a macro without either supplying your own (), which
> defeats the demonstration, or relying on a helper macro such as
> raw_pair() here):
> 
> $ cat foo.c
> struct s {
>      int i;
> };
> #define raw_pair(a, b) a, b
> #define A(arg) { .i = (arg) }
> #define B(arg) { .i = arg }
> 
> int
> main(int argc, char **argv)
> {
>      struct s s1 = A(raw_pair(1, 2));
>      struct s s2 = B(raw_pair(4, 8));
>      return s1.i + s2.i;
> }
> $ gcc -o foo foo.c
> foo.c: In function ‘main’:
> foo.c:12:33: warning: excess elements in struct initializer
>       struct s s2 = B(raw_pair(4, 8));
>                                   ^
> foo.c:6:23: note: in definition of macro ‘B’
>   #define B(arg) { .i = arg }
>                         ^~~
> foo.c:12:21: note: in expansion of macro ‘raw_pair’
>       struct s s2 = B(raw_pair(4, 8));
>                       ^~~~~~~~
> foo.c:12:33: note: (near initialization for ‘s2’)
>       struct s s2 = B(raw_pair(4, 8));
>                                   ^
> foo.c:6:23: note: in definition of macro ‘B’
>   #define B(arg) { .i = arg }
>                         ^~~
> foo.c:12:21: note: in expansion of macro ‘raw_pair’
>       struct s s2 = B(raw_pair(4, 8));
>                       ^~~~~~~~
> $ ./foo; echo $?
> 6
> 
> which says that for A() using the (), there were no warnings and the
> second value was assigned (-Wall changes that, complaining about unused
> value of the left side of the comma operator - but that's okay); but for
> B() without (), there was a diagnostic even without -Wall about too many
> initializers, and the first value was assigned.

Oh, that's cool!

> 
>>> Why both a macro and a function that uses the macro?
> 
> Also answered in 2/2 - the macro is for variable declarations; the
> function is for runtime code such as for-loops that start with an
> uninitialized declaration and have to reinitialize things. They also
> differ in that one takes a struct, the other takes a pointer to a struct.
> 
> 


-- 
Best regards,
Vladimir