LWAPP: Modernize packet parsing style. #1368
Conversation
|
Not review all changes. diff --git a/print-lwapp.c b/print-lwapp.c
index e20b77b76..73f4c410b 100644
--- a/print-lwapp.c
+++ b/print-lwapp.c
@@ -707,6 +707,8 @@ lwapp_print(netdissect_options *ndo,
ndo->ndo_protocol = "lwapp";
const u_char *cp = pptr;
u_int hdrlen = sizeof(struct lwapp_transport_header);
+
+ nd_print_protocol_caps(ndo);
/*
* The [documented] transport header begins after the [undocumented] AP
* identity if the latter is present. This is not in RFC 5412, but is
@@ -725,11 +727,8 @@ lwapp_print(netdissect_options *ndo,
uint8_t version = LWAPP_EXTRACT_VER(ver_rid_cfl);
uint8_t radio_id = LWAPP_EXTRACT_RID(ver_rid_cfl);
uint8_t flags = LWAPP_EXTRACT_CFL(ver_rid_cfl);
- if (version != LWAPP_VERSION) {
- ND_PRINT("LWAPP version %u packet not supported", version);
- return;
- }
- ND_PRINT("LWAPPv%u, %s frame",
+ ND_ICHECK_U(version, !=, LWAPP_VERSION);
+ ND_PRINT("v%u, %s frame",
version,
(flags & LWAPP_C_BIT) ? "Control" : "Data");
if (ndo->ndo_vflag) |
The first revision of print-lwapp.c supposedly implemented draft-ohara-capwap-lwapp-04, all subsequent revisions made various code clean-ups only and the differences between the I-D and RFC 5412 seem to be purely editorial. However, the current implementation has substantial discrepancies with the specification. Moreover, several aspects of the specification are plainly not implementable due to various technical issues. The matter is, RFC 5412 was published for posterity rather than implementation: CAPWAP superseded LWAPP before the latter was complete. Before time is good for a more substantial reorganization of tcpdump decoders, clean the LWAPP decoder up once more and make it follow the specification better where practicable. Improve various comments. Define ND_LONGJMP_FROM_TCHECK. In lwapp_transport_header rename version to ver_rid_cfl and rearrange the associated macros for clarity. lwapp_control_print() and lwapp_data_print() each parse an LWAPP transport header in a slightly different way. Move the common code to a new function, lwapp_print(), and make it the only function exported from the decoder. To print the value of "length", the old common code used UDP payload length for ndo_vflag == 0 and LWAPP transport header payload length otherwise; in the new function make it the latter in both cases. Ibid., add diagnostics to flag unexpected fragmentation in the transport header. Ibid., use the transport header C bit instead of UDP ports to tell control and data frames apart. Rename lwapp_message_header to lwapp_msgelem_header to match the data it represents. Add a registry of message elements and specify the minimum length where possible. Implement a new function, permitted_msg_elem(), to tell whether a message element is valid for a message type. Fix lwapp_control_print() to iterate over message elements rather than messages and use the new props to validate and to print control messages a bit better. In lwapp_data_print() remove the mention of AP identity field because the function does not actually implement it. Ibid., do not require the transport payload length to be greater or equal to the transport header length and do not subtract the latter from the former for the hex dump because the former does not include the latter (this recovers the overlooked last 6 bytes of the payload in the output); require the transport payload end and the UDP payload end to be the same. With these changes made lwapp_data_print() becomes equivalent to just print_unknown_data(), so remove the former function. Since these clean-ups affect most of the code, in the same go reindent the entire file in a more sensible way. Update the tests.
|
Thank you. This revision incorporates the suggested changes, loses a single-use variable and adds a couple empty lines. |
|
Why the LWAPP tests are defined in tests/TESTrun and not in tests/TESTLIST? They don't seems "special" (i.e no "skip"). |
|
Same question for ARCnet tests added in May. |
|
Generally I add all new tests to the native Perl test definition space to avoid an extra conversion step in case a test needs to become more sophisticated later. In this specific case it would make sense eventually to use LWAPP to work a policy out for "antique" decoders, in which case the decoder would become off-by-default and the associated tests would become conditional. |
Yes, some Wireshark dissectors have the same problem, where an exception is thrown before they get the chance to report on data that is present. |
"Antique" in the sense of still using the old-style parsing style? Presumably the long-term goal (for 5.0?) is to modernize all decoders. |
It took much longer than I originally expected.