Add fxstat check to glibc_syscall_wrappers test.

Add fxstat check to glibc_syscall_wrappers test.

BUG=
TEST=run_fstat_test

Committed: http://src.chromium.org/viewvc/native_client?view=rev&revision=4164

[pasko-google - do not use, 2010-12-21 12:00:41]

http://codereview.chromium.org/6018003/diff/1/tests/glibc_syscall_wrappers/te...
File tests/glibc_syscall_wrappers/test_stat.c (right):

http://codereview.chromium.org/6018003/diff/1/tests/glibc_syscall_wrappers/te...
tests/glibc_syscall_wrappers/test_stat.c:20: int fileDesc;
s/fileDesc/fd/

http://codereview.chromium.org/6018003/diff/1/tests/glibc_syscall_wrappers/te...
tests/glibc_syscall_wrappers/test_stat.c:47: file = fopen(argv[1], "r");
please, make a separate test_fstat.c

http://codereview.chromium.org/6018003/diff/1/tests/glibc_syscall_wrappers/te...
tests/glibc_syscall_wrappers/test_stat.c:51: assert (KNOWN_FILE_SIZE ==
st.st_size);
also check for errno

[halyavin, 2010-12-21 12:46:55]

On 2010/12/21 12:00:41, pasko wrote:
>
http://codereview.chromium.org/6018003/diff/1/tests/glibc_syscall_wrappers/te...
> File tests/glibc_syscall_wrappers/test_stat.c (right):
> 
>
http://codereview.chromium.org/6018003/diff/1/tests/glibc_syscall_wrappers/te...
> tests/glibc_syscall_wrappers/test_stat.c:20: int fileDesc;
> s/fileDesc/fd/
> 
>
http://codereview.chromium.org/6018003/diff/1/tests/glibc_syscall_wrappers/te...
> tests/glibc_syscall_wrappers/test_stat.c:47: file = fopen(argv[1], "r");
> please, make a separate test_fstat.c
> 
>
http://codereview.chromium.org/6018003/diff/1/tests/glibc_syscall_wrappers/te...
> tests/glibc_syscall_wrappers/test_stat.c:51: assert (KNOWN_FILE_SIZE ==
> st.st_size);
> also check for errno

Done

Andrey Khalyavin

[Mark Seaborn, 2011-01-14 16:19:22]

Please add filled-out BUG= and TEST= fields.

Your test_fstat.c file seems to be missing.

Mark

[halyavin, 2011-01-18 13:36:12]

On 2011/01/14 16:19:22, Mark Seaborn wrote:
> Please add filled-out BUG= and TEST= fields.
> 
> Your test_fstat.c file seems to be missing.
> 
> Mark
Fixed.

Andrey Khalyavin

[khim, 2011-01-18 16:05:41]

http://codereview.chromium.org/6018003/diff/19001/tests/glibc_syscall_wrapper...
File tests/glibc_syscall_wrappers/test_fstat.c (right):

http://codereview.chromium.org/6018003/diff/19001/tests/glibc_syscall_wrapper...
tests/glibc_syscall_wrappers/test_fstat.c:13: #pragma GCC diagnostic ignored
"-Wnonnull"
Why do you need this option?

http://codereview.chromium.org/6018003/diff/19001/tests/glibc_syscall_wrapper...
tests/glibc_syscall_wrappers/test_fstat.c:15: #define KNOWN_FILE_SIZE 30
This is kind of strange: some information goes to test_fstat.stdout while the
filesize is hardcoded in source. Why not print it out and add to
test_fstat.stdout ?

[halyavin, 2011-01-18 16:20:10]

On 2011/01/18 16:05:41, khim wrote:
>
http://codereview.chromium.org/6018003/diff/19001/tests/glibc_syscall_wrapper...
> File tests/glibc_syscall_wrappers/test_fstat.c (right):
> 
>
http://codereview.chromium.org/6018003/diff/19001/tests/glibc_syscall_wrapper...
> tests/glibc_syscall_wrappers/test_fstat.c:13: #pragma GCC diagnostic ignored
> "-Wnonnull"
> Why do you need this option?
I removed the option
> 
>
http://codereview.chromium.org/6018003/diff/19001/tests/glibc_syscall_wrapper...
> tests/glibc_syscall_wrappers/test_fstat.c:15: #define KNOWN_FILE_SIZE 30
> This is kind of strange: some information goes to test_fstat.stdout while the
> filesize is hardcoded in source. Why not print it out and add to
> test_fstat.stdout ?
Good idea, done.

Andrey Khalyavin

[eaeltsin, 2011-01-19 12:54:28]

http://codereview.chromium.org/6018003/diff/24001/tests/glibc_syscall_wrapper...
File tests/glibc_syscall_wrappers/test_fstat.c (right):

http://codereview.chromium.org/6018003/diff/24001/tests/glibc_syscall_wrapper...
tests/glibc_syscall_wrappers/test_fstat.c:23: "%d+%d %d+%d %d+%d %d+%d %d+%d
%d+%d %d+%d %d+%d %d+%d %d+%d %d+%d %d\n",
Probably make the output more verbose? Like

  st_dev offset: %d, size: %d
  st_ino offset: %d, size: %d
  ...

Probably print struct stat sizeof as well?

http://codereview.chromium.org/6018003/diff/24001/tests/glibc_syscall_wrapper...
tests/glibc_syscall_wrappers/test_fstat.c:40: assert (0 ==  fstat(fd, &st));
I doubt having meaningful side effects inside assert is a good practice. There
is nothing that prevents compiling this with NDEBUG set to true (yes, I see that
other tests use this practice, this is not an excuse).

Why not
  if (!fstat(fd, &st))
    abort();

Or even better
  if (!fstat(fd, &st))
    return -1;  /* Use different negative return values for different failures
*/

[halyavin, 2011-01-19 14:19:25]

http://codereview.chromium.org/6018003/diff/24001/tests/glibc_syscall_wrapper...
File tests/glibc_syscall_wrappers/test_fstat.c (right):

http://codereview.chromium.org/6018003/diff/24001/tests/glibc_syscall_wrapper...
tests/glibc_syscall_wrappers/test_fstat.c:23: "%d+%d %d+%d %d+%d %d+%d %d+%d
%d+%d %d+%d %d+%d %d+%d %d+%d %d+%d %d\n",
On 2011/01/19 12:54:28, Evgeny Eltsin wrote:
> Probably make the output more verbose? Like
> 
>   st_dev offset: %d, size: %d
>   st_ino offset: %d, size: %d
>   ...
> 
> Probably print struct stat sizeof as well?

Done. I already print size of stat structure.

http://codereview.chromium.org/6018003/diff/24001/tests/glibc_syscall_wrapper...
tests/glibc_syscall_wrappers/test_fstat.c:40: assert (0 ==  fstat(fd, &st));
On 2011/01/19 12:54:28, Evgeny Eltsin wrote:
> I doubt having meaningful side effects inside assert is a good practice. There
> is nothing that prevents compiling this with NDEBUG set to true (yes, I see
that
> other tests use this practice, this is not an excuse).
> 
> Why not
>   if (!fstat(fd, &st))
>     abort();
> 
> Or even better
>   if (!fstat(fd, &st))
>     return -1;  /* Use different negative return values for different failures
> */
Done. I use positive return values though.

[khimg, 2011-01-19 14:39:15]

LGTM