Re: Compile error in current CVS, src/server.c:5500

[Derek Robert Price]

Brian Murphy wrote:

> Derek Robert Price wrote:
>
> > What exactly is the "error 0" you cite?  I'd rather CVS not start 
> > outputting arbitrary error numbers without good reason.  Also, if 
> > pam_strerror is called on the output of pam_end, I don't think 
> > printing the actual error number matters much.  It shouldn't matter 
> > to an end user and likely not even to us with a good text message.  
> > Then again, and please feel free to correct me on this one, maybe we 
> > should leave the error number in there through the experimental 
> > stage, until we discover that the text messages in error reports are 
> > satisfying?
>
> From the linux source the only other code returned from pam_end other 
> than PAM_SUCCESS is PAM_SYSTEM_ERR which gives the message "System 
> error" which doesn't explain a lot.


Still, as long as the PAM folks left themselves a way to improve their 
error messages in their API, we should use it and give them the chance.  
Perhaps actually outputting the retval for the user to see is pointless, 
but I think I like it, at least until we're out of the experimental 
stage.  Partly because I'm not very familiar with PAM, of course.

> Here is a patch fixing the nesting problem.


I think you resent your previous patch.  I added a comment I missed the 
first time through anyhow.  Also, please don't forget your ChangeLog entry.

> Index: src/server.c
> ===================================================================
> RCS file: /cvs/ccvs/src/server.c,v
> retrieving revision 1.301
> diff -u -r1.301 server.c
> --- src/server.c        20 Jul 2003 16:38:55 -0000      1.301
> +++ src/server.c        21 Jul 2003 18:54:54 -0000
> @@ -5483,7 +5483,7 @@
>     char *username, *password;
> {
>     pam_handle_t *pamh = NULL;
> -    int retval;
> +    int retval, err;
>     struct cvs_pam_userinfo ui = { username, password };
>     struct pam_conv conv = { cvs_pam_conv, (void *)&ui };
>
> @@ -5491,14 +5491,19 @@
>
>     if (retval == PAM_SUCCESS)
>         retval = pam_authenticate(pamh, 0);
> +    else
> +       printf("E PAM Error: %s\n", pam_strerror(NULL, retval));
>
>     if (retval == PAM_SUCCESS)
>         retval = pam_acct_mgmt(pamh, 0);
> +    else
> +       printf("E PAM Error: %s\n", pam_strerror(pamh, retval));
>
> -    if (pam_end(pamh,retval) != PAM_SUCCESS)
> +    if ((err = pam_end(pamh, retval)) != PAM_SUCCESS)

There's nothing wrong with retval = pam_end(pamh, retval), assuming 
you've already checked the previous retval and printed the relevant 
error messages.  There's no need for `err' here.

>     {
> -       printf("E Fatal error, aborting.\n
> -               pam failed to release authenticator\n");
> +       printf("E Fatal error, aborting.\n\
> +error 0 pam failed to release authenticator\n\
> +PAM error number %d: %s\n", err, pam_strerror(NULL, err));
>         error_exit ();
>     }
>  

Derek

--
               *8^)

Email: derek@ximbiot.com

Get CVS support at <http://ximbiot.com>!
--
It is error alone which needs the support of government.  Truth can stand by 
itself.
                        - Thomas Jefferson