Thanks for the detailed feedback.
On Wed, 22 Jul 2026 at 14:57:28 +0200, Oswald Buddenhagen wrote:
> On Sat, Jul 18, 2026 at 03:29:40PM +1000, Seth McDonald via isync-devel wrote:
> > The interface's implementation adds minor changes in behaviour to
> > simplify the authentication logic. This includes always attempting to
> > fall back to LOGIN when SASL fails and unifying similar error messages.
> >
> semantically, that seems just wrong. you need to work a lot harder to
> convince me that this is a wanted change.
The primary motivation is that the current "flow" of the SASL logic is
incompatible with the GNU SASL library. Many error messages simply do
not apply or cannot occur in the same order. So to ensure minimal
functional difference between the two implementations, I have to make
the current logic for Cyrus SASL more compatible with the new logic for
GNU SASL.
If I don't make these changes, then not only will the two
implementations be more functionally different than necessary, but it
becomes near impossible to have more fine-grained #ifdefs (as you
requested later in the message) due to the divergent logical flow. It's
really one or the other.
Regarding the falling back to LOGIN, it simplifies the logic from
unnecessarily complex "if it fails in this specific way, goto here; but
if it fails like this, goto there" to "if it fails, try LOGIN instead".
And as a user, I don't want the program to bother me about its troubles
if it can sort itself out by trying a different method. I don't care
about the method used as long as it's secure enough and it works.
> structurally, this cannot go into the same patch. do it before or after.
>
> have a look at the patches surrounding previous refactorings to get an idea
> of the things i like to separate from them, and how.
>
> i looked through the history for references, and d5a5da947 seems comparable
> in scope.
> it's clearly changing the code structure in addition to moving the code, but
> notably the visible behavior is not changed.
Thanks; I'll give it a good look.
> > +static const char *
> > +query_sasl_error( sasl_conn_t *ctx, int rc )
> >
> i don't want that obfuscation layer without good reason.
I added it for two reasons:
(0) It's simpler to have a single function to call to retrieve the
appropriate error message, rather than manually switching
between sasl_errdetail() and sasl_errstring() all the time.
(1) To encompass the following fallback.
> > + const char *err = ctx
> > + ? sasl_errdetail( ctx )
> > + : sasl_errstring( rc, NULL, NULL );
>
> > + return err ? err : "Unknown error";
> >
> what is that fallback for?
>From the documentation for sasl_errdetail() (see [0]):
| Returns the string describing the error that occurred, or NULL if no
| error has occurred, or there was an error retrieving it.
> > +static int
> > +process_sasl_step(
> > + } else {
> > + const char *err = query_sasl_error( session, rc );
> >
> i don't like these pointless temporaries.
I tend to use them for a few reasons:
(0) They make debugging line-by-line with GDB easier by keeping calls on
their own lines, which allows for easy stepping into/over select
calls. This is especially true for function calls inside the
conditions of if-statements.
(1) They make git diff/blame cleaner by separating operations with
newlines, which allows diff/blame to better identify and show
the exact operations that were changed.
(2) They tell you what exactly you're retrieving from the function,
which may not always be so obvious. For example, if a call
returns a status, does zero indicate success or failure? A line
like `if (!foo())` doesn't tell you much, but a line like
`int bad = !foo();` tells you immediately it means failure.
I honestly don't see why they shouldn't be used. There's no runtime
cost as they'll be optimised out by the compiler.
> > + intersect_mechs(
> > + common_mechs + 1,
> > + size - 1,
> > + any_mech ? server_mechs : client_mechs,
> > + server_mechs );
> >
> that call formatting is rarely used in isync, and looks weird even with the
> smaller tabs. don't use it without strong reason (long argument list
> starting beyond column ~60, or arguments with long comments).
For context, my personal rule is: if it's within 80 columns, format as
one line; otherwise, split the arguments as a separate line if they're
within 80 columns; otherwise, have each argument on a separate line.
(I like the 80-column limit as it makes side-by-side diffs a lot lot
easier to view.)
So using this case as an example, would you prefer a more compact
formatting like so:
intersect_mechs(
common_mechs + 1, size - 1,
any_mech ? server_mechs : client_mechs, server_mechs );
> > +typedef const char *(*imap_auth_cred_t)( void * );
> > +
> the consequences of having these are obviously ugly.
> just factor out imap_auth_conf_t as i hinted at previously.
Sure, but just FYI without it the logic will no longer be
self-contained. It'll depend on the implementation of the IMAP driver,
and thus won't be able to be understood or maintained purely as a
standalone unit (which was partly my goal).
> > + int force; /* Whether to allow use of LOGIN over an insecure channel. */
> > + int saslir; /* Whether the SASL-IR capability is available. */
> > + int secure; /* Whether the connection is over a secure channel. */
> >
> it's debatable whether these should be structure members. based on their
> lifetime, i'd expect function parameters.
Fair. I just had these as members cause I didn't want too many boolean
parameters.
> > +void imap_auth_new_client( imap_auth_client_t *client );
>
> also, the _client suffixes seem annoying.
> instead, the global functions should get _lib suffixes or something.
Sure, that works.
> > +/*
> > + * Formats a list of SASL mechanisms supported by both client and server
> > side.
> > + * The list is space-delimited, including starting and ending with
> > whitespace.
> > + * Mechanisms are parsed in a case-insensitive manner.
> > + */
> > +void imap_auth_filter_mechs(
> >
> this seems like an implementation detail that should not leak through the
> abstraction.
Not sure I entirely agree. The case-insensitive detail is definitely
relevant as the passed server side mechs are upper case, while the
passed client side mechs may be lower case. So the caller knows they
don't have to format the client side mechs before calling.
The space-delimited detail I agree is an implementation detail, but
should probably be mentioned at least somewhere since it's directly
relied upon for searching the list. I'll probably just move it to the
source file instead of the header.
> > +char *
> > +find_string_list( const string_list_t *list, const char *str )
> > +
> > +char *
> > +find_string_list_case( const string_list_t *list, const char *str )
>
> the return values make no sense. these should be bools, and the functions
> called contains_....
I was aiming for a function analogous to strstr(). It's purpose is to
find and, if found, return a string within a string list. And like
strstr(), it can be used secondarily to only check if the string list
contains the string by checking if the return value is NULL.
> related to other comments, i find it debatable that factoring these out
> actually makes sense. but if it does, it would have to happen in a separate
> patch.
So you're okay with patches that add general functions but don't include
their usage until later patches? Not saying that's bad; I just usually
include such usage as implicit justification for the patch.
> i think it would yield cleaner code to amend HAVE_LIBSASL with SASL_IS_GNU
> rather than having two mutually exclusive defines.
Agreed.
> your prior macos keychain patches should be integrated into this series to
> reduce overall churn. for that purpose, it's ok to either
> - during refactoring, leave code in its old place when it will be replaced
> in subsequent patches, or
> - to add code in a new file that will be logically created only in the
> subsequent refactoring (i think this is the case applicable here).
I'm afraid I don't see how integrating the keychain patches into this
patchset will reduce churn. The imap_auth_* interface doesn't touch nor
take responsibility for either the keychain code or the
password-querying logic as a whole. It calls (directly or indirectly)
ensure_password(), but doesn't care how that function works.
> finally, as i already pointed out in
> https://sourceforge.net/p/isync/mailman/message/59319314/ , i'm not a fan of
> verbosity. document only things that are not obvious, and check first
> whether they can't be made obvious instead.
Good to know; I'll revise the comments.
For (more) context about me, I like to explicitly document functions'
effects that are relied upon in the codebase. This is because when
analysing code, I find it very useful to know exactly what a function is
_supposed_ to do, and to contrast that with what it _actually_ does. I
don't assume everyone can infer all my intentions (including myself),
however obvious I think it to be.
Take care,
Seth McDonald.
[0]
<https://www.cyrusimap.org/sasl/sasl/reference/manpages/library/sasl_errdetail.html>
--
E9D1 26A5 F0D4 9DF7 792B C2E2 B4BF 4530 D39B 2D51
_______________________________________________
isync-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/isync-devel
