Use a new funtion to instead of outing error message for field checking
Subsystems (2)
PATCH 2/2 Use a new funtion to instead of outing error message for field checking
Hello Steve,
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.
--- lib/Makefile.am | 2 +- lib/errormsg.h | 58 ++++++++++++++++++++++ lib/libaudit.c | 26 ++++++++++ src/auditctl.c | 135 ++++------------------------------------------------ src/mt/Makefile.am | 4 +- 5 files changed, 97 insertions(+), 128 deletions(-) create mode 100644 lib/errormsg.h
lib/Makefile.am +1 -1
@@ -30,7 +30,7 @@ lib_LTLIBRARIES = libaudit.la include_HEADERS = libaudit.h libaudit_la_SOURCES = libaudit.c message.c netlink.c \ lookup_table.c audit_logging.c deprecated.c \ - private.h $(BUILT_SOURCES) + private.h errormsg.h $(BUILT_SOURCES) libaudit_la_LIBADD = libaudit_la_DEPENDENCIES = $(libaudit_la_SOURCES) ../config.h libaudit_la_LDFLAGS = -Wl,-z,relro
lib/errormsg.h new file +58 -0
58 lines not on this page · raw message
lib/libaudit.c +26 -0
@@ -39,6 +39,7 @@#include "libaudit.h" #include "private.h" +#include "errormsg.h" /* #defines for the audit failure query */ #define CONFIG_FILE "/etc/libaudit.conf"
@@ -1153,3 +1154,28 @@ int audit_detect_machine(void)return -1 ; }hidden_def (audit_detect_machine) + +void audit_number_to_errmsg(int errnumber, const char *opt) +{ +unsigned int i; + +for (i =0 ; i <sizeof (err_msgtab)/sizeof (struct msg_tab ); i++) { +if (err_msgtab[i].key == errnumber) { +switch (err_msgtab[i].position) + { +case 0 : +fprintf (stderr,"%s\n" , err_msgtab[i].cvalue); +break ; +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 ; + } + } +}
src/auditctl.c +9 -126
142 lines not on this page · raw message
src/mt/Makefile.am +3 -1
@@ -43,7 +43,7 @@ lib_OBJECTS = $(libauditmt_a_OBJECTS) libaudit.h: cp ${top_srcdir}/lib/libaudit.h . -libaudit.c: libaudit.h private.h +libaudit.c: libaudit.h private.h errormsg.h cp ${top_srcdir}/lib/libaudit.c . message.c: libaudit.h cp ${top_srcdir}/lib/message.c .
@@ -89,6 +89,8 @@ optabs.h: cp ${top_builddir}/lib/optabs.h . errtabs.h: cp ${top_builddir}/lib/errtabs.h . +errormsg.h: + cp ${top_builddir}/lib/errormsg.h . lookup_table.o: ${top_builddir}/config.h gen_tables.h i386_tables.h \ ia64_tables.h ppc_tables.h s390_tables.h s390x_tables.h \
Zhang Xiliang píše v Čt 07. 08. 2008 v 18:58 +0800:
Quoted 3 lines
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.
lib/errormsg.h line 8
Quoted 11 lines
lib/errormsg.h new file +8
-0@@ -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.
lib/errormsg.h line 35
Quoted 5 lines
lib/errormsg.h new file +5
-0+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.)
Quoted 1 line
lib/errormsg.h new file +1
-0+void audit_number_to_errmsg (int errnumber,const char *opt)
<SNIP>
lib/errormsg.h
Quoted 4 lines
lib/errormsg.h new file +4
-0+{ +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.)
lib/errormsg.h
Quoted 11 lines
lib/errormsg.h new file +11
-0+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
Quoted 10 lines · On Thursday 07 August 2008 11:27:25 Miloslav Trmač wrote:
Quoted 4 lines
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.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?
Not that I know of...but that doesn't mean that someone somewhere isn't selling an app that does.
Quoted 1 line
Even if this patch is accepted (and it does improve the code),
I can't take a patch like this right now. It changes the API. I would however take a patch that just moves things in auditctl.c. Big API changes need to wait for 1 or 2 more releases and then we can restructure things.
Quoted 5 lines
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 ofsrc/auditctl.cinto src/errormsg.* and do not add it to libaudit at all.
I think that errors originating in libaudit should probably have text string in it that explains the errors. But we have to wait to change the API another release or two. We also have to make sure that we don't introduce text relocations as we add strings in libraries.
Thanks,
-Steve
Steve Grubb píše v Čt 07. 08. 2008 v 13:45 -0400:
Quoted 3 lines
Quoted 1 line
Even if this patch is accepted (and it does improve the code),
I can't take a patch like this right now. It changes the API.
FWIW, it only adds a function to libaudit, it doesn't change anything.
Mirek