change NextMsgLen() to return an uint32 instead of int (fixes #7) #8
Conversation
|
this LGTM. @whyrusleeping check? (important not to screw this up) |
|
Uhm, wait.. |
| if _, err := io.ReadFull(s.R, s.lbuf); err != nil { | ||
| return 0, err | ||
| } | ||
| s.next = int(NBO.Uint32(s.lbuf)) | ||
| n := NBO.Uint32(s.lbuf) | ||
| if n < 0 { |
There was a problem hiding this comment.
will this ever be true? if its a uint32?
There was a problem hiding this comment.
True, I overdid it.
There was a problem hiding this comment.
going to remove this?
was waiting for this to merge, but need my changes (#9) sooner. also cleans up all this logic and puts it in one place-- may be easier to rebase on that than vv
There was a problem hiding this comment.
Sorry, Sure - i can rebase, got derailed again. Actually only saw this twice on a box. I guess it was a dying client or something.
Sent from my iPhone
On 04.09.2015, at 16:32, Juan Benet notifications@github.com wrote:
In msgio.go:
if _, err := io.ReadFull(s.R, s.lbuf); err != nil { return 0, err }
s.next = int(NBO.Uint32(s.lbuf))n := NBO.Uint32(s.lbuf)if n < 0 {was waiting for this to merge, but need my changes (#9) sooner. also cleans up all this logic and puts it in one place-- may be easier to rebase on that than vv
—
Reply to this email directly or view it on GitHub.
cryptix
force-pushed
the
NextMsgLen
branch
2 times, most recently
from
dbf86f4
to
7e1bd4b
Compare
|
@jbenet @whyrusleeping rebased - can you take another look? |
| if s.next > s.max { | ||
| return 0, ErrMsgTooLarge | ||
| } | ||
|
|
There was a problem hiding this comment.
the point of this function is to cache the result of ReadLen. i believe it will be called multiple times before another read is merited.
There was a problem hiding this comment.
@jbenet I dont think thats right.
// NextMsgLen reads the length of the next msg into s.lbuf, and returns it.
// WARNING: like Read, NextMsgLen is destructive. It reads from the internal
// reader.
There was a problem hiding this comment.
look at the older code. it's meant to be able to call nextMsgLen multiple times.
There was a problem hiding this comment.
oooh, okay. so the comment on NextMsgLen is incorrect.
|
The commit claims "change NextMsgLen() to return an uint32 instead of int ". this commit also changes nextMsgLen to something that i think is incorrect, as the func will be called multiple times. Maybe divide this into two PRs? the Errs and |
|
This is waiting for a split between the two pieces. |
|
@cryptix do you have time soon? i think @whyrusleeping wants it to fix #1767 -- else we can probably do it |
|
Sorry, no you go ahead please. This code is too fragile for my taste and you seem to have a lot of assumptions about it that aren't tested. |
see discussion on #7