Wireshark-dev: Re: [Wireshark-dev] [Wireshark-commits] rev 45313: /trunk/ /trunk/doc/: reorderc

From: Martin Mathieson <martin.r.mathieson@xxxxxxxxxxxxxx>
Date: Fri, 5 Oct 2012 13:36:58 -0400


On Fri, Oct 5, 2012 at 1:25 PM, Jakub Zawadzki <darkjames-ws@xxxxxxxxxxxx> wrote:
On Fri, Oct 05, 2012 at 06:11:02PM +0200, Joerg Mayer wrote:
> On Thu, Oct 04, 2012 at 06:24:22PM +0000, martinm@xxxxxxxxxxxxx wrote:
> > http://anonsvn.wireshark.org/viewvc/viewvc.cgi?view=rev&revision=45313
> >
> > User: martinm
> > Date: 2012/10/04 11:24 AM
> >
> > Log:
> >  This is basically a rewrite from Jakub Zawadzki.
> >  Rather than store the FrameRecord entries in a sorted linked list,
> >  instead use an unsorted GPtrArray, then sort it all at once.
> >
> >  Also, there is no longer the option to limit the amount of sorting (and memory
> >  used), but a new option means we can avoid writing the output file
> >  altogether if the input file is found already to be in order.
>
> Sigh - that basically makes it final that reordercap will not be integrated
> into editcap :-(

Why?
Looking at editcap sourcse I don't see any use of linked list?

For duplicate there's array with fixed size (so it'd also benefit from some -l option, which I plan to reintroduce to reordercap),
which could be replaced also with GPtrArray.

And less code -> easier to integrate (was: 404LOC, now: 278LOC)

Cheers,
 Jakub.


I think Joerg might have meant that adding a man page makes it look more final that it will not be integrated?

editcap.c already got a lot of options, and I find it difficult to read.  Most of these options *could* be applied at the same time as reordering, but I don't think I'd want to.  Someone else might do.

The main loop is like this:

while not finished input file
    read next frame from input file
    mess around with it based upon options
    write changed frame to output file
end while

whereas reordercap needs to buffer details about the frames it has read but not yet written to disk.

I don't like the way that the transformations are all done in-line in the loop.  I'd prefer if the various options were broken out into functions where pretty took the current frame's struct wtap_pkthdr* and gave back the modified version.   We could also abstract out the way the processed frame is output (usually dumped to an output file, except in reordercap its highlights would be sorted/stored, and used later to go back and re-read, modify, then dump).

I could do this, but I'm not convinced it'll be easy to read/maintain.  And for the next year I'll think every bug in editcap is my fault (and I'll probably be right).

Martin