[Asterisk-code-review] res_hiredis: REDIS DeviceState and Dialplan functions (asterisk[master])

[N A]

Attention is currently required from: Sean Bright, Alec Davis.
N A has posted comments on this change. ( https://gerrit.asterisk.org/c/asterisk/+/18829 )

Change subject: res_hiredis: REDIS DeviceState and Dialplan functions
......................................................................


Patch Set 15: Code-Review-1

(13 comments)

Commit Message:

https://gerrit.asterisk.org/c/asterisk/+/18829/comment/9f72b48c_d65ae027 
PS14, Line 20:  same => n,Verbose(test_sentence=${REDIS(test_sentence)})
The commit message is not the right place for an example. Include this in the XML documentation instead, and make sure to use the <example> tags.


File include/asterisk/autoconfig.h.in:

https://gerrit.asterisk.org/c/asterisk/+/18829/comment/d415db72_587ab72b 
PS14, Line 29: /* Define to 1 if using 'alloca.c'. */
Did you mean to remove CRAY_STACKSEG_END? Or is this a possible rebase issue?


https://gerrit.asterisk.org/c/asterisk/+/18829/comment/5589d2ec_d7304cd3 
PS14, Line 504: 
Same for this, why is memory.h gone?


File res/res_hiredis.c:

https://gerrit.asterisk.org/c/asterisk/+/18829/comment/b96211ae_7cc39b68 
PS14, Line 49: 		<syntax argsep="">
Why is the argsep empty? If it's not relevant, then remove the argsep attribute.


https://gerrit.asterisk.org/c/asterisk/+/18829/comment/1c4dad48_00688c2f 
PS14, Line 50: 			<parameter name="valid redis syntax" required="true" />
I would use underscores instead of spaces, if only to adhere to how all other modules do it.


https://gerrit.asterisk.org/c/asterisk/+/18829/comment/5edf27b1_771ae18a 
PS14, Line 53: 			<para>This function will pass without validation any Redis syntax to a Redis database</para>
add a comma before and after "without validation"


https://gerrit.asterisk.org/c/asterisk/+/18829/comment/fba2ee85_4e2e21fc 
PS14, Line 79: 			If you wish to find out if an entry exists, use a REDIS_EXISTS
I think "If you wish..." should be a separate <para> for readability.


https://gerrit.asterisk.org/c/asterisk/+/18829/comment/65ee2198_53993ed4 
PS14, Line 121: 			<ref type="function">REDIS</ref>
I noticed you only have see also for the REDIS function for several of these functions.
Are the other functions not also possible relevant? If so, maybe include all the REDIS functions. That way, on a wiki page, a user can see all the other relevant REDIS functions available.


https://gerrit.asterisk.org/c/asterisk/+/18829/comment/3f85b012_e134f22b 
PS14, Line 152: 					<synopsis>Background Save on module unload</synopsis>
The synopsis for this option is a little unclear and could be expanded/clarified


https://gerrit.asterisk.org/c/asterisk/+/18829/comment/c3493e23_924e0da5 
PS14, Line 211: 
Extra line


https://gerrit.asterisk.org/c/asterisk/+/18829/comment/3dbb0651_8488b393 
PS14, Line 305: static int __attribute__((format(printf, 3, 0))) hiredis_command(char* buf, size_t len, const char* format, ...) 
redness


https://gerrit.asterisk.org/c/asterisk/+/18829/comment/52abcc3b_1f80d1a6 
PS14, Line 638:  * even if live_dangerously is disabled.
I thought DB writing requires live dangerously be enabled. Should this not be consistent with that?


https://gerrit.asterisk.org/c/asterisk/+/18829/comment/4bc5c5e4_6502f671 
PS14, Line 767: 	ast_debug(6, "  password = %s\n", cfg->general->password);
I think some debugs are fine but writing the password into the log on init might be a bit too much. I would at least remove that.



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

Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Change-Id: Ifdf4d33d2a2a5fd044fac13b201c7437de34ba6a
Gerrit-Change-Number: 18829
Gerrit-PatchSet: 15
Gerrit-Owner: Alec Davis <alec at bdt.co.nz>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: Joshua Colp <jcolp at sangoma.com>
Gerrit-Reviewer: N A <mail at interlinked.x10host.com>
Gerrit-Reviewer: Sean Bright <sean at seanbright.com>
Gerrit-Attention: Sean Bright <sean at seanbright.com>
Gerrit-Attention: Alec Davis <alec at bdt.co.nz>
Gerrit-Comment-Date: Fri, 29 Jul 2022 12:33:38 +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/20220729/8892df10/attachment.html>