Re: [dpdk-dev] [PATCH v3 05/19] vhost: fix error handling when mem table gets updated

[Maxime Coquelin <maxime.coquelin@redhat.com>]

On 10/04/2018 04:59 PM, Ilya Maximets wrote:
> On 04.10.2018 11:13, Maxime Coquelin wrote:
>> When the memory table gets updated, the rings addresses need
>> to be translated again. If it fails, we need to exit cleanly
>> by unmapping memory regions.
>>
>> Fixes: d5022533c20a ("vhost: retranslate vring addr when memory table changes")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>> ---
> 
> Acked-by: Ilya Maximets <i.maximets@samsung.com>
> 
> Minor comments inline.
> 
>>   lib/librte_vhost/vhost_user.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
>> index 8ffe5aa66..b6eae8dc5 100644
>> --- a/lib/librte_vhost/vhost_user.c
>> +++ b/lib/librte_vhost/vhost_user.c
>> @@ -964,7 +964,8 @@ vhost_user_set_mem_table(struct virtio_net **pdev, struct VhostUserMsg *msg)
>>   
>>   			dev = translate_ring_addresses(dev, i);
>>   			if (!dev)
>> -				return VH_RESULT_ERR;
>> +				goto err_mmap;
>> +
> 
> 1. No need to have two empty lines. (You could fix this while applying)

Right. I will either fix it while applying, or it will be in next
revision, if any.

> 2. In current code, error on message handling will cause disconnect and
>     memory regions will be freed anyway. So, the change is not very
>     important for master (maybe just for consistency with surrounding
>     code) but it could be important for stable versions.

I may not disconnect directly, as if reply-ack feature is negotiated[0],
the slave would reply with a NACK and function handler will return 0.
I guess that in that case the master (QEMU) will disconnect anyway, but
this is just an assumption from slave point of view.

[0]: REPLY_ACK protocol feature is only advertised when IOMMU support is
requested because of a bug in an old upstream QEMU version.

Thanks!
Maxime

>>   
>>   			*pdev = dev;
>>   		}
>>