Message-ID: <20140307025630.GH9875@birch.djwong.org>
Date:	Thu, 6 Mar 2014 18:56:30 -0800
From:	"Darrick J. Wong" <darrick.wong@...cle.com>
To:	Robert Yang <liezhi.yang@...driver.com>
Cc:	"Theodore Ts'o" <tytso@....edu>, dvhart@...ux.intel.com,
	linux-ext4@...r.kernel.org
Subject: Re: [PATCH V4 05/11] misc/create_inode.c: copy regular file

On Fri, Mar 07, 2014 at 09:24:12AM +0800, Robert Yang wrote:
> 
> 
> On 03/07/2014 06:57 AM, Theodore Ts'o wrote:
> >On Thu, Mar 06, 2014 at 12:14:39PM -0800, Darrick J. Wong wrote:
> >>
> >>I'm already queuing up a bunch of (more) fixes... there's more weird things I
> >>didn't notice.  Such as, why is current_fs now defined in current_inode.h?
> >>That really ought to have stayed in debugfs.c, and current_inode.h should have
> >>'extern ext2_filsys current_fs;', no?
> >
> >Yes, that would be better --- although in the long term we should
> >probably try to get rid of the global variable and pass in an "fs"
> >parameter into functions in misc/create_inode.c.
> >
> >Since these aren't in a shared library, I wasn't worried that much
> >about the details of the abstraction interface, but I'm sure there are
> >some ways that we can improve things.
> >
> >BTW, one of my plans for 1.43 is to rename libquota.a to libe2int.a,
> >and to move things like profile.c, and other files shared between misc
> >and e2fsck, etc., into an "internal support" library.  I suspect
> >create_inoode.c would be a candidate for moving into this internal
> >support library.
> >
> 
> Hi Ted and Darrick,
> 
> Thank you very much for the great help, I think that I don't have to
> submit a fix patch again since Darrick has helped me to fix the
> problem, please feel free to let me know if there is anything I
> can do.

I'll have 6 patches for you to review soon.  I also fixed a number of style and
whitespace errors. :)

I had another thought about populate_fs -- it should be in charge of setting up
and tearing down the hdlinks_s hardlink map, not the caller, and it shouldn't
really be a global variable.  I noticed that populate_fs recursively calls
itself, so I moved the functionality to a static function and wrote a wrapper
that takes care of hdlinks and calls the static function.

By the way, one of the things I /didn't/ fix was the root inode parameter that
you pass to ext2fs_namei.  I couldn't tell if supporting debugfs' chroot
command is part of your requirements set (though it doesn't seem likely to me),
but I also think that a better interface would be to have callers of the
create_inode functions pass in the destination dir inode instead of a pathname,
similar to the do_mknod_internal interface.  debugfs is the only tool that
knows about the notion of a 'chroot'; the rest would seem to do all namei
operations starting at EXT2_ROOT_INO.

Also I recommend running sparse/cppcheck on any source files that a patch of
yours touches.

--D
> 
> // Robert
> 
> >Cheers,
> >
> >>...I'll also respin the patchset I sent out a few days ago.
> >
> >Sorry for having you respin the patchset yet again --- although
> >hopefully it should be easier this time around.  I'm trying to be fair
> >in catching up with th e2fsprogs backlog, and Robert and Zheng's
> >patches have been outstanding for a long time.  Don't worry, yours are
> >next on the list.  :-)
> >
> >Cheers,
> >
> >						- Ted
> >
> >
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists