Re: [RFC PATCH v8 05/13] trace-cmd: Add TRACE_REQ and TRACE_RESP messages

[Slavomir Kaslev <kaslevs@vmware.com>]

On Fri, Feb 22, 2019 at 11:09 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Fri, 22 Feb 2019 20:05:31 +0200
> Slavomir Kaslev <kaslevs@vmware.com> wrote:
>
> > +static int make_trace_resp(struct tracecmd_msg *msg,
> > +                        int page_size, int nr_cpus, unsigned int *ports)
>
> I wonder if we should just have a generic "argc"/"argv" protocol to
> send dynamic amounts of data, and not have specific dynamic messages?
>
> Perhaps pass the numbers as ASCII? And then converted them at the other
> end. We need to handle ntoh anyway, so a conversion (albeit a fast one)
> is probably done anyway.
>
> We could send:
>  int argc =  6;
>  char **argv = { "4096", "16", "33445", "33446", "33448", "33449" };
>
> Or, if you are not comfortable with sending ASCII and converting, what
> about have a specific "sending an array of numbers"?
>
>  int argc = 6;
>  int *argv = { 4096, 16, 33445, 33446, 33448, 33449 };
>
> Thoughts?

I don't see any drawbacks of this approach (modulo a bit of
arithmetic). I implemented it the way it is following how the listener
is sending port numbers back to the recording process.

Should we also switch the listener to use the same encoding of ports
with protocol V3 before the next release?

How about options (which is the only other non-text use of the
variable buffer part of messages)? Currently the only option in use is
MSGOPT_USETCP and can become simply "tcp" on the wire.

We can then have a convention that the buffer at then end of messages
is always text and drop the second union in tracecmd_msg.

-- Slavi

>
> -- Steve
>
> > +{
> > +     int ports_size = nr_cpus * sizeof(*msg->port_array);
> > +     int i;
> > +
> > +     msg->hdr.size = htonl(ntohl(msg->hdr.size) + ports_size);
> > +     msg->trace_resp.cpus = htonl(nr_cpus);
> > +     msg->trace_resp.page_size = htonl(page_size);
> > +
> > +     msg->port_array = malloc(ports_size);
> > +     if (!msg->port_array)
> > +             return -ENOMEM;
> > +
> > +     for (i = 0; i < nr_cpus; i++)
> > +             msg->port_array[i] = htonl(ports[i]);
> > +
> > +     return 0;
> > +}
> > +
> > +int tracecmd_msg_send_trace_resp(struct tracecmd_msg_handle *msg_handle,
> > +                              int nr_cpus, int page_size,
> > +                              unsigned int *ports)
> > +{
> > +     struct tracecmd_msg msg;
> > +     int ret;
> > +
> > +     tracecmd_msg_init(MSG_TRACE_RESP, &msg);
> > +     ret = make_trace_resp(&msg, page_size, nr_cpus, ports);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     return tracecmd_msg_send(msg_handle->fd, &msg);
> > +}
> > +
> > +int tracecmd_msg_recv_trace_resp(struct tracecmd_msg_handle *msg_handle,
> > +                              int *nr_cpus, int *page_size,
> > +                              unsigned int **ports)
> > +{
> > +     struct tracecmd_msg msg;
> > +     ssize_t buf_len;
> > +     int i, ret;
> > +
> > +     ret = tracecmd_msg_recv(msg_handle->fd, &msg);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     if (ntohl(msg.hdr.cmd) != MSG_TRACE_RESP) {
> > +             ret = -ENOTSUP;
> > +             goto out;
> > +     }
> > +
> > +     buf_len = ntohl(msg.hdr.size) - MSG_HDR_LEN - ntohl(msg.hdr.cmd_size);
> > +     if (buf_len <= 0 ||
> > +         buf_len != sizeof(*msg.port_array) * ntohl(msg.trace_resp.cpus)) {
> > +             ret = -EINVAL;
> > +             goto out;
> > +     }
> > +
> > +     *nr_cpus = ntohl(msg.trace_resp.cpus);
> > +     *page_size = ntohl(msg.trace_resp.page_size);
> > +     *ports = calloc(*nr_cpus, sizeof(**ports));
> > +     if (!*ports) {
> > +             ret = -ENOMEM;
> > +             goto out;
> > +     }
> > +     for (i = 0; i < *nr_cpus; i++)
> > +             (*ports)[i] = ntohl(msg.port_array[i]);
> > +
> > +     msg_free(&msg);
> > +     return 0;
> > +
> > +out:
> > +     error_operation(&msg);
> > +     if (ret == -EOPNOTSUPP)
> > +             handle_unexpected_msg(msg_handle, &msg);
> > +     msg_free(&msg);
> > +     return ret;
> > +}
>