[Asterisk-code-review] res_redisd: add REDIS support modules to send asterisk DeviceState up... (asterisk[master])

[Sean Bright]

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

Change subject: res_redisd: add REDIS support modules to send asterisk DeviceState updates to a REDIS server
......................................................................


Patch Set 7: Code-Review-1

(6 comments)

Patchset:

PS7: 
Try to use 'Redis' or 'redis' consistent in docs, comments, log output, etc. I do not see anything canonical that stylizes it as 'REDIS.'


File configs/samples/redisd.conf.sample:

https://gerrit.asterisk.org/c/asterisk/+/18829/comment/cacf43c6_aef477ed 
PS7, Line 1: [general]
For config files like this, I would prefer to see things lined up using all spaces instead of tabs.


https://gerrit.asterisk.org/c/asterisk/+/18829/comment/4f80a174_62b6663f 
PS7, Line 6: ;dbname = 0			; DB to select, default = 0
How about 'database' or 'database_name?'


https://gerrit.asterisk.org/c/asterisk/+/18829/comment/5c1c404f_9e269131 
PS7, Line 8: bgsave = yes			; When set to yes, initiate a Background save when unloading module, default=no
How about 'background_save_on_unload?'


File include/asterisk/redisd.h:

https://gerrit.asterisk.org/c/asterisk/+/18829/comment/2a715cf8_424d6559 
PS7, Line 48: AST_OPTIONAL_API(void, ast_redisd_command, (const char *command, const char *key_path, const char *key_value, char *return_buffer, size_t return_buffwe_len), {});
I do not think this is good API for all of Asterisk (it only allows for a single Redis connection, for example).

My suggestion would be to put the contents of res/res_redisd.c directly into res/res_redis_device_state.c and if someone wants to come along at a later time and create a general purpose Redis API they can.


File res/res_redis_device_state.c:

https://gerrit.asterisk.org/c/asterisk/+/18829/comment/30d81c8e_acfa1b95 
PS7, Line 88:         device_state_sub = stasis_subscribe(ast_device_state_topic_all(), device_state_cb, NULL);
            :         if (!device_state_sub) {
            : 		unload_module();
            : 		return AST_MODULE_LOAD_DECLINE;
            : 	}
            : 
            :         stasis_subscription_accept_message_type(device_state_sub, ast_device_state_message_type());
            :         stasis_subscription_set_filter(device_state_sub, STASIS_SUBSCRIPTION_FILTER_SELECTIVE);
            : 
            :         return AST_MODULE_LOAD_SUCCESS;
Leading whitespace got a little funky here.



-- 
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: 7
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: Alec Davis <alec at bdt.co.nz>
Gerrit-Attention: Joshua Colp <jcolp at sangoma.com>
Gerrit-Attention: N A <mail at interlinked.x10host.com>
Gerrit-Comment-Date: Sun, 24 Jul 2022 15:18:00 +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/20220724/df1733a1/attachment-0001.html>