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

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3020 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]