Patchwork [02/31] libext2fs: Only link an inode into a directory once

login
register
mail settings
Submitter Darrick J. Wong
Date Oct. 1, 2013, 1:26 a.m.
Message ID <20131001012655.28415.38313.stgit@birch.djwong.org>
Download mbox | patch
Permalink /patch/279282/
State Accepted
Headers show

Comments

Darrick J. Wong - Oct. 1, 2013, 1:26 a.m.
The ext2fs_link helper function link_proc does not check the value of ls->done,
which means that if the function finds multiple empty spaces that will fit the
new directory entry, it will create a directory entry in each of the spaces.
Instead of doing that, check the done value and don't do anything more if we've
already added the directory entry.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 lib/ext2fs/link.c |    3 +++
 1 file changed, 3 insertions(+)



--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
jon ernst - Oct. 1, 2013, 3:37 p.m.
On Mon, Sep 30, 2013 at 9:26 PM, Darrick J. Wong
<darrick.wong@oracle.com> wrote:
> The ext2fs_link helper function link_proc does not check the value of ls->done,
> which means that if the function finds multiple empty spaces that will fit the
> new directory entry, it will create a directory entry in each of the spaces.
> Instead of doing that, check the done value and don't do anything more if we've
> already added the directory entry.
>
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  lib/ext2fs/link.c |    3 +++
>  1 file changed, 3 insertions(+)
>
>
> diff --git a/lib/ext2fs/link.c b/lib/ext2fs/link.c
> index f715067..e3ff450 100644
> --- a/lib/ext2fs/link.c
> +++ b/lib/ext2fs/link.c
> @@ -43,6 +43,9 @@ static int link_proc(struct ext2_dir_entry *dirent,
>         int ret = 0;
>         int csum_size = 0;
>
> +       if (ls->done)
> +               return 0;
maybe "return ret;" here to keep consistency.
> +
>         rec_len = EXT2_DIR_REC_LEN(ls->namelen);
>
>         ls->err = ext2fs_get_rec_len(ls->fs, dirent, &curr_rec_len);
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@vger.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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Darrick J. Wong - Oct. 1, 2013, 9:11 p.m.
On Tue, Oct 01, 2013 at 11:37:50AM -0400, jon ernst wrote:
> On Mon, Sep 30, 2013 at 9:26 PM, Darrick J. Wong
> <darrick.wong@oracle.com> wrote:
> > The ext2fs_link helper function link_proc does not check the value of ls->done,
> > which means that if the function finds multiple empty spaces that will fit the
> > new directory entry, it will create a directory entry in each of the spaces.
> > Instead of doing that, check the done value and don't do anything more if we've
> > already added the directory entry.
> >
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  lib/ext2fs/link.c |    3 +++
> >  1 file changed, 3 insertions(+)
> >
> >
> > diff --git a/lib/ext2fs/link.c b/lib/ext2fs/link.c
> > index f715067..e3ff450 100644
> > --- a/lib/ext2fs/link.c
> > +++ b/lib/ext2fs/link.c
> > @@ -43,6 +43,9 @@ static int link_proc(struct ext2_dir_entry *dirent,
> >         int ret = 0;
> >         int csum_size = 0;
> >
> > +       if (ls->done)
> > +               return 0;
> maybe "return ret;" here to keep consistency.

Actually, DIRENT_ABORT is more appropriate here since the iteration stops.
I'll update the patch; thank you for pointing this out.

(This patch is years old now...)

--D
> > +
> >         rec_len = EXT2_DIR_REC_LEN(ls->namelen);
> >
> >         ls->err = ext2fs_get_rec_len(ls->fs, dirent, &curr_rec_len);
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> > the body of a message to majordomo@vger.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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Theodore Ts'o - Oct. 7, 2013, 1:17 p.m.
On Mon, Sep 30, 2013 at 06:26:55PM -0700, Darrick J. Wong wrote:
> The ext2fs_link helper function link_proc does not check the value of ls->done,
> which means that if the function finds multiple empty spaces that will fit the
> new directory entry, it will create a directory entry in each of the spaces.
> Instead of doing that, check the done value and don't do anything more if we've
> already added the directory entry.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Thanks, applied.

One minor nit, which I'll generally fix myself when I have the chance:

I generally like to fill comments at column 70, and in general, to try
to keep the summary line under 76 columns if possible, so that "git
log" doesn't wrap on 80 column screens.  It's not always possible to
do the latter, but it's something I will try to do.  Also, personally
prefer to use a lowercase to start the description since these
constraints generally mean the summary line can't be a complete
sentence.

						- Ted
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Darrick J. Wong - Oct. 7, 2013, 6:53 p.m.
On Mon, Oct 07, 2013 at 09:17:27AM -0400, Theodore Ts'o wrote:
> On Mon, Sep 30, 2013 at 06:26:55PM -0700, Darrick J. Wong wrote:
> > The ext2fs_link helper function link_proc does not check the value of ls->done,
> > which means that if the function finds multiple empty spaces that will fit the
> > new directory entry, it will create a directory entry in each of the spaces.
> > Instead of doing that, check the done value and don't do anything more if we've
> > already added the directory entry.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Thanks, applied.
> 
> One minor nit, which I'll generally fix myself when I have the chance:
> 
> I generally like to fill comments at column 70, and in general, to try
> to keep the summary line under 76 columns if possible, so that "git
> log" doesn't wrap on 80 column screens.  It's not always possible to
> do the latter, but it's something I will try to do.  Also, personally
> prefer to use a lowercase to start the description since these
> constraints generally mean the summary line can't be a complete
> sentence.

Ok.  Assuming that you're reformatting the descriptions of the patches you're
taking into your tree, I'll reformat the descriptions on the ones that don't
make it in or require rework.

--D
> 
> 						- Ted
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@vger.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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/lib/ext2fs/link.c b/lib/ext2fs/link.c
index f715067..e3ff450 100644
--- a/lib/ext2fs/link.c
+++ b/lib/ext2fs/link.c
@@ -43,6 +43,9 @@  static int link_proc(struct ext2_dir_entry *dirent,
 	int ret = 0;
 	int csum_size = 0;
 
+	if (ls->done)
+		return 0;
+
 	rec_len = EXT2_DIR_REC_LEN(ls->namelen);
 
 	ls->err = ext2fs_get_rec_len(ls->fs, dirent, &curr_rec_len);