[PATCH v2] make function rtnl_link_set_name() more reasonable

[Thomas Haller]

Hi Hongwei,


I like last code the best. I would be fine with that, if you
could send a [PATCH v3]?

@Teto, I wouldn't make this an #ifndef, because then the return
parameter in release mode is not useful at all.

Sorry, for the bikeshedding :)


Thanks,
Thomas



On Wed, 2014-01-22 at 21:59 +0800, Hongwei Bi wrote:
> Hi Thomas,
> 
> The full check is overkill surely. I think there are two method we can
> do to solve it.
> 
> first: we don't tune the code. when the length of name is equal or
> greater then 16, we just notify the user that  the length has been
> truncated to 15 bytes by means of modifying the code comment.
> 
> second:  tune the code as follows:
> 
> int rtnl_link_set_name(struct rtnl_link *link, const char *name)
> {
> int err = 0;
> if (strlen(name) >= IFNAMSIZ)
>    err = -1;
> 
> strncpy(link->l_name, name, sizeof(link->l_name) - 1);
> link->ce_mask |= LINK_ATTR_IFNAME;
> 
> return err;
> }
> 
> If you have any other suggests, please let me know. Thanks.
> 
> Hongwei
> 
> 
> 2014/1/22 Thomas Haller <thaller at redhat.com>:
> > Hi Hongwei,
> >
> > sorry, re-thinking this again... :)
> >
> > The users ~should~ provide valid arguments.
> > If they don't, it's basically a bug on their side and libnl is generally
> > not forgiving about that (just note, that most functions don't do any
> > input validation or range checking).
> >
> > If you want to validate the ifname, shouldn't you also check that the
> > whole name is valid? Something like
> > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/net/core/dev.c?id=d8ec26d7f8287f5788a494f56e8814210f0e64be#n929
> >
> > Otherwise the return value, just tells you, "yeah, it's maybe
> > valid" -- which isn't much use. And the full check I think is overkill.
> >
> > What do you guys think?
> >
> >
> > Thomas
> >
> >
> >
> > On Wed, 2014-01-15 at 22:47 +0800, Hongwei Bi wrote:
> >> We set the maximum value of IFNAMSIZ is 16 bytes.So when the length of
> >> the new link's name is equal or greater then 16,we should notify the user
> >> that the length of the name has been truncated to 15 bytes.
> >>
> >> Signed-off-by: Hongwei Bi <hwbi2008 at gmail.com>
> >> ---
> >>  include/netlink/route/link.h |  2 +-
> >>  lib/route/link.c             | 19 ++++++++++++++++++-
> >>  2 files changed, 19 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/include/netlink/route/link.h b/include/netlink/route/link.h
> >> index 3345787..566e224 100644
> >> --- a/include/netlink/route/link.h
> >> +++ b/include/netlink/route/link.h
> >> @@ -151,7 +151,7 @@ extern int        rtnl_link_str2carrier(const char *);
> >>  extern void  rtnl_link_set_qdisc(struct rtnl_link *, const char *);
> >>  extern char *        rtnl_link_get_qdisc(struct rtnl_link *);
> >>
> >> -extern void  rtnl_link_set_name(struct rtnl_link *, const char *);
> >> +extern int   rtnl_link_set_name(struct rtnl_link *, const char *);
> >>  extern char *        rtnl_link_get_name(struct rtnl_link *);
> >>
> >>  extern void  rtnl_link_set_group(struct rtnl_link *, uint32_t);
> >> diff --git a/lib/route/link.c b/lib/route/link.c
> >> index 9979b5b..9b2b561 100644
> >> --- a/lib/route/link.c
> >> +++ b/lib/route/link.c
> >> @@ -1651,11 +1651,28 @@ void rtnl_link_put(struct rtnl_link *link)
> >>   * @route_doc{link_attr_name, Link Name}
> >>   * @see rtnl_link_get_name()
> >>   * @see rtnl_link_set_ifindex()
> >> + *
> >> + *@return -1 when the length of the name is equal or greater than IFNAMSIZ.
> >> + *      In this case,the length of the name has been truncated to 15 bytes.
> >> + *@return 0 normally
> >>   */
> >> -void rtnl_link_set_name(struct rtnl_link *link, const char *name)
> >> +int rtnl_link_set_name(struct rtnl_link *link, const char *name)
> >>  {
> >> +     int err = 0;
> >> +     if (!name)
> >> +             goto errout;
> >> +     else if (strlen(name) == 0)
> >> +             goto errout;
> >> +     else if (IFNAMSIZ > strlen(name))
> >> +             err = 0;
> >> +     else
> >> +             err = -1;
> >> +
> >>       strncpy(link->l_name, name, sizeof(link->l_name) - 1);
> >>       link->ce_mask |= LINK_ATTR_IFNAME;
> >> +
> >> +     errout:
> >> +             return err;
> >>  }
> >>
> >>  /**
> >

-------------- 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/20140122/13453d76/attachment.sig>