[PATCH iproute2 1/2] tc/taprio: don't print netlink attributes which weren't reported by the kernel

[PATCH iproute2 1/2] tc/taprio: don't print netlink attributes which weren't reported by the kernel

[Vladimir Oltean]

When an admin schedule is pending and hasn't yet become operational, the
kernel will report only the parameters of the admin schedule in a nested
TCA_TAPRIO_ATTR_ADMIN_SCHED attribute.

However, we default to printing zeroes even for the parameters of the
operational base time, when that doesn't exist.

Link: https://lore.kernel.org/netdev/87il9w0xx7.fsf@intel.com/
Fixes: 0dd16449356f ("tc: Add support for configuring the taprio scheduler")
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 tc/q_taprio.c | 91 ++++++++++++++++++++++++++++++---------------------
 1 file changed, 53 insertions(+), 38 deletions(-)

diff --git a/tc/q_taprio.c b/tc/q_taprio.c
index 913197f68caa..795c013c1c2a 100644
--- a/tc/q_taprio.c
+++ b/tc/q_taprio.c
@@ -416,14 +416,11 @@ static int taprio_parse_opt(struct qdisc_util *qu, int argc,
 	return 0;
 }
 
-static int print_sched_list(FILE *f, struct rtattr *list)
+static void print_sched_list(FILE *f, struct rtattr *list)
 {
-	struct rtattr *item;
+	struct rtattr *item, *nla;
 	int rem;
 
-	if (list == NULL)
-		return 0;
-
 	rem = RTA_PAYLOAD(list);
 
 	open_json_array(PRINT_JSON, "schedule");
@@ -432,60 +429,78 @@ static int print_sched_list(FILE *f, struct rtattr *list)
 
 	for (item = RTA_DATA(list); RTA_OK(item, rem); item = RTA_NEXT(item, rem)) {
 		struct rtattr *tb[TCA_TAPRIO_SCHED_ENTRY_MAX + 1];
-		__u32 index = 0, gatemask = 0, interval = 0;
-		__u8 command = 0;
+		__u32 index, gatemask, interval;
+		__u8 command;
 
 		parse_rtattr_nested(tb, TCA_TAPRIO_SCHED_ENTRY_MAX, item);
 
-		if (tb[TCA_TAPRIO_SCHED_ENTRY_INDEX])
-			index = rta_getattr_u32(tb[TCA_TAPRIO_SCHED_ENTRY_INDEX]);
+		open_json_object(NULL);
+
+		nla = tb[TCA_TAPRIO_SCHED_ENTRY_INDEX];
+		if (nla) {
+			index = rta_getattr_u32(nla);
+			print_uint(PRINT_ANY, "index", "\tindex %u", index);
+		}
 
-		if (tb[TCA_TAPRIO_SCHED_ENTRY_CMD])
-			command = rta_getattr_u8(tb[TCA_TAPRIO_SCHED_ENTRY_CMD]);
+		nla = tb[TCA_TAPRIO_SCHED_ENTRY_CMD];
+		if (nla) {
+			command = rta_getattr_u8(nla);
+			print_string(PRINT_ANY, "cmd", " cmd %s",
+				     entry_cmd_to_str(command));
+		}
 
-		if (tb[TCA_TAPRIO_SCHED_ENTRY_GATE_MASK])
-			gatemask = rta_getattr_u32(tb[TCA_TAPRIO_SCHED_ENTRY_GATE_MASK]);
+		nla = tb[TCA_TAPRIO_SCHED_ENTRY_GATE_MASK];
+		if (nla) {
+			gatemask = rta_getattr_u32(nla);
+			print_0xhex(PRINT_ANY, "gatemask", " gatemask %#llx",
+				    gatemask);
+		}
 
-		if (tb[TCA_TAPRIO_SCHED_ENTRY_INTERVAL])
-			interval = rta_getattr_u32(tb[TCA_TAPRIO_SCHED_ENTRY_INTERVAL]);
+		nla = tb[TCA_TAPRIO_SCHED_ENTRY_INTERVAL];
+		if (nla) {
+			interval = rta_getattr_u32(nla);
+			print_uint(PRINT_ANY, "interval", " interval %u",
+				   interval);
+		}
 
-		open_json_object(NULL);
-		print_uint(PRINT_ANY, "index", "\tindex %u", index);
-		print_string(PRINT_ANY, "cmd", " cmd %s", entry_cmd_to_str(command));
-		print_0xhex(PRINT_ANY, "gatemask", " gatemask %#llx", gatemask);
-		print_uint(PRINT_ANY, "interval", " interval %u", interval);
 		close_json_object();
 
 		print_nl();
 	}
 
 	close_json_array(PRINT_ANY, "");
-
-	return 0;
 }
 
 static int print_schedule(FILE *f, struct rtattr **tb)
 {
-	int64_t base_time = 0, cycle_time = 0, cycle_time_extension = 0;
-
-	if (tb[TCA_TAPRIO_ATTR_SCHED_BASE_TIME])
-		base_time = rta_getattr_s64(tb[TCA_TAPRIO_ATTR_SCHED_BASE_TIME]);
+	int64_t base_time, cycle_time, cycle_time_extension;
+	struct rtattr *nla;
+
+	nla = tb[TCA_TAPRIO_ATTR_SCHED_BASE_TIME];
+	if (nla) {
+		base_time = rta_getattr_s64(nla);
+		print_lluint(PRINT_ANY, "base_time", "\tbase-time %lld",
+			     base_time);
+	}
 
-	if (tb[TCA_TAPRIO_ATTR_SCHED_CYCLE_TIME])
+	nla = tb[TCA_TAPRIO_ATTR_SCHED_CYCLE_TIME];
+	if (nla) {
 		cycle_time = rta_getattr_s64(tb[TCA_TAPRIO_ATTR_SCHED_CYCLE_TIME]);
+		print_lluint(PRINT_ANY, "cycle_time", " cycle-time %lld",
+			     cycle_time);
+	}
 
-	if (tb[TCA_TAPRIO_ATTR_SCHED_CYCLE_TIME_EXTENSION])
-		cycle_time_extension = rta_getattr_s64(
-			tb[TCA_TAPRIO_ATTR_SCHED_CYCLE_TIME_EXTENSION]);
-
-	print_lluint(PRINT_ANY, "base_time", "\tbase-time %lld", base_time);
-
-	print_lluint(PRINT_ANY, "cycle_time", " cycle-time %lld", cycle_time);
-
-	print_lluint(PRINT_ANY, "cycle_time_extension",
-		     " cycle-time-extension %lld", cycle_time_extension);
+	nla = tb[TCA_TAPRIO_ATTR_SCHED_CYCLE_TIME_EXTENSION];
+	if (nla) {
+		cycle_time_extension = rta_getattr_s64(nla);
+		print_lluint(PRINT_ANY, "cycle_time_extension",
+			     " cycle-time-extension %lld",
+			     cycle_time_extension);
+	}
 
-	print_sched_list(f, tb[TCA_TAPRIO_ATTR_SCHED_ENTRY_LIST]);
+	nla = tb[TCA_TAPRIO_ATTR_SCHED_ENTRY_LIST];
+	if (nla)
+		print_sched_list(f, nla);
 
 	return 0;
 }
-- 
2.34.1

[PATCH iproute2 2/2] tc/taprio: fix JSON output when TCA_TAPRIO_ATTR_ADMIN_SCHED is present

[Vladimir Oltean]

When the kernel reports that a configuration change is pending
(and that the schedule is still in the administrative state and
not yet operational), we (tc -j -p qdisc show) produce the following
output:

[ {
        "kind": "taprio",
        "handle": "8001:",
        "root": true,
        "refcnt": 9,
        "options": {
            "tc": 8,
            "map": [ 0,1,2,3,4,5,6,7,0,0,0,0,0,0,0,0 ],
            "queues": [ {
                    "offset": 0,
                    "count": 1
                },{
                    "offset": 1,
                    "count": 1
                },{
                    "offset": 2,
                    "count": 1
                },{
                    "offset": 3,
                    "count": 1
                },{
                    "offset": 4,
                    "count": 1
                },{
                    "offset": 5,
                    "count": 1
                },{
                    "offset": 6,
                    "count": 1
                },{
                    "offset": 7,
                    "count": 1
                } ],
            "clockid": "TAI",
            "base_time": 0,
            "cycle_time": 20000000,
            "cycle_time_extension": 0,
            "schedule": [ {
                    "index": 0,
                    "cmd": "S",
                    "gatemask": "0xff",
                    "interval": 20000000
                } ],{
                "base_time": 1691160103110424418,
                "cycle_time": 20000000,
                "cycle_time_extension": 0,
                "schedule": [ {
                        "index": 0,
                        "cmd": "S",
                        "gatemask": "0xff",
                        "interval": 20000000
                    } ]
            },
            "max-sdu": [ 0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0 ],
            "fp": [ "E","E","E","E","E","E","E","E","E","E","E","E","E","E","E","E" ]
        }
    } ]

which is invalid json, because the second group of "base_time",
"cycle_time", etc etc is placed in an unlabeled sub-object. If we pipe
it into jq, it complains:

parse error: Objects must consist of key:value pairs at line 53, column 14

Since it represents the administrative schedule, give this unnamed JSON
object the "admin" name. We now print valid JSON which looks like this:

[ {
        "kind": "taprio",
        "handle": "8001:",
        "root": true,
        "refcnt": 9,
        "options": {
            "tc": 8,
            "map": [ 0,1,2,3,4,5,6,7,0,0,0,0,0,0,0,0 ],
            "queues": [ {
                    "offset": 0,
                    "count": 1
                },{
                    "offset": 1,
                    "count": 1
                },{
                    "offset": 2,
                    "count": 1
                },{
                    "offset": 3,
                    "count": 1
                },{
                    "offset": 4,
                    "count": 1
                },{
                    "offset": 5,
                    "count": 1
                },{
                    "offset": 6,
                    "count": 1
                },{
                    "offset": 7,
                    "count": 1
                } ],
            "clockid": "TAI",
            "base_time": 0,
            "cycle_time": 20000000,
            "cycle_time_extension": 0,
            "schedule": [ {
                    "index": 0,
                    "cmd": "S",
                    "gatemask": "0xff",
                    "interval": 20000000
                } ],
            "admin": {
                "base_time": 1691160511783528178,
                "cycle_time": 20000000,
                "cycle_time_extension": 0,
                "schedule": [ {
                        "index": 0,
                        "cmd": "S",
                        "gatemask": "0xff",
                        "interval": 20000000
                    } ]
            },
            "max-sdu": [ 0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0 ],
            "fp": [ "E","E","E","E","E","E","E","E","E","E","E","E","E","E","E","E" ]
        }
    } ]

Fixes: 602fae856d80 ("taprio: Add support for changing schedules")
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 tc/q_taprio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tc/q_taprio.c b/tc/q_taprio.c
index 795c013c1c2a..16a4992d70d2 100644
--- a/tc/q_taprio.c
+++ b/tc/q_taprio.c
@@ -650,7 +650,7 @@ static int taprio_print_opt(struct qdisc_util *qu, FILE *f, struct rtattr *opt)
 		parse_rtattr_nested(t, TCA_TAPRIO_ATTR_MAX,
 				    tb[TCA_TAPRIO_ATTR_ADMIN_SCHED]);
 
-		open_json_object(NULL);
+		open_json_object("admin");
 
 		print_schedule(f, t);
 
-- 
2.34.1

Re: [PATCH iproute2 1/2] tc/taprio: don't print netlink attributes which weren't reported by the kernel

[Vinicius Costa Gomes]

Vladimir Oltean <vladimir.oltean@nxp.com> writes:

> When an admin schedule is pending and hasn't yet become operational, the
> kernel will report only the parameters of the admin schedule in a nested
> TCA_TAPRIO_ATTR_ADMIN_SCHED attribute.
>
> However, we default to printing zeroes even for the parameters of the
> operational base time, when that doesn't exist.
>
> Link: https://lore.kernel.org/netdev/87il9w0xx7.fsf@intel.com/
> Fixes: 0dd16449356f ("tc: Add support for configuring the taprio scheduler")
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
>  tc/q_taprio.c | 91 ++++++++++++++++++++++++++++++---------------------
>  1 file changed, 53 insertions(+), 38 deletions(-)
>
> diff --git a/tc/q_taprio.c b/tc/q_taprio.c
> index 913197f68caa..795c013c1c2a 100644
> --- a/tc/q_taprio.c
> +++ b/tc/q_taprio.c
> @@ -416,14 +416,11 @@ static int taprio_parse_opt(struct qdisc_util *qu, int argc,
>  	return 0;
>  }
>  
> -static int print_sched_list(FILE *f, struct rtattr *list)
> +static void print_sched_list(FILE *f, struct rtattr *list)
>  {
> -	struct rtattr *item;
> +	struct rtattr *item, *nla;
>  	int rem;
>  
> -	if (list == NULL)
> -		return 0;
> -
>  	rem = RTA_PAYLOAD(list);
>  
>  	open_json_array(PRINT_JSON, "schedule");
> @@ -432,60 +429,78 @@ static int print_sched_list(FILE *f, struct rtattr *list)
>  
>  	for (item = RTA_DATA(list); RTA_OK(item, rem); item = RTA_NEXT(item, rem)) {
>  		struct rtattr *tb[TCA_TAPRIO_SCHED_ENTRY_MAX + 1];
> -		__u32 index = 0, gatemask = 0, interval = 0;
> -		__u8 command = 0;
> +		__u32 index, gatemask, interval;
> +		__u8 command;

nitpick, optional: as you are already opening blocks for each of the
fields, you could move these declarations there. (same comment for the
next patch)

For the series:

Acked-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>


Cheers,
-- 
Vinicius

Re: [PATCH iproute2 1/2] tc/taprio: don't print netlink attributes which weren't reported by the kernel

[Vladimir Oltean]

On Mon, Aug 07, 2023 at 01:15:45PM -0700, Vinicius Costa Gomes wrote:
> nitpick, optional: as you are already opening blocks for each of the
> fields, you could move these declarations there. (same comment for the
> next patch)
> 
> For the series:
> 
> Acked-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>

Thanks for the review. I will submit a v2 with your comment addressed
(all variables defined within the smallest block in which they are used).
But as for "same comment for the next patch": the next patch is a single
line change which keeps the control flow the same.