2007-10-02 Marcus Brinkmann <marcus@g10code.de>

* priv-io.h, engine-gpgsm.c: Add comments.
	* w32-qt-io.cpp (_gpgme_io_select): Remove code handling frozen FDs.
	* w32-glib-io.c (_gpgme_io_close): Always dereference the channel,
	even if not primary.
	(_gpgme_io_dup): Acquire a reference.  Replace unused
	implementation by assertion.

gpgme/ChangeLog

@@ -1,3 +1,12 @@
+2007-10-02  Marcus Brinkmann  <marcus@g10code.de>
+
+	* priv-io.h, engine-gpgsm.c: Add comments.
+	* w32-qt-io.cpp (_gpgme_io_select): Remove code handling frozen FDs.
+	* w32-glib-io.c (_gpgme_io_close): Always dereference the channel,
+	even if not primary.
+	(_gpgme_io_dup): Acquire a reference.  Replace unused
+	implementation by assertion.
+
 2007-09-28  Werner Koch  <wk@g10code.com>
 
 	* engine-gpgsm.c (iocb_data_t): Add SERVER_FD_STR.

gpgme/engine-gpgsm.c

@@ -54,7 +54,7 @@ typedef struct
   void *data;	/* Handler-specific data.  */
   void *tag;	/* ID from the user for gpgme_remove_io_callback.  */
   char server_fd_str[15]; /* Same as SERVER_FD but as a string.  We
-                             need this because _gpgme_io_fdstr can't
+                             need this because _gpgme_io_fd2str can't
                              be used on a closed descriptor.  */
 } iocb_data_t;
 
@@ -530,8 +530,9 @@ gpgsm_new (void **engine, const char *file_name, const char *home_dir)
 #endif
 
  leave:
-  /* Close the server ends of the pipes.  Our ends are closed in
-     gpgsm_release().  */
+  /* Close the server ends of the pipes (because of this, we must use
+     the stored server_fd_str in the function start).  Our ends are
+     closed in gpgsm_release().  */
 #if !USE_DESCRIPTOR_PASSING
   if (gpgsm->input_cb.server_fd != -1)
     _gpgme_io_close (gpgsm->input_cb.server_fd);
@@ -1013,11 +1014,15 @@ start (engine_gpgsm_t gpgsm, const char *command)
   if (nfds < 1)
     return gpg_error (GPG_ERR_GENERAL);	/* FIXME */
 
-  /* We duplicate the file descriptor, so we can close it without
-     disturbing assuan.  Alternatively, we could special case
-     status_fd and register/unregister it manually as needed, but this
-     increases code duplication and is more complicated as we can not
-     use the close notifications etc.  */
+  /* We "duplicate" the file descriptor, so we can close it here (we
+     can't close fdlist[0], as that is closed by libassuan, and
+     closing it here might cause libassuan to close some unrelated FD
+     later).  Alternatively, we could special case status_fd and
+     register/unregister it manually as needed, but this increases
+     code duplication and is more complicated as we can not use the
+     close notifications etc.  A third alternative would be to let
+     Assuan know that we closed the FD, but that complicates the
+     Assuan interface.  */
 
   gpgsm->status_cb.fd = _gpgme_io_dup (fdlist[0]);
   if (gpgsm->status_cb.fd < 0)

gpgme/priv-io.h

@@ -64,7 +64,13 @@ int _gpgme_io_select (struct io_select_fd_s *fds, size_t nfds, int nonblock);
    line that the child process expects.  */
 int _gpgme_io_fd2str (char *buf, int buflen, int fd);
 
-/* Like dup().  */
+/* Duplicate a file descriptor.  This is more restrictive than dup():
+   it assumes that the resulting file descriptors are essentially
+   co-equal (for example, no private offset), which is true for pipes
+   and sockets (but not files) under Unix with the standard dup()
+   function.  Basically, this function is used to reference count the
+   status output file descriptor shared between GPGME and libassuan
+   (in engine-gpgsm.c).  */
 int _gpgme_io_dup (int fd);
 
 #endif /* IO_H */

gpgme/w32-glib-io.c

@@ -81,7 +81,23 @@
 static struct 
 {
   GIOChannel *chan;
-  int primary;   /* Set if CHAN is the one we used to create the channel.  */
+  /* The boolean PRIMARY is true if this file descriptor caused the
+     allocation of CHAN.  Only then should CHAN be destroyed when this
+     FD is closed.  This, together with the fact that dup'ed file
+     descriptors are closed before the file descriptors from which
+     they are dup'ed are closed, ensures that CHAN is always valid,
+     and shared among all file descriptors refering to the same
+     underlying object.
+
+     The logic behind this is that there is only one reason for us to
+     dup file descriptors anyway: to allow simpler book-keeping of
+     file descriptors shared between GPGME and libassuan, which both
+     want to close something.  Using the same channel for these
+     duplicates works just fine (and in fact, using different channels
+     does not work because the W32 backend in glib does not support
+     that: One would end up with several competing reader/writer
+     threads.  */
+  int primary;
 } giochannel_table[MAX_SLAFD];
 
 
@@ -306,10 +322,11 @@ _gpgme_io_close (int fd)
   if (giochannel_table[fd].chan)
     {
       if (giochannel_table[fd].primary)
-        {
 	g_io_channel_shutdown (giochannel_table[fd].chan, 1, NULL);
+      else
+	_close (fd);
+
       g_io_channel_unref (giochannel_table[fd].chan);
-        }
       giochannel_table[fd].chan = NULL;
     }
   else
@@ -731,40 +748,24 @@ _gpgme_io_dup (int fd)
   
   TRACE_BEG1 (DEBUG_SYSIO, "_gpgme_io_dup", fd, "dup (%d)", fd);
 
-  newfd =_dup (fd);
+  newfd = _dup (fd);
   if (newfd == -1)
     return TRACE_SYSRES (-1);
   if (newfd < 0 || newfd >= MAX_SLAFD)
     {
-      /* New fd won't fit into our table.  */
+      /* New FD won't fit into our table.  */
       _close (newfd);
       errno = EIO; 
       return TRACE_SYSRES (-1);
     }
+  assert (giochannel_table[newfd].chan == NULL);
 
   chan = find_channel (fd, 0);
-  if (!chan)
-    {
-      /* No channel exists for the original fd, thus we create one for
-         our new fd.  */
-      if ( !find_channel (newfd, 1) )
-        {
-          _close (newfd);
-          errno = EIO;
-          return TRACE_SYSRES (-1);
-        }
-    }
-  else
-    {
-      /* There is already a channel for the original one.  Copy that
-         channel into a new table entry unless we already did so.  */
-      if ( !giochannel_table[newfd].chan)
-        {
+  assert (chan);
+
+  g_io_channel_ref (chan);
   giochannel_table[newfd].chan = chan;
   giochannel_table[newfd].primary = 0;
-        }
-      assert (giochannel_table[newfd].chan == chan);
-    }
 
   return TRACE_SYSRES (newfd);
 }

gpgme/w32-qt-io.cpp

@@ -1,4 +1,4 @@
-/* w32-glib-io.c - W32 Glib I/O functions
+/* w32-qt-io.c - W32 Glib I/O functions
    Copyright (C) 2000 Werner Koch (dd9jn)
    Copyright (C) 2001, 2002, 2004, 2005, 2007 g10 Code GmbH
 
@@ -583,12 +583,7 @@ _gpgme_io_select (struct io_select_fd_s *fds, size_t nfds, int nonblock)
         {
           fds[i].signaled = 0;
         }
-      else if (fds[i].frozen)
-        {
-          TRACE_ADD1 (dbg_help, "f0x%x ", fds[i].fd);
-          fds[i].signaled = 0;
-        }
-      else if (fds[i].for_read )
+      else if (fds[i].for_read)
         {
           const QIODevice * const chan = find_channel (fds[i].fd, 0);
           assert (chan);