[Asterisk-code-review] term: truncate the message rather than the escapes. (asterisk[master])

[Kevin Harwell]

Attention is currently required from: Joshua Colp, Jaco Kroon.
Kevin Harwell has posted comments on this change. ( https://gerrit.asterisk.org/c/asterisk/+/16640 )

Change subject: term: truncate the message rather than the escapes.
......................................................................


Patch Set 3: Code-Review-1

(4 comments)

Patchset:

PS3: 
I'm not sure this patch is even needed or should be allowed in at all based on deprecation policy? After looking at the current code it seems the 'term_color' function is deprecated and probably has been for a while. It's stated in the doxygen to use:

"...ast_term_color_code() or ast_term_color()"

instead. So perhaps the solution is to call update your code to call one of those functions?


File main/term.c:

https://gerrit.asterisk.org/c/asterisk/+/16640/comment/a22c004f_fa76e9ce 
PS3, Line 273: 
             : 	if (p < maxout)
             : 		return outbuf;
Per coding guidelines if statements need braces


https://gerrit.asterisk.org/c/asterisk/+/16640/comment/dc5f07e0_1d82db0b 
PS3, Line 277: 	if (ast_opt_force_black_background) {
             : 		/* bgcolor will be handled above already */
             : 		p = snprintf(outbuf, maxout, "%c[%d;%d;%dm", ESC, attr, fgcolor, bgcolor + 10);
             : 	} else {
             : 		p = snprintf(outbuf, maxout, "%c[%d;%dm", ESC, attr, fgcolor);
             : 	}
Why rewrite beginning part of outbuf with the same data? You should have all the values needed (p, maxout, tail_len) to just overwrite only the last part of outbuf.


https://gerrit.asterisk.org/c/asterisk/+/16640/comment/ff46b8d6_bc0aa342 
PS3, Line 284: 	if (!tail_out) {
             : 		tail_out = term_end();
             : 		tail_len = strlen(tail_out);
             : 	}
It's possible for the setting of these variables in a multi-threaded environment to result in undefined behavior. Adding a lock here just for this seems not great, so I'd recommend initializing them outside this function somehow.

Based on other comments you probably just need to "cache" tail_len though. I'd let the compiler optimize the term_end() call.



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

Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Change-Id: I80ef7a4bfd2947e090ef830143391d11baebdb0d
Gerrit-Change-Number: 16640
Gerrit-PatchSet: 3
Gerrit-Owner: Jaco Kroon <jaco at uls.co.za>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-CC: Joshua Colp <jcolp at sangoma.com>
Gerrit-Attention: Joshua Colp <jcolp at sangoma.com>
Gerrit-Attention: Jaco Kroon <jaco at uls.co.za>
Gerrit-Comment-Date: Wed, 17 Nov 2021 18:34:14 +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/20211117/f12ea7fb/attachment.html>