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

Tue Nov 16 18:40:26 CST 2021

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

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

Patch Set 10: Code-Review-1

(14 comments)

File include/asterisk/utils.h:

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

https://gerrit.asterisk.org/c/asterisk/+/16665/comment/cc306544_6f35cdcc 
PS10, Line 346: ast_file_base_encode
Personally I think this should be renamed to ast_base64_encode_file. At the very least use base64 specifically in the name.

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

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

https://gerrit.asterisk.org/c/asterisk/+/16665/comment/e044b531_e7d36781 
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 two file pointers vs a path and pointer. As a utility function it's more useful that way.

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.

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

https://gerrit.asterisk.org/c/asterisk/+/16665/comment/da0053d3_5c5758d3 
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.h/c instead as that's where most of the file handling utilities are already.

The base64 encoding function is more debatable imo, so can be left here.

File main/utils.c:

https://gerrit.asterisk.org/c/asterisk/+/16665/comment/93751d33_8db424f4 
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. That said add open/close braces to this "if"

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

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

https://gerrit.asterisk.org/c/asterisk/+/16665/comment/cc04495f_f4427635 
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.

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

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

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

-- 
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: 10
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: N A <mail at interlinked.x10host.com>
Gerrit-Attention: Stanislav Abramenkov <stas.abramenkov at gmail.com>
Gerrit-Attention: George Joseph <gjoseph at digium.com>
Gerrit-Attention: Benjamin Keith Ford <bford at digium.com>
Gerrit-Comment-Date: Wed, 17 Nov 2021 00:40:26 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20211116/ecfe341b/attachment-0001.html>