Stefan Hajnoczi <stefa...@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 <stefa...@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
>
>
