Josh Steadmon <stead...@google.com> writes:
>> + packet_reader_init(&reader, -1, d->buf, d->len,
>> + PACKET_READ_CHOMP_NEWLINE |
>> + PACKET_READ_DIE_ON_ERR_PACKET);
>> + if (packet_reader_read(&reader) != PACKET_READ_NORMAL)
>> + die("invalid server response; expected service, got flush
>> packet");
>
> This can also trigger on an EOF or a delim packet, should we clarify the
> error message?
You mean that it may not be "flush packet"? I guess we can remove
", got X" part and that would probably be an improvement.
>> +
>> + if (skip_prefix(reader.line, "# service=", &p) && !strcmp(p, service)) {
>> + /*
>> + * The header can include additional metadata lines, up
>> + * until a packet flush marker. Ignore these now, but
>> + * in the future we might start to scan them.
>> + */
>> + for (;;) {
>> + packet_reader_read(&reader);
>> + if (reader.pktlen <= 0) {
>> + break;
>> + }
>> + }
>
> Could we make this loop cleaner as:
>
> while (packet_reader_read(&reader) != PACKET_READ_NORMAL)
> ;
The only case as far as I can tell that would make the difference
between the original and your simplified one would be if a packet
had a single newline and nothing else in it, in which case pktlen
would be zero (since we pass CHOMP_NEWLINE) and the return value
would be READ_NORMAL. The original would break, while yours will
spin one more time.
Thanks.
