On 11/27/19 3:36 AM, Yu, Jin wrote: >> -----Original Message----- >> From: Bruce Richardson <[email protected]> >> Sent: Tuesday, November 26, 2019 6:26 PM >> To: Yu, Jin <[email protected]> >> Cc: Maxime Coquelin <[email protected]>; Bie, Tiwei >> <[email protected]>; Wang, Zhihong <[email protected]>; >> [email protected]; [email protected] >> 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: [email protected] >>> >>> Signed-off-by: Jin Yu <[email protected]> >>> --- >>> 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 >
