best practice for error handling between PAM_SUCCESS and PAM_AUTH_ERR macros

Question

We have two exit code code PAM_SUCCESS(0), PAM_AUTH_ERR(7).

If we wanna return 0 or 7 when we compared password with our password in following code which code is better that this code ?

if (strcmp (expected_password, crypt (password, expected_password)) == 0)
    retval = PAM_SUCCESS;
else
    retval = PAM_AUTH_ERR;

But we want to decrease our lines of code to one or two lines!

  retval = -!!strcmp (expected_password, crypt (password,
                      expected_password)) & PAM_AUTH_ERR;

I think this code is bad cause we have many character on one line and using bitwise operators is bad practice. We can break this line in following format :

  retval = -!!strcmp (expected_password, crypt (password, expected_password));
  retval &= PAM_AUTH_ERR;

Also I think this code is bad cause we can refactor it on previous code :))

Actually i don't know what is the best way for refactoring this code.

We also can use following codes :

retval = strcmp (expected_password, crypt (password,
                   expected_password)) == 0 ? PAM_SUCCESS : PAM_AUTH_ERR;

This code is very long and dirty! we can refactor it in previous codes.

Also we can use this pattern :

retval = strcmp (expected_password, crypt (password, expected_password)) == 0;
retval = retval ? PAM_SUCCESS : PAM_AUTH_ERR;

But on second line we repeated retval word though we can use another codes!

Which is the best code and why ?

Answer 1

None of the above seems right to me except the first one with one exception:

if (strcmp (expected_password, crypt (password, expected_password)) == 0)
    retval = PAM_SUCCESS;
else
    retval = PAM_AUTH_ERR;

The above example assumes that retval is not initialized which is a bad practice. I suggest initializing retval to PAM_SUCCESS because otherwise there's no good logic to pick a default value to determine the reason of the failure (I mean there can be multiple reasons why the authentication process can fail). So I suggest the below example which read fine, is easy to review.

retval = PAM_SUCCESS;

if (strcmp (expected_password, crypt (password, expected_password)))
    retval = PAM_AUTH_ERR;

return retval;

The following example is hard to read and unnecessarily complex. AVOID IT!

  retval = -!!strcmp (expected_password, crypt (password,
                      expected_password)) & PAM_AUTH_ERR;

The next example is practically the same, but assign the retval a second value which is also unnecessary and hard to read and review.

  retval = -!!strcmp (expected_password, crypt (password, expected_password));
  retval &= PAM_AUTH_ERR;

The next example below is a little better but again, misses the opportunity to set a default value for retval which I personally prefer to have.

retval = strcmp (expected_password, crypt (password,
                   expected_password)) == 0 ? PAM_SUCCESS : PAM_AUTH_ERR;

and finally the last example below, is bad practice because it keeps two different values into retval (the strcmp return value and then the function's return value) which is confusing and thus a bad practice.

retval = strcmp (expected_password, crypt (password, expected_password)) == 0;
retval = retval ? PAM_SUCCESS : PAM_AUTH_ERR;

Answer 2

In days of yore, we were taught to draw flow-charts, and then write pseudo-code... later we were taught structured programming (such as Jackson Design, Mascot or SSADM)

The pseudo-code for your snippet is:

If password is correct THEN
   status is PAM_SUCCESS
ELSE
   status is PAM_AUTH_ERR
ENDIF

Therefore your code should be

if ( strcmp(expected_password, crypt( password, expected_password ) ) == 0 )
{
    retval = PAM_SUCCESS;
}
else
{
    retval = PAM_AUTH_ERR;
}

No ifs, no buts... anything else reduces the clarity of what you are doing. Don't try and double guess what your compiler might do, either.

---

Hopefully, you would not write your pseudo-code as

status is PAM_SUCCESS
If password is incorrect THEN
   status is PAM_AUTH_ERR
ENDIF

This is especially unclear if the variable declaration or initialisation is not immediately above the condition... it gets lost in the other code.

As an aside, many moons back, I broke some assembler, when a register was assumed to have a value 500 lines after it was loaded... and I stuck some code in between that broke the assumption...

Answer 3

My personal preference, if you're going to be using strcmp for equality, is to wrap it in a function streql that returns true (1) for equal and false (0) for not equal. Then, use that function, streql, and forget about the 3-way strcmp (which you rarely need anyway - the 3-way-ness - unless you're in specific code traversing a search tree). Then your code is simpler and more obviously correct.

And in the end, for statements this simple, you should just rely on the compiler to generate good tight code while you concentrate on basic correctness and readability and not try to worry about how many characters you're typing.

Answer 4

Two moderately long lines:

int compare = strcmp(...);
retval = (compare == 0 ? PAM_THIS : PAM_THAT);

Using the same variable to store the result of strcmp is baaaaad. You don't know if the types are the same. retval could be char, and compare returns 256 which is then stored as 0.

If you wrote code with !! or with bit operations, and I had to review it, I wouldn't even bother reading it and figuring out what it does, it would be straight rejected. So good that you don't consider this.

Answer 5

As a general rule of thumb, simpler code is better. trying to cut corners when it comes to lines of codes generally makes sense when you're trying to satisfy a project which has metrics in place for source lines of code or function lines of code.