Opened 4 years ago

Last modified 23 months ago

#5923 new Bug

Webseeds: patch to address multiple issues

Reported by: x190 Owned by: jordan
Priority: Normal Milestone: 3.00
Component: libtransmission Version: 2.84
Severity: Normal Keywords: webseeds, patched
Cc:

Description

  • Full blocklist/whitelist (#5907) integration with IP addresses provided.
  • Full support for the '5 bad pieces' rule.
  • Basic integration with maxConnectedPeers --- webseeds have priority.
  • Fixes #4677 - Webseeds should be filtered by loaded blocklists.
  • Fixes #5837 - Stopping a torrent does not stop downloading from webseeds.
  • Fixes #5873 - Corrupt webseed causes excessive bandwidth usage.
  • Includes the patch for #5912 - corrupt blocks overwrite previously verified OK blocks.

Attachments (9)

fix_webseed_issues_take2.patch (29.4 KB) - added by x190 4 years ago.
5912.patch (1.6 KB) - added by x190 4 years ago.
5907.patch (732 bytes) - added by x190 4 years ago.
integrate_webseeds_with_max_peers.patch (3.0 KB) - added by x190 4 years ago.
5912.2.patch (930 bytes) - added by x190 4 years ago.
5912.3.patch (1.3 KB) - added by x190 4 years ago.
5923-webseeds-against-r14714.patch (27.5 KB) - added by mike.dld 3 years ago.
fix_webseed_issues_take3.patch (44.0 KB) - added by x190 3 years ago.
update.webseed.c.patch (1.9 KB) - added by x190 3 years ago.

Download all attachments as: .zip

Change History (44)

Changed 4 years ago by x190

comment:1 Changed 4 years ago by x190

  • Keywords webseeds patched added

comment:2 Changed 4 years ago by mike.dld

Is it possible to address multiple issues with multiple separate patches? I'd like to try merging this in, and small atomic changes are always better in case of possible new defects. Maybe not the same number as the resolved issues listed, but at least 2-4 patches...

BTW, the right term should probably be "blacklist(ed)" (as opposed to "whitelist(ed)") instead of "blocklist(ed)".

Changed 4 years ago by x190

comment:3 Changed 4 years ago by x190

Fix for #5912 --- corrupt blocks overwrite previously verified OK blocks --- provided by cfpp2p. (patch to r14549)

See updated patch below: https://trac.transmissionbt.com/ticket/5923#comment:9

Last edited 4 years ago by x190 (previous) (diff)

Changed 4 years ago by x190

comment:4 Changed 4 years ago by x190

Fix for #5907 --- Whitelist, takes precedence over blocklist --- provided by cfpp2p. (patch to r14549)

Info needs to be added to wiki.

https://trac.transmissionbt.com/wiki/Blocklists

Last edited 4 years ago by x190 (previous) (diff)

Changed 4 years ago by x190

comment:5 Changed 4 years ago by x190

...not meant to be the last word on the subject, but might be useful for those with router issues, or it could be subtracted from 'fix_webseed_issues_take2.patch​', if you don't want it.

(patch to r14549)

Last edited 4 years ago by x190 (previous) (diff)

comment:6 Changed 4 years ago by mike.dld

Fixed #5912 in r14550, with patch provided by cfpp2p and not the one attached above since the latter leaves fire_client_got_blocks to be called even when the data is discarded.

EDIT: Hmm, the patch attached is actually a bit malformed (missing closing brace), which I didn't notice right away...

Last edited 4 years ago by mike.dld (previous) (diff)

comment:7 follow-up: Changed 4 years ago by mike.dld

I'd rather not fix #5907, at least not without prior discussion since I'm not sure how valuable is it.

As for the rest, is it possible to make 3 separate patches for:

  • "Full support for the '5 bad pieces' rule" and "Fixes #5873 - Corrupt webseed causes excessive bandwidth usage" (I presume both aim for the same issue)
  • "Fixes #4677 - Webseeds should be filtered by loaded blocklists"
  • "Fixes #5837 - Stopping a torrent does not stop downloading from webseeds"

? I can do that myself, but it might take less time for you.

comment:8 in reply to: ↑ 7 ; follow-up: Changed 4 years ago by x190

Replying to mike.dld:

I'd rather not fix #5907, at least not without prior discussion since I'm not sure how valuable is it.

This needs to be committed as many webseeds will be in a basic blocklist. In any case, it is a simple patch which in no way affects normal blocklist operation and it is a feature that has been requested numerous times in the forum.

As for the rest, is it possible to make 3 separate patches for:

  • "Full support for the '5 bad pieces' rule" and "Fixes #5873 - Corrupt webseed causes excessive bandwidth usage" (I presume both aim for the same issue)

'5 bad pieces' rule uses a 'blame' bitfield, while corrupt (non-206) webseeds are aborted by writeFunc(), but keeping them out involves a similar mechanism.

  • "Fixes #4677 - Webseeds should be filtered by loaded blocklists"
  • "Fixes #5837 - Stopping a torrent does not stop downloading from webseeds"

? I can do that myself, but it might take less time for you.

I would really like to see you do it, as then I will know you have read and understood the code. I would be happy to try to answer any questions you may have. All of these features have been quite well tested with both public torrents and a suite of test torrents provided by cfpp2p.

#4677 uses task_addr_is_blocklisted() in web.c, while #5837 uses progress_func(), also in web.c.

Changed 4 years ago by x190

Changed 4 years ago by x190

comment:9 Changed 4 years ago by x190

Last edited 4 years ago by x190 (previous) (diff)

comment:10 in reply to: ↑ 8 Changed 4 years ago by mike.dld

Replying to x190:

I would really like to see you do it, as then I will know you have read and understood the code. I would be happy to try to answer any questions you may have. All of these features have been quite well tested with both public torrents and a suite of test torrents provided by cfpp2p.

Fair enough... Thanks for making the patch in the first place.

comment:11 Changed 4 years ago by mike.dld

Closed #5969 as duplicate of this ticket.

comment:12 Changed 4 years ago by mike.dld

After applying the rest of the patch, I notice Transmission starting to print bencoded data (looks related to announces) to stdout (fd 1). Stdout isn't being closed before that, so it's not a descriptor reuse issue. The write originates from curl used inside tr_webThreadFunc. I think it's connected to the fact that prevously CURLOPT_WRITEDATA and CURLOPT_WRITEFUNCTION were set unconditionally, but now they are only set if task->range != NULL, and default to stdout+fwrite otherwise (per curl documentation and source code). createEasy function isn't used solely for webseeds, hense the check for range is not valid in all cases, or at least not for all the curl options being set.

comment:13 Changed 4 years ago by cfpp2p

Within createEasy function I've been using a variation of the patch where CURLOPT_WRITEDATA, CURLOPT_WRITEFUNCTION and other curl options being set are left as they were.

task->freebuf == NULL always indicates a webseed. I'm not getting announces printing bencoded data to stdout (fd 1) this way.

  if (task->freebuf == NULL) 
     {
      curl_easy_setopt (e, CURLOPT_PROGRESSFUNCTION, progress_func);
      /* pass the struct pointer into the progress function */
      curl_easy_setopt (e, CURLOPT_PROGRESSDATA, task);
      curl_easy_setopt (e, CURLOPT_NOPROGRESS, 0L);
     }

comment:14 Changed 4 years ago by x190

CURLOPT_WRITEDATA and CURLOPT_WRITEFUNCTION should be outside the 'if (task->range != NULL)' conditional in createEasy ().

Line 546-548 (libtransmission/webseed.c) of the patch (tr_webseed_add_strike () ) needs a minor formatting clean-up.

fix_webseed_issues_take2.patch​: libtransmission/web.c:

-281	      curl_easy_setopt (e, CURLOPT_WRITEDATA, task);
-282	      curl_easy_setopt (e, CURLOPT_WRITEFUNCTION, writeFunc);

+251	      curl_easy_setopt (e, CURLOPT_WRITEDATA, task);
+252	      curl_easy_setopt (e, CURLOPT_WRITEFUNCTION, writeFunc);
Last edited 3 years ago by x190 (previous) (diff)

comment:15 Changed 4 years ago by x190

---

Last edited 4 years ago by x190 (previous) (diff)

comment:16 follow-up: Changed 3 years ago by mike.dld

Since I've been blamed for not giving this a go (I presume), I need to notify you guys that I tried :) The most recent issues I had were:

  1. n == tor->info.webseedCount assert in tr_peerMgrWebSpeeds_KBps() being hit, IIUC caused by change in rebuildWebseedArray() (loop is now for up to MAX_WEBSEEDS_PER_SWARM iterations instead of inf->webseedCount),
  2. invalid (random) webseed speeds display in GTK+ and Mac clients after I commented out the above assert, even if torrent is idle.

I'm attaching current patch I'm using (based on the original, against trunk). I've fixed formatting a bit, and removed unneeded changes (hopefully I didn't remove anything that was in fact needed). I may have also skipped some changes proposed in previous comments, so speak up in they are still necessary.

Changed 3 years ago by mike.dld

comment:17 in reply to: ↑ 16 Changed 3 years ago by cfpp2p

Replying to mike.dld:

... I need to notify you guys that I tried :) ...

Thank you.

The assert something like this

  assert (n == MIN (tor->info.webseedCount, MAX_WEBSEEDS_PER_SWARM));

I think might be the correct.

I'm unclear right now why we're using tr_peerIsTransferringPieces() rather than tr_peerGetPieceSpeed_Bps() to determine the speeds.

I believe that with a little of x190's help we could fix all the issues.

comment:18 follow-up: Changed 3 years ago by x190

Let me know when you have a valid question that can't be fixed by yourself in about 2 minutes or less.

svn does not work for me and I lost access to the Mac Client long ago, however, I have by literal 'blood, sweat, and tears' provided a functioning patch for all these issues, portions of which have already been usurped by the very one who is now suggesting I should 'help', while at the time the patch was produced, steadfastly refused to do any testing on my behalf!

If you want it bad enough, I know you can make it work, and yes, if you have a question that I can address, I will try to provide an answer.

comment:19 in reply to: ↑ 18 ; follow-up: Changed 3 years ago by mike.dld

I have absolutely no idea what you're talking about, but I'm surely trying to work this out on my own (see my comment:10 and your comment:8). Just please don't blame me for not accepting the patch which isn't functioning after all, no matter how hard you worked on it... Hopefully you understand.

comment:20 in reply to: ↑ 19 Changed 3 years ago by x190

The patch worked very well for me; I would not have posted it otherwise. The issues raised so far should be extremely easy to fix and the bit about speed display seems unlikely to be related to my patch, but I can't be sure since I only had access to the daemon. I believe a skilled programmer such as yourself could easily fix these issues, or rewrite sections of the patch, if you felt it was important for the project.

I understand all too well how the Transmission Project 'works' and, without assigning blame, right now it hurts real bad to contemplate the result.

comment:21 Changed 3 years ago by cfpp2p

Replying to x190:

Let me know when you have a valid question that can't be fixed by yourself in about 2 minutes or less.

svn does not work for me and I lost access to the Mac Client long ago, however, I have by literal 'blood, sweat, and tears' provided a functioning patch for all these issues, portions of which have already been usurped by the very one who is now suggesting I should 'help', while at the time the patch was produced, steadfastly refused to do any testing on my behalf!

If you want it bad enough, I know you can make it work, and yes, if you have a question that I can address, I will try to provide an answer.

Regarding "... usurped by the very one who is now suggesting I should 'help', while at the time the patch was produced, steadfastly refused to do any testing on my behalf!"
If in fact "the one" was in reference to the prior collaborative work of x190 and myself I must agree that is was a "literal 'blood, sweat, and tears'" effort for both parties. As in all projects worthy of extreme effort the side effects of such exertions can be misconceptions and misunderstandings.

At comment:19 mike.dld responds "I have absolutely no idea what you're talking about.." in reference to the same. The results seemingly wanted by all here working together on x190's Webseeds: patch to address multiple issues ticket is a functional and well developed patch for the 2.9x branch.

For a completed and well tested webseed development for the 2.77 branch see:
Commits on Jan 27, 2015 through Commits on Mar 31, 2015 INCLUSIVE
https://github.com/cfpp2p/transmission/commits/svn/trunk?after=8e73c16a1801589c08df72029d18a609196788eb+34

and
https://github.com/cfpp2p/transmission/commit/946207b895ebf7c228178ca7cb205aea3ba39c8e
https://github.com/cfpp2p/transmission/commit/d2143dceb11776841a76b05ff395a9ba1122ebeb

Although x190 at some point took a significantly different developmental direction to the issues, and the fixes for the 2.9x branch will likely do the same, I am willing to help when I can.

Last edited 23 months ago by cfpp2p (previous) (diff)

comment:22 Changed 3 years ago by x190

Last edited 3 years ago by x190 (previous) (diff)

comment:23 Changed 3 years ago by mike.dld

  • Milestone changed from None Set to 3.00

comment:24 follow-up: Changed 3 years ago by x190

@cfpp2p: Are you willing to provide your test suite of webseed torrents exhibiting faults addressed by my patch, including the current archlinux one? If so, please attach them here clearly labeled as to what is being tested.

comment:25 Changed 3 years ago by x190

I stand by my original patch. So far only very minor one line changes are required as mentioned above (and 'freebuf') is not one of them.

I am able to build the daemon only, so must leave testing the gui apps to others, although I did find the daemon + Web Interface worked perfectly.

This patch was quite technically complicated to produce (for me at least, a complete amateur), so I'm waiting for someone to point out a major error.

The archlinux iso is a good public torrent to test with, as it has over 200 webseeds, although cfpp2p also has a test suite that demonstrates other issues, such as a webseed sending 'bad blocks' or a webseed sending 'non-206' data, and others.

comment:26 in reply to: ↑ 24 Changed 3 years ago by cfpp2p

Replying to x190:

@cfpp2p: Are you willing to provide your test suite of webseed torrents exhibiting faults addressed by my patch, including the current archlinux one? If so, please attach them here clearly labeled as to what is being tested.

Attached is inclusive of all that I tested, from archives. I believe everything that's direct on server should still be testable.

comment:27 Changed 3 years ago by cfpp2p

couldn't attach, kept receiving 500 internal server error

so here's a link
5923-test-torrents.zip

those at #5969 should be tested too

Last edited 3 years ago by cfpp2p (previous) (diff)

comment:28 Changed 3 years ago by x190

I have tried to proofread https://trac.transmissionbt.com/attachment/ticket/5923/5923-webseeds-against-r14714.patch and I do see a number of changes that may, in fact, materially change the way the patch works.

I think it should be up to the one making the changes to justify and test said changes.

  • comment:14 change is required to fix http trackers.
  • comment:16 assert should be removed.
  • The Mac Client and GTK+ may need a little tweaking in their 'webseed' code --- I cannot build or test either one for current trunk.
  • One developer seemed to be quite happy with the basic original patch, as noted here.
Last edited 3 years ago by x190 (previous) (diff)

comment:29 follow-up: Changed 3 years ago by jordan

A few comments on 5923-webseeds-against-r14714.patch

transmission.h

  • Looks like next_webseed_i should be in tr_torrent rather than tr_info, since it's not a public variable. If it *has* to be in tr_info because of $reasons, then we should add a comment loudly shouting that it's an implementation field and client code shouldn't touch it.

web.[ch]

  • Implementing blacklist support by aborting the transfer from progress_func() and writeFunc() seems suboptimal, since it implies we've already opened a connection to the webseed and have started downloading from it before progress_func() gets a chance to abort.

It would be much better to blacklist before the connection is made; eg. by setting CURLOPT_OPENSOCKETFUNCTION to a wrapper function around socket() that converts the curl_sockaddr argument into a tr_address, checks for a blacklist (or whitelist) entry, and returns CURL_SOCKET_BAD if we don't want to connect to it.

  • writeFunc(): if the blacklist is changed as described above, then the blacklist test won't be necessary here.

if ((task->code != 206 && byteCount > 0) || byteCount == 0) ... task->aborted = true I must be reading this wrong, looks like an HTTP OK with >0 bytes would be aborted?

Also, do we need the 'abort' flag at all? It seems to be used for two cases: (1) blacklisting (which isn't needed here, it can be fixed by handling blacklisting as described above) and (2) handling webseeds that give us bad data, but writing 0 bytes will cause the curl task to abort anyway, so we'd still reach webseed.c's callback anyway and we could still test for 206 and !bytesWritten there without needing to change the public API with the 'abort' flag.

  • the function name "task_addr_is_blocklisted" should be prefixed by "tr_"
  • the function task_addr_is_blocklisted() is being leaked into the public API. Suggest moving the declaration to an existing header with the TRANSMISSION include guard, or a new web-priv.h header if no existing one is appropriate
  • the function task_addr_is_blocklisted() sets the `abort' flag, but the function name implies the function is an accessor. This is more confusing when on the line "if (aborted && task_addr_is_blocklisted (t->web_task))" where that 'aborted' flag comes from the web.c callback, is it the same flag?
  • trivial, task_addr_is_blocklisted() should return 'true' and 'false' instead of 1 and 0
  • The new tr_web_task field 'task_addr' appears to be set but not used. Why do we have this new field?
  • Not new to this code, but I we really are bleeding a lot of libcurl out through web.h. We should look at moving the curl include and veneer functions like tr_webGetTaskInfo() into a web-priv.h header mentioned in point 2 above :)

webseed.c

  • struct tr_webseed doesn't need a new blame and strike_count field, it already has parent.blame and parent.strikes.
  • I don't understand what addr_checked is for. Please explain it to me.
  • MAX_BAD_PIECES_PER_PEER and MAX_BAD_PIECES_PER_WEBSEED are redundant. The former should be moved into peer-mgr.h where webseed.c can use it, and the latter should be removed.
  • on_idle(): Missing braces + indent before and after the 'want = 1' line
  • tr_webseed_set_error() and clear_error(): Big picture, I'm not sure this feature is actually needed -- we don't have a similar "all peers are blacklisted" message for BT peers, and I wonder if this error message might step on some other error message.

For the sake of argument, though, if we do keep this it should be done by refactoring tr_torrent{Set,Clear}LocalError?() to give webseed.c a utility function it can use. Duplicating the the tor->isStopping and tor->errorState semantics here is just asking for trouble down the road.

Also, saying "this torrent is running, and isn't a seed, and isn't in the middle of stopping, but we're not currently getting piece data from a webseed" DOES NOT MEAN all the webseeds are blacklisted / struck out! It's super misleading.

  • unexplained "TODO: eliminate" line. Is it saying set_error and clear_error shouldn't be calling countActiveWebseeds()?
  • web_response_func(): see above comment on web.c's writeFunc().

I don't really understand the change of "if (!success)" to "if (!success && !w->is_blocklisted)" but it looks like it would cause us to slowly corrupt the value of idle_connections a few lines down. I'm not sure about that though. This code (both pre- and post-patch) is too complicated for its own good.

if (!w->addr) .. tr_logAddInfo(tor, "webseed %s is not returning a valid address.", w->addr) WAT :-)

  • tr_webseed_add_strike(): This should be deep logging, just like tordbg() calls in peer-mgr.c's addStrikes() function. Also, testing w->addr != NULL before printing it.

session.c

  • what's the real-world use case for having IP whitelists?

If this is really necessary, would prefer a helper function, eg bool tr_blocklistIsWhitelist(const tr_blocklist*)

peer-mgr.c

  • Changing the semantics of getMaxPeerCount() like this is really broken. The function only does one thing, but now it doesn't do what it says. Haven't thought this through fully, but may instead getPeerCount() should be changed to return p2p peers + webseed peers?
  • enforceTorrentPeerLimit(): hardcoding 4x multiplier will break if MAX_WEBSEED_CONNECTIONS changes in the future. Instead, let's move it to peer-common.h (and rename as MAX_CONNECTIONS_PER_WEBSEED to be clearer?) so that both webseed.c and peer-mgr.c can use it.
  • poke_webseeds: I don't understand this function at all. There doesn't appear to be any memory management of the webseeds being removed at all.

The error logging mistakes of tr_webseed_set_error() and clear_error() (see above) are being repeated here.

comment:30 Changed 3 years ago by jordan

After sleeping on it a bit, the hackier I think doing per-webseed error states in tor.errorString like this is. Maybe we need a webseed equivalent to tr_peer_stat that could include things like a 'blacklisted' bool. Right now we've got tr_torrentWebSpeeds_KBps(), maybe that should be changed to a sibling function of tr_torrentPeers().

comment:31 in reply to: ↑ 29 Changed 3 years ago by x190

Replying to jordan:

A few comments on 5923-webseeds-against-r14714.patch

transmission.h

  • Looks like next_webseed_i should be in tr_torrent rather than tr_info,

done

web.[ch]

  • Implementing blacklist support by aborting the transfer from progress_func() and writeFunc() seems suboptimal, since it implies we've already opened a connection to the webseed

new patch will do address checking before connect

if ((task->code != 206 && byteCount > 0) || byteCount == 0) ... task->aborted = true I must be reading this wrong, looks like an HTTP OK with >0 bytes would be aborted?

mike.dld made changes, but anyway, a new patch will only apply to webseeds for now, as the original patch originally did.

webseed.c

  • struct tr_webseed doesn't need a new blame and strike_count field, it already has parent.blame and parent.strikes.

done and done

  • I don't understand what addr_checked is for.

redesigned - using status flags

  • MAX_BAD_PIECES_PER_PEER and MAX_BAD_PIECES_PER_WEBSEED are redundant. The former should be moved into peer-mgr.h where webseed.c can use it, and the latter should be removed.

done

  • tr_webseed_set_error() and clear_error():

gone - too intrusive - now can tracker stuff go (for Web Interface anyway)?

I don't really understand the change of "if (!success)" to "if (!success && !w->is_blocklisted)" but it looks like it would cause us to slowly corrupt the value of idle_connections a few lines down. I'm not sure about that though. This code (both pre- and post-patch) is too complicated for its own good.

totally redesigned, but didn't you write this 'complicated' (persnickety - per forum poster) stuff in the first place? :P

session.c

  • what's the real-world use case for having IP whitelists?

so you can whitelist blocklisted addresses, probably essential for webseeds and many forum posters - users can make an informed decision about how to proceed.

peer-mgr.c

  • Changing the semantics of getMaxPeerCount() like this is really broken. The function only does one thing, but now it doesn't do what it says. Haven't thought this through fully, but may instead getPeerCount() should be changed to return p2p peers + webseed peers?

definitely needs to be addressed, but I opened a separate ticket (#6121) for this issue.

  • poke_webseeds: I don't understand this function at all. There doesn't appear to be any memory management of the webseeds being removed at all.

archlinux.iso has 256 webseeds, of which we only start with the first 16. On each restart for the session, poke_webseed_array () will remove any blocklisted, or otherwise non-performing webseeds (404, 503, corrupt data, too many redirects (new - #6110), etc.), and replace them with a full complement of 16 until webseedCount is reached. Poking webseeds is essential, since you got to 'poke the clock' and the peerMgr, and only Texans are allowed to poke rattlesnakes (unless, of course, the snake pokes the Texan first). :) Webseeds poked will now be freed from leaky memory hell.

New patch uses 3 new 'virtual functions' - I'm sure you'll approve!

Last edited 3 years ago by x190 (previous) (diff)

Changed 3 years ago by x190

comment:32 follow-up: Changed 3 years ago by x190

This patch is optimized for webseed testing (see comment:27). You will get scads of dbgmsgs from webseed.c and lesser amounts from web.c. You will need to keep your download speed to a low figure, say 16-24 KB/s to avoid clogging the stderr pipe (around 150 KB/s absolute max). You can disable dbgmsg logging by changing the 1 to 0 in the #define for dbgmsg in webseed.c and web.c after patching, which you must do to test the patch with higher speeds (at least for webseed.c).

To test the daemon on a Mac, run in Terminal.app with the -f option set and set --logfile [logfile.log]. This way a task's journey, from start to finish, will be displayed in Terminal.app, while the Message Log stuff will be displayed in Console.app.

To apply this patch, download it and place it in the previously downloaded source code directory (from svn: for Mac):

svn co svn://svn.transmissionbt.com/Transmission/trunk Transmission

...and run the patch with the following command in Terminal.app after 'cd'ing to the source code directory:

patch -p0 < ./fix_webseed_issues_take3.patch

N.B. I have included a test version of write_block_func () in webseed.c that avoids overwriting blocks as well as pieces. The original code is still there; just comment out/uncomment one or the other, but I would appreciate testing with patch code as is.

Last block write in web_response_func () has been changed for this as well, but is more straightforward.

Enjoy your testing, and please comment. If I get any positive feedback from management, I can produce a more commit-ready version of the patch.

EDIT: After applying this patch, please apply the one from #comment:34.

Both should work with r14732 and up to r14736 (current revision).

Last edited 3 years ago by x190 (previous) (diff)

comment:33 Changed 3 years ago by cfpp2p

I like your implementation of MAX_REDIRECT_COUNT however, if you're checking for exceeding the count in sockoptfunction() I don't see the reason for curl_easy_setopt (e, CURLOPT_MAXREDIRS, MAX_REDIRECT_COUNT); as it's not needed. Counting the redirects yourself makes this unnecessary. r12539 introduced tr_webGetTaskInfo( task->web_task, TR_WEB_GET_REDIRECTS, &redirects ); and TR_WEB_GET_REDIRECTS = CURLINFO_REDIRECT_COUNT, but CURLOPT_MAXREDIRS wasn't necessary for functionality. r12860 removed tr_webGetTaskInfo( task->web_task, TR_WEB_GET_REDIRECTS, &redirects ); in favor of using tr_webGetTaskInfo( task->web_task, TR_WEB_GET_REAL_URL, &url );, TR_WEB_GET_REDIRECTS = CURLINFO_REDIRECT_COUNT, remained however. That would have been a good place for MAX_REDIRECT_COUNT, but anyway the way you have it you've got it's fixed for not just webseeds, which is great!

I don't particularly like using parent for the blame and strike, but I see management indicates a preference. typedef struct tr_peer as defined by peer-common.h has a few unused fields for webseeds implicated. A blame bitfield is allocated on the fly as needed by `tr_bitfieldEnsureBitsAlloced()' so it wouldn't waste to create a new field for webseeds. I think it would be more definitive that way.

I like separate MAX_BAD_PIECES_PER_PEER and MAX_BAD_PIECES_PER_WEBSEED. Webseeds don't behave the same way as regular torrent peers. Additionally, a large difference in the number of regular peers and webseeds is sometimes the norm. These dictate a reason to me for available different values.

Changed 3 years ago by x190

comment:34 Changed 3 years ago by x190

Please apply this patch (update.webseed.c.patch) after applying:

patch -p0 < ./fix_webseed_issues_take3.patch

Fixes webseed.c's 'webseed_add_strike ()' behavior for the new write_block_func () and adds that functionality to the currently commented-out original function.

Last edited 3 years ago by x190 (previous) (diff)

comment:35 in reply to: ↑ 32 Changed 3 years ago by x190

Replying to x190:

This patch is optimized for webseed testing (see comment:27). You will get scads of dbgmsgs from webseed.c and lesser amounts from web.c. You will need to keep your download speed to a low figure, say 16-24 KB/s to avoid clogging the stderr pipe (around 150 KB/s absolute max). You can disable dbgmsg logging by changing the 1 to 0 in the #define for dbgmsg in webseed.c and web.c after patching, which you must do to test the patch with higher speeds (at least for webseed.c).

To test the daemon on a Mac, run in Terminal.app with the -f option set and set --logfile [logfile.log]. This way a task's journey, from start to finish, will be displayed in Terminal.app, while the Message Log stuff will be displayed in Console.app.

...

EDIT: After applying this patch, please apply the one from #comment:34.

Both should work with r14732 and up to r14736 (current revision).

Further testing indicates that the dbgmsg logging issue may be more related to my older iMac than speed issues. In any case, using the following code, as a replacement, in web.c (Line 47 - dbgmsg must be defined - set "webseed" to "web") and webseed.c (Line 28 - both can be 0 - undefined) should solve the issue.

#if 1
#define dbgmsg(...) \
  do { \
    fprintf (stderr, __VA_ARGS__); \
    fprintf (stderr, "\n"); \
    fflush (stderr); \
  } while (0)
#elif 0
#define dbgmsg(...) \
  do { \
    if (tr_logGetDeepEnabled ()) \
      tr_logAddDeep (__FILE__, __LINE__, "webseed", __VA_ARGS__); \
  } while (0)
#endif


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