[LTP] [PATCH 1/2] open12: Convert to new API

[Martin Doucha]

Hi,
I'll send v2 shortly, some notes below.

On 03. 06. 25 10:55, Andrea Cervesato wrote:
> Hi!
> 
> On 6/2/25 17:57, Martin Doucha wrote:
>>   static void test_append(void)
>>   {
>>       off_t len1, len2;
>> -    TEST(open(TEST_FILE, O_RDWR | O_APPEND, 0777));
>> +    tst_res(TINFO, "Testing O_APPEND");
>> +
>> +    fd = TST_EXP_FD_SILENT(open(TEST_FILE, O_RDWR | O_APPEND, 0644));
> Is there a reason why not using SAFE_OPEN() ? It doesn't seem to affect 
> the code.
> Also, if open() syscall fails we are not printing anything but the TINFO 
> message and it makes sense to TBROK if there's a problem with opening 
> the test file (LTP or system bug).

TST_*_SILENT() macros are silent only on success. They will complain 
loudly on failure. The reason to avoid SAFE_OPEN() is that it'll 
terminate the test on failure. I prefer to finish the other test cases 
instead.

>> -    if (TEST_RETURN == -1) {
>> -        tst_resm(TFAIL | TTERRNO, "open failed");
>> +    if (!TST_PASS)
>>           return;
>> -    }
>> -    len1 = SAFE_LSEEK(cleanup, TEST_RETURN, 0, SEEK_CUR);
>> -    SAFE_WRITE(cleanup, SAFE_WRITE_ALL, TEST_RETURN, TEST_FILE,
>> -        sizeof(TEST_FILE));
>> -    len2 = SAFE_LSEEK(cleanup, TEST_RETURN, 0, SEEK_CUR);
>> -    SAFE_CLOSE(cleanup, TEST_RETURN);
>> +    len1 = SAFE_LSEEK(fd, 0, SEEK_CUR);
>> +    SAFE_WRITE(1, fd, TEST_FILE, strlen(TEST_FILE));
>> +    len2 = SAFE_LSEEK(fd, 0, SEEK_CUR);
>> +    SAFE_CLOSE(fd);
>>       if (len2 > len1)
>> -        tst_resm(TPASS, "test O_APPEND for open success");
>> +        tst_res(TPASS, "O_APPEND works as expected");
>>       else
>> -        tst_resm(TFAIL, "test O_APPEND for open failed");
>> +        tst_res(TFAIL, "O_APPEND did not move write cursor");
> Maybe we can use TST_EXP_EXPR here

This check is pretty much useless and the next patch rewrites it from 
scratch. Not worth the effort.

>>   }
>>   static void test_noatime(void)
>> @@ -136,31 +69,23 @@ static void test_noatime(void)
>>       char read_buf;
>>       struct stat old_stat, new_stat;
>> -    if (skip_noatime) {
>> -        tst_resm(TCONF,
>> -                 "test O_NOATIME flag for open needs filesystems which "
>> -                 "is mounted without noatime and relatime");
>> -        return;
>> -    }
>> -
>> -    SAFE_STAT(cleanup, TEST_FILE, &old_stat);
>> +    tst_res(TINFO, "Testing O_NOATIME");
>> +    SAFE_STAT(TEST_FILE, &old_stat);
>>       sleep(1);
>> +    fd = TST_EXP_FD_SILENT(open(TEST_FILE, O_RDONLY | O_NOATIME, 0644));
>> -    TEST(open(TEST_FILE, O_RDONLY | O_NOATIME, 0777));
>> -
>> -    if (TEST_RETURN == -1) {
>> -        tst_resm(TFAIL | TTERRNO, "open failed");
>> +    if (!TST_PASS)
>>           return;
>> -    }
>> -    SAFE_READ(cleanup, 1, TEST_RETURN, &read_buf, 1);
>> -    SAFE_CLOSE(cleanup, TEST_RETURN);
>> -    SAFE_STAT(cleanup, TEST_FILE, &new_stat);
>> +
>> +    SAFE_READ(1, fd, &read_buf, 1);
>> +    SAFE_CLOSE(fd);
>> +    SAFE_STAT(TEST_FILE, &new_stat);
>>       if (old_stat.st_atime == new_stat.st_atime)
>> -        tst_resm(TPASS, "test O_NOATIME for open success");
>> +        tst_res(TPASS, "File access time was not modified");
>>       else
>> -        tst_resm(TFAIL, "test O_NOATIME for open failed");
>> +        tst_res(TFAIL, "File access time changed");
> TST_EXP_EQ_LI() would also print the values

Do we care about random timestamp values?

>>   }
>>   static void test_cloexec(void)
>> @@ -169,80 +94,88 @@ static void test_cloexec(void)
>>       int status;
>>       char buf[20];
>> -    TEST(open(TEST_FILE, O_RDWR | O_APPEND | O_CLOEXEC, 0777));
>> -
>> -    if (TEST_RETURN == -1) {
>> -        tst_resm(TFAIL | TTERRNO, "open failed");
>> -        return;
>> -    }
>> -
>> -    sprintf(buf, "%ld", TEST_RETURN);
>> +    tst_res(TINFO, "Testing O_CLOEXEC");
>> -    pid = tst_fork();
>> -    if (pid < 0)
>> -        tst_brkm(TBROK | TERRNO, cleanup, "fork() failed");
>> +    fd = TST_EXP_FD_SILENT(open(TEST_FILE, O_RDWR | O_APPEND | 
>> O_CLOEXEC,
>> +        0644));
> Just in case of TST_EXP_FD_SILENT usage around the tests, TST_PASS is 
> not checked here.

Good catch, will fix in v2.

>> +    sprintf(buf, "%d", fd);
> snprintf() probably would be a better choice, even if buffer is big 
> enough (64bits would be converted into 21 chars, so 20 chars are enough 
> for 32-bits integer)

Also fixed in v2 along with the switch and lseek() suggestions.

-- 
Martin Doucha   mdoucha@suse.cz
SW Quality Engineer
SUSE LINUX, s.r.o.
CORSO IIa
Krizikova 148/34
186 00 Prague 8
Czech Republic