Code review of networkmanagement/libs/service

[David Edmundson]

I set out to fix "infamous "networkmanagement's kded module crashes
when opening kwallet" bug" that I saw in a blog post, as I quite like
being a crash hunter. However I found myself simply doing a code
review. You may find one of the comments below even fixes the bug.
Even if it doesn't, cleaner code makes bug hunting easier.

I could simply fix all the below, but it's more important to point
everything out so we don't keep making the same mistakes. Nothing
below is meant as a criticism, I've made all these mistakes before and
had them pointed out in code review, now I'm doing the same.

Ping me if you have any questions or disagree with anything.

Review below:

--

Your git logs:

I've seen in several entries the comment "thanks to blah blah for the
patch". Instead commit as "git commit --author 'theirname
<their at emailaddress.com>' ".
It will mark it in as git as "you committed it on behalf of this person".

--

secretstorage.h:56

>From an API POV it's not clear who's reponsible for deleting this object.
It's not a QSharedData, it doesn't take set a QObject* as a parent,
and it doesn't delete itself.

--

secretstorage.h:57

destructors should always be virtual
(it's only needed if someone subclasses your class.. but you never
know who's going to do that, so play safe and do it on all of them)

--

secretstorage.cpp:99

if you have a QObject always use qobject_cast for casting not
static/dynamic cast, it's safer.

Also after casting put in a Q_ASSERT(wallet).
In debug mode this will:
 - check wallet exists
 - "crash" immediately so you know where the bug is
In normal mode this will:
 - no nothing.

So you may as well put in lots of asserts.

--

secretstorage:134, 184, 223, 335

_never_ call "delete myQObject" always "myQObject->deleteLater()".
This deletes it when it next hits the event loop.
It's much safer. KWallet::Wallet is a QObject.

--

networkinterfacemonitor.h:43

destructors should be virtual

--

networkinterfacemonitor.h:52

Why is "dialog" not in the d_ptr? and if it shouldn't be there - it
should be called m_dialog to show it's a member variable.
This is an object that gets created and deleted during the running of
NetworkInterfaceMonitor. Therefore IMHO it should be wrapped in a
QWeakPointer.

--

networkinterfaceonitor.cpp:67
Move to the top. Otherwise you risk calling code that checks dialog
before you've initialised it.

--

NetworkInterfaceMonitor.cpp:166

calling delete on a qobject.

--

networkinterfacemonitor.cpp:169

dialog->exec(). Exec loops always cause lots of problems. In
particular brace yourself for this: http://blogs.kde.org/node/3919

Also this code is really confusing with the two code paths for > 4.6
or not right in the middle of some creating and deleting. It would be
easier to read it if it said:
#if KDE4.6 (all the code) #else (all the other code) #endif even if
it's a bit of duplication.

--

networkinterfacemonitor.cpp:170
"goto"?!?!? You can always rewrite code such that you don't need a
goto. Especially important in code which creates and deletes objects.

--
general:

code consistency with your *'s are a bit off. i.e "(ActivatableList *
list, QObject *parent)". For consistency it should be "ActivatableList
*list". I think this is what the coding standard says too.