451205 - NSPR doesn't provide a way for setting the Don't Fragment (DF) bit for UDP sockets

Status: NEW

(NSPR :: NSPR, enhancement)

Description

[Eilon]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.1) Gecko/2008071618 Iceweasel/3.0.1 (Debian-3.0.1-1)
Build Identifier: 

It is useful for some applications to set/unset the DF bit to make sure data is not fragmented regardless of the local MTU.
For example, Path MTU Discovery needs to send probes with the DF bit set.
Unfortunately, not all platforms can support this for non-privileged sockets (i.e. Mac OS), but it is still useful to have this option for platforms that does support it.

Reproducible: Always

Comment 1

[Eilon]

Attachment: DF bit patch

Proposed patch for allowing to set the DF bit. This works for Windows and Linux and should work for AIX.

Comment 2

[Wan-Teh Chang]

Comment on attachment 334503 [details] [diff] [review]
DF bit patch

Thanks for the patch.

Just curious: among all the platforms, why did you
mention only Windows, Linux, and AIX?

Some quick comments:
1. Don't use C++ style comments //.
2. Instead of

#else
#ifdef FOO
    AAA
#else
    BBB
#endif
#endif


use

#elif defined(FOO)
    AAA
#else
    BBB
#endif

Note that you need to remove one #endif.

Comment 3

[Eilon]

Thanks for your comments.

Regarding the platforms, I was looking at the platforms we mostly need for
zap. Out of these Mac OS does not allow setting the DF bit.
I added AIX as it was very easy to do (just a different #define).

Comment 4

[Eilon]

Attachment: updated patch

Fixed previous comments on the patch.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 335949 [details] [diff] [review]
updated patch

I just discovered this patch, sitting here for months without any 
review request. :(  

r- for several issues.

>diff --git a/nsprpub/pr/include/prio.h b/nsprpub/pr/include/prio.h
>--- a/nsprpub/pr/include/prio.h
>+++ b/nsprpub/pr/include/prio.h

1. Please submit NSPR patches as CVS diff's not Hg/git diffs. 
The primary upstream NSPR repository is CVS.  Please ask Wan-Teh or
myself to review the next patch.  Thanks.

>@@ -225,16 +225,17 @@ typedef enum PRSockOption
>     PR_SockOpt_IpTypeOfService, /* type of service and precedence */
>+    PR_SockOpt_IpDontFragment,  /* data should not be fragmented */ 
>     PR_SockOpt_AddMember,       /* add an IP group membership */

2. To preserve binary compatibility, in public header files, any time
a new symbol is added to an enumerated type, it must be added at the 
end, not in the middle.  This change will also affect the order of the 
members of the arrays "socketOptions" and "socketLevels" in 
_PR_SocketSetSocket.

>+#ifdef IP_MTU_DISCOVER /* For Linux */
>+#define IP_DONTFRAGMENT IP_MTU_DISCOVER
>+#elif defined(IP_DONTFRAG) /* For AIX */ 
>+#define IP_DONTFRAGMENT IP_DONTFRAG
>+#endif

3. What about other platforms?

>+            case PR_SockOpt_IpDontFragment:
>+            {
>+#ifdef WIN32 /* Winsock */
>+                BOOL value = data->value.ip_df ? 1 : 0;
>+#elif defined(IP_MTU_DISCOVER)
>+                /* 3 - IP_PMTUDISC_PROBE */
>+                /* 0 - IP_PMTUDISC_DONT  */
>+                PRIntn value = (data->value.ip_df) ? 3 : 0;
>+#else
>+                PRIntn value = (data->value.ip_df) ? 1 : 0;
>+#endif

4. Why are these values 3 and 0 used as commented magic numbers?
If the symbols IP_PMTUDISC_PROBE and IP_PMTUDISC_DONT are defined,
why not use them?

5. Shouldn't that be 
#elif defined(IP_PMTUDISC_PROBE) 
or
#elif defined(IP_DONTFRAGMENT) 
instead of 
#elif defined(IP_MTU_DISCOVER) 
?  

Comments 4 and 5 also apply to case PR_SockOpt_IpDontFragment: in
function pt_SetSocketOption.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

I confirm this is a request to add a new feature to NSPR.

Comment 7

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.