Moves cli helpers to pkgs #31
Conversation
In particular, use io.ReadFull to ensure we don't have partial reads.
| "github.com/Comcast/gots" | ||
| ) | ||
|
|
||
| // Sync finds the offset of the next packet sync byte and returns the offset of |
There was a problem hiding this comment.
Comment should start with FindNextSync.
Also, I think that there is a bug here. I know that you didn't introduce it, but it should be fixed. If we are checking the next packet's sync byte after we find the first sync byte, the reader is left skipping the first valid packet. I would argue that this func should only consume those bytes before the first sync byte. A reader is tricky because once we find a sync byte, we have consumed it. I am not sure the best way to solve this, but I would consider using a []byte].
There was a problem hiding this comment.
After checking #18 We could likely solve this with bufio.Reader
There was a problem hiding this comment.
Yeah I noticed this and actually that is one reason I changed the name (I missed the comment). In the parsefile example it does the seek to the position returned so the change of the reader position caused here doesn't affect it, but it would be better to have this actually bring the reader to the right point instead.
Out of curiosity, do you know the reason it also checks the 2nd sync after the first found?
There was a problem hiding this comment.
Not sure why the next sync byte is checked. It could be just to make sure it's actually a packet start and not an arbitrary 0x47. Any additional sync byte verification should be done explicitly by the user of the library though.
| return pat, nil | ||
| } | ||
| } | ||
| return nil, gots.ErrPATNotFound |
|
LGTM. Once you send me the signed CLA the changes can be merged. [email protected] |
|
@davemt Are you going to sign the CLA or should I close this? |
|
Ah, your previous comment slipped by me. I'll send it over to you. |
Moves the CLI helper functions to packages so that they are made available as part of the API.
It also switches some of the Read functions to be io.ReadFull to ensure full reads as per recommendation in #18.
Resolves #30.