Re: [PATCH 1/3] util/iov: add iov_discard_undo()

[Li Qiang]

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.