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

[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?
> 
> After we agree on this VM interface (and I think it is good, I just want
> to try it out first) we can discuss the rest of the patch. Just one
> quick nitpick now already: please keep the boilerplate at the top of the
> file intact, some of the files in the patch have the old FSF address.

Sorry to take so long for following up on this. I had hoped that some
review would just fall out from the recent remerge of classpath into
libgcj, but the implementations have diverged a lot.

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

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

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

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

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

- PlainSocketImpl.sendUrgentData() should delegate to VMPlainSocketImpl.

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

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

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

Cheers,

Mark