On Mon, Nov 14, 2016 at 11:25:30PM +0000, David Turner wrote:
> > But it does seem to work. At least it doesn't seem to break anything
> > in the test suite, and it fixes the new tests you added. I'd worry
> > that there's some obscure case where the response isn't packetized
> > in the same way.
>
> Overall, this looks good to me. The state machine is pretty clean. I
> think I would have used a tiny buffer for the length field, and then I
> would have regretted it. Your way looks nicer than my unwritten patch
> would have looked.
Heh, I started it that way but you end up dealing with the same states
(they're just implicit in your "how big is my temp buffer" field).
> > +#define READ_ONE_HEX(shift) do { \
> > + int val = hexval(buf[0]); \
> > + if (val < 0) { \
> > + warning("error on %d", *buf); \
> > + rpc->pktline_state = RPC_PKTLINE_ERROR; \
> > + return; \
> > + } \
> > + rpc->pktline_len |= val << shift; \
>
> Nit: parenthesize shift here, since it is a parameter to a macro.
Yeah, I'm often a bit slack on these one-off inside-a-function macros.
But it does not hurt to to be careful.
I'll make that change and then try to wrap this up with a commit
message. I plan to steal your tests, if that's OK.
-Peff