On Thu, Aug 11, 2016 at 1:30 PM, Tobias Klauser <tklau...@distanz.ch> wrote:
> 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)?
The reason is just because in field_inc function I use the following:
next = (val + inc) % (max + 1);
and in case when max == UINT32_MAX I will get division by 0.
If you dont like UINT32_MAX - 1 then I will try to handle max + 1
overflow somehow differently ...
>
>> +
>> + 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.
