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

[Mark Wu <wudxw@linux.vnet.ibm.com>]

On 10/28/2011 05:13 PM, Stefan Hajnoczi wrote:
> On Thu, Oct 27, 2011 at 10:02 AM, Mark Wu<wudxw@linux.vnet.ibm.com>  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<wudxw@linux.vnet.ibm.com>
>> ---
>>   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
>