Does this actually work?

After you have edited the source code, post your patch here.
Forum rules
Home | Wiki | Rules

Re: Does this actually work?

Postby old_death » 08 Apr 2011 02:50

I have reviewed my code for things to clean up - and I couldn't find any. Could you add a bit more precision to what exactly you think I should clean up, ailurophobe?

This new version contains additions to some ASSERTS as well as fixes for two minor issues I detected while reviewing the code...
Attachments
security code 2011-04-07b.7z
(279.54 KiB) Downloaded 215 times
User avatar
old_death
 
Posts: 1950
Joined: 13 Jun 2009 16:19

Re: Does this actually work?

Postby ailurophobe » 08 Apr 2011 11:48

Not looking at your code right now but speaking in the past tense and explaining my previous comment things like:

Changing the way miss cache worked so that the comment attached no longer matches... (Not sure of that, but "IIRC"... LOL)

Having multiple separate if clauses to handle one case, the "clean" way is to have all code handling one case in one place, either in inside one if block or in this case (handling different rule types) using switch case statements.

Having complex structures like multiple separate address maps (plus a set) without it being obvious what the separate maps are needed for. This is problematic because maps are O(log N) so splitting them almost always hurts performance and unless explained will "look like" bad, messy code. (And clean/messy is about impressions, which is important since "clean" code is easier to understand and work with. Which is kind of important when talking about committing the code to a project with very clean coding style.)

Having a miss cache that is actually an allow cache; sounds nit-picking but unless you actually read and understand the code you are going to assume it does something different from what it now does, a potential source of bugs when working something connected but different.

EDIT:
And all those cache evaluations obscure when the cache is used and when not. Also there is a lot of them, are you sure that adding constant overhead to operations that otherwise would be O(log N) really speeds it up? If you have a miss cache at all I find it hard to believe that constantly checking if to use it or not is faster than just using it always. This is because realistically transitions between the two modes should be very rare and using miss cache when it is not needed is pretty fast. Basically there is unlikely to be a case where it makes sense to have a cache at all, but it is useful to constantly check is you should enable or disable it.

Also handling single addresses and ranges as different rule types is not really optimal, it would be much "cleaner" to use structures that handle all ranges correctly including the single address special case. Faster too, obviously. It is complex enough that I couldn't bothered but if you are basically rewriting the security manager anyway...

map<mask,map<address,rule>> or something like that. On look-up iterate thru the mask map and check the maps for address masked by the mask of the map.

END EDIT

Seriously though, I am not a coding style critic or a great authority on coding. What I was really talking about was my impression of ryo which is that he is a great believer in consistent and very clean and easy to understand style of coding, which your code doesn't match. Which made me suspect that ryo might have issues committing your code without a rewrite by either himself or you. Obviously just utter speculation on my part, though. (And IIRC with the scheduler code he did a commit first, rewrite later.)
ailurophobe
 
Posts: 709
Joined: 11 Nov 2009 05:25

Re: Does this actually work?

Postby old_death » 08 Apr 2011 16:14

User avatar
old_death
 
Posts: 1950
Joined: 13 Jun 2009 16:19

Re: Does this actually work?

Postby ailurophobe » 08 Apr 2011 21:41

ailurophobe
 
Posts: 709
Joined: 11 Nov 2009 05:25

Re: Does this actually work?

Postby old_death » 09 Apr 2011 04:07

I promise I'll do some changes once I am back. :mrgreen:
User avatar
old_death
 
Posts: 1950
Joined: 13 Jun 2009 16:19

Re: Does this actually work?

Postby cyko_01 » 09 Apr 2011 13:01

User avatar
cyko_01
 
Posts: 938
Joined: 13 Jun 2009 15:51

Re: Does this actually work?

Postby ailurophobe » 11 Apr 2011 02:11

IIRC the reason some people wanted this fell into one of two categories:

1. Their ISP had a limit or extra charge on connecting to foreign addresses, so they wanted block foreign addresses and allow domestic ones.
2. Worried about the copyright cops and wanted to block domestic addresses and allow foreign ones.

Not sure if either of those is still relevant. It might indeed be better to include this (if it is wanted) to the country code. Maybe add an advanced setting that if true makes shareaza only connect to domestic peers. (No real point wasting time on supporting the category 2, just point out that the companies that do the look ups work international nowadays if somebody asks for it.)
ailurophobe
 
Posts: 709
Joined: 11 Nov 2009 05:25

Re: Does this actually work?

Postby cyko_01 » 11 Apr 2011 13:07

AFAIK there is no other P2P app that has this feature so failing to add it would not put us at a disadvantage. If we were to add this then it would severely restrict sources and make downloading much more difficult, if not impossible - thus giving shareaza a bad reputation from people who do not understand how P2P works (who think shareaza is just a bunch of servers) and decide to use this feature.
User avatar
cyko_01
 
Posts: 938
Joined: 13 Jun 2009 15:51

Re: Does this actually work?

Postby ailurophobe » 12 Apr 2011 01:46

Therefore I said "an advanced setting", something people don't click by accident, but that we can guide anyone who actually needs to avoid connecting to foreign peers. But as I implied I am not sure if any ISPs still have such policies. Some countries in Africa, the Middle East, and the Pacific have pretty limited global connections, though. In such countries prioritizing domestic peers could improve performance. Would need to be sure that the option is only enabled for leaf mode not hubs, though. Splitting the network to separate national networks, even partially, would have a bad impact on search performance. Maybe that was what ryo was worried about?
ailurophobe
 
Posts: 709
Joined: 11 Nov 2009 05:25

Re: Does this actually work?

Postby old_death » 22 Apr 2011 15:39

So, just to keep you informed about what is currently being done: I've made the miss cache become a miss cache again to make sure the rule counters are correct again, some bug with the default policy not being added to the list has been fixed etc. etc. etc.

Right now I am thinking about the following:

If a new rule is added, all the IPs everywhere within Shareaza are checked against all rules again (AFAIK). It might be possible to check them only against the newly added rules, which could remove the lag on new IP bans almost completely, don't you think?
User avatar
old_death
 
Posts: 1950
Joined: 13 Jun 2009 16:19

Re: Does this actually work?

Postby old_death » 23 Apr 2011 03:47

So, this is what I got so far. It does not crash any more and everything works as expected, except for the move up and down GUI issues. (I have done extensive testing.)

I think this is ready to be committed - except if somebody was willing to help me with that GUI issue...

PS: I have added what I proposed previously...

EDIT: f*ck... there's a memory leak somewhere...
Attachments
security 2011-04-23.7z
Yet an other update...
(300.7 KiB) Downloaded 210 times
User avatar
old_death
 
Posts: 1950
Joined: 13 Jun 2009 16:19

Re: Does this actually work?

Postby old_death » 25 Apr 2011 02:23

BTW, I think Cyko will love me: I managed to completely remove the lag on "Ban User" within searches - or almost at least: When searching for "shareaza" and selecting all files to ban, it takes approximately 30 seconds until all of them are banned. :mrgreen:


EDIT: Seems I forgot to post the code yesterday evening:
security 2011-04-25c.7z
Most recent version of the security code. :)
(273.72 KiB) Downloaded 209 times
User avatar
old_death
 
Posts: 1950
Joined: 13 Jun 2009 16:19

Re: Does this actually work?

Postby old_death » 28 Apr 2011 22:45

I have been running this code for 3 days now without problems... Can anyone else confirm this so that it can be committed?
User avatar
old_death
 
Posts: 1950
Joined: 13 Jun 2009 16:19

Re: Does this actually work?

Postby ailurophobe » 29 Apr 2011 21:10

Ryo has been doing "refactoring" in lots of files recently. I presume you have merged his changes to your code?
ailurophobe
 
Posts: 709
Joined: 11 Nov 2009 05:25

Re: Does this actually work?

Postby old_death » 30 Apr 2011 00:19

My code has been merged as of r8998. I can do the merging with any new version if requested for committing.

EDIT: I've just done so for the latest revision:
security 2011-04-30.7z
Security code merged with r9002.
(274.33 KiB) Downloaded 239 times
User avatar
old_death
 
Posts: 1950
Joined: 13 Jun 2009 16:19

Re: Does this actually work?

Postby ailurophobe » 30 Apr 2011 20:38

Then IMHO it should be merged ASAP so ryo can get started debugging and refactoring it. I doubt ryo wants the extra work, but the security manager does need to be updated and not committing this just wastes time. But that is just my opinion, and being that, apart from as usual being too lazy to help with actual work, I am just flat out going to be gone for few weeks, there is no particular reason anyone should mind my opinion.
ailurophobe
 
Posts: 709
Joined: 11 Nov 2009 05:25

Re: Does this actually work?

Postby old_death » 01 May 2011 00:54

Oh, I've just discovered a way to access a NULL pointer. :) Searching for the bug now. Could take a while though...
User avatar
old_death
 
Posts: 1950
Joined: 13 Jun 2009 16:19

Re: Does this actually work?

Postby old_death » 02 May 2011 18:37

OK, fixed that now (Or at least, I think I did :mrgreen: ).

And here's the code:
security 2011-05-02.7z
Merged with r9010.
(274.61 KiB) Downloaded 232 times


BTW, I don't think it should be committed before introducing the next version (which I suppose will happen soon), to avoid possibly annoying bugs in that version. I think it should be committed directly after this new version has been released to allow extensive testing...

PS.: Maybe someone could help me implement the Ctrl+A switch and drag&drop of rules, which I haven't done yet as IDK enough about the GUI stuff...
PPS.: Cyko, have you tested the code yet? It is amazingly fast at banning people directly from the search window. 8-)
User avatar
old_death
 
Posts: 1950
Joined: 13 Jun 2009 16:19

Re: Does this actually work?

Postby old_death » 02 May 2011 18:43

OK, that was not funny... I've just posted the new code (after 2 days of testing) and *bam*, there it is again... That's called ironic timing...

:evil:

EDIT: Fixed.
security 2011-05-03.7z
(274.73 KiB) Downloaded 224 times
User avatar
old_death
 
Posts: 1950
Joined: 13 Jun 2009 16:19

Re: Does this actually work?

Postby old_death » 18 May 2011 17:53

While having a look at the search filters, I stumbled upon a place where theoretically banned hits may enter into the search window. I'll fix this as well.
User avatar
old_death
 
Posts: 1950
Joined: 13 Jun 2009 16:19

Re: Does this actually work?

Postby old_death » 24 May 2011 16:09

So, now all security rules are applied to the searches again (including but not limited to keyword rules for instance).

Also, depending on the usage of Shareaza, the code causes up to 20% less file reads/writes (as the security.dat file is now only saved to disk if this is really necessary and not each time you do stuff even if that does not change anything within the security manager). Also, debug output ("TRACE") has been added to view loading and saving of the security.dat file on the debug output of visual studio.

Some more "ASSERT"s have been added to assure the validity of security rules inside the code.

The "ban file" command no also blocks a file's hashes instead of only the IPs of its hits.

The mechanisms of the security manager have been further protected by migrating even more of them into private space - external edits of the rule lists or the rules should not be necessary anyway... And while doing so, a row of locks became unnecessary as the methods in question now deal with copies of the data structures of the security manager instead of modifying these data structures directly. However some more protection of the security manager is possible - which I will do eventually to completely prevent external editing of the internal security manager objects...

Also, there has been a problem with up/downgrading my security code as the generated security.dat file was not compatible. This has been fixed now and should not cause problems any more. (A switch from BOOL (old code) to bool (new code) caused problems as the first one is treated as an int when reading/writing to the file while the second one is treated as a bool (obvious) - and both types don't have the same size within the file, which caused the file content to be messed up when writing the file with one version of the security code and reading it with the other version.) Of course, while downgrading, the security rules are lost (and I can't guarantee that there is not some strange behaviour every now and then - however this cannot be avoided as the way the rules are stored has been changed for better efficiency and improved code cleanness).

Here you go with the code:
security 2011-05-24b.7z
(276.13 KiB) Downloaded 226 times


mfg,
Old

PS.: @Ryo, while working with the code, I had to revert everything to a revision previous to your project/build changes to be able to do the necessary debugging. Could you plz see that debugging the Shareaza code with VS2010 will be possible again soon without having to relay on such a trickery...?

EDIT: PPS: Some more work is in progress.
User avatar
old_death
 
Posts: 1950
Joined: 13 Jun 2009 16:19

Re: Does this actually work?

Postby old_death » 27 May 2011 21:06

This fixes some problems I had with changes to the English language (concerning the added strings etc.). Thanks to skinvista for explaining me how to.

Also, it fixes a wrong usage of the system window messaging method and replaces a wrongly used string.
security 2011-05-27.7z
Contains some minor fixes mainly to message posting...
(304.33 KiB) Downloaded 245 times
User avatar
old_death
 
Posts: 1950
Joined: 13 Jun 2009 16:19

Re: Does this actually work?

Postby old_death » 08 Jun 2011 21:33

This fixes some glitch while saving/loading rules. Also, parts of the serialize methods have been shortened to improve the coding style.
security 2011-06-08.7z
(382.5 KiB) Downloaded 237 times
User avatar
old_death
 
Posts: 1950
Joined: 13 Jun 2009 16:19

Re: Does this actually work?

Postby old_death » 23 Jul 2011 00:20

Some code cleanup + reviewed & improved locking. Extended CQuickLock by adding an Unlock() and a Relock() method.

If no further issues arise, this can be considered as final version of the code update. I therefore request for it to be committed for public testing.
security code update 2011-07-22.7z
Hopefully this is the final version of the code...
(400.34 KiB) Downloaded 251 times


mfg,
Old

PS.: Plz note I still haven't done any GUI changes, because I simply don't know how to. This means that parts of the new features cannot yet be accessed from the UI...
User avatar
old_death
 
Posts: 1950
Joined: 13 Jun 2009 16:19

Re: Does this actually work?

Postby siavoshkc » 25 Mar 2013 20:50

OldDeath, can you please update your patch with the last repo revision after a review and it would be very helpful if you use SVN patch feature to create a patch file. It would also be a good idea to put the revision number in the patch file name.
siavoshkc
 
Posts: 347
Joined: 02 Nov 2009 09:37

Re: Does this actually work?

Postby old_death » 26 Mar 2013 19:36

Voilà:
security code update 2011-07-22 (patch for r9263).7z
(51.95 KiB) Downloaded 207 times

This contains all the changes of my working repo. I've updated it to the latest revision (r9263) and resolved the handful of conflicts that occurred.

sia, plz note that I have no environment set up to compile Shareaza ATM (and will not for a few more weeks), so I cannot guarantee that the patch works as expected. I can guarantee you however that it compiled fine and worked without crashes when I last tested it.
User avatar
old_death
 
Posts: 1950
Joined: 13 Jun 2009 16:19

Re: Does this actually work?

Postby siavoshkc » 26 Mar 2013 21:49

Thank you. I didn't analyze it thoroughly but took a look at it. First we should keep the changed lines to minimum. In order to do so:
Do not change types like BOOL->bool even if you feel it is a more standard coding practice
Do not correct typos and formatting
And I have some questions:
Security.cpp line 2148 why break loop? What about other worlds?
Why we don't need delete in WndSecurity.cpp after L281?
What happened to locks in WndSecurity.cpp?

You should setup your build env ASAP and we can then chat while both having the code in front of us :-)
siavoshkc
 
Posts: 347
Joined: 02 Nov 2009 09:37

Re: Does this actually work?

Postby old_death » 26 Mar 2013 22:35

User avatar
old_death
 
Posts: 1950
Joined: 13 Jun 2009 16:19

Re: Does this actually work?

Postby raspopov » 27 Mar 2013 04:10

User avatar
raspopov
Project Admin
 
Posts: 945
Joined: 13 Jun 2009 12:30

Re: Does this actually work?

Postby old_death » 27 Mar 2013 19:11

User avatar
old_death
 
Posts: 1950
Joined: 13 Jun 2009 16:19

Re: Does this actually work?

Postby raspopov » 27 Mar 2013 19:32

I do not see how it can help you and Shareaza. You ignored suggestions for years. :?
User avatar
raspopov
Project Admin
 
Posts: 945
Joined: 13 Jun 2009 12:30

Re: Does this actually work?

Postby siavoshkc » 28 Mar 2013 09:10

siavoshkc
 
Posts: 347
Joined: 02 Nov 2009 09:37

Re: Does this actually work?

Postby lwunder » 31 Jul 2013 20:39

lwunder
 
Posts: 2
Joined: 31 Jul 2013 20:35

Re: Does this actually work?

Postby old_death » 01 Aug 2013 09:21

You might want to have a look at Quazaa. We do encourage new developers and getting started with our code base is more easy, too. ;)
User avatar
old_death
 
Posts: 1950
Joined: 13 Jun 2009 16:19

Re: Does this actually work?

Postby raspopov » 01 Aug 2013 09:49

User avatar
raspopov
Project Admin
 
Posts: 945
Joined: 13 Jun 2009 12:30

Skills

Postby lwunder » 02 Aug 2013 20:09

lwunder
 
Posts: 2
Joined: 31 Jul 2013 20:35

Previous

Return to Code Submission

Who is online

Users browsing this forum: No registered users and 1 guest

cron