Thanks for the work. I would however like to see the changes reworked a bit. I think it is hard to follow the changes and hard to give structured feedback. So the primary feedback is that you should try to make it more structured. It would be much easier to review if was a chain of individual "perfect" steps that each reached a goal or made a "full" step towards it - and if the changes and the reached goal was described in the commit message.
I am not a git user, but I know there are ways to refine patch series. For a starter, some changes I think would be nice to see: Try to put the simple fixes first in the series - such as the "unsigned" changes ... if they matter at all? The first change, "fix compiler warnings" - I think that is better described as "fix MSC compile time warning for missing configuration". And if some poor guy using another compiler should deserve the warning then it would be better to try to issue one of the warnings (which probably would fail) than to silently not issue any warnings. config.h ... wouldn't it be simpler and better to create and commit a config.h for windows and set the include path so VS finds it, rather than making its inclusion conditional? Regarding includes from ../common: On Unix this is added to the include path. We should try to do it the same way on all platforms. The "os abstraction" ... there is a lot of changes that are hard to follow. I suggest reworking it for example as: 1. introduce the library and start it by moving the stuff from libfreerdpchanman.c without other changes and start using it in libfreerdpchanman. 2. convert chan_plugin.c x. introduce other stuff in os.h as it gets used ... but hardcoding an absolute PLUGIN_PATH is wrong 3. introduce unicode/ansi functionality and start using it - but the TODOs and leaks should be fixed first I see more and more need for a new shared library that can be used both by libfreerdp and the plugins. That would be the natural home for this os abstraction. Some stream and debugging and iconv stuff could be nice to have there too. See also the stale branch on http://freerdp.git.sourceforge.net/git/gitweb.cgi?p=freerdp/freerdp.git;a=commitdiff;h=79346354ca7b62acde1ea1d7def5231038110910 - it shows what could be needed to compile on OS/X. I don't know if that could influence the design of the os abstraction. "add vcproj file for libcommon" and "add libcommon to freerdp solution" should be one commit. But I'm not sure we want to add this as a separate library. Perhaps it could be moved to the new common shared library? All the "compile fixes" should either get a better description that justifies they are separate, or they should be merged into one changeset. Is it possible for you to test/fix your changes on unix too? I'm not sure others agree with all my comments, but I'm sure you will get more feedback when it is more clear what the changes does. By the way: Note that a major rework of the build system might be coming soon, changing it to be cmake based and cross platform. I am sitting on some changes (similar to yours) for compiling for windows with gcc, but am waiting for the build system changes. /Mads Martin Vogt wrote, On 01/15/2011 06:34 PM: > Hello, > > I have some patches which enables channel/librdpdr support for windows. > > Attached are the patches for review, but only for the > freerdp code, and not for the Visual Studio .vcproj project files, > they are only in the repository. > > > The repository is here: > > http://gitorious.org/freerdp-smartcard/winport > > I have never used "git" thus most likely I made some stupid mistakes. > > But I could pull the patches into a clone of freerdp master with > the command: > > git pull g...@gitorious.org:freerdp-smartcard/winport.git winport > > Is it possible to apply these? > > regards, > > Martin > > channels/common/chan_plugin.c | 34 +++-- > channels/common/chan_stream.c | 2 + > channels/common/wait_obj.c | 2 +- > channels/common/wait_obj.h | 2 +- > channels/common/wait_obj_win.c | 164 ++++++++++++++++++++++ > channels/rdpdr/devman.c | 37 +++++- > channels/rdpdr/devman.h | 2 +- > channels/rdpdr/irp_queue.c | 3 +- > channels/rdpdr/rdpdr_capabilities.c | 4 + > channels/rdpdr/rdpdr_constants.h | 4 + > channels/rdpdr/rdpdr_main.c | 61 +++++---- > channels/rdpdr/rdpdr_main.h | 8 +- > channels/rdpdr/rdpdr_types.h | 4 +- > include/freerdp/os/os.h | 111 +++++++++++++++ > include/freerdp/types_ui.h | 4 +- > libfreerdp/os/os_win.c | 98 ++++++++++++++ > libfreerdp/stream.h | 6 + > libfreerdp/tcp.c | 2 +- > libfreerdpchanman/libfreerdpchanman.c | 44 +------ > win/freerdp.sln | 16 +++ > win/libcommon.vcproj | 202 ++++++++++++++++++++++++++++ > win/libfreerdp.vcproj | 8 + > win/librdpdr.def | 10 ++ > win/librdpdr.vcproj | 239 > +++++++++++++++++++++++++++++++++ > win/wfreerdp/wf_event.h | 1 - > win/wfreerdp/wf_win.cpp | 3 +- > win/wfreerdp/wf_win.h | 1 - > win/wfreerdp/wfreerdp.cpp | 51 +++++++- > 28 files changed, 1012 insertions(+), 111 deletions(-) > > > ------------------------------------------------------------------------------ > Protect Your Site and Customers from Malware Attacks > Learn about various malware tactics and how to avoid them. Understand > malware threats, the impact they can have on your business, and how you > can protect your company and customers by using code signing. > http://p.sf.net/sfu/oracle-sfdevnl > > > _______________________________________________ > Freerdp-devel mailing list > Freerdp-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/freerdp-devel ------------------------------------------------------------------------------ Protect Your Site and Customers from Malware Attacks Learn about various malware tactics and how to avoid them. Understand malware threats, the impact they can have on your business, and how you can protect your company and customers by using code signing. http://p.sf.net/sfu/oracle-sfdevnl _______________________________________________ Freerdp-devel mailing list Freerdp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/freerdp-devel
