Theodore Y. Ts'o wrote:
> On Mon, May 07, 2018 at 08:16:58PM +0900, Tetsuo Handa wrote:
> > Oh, your message did not arrive at news.gmane.org and I didn't notice that
> > you
> > already wrote this patch. But please update yours or review mine shown
> > below.
> >
> > > @@ -673,14 +703,13 @@ static int loop_change_fd(struct loop_device *lo,
> > > struct block_device *bdev,
> > > if (!file)
> > > goto out;
> > >
> > > + error = loop_validate_file(file, bdev);
> > > + if (error)
> > > + goto out_putf;
> > > +
> > > inode = file->f_mapping->host;
> > > old_file = lo->lo_backing_file;
> > >
> > > - error = -EINVAL;
> > > -
> > > - if (!S_ISREG(inode->i_mode) && !S_ISBLK(inode->i_mode))
> > > - goto out_putf;
> > > -
> > > /* size of the new backing store needs to be the same */
> > > if (get_loop_size(lo, file) != get_loop_size(lo, old_file))
> > > goto out_putf;
> >
> > error == 0 upon "goto out_putf" is wrong.
>
> I don't understand your concern; where are we going to out_putf when
> error == 0?
- error = -EINVAL; /* <= You are trying to remove this assignment. */
-
- if (!S_ISREG(inode->i_mode) && !S_ISBLK(inode->i_mode))
- goto out_putf;
/* size of the new backing store needs to be the same */
if (get_loop_size(lo, file) != get_loop_size(lo, old_file))
goto out_putf; /* <= Hence error == 0 at this point. */
By the way, are you aware that current "/* Avoid recursion */" loop is not
thread safe?
