| Message ID | 20230518134601.17873-1-vladimir@nikishkin.pw (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | David Ahern |
| Headers | show |
| Series | [iproute2-next,v4] ip-link: add support for nolocalbypass in vxlan | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Thu, 18 May 2023 21:46:01 +0800 Vladimir Nikishkin <vladimir@nikishkin.pw> wrote: > + if (tb[IFLA_VXLAN_LOCALBYPASS]) { > + __u8 localbypass = rta_getattr_u8(tb[IFLA_VXLAN_LOCALBYPASS]); > + > + print_bool(PRINT_JSON, "localbypass", NULL, localbypass); > + if (localbypass) { > + print_string(PRINT_FP, NULL, "localbypass ", NULL); > + } else { > + print_string(PRINT_FP, NULL, "nolocalbypass ", NULL); > + } > + } You don't have to print anything if nolocalbypass. Use presence as a boolean in JSON. I.e. if (tb[IFLA_VXLAN_LOCALBYPASS] && rta_getattr_u8(tb[IFLA_VXLAN_LOCALBYPASS])) { print_bool(PRINT_ANY, "localbypass", "localbypass", true); } That is what other options do. Follows the best practices for changes to existing programs: your new feature should look like all the others.
Stephen Hemminger <stephen@networkplumber.org> writes: > On Thu, 18 May 2023 21:46:01 +0800 > Vladimir Nikishkin <vladimir@nikishkin.pw> wrote: > >> + if (tb[IFLA_VXLAN_LOCALBYPASS]) { >> + __u8 localbypass = rta_getattr_u8(tb[IFLA_VXLAN_LOCALBYPASS]); >> + >> + print_bool(PRINT_JSON, "localbypass", NULL, localbypass); >> + if (localbypass) { >> + print_string(PRINT_FP, NULL, "localbypass ", NULL); >> + } else { >> + print_string(PRINT_FP, NULL, "nolocalbypass ", NULL); >> + } >> + } > > You don't have to print anything if nolocalbypass. Use presence as > a boolean in JSON. > > I.e. > if (tb[IFLA_VXLAN_LOCALBYPASS] && > rta_getattr_u8(tb[IFLA_VXLAN_LOCALBYPASS])) { > print_bool(PRINT_ANY, "localbypass", "localbypass", true); > } > > That is what other options do. > Follows the best practices for changes to existing programs: your > new feature should look like all the others. Sorry, I do not understand. I intended to do exactly that, and I copied and adjusted for the option name the code currently used for the "udpcsum" option. Which is exactly if (is_json_context()) { print_bool(PRINT_ANY, "udp_csum", NULL, udp_csum); } else { if (!udp_csum) fputs("no", f); fputs("udpcsum ", f); } I just replaced that option name with [no]localbypass. Fairly straightforward, prints noudpcsum or udpcsum. Later Andrea C Then Andrea Claudi suggested that print_bool knows about the json context itself, so the outer check is not needed, so I removed that. But the "model option" I used (really the simplest one), does have output both when set to true, and when set to false. I have neither an opinion on this nor an understanding what is better for scripting. But I do not understand the suggestion "do like the other options do", when seemingly, other options do what I suggest in the first place.
On Fri, May 19, 2023 at 11:50:03AM +0800, Vladimir Nikishkin wrote: > > Stephen Hemminger <stephen@networkplumber.org> writes: > > > On Thu, 18 May 2023 21:46:01 +0800 > > Vladimir Nikishkin <vladimir@nikishkin.pw> wrote: > > > >> + if (tb[IFLA_VXLAN_LOCALBYPASS]) { > >> + __u8 localbypass = rta_getattr_u8(tb[IFLA_VXLAN_LOCALBYPASS]); > >> + > >> + print_bool(PRINT_JSON, "localbypass", NULL, localbypass); > >> + if (localbypass) { > >> + print_string(PRINT_FP, NULL, "localbypass ", NULL); > >> + } else { > >> + print_string(PRINT_FP, NULL, "nolocalbypass ", NULL); > >> + } > >> + } > > > > You don't have to print anything if nolocalbypass. Use presence as > > a boolean in JSON. > > > > I.e. > > if (tb[IFLA_VXLAN_LOCALBYPASS] && > > rta_getattr_u8(tb[IFLA_VXLAN_LOCALBYPASS])) { > > print_bool(PRINT_ANY, "localbypass", "localbypass", true); > > } > > > > That is what other options do. > > Follows the best practices for changes to existing programs: your > > new feature should look like all the others. > > Sorry, I do not understand. I intended to do exactly that, and I copied > and adjusted for the option name the code currently used for the > "udpcsum" option. Which is exactly > > if (is_json_context()) { > print_bool(PRINT_ANY, "udp_csum", NULL, udp_csum); > } else { > if (!udp_csum) > fputs("no", f); > fputs("udpcsum ", f); > } > I just replaced that option name with [no]localbypass. Fairly > straightforward, prints noudpcsum or udpcsum. Later Andrea C > > Then Andrea Claudi suggested that print_bool knows about the json > context itself, so the outer check is not needed, so I removed that. > But the "model option" I used (really the simplest one), does have > output both when set to true, and when set to false. I have neither an > opinion on this nor an understanding what is better for scripting. But I > do not understand the suggestion "do like the other options do", when > seemingly, other options do what I suggest in the first place. > If I get Stephen corretly, he is simply suggesting that printing "nolocalbypass" is unnecessary. If you find don't find "localbypass" in the output, you know it's not enabled. Unfortunately iplink_vxlan does not work according to this logic, as you are pointing out, but there are several places where this happens, like link_gre6.c:543. So you can do that for "localbypass" here. Fixing old options, on the other hand, is not easy, as we may end up breaking user scripts relying on "no<whatever>" option. I can work on a patch for that, but we probably need some kind of deprecation warning to users. Stephen, what do you think?
On Fri, 19 May 2023 11:11:12 +0200 Andrea Claudi <aclaudi@redhat.com> wrote: > On Fri, May 19, 2023 at 11:50:03AM +0800, Vladimir Nikishkin wrote: > > > > Stephen Hemminger <stephen@networkplumber.org> writes: > > > > > On Thu, 18 May 2023 21:46:01 +0800 > > > Vladimir Nikishkin <vladimir@nikishkin.pw> wrote: > > > > > >> + if (tb[IFLA_VXLAN_LOCALBYPASS]) { > > >> + __u8 localbypass = rta_getattr_u8(tb[IFLA_VXLAN_LOCALBYPASS]); > > >> + > > >> + print_bool(PRINT_JSON, "localbypass", NULL, localbypass); > > >> + if (localbypass) { > > >> + print_string(PRINT_FP, NULL, "localbypass ", NULL); > > >> + } else { > > >> + print_string(PRINT_FP, NULL, "nolocalbypass ", NULL); > > >> + } > > >> + } > > > > > > You don't have to print anything if nolocalbypass. Use presence as > > > a boolean in JSON. > > > > > > I.e. > > > if (tb[IFLA_VXLAN_LOCALBYPASS] && > > > rta_getattr_u8(tb[IFLA_VXLAN_LOCALBYPASS])) { > > > print_bool(PRINT_ANY, "localbypass", "localbypass", true); > > > } > > > > > > That is what other options do. > > > Follows the best practices for changes to existing programs: your > > > new feature should look like all the others. > > > > Sorry, I do not understand. I intended to do exactly that, and I copied > > and adjusted for the option name the code currently used for the > > "udpcsum" option. Which is exactly > > > > if (is_json_context()) { > > print_bool(PRINT_ANY, "udp_csum", NULL, udp_csum); > > } else { > > if (!udp_csum) > > fputs("no", f); > > fputs("udpcsum ", f); > > } > > I just replaced that option name with [no]localbypass. Fairly > > straightforward, prints noudpcsum or udpcsum. Later Andrea C > > > > Then Andrea Claudi suggested that print_bool knows about the json > > context itself, so the outer check is not needed, so I removed that. > > But the "model option" I used (really the simplest one), does have > > output both when set to true, and when set to false. I have neither an > > opinion on this nor an understanding what is better for scripting. But I > > do not understand the suggestion "do like the other options do", when > > seemingly, other options do what I suggest in the first place. > > > > If I get Stephen corretly, he is simply suggesting that printing > "nolocalbypass" is unnecessary. If you find don't find "localbypass" in > the output, you know it's not enabled. > > Unfortunately iplink_vxlan does not work according to this logic, as you > are pointing out, but there are several places where this happens, like > link_gre6.c:543. So you can do that for "localbypass" here. > > Fixing old options, on the other hand, is not easy, as we may end up > breaking user scripts relying on "no<whatever>" option. I can work on a > patch for that, but we probably need some kind of deprecation warning to > users. > > Stephen, what do you think? > Scripts parsing non-json output are fragile. There was never a hard guarantee that non-json output was stable. I was looking at the existing vxlan_print_opts() and it already does this for several options. static void vxlan_print_opt(struct link_util *lu, FILE *f, struct rtattr *tb[]) { ... if (tb[IFLA_VXLAN_COLLECT_METADATA] && rta_getattr_u8(tb[IFLA_VXLAN_COLLECT_METADATA])) { print_bool(PRINT_ANY, "external", "external ", true); } if (tb[IFLA_VXLAN_VNIFILTER] && rta_getattr_u8(tb[IFLA_VXLAN_VNIFILTER])) { print_bool(PRINT_ANY, "vnifilter", "vnifilter", true); }
diff --git a/ip/iplink_vxlan.c b/ip/iplink_vxlan.c index c7e0e1c4..966b0daf 100644 --- a/ip/iplink_vxlan.c +++ b/ip/iplink_vxlan.c @@ -45,6 +45,7 @@ static void print_explain(FILE *f) " [ [no]remcsumtx ] [ [no]remcsumrx ]\n" " [ [no]external ] [ gbp ] [ gpe ]\n" " [ [no]vnifilter ]\n" + " [ [no]localbypass ]\n" "\n" "Where: VNI := 0-16777215\n" " ADDR := { IP_ADDRESS | any }\n" @@ -276,6 +277,14 @@ static int vxlan_parse_opt(struct link_util *lu, int argc, char **argv, } else if (!matches(*argv, "noudpcsum")) { check_duparg(&attrs, IFLA_VXLAN_UDP_CSUM, *argv, *argv); addattr8(n, 1024, IFLA_VXLAN_UDP_CSUM, 0); + } else if (strcmp(*argv, "localbypass") == 0) { + check_duparg(&attrs, IFLA_VXLAN_LOCALBYPASS, + *argv, *argv); + addattr8(n, 1024, IFLA_VXLAN_LOCALBYPASS, 1); + } else if (strcmp(*argv, "nolocalbypass") == 0) { + check_duparg(&attrs, IFLA_VXLAN_LOCALBYPASS, + *argv, *argv); + addattr8(n, 1024, IFLA_VXLAN_LOCALBYPASS, 0); } else if (!matches(*argv, "udp6zerocsumtx")) { check_duparg(&attrs, IFLA_VXLAN_UDP_ZERO_CSUM6_TX, *argv, *argv); @@ -613,6 +622,17 @@ static void vxlan_print_opt(struct link_util *lu, FILE *f, struct rtattr *tb[]) } } + if (tb[IFLA_VXLAN_LOCALBYPASS]) { + __u8 localbypass = rta_getattr_u8(tb[IFLA_VXLAN_LOCALBYPASS]); + + print_bool(PRINT_JSON, "localbypass", NULL, localbypass); + if (localbypass) { + print_string(PRINT_FP, NULL, "localbypass ", NULL); + } else { + print_string(PRINT_FP, NULL, "nolocalbypass ", NULL); + } + } + if (tb[IFLA_VXLAN_UDP_ZERO_CSUM6_TX]) { __u8 csum6 = rta_getattr_u8(tb[IFLA_VXLAN_UDP_ZERO_CSUM6_TX]); diff --git a/man/man8/ip-link.8.in b/man/man8/ip-link.8.in index bf3605a9..27ebeeac 100644 --- a/man/man8/ip-link.8.in +++ b/man/man8/ip-link.8.in @@ -630,6 +630,8 @@ the following additional arguments are supported: ] [ .RB [ no ] udpcsum ] [ +.RB [ no ] localbypass +] [ .RB [ no ] udp6zerocsumtx ] [ .RB [ no ] udp6zerocsumrx @@ -734,6 +736,14 @@ are entered into the VXLAN device forwarding database. .RB [ no ] udpcsum - specifies if UDP checksum is calculated for transmitted packets over IPv4. +.sp +.RB [ no ] localbypass +- if FDB destination is local, with nolocalbypass set, forward encapsulated +packets to the userspace network stack. If there is a userspace process +listening for these packets, it will have a chance to process them. If +localbypass is active (default), bypass the kernel network stack and +inject the packets into the target VXLAN device, assuming one exists. + .sp .RB [ no ] udp6zerocsumtx - skip UDP checksum calculation for transmitted packets over IPv6.
Add userspace support for the [no]localbypass vxlan netlink attribute. With localbypass on (default), the vxlan driver processes the packets destined to the local machine by itself, bypassing the userspace nework stack. With nolocalbypass the packets are always forwarded to the userspace network stack, so userspace programs, such as tcpdump have a chance to process them. Signed-off-by: Vladimir Nikishkin <vladimir@nikishkin.pw> --- v2=>v3: 1. replace fputs with print_string 2. fix 77 char line length 3. fix typos and improve man page 4. reformat strcmp usage this patch matches commit 69474a8a5837be63f13c6f60a7d622b98ed5c539 in the main tree. v3=>v4: 1. fix typos in man/man8/ip-link.8.in ip/iplink_vxlan.c | 20 ++++++++++++++++++++ man/man8/ip-link.8.in | 10 ++++++++++ 2 files changed, 30 insertions(+)