Hi Vadim

On 2016-07-26 at 21:35:05 +0200, Vadim Kochan <vadi...@gmail.com> 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 netsniff-ng+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Reply via email to