Re: [PATCH v2 1/2] Use packet_reader instead of packet_read_line
- From
- Jonathan Tan <jonathantanmy@google.com>
- Date
- Jan 7, 2019, 19:01 UTC
- Message-ID
- <20190107190111.227874-1-jonathantanmy@google.com>
- In-Reply-To
- <20181229211915.161686-2-masayasuzuki@google.com>
> By using and sharing a packet_reader while handling a Git pack protocol > request, the same reader option is used throughout the code. This makes > it easy to set a reader option to the request parsing code.
I see that packet_read() and packet_read_line_buf() invocations are also removed, so it might be better to use "Use packet_reader instead of packet_read.*" as the commit title.
The code looks correct to me - most of the changes are removals of packet_read_line(), replaced with a packet_reader that has PACKET_READ_CHOMP_NEWLINE. One instance is packet_read(), for which the corresponding reader does not have PACKET_READ_CHOMP_NEWLINE (noted below); and another instance is packet_read_line_buf(), for which the corresponding reader is instantiated accordingly with the buffer (also noted below).
Show 26 quoted lines
> - if (!strcmp(line, "push-cert")) {
> + if (!strcmp(reader->line, "push-cert")) {
> int true_flush = 0;
> - char certbuf[1024];
> + int saved_options = reader->options;
> + reader->options &= ~PACKET_READ_CHOMP_NEWLINE;
>
> for (;;) {
> - len = packet_read(0, NULL, NULL,
> - certbuf, sizeof(certbuf), 0);
> - if (!len) {
> + packet_reader_read(reader);
> + if (reader->status == PACKET_READ_FLUSH) {
> true_flush = 1;
> break;
> }
> - if (!strcmp(certbuf, "push-cert-end\n"))
> + if (reader->status != PACKET_READ_NORMAL) {
> + die("protocol error: got an unexpected packet");
> + }
> + if (!strcmp(reader->line, "push-cert-end\n"))
> break; /* end of cert */
> - strbuf_addstr(&push_cert, certbuf);
> + strbuf_addstr(&push_cert, reader->line);
> }
> + reader->options = saved_options;Here, packet_read() is used, so we shouldn't chomp the newline, so this is correct.
Show 37 quoted lines
> - char *line;
> + struct packet_reader reader;
> + packet_reader_init(&reader, -1, last->buf, last->len,
> + PACKET_READ_CHOMP_NEWLINE);
>
> /*
> * smart HTTP response; validate that the service
> * pkt-line matches our request.
> */
> - line = packet_read_line_buf(&last->buf, &last->len, NULL);
> - if (!line)
> + if (packet_reader_read(&reader) != PACKET_READ_NORMAL)
> die("invalid server response; expected service, got flush packet");
>
> strbuf_reset(&exp);
> strbuf_addf(&exp, "# service=%s", service);
> - if (strcmp(line, exp.buf))
> - die("invalid server response; got '%s'", line);
> + if (strcmp(reader.line, exp.buf))
> + die("invalid server response; got '%s'", reader.line);
> strbuf_release(&exp);
>
> /* 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.
> */
> - while (packet_read_line_buf(&last->buf, &last->len, NULL))
> - ;
> + for (;;) {
> + packet_reader_read(&reader);
> + if (reader.pktlen <= 0) {
> + break;
> + }
> + }
> +
> + last->buf = reader.src_buffer;
> + last->len = reader.src_len;And here, packet_reader_init() correctly initializes the packet_reader with the buffer, and we need to know where in the buffer to continue after parsing the additional metadata lines and the packet flush, so we assign the state of the reader to last->buf and last->len.
(Incidentally, with this change, there is no longer any usage of packet_read_line_buf(), but we can remove that in a subsequent patch.)
In summary, this looks like a good change. Configuration of reader metadata (file descriptors, input buffers, and flags) is now more centralized, and there are fewer file descriptor constants and variables (which all look like ints) strewn around.