[Asterisk-code-review] app_signal: Add signaling applications (asterisk[master])

[N A]

Attention is currently required from: Joshua Colp, Mark Murawski.

N A has posted comments on this change. ( https://gerrit.asterisk.org/c/asterisk/+/17786 )

Change subject: app_signal: Add signaling applications
......................................................................


Patch Set 5:

(4 comments)

File apps/app_signal.c:

https://gerrit.asterisk.org/c/asterisk/+/17786/comment/85d31de8_674439d3 
PS4, Line 160: 	}
> This hasn't called ast_mutex_destroy
Done


https://gerrit.asterisk.org/c/asterisk/+/17786/comment/291158be_611c08a1 
PS4, Line 171: 		ast_mutex_lock(&s->lock);
> Why lock and then unlock?
Done


https://gerrit.asterisk.org/c/asterisk/+/17786/comment/72629d62_74298b26 
PS4, Line 278: 		remove_signal(signame);
> This has a race condition where another thread could be finding this signal, while it is being remov […]
I think this could be fixed by simply calling remove_signal before ast_mutex_unlock, i.e. moving it inside the lock. Given that ast_mutex_destroy is called though, is it problematic to destroy objects with locks held? If not, then this would be the cleanest way to do it I think.

Alternately, s->watchers = s->watchers - 1; is executed with the lock held. So as soon as the signal is unlocked, if it's about to be destroyed, s->watchers == 0.

So on both the wait and send side, we could also check for 0 and recognize it's about to be destroyed and just bail early in that case.

Alternately, I could WRLOCK the list right before unlocking the signal, which should just guarantee the signal will be destroyed before anyone else can grab it. I'm thinking that may be the cleanest and simplest option. This is the one I've taken in the updated review.


https://gerrit.asterisk.org/c/asterisk/+/17786/comment/122e476d_6907a855 
PS4, Line 422: 		ast_log(LOG_WARNING, "One or more signals is currently in use. Unload failed.\n");
> What does this actually mean for a user? You've removed all of the signals from the list, so will th […]
Done



-- 
To view, visit https://gerrit.asterisk.org/c/asterisk/+/17786
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Change-Id: Ic34439de3d60f8609357666a465c354d81f5fef3
Gerrit-Change-Number: 17786
Gerrit-PatchSet: 5
Gerrit-Owner: N A <asterisk at phreaknet.org>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: Joshua Colp <jcolp at sangoma.com>
Gerrit-Reviewer: Mark Murawski <markm at intellasoft.net>
Gerrit-Reviewer: N A <asterisk at phreaknet.org>
Gerrit-Attention: Joshua Colp <jcolp at sangoma.com>
Gerrit-Attention: Mark Murawski <markm at intellasoft.net>
Gerrit-Comment-Date: Thu, 22 Dec 2022 12:47:50 +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/20221222/bd9de670/attachment.html>