Re: [PATCH 2/3] selftests/bpf: convert test_dev_cgroup to test_progs

[Alexis Lothoré]

Hello Stanislas, thanks for the review

On 7/27/24 00:48, Stanislav Fomichev wrote:
> On 07/25, Alexis Lothoré (eBPF Foundation) wrote:

[...]

>> +	if (should_fail)
>> +		ASSERT_ERR(ret, "mknod");
>> +	else
>> +		ASSERT_OK(ret, "mknod");
> 
> Optional: might be easier to use something like expected_ret instead
> of should_fail and then do:
> 
> ASSERT_EQ(ret, expected_ret)

Yes, you are right. I initially went with a version relying on system() to
perform the mknods/dd calls, which could return different errors codes so I used
this should_fail. But while debugging some issues in CI with this series, I
realized that the needed commands are basic enough to be replaced with direct
library calls and I forgot to update this part, which can now assert an exact
return value. I will update this accordingly.

> I see this part being copy-pasted in a bunch of places below.
> 
>> +	unlink(path);
>> +}
>> +
>> +static void test_read(const char *path, int should_fail)
>> +{
>> +	char buf[TEST_BUFFER_SIZE];
>> +	int ret, fd;
>> +
>> +	fd = open(path, O_RDONLY);
>> +
>> +	/* A bare open on unauthorized device should fail */
>> +	if (should_fail) {
>> +		ASSERT_ERR(fd, "open file for read");
> 
> [..]
> 
>> +		if (fd)
>> +			close(fd);
> 
> nit: should this be 'if (fd >= 0)'? I'm assuming the intention is to
> avoid close(-1)?

Right as well, I'll fix it (here and below) in v2

Thanks,

Alexis

-- 
Alexis Lothoré, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com