The change looks right to me. As is, it looks like the code is skipping over descriptors when the intent should be to bounce data into a single descriptor.
I agree the variable rename should go in as a separate change. On Thu, Apr 18, 2024 at 6:56 AM Daniel P. Berrangé <berra...@redhat.com> wrote: > > On Sat, Jan 13, 2024 at 04:27:40AM +0300, Temir Zharaspayev wrote: > > When zero-copy usage of indirect descriptors buffer table isn't > > possible, library gather scattered memory chunks in a local copy. > > This commit fixes the issue with pointer arithmetic for the local copy > > buffer. > > > > Signed-off-by: Temir Zharaspayev <mass...@gmail.com> > > --- > > subprojects/libvhost-user/libvhost-user.c | 11 ++++++----- > > 1 file changed, 6 insertions(+), 5 deletions(-) > > > > diff --git a/subprojects/libvhost-user/libvhost-user.c > > b/subprojects/libvhost-user/libvhost-user.c > > index 6684057370..e952c098a3 100644 > > --- a/subprojects/libvhost-user/libvhost-user.c > > +++ b/subprojects/libvhost-user/libvhost-user.c > > @@ -2307,7 +2307,7 @@ static int > > virtqueue_read_indirect_desc(VuDev *dev, struct vring_desc *desc, > > uint64_t addr, size_t len) > > { > > - struct vring_desc *ori_desc; > > + uint8_t *src_cursor, *dst_cursor; > > uint64_t read_len; > > > > if (len > (VIRTQUEUE_MAX_SIZE * sizeof(struct vring_desc))) { > > @@ -2318,17 +2318,18 @@ virtqueue_read_indirect_desc(VuDev *dev, struct > > vring_desc *desc, > > return -1; > > } > > > > + dst_cursor = (uint8_t *) desc; Nit - no space on cast > > while (len) { > > read_len = len; > > - ori_desc = vu_gpa_to_va(dev, &read_len, addr); > > - if (!ori_desc) { > > + src_cursor = vu_gpa_to_va(dev, &read_len, addr); > > + if (!src_cursor) { > > return -1; > > } > > > > - memcpy(desc, ori_desc, read_len); > > + memcpy(dst_cursor, src_cursor, read_len); > > len -= read_len; > > addr += read_len; > > - desc += read_len; > > + dst_cursor += read_len; > > The ori_desc -> src_cursor changes don't look to have any functional > effect. Having that change present obscures the functional part of > the patch, which is this line. FWIW, it is generally preferrable to > not mix functional and non-functional changes in the same patch > > It now interprets 'read_len' as the number of bytes to increment the > address by, rather than incrementing by the number of elements of > size 'sizeof(struct vring_desc)'. > > I don't know enough about this area of QEMU code to say which > semantics were desired, so I'll defer to the Michael as maintainer > to give a formal review. > > > With regards, > Daniel > -- > |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| > |: https://libvirt.org -o- https://fstop138.berrange.com :| > |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| > >