Hello Jonathan,
On Mon, May 30, 2022 at 11:27:55AM -0400, Jonathan Rajotte-Julien wrote:
> [Please note: This e-mail is from an EXTERNAL e-mail address]
>
> Hi Marcel,
>
> Thanks for sending this patch.
>
> Looks sensible to me, still do you have a reproducer for it? I went back to
> bug 1352 and even with https://bugs.lttng.org/attachments/546 was unable to
> force the assert failure.
I can only reproduce it when running lttng-consumerd in a debugger
environment, in my case gdb. My reproduction scenario is:
1. Setting a breakpoint on snapshot_channel() inside
src/common/ust-consumer/ust-consumer.c
2. When the breakpoint hits, remove the the complete lttng directory
containing the session data.
3. Continue the lttng_consumerd process from gdb.
4. In that case you see a negative return value -1 from
consumer_stream_create_output_files() inside snapshot_channel().
5. Take another snapshot and you will see lttng_consumerd crash because
of the assert(!stream->trace_chunk); inside snapshot_channel(). This
last action does not require any breakpoint intervention.
The scenario seems to be very timing sensitive to reproduce. I do not
have a clear command sequence to achieve the same error.
The proposed patch prevents lttng_consumerd from crashing in step 5.
Kind regards,
Marcel
>
> Cheers
>
> ----- Original Message -----
> > From: "Marcel Hamer via lttng-dev" <lttng-dev@lists.lttng.org>
> > To: "lttng-dev" <lttng-dev@lists.lttng.org>
> > Sent: Monday, 30 May, 2022 10:10:21
> > Subject: [lttng-dev] [PATCH lttng-tools] Fix: cleanup stream on snapshot
> > failure
>
> > When a channel snapshot creation fails the stream should be cleaned up
> > properly. If the stream is not closed and cleaned properly on a failure,
> > the next time a snapshot is created an assert is triggered for:
> >
> > assert(!stream->trace_chunk);
> >
> > inside the snapshot_channel function. Since the stream->trace_chunk was
> > not reset to NULL. The reset to NULL happens inside the
> > consumer_stream_close function.
> >
> > Fixes #1352
> >
> > Signed-off-by: Marcel Hamer <marcel.ha...@windriver.com>
> > ---
> > src/common/ust-consumer/ust-consumer.c | 10 +++++-----
> > 1 file changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/src/common/ust-consumer/ust-consumer.c
> > b/src/common/ust-consumer/ust-consumer.c
> > index f176ca40a..f43216829 100644
> > --- a/src/common/ust-consumer/ust-consumer.c
> > +++ b/src/common/ust-consumer/ust-consumer.c
> > @@ -1147,13 +1147,13 @@ static int snapshot_channel(struct
> > lttng_consumer_channel *channel,
> > if (use_relayd) {
> > ret = consumer_send_relayd_stream(stream, path);
> > if (ret < 0) {
> > - goto error_unlock;
> > + goto error_close_stream;
> > }
> > } else {
> > ret = consumer_stream_create_output_files(stream,
> > false);
> > if (ret < 0) {
> > - goto error_unlock;
> > + goto error_close_stream;
> > }
> > DBG("UST consumer snapshot stream (%" PRIu64 ")",
> > stream->key);
> > @@ -1170,19 +1170,19 @@ static int snapshot_channel(struct
> > lttng_consumer_channel *channel,
> > ret = lttng_ustconsumer_take_snapshot(stream);
> > if (ret < 0) {
> > ERR("Taking UST snapshot");
> > - goto error_unlock;
> > + goto error_close_stream;
> > }
> >
> > ret = lttng_ustconsumer_get_produced_snapshot(stream,
> > &produced_pos);
> > if (ret < 0) {
> > ERR("Produced UST snapshot position");
> > - goto error_unlock;
> > + goto error_close_stream;
> > }
> >
> > ret = lttng_ustconsumer_get_consumed_snapshot(stream,
> > &consumed_pos);
> > if (ret < 0) {
> > ERR("Consumerd UST snapshot position");
> > - goto error_unlock;
> > + goto error_close_stream;
> > }
> >
> > /*
> > --
> > 2.25.1
> >
> > _______________________________________________
> > lttng-dev mailing list
> > lttng-dev@lists.lttng.org
> > https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
