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.orgHarald 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>