common/read_peer_msg: deconstruct into individual helper routines.

The One Big API is confusing, and has enough corner cases that we should
ditch it rather than add more.

See: https://www.sandimetz.com/blog/2016/1/20/the-wrong-abstraction

In particular, when openingd is changed to chat to peers even when
it's not actively opening a channel, it wants to handle (most) errors
by continuing, not calling peer_failed().

This exposes the constituent parts.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

common/peer_failed.c

@@ -36,8 +36,12 @@ void peer_failed_received_errmsg(int peer_fd, int gossip_fd,
 				 const char *desc,
 				 const struct channel_id *channel_id)
 {
-	u8 *msg = towire_status_peer_error(NULL, channel_id,
+	static const struct channel_id all_channels;
-					   desc, cs, NULL);
+	u8 *msg;
+
+	if (!channel_id)
+		channel_id = &all_channels;
+	msg = towire_status_peer_error(NULL, channel_id, desc, cs, NULL);
 	peer_billboard(true, "Received error from peer: %s", desc);
 	status_send_fatal(take(msg), peer_fd, gossip_fd);
 }

common/peer_failed.h

@@ -21,7 +21,8 @@ void peer_failed_(int peer_fd, int gossip_fd,
 		  const char *fmt, ...)
 	PRINTF_FMT(5,6) NORETURN;
 
-/* We're failing because peer sent us an error message */
+/* We're failing because peer sent us an error message: NULL
+ * channel_id means all channels. */
 void peer_failed_received_errmsg(int peer_fd, int gossip_fd,
 				 struct crypto_state *cs,
 				 const char *desc,

common/read_peer_msg.c

@@ -11,6 +11,77 @@
 #include <wire/peer_wire.h>
 #include <wire/wire_sync.h>
 
+u8 *peer_or_gossip_sync_read(const tal_t *ctx,
+			     int peer_fd, int gossip_fd,
+			     struct crypto_state *cs,
+			     bool *from_gossipd)
+{
+	fd_set readfds;
+	u8 *msg;
+
+	FD_ZERO(&readfds);
+	FD_SET(peer_fd, &readfds);
+	FD_SET(gossip_fd, &readfds);
+
+	select(peer_fd > gossip_fd ? peer_fd + 1 : gossip_fd + 1,
+	       &readfds, NULL, NULL, NULL);
+
+	if (FD_ISSET(gossip_fd, &readfds)) {
+		msg = wire_sync_read(ctx, gossip_fd);
+		if (!msg)
+			status_failed(STATUS_FAIL_GOSSIP_IO,
+				      "Error reading gossip msg: %s",
+				      strerror(errno));
+		*from_gossipd = true;
+		return msg;
+	}
+
+	msg = sync_crypto_read(ctx, cs, peer_fd);
+	if (!msg)
+		peer_failed_connection_lost();
+	*from_gossipd = false;
+	return msg;
+}
+
+bool is_peer_error(const tal_t *ctx, const u8 *msg,
+		   const struct channel_id *channel_id,
+		   char **desc, bool *all_channels)
+{
+	struct channel_id err_chanid;
+
+	if (fromwire_peektype(msg) != WIRE_ERROR)
+		return false;
+
+	*desc = sanitize_error(ctx, msg, &err_chanid);
+
+	/* BOLT #1:
+	 *
+	 * The channel is referred to by `channel_id`, unless `channel_id` is
+	 * 0 (i.e. all bytes are 0), in which case it refers to all channels.
+	 * ...
+	 * The receiving node:
+	 *   - upon receiving `error`:
+	 *    - MUST fail the channel referred to by the error message, if that
+	 *      channel is with the sending node.
+	 *  - if no existing channel is referred to by the message:
+	 *    - MUST ignore the message.
+	 */
+	*all_channels = channel_id_is_all(&err_chanid);
+	if (!*all_channels && !channel_id_eq(&err_chanid, channel_id))
+		*desc = tal_free(*desc);
+
+	return true;
+}
+
+bool is_wrong_channel(const u8 *msg, const struct channel_id *expected,
+		      struct channel_id *actual)
+{
+	if (!extract_channel_id(msg, actual))
+		return false;
+
+	return !channel_id_eq(expected, actual);
+}
+
 void handle_gossip_msg_(const u8 *msg TAKES, int peer_fd,
 			struct crypto_state *cs,
 			bool (*send_msg)(struct crypto_state *cs, int fd,
@@ -42,6 +113,53 @@ void handle_gossip_msg_(const u8 *msg TAKES, int peer_fd,
 		tal_free(msg);
 }
 
+bool handle_peer_gossip_or_error(int peer_fd, int gossip_fd,
+				 struct crypto_state *cs,
+				 const struct channel_id *channel_id,
+				 const u8 *msg TAKES)
+{
+	char *err;
+	bool all_channels;
+	struct channel_id actual;
+
+	if (is_msg_for_gossipd(msg)) {
+		wire_sync_write(gossip_fd, msg);
+		/* wire_sync_write takes, so don't take again. */
+		return true;
+	}
+
+	if (is_peer_error(tmpctx, msg, channel_id, &err, &all_channels)) {
+		if (err)
+			peer_failed_received_errmsg(peer_fd, gossip_fd,
+						    cs, err,
+						    all_channels
+						    ? NULL : channel_id);
+
+		/* Ignore unknown channel errors. */
+		goto handled;
+	}
+
+	/* They're talking about a different channel? */
+	if (is_wrong_channel(msg, channel_id, &actual)) {
+		status_trace("Rejecting %s for unknown channel_id %s",
+			     wire_type_name(fromwire_peektype(msg)),
+			     type_to_string(tmpctx, struct channel_id, &actual));
+		if (!sync_crypto_write(cs, peer_fd,
+				take(towire_errorfmt(NULL, &actual,
+						     "Multiple channels"
+						     " unsupported"))))
+			peer_failed_connection_lost();
+		goto handled;
+	}
+
+	return false;
+
+handled:
+	if (taken(msg))
+		tal_free(msg);
+	return true;
+}
+
 u8 *read_peer_msg_(const tal_t *ctx,
 		   int peer_fd, int gossip_fd,
 		   struct crypto_state *cs,
@@ -51,73 +169,49 @@ u8 *read_peer_msg_(const tal_t *ctx,
 		   void *arg)
 {
 	u8 *msg;
-	struct channel_id chanid;
+	bool from_gossipd, all_channels;
-	fd_set readfds;
+	struct channel_id actual;
+	char *err;
 
-	FD_ZERO(&readfds);
+	if (gossip_fd > 0) {
-	FD_SET(peer_fd, &readfds);
+		msg = peer_or_gossip_sync_read(ctx, peer_fd, gossip_fd,
-	if (gossip_fd >= 0)
+					       cs, &from_gossipd);
-		FD_SET(gossip_fd, &readfds);
+	} else {
-
+		msg = sync_crypto_read(ctx, cs, peer_fd);
-	select(peer_fd > gossip_fd ? peer_fd + 1 : gossip_fd + 1,
-	       &readfds, NULL, NULL, NULL);
-
-	if (gossip_fd >= 0 && FD_ISSET(gossip_fd, &readfds)) {
-		msg = wire_sync_read(NULL, gossip_fd);
 		if (!msg)
-			status_failed(STATUS_FAIL_GOSSIP_IO,
+			peer_failed_connection_lost();
-				      "Error reading gossip msg: %s",
+		from_gossipd = false;
-				      strerror(errno));
+	}
 
-		handle_gossip_msg_(msg, peer_fd, cs, send_reply, arg);
+	if (from_gossipd) {
+		handle_gossip_msg_(take(msg), peer_fd, cs, send_reply, arg);
 		return NULL;
 	}
 
-	msg = sync_crypto_read(ctx, cs, peer_fd);
-	if (!msg)
-		peer_failed_connection_lost();
-
 	if (is_msg_for_gossipd(msg)) {
 		/* Forward to gossip daemon */
 		wire_sync_write(gossip_fd, take(msg));
 		return NULL;
 	}
 
-	if (fromwire_peektype(msg) == WIRE_ERROR) {
+	if (is_peer_error(tmpctx, msg, channel, &err, &all_channels)) {
-		char *err = sanitize_error(msg, msg, &chanid);
+		if (err)
-
-		/* BOLT #1:
-		 *
-		 * The channel is referred to by `channel_id`, unless
-		 * `channel_id` is 0 (i.e. all bytes are 0), in which
-		 * case it refers to all channels.
-		 * ...
-
-		 * The receiving node:
-		 *   - upon receiving `error`:
-		 *    - MUST fail the channel referred to by the error
-		 *       message, if that channel is with the sending node.
-		 *  - if no existing channel is referred to by the
-		 *    message:
-		 *    - MUST ignore the message.
-		 */
-		if (channel_id_eq(&chanid, channel)
-		    || channel_id_is_all(&chanid)) {
 			peer_failed_received_errmsg(peer_fd, gossip_fd,
-						    cs, err, &chanid);
+						    cs, err,
-		}
+						    all_channels
+						    ? NULL : channel);
 
+		/* Ignore unknown channel errors. */
 		return tal_free(msg);
 	}
 
 	/* They're talking about a different channel? */
-	if (extract_channel_id(msg, &chanid)
+	if (is_wrong_channel(msg, channel, &actual)) {
-	    && !channel_id_eq(&chanid, channel)) {
 		status_trace("Rejecting %s for unknown channel_id %s",
 			     wire_type_name(fromwire_peektype(msg)),
-			     type_to_string(tmpctx, struct channel_id, &chanid));
+			     type_to_string(tmpctx, struct channel_id, &actual));
 		if (!send_reply(cs, peer_fd,
-				take(towire_errorfmt(NULL, &chanid,
+				take(towire_errorfmt(NULL, &actual,
 						     "Multiple channels"
 						     " unsupported")),
 				arg))

common/read_peer_msg.h

@@ -8,6 +8,69 @@
 struct crypto_state;
 struct channel_id;
 
+/**
+ * peer_or_gossip_sync_read - read a peer message, or maybe a gossip msg.
+ * @ctx: context to allocate return packet from.
+ * @peer_fd, @gossip_fd: peer and gossip fd.
+ * @cs: the cryptostate (updated)
+ * @from_gossipd: true if the msg was from gossipd, otherwise false.
+ *
+ * Will call peer_failed_connection_lost() or
+ * status_failed(STATUS_FAIL_GOSSIP_IO) or return a message.
+ *
+ * Usually, you should call handle_gossip_msg if *@from_gossipd is
+ * true, otherwise if is_peer_error() handle the error, otherwise if
+ * is_msg_for_gossipd() then send to gossipd, otherwise if is
+ * is_wrong_channel() send that as a reply.  Otherwise it should be
+ * a valid message.
+ */
+u8 *peer_or_gossip_sync_read(const tal_t *ctx,
+			     int peer_fd, int gossip_fd,
+			     struct crypto_state *cs,
+			     bool *from_gossipd);
+
+/**
+ * is_peer_error - if it's an error, describe if it applies to this channel.
+ * @ctx: context to allocate return from.
+ * @msg: the peer message.
+ * @channel_id: the channel id of the current channel.
+ * @desc: set to non-NULL if this describes a channel we care about.
+ * @all_channels: set to true if this applies to all channels.
+ *
+ * If @desc is NULL, ignore this message.  Otherwise, that's usually passed
+ * to peer_failed_received_errmsg().
+ */
+bool is_peer_error(const tal_t *ctx, const u8 *msg,
+		   const struct channel_id *channel_id,
+		   char **desc, bool *all_channels);
+
+/**
+ * is_wrong_channel - if it's a message about a different channel, return true
+ * @msg: the peer message.
+ * @channel_id: the channel id of the current channel.
+ * @actual: set to the actual channel id if this returns false.
+ *
+ * Note that this only handles some message types, returning false for others.
+ */
+bool is_wrong_channel(const u8 *msg, const struct channel_id *expected,
+		      struct channel_id *actual);
+
+
+/**
+ * handle_peer_gossip_or_error - simple handler for all the above cases.
+ * @peer_fd, @gossip_fd: peer and gossip fd.
+ * @cs: the cryptostate (updated)
+ * @msg: the peer message (only taken if returns true).
+ *
+ * This returns true if it handled the packet: a gossip packet (forwarded
+ * to gossipd), an error packet (causes peer_failed_received_errmsg or
+ * ignored), or a message about the wrong channel (sends sync error reply).
+ */
+bool handle_peer_gossip_or_error(int peer_fd, int gossip_fd,
+				 struct crypto_state *cs,
+				 const struct channel_id *channel_id,
+				 const u8 *msg TAKES);
+
 /**
  * read_peer_msg - read & decode in a peer message, handling common ones.
  * @ctx: context to allocate return packet from.