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

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

[Zhang Xiliang]

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.

Signed-off-by: Zhang Xiliang <zhangxiliang@cn.fujitsu.com>
---
 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

diff --git a/lib/Makefile.am b/lib/Makefile.am
index 13ccbb9..c5b2c6c 100644
--- a/lib/Makefile.am
+++ b/lib/Makefile.am
@@ -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
diff --git a/lib/errormsg.h b/lib/errormsg.h
new file mode 100644
index 0000000..6ee68d1
--- /dev/null
+++ b/lib/errormsg.h
@@ -0,0 +1,58 @@
+/* errormsg.h --
+ * Copyright 2008 FUJITSU Inc.
+ * All Rights Reserved.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ *
+ * Authors:
+ *      Zhang Xiliang <zhangxiliang@cn.fujitsu.com>
+ */
+
+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
+     */
+    int position;
+    const char	*cvalue;
+};
+
+static const struct msg_tab err_msgtab[] = {
+    { -1,    2,    "-F missing opration for" },
+    { -2,    2,    "-F unknown field:" },
+    { -3,    1,    "must be before -S" },
+    { -4,    1,    "machine type not found" },
+    { -5,    1,    "elf mapping not found" },
+    { -6,    1,    "requested bit level not supported by machine" },
+    { -7,    1,    "can only be used with exit filter list" },
+    { -8,    2,    "-F unknown message type -" },
+    { -9,    0,    "msgtype field can only be used with exclude filter list" },
+    { -10,    0,    "Failed upgrading rule" },
+    { -11,    0,    "String value too long" },
+    { -12,    0,    "Only msgtype field can be used with exclude filter" },
+    { -13,    1,    "only takes = or != operators" },
+    { -14,    0,    "Permission can only contain  \'rwxa\'" },
+    { -15,    2,    "-F unknown errno -"},
+    { -16,    2,    "-F unknown file type - " },
+    { -17,    1,    "can only be used with exit and entry filter list" },
+    { -18,    1,    "can not be used with exclude filter list" },
+    { -19,    0,    "Key field needs a watch or syscall given prior to it" },
+    { -20,    2,    "-F missing value after opration for" },
+    { -21,    2,    "-F value should be number for" },
+    { -22,    2,    "-F missing field name before operator for" }
+};
diff --git a/lib/libaudit.c b/lib/libaudit.c
index e0f108a..7d48d78 100644
--- a/lib/libaudit.c
+++ b/lib/libaudit.c
@@ -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;
+		}
+	}
+}
diff --git a/src/auditctl.c b/src/auditctl.c
index 6144795..96aebe7 100644
--- a/src/auditctl.c
+++ b/src/auditctl.c
@@ -733,133 +733,16 @@ static int setopt(int count, char *vars[])
 		}
 		if (which == NEW) 
 			rc = audit_rule_fieldpair_data(&rule_new,optarg,flags);
-//FIXME: make this a function
-		switch (rc)
-		{
-			case 0:
-				if (which == NEW && rule_new->fields[rule_new->field_count-1] ==
-							AUDIT_PERM)
-					audit_permadded = 1;
-				break;
-			case -1:
-				fprintf(stderr, "-F missing operator for %s\n", 
-					optarg);
-				retval = -1;
-				break;
-			case -2:
-				fprintf(stderr, "-F unknown field: %s\n", 
-					optarg);
-				retval = -1;
-				break;
-			case -3:
-				fprintf(stderr, 
-					"-F %s must be before -S\n", 
-					optarg);
-				retval = -1;
-				break;
-			case -4:
-				fprintf(stderr, 
-					"-F %s machine type not found\n", 
-					optarg);
-				retval = -1;
-				break;
-			case -5:
-				fprintf(stderr, 
-					"-F %s elf mapping not found\n", 
-					optarg);
-				retval = -1;
-				break;
-			case -6:
-				fprintf(stderr, 
-			"-F %s requested bit level not supported by machine\n", 
-					optarg);
-				retval = -1;
-				break;
-			case -7:
-				fprintf(stderr,
-			 "Field %s can only be used with exit filter list\n",
-					 optarg);
-				retval = -1;
-				break;
-			case -8:
-				fprintf(stderr, 
-					"-F unknown message type - %s\n",
-					 optarg);
-				retval = -1;
-				break;
-			case -9:
-				fprintf(stderr,
-		 "msgtype field can only be used with exclude filter list\n");
-				retval = -1;
-				break;
-			case -10:
-				fprintf(stderr,
-					"Failed upgrading rule\n");
-				retval = -1;
-			case -11:
-				fprintf(stderr,
-					"String value too long\n");
-				retval = -1;
-				break;
-			case -12:
-				fprintf(stderr,
-			"Only msgtype field can be used with exclude filter\n");
-				retval = -1;
-				break;
-			case -13:
-				fprintf(stderr,
-			"Field (%s) only takes = or != operators\n", optarg);
-				retval = -1;
-				break;
-			case -14:
-				fprintf(stderr,
-				"Permission (%s) can only contain \'rwxa\n",
-					optarg);
-				retval = -1;
-				break;
-			case -15:
-				fprintf(stderr, 
-					"-F unknown errno - %s\n", optarg);
-				retval = -1;
-				break;
-			case -16:
-				fprintf(stderr, 
-					"-F unknown file type - %s\n", optarg);
-				retval = -1;
-				break;
-			case -17:
-				fprintf(stderr,
-					"Field %s can only be used with exit and entry filter list\n", optarg);
-				retval = -1;
-				break;
-			case -18:
-				fprintf(stderr,
-					"Field %s can not be used with exclude filter list\n", optarg);
-				retval = -1;
-				break;
-			case -19:
-				fprintf(stderr,
-					"Key field needs a watch or syscall given prior to it\n");
-				retval = -1;
-				break;
-			case -20:
-				fprintf(stderr,
-					"-F missing value after operator for %s\n", optarg);
-				retval = -1;
-				break;
-			case -21:
-				fprintf(stderr,
-					"-F value should be a number for %s\n", optarg);
-				retval = -1;
-				break;
-			case -22:
-				fprintf(stderr,
-					"-F missing field name before operator for %s\n", optarg);
-				retval = -1;
-			default:
-				retval = -1;
-				break;
+
+		if (rc != 0) {
+			audit_number_to_errmsg(rc, optarg);
+			retval = -1;
+		} else {
+			if (which == NEW && rule_new->fields[rule_new->field_count-1] ==
+						AUDIT_PERM)
+				audit_permadded = 1;
 		}
+
 		break;
         case 'm':
 		if (audit_log_user_message( fd, AUDIT_USER, optarg, NULL, 
diff --git a/src/mt/Makefile.am b/src/mt/Makefile.am
index e840287..7581225 100644
--- a/src/mt/Makefile.am
+++ b/src/mt/Makefile.am
@@ -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 \

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

[Miloslav Trmač]

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

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

[Steve Grubb]

On Thursday 07 August 2008 11:27:25 Miloslav Trmač wrote:
> > 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. 


> 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.

> 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.

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

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

[Miloslav Trmač]

Steve Grubb píše v Čt 07. 08. 2008 v 13:45 -0400:
> > 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