On 11/27/19 3:36 AM, Yu, Jin wrote:
>> -----Original Message-----
>> From: Bruce Richardson <bruce.richard...@intel.com>
>> Sent: Tuesday, November 26, 2019 6:26 PM
>> To: Yu, Jin <jin...@intel.com>
>> Cc: Maxime Coquelin <maxime.coque...@redhat.com>; Bie, Tiwei
>> <tiwei....@intel.com>; Wang, Zhihong <zhihong.w...@intel.com>;
>> dev@dpdk.org; sta...@dpdk.org
>> Subject: Re: [dpdk-dev] [PATCH] examples/vhost_blk: fix the TOCTOU
>>
>> On Tue, Nov 26, 2019 at 11:32:14PM +0800, Jin Yu wrote:
>>> Fix the time of check time of use warning in example code
>>>
>>> Coverity issue: 350589 158663
>>> Fixes: c19beb3f38cd ("examples/vhost_blk: introduce vhost storage
>>> sample")
>>> Cc: sta...@dpdk.org
>>>
>>> Signed-off-by: Jin Yu <jin...@intel.com>
>>> ---
>>> examples/vhost_blk/vhost_blk.c | 9 ++-------
>>> 1 file changed, 2 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/examples/vhost_blk/vhost_blk.c
>>> b/examples/vhost_blk/vhost_blk.c index 3182a488b..bcb4ebb0b 100644
>>> --- a/examples/vhost_blk/vhost_blk.c
>>> +++ b/examples/vhost_blk/vhost_blk.c
>>> @@ -993,11 +993,7 @@ vhost_blk_ctrlr_construct(const char *ctrlr_name)
>>> }
>>> snprintf(dev_pathname, sizeof(dev_pathname), "%s/%s", path,
>>> ctrlr_name);
>>>
>>> - if (access(dev_pathname, F_OK) != -1) {
>>> - if (unlink(dev_pathname) != 0)
>>> - rte_exit(EXIT_FAILURE, "Cannot remove %s.\n",
>>> - dev_pathname);
>>> - }
>>> + unlink(dev_pathname);
>>>
>>
>> The original code did an exit if the delete failed, do you intend there to
>> be a
>> behaviour change here? You can probably get the same behaviour if you
>> check the errno on an unlink failure, e.g. ENOENT means file doesn't exist.
>>
>> If not having the app exit on unlink failure is reasonable behaviour then
>> ignore this comment.
>
> I considered it. I think it's ok to ignore the errno of unlink failure. This
> code just want
> to remove the file. There are two situations. The first one is that file
> doesn't exist the unlink
> fails and it's ok to ignore. The second one is that unlink fails to remove
> file but the next bind()
> would fail too so I think it's ok to ignore too.
That's fine by me, but please could you mention it in the commit
message?
>> Regards,
>> /Bruce
>
