Change in simtrace2[master]: sniffer USB: implement USB communication and send parsed messages

This is merely a historical archive of years 2008-2021, before the migration to mailman3.

A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.

Harald Welte gerrit-no-reply at lists.osmocom.org
Wed Jul 4 12:16:19 UTC 2018


Harald Welte has posted comments on this change. ( https://gerrit.osmocom.org/9873 )

Change subject: sniffer USB: implement USB communication and send parsed messages
......................................................................


Patch Set 5: Code-Review-1

(7 comments)

https://gerrit.osmocom.org/#/c/9873/5/firmware/libcommon/source/sniffer.c
File firmware/libcommon/source/sniffer.c:

https://gerrit.osmocom.org/#/c/9873/5/firmware/libcommon/source/sniffer.c@206
PS5, Line 206: struct simtrace_msg_hdr *usb_msg_header;
             : 	usb_msg->l1h = msgb_put(usb_msg, sizeof(*usb_msg_header));
             : 	usb_msg_header = (struct simtrace_msg_hdr *) usb_msg->l1h;
             : 	memset(usb_msg_header, 0, sizeof(*usb_msg_header));
             : 	usb_msg_header->msg_class = SIMTRACE_MSGC_SNIFF;
             : 	usb_msg_header->msg_type = SIMTRACE_MSGT_SNIFF_CHANGE;
             : 	usb_msg->l2h = usb_msg->l1h + sizeof(*usb_msg_header)
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.


https://gerrit.osmocom.org/#/c/9873/5/firmware/libcommon/source/sniffer.c@216
PS5, Line 216: usb_msg_header->msg_len = msgb_length(usb_msg);
If you re-ordered it to:
* first msgb_put the usb_sniff change
* then msgb_push the usb_msg_header in front
it would not be needed to update the length at the end.

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)

This way the API becomes super easy to use (and get right)


https://gerrit.osmocom.org/#/c/9873/5/firmware/libcommon/source/sniffer.c@275
PS5, Line 275: 		TRACE_ERROR("ATR buffer overflow\n\r");
this might be something worth counting and/or reporting to the host?  Keep in mind, the normal user has nothing connected to the UART...


https://gerrit.osmocom.org/#/c/9873/5/firmware/libcommon/source/sniffer.c@295
PS5, Line 295: struct simtrace_msg_hdr *usb_msg_header;
             : 	usb_msg->l1h = msgb_put(usb_msg, sizeof(*usb_msg_header));
             : 	usb_msg_header = (struct simtrace_msg_hdr *) usb_msg->l1h;
             : 	memset(usb_msg_header, 0, sizeof(*usb_msg_header));
             : 	usb_msg_header->msg_class = SIMTRACE_MSGC_SNIFF;
             : 	usb_msg_header->msg_type = SIMTRACE_MSGT_SNIFF_ATR;
             : 	usb_msg->l2h = usb_msg->l1h + sizeof(*usb_msg_header);
             : 	struct sniff_data *usb_sniff_data_atr;
             : 	usb_sniff_data_atr = (struct sniff_data *) msgb_put(usb_msg, sizeof(*usb_sniff_data_atr));
             : 	usb_sniff_data_atr->complete = true;
             : 	usb_sniff_data_atr->length = atr_i;
             : 	uint8_t *data = msgb_put(usb_msg, usb_sniff_data_atr->length);
             : 	memcpy(data, atr, atr_i);
             : 	usb_msg_header->msg_len = msgb_length(usb_msg);
see my comments above on how to simplify this


https://gerrit.osmocom.org/#/c/9873/5/firmware/libcommon/source/sniffer.c@450
PS5, Line 450: struct simtrace_msg_hdr *usb_msg_header;
             : 	usb_msg->l1h = msgb_put(usb_msg, sizeof(*usb_msg_header));
             : 	usb_msg_header = (struct simtrace_msg_hdr *) usb_msg->l1h;
             : 	memset(usb_msg_header, 0, sizeof(*usb_msg_header));
             : 	usb_msg_header->msg_class = SIMTRACE_MSGC_SNIFF;
             : 	usb_msg_header->msg_type = SIMTRACE_MSGT_SNIFF_PPS;
             : 	usb_msg->l2h = usb_msg->l1h + sizeof(*usb_msg_header);
             : 	struct sniff_data *usb_sniff_data_pps;
             : 	usb_sniff_data_pps = (struct sniff_data *) msgb_put(usb_msg, sizeof(*usb_sniff_data_pps));
             : 	usb_sniff_data_pps->complete = true;
             : 	usb_sniff_data_pps->length = pps_i;
             : 	uint8_t *data = msgb_put(usb_msg, usb_sniff_data_pps->length);
             : 	memcpy(data, pps, pps_i);
             : 	usb_msg_header->msg_len = msgb_length(usb_msg);
same here: see comments above


https://gerrit.osmocom.org/#/c/9873/5/firmware/libcommon/source/sniffer.c@477
PS5, Line 477: struct simtrace_msg_hdr *usb_msg_header;
             : 	usb_msg->l1h = msgb_put(usb_msg, sizeof(*usb_msg_header));
             : 	usb_msg_header = (struct simtrace_msg_hdr *) usb_msg->l1h;
             : 	memset(usb_msg_header, 0, sizeof(*usb_msg_header));
             : 	usb_msg_header->msg_class = SIMTRACE_MSGC_SNIFF;
             : 	usb_msg_header->msg_type = SIMTRACE_MSGT_SNIFF_FIDI;
             : 	usb_msg->l2h = usb_msg->l1h + sizeof(*usb_msg_header);
             : 	struct sniff_fidi *usb_sniff_fidi;
             : 	usb_sniff_fidi = (struct sniff_fidi *) msgb_put(usb_msg, sizeof(*usb_sniff_fidi));
             : 	usb_sniff_fidi->fidi = fidi;
             : 	usb_msg_header->msg_len = msgb_length(usb_msg);
             : 	usb_buf_submit(usb_msg);
and once more


https://gerrit.osmocom.org/#/c/9873/5/firmware/libcommon/source/sniffer.c@612
PS5, Line 612: struct simtrace_msg_hdr *usb_msg_header;
             : 	usb_msg->l1h = msgb_put(usb_msg, sizeof(*usb_msg_header));
             : 	usb_msg_header = (struct simtrace_msg_hdr *) usb_msg->l1h;
             : 	memset(usb_msg_header, 0, sizeof(*usb_msg_header));
             : 	usb_msg_header->msg_class = SIMTRACE_MSGC_SNIFF;
             : 	usb_msg_header->msg_type = SIMTRACE_MSGT_SNIFF_ATR;
             : 	usb_msg->l2h = usb_msg->l1h + sizeof(*usb_msg_header);
             : 	struct sniff_data *usb_sniff_data_tpdu;
             : 	usb_sniff_data_tpdu = (struct sniff_data *) msgb_put(usb_msg, sizeof(*usb_sniff_data_tpdu));
             : 	usb_sniff_data_tpdu->complete = true;
             : 	usb_sniff_data_tpdu->length = tpdu_packet_i;
             : 	uint8_t *data = msgb_put(usb_msg, usb_sniff_data_tpdu->length);
             : 	memcpy(data, tpdu_packet, tpdu_packet_i);
             : 	usb_msg_header->msg_len = msgb_length(usb_msg);
and yet again



-- 
To view, visit https://gerrit.osmocom.org/9873
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings

Gerrit-Project: simtrace2
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ice7817480705f2124b08c1ff9a8826558b6d8b2b
Gerrit-Change-Number: 9873
Gerrit-PatchSet: 5
Gerrit-Owner: Kévin Redon <kredon at sysmocom.de>
Gerrit-Reviewer: Harald Welte <laforge at gnumonks.org>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Comment-Date: Wed, 04 Jul 2018 12:16:19 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20180704/6bd1af4b/attachment.htm>


More information about the gerrit-log mailing list