[PATCH v10 1/5] PID namespace translation support

[Dmitry V. Levin]

On Wed, Aug 19, 2020 at 08:16:33PM +0200, Ákos Uzonyi wrote:
[...]
> +		errno = 0;
> +		long id = strtol(p, NULL, 10);
> +
> +		if (errno || id < 1 || id > INT_MAX) {
> +			perror_func_msg("converting pid (%ld) to int", id);
> +			break;
> +		}

I generally prefer to check local variables ("id" in this case)
prior to global ("errno") in this case.

[...]
> +		errno = 0;
> +		long proc_pid = strtol(entry->d_name, NULL, 10);
> +		if (errno)
> +			continue;
> +		if (proc_pid < 1 || proc_pid > INT_MAX)
> +			continue;

Likewise.  By the way, could you merge checks for proc_pid and errno
into a single condition, please?

> +/**
> + * Iterator function of the proc_data_cache for id translation.
> + * If the cache contains the id we are looking for, reads the corresponding
> + * directory in /proc, and if cache is valid, saves the result.
> + */
> +static void
> +proc_data_cache_iterator_fn(void* fn_data, uint64_t key, uint64_t val)
> +{
> +	struct translate_id_params *tip = (struct translate_id_params *)fn_data;
> +	struct proc_data *pd = (struct proc_data *) (uintptr_t) val;
> +
> +	if (!pd)
> +		return;
> +
> +	/* Result already found in an earlier iteration */
> +	if (tip->result_id)
> +		return;
> +
> +	/* Translate from cache */
> +	tip->pd = pd;
> +	translate_id_proc_pid(tip, 0);
> +	if (!tip->result_id)
> +		return;
> +
> +	/* Now translate from actual data in /proc, to check cache validity */
> +	translate_id_proc_pid(tip, pd->proc_pid);
> +}

According to the coverage report [1], the last translate_id_proc_pid call
is not covered by tests as if tip->result_id was always equal to 0.
Do you have any ideas why?

[...]
> +	/* Iterate through the cache, find potential proc_data */
> +	trie_iterate_keys(proc_data_cache, 0, pid_max,
> +		proc_data_cache_iterator_fn, &tip);
> +	/* (proc_data_cache_iterator_fn takes care about updating proc_data) */
> +	if (tip.result_id)
> +		goto exit;
> +
> +	/* No cache helped, read all entries in /proc */
> +	translate_id_dir(&tip, "/proc", true);
> +
> +exit:

Likewise, according to [1], this "goto exit" is somehow not covered by tests
as if translate_id_dir was always called.

[1] https://codecov.io/gh/strace/strace/src/a5f58ab7ded18c14dd055e38a0b4899c948bdb59/pidns.c


-- 
ldv