Closed
Bug 1007024
Opened 10 years ago
Closed 10 years ago
Http2Session: simplify 32bit numeric insertions in http2 frames
Categories
(Core :: Networking: HTTP, defect)
Core
Networking: HTTP
Tracking
()
RESOLVED
FIXED
mozilla32
People
(Reporter: bagder, Assigned: bagder)
References
Details
(Whiteboard: [spdy])
Attachments
(1 file, 3 obsolete files)
6.08 KB,
patch
|
bagder
:
review+
|
Details | Diff | Splinter Review |
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).
Updated•10 years ago
|
Assignee: mcmanus → daniel
Comment 1•10 years ago
|
||
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..
Assignee | ||
Comment 2•10 years ago
|
||
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?
Comment 3•10 years ago
|
||
(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.
Assignee | ||
Comment 4•10 years ago
|
||
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 5•10 years ago
|
||
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)
Assignee | ||
Comment 6•10 years ago
|
||
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 7•10 years ago
|
||
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?
Assignee | ||
Comment 8•10 years ago
|
||
(In reply to Patrick McManus [:mcmanus] from comment #7) > shouldn't that be just charType dest? Oh yes it should! :-/
Assignee | ||
Comment 9•10 years ago
|
||
Fixed first input argument type to CopyAsNetwork32()
Attachment #8420315 -
Attachment is obsolete: true
Attachment #8420315 -
Flags: review?(mcmanus)
Attachment #8422262 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 10•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/72e9b97fbcb7
Keywords: checkin-needed
Comment 11•10 years ago
|
||
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
Comment 13•10 years ago
|
||
(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.
Description
•