From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Mon, 01 Aug 2022 11:33:36 GMT
Subject: Re: [PATCH v3 2/3] t0021: implementation the rot13-filter.pl script in C
Message-ID: <220801.86les8i495.gmgdl@evledraar.gmail.com>
In-Reply-To: <86e6baba460f4d0fce353d1fb6a0e18b57ecadaa.1659291025.git.matheus.bernardino@usp.br>


On Sun, Jul 31 2022, Matheus Tavares wrote:


> +static char *rot13_usage = "test-tool rot13-filter [--always-delay] <log path> <capabilities>";
> +
> +int cmd__rot13_filter(int argc, const char **argv)
> +{
> +	const char **caps;
> +	int cap_count, i = 1;
> +	struct strset remote_caps = STRSET_INIT;
> +
> +	if (argc > 1 && !strcmp(argv[1], "--always-delay")) {
> +		always_delay = 1;
> +		i++;
> +	}
> +	if (argc - i < 2)
> +		usage(rot13_usage);
> +
> +	logfile = fopen(argv[i++], "a");
> +	if (!logfile)
> +		die_errno("failed to open log file");
> +
> +	caps = argv + i;
> +	cap_count = argc - i;

Since you need to change every single caller consider just starting out
with parse_options() here instead of rolling your own parsing. You could
use it for --always-delay in any case, but you could also just add a
--log-path and --capability (an OPT_STRING_LIST), so:

	test-tool rot13-filter [--always-delay] --log-path=<path> [--capability <capbility]...

> +
> +	for (i = 0; i < cap_count; i++) {
> +		if (!strcmp(caps[i], "clean"))
> +			has_clean_cap = 1;
> +		else if (!strcmp(caps[i], "smudge"))
> +			has_smudge_cap = 1;

In any case, maybe BUG() in an "else" here with "unknown capability"?

> +	fclose(logfile);

Perhaps check the return value & die_errno() if we fail to fclose()
(happens e.g. if the disk fills up).