added a test to check glibc socket functions wrappers

added a test to check glibc socket functions wrappers
Committed: https://src.chromium.org/viewvc/native_client?view=rev&revision=8430

[vissi, 2012-04-16 11:15:30]

test: https://chromiumcodereview.appspot.com/10095010

[vissi, 2012-04-16 11:20:48]

https://chromiumcodereview.appspot.com/10096008/diff/1/tests/glibc_socket_wra...
File tests/glibc_socket_wrappers/test_sock.c (right):

https://chromiumcodereview.appspot.com/10096008/diff/1/tests/glibc_socket_wra...
tests/glibc_socket_wrappers/test_sock.c:2: * Copyright 2010 The Native Client
Authors. All rights reserved.
2012

[Evgeniy Stepanov, 2012-04-16 11:26:50]

https://chromiumcodereview.appspot.com/10096008/diff/1/tests/glibc_socket_wra...
File tests/glibc_socket_wrappers/test_sock.c (right):

https://chromiumcodereview.appspot.com/10096008/diff/1/tests/glibc_socket_wra...
tests/glibc_socket_wrappers/test_sock.c:19: #pragma GCC diagnostic ignored
"-Wnonnull"
Do you really need this?

https://chromiumcodereview.appspot.com/10096008/diff/1/tests/glibc_socket_wra...
tests/glibc_socket_wrappers/test_sock.c:31: ++test_var;
It might be easier to return a magic value or an error code, and then check the
function return value or errno?

https://chromiumcodereview.appspot.com/10096008/diff/1/tests/glibc_socket_wra...
tests/glibc_socket_wrappers/test_sock.c:35: int WRAP(accept) (int sockfd, struct
sockaddr* addr, socklen_t* addrlen) {
Please remove spaces before argument list and fix lint warnings.

[pasko-google - do not use, 2012-04-16 11:32:26]

first quick pass

https://chromiumcodereview.appspot.com/10096008/diff/1/tests/glibc_socket_wra...
File tests/glibc_socket_wrappers/nacl_stat.h (right):

https://chromiumcodereview.appspot.com/10096008/diff/1/tests/glibc_socket_wra...
tests/glibc_socket_wrappers/nacl_stat.h:8: keeping a copy in the glibc tree. */
this means there is a copy of this header in the nacl repository. I.e. you
should not create another instance of it.

https://chromiumcodereview.appspot.com/10096008/diff/1/tests/glibc_socket_wra...
File tests/glibc_socket_wrappers/test_sock.c (right):

https://chromiumcodereview.appspot.com/10096008/diff/1/tests/glibc_socket_wra...
tests/glibc_socket_wrappers/test_sock.c:19: #pragma GCC diagnostic ignored
"-Wnonnull"
why so?

https://chromiumcodereview.appspot.com/10096008/diff/1/tests/glibc_socket_wra...
tests/glibc_socket_wrappers/test_sock.c:23: extern "C" {
there is no need for this in test

https://chromiumcodereview.appspot.com/10096008/diff/1/tests/glibc_socket_wra...
tests/glibc_socket_wrappers/test_sock.c:25: #define DO_WRAP(name) do { \
a better name would be REGISTER_WRAPPER_IN_GLIBC

[vissi, 2012-04-18 16:28:33]

Working test, but it still has a couple of lint warning.

https://chromiumcodereview.appspot.com/10096008/diff/1/tests/glibc_socket_wra...
File tests/glibc_socket_wrappers/nacl_stat.h (right):

https://chromiumcodereview.appspot.com/10096008/diff/1/tests/glibc_socket_wra...
tests/glibc_socket_wrappers/nacl_stat.h:8: keeping a copy in the glibc tree. */
On 2012/04/16 11:32:26, pasko wrote:
> this means there is a copy of this header in the nacl repository. I.e. you
> should not create another instance of it.
There's a copy of it in glibc. Copy of irt_syscalls.h is also there, but how can
I include it?

https://chromiumcodereview.appspot.com/10096008/diff/1/tests/glibc_socket_wra...
File tests/glibc_socket_wrappers/test_sock.c (right):

https://chromiumcodereview.appspot.com/10096008/diff/1/tests/glibc_socket_wra...
tests/glibc_socket_wrappers/test_sock.c:2: * Copyright 2010 The Native Client
Authors. All rights reserved.
On 2012/04/16 11:20:48, vissi wrote:
> 2012

Done.

https://chromiumcodereview.appspot.com/10096008/diff/1/tests/glibc_socket_wra...
tests/glibc_socket_wrappers/test_sock.c:19: #pragma GCC diagnostic ignored
"-Wnonnull"
On 2012/04/16 11:26:50, Evgeniy Stepanov wrote:
> Do you really need this?
no need, removed

https://chromiumcodereview.appspot.com/10096008/diff/1/tests/glibc_socket_wra...
tests/glibc_socket_wrappers/test_sock.c:23: extern "C" {
On 2012/04/16 11:32:26, pasko wrote:
> there is no need for this in test

Done.

https://chromiumcodereview.appspot.com/10096008/diff/1/tests/glibc_socket_wra...
tests/glibc_socket_wrappers/test_sock.c:25: #define DO_WRAP(name) do { \
On 2012/04/16 11:32:26, pasko wrote:
> a better name would be REGISTER_WRAPPER_IN_GLIBC

Done.

https://chromiumcodereview.appspot.com/10096008/diff/1/tests/glibc_socket_wra...
tests/glibc_socket_wrappers/test_sock.c:31: ++test_var;
On 2012/04/16 11:26:50, Evgeniy Stepanov wrote:
> It might be easier to return a magic value or an error code, and then check
the
> function return value or errno?

Done.

https://chromiumcodereview.appspot.com/10096008/diff/1/tests/glibc_socket_wra...
tests/glibc_socket_wrappers/test_sock.c:35: int WRAP(accept) (int sockfd, struct
sockaddr* addr, socklen_t* addrlen) {
On 2012/04/16 11:26:50, Evgeniy Stepanov wrote:
> Please remove spaces before argument list and fix lint warnings.
Done. Don't we use lint in glibc (irt_syscalls was taken from there)?

[vissi, 2012-04-18 16:53:08]

Per offline discussion: removed nacl_stat and syscalls that use it from
irt_syscalls.h to minimize test and its further modifications.

[vissi, 2012-04-18 17:07:13]

On 2012/04/18 16:53:08, vissi wrote:
> Per offline discussion: removed nacl_stat and syscalls that use it from
> irt_syscalls.h to minimize test and its further modifications.

Fixed headers, rm lint warnings.

[vissi, 2012-04-18 18:09:53]

On 2012/04/18 17:07:13, vissi wrote:
> On 2012/04/18 16:53:08, vissi wrote:
> > Per offline discussion: removed nacl_stat and syscalls that use it from
> > irt_syscalls.h to minimize test and its further modifications.
> 
> Fixed headers, rm lint warnings.

Just noticed that code review lint is stronger than my local.

[vissi, 2012-04-24 09:07:21]

On 2012/04/18 18:09:53, vissi wrote:
> On 2012/04/18 17:07:13, vissi wrote:
> > On 2012/04/18 16:53:08, vissi wrote:
> > > Per offline discussion: removed nacl_stat and syscalls that use it from
> > > irt_syscalls.h to minimize test and its further modifications.
> > 
> > Fixed headers, rm lint warnings.
> 
> Just noticed that code review lint is stronger than my local.

Fixed header reference, all lint warnings.

[pasko-google - do not use, 2012-04-24 09:24:15]

looking much nicer now. I have some small comments on style.

From a bird eye view looks good. I'll leave it to Victor and Evgeniy to check
whether we are testing enough here.

https://chromiumcodereview.appspot.com/10096008/diff/22001/tests/glibc_socket...
File tests/glibc_socket_wrappers/test_sock.c (right):

https://chromiumcodereview.appspot.com/10096008/diff/22001/tests/glibc_socket...
tests/glibc_socket_wrappers/test_sock.c:10: #include
<native_client/tests/glibc_socket_wrappers/irt_syscalls.h>
this should go in double quotes and separated from the system headers with one
empty line

https://chromiumcodereview.appspot.com/10096008/diff/22001/tests/glibc_socket...
tests/glibc_socket_wrappers/test_sock.c:42: return sockfd > 0 ? 0 : -53;
this is part of the test code, one of these cases is not covered in it, no clear
reason why.

Also, it might be helpful to provide comments on what we are testing.

https://chromiumcodereview.appspot.com/10096008/diff/22001/tests/glibc_socket...
tests/glibc_socket_wrappers/test_sock.c:53:
assert(reinterpret_cast<void*>(&s_in) == reinterpret_cast<void*>(addr));
reinterpret_cast in C?

[vissi, 2012-04-24 10:01:54]

fixed

https://chromiumcodereview.appspot.com/10096008/diff/22001/tests/glibc_socket...
File tests/glibc_socket_wrappers/test_sock.c (right):

https://chromiumcodereview.appspot.com/10096008/diff/22001/tests/glibc_socket...
tests/glibc_socket_wrappers/test_sock.c:10: #include
<native_client/tests/glibc_socket_wrappers/irt_syscalls.h>
On 2012/04/24 09:24:15, pasko wrote:
> this should go in double quotes and separated from the system headers with one
> empty line
in fact, it should take pre-installed irt_syscalls, as arranged, fixed now.

https://chromiumcodereview.appspot.com/10096008/diff/22001/tests/glibc_socket...
tests/glibc_socket_wrappers/test_sock.c:42: return sockfd > 0 ? 0 : -53;
On 2012/04/24 09:24:15, pasko wrote:
> this is part of the test code, one of these cases is not covered in it, no
clear
> reason why.
> 
> Also, it might be helpful to provide comments on what we are testing.

Now covered.

https://chromiumcodereview.appspot.com/10096008/diff/22001/tests/glibc_socket...
tests/glibc_socket_wrappers/test_sock.c:53:
assert(reinterpret_cast<void*>(&s_in) == reinterpret_cast<void*>(addr));
On 2012/04/24 09:24:15, pasko wrote:
> reinterpret_cast in C?

no, it was a lint warning, removed now.

[pasko-google - do not use, 2012-04-24 10:48:21]

lgtm as far as Victor is ok

btw, why are we always checking ret xor errno? Is it hard to test both?

[vissi, 2012-04-24 10:51:47]

On 2012/04/24 10:48:21, pasko wrote:
> lgtm as far as Victor is ok
> 
> btw, why are we always checking ret xor errno? Is it hard to test both?

not hard, I just supposed there's no need to test every effect possible for
similar calls.

[khim, 2012-04-24 15:15:38]

LGTM

errno has strange semantics: it's ONLY set if the function in question returns
-1.

If it returns zero or positive value then errno is NOT defined!

It's POSIX: http://pubs.opengroup.org/onlinepubs/009695399/functions/errno.html

[vissi, 2012-04-26 09:31:33]

On 2012/04/24 15:15:38, khim wrote:
> LGTM
> 
> errno has strange semantics: it's ONLY set if the function in question returns
> -1.
> 
> If it returns zero or positive value then errno is NOT defined!
> 
> It's POSIX:
http://pubs.opengroup.org/onlinepubs/009695399/functions/errno.html

chrome browser tests fail, not my fault afaik, waiting for open tree

[vissi, 2012-04-26 12:00:48]

On 2012/04/26 09:31:33, vissi wrote:
> On 2012/04/24 15:15:38, khim wrote:
> > LGTM
> > 
> > errno has strange semantics: it's ONLY set if the function in question
returns
> > -1.
> > 
> > If it returns zero or positive value then errno is NOT defined!
> > 
> > It's POSIX:
> http://pubs.opengroup.org/onlinepubs/009695399/functions/errno.html
> 
> chrome browser tests fail, not my fault afaik, waiting for open tree

So, it turned out that toolchain does not get installed correctly at win and
mac, trying a tiny patch. Thinking on lucid-32 problem.

[vissi, 2012-04-26 17:22:40]

On 2012/04/26 12:00:48, vissi wrote:
> On 2012/04/26 09:31:33, vissi wrote:
> > On 2012/04/24 15:15:38, khim wrote:
> > > LGTM
> > > 
> > > errno has strange semantics: it's ONLY set if the function in question
> returns
> > > -1.
> > > 
> > > If it returns zero or positive value then errno is NOT defined!
> > > 
> > > It's POSIX:
> > http://pubs.opengroup.org/onlinepubs/009695399/functions/errno.html
> > 
> > chrome browser tests fail, not my fault afaik, waiting for open tree
> 
> So, it turned out that toolchain does not get installed correctly at win and
> mac, trying a tiny patch. Thinking on lucid-32 problem.

Lucid-32 fixed, if toolchain is ok, will also do DEPS roll.