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