Wireshark-bugs: [Wireshark-bugs] [Bug 7729] Full support of RFC2428 in FTP dissector
https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=7729
--- Comment #4 from Martin Kaiser <wireshark@xxxxxxxxx> 2012-09-16 05:29:00 PDT ---
Hi,
some comments about the patch
+ { &hf_ftp_epsv_ip,
+ { "Extended passive IPv4 address", "ftp.epsv.ip",
+ FT_IPv6, BASE_NONE, NULL, 0,
+ "Extended passive server IPv4 address", HFILL }},
is probably a typo at should be FT_IPv4.
$ ./tools/checkAPIs.pl epan/dissectors/packet-ftp.c
Error: the blurb for hf_ftp_eprt_af ("ftp.eprt.af") matches the field name in
epan/dissectors/packet-ftp.c
+ { &hf_ftp_eprt_af,
+ { "Extended active address family", "ftp.eprt.af",
+ FT_UINT8, BASE_DEC, VALS(eprt_af_vals), 0,
+ "Extended active address family", HFILL }},
It seems that name and blurb mustn't be identical strings. Set blurb to NULL to
make name and blurb identical.
+ if (delimiters_seen == 2) { /* end of address family field */
+ gint fieldlen = n - lastn - 1;
+ gchar *field = p + lastn + 1;
+ gchar *af_str = g_strndup(field, fieldlen);
+
+ eprt_af = atoi(af_str);
+
I might be wrong about this one but my understanding is that it's not portable
to initialize variables with non-constant values in the declaration
(README.developer, section 1.1.1)
I did not get to the bottom of the index handling. Can you be sure that
fieldlen will never be -1 or would it be better to check this explicitly?
+ for (e = p + linelen;e != NULL && e != p && *e != '\n' && *e != '\r';e--);
Although this is correct, I'd prefer making it more explicit, like
for (e = p + linelen;e != NULL && e != p && *e != '\n' && *e != '\r';e--) {
}
Best regards,
Martin
--
Configure bugmail: https://bugs.wireshark.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching all bug changes.
