[CRIU] [PATCH] util: use F_DUPFD when we don't want to overwrite an existing descriptor

[Andrei Vagin]

Applied

On Tue, May 21, 2019 at 12:13:27AM -0700, Andrei Vagin wrote:
> Right now we use fcntl(F_GETFD) to check whether a target descriptor
> is used and then we call dup2(). Actually, we can do this for one system
> call.
> 
> Cc: Cyrill Gorcunov <gorcunov at openvz.org>
> Signed-off-by: Andrei Vagin <avagin at gmail.com>
> ---
>  criu/include/servicefd.h | 11 -----------
>  criu/servicefd.c         | 37 +++++++++++++++++++++++--------------
>  criu/util.c              | 18 +++++++++---------
>  3 files changed, 32 insertions(+), 34 deletions(-)
> 
> diff --git a/criu/include/servicefd.h b/criu/include/servicefd.h
> index 7be472cf4..986c46af5 100644
> --- a/criu/include/servicefd.h
> +++ b/criu/include/servicefd.h
> @@ -35,17 +35,6 @@ struct pstree_item;
>  extern bool sfds_protected;
>  
>  
> -#define sfd_verify_target(_type, _old_fd, _new_fd)			\
> -	({								\
> -		int __ret = 0;						\
> -		if (fcntl(_new_fd, F_GETFD) != -1 && errno != EBADF) {	\
> -			pr_err("%s busy target %d -> %d\n",		\
> -			       sfd_type_name(_type), _old_fd, _new_fd);	\
> -			__ret = -1;					\
> -		}							\
> -		__ret;							\
> -	})
> -
>  extern const char *sfd_type_name(enum sfd_type type);
>  extern int init_service_fd(void);
>  extern int get_service_fd(enum sfd_type type);
> diff --git a/criu/servicefd.c b/criu/servicefd.c
> index 82147921c..dc423895b 100644
> --- a/criu/servicefd.c
> +++ b/criu/servicefd.c
> @@ -153,6 +153,7 @@ static void sfds_protection_bug(enum sfd_type type)
>  int install_service_fd(enum sfd_type type, int fd)
>  {
>  	int sfd = __get_service_fd(type, service_fd_id);
> +	int tmp;
>  
>  	BUG_ON((int)type <= SERVICE_FD_MIN || (int)type >= SERVICE_FD_MAX);
>  	if (sfds_protected && !test_bit(type, sfd_map))
> @@ -166,16 +167,19 @@ int install_service_fd(enum sfd_type type, int fd)
>  		return fd;
>  	}
>  
> -	if (!test_bit(type, sfd_map)) {
> -		if (sfd_verify_target(type, fd, sfd))
> -			return -1;
> -	}
> -
> -	if (dup3(fd, sfd, O_CLOEXEC) != sfd) {
> +	if (!test_bit(type, sfd_map))
> +		tmp = fcntl(fd, F_DUPFD, sfd);
> +	else
> +		tmp = dup3(fd, sfd, O_CLOEXEC);
> +	if (tmp < 0) {
>  		pr_perror("%s dup %d -> %d failed",
>  			  sfd_type_name(type), fd, sfd);
>  		close(fd);
>  		return -1;
> +	} else if (tmp != sfd) {
> +		pr_err("%s busy target %d -> %d\n", sfd_type_name(type), fd, sfd);
> +		close(fd);
> +		return -1;
>  	}
>  
>  	set_bit(type, sfd_map);
> @@ -201,25 +205,30 @@ int close_service_fd(enum sfd_type type)
>  	return 0;
>  }
>  
> -static void move_service_fd(struct pstree_item *me, int type, int new_id, int new_base)
> +static int move_service_fd(struct pstree_item *me, int type, int new_id, int new_base)
>  {
>  	int old = get_service_fd(type);
>  	int new = new_base - type - SERVICE_FD_MAX * new_id;
>  	int ret;
>  
>  	if (old < 0)
> -		return;
> +		return 0;
>  
>  	if (!test_bit(type, sfd_map))
> -		sfd_verify_target(type, old, new);
> -
> -	ret = dup2(old, new);
> +		ret = fcntl(old, F_DUPFD, new);
> +	else
> +		ret = dup2(old, new);
>  	if (ret == -1) {
> -		if (errno != EBADF)
> -			pr_perror("%s unable to clone %d->%d",
> -				  sfd_type_name(type), old, new);
> +		pr_perror("%s unable to clone %d->%d",
> +			  sfd_type_name(type), old, new);
> +		return -1;
> +	} else if (ret != new) {
> +		pr_err("%s busy target %d -> %d\n", sfd_type_name(type), old, new);
> +		return -1;
>  	} else if (!(rsti(me)->clone_flags & CLONE_FILES))
>  		close(old);
> +
> +	return 0;
>  }
>  
>  static int choose_service_fd_base(struct pstree_item *me)
> diff --git a/criu/util.c b/criu/util.c
> index 41b6915b7..8faffb859 100644
> --- a/criu/util.c
> +++ b/criu/util.c
> @@ -229,19 +229,19 @@ int reopen_fd_as_safe(char *file, int line, int new_fd, int old_fd, bool allow_r
>  	int tmp;
>  
>  	if (old_fd != new_fd) {
> -		if (!allow_reuse_fd) {
> -			if (fcntl(new_fd, F_GETFD) != -1 || errno != EBADF) {
> -				pr_err("fd %d already in use (called at %s:%d)\n",
> -					new_fd, file, line);
> -				return -1;
> -			}
> -		}
> -
> -		tmp = dup2(old_fd, new_fd);
> +		if (!allow_reuse_fd)
> +			tmp = fcntl(old_fd, F_DUPFD, new_fd);
> +		else
> +			tmp = dup2(old_fd, new_fd);
>  		if (tmp < 0) {
>  			pr_perror("Dup %d -> %d failed (called at %s:%d)",
>  				  old_fd, new_fd, file, line);
>  			return tmp;
> +		} else if (tmp != new_fd) {
> +			close(tmp);
> +			pr_err("fd %d already in use (called at %s:%d)\n",
> +				new_fd, file, line);
> +			return -1;
>  		}
>  
>  		/* Just to have error message if failed */
> -- 
> 2.20.1
>