[lttng-dev] Working on some bugs

[Sandeep K Chaudhary]

Hi David,

What about the following changes?

1071a1072,1079
>
> bool check_conn_version(struct relay_connection *conn, uint32_t major,
uint32_t minor) {
> if((conn->major == major) && (conn->minor == minor))
> return true;
>
> return false;
> }
>
1097,1103c1105
< switch (conn->minor) {
< case 1:
< case 2:
< case 3:
< break;
< case 4: /* LTTng sessiond 2.4 */
< default:
---
> if(check_conn_version(conn, conn->major, 4))
1105d1106
< }
1185,1193c1186,1191
< switch (conn->minor) {
< case 1: /* LTTng sessiond 2.1 */
< ret = cmd_recv_stream_2_1(conn, stream);
< break;
< case 2: /* LTTng sessiond 2.2 */
< default:
< ret = cmd_recv_stream_2_2(conn, stream);
< break;
< }
---
>         if(check_conn_version(conn, conn->major, 1))
>                 ret = cmd_recv_stream_2_1(conn, stream);
>
>         else if(check_conn_version(conn, conn->major, 2))
>                 ret = cmd_recv_stream_2_2(conn, stream);
>

On Thu, Mar 13, 2014 at 7:44 AM, David Goulet <dgoulet at efficios.com> wrote:

> On 12 Mar (22:59:20), Sandeep K Chaudhary wrote:
> > Thanks for pointing it to me, David !
> >
> > However I see the following in relay_add_stream in main.c
> >
> >         switch (conn->minor) {
> >         case 1: /* LTTng sessiond 2.1 */
> >                 ret = cmd_recv_stream_2_1(conn, stream);
> >                 break;
> >         case 2: /* LTTng sessiond 2.2 */
> >         default:
> >                 ret = cmd_recv_stream_2_2(conn, stream);
> >                 break;
> >         }
> >
> > This means that it's calling either 'cmd_recv_stream_2_1' or
> > 'cmd_recv_stream_2_2' depending on the minor version number. Is this not
> we
> > want? Though I don't understand why it should be calling
> > 'cmd_recv_stream_2_2' for minor versions other than 1. Is this correct
> > behavior?
>
> Yes so for now only the minor version matter since a major number other
> than 2 can't be negotiated during the send_version step (the first thing
> the relayd receives for a connection).
>

 In the proposed changes, we simply call the version checker function with
the same major version as in conn. In future, the calls can be made with
desired major and minor version numbers.


>
> At some point in time, we would need to check the major number to here
> so calling cmd_recv_stream_2_2(...) would make more sense. Maybe it's
> something you want to do here before anything, adding a layer that
> checks major + minor and calls the right one? Something that could be
> generic enough so we don't have those ugly switch cases in each
> command... ?
>

 Yes, I understand. And this would make the checking generic and better
than switch statements. Please see the proposed changes and let me know
what you think about it. Though, I have deliberately not taken care of the
'default' case in earlier switch statements. I am not convinced
why cmd_create_session_2_4 (in relay_create_session)
and cmd_recv_stream_2_2 ( in relay_add_stream) should be called in default
cases.

 Thanks and regards,
Sandeep.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.lttng.org/pipermail/lttng-dev/attachments/20140313/9a1d6d05/attachment-0001.html>