From mboxrd@z Thu Jan  1 00:00:00 1970
From: "Burakov, Anatoly" <anatoly.burakov@intel.com>
Subject: Re: [PATCH v5] vfio: change to use generic multi-process
	channel
Date: Tue, 20 Mar 2018 10:33:00 +0000
Message-ID: <2936e6e0-1ec3-d937-5e83-9938d76da814@intel.com>
References: <1515643654-129489-5-git-send-email-jianfeng.tan@intel.com>
 <1520175456-52990-1-git-send-email-jianfeng.tan@intel.com>
 <a2fa2595-a1c2-7aea-49ca-499415828541@intel.com>
 <ED26CBA2FAD1BF48A8719AEF02201E36514939CC@SHSMSX103.ccr.corp.intel.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit
Cc: "Richardson, Bruce" <bruce.richardson@intel.com>,
 "Ananyev, Konstantin" <konstantin.ananyev@intel.com>,
 "thomas@monjalon.net" <thomas@monjalon.net>
To: "Tan, Jianfeng" <jianfeng.tan@intel.com>, "dev@dpdk.org" <dev@dpdk.org>
Return-path: <dev-bounces@dpdk.org>
Received: from mga11.intel.com (mga11.intel.com [192.55.52.93])
 by dpdk.org (Postfix) with ESMTP id 7625044CF
 for <dev@dpdk.org>; Tue, 20 Mar 2018 11:33:03 +0100 (CET)
In-Reply-To: <ED26CBA2FAD1BF48A8719AEF02201E36514939CC@SHSMSX103.ccr.corp.intel.com>
Content-Language: en-US
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <https://dpdk.org/ml/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://dpdk.org/ml/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://dpdk.org/ml/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>

On 19-Mar-18 6:53 AM, Tan, Jianfeng wrote:
> Hi Anatoly,
> 
> Thank you for the review. All your comments will be addressed in next version, except for below concern which might be taken care of in another patch if it also concerns you.
> 
>> -----Original Message-----
>> From: Burakov, Anatoly
>> Sent: Wednesday, March 14, 2018 9:27 PM
>> To: Tan, Jianfeng; dev@dpdk.org
>> Cc: Richardson, Bruce; Ananyev, Konstantin; thomas@monjalon.net
>> Subject: Re: [PATCH v5] vfio: change to use generic multi-process channel
> [...]
>>
>>> +	mp_req.len_param = sizeof(*p);
>>> +	mp_req.num_fds = 0;
>>> +
>>> +	vfio_group_fd = -1;
>>> +	if (rte_mp_request(&mp_req, &mp_reply, &ts) == 0 &&
>>> +	    mp_reply.nb_received == 1) {
>>> +		mp_rep = &mp_reply.msgs[0];
>>> +		p = (struct vfio_mp_param *)mp_rep->param;
>>> +		if (p->result == SOCKET_OK && mp_rep->num_fds == 1) {
>>> +			cur_grp->group_no = iommu_group_no;
>>> +			vfio_group_fd = mp_rep->fds[0];
>>> +			cur_grp->fd = vfio_group_fd;
>>> +			vfio_cfg.vfio_active_groups++;
>>>    		}
>>> +		free(mp_reply.msgs);
>>>    	}
>>> -	return -1;
>>> +
>>> +	if (vfio_group_fd < 0)
>>> +		RTE_LOG(ERR, EAL, "  cannot request group fd\n");
>>> +	return vfio_group_fd;
>>
>> p->result can be SOCKET_NO_FD, in which case returned value should be
>> zero. I think this is missing from this code. There probably should be
>> an "else if (p->result == SOCKET_NO_FD)" clause that sets return value to 0.
>>
>> You should be able to test this by trying to set up a device for VFIO
>> that isn't bound to VFIO driver, in a secondary process.
> 
> OK, I will fix this.
> 
> But really, "zero" could be ambiguous as a fd could, theoretically, be zero too.

You're correct. Maybe return 0/-1 in case of success/failure and put fd 
into a pointer? i.e.

int func(int *vfio_group_fd) {
<...>
*vfio_group_fd = fd;
return 0;
}

> 
> Thanks,
> Jianfeng
> 


-- 
Thanks,
Anatoly