[prev in list] [next in list] [prev in thread] [next in thread]
List: kde-hardware-devel
Subject: Re: [Kde-hardware-devel] QtNetworkManager (aka libnm-qt) pending issues
From: "Lamarque V. Souza" <lamarque () kde ! org>
Date: 2013-03-24 23:00:44
Message-ID: 201303242000.44709.lamarque () kde ! org
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
Em Sunday 24 March 2013, Jan Grulich escreveu:
> Hi,
>
> thanks for the first round of review. I've made some changes, see below:
>
> > . NetworkManager::Device::availableConnections() allocate new objects
> > without parent and without warning the user of that method that they
> > should deallocate the memory used. Preferrably you should use a cache
> > mechanism like the one used by NetworkManager::networkInterfaces() to
> > prevent this memory leak.
>
> SOLVED - It's not necessary to create new connections
Yes, but you call NetworkManager::Settings::findConnection() for each
available connection now. We can optimize that.
> > . Use qobject_cast instead of dynamic_cast with QObjects.
>
> There is no dynamic_cast, only static_cast like in Plasma NM. It's not
> possible to use qobject_cast since these classes are not derived from
> QObject.
In examples/createconnection/main.cpp, line 54 you use dynamic_cast in a
WirelessDevice, which derives from QObject.
> > . In examples/createconnection/main.cpp you can use 'result.ToLower()
> > == "yes"' instead of those three comparisons with "yes", "YES" and
> > "Yes". You could also add more comments describing in details what the
> > examples do. I also see that you use
> > Settings::WirelessSetting::setSsid(ssid.toAscii()) there. Is the
> > 'toAscii()' part required for this to work perperly? If so then that
> > should be added to the setSsid() API doc (in
> > settings/802-11-wireless.h). By the way, all class methods in settings
> > directory lack API doc comments, I know, that is not your fault since
> > you copied the classes from Plasma NM. Yet that is one thing we need
> > to fix before the first release.
>
> I'm using "toAscii()" only when I need to convert QString to QByteArray.
> It's because from NetworkManager::AccessPoint you get ssid in QString
> and in NetworkManager::Settings::WirelessSetting::setSsid() you have to
> pass QByteArray. I know that documentation is missing, but I think it's
> not so important for now. By the way, I didn't copy these classes from
> Plasma NM.
"not so important for now", never say that about API documentation :-P
If you had not copied then from Plasma NM then I can say you must (not only
should) fix that :-)
> > . in NetworkManager::Settings::WirelessSecuritySetting::fromMap() I
> > think you should add a 'else { nmDebug() << "Unhandled item comment";
> > }' for each of the if's in there. That would help us detect when
> > NetworkManager adds new possible values in their API.
>
> That's a good idea, I think we should add it also into other classes,
> not only into WirelessSecuritySetting. I'll implement it later, I think
> it's not a blocker for this merge.
Yeah, that's not a blocker.
> > . also in NetworkManager::Settings::WirelessSecuritySetting::fromMap()
> > you insert some QStringList without checking if they are not empty.
> > Usually NetworkManager refuses settings with empty values, which is
> > the case here. You can add some Q_ASSERT() to check for that. This way
> > the check will only be add to the final binary if Qt is compiled with
> > debug enabled.
>
> I think that this problem cannot happen, if you mean QStringLists that
> are created where I put proto, pairwise and group values. I'm checking
> whether these values are empty, if not then there cannot be empty
> QStringList because it's filled with these values.
You are assuming the QVariantMap passed to
WirelessSecuritySetting::fromMap() is always in good state. I am asking you to
add the Q_ASSERT() to detect the case when the QVariantMap is not in good
state. Using Q_ASSERT() ensures that it will not cause runtime overhead if qt
is compiled in release mode.
> Important issues should be fixed. Could you please do another round of
> review? :)
Ok, I will do it when I have some more spare time. By the way, since you
moved the connection-editor from networkmanagement repo to plasma-nm repo that
means you need to make sure Plasma NM stills works with the settings branch of
libnm-qt, if anything breaks then that is a showstopper for the settings
branch. At first I thought you would be updating Plasma NM to work with the
settings branch, is that still your plans?
--
Lamarque V. Souza
KDE's Network Management maintainer
http://planetkde.org/pt-br
[Attachment #5 (text/html)]
<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.0//EN" \
"http://www.w3.org/TR/REC-html40/strict.dtd"> <html><head><meta name="qrichtext" \
content="1" /><style type="text/css"> p, li { white-space: pre-wrap; }
</style></head><body style=" font-family:'Tahoma'; font-size:12pt; font-weight:400; \
font-style:normal;"> <p style=" margin-top:0px; margin-bottom:0px; margin-left:0px; \
margin-right:0px; -qt-block-indent:0; text-indent:0px; -qt-user-state:0;">Em Sunday \
24 March 2013, Jan Grulich escreveu:</p> <p style=" margin-top:0px; \
margin-bottom:0px; margin-left:0px; margin-right:0px; -qt-block-indent:0; \
text-indent:0px; -qt-user-state:0;">> Hi,</p> <p style=" margin-top:0px; \
margin-bottom:0px; margin-left:0px; margin-right:0px; -qt-block-indent:0; \
text-indent:0px; -qt-user-state:0;">> </p> <p style=" margin-top:0px; \
margin-bottom:0px; margin-left:0px; margin-right:0px; -qt-block-indent:0; \
text-indent:0px; -qt-user-state:0;">> thanks for the first round of review. I've \
made some changes, see below:</p> <p style=" margin-top:0px; margin-bottom:0px; \
margin-left:0px; margin-right:0px; -qt-block-indent:0; text-indent:0px; \
-qt-user-state:0;">> </p> <p style=" margin-top:0px; margin-bottom:0px; \
margin-left:0px; margin-right:0px; -qt-block-indent:0; text-indent:0px; \
-qt-user-state:0;">> > . NetworkManager::Device::availableConnections() \
allocate new objects</p> <p style=" margin-top:0px; margin-bottom:0px; \
margin-left:0px; margin-right:0px; -qt-block-indent:0; text-indent:0px; \
-qt-user-state:0;">> > without parent and without warning the user of that \
method that they</p> <p style=" margin-top:0px; margin-bottom:0px; margin-left:0px; \
margin-right:0px; -qt-block-indent:0; text-indent:0px; -qt-user-state:0;">> > \
should deallocate the memory used. Preferrably you should use a cache</p> <p style=" \
margin-top:0px; margin-bottom:0px; margin-left:0px; margin-right:0px; \
-qt-block-indent:0; text-indent:0px; -qt-user-state:0;">> > mechanism like the \
one used by NetworkManager::networkInterfaces() to</p> <p style=" margin-top:0px; \
margin-bottom:0px; margin-left:0px; margin-right:0px; -qt-block-indent:0; \
text-indent:0px; -qt-user-state:0;">> > prevent this memory leak.</p> <p \
style=" margin-top:0px; margin-bottom:0px; margin-left:0px; margin-right:0px; \
-qt-block-indent:0; text-indent:0px; -qt-user-state:0;">> </p> <p style=" \
margin-top:0px; margin-bottom:0px; margin-left:0px; margin-right:0px; \
-qt-block-indent:0; text-indent:0px; -qt-user-state:0;">> SOLVED - It's not \
necessary to create new connections</p> <p style="-qt-paragraph-type:empty; \
margin-top:0px; margin-bottom:0px; margin-left:0px; margin-right:0px; \
-qt-block-indent:0; text-indent:0px; "> </p> <p style=" margin-top:0px; \
margin-bottom:0px; margin-left:0px; margin-right:0px; -qt-block-indent:0; \
text-indent:0px; -qt-user-state:0;"> Yes, but you call \
NetworkManager::Settings::findConnection() for each available connection now. We can \
optimize that.</p> <p style=" margin-top:0px; margin-bottom:0px; margin-left:0px; \
margin-right:0px; -qt-block-indent:0; text-indent:0px; -qt-user-state:0;"> </p> <p \
style=" margin-top:0px; margin-bottom:0px; margin-left:0px; margin-right:0px; \
-qt-block-indent:0; text-indent:0px; -qt-user-state:0;">> > . Use qobject_cast \
instead of dynamic_cast with QObjects.</p> <p style=" margin-top:0px; \
margin-bottom:0px; margin-left:0px; margin-right:0px; -qt-block-indent:0; \
text-indent:0px; -qt-user-state:0;">> </p> <p style=" margin-top:0px; \
margin-bottom:0px; margin-left:0px; margin-right:0px; -qt-block-indent:0; \
text-indent:0px; -qt-user-state:0;">> There is no dynamic_cast, only static_cast \
like in Plasma NM. It's not</p> <p style=" margin-top:0px; margin-bottom:0px; \
margin-left:0px; margin-right:0px; -qt-block-indent:0; text-indent:0px; \
-qt-user-state:0;">> possible to use qobject_cast since these classes are not \
derived from</p> <p style=" margin-top:0px; margin-bottom:0px; margin-left:0px; \
margin-right:0px; -qt-block-indent:0; text-indent:0px; -qt-user-state:0;">> \
QObject.</p> <p style="-qt-paragraph-type:empty; margin-top:0px; margin-bottom:0px; \
margin-left:0px; margin-right:0px; -qt-block-indent:0; text-indent:0px; "> </p> \
<p style=" margin-top:0px; margin-bottom:0px; margin-left:0px; margin-right:0px; \
-qt-block-indent:0; text-indent:0px; -qt-user-state:0;"> In \
examples/createconnection/main.cpp, line 54 you use dynamic_cast in a WirelessDevice, \
which derives from QObject.</p> <p style=" margin-top:0px; margin-bottom:0px; \
margin-left:0px; margin-right:0px; -qt-block-indent:0; text-indent:0px; \
-qt-user-state:0;"> </p> <p style=" margin-top:0px; margin-bottom:0px; \
margin-left:0px; margin-right:0px; -qt-block-indent:0; text-indent:0px; \
-qt-user-state:0;">> > . In examples/createconnection/main.cpp you can use \
'result.ToLower()</p> <p style=" margin-top:0px; margin-bottom:0px; margin-left:0px; \
margin-right:0px; -qt-block-indent:0; text-indent:0px; -qt-user-state:0;">> > \
== "yes"' instead of those three comparisons with "yes", \
"YES" and</p> <p style=" margin-top:0px; margin-bottom:0px; \
margin-left:0px; margin-right:0px; -qt-block-indent:0; text-indent:0px; \
-qt-user-state:0;">> > "Yes". You could also add more comments \
describing in details what the</p> <p style=" margin-top:0px; margin-bottom:0px; \
margin-left:0px; margin-right:0px; -qt-block-indent:0; text-indent:0px; \
-qt-user-state:0;">> > examples do. I also see that you use</p> <p style=" \
margin-top:0px; margin-bottom:0px; margin-left:0px; margin-right:0px; \
-qt-block-indent:0; text-indent:0px; -qt-user-state:0;">> > \
Settings::WirelessSetting::setSsid(ssid.toAscii()) there. Is the</p> <p style=" \
margin-top:0px; margin-bottom:0px; margin-left:0px; margin-right:0px; \
-qt-block-indent:0; text-indent:0px; -qt-user-state:0;">> > 'toAscii()' part \
required for this to work perperly? If so then that</p> <p style=" margin-top:0px; \
margin-bottom:0px; margin-left:0px; margin-right:0px; -qt-block-indent:0; \
text-indent:0px; -qt-user-state:0;">> > should be added to the setSsid() API \
doc (in</p> <p style=" margin-top:0px; margin-bottom:0px; margin-left:0px; \
margin-right:0px; -qt-block-indent:0; text-indent:0px; -qt-user-state:0;">> > \
settings/802-11-wireless.h). By the way, all class methods in settings</p> <p style=" \
margin-top:0px; margin-bottom:0px; margin-left:0px; margin-right:0px; \
-qt-block-indent:0; text-indent:0px; -qt-user-state:0;">> > directory lack API \
doc comments, I know, that is not your fault since</p> <p style=" margin-top:0px; \
margin-bottom:0px; margin-left:0px; margin-right:0px; -qt-block-indent:0; \
text-indent:0px; -qt-user-state:0;">> > you copied the classes from Plasma NM. \
Yet that is one thing we need</p> <p style=" margin-top:0px; margin-bottom:0px; \
margin-left:0px; margin-right:0px; -qt-block-indent:0; text-indent:0px; \
-qt-user-state:0;">> > to fix before the first release.</p> <p style=" \
margin-top:0px; margin-bottom:0px; margin-left:0px; margin-right:0px; \
-qt-block-indent:0; text-indent:0px; -qt-user-state:0;">> </p> <p style=" \
margin-top:0px; margin-bottom:0px; margin-left:0px; margin-right:0px; \
-qt-block-indent:0; text-indent:0px; -qt-user-state:0;">> I'm using \
"toAscii()" only when I need to convert QString to QByteArray.</p> <p \
style=" margin-top:0px; margin-bottom:0px; margin-left:0px; margin-right:0px; \
-qt-block-indent:0; text-indent:0px; -qt-user-state:0;">> It's because from \
NetworkManager::AccessPoint you get ssid in QString</p> <p style=" margin-top:0px; \
margin-bottom:0px; margin-left:0px; margin-right:0px; -qt-block-indent:0; \
text-indent:0px; -qt-user-state:0;">> and in \
NetworkManager::Settings::WirelessSetting::setSsid() you have to</p> <p style=" \
margin-top:0px; margin-bottom:0px; margin-left:0px; margin-right:0px; \
-qt-block-indent:0; text-indent:0px; -qt-user-state:0;">> pass QByteArray. I know \
that documentation is missing, but I think it's</p> <p style=" margin-top:0px; \
margin-bottom:0px; margin-left:0px; margin-right:0px; -qt-block-indent:0; \
text-indent:0px; -qt-user-state:0;">> not so important for now. By the way, I \
didn't copy these classes from</p> <p style=" margin-top:0px; margin-bottom:0px; \
margin-left:0px; margin-right:0px; -qt-block-indent:0; text-indent:0px; \
-qt-user-state:0;">> Plasma NM.</p> <p style="-qt-paragraph-type:empty; \
margin-top:0px; margin-bottom:0px; margin-left:0px; margin-right:0px; \
-qt-block-indent:0; text-indent:0px; "> </p> <p style=" margin-top:0px; \
margin-bottom:0px; margin-left:0px; margin-right:0px; -qt-block-indent:0; \
text-indent:0px; -qt-user-state:0;"> "not so important for now", never say \
that about API documentation :-P If you had not copied then from Plasma NM then I can \
say you must (not only should) fix that :-)</p> <p style=" margin-top:0px; \
margin-bottom:0px; margin-left:0px; margin-right:0px; -qt-block-indent:0; \
text-indent:0px; -qt-user-state:0;"> </p> <p style=" margin-top:0px; \
margin-bottom:0px; margin-left:0px; margin-right:0px; -qt-block-indent:0; \
text-indent:0px; -qt-user-state:0;">> > . in \
NetworkManager::Settings::WirelessSecuritySetting::fromMap() I</p> <p style=" \
margin-top:0px; margin-bottom:0px; margin-left:0px; margin-right:0px; \
-qt-block-indent:0; text-indent:0px; -qt-user-state:0;">> > think you should \
add a 'else { nmDebug() << "Unhandled item comment";</p> <p style=" \
margin-top:0px; margin-bottom:0px; margin-left:0px; margin-right:0px; \
-qt-block-indent:0; text-indent:0px; -qt-user-state:0;">> > }' for each of the \
if's in there. That would help us detect when</p> <p style=" margin-top:0px; \
margin-bottom:0px; margin-left:0px; margin-right:0px; -qt-block-indent:0; \
text-indent:0px; -qt-user-state:0;">> > NetworkManager adds new possible values \
in their API.</p> <p style=" margin-top:0px; margin-bottom:0px; margin-left:0px; \
margin-right:0px; -qt-block-indent:0; text-indent:0px; -qt-user-state:0;">> </p> \
<p style=" margin-top:0px; margin-bottom:0px; margin-left:0px; margin-right:0px; \
-qt-block-indent:0; text-indent:0px; -qt-user-state:0;">> That's a good idea, I \
think we should add it also into other classes,</p> <p style=" margin-top:0px; \
margin-bottom:0px; margin-left:0px; margin-right:0px; -qt-block-indent:0; \
text-indent:0px; -qt-user-state:0;">> not only into WirelessSecuritySetting. I'll \
implement it later, I think</p> <p style=" margin-top:0px; margin-bottom:0px; \
margin-left:0px; margin-right:0px; -qt-block-indent:0; text-indent:0px; \
-qt-user-state:0;">> it's not a blocker for this merge.</p> <p \
style="-qt-paragraph-type:empty; margin-top:0px; margin-bottom:0px; margin-left:0px; \
margin-right:0px; -qt-block-indent:0; text-indent:0px; "> </p> <p style=" \
margin-top:0px; margin-bottom:0px; margin-left:0px; margin-right:0px; \
-qt-block-indent:0; text-indent:0px; -qt-user-state:0;"> Yeah, that's not a \
blocker.</p> <p style=" margin-top:0px; margin-bottom:0px; margin-left:0px; \
margin-right:0px; -qt-block-indent:0; text-indent:0px; -qt-user-state:0;"> </p> <p \
style=" margin-top:0px; margin-bottom:0px; margin-left:0px; margin-right:0px; \
-qt-block-indent:0; text-indent:0px; -qt-user-state:0;">> > . also in \
NetworkManager::Settings::WirelessSecuritySetting::fromMap()</p> <p style=" \
margin-top:0px; margin-bottom:0px; margin-left:0px; margin-right:0px; \
-qt-block-indent:0; text-indent:0px; -qt-user-state:0;">> > you insert some \
QStringList without checking if they are not empty.</p> <p style=" margin-top:0px; \
margin-bottom:0px; margin-left:0px; margin-right:0px; -qt-block-indent:0; \
text-indent:0px; -qt-user-state:0;">> > Usually NetworkManager refuses settings \
with empty values, which is</p> <p style=" margin-top:0px; margin-bottom:0px; \
margin-left:0px; margin-right:0px; -qt-block-indent:0; text-indent:0px; \
-qt-user-state:0;">> > the case here. You can add some Q_ASSERT() to check for \
that. This way</p> <p style=" margin-top:0px; margin-bottom:0px; margin-left:0px; \
margin-right:0px; -qt-block-indent:0; text-indent:0px; -qt-user-state:0;">> > \
the check will only be add to the final binary if Qt is compiled with</p> <p style=" \
margin-top:0px; margin-bottom:0px; margin-left:0px; margin-right:0px; \
-qt-block-indent:0; text-indent:0px; -qt-user-state:0;">> > debug enabled.</p> \
<p style=" margin-top:0px; margin-bottom:0px; margin-left:0px; margin-right:0px; \
-qt-block-indent:0; text-indent:0px; -qt-user-state:0;">> </p> <p style=" \
margin-top:0px; margin-bottom:0px; margin-left:0px; margin-right:0px; \
-qt-block-indent:0; text-indent:0px; -qt-user-state:0;">> I think that this \
problem cannot happen, if you mean QStringLists that</p> <p style=" margin-top:0px; \
margin-bottom:0px; margin-left:0px; margin-right:0px; -qt-block-indent:0; \
text-indent:0px; -qt-user-state:0;">> are created where I put proto, pairwise and \
group values. I'm checking</p> <p style=" margin-top:0px; margin-bottom:0px; \
margin-left:0px; margin-right:0px; -qt-block-indent:0; text-indent:0px; \
-qt-user-state:0;">> whether these values are empty, if not then there cannot be \
empty</p> <p style=" margin-top:0px; margin-bottom:0px; margin-left:0px; \
margin-right:0px; -qt-block-indent:0; text-indent:0px; -qt-user-state:0;">> \
QStringList because it's filled with these values.</p> <p \
style="-qt-paragraph-type:empty; margin-top:0px; margin-bottom:0px; margin-left:0px; \
margin-right:0px; -qt-block-indent:0; text-indent:0px; "> </p> <p style=" \
margin-top:0px; margin-bottom:0px; margin-left:0px; margin-right:0px; \
-qt-block-indent:0; text-indent:0px; -qt-user-state:0;"> You are assuming the \
QVariantMap passed to WirelessSecuritySetting::fromMap() is always in good state. I \
am asking you to add the Q_ASSERT() to detect the case when the QVariantMap is not in \
good state. Using Q_ASSERT() ensures that it will not cause runtime overhead if qt is \
compiled in release mode.</p> <p style="-qt-paragraph-type:empty; margin-top:0px; \
margin-bottom:0px; margin-left:0px; margin-right:0px; -qt-block-indent:0; \
text-indent:0px; "> </p> <p style=" margin-top:0px; margin-bottom:0px; \
margin-left:0px; margin-right:0px; -qt-block-indent:0; text-indent:0px; \
-qt-user-state:0;">> Important issues should be fixed. Could you please do another \
round of</p> <p style=" margin-top:0px; margin-bottom:0px; margin-left:0px; \
margin-right:0px; -qt-block-indent:0; text-indent:0px; -qt-user-state:0;">> \
review? :)</p> <p style="-qt-paragraph-type:empty; margin-top:0px; margin-bottom:0px; \
margin-left:0px; margin-right:0px; -qt-block-indent:0; text-indent:0px; "> </p> \
<p style=" margin-top:0px; margin-bottom:0px; margin-left:0px; margin-right:0px; \
-qt-block-indent:0; text-indent:0px; -qt-user-state:0;"> Ok, I will do it when I have \
some more spare time. By the way, since you moved the connection-editor from \
networkmanagement repo to plasma-nm repo that means you need to make sure Plasma NM \
stills works with the settings branch of libnm-qt, if anything breaks then that is a \
showstopper for the settings branch. At first I thought you would be updating Plasma \
NM to work with the settings branch, is that still your plans?</p> <p \
style="-qt-paragraph-type:empty; margin-top:0px; margin-bottom:0px; margin-left:0px; \
margin-right:0px; -qt-block-indent:0; text-indent:0px; "> </p> <p style=" \
margin-top:0px; margin-bottom:0px; margin-left:0px; margin-right:0px; \
-qt-block-indent:0; text-indent:0px; -qt-user-state:0;">-- </p> <p style=" \
margin-top:0px; margin-bottom:0px; margin-left:0px; margin-right:0px; \
-qt-block-indent:0; text-indent:0px; -qt-user-state:0;">Lamarque V. Souza</p> <p \
style=" margin-top:0px; margin-bottom:0px; margin-left:0px; margin-right:0px; \
-qt-block-indent:0; text-indent:0px; -qt-user-state:0;">KDE's Network Management \
_______________________________________________
Kde-hardware-devel mailing list
Kde-hardware-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-hardware-devel
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic