On Sun, 26 Jan 2025 13:03:21 +0100
Claudio Jeker <cjeker@diehard.n-r-g.com> wrote:
> On Fri, Jan 24, 2025 at 11:21:23AM +1030, Jack Burton wrote:
> > On Thu, 23 Jan 2025 13:14:30 +0100
> > Claudio Jeker <cjeker@diehard.n-r-g.com> wrote:  
> > > 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.  
>  
> So I finally setup a lio target and was able to see the same problems.
> We get stuck because of the HeaderDigest and DataDigest fields.
> Now your diff below blows up on discovery sessions since TargetName is
> NULL. Fixing that is not hard but also a bit ugly.

Well caught!  Yes, sorry, discovery sessions were the one thing I
hadn't been testing.

Although RFC 7143 does allow for TargetName (optionally) to be sent in
discovery sessions, our iscsictl(8) does not support named discovery
sessions (and I don't see any real benefit in doing so), so I've stuck
with the all-discovery-sessions-are-unnamed approach already in place.

The slightly revised diff below now works for discovery sessions too.
I figured that a separate list for discovery would be slightly less ugly
than special-casing TargetName.

> I try to spend some time on this on monday to see if I can make this
> better.

No worries.  If you can come up with a better way I'm all ears.


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	28 Jan 2025 02:10:14 -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	28 Jan 2025 02:10:14 -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,76 @@ 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 *discovery[] = {"SessionType", "InitiatorName",
+	    "AuthMethod", NULL};
+	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, **q;
 
 	switch (stage) {
 	case ISCSI_LOGIN_STG_SECNEG:
-		if (!(kvp = calloc(5, 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";
+			len = sizeof(discovery) / sizeof(*discovery);
+			q = discovery;
 		} else {
-			kvp[2].key = "SessionType";
-			kvp[2].value = "Normal";
-			kvp[3].key = "TargetName";
-			kvp[3].value = c->session->config.TargetName;
+			len = sizeof(secneg) / sizeof(*secneg);
+			q = secneg;
 		}
+		if (!(kvp = calloc(len + 1, sizeof(*kvp))))
+			return NULL;
+		for (p = q; *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	28 Jan 2025 02:10:14 -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);