Pls review 7091417: recvfrom's 6th input should be of type socklen_t (M)

[Paul Hohensee]

Thank you, Coleen.

Paul

On 12/21/11 11:56 AM, Coleen Phillimore wrote:
> looks good to me!
> coleenp
>
> On 12/21/2011 9:03 AM, Paul Hohensee wrote:
>> Thank you for the thorough review.  New webrev here
>>
>> http://cr.openjdk.java.net/~phh/7091417.01/
>>
>> Any other takers?  I'd like to push this today in time for next 
>> week's promotion.
>>
>> Response inline.
>>
>> On 12/20/11 7:20 PM, David Holmes wrote:
>>> Hi Paul,
>>>
>>> A few comments below.
>>>
>>> Cheers,
>>> David
>>>
>>> On 21/12/2011 12:19 AM, Paul Hohensee wrote:
>>>> This is actually a bit broader scope than the Summary suggests, in 
>>>> that
>>>> it takes into
>>>> account the possibility that socklen_t might not be the same bit 
>>>> width as
>>>> int and jint (which latter two are assumed synonymous in the jvm). 
>>>> I spent
>>>> a bit of time with socket.h on linux/bsd/solaris to verify the actual
>>>> argument
>>>> types of the socket methods used by the jvm. I changed the os methods
>>>> so that their formal argument types match socket.h and put that 
>>>> adapter
>>>> code in jvm.cpp so as to have only a single copy of it.
>>>>
>>>> Webrev here
>>>>
>>>> http://cr.openjdk.java.net/~phh/7091417.00/
>>>
>>> jvm.cpp:
>>>
>>> I would have expected this to need explicit casts on some platforms, 
>>> else I'd expect compiler warnings if a socklen_t is not an int:
>>>
>>> 3557 JVM_LEAF(jint, JVM_Accept(jint fd, struct sockaddr *him, jint 
>>> *len))
>>> 3558   JVMWrapper2("JVM_Accept (0x%x)", fd);
>>> 3559   //%note jvm_r6
>>> 3560   socklen_t socklen = *len;
>>> 3561   jint result = os::accept(fd, him, &socklen);
>>> 3562   *len = socklen;
>>> 3563   return result;
>>> 3564 JVM_END
>>>
>>> and similarly for JVM_Recvfrom / JVM_GetSockName / JVM_GetSockOpt
>>>
>>
>> Fixed.
>>
>>> ---
>>>
>>> jvm_bsd.h:
>>>
>>> I think the include for sys/socket.h needs to be moved to where the 
>>> other includes start. It seems reasonable to me that its inclusion 
>>> may be affected by the STDC macro stuff.
>>
>> Moved down to just after the include of sys/param.h.  Ditto in 
>> jvm_solaris/
>> linux/windows.h  All the jvm_<os>.h files seem to be kludges, but fixing
>> that is something for another time.
>>
>>>
>>> ---
>>>
>>> os_bsd.inline.hpp
>>>
>>> I was wondering about the issue of the flags being int or uint. The 
>>> linux manpage for recv:
>>>
>>> http://linux.die.net/man/2/recv
>>>
>>> sheds some light:
>>>
>>> "The prototypes given above follow glibc2. The Single UNIX 
>>> Specification agrees, except that it has return values of type 
>>> ssize_t (while 4.x BSD and libc4 and libc5 all have int). The flags 
>>> argument is int in 4.x BSD, but unsigned int in libc4 and libc5. The 
>>> len argument is int in 4.x BSD, but size_t in libc4 and libc5. The 
>>> addrlen argument is int * in 4.x BSD, libc4 and libc5. The present 
>>> socklen_t * was invented by POSIX. See also accept(2). "
>>>
>>> I'm assuming from this that libc4 and libc5 pre-date glibc2 and that 
>>> we no longer have to worry about them?
>>
>> That's what I'm assuming.  But thinking about it a bit more, it would 
>> be a
>> good idea to cast the flags arguments to uint so that whether or not the
>> formal flags argument types in the OS include files are signed or 
>> unsigned,
>> the value will be zero-extended if it's wider.  Done in the new webrev.
>>
>>>
>>> ---
>>>
>>> os_<os>.inline.hpp
>>>
>>> These files redundantly include sys/socket.h directly when it is now 
>>> coming in via jvm_<os>.hpp via os.hpp
>>
>> Yes, but the jvm_<os>.h files seem to be a hack, so I'd prefer to leave
>> the includes in the os_<os>.inline.hpp files.
>>
>>>
>>> ---
>>>
>>> os_solaris.cpp
>>>
>>> In recv/send etc do we have a potential issue with the casts from 
>>> ssize_t to int on 64-bit systems?
>>
>> We do, but since the jdk interfaces return ints, there's nothing we 
>> can do
>> unless we change those interfaces, so I didn't bother fixing it in 
>> the jvm.
>> I'll file a CR against the jdk.
>>
>>>
>>> ---
>>>
>>> jvm_windows.h
>>>
>>> Shouldn't you include ws2tcpip.h to get socklen_t? That's what the 
>>> JDK networking code uses. (Though as all this sock stuff is unused 
>>> on win32 I guess it doesn't really matter.)
>>
>> Done.
>>
>> Thanks again,
>>
>> Paul
>>
>>>
>>>
>>>> Thanks,
>>>>
>>>> Paul
>>>>