Message-ID: <53193B63.20905@windriver.com>
Date:	Fri, 7 Mar 2014 11:22:11 +0800
From:	Robert Yang <liezhi.yang@...driver.com>
To:	"Darrick J. Wong" <darrick.wong@...cle.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 03/07/2014 10:56 AM, Darrick J. Wong wrote:
> 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. :)
>

Thank you very much:-)

> 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.
>

Thank you very much, I will try later.

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

Thanks, got it.

// Robert

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