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

[Nye Liu <nyet@nyet.org>]

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 :)