Re: NSL_FORK [Re: LYNX-DEV Lynx 3.0]

[Bela Lubkin]

Tom Zerucha wrote:

> I think I am missing a close (that the child exit does not handle) on one
> of the two fds I create with the pipe.
> 
> It is only in WWW/.../HTTCP.c, do a search on NSL_FORK.
> 
> Where you see a close(pfd[0]), you also need a close(pfd[1]), or can
> probably close this side just after forking (in the parent).
> 
> I think I fixed it, but the patch integrated into 2.6/7 didn't contain it,
> and I forgot about it.

I had sent you a patch-to-your-patch, but it never seemed to get
integrated.  I should have followed Fote's Golden Rule and mailed it to
the list, not privately...

Not sure how this applies to current code sets, and I don't have time to
figure it out right now, but here's the old message.  (It contains a
unified, not context, diff... sorry.  It wasn't originally targeted at
the whole list.)

>Bela<

From: address@hidden (Bela Lubkin)
Date: Mon, 4 Nov 1996 02:35:13 -0800
X-Mailer: Mail User's Shell (7.2.5 10/14/92)
To: address@hidden
Subject: updated NSL_FORK patch

Tom --

First, just curious, are you in Milwaukee?  I grew up in Shorewood...

I've done a bunch of work on the NSL_FORK patch.  I believe the new
version should be portable to as wide a range of Unix systems as the
Lynx code itself, because this version of the patch doesn't introduce
any new operating system facilities.  Your version used ioctl(FIONREAD).
As it turns out, my operating system (SCO OpenServer Release 5) *does*
support this ioctl, but *only* on sockets, and pipes are not implemented
on top of sockets.  (This differs from BSD systems where pipes are a
special form of socket.)  So your code would compile (the header files
supplied a value for the FIONREAD macro), but it didn't actually work
because the ioctl just returned EINVAL.

Along the way I made a *lot* of other changes -- I would say I
essentially rewrote the whole patch.  Which is not to take away from
what you did: you located the right place and the right sort of
solution.  I "integrated" the solution so that it would be more robust
in a variety of real-world situations.

In some detail, what I did was:

  o Add comments at top of the file showing both of our contributions
  o Change indentation to match the rest of the file
  o Change some of the spacing to match my programming style, mostly
    because I rewrote a lot of it and it ended up looking weirdly
    inconsistent
  o Undid some code duplication that you introduced (the error-return
    "Can't find internet node name" section)
  o Used select() instead of ioctl(FIONREAD)
  o select() on stdin as well as the child pipe, when using curses (not
    SLANG).  This allows interrupt to be processed instantly, without
    up-to-1-second delay.
  o Close pipe ends as soon as possible
  o While looping, 1 second at a time, bail out after "too many"
    (currently 50) loops: just in case something weird happens, like
    select() returning some error
  o Make sure all pipe ends closed before returning
  o Make sure child processes always killed and reaped
  o Fixed CMU_TCP code path (though in fact I doubt any of this will
    work on VMS anyway) -- you had it writing h_length down the pipe,
    which presumably would have hit that bug; then always reading 4
    bytes...
  o Now write the length down the pipe (using a known-to-the-program
    length of sizeof(int)) first -- this both helps workaround the
    CMU_TCP bug and the fact that select() doesn't let us get number of
    bytes ready, like FIONREAD does.
  o Check system call returns everywhere that it makes sense
  o Note: #ifdef MVS stuff faithfully duplicated, but that looks to me
    like somebody's long-ago debug code and should probably be shot (why
    would you want lookups to be traced under MVS but not elsewhere?)

So... I don't want to post this to the list, because I still consider it
"your" patch.  Would you check this over and then update your patch to
match (preferably by simply replacing it, unless you have some additions
to make).  I feel that if I post it to the list with this sort of
explanation, it will be taken as a comment that "Tom wrote this bad
code, which I have fixed", which isn't at all what I intend.

This version should be portable enough to be merged into Rob's
coalescing developmental tree.  Let's work to get that to happen.

Thanks for the seed; I'm finding Lynx considerably more responsive now
that this works...  Now to try to deal with the asynchronous status line
patch...

>Bela<

PS: the following patch is against the original, not-patched-with-your-
    patch version of HTTCP.c.  As far as diff can tell, it's actually
    more different from your version than from the original!  ;-}

=============================================================================

*** WWW/Library/Implementation/HTTCP.c-pre-nsl  Sat Oct 19 16:20:40 1996
--- WWW/Library/Implementation/HTTCP.c  Mon Nov  4 01:23:52 1996
***************
*** 14,21 ****
--- 14,27 ----
  **    20 May 94  Andy Harper  Added support for CMU TCP/IP transport
  **    17 Nov 94  Andy Harper  Added support for SOCKETSHR transport
  **      16 Jul 95  S. Bjorndahl added kluge to deal with LIBCMU bug
+ **     4 Sep 96  Thomas Zerucha  Added NSL_FORK for interruptable NS lookup
+ **     4 Nov 96  Bela Lubkin  Made NSL_FORK code more portable
  */
  
+ #ifdef NSL_FORK
+ #include <signal.h>
+ #include <sys/wait.h>
+ #endif
  
  #include "HTUtils.h"
  #include "tcp.h"              /* Defines SHORT_NAMES if necessary */
***************
*** 328,333 ****
--- 334,340 ----
      char *host = NULL;
      int dotcount_ip = 0;      /* for dotted decimal IP addr */
      struct hostent  *phost;   /* Pointer to host - See netdb.h */
+     int success = 0;
  
      if (!str) {
        if (TRACE)
***************
*** 410,428 ****
        if(TRACE)
            fprintf(stderr, "HTTCP: Calling gethostbyname(%s)\n", host);
  #endif
        phost=gethostbyname(host);      /* See netdb.h */
  #ifdef MVS
        if(TRACE)
            fprintf(stderr, "HTTCP: gethostbyname() returned %d\n", phost);
  #endif
!       if (!phost) {
!           if (TRACE)
!               fprintf(stderr, 
!                   "HTTPAccess: Can't find internet node name `%s'.\n",host);
!           FREE(host);
!           return -1;  /* Fail? */
!       }
!       FREE(host);
  #if defined(VMS) && defined(CMU_TCP)
   /*  In LIBCMU, phost->h_length contains not the length of one address
    *  (four bytes) but the number of bytes in *h_addr, i.e. some multiple
--- 417,431 ----
        if(TRACE)
            fprintf(stderr, "HTTCP: Calling gethostbyname(%s)\n", host);
  #endif
+ 
+ #ifndef NSL_FORK
        phost=gethostbyname(host);      /* See netdb.h */
  #ifdef MVS
        if(TRACE)
            fprintf(stderr, "HTTCP: gethostbyname() returned %d\n", phost);
  #endif
!       if (phost) {
!           success = 1;
  #if defined(VMS) && defined(CMU_TCP)
   /*  In LIBCMU, phost->h_length contains not the length of one address
    *  (four bytes) but the number of bytes in *h_addr, i.e. some multiple
***************
*** 432,441 ****
    *  SOCKETSHR/NETLIB instead.
    *  S. Bjorndahl 
    */
!       memcpy((void *)&sin->sin_addr, phost->h_addr, 4);
  #else
!       memcpy((void *)&sin->sin_addr, phost->h_addr, phost->h_length);
  #endif /* VMS && CMU_TCP */
      }
  
      if (TRACE)
--- 435,556 ----
    *  SOCKETSHR/NETLIB instead.
    *  S. Bjorndahl 
    */
!           memcpy((void *)&sin->sin_addr, phost->h_addr, 4);
  #else
!           memcpy((void *)&sin->sin_addr, phost->h_addr, phost->h_length);
  #endif /* VMS && CMU_TCP */
+       }
+ 
+ #else /* NSL_FORK */
+       /* start block for my changes - tz */
+       { 
+           /* pipe, child pid, status buffers */
+           int pfd[2],fpid,cstat,cst1,cycle=0;
+           fd_set readfds;
+           struct timeval timeout;
+ 
+           if (pipe(pfd) < 0) {
+               HTAlert("pipe failed.");
+               FREE(host);
+               return -1;
+           }
+ 
+           if( ( fpid = fork() ) == 0 ) {
+               /* child - for the long call */
+               close(pfd[0]);  /* child won't use read side -BL */
+               phost=gethostbyname(host);
+ #ifdef MVS
+               if(TRACE)
+                   fprintf(stderr, "HTTCP: gethostbyname() returned %d\n", 
phost);
+ #endif
+               /* send parent the length of subsequent value (as a native int) 
*/
+               if( phost != NULL )
+ #if defined(VMS) && defined(CMU_TCP)
+                   cstat = 4;  /* see S. Bjorndahl comment above -BL */
+ #else
+                   cstat = phost->h_length;
+ #endif
+               else
+                   cstat = 0;
+               write(pfd[1], &cstat, sizeof cstat);
+               /* return value (if we have one) */
+               if( phost != NULL )
+                   write( pfd[1], phost->h_addr , phost->h_length );
+               /* return an error code */
+               _exit( phost == NULL );
+           }
+ 
+           /* (parent) wait until lookup finishes, or interrupt,
+              or cycled too many times (just in case) -BL */
+ 
+           close(pfd[1]);      /* parent won't use write side -BL */
+           while( cycle < 50 ) {
+               /* avoid infinite loop in the face of the unexpected -BL */
+               cycle++;
+               timeout.tv_sec = 1;
+               timeout.tv_usec = 0;
+               FD_ZERO(&readfds);
+               FD_SET(pfd[0], &readfds);
+ #ifndef USE_SLANG
+               /* This allows us to abort immediately, not after 1-second
+                  timeout, when user hits abort key.  Can't do this when
+                  using SLANG (or at least I don't know how), so SLANG
+                  users must live with up-to-1s timeout.  -BL */
+               FD_SET(0, &readfds);    /* stdin -BL */
+ #endif /* USE_SLANG */
+ 
+               /* exit when data sent */
+ #ifdef SOCKS
+               if (socks_flag)
+                   cst1 = Rselect(pfd[0] + 1, (void *)&readfds, NULL, NULL, 
&timeout);
+               else
+ #endif /* SOCKS */
+                   cst1 = select(pfd[0] + 1, (void *)&readfds, NULL, NULL, 
&timeout);
+               /* get response and end loop, if response from child -BL */
+               if ((cst1 > 0) && FD_ISSET(pfd[0], &readfds)) {
+                   /* first get length of address -BL */
+                   cst1 = read(pfd[0], (void *)&cstat, sizeof cstat);
+                   if (cst1 == sizeof cstat) {
+                       /* then get address itself -BL */
+                       cst1 = read(pfd[0], (void *)&sin->sin_addr, cstat);
+                       if (cst1 == cstat) success = 1;
+                   }
+                   /* make sure child is cleaned up -BL */
+                   waitpid(fpid, &cst1, WNOHANG);
+                   if (!WIFEXITED(cst1) && !WIFSIGNALED(cst1)) {
+                       kill( fpid, SIGKILL );
+                       waitpid(fpid, (void *)0, WNOHANG);
+                   }
+                   break;
+               }
+ 
+               /* end loop if child exited */
+               if( waitpid(fpid, (void *)0, WNOHANG) > 0 )
+                   break;
+ 
+               /* abort if interrupt key pressed */
+               if (HTCheckForInterrupt()) {
+                   if (TRACE)
+                       fprintf (stderr, "*** INTERRUPTED gethostbyname.\n");
+                   kill(fpid, SIGKILL);
+                   close(pfd[0]);
+                   waitpid(fpid, &cst1, WNOHANG);
+                   FREE(host);
+                   return HT_INTERRUPTED;
+               }
+           }
+           close(pfd[0]);
+       }
+ #endif /* NSL_FORK */
+ 
+       if( !success ) {
+           if (TRACE)
+               fprintf(stderr, 
+                   "HTTPAccess: Can't find internet node name `%s'.\n",host);
+             FREE(host);
+             return -1;
+       }
+       FREE(host);
      }
  
      if (TRACE)
;
; To UNSUBSCRIBE:  Send a mail message to address@hidden
;                  with "unsubscribe lynx-dev" (without the
;                  quotation marks) on a line by itself.
;