On Wed, Nov 28, 2018 at 5:19 AM Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:
>
> On Tue, 27 Nov 2018 10:15:55 +0000
> Slavomir Kaslev <kaslevs@xxxxxxxxxx> 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).
>
> That said, I have one minor nit with the patch. I'll reply to the patch
> itself.
>
> Thanks!
>
> -- Steve
>
|