D27999: [DesktopExecParser] Open {ssh, telnet, rlogin}:// urls with ktelnetservice

[Alexander Saoutkin]

feverfew added a comment.


  don't merge yet, looking at the code I sense something might break here but I 
need some time to do my own testing first.

REPOSITORY
  R241 KIO

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

To: ahmadsamir, #frameworks, dfaure, sitter, meven, feverfew
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27999: [DesktopExecParser] Open {ssh, telnet, rlogin}:// urls with ktelnetservice

[Alexander Saoutkin]

feverfew added a reviewer: ngraham.
feverfew accepted this revision.
feverfew added a subscriber: ngraham.
feverfew added a comment.


  Quick testing with `fish` protocol doesn't break anything KIOFuse side. Going 
to page in @ngraham, if it works for him with `smb` it's an all good from the 
"KIOFuse people" ;)
  
  Sorry for the delay!

REPOSITORY
  R241 KIO

BRANCH
  l-krun-ssh (branched from master)

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

To: ahmadsamir, #frameworks, dfaure, sitter, meven, feverfew, ngraham
Cc: ngraham, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, bruns

D28440: Mark KIOFuse mounts as Probably slow

[Alexander Saoutkin]

feverfew created this revision.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
feverfew requested review of this revision.

REVISION SUMMARY
  Mark KIOFuse mounts as "Probably Slow". Note that this isn't always true,
  there are plenty of slaves that simply refer to local files that can be
  mounted in a KIOFuse mounts, but likewise, there are many slaves that
  refer to network mounts that are slow, so better safe than sorry. And in
  any case, the function is "probablySlow()", we don't have to confirm that
  this is actually the case.

TEST PLAN
  Compiles.
  KIOFuse mounts indeed have a mount type of "fuse.kio-fuse"

REPOSITORY
  R241 KIO

BRANCH
  slowKIOFUse (branched from master)

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

AFFECTED FILES
  src/core/kmountpoint.cpp

To: feverfew
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28440: Mark KIOFuse mounts as Probably slow

[Alexander Saoutkin]

feverfew added reviewers: Frameworks, dfaure, broulik, bruns.

REPOSITORY
  R241 KIO

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

To: feverfew, #frameworks, dfaure, broulik, bruns
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28440: Mark KIOFuse mounts as Probably slow

[Alexander Saoutkin]

This revision was automatically updated to reflect the committed changes.
Closed by commit R241:ec5adbccc23d: Mark KIOFuse mounts as Probably slow 
(authored by feverfew).

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28440?vs=78907&id=78920

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

AFFECTED FILES
  src/core/kmountpoint.cpp

To: feverfew, #frameworks, dfaure, broulik, bruns, meven
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28520: Fix lifetime of slot in KIO-MTP

[Alexander Saoutkin]

feverfew created this revision.
feverfew added reviewers: akrutzler, dfaure, elvisangelaccio.
Herald added projects: Dolphin, Frameworks.
Herald added subscribers: kfm-devel, kde-frameworks-devel.
feverfew requested review of this revision.

REVISION SUMMARY
  This slot was incorrectly made to last the lifetime of the slave. In fact, 
this
  slot should only be live for the lifetime of the event loop (one I/O 
operation).
  
  This can cause issues with the data being emitted for data from other IO 
  operations. By making it live in the lifetime of the local event loop this 
  scenario cannot occur as LIBMTP only allows one operation at a time.

TEST PLAN
  Via the use of KIOFuse (as this is where the bug was noticed). 
  Before: Opening a file for the first time worked. Subsequent opens would fail 
  due to garbage output After: Opening any number of files at any time works.

REPOSITORY
  R320 KIO Extras

BRANCH
  slotLifetime (branched from master)

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

AFFECTED FILES
  mtp/kio_mtp.cpp

To: feverfew, akrutzler, dfaure, elvisangelaccio
Cc: kde-frameworks-devel, kfm-devel, fvogt, nikolaik, pberestov, iasensio, 
fprice, LeGast00n, cblack, fbampaloukas, alexde, GB_2, Codezela, feverfew, 
meven, michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, bruns, 
emmanuelp, mikesomov

D28520: Fix lifetime of slot in KIO-MTP

[Alexander Saoutkin]

feverfew updated this revision to Diff 79196.
feverfew added a comment.


  - Fix another slot lifetime

REPOSITORY
  R320 KIO Extras

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28520?vs=79154&id=79196

BRANCH
  slotLifetime (branched from master)

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

AFFECTED FILES
  mtp/kio_mtp.cpp

To: feverfew, akrutzler, dfaure, elvisangelaccio, apol
Cc: apol, kde-frameworks-devel, kfm-devel, fvogt, nikolaik, pberestov, 
iasensio, fprice, LeGast00n, cblack, fbampaloukas, alexde, GB_2, Codezela, 
feverfew, meven, michaelh, spoorun, navarromorales, firef, ngraham, 
andrebarros, bruns, emmanuelp, mikesomov

D28520: Fix lifetime of slot in KIO-MTP

[Alexander Saoutkin]

feverfew added a comment.


  @apol just found another one where this occurs, forgot about it but noticed 
it earlier as well. This doesn't affect KIOFuse as much though (as we don't 
listen to the processedSize() signal).
  
  I'll wait for another approval, or before 20.04 tagging, whichever comes 
first :P

REPOSITORY
  R320 KIO Extras

BRANCH
  slotLifetime (branched from master)

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

To: feverfew, akrutzler, dfaure, elvisangelaccio, apol
Cc: apol, kde-frameworks-devel, kfm-devel, fvogt, nikolaik, pberestov, 
iasensio, fprice, LeGast00n, cblack, fbampaloukas, alexde, GB_2, Codezela, 
feverfew, meven, michaelh, spoorun, navarromorales, firef, ngraham, 
andrebarros, bruns, emmanuelp, mikesomov

D28535: [KIO-MTP] Fix null pointer dereference

[Alexander Saoutkin]

feverfew created this revision.
feverfew added reviewers: akrutzler, dfaure, elvisangelaccio.
Herald added projects: Dolphin, Frameworks.
Herald added subscribers: kfm-devel, kde-frameworks-devel.
feverfew requested review of this revision.

REVISION SUMMARY
  A null pointer can be returned from getDevice() if a device is disconnected.
  Passing NULL into LIBMTP_Get_Storage() results in a NULL pointer dereference.
  
  BUG: 405838

TEST PLAN
  Compiles. I couldn't reproduce this as described in the bug report, but from
  reading the attached stacktrace in the bug report it's obvious what went wrong
  here.

REPOSITORY
  R320 KIO Extras

BRANCH
  fixNullPtr (branched from master)

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

AFFECTED FILES
  mtp/kiod_module/mtpstorage.cpp

To: feverfew, akrutzler, dfaure, elvisangelaccio
Cc: kde-frameworks-devel, fvogt, kfm-devel, ngraham, nikolaik, pberestov, 
iasensio, fprice, LeGast00n, cblack, fbampaloukas, alexde, GB_2, Codezela, 
feverfew, meven, michaelh, spoorun, navarromorales, firef, andrebarros, bruns, 
emmanuelp, mikesomov

D28535: [KIO-MTP] Fix null pointer dereference

[Alexander Saoutkin]

feverfew added a comment.


  In D28535#640674 , @fvogt wrote:
  
  > What you're suggesting is to change `MTPDevice::getDevice` to return the 
old device if reopening fails - but reopening without releasing might not work.
  
  
  This seems to be a robust solution IMO, why do you suspect this might not 
work?

REPOSITORY
  R320 KIO Extras

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

To: feverfew, akrutzler, dfaure, elvisangelaccio
Cc: anthonyfieroni, kde-frameworks-devel, fvogt, kfm-devel, ngraham, nikolaik, 
pberestov, iasensio, fprice, LeGast00n, cblack, fbampaloukas, alexde, GB_2, 
Codezela, feverfew, meven, michaelh, spoorun, navarromorales, firef, 
andrebarros, bruns, emmanuelp, mikesomov

D28535: [KIO-MTP] Fix null pointer dereference

[Alexander Saoutkin]

feverfew added a comment.


  If you want to, feel free, I'm a bit tight on time. But I will say this. the 
whole `getDevice()` function confuses me. I'm not entirely sure why this `if` 
check is necessary at all. The lifetime of devices and its children (i.e 
storage) should be managed by the KMTPD daemon AFAICT. A device shouldn't be 
trying to re-open itself anywhere IMO. To me the getDevice() function should 
simply be a simple return to avoid this NULL issue happening. Even if a device 
doesn't exist anymore, no segfaults should happen when passing an "invalid" 
`LIBMTP_device_T` to any other `LIBMTP` function?
  
  So to be succinct, the only correct fix here is to change `getDevice()` to 
`return m_mtpdevice`?

REPOSITORY
  R320 KIO Extras

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

To: feverfew, akrutzler, dfaure, elvisangelaccio
Cc: anthonyfieroni, kde-frameworks-devel, fvogt, kfm-devel, ngraham, nikolaik, 
pberestov, iasensio, fprice, LeGast00n, cblack, fbampaloukas, alexde, GB_2, 
Codezela, feverfew, meven, michaelh, spoorun, navarromorales, firef, 
andrebarros, bruns, emmanuelp, mikesomov

D28535: [KIO-MTP] Fix null pointer dereference

[Alexander Saoutkin]

feverfew added a comment.


  In D28535#640703 , @anthonyfieroni 
wrote:
  
  > In D28535#640699 , @feverfew 
wrote:
  >
  > > So to be succinct, the only correct fix here is to change `getDevice()` 
to `return m_mtpdevice`?
  >
  >
  > Yes, then check if it's crash, in all other places `LIBMTP_xxx` should take 
care of and return false or nullptr depend of function returning value.
  
  
  What do you mean by "then check if it's crash"? Surely we should do nothing 
and let LIBMTP sort out printing errors and stuff? Eventually the device should 
be reclaimed by the daemon anyway via the help of `Solid::deviceRemoved()`. I 
agree with everything after the comma though, `LIBMTP_mtpdevice_t *` should 
never be freed unless we explicitly do so ourselves.

REPOSITORY
  R320 KIO Extras

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

To: feverfew, akrutzler, dfaure, elvisangelaccio
Cc: anthonyfieroni, kde-frameworks-devel, fvogt, kfm-devel, ngraham, nikolaik, 
pberestov, iasensio, fprice, LeGast00n, cblack, fbampaloukas, alexde, GB_2, 
Codezela, feverfew, meven, michaelh, spoorun, navarromorales, firef, 
andrebarros, bruns, emmanuelp, mikesomov

D28535: [KIO-MTP] Fix null pointer dereference

[Alexander Saoutkin]

feverfew updated this revision to Diff 79252.
feverfew added a comment.


  - Don't try to release device in get method

REPOSITORY
  R320 KIO Extras

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28535?vs=79198&id=79252

BRANCH
  fixNullPtr (branched from master)

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

AFFECTED FILES
  mtp/kiod_module/mtpdevice.cpp

To: feverfew, akrutzler, dfaure, elvisangelaccio
Cc: anthonyfieroni, kde-frameworks-devel, fvogt, kfm-devel, ngraham, nikolaik, 
pberestov, iasensio, fprice, LeGast00n, cblack, fbampaloukas, alexde, GB_2, 
Codezela, feverfew, meven, michaelh, spoorun, navarromorales, firef, 
andrebarros, bruns, emmanuelp, mikesomov

D28535: [KIO-MTP] Fix null pointer dereference

[Alexander Saoutkin]

feverfew added a comment.


  Ok so I've updated the code as can be seen. Mucked around with 
disconnecting/connecting, no issues on my side at least.

REPOSITORY
  R320 KIO Extras

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

To: feverfew, akrutzler, dfaure, elvisangelaccio
Cc: anthonyfieroni, kde-frameworks-devel, fvogt, kfm-devel, ngraham, nikolaik, 
pberestov, iasensio, fprice, LeGast00n, cblack, fbampaloukas, alexde, GB_2, 
Codezela, feverfew, meven, michaelh, spoorun, navarromorales, firef, 
andrebarros, bruns, emmanuelp, mikesomov

D28535: [KIO-MTP] Fix null pointer dereference

[Alexander Saoutkin]

feverfew added a comment.


  In D28535#642289 , 
@elvisangelaccio wrote:
  
  > In D28535#640833 , @fvogt wrote:
  >
  > > I assume there is a reason why `MTPDevice::getDevice()` has code for 
handling this very specific case, so I wouldn't just remove it without figuring 
out why: https://i.redd.it/hfnl7xo8yovy.gif
  > >
  > > If not, that would indeed be the best option.
  >
  >
  > Unfortunately git blame doesn't seem to help us here.
  >
  > I suggest to push this fix to master only and see what happens.
  
  
  By pushing this to master would we still be able to throw it up to 20.04.* if 
we decide it's stable enough?

REPOSITORY
  R320 KIO Extras

BRANCH
  fixNullPtr (branched from master)

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

To: feverfew, akrutzler, dfaure, elvisangelaccio
Cc: anthonyfieroni, kde-frameworks-devel, fvogt, kfm-devel, ngraham, nikolaik, 
pberestov, iasensio, fprice, LeGast00n, cblack, fbampaloukas, alexde, GB_2, 
Codezela, feverfew, meven, michaelh, spoorun, navarromorales, firef, 
andrebarros, bruns, emmanuelp, rdieter, mikesomov

D28520: Fix lifetime of slot in KIO-MTP

[Alexander Saoutkin]

This revision was automatically updated to reflect the committed changes.
Closed by commit R320:f4e0183adfd8: Fix lifetime of slot in KIO-MTP (authored 
by feverfew).

REPOSITORY
  R320 KIO Extras

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28520?vs=79196&id=79609

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

AFFECTED FILES
  mtp/kio_mtp.cpp

To: feverfew, akrutzler, dfaure, elvisangelaccio, apol, meven
Cc: apol, kde-frameworks-devel, kfm-devel, fvogt, azyx, nikolaik, pberestov, 
iasensio, fprice, LeGast00n, cblack, fbampaloukas, alexde, GB_2, Codezela, 
feverfew, meven, michaelh, spoorun, navarromorales, firef, ngraham, 
andrebarros, bruns, emmanuelp, rdieter, mikesomov

D28535: [KIO-MTP] Fix null pointer dereference

[Alexander Saoutkin]

feverfew added a comment.


  In D28535#642298 , @feverfew wrote:
  
  > In D28535#642289 , 
@elvisangelaccio wrote:
  >
  > > In D28535#640833 , @fvogt 
wrote:
  > >
  > > > I assume there is a reason why `MTPDevice::getDevice()` has code for 
handling this very specific case, so I wouldn't just remove it without figuring 
out why: https://i.redd.it/hfnl7xo8yovy.gif
  > > >
  > > > If not, that would indeed be the best option.
  > >
  > >
  > > Unfortunately git blame doesn't seem to help us here.
  > >
  > > I suggest to push this fix to master only and see what happens.
  >
  >
  > By pushing this to master would we still be able to throw it up to 20.04.* 
if we decide it's stable enough? (also need to know to know what to put down as 
the FIXED-IN in the commit message)?
  
  
  @elvisangelaccio ping seeming as 20.04 release is close...

REPOSITORY
  R320 KIO Extras

BRANCH
  fixNullPtr (branched from master)

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

To: feverfew, akrutzler, dfaure, elvisangelaccio
Cc: anthonyfieroni, kde-frameworks-devel, fvogt, kfm-devel, ngraham, azyx, 
nikolaik, pberestov, iasensio, fprice, LeGast00n, cblack, fbampaloukas, alexde, 
Codezela, feverfew, meven, michaelh, spoorun, navarromorales, firef, 
andrebarros, bruns, emmanuelp, rdieter, mikesomov

D28535: [KIO-MTP] Fix null pointer dereference

[Alexander Saoutkin]

This revision was automatically updated to reflect the committed changes.
Closed by commit R320:94e7b64325f9: [KIO-MTP] Fix null pointer dereference 
(authored by feverfew).

REPOSITORY
  R320 KIO Extras

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28535?vs=79252&id=79998

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

AFFECTED FILES
  mtp/kiod_module/mtpdevice.cpp

To: feverfew, akrutzler, dfaure, elvisangelaccio
Cc: anthonyfieroni, kde-frameworks-devel, fvogt, kfm-devel, ngraham, azyx, 
nikolaik, pberestov, iasensio, fprice, LeGast00n, cblack, fbampaloukas, alexde, 
Codezela, feverfew, meven, michaelh, spoorun, navarromorales, firef, 
andrebarros, bruns, emmanuelp, rdieter, mikesomov

D7563: Add privilegeExecution field to file protocol description

[Alexander Saoutkin]

feverfew added a comment.


  In D7563#653171 , @elvisangelaccio 
wrote:
  
  > In D7563#650117 , @ngraham wrote:
  >
  > > [insert I-have-no-idea-what-I'm-doing dog meme here]
  > >
  > > When trying to create items in root-owned locations, I'm getting an 
errors saying "The process for the file protocol died unexpectedly." or else 
Dolphin simply crashes with a totally unhelpful backtrace.
  >
  >
  > If it's still happening, please have a look at 
https://community.kde.org/Guidelines_and_HOWTOs/Debugging/Debugging_IOSlaves
  
  
  I managed to find and fix one of the crashes (not sure if it's the same one 
that @ngraham had).
  
  However, even with this patch I can't get it to work, I notice this in the 
debug output: `kf5.kauth tried to start an invalid action`
  
  This sounds to me like I'm testing wrong but I can't tell what...

REPOSITORY
  R241 KIO

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

To: ngraham, #frameworks, dfaure, chinmoyr
Cc: kde-frameworks-devel, feverfew, mreeves, mati865, ngraham, elvisangelaccio, 
LeGast00n, cblack, michaelh, bruns

D29385: Introduce KIO::OpenUrlJob, a rewrite and replacement for KRun

[Alexander Saoutkin]

feverfew added a comment.


  Quick question, how does this affect D23384 
? Previously KRun used 
KIO::DesktopExecParser::resultingArguments() which handled the conversion of 
URLs to local KIOFuse URLs if needed, but now I believe this new API doesn't?

REPOSITORY
  R241 KIO

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

To: dfaure, ahmadsamir, broulik, meven, kossebau, davidedmundson, nicolasfella, 
svuorela
Cc: feverfew, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29385: Introduce KIO::OpenUrlJob, a rewrite and replacement for KRun

[Alexander Saoutkin]

feverfew added a comment.


  In D29385#662435 , @dfaure wrote:
  
  > In D29385#662422 , @feverfew 
wrote:
  >
  > > Quick question, how does this affect D23384 
? Previously KRun used 
KIO::DesktopExecParser::resultingArguments() which handled the conversion of 
URLs to local KIOFuse URLs if needed, but now I believe this new API doesn't?
  >
  >
  > No worries, this should still happen.
  >  KRun is actually split into three classes: OpenUrlJob, CommandLauncherJob 
and ApplicationLauncherJob. The last two delegate the work to an internal 
class, KProcessRunner (like KRun did).
  >  So (to roll out the common case of a document-type file), once OpenUrlJob 
finds out which application to start, it calls ApplicationLauncherJob, which 
creates a KProcessRunner, which uses 
KIO::DesktopExecParser::resultingArguments().
  
  
  Ahhh yes correct. Seeming as these jobs are now async (unlike KRun IIRC?), it 
isn't a problem that `resultingArguments` makes several blocking calls to the 
KIOFuse daemon?

REPOSITORY
  R241 KIO

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

To: dfaure, ahmadsamir, broulik, meven, kossebau, davidedmundson, nicolasfella, 
svuorela
Cc: feverfew, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29634: sftp: break large writes into multiple requests

[Alexander Saoutkin]

feverfew added a comment.


  Seems like something similar should also occur in `FileJob::write`?

INLINE COMMENTS

> kio_sftp.cpp:1831-1832
> +while (offset < buffer.size()) {
> +const auto length = qMin(MAX_XFER_BUF_SIZE, 
> buffer.size());
> +ssize_t bytesWritten = sftp_write(file, buffer.data() + 
> offset, length);
> +if (bytesWritten < 0) {

AFAICT the size of the buffer never changes so this will easily cause a buffer 
overrun if I'm not mistaken?

Say for example you have a buffer with `buffer.size() == MAX_XFER_BUF_SIZE + 
1`. Then on the second iteration of the while loop (assuming `bytesWritten == 
MAX_XFER_BUF_SIZE`) you'll do a `sftp_write()` pointing to a `char` buffer of 
size 1, but which incorrectly states that the size is `MAX_XFER_BUF_SIZE`.

REPOSITORY
  R320 KIO Extras

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

To: sitter, ngraham
Cc: feverfew, kde-frameworks-devel, kfm-devel, waitquietly, azyx, nikolaik, 
pberestov, iasensio, aprcela, fprice, LeGast00n, cblack, fbampaloukas, alexde, 
Codezela, meven, michaelh, spoorun, navarromorales, firef, ngraham, 
andrebarros, bruns, emmanuelp, rdieter, mikesomov

D29634: sftp: break large writes into multiple requests

[Alexander Saoutkin]

feverfew added inline comments.

INLINE COMMENTS

> meven wrote in kio_sftp.cpp:1831-1832
> Maybe we can deduce the server buffer size based on the `bytesWritten` value 
> : at init `serv_buffer_size =MAX_XFER_BUF_SIZE; ` and then ` if (length > 
> bytesWritten) { serv_buffer_size = bytesWritten }` and use `serv_buffer_size` 
> instead of MAX_XFER_BUF_SIZE in the loop.

I'm not sure if that would help. `MAX_XFER_BUF_SIZE` would be the upper bound 
of this approach, and if the server buffer size is less I imagine we'll crash 
anyway (as detailed in the bug report). If we simply write less then yes, we 
can use this to "calibrate" but seeming as it crashes instead then 
unfortunately I don't think this will work.

REPOSITORY
  R320 KIO Extras

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

To: sitter, ngraham, meven
Cc: meven, feverfew, kde-frameworks-devel, kfm-devel, waitquietly, azyx, 
nikolaik, pberestov, iasensio, aprcela, fprice, LeGast00n, cblack, 
fbampaloukas, alexde, Codezela, michaelh, spoorun, navarromorales, firef, 
ngraham, andrebarros, bruns, emmanuelp, rdieter, mikesomov

D29634: sftp: break large writes into multiple requests

[Alexander Saoutkin]

feverfew accepted this revision.
feverfew added a comment.


  I imagine something similar should be done for FileJob::write?

REPOSITORY
  R320 KIO Extras

BRANCH
  release/20.04

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

To: sitter, ngraham, meven, feverfew
Cc: meven, feverfew, kde-frameworks-devel, kfm-devel, waitquietly, azyx, 
nikolaik, pberestov, iasensio, aprcela, fprice, LeGast00n, cblack, 
fbampaloukas, alexde, Codezela, michaelh, spoorun, navarromorales, firef, 
ngraham, andrebarros, bruns, emmanuelp, rdieter, mikesomov

D29634: sftp: break large writes into multiple requests

[Alexander Saoutkin]

feverfew added a comment.


  In D29634#670419 , @meven wrote:
  
  > In D29634#670377 , @ngraham 
wrote:
  >
  > > Nice work.
  > >
  > > In D29634#670159 , @feverfew 
wrote:
  > >
  > > > I imagine something similar should be done for FileJob::write?
  > >
  > >
  > > Yeah.
  >
  >
  > I guess you meant `FileProtocol::write`.
  >  There is no need there, it uses `QIODevice::write` directly.
  
  
  Sorry, I meant `kio_sftp`'s implementation of this: 
https://api.kde.org/frameworks/kio/html/classKIO_1_1FileJob.html#a481871536fb9471ccb64929792f31165

REPOSITORY
  R320 KIO Extras

BRANCH
  release/20.04

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

To: sitter, ngraham, meven, feverfew
Cc: meven, feverfew, kde-frameworks-devel, kfm-devel, waitquietly, azyx, 
nikolaik, pberestov, iasensio, aprcela, fprice, LeGast00n, cblack, 
fbampaloukas, alexde, Codezela, michaelh, spoorun, navarromorales, firef, 
ngraham, andrebarros, bruns, emmanuelp, rdieter, mikesomov

D29743: sftp: map sftp_open error to kio error

[Alexander Saoutkin]

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

REPOSITORY
  R320 KIO Extras

BRANCH
  release/20.04

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

To: sitter, feverfew
Cc: kde-frameworks-devel, kfm-devel, waitquietly, azyx, nikolaik, pberestov, 
iasensio, aprcela, fprice, LeGast00n, cblack, fbampaloukas, alexde, Codezela, 
feverfew, meven, michaelh, spoorun, navarromorales, firef, ngraham, 
andrebarros, bruns, emmanuelp, rdieter, mikesomov

D7563: Add privilegeExecution field to file protocol description

[Alexander Saoutkin]

feverfew added a comment.


  In D7563#674682 , @sitter wrote:
  
  > This really cannot land right now IMHO. Dolphin can actually deadlock 
itself because it uses way too much nested event looping and will be entirely 
unresponsive to mouse inputs when certain timers happen to trigger. A trivial 
way to reproduce this is to try and duplicate a file in file:/
  
  
  Interesting... Yes if so that's a serious blocker.
  
  > Also making folders is actually not even implemented as the relevant 
mkdirjob seems to lack privilege enablement. That's not strictly speaking 
blocking but renders the actual UX broken.
  
  I can confirm that I noticed this too. I also noticed that sometimes the 
dialog wouldn't show at all because the slave kept state that said that 
permission was denied (and hence it believed showing the dialog was redundant) 
when in fact my permission was never asked. Although this only happened for 
`mkdir` for me IIRC. So the slave might be mishandling state. At the same time, 
I'm not sure if the slave is in a position to handle such state (they're 
short-lived?).
  
  Creating new files I believe also asks permission twice (one for file.part 
and the other for file.txt), that comes across as bad UX for me.

REPOSITORY
  R241 KIO

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

To: ngraham, #frameworks, dfaure, chinmoyr, sitter
Cc: sitter, kkong, kde-frameworks-devel, feverfew, mreeves, mati865, ngraham, 
elvisangelaccio, LeGast00n, cblack, michaelh, bruns

T10262: Integrate KIO Slaves into file system using FUSE gateway

[Alexander Saoutkin]

feverfew added a comment.


  @ngraham I'll be having some free time this summer. I was thinking if we 
could solidify this task into exactly what we need to do so we can get a 
proposal in for GSoC?

TASK DETAIL
  https://phabricator.kde.org/T10262

To: cfeck, feverfew
Cc: feverfew, #frameworks, ngraham, #dolphin, cfeck, alexde, spoorun, 
navarromorales, firef, andrebarros, emmanuelp, mikesomov

D23159: Prevent error() being emitted when purposefully reading 0 bytes

[Alexander Saoutkin]

feverfew created this revision.
feverfew added reviewers: dfaure, fvogt, chinmoyr.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
feverfew requested review of this revision.

REVISION SUMMARY
  Currently when 0 is passed as a parameter, if the file to be read is 
non-empty both data and error signals 
  are emitted. This behaviour is incorrect, as there is no error. This patch 
makes sure that only data is 
  emitted.

TEST PLAN
  The application I'm developing that hit this bug now no longer hits this 
issue.

REPOSITORY
  R241 KIO

BRANCH
  Read0Bug (branched from master)

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

AFFECTED FILES
  src/ioslaves/file/file.cpp

To: feverfew, dfaure, fvogt, chinmoyr
Cc: kfm-devel, kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D23159: Prevent error() being emitted when purposefully reading 0 bytes

[Alexander Saoutkin]

feverfew added inline comments.

INLINE COMMENTS

> apol wrote in file.cpp:520
> this should be
> 
>   {
>   Q_EMIT data({});`
>   return;
>   }
> 
> I'm not sure that we'd need an empty data emitted then. Are you trying to fix 
> a specific bug that triggers this?

I('ve not seen any IOSlave use a Q_EMIT macro, any reason why it's necessary 
here (what's the change in the behaviour).

Please re-read the summary, carefully, the error is subtle.

> apol wrote in file.cpp:526
> alternatively this could also check ` && bytes != 0`

Please reread the summary, this would not fix the bug.

REPOSITORY
  R241 KIO

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

To: feverfew, dfaure, fvogt, chinmoyr
Cc: apol, kfm-devel, kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D23159: Prevent error() being emitted when purposefully reading 0 bytes

[Alexander Saoutkin]

feverfew added inline comments.

INLINE COMMENTS

> chinmoyr wrote in file.cpp:524
> This doesn't support error reporting. Why not refactor this loop using 
> read(char *, qint64)? I believe you can accommodate your changes inside the 
> loop cleanly then.

I think that would be appropriate for a separate revision which I'm doing. This 
revision is only to handle the case outlined in the summary.

> chinmoyr wrote in file.cpp:538
> here `bytes` being negative should be an error..no?

bytes is unsigned, so it is not a worry for us.

REPOSITORY
  R241 KIO

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

To: feverfew, dfaure, fvogt, chinmoyr
Cc: apol, kfm-devel, kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D23159: Prevent error() being emitted when purposefully reading 0 bytes

[Alexander Saoutkin]

feverfew abandoned this revision.
feverfew added inline comments.

INLINE COMMENTS

> feverfew wrote in file.cpp:524
> I think that would be appropriate for a separate revision which I'm doing. 
> This revision is only to handle the case outlined in the summary.

Actually I've realised using the other function would allow me to solve this 
problem much more cleanly. I'll abandon this revision and upload a new patch 
that addresses this problem (and others).

REPOSITORY
  R241 KIO

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

To: feverfew, dfaure, fvogt, chinmoyr
Cc: apol, kfm-devel, kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D23194: Making FileJob behave consistently.

[Alexander Saoutkin]

feverfew created this revision.
feverfew added reviewers: dfaure, fvogt, chinmoyr, apol.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
feverfew requested review of this revision.

REVISION SUMMARY
  This patch does the following:
  
  1. Makes sure the close() signal is actually emitted when close() is called.
  2. Documents the FileJob functions more accurately, and ensures the file 
slave acts similarly to the two other slaves that implement these functions
  
  (smb/sftp).
  
  3. Fixes an issue when purposefully reading 0 bytes.

TEST PLAN
  The application I am developing on that depends on FileJob now successfully 
receives the close() signal when required and does not experience the bug 
  mentioned when reading 0 bytes. 
  Existing read/write/seek functionality is not broken.
  Tests also pass.

REPOSITORY
  R241 KIO

BRANCH
  ConsistentRead (branched from master)

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

AFFECTED FILES
  src/core/filejob.cpp
  src/core/filejob.h
  src/core/slavebase.cpp
  src/core/slavebase.h
  src/core/slaveinterface.cpp
  src/core/slaveinterface.h
  src/ioslaves/file/file.cpp
  src/ioslaves/file/file.h

To: feverfew, dfaure, fvogt, chinmoyr, apol
Cc: kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D23194: Making FileJob behave consistently.

[Alexander Saoutkin]

feverfew edited the summary of this revision.

REPOSITORY
  R241 KIO

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

To: feverfew, dfaure, fvogt, chinmoyr, apol
Cc: kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D23194: Making FileJob behave consistently.

[Alexander Saoutkin]

feverfew edited the summary of this revision.

REPOSITORY
  R241 KIO

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

To: feverfew, dfaure, fvogt, chinmoyr, apol
Cc: kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D23194: Making FileJob behave consistently.

[Alexander Saoutkin]

feverfew updated this revision to Diff 63861.
feverfew added a comment.


  Addressing comments

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23194?vs=63841&id=63861

BRANCH
  ConsistentRead (branched from master)

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

AFFECTED FILES
  src/core/filejob.cpp
  src/core/filejob.h
  src/core/slavebase.h
  src/core/slaveinterface.h
  src/ioslaves/file/file.cpp
  src/ioslaves/file/file.h

To: feverfew, dfaure, fvogt, chinmoyr, apol
Cc: kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D23194: Making FileJob behave consistently.

[Alexander Saoutkin]

feverfew added inline comments.

INLINE COMMENTS

> chinmoyr wrote in file.cpp:520
> A loop is required here. The docs don't really specify anything about reading 
> data in one go.

Exactly. The docs weren't specific at all. smb/sftp would read once, and return 
the result. the file slave would read until size was reached (or EOD). I 
decided to just convert file's behaviour to what smb/sftp do and updated the 
docs accordingly, for ease of use (and so that if new slaves implement 
KIO::open() they'd know how to implement the functions. Of course, I could do 
the converse, and change smb/sftp to behave like the file slave when 
reading/writing, but I can leave that up to debate, if others want to chime in.

REPOSITORY
  R241 KIO

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

To: feverfew, dfaure, fvogt, chinmoyr, apol
Cc: kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D23194: Making FileJob behave consistently.

[Alexander Saoutkin]

feverfew added inline comments.

INLINE COMMENTS

> chinmoyr wrote in file.h:117
> Nitpick; why not just closeFile() ?

That's how sftp does it, so for consistency.

REPOSITORY
  R241 KIO

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

To: feverfew, dfaure, fvogt, chinmoyr, apol
Cc: kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D23207: Fixing finished() called after error() in smb slave. Do not call finished() in open/read/write/seek operations

[Alexander Saoutkin]

feverfew created this revision.
Herald added projects: Dolphin, Frameworks.
Herald added subscribers: kfm-devel, kde-frameworks-devel.
feverfew requested review of this revision.

REPOSITORY
  R320 KIO Extras

BRANCH
  fixSFTP (branched from master)

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

AFFECTED FILES
  CMakeLists.txt
  sftp/kio_sftp.cpp
  smb/kio_smb.h
  smb/kio_smb_file.cpp

To: feverfew
Cc: kde-frameworks-devel, kfm-devel, aprcela, vmarinescu, fprice, LeGast00n, 
MrPepe, fbampaloukas, alexde, Codezela, feverfew, meven, michaelh, spoorun, 
navarromorales, firef, ngraham, andrebarros, bruns, emmanuelp, mikesomov

D23207: Fixing implementation of FileJob interface in smb/sftp slaves

[Alexander Saoutkin]

feverfew retitled this revision from "Fixing finished() called after error() in 
smb slave. Do not call finished() in open/read/write/seek operations" to 
"Fixing implementation of FileJob interface in smb/sftp slaves".
feverfew edited the summary of this revision.
feverfew edited the test plan for this revision.
feverfew added reviewers: chinmoyr, fvogt.

REPOSITORY
  R320 KIO Extras

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

To: feverfew, chinmoyr, fvogt
Cc: kde-frameworks-devel, kfm-devel, aprcela, vmarinescu, fprice, LeGast00n, 
MrPepe, fbampaloukas, alexde, Codezela, feverfew, meven, michaelh, spoorun, 
navarromorales, firef, ngraham, andrebarros, bruns, emmanuelp, mikesomov

D23207: Fixing implementation of FileJob interface in smb/sftp slaves

[Alexander Saoutkin]

feverfew updated this revision to Diff 63876.
feverfew added a comment.


  Unnecessary min version upgraded

REPOSITORY
  R320 KIO Extras

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23207?vs=63875&id=63876

BRANCH
  fixSFTP (branched from master)

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

AFFECTED FILES
  sftp/kio_sftp.cpp
  smb/kio_smb.h
  smb/kio_smb_file.cpp

To: feverfew, chinmoyr, fvogt
Cc: kde-frameworks-devel, kfm-devel, aprcela, vmarinescu, fprice, LeGast00n, 
MrPepe, fbampaloukas, alexde, Codezela, feverfew, meven, michaelh, spoorun, 
navarromorales, firef, ngraham, andrebarros, bruns, emmanuelp, mikesomov

D23194: Making FileJob behave consistently.

[Alexander Saoutkin]

feverfew added inline comments.

INLINE COMMENTS

> file.cpp:550
> +// Make sure data gets to disk.
> +if (mFile->flush())
> +written(bytesWritten);

Flushing on every write IMO is a bit expensive, but I don't like the fact that 
close() can't report errors and so we have silent data loss. Maybe we should 
implement a flush method in FileJob. It only really makes sense for the file 
slave, for sftp/smb it will be a noop... Thoughts?

REPOSITORY
  R241 KIO

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

To: feverfew, dfaure, fvogt, chinmoyr, apol
Cc: kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D23194: Making FileJob behave consistently.

[Alexander Saoutkin]

feverfew marked 4 inline comments as done.
feverfew added inline comments.

INLINE COMMENTS

> chinmoyr wrote in filejob.h:48
> Document the methods in SlaveBase as well.

I've used @see to help me out. Is that enough? I don't want to duplicate the 
docs, especially if someone changes it and forgets to change the other...

REPOSITORY
  R241 KIO

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

To: feverfew, dfaure, fvogt, chinmoyr, apol
Cc: kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D23194: Making FileJob behave consistently.

[Alexander Saoutkin]

feverfew updated this revision to Diff 64051.
feverfew added a comment.


  Making comments clearer

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23194?vs=63861&id=64051

BRANCH
  ConsistentRead (branched from master)

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

AFFECTED FILES
  src/core/filejob.cpp
  src/core/filejob.h
  src/core/slavebase.h
  src/core/slaveinterface.h
  src/ioslaves/file/file.cpp
  src/ioslaves/file/file.h

To: feverfew, dfaure, fvogt, chinmoyr, apol
Cc: kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D22708: [WIP] Add a kded module infra for fuse mount services

[Alexander Saoutkin]

feverfew added a comment.


  In D22708#507449 , @ngraham wrote:
  
  > What?
  
  
  Don't worry, all is fine. I've taken over Chinmoy's work on this for now. We 
agreed on a better way to go about integrating KIOFuse into KIO.

REPOSITORY
  R241 KIO

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

To: chinmoyr
Cc: feverfew, ngraham, andriusr, kde-frameworks-devel, LeGast00n, GB_2, 
michaelh, bruns

D22708: [WIP] Add a kded module infra for fuse mount services

[Alexander Saoutkin]

feverfew added a comment.


  In D22708#516128 , @ngraham wrote:
  
  > Oh good. :) Is there anywhere I can follow the progress?
  
  
  All my work goes on KIOFuse goes into review here:
  https://gitlab.com/Vogtinator/kio-fuse/merge_requests
  
  Sometimes I need to upload diffs upstream to KIO to fix bugs or for 
integrating KIOFuse into KIO. Those can be followed via my phabricator diffs. I 
have some (i.e. KIO bug fixes) that I would like to get into 5.62, but no one 
is able to review/approve my KIO changes atm...
  
  Also feel free to pop by #kde-fm for discussions or if you want to ask a 
question.

REPOSITORY
  R241 KIO

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

To: chinmoyr
Cc: feverfew, ngraham, andriusr, kde-frameworks-devel, LeGast00n, GB_2, 
michaelh, bruns

D22708: [WIP] Add a kded module infra for fuse mount services

[Alexander Saoutkin]

feverfew added a comment.


  In D22708#516132 , @ngraham wrote:
  
  > In D22708#516131 , @feverfew 
wrote:
  >
  > > In D22708#516128 , @ngraham 
wrote:
  > >
  > > >
  > >
  > >
  > > Also feel free to pop by #kde-fm for discussions or if you want to ask a 
question.
  >
  >
  > It kicks me out for not having a registered nickname even though I totally 
do and am using it. If it's not necessary anymore, it might be nice to remove 
that restriction.
  
  
  Yeah I've had problems accessing that channel via matrix unless I used 
Konversation.

REPOSITORY
  R241 KIO

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

To: chinmoyr
Cc: feverfew, ngraham, andriusr, kde-frameworks-devel, LeGast00n, GB_2, 
michaelh, bruns

D23384: [WIP] Adding support for mounting KIOFuse URLs for applications that don't use KIO

[Alexander Saoutkin]

feverfew created this revision.
feverfew added reviewers: fvogt, chinmoyr.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
feverfew requested review of this revision.

TEST PLAN
  Make sure kded module is installed and loaded.
  Use dolphin and observe that when opening applications that don't support KIO,
  the resulting files are opened in a KIOFuse mount instead of opened via the 
  help of kioexec.

REPOSITORY
  R241 KIO

BRANCH
  KDEDModule (branched from master)

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

AFFECTED FILES
  src/core/desktopexecparser.cpp
  src/widgets/krun.cpp

To: feverfew, fvogt, chinmoyr
Cc: kde-frameworks-devel, ngraham, LeGast00n, GB_2, michaelh, bruns

D23454: Fixing bug where MTP slave does not return error in stat()/mimetype()

[Alexander Saoutkin]

feverfew created this revision.
feverfew added reviewers: chinmoyr, akrutzler, elvisangelaccio, dfaure.
Herald added projects: Dolphin, Frameworks.
Herald added subscribers: kfm-devel, kde-frameworks-devel.
feverfew requested review of this revision.

REVISION SUMMARY
  This patch fixes two cases where invalid KTMPFile objects would be treated as 
  valid. In particular, this would manifest in KIO::stat() and KIO::mimetype() 
  returning without error when they, in fact, should do. One would note that in 
  the rest of the code, KMTPFile objects that are received via 
  KMTPStorageInterface::getFileMetaData() always have a isValid() check applied 
  before use. The two previously mentioned functions, incorrectly, do not have 
  this check applied. This patch simply adds these necessary checks.

TEST PLAN
  KIOFuse uses KIO::stat() to complete a lookup. Before this patch lookup would
  always succeed and this was determined to be because stat would always 
succeed.
  This bug could be easily found as tab completion would never work correctly. 
  Trying to tab complete would seem to add what was already typed in to the 
  directory. With this patch applied, lookup no longer always succeeds and tab 
  completion now works.

REPOSITORY
  R320 KIO Extras

BRANCH
  mtpFixStat (branched from master)

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

AFFECTED FILES
  mtp/kio_mtp.cpp

To: feverfew, chinmoyr, akrutzler, elvisangelaccio, dfaure
Cc: kde-frameworks-devel, kfm-devel, fvogt, aprcela, vmarinescu, fprice, 
LeGast00n, MrPepe, fbampaloukas, alexde, GB_2, Codezela, feverfew, meven, 
michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, bruns, 
emmanuelp, mikesomov

D23194: Making FileJob behave consistently.

[Alexander Saoutkin]

feverfew added inline comments.

INLINE COMMENTS

> dfaure wrote in filejob.h:112
> Why would someone request reading 0 bytes? That doesn't seem sensible to me.

Well it can happen and this is technically more correct than saying that an 
empty `QByteArray()` is always EOD. In fact, the one of the motivations for 
this patch was that for KIOFuse, we do read 0 bytes in case the user truncates 
to 0, otherwise we'd have to have a workaround if it didn't work (for example, 
before this patch, we'd get both an empty QByteArray() and the error signal...).

REPOSITORY
  R241 KIO

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

To: feverfew, dfaure, fvogt, chinmoyr, apol
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23194: Making FileJob behave consistently.

[Alexander Saoutkin]

feverfew updated this revision to Diff 64672.
feverfew added a comment.


  - Fixing typos
  - Adding test for reading of 0 bytes and asserting that the close signal is
  
  actually emitted.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23194?vs=64051&id=64672

BRANCH
  ConsistentRead (branched from master)

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

AFFECTED FILES
  autotests/jobremotetest.cpp
  autotests/jobremotetest.h
  src/core/filejob.cpp
  src/core/filejob.h
  src/core/slavebase.h
  src/core/slaveinterface.h
  src/ioslaves/file/file.cpp
  src/ioslaves/file/file.h

To: feverfew, dfaure, fvogt, chinmoyr, apol
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23194: Making FileJob behave consistently.

[Alexander Saoutkin]

feverfew marked 3 inline comments as done.

REPOSITORY
  R241 KIO

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

To: feverfew, dfaure, fvogt, chinmoyr, apol
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23194: Making FileJob behave consistently.

[Alexander Saoutkin]

feverfew updated this revision to Diff 64673.
feverfew added a comment.


  - Removing flush on write

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23194?vs=64672&id=64673

BRANCH
  ConsistentRead (branched from master)

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

AFFECTED FILES
  autotests/jobremotetest.cpp
  autotests/jobremotetest.h
  src/core/filejob.cpp
  src/core/filejob.h
  src/core/slavebase.h
  src/core/slaveinterface.h
  src/ioslaves/file/file.cpp
  src/ioslaves/file/file.h

To: feverfew, dfaure, fvogt, chinmoyr, apol
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23194: Making FileJob behave consistently.

[Alexander Saoutkin]

feverfew marked an inline comment as done.

REPOSITORY
  R241 KIO

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

To: feverfew, dfaure, fvogt, chinmoyr, apol
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23207: Fixing implementation of FileJob interface in smb/sftp slaves

[Alexander Saoutkin]

feverfew added reviewers: sitter, dfaure.

REPOSITORY
  R320 KIO Extras

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

To: feverfew, chinmoyr, fvogt, sitter, dfaure
Cc: kde-frameworks-devel, kfm-devel, aprcela, vmarinescu, fprice, LeGast00n, 
MrPepe, fbampaloukas, alexde, GB_2, Codezela, feverfew, meven, michaelh, 
spoorun, navarromorales, firef, ngraham, andrebarros, bruns, emmanuelp, 
mikesomov

D23207: Fixing implementation of FileJob interface in smb/sftp slaves

[Alexander Saoutkin]

feverfew added a comment.


  In D23207#520233 , @sitter wrote:
  
  > Not knowing the background here at a glance I would argue that SlaveBase in 
KIO should be getting state verification on all of this,.
  
  
  Sorry, I'm not sure what you mean by "state verification" in this case and 
how it relates to SlaveBase.

REPOSITORY
  R320 KIO Extras

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

To: feverfew, chinmoyr, fvogt, sitter, dfaure
Cc: kde-frameworks-devel, kfm-devel, aprcela, vmarinescu, fprice, LeGast00n, 
MrPepe, fbampaloukas, alexde, GB_2, Codezela, feverfew, meven, michaelh, 
spoorun, navarromorales, firef, ngraham, andrebarros, bruns, emmanuelp, 
mikesomov

D23194: Making FileJob behave consistently.

[Alexander Saoutkin]

This revision was automatically updated to reflect the committed changes.
Closed by commit R241:3154fc3e096e: Making FileJob behave consistently. 
(authored by feverfew).

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23194?vs=64673&id=64976

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

AFFECTED FILES
  autotests/jobremotetest.cpp
  autotests/jobremotetest.h
  src/core/filejob.cpp
  src/core/filejob.h
  src/core/slavebase.h
  src/core/slaveinterface.h
  src/ioslaves/file/file.cpp
  src/ioslaves/file/file.h

To: feverfew, dfaure, fvogt, chinmoyr, apol
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23207: Fixing implementation of FileJob interface in smb/sftp slaves

[Alexander Saoutkin]

This revision was automatically updated to reflect the committed changes.
Closed by commit R320:28fdc41f1d45: Fixing implementation of FileJob interface 
in smb/sftp slaves (authored by feverfew).

REPOSITORY
  R320 KIO Extras

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23207?vs=63876&id=64977

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

AFFECTED FILES
  sftp/kio_sftp.cpp
  smb/kio_smb.h
  smb/kio_smb_file.cpp

To: feverfew, chinmoyr, fvogt, sitter, dfaure
Cc: bcooksley, kde-frameworks-devel, kfm-devel, vmarinescu, fprice, LeGast00n, 
MrPepe, fbampaloukas, alexde, GB_2, Codezela, feverfew, meven, michaelh, 
spoorun, navarromorales, firef, ngraham, andrebarros, bruns, emmanuelp, 
mikesomov

D24076: add a simple smoke test for slaves by using kioclient5

[Alexander Saoutkin]

feverfew added a comment.


  I won't review, but would like to note I think that with KIOFuse, in 
conjuction with fio  we can do some really 
complex testing on KIO, correct me if I'm wrong.
  
  If I'm right, then we can test KIO nicely, although if we do notice a bug via 
using fio, it can technically be in any of FUSE/KIOFuse/KIO, but in my 
experience KIO is the most buggy of the lot.

REPOSITORY
  R320 KIO Extras

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

To: sitter, dfaure
Cc: feverfew, kde-frameworks-devel, kfm-devel, iasensio, fprice, LeGast00n, 
MrPepe, fbampaloukas, alexde, GB_2, Codezela, meven, michaelh, spoorun, 
navarromorales, firef, ngraham, andrebarros, bruns, emmanuelp, mikesomov

D23384: [WIP] Adding support for mounting KIOFuse URLs for applications that don't use KIO

[Alexander Saoutkin]

feverfew updated this revision to Diff 68931.
feverfew added a comment.


  - Switch from KDED module to DBus service

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23384?vs=64440&id=68931

BRANCH
  KDEDModule (branched from master)

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

AFFECTED FILES
  src/core/CMakeLists.txt
  src/core/desktopexecparser.cpp
  src/core/kiofuseinterface.cpp
  src/core/kiofuseinterface.h
  src/widgets/krun.cpp

To: feverfew, fvogt, chinmoyr
Cc: davidedmundson, kde-frameworks-devel, ngraham, LeGast00n, GB_2, michaelh, 
bruns

D23384: [WIP] Adding support for mounting KIOFuse URLs for applications that don't use KIO

[Alexander Saoutkin]

feverfew updated this revision to Diff 69201.
feverfew added a comment.


  Don't use KIOFuse with mtp/gdrive

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23384?vs=68931&id=69201

BRANCH
  KDEDModule (branched from master)

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

AFFECTED FILES
  src/core/CMakeLists.txt
  src/core/desktopexecparser.cpp
  src/core/kiofuseinterface.cpp
  src/core/kiofuseinterface.h
  src/widgets/krun.cpp

To: feverfew, fvogt, chinmoyr
Cc: davidedmundson, kde-frameworks-devel, ngraham, LeGast00n, GB_2, michaelh, 
bruns

D23384: [WIP] Adding support for mounting KIOFuse URLs for applications that don't use KIO

[Alexander Saoutkin]

feverfew edited the summary of this revision.
feverfew edited the test plan for this revision.
feverfew edited reviewers, added: davidedmundson, dfaure, ngraham; removed: 
chinmoyr.

REPOSITORY
  R241 KIO

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

To: feverfew, fvogt, davidedmundson, dfaure, ngraham, chinmoyr
Cc: davidedmundson, kde-frameworks-devel, ngraham, LeGast00n, GB_2, michaelh, 
bruns

D23384: [WIP] Adding support for mounting KIOFuse URLs for applications that don't use KIO

[Alexander Saoutkin]

feverfew added a comment.


  @dfaure Just to give you a bit of context. KIOFuse is about to enter KDE 
Review, but at the same time, KIOFuse is most useful when integrated to KIO via 
this patch. So we'd like to get this reviewed early to save a bit of effort; 
instead of getting it passed KDE Review and then having to wait for approval 
for this patch, we'd like to do both concurrently, and simply merge this once 
KIOFuse is officially a KDE project.
  
  @ngraham can do the QA side of the review to save some time, but you are of 
course welcome to test this yourself!

REPOSITORY
  R241 KIO

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

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: davidedmundson, kde-frameworks-devel, ngraham, LeGast00n, GB_2, michaelh, 
bruns

D23384: [WIP] Adding support for mounting KIOFuse URLs for applications that don't use KIO

[Alexander Saoutkin]

feverfew updated this revision to Diff 69769.
feverfew added a comment.


  - Align function arguments
  - Remove hack
  - Get rid of iterator

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23384?vs=69201&id=69769

BRANCH
  KDEDModule (branched from master)

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

AFFECTED FILES
  src/core/CMakeLists.txt
  src/core/desktopexecparser.cpp
  src/core/kiofuseinterface.cpp
  src/core/kiofuseinterface.h
  src/widgets/krun.cpp

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: sitter, davidedmundson, kde-frameworks-devel, ngraham, LeGast00n, GB_2, 
michaelh, bruns

D23384: [WIP] Adding support for mounting KIOFuse URLs for applications that don't use KIO

[Alexander Saoutkin]

feverfew updated this revision to Diff 69774.
feverfew added a comment.


  - Get rid of unnecessary copying of URLs
  - Switch to interface generated at compile time
  - Delete unused interfaces
  - Merge branch 'master' into arcpatch-D23384
  - remove kiofuseinterface from build

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23384?vs=69769&id=69774

BRANCH
  arcpatch-D23384

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

AFFECTED FILES
  src/core/CMakeLists.txt
  src/core/desktopexecparser.cpp
  src/core/org.kde.KIOFuse.VFS.xml
  src/widgets/CMakeLists.txt
  src/widgets/krun.cpp
  src/widgets/org.kde.KIOFuse.VFS.xml

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: sitter, davidedmundson, kde-frameworks-devel, ngraham, LeGast00n, GB_2, 
michaelh, bruns

D23384: [WIP] Adding support for mounting KIOFuse URLs for applications that don't use KIO

[Alexander Saoutkin]

feverfew marked 8 inline comments as done.
feverfew added inline comments.

INLINE COMMENTS

> sitter wrote in desktopexecparser.cpp:317
> There's nothing wrong with this. But wouldn't using 
> `QHash, QUrl>` make for easier to read code all in 
> all since you don't have to mess with pairs?
> 
> Alternatively with a vector I'd still make a struct for the data
> 
>   struct MountRequest { QDBusPendingReply reply, QUrl url };
>   QVector requests;
>   ...
>   requests.push_back({ mountUrl(url), url });

Gone for the vector option.

REPOSITORY
  R241 KIO

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

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: sitter, davidedmundson, kde-frameworks-devel, ngraham, LeGast00n, GB_2, 
michaelh, bruns

D23384: [WIP] Adding support for mounting KIOFuse URLs for applications that don't use KIO

[Alexander Saoutkin]

feverfew updated this revision to Diff 69788.
feverfew added a comment.


  - Align arguments

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23384?vs=69774&id=69788

BRANCH
  arcpatch-D23384

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

AFFECTED FILES
  src/core/CMakeLists.txt
  src/core/desktopexecparser.cpp
  src/core/org.kde.KIOFuse.VFS.xml
  src/widgets/CMakeLists.txt
  src/widgets/krun.cpp
  src/widgets/org.kde.KIOFuse.VFS.xml

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: sitter, davidedmundson, kde-frameworks-devel, ngraham, LeGast00n, GB_2, 
michaelh, bruns

D23384: [WIP] Adding support for mounting KIOFuse URLs for applications that don't use KIO

[Alexander Saoutkin]

feverfew added inline comments.

INLINE COMMENTS

> sitter wrote in krun.cpp:583
> Coding style says we use curly braces even for single-line if statements I 
> think.

Yep you're right, will fix in a mo.

> sitter wrote in krun.cpp:598
> What I meant is you should literally iterate using
> 
>   for (QUrl &url : urls) {
> 
> which is what the code did before but you changed it for some reason.

Yes, but note later I need to then change the values in the `QList` outside of 
the for loop, hence why I store the index in a struct associated with the 
reply. How would I do that easily with a range based for loop?

REPOSITORY
  R241 KIO

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

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: sitter, davidedmundson, kde-frameworks-devel, ngraham, LeGast00n, GB_2, 
michaelh, bruns

D23384: [WIP] Adding support for mounting KIOFuse URLs for applications that don't use KIO

[Alexander Saoutkin]

feverfew added inline comments.

INLINE COMMENTS

> sitter wrote in krun.cpp:598
> So did the old code though. See old line 584.
> 
> It's iterating by-reference, so you can simply assign to it via
> 
> https://doc.qt.io/qt-5/qurl.html#operator-eq-1
> 
> to change the content of the QUrl object (not the instance of the object in 
> the list, but rather the content of that object).

That's within the same loop though?

I explicitly want to change the QUrl in that list, so that I can simply return 
the same list without having to do a copy of it all.

I can't get this to work. I need to change the definition of the struct to be:

`struct MountRequest {QDBusReply reply; QUrl &url;};`

This doesn't compile because "is implicitly deleted because the default 
definition would be ill-formed"...

REPOSITORY
  R241 KIO

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

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: sitter, davidedmundson, kde-frameworks-devel, ngraham, LeGast00n, GB_2, 
michaelh, bruns

D23384: [WIP] Adding support for mounting KIOFuse URLs for applications that don't use KIO

[Alexander Saoutkin]

feverfew edited the test plan for this revision.

REPOSITORY
  R241 KIO

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

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: sitter, davidedmundson, kde-frameworks-devel, ngraham, LeGast00n, GB_2, 
michaelh, bruns

D23384: [WIP] Adding support for mounting KIOFuse URLs for applications that don't use KIO

[Alexander Saoutkin]

feverfew added a comment.


  In D23384#569926 , @ngraham wrote:
  
  > In D23384#569645 , @fvogt wrote:
  >
  > > **Issue #1:**
  > >
  > > That happens because the .desktop file sets 
`X-KDE-Protocols=ftp,http,https,mms,rtmp,rtsp,sftp,smb`:
  > >  
https://code.videolan.org/videolan/vlc/blob/master/share/vlc.desktop.in#L124
  >
  >
  > I'm not sure about that. If I remove that line from VLC's installed desktop 
file, the problem still happens. And SMPlayer exhibits the same problem yet its 
desktop file doesn't define `X-KDE-Protocols` at all: 
https://sourceforge.net/p/smplayer/code/HEAD/tree/smplayer/trunk/smplayer.desktop
  
  
  This isn't reaching KIOFuse at all. I believe this is related to this bug and 
that you've also blogged about:
  https://bugs.kde.org/show_bug.cgi?id=330192
  https://pointieststick.com/2018/01/17/videos-on-samba-shares/
  
  This can be solved in this patch (and I did, although I removed it at 
Harald's request). I think I'll move it back in, and will leave it if it does 
solve this issue for you.
  
  >> **Issue #2:**
  >> 
  >> The only explanation I have for that is that totem for some reason starts 
reading the file from the end.
  >>  If you start `kio-fuse -d` manually (kill the other kio-fuse process 
first) and then use totem again, what's the debug output
  > 
  > There is no debug output, and the whole file is downloaded locally as with 
the other apps. It seems like Totem is not getting the FUSE mount path; it gets 
an `smb://` URL and it or KIO downloads the file normally. Again, this happens 
even if I open the file from the FUSE mount path itself.
  > 
  > I can confirm that the FUSE path handoff is working properly. For example 
if I open `smb://gaston@living-room-pc/Users/Gaston/Desktop/Shep Face Code.txt` 
in Gedit, Gedit displays the file's path as follows: F7791491: 
Screenshot_20191130_133857.png 
  
  You must be doing something wrong. FUSE provides output of all calls that it 
receives (and how kio-fuse responds). Let me write the instructions more 
verbosely for CLI to make sure no steps are missed:
  `$ killall kio-fuse`
  `$ export QT_LOGGING_RULES="*.debug=true"
  `$ kio-fuse $myFavLocation -d &> ~/kio-fuse-debug.txt` (I recommend 
`$myFabLocation` isn't empty if you haven't installed the 
`kio-fuse-tmpfiles.conf` exclusion file at the system level.)
  
  Then open the file in the media player. Once your done simply attach 
`~/kio-fuse-debug.txt` here.
  It's really important for us to know what kind of read requests are being 
called to help us diagnose the issue.
  
  >> **Issue #3:**
  >> 
  >> Everything is in a separate process and async already. When/how does it 
happen?
  > 
  > When I open a large video file from the hidden mount path in any of the 
media player apps. KIO (or something) downloads the entire file, during which 
time Dolphin freezes. The moment the network activity ends because the file is 
finished downloading, Dolphin becomes responsive again. It's 100% reproducible 
for me.
  
  I'm a bit confused. Which one of the three are you doing:
  
  1. Opening a KIO Url via Dolphin.
  2. Opening a KIOFuse mount local URL via Dolphin.
  3. Opening a KIOFuse URL via the media players file picker.

REPOSITORY
  R241 KIO

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

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: sitter, davidedmundson, kde-frameworks-devel, ngraham, LeGast00n, GB_2, 
michaelh, bruns

D23384: [WIP] Adding support for mounting KIOFuse URLs for applications that don't use KIO

[Alexander Saoutkin]

This revision was not accepted when it landed; it landed in state "Needs 
Revision".
This revision was automatically updated to reflect the committed changes.
Closed by commit R241:92c2886c3714: [WIP] Adding support for mounting KIOFuse 
URLs for applications that don't use… (authored by feverfew).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D23384?vs=69788&id=70657#toc

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23384?vs=69788&id=70657

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

AFFECTED FILES
  src/core/CMakeLists.txt
  src/core/desktopexecparser.cpp
  src/core/kiofuseinterface.cpp
  src/core/kiofuseinterface.h
  src/widgets/krun.cpp

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: sitter, davidedmundson, kde-frameworks-devel, ngraham, LeGast00n, GB_2, 
michaelh, bruns

D23384: [WIP] Adding support for mounting KIOFuse URLs for applications that don't use KIO

[Alexander Saoutkin]

feverfew updated this revision to Diff 70661.
feverfew added a comment.


  - Get rid of unnecessary copying of URLs
  - Switch to interface generated at compile time
  - Delete unused interfaces
  - Merge branch 'master' into arcpatch-D23384
  - remove kiofuseinterface from build
  - Align arguments
  - Only send original URLs to non-KIO apps if authority is empty

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23384?vs=70657&id=70661

BRANCH
  arcpatch-D23384

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

AFFECTED FILES
  src/core/CMakeLists.txt
  src/core/desktopexecparser.cpp
  src/core/org.kde.KIOFuse.VFS.xml
  src/widgets/CMakeLists.txt
  src/widgets/krun.cpp
  src/widgets/org.kde.KIOFuse.VFS.xml

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: sitter, davidedmundson, kde-frameworks-devel, ngraham, LeGast00n, GB_2, 
michaelh, bruns

D23384: [WIP] Adding support for mounting KIOFuse URLs for applications that don't use KIO

[Alexander Saoutkin]

feverfew reopened this revision.

REPOSITORY
  R241 KIO

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

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: sitter, davidedmundson, kde-frameworks-devel, ngraham, LeGast00n, GB_2, 
michaelh, bruns

D23384: [WIP] Adding support for mounting KIOFuse URLs for applications that don't use KIO

[Alexander Saoutkin]

feverfew added a comment.


  In D23384#570016 , @ngraham wrote:
  
  > In D23384#569929 , @feverfew 
wrote:
  >
  > > This isn't reaching KIOFuse at all. I believe this is related to this bug 
and that you've also blogged about:
  > >  https://bugs.kde.org/show_bug.cgi?id=330192
  > >  https://pointieststick.com/2018/01/17/videos-on-samba-shares/
  > >
  > > This can be solved in this patch (and I did, although I removed it at 
Harald's request). I think I'll move it back in, and will leave it if it does 
solve this issue for you.
  >
  >
  > Great! Once it's back in, I'll re-test that.
  
  
  Updated the diff. give it a go...
  
  >> You must be doing something wrong. FUSE provides output of all calls that 
it receives (and how kio-fuse responds). Let me write the instructions more 
verbosely for CLI to make sure no steps are missed:
  >>  `$ killall kio-fuse`
  >>  `$ export QT_LOGGING_RULES="*.debug=true"`
  >>  `$ kio-fuse $myFavLocation -d &> ~/kio-fuse-debug.txt` (I recommend 
`$myFabLocation` isn't empty if you haven't installed the 
`kio-fuse-tmpfiles.conf` exclusion file at the system level.)
  >> 
  >> Then open the file in the media player. Once your done simply attach 
`~/kio-fuse-debug.txt` here.
  >>  It's really important for us to know what kind of read requests are being 
called to help us diagnose the issue.
  > 
  > I think I found part of the problem. When I follow these instructions, the 
log file has only a single line:
  > 
  >   kio-fuse: command not found
  > 
  > 
  > This is because the path it is installed to (`~/kde/usr/lib64/libexec/`) is 
not in `$PATH`. When I add that location to `$PATH` and try again, the log file 
contains the following:
  > 
  > It should probably not assume that `/usr/lib64/libexec/` is in 
`$PATH`, at the minimum.
  
  That's just an annoying issue locally, which is resolved by that hack. On the 
system level this won't be a problem at all.
  
  Also, pls put debug output into an attachment in the future (and probably 
edit your comment above), it's now quite hard to scroll through this diff.
  
  > 
  > 
  >> I'm a bit confused. Which one of the three are you doing:
  >> 
  >> 1. Opening a KIO Url via Dolphin.
  >> 2. Opening a KIOFuse mount local URL via Dolphin.
  >> 3. Opening a KIOFuse URL via the media players file picker.
  > 
  > #2.
  
  So as in, you're Dolphin is browsing inside 
`/run/user//kio-fuse-XX/some/location` and you open a file from there?

REPOSITORY
  R241 KIO

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

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: sitter, davidedmundson, kde-frameworks-devel, ngraham, LeGast00n, GB_2, 
michaelh, bruns

D23384: [WIP] Adding support for mounting KIOFuse URLs for applications that don't use KIO

[Alexander Saoutkin]

feverfew added a comment.


  In D23384#570349 , @ngraham wrote:
  
  > In D23384#570125 , @dfaure wrote:
  >
  > > @ngraham AFAIK gnome has a trick where a fuse mount is created, its path 
is passed to the application being started, and the application, if it supports 
gvfs, re-translates that into a URL and uses that instead if it makes more 
sense. This way "dumb" apps get a local file (with all the limitations of doing 
synchronous I/O over the network) and network-transparent applications use URLs.
  > >  On the other hand, the KDE logic is "if the app takes %f and not %u in 
the Exec line, it doesn't support remote URLs, so we need to download the file 
first" (that's done by kioexec). If you see a "download first" check if kioexec 
is running. But if it's the app doing it, then I have no idea.
  >
  >
  > `kioexec` is not running during any of these lengthy downloads. Totem, VLC, 
and SMplayer all have %U in their desktop files, FWIW.
  >
  > We may want to rethink this logic anyway. Any app that doesn't integrate 
with ioslaves should get the FUSE path rather than a locally-downloaded file, 
irrespective of whether or not its desktop file has %f or %u IMO. There are 
just too many disadvantages to the locally downloaded file approach, which is 
why we're doing this FUSE thing in the first place.
  >
  > Perhaps the "lengthy local download first" issue is caused by not having 
https://invent.kde.org/kde/kio-fuse/merge_requests/2 yet.
  
  
  Yes, that's precisely why. !2 
 implements seeking to 
any point in the file without having to download all of it. I'll rebase all of 
it such that you can test all of it easily tomorrow.

REPOSITORY
  R241 KIO

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

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: sitter, davidedmundson, kde-frameworks-devel, ngraham, LeGast00n, GB_2, 
michaelh, bruns

D23384: [WIP] Adding support for mounting KIOFuse URLs for applications that don't use KIO

[Alexander Saoutkin]

feverfew added a comment.


  In D23384#570347 , @ngraham wrote:
  
  > In D23384#570119 , @feverfew 
wrote:
  >
  > > In D23384#570016 , @ngraham 
wrote:
  > >
  > > > In D23384#569929 , @feverfew 
wrote:
  > > >
  > > > > This isn't reaching KIOFuse at all. I believe this is related to this 
bug and that you've also blogged about:
  > > > >  https://bugs.kde.org/show_bug.cgi?id=330192
  > > > >  https://pointieststick.com/2018/01/17/videos-on-samba-shares/
  > > > >
  > > > > This can be solved in this patch (and I did, although I removed it at 
Harald's request). I think I'll move it back in, and will leave it if it does 
solve this issue for you.
  > > >
  > > >
  > > > Great! Once it's back in, I'll re-test that.
  > >
  > >
  > > Updated the diff. give it a go...
  > >  Also I assume that if it does work for you it resolves 330192 
?
  >
  >
  > Yay case #1 is fixed now! VLC and SMPlayer still unnecessarily download the 
URL locally like Totem does, but the large video file does at least open now 
when accesses from the non-FUSE location in Dolphin.
  
  
  This seems like the best solution. From what I understand, not sending the 
credentials is a security feature, so doing it via KIOFuse is unavoidable

REPOSITORY
  R241 KIO

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

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: sitter, davidedmundson, kde-frameworks-devel, ngraham, LeGast00n, GB_2, 
michaelh, bruns

D23384: [WIP] Adding support for mounting KIOFuse URLs for applications that don't use KIO

[Alexander Saoutkin]

feverfew updated this revision to Diff 70773.
feverfew added a comment.


  Check for password instead of authority

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23384?vs=70661&id=70773

BRANCH
  arcpatch-D23384

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

AFFECTED FILES
  src/core/CMakeLists.txt
  src/core/desktopexecparser.cpp
  src/core/org.kde.KIOFuse.VFS.xml
  src/widgets/CMakeLists.txt
  src/widgets/krun.cpp
  src/widgets/org.kde.KIOFuse.VFS.xml

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: broulik, sitter, davidedmundson, kde-frameworks-devel, ngraham, LeGast00n, 
GB_2, michaelh, bruns

D23384: [WIP] Adding support for mounting KIOFuse URLs for applications that don't use KIO

[Alexander Saoutkin]

feverfew updated this revision to Diff 70799.
feverfew added a comment.


  Chek for userInfo instead of just password

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23384?vs=70773&id=70799

BRANCH
  arcpatch-D23384

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

AFFECTED FILES
  src/core/CMakeLists.txt
  src/core/desktopexecparser.cpp
  src/core/org.kde.KIOFuse.VFS.xml
  src/widgets/CMakeLists.txt
  src/widgets/krun.cpp
  src/widgets/org.kde.KIOFuse.VFS.xml

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: broulik, sitter, davidedmundson, kde-frameworks-devel, ngraham, LeGast00n, 
GB_2, michaelh, bruns

D23384: [WIP] Adding support for mounting KIOFuse URLs for applications that don't use KIO

[Alexander Saoutkin]

feverfew updated this revision to Diff 70823.
feverfew added a comment.


  Convert KIO URLs to KIOFuse URL is app is not KIO-enabled.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23384?vs=70799&id=70823

BRANCH
  arcpatch-D23384

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

AFFECTED FILES
  src/core/CMakeLists.txt
  src/core/desktopexecparser.cpp
  src/core/org.kde.KIOFuse.VFS.xml
  src/widgets/CMakeLists.txt
  src/widgets/krun.cpp
  src/widgets/org.kde.KIOFuse.VFS.xml

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: broulik, sitter, davidedmundson, kde-frameworks-devel, ngraham, LeGast00n, 
GB_2, michaelh, bruns

D23384: [WIP] Adding support for mounting KIOFuse URLs for applications that don't use KIO

[Alexander Saoutkin]

feverfew added a comment.


  In D23384#571358 , @fvogt wrote:
  
  > In D23384#571276 , @ngraham 
wrote:
  >
  > > I'm afraid that even with that change, the issue is still present. I 
honestly don't think it would be the worst thing in the world if we always 
handed the kio-fuse paths to apps that don't use ioslaves.
  >
  >
  > It would be. I like to have links like http://kde.org opened properly in 
the web browser, ftp://some/where opened in an FTP client and so on...
  >  Media players know more about the format and streaming it than kio-fuse 
ever could, so avoiding layers in between if possible is definitely an 
advantage.
  
  
  If the URL is protected then we have to pass it to KIOFuse as most apps don't 
know how to get the credentials from kpasswdserver. This is a bug that has 
always existed with KIOExec, where before this patch it would always pass on 
the original URL to non-KIO apps if they understood them, but would have the 
same issue with credentials being required. From what I understand, 
`userInfo()` always seems to be emtpy even if it is password protected, so 
there's nothing clever I can do where if it isn't password protected I can just 
pass it straight to the app. It looks like we'll simply have to send the 
KIOFuse URL to all non-KIO apps. Trying to optimise I think is out of scope for 
this patch. I've blacklisted http/https seeming as they don't make much sense 
as a filesystem.

REPOSITORY
  R241 KIO

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

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: broulik, sitter, davidedmundson, kde-frameworks-devel, ngraham, LeGast00n, 
GB_2, michaelh, bruns

D23384: [WIP] Adding support for mounting KIOFuse URLs for applications that don't use KIO

[Alexander Saoutkin]

feverfew updated this revision to Diff 70830.
feverfew added a comment.


  ignore scheme handler

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23384?vs=70823&id=70830

BRANCH
  arcpatch-D23384

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

AFFECTED FILES
  src/core/CMakeLists.txt
  src/core/desktopexecparser.cpp
  src/core/org.kde.KIOFuse.VFS.xml
  src/widgets/CMakeLists.txt
  src/widgets/krun.cpp
  src/widgets/org.kde.KIOFuse.VFS.xml

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: broulik, sitter, davidedmundson, kde-frameworks-devel, ngraham, LeGast00n, 
GB_2, michaelh, bruns

D23384: [WIP] Adding support for mounting KIOFuse URLs for applications that don't use KIO

[Alexander Saoutkin]

feverfew added a comment.


  So before this patch we had the following behaviour:
  
  1. If app is KIO-enabled, pass URLs unchanged.
  2. else for each URL, if app claims to support the protocol pass the original 
URL, otherwise change the URL to the corresponding KIOExec path.
  
  This causes a problem as noted in 330192 
 and this blog post 
. In particular, 
if a non-KIO claims to support a certain protocol (e.g., VLC supports smb), 
then we won't convert to a KIOExec path, but we strip out the stuff that's in 
`userInfo()` (credentials) before sending it over.
  
  What I previously tried to do was the following, in an attempt to close:
  
  1. If app is KIO-enabled, pass URLs unchanged.
  2. else for each URL, if app claims to support the protocol **AND**  
`userInfo()` is empty pass the original URL, otherwise change the URL to the 
corresponding KIOExec path.
  
  However, that doesn't really help as apparently even if, say, a samba share 
is password protected, at the point I'm checking the password in 
`resolveURLs()` or `resultingArguments()` (truth be told, I'm not sure which, 
if not both, codepaths are being hit in our current testing), `userInfo()` is 
always empty, so this optimisation isn't possible.
  
  So then one could just always direct non-KIO apps to use KIOFuse. Well that 
causes another problem, what about stuff like http(s)? It doesn't really make 
sense as a filesystem and we'd definitely like to forward it to the browser. So 
currently I can special case http/https. but is that the only one we want to 
special case?
  
  Ideally, I'd like to know if there is a place where `userInfo` is only empty 
when there is genuinely no user/pass combo needed to access the resource at the 
URL. If so, then it can actually be possible to use KIOFuse on apps that 
support a URL only if it needs to have credentials (which it won't have) such 
that 330192 can be closed, without needing any hacks.

REPOSITORY
  R241 KIO

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

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: broulik, sitter, davidedmundson, kde-frameworks-devel, ngraham, LeGast00n, 
GB_2, michaelh, bruns

D23384: [WIP] Adding support for mounting KIOFuse URLs for applications that don't use KIO

[Alexander Saoutkin]

feverfew updated this revision to Diff 70866.
feverfew added a comment.


  don't send http(s) to kioexec

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23384?vs=70830&id=70866

BRANCH
  arcpatch-D23384

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

AFFECTED FILES
  src/core/CMakeLists.txt
  src/core/desktopexecparser.cpp
  src/core/org.kde.KIOFuse.VFS.xml
  src/widgets/CMakeLists.txt
  src/widgets/krun.cpp
  src/widgets/org.kde.KIOFuse.VFS.xml

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: broulik, sitter, davidedmundson, kde-frameworks-devel, ngraham, LeGast00n, 
GB_2, michaelh, bruns

D23384: [WIP] Adding support for mounting KIOFuse URLs for applications that don't use KIO

[Alexander Saoutkin]

feverfew edited the summary of this revision.

REPOSITORY
  R241 KIO

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

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: broulik, sitter, davidedmundson, kde-frameworks-devel, ngraham, LeGast00n, 
GB_2, michaelh, bruns

D23384: [WIP] Adding support for mounting KIOFuse URLs for applications that don't use KIO

[Alexander Saoutkin]

feverfew added a comment.


  Issues with KIOFuse itself should be opened up as an issue, or as a reply to 
the MR that is being tested; this differential only concerns communication 
between the KIOFuse mount and KIO itself.
  
  In particular, if you could test writing with both master and FileJobCombo 
(!2, i.e. the seeking MR) and tell us what happens that would be great. What 
slave were you testing? Does this also occur if you mount something from the 
file slave (e.g. file:///home/nate/image.xcf); instructions for mounting 
without this differential are available in the README of the KIOFuse repo. A 
log for both branches would be great (as you did earlier following fvogt's 
instructions). A reply to all of this in the relevant issue tracker or MR would 
be great. Thanks.

REPOSITORY
  R241 KIO

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

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: alexde, broulik, sitter, davidedmundson, kde-frameworks-devel, ngraham, 
LeGast00n, GB_2, michaelh, bruns

D23384: [WIP] Adding support for mounting KIOFuse URLs for applications that don't use KIO

[Alexander Saoutkin]

feverfew added a comment.


  Thanks for the reviews everyone! I'll update the diff and respond to the 
comments no later than Tuesday

REPOSITORY
  R241 KIO

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

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: alexde, broulik, sitter, davidedmundson, kde-frameworks-devel, ngraham, 
LeGast00n, GB_2, michaelh, bruns

D23384: [WIP] Adding support for mounting KIOFuse URLs for applications that don't use KIO

[Alexander Saoutkin]

feverfew added a comment.


  In D23384#572363 , @dfaure wrote:
  
  > Why does KRun duplicate all of the (new) code from DesktopExecParser, when 
DesktopExecParser is actually a helper class for KRun?
  >  I would expect it to have solved all this already, unless I'm missing 
something about the various code paths.
  >  The code you changed in krun.cpp was supposed to simply resolve 
desktop:/foo to file:///bleh but everything else about %u/%f is done by 
DesktopExecParser.
  
  
  Using git grep it seems to me that `resolveURLs` is called in 
`KRun::runApplicationImpl` and `KRun::runService` but 
`KIO::DesktopExecParser::resultingArguments` isn't called in those two 
functions. If URL conversion isn't necessary in those functions then I'll 
happily get rid of the code.

REPOSITORY
  R241 KIO

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

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: alexde, broulik, sitter, davidedmundson, kde-frameworks-devel, ngraham, 
LeGast00n, GB_2, michaelh, bruns

D23384: [WIP] Adding support for mounting KIOFuse URLs for applications that don't use KIO

[Alexander Saoutkin]

feverfew added inline comments.

INLINE COMMENTS

> dfaure wrote in desktopexecparser.cpp:323
> Could we have a kill switch for KIOFuse if it fails to work properly in some 
> release?
> 
> E.g. some `export KIO_FUSE=0` or `export KIO_FUSE_DISABLE=1`.
> Or a KConfig entry (use `KSharedConfig::openConfig()->group("KIO")` for best 
> performance and flexibility -- then it can be disabled for one calling 
> application or for all)

One can always remove/uninstall kiofuse if it doesn't work or if they don't 
really want the feature?

> dfaure wrote in desktopexecparser.cpp:331
> The comment says "in case there is a password", the code doesn't.
> Probably historical, please fix :)
> 
> On this topic I saw earlier comments in the merge request which I found 
> confusing.
> It's very rare for an application to have the password stored inside the URL 
> itself. That would mean the user typing it in clear in a location bar or in a 
> terminal, bad idea. Instead, the user typically types a URL like 
> ftp://user@host/ and the kioslave asks for the password, and stores it in 
> kpasswdserver.
> 
> Also you wrote "we strip out the stuff that's in userInfo()". (where 
> QUrl::userInfo is username+password). But surely, while we might strip out 
> passwords for security reasons, we never strip out the username, do we?

From what others have tested `userInfo()` is always empty even when the URL is 
actually a protected Samba share... This means I can't know if a certain URL is 
likely to have a password or not, I just have to assume it does. I'll check 
again just in case, as indeed it does seem odd, but that was my conclusion.

> broulik wrote in desktopexecparser.cpp:356
> Blocking IO is worse than blocking DBus, still not nice, especially given the 
> "job"-like nature of `KRun` I would expect it to be fully async.

In these methods I have to return the URLs within the function, so blocking is 
inevitable. If I could return the URLs via a signal then blocking could be 
avoided.

REPOSITORY
  R241 KIO

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

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: alexde, broulik, sitter, davidedmundson, kde-frameworks-devel, ngraham, 
LeGast00n, GB_2, michaelh, bruns

D23384: [WIP] Adding support for mounting KIOFuse URLs for applications that don't use KIO

[Alexander Saoutkin]

feverfew updated this revision to Diff 71423.
feverfew added a comment.


  - Merge branch 'master' into arcpatch-D23384
  - Address comments
  - Remove unnecessary mount requests in krun
  - Only use KIOFuse if password is empty

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23384?vs=70866&id=71423

BRANCH
  arcpatch-D23384

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

AFFECTED FILES
  src/core/CMakeLists.txt
  src/core/desktopexecparser.cpp
  src/core/org.kde.KIOFuse.VFS.xml

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: alexde, broulik, sitter, davidedmundson, kde-frameworks-devel, ngraham, 
LeGast00n, GB_2, michaelh, bruns

D23384: [WIP] Adding support for mounting KIOFuse URLs for applications that don't use KIO

[Alexander Saoutkin]

feverfew added a comment.


  On further looking, it seems like git grep doesn't really tell the full 
picture. It seems like `resultingArguments` is called before `resolveURLs` is, 
so I've simplified the diff as requested.

INLINE COMMENTS

> feverfew wrote in desktopexecparser.cpp:331
> From what others have tested `userInfo()` is always empty even when the URL 
> is actually a protected Samba share... This means I can't know if a certain 
> URL is likely to have a password or not, I just have to assume it does. I'll 
> check again just in case, as indeed it does seem odd, but that was my 
> conclusion.

From my own testing your intuition seems correct. I've now amended my diff 
appropriately

REPOSITORY
  R241 KIO

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

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: alexde, broulik, sitter, davidedmundson, kde-frameworks-devel, ngraham, 
LeGast00n, GB_2, michaelh, bruns

D23384: [WIP] Adding support for mounting KIOFuse URLs for applications that don't use KIO

[Alexander Saoutkin]

feverfew marked 9 inline comments as done.
feverfew added a comment.


  The current patch should be tested with latest kio-fuse master, as the 
blacklisting has been moved there to keep this patch clean.

REPOSITORY
  R241 KIO

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

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: alexde, broulik, sitter, davidedmundson, kde-frameworks-devel, ngraham, 
LeGast00n, GB_2, michaelh, bruns

D23384: [WIP] Adding support for mounting KIOFuse URLs for applications that don't use KIO

[Alexander Saoutkin]

feverfew added a comment.


  In D23384#577174 , @dfaure wrote:
  
  > For SMB shares I guess what can happen is that the URL doesn't have a 
username in it, but it still needs a password? IIRC there was no username in 
"old" samba (W95 , W98 
, XP, ...?).
  >  This might explain what you were saying about people having password 
problems even with empty userInfo() (which means username and password).
  >  But we can hardly know, just by looking at a URL, if a password is 
needed... I guess most of time a password is actually needed? Maybe you're 
right after all, and we should always send SMB urls to kiofuse. Dunno.
  
  
  @ngraham could you confirm if you have the credential issue with the current 
state of this patch. If you do, I'll simplify the logic seeming as there's no 
chance I can be "smart" about this here.

REPOSITORY
  R241 KIO

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

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: alexde, broulik, sitter, davidedmundson, kde-frameworks-devel, ngraham, 
LeGast00n, GB_2, michaelh, bruns

D23384: [WIP] Adding support for mounting KIOFuse URLs for applications that don't use KIO

[Alexander Saoutkin]

feverfew updated this revision to Diff 71543.
feverfew added a comment.


  - fix code style

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23384?vs=71423&id=71543

BRANCH
  arcpatch-D23384

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

AFFECTED FILES
  src/core/CMakeLists.txt
  src/core/desktopexecparser.cpp
  src/core/org.kde.KIOFuse.VFS.xml

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: alexde, broulik, sitter, davidedmundson, kde-frameworks-devel, ngraham, 
LeGast00n, GB_2, michaelh, bruns

D23384: Adding support for mounting KIOFuse URLs for applications that don't use KIO

[Alexander Saoutkin]

feverfew retitled this revision from "[WIP] Adding support for mounting KIOFuse 
URLs for applications that don't use KIO" to "Adding support for mounting 
KIOFuse URLs for applications that don't use KIO".
feverfew edited the summary of this revision.
feverfew edited the test plan for this revision.

REPOSITORY
  R241 KIO

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

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: alexde, broulik, sitter, davidedmundson, kde-frameworks-devel, ngraham, 
LeGast00n, GB_2, michaelh, bruns

D23384: Adding support for mounting KIOFuse URLs for applications that don't use KIO

[Alexander Saoutkin]

feverfew updated this revision to Diff 71666.
feverfew added a comment.


  fix indentation

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23384?vs=71543&id=71666

BRANCH
  arcpatch-D23384_1

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

AFFECTED FILES
  src/core/CMakeLists.txt
  src/core/desktopexecparser.cpp
  src/core/org.kde.KIOFuse.VFS.xml

To: feverfew, fvogt, davidedmundson, dfaure, ngraham
Cc: alexde, broulik, sitter, davidedmundson, kde-frameworks-devel, ngraham, 
LeGast00n, GB_2, michaelh, bruns

D26148: Add truncation support to FileJob

[Alexander Saoutkin]

feverfew created this revision.
feverfew added reviewers: fvogt, dfaure, sitter.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
feverfew requested review of this revision.

REVISION SUMMARY
  This patch adds support for truncation for FileJob.
  It also implements this function in the file slave.

TEST PLAN
  Tested in conjunction with KIOFuse, does truncation
  correctly.

REPOSITORY
  R241 KIO

BRANCH
  TruncateSupport

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

AFFECTED FILES
  src/core/commands_p.h
  src/core/filejob.cpp
  src/core/filejob.h
  src/core/global.h
  src/core/slavebase.cpp
  src/core/slavebase.h
  src/core/slaveinterface.cpp
  src/core/slaveinterface.h
  src/ioslaves/file/file.cpp
  src/ioslaves/file/file.h

To: feverfew, fvogt, dfaure, sitter
Cc: ngraham, sitter, dfaure, kde-frameworks-devel, fvogt, LeGast00n, GB_2, 
michaelh, bruns

D26148: Add truncation support to FileJob

[Alexander Saoutkin]

feverfew updated this revision to Diff 71990.
feverfew added a comment.


  Add unit test for truncation

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26148?vs=71963&id=71990

BRANCH
  TruncateSupport

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

AFFECTED FILES
  autotests/jobremotetest.cpp
  autotests/jobremotetest.h
  src/core/commands_p.h
  src/core/filejob.cpp
  src/core/filejob.h
  src/core/global.h
  src/core/slavebase.cpp
  src/core/slavebase.h
  src/core/slaveinterface.cpp
  src/core/slaveinterface.h
  src/ioslaves/file/file.cpp
  src/ioslaves/file/file.h

To: feverfew, fvogt, dfaure, sitter
Cc: ngraham, sitter, dfaure, kde-frameworks-devel, fvogt, LeGast00n, GB_2, 
michaelh, bruns

D26191: Add support for FileJob->truncate() in smb/sftp slaves

[Alexander Saoutkin]

feverfew created this revision.
feverfew added reviewers: sitter, dfaure, fvogt.
Herald added projects: Dolphin, Frameworks.
Herald added subscribers: kfm-devel, kde-frameworks-devel.
feverfew requested review of this revision.

REVISION SUMMARY
  This implements the new method added in D26148 
 in the sftp/smb slaves.

TEST PLAN
  Tested sftp locally with truncate. Seems to work.
  Haven't tested locally with smb though.

REPOSITORY
  R320 KIO Extras

BRANCH
  master

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

AFFECTED FILES
  CMakeLists.txt
  sftp/kio_sftp.cpp
  sftp/kio_sftp.h
  smb/kio_smb.h
  smb/kio_smb_file.cpp

To: feverfew, sitter, dfaure, fvogt
Cc: kde-frameworks-devel, kfm-devel, ngraham, pberestov, iasensio, fprice, 
LeGast00n, MrPepe, fbampaloukas, alexde, GB_2, Codezela, feverfew, meven, 
michaelh, spoorun, navarromorales, firef, andrebarros, bruns, emmanuelp, 
mikesomov

D26191: Add support for FileJob->truncate() in smb/sftp slaves

[Alexander Saoutkin]

feverfew updated this revision to Diff 72104.
feverfew added a comment.


  - Free unneeded struct pointer

REPOSITORY
  R320 KIO Extras

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26191?vs=72103&id=72104

BRANCH
  master

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

AFFECTED FILES
  CMakeLists.txt
  sftp/kio_sftp.cpp
  sftp/kio_sftp.h
  smb/kio_smb.h
  smb/kio_smb_file.cpp

To: feverfew, sitter, dfaure, fvogt
Cc: kde-frameworks-devel, kfm-devel, ngraham, pberestov, iasensio, fprice, 
LeGast00n, MrPepe, fbampaloukas, alexde, GB_2, Codezela, feverfew, meven, 
michaelh, spoorun, navarromorales, firef, andrebarros, bruns, emmanuelp, 
mikesomov