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

[Corey Farrell]

Corey Farrell has posted comments on this change. ( https://gerrit.asterisk.org/7471 )

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


Patch Set 1:

(2 comments)

I wrote this before you updated the patch.  I haven't reviewed the bulk changes yet, couple thoughts/nits on the new function itself.

Nice patch BTW.  I always found fcntl to be awkward.

https://gerrit.asterisk.org/#/c/7471/1/main/utils.c
File main/utils.c:

https://gerrit.asterisk.org/#/c/7471/1/main/utils.c@2802
PS1, Line 2802: int _ast_fd_set_flags(int fd, int flags, int set, const char *file, int lineno)
1) Should this function be in main/astfd.c (not a statement a question).
2) Why not include the caller's function name?
3) Can you use __ast_fd_set_flags to go with normal debug variant naming?


https://gerrit.asterisk.org/#/c/7471/1/main/utils.c@2808
PS1, Line 2808: 		ast_log(LOG_ERROR, "%s:%d: Failed to get flags for file descriptor: %s\n",
Any reason you didn't use:
ast_log(__LOG_ERROR, file, func, line, "Failed to get ...", strerror(errno))?

Also (nit/ignore if you want): I think "Failed to get fcntl flags .." would improve the message - that would tell me immediately what the problem was.



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

Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-MessageType: comment
Gerrit-Change-Id: I8b81901e1b1bd537ca632567cdb408931c6eded7
Gerrit-Change-Number: 7471
Gerrit-PatchSet: 1
Gerrit-Owner: Sean Bright <sean.bright at gmail.com>
Gerrit-Reviewer: Corey Farrell <git at cfware.com>
Gerrit-Reviewer: Jenkins2
Gerrit-Comment-Date: Thu, 07 Dec 2017 17:05:50 +0000
Gerrit-HasComments: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20171207/94a198ec/attachment.html>