On Sat, Aug 06, 2016 at 01:36:26AM +0300, Vadim Kochan wrote: > On Wed, Aug 03, 2016 at 04:27:14PM +0200, Tobias Klauser wrote: > > On 2016-07-26 at 21:35:10 +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. > > > > > > 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). > > > > The documentation of these callbacks would also make sense where they're > > defined. > > > > > Signed-off-by: Vadim Kochan <vadi...@gmail.com> > > > --- > > > trafgen.c | 9 ++++++ > > > trafgen_conf.h | 7 ++++ > > > trafgen_proto.c | 99 > > > +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > > > trafgen_proto.h | 26 +++++++++++++++ > > > 4 files changed, 141 insertions(+) > > > > > > diff --git a/trafgen.c b/trafgen.c > > > index b76b5d7..553dfa5 100644 > > > --- a/trafgen.c > > > +++ b/trafgen.c > > > void proto_packet_finish(void) > > > { > > > struct proto_hdr **headers = ¤t_packet()->headers[0]; > > > @@ -433,3 +446,89 @@ void proto_packet_finish(void) > > > p->packet_finish(p); > > > } > > > } > > > + > > > +static inline unsigned int field_inc(struct proto_field *field) > > > +{ > > > + uint32_t val; > > > + > > > + val = field->func.val - field->func.min; > > > + val = (val + field->func.inc) % field->func.max; > > > > Shouldn't this be > > > > val = (val + field->func.inc) % (field->func.max - field->func.min + 1) > > > > to be consistent with apply_counter()?
I simplified it and now it really works well: #define max_int32(a, b) \ ({ \ int32_t _a = (int32_t) (a); \ int32_t _b = (int32_t) (b); \ _a - ((_a - _b) & ((_a - _b) >> (sizeof(int32_t) * 8 - 1))); \ }) static inline unsigned int field_inc(struct proto_field *field) { 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_int32(next, min); return val; } so max_int32(a,b) should be fast enough w/o branching. > > Sure, I tried this approach while implementing 1st version but when I > used the following case: > > trafgen/trafgen -o lo -n 10 --cpu 1 '{ eth(type=0x800), fill(0xff, 10), > dinc(5, 20, 5) }' > > then interval between 5 & 20 changes very differently. But in my version > it repeats from 5 till 20 (yes here is a little difference that initial > value is incremented immideately). Also semantic of proto dinc is > dinc(step, min, max), and I will change it to looks like low-level one - > dinc(min, max, step). > > > > > Also, I think you should probably get rid of as many pointer > > dereferences as possible in these runtime functions, i.e. store max and > > min in temporary variables. > > OK, makes sense. > > > > > > + field->func.val = val + field->func.min; > > > + > > > + return field->func.val; > > > +} > > > + > > > +static void field_inc_func(struct proto_field *field) > > > +{ > > > + if (field->len == 1) { > > > + uint8_t val; > > > + > > > + val = field_inc(field); > > > + proto_field_set_u8(field->hdr, field->id, val); > > > > Assignment on declaration please. Or even better: > > > > proto_field_set_u8(field->hdr, field->id, field_inc(field)); > > OK > > > > > > + } else if (field->len == 2) { > > > + uint16_t val; > > > + > > > + val = field_inc(field); > > > + proto_field_set_be16(field->hdr, field->id, val); > > > > Same. > OK > > > > > > + } else if (field->len == 4) { > > > + uint32_t val; > > > + > > > + val = field_inc(field); > > > + proto_field_set_be32(field->hdr, field->id, val); > > > > Same. > OK > > > > > > + } else if (field->len > 4) { > > > + uint8_t *bytes = __proto_field_get_bytes(field); > > > + uint32_t val; > > > + > > > + bytes += field->len - 4; > > > + val = field_inc(field); > > > + > > > + *(uint32_t *)bytes = bswap_32(val); > > > > This part looks really odd. Did you actually verify it produces the > > correct result on both big/little endian and for various field lengths? > > > > To be honest I don't see much use for counters going beyond UINT32_T_MAX > > (or maybe UINT64_T_MAX, which should be handled as a separate case if > > then). Or do you know of a protocol with sequence numbers (or similar) > > > 64 bit for which this would really be useful? > > Hm, may be it looks & sounds odd but I use it for incrementing MAC & IPv6 > addresses (the last 4 bytes, it might be improved to 8 bytes for x64 > arch). In the future I think to extend syntax to allow specify interval > of incrementing like: > > ipv4(saddr[0:3]=dinc()) > > or may be you have better idea, but I dont wanna extend dinc() for this. Also I think may be in case of MAC/IPv6 (field->len > 4) - use index of the current incremented byte and when it is reached 0xFF - pick the next one. But if you OK with current approach (but I am not sure you do) - I will change it in future patches. > > > > > > + } > > > +} > > > + > > > +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)); > > > + > > > + if (func->type & PROTO_FIELD_FUNC_INC) { > > > + 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); > > > + } > > > + > > > + if (field->func.max) { > > > + field->func.max = (field->func.max - field->func.min + > > > + field->func.inc) ?: > > > + 1; > > > > To me it is not entirely obvious why you do this here (I assume it has > > something to do with reducing calculations at runtime?). In any case > > this needs an explanatory comment. I removed this. > > Fuf, sorry, I can't say why I did so, probably I was fighting with > circular increment problem, anyway I can't say exactly why, need to look > closer. > > > > > > + } else { > > > + field->func.max = (uint32_t)~0; > > > + } > > > + > > > + if (func->type & PROTO_FIELD_FUNC_MIN) > > > + field->func.val = field->func.min; OK, I removed this. > > > > Why does this need an own condition? Don't we expect to start > > incrementing from the min value in any case? > > > I was thinking about to keep existing field value if MIN was not > specified. For example of MAC/IPv6 address was used, then I increment > existing bytes array. Probably it looks ugly, not sure how to simplify it. > > > > + > > > + field->func.update_field = field_inc_func; > > > + } > > > +} > > > + > > Thanks for comments, > 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.