Stefan Hajnoczi <stefa...@redhat.com> 于2020年9月16日周三 下午6:09写道: > > On Sun, Aug 16, 2020 at 04:26:45PM +0800, Li Qiang wrote: > > Stefan Hajnoczi <stefa...@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 <liq...@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.