[Asterisk-code-review] utils: Add convenience function for setting fd flags (asterisk[13])
Sean Bright
asteriskteam at digium.com
Thu Dec 7 10:42:10 CST 2017
Sean Bright has uploaded this change for review. ( https://gerrit.asterisk.org/7471
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/chan_sip.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/http.c
M main/manager.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
19 files changed, 83 insertions(+), 103 deletions(-)
git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/71/7471/1
diff --git a/apps/app_ices.c b/apps/app_ices.c
index d0fdf5c..a043d3d 100644
--- a/apps/app_ices.c
+++ b/apps/app_ices.c
@@ -115,7 +115,6 @@
int fds[2];
int ms = -1;
int pid = -1;
- int flags;
struct ast_format *oreadformat;
struct ast_frame *f;
char filename[256]="";
@@ -130,8 +129,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 b418b28..67b1e12 100644
--- a/channels/chan_phone.c
+++ b/channels/chan_phone.c
@@ -1193,7 +1193,6 @@
{
/* Make a phone_pvt structure for this interface */
struct phone_pvt *tmp;
- int flags;
tmp = ast_calloc(1, sizeof(*tmp));
if (tmp) {
@@ -1226,8 +1225,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/chan_sip.c b/channels/chan_sip.c
index 559e5c0..b8cc7bf 100644
--- a/channels/chan_sip.c
+++ b/channels/chan_sip.c
@@ -2948,14 +2948,7 @@
goto cleanup;
}
- if ((flags = fcntl(tcptls_session->fd, F_GETFL)) == -1) {
- ast_log(LOG_ERROR, "error setting socket to non blocking mode, fcntl() failed: %s\n", strerror(errno));
- goto cleanup;
- }
-
- flags |= O_NONBLOCK;
- if (fcntl(tcptls_session->fd, F_SETFL, flags) == -1) {
- ast_log(LOG_ERROR, "error setting socket to non blocking mode, fcntl() failed: %s\n", strerror(errno));
+ if (ast_fd_set_flags(tcptls_session->fd, O_NONBLOCK)) {
goto cleanup;
}
diff --git a/channels/vgrabbers.c b/channels/vgrabbers.c
index 45dced4..3deb32f 100644
--- a/channels/vgrabbers.c
+++ b/channels/vgrabbers.c
@@ -227,12 +227,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 f207b64..edf5038 100644
--- a/codecs/codec_dahdi.c
+++ b/codecs/codec_dahdi.c
@@ -615,7 +615,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";
@@ -661,11 +660,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 03705b3..7d386b3 100644
--- a/include/asterisk/utils.h
+++ b/include/asterisk/utils.h
@@ -1142,4 +1142,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 0abb360..297915d 100644
--- a/main/asterisk.c
+++ b/main/asterisk.c
@@ -1691,7 +1691,6 @@
int s;
socklen_t len;
int x;
- int flags;
struct pollfd fds[1];
for (;;) {
if (ast_socket < 0)
@@ -1729,8 +1728,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/http.c b/main/http.c
index d1a443a..e8d395b 100644
--- a/main/http.c
+++ b/main/http.c
@@ -1955,9 +1955,7 @@
}
/* make sure socket is non-blocking */
- flags = fcntl(ser->fd, F_GETFL);
- flags |= O_NONBLOCK;
- fcntl(ser->fd, F_SETFL, flags);
+ ast_fd_set_flags(ser->fd, O_NONBLOCK);
/* Setup HTTP worker private data to keep track of request body reading. */
ao2_cleanup(ser->private_data);
diff --git a/main/manager.c b/main/manager.c
index 5bc87d5..8105265 100644
--- a/main/manager.c
+++ b/main/manager.c
@@ -6684,9 +6684,7 @@
}
/* make sure socket is non-blocking */
- flags = fcntl(ser->fd, F_GETFL);
- flags |= O_NONBLOCK;
- fcntl(ser->fd, F_SETFL, flags);
+ ast_fd_set_flags(ser->fd, O_NONBLOCK);
ao2_lock(session);
/* Hook to the tail of the event queue */
diff --git a/main/tcptls.c b/main/tcptls.c
index ef22094..3c2d678 100644
--- a/main/tcptls.c
+++ b/main/tcptls.c
@@ -805,7 +805,7 @@
pthread_t launched;
for (;;) {
- int i, flags;
+ int i;
if (desc->periodic_fn) {
desc->periodic_fn(desc);
@@ -843,8 +843,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->fd = fd;
tcptls_session->parent = desc;
ast_sockaddr_copy(&tcptls_session->remote_address, &addr);
@@ -1061,7 +1060,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;
@@ -1075,8 +1073,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;
@@ -1164,7 +1161,6 @@
void ast_tcptls_server_start(struct ast_tcptls_session_args *desc)
{
- int flags;
int x = 1;
int tls_changed = 0;
@@ -1267,8 +1263,7 @@
ast_log(LOG_ERROR, "Unable to listen for %s!\n", desc->name);
goto error;
}
- 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 a568cd1..44e9eb9 100644
--- a/main/udptl.c
+++ b/main/udptl.c
@@ -1014,7 +1014,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) {
@@ -1045,8 +1044,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 0342030..705b965 100644
--- a/main/utils.c
+++ b/main/utils.c
@@ -2798,3 +2798,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 4caa13b..f19303f 100644
--- a/res/res_agi.c
+++ b/res/res_agi.c
@@ -2048,7 +2048,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;
@@ -2078,14 +2078,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;
}
@@ -2251,9 +2244,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 75a6eba..a65fc8a 100644
--- a/res/res_http_websocket.c
+++ b/res/res_http_websocket.c
@@ -445,19 +445,7 @@
int AST_OPTIONAL_API_NAME(ast_websocket_set_nonblock)(struct ast_websocket *session)
{
- int flags;
-
- if ((flags = fcntl(session->fd, F_GETFL)) == -1) {
- return -1;
- }
-
- flags |= O_NONBLOCK;
-
- if ((flags = fcntl(session->fd, F_SETFL, flags)) == -1) {
- return -1;
- }
-
- return 0;
+ return ast_fd_set_flags(session->fd, O_NONBLOCK);
}
int AST_OPTIONAL_API_NAME(ast_websocket_set_timeout)(struct ast_websocket *session, int timeout)
@@ -944,17 +932,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 d791516..dee9c2d 100644
--- a/res/res_musiconhold.c
+++ b/res/res_musiconhold.c
@@ -924,7 +924,6 @@
static struct mohdata *mohalloc(struct mohclass *cl)
{
struct mohdata *moh;
- long flags;
if (!(moh = ast_calloc(1, sizeof(*moh))))
return NULL;
@@ -936,10 +935,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 037e533..6a3ab78 100644
--- a/res/res_pktccops.c
+++ b/res/res_pktccops.c
@@ -676,8 +676,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 5271d1b..c921f36 100644
--- a/res/res_rtp_asterisk.c
+++ b/res/res_rtp_asterisk.c
@@ -2765,8 +2765,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 1e76720..0654059 100644
--- a/res/res_timing_pthread.c
+++ b/res/res_timing_pthread.c
@@ -132,9 +132,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/7471
To unsubscribe, visit https://gerrit.asterisk.org/settings
Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-MessageType: newchange
Gerrit-Change-Id: I8b81901e1b1bd537ca632567cdb408931c6eded7
Gerrit-Change-Number: 7471
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/7a96ddb1/attachment-0001.html>
More information about the asterisk-code-review
mailing list