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

[N A]

Attention is currently required from: Joshua Colp.
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 2:

(6 comments)

Patchset:

PS1: 
> Yes, and there's additional comments.
Done


File include/asterisk/mail.h:

https://gerrit.asterisk.org/c/asterisk/+/16665/comment/e18a2cfd_7742257d 
PS1, Line 24: #define BASELINELEN 72
> What do these mean within the context of the base encoding?
Well, here is the code where it's used:
if (bio->linelength >= BASELINELEN) {
		if (fputs(ENDL, so) == EOF) {
			return -1;
		}

		bio->linelength = 0;
	}

My guess is that since 72 is the magic wrapping number in plain text email, this might have something to do with adding a line break after every 72 characters, or something like that.


https://gerrit.asterisk.org/c/asterisk/+/16665/comment/0a275320_7c354848 
PS1, Line 36: int ast_base_encode(char *filename, FILE *so);
> Needs a more explicit function name
Done


https://gerrit.asterisk.org/c/asterisk/+/16665/comment/d6cd72a8_36f30757 
PS1, Line 40:  * \param template The template for the unique file name to generate
> Should be documented that the filename is returned in here
Done


File main/mail.c:

https://gerrit.asterisk.org/c/asterisk/+/16665/comment/2ef15e38_c8e9e0ee 
PS1, Line 131: 	while (!hiteof){
> Can this reuse the existing base64 code at all?
Unfortunately, I think not, at least without a bit of rework. The existing code uses an entire string while the existing one goes through a file byte by byte. I'm not sure it would be prudent to try to load potentially a very large attachment all in at once, just to use the existing function. It's different enough that I think it serves its own purpose. Accordingly, I've named it ast_file_base_encode.


https://gerrit.asterisk.org/c/asterisk/+/16665/comment/c96dd442_e6960d9a 
PS1, Line 178: 	chmod(template, VOICEMAIL_FILE_MODE & ~my_umask);
> If this supports changing permissions then they should be passed in
Makes sense, I've done the same with the umask as well, so it shouldn't have any direct ties to voicemail at all now.



-- 
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: 2
Gerrit-Owner: N A <mail at interlinked.x10host.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: Joshua Colp <jcolp at sangoma.com>
Gerrit-Attention: Joshua Colp <jcolp at sangoma.com>
Gerrit-Comment-Date: Mon, 01 Nov 2021 18:11:10 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Joshua Colp <jcolp at sangoma.com>
Comment-In-Reply-To: N A <mail at interlinked.x10host.com>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20211101/ac367000/attachment.html>