191187 - Request for UDP/IP (datagram) socket provider and support

Status: RESOLVED INCOMPLETE

(Core :: Networking, enhancement)

Description

[Charles Melhorn]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.2.1) Gecko/20021130
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.2.1) Gecko/20021130

Currently, the Netlib CreateTransportOfType() method on
nsISocketTransportService provides only the ability to create TCP/IP sockets or
tcp variants: tcp+SSL, tcp+TLS, tcp+SOCKS, and tcp+SOCKS4. It would be useful if
you could extend the library's set of socket providers to include one for UDP
(datagram) sockets, perhaps by making use of the portable runtime function
PR_NewUDPSocket() and related routines, and provide general support for
(scriptable) UDP sockets as necessary.

Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[Darin Fisher]

see the thread on n.p.m.netlib: http://makeashorterlink.com/?G32D32543

Comment 2

[Alex Fritze]

I have some code for this on the zap branch, but it will need some tlc before rolling it into a patch.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Alex, do you have any update for us what's happen with your code? Is it in a state to create a patch? It would be very helpful for me due to I need to send UDP datagrams. Or do you have another hint? Thanks

Comment 4

[:Mook]

Attachment: dougt's patch, updated to trunk and built as part of netwerk

This is just dougt's patch (from the newsgroups) made to compile on trunk and built in netwerk.  It seems to be good enough to send something and get a response, at least.

Comment 5

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 262730 [details] [diff] [review]
dougt's patch, updated to trunk and built as part of netwerk

- please use consistent indentation in netwerk/socket/base/nsUDPSocketProvider.cpp, ideally 4-space per the style in this directory (same for the .h)

- where did you copy the code in that file from, given that it was supposedly created by Netscape in 1998? (same for the .h)

- AddToSocket should get an NS_NOTREACHED and a failure return (NS_ERROR_UNEXPECTED, I guess)


this needs a unit test before this is checked in.

Comment 6

[Alex Fritze]

(In reply to comment #3)
> Alex, do you have any update for us what's happen with your code? Is it in a
> state to create a patch? It would be very helpful for me due to I need to send
> UDP datagrams. Or do you have another hint? Thanks
> 

Henrik: Sorry for the lag - I'm gonna attach a patch with what I have in a minute. It's basic, but works well for me in zap.

Comment 7

[Alex Fritze]

Attachment: udp socket support from zap branch

Comment 8

[:Mook]

Attachment: now with a test

- indentation fixed
- the 1998 files are from dougt's newsgroup post; I left the copyrights as-is because as far as I know that's it.  (The new test, which I did write from scratch, has (c) 2007)
- AddToSocket fixed.

Of course, now that Alex attached his patch, I have no idea where to go from here - his patch actually makes more sense (in that it has UDP-specific things).  This one is still good enough to send something and receive responses, but that's about it...

Comment 9

[Christian :Biesinger (don't email me, ping me on IRC)]

OK, so I do want to take something like afri's patch, since that allows listening for datagrams too. License headers would be good though; and I didn't do a real review of the interface (or implementation, for that matter) yet.

It also makes it easier to tell where the boundaries of the individual datagrams are.

Now... Mook's patch is nice in that it parallels TCP. It now occurred to me though that it has the problem that you don't see packet boundaries there, which can be a problem because datagrams can get lost... hm...

Comment 10

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 263953 [details] [diff] [review]
now with a test

OK, this approach is probably good enough too. I still want to take something like afri's patch in addition.

seems like you could use PR_ErrorToString instead of the buffer allocation stuff

+        rv = outstream->Write((char*)&data, sizeof(PRUint32), &count);

please don't cast away const, that's not nice

Comment 11

[:Mook]

Attachment: udp socket provider+test, addresses comment 10

(In reply to comment #10)
> 
> seems like you could use PR_ErrorToString instead of the buffer allocation
> stuff
>
Fixed, didn't know that existed :)
> +        rv = outstream->Write((char*)&data, sizeof(PRUint32), &count);
> 
> please don't cast away const, that's not nice
> 
Fixed, that was silly of me.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 264541 [details] [diff] [review]
udp socket provider+test, addresses comment 10

>Index: netwerk/socket/base/nsUDPSocketProvider.cpp
>+ * Version: NPL 1.1/GPL 2.0/LGPL 2.1

Really?  And is Netscape really the original developer?

Check with dougt on the latter; and use MPL when landing this, no matter what the answer to that question is.  There should be no more NPL landing into the tree.

Same thing for the header.

With that, sr=bzbarsky (note: I didn't really read the test code).

Comment 13

[:Mook]

Attachment: udp socket provider+test, MPL

Updated to MPL, sorry about that.  Carrying forward r/sr

I did ask dougt on IRC, and as far as I understand his response, yes, Netscape/1998.

(Sorry about the delay, my machine was having fun deciding that it no can no longer link libxul for some reason... bah)

Comment 14

[Shawn Wilsher :sdwilsh]

(In reply to comment #5)
> this needs a unit test before this is checked in.

Ahem.  Please get this, then I can check it in.

Comment 15

[Shawn Wilsher :sdwilsh]

Disregard previous comment, I can't read patches apparently.

Comment 16

[timeless]

Comment on attachment 265390 [details] [diff] [review]
udp socket provider+test, MPL

mozilla/netwerk/build/nsNetCID.h 	1.66
mozilla/netwerk/build/nsNetModule.cpp 	1.141
mozilla/netwerk/socket/base/Makefile.in 	1.22
mozilla/netwerk/socket/base/nsUDPSocketProvider.cpp 	1.1
mozilla/netwerk/socket/base/nsUDPSocketProvider.h 	1.1	mozilla/netwerk/test/Makefile.in 	1.95
mozilla/netwerk/test/TestUDPSocketProvider.cpp 	1.1

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+1]]

UDP support should be available for 1.9a5. We also have an unit test so the in-testsuite flag should be set?

Comment 18

[:Gavin Sharp [email: [e-mail]]]

If this has a unit test it's "in-testsuite" flag should be set to "+".

Comment 19

[:Mook]

Umm, there's still afri's zap branch thing that provides UDP-specific support...

Should that be spun into a new bug or something?  Or reopen this one?  Because the socket provider itself doesn't really expose everything UDP would make sense for...

Comment 20

[:Mook]

Reopening for afri's more useful part from the zap branch.

Comment 21

[Jonathan Watt [:jwatt]]

Alex has been way too busy to pay attention to bugzilla for some time, but he's more than happy for people to pick up any of his stuff and run with it (as Robert did in bug 238324). Feel free to work on anything you find useful from the zap to get it landed on trunk.

Comment 22

[Caio Tiago Oliveira (asrail)]

Comment on attachment 263590 [details] [diff] [review]
udp socket support from zap branch

Asking for r on behalf of NSA.

Comment 23

[Caio Tiago Oliveira (asrail)]

Attachment: Unbitrotting Alex patch

Comment 24

[Boris Zbarsky [:bzbarsky]]

1)  I doubt I can do serious reviews here until sometime in the spring.
2)  This last patch is clearly missing pieces (e.g. nsUDPSocket.cpp).

Comment 25

[Caio Tiago Oliveira (asrail)]

Attachment: Missed the new files, sorry

Comment 26

[Boris Zbarsky [:bzbarsky]]

See comment 24 item 1.  I don't really know our socket code well, so I'll have to learn it to review this.  You might have better luck with biesi.

Comment 27

[Caio Tiago Oliveira (asrail)]

Comment on attachment 286518 [details] [diff] [review]
Missed the new files, sorry

Asking for biesi review.

I was not on the CC list and haven't seen bz comments...

Comment 28

[alexanderz]

Alex, are you still planning to do this or have you abandoned it?

Comment 29

[Alex Fritze]

(In reply to comment #28)
> Alex, are you still planning to do this or have you abandoned it?

The patch is pretty much obsolete. There is much more complete UDP socket support in the zap repository (http://hg.mozilla.org/users/alex_croczilla.com/zap-central/file/ecd584fc9599/zap/netutils/src/). I might try to port this to Mozilla proper at some point in the future, but that should really be a bug on its own. So I'm closing this one.