Hello,
On Thu, Jan 20, 2011 at 3:05 AM, Mads Kiilerich <m...@kiilerich.com> wrote:
> Thanks for the work.
>
> I would however like to see the changes reworked a bit.
Ok, I try to restructure the series (with regards to the comments) and
post them again.
> Try to put the simple fixes first in the series - such as the "unsigned"
> changes ... if they matter at all?
IIIRC they dont matter. But "unsigned" was better than only "int", for an index.
The windows compiler complains at some places about an
"unsigned/signed" mismatch, this was only one place where I actually fixed
it.
> 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?
The config.h is only necessary for the PLUGIN_PATH and the
version number. Both of it have not really good defaults (see below).
It is possible to add a config.h, of course.
> x. introduce other stuff in os.h as it gets used ... but hardcoding an
> absolute PLUGIN_PATH is wrong
The Linux version generates a plugin search path during configure.
configure.ac:PLUGIN_PATH='${libdir}/freerdp'
configure.ac:AC_SUBST(PLUGIN_PATH)
I tried in the windows version to have something like ".", which would
be a good default,
but that somehow didnt work, but I will check that.
Otherwise the only solution would be to let it undefined and let the compiler
throw an error, thus the user needs to define it for itsself.
> 3. introduce unicode/ansi functionality and start using it - but the TODOs
> and leaks should be fixed first
There is only one leak I know of with regards to these functions. The patch
in wfreerdp.c where it parses the unicode command line and forwards the data
to the plugin as ansi. I think these ansi pointer can be freed after
freerdp_chanman_load_plugin, but in linux its the char** argv and thus
never gets freed.
A free on these strings then could be dangerous.(it would break the
behaviour between
linux/windows)
The other one frees the malloc (in devman.c).
Its only that these functions produce a possible memory leak, if the
caller does not take care of it. Thats how the functions work, they
allocate an ansi or unicode version of
the given unicode/ansi string.
>
> 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.
Maybe a libfreerdp_utils? Yes. When I did the patches i saw some places, where
the windows port needs to add implementations for synchronisation
(WaitForMultipleObjects etc..)
Another example would be a native windows implementation for the disk
redirection.
This needs similar behaviour like fdset,fdzero and the event mechanism
is based on
select (and the header is an "int" for a filedescriptor) I wrote an abstraction
for the fd_xyz function etc in rdpdr_main, but removed it later,
because it was not
necessary. These abstractions can be put in such a library.
>
> 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.
Currently the abstraction based on "define" for semaphores,threads,mutexes
and dlopen works fine with linux/windows but I dont know what is
needed for OS/X.
>
> "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 have tested the "winport" version with linux and it had no obvious errors.
Of course the current patch series may break the linux build at some steps,
but applying them all worked on linux too.
regards,
Martin
------------------------------------------------------------------------------
Special Offer-- Download ArcSight Logger for FREE (a $49 USD value)!
Finally, a world-class log management solution at an even better price-free!
Download using promo code Free_Logger_4_Dev2Dev. Offer expires
February 28th, so secure your free ArcSight Logger TODAY!
http://p.sf.net/sfu/arcsight-sfd2d
_______________________________________________
Freerdp-devel mailing list
Freerdp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/freerdp-devel
