[v3,2/3] pkt-line: use string versions of functions

Commit Message

Denton Liu June 23, 2020, 5:55 p.m. UTC

We have many cases where we are writing a control packet as a string
constant out and we need to specify the length of the string. Currently,
the length is specified as a magical `4` literal.

Change these instances to use a function that calls strlen() to
determine the length of the string removing the need to specify the
length at all. Since these functions are inline, the strlen()s should be
replaced with constants at compile-time so this should not result in any
performance penalty.

Signed-off-by: Denton Liu <[e-mail]>
---

Notes:
    Junio, you mentioned in an earlier email[0] that write_str_in_full() and
    strbuf_addstr() each count the string length at runtime. However, I
    don't think that's true since they're both inline functions and
    strbuf_addstr() has the following comment:
    
    	/**
    	 * Add a NUL-terminated string to the buffer.
    	 *
    	 * NOTE: This function will *always* be implemented as an inline or a macro
    	 * using strlen, meaning that this is efficient to write things like:
    	 *
    	 *     strbuf_addstr(sb, "immediate string");
    	 *
    	 */
    
    so I believe that the lengths should be determined at compile-time.
    
    [0]: https://lore.kernel.org/git/[e-mail]/

 pkt-line.c | 48 ++++++++++++++++++++++++++++++------------------
 1 file changed, 30 insertions(+), 18 deletions(-)

Comments

Jeff King June 23, 2020, 7:11 p.m. UTC | #1

On Tue, Jun 23, 2020 at 01:55:33PM -0400, Denton Liu wrote:

>  void packet_flush(int fd)
>  {
> -	packet_trace("0000", 4, 1);
> -	if (write_in_full(fd, "0000", 4) < 0)
> -		die_errno(_("unable to write flush packet"));
> +	control_packet_write(fd, "0000", _("unable to write flush packet"));
>  }

I like this kind of abstraction much more than what was going on in the
previous patch.

> +#define control_packet_write(fd, s, errstr) \
> +	do { \
> +		(void)s"is a string constant"; \

This is a neat trick. It might be nice to wrap in
BUILD_ASSERT_IS_STRING_LITERAL() or similar. (Though I wonder a bit if
we even really need to assert that; wouldn't it be OK to use this
function without it? We're using strlen(), after all, and not sizeof).

Would it also be useful to assert that the length of the control packet
is 4? And likewise that it's less than 4? That seems much more
interesting to me (as we'd be violating the protocol otherwise). And
that would be easy to do if the we passed the packet number as an
integer and formatted it ourselves.

But the result gets kind of ugly:

  void control_packet_write(int fd, unsigned packet_id, const char *errstr)
  {
	char buf[4 + 1];

	assert(packet_id < 4); /* >= 4 are actual data packets */
	vsnprintf(buf, sizeof(buf), "%04u", packet_id);
	if (write_in_full(fd, buf, 4))
		...;
  }

There are only 3 of these, and their whole point is to hide not just
this magic "4", but the whole idea of "this is what a control packet
looks like". I kind of wonder if the abstractions we need to reduce the
3 lines to 1 is really making anything more readable.

-Peff

Patch

diff --git a/pkt-line.c b/pkt-line.c
index 8f9bc68ee2..e29f427b19 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -81,49 +81,61 @@  static void packet_trace(const char *buf, unsigned int len, int write)
 	strbuf_release(&out);
 }
 
+static inline void packet_trace_str(const char *buf, int write)
+{
+	packet_trace(buf, strlen(buf), write);
+}
+
+#define control_packet_write(fd, s, errstr) \
+	do { \
+		(void)s"is a string constant"; \
+		packet_trace_str(s, 1); \
+		if (write_str_in_full((fd), s) < 0) \
+			die_errno((errstr)); \
+	} while (0)
+
 /*
  * If we buffered things up above (we don't, but we should),
  * we'd flush it here
  */
 void packet_flush(int fd)
 {
-	packet_trace("0000", 4, 1);
-	if (write_in_full(fd, "0000", 4) < 0)
-		die_errno(_("unable to write flush packet"));
+	control_packet_write(fd, "0000", _("unable to write flush packet"));
 }
 
 void packet_delim(int fd)
 {
-	packet_trace("0001", 4, 1);
-	if (write_in_full(fd, "0001", 4) < 0)
-		die_errno(_("unable to write delim packet"));
+	control_packet_write(fd, "0001", _("unable to write delim packet"));
 }
 
 void packet_response_end(int fd)
 {
-	packet_trace("0002", 4, 1);
-	if (write_in_full(fd, "0002", 4) < 0)
-		die_errno(_("unable to write stateless separator packet"));
+	control_packet_write(fd, "0002", _("unable to write stateless separator packet"));
 }
 
 int packet_flush_gently(int fd)
 {
-	packet_trace("0000", 4, 1);
-	if (write_in_full(fd, "0000", 4) < 0)
+	packet_trace_str("0000", 1);
+	if (write_str_in_full(fd, "0000") < 0)
 		return error(_("flush packet write failed"));
 	return 0;
 }
 
+#define control_packet_buf_write(buf, s) \
+	do { \
+		(void)s"is a string constant"; \
+		packet_trace_str(s, 1); \
+		strbuf_addstr((buf), s); \
+	} while (0)
+
 void packet_buf_flush(struct strbuf *buf)
 {
-	packet_trace("0000", 4, 1);
-	strbuf_add(buf, "0000", 4);
+	control_packet_buf_write(buf, "0000");
 }
 
 void packet_buf_delim(struct strbuf *buf)
 {
-	packet_trace("0001", 4, 1);
-	strbuf_add(buf, "0001", 4);
+	control_packet_buf_write(buf, "0001");
 }
 
 void set_packet_header(char *buf, int size)
@@ -337,15 +349,15 @@  enum packet_read_status packet_read_with_status(int fd, char **src_buffer,
 	if (len < 0) {
 		die(_("protocol error: bad line length character: %.4s"), linelen);
 	} else if (!len) {
-		packet_trace("0000", 4, 0);
+		packet_trace_str("0000", 0);
 		*pktlen = 0;
 		return PACKET_READ_FLUSH;
 	} else if (len == 1) {
-		packet_trace("0001", 4, 0);
+		packet_trace_str("0001", 0);
 		*pktlen = 0;
 		return PACKET_READ_DELIM;
 	} else if (len == 2) {
-		packet_trace("0002", 4, 0);
+		packet_trace_str("0002", 0);
 		*pktlen = 0;
 		return PACKET_READ_RESPONSE_END;
 	} else if (len < 4) {