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

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