| Message ID | 20221208011122.2343363-7-jesse.brandeburg@intel.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Michal Kubecek |
| Headers | show |
| Series | ethtool: clean up and fix | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Wed, Dec 07, 2022 at 05:11:15PM -0800, Jesse Brandeburg wrote: > '$ scan-build make' reports: > > netlink/parser.c:252:25: warning: The left operand of '*' is a garbage value [core.UndefinedBinaryOperatorResult] > cm = (uint32_t)(meters * 100 + 0.5); > ~~~~~~ ^ > > This is a little more complicated than it seems, but for some > unexplained reason, parse_float always returns integers but was declared > to return a float. Looks like i got that wrong. Duh! > This is confusing at best. In the case of the error > above, parse_float could conceivably return without initializing it's > output variable, and because the function return variable was declared > as float but downgraded to an int via assignment (-Wconversion anyone?) > upon the return, the return value could be ignored. > > To fix the bug above, declare an initial value for meters, and make sure > that parse_float() always returns an integer value. > > It would probably be even more ideal if parse_float always initialized > it's output variables before even checking for input errors, but that's > for some future day. It is also following the pattern of other parse_foo functions. None of them set the result in case of errors. > > CC: Andrew Lunn <andrew@lunn.ch> > Fixes: 9561db9b76f4 ("Add cable test TDR support") > Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com> > --- > netlink/parser.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/netlink/parser.c b/netlink/parser.c > index f982f229a040..70f451008eb4 100644 > --- a/netlink/parser.c > +++ b/netlink/parser.c > @@ -54,8 +54,7 @@ static bool __prefix_0x(const char *p) > return p[0] == '0' && (p[1] == 'x' || p[1] == 'X'); > } > > -static float parse_float(const char *arg, float *result, float min, > - float max) > +static int parse_float(const char *arg, float *result, float min, float max) > { > char *endptr; > float val; > @@ -237,7 +236,7 @@ int nl_parse_direct_m2cm(struct nl_context *nlctx, uint16_t type, > struct nl_msg_buff *msgbuff, void *dest) > { > const char *arg = *nlctx->argp; > - float meters; > + float meters = 0; > uint32_t cm; > int ret; Should this actually be 0.0? Otherwise if -Wconversion gets turned on, you have an int being converted to a float? Anyway: Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew
diff --git a/netlink/parser.c b/netlink/parser.c index f982f229a040..70f451008eb4 100644 --- a/netlink/parser.c +++ b/netlink/parser.c @@ -54,8 +54,7 @@ static bool __prefix_0x(const char *p) return p[0] == '0' && (p[1] == 'x' || p[1] == 'X'); } -static float parse_float(const char *arg, float *result, float min, - float max) +static int parse_float(const char *arg, float *result, float min, float max) { char *endptr; float val; @@ -237,7 +236,7 @@ int nl_parse_direct_m2cm(struct nl_context *nlctx, uint16_t type, struct nl_msg_buff *msgbuff, void *dest) { const char *arg = *nlctx->argp; - float meters; + float meters = 0; uint32_t cm; int ret;
'$ scan-build make' reports: netlink/parser.c:252:25: warning: The left operand of '*' is a garbage value [core.UndefinedBinaryOperatorResult] cm = (uint32_t)(meters * 100 + 0.5); ~~~~~~ ^ This is a little more complicated than it seems, but for some unexplained reason, parse_float always returns integers but was declared to return a float. This is confusing at best. In the case of the error above, parse_float could conceivably return without initializing it's output variable, and because the function return variable was declared as float but downgraded to an int via assignment (-Wconversion anyone?) upon the return, the return value could be ignored. To fix the bug above, declare an initial value for meters, and make sure that parse_float() always returns an integer value. It would probably be even more ideal if parse_float always initialized it's output variables before even checking for input errors, but that's for some future day. CC: Andrew Lunn <andrew@lunn.ch> Fixes: 9561db9b76f4 ("Add cable test TDR support") Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com> --- netlink/parser.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)