Re: [cp-patches] RFC: new VM interface for Socket impls

[Roman Kennke]

Hi Mark,

Am Mittwoch, den 07.12.2005, 13:35 +0100 schrieb Mark Wielaard:
> On Fri, 2005-11-11 at 17:53 +0100, Mark Wielaard wrote:
> > On Wed, 2005-11-09 at 21:16 +0000, Roman Kennke wrote:
> > > Ingo has abstracted out the native code from gnu.java.net.PlainSocketImpl
> > > and gnu.java.net.PlainDatagramSocketImpl into a new VM interface. This
> > > allows VM implementors to provide a different implementation for the
> > > native parts of these classes if they wish. Is this ok to commit as it
> > > is? Do you have any suggestions/improvements to the interface? We would
> > > like to have a stable VM interface for this area, so maybe it would be
> > > helpful to discuss this with other VM implementors...
> > 
> > A first comment is that for the VM reference implementation I would just
> > make all methods native directly, no need to chain them to some helper
> > method here. Is there a reason that only nativeLeave() is synchronized?

Ok, then lets make them native directly. This is only a habit, because
in many cases it turned out to be more convenient to wrap a native
method in a java method, i.e. to prepare arguments or do some additional
things on the java side. It doesn't matter much here I guess.

> This isn't a full review, just some additional comments in addition to
> my earlier remarks after studying the libgcj versions. Maybe others
> (Guilhem said Kaffe also doesn't have this part merged) have some
> comments and then you can sent an updated proposal/patch. (I realize
> some of the comments are just reflections on previous design mistakes,
> but lets try clean them up now.)

yeah.

> - Do we need the boolean stream argument in
>   VMPlainSocketImpl.create()?

I think not, datagram sockets are handled separately in
VMDatagramSocketImpl AFAICS.

> - VMPlainSocketImpl.bind() has as comment
>   **** How bind to INADDR_ANY ****
>   Should we make a convention for that one? For example just use null
>   add addr?

I don't really know. I am not really familiar with the net code...

> - There is a PlainSocketImpl.connect() method that takes a timeout
>   argument. We provide a default implementation now, but shouldn't this
>   version move to the VMPlainSocketImpl? I don't know if our default
>   implementation is actually that create, maybe you want to do the
>   actual timeout in a more specific way.

Agreed. So we would have two connect() methods in the VMPlainSocketImpl.

> - Should we provide wrappers/unwrappers for the Integer and Boolean
>   arguments/return values of setOption() and getOption() that call the
>   VM versions with primitive types? That would make the JNI code much
>   cleaner.

Sounds reasonable. I would think that value always is some primitive
type, so we don't really need Object there, right?

> - PlainSocketImpl.sendUrgentData() should delegate to VMPlainSocketImpl.

Agreed.

> - It might makes sense to have a no argument read() and write() method
>   in the VM interface instead of just the byte[] versions.

Also agreed.

> - We should explicitly document the new constant IP_TTL we use in
>   PlainDatagramSocketImpl for the VM interface since it isn't defined as
>   SocketOption. Or should we just make a special VM interface to set/get
>   the TTL?

I vote for documenting the constant.

> - The PlainDatagramSocket multicast opertations join(Group),
>   leave(Group) etc, need to go through to the VM interface.

Agreed.

I will put together a new patch that incorporates the suggested changes.

Cheers, Roman

signature.asc
Description: Dies ist ein digital signierter Nachrichtenteil