Re: [PATCH v2 bpf-next 2/2] selftests: bpf: Add a new test for bare tracepoints

[Qais Yousef <qais.yousef@arm.com>]

Hi Yonghong

On 01/18/21 09:48, Yonghong Song wrote:
> The original patch code:
> 
> +static int trigger_module_test_write(int write_sz)
> +{
> +	int fd, err;
> +	char *buf = malloc(write_sz);
> +
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	memset(buf, 'a', write_sz);
> +	buf[write_sz-1] = '\0';
> +
> +	fd = open("/sys/kernel/bpf_testmod", O_WRONLY);
> +	err = -errno;
> +	if (CHECK(fd < 0, "testmod_file_open", "failed: %d\n", err))
> +		goto out;
> +
> +	write(fd, buf, write_sz);
> +	close(fd);
> +out:
> +	free(buf);
> +
> +	return 0;
> +}
> 
> Even for "fd < 0" case, it "goto out" and "return 0". We should return
> error code here instead of 0.
> 
> Second, "err = -errno" is set before checking fd < 0. If fd >= 0, err might
> inherit an postive errno from previous failure.
> In trigger_module_test_write(), it is okay since the err is only used
> when fd < 0:
>         err = -errno;
>         if (CHECK(fd < 0, "testmod_file_open", "failed: %d\n", err))
>                 return err;
> 
> My above rewrite intends to use "err" during final "return" statement,
> so I put assignment of "err = -errno" inside the CHECK branch.
> But there are different ways to implement this properly.

Okay I see now. Sorry I missed your point initially. I will fix and send v3.

Thanks

--
Qais Yousef