Re: [LTP] [PATCH] lib: tst_detach_device_by_fd() set dev_fd to -1

[Cyril Hrubis <chrubis@suse.cz>]

Hi!
> > Apparently I tend to forget that tst_detach_device_by_fd() closes the
> > file descriptor. This change makes it more obvious by chaning the fd to
> > a pointer and also sets the dev_fd to -1, which matches the SAFE_CLOSE()
> > behavior.
> >
> 
> It took me a while to think why we need to do this since we already had:
> https://github.com/linux-test-project/ltp/commit/c02d8ddc5f9d312ca5c384317141e213412a5c42
> 
> However, this patch makes sense because:
> 
> After the tst_detach_device_by_fd() call, dev_fd is silently closed,
> but the variable in the caller still retains its old value. If the caller
> attempts to use it again, getting use-after-close may cause problems.
> Therefore, reset it to -1 to prevent accidental use of a closed fd.

Yes it's about making the API more obvious and less dangerous.

> So maybe we could explicitly add the above explanation in the description
> when merging?

I've changed the function description to:

/*
 * Detaches a file from a loop device by a fd.
 *
 * The dev_fd needs to be the last file descriptor opened for the
 * device. Call to this function will close dev_fd and set it to -1 in
 * order to avoid incorrect usage after it's closed.
 *
 * @dev_path Path to the loop device e.g. /dev/loop0
 * @dev_fd An open fd for the loop device, set to -1 after the completion.
 * @return Zero on succes, non-zero otherwise.
 */

I hope that it makes it clearer.

> Anyway:
> Reviewed-by: Li Wang <liwang@redhat.com>

Pushed, thanks for the review.

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp