[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kde-core-devel
Subject:    Re: PtyProcess superflous linebreaks (patch)
From:       Thomas Friedrichsmeier <thomas.friedrichsmeier () ruhr-uni-bochum ! de>
Date:       2007-11-21 14:40:03
Message-ID: 200711211540.09358.thomas.friedrichsmeier () ruhr-uni-bochum ! de
[Download RAW message or body]

[Attachment #2 (multipart/mixed)]


On Wednesday 21 November 2007, Johannes Sixt wrote:
> Thomas Friedrichsmeier schrieb:
> > Another option would be to add a parameter "bool strip_linebreaks=true"
> > to readLines(), but I'm not clear on whether this sort of API change is
> > still acceptable. Ideas?
>
> Add a new (non-virtual, to stay BIC) function readLineOrPiece() that does
> not strip the new-line character?

For what it's worth, here's a patch that adds a "readAll()" function to 
KDESu::PtyProcess. This is then used in waitForChild() to correct the 
terminal output with respect to line breaks.

This patch tries to make sure to *preserve* the current behavior of 
readLine(), even that behavior may not be what the name suggests. It does add 
a few words of clarification to the API docs, however.

If somebody else would like to work on making readLine() do what the name 
suggests, I have no objections to that. But it's not the sort of change I'd 
personally feel comfortable with doing, at this point of the release process 
(I guess the kdesu libs is not heavily used, so it should be possible, but 
would definitely need good testing).

Regardless what happens to readLine(), though, I think having a readAll() 
function is definitely desirable. So - is it ok to commit the current patch?

Regards
Thomas

["process_readall.diff" (text/x-diff)]

Index: process.cpp
===================================================================
--- process.cpp	(revision 739635)
+++ process.cpp	(working copy)
@@ -188,30 +188,17 @@
     return d->env;
 }
 
-/*
- * Read one line of input. The terminal is in canonical mode, so you always
- * read a line at at time, but it's possible to receive multiple lines in
- * one time.
- */
 
-QByteArray PtyProcess::readLine(bool block)
+QByteArray PtyProcess::readAll(bool block)
 {
-    int pos;
     QByteArray ret;
-
     if (!d->m_Inbuf.isEmpty())
     {
-        pos = d->m_Inbuf.indexOf('\n');
-        if (pos == -1)
-        {
-            ret = d->m_Inbuf;
-            d->m_Inbuf.resize(0);
-        } else
-        {
-            ret = d->m_Inbuf.left(pos);
-            d->m_Inbuf = d->m_Inbuf.mid(pos+1);
-        }
-        return ret;
+        // if there is still something in the buffer, we need not block.
+        // we should still try to read any further output, from the fd, though.
+        block = false;
+        ret = d->m_Inbuf;
+        d->m_Inbuf.resize(0);
     }
 
     int flags = fcntl(fd(), F_GETFL);
@@ -245,14 +232,29 @@
             else break;
         }
         if (nbytes == 0)
-            break;        // eof
+            break;        // nothing available / eof
 
         buf[nbytes] = '\000';
-        d->m_Inbuf += buf;
+        ret += buf;
+        break;
+    }
 
+    return ret;
+}
+
+
+QByteArray PtyProcess::readLine(bool block)
+{
+    d->m_Inbuf = readAll(block);
+
+    int pos;
+    QByteArray ret;
+    if (!d->m_Inbuf.isEmpty())
+    {
         pos = d->m_Inbuf.indexOf('\n');
         if (pos == -1)
         {
+            // NOTE: this means we return something even if there in no full line!
             ret = d->m_Inbuf;
             d->m_Inbuf.resize(0);
         } else
@@ -260,7 +262,6 @@
             ret = d->m_Inbuf.left(pos);
             d->m_Inbuf = d->m_Inbuf.mid(pos+1);
         }
-        break;
     }
 
     return ret;
@@ -449,17 +450,24 @@
 
         if (ret)
         {
-            QByteArray line = readLine(false);
-            while (!line.isNull())
+            QByteArray output = readAll(false);
+            while (!output.isNull())
             {
-                if (!m_Exit.isEmpty() && !qstrnicmp(line.constData(), m_Exit, m_Exit.length()))
-                    kill(m_Pid, SIGTERM);
+                if (!m_Exit.isEmpty())
+                {
+                    // match exit string only at line starts
+                    int pos = output.indexOf(m_Exit);
+                    if ((pos >= 0) && ((pos == 0) || (output.at (pos - 1) == '\n')))
+                    {
+                        kill(m_Pid, SIGTERM);
+                    }
+                }
                 if (m_bTerminal)
                 {
-                    fputs(line, stdout);
-                    fputc('\n', stdout);
+                    fputs(output, stdout);
+                    fflush(stdout);
                 }
-                line = readLine(false);
+                output = readAll(false);
             }
         }
 
Index: process.h
===================================================================
--- process.h	(revision 739635)
+++ process.h	(working copy)
@@ -51,13 +51,23 @@
 
     /**
      * Reads a line from the program's standard out. Depending on the @em block
-     * parameter, this call blocks until a single, full line is read.
+     * parameter, this call blocks until something was read.
+     * Note that in some situations this function will return less than a full
+     * line of output, but never more. Newline characters are stripped.
      * @param block Block until a full line is read?
      * @return The output string.
      */
     QByteArray readLine(bool block=true);
 
     /**
+     * Read all available output from the program's standard out.
+     * @param block If no output is in the buffer, should the function block
+     * (else it will return an empty QByteArray)?
+     * @return The output.
+     */
+    QByteArray readAll(bool block=true);
+
+    /**
      * Writes a line of text to the program's standard in.
      * @param line The text to write.
      * @param addNewline Adds a '\n' to the line.

["signature.asc" (application/pgp-signature)]

[prev in list] [next in list] [prev in thread] [next in thread]