You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by Manuel Teira Paz <mt...@tid.es> on 2009/03/02 17:07:21 UTC
Re: About socklen_t
Steve Huston escribió:
>> -----Original Message-----
>> From: Alan Conway [mailto:aconway@redhat.com]
>>
>> Manuel Teira Paz wrote:
>>
>>> Alan Conway escribió:
>>>
>>>> Manuel Teira Paz wrote:
>>>>
>>>>
>>>>> Hello again.
>>>>>
>>>>> During the compilation stage of the trunk, I run into this
>>>>>
> error:
>
>>>>> source='qpid/broker/Bridge.cpp' object='qpid/broker/Bridge.lo'
>>>>> libtool=yes \
>>>>> DEPDIR=.deps depmode=dashXmstdout /bin/bash ../build-aux/depcomp
>>>>>
> \
>
>>>>> /bin/bash ../libtool --tag=CXX --mode=compile CC -g -m64
>>>>> -DHAVE_CONFIG_H -I. -Igen -I./gen -I/opt/qpid/include
>>>>> -library=stlport4 -mt -c -o qpid/broker/Bridge.lo
>>>>>
>> qpid/broker/Bridge.cpp
>>
>>>>> CC -g -m64 -DHAVE_CONFIG_H -I. -Igen -I./gen -I/opt/qpid/include
>>>>>
>
>
>>>>> -library=stlport4 -mt -c qpid/broker/Bridge.cpp -KPIC -DPIC -o
>>>>> qpid/broker/.libs/Bridge.o
>>>>> "./qpid/sys/Socket.h", line 96: Error: socklen_t is not defined.
>>>>> "qpid/broker/Bridge.cpp", line 170: Warning: id hides
>>>>> qpid::broker::Bridge::id.
>>>>> 1 Error(s) and 1 Warning(s) detected.
>>>>>
>>>>> I started looking in my Solaris system, where socklen_t
>>>>>
>> is defined,
>>
>>>>> and the place was sys/socket.h. Nevertheless, I see that the way
>>>>>
>
>
>>>>> integer types are intended to be imported now regardless
>>>>>
>> the platform
>>
>>>>> is:
>>>>> qpid/sys/IntegerTypes.h
>>>>>
>>>>> that relies on a platform dependent file.
>>>>>
>>>>> The question here is, where exactly the socklen_t is
>>>>>
>> suposed to be
>>
>>>>> defined in linux or a posix compliant system? The posix
>>>>> IntegerTypes.h in qpid is just including <stdint.h>, but
>>>>>
>> after a fast
>>
>>>>> look in an Ubuntu system, I didn't find there any declaration
>>>>>
> for
>
>>>>> socklen_t. Another curious thing about socklen_t is what
>>>>>
>> I've found
>>
>>>>> here:
>>>>>
>>>>>
>>>>>
>> http://portabilityblog.com/blog/archives/7-socklen_t-confusion.html
>>
>>>>> Also, the declaration for solaris is not trivial:
>>>>>
>>>>>
>>>>> #if defined(_XPG4_2) && !defined(_XPG5) && !defined(_LP64)
>>>>> typedef size_t socklen_t;
>>>>> #else
>>>>> typedef uint32_t socklen_t;
>>>>> #endif /* defined(_XPG4_2) && !defined(_XPG5) &&
>>>>>
>> !defined(_LP64) */
>>
>>>>> Anyway, it seems to rely to uint32_t on normal cases.
>>>>>
>>>>>
>>>>> What do you think is the best way to proceed here? Just add a
>>>>> #include <sys/socket.h> in the posix IntegerTypes.h?
>>>>>
>>>> I would only #include socket.h in network-related files that need
>>>>
>
>
>>>> socklen_t, it's not really a generic integer type it's specific
>>>>
> to
>
>>>> sockets. Where is it being used that we're not already including
>>>> socket.h?
>>>>
>>>>
>>>>
>>> In qpid/sys/Socket.h.
>>>
>>> Furthermore, socklen_t is defined for the windows platform in
>>>
> their
>
>>> specific IntegerTypes.h, that was what led me to think that
>>>
> perhaps
>
>>> including socket.h in the posix counterpart, could be a (somewhat)
>>>
>
>
>>> coherent move.
>>>
>>> Another posibility would be to include sys/socket.h in
>>> qpid/sys/Socket.h. But since this is not posix specific,
>>>
>> perhaps the
>>
>>> windows port could get damaged.
>>>
>> That would be my preference, check with Steve if it's OK on
>> windows. I'm fairly
>> sure it is. If not you could add a sys/socklen.h with an
>> #ifdef to include the
>> appropriate file and include that in Socket.h.
>>
>
> Adding a #include <sys/socket.h> would require a platform-specific
> ifdef, which I thought were highly frowned on. The approach taken in
> similar situations is to add a platform-specific header file
> (qpid/sys/posix/sys_socket.h or similar) and include that from
> qpid/sys/Socket.h.
>
That's true. I was just hoping that perhaps a sys/socket.h also existed
on windows. Unfortunately, that's not the case.
But, if I'm not missunderstanding you: your proposal is to move the
Socket definitions to a Socket.h specific file for Posix and Windows,
and doing the decission of what to include in the upper level Socket.h
file? isn't that leading to a lot of duplicated code? Actually, the
windows and posix specific Socket definitions are only going to differ
in one line. I know that this is the usual way to proceed, but is it
used in extreme cases like this, where both models just differ in the
inclusion of a header?
Or did I miss something?
Regards.
--
Manuel
> I put the socklen_t typedef for Windows in IntegerTypes.h because
> Windows doesn't define socklen_t anywhere.
>
> -Steve
>
>
> ---------------------------------------------------------------------
> Apache Qpid - AMQP Messaging Implementation
> Project: http://qpid.apache.org
> Use/Interact: mailto:dev-subscribe@qpid.apache.org
>
>
---------------------------------------------------------------------
Apache Qpid - AMQP Messaging Implementation
Project: http://qpid.apache.org
Use/Interact: mailto:dev-subscribe@qpid.apache.org
RE: RFC (Was About socklen_t )
Posted by Andrew Stitcher <as...@redhat.com>.
On Tue, 2009-03-03 at 13:06 -0500, Andrew Stitcher wrote:
> On Tue, 2009-03-03 at 12:10 -0500, Steve Huston wrote:
> > I also think this is good, for a different reason... Taking Manuel's
> > case that sometimes socklen_t is 32 bits, sometimes 64, it may help
> to
> > avoid wierd run-time errors when a 32-bit app links to a qpid lib
> > built with 64-bit socklen_t. That couldn't happen with socklen_t's
> > current usage buried away from the public API as it is, but could it
> > be an issue if the type appeared closer to the user?
>
> As I indicated in my original mail I have a hard time believing that
> even 32 bits is necessary for a socket address length so I can't
> imagine
> that casting from a uint32_t to one of size_t/socklen_t/uint64 etc.
> could actually be a real problem.
I've now looked at the cause of this whole issue and realised that we're
discussing a type that isn't actually really used in our code!
The problem is in the definition of Socket::accept() -
cpp/qpid/sys/Socket.h:96
But our code only ever calls socket::accept(0,0)! It never actually
cares about the connecting address as a result of the accept.
Even if we wanted to return the connecting address from accept the way
we should do that which is consonant with the rest of the Socket class
is as a string address/port (I think this is yucky myself). In any event
we know we can can only get a TCP/TCP6 address out of here cause we
accepted on a TCP/TCP6 socket.
But given that we don't currently use the returned address I'm just
going to remove it from the API.
To leave the signature Socket* Socket::accept(void);
That should remove both sockaddr and socklen_t from the Socket API.
Andrew
---------------------------------------------------------------------
Apache Qpid - AMQP Messaging Implementation
Project: http://qpid.apache.org
Use/Interact: mailto:dev-subscribe@qpid.apache.org
RE: RFC (Was About socklen_t )
Posted by Andrew Stitcher <as...@redhat.com>.
On Tue, 2009-03-03 at 12:10 -0500, Steve Huston wrote:
> > > I think this is the wrong solution: If socklen_t isn't a platform
> > > portable type then it has no place being used in platform
> > > independent header
>
> Right - the question is, is socklen_t a portable type. I'd argue it
> is, and that Windows is about the only popular OS that doesn't define
> it natively. Hence, why I added it to IntegerTypes.h.
This is true, but it's only portable in the same way that POSIX is
portable ie not (necessarily) to Windows!
>
> > > - The only platform dependent code should be in
> > > subdirectories of
> > > qpid/sys, not in qpid/sys itself. So we should change the
> > type to be a
> > > different type that is platform portable and deal with the
> platform
> > > specifics in the platform layer instead (simple casting will do).
> > >
> > > I suggest we just change qpid/sys/Socket.h to use uint32_t
> > instead - in
> > > fact I will make that change unless anyone complains by the
> > end of day.
>
> I also think this is good, for a different reason... Taking Manuel's
> case that sometimes socklen_t is 32 bits, sometimes 64, it may help to
> avoid wierd run-time errors when a 32-bit app links to a qpid lib
> built with 64-bit socklen_t. That couldn't happen with socklen_t's
> current usage buried away from the public API as it is, but could it
> be an issue if the type appeared closer to the user?
As I indicated in my original mail I have a hard time believing that
even 32 bits is necessary for a socket address length so I can't imagine
that casting from a uint32_t to one of size_t/socklen_t/uint64 etc.
could actually be a real problem.
Andrew
---------------------------------------------------------------------
Apache Qpid - AMQP Messaging Implementation
Project: http://qpid.apache.org
Use/Interact: mailto:dev-subscribe@qpid.apache.org
RE: RFC (Was About socklen_t )
Posted by Steve Huston <sh...@riverace.com>.
> > I think this is the wrong solution: If socklen_t isn't a platform
> > portable type then it has no place being used in platform
> > independent header
Right - the question is, is socklen_t a portable type. I'd argue it
is, and that Windows is about the only popular OS that doesn't define
it natively. Hence, why I added it to IntegerTypes.h.
> > - The only platform dependent code should be in
> > subdirectories of
> > qpid/sys, not in qpid/sys itself. So we should change the
> type to be a
> > different type that is platform portable and deal with the
platform
> > specifics in the platform layer instead (simple casting will do).
> >
> > I suggest we just change qpid/sys/Socket.h to use uint32_t
> instead - in
> > fact I will make that change unless anyone complains by the
> end of day.
I also think this is good, for a different reason... Taking Manuel's
case that sometimes socklen_t is 32 bits, sometimes 64, it may help to
avoid wierd run-time errors when a 32-bit app links to a qpid lib
built with 64-bit socklen_t. That couldn't happen with socklen_t's
current usage buried away from the public API as it is, but could it
be an issue if the type appeared closer to the user?
-Steve
---------------------------------------------------------------------
Apache Qpid - AMQP Messaging Implementation
Project: http://qpid.apache.org
Use/Interact: mailto:dev-subscribe@qpid.apache.org
Re: RFC (Was About socklen_t )
Posted by Manuel Teira Paz <mt...@tid.es>.
Andrew Stitcher escribió:
> On Tue, 2009-03-03 at 17:10 +0100, Manuel Teira Paz wrote:
>
>> Coming back to this issue.
>>
>> I don't like the option of having a different Socket.h for windows and
>> posix, since they would only differ in one include sentence.
>>
>> On the other hand, I don't see any problem into having socklen_t defined
>> into IntegerTypes.h, since that's what it is: an integer type. Perhaps
>> including the whole <sys/socket.h> in the posix IntegerTypes.h is not
>> very polite, but, what other options do we have?
>>
>> To have a specific SocketTypes.h ? It seems a little odd, just to
>> support socklen_t. Or to define a own typedef to be used in the
>> Socket::accept method (the only one using socklen_t actually)?
>>
>> So, my +1 for adding sys/socket.h to the posix IntegerTypes.h header.
>>
>
> I think this is the wrong solution: If socklen_t isn't a platform
> portable type then it has no place being used in platform independent
> header - The only platform dependent code should be in subdirectories of
> qpid/sys, not in qpid/sys itself. So we should change the type to be a
> different type that is platform portable and deal with the platform
> specifics in the platform layer instead (simple casting will do).
>
> I suggest we just change qpid/sys/Socket.h to use uint32_t instead - in
> fact I will make that change unless anyone complains by the end of day.
> uint32_t is big enough for any currently conceivable socket address for
> any currently conceivable protocol (dons hard hat and ducks).
>
>
My fear here is that since POSIX.g seems to have defined socklen_t as
size_t (a wrong decision, anyway), a 64 bits platform can handle the
passed socklen_t pointer as something handling 8 bytes. Something like
bzero(ptr, sizeof(ptr)) when ptr is a socklen_t pointer, will lead to
overwriting past the end of our uint32_t pointer. So, perhaps casting
is not enough in this case.
Reading the socket.h solaris header, it seems that XPG5 (Unix98?)
defines socklen_t as uint32_t, but XPG4 defines it as size_t.
About the decision itself, I think it is the right one, and socklen_t
shouldn't appear in a sys top header.
Regards.
> Andrew
>
>
>
> ---------------------------------------------------------------------
> Apache Qpid - AMQP Messaging Implementation
> Project: http://qpid.apache.org
> Use/Interact: mailto:dev-subscribe@qpid.apache.org
>
>
---------------------------------------------------------------------
Apache Qpid - AMQP Messaging Implementation
Project: http://qpid.apache.org
Use/Interact: mailto:dev-subscribe@qpid.apache.org
Re: RFC (Was About socklen_t )
Posted by Andrew Stitcher <as...@redhat.com>.
On Tue, 2009-03-03 at 17:10 +0100, Manuel Teira Paz wrote:
> Coming back to this issue.
>
> I don't like the option of having a different Socket.h for windows and
> posix, since they would only differ in one include sentence.
>
> On the other hand, I don't see any problem into having socklen_t defined
> into IntegerTypes.h, since that's what it is: an integer type. Perhaps
> including the whole <sys/socket.h> in the posix IntegerTypes.h is not
> very polite, but, what other options do we have?
>
> To have a specific SocketTypes.h ? It seems a little odd, just to
> support socklen_t. Or to define a own typedef to be used in the
> Socket::accept method (the only one using socklen_t actually)?
>
> So, my +1 for adding sys/socket.h to the posix IntegerTypes.h header.
I think this is the wrong solution: If socklen_t isn't a platform
portable type then it has no place being used in platform independent
header - The only platform dependent code should be in subdirectories of
qpid/sys, not in qpid/sys itself. So we should change the type to be a
different type that is platform portable and deal with the platform
specifics in the platform layer instead (simple casting will do).
I suggest we just change qpid/sys/Socket.h to use uint32_t instead - in
fact I will make that change unless anyone complains by the end of day.
uint32_t is big enough for any currently conceivable socket address for
any currently conceivable protocol (dons hard hat and ducks).
Andrew
---------------------------------------------------------------------
Apache Qpid - AMQP Messaging Implementation
Project: http://qpid.apache.org
Use/Interact: mailto:dev-subscribe@qpid.apache.org
Re: RFC (Was About socklen_t )
Posted by Manuel Teira Paz <mt...@tid.es>.
Andrew Stitcher escribió:
> I've now fixed this issue (i think) by removing the use of socklen_t
> from the Socket class API completely.
>
> Manuel - confirm this fixes your issue.
>
Yes, it must fix it. I'm in the process of configuring a new build
machine (since the older one was lasting ages to compile qpid). I will
confirm it as soon as the new machine is ready.
> Steve - does this allow you to simplify the windows version of
> IntegerTypes.h?
>
> Andrew
>
>
>
> ---------------------------------------------------------------------
> Apache Qpid - AMQP Messaging Implementation
> Project: http://qpid.apache.org
> Use/Interact: mailto:dev-subscribe@qpid.apache.org
>
>
---------------------------------------------------------------------
Apache Qpid - AMQP Messaging Implementation
Project: http://qpid.apache.org
Use/Interact: mailto:dev-subscribe@qpid.apache.org
RE: RFC (Was About socklen_t )
Posted by Steve Huston <sh...@riverace.com>.
Hi Andrew,
> I've now fixed this issue (i think) by removing the use of socklen_t
> from the Socket class API completely.
Ok.
> Manuel - confirm this fixes your issue.
> Steve - does this allow you to simplify the windows version of
> IntegerTypes.h?
Yes, I removed the socklen_t typedef and aligned windows/Socket.cpp
with your changes to Socket.h - the Windows build is good now.
Thanks,
-Steve
---------------------------------------------------------------------
Apache Qpid - AMQP Messaging Implementation
Project: http://qpid.apache.org
Use/Interact: mailto:dev-subscribe@qpid.apache.org
Re: RFC (Was About socklen_t )
Posted by Andrew Stitcher <as...@redhat.com>.
I've now fixed this issue (i think) by removing the use of socklen_t
from the Socket class API completely.
Manuel - confirm this fixes your issue.
Steve - does this allow you to simplify the windows version of
IntegerTypes.h?
Andrew
---------------------------------------------------------------------
Apache Qpid - AMQP Messaging Implementation
Project: http://qpid.apache.org
Use/Interact: mailto:dev-subscribe@qpid.apache.org
RFC (Was About socklen_t )
Posted by Manuel Teira Paz <mt...@tid.es>.
Coming back to this issue.
I don't like the option of having a different Socket.h for windows and
posix, since they would only differ in one include sentence.
On the other hand, I don't see any problem into having socklen_t defined
into IntegerTypes.h, since that's what it is: an integer type. Perhaps
including the whole <sys/socket.h> in the posix IntegerTypes.h is not
very polite, but, what other options do we have?
To have a specific SocketTypes.h ? It seems a little odd, just to
support socklen_t. Or to define a own typedef to be used in the
Socket::accept method (the only one using socklen_t actually)?
So, my +1 for adding sys/socket.h to the posix IntegerTypes.h header.
Manuel Teira Paz escribió:
> Steve Huston escribió:
>
>>> -----Original Message-----
>>> From: Alan Conway [mailto:aconway@redhat.com]
>>>
>>> Manuel Teira Paz wrote:
>>>
>>>
>>>> Alan Conway escribió:
>>>>
>>>>
>>>>> Manuel Teira Paz wrote:
>>>>>
>>>>>
>>>>>
>>>>>> Hello again.
>>>>>>
>>>>>> During the compilation stage of the trunk, I run into this
>>>>>>
>>>>>>
>> error:
>>
>>
>>>>>> source='qpid/broker/Bridge.cpp' object='qpid/broker/Bridge.lo'
>>>>>> libtool=yes \
>>>>>> DEPDIR=.deps depmode=dashXmstdout /bin/bash ../build-aux/depcomp
>>>>>>
>>>>>>
>> \
>>
>>
>>>>>> /bin/bash ../libtool --tag=CXX --mode=compile CC -g -m64
>>>>>> -DHAVE_CONFIG_H -I. -Igen -I./gen -I/opt/qpid/include
>>>>>> -library=stlport4 -mt -c -o qpid/broker/Bridge.lo
>>>>>>
>>>>>>
>>> qpid/broker/Bridge.cpp
>>>
>>>
>>>>>> CC -g -m64 -DHAVE_CONFIG_H -I. -Igen -I./gen -I/opt/qpid/include
>>>>>>
>>>>>>
>>
>>
>>>>>> -library=stlport4 -mt -c qpid/broker/Bridge.cpp -KPIC -DPIC -o
>>>>>> qpid/broker/.libs/Bridge.o
>>>>>> "./qpid/sys/Socket.h", line 96: Error: socklen_t is not defined.
>>>>>> "qpid/broker/Bridge.cpp", line 170: Warning: id hides
>>>>>> qpid::broker::Bridge::id.
>>>>>> 1 Error(s) and 1 Warning(s) detected.
>>>>>>
>>>>>> I started looking in my Solaris system, where socklen_t
>>>>>>
>>>>>>
>>> is defined,
>>>
>>>
>>>>>> and the place was sys/socket.h. Nevertheless, I see that the way
>>>>>>
>>>>>>
>>
>>
>>>>>> integer types are intended to be imported now regardless
>>>>>>
>>>>>>
>>> the platform
>>>
>>>
>>>>>> is:
>>>>>> qpid/sys/IntegerTypes.h
>>>>>>
>>>>>> that relies on a platform dependent file.
>>>>>>
>>>>>> The question here is, where exactly the socklen_t is
>>>>>>
>>>>>>
>>> suposed to be
>>>
>>>
>>>>>> defined in linux or a posix compliant system? The posix
>>>>>> IntegerTypes.h in qpid is just including <stdint.h>, but
>>>>>>
>>>>>>
>>> after a fast
>>>
>>>
>>>>>> look in an Ubuntu system, I didn't find there any declaration
>>>>>>
>>>>>>
>> for
>>
>>
>>>>>> socklen_t. Another curious thing about socklen_t is what
>>>>>>
>>>>>>
>>> I've found
>>>
>>>
>>>>>> here:
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>> http://portabilityblog.com/blog/archives/7-socklen_t-confusion.html
>>>
>>>
>>>>>> Also, the declaration for solaris is not trivial:
>>>>>>
>>>>>>
>>>>>> #if defined(_XPG4_2) && !defined(_XPG5) && !defined(_LP64)
>>>>>> typedef size_t socklen_t;
>>>>>> #else
>>>>>> typedef uint32_t socklen_t;
>>>>>> #endif /* defined(_XPG4_2) && !defined(_XPG5) &&
>>>>>>
>>>>>>
>>> !defined(_LP64) */
>>>
>>>
>>>>>> Anyway, it seems to rely to uint32_t on normal cases.
>>>>>>
>>>>>>
>>>>>> What do you think is the best way to proceed here? Just add a
>>>>>> #include <sys/socket.h> in the posix IntegerTypes.h?
>>>>>>
>>>>>>
>>>>> I would only #include socket.h in network-related files that need
>>>>>
>>>>>
>>
>>
>>>>> socklen_t, it's not really a generic integer type it's specific
>>>>>
>>>>>
>> to
>>
>>
>>>>> sockets. Where is it being used that we're not already including
>>>>> socket.h?
>>>>>
>>>>>
>>>>>
>>>>>
>>>> In qpid/sys/Socket.h.
>>>>
>>>> Furthermore, socklen_t is defined for the windows platform in
>>>>
>>>>
>> their
>>
>>
>>>> specific IntegerTypes.h, that was what led me to think that
>>>>
>>>>
>> perhaps
>>
>>
>>>> including socket.h in the posix counterpart, could be a (somewhat)
>>>>
>>>>
>>
>>
>>>> coherent move.
>>>>
>>>> Another posibility would be to include sys/socket.h in
>>>> qpid/sys/Socket.h. But since this is not posix specific,
>>>>
>>>>
>>> perhaps the
>>>
>>>
>>>> windows port could get damaged.
>>>>
>>>>
>>> That would be my preference, check with Steve if it's OK on
>>> windows. I'm fairly
>>> sure it is. If not you could add a sys/socklen.h with an
>>> #ifdef to include the
>>> appropriate file and include that in Socket.h.
>>>
>>>
>> Adding a #include <sys/socket.h> would require a platform-specific
>> ifdef, which I thought were highly frowned on. The approach taken in
>> similar situations is to add a platform-specific header file
>> (qpid/sys/posix/sys_socket.h or similar) and include that from
>> qpid/sys/Socket.h.
>>
>>
> That's true. I was just hoping that perhaps a sys/socket.h also existed
> on windows. Unfortunately, that's not the case.
>
> But, if I'm not missunderstanding you: your proposal is to move the
> Socket definitions to a Socket.h specific file for Posix and Windows,
> and doing the decission of what to include in the upper level Socket.h
> file? isn't that leading to a lot of duplicated code? Actually, the
> windows and posix specific Socket definitions are only going to differ
> in one line. I know that this is the usual way to proceed, but is it
> used in extreme cases like this, where both models just differ in the
> inclusion of a header?
>
> Or did I miss something?
>
> Regards.
>
> --
> Manuel
>
>> I put the socklen_t typedef for Windows in IntegerTypes.h because
>> Windows doesn't define socklen_t anywhere.
>>
>> -Steve
>>
>>
>> ---------------------------------------------------------------------
>> Apache Qpid - AMQP Messaging Implementation
>> Project: http://qpid.apache.org
>> Use/Interact: mailto:dev-subscribe@qpid.apache.org
>>
>>
>>
>
>
> ---------------------------------------------------------------------
> Apache Qpid - AMQP Messaging Implementation
> Project: http://qpid.apache.org
> Use/Interact: mailto:dev-subscribe@qpid.apache.org
>
>
---------------------------------------------------------------------
Apache Qpid - AMQP Messaging Implementation
Project: http://qpid.apache.org
Use/Interact: mailto:dev-subscribe@qpid.apache.org