[lttng-dev] [PATCH lttng-tools 1/4] Rename consumer threads and spawn them in daemon

[David Goulet]

Mathieu Desnoyers:
> * David Goulet (dgoulet at efficios.com) wrote:
>>
>>
>> Mathieu Desnoyers:
>>>>>> diff --git a/src/common/consumer.c b/src/common/consumer.c
>>>>>> index 242b05b..055de1b 100644
>>>>>> --- a/src/common/consumer.c
>>>>>> +++ b/src/common/consumer.c
>>>>>> @@ -1131,6 +1131,8 @@ void lttng_consumer_destroy(struct lttng_consumer_local_data *ctx)
>>>>>>  		PERROR("close");
>>>>>>  	}
>>>>>>  	utils_close_pipe(ctx->consumer_splice_metadata_pipe);
>>>>>> +	/* This should trigger the metadata thread to exit */
>>>>>> +	close(ctx->consumer_metadata_pipe[1]);
>>>>>
>>>>> this is adding a close, but did not remove any other remove that might
>>>>> previously be in place elsewhere.
>>>>
>>>> So we got two possible error path which is either the poll thread fails
>>>> or the consumer could be destroy by hand even though the threads are
>>>> working well.
>>>>
>>>> Actually, this close should check if the value is valid and close it. To
>>>> be honest, this is just a shortcut since close(-1) does not fail and
>>>> ignoring the close error here since we are in the cleanup path anyway so
>>>> we don't necessarily care about the perror message.
>>>>
>>>> Anyhow, we have to handle both error path. An if plus set -1 after close
>>>> can be done so not to confuse.
>>>
>>> if two threads can concurrently perform close on the same fd value, how
>>> can you prove there are no possible races ?
>>
>> Nothing to prove, the race is possible. The point I was trying to
>> explain is that it does not matter actually since we are in a cleanup
>> code path.
> 
> what happens if we close FD 0, 1, 2 or another FD, due to this race ?

This does not matter because we are in a "cleanup code path" ... closing
everything is the goal. In other circumstances, I agree that this is
unacceptable.

> 
> what happens if our code evolve to restart threads after errors, and we
> leave this race in place, so it becomes hard to reproduce that in some
> occasions we are closing random file descriptors ?

Well... agree but I doubt "lttng_destroy_consumer" will change it's
behavior :P

David

> 
>> Anyway, let's remove it since the data thread, when dying, will close
>> the metadata pipe anyway.
> 
> my point exactly :)
> 
>>
>> This will avoid more discussion for this small detail :).
> 
> The devil is in the details, as someone famous said before me.
> 
> Thanks,
> 
> Mathieu
> 
>>
>> David
>>
>>>
>>> Mathieu
>>>
>