trafgen: l3: Support interface without IP address #165
Conversation
|
Right, I faced with this few days ago, but ... looks like a forgot about this. Thanks for point to this issue. |
| @@ -81,6 +80,7 @@ static void ipv4_packet_finish(struct proto_hdr *hdr) | |||
|
|
|||
| total_len = pkt->len - hdr->pkt_offset; | |||
| proto_field_set_default_be16(hdr, IP4_LEN, total_len); | |||
| proto_field_set_default_dev_ipv4(hdr, IP4_SADDR); | |||
There was a problem hiding this comment.
I am thinking that may be it is better to handle failing of getting Ipv4/6 address from device and only warning about this
w/o panic.
There was a problem hiding this comment.
Sounds good to me. But then we need a reasonable default value for that case. 0.0.0.0 might be a good choice.
There was a problem hiding this comment.
W/o digging into the code as far I remember by default the field value will be zeroed.
There was a problem hiding this comment.
Good, I will check that tomorrow morning and make necessary changes.
| @@ -161,6 +160,7 @@ static void ipv6_packet_finish(struct proto_hdr *hdr) | |||
| uint16_t total_len = pkt->len - hdr->pkt_offset - IPV6_HDR_LEN; | |||
|
|
|||
| proto_field_set_default_be16(hdr, IP6_LEN, total_len); | |||
| proto_field_set_default_dev_ipv6(hdr, IP6_SADDR); | |||
There was a problem hiding this comment.
Looks like you forgot to use TAB here.
|
Apart from the issues that Vadim addressed I think this is fine. When you push your updated branch, could you please also include your Signed-off-by line in the commit as indicated in the SubmittingPatches document? Thanks a lot for your contribution! |
abaw
force-pushed
the
master
branch
3 times, most recently
from
3c861b7
to
c7445ab
Compare
|
I made the suggested changes, please take a look. Thanks. |
|
LGTM. @vkochan, good to merge for you too? |
| if (ret < 0) { | ||
| fprintf(stderr,"Warning: Could not get device IPv4 address for %s\n", ctx.dev); | ||
| return; | ||
| } |
There was a problem hiding this comment.
You need to shift '}' with a TAB to be aligned under 'if' on the same column.
There was a problem hiding this comment.
Good eye. Will fix when applying. Also the missing space after the comma...
| if (ret < 0) { | ||
| fprintf(stderr, "Warning: Could not get device IPv6 address for %s\n", ctx.dev); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Please see above's comment.
Move default source address setting to packet_finish so that we do not need to get the device's address if the source address is set in the packet. Without this, trafgen does not work with an interface without address configured. In addition, in the case failing to get the address for an interface, instead of panic, it now prints a warning and use a value of 0.0.0.0. Signed-off-by: Ken Wu <[email protected]>
|
Thanks, I just fixed the tab and white space after the comma. And finally I learned to use checkpatch.pl from kernel source to check the coding style. Apart from these mentioned style problems, there are also lines exceeding 80 characters. Should I fix those long lines too? |
Add Ken Wu for commit c4e07d5 ("trafgen: l3: Support interface without IP address"), submitted via PR #165 Signed-off-by: Tobias Klauser <[email protected]>
Hi,
I found the project this weekend and trafgen is exactly what I need at this moment. But playing with it, I found that it does not work with an interface without any IP address configured. For testing network equipments, I always remove the IP addresses of an interface to prevent Linux from sending unwanted packets(ARP, IGMP and so on) because those unwanted packets might affect my testing.
So I just fixed this by myself. Please take a look if you have time. Thanks for making this great tool.