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
