On Mon, Jan 25, 2016 at 12:09:34PM +0100, walter harms wrote:
>
>
> Am 23.12.2015 21:04, schrieb Guillaume Nault:
> > @@ -1012,7 +1017,24 @@ static int ppp_dev_configure(struct net *src_net,
> > struct net_device *dev,
> > int indx;
> > int err;
> >
> > - file = conf->file;
> > + if (conf->fd >= 0) {
> > + file = fget(conf->fd);
> > + if (file) {
> > + if (file->f_op != &ppp_device_fops) {
> > + fput(file);
> > + return -EBADF;
> > + }
> > +
> > + /* Don't hold reference on file: ppp_release() is
> > + * responsible for safely freeing the associated
> > + * resources upon release. So file won't go away
> > + * from under us.
> > + */
> > + fput(file);
> > + }
> > + } else {
> > + file = conf->file;
> > + }
> > if (!file)
> > return -EBADF;
>
>
> I would write that a bid different to reduce indent
> und improve readability
>
> (note: totaly untested just reviewing)
>
> if (conf->fd < 0) {
> file = conf->file;
> if (!file)
> return -EBADF;
> }
> else
> {
> file = fget(conf->fd);
> if (!file)
> return -EBADF;
>
Early return on fget() failure looks indeed simpler.
> fput(file);
> if (file->f_op != &ppp_device_fops) {
> return -EBADF;
> }
>
But this is wrong: we can't act on file after fput(). So we have to
place fput() after the test.
Thanks for your review.
--
To unsubscribe from this list: send the line "unsubscribe linux-ppp" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
