| Message ID | 1467176548-40488-6-git-send-email-u9012063@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
On Tue, Jun 28, 2016 at 10:02 PM, William Tu <u9012063@gmail.com> wrote: > Variable 'error' has been free in line 1795. > > Signed-off-by: William Tu <u9012063@gmail.com> > --- > ovn/utilities/ovn-nbctl.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/ovn/utilities/ovn-nbctl.c b/ovn/utilities/ovn-nbctl.c > index 345647a..3228a03 100644 > --- a/ovn/utilities/ovn-nbctl.c > +++ b/ovn/utilities/ovn-nbctl.c > @@ -1805,7 +1805,6 @@ nbctl_lr_route_list(struct ctl_context *ctx) > VLOG_WARN("router "UUID_FMT" (%s) has invalid prefix: %s", > UUID_ARGS(&lr->header_.uuid), lr->name, > route->ip_prefix); > - free(error); > continue; > } > } > Possibly, the intention was to check for parse error from ipv6 as well as ipv4. The first free is for the ipv4 case. The second free is for the ipv6 case. error = ip_parse_cidr(route->ip_prefix, &ipv4, &plen); if (!error) { ipv4_routes[n_ipv4_routes].plen = plen; ipv4_routes[n_ipv4_routes].addr = ipv4; ipv4_routes[n_ipv4_routes].route = route; n_ipv4_routes++; } else { free(error); struct in6_addr ipv6; - if (!ipv6_parse_cidr(route->ip_prefix, &ipv6, &plen)) { + error = ipv6_parse_cidr(route->ip_prefix, &ipv6, &plen); + if (!error) { ipv6_routes[n_ipv6_routes].plen = plen; ipv6_routes[n_ipv6_routes].addr = ipv6; ipv6_routes[n_ipv6_routes].route = route; n_ipv6_routes++; } else { /* Invalid prefix. */ VLOG_WARN("router "UUID_FMT" (%s) has invalid prefix: %s", UUID_ARGS(&lr->header_.uuid), lr->name, route->ip_prefix); free(error); continue; } > -- > 2.5.0 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev >
Hi Darrell, Thanks, I think we should print the error in VLOG_WARN and free it . Regards, William On Wed, Jun 29, 2016 at 10:08 AM, Darrell Ball <dlu998@gmail.com> wrote: > > > On Tue, Jun 28, 2016 at 10:02 PM, William Tu <u9012063@gmail.com> wrote: >> >> Variable 'error' has been free in line 1795. >> >> Signed-off-by: William Tu <u9012063@gmail.com> >> --- >> ovn/utilities/ovn-nbctl.c | 1 - >> 1 file changed, 1 deletion(-) >> >> diff --git a/ovn/utilities/ovn-nbctl.c b/ovn/utilities/ovn-nbctl.c >> index 345647a..3228a03 100644 >> --- a/ovn/utilities/ovn-nbctl.c >> +++ b/ovn/utilities/ovn-nbctl.c >> @@ -1805,7 +1805,6 @@ nbctl_lr_route_list(struct ctl_context *ctx) >> VLOG_WARN("router "UUID_FMT" (%s) has invalid prefix: >> %s", >> UUID_ARGS(&lr->header_.uuid), lr->name, >> route->ip_prefix); >> - free(error); >> continue; >> } >> } > > > > Possibly, the intention was to check for parse error from ipv6 as well as > ipv4. > The first free is for the ipv4 case. > The second free is for the ipv6 case. > > error = ip_parse_cidr(route->ip_prefix, &ipv4, &plen); > if (!error) { > ipv4_routes[n_ipv4_routes].plen = plen; > ipv4_routes[n_ipv4_routes].addr = ipv4; > ipv4_routes[n_ipv4_routes].route = route; > n_ipv4_routes++; > } else { > free(error); > > struct in6_addr ipv6; > - if (!ipv6_parse_cidr(route->ip_prefix, &ipv6, &plen)) { > + error = ipv6_parse_cidr(route->ip_prefix, &ipv6, &plen); > + if (!error) { > ipv6_routes[n_ipv6_routes].plen = plen; > ipv6_routes[n_ipv6_routes].addr = ipv6; > ipv6_routes[n_ipv6_routes].route = route; > n_ipv6_routes++; > } else { > /* Invalid prefix. */ > VLOG_WARN("router "UUID_FMT" (%s) has invalid prefix: %s", > UUID_ARGS(&lr->header_.uuid), lr->name, > route->ip_prefix); > free(error); > continue; > } > > > >> >> -- >> 2.5.0 >> >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> http://openvswitch.org/mailman/listinfo/dev > >
On Thu, Jun 30, 2016 at 2:31 PM, William Tu <u9012063@gmail.com> wrote: > Hi Darrell, > > Thanks, I think we should print the error in VLOG_WARN and free it . > > Regards, > William > Thanks I only commented on grabbing the error return value and allowing it to be freed instead of removing the free() call to satisfy the clang warning. If you think also adding the error returned is helpful to add to the existing below warn log, then probably best to check with the author of the code, but I agree with you. > VLOG_WARN("router "UUID_FMT" (%s) has invalid prefix: %s", > UUID_ARGS(&lr->header_.uuid), lr->name, > route->ip_prefix); > > On Wed, Jun 29, 2016 at 10:08 AM, Darrell Ball <dlu998@gmail.com> wrote: > > > > > > On Tue, Jun 28, 2016 at 10:02 PM, William Tu <u9012063@gmail.com> wrote: > >> > >> Variable 'error' has been free in line 1795. > >> > >> Signed-off-by: William Tu <u9012063@gmail.com> > >> --- > >> ovn/utilities/ovn-nbctl.c | 1 - > >> 1 file changed, 1 deletion(-) > >> > >> diff --git a/ovn/utilities/ovn-nbctl.c b/ovn/utilities/ovn-nbctl.c > >> index 345647a..3228a03 100644 > >> --- a/ovn/utilities/ovn-nbctl.c > >> +++ b/ovn/utilities/ovn-nbctl.c > >> @@ -1805,7 +1805,6 @@ nbctl_lr_route_list(struct ctl_context *ctx) > >> VLOG_WARN("router "UUID_FMT" (%s) has invalid prefix: > >> %s", > >> UUID_ARGS(&lr->header_.uuid), lr->name, > >> route->ip_prefix); > >> - free(error); > >> continue; > >> } > >> } > > > > > > > > Possibly, the intention was to check for parse error from ipv6 as well as > > ipv4. > > The first free is for the ipv4 case. > > The second free is for the ipv6 case. > > > > error = ip_parse_cidr(route->ip_prefix, &ipv4, &plen); > > if (!error) { > > ipv4_routes[n_ipv4_routes].plen = plen; > > ipv4_routes[n_ipv4_routes].addr = ipv4; > > ipv4_routes[n_ipv4_routes].route = route; > > n_ipv4_routes++; > > } else { > > free(error); > > > > struct in6_addr ipv6; > > - if (!ipv6_parse_cidr(route->ip_prefix, &ipv6, &plen)) { > > + error = ipv6_parse_cidr(route->ip_prefix, &ipv6, &plen); > > + if (!error) { > > ipv6_routes[n_ipv6_routes].plen = plen; > > ipv6_routes[n_ipv6_routes].addr = ipv6; > > ipv6_routes[n_ipv6_routes].route = route; > > n_ipv6_routes++; > > } else { > > /* Invalid prefix. */ > > VLOG_WARN("router "UUID_FMT" (%s) has invalid prefix: > %s", > > UUID_ARGS(&lr->header_.uuid), lr->name, > > route->ip_prefix); > > free(error); > > continue; > > } > > > > > > > >> > >> -- > >> 2.5.0 > >> > >> _______________________________________________ > >> dev mailing list > >> dev@openvswitch.org > >> http://openvswitch.org/mailman/listinfo/dev > > > > >
diff --git a/ovn/utilities/ovn-nbctl.c b/ovn/utilities/ovn-nbctl.c index 345647a..3228a03 100644 --- a/ovn/utilities/ovn-nbctl.c +++ b/ovn/utilities/ovn-nbctl.c @@ -1805,7 +1805,6 @@ nbctl_lr_route_list(struct ctl_context *ctx) VLOG_WARN("router "UUID_FMT" (%s) has invalid prefix: %s", UUID_ARGS(&lr->header_.uuid), lr->name, route->ip_prefix); - free(error); continue; } }
Variable 'error' has been free in line 1795. Signed-off-by: William Tu <u9012063@gmail.com> --- ovn/utilities/ovn-nbctl.c | 1 - 1 file changed, 1 deletion(-)