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

[David Holmes]

Responses inline ...

On 22/12/2011 12:03 AM, Paul Hohensee wrote:
> Thank you for the thorough review. New webrev here
>
> http://cr.openjdk.java.net/~phh/7091417.01/
<snip>
>> 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.

This confuses me. The current system APIs are defined to take int, but 
we're passing in uint. But at least this change means that in effect 
there is no change to this code (uint before, uint now).

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

I don't quite follow the reasoning. Whether a hack or not the jvm_* 
files exist and are used and so the pre-existing includes are now 
redundant. Not a big deal.

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

Ok.

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

I understand this change has been backed out as ws2tcpip.h caused 
additional problems. Using an explicit typedef here (whether int or 
uint) is fine as it is unused code.


David
-----


>
> Done.
>
> Thanks again,
>
> Paul
>
>>
>>
>>> Thanks,
>>>
>>> Paul
>>>