[Asterisk-code-review] utils: Add convenience function for setting fd flags (asterisk[master])

Sean Bright asteriskteam at digium.com
Thu Dec 7 10:53:38 CST 2017


Sean Bright has uploaded this change for review. ( https://gerrit.asterisk.org/7473


Change subject: utils: Add convenience function for setting fd flags
......................................................................

utils: Add convenience function for setting fd flags

There are many places in the code base where we ignore the return value
of fcntl() when getting/setting file descriptior flags. This patch
introduces a convenience function that allows setting or clearing file
descriptor flags and will also log an error on failure for later
analysis.

Change-Id: I8b81901e1b1bd537ca632567cdb408931c6eded7
---
M apps/app_ices.c
M channels/chan_phone.c
M channels/vgrabbers.c
M codecs/codec_dahdi.c
M include/asterisk/utils.h
M main/alertpipe.c
M main/asterisk.c
M main/tcptls.c
M main/udptl.c
M main/utils.c
M res/res_agi.c
M res/res_http_websocket.c
M res/res_musiconhold.c
M res/res_pktccops.c
M res/res_rtp_asterisk.c
M res/res_timing_pthread.c
16 files changed, 80 insertions(+), 78 deletions(-)



  git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/73/7473/1

diff --git a/apps/app_ices.c b/apps/app_ices.c
index 4ca4b67..1194384 100644
--- a/apps/app_ices.c
+++ b/apps/app_ices.c
@@ -113,7 +113,6 @@
 	int fds[2];
 	int ms = -1;
 	int pid = -1;
-	int flags;
 	struct ast_format *oreadformat;
 	struct ast_frame *f;
 	char filename[256]="";
@@ -128,8 +127,7 @@
 		ast_log(LOG_WARNING, "Unable to create pipe\n");
 		return -1;
 	}
-	flags = fcntl(fds[1], F_GETFL);
-	fcntl(fds[1], F_SETFL, flags | O_NONBLOCK);
+	ast_fd_set_flags(fds[1], O_NONBLOCK);
 	
 	ast_stopstream(chan);
 
diff --git a/channels/chan_phone.c b/channels/chan_phone.c
index 76891ac..832f28b 100644
--- a/channels/chan_phone.c
+++ b/channels/chan_phone.c
@@ -1191,7 +1191,6 @@
 {
 	/* Make a phone_pvt structure for this interface */
 	struct phone_pvt *tmp;
-	int flags;	
 	
 	tmp = ast_calloc(1, sizeof(*tmp));
 	if (tmp) {
@@ -1224,8 +1223,7 @@
 		ioctl(tmp->fd, PHONE_VAD, tmp->silencesupression);
 #endif
 		tmp->mode = mode;
-		flags = fcntl(tmp->fd, F_GETFL);
-		fcntl(tmp->fd, F_SETFL, flags | O_NONBLOCK);
+		ast_fd_set_flags(tmp->fd, O_NONBLOCK);
 		tmp->owner = NULL;
 		ao2_cleanup(tmp->lastformat);
 		tmp->lastformat = NULL;
diff --git a/channels/vgrabbers.c b/channels/vgrabbers.c
index 2581740..169e59c 100644
--- a/channels/vgrabbers.c
+++ b/channels/vgrabbers.c
@@ -226,12 +226,8 @@
 	v->b = *geom;
 	b = &v->b;	/* shorthand */
 
-	i = fcntl(fd, F_GETFL);
-	if (-1 == fcntl(fd, F_SETFL, i | O_NONBLOCK)) {
-		/* non fatal, just emit a warning */
-		ast_log(LOG_WARNING, "error F_SETFL for %s [%s]\n",
-			dev, strerror(errno));
-	}
+	ast_fd_set_flags(fd, O_NONBLOCK);
+
 	/* set format for the camera.
 	 * In principle we could retry with a different format if the
 	 * one we are asking for is not supported.
diff --git a/codecs/codec_dahdi.c b/codecs/codec_dahdi.c
index efb0168..941bb1f 100644
--- a/codecs/codec_dahdi.c
+++ b/codecs/codec_dahdi.c
@@ -613,7 +613,6 @@
 	/* Request translation through zap if possible */
 	int fd;
 	struct codec_dahdi_pvt *dahdip = pvt->pvt;
-	int flags;
 	int tried_once = 0;
 	const char *dev_filename = "/dev/dahdi/transcode";
 
@@ -659,11 +658,7 @@
 		return -1;
 	}
 
-	flags = fcntl(fd, F_GETFL);
-	if (flags > - 1) {
-		if (fcntl(fd, F_SETFL, flags | O_NONBLOCK))
-			ast_log(LOG_WARNING, "Could not set non-block mode!\n");
-	}
+	ast_fd_set_flags(fd, O_NONBLOCK);
 
 	dahdip->fd = fd;
 
diff --git a/include/asterisk/utils.h b/include/asterisk/utils.h
index 0a12b1d..ae9475a 100644
--- a/include/asterisk/utils.h
+++ b/include/asterisk/utils.h
@@ -1141,4 +1141,38 @@
  */
 int ast_check_ipv6(void);
 
+/*
+ * \brief Set flags on the given file descriptor
+ * \since 13.19
+ *
+ * If getting or setting flags of the given file descriptor fails, logs an
+ * error message.
+ *
+ * \param fd File descriptor to set flags on
+ * \param flags The flag(s) to set
+ *
+ * \return -1 on error
+ * \return 0 if successful
+ */
+#define ast_fd_set_flags(fd, flags) \
+	_ast_fd_set_flags((fd), (flags), 1, __FILE__, __LINE__)
+
+/*
+ * \brief Clear flags on the given file descriptor
+ * \since 13.19
+ *
+ * If getting or setting flags of the given file descriptor fails, logs an
+ * error message.
+ *
+ * \param fd File descriptor to clear flags on
+ * \param flags The flag(s) to clear
+ *
+ * \return -1 on error
+ * \return 0 if successful
+ */
+#define ast_fd_clear_flags(fd, flags) \
+	_ast_fd_set_flags((fd), (flags), 0, __FILE__, __LINE__)
+
+int _ast_fd_set_flags(int fd, int flags, int set, const char *file, int lineno);
+
 #endif /* _ASTERISK_UTILS_H */
diff --git a/main/alertpipe.c b/main/alertpipe.c
index fa6ec7b..7932a73 100644
--- a/main/alertpipe.c
+++ b/main/alertpipe.c
@@ -55,17 +55,8 @@
 		ast_log(LOG_WARNING, "Failed to create alert pipe: %s\n", strerror(errno));
 		return -1;
 	} else {
-		int flags = fcntl(alert_pipe[0], F_GETFL);
-		if (fcntl(alert_pipe[0], F_SETFL, flags | O_NONBLOCK) < 0) {
-			ast_log(LOG_WARNING, "Failed to set non-blocking mode on alert pipe: %s\n",
-				strerror(errno));
-			ast_alertpipe_close(alert_pipe);
-			return -1;
-		}
-		flags = fcntl(alert_pipe[1], F_GETFL);
-		if (fcntl(alert_pipe[1], F_SETFL, flags | O_NONBLOCK) < 0) {
-			ast_log(LOG_WARNING, "Failed to set non-blocking mode on alert pipe: %s\n",
-				strerror(errno));
+		if (ast_fd_set_flags(alert_pipe[0], O_NONBLOCK)
+		   || ast_fd_set_flags(alert_pipe[1], O_NONBLOCK)) {
 			ast_alertpipe_close(alert_pipe);
 			return -1;
 		}
diff --git a/main/asterisk.c b/main/asterisk.c
index dd66867..ccd606a 100644
--- a/main/asterisk.c
+++ b/main/asterisk.c
@@ -1569,7 +1569,6 @@
 	int s;
 	socklen_t len;
 	int x;
-	int flags;
 	struct pollfd fds[1];
 	for (;;) {
 		if (ast_socket < 0)
@@ -1607,8 +1606,7 @@
 						close(s);
 						break;
 					}
-					flags = fcntl(consoles[x].p[1], F_GETFL);
-					fcntl(consoles[x].p[1], F_SETFL, flags | O_NONBLOCK);
+					ast_fd_set_flags(consoles[x].p[1], O_NONBLOCK);
 					consoles[x].mute = 1; /* Default is muted, we will un-mute if necessary */
 					/* Default uid and gid to -2, so then in cli.c/cli_has_permissions() we will be able
 					   to know if the user didn't send the credentials. */
diff --git a/main/tcptls.c b/main/tcptls.c
index a6d0538..6b5dcd4 100644
--- a/main/tcptls.c
+++ b/main/tcptls.c
@@ -223,7 +223,7 @@
 	pthread_t launched;
 
 	for (;;) {
-		int i, flags;
+		int i;
 
 		if (desc->periodic_fn) {
 			desc->periodic_fn(desc);
@@ -261,8 +261,7 @@
 			close(fd);
 			continue;
 		}
-		flags = fcntl(fd, F_GETFL);
-		fcntl(fd, F_SETFL, flags & ~O_NONBLOCK);
+		ast_fd_clear_flags(fd, O_NONBLOCK);
 
 		tcptls_session->stream = ast_iostream_from_fd(&fd);
 		if (!tcptls_session->stream) {
@@ -270,7 +269,6 @@
 			close(fd);
 			continue;
 		}
-
 		tcptls_session->parent = desc;
 		ast_sockaddr_copy(&tcptls_session->remote_address, &addr);
 
@@ -514,7 +512,6 @@
 struct ast_tcptls_session_instance *ast_tcptls_client_start(struct ast_tcptls_session_instance *tcptls_session)
 {
 	struct ast_tcptls_session_args *desc;
-	int flags;
 
 	if (!(desc = tcptls_session->parent)) {
 		goto client_start_error;
@@ -528,8 +525,7 @@
 		goto client_start_error;
 	}
 
-	flags = fcntl(desc->accept_fd, F_GETFL);
-	fcntl(desc->accept_fd, F_SETFL, flags & ~O_NONBLOCK);
+	ast_fd_clear_flags(desc->accept_fd, O_NONBLOCK);
 
 	if (desc->tls_cfg) {
 		desc->tls_cfg->enabled = 1;
@@ -621,7 +617,6 @@
 
 void ast_tcptls_server_start(struct ast_tcptls_session_args *desc)
 {
-	int flags;
 	int x = 1;
 	int tls_changed = 0;
 	int sd_socket;
@@ -740,8 +735,7 @@
 	}
 
 systemd_socket_activation:
-	flags = fcntl(desc->accept_fd, F_GETFL);
-	fcntl(desc->accept_fd, F_SETFL, flags | O_NONBLOCK);
+	ast_fd_set_flags(desc->accept_fd, O_NONBLOCK);
 	if (ast_pthread_create_background(&desc->master, NULL, desc->accept_fn, desc)) {
 		ast_log(LOG_ERROR, "Unable to launch thread for %s on %s: %s\n",
 			desc->name,
diff --git a/main/udptl.c b/main/udptl.c
index 853e43c..d982f6b 100644
--- a/main/udptl.c
+++ b/main/udptl.c
@@ -1012,7 +1012,6 @@
 	int x;
 	int startplace;
 	int i;
-	long int flags;
 	RAII_VAR(struct udptl_config *, cfg, ao2_global_obj_ref(globals), ao2_cleanup);
 
 	if (!cfg || !cfg->general) {
@@ -1043,8 +1042,7 @@
 		ast_log(LOG_WARNING, "Unable to allocate socket: %s\n", strerror(errno));
 		return NULL;
 	}
-	flags = fcntl(udptl->fd, F_GETFL);
-	fcntl(udptl->fd, F_SETFL, flags | O_NONBLOCK);
+	ast_fd_set_flags(udptl->fd, O_NONBLOCK);
 
 #ifdef SO_NO_CHECK
 	if (cfg->general->nochecksums)
diff --git a/main/utils.c b/main/utils.c
index c553207..abf2a38 100644
--- a/main/utils.c
+++ b/main/utils.c
@@ -2749,3 +2749,24 @@
 	}
 	return extra[0] - extra[1];
 }
+
+int _ast_fd_set_flags(int fd, int flags, int set, const char *file, int lineno)
+{
+	int f;
+
+	f = fcntl(fd, F_GETFL);
+	if (f == -1) {
+		ast_log(LOG_ERROR, "%s:%d: Failed to get flags for file descriptor: %s\n",
+			file, lineno, strerror(errno));
+		return -1;
+	}
+
+	f = fcntl(fd, F_SETFL, set ? (f | flags) : (f & ~flags));
+	if (f == -1) {
+		ast_log(LOG_ERROR, "%s:%d: Failed to set flags for file descriptor: %s\n",
+			file, lineno, strerror(errno));
+		return -1;
+	}
+
+	return 0;
+}
diff --git a/res/res_agi.c b/res/res_agi.c
index 13af48f..85914c0 100644
--- a/res/res_agi.c
+++ b/res/res_agi.c
@@ -2046,7 +2046,7 @@
 	FastAGI defaults to port 4573 */
 static enum agi_result launch_netscript(char *agiurl, char *argv[], int *fds)
 {
-	int s = 0, flags;
+	int s = 0;
 	char *host, *script;
 	int num_addrs = 0, i = 0;
 	struct ast_sockaddr *addrs;
@@ -2076,14 +2076,7 @@
 			continue;
 		}
 
-		if ((flags = fcntl(s, F_GETFL)) < 0) {
-			ast_log(LOG_WARNING, "fcntl(F_GETFL) failed: %s\n", strerror(errno));
-			close(s);
-			continue;
-		}
-
-		if (fcntl(s, F_SETFL, flags | O_NONBLOCK) < 0) {
-			ast_log(LOG_WARNING, "fnctl(F_SETFL) failed: %s\n", strerror(errno));
+		if (ast_fd_set_flags(s, O_NONBLOCK)) {
 			close(s);
 			continue;
 		}
@@ -2249,9 +2242,8 @@
 			close(toast[1]);
 			return AGI_RESULT_FAILURE;
 		}
-		res = fcntl(audio[1], F_GETFL);
-		if (res > -1)
-			res = fcntl(audio[1], F_SETFL, res | O_NONBLOCK);
+
+		res = ast_fd_set_flags(audio[1], O_NONBLOCK);
 		if (res < 0) {
 			ast_log(LOG_WARNING, "unable to set audio pipe parameters: %s\n", strerror(errno));
 			close(fromast[0]);
diff --git a/res/res_http_websocket.c b/res/res_http_websocket.c
index c1f9a29..baaa40f 100644
--- a/res/res_http_websocket.c
+++ b/res/res_http_websocket.c
@@ -951,17 +951,11 @@
 /*! \brief Simple echo implementation which echoes received text and binary frames */
 static void websocket_echo_callback(struct ast_websocket *session, struct ast_variable *parameters, struct ast_variable *headers)
 {
-	int flags, res;
+	int res;
 
 	ast_debug(1, "Entering WebSocket echo loop\n");
 
-	if ((flags = fcntl(ast_websocket_fd(session), F_GETFL)) == -1) {
-		goto end;
-	}
-
-	flags |= O_NONBLOCK;
-
-	if (fcntl(ast_websocket_fd(session), F_SETFL, flags) == -1) {
+	if (ast_fd_set_flags(ast_websocket_fd(session), O_NONBLOCK)) {
 		goto end;
 	}
 
diff --git a/res/res_musiconhold.c b/res/res_musiconhold.c
index e4bb7a2..ef1b81c 100644
--- a/res/res_musiconhold.c
+++ b/res/res_musiconhold.c
@@ -922,7 +922,6 @@
 static struct mohdata *mohalloc(struct mohclass *cl)
 {
 	struct mohdata *moh;
-	long flags;
 
 	if (!(moh = ast_calloc(1, sizeof(*moh))))
 		return NULL;
@@ -934,10 +933,8 @@
 	}
 
 	/* Make entirely non-blocking */
-	flags = fcntl(moh->pipe[0], F_GETFL);
-	fcntl(moh->pipe[0], F_SETFL, flags | O_NONBLOCK);
-	flags = fcntl(moh->pipe[1], F_GETFL);
-	fcntl(moh->pipe[1], F_SETFL, flags | O_NONBLOCK);
+	ast_fd_set_flags(moh->pipe[0], O_NONBLOCK);
+	ast_fd_set_flags(moh->pipe[1], O_NONBLOCK);
 
 	moh->f.frametype = AST_FRAME_VOICE;
 	moh->f.subclass.format = cl->format;
diff --git a/res/res_pktccops.c b/res/res_pktccops.c
index e8d266c..156c49d 100644
--- a/res/res_pktccops.c
+++ b/res/res_pktccops.c
@@ -648,7 +648,7 @@
 
 static int cops_connect(char *host, char *port)
 {
-	int s, sfd = -1, flags;
+	int s, sfd = -1;
 	struct addrinfo hints;
 	struct addrinfo *rp;
 	struct addrinfo *result;
@@ -674,8 +674,7 @@
 		if (sfd == -1) {
 			ast_log(LOG_WARNING, "Failed socket\n");
 		}
-		flags = fcntl(sfd, F_GETFL);
-		fcntl(sfd, F_SETFL, flags | O_NONBLOCK);
+		ast_fd_set_flags(sfd, O_NONBLOCK);
 #ifdef HAVE_SO_NOSIGPIPE
 		setsockopt(sfd, SOL_SOCKET, SO_NOSIGPIPE, &trueval, sizeof(trueval));
 #endif
diff --git a/res/res_rtp_asterisk.c b/res/res_rtp_asterisk.c
index 5aeb791..16eb7dd 100644
--- a/res/res_rtp_asterisk.c
+++ b/res/res_rtp_asterisk.c
@@ -3048,8 +3048,7 @@
 		}
 		ast_log(LOG_WARNING, "Unable to allocate %s socket: %s\n", type, strerror(errno));
 	} else {
-		long flags = fcntl(sock, F_GETFL);
-		fcntl(sock, F_SETFL, flags | O_NONBLOCK);
+		ast_fd_set_flags(sock, O_NONBLOCK);
 #ifdef SO_NO_CHECK
 		if (nochecksums) {
 			setsockopt(sock, SOL_SOCKET, SO_NO_CHECK, &nochecksums, sizeof(nochecksums));
diff --git a/res/res_timing_pthread.c b/res/res_timing_pthread.c
index 09952f9..f520796 100644
--- a/res/res_timing_pthread.c
+++ b/res/res_timing_pthread.c
@@ -130,9 +130,7 @@
 	}
 
 	for (i = 0; i < ARRAY_LEN(timer->pipe); ++i) {
-		int flags = fcntl(timer->pipe[i], F_GETFL);
-		flags |= O_NONBLOCK;
-		fcntl(timer->pipe[i], F_SETFL, flags);
+		ast_fd_set_flags(timer->pipe[i], O_NONBLOCK);
 	}
 	
 	ao2_lock(pthread_timers);

-- 
To view, visit https://gerrit.asterisk.org/7473
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I8b81901e1b1bd537ca632567cdb408931c6eded7
Gerrit-Change-Number: 7473
Gerrit-PatchSet: 1
Gerrit-Owner: Sean Bright <sean.bright at gmail.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20171207/6dd8fd89/attachment-0001.html>


More information about the asterisk-code-review mailing list