On Monday, 10 May 2021 09:58:26 CEST asarmanow@gmail.com wrote:
Sven Eckelmann wrote:
Why are you only printing selected attributes and not all?
I wanted to reduce the amount to query for the netlink_print_json_entries function. There are only a few of the attributes needed for each of the JSON commands.
You are doing currently two queries (one BATADV_CMD_GET_MESH_INFO and one for the actual data). I want to you to get rid of the BATADV_CMD_GET_MESH_INFO and print all data which was returned for the actual data query.
So I don't see a reason why you're approach would help here at all. Just go with netlink_print_json_entries through all entries in attrs till BATADV_ATTR_MAX and check if you have a function to print them. If you have, just print it.
You can then also get rid of missing_mandatory_attrs and also extra checks like MULTICAST_ONLY/UNICAST_ONLY/NO_OLD_ORIGS. The json is not for human consumption and thus we don't need special flags to prefilter the data. The consumer can take care of processing the data.
Btw. I am also more of a fan of following style of struct/array initialization.
struct nla_policy_json batadv_netlink_policy_json[NUM_BATADV_ATTR] = { [BATADV_ATTR_VERSION] = { .name = "version", .cb = nljson_print_str }, [.....]
I have to change this at some point in netlink.c
+void create_json_print_string(const char *str, const char **str_ret) +{
size_t needed = snprintf(NULL, 0, "\"%s\"", str);char *str_buf = malloc(needed+1);sprintf(str_buf, "\"%s\"", str);*str_ret = str_buf;+}
Another thing I saw: you allocate a lot of strings but never free them. Can we just avoid to allocate them?
Other than that - why? This function doesn't make a lot of sense in the first place.
[BATADV_ATTR_FLAG_BEST] = { .name = "best",.cb = nljson_print_bool},
Also: BATADV_ATTR_FLAG_BEST is not a boolean - it is a flag. So it can be present and absent. So is comparable to an HTML attribute without value (for example "disabled"). I think we can encode it as "best": true when it exists.
+void nljson_print_ttflags(struct json_opts *json_opts, struct print_opts *opts,
struct nlattr *attrs[], int idx)+{
const char *key_ls;uint32_t flags;char r, n, x, w, i, p, t;flags = nla_get_u32(attrs[idx]);r = '.', p = '.', n = '.', x = '.', w = '.', i = '.', t = '.';if (opts->nl_cmd == BATADV_CMD_GET_TRANSTABLE_LOCAL) {
[...]
} else {if (flags & BATADV_TT_CLIENT_ROAM)r = 'R';if (flags & BATADV_TT_CLIENT_WIFI)w = 'W';if (flags & BATADV_TT_CLIENT_ISOLA)i = 'I';if (flags & BATADV_TT_CLIENT_TEMP)t = 'T';goto print_global_table;}+print_local_table:
printf("\"%c%c%c%c%c%c\"", r, n, x, w, i, p);printf(",");create_json_print_string(batadv_netlink_policy_json[BATADV_ATTR_LAST_SEEN_MSECS].name,&key_ls);sanitize_string(key_ls, strlen(key_ls));printf(":");nljson_print_int(json_opts, opts, attrs, BATADV_ATTR_LAST_SEEN_MSECS);return;+print_global_table:
printf("\"%c%c%c%c\"", r, w, i, t);+}
nljson_print_ttflags is also odd. If you decode the binary, why are you creating a compact representation again? Why aren't you just create a sub object with the parsed data? And there is no need to differentiate between BATADV_CMD_GET_TRANSTABLE_LOCAL and BATADV_CMD_GET_TRANSTABLE_GLOBAL.
And why is there a goto in the first place at the end of each scope?
And you interpretation of BATADV_ATTR_LAST_SEEN_MSECS seems to be really messed up. Just don't print BATADV_ATTR_LAST_SEEN_MSECS when it is not there - which can be evaluated automatically. And just as info: the kernel will not send it when the nopurge bit is set. So it is absolutely not what you've coded here.
+void nljson_print_crc32(struct json_opts *json_opts, struct print_opts *opts,
struct nlattr *attrs[], int idx)+{
(void) json_opts;(void) opts;int32_t value = nla_get_u32(attrs[idx]);printf("\"0x%.8x\"", value);+}
Can we just print the raw 32 bit unsigned integer?
Also, please not the name but the actual number in places were you are now calling nljson_print_ifname_by_ifindex. Otherwise you would print the name twice and make unnecessary syscalls. And if you want to print the actual name for things like the originator/neighbor dump, maybe just add it to the generic netlink message in the kernel.
+void netlink_print_json_entries(struct nlattr *attrs[], int selected_attrs[],
int arr_len, struct print_opts *opts)+{
const char *name;uint8_t first_valid_attr = 1;int idx, i;struct json_opts json_opts = {.use_alt_int_val = 0,.alt_int_val = 0,};if (!opts->is_first)printf(",");for (i = 0; i < arr_len; i++) {idx = selected_attrs[i];if (attrs[idx] && batadv_netlink_policy_json[idx].cb) {if (!first_valid_attr)printf(",");elseprintf("{");create_json_print_string(batadv_netlink_policy_json[idx].name,&name);sanitize_string(name, strlen(name));printf(":");batadv_netlink_policy_json[idx].cb(&json_opts, opts,attrs, idx);first_valid_attr = 0;}}if (!first_valid_attr)printf("}");+}
Why aren't you handling the is_first = 0 in this function? And why aren't you printing empty objects?
And please try to handle rejections and not acceptance. So instead of:
for (i = 0; i < arr_len; i++) { idx = selected_attrs[i];
if (attrs[idx] && batadv_netlink_policy_json[idx].cb) {
Maybe something like:
for (i = 0; i < arr_len; i++) { [...] if (!attrs[idx]) continue;
if (!batadv_netlink_policy_json[idx].cb) continue;
+void sanitize_string(const char *str, int str_len) +{
int i;for (i = 0; i < str_len; i++) {if (str[i] == '"')printf("\"");else if (str[i] == '\\')printf("\\\\");else if (!isprint(str[i]))printf("\\x%02x", str[i]);elseprintf("%c", str[i]);}+}
I know, it is the similar in alfred but maybe you can change it to something like:
void sanitize_string(const char *str) { while (*str) { if (*str == '"') puts("""); else if (*str == '\') puts("\\"); else if (!isprint(*str)) printf("\x%02x", *str); else putc(*str);
str++; } }
+static int meshinfo_callback(struct nl_msg *msg, void *arg) {
[...]
int selected_attrs[10] = { BATADV_ATTR_MESH_IFNAME,BATADV_ATTR_MESH_ADDRESS,BATADV_ATTR_HARD_IFNAME,BATADV_ATTR_VERSION,BATADV_ATTR_ALGO_NAME,BATADV_ATTR_HARD_ADDRESS,BATADV_ATTR_TT_TTVN };
I hope to get rid of this anyway but this should have been:
static const enum batadv_nl_attrs selected_attrs[] = { BATADV_ATTR_MESH_IFNAME, BATADV_ATTR_MESH_ADDRESS, BATADV_ATTR_HARD_IFNAME, BATADV_ATTR_VERSION, BATADV_ATTR_ALGO_NAME, BATADV_ATTR_HARD_ADDRESS, BATADV_ATTR_TT_TTVN, }
So no hardcoded (and wrong) length, correct type, constant, not allocated on the stack, different identation.
+static int netlink_print_meshinfo_json(struct state *state, char *orig_iface,
int read_opts, float orig_timeout,float watch_interval)+{
(void) orig_iface;(void) orig_timeout;(void) watch_interval;(void) read_opts;
Please annotate unused parameters correctly with __maybe_unused
Also use the already existing netlink socket and don't create a new one. Maybe even with sys_simple_nlquery. Most functionality which creates its own batadv generic netlink socket only does it because it needed to support the old sysfs functionality in parallel in the past. And no on found the time to clean this up after the sysfs stuff was dropped.
If we don't need the "fancy" features of the debug tables then we can also use the functionality sys_simple_nlquery for the rest. At least we don't have the bat-host handling anymore, header is dropped and the filter might be dropped. And the only reason why I was against the special code to prevent the "watch" functionality was that we need to introduce a new hack for it.
Just make sure that you allow the caller to change the 6th parameter of genlmsg_put. Maybe by introducing a new function which allows to set this parameter. And the sys_simple_nlquery is changed to a wrapper which omits the new "flags" parameter.
And then you can also add the putc for '[' and ']' directly to the caller of sys_simple_nlquery (or whatever the new function will be called).
+#define BOOL_STRING(x) (x ? "true" : "false")
I don't see a lot of benefit in this macro.
- Andrew Lunn <andrew(a)lunn.ch>
- Sven Eckelmann <sven(a)narfation.org>
- Alexander Sarmanow <asarmanow(a)gmail.com>
No fancy html tags in headers please.
Overall: please run you patches through Linux's scripts/checkpatch.pl --strict and check what of the things you see is only relevant for the kernel and what might be actual things you should clean up.
[BATADV_ATTR_MESH_IFNAME] = { .name = "mesh_if",.cb = nljson_print_str },[BATADV_ATTR_MESH_ADDRESS] = { .name = "mesh_address",.cb = nljson_print_mac},[BATADV_ATTR_ORIG_ADDRESS] = { .name = "originator",.cb = nljson_print_mac },
Please keep the same order as the attributes in batman_adv.h
Missing print functions:
* BATADV_ATTR_TPMETER_RESULT * BATADV_ATTR_TPMETER_TEST_TIME * BATADV_ATTR_TPMETER_BYTES * BATADV_ATTR_TPMETER_COOKIE * BATADV_ATTR_ACTIVE * BATADV_ATTR_BANDWIDTH_UP * BATADV_ATTR_BANDWIDTH_DOWN * BATADV_ATTR_ROUTER * BATADV_ATTR_BLA_OWN * BATADV_ATTR_BLA_ADDRESS * BATADV_ATTR_BLA_VID * BATADV_ATTR_BLA_BACKBONE * BATADV_ATTR_DAT_CACHE_IP4ADDRESS * BATADV_ATTR_DAT_CACHE_HWADDRESS * BATADV_ATTR_DAT_CACHE_VID * BATADV_ATTR_MCAST_FLAGS * BATADV_ATTR_MCAST_FLAGS_PRIV * BATADV_ATTR_VLANID * BATADV_ATTR_AGGREGATED_OGMS_ENABLED * BATADV_ATTR_AP_ISOLATION_ENABLED * BATADV_ATTR_ISOLATION_MARK * BATADV_ATTR_ISOLATION_MASK * BATADV_ATTR_BONDING_ENABLED * BATADV_ATTR_BRIDGE_LOOP_AVOIDANCE_ENABLED * BATADV_ATTR_DISTRIBUTED_ARP_TABLE_ENABLED * BATADV_ATTR_FRAGMENTATION_ENABLED * BATADV_ATTR_GW_BANDWIDTH_DOWN * BATADV_ATTR_GW_BANDWIDTH_UP * BATADV_ATTR_GW_MODE * BATADV_ATTR_GW_SEL_CLASS * BATADV_ATTR_HOP_PENALTY * BATADV_ATTR_LOG_LEVEL * BATADV_ATTR_MULTICAST_FORCEFLOOD_ENABLED * BATADV_ATTR_NETWORK_CODING_ENABLED * BATADV_ATTR_ORIG_INTERVAL * BATADV_ATTR_ELP_INTERVAL * BATADV_ATTR_THROUGHPUT_OVERRIDE * BATADV_ATTR_MULTICAST_FANOUT
[BATADV_ATTR_MESH_IFINDEX] = { .name = "mesh_if_idx",
meshif_idx or mesh_ifindex
[BATADV_ATTR_MESH_IFNAME] = { .name = "mesh_if",
meshif or mesh_ifname
[BATADV_ATTR_HARD_IFINDEX] = { .name = "hard_if_idx",
hardif_idx or hard_ifindex
[BATADV_ATTR_HARD_IFNAME] = { .name = "hard_if",
hardif or hard_ifname
[BATADV_ATTR_TT_ADDRESS] = { .name = "client",
why client? It is not really a client.
tt_address
[BATADV_ATTR_TT_VID] = { .name = "vid",
tt_vid
[BATADV_ATTR_TT_FLAGS] = { .name = "flags",
tt_flags
[BATADV_ATTR_LAST_SEEN_MSECS] = { .name = "last_seen",
last_seen_msecs
[BATADV_ATTR_NEIGH_ADDRESS] = { .name = "neighbor",
neigh_address
@@ -21,8 +21,22 @@ 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;uint8_t is_json;uint8_t is_first;+};
uint8_t is_json:1 uint8_t is_first:1
And what is remaining_entry?
Kind regards, Sven