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

[Coleen Phillimore]

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
>>>