On Wed, Aug 03, 2016 at 04:27:14PM +0200, Tobias Klauser wrote:
> On 2016-07-26 at 21:35:10 +0200, Vadim Kochan <[email protected]> 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 <[email protected]>
> > ---
> > 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()?
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.
>
> > + }
> > +}
> > +
> > +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.
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;
>
> 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 [email protected].
For more options, visit https://groups.google.com/d/optout.
