[Asterisk-code-review] app_voicemail: Refactor email generation functions (asterisk[master])

[N A]

Attention is currently required from: Sean Bright, Stanislav Abramenkov, George Joseph, Kevin Harwell, Benjamin Keith Ford.
N A has posted comments on this change. ( https://gerrit.asterisk.org/c/asterisk/+/16665 )

Change subject: app_voicemail: Refactor email generation functions
......................................................................


Patch Set 14:

(17 comments)

File include/asterisk/utils.h:

https://gerrit.asterisk.org/c/asterisk/+/16665/comment/ed2762c3_564383da 
PS11, Line 347: int ast_base64_encode_file(FILE *inputfile, FILE *outputfile, char *endl);
> Make endl const
Done


https://gerrit.asterisk.org/c/asterisk/+/16665/comment/39bc6858_60b846e6 
PS11, Line 355:  * \return zero on success, -1 on error.
> Change this to '\retval 0' vs return zero
I did change return to retval but the merge conflict screwed up some of my changes I think... trying to figure this out :)


https://gerrit.asterisk.org/c/asterisk/+/16665/comment/48a66764_99c130a7 
PS11, Line 357: int ast_base64_encode_file_path(const char *filename, FILE *outputfile, char *endl);
> make endl const
Done


File include/asterisk/utils.h:

https://gerrit.asterisk.org/c/asterisk/+/16665/comment/7778e998_ce659704 
PS10, Line 344:  * \return zero on success, -1 on error.
> Make this \retval instead of \return.
Done


https://gerrit.asterisk.org/c/asterisk/+/16665/comment/4d4b681b_ea7493dc 
PS10, Line 346: ast_file_base_encode
> Personally I think this should be renamed to ast_base64_encode_file. […]
Done


https://gerrit.asterisk.org/c/asterisk/+/16665/comment/39aa5052_076fc730 
PS10, Line 346: so
> rename this to output_file, or something more obvious that it's the file being written to.
Done


https://gerrit.asterisk.org/c/asterisk/+/16665/comment/cc305398_3856057c 
PS10, Line 346: char *endl
> Make this a const, and document the parameter
Done


https://gerrit.asterisk.org/c/asterisk/+/16665/comment/37d5455c_08784afd 
PS10, Line 346: int ast_file_base_encode(char *filename, FILE *so, char *endl);
> I agree with Sean though, in that since this is now a public API call this should probably accept tw […]
Done


https://gerrit.asterisk.org/c/asterisk/+/16665/comment/ed76f704_17ee4643 
PS10, Line 346: char *filename
> Make this a const
Done


https://gerrit.asterisk.org/c/asterisk/+/16665/comment/678e013d_cf75c55c 
PS10, Line 347: 
              : /*!
              :  * \brief same as mkstemp, but return a FILE
              :  * \param template The template for the unique file name to generate. Modified in place to return the file name.
              :  * \param mode The mode for file permissions
              :  * \param mask The mask for file permissions
              :  *
              :  * \return FILE handle to the temporary file on success or NULL if creation failed
              :  */
              : FILE *ast_file_mkftemp(char *template, mode_t mode, mode_t mask);
> I'm thinking this whole function should be moved to file. […]
Done


File main/utils.c:

https://gerrit.asterisk.org/c/asterisk/+/16665/comment/a65d382c_1c330323 
PS10, Line 592: 	if (bio->ateof)
              : 		return 0;
> I know this was from before, but since moving mind as well bring the code up to current guidelines. […]
Done


https://gerrit.asterisk.org/c/asterisk/+/16665/comment/c0a414e1_bfd8ed7f 
PS10, Line 614: 	if (bio->iocp>=bio->iolen) {
> add a space before and after >=
Done


https://gerrit.asterisk.org/c/asterisk/+/16665/comment/7c9bce4e_dd01d90e 
PS10, Line 615: 		if (!inbuf(bio, fi))
              : 			return EOF;
> braces here too
Done


https://gerrit.asterisk.org/c/asterisk/+/16665/comment/854bbdb6_58ae714c 
PS10, Line 644: /*!
              :  * \brief Performs a base 64 encode algorithm on the contents of a File
              :  * \param filename The path to the file to be encoded. Must be readable, file is opened in read mode.
              :  * \param so A FILE handle to the output file to receive the base 64 encoded contents of the input file, identified by filename.
              :  *
              :  * \return zero on success, -1 on error.
              :  */
> Remove this comment since it's documented publicly.
Done


https://gerrit.asterisk.org/c/asterisk/+/16665/comment/d42d9c30_f16aa02f 
PS10, Line 693: 				if (n < 2)
              : 					ogroup[2] = '=';
> Add braces to "if"
Done


https://gerrit.asterisk.org/c/asterisk/+/16665/comment/8cc97227_677c15ce 
PS10, Line 697: 			for (i = 0; i < 4; i++)
              : 				ochar(&bio, ogroup[i], so, endl);
> Add braces to "for"
Done


https://gerrit.asterisk.org/c/asterisk/+/16665/comment/7e39eaa8_7764a1ed 
PS10, Line 711: /* same as mkstemp, but return a FILE * */
> Remove comment
Done



-- 
To view, visit https://gerrit.asterisk.org/c/asterisk/+/16665
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Change-Id: I1de0ed3483623e9599711129edc817c45ad237ee
Gerrit-Change-Number: 16665
Gerrit-PatchSet: 14
Gerrit-Owner: N A <mail at interlinked.x10host.com>
Gerrit-Reviewer: Benjamin Keith Ford <bford at digium.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Joshua Colp <jcolp at sangoma.com>
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-Reviewer: Sean Bright <sean at seanbright.com>
Gerrit-CC: Stanislav Abramenkov <stas.abramenkov at gmail.com>
Gerrit-Attention: Sean Bright <sean at seanbright.com>
Gerrit-Attention: Stanislav Abramenkov <stas.abramenkov at gmail.com>
Gerrit-Attention: George Joseph <gjoseph at digium.com>
Gerrit-Attention: Kevin Harwell <kharwell at digium.com>
Gerrit-Attention: Benjamin Keith Ford <bford at digium.com>
Gerrit-Comment-Date: Wed, 17 Nov 2021 20:01:32 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Kevin Harwell <kharwell at digium.com>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20211117/b5e0b87f/attachment-0001.html>