On Mon, 28 Jun 2021 11:58:10 +0300
Yordan Karadzhov <y.karadz@xxxxxxxxx> wrote:
> > +
> > +static ssize_t read_trace_pipe(bool *keep_going, int in_fd, int out_fd)
> > +{
> > + char buf[BUFSIZ];
> > + ssize_t bread = 0;
> > + int ret;
> > +
> > + do {
> > + ret = read(in_fd, buf, BUFSIZ);
> > + if (ret > 0) {
> > + ret = write(STDOUT_FILENO, buf, ret);
>
> this should be
Doh! I know I was hard coding that in some places for debugging, and
that may have been me not cleaning it up. Thanks, will fix.
> ret = write(in_fd, buf, ret);
>
> > + if (ret > 0)
> > + bread += ret;
> > + }
> > + } while (ret > 0 && *(volatile bool *)keep_going);
> > +
>
> Nit: When looping you check the value of 'ret' 3 times.
> The same can be done with only 2 checks.
>
> while (*(volatile bool *)keep_going) {
> ret = read(in_fd, buf, BUFSIZ);
> if (ret <= 0)
> break;
>
> ret = write(out_fd, buf, ret);
> if (ret <= 0)
> break;
>
> bread += ret;
> }
Nice, I got you thinking about optimizations ;-)
I'll update it.
>
> > + if (ret < 0 && (errno == EAGAIN || errno == EINTR))
> > + ret = 0;
> > +
> > + return ret < 0 ? ret : bread;
> > +}
> > +
> > +static bool top_pipe_keep_going;
> > +
> > +/**
> > + * tracefs_trace_pipe_stream - redirect the stream of trace data to an output
> > + * file. The "splice" system call is used to moves the data without copying
> > + * between kernel address space and user address space. The user can interrupt
> > + * the streaming of the data by pressing Ctrl-c.
> > + * @fd: The file descriptor of the output file.
> > + * @instance: ftrace instance, can be NULL for top tracing instance.
> > + * @flags: flags to be passed to the "splice" system call.
> > + *
> > + * Returns -1 in case of an error or number of bytes transferred otherwise.
> > + */
> > +ssize_t tracefs_trace_pipe_stream(int fd, struct tracefs_instance *instance,
> > + int flags)
> > +{
> > + bool *keep_going = instance ? &instance->pipe_keep_going :
> > + &top_pipe_keep_going;
> > + const char *file = "trace_pipe";
> > + int brass[2], in_fd, ret = -1;
> > + int oflags = flags & SPLICE_F_NONBLOCK ? O_NONBLOCK: 0;
> > + off_t data_size;
> > + ssize_t bread = 0;
> > +
> > + (*(volatile bool *)keep_going) = true;
> > +
> > + in_fd = tracefs_instance_file_open(instance, file, O_RDONLY | oflags);
> > + if (in_fd < 0) {
> > + tracefs_warning("Failed to open 'trace_pipe'.");
> > + return ret;
> > + }
> > +
> > + if(pipe(brass) < 0) {
> > + tracefs_warning("Failed to open pipe.");
> > + goto close_file;
> > + }
> > +
> > + data_size = fcntl(brass[0], F_GETPIPE_SZ);
> > + if (data_size <= 0) {
> > + tracefs_warning("Failed to open pipe (size=0).");
> > + goto close_all;
> > + }
> > +
> > + /* Test if the output is splice safe */
> > + if (!splice_safe(fd, brass[0])) {
> > + close(brass[0]);
> > + close(brass[1]);
> > + return read_trace_pipe(keep_going, in_fd, fd);
>
> we must close 'in_fd' before returning.
Good catch. I was planning on closing it in read_trace_pipe() and
forgot. I'll close it here.
>
> > + }
> > +
> > + errno = 0;
> > +
> > + while (*(volatile bool *)keep_going) {
> > + ret = splice(in_fd, NULL,
> > + brass[1], NULL,
> > + data_size, flags);
> > + if (ret < 0)
> > + break;
> > +
> > + ret = splice(brass[0], NULL,
> > + fd, NULL,
> > + data_size, flags);
> > + if (ret < 0)
> > + break;
> > + bread += ret;
> > + }
> > +
> > + /*
> > + * Do not return error in the case when the "splice" system call
> > + * was interrupted by the user (pressing Ctrl-c).
> > + * Or if NONBLOCK was specified.
> > + */
> > + if (!keep_going || errno == EAGAIN || errno == EINTR)
> > + ret = 0;
> > +
> > + close_all:
> > + close(brass[0]);
> > + close(brass[1]);
> > + close_file:
> > + close(in_fd);
> > +
> > + return ret ? ret : bread;
> > +}
> > +
> > +/**
> > + * tracefs_trace_pipe_print - redirect the stream of trace data to "stdout".
> > + * The "splice" system call is used to moves the data without copying
> > + * between kernel address space and user address space.
> > + * @instance: ftrace instance, can be NULL for top tracing instance.
> > + *
> > + * Returns -1 in case of an error or number of bytes transferred otherwise.
> > + */
> > +
> > +ssize_t tracefs_trace_pipe_print(struct tracefs_instance *instance)
> > +{
> > + return tracefs_trace_pipe_stream(STDOUT_FILENO, instance,
> > + SPLICE_F_NONBLOCK);
> > +}
> > +
>
> In fact I want to use this function in "blocking" mode. I would like to keep listening for new trace data until the user
> presses 'Ctrl-c'. Can we make this function to take a second parameter that will be the splice flags?
Yeah, I was going to ask you if you wanted this to have flags too.
I was also thinking that we should use the O_ flags and not the
SPLICE_F_ flags, as splice is more niche, and O_ is POSIX. We can check
the flags and change them for splice inside these functions.
Are you OK with that?
-- Steve
>
> Thanks a lot!
> Yordan
>
|