| Message ID | 20180820214215.218127-1-mahesh@bandewar.net |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | stephen hemminger |
| Headers | show |
| Series | [iproute2] iproute: make clang happy | expand |
On Mon, 20 Aug 2018 14:42:15 -0700 Mahesh Bandewar <mahesh@bandewar.net> wrote: > > if (is_json_context()) { > + json_writer_t *jw; > + > open_json_object("bittiming"); > print_int(PRINT_ANY, "bitrate", NULL, bt->bitrate); > - jsonw_float_field_fmt(get_json_writer(), > - "sample_point", "%.3f", > - (float) bt->sample_point / 1000.); > + jw = get_json_writer(); > + jsonw_name(jw, "sample_point"); > + jsonw_printf(jw, "%.3f", > + (float) bt->sample_point / 1000); I think it would be better to get rid of the is_json_context() here in the CAN code and just use the print_json functions completely. Most of the other code is able to do that already.
On Mon, 20 Aug 2018 14:42:15 -0700 Mahesh Bandewar <mahesh@bandewar.net> wrote: > diff --git a/tc/m_ematch.c b/tc/m_ematch.c > index ace4b3dd738b..a524b520b276 100644 > --- a/tc/m_ematch.c > +++ b/tc/m_ematch.c > @@ -277,6 +277,7 @@ static int flatten_tree(struct ematch *head, struct ematch *tree) > return count; > } > > +__attribute__((format(printf, 5, 6))) > int em_parse_error(int err, struct bstr *args, struct bstr *carg, > struct ematch_util *e, char *fmt, ...) I think the printf attribute needs to go on the function prototype here: tc/m_ematch.h:extern int em_parse_error(int err, struct bstr *args, struct bstr *carg, PS: I need to take the extern of those function prototypes.
On Mon, Aug 20, 2018 at 3:52 PM, Stephen Hemminger <stephen@networkplumber.org> wrote: > On Mon, 20 Aug 2018 14:42:15 -0700 > Mahesh Bandewar <mahesh@bandewar.net> wrote: > >> diff --git a/tc/m_ematch.c b/tc/m_ematch.c >> index ace4b3dd738b..a524b520b276 100644 >> --- a/tc/m_ematch.c >> +++ b/tc/m_ematch.c >> @@ -277,6 +277,7 @@ static int flatten_tree(struct ematch *head, struct ematch *tree) >> return count; >> } >> >> +__attribute__((format(printf, 5, 6))) >> int em_parse_error(int err, struct bstr *args, struct bstr *carg, >> struct ematch_util *e, char *fmt, ...) > > I think the printf attribute needs to go on the function prototype > here: > tc/m_ematch.h:extern int em_parse_error(int err, struct bstr *args, struct bstr *carg, > The attributes are attached to the definitions only and not prototype declarations. Please see the definition/declaration for jsonw_printf() in the same patch. > > PS: I need to take the extern of those function prototypes.
On Mon, Aug 20, 2018 at 4:38 PM, Mahesh Bandewar (महेश बंडेवार) <maheshb@google.com> wrote: > On Mon, Aug 20, 2018 at 3:52 PM, Stephen Hemminger > <stephen@networkplumber.org> wrote: >> On Mon, 20 Aug 2018 14:42:15 -0700 >> Mahesh Bandewar <mahesh@bandewar.net> wrote: >> >>> diff --git a/tc/m_ematch.c b/tc/m_ematch.c >>> index ace4b3dd738b..a524b520b276 100644 >>> --- a/tc/m_ematch.c >>> +++ b/tc/m_ematch.c >>> @@ -277,6 +277,7 @@ static int flatten_tree(struct ematch *head, struct ematch *tree) >>> return count; >>> } >>> >>> +__attribute__((format(printf, 5, 6))) >>> int em_parse_error(int err, struct bstr *args, struct bstr *carg, >>> struct ematch_util *e, char *fmt, ...) >> >> I think the printf attribute needs to go on the function prototype >> here: >> tc/m_ematch.h:extern int em_parse_error(int err, struct bstr *args, struct bstr *carg, >> > The attributes are attached to the definitions only and not prototype > declarations. Please see the definition/declaration for jsonw_printf() > in the same patch. I take that back. Seems like it's fine either way. >> >> PS: I need to take the extern of those function prototypes.
On Mon, Aug 20, 2018 at 3:50 PM, Stephen Hemminger <stephen@networkplumber.org> wrote: > On Mon, 20 Aug 2018 14:42:15 -0700 > Mahesh Bandewar <mahesh@bandewar.net> wrote: > >> >> if (is_json_context()) { >> + json_writer_t *jw; >> + >> open_json_object("bittiming"); >> print_int(PRINT_ANY, "bitrate", NULL, bt->bitrate); >> - jsonw_float_field_fmt(get_json_writer(), >> - "sample_point", "%.3f", >> - (float) bt->sample_point / 1000.); >> + jw = get_json_writer(); >> + jsonw_name(jw, "sample_point"); >> + jsonw_printf(jw, "%.3f", >> + (float) bt->sample_point / 1000); > > I think it would be better to get rid of the is_json_context() here in the CAN code > and just use the print_json functions completely. Most of the other code is able to > do that already. Seems like this is not the only location and need to be taken care every where in the CAN code. I'll leave it to some JSON / print expert
On Mon, Aug 20, 2018 at 4:44 PM, Mahesh Bandewar (महेश बंडेवार) <maheshb@google.com> wrote: > On Mon, Aug 20, 2018 at 4:38 PM, Mahesh Bandewar (महेश बंडेवार) > <maheshb@google.com> wrote: >> On Mon, Aug 20, 2018 at 3:52 PM, Stephen Hemminger >> <stephen@networkplumber.org> wrote: >>> On Mon, 20 Aug 2018 14:42:15 -0700 >>> Mahesh Bandewar <mahesh@bandewar.net> wrote: >>> >>>> diff --git a/tc/m_ematch.c b/tc/m_ematch.c >>>> index ace4b3dd738b..a524b520b276 100644 >>>> --- a/tc/m_ematch.c >>>> +++ b/tc/m_ematch.c >>>> @@ -277,6 +277,7 @@ static int flatten_tree(struct ematch *head, struct ematch *tree) >>>> return count; >>>> } >>>> >>>> +__attribute__((format(printf, 5, 6))) >>>> int em_parse_error(int err, struct bstr *args, struct bstr *carg, >>>> struct ematch_util *e, char *fmt, ...) >>> >>> I think the printf attribute needs to go on the function prototype >>> here: >>> tc/m_ematch.h:extern int em_parse_error(int err, struct bstr *args, struct bstr *carg, >>> >> The attributes are attached to the definitions only and not prototype >> declarations. Please see the definition/declaration for jsonw_printf() >> in the same patch. > I take that back. Seems like it's fine either way. >>> >>> PS: I need to take the extern of those function prototypes. I can take care of this in v2
On Mon, 20 Aug 2018 16:44:28 -0700 Mahesh Bandewar (महेश बंडेवार) <maheshb@google.com> wrote: > On Mon, Aug 20, 2018 at 4:38 PM, Mahesh Bandewar (महेश बंडेवार) > <maheshb@google.com> wrote: > > On Mon, Aug 20, 2018 at 3:52 PM, Stephen Hemminger > > <stephen@networkplumber.org> wrote: > >> On Mon, 20 Aug 2018 14:42:15 -0700 > >> Mahesh Bandewar <mahesh@bandewar.net> wrote: > >> > >>> diff --git a/tc/m_ematch.c b/tc/m_ematch.c > >>> index ace4b3dd738b..a524b520b276 100644 > >>> --- a/tc/m_ematch.c > >>> +++ b/tc/m_ematch.c > >>> @@ -277,6 +277,7 @@ static int flatten_tree(struct ematch *head, struct ematch *tree) > >>> return count; > >>> } > >>> > >>> +__attribute__((format(printf, 5, 6))) > >>> int em_parse_error(int err, struct bstr *args, struct bstr *carg, > >>> struct ematch_util *e, char *fmt, ...) > >> > >> I think the printf attribute needs to go on the function prototype > >> here: > >> tc/m_ematch.h:extern int em_parse_error(int err, struct bstr *args, struct bstr *carg, > >> > > The attributes are attached to the definitions only and not prototype > > declarations. Please see the definition/declaration for jsonw_printf() > > in the same patch. > I take that back. Seems like it's fine either way. The reason to put the attributes in the .h file is that then the compiler can test for misuse in other files. For example if em_parse_error had a bad format in em_u32.c, then the warning would not happen unless the attribute was on the function prototype.
On Mon, Aug 20, 2018 at 5:52 PM, Stephen Hemminger <stephen@networkplumber.org> wrote: > On Mon, 20 Aug 2018 16:44:28 -0700 > Mahesh Bandewar (महेश बंडेवार) <maheshb@google.com> wrote: > >> On Mon, Aug 20, 2018 at 4:38 PM, Mahesh Bandewar (महेश बंडेवार) >> <maheshb@google.com> wrote: >> > On Mon, Aug 20, 2018 at 3:52 PM, Stephen Hemminger >> > <stephen@networkplumber.org> wrote: >> >> On Mon, 20 Aug 2018 14:42:15 -0700 >> >> Mahesh Bandewar <mahesh@bandewar.net> wrote: >> >> >> >>> diff --git a/tc/m_ematch.c b/tc/m_ematch.c >> >>> index ace4b3dd738b..a524b520b276 100644 >> >>> --- a/tc/m_ematch.c >> >>> +++ b/tc/m_ematch.c >> >>> @@ -277,6 +277,7 @@ static int flatten_tree(struct ematch *head, struct ematch *tree) >> >>> return count; >> >>> } >> >>> >> >>> +__attribute__((format(printf, 5, 6))) >> >>> int em_parse_error(int err, struct bstr *args, struct bstr *carg, >> >>> struct ematch_util *e, char *fmt, ...) >> >> >> >> I think the printf attribute needs to go on the function prototype >> >> here: >> >> tc/m_ematch.h:extern int em_parse_error(int err, struct bstr *args, struct bstr *carg, >> >> >> > The attributes are attached to the definitions only and not prototype >> > declarations. Please see the definition/declaration for jsonw_printf() >> > in the same patch. >> I take that back. Seems like it's fine either way. > > The reason to put the attributes in the .h file is that then the compiler > can test for misuse in other files. For example if em_parse_error had > a bad format in em_u32.c, then the warning would not happen unless > the attribute was on the function prototype. > correct, will take care in v2
diff --git a/include/json_writer.h b/include/json_writer.h index 9ab88e1dbdd9..a111cc62e7b2 100644 --- a/include/json_writer.h +++ b/include/json_writer.h @@ -59,8 +59,6 @@ void jsonw_luint_field(json_writer_t *self, const char *prop, unsigned long int num); void jsonw_lluint_field(json_writer_t *self, const char *prop, unsigned long long int num); -void jsonw_float_field_fmt(json_writer_t *self, const char *prop, - const char *fmt, double val); /* Collections */ void jsonw_start_object(json_writer_t *self); diff --git a/ip/iplink_can.c b/ip/iplink_can.c index 587413da15c4..c0deeb1f1fcf 100644 --- a/ip/iplink_can.c +++ b/ip/iplink_can.c @@ -316,11 +316,14 @@ static void can_print_opt(struct link_util *lu, FILE *f, struct rtattr *tb[]) struct can_bittiming *bt = RTA_DATA(tb[IFLA_CAN_BITTIMING]); if (is_json_context()) { + json_writer_t *jw; + open_json_object("bittiming"); print_int(PRINT_ANY, "bitrate", NULL, bt->bitrate); - jsonw_float_field_fmt(get_json_writer(), - "sample_point", "%.3f", - (float) bt->sample_point / 1000.); + jw = get_json_writer(); + jsonw_name(jw, "sample_point"); + jsonw_printf(jw, "%.3f", + (float) bt->sample_point / 1000); print_int(PRINT_ANY, "tq", NULL, bt->tq); print_int(PRINT_ANY, "prop_seg", NULL, bt->prop_seg); print_int(PRINT_ANY, "phase_seg1", @@ -415,12 +418,14 @@ static void can_print_opt(struct link_util *lu, FILE *f, struct rtattr *tb[]) RTA_DATA(tb[IFLA_CAN_DATA_BITTIMING]); if (is_json_context()) { + json_writer_t *jw; + open_json_object("data_bittiming"); print_int(PRINT_JSON, "bitrate", NULL, dbt->bitrate); - jsonw_float_field_fmt(get_json_writer(), - "sample_point", - "%.3f", - (float) dbt->sample_point / 1000.); + jw = get_json_writer(); + jsonw_name(jw, "sample_point"); + jsonw_printf(jw, "%.3f", + (float) dbt->sample_point / 1000.); print_int(PRINT_JSON, "tq", NULL, dbt->tq); print_int(PRINT_JSON, "prop_seg", NULL, dbt->prop_seg); print_int(PRINT_JSON, "phase_seg1", diff --git a/lib/color.c b/lib/color.c index eaf69e74d673..e5406294dfc4 100644 --- a/lib/color.c +++ b/lib/color.c @@ -132,6 +132,7 @@ void set_color_palette(void) is_dark_bg = 1; } +__attribute__((format(printf, 3, 4))) int color_fprintf(FILE *fp, enum color_attr attr, const char *fmt, ...) { int ret = 0; diff --git a/lib/json_print.c b/lib/json_print.c index 5dc41bfabfd4..77902824a738 100644 --- a/lib/json_print.c +++ b/lib/json_print.c @@ -100,6 +100,7 @@ void close_json_array(enum output_type type, const char *str) * functions handling different types */ #define _PRINT_FUNC(type_name, type) \ + __attribute__((format(printf, 4, 0))) \ void print_color_##type_name(enum output_type t, \ enum color_attr color, \ const char *key, \ diff --git a/lib/json_writer.c b/lib/json_writer.c index aa9ce1c65e51..68890b34ee92 100644 --- a/lib/json_writer.c +++ b/lib/json_writer.c @@ -152,6 +152,7 @@ void jsonw_name(json_writer_t *self, const char *name) putc(' ', self->out); } +__attribute__((format(printf, 2, 3))) void jsonw_printf(json_writer_t *self, const char *fmt, ...) { va_list ap; @@ -205,11 +206,6 @@ void jsonw_null(json_writer_t *self) jsonw_printf(self, "null"); } -void jsonw_float_fmt(json_writer_t *self, const char *fmt, double num) -{ - jsonw_printf(self, fmt, num); -} - void jsonw_float(json_writer_t *self, double num) { jsonw_printf(self, "%g", num); @@ -274,15 +270,6 @@ void jsonw_float_field(json_writer_t *self, const char *prop, double val) jsonw_float(self, val); } -void jsonw_float_field_fmt(json_writer_t *self, - const char *prop, - const char *fmt, - double val) -{ - jsonw_name(self, prop); - jsonw_float_fmt(self, fmt, val); -} - void jsonw_uint_field(json_writer_t *self, const char *prop, unsigned int num) { jsonw_name(self, prop); diff --git a/misc/ss.c b/misc/ss.c index 41e7762bb61f..93b1baf5dc40 100644 --- a/misc/ss.c +++ b/misc/ss.c @@ -976,6 +976,7 @@ static int buf_update(int len) } /* Append content to buffer as part of the current field */ +__attribute__((format(printf, 1, 2))) static void out(const char *fmt, ...) { struct column *f = current_field; @@ -1093,7 +1094,7 @@ static void print_header(void) { while (!field_is_last(current_field)) { if (!current_field->disabled) - out(current_field->header); + out("%s", current_field->header); field_next(); } } diff --git a/tc/m_ematch.c b/tc/m_ematch.c index ace4b3dd738b..a524b520b276 100644 --- a/tc/m_ematch.c +++ b/tc/m_ematch.c @@ -277,6 +277,7 @@ static int flatten_tree(struct ematch *head, struct ematch *tree) return count; } +__attribute__((format(printf, 5, 6))) int em_parse_error(int err, struct bstr *args, struct bstr *carg, struct ematch_util *e, char *fmt, ...) {