[PATCH] INITRAMFS: Add option to preserve mtime from INITRAMFS cpio images

[PATCH] INITRAMFS: Add option to preserve mtime from INITRAMFS cpio images

[Nye Liu]

From: Nye Liu <nyet@nyet.org>

When unpacking the cpio into the initramfs, mtimes are not preserved by
default. This patch adds an INITRAMFS_PRESERVE_MTIME option that allows
mtimes stored in the cpio image to be used when constructing the
initramfs. For embedded applications that run exclusively out of the
initramfs, this is invaluable.

Signed-off-by: Nye Liu <nyet@nyet.org>

---

diff --git a/init/initramfs.c b/init/initramfs.c
index 644fc01..0dd0a73 100644
--- a/init/initramfs.c
+++ b/init/initramfs.c
@@ -7,6 +7,12 @@
 #include <linux/string.h>
 #include <linux/syscalls.h>
 
+#ifdef CONFIG_INITRAMFS_PRESERVE_MTIME
+#include <linux/utime.h>
+#include <linux/namei.h>
+#include <linux/list.h>
+#endif
+
 static __initdata char *message;
 static void __init error(char *x)
 {
@@ -72,6 +78,38 @@ static void __init free_hash(void)
 	}
 }
 
+#ifdef CONFIG_INITRAMFS_PRESERVE_MTIME
+static __initdata LIST_HEAD(dir_list);
+struct dir_entry {
+	struct list_head list;
+	char *name;
+	struct utimbuf mtime;
+};
+
+static void __init dir_add(const char *name, struct utimbuf mtime)
+{
+	struct dir_entry *de = kmalloc(sizeof(struct dir_entry), GFP_KERNEL);
+	INIT_LIST_HEAD(&de->list);
+	de->name = kstrdup(name, GFP_KERNEL);
+	de->mtime = mtime;
+	list_add(&de->list, &dir_list);
+}
+
+static void __init dir_utime(void)
+{
+	struct list_head *e, *tmp;
+	list_for_each_safe(e, tmp, &dir_list) {
+		struct dir_entry *de = list_entry(e, struct dir_entry, list);
+		list_del(e);
+		sys_utime(de->name, &de->mtime);
+		kfree(de->name);
+		kfree(de);
+	}
+}
+
+static __initdata struct utimbuf mtime;
+#endif
+
 /* cpio header parsing */
 
 static __initdata unsigned long ino, major, minor, nlink;
@@ -97,6 +135,9 @@ static void __init parse_header(char *s)
 	uid = parsed[2];
 	gid = parsed[3];
 	nlink = parsed[4];
+#ifdef CONFIG_INITRAMFS_PRESERVE_MTIME
+	mtime.actime = mtime.modtime = parsed[5];
+#endif
 	body_len = parsed[6];
 	major = parsed[7];
 	minor = parsed[8];
@@ -130,6 +171,10 @@ static inline void __init eat(unsigned n)
 	count -= n;
 }
 
+#ifdef CONFIG_INITRAMFS_PRESERVE_MTIME
+static __initdata char *vcollected;
+#endif
+
 static __initdata char *collected;
 static __initdata int remains;
 static __initdata char *collect;
@@ -271,6 +316,9 @@ static int __init do_name(void)
 			if (wfd >= 0) {
 				sys_fchown(wfd, uid, gid);
 				sys_fchmod(wfd, mode);
+#ifdef CONFIG_INITRAMFS_PRESERVE_MTIME
+				vcollected = kstrdup(collected, GFP_KERNEL);
+#endif
 				state = CopyFile;
 			}
 		}
@@ -278,12 +326,18 @@ static int __init do_name(void)
 		sys_mkdir(collected, mode);
 		sys_chown(collected, uid, gid);
 		sys_chmod(collected, mode);
+#ifdef CONFIG_INITRAMFS_PRESERVE_MTIME
+		dir_add(collected, mtime);
+#endif
 	} else if (S_ISBLK(mode) || S_ISCHR(mode) ||
 		   S_ISFIFO(mode) || S_ISSOCK(mode)) {
 		if (maybe_link() == 0) {
 			sys_mknod(collected, mode, rdev);
 			sys_chown(collected, uid, gid);
 			sys_chmod(collected, mode);
+#ifdef CONFIG_INITRAMFS_PRESERVE_MTIME
+			sys_utime(collected, &mtime);
+#endif
 		}
 	}
 	return 0;
@@ -294,6 +348,10 @@ static int __init do_copy(void)
 	if (count >= body_len) {
 		sys_write(wfd, victim, body_len);
 		sys_close(wfd);
+#ifdef CONFIG_INITRAMFS_PRESERVE_MTIME
+		sys_utime(vcollected, &mtime);
+		kfree(vcollected);
+#endif
 		eat(body_len);
 		state = SkipIt;
 		return 0;
@@ -305,12 +363,48 @@ static int __init do_copy(void)
 	}
 }
 
+#ifdef CONFIG_INITRAMFS_PRESERVE_MTIME
+static long __init sys_lutime(char __user * filename,
+	struct utimbuf __user * times)
+{
+	int error;
+	struct path path;
+	struct inode * inode;
+	struct iattr newattrs;
+
+	error = user_lpath(filename, &path);
+	if (error) return error;
+
+	inode = path.dentry->d_inode;
+
+	newattrs.ia_valid = ATTR_CTIME | ATTR_MTIME | ATTR_ATIME;
+	error = 0;
+	if (IS_APPEND(inode) || IS_IMMUTABLE(inode))
+		goto pput_and_out;
+
+	newattrs.ia_atime.tv_sec = times->actime;
+	newattrs.ia_atime.tv_nsec = 0;
+	newattrs.ia_mtime.tv_sec = times->modtime;
+	newattrs.ia_mtime.tv_nsec = 0;
+	newattrs.ia_valid |= ATTR_ATIME_SET | ATTR_MTIME_SET;
+	mutex_lock(&inode->i_mutex);
+	error = notify_change(path.dentry, &newattrs);
+	mutex_unlock(&inode->i_mutex);
+pput_and_out:
+	path_put(&path);
+	return error;
+}
+#endif
+
 static int __init do_symlink(void)
 {
 	collected[N_ALIGN(name_len) + body_len] = '\0';
 	clean_path(collected, 0);
 	sys_symlink(collected + N_ALIGN(name_len), collected);
 	sys_lchown(collected, uid, gid);
+#ifdef CONFIG_INITRAMFS_PRESERVE_MTIME
+	sys_lutime(collected, &mtime);
+#endif
 	state = SkipIt;
 	next_state = Reset;
 	return 0;
@@ -466,6 +560,9 @@ static char * __init unpack_to_rootfs(char *buf, unsigned len, int check_only)
 		buf += inptr;
 		len -= inptr;
 	}
+#ifdef CONFIG_INITRAMFS_PRESERVE_MTIME
+	dir_utime();
+#endif
 	kfree(window);
 	kfree(name_buf);
 	kfree(symlink_buf);
diff --git a/usr/Kconfig b/usr/Kconfig
index 86cecb5..f7d9f60 100644
--- a/usr/Kconfig
+++ b/usr/Kconfig
@@ -44,3 +44,13 @@ config INITRAMFS_ROOT_GID
 	  owned by group root in the initial ramdisk image.
 
 	  If you are not sure, leave it set to "0".
+
+config INITRAMFS_PRESERVE_MTIME
+	bool "Preserve mtimes from INITRAMFS image"
+	depends on BLK_DEV_INITRD
+	default n
+	help
+	  Preserve mtimes (last modified times) for all items stored in the
+	  initramfs image.
+
+	  If you are not sure, leave it set to "N"

Re: [PATCH] INITRAMFS: Add option to preserve mtime from INITRAMFS cpio images

[Andrew Morton]

On Tue, 5 Aug 2008 12:52:32 -0700 Nye Liu <nyet@nyet.org> wrote:

> From: Nye Liu <nyet@nyet.org>
> 
> When unpacking the cpio into the initramfs, mtimes are not preserved by
> default. This patch adds an INITRAMFS_PRESERVE_MTIME option that allows
> mtimes stored in the cpio image to be used when constructing the
> initramfs. For embedded applications that run exclusively out of the
> initramfs, this is invaluable.
> 
> Signed-off-by: Nye Liu <nyet@nyet.org>
> 
> ---
> 
> diff --git a/init/initramfs.c b/init/initramfs.c
> index 644fc01..0dd0a73 100644
> --- a/init/initramfs.c
> +++ b/init/initramfs.c
> @@ -7,6 +7,12 @@
>  #include <linux/string.h>
>  #include <linux/syscalls.h>
>  
> +#ifdef CONFIG_INITRAMFS_PRESERVE_MTIME
> +#include <linux/utime.h>
> +#include <linux/namei.h>
> +#include <linux/list.h>
> +#endif

It would be better to do it unconditionally if possible - remove the
config option and all the ifdefs.

>  static __initdata char *message;
>  static void __init error(char *x)
>  {
> @@ -72,6 +78,38 @@ static void __init free_hash(void)
>  	}
>  }
>  
> +#ifdef CONFIG_INITRAMFS_PRESERVE_MTIME
> +static __initdata LIST_HEAD(dir_list);
> +struct dir_entry {
> +	struct list_head list;
> +	char *name;
> +	struct utimbuf mtime;
> +};
> +
> +static void __init dir_add(const char *name, struct utimbuf mtime)
> +{
> +	struct dir_entry *de = kmalloc(sizeof(struct dir_entry), GFP_KERNEL);

kmalloc() can fail..

> +	INIT_LIST_HEAD(&de->list);
> +	de->name = kstrdup(name, GFP_KERNEL);
> +	de->mtime = mtime;
> +	list_add(&de->list, &dir_list);
> +}
> +
> +static void __init dir_utime(void)
> +{
> +	struct list_head *e, *tmp;
> +	list_for_each_safe(e, tmp, &dir_list) {
> +		struct dir_entry *de = list_entry(e, struct dir_entry, list);
> +		list_del(e);
> +		sys_utime(de->name, &de->mtime);
> +		kfree(de->name);
> +		kfree(de);
> +	}
> +}
> +
> +static __initdata struct utimbuf mtime;
> +#endif
> +
>  /* cpio header parsing */
>  
>  static __initdata unsigned long ino, major, minor, nlink;
> @@ -97,6 +135,9 @@ static void __init parse_header(char *s)
>  	uid = parsed[2];
>  	gid = parsed[3];
>  	nlink = parsed[4];
> +#ifdef CONFIG_INITRAMFS_PRESERVE_MTIME
> +	mtime.actime = mtime.modtime = parsed[5];
> +#endif
>  	body_len = parsed[6];
>  	major = parsed[7];
>  	minor = parsed[8];
> @@ -130,6 +171,10 @@ static inline void __init eat(unsigned n)
>  	count -= n;
>  }
>  
> +#ifdef CONFIG_INITRAMFS_PRESERVE_MTIME
> +static __initdata char *vcollected;
> +#endif
> +
>  static __initdata char *collected;
>  static __initdata int remains;
>  static __initdata char *collect;
> @@ -271,6 +316,9 @@ static int __init do_name(void)
>  			if (wfd >= 0) {
>  				sys_fchown(wfd, uid, gid);
>  				sys_fchmod(wfd, mode);
> +#ifdef CONFIG_INITRAMFS_PRESERVE_MTIME
> +				vcollected = kstrdup(collected, GFP_KERNEL);
> +#endif
>  				state = CopyFile;
>  			}
>  		}
> @@ -278,12 +326,18 @@ static int __init do_name(void)
>  		sys_mkdir(collected, mode);
>  		sys_chown(collected, uid, gid);
>  		sys_chmod(collected, mode);
> +#ifdef CONFIG_INITRAMFS_PRESERVE_MTIME
> +		dir_add(collected, mtime);
> +#endif
>  	} else if (S_ISBLK(mode) || S_ISCHR(mode) ||
>  		   S_ISFIFO(mode) || S_ISSOCK(mode)) {
>  		if (maybe_link() == 0) {
>  			sys_mknod(collected, mode, rdev);
>  			sys_chown(collected, uid, gid);
>  			sys_chmod(collected, mode);
> +#ifdef CONFIG_INITRAMFS_PRESERVE_MTIME
> +			sys_utime(collected, &mtime);
> +#endif
>  		}
>  	}
>  	return 0;
> @@ -294,6 +348,10 @@ static int __init do_copy(void)
>  	if (count >= body_len) {
>  		sys_write(wfd, victim, body_len);
>  		sys_close(wfd);
> +#ifdef CONFIG_INITRAMFS_PRESERVE_MTIME
> +		sys_utime(vcollected, &mtime);
> +		kfree(vcollected);
> +#endif
>  		eat(body_len);
>  		state = SkipIt;
>  		return 0;
> @@ -305,12 +363,48 @@ static int __init do_copy(void)
>  	}
>  }
>  
> +#ifdef CONFIG_INITRAMFS_PRESERVE_MTIME
> +static long __init sys_lutime(char __user * filename,
> +	struct utimbuf __user * times)

Please avoid using the sys_ namespace.    That is for system calls.

Please pass the patch through scripts/checkpatch.pl and review the report.

> +{
> +	int error;
> +	struct path path;
> +	struct inode * inode;
> +	struct iattr newattrs;
> +
> +	error = user_lpath(filename, &path);
> +	if (error) return error;
> +
> +	inode = path.dentry->d_inode;
> +
> +	newattrs.ia_valid = ATTR_CTIME | ATTR_MTIME | ATTR_ATIME;
> +	error = 0;
> +	if (IS_APPEND(inode) || IS_IMMUTABLE(inode))
> +		goto pput_and_out;
> +
> +	newattrs.ia_atime.tv_sec = times->actime;
> +	newattrs.ia_atime.tv_nsec = 0;
> +	newattrs.ia_mtime.tv_sec = times->modtime;
> +	newattrs.ia_mtime.tv_nsec = 0;
> +	newattrs.ia_valid |= ATTR_ATIME_SET | ATTR_MTIME_SET;
> +	mutex_lock(&inode->i_mutex);
> +	error = notify_change(path.dentry, &newattrs);
> +	mutex_unlock(&inode->i_mutex);
> +pput_and_out:
> +	path_put(&path);
> +	return error;
> +}

How does this differ from sys_utime()?

Can this use do_utimes()?

> +#endif
> +
>  static int __init do_symlink(void)
>  {
>  	collected[N_ALIGN(name_len) + body_len] = '\0';
>  	clean_path(collected, 0);
>  	sys_symlink(collected + N_ALIGN(name_len), collected);
>  	sys_lchown(collected, uid, gid);
> +#ifdef CONFIG_INITRAMFS_PRESERVE_MTIME
> +	sys_lutime(collected, &mtime);
> +#endif
>  	state = SkipIt;
>  	next_state = Reset;
>  	return 0;
> @@ -466,6 +560,9 @@ static char * __init unpack_to_rootfs(char *buf, unsigned len, int check_only)
>  		buf += inptr;
>  		len -= inptr;
>  	}
> +#ifdef CONFIG_INITRAMFS_PRESERVE_MTIME
> +	dir_utime();
> +#endif
>  	kfree(window);
>  	kfree(name_buf);
>  	kfree(symlink_buf);
> diff --git a/usr/Kconfig b/usr/Kconfig
> index 86cecb5..f7d9f60 100644
> --- a/usr/Kconfig
> +++ b/usr/Kconfig
> @@ -44,3 +44,13 @@ config INITRAMFS_ROOT_GID
>  	  owned by group root in the initial ramdisk image.
>  
>  	  If you are not sure, leave it set to "0".
> +
> +config INITRAMFS_PRESERVE_MTIME
> +	bool "Preserve mtimes from INITRAMFS image"
> +	depends on BLK_DEV_INITRD
> +	default n
> +	help
> +	  Preserve mtimes (last modified times) for all items stored in the
> +	  initramfs image.
> +
> +	  If you are not sure, leave it set to "N"

[PATCH] INITRAMFS: Add option to preserve mtime from INITRAMFS cpio images

[Nye Liu]

Resubmission of initramfs preserve mtime patch following Andrew Morton's
suggestions.

From: Nye Liu <nyet@nyet.org>

When unpacking the cpio into the initramfs, mtimes are not preserved by
default. This patch adds an INITRAMFS_PRESERVE_MTIME option that allows mtimes
stored in the cpio image to be used when constructing the initramfs. For
embedded applications that run exclusively out of the initramfs, this is
invaluable.

Signed-off-by: Nye Liu <nyet@nyet.org>

diff --git a/init/initramfs.c b/init/initramfs.c
index 644fc01..ebfc049 100644
--- a/init/initramfs.c
+++ b/init/initramfs.c
@@ -6,6 +6,7 @@
 #include <linux/delay.h>
 #include <linux/string.h>
 #include <linux/syscalls.h>
+#include <linux/utime.h>
 
 static __initdata char *message;
 static void __init error(char *x)
@@ -72,6 +73,38 @@ static void __init free_hash(void)
 	}
 }
 
+static __initdata LIST_HEAD(dir_list);
+struct dir_entry {
+	struct list_head list;
+	char *name;
+	struct utimbuf mtime;
+};
+
+static void __init dir_add(const char *name, struct utimbuf mtime)
+{
+	struct dir_entry *de = kmalloc(sizeof(struct dir_entry), GFP_KERNEL);
+	if (!de)
+		panic("can't allocate dir_entry buffer");
+	INIT_LIST_HEAD(&de->list);
+	de->name = kstrdup(name, GFP_KERNEL);
+	de->mtime = mtime;
+	list_add(&de->list, &dir_list);
+}
+
+static void __init dir_utime(void)
+{
+	struct list_head *e, *tmp;
+	list_for_each_safe(e, tmp, &dir_list) {
+		struct dir_entry *de = list_entry(e, struct dir_entry, list);
+		list_del(e);
+		sys_utime(de->name, &de->mtime);
+		kfree(de->name);
+		kfree(de);
+	}
+}
+
+static __initdata struct utimbuf mtime;
+
 /* cpio header parsing */
 
 static __initdata unsigned long ino, major, minor, nlink;
@@ -97,6 +130,7 @@ static void __init parse_header(char *s)
 	uid = parsed[2];
 	gid = parsed[3];
 	nlink = parsed[4];
+	mtime.actime = mtime.modtime = parsed[5];
 	body_len = parsed[6];
 	major = parsed[7];
 	minor = parsed[8];
@@ -130,6 +164,7 @@ static inline void __init eat(unsigned n)
 	count -= n;
 }
 
+static __initdata char *vcollected;
 static __initdata char *collected;
 static __initdata int remains;
 static __initdata char *collect;
@@ -271,6 +306,7 @@ static int __init do_name(void)
 			if (wfd >= 0) {
 				sys_fchown(wfd, uid, gid);
 				sys_fchmod(wfd, mode);
+				vcollected = kstrdup(collected, GFP_KERNEL);
 				state = CopyFile;
 			}
 		}
@@ -278,12 +314,14 @@ static int __init do_name(void)
 		sys_mkdir(collected, mode);
 		sys_chown(collected, uid, gid);
 		sys_chmod(collected, mode);
+		dir_add(collected, mtime);
 	} else if (S_ISBLK(mode) || S_ISCHR(mode) ||
 		   S_ISFIFO(mode) || S_ISSOCK(mode)) {
 		if (maybe_link() == 0) {
 			sys_mknod(collected, mode, rdev);
 			sys_chown(collected, uid, gid);
 			sys_chmod(collected, mode);
+			sys_utime(collected, &mtime);
 		}
 	}
 	return 0;
@@ -294,6 +332,8 @@ static int __init do_copy(void)
 	if (count >= body_len) {
 		sys_write(wfd, victim, body_len);
 		sys_close(wfd);
+		sys_utime(vcollected, &mtime);
+		kfree(vcollected);
 		eat(body_len);
 		state = SkipIt;
 		return 0;
@@ -305,12 +345,26 @@ static int __init do_copy(void)
 	}
 }
 
+static long __init do_lutime(char __user *filename,
+	struct utimbuf __user *times)
+{
+	struct timespec t[2];
+
+	t[0].tv_sec = times->actime;
+	t[0].tv_nsec = 0;
+	t[1].tv_sec = times->modtime;
+	t[1].tv_nsec = 0;
+
+	return do_utimes(AT_FDCWD, filename, t, AT_SYMLINK_NOFOLLOW);
+}
+
 static int __init do_symlink(void)
 {
 	collected[N_ALIGN(name_len) + body_len] = '\0';
 	clean_path(collected, 0);
 	sys_symlink(collected + N_ALIGN(name_len), collected);
 	sys_lchown(collected, uid, gid);
+	do_lutime(collected, &mtime);
 	state = SkipIt;
 	next_state = Reset;
 	return 0;
@@ -466,6 +520,7 @@ static char * __init unpack_to_rootfs(char *buf, unsigned len, int check_only)
 		buf += inptr;
 		len -= inptr;
 	}
+	dir_utime();
 	kfree(window);
 	kfree(name_buf);
 	kfree(symlink_buf);

Re: [PATCH] INITRAMFS: Add option to preserve mtime from INITRAMFS cpio images

[Andrew Morton]

On Wed, 3 Sep 2008 13:29:38 -0700
Nye Liu <nyet@nyet.org> wrote:

> Resubmission of initramfs preserve mtime patch following Andrew Morton's
> suggestions.

It's nice to provide some sort of accounting for the reviewer's
individual comments.  But in this case I only had three or fopur and I
see how they were addressed.

> From: Nye Liu <nyet@nyet.org>
> 
> When unpacking the cpio into the initramfs, mtimes are not preserved by
> default. This patch adds an INITRAMFS_PRESERVE_MTIME option that allows mtimes
> stored in the cpio image to be used when constructing the initramfs. For
> embedded applications that run exclusively out of the initramfs, this is
> invaluable.

Why is it "invlauable".  Please explain this value in full detail -
it's the whole reason for merging the patch!

> Signed-off-by: Nye Liu <nyet@nyet.org>
> 
> diff --git a/init/initramfs.c b/init/initramfs.c
> index 644fc01..ebfc049 100644
> --- a/init/initramfs.c
> +++ b/init/initramfs.c
> @@ -6,6 +6,7 @@
>  #include <linux/delay.h>
>  #include <linux/string.h>
>  #include <linux/syscalls.h>
> +#include <linux/utime.h>
>  
>  static __initdata char *message;
>  static void __init error(char *x)
> @@ -72,6 +73,38 @@ static void __init free_hash(void)
>  	}
>  }
>  
> +static __initdata LIST_HEAD(dir_list);
> +struct dir_entry {
> +	struct list_head list;
> +	char *name;
> +	struct utimbuf mtime;
> +};
> +
> +static void __init dir_add(const char *name, struct utimbuf mtime)
> +{
> +	struct dir_entry *de = kmalloc(sizeof(struct dir_entry), GFP_KERNEL);
> +	if (!de)
> +		panic("can't allocate dir_entry buffer");
> +	INIT_LIST_HEAD(&de->list);
> +	de->name = kstrdup(name, GFP_KERNEL);
> +	de->mtime = mtime;
> +	list_add(&de->list, &dir_list);
> +}
> +
> +static void __init dir_utime(void)
> +{
> +	struct list_head *e, *tmp;
> +	list_for_each_safe(e, tmp, &dir_list) {
> +		struct dir_entry *de = list_entry(e, struct dir_entry, list);
> +		list_del(e);
> +		sys_utime(de->name, &de->mtime);

gargh.  Why does this work?  It's normally a big fail to pass a kernel
address into a system call.  I guess we're running under KERNEL_DS here
and getname() and strncpy_from_user() did the right thing.

On what CPU architecture was this tested?

Wouldn't it be simpler to put a timespec into struct dir_entry then go
direct to do_utimes() here?

> +		kfree(de->name);
> +		kfree(de);
> +	}
> +}
> +
> +static __initdata struct utimbuf mtime;
> +
>  /* cpio header parsing */
>  
>  static __initdata unsigned long ino, major, minor, nlink;
> @@ -97,6 +130,7 @@ static void __init parse_header(char *s)
>  	uid = parsed[2];
>  	gid = parsed[3];
>  	nlink = parsed[4];
> +	mtime.actime = mtime.modtime = parsed[5];
>  	body_len = parsed[6];
>  	major = parsed[7];
>  	minor = parsed[8];
> @@ -130,6 +164,7 @@ static inline void __init eat(unsigned n)
>  	count -= n;
>  }
>  
> +static __initdata char *vcollected;
>  static __initdata char *collected;
>  static __initdata int remains;
>  static __initdata char *collect;
> @@ -271,6 +306,7 @@ static int __init do_name(void)
>  			if (wfd >= 0) {
>  				sys_fchown(wfd, uid, gid);
>  				sys_fchmod(wfd, mode);
> +				vcollected = kstrdup(collected, GFP_KERNEL);
>  				state = CopyFile;
>  			}
>  		}
> @@ -278,12 +314,14 @@ static int __init do_name(void)
>  		sys_mkdir(collected, mode);
>  		sys_chown(collected, uid, gid);
>  		sys_chmod(collected, mode);
> +		dir_add(collected, mtime);
>  	} else if (S_ISBLK(mode) || S_ISCHR(mode) ||
>  		   S_ISFIFO(mode) || S_ISSOCK(mode)) {
>  		if (maybe_link() == 0) {
>  			sys_mknod(collected, mode, rdev);
>  			sys_chown(collected, uid, gid);
>  			sys_chmod(collected, mode);
> +			sys_utime(collected, &mtime);
>  		}
>  	}
>  	return 0;
> @@ -294,6 +332,8 @@ static int __init do_copy(void)
>  	if (count >= body_len) {
>  		sys_write(wfd, victim, body_len);
>  		sys_close(wfd);
> +		sys_utime(vcollected, &mtime);

and here?

> +		kfree(vcollected);
>  		eat(body_len);
>  		state = SkipIt;
>  		return 0;
> @@ -305,12 +345,26 @@ static int __init do_copy(void)
>  	}
>  }
>  
> +static long __init do_lutime(char __user *filename,
> +	struct utimbuf __user *times)
> +{
> +	struct timespec t[2];
> +
> +	t[0].tv_sec = times->actime;
> +	t[0].tv_nsec = 0;
> +	t[1].tv_sec = times->modtime;
> +	t[1].tv_nsec = 0;
> +
> +	return do_utimes(AT_FDCWD, filename, t, AT_SYMLINK_NOFOLLOW);
> +}
> +
>  static int __init do_symlink(void)
>  {
>  	collected[N_ALIGN(name_len) + body_len] = '\0';
>  	clean_path(collected, 0);
>  	sys_symlink(collected + N_ALIGN(name_len), collected);
>  	sys_lchown(collected, uid, gid);
> +	do_lutime(collected, &mtime);
>  	state = SkipIt;
>  	next_state = Reset;
>  	return 0;
> @@ -466,6 +520,7 @@ static char * __init unpack_to_rootfs(char *buf, unsigned len, int check_only)
>  		buf += inptr;
>  		len -= inptr;
>  	}
> +	dir_utime();

Perhaps this is the simplest implementation - I didn't check the fine
details.  What's your thinking here?  

Re: [PATCH] INITRAMFS: Add option to preserve mtime from INITRAMFS cpio images

[Nye Liu]

On Wed, Sep 03, 2008 at 03:22:31PM -0700, Andrew Morton wrote:
> > From: Nye Liu <nyet@nyet.org>
> > 
> > When unpacking the cpio into the initramfs, mtimes are not preserved by
> > default. This patch adds an INITRAMFS_PRESERVE_MTIME option that allows mtimes
> > stored in the cpio image to be used when constructing the initramfs. For
> > embedded applications that run exclusively out of the initramfs, this is
> > invaluable.
> 
> Why is it "invlauable".  Please explain this value in full detail -
> it's the whole reason for merging the patch!

When building embedded application initramfs images, its nice to know
when the files were actually created during the build process - that
makes it easier to see what files were modified when so we can compare
the files that are being used on the image with the files used during
the build process. This might help (for example) to determine if the
target system has all the updated files you expect to see w/o having to
check MD5s etc.

In our environment, the whole system runs off the initramfs partition,
and seeing the modified times of the shared libraries (for example)
helps us find bugs that may have been introduced by the build system
incorrectly propogating outdated shared libraries into the image.

Similarly, many of the initializion/configuration files in /etc
might be dynamically built by the build system, and knowing when
they were modified helps us sanity check whether the target system
has the "latest" files etc.

Finally, we might use last modified times to determine whether a
hot fix should be applied or not to the running ramfs.

Nye

> 
> > Signed-off-by: Nye Liu <nyet@nyet.org>
> > 
> > diff --git a/init/initramfs.c b/init/initramfs.c
> > index 644fc01..ebfc049 100644
> > --- a/init/initramfs.c
> > +++ b/init/initramfs.c
> > @@ -6,6 +6,7 @@
> >  #include <linux/delay.h>
> >  #include <linux/string.h>
> >  #include <linux/syscalls.h>
> > +#include <linux/utime.h>
> >  
> >  static __initdata char *message;
> >  static void __init error(char *x)
> > @@ -72,6 +73,38 @@ static void __init free_hash(void)
> >  	}
> >  }
> >  
> > +static __initdata LIST_HEAD(dir_list);
> > +struct dir_entry {
> > +	struct list_head list;
> > +	char *name;
> > +	struct utimbuf mtime;
> > +};
> > +
> > +static void __init dir_add(const char *name, struct utimbuf mtime)
> > +{
> > +	struct dir_entry *de = kmalloc(sizeof(struct dir_entry), GFP_KERNEL);
> > +	if (!de)
> > +		panic("can't allocate dir_entry buffer");
> > +	INIT_LIST_HEAD(&de->list);
> > +	de->name = kstrdup(name, GFP_KERNEL);
> > +	de->mtime = mtime;
> > +	list_add(&de->list, &dir_list);
> > +}
> > +
> > +static void __init dir_utime(void)
> > +{
> > +	struct list_head *e, *tmp;
> > +	list_for_each_safe(e, tmp, &dir_list) {
> > +		struct dir_entry *de = list_entry(e, struct dir_entry, list);
> > +		list_del(e);
> > +		sys_utime(de->name, &de->mtime);
> 
> gargh.  Why does this work?  It's normally a big fail to pass a kernel
> address into a system call.  I guess we're running under KERNEL_DS here
> and getname() and strncpy_from_user() did the right thing.
> 
> On what CPU architecture was this tested?
> 
> Wouldn't it be simpler to put a timespec into struct dir_entry then go
> direct to do_utimes() here?
> 
> > +		kfree(de->name);
> > +		kfree(de);
> > +	}
> > +}
> > +
> > +static __initdata struct utimbuf mtime;
> > +
> >  /* cpio header parsing */
> >  
> >  static __initdata unsigned long ino, major, minor, nlink;
> > @@ -97,6 +130,7 @@ static void __init parse_header(char *s)
> >  	uid = parsed[2];
> >  	gid = parsed[3];
> >  	nlink = parsed[4];
> > +	mtime.actime = mtime.modtime = parsed[5];
> >  	body_len = parsed[6];
> >  	major = parsed[7];
> >  	minor = parsed[8];
> > @@ -130,6 +164,7 @@ static inline void __init eat(unsigned n)
> >  	count -= n;
> >  }
> >  
> > +static __initdata char *vcollected;
> >  static __initdata char *collected;
> >  static __initdata int remains;
> >  static __initdata char *collect;
> > @@ -271,6 +306,7 @@ static int __init do_name(void)
> >  			if (wfd >= 0) {
> >  				sys_fchown(wfd, uid, gid);
> >  				sys_fchmod(wfd, mode);
> > +				vcollected = kstrdup(collected, GFP_KERNEL);
> >  				state = CopyFile;
> >  			}
> >  		}
> > @@ -278,12 +314,14 @@ static int __init do_name(void)
> >  		sys_mkdir(collected, mode);
> >  		sys_chown(collected, uid, gid);
> >  		sys_chmod(collected, mode);
> > +		dir_add(collected, mtime);
> >  	} else if (S_ISBLK(mode) || S_ISCHR(mode) ||
> >  		   S_ISFIFO(mode) || S_ISSOCK(mode)) {
> >  		if (maybe_link() == 0) {
> >  			sys_mknod(collected, mode, rdev);
> >  			sys_chown(collected, uid, gid);
> >  			sys_chmod(collected, mode);
> > +			sys_utime(collected, &mtime);
> >  		}
> >  	}
> >  	return 0;
> > @@ -294,6 +332,8 @@ static int __init do_copy(void)
> >  	if (count >= body_len) {
> >  		sys_write(wfd, victim, body_len);
> >  		sys_close(wfd);
> > +		sys_utime(vcollected, &mtime);
> 
> and here?
> 
> > +		kfree(vcollected);
> >  		eat(body_len);
> >  		state = SkipIt;
> >  		return 0;
> > @@ -305,12 +345,26 @@ static int __init do_copy(void)
> >  	}
> >  }
> >  
> > +static long __init do_lutime(char __user *filename,
> > +	struct utimbuf __user *times)
> > +{
> > +	struct timespec t[2];
> > +
> > +	t[0].tv_sec = times->actime;
> > +	t[0].tv_nsec = 0;
> > +	t[1].tv_sec = times->modtime;
> > +	t[1].tv_nsec = 0;
> > +
> > +	return do_utimes(AT_FDCWD, filename, t, AT_SYMLINK_NOFOLLOW);
> > +}
> > +
> >  static int __init do_symlink(void)
> >  {
> >  	collected[N_ALIGN(name_len) + body_len] = '\0';
> >  	clean_path(collected, 0);
> >  	sys_symlink(collected + N_ALIGN(name_len), collected);
> >  	sys_lchown(collected, uid, gid);
> > +	do_lutime(collected, &mtime);
> >  	state = SkipIt;
> >  	next_state = Reset;
> >  	return 0;
> > @@ -466,6 +520,7 @@ static char * __init unpack_to_rootfs(char *buf, unsigned len, int check_only)
> >  		buf += inptr;
> >  		len -= inptr;
> >  	}
> > +	dir_utime();
> 
> Perhaps this is the simplest implementation - I didn't check the fine
> details.  What's your thinking here?  

-- 
Nye Liu
nliu@mrv.com
(818) 772-6235x248
(818) 772-0576 fax

"Who would be stupid enough to quote a fictitious character?"
	-- Don Quixote

Re: [PATCH] INITRAMFS: Add option to preserve mtime from INITRAMFS cpio images

[Andrew Morton]

On Wed, 3 Sep 2008 15:31:14 -0700
Nye Liu <nyet@mrv.com> wrote:

> On Wed, Sep 03, 2008 at 03:22:31PM -0700, Andrew Morton wrote:
> > > From: Nye Liu <nyet@nyet.org>
> > > 
> > > When unpacking the cpio into the initramfs, mtimes are not preserved by
> > > default. This patch adds an INITRAMFS_PRESERVE_MTIME option that allows mtimes
> > > stored in the cpio image to be used when constructing the initramfs. For
> > > embedded applications that run exclusively out of the initramfs, this is
> > > invaluable.
> > 
> > Why is it "invlauable".  Please explain this value in full detail -
> > it's the whole reason for merging the patch!
> 
> When building embedded application initramfs images, its nice to know
> when the files were actually created during the build process - that
> makes it easier to see what files were modified when so we can compare
> the files that are being used on the image with the files used during
> the build process. This might help (for example) to determine if the
> target system has all the updated files you expect to see w/o having to
> check MD5s etc.
> 
> In our environment, the whole system runs off the initramfs partition,
> and seeing the modified times of the shared libraries (for example)
> helps us find bugs that may have been introduced by the build system
> incorrectly propogating outdated shared libraries into the image.
> 
> Similarly, many of the initializion/configuration files in /etc
> might be dynamically built by the build system, and knowing when
> they were modified helps us sanity check whether the target system
> has the "latest" files etc.
> 
> Finally, we might use last modified times to determine whether a
> hot fix should be applied or not to the running ramfs.
> 

Thanks, I updated the changelog.

> > gargh.  Why does this work?  It's normally a big fail to pass a kernel
> > address into a system call.  I guess we're running under KERNEL_DS here
> > and getname() and strncpy_from_user() did the right thing.
> > 
> > On what CPU architecture was this tested?
> > 
> > Wouldn't it be simpler to put a timespec into struct dir_entry then go
> > direct to do_utimes() here?
> > 

Did you see this stuff?

Re: [PATCH] INITRAMFS: Add option to preserve mtime from INITRAMFS cpio images

[Frans Meulenbroeks]

2008/9/4 Nye Liu <nyet@mrv.com>:
> On Wed, Sep 03, 2008 at 03:22:31PM -0700, Andrew Morton wrote:
>> > From: Nye Liu <nyet@nyet.org>
>> >
>> > When unpacking the cpio into the initramfs, mtimes are not preserved by
>> > default. This patch adds an INITRAMFS_PRESERVE_MTIME option that allows mtimes
>> > stored in the cpio image to be used when constructing the initramfs. For
>> > embedded applications that run exclusively out of the initramfs, this is
>> > invaluable.
>>
>> Why is it "invlauable".  Please explain this value in full detail -
>> it's the whole reason for merging the patch!
>
> When building embedded application initramfs images, its nice to know
> when the files were actually created during the build process - that
> makes it easier to see what files were modified when so we can compare
> the files that are being used on the image with the files used during
> the build process. This might help (for example) to determine if the
> target system has all the updated files you expect to see w/o having to
> check MD5s etc.

Hm.

"Invaluable" != "nice to know".

What worries me is that this code is executed at boot time (when
populating the ramfs).
For embedded systems a fast boot time is often important.
I admit that the net effect of this on boot time is marginal (but some
might consider having mtime a marginal benefit), and 100 cents also
make a dollar, so my suggestion would be to either reject this patch
or make it optional (e.g. depending on some debug config flag).

Best regards, Frans.

Re: [PATCH] INITRAMFS: Add option to preserve mtime from INITRAMFS cpio images

[nyet]

Frans Meulenbroeks wrote:
> 2008/9/4 Nye Liu <nyet@mrv.com>:
>   
>> On Wed, Sep 03, 2008 at 03:22:31PM -0700, Andrew Morton wrote:
>>     
>>>> From: Nye Liu <nyet@nyet.org>
>>>>
>>>> When unpacking the cpio into the initramfs, mtimes are not preserved by
>>>> default. This patch adds an INITRAMFS_PRESERVE_MTIME option that allows mtimes
>>>> stored in the cpio image to be used when constructing the initramfs. For
>>>> embedded applications that run exclusively out of the initramfs, this is
>>>> invaluable.
>>>>         
>>> Why is it "invlauable".  Please explain this value in full detail -
>>> it's the whole reason for merging the patch!
>>>       
>> When building embedded application initramfs images, its nice to know
>> when the files were actually created during the build process - that
>> makes it easier to see what files were modified when so we can compare
>> the files that are being used on the image with the files used during
>> the build process. This might help (for example) to determine if the
>> target system has all the updated files you expect to see w/o having to
>> check MD5s etc.
>>     
>
> Hm.
>
> "Invaluable" != "nice to know".
>
> What worries me is that this code is executed at boot time (when
> populating the ramfs).
> For embedded systems a fast boot time is often important.
> I admit that the net effect of this on boot time is marginal (but some
> might consider having mtime a marginal benefit), and 100 cents also
> make a dollar, so my suggestion would be to either reject this patch
> or make it optional (e.g. depending on some debug config flag).
>
> Best regards, Frans.
>   
An earlier rev patch made it optional. I can resubmit of course.

Re: [PATCH] INITRAMFS: Add option to preserve mtime from INITRAMFS cpio images

[Krzysztof Halasa]

"Frans Meulenbroeks" <fransmeulenbroeks@gmail.com> writes:

> "Invaluable" != "nice to know".

Well, I guess it's as "invaluable" for embedded systems running on
initramfs as mtime on regular filesystems in case of normal systems.

Is mtime on ext3 "invaluable" or just "nice to know"?

> I admit that the net effect of this on boot time is marginal (but some
> might consider having mtime a marginal benefit), and 100 cents also
> make a dollar, so my suggestion would be to either reject this patch
> or make it optional (e.g. depending on some debug config flag).

It depends on how marginal is it. If it makes unpacking 1% slower I
can see no need for being optional. 10% - maybe it should?
-- 
Krzysztof Halasa

Re: [PATCH] INITRAMFS: Add option to preserve mtime from INITRAMFS cpio images

[Nye Liu]

On Wed, Sep 03, 2008 at 03:22:31PM -0700, Andrew Morton wrote:
> On Wed, 3 Sep 2008 13:29:38 -0700
> > Signed-off-by: Nye Liu <nyet@nyet.org>
> > 
> > diff --git a/init/initramfs.c b/init/initramfs.c
> > index 644fc01..ebfc049 100644
> > --- a/init/initramfs.c
> > +++ b/init/initramfs.c
> > @@ -6,6 +6,7 @@
> >  #include <linux/delay.h>
> >  #include <linux/string.h>
> >  #include <linux/syscalls.h>
> > +#include <linux/utime.h>
> >  
> >  static __initdata char *message;
> >  static void __init error(char *x)
> > @@ -72,6 +73,38 @@ static void __init free_hash(void)
> >  	}
> >  }
> >  
> > +static __initdata LIST_HEAD(dir_list);
> > +struct dir_entry {
> > +	struct list_head list;
> > +	char *name;
> > +	struct utimbuf mtime;
> > +};
> > +
> > +static void __init dir_add(const char *name, struct utimbuf mtime)
> > +{
> > +	struct dir_entry *de = kmalloc(sizeof(struct dir_entry), GFP_KERNEL);
> > +	if (!de)
> > +		panic("can't allocate dir_entry buffer");
> > +	INIT_LIST_HEAD(&de->list);
> > +	de->name = kstrdup(name, GFP_KERNEL);
> > +	de->mtime = mtime;
> > +	list_add(&de->list, &dir_list);
> > +}
> > +
> > +static void __init dir_utime(void)
> > +{
> > +	struct list_head *e, *tmp;
> > +	list_for_each_safe(e, tmp, &dir_list) {
> > +		struct dir_entry *de = list_entry(e, struct dir_entry, list);
> > +		list_del(e);
> > +		sys_utime(de->name, &de->mtime);
> 
> gargh.  Why does this work?  It's normally a big fail to pass a kernel
> address into a system call.  I guess we're running under KERNEL_DS here
> and getname() and strncpy_from_user() did the right thing.

Possibly, i was not aware of that limitation :( I'll look into it

> On what CPU architecture was this tested?

ppc and powerpc

> Wouldn't it be simpler to put a timespec into struct dir_entry then go
> direct to do_utimes() here?

No, if i understand your question correctly. I have to do the mtime
modifications in two passes (see last comment below)

> 
> > +		kfree(de->name);
> > +		kfree(de);
> > +	}
> > +}
> > +
> > +static __initdata struct utimbuf mtime;
> > +
> >  /* cpio header parsing */
> >  
> >  static __initdata unsigned long ino, major, minor, nlink;
> > @@ -97,6 +130,7 @@ static void __init parse_header(char *s)
> >  	uid = parsed[2];
> >  	gid = parsed[3];
> >  	nlink = parsed[4];
> > +	mtime.actime = mtime.modtime = parsed[5];
> >  	body_len = parsed[6];
> >  	major = parsed[7];
> >  	minor = parsed[8];
> > @@ -130,6 +164,7 @@ static inline void __init eat(unsigned n)
> >  	count -= n;
> >  }
> >  
> > +static __initdata char *vcollected;
> >  static __initdata char *collected;
> >  static __initdata int remains;
> >  static __initdata char *collect;
> > @@ -271,6 +306,7 @@ static int __init do_name(void)
> >  			if (wfd >= 0) {
> >  				sys_fchown(wfd, uid, gid);
> >  				sys_fchmod(wfd, mode);
> > +				vcollected = kstrdup(collected, GFP_KERNEL);
> >  				state = CopyFile;
> >  			}
> >  		}
> > @@ -278,12 +314,14 @@ static int __init do_name(void)
> >  		sys_mkdir(collected, mode);
> >  		sys_chown(collected, uid, gid);
> >  		sys_chmod(collected, mode);
> > +		dir_add(collected, mtime);
> >  	} else if (S_ISBLK(mode) || S_ISCHR(mode) ||
> >  		   S_ISFIFO(mode) || S_ISSOCK(mode)) {
> >  		if (maybe_link() == 0) {
> >  			sys_mknod(collected, mode, rdev);
> >  			sys_chown(collected, uid, gid);
> >  			sys_chmod(collected, mode);
> > +			sys_utime(collected, &mtime);
> >  		}
> >  	}
> >  	return 0;
> > @@ -294,6 +332,8 @@ static int __init do_copy(void)
> >  	if (count >= body_len) {
> >  		sys_write(wfd, victim, body_len);
> >  		sys_close(wfd);
> > +		sys_utime(vcollected, &mtime);
> 
> and here?

see below regarding two passes.

> > +		kfree(vcollected);
> >  		eat(body_len);
> >  		state = SkipIt;
> >  		return 0;
> > @@ -305,12 +345,26 @@ static int __init do_copy(void)
> >  	}
> >  }
> >  
> > +static long __init do_lutime(char __user *filename,
> > +	struct utimbuf __user *times)
> > +{
> > +	struct timespec t[2];
> > +
> > +	t[0].tv_sec = times->actime;
> > +	t[0].tv_nsec = 0;
> > +	t[1].tv_sec = times->modtime;
> > +	t[1].tv_nsec = 0;
> > +
> > +	return do_utimes(AT_FDCWD, filename, t, AT_SYMLINK_NOFOLLOW);
> > +}
> > +
> >  static int __init do_symlink(void)
> >  {
> >  	collected[N_ALIGN(name_len) + body_len] = '\0';
> >  	clean_path(collected, 0);
> >  	sys_symlink(collected + N_ALIGN(name_len), collected);
> >  	sys_lchown(collected, uid, gid);
> > +	do_lutime(collected, &mtime);
> >  	state = SkipIt;
> >  	next_state = Reset;
> >  	return 0;
> > @@ -466,6 +520,7 @@ static char * __init unpack_to_rootfs(char *buf, unsigned len, int check_only)
> >  		buf += inptr;
> >  		len -= inptr;
> >  	}
> > +	dir_utime();
> 
> Perhaps this is the simplest implementation - I didn't check the fine
> details.  What's your thinking here?  
> 

The main problem is that i need to save off the entire list for later
processing of the directory mtimes... if i process the directory mtimes
in the same pass as the file/link mtimes, touching the directory inode
when creating/modifying the file/links updates the directory mtime, and
overwrites whatever mtime i set the directory to when i created it.

The only solution is to do a two pass, which is why the list is
necessary. If there is a better way, i did not find it :(

It could be that i misunderstood your question though :)

Re: [PATCH] INITRAMFS: Add option to preserve mtime from INITRAMFS cpio images

[Andrew Morton]

On Wed, 3 Sep 2008 15:41:31 -0700
Nye Liu <nyet@nyet.org> wrote:

> > >  	collected[N_ALIGN(name_len) + body_len] = '\0';
> > >  	clean_path(collected, 0);
> > >  	sys_symlink(collected + N_ALIGN(name_len), collected);
> > >  	sys_lchown(collected, uid, gid);
> > > +	do_lutime(collected, &mtime);
> > >  	state = SkipIt;
> > >  	next_state = Reset;
> > >  	return 0;
> > > @@ -466,6 +520,7 @@ static char * __init unpack_to_rootfs(char *buf, unsigned len, int check_only)
> > >  		buf += inptr;
> > >  		len -= inptr;
> > >  	}
> > > +	dir_utime();
> > 
> > Perhaps this is the simplest implementation - I didn't check the fine
> > details.  What's your thinking here?  
> > 
> 
> The main problem is that i need to save off the entire list for later
> processing of the directory mtimes... if i process the directory mtimes
> in the same pass as the file/link mtimes, touching the directory inode
> when creating/modifying the file/links updates the directory mtime, and
> overwrites whatever mtime i set the directory to when i created it.
> 
> The only solution is to do a two pass, which is why the list is
> necessary. If there is a better way, i did not find it :(
> 
> It could be that i misunderstood your question though :)

I'm wondering whether this code need to use `struct utimbuf' at all. 
Or at least, as much as it does.  utimbuf is more a userspace-facing
thing whereas in-kernel timespecs and timevals are more common.

The code as you have it does a fair few conversions into utimbuf format
(both directly and via the existing functions which it calls).  Would
it be simpler if it dealt in terms of timespecs?

Re: [PATCH] INITRAMFS: Add option to preserve mtime from INITRAMFS cpio images

[Nye Liu]

On Wed, Sep 03, 2008 at 03:48:40PM -0700, Andrew Morton wrote:
> On Wed, 3 Sep 2008 15:41:31 -0700
> Nye Liu <nyet@nyet.org> wrote:
> 
> > > >  	collected[N_ALIGN(name_len) + body_len] = '\0';
> > > >  	clean_path(collected, 0);
> > > >  	sys_symlink(collected + N_ALIGN(name_len), collected);
> > > >  	sys_lchown(collected, uid, gid);
> > > > +	do_lutime(collected, &mtime);
> > > >  	state = SkipIt;
> > > >  	next_state = Reset;
> > > >  	return 0;
> > > > @@ -466,6 +520,7 @@ static char * __init unpack_to_rootfs(char *buf, unsigned len, int check_only)
> > > >  		buf += inptr;
> > > >  		len -= inptr;
> > > >  	}
> > > > +	dir_utime();
> > > 
> > > Perhaps this is the simplest implementation - I didn't check the fine
> > > details.  What's your thinking here?  
> > > 
> > 
> > The main problem is that i need to save off the entire list for later
> > processing of the directory mtimes... if i process the directory mtimes
> > in the same pass as the file/link mtimes, touching the directory inode
> > when creating/modifying the file/links updates the directory mtime, and
> > overwrites whatever mtime i set the directory to when i created it.
> > 
> > The only solution is to do a two pass, which is why the list is
> > necessary. If there is a better way, i did not find it :(
> > 
> > It could be that i misunderstood your question though :)
> 
> I'm wondering whether this code need to use `struct utimbuf' at all. 
> Or at least, as much as it does.  utimbuf is more a userspace-facing
> thing whereas in-kernel timespecs and timevals are more common.
> 
> The code as you have it does a fair few conversions into utimbuf format
> (both directly and via the existing functions which it calls).  Would
> it be simpler if it dealt in terms of timespecs?
> 

It could be. Heck, it would be even simpler to just use a single time_t (since
cpio doesn't have timespecs OR seperate atime/mtimes):

static __initdata LIST_HEAD(dir_list);
struct dir_entry {
        struct list_head list;
        char *name;
        time_t mtime;
};

then make a simple wrapper for sys_utime() to convert the time_t into a the
utimbuf:

static void __init do_utime(char *name, time_t mtime)
{
        struct utimbuf time;
        time.actime=time.modtime=mtime;
        sys_utime(name, &time);
}

I can try it that way and resubmit. Gimme a bit to compose a patch

ALSO it disturbs me about the alloc problem you mentioned, especially since
i did NOT TEST on anything but our embedded (ppc/powerpc) target.

-- 
Nye Liu
nliu@mrv.com
(818) 772-6235x248
(818) 772-0576 fax

"Who would be stupid enough to quote a fictitious character?"
	-- Don Quixote

Re: [PATCH] INITRAMFS: Add option to preserve mtime from INITRAMFS cpio images

[Nye Liu]

On Wed, Sep 03, 2008 at 03:48:40PM -0700, Andrew Morton wrote:
> On Wed, 3 Sep 2008 15:41:31 -0700
> Nye Liu <nyet@nyet.org> wrote:
> 
> > > >  	collected[N_ALIGN(name_len) + body_len] = '\0';
> > > >  	clean_path(collected, 0);
> > > >  	sys_symlink(collected + N_ALIGN(name_len), collected);
> > > >  	sys_lchown(collected, uid, gid);
> > > > +	do_lutime(collected, &mtime);
> > > >  	state = SkipIt;
> > > >  	next_state = Reset;
> > > >  	return 0;
> > > > @@ -466,6 +520,7 @@ static char * __init unpack_to_rootfs(char *buf, unsigned len, int check_only)
> > > >  		buf += inptr;
> > > >  		len -= inptr;
> > > >  	}
> > > > +	dir_utime();
> > > 
> > > Perhaps this is the simplest implementation - I didn't check the fine
> > > details.  What's your thinking here?  
> > > 
> > 
> > The main problem is that i need to save off the entire list for later
> > processing of the directory mtimes... if i process the directory mtimes
> > in the same pass as the file/link mtimes, touching the directory inode
> > when creating/modifying the file/links updates the directory mtime, and
> > overwrites whatever mtime i set the directory to when i created it.
> > 
> > The only solution is to do a two pass, which is why the list is
> > necessary. If there is a better way, i did not find it :(
> > 
> > It could be that i misunderstood your question though :)
> 
> I'm wondering whether this code need to use `struct utimbuf' at all. 
> Or at least, as much as it does.  utimbuf is more a userspace-facing
> thing whereas in-kernel timespecs and timevals are more common.
> 
> The code as you have it does a fair few conversions into utimbuf format
> (both directly and via the existing functions which it calls).  Would
> it be simpler if it dealt in terms of timespecs?
> 

maybe something like this:


diff --git a/init/initramfs.c b/init/initramfs.c
index 644fc01..89d3e53 100644
--- a/init/initramfs.c
+++ b/init/initramfs.c
@@ -6,6 +6,7 @@
 #include <linux/delay.h>
 #include <linux/string.h>
 #include <linux/syscalls.h>
+#include <linux/utime.h>
 
 static __initdata char *message;
 static void __init error(char *x)
@@ -72,6 +73,45 @@ static void __init free_hash(void)
 	}
 }
 
+static __initdata LIST_HEAD(dir_list);
+struct dir_entry {
+	struct list_head list;
+	char *name;
+	time_t mtime;
+};
+
+static void __init dir_add(const char *name, time_t mtime)
+{
+	struct dir_entry *de = kmalloc(sizeof(struct dir_entry), GFP_KERNEL);
+	if (!de)
+		panic("can't allocate dir_entry buffer");
+	INIT_LIST_HEAD(&de->list);
+	de->name = kstrdup(name, GFP_KERNEL);
+	de->mtime = mtime;
+	list_add(&de->list, &dir_list);
+}
+
+static void __init do_utime(char *name, time_t mtime)
+{
+	struct utimbuf time;
+	time.actime=time.modtime=mtime;
+	sys_utime(name, &time);
+}
+
+static void __init dir_utime(void)
+{
+	struct list_head *e, *tmp;
+	list_for_each_safe(e, tmp, &dir_list) {
+		struct dir_entry *de = list_entry(e, struct dir_entry, list);
+		list_del(e);
+		do_utime(de->name, de->mtime);
+		kfree(de->name);
+		kfree(de);
+	}
+}
+
+static __initdata time_t mtime;
+
 /* cpio header parsing */
 
 static __initdata unsigned long ino, major, minor, nlink;
@@ -97,6 +137,7 @@ static void __init parse_header(char *s)
 	uid = parsed[2];
 	gid = parsed[3];
 	nlink = parsed[4];
+	mtime = parsed[5];
 	body_len = parsed[6];
 	major = parsed[7];
 	minor = parsed[8];
@@ -130,6 +171,7 @@ static inline void __init eat(unsigned n)
 	count -= n;
 }
 
+static __initdata char *vcollected;
 static __initdata char *collected;
 static __initdata int remains;
 static __initdata char *collect;
@@ -271,6 +313,7 @@ static int __init do_name(void)
 			if (wfd >= 0) {
 				sys_fchown(wfd, uid, gid);
 				sys_fchmod(wfd, mode);
+				vcollected = kstrdup(collected, GFP_KERNEL);
 				state = CopyFile;
 			}
 		}
@@ -278,12 +321,14 @@ static int __init do_name(void)
 		sys_mkdir(collected, mode);
 		sys_chown(collected, uid, gid);
 		sys_chmod(collected, mode);
+		dir_add(collected, mtime);
 	} else if (S_ISBLK(mode) || S_ISCHR(mode) ||
 		   S_ISFIFO(mode) || S_ISSOCK(mode)) {
 		if (maybe_link() == 0) {
 			sys_mknod(collected, mode, rdev);
 			sys_chown(collected, uid, gid);
 			sys_chmod(collected, mode);
+			do_utime(collected, mtime);
 		}
 	}
 	return 0;
@@ -294,6 +339,8 @@ static int __init do_copy(void)
 	if (count >= body_len) {
 		sys_write(wfd, victim, body_len);
 		sys_close(wfd);
+		do_utime(vcollected, mtime);
+		kfree(vcollected);
 		eat(body_len);
 		state = SkipIt;
 		return 0;
@@ -305,12 +352,26 @@ static int __init do_copy(void)
 	}
 }
 
+static long __init do_lutime(char __user *filename,
+	time_t mtime)
+{
+	struct timespec t[2];
+
+	t[0].tv_sec = mtime;
+	t[0].tv_nsec = 0;
+	t[1].tv_sec = mtime;
+	t[1].tv_nsec = 0;
+
+	return do_utimes(AT_FDCWD, filename, t, AT_SYMLINK_NOFOLLOW);
+}
+
 static int __init do_symlink(void)
 {
 	collected[N_ALIGN(name_len) + body_len] = '\0';
 	clean_path(collected, 0);
 	sys_symlink(collected + N_ALIGN(name_len), collected);
 	sys_lchown(collected, uid, gid);
+	do_lutime(collected, mtime);
 	state = SkipIt;
 	next_state = Reset;
 	return 0;
@@ -466,6 +527,7 @@ static char * __init unpack_to_rootfs(char *buf, unsigned len, int check_only)
 		buf += inptr;
 		len -= inptr;
 	}
+	dir_utime();
 	kfree(window);
 	kfree(name_buf);
 	kfree(symlink_buf);
-- 
Nye Liu
nliu@mrv.com
(818) 772-6235x248
(818) 772-0576 fax

"Who would be stupid enough to quote a fictitious character?"
	-- Don Quixote

Re: [PATCH] INITRAMFS: Add option to preserve mtime from INITRAMFS cpio images

[Nye Liu]

On Wed, Sep 03, 2008 at 03:48:40PM -0700, Andrew Morton wrote:
> On Wed, 3 Sep 2008 15:41:31 -0700
> Nye Liu <nyet@nyet.org> wrote:
> 
> > > >  	collected[N_ALIGN(name_len) + body_len] = '\0';
> > > >  	clean_path(collected, 0);
> > > >  	sys_symlink(collected + N_ALIGN(name_len), collected);
> > > >  	sys_lchown(collected, uid, gid);
> > > > +	do_lutime(collected, &mtime);
> > > >  	state = SkipIt;
> > > >  	next_state = Reset;
> > > >  	return 0;
> > > > @@ -466,6 +520,7 @@ static char * __init unpack_to_rootfs(char *buf, unsigned len, int check_only)
> > > >  		buf += inptr;
> > > >  		len -= inptr;
> > > >  	}
> > > > +	dir_utime();
> > > 
> > > Perhaps this is the simplest implementation - I didn't check the fine
> > > details.  What's your thinking here?  
> > > 
> > 
> > The main problem is that i need to save off the entire list for later
> > processing of the directory mtimes... if i process the directory mtimes
> > in the same pass as the file/link mtimes, touching the directory inode
> > when creating/modifying the file/links updates the directory mtime, and
> > overwrites whatever mtime i set the directory to when i created it.
> > 
> > The only solution is to do a two pass, which is why the list is
> > necessary. If there is a better way, i did not find it :(
> > 
> > It could be that i misunderstood your question though :)
> 
> I'm wondering whether this code need to use `struct utimbuf' at all. 
> Or at least, as much as it does.  utimbuf is more a userspace-facing
> thing whereas in-kernel timespecs and timevals are more common.
> 
> The code as you have it does a fair few conversions into utimbuf format
> (both directly and via the existing functions which it calls).  Would
> it be simpler if it dealt in terms of timespecs?
> 

or maybe this, this one ONLY using do_utimes() and a single wrapper
to convert the time_t

diff --git a/init/initramfs.c b/init/initramfs.c
index 644fc01..1360a67 100644
--- a/init/initramfs.c
+++ b/init/initramfs.c
@@ -6,6 +6,7 @@
 #include <linux/delay.h>
 #include <linux/string.h>
 #include <linux/syscalls.h>
+#include <linux/utime.h>
 
 static __initdata char *message;
 static void __init error(char *x)
@@ -72,6 +73,51 @@ static void __init free_hash(void)
 	}
 }
 
+static long __init do_utime(char __user *filename,
+	time_t mtime)
+{
+	struct timespec t[2];
+
+	t[0].tv_sec = mtime;
+	t[0].tv_nsec = 0;
+	t[1].tv_sec = mtime;
+	t[1].tv_nsec = 0;
+
+	return do_utimes(AT_FDCWD, filename, t, AT_SYMLINK_NOFOLLOW);
+}
+
+static __initdata LIST_HEAD(dir_list);
+struct dir_entry {
+	struct list_head list;
+	char *name;
+	time_t mtime;
+};
+
+static void __init dir_add(const char *name, time_t mtime)
+{
+	struct dir_entry *de = kmalloc(sizeof(struct dir_entry), GFP_KERNEL);
+	if (!de)
+		panic("can't allocate dir_entry buffer");
+	INIT_LIST_HEAD(&de->list);
+	de->name = kstrdup(name, GFP_KERNEL);
+	de->mtime = mtime;
+	list_add(&de->list, &dir_list);
+}
+
+static void __init dir_utime(void)
+{
+	struct list_head *e, *tmp;
+	list_for_each_safe(e, tmp, &dir_list) {
+		struct dir_entry *de = list_entry(e, struct dir_entry, list);
+		list_del(e);
+		do_utime(de->name, de->mtime);
+		kfree(de->name);
+		kfree(de);
+	}
+}
+
+static __initdata time_t mtime;
+
 /* cpio header parsing */
 
 static __initdata unsigned long ino, major, minor, nlink;
@@ -97,6 +143,7 @@ static void __init parse_header(char *s)
 	uid = parsed[2];
 	gid = parsed[3];
 	nlink = parsed[4];
+	mtime = parsed[5];
 	body_len = parsed[6];
 	major = parsed[7];
 	minor = parsed[8];
@@ -130,6 +177,7 @@ static inline void __init eat(unsigned n)
 	count -= n;
 }
 
+static __initdata char *vcollected;
 static __initdata char *collected;
 static __initdata int remains;
 static __initdata char *collect;
@@ -271,6 +319,7 @@ static int __init do_name(void)
 			if (wfd >= 0) {
 				sys_fchown(wfd, uid, gid);
 				sys_fchmod(wfd, mode);
+				vcollected = kstrdup(collected, GFP_KERNEL);
 				state = CopyFile;
 			}
 		}
@@ -278,12 +327,14 @@ static int __init do_name(void)
 		sys_mkdir(collected, mode);
 		sys_chown(collected, uid, gid);
 		sys_chmod(collected, mode);
+		dir_add(collected, mtime);
 	} else if (S_ISBLK(mode) || S_ISCHR(mode) ||
 		   S_ISFIFO(mode) || S_ISSOCK(mode)) {
 		if (maybe_link() == 0) {
 			sys_mknod(collected, mode, rdev);
 			sys_chown(collected, uid, gid);
 			sys_chmod(collected, mode);
+			do_utime(collected, mtime);
 		}
 	}
 	return 0;
@@ -294,6 +345,8 @@ static int __init do_copy(void)
 	if (count >= body_len) {
 		sys_write(wfd, victim, body_len);
 		sys_close(wfd);
+		do_utime(vcollected, mtime);
+		kfree(vcollected);
 		eat(body_len);
 		state = SkipIt;
 		return 0;
@@ -311,6 +364,7 @@ static int __init do_symlink(void)
 	clean_path(collected, 0);
 	sys_symlink(collected + N_ALIGN(name_len), collected);
 	sys_lchown(collected, uid, gid);
+	do_utime(collected, mtime);
 	state = SkipIt;
 	next_state = Reset;
 	return 0;
@@ -466,6 +520,7 @@ static char * __init unpack_to_rootfs(char *buf, unsigned len, int check_only)
 		buf += inptr;
 		len -= inptr;
 	}
+	dir_utime();
 	kfree(window);
 	kfree(name_buf);
 	kfree(symlink_buf);
-- 
Nye Liu
nliu@mrv.com
(818) 772-6235x248
(818) 772-0576 fax

"Who would be stupid enough to quote a fictitious character?"
	-- Don Quixote

Re: [PATCH] INITRAMFS: Add option to preserve mtime from INITRAMFS cpio images

[Andrew Morton]

On Wed, 3 Sep 2008 16:04:30 -0700
Nye Liu <nyet@mrv.com> wrote:

> On Wed, Sep 03, 2008 at 03:48:40PM -0700, Andrew Morton wrote:
> > On Wed, 3 Sep 2008 15:41:31 -0700
> > Nye Liu <nyet@nyet.org> wrote:
> > 
> > > > >  	collected[N_ALIGN(name_len) + body_len] = '\0';
> > > > >  	clean_path(collected, 0);
> > > > >  	sys_symlink(collected + N_ALIGN(name_len), collected);
> > > > >  	sys_lchown(collected, uid, gid);
> > > > > +	do_lutime(collected, &mtime);
> > > > >  	state = SkipIt;
> > > > >  	next_state = Reset;
> > > > >  	return 0;
> > > > > @@ -466,6 +520,7 @@ static char * __init unpack_to_rootfs(char *buf, unsigned len, int check_only)
> > > > >  		buf += inptr;
> > > > >  		len -= inptr;
> > > > >  	}
> > > > > +	dir_utime();
> > > > 
> > > > Perhaps this is the simplest implementation - I didn't check the fine
> > > > details.  What's your thinking here?  
> > > > 
> > > 
> > > The main problem is that i need to save off the entire list for later
> > > processing of the directory mtimes... if i process the directory mtimes
> > > in the same pass as the file/link mtimes, touching the directory inode
> > > when creating/modifying the file/links updates the directory mtime, and
> > > overwrites whatever mtime i set the directory to when i created it.
> > > 
> > > The only solution is to do a two pass, which is why the list is
> > > necessary. If there is a better way, i did not find it :(
> > > 
> > > It could be that i misunderstood your question though :)
> > 
> > I'm wondering whether this code need to use `struct utimbuf' at all. 
> > Or at least, as much as it does.  utimbuf is more a userspace-facing
> > thing whereas in-kernel timespecs and timevals are more common.
> > 
> > The code as you have it does a fair few conversions into utimbuf format
> > (both directly and via the existing functions which it calls).  Would
> > it be simpler if it dealt in terms of timespecs?
> > 
> 
> or maybe this, this one ONLY using do_utimes() and a single wrapper
> to convert the time_t

Getting better ;)

> diff --git a/init/initramfs.c b/init/initramfs.c
> index 644fc01..1360a67 100644
> --- a/init/initramfs.c
> +++ b/init/initramfs.c
> @@ -6,6 +6,7 @@
>  #include <linux/delay.h>
>  #include <linux/string.h>
>  #include <linux/syscalls.h>
> +#include <linux/utime.h>
>  
>  static __initdata char *message;
>  static void __init error(char *x)
> @@ -72,6 +73,51 @@ static void __init free_hash(void)
>  	}
>  }
>  
> +static long __init do_utime(char __user *filename,
> +	time_t mtime)

Please avoid wrapping things which don't need it.

> +{
> +	struct timespec t[2];
> +
> +	t[0].tv_sec = mtime;
> +	t[0].tv_nsec = 0;
> +	t[1].tv_sec = mtime;
> +	t[1].tv_nsec = 0;

Sub-second information is lost.  It'd be nice to preserve it if we're
going to do this thing.

> +	return do_utimes(AT_FDCWD, filename, t, AT_SYMLINK_NOFOLLOW);
> +}
> +
> +static __initdata LIST_HEAD(dir_list);
> +struct dir_entry {
> +	struct list_head list;
> +	char *name;
> +	time_t mtime;
> +};
> +
> +static void __init dir_add(const char *name, time_t mtime)
> +{
> +	struct dir_entry *de = kmalloc(sizeof(struct dir_entry), GFP_KERNEL);
> +	if (!de)
> +		panic("can't allocate dir_entry buffer");
> +	INIT_LIST_HEAD(&de->list);
> +	de->name = kstrdup(name, GFP_KERNEL);

Alas, kstrdup() can fail too.

It's all a bit silly checking for kmalloc() failures at this stage in
the boot process.  Particularly if all we can do is panic - we might as
well blunder ahead and dereference the NULL pointer, which gives us
just as much info as the panic.  And there's not much point in handling
the allocation and continuing the boot, because the system obviously
won't be booting.

So it'd be understandable to just omit the error-checking here, despite
my earlier mentioning of its absence ;)

> +	de->mtime = mtime;
> +	list_add(&de->list, &dir_list);
> +}
> +
> +static void __init dir_utime(void)
> +{
> +	struct list_head *e, *tmp;
> +	list_for_each_safe(e, tmp, &dir_list) {
> +		struct dir_entry *de = list_entry(e, struct dir_entry, list);

could use list_for_each_entry_safe() here.

> +		list_del(e);
> +		do_utime(de->name, de->mtime);
> +		kfree(de->name);
> +		kfree(de);
> +	}
> +}

Re: [PATCH] INITRAMFS: Add option to preserve mtime from INITRAMFS cpio images

[Nye Liu]

On Wed, Sep 03, 2008 at 04:19:55PM -0700, Andrew Morton wrote:
> On Wed, 3 Sep 2008 16:04:30 -0700
> Nye Liu <nyet@mrv.com> wrote:
> 
> > On Wed, Sep 03, 2008 at 03:48:40PM -0700, Andrew Morton wrote:
> > > On Wed, 3 Sep 2008 15:41:31 -0700
> > > Nye Liu <nyet@nyet.org> wrote:
> > > 
> > > > > >  	collected[N_ALIGN(name_len) + body_len] = '\0';
> > > > > >  	clean_path(collected, 0);
> > > > > >  	sys_symlink(collected + N_ALIGN(name_len), collected);
> > > > > >  	sys_lchown(collected, uid, gid);
> > > > > > +	do_lutime(collected, &mtime);
> > > > > >  	state = SkipIt;
> > > > > >  	next_state = Reset;
> > > > > >  	return 0;
> > > > > > @@ -466,6 +520,7 @@ static char * __init unpack_to_rootfs(char *buf, unsigned len, int check_only)
> > > > > >  		buf += inptr;
> > > > > >  		len -= inptr;
> > > > > >  	}
> > > > > > +	dir_utime();
> > > > > 
> > > > > Perhaps this is the simplest implementation - I didn't check the fine
> > > > > details.  What's your thinking here?  
> > > > > 
> > > > 
> > > > The main problem is that i need to save off the entire list for later
> > > > processing of the directory mtimes... if i process the directory mtimes
> > > > in the same pass as the file/link mtimes, touching the directory inode
> > > > when creating/modifying the file/links updates the directory mtime, and
> > > > overwrites whatever mtime i set the directory to when i created it.
> > > > 
> > > > The only solution is to do a two pass, which is why the list is
> > > > necessary. If there is a better way, i did not find it :(
> > > > 
> > > > It could be that i misunderstood your question though :)
> > > 
> > > I'm wondering whether this code need to use `struct utimbuf' at all. 
> > > Or at least, as much as it does.  utimbuf is more a userspace-facing
> > > thing whereas in-kernel timespecs and timevals are more common.
> > > 
> > > The code as you have it does a fair few conversions into utimbuf format
> > > (both directly and via the existing functions which it calls).  Would
> > > it be simpler if it dealt in terms of timespecs?
> > > 
> > 
> > or maybe this, this one ONLY using do_utimes() and a single wrapper
> > to convert the time_t
> 
> Getting better ;)
> 
> > diff --git a/init/initramfs.c b/init/initramfs.c
> > index 644fc01..1360a67 100644
> > --- a/init/initramfs.c
> > +++ b/init/initramfs.c
> > @@ -6,6 +6,7 @@
> >  #include <linux/delay.h>
> >  #include <linux/string.h>
> >  #include <linux/syscalls.h>
> > +#include <linux/utime.h>
> >  
> >  static __initdata char *message;
> >  static void __init error(char *x)
> > @@ -72,6 +73,51 @@ static void __init free_hash(void)
> >  	}
> >  }
> >  
> > +static long __init do_utime(char __user *filename,
> > +	time_t mtime)
> 
> Please avoid wrapping things which don't need it.

Hard to avoid, unless you want me composing the following every place 
i'm currently calling do_utime():

> > +{
> > +	struct timespec t[2];
> > +
> > +	t[0].tv_sec = mtime;
> > +	t[0].tv_nsec = 0;
> > +	t[1].tv_sec = mtime;
> > +	t[1].tv_nsec = 0;
> 
> Sub-second information is lost.  It'd be nice to preserve it if we're
> going to do this thing.

Can't! cpio isn't that smart!

> 
> > +	return do_utimes(AT_FDCWD, filename, t, AT_SYMLINK_NOFOLLOW);
> > +}
> > +
> > +static __initdata LIST_HEAD(dir_list);
> > +struct dir_entry {
> > +	struct list_head list;
> > +	char *name;
> > +	time_t mtime;
> > +};
> > +
> > +static void __init dir_add(const char *name, time_t mtime)
> > +{
> > +	struct dir_entry *de = kmalloc(sizeof(struct dir_entry), GFP_KERNEL);
> > +	if (!de)
> > +		panic("can't allocate dir_entry buffer");
> > +	INIT_LIST_HEAD(&de->list);
> > +	de->name = kstrdup(name, GFP_KERNEL);
> 
> Alas, kstrdup() can fail too.
> 
> It's all a bit silly checking for kmalloc() failures at this stage in
> the boot process.  Particularly if all we can do is panic - we might as
> well blunder ahead and dereference the NULL pointer, which gives us
> just as much info as the panic.  And there's not much point in handling
> the allocation and continuing the boot, because the system obviously
> won't be booting.
> 
> So it'd be understandable to just omit the error-checking here, despite
> my earlier mentioning of its absence ;)

LOL. You're the boss.

> 
> > +	de->mtime = mtime;
> > +	list_add(&de->list, &dir_list);
> > +}
> > +
> > +static void __init dir_utime(void)
> > +{
> > +	struct list_head *e, *tmp;
> > +	list_for_each_safe(e, tmp, &dir_list) {
> > +		struct dir_entry *de = list_entry(e, struct dir_entry, list);
> 
> could use list_for_each_entry_safe() here.

static void __init dir_utime(void)
{
        struct dir_entry *de, *tmp;
        list_for_each_entry_safe(de, tmp, &dir_list, list) {
                list_del(de);
                do_utime(de->name, de->mtime);
                kfree(de->name);
                kfree(de);
        }
}

something like that? I assume list_del(de) is fine.

> > +		list_del(e);
> > +		do_utime(de->name, de->mtime);
> > +		kfree(de->name);
> > +		kfree(de);
> > +	}
> > +}
> 

-- 
Nye Liu
nliu@mrv.com
(818) 772-6235x248
(818) 772-0576 fax

"Who would be stupid enough to quote a fictitious character?"
	-- Don Quixote

[PATCH] INITRAMFS: Preserve mtime from INITRAMFS cpio images

[Nye Liu]

one last shot :)

supercedes
initramfs-add-option-to-preserve-mtime-from-initramfs-cpio-images.patch

Signed-off-by: Nye Liu <nyet@nyet.org>

diff --git a/init/initramfs.c b/init/initramfs.c
index 644fc01..4f5ba75 100644
--- a/init/initramfs.c
+++ b/init/initramfs.c
@@ -6,6 +6,7 @@
 #include <linux/delay.h>
 #include <linux/string.h>
 #include <linux/syscalls.h>
+#include <linux/utime.h>
 
 static __initdata char *message;
 static void __init error(char *x)
@@ -72,6 +73,49 @@ static void __init free_hash(void)
 	}
 }
 
+static long __init do_utime(char __user *filename, time_t mtime)
+{
+	struct timespec t[2];
+
+	t[0].tv_sec = mtime;
+	t[0].tv_nsec = 0;
+	t[1].tv_sec = mtime;
+	t[1].tv_nsec = 0;
+
+	return do_utimes(AT_FDCWD, filename, t, AT_SYMLINK_NOFOLLOW);
+}
+
+static __initdata LIST_HEAD(dir_list);
+struct dir_entry {
+	struct list_head list;
+	char *name;
+	time_t mtime;
+};
+
+static void __init dir_add(const char *name, time_t mtime)
+{
+	struct dir_entry *de = kmalloc(sizeof(struct dir_entry), GFP_KERNEL);
+	if (!de)
+		panic("can't allocate dir_entry buffer");
+	INIT_LIST_HEAD(&de->list);
+	de->name = kstrdup(name, GFP_KERNEL);
+	de->mtime = mtime;
+	list_add(&de->list, &dir_list);
+}
+
+static void __init dir_utime(void)
+{
+	struct dir_entry *de, *tmp;
+	list_for_each_entry_safe(de, tmp, &dir_list, list) {
+		list_del(&de->list);
+		do_utime(de->name, de->mtime);
+		kfree(de->name);
+		kfree(de);
+	}
+}
+
+static __initdata time_t mtime;
+
 /* cpio header parsing */
 
 static __initdata unsigned long ino, major, minor, nlink;
@@ -97,6 +141,7 @@ static void __init parse_header(char *s)
 	uid = parsed[2];
 	gid = parsed[3];
 	nlink = parsed[4];
+	mtime = parsed[5];
 	body_len = parsed[6];
 	major = parsed[7];
 	minor = parsed[8];
@@ -130,6 +175,7 @@ static inline void __init eat(unsigned n)
 	count -= n;
 }
 
+static __initdata char *vcollected;
 static __initdata char *collected;
 static __initdata int remains;
 static __initdata char *collect;
@@ -271,6 +317,7 @@ static int __init do_name(void)
 			if (wfd >= 0) {
 				sys_fchown(wfd, uid, gid);
 				sys_fchmod(wfd, mode);
+				vcollected = kstrdup(collected, GFP_KERNEL);
 				state = CopyFile;
 			}
 		}
@@ -278,12 +325,14 @@ static int __init do_name(void)
 		sys_mkdir(collected, mode);
 		sys_chown(collected, uid, gid);
 		sys_chmod(collected, mode);
+		dir_add(collected, mtime);
 	} else if (S_ISBLK(mode) || S_ISCHR(mode) ||
 		   S_ISFIFO(mode) || S_ISSOCK(mode)) {
 		if (maybe_link() == 0) {
 			sys_mknod(collected, mode, rdev);
 			sys_chown(collected, uid, gid);
 			sys_chmod(collected, mode);
+			do_utime(collected, mtime);
 		}
 	}
 	return 0;
@@ -294,6 +343,8 @@ static int __init do_copy(void)
 	if (count >= body_len) {
 		sys_write(wfd, victim, body_len);
 		sys_close(wfd);
+		do_utime(vcollected, mtime);
+		kfree(vcollected);
 		eat(body_len);
 		state = SkipIt;
 		return 0;
@@ -311,6 +362,7 @@ static int __init do_symlink(void)
 	clean_path(collected, 0);
 	sys_symlink(collected + N_ALIGN(name_len), collected);
 	sys_lchown(collected, uid, gid);
+	do_utime(collected, mtime);
 	state = SkipIt;
 	next_state = Reset;
 	return 0;
@@ -466,6 +518,7 @@ static char * __init unpack_to_rootfs(char *buf, unsigned len, int check_only)
 		buf += inptr;
 		len -= inptr;
 	}
+	dir_utime();
 	kfree(window);
 	kfree(name_buf);
 	kfree(symlink_buf);
On Wed, Sep 03, 2008 at 04:41:44PM -0700, Andrew Morton wrote:
> On Wed, 3 Sep 2008 16:30:56 -0700
> Nye Liu <nyet@mrv.com> wrote:
> 
> > > > +static long __init do_utime(char __user *filename,
> > > > +	time_t mtime)
> > > 
> > > Please avoid wrapping things which don't need it.
> > 
> > Hard to avoid, unless you want me composing the following every place 
> > i'm currently calling do_utime():
> 
> I meant "wordwrapping":
> 
> static long __init do_utime(char __user *filename, time_t mtime)
> 
> easily fits in 80-cols.

-- 
Nye Liu
nliu@mrv.com
(818) 772-6235x248
(818) 772-0576 fax

"Who would be stupid enough to quote a fictitious character?"
	-- Don Quixote