<p>Sean Bright <strong>posted comments</strong> on this change.</p><p><a href="https://gerrit.asterisk.org/7471">View Change</a></p><p>Patch set 4:</p><p>(2 comments)</p><ul style="list-style: none; padding-left: 20px;"><li><p><a href="https://gerrit.asterisk.org/#/c/7471/1/main/utils.c">File main/utils.c:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/7471/1/main/utils.c@2802">Patch Set #1, Line 2802:</a> <code style="font-family:monospace,monospace">int _ast_fd_set_flags(int fd, int flags, enum ast_fd_flag_operation op,</code></p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><p style="white-space: pre-wrap; word-wrap: break-word;">1) Should this function be in main/astfd.c (not a statement a<br>question).</p></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">Looking at that file, that appears to be debug only (most of the file is #ifdef'd out if DEBUG_FDLEAKS isn't defined)</p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><p style="white-space: pre-wrap; word-wrap: break-word;">2) Why not include the caller's function name?</p></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">Done.</p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><p style="white-space: pre-wrap; word-wrap: break-word;">3) Can you use __ast_fd_set_flags to go with normal debug variant<br>naming?</p></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/7471/1/main/utils.c@2808">Patch Set #1, Line 2808:</a> <code style="font-family:monospace,monospace">        if (f == -1) {</code></p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><p style="white-space: pre-wrap; word-wrap: break-word;">Any reason you didn't use:<br>ast_log(__LOG_ERROR, file, func, line, "Failed to get ...",<br>strerror(errno))?</p></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">Nope. Changed.</p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><p style="white-space: pre-wrap; word-wrap: break-word;">Also (nit/ignore if you want): I think "Failed to get fcntl flags<br>.." would improve the message - that would tell me immediately what<br>the problem was.</p></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">Works for me. Updated.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/7471">change 7471</a>. To unsubscribe, visit <a href="https://gerrit.asterisk.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.asterisk.org/7471"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: 13 </div>
<div style="display:none"> Gerrit-MessageType: comment </div>
<div style="display:none"> Gerrit-Change-Id: I8b81901e1b1bd537ca632567cdb408931c6eded7 </div>
<div style="display:none"> Gerrit-Change-Number: 7471 </div>
<div style="display:none"> Gerrit-PatchSet: 4 </div>
<div style="display:none"> Gerrit-Owner: Sean Bright <sean.bright@gmail.com> </div>
<div style="display:none"> Gerrit-Reviewer: Corey Farrell <git@cfware.com> </div>
<div style="display:none"> Gerrit-Reviewer: George Joseph <gjoseph@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins2 </div>
<div style="display:none"> Gerrit-Reviewer: Sean Bright <sean.bright@gmail.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Thu, 07 Dec 2017 19:52:31 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>