[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-runtime-dev
Subject:    Pls review 7091417: recvfrom's 6th input should be of type socklen_t (M)
From:       paul.hohensee () oracle ! com (Paul Hohensee)
Date:       2011-12-22 1:02:20
Message-ID: 4EF2819C.1080904 () oracle ! com
[Download RAW message or body]

Thank you for the re-review.

Inline...

On 12/21/11 7:53 PM, David Holmes wrote:
> 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).

Yes.  I now think it's a good idea to start with an equal-to-int-width 
unsigned rather
than signed type in case the OS interface ever changes flags to a wider 
unsigned
type.  Overkill, I guess.

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

Thanks.

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

Ok.

Paul
>
>
> David
> -----
>
>
>>
>> Done.
>>
>> Thanks again,
>>
>> Paul
>>
>>>
>>>
>>>> Thanks,
>>>>
>>>> Paul
>>>>

[prev in list] [next in list] [prev in thread] [next in thread]