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]> 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] > https://emu.freenetproject.org/cgi-bin/mailman/listinfo/devl _______________________________________________ Devl mailing list [email protected] https://emu.freenetproject.org/cgi-bin/mailman/listinfo/devl
