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

[Andrew Morton]

On Wed, 3 Sep 2008 16:04:30 -0700
Nye Liu <nyet@xxxxxxx> 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@xxxxxxxx> 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);
> +	}
> +}

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/