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

[Paul Hohensee]

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