Re: [iproute2 v3] Read configuration files from /etc and /usr

[Stephen Hemminger <stephen@networkplumber.org>]

On Tue, 25 Jul 2023 16:27:59 +0200
Gioele Barabucci <gioele@svario.it> wrote:

> @@ -2924,7 +2926,9 @@ static int bpf_elf_ctx_init(struct bpf_elf_ctx *ctx, const char *pathname,
>  	}
>  
>  	bpf_save_finfo(ctx);
> -	bpf_hash_init(ctx, CONFDIR "/bpf_pinning");
> +	ret = bpf_hash_init(ctx, CONF_USR_DIR "/bpf_pinning");
> +	if (ret == -ENOENT)
> +		bpf_hash_init(ctx, CONF_ETC_DIR "/bpf_pinning");
>  

Luca spotted this one, other places prefer /etc over /use but in BPF it is swapped?

> -static void
> +static int
>  rtnl_hash_initialize(const char *file, struct rtnl_hash_entry **hash, int size)
>  {
>  	struct rtnl_hash_entry *entry;
> @@ -67,14 +69,14 @@ rtnl_hash_initialize(const char *file, struct rtnl_hash_entry **hash, int size)
>  
>  	fp = fopen(file, "r");
>  	if (!fp)
> -		return;
> +		return -errno;
>  
>  	while ((ret = fread_id_name(fp, &id, &namebuf[0]))) {
>  		if (ret == -1) {
>  			fprintf(stderr, "Database %s is corrupted at %s\n",
>  					file, namebuf);
>  			fclose(fp);
> -			return;
> +			return -1;

Having some errors return -errno and others return -1 is confusing.
Perhaps -EINVAL?

> +static void
> +rtnl_tabhash_initialize_dir(const char *ddir, void *tabhash, const int size,
> +                            const bool is_tab)
> +{
> +	char dirpath_usr[PATH_MAX], dirpath_etc[PATH_MAX];
> +	struct dirent *de;
> +	DIR *d;
> +
> +	snprintf(dirpath_usr, sizeof(dirpath_usr), "%s/%s", CONF_USR_DIR, ddir);
> +	snprintf(dirpath_etc, sizeof(dirpath_etc), "%s/%s", CONF_ETC_DIR, ddir);
> +
> +	/* load /usr/lib/iproute2/foo.d/X conf files, unless /etc/iproute2/foo.d/X exists */
> +	d = opendir(dirpath_usr);
> +	while (d && (de = readdir(d)) != NULL) {
> +		char path[PATH_MAX];
> +		size_t len;
> +		struct stat sb;
> +
> +		if (*de->d_name == '.')
> +			continue;
> +
> +		/* only consider filenames ending in '.conf' */
> +		len = strlen(de->d_name);
> +		if (len <= 5)
> +			continue;
> +		if (strcmp(de->d_name + len - 5, ".conf"))
> +			continue;
> +
> +		/* only consider filenames not present in /etc */
> +		snprintf(path, sizeof(path), "%s/%s", dirpath_etc, de->d_name);
> +		if (lstat(path, &sb) == 0)
> +			continue;

Do you want lstat() it will return 0 it is a symlink to non existent target.

> +		/* load the conf file in /etc */
> +		snprintf(path, sizeof(path), "%s/%s", dirpath_etc, de->d_name);
> +		if (is_tab)
> +			rtnl_tab_initialize(path, (char**)tabhash, size);
> +		else
> +			rtnl_hash_initialize(path, (struct rtnl_hash_entry**)tabhash, size);

I would prefer that generic function not loose all type information.
Maybe using a union for the two possible usages? Or type specific callback instead of is_tab.