Re: Built XWin on mingw - with patches

[Ryan Pavlik <rpavlik@iastate.edu>]

On Mon, Nov 7, 2011 at 12:10 PM, Jon TURNEY <jon.turney@dronecode.org.uk> wrote:
> Sorry, I should have mentioned this before, but please can you add a
> 'Signed-off-by' line to these (git commit --amend --signoff will add one for
> you)
>
> A few comments, I'll take a deeper look later:
>
> 0001-os-osinit.c-Exclude-new-signal-sigaction-code-on-non.patch
>
> Shouldn't this be X_NOT_POSIX rather than X_NO_POSIX?

Good catch, thanks!

>
> 0006-hw-xwin-Makefile.am-Include-manifest-in-the-dist-tar.patch
>
> Good catch! :-)
>

Yeah, notices this when trying to build for tarballs.

> 0009-os-utils.c-Use-winxp-or-better-for-Winsock-API.patch
>
> I am a bit unclear why this is needed, surely the winsock API predates XP?
> It might be better to add this define to CFLAGS rather than to start
> sprinkling it around source files as needed?
>

Yes, but one of the calls in that file uses a part of the winsock API
introduced in XP - getaddrinfo and freeaddrinfo.
http://cygwin.com/cgi-bin/cvsweb.cgi/src/winsup/w32api/include/ws2tcpip.h?rev=1.12&content-type=text/x-cvsweb-markup&cvsroot=src

> 0013-hw-xwin-InitOutput.c-Remove-duplicated-code-for-sett.patch
>
> Comment should probably say 'Consolidate duplicate code' rather than
> 'Remove'
>
> It seems this changes more than that, though, as it now looks for the files
> in both PROJECTROOT and basedir?
>
> 0017-dix-registry.c-non-cygwin-find-protocol.txt-in-reloc.patch
>
> I think the answer to the question 'Should this actually be checking
> RELOCATE_PROJECTROOT ?' is yes
>

The catch here is that RELOCATE_PROJECTROOT is currently (added by me,
since it wasn't in any header except the unused autogenerated one) in
xwin-config.h. Would it be appropriate to move it to dix-config.h for
this purpose?

> I think it would probably be neater to do something like arrange for
> FILENAME to start with the platform-appropriate path separator, rather than
> to define FILENAME_ONLY as the same name without an initial path separator?
>

The difference is not just the path separator - the FILENAME also
includes the macro define (string literal) of the install location,
which is concatenated with the filename by the preprocessor.

> 0027-dix-registry.c-Free-old-memory-upon-realloc-failure.patch
>
> Interesting.
>
> It would probably be useful to quote the language from the appropriate
> standard which describes the behavior of realloc() in this error case in the
> comment.
>
> I don't think this change is fully correct however.  If the realloc'ed size
> is 0, realloc() may return NULL, but the previously allocated memory has
> been freed.  Perhaps you need to check if errno has been set by realloc to
> distinguish these two cases?
>
> Did you notice this by inspection or actually have a problem caused by this
> code? Have you audited the rest of the xserver code for this class of error?
>

Good point. I found this with cppcheck - a static analysis tool that,
despite its name, is useful for C code as well. There were other
issues it mentioned in the xserver code, but I didn't get to any of
the others yet. In any case, it's a completely orthogonal patch. Might
be useful for someone more familiar with the code to run cppcheck and
address the issues.

> 0041-configure.ac-mingw-doesn-t-have-setuid-either.patch
>
> Use whitespace consistently with the context, please

Oops - will correct.

>
> --
> Jon TURNEY
> Volunteer Cygwin/X X Server maintainer
>



-- 
Ryan Pavlik
HCI Graduate Student
Virtual Reality Applications Center
Iowa State University

rpavlik@iastate.edu
http://academic.cleardefinition.com

--
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple
Problem reports:       http://cygwin.com/problems.html
Documentation:         http://x.cygwin.com/docs/
FAQ:                   http://x.cygwin.com/docs/faq/