Hi Xiaoyu, That's a huge improvement already, thanks again for all the work!
I'm a bit short on time at the moment, I hope to get around fully reading and commenting on your code during the next week. All the best, Bert On Mon, Jan 18, 2016 at 4:50 PM, Huang Xiaoyu <[email protected]> wrote: > Hi Bert, > > I’ve updated my code at > https://github.com/007pig/plugin-UPnP2 > > When you have time, can you please have another look at my code? > > Thanks a lot, > Xiaoyu > >> On Jan 8, 2016, at 6:29 PM, Bert Massop <[email protected]> wrote: >> >> Hi Xiaoyu, >> >> Thanks for all your work so far, I'm really happy to see a future >> without the current CyberGarage code! >> >> I'm not sure how to proceed with code review, as there obviously is no >> related pull request and Github does not allow comments on trees as a >> whole. From quickly looking through your code, there are a few general >> things I'd like to see changed (or clarified): >> 1. Logging, as has already been pointed out. >> 2. I'm not sure what the intended purpose of ClingTest is, but it >> doesn't seem to be a part of the plugin and it should IMHO be removed. >> 3. UPnP2 follows a "god class" anti-pattern. Please don't place all >> unrelated functionality in one single class; plugin-related code >> (runPlugin, terminate, version stuff, possibly l10n in the future) >> should wherever possible be separate from fred-related code (keeping >> track of the ports to forward) and UPnP-related code (actually >> forwarding the ports). >> 4. Ensure your locking/synchronization is correct. One obvious failure >> here is connectionServices, which is accessed by multiple threads >> without synchronization. >> 5. Using int[] for returning the bandwidths from getRates() is >> error-prone, use an object instead (compare line 330 and line 332 of >> UPnP2.java for an example of it already going wrong) >> 6. Don't just swallow InterruptedExceptions, those are usually thrown >> for a reason (of actually being interrupted due to shutting down, that >> is, although they may happen spuriously). Neither log them to standard >> error by using printStackTrace(). >> 7. It looks like the portMappingRunnable will keep being scheduled >> when the plugin has been terminated. For this reason plugins usually >> maintain a flag that indicated whether they have been terminated (or >> its inverse, that it is still running), that all their threads or jobs >> will check periodically (or on interruption if that's relevant). >> >> I hope the above will provide some guidance on where and how to >> improve your work. >> >> Kind regards, >> Bert >> >> On Fri, Dec 25, 2015 at 5:14 PM, Xiaoyu Huang <[email protected] >> <mailto:[email protected]>> wrote: >>> Hi All, >>> >>> As nextgens suggests, I write this plugin which is based on the newer and >>> more stable UPnP lib named Cling (http://4thline.org/projects/cling/). >>> >>> Currently It only supports FredPluginIPDetector and FredPluginPortForward. >>> I will add FredPluginBandwidthIndicator and Web UI support when I have time. >>> >>> Code review, suggestions and tests are required. The source code can be >>> accessed here: >>> >>> https://github.com/007pig/plugin-UPnP2 >>> >>> I uses Gradle as the build system. So to build the plugin and package it as >>> plugin jar, you need: >>> >>> 1. Copy freenet.jar, freenet-ext.jar and bcprov-jdk15on-152.jar to >>> "[project root]/libs" dir. >>> 2. Run "./gradlew build shadowjar". >>> 3. After build, you can find the plugin in "[project root]/build/libs" >>> with file name "plugin-UPnP2-1.0-SNAPSHOT-all.jar". >>> >>> Thanks, >>> Xiaoyu >>> _______________________________________________ >>> Devl mailing list >>> [email protected] <mailto:[email protected]> >>> https://emu.freenetproject.org/cgi-bin/mailman/listinfo/devl >>> <https://emu.freenetproject.org/cgi-bin/mailman/listinfo/devl> >> _______________________________________________ >> Devl mailing list >> [email protected] <mailto:[email protected]> >> https://emu.freenetproject.org/cgi-bin/mailman/listinfo/devl >> <https://emu.freenetproject.org/cgi-bin/mailman/listinfo/devl> > _______________________________________________ > Devl mailing list > [email protected] > https://emu.freenetproject.org/cgi-bin/mailman/listinfo/devl _______________________________________________ Devl mailing list [email protected] https://emu.freenetproject.org/cgi-bin/mailman/listinfo/devl
