search

codegooglecom / libproxy

Automatically exported from code.google.com/p/libproxy
GNU Lesser General Public License v2.1
0 stars 0 forks source link

KDE module does not make use of full-featured KConfig system #70

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago 
The KDE module just reads the users kioslaverc, and doesn't make use of
other files on system level.

To solve this issue, we have to use KConfig to get the values. This way, we
get exactly the same behaviour as KDE itself.

A patch was discussed as part of issue 67, but there were problems with
additional output. Finally, it tourned out the the additional output that I
had seen was caused by my own testig application, who was buggy. It seems
that Qt warnings are automatically suppressed in libraries. But to be sure
and not just relay on this assumtion, we can temporally modify the message
handler (the component that Qt uses internally for the output of all it's
messages) to supress all output of Qt in a reliably way.

A new version of the KDE module that implements this is attached.

It introduces a new function:

void dummyMessageHandler(QtMsgType, const char *)

I've noticed that all other functions are "static" and the name starts with
an "_". As I am not sure why, I haven't applied this to the new function.
But maybe this should be done?

Original issue reported on code.google.com by lukas.so...@gmx-topmail.de on 9 Oct 2009 at 10:47

Attachments:

GoogleCodeExporter commented 9 years ago 
I've tested it now another time and this new patch works fine for me. The 
additional
include files don't result in additional dependencies because all of them are 
part of
QtCore or kdecore.

So I think we could try this now...

Original comment by lukas.so...@gmx-topmail.de on 20 Nov 2009 at 7:28

GoogleCodeExporter commented 9 years ago 
Please port this against the new C++ rewrite in trunk (should only take you a 
few
minutes) and provide as a patch.

I'll also need some understanding as to how this actually works.  In libproxy we
*cannot* assume that the app that is running is a KDE/Qt app (it could be 
command
line).  Thus we cannot depend on things like mainloops, etc.  Thus, things like
message handlers make me think there is some type of mainloop going on.  Is 
this the
case?

Original comment by npmccallum@gmail.com on 22 Jan 2010 at 8:39

GoogleCodeExporter commented 9 years ago

Original comment by npmccallum@gmail.com on 22 Jan 2010 at 9:03

GoogleCodeExporter commented 9 years ago 
Attaching a new version, implementing the following:

- Depend on KDE_FULL_ENV for KDE session detection
- use full-featured KConfig system
- apply ignore list only if KDE uses the manual configuration

1.) All used Qt classes are reentrant. For the KDE classes it is not documented 
if
they are reentrant, but I asume that they are. (libproxy must be thread-save,
right?)

2.) The idea with the message handler was to supress the error messages. This 
has
nothing to do with messages between objects or in the event loop, but is just a
better printf() function that Qt (and KDE) uses internally to print it's error
messages. Problem: Qt documentation says that there is only _one_ message 
handler
per application. I'm not sure if it's a good idea to change this in a library.
Furhtermore, the call is not reentrant.

3.) I didn't notice any additional output in the last tests. (It seems that the
additional output that I had reported in the old issue came from my own test
application.) So I've commented out the qt-messagehandler changes. Could you 
test
this?

4.) Libproxy 495 does not compile for me. CMake error in 
libproxy/CMakeLists.txt:95
So I haven't done any testing. When the compile issue is fixed, I'll test it...

Original comment by lukas.so...@gmx-topmail.de on 24 Jan 2010 at 11:50

Attachments:

GoogleCodeExporter commented 9 years ago 
Alright, we're getting closer on this.  Please remove your debugging code and 
submit
as a *patch* against trunk.

1. libproxy is thread-safe, but in a very inefficient way.  I hope to improve 
this in
the future and perhaps provide an async interface.
2. How do we determine how bad our replacement of the message handler is?
3. I'll gladly test once I have a merge-able patch.
4. Please check against the latest trunk.  Also, if you can reproduce the error,
please paste the full cmake error.

Original comment by npmccallum@gmail.com on 24 Jan 2010 at 9:46

GoogleCodeExporter commented 9 years ago 
Configuring with CMake fails still with 507:

-- The C compiler identification is GNU                                         

-- The CXX compiler identification is GNU                                       

-- Check for working C compiler: /usr/bin/gcc                                   

-- Check for working C compiler: /usr/bin/gcc -- works                          

-- Detecting C compiler ABI info                                                

-- Detecting C compiler ABI info - done                                         

-- Check for working CXX compiler: /usr/bin/c++                                 

-- Check for working CXX compiler: /usr/bin/c++ -- works                        

-- Detecting CXX compiler ABI info                                              

-- Detecting CXX compiler ABI info - done                                       

-- checking for modules 'x11;xmu'                                               

--   found x11, version 1.2.2                                                   

--   found xmu, version 1.0.4                                                   

-- checking for modules 'NetworkManager;dbus-1'                                 

--   found NetworkManager, version 0.7.1                                        

--   found dbus-1, version 1.2.16                                               

-- checking for module 'webkit-1.0'                                             

--   package 'webkit-1.0' not found                                             

-- checking for module 'xulrunner-js'                                           

--   package 'xulrunner-js' not found                                           

-- checking for module 'firefox-js'                                             

--   package 'firefox-js' not found                                             

-- checking for module 'mozilla-js'                                             

--   found mozilla-js, version 1.9.1.7                                          

-- checking for module 'gconf-2.0'                                              

--   package 'gconf-2.0' not found                                              

-- Looking for Q_WS_X11                                                         

-- Looking for Q_WS_X11 - found                                                 

-- Looking for Q_WS_WIN                                                         

-- Looking for Q_WS_WIN - not found.                                            

-- Looking for Q_WS_QWS                                                         

-- Looking for Q_WS_QWS - not found.                                            

-- Looking for Q_WS_MAC                                                         

-- Looking for Q_WS_MAC - not found.                                            

-- Found Qt-Version 4.5.3 (using /usr/bin/qmake)                                

-- Looking for XOpenDisplay in
Xmu;Xt;X11;SM;ICE;/usr/lib64/libX11.so;/usr/lib64/libXext.so;/usr/lib64/libXft.s
o;/usr/lib64/libXau.so;/usr/lib64/libXdmcp.so;/usr/lib64/libXpm.so

-- Looking for XOpenDisplay in
Xmu;Xt;X11;SM;ICE;/usr/lib64/libX11.so;/usr/lib64/libXext.so;/usr/lib64/libXft.s
o;/usr/lib64/libXau.so;/usr/lib64/libXdmcp.so;/usr/lib64/libXpm.so
- found                                                                         

-- Looking for gethostbyname                                                    

-- Looking for gethostbyname - found                                            

-- Looking for connect                                                          

-- Looking for connect - found                                                  

-- Looking for remove                                                           

-- Looking for remove - found                                                   

-- Looking for shmat                                                            

-- Looking for shmat - found                                                    

-- Looking for IceConnectionNumber in ICE                                       

-- Looking for IceConnectionNumber in ICE - found                               

-- Found X11: /usr/lib64/libX11.so                                              

-- Looking for include files CMAKE_HAVE_PTHREAD_H                               

-- Looking for include files CMAKE_HAVE_PTHREAD_H - found                       

-- Looking for pthread_create in pthreads                                       

-- Looking for pthread_create in pthreads - not found                           

-- Looking for pthread_create in pthread                                        

-- Looking for pthread_create in pthread - found                                

-- Found Threads: TRUE                                                          

-- Found Automoc4: /usr/bin/automoc4                                            

-- Found Perl: /usr/bin/perl                                                    

-- Phonon Version: 4.3.0                                                        

-- Found Phonon: /usr/lib64/libphonon.so                                        

-- Found Phonon Includes: /usr/include/KDE;/usr/include                         

-- Performing Test _OFFT_IS_64BIT                                               

-- Performing Test _OFFT_IS_64BIT - Success                                     

-- Performing Test HAVE_FPIE_SUPPORT                                            

-- Performing Test HAVE_FPIE_SUPPORT - Success                                  

-- Performing Test __KDE_HAVE_W_OVERLOADED_VIRTUAL                              

-- Performing Test __KDE_HAVE_W_OVERLOADED_VIRTUAL - Success                    

-- Performing Test __KDE_HAVE_GCC_VISIBILITY                                    

-- Performing Test __KDE_HAVE_GCC_VISIBILITY - Success                          

-- Found KDE 4.3 include dir: /usr/include                                      

-- Found KDE 4.3 library dir: /usr/lib64                                        

-- Found the KDE4 kconfig_compiler preprocessor: /usr/bin/kconfig_compiler      

-- Found automoc4: /usr/bin/automoc4                                            

MODULES TO BUILD:                                                               

        m       config_wpad                                                         

        +       config_envvar                                                       

        +       config_file
                config_gnome
        m       config_kde4
        m       network_networkmanager
        m       pacrunner_mozjs
CMake Error at libproxy/CMakeLists.txt:94 (px_module_condmod):
  px_module_condmod Function invoked with incorrect arguments for function
  named: px_module_condmod

        +       ignore_domain
        +       ignore_ip
        +       wpad_dns_alias

-- Found PythonInterp: /usr/bin/python2.6
-- Configuring incomplete, errors occurred!

Original comment by lukas.so...@gmx-topmail.de on 25 Jan 2010 at 12:59

GoogleCodeExporter commented 9 years ago 
Providing the patch (without debugging code). Sorry for inconvenience.

Original comment by lukas.so...@gmx-topmail.de on 25 Jan 2010 at 1:55

Attachments:

GoogleCodeExporter commented 9 years ago 
> 1. libproxy is thread-safe, but in a very
> inefficient way.  I hope to improve this in
> the future and perhaps provide an async interface.

Okay, in this case we must sure to use only reentrant classes. The Qt classes 
in the
patch are all reentrant. I'll try to find out what's about KConfig and 
KConfigGroup...

> 2. How do we determine how bad our replacement
> of the message handler is?

Difficult question. As Qt's documentation says that this is set *once* per
application, it will work fine as far as the application doesn't use threads - 
after
each change of the message handler we restore the old behavior, so there is no 
problem.

But when the application uses threads, this will interfere: When the application
prints error messages in the thread *during* another thread is calling 
libproxy, than
the error messages will be suppressed. Furthermore, the function 
qInstallMsgHandler
is not marked as thread save in Qt's documentation!

I propose to test is we can work without changing the message handler. In my 
tests, I
didn't notice additional output. It seems that the error message of the KConfig
system is not printed when used in a library, but only when used directly in an
application. I don't know why, but when this is reliable, than we don't need the
message handler change...

Original comment by lukas.so...@gmx-topmail.de on 25 Jan 2010 at 2:09

GoogleCodeExporter commented 9 years ago 
Please file a separate bug for the cmake problem.  Also please attach the 
output of
cmake -version.

Original comment by npmccallum@gmail.com on 26 Jan 2010 at 1:14

GoogleCodeExporter commented 9 years ago 
Testing with r508 and the above patch.

It seems that the KDE module is not loaded at all. Not even the function used in
PX_MODULE_LOAD seems to be called?

Original comment by lukas.so...@gmx-topmail.de on 28 Jan 2010 at 10:23

GoogleCodeExporter commented 9 years ago 
I've merged your kde patch into r513.  It builds, however I have not tested its
functionality nor have I cleaned up any of the debug stuff.  Please take a look 
at it.

Original comment by npmccallum@gmail.com on 5 Feb 2010 at 7:02

GoogleCodeExporter commented 9 years ago 
Also of note, I elected not to use the envvar test as the kde module condition. 
 This
is due to the fact that we do not want the user to be able to manually tweak 
this
condition (ie via "unset KDE_FULL_SESSION").

POSIX appears to support a readonly syntax for envvars.  If KDE_FULL_SESSION was
marked as readonly, we could use it (and it would actually be my preferred 
solution)
since the user would be unable to modify it.

Original comment by npmccallum@gmail.com on 5 Feb 2010 at 3:23

GoogleCodeExporter commented 9 years ago 
I've tested r513, with bad result. The error message appears now always when a
non-Qt-application uses libproxy. It is a hassle...

As I don't find better solution, what could be our options?

1.) Use the actual code with error message. This solution is thread-save.

2.) Suppress the error message for non-Qt clients explicitly. (For Qt clients 
it will
not be generated, so no need to suppress this.) This is not 100% thread-save. We
could run into a problem if someone calls libproxy in an independent thread and 
at
the same time instantiates the QApplication object in the main thread. It is 
very
unlikely that someone will construct its application this way, but we can't 
exclude it.

3.) Just drop support for the full-features KConfig system and return to the 
previous
solution. This way, thread-safety can be made sure within libproxy itself.

What will we do?

Original comment by sommer...@gmail.com on 6 Feb 2010 at 10:15

GoogleCodeExporter commented 9 years ago 
#3 is not an option, we want full KConfig support.

What are the repercussions of #1? Just a message printed on stderr?  If 
repercussions
are minimal, I'm inclined to chose this option.

Original comment by npmccallum@gmail.com on 6 Feb 2010 at 6:51

GoogleCodeExporter commented 9 years ago 
Hm, I've passed some time investigating the repercussions of #1 and #2. KDE's 
API
documentation is mostly well done, but doesn't say much about thread-safty. 
However,
after searching a little bit, it looks really bad: KConfig is neither 
thread-safe nor
even reentrant:

http://lists.zerezo.com/kde-devel/msg12619.html
http://lists.kde.org/?l=kde-devel&m=118037451920733&w=2

It may be used only from the main thread. And there is no way arround KConfig 
(if we
don't wont to implement it on our own, which would be difficult).

I would stronly opt for #3. KConfig-support is nice to have, but not having it 
will
make no difference for most users (let's say at least 98%).

Original comment by sommer...@gmail.com on 12 Feb 2010 at 3:56

GoogleCodeExporter commented 9 years ago 
In reply to Comment 12

The check for "kicker" doesn't work because KDE4 doesn't use kicker anymore. So 
the
result is always FALSE.

KDE4 now uses by default Kickoff as menu, but only if the user hasn't changed 
it -
checking for Kickoff would also be not reliable.

Original comment by sommer...@gmail.com on 12 Feb 2010 at 4:01

GoogleCodeExporter commented 9 years ago 
PS: KStandardDirs, which was used in r492 to read the KDE configuration, seems 
to be
thread-safe.
(http://api.kde.org/4.x-api/kdelibs-apidocs/kdecore/html/classKStandardDirs.html
#a0dea59353fd7424bbfefc83764db1536)

Original comment by sommer...@gmail.com on 12 Feb 2010 at 4:02

GoogleCodeExporter commented 9 years ago 
The only way I could image to provide thread-safe KConfig-support would be to 
make a
little helper application that accesses KConfig. This helper application could 
be
invoked in the threaad-safe way from the plugin.

However, it seems to me too much work for little benefit.

Original comment by sommer...@gmail.com on 15 Feb 2010 at 3:21

GoogleCodeExporter commented 9 years ago 
We only need thread safety within a config_extension instance (calling into each
instance will be wrapped in mutexes).  Thus, the only concern is accessing KDE 
globals.

Original comment by npmccallum@gmail.com on 15 Feb 2010 at 3:40

GoogleCodeExporter commented 9 years ago 
Fixed in r579.  In particular, we only do the messageHandler in the 
constructor, so
the risk of anything happening is pretty small.  I'll also make a documentation 
note
about that risk.

Original comment by npmccallum@gmail.com on 25 Feb 2010 at 4:27

GoogleCodeExporter commented 9 years ago 
> Fixed in r579.

Looks fine for me. We can prevent side effects of changed the message handling 
in
threads by an additional check in the constructor. A new constructor is 
proposed in
the attached file.

> In particular, we only do the messageHandler in the
> constructor, so the risk of anything happening is
> pretty small.  I'll also make a documentation note
> about that risk.

Okay, it seems that I have explained me bad (I'm really not good in writing 
english).
The messageHandler is a problem. But the biggest problem is that the classes 
KConfig
and KConfigGroup (following the documentation) may not even be instanciated nor 
used
in a thread if these classes are also instanciated in another thread. (I asume 
they
use static elements that aren't protected by a mutex.) So here is a list of 
possible
crashs:

- Client app uses KConfig in the main or any other thread (which is quite 
probably)
and libproxy in a different thread -> crash because of KConfig

- Client app makes use of qInstallMsgHandler (which may only be used from the 
main
thread) and uses libproxy in a thread. (The patch that I have attached only 
solves
the problem of confusing message handling, not it's thread-safty.)

In consequence: The documentation note should contain something like: "In Qt or 
KDE
based applications, you should use libproxy only from the main thread."

Original comment by sommer...@gmail.com on 27 Feb 2010 at 10:44

Attachments: