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
