Re: [PATCH] trace-cmd: Fix record --date flag when sending tracing data to a listener

[Slavomir Kaslev <kaslevs@vmware.com>]

On Fri, Nov 30, 2018 at 2:33 PM Slavomir Kaslev <kaslevs@vmware.com> wrote:
>
> On Wed, Nov 28, 2018 at 5:19 AM Steven Rostedt <rostedt@goodmis.org> wrote:
> >
> > On Tue, 27 Nov 2018 10:15:55 +0000
> > Slavomir Kaslev <kaslevs@vmware.com> wrote:
> >
> > > > I don't see anything too wrong with it, accept the following test broke:
> > > >
> > > >  $ git checkout trace-cmd-v2.6
> > > >  $ make
> > > >  $ sudo cp trace-cmd /usr/local/bin/trace-cmd-v2.6
> > > >  $ git checkout origin/master
> > > >  $ patch -p1 < this.patch
> > > >  $ make
> > > >  $ trace-cmd-v2.6 listen -p 12345
> > > >
> > > > In another terminal:
> > > >
> > > >  $ sudo trace-cmd record -N 127.0.0.1:12345 -e sched sleep 1
> > > > trace-cmd: No such file or directory
> > > >   Cannot handle the protocol
> > > >
> > > >
> > > > Remember, we need to remain backward compatible. We also need to test
> > > > this code running as a listener, and the trace-cmd-v2.6 (and earlier)
> > > > as the recorder.
> > >
> > > This is a design bug (the best kind), metadata should really be written from the
> > > recording side and not from the listener. A backward compatible fix should have
> > > the newer recorder and listener detect they're talking to an older version and
> > > fallback to broken behavior. This implies a new protocol version or extending
> > > MSG_TINIT/MSG_RINIT so that we can infer the behavior on the other side and
> > > fallback to being broken when necessary.
> >
> > Agreed.
> >
> > >
> > > What would you suggest?
> >
> > I just want to stress that I feel as strong for backward compatibility
> > as Linus feels for not breaking user space. That is, I'll go without a
> > fix if it breaks backward compatibility.
> >
> > But the really good news is, your code didn't break backward
> > compatibility. It uncovered a bug :-) :-) :-)
> >
> > The failure of the old code is that it looked at buf[0] without
> > initializing it. The fix is this:
> >
> > diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c
> > index e1e2f433..143793da 100644
> > --- a/tracecmd/trace-record.c
> > +++ b/tracecmd/trace-record.c
> > @@ -2775,6 +2775,8 @@ static void check_protocol_version(struct tracecmd_msg_handle *msg_handle)
> >          */
> >         write(fd, V2_CPU, sizeof(V2_CPU));
> >
> > +       buf[0] = 0;
> > +
> >         /* read a reply message */
> >         n = read(fd, buf, BUFSIZ);
> >
>
> This fix does work and recorder fallbacks to protocol v1 when talking
> with v2.6 listener.
>
> The resulting trace file fails to parse though
>
> kaslevs@box:~/tmp$ tc report -i trace.localhost:39754.dat
>   failed to init data
>
> because both the recorder and the listener wrote options to the .dat file.
>
> So we still have to detect that the listener end is an older version
> and fallback to old behavior in the recorder.
>
> Our current protocol doesn't allow to easily add new fields to
> messages because the size of messages is hard coded in the executable.
> Thus extending
>
> struct tracecmd_msg_rinit {
>         be32 cpus;
> } __attribute__((packed));
>
> or reusing bits from .cpus will not fly.
>
> Alternatively, we can add tracecmd_msg_rinit2 with which the listener
> (new versions) responds depending on the options set in
> tracecmd_msg_tinit by the recorder (when the recorder is new version
> too).

Another option is to fix the protocol itself to be future proof and
allow adding new message fields without breaking old versions. In
other words, make each command write it's .size on the wire instead of
having it hard-coded (the current .size in tracecmd_msg_header bounds
the whole message (command and additional data) and doesn't allow to
differentiate between different versions of say the tracecmd_msg_rinit
command).