<p>Patch set 5:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4;">Code-Review -1</span></p><p><a href="https://gerrit.osmocom.org/9873">View Change</a></p><p>7 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/9873/5/firmware/libcommon/source/sniffer.c">File firmware/libcommon/source/sniffer.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/9873/5/firmware/libcommon/source/sniffer.c@206">Patch Set #5, Line 206:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;">struct simtrace_msg_hdr *usb_msg_header;<br> usb_msg->l1h = msgb_put(usb_msg, sizeof(*usb_msg_header));<br> usb_msg_header = (struct simtrace_msg_hdr *) usb_msg->l1h;<br> memset(usb_msg_header, 0, sizeof(*usb_msg_header));<br> usb_msg_header->msg_class = SIMTRACE_MSGC_SNIFF;<br> usb_msg_header->msg_type = SIMTRACE_MSGT_SNIFF_CHANGE;<br> usb_msg->l2h = usb_msg->l1h + sizeof(*usb_msg_header)<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">this kind of block seems to repeat over the file. I think it makes sense to factor it out into a separate function like msgb_put_usb_msg_header(msg, msg_class, msg_type) which would then contain all of these lines marked here.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/9873/5/firmware/libcommon/source/sniffer.c@216">Patch Set #5, Line 216:</a> <code style="font-family:monospace,monospace">usb_msg_header->msg_len = msgb_length(usb_msg);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">If you re-ordered it to:</p><ul><li>first msgb_put the usb_sniff change</li><li>then msgb_push the usb_msg_header in front</li><li>it would not be needed to update the length at the end.</li></ul><p style="white-space: pre-wrap; word-wrap: break-word;">In fact, the pushing of the usb_msg_header could be done in a wrapper function around usb_buf_submit, so it would become simply a usb_push_header_and_submit(usb_msg, SIMTRACE_MSGC_SNIFF, SIMTRACE_MSGT_SNIFF_CHANGE)</p><p style="white-space: pre-wrap; word-wrap: break-word;">This way the API becomes super easy to use (and get right)</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/9873/5/firmware/libcommon/source/sniffer.c@275">Patch Set #5, Line 275:</a> <code style="font-family:monospace,monospace"> TRACE_ERROR("ATR buffer overflow\n\r");</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">this might be something worth counting and/or reporting to the host? Keep in mind, the normal user has nothing connected to the UART...</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/9873/5/firmware/libcommon/source/sniffer.c@295">Patch Set #5, Line 295:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;">struct simtrace_msg_hdr *usb_msg_header;<br> usb_msg->l1h = msgb_put(usb_msg, sizeof(*usb_msg_header));<br> usb_msg_header = (struct simtrace_msg_hdr *) usb_msg->l1h;<br> memset(usb_msg_header, 0, sizeof(*usb_msg_header));<br> usb_msg_header->msg_class = SIMTRACE_MSGC_SNIFF;<br> usb_msg_header->msg_type = SIMTRACE_MSGT_SNIFF_ATR;<br> usb_msg->l2h = usb_msg->l1h + sizeof(*usb_msg_header);<br> struct sniff_data *usb_sniff_data_atr;<br> usb_sniff_data_atr = (struct sniff_data *) msgb_put(usb_msg, sizeof(*usb_sniff_data_atr));<br> usb_sniff_data_atr->complete = true;<br> usb_sniff_data_atr->length = atr_i;<br> uint8_t *data = msgb_put(usb_msg, usb_sniff_data_atr->length);<br> memcpy(data, atr, atr_i);<br> usb_msg_header->msg_len = msgb_length(usb_msg);<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">see my comments above on how to simplify this</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/9873/5/firmware/libcommon/source/sniffer.c@450">Patch Set #5, Line 450:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;">struct simtrace_msg_hdr *usb_msg_header;<br> usb_msg->l1h = msgb_put(usb_msg, sizeof(*usb_msg_header));<br> usb_msg_header = (struct simtrace_msg_hdr *) usb_msg->l1h;<br> memset(usb_msg_header, 0, sizeof(*usb_msg_header));<br> usb_msg_header->msg_class = SIMTRACE_MSGC_SNIFF;<br> usb_msg_header->msg_type = SIMTRACE_MSGT_SNIFF_PPS;<br> usb_msg->l2h = usb_msg->l1h + sizeof(*usb_msg_header);<br> struct sniff_data *usb_sniff_data_pps;<br> usb_sniff_data_pps = (struct sniff_data *) msgb_put(usb_msg, sizeof(*usb_sniff_data_pps));<br> usb_sniff_data_pps->complete = true;<br> usb_sniff_data_pps->length = pps_i;<br> uint8_t *data = msgb_put(usb_msg, usb_sniff_data_pps->length);<br> memcpy(data, pps, pps_i);<br> usb_msg_header->msg_len = msgb_length(usb_msg);<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">same here: see comments above</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/9873/5/firmware/libcommon/source/sniffer.c@477">Patch Set #5, Line 477:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;">struct simtrace_msg_hdr *usb_msg_header;<br> usb_msg->l1h = msgb_put(usb_msg, sizeof(*usb_msg_header));<br> usb_msg_header = (struct simtrace_msg_hdr *) usb_msg->l1h;<br> memset(usb_msg_header, 0, sizeof(*usb_msg_header));<br> usb_msg_header->msg_class = SIMTRACE_MSGC_SNIFF;<br> usb_msg_header->msg_type = SIMTRACE_MSGT_SNIFF_FIDI;<br> usb_msg->l2h = usb_msg->l1h + sizeof(*usb_msg_header);<br> struct sniff_fidi *usb_sniff_fidi;<br> usb_sniff_fidi = (struct sniff_fidi *) msgb_put(usb_msg, sizeof(*usb_sniff_fidi));<br> usb_sniff_fidi->fidi = fidi;<br> usb_msg_header->msg_len = msgb_length(usb_msg);<br> usb_buf_submit(usb_msg);<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">and once more</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/9873/5/firmware/libcommon/source/sniffer.c@612">Patch Set #5, Line 612:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;">struct simtrace_msg_hdr *usb_msg_header;<br> usb_msg->l1h = msgb_put(usb_msg, sizeof(*usb_msg_header));<br> usb_msg_header = (struct simtrace_msg_hdr *) usb_msg->l1h;<br> memset(usb_msg_header, 0, sizeof(*usb_msg_header));<br> usb_msg_header->msg_class = SIMTRACE_MSGC_SNIFF;<br> usb_msg_header->msg_type = SIMTRACE_MSGT_SNIFF_ATR;<br> usb_msg->l2h = usb_msg->l1h + sizeof(*usb_msg_header);<br> struct sniff_data *usb_sniff_data_tpdu;<br> usb_sniff_data_tpdu = (struct sniff_data *) msgb_put(usb_msg, sizeof(*usb_sniff_data_tpdu));<br> usb_sniff_data_tpdu->complete = true;<br> usb_sniff_data_tpdu->length = tpdu_packet_i;<br> uint8_t *data = msgb_put(usb_msg, usb_sniff_data_tpdu->length);<br> memcpy(data, tpdu_packet, tpdu_packet_i);<br> usb_msg_header->msg_len = msgb_length(usb_msg);<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">and yet again</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/9873">change 9873</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://gerrit.osmocom.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.osmocom.org/9873"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: simtrace2 </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-MessageType: comment </div>
<div style="display:none"> Gerrit-Change-Id: Ice7817480705f2124b08c1ff9a8826558b6d8b2b </div>
<div style="display:none"> Gerrit-Change-Number: 9873 </div>
<div style="display:none"> Gerrit-PatchSet: 5 </div>
<div style="display:none"> Gerrit-Owner: Kévin Redon <kredon@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Harald Welte <laforge@gnumonks.org> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder </div>
<div style="display:none"> Gerrit-Comment-Date: Wed, 04 Jul 2018 12:16:19 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: Yes </div>