occtl: Fix 'show ip bans' may produce invalid JSON (#683)

Modified the printing logic to use a 'comma-before' approach instead of
'comma-after'.

Signed-off-by: Grigory Trenin <grigory.trenin@gmail.com>

NEWS

@@ -1,5 +1,6 @@
 * Version 1.4.1 (unreleased)
 - occtl: Fix column misalignment in ban command outputs
+- occtl: Fix 'show ip bans' may produce invalid JSON (#683)
 - Handle dotted client hostnames (e.g., .local) by stripping the domain suffix
 
 

src/occtl/occtl.h

@@ -1,6 +1,7 @@
 #ifndef OCCTL_H
 #define OCCTL_H
 
+#include <stdbool.h>
 #include <stdlib.h>
 #include <time.h>
 #include "common.h"
@@ -35,10 +36,12 @@ int print_fwport_entries(FILE *out, cmd_params_st *params, const char *name,
 			 FwPortSt **val, unsigned int vsize,
 			 unsigned int have_more);
 void print_start_block(FILE *out, cmd_params_st *params);
+void print_end_block_simple(FILE *out, cmd_params_st *params);
 void print_end_block(FILE *out, cmd_params_st *params, unsigned int have_more);
 void print_array_block(FILE *out, cmd_params_st *params);
 void print_end_array_block(FILE *out, cmd_params_st *params);
 void print_separator(FILE *out, cmd_params_st *params);
+void print_value_separator(FILE *out, cmd_params_st *params, bool need_comma);
 void print_single_value(FILE *out, cmd_params_st *params, const char *name,
 			const char *value, unsigned int have_more);
 void print_single_value_int(FILE *out, cmd_params_st *params, const char *name,

src/occtl/print.c

@@ -124,6 +124,12 @@ void print_start_block(FILE *out, cmd_params_st *params)
 		fprintf(out, "  {\n");
 }
 
+void print_end_block_simple(FILE *out, cmd_params_st *params)
+{
+	if (HAVE_JSON(params))
+		fprintf(out, "  }");
+}
+
 void print_end_block(FILE *out, cmd_params_st *params, unsigned int have_more)
 {
 	if (HAVE_JSON(params))
@@ -148,6 +154,12 @@ void print_separator(FILE *out, cmd_params_st *params)
 		fprintf(out, "\n");
 }
 
+void print_value_separator(FILE *out, cmd_params_st *params, bool need_comma)
+{
+	if (HAVE_JSON(params) && need_comma)
+		fprintf(out, ",\n");
+}
+
 void print_single_value(FILE *out, cmd_params_st *params, const char *name,
 			const char *value, unsigned int have_more)
 {

src/occtl/unix.c

@@ -1078,7 +1078,7 @@ static int handle_list_banned_cmd(struct unix_ctx *ctx, const char *arg,
 	FILE *out;
 	struct tm *tm, _tm;
 	time_t t;
-	bool header_printed = false;
+	bool item_printed = false;
 
 	PROTOBUF_ALLOCATOR(pa, ctx);
 	char txt_ip[MAX_IP_STR];
@@ -1125,11 +1125,10 @@ static int handle_list_banned_cmd(struct unix_ctx *ctx, const char *arg,
 				continue;
 			}
 
-			if (!header_printed && NO_JSON(params)) {
+			if (!item_printed && NO_JSON(params))
 				fprintf(out, "%15s %10s %30s\n", "IP", "score",
 					"expires");
-				header_printed = true;
-			}
+			print_value_separator(out, params, item_printed);
 			print_start_block(out, params);
 
 			print_time_ival7(tmpbuf, t, time(NULL));
@@ -1146,10 +1145,9 @@ static int handle_list_banned_cmd(struct unix_ctx *ctx, const char *arg,
 					rep->info[i]->score, str_since, tmpbuf);
 			}
 		} else {
-			if (!header_printed && NO_JSON(params)) {
+			if (!item_printed && NO_JSON(params))
 				fprintf(out, "%15s %10s\n", "IP", "score");
-				header_printed = true;
-			}
+			print_value_separator(out, params, item_printed);
 			print_start_block(out, params);
 
 			if (HAVE_JSON(params)) {
@@ -1163,11 +1161,14 @@ static int handle_list_banned_cmd(struct unix_ctx *ctx, const char *arg,
 			}
 		}
 
-		print_end_block(out, params, i < (rep->n_info - 1) ? 1 : 0);
+		print_end_block_simple(out, params);
+		item_printed = true;
 
 		ip_entries_add(ctx, txt_ip, strlen(txt_ip));
 	}
 
+	if (item_printed && HAVE_JSON(params))
+		fprintf(out, "\n");
 	print_end_array_block(out, params);
 
 	ret = 0;

tests/test-ban

@@ -59,6 +59,25 @@ echo "notest" | ${CMDNS1} ${OPENCONNECT} --passwd-on-stdin -q ${ADDRESS}:${PORT}
 echo "notest" | ${CMDNS1} ${OPENCONNECT} --passwd-on-stdin -q ${ADDRESS}:${PORT} -u test --authenticate --servercert=pin-sha256:xp3scfzy3rOQsv9NcOve/8YVVv+pHr4qNCXEXrNl5s8=
 echo "notest" | ${CMDNS1} ${OPENCONNECT} --passwd-on-stdin -q ${ADDRESS}:${PORT} -u test --authenticate --servercert=pin-sha256:xp3scfzy3rOQsv9NcOve/8YVVv+pHr4qNCXEXrNl5s8=
 
+# For the JSON validity test, multiple IP addresses must be present in the ban list.
+# Connect from a different IP address to add another IP to the list.
+echo "notest" | ${CMDNS2} ${OPENCONNECT} --passwd-on-stdin -q ${ADDRESS}:${PORT} -u test --authenticate --servercert=pin-sha256:xp3scfzy3rOQsv9NcOve/8YVVv+pHr4qNCXEXrNl5s8=
+
+BAN_POINTS_CNT=$(${OCCTL} -s ${OCCTL_SOCKET} --json show ip ban points | jq '. | length')
+if [ $? -ne 0 ]; then
+	fail $PID "Failed to retrieve IP ban points: invalid JSON format"
+fi
+if [ "${BAN_POINTS_CNT}" -ne 2 ]; then
+	fail $PID "Unexpected IP ban points count (expected 2, got ${BAN_POINTS_CNT})"
+fi
+BAN_CNT=$(${OCCTL} -s ${OCCTL_SOCKET} --json show ip bans | jq '. | length')
+if [ $? -ne 0 ]; then
+	fail $PID "Failed to retrieve IP bans: invalid JSON format"
+fi
+if [ "${BAN_CNT}" -ne 1 ]; then
+	fail $PID "Unexpected IP bans count (expected 1, got ${BAN_CNT})"
+fi
+
 echo ""
 echo "Connecting with correct password... "
 eval `echo "test" | ${CMDNS1} ${OPENCONNECT} --passwd-on-stdin -q ${ADDRESS}:${PORT} -u test --authenticate --servercert=pin-sha256:xp3scfzy3rOQsv9NcOve/8YVVv+pHr4qNCXEXrNl5s8=`