On 2016-08-11 at 00:57:03 +0200, Vadim Kochan <vadi...@gmail.com> wrote: > Extended 'struct packet_dyn' with proto fields which has > dynamically changing values at runtime. > > Implement incrementing of proto field at runtime with min & max > parameters, by default if the 'min' parameter is not specified > then original value is used. For fields which len is greater > than 4 - last 4 bytes are incremented as unsigned int value (this > trick is used for increment MAC/IPv6 addresses). > > Added 'field_changed' callback for proto header which > may be used for check if csum updating is needed. This callback > is called after field was changed at runtime. > > Added 'packet_update' callback to let proto header know > when to apply final proto header changes at runtime (csum update).
I'd split this patch into two: 1) Introduce generic infrastructure to dynamically update protocol fields at runtime (which will then be used by the inc and rnd functionality) 2) Increment functionality And as mentioned in [1], I'd like to do the proto_ops/proto_hdr split before applying this series as it further extends the current struct proto_hdr with both static and runtime information. [1] https://groups.google.com/forum/#!msg/netsniff-ng/20RvwJdh50Y/eMkbmKSaBgAJ I'll send the RFC patch later today and would like to ask you to - if the patch makes sense to you and you agree with it - to rebase v3 of this series on top of it. Some more minor comments inline, but in general this looks good. > Signed-off-by: Vadim Kochan <vadi...@gmail.com> > --- > trafgen.c | 9 +++++++ > trafgen_conf.h | 7 +++++ > trafgen_proto.c | 84 > +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > trafgen_proto.h | 26 ++++++++++++++++++ > 4 files changed, 126 insertions(+) > > diff --git a/trafgen.c b/trafgen.c > index b76b5d7..553dfa5 100644 > --- a/trafgen.c > +++ b/trafgen.c > @@ -619,6 +619,15 @@ static inline void packet_apply_dyn_elements(int idx) > apply_randomizer(idx); > apply_csum16(idx); > } > + > + if (packet_dyn_has_fields(&packet_dyn[idx])) { > + uint32_t i; > + > + for (i = 0; i < packet_dyn[idx].flen; i++) > + proto_field_dyn_apply(packet_dyn[idx].fields[i]); > + > + proto_packet_update(idx); > + } > } > > static void xmit_slowpath_or_die(struct ctx *ctx, unsigned int cpu, unsigned > long orig_num) > diff --git a/trafgen_conf.h b/trafgen_conf.h > index 934f8fe..7f3616c 100644 > --- a/trafgen_conf.h > +++ b/trafgen_conf.h > @@ -49,6 +49,8 @@ struct packet_dyn { > size_t rlen; > struct csum16 *csum; > size_t slen; > + struct proto_field **fields; > + uint32_t flen; This should be size_t as well. > }; > > static inline bool packet_dyn_has_elems(struct packet_dyn *p) > @@ -61,6 +63,11 @@ static inline bool packet_dyn_has_only_csums(struct > packet_dyn *p) > return (p->clen == 0 && p->rlen == 0 && p->slen); > } > > +static inline bool packet_dyn_has_fields(struct packet_dyn *p) > +{ > + return !!p->flen; The double negation shouldn't be necessary. > +} > + > extern void compile_packets_str(char *str, bool verbose, unsigned int cpu); > extern void compile_packets(char *file, bool verbose, unsigned int cpu, > bool invoke_cpp, char *const cpp_argv[]); > diff --git a/trafgen_proto.c b/trafgen_proto.c > index 4219794..72dbad3 100644 > --- a/trafgen_proto.c > +++ b/trafgen_proto.c > @@ -407,6 +407,19 @@ void protos_init(const char *dev) > protos_l4_init(); > } > > +void proto_packet_update(uint32_t idx) > +{ > + struct packet *pkt = packet_get(idx); > + ssize_t i; > + > + for (i = pkt->headers_count - 1; i >= 0; i--) { > + struct proto_hdr *hdr = pkt->headers[i]; > + > + if (hdr->packet_update) > + hdr->packet_update(hdr); > + } > +} > + > void proto_packet_finish(void) > { > struct proto_hdr **headers = current_packet()->headers; > @@ -421,3 +434,74 @@ void proto_packet_finish(void) > p->packet_finish(p); > } > } > + > +static inline unsigned int field_inc(struct proto_field *field) return type should be uint32_t > +{ > + uint32_t min = field->func.min; > + uint32_t max = field->func.max; > + uint32_t val = field->func.val; > + uint32_t inc = field->func.inc; > + uint32_t next; > + > + next = (val + inc) % (max + 1); > + field->func.val = max(next, min); > + > + return val; > +} > + > +static void field_inc_func(struct proto_field *field) > +{ > + if (field->len == 1) { > + proto_field_set_u8(field->hdr, field->id, field_inc(field)); > + } else if (field->len == 2) { > + proto_field_set_be16(field->hdr, field->id, field_inc(field)); > + } else if (field->len == 4) { > + proto_field_set_be32(field->hdr, field->id, field_inc(field)); > + } else if (field->len > 4) { > + uint8_t *bytes = __proto_field_get_bytes(field); > + > + bytes += field->len - 4; > + > + *(uint32_t *)bytes = bswap_32(field_inc(field)); > + } > +} > + > +void proto_field_func_add(struct proto_hdr *hdr, uint32_t fid, > + struct proto_field_func *func) > +{ > + struct proto_field *field = proto_field_by_id(hdr, fid); > + > + bug_on(!func); > + > + memcpy(&field->func, func, sizeof(*func)); I'd rather have an explicit assignment of the respective members here, i.e. type, inc, min, max. And set val and update_field explicitely to 0/NULL. > + > + field->func.max = field->func.max ?: UINT32_MAX - 1; Shouldnt't this be UINT32_MAX (without the -1)? > + > + if (func->type & PROTO_FIELD_FUNC_INC) { > + if (func->type & PROTO_FIELD_FUNC_MIN) > + field->func.val = func->min; > + else if (field->len == 1) > + field->func.val = proto_field_get_u8(hdr, fid); > + else if (field->len == 2) > + field->func.val = proto_field_get_u16(hdr, fid); > + else if (field->len == 4) > + field->func.val = proto_field_get_u32(hdr, fid); > + else if (field->len > 4) { > + uint8_t *bytes = __proto_field_get_bytes(field); > + > + bytes += field->len - 4; > + field->func.val = bswap_32(*(uint32_t *)bytes); > + } > + > + field->func.update_field = field_inc_func; > + } > +} > + > +void proto_field_dyn_apply(struct proto_field *field) > +{ > + if (field->func.update_field) > + field->func.update_field(field); > + > + if (field->hdr->field_changed) > + field->hdr->field_changed(field->hdr, field); > +} > diff --git a/trafgen_proto.h b/trafgen_proto.h > index dbba700..2b48c68 100644 > --- a/trafgen_proto.h > +++ b/trafgen_proto.h > @@ -27,8 +27,24 @@ enum proto_layer { > PROTO_L4, > }; > > +struct proto_field; > struct proto_hdr; > > +enum proto_field_func_t { > + PROTO_FIELD_FUNC_INC = 1 << 0, > + PROTO_FIELD_FUNC_MIN = 1 << 1, > +}; > + > +struct proto_field_func { > + enum proto_field_func_t type; > + uint32_t min; > + uint32_t max; > + int32_t inc; > + uint32_t val; > + > + void (* update_field)(struct proto_field *field); > +}; > + > struct proto_field { > uint32_t id; > size_t len; > @@ -37,6 +53,7 @@ struct proto_field { > /* might be negative (e.g. VLAN TPID field) */ > int16_t offset; > > + struct proto_field_func func; > bool is_set; > uint16_t pkt_offset; > struct proto_hdr *hdr; > @@ -55,7 +72,9 @@ struct proto_hdr { > > void (*header_init)(struct proto_hdr *hdr); > void (*header_finish)(struct proto_hdr *hdr); > + void (*field_changed)(struct proto_hdr *hdr, struct proto_field *field); Is it necessary to pass the struct proto_hdr here? Isn't it always field->hdr? > void (*packet_finish)(struct proto_hdr *hdr); > + void (*packet_update)(struct proto_hdr *hdr); > void (*set_next_proto)(struct proto_hdr *hdr, enum proto_id pid); > }; > > @@ -65,6 +84,8 @@ extern void proto_header_register(struct proto_hdr *hdr); > extern struct proto_hdr *proto_header_init(enum proto_id pid); > extern void proto_header_finish(struct proto_hdr *hdr); > extern void proto_packet_finish(void); > +extern void proto_packet_update(uint32_t idx); > + > extern struct proto_hdr *proto_lower_default_add(struct proto_hdr *hdr, > enum proto_id pid); > > @@ -111,4 +132,9 @@ extern void proto_field_set_default_dev_ipv4(struct > proto_hdr *hdr, uint32_t fid > extern void proto_field_set_dev_ipv6(struct proto_hdr *hdr, uint32_t fid); > extern void proto_field_set_default_dev_ipv6(struct proto_hdr *hdr, uint32_t > fid); > > +extern void proto_field_dyn_apply(struct proto_field *field); > + > +extern void proto_field_func_add(struct proto_hdr *hdr, uint32_t fid, > + struct proto_field_func *func); > + > #endif /* TRAFGEN_PROTO_H */ > -- > 2.9.2 > -- 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.