Opened 13 years ago

Closed 13 years ago

Last modified 12 years ago

#1787 closed Enhancement (fixed)

Add support for seeding ratio limiting in libtransmission

Reported by: JasonBorden Owned by: charles
Priority: Normal Milestone: 1.60
Component: Transmission Version: 1.42
Severity: Normal Keywords:
Cc:

Description

The attached patch adds the ability to limit a torrent's seeding ratio. So far I only have it implemented in the GTK+ client, but it should allow for people to test the feature and provide feedback and bug reports.

Attachments (13)

libtransmission-ratiolimit.patch (20.9 KB) - added by JasonBorden 13 years ago.
Ratio limit support for libtransmission
libtransmission-ratiolimit-new.patch (19.4 KB) - added by JasonBorden 13 years ago.
Now with 50% less SIGSEGVs
gtk-ratiolimit.patch (9.8 KB) - added by livings124 13 years ago.
GTK+ seeding ratio limiting support
gtk+libtransmission-ratelimit-v2.patch (41.6 KB) - added by JasonBorden 13 years ago.
Patch includes the requested changes and bug fixes
gtk+libtransmission-ratelimit-v3.patch (42.2 KB) - added by JasonBorden 13 years ago.
makes tr_ratiolimit enum, removes TR_RATIO from tr_speedlimit, and fixes gtk/main.c's missing patch from v2
gtk+libtransmission-ratelimit-v4.patch (41.9 KB) - added by charles 13 years ago.
differences from v3: (1) modified the gtk+ prefs & details dialogs (2) revert fastresume.c
gtk+libtransmission-ratelimit-v5.patch (47.6 KB) - added by charles 13 years ago.
differences from v4: (1) add transmission-remote support (2) in "torrent-set", replace "ratio-limit-enabled" with "ratio-limit-mode" (3) make the enum values in transmission.h explicit for the benefit of rpc spec readers who might not be familiar with C's enum defaults
gtk+libtransmission-ratiolimit-v6.patch (42.8 KB) - added by JasonBorden 13 years ago.
Reverted back to using tr_torrentStop in peerCalllbackFunc. Added sanity checks in didWrite and didWriteWrapper. Added code to select the appripriate radio button when the details page is opened. Added code to remove the "prefs-changed" signal from the core when the dialog is closed.
gtk+libtransmission-ratiolimit-v6.1.patch (42.2 KB) - added by JasonBorden 13 years ago.
Minor improvment to the didWriteWrapper sanity check
gtk+libtransmission-ratiolimit-v7.patch (41.9 KB) - added by JasonBorden 13 years ago.
1) Now uses tr_getRatio to calculate the current seeding ratio. 2) Removed some g_message debug messages from the spin dials in the GTK+ client
gtk+libtransmission-ratelimit-v8.patch (41.6 KB) - added by charles 13 years ago.
changes from v7: (1) fix a bug in rpcimpl.c's sessionGet() that assigned the wrong key for the ratio 'enabled' flag (2) add tr_torrentGetSeedRatio() convenience function to make the peer-mgr changes a little more readable
gtk+libtransmission-ratiolimit-v8.patch (41.8 KB) - added by JasonBorden 13 years ago.
Torrents now default to use the global setting for ratio limiting
gtk+libtransmission-ratiolimit-v9.patch (41.7 KB) - added by charles 13 years ago.
merge the two v8 patchfiles and resync w/trunk

Download all attachments as: .zip

Change History (36)

Changed 13 years ago by JasonBorden

Ratio limit support for libtransmission

Changed 13 years ago by JasonBorden

Now with 50% less SIGSEGVs

Changed 13 years ago by livings124

GTK+ seeding ratio limiting support

comment:1 Changed 13 years ago by livings124

added patch from #1788

comment:2 Changed 13 years ago by livings124

This looks pretty good. Could I recommend adding the option to have a single global setting (look at how tr_speedlimit works), so that a user can set one ratio setting for "global", and then each individual torrent can have no limit, a ratio limit, or use the global limit. This is how the Mac client behaves.

comment:3 Changed 13 years ago by livings124

Can I also suggest a callback similar to tr_torrentSetCompletenessCallback() so that each ui can perform some actions when the ratio is reached.

comment:4 Changed 13 years ago by charles

livings124: you're gtk hacking now?! nice.

comment:5 Changed 13 years ago by livings124

charles: #1788 ;)

Changed 13 years ago by JasonBorden

Patch includes the requested changes and bug fixes

comment:6 Changed 13 years ago by JasonBorden

I've posted the patch that changes behavior to the same way tr_speedlimit works, and added support for callback functionality. I haven't added an implementation of the callback in the gtk client, however, because I'm not sure how you'd like the callback used. Let me know if you'd like any more changes or if you find any bugs.

comment:7 Changed 13 years ago by livings124

Looking good so far. A comment based on quick look: Using tr_speedlimit is confusing - perhaps make a new enum for ratio or rename the enum. Also, sticking ratio in tr_direction doesn't make much sense; that should be separated.

Changed 13 years ago by JasonBorden

makes tr_ratiolimit enum, removes TR_RATIO from tr_speedlimit, and fixes gtk/main.c's missing patch from v2

comment:8 Changed 13 years ago by JasonBorden

Patch v3 should have the needed fixes to tr_speedlimit and tr_direction. Let me know if you'd like any more changes or if you find any bugs.

comment:9 Changed 13 years ago by livings124

Why did you move a lot of peer-msgs.c to peer-msgs.h?

comment:10 Changed 13 years ago by charles

Hi Jason,

Overall the patch looks really good, and in the spirit of the existing codebase. Thanks for submitting such a good patch.

Here are a couple of comments I have on it:

Important items

  • I don't think peer-io knowing about peer-msgs and tr_torrent is a good idea. peer-io is supposed to be lower level than those two classes. I think what you're intending there would be better put in peer-mgr.c's peerCallbackFunc() handler for "TR_PEER_PEER_GOT_DATA" which is invoked whenever we send data to peer.
  • With that point in mind, I think the changes to peer-msgs.[ch] can be rolled back, so that tr_peermsgs is an opaque class again.
  • The RPC changes need to be documented in the "session-set", "session-get", "torrent-set", "torrent-get", and "Protocol Versions" sections of docs/rpc-spec.txt
  • The torrent-get version of ratio-limit and ratio-limit-enabled are not implemented.

Minor items

  • You don't need to change anything in fastresume.c -- that file is kept only for backwards compatiblity. That wasn't clear from the code, so I've added a comment explaining this.
  • The "assert( tor != NULL );" calls should be "assert( tr_isTorrent( tor ) );"
  • tr_torrentSetRatioLimit() and tr_torrentGetRatioLimit() need the tr_isTorrent() assertions
  • The default ratio should be 2.0 rather than 1.5
  • The gtk+ user interface was kind of hard to follow. I'm attaching a draft revision that reworks the gtk+ preferences & options dialogs. It takes up more space but IMO is easier to understand.

Changed 13 years ago by charles

differences from v3: (1) modified the gtk+ prefs & details dialogs (2) revert fastresume.c

comment:11 Changed 13 years ago by charles

xref: #671

comment:12 Changed 13 years ago by JasonBorden

Ok, I guess I've got some explaining to do.

My original patch was calling tr_torrentStop in the TR_PEER_PEER_GOT_DATA part of peerCallbackFunc, this however was causing seg faults to occur. Debugging through gdb revealed that functions that eventually called peerCallbackFunc were later trying to access its torrent pointer, which had become invalid ( = -1 ). In attempt to remedy the situation, I moved tr_torrentStop up the calling stack, eventually to the original calling function event_write_cb. This was the reason I moved the data struct for tr_peermsgs from peer-msgs.c to peer-msgs.h . In hind sight, it probably would've been easier to figure out why the torrent pointer was becoming invalidated and fixed that instead. So, yes, I'll be working on that today and then we can hopefully get tr_torrentStop back into peerCallbackFunc. I'll get the other issues fixed as well.

Changed 13 years ago by charles

differences from v4: (1) add transmission-remote support (2) in "torrent-set", replace "ratio-limit-enabled" with "ratio-limit-mode" (3) make the enum values in transmission.h explicit for the benefit of rpc spec readers who might not be familiar with C's enum defaults

comment:13 Changed 13 years ago by charles

Since you're working on a patch I'll stop changing things, so that our diffs don't shear. :)

I don't think much of v5 will affect you, since most of it is in the daemon/ directory adding support to the "transmission-remote" application

Changed 13 years ago by JasonBorden

Reverted back to using tr_torrentStop in peerCalllbackFunc. Added sanity checks in didWrite and didWriteWrapper. Added code to select the appripriate radio button when the details page is opened. Added code to remove the "prefs-changed" signal from the core when the dialog is closed.

comment:14 Changed 13 years ago by JasonBorden

The changes in the v6 patch undid all the peer-msgs.c -> peer-msgs.h stuff, and put the torrent stopping functionality back in peerCallbackFunc where it belongs. I was a little bit off on my original assessment of the torrent pointer being invalidated, it was actually the peer pointers being invalidated. This makes sense since stopping the torrent causes the peers to disconnect and free up their resources. To remedy this I merely added a sanity check to didWrite and didWriteWrapper to make sure the data they were accessing was valid. I also did a few fixes on the gtk code. I made the dialog select the appropriate radio button for the torrent's settings when the torrent preference dialog is created. I also added code to remove the prefs-changed signal when the torrent preference dialog is closed, as it was attempting to send signals to the dialog box even after it had closed, sometimes crashing the ui. As always, let me know if there's other changes to be made or bugs to be squished.

Changed 13 years ago by JasonBorden

Minor improvment to the didWriteWrapper sanity check

comment:15 Changed 13 years ago by livings124

For calculating the ratio you should use tr_getRatio. An example of how it's used is in torrent.c:831.

Changed 13 years ago by JasonBorden

1) Now uses tr_getRatio to calculate the current seeding ratio. 2) Removed some g_message debug messages from the spin dials in the GTK+ client

comment:16 Changed 13 years ago by charles

  • Milestone changed from None Set to 1.60
  • Owner set to charles
  • Status changed from new to assigned
  • Version changed from 1.42+ to 1.42

Changed 13 years ago by charles

changes from v7: (1) fix a bug in rpcimpl.c's sessionGet() that assigned the wrong key for the ratio 'enabled' flag (2) add tr_torrentGetSeedRatio() convenience function to make the peer-mgr changes a little more readable

comment:17 Changed 13 years ago by charles

JasonBorden?:

The patch is looking pretty good. Thanks for catching that dangling prefsChanged signal, too. ;)

One last thing I'm not sure I understand is the "if( !( loaded & TR_FR_RATIOLIMIT ) )" block in torrentRealInit(). If we can't load a ratio limit setting from the resume file, wouldn't we always want the fallback to be TR_RATIOLIMIT_GLOBAL?

Changed 13 years ago by JasonBorden

Torrents now default to use the global setting for ratio limiting

comment:18 Changed 13 years ago by JasonBorden

You're absolutely right. I was actually thinking about this before I went to bed last night and forgot all about it today.

comment:19 Changed 13 years ago by charles

I realize that my patch from last night misspelled "ratio" as "rate" in the filename, but we now have two v8 patches, and yours ignores the changes that I put into mine, including the rpcimp.c bugfix.

Once that's taken care of, and once the rpc-spec.txt file is updated to reflect the rpc changes, IMO this patch is ready to be checked in.

I wish all submitted patches went through this kind of a cycle. Thanks for the timely responses.

Changed 13 years ago by charles

merge the two v8 patchfiles and resync w/trunk

comment:20 Changed 13 years ago by charles

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

committed to trunk in r7888

comment:21 Changed 13 years ago by qqlapraline

We have tried to implement the patch over the regular 1.51 tag and it seems that setting an ratio-limit on a per torrent basis does now work using RPC. I have tried to use the field "ratio-limit" or "seedRatioLimit" or "UploadRatio?" - the doc on RPC is unclear - but it does not work... Any hint ? Regards.

comment:22 Changed 13 years ago by livings124

Don't use the patch - use the current trunk source code.

comment:23 Changed 12 years ago by sim

(deleted spam)

Last edited 12 years ago by charles (previous) (diff)
Note: See TracTickets for help on using tickets.