[Asterisk-code-review] DeviceState and Dialplan functions for Redis DB (asterisk[master])

[Joshua Colp]

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

Change subject: DeviceState and Dialplan functions for Redis DB
......................................................................


Patch Set 13: Code-Review-1

(17 comments)

Commit Message:

https://gerrit.asterisk.org/c/asterisk/+/18829/comment/c96d7d7d_0b1b432b 
PS12, Line 9: Simplify, Simplify.
The commit message is what will go into Asterisk, and what will show up in the changelog and such. It should reflect what is going into Asterisk and not the history of its development such as that it is now a single module.


File configs/samples/hiredis.conf.sample:

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


File res/res_hiredis.c:

https://gerrit.asterisk.org/c/asterisk/+/18829/comment/51694e89_1fc2ffa4 
PS12, Line 137: #define MAX_PREFIX 40
Document what this is for, same for STR_CONF_SZ


https://gerrit.asterisk.org/c/asterisk/+/18829/comment/c0945bc0_d68d591d 
PS12, Line 166: 	struct conf_global_options* global;
*global


https://gerrit.asterisk.org/c/asterisk/+/18829/comment/1ecb399e_140e46a5 
PS12, Line 204: 		ast_log(AST_LOG_ERROR, "Couldn't establish connection to %s:%d socket_fd[%d] Reason: %s\n", \
You can use ast_sockaddr_stringify_fmt to have the port automatically put in


https://gerrit.asterisk.org/c/asterisk/+/18829/comment/33d79c4b_be8f9ad9 
PS12, Line 213: 	ast_debug(6, "socket_fd[%d]\n", hiredis_context->fd);
I don't think this is useful


https://gerrit.asterisk.org/c/asterisk/+/18829/comment/de0d3523_67f80506 
PS12, Line 243: 	memcpy(data, hiredis_context, sizeof(redisContext));
I don't understand how this works. You're copying the contents of hiredis_context over, but in the next line you seemingly free it?


https://gerrit.asterisk.org/c/asterisk/+/18829/comment/5227417b_0c95fdc7 
PS12, Line 244: 	ast_free(hiredis_context);
Should this be using redisFree?


https://gerrit.asterisk.org/c/asterisk/+/18829/comment/a8c5faa4_834aefaf 
PS12, Line 249: static void hiredis_disconnect(void* data)
Document that this is a void pointer because the redisContext is stored directly using a threadstorage, instead of wrapped in some way.


https://gerrit.asterisk.org/c/asterisk/+/18829/comment/aa119571_24c8c57b 
PS12, Line 254: 		ast_log(AST_LOG_ERROR, "No hiredis_context. Reason: UNKNOWN\n");
This feels more like a coding error, then something a user can really solve or should know about it so I don't think the log message is really valuable as an error.


https://gerrit.asterisk.org/c/asterisk/+/18829/comment/bcf10b1e_7752410b 
PS12, Line 258: 	ast_debug(6, "socket_fd[%d]\n", hiredis_context->fd);
I don't think this is useful.


https://gerrit.asterisk.org/c/asterisk/+/18829/comment/e0bd533a_4d53fd0f 
PS12, Line 289: 		if (attempt++ < 3) {
This retry logic should be documented, such as in the conf file and XML


https://gerrit.asterisk.org/c/asterisk/+/18829/comment/1de64399_a6625bb1 
PS12, Line 302: 		ast_log(AST_LOG_ERROR, "%s\n", reply->str);
I think the error message should include something like: "Received error 'blah' while executing redis command"


https://gerrit.asterisk.org/c/asterisk/+/18829/comment/f6fab9b3_3dbc7054 
PS12, Line 539: 	pbx_builtin_setvar_helper(chan, "REDIS_RESULT", buf);
What is the reasoning behind also setting REDIS_RESULT? Dialplan functions don't also set dialplan variables, they're meant to return the information themselves only.


https://gerrit.asterisk.org/c/asterisk/+/18829/comment/5259be5f_be590f3b 
PS12, Line 763: 		redisFree(hiredis_context);
Would this result in a double free when the threadstorage gets destroyed?


https://gerrit.asterisk.org/c/asterisk/+/18829/comment/a4a2107f_d2166f45 
PS12, Line 783: 	hiredis_shutdown();
This doesn't actually shut down all connections, because all other threads won't shut down theirs. What state does this leave them in? I think either unload can't be allowed, or module reference counting has to be used so that if any thread is using a redis connection then it can't be unloaded.


https://gerrit.asterisk.org/c/asterisk/+/18829/comment/5e0e080d_11274302 
PS12, Line 850: 	if (hiredis_init()) {
This will just keep a connection open for no reason on the main Asterisk thread, or CLI thread.



-- 
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: 13
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-Attention: N A <mail at interlinked.x10host.com>
Gerrit-Comment-Date: Thu, 28 Jul 2022 09:30:08 +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/20220728/93c211ac/attachment.html>