qpid-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Cliff Jansen" <cliffjan...@gmail.com>
Subject Re: Review Request: patches for mingw
Date Tue, 07 Aug 2012 17:06:59 GMT


> On Aug. 7, 2012, 1:37 p.m., Kenneth Giusti wrote:
> > http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/include/proton/driver.h,
line 192
> > <https://reviews.apache.org/r/6302/diff/1-2/?file=132726#file132726line192>
> >
> >     Minor "artistic flourish" - how about we change this api to substitute "socket"
for "fd"?   I think that makes it's intent clearer.  E.g:
> >     
> >     "pn_connector_socket( pn_driver_t *driver, pn_socket_t sock, void *context)"
> >     
> >     Just wonderin'....
> 
> Rafael Schloming wrote:
>     Sorry if I'm missing some context here, still catching up on the details, but wouldn't
that simply shift the inaccuracy from windows to unix, i.e. it would be called _socket_, but
fd's would also work? Or are you suggesting a separate API for windows and it would remain
the same on unix?
> 
> Kenneth Giusti wrote:
>     I'm not recommending separate windows/unix api, but the type "pn_socket_t" seems
likely to imply the intent of that parameter, no?   And to a windows programmer, a file descriptor
(fd) wouldn't be the correct argument.
>     
>     Would it be more appropriate to typedef the parameter as pn_network_handle_t, or
a more generic pn_io_handle_t?
> 
> Rafael Schloming wrote:
>     I think my question was more whether we're trying to shoehorn two concepts into one
by having one type that is both the generic notion of fd on unix, but is restricted to a socket
on windows. It seems like we have the following options:
>     
>       (1) restrict the unix usage to socket only (in which case the naming you suggest
is appropriate)
>       (2) allow generic fds on unix and sockets only on windows and be vague/fuzzy about
the naming
>       (3) split this portion of API according to platform, e.g. have a windows include
and a unix include
>       (4) introduce a more sophisticated facade layer akin to OpenSSL's BIOs that could
represent both socket and non socket sources of I/O on any platform
>     
>     We also have the option to omit this portion of the API for now as I don't think
it's currently used, and defer the decision till later.

Is the API intended to work on non-sockets, such as pipes?  In that case a Windows HANDLE
would be the common denominator.  Windows SOCKETs are essentially OS HANDLEs from our perspective.
 The overlapped I/O implementation in the C++ code should be compatible with this usage. 
I would second pn_io_handle_t.


- Cliff


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6302/#review9957
-----------------------------------------------------------


On Aug. 6, 2012, 8:16 p.m., Cliff Jansen wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/6302/
> -----------------------------------------------------------
> 
> (Updated Aug. 6, 2012, 8:16 p.m.)
> 
> 
> Review request for qpid, Andrew Stitcher, Kenneth Giusti, Steve Huston, and Rafael Schloming.
> 
> 
> Description
> -------
> 
> This patch set works with a recent mingw32, cmake 2.8.1, python 2.5, swig 2.0.7.
> 
> A push-button build is still a ways off.  The custom_commands in the cmake script to
generate the protocol headers don't work yet on Windows.
> 
> The most intrusive change was the introduction of a pn_socket_t type to hold a socket
on both Windows and Posix platforms.  An attempt was made to minimize the use of #ifdefs and
split platform code into separate posix and windows directories, as is done for the C++ code.
 There is so little needed at the moment, this may be overkill.  The qpid-proton-posix library
was ditched and combined with the main qpid-proton library.  Instead, the work is done in
CMake to assemble the correct shared and platform specific sources as is done in the C++ tree.
> 
> The driver.c implementation is proof of concept using Winsock select().  Future work
would most likely replace this with an I/O completion port implementation.
> 
> 
> 1. mkdir ...\trunk\proton-c\build
> 
> 2. set env vars as per trunk\config.sh
> 
> 
>   set PATH=C:\Program Files (x86)\CMake 2.8\bin;C:\python25;C:\mingw_ptn\bin;C:\Windows\System32;c:\cj\work\amqp\proton\mingw4\trunk\proton-c\build
>   set PYTHONPATH=c:\cj\work\amqp\proton\mingw4\trunk\tests;c:\cj\work\amqp\proton\mingw4\trunk\proton-c;c:\cj\work\amqp\proton\mingw4\trunk\proton-c\build\bindings\python
>   set PYTHON_BINDINGS=c:\cj\work\amqp\proton\mingw4\trunk\proton-c\build\bindings\python
>   set PROTON_HOME=C:\cj\work\amqp\proton\mingw4\trunk
> 
> 3. generate the headers:
> 
>   cd trunk\proton-c\build
>   python c:\cj\work\amqp\proton\mingw4\trunk\proton-c\src\codec\encodings.h.py >encodings.h
>   python c:\cj\work\amqp\proton\mingw4\trunk\proton-c\src\protocol.h.py >protocol.h
> 
> 4. build
> 
>   cmake -G "MinGW Makefiles" -DSWIG_EXECUTABLE=C:\swigwin-2.0.7\swig.exe c:\cj\work\amqp\proton\mingw4\trunk\proton-c
>   mingw32-make
>   python ..\..\tests\proton-test
> 
> 
> This addresses bug QPID-4181.
>     https://issues.apache.org/jira/browse/QPID-4181
> 
> 
> Diffs
> -----
> 
>   http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/CMakeLists.txt 1369190 
>   http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/bindings/CMakeLists.txt
1369190 
>   http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/bindings/php/php.i 1369190

>   http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/bindings/python/python.i
1369190 
>   http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/include/proton/driver.h
1369190 
>   http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/codec/codec.c 1369190

>   http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/dispatcher/dispatcher.c
1369190 
>   http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/driver.c 1369190 
>   http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/framing/framing.c 1369190

>   http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/message/message.c 1369190

>   http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/messenger.c 1369190

>   http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/proton-dump.c 1369190

>   http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/proton.c 1369190 
>   http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/sys/network.h PRE-CREATION

>   http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/sys/posix/time.c PRE-CREATION

>   http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/sys/uuid.h PRE-CREATION

>   http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/sys/windows/driver.c
PRE-CREATION 
>   http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/sys/windows/time.c PRE-CREATION

>   http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/sys/windows/uuid.c PRE-CREATION

>   http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/util.h 1369190 
>   http://svn.apache.org/repos/asf/qpid/proton/trunk/proton-c/src/util.c 1369190 
>   http://svn.apache.org/repos/asf/qpid/proton/trunk/tests/proton_tests/messenger.py 1369190

> 
> Diff: https://reviews.apache.org/r/6302/diff/
> 
> 
> Testing
> -------
> 
> Fedora, Windows
> 
> 
> Thanks,
> 
> Cliff Jansen
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message