Skip to contentSkip to conversation

Extend print_symbol capability TRY #3

PATCH In review Robert Peterson 2 participants 4 messages Mar 6, 2007 – Mar 7, 2007

lore
Share & tools
lore

Robert Peterson

author
#0 lore raw

PATCH 1/1 Extend print_symbol capability TRY #3

This is try #3 for this patch, with corrections based on feedback. It is the same as try #2 except:

(1) I changed the comment on function __print_symbol. Although my original patch didn't change this comment, Randy Dunlap noted that the comment was wrong, so I changed it. Hopefully it makes more sense now.
(2) I followed Andrew Morton's suggestion and reordered the parameters to the sprint_symbol function so that buffer comes first. That makes it more like other sprint functions elsewhere.
(3) I hope to <insert diety/> this time it's inlined correctly, with no word wrap, and my tabs haven't been converted to spaces again. (Sorry).
One last note: This has been tested with the patch to GFS2 to print symbols through debugfs (see original comment), and it works.

Original comment:

Today's print_symbol function dumps a kernel symbol with printk. This patch extends the functionality of kallsyms.c so that the symbol lookup function may be used without the printk. This is useful for modules that want to dump symbols elsewhere, for example, to debugfs. I intend to use the new function call in the GFS2 file system (which will be a separate patch).

Signed-off-by Robert Peterson <[email protected]>

---

include/linux/kallsyms.h |   12 +++++++++++-
kernel/kallsyms.c        |   17 ++++++++++++-----
2 files changed, 23 insertions(+), 6 deletions(-)
include/linux/kallsyms.h +0 -0
index 1cebcbc..5356133 100644
@@ -7,6 +7,8 @@

#define KSYM_NAME_LEN 127
+#define KSYM_SYMBOL_LEN (sizeof("%s+%#lx/%#lx [%s]") + KSYM_NAME_LEN + \
+ 2*(BITS_PER_LONG*3/10) + MODULE_NAME_LEN + 1)

#ifdef CONFIG_KALLSYMS
/* Lookup the address for a symbol. Returns 0 if not found. */ @@ -22,7 +24,10 @@ const char *kallsyms_lookup(unsigned long addr,

			    unsigned long *offset,
			    char **modname, char *namebuf);

-/* Replace "%s" in format with address, if found */
+/* Look up a kernel symbol and return it in a text buffer. */ +extern void sprint_symbol(char *buffer, unsigned long address); +
+/* Look up a kernel symbol and print it to the kernel messages. */ extern void __print_symbol(const char *fmt, unsigned long address);

#else /* !CONFIG_KALLSYMS */
@@ -47,6 +52,11 @@ static inline const char *kallsyms_lookup(unsigned long addr,

	return NULL;
}

+static inline void sprint_symbol(char *buffer, unsigned long addr) +{
+ return;
+}
+
/* Stupid that this does nothing, but I didn't create this mess. */ #define __print_symbol(fmt, addr)
#endif /*CONFIG_KALLSYMS*/

kernel/kallsyms.c +0 -0
index 6f294ff..0d6ae07 100644
@@ -267,18 +267,15 @@ const char *kallsyms_lookup(unsigned long addr,

	return NULL;
}

-/* Replace "%s" in format with address, or returns -errno. */ -void __print_symbol(const char *fmt, unsigned long address) +/* Look up a kernel symbol and return it in a text buffer. */ +void sprint_symbol(char *buffer, unsigned long address)

{
	char *modname;
	const char *name;
	unsigned long offset, size;
	char namebuf[KSYM_NAME_LEN+1];

- char buffer[sizeof("%s+%#lx/%#lx [%s]") + KSYM_NAME_LEN +
- 2*(BITS_PER_LONG*3/10) + MODULE_NAME_LEN + 1];

	name = kallsyms_lookup(address, &size, &offset, &modname, namebuf);
-
	if (!name)
		sprintf(buffer, "0x%lx", address);
	else {
@@ -288,6 +285,15 @@ void __print_symbol(const char *fmt, unsigned long address)
		else
			sprintf(buffer, "%s+%#lx/%#lx", name, offset, size);
	}

+}
+
+/* Look up a kernel symbol and print it to the kernel messages. */ +void __print_symbol(const char *fmt, unsigned long address) +{
+ char buffer[KSYM_SYMBOL_LEN];
+
+ sprint_symbol(buffer, address);
+

	printk(fmt, buffer);
}

@@ -452,3 +458,4 @@ static int __init kallsyms_init(void) __initcall(kallsyms_init);

EXPORT_SYMBOL(__print_symbol);
+EXPORT_SYMBOL_GPL(sprint_symbol);

#1 lore raw

Review static inline const char *kallsyms_lookup(unsigned · line 59

Quoted 10 lines · Robert Peterson

[...]

(quoted diff) +4 -0
@@ -47,6 +52,11 @@ static inline const char *kallsyms_lookup(unsigned 
 long addr,
    return NULL;
 }
 
+static inline void sprint_symbol(char *buffer, unsigned long addr)
+{
+    return;
+}

I'm really sorry for not replying sooner (I've been really busy), but this function still doesn't seem right.

If someone does something like:

Quoted 9 lines
(quoted diff) +0 -0
 void my_function(unsigned long addr)
 {

	char buffer[KSYM_SYMBOL_LEN];

	sprint_symbol(buffer, addr);
	...
	// use buffer to print somewhere
	...
}

which seems like a perfectly natural thing to do, it might just oops the kernel if CONFIG_KALLSYMS is not set, because the buffer will be left uninitialized.

That is why I suggested to change it to something like "*buffer = '\0'" instead.

The really nice solution IMHO, would be to remove the print_symbol and sprint_symbol functions from the the "#ifdef CONFIG_KALLSYMS" and just let them be available even in a not CONFIG_KALLSYMS kernel.

Since kallsyms_lookup is already #ifdef'ed to something sane, sprint_symbol will just print out the symbol address in that case, but it is better than not printing anything at all.

Signature · 3 lines
Paulo Marques - www.grupopie.com

"Very funny Scotty. Now beam up my clothes."
author
#2 lore raw
Quoted 1 line · Paulo Marques

That is why I suggested to change it to something like "*buffer = '\0'" instead.

Point well taken. A revised patch with your suggested fix is below.

Quoted 3 lines

The really nice solution IMHO, would be to remove the print_symbol and sprint_symbol functions from the the "#ifdef CONFIG_KALLSYMS" and just let them be available even in a not CONFIG_KALLSYMS kernel.

Since kallsyms_lookup is already #ifdef'ed to something sane, sprint_symbol will just print out the symbol address in that case, but it is better than not printing anything at all.

Although this sounds like a good idea, I'm uncomfortable taking the fix to this level as part of this patch. If CONFIG_KALLSYMS is not set, the Makefile doesn't even compile kallsyms.c due to this:

kernel/Makefile:obj-$(CONFIG_KALLSYMS) += kallsyms.o

To keep the functionality you mentioned, we'd have to keep kallsyms.o and keeping kallsyms.o in the kernel would have a whole different set of ramifications in the kernel that I don't want to chase down.
It probably would be easy, but it's beyond the scope of this fix,
and therefore probably best left to a separate patch.
Feel free to send that out in another patch if you want. If you do,
I'd be happy to test it, review it, and ACK it.

Regards,

Bob Peterson

Amended patch (TRY #4) follows:

Signed-off-by Robert Peterson <[email protected]>

--- include/linux/kallsyms.h | 13 ++++++++++++-
kernel/kallsyms.c | 17 ++++++++++++-----

2 files changed, 24 insertions(+), 6 deletions(-)
include/linux/kallsyms.h +0 -0
index 1cebcbc..cd83e89 100644
@@ -7,6 +7,8 @@

#define KSYM_NAME_LEN 127
+#define KSYM_SYMBOL_LEN (sizeof("%s+%#lx/%#lx [%s]") + KSYM_NAME_LEN + \
+ 2*(BITS_PER_LONG*3/10) + MODULE_NAME_LEN + 1)

#ifdef CONFIG_KALLSYMS
/* Lookup the address for a symbol. Returns 0 if not found. */ @@ -22,7 +24,10 @@ const char *kallsyms_lookup(unsigned long addr,

			    unsigned long *offset,
			    char **modname, char *namebuf);

-/* Replace "%s" in format with address, if found */
+/* Look up a kernel symbol and return it in a text buffer. */ +extern void sprint_symbol(char *buffer, unsigned long address); +
+/* Look up a kernel symbol and print it to the kernel messages. */ extern void __print_symbol(const char *fmt, unsigned long address);

#else /* !CONFIG_KALLSYMS */
@@ -47,6 +52,12 @@ static inline const char *kallsyms_lookup(unsigned long addr,

	return NULL;
}

+static inline void sprint_symbol(char *buffer, unsigned long addr) +{
+ *buffer = '\0';
+ return;
+}
+
/* Stupid that this does nothing, but I didn't create this mess. */ #define __print_symbol(fmt, addr)
#endif /*CONFIG_KALLSYMS*/

kernel/kallsyms.c +0 -0
index 6f294ff..0d6ae07 100644
@@ -267,18 +267,15 @@ const char *kallsyms_lookup(unsigned long addr,

	return NULL;
}

-/* Replace "%s" in format with address, or returns -errno. */ -void __print_symbol(const char *fmt, unsigned long address) +/* Look up a kernel symbol and return it in a text buffer. */ +void sprint_symbol(char *buffer, unsigned long address)

{
	char *modname;
	const char *name;
	unsigned long offset, size;
	char namebuf[KSYM_NAME_LEN+1];

- char buffer[sizeof("%s+%#lx/%#lx [%s]") + KSYM_NAME_LEN +
- 2*(BITS_PER_LONG*3/10) + MODULE_NAME_LEN + 1];

	name = kallsyms_lookup(address, &size, &offset, &modname, namebuf);
-
	if (!name)
		sprintf(buffer, "0x%lx", address);
	else {
@@ -288,6 +285,15 @@ void __print_symbol(const char *fmt, unsigned long address)
		else
			sprintf(buffer, "%s+%#lx/%#lx", name, offset, size);
	}

+}
+
+/* Look up a kernel symbol and print it to the kernel messages. */ +void __print_symbol(const char *fmt, unsigned long address) +{
+ char buffer[KSYM_SYMBOL_LEN];
+
+ sprint_symbol(buffer, address);
+

	printk(fmt, buffer);
}

@@ -452,3 +458,4 @@ static int __init kallsyms_init(void) __initcall(kallsyms_init);

EXPORT_SYMBOL(__print_symbol);
+EXPORT_SYMBOL_GPL(sprint_symbol);

Acked-by
#3 lore raw
Quoted 5 lines · Robert Peterson

[...]
It probably would be easy, but it's beyond the scope of this fix, and therefore probably best left to a separate patch. Feel free to send that out in another patch if you want. If you do, I'd be happy to test it, review it, and ACK it.

Fair enough :)

For what is worth,

Acked-by Paulo Marques <[email protected]>
Signature · 3 lines
Paulo Marques - www.grupopie.com

"Very funny Scotty. Now beam up my clothes."