From mboxrd@z Thu Jan 1 00:00:00 1970 From: Miloslav =?UTF-8?Q?Trma=C4=8D?= Subject: Re: [PATCH 2/2] Use a new funtion to instead of outing error message for field checking Date: Thu, 07 Aug 2008 17:27:25 +0200 Message-ID: <1218122846.5618.99.camel@amilo> References: <489AD555.2080500@cn.fujitsu.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Return-path: In-Reply-To: <489AD555.2080500@cn.fujitsu.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: linux-audit-bounces@redhat.com Errors-To: linux-audit-bounces@redhat.com To: Zhang Xiliang Cc: Linux Audit List-Id: linux-audit@redhat.com Zhang Xiliang p=C3=AD=C5=A1e v =C4=8Ct 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[] =3D { 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) > +{ > + 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