Re: [PATCH] batctl: Add JSON debug support

[Sven Eckelmann <sven@narfation.org>]

[-- Attachment #1: Type: text/plain, Size: 4861 bytes --]

On Wednesday, 28 April 2021 12:16:08 CEST Alexander Sarmanow wrote:
> A JSON output of the debug tables is still missing. Corresponding JSON
> output is added for originators, neighbors, translocal, transglobal and
> interfaces tables.

The interface list is not a debug table.

Btw. you print strings without making sure it doesn't break the json. There is 
something like this in alfred:

		for (i = 0; i < data_len; i++) {
			if (pos[i] == '"')
				printf("\\\"");
			else if (pos[i] == '\\')
				printf("\\\\");
			else if (!isprint(pos[i]))
				printf("\\x%02x", pos[i]);
			else
				printf("%c", pos[i]);
		}

> Same parameters of the debug tables can be used for
> the JSON, except the "-w [interval]" (not useful). The table header is
> implemented as a JSON equivalent and can be also optionally omitted.
> 

I would like to disagree. Why is it a problem to continuously emit json 
objects? 

> @@ -21,8 +22,12 @@ struct print_opts {
>         float watch_interval;
>         nl_recvmsg_msg_cb_t callback;
>         char *remaining_header;
> +       char *remaining_entry;
>         const char *static_header;
>         uint8_t nl_cmd;
> +       bool is_json;
> +       bool is_first;
> +       bool is_last;

Please don't use bools here. Change it to something more  like

    uint8_t is_json:1;
    uint8_t is_first:1;
    uint8_t is_last:1;


> +$(eval $(call add_command,originators_json,y))
> +$(eval $(call add_command,neighbors_json,y))
> +$(eval $(call add_command,translocal_json,y))
> +$(eval $(call add_command,transglobal_json,y))
> +$(eval $(call add_command,interfaces_json,y))

Please inserted it sorted.


> --- a/netlink.c
> +++ b/netlink.c
> @@ -318,15 +318,29 @@ static int info_callback(struct nl_msg *msg, void *arg)
>                 else
>                         extra_header = "";
>  
> -               ret = asprintf(&opts->remaining_header,
> -                              "[B.A.T.M.A.N. adv %s, MainIF/MAC: %s/%02x:%02x:%02x:%02x:%02x:%02x (%s/%02x:%02x:%02x:%02x:%02x:%02x %s)%s]\n%s",
> -                              version, primary_if,
> -                              primary_mac[0], primary_mac[1], primary_mac[2],
> -                              primary_mac[3], primary_mac[4], primary_mac[5],
> -                              mesh_name,
> -                              mesh_mac[0], mesh_mac[1], mesh_mac[2],
> -                              mesh_mac[3], mesh_mac[4], mesh_mac[5],
> -                              algo_name, extra_info, extra_header);
> +               if (opts->is_json) {
> +                       ret = asprintf(&opts->remaining_header,
> +                                      "{\"version\":\"%s\",\"main_if\":\"%s\",\"main_mac\":\"%02x:%02x:%02x:%02x:%02x:%02x\",\"mesh_if\":\"%s\",\"mesh_mac\":\"%02x:%02x:%02x:%02x:%02x:%02x\",\"algo_name\":\"%s\",\"extra_info\":\"%s\",\"data\":[",
> +                                      version, primary_if,
> +                                      primary_mac[0], primary_mac[1],
> +                                      primary_mac[2], primary_mac[3],
> +                                      primary_mac[4], primary_mac[5],
> +                                      mesh_name,
> +                                      mesh_mac[0], mesh_mac[1], mesh_mac[2],
> +                                      mesh_mac[3], mesh_mac[4], mesh_mac[5],
> +                                      algo_name, extra_info);
> +               } else {
> +                       ret = asprintf(&opts->remaining_header,
> +                                      "[B.A.T.M.A.N. adv %s, MainIF/MAC: %s/%02x:%02x:%02x:%02x:%02x:%02x (%s/%02x:%02x:%02x:%02x:%02x:%02x %s)%s]\n%s",
> +                                      version, primary_if,
> +                                      primary_mac[0], primary_mac[1],
> +                                      primary_mac[2], primary_mac[3],
> +                                      primary_mac[4], primary_mac[5],
> +                                      mesh_name,
> +                                      mesh_mac[0], mesh_mac[1], mesh_mac[2],
> +                                      mesh_mac[3], mesh_mac[4], mesh_mac[5],
> +                                      algo_name, extra_info, extra_header);
> +               }

Do we really have to add such kind of output to each table? Can't we just have 
another command to get the meshif info and print it?

Regarding the actual command code - why do we have to have manual print code? 
Can't we just define a structures how things should be printed and then have a 
semi-generic print function for netlink responses (which just needs the 
netlink to json definition)? This would also avoid this extremely ugly print 
which you have and avoids implementation errors for new tables.

There is most likely more stuff in there but I will stop the review for now.

Kind regards,
	Sven

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]