On Wed, Dec 21, 2016 at 10:17:17AM +0100, Tobias Klauser wrote:
> On 2016-12-20 at 22:10:54 +0100, Vadim Kochan <vadi...@gmail.com> wrote:
> > On Tue, Dec 20, 2016 at 12:33:47PM +0100, Tobias Klauser wrote:
> > > On 2016-12-18 at 10:52:49 +0100, Vadim Kochan <vadi...@gmail.com> wrote:
> > > > Extend proto field expression to:
> > > >
> > > > proto_field[{index}:{len}] = {func}
> > >
> > > I like the idea behind this very much, but I'm not particularly happy
> > > with the syntax of this.
> > >
> > > First, I find it a bit confusing that the length is specified and not
> > > the end index (as at least I would assume it i.e. like array indexing in
> > > Python works).
> >
> > Well, I do not thing we need to rely on Python's syntax, actually
> > I used such syntax from pcap filter (man pcap-filter):
>
> No, I don't think we need Python's syntax either. It's just what I'm
> currently more used to - so it was rather a statement of personal
> preference. Annd I didn't really know about the pcap filter synatax. In
> any case, I'm also fine with how you currently implemented it.
>
> ICould you please mention the fact that it is inspired by pcap
> filter syntax in the man page?
>
> > ...
> >
> > To access data inside the packet, use the following syntax:
> > proto [ expr : size ] Proto is one of ether, fddi, tr, wlan, ppp,
> > slip, link, ip, arp, rarp, tcp, udp, icmp, ip6 or radio, and indicates
> > the protocol layer for the index operation. (ether, fddi, wlan, tr,
> > ppp, slip and link all refer to the link layer. radio refers to the
> > "radio header" added to some 802.11 captures.) Note that tcp, udp and
> > other upper-layer protocol types only apply to IPv4, not IPv6 (this will
> > be fixed in the future). The byte offset, relative to the indicated
> > protocol layer, is given by expr. Size is optional and indicates the
> > number of bytes in the field of interest; it can be either one, two, or
> > four, and defaults to one. The length operator, indicated by the
> > keyword len, gives the length of the packet.
> >
> > ...
> > >
> > > Second, the need to mention a field twice with
> > > something like:
> > >
> > > eth(saddr=aa:bb:cc:dd:ee:ff, saddr[0]=dinc())
> > >
> > > isn't very intuitive to understand IMO. And moreover, from the existing
> > > syntax I'd assume some kind of function call semantics (like in C) with
> > > the fields being parameters to the function. But indexing a function
> > > parameter isn't really what I'm used to (but then again this is just
> > > personal taste).
> >
> > Hm, to reduce using field assignment twice I think it is possible to
> > extend proto dynamic functions with default value as parameter then
> > it make possible to have a mix of index + function parameter like:
> >
> > eth(sa[0]=dinc(11:22:33:44:55:66))
> >
> > with min & max drnd parameters:
> >
> > eth(sa[0]=dinc(100, 200, 11:22:33:44:55:66))
>
> I think these are more confusing than the current solution.
>
> > If you like it then can we 1st introduce current solution and after
> > just extend this to the above's syntax ?
>
> Yes, I guess we can introduce your currently proposed solution and then
> extend upon it if need be.
>
> > > And third, it's not immediately intuitive to which item the index
> > > refers, i.e. is the MAC address ab:bb:cc:dd:ee:ff afterwards or
> > > aa:bb:cc:dd:ee:00?
> >
> > Indexing starts from 0 byte in the network order, please see a below
> > examples of
> > trafgen + netsniff-ng outputs:
>
> Ok. Could you please mention this fact (that indices start from byte 0
> in network order) in the man page?
>
Hm, actually I did it in the man patch, but may be too shortly ...
> > 1) trafgen -o lo --cpus 1 -n 1 '{ eth(da[0]=dinc(), da=11:22:33:44:55:66),
> > tcp() }'
> >
> > P lo 54 1482259546s.934331688ns #1
> > [ Eth MAC (00:00:00:00:00:00 => 00:22:33:44:55:66)
> >
> > Hm, here you can see that if to specify 1st proto function and after a
> > field value then looks like the value will be overwritten by function, not
> > sure if it is a bug or feature ...
>
> Might be a bit confusing. But I'd say we can currently live with that.
> Maybe it's worth to mention this fact in the man page too, i.e. that
> functions are always applied after setting field values?
>
> > >
...
> > > But then I currently don't immediately see a better way to implement
> > > this behavior and I also see why you'd chosen the indexing to work with
> > > length rather than end index.
> > >
> > > One option might be to let the user specify multi-byte/-word fields as a
> > > C-array-like initializer list and support function calls therein, i.e.
> > > the example above could be written as:
> > >
> > > eth(saddr={ 0xaa, 0xbb, 0xcc, 0xdd, 0xee, drnd() })
> >
> > Hm, looks interesting, in such way it will possible to specify few
> > functions at 1 shot, need to think how it would be possible to specify
> > the value size.
> >
> > >
> > > which would be a bit more in line with the traditional trafgen syntax.
> > >
> > > Let me think about this a bit more...
> >
> > I think we can have 1 or 3 different solutions:
> >
> > 1) Current
> >
> > 2) Current + function param default value
> >
> > 3) Array-like
>
> I'll go ahead and apply your series except for the man page patch. Could
> you please send an update for this one incorporating the changes
> mentioned above?
>
> From there on we could then think about implementing option 3 in
> addition...
>
> Thanks a lot!
Sure I will collect your comments and use them for man page update.
Regards,
Vadim Kochan
--
You received this message because you are subscribed to the Google Groups
"netsniff-ng" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to netsniff-ng+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
