Re: [PATCH 1/3] remote-curl: refactor smart-http discovery

Josh Steadmon [off-list ref] writes:

quoted

+	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.

quoted

+
+	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.