Download to network location error

[Stane1983]

Client cannot download (save) to network location on Windows. Reports IO error in system tray. By network location I mean \192.168.1.34\somefolder. If folder is mounted as network drive in Windows it works.

[Gelmir]

@cdumez, I found out that this patch allows to download to samba shares directly

diff -ruN --binary -x '.git*' -x '*.user' -x '*.orig' -x '*.dat' -x '*.qm' qBittorrent_orig/src/qtlibtorrent/qbtsession.cpp qBittorrent/src/qtlibtorrent/qbtsession.cpp
--- qBittorrent_orig/src/qtlibtorrent/qbtsession.cpp    2013-01-28 17:04:55.214684200 +0400
+++ qBittorrent/src/qtlibtorrent/qbtsession.cpp 2013-01-30 20:10:18.922723000 +0400
@@ -1123,7 +1123,7 @@
     // Remember label
     TorrentTempData::setLabel(hash, savePath_label.second);
   } else {
-    savePath = getSavePath(hash, fromScanDir, path);
+    savePath = QDir::toNativeSeparators(getSavePath(hash, fromScanDir, path));
   }
   if (!defaultTempPath.isEmpty() && !TorrentPersistentData::isSeed(hash)) {
     qDebug("addTorrent::Temp folder is enabled.");

Unfortunately moving torrent to samba share won't work w/o a similar modification. The problem seems to be in separators handling. My suggestion is rewriting code to ensure 'separator' consistency. libtorrent itself should have no problems with win-style separators.

I suggest adding wrappers to fsutils class

QString fsutils::toNativePath(const QString& path)
{
#if defined(Q_OS_WIN) || defined (Q_OS_OS2)
  return QDir::toNativeSeparators(path);
#else
  return path;
#endif
}

QString fsutils::fromNativePath(const QString& path)
{
#if defined(Q_OS_WIN) || defined (Q_OS_OS2)
  return QDir::fromNativeSeparators(path);
#else
  return path;
#endif
}

And furthermore ensure that:

1. fsutils methods
   • can take native/qt style separators as argument
   • internally work with qt style separators
   • return strings with qt style separators except suggested toNativePath
2. preferences methods
   • take qt style separators as argument (writing prefs)
   • take native/qt style separators as argument (reading prefs; this will maintain backward compatibility with old config)
   • internally work with qt style separators
   • return strings with qt style separators
3. calls to libtorrent
   • use native separators

This will at least get rid of a lot of regex "\" "/" (and vice versa) replaces.

[sledgehammer999]

@Gelmir

Commenting on the proposed wrappers in fsutils. I think they are redundant. We should use QDir::to/fromNativeSeparators() directly. It always return the correct separators for each system.

Also I thought that libtorrent accepted '/' as a universal separator... :s

[Gelmir]

@sledgehammer999, that's a lot of calls. Just look how many "\" -> "/" and vice-versa replaces are in the code. The only OSes that use \ as separator are OS2 and Windows (I'm not counting CE here), so using it on Unix systems is useless (according to docs https://qt-project.org/doc/qt-4.8/qdir.html#toNativeSeparators) and we already do it in some files https://github.com/qbittorrent/qBittorrent/blob/master/src/preferences/options_imp.cpp#L388

What I'm proposing is using Qt-style separators everywhere possible and switch to win-style separators when we need to show path in labels, text boxes, etc and when we call libtorrent methods (that's why downloading to samba shares fails in the first place: using //location/folder instead of \\location\folder is not supported network share syntax); when we save settings (I've seen both separator types in ini file).

[sledgehammer999]

Yep, this is a mess. And needs work, and possibly breaking compatibility with earlier settings. As you said, we should agree on some ground rules and then enforce them to the source. I think @cdumez should comment on this before you spend too much time on this.

EDIT: But I was only refering to the class prototype. It doesn't make much sense.

[Gelmir]

I'll bump this. #861 #697 #840 #493 are all duplicates.

@sledgehammer999 Something needs to be done about separators handling. The biggest problem is preventing excessive calling of to/fromNativeSeparators. Wish I had a call graph. Right now the best way is starting from fsuils, preferences and qbtsession. Using native separators and QDir::separator() everywhere would be best.

[sledgehammer999]

The biggest problem is preventing excessive calling of to/fromNativeSeparators.

How are you going to prevent this? What's the alternative? A sample patch on one of our files would be great to demonstrate it.

Also will, the network paths work correctly on Linux and Windows?

PS: I suppose you are up to the task of converting everything, right?

[Gelmir]

Also will, the network paths work correctly on Linux and Windows?

Yes. Though we'll have to do additional checks on Linux. On Linux network path can start with \, which will be replaced by // thanks to QDir::toNativeSeparators. Thus it is preferable to use smb:// for network paths.

[Gelmir]

@sledgehammer999, so here's the proposition:

1. fsutils class would be separator unaware except *Location() methods
2. preferences classes would also be separator unaware - doesn't really matter how we save, if we can correct it on start
3. qbtsession class would be separator aware - all the magic will happen here
4. UI classes would cast strings to native separators to show to user and also check if smb:// prefix is needed

[sledgehammer999]

From my POV there isn't much risk here. Local locations will continue to work anyway. At worst the network locations will still not work. So go ahead and make the necessary changes.

smb://

Is this the only network prefix? How about nfs?

PS: If you cannot support everything, you can go as far as you can. We can do this in multiple stages/phases.

[sledgehammer999]

Also one thought I had about fsutils code: I think it is most suitable as a namespace rather than a classs with static methods. Since you'll be touching that code, see if a convertion is possible. If yes, make it in a separate commit so we can revert if need be. Also take a look at the conversion I did in the misc class: 7a16146f6f9af

[Gelmir]

Is this the only network prefix? How about nfs?

https://en.wikipedia.org/wiki/URI_scheme I would guess user will have to enter this manually. Linux file managers usually automatically convert \ to smb:// because it's the right thing to do (and because this causes less pain to windows refugees), but nfs is off limits to such conversions.

Also one thought I had about fsutils code: I think it is most suitable as a namespace

I'll take a look at this.

[Gelmir]

@sledgehammer999 Now I'm thinking about a different approach. How about extending fsutils class to accommodate all path/fs related checks and conversions. Basically we should remove all separator-related code from everything but fsutils.

The idea behind this is doing stuff in one swipe, e.g. prepareAddTorrentPath will extend path, convert separators, prepend smb:// URI handler if needed, so that QBTSession doesn't need to think about it.

For starters prepareAddTorrentPath and sanitizeDisplayPath should be enough. fsutils methods will be called only from UI; this should offload some work from QBTSession

[sledgehammer999]

Yeah go ahead.

[Gelmir]

Had 3 attempts on this one. Was choosing between dropping all heavy stuff on fsutils, using a separate class QPath (was half-written already) and good old 'do it manually'. The last one worked out better than the others.

QPath will be too heavy to support everything: craploads of overloads and operator definitions to support QDir, QString, etc; plus all methods' arguments in almost every class will need to be updated to use new class.

Using fsutils for heavy stuff is redundant. We use QDir and QFile a lot everywhere - both don't care about separators, but will return string with qt-style separators. So this approach would have a lot of back-and-forth separator conversions.

Thus, I've made session, handle, preferences, fsutils to accept any separators in methods args and return qt-style separators. Session and handle call libtorrent API with native separators. So, currently, you only need to convert separators to show in UI or use libtorrent API directly.

This is still in alpha - haven't tested on Linux. The branch is https://github.com/Gelmir/qBittorrent/tree/separators

P.S. @sledgehammer999, since I had to read through half the code, I've noticed some minor problems:

1. LegalNotice is shown after splash and is moved to background, while splash is being shown.
2. Italic font is event log makes backslashes look like vertical lines

[Gelmir]

Well. I had a tough time trying to make this work on Linux. Both nautilus and thunar didn't allow using network shares. Dolphin, at least, did get into samba share, but downloading there was prohibited with "You can only select local files". This looks like limitation of QFileDialog and we can't do much about it right now.

I guess I'll have to leave Unix smb support at this state.

[sledgehammer999]

I had to read through half the code

Yikes, but kudos for this huge endeavor!!!

I've noticed some minor problems:

Noted. They seem pretty easy to fix too.