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

Fri Jul 29 07:35:02 CDT 2022

Attention is currently required from: Sean Bright, 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:

(18 comments)

Commit Message:

https://gerrit.asterisk.org/c/asterisk/+/18829/comment/577802df_c80baba1 
PS12, Line 9: Simplify, Simplify.
> The commit message is what will go into Asterisk, and what will show up in the changelog and such. […]
Done

Patchset:

PS15: 
Hopefully I got everything, either sorted or at least answered.

The reason for all of the function to have variadic was that to add a key value with spaces required it, otherwise the hiredis library returned a syntax error.

Following the hiredis example from https://github.com/redis/hiredis/blob/master/examples/example.c
    /* Set a key */
    reply = redisCommand(c,"SET %s %s", "foo", "hello world");
    printf("SET: %s\n", reply->str);
    freeReplyObject(reply);

File res/res_hiredis.c:

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

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

Reference code was res_statsd.c

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

https://gerrit.asterisk.org/c/asterisk/+/18829/comment/128aaff6_627c96c9 
PS12, Line 213: 	ast_debug(6, "socket_fd[%d]\n", hiredis_context->fd);
> I don't think this is useful
Going forward not any more, but was in early days, as we had 100's of fd leaks.

Removed.

https://gerrit.asterisk.org/c/asterisk/+/18829/comment/b96a5019_9df3da0b 
PS12, Line 243: 	memcpy(data, hiredis_context, sizeof(redisContext));
> I don't understand how this works. […]
reworked hiredis_connect, new variable to indicate that the context is temporary.

tmp_hiredis_context is allocated a new context when redisConnectWithTimeout
so has to be copied back to threadstorage, then free'd

https://gerrit.asterisk.org/c/asterisk/+/18829/comment/3e787784_aed895c4 
PS12, Line 244: 	ast_free(hiredis_context);
> Should this be using redisFree?
nope, after rework, hopefully this is clearer.

https://gerrit.asterisk.org/c/asterisk/+/18829/comment/33985d25_d030244c 
PS12, Line 249: static void hiredis_disconnect(void* data)
> Document that this is a void pointer because the redisContext is stored directly using a threadstora […]
Done

https://gerrit.asterisk.org/c/asterisk/+/18829/comment/2ceaeb9b_8f0e324b 
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  […]
removed log message.

https://gerrit.asterisk.org/c/asterisk/+/18829/comment/bb0b6b18_ebbe9deb 
PS12, Line 258: 	ast_debug(6, "socket_fd[%d]\n", hiredis_context->fd);
> I don't think this is useful.
Was useful to debug fd leak that happened just by unloading and reloading module.

Also see hiredis_shutdown comments.

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

https://gerrit.asterisk.org/c/asterisk/+/18829/comment/7ab7979e_12ab37fe 
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 redi […]
changed to:
ast_log(AST_LOG_ERROR, "Received error [%s] while executing Redis command [%s]\n", reply->str, cmd_string);

https://gerrit.asterisk.org/c/asterisk/+/18829/comment/67787f9f_24ca9751 
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 v […]
The functions are a direct copy of func_db.c

copy and rename from func_db:function_db_exists

Now have removed all instances of REDIS_RESULT

https://gerrit.asterisk.org/c/asterisk/+/18829/comment/899a3058_703f0259 
PS12, Line 763: 		redisFree(hiredis_context);
> Would this result in a double free when the threadstorage gets destroyed?
The thread that calls unload_module never exited, thus never called hiredis_disconnect as defined below.

AST_THREADSTORAGE_CUSTOM(hiredis_instance, hiredis_connect, hiredis_disconnect)

So unloading and reloading the module caused an fd leak.

This fixed the fd leak issue, but may cause a problem when the main thread exits.

https://gerrit.asterisk.org/c/asterisk/+/18829/comment/502c3a39_0c3b7df5 
PS12, Line 783: 	hiredis_shutdown();
> This doesn't actually shut down all connections, because all other threads won't shut down theirs. […]
I presume when you say 'connections' you're referring to sockets connected to redis?

There is only 1 long lasting connection and that is the deviceState one, the others only last the length of a redis command / reply.

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

File res/res_hiredis.c:

https://gerrit.asterisk.org/c/asterisk/+/18829/comment/31db3554_d47e73cb 
PS13, Line 302: 	char cmd_string[256]; 
Fixed redness

-- 
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: Joshua Colp <jcolp at sangoma.com>
Gerrit-Comment-Date: Fri, 29 Jul 2022 12:35:02 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
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/4358c1ec/attachment-0001.html>