Wireshark-bugs: [Wireshark-bugs] [Bug 7554] Add ability to add/remove packet comments using edit
https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=7554
--- Comment #3 from Niels Widger <niels@xxxxxxxxxx> 2012-08-03 06:53:05 PDT ---
(In reply to comment #2)
Hi Jakub,
Thanks for your feedback. My comments are inline below.
> Hi,
>
> Some comments:
> - please use FILE API from stdio.h, and especially fgets().
> Yours fgets() have off-by-one error when comment takes COMMENT_READ_SIZE
> bytes (NUL termination is written to buf[COMMENT_READ_SIZE]).
Sure, I'll switch over to fgets() right away, I didn't realize that was
available.
>
> - another off-by-one is in add_comment(), first comment is written at
> commentfrm[1] and not commentfrm[0].
Actually since max_commented is initialized to -1 in main() it will actually
write the first comment to commentfrm[0]. I was just copying the convention
used by add_selection(). However, it is a bit misleading so in the next
revision of the patch I'll change max_commented to start at 0.
>
>
> + comment_phdr.opt_comment = g_strdup(comment_item->comment);
> + g_free(comment_item->comment);
>
> I'd rather free memory (if you really need) at end of program,
> User can specify any packet number, (s)he can also specify frame number which
> doesn't exists. In such case we won't free memory.
Sure, I'll call a new free_comments() function at the end of main() to free
that memory instead of doing it in the main loop.
>
>
> And finally I'd use NULL comment as indicator to remove comment, instead of
> another 'remove' variable. But current version is ok for me.
The 'remove' variable is a bit redundant. I'll take it out.
I'll be posting a new version of the patch here shortly. Thanks again!
--
Configure bugmail: https://bugs.wireshark.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching all bug changes.
