[ltt-dev] [UST] open_buffers_list in tracectl.c

[ltt-dev] [UST] open_buffers_list in tracectl.c

[Yannick Brosseau]

Question for UST experts (Nils?)

While trying to fix the fork case, I've found problems with the the
open_buffers_list. I've fixed the segfault by moving the
ltt_trace_destroy later in ust_fork, but now I get some warnings about
already closed file descriptor.
It seems that we try to close the fd twice.
Now I'm wondering if we really need to close the fd referenced by the
open buffers list in ust_fork()? All these FD are closed when we destroy
the channels in the call to ltt_trace_destroy, so it seems redundant to
me. 

Am I missing something?

For quick reference here's the code using open_buffer_list from
tracectl.c in ust_fork(void)

    /* Delete all blocked consumers */
    cds_list_for_each_entry_safe(buf, buf_tmp, &open_buffers_list,
                 open_buffers_list) {
        result = close(buf->data_ready_fd_read);
        if (result == -1) {
            PERROR("close");
        }
        result = close(buf->data_ready_fd_write);
        if (result == -1) {
            PERROR("close");
        }
        cds_list_del(&buf->open_buffers_list);
    }

It would  suggest to change it to simply

    /* Empty the list of open buffers */
    cds_list_for_each_entry_safe(buf, buf_tmp, &open_buffers_list,
                 open_buffers_list) {
        cds_list_del(&buf->open_buffers_list);
    }

[ltt-dev] [UST] open_buffers_list in tracectl.c

[Nils Carlson]

Yannick Brosseau wrote:
> Question for UST experts (Nils?)
Not an expert, at least in this part of the code... :-) The plan is after
all to replace the buffer handling with mathieus generic buffer library
so I haven't put too much work into it.
>
> While trying to fix the fork case, I've found problems with the the
> open_buffers_list. I've fixed the segfault by moving the
> ltt_trace_destroy later in ust_fork, but now I get some warnings about
> already closed file descriptor.
> It seems that we try to close the fd twice.
> Now I'm wondering if we really need to close the fd referenced by the
> open buffers list in ust_fork()? All these FD are closed when we destroy
> the channels in the call to ltt_trace_destroy, so it seems redundant to
> me. 
>
> Am I missing something?
No, this is actually on my "to fix list". The trace name however isn't
guaranteed to be "auto" anymore so we should actually loop over the
existing traces and destroy each of them properly. We have a number
of these "auto" assumptions scattered through tracectrl.c and all of
them should be fixed.

/Nils
>
> For quick reference here's the code using open_buffer_list from
> tracectl.c in ust_fork(void)
>
>     /* Delete all blocked consumers */
>     cds_list_for_each_entry_safe(buf, buf_tmp, &open_buffers_list,
>                  open_buffers_list) {
>         result = close(buf->data_ready_fd_read);
>         if (result == -1) {
>             PERROR("close");
>         }
>         result = close(buf->data_ready_fd_write);
>         if (result == -1) {
>             PERROR("close");
>         }
>         cds_list_del(&buf->open_buffers_list);
>     }
>
> It would  suggest to change it to simply
>
>     /* Empty the list of open buffers */
>     cds_list_for_each_entry_safe(buf, buf_tmp, &open_buffers_list,
>                  open_buffers_list) {
>         cds_list_del(&buf->open_buffers_list);
>     }
>
> _______________________________________________
> ltt-dev mailing list
> ltt-dev at lists.casi.polymtl.ca
> http://lists.casi.polymtl.ca/cgi-bin/mailman/listinfo/ltt-dev