| Message ID | 20221227173620.6577-1-glipus@gmail.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Delegated to: | Michal Kubecek |
| Headers | show |
| Series | [ethtool-next] Fixing boolean value output for Netlink reported values in JSON format | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Tue, Dec 27, 2022 at 10:36:20AM -0700, Maxim Georgiev wrote: > Current implementation of show_bool_val() passes "val" parameter of pointer > type as a last parameter to print_bool(): > https://git.kernel.org/pub/scm/network/ethtool/ethtool.git/tree/netlink/netlink.h#n131 > ... > static inline void show_bool_val(const char *key, const char *fmt, uint8_t *val) > { > if (is_json_context()) { > if (val) > > print_bool(PRINT_JSON, key, NULL, val); > } else { > ... > print_bool() expects the last parameter to be bool: > https://git.kernel.org/pub/scm/network/ethtool/ethtool.git/tree/json_print.c#n153 > ... > void print_bool(enum output_type type, > const char *key, > const char *fmt, > bool value) > { > ... > Current show_bool_val() implementation converts "val" pointer to bool while > calling show_bool_val(). As a result show_bool_val() always prints the value > as "true" as long as it gets a non-null pointer to the boolean value, even if > the referred boolean value is false. > > Fixes: 7e5c1ddbe67d ("pause: add --json support") > Signed-off-by: Maxim Georgiev <glipus@gmail.com> Reviewed-by: Simon Horman <simon.horman@corigine.com> I'm assuming that val is never NULL :) > --- > netlink/netlink.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/netlink/netlink.h b/netlink/netlink.h > index 3240fca..1274a3b 100644 > --- a/netlink/netlink.h > +++ b/netlink/netlink.h > @@ -128,7 +128,7 @@ static inline void show_bool_val(const char *key, const char *fmt, uint8_t *val) > { > if (is_json_context()) { > if (val) > - print_bool(PRINT_JSON, key, NULL, val); > + print_bool(PRINT_JSON, key, NULL, *val); > } else { > print_string(PRINT_FP, NULL, fmt, u8_to_bool(val)); > } > -- > 2.38.1 >
On Thu, Jan 5, 2023 at 5:11 AM Simon Horman <simon.horman@corigine.com> wrote: > > On Tue, Dec 27, 2022 at 10:36:20AM -0700, Maxim Georgiev wrote: > > Current implementation of show_bool_val() passes "val" parameter of pointer > > type as a last parameter to print_bool(): > > https://git.kernel.org/pub/scm/network/ethtool/ethtool.git/tree/netlink/netlink.h#n131 > > ... > > static inline void show_bool_val(const char *key, const char *fmt, uint8_t *val) > > { > > if (is_json_context()) { > > if (val) > > > print_bool(PRINT_JSON, key, NULL, val); > > } else { > > ... > > print_bool() expects the last parameter to be bool: > > https://git.kernel.org/pub/scm/network/ethtool/ethtool.git/tree/json_print.c#n153 > > ... > > void print_bool(enum output_type type, > > const char *key, > > const char *fmt, > > bool value) > > { > > ... > > Current show_bool_val() implementation converts "val" pointer to bool while > > calling show_bool_val(). As a result show_bool_val() always prints the value > > as "true" as long as it gets a non-null pointer to the boolean value, even if > > the referred boolean value is false. > > > > Fixes: 7e5c1ddbe67d ("pause: add --json support") > > Signed-off-by: Maxim Georgiev <glipus@gmail.com> > > Reviewed-by: Simon Horman <simon.horman@corigine.com> > > I'm assuming that val is never NULL :) Simon, thank you for taking a look! Yes, the "if (val)" check on line 130 guarantees that "print_bool()" on line 131 is called only if val is not null. > > > --- > > netlink/netlink.h | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/netlink/netlink.h b/netlink/netlink.h > > index 3240fca..1274a3b 100644 > > --- a/netlink/netlink.h > > +++ b/netlink/netlink.h > > @@ -128,7 +128,7 @@ static inline void show_bool_val(const char *key, const char *fmt, uint8_t *val) > > { > > if (is_json_context()) { > > if (val) > > - print_bool(PRINT_JSON, key, NULL, val); > > + print_bool(PRINT_JSON, key, NULL, *val); > > } else { > > print_string(PRINT_FP, NULL, fmt, u8_to_bool(val)); > > } > > -- > > 2.38.1 > >
On Fri, Jan 06, 2023 at 03:16:30PM -0700, Max Georgiev wrote: > On Thu, Jan 5, 2023 at 5:11 AM Simon Horman <simon.horman@corigine.com> wrote: > > > > On Tue, Dec 27, 2022 at 10:36:20AM -0700, Maxim Georgiev wrote: > > > Current implementation of show_bool_val() passes "val" parameter of pointer > > > type as a last parameter to print_bool(): > > > https://git.kernel.org/pub/scm/network/ethtool/ethtool.git/tree/netlink/netlink.h#n131 > > > ... > > > static inline void show_bool_val(const char *key, const char *fmt, uint8_t *val) > > > { > > > if (is_json_context()) { > > > if (val) > > > > print_bool(PRINT_JSON, key, NULL, val); > > > } else { > > > ... > > > print_bool() expects the last parameter to be bool: > > > https://git.kernel.org/pub/scm/network/ethtool/ethtool.git/tree/json_print.c#n153 > > > ... > > > void print_bool(enum output_type type, > > > const char *key, > > > const char *fmt, > > > bool value) > > > { > > > ... > > > Current show_bool_val() implementation converts "val" pointer to bool while > > > calling show_bool_val(). As a result show_bool_val() always prints the value > > > as "true" as long as it gets a non-null pointer to the boolean value, even if > > > the referred boolean value is false. > > > > > > Fixes: 7e5c1ddbe67d ("pause: add --json support") > > > Signed-off-by: Maxim Georgiev <glipus@gmail.com> > > > > Reviewed-by: Simon Horman <simon.horman@corigine.com> > > > > I'm assuming that val is never NULL :) > > Simon, thank you for taking a look! > Yes, the "if (val)" check on line 130 guarantees that "print_bool()" > on line 131 is called only if val is not null. Thanks, that is pretty obvious now you point it out. Looks good to me :) > > > --- > > > netlink/netlink.h | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/netlink/netlink.h b/netlink/netlink.h > > > index 3240fca..1274a3b 100644 > > > --- a/netlink/netlink.h > > > +++ b/netlink/netlink.h > > > @@ -128,7 +128,7 @@ static inline void show_bool_val(const char *key, const char *fmt, uint8_t *val) > > > { > > > if (is_json_context()) { > > > if (val) > > > - print_bool(PRINT_JSON, key, NULL, val); > > > + print_bool(PRINT_JSON, key, NULL, *val); > > > } else { > > > print_string(PRINT_FP, NULL, fmt, u8_to_bool(val)); > > > } > > > -- > > > 2.38.1 > > >
diff --git a/netlink/netlink.h b/netlink/netlink.h index 3240fca..1274a3b 100644 --- a/netlink/netlink.h +++ b/netlink/netlink.h @@ -128,7 +128,7 @@ static inline void show_bool_val(const char *key, const char *fmt, uint8_t *val) { if (is_json_context()) { if (val) - print_bool(PRINT_JSON, key, NULL, val); + print_bool(PRINT_JSON, key, NULL, *val); } else { print_string(PRINT_FP, NULL, fmt, u8_to_bool(val)); }
Current implementation of show_bool_val() passes "val" parameter of pointer type as a last parameter to print_bool(): https://git.kernel.org/pub/scm/network/ethtool/ethtool.git/tree/netlink/netlink.h#n131 ... static inline void show_bool_val(const char *key, const char *fmt, uint8_t *val) { if (is_json_context()) { if (val) > print_bool(PRINT_JSON, key, NULL, val); } else { ... print_bool() expects the last parameter to be bool: https://git.kernel.org/pub/scm/network/ethtool/ethtool.git/tree/json_print.c#n153 ... void print_bool(enum output_type type, const char *key, const char *fmt, bool value) { ... Current show_bool_val() implementation converts "val" pointer to bool while calling show_bool_val(). As a result show_bool_val() always prints the value as "true" as long as it gets a non-null pointer to the boolean value, even if the referred boolean value is false. Fixes: 7e5c1ddbe67d ("pause: add --json support") Signed-off-by: Maxim Georgiev <glipus@gmail.com> --- netlink/netlink.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)