The iov_discard_front/back() operations are useful for parsing iovecs
but they modify the array elements. If the original array is needed
after parsing finishes there is currently no way to restore it.

Although g_memdup() can be used before performing destructive
iov_discard_front/back() operations, this is inefficient.

Introduce iov_discard_undo() to restore the array to the state prior to
an iov_discard_front/back() operation.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 include/qemu/iov.h |  23 +++++++
 tests/test-iov.c   | 165 +++++++++++++++++++++++++++++++++++++++++++++
 util/iov.c         |  50 ++++++++++++--
 3 files changed, 234 insertions(+), 4 deletions(-)

diff --git a/include/qemu/iov.h b/include/qemu/iov.h
index bffc151282..b6b283a5e5 100644
--- a/include/qemu/iov.h
+++ b/include/qemu/iov.h
@@ -130,6 +130,29 @@ size_t iov_discard_front(struct iovec **iov, unsigned int *iov_cnt,
 size_t iov_discard_back(struct iovec *iov, unsigned int *iov_cnt,
                         size_t bytes);
 
+/* Information needed to undo an iov_discard_*() operation */
+typedef struct {
+    struct iovec *modified_iov;
+    struct iovec orig;
+} IOVDiscardUndo;
+
+/*
+ * Undo an iov_discard_front_undoable() or iov_discard_back_undoable()
+ * operation. If multiple operations are made then each one needs a separate
+ * IOVDiscardUndo and iov_discard_undo() must be called in the reverse order
+ * that the operations were made.
+ */
+void iov_discard_undo(IOVDiscardUndo *undo);
+
+/*
+ * Undoable versions of iov_discard_front() and iov_discard_back(). Use
+ * iov_discard_undo() to reset to the state before the discard operations.
+ */
+size_t iov_discard_front_undoable(struct iovec **iov, unsigned int *iov_cnt,
+                                  size_t bytes, IOVDiscardUndo *undo);
+size_t iov_discard_back_undoable(struct iovec *iov, unsigned int *iov_cnt,
+                                 size_t bytes, IOVDiscardUndo *undo);
+
 typedef struct QEMUIOVector {
     struct iovec *iov;
     int niov;
diff --git a/tests/test-iov.c b/tests/test-iov.c
index 458ca25099..9c415e2f1f 100644
--- a/tests/test-iov.c
+++ b/tests/test-iov.c
@@ -26,6 +26,12 @@ static void iov_free(struct iovec *iov, unsigned niov)
     g_free(iov);
 }
 
+static bool iov_equals(const struct iovec *a, const struct iovec *b,
+                       unsigned niov)
+{
+    return memcmp(a, b, sizeof(a[0]) * niov) == 0;
+}
+
 static void test_iov_bytes(struct iovec *iov, unsigned niov,
                            size_t offset, size_t bytes)
 {
@@ -335,6 +341,87 @@ static void test_discard_front(void)
     iov_free(iov, iov_cnt);
 }
 
+static void test_discard_front_undo(void)
+{
+    IOVDiscardUndo undo;
+    struct iovec *iov;
+    struct iovec *iov_tmp;
+    struct iovec *iov_orig;
+    unsigned int iov_cnt;
+    unsigned int iov_cnt_tmp;
+    size_t size;
+
+    /* Discard zero bytes */
+    iov_random(&iov, &iov_cnt);
+    iov_orig = g_memdup(iov, sizeof(iov[0]) * iov_cnt);
+    iov_tmp = iov;
+    iov_cnt_tmp = iov_cnt;
+    iov_discard_front_undoable(&iov_tmp, &iov_cnt_tmp, 0, &undo);
+    iov_discard_undo(&undo);
+    assert(iov_equals(iov, iov_orig, iov_cnt));
+    g_free(iov_orig);
+    iov_free(iov, iov_cnt);
+
+    /* Discard more bytes than vector size */
+    iov_random(&iov, &iov_cnt);
+    iov_orig = g_memdup(iov, sizeof(iov[0]) * iov_cnt);
+    iov_tmp = iov;
+    iov_cnt_tmp = iov_cnt;
+    size = iov_size(iov, iov_cnt);
+    iov_discard_front_undoable(&iov_tmp, &iov_cnt_tmp, size + 1, &undo);
+    iov_discard_undo(&undo);
+    assert(iov_equals(iov, iov_orig, iov_cnt));
+    g_free(iov_orig);
+    iov_free(iov, iov_cnt);
+
+    /* Discard entire vector */
+    iov_random(&iov, &iov_cnt);
+    iov_orig = g_memdup(iov, sizeof(iov[0]) * iov_cnt);
+    iov_tmp = iov;
+    iov_cnt_tmp = iov_cnt;
+    size = iov_size(iov, iov_cnt);
+    iov_discard_front_undoable(&iov_tmp, &iov_cnt_tmp, size, &undo);
+    iov_discard_undo(&undo);
+    assert(iov_equals(iov, iov_orig, iov_cnt));
+    g_free(iov_orig);
+    iov_free(iov, iov_cnt);
+
+    /* Discard within first element */
+    iov_random(&iov, &iov_cnt);
+    iov_orig = g_memdup(iov, sizeof(iov[0]) * iov_cnt);
+    iov_tmp = iov;
+    iov_cnt_tmp = iov_cnt;
+    size = g_test_rand_int_range(1, iov->iov_len);
+    iov_discard_front_undoable(&iov_tmp, &iov_cnt_tmp, size, &undo);
+    iov_discard_undo(&undo);
+    assert(iov_equals(iov, iov_orig, iov_cnt));
+    g_free(iov_orig);
+    iov_free(iov, iov_cnt);
+
+    /* Discard entire first element */
+    iov_random(&iov, &iov_cnt);
+    iov_orig = g_memdup(iov, sizeof(iov[0]) * iov_cnt);
+    iov_tmp = iov;
+    iov_cnt_tmp = iov_cnt;
+    iov_discard_front_undoable(&iov_tmp, &iov_cnt_tmp, iov->iov_len, &undo);
+    iov_discard_undo(&undo);
+    assert(iov_equals(iov, iov_orig, iov_cnt));
+    g_free(iov_orig);
+    iov_free(iov, iov_cnt);
+
+    /* Discard within second element */
+    iov_random(&iov, &iov_cnt);
+    iov_orig = g_memdup(iov, sizeof(iov[0]) * iov_cnt);
+    iov_tmp = iov;
+    iov_cnt_tmp = iov_cnt;
+    size = iov->iov_len + g_test_rand_int_range(1, iov[1].iov_len);
+    iov_discard_front_undoable(&iov_tmp, &iov_cnt_tmp, size, &undo);
+    iov_discard_undo(&undo);
+    assert(iov_equals(iov, iov_orig, iov_cnt));
+    g_free(iov_orig);
+    iov_free(iov, iov_cnt);
+}
+
 static void test_discard_back(void)
 {
     struct iovec *iov;
@@ -404,6 +491,82 @@ static void test_discard_back(void)
     iov_free(iov, iov_cnt);
 }
 
+static void test_discard_back_undo(void)
+{
+    IOVDiscardUndo undo;
+    struct iovec *iov;
+    struct iovec *iov_orig;
+    unsigned int iov_cnt;
+    unsigned int iov_cnt_tmp;
+    size_t size;
+
+    /* Discard zero bytes */
+    iov_random(&iov, &iov_cnt);
+    iov_orig = g_memdup(iov, sizeof(iov[0]) * iov_cnt);
+    iov_cnt_tmp = iov_cnt;
+    iov_discard_back_undoable(iov, &iov_cnt_tmp, 0, &undo);
+    iov_discard_undo(&undo);
+    assert(iov_equals(iov, iov_orig, iov_cnt));
+    g_free(iov_orig);
+    iov_free(iov, iov_cnt);
+
+    /* Discard more bytes than vector size */
+    iov_random(&iov, &iov_cnt);
+    iov_orig = g_memdup(iov, sizeof(iov[0]) * iov_cnt);
+    iov_cnt_tmp = iov_cnt;
+    size = iov_size(iov, iov_cnt);
+    iov_discard_back_undoable(iov, &iov_cnt_tmp, size + 1, &undo);
+    iov_discard_undo(&undo);
+    assert(iov_equals(iov, iov_orig, iov_cnt));
+    g_free(iov_orig);
+    iov_free(iov, iov_cnt);
+
+    /* Discard entire vector */
+    iov_random(&iov, &iov_cnt);
+    iov_orig = g_memdup(iov, sizeof(iov[0]) * iov_cnt);
+    iov_cnt_tmp = iov_cnt;
+    size = iov_size(iov, iov_cnt);
+    iov_discard_back_undoable(iov, &iov_cnt_tmp, size, &undo);
+    iov_discard_undo(&undo);
+    assert(iov_equals(iov, iov_orig, iov_cnt));
+    g_free(iov_orig);
+    iov_free(iov, iov_cnt);
+
+    /* Discard within last element */
+    iov_random(&iov, &iov_cnt);
+    iov_orig = g_memdup(iov, sizeof(iov[0]) * iov_cnt);
+    iov_cnt_tmp = iov_cnt;
+    size = g_test_rand_int_range(1, iov[iov_cnt - 1].iov_len);
+    iov_discard_back_undoable(iov, &iov_cnt_tmp, size, &undo);
+    iov_discard_undo(&undo);
+    assert(iov_equals(iov, iov_orig, iov_cnt));
+    g_free(iov_orig);
+    iov_free(iov, iov_cnt);
+
+    /* Discard entire last element */
+    iov_random(&iov, &iov_cnt);
+    iov_orig = g_memdup(iov, sizeof(iov[0]) * iov_cnt);
+    iov_cnt_tmp = iov_cnt;
+    size = iov[iov_cnt - 1].iov_len;
+    iov_discard_back_undoable(iov, &iov_cnt_tmp, size, &undo);
+    iov_discard_undo(&undo);
+    assert(iov_equals(iov, iov_orig, iov_cnt));
+    g_free(iov_orig);
+    iov_free(iov, iov_cnt);
+
+    /* Discard within second-to-last element */
+    iov_random(&iov, &iov_cnt);
+    iov_orig = g_memdup(iov, sizeof(iov[0]) * iov_cnt);
+    iov_cnt_tmp = iov_cnt;
+    size = iov[iov_cnt - 1].iov_len +
+           g_test_rand_int_range(1, iov[iov_cnt - 2].iov_len);
+    iov_discard_back_undoable(iov, &iov_cnt_tmp, size, &undo);
+    iov_discard_undo(&undo);
+    assert(iov_equals(iov, iov_orig, iov_cnt));
+    g_free(iov_orig);
+    iov_free(iov, iov_cnt);
+}
+
 int main(int argc, char **argv)
 {
     g_test_init(&argc, &argv, NULL);
@@ -412,5 +575,7 @@ int main(int argc, char **argv)
     g_test_add_func("/basic/iov/io", test_io);
     g_test_add_func("/basic/iov/discard-front", test_discard_front);
     g_test_add_func("/basic/iov/discard-back", test_discard_back);
+    g_test_add_func("/basic/iov/discard-front-undo", test_discard_front_undo);
+    g_test_add_func("/basic/iov/discard-back-undo", test_discard_back_undo);
     return g_test_run();
 }
diff --git a/util/iov.c b/util/iov.c
index 45ef3043ee..efcf04b445 100644
--- a/util/iov.c
+++ b/util/iov.c
@@ -636,14 +636,33 @@ void qemu_iovec_clone(QEMUIOVector *dest, const QEMUIOVector *src, void *buf)
     }
 }
 
-size_t iov_discard_front(struct iovec **iov, unsigned int *iov_cnt,
-                         size_t bytes)
+void iov_discard_undo(IOVDiscardUndo *undo)
+{
+    /* Restore original iovec if it was modified */
+    if (undo->modified_iov) {
+        *undo->modified_iov = undo->orig;
+    }
+}
+
+size_t iov_discard_front_undoable(struct iovec **iov,
+                                  unsigned int *iov_cnt,
+                                  size_t bytes,
+                                  IOVDiscardUndo *undo)
 {
     size_t total = 0;
     struct iovec *cur;
 
+    if (undo) {
+        undo->modified_iov = NULL;
+    }
+
     for (cur = *iov; *iov_cnt > 0; cur++) {
         if (cur->iov_len > bytes) {
+            if (undo) {
+                undo->modified_iov = cur;
+                undo->orig = *cur;
+            }
+
             cur->iov_base += bytes;
             cur->iov_len -= bytes;
             total += bytes;
@@ -659,12 +678,24 @@ size_t iov_discard_front(struct iovec **iov, unsigned int *iov_cnt,
     return total;
 }
 
-size_t iov_discard_back(struct iovec *iov, unsigned int *iov_cnt,
-                        size_t bytes)
+size_t iov_discard_front(struct iovec **iov, unsigned int *iov_cnt,
+                         size_t bytes)
+{
+    return iov_discard_front_undoable(iov, iov_cnt, bytes, NULL);
+}
+
+size_t iov_discard_back_undoable(struct iovec *iov,
+                                 unsigned int *iov_cnt,
+                                 size_t bytes,
+                                 IOVDiscardUndo *undo)
 {
     size_t total = 0;
     struct iovec *cur;
 
+    if (undo) {
+        undo->modified_iov = NULL;
+    }
+
     if (*iov_cnt == 0) {
         return 0;
     }
@@ -673,6 +704,11 @@ size_t iov_discard_back(struct iovec *iov, unsigned int *iov_cnt,
 
     while (*iov_cnt > 0) {
         if (cur->iov_len > bytes) {
+            if (undo) {
+                undo->modified_iov = cur;
+                undo->orig = *cur;
+            }
+
             cur->iov_len -= bytes;
             total += bytes;
             break;
@@ -687,6 +723,12 @@ size_t iov_discard_back(struct iovec *iov, unsigned int *iov_cnt,
     return total;
 }
 
+size_t iov_discard_back(struct iovec *iov, unsigned int *iov_cnt,
+                        size_t bytes)
+{
+    return iov_discard_back_undoable(iov, iov_cnt, bytes, NULL);
+}
+
 void qemu_iovec_discard_back(QEMUIOVector *qiov, size_t bytes)
 {
     size_t total;
-- 
2.26.2

Stefan Hajnoczi <stefanha@redhat.com> 于2020年8月12日周三 下午6:52写道：

> The iov_discard_front/back() operations are useful for parsing iovecs
> but they modify the array elements. If the original array is needed
> after parsing finishes there is currently no way to restore it.
>
> Although g_memdup() can be used before performing destructive
> iov_discard_front/back() operations, this is inefficient.
>
> Introduce iov_discard_undo() to restore the array to the state prior to
> an iov_discard_front/back() operation.
>
>

Seems there are some errors. See below



> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  include/qemu/iov.h |  23 +++++++
>  tests/test-iov.c   | 165 +++++++++++++++++++++++++++++++++++++++++++++
>  util/iov.c         |  50 ++++++++++++--
>  3 files changed, 234 insertions(+), 4 deletions(-)
>
> diff --git a/include/qemu/iov.h b/include/qemu/iov.h
> index bffc151282..b6b283a5e5 100644
> --- a/include/qemu/iov.h
> +++ b/include/qemu/iov.h
> @@ -130,6 +130,29 @@ size_t iov_discard_front(struct iovec **iov, unsigned
> int *iov_cnt,
>  size_t iov_discard_back(struct iovec *iov, unsigned int *iov_cnt,
>                          size_t bytes);
>
> +/* Information needed to undo an iov_discard_*() operation */
> +typedef struct {
> +    struct iovec *modified_iov;
> +    struct iovec orig;
> +} IOVDiscardUndo;
> +
> +/*
> + * Undo an iov_discard_front_undoable() or iov_discard_back_undoable()
> + * operation. If multiple operations are made then each one needs a
> separate
> + * IOVDiscardUndo and iov_discard_undo() must be called in the reverse
> order
> + * that the operations were made.
> + */
> +void iov_discard_undo(IOVDiscardUndo *undo);
> +
> +/*
> + * Undoable versions of iov_discard_front() and iov_discard_back(). Use
> + * iov_discard_undo() to reset to the state before the discard operations.
> + */
> +size_t iov_discard_front_undoable(struct iovec **iov, unsigned int
> *iov_cnt,
> +                                  size_t bytes, IOVDiscardUndo *undo);
> +size_t iov_discard_back_undoable(struct iovec *iov, unsigned int *iov_cnt,
> +                                 size_t bytes, IOVDiscardUndo *undo);
> +
>  typedef struct QEMUIOVector {
>      struct iovec *iov;
>      int niov;
> diff --git a/tests/test-iov.c b/tests/test-iov.c
> index 458ca25099..9c415e2f1f 100644
> --- a/tests/test-iov.c
> +++ b/tests/test-iov.c
> @@ -26,6 +26,12 @@ static void iov_free(struct iovec *iov, unsigned niov)
>      g_free(iov);
>  }
>
> +static bool iov_equals(const struct iovec *a, const struct iovec *b,
> +                       unsigned niov)
> +{
> +    return memcmp(a, b, sizeof(a[0]) * niov) == 0;
> +}
> +
>  static void test_iov_bytes(struct iovec *iov, unsigned niov,
>                             size_t offset, size_t bytes)
>  {
> @@ -335,6 +341,87 @@ static void test_discard_front(void)
>      iov_free(iov, iov_cnt);
>  }
>
> +static void test_discard_front_undo(void)
> +{
> +    IOVDiscardUndo undo;
> +    struct iovec *iov;
> +    struct iovec *iov_tmp;
> +    struct iovec *iov_orig;
> +    unsigned int iov_cnt;
> +    unsigned int iov_cnt_tmp;
> +    size_t size;
> +
> +    /* Discard zero bytes */
> +    iov_random(&iov, &iov_cnt);
> +    iov_orig = g_memdup(iov, sizeof(iov[0]) * iov_cnt);
> +    iov_tmp = iov;
> +    iov_cnt_tmp = iov_cnt;
> +    iov_discard_front_undoable(&iov_tmp, &iov_cnt_tmp, 0, &undo);
> +    iov_discard_undo(&undo);
> +    assert(iov_equals(iov, iov_orig, iov_cnt));
> +    g_free(iov_orig);
> +    iov_free(iov, iov_cnt);
> +
> +    /* Discard more bytes than vector size */
> +    iov_random(&iov, &iov_cnt);
> +    iov_orig = g_memdup(iov, sizeof(iov[0]) * iov_cnt);
> +    iov_tmp = iov;
> +    iov_cnt_tmp = iov_cnt;
> +    size = iov_size(iov, iov_cnt);
> +    iov_discard_front_undoable(&iov_tmp, &iov_cnt_tmp, size + 1, &undo);
> +    iov_discard_undo(&undo);
> +    assert(iov_equals(iov, iov_orig, iov_cnt));
>

The 'iov_discard_front_undoable' will change the 'iov_tmp' it will not
touch 'iov_orig'.
So the test will be always passed. The actually function will not be tested.

Also, maybe we could abstract a function to do these discard test?


> +    g_free(iov_orig);
> +    iov_free(iov, iov_cnt);
> +
> +    /* Discard entire vector */
> +    iov_random(&iov, &iov_cnt);
> +    iov_orig = g_memdup(iov, sizeof(iov[0]) * iov_cnt);
> +    iov_tmp = iov;
> +    iov_cnt_tmp = iov_cnt;
> +    size = iov_size(iov, iov_cnt);
> +    iov_discard_front_undoable(&iov_tmp, &iov_cnt_tmp, size, &undo);
> +    iov_discard_undo(&undo);
> +    assert(iov_equals(iov, iov_orig, iov_cnt));
> +    g_free(iov_orig);
> +    iov_free(iov, iov_cnt);
> +
> +    /* Discard within first element */
> +    iov_random(&iov, &iov_cnt);
> +    iov_orig = g_memdup(iov, sizeof(iov[0]) * iov_cnt);
> +    iov_tmp = iov;
> +    iov_cnt_tmp = iov_cnt;
> +    size = g_test_rand_int_range(1, iov->iov_len);
> +    iov_discard_front_undoable(&iov_tmp, &iov_cnt_tmp, size, &undo);
> +    iov_discard_undo(&undo);
> +    assert(iov_equals(iov, iov_orig, iov_cnt));
> +    g_free(iov_orig);
> +    iov_free(iov, iov_cnt);
> +
> +    /* Discard entire first element */
> +    iov_random(&iov, &iov_cnt);
> +    iov_orig = g_memdup(iov, sizeof(iov[0]) * iov_cnt);
> +    iov_tmp = iov;
> +    iov_cnt_tmp = iov_cnt;
> +    iov_discard_front_undoable(&iov_tmp, &iov_cnt_tmp, iov->iov_len,
> &undo);
> +    iov_discard_undo(&undo);
> +    assert(iov_equals(iov, iov_orig, iov_cnt));
> +    g_free(iov_orig);
> +    iov_free(iov, iov_cnt);
> +
> +    /* Discard within second element */
> +    iov_random(&iov, &iov_cnt);
> +    iov_orig = g_memdup(iov, sizeof(iov[0]) * iov_cnt);
> +    iov_tmp = iov;
> +    iov_cnt_tmp = iov_cnt;
> +    size = iov->iov_len + g_test_rand_int_range(1, iov[1].iov_len);
> +    iov_discard_front_undoable(&iov_tmp, &iov_cnt_tmp, size, &undo);
> +    iov_discard_undo(&undo);
> +    assert(iov_equals(iov, iov_orig, iov_cnt));
> +    g_free(iov_orig);
> +    iov_free(iov, iov_cnt);
> +}
> +
>  static void test_discard_back(void)
>  {
>      struct iovec *iov;
> @@ -404,6 +491,82 @@ static void test_discard_back(void)
>      iov_free(iov, iov_cnt);
>  }
>
> +static void test_discard_back_undo(void)
> +{
> +    IOVDiscardUndo undo;
> +    struct iovec *iov;
> +    struct iovec *iov_orig;
> +    unsigned int iov_cnt;
> +    unsigned int iov_cnt_tmp;
> +    size_t size;
> +
> +    /* Discard zero bytes */
> +    iov_random(&iov, &iov_cnt);
> +    iov_orig = g_memdup(iov, sizeof(iov[0]) * iov_cnt);
> +    iov_cnt_tmp = iov_cnt;
> +    iov_discard_back_undoable(iov, &iov_cnt_tmp, 0, &undo);
> +    iov_discard_undo(&undo);
> +    assert(iov_equals(iov, iov_orig, iov_cnt));
> +    g_free(iov_orig);
> +    iov_free(iov, iov_cnt);
> +
> +    /* Discard more bytes than vector size */
> +    iov_random(&iov, &iov_cnt);
> +    iov_orig = g_memdup(iov, sizeof(iov[0]) * iov_cnt);
> +    iov_cnt_tmp = iov_cnt;
> +    size = iov_size(iov, iov_cnt);
> +    iov_discard_back_undoable(iov, &iov_cnt_tmp, size + 1, &undo);
> +    iov_discard_undo(&undo);
> +    assert(iov_equals(iov, iov_orig, iov_cnt));
> +    g_free(iov_orig);
> +    iov_free(iov, iov_cnt);
> +
> +    /* Discard entire vector */
> +    iov_random(&iov, &iov_cnt);
> +    iov_orig = g_memdup(iov, sizeof(iov[0]) * iov_cnt);
> +    iov_cnt_tmp = iov_cnt;
> +    size = iov_size(iov, iov_cnt);
> +    iov_discard_back_undoable(iov, &iov_cnt_tmp, size, &undo);
> +    iov_discard_undo(&undo);
> +    assert(iov_equals(iov, iov_orig, iov_cnt));
> +    g_free(iov_orig);
> +    iov_free(iov, iov_cnt);
> +
> +    /* Discard within last element */
> +    iov_random(&iov, &iov_cnt);
> +    iov_orig = g_memdup(iov, sizeof(iov[0]) * iov_cnt);
> +    iov_cnt_tmp = iov_cnt;
> +    size = g_test_rand_int_range(1, iov[iov_cnt - 1].iov_len);
> +    iov_discard_back_undoable(iov, &iov_cnt_tmp, size, &undo);
> +    iov_discard_undo(&undo);
> +    assert(iov_equals(iov, iov_orig, iov_cnt));
> +    g_free(iov_orig);
> +    iov_free(iov, iov_cnt);
> +
> +    /* Discard entire last element */
> +    iov_random(&iov, &iov_cnt);
> +    iov_orig = g_memdup(iov, sizeof(iov[0]) * iov_cnt);
> +    iov_cnt_tmp = iov_cnt;
> +    size = iov[iov_cnt - 1].iov_len;
> +    iov_discard_back_undoable(iov, &iov_cnt_tmp, size, &undo);
> +    iov_discard_undo(&undo);
> +    assert(iov_equals(iov, iov_orig, iov_cnt));
> +    g_free(iov_orig);
> +    iov_free(iov, iov_cnt);
> +
> +    /* Discard within second-to-last element */
> +    iov_random(&iov, &iov_cnt);
> +    iov_orig = g_memdup(iov, sizeof(iov[0]) * iov_cnt);
> +    iov_cnt_tmp = iov_cnt;
> +    size = iov[iov_cnt - 1].iov_len +
> +           g_test_rand_int_range(1, iov[iov_cnt - 2].iov_len);
> +    iov_discard_back_undoable(iov, &iov_cnt_tmp, size, &undo);
> +    iov_discard_undo(&undo);
> +    assert(iov_equals(iov, iov_orig, iov_cnt));
> +    g_free(iov_orig);
> +    iov_free(iov, iov_cnt);
> +}
> +
>  int main(int argc, char **argv)
>  {
>      g_test_init(&argc, &argv, NULL);
> @@ -412,5 +575,7 @@ int main(int argc, char **argv)
>      g_test_add_func("/basic/iov/io", test_io);
>      g_test_add_func("/basic/iov/discard-front", test_discard_front);
>      g_test_add_func("/basic/iov/discard-back", test_discard_back);
> +    g_test_add_func("/basic/iov/discard-front-undo",
> test_discard_front_undo);
> +    g_test_add_func("/basic/iov/discard-back-undo",
> test_discard_back_undo);
>      return g_test_run();
>  }
> diff --git a/util/iov.c b/util/iov.c
> index 45ef3043ee..efcf04b445 100644
> --- a/util/iov.c
> +++ b/util/iov.c
> @@ -636,14 +636,33 @@ void qemu_iovec_clone(QEMUIOVector *dest, const
> QEMUIOVector *src, void *buf)
>      }
>  }
>
> -size_t iov_discard_front(struct iovec **iov, unsigned int *iov_cnt,
> -                         size_t bytes)
> +void iov_discard_undo(IOVDiscardUndo *undo)
> +{
> +    /* Restore original iovec if it was modified */
> +    if (undo->modified_iov) {
> +        *undo->modified_iov = undo->orig;
> +    }
> +}
> +
> +size_t iov_discard_front_undoable(struct iovec **iov,
> +                                  unsigned int *iov_cnt,
> +                                  size_t bytes,
> +                                  IOVDiscardUndo *undo)
>  {
>      size_t total = 0;
>      struct iovec *cur;
>
> +    if (undo) {
> +        undo->modified_iov = NULL;
> +    }
> +
>      for (cur = *iov; *iov_cnt > 0; cur++) {
>          if (cur->iov_len > bytes) {
> +            if (undo) {
> +                undo->modified_iov = cur;
> +                undo->orig = *cur;
> +            }
> +
>

Why here we remember the 'cur'? 'cur' is the some of the 'iov'.
Maybe we remember the 'iov'. I think we need the undo structure like this:

struct IOVUndo {
    iov **modified_iov;
    iov *orig;
};

Then we can remeber the origin 'iov'.

Thanks,
Li Qiang



>              cur->iov_base += bytes;
>              cur->iov_len -= bytes;
>              total += bytes;
> @@ -659,12 +678,24 @@ size_t iov_discard_front(struct iovec **iov,
> unsigned int *iov_cnt,
>      return total;
>  }
>
> -size_t iov_discard_back(struct iovec *iov, unsigned int *iov_cnt,
> -                        size_t bytes)
> +size_t iov_discard_front(struct iovec **iov, unsigned int *iov_cnt,
> +                         size_t bytes)
> +{
> +    return iov_discard_front_undoable(iov, iov_cnt, bytes, NULL);
> +}
> +
> +size_t iov_discard_back_undoable(struct iovec *iov,
> +                                 unsigned int *iov_cnt,
> +                                 size_t bytes,
> +                                 IOVDiscardUndo *undo)
>  {
>      size_t total = 0;
>      struct iovec *cur;
>
> +    if (undo) {
> +        undo->modified_iov = NULL;
> +    }
> +
>      if (*iov_cnt == 0) {
>          return 0;
>      }
> @@ -673,6 +704,11 @@ size_t iov_discard_back(struct iovec *iov, unsigned
> int *iov_cnt,
>
>      while (*iov_cnt > 0) {
>          if (cur->iov_len > bytes) {
> +            if (undo) {
> +                undo->modified_iov = cur;
> +                undo->orig = *cur;
> +            }
> +
>              cur->iov_len -= bytes;
>              total += bytes;
>              break;
> @@ -687,6 +723,12 @@ size_t iov_discard_back(struct iovec *iov, unsigned
> int *iov_cnt,
>      return total;
>  }
>
> +size_t iov_discard_back(struct iovec *iov, unsigned int *iov_cnt,
> +                        size_t bytes)
> +{
> +    return iov_discard_back_undoable(iov, iov_cnt, bytes, NULL);
> +}
> +
>  void qemu_iovec_discard_back(QEMUIOVector *qiov, size_t bytes)
>  {
>      size_t total;
> --
> 2.26.2
>
>

On Sun, Aug 16, 2020 at 04:26:45PM +0800, Li Qiang wrote:
> Stefan Hajnoczi <stefanha@redhat.com> 于2020年8月12日周三 下午6:52写道：

Thanks for your review!

> > +    /* Discard more bytes than vector size */
> > +    iov_random(&iov, &iov_cnt);
> > +    iov_orig = g_memdup(iov, sizeof(iov[0]) * iov_cnt);
> > +    iov_tmp = iov;
> > +    iov_cnt_tmp = iov_cnt;
> > +    size = iov_size(iov, iov_cnt);
> > +    iov_discard_front_undoable(&iov_tmp, &iov_cnt_tmp, size + 1, &undo);
> > +    iov_discard_undo(&undo);
> > +    assert(iov_equals(iov, iov_orig, iov_cnt));
> >
> 
> The 'iov_discard_front_undoable' will change the 'iov_tmp' it will not
> touch 'iov_orig'.
> So the test will be always passed. The actually function will not be tested.

The test verifies that the iovec elements are restored to their previous
state by iov_discard_undo().

I think you are saying you'd like iov_discard_undo() to reset the
iov_tmp pointer? Currently that is not how the API works. The caller is
assumed to have the original pointer (e.g. virtio-blk has
req->elem.in/out_sg) and therefore it is not necessary to reset iov_tmp.

> Also, maybe we could abstract a function to do these discard test?

The structure of the test cases is similar but they vary in different
places. I'm not sure if this can be abstracted nicely.

> > diff --git a/util/iov.c b/util/iov.c
> > index 45ef3043ee..efcf04b445 100644
> > --- a/util/iov.c
> > +++ b/util/iov.c
> > @@ -636,14 +636,33 @@ void qemu_iovec_clone(QEMUIOVector *dest, const
> > QEMUIOVector *src, void *buf)
> >      }
> >  }
> >
> > -size_t iov_discard_front(struct iovec **iov, unsigned int *iov_cnt,
> > -                         size_t bytes)
> > +void iov_discard_undo(IOVDiscardUndo *undo)
> > +{
> > +    /* Restore original iovec if it was modified */
> > +    if (undo->modified_iov) {
> > +        *undo->modified_iov = undo->orig;
> > +    }
> > +}
> > +
> > +size_t iov_discard_front_undoable(struct iovec **iov,
> > +                                  unsigned int *iov_cnt,
> > +                                  size_t bytes,
> > +                                  IOVDiscardUndo *undo)
> >  {
> >      size_t total = 0;
> >      struct iovec *cur;
> >
> > +    if (undo) {
> > +        undo->modified_iov = NULL;
> > +    }
> > +
> >      for (cur = *iov; *iov_cnt > 0; cur++) {
> >          if (cur->iov_len > bytes) {
> > +            if (undo) {
> > +                undo->modified_iov = cur;
> > +                undo->orig = *cur;
> > +            }
> > +
> >
> 
> Why here we remember the 'cur'? 'cur' is the some of the 'iov'.
> Maybe we remember the 'iov'. I think we need the undo structure like this:
> 
> struct IOVUndo {
>     iov **modified_iov;
>     iov *orig;
> };
> 
> Then we can remeber the origin 'iov'.

Yes, this could be done but it's not needed (yet?). VirtQueueElement has
the original struct iovec pointers so adding this would be redundant.

Stefan Hajnoczi <stefanha@redhat.com> 于2020年9月16日周三 下午6:09写道：
>
> On Sun, Aug 16, 2020 at 04:26:45PM +0800, Li Qiang wrote:
> > Stefan Hajnoczi <stefanha@redhat.com> 于2020年8月12日周三 下午6:52写道：
>
> Thanks for your review!
>
> > > +    /* Discard more bytes than vector size */
> > > +    iov_random(&iov, &iov_cnt);
> > > +    iov_orig = g_memdup(iov, sizeof(iov[0]) * iov_cnt);
> > > +    iov_tmp = iov;
> > > +    iov_cnt_tmp = iov_cnt;
> > > +    size = iov_size(iov, iov_cnt);
> > > +    iov_discard_front_undoable(&iov_tmp, &iov_cnt_tmp, size + 1, &undo);
> > > +    iov_discard_undo(&undo);
> > > +    assert(iov_equals(iov, iov_orig, iov_cnt));
> > >
> >
> > The 'iov_discard_front_undoable' will change the 'iov_tmp' it will not
> > touch 'iov_orig'.
> > So the test will be always passed. The actually function will not be tested.
>
> The test verifies that the iovec elements are restored to their previous
> state by iov_discard_undo().
>
> I think you are saying you'd like iov_discard_undo() to reset the
> iov_tmp pointer? Currently that is not how the API works. The caller is
> assumed to have the original pointer (e.g. virtio-blk has
> req->elem.in/out_sg) and therefore it is not necessary to reset iov_tmp.
>

Hi Stefan,

You're right, I just have the idea in my mind the the 'discard'
function change the *iov, but the caller have the origin pointer.
So

Reviewed-by: Li Qiang <liq3ea@gmail.com>

> > Also, maybe we could abstract a function to do these discard test?
>
> The structure of the test cases is similar but they vary in different
> places. I'm not sure if this can be abstracted nicely.
>
> > > diff --git a/util/iov.c b/util/iov.c
> > > index 45ef3043ee..efcf04b445 100644
> > > --- a/util/iov.c
> > > +++ b/util/iov.c
> > > @@ -636,14 +636,33 @@ void qemu_iovec_clone(QEMUIOVector *dest, const
> > > QEMUIOVector *src, void *buf)
> > >      }
> > >  }
> > >
> > > -size_t iov_discard_front(struct iovec **iov, unsigned int *iov_cnt,
> > > -                         size_t bytes)
> > > +void iov_discard_undo(IOVDiscardUndo *undo)
> > > +{
> > > +    /* Restore original iovec if it was modified */
> > > +    if (undo->modified_iov) {
> > > +        *undo->modified_iov = undo->orig;
> > > +    }
> > > +}
> > > +
> > > +size_t iov_discard_front_undoable(struct iovec **iov,
> > > +                                  unsigned int *iov_cnt,
> > > +                                  size_t bytes,
> > > +                                  IOVDiscardUndo *undo)
> > >  {
> > >      size_t total = 0;
> > >      struct iovec *cur;
> > >
> > > +    if (undo) {
> > > +        undo->modified_iov = NULL;
> > > +    }
> > > +
> > >      for (cur = *iov; *iov_cnt > 0; cur++) {
> > >          if (cur->iov_len > bytes) {
> > > +            if (undo) {
> > > +                undo->modified_iov = cur;
> > > +                undo->orig = *cur;
> > > +            }
> > > +
> > >
> >
> > Why here we remember the 'cur'? 'cur' is the some of the 'iov'.
> > Maybe we remember the 'iov'. I think we need the undo structure like this:
> >
> > struct IOVUndo {
> >     iov **modified_iov;
> >     iov *orig;
> > };
> >
> > Then we can remeber the origin 'iov'.
>
> Yes, this could be done but it's not needed (yet?). VirtQueueElement has
> the original struct iovec pointers so adding this would be redundant.