Re: [PATCH RFC 08/15] hw/virtio/vhost-shadow-virtqueue: used handler

On Thu, Aug 6, 2026 at 3:28 AM Hanna Czenczek <[email protected]> wrote:
>
> On 06.08.26 04:50, Connor Kite wrote:
> > On Fri, Jul 24, 2026 at 4:29 AM Akihiko Odaki
> > <[email protected]> wrote:
> >>
> >> The error of the handler that may have left elem stale is ignored here.
> >>
> >> Regards,
> >> Akihiko Odaki
> > I have now added in a check that returns on error without setting the guest
> > notifier.  So essentially the call is ignored.  Maybe this could be expanded
> > on to handle errors more gracefully depending on the error code.
> >
> > On Tue, Jul 28, 2026 at 8:06 AM Stefan Hajnoczi <[email protected]> wrote:
> >>> +/**
> >>> + * Callback to handle a used buffer
> >>> + *
> >>> + * @svq: Shadow virtqueue
> >>> + * @elem: Element placed in the queue by the device
> >>> + * @vq_callback_opaque: Used to pass arguments to callback
> >>> + *
> >>> + * Returns 0 if the vq is running as expected
> >> The doc comment should mention what non-0 look like. Are errors reported
> >> as a negative errno?
> >>
> > Yes, a negative errno is expected on failure.  I have added clarification to the
> > comment.
> >
> > On Mon, Aug 3, 2026 at 5:42 AM Hanna Czenczek <[email protected]> wrote:
> >> On 24.07.26 00:30, Connor Kite wrote:
> >>> Adds a handler callback to shadow virtqueues in order to enable
> >>> non-default processing of used vring elements from the device.
> >>>
> >>> Signed-off-by: Connor Kite <[email protected]>
> >>> ---
> >>>    hw/virtio/vhost-shadow-virtqueue.c |  4 ++++
> >>>    hw/virtio/vhost-shadow-virtqueue.h | 14 ++++++++++++++
> >>>    2 files changed, 18 insertions(+)
> >> The avail handler replaces `vhost_svq_add_element()`. I find it a bit
> >> unintuitive that this handler does not replace `virtqueue_fill()`, i.e.
> >> that it is not a “mirror” of the avail handler; that the avail handler
> >> actually handles avail elements, whereas the used handler is “just” a
> >> callback on each used element. Was that intentional? (If so, I think the
> >> comment should be explicit about this choice.)
> >>
> >> Hanna
> >>
> > I implemented it as a more narrowly-focused callback since virtqueue_fill
> > needs to be called regardless.  If I instead called virtqueue_fill
> > from the handler,
> > I would need to pass in the elem index and length via svq->ops_opaque,
> > which is a bit awkward as vhost_user defines ops_opaque when registering
> > the callback.  That's certainly not impossible to do, but this way felt cleaner.
> > For now, I will add a comment explaining that the callback should not be used
> > to add elements to the underlying vq's used ring.
>
> Then I personally wouldn’t call it a “handler” if it does not handle the
> element like the avail handler does. Maybe “callback”.
>
> Hanna
>

I see how that's more descriptive.  Updating the name.