On Thu, 23 Jan 2025 13:14:30 +0100
Claudio Jeker <cjeker@diehard.n-r-g.com> wrote:
> On Thu, Jan 23, 2025 at 03:14:42PM +1030, Jack Burton wrote:
> > 1. in conn_parse_kvp(), initialised err to NULL.  Without that I was
> > seeing all sorts of bogus errors today (which didn't happen when I
> > first tested #3 a few weeks back ... most likely that was just dumb
> > luck the first time around, undone by random memory layout changes
> > from an unrelated commit last night).  
> 
> I sent a better fix for this to the list. I will probably rewrite the
> text_to_XYZ functions to return an enum and pass the value to write
> to as pointer. This way those functions will also be able to return
> the other possible values for NotUnderstood|Irrelevant|Reject.

Agreed.  Your fix is better for the time being ... and that enum will
come in handy for full negotiation.

...

> > 3. in GET_DIGEST_P included ",None" as a fallback (which I really
> > should have done in the first place).  
> 
> While correct right now I think in the future we may want to force
> CRC32C for header or data.

Yes, that may well make good sense (overhead is minimal and the standard
requires targets to support it) ... once we've implemented CRC32C of
course.  Happy to have a stab at adding support for CRC32C digests after
we have full negotiation working.

...

> More comments inline.

Thanks.  Have taken all your comments on board.  Revised diff follows.

All that's left in it is just replacing the two-shot conn_gen_kvp()
with a one-shot-per-key kvp_set_from_mine() and its associated macros,
then implementing "get in first" by adding the missing keys in
initiator_login_kvp(), categorised according to the rules in section 13
of RFC 7143.


cvs server: Diffing .
Index: connection.c
===================================================================
RCS file: /cvs/src/usr.sbin/iscsid/connection.c,v
diff -u -p -r1.24 connection.c
--- connection.c	22 Jan 2025 10:14:54 -0000	1.24
+++ connection.c	24 Jan 2025 00:25:56 -0000
@@ -335,41 +335,107 @@ log_debug("conn_parse_kvp: %s = %s", k->
 #undef SET_BOOL
 #undef SET_DIGEST
 
+#define GET_BOOL_P(dst, req, k, src, f)				\
+do {								\
+	if (f == 0 && !strcmp(req, #k)) {			\
+	(dst)->key = #k;					\
+	(dst)->value = ((src)->mine.k) ? "Yes" : "No";		\
+	f++;							\
+	}							\
+} while (0)
+
+#define GET_DIGEST_P(dst, req, k, src, f)				\
+do {									\
+	if (f == 0 && !strcmp(req, #k)) {				\
+		(dst)->key = #k;					\
+		(dst)->value =						\
+		    ((src)->mine.k == DIGEST_NONE) ? "None" : "CRC32C,None";\
+		f++;							\
+	}								\
+} while (0)
+
+#define GET_NUM_P(dst, req, k, src, f, e)				\
+do {									\
+	if (f == 0 && !strcmp(req, #k)) {				\
+		(dst)->key = #k;					\
+		if (asprintf(&((dst)->value), "%u", (src)->mine.k) == -1)\
+			e++;						\
+		else							\
+			(dst)->flags |= KVP_VALUE_ALLOCED;		\
+		f++;							\
+	}								\
+} while (0)
+
+#define GET_STR_C(dst, req, k, src, f)				\
+do {								\
+	if (f == 0 && !strcmp(req, #k)) {			\
+		(dst)->key = #k;				\
+		(dst)->value = (src)->config.k;			\
+		f++;						\
+	}							\
+} while (0)
+
+#define GET_STYPE_C(dst, req, k, src, f)				\
+do {									\
+	if (f == 0 && !strcmp(req, #k)) {				\
+		(dst)->key = #k;					\
+		(dst)->value = ((src)->config.k == SESSION_TYPE_DISCOVERY)\
+		    ? "Discovery" : "Normal";				\
+		f++;							\
+	}								\
+} while (0)
+
 int
-conn_gen_kvp(struct connection *c, struct kvp *kvp, size_t *nkvp)
+kvp_set_from_mine(struct kvp *kvp, const char *key, struct connection *c)
 {
-	struct session *s = c->session;
-	size_t i = 0;
+	int e = 0, f = 0;
+
+	if (kvp->flags & KVP_KEY_ALLOCED)
+		free(kvp->key);
+	kvp->key = NULL;
+	if (kvp->flags & KVP_VALUE_ALLOCED)
+		free(kvp->value);
+	kvp->value = NULL;
+	kvp->flags = 0;
 
-	if (s->mine.MaxConnections != iscsi_sess_defaults.MaxConnections) {
-		if (kvp && i < *nkvp) {
-			kvp[i].key = "MaxConnections";
-			if (asprintf(&kvp[i].value, "%hu",
-			    s->mine.MaxConnections) == -1) {
-				kvp[i].value = NULL;
-				return -1;
-			}
-			kvp[i].flags |= KVP_VALUE_ALLOCED;
-		}
-		i++;
+	/* XXX handle at least CHAP */
+	if (!strcmp(key, "AuthMethod")) {
+		kvp->key = "AuthMethod";
+		kvp->value = "None";
+		return 0;
 	}
-	if (c->mine.MaxRecvDataSegmentLength !=
-	    iscsi_conn_defaults.MaxRecvDataSegmentLength) {
-		if (kvp && i < *nkvp) {
-			kvp[i].key = "MaxRecvDataSegmentLength";
-			if (asprintf(&kvp[i].value, "%u",
-			    c->mine.MaxRecvDataSegmentLength) == -1) {
-				kvp[i].value = NULL;
-				return -1;
-			}
-			kvp[i].flags |= KVP_VALUE_ALLOCED;
-		}
-		i++;
+	GET_DIGEST_P(kvp, key, HeaderDigest, c, f);
+	GET_DIGEST_P(kvp, key, DataDigest, c, f);
+	GET_NUM_P(kvp, key, MaxConnections, c->session, f, e);
+	GET_STR_C(kvp, key, TargetName, c->session, f);
+	GET_STR_C(kvp, key, InitiatorName, c->session, f);
+	GET_BOOL_P(kvp, key, InitialR2T, c->session, f);
+	GET_BOOL_P(kvp, key, ImmediateData, c->session, f);
+	GET_NUM_P(kvp, key, MaxRecvDataSegmentLength, c, f, e);
+	GET_NUM_P(kvp, key, MaxBurstLength, c->session, f, e);
+	GET_NUM_P(kvp, key, FirstBurstLength, c->session, f, e);
+	GET_NUM_P(kvp, key, DefaultTime2Wait, c->session, f, e);
+	GET_NUM_P(kvp, key, DefaultTime2Retain, c->session, f, e);
+	GET_NUM_P(kvp, key, MaxOutstandingR2T, c->session, f, e);
+	GET_BOOL_P(kvp, key, DataPDUInOrder, c->session, f);
+	GET_BOOL_P(kvp, key, DataSequenceInOrder, c->session, f);
+	GET_NUM_P(kvp, key, ErrorRecoveryLevel, c->session, f, e);
+	GET_STYPE_C(kvp, key, SessionType, c->session, f);
+	/* XXX handle TaskReporting */
+
+	if (f == 0) {
+		errno = EINVAL;
+		return 1;
 	}
 
-	*nkvp = i;
-	return 0;
+	return e;
 }
+
+#undef GET_BOOL_P
+#undef GET_DIGEST_P
+#undef GET_NUM_P
+#undef GET_STR_C
+#undef GET_STYPE_C
 
 void
 conn_pdu_write(struct connection *c, struct pdu *p)
Index: initiator.c
===================================================================
RCS file: /cvs/src/usr.sbin/iscsid/initiator.c,v
diff -u -p -r1.20 initiator.c
--- initiator.c	22 Jan 2025 16:06:36 -0000	1.20
+++ initiator.c	24 Jan 2025 00:25:56 -0000
@@ -48,6 +48,7 @@ struct task_logout {
 	u_int8_t		 reason;
 };
 
+int		 conn_is_leading(struct connection *);
 struct kvp	*initiator_login_kvp(struct connection *, u_int8_t);
 struct pdu	*initiator_login_build(struct connection *,
 		    struct task_login *);
@@ -308,49 +309,68 @@ initiator_nop_in_imm(struct connection *
 	conn_task_issue(c, t);
 }
 
+int
+conn_is_leading(struct connection *c)
+{
+	return c == TAILQ_FIRST(&c->session->connections);
+}
+
+#define MINE_NOT_DEFAULT(c, k) ((c)->mine.k != iscsi_conn_defaults.k)
+
 struct kvp *
 initiator_login_kvp(struct connection *c, u_int8_t stage)
 {
-	struct kvp *kvp;
-	size_t nkvp;
+	struct kvp *kvp = NULL;
+	size_t i = 0, len;
+	const char *leading_only[] = {"MaxConnections", "InitialR2T",
+	    "ImmediateData", "MaxBurstLength", "FirstBurstLength",
+	    "DefaultTime2Wait", "DefaultTime2Retain", "MaxOutstandingR2T",
+	    "DataPDUInOrder", "DataSequenceInOrder", "ErrorRecoveryLevel",
+	    NULL};
+	const char *opneg_always[] = {"HeaderDigest", "DataDigest", NULL};
+	const char *secneg[] = {"SessionType", "InitiatorName", "TargetName",
+	    "AuthMethod", NULL};
+	const char **p;
 
 	switch (stage) {
 	case ISCSI_LOGIN_STG_SECNEG:
-		if (!(kvp = calloc(5, sizeof(*kvp))))
+		len = sizeof(secneg) / sizeof(*secneg);
+		if (!(kvp = calloc(len + 1, sizeof(*kvp))))
 			return NULL;
-		kvp[0].key = "AuthMethod";
-		kvp[0].value = "None";
-		kvp[1].key = "InitiatorName";
-		kvp[1].value = c->session->config.InitiatorName;
-
-		if (c->session->config.SessionType == SESSION_TYPE_DISCOVERY) {
-			kvp[2].key = "SessionType";
-			kvp[2].value = "Discovery";
-		} else {
-			kvp[2].key = "SessionType";
-			kvp[2].value = "Normal";
-			kvp[3].key = "TargetName";
-			kvp[3].value = c->session->config.TargetName;
-		}
+		for (p = secneg; *p != NULL; i++, p++)
+			if (kvp_set_from_mine(&kvp[i], *p, c))
+				goto fail;
 		break;
 	case ISCSI_LOGIN_STG_OPNEG:
-		if (conn_gen_kvp(c, NULL, &nkvp) == -1)
-			return NULL;
-		nkvp += 1; /* add slot for terminator */
-		if (!(kvp = calloc(nkvp, sizeof(*kvp))))
+		len = sizeof(opneg_always) / sizeof(*opneg_always);
+		if (conn_is_leading(c))
+			len += sizeof(leading_only) / sizeof(*leading_only);
+		if (MINE_NOT_DEFAULT(c, MaxRecvDataSegmentLength))
+			len++;
+		if (!(kvp = calloc(len + 1, sizeof(*kvp))))
 			return NULL;
-		if (conn_gen_kvp(c, kvp, &nkvp) == -1) {
-			kvp_free(kvp);
-			return NULL;
-		}
+		for (p = opneg_always; *p != NULL; i++, p++)
+			if (kvp_set_from_mine(&kvp[i], *p, c))
+				goto fail;
+		if (conn_is_leading(c))
+			for (p = leading_only; *p != NULL; i++, p++)
+				if (kvp_set_from_mine(&kvp[i], *p, c))
+					goto fail;
+		if (MINE_NOT_DEFAULT(c, MaxRecvDataSegmentLength) &&
+		    kvp_set_from_mine(&kvp[i], "MaxRecvDataSegmentLength", c))
+			goto fail;
 		break;
 	default:
 		log_warnx("initiator_login_kvp: exit stage left");
 		return NULL;
 	} 
 	return kvp;
+fail:
+	kvp_free(kvp);
+	return NULL;
 }
 
+#undef MINE_NOT_DEFAULT
 struct pdu *
 initiator_login_build(struct connection *c, struct task_login *tl)
 {
Index: iscsid.h
===================================================================
RCS file: /cvs/src/usr.sbin/iscsid/iscsid.h,v
diff -u -p -r1.23 iscsid.h
--- iscsid.h	22 Jan 2025 16:06:36 -0000	1.23
+++ iscsid.h	24 Jan 2025 00:25:56 -0000
@@ -367,7 +367,7 @@ void	conn_task_issue(struct connection *
 void	conn_task_schedule(struct connection *);
 void	conn_task_cleanup(struct connection *, struct task *);
 int	conn_parse_kvp(struct connection *, struct kvp *);
-int	conn_gen_kvp(struct connection *, struct kvp *, size_t *);
+int	kvp_set_from_mine(struct kvp *, const char *, struct connection *);
 void	conn_pdu_write(struct connection *, struct pdu *);
 void	conn_fail(struct connection *);
 void	conn_fsm(struct connection *, enum c_event);