Opened 13 years ago

Closed 12 years ago

#920 closed Enhancement (fixed)

Add "Move Data" to libT so all clients can use it

Reported by: compotatoj Owned by: charles
Priority: Normal Milestone: 1.70
Component: Transmission Version: 1.20
Severity: Normal Keywords: patch
Cc: colrol@…, dsvensson, woodzu123@…

Description

Currently, the only way to move where the data is stored in a torrent is to remove the torrent, move the data, and re-add the torrent pointing to the new location. This causes Transmission to have to rehash the entire torrent and can be tedious. Please add an option to move the torrent data around from within Transmission like the Mac OS X interface has. Thank you!

Attachments (15)

transmission-movecmd.diff (15.0 KB) - added by turbo 12 years ago.
Move data, update downloadDir for torrent and then remove old location.
transmission-movecmd_v2.diff (28.8 KB) - added by turbo 12 years ago.
Final version (? :) - accept 'torrent-move', open new thread, check avail disk space, check if torrent fit in dest, stop torrent if DOWNLOADING or SEEDING, copy torrent download dir, change download dir for torrent, remove old directory, start torrent again (if stopped earlier). Refuse to move if/while verify is done (wait for it to finish). When done, close thread cleanly.
transmission-movecmd_v3.diff (21.5 KB) - added by turbo 12 years ago.
NOW I'm happy: Found the missing thread lock - now I can move multiple files, not just one at a time (after the first have finished :). Finish whole move (copy+delete) before starting up the torrent again (messed up the status)
transmission-movecmd_v4.diff (34.5 KB) - added by turbo 12 years ago.
Fixing points in http://trac.transmissionbt.com/ticket/920#comment:21.
transmission-movecmd_v5.diff (43.0 KB) - added by turbo 12 years ago.
Only need to calculate the percentDone - use that if availible (won't it always with this fix?) instead of calculating using sizeWhenDone and leftUntilDone.
transmission-movecmd_v5.2.diff (43.2 KB) - added by turbo 12 years ago.
tr_torrentRevertState() but (naturaly!) be inside the thread so it's executed at the correct time. The func might still need work though (could only test with Stopped and Idle threads)
transmission-movecmd_v6.diff (44.4 KB) - added by turbo 12 years ago.
Deal better with failures - status='Failed Move' and leave the torrent stopped (?).
transmission-movecmd_v7.diff (52.8 KB) - added by turbo 12 years ago.
Web GUI fixes (RPC 'publish' filesMoved and filesScheduledForMove). CSS patch requires updated progress.png (next attachement).
progress.png (3.0 KB) - added by turbo 12 years ago.
Proof-of-concept progress image. It's not to bad, just 15 times larger than the original (!?). Feel free to redo it... Or shrink it :)
transmission-movecmd_v8.diff (66.6 KB) - added by turbo 12 years ago.
Move 'remove torrent' to separate thread - much like verify/move. This so that we can wait() until a move is done, before we remove the torrent (see remove.c for more). Better (or rather less) thread locking problem. Move support funcs from {move,torrent}.[ch] to utils.[ch] (making them private).
transmission-movecmd_v9.diff (69.7 KB) - added by turbo 12 years ago.
Copy torrent into a dot dir (.TORRENT) which later gets renamed (quite securily) to avoid confusion.
move-torrent-rev1.diff (19.2 KB) - added by charles 12 years ago.
first draft
set-torrent-location-2.png (59.2 KB) - added by charles 12 years ago.
in action
set-torrent-location-1.png (21.2 KB) - added by charles 12 years ago.
"set torrent location" dialog
move-torrent-rev2.diff (26.1 KB) - added by charles 12 years ago.
add rpc support. add first part of qt support.

Download all attachments as: .zip

Change History (52)

comment:1 Changed 13 years ago by charles

  • Milestone changed from None Set to 1.30
  • Status changed from new to assigned
  • Version set to 1.20

comment:2 Changed 13 years ago by charles

  • Milestone changed from 1.30 to 1.40

comment:3 Changed 12 years ago by charles

  • Milestone changed from 1.40 to Sometime

comment:4 Changed 12 years ago by charles

  • Milestone changed from Sometime to 1.50

comment:5 Changed 12 years ago by charles

  • Milestone changed from 1.50 to 1.60

comment:6 Changed 12 years ago by charles

  • Component changed from GTK+ Client to Transmission
  • Summary changed from Add Move Data option to GTK+ Interface to add "Move Data" into libtransmission so that gtk and daemon can use it too

Changed 12 years ago by turbo

Move data, update downloadDir for torrent and then remove old location.

comment:7 Changed 12 years ago by turbo

The patch added might not be the pretties you've seen (I'm using my own function for traversing directory etc and the names might not be according to the Transmission namingstandard). But it works...

PS. I've only added the command '--move' to transmission-remote. Adding it to the GUI(s) is left as an exersise for the reader :).

comment:8 Changed 12 years ago by charles

not a bad starting point... thanks for the patch

comment:9 Changed 12 years ago by turbo

I'm trying to get some status going. Like the 'Verifying' status, but 'Moving' (with a % done).

This unfortunatly _require_ me to release control almost directly, hence very little status can be sent back to the client. And as in #554, moving all that data takes me 4.5 minutes with my test torrent. And that's only 4.4Gb. Don't want to lock that long.

So a simple fork should do it.

I'm not very ... happy about 'worker thread(s)'. They need communication and I worked with semaphores about two years ago in a previous job. Quite frankly, the're a mess to get stable!

I'll submit a special bug report about this, which all bugs mentioned in #554 can depend on... Let's dicuss it in that ticket instead.

comment:10 Changed 12 years ago by turbo

See #1753. I don't know if Trac can do it, or if it's me that don't have the access, but I couldn't seem to do a 'Depend on Bug'.

comment:11 Changed 12 years ago by turbo

Regarding transmission-movecmd_v2.diff: I just couldn't figure out how to do the counter like Verify does it. It's a lot of cut-and-past, but still couldn't get it working.

It at least say 'Moving' now...

Any pointers would be appreciated.

comment:12 Changed 12 years ago by turbo

Thought: 1 Should a Verify be allowed to finnish before a Move can take place

or:

2 Should the Move be iniated immidiatly and the Verify (continue) afterwards?

One point that speaks for point two is that a Move is tripple-checked afterwards...

Changed 12 years ago by turbo

Final version (? :) - accept 'torrent-move', open new thread, check avail disk space, check if torrent fit in dest, stop torrent if DOWNLOADING or SEEDING, copy torrent download dir, change download dir for torrent, remove old directory, start torrent again (if stopped earlier). Refuse to move if/while verify is done (wait for it to finish). When done, close thread cleanly.

comment:13 Changed 12 years ago by charles

(See also ticket #1768)

Changed 12 years ago by turbo

NOW I'm happy: Found the missing thread lock - now I can move multiple files, not just one at a time (after the first have finished :). Finish whole move (copy+delete) before starting up the torrent again (messed up the status)

comment:14 Changed 12 years ago by turbo

I could REALLY need some input on this - try v3 (ONLY!! :).

comment:15 Changed 12 years ago by turbo

#1768, #1483, #1647 and in some degree #1496 is dependent on this ticket... I think I've found all of them :)

comment:16 Changed 12 years ago by turbo

Forgot #1768...

comment:17 Changed 12 years ago by turbo

Ah, an 8 looks like a 0 when it's crossed over.

comment:18 Changed 12 years ago by turbo

  • Keywords patch added

comment:19 Changed 12 years ago by turbo

#1212 contains code to get quota - currently my patch(es) does not take that into account. I'm going to 'steal' some of the code and update the patch...

comment:20 Changed 12 years ago by charles

Sorry I haven't been more involved in this -- I've been very busy with RL duties and will probably be even busier in February, but I'll try to make a little more time for Transmission.

Here's some very early feedback:

  1. Overall the basic idea looks good.
  1. It doesn't make sense to make so much low-level implementation public in transmission.h. A simple function like "int tr_torrentMoveLocalData( tr_torrent*, const char * destination );" would be more appropriate, and the implementation details could be put in a private header.
  1. It's not clear to me what happens if a torrent is moved while it's running. It needs to be stopped before it's moved, and then after it's moved, it should probably be reverted to the state it was in before. This bug is also in the current verify code... the Torrent's state really needs to be cleaned up in this regard. From an design standpoint this is probably the biggest item in this feedback list.
  1. move.[ch] are in the Makefile.am, but aren't in the patch
  1. the added comment "return FALSE for failure or TRUE for success" in the prototype for "void tr_torrentDeleteLocalData" doesn't make sense.. shouldn't it be returning a tr_bool instead of void?
  1. I'm not sure I understand why tr_stat.sizeWhenDone, tr_stat.percentDone, and tr_stat.leftUntilDone have different meanings when the torrent is verifying or being moved. Could you add some comments explaining what's going on there?
  1. In do_copy_file(), better to call fstat() and look at st_blksize, than to use BUFSIZ.
  1. It looks like the patch leaks the variables created by tr_buildPath() in get_directory(), do_traverse_dir(), and tr_torrentCopyLocalData()

Thanks for the patch. I'm looking forward to your next revision.

comment:21 Changed 12 years ago by turbo

Thanx, good input! I'm done with points [12457]. Working on point 3b (3a being stopping the torrent a little earlier - it was done AFTER the copy, but before the 'move update'and remove old). And I'll have another look at 6, but basically there's something wrong with the leftUntilDone and sizeWhenDone. The're not correct! Look at #1777. The calculated size does not match the actuall FS usage! But I'll have another look if I overlooked something and/or if I can make it cleaner...

But in the meantime, could you please ellaborate on 8? What do you mean by 'leak'?

comment:22 Changed 12 years ago by charles

the variables created by tr_buildPath() need to be freed by calling tr_free().

Actually the other malloc/frees in the patch should probably be replaced with tr_new(), tr_new0(), and tr_free(), just for consistency's sake with the rest of libtransmission.

comment:23 Changed 12 years ago by turbo

That should of course be http://trac.transmissionbt.com/ticket/920#comment:20. Getting tired...

comment:24 Changed 12 years ago by turbo

Point six in http://trac.transmissionbt.com/ticket/920#comment:20 is still broken... Fixing.

Changed 12 years ago by turbo

Only need to calculate the percentDone - use that if availible (won't it always with this fix?) instead of calculating using sizeWhenDone and leftUntilDone.

Changed 12 years ago by turbo

tr_torrentRevertState() but (naturaly!) be inside the thread so it's executed at the correct time. The func might still need work though (could only test with Stopped and Idle threads)

Changed 12 years ago by turbo

Deal better with failures - status='Failed Move' and leave the torrent stopped (?).

Changed 12 years ago by turbo

Web GUI fixes (RPC 'publish' filesMoved and filesScheduledForMove). CSS patch requires updated progress.png (next attachement).

Changed 12 years ago by turbo

Proof-of-concept progress image. It's not to bad, just 15 times larger than the original (!?). Feel free to redo it... Or shrink it :)

Changed 12 years ago by turbo

Move 'remove torrent' to separate thread - much like verify/move. This so that we can wait() until a move is done, before we remove the torrent (see remove.c for more). Better (or rather less) thread locking problem. Move support funcs from {move,torrent}.[ch] to utils.[ch] (making them private).

Changed 12 years ago by turbo

Copy torrent into a dot dir (.TORRENT) which later gets renamed (quite securily) to avoid confusion.

comment:25 Changed 12 years ago by charles

  • are tr_torrentSaveState() and tr_torrentRevertState() intended to be public methods? If not, they should go in torrent.h instead of transmission.h.
  • is struct directory_entries intended to be public to clients? If not, it should go in a non-public header instead of transmission.h
  • v9 adds a lot of Linux-only code to utils.c
  • functions like do_traverse_dir() should probably be static (private) to utils.c and not be advertised in utils.h... it's currently just a helper function for get_directory()

comment:26 Changed 12 years ago by turbo

I've fixed all but point 3. Since I don't code (and never have - exept the occasional PalmOS and AmigaOS decades ago :) on anything else than Linux, I'm not sure exactly what is Linux specific and what's not...

The most obvious is probably tr_getFSDevice() and perhaps tr_getFSSize() - don't know how quota's are handled on *IX or Win (guess Win don't have quotas ? ).

But the dir traversing and rename etc should be ok on everything - it uses stat() which I assume exists on all systems (?). At least the man pages for both rename() and stat() claims to be POSIX (don't know how Win conforms to that though)...

I'd appreciate more input on this from someone that does programming in multiple environments. Maybe a few eyeballs on the patch to see if it's easy to 'port' or add to... ?

comment:27 Changed 12 years ago by turbo

  • Summary changed from add "Move Data" into libtransmission so that gtk and daemon can use it too to [PATCH] Add "Move Data" into libtransmission so that gtk and daemon can use it too

comment:28 Changed 12 years ago by Rolcol

  • Cc colrol@… added

comment:29 Changed 12 years ago by livings124

  • Milestone changed from 1.60 to None Set

comment:30 Changed 12 years ago by dsvensson

  • Cc dsvensson added

comment:31 Changed 12 years ago by woodzu123

  • Cc woodzu123@… added

Changed 12 years ago by charles

first draft

Changed 12 years ago by charles

in action

comment:32 Changed 12 years ago by charles

  • Milestone changed from None Set to 1.70

move-torrent-rev1.diff is a new rough draft at implementing this. I've confirmed that it works, at least, though it doesn't handle any state problems such as queueing s.t. two torrents aren't moved at once. I'm leaving that issue for #1753.

The two screenshots set-torrent-location-1 and set-torrent-location-2 show what this looks like in the gtk+ client.

The dialog is named "set location" rather than "move" so that it can serve a dual purpose -- sometimes due to a system error Transmission will forget where a torrent's been downloaded. Previously the only workaround was to find a copy of the .torrent file, remove the torrent from Transmission, then re-add it with the correct location. With this dialog, you can tell Transmission where to look without having to remove + readd.

Todo: daemon, qt, mac.

Changed 12 years ago by charles

"set torrent location" dialog

comment:33 Changed 12 years ago by charles

  • Summary changed from [PATCH] Add "Move Data" into libtransmission so that gtk and daemon can use it too to [PATCH] Add "Move Data" to libtransmission so all clients to use it

comment:34 Changed 12 years ago by charles

  • Summary changed from [PATCH] Add "Move Data" to libtransmission so all clients to use it to [PATCH] Add "Move Data" to libT so all clients can use it

"so all clients to use it"? :)

Changed 12 years ago by charles

add rpc support. add first part of qt support.

comment:35 Changed 12 years ago by charles

  • Resolution set to fixed
  • Status changed from assigned to closed

committed to r8389.

comment:36 Changed 12 years ago by livings124

  • Resolution fixed deleted
  • Status changed from closed to reopened
  • Summary changed from [PATCH] Add "Move Data" to libT so all clients can use it to Add "Move Data" to libT so all clients can use it

There needs to be a way for libtransmission to handle and report errors while moving, and set the location accordingly.

comment:37 Changed 12 years ago by charles

  • Resolution set to fixed
  • Status changed from reopened to closed

better handling of error conditions in r8453

Note: See TracTickets for help on using tickets.