Re: [PATCH v3 iproute2 2/3] tc: Add support for the ETF Qdisc

[Jesus Sanchez-Palencia <jesus.sanchez-palencia@intel.com>]

Hi David,


On 07/06/2018 08:58 AM, David Ahern wrote:
> On 7/5/18 4:42 PM, Jesus Sanchez-Palencia wrote:
> 
>> +static int get_clockid(__s32 *val, const char *arg)
>> +{
>> +	const struct static_clockid {
>> +		const char *name;
>> +		clockid_t clockid;
>> +	} clockids_sysv[] = {
>> +		{ "CLOCK_REALTIME", CLOCK_REALTIME },
>> +		{ "CLOCK_TAI", CLOCK_TAI },
>> +		{ "CLOCK_BOOTTIME", CLOCK_BOOTTIME },
>> +		{ "CLOCK_MONOTONIC", CLOCK_MONOTONIC },
>> +		{ NULL }
>> +	};
>> +
>> +	const struct static_clockid *c;
>> +
>> +	for (c = clockids_sysv; c->name; c++) {
>> +		if (strncasecmp(c->name, arg, 25) == 0) {
> 
> Why 25?


That was just an upper bound giving some room beyond the longest
clockid name we have today. Should I add a define MAX_CLOCK_NAME ?


> 
> be nice to allow shortcuts -- e.g., just REALTIME or realtime.


I'd rather just keep it as is and use the names as they are defined for
everything else (i.e. CLOCK_REALTIME), unless there are some strong objections.



> 
>> +			*val = c->clockid;
>> +
>> +			return 0;
>> +		}
>> +	}
>> +
>> +	return -1;
>> +}
>> +
>> +
>> +static int etf_parse_opt(struct qdisc_util *qu, int argc,
>> +			 char **argv, struct nlmsghdr *n, const char *dev)
>> +{
>> +	struct tc_etf_qopt opt = {
>> +		.clockid = CLOCKID_INVALID,
>> +	};
>> +	struct rtattr *tail;
>> +
>> +	while (argc > 0) {
>> +		if (matches(*argv, "offload") == 0) {
>> +			if (opt.flags & TC_ETF_OFFLOAD_ON) {
>> +				fprintf(stderr, "etf: duplicate \"offload\" specification\n");
>> +				return -1;
>> +			}
>> +
>> +			opt.flags |= TC_ETF_OFFLOAD_ON;
>> +		} else if (matches(*argv, "deadline_mode") == 0) {
>> +			if (opt.flags & TC_ETF_DEADLINE_MODE_ON) {
>> +				fprintf(stderr, "etf: duplicate \"deadline_mode\" specification\n");
>> +				return -1;
>> +			}
>> +
>> +			opt.flags |= TC_ETF_DEADLINE_MODE_ON;
>> +		} else if (matches(*argv, "delta") == 0) {
>> +			NEXT_ARG();
>> +			if (opt.delta) {
>> +				fprintf(stderr, "etf: duplicate \"delta\" specification\n");
>> +				return -1;
>> +			}
>> +			if (get_s32(&opt.delta, *argv, 0)) {
>> +				explain1("delta", *argv);
>> +				return -1;
>> +			}
>> +		} else if (matches(*argv, "clockid") == 0) {
>> +			NEXT_ARG();
>> +			if (opt.clockid != CLOCKID_INVALID) {
>> +				fprintf(stderr, "etf: duplicate \"clockid\" specification\n");
>> +				return -1;
>> +			}
>> +			if (get_clockid(&opt.clockid, *argv)) {
>> +				explain_clockid(*argv);
>> +				return -1;
>> +			}
>> +		} else if (strcmp(*argv, "help") == 0) {
>> +			explain();
>> +			return -1;
>> +		} else {
>> +			fprintf(stderr, "etf: unknown parameter \"%s\"\n", *argv);
>> +			explain();
>> +			return -1;
>> +		}
>> +		argc--; argv++;
>> +	}
>> +
>> +	tail = NLMSG_TAIL(n);
>> +	addattr_l(n, 1024, TCA_OPTIONS, NULL, 0);
>> +	addattr_l(n, 2024, TCA_ETF_PARMS, &opt, sizeof(opt));
>> +	tail->rta_len = (void *) NLMSG_TAIL(n) - (void *) tail;
>> +	return 0;
>> +}
>> +
>> +static int etf_print_opt(struct qdisc_util *qu, FILE *f, struct rtattr *opt)
>> +{
>> +	struct rtattr *tb[TCA_ETF_MAX+1];
>> +	struct tc_etf_qopt *qopt;
>> +
>> +	if (opt == NULL)
>> +		return 0;
>> +
>> +	parse_rtattr_nested(tb, TCA_ETF_MAX, opt);
>> +
>> +	if (tb[TCA_ETF_PARMS] == NULL)
>> +		return -1;
>> +
>> +	qopt = RTA_DATA(tb[TCA_ETF_PARMS]);
>> +	if (RTA_PAYLOAD(tb[TCA_ETF_PARMS])  < sizeof(*qopt))
>> +		return -1;
>> +
>> +	if (qopt->clockid == CLOCKID_INVALID)
>> +		print_string(PRINT_ANY, "clockid", "clockid %s ", "invalid");
>> +	else
>> +		print_uint(PRINT_ANY, "clockid", "clockid %d ", qopt->clockid);
> 
> If you allow the user to input a string, then you should return it here too.


Ok, will fix.

Thanks,
Jesus