Re: [Wireshark-commits] rev 43908: /trunk/epan/dissectors/ /trunk/epan/dissectors/: packet-ftp.c

[Martin Mathieson <martin.r.mathieson () googlemail com>]

Thanks Alexis,

I submitted this change (losing the tree) in r43929.

As for just using ftp.data - I don't feel strongly about it.  It is
unusual, but anyone who ever looks at FTP is probably used to it.
Also, it might be convenient to be able to build display display filters
that will only apply to the control or data part of FTP.  e.g. 'ftp
contains "PASS"' currently won't look inside the transferred files, but it
would after making this change.

Martin

On Mon, Jul 23, 2012 at 10:23 AM, Alexis La Goutte <
alexis.lagoutte () gmail com> wrote:

> Hi Martin,
>
> It look good for me.
>
> But you need to remove "ftp_data_tree = proto_item_add_subtree(ti,
> ett_ftp_data);", it is no longer use !
>
> Also why use ftp and ftp-data, it is not better to use a ftp.data (filter)
> ? (and known ftp and ftp-data is not the same protocol...)
>
> Alexis
>
>
>
> On Mon, Jul 23, 2012 at 3:42 AM, Martin Mathieson <
> martin.r.mathieson () googlemail com> wrote:
>
> > I think I'd like to change it to what the attached patch does.
> >
> > i.e. have a quick look to see if the first few characters are printable.
> > - if yes, show the string (but as before not formatting more text than
> > will be used)
> > - if no, just add the details of how many bytes are in the segment
> > In both cases, I append the details to the ftp-data root itself, rather
> > than make that awkward call to proto_item_add_text().  This saves you
> > opening up the tree (there is now nothing inside it).  And you can still
> > make expressions like:
> >       ftp-data contains "PASS"
> >       ftp-data contains 00:01:02
> >
> > Unless anyone feels strongly about it I'll submit this in a day or 2.
>
> > Martin
>
> > On Sun, Jul 22, 2012 at 7:56 AM, Joerg Mayer <jmayer () loplof de> wrote:
> >
> > > Maybe just calling packet-data is the right way to go here?
> > >
> > >  ciao
> > >       Jörg
> > >
> > > On Sun, Jul 22, 2012 at 04:44:17AM +0000, martinm () wireshark org wrote:
> > > > http://anonsvn.wireshark.org/viewvc/viewvc.cgi?view=rev&revision=43908
> > > >
> > > > User: martinm
> > > > Date: 2012/07/21 09:44 PM
> > > >
> > > > Log:
> > > >  Calling tvb_format_text() for binary data segments (c1400 bytes) was
> > > >  *very* slow (in a profiled run where FTP data is carried over LTE
> > > >  MAC/RLC/PDCP/IP/TCP, this one function call was 20% of overall
> > > runtime).
> > > >  Have limited to call to ITEM_LABEL_LENGTH, as that is all that
> > > >  will be displayed anyway.  As per comment, I'm not convinced that
> > > doing
> > > >  this for binary FTP data segments is worthwhile at all.  It doesn't
> > > >  even display as hex.
> > > >
> > > > Directory: /trunk/epan/dissectors/
> > > >   Changes    Path            Action
> > > >   +6 -3      packet-ftp.c    Modified
> > > ___________________________________________________________________________
> > > > Sent via:    Wireshark-commits mailing list <
> > > wireshark-commits () wireshark org>
> > > > Archives:    http://www.wireshark.org/lists/wireshark-commits
> > > > Unsubscribe: https://wireshark.org/mailman/options/wireshark-commits
> > > >              mailto:wireshark-commits-request () wireshark org
> > > ?subject=unsubscribe
> > >
> > > --
> > > Joerg Mayer                                           <jmayer () loplof de>
> > > We are stuck with technology when what we really want is just stuff that
> > > works. Some say that should read Microsoft instead of technology.
> > >
> > > ___________________________________________________________________________
> > > Sent via:    Wireshark-dev mailing list <wireshark-dev () wireshark org>
> > > Archives:    http://www.wireshark.org/lists/wireshark-dev
> > > Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev
> > >              mailto:wireshark-dev-request () wireshark org
> > > ?subject=unsubscribe
> >
> >
> >
> >
> > ___________________________________________________________________________
> > Sent via:    Wireshark-dev mailing list <wireshark-dev () wireshark org>
> > Archives:    http://www.wireshark.org/lists/wireshark-dev
> > Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev
> >              mailto:wireshark-dev-request () wireshark org
> > ?subject=unsubscribe
>
>
> ___________________________________________________________________________
> Sent via:    Wireshark-dev mailing list <wireshark-dev () wireshark org>
> Archives:    http://www.wireshark.org/lists/wireshark-dev
> Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev
>              mailto:wireshark-dev-request () wireshark org
> ?subject=unsubscribe
___________________________________________________________________________
Sent via:    Wireshark-dev mailing list <wireshark-dev () wireshark org>
Archives:    http://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev
             mailto:wireshark-dev-request () wireshark org?subject=unsubscribe