Re: [libvirt] [PATCH v3] sasl: Fix authentication when using PLAIN mechanism

On 11/22/2013 10:26 AM, Christophe Fergeau wrote: > With some authentication mechanism (PLAIN for example), sasl_client_start() > can return SASL_OK, which translates to virNetSASLSessionClientStart() > returning VIR_NET_SASL_COMPLETE. > cyrus-sasl documentation is a bit vague as to what to do in such situation, > but upstream clarified this a bit in > http://asg.andrew.cmu.edu/archive/message.php?mailbox=archive.cyrus-sasl&... > > When we got VIR_NET_SASL_COMPLETE after virNetSASLSessionClientStart() and > if the remote also tells us that authentication is complete, then we should > end the authentication procedure rather than forcing a call to > virNetSASLSessionClientStep(). Without this patch, when trying to use SASL > PLAIN, I get: > error :authentication failed : Failed to step SASL negotiation: -1 > (SASL(-1): generic failure: Unable to find a callback: 32775) > > This patch is based on a spice-gtk patch by Dietmar Maurer. > --- > Change since v2: > - move the added test out of the for(;;) loop > /* Loop-the-loop... > - * Even if the server has completed, the client must *always* do at least one step > - * in this loop to verify the server isn't lying about something. Mutual auth */ > + * Even if the server has completed, the client must loop until sasl_client_start() or > + * sasl_client_step() return SASL_OK to verify the server isn't lying > + * about something. Mutual auth > + * */ > for (;;) { > + This blank line seems spurious > restep: now that you aren't modifying the head of the loop, you could follow my earlier suggestion of dropping the 'restep' label and replacing 'goto restep' with 'continue'. But that's trivial, so I don't care either way, and don't need to see a v4 if you choose to change before pushing. ACK.