Mark Smith wrote:
> Rich Megginson wrote:
>> The definition of LBER_SOCKET in lber.h:
>> #ifndef macintosh
>> #if defined( _WINDOWS ) || defined( _WIN32) || defined( _CONSOLE )
>> #include <winsock.h> /* for SOCKET */
>> typedef SOCKET LBER_SOCKET;
>> #else
>> typedef long LBER_SOCKET;
>> #endif /* _WINDOWS */
>> #else /* macintosh */
>> typedef void *LBER_SOCKET;
>> #endif /* macintosh */
>>
>> (as an aside, what is a socket/file descriptor on mac? Why a void *?)
>
> A void * is used on Mac OS for historical reasons. In the old days with
> Mac OS classic, the TCP stack was not so standardized and most
> programmers had their own layer on top of either MacTCP or
> OpenTransport... and in many cases a pointer was used (hence the void *
> to cover that case).
>
> A long is used on the Unix-y platforms for historical reasons. Probably
> something about platforms where integers are 16 bits (I hope none of
> those platforms matter much anymore). But the idea of defining a
> typedef for LBER_SOCKET in the first place was so most of the code would
> not need to know what type it was....
>
>> This should correspond to the native socket/file descriptor type on
>> the platform, and is used as such (lber-int.h):
>>
>> struct sockbuf {
>> LBER_SOCKET sb_sd;
>> ...
>> LBER_SOCKET sb_copyfd; /* for LBER_SOCKBUF_OPT_TO_FILE* opts */
>> ...
>>
>> On every unix/linux platform, the native socket/file descriptor type
>> is a 32bit int. So, I propose we change LBER_SOCKET to int.
>
> That sounds OK to me, although I worry the change may cause problems for
> some people (probably not major ones though). I assume Mac OS X uses an
> int. What about Win32 and Win64? Or are you proposing to change to int
> only for the Unix-y platforms?
Only for *nix platforms. Windows is completely different, and I just
don't know what it is on Mac, so I guess we could leave that alone as
well. Ideally, there would be a way to just figure out what the
socket/file descriptor type is on the platform and just use that type.
I'll note that OpenLDAP defines LBER_SOCKET_T int on all platforms, even
on platforms with 16 bit ints. I'm not sure how this works on Windows
or Mac. Perhaps the Windows SOCKET type is the same size as int, and so
is whatever is used on Mac. Not sure about 64bit Windows, but probably
SOCKET is 32bits there too.
>
>
>> In addition, we don't use the type LBER_SOCKET consistently. There
>> are a number of places where we use the value of sb_sd such as the
>> following (liblber/io.c):
>> if ( sb->sb_ext_io_fns.lbextiofn_read != NULL ) {
>> rc = sb->sb_ext_io_fns.lbextiofn_read(
>> sb->sb_sd, sb->sb_ber.ber_buf,
>> ((sb->sb_options & LBER_SOCKBUF_OPT_NO_READ_AHEAD)
>> && (len < READBUFSIZ)) ? len : READBUFSIZ,
>> sb->sb_ext_io_fns.lbextiofn_socket_arg );
>>
>> However, the definition of sb->sb_ext_io_fns.lbextiofn_read() is this:
>> typedef int (LDAP_C LDAP_CALLBACK LDAP_X_EXTIOF_READ_CALLBACK)( int s,
>> void *buf, int bufsize, struct lextiof_socket_private *socketarg );
>>
>> The first argument should really be LBER_SOCKET s instead of int s.
>
> Yes, although there was some reason we made it an int. I cannot
> remember the details right now though.
I think it has to be LBER_SOCKET instead of int, because that is the
value passed in as the first argument.
>
>
>> Finally, there are a few places where we should use casts to make
>> clear what is going on e.g. ldappr-public.c:
>> int LDAP_CALL
>> prldap_import_connection (LDAP *ld)
>> {
>> int rc = LDAP_SUCCESS; /* optimistic */
>> int shared = 1; /* Assume shared init */
>> LBER_SOCKET orig_socket = -1;
>> PRLDAPIOSessionArg *prsessp = NULL;
>> PRLDAPIOSocketArg *prsockp = NULL;
>> PRFileDesc *pr_socket = NULL;
>> ...
>> /* Retrieve TCP socket's integer file descriptor */
>> if ( ldap_get_option( ld, LDAP_OPT_DESC, &orig_socket ) < 0 ) {
>> ...
>> /* Import file descriptor of connection made via ldap_init() */
>> if (NULL == (pr_socket = PR_ImportTCPSocket(orig_socket)) ) {
>> ldap_set_lderrno( ld, LDAP_LOCAL_ERROR, NULL, NULL );
>>
>> The definition of PR_ImportTCPSocket() is this:
>> NSPR_API(PRFileDesc*) PR_ImportTCPSocket(PRInt32 osfd);
>>
>> I suppose we don't need the cast (PRInt32)orig_socket if we change
>> LBER_SOCKET to int, but it is of a different type nonetheless.
>
> The only danger in adding casts is if LBER_SOCKET was defined as some
> non-integer compatible type the compiler will catch it right now. With
> the casts added it may not.
>
Whatever type LBER_SOCKET is, it must be compatible with the type
expected by PR_ImportTCPSocket() and other functions that take a
LBER_SOCKET argument. At least the cast would force it to be the right
size (however data truncation may occur).
_______________________________________________
dev-tech-ldap mailing list
[email protected]
https://lists.mozilla.org/listinfo/dev-tech-ldap
