This is a preliminary patch set as the root Makefile changes are not yet
correct.
Summary
Typically, we don't care about error messages or names in the kernel because
userspace will manage that. But sometimes we need to output an error number
to printks and that creates a situation where a user, system admistrator or
developer must find an error number reference to figure out what went wrong
with a particular driver or whatever. This patch adds two alternatives at
increasing memory costs:
1. print the number in addition to the name for 2k extra or
2. print the number, name and description for 6k extra.
This is fairly low cost for the person who wants to make life just a little
bit easier. The format of each is respectively the same as the following:
printk("%d (%s)", err, err_name);
printk("%d (%s: %s)", err, err_name, err_desc);
Theory
Error messages aren't printed often, so this data and code is designed to be
compact at the expense of speed. Rather than using an array of strings that
would require both the text and a pointer to that text, we just cram a range
of error names or descriptions into a single string with null character
delimiters. When we want to retrieve a string, we just iterate through that
string and count nulls. This is slow, but it keeps it compact. (If this
becomes a bottleneck then something else is seriously wrong! :)
This is a simple bash script that parses our errno*.h files and formats
them into the error_strings.h header that our strerror and strerror_name
functions will use later.
First it looks at $ARCH and examines the errno.h files and figures out
which to use. Then, it parses their error definitions into a
pipe-character delimited table with the fields name, number and
descrption from the comments. Finally, it does some consistency checks
and output them as usable C code.
On my Phenom it takes between 1.2 and 2 seconds to run depending upon
the arch. There are a few arch-specific conditions that the script has
to manage however:
* alpha: EAGAIN is redefined as 35 while EDEADLK is defined as 11
* mips: EDQUOT is 1133, which overlaps the internal error range
(512-529), so I'm just removing it.
This is done in a little case $ARCH statement in parse_errors().
Signed-off-by: Daniel Santos <[email protected]>
---
scripts/mkstrerror.sh | 209 ++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 209 insertions(+)
create mode 100755 scripts/mkstrerror.sh
diff --git a/scripts/mkstrerror.sh b/scripts/mkstrerror.sh
new file mode 100755
index 0000000..e7842fc
--- /dev/null
+++ b/scripts/mkstrerror.sh
@@ -0,0 +1,209 @@
+#!/bin/bash
+
+# Generate lib/error_names.h by parsing errno*.h files
+
+typeset -i first=0
+typeset -i last=0
+typeset -i count=0
+
+die() {
+ echo "ERROR: $*" >&2
+ exit 1
+}
+
+parse_errors() {
+ egrep -h '^#define\s+\w+\s+[0-9]+' "$@" | # Extract error definitions
+ case "${ARCH}" in # Apply per-arch fixups
+ alpha) egrep -v 'EAGAIN\s+11';;
+ mips) egrep -v 'EDQUOT\s+1133';;
+ *) cat;;
+ esac |
+ perl -pe '
+ # Replace missing comments with "/* \0 */"
+ s:(\d+)\s*$:$1 /\* \\0 \*/\n:g;
+
+ # Parse into pipe-delimited table
+ s:^#define\s+(\w+)\s+(\w+)\s+/\*\s+(.*?)\s+\*/:$1|$2|$3:g;
+
+ # Since we will be feeding this to printf later, double any
+ # occurrence of %
+ s:%:%%:g;
+ ' |
+ sort '-t|' -nk2 # Sort by error number
+}
+
+tab_align() {
+ typeset -i tabs=$1
+ typeset -i next=0
+ typeset -i i
+ typeset -i len
+ local s
+ shift
+
+ while (($#)); do
+ for (( i = 0; i < next; ++i)); do
+ printf "\t"
+ done
+
+ printf "$1"
+
+ # Get un-escaped string size
+ s=$(printf "$1")
+ (( next = tabs - (${#s} ) / 8 ))
+ shift
+ done
+}
+
+print_errors() {
+ typeset -i next_err=$1
+ typeset -i tabs=$2
+ typeset -i errnum
+ local errname
+ while read; do
+ errnum=${REPLY/*|/}
+ errname=${REPLY/|*/}
+
+ (( next_err <= errnum )) || die "errors out of order :("
+
+ # Fill in any gaps with empty names
+ while (( next_err < errnum )); do
+ printf "\t%s\n" "$(tab_align ${tabs} '"\\0"' "/* ${next_err} */\n")"
+ (( ++next_err ))
+ done
+
+ printf "\t%s\n" "$(tab_align ${tabs} "\"${errname}"'\\0"' "/* ${errnum} */\n")"
+ (( ++next_err ))
+ done
+
+}
+
+count_and_validate() {
+ local names="$1"
+ typeset -i expected_count
+
+ first=$(echo "${names}" | head -1 | awk '{print $3}')
+ last=$(echo "${names}" | tail -1 | awk '{print $3}')
+ count=$(echo "${names}" | wc -l)
+ expected_count=$((last - first + 1))
+
+ if (( count != expected_count )); then
+ echo "ERROR: count = ${count}, expected ${expected_count}"
+ return 1;
+ fi
+ return 0;
+}
+
+find_arch_errno() {
+ for d in $(find arch/${ARCH}/include -name errno.h); do
+ # If it just includes asm-generic/errno.h then skip it
+ if ! grep '#include <asm-generic/errno.h>' $d > /dev/null; then
+ arch_errno="$d"
+ return;
+ fi
+ done
+
+ # Otherwise, no arch-specific errno
+ arch_errno=include/uapi/asm-generic/errno.h
+}
+
+find_arch_errno
+
+base_err_table=$(
+ parse_errors include/uapi/asm-generic/errno-base.h ${arch_errno}
+) || die
+
+internal_err_table=$(
+ parse_errors include/linux/errno.h
+) || die
+
+base_err_names=$(
+ echo "${base_err_table}" |
+ perl -pe 's:(\d+)\|.*:$1:g;'|
+ print_errors 0 4
+) || die
+
+count_and_validate "${base_err_names}" || die
+typeset -i base_err_first=${first}
+typeset -i base_err_last=${last}
+typeset -i base_err_count=${count}
+
+int_err_names=$(
+ echo "${internal_err_table}" |
+ perl -pe 's:(\d+)\|.*:$1:g;'|
+ print_errors 512 4
+) || die
+
+count_and_validate "${int_err_names}" || die
+typeset -i int_err_first=${first}
+typeset -i int_err_last=${last}
+typeset -i int_err_count=${count}
+
+
+cat << asdf
+/* DO NOT EDIT!
+ *
+ * This file is automatically generated by scripts/mkstrerror.sh
+ */
+
+#ifndef _KERNEL_STRERROR_H
+#define _KERNEL_STRERROR_H
+
+#if defined(CONFIG_STRERROR) || defined(CONFIG_STRERROR_NAME)
+
+static const struct error_strings {
+ const unsigned first;
+ const unsigned last;
+ const unsigned count;
+ const char * const desc[2];
+} error_strings[2] = {
+ {
+ .first = ${base_err_first},
+ .last = ${base_err_last},
+ .count = ${base_err_count},
+ .desc = {
+#ifdef CONFIG_STRERROR_NAME
+${base_err_names},
+#else
+ NULL,
+#endif /* CONFIG_STRERROR_NAME */
+
+#ifdef CONFIG_STRERROR
+$(
+ echo "${base_err_table}" |
+ perl -pe 's:.*?(\d+)\|(.*):$2|$1:g;'|
+ print_errors 0 7
+),
+#else
+ NULL,
+#endif /* CONFIG_STRERROR */
+ },
+ }, {
+ .first = ${int_err_first},
+ .last = ${int_err_last},
+ .count = ${int_err_count},
+ .desc = {
+#ifdef CONFIG_STRERROR_NAME
+${int_err_names},
+#else
+ NULL,
+#endif /* CONFIG_STRERROR_NAME */
+
+
+#ifdef CONFIG_STRERROR
+$(
+ echo "${internal_err_table}" |
+ perl -pe 's:.*?(\d+)\|(.*):$2|$1:g;'|
+ print_errors 512 7
+),
+#else
+ NULL,
+#endif /* CONFIG_STRERROR */
+ },
+ }
+};
+
+#endif /* defined(CONFIG_STRERROR) || defined(CONFIG_STRERROR_NAME) */
+
+#endif /* _KERNEL_STRERROR_H */
+asdf
+
--
1.8.1.5
Signed-off-by: Daniel Santos <[email protected]>
---
include/linux/string.h | 8 +++++++
lib/string.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 68 insertions(+)
diff --git a/include/linux/string.h b/include/linux/string.h
index ac889c5..76ce2ff 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -154,4 +154,12 @@ static inline const char *kbasename(const char *path)
return tail ? tail + 1 : path;
}
+#ifdef CONFIG_STRERROR
+extern const char *strerror(int error);
+#endif
+
+#ifdef CONFIG_STRERROR_NAME
+extern const char *strerror_name(int error);
+#endif
+
#endif /* _LINUX_STRING_H_ */
diff --git a/lib/string.c b/lib/string.c
index e5878de..7fd2704 100644
--- a/lib/string.c
+++ b/lib/string.c
@@ -27,6 +27,10 @@
#include <linux/bug.h>
#include <linux/errno.h>
+#if defined(CONFIG_STRERROR) || defined(CONFIG_STRERROR_NAME)
+# include <generated/error_strings.h>
+#endif
+
#ifndef __HAVE_ARCH_STRNICMP
/**
* strnicmp - Case insensitive, length-limited string comparison
@@ -824,3 +828,59 @@ void *memchr_inv(const void *start, int c, size_t bytes)
return check_bytes8(start, value, bytes % 8);
}
EXPORT_SYMBOL(memchr_inv);
+
+#if defined(CONFIG_STRERROR) || defined(CONFIG_STRERROR_NAME)
+static const char *_strerror(int error, unsigned index)
+{
+ unsigned uerror = error < 0 ? -error : error;
+ const struct error_strings *es;
+ const struct error_strings *es_end = &error_strings[
+ sizeof(error_strings) / sizeof(*error_strings)];
+ const char *ret;
+ unsigned i;
+
+ for (es = error_strings; es != es_end; ++es) {
+ if (uerror >= es->first && uerror <= es->last)
+ break;
+ }
+
+ if (es == es_end)
+ return NULL;
+
+ for (i = es->first, ret = es->desc[index]; i < uerror; ++ret)
+ i += !*ret;
+
+ BUG_ON(i > es->last);
+ BUG_ON(i != uerror);
+
+ return *ret ? ret : NULL;
+}
+#endif /* defined(CONFIG_STRERROR) || defined(CONFIG_STRERROR_NAME) */
+
+#ifdef CONFIG_STRERROR
+/**
+ * strerror - Translates an error code into a description.
+ * @error: The error to describe (may be positive or negative)
+ *
+ * Returns a pointer to the error description or NULL if none is found.
+ */
+const char *strerror(int error)
+{
+ return _strerror(error, 1);
+}
+EXPORT_SYMBOL(strerror);
+#endif
+
+#ifdef CONFIG_STRERROR_NAME
+/**
+ * strerror_name - Translates an error code into a name
+ * @error: The error to describe (may be positive or negative)
+ *
+ * Returns a pointer to the error name or NULL if none is found.
+ */
+const char *strerror_name(int error)
+{
+ return _strerror(error, 0);
+}
+EXPORT_SYMBOL(strerror_name);
+#endif
--
1.8.1.5
This is an initial attempt and needs improvement. Ideally,
error_strings.h should only be generated when STRERROR or STRERROR_NAME
are enabled. This implementation also fails to remake error_strings.h
when arch-specific dependencies change. Also, I've noticed that this
implementation fails to output a message when it's running, so help with
this would be appreciated.
Signed-off-by: Daniel Santos <[email protected]>
---
Makefile | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/Makefile b/Makefile
index 8243a23..38049ca 100644
--- a/Makefile
+++ b/Makefile
@@ -835,7 +835,7 @@ endif
prepare2: prepare3 outputmakefile asm-generic
prepare1: prepare2 $(version_h) include/generated/utsrelease.h \
- include/config/auto.conf
+ include/config/auto.conf include/generated/error_strings.h
$(cmd_crmodverdir)
archprepare: archheaders archscripts prepare1 scripts_basic
@@ -873,6 +873,13 @@ $(version_h): $(srctree)/Makefile FORCE
include/generated/utsrelease.h: include/config/kernel.release FORCE
$(call filechk,utsrelease.h)
+include/generated/error_strings.h: include/uapi/asm-generic/errno-base.h \
+ include/uapi/asm-generic/errno.h \
+ include/uapi/linux/errno.h \
+ include/linux/errno.h
+ $(shell ARCH=$(ARCH) $(srctree)/scripts/mkstrerror.sh \
+ > $(srctree)/include/generated/error_strings.h)
+
PHONY += headerdep
headerdep:
$(Q)find $(srctree)/include/ -name '*.h' | xargs --max-args 1 \
--
1.8.1.5
This adds an extension for the integral format specifier suffix of 'e',
so that the format %[duxXo]e will result in printing an number (as
before) in addition to a name and descrption for an error code, if such
support is enabled and a name and descrption is found.
My initial thought was to use the glibc extension of '%m', but this
format specifier uses the value of libc's errno and adding a value
breaks gcc's printf parsing. I'm not convinced that the 'e' extension
is optimal, although there are only four instances of this pattern in
the kernel that would need to be changed.
git grep -E '".*%[#0\ +\-]*[0-9]*[hljzt]*[idoxXu]e'
Alternatively, 'E' was another thought for a suffix as well.
Signed-off-by: Daniel Santos <[email protected]>
---
lib/vsprintf.c | 92 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 92 insertions(+)
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 26559bd..d96d675 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -345,6 +345,12 @@ int num_to_str(char *buf, int size, unsigned long long num)
#define SMALL 32 /* use lowercase in hex (must be 32 == 0x20) */
#define SPECIAL 64 /* prefix hex with "0x", octal with "0" */
+#if defined(CONFIG_PRINTK_ERROR_NAMES) || defined(CONFIG_PRINTK_ERROR_DESCS)
+# define ERROR 128 /* error name and/or descrption */
+#else
+# define ERROR 0
+#endif
+
enum format_type {
FORMAT_TYPE_NONE, /* Just a string part */
FORMAT_TYPE_WIDTH,
@@ -1346,6 +1352,75 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
return number(buf, end, (unsigned long) ptr, spec);
}
+/**
+ * error - format an error code into a descrption
+ *
+ * Basically, we print the number, name and descrption, but only if enabled.
+ * If there is no string for an error number, it's printed as if the option is
+ * disabled. The format is basically equivalent to:
+ *
+ * printk("%d (%s: %s)", num, name, descrption);
+ *
+ * If there's no descrption, it would end up like:
+ *
+ * printk("%d (%s)", num, name);
+ *
+ * etc.
+ *
+ * Note that we ignore width and precision here, although they are applied to
+ * the numerical portion of the output in number().
+ *
+ * Since we're initializing desc to NULL and then only setting it based upon
+ * a pre-compiler constant value, all code expecting it to be non-zero should
+ * compile-out when CONFIG_PRINTK_ERROR_DESCS is not enabled.
+ */
+#if defined(CONFIG_PRINTK_ERROR_NAMES) || defined(CONFIG_PRINTK_ERROR_DESCS)
+static noinline_for_stack
+char *error(char *buf, char *end, int err, struct printf_spec spec)
+{
+ const char *name = strerror_name(err);
+ const char *desc = NULL;
+
+#if defined(CONFIG_PRINTK_ERROR_DESCS)
+ desc = strerror(err);
+#endif
+
+ if (!(name || desc))
+ return buf;
+
+ if (&buf[1] < end) {
+ *buf++ = ' ';
+ *buf++ = '(';
+ }
+
+ if (name)
+ while (buf < end && *name)
+ *buf++ = *name++;
+
+ if (desc) {
+ if (name && &buf[1] < end) {
+ *buf++ = ':';
+ *buf++ = ' ';
+ }
+
+ while (buf < end && *desc)
+ *buf++ = *desc++;
+ }
+
+ if (buf < end)
+ *buf++ = ')';
+
+ return buf;
+}
+#else
+static inline
+char *error(char *buf, char *end, int err, struct printf_spec spec)
+{
+ return buf;
+}
+#endif /* CONFIG_PRINTK_ERROR_NAMES || CONFIG_PRINTK_ERROR_DESCS */
+
+
/*
* Helper function to decode printf style format.
* Each call decode a token from the format and return the
@@ -1501,12 +1576,20 @@ qualifier:
case 'X':
spec->base = 16;
+ if (fmt[1] == 'e') {
+ spec->flags |= ERROR;
+ ++fmt;
+ }
break;
case 'd':
case 'i':
spec->flags |= SIGN;
case 'u':
+ if (fmt[1] == 'e') {
+ spec->flags |= ERROR;
+ ++fmt;
+ }
break;
default:
@@ -1577,6 +1660,9 @@ qualifier:
* %*ph[CDN] a variable-length hex string with a separator (supports up to 64
* bytes of the input)
* %n is ignored
+ * %[idxXu]e will print the name of error code, if one exists. If the number
+ * doesn't match a known error code, it is printed as a normal signed int.
+ * TODO: explain CONFIG_PRINTK_ERROR*
*
* ** Please update Documentation/printk-formats.txt when making changes **
*
@@ -1738,6 +1824,9 @@ int vsnprintf(char *buf, size_t size, const char *fmt, va_list args)
}
str = number(str, end, num, spec);
+
+ if (spec.flags & ERROR)
+ str = error(str, end, num, spec);
}
}
@@ -2185,6 +2274,9 @@ int bstr_printf(char *buf, size_t size, const char *fmt, const u32 *bin_buf)
}
str = number(str, end, num, spec);
+
+ if (spec.flags & ERROR)
+ str = error(str, end, num, spec);
} /* default: */
} /* switch(spec.type) */
} /* while(*fmt) */
--
1.8.1.5
This adds to lib/Kconfig.debug the options for printk messages to
display either error number only (the current behavior), number and
error name or number, name and description. These options in turn select
STRERROR_NAME and STRERROR as needed, so I'm not adding any direct
options to enable those, although it can be added later if somebody
needs strerror() or strerror_name() when not enabled in printks.
kconfg
Signed-off-by: Daniel Santos <[email protected]>
---
lib/Kconfig.debug | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 53 insertions(+)
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index c9eef36..919f371 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -58,6 +58,7 @@ config DYNAMIC_DEBUG
implicitly compiles in all pr_debug() and dev_dbg() calls, which
enlarges the kernel text size by about 2%.
+
If a source file is compiled with DEBUG flag set, any
pr_debug() calls in it are enabled by default, but can be
disabled at runtime as below. Note that DEBUG flag is
@@ -113,6 +114,58 @@ config DYNAMIC_DEBUG
See Documentation/dynamic-debug-howto.txt for additional information.
+choice
+ prompt "Display errors in printk as"
+ default PRINTK_ERROR_CODES_ONLY
+ depends on PRINTK
+ help
+ This option tells printk how to display error codes when
+ formatted with the %de format code (a Linux printk extension).
+ Error codes are most commonly passed to userspace when syscalls
+ return where they are then translated into localized descrptions
+ via libc's strerror(), perror(), etc. However, many error
+ conditions (for drivers and such) are only reported via printks
+ where there is no such userspace mechanism to reliably translate
+ error numbers into something a little more human.
+
+ This option allows you to perform that translation in the kernel,
+ at the cost of a slightly larger kernel. Note that no native
+ language support is currently, or may ever be provided.
+
+ If unsure, choose "Error codes only".
+
+config PRINTK_ERROR_CODES_ONLY
+ bool "Error codes only"
+ help
+ Just display the error number.
+
+config PRINTK_ERROR_NAMES
+ bool "Error names"
+ select STRERROR_NAME
+ help
+ This option causes printk messages containing an error code to
+ print the name of the error in addition to the error number.
+ Enabling this adds about 2k. Example: -22 (EINVAL)
+
+config PRINTK_ERROR_DESCS
+ bool "Full descrptions"
+ select STRERROR
+ select STRERROR_NAME
+ help
+ This option causes printk messages containing an error code to
+ print the description of errors (in addition to the name and
+ number) and can be helpful for those with aversions to search
+ engines. Enabling this adds about 6k over error codes alone.
+ Example: -90 (EMSGSIZE: Message too long)
+
+endchoice
+
+config STRERROR
+ bool
+
+config STRERROR_NAME
+ bool
+
endmenu # "printk and dmesg options"
menu "Compile-time checks and compiler options"
--
1.8.1.5
On 09/17/2013 06:08 PM, [email protected] wrote:
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -58,6 +58,7 @@ config DYNAMIC_DEBUG
> implicitly compiles in all pr_debug() and dev_dbg() calls, which
> enlarges the kernel text size by about 2%.
>
> +
> If a source file is compiled with DEBUG flag set, any
> pr_debug() calls in it are enabled by default, but can be
> disabled at runtime as below. Note that DEBUG flag is
> @@ -113,6 +114,58 @@ config DYNAMIC_DEBUG
whoops! Please ignore this carriage return! :)
On Tue, 2013-09-17 at 18:08 -0500, [email protected] wrote:
> This adds an extension for the integral format specifier suffix of 'e',
> so that the format %[duxXo]e will result in printing an number (as
> before) in addition to a name and descrption for an error code, if such
> support is enabled and a name and descrption is found.
>
> My initial thought was to use the glibc extension of '%m', but this
> format specifier uses the value of libc's errno and adding a value
> breaks gcc's printf parsing. I'm not convinced that the 'e' extension
> is optimal, although there are only four instances of this pattern in
> the kernel that would need to be changed.
>
> git grep -E '".*%[#0\ +\-]*[0-9]*[hljzt]*[idoxXu]e'
>
> Alternatively, 'E' was another thought for a suffix as well.
I'd much rather see another %p extension used instead
of generating odd output given normal printf formats.
%pE might work
Joe Perches <[email protected]> wrote:
> On Tue, 2013-09-17 at 18:08 -0500, [email protected] wrote:
> > This adds an extension for the integral format specifier suffix of 'e',
> > so that the format %[duxXo]e will result in printing an number (as
> > before) in addition to a name and descrption for an error code, if such
> > support is enabled and a name and descrption is found.
> >
> > My initial thought was to use the glibc extension of '%m', but this
> > format specifier uses the value of libc's errno and adding a value
> > breaks gcc's printf parsing. I'm not convinced that the 'e' extension
> > is optimal, although there are only four instances of this pattern in
> > the kernel that would need to be changed.
> >
> > git grep -E '".*%[#0\ +\-]*[0-9]*[hljzt]*[idoxXu]e'
> >
> > Alternatively, 'E' was another thought for a suffix as well.
>
> I'd much rather see another %p extension used instead
> of generating odd output given normal printf formats.
>
> %pE might work
I guess you'd use that with ERR_PTR(). On one hand, it would look decidedly
odd, but on the other hand, we have errors in pointers in a lot of places
already which wouldn't then need converting back - so it doesn't seem entirely
unreasonable.
David
[email protected] wrote:
> Typically, we don't care about error messages or names in the kernel because
> userspace will manage that. But sometimes we need to output an error number
> to printks and that creates a situation where a user, system admistrator or
> developer must find an error number reference to figure out what went wrong
> with a particular driver or whatever. This patch adds two alternatives at
> increasing memory costs:
>
> 1. print the number in addition to the name for 2k extra or
> 2. print the number, name and description for 6k extra.
I like the idea generally - and have occasionally entertained the idea of
implementing it myself. However, I wouldn't bother with the "human readable"
description if we're going to do this. Generally, the symbolic representation
is good enough - and that's what you're going to grep the code for anyway.
David
[email protected] wrote:
> This is a simple bash script that parses our errno*.h files and formats
> them into the error_strings.h header that our strerror and strerror_name
> functions will use later.
I presume you haven't tried building with a "make O=foo" build directory? I
see:
/bin/sh: /data/fs/linux-2.6-fscache/include/generated/error_strings.h: No such file or directory
when I try it.
Looking in your generated error_strings.h file, I see:
static const struct error_strings {
const unsigned first;
const unsigned last;
const unsigned count;
const char * const desc[2];
} error_strings[2] = {
{
.first = 0,
.last = 133,
.count = 134,
.desc = {
#ifdef CONFIG_STRERROR_NAME
"\0" /* 0 */
"EPERM\0" /* 1 */
"ENOENT\0" /* 2 */
"ESRCH\0" /* 3 */
"EINTR\0" /* 4 */
...
"ERFKILL\0" /* 132 */
"EHWPOISON\0" /* 133 */,
#else
NULL,
#endif /* CONFIG_STRERROR_NAME */
#ifdef CONFIG_STRERROR
"\0" /* 0 */
"Operation not permitted\0" /* 1 */
"No such file or directory\0" /* 2 */
"No such process\0" /* 3 */
...
"State not recoverable\0" /* 131 */
"Operation not possible due to RF-kill\0" /* 132 */
"Memory page has hardware error\0" /* 133 */,
#else
NULL,
#endif /* CONFIG_STRERROR */
},
}, {
.first = 512,
.last = 529,
.count = 18,
.desc = {
#ifdef CONFIG_STRERROR_NAME
"ERESTARTSYS\0" /* 512 */
"ERESTARTNOINTR\0" /* 513 */
...
"Request initiated, but will not complete before timeout\0"/* 528 */
"iocb queued, will get completion event\0" /* 529 */,
#else
NULL,
#endif /* CONFIG_STRERROR */
},
}
};
Some thoughts for you:
(1) Why are you double-NUL'ing all your strings? (see the \0 in the strings)
(2) Storing the leading 'E' for each symbol is redundant as you can add that
later so you might want to discard it.
(3) You are storing a pointer to the symbolic name for each error. On a
64-bit machine, that's 8 bytes. If you drop the leading 'E' and the
trailing NUL, most symbols will fit into an 8 character slot saving you
the cost of a pointer.
Okay, you'll have to truncate some of the names (ERESTARTNOINTR ->
RESNOINT for example) but probably not that many. Run this:
grep '["]E[A-Z0-9]' include/generated/error_strings.h |
cut '-d\' -f1 | cut -dE -f2- | cut -c1-8
and you'll see what I mean. Most of them are still recognisable,
particularly once you stick the 'E' back on the front. Piping the output
of that through "wc -l", I get:
147
which means the entire string table could be squeezed into 8 * 147 or
1176 bytes with only one pointer and one count required for each segment
of the table (you generate two segments).
David
David Howells <[email protected]> wrote:
> (1) Why are you double-NUL'ing all your strings? (see the \0 in the strings)
Ah... I see what you're doing. I missed the fact that you don't have a comma
after each string.
> (3) You are storing a pointer to the symbolic name for each error. On a
> 64-bit machine, that's 8 bytes. If you drop the leading 'E' and the
> trailing NUL, most symbols will fit into an 8 character slot saving you
> the cost of a pointer.
> ...
In which case, you can ignore this too.
David
On 09/18/2013 06:55 AM, David Howells wrote:
> David Howells <[email protected]> wrote:
>
>> (1) Why are you double-NUL'ing all your strings? (see the \0 in the strings)
> Ah... I see what you're doing. I missed the fact that you don't have a comma
> after each string.
Yeah, I was trying to format the code so that it's efficient and still
easy to read, but then there's no comma where we normally expect one.
It's the best middle ground solution I could come up with.
On 09/18/2013 06:38 AM, David Howells wrote:
> [email protected] wrote:
>
>> This is a simple bash script that parses our errno*.h files and formats
>> them into the error_strings.h header that our strerror and strerror_name
>> functions will use later.
> I presume you haven't tried building with a "make O=foo" build directory? I
> see:
>
> /bin/sh: /data/fs/linux-2.6-fscache/include/generated/error_strings.h: No such file or directory
>
> when I try it.
No, indeed I haven't, thanks. I'm going to need help figuring out the
correct way to put this in the build because the current definitely
isn't correct. From what I could tell from digging into the build last
night (which I've never carefully analyzed), this should be added
somewhere in one of the scripts/Makefile*s and not in the root makefile
(as I have done), or maybe even in lib/Makefile.
>
> (2) Storing the leading 'E' for each symbol is redundant as you can add that
> later so you might want to discard it.
This is a good thought, that's 150-ish bytes, but it will introduce some
new challenges. Currently, strerror() functions exactly like "man 3p
strerror", except that it returns a pointer to a const string like the
POSIX specification should have done. :) So I directly return a pointer
to the string within the .rodata section of the object file (same for
strerror_name). Omitting the 'E' would require I work up another
solution requiring a return buffer or some such, thus increasing
complexity and possibly loosing that savings to code.
However, if we wanted to squeze that much more out of it, then we could
7-th bit terminate the strings and save another 150-ish bytes on null
terminators or go even further and use some encoding scheme (maybe 32
bits per character)? At 2k for both the error names and the code, I
figured it was already pretty low, but if there are some existing
libraries in the kernel we could use to do this and not further bloat
the text size for decompression then I wouldn't be opposed, so long as
it makes sense. And since we're dealing with error conditions which
don't happen often, speed shouldn't be a concern.
Thanks!
Daniel
On 09/18/2013 06:08 AM, David Howells wrote:
> [email protected] wrote:
>
>> Typically, we don't care about error messages or names in the kernel because
>> userspace will manage that. But sometimes we need to output an error number
>> to printks and that creates a situation where a user, system admistrator or
>> developer must find an error number reference to figure out what went wrong
>> with a particular driver or whatever. This patch adds two alternatives at
>> increasing memory costs:
>>
>> 1. print the number in addition to the name for 2k extra or
>> 2. print the number, name and description for 6k extra.
> I like the idea generally - and have occasionally entertained the idea of
> implementing it myself. However, I wouldn't bother with the "human readable"
> description if we're going to do this. Generally, the symbolic representation
> is good enough - and that's what you're going to grep the code for anyway.
>
> David
Yeah, I thought about that too, but I figured that descriptions were
already there so I may as well make it an option for the
search-engine-impared. :) I was also thinking about system
administrators and ordinary users trying to installing device drivers
and other such situations where they would have to look at their syslogs
or dmesg -- or just the lazy. :)
Daniel
On 09/18/2013 06:04 AM, David Howells wrote:
> Joe Perches <[email protected]> wrote:
>
>> On Tue, 2013-09-17 at 18:08 -0500, [email protected] wrote:
>>> This adds an extension for the integral format specifier suffix of 'e',
>>> so that the format %[duxXo]e will result in printing an number (as
>>> before) in addition to a name and descrption for an error code, if such
>>> support is enabled and a name and descrption is found.
>>>
>>> My initial thought was to use the glibc extension of '%m', but this
>>> format specifier uses the value of libc's errno and adding a value
>>> breaks gcc's printf parsing. I'm not convinced that the 'e' extension
>>> is optimal, although there are only four instances of this pattern in
>>> the kernel that would need to be changed.
>>>
>>> git grep -E '".*%[#0\ +\-]*[0-9]*[hljzt]*[idoxXu]e'
>>>
>>> Alternatively, 'E' was another thought for a suffix as well.
>> I'd much rather see another %p extension used instead
>> of generating odd output given normal printf formats.
>>
>> %pE might work
> I guess you'd use that with ERR_PTR(). On one hand, it would look decidedly
> odd, but on the other hand, we have errors in pointers in a lot of places
> already which wouldn't then need converting back - so it doesn't seem entirely
> unreasonable.
>
> David
Hmm, this discussion makes me pine for the gnu libc '%m' extension even
further. I wish there was an easy way to tell gcc that you want it to
check your format, but here are my extensions. Oh well.
Honestly, I'm not too keen on the %pE idea, although I can see that %p
is where all of the kernel extensions currently are. Unless I'm
incorrect, if I use ERR_PTR() on a signed int on a x86_64 where pointer
is 64 bits and int is 32, wouldn't that mean a signed conversion
instruction where the sign bit has to be moved from bit 31 to 63?
Either way, %pE does seem to make a lot of sense for conditions where we
already have a pointer that we would otherwise use PTR_ERR() to convert,
but it just seems klunky to use it on an int, to have it treated like a
pointer and then re-interpreted as an int. Maybe we can add %pE as well
as %dE and leave [ioxXu] out of it?
Daniel
On 09/18/2013 06:38 AM, David Howells wrote:
> [email protected] wrote:
>
>> This is a simple bash script that parses our errno*.h files and formats
>> them into the error_strings.h header that our strerror and strerror_name
>> functions will use later.
> I presume you haven't tried building with a "make O=foo" build directory? I
> see:
>
> /bin/sh: /data/fs/linux-2.6-fscache/include/generated/error_strings.h: No such file or directory
>
> when I try it.
Hmm, I cannot reproduce the error. :( I'm using next-20130919 currently
(x86_64), and if I try to just "make O=lib" it fails w/o my patches.
The only file that should depend upon error_strings.h is lib/string.c.
/home/daniel/proj/kernel/git
(daniel@loudmouth)$ make mrproper -j4
/home/daniel/proj/kernel/git
(daniel@loudmouth)$ make defconfig -j4
HOSTCC scripts/basic/fixdep
HOSTCC scripts/kconfig/conf.o
SHIPPED scripts/kconfig/zconf.tab.c
SHIPPED scripts/kconfig/zconf.lex.c
SHIPPED scripts/kconfig/zconf.hash.c
HOSTCC scripts/kconfig/zconf.tab.o
HOSTLD scripts/kconfig/conf
*** Default configuration is based on 'x86_64_defconfig'
#
# configuration written to .config
#
/home/daniel/proj/kernel/git
(daniel@loudmouth)$ make O=lib -j4
HOSTCC scripts/basic/fixdep
HOSTCC scripts/kconfig/conf.o
HOSTCC scripts/kconfig/zconf.tab.o
HOSTLD scripts/kconfig/conf
scripts/kconfig/conf --silentoldconfig Kconfig
***
*** Configuration file ".config" not found!
***
*** Please run some configurator (e.g. "make oldconfig" or
*** "make menuconfig" or "make xconfig").
***
make[3]: *** [silentoldconfig] Error 1
make[2]: *** [silentoldconfig] Error 2
make[2]: Nothing to be done for `all'.
make[1]: *** No rule to make target `include/config/auto.conf', needed
by `include/config/kernel.release'. Stop.
make[1]: *** Waiting for unfinished jobs....
make: *** [sub-make] Error 2
Is this some other subtle bug currently in -next or are you just
supposed to run "make prepare" first? I injected the generation of
error_names.h as a dependency of prepare1 (rightly or wrongly). I'm
still studying the kbuild process to try to find a better place for it
or to at least clean up the way it's generated.
Daniel
On 09/19/2013 05:35 PM, Daniel Santos wrote:
>
> Hmm, I cannot reproduce the error. :( I'm using next-20130919
> currently (x86_64), and if I try to just "make O=lib" it fails w/o my
> patches. The only file that should depend upon error_strings.h is
> lib/string.c.
Ahh! I've never seen the "make O=foo" before and just presumed it was an
alternate directive to specify a directory of the source tree to build.
I'm certainly glad to learn that the kernel supports out of tree builds! :)
So I am getting the error now and this is because the script expects to
run in the root of the source tree. This will be an easy fix and I think
I've figured out the correct place to put the make target for it as well.
Daniel
On Wed, 2013-09-18 at 20:27 -0500, Daniel Santos wrote:
> if I use ERR_PTR() on a signed int on a x86_64 where pointer
> is 64 bits and int is 32, wouldn't that mean a signed conversion
> instruction where the sign bit has to be moved from bit 31 to 63?
No. It's cast to long
static inline void * __must_check ERR_PTR(long error)
{
return (void *) error;
}
> Either way, %pE does seem to make a lot of sense for conditions where we
> already have a pointer that we would otherwise use PTR_ERR() to convert,
> but it just seems klunky to use it on an int, to have it treated like a
> pointer and then re-interpreted as an int. Maybe we can add %pE as well
> as %dE and leave [ioxXu] out of it?
I think having just one way to format is better.
On 09/19/2013 08:07 PM, Joe Perches wrote:
> On Wed, 2013-09-18 at 20:27 -0500, Daniel Santos wrote:
>> if I use ERR_PTR() on a signed int on a x86_64 where pointer
>> is 64 bits and int is 32, wouldn't that mean a signed conversion
>> instruction where the sign bit has to be moved from bit 31 to 63?
> No. It's cast to long
>
> static inline void * __must_check ERR_PTR(long error)
> {
> return (void *) error;
> }
Yes, but it is that cast from int to long that costs us a signed extend
instruction on platforms where sizeof(int) != sizeof(long). This
example should demonstrate the issue:
extern void funca(void *ptr);
static inline void * ERR_PTR(long error)
{
return (void *) error;
}
void funcb(int i)
{
funca(ERR_PTR(i));
}
void funcc(long l)
{
funca(ERR_PTR(l));
}
And here is the generated code on x86_64 with -O2:
0000000000000000 <funcb>:
return (void *) error;
}
void funcb(int i)
{
funca(ERR_PTR(i));
0: 48 63 ff movslq %edi,%rdi
3: e9 00 00 00 00 jmpq 8 <funcb+0x8>
4: R_X86_64_PC32 funca-0x4
8: 0f 1f 84 00 00 00 00 nopl 0x0(%rax,%rax,1)
f: 00
0000000000000010 <funcc>:
}
void funcc(long l)
{
funca(ERR_PTR(l));
10: e9 00 00 00 00 jmpq 15 <funcc+0x5>
11: R_X86_64_PC32 funca-0x4
So on x86_64 this movslq is 3 bytes of text, plus register pollution for
each time you convert an int to a pointer. I don't know the precise
number of cases where error numbers are passed to printk as ints, but I
presume it is enough that this could add several kilobytes of text.
Either way, for a popular function like vsnprintf, it's better to take a
moderate bloat in the function than a little bloat at many call sites,
especially when it's not performance critical.
>> Either way, %pE does seem to make a lot of sense for conditions where we
>> already have a pointer that we would otherwise use PTR_ERR() to convert,
>> but it just seems klunky to use it on an int, to have it treated like a
>> pointer and then re-interpreted as an int. Maybe we can add %pE as well
>> as %dE and leave [ioxXu] out of it?
> I think having just one way to format is better.
>
Yeah, I do agree, I just don't see how to do it without introducing
unnecessary bloat.
Daniel
On Fri, 2013-09-20 at 00:21 -0500, Daniel Santos wrote:
[nice example about bloat]
> Yeah, I do agree, I just don't see how to do it without introducing
> unnecessary bloat.
I think the code size cost of %pE is pretty trivial
compared to the log size/runtime cost.
I would not want to see it used generically.
Joe Perches <[email protected]> wrote:
> > Yeah, I do agree, I just don't see how to do it without introducing
> > unnecessary bloat.
>
> I think the code size cost of %pE is pretty trivial
> compared to the log size/runtime cost.
>
> I would not want to see it used generically.
I suspect he doesn't really need to implement a "strerror()" function but
should rather build it straight into printk()/vsprintf().
David
On 09/20/2013 06:45 AM, Joe Perches wrote:
> On Fri, 2013-09-20 at 00:21 -0500, Daniel Santos wrote:
>
> [nice example about bloat]
>
>> Yeah, I do agree, I just don't see how to do it without introducing
>> unnecessary bloat.
> I think the code size cost of %pE is pretty trivial
> compared to the log size/runtime cost.
I suppose that would depend upon the conditions. If no printks with
error codes/names/messages are triggered, then the cost on the syslog is
zero. Either way, we're already looking at adding one byte to the format
string for the extra 'E' character and the 3-4 bytes for sign extend
instruction would not exist on arm and most other archs. I guess I'm
usually trying to consider the memory constrained system where you
wouldn't enable this feature but still suffer the sign extend bloat --
then again, I don't know of many embedded x86_64 systems. :)
> I would not want to see it used generically.
Hmm, I had considered this as a new mechanism to emit error codes to the
ring buffer. What sort of guidelines do you think are reasonable for
when to use this proposed extension and when not to? Maybe on printks
for error conditions that are most likely?
Daniel
On 09/20/2013 08:07 AM, David Howells wrote:
> I suspect he doesn't really need to implement a "strerror()" function but
> should rather build it straight into printk()/vsprintf().
>
> David
Indeed we don't necessarily *need* a strerror() function per-se, but it
is an addition to the libc support in the kernel. I'll leave it up to
the community to decide rather or not it should be separate or
integrated into vsprintf.c. Also, there is no POSIX or GNU libc
extension (that I am aware of) to print an error name -- a real shame
imho, so the name strerror_name() is pretty much artificial. Is there a
significant overhead in having these functions exported?
Personally, I would think that the only reason to have interest in an
error name or message in the kernel is to output via printk, but I like
to allow room for the conditions that I may not have thought of.
Daniel