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

[Alec Davis]

Attention is currently required from: Sean Bright, N A, Joshua Colp.
Alec Davis 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:

(15 comments)

Commit Message:

https://gerrit.asterisk.org/c/asterisk/+/18829/comment/1829eb6b_ac519136 
PS14, Line 20:  same => n,Verbose(test_sentence=${REDIS(test_sentence)})
> The commit message is not the right place for an example. […]
Ack


Patchset:

PS15: 
Thanks

Fixed most.


File configs/samples/hiredis.conf.sample:

https://gerrit.asterisk.org/c/asterisk/+/18829/comment/48f8a09a_901a14f1 
PS12, Line 3: ;server = 127.0.0.1             ; server[:port] of Redis server to use, default=127.0.0.1.
> Does this support IPv6 as well? If so, it should be stated
hiredis 0.12.0 (2015-01-22) added IPV6 support.

Debian buster hiredis 0.14.0-3, which is now considered old.


File include/asterisk/autoconfig.h.in:

https://gerrit.asterisk.org/c/asterisk/+/18829/comment/7c415cb4_9cc1d2c1 
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?
I now don't use alloca.c


https://gerrit.asterisk.org/c/asterisk/+/18829/comment/5497e04d_bb1e3bbb 
PS14, Line 504: 
> Same for this, why is memory. […]
probably alloca not used anymore


File res/res_hiredis.c:

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


https://gerrit.asterisk.org/c/asterisk/+/18829/comment/cfe2ecff_5a01a1a2 
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.
Done


https://gerrit.asterisk.org/c/asterisk/+/18829/comment/de6c53b5_91246860 
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"
Done


https://gerrit.asterisk.org/c/asterisk/+/18829/comment/0186c67f_501dd9d2 
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.
Done


https://gerrit.asterisk.org/c/asterisk/+/18829/comment/83263ac8_9cc64dbd 
PS14, Line 121: 			<ref type="function">REDIS</ref>
> I noticed you only have see also for the REDIS function for several of these functions. […]
Done


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


https://gerrit.asterisk.org/c/asterisk/+/18829/comment/32dc12e2_c7d39211 
PS14, Line 211: 
> Extra line
Done


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


https://gerrit.asterisk.org/c/asterisk/+/18829/comment/da37c149_b1c2fa44 
PS14, Line 638:  * even if live_dangerously is disabled.
> I thought DB writing requires live dangerously be enabled. […]
I copied func_db.c for these function, so will have to look deeper.


https://gerrit.asterisk.org/c/asterisk/+/18829/comment/516ce2cc_63964b83 
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. […]
agreed



-- 
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: N A <mail at interlinked.x10host.com>
Gerrit-Attention: Joshua Colp <jcolp at sangoma.com>
Gerrit-Comment-Date: Fri, 29 Jul 2022 13:16:21 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: N A <mail at interlinked.x10host.com>
Comment-In-Reply-To: Joshua Colp <jcolp at sangoma.com>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20220729/8aa43fa1/attachment-0001.html>