Code review of networkmanagement/libs/service

[Lamarque V. Souza]

Em Friday 13 April 2012, David Edmundson escreveu:
> 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.
 
> 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.
 
> 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".

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

	Ok.
 
> So you may as well put in lots of asserts.

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

	I know.

> 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().
 
> --
> 
> networkinterfacemonitor.h:43
> 
> destructors should be virtual

	Ok.
 
> --
> 
> 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.

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

	Ok.
 
> --
> 
> 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.
 
> --
> 
> NetworkInterfaceMonitor.cpp:166
> 
> calling delete on a qobject.

	Well, qobject is still C++, delete is possible here.
 
> --
> 
> 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).
 
> 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.

	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.

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

	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.

-- 
Lamarque V. Souza
KDE's Network Management maintainer
http://planetkde.org/pt-br
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-networkmanager/attachments/20120413/c72cf7b7/attachment-0001.html>