always store MODULE_VERSION("") data?

always store MODULE_VERSION("") data?

[Matt Domsch]

[-- Attachment #1: Type: text/plain, Size: 653 bytes --]

Rusty,

I started going through the kernel, adding MODULE_VERSION("foo")
everywhere, but there are a lot of modules which are not separately
versioned, and a value of MODULE_VERSION("") would be appropriate.

How hard would it be to always include the space for the
MODULE_VERSION("") data rather than specifying it in each file that
doesn't care, and only modules with their own versioning could put
MODULE_VERSION("myversion") to override the default?

Thanks,
Matt

-- 
Matt Domsch
Sr. Software Engineer, Lead Engineer
Dell Linux Solutions linux.dell.com & www.dell.com/linux
Linux on Dell mailing lists @ http://lists.us.dell.com

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: always store MODULE_VERSION("") data?

[Rusty Russell]

On Wed, 2004-04-28 at 00:58, Matt Domsch wrote:
> Rusty,
> 
> I started going through the kernel, adding MODULE_VERSION("foo")
> everywhere, but there are a lot of modules which are not separately
> versioned, and a value of MODULE_VERSION("") would be appropriate.
> 
> How hard would it be to always include the space for the
> MODULE_VERSION("") data rather than specifying it in each file that
> doesn't care, and only modules with their own versioning could put
> MODULE_VERSION("myversion") to override the default?

Hi Matt,

	If this is desirable, I would prefer to separate "version" and
"srcversion" (or some other name).  This is done in the following patch
(we still mangle RCS-style version strings), for all modules using
MODULE_VERSION, and adds CONFIG_MODULE_SRCVERSION_ALL if you want it in
all modules.

Works here, but these changes tend to break things...
Rusty.

Name: Put Module Source Checksum In "srcversion"
Status: Tested on 2.6.6-rc2-bk5

Matt Domsch <Matt_Domsch@dell.com> writes:
> How hard would it be to always include the space for the
> MODULE_VERSION("") data rather than specifying it in each file that
> doesn't care, and only modules with their own versioning could put
> MODULE_VERSION("myversion") to override the default?

Actually, it's probably best to separate the source checksum from the
rest of the version in this case.  Then a config option will turn them
on for all modules (usually it's only for modules with MODULE_VERSION()).

diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .27380-linux-2.6.6-rc2-bk5/include/linux/module.h .27380-linux-2.6.6-rc2-bk5.updated/include/linux/module.h
--- .27380-linux-2.6.6-rc2-bk5/include/linux/module.h	2004-04-22 08:03:55.000000000 +1000
+++ .27380-linux-2.6.6-rc2-bk5.updated/include/linux/module.h	2004-04-28 15:16:34.000000000 +1000
@@ -140,11 +140,9 @@ extern struct module __this_module;
            customizations, eg "rh3" or "rusty1".
 
   Using this automatically adds a checksum of the .c files and the
-  local headers to the end.  Use MODULE_VERSION("") if you want just
-  this.  Macro includes room for this.
+  local headers in "srcversion".
 */
-#define MODULE_VERSION(_version) \
-  MODULE_INFO(version, _version "\0xxxxxxxxxxxxxxxxxxxxxxxx")
+#define MODULE_VERSION(_version) MODULE_INFO(version, _version)
 
 /* Given an address, look for it in the exception tables */
 const struct exception_table_entry *search_exception_tables(unsigned long add);
diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .27380-linux-2.6.6-rc2-bk5/init/Kconfig .27380-linux-2.6.6-rc2-bk5.updated/init/Kconfig
--- .27380-linux-2.6.6-rc2-bk5/init/Kconfig	2004-04-28 08:53:11.000000000 +1000
+++ .27380-linux-2.6.6-rc2-bk5.updated/init/Kconfig	2004-04-28 16:15:57.000000000 +1000
@@ -329,6 +329,18 @@ config MODVERSIONS
 	  make them incompatible with the kernel you are running.  If
 	  unsure, say N.
 
+config MODULE_SRCVERSION_ALL
+	bool "Source checksum for all modules"
+	depends on MODULES
+	help
+	  Modules which contain a MODULE_VERSION get an extra "srcversion"
+	  field inserting into their modinfo section, which contains a
+    	  sum of the source files which made it.  This helps maintainers
+	  see exactly which source was used to build a module (since
+	  others sometimes change the module source without updating
+	  the version).  With this option, such a "srcversion" field
+	  will be created for all modules.  If unsure, say N.
+
 config KMOD
 	bool "Automatic kernel module loading"
 	depends on MODULES
diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .27380-linux-2.6.6-rc2-bk5/scripts/Makefile.modpost .27380-linux-2.6.6-rc2-bk5.updated/scripts/Makefile.modpost
--- .27380-linux-2.6.6-rc2-bk5/scripts/Makefile.modpost	2004-04-28 08:53:14.000000000 +1000
+++ .27380-linux-2.6.6-rc2-bk5.updated/scripts/Makefile.modpost	2004-04-28 15:40:57.000000000 +1000
@@ -51,6 +51,7 @@ _modpost: $(modules)
 #  Includes step 3,4
 quiet_cmd_modpost = MODPOST
       cmd_modpost = scripts/modpost \
+	$(if $(CONFIG_MODULE_SRCVERSION_ALL),-a,) \
 	$(if $(KBUILD_EXTMOD),-i,-o) $(symverfile) \
 	$(filter-out FORCE,$^)
 
diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .27380-linux-2.6.6-rc2-bk5/scripts/modpost.c .27380-linux-2.6.6-rc2-bk5.updated/scripts/modpost.c
--- .27380-linux-2.6.6-rc2-bk5/scripts/modpost.c	2004-04-28 08:53:14.000000000 +1000
+++ .27380-linux-2.6.6-rc2-bk5.updated/scripts/modpost.c	2004-04-28 16:18:08.000000000 +1000
@@ -1,7 +1,7 @@
 /* Postprocess module symbol versions
  *
  * Copyright 2003       Kai Germaschewski
- *           2002-2003  Rusty Russell, IBM Corporation
+ * Copyright 2002-2004  Rusty Russell, IBM Corporation
  *
  * Based in part on module-init-tools/depmod.c,file2alias
  *
@@ -18,6 +18,8 @@
 int modversions = 0;
 /* Warn about undefined symbols? (do so if we have vmlinux) */
 int have_vmlinux = 0;
+/* Is CONFIG_MODULE_SRCVERSION_ALL set? */
+static int all_versions = 0;
 
 void
 fatal(const char *fmt, ...)
@@ -393,10 +395,44 @@ is_vmlinux(const char *modname)
 	return strcmp(myname, "vmlinux") == 0;
 }
 
+/* Parse tag=value strings from .modinfo section */
+static char *next_string(char *string, unsigned long *secsize)
+{
+	/* Skip non-zero chars */
+	while (string[0]) {
+		string++;
+		if ((*secsize)-- <= 1)
+			return NULL;
+	}
+
+	/* Skip any zero padding. */
+	while (!string[0]) {
+		string++;
+		if ((*secsize)-- <= 1)
+			return NULL;
+	}
+	return string;
+}
+
+static char *get_modinfo(void *modinfo, unsigned long modinfo_len,
+			 const char *tag)
+{
+	char *p;
+	unsigned int taglen = strlen(tag);
+	unsigned long size = modinfo_len;
+
+	for (p = modinfo; p; p = next_string(p, &size)) {
+		if (strncmp(p, tag, taglen) == 0 && p[taglen] == '=')
+			return p + taglen + 1;
+	}
+	return NULL;
+}
+
 void
 read_symbols(char *modname)
 {
 	const char *symname;
+	char *version;
 	struct module *mod;
 	struct elf_info info = { };
 	Elf_Sym *sym;
@@ -423,8 +459,15 @@ read_symbols(char *modname)
 		handle_modversions(mod, &info, sym, symname);
 		handle_moddevtable(mod, &info, sym, symname);
 	}
-	maybe_frob_version(modname, info.modinfo, info.modinfo_len,
-			   (void *)info.modinfo - (void *)info.hdr);
+
+	version = get_modinfo(info.modinfo, info.modinfo_len, "version");
+	if (version)
+		maybe_frob_rcs_version(modname, version, info.modinfo,
+				       version - (char *)info.hdr);
+	if (version || (all_versions && !is_vmlinux(modname)))
+		get_src_version(modname, mod->srcversion,
+				sizeof(mod->srcversion)-1);
+
 	parse_elf_finish(&info);
 
 	/* Our trick to get versioning for struct_module - it's
@@ -574,6 +617,16 @@ add_depends(struct buffer *b, struct mod
 }
 
 void
+add_srcversion(struct buffer *b, struct module *mod)
+{
+	if (mod->srcversion[0]) {
+		buf_printf(b, "\n");
+		buf_printf(b, "MODULE_INFO(srcversion, \"%s\");\n",
+			   mod->srcversion);
+	}
+}
+
+void
 write_if_changed(struct buffer *b, const char *fname)
 {
 	char *tmp;
@@ -695,7 +748,7 @@ main(int argc, char **argv)
 	char *dump_read = NULL, *dump_write = NULL;
 	int opt;
 
-	while ((opt = getopt(argc, argv, "i:o:")) != -1) {
+	while ((opt = getopt(argc, argv, "i:o:a")) != -1) {
 		switch(opt) {
 			case 'i':
 				dump_read = optarg;
@@ -703,6 +756,9 @@ main(int argc, char **argv)
 			case 'o':
 				dump_write = optarg;
 				break;
+			case 'a':
+				all_versions = 1;
+				break;
 			default:
 				exit(1);
 		}
@@ -725,6 +781,7 @@ main(int argc, char **argv)
 		add_versions(&buf, mod);
 		add_depends(&buf, mod, modules);
 		add_moddevtable(&buf, mod);
+		add_srcversion(&buf, mod);
 
 		sprintf(fname, "%s.mod.c", mod->name);
 		write_if_changed(&buf, fname);
diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .27380-linux-2.6.6-rc2-bk5/scripts/modpost.h .27380-linux-2.6.6-rc2-bk5.updated/scripts/modpost.h
--- .27380-linux-2.6.6-rc2-bk5/scripts/modpost.h	2004-04-22 08:04:10.000000000 +1000
+++ .27380-linux-2.6.6-rc2-bk5.updated/scripts/modpost.h	2004-04-28 15:39:00.000000000 +1000
@@ -75,6 +75,7 @@ struct module {
 	int seen;
 	int skip;
 	struct buffer dev_table_buf;
+	char	     srcversion[25];
 };
 
 struct elf_info {
@@ -93,10 +94,11 @@ void handle_moddevtable(struct module *m
 
 void add_moddevtable(struct buffer *buf, struct module *mod);
 
-void maybe_frob_version(const char *modfilename,
-			void *modinfo,
-			unsigned long modinfo_len,
-			unsigned long modinfo_offset);
+void maybe_frob_rcs_version(const char *modfilename,
+			    char *version,
+			    void *modinfo,
+			    unsigned long modinfo_offset);
+void get_src_version(const char *modname, char sum[], unsigned sumlen);
 
 void *grab_file(const char *filename, unsigned long *size);
 char* get_next_line(unsigned long *pos, void *file, unsigned long size);
diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .27380-linux-2.6.6-rc2-bk5/scripts/sumversion.c .27380-linux-2.6.6-rc2-bk5.updated/scripts/sumversion.c
--- .27380-linux-2.6.6-rc2-bk5/scripts/sumversion.c	2004-04-05 09:04:50.000000000 +1000
+++ .27380-linux-2.6.6-rc2-bk5.updated/scripts/sumversion.c	2004-04-28 15:39:17.000000000 +1000
@@ -5,39 +5,6 @@
 #include <string.h>
 #include "modpost.h"
 
-/* Parse tag=value strings from .modinfo section */
-static char *next_string(char *string, unsigned long *secsize)
-{
-	/* Skip non-zero chars */
-	while (string[0]) {
-		string++;
-		if ((*secsize)-- <= 1)
-			return NULL;
-	}
-
-	/* Skip any zero padding. */
-	while (!string[0]) {
-		string++;
-		if ((*secsize)-- <= 1)
-			return NULL;
-	}
-	return string;
-}
-
-static char *get_modinfo(void *modinfo, unsigned long modinfo_len,
-			 const char *tag)
-{
-	char *p;
-	unsigned int taglen = strlen(tag);
-	unsigned long size = modinfo_len;
-
-	for (p = modinfo; p; p = next_string(p, &size)) {
-		if (strncmp(p, tag, taglen) == 0 && p[taglen] == '=')
-			return p + taglen + 1;
-	}
-	return NULL;
-}
-
 /*
  * Stolen form Cryptographic API.
  *
@@ -404,11 +370,11 @@ out:
 	return ret;
 }
 
-static int get_version(const char *modname, char sum[])
+/* Calc and record src checksum. */
+void get_src_version(const char *modname, char sum[], unsigned sumlen)
 {
 	void *file;
 	unsigned long len;
-	int ret = 0;
 	struct md4_ctx md;
 	char *sources, *end, *fname;
 	const char *basename;
@@ -428,7 +394,7 @@ static int get_version(const char *modna
 	if (!file) {
 		fprintf(stderr, "Warning: could not find versions for %s\n",
 			filelist);
-		return 0;
+		return;
 	}
 
 	sources = strchr(file, '\n');
@@ -453,12 +419,9 @@ static int get_version(const char *modna
 			goto release;
 	}
 
-	/* sum is of form \0<padding>. */
-	md4_final_ascii(&md, sum, 1 + strlen(sum+1));
-	ret = 1;
+	md4_final_ascii(&md, sum, sumlen);
 release:
 	release_file(file, len);
-	return ret;
 }
 
 static void write_version(const char *filename, const char *sum,
@@ -488,12 +451,12 @@ out:
 	close(fd);
 }
 
-void strip_rcs_crap(char *version)
+static int strip_rcs_crap(char *version)
 {
 	unsigned int len, full_len;
 
 	if (strncmp(version, "$Revision", strlen("$Revision")) != 0)
-		return;
+		return 0;
 
 	/* Space for version string follows. */
 	full_len = strlen(version) + strlen(version + strlen(version) + 1) + 2;
@@ -514,31 +477,15 @@ void strip_rcs_crap(char *version)
 		len++;
 	memmove(version + len, version + strlen(version),
 		full_len - strlen(version));
+	return 1;
 }
 
-/* If the modinfo contains a "version" value, then set this. */
-void maybe_frob_version(const char *modfilename,
-			void *modinfo,
-			unsigned long modinfo_len,
-			unsigned long modinfo_offset)
+/* Clean up RCS-style version numbers. */
+void maybe_frob_rcs_version(const char *modfilename,
+			    char *version,
+			    void *modinfo,
+			    unsigned long version_offset)
 {
-	char *version, *csum;
-
-	version = get_modinfo(modinfo, modinfo_len, "version");
-	if (!version)
-		return;
-
-	/* RCS $Revision gets stripped out. */
-	strip_rcs_crap(version);
-
-	/* Check against double sumversion */
-	if (strchr(version, ' '))
-		return;
-
-	/* Version contains embedded NUL: second half has space for checksum */
-	csum = version + strlen(version);
-	*(csum++) = ' ';
-	if (get_version(modfilename, csum))
-		write_version(modfilename, version,
-			      modinfo_offset + (version - (char *)modinfo));
+	if (strip_rcs_crap(version))
+		write_version(modfilename, version, version_offset);
 }

-- 
Anyone who quotes me in their signature is an idiot -- Rusty Russell

Re: always store MODULE_VERSION("") data?

[Matt Domsch]

[-- Attachment #1: Type: text/plain, Size: 1235 bytes --]

On Thu, Apr 29, 2004 at 10:55:22AM +1000, Rusty Russell wrote:
> On Wed, 2004-04-28 at 00:58, Matt Domsch wrote:
> > How hard would it be to always include the space for the
> > MODULE_VERSION("") data rather than specifying it in each file that
> > doesn't care, and only modules with their own versioning could put
> > MODULE_VERSION("myversion") to override the default?
> 
> 	If this is desirable, I would prefer to separate "version" and
> "srcversion" (or some other name).  This is done in the following patch
> (we still mangle RCS-style version strings), for all modules using
> MODULE_VERSION, and adds CONFIG_MODULE_SRCVERSION_ALL if you want it in
> all modules.
> 
> Works here, but these changes tend to break things...

Works for me too.  All modules regardless if if they have a
MODULE_VERSION() entry get the srcversion field in .modinfo.  If they
also have a MODULE_VERSION(), that data shows up in .modinfo also.
This is exactly what I wanted.

Now to find GregKH's patch to export this stuff via sysfs...

Thanks,
Matt

-- 
Matt Domsch
Sr. Software Engineer, Lead Engineer
Dell Linux Solutions linux.dell.com & www.dell.com/linux
Linux on Dell mailing lists @ http://lists.us.dell.com

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: always store MODULE_VERSION("") data?

[Greg KH]

On Thu, Apr 29, 2004 at 09:55:58PM -0500, Matt Domsch wrote:
> 
> Now to find GregKH's patch to export this stuff via sysfs...

Heh, it's in the archive.  I really need to spend the time and dig it up
and get it working again better.  Rusty sent the last version to me, and
hopefully I'll get to it next week...

thanks,

greg k-h

Re: always store MODULE_VERSION("") data?

[Matt Domsch]

On Thu, Apr 29, 2004 at 09:55:58PM -0500, Matt Domsch wrote:
> On Thu, Apr 29, 2004 at 10:55:22AM +1000, Rusty Russell wrote:
> > On Wed, 2004-04-28 at 00:58, Matt Domsch wrote:
> > > How hard would it be to always include the space for the
> > > MODULE_VERSION("") data rather than specifying it in each file that
> > > doesn't care, and only modules with their own versioning could put
> > > MODULE_VERSION("myversion") to override the default?
> > 
> > 	If this is desirable, I would prefer to separate "version" and
> > "srcversion" (or some other name).  This is done in the following patch
> > (we still mangle RCS-style version strings), for all modules using
> > MODULE_VERSION, and adds CONFIG_MODULE_SRCVERSION_ALL if you want it in
> > all modules.
> > 
> > Works here, but these changes tend to break things...
> 
> Works for me too.  All modules regardless if if they have a
> MODULE_VERSION() entry get the srcversion field in .modinfo.  If they
> also have a MODULE_VERSION(), that data shows up in .modinfo also.
> This is exactly what I wanted.

Rusty, digging up the long forgotten past here.  This works for me.  I
re-diffed your patch to 2.6.9-rc, updated patch attached.  Please
review and submit.

Signed-off-by: Matt Domsch

-- 
Matt Domsch
Sr. Software Engineer, Lead Engineer
Dell Linux Solutions linux.dell.com & www.dell.com/linux
Linux on Dell mailing lists @ http://lists.us.dell.com

===== include/linux/module.h 1.79 vs edited =====
--- 1.79/include/linux/module.h	2004-06-27 02:19:28 -05:00
+++ edited/include/linux/module.h	2004-09-03 17:04:05 -05:00
@@ -141,11 +141,9 @@
            customizations, eg "rh3" or "rusty1".
 
   Using this automatically adds a checksum of the .c files and the
-  local headers to the end.  Use MODULE_VERSION("") if you want just
-  this.  Macro includes room for this.
+  local headers in "srcversion".
 */
-#define MODULE_VERSION(_version) \
-  MODULE_INFO(version, _version "\0xxxxxxxxxxxxxxxxxxxxxxxx")
+#define MODULE_VERSION(_version) MODULE_INFO(version, _version)
 
 /* Given an address, look for it in the exception tables */
 const struct exception_table_entry *search_exception_tables(unsigned long add);
===== init/Kconfig 1.48 vs edited =====
--- 1.48/init/Kconfig	2004-08-31 03:00:08 -05:00
+++ edited/init/Kconfig	2004-09-03 17:01:00 -05:00
@@ -360,6 +360,18 @@
 	  make them incompatible with the kernel you are running.  If
 	  unsure, say N.
 
+config MODULE_SRCVERSION_ALL
+	bool "Source checksum for all modules"
+	depends on MODULES
+	help
+	  Modules which contain a MODULE_VERSION get an extra "srcversion"
+	  field inserting into their modinfo section, which contains a
+    	  sum of the source files which made it.  This helps maintainers
+	  see exactly which source was used to build a module (since
+	  others sometimes change the module source without updating
+	  the version).  With this option, such a "srcversion" field
+	  will be created for all modules.  If unsure, say N.
+
 config KMOD
 	bool "Automatic kernel module loading"
 	depends on MODULES
===== scripts/Makefile.modpost 1.13 vs edited =====
--- 1.13/scripts/Makefile.modpost	2004-08-15 04:54:12 -05:00
+++ edited/scripts/Makefile.modpost	2004-09-03 16:48:05 -05:00
@@ -52,6 +52,7 @@
 quiet_cmd_modpost = MODPOST
       cmd_modpost = scripts/mod/modpost            \
         $(if $(CONFIG_MODVERSIONS),-m)             \
+	$(if $(CONFIG_MODULE_SRCVERSION_ALL),-a,)  \
 	$(if $(KBUILD_EXTMOD),-i,-o) $(symverfile) \
 	$(filter-out FORCE,$^)
 
===== scripts/mod/modpost.c 1.32 vs edited =====
--- 1.32/scripts/mod/modpost.c	2004-08-15 04:54:12 -05:00
+++ edited/scripts/mod/modpost.c	2004-09-03 16:44:19 -05:00
@@ -1,7 +1,7 @@
 /* Postprocess module symbol versions
  *
  * Copyright 2003       Kai Germaschewski
- *           2002-2003  Rusty Russell, IBM Corporation
+ * Copyright 2002-2004  Rusty Russell, IBM Corporation
  *
  * Based in part on module-init-tools/depmod.c,file2alias
  *
@@ -18,6 +18,8 @@
 int modversions = 0;
 /* Warn about undefined symbols? (do so if we have vmlinux) */
 int have_vmlinux = 0;
+/* Is CONFIG_MODULE_SRCVERSION_ALL set? */
+static int all_versions = 0;
 
 void
 fatal(const char *fmt, ...)
@@ -393,10 +395,44 @@
 	return strcmp(myname, "vmlinux") == 0;
 }
 
+/* Parse tag=value strings from .modinfo section */
+static char *next_string(char *string, unsigned long *secsize)
+{
+	/* Skip non-zero chars */
+	while (string[0]) {
+		string++;
+		if ((*secsize)-- <= 1)
+			return NULL;
+	}
+
+	/* Skip any zero padding. */
+	while (!string[0]) {
+		string++;
+		if ((*secsize)-- <= 1)
+			return NULL;
+	}
+	return string;
+}
+
+static char *get_modinfo(void *modinfo, unsigned long modinfo_len,
+			 const char *tag)
+{
+	char *p;
+	unsigned int taglen = strlen(tag);
+	unsigned long size = modinfo_len;
+
+	for (p = modinfo; p; p = next_string(p, &size)) {
+		if (strncmp(p, tag, taglen) == 0 && p[taglen] == '=')
+			return p + taglen + 1;
+	}
+	return NULL;
+}
+
 void
 read_symbols(char *modname)
 {
 	const char *symname;
+	char *version;
 	struct module *mod;
 	struct elf_info info = { };
 	Elf_Sym *sym;
@@ -423,8 +459,15 @@
 		handle_modversions(mod, &info, sym, symname);
 		handle_moddevtable(mod, &info, sym, symname);
 	}
-	maybe_frob_version(modname, info.modinfo, info.modinfo_len,
-			   (void *)info.modinfo - (void *)info.hdr);
+
+	version = get_modinfo(info.modinfo, info.modinfo_len, "version");
+	if (version)
+		maybe_frob_rcs_version(modname, version, info.modinfo,
+				       version - (char *)info.hdr);
+	if (version || (all_versions && !is_vmlinux(modname)))
+		get_src_version(modname, mod->srcversion,
+				sizeof(mod->srcversion)-1);
+
 	parse_elf_finish(&info);
 
 	/* Our trick to get versioning for struct_module - it's
@@ -574,6 +617,16 @@
 }
 
 void
+add_srcversion(struct buffer *b, struct module *mod)
+{
+	if (mod->srcversion[0]) {
+		buf_printf(b, "\n");
+		buf_printf(b, "MODULE_INFO(srcversion, \"%s\");\n",
+			   mod->srcversion);
+	}
+}
+
+void
 write_if_changed(struct buffer *b, const char *fname)
 {
 	char *tmp;
@@ -694,7 +747,7 @@
 	char *dump_read = NULL, *dump_write = NULL;
 	int opt;
 
-	while ((opt = getopt(argc, argv, "i:mo:")) != -1) {
+	while ((opt = getopt(argc, argv, "i:mo:a")) != -1) {
 		switch(opt) {
 			case 'i':
 				dump_read = optarg;
@@ -705,6 +758,9 @@
 			case 'o':
 				dump_write = optarg;
 				break;
+			case 'a':
+				all_versions = 1;
+				break;
 			default:
 				exit(1);
 		}
@@ -727,6 +783,7 @@
 		add_versions(&buf, mod);
 		add_depends(&buf, mod, modules);
 		add_moddevtable(&buf, mod);
+		add_srcversion(&buf, mod);
 
 		sprintf(fname, "%s.mod.c", mod->name);
 		write_if_changed(&buf, fname);
===== scripts/mod/modpost.h 1.7 vs edited =====
--- 1.7/scripts/mod/modpost.h	2004-07-22 17:51:17 -05:00
+++ edited/scripts/mod/modpost.h	2004-09-03 16:45:05 -05:00
@@ -75,6 +75,7 @@
 	int seen;
 	int skip;
 	struct buffer dev_table_buf;
+	char	     srcversion[25];
 };
 
 struct elf_info {
@@ -93,10 +94,11 @@
 
 void add_moddevtable(struct buffer *buf, struct module *mod);
 
-void maybe_frob_version(const char *modfilename,
-			void *modinfo,
-			unsigned long modinfo_len,
-			unsigned long modinfo_offset);
+void maybe_frob_rcs_version(const char *modfilename,
+			    char *version,
+			    void *modinfo,
+			    unsigned long modinfo_offset);
+void get_src_version(const char *modname, char sum[], unsigned sumlen);
 
 void *grab_file(const char *filename, unsigned long *size);
 char* get_next_line(unsigned long *pos, void *file, unsigned long size);
===== scripts/mod/sumversion.c 1.6 vs edited =====
--- 1.6/scripts/mod/sumversion.c	2004-08-24 04:08:48 -05:00
+++ edited/scripts/mod/sumversion.c	2004-09-03 16:43:33 -05:00
@@ -5,39 +5,6 @@
 #include <string.h>
 #include "modpost.h"
 
-/* Parse tag=value strings from .modinfo section */
-static char *next_string(char *string, unsigned long *secsize)
-{
-	/* Skip non-zero chars */
-	while (string[0]) {
-		string++;
-		if ((*secsize)-- <= 1)
-			return NULL;
-	}
-
-	/* Skip any zero padding. */
-	while (!string[0]) {
-		string++;
-		if ((*secsize)-- <= 1)
-			return NULL;
-	}
-	return string;
-}
-
-static char *get_modinfo(void *modinfo, unsigned long modinfo_len,
-			 const char *tag)
-{
-	char *p;
-	unsigned int taglen = strlen(tag);
-	unsigned long size = modinfo_len;
-
-	for (p = modinfo; p; p = next_string(p, &size)) {
-		if (strncmp(p, tag, taglen) == 0 && p[taglen] == '=')
-			return p + taglen + 1;
-	}
-	return NULL;
-}
-
 /*
  * Stolen form Cryptographic API.
  *
@@ -404,11 +371,11 @@
 	return ret;
 }
 
-static int get_version(const char *modname, char sum[])
+/* Calc and record src checksum. */
+void get_src_version(const char *modname, char sum[], unsigned sumlen)
 {
 	void *file;
 	unsigned long len;
-	int ret = 0;
 	struct md4_ctx md;
 	char *sources, *end, *fname;
 	const char *basename;
@@ -428,7 +395,7 @@
 	if (!file) {
 		fprintf(stderr, "Warning: could not find versions for %s\n",
 			filelist);
-		return 0;
+		return;
 	}
 
 	sources = strchr(file, '\n');
@@ -453,12 +420,9 @@
 			goto release;
 	}
 
-	/* sum is of form \0<padding>. */
-	md4_final_ascii(&md, sum, 1 + strlen(sum+1));
-	ret = 1;
+	md4_final_ascii(&md, sum, sumlen);
 release:
 	release_file(file, len);
-	return ret;
 }
 
 static void write_version(const char *filename, const char *sum,
@@ -488,12 +452,12 @@
 	close(fd);
 }
 
-void strip_rcs_crap(char *version)
+static int strip_rcs_crap(char *version)
 {
 	unsigned int len, full_len;
 
 	if (strncmp(version, "$Revision", strlen("$Revision")) != 0)
-		return;
+		return 0;
 
 	/* Space for version string follows. */
 	full_len = strlen(version) + strlen(version + strlen(version) + 1) + 2;
@@ -514,31 +478,15 @@
 		len++;
 	memmove(version + len, version + strlen(version),
 		full_len - strlen(version));
+	return 1;
 }
 
-/* If the modinfo contains a "version" value, then set this. */
-void maybe_frob_version(const char *modfilename,
-			void *modinfo,
-			unsigned long modinfo_len,
-			unsigned long modinfo_offset)
+/* Clean up RCS-style version numbers. */
+void maybe_frob_rcs_version(const char *modfilename,
+			    char *version,
+			    void *modinfo,
+			    unsigned long version_offset)
 {
-	char *version, *csum;
-
-	version = get_modinfo(modinfo, modinfo_len, "version");
-	if (!version)
-		return;
-
-	/* RCS $Revision gets stripped out. */
-	strip_rcs_crap(version);
-
-	/* Check against double sumversion */
-	if (strchr(version, ' '))
-		return;
-
-	/* Version contains embedded NUL: second half has space for checksum */
-	csum = version + strlen(version);
-	*(csum++) = ' ';
-	if (get_version(modfilename, csum))
-		write_version(modfilename, version,
-			      modinfo_offset + (version - (char *)modinfo));
+	if (strip_rcs_crap(version))
+		write_version(modfilename, version, version_offset);
 }

Re: always store MODULE_VERSION("") data?

[Sam Ravnborg]

On Thu, Apr 29, 2004 at 10:55:22AM +1000, Rusty Russell wrote:
>  
> +config MODULE_SRCVERSION_ALL
> +	bool "Source checksum for all modules"
> +	depends on MODULES
> +	help
> +	  Modules which contain a MODULE_VERSION get an extra "srcversion"
> +	  field inserting into their modinfo section, which contains a
> +    	  sum of the source files which made it.  This helps maintainers
> +	  see exactly which source was used to build a module (since
> +	  others sometimes change the module source without updating
> +	  the version).  With this option, such a "srcversion" field
> +	  will be created for all modules.  If unsure, say N.

I had to read the above twice to get the fact that it was added to all modules regardless.
Move the second last sentence first, so the explanation comes after?
Any reason not to enable it pr. default, at least to give it some exposure?

>  void
> +add_srcversion(struct buffer *b, struct module *mod)
> +{
> +	if (mod->srcversion[0]) {

Why not checking the flag?

	Sam

Re: always store MODULE_VERSION("") data?

[Rusty Russell]

On Mon, 2004-05-03 at 04:31, Sam Ravnborg wrote:
> On Thu, Apr 29, 2004 at 10:55:22AM +1000, Rusty Russell wrote:
> >  
> > +config MODULE_SRCVERSION_ALL
> > +	bool "Source checksum for all modules"
> > +	depends on MODULES
> > +	help
> > +	  Modules which contain a MODULE_VERSION get an extra "srcversion"
> > +	  field inserting into their modinfo section, which contains a
> > +    	  sum of the source files which made it.  This helps maintainers
> > +	  see exactly which source was used to build a module (since
> > +	  others sometimes change the module source without updating
> > +	  the version).  With this option, such a "srcversion" field
> > +	  will be created for all modules.  If unsure, say N.
> 
> I had to read the above twice to get the fact that it was added to all modules regardless.
> Move the second last sentence first, so the explanation comes after?
> Any reason not to enable it pr. default, at least to give it some exposure?

Good question.  I didn't want to slow the build process, but measuring
here gives no real difference.  My original feeling was that if the
author doesn't care enough to put a version string in, then usually
they're relying on the "oh, you're using 2.6.5" method and won't use
srcversion either.

Patch below makes it unconditional.

Name: Put Module Source Checksum In "srcversion"
Status: Booted on 2.6.6-rc3-bk4

Matt Domsch <Matt_Domsch@dell.com> writes:
> How hard would it be to always include the space for the
> MODULE_VERSION("") data rather than specifying it in each file that
> doesn't care, and only modules with their own versioning could put
> MODULE_VERSION("myversion") to override the default?

Actually, it's probably best to separate the source checksum from the
rest of the version, and do it for all modules: it's so fast it has no
measurable effect on build times here.

diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .27380-linux-2.6.6-rc2-bk5/include/linux/module.h .27380-linux-2.6.6-rc2-bk5.updated/include/linux/module.h
--- .27380-linux-2.6.6-rc2-bk5/include/linux/module.h	2004-04-22 08:03:55.000000000 +1000
+++ .27380-linux-2.6.6-rc2-bk5.updated/include/linux/module.h	2004-04-28 15:16:34.000000000 +1000
@@ -140,11 +140,9 @@ extern struct module __this_module;
            customizations, eg "rh3" or "rusty1".
 
-  Using this automatically adds a checksum of the .c files and the
-  local headers to the end.  Use MODULE_VERSION("") if you want just
-  this.  Macro includes room for this.
+  Note that the checksum of the .c file and local headers is always
+  included in a modules "srcversion" modinfo field.
 */
-#define MODULE_VERSION(_version) \
-  MODULE_INFO(version, _version "\0xxxxxxxxxxxxxxxxxxxxxxxx")
+#define MODULE_VERSION(_version) MODULE_INFO(version, _version)
 
 /* Given an address, look for it in the exception tables */
 const struct exception_table_entry *search_exception_tables(unsigned long add);
diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .27380-linux-2.6.6-rc2-bk5/scripts/modpost.c .27380-linux-2.6.6-rc2-bk5.updated/scripts/modpost.c
--- .27380-linux-2.6.6-rc2-bk5/scripts/modpost.c	2004-04-28 08:53:14.000000000 +1000
+++ .27380-linux-2.6.6-rc2-bk5.updated/scripts/modpost.c	2004-04-28 16:18:08.000000000 +1000
@@ -1,7 +1,7 @@
 /* Postprocess module symbol versions
  *
  * Copyright 2003       Kai Germaschewski
- *           2002-2003  Rusty Russell, IBM Corporation
+ * Copyright 2002-2004  Rusty Russell, IBM Corporation
  *
  * Based in part on module-init-tools/depmod.c,file2alias
  *
@@ -393,10 +395,44 @@ is_vmlinux(const char *modname)
 	return strcmp(myname, "vmlinux") == 0;
 }
 
+/* Parse tag=value strings from .modinfo section */
+static char *next_string(char *string, unsigned long *secsize)
+{
+	/* Skip non-zero chars */
+	while (string[0]) {
+		string++;
+		if ((*secsize)-- <= 1)
+			return NULL;
+	}
+
+	/* Skip any zero padding. */
+	while (!string[0]) {
+		string++;
+		if ((*secsize)-- <= 1)
+			return NULL;
+	}
+	return string;
+}
+
+static char *get_modinfo(void *modinfo, unsigned long modinfo_len,
+			 const char *tag)
+{
+	char *p;
+	unsigned int taglen = strlen(tag);
+	unsigned long size = modinfo_len;
+
+	for (p = modinfo; p; p = next_string(p, &size)) {
+		if (strncmp(p, tag, taglen) == 0 && p[taglen] == '=')
+			return p + taglen + 1;
+	}
+	return NULL;
+}
+
 void
 read_symbols(char *modname)
 {
 	const char *symname;
+	char *version;
 	struct module *mod;
 	struct elf_info info = { };
 	Elf_Sym *sym;
@@ -423,8 +459,15 @@ read_symbols(char *modname)
 		handle_modversions(mod, &info, sym, symname);
 		handle_moddevtable(mod, &info, sym, symname);
 	}
-	maybe_frob_version(modname, info.modinfo, info.modinfo_len,
-			   (void *)info.modinfo - (void *)info.hdr);
+
+	version = get_modinfo(info.modinfo, info.modinfo_len, "version");
+	if (version)
+		maybe_frob_rcs_version(modname, version, info.modinfo,
+				       version - (char *)info.hdr);
+	if (!is_vmlinux(modname))
+		get_src_version(modname, mod->srcversion,
+				sizeof(mod->srcversion)-1);
+
 	parse_elf_finish(&info);
 
 	/* Our trick to get versioning for struct_module - it's
@@ -574,6 +617,14 @@ add_depends(struct buffer *b, struct mod
 }
 
 void
+add_srcversion(struct buffer *b, struct module *mod)
+{
+	buf_printf(b, "\n");
+	buf_printf(b, "MODULE_INFO(srcversion, \"%s\");\n",
+		   mod->srcversion);
+}
+
+void
 write_if_changed(struct buffer *b, const char *fname)
 {
 	char *tmp;
@@ -725,6 +781,7 @@ main(int argc, char **argv)
 		add_versions(&buf, mod);
 		add_depends(&buf, mod, modules);
 		add_moddevtable(&buf, mod);
+		add_srcversion(&buf, mod);
 
 		sprintf(fname, "%s.mod.c", mod->name);
 		write_if_changed(&buf, fname);
diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .27380-linux-2.6.6-rc2-bk5/scripts/modpost.h .27380-linux-2.6.6-rc2-bk5.updated/scripts/modpost.h
--- .27380-linux-2.6.6-rc2-bk5/scripts/modpost.h	2004-04-22 08:04:10.000000000 +1000
+++ .27380-linux-2.6.6-rc2-bk5.updated/scripts/modpost.h	2004-04-28 15:39:00.000000000 +1000
@@ -75,6 +75,7 @@ struct module {
 	int seen;
 	int skip;
 	struct buffer dev_table_buf;
+	char	     srcversion[25];
 };
 
 struct elf_info {
@@ -93,10 +94,11 @@ void handle_moddevtable(struct module *m
 
 void add_moddevtable(struct buffer *buf, struct module *mod);
 
-void maybe_frob_version(const char *modfilename,
-			void *modinfo,
-			unsigned long modinfo_len,
-			unsigned long modinfo_offset);
+void maybe_frob_rcs_version(const char *modfilename,
+			    char *version,
+			    void *modinfo,
+			    unsigned long modinfo_offset);
+void get_src_version(const char *modname, char sum[], unsigned sumlen);
 
 void *grab_file(const char *filename, unsigned long *size);
 char* get_next_line(unsigned long *pos, void *file, unsigned long size);
diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .27380-linux-2.6.6-rc2-bk5/scripts/sumversion.c .27380-linux-2.6.6-rc2-bk5.updated/scripts/sumversion.c
--- .27380-linux-2.6.6-rc2-bk5/scripts/sumversion.c	2004-04-05 09:04:50.000000000 +1000
+++ .27380-linux-2.6.6-rc2-bk5.updated/scripts/sumversion.c	2004-04-28 15:39:17.000000000 +1000
@@ -5,39 +5,6 @@
 #include <string.h>
 #include "modpost.h"
 
-/* Parse tag=value strings from .modinfo section */
-static char *next_string(char *string, unsigned long *secsize)
-{
-	/* Skip non-zero chars */
-	while (string[0]) {
-		string++;
-		if ((*secsize)-- <= 1)
-			return NULL;
-	}
-
-	/* Skip any zero padding. */
-	while (!string[0]) {
-		string++;
-		if ((*secsize)-- <= 1)
-			return NULL;
-	}
-	return string;
-}
-
-static char *get_modinfo(void *modinfo, unsigned long modinfo_len,
-			 const char *tag)
-{
-	char *p;
-	unsigned int taglen = strlen(tag);
-	unsigned long size = modinfo_len;
-
-	for (p = modinfo; p; p = next_string(p, &size)) {
-		if (strncmp(p, tag, taglen) == 0 && p[taglen] == '=')
-			return p + taglen + 1;
-	}
-	return NULL;
-}
-
 /*
  * Stolen form Cryptographic API.
  *
@@ -404,11 +370,11 @@ out:
 	return ret;
 }
 
-static int get_version(const char *modname, char sum[])
+/* Calc and record src checksum. */
+void get_src_version(const char *modname, char sum[], unsigned sumlen)
 {
 	void *file;
 	unsigned long len;
-	int ret = 0;
 	struct md4_ctx md;
 	char *sources, *end, *fname;
 	const char *basename;
@@ -428,7 +394,7 @@ static int get_version(const char *modna
 	if (!file) {
 		fprintf(stderr, "Warning: could not find versions for %s\n",
 			filelist);
-		return 0;
+		return;
 	}
 
 	sources = strchr(file, '\n');
@@ -453,12 +419,9 @@ static int get_version(const char *modna
 			goto release;
 	}
 
-	/* sum is of form \0<padding>. */
-	md4_final_ascii(&md, sum, 1 + strlen(sum+1));
-	ret = 1;
+	md4_final_ascii(&md, sum, sumlen);
 release:
 	release_file(file, len);
-	return ret;
 }
 
 static void write_version(const char *filename, const char *sum,
@@ -488,12 +451,12 @@ out:
 	close(fd);
 }
 
-void strip_rcs_crap(char *version)
+static int strip_rcs_crap(char *version)
 {
 	unsigned int len, full_len;
 
 	if (strncmp(version, "$Revision", strlen("$Revision")) != 0)
-		return;
+		return 0;
 
 	/* Space for version string follows. */
 	full_len = strlen(version) + strlen(version + strlen(version) + 1) + 2;
@@ -514,31 +477,15 @@ void strip_rcs_crap(char *version)
 		len++;
 	memmove(version + len, version + strlen(version),
 		full_len - strlen(version));
+	return 1;
 }
 
-/* If the modinfo contains a "version" value, then set this. */
-void maybe_frob_version(const char *modfilename,
-			void *modinfo,
-			unsigned long modinfo_len,
-			unsigned long modinfo_offset)
+/* Clean up RCS-style version numbers. */
+void maybe_frob_rcs_version(const char *modfilename,
+			    char *version,
+			    void *modinfo,
+			    unsigned long version_offset)
 {
-	char *version, *csum;
-
-	version = get_modinfo(modinfo, modinfo_len, "version");
-	if (!version)
-		return;
-
-	/* RCS $Revision gets stripped out. */
-	strip_rcs_crap(version);
-
-	/* Check against double sumversion */
-	if (strchr(version, ' '))
-		return;
-
-	/* Version contains embedded NUL: second half has space for checksum */
-	csum = version + strlen(version);
-	*(csum++) = ' ';
-	if (get_version(modfilename, csum))
-		write_version(modfilename, version,
-			      modinfo_offset + (version - (char *)modinfo));
+	if (strip_rcs_crap(version))
+		write_version(modfilename, version, version_offset);
 }

-- 
Anyone who quotes me in their signature is an idiot -- Rusty Russell