Re: Generic gloss read patch

Re: Generic gloss read patch

[J. Johnston]

Ok,

  Instead of making the gloss32::read wait until len characters are available, I have
changed the patch as Frank suggested: it will read all available characters from the rx buffer
at once up to the length requested.  I changed the end of the loop to exit if we have read
from the rx buffer so we won't end up looping endlessly and lose the input thus far.

  The other part of the problem stemmed from the fact that the stdio component was only
reading one character each time it was polled which meant there was only one character
in the buffer when we came in to do a read.  This code has been changed to
try to read a maximum of 1000 bytes.

  With the change the test sequence below behaves as expected under sid gloss:

 write (1, "\nEnter 10 characters\n", 21);
 write (1, "0123456789\n", 11);
 read (0, buffer, 10);
 write (1, buffer, 10);
 write (1, "\n", 1);

I have attached the new patch.  The following are the ChangeLog entries.  Ok to commit?

sid/component/consoles/ChangeLog

2001-01-17  Jeff Johnston  <jjohnstn@redhat.com>

        * stdio.cxx (read): Change to read a reasonable size of data each
	time stdin gets polled.


sid/component/gloss/ChangeLog

2001-01-17  Jeff Johnston  <jjohnstn@redhat.com>

        * gloss.cxx (read): Change use_rx_p code to extract as much
        as possible from the rx buffer (up to the len requested)
        and to exit at the end of the main loop if any characters
        are successfully read.


-- Jeff J.
Index: consoles/stdio.cxx
===================================================================
RCS file: /cvs/src/src/sid/component/consoles/stdio.cxx,v
retrieving revision 1.1
diff -u -r1.1 stdio.cxx
--- stdio.cxx	2000/12/07 19:30:50	1.1
+++ stdio.cxx	2001/01/17 19:45:17
@@ -32,17 +32,20 @@
 void
 stdioConsole::read(host_int_4)
 {
-  unsigned char c;
+  unsigned char buf[1000];
+  int len;
   host_int_4 value;
 
   // Switch to non-blocking input.
   long flags = fcntl(0, F_GETFL);
   fcntl(0, F_SETFL, flags | O_NONBLOCK);
 
-  if (::read(0, &c, 1) > 0)
+  if ((len = ::read(0, buf, 1000)) > 0)
     {
-      value = c;
-      stdin_pin.drive(value);
+      for (int i = 0; i < len; ++i)
+	{
+	  stdin_pin.drive(buf[i]);
+	}
     }
 
   // Restore flags.
Index: gloss/gloss.cxx
===================================================================
RCS file: /cvs/src/src/sid/component/gloss/gloss.cxx,v
retrieving revision 1.3
diff -u -r1.3 gloss.cxx
--- gloss.cxx	2001/01/08 04:30:42	1.3
+++ gloss.cxx	2001/01/17 19:45:18
@@ -1038,8 +1038,13 @@
 
 	  if (rx_buffer.size() > 0)
 	    {
-	      c = rx_buffer.front();
-	      rx_buffer.erase(rx_buffer.begin());
+	      count_read = min (len, rx_buffer.size());
+	      for (int i = 0; i < count_read; ++i)
+		{
+		  c = rx_buffer.front();
+		  rx_buffer.erase (rx_buffer.begin());
+		  strbuf += c;
+		}
 	    }
 	  else
 	    {
@@ -1047,8 +1052,6 @@
 	      errcode = EAGAIN;
 	      return false;
 	    }
-	  count_read = 1;
-	  strbuf = c;
 	}
       else
 	{
@@ -1095,6 +1098,11 @@
       addr = addr + count_read;
       total_read += count_read;
       len -= count_read;
+
+      // if we have read from the rx_buffer, then we have either emptied it
+      // or read the required number of characters so we should exit the loop
+      if (use_rx_p && count_read > 0)
+	break;
     }
 
   len_read = total_read;

Re: Generic gloss read patch

[Ben Elliston]

   I have attached the new patch.  The following are the ChangeLog
   entries.  Ok to commit?

Yes, but please include the class name in the ChangeLog -- eg.
"(stdioConsole::read): Change to read ..".

Ben

Re: Generic gloss read patch

[J. Johnston]

Ben Elliston wrote:
> 
>    I have attached the new patch.  The following are the ChangeLog
>    entries.  Ok to commit?
> 
> Yes, but please include the class name in the ChangeLog -- eg.
> "(stdioConsole::read): Change to read ..".
> 
> Ben

Done.

-- Jeff J.

Re: Generic gloss read patch

[Frank Ch. Eigler]

Hi -

jjohnstn wrote:

: I ran into a problem with gloss32::read trying to read n characters
: from stdin.
:    read (0, buffer, 10);
: What happens is that the gloss32::read function reads one character at
: a time from the buffer in a loop that is checking to see if the total
: number of characters has been read.  The read buffer doesn't get filled
: right away so if read empties the buffer before the len characters are
: read, it returns false and denotes the read as blocked.  [...]

That's true - gloss is not right to do that.  However, the UNIX read system
call is permitted to return any number of bytes > 0 and <= 10 in response to
such a call; it need not block until exactly 10 arrive.

This means that your fix is nearly right, except that it should not assert

! 	  // check if we have enough characters to satisfy the request
! 	  // if not, no point in reading since we will end up blocked
! 	  // and the read will be reissued
! 	  if (rx_buffer.size() >= len)

but rather "> 0"; and then the loop

! 	      for (int i = 0; i < len; ++i)

should read

! 	      for (int i = 0; i < len && rx_buffer.size() > 0; ++i)


By the way, as an alternative to the character-by-character copy loop
from rx_buffer to strbuf, consider something like

	strbuf = rx_buffer.substr (0, min(len, rx_buffer.size()));
	rx_buffer = rx_buffer.substr (min(len, rx_buffer.size()));


- FChE
-- 
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.0.4 (GNU/Linux)
Comment: For info see http://www.gnupg.org

iD8DBQE6ZOefVZbdDOm/ZT0RAmWpAJ0b98jLi4lVHwjTLsWGbBMoVXqMigCcDWkJ
4N2qNQbWMrW8AlYVuHUb2ZA=
=R3cT
-----END PGP SIGNATURE-----

Generic gloss read patch

[J. Johnston]

I ran into a problem with gloss32::read trying to read n characters from stdin.  

   read (0, buffer, 10);

What happens is that the gloss32::read function reads one character at a time from the buffer
in a loop that is checking to see if the total number of characters has
been read.  The read buffer doesn't get filled right away so if read empties the buffer 
before the len characters are read, it returns false and denotes the read as blocked.  
This causes the read to be reissued with the original buffer and len specified.  Unfortunately,
gloss32::read has already eaten some characters from the rx buffer which are lost forever.
In the end, the application just hangs waiting for the read to be satisfied.

The fix changes the code so that the rx buffer read is not performed unless
the desired number of characters are already there in which case, it reads them all at once.

I have attached a patch for sid/component/gloss/gloss.cxx.  Ok to commit?

-- Jeff J.
Index: gloss.cxx
===================================================================
RCS file: /cvs/src/src/sid/component/gloss/gloss.cxx,v
retrieving revision 1.3
diff -c -p -r1.3 gloss.cxx
*** gloss.cxx	2001/01/08 04:30:42	1.3
--- gloss.cxx	2001/01/16 23:19:57
*************** gloss32::read (int fd, address32 addr, s
*** 1036,1045 ****
  	{
  	  char c;
  
! 	  if (rx_buffer.size() > 0)
  	    {
! 	      c = rx_buffer.front();
! 	      rx_buffer.erase(rx_buffer.begin());
  	    }
  	  else
  	    {
--- 1036,1054 ----
  	{
  	  char c;
  
! 	  // check if we have enough characters to satisfy the request
! 	  // if not, no point in reading since we will end up blocked
! 	  // and the read will be reissued
! 	  if (rx_buffer.size() >= len)
  	    {
! 	      count_read = 0;
! 	      for (int i = 0; i < len; ++i)
! 		{
! 		  c = rx_buffer.front();
! 		  rx_buffer.erase(rx_buffer.begin());
! 		  ++count_read;
! 		  strbuf += c;
! 		}
  	    }
  	  else
  	    {
*************** gloss32::read (int fd, address32 addr, s
*** 1047,1054 ****
  	      errcode = EAGAIN;
  	      return false;
  	    }
- 	  count_read = 1;
- 	  strbuf = c;
  	}
        else
  	{
--- 1056,1061 ----