On Tue, Aug 9, 2016 at 5:11 PM, Tobias Klauser <[email protected]> wrote: > Hi Vadim > > On 2016-07-26 at 21:35:05 +0200, Vadim Kochan <[email protected]> wrote: >> Implemented 'dinc' and 'drnd' functions to be used for proto fields, >> and generate values at runtime. Parsing of proto field values >> for unified to make extending of field functions more easier w/o >> copy/paste similar rules for each proto field. Instead of that >> the field_expr struct is used to keep specified field and value which >> might be a func in the following form: >> >> <proto>(<field>=<func>()) >> >> Fields which has dynamic functions are stored in packet_dyn structure and >> generated after each packet run. After fields are updated the L3/L4 headers >> updates its csum if it is needed. >> >> Changed field lookup logic to make field set/get operations a little bit >> faster by >> using field id as index in the array, as usually fields are defined regarding >> its index. >> >> Example: >> >> { eth(sa=11:22:33:44:55:66, sa=dinc()), udp(sp=drnd(), dp=drnd()) } >> >> Vadim Kochan (15): >> trafgen: Move applying dynamic elements to function >> trafgen: proto: Reference to packet from proto_hdr struct >> trafgen: proto: Move proto headers into packet >> trafgen: proto: Force field id to be index in array >> trafgen: proto: Increment proto field at runtime >> trafgen: proto: Randomize proto field at runtime >> trafgen: ipv4: Update csum at runtime if needed >> trafgen: icmpv4: Update csum at runtime if needed >> trafgen: icmpv6: Update csum at runtime if needed >> trafgen: udp: Update csum at runtime if needed >> trafgen: tcp: Update csum at runtime if it needed >> trafgen: parser: Unify proto field value parsing >> trafgen: parser: Add support of 'dinc' function for proto fields >> trafgen: parser: Add 'drnd()' function for proto fields >> trafgen: man: Add description for 'dinc' and 'drnd' field functions > > While reviewing (and partially applying) this series I noticed something > about the current implementation of trafgen protos which I find quite > odd: > > The struct proto_hdr is on one hand used to statically define protocol > behavior (i.e. all the *_hdr definitions in trafgen_l{2,3,4}.c) and > these are then memcpy()'ed for each created packed at parse time and > then used to store dynamically changing information at parse/run time. > This way, members such as the proto id, layer and the pointers callback > functions get copied for each created packet (in addition to the other > fields which get changed during parsing) and static/dynamic information > get mixed and we e.g. can't make the protocol definitions const to > ensure they'll not get changed. > > Rather than copying the struct proto_hdr for every packet, I think it > would be cleaner to separate it into two structs, one for the statically > defined protocol (let's call it struct proto_ops) and one for the > dynamic information (this would be the 'new', reduced struct proto_hdr). > The struct proto_hdr would then have a pointer to the corresponding > proto_ops instance and could use it to execute callbacks. The new > structs would look something like this: > > struct proto_ops { > enum proto_id id; > enum proto_layer layer; > > void (*header_init)(struct proto_hdr *hdr); > void (*header_finish)(struct proto_hdr *hdr); > void (*packet_finish)(struct proto_hdr *hdr); > void (*set_next_proto)(struct proto_hdr *hdr, enum proto_id pid); > }; > > struct proto_hdr { > struct proto_ops *ops; > uint16_t pkt_offset; > uint32_t pkt_id; > struct proto_field *fields; > size_t fields_count; > size_t len; > }; > > Maybe I missed something important, why this wouldn't work or isn't a > good idea? But if not, and if you agree I'd like to make this change > before the reroll of your series. > > If that's OK, I'll prepare the series and send it as an RFC to you, > Daniel and the list for review. > > -- > 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 [email protected]. > For more options, visit https://groups.google.com/d/optout.
Hi Tobias, proto_ops sounds good, *BUT* would not it better to do this refactoring ( I can do this by myself ) after applying this v2 series (or vX if needed), really it will save me from conflicts fighting :), of course I dont think the conflicts will be serious but because of this series is quite big - I'd like to focus manly on fixes which are related to this series, if you dont mind. Regarding why I used one proto_hdr to keep it all here, well I did not take it into account, and I was mostly focused on if it is possible to implement it and how it will work and if it can be simple as possible and +/- to make able to create some generic API, so there was no some serious reason, may be after I got something working I did not want to do such refactoring ( I do not remember ) just do not break existing code. 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 [email protected]. For more options, visit https://groups.google.com/d/optout.
