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

[Sean Bright]

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

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


Patch Set 4:

(2 comments)

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, enum ast_fd_flag_operation op,
> 1) Should this function be in main/astfd.c (not a statement a
 > question).

Looking at that file, that appears to be debug only (most of the file is #ifdef'd out if DEBUG_FDLEAKS isn't defined)

 > 2) Why not include the caller's function name?

Done.

 > 3) Can you use __ast_fd_set_flags to go with normal debug variant
 > naming?

This is not a debug function. I used the same convention as other functions in utils.h. If you feel strongly I don't have a problem adding another _. Let me know.


https://gerrit.asterisk.org/#/c/7471/1/main/utils.c@2808
PS1, Line 2808: 	if (f == -1) {
> Any reason you didn't use:
 > ast_log(__LOG_ERROR, file, func, line, "Failed to get ...",
 > strerror(errno))?

Nope. Changed.

 > 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.

Works for me. Updated.



-- 
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: 4
Gerrit-Owner: Sean Bright <sean.bright at gmail.com>
Gerrit-Reviewer: Corey Farrell <git at cfware.com>
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Sean Bright <sean.bright at gmail.com>
Gerrit-Comment-Date: Thu, 07 Dec 2017 19:52:31 +0000
Gerrit-HasComments: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20171207/1e007953/attachment-0001.html>