Re: [PATCH 2/2] ppp: implement rtnetlink device handling

[Guillaume Nault]

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 != _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 != _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

Re: [PATCH 2/2] ppp: implement rtnetlink device handling

[walter harms]

Am 23.12.2015 21:04, schrieb Guillaume Nault:
> Define PPP device handler for use with rtnetlink.
> 
> The only PPP specific attribute is IFLA_PPP_DEV_FD. It is mandatory and
> contains the file descriptor of the associated /dev/ppp instance (the
> file descriptor which would have been used for ioctl(PPPIOCNEWUNIT) in
> the ioctl-based API).
> 
> PPP devices created with the rtnetlink API behave like the ones created
> with ioctl(PPPIOCNEWUNIT). In particular existing ioctls still apply to
> PPP units created with the rtnetlink API.
> 
> Signed-off-by: Guillaume Nault 
> ---
>  drivers/net/ppp/ppp_generic.c | 109 
> --
>  include/uapi/linux/if_link.h  |   8 
>  2 files changed, 113 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
> index bc24537..19476d6 100644
> --- a/drivers/net/ppp/ppp_generic.c
> +++ b/drivers/net/ppp/ppp_generic.c
> @@ -46,6 +46,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -185,7 +186,9 @@ struct channel {
>  
>  struct ppp_config {
>   struct file *file;
> + s32 fd;
>   s32 unit;
> + bool ifname_is_set;
>  };
>  
>  /*
> @@ -286,6 +289,7 @@ static int unit_get(struct idr *p, void *ptr);
>  static int unit_set(struct idr *p, void *ptr, int n);
>  static void unit_put(struct idr *p, int n);
>  static void *unit_find(struct idr *p, int n);
> +static void ppp_setup(struct net_device *dev);
>  
>  static const struct net_device_ops ppp_netdev_ops;
>  
> @@ -954,7 +958,7 @@ static struct pernet_operations ppp_net_ops = {
>   .size = sizeof(struct ppp_net),
>  };
>  
> -static int ppp_unit_register(struct ppp *ppp, int unit)
> +static int ppp_unit_register(struct ppp *ppp, int unit, bool ifname_is_set)
>  {
>   struct ppp_net *pn = ppp_pernet(ppp->ppp_net);
>   int ret;
> @@ -984,7 +988,8 @@ static int ppp_unit_register(struct ppp *ppp, int unit)
>  
>   ppp->file.index = ret;
>  
> - snprintf(ppp->dev->name, IFNAMSIZ, "ppp%i", ppp->file.index);
> + if (!ifname_is_set)
> + snprintf(ppp->dev->name, IFNAMSIZ, "ppp%i", ppp->file.index);
>  
>   ret = register_netdevice(ppp->dev);
>   if (ret)
> @@ -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 != _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;

fput(file);
if (file->f_op != _device_fops) {   
return -EBADF;
}

}


just my 2 cents

re,
 wh

>   if (file->private_data)
> @@ -1040,7 +1062,7 @@ static int ppp_dev_configure(struct net *src_net, 
> struct net_device *dev,
>   ppp->mru = PPP_MRU;
>   ppp->ppp_net = src_net;
>  
> - err = ppp_unit_register(ppp, conf->unit);
> + err = ppp_unit_register(ppp, conf->unit, conf->ifname_is_set);
>   if (err < 0)
>   return err;
>  
> @@ -1049,6 +1071,73 @@ static int ppp_dev_configure(struct net *src_net, 
> struct net_device *dev,
>   return 0;
>  }
>  
> +static const struct nla_policy ppp_nl_policy[IFLA_PPP_MAX + 1] = {
> + [IFLA_PPP_DEV_FD]   = { .type = NLA_S32 },
> +};
> +
> +static int ppp_nl_validate(struct nlattr *tb[], struct nlattr *data[])
> +{
> + if (!data)
> + return -EINVAL;
> +
> + if (!data[IFLA_PPP_DEV_FD])
> + return -EINVAL;
> + if (nla_get_s32(data[IFLA_PPP_DEV_FD]) < 0)
> + return -EBADF;
> +
> + return 0;
> +}
> +
> +static int ppp_nl_newlink(struct net *src_net, struct net_device *dev,
> +   struct nlattr *tb[], struct nlattr *data[])
> +{
> + struct ppp_config conf = {
> + .file = NULL,
> + .unit = -1,
> + .ifname_is_set = true,
> + };
> +
> + conf.fd = nla_get_s32(data[IFLA_PPP_DEV_FD]);
> +
> + return ppp_dev_configure(src_net, dev, );
> +}
> +
> +static void ppp_nl_dellink(struct