threads / patch / 29151

patch, 3 partsuse constants for sideband communication channels

Subject: [PATCH 0/3] use constants for sideband communication channels

## tl;dr

5 messages between Dec 13, 2011 and Dec 14, 2011. Diffs are folded; open one to read it.

replies: 4people: 2as markdown or json

iheffner@gmail.com· Dec 13, 2011, 18:28 UTC · lore

In order to make more clear how the different channels in sidechannel.c are to be used, I'm proposing some macros/constants which can be used in place of the "magic numbers" that mean little or nothing to someone not familiar with the protocol.

In working on a script to handle archiving from a bare repository a project which contained submodules, I had a difficult time determining how to talk between my git-upload-archive replacement and git-archive. It did not occur to me to look at documentation for git-send-pack or git-receive-pack when trying to understand the communication protocol. But looking through the code and poking at an implementation, I finally understood that I needed to send a channel identifier to say what type of communication I was sending. I determined that there were three possible channels. I could easily tell that channel three (3) was for catastrophic errors on the server side. But it was not clear what the difference between channel 1 and channel 2 were. Because channel 2 was the one that appeared to be read and parsed in some manner, I assumed that this was the "important" data channel and tried sending my data on that channel.

I was confused and frustrated when it appeared that git-archive treated data coming from my --exec'd git-upload-archive replacement differently than it did when receiving data from the real git-upload-archive. Eventually I figured out that channel 1 was for data, channel 2 was for non-fatal messages, and channel 3 was for fatal error communications. Having comments in sidechannel.h would have helped. But constants or macros would have helped as well and makes the code that uses them more clear.

Ivan
 [PATCH 1/3] add constants for sideband communication channels
 [PATCH 2/3] switch sideband communication to use constants
 [PATCH 3/3] use SIDEBAND_*_ERROR constants in pack protocol
 builtin/fetch-pack.c     |    2 +-
 builtin/receive-pack.c   |    6 +++---
 builtin/send-pack.c      |    4 ++--
 builtin/upload-archive.c |    6 +++---
 sideband.c               |    6 +++---
 sideband.h               |    6 +++++-
 upload-pack.c            |   22 +++++++++++-----------
 7 files changed, 28 insertions(+), 24 deletions(-)
iheffner@gmail.com· Dec 13, 2011, 18:28 UTC · re: iheffner@gmail.com · lore

[PATCH 1/3] add constants for sideband communication channels

From: Ivan Heffner <iheffner@gmail.com>
Add constants for sideband channel identifiers.
* SIDEBAND_CHAN_PACK     => 1
* SIDEBAND_CHAN_PROGRESS => 2
* SIDEBAND_CHAN_ERROR    => 3
Signed-off-by: Ivan Heffner <iheffner@gmail.com>
---
 sideband.h |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)
Show changes to sideband.h +5 −1
diff --git a/sideband.h b/sideband.h
index d72db35..2954979 100644
--- a/sideband.h
+++ b/sideband.h
@@ -2,7 +2,11 @@
 #define SIDEBAND_H
 
 #define SIDEBAND_PROTOCOL_ERROR -2
-#define SIDEBAND_REMOTE_ERROR -1
+#define SIDEBAND_REMOTE_ERROR   -1
+#define SIDEBAND_CHAN_PACK       1
+#define SIDEBAND_CHAN_PROGRESS   2
+#define SIDEBAND_CHAN_ERROR      3
+
 
 #define DEFAULT_PACKET_MAX 1000
 #define LARGE_PACKET_MAX 65520
-- 
1.7.6.553.g917d7.dirty
iheffner@gmail.com· Dec 13, 2011, 18:28 UTC · re: iheffner@gmail.com · lore

[PATCH 2/3] switch sideband communication to use constants

From: Ivan Heffner <iheffner@gmail.com>

Found all uses of magic numbers for sideband channel indicators and changed them to use the new SIDEBAND_CHAN_* constants.

Signed-off-by: Ivan Heffner <iheffner@gmail.com>
---
 builtin/receive-pack.c   |    6 +++---
 builtin/upload-archive.c |    6 +++---
 sideband.c               |    6 +++---
 upload-pack.c            |   22 +++++++++++-----------
 4 files changed, 20 insertions(+), 20 deletions(-)
Show changes to 4 files +20 −20

builtin/receive-pack.c, builtin/upload-archive.c, sideband.c, upload-pack.c

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 7ec68a1..43f7a55 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -179,7 +179,7 @@ static void report_message(const char *prefix, const char *err, va_list params)
 	msg[sz++] = '\n';
 
 	if (use_sideband)
-		send_sideband(1, 2, msg, sz, use_sideband);
+		send_sideband(1, SIDEBAND_CHAN_PROGRESS, msg, sz, use_sideband);
 	else
 		xwrite(2, msg, sz);
 }
@@ -207,7 +207,7 @@ static int copy_to_sideband(int in, int out, void *arg)
 		ssize_t sz = xread(in, data, sizeof(data));
 		if (sz <= 0)
 			break;
-		send_sideband(1, 2, data, sz, use_sideband);
+		send_sideband(1, SIDEBAND_CHAN_PROGRESS, data, sz, use_sideband);
 	}
 	close(in);
 	return 0;
@@ -851,7 +851,7 @@ static void report(struct command *commands, const char *unpack_status)
 	packet_buf_flush(&buf);
 
 	if (use_sideband)
-		send_sideband(1, 1, buf.buf, buf.len, use_sideband);
+		send_sideband(1, SIDEBAND_CHAN_PACK, buf.buf, buf.len, use_sideband);
 	else
 		safe_write(1, buf.buf, buf.len);
 	strbuf_release(&buf);
diff --git a/builtin/upload-archive.c b/builtin/upload-archive.c
index 2d0b383..4ac7984 100644
--- a/builtin/upload-archive.c
+++ b/builtin/upload-archive.c
@@ -77,7 +77,7 @@ static void error_clnt(const char *fmt, ...)
 	va_start(params, fmt);
 	len = vsprintf(buf, fmt, params);
 	va_end(params);
-	send_sideband(1, 3, buf, len, LARGE_PACKET_MAX);
+	send_sideband(1, SIDEBAND_CHAN_ERROR, buf, len, LARGE_PACKET_MAX);
 	die("sent error to the client: %s", buf);
 }
 
@@ -149,11 +149,11 @@ int cmd_upload_archive(int argc, const char **argv, const char *prefix)
 		}
 		if (pfd[1].revents & POLLIN)
 			/* Status stream ready */
-			if (process_input(pfd[1].fd, 2))
+			if (process_input(pfd[1].fd, SIDEBAND_CHAN_PROGRESS))
 				continue;
 		if (pfd[0].revents & POLLIN)
 			/* Data stream ready */
-			if (process_input(pfd[0].fd, 1))
+			if (process_input(pfd[0].fd, SIDEBAND_CHAN_PACK))
 				continue;
 
 		if (waitpid(writer, &status, 0) < 0)
diff --git a/sideband.c b/sideband.c
index d5ffa1c..4b4199a 100644
--- a/sideband.c
+++ b/sideband.c
@@ -47,12 +47,12 @@ int recv_sideband(const char *me, int in_stream, int out)
 		band = buf[pf] & 0xff;
 		len--;
 		switch (band) {
-		case 3:
+		case SIDEBAND_CHAN_ERROR:
 			buf[pf] = ' ';
 			buf[pf+1+len] = '\0';
 			fprintf(stderr, "%s\n", buf);
 			return SIDEBAND_REMOTE_ERROR;
-		case 2:
+		case SIDEBAND_CHAN_PROGRESS:
 			buf[pf] = ' ';
 			do {
 				char *b = buf;
@@ -107,7 +107,7 @@ int recv_sideband(const char *me, int in_stream, int out)
 				memmove(buf + pf+1, b + brk, len);
 			} while (len);
 			continue;
-		case 1:
+		case SIDEBAND_CHAN_PACK:
 			safe_write(out, buf + pf+1, len);
 			continue;
 		default:
diff --git a/upload-pack.c b/upload-pack.c
index 470cffd..98c2542 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -56,19 +56,19 @@ static int strip(char *line, int len)
 	return len;
 }
 
-static ssize_t send_client_data(int fd, const char *data, ssize_t sz)
+static ssize_t send_client_data(int chan, const char *data, ssize_t sz)
 {
 	if (use_sideband)
-		return send_sideband(1, fd, data, sz, use_sideband);
-	if (fd == 3)
+		return send_sideband(1, chan, data, sz, use_sideband);
+	if (chan == SIDEBAND_CHAN_ERROR)
 		/* emergency quit */
-		fd = 2;
-	if (fd == 2) {
+		chan = SIDEBAND_CHAN_PROGRESS;
+	if (chan == SIDEBAND_CHAN_PROGRESS) {
 		/* XXX: are we happy to lose stuff here? */
-		xwrite(fd, data, sz);
+		xwrite(chan, data, sz);
 		return sz;
 	}
-	return safe_write(fd, data, sz);
+	return safe_write(chan, data, sz);
 }
 
 static FILE *pack_pipe = NULL;
@@ -243,7 +243,7 @@ static void create_pack_file(void)
 			sz = xread(pack_objects.err, progress,
 				  sizeof(progress));
 			if (0 < sz)
-				send_client_data(2, progress, sz);
+				send_client_data(SIDEBAND_CHAN_PROGRESS, progress, sz);
 			else if (sz == 0) {
 				close(pack_objects.err);
 				pack_objects.err = -1;
@@ -286,7 +286,7 @@ static void create_pack_file(void)
 			}
 			else
 				buffered = -1;
-			sz = send_client_data(1, data, sz);
+			sz = send_client_data(SIDEBAND_CHAN_PACK, data, sz);
 			if (sz < 0)
 				goto fail;
 		}
@@ -302,7 +302,7 @@ static void create_pack_file(void)
 	/* flush the data */
 	if (0 <= buffered) {
 		data[0] = buffered;
-		sz = send_client_data(1, data, 1);
+		sz = send_client_data(SIDEBAND_CHAN_PACK, data, 1);
 		if (sz < 0)
 			goto fail;
 		fprintf(stderr, "flushed.\n");
@@ -312,7 +312,7 @@ static void create_pack_file(void)
 	return;
 
  fail:
-	send_client_data(3, abort_msg, sizeof(abort_msg));
+	send_client_data(SIDEBAND_CHAN_ERROR, abort_msg, sizeof(abort_msg));
 	die("git upload-pack: %s", abort_msg);
 }
 
-- 
1.7.6.553.g917d7.dirty
iheffner@gmail.com· Dec 13, 2011, 18:28 UTC · re: iheffner@gmail.com · lore

[PATCH 3/3] use SIDEBAND_*_ERROR constants in pack protocol

From: Ivan Heffner <iheffner@gmail.com>

Switched calls to send_sideband() for pack protocol errors to use SIDEBAND_PROTOCOL_ERROR and SIDEBAND_REMOTE_ERROR in place of the sideband magic numbers of -2 and -1, respectively.

Signed-off-by: Ivan Heffner <iheffner@gmail.com>
---
 builtin/fetch-pack.c |    2 +-
 builtin/send-pack.c  |    4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)
Show changes to 2 files +3 −3

builtin/fetch-pack.c, builtin/send-pack.c

diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index c6bc8eb..63e9ac4 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -245,7 +245,7 @@ static enum ack_type get_ack(int fd, unsigned char *result_sha1)
 static void send_request(int fd, struct strbuf *buf)
 {
 	if (args.stateless_rpc) {
-		send_sideband(fd, -1, buf->buf, buf->len, LARGE_PACKET_MAX);
+		send_sideband(fd, SIDEBAND_REMOTE_ERROR, buf->buf, buf->len, LARGE_PACKET_MAX);
 		packet_flush(fd);
 	} else
 		safe_write(fd, buf->buf, buf->len);
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index e0b8030..67c9fe5 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -96,7 +96,7 @@ static int pack_objects(int fd, struct ref *refs, struct extra_have_objects *ext
 			ssize_t n = xread(po.out, buf, LARGE_PACKET_MAX);
 			if (n <= 0)
 				break;
-			send_sideband(fd, -1, buf, n, LARGE_PACKET_MAX);
+			send_sideband(fd, SIDEBAND_REMOTE_ERROR, buf, n, LARGE_PACKET_MAX);
 		}
 		free(buf);
 		close(po.out);
@@ -320,7 +320,7 @@ int send_pack(struct send_pack_args *args,
 	if (args->stateless_rpc) {
 		if (!args->dry_run && cmds_sent) {
 			packet_buf_flush(&req_buf);
-			send_sideband(out, -1, req_buf.buf, req_buf.len, LARGE_PACKET_MAX);
+			send_sideband(out, SIDEBAND_REMOTE_ERROR, req_buf.buf, req_buf.len, LARGE_PACKET_MAX);
 		}
 	} else {
 		safe_write(out, req_buf.buf, req_buf.len);
-- 
1.7.6.553.g917d7.dirty
Junio C Hamano· Dec 14, 2011, 04:56 UTC · re: iheffner@gmail.com · lore

Re: [PATCH 0/3] use constants for sideband communication channels

iheffner@gmail.com writes:
> In order to make more clear how the different channels in sidechannel.c
> are to be used, I'm proposing some macros/constants which can be used in
> place of the "magic numbers" that mean little or nothing to someone not
> familiar with the protocol.

I am not fundamentally opposed to the stated goal, but the posted patches make the resulting code way too wide for comfortable reading. Can we use a bit shorter symbols?

Perhaps a good way to start would be to first refrain from using these symbols, but give a prominent comment near the API functions that are used to send and receive sideband data to explain which band is used for what purpose, which should be enough for people who are writing the code to link with these functions.

← back to recent threads