KProcess Line Mode Patch

[Thiago Macieira]

On Monday 01 December 2008 12:32:08 Ralf Habacker wrote:
> Hi,
>
> based on the work described on
> http://lists.kde.org/?l=kde-core-devel&m=120522889930459&w=2 there is a
> patch appended which implements a line buffering mode directly in KProcess.
> The advantage of this approach is that this feature is directly
> available from kdecore and therefore no additional class located
> somewhere in the related package source is required.
>
>  How it works:
>
> 1.Line buffering mode ist enabled with the
>
>     void setOutputLineMode(bool mode);
>
> 2.There are new KProcess signals available which indicates available lines.
>
>     void readyReadLineStandardOutput();
>     void readyReadLineStandardError();
>
> 3. The lines could be fetched with
>
>     QStringList &readLinesStandardOutput(QStringList &out=QStringList());
>     QStringList &readLinesStandardError(QStringList &out=QStringList());
>
> 4. non standard line delimiters could be set by
>
>     void setOutputLineDelimiter(QString &delim=QString());
>
> Any comments or objections ?

Yes, I object :-)

A few problems with your approach:

1) you're using QByteArray for storing the buffer. That's incredibly 
inefficient for large buffers. Imagine that the child program is writing 1MB 
lines: that will cause a lot of memory reallocations and memcpy.

Please use peek() from QIODevice to determine if there's a newline in the 
buffer. You could/should also restrict the length of the search for newline. 
By doing this, you let QIODevice / QProcess manage the buffer, which it does 
using the more-efficient QRingBuffer class (not public)

If you choose not to do this, then I recommend you only read the first line. 
There's no need to keep reading in _k_receivedStdout and _k_receivedStderr if 
you already have one line read.

2) Note that, with your patch, then you *cannot* use the normal 
QProcess/KProcess functions read(), readLine(), peek(), etc. because you 
drained the buffers. KProcess does not override the QProcess virtuals 
readData() and writeData(), meaning those still operate on QProcess buffers.

In fact, with your patch, you're restricted to the new line mode. The non-line 
mode will not work.

3) The API you presented:

    QStringList &readLinesStandardOutput(QStringList &out=QStringList());
    QStringList &readLinesStandardError(QStringList &out=QStringList());

is IMHO awful. It has no parallel in Qt or KDE core classes. It violates the 
Library Coding Policy by returning a reference. And it takes a reference as a 
parameter, which has a default value(!). Worse yet, you return that temporary 
as a non-const reference. 

I think the compiler doesn't even let you write:
	QStringList &out = QStringList();

See:
$ echo 'class Foo {}; int main() { Foo &foo = Foo(); }' | \
pipe> /usr/bin/g++ -fsyntax-only -xc++ -
<stdin>: In function ‘int main()’:
<stdin>:1: error: invalid initialization of non-const reference of type ‘Foo&’ 
from a temporary of type ‘Foo’

Please use QString returns and read one line at a time. That will parallel the 
readLine() function that is already there, by extending it to match what the 
readAll()/readAllStandardError()/readAllStandardOutput() do. That would 
require you to have a canReadLineStandardError and canReadLineStandardOutput() 
already.

4) Your signals emit only once. The readyRead(), readyReadStandardOutput() and  
and readyReadStandardError() signals emit whenever there's new data. 

If you choose to continue with this, your code should emit the signals 
whenever there's a new newline. But I don't see a way of efficiently 
implementing this without direct access to QProcess's buffers -- which 
QProcess won't give you.

5) Finally, I'm not convinced of the need for this. Why can't people call 
canReadLine() when readyRead() is emitted?

What I'd recommend instead is a line-based reader that works on top of a plain 
QIODevice. That way, you can plug it on a QTcpSocket, QLocalSocket, 
QNetworkReply or KTcpSocket objects as well. (the finished() signal is 
replaced by the readChannelFinished() signal)

This would have the issue, of course, that it doesn't handle a multi-channel 
device like QProcess.

-- 
Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org
  Senior Software Engineer - Nokia, Qt Software
  Qt Software is hiring - ask me
      PGP/GPG: 0x6EF45358; fingerprint:
      E067 918B B660 DBD1 105C  966C 33F5 F005 6EF4 5358
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part.
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20081201/56e5dcf1/attachment.sig>