[Asterisk-code-review] revert deleted files that create 10000 lines of change (asterisk[master])

[Joshua Colp]

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

Change subject: revert deleted files that create 10000 lines of change
......................................................................


Patch Set 4: Code-Review-1

(13 comments)

Commit Message:

https://gerrit.asterisk.org/c/asterisk/+/18829/comment/72f0d55e_6fc1b493 
PS4, Line 7: revert deleted files that create 10000 lines of change
The commit message should go back to the way it was.


File configs/samples/redisd.conf.sample:

https://gerrit.asterisk.org/c/asterisk/+/18829/comment/55fa9e39_40ec766d 
PS4, Line 2: enabled = yes			; When set to yes, redisd support is enabled
What's the default?


File include/asterisk/redisd.h:

https://gerrit.asterisk.org/c/asterisk/+/18829/comment/508a0d0c_76677d75 
PS4, Line 30: AST_OPTIONAL_API(void, ast_redisd_command, (const char *command, const char *key_path, const char *_keyvalue, char *return_buffer, size_t return_buffwe_len), {});
This needs to be documented


File res/res_redis_device_state.c:

https://gerrit.asterisk.org/c/asterisk/+/18829/comment/35a82351_d5feb335 
PS4, Line 49: 	char return_buffer[4096];
Why 4096?


https://gerrit.asterisk.org/c/asterisk/+/18829/comment/c61dd304_c0c6d1b7 
PS4, Line 69: 		ast_debug(6, "device length zero\n");
Is it possible for this to happen?


https://gerrit.asterisk.org/c/asterisk/+/18829/comment/ef3fe8c6_4f74e17f 
PS4, Line 78: 	ast_str_set(&key_path, 0, "deviceState:%s", device);
This is easy to calculate as a fixed length, which means ast_str_alloca could be used instead of mallocing/freeing underneath.


https://gerrit.asterisk.org/c/asterisk/+/18829/comment/4ea41ee0_8ba5d236 
PS4, Line 96: 	device_state_sub = stasis_subscribe(ast_device_state_topic_all(), device_state_cb, NULL);
This can fail


https://gerrit.asterisk.org/c/asterisk/+/18829/comment/0c0f9367_e944f851 
PS4, Line 97: 
Filtering was added for stasis subscriptions to reduce the work done within dispatching, this is done by res_stasis_device_state for example for device state and should be done here as well.


https://gerrit.asterisk.org/c/asterisk/+/18829/comment/c3c5a906_f55e99f3 
PS4, Line 101: AST_MODULE_INFO(ASTERISK_GPL_KEY, AST_MODFLAG_GLOBAL_SYMBOLS | AST_MODFLAG_LOAD_ORDER, "REDIS Device State support",
Why does this export global symbols?


File res/res_redisd.c:

https://gerrit.asterisk.org/c/asterisk/+/18829/comment/ea83baf5_8b5bceb7 
PS4, Line 226: 		if (attempt++ < 3) {
Should this be configurable?


https://gerrit.asterisk.org/c/asterisk/+/18829/comment/076de79d_c337a940 
PS4, Line 245: 	msg = ast_str_create(4096);
Why 4096? If you set it to a lower initial allocation, it will automatically grow as needed. You could also store it on the thread local storage instead and reuse it.


https://gerrit.asterisk.org/c/asterisk/+/18829/comment/f95358ec_a44ceb1e 
PS4, Line 287: 		strncpy(return_buffer, ast_str_buffer(msg), return_buffer_length);
Why not make a return buffer optional so the caller doesn't have to allocate one that they'll just end up ignoring?


https://gerrit.asterisk.org/c/asterisk/+/18829/comment/50d2cefd_0e522270 
PS4, Line 361: 	if (!(cfg = ao2_alloc(sizeof(*cfg), conf_destructor))) {
These two things don't need to be allocated with locks



-- 
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: 4
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-CC: 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: Fri, 22 Jul 2022 14:21:09 +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/20220722/04e942d8/attachment-0001.html>