Download raw body.
[patch 3 of 3] iscsid(8): support a wider variety of iscsi targets
[patch 3 of 3] iscsid(8): support a wider variety of iscsi targets
[patch 3 of 3] iscsid(8): support a wider variety of iscsi targets
On Thu, Jan 23, 2025 at 03:14:42PM +1030, Jack Burton wrote:
> On Wed, 22 Jan 2025 22:23:39 +1030
> Jack Burton <jack@saosce.com.au> wrote:
> > On Wed, 22 Jan 2025 10:56:27 +0100
> > Claudio Jeker <cjeker@diehard.n-r-g.com> wrote:
> > > Right now I think we should get something in that allows to connect
> > > to more targets and then see how we can improve the negotiation.
> >
> > Sounds good. Once the next round of commits are in I'll put together
> > a revised (and now should be much smaller) version of that.
>
> Done: see diff below. Still longer than I would have liked, but at
> least not as big as my original #3. I can't really see how to split the
> rest of this bit up sensibly any further.
>
> This diff is essentially my original #3 rebased on the current tree,
> except for these 3 further changes:
>
> 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.
> 2. no longer allocating kvp keys at all, anywhere (as discussed on the
> list a few days ago). I've left the ability to do so in place, as
> there's a reasonable chance we'll need it again for full negotiation
> (if the kvp api survives until then). In kvp_set_from_mine() I'm still
> allocing kvp values only in GET_NUM_P. This could easily be changed to
> allocing all kvp values everywhere if you really want to get rid of the
> kvp flags member (but that probably belongs as a separate patch after
> this one).
I need to check that part of the diff closer.
> 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.
> With this diff applied, iscsid(8) can establish sessions to LIO targets
> successfully. Also tested against IET (which has worked since a few
> commits ago) and netbsd-iscsi-target from ports (which has always
> worked); can't see any regressions there.
>
> Yes it's a bit ugly in places, but we'll probably end up throwing
> some of those bits away when we implement real negotiation and at least
> with this in place we support LIO targets (probably one of the most
> common today) and most likely some others that didn't work before too.
>
> Would appreciate test reports against other targets from anyone who has
> access to them.
More comments inline.
> 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 23 Jan 2025 03:59:18 -0000
> @@ -300,7 +300,7 @@ conn_parse_kvp(struct connection *c, str
> {
> struct kvp *k;
> struct session *s = c->session;
> - const char *err;
> + const char *err = NULL;
> int errors = 0;
>
>
> @@ -335,41 +335,106 @@ 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)
> -{
> - struct session *s = c->session;
> - size_t i = 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++;
> +kvp_set_from_mine(struct kvp *kvp, const char *key, struct connection *c) {
Per style please put the { on a new line.
> + int e = 0, f = 0;
> +
> + if ((kvp->flags & KVP_KEY_ALLOCED))
No need for extra ().
> + free(kvp->key);
> + kvp->key = NULL;
> + if ((kvp->flags & KVP_VALUE_ALLOCED))
Same as above
> + free(kvp->value);
> + kvp->value = NULL;
> + kvp->flags = 0;
> +
> + /* 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 23 Jan 2025 03:59:18 -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,48 +309,73 @@ initiator_nop_in_imm(struct connection *
> conn_task_issue(c, t);
> }
>
> +int
> +conn_is_leading(struct connection *c)
> +{
> + struct connection *lead;
> +
> + if (c == NULL || c->session == NULL
> + || (lead = TAILQ_FIRST(&c->session->connections)) == NULL)
Please move the || to the prvious line.
But to be honest I think you can drop most of that.
c can not be NULL unless there is a big bug, c->session can also not be NULL.
Not sure if the TAILQ can be empty. That's something one could keep.
So you could write this function just as:
int
conn_is_leading(struct connection *c)
{
return c == TAILQ_FIRST(&c->session->connections);
}
> + return 0;
> + return (c == lead) ? 1 : 0;
> +}
> +
> +#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))))
> - return NULL;
> - if (conn_gen_kvp(c, kvp, &nkvp) == -1) {
> - kvp_free(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;
> - }
> + 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 23 Jan 2025 03:59:18 -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);
> Index: session.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/iscsid/session.c,v
> diff -u -p -r1.13 session.c
> --- session.c 22 Jan 2025 16:06:36 -0000 1.13
> +++ session.c 23 Jan 2025 03:59:18 -0000
> @@ -103,6 +103,10 @@ session_config(struct session *s, struct
> fatal("strdup");
> } else
> s->config.InitiatorName = default_initiator_name();
> + if (!s->config.HeaderDigest)
> + s->config.HeaderDigest = DIGEST_NONE;
> + if (!s->config.DataDigest)
> + s->config.DataDigest = DIGEST_NONE;
This part is not needed, this is handled in connection_new() where the
default value is inherited from the initator and then the session config
is used if the value is != 0.
> }
>
> void
>
--
:wq Claudio
[patch 3 of 3] iscsid(8): support a wider variety of iscsi targets
[patch 3 of 3] iscsid(8): support a wider variety of iscsi targets
[patch 3 of 3] iscsid(8): support a wider variety of iscsi targets