| Message ID | b4ca4f2204f0ffb0bd7f959192462944a8d846a4.1384436410.git.mkubecek@suse.cz |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On 11/14/2013 09:00 AM, Michal Kubecek wrote: > Introduce helper function macvlan_dev_real_dev which returns the > underlying device of a macvlan device, similar to vlan_dev_real_dev() > for 802.1q VLAN devices. > > v2: IFF_MACVLAN flag and equivalent of is_macvlan_dev() were > introduced in the meantime > > Signed-off-by: Michal Kubecek <mkubecek@suse.cz> > --- > include/linux/if_macvlan.h | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) > > diff --git a/include/linux/if_macvlan.h b/include/linux/if_macvlan.h > index c270285..ac9aab2 100644 > --- a/include/linux/if_macvlan.h > +++ b/include/linux/if_macvlan.h > @@ -119,4 +119,20 @@ extern int macvlan_link_register(struct rtnl_link_ops *ops); > extern netdev_tx_t macvlan_start_xmit(struct sk_buff *skb, > struct net_device *dev); > > +#if IS_ENABLED(CONFIG_MACVLAN) > +static inline struct net_device * > +macvlan_dev_real_dev(const struct net_device *dev) > +{ > + struct macvlan_dev *macvlan = netdev_priv(dev); > + > + return macvlan->lowerdev; > +} > +#else > +static inline struct net_device * > +macvlan_dev_real_dev(const struct net_device *dev) > +{ > + return NULL; > +} > +#endif > + You may want to do the same here as was done for vlan_dev_real_dev(). This function is not intended to be called blindly and should always be called after netif_is_macvlan(). -vlad > #endif /* _LINUX_IF_MACVLAN_H */ > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Nov 14, 2013 at 10:03:19AM -0500, Vlad Yasevich wrote: > On 11/14/2013 09:00 AM, Michal Kubecek wrote: > >+#if IS_ENABLED(CONFIG_MACVLAN) > >+static inline struct net_device * > >+macvlan_dev_real_dev(const struct net_device *dev) > >+{ > >+ struct macvlan_dev *macvlan = netdev_priv(dev); > >+ > >+ return macvlan->lowerdev; > >+} > >+#else > >+static inline struct net_device * > >+macvlan_dev_real_dev(const struct net_device *dev) > >+{ > >+ return NULL; > >+} > >+#endif > >+ > > You may want to do the same here as was done for > vlan_dev_real_dev(). This function is not intended to be called > blindly and should always > be called after netif_is_macvlan(). I'm not sure. It makes sense from the developer point of view: if we find an inconsistency which must be caused by a bug in kernel code, do panic so that the bug is found and fixed as soon as possible. However, I remember a discussion where the point was that BUG() and BUG_ON() should only be used if there is no way to recover. From this point of view, WARN or WARN_ONCE might be better choice - but I'm not strictly opposed to BUG(). Michal Kubecek -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Michal Kubecek <mkubecek@suse.cz> Date: Thu, 14 Nov 2013 16:57:57 +0100 > On Thu, Nov 14, 2013 at 10:03:19AM -0500, Vlad Yasevich wrote: >> On 11/14/2013 09:00 AM, Michal Kubecek wrote: >> >+#if IS_ENABLED(CONFIG_MACVLAN) >> >+static inline struct net_device * >> >+macvlan_dev_real_dev(const struct net_device *dev) >> >+{ >> >+ struct macvlan_dev *macvlan = netdev_priv(dev); >> >+ >> >+ return macvlan->lowerdev; >> >+} >> >+#else >> >+static inline struct net_device * >> >+macvlan_dev_real_dev(const struct net_device *dev) >> >+{ >> >+ return NULL; >> >+} >> >+#endif >> >+ >> >> You may want to do the same here as was done for >> vlan_dev_real_dev(). This function is not intended to be called >> blindly and should always >> be called after netif_is_macvlan(). > > I'm not sure. It makes sense from the developer point of view: if we > find an inconsistency which must be caused by a bug in kernel code, do > panic so that the bug is found and fixed as soon as possible. However, > I remember a discussion where the point was that BUG() and BUG_ON() > should only be used if there is no way to recover. From this point of > view, WARN or WARN_ONCE might be better choice - but I'm not strictly > opposed to BUG(). At least for the time being use BUG(), to be consistent with the same way how VLAN handles this situation. Thanks. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/14/2013 10:57 AM, Michal Kubecek wrote: > On Thu, Nov 14, 2013 at 10:03:19AM -0500, Vlad Yasevich wrote: >> On 11/14/2013 09:00 AM, Michal Kubecek wrote: >>> +#if IS_ENABLED(CONFIG_MACVLAN) >>> +static inline struct net_device * >>> +macvlan_dev_real_dev(const struct net_device *dev) >>> +{ >>> + struct macvlan_dev *macvlan = netdev_priv(dev); >>> + >>> + return macvlan->lowerdev; >>> +} >>> +#else >>> +static inline struct net_device * >>> +macvlan_dev_real_dev(const struct net_device *dev) >>> +{ >>> + return NULL; >>> +} >>> +#endif >>> + >> >> You may want to do the same here as was done for >> vlan_dev_real_dev(). This function is not intended to be called >> blindly and should always >> be called after netif_is_macvlan(). > > I'm not sure. It makes sense from the developer point of view: if we > find an inconsistency which must be caused by a bug in kernel code, do > panic so that the bug is found and fixed as soon as possible. However, > I remember a discussion where the point was that BUG() and BUG_ON() > should only be used if there is no way to recover. From this point of > view, WARN or WARN_ONCE might be better choice - but I'm not strictly > opposed to BUG(). > > Michal Kubecek > If someone blindly calls macvlan_dev_real_dev() and macvlan module is enabled, but there is no macvlan device, you are going to get garbage and crash. So crashing if module is disabled for the same case seems perfectly reasonable. -vlad -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Nov 14, 2013 at 05:03:58PM -0500, David Miller wrote: > >> On 11/14/2013 09:00 AM, Michal Kubecek wrote: > >> >+#if IS_ENABLED(CONFIG_MACVLAN) > >> >+static inline struct net_device * > >> >+macvlan_dev_real_dev(const struct net_device *dev) > >> >+{ > >> >+ struct macvlan_dev *macvlan = netdev_priv(dev); > >> >+ > >> >+ return macvlan->lowerdev; > >> >+} > >> >+#else > >> >+static inline struct net_device * > >> >+macvlan_dev_real_dev(const struct net_device *dev) > >> >+{ > >> >+ return NULL; > >> >+} > >> >+#endif > >> >+ > > At least for the time being use BUG(), to be consistent with the same > way how VLAN handles this situation. OK, v3 sent. Michal Kubecek -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/linux/if_macvlan.h b/include/linux/if_macvlan.h index c270285..ac9aab2 100644 --- a/include/linux/if_macvlan.h +++ b/include/linux/if_macvlan.h @@ -119,4 +119,20 @@ extern int macvlan_link_register(struct rtnl_link_ops *ops); extern netdev_tx_t macvlan_start_xmit(struct sk_buff *skb, struct net_device *dev); +#if IS_ENABLED(CONFIG_MACVLAN) +static inline struct net_device * +macvlan_dev_real_dev(const struct net_device *dev) +{ + struct macvlan_dev *macvlan = netdev_priv(dev); + + return macvlan->lowerdev; +} +#else +static inline struct net_device * +macvlan_dev_real_dev(const struct net_device *dev) +{ + return NULL; +} +#endif + #endif /* _LINUX_IF_MACVLAN_H */
Introduce helper function macvlan_dev_real_dev which returns the underlying device of a macvlan device, similar to vlan_dev_real_dev() for 802.1q VLAN devices. v2: IFF_MACVLAN flag and equivalent of is_macvlan_dev() were introduced in the meantime Signed-off-by: Michal Kubecek <mkubecek@suse.cz> --- include/linux/if_macvlan.h | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)