Re: [PATCH 2/2] Use a new funtion to instead of outing error message for field checking

["Miloslav Trmač" <mitr@redhat.com>]

Zhang Xiliang píše v Čt 07. 08. 2008 v 18:58 +0800:
> The method of outing error message for field checking is too big. It is disadvantage to modify.
> Create a helper function to output error messages.
> It should be more pretty and smart.
(Disclaimer: I'm not the audit maintainer.)

I think the current system of error reporting is rather suboptimal.  New
error types must be added in two separate places (and if you forget, no
error message will be printed) and the error message cannot contain
additional information from the function reporting the error (e.g.
strerror(errno), or anything else - see the "position" hack in your
patch).

Ideally, the failing function should return a newly allocated string
returning the error message instead of a negative number; OTOH that
means changing the libaudit API and I'm not sure that's desirable now.

The whole part of libaudit that deals with audit rules seems to be only
usable by auditctl - after all, all the error codes added by recent
patches are not handled by any other application that might be using the
function.  Are there any external applications that use
audit_rule_fieldpair_data(), for example?

Even if this patch is accepted (and it does improve the code), I think
long-term it would be good not to enshrine the current error reporting
system - at minimum it should be very clearly documented
audit_number_to_errmsg() is not a long-term API and applications other
than auditctl should not use it.  Or perhaps only move the code out of
src/auditctl.c into src/errormsg.* and do not add it to libaudit at all.


> --- /dev/null
> +++ b/lib/errormsg.h
> @@ -0,0 +1,58 @@
> +struct msg_tab {
> +    int key; /* error number */
> +    /*
> +     * the field string position in the error message
> +     * 0: don't output field string
> +     * 1: output field string before error message
> +     * 2: output field string after error message
> +     */
Use an enum instead of hard-coded values, please.
> +    int position;
> +    const char	*cvalue;
> +};
> +
> +static const struct msg_tab err_msgtab[] = {
This might use the lookup_table.c mechanism to avoid 21 data relocations
- at the cost of using two separate tables, one mapping "key"s to
strings and one mapping "key"s to "position"s.  I don't know whether
it's worth it.  (Another advantage of returning strings from the
function directly - data relocations are not necessary for the strings.)

> +void audit_number_to_errmsg(int errnumber, const char *opt)
<SNIP>
> +{
> +				case 0:
> +					fprintf(stderr, "%s\n", err_msgtab[i].cvalue);
> +					break;
If this is supposed to be a long-term maintained API, the function
should return an allocated string (and let the caller report it in any
way it wants - to stderr, syslog, a log file...) instead.

(If audit_number_to_errmsg were a function in src/auditctl.c, using
fprintf would be perfectly OK.)

> +				case 1:
> +					fprintf(stderr, "%s %s\n", opt, err_msgtab[i].cvalue);
> +					break;
> +				case 2:
> +					fprintf(stderr, "%s %s\n", err_msgtab[i].cvalue, opt);
> +					break;
> +				default:
> +					break;
> +			}
> +			return;
> +		}
fprintf(stderr, "unknown error %d", errnumber); here

	Mirek