D17737: [CopyJob] Create clones in btrf/xfs mount

[Oswald Buddenhagen]

ossi added a comment.


  In D17737#517736 , @thiago wrote:
  
  > QFile::copy already implements FICLONE, sendfile() and Darwin's clonefile().
  
  
  yes, and an api that is utterly unsuitable for interactive applications. ;)
  
  if you just meant it as a source to steal from, fair enough.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D17737

To: chinmoyr, dfaure, #frameworks, thiago, ossi
Cc: ngraham, bruns, kde-frameworks-devel, LeGast00n, GB_2, michaelh

D11236: [KCrash] Establish socket to allow change of ptracer

[Oswald Buddenhagen]

ossi accepted this revision.
ossi added a comment.
This revision is now accepted and ready to land.


  here too, it should be "ancestor" not "parent" for extra correctness.
  amending the code comment is optional, too.

INLINE COMMENTS

> kcrash.cpp:662
> +// For now that will be DrKonqi, which may ask to transfer the 
> ptrace scope to
> +// a debugger using a socket.
>  #ifndef PR_SET_PTRACER

you could make it more specific by saying "a debugger it is not an ancestor of 
(because it started it via kdeinit or startDetached())"

REPOSITORY
  R285 KCrash

BRANCH
  ptracer

REVISION DETAIL
  https://phabricator.kde.org/D11236

To: croick, #frameworks, ossi
Cc: dfaure, lepagevalleeemmanuel, kde-frameworks-devel, sitter, michaelh, 
ngraham, bruns

D11235: [DrKonqi] Request change of ptrace scope from KCrash

[Oswald Buddenhagen]

ossi accepted this revision.
ossi added a comment.
This revision is now accepted and ready to land.


  note that for extra correctness it should be "ancestor" not "parent".

REPOSITORY
  R871 DrKonqi

BRANCH
  ptracer

REVISION DETAIL
  https://phabricator.kde.org/D11235

To: croick, #plasma_workspaces, #frameworks, ossi
Cc: ossi, lepagevalleeemmanuel, maximilianocuria, adridg, plasma-devel, 
jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D11236: [KCrash] Establish socket to allow change of ptracer

[Oswald Buddenhagen]

ossi added a comment.


  i said "3rd line", not "3rd case". it's about the ancestry; see the other 
change.

REPOSITORY
  R285 KCrash

REVISION DETAIL
  https://phabricator.kde.org/D11236

To: croick, #frameworks, ossi
Cc: dfaure, lepagevalleeemmanuel, kde-frameworks-devel, sitter, michaelh, 
ngraham, bruns

D11236: [KCrash] Establish socket to allow change of ptracer

[Oswald Buddenhagen]

ossi added a comment.


  the 3rd line is still wrong ...

REPOSITORY
  R285 KCrash

REVISION DETAIL
  https://phabricator.kde.org/D11236

To: croick, #frameworks, ossi
Cc: dfaure, lepagevalleeemmanuel, kde-frameworks-devel, sitter, michaelh, 
ngraham, bruns

D11236: [KCrash] Establish socket to allow change of ptracer

[Oswald Buddenhagen]

ossi added a comment.


  right, threads, i didn't think of that. i suppose it might make sense to 
pthread_kill() from within kcrash to freeze the other threads, but that will be 
non-atomic, and i wonder whether it wouldn't have undesirable interactions with 
the attached debugger and/or exiting the process.

REPOSITORY
  R285 KCrash

REVISION DETAIL
  https://phabricator.kde.org/D11236

To: croick, #frameworks, ossi
Cc: dfaure, lepagevalleeemmanuel, kde-frameworks-devel, sitter, michaelh, 
ngraham, bruns

D11236: [KCrash] Establish socket to allow change of ptracer

[Oswald Buddenhagen]

ossi added inline comments.

INLINE COMMENTS

> kcrash.cpp:658
> +// when launching drkonqi. Note that DrKonqi will SIGSTOP this 
> process in the meantime
> +// and only send SIGCONT when it is about to attach a debugger.
>  #ifdef Q_OS_LINUX

ok. but come to think of it, maybe that should be revised - that SIGSTOPping 
sounds a tad paranoid to start with. feel like researching the history of that?

REPOSITORY
  R285 KCrash

REVISION DETAIL
  https://phabricator.kde.org/D11236

To: croick, #frameworks, ossi
Cc: dfaure, lepagevalleeemmanuel, kde-frameworks-devel, sitter, michaelh, 
ngraham, bruns

D11235: [DrKonqi] Request change of ptrace scope from KCrash

[Oswald Buddenhagen]

ossi added a comment.


  In D11235#395021 , @croick wrote:
  
  > What do you mean? The (internal) debugger is a child of the debuggee.
  
  
  yes, that's why you need setPtracer() - if crashtest was *under* gdb, that 
would be unnecessary. ;)
  (i'm still referring to the enumeration of cases, if that wasn't clear.)

INLINE COMMENTS

> ptracer.cpp:63
> +
> +int bytes = 0, r;
> +while (bytes < msize) {

i meant this line ...
(but your change was good, too).

REPOSITORY
  R871 DrKonqi

REVISION DETAIL
  https://phabricator.kde.org/D11235

To: croick, #plasma_workspaces, #frameworks, ossi
Cc: ossi, lepagevalleeemmanuel, maximilianocuria, adridg, plasma-devel, 
jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D11236: [KCrash] Establish socket to allow change of ptracer

[Oswald Buddenhagen]

ossi added a comment.


  commit message inverted here as well.

INLINE COMMENTS

> kcrash.cpp:658
> +// when launching drkonqi. Note that DrKonqi will SIGSTOP this 
> process in the meantime
> +// and only send SIGCONT when it is to be attached by a debugger.
>  #ifdef Q_OS_LINUX

"when it is about to attach a debugger".

REPOSITORY
  R285 KCrash

REVISION DETAIL
  https://phabricator.kde.org/D11236

To: croick, #frameworks, ossi
Cc: dfaure, lepagevalleeemmanuel, kde-frameworks-devel, sitter, michaelh, 
ngraham, bruns

D11235: [DrKonqi] Request change of ptrace scope from KCrash

[Oswald Buddenhagen]

ossi added a comment.


  you got the parent-child ordering wrong in the commit message. ^^

INLINE COMMENTS

> ptracer.cpp:61
> +char msg[msize], rmsg[msize];
> +int bytes = 0, r;
> +sprintf(msg, "%lld", debuggerpid);

this looks stupid, i'd swap the order.

REPOSITORY
  R871 DrKonqi

REVISION DETAIL
  https://phabricator.kde.org/D11235

To: croick, #plasma_workspaces, #frameworks, ossi
Cc: ossi, lepagevalleeemmanuel, maximilianocuria, adridg, plasma-devel, 
jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D11235: [DrKonqi] Request change of ptrace scope from KCrash

[Oswald Buddenhagen]

ossi added a comment.


  will also need to wait for commit message update, like the other change.

INLINE COMMENTS

> ptracer.cpp:92
> +
> +qCWarning(DRKONQI_LOG) << "debugged process did not acknowledge setting 
> ptracer to" << debuggerpid;
> +close(sfd);

not wrong per se, but mildly misleading, as it suggests the problem is on the 
other end, which is not necessarily true, this being a catch-all. "cannot set 
ptracer" would cover all bases (while still not being particularly helpful).

REPOSITORY
  R871 DrKonqi

REVISION DETAIL
  https://phabricator.kde.org/D11235

To: croick, #plasma_workspaces, #frameworks, ossi
Cc: ossi, lepagevalleeemmanuel, maximilianocuria, adridg, plasma-devel, 
jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D11235: [DrKonqi] Request change of ptrace scope from KCrash

[Oswald Buddenhagen]

ossi requested changes to this revision.
ossi added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> ptracer.cpp:65
> +bytes += r;
> +else if (r == -1 && !(errno == EINTR))
> +break;

you really could just use != here ... ^^

> ptracer.cpp:78
> +bytes += r;
> +else if (r == -1 && !(errno == EINTR))
> +break;

ditto

> ptracer.cpp:82
> +if (bytes == msize && memcmp(msg, rmsg, msize) == 0)
> +qCInfo(DRKONQI_LOG) << "ptracer set to" << debuggerpid 
> << "by debugged process";
> +}

a complementary warning if any of the steps goes wrong would seem appropriate.

> crashtest.cpp:139
>  
> -  //start drkonqi directly so that drkonqi's output goes to the console
> +  //start drkonqi directly by default so that drkonqi's output goes to the 
> console
>KCrash::CrashFlags flags = KCrash::AlwaysDirectly;

add comment: this can be disabled to be able to test kcrash's real default 
behavior.

> crashtest.cpp:143
>  flags |= KCrash::AutoRestart;
> +  if (parser.isSet(QStringLiteral("kdeinit")))
> +flags &= ~KCrash::AlwaysDirectly;

move that up, so the AlwaysDirectly stuff is in one block.

REPOSITORY
  R871 DrKonqi

REVISION DETAIL
  https://phabricator.kde.org/D11235

To: croick, #plasma_workspaces, #frameworks, ossi
Cc: ossi, lepagevalleeemmanuel, maximilianocuria, adridg, plasma-devel, 
jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D11236: [KCrash] Establish socket to allow change of ptracer

[Oswald Buddenhagen]

ossi added a comment.


  In D11236#394363 , @croick wrote:
  
  > Actually the point I removed does not seem to be true any longer. I'm 
almost certain, that the `prctrl(PR_SET_PTRACER, ...)` was not required until 
some time ago when starting DrKonqi as a fork. But now the internal backtrace 
is not working without. I cannot tell, when that started (currently using 
kernel 4.20).
  
  
  that sounds suspicious. i don't think the kernel's behavior did changed, and 
the process hierarchy presumably didn't, either. the right is handed down the 
ancestry, and that's irrespective of whether the tracer is a "natural" ancestor 
or the tracee, or was set via prctl(). maybe your sysctl settings simply 
changed?

INLINE COMMENTS

> kcrash.cpp:657
>  // Wait forever until the started process exits. This code path is 
> executed
> -// when launching drkonqi. Note that drkonqi will stop this process 
> in the meantime.
> -if (directly) {
> -//if the process was started directly, use waitpid(), as it's a 
> child...
> -while (waitpid(-1, nullptr, 0) != pid) {}
> -} else {
> +// when launching drkonqi. Note that DrKonqi will SIGSTOP this 
> process in the meantime
> +// and a backtrace will end here.

ok, so now it's claimed to apply to the entire block. but how can that be? 
thepollDrKonqiSocket() couldn't possibly be called then ...

> kcrash.cpp:907
> +r = poll(, 1, 1000); // wait for 1 second for a request by DrKonqi
> +} while (r == -1 && (errno == EINTR || errno == EAGAIN));
> +// only continue if POLLIN event returned

remove eagain here as well.

REPOSITORY
  R285 KCrash

REVISION DETAIL
  https://phabricator.kde.org/D11236

To: croick, #frameworks, ossi
Cc: dfaure, lepagevalleeemmanuel, kde-frameworks-devel, sitter, michaelh, 
ngraham, bruns

D11235: [DrKonqi] Request change of ptrace scope from KCrash

[Oswald Buddenhagen]

ossi requested changes to this revision.
ossi added a comment.
This revision now requires changes to proceed.


  mark handled issues as done here as well.

INLINE COMMENTS

> ptracer.cpp:47
> +
> QStringLiteral("%1/kcrash_%2").arg(QStandardPaths::writableLocation(QStandardPaths::RuntimeLocation))
> +  
> .arg(QCoreApplication::applicationPid());
> +

see complementary patch

> ptracer.cpp:60
> +if (write(sfd, msg, 22) > 0) {
> +fd_set set;
> +FD_ZERO();

cruft

> ptracer.cpp:68
> +if (poll(, 1, 1000) > 0 && (fd.revents & POLLIN) &&
> +read(sfd, rmsg, 22) > 0 &&
> +strncmp(msg, rmsg, 22) == 0) {

i'd compare to 22, because we know that the other side sends that.

> ptracer.cpp:69
> +read(sfd, rmsg, 22) > 0 &&
> +strncmp(msg, rmsg, 22) == 0) {
> +qCInfo(DRKONQI_LOG) << "ptracer set to" << pid << "by 
> debugged process";

i'd use memcmp(), as it's a fixed-size data block.

> ptracer.h:22
> +
> +/** On Linux, tell the process to allow the debugger to attach to itr */
> +void setPtracer(qint64 pid);

-r

> crashtest.cpp:133
>parser.addOption(QCommandLineOption(QStringLiteral("autorestart"), 
> i18n("Automatically restart")));
> +  parser.addOption(QCommandLineOption(QStringLiteral("kdeinit"), i18n("Start 
> DrKonqi using kdeinit")));
>parser.addPositionalArgument(QStringLiteral("type"), i18n("Type of 
> crash."), QStringLiteral("crash|malloc|div0|assert|threads"));

this addition isn't used or explained anywhere for all i can tell.

REPOSITORY
  R871 DrKonqi

REVISION DETAIL
  https://phabricator.kde.org/D11235

To: croick, #plasma_workspaces, #frameworks, ossi
Cc: ossi, lepagevalleeemmanuel, maximilianocuria, adridg, plasma-devel, 
jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D11236: [KCrash] Establish socket to allow change of ptracer

[Oswald Buddenhagen]

ossi requested changes to this revision.
ossi added a comment.
This revision now requires changes to proceed.


  mostly minor issues left.
  
  please explicitly mark all handled issues as done - you'll notice on the way 
that you didn't address some of them.
  
  you adjusted the summary kinda the wrong way around ... but come to think of 
it, i was actually kinda wrong - you indeed need to list all three cases to 
illustrate that neither applies. the ancestry case is special only in the sense 
that it automatically makes the "internal" debugger work (i'd mention that in 
parentheses of that case's bullet point).

INLINE COMMENTS

> kcrash.cpp:668
> +
> QStringLiteral("%1/kcrash_%2").arg(QStandardPaths::writableLocation(QStandardPaths::RuntimeLocation))
> +  .arg(pid));
> +int sockfd = openDrKonqiSocket(socketpath);

not really significant, but imo it's backwards that you let the drkonqi pid 
rather than the kcrash pid determine the socket name.

> kcrash.cpp:674
> +if (directly) {
> +//if the process was started directly, use waitpid(), as 
> it's a child...
> +while ((running = waitpid(pid, nullptr, WNOHANG) != pid) && 
> pollDrKonqiSocket(pid, sockfd) >= 0) {}

please consistently put a space after the // marker. also, stick to the file's 
capitalization style.

> kcrash.cpp:686
> +// Wait forever until the started process exits. This code path is 
> executed
> +// when launching drkonqi. Note that drkonqi will SIGSTOP this 
> process in the meantime.
> +if (running) {

hmm, what about the last sentence? it seems to me that some adjustment 
(possibly just additional comments) is required.

> kcrash.cpp:870
> +{
> +struct sockaddr_un drkonqi_server;
> +

i'd move that down, before the member init.

REPOSITORY
  R285 KCrash

REVISION DETAIL
  https://phabricator.kde.org/D11236

To: croick, #frameworks, ossi
Cc: dfaure, lepagevalleeemmanuel, kde-frameworks-devel, sitter, michaelh, 
ngraham, bruns

D11236: [KCrash] Establish socket to allow change of ptrace scope

[Oswald Buddenhagen]

ossi added a comment.


  In D11236#393789 , @ossi wrote:
  
  > detaching via double fork() "protects" the child process,
  
  
  
  
  > but does *not* break the permission inheritance.
  
  ok, i take that back and claim the opposite - i misinterpreted the kernel's 
task_struct's real_parent member's purpose.
  
  that also means that the kernel doc is mildly misleading ("descendant" 
clearly refers to the current relationship after possible re-parenting, not the 
real ancestry at time of forking), but i suppose one is expected to know the 
exact definitions of the used terminology.

REPOSITORY
  R285 KCrash

REVISION DETAIL
  https://phabricator.kde.org/D11236

To: croick, #frameworks, ossi
Cc: dfaure, lepagevalleeemmanuel, kde-frameworks-devel, sitter, michaelh, 
ngraham, bruns

D11236: [KCrash] Establish socket to allow change of ptrace scope

[Oswald Buddenhagen]

ossi added a comment.


  In D11236#393720 , @croick wrote:
  
  > Actually it's only the internal process that creates the backtraces, that 
is a descendant of DrKonqi. All debuggers that are launched via the "Debug" 
button are processes on their own (to remain alive, even if DrKonqi already 
quit).
  
  
  if by that you're implying that external debuggers are always launched via 
kdeinit, then please put that as the reason in the commit message.
  irrespective of that, the "to remain alive" argument is bogus, because 
detaching via double fork() (as qprocess does) "protects" the child process, 
but does *not* break the permission inheritance.

REPOSITORY
  R285 KCrash

REVISION DETAIL
  https://phabricator.kde.org/D11236

To: croick, #frameworks, ossi
Cc: dfaure, lepagevalleeemmanuel, kde-frameworks-devel, sitter, michaelh, 
ngraham, bruns

D11235: [DrKonqi] Request change of ptrace scope from KCrash

[Oswald Buddenhagen]

ossi requested changes to this revision.
ossi added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> debuggerlaunchers.cpp:57
>  if ( pid > 0 ) {
> +queryPtrace(pid);
>  m_monitor->startMonitoring(pid);

according to my reading of the documentation 
(https://www.kernel.org/doc/Documentation/security/Yama.txt) and the kernel 
source, this is unnecessary in this branch, as descendants of the assigned 
ptracer have the right as well (that's why the "normal" backtrace creation 
works without your patch), and not even detaching breaks the chain.

> queryptrace.cpp:67
> +
> +if (select(sfd + 1, , NULL, NULL, ) > 0 &&
> +read(sfd, msg, 3) > 0 && strcmp(msg, "OK") == 0) {

use poll() here, too.

> queryptrace.cpp:68
> +if (select(sfd + 1, , NULL, NULL, ) > 0 &&
> +read(sfd, msg, 3) > 0 && strcmp(msg, "OK") == 0) {
> +qCInfo(DRKONQI_LOG) << "ptrace granted by debugged process";

the current version of the complementary patch just echoes back the pid, so 
this cannot possibly work.

> croick wrote in queryptrace.cpp:26
> QTemporaryDir will not work, since the location is set in KCrash.

the location is still bogus, though. see comment on the other diff.

> queryptrace.h:22
> +
> +/** In Linux try to make the process attach to the debugger */
> +void queryPtrace(qint64 pid);

no. "on linux, tell the process to allow the debugger to attach to it".

> queryptrace.h:23
> +/** In Linux try to make the process attach to the debugger */
> +void queryPtrace(qint64 pid);
> +

that's a rather misleading function name. setPtracer() would reflect the 
purpose and upstream naming.

> croick wrote in crashtest.cpp:138
> That reasoning does not seem to be true. The output can still be read when 
> started "normally".

only if kdeinit was started from the same console, which you cannot assume.

also, this change wouldn't belong into this patch anyway.

REPOSITORY
  R871 DrKonqi

REVISION DETAIL
  https://phabricator.kde.org/D11235

To: croick, #plasma_workspaces, #frameworks, ossi
Cc: ossi, lepagevalleeemmanuel, maximilianocuria, adridg, plasma-devel, 
sukalyanbanga, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D11236: [KCrash] Establish socket to allow change of ptrace scope

[Oswald Buddenhagen]

ossi requested changes to this revision.
ossi added a comment.
This revision now requires changes to proceed.


  yay, i finally have "some" time for this. ^^
  
  it took me a while to get a coherent picture of the problem and solution, 
because your description unnecessarily dwells on the irrelevant cases of ptrace 
restrictions, but omits the crucial fact that this is about tracers that are 
not descendant processes of drkonqi, specifically kdevelop launched via 
kdeinit. see also comment on DefaultDebuggerLauncher in the complementary 
change. please make sure to point that out in the commit messages.
  
  the socket code seemed like an unnecessary complication at first, but then i 
realized that launching via kdeinit makes it impossible to just talk to the 
child via stdout/stderr or at least pass a file descriptor of a socketpair end. 
that's something that could be actually addressed in the klauncher api, because 
it's possible to pass open fds to other processes over sockets.

INLINE COMMENTS

> kcrash.cpp:655
>  //if the process was started directly, use waitpid(), as it's a 
> child...
>  while (waitpid(-1, nullptr, 0) != pid) {}
>  } else {

you need to cover that branch as well.

> kcrash.cpp:668
> +// create socket path to transfer ptrace scope and open 
> connection
> +const QByteArray socketpath = 
> QStringLiteral("%1/kcrash_%2").arg(QDir::tempPath()).arg(pid).toLocal8Bit();
> +int sockfd = openDrKonqiSocket(socketpath);

use RuntimeLocation, as the kdeinit-related code (at start of 
setCrashHandler()) does.

> kcrash.cpp:870
> +
> +if (bind(sockfd, (struct sockaddr *)_server, 
> sizeof(drkonqi_server)) < 0) {
> +perror("Warning: bind() for communication with DrKonqi failed");

you need to unlink first, otherwise stale sockets will throw you off.

> kcrash.cpp:884
> +{
> +fd_set set;
> +FD_ZERO();

why don't you use poll()? this code is linux-specific anyway, so you can rely 
on posix functions from the previous decade.

> kcrash.cpp:904
> +clsockfd = accept(sockfd, (struct sockaddr *)_client, 
> );
> +} while (clsockfd == -1 && (errno == EINTR || errno == EAGAIN));
> +if (clsockfd < 0)

EAGAIN is a workaround for some unspecified broken non-linux systems, so you 
can drop that here.

> kcrash.cpp:915
> +if (ucred.pid != pid) {
> +perror("Warning: peer pid does not match DrKonqi pid");
> +return -1;

that's bogus use of perror()

REPOSITORY
  R285 KCrash

REVISION DETAIL
  https://phabricator.kde.org/D11236

To: croick, #frameworks, ossi
Cc: dfaure, lepagevalleeemmanuel, kde-frameworks-devel, sitter, michaelh, 
ngraham, bruns

D12745: Unify API for file descriptor sharing

[Oswald Buddenhagen]

ossi added a comment.


  \o/
  ;)

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D12745

To: chinmoyr, dfaure, ossi
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D12291: Accept file descriptor only from root owned process

[Oswald Buddenhagen]

ossi added inline comments.

INLINE COMMENTS

> chinmoyr wrote in fdreceiver.cpp:89
> > i don't see why that would be horrible
> 
> I meant adding "acceptConnection = true;" after #warning would look weird. 
> Obviously that's not even an issue and I shouldn't have mentioned it.
> 
> There is a discussion[1] going on related to a similar change in ktexteditor. 
> Because ktexteditor also uses polkit to save files in read-only location, one 
> of the suggestions to improve this process, in case the owner of target is 
> not root, was to either ignore the operation or drop privileges to 
> owner/group of the directory. Now in KIO the kauth helper performs every 
> operation as root. So if in future it is decided to do a privilege drop 
> before performing any file operation on non-root targets then this change 
> will likely be a hindrance. After considering the  fact that this is also 
> redundant, now I am not really feeling confident about this change. Just out 
> of curiosity, I want to know (although I feel weird for asking this) what was 
> your reason for accepting this patch?
> 
> [1]: https://bugzilla.suse.com/show_bug.cgi?id=1033055#c13

i initially didn't notice the problem we're currently discussing.
but more generally: it's a second layer of security, just in case somebody 
accidentally f*cks up the perms of their runtime dir (not something to be 
particularly concerned about; you'd certainly have bigger problems in this 
case). it might also help detecting configuration problems (though for that 
you'd have to add reasonable error reporting). and if done right, it's 
(currently) harmless, and i didn't feel like arguing over it. but i myself 
would just drop it.

REPOSITORY
  R241 KIO

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D12291

To: chinmoyr, #frameworks, dfaure, ossi
Cc: kde-frameworks-devel, ossi, michaelh, ngraham, bruns

D12291: Accept file descriptor only from root owned process

[Oswald Buddenhagen]

ossi added inline comments.

INLINE COMMENTS

> chinmoyr wrote in fdreceiver.cpp:89
> I don't think making acceptConnection unconditionally true is a good idea. At 
> least it will make this patch look horrible.
> Since getsocketopt conforms to posix, how about we check for __ unix __ , and 
> posix compliance instead of __ linux __ and upon failure inform the user to 
> install a posix compliant libc?

i don't see why that would be horrible; as i pointed out multiple times 
already, this change is redundant. one correction, though: add a code comment 
here rather than extending the commit message.

getsockopt() is standard, but the actual options aren't. you could change the 
ifdef to SO_PEERCRED itself, but that wouldn't actually add any portability.

REPOSITORY
  R241 KIO

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D12291

To: chinmoyr, #frameworks, dfaure, ossi
Cc: kde-frameworks-devel, ossi, michaelh, ngraham, bruns

D12291: Accept file descriptor only from root owned process

[Oswald Buddenhagen]

ossi added inline comments.

INLINE COMMENTS

> fdreceiver.cpp:89
> +#else
> +#warning Cannot get socket credentials!
> +#endif

i wonder whether that shouldn't come with an unconditional acceptConnection = 
true; then - now the compilation succeeds, but it will fail to operate. given 
that the whole feature is redundant with moving the socket to a safe place 
(something you really should mention in the commit message), this seems like a 
rather pointless limitation.

REPOSITORY
  R241 KIO

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D12291

To: chinmoyr, #frameworks, dfaure, ossi
Cc: kde-frameworks-devel, ossi, michaelh, ngraham, bruns

D12291: Accept file descriptor only from root owned process

[Oswald Buddenhagen]

ossi accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R241 KIO

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D12291

To: chinmoyr, #frameworks, dfaure, ossi
Cc: kde-frameworks-devel, ossi, michaelh, ngraham, bruns

D12745: Unify API for file descriptor sharing

[Oswald Buddenhagen]

ossi accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R241 KIO

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D12745

To: chinmoyr, dfaure, ossi
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D9966: [KIO] Fix issues with sharing of file descriptor

[Oswald Buddenhagen]

ossi added a comment.
Restricted Application added a subscriber: kde-frameworks-devel.


  this dependency tree is a mess. please remove deps to abandoned changes or 
unabandon what you actually still need, and linearize the history.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D9966

To: chinmoyr, #frameworks, thiago, dfaure, ossi
Cc: kde-frameworks-devel, ngraham, fvogt, lbeltrame, dfaure, michaelh, bruns

D12744: Add null pointer check when creating SocketAddress

[Oswald Buddenhagen]

ossi accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R241 KIO

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D12744

To: chinmoyr, dfaure, ossi
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D10410: Move the task of cleaning up of socket file to file ioslave and FdReceiver

[Oswald Buddenhagen]

ossi accepted this revision.
ossi added a comment.
This revision is now accepted and ready to land.


  note that the commit message needs a minor adjustment now.

INLINE COMMENTS

> chinmoyr wrote in fdreceiver.cpp:57
> So shall I commit this change separately right after pushing this patch? Or 
> after all the related patches are pushed?

the order really doesn't matter here.

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D10410

To: chinmoyr, #frameworks, ossi, dfaure
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D12745: Unify API for file descriptor sharing

[Oswald Buddenhagen]

ossi added a comment.


  but so does using raw pointers. as the stl is available here anyway, it seems 
like the preferable abstraction layer.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D12745

To: chinmoyr, dfaure, ossi
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D12745: Unify API for file descriptor sharing

[Oswald Buddenhagen]

ossi added a comment.


  why aren't you standardizing on std::string? that's cleaner than raw char 
pointers.
  i know we discussed this before to some degree, but i don't remember the 
particulars.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D12745

To: chinmoyr, dfaure, ossi
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D12744: Add null pointer check when creating SocketAddress

[Oswald Buddenhagen]

ossi requested changes to this revision.
ossi added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> fdreceiver.cpp:34
>  {
> +const SocketAddress addr(m_path.toLocal8Bit().constData());
> +if (!addr.address()) {

it would be more elegant to use m_path.toLocal8Bit().toStdString() here.

> fdsender.cpp:29
>  {
> +const SocketAddress addr(path.c_str());
> +if (!addr.address()) {

you're changing the type you're using for the call here. that's a good change, 
but logically not part of this patch.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D12744

To: chinmoyr, dfaure, ossi
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D10411: Create socket file in user's runtime directory

[Oswald Buddenhagen]

ossi accepted this revision.
Restricted Application added a subscriber: kde-frameworks-devel.

REPOSITORY
  R241 KIO

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D10411

To: chinmoyr, #frameworks, ossi
Cc: kde-frameworks-devel, dfaure, michaelh, ngraham, bruns

D10410: Move the task of cleaning up of socket file to file ioslave and FdReceiver

[Oswald Buddenhagen]

ossi requested changes to this revision.
ossi added a comment.
This revision now requires changes to proceed.


  did you make sure that this is the only place where SocketAddress is used?

INLINE COMMENTS

> fdreceiver.cpp:41
>  const SocketAddress addr(path.toStdString());
>  if (bind(m_socketDes, addr.address(), addr.length()) != 0 || 
> listen(m_socketDes, 5) != 0) {
>  std::cerr << "bind/listen error:" << strerror(errno) << std::endl;

do the first unlink right before here, so it's equivalent with the old code, 
just better structured.

> fdreceiver.cpp:57
>  }
> +::unlink(m_path.toLocal8Bit().constData());
>  }

that's a good addition, but it isn't logically part of this patch, because it 
adds a new feature (cleanup at exit) instead of only refactoring.

> file_unix.cpp:87
>  const QString sockPath = socketPath();
> +QFile::remove(sockPath);
>  FdReceiver fdRecv(sockPath);

that's the wrong place, imo. leave it FdReceiver, so it's more local.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D10410

To: chinmoyr, #frameworks, ossi, dfaure
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D12291: Accept file descriptor only from root owned process

[Oswald Buddenhagen]

ossi added a comment.


  as i certainly mentioned somewhere else already, this is redundant with 
putting the socket in a safe place. but fair enough ...

INLINE COMMENTS

> fdreceiver.cpp:67
>  if (client > 0) {
> -FDMessageHeader msg;
> -if (::recvmsg(client, msg.message(), 0) == 2) {
> -::memcpy(_fileDes, CMSG_DATA(msg.cmsgHeader()), sizeof 
> m_fileDes);
> +// Receive fd only if socket owner is root
> +bool acceptConnection = false;

i'd append "(our setuid helper)" to that - i wondered for a moment why the 
limitation.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D12291

To: chinmoyr, #frameworks, dfaure
Cc: ossi, michaelh, ngraham, bruns

D10273: Create proper SocketAddress

[Oswald Buddenhagen]

ossi requested changes to this revision.
ossi added a comment.
This revision now requires changes to proceed.


  note that there are also unaddressed comments from previous rounds.

INLINE COMMENTS

> sharefd_p.h:50
>  {
> -return reinterpret_cast();
> +return (addr.sun_path[0] || addr.sun_path[1]) ? 
> reinterpret_cast() : nullptr;
>  }

given that the linux-specific part is already gone by now, checking [1] doesn't 
make sense any more.

> sharefd_p.h:59
> +a.sun_family = AF_UNIX;
> +const QByteArray finalPath = "/tmp/" + path;
> +const size_t pathSize = finalPath.size();

wait a sec, didn't an ancestor commit change that?

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D10273

To: chinmoyr, #frameworks, dfaure, ossi
Cc: ossi, thiago, dfaure, michaelh, ngraham, bruns

D10409: In linux don't use abstract socket to share file descriptor

[Oswald Buddenhagen]

ossi accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R241 KIO

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D10409

To: chinmoyr, #frameworks, ossi
Cc: dfaure, michaelh, ngraham, bruns

D10410: Move the task of cleaning up of socket file to file ioslave and FdReceiver

[Oswald Buddenhagen]

ossi requested changes to this revision.
ossi added a comment.
This revision now requires changes to proceed.


  still needs update

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D10410

To: chinmoyr, #frameworks, ossi, dfaure
Cc: michaelh, ngraham, bruns

D10411: Create socket file in user's runtime directory

[Oswald Buddenhagen]

ossi accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R241 KIO

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D10411

To: chinmoyr, #frameworks, ossi
Cc: dfaure, michaelh, ngraham, bruns

D10409: In linux don't use abstract socket to share file descriptor

[Oswald Buddenhagen]

ossi added a comment.


  not sure why; the changes are semantically separate.
  
  my suggestion was to put this before D10273 
, thus reducing the latter's size.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D10409

To: chinmoyr, #frameworks, ossi
Cc: dfaure, michaelh

D10409: In linux don't use abstract socket to share file descriptor

[Oswald Buddenhagen]

ossi added a comment.


  that contradicts the comments i added to both reviews.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D10409

To: chinmoyr, #frameworks, ossi
Cc: dfaure, michaelh

D10409: In linux don't use abstract socket to share file descriptor

[Oswald Buddenhagen]

ossi added a comment.


  the idea is that you can do directory-based access controls on file-based 
sockets, while the abstract namespace has no controls.
  otoh, only linux has the abstract namespace, and it supports peer credential 
verification as well, so this doesn't actually add any security afaict.
  arguably, the patch still makes sense from a maintenance perspective, 
removing a redundant code path.
  fwiw, i'd re-order this patch before the other one - it makes for smaller 
patches to first remove code and then refactor only what's left.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D10409

To: chinmoyr, #frameworks, ossi
Cc: dfaure, michaelh

D10273: Create proper SocketAddress

[Oswald Buddenhagen]

ossi added inline comments.

INLINE COMMENTS

> dfaure wrote in fdsender.cpp:24
> In that case I don't understand why SocketAddress takes a QByteArray and not 
> a std::string Because ossi suggested it to unify the API and avoid one 
> conversion to std::string in fdereceiver? But then we have two contradictory 
> goals, we need to decide whether we use Qt or not in fdsender and therefore 
> in SocketAddress. Since one is not supposed to use Qt API without a 
> QCoreApplication instance, and since it's not recommented to run Qt code as 
> root, I think your original idea made sense, no Qt in fdsender nor in 
> SocketAddress.

i can get behind the idea of keeping SocketAddress and FdSender qt-free. it's 
ugly that FdReceiver stands out, but it would be way harder to make that one 
qt-free due to its use of the event loop.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D10273

To: chinmoyr, #frameworks, dfaure
Cc: ossi, thiago, dfaure, michaelh

D9966: [KIO] Fix issues with sharing of file descriptor

[Oswald Buddenhagen]

ossi added a comment.


  looks good, though technically speaking this still fixes two separate issues.

REPOSITORY
  R241 KIO

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D9966

To: chinmoyr, #frameworks, thiago, dfaure, ossi
Cc: ngraham, fvogt, lbeltrame, dfaure, michaelh

D10410: Move the task of cleaning up of socket file to file ioslave and FdReceiver

[Oswald Buddenhagen]

ossi added inline comments.

INLINE COMMENTS

> chinmoyr wrote in fdreceiver.cpp:62
> I didn't want to include QFile. That's the only reason.

uhm, you're still using it in this very line, just differently ;)

REPOSITORY
  R241 KIO

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D10410

To: chinmoyr, #frameworks, ossi, dfaure
Cc: michaelh

D10410: Move the task of cleaning up of socket file to file ioslave and FdReceiver

[Oswald Buddenhagen]

ossi added inline comments.

INLINE COMMENTS

> fdreceiver.cpp:62
>  }
> +::unlink(QFile::encodeName(m_path));
>  }

any particular reason not to use QFile::remove() here as well?

REPOSITORY
  R241 KIO

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D10410

To: chinmoyr, #frameworks, ossi, dfaure
Cc: michaelh

D8532: [WIP] Restrict file extractor with Seccomp

[Oswald Buddenhagen]

ossi added a comment.


  > But i'm not sure if thats feasible.
  
  launching subprocesses is no biggie; e.g., the kprocess test does it.
  if you're lucky, a few env variables will be sufficient. in the worst case, 
you'd need to chroot and do bind mounts or something like that, which is of 
course "a bit more challenging".

REPOSITORY
  R293 Baloo

REVISION DETAIL
  https://phabricator.kde.org/D8532

To: davidk, apol, ossi
Cc: detlefe, ngraham, nicolasfella, #frameworks, michaelh

D10273: Create proper SocketAddress

[Oswald Buddenhagen]

ossi added inline comments.

INLINE COMMENTS

> sharefd_p.h:61
> +const size_t pathSize = finalPath.size();
> +if (pathSize > 5 && pathSize < sizeof(a.sun_path) - 1) {
>  #ifdef __linux__

you now have a buffer overflow on linux. you need to split the conditional.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D10273

To: chinmoyr, #frameworks
Cc: ossi, thiago, dfaure, michaelh, ngraham

D10273: Create proper SocketAddress

[Oswald Buddenhagen]

ossi added inline comments.

INLINE COMMENTS

> fdsender.cpp:27
>  {
> +SocketAddress addr(path.c_str());
> +if (!addr.address()) {

this actually constructs a qbytearray. that should be probably explicit.

a better approach would be moving the class' interface to qbytearray (or 
actually qstring) as well, to unify the API. but that's again a separate 
refactoring.

> sharefd_p.h:55
> -sockaddr_un a{ AF_UNIX, {0}};
> -std::string finalPath = "/tmp/" + path;
> -#ifdef __linux__

you're removing the auto-prefixing with /tmp/. if that is intentional, you need 
to atomically adjust the callers, as otherwise you're changing behavior.

> sharefd_p.h:56
> -std::string finalPath = "/tmp/" + path;
> -#ifdef __linux__
> -::strcpy(_path[1], finalPath.c_str());

i now noticed that you removed that branch, thus removing the use of linux' 
abstract address space. that also calls for a justification.
did you research the git history to find out why it was introduced in the first 
place?

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D10273

To: chinmoyr, #frameworks
Cc: ossi, thiago, dfaure, michaelh, ngraham

D10273: Create proper SocketAddress

[Oswald Buddenhagen]

ossi added inline comments.

INLINE COMMENTS

> chinmoyr wrote in sharefd_p.h:60
> I just feel like the job of SocketAddress should be to create the address 
> structure and not perform any file operation. Removal of the socket file 
> should be handled by file ioslave or FdReceiver.

that's a valid concern, but doing that belongs into a separate patch which does 
the complementary change (as far as necessary) atomically, and explains it 
properly in the commit message.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D10273

To: chinmoyr, #frameworks
Cc: ossi, thiago, dfaure, michaelh, ngraham

D10273: Create proper SocketAddress

[Oswald Buddenhagen]

ossi added inline comments.

INLINE COMMENTS

> sharefd_p.h:60
> -::strcpy(a.sun_path, finalPath.c_str());
> -::unlink(finalPath.c_str());
> -#endif

you still need to address that *somehow* ;)

> sharefd_p.h:61
> +if (pathSize > 0 && pathSize < sizeof(a.sun_path) - 1) {
> +memcpy(a.sun_path, path.constData(), sizeof(a.sun_path) - 1);
> +}

the correct length is pathSize + 1

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D10273

To: chinmoyr, #frameworks
Cc: ossi, thiago, dfaure, michaelh, ngraham

D10273: Create proper SocketAddress

[Oswald Buddenhagen]

ossi added inline comments.

INLINE COMMENTS

> fdreceiver.h:45
> +QString m_path;
> +QSocketNotifier *m_readNotifier;
>  };

why are you moving the member?
it doesn't matter in this case, but generally it's better to have the bigger 
members first, concentrating in particular on equal size (and sizeof(QString) 
== sizeof(void*)).
all other things being equal, prefer no change to reduce the diff size.

> sharefd_p.h:54
>  private:
> -static sockaddr_un make_address(const std::string& path)
> +static sockaddr_un make_address(const QByteArray& path)
>  {

space on wrong side of amp

> sharefd_p.h:60
> +const size_t pathSize = path.size();
> +if (pathSize > 0 && pathSize < sizeof(a.sun_path)-1) {
> +::strncpy(a.sun_path, path.constData(), sizeof(a.sun_path)-1);

put spaces around binary operators

> sharefd_p.h:60
> -::strcpy(a.sun_path, finalPath.c_str());
> -::unlink(finalPath.c_str());
> -#endif

you're removing that without replacement. that makes me think that the patch is 
still non-atomic.

> sharefd_p.h:61
> +if (pathSize > 0 && pathSize < sizeof(a.sun_path)-1) {
> +::strncpy(a.sun_path, path.constData(), sizeof(a.sun_path)-1);
> +}

using strncpy() after an explicit length check is relatively pointless (unless 
the trailing zero padding is important).
but the operation can be optimized by using memcpy().

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D10273

To: chinmoyr, #frameworks
Cc: ossi, thiago, dfaure, michaelh, ngraham

D9966: [KIO] Fix issues with sharing of file descriptor

[Oswald Buddenhagen]

ossi added a comment.


  In https://phabricator.kde.org/D9966#199979, @dfaure wrote:
  
  > Thiago: now it's ready for your review ;)
  
  
  if he wants to look past the already pointed out non-atomicity. i for one 
just refuse to look at this mess any further.
  https://community.kde.org/Policies/Commit_Policy#Commit_complete_changesets 
(i find https://wiki.qt.io/Commit_Policy point 8 clearer (yeah, i wrote it down 
myself ^^))

REPOSITORY
  R241 KIO

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D9966

To: chinmoyr, #frameworks, thiago, dfaure, ossi
Cc: ngraham, fvogt, lbeltrame, dfaure, michaelh

Re: kcrash, fork, and stdout/stderr

[Oswald Buddenhagen]

On Tue, Dec 05, 2017 at 01:12:30PM +0100, David Faure wrote:
> On lundi 4 décembre 2017 17:04:25 CET Thiago Macieira wrote:
> > forking inside a signal handler is a bad idea because it may deadlock. If
> > the crash happens while glibc holds some mutexes relating to
> > pthread_atfork, then you'll have a problem.
> 
> I see. But how should one implement a crash handler that autorestarts an app, 
> then, in a "standalone application" use case, i.e. no kdeinit or other daemon 
> running in the background?
> 
you don't ...
the fail-safe solution would be forking right after start, basically
having a privately owned mini-kdeinit. but that seems like major
overkill. the crash handler (and even more auto-restart) are
nice-to-have features, so it seems reasonable that they don't work
reliably outside a kde session.

D8532: [WIP] Restrict file extractor with Seccomp

[Oswald Buddenhagen]

ossi added a comment.


  you *really* should use a whitelist. it's ok if that breaks some 3rdparty 
extractor; you'll get a bug report which you can properly evaluate.
  you could go totally overboard and assign fine-grained syscall capabilities 
to individual extractors, but i can't really think of legitimate reasons why 
that would be necessary in this context.

REPOSITORY
  R293 Baloo

REVISION DETAIL
  https://phabricator.kde.org/D8532

To: davidk, apol, ossi
Cc: #frameworks

D8254: Make file ioslave backend on removeables are user-frendly on linux

[Oswald Buddenhagen]

ossi added inline comments.

INLINE COMMENTS

> file_unix.cpp:241
>  QFile dest_file(dest);
> -if (!dest_file.open(QIODevice::Truncate | QIODevice::WriteOnly)) {
> +int dest_fd = ::open(dest.toStdString().c_str(), O_CREAT | O_TRUNC | 
> (is_removeable? O_SYNC : 0) | O_WRONLY);
> +if (!dest_file.open(dest_fd, QIODevice::Truncate | QIODevice::WriteOnly, 
> QFile::AutoCloseHandle)) {

you want O_DSYNC

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D8254

To: vova7890, #frameworks, dfaure, ossi, thiago
Cc: dfaure, ngraham, alexeymin, #frameworks

Re: Review Request 130084: Add a pair of flags forcing fsync during copy loop

[Oswald Buddenhagen]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/130084/#review103186
---




src/ioslaves/file/file_unix.cpp (line 112)
<https://git.reviewboard.kde.org/r/130084/#comment68663>

typo



src/ioslaves/file/file_unix.cpp (line 138)
<https://git.reviewboard.kde.org/r/130084/#comment68664>

why are you testing hotpluggable? what's the exact definition in this 
context? technically, any sata drive is hotpluggable ...



src/ioslaves/file/file_unix.cpp (line 324)
<https://git.reviewboard.kde.org/r/130084/#comment68665>

i'd use some parens here to make it more readable.


- Oswald Buddenhagen


On April 17, 2017, 1:27 p.m., KJ Tsanaktsidis wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/130084/
> ---
> 
> (Updated April 17, 2017, 1:27 p.m.)
> 
> 
> Review request for KDE Frameworks, Oswald Buddenhagen and Thiago Macieira.
> 
> 
> Repository: kio
> 
> 
> Description
> ---
> 
> When copying a large-ish file (~1-2GB) from very fast storage to very slow 
> storage (e.g. an NVME SSD to a cheap USB flash drive) on a machine with lots 
> of RAM, Dolphin displays a progress bar which finishes in a fraction of a 
> second (i.e. as fast as it takes to read the source file into the Linux page 
> cache). Unmounting the drive then of course takes a long time, with only an 
> indeterminate spinner.
> 
> This patch adds an option to force fsync during copy jobs, so that the copy 
> progress bar measures how long it will take to actually copy the file to the 
> destination.
> 
> I've added two flags - Fsync and FsyncCrossFilesystem - to the JobEnum flag. 
> The former will cause all copy operations to fsync during the copy loop, 
> whilst the latter will only fsync copies that are across different 
> filesystems.
> 
> If this patch gets OK'd, I have another patch which adds support for this 
> into the appropriate places in Dolphin. I would think that at least 
> FsyncCrossFilesystem should be the default, but Fsync always might be a 
> little heavy handed. At the least fsync'ing cross-filesystem copies ensures 
> that the unmount won't take forever.
> 
> 
> Diffs
> -
> 
>   src/ioslaves/file/CMakeLists.txt b9132ced9d4a08b2cf9f9bbbaa3bd43f026c6469 
>   src/ioslaves/file/ConfigureChecks.cmake 
> 5a83d1b9fbe90c851c774e3b467468d93b5a2bd4 
>   src/ioslaves/file/config-kioslave-file.h.cmake 
> 372f79d01ad4597aae0b2ae62627648fe7680b64 
>   src/ioslaves/file/file_unix.cpp 3c1b9927e3dd2d0134f77caec6e6b24a0356d26f 
> 
> Diff: https://git.reviewboard.kde.org/r/130084/diff/
> 
> 
> Testing
> ---
> 
> Tested the patch with KDE/Dolphin on Arch Linux, which is version 5.32.0. The 
> diff applies cleanly to master so I assume there shouldn't be any issues 
> there, but I've not actually checked that. As advertised, copying a file to 
> USB flash storage now displays an accurate progress bar.
> 
> I experimented with how often fsync should be called on my hardware, and I 
> found calling it every ~1M copied caused no decrease in copy performance 
> whilst still providing accurate progress info. That is the setting I've gone 
> with in this patch. I'm open to suggestions on how this could be tuned better 
> though.
> 
> 
> Thanks,
> 
> KJ Tsanaktsidis
> 
>

Re: Review Request 130084: Add a pair of flags forcing fsync during copy loop

[Oswald Buddenhagen]

> On April 15, 2017, 10:30 a.m., David Faure wrote:
> > But doesn't this make copying much slower in the normal case? (copying onto 
> > a non-removable harddisk partition).
> > 
> > It sounds to me like this should be
> > 1) done internally in kio_file (no Job API for this)
> > 2) only when the destination is a removable device

i've read multiple times that fsync isn't a performance problem on modern file 
systems any more. whatever that may mean.
limiting this to cross-device isn't really sensible imo - a) one can have 
multiple internal disks and b) even if the disk stays in, at some point the 
flushing will commence and will be a major slowdown for subsequent operations.
in fact, this problem is bad enough that the linux kernel community realized it 
(which, in the area of disk i/o, never ceases to amaze) - 
https://lwn.net/Articles/682582/ (obvious followup question: what kernel do you 
use? this code seems to have landed in 4.10)


- Oswald


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/130084/#review103048
---


On April 15, 2017, 10:28 a.m., KJ Tsanaktsidis wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/130084/
> ---
> 
> (Updated April 15, 2017, 10:28 a.m.)
> 
> 
> Review request for KDE Frameworks, Oswald Buddenhagen and Thiago Macieira.
> 
> 
> Repository: kio
> 
> 
> Description
> ---
> 
> When copying a large-ish file (~1-2GB) from very fast storage to very slow 
> storage (e.g. an NVME SSD to a cheap USB flash drive) on a machine with lots 
> of RAM, Dolphin displays a progress bar which finishes in a fraction of a 
> second (i.e. as fast as it takes to read the source file into the Linux page 
> cache). Unmounting the drive then of course takes a long time, with only an 
> indeterminate spinner.
> 
> This patch adds an option to force fsync during copy jobs, so that the copy 
> progress bar measures how long it will take to actually copy the file to the 
> destination.
> 
> I've added two flags - Fsync and FsyncCrossFilesystem - to the JobEnum flag. 
> The former will cause all copy operations to fsync during the copy loop, 
> whilst the latter will only fsync copies that are across different 
> filesystems.
> 
> If this patch gets OK'd, I have another patch which adds support for this 
> into the appropriate places in Dolphin. I would think that at least 
> FsyncCrossFilesystem should be the default, but Fsync always might be a 
> little heavy handed. At the least fsync'ing cross-filesystem copies ensures 
> that the unmount won't take forever.
> 
> 
> Diffs
> -
> 
>   src/core/copyjob.cpp 7c02cb50d7e9c11bbcd9264832357f3fd6dc8c16 
>   src/core/filecopyjob.cpp 301b7039158b7dc537b9004c14845b3d1d60f8eb 
>   src/core/job_base.h 0be9629f42277afc5f72d00d0cae5c9c1cd2b8bc 
>   src/core/slavebase.cpp 3778df813b8568657a2cbd9412c1244f94696a0c 
>   src/ioslaves/file/file_unix.cpp 3c1b9927e3dd2d0134f77caec6e6b24a0356d26f 
> 
> Diff: https://git.reviewboard.kde.org/r/130084/diff/
> 
> 
> Testing
> ---
> 
> Tested the patch with KDE/Dolphin on Arch Linux, which is version 5.32.0. The 
> diff applies cleanly to master so I assume there shouldn't be any issues 
> there, but I've not actually checked that. As advertised, copying a file to 
> USB flash storage now displays an accurate progress bar.
> 
> I experimented with how often fsync should be called on my hardware, and I 
> found calling it every ~1M copied caused no decrease in copy performance 
> whilst still providing accurate progress info. That is the setting I've gone 
> with in this patch. I'm open to suggestions on how this could be tuned better 
> though.
> 
> 
> Thanks,
> 
> KJ Tsanaktsidis
> 
>

D4975: Prevent misdetection of EOF on Linux

[Oswald Buddenhagen]

ossi added a comment.


  yeah, but without knowing the root cause, your fix may be just wrong, or at 
least the comment/commit message may be misleading.
  also, 
https://community.kde.org/Policies/Commit_Policy#Don.27t_commit_code_you_don.27t_understand

REPOSITORY
  R291 KPty

REVISION DETAIL
  https://phabricator.kde.org/D4975

To: Lekensteyn, #konsole, hindenburg, ossi
Cc: #frameworks

D4975: Prevent misdetection of EOF on Linux

[Oswald Buddenhagen]

ossi added a comment.


  first try googling it; maybe there is even a response on stackoverflow (as so 
often happens).
  then try posting it yourself to stackoverflow.
  dunno whether such questions are welcome on the linux-kernel list, but if 
not, there might be other lists where it would fit.

INLINE COMMENTS

> Lekensteyn wrote in kptydevice.cpp:307
> Constrained in what sense? Should the first known broken kernel version be 
> noted here?

basically what you put in the commit message.

REPOSITORY
  R291 KPty

REVISION DETAIL
  https://phabricator.kde.org/D4975

To: Lekensteyn, #konsole, hindenburg, ossi
Cc: #frameworks

D4975: Prevent misdetection of EOF on Linux

[Oswald Buddenhagen]

ossi added a comment.


  please verify that this is in fact a kernel bug (include ml references), not 
something that should be expected for some weird reasons (compare the solaris 
path).
  if this is a bug, does it actually affect released versions which are not 
being "upgraded away" on short notice? i wouldn't want a workaround for a bug 
that had a very short life span.

INLINE COMMENTS

> kptydevice.cpp:307
> +#else
> +// On Linux, for some unknown reason 0 bytes can be returned after an
> +// interrupt signal even if a slave is still attached. To avoid 
> marking

this should be constrained more - affected versions, and the fact that it's a 
bug.

> kptydevice.cpp:315
> +return true;
> +} else if (available) {
> +maybeEof = false;

need no else after return.

REPOSITORY
  R291 KPty

REVISION DETAIL
  https://phabricator.kde.org/D4975

To: Lekensteyn, #konsole, hindenburg, ossi
Cc: #frameworks

Re: Review Request 129122: Try to use ulog-helper if utempter does not exist

[Oswald Buddenhagen]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129122/#review100380
---


Ship it!




if you want to push it already, make sure to omit the other commit for now.

- Oswald Buddenhagen


On Oct. 20, 2016, 7:52 a.m., Tobias Berner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129122/
> ---
> 
> (Updated Oct. 20, 2016, 7:52 a.m.)
> 
> 
> Review request for KDE Frameworks, Adriaan de Groot, Gleb Popov, Oswald 
> Buddenhagen, and Martin Tobias Holmedahl Sandsmark.
> 
> 
> Repository: kpty
> 
> 
> Description
> ---
> 
> FreeBSD does not have `/usr/libexec/*/utempter`. It does however have 
> `/usr/libexec/ulog-helper` [1].
> 
> It uses `login` instead of `add` and `logout` instead of `del`.
> 
> 
> [1] 
> https://svnweb.freebsd.org/base/head/libexec/ulog-helper/ulog-helper.c?revision=234469=markup
> 
> 
> Diffs
> -
> 
>   cmake/FindUTEMPTER.cmake 4921e58 
>   src/kpty.cpp 7bf31c3 
> 
> Diff: https://git.reviewboard.kde.org/r/129122/diff/
> 
> 
> Testing
> ---
> 
> Builds fine. And it is actually working :)
> 
> 
> Thanks,
> 
> Tobias Berner
> 
>

Re: Review Request 129197: Fix tests on FreeBSD

[Oswald Buddenhagen]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129197/#review100148
---




autotests/kptyprocesstest.cpp (line 123)
<https://git.reviewboard.kde.org/r/129197/#comment67266>

just use QStandardPaths::findExecutable()

anyway, that's for a separate commit.


- Oswald Buddenhagen


On Oct. 16, 2016, 11:44 a.m., Gleb Popov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129197/
> ---
> 
> (Updated Oct. 16, 2016, 11:44 a.m.)
> 
> 
> Review request for KDE Frameworks, Adriaan de Groot, Tobias Berner, Oswald 
> Buddenhagen, and Martin Tobias Holmedahl Sandsmark.
> 
> 
> Repository: kpty
> 
> 
> Description
> ---
> 
> Apparently, KPtyDevice can't be operated on after KPtyProcess finishes. Tweak 
> tests accordingly, so they actually test things while the process is still 
> running.
> 
> 
> Diffs
> -
> 
>   autotests/kptyprocesstest.cpp 8b0b5b0 
> 
> Diff: https://git.reviewboard.kde.org/r/129197/diff/
> 
> 
> Testing
> ---
> 
> make test on FreeBSD
> 
> 
> Thanks,
> 
> Gleb Popov
> 
>

Re: Review Request 129197: Fix tests on FreeBSD

[Oswald Buddenhagen]

> On Oct. 16, 2016, 1:12 p.m., Tobias Berner wrote:
> > I'm kind of unsure if this is right. Yes, the tests run now, but isn't the 
> > issue rather in the way kpty works (or fails to work on FreeBSD)?
> 
> Gleb Popov wrote:
> From what i've understood, this boils down to 
> `KPtyDevicePrivate::_k_canRead()` method in kptydevice.cpp. The line 284
> 
> if (!::ioctl(q->masterFd(), PTY_BYTES_AVAILABLE, (char *) ))
> 
> returns 0 in `available` and this makes method return `false`. This, in 
> turn, make `waitFor*` methods return false too.
> 
> Now you mention it, i'm also unsure if this `ioctl` behaves different on 
> Linux.

i've been trying to make sense of the freebsd pts layer, and utterly failed. 
the documentation is anything between abysmal and non-existing. i suggest you 
find and invite an actual expert.

kptyprocess keeps both ends of the pty open, so whether the child process is 
running or not should be irrelevant. apparently, it's not. maybe this is 
somehow related to whether the tty is the process' controlling terminal.

you can try the following:
- remove the freebsd case from the PTY_BYTES_AVAILABLE definition near the top. 
this was introduced 2008, before freebsd 8's release, which got an entirely new 
pts layer. it may just work with the generic code now.
- try replacing the masterFd() with slaveFd() in the above ioctl. that would be 
kinda broken, but at least it would be a data point.


- Oswald


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129197/#review100039
---


On Oct. 16, 2016, 11:44 a.m., Gleb Popov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129197/
> ---
> 
> (Updated Oct. 16, 2016, 11:44 a.m.)
> 
> 
> Review request for KDE Frameworks, Adriaan de Groot, Tobias Berner, Oswald 
> Buddenhagen, and Martin Tobias Holmedahl Sandsmark.
> 
> 
> Repository: kpty
> 
> 
> Description
> ---
> 
> Apparently, KPtyDevice can't be operated on after KPtyProcess finishes. Tweak 
> tests accordingly, so they actually test things while the process is still 
> running.
> 
> 
> Diffs
> -
> 
>   autotests/kptyprocesstest.cpp 8b0b5b0 
> 
> Diff: https://git.reviewboard.kde.org/r/129197/diff/
> 
> 
> Testing
> ---
> 
> make test on FreeBSD
> 
> 
> Thanks,
> 
> Gleb Popov
> 
>

Re: Review Request 129122: Try to use ulog-helper if utempter does not exist

[Oswald Buddenhagen]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129122/#review100144
---



an update about the runtime testing would be nice ;)


cmake/FindUTEMPTER.cmake (line 47)
<https://git.reviewboard.kde.org/r/129122/#comment67264>

just noticed: indentation wrong.


- Oswald Buddenhagen


On Oct. 19, 2016, 7:51 p.m., Tobias Berner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129122/
> ---
> 
> (Updated Oct. 19, 2016, 7:51 p.m.)
> 
> 
> Review request for KDE Frameworks, Adriaan de Groot, Gleb Popov, Oswald 
> Buddenhagen, and Martin Tobias Holmedahl Sandsmark.
> 
> 
> Repository: kpty
> 
> 
> Description
> ---
> 
> FreeBSD does not have `/usr/libexec/*/utempter`. It does however have 
> `/usr/libexec/ulog-helper` [1].
> 
> It uses `login` instead of `add` and `logout` instead of `del`.
> 
> 
> [1] 
> https://svnweb.freebsd.org/base/head/libexec/ulog-helper/ulog-helper.c?revision=234469=markup
> 
> 
> Diffs
> -
> 
>   cmake/FindUTEMPTER.cmake 4921e58 
>   src/kpty.cpp 7bf31c3 
> 
> Diff: https://git.reviewboard.kde.org/r/129122/diff/
> 
> 
> Testing
> ---
> 
> Builds fine. Still need to test if it is actually working.
> 
> 
> Thanks,
> 
> Tobias Berner
> 
>

Re: Review Request 129122: Try to use ulog-helper if utempter does not exist

[Oswald Buddenhagen]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129122/#review100127
---



doesn't look fundamentally wrong ^^


src/kpty.cpp (line 78)
<https://git.reviewboard.kde.org/r/129122/#comment67254>

no space after the !

or just use ifndef


- Oswald Buddenhagen


On Oct. 13, 2016, 2:08 p.m., Tobias Berner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129122/
> ---
> 
> (Updated Oct. 13, 2016, 2:08 p.m.)
> 
> 
> Review request for KDE Frameworks, Adriaan de Groot, Gleb Popov, Oswald 
> Buddenhagen, and Martin Tobias Holmedahl Sandsmark.
> 
> 
> Repository: kpty
> 
> 
> Description
> ---
> 
> FreeBSD does not have `/usr/libexec/*/utempter`. It does however have 
> `/usr/libexec/ulog-helper` [1].
> 
> It uses `login` instead of `add` and `logout` instead of `del`.
> 
> 
> [1] 
> https://svnweb.freebsd.org/base/head/libexec/ulog-helper/ulog-helper.c?revision=234469=markup
> 
> 
> Diffs
> -
> 
>   cmake/FindUTEMPTER.cmake 9773963 
>   src/kpty.cpp 7bf31c3 
> 
> Diff: https://git.reviewboard.kde.org/r/129122/diff/
> 
> 
> Testing
> ---
> 
> Builds fine. Still need to test if it is actually working.
> 
> 
> Thanks,
> 
> Tobias Berner
> 
>

Re: Review Request 128790: Call utempter manually

[Oswald Buddenhagen]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128790/#review99696
---


Ship it!




Ship It!

- Oswald Buddenhagen


On Oct. 1, 2016, 10:37 a.m., Martin Tobias Holmedahl Sandsmark wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128790/
> ---
> 
> (Updated Oct. 1, 2016, 10:37 a.m.)
> 
> 
> Review request for KDE Frameworks, David Faure, Kurt Hindenburg, Oswald 
> Buddenhagen, Rex Dieter, and Thiago Macieira.
> 
> 
> Bugs: 364779
> https://bugs.kde.org/show_bug.cgi?id=364779
> 
> 
> Repository: kpty
> 
> 
> Description
> ---
> 
> According to the investigation in https://bugs.kde.org/show_bug.cgi?id=364779 
> utempter does stuff in a way that isn't compatible with 
> QProcess/KProcess/KPtyProcess (calling sigaction() before launching its child 
> process).
> 
> So instead we invoke it manually with QProcess.
> 
> 
> Diffs
> -
> 
>   cmake/FindUTEMPTER.cmake a3ea06a 
>   src/CMakeLists.txt caab96f 
>   src/kpty.cpp 15c3b81 
>   src/kpty_p.h 192bf1c 
> 
> Diff: https://git.reviewboard.kde.org/r/128790/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Martin Tobias Holmedahl Sandsmark
> 
>

Re: Review Request 128790: Call utempter manually

[Oswald Buddenhagen]

> On Sept. 23, 2016, 5:17 a.m., Oswald Buddenhagen wrote:
> > cmake/FindUTEMPTER.cmake, line 53
> > <https://git.reviewboard.kde.org/r/128790/diff/5/?file=476595#file476595line53>
> >
> > i suppose it would make sense to keep it, but with the executable.
> 
> Martin Tobias Holmedahl Sandsmark wrote:
> I don't think mark_as_advanced() is appropriate, it makes sense to allow 
> packagers to manually specify the path.

the question is whether an average build-from-source user would ever need it. i 
don't think so.


- Oswald


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128790/#review99479
---


On Sept. 9, 2016, 4:50 a.m., Martin Tobias Holmedahl Sandsmark wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128790/
> ---
> 
> (Updated Sept. 9, 2016, 4:50 a.m.)
> 
> 
> Review request for KDE Frameworks, David Faure, Kurt Hindenburg, Oswald 
> Buddenhagen, Rex Dieter, and Thiago Macieira.
> 
> 
> Bugs: 364779
> https://bugs.kde.org/show_bug.cgi?id=364779
> 
> 
> Repository: kpty
> 
> 
> Description
> ---
> 
> According to the investigation in https://bugs.kde.org/show_bug.cgi?id=364779 
> utempter does stuff in a way that isn't compatible with 
> QProcess/KProcess/KPtyProcess (calling sigaction() before launching its child 
> process).
> 
> So instead we invoke it manually with QProcess.
> 
> 
> Diffs
> -
> 
>   cmake/FindUTEMPTER.cmake a3ea06a 
>   src/CMakeLists.txt caab96f 
>   src/kpty.cpp 15c3b81 
>   src/kpty_p.h 192bf1c 
> 
> Diff: https://git.reviewboard.kde.org/r/128790/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Martin Tobias Holmedahl Sandsmark
> 
>

Re: Review Request 128790: Call utempter manually

[Oswald Buddenhagen]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128790/#review99479
---



a bit of delay due to vacation ...


cmake/FindUTEMPTER.cmake 
<https://git.reviewboard.kde.org/r/128790/#comment66914>

i suppose it would make sense to keep it, but with the executable.


- Oswald Buddenhagen


On Sept. 9, 2016, 4:50 a.m., Martin Tobias Holmedahl Sandsmark wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128790/
> ---
> 
> (Updated Sept. 9, 2016, 4:50 a.m.)
> 
> 
> Review request for KDE Frameworks, David Faure, Kurt Hindenburg, Oswald 
> Buddenhagen, Rex Dieter, and Thiago Macieira.
> 
> 
> Bugs: 364779
> https://bugs.kde.org/show_bug.cgi?id=364779
> 
> 
> Repository: kpty
> 
> 
> Description
> ---
> 
> According to the investigation in https://bugs.kde.org/show_bug.cgi?id=364779 
> utempter does stuff in a way that isn't compatible with 
> QProcess/KProcess/KPtyProcess (calling sigaction() before launching its child 
> process).
> 
> So instead we invoke it manually with QProcess.
> 
> 
> Diffs
> -
> 
>   cmake/FindUTEMPTER.cmake a3ea06a 
>   src/CMakeLists.txt caab96f 
>   src/kpty.cpp 15c3b81 
>   src/kpty_p.h 192bf1c 
> 
> Diff: https://git.reviewboard.kde.org/r/128790/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Martin Tobias Holmedahl Sandsmark
> 
>

Re: Review Request 128790: Call utempter manually

[Oswald Buddenhagen]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128790/#review98963
---




cmake/FindUTEMPTER.cmake (line 35)
<https://git.reviewboard.kde.org/r/128790/#comment66626>

this is not necessary any more unless you use it to compute a possible 
search path



cmake/FindUTEMPTER.cmake (line 37)
<https://git.reviewboard.kde.org/r/128790/#comment66625>

remove this and the respective link line



src/kpty.cpp (line 28)
<https://git.reviewboard.kde.org/r/128790/#comment66624>

    dead include


- Oswald Buddenhagen


On Sept. 4, 2016, 10:20 a.m., Martin Tobias Holmedahl Sandsmark wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128790/
> ---
> 
> (Updated Sept. 4, 2016, 10:20 a.m.)
> 
> 
> Review request for KDE Frameworks, David Faure, Kurt Hindenburg, Oswald 
> Buddenhagen, Rex Dieter, and Thiago Macieira.
> 
> 
> Bugs: 364779
> https://bugs.kde.org/show_bug.cgi?id=364779
> 
> 
> Repository: kpty
> 
> 
> Description
> ---
> 
> According to the investigation in https://bugs.kde.org/show_bug.cgi?id=364779 
> utempter does stuff in a way that isn't compatible with 
> QProcess/KProcess/KPtyProcess (calling sigaction() before launching its child 
> process).
> 
> So instead we invoke it manually with QProcess.
> 
> 
> Diffs
> -
> 
>   cmake/FindUTEMPTER.cmake a3ea06a 
>   src/kpty.cpp 15c3b81 
>   src/kpty_p.h 192bf1c 
> 
> Diff: https://git.reviewboard.kde.org/r/128790/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Martin Tobias Holmedahl Sandsmark
> 
>

Re: Review Request 128790: Call utempter manually

[Oswald Buddenhagen]

> On Aug. 28, 2016, 3:24 p.m., Oswald Buddenhagen wrote:
> > src/kpty.cpp, line 165
> > <https://git.reviewboard.kde.org/r/128790/diff/2/?file=475587#file475587line165>
> >
> > hmpf, i'd really prefer this to be a configure/cmake thing rather than 
> > a runtime detection. less runtime code, and allows for a user override 
> > (without reading yet another custom env variable, which sounds really off).
> 
> Martin Tobias Holmedahl Sandsmark wrote:
> you mean make cmake do the whole figure-out-where-the-executable-is 
> thing? because that's beyond my abilities to do. and it doesn't really 
> require yet another environment variable, just normal $PATH tweaking afaics.

yes, i do.
learn it (it's really not hard, and the kde sources are full of relevant 
examples), or ask someone to help (it won't be me).

extending PATH with libexec stuff is just wrong. also, this is plainly not 
something anyone should need to bother with after the build, as it's a static 
adjustment, just like linking libutempter.


- Oswald


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128790/#review98749
---


On Aug. 28, 2016, 5:36 p.m., Martin Tobias Holmedahl Sandsmark wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128790/
> ---
> 
> (Updated Aug. 28, 2016, 5:36 p.m.)
> 
> 
> Review request for KDE Frameworks, David Faure, Kurt Hindenburg, Rex Dieter, 
> and Thiago Macieira.
> 
> 
> Bugs: 364779
> https://bugs.kde.org/show_bug.cgi?id=364779
> 
> 
> Repository: kpty
> 
> 
> Description
> ---
> 
> According to the investigation in https://bugs.kde.org/show_bug.cgi?id=364779 
> utempter does stuff in a way that isn't compatible with 
> QProcess/KProcess/KPtyProcess (calling sigaction() before launching its child 
> process).
> 
> So instead we invoke it manually with QProcess.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt caab96f 
>   src/kpty.cpp 15c3b81 
>   src/kpty_p.h 192bf1c 
> 
> Diff: https://git.reviewboard.kde.org/r/128790/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Martin Tobias Holmedahl Sandsmark
> 
>

Re: Review Request 128790: Call utempter manually

[Oswald Buddenhagen]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128790/#review98749
---




src/kpty.cpp (line 165)
<https://git.reviewboard.kde.org/r/128790/#comment66494>

hmpf, i'd really prefer this to be a configure/cmake thing rather than a 
runtime detection. less runtime code, and allows for a user override (without 
reading yet another custom env variable, which sounds really off).



src/kpty.cpp (line 528)
<https://git.reviewboard.kde.org/r/128790/#comment66495>

please add a comment noting that you're reproducing libutempter x.y 
behavior on platform foo. just in case there are still incompatible versions 
around.


- Oswald Buddenhagen


On Aug. 28, 2016, 2:58 p.m., Martin Tobias Holmedahl Sandsmark wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128790/
> ---
> 
> (Updated Aug. 28, 2016, 2:58 p.m.)
> 
> 
> Review request for KDE Frameworks, David Faure, Kurt Hindenburg, Rex Dieter, 
> and Thiago Macieira.
> 
> 
> Bugs: 364779
> https://bugs.kde.org/show_bug.cgi?id=364779
> 
> 
> Repository: kpty
> 
> 
> Description
> ---
> 
> According to the investigation in https://bugs.kde.org/show_bug.cgi?id=364779 
> utempter does stuff in a way that isn't compatible with 
> QProcess/KProcess/KPtyProcess (calling sigaction() before launching its child 
> process).
> 
> So instead we invoke it manually with QProcess.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt caab96f 
>   src/kpty.cpp 15c3b81 
>   src/kpty_p.h 192bf1c 
> 
> Diff: https://git.reviewboard.kde.org/r/128790/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Martin Tobias Holmedahl Sandsmark
> 
>

Re: Review Request 128790: Remove usage of utempter

[Oswald Buddenhagen]

> On Aug. 28, 2016, 1:41 p.m., Oswald Buddenhagen wrote:
> > uhm, and why do you think utempter is the preferred choice?
> > last time i checked, nobody was shipping konsole setgid utmp. with kdeinit, 
> > that's not even an option.
> > 
> > there are several options how to deal with this:
> > - fork()/wait() around the utempter calls, so it can't mess with the signal 
> > handling of the current process. though i seem to remmber that the 
> > addToUtmp() call actually uses the PID
> > - re-implement the libutempter calls with QProcess. that's actually how it 
> > was originally, but was changed because there were incompatible versions of 
> > utempter - but that seems like a minor concern compared to the status quo.
> > - drop utmp handling altogether, as it's been mostly superseded, first by 
> > consolekit, and now logind. however, respective bindings would have to be 
> > actually implemented, and i have no clue how things are supposed to be 
> > done. just some dbus calls?
> > -- what about non-linux systems?
> 
> Martin Tobias Holmedahl Sandsmark wrote:
> > uhm, and why do you think utempter is the preferred choice?
> > last time i checked, nobody was shipping konsole setgid utmp. with 
> kdeinit, that's not even an option.
> 
> Why is the fallback code there at all, then?
> 
> 
> > - fork()/wait() around the utempter calls, so it can't mess with the 
> signal handling of the current process. though i seem to remmber that the 
> addToUtmp() call actually uses the PID
> 
> Won't that break if another process exits at the wrong time?
> 
> 
> > - re-implement the libutempter calls with QProcess. that's actually how 
> it was originally, but was changed because there were incompatible versions 
> of utempter - but that seems like a minor concern compared to the status quo.
> 
> I looked at that as well, the issue is finding the correct path for the 
> utempter helper.
> 
>  
> > - drop utmp handling altogether, as it's been mostly superseded, first 
> by consolekit, and now logind. however, respective bindings would have to be 
> actually implemented, and i have no clue how things are supposed to be done. 
> just some dbus calls?
> 
> There seems to be a simple dbus call to register with logind at least. I 
> tried looking briefly at it, but I couldn't quickly find any logind code that 
> did utmp stuff. I didn't look very hard, though.
> 
> 
> > -- what about non-linux systems?
> 
> libutempter only supports Linux and FreeBSD, the fallback code seems to 
> at least try to be compatible with other platforms.

> Why is the fallback code there at all, then?

it was there first. it will actually work if somebody runs konsole as root. 
which nobody does, of course.

> Won't that break if another process exits at the wrong time?

not if the parent doesn't do any messing with the signal handling. which it 
doesn't need to, as the defaults (and what qprocess does) are perfectly fine.
the likely pid problem remains. one could wrap only the unregistration, but 
then a hypothetical race between utempter registration calls and unrelated 
qprocess exits remains.

> I looked at that as well, the issue is finding the correct path for the 
> utempter helper.

the configure test could run 'strings' over libutempter.so and grep for 
relevant patterns. ^^
or just try to divine it from the libutempter location based on typical 
directory structures.
the last resort would be letting the user specify it.

> I tried looking briefly at it, but I couldn't quickly find any logind code 
> that did utmp stuff

logind doesn't do the legacy stuff any more (consolekit still did, iirc). 
you're supposed to use loginctl.
but i don't know whether one is actually supposed to register pseudo ttys in 
the first place.

> libutempter only supports Linux and FreeBSD

the two dashes were to meant to illustrate a sub-point. i.e., what about 
non-systemd systems?

a whole different approach would be providing an own kutempter - quite similar 
to kcheckpass (which is mostly redundant with pam's unix_chkpwd). or actually, 
to kgrantpty, which is redundant with the pt_chown helper some grantpt() 
implementations use.
i actually once had a todo item about that ...


- Oswald


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128790/#review98742
---


On Aug. 28, 2016, 1:13 p.m., Martin Tobias Holmedahl Sandsmark wrote:
> 
> ---
> This is an automatically generated e-mail. To reply,

Re: Review Request 128790: Remove usage of utempter

[Oswald Buddenhagen]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128790/#review98742
---



uhm, and why do you think utempter is the preferred choice?
last time i checked, nobody was shipping konsole setgid utmp. with kdeinit, 
that's not even an option.

there are several options how to deal with this:
- fork()/wait() around the utempter calls, so it can't mess with the signal 
handling of the current process. though i seem to remmber that the addToUtmp() 
call actually uses the PID
- re-implement the libutempter calls with QProcess. that's actually how it was 
originally, but was changed because there were incompatible versions of 
utempter - but that seems like a minor concern compared to the status quo.
- drop utmp handling altogether, as it's been mostly superseded, first by 
consolekit, and now logind. however, respective bindings would have to be 
actually implemented, and i have no clue how things are supposed to be done. 
just some dbus calls?
-- what about non-linux systems?

- Oswald Buddenhagen


On Aug. 28, 2016, 1:13 p.m., Martin Tobias Holmedahl Sandsmark wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128790/
> ---
> 
> (Updated Aug. 28, 2016, 1:13 p.m.)
> 
> 
> Review request for KDE Frameworks, David Faure, Kurt Hindenburg, Rex Dieter, 
> and Thiago Macieira.
> 
> 
> Bugs: 364779
> https://bugs.kde.org/show_bug.cgi?id=364779
> 
> 
> Repository: kpty
> 
> 
> Description
> ---
> 
> According to the investigation in https://bugs.kde.org/show_bug.cgi?id=364779 
> utempter does stuff in a way that isn't compatible with 
> QProcess/KProcess/KPtyProcess (calling sigaction() before launching its child 
> process). So remove it, and rely on the fallback methods already implemented.
> 
> 
> Diffs
> -
> 
>   CMakeLists.txt 3e17cac 
>   KF5PtyConfig.cmake.in 66f8c43 
>   cmake/FindUTEMPTER.cmake a3ea06a 
>   src/CMakeLists.txt caab96f 
>   src/ConfigureChecks.cmake ded08f4 
>   src/config-pty.h.cmake aaaf8d9 
>   src/kpty.cpp 15c3b81 
> 
> Diff: https://git.reviewboard.kde.org/r/128790/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Martin Tobias Holmedahl Sandsmark
> 
>

Re: kptyprocesstest is still flaky

[Oswald Buddenhagen]

On Sun, May 22, 2016 at 03:59:59PM +0200, David Faure wrote:
> The CI for KF5 is now all green except for kpty which is still flaky.
> 
> https://build.kde.org/view/Frameworks%20kf5-qt5/job/kpty%20master%20stable-kf5-qt5/42/PLATFORM=Linux,compiler=gcc/testReport/junit/%28root%29/TestSuite/kptyprocesstest/
> 
> Same failure as a few years ago:
> 
> FAIL!  : KPtyProcessTest::test_pty_signals() Compared values are not the same
>Actual   (QLatin1String(log)) : 
> "<<<>bla\r\n$\n>foobar\r\n$\n!\n<>fooish\r\n$\n>bar\r\n$\n!\n<|$\n"
>Expected (QLatin1String(want)): 
> "<>bla\r\n$\n!\nfoobar\r\n$\n!\n<>fooish\r\n$\n>bar\r\n$\n!\n<|$\n"
>Loc: 
> [/home/jenkins/sources/kpty/stable-kf5-qt5/autotests/kptyprocesstest.cpp(201)]
> 
> I can reproduce it locally if I run the test in a loop.
> 
> Is this worth fixing, or should we just disable the test?
>
the test certainly is worth keeping.

you may get lucky with increasing the step timer to 100 ms.
of course, that isn't a real fix, but that would be a lot more complex
(it would be necessary to have a handshake channel with the other end,
which implies a custom executable instead of cat).
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 123811: Use tcgetattr tcsetattr if available

[Oswald Buddenhagen]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/123811/#review80499
---

Ship it!


Ship It!

- Oswald Buddenhagen


On May 16, 2015, 1:33 p.m., Pino Toscano wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/123811/
 ---
 
 (Updated May 16, 2015, 1:33 p.m.)
 
 
 Review request for KDE Frameworks and Oswald Buddenhagen.
 
 
 Repository: kpty
 
 
 Description
 ---
 
 Look for `tcgetattr`  `tcsetattr`, and use them if found before trying the 
 own OS checks. They are specified by POSIX.1-2008, so they should be 
 available on platforms implementing modern POSIX interfaces.
 
 The rest of the fallback code is left as is for platforms not previously 
 using `tcgetattr`  `tcsetattr`.
 
 
 Diffs
 -
 
   src/ConfigureChecks.cmake 35d7402567016c89c1fd947f44e29351aca4fe4a 
   src/config-pty.h.cmake e8dc262df0d1e6342e869fe17509b10a64baf300 
   src/kpty.cpp 145514cdee53dc81cf7453bd8d1514f173cb972e 
 
 Diff: https://git.reviewboard.kde.org/r/123811/diff/
 
 
 Testing
 ---
 
 On Linux:
 
 1. builds fine, and tests pass
 2. manually set `HAVE_TCGETATTR`  `HAVE_TCSETATTR` to `FALSE`, the `ioctl` 
 code is used as before, build is fine and so tests too 
 
 Not tested on other OSes, but the second test above should make sure there's 
 no build behaviour change if a platform does not have those functions.
 
 
 Thanks,
 
 Pino Toscano
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 123811: Use tcgetattr tcsetattr if available

[Oswald Buddenhagen]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/123811/#review80462
---



src/kpty.cpp (line 115)
https://git.reviewboard.kde.org/r/123811/#comment55181

there is no point in leaving that path in, and it makes the code less 
readable.


- Oswald Buddenhagen


On May 16, 2015, 9:56 a.m., Pino Toscano wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/123811/
 ---
 
 (Updated May 16, 2015, 9:56 a.m.)
 
 
 Review request for KDE Frameworks and Oswald Buddenhagen.
 
 
 Repository: kpty
 
 
 Description
 ---
 
 Look for `tcgetattr`  `tcsetattr`, and use them if found before trying the 
 own OS checks. They are specified by POSIX.1-2008, so they should be 
 available on platforms implementing modern POSIX interfaces.
 
 The rest of the fallback code is left as is (including manually selecting 
 `tcgetattr`  `tcsetattr` on some OSes), to potentially avoid other platforms.
 
 
 Diffs
 -
 
   src/ConfigureChecks.cmake 35d7402567016c89c1fd947f44e29351aca4fe4a 
   src/config-pty.h.cmake e8dc262df0d1e6342e869fe17509b10a64baf300 
   src/kpty.cpp 145514cdee53dc81cf7453bd8d1514f173cb972e 
 
 Diff: https://git.reviewboard.kde.org/r/123811/diff/
 
 
 Testing
 ---
 
 On Linux:
 
 1. builds fine, and tests pass
 2. manually set `HAVE_TCGETATTR`  `HAVE_TCSETATTR` to `FALSE`, the `ioctl` 
 code is used as before, build is fine and so tests too 
 
 Not tested on other OSes, but the second test above should make sure there's 
 no build behaviour change if a platform does not have those functions.
 
 
 Thanks,
 
 Pino Toscano
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 118692: Fix reading of entries for language/country combinations

[Oswald Buddenhagen]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/118692/#review60351
---


i don't see that you are proactively excluding the @modifier. it would seem 
that it's effectively part of the country, and the test works only because it's 
expected to fall back to de instead of de_AT anyway.

there is another consideration ... matching follows a strict hierarchy, so an 
entry for de_DE will never be used to satisfy a request for de. arguably, 
that's correct - it becomes obvious when there is both de_DE and de_AT to 
choose from. even less, de_DE is used to satisfy a request for de_AT, which 
seems even more obvious. otoh, this stuff is mostly used for translating 
desktop files, so one can argue that being strict puts unreasonable demands on 
the population of the entries. i suggest you talk to albert.

- Oswald Buddenhagen


On June 16, 2014, 8:26 a.m., Martin Gräßlin wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/118692/
 ---
 
 (Updated June 16, 2014, 8:26 a.m.)
 
 
 Review request for KDE Frameworks, David Faure, John Layt, and Oswald 
 Buddenhagen.
 
 
 Repository: kconfig
 
 
 Description
 ---
 
 Fix reading of entries for language/country combinations
 
 This fixes a regression introduced in
 988f09bb051dca0437ecec431ee44ed5b4a560d8.
 
 The mentioned commit ensures that if the locale is e.g. de_DE the
 entry de will be used. But this breaks if there is a translation
 for another country. E.g. for de_CH it would also pick the de
 entry.
 
 This change now operates on both just the language code and the locale.
 If an entry with the language code is present it will be picked. If
 another entry with the exact locale is found it will be overwritten.
 
 
 Diffs
 -
 
   autotests/kdesktopfiletest.cpp 6c3af4c2cd55b9140c0cd48526947431ceb7e798 
   src/core/kconfig.cpp a2598f8e8fce91a8de3f34b272e15a6c280a50db 
   src/core/kconfigdata.h fdec85dc90467560bd312b72266ef0b3f243d076 
   src/core/kconfigdata.cpp 109063d65e97bcb7ba08cf6e5a6f3acb885fe845 
   src/core/kconfigini.cpp a882ee31150658f3e5cfb036362ff0583f71cbd9 
 
 Diff: https://git.reviewboard.kde.org/r/118692/diff/
 
 
 Testing
 ---
 
 unit tests still pass, but my knowledge about KConfig and locales is not 
 sufficient to be sure that this is now 100 % correct.
 
 
 Thanks,
 
 Martin Gräßlin
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 118692: Fix reading of entries for language/country combinations

[Oswald Buddenhagen]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/118692/#review60067
---



autotests/kdesktopfiletest.cpp
https://git.reviewboard.kde.org/r/118692/#comment41817

what about trying de_AT and seeing whether it correctly falls back?
what about de_AT@tirol (or whatever)?



src/core/kconfigdata.h
https://git.reviewboard.kde.org/r/118692/#comment41822

i didn't bother to re-read the whole code, so maybe that's why i can't 
claim to understand how that is supposed to work, given that you are clearly 
not explicitly comparing any such (sub)string.

also, what modifiers are possible/useful? most of them relate to encodings, 
which are totally meaningless in our context, i'd think. though i seem to 
remember something like es_ES@valencia.



src/core/kconfigini.cpp
https://git.reviewboard.kde.org/r/118692/#comment41821

this is slow. use indexOf on locale.


- Oswald Buddenhagen


On June 14, 2014, 8:24 a.m., Martin Gräßlin wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/118692/
 ---
 
 (Updated June 14, 2014, 8:24 a.m.)
 
 
 Review request for KDE Frameworks, David Faure, John Layt, and Oswald 
 Buddenhagen.
 
 
 Repository: kconfig
 
 
 Description
 ---
 
 Fix reading of entries for language/country combinations
 
 This fixes a regression introduced in
 988f09bb051dca0437ecec431ee44ed5b4a560d8.
 
 The mentioned commit ensures that if the locale is e.g. de_DE the
 entry de will be used. But this breaks if there is a translation
 for another country. E.g. for de_CH it would also pick the de
 entry.
 
 This change now operates on both just the language code and the locale.
 If an entry with the language code is present it will be picked. If
 another entry with the exact locale is found it will be overwritten.
 
 
 Diffs
 -
 
   autotests/kdesktopfiletest.cpp 6c3af4c2cd55b9140c0cd48526947431ceb7e798 
   src/core/kconfig.cpp cfa6c2071a21a642833291c9de41060a7431b765 
   src/core/kconfigdata.h e57becb24936865d009a700892893049b3e079f1 
   src/core/kconfigdata.cpp 109063d65e97bcb7ba08cf6e5a6f3acb885fe845 
   src/core/kconfigini.cpp df834f57fc44bbf9f4f28f1bc4eb5717e2ff1cee 
 
 Diff: https://git.reviewboard.kde.org/r/118692/diff/
 
 
 Testing
 ---
 
 unit tests still pass, but my knowledge about KConfig and locales is not 
 sufficient to be sure that this is now 100 % correct.
 
 
 Thanks,
 
 Martin Gräßlin
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 116699: No longer use pid_t or Q_PID

[Oswald Buddenhagen]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/116699/#review52731
---


there is QProcess::processId() since 5.3 (replacing KProcess::pid()). it simply 
returns a somewhat wasteful qint64. you may want to follow the same practice.

windows does of course not have a native getpid() function, but it is easy 
enough to replicate.

- Oswald Buddenhagen


On March 12, 2014, 7:36 a.m., Alexander Richardson wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/116699/
 ---
 
 (Updated March 12, 2014, 7:36 a.m.)
 
 
 Review request for KDE Frameworks, David Faure and Oswald Buddenhagen.
 
 
 Repository: kio
 
 
 Description
 ---
 
 No longer use pid_t or Q_PID
 
 Instead use a new typedef KIO::ProcessId. This is needed for Windows where
 pid_t doesn't exist and Q_PID is actually a pointer to a structure
 
 
 Diffs
 -
 
   src/core/global.h 7814a52c9656719d301ebd012434a53491ffe159 
   src/core/idleslave.h d3e7add4460d38212858666afa22deb3e5ef8687 
   src/core/idleslave.cpp 31d145e5d5e0c4205eb7deec6de7677061bb8a25 
   src/core/scheduler.h 593174e770d24c6ef813d91dfb6d6ed716bba4f9 
   src/core/scheduler.cpp 5a4ad310e34587249e0b50c67e011d516858e130 
   src/core/slave.h e4a443d191faefa535072d3821bb84cb6ab55909 
   src/core/slave.cpp 404a0c20f57e7c2badd7ac9c1294224d59960f7c 
   src/core/slavebase.cpp a32c6b6ad2b803e24c1db75321aefb916678c03d 
   src/core/slaveinterface.h 1ee7c1aebbf29271da605f33a5587974a4e71a5b 
   src/core/slaveinterface.cpp f8d7d52864ab6dd11316a1252fbd1e372f7f9ee2 
   src/widgets/krun.cpp 34708cc5a4b4cf3627deb750020104dd3593ef92 
   src/widgets/krun_p.h cf44e327e6d5bac0fa69b989bd6036380fc5180c 
 
 Diff: https://git.reviewboard.kde.org/r/116699/diff/
 
 
 Testing
 ---
 
 tests still pass
 
 
 Thanks,
 
 Alexander Richardson
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 109667: Make some KConfig classes return a bool when saving

[Oswald Buddenhagen]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/109667/#review34870
---


i didn't check whether every virtual override is correct, but generally the 
patch looks correct.


tier2/kconfig/autotests/kconfigguitest.cpp
http://git.reviewboard.kde.org/r/109667/#comment25551

you should wrap the line as you change the code.


- Oswald Buddenhagen


On June 22, 2013, 9:31 a.m., Albert Astals Cid wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/109667/
 ---
 
 (Updated June 22, 2013, 9:31 a.m.)
 
 
 Review request for KDE Frameworks and Oswald Buddenhagen.
 
 
 Description
 ---
 
 If it can fail, i want to know it so i can tell my users
 
 
 Diffs
 -
 
   KDE5PORTING.html ba67bdc 
   tier2/kconfig/autotests/kconfig_compiler/test_signal.cpp.ref fc04701 
   tier2/kconfig/autotests/kconfig_compiler/test_signal.h.ref f43f7dd 
   tier2/kconfig/autotests/kconfigguitest.cpp e6b4c17 
   tier2/kconfig/autotests/kconfigtest.h 17a5294 
   tier2/kconfig/autotests/kconfigtest.cpp 00ffc2a 
   tier2/kconfig/autotests/ksharedconfigtest.cpp 2bb612f 
   tier2/kconfig/src/core/kconfig.h 597330f 
   tier2/kconfig/src/core/kconfig.cpp d26f941 
   tier2/kconfig/src/core/kconfigbase.h ce190f3 
   tier2/kconfig/src/core/kconfiggroup.h ff63afa 
   tier2/kconfig/src/core/kconfiggroup.cpp 53cac13 
   tier2/kconfig/src/core/kcoreconfigskeleton.h 3e4971b 
   tier2/kconfig/src/core/kcoreconfigskeleton.cpp c0ad044 
   tier2/kconfig/src/kconfig_compiler/kconfig_compiler.cpp 47963a5 
 
 Diff: http://git.reviewboard.kde.org/r/109667/diff/
 
 
 Testing
 ---
 
 New Unitttest passes
 
 
 Thanks,
 
 Albert Astals Cid
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Finalized proposal for i18n in KF5, second iteration

[Oswald Buddenhagen]

On Mon, May 20, 2013 at 12:08:36AM +0200, Chusslove Illich wrote:
  [: Oswald Buddenhagen, 2013-04-06 :]
  the .h file postprocessing in \subsubsection link_ui is a gross hack, and
  i don't see any plausible reason why this should not be fixed in uic
  instead.
 
 I agree, but I saw it done like this so far in
 kdelibs/cmake/modules/kde4uic.cmake (lines 44-55),

historical reasons.

 For example, one could add a single option for ki18n support (which
 I think would be too specific),

yeah, no-go.

 or maybe an option to include extra headers

qdbusxml2cpp already has an -i option for that purpose. i guess that
makes a good precedent.

as for the TRANSLATION_DOMAIN define, that really should come from the
build system to start with. it's always a bad idea to use defines which
configure headers in the sources themselves (you run into include order
problems really quickly).
fwiw, this also means that the trick suggested at the bottom of
klocalizedstring.h is not such a great idea (and to start with, to avoid
that the preprocessor floods you with redefinition warnings, the #undefs
have to come first, unconditionally).

 plus an option not to emit empty-string messages.

why an option? it's fairly pointless to emit tr()s for empty messages to
start with (and an empty message with a non-empty disambiguation has a
special meaning in linguist anyway).
fwiw, since qt 4.6, ui files support the notr attribute on (almost) all
strings, so it's already possible to suppress this stupid behavior
without postprocessing, even if it would be a tad laborious.

 And then maybe one should also consider the non-i18n related
 modifications performed in kde4uic.cmake.
 
yeah. but that can be considered separately anyway.

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: please make it easier to hack on frameworks

[Oswald Buddenhagen]

On Mon, May 13, 2013 at 09:43:56PM +0200, Alexander Neundorf wrote:
 I did: 
 $ git pull
 $ git submodule update
 $ rm -rf builddir (as recommended by Ossi)
 $ mkdir build
 $ cd build
 $ ../configure -prefix $KF5 -opensource... as given in the wiki

with qmake, it is never a particularly good idea to put the builddir
inside the source dir.
however, it seems unlikely that you would get a partial build without
any error messages because of that. and you did not get any error
messages, and make exited with status zero, otherwise you'd have told
us, right?

actually, i have no other idea than an ignored build failure. it would
be quite extraordinary if it was not, even by qmake bizarreness
standards.

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: please make it easier to hack on frameworks

[Oswald Buddenhagen]

On Sun, May 05, 2013 at 11:47:04AM +0200, Alexander Neundorf wrote:
 Ok, so what is the right way to get Qt properly rebuilt ?
 
mv $builddir/config.status $safeplace
rm -rf $builddir/*
mv $safeplace $builddir/config.status
cd $builddir
./config.status
make

works also for a top-level build, except that you should keep the
top-level Makefile (as the qtbase config.status doesn't re-create it).

a no-op rebuild on my work machine this takes  5 min for qtbase, and 
15 min for all of qt (the bulk being linking webkit).

export CCACHE_HARDLINK=1 is recommended for maximum speed and reduced
disk usage.
i also use CCACHE_UNIFY=1, but that doesn't help when compiling with
debug info. it doesn't help much for qt anyway, but is a potentially big
win for kde (because the apidox are in headers).

i use the attached wrapper program to get ccache and icecream into the
build process.

i also use that for compiling kde - cmake isn't smart enough to track
dependencies of configure tests, so when i update after several weeks
(or even months nowadays) i regularly get configure-related breakages
that go away with a clean builddir. having the first attempt cached
sometimes goes a long way. and ccache is cool for testing patches anyway
(apply = revert = no-op rebuild).
/*
Copyright (C) 2013 Oswald Buddenhagen o...@kde.org

Redistribution and use in source and binary forms, with or without
modification, are permitted provided that the following conditions
are met:
1. Redistributions of source code must retain the above copyright
   notice, this list of conditions and the following disclaimer.
2. Redistributions in binary form must reproduce the above copyright
   notice, this list of conditions and the following disclaimer in the
   documentation and/or other materials provided with the distribution.
3. Neither the name of the University nor the names of its contributors
   may be used to endorse or promote products derived from this software
   without specific prior written permission.

THIS SOFTWARE IS PROVIDED BY THE REGENTS AND CONTRIBUTORS ``AS IS'' AND
ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
ARE DISCLAIMED.  IN NO EVENT SHALL THE REGENTS OR CONTRIBUTORS BE LIABLE
FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
SUCH DAMAGE.
*/

/*
  Build: gcc -Os -o wrapcc3 wrapcc3.c
  Install: copy to /usr/local/bin, symlink gcc, g++, cc, c++, make  gmake to it
  Setup:
export WRAPCCFLAGS=cache,jobs=4   # typical desktop system
export WRAPCCFLAGS=cache,ice=40   # system backed by icecream cluster
*/

#define _GNU_SOURCE
#include stdio.h
#include ctype.h
#include stdlib.h
#include unistd.h
#include string.h
#include alloca.h
#include stdarg.h
#include limits.h

static void __attribute__((noreturn))
Panic( const char *msg, ... )
{
	va_list va;

	va_start( va, msg );
	vfprintf( stderr, msg, va );
	va_end( va );
	exit( 1 );
}

static int debug;

static void
Debug( const char *msg, ... )
{
	va_list va;

	if (debug) {
		va_start( va, msg );
		vfprintf( stderr, msg, va );
		va_end( va );
	}
}

char *
findpath( const char *path, const char *name0 )
{
	const char *pathe;
	char *name, *thenam;
	int len;
	char nambuf[PATH_MAX+1], resolved[PATH_MAX+1];
	static char myself[PATH_MAX+1], directory[PATH_MAX+1];

	if (!directory[0]  !getcwd( directory, sizeof(directory) ))
		Panic( getcwd failed.\n );
	if (!myself[0]) {
		if ((unsigned)(len = readlink( /proc/self/exe, myself, sizeof(myself) ))  PATH_MAX)
			Panic( readlen /proc/self/exe failed.\n );
		myself[len] = 0;
		Debug( i am %s\n, myself );
	}
	len = strlen( name0 );
	name = nambuf + PATH_MAX - len;
	memcpy( name, name0, len + 1 );
	*--name = '/';
	do {
		if (!(pathe = strchr( path, ':' )))
			pathe = path + strlen( path );
		len = pathe - path;
		if (!len || (len == 1  *path == '.')) {
			len = strlen( directory );
			path = directory;
		}
		thenam = name - len;
		if (thenam = nambuf) {
			memcpy( thenam, path, len );
			if (!realpath( thenam, resolved ))
Debug( %s cannot be resolved\n, thenam );
			else if (access( thenam, X_OK ))
Debug( %s is not executable\n, thenam );
			else if (!strcmp( resolved, myself ))
Debug( %s is myself\n, thenam );
			else {
Debug( %s is %s\n, name0, thenam );
return strdup( thenam );
			}
		}
		path = pathe;
	} while (*path++);
	Panic( Can't find %s in $PATH.\n, name0 );
}

#define NPARTS 2

int main( int argc, char **argv )
{
	char **nargv = alloca( sizeof(*nargv) * (NPARTS + argc + 1) );
	char *p, *p2, *path, *name0, *name1, *env;
	int nargc = 0, i

Re: Review Request 110264: Change uses of QCoreApplication::translate() with no context to tr() in tier1 frameworks.

[Oswald Buddenhagen]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/110264/#review31987
---



tier1/kcodecs/src/kcharsets.h
http://git.reviewboard.kde.org/r/110264/#comment23840

you have some weird indentation here ...

repeats below



tier1/kcodecs/src/kencodingprober.cpp
http://git.reviewboard.kde.org/r/110264/#comment23841

not needed



tier1/kcodecs/src/kencodingprober.cpp
http://git.reviewboard.kde.org/r/110264/#comment23842

you could add the missing spaces around the equal signs as you are changing 
this code ...


- Oswald Buddenhagen


On May 3, 2013, 8:35 p.m., George Goldberg wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/110264/
 ---
 
 (Updated May 3, 2013, 8:35 p.m.)
 
 
 Review request for KDE Frameworks and David Faure.
 
 
 Description
 ---
 
 Change uses of QCoreApplication::translate() with no context to QObject::tr() 
 in tier1 frameworks.
 
 There are no changes needed in tier 2.
 
 Issues outstanding:
 * What do we do with the QT_TRANSLATE_NOOP_3 macro usage that we put in when 
 changing it to QCoreApplication::translate() previously?  I'd propose leaving 
 it as is, because it seems like it would still work the same, but maybe 
 that's not the neatest plan
 * kconfig/src/core/kconfig.cpp uses QCoreApplication::translate with the 
 KConfig context in one instance. Should this be left alone?
 
 
 Diffs
 -
 
   tier1/kcodecs/src/kcharsets.h a075e5348b6ca9305cf77f3d05062fed2ca5b229 
   tier1/kcodecs/src/kcharsets.cpp 45f80b609abfdd131eded0cbe91e2287973046ca 
   tier1/kcodecs/src/kencodingprober.h 
 d6a940b275317f75e53f097e25bc92ecf2fd4435 
   tier1/kcodecs/src/kencodingprober.cpp 
 3008f3ba5fc833ae999194f7d33b9a8d95170398 
   tier1/solid/src/solid/backends/fstab/fstabmanager.cpp 
 46415e0776fcef4a8576ccc1f57417f5465d5484 
   tier1/solid/src/solid/backends/hal/haldevice.cpp 
 8e433d9a96a9e15245f0c2d735950dfca8730ae6 
   tier1/solid/src/solid/backends/kupnp/internetgatewaydevice1.cpp 
 2cff30781764dcadde045fbed1f8a5ea24aa1b1f 
   tier1/solid/src/solid/backends/kupnp/kupnprootdevice.cpp 
 664e72e5fbffb246402f7f1c87132125f7d87720 
   tier1/solid/src/solid/backends/kupnp/mediaserver1.cpp 
 28e8267afefe23382898dac5c86242db41746d6f 
   tier1/solid/src/solid/backends/kupnp/mediaserver2.cpp 
 9967680429accc7fe7e45a73d65e7c33a6b308d5 
   tier1/solid/src/solid/backends/kupnp/mediaserver3.cpp 
 3805a5c52d89043046d66c2aaaf01b3699e5a13f 
   tier1/solid/src/solid/backends/udev/udevdevice.cpp 
 e00c37c816cb1c7c94a3460341f5b0b13088eb3a 
   tier1/solid/src/solid/backends/udev/udevmanager.cpp 
 1446868cca7a42155478be0dc75be2fd47725e7f 
   tier1/solid/src/solid/backends/udisks/udisksdevice.cpp 
 e1c47afc952bf232b9fce1ffe053cce1b3b80a67 
   tier1/solid/src/solid/backends/udisks/udisksmanager.cpp 
 31e581656d98c0e103d6ceb605b3b0bfb4b00a54 
   tier1/solid/src/solid/backends/udisks2/udisksdevice.cpp 
 1aa7b6d139be3ba5dc6d78e1ae55b2d0841dbfbd 
   tier1/solid/src/solid/backends/udisks2/udisksmanager.cpp 
 e76dfd1c94b39009ae4522ce9d3f535fc2c5eac2 
   tier1/solid/src/solid/backends/upnp/upnpdevicemanager.cpp 
 e3cf29954da081bb5f23b859310f4f5a57e2702c 
   tier1/solid/src/solid/backends/upower/upowerdevice.cpp 
 c61bee940cf5e8295d99f055582317e7e626eedd 
   tier1/solid/src/solid/backends/upower/upowermanager.cpp 
 ee198a0ae0da44e35cc911e9fd830a78241bc867 
   tier1/solid/src/solid/backends/wmi/wmibattery.cpp 
 4b72f80514afa7fd7bbfe706d231ec669ed2f8d4 
   tier1/solid/src/solid/backends/wmi/wmidevice.cpp 
 0e85ae9c31eaab8c863e86845733dff17453456a 
   tier1/solid/src/solid/deviceinterface.cpp 
 0cfc8e84ddbcbf31b44e881055ae173a6c589fdb 
 
 Diff: http://git.reviewboard.kde.org/r/110264/diff/
 
 
 Testing
 ---
 
 Everything still compiles.
 
 
 Thanks,
 
 George Goldberg
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Finalized proposal for i18n in KF5, second iteration

[Oswald Buddenhagen]

On Tue, Apr 02, 2013 at 03:46:52PM +0200, Chusslove Illich wrote:
 After considering comments from the previous iteration:
 
   http://mail.kde.org/pipermail/kde-frameworks-devel/2012-December/001358.html
 
 here is the updated version:
 
just a quick scan of the diffs from me:

the .h file postprocessing in \subsubsection link_ui is a gross hack,
and i don't see any plausible reason why this should not be fixed in
uic instead.

both the api and the docs still refer to catalogs in many places. while
it may make sense to keep talking about catalogs when referring
specifically to files, i have the impression that the use is not
entirely consistent.

+ * Instructions for building the configuration code from [a] \c .kcfg file

style issues:
- in all your function prototypes/definitions, you are putting a space
  after the function name
- in else constructs you are not attaching the else to the closing
  brace
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 109551: port KPtyProcess to QProcess

[Oswald Buddenhagen]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/109551/#review29802
---


after extensive deliberation of the code i found that ... i have no clue what 
is wrong.
you'll need to find out yourself (with strace -f -tt).

one notable difference between kprocess and qprocess is that the output channel 
mode default differs (kprocess forwards, while qprocess collects). however, i 
wasn't able to come up with an obvious explanation how that would correlate 
with your hangs.

- Oswald Buddenhagen


On March 18, 2013, 7:54 p.m., Martin Tobias Holmedahl Sandsmark wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/109551/
 ---
 
 (Updated March 18, 2013, 7:54 p.m.)
 
 
 Review request for KDE Frameworks, kdelibs, David Faure, and Oswald 
 Buddenhagen.
 
 
 Description
 ---
 
 Just a simple port of KPtyProcess away from using KProcess.
 
 
 Diffs
 -
 
   kpty/kptyprocess.h 5e0df96 
   kpty/kptyprocess.cpp 015a58c 
   kpty/tests/kptyprocesstest.cpp 04990a0 
 
 Diff: http://git.reviewboard.kde.org/r/109551/diff/
 
 
 Testing
 ---
 
 builds and tests pass.
 
 
 Thanks,
 
 Martin Tobias Holmedahl Sandsmark
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 109667: Make some KConfig classes return a bool when saving

[Oswald Buddenhagen]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/109667/#review29769
---



KDE5PORTING.html
http://git.reviewboard.kde.org/r/109667/#comment22159

typo



tier2/kconfig/src/core/kconfig.cpp
http://git.reviewboard.kde.org/r/109667/#comment22160

well, it appears to fit the pre-existing logic. any particular reason why 
you are not sure?


- Oswald Buddenhagen


On March 23, 2013, 3:47 p.m., Albert Astals Cid wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/109667/
 ---
 
 (Updated March 23, 2013, 3:47 p.m.)
 
 
 Review request for KDE Frameworks and Oswald Buddenhagen.
 
 
 Description
 ---
 
 If it can fail, i want to know it so i can tell my users
 
 Not totally sure of the
d-bDirty = true;
 i added in 
if (d-configState == ReadWrite  !tmp-lock()) {
 
 
 Diffs
 -
 
   KDE5PORTING.html a01f3ec 
   tier2/kconfig/autotests/kconfigguitest.cpp e6b4c17 
   tier2/kconfig/autotests/kconfigtest.h 17a5294 
   tier2/kconfig/autotests/kconfigtest.cpp 86ff718 
   tier2/kconfig/autotests/ksharedconfigtest.cpp 2bb612f 
   tier2/kconfig/src/core/kconfig.h 597330f 
   tier2/kconfig/src/core/kconfig.cpp 5809275 
   tier2/kconfig/src/core/kconfigbase.h ce190f3 
   tier2/kconfig/src/core/kconfiggroup.h ff63afa 
   tier2/kconfig/src/core/kconfiggroup.cpp 53cac13 
   tier2/kconfig/src/core/kcoreconfigskeleton.h 3e4971b 
   tier2/kconfig/src/core/kcoreconfigskeleton.cpp c0ad044 
 
 Diff: http://git.reviewboard.kde.org/r/109667/diff/
 
 
 Testing
 ---
 
 New Unitttest passes
 
 
 Thanks,
 
 Albert Astals Cid
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 109551: port KPtyProcess to QProcess

[Oswald Buddenhagen]

 On March 18, 2013, 10:04 p.m., Oswald Buddenhagen wrote:
  kpty/tests/kptyprocesstest.cpp, line 193
  http://git.reviewboard.kde.org/r/109551/diff/2/?file=120310#file120310line193
 
  i don't think eating the sleep is a good idea. i'm sure i added it for 
  a reason (in a previous life ^^).
 
 Martin Tobias Holmedahl Sandsmark wrote:
 with the sleep the test fails, and even with high load I can't get the 
 test to fail without it.

hmm, that means that something is actually broken in the port ... i'll need to 
look at it a bit more thoroughly. don't expect this to happen before the 
weekend.


 On March 18, 2013, 10:04 p.m., Oswald Buddenhagen wrote:
  kpty/tests/kptyprocesstest.cpp, line 210
  http://git.reviewboard.kde.org/r/109551/diff/2/?file=120310#file120310line210
 
  the -c needs to be a separate argument.
  
  the quotes, backslashes and attempt at a newline are all garbage.
 
 Martin Tobias Holmedahl Sandsmark wrote:
 I first had -c as a separate argument, and it didn't matter. And I had to 
 add an extra newline to get it to work with doing it manually.

well, i'm just telling you how it should be, i.e., what setShellCommand() did. 
if that doesn't work, we have a real problem - most likely related to the other 
failure.


- Oswald


---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/109551/#review29478
---


On March 18, 2013, 7:54 p.m., Martin Tobias Holmedahl Sandsmark wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/109551/
 ---
 
 (Updated March 18, 2013, 7:54 p.m.)
 
 
 Review request for KDE Frameworks, kdelibs, David Faure, and Oswald 
 Buddenhagen.
 
 
 Description
 ---
 
 Just a simple port of KPtyProcess away from using KProcess.
 
 
 Diffs
 -
 
   kpty/kptyprocess.h 5e0df96 
   kpty/kptyprocess.cpp 015a58c 
   kpty/tests/kptyprocesstest.cpp 04990a0 
 
 Diff: http://git.reviewboard.kde.org/r/109551/diff/
 
 
 Testing
 ---
 
 builds and tests pass.
 
 
 Thanks,
 
 Martin Tobias Holmedahl Sandsmark
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 109551: port KPtyProcess to QProcess

[Oswald Buddenhagen]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/109551/#review29421
---



kpty/tests/kptyprocesstest.cpp
http://git.reviewboard.kde.org/r/109551/#comment21968

why should they? you already have the solution in the previous hunk.


- Oswald Buddenhagen


On March 17, 2013, 4:44 p.m., Martin Tobias Holmedahl Sandsmark wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/109551/
 ---
 
 (Updated March 17, 2013, 4:44 p.m.)
 
 
 Review request for KDE Frameworks, kdelibs and David Faure.
 
 
 Description
 ---
 
 Just a simple port of KPtyProcess away from using KProcess.
 
 
 Diffs
 -
 
   kpty/kptyprocess.h 5e0df96 
   kpty/kptyprocess.cpp 015a58c 
   kpty/tests/kptyprocesstest.h 64bded0 
   kpty/tests/kptyprocesstest.cpp 04990a0 
 
 Diff: http://git.reviewboard.kde.org/r/109551/diff/
 
 
 Testing
 ---
 
 builds and tests pass.
 
 
 Thanks,
 
 Martin Tobias Holmedahl Sandsmark
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 109551: port KPtyProcess to QProcess

[Oswald Buddenhagen]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/109551/#review29478
---



kpty/tests/kptyprocesstest.cpp
http://git.reviewboard.kde.org/r/109551/#comment21999

i don't think eating the sleep is a good idea. i'm sure i added it for a 
reason (in a previous life ^^).



kpty/tests/kptyprocesstest.cpp
http://git.reviewboard.kde.org/r/109551/#comment21997

because it's completely broken ^^



kpty/tests/kptyprocesstest.cpp
http://git.reviewboard.kde.org/r/109551/#comment21998

the -c needs to be a separate argument.

the quotes, backslashes and attempt at a newline are all garbage.


- Oswald Buddenhagen


On March 18, 2013, 7:54 p.m., Martin Tobias Holmedahl Sandsmark wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/109551/
 ---
 
 (Updated March 18, 2013, 7:54 p.m.)
 
 
 Review request for KDE Frameworks, kdelibs, David Faure, and Oswald 
 Buddenhagen.
 
 
 Description
 ---
 
 Just a simple port of KPtyProcess away from using KProcess.
 
 
 Diffs
 -
 
   kpty/kptyprocess.h 5e0df96 
   kpty/kptyprocess.cpp 015a58c 
   kpty/tests/kptyprocesstest.cpp 04990a0 
 
 Diff: http://git.reviewboard.kde.org/r/109551/diff/
 
 
 Testing
 ---
 
 builds and tests pass.
 
 
 Thanks,
 
 Martin Tobias Holmedahl Sandsmark
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 108385: Reduce risk of timeout and race condition in KPtyProcessTest

[Oswald Buddenhagen]

 On Jan. 13, 2013, 6:45 p.m., Oswald Buddenhagen wrote:
  and why exactly do you sleep instead of looping with waitforreadyread?
 
 Jon Severinsson wrote:
 Because that would be an (almost) busy-loop (there are already *some* 
 data, so waitForReadyRead could return before the timeout).
 
 Oswald Buddenhagen wrote:
 right. my (kpty) implementation waits for *any* data to be available 
 (which is (or was) consistent with something i don't remember), while 
 thiago's (qsocket  co.) implementations wait for *more* data to be 
 available. this really should be made consistent at some point ... at this 
 point i think thiago's interpretation is more useful, even if it means 
 writing more code in the common case.
 
 Jon Severinsson wrote:
 I have not looked too closely at either implementation, and can't say I 
 really understand how either actually work, so this might be pointless, but I 
 would still want to give a warning about the whole waiting for *more* data 
 (as opposed to waiting for *some* data) concept, as that immediately brings 
 to question more since *when*?.
 
 More data since the waitForReadyRead() call *can* *not* be used 
 correctly. Never under any circumstance!  Every call would be a race 
 condition (all data that is ever going to come might have come just before 
 you made the call), and would at best result in an unnecessary sleep of 
 timeout ms, and at worst result in a complete deadlock, or if the rest of 
 the system conspires against you, an infinite loop.
 
 More data since last relevant API call prior to waitForReadyRead() 
 could work, but defining relevant, and getting both the implementation and 
 documentation right would likely be a nightmare, and chances a API consumer 
 makes a mistake is still possible, even likely, so this is not something I 
 would recommend.
 
 So imho waiting for *some* data is the only right thing to do, even 
 though it results in this ugly code in some cases...

no. the actual querying of the data source is synchronous as far as threading 
is concerned: you either do it via waitFor*() or by returning to the event 
loop. consequently, code like if (!dev-bytesAvailable()) 
dev-waitForReadyRead(); is entirely well-defined. the waitFor*() functions 
are thus defined to mean synchronously wait for the emission of the likewise 
named signal.


- Oswald


---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/108385/#review25394
---


On Jan. 13, 2013, 1:03 p.m., Jon Severinsson wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/108385/
 ---
 
 (Updated Jan. 13, 2013, 1:03 p.m.)
 
 
 Review request for KDE Frameworks.
 
 
 Description
 ---
 
 Increase timeout, and sleep a while after waitForReadyRead() returns,
 as it only guarantees that *some* data is available to read, while
 the test assumes that a full line of data is available to read...
 
 This reduces failure rate from 10% to 2% on my setup.
 
 
 Diffs
 -
 
   kpty/tests/kptyprocesstest.cpp b95ae26 
 
 Diff: http://git.reviewboard.kde.org/r/108385/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Jon Severinsson
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 108385: Reduce risk of timeout and race condition in KPtyProcessTest

[Oswald Buddenhagen]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/108385/#review25394
---


and why exactly do you sleep instead of looping with waitforreadyread?

- Oswald Buddenhagen


On Jan. 13, 2013, 1:03 p.m., Jon Severinsson wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/108385/
 ---
 
 (Updated Jan. 13, 2013, 1:03 p.m.)
 
 
 Review request for KDE Frameworks.
 
 
 Description
 ---
 
 Increase timeout, and sleep a while after waitForReadyRead() returns,
 as it only guarantees that *some* data is available to read, while
 the test assumes that a full line of data is available to read...
 
 This reduces failure rate from 10% to 2% on my setup.
 
 
 Diffs
 -
 
   kpty/tests/kptyprocesstest.cpp b95ae26 
 
 Diff: http://git.reviewboard.kde.org/r/108385/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Jon Severinsson
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Finalized proposal for changes to i18n in KF5

[Oswald Buddenhagen]

On Tue, Jan 08, 2013 at 03:05:25PM +0100, Chusslove Illich wrote:
  [: Chusslove Illich :]
  I'm not opposed to some additional bureaucracy in order to make the
  framework more accessible to potential users. [...]
 
  [: Oswald Buddenhagen :]
  [...] i'd hate it to see my thoroughly engineered code being displaced by
  a (possibly even slightly inferior) competitor just because i can't
  compete on licensing compatibility terms.
 
 One step back: who exactly would find KDE Frameworks licensing terms non-
 workable? I can't say I care what a party for which none of the options at
 http://techbase.kde.org/Policies/Licensing_Policy works will do. By more
 accessible I meant in technical and organizational terms.
 
well, the mit/bsd/x11 licenses are in principle ok (though i suspect the
users in question still prefer CLA'd code, as then they have less legal
work, as it all goes under the contract they have with digia anyway).
it's probably more of a marketing problem to point out but this kde
framework - unlike most others - comes with a no-strings-attached
license, and thus you can use it as a licensing-neutral dependency of
your frameworks.

 In fact, PO comments unfortunately have no multi-line semantics so the
 editor must present them as is, while it can rewrap PO contexts
 according to user's set width.
 
interesting. for .ts comments (//: blah), i just defined that it is
flowed text.
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Finalized proposal for changes to i18n in KF5

[Oswald Buddenhagen]

On Sat, Jan 05, 2013 at 06:38:58PM +0100, Chusslove Illich wrote:
  [: Oswald Buddenhagen :]
  of course, it would be even better if you strived for submission to qt-
  project, if at all realistic (for now probably an add-on, but definitely
  under cla). otherwise you'll see the same effect every other useful lgpl'd
  qt framework sees sonner or later: it gets re-implemented (if the effort
  is deemed acceptable by an interested party).
 
 I'm not opposed to some additional bureaucracy in order to make the
 framework more accessible to potential users. But I'd have to see what it
 actually means, and what could be the tradeoff.
 
well, the bureaucracy is finding all code which is not fully copyrighted
by you, and determining whether the authors can be reached and agree to
signing the CLA. if not, rewrite the code, or drop the idea.

 As for another party reimplementating the framework, I don't see what factor
 is that.

well, it's a personal factor. i'd hate it to see my thoroughly
engineered code being displaced by a (possibly even slightly inferior)
competitor just because i can't compete on licensing compatibility
terms.
this is of course a challenge that all of kde frameworks will face, and
all but the most specialized+big ones will lose in the longer term - as
they always have.

 (Hypothetically speaking, though, I don't see it happening: if someone
 wants Gettext-based translation in Qt code but not through Ki18n, I
 expect he will, well, use Gettext directly.)
 
nobody wants gettext as such (only setting up a compatible workflow is
important, and that mostly only in the FOSS world). i couldn't care
less, and i even see it as an disincentive (because i have no direct
control over the data formats and tools, and even if i can change
something, there is the problem of slow independent distribution of
these newer versions).
the real worth of your work (and majority of effort, i suppose) lies in
all that semantic and scripting stuff, including the documentation
(which implies standardization on sane guidelines).

  [...] make klocalizedstring.h #undef TRANSLATION_CATALOG at the end (may
  need to use a macro indirection to ensure that the macro is fully expanded
  already at definition time (see QT_STRINGIFY in qt 5)).
 
 I looked, but couldn't figure out how to use it in this context.
 
#define RESOLVE_TRANSLATION_CATALOG(c) (c)
#define i18n(...) i18nd(RESOLVE_TRANSLATION_CATALOG(TRANSLATION_CATALOG), 
__VA_ARGS__)
#undef RESOLVE_TRANSLATION_CATALOG
#undef TRANSLATION_CATALOG

i'm not sure this actually works ...

 I'd definitelly like to have the version markers visible for all elements.
 For example, so that there is no uncertainty whether a marker was forgotten
 or not.
 
 I struggled with how to present it, and in particular thought that a
 separate colon is an overkill. Maybe have the superscript yet smaller and a
 bit dimmer?
 
dunno. not sure what vision-impaired people will say to that.
but a separate column is definitely easier to (visually) skip over than
some appendage to the actual items. especially if you put that column
last.

  and as usual for native-only-in-slavic speakers, some thes are missing.
  i was too lame to record their locations. ^^
 
 I've given up and put a pox on them.
 
 (I did toy with another idea though: compute the statistical average of
 the's-per-word for a given class of texts, and then pepper my text
 proportionally.)
 
lol

  i don't like the recommendation for extracted vs. disambiguating comments
  (and closed-source authors will typically do the exact opposite anyway).
  wouldn't it be sufficient for disambiguiation to strongly recommend
  consistent use of user interface markers, and thus allow all comments to
  be extracted?

  the matter of flagging changes is merely tooling-related.
 
 Yes, but tooling decisions are related to PO convention and workflow.
 There'd be awful lot of tooling to modify, and modify by adding
 options and not changeing the default behavior.

the change is trivial: just add a flag to indicate that the comment
changed. the backwards compatible variant would be just (ab)using fuzzy
for that (e.g., set fuzzy,fuzzcmt, so updated tools see these soft
fuzzies, while older tools just treat them as normal fuzzies).

 There would also be no practical purpose to having both types of
 contexts, unless there was a significant difference between them.
 
well, proprietary users don't like exposing their comments, so they want
minimal disambiguation (what the end user will see in the ui anyway).
also, getting people into the habit of defaulting to comments (paired
with consistent use of @markers) makes it less likely that they put an
epic into a disambiguation when they really meant to put it into a
comment (which is easier to edit and produces leaner catalogs).
of course, sometimes the line between comment and disambiguation is a
tad thin (think annotating %1: %2), so either variant doesn't work
entirely without thinking...

  one thing i

Re: Review Request: port Sonnet to QSettings

[Oswald Buddenhagen]

 On Dec. 17, 2012, 10:38 p.m., Kevin Krammer wrote:
  IMHO this is wrong.
  Not code wise but conceptual. As far as I understand QSettings is basically 
  deprecated, it is just not official marked as such because there is no 
  replacement. This would be porting away from a fully functional, way more 
  advanced and maintained settings.
  
  If the sole goal of this endavor is to remove the KConfig dependency than 
  this ought to be done by either having an interface that can be implemented 
  through KConfig or by passing values as QVariant maps or hashes.
 
 
 Oswald Buddenhagen wrote:
 and where exactly do you see that kconfig maintainer? ;)
 
 it's unfortunate that the chosen config class is part of the API.
 judging by uses, would it be reasonable to just disable that part of the 
 API indefinitely?
 less drastically, would it be acceptable to pass a file name instead of a 
 config object? that would of course incur some overhead (assuming we are 
 passing the application's main config file).
 
 Kevin Krammer wrote:
 it's unfortunate that the chosen config class is part of the API.
 
 Indeed. Most likely out of convenience. Hence the idea to just replace it 
 with a generic key/value object that doesn't do any specific form of I/O and 
 can allowing the using application to decide on persistant storage. But if I 
 understand you correctly you would rather go for the full solution and make 
 those properties directly read-/writable, right?

the idea with the file name has the advantage that it is most natural, but sort 
of slow, and it may actually not work - if the app uses kconfig, but sonnet 
uses qsettings on the same file, you may actually get garbage out of it.

i'd strongly recommend not using a variant map, etc., as using it would require 
lots of boilerplate code.

if you make it so that instantiating is nothing else than
  new Sonnet::ConfigDialog(new KConfigWrapper(new 
KConfigGroup(KGlobal::config(), Spellchecking)));
it may be ok. still a bit ... weird.
you could make kconfiggroup directly implement the interface, but then you'd 
get an asymmetry with qsettings.
also, where would KMapInterface be defined? where would the qsettings wrapper 
live?
or maybe upstream QMapInterface and make QSettings implement it, too? would it 
even fit the API?


- Oswald


---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/107791/#review23643
---


On Dec. 17, 2012, 9:15 p.m., Martin Tobias Holmedahl Sandsmark wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/107791/
 ---
 
 (Updated Dec. 17, 2012, 9:15 p.m.)
 
 
 Review request for KDE Frameworks, kdelibs and David Faure.
 
 
 Description
 ---
 
 Ported everything away from KConfig to QSettings.
 
 I couldn't really find any users of the ::save function, so I think the 
 source incompatible change would be worth it. Alternatively we could add a 
 deprecated dummy function that takes in a KConfig object and just ignores it, 
 and uses QSettings.
 
 
 Diffs
 -
 
   kdeui/sonnet/configdialog.h 7c4993b 
   kdeui/sonnet/configdialog.cpp 625441b 
   kdeui/sonnet/configwidget.h 023b659 
   kdeui/sonnet/configwidget.cpp 549d5af 
   kdeui/sonnet/highlighter.cpp 6cbb14c 
   kdeui/widgets/ktextedit.cpp 71d2a9f 
   staging/sonnet/src/core/backgroundchecker.h f0da3a3 
   staging/sonnet/src/core/backgroundchecker.cpp dc05b94 
   staging/sonnet/src/core/globals.cpp bf4f504 
   staging/sonnet/src/core/loader.cpp 887aee5 
   staging/sonnet/src/core/settings.cpp 59cb593 
   staging/sonnet/src/core/settings_p.h e14bad7 
   staging/sonnet/src/core/speller.h 37dd82f 
   staging/sonnet/src/core/speller.cpp f831f55 
 
 Diff: http://git.reviewboard.kde.org/r/107791/diff/
 
 
 Testing
 ---
 
 it builds.
 
 
 Thanks,
 
 Martin Tobias Holmedahl Sandsmark
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request: Port some uses of KProcess to QProcess

[Oswald Buddenhagen]

On Sat, Sep 22, 2012 at 12:42:26PM +0200, David Faure wrote:
 On Thursday 20 September 2012 22:02:03 Oswald Buddenhagen wrote:
  yes. a rather suboptimal replacement. before you ask, search the core-devel
  archive shortly before the time the new kprocess was added.
 
 I can't do *everything* in the KDE Frameworks 5 effort.
 
no. but then, you don't have to kill everything which is small but
still wasn't upstreamed, either. at the very least, you can keep it as
an internal class.

 Both QProcess and KProcess are mostly your code, aren't they?

kprocess is mine. i made only few contributions to qprocess.

 How about you fix QProcess then, if you consider it suboptimal?
 
priorities. ;)
(which is ironic, as i joined trolltech because i wanted to work on
qprocess :}).
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request: Port some uses of KProcess to QProcess

[Oswald Buddenhagen]

 On Sept. 19, 2012, 5:11 p.m., Oswald Buddenhagen wrote:
  every single change of this patch is a regression
 
 David Faure wrote:
 Very encouraging, as always. Care to give us more details?

the reasons for kprocess' existence didn't go away, so every change away from 
it is by definition a regression, either in functionality or maintainablity. 
and the consistent replacement of setOutputChannelMode() with setReadChannel() 
is so bizarre that one has to wonder what i wrote the apidoc for in the first 
place.

in case somebody is interested, i still have the (rather unfinished) qprocess 
patches i wrote five years ago to address (some of) the issues upstream. it 
would be qt 5.1 material, obviously.


- Oswald


---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/106500/#review19174
---


On Sept. 18, 2012, 11:12 p.m., Kevin Funk wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/106500/
 ---
 
 (Updated Sept. 18, 2012, 11:12 p.m.)
 
 
 Review request for KDE Frameworks.
 
 
 Description
 ---
 
 Port some uses of KProcess to QProcess
 
 
 Diffs
 -
 
   kdeui/dialogs/kedittoolbar.cpp 0582934b0bf8cb7160cb48b4c8151c81b35277f0 
   kinit/klauncher.cpp 855e56041be5a5b76b9a7e9d0597ac7ad485682e 
   kio/kfile/kfilemetadatareader.cpp 88cadaa2edf1b1de24c0e91576cca368db41f470 
   kio/kio/krun.h 7bfe66b59f1deffc37d3ceae999fb929e453fd31 
   kio/kio/krun.cpp 031dbc1dfef685729038b4a59cbeacd34d448ed2 
   kio/kio/krun_p.h 0ad15c8434599ccabcd649f251aa622d4fb0b0f7 
   staging/kwidgets/autotests/kglobalsettingstest.cpp 
 4426fee08427499c777cc7fc94e4b1345c790ac2 
 
 Diff: http://git.reviewboard.kde.org/r/106500/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Kevin Funk
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request: Port some uses of KUrl to QUrl

[Oswald Buddenhagen]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/106501/#review19241
---



kio/tests/krununittest.cpp
http://git.reviewboard.kde.org/r/106501/#comment15262

this whole hunk doesn't belong into this patch at all. one part of it 
belongs into the kprocess patch, the other (the kdesuExe part) is a commit of 
its own.


- Oswald Buddenhagen


On Sept. 19, 2012, 10:19 p.m., Kevin Funk wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/106501/
 ---
 
 (Updated Sept. 19, 2012, 10:19 p.m.)
 
 
 Review request for KDE Frameworks.
 
 
 Description
 ---
 
 Port some uses of KUrl to QUrl
 
 
 kio: Cleanup header a bit
 
 
 kio: Merge some functions using default arguments
 
 
 kio: Mark test with QSKIP_PORTING
 
 
 Diffs
 -
 
   kio/kio/krun.h 7bfe66b59f1deffc37d3ceae999fb929e453fd31 
   kio/kio/krun.cpp 031dbc1dfef685729038b4a59cbeacd34d448ed2 
   kio/tests/kruntest.h 6f5eae6147e2fbef25025976a17869af4d0f12f9 
   kio/tests/kruntest.cpp 92496bd1d4b98ea8f7dc13977cd3d2aa0dae7145 
   kio/tests/krununittest.cpp 314da79b9ee4f95bcc9f768a95810f7de3125ac1 
 
 Diff: http://git.reviewboard.kde.org/r/106501/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Kevin Funk
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request: Port some uses of KProcess to QProcess

[Oswald Buddenhagen]

 On Sept. 19, 2012, 5:11 p.m., Oswald Buddenhagen wrote:
  every single change of this patch is a regression
 
 David Faure wrote:
 Very encouraging, as always. Care to give us more details?
 
 Oswald Buddenhagen wrote:
 the reasons for kprocess' existence didn't go away, so every change away 
 from it is by definition a regression, either in functionality or 
 maintainablity. and the consistent replacement of setOutputChannelMode() with 
 setReadChannel() is so bizarre that one has to wonder what i wrote the apidoc 
 for in the first place.
 
 in case somebody is interested, i still have the (rather unfinished) 
 qprocess patches i wrote five years ago to address (some of) the issues 
 upstream. it would be qt 5.1 material, obviously.
 
 David Faure wrote:
 That reasoning assumes that we need the additional-features-in-KProcess 
 everywhere KProcess is used, which is just not the case.
 
 See commits ed71a84ca2178865d947169a8f35a025c708a2ef and 
 66fda6d3faafeadac82eae6b8308787b3fe96911 which already ported some KProcess 
 to QProcess, and which did not introduce regressions (most are in unittests, 
 which still pass).
 
 You're right about setOutputChannelMode, though -- apparently the proper 
 replacement is QProcess::setProcessChannelMode.
 
 Still I don't see why we can't use QProcess in its current form... like 
 everything it could be improved, but surely all other Qt apps manage just 
 fine?

ed71a84ca2178 is arguably broken.

yes. a rather suboptimal replacement. before you ask, search the core-devel 
archive shortly before the time the new kprocess was added.

other apps managed with motif just fine, too. doesn't mean the users liked the 
outcome.


- Oswald


---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/106500/#review19174
---


On Sept. 18, 2012, 11:12 p.m., Kevin Funk wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/106500/
 ---
 
 (Updated Sept. 18, 2012, 11:12 p.m.)
 
 
 Review request for KDE Frameworks.
 
 
 Description
 ---
 
 Port some uses of KProcess to QProcess
 
 
 Diffs
 -
 
   kdeui/dialogs/kedittoolbar.cpp 0582934b0bf8cb7160cb48b4c8151c81b35277f0 
   kinit/klauncher.cpp 855e56041be5a5b76b9a7e9d0597ac7ad485682e 
   kio/kfile/kfilemetadatareader.cpp 88cadaa2edf1b1de24c0e91576cca368db41f470 
   kio/kio/krun.h 7bfe66b59f1deffc37d3ceae999fb929e453fd31 
   kio/kio/krun.cpp 031dbc1dfef685729038b4a59cbeacd34d448ed2 
   kio/kio/krun_p.h 0ad15c8434599ccabcd649f251aa622d4fb0b0f7 
   staging/kwidgets/autotests/kglobalsettingstest.cpp 
 4426fee08427499c777cc7fc94e4b1345c790ac2 
 
 Diff: http://git.reviewboard.kde.org/r/106500/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Kevin Funk
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request: Port some uses of KProcess to QProcess

[Oswald Buddenhagen]

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/106500/#review19174
---


every single change of this patch is a regression

- Oswald Buddenhagen


On Sept. 18, 2012, 11:12 p.m., Kevin Funk wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/106500/
 ---
 
 (Updated Sept. 18, 2012, 11:12 p.m.)
 
 
 Review request for KDE Frameworks.
 
 
 Description
 ---
 
 Port some uses of KProcess to QProcess
 
 
 Diffs
 -
 
   kdeui/dialogs/kedittoolbar.cpp 0582934b0bf8cb7160cb48b4c8151c81b35277f0 
   kinit/klauncher.cpp 855e56041be5a5b76b9a7e9d0597ac7ad485682e 
   kio/kfile/kfilemetadatareader.cpp 88cadaa2edf1b1de24c0e91576cca368db41f470 
   kio/kio/krun.h 7bfe66b59f1deffc37d3ceae999fb929e453fd31 
   kio/kio/krun.cpp 031dbc1dfef685729038b4a59cbeacd34d448ed2 
   kio/kio/krun_p.h 0ad15c8434599ccabcd649f251aa622d4fb0b0f7 
   staging/kwidgets/autotests/kglobalsettingstest.cpp 
 4426fee08427499c777cc7fc94e4b1345c790ac2 
 
 Diff: http://git.reviewboard.kde.org/r/106500/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Kevin Funk
 


___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel