| Message ID | 20230227200344.1308604-3-i.maximets@ovn.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | northd: Optimize parsing of LSP addresses. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | warning | apply and check: warning |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | fail | github build: failed |
Bleep bloop. Greetings Ilya Maximets, I am a robot and I have tried out your patch.
Thanks for your contribution.
I encountered some error that I wasn't expecting. See the details below.
checkpatch:
WARNING: Comment with 'xxx' marker
#52 FILE: lib/ovn-util.c:123:
/* "dynamic x.x.x.x xxxx::xxxx" */
WARNING: Comment with 'xxx' marker
#59 FILE: lib/ovn-util.c:130:
/* "dynamic xxxx::xxxx" */
Lines checked: 78, Warnings: 2, Errors: 0
Please check this out. If you feel there has been an error, please email aconole@redhat.com
Thanks,
0-day Robot
On Mon, Feb 27, 2023 at 09:03:43PM +0100, Ilya Maximets wrote: > For non-dynamic address, current version performs 1 strcmp + 4 attempts > for ovs_scan. Updated code perfroms 1 strncmp + 1 ovs_scan. > > Also, for ports with both IPv4 and IPv6 specified, new version doesn't > re-parse IPv4 twice. > > Signed-off-by: Ilya Maximets <i.maximets@ovn.org> Reviewed-by: Simon Horman <simon.horman@corigine.com>
Hi Ilya, I have a small suggestion below. On 2/27/23 15:03, Ilya Maximets wrote: > For non-dynamic address, current version performs 1 strcmp + 4 attempts > for ovs_scan. Updated code perfroms 1 strncmp + 1 ovs_scan. > > Also, for ports with both IPv4 and IPv6 specified, new version doesn't > re-parse IPv4 twice. > > Signed-off-by: Ilya Maximets <i.maximets@ovn.org> > --- > lib/ovn-util.c | 47 +++++++++++++++++++++++++++++++++++------------ > 1 file changed, 35 insertions(+), 12 deletions(-) > > diff --git a/lib/ovn-util.c b/lib/ovn-util.c > index 47484ef01..e683abb45 100644 > --- a/lib/ovn-util.c > +++ b/lib/ovn-util.c > @@ -105,18 +105,41 @@ is_dynamic_lsp_address(const char *address) > char ipv6_s[IPV6_SCAN_LEN + 1]; > struct eth_addr ea; > ovs_be32 ip; > - int n; > - return (!strcmp(address, "dynamic") > - || (ovs_scan(address, "dynamic "IP_SCAN_FMT"%n", > - IP_SCAN_ARGS(&ip), &n) > - && address[n] == '\0') > - || (ovs_scan(address, "dynamic "IP_SCAN_FMT" "IPV6_SCAN_FMT"%n", > - IP_SCAN_ARGS(&ip), ipv6_s, &n) > - && address[n] == '\0') > - || (ovs_scan(address, "dynamic "IPV6_SCAN_FMT"%n", > - ipv6_s, &n) && address[n] == '\0') > - || (ovs_scan(address, ETH_ADDR_SCAN_FMT" dynamic%n", > - ETH_ADDR_SCAN_ARGS(ea), &n) && address[n] == '\0')); > + int n = 0; > + > + if (!strncmp(address, "dynamic", 7)) { > + n = 7; > + if (!address[n]) { > + /* "dynamic" */ > + return true; > + } marker (see below) > + if (ovs_scan_len(address, &n, " "IP_SCAN_FMT, IP_SCAN_ARGS(&ip))) { > + if (!address[n]) { > + /* "dynamic x.x.x.x" */ > + return true; > + } > + if (ovs_scan_len(address, &n, " "IPV6_SCAN_FMT, ipv6_s) > + && !address[n]) { > + /* "dynamic x.x.x.x xxxx::xxxx" */ > + return true; > + } > + return false; > + } > + if (ovs_scan_len(address, &n, " "IPV6_SCAN_FMT, ipv6_s) > + && !address[n]) { > + /* "dynamic xxxx::xxxx" */ > + return true; > + } > + return false; > + } I think duplication can be reduced from the above marker to here: const char *addr = address; if (ovs_scan_len(addr, &n, " "IP_SCAN_FMT, IP_SCAN_ARGS(&ip))) { if (!addr[n]) { /* "dynamic x.x.x.x" */ return true; } addr += n; } if (ovs_scan_len(addr, &n, " "IPV6_SCAN_FMT, ipv6_s) && !address[n]) { /* Either "dynamic xxxx::xxxx" or "dynamic x.x.x.x xxxx::xxxx" return true; } return false; > + > + if (ovs_scan_len(address, &n, ETH_ADDR_SCAN_FMT" dynamic", > + ETH_ADDR_SCAN_ARGS(ea)) && !address[n]) { > + /* "xx:xx:xx:xx:xx:xx dynamic" */ > + return true; > + } > + > + return false; > } > > static bool
On 3/1/23 20:17, Mark Michelson wrote: > Hi Ilya, I have a small suggestion below. > > On 2/27/23 15:03, Ilya Maximets wrote: >> For non-dynamic address, current version performs 1 strcmp + 4 attempts >> for ovs_scan. Updated code perfroms 1 strncmp + 1 ovs_scan. >> >> Also, for ports with both IPv4 and IPv6 specified, new version doesn't >> re-parse IPv4 twice. >> >> Signed-off-by: Ilya Maximets <i.maximets@ovn.org> >> --- >> lib/ovn-util.c | 47 +++++++++++++++++++++++++++++++++++------------ >> 1 file changed, 35 insertions(+), 12 deletions(-) >> >> diff --git a/lib/ovn-util.c b/lib/ovn-util.c >> index 47484ef01..e683abb45 100644 >> --- a/lib/ovn-util.c >> +++ b/lib/ovn-util.c >> @@ -105,18 +105,41 @@ is_dynamic_lsp_address(const char *address) >> char ipv6_s[IPV6_SCAN_LEN + 1]; >> struct eth_addr ea; >> ovs_be32 ip; >> - int n; >> - return (!strcmp(address, "dynamic") >> - || (ovs_scan(address, "dynamic "IP_SCAN_FMT"%n", >> - IP_SCAN_ARGS(&ip), &n) >> - && address[n] == '\0') >> - || (ovs_scan(address, "dynamic "IP_SCAN_FMT" "IPV6_SCAN_FMT"%n", >> - IP_SCAN_ARGS(&ip), ipv6_s, &n) >> - && address[n] == '\0') >> - || (ovs_scan(address, "dynamic "IPV6_SCAN_FMT"%n", >> - ipv6_s, &n) && address[n] == '\0') >> - || (ovs_scan(address, ETH_ADDR_SCAN_FMT" dynamic%n", >> - ETH_ADDR_SCAN_ARGS(ea), &n) && address[n] == '\0')); >> + int n = 0; >> + >> + if (!strncmp(address, "dynamic", 7)) { >> + n = 7; >> + if (!address[n]) { >> + /* "dynamic" */ >> + return true; >> + } > > marker (see below) > >> + if (ovs_scan_len(address, &n, " "IP_SCAN_FMT, IP_SCAN_ARGS(&ip))) { >> + if (!address[n]) { >> + /* "dynamic x.x.x.x" */ >> + return true; >> + } >> + if (ovs_scan_len(address, &n, " "IPV6_SCAN_FMT, ipv6_s) >> + && !address[n]) { >> + /* "dynamic x.x.x.x xxxx::xxxx" */ >> + return true; >> + } >> + return false; I guess, 6 lines above can be just removed without any logic change, right? I mean, we'll fall-through to the exactly same 6 lines below, except for the comment. >> + } >> + if (ovs_scan_len(address, &n, " "IPV6_SCAN_FMT, ipv6_s) >> + && !address[n]) { >> + /* "dynamic xxxx::xxxx" */ >> + return true; >> + } >> + return false; >> + } > > > I think duplication can be reduced from the above marker to here: see above. > > const char *addr = address; > if (ovs_scan_len(addr, &n, " "IP_SCAN_FMT, IP_SCAN_ARGS(&ip))) { > if (!addr[n]) { > /* "dynamic x.x.x.x" */ > return true; > } > addr += n; We would also need to reset 'n' here. > } > if (ovs_scan_len(addr, &n, " "IPV6_SCAN_FMT, ipv6_s) && !address[n]) { > /* Either "dynamic xxxx::xxxx" or "dynamic x.x.x.x xxxx::xxxx" > return true; > } > return false; > >> + >> + if (ovs_scan_len(address, &n, ETH_ADDR_SCAN_FMT" dynamic", >> + ETH_ADDR_SCAN_ARGS(ea)) && !address[n]) { >> + /* "xx:xx:xx:xx:xx:xx dynamic" */ >> + return true; >> + } > + >> + return false; >> } >> static bool >
diff --git a/lib/ovn-util.c b/lib/ovn-util.c index 47484ef01..e683abb45 100644 --- a/lib/ovn-util.c +++ b/lib/ovn-util.c @@ -105,18 +105,41 @@ is_dynamic_lsp_address(const char *address) char ipv6_s[IPV6_SCAN_LEN + 1]; struct eth_addr ea; ovs_be32 ip; - int n; - return (!strcmp(address, "dynamic") - || (ovs_scan(address, "dynamic "IP_SCAN_FMT"%n", - IP_SCAN_ARGS(&ip), &n) - && address[n] == '\0') - || (ovs_scan(address, "dynamic "IP_SCAN_FMT" "IPV6_SCAN_FMT"%n", - IP_SCAN_ARGS(&ip), ipv6_s, &n) - && address[n] == '\0') - || (ovs_scan(address, "dynamic "IPV6_SCAN_FMT"%n", - ipv6_s, &n) && address[n] == '\0') - || (ovs_scan(address, ETH_ADDR_SCAN_FMT" dynamic%n", - ETH_ADDR_SCAN_ARGS(ea), &n) && address[n] == '\0')); + int n = 0; + + if (!strncmp(address, "dynamic", 7)) { + n = 7; + if (!address[n]) { + /* "dynamic" */ + return true; + } + if (ovs_scan_len(address, &n, " "IP_SCAN_FMT, IP_SCAN_ARGS(&ip))) { + if (!address[n]) { + /* "dynamic x.x.x.x" */ + return true; + } + if (ovs_scan_len(address, &n, " "IPV6_SCAN_FMT, ipv6_s) + && !address[n]) { + /* "dynamic x.x.x.x xxxx::xxxx" */ + return true; + } + return false; + } + if (ovs_scan_len(address, &n, " "IPV6_SCAN_FMT, ipv6_s) + && !address[n]) { + /* "dynamic xxxx::xxxx" */ + return true; + } + return false; + } + + if (ovs_scan_len(address, &n, ETH_ADDR_SCAN_FMT" dynamic", + ETH_ADDR_SCAN_ARGS(ea)) && !address[n]) { + /* "xx:xx:xx:xx:xx:xx dynamic" */ + return true; + } + + return false; } static bool
For non-dynamic address, current version performs 1 strcmp + 4 attempts for ovs_scan. Updated code perfroms 1 strncmp + 1 ovs_scan. Also, for ports with both IPv4 and IPv6 specified, new version doesn't re-parse IPv4 twice. Signed-off-by: Ilya Maximets <i.maximets@ovn.org> --- lib/ovn-util.c | 47 +++++++++++++++++++++++++++++++++++------------ 1 file changed, 35 insertions(+), 12 deletions(-)