Re: [RFC: PATCH] Audit Failure Query Functionality

[Lisa Smith <lisa.m.smith@hp.com>]

Tim,

>> +       /* Find the audit failure action tunable in the config file */
>> +       ret = search_audituser_conf(file, AUDIT_FAIL_KEYWORD, &nv);
>> +
>> +       if (ret == -1) {
>> +               audit_msg(LOG_WARNING, "Error in %s", file);
>>     
>
> Would you not want to use LOG_ERR here?  Or if you really did intend a
> LOG_WARNING using the word "Error" in your message might be confusing. 
>   

Good catch.  I've changed this to LOG_ERR.

>> +               return ERR;
>>     
> This is really awkward to me.  I think it makes more sense to make this
> function simply return success or failure and update a parameter passed
> to it by the user with the correct failure mode.  As it stands now, ERR
> isn't really a failure mode.  May I also suggest making your enumeration
> a little more descriptive?
>   

How about on any error IGNORE is simply returned, and the ERR mode is
eliminated?  There will still be an audit log message, so the error does
not vanish.  This means the caller will always have one of the three
failure modes to handle.

What did you have in mind to make the enumeration more descriptive?

>> +
>> +       /* Open the file for line by line reading*/
>> +       fp = fopen(file, "r");
>> +       if (fp == NULL) {
>> +               audit_msg(LOG_ERR, "Error - fdopen failed for %s (%s)",
>> +                       file, strerror(errno));
>> +               return -1;
>> +        }
>>     
>
> Why return -1, why not return errno?
>   

The caller only cares that the file could not be opened - it doesn't
matter why.  If we passes errno back, nothing would be done with that
value anyway.  The errno will be posted in the log, however, if an admin
has a need to debug a problem.

> [..]
>   
>> +
>> +       while ((bytesread = getline(&buf, &len, fp)) != -1) {
>> +
>> +               if (buf[0] == '#') {
>> +                       lineno++;
>> +                       continue;       // Ignore comments
>> +               }
>>     
>
> So will this blow up if buf[0] == ' ' ?
>   

No.  getline() will retrieve a new line from a file until there are no
more lines in the file.  After all lines have been read, getline() will
return -1 and drop out of the while loop.

> [..]
>   
>> +
>> +               /* Convert line into name-value pair */
>> +               rc = nv_split(buf, nv);
>> +               if (rc == 1) {
>> +                       audit_msg(LOG_ERR, "Error on line %d in %s", lineno,
>> +                               file);
>> +                       lineno++;
>> +                       continue;
>> +                }
>> +
>> +               /* Find the name-value pair */
>> +               if (strcmp(nv->name, keyword) == 0)
>> +               {
>> +                       fclose(fp);
>> +                       if (buf)
>>     
>
> Superfluous conditional.
>   

Fixed, thanks.

>> +       return 1;
>> +}
>> +
>> +/*
>> + *  This function parses a line looking for a keyword = value pair
>> + *  and if found, returns it in the nv structure. If the function
>> + *  returns success, the calling function is expected to free
>> + *  nv->name and nv->value.
>> + */
>> +int nv_split(char *buffer, struct nv_pair *nv)
>> +{
>> +       /* Get the name part */
>> +       char *saveptr, *ptr = NULL;
>> +       char *buf = strdup(buffer);
>> +
>> +       /* Look for = in buf */
>> +       nv->name = NULL;
>> +       nv->value = NULL;
>> +       ptr = strtok_r(buf, " =", &saveptr);
>> +       if ((ptr == NULL) || !(strcmp(ptr,"\n"))) {
>>     
>
> You can kill that strcmp, right?
>
> if (!ptr || *ptr == '\n') {
>   

Yup, corrected.

>
> You should just write a little helper routine called something like:
> 	
> 	free_nv(struct nv_pair *nv);
>
> It's obvious what it does and it consolidates some code and makes things
> look cleaner. 
>   

Great idea; will do.

Thanks for your comments.

Lisa