<div dir="ltr">Dear David,<div><br></div><div>Can you please reply?</div><div class="gmail_extra"><br></div><div class="gmail_extra">Thanks,<br>Sandeep.<br><br><div class="gmail_quote">On Thu, Mar 13, 2014 at 11:47 PM, Sandeep K Chaudhary <span dir="ltr"><<a href="mailto:babbusandy2006@gmail.com" target="_blank">babbusandy2006@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Hi David,<div><br></div><div>What about the following changes?</div><div><br></div><div><div>1071a1072,1079</div>
<div>> </div><div>> bool check_conn_version(struct relay_connection *conn, uint32_t major, uint32_t minor) {</div>
<div>> <span style="white-space:pre-wrap"> </span>if((conn->major == major) && (conn->minor == minor))</div><div>> <span style="white-space:pre-wrap"> </span>return true;</div><div>> </div>
<div>> <span style="white-space:pre-wrap"> </span>return false;</div><div>> }</div><div>> </div><div>1097,1103c1105</div><div class=""><div>< <span style="white-space:pre-wrap"> </span>switch (conn->minor) {</div>
<div>< <span style="white-space:pre-wrap"> </span>case 1:</div></div><div>< <span style="white-space:pre-wrap"> </span>case 2:</div><div>< <span style="white-space:pre-wrap"> </span>case 3:</div><div>< <span style="white-space:pre-wrap"> </span>break;</div>
<div>< <span style="white-space:pre-wrap"> </span>case 4: /* LTTng sessiond 2.4 */</div><div>< <span style="white-space:pre-wrap"> </span>default:</div><div>---</div><div>> <span style="white-space:pre-wrap"> </span>if(check_conn_version(conn, conn->major, 4))</div>
<div>1105d1106</div><div>< <span style="white-space:pre-wrap"> </span>}</div><div>1185,1193c1186,1191</div><div class=""><div>< <span style="white-space:pre-wrap"> </span>switch (conn->minor) {</div><div>< <span style="white-space:pre-wrap"> </span>case 1: /* LTTng sessiond 2.1 */</div>
<div>< <span style="white-space:pre-wrap"> </span>ret = cmd_recv_stream_2_1(conn, stream);</div><div>< <span style="white-space:pre-wrap"> </span>break;</div><div>< <span style="white-space:pre-wrap"> </span>case 2: /* LTTng sessiond 2.2 */</div>
<div>< <span style="white-space:pre-wrap"> </span>default:</div><div>< <span style="white-space:pre-wrap"> </span>ret = cmd_recv_stream_2_2(conn, stream);</div><div>< <span style="white-space:pre-wrap"> </span>break;</div>
<div>< <span style="white-space:pre-wrap"> </span>}</div></div><div>---</div><div>> if(check_conn_version(conn, conn->major, 1))</div><div>> ret = cmd_recv_stream_2_1(conn, stream);</div>
<div>> </div><div>> else if(check_conn_version(conn, conn->major, 2))</div><div>> ret = cmd_recv_stream_2_2(conn, stream);</div><div>> </div></div><div class="gmail_extra"><br><div class="gmail_quote">
<div class="">
On Thu, Mar 13, 2014 at 7:44 AM, David Goulet <span dir="ltr"><<a href="mailto:dgoulet@efficios.com" target="_blank">dgoulet@efficios.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div><div>On 12 Mar (22:59:20), Sandeep K Chaudhary wrote:<br>
> Thanks for pointing it to me, David !<br>
><br>
> However I see the following in relay_add_stream in main.c<br>
><br>
> switch (conn->minor) {<br>
> case 1: /* LTTng sessiond 2.1 */<br>
> ret = cmd_recv_stream_2_1(conn, stream);<br>
> break;<br>
> case 2: /* LTTng sessiond 2.2 */<br>
> default:<br>
> ret = cmd_recv_stream_2_2(conn, stream);<br>
> break;<br>
> }<br>
><br>
> This means that it's calling either 'cmd_recv_stream_2_1' or<br>
> 'cmd_recv_stream_2_2' depending on the minor version number. Is this not we<br>
> want? Though I don't understand why it should be calling<br>
> 'cmd_recv_stream_2_2' for minor versions other than 1. Is this correct<br>
> behavior?<br>
<br>
</div></div>Yes so for now only the minor version matter since a major number other<br>
than 2 can't be negotiated during the send_version step (the first thing<br>
the relayd receives for a connection).<br></blockquote><div><br></div></div><div> 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.</div>
<div class="">
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
At some point in time, we would need to check the major number to here<br>
so calling cmd_recv_stream_2_2(...) would make more sense. Maybe it's<br>
something you want to do here before anything, adding a layer that<br>
checks major + minor and calls the right one? Something that could be<br>
generic enough so we don't have those ugly switch cases in each<br>
command... ?<br></blockquote><div><br></div></div><div> 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.</div>
<div><br></div><div> Thanks and regards,</div><div>Sandeep.</div></div></div></div>
</blockquote></div><br><br clear="all"><div><br></div>-- <br>Thanks and regards,<br>Sandeep K Chaudhary.
</div></div>