Hi, thanks, and sorry for the very late review of your patch...
On Tue, May 31, 2016 at 12:26 AM, Madhuri Yechuri <[email protected]> wrote: > --- > libc/af_local.cc | 27 +++++++++++++++------- > libc/pipe_buffer.cc | 17 ++++++++++++++ > tests/tst-af-local.cc | 64 > ++++++++++++++++++++++++++++++++++++++++++++++++++- > 3 files changed, 99 insertions(+), 9 deletions(-) > > diff --git a/libc/af_local.cc b/libc/af_local.cc > index b68c2b0..b98e87c 100644 > --- a/libc/af_local.cc > +++ b/libc/af_local.cc > @@ -42,15 +42,26 @@ int af_local::ioctl(u_long cmd, void *data) > int error = ENOTTY; > switch (cmd) { > case FIONBIO: > - SCOPE_LOCK(f_lock); > - if (*(int *)data) > - f_flags |= FNONBLOCK; > - else > - f_flags &= ~FNONBLOCK; > - error = 0; > - break; > + { > + SCOPE_LOCK(f_lock); > + if (*(int *)data) > + f_flags |= FNONBLOCK; > + else > + f_flags &= ~FNONBLOCK; > + error = 0; > + break; > + } > Just curious, why did you need to add the "{" here? It's indeed suspicious how SCOPE_LOCK() worked without it, but it did seem to work before, so why this change? > + case FIOASYNC: > + { > + SCOPE_LOCK(f_lock); > + if (*(int *)data) > + f_flags |= FASYNC; > + else > + f_flags &= ~FASYNC; > + error = 0; > + break; > + } > } > - > return error; > } > > diff --git a/libc/pipe_buffer.cc b/libc/pipe_buffer.cc > index acd5d0b..04d73a0 100644 > --- a/libc/pipe_buffer.cc > +++ b/libc/pipe_buffer.cc > @@ -11,6 +11,7 @@ > #include "pipe_buffer.hh" > > #include <osv/poll.h> > +#include <signal.h> > > void pipe_buffer::detach_sender() > { > @@ -110,6 +111,14 @@ int pipe_buffer::read(uio* data, bool nonblock) > copy_to_uio(q, data); > if (write_events_unlocked() & POLLOUT) > poll_wake(sender, (POLLOUT | POLLWRNORM)); > + if (sender->f_flags & FASYNC) { > How does read() know that "sender" even exists? I.e., the sending end of the pipe may have already been closed and this test would have caused read() to crash. I'm surprised you didn't see this in one of the existing pipe unit tests (unless I'm misunderstanding something). Note that poll_wake() above doesn't have this problem because it does nothing when given a null parameter. Perhaps need to do if (sender && sender->f_flags & FASYNC)? By the way, I'm not familiar enough with FASYNC, maybe you can help me understand here: What is the signal meant to signal? If the read() only reads part of the data, and not everything the writer has previously written (i.e., the hasn't yet become empty), are we still supposed to send this signal? > + // Deliver SIGIO to the OSv process. > + // Note: There is no need to gather sender fd owner pid > + // because OSv implements one process. > + // FIXME: Once Out-of-band support is available in OSv, > + // support delivery of SIGURG. > + kill(getpid(), SIGIO); > Just curious (again, I'm not a FASYNC expert) - how is the handler supposed to know why it got a SIGIO if there are multiple file descriptors? Is it supposed to check all the file descriptors each time it receives a signal? > + } > lock.unlock(); > may_write.wake_all(); > return 0; > @@ -190,6 +199,14 @@ int pipe_buffer::write(uio* data, bool nonblock) > } > if (read_events_unlocked() & POLLIN) > poll_wake(receiver, (POLLIN | POLLRDNORM)); > + if (receiver->f_flags & FASYNC) { > + // Deliver SIGIO to the OSv process. > + // Note: There is no need to gather receiver fd owner pid > + // because OSv implements one process. > + // FIXME: Once Out-of-band support is available in OSv, > + // support delivery of SIGURG. > + kill(getpid(), SIGIO); > + } > } > may_read.wake_all(); > return 0; > diff --git a/tests/tst-af-local.cc b/tests/tst-af-local.cc > index f65b2d9..3e66001 100644 > --- a/tests/tst-af-local.cc > +++ b/tests/tst-af-local.cc > @@ -3,16 +3,23 @@ > * > * This work is open source software, licensed under the terms of the > * BSD license as described in the LICENSE file in the top-level > directory. > + * > */ > > #include <sys/socket.h> > #include <sys/poll.h> > #include <unistd.h> > +#include <signal.h> > +#include <thread> > +#include <osv/ioctl.h> > #include <osv/sched.hh> > #include <osv/debug.hh> > > int tests = 0, fails = 0; > > +std::atomic<int> sigio_count { 0 }; > Nitpick: please use "static" on this variable, so it's not visible to other code. (this nitpick also applies to other variable of this file, but they are not new in this patch...) > +void handler(int sig) { sigio_count += 1;} > + > static void report(bool ok, const char* msg) > { > ++tests; > @@ -20,14 +27,70 @@ static void report(bool ok, const char* msg) > debug("%s: %s\n", (ok ? "PASS" : "FAIL"), msg); > } > > +void reset_fio_flags(int *s) { > Again, nice to add "static" if no other code uses this. > + int flag = 0; > + int r = ioctl(s[0], FIONBIO, &flag); > + report(r == 0, "ioctl, turned off FIONBIO for write"); > + r = ioctl(s[1], FIONBIO, &flag); > + report(r == 0, "ioctl, turned off FIONBIO for read"); > + r = ioctl(s[0], FIOASYNC, &flag); > + report(r == 0, "ioctl, turned off FIOASYNC for write"); > + r = ioctl(s[1], FIOASYNC, &flag); > + report(r == 0, "ioctl, turned off FIOASYNC for read"); > +} > int main(int ac, char** av) > { > int s[2]; > + int flag = 1; > > int r = socketpair(AF_LOCAL, SOCK_STREAM, 0, s); > report(r == 0, "socketpair call"); > > + // Begin: Test ioctl FIOCLEX (randomly picked from unsupported > commands). > + r = ioctl(s[0], FIOCLEX, &flag); > + report(r == -1 && errno == ENOTTY, "ioctl, unsupported command > FIOCLEX"); > + // End: Test ioctl FIOCLEX. > + > + // Begin: Test ioctl FIONBIO (supported). > + flag = 1; > + r = ioctl(s[1], FIONBIO, &flag); > + report(r == 0, "ioctl, turned on FIONBIO for read"); > char msg[] = "hello", reply[] = "wrong"; > + r = read(s[1], reply, 5); > + report(r == -1, > + "read, non-blocking read succeeded when there is nothing to > read"); > + r = write(s[0], msg, 5); > + report(r == 5, "write to empty socket"); > + r = read(s[1], reply, 5); > + report(r == 5 && memcmp(msg, reply, 5) == 0, "read after write"); > + reset_fio_flags(s); > + // End: Test ioctl FIONBIO (supported). > + > + // Begin: Test ioctl FIOASYNC (supported). > + auto sr = signal(SIGIO, handler); > + flag = 1; > + r = ioctl(s[0], FIOASYNC, &flag); > + report(r == 0, "ioctl, turned on FIOASYNC for write"); > + r = ioctl(s[1], FIOASYNC, &flag); > + report(r == 0, "ioctl, turned on FIOASYNC for read"); > + std::thread thread1([&] {r = write(s[0], msg, 5);} ); > + while (sigio_count == 0) { > + usleep(100000); > + }; > + report(sigio_count == 1, "SIGIO received after write completed"); > + r = read(s[1], reply, 5); > + report(r == 5 && memcmp(msg, reply, 5) == 0, "read after write"); > + while (sigio_count == 1) { > + usleep(100000); > + } > + report(sigio_count == 2, "SIGIO received after read completed"); > + thread1.join(); > + close(s[0]); > + close(s[1]); > + // End: Test ioctl FIOASYNC. > + > + r = socketpair(AF_LOCAL, SOCK_STREAM, 0, s); > + report(r == 0, "socketpair call"); > r = write(s[0], msg, 5); > report(r == 5, "write to empty socket"); > r = read(s[1], reply, 5); > @@ -131,7 +194,6 @@ int main(int ac, char** av) > } > report(nsock > 100, "create many sockets"); > > - > debug("SUMMARY: %d tests, %d failures\n", tests, fails); > } > > -- > 2.7.4 > > -- > You received this message because you are subscribed to the Google Groups > "OSv Development" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to [email protected]. > For more options, visit https://groups.google.com/d/optout. > -- You received this message because you are subscribed to the Google Groups "OSv Development" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. For more options, visit https://groups.google.com/d/optout.
