[asterisk-dev] [Code Review] unload/load/reload support for chan_skinny

[Dan Austin]

Russell's response:
> trunk/channels/chan_skinny.c
> <http://reviewboard.digium.com/r/130/#comment722>

>    It looks like this is done without protecting the lines list with a lock.

I started addressing these issues, and thought to restructure the approach
a bit.  It only took me most of a day to remember why I used the approach
in this patch.  I threw away the reworked code and will be just fixing
the issues raised

> trunk/channels/chan_skinny.c
> <http://reviewboard.digium.com/r/130/#comment723>

>    As a suggestion, you can collapse this down to ...

>    ast_verb(3, "%s config for line '%s'\n", l->name,
>       update ? "Updated" : (skinnyreload ? "Reloaded" : "Created"));

Got it.


> trunk/channels/chan_skinny.c
> <http://reviewboard.digium.com/r/130/#comment724>

>    I see you go through the list removing devices and just throw them
>  away.  Should you be freeing them or something?

I am embarrassed that I got it right in one spot and screwed it up
here.  Will be fixed.

> trunk/channels/chan_skinny.c
> <http://reviewboard.digium.com/r/130/#comment725>

>    From a high level look at this function ...

>    1) It looks like no locking is being done to protect your list operations.

                Will be fixed.

>    2) Keep in mind that it is possible for 2 reloads to be issued at the same
>     time from different threads.

        Will a simple test to see if a reload is in process be acceptable, or
should I be looking at additional locking?


Dan