qemu-block
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Stefan Hajnoczi
Subject: Re: [PATCH 1/3] util/iov: add iov_discard_undo()
Date: Wed, 16 Sep 2020 11:09:05 +0100

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: signature.asc
Description: PGP signature


reply via email to

[Prev in Thread] Current Thread [Next in Thread]