util: use F_DUPFD when we don't want to overwrite an existing descriptor

Submitted by Andrei Vagin on May 21, 2019, 7:13 a.m.

Details

Message ID 20190521071327.8387-1-avagin@gmail.com
State Accepted
Series "util: use F_DUPFD when we don't want to overwrite an existing descriptor"
Commit aad606a988b7c3996b36741c0da7753f97f524ab
Headers show

Commit Message

Andrei Vagin May 21, 2019, 7:13 a.m.
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@openvz.org>
Signed-off-by: Andrei Vagin <avagin@gmail.com>
---
 criu/include/servicefd.h | 11 -----------
 criu/servicefd.c         | 37 +++++++++++++++++++++++--------------
 criu/util.c              | 18 +++++++++---------
 3 files changed, 32 insertions(+), 34 deletions(-)

Patch hide | download patch | download mbox

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 */

Comments

Cyrill Gorcunov May 21, 2019, 7:29 a.m.
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@openvz.org>
> Signed-off-by: Andrei Vagin <avagin@gmail.com>
Reviewed-by: Cyrill Gorcunov <gorcunov@gmail.com>
Andrei Vagin May 25, 2019, 5:48 a.m.
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@openvz.org>
> Signed-off-by: Andrei Vagin <avagin@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
>