Closed Bug 1007024 Opened 10 years ago Closed 10 years ago

Http2Session: simplify 32bit numeric insertions in http2 frames

Categories

(Core :: Networking: HTTP, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: bagder, Assigned: bagder)

References

Details

(Whiteboard: [spdy])

Attachments

(1 file, 3 obsolete files)

Upon code review of Http2Session.cpp I found the use of PR_htonl() and associated memcpy() in the code a bit awkward and it feels error-prone.

I therefore suggest a convenience function that simply inserts a 32 bit numerical in network byte order at the given location. See patch.

It also removes a whole bunch of "magic numbers" in the code (namely the size of the data it memcpys).
Blocks: 1007035
Assignee: mcmanus → daniel
Comment on attachment 8418593 [details] [diff] [review]
0001-Http2Session-insert-32bit-network-byte-order-values-.patch

Review of attachment 8418593 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/protocol/http/Http2Session.cpp
@@ +130,5 @@
> +// copy the 32 bit number into the destination, using network byte order
> +// in the destination
> +//
> +static void
> +copy_32bit(char *dest,      /* where to store it */

I would like a different name to reflect the endinaness going on here. CopyAsNetwork32() perhaps? or CopyAsBigEndian32()?

// is preferred comment style - especially on same line

@@ +617,2 @@
>  
> +  memcpy(dest, &frameLength, sizeof(frameLength));

I don't like this change because the spec deals in a length of 2, not the sizeof a stack variable. So its more clear to me as 2.

Adding static_assert(sizeof(frameLength) == 2) if you're nervous about it is fine - but the thing is a uint16_t afterall.

@@ +619,3 @@
>    dest[2] = frameType;
>    dest[3] = frameFlags;
> +  copy_32bit((char *) dest + 4, streamID);

we use c++ style casts.. and you shouldn't need to cast this - provide a template of copy_32bit or two versions or whatever..
All good comments.

This PR_htonl()+memcpy-using pattern is also very common in the SPDY-code, do you think it would be worth it making a single function that all of them can use?
(In reply to Daniel Stenberg [:bagder] from comment #2)
> All good comments.
> 
> This PR_htonl()+memcpy-using pattern is also very common in the SPDY-code,
> do you think it would be worth it making a single function that all of them
> can use?

good question - the duplication is because each new generation of spdy (including http2) was a fork of the previous generation. We don't really do improvements to the old ones (other than bug fixes, of course) - preferring instead to let them retire. So a nice incremental update like this one would only be done to http2 at this point.

Its an unusual strategy, but because spdy is meant to be an interim step on the way to http2 its actually worked well - new features don't need to live in a confusing pile of "what version am I?" branches. We'll retire spdy3 and spdy31 after http2 is solid.
Fixed the function name, replaced the typecast with a template, removed sizeof() (which really shouldn't have been there to begin with as that was an unrelated changed). Submitted to the try server:

https://tbpl.mozilla.org/?tree=Try&rev=9c7fa59ef06f
Attachment #8418593 - Attachment is obsolete: true
Attachment #8420131 - Flags: review?(mcmanus)
Comment on attachment 8420131 [details] [diff] [review]
v2-0001-Bug-1007024-CopyAsNetwork32-inserts-32bit-network.patch

Review of attachment 8420131 [details] [diff] [review]:
-----------------------------------------------------------------

your try is burning
https://tbpl.mozilla.org/php/getParsedLog.php?id=39361549&tree=Try#error0
Attachment #8420131 - Flags: review?(mcmanus)
Oh, that was a lesson into C++ template land. Clearly gcc was not as strict as clang there. Sorry for being a bit rusty in my C++iness. I removed the static keywords from the templates and here's the try run on this v3 of the patch: https://tbpl.mozilla.org/?tree=Try&rev=8d04609a273e
Attachment #8420131 - Attachment is obsolete: true
Attachment #8420315 - Flags: review?(mcmanus)
Comment on attachment 8420315 [details] [diff] [review]
v3-0001-Bug-1007024-CopyAsNetwork32-inserts-32bit-network.patch

Review of attachment 8420315 [details] [diff] [review]:
-----------------------------------------------------------------

one q, otherwise lgtm

::: netwerk/protocol/http/Http2Session.cpp
@@ +128,5 @@
>  
> +// Copy the 32 bit number into the destination, using network byte order
> +// in the destination.
> +template<typename charType> static void
> +CopyAsNetwork32(charType *dest,  // where to store it

shouldn't that be just charType dest?
(In reply to Patrick McManus [:mcmanus] from comment #7)

> shouldn't that be just charType dest?

Oh yes it should! :-/
Fixed first input argument type to CopyAsNetwork32()
Attachment #8420315 - Attachment is obsolete: true
Attachment #8420315 - Flags: review?(mcmanus)
Attachment #8422262 - Flags: review+
Keywords: checkin-needed
FWIW, using mozilla/Endian.h's NetworkEndian::writeUint32 might have been even better.
https://hg.mozilla.org/mozilla-central/rev/72e9b97fbcb7
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
(In reply to Nathan Froyd (:froydnj) from comment #11)
> FWIW, using mozilla/Endian.h's NetworkEndian::writeUint32 might have been
> even better.

We should continue to bug 945533.  PR_ntoh* and PR_hton* don't use bswap ops on Windowns Socket.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: