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.

[Lamarque V. Souza]

Post by 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.

Thanks for the help.

Post by David Edmundson
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.

The problem is finding time to fix all the problems in Plasma NM. I am
already aware of several things you pointed out here and there are several
others more that you have not spotted in this e-mail.

Post by David Edmundson
Ping me if you have any questions or disagree with anything.
--
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".

The intention is to thank the person who worked on the patch, not only
say "I commited your patch". Think about this as a stimulus for the person to
keep sending more patches.

Post by David Edmundson
--
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.

The object should survive as long as the Plasma NM's kded module is
alive. I will add a parent for it.

Post by David Edmundson
--
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)

Ok.

Post by David Edmundson
--
secretstorage.cpp:99
if you have a QObject always use qobject_cast for casting not
static/dynamic cast, it's safer.

static_cast is fast and qobject_cast implementation basically checks if
it is safe to it. If it is safe then qobject_cast does a static_cast,
otherwise it does a dynamic_cast. The code in there is just doing a shortcut.
I did not write this part of the code, but I do not see why change it.

Post by David Edmundson
Also after casting put in a Q_ASSERT(wallet).
- check wallet exists
- "crash" immediately so you know where the bug is
- no nothing.

Ok.

Post by David Edmundson
So you may as well put in lots of asserts.

Ok.

Post by David Edmundson
--
secretstorage:134, 184, 223, 335
_never_ call "delete myQObject" always "myQObject->deleteLater()".
This deletes it when it next hits the event loop.

I know.

Post by David Edmundson
It's much safer. KWallet::Wallet is a QObject.

It is sometimes safer, most of the time it does not matter which one you
use. I even use deleteLater() in other parts of the code to prevent crashes by
dangling pointers (it's a hack but since I do not have time to properly fix
that I think it is better prevent crashes and do the proper fix when I have
more time). In this particular case the kwallet object is never used after the
delete, so it does not matter if I use delete or deleteLater().

Post by David Edmundson
--
networkinterfacemonitor.h:43
destructors should be virtual

Ok.

Post by David Edmundson
--
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.

Ok, I can fix that.

Post by David Edmundson
This is an object that gets created and deleted during the running of
NetworkInterfaceMonitor. Therefore IMHO it should be wrapped in a
QWeakPointer.

Ok.

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

I think you mixed networkInterfaceAdded with modemInterfaceAdded. The
latter is the one that checks dialog and it already comes after dialog is
initialized, so there is no real problem here.

Post by David Edmundson
--
NetworkInterfaceMonitor.cpp:166
calling delete on a qobject.

Well, qobject is still C++, delete is possible here.

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

Yes, I know that, I read that blog post and already changed most of the
dialog->exec() calls in master branch (not in nm09 branch). This particular
case I have not changed yet because I have not had time, other more important
chages required my attention and this code is not oftenly used, so few people
would be hurt if a crash happens here. When I have more time I am going to
change this (or if someone reports a crash in there, which has never
happened).

Post by David Edmundson
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
#if KDE4.6 (all the code) #else (all the other code) #endif even if
it's a bit of duplication.

I do not agree with that. The only difference between those two code
patchs is that the first creates a watcher object to track when the operation
finishes, that is easy to see by anybody that has used QDBusPendingCallWatcher
before. Who has never used QDBusPendingCallWatcher really needs start using it
to prevent freezes in their programs. For the matter, in this case the watcher
object prefevents freezes in kded and consequently in the whole Plasma
Desktop.

Post by David Edmundson
--
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.

Yes, I can but I not obliged to. In this case the goto makes the code
shorter and does not compromise readability. For anybody used to read Linux
kernel's source code this is a common design pattern too.

Post by David Edmundson
--
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.

Hey, Plasma NM was developed by more than ten people. I am the
maintainer now but I cannot review the whole +20K lines of code that existed
before I started developing Plasma NM for the sake of sticking to the code
style. There are other things more important to do, though sometimes I do push
patches that deals only with code style. For the matter, I try to be
consistent and I ask anybody who submit patches to reviewboard.kde.org to do
the same (not only for patches to Plasma NM).

Thanks for the tips I will try to implement the ones I agree with during
the next weekends.