Huge thanks to our Platinum Members Endace and LiveAction,
and our Silver Member Veeam, for supporting the Wireshark Foundation and project.

Wireshark-bugs: [Wireshark-bugs] [Bug 7554] Add ability to add/remove packet comments using edit

Date: Fri, 3 Aug 2012 06:53:06 -0700 (PDT)
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.