[prev in list] [next in list] [prev in thread] [next in thread]
List: konsole-devel
Subject: Re: [Konsole-devel] Review Request: Add process info gathering support on OpenBSD
From: Vadim Zhukov <persgray () gmail ! com>
Date: 2012-10-03 10:14:57
Message-ID: CAMy=nGGqj1zLgLbyRtstwEV6cs+u+7aV2p+qc1YJgNnzjRc+Uw () mail ! gmail ! com
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
03.10.2012 13:57 пользователь "Jekyll Wu" <adaptee@gmail.com> написал:
>
> This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/106129/
>
> Sorry for the so late response. I have been blocked by setting up an
OpenBSD environment for testing this patch. The funny thing is in the same
time I have managed to set up the testing environment for NetBSD and
DragonFly without much trouble :)
M-m-m... Official OpenBSD Ports tree doesn't have (working) KDE 4; I'm
actually working on it. This patch was tested at least by one another
person. I can help you in setting up OpenBSD testing environment, if you
wish.
> Back to this patch. Since I (and probably Kurt also) can't test it now,
the safe way is to prepare a patch which does not contain any modification
to existing code/class. So please use a separate class for OpenBSD.
Acknowledged. I'll send new revision ASAP.
> Also, leave alone that fix for allocating kinfo_proc in the wrong unit in
class FreeBSDProcessInfo. I will try to fix it in a separate commit.
Okay.
> Beside that, there are some minor coding style issues.
Will fix them too. Sorry for C-style, it's much easier to follow one manual
(OpenBSD) than for (OpenBSD, Qt and KDE). :)
> src/ProcessInfo.cpp (Diff revision 2)
>
> private:
>
> 681
>
> void *buf = NULL, *nbuf;
>
> konsole code generally uses the form of
>
> void* ptr = ...;
>
> Also, it is preferred to have only one declaration per line.
>
> There are similar issues in the following code.
>
>
> src/ProcessInfo.cpp (Diff revision 2)
>
> private:
>
> 719
>
> for (char **p = argv; *p != NULL; p++)
>
> konsole code generally uses the kdelibs style:
>
> for (....) {
>
> }
>
>
> src/ProcessInfo.cpp (Diff revision 2)
>
> private:
>
> 754
>
> envp = readProcArgs(aPid, KERN_PROC_ENV);
>
> This part feels so "C", which makes it out of place among other "C++"
code. Also applies to the readProcArgs method above.
>
> But that is just my taste. If you feel uncertain about making it more
"C++" like, just ignore this issue.
>
>
> - Jekyll
>
>
> On August 23rd, 2012, 7:29 p.m., Vadim Zhukov wrote:
>
> Review request for Konsole and Jekyll Wu.
> By Vadim Zhukov.
>
> Updated Aug. 23, 2012, 7:29 p.m.
>
> Description
>
> ATM Konsole does not support gathering process information on OpenBSD.
I've a patch adding such support.
> The main idea is to extend already existing FreeBSDProcessInfo class. I
renamed it to a BSDProcessInfo and added a few #ifs here and there. If
that's not desirable, I could create separate OpenBSDProcessInfo class.
>
> I've also fixed a few nits regarding calling C routines (prepended "::"
to sysctl() and such; added kWarning() printouts if they fail). If needed I
can do them as a separate diff.
>
> Testing
>
> On OpenBSD-CURRENT.
>
> Diffs
> src/ProcessInfo.cpp (32c86b1)
>
> View Diff
[Attachment #5 (text/html)]
<p>03.10.2012 13:57 пользователь "Jekyll Wu" <<a \
href="mailto:adaptee@gmail.com">adaptee@gmail.com</a>> написал:<br> \
><br> > This is an automatically generated e-mail. To reply, visit: <a \
href="http://git.reviewboard.kde.org/r/106129/">http://git.reviewboard.kde.org/r/106129/</a><br>
><br>
> Sorry for the so late response. I have been blocked by setting up an OpenBSD \
environment for testing this patch. The funny thing is in the same time I have \
managed to set up the testing environment for NetBSD and DragonFly without much \
trouble :)</p>
<p>M-m-m... Official OpenBSD Ports tree doesn't have (working) KDE 4; I'm \
actually working on it. This patch was tested at least by one another person. I can \
help you in setting up OpenBSD testing environment, if you wish.</p>
<p>> Back to this patch. Since I (and probably Kurt also) can't test it now, \
the safe way is to prepare a patch which does not contain any modification to \
existing code/class. So please use a separate class for OpenBSD.</p>
<p>Acknowledged. I'll send new revision ASAP.</p>
<p>> Also, leave alone that fix for allocating kinfo_proc in the wrong unit in \
class FreeBSDProcessInfo. I will try to fix it in a separate commit.</p> <p>Okay.</p>
<p>> Beside that, there are some minor coding style issues. </p>
<p>Will fix them too. Sorry for C-style, it's much easier to follow one manual \
(OpenBSD) than for (OpenBSD, Qt and KDE). :)</p> <p>> src/ProcessInfo.cpp (Diff \
revision 2)<br> ><br>
> private:<br>
><br>
> 681<br>
><br>
> void *buf = NULL, *nbuf;<br>
><br>
> konsole code generally uses the form of <br>
><br>
> void* ptr = ...;<br>
><br>
> Also, it is preferred to have only one declaration per line.<br>
><br>
> There are similar issues in the following code.<br>
><br>
><br>
> src/ProcessInfo.cpp (Diff revision 2)<br>
><br>
> private:<br>
><br>
> 719<br>
><br>
> for (char **p = argv; *p != NULL; p++)<br>
><br>
> konsole code generally uses the kdelibs style: <br>
><br>
> for (....) {<br>
><br>
> }<br>
><br>
><br>
> src/ProcessInfo.cpp (Diff revision 2)<br>
><br>
> private:<br>
><br>
> 754<br>
><br>
> envp = readProcArgs(aPid, KERN_PROC_ENV);<br>
><br>
> This part feels so "C", which makes it out of place among other \
"C++" code. Also applies to the readProcArgs method above.<br> ><br>
> But that is just my taste. If you feel uncertain about making it more \
"C++" like, just ignore this issue.<br> ><br>
><br>
> - Jekyll<br>
><br>
><br>
> On August 23rd, 2012, 7:29 p.m., Vadim Zhukov wrote:<br>
><br>
> Review request for Konsole and Jekyll Wu.<br>
> By Vadim Zhukov.<br>
><br>
> Updated Aug. 23, 2012, 7:29 p.m.<br>
><br>
> Description<br>
><br>
> ATM Konsole does not support gathering process information on OpenBSD. I've \
a patch adding such support.<br> > The main idea is to extend already existing \
FreeBSDProcessInfo class. I renamed it to a BSDProcessInfo and added a few #ifs here \
and there. If that's not desirable, I could create separate OpenBSDProcessInfo \
class.<br>
><br>
> I've also fixed a few nits regarding calling C routines (prepended \
"::" to sysctl() and such; added kWarning() printouts if they fail). If \
needed I can do them as a separate diff.<br> ><br>
> Testing<br>
><br>
> On OpenBSD-CURRENT.<br>
><br>
> Diffs<br>
> src/ProcessInfo.cpp (32c86b1)<br>
><br>
> View Diff</p>
_______________________________________________
konsole-devel mailing list
konsole-devel@kde.org
https://mail.kde.org/mailman/listinfo/konsole-devel
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic