[dpdk-dev] [PATCH] reset src fd field to -1 in fdset_move of vhost

[dpdk-dev] [PATCH] reset src fd field to -1 in fdset_move of vhost

[Bing Zhao]

In the fdset_move, after copying the fd&rwfds from the src to the dst, the fd should be set to -1. Or else in some cases, there will be a fault missing. E.g:
Before: 1 -1 3 4 -1 6 7 -1 9 10
After: 1 10 3 4 9 6 7 -1 9 10
Then the index7 will be returned correctly for the first time, but if another fd is to be added, it will fail.

Signed-off-by: Bing Zhao <bing.zhao@hxt-semitech.com>
---
 lib/librte_vhost/fd_man.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/librte_vhost/fd_man.c b/lib/librte_vhost/fd_man.c
index 4c6fed418..48594dd7f 100644
--- a/lib/librte_vhost/fd_man.c
+++ b/lib/librte_vhost/fd_man.c
@@ -63,6 +63,7 @@ fdset_move(struct fdset *pfdset, int dst, int src)
 {
 	pfdset->fd[dst]    = pfdset->fd[src];
 	pfdset->rwfds[dst] = pfdset->rwfds[src];
+	pfdset->fd[src].fd = -1;
 }
 
 static void
-- 
2.11.0.windows.1

Re: [dpdk-dev] [PATCH] reset src fd field to -1 in fdset_move of vhost

[Yuanhan Liu]

On Thu, Dec 21, 2017 at 05:15:40PM +0800, Bing Zhao wrote:
> In the fdset_move, after copying the fd&rwfds from the src to the dst, the fd should be set to -1. Or else in some cases, there will be a fault missing. E.g:
> Before: 1 -1 3 4 -1 6 7 -1 9 10
> After: 1 10 3 4 9 6 7 -1 9 10
> Then the index7 will be returned correctly for the first time, but if another fd is to be added, it will fail.

Hi,

Have you met a real issue? I'm a bit doubt about that, since the fd array
is also guarded by "pfdset->num", which makes sure we will not access
those invalid entries (i.e. the last 2 entries in above example).

	--yliu

> Signed-off-by: Bing Zhao <bing.zhao@hxt-semitech.com>
> ---
>  lib/librte_vhost/fd_man.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/lib/librte_vhost/fd_man.c b/lib/librte_vhost/fd_man.c
> index 4c6fed418..48594dd7f 100644
> --- a/lib/librte_vhost/fd_man.c
> +++ b/lib/librte_vhost/fd_man.c
> @@ -63,6 +63,7 @@ fdset_move(struct fdset *pfdset, int dst, int src)
>  {
>  	pfdset->fd[dst]    = pfdset->fd[src];
>  	pfdset->rwfds[dst] = pfdset->rwfds[src];
> +	pfdset->fd[src].fd = -1;
>  }
>  
>  static void
> -- 
> 2.11.0.windows.1
> 

Re: [dpdk-dev] [PATCH] reset src fd field to -1 in fdset_move of vhost

[Zhao, Bing]

On 2018/1/19 22:37, Yuanhan Liu wrote:
> On Thu, Dec 21, 2017 at 05:15:40PM +0800, Bing Zhao wrote:
>> In the fdset_move, after copying the fd&rwfds from the src to the dst, the fd should be set to -1. Or else in some cases, there will be a fault missing. E.g:
>> Before: 1 -1 3 4 -1 6 7 -1 9 10
>> After: 1 10 3 4 9 6 7 -1 9 10
>> Then the index7 will be returned correctly for the first time, but if another fd is to be added, it will fail.
> 
> Hi,
> 
> Have you met a real issue? I'm a bit doubt about that, since the fd array
> is also guarded by "pfdset->num", which makes sure we will not access
> those invalid entries (i.e. the last 2 entries in above example).
> 
> 	--yliu
> 
>> Signed-off-by: Bing Zhao <bing.zhao@hxt-semitech.com>
>> ---
>>   lib/librte_vhost/fd_man.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/lib/librte_vhost/fd_man.c b/lib/librte_vhost/fd_man.c
>> index 4c6fed418..48594dd7f 100644
>> --- a/lib/librte_vhost/fd_man.c
>> +++ b/lib/librte_vhost/fd_man.c
>> @@ -63,6 +63,7 @@ fdset_move(struct fdset *pfdset, int dst, int src)
>>   {
>>   	pfdset->fd[dst]    = pfdset->fd[src];
>>   	pfdset->rwfds[dst] = pfdset->rwfds[src];
>> +	pfdset->fd[src].fd = -1;
>>   }
>>   
>>   static void
>> -- 
>> 2.11.0.windows.1
>>
Hello Yuanhan,
Thanks for your information. The answer is "no", and I just study the 
code and notice this. But yes you're right, I missed this. At first I 
thought there was a "-1" check in the "fdset_add_fd", indeed there isn't 
:). And no matter "-1" or other values in the "fd" element, if the "num" 
index points at that position, all the fields will be rewritten. So 
there is no problem.

Thanks again and please just ignore this.