Re: [PATCH v3 1/5] libsupport: add JSON output helpers

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Feb 20, 2018, at 2:59 AM, Viktor Prutyanov <viktor.prutyanov@xxxxxxxxxxxxx> wrote:
> 
> This patch adds JSON objects and lists and methods to print them

This patch also adds the JSON routines to dumpe2fs, but in a way that is
non-functional.  Those changes to dumpe2fs should be moved into the 2/3
patch so that git bisect does not break if only this patch is applied.

Secondly, my preference would be if the JSON functionality depended on a
"configure --enable-json" check that set JSON_CMT, JSON_MAN, and ENABLE_JSON
appropriately, so that this functionality could be #ifdef'd out if not needed.
It is fine to have JSON formatting enabled by default, but I don't think it
is needed in all environments.

More comments inline.

> Signed-off-by: Viktor Prutyanov <viktor.prutyanov@xxxxxxxxxxxxx>
> ---
> 
> new file mode 100644
> index 00000000..8c822f35
> --- /dev/null
> +++ b/lib/support/json-out.c
> @@ -0,0 +1,326 @@
> +/*
> + * json-out.c -- JSON output
> + *
> + * Copyright (c) 2018 Virtuozzo International GmbH
> + *
> + * %Begin-Header%
> + * This file may be redistributed under the terms of the GNU Public
> + * License.
> + * %End-Header%
> 
> +static struct json_pair *json_obj_add_pair(struct json_obj *obj, const char *key, enum json_val_type type)
> +{
> +	struct json_pair *new_pair = xmalloc(sizeof(struct json_pair));

(style) blank line after variable declaration.

> +	new_pair->key = xstrdup(key);
> +	new_pair->next = NULL;
> +	new_pair->type = type;
> +
> +	if (obj->pair) {
> +		struct json_pair *pair;
> +
> +		for (pair = obj->pair; pair && pair->next; pair = pair->next);
> +		pair->next = new_pair;
> +	} else
> +		obj->pair = new_pair;

(style) use {} in "else" block when "if" block has {}

> +void json_obj_add_str(struct json_obj *obj, const char *key, const char *str)
> +{
> +	struct json_pair *new_pair = json_obj_add_pair(obj, key, JSON_VAL_STRING);
> +
> +	new_pair->val.str = xstrdup(str);
> +}
> +
> +void json_obj_add_fmt_buf_str(struct json_obj *obj, const char *key, char *buf, size_t size, const char *fmt, ...)

(style) wrap line at 80 chars

> +{
> +	va_list ap;
> +
> +	va_start(ap, fmt);
> +	vsnprintf(buf, size, fmt, ap);
> +	va_end(ap);
> +
> +	json_obj_add_str(obj, key, buf);
> +}
> +
> +void json_obj_add_fmt_str(struct json_obj *obj, const char *key, size_t size, const char *fmt, ...)

(style) wrap line at 80 chars

> +{
> +	va_list ap;
> +	char *buf = malloc(size);
> +
> +	va_start(ap, fmt);
> +	vsnprintf(buf, size, fmt, ap);
> +	va_end(ap);
> +
> +	json_obj_add_str(obj, key, buf);
> +	free(buf);
> +}
> +
> +void json_obj_add_list(struct json_obj *obj, const char *key, struct json_list *list)

...

> +{
> +	struct json_pair *new_pair = json_obj_add_pair(obj, key, JSON_VAL_LIST);
> +
> +	new_pair->val.list = list;
> +}
> +
> +void json_obj_add_obj(struct json_obj *obj, const char *key, struct json_obj *new_obj)

...

> +{
> +	struct json_pair *new_pair = json_obj_add_pair(obj, key, JSON_VAL_OBJECT);
> +
> +	new_pair->val.obj = new_obj;
> +}
> +
> +static void json_pair_print_json(struct json_pair *pair, int ind_lvl);
> +static void json_list_node_print_json(struct json_list_node *node, enum json_val_type type, int ind_lvl);

...

> +
> +static void
> +print_ind(int ind_lvl)

(style) "static void" should be on the same line as "print_ind()".

IMHO, calling this function "print_indent()" would be more clear.  I thought
it was "print_index()"...

> +{
> +	int i;
> +
> +	putchar('\n');
> +	for (i = 0; i < ind_lvl; i++)
> +	fputs("  ", stdout);

(style) this line should be indented

> +static void
> +json_list_node_print_json(struct json_list_node *node, enum json_val_type type, int ind_lvl)

(style) "static void" on same line as function, wrap at 80 chars

> diff --git a/lib/support/json-out.h b/lib/support/json-out.h
> new file mode 100644
> index 00000000..0132bd2c
> --- /dev/null
> +++ b/lib/support/json-out.h
> @@ -0,0 +1,69 @@
> 
> +void json_obj_add_list(struct json_obj *obj, const char *key, struct json_list *list);
> +void json_obj_add_obj(struct json_obj *obj, const char *key, struct json_obj *new_obj);
> +void json_obj_add_fmt_str(struct json_obj *obj, const char *key, size_t size, const char *fmt, ...);
> +void json_obj_add_fmt_buf_str(struct json_obj *obj, const char *key, char *buf, size_t size, const char *fmt, ...);
> +struct json_list *json_list_create_in_obj(struct json_obj *parent_obj, char *key, enum json_val_type type);

(style) wrap lines at 80 chars

> diff --git a/misc/dumpe2fs.c b/misc/dumpe2fs.c
> index 395ea9ee..ca9953a1 100644
> --- a/misc/dumpe2fs.c
> +++ b/misc/dumpe2fs.c
> @@ -607,7 +607,12 @@ try_open_again:
> 			goto just_descriptors;
> 		list_super (fs->super);
> 		if (ext2fs_has_feature_journal_dev(fs->super)) {
> -			print_journal_information(fs);
> +			print_journal_information(fs, dump_obj);
> +			if (json) {
> +				json_obj_print_json(dump_obj, 0);
> +				putchar('\n');
> +				json_obj_delete(dump_obj);
> +			}

(defect) the "json" variable is not yet declared in this patch, it
is only added in patch 2/3, so all of these changes should also be
moved to the 2/3 patch.

> 			ext2fs_close_free(&fs);
> 			exit(0);
> 		}
> @@ -616,6 +621,11 @@ try_open_again:
> 			print_inline_journal_information(fs);
> 		list_bad_blocks(fs, 0);
> 		if (header_only) {
> +			if (json) {
> +				json_obj_print_json(dump_obj, 0);
> +				putchar('\n');
> +				json_obj_delete(dump_obj);
> +			}
> 			ext2fs_close_free(&fs);
> 			exit (0);
> 		}
> @@ -636,6 +646,11 @@ just_descriptors:
> 			       error_message(retval));
> 		}
> 	}
> +	if (json) {
> +		json_obj_print_json(dump_obj, 0);
> +		putchar('\n');
> +		json_obj_delete(dump_obj);
> +	}
> 	ext2fs_close_free(&fs);
> 	remove_error_table(&et_ext2_error_table);
> 	exit (0);
> --
> 2.14.1
> 


Cheers, Andreas





Attachment: signature.asc
Description: Message signed with OpenPGP


[Index of Archives]     [Reiser Filesystem Development]     [Ceph FS]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux FS]     [Yosemite National Park]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Device Mapper]     [Linux Media]

  Powered by Linux