Page MenuHomePhabricator

Fix eina file thread test on Windows
ClosedPublic

Authored by walac on Jun 23 2020, 6:55 AM.

Details

Summary

On windows, we try to open the "cmd.exe" file, but without the full path
the test fails unless it runs from the system directory.

We now use the full path to test the eina_file_open function.

Test Plan

meson test eina

Diff Detail

Repository
rEFL core/efl
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
walac created this revision.Jun 23 2020, 6:55 AM
walac requested review of this revision.Jun 23 2020, 6:55 AM
vtorri requested changes to this revision.Jun 23 2020, 7:24 AM
vtorri added inline comments.
src/tests/eina/eina_test_file.c
499

isn't len-MAX_PATH-1 negative ?

instead of strncat, i would do a test before, and append with 2 strcat

something like :

if (len + 1 /* '\' */+ 7 /* "cmd.exe" */ + 1 /* nul */ < MAX_PATH)

we fail

then the 2 strcat.

That way, we are sure than the path is really the one of cmd.exe. Otherwise, it can be truncated (though the probability to have it truncated is almost 0)

This revision now requires changes to proceed.Jun 23 2020, 7:24 AM
walac updated this revision to Diff 30758.Jun 23 2020, 7:35 AM

Fix negative path length calculation

walac updated this revision to Diff 30759.Jun 23 2020, 7:36 AM

Fix patch update

walac added inline comments.Jun 23 2020, 7:37 AM
src/tests/eina/eina_test_file.c
499

I have fixed the path calculation.

As discussed in IRC, I avoided the use of strcat because it triggers a warning on modern compilers.

vtorri added inline comments.Jun 23 2020, 7:39 AM
src/tests/eina/eina_test_file.c
499

missing - :-)

walac updated this revision to Diff 30760.Jun 23 2020, 7:40 AM

Fix typo

last thing for me : we declare variables always at the beginning of the block (exception in eo/eolian)
otherwise, good for me (tested)

jptiz added a comment.Jun 23 2020, 7:59 AM

last thing for me : we declare variables always at the beginning of the block (exception in eo/eolian)
otherwise, good for me (tested)

Isn't this just because of older compilers/versions of C (i.e. <c99)? Or ist it an EFL coding convention?

more a coding convention

walac updated this revision to Diff 30761.Jun 23 2020, 8:16 AM

Update the patch based on reviewer feedback

vtorri accepted this revision.Jun 23 2020, 8:18 AM
This revision is now accepted and ready to land.Jun 23 2020, 8:18 AM
jptiz accepted this revision.Jun 23 2020, 8:54 AM

more a coding convention

I see.

Well then...looks good.

stefan_schmidt added inline comments.
src/tests/eina/eina_test_file.c
506

Given the comment above I would have expected something like this:
MAX_PATH < len + 1 + strlen(test_file) + 1

518

I don't think we need a comment saying why we use strncat here.

vtorri added inline comments.Jun 24 2020, 2:49 AM
src/tests/eina/eina_test_file.c
495

const char * test_file = "cmd.exe";

and see below

506

and

sizeof(test_file)

instead of

sizeof(test_file) / sizeof(test_file[0])

because as static string, sizeof(test_file) is strlen(test_file) + 1

walac added inline comments.Jun 24 2020, 6:16 AM
src/tests/eina/eina_test_file.c
495

sizeof(test_file) != sizeof("cmd.exe") if test_file is a pointer. In this case, there is a coincidence that the string is 8 bytes long, exactly the same size as a pointer in 64 bit systems. That's the reasoning behind the use of a matrix. As an alternative, I could make it a macro.

vtorri added inline comments.Jun 24 2020, 6:26 AM
src/tests/eina/eina_test_file.c
495

you're right, sorry, needs to be an array

vtorri added inline comments.Jun 24 2020, 6:28 AM
src/tests/eina/eina_test_file.c
495

it's sizeof("foooooooooo"); which returns strlen("foooooooooo") + 1

walac updated this revision to Diff 30767.Jun 24 2020, 6:32 AM

Address review comments

walac marked 3 inline comments as done.Jun 24 2020, 6:33 AM

@stefan_schmidt is there something else to do ?

stefan_schmidt accepted this revision.Jun 26 2020, 6:35 AM
This revision was automatically updated to reflect the committed changes.