On Wed, 22 Jan 2025 10:56:27 +0100
Claudio Jeker <cjeker@diehard.n-r-g.com> wrote:
> On Wed, Jan 22, 2025 at 05:25:33PM +1030, Jack Burton wrote:
> > On Tue, 21 Jan 2025 17:21:33 +0100
> > Claudio Jeker <cjeker@diehard.n-r-g.com> wrote:
...
> > > I cherry-picked the bits to set the HeaderDigest and DataDigest.
> > > I did change a few things, most prominently the param merge can't
> > > be a simple MIN().  I refactored text_to_digest() a bit.
> > > Also the setting of the digests was moved to connection_new() plus
> > > some other cleanup around the various params.
> > > 
> > > Please have a look if that works for you.  
> > 
> > That's a good start, but it's not enough to support lio targets.  
> 
> Yes, I only picked one bit out of your diff. I will also commit the
> diff below afterwards. It is another bit of the puzzle.

Okay great.  Thanks.  I'll wait to see the latest commits before I
propose anything else, so I can generate clean diffs again.

...

> > I agree that proposing defaults first every time is not ideal, but I
> > don't really see the point of implementing negotiation fully unless
> > we intend to expose a bunch more parameters to user-visible knobs,
> > most which I doubt would ever be needed anyway.  One of the
> > strengths of iscsi.conf(5) as it stands is its simplicity.  
> 
> Agreed. I think the one that may make sense as a userland knob is
> MaxConnections and even that one is questionable. OK maybe the
> timeouts (DefaultTime2Wait and DefaultTime2Retain) but that is for
> later anyway.

Yes, MaxConnections > 1 could be handy ... although once we have full
negotiation in place we could always just set our default
MaxConnections fairly high then just accept whatever lower value the
target wants if it rejects our default (but until we have full
negotiation, 1 is the only safe value to propose).

I'd suggest adding chap auth to that list (and that of course will need
a config file directive).  Although I'm not likely to use it here (I
prefer dedicated links for iscsi), there are probably a bunch of
targets in the wild that might need it.

...

> > I think the "get in first proposing everything we're allowed to"
> > approach is at least a good start.  It is sufficient to enable using
> > lio targets, with the subset of iscsi features we support today.
> > 
> > On the other hand, if/when we add support for e.g. crc32c digests or
> > chap auth (the only two extra knobs to twiddle which might be
> > useful in some circumstances) then we'll need real negotiation to
> > work.  
> 
> Yes, when I hacked up iscsid I tried to dodge that bullet but at one
> point we need to implement proper negotiation.

Agreed.


> > So if you see adding either of those as desirable sometime soon,
> > then yes it's probably worth implementing negotiation fully
> > (generating appropriate responses to a target's proposals [which we
> > get many of because we don't get in first to propose most params]
> > and gracefully handling a target's rejections of our proposals
> > [which right now can only happen if the target requires auth]) now,
> > so that it's in place before we start implementing more of the
> > standard.
> > 
> > Otherwise, "get in first" is likely to keep things much simpler.
> > 
> > What do you think?  I don't mind having a go at full negotiation if
> > that's the approach you prefer.  Only reason I avoided it to begin
> > with was that there didn't seem much point adding the extra
> > complexity when our current desired end state is all-defaults
> > anyway (except for MaxRecvDataSegmentLength, but that's declarative
> > so negotiation doesn't come into play).  
> 
> 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.

Hopefully that should provide enough for others to test with a broader
range of targets than you & I have available, while we work on improving
negotiation ... and those test reports should give us a better idea of
which of other bits of the standard are worth doing next.


> I also want to look at the kvp api and make that one better. I
> dislike the macros but have not come up with a better solution yet.

Yes, they're not pretty.  But then again a big cascade of strcmps would
be even less pretty.  I couldn't think of a better approach either,
which is why I did GET_* in a very similar style (and to a lesser
extent also MINE_NOT_DEFAULT).  Also, without macro stringification we
may need to go back to allocating (almost) all kvp keys on the heap,
reversing the presumption made last week (although I don't see that as
a big issue: they're tiny and it only affects text/login pdus, not data
pdus).