Re: Review Request: Workaround for the hang (freeze) when opening VLC's file dialog under KDE...
---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/100539/#review1190
---
Please also add the { } around single line blocks (as per kdelibs coding style).
kdecore/services/kmimetyperepository.cpp
http://git.reviewboard.kde.org/r/100539/#comment1004
QStringList paths;
const QByteArray pkgConfigPath = qgetenv(PKG_CONFIG_PATH);
if (!pkgConfigPath.isEmpty())
paths QFile::decodeName(pkgConfigPath).split(QLatin1Char(':'),
QString::SkipEmptyParts);
paths QLatin1String(/usr/share/pkgconfig);
- Pino
On Feb. 4, 2011, 1:29 a.m., Dawit Alemayehu wrote:
---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/100539/
---
(Updated Feb. 4, 2011, 1:29 a.m.)
Review request for kdelibs and David Faure.
Summary
---
The attached patch is a workaround to the much discussed issue with VLC
hanging when opening a KDE file dialog. For the details about the causes of
this bug, see http://lists.kde.org/?t=12957244751r=1w=2 and the bug
report linked above...
This addresses bug 260719.
http://bugs.kde.org/show_bug.cgi?id=260719
Diffs
-
kdecore/services/kmimetyperepository.cpp 9f4c3ca
Diff: http://git.reviewboard.kde.org/r/100539/diff
Testing
---
Thanks,
Dawit
Re: Initial merge (was: Re: Merge or Cherry-Pick?)
On Friday, 4 de February de 2011 08:47:08 Johannes Sixt wrote: I can offer a set of git bundles that contain the merge results. (I don't have push access yet.) Anyone interested? Send to me. By the way, for the KDE readers here: git mergetool running kdiff3 is very useful too. Select the Local side usually. (that's C I think) -- Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org Senior Product Manager - Nokia, Qt Development Frameworks PGP/GPG: 0x6EF45358; fingerprint: E067 918B B660 DBD1 105C 966C 33F5 F005 6EF4 5358 signature.asc Description: This is a digitally signed message part.
Re: Review Request: Workaround for the hang (freeze) when opening VLC's file dialog under KDE...
On Feb. 4, 2011, 8:02 a.m., Pino Toscano wrote:
Please also add the { } around single line blocks (as per kdelibs coding
style).
Dawit Alemayehu wrote:
Lets us forget about such nitpicks, okay... If such unnecessary coding
styles were to be enforced, there would be so many lines of code that would
need fixing in this very same file.
Well, is writing new code correctly so hard? I'm not asking you to fix all the
other issues, but to just write the new code correctly.
- Pino
---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/100539/#review1190
---
On Feb. 4, 2011, 1:29 a.m., Dawit Alemayehu wrote:
---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/100539/
---
(Updated Feb. 4, 2011, 1:29 a.m.)
Review request for kdelibs and David Faure.
Summary
---
The attached patch is a workaround to the much discussed issue with VLC
hanging when opening a KDE file dialog. For the details about the causes of
this bug, see http://lists.kde.org/?t=12957244751r=1w=2 and the bug
report linked above...
This addresses bug 260719.
http://bugs.kde.org/show_bug.cgi?id=260719
Diffs
-
kdecore/services/kmimetyperepository.cpp 9f4c3ca
Diff: http://git.reviewboard.kde.org/r/100539/diff
Testing
---
Thanks,
Dawit
Re: moving scratch repositories?
On Fri, Feb 4, 2011 at 11:26 PM, Stefan Majewsky stefan.majew...@googlemail.com wrote: On Thu, Feb 3, 2011 at 7:28 PM, Marco Martin notm...@gmail.com wrote: so, what should happen is, everyting in the scratch repo, should become basically a branch of the master branch (in my first two cases runtime) with all the commits (just linear story) of the repo as commits in the branch, in that subfolder. I think it should work to rewrite the original repo into a subdirectory with a git-filter-branch tree filter. From the top of my head, it might look like this: git filter-branch --tree-filter mkdir -p some/sub/directory; mv * some/sub/directory -- master After this step, everything in all commits has been moved into some/sub/directory. The modified master branch of the repo can now be pulled into the main repo as a feature branch to be merged into master/next/stable/4.6/whatever. However, I remember someone saying that this should be coordinated with sysadmins because it generates a high load on the commit hooks. That was me :) As a protection measure, our hooks will prohibit any pushes in excess of 100 new commits, which should hopefully avoid this in the future. However, it appears that Marco's code is new code, so it may need a trip through the Git equivalent of KDE Review first (which requires moving the repository to playground then to KDE Review) Greetings Stefan
Re: Review Request: Workaround for the hang (freeze) when opening VLC's file dialog under KDE...
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/100539/#review1199 --- Still the coding style issues to fix in the new code. kdecore/services/kmimetyperepository.cpp http://git.reviewboard.kde.org/r/100539/#comment1008 Q_OS_UNIX here (and comment updated for Unix) - Pino On Feb. 4, 2011, 10:11 a.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/100539/ --- (Updated Feb. 4, 2011, 10:11 a.m.) Review request for kdelibs and David Faure. Summary --- The attached patch is a workaround to the much discussed issue with VLC hanging when opening a KDE file dialog. For the details about the causes of this bug, see http://lists.kde.org/?t=12957244751r=1w=2 and the bug report linked above... This addresses bug 260719. http://bugs.kde.org/show_bug.cgi?id=260719 Diffs - kdecore/services/kmimetyperepository.cpp 9f4c3ca Diff: http://git.reviewboard.kde.org/r/100539/diff Testing --- Thanks, Dawit
Re: Review Request: Workaround for the hang (freeze) when opening VLC's file dialog under KDE...
On Friday 04 February 2011, Thiago Macieira wrote: Hmm... I don't get it. Isn't the database version saved in the database? Why do we need to search the pkg-config file (which is a development file!) to find out how to use the database? If this is required, it sounds like shared-mime-info is broken and should be fixed instead. I never got why we needed to run a subprocess in the first place. Good point, I'll post to xdg with a patch for creating a version file. -- David Faure, fa...@kde.org, http://www.davidfaure.fr Sponsored by Nokia to work on KDE, incl. Konqueror (http://www.konqueror.org).
Re: phononserver phonondevicesrc OpenFlag, askToRemove
On Thursday 06 January 2011, Edgar Fuß wrote: Would it be better to have some configuration option in a phononserverrc not to complain about vanishing sound cards? Yes I think this would be a useful configuration option. -- David Faure, fa...@kde.org, http://www.davidfaure.fr Sponsored by Nokia to work on KDE, incl. Konqueror (http://www.konqueror.org).
Re: irc meeting for kdelibs git workflow
On Friday, February 4, 2011, you wrote: On Tuesday 01 February 2011, Aaron J. Seigo wrote: * 3rd party examples we can learn from: http://public.kitware.com/Wiki/Git/Workflow/Topic Qt? http://wiki.videolan.org/Git thanks; added to http://community.kde.org/20110213_GitWorkflowAgenda (feel free to add things yourselves as well :) -- Aaron J. Seigo humru othro a kohnu se GPG Fingerprint: 8B8B 2209 0C6F 7C47 B1EA EE75 D6B7 2EB1 A7F1 DB43 KDE core developer sponsored by Qt Development Frameworks signature.asc Description: This is a digitally signed message part.
Re: Review Request: New KIO::http_post and KIO::StoredHttpPost APIs that accept a QIODevice as input...
Is QIODevice the best idea to use as source? Since we are talking KIO, I believe we can espect the user of KIO::http_post to be using KIO and not Qt IO. So would it instead be possible to make the source a KIO job or KUrl? Regards `Allan On Wednesday 02 February 2011, Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/100516/ --- Review request for kdelibs. Summary --- The attached patch is the first portion a set of patches to make uploading data through HTTP more efficient without affecting the existing implementation. Right now the amount of memory consumed when uploading large files through http or webdav is really not acceptable because only a QByteArray based API is available. That means if you want to upload a file of say 50 or 100 MB to a server, then you have to read the entire thing first before you can call KIO::http_post! This addresses bug 34578. http://bugs.kde.org/show_bug.cgi?id=34578 Diffs - kio/kio/jobclasses.h e9bd191 kio/kio/job_p.h daac895 kio/kio/job.cpp 7d4a849 kio/kio/job.h 632dfc8 Diff: http://git.reviewboard.kde.org/r/100516/diff Testing --- Used by changing kdewebkit to use the new API. Thanks, Dawit
Re: Initial merge (was: Re: Merge or Cherry-Pick?)
On 04.02.11 08:47:08, Johannes Sixt wrote: Am 2/3/2011 13:04, schrieb Johannes Sixt: Before anybody begins to work in this way, someone with sufficient knowlege must introduce the first real merge of the 4.6 branch into the master branch. The conflicts must be resolved; or it is possible to punt by using -s ours. As long as this merge did not happen, anyone who wants to use the merge workflow is at a loss, unfortunately. I tried to do the initial merges in kdelibs, kde-runtime, kde-baseapps, and kde-workspace yesterday, and they turned out to be rather simple with a surprisingly trivial result. Fast-forward to 5. below if you want to know what it is. The simplicity results from two assumptions: (1) All back- and forward-porting was complete when SVN went read-only. I'm not sure that assumption is safe. (2) *.desktop and similar files are generated files, i.e., their content is dictated from outside the repository. And that one is certainly not safe, unless you're solely talking about the translated parts in them. Not everything in a .desktop file is generated. I'm sure you know that already, just wanted to point that out in case somebody stumbles over this in the future. Andreas -- Make a wish, it might come true.
Re: Initial merge (was: Re: Merge or Cherry-Pick?)
On Fri, Feb 04, 2011 at 08:47:08AM +0100, Johannes Sixt wrote: (e) There is no (e). as far as i'm concerned, there isn't even an (a). i said about a year ago and repeated it later that *if* we wanted a forward-merge based process, we would have to do the git migration *before* branching off the next stable branch. that ship has sailed - we are in our old cherry-picking process now, and merging will just make the history an utter mess (but then, i'm sure everybody loves seeing every commit twice - right?). and i'm strongly opposed to merging any previous branches back to master with -s ours, as this will effectively rewrite history (we did *not* merge back, and claiming that now will possibly hide actual omissions).
Re: Review Request: New KIO::http_post and KIO::StoredHttpPost APIs that accept a QIODevice as input...
Ahh... I think you misunderstood the purpose of the patch or rather the title of this review. The new APIs simply overload the existing http post APIs such that the data you are going to post is sent through a QIODevice (QFile or QBuffer) rather than a QByteArray. To be clear here is the new overloaded API: KIO_EXPORT TransferJob *http_post( const KUrl url, QIODevice* device, JobFlags flags = DefaultFlags ); compared to the existing one KIO_EXPORT TransferJob *http_post( const KUrl url, const QByteArray _staticData, JobFlags flags = DefaultFlags ); The flaws of the old API are very obvious and have been an issue since ages. We got a 9 year old bug report about it. :( Passing the data to be uploaded as a QByteArray is fine for small post data like this repose I am writing in a web mail client. However, the memory usage it will incur when normal users attempt to upload today's multimedia objects (music, videos, and even some image files) is really not acceptable. That is what the new API is trying to address. Hope that clarifies it... On Fri, Feb 4, 2011 at 7:12 AM, Allan Sandfeld Jensen k...@carewolf.com wrote: Is QIODevice the best idea to use as source? Since we are talking KIO, I believe we can espect the user of KIO::http_post to be using KIO and not Qt IO. So would it instead be possible to make the source a KIO job or KUrl? Regards `Allan On Wednesday 02 February 2011, Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/100516/ --- Review request for kdelibs. Summary --- The attached patch is the first portion a set of patches to make uploading data through HTTP more efficient without affecting the existing implementation. Right now the amount of memory consumed when uploading large files through http or webdav is really not acceptable because only a QByteArray based API is available. That means if you want to upload a file of say 50 or 100 MB to a server, then you have to read the entire thing first before you can call KIO::http_post! This addresses bug 34578. http://bugs.kde.org/show_bug.cgi?id=34578 Diffs - kio/kio/jobclasses.h e9bd191 kio/kio/job_p.h daac895 kio/kio/job.cpp 7d4a849 kio/kio/job.h 632dfc8 Diff: http://git.reviewboard.kde.org/r/100516/diff Testing --- Used by changing kdewebkit to use the new API. Thanks, Dawit
Re: Review Request: Workaround for the hang (freeze) when opening VLC's file dialog under KDE...
On Feb. 4, 2011, 10:54 a.m., Pino Toscano wrote: Still the coding style issues to fix in the new code. Sorry, but I do not see that as an issue... On Feb. 4, 2011, 10:54 a.m., Pino Toscano wrote: kdecore/services/kmimetyperepository.cpp, lines 688-689 http://git.reviewboard.kde.org/r/100539/diff/7/?file=8344#file8344line688 Q_OS_UNIX here (and comment updated for Unix) Not really. It is pointless to add a path that is meaningless in *BSD. Anyhow, I have fixed the #ifdef to be more robust. It will now use the Linux arch-independent path for all linux and as a fall back for the remaining unix flavors. - Dawit --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/100539/#review1199 --- On Feb. 4, 2011, 10:11 a.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/100539/ --- (Updated Feb. 4, 2011, 10:11 a.m.) Review request for kdelibs and David Faure. Summary --- The attached patch is a workaround to the much discussed issue with VLC hanging when opening a KDE file dialog. For the details about the causes of this bug, see http://lists.kde.org/?t=12957244751r=1w=2 and the bug report linked above... This addresses bug 260719. http://bugs.kde.org/show_bug.cgi?id=260719 Diffs - kdecore/services/kmimetyperepository.cpp 9f4c3ca Diff: http://git.reviewboard.kde.org/r/100539/diff Testing --- Thanks, Dawit
Re: Review Request: Workaround for the hang (freeze) when opening VLC's file dialog under KDE...
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/100539/ --- (Updated Feb. 4, 2011, 3:52 p.m.) Review request for kdelibs and David Faure. Changes --- Made addPlatformSpecificPkgConfigPaths more robust... Summary --- The attached patch is a workaround to the much discussed issue with VLC hanging when opening a KDE file dialog. For the details about the causes of this bug, see http://lists.kde.org/?t=12957244751r=1w=2 and the bug report linked above... This addresses bug 260719. http://bugs.kde.org/show_bug.cgi?id=260719 Diffs (updated) - kdecore/services/kmimetyperepository.cpp 9f4c3ca Diff: http://git.reviewboard.kde.org/r/100539/diff Testing --- Thanks, Dawit
Re: Review Request: Workaround for the hang (freeze) when opening VLC's file dialog under KDE...
On Feb. 4, 2011, 8:02 a.m., Pino Toscano wrote:
Please also add the { } around single line blocks (as per kdelibs coding
style).
Dawit Alemayehu wrote:
Lets us forget about such nitpicks, okay... If such unnecessary coding
styles were to be enforced, there would be so many lines of code that would
need fixing in this very same file.
Pino Toscano wrote:
Well, is writing new code correctly so hard? I'm not asking you to fix
all the other issues, but to just write the new code correctly.
It is when I have to unnecessarily type more than I have to. Seriously this is
getting to be annoying and I do not mean you personally. These rigid and
brittle coding styles. One project says no braces for single line statements
and another says the complete opposite. I have no problem with some common
sense coding style rules, but please spare me from such nonsense micro
management of each and every line of code, specially since the people working
on kdelibs should in the least be well versed about the pitfalls of using or
not using braces, even for single line statements.
- Dawit
---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/100539/#review1190
---
On Feb. 4, 2011, 10:11 a.m., Dawit Alemayehu wrote:
---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/100539/
---
(Updated Feb. 4, 2011, 10:11 a.m.)
Review request for kdelibs and David Faure.
Summary
---
The attached patch is a workaround to the much discussed issue with VLC
hanging when opening a KDE file dialog. For the details about the causes of
this bug, see http://lists.kde.org/?t=12957244751r=1w=2 and the bug
report linked above...
This addresses bug 260719.
http://bugs.kde.org/show_bug.cgi?id=260719
Diffs
-
kdecore/services/kmimetyperepository.cpp 9f4c3ca
Diff: http://git.reviewboard.kde.org/r/100539/diff
Testing
---
Thanks,
Dawit
Re: Review Request: New KIO::http_post and KIO::StoredHttpPost APIs that accept a QIODevice as input...
On Friday 04 February 2011, Allan Sandfeld Jensen wrote: Is QIODevice the best idea to use as source? I believe we can espect the user of KIO::http_post to be using KIO and not Qt IO I like QIODevice actually, for reading stuff on demand. I use it everywhere in KArchive (KZip, KTar...) and KFilterDev. QIODevice is a pull solution - you can ask for 1MB of data now. Well, at least with buffers and files, not necessarily with sockets :-) The good thing with QIODevice is that it works on top of both memory (QBuffer) and files (QFile), so in kde5 we could even remove the QByteArray overload. And if you want to send bzip2 compressed data you could even insert a KFilterDev to do it on the fly. KIO::Job on the other hand is a push solution - it will tell you when it has something. Well in this case both would work I guess, slotDataReqFromDevice could just send whatever is available, using an intermediate storage for what the underlying get job is emitting. But the only benefit would be being able to upload remote data, not sure if we have a use case for that. -- David Faure, fa...@kde.org, http://www.davidfaure.fr Sponsored by Nokia to work on KDE, incl. Konqueror (http://www.konqueror.org).
Re: Review Request: Workaround for the hang (freeze) when opening VLC's file dialog under KDE...
- Original Message - It is when I have to unnecessarily type more than I have to. Seriously this is getting to be annoying and I do not mean you personally. These rigid and brittle coding styles. One project says no braces for single line statements and another says the complete opposite. I have no problem with some common sense coding style rules, but please spare me from such nonsense micro management of each and every line of code, specially since the people working on kdelibs should in the least be well versed about the pitfalls of using or not using braces, even for single line statements. For new code, we very much like to follow: http://techbase.kde.org/Policies/Kdelibs_Coding_Style Which can be different in other area's within KDE, which I hope one day will not be the case. And before you ask, yes, I very much would like a 'astyle' run on all the code in kdelibs to settle this once and for all. Best, -- Tom Albers
Re: Review Request: Workaround for the hang (freeze) when opening VLC's file dialog under KDE...
On Friday 04 February 2011 17:05:07 Tom Albers wrote: - Original Message - It is when I have to unnecessarily type more than I have to. Seriously this is getting to be annoying and I do not mean you personally. These rigid and brittle coding styles. One project says no braces for single line statements and another says the complete opposite. I have no problem with some common sense coding style rules, but please spare me from such nonsense micro management of each and every line of code, specially since the people working on kdelibs should in the least be well versed about the pitfalls of using or not using braces, even for single line statements. For new code, we very much like to follow: http://techbase.kde.org/Policies/Kdelibs_Coding_Style Which can be different in other area's within KDE, which I hope one day will not be the case. And before you ask, yes, I very much would like a 'astyle' run on all the code in kdelibs to settle this once and for all. Just to show that it matters: the KWin source code got reformatted with the awesome kdelibs-astyle script on Monday. signature.asc Description: This is a digitally signed message part.
Re: Review Request: Workaround for the hang (freeze) when opening VLC's file dialog under KDE...
On 4 February 2011 17:05, Tom Albers t...@kde.org wrote: - Original Message - It is when I have to unnecessarily type more than I have to. Seriously this is getting to be annoying and I do not mean you personally. These rigid and brittle coding styles. One project says no braces for single line statements and another says the complete opposite. I have no problem with some common sense coding style rules, but please spare me from such nonsense micro management of each and every line of code, specially since the people working on kdelibs should in the least be well versed about the pitfalls of using or not using braces, even for single line statements. For new code, we very much like to follow: http://techbase.kde.org/Policies/Kdelibs_Coding_Style Which can be different in other area's within KDE, which I hope one day will not be the case. And before you ask, yes, I very much would like a 'astyle' run on all the code in kdelibs to settle this once and for all. And then how about a .kateconfig file (http://kate-editor.org/2006/02/09/kateconfig-files/) to help 'enforce' this? -- Matt Williams http://milliams.com
Re: Review Request: Workaround for the hang (freeze) when opening VLC's file dialog under KDE...
Em sexta-feira, 4 de fevereiro de 2011, às 05:24:26, Dawit A escreveu:
In the meantime, I've been discussing with Rémi about the issue and he's
not budging from his position that libraries shoul
never use Unix signals. To be
honest, he's right: Unix signals are meant to be used centrally only.
But we need SIGCHLD to be notified that a child exited, so we need a
compromise between apps and libraries.
That is especially true for QProcess. It is used in several place
within kdelibs. If any of those code paths are hit from an app like
VLC, the same issue will manifest itself as well. I am still wondering
whether or not the QtCreator freeze I get to this day is somehow
related to this issue even though I doubt QtCreator does any signal
blocking...
Qt workaround patch.
I will most definitely not add this to 4.7. I will show to other engineers in
the office and we'll consider 4.8.
http://qt.pastebin.com/U6dF8kzd
--
Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org
Senior Product Manager - Nokia, Qt Development Frameworks
PGP/GPG: 0x6EF45358; fingerprint:
E067 918B B660 DBD1 105C 966C 33F5 F005 6EF4 5358
From 3fcabe791c4d6b939e6d43b7fbd4d8b4bb792e0c Mon Sep 17 00:00:00 2001
From: Thiago Macieira thiago.macie...@nokia.com
Date: Fri, 4 Feb 2011 17:39:09 +0100
Subject: [PATCH] Make QProcess on Unix use threads, not SIGCHLD.
This makes each process started spawn a new thread, which
will block on waitpid(2). There's no thread reusal or a
thread pool (we need one thread running per subprocess being
watched).
We really need instead the kernel to give us a file descriptor
that does the job of this thread.
I don't like this solution, as it's wasteful of resources. I'm
working on a solution that won't start the thread unless the
stderr and stdout pipes close (if they were open in the first place)
but the subprocess didn't exit. Those kinds of processes are
rare and I would accept the resource usage.
Reviewed-By: no one yet
---
src/corelib/io/qprocess.cpp |8 +-
src/corelib/io/qprocess_p.h |3 +
src/corelib/io/qprocess_unix.cpp | 249 ++
3 files changed, 48 insertions(+), 212 deletions(-)
diff --git a/src/corelib/io/qprocess.cpp b/src/corelib/io/qprocess.cpp
index 1ce1256..8575f0d 100644
--- a/src/corelib/io/qprocess.cpp
+++ b/src/corelib/io/qprocess.cpp
@@ -757,6 +757,7 @@ QProcessPrivate::QProcessPrivate()
#endif // Q_WS_WIN
#ifdef Q_OS_UNIX
serial = 0;
+unixWaiterThread = 0;
#endif
#ifdef Q_OS_SYMBIAN
symbianProcess = NULL;
@@ -826,14 +827,15 @@ void QProcessPrivate::cleanup()
qDeleteInEventHandler(notifier);
notifier = 0;
}
+#ifdef Q_OS_UNIX
+serial = 0;
+cleanUpWaiterThread();
+#endif
destroyPipe(stdoutChannel.pipe);
destroyPipe(stderrChannel.pipe);
destroyPipe(stdinChannel.pipe);
destroyPipe(childStartedPipe);
destroyPipe(deathPipe);
-#ifdef Q_OS_UNIX
-serial = 0;
-#endif
#ifdef Q_OS_SYMBIAN
if (symbianProcess) {
symbianProcess-Close();
diff --git a/src/corelib/io/qprocess_p.h b/src/corelib/io/qprocess_p.h
index be4f2a0..1b317be 100644
--- a/src/corelib/io/qprocess_p.h
+++ b/src/corelib/io/qprocess_p.h
@@ -76,6 +76,7 @@ QT_BEGIN_NAMESPACE
class QSocketNotifier;
class QWindowsPipeWriter;
class QWinEventNotifier;
+struct QUnixWaiterThread;
class QTimer;
#if defined(Q_OS_SYMBIAN)
class RProcess;
@@ -205,6 +206,7 @@ public:
void startProcess();
#if defined(Q_OS_UNIX) !defined(Q_OS_SYMBIAN)
void execChild(const char *workingDirectory, char **path, char **argv, char **envp);
+void cleanUpWaiterThread();
#endif
bool processStarted();
void terminateProcess();
@@ -225,6 +227,7 @@ public:
QProcess::ExitStatus exitStatus;
bool crashed;
#ifdef Q_OS_UNIX
+QUnixWaiterThread *unixWaiterThread;
int serial;
#endif
diff --git a/src/corelib/io/qprocess_unix.cpp b/src/corelib/io/qprocess_unix.cpp
index 5f53300..b249538 100644
--- a/src/corelib/io/qprocess_unix.cpp
+++ b/src/corelib/io/qprocess_unix.cpp
@@ -105,6 +105,7 @@ QT_END_NAMESPACE
#include errno.h
#include stdlib.h
#include string.h
+#include pthread.h
QT_BEGIN_NAMESPACE
@@ -119,21 +120,6 @@ static inline char *strdup(const char *data)
}
#endif
-static int qt_qprocess_deadChild_pipe[2];
-static struct sigaction qt_sa_old_sigchld_handler;
-static void qt_sa_sigchld_handler(int signum)
-{
-qt_safe_write(qt_qprocess_deadChild_pipe[1], , 1);
-#if defined (QPROCESS_DEBUG)
-fprintf(stderr, *** SIGCHLD\n);
-#endif
-
-// load it as volatile
-void (*oldAction)(int) = ((volatile struct sigaction *)qt_sa_old_sigchld_handler)-sa_handler;
-if (oldAction oldAction != SIG_IGN)
-oldAction(signum);
-}
-
static inline void add_fd(int nfds, int fd, fd_set *fdset)
{
FD_SET(fd, fdset);
@@ -141,187 +127,24 @@ static inline void add_fd(int nfds, int fd, fd_set *fdset)
nfds = fd;
}
Re: Review Request: New KIO::http_post and KIO::StoredHttpPost APIs that accept a QIODevice as input...
Em sexta-feira, 4 de fevereiro de 2011, às 16:52:54, Allan Sandfeld Jensen escreveu: Or to put another way; PUT takes a KUrl to send to and gets the data it sends from a slot. POST is essentially just a PUT with return data, so I would find it most natural if POST mirrored PUT in how it sends data just like it mirrors GET in how it receives it. A PUT-from-GET operation is called copy and we already have that operation: KIO::file_copy. A streaming POST is not a common case, though, because POSTs most often require a form-like transport or, in the case of SOAP, XML. -- Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org Senior Product Manager - Nokia, Qt Development Frameworks PGP/GPG: 0x6EF45358; fingerprint: E067 918B B660 DBD1 105C 966C 33F5 F005 6EF4 5358 signature.asc Description: This is a digitally signed message part.
Re: Initial merge (was: Re: Merge or Cherry-Pick?)
Em sexta-feira, 4 de fevereiro de 2011, às 15:27:49, Oswald Buddenhagen escreveu: and i'm strongly opposed to merging any previous branches back to master with -s ours, as this will effectively rewrite history (we did *not* merge back, and claiming that now will possibly hide actual omissions). I don't think that's the message here. The message of the -s ours merge is that no forward-porting of fixes is necessary, so it's easy to find out what changes need to be forward-merged. -- Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org Senior Product Manager - Nokia, Qt Development Frameworks PGP/GPG: 0x6EF45358; fingerprint: E067 918B B660 DBD1 105C 966C 33F5 F005 6EF4 5358 signature.asc Description: This is a digitally signed message part.
Re: Review Request: Workaround for the hang (freeze) when opening VLC's file dialog under KDE...
On Fri, Feb 04, 2011 at 05:41:16PM +0100, Thiago Macieira wrote:
This makes each process started spawn a new thread, which
will block on waitpid(2).
i have the vague notion that blocking/ignoring sigchld will prevent
wait*() from working on some systems. dunno where i got that from.
I'm working on a solution that won't start the thread unless the
stderr and stdout pipes close (if they were open in the first place)
but the subprocess didn't exit.
that won't work, because the process may spawn children who inherit its
stdio and thus keep it open even after it exits.
void QProcessPrivate::initializeProcessManager()
{
-(void) processManager();
+// Unix no longer has a process manager
neither should windows. = trash?
Re: Review Request: Workaround for the hang (freeze) when opening VLC's file dialog under KDE...
Em sexta-feira, 4 de fevereiro de 2011, às 18:05:09, Oswald Buddenhagen
escreveu:
On Fri, Feb 04, 2011 at 05:41:16PM +0100, Thiago Macieira wrote:
This makes each process started spawn a new thread, which
will block on waitpid(2).
i have the vague notion that blocking/ignoring sigchld will prevent
wait*() from working on some systems. dunno where i got that from.
From the man page:
ECHILD (for waitpid() or waitid()) The process specified by pid
(waitpid()) or idtype and id (waitid()) does not exist or
is not a child of the calling process. (This can happen for
one's own child if the action for SIGCHLD is set to
SIG_IGN. See also the Linux Notes section about threads.)
I'm working on a solution that won't start the thread unless the
stderr and stdout pipes close (if they were open in the first place)
but the subprocess didn't exit.
that won't work, because the process may spawn children who inherit its
stdio and thus keep it open even after it exits.
I thought about that. My work on no-thread-until-pipes-close had already
documented this as a behaviour change.
void QProcessPrivate::initializeProcessManager()
{
-(void) processManager();
+// Unix no longer has a process manager
neither should windows. = trash?
Symbian still does.
--
Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org
Senior Product Manager - Nokia, Qt Development Frameworks
PGP/GPG: 0x6EF45358; fingerprint:
E067 918B B660 DBD1 105C 966C 33F5 F005 6EF4 5358
signature.asc
Description: This is a digitally signed message part.
Re: Review Request: Workaround for the hang (freeze) when opening VLC's file dialog under KDE...
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/100539/ --- (Updated Feb. 4, 2011, 6:22 p.m.) Review request for kdelibs and David Faure. Changes --- Please the coding style enforcers... Summary --- The attached patch is a workaround to the much discussed issue with VLC hanging when opening a KDE file dialog. For the details about the causes of this bug, see http://lists.kde.org/?t=12957244751r=1w=2 and the bug report linked above... This addresses bug 260719. http://bugs.kde.org/show_bug.cgi?id=260719 Diffs (updated) - kdecore/services/kmimetyperepository.cpp 9f4c3ca Diff: http://git.reviewboard.kde.org/r/100539/diff Testing --- Thanks, Dawit
Re: Merge or Cherry-Pick?
Can somone please clarify the proper rule to follow for committing bug fixes as it relates to the KDE 4.6 branch ? The conclusion from the discussion here is rather confusing as to what should be done going forward. Should we still commit new bug fixes to the 4.6 branch first and merge into master which, from all indications here, seems not to be possibe with the current state of the KDE 4.6 branch ? Or it does not matter now so we can go ahead and commit to master and backport to the branch if we so choose ?
Re: Merge or Cherry-Pick?
On Tue, Feb 1, 2011 at 06:29, Johannes Sixt wrote: Am 2/1/2011 10:31, schrieb David Jarvie: On Mon, January 31, 2011 11:27 pm, Thiago Macieira wrote: On Monday, 31 de January de 2011 23:34:39 Arno Rehn wrote: I guess that won't quite work when there are commits specific to 4.6 in the 4.6 branch that shouldn't end up in master. And it clutters history with tons of merges. Then let's solve the problem by not having anything specific to 4.6. If it belongs in the stable release, it belongs in the next stable release too. That's not always true. Some changes *will* be specific to 4.6, because sections of code in master can get rewritten, features added or removed, so the changes won't be applicable there. In such a situation, you make the merge anyway, but you resolve the ensuing conflicts by taking master's state (i.e., it amounts to a merge --ours). You should write in the message of the merge commit that the change made to 4.6 is not relevant on master anymore (and why it is not relevant anymore). Another special and unavoidable case of this will be applications bumping their stable version numbers. It seems weird to have a Updated version to 4.6.2 commit in a 4.7 master, for example, but I guess that the (small) price one pays for mergeable stable branches. Parker
Re: Initial merge (was: Re: Merge or Cherry-Pick?)
Thiago Macieira wrote: On Friday, 4 de February de 2011 15:18:58 Nicolas Alvarez wrote: Johannes Sixt wrote: (1) All back- and forward-porting was complete when SVN went read-only. IMHO this is not a safe assumption. As I said before, I found a missing forward-port in kdeplasma-addons, which is a 'small' repository. I see a higher chance of a missing forward port among the 200+ 4.6 revisions in kdelibs. Those missing forward-ports need to be forward-ported anyway. The merging doesn't change that. Whoever forgot to do forward-ports should be doing it *yesterday*. I know forward-ports should be done anyway and asap. But until they aren't done, we can't do the 4.6 merge. -- Nicolas
Re: Initial merge (was: Re: Merge or Cherry-Pick?)
On Friday, 4 de February de 2011 17:13:31 Nicolas Alvarez wrote: Whoever forgot to do forward-ports should be doing it *yesterday*. I know forward-ports should be done anyway and asap. But until they aren't done, we can't do the 4.6 merge. Sure we can. It doesn't influence in anyway. -- Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org Senior Product Manager - Nokia, Qt Development Frameworks PGP/GPG: 0x6EF45358; fingerprint: E067 918B B660 DBD1 105C 966C 33F5 F005 6EF4 5358 signature.asc Description: This is a digitally signed message part.
Re: Review Request: Workaround for the hang (freeze) when opening VLC's file dialog under KDE...
On Friday, 4 de February de 2011 19:38:51 Oswald Buddenhagen wrote: On Fri, Feb 04, 2011 at 06:28:10PM +0100, Thiago Macieira wrote: Em sexta-feira, 4 de fevereiro de 2011, às 18:17:13, Thiago Macieira escreveu: I thought about that. My work on no-thread-until-pipes-close had already documented this as a behaviour change. Think about it anyway: if the child process dies but the grandchild one doesn't, it will die when the current application exits. not if it doesn't actually care for its stdio, like most gui applications. and as redirecting stdio is the default in qprocess, this would happen rather often. There is another drawback: the child process would be a Zombie until all its children died too. yes, and expected qt signals would not be delivered, so the parent would livelock. i've been through this ith naive implementations. trust me, it just doesn't work. Ok, fair enough. Threading-after-pipes-closed is not an option then. That leaves back again at square one: - one thread per subprocess OR - SIGCHLD handler OR - kernel developers give us some new syscalls like pidfd or waitpidv -- Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org Senior Product Manager - Nokia, Qt Development Frameworks PGP/GPG: 0x6EF45358; fingerprint: E067 918B B660 DBD1 105C 966C 33F5 F005 6EF4 5358 pgpmA0f5CT6Dd.pgp Description: This is a digitally signed message part.
Re: Review Request: Workaround for the hang (freeze) when opening VLC's file dialog under KDE...
On Friday, 4 de February de 2011 14:06:58 Dawit A wrote: BTW, there is one side issue I noticed in QProcess through this whole process. Why does QProcess not exit immediately if I invoke kill or terminate or even when it just timed out from waiting for the child process ? IOW, why wait some more for the child process to exit under those circumstances ? That is what unnecessarily prolongs or even doubles the blocking period. kill() or terminate() just send the signals. There's no guarantee that the process will exit immediately, so why should QProcess notify you of that immediately? This is especially true for SIGTERM, since that one can be caught and handled. -- Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org Senior Product Manager - Nokia, Qt Development Frameworks PGP/GPG: 0x6EF45358; fingerprint: E067 918B B660 DBD1 105C 966C 33F5 F005 6EF4 5358 signature.asc Description: This is a digitally signed message part.
Re: Merge or Cherry-Pick?
On Friday, 4 de February de 2011 14:58:52 Parker Coates wrote: Another special and unavoidable case of this will be applications bumping their stable version numbers. It seems weird to have a Updated version to 4.6.2 commit in a 4.7 master, for example, but I guess that the (small) price one pays for mergeable stable branches. That is the type of commit that *will* conflict when you try to merge. Then you should be undumb and choose the side that says the correct version for the branch. -- Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org Senior Product Manager - Nokia, Qt Development Frameworks PGP/GPG: 0x6EF45358; fingerprint: E067 918B B660 DBD1 105C 966C 33F5 F005 6EF4 5358 signature.asc Description: This is a digitally signed message part.
Re: Review Request: Workaround for the hang (freeze) when opening VLC's file dialog under KDE...
On Friday, 4 de February de 2011 16:35:32 Dawit A wrote: Let us take out terminate from the equation. It is my mistake I included it here because its documentation states that The process may not exit as a result of calling this function. However, I really do not understand why what you stated matters for the kill() and timed out scenarios... QProcess process; process.start(...); If the next command I issue is process.waitForFinished(); I expect it to wait 30 secs since that is what the default value for the timeout parameter states. There is no way I would expect it to block for another 30 sec when the variable goes out of scope because of its own internal implementation issues. It is doing waitForFinished on a child process it created itself. Nothing to do with the API caller. Ok, that sounds like unexpected behaviour. Has this been filed as a bug? The case of the kill is even more baffling to me because its documentation clearly says Kills the current process, causing it to exit immediately. So If I kill is invoked as such process.kill(); what should a reasonable expectation be ? To me the process gets killed immediately. Done, poof, gone. Instead that too blocks for the default amount of period if there are internally forked processes that are still running when the dtor is encountered yet again. A killed process is still not reaped. Subprocesses need to be reaped by their parents to be cleaned up. There is more information that you can extract from a process after death, like CPU time consumed. So the implementation will have to clean up after the child process anyway. Is the problem that you're encountering that kill() doesn't kill the grandchildren processes? But I also don't see why a killed process would cause waitForFinished() to take more than some microseconds to complete. Do you have a testcase? Oh well, perhaps I am simply expecting QProcess to work the way it makes sense to me and am simply missing a larger point which I fail to recognize. -- Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org Senior Product Manager - Nokia, Qt Development Frameworks PGP/GPG: 0x6EF45358; fingerprint: E067 918B B660 DBD1 105C 966C 33F5 F005 6EF4 5358 signature.asc Description: This is a digitally signed message part.
Re: Review Request: Workaround for the hang (freeze) when opening VLC's file dialog under KDE...
On Fri, Feb 4, 2011 at 5:00 PM, Thiago Macieira thi...@kde.org wrote:
On Friday, 4 de February de 2011 16:35:32 Dawit A wrote:
Let us take out terminate from the equation. It is my mistake I
included it here because its documentation states that The process
may not exit as a result of calling this function. However, I really
do not understand why what you stated matters for the kill() and timed
out scenarios...
QProcess process;
process.start(...);
If the next command I issue is
process.waitForFinished();
I expect it to wait 30 secs since that is what the default value for
the timeout parameter states. There is no way I would expect it to
block for another 30 sec when the variable goes out of scope because
of its own internal implementation issues. It is doing waitForFinished
on a child process it created itself. Nothing to do with the API
caller.
Ok, that sounds like unexpected behaviour. Has this been filed as a bug?
Not yet. I simply mentioned it here because you seem to be addressing
the issue that exposed this behavior, but I guess I can open a ticket
against Qt for this.
The case of the kill is even more baffling to me because its
documentation clearly says Kills the current process, causing it to
exit immediately. So If I kill is invoked as such
process.kill();
what should a reasonable expectation be ? To me the process gets
killed immediately. Done, poof, gone. Instead that too blocks for the
default amount of period if there are internally forked processes that
are still running when the dtor is encountered yet again.
A killed process is still not reaped. Subprocesses need to be reaped by their
parents to be cleaned up. There is more information that you can extract from
a process after death, like CPU time consumed. So the implementation will have
to clean up after the child process anyway.
Is the problem that you're encountering that kill() doesn't kill the
grandchildren processes?
But I also don't see why a killed process would cause waitForFinished() to
take more than some microseconds to complete. Do you have a testcase?
Yep, I do. It is the same thing I cobbled together to replicate the
VLC hang bug and posted a while back. I can dust that up and create
bug reports against Qt if you like.
Should I open two separate tickets for each one of these issue
eventhough the cause is the same, i.e. ~QProcess always calling
waitForFinished() so long as d-processState != NoRunning ?
1113QProcess::~QProcess()
1114{
1115Q_D(QProcess);
1116if (d-processState != NotRunning) {
1117qWarning(QProcess: Destroyed while process is still running.);
1118kill();
1119waitForFinished();
1120}
1121#ifdef Q_OS_UNIX
1122// make sure the process manager removes this entry
1123d-findExitCode();
1124#endif
1125d-cleanup();
1126}
Please note that were it not for VLC blocking of the SIGCHLD signal, I
doubt I would have noticed this issue because the condition under
which waitForFinished is called in QProcess' dtor above would probably
never be met ??
Re: Review Request: Workaround for the hang (freeze) when opening VLC's file dialog under KDE...
On Friday, 4 de February de 2011 19:28:06 Nicolás Alvarez wrote: On 04/02/2011, Thiago Macieira thi...@kde.org wrote: Ok, fair enough. Threading-after-pipes-closed is not an option then. That leaves back again at square one: - one thread per subprocess OR - SIGCHLD handler OR - kernel developers give us some new syscalls like pidfd or waitpidv Would signalfd (Linux-specific) be useful for this? No, completely useless. I tried it yesterday. signalfd is as useful as signals themselves: they work if and only if you have a central place of authority for receiving the signal. It requires blocking the reception of that signal in all threads, otherwise behaviour is unpredictable. In turn, since VLC unblocks SIGCHLD in one thread and even sigwaits on it, the signalfd never fires. I threw aw y the code. signalfd is not for libraries. -- Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org Senior Product Manager - Nokia, Qt Development Frameworks PGP/GPG: 0x6EF45358; fingerprint: E067 918B B660 DBD1 105C 966C 33F5 F005 6EF4 5358 pgpGkFdgf4c1s.pgp Description: This is a digitally signed message part.
Re: Review Request: Workaround for the hang (freeze) when opening VLC's file dialog under KDE...
On Friday, 4 de February de 2011 17:35:00 Dawit A wrote: Ok, that sounds like unexpected behaviour. Has this been filed as a bug? Not yet. I simply mentioned it here because you seem to be addressing the issue that exposed this behavior, but I guess I can open a ticket against Qt for this. Well, I thought you meant in a scenario QProcess's internals are still working as expected. The moment that VLC blocked the registration of QProcess's signal handler (this was the source of the issue), all bets are off. Yep, I do. It is the same thing I cobbled together to replicate the VLC hang bug and posted a while back. I can dust that up and create bug reports against Qt if you like. Again that's interfering with SIGCHLD. I need a testcase that doesn't break that functionality. It's perfectly possible for other libraries and the application to use SIGCHLD too without breaking QProcess. VLC doesn't want to play ball with us, so there's nothing we can do, unless we switch to one-thread-per-subprocess solution. Should I open two separate tickets for each one of these issue eventhough the cause is the same, i.e. ~QProcess always calling waitForFinished() so long as d-processState != NoRunning ? No. I thought you meant two different sources. Please note that were it not for VLC blocking of the SIGCHLD signal, I doubt I would have noticed this issue because the condition under which waitForFinished is called in QProcess' dtor above would probably never be met ?? Right. I'm guessing that there is no bug in the code if the SIGCHLD handler is working. -- Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org Senior Product Manager - Nokia, Qt Development Frameworks PGP/GPG: 0x6EF45358; fingerprint: E067 918B B660 DBD1 105C 966C 33F5 F005 6EF4 5358 signature.asc Description: This is a digitally signed message part.
Review Request: Prevent KHTML's adblock filter parser from incorrectly parsing rules with options...
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/100572/ --- Review request for kdelibs. Summary --- This request is created because no one from the KHTML developerment team responded to my post in kfm-devel. Basically this patch prevents the FilterSet::addFilter function in khtml_filter.cpp from incorrectly interpreting a rule with options such as *$image,~image,donottrack as the wildcard matching *. Obviously doing that causes every request processed to be matched and hence blocked. The above rule was extracted from a recent version of Easy Privacy + EasyList filter. Here the relevant snippet: [Adblock Plus 1.1] ! Checksum: BjTRSfga8mRDTYGW3UpIDQ ! EasyPrivacy and EasyList combination subscription ! Last modified: 29 Jan 2011 18:30 UTC ! ! *** easyprivacy.txt *** ! EasyPrivacy - https://easylist.adblockplus.org/ ! Expires: 5 days (update frequency) ! Licence: https://easylist-downloads.adblockplus.org/COPYING ! ! Please report any unblocked tracking or problems ! in the forums (http://forums.lanik.us/) ! or via e-mail (easylist.subscript...@gmail.com). ! !-The X-Do-Not-Track header-! *$image,~image,donottrack RULE THAT CAUSES THE PROBLEM !-General tracking systems-! ! *** easyprivacy/easyprivacy_general.txt *** If you do not have this patch and you activate adblocking with the EasyPrivacy + Easy List filter selected, then the images will be missing when Konqueror's default page (about:konqueror) is shown. Diffs - khtml/khtml_filter.cpp dc3d0f50b Diff: http://git.reviewboard.kde.org/r/100572/diff Testing --- Thanks, Dawit
Re: Review Request: Workaround for the hang (freeze) when opening VLC's file dialog under KDE...
On Fri, Feb 04, 2011 at 04:35:32PM -0500, Dawit Alemayehu wrote:
The case of the kill is even more baffling to me because its
documentation clearly says Kills the current process, causing it to
exit immediately. So If I kill is invoked as such
process.kill();
what should a reasonable expectation be ? To me the process gets
killed immediately. Done, poof, gone.
yes, except it is stuck in D state. happy waiting then.
but even if the kill is successful, the doc doesn't mention anything
about the state of the qprocess object - it refers exclusively to the
process itself. you just interpolated too much into it.
On Fri, Feb 04, 2011 at 05:35:00PM -0500, Dawit Alemayehu wrote:
1113 QProcess::~QProcess()
1114 {
1118 kill();
1119 waitForFinished();
Please note that were it not for VLC blocking of the SIGCHLD signal, I
doubt I would have noticed this issue because the condition under
which waitForFinished is called in QProcess' dtor above would probably
never be met ??
the condition is very often met - each time you delete a running
qprocess. under normal conditions that waitForFinished() will almost
immediately return. you get a double hang because qprocess is already in
an internally inconsistent state, which i wouldn't call a bug in the
above code. you would also get a hang in the D state case, but, uhm,
it's hung - not exactly a normal state, either.
Re: Merge or Cherry-Pick?
On Friday, February 04, 2011 19:16:03 Tom Albers wrote: - Original Message - this will get us used to the merge workflow which starts with fix it in the stable branch first. So, in conclusion, we did not pick a tool which fits our workflow, instead we are now adjusting our workflow to the tool. Adjusting the workflow is not bad per se though. It is bad if the new workflow is inferior or hinders development. I haven't been using any of the git modules long enough to see if that's the case, but I don't see any reason why the proposed new workflow shouldn't actually be an improvement... Regards, - Michael Pyne signature.asc Description: This is a digitally signed message part.
