[Spice-devel] [PATCH v2] red-channel: make red_client_{ref, unref} thread safe

[Jonathon Jongsma]

Just a few minor grammer fixes, otherwise looks good to me

On Tue, 2016-04-12 at 14:24 +0100, Frediano Ziglio wrote:
> These function are called on both side of dispatcher so the

side -> sides

> increment/decrement of the counter is done in multiple thread.

thread -> threads

> This caused the counter to not get incremented correctly and
> a free when the structure was still pointed.

suggest changing the last line to something like:

"freed the structure too early, leaving a dangling pointer in the other thread"


Acked-by: Jonathon Jongsma <jjongsma at redhat.com>


> 
> This fixes https://bugzilla.redhat.com/show_bug.cgi?id=1253375.
> 
> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> ---
>  server/red-channel.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> Changes from v1:
> - more detailed explanation;
> - fixed a typo.
> 
> diff --git a/server/red-channel.c b/server/red-channel.c
> index d8f1d27..cfddea0 100644
> --- a/server/red-channel.c
> +++ b/server/red-channel.c
> @@ -2064,13 +2064,13 @@ RedClient *red_client_new(RedsState *reds, int
> migrated)
>  RedClient *red_client_ref(RedClient *client)
>  {
>      spice_assert(client);
> -    client->refs++;
> +    g_atomic_int_inc(&client->refs);
>      return client;
>  }
>  
>  RedClient *red_client_unref(RedClient *client)
>  {
> -    if (!--client->refs) {
> +    if (g_atomic_int_dec_and_test(&client->refs)) {
>          spice_debug("release client=%p", client);
>          pthread_mutex_destroy(&client->lock);
>          free(client);