Brandon Williams <bmwill@xxxxxxxxxx> writes:
> +enum packet_read_status packet_reader_read(struct packet_reader *reader)
> +{
> + if (reader->line_peeked) {
> + reader->line_peeked = 0;
> + return reader->status;
> + }
> +
> + reader->status = packet_read_with_status(reader->fd,
> + &reader->src_buffer,
> + &reader->src_len,
> + reader->buffer,
> + reader->buffer_size,
> + &reader->pktlen,
> + reader->options);
> +
> + switch (reader->status) {
> + case PACKET_READ_EOF:
> + reader->pktlen = -1;
> + reader->line = NULL;
> + break;
> + case PACKET_READ_NORMAL:
> + reader->line = reader->buffer;
> + break;
> + case PACKET_READ_FLUSH:
> + reader->pktlen = 0;
> + reader->line = NULL;
> + break;
> + }
> +
> + return reader->status;
> +}
With the way _peek() interface interacts with the reader instance
(which by the way I find is well designed), it is understandable
that we want almost everything available in reader's fields, but
having to manually clear pktlen field upon non NORMAL status feels
a bit strange.
Perhaps that is because the underlying packet_read_with_status()
does not set *pktlen in these cases? Shouldn't it be doing that so
the caller does not have to?
A similar comment applies for reader's line field. In priniciple,
as the status field is part of a reader, it does not have to exist
as a separate field, i.e.
#define line_of(reader) \
((reader).status == PACKET_READ_NORMAL ? \
(reader).buffer : NULL)
can be used to as substitute for it. I guess it depends on how the
actual callers wants to use this interface.
