Mathieu Desnoyers: > * David Goulet ([email protected]) 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 >>> > _______________________________________________ lttng-dev mailing list [email protected] http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
