Re: [PATCH] INITRAMFS: Add option to preserve mtime from INITRAMFScpio 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@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.

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@xxxxxxx
(818) 772-6235x248
(818) 772-0576 fax

"Who would be stupid enough to quote a fictitious character?"
	-- Don Quixote
--
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/