| Message ID | 20160621091210.092baa99@xeon-e3 |
|---|---|
| State | Rejected, archived |
| Delegated to: | stephen hemminger |
| Headers | show |
On Tue, Jun 21, 2016 at 9:12 AM, Stephen Hemminger <stephen@networkplumber.org> wrote: > On Mon, 20 Jun 2016 23:39:43 -0700 > Roopa Prabhu <roopa@cumulusnetworks.com> wrote: > >> From: Anuradha Karuppiah <anuradhak@cumulusnetworks.com> >> >> This patch adds a type qualifier to json_writer. Type can be a >> json object or array. This can be extended to other types like >> json-string, json-number etc in the future. >> >> Signed-off-by: Anuradha Karuppiah <anuradhak@cumulusnetworks.com> > > Since json writer is not used in many places yet, why not just > get rid of the automatic object in the constructor. I wanted to force the external api to start with an json-object or json-array. It reduces the chance of mistakes vs. a typeless constructor. With a typeless constructor you can accidentally end up with a json output that doesn't pass json lint; especially if optional params are being suppressed at different places. > > > > diff --git a/lib/json_writer.c b/lib/json_writer.c > index 2af16e1..5e588d8 100644 > --- a/lib/json_writer.c > +++ b/lib/json_writer.c > @@ -102,7 +102,6 @@ json_writer_t *jsonw_new(FILE *f) > self->depth = 0; > self->pretty = false; > self->sep = '\0'; > - putc('{', self->out); > } > return self; > } > @@ -114,7 +113,6 @@ void jsonw_destroy(json_writer_t **self_p) > > assert(self->depth == 0); > jsonw_eol(self); > - fputs("}\n", self->out); > fflush(self->out); > free(self); > *self_p = NULL; > diff --git a/misc/ifstat.c b/misc/ifstat.c > index abbb4e7..d9a7e50 100644 > --- a/misc/ifstat.c > +++ b/misc/ifstat.c > @@ -246,7 +246,6 @@ static void dump_raw_db(FILE *fp, int to_hist) > h = hist_db; > if (jw) { > jsonw_pretty(jw, pretty); > - jsonw_name(jw, info_source); > jsonw_start_object(jw); > } else > fprintf(fp, "#%s\n", info_source); > @@ -452,7 +451,6 @@ static void dump_kern_db(FILE *fp) > > if (jw) { > jsonw_pretty(jw, pretty); > - jsonw_name(jw, info_source); > jsonw_start_object(jw); > } else > print_head(fp); > @@ -466,8 +464,10 @@ static void dump_kern_db(FILE *fp) > else > print_one_if(fp, n, n->val); > } > - if (json_output) > - fprintf(fp, "\n} }\n"); > + if (jw) { > + jsonw_end_object(jw); > + jsonw_destroy(&jw); > + } > } > > static void dump_incr_db(FILE *fp) > @@ -478,7 +478,6 @@ static void dump_incr_db(FILE *fp) > h = hist_db; > if (jw) { > jsonw_pretty(jw, pretty); > - jsonw_name(jw, info_source); > jsonw_start_object(jw); > } else > print_head(fp); > diff --git a/misc/nstat.c b/misc/nstat.c > index a9e0f20..411cd87 100644 > --- a/misc/nstat.c > +++ b/misc/nstat.c > @@ -285,7 +285,6 @@ static void dump_kern_db(FILE *fp, int to_hist) > h = hist_db; > if (jw) { > jsonw_pretty(jw, pretty); > - jsonw_name(jw, info_source); > jsonw_start_object(jw); > } else > fprintf(fp, "#%s\n", info_source);
On Tue, 21 Jun 2016 09:24:50 -0700 Anuradha Karuppiah <anuradhak@cumulusnetworks.com> wrote: > On Tue, Jun 21, 2016 at 9:12 AM, Stephen Hemminger > <stephen@networkplumber.org> wrote: > > On Mon, 20 Jun 2016 23:39:43 -0700 > > Roopa Prabhu <roopa@cumulusnetworks.com> wrote: > > > >> From: Anuradha Karuppiah <anuradhak@cumulusnetworks.com> > >> > >> This patch adds a type qualifier to json_writer. Type can be a > >> json object or array. This can be extended to other types like > >> json-string, json-number etc in the future. > >> > >> Signed-off-by: Anuradha Karuppiah <anuradhak@cumulusnetworks.com> > > > > Since json writer is not used in many places yet, why not just > > get rid of the automatic object in the constructor. > > I wanted to force the external api to start with an json-object or > json-array. It reduces the chance of mistakes vs. a typeless > constructor. With a typeless constructor you can accidentally end up > with a json output that doesn't pass json lint; especially if optional > params are being suppressed at different places. Still, this is not how jsonwriter works in .NET, Android, or Java. It is easily confusing to developers if similar API's behave differently. Kind of like if printf() always appended a new line on some platforms.
diff --git a/lib/json_writer.c b/lib/json_writer.c index 2af16e1..5e588d8 100644 --- a/lib/json_writer.c +++ b/lib/json_writer.c @@ -102,7 +102,6 @@ json_writer_t *jsonw_new(FILE *f) self->depth = 0; self->pretty = false; self->sep = '\0'; - putc('{', self->out); } return self; } @@ -114,7 +113,6 @@ void jsonw_destroy(json_writer_t **self_p) assert(self->depth == 0); jsonw_eol(self); - fputs("}\n", self->out); fflush(self->out); free(self); *self_p = NULL; diff --git a/misc/ifstat.c b/misc/ifstat.c index abbb4e7..d9a7e50 100644 --- a/misc/ifstat.c +++ b/misc/ifstat.c @@ -246,7 +246,6 @@ static void dump_raw_db(FILE *fp, int to_hist) h = hist_db; if (jw) { jsonw_pretty(jw, pretty); - jsonw_name(jw, info_source); jsonw_start_object(jw); } else fprintf(fp, "#%s\n", info_source); @@ -452,7 +451,6 @@ static void dump_kern_db(FILE *fp) if (jw) { jsonw_pretty(jw, pretty); - jsonw_name(jw, info_source); jsonw_start_object(jw); } else print_head(fp); @@ -466,8 +464,10 @@ static void dump_kern_db(FILE *fp) else print_one_if(fp, n, n->val); } - if (json_output) - fprintf(fp, "\n} }\n"); + if (jw) { + jsonw_end_object(jw); + jsonw_destroy(&jw); + } } static void dump_incr_db(FILE *fp) @@ -478,7 +478,6 @@ static void dump_incr_db(FILE *fp) h = hist_db; if (jw) { jsonw_pretty(jw, pretty); - jsonw_name(jw, info_source); jsonw_start_object(jw); } else print_head(fp); diff --git a/misc/nstat.c b/misc/nstat.c index a9e0f20..411cd87 100644 --- a/misc/nstat.c +++ b/misc/nstat.c @@ -285,7 +285,6 @@ static void dump_kern_db(FILE *fp, int to_hist) h = hist_db; if (jw) { jsonw_pretty(jw, pretty); - jsonw_name(jw, info_source); jsonw_start_object(jw); } else fprintf(fp, "#%s\n", info_source);