Skip to content

Use Qt in wahjamsrv and remove jnetlib #45

Merged
merged 17 commits into from May 27, 2012

3 participants

@stefanha

This patch series converts wahjamsrv and the common core code to use Qt instead of WDL jnetlib. This is necessary in order to switch from a polling architecture to an event-driven architecture that does not consume CPU when there is no work to do.

JNL_Connect is converted to QTcpSocket and JNL_Listen is converted to QTcpServer. The main loop still requires polling but this will be addressed in a future patch series.

stefanha added some commits Apr 4, 2012
@stefanha stefanha Remove platform-specific clients
The Qt client is mature and stable enough for regular use.  Maintaining
the other, platform-specific clients is not feasible since any client
improvement needs to be reimplemented for each client.  It is also
difficult to improve core client code without making code changes to
platform-specific clients and testing them on all platforms.

For these reasons it is now time to remove the platform-specific clients
and rely solely on Qt and PortAudio cross-platform libraries.

Signed-off-by: Stefan Hajnoczi <stefanha@gmail.com>
ae5ee66
@stefanha stefanha Include library header dependencies in WDL/heapbuf.h
Signed-off-by: Stefan Hajnoczi <stefanha@gmail.com>
3adab76
@stefanha stefanha Replace wahjamsrv build with qmake project file
The Qt framework provides a cross-platform build tool called qmake.
qmake simplifies the build process and has support for easily linking
against Qt libraries.

This is the first step to using Qt in wahjamsrv.

Signed-off-by: Stefan Hajnoczi <stefanha@gmail.com>
cec48bf
@stefanha stefanha Add wahjamsrv/Makefile to .gitignore
The wahjamsrv makefile is auto-generated by qmake, do not show it in
git-status(1) output.  Also remove the cursesclient gitignore line since
cursesclient no longer exists.

Signed-off-by: Stefan Hajnoczi <stefanha@gmail.com>
dbe39e3
@stefanha stefanha Use const char* for ninjamsrv logText() fmt string
Using const char* for strings avoids compiler warnings with string
literals.

Signed-off-by: Stefan Hajnoczi <stefanha@gmail.com>
b2c7328
@stefanha stefanha Declare logText() in ninjamsrv.h
Several files use logText() and it's time to declare the function in a
header file.

Signed-off-by: Stefan Hajnoczi <stefanha@gmail.com>
41d56c5
@stefanha stefanha Capture server configuration in a struct
The server uses a number of globals to query the configuration
variables.  This patch introduces the ServerConfig struct and moves
configuration variables into this new struct.

This will make it possible to clean up server state changes.

Signed-off-by: Stefan Hajnoczi <stefanha@gmail.com>
a307721
@stefanha stefanha Unify config loading in wahjamsrv
The configuration file is loaded when wahjamsrv starts.  It can also be
reloaded while the server is running by sending SIGHUP or using the
Windows server console.  This patch introduces the reloadConfig()
function which handles both instances of configuration file loading.

Signed-off-by: Stefan Hajnoczi <stefanha@gmail.com>
ebd045e
@stefanha stefanha Drop wahjamsrv Windows prompt
On Windows there is a administrator prompt which can be used to reload
the config, list users, and kick users.  Drop this feature for now to
make it easier to convert wahjamsrv to Qt.  This sort of feature can be
reintroduced in a cross-platform way again later.

Signed-off-by: Stefan Hajnoczi <stefanha@gmail.com>
181ca9d
@stefanha stefanha Introduce a Server class
Extract the server code into its own class and leave ninjamsrv.cpp with
the main loop, configuration code, and user lookup.

This change is necessary so that an event-driven main loop can be used
instead of the existing polling behavior.

Signed-off-by: Stefan Hajnoczi <stefanha@gmail.com>
9da8e4d
@stefanha stefanha Add QCoreApplication event loop to wahjamsrv
Use QCoreApplication::processEvents() instead of sleeping in the main
loop.  This allows us to begin using Qt events in the server and it will
eventually be possible to block while waiting for events instead of
spinning.

Signed-off-by: Stefan Hajnoczi <stefanha@gmail.com>
608bd06
@stefanha stefanha Convert listener to QTcpServer
Use the Qt QTcpServer class to listen for connections instead of
JNL_Listen.  The advantage of QTcpServer is that it is event-driven and
does not need polling.

Since Net_Connection has not been converted to QTcpSocket yet we dup(2)
the socket file descriptor and then close the QTcpSocket.  The server
still uses JNL_Connection for client connections.  Future patches will
make it possible to use QTcpSocket instead of JNL_Connection.

Signed-off-by: Stefan Hajnoczi <stefanha@gmail.com>
9cf0add
@stefanha stefanha Convert wsprintf() back to sprintf()
The jnetlib library #defined wsprintf to sprintf.  Once we switch to
QTcpSocket and drop jnetlib we will no longer have this macro.  Switch
back to sprintf() for now.

The long term plan for strings is to use QString so that encoding is
handled and the const char* vs char* warnings can be fixed.

Signed-off-by: Stefan Hajnoczi <stefanha@gmail.com>
09f6ec6
@stefanha stefanha Replace JNL_Connection with QTcpSocket
Convert the client connections from JNL_Connection to QTcpSocket.  This
is just the first step and still relies on polling.

The next step is to use signals/slots for event-driven I/O and
disconnect handling.

Note: after disconnect the remote address is no longer available and the
server will print an empty IP address in its log.  This will be fixed
once signals/slots are used for disconnect handling.

Signed-off-by: Stefan Hajnoczi <stefanha@gmail.com>
71503ff
@stefanha stefanha Use QString for strcasecmp() in wahjamsrv
The strcasecmp() (POSIX) and stricmp() (Windows) functions are not
portable.  WDL jnetlib defines macros to make them available on both
platforms.

Since we intend to get rid of WDL jnetlib switch to QString for
case-insensitive string comparison.

Signed-off-by: Stefan Hajnoczi <stefanha@gmail.com>
d7ee24a
@stefanha stefanha Remove WDL jnetlib dependency
Since network I/O now uses Qt's network classes the WDL jnetlib library
is no longer used.  Remove the last dependencies and drop it from the
makefiles.

Note that jnetlib includes system headers so we need to explicitly
include <time.h> now.

Signed-off-by: Stefan Hajnoczi <stefanha@gmail.com>
424b157
@stefanha stefanha Purge WDL jnetlib from the source tree
The WDL jnetlib networking library is no longer used.  This patch
removes it from the source tree.

Signed-off-by: Stefan Hajnoczi <stefanha@gmail.com>
cc5ba22
@teamikl
teamikl commented on 09f6ec6 May 2, 2012

Thread-safety was that ok ?

guidtostr() may not be thread-safe (or up to the implementation depending of sprintf).
I had not get how wahjam use threads (I saw now was only Timers), yet. so this is a question.
as I understand current code now (but not sure yet), its safe uses. that was not multi-thread per connection.

I think, even that was safe in current code uses, However, that may be useful to leave as comment for when architecture changes.


Two same functions can be moved to common file shared from both projects.

There are 2 threads in the client - the Qt main loop and the real-time audio thread. The server does not use threads (explicitly).

I did a quick search and could not find guarantees for sprintf(3) thread-safety but am not worried about it. We are passing no input that is global/static.

@teamikl
teamikl commented on 71503ff May 2, 2012

Net_Connection::Send is thread-safe?

The previous code was just add to send queue, and since they was single thread, did not need lock.

  • QTcpSocket::write is async operation. (QIODevice::write asynchronous is depend on the virtual method implementation)
  • There are keep-alive message send from Timer thread.

http://qt-project.org/doc/qt-4.8/qabstractsocket.html#details
http://qt.gitorious.org/qt/qt/blobs/4.8/src/network/socket/qabstractsocket.cpp#line2268 (writeData implementation)

I think using qt's event (signals/slots) can solve that timing issue. (or using send-queue)
but I have no details ideas yet, I am not figure out all of those changes yet. I still need to read rest of commits.
so I just now leave this review comment what I worry by first reading the code. (sorry if this was pointless)


netmsg.h comment in header is not updated.
in Net_Connection class, Send method has /* -1 on error, i.e. queue full */ but no queue anymore?

and seem return value of Net_Connection::Send() are all ignored.

Net_Connection::Send() was not thread-safe and is still not thread-safe. It's up to the caller to use a lock. The clients do this by locking every time they access the NJClient instance - this prevents the ClientRunThread (or another mechanism for calling NJClient::Run() from a thread) and the Qt main loop from modifying NJClient at the same time. We don't have to worry about thread-safety in Net_Connection.

QTcpSocket::Write() internally buffers data and send it once the socket becomes readable, this is why we don't need a send queue anymore.

In a later commit the timer is replaced using Qt, which makes Net_Connection nicer.

There are keep-alive message send from Timer thread.

Sorry, this was my misunderstand of QTimer implementation.
I worryed Net_Connection::Send was called from another thread.

We don't have to worry about thread-safety in Net_Connection.

Ok

QTcpSocket::Write() internally buffers data and send it once the socket becomes readable, this is why we don't need a send queue anymore.

I did not worry about buffering.
about send queue, It was about synchronization. I think the reason should be it uses Qt's event queue instead.

For now, I agree no need send queue.
For future, send queue with maxsize may be useful to manage write buffer size.

@wahjam wahjam merged commit cc5ba22 into wahjam:master May 27, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.