Corinna Vinschen wrote:
Hi Mark,
this looks good. Inline comments as usual.
Thank you; OK.
On Jul 15 01:20, Mark Geisert wrote:
[...]
+static int
+aiochkslot (struct aiocb *aio)
+{
+ /* Sanity check.. make sure this AIO is not already busy */
+ for (int slot = 0; slot < AIO_MAX; ++slot)
+ if (evt_aiocbs[slot] == aio)
+ {
+ debug_printf ("aio %p is already busy in slot %d", aio, slot);
+ return slot;
+ }
+
+ return -1;
+}
Shouldn't this check be under lock as well? Or I am missing something.
Why is the lock not necessary here?
Brain hiccup on my part. I was thinking read-only simple access doesn't need to
be locked. That's wrong and fixed now.
+aionotify_on_pthread (struct sigevent *evp)
+{
+ pthread_attr_t *attr;
+ pthread_attr_t default_attr;
+ int rc;
+ pthread_t vaquita; /* == "little porpoise", endangered */
+
+ if (evp->sigev_notify_attributes)
+ attr = evp->sigev_notify_attributes;
+ else
+ {
+ pthread_attr_init (attr = &default_attr);
+ pthread_attr_setdetachstate (attr, PTHREAD_CREATE_DETACHED);
+ }
+
+ rc = pthread_create (&vaquita, attr,
+ (void * (*) (void *)) evp->sigev_notify_function,
+ evp->sigev_value.sival_ptr);
+
+ /* The following error is not expected. If seen often, develop a recovery. */
+ if (rc)
+ debug_printf ("aio vaquita thread creation failed, %E");
I like the name, but what's the background for naming a thread like this?
Just curious. A bit of comment might help to keep it in mind, too :)
I've now added commentary above the pthread_create() call. It reads:
/* A "vaquita" thread is a temporary pthread created to deliver a signal to
* the application. We don't wait around for the thread to return from the
* app. There's some symbolism here of sending a little creature off to tell
* the app something important. If all the vaquitas end up wiped out in the
* wild, a distinct near-term possibility, at least this code remembers them.
*/
If all this vaquita stuff is deemed too precious for industrial-grade software I
can recast this code in more professional terms and wouldn't mind doing it.
[...]
+ /* Do the requested AIO operation manually, synchronously */
+ switch (aio->aio_lio_opcode)
+ {
+ case LIO_READ:
+ /*XXX Hmm, no direct result? This OK? */
Unfortunately the fhandler read method has been written this way more
than 20 years ago. I have no idea why and it's ugly as hell.
But there is a result: The second parameter is set to the number of bytes
read or -1 on error, in which case errno is set.
Feel free to rewrite the fhandler read method to look more sane :)
Thanks for the background; I missed the 2nd arg being passed by reference. I've
left the code as-is but have now added a little commentary to describe what's
going on. Re-writing the fhandler read method is left for the future...
+#ifdef _POSIX_SYNCHRONIZED_IO
We really don't need this ifdef, not even in the header. The macro is
certainly defined.
All occurrences have now been deleted.
diff --git a/winsup/cygwin/include/aio.h b/winsup/cygwin/include/aio.h
new file mode 100644
index 000000000..34ff29969
--- /dev/null
+++ b/winsup/cygwin/include/aio.h
@@ -0,0 +1,82 @@
+/* aio.h: Support for Posix asynchronous i/o routines.
+
+This file is part of Cygwin.
+
+This software is a copyrighted work licensed under the terms of the
+Cygwin license. Please consult the file "CYGWIN_LICENSE" for
+details. */
+
+#ifndef _AIO_H
+#define _AIO_H
+
+#include <sys/features.h>
+#include <sys/queue.h>
+#include <sys/signal.h>
+#include <sys/types.h>
+#include <limits.h> // for AIO_LISTIO_MAX, AIO_MAX, and AIO_PRIO_DELTA_MAX
+
+#ifndef __INSIDE_CYGWIN__
+#include <w32api/winternl.h> // for HANDLE and IO_STATUS_BLOCK
+#endif
Hmm. Did I miss this last time? Looks like it.
+/* struct aiocb is defined by Posix */
+struct aiocb {
+ /* these elements of aiocb are Cygwin-specific */
+ TAILQ_ENTRY(aiocb) aio_chain;
+ struct liocb *aio_liocb;
+ HANDLE aio_win_event;
+ IO_STATUS_BLOCK aio_win_iosb;
+ ssize_t aio_rbytes;
+ int aio_errno;
+ /* the remaining elements of aiocb are defined by Posix */
+ int aio_lio_opcode;
+ int aio_reqprio; /* Not yet implemented; must be zero */
+ int aio_fildes;
+ volatile void *aio_buf;
+ size_t aio_nbytes;
+ off_t aio_offset;
+ struct sigevent aio_sigevent;
+};
Can you please change this so it doesn't require to include a
windows header? You could use void * instead of HANDLE and define
your own __io_t (or whatever) as a struct of void * and uintptr_t
values and only cast them to the Windows types inside of Cygwin.
That ok?
It's totally OK; I'm still fine tuning the replacement code for readability and
ease of coding.
..mark
