Re: [Qemu-devel] [PATCH] net: Only flush queue or call sent callback on successful delivery

[Mark Wu]

On 10/28/2011 05:13 PM, Stefan Hajnoczi wrote:
> On Thu, Oct 27, 2011 at 10:02 AM, Mark Wu<address@hidden>  wrote:
> > Now queue flushing and sent callback could be invoked even on delivery
> > failure. We add a checking of receiver's return value to avoid this
> > case.
> >
> > Signed-off-by: Mark Wu<address@hidden>
> > ---
> >   net/queue.c |   12 +++++++-----
> >   1 files changed, 7 insertions(+), 5 deletions(-)
> What problem are you trying to fix?
>
> > @@ -251,7 +253,7 @@ void qemu_net_queue_flush(NetQueue *queue)
> >              break;
> >          }
> >
> > -        if (packet->sent_cb) {
> > +        if (ret>  0&&  packet->sent_cb) {
> >              packet->sent_cb(packet->sender, ret);
> This looks wrong.  ret is passed as an argument to the callback.  You
> are skipping the callback on error and not giving it a chance to see
> negative ret.
>
> Looking at virtio_net_tx_complete() this causes a virtqueue element leak.
Thanks for your review!
Yes, that's a problem. I thought only tap call queue send function with 
a callback (tap_send_completed) and confirmed that no memory leak in the 
case of tap. I agree that it will cause a
descriptor leak, but actually virtio_net_tx_complete doesn't check 
'ret'. It just pushes the elem to the virtqueue with 'async_tx.len' and 
flushes the tx queue. I think it assumes the callback is only called on 
success. Otherwise, it doesn't make sense for me. My point is that flush 
shouldn't happen on a deliver failure. Probably it will cause more 
failures. tap_send_completed assumes it's called on successfully deliver 
a packet too because it re-enables polling of tap fd.  That's why I add 
a checking of 'ret'.

I am not sure if the original code really needs a fix because it will 
not cause any visible problems.
> Stefan