[PATCH 1/2] Add ipvlan support

[Cong Wang]

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/12e87e444dc4ffd4e5d3ca2e99b08e4
> 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.

>
>> +/**
>> + * 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.