<p> Attention is currently required from: Sean Bright, N A, Stanislav Abramenkov, George Joseph, Benjamin Keith Ford. </p>
<p>Patch set 10:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4; color: #000000;">Code-Review -1</span></p><p><a href="https://gerrit.asterisk.org/c/asterisk/+/16665">View Change</a></p><p>14 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="null">File include/asterisk/utils.h:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/16665/comment/dac87ef5_0dfb3d03">Patch Set #10, Line 344:</a> <code style="font-family:monospace,monospace"> * \return zero on success, -1 on error.</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Make this \retval instead of \return.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/16665/comment/cc306544_6f35cdcc">Patch Set #10, Line 346:</a> <code style="font-family:monospace,monospace">ast_file_base_encode</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Personally I think this should be renamed to ast_base64_encode_file. At the very least use base64 specifically in the name.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/16665/comment/0831d5b8_7a3926d0">Patch Set #10, Line 346:</a> <code style="font-family:monospace,monospace">so</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">rename this to output_file, or something more obvious that it's the file being written to.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/16665/comment/91d8e25e_999a7d5f">Patch Set #10, Line 346:</a> <code style="font-family:monospace,monospace">char *endl</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Make this a const, and document the parameter</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/16665/comment/e044b531_e7d36781">Patch Set #10, Line 346:</a> <code style="font-family:monospace,monospace">int ast_file_base_encode(char *filename, FILE *so, char *endl);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">I agree with Sean though, in that since this is now a public API call this should probably accept two file pointers vs a path and pointer. As a utility function it's more useful that way.</p><p style="white-space: pre-wrap; word-wrap: break-word;">That said I'm not necessarily against a helper function that takes takes a path and opens it readonly that then calls the other one.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/16665/comment/84465a66_c510428e">Patch Set #10, Line 346:</a> <code style="font-family:monospace,monospace">char *filename</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Make this a const</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/16665/comment/da0053d3_5c5758d3">Patch Set #10, Line 347:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"><br>/*!<br> * \brief same as mkstemp, but return a FILE<br> * \param template The template for the unique file name to generate. Modified in place to return the file name.<br> * \param mode The mode for file permissions<br> * \param mask The mask for file permissions<br> *<br> * \return FILE handle to the temporary file on success or NULL if creation failed<br> */<br>FILE *ast_file_mkftemp(char *template, mode_t mode, mode_t mask);<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I'm thinking this whole function should be moved to file.h/c instead as that's where most of the file handling utilities are already.</p><p style="white-space: pre-wrap; word-wrap: break-word;">The base64 encoding function is more debatable imo, so can be left here.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="null">File main/utils.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/16665/comment/93751d33_8db424f4">Patch Set #10, Line 592:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> if (bio->ateof)<br> return 0;<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I know this was from before, but since moving mind as well bring the code up to current guidelines. That said add open/close braces to this "if"</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/16665/comment/09872f3b_5aa3f1b5">Patch Set #10, Line 614:</a> <code style="font-family:monospace,monospace"> if (bio->iocp>=bio->iolen) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">add a space before and after >=</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/16665/comment/622c3644_f3ab2977">Patch Set #10, Line 615:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> if (!inbuf(bio, fi))<br> return EOF;<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">braces here too</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/16665/comment/cc04495f_f4427635">Patch Set #10, Line 644:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;">/*!<br> * \brief Performs a base 64 encode algorithm on the contents of a File<br> * \param filename The path to the file to be encoded. Must be readable, file is opened in read mode.<br> * \param so A FILE handle to the output file to receive the base 64 encoded contents of the input file, identified by filename.<br> *<br> * \return zero on success, -1 on error.<br> */<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Remove this comment since it's documented publicly.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/16665/comment/fe40a2b8_d94ed0c3">Patch Set #10, Line 693:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> if (n < 2)<br> ogroup[2] = '=';<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Add braces to "if"</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/16665/comment/604d1efa_bee6d11c">Patch Set #10, Line 697:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> for (i = 0; i < 4; i++)<br> ochar(&bio, ogroup[i], so, endl);<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Add braces to "for"</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/16665/comment/971d5010_a38abdf3">Patch Set #10, Line 711:</a> <code style="font-family:monospace,monospace">/* same as mkstemp, but return a FILE * */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Remove comment</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/c/asterisk/+/16665">change 16665</a>. To unsubscribe, or for help writing mail filters, 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/c/asterisk/+/16665"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-Change-Id: I1de0ed3483623e9599711129edc817c45ad237ee </div>
<div style="display:none"> Gerrit-Change-Number: 16665 </div>
<div style="display:none"> Gerrit-PatchSet: 10 </div>
<div style="display:none"> Gerrit-Owner: N A <mail@interlinked.x10host.com> </div>
<div style="display:none"> Gerrit-Reviewer: Benjamin Keith Ford <bford@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Friendly Automation </div>
<div style="display:none"> Gerrit-Reviewer: George Joseph <gjoseph@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Joshua Colp <jcolp@sangoma.com> </div>
<div style="display:none"> Gerrit-Reviewer: Kevin Harwell <kharwell@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Sean Bright <sean@seanbright.com> </div>
<div style="display:none"> Gerrit-CC: Stanislav Abramenkov <stas.abramenkov@gmail.com> </div>
<div style="display:none"> Gerrit-Attention: Sean Bright <sean@seanbright.com> </div>
<div style="display:none"> Gerrit-Attention: N A <mail@interlinked.x10host.com> </div>
<div style="display:none"> Gerrit-Attention: Stanislav Abramenkov <stas.abramenkov@gmail.com> </div>
<div style="display:none"> Gerrit-Attention: George Joseph <gjoseph@digium.com> </div>
<div style="display:none"> Gerrit-Attention: Benjamin Keith Ford <bford@digium.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Wed, 17 Nov 2021 00:40:26 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: Yes </div>
<div style="display:none"> Gerrit-MessageType: comment </div>