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

RESOLVED INCOMPLETE

Status

()

Core
Networking
--
enhancement
RESOLVED INCOMPLETE
15 years ago
6 years ago

People

(Reporter: Charles Melhorn, Assigned: Alex Fritze)

Tracking

Trunk
mozilla1.9alpha5
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 5 obsolete attachments)

(Reporter)

Description

15 years ago
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

15 years ago
see the thread on n.p.m.netlib: http://makeashorterlink.com/?G32D32543
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: helpwanted
Target Milestone: --- → Future
(Assignee)

Comment 2

12 years ago
I have some code for this on the zap branch, but it will need some tlc before rolling it into a patch.
Blocks: 317491

Updated

10 years ago
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

Updated

10 years ago
Assignee: dougt → nobody
QA Contact: benc → networking

Comment 4

10 years ago
Created attachment 262730 [details] [diff] [review]
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.
Assignee: nobody → mook.moz+mozbz
Status: NEW → ASSIGNED
Attachment #262730 - Flags: review?(cbiesinger)
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.
Attachment #262730 - Flags: review?(cbiesinger) → review-
(Assignee)

Comment 6

10 years ago
(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.
(Assignee)

Comment 7

10 years ago
Created attachment 263590 [details] [diff] [review]
udp socket support from zap branch

Comment 8

10 years ago
Created attachment 263953 [details] [diff] [review]
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...
Attachment #262730 - Attachment is obsolete: true
Attachment #263953 - Flags: review?(cbiesinger)
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 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
Attachment #263953 - Flags: review?(cbiesinger) → review+

Comment 11

10 years ago
Created attachment 264541 [details] [diff] [review]
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.
Attachment #263953 - Attachment is obsolete: true
Attachment #264541 - Flags: review?(cbiesinger)
Attachment #264541 - Flags: review?(cbiesinger) → review+

Updated

10 years ago
Attachment #264541 - Flags: superreview?(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).
Attachment #264541 - Flags: superreview?(bzbarsky) → superreview+

Comment 13

10 years ago
Created attachment 265390 [details] [diff] [review]
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)
Attachment #264541 - Attachment is obsolete: true
Attachment #265390 - Flags: superreview+
Attachment #265390 - Flags: review+
Keywords: helpwanted
Whiteboard: [checkin needed]
(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.
Whiteboard: [checkin needed]
Disregard previous comment, I can't read patches apparently.
Whiteboard: [checkin needed]

Comment 16

10 years ago
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
UDP support should be available for 1.9a5. We also have an unit test so the in-testsuite flag should be set?
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Whiteboard: [checkin needed]
Target Milestone: Future → mozilla1.9alpha5
Flags: in-testsuite?
If this has a unit test it's "in-testsuite" flag should be set to "+".
Flags: in-testsuite? → in-testsuite+

Comment 19

10 years ago
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

10 years ago
Reopening for afri's more useful part from the zap branch.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Updated

10 years ago
Assignee: mook.moz+mozbz → alex
Status: REOPENED → NEW
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 on attachment 263590 [details] [diff] [review]
udp socket support from zap branch

Asking for r on behalf of NSA.
Attachment #263590 - Flags: review?(bzbarsky)
Created attachment 286516 [details] [diff] [review]
Unbitrotting Alex patch
Attachment #263590 - Attachment is obsolete: true
Attachment #286516 - Flags: review?(bzbarsky)
Attachment #263590 - Flags: review?(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).
Created attachment 286518 [details] [diff] [review]
Missed the new files, sorry
Attachment #286516 - Attachment is obsolete: true
Attachment #286518 - Flags: review?(bzbarsky)
Attachment #286516 - Flags: review?(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 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...
Attachment #286518 - Flags: review?(bzbarsky) → review?(cbiesinger)

Comment 28

9 years ago
Alex, are you still planning to do this or have you abandoned it?
(Assignee)

Comment 29

9 years ago
(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.
Status: NEW → RESOLVED
Last Resolved: 10 years ago9 years ago
Resolution: --- → INCOMPLETE
Attachment #286518 - Flags: review?(cbiesinger)
You need to log in before you can comment on or make changes to this bug.