[PATCH 1/2] Add ipvlan support

[Thomas Haller]

On Mon, 2015-06-22 at 09:43 -0700, Cong Wang wrote:
> On Fri, Jun 19, 2015 at 9:14 AM, Thomas Haller <thaller at redhat.com> 
> wrote:
> > Hi Cong,
> > 
> > Thanks for the patches. Both unmodified applied as
> > https://github.com/thom311/libnl/commit/12e87e444dc4ffd4e5d3ca2e99b
> > 08e4
> > e22665a86
> > 
> > 
> 
> Thanks!
> 
> > 
> > 
> > Two follow-up questions though...
> > 
> > 
> > > +int rtnl_link_ipvlan_set_mode(struct rtnl_link *link, uint16_t 
> > > mode)
> > > +{
> > > +     struct ipvlan_info *ipi = link->l_info;
> > > +
> > > +     IS_IPVLAN_LINK_ASSERT(link);
> > > +
> > > +     if (mode != IPVLAN_MODE_L2 && mode != IPVLAN_MODE_L3)
> > > +             return -NLE_INVAL;
> > > +
> > > +     ipi->ipi_mode = mode;
> > > +     ipi->ipi_mask |= IPVLAN_HAS_MODE;
> > > +
> > > +     return 0;
> > > +}
> > 
> > wouldn't it be better to allow any mode? Otherwise, if a new mode 
> > gets
> > added, libnl needs to be patched too.
> > Or do you think it's highly unlikely that another mode will be 
> > added in
> > the future?
> > 
> 
> We could add more modes in the future, but it should be fine to
> add them here too, right? I mean, it is valid to change:
> 
> rtnl_link_ipvlan_set_mode(NEW_MODE) == -EINVAL
> 
> to:
> 
> rtnl_link_ipvlan_set_mode(NEW_MODE) == 0
> 
> It is not considered as an ABI breakage.

Yes. But when adding NEW_MODE to kernel, old libnl3 does't work with
that mode until upgrading the library too.

I would just drop the check.

Pushed https://github.com/thom311/libnl/commit/371226b834b662e6ad99400f
fc20450e574ca5c2

If you disagree, we can revert/rework that.





> > > +/**
> > > + * Get IPVLAN Mode
> > > + * @arg link         Link object
> > > + *
> > > + * @return IPVLAN mode, 0 if not set or a negative error code.
> > > + */
> > > +uint16_t rtnl_link_ipvlan_get_mode(struct rtnl_link *link)
> > > +{
> > > +     struct ipvlan_info *ipi = link->l_info;
> > > +
> > > +     IS_IPVLAN_LINK_ASSERT(link);
> > > +
> > > +     if (ipi->ipi_mask & IPVLAN_HAS_MODE)
> > > +             return ipi->ipi_mode;
> > > +     else
> > > +             return 0;
> > > +}
> > 
> > 0 is equal to IPVLAN_MODE_L2, so you cannot signal an error.
> > Also, the function cannot ever return a negative error code.
> > 
> > How about changing the signature to:
> > 
> > int rtnl_link_ipvlan_get_mode(struct rtnl_link *link, uint16_t 
> > *out_mode);
> > 
> 
> Good catch! Sounds good to me. Let me know if you want me
> to send a patch or you will do it.

done as
https://github.com/thom311/libnl/commit/b4afcadc30f0133135c6f8b062d6e1c
a68b645a7


Looks good?


Thomas
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part
URL: <http://lists.infradead.org/pipermail/libnl/attachments/20150626/0a901d44/attachment.sig>