| Message ID | 1516195680-33683-1-git-send-email-arkadis@mellanox.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | stephen hemminger |
| Headers | show |
| Series | [iproute2] devlink: Ignore unknown attributes | expand |
Wed, Jan 17, 2018 at 02:28:00PM CET, arkadis@mellanox.com wrote: >In case of extending the UAPI old packages would break. > >Signed-off-by: Arkadi Sharshevsky <arkadis@mellanox.com> Acked-by: Jiri Pirko <jiri@mellanox.com>
On Wed, 17 Jan 2018 15:28:00 +0200 Arkadi Sharshevsky <arkadis@mellanox.com> wrote: > In case of extending the UAPI old packages would break. > > Signed-off-by: Arkadi Sharshevsky <arkadis@mellanox.com> Looks like good future proofing. Applied.
On 1/17/18 5:28 AM, Arkadi Sharshevsky wrote: > In case of extending the UAPI old packages would break. > > Signed-off-by: Arkadi Sharshevsky <arkadis@mellanox.com> > --- > devlink/devlink.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/devlink/devlink.c b/devlink/devlink.c > index 39cda06..c9d1838 100644 > --- a/devlink/devlink.c > +++ b/devlink/devlink.c > @@ -343,7 +343,7 @@ static int attr_cb(const struct nlattr *attr, void *data) > int type; > > if (mnl_attr_type_valid(attr, DEVLINK_ATTR_MAX) < 0) > - return MNL_CB_ERROR; > + return MNL_CB_OK; > > type = mnl_attr_get_type(attr); > if (mnl_attr_validate(attr, devlink_policy[type]) < 0) > What's the point of calling mnl_attr_type_valid if you disregard a failure? you might as well not call mnl_attr_type_valid at all.
On Thu, 18 Jan 2018 20:42:44 -0800 David Ahern <dsahern@gmail.com> wrote: > On 1/17/18 5:28 AM, Arkadi Sharshevsky wrote: > > In case of extending the UAPI old packages would break. > > > > Signed-off-by: Arkadi Sharshevsky <arkadis@mellanox.com> > > --- > > devlink/devlink.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/devlink/devlink.c b/devlink/devlink.c > > index 39cda06..c9d1838 100644 > > --- a/devlink/devlink.c > > +++ b/devlink/devlink.c > > @@ -343,7 +343,7 @@ static int attr_cb(const struct nlattr *attr, void *data) > > int type; > > > > if (mnl_attr_type_valid(attr, DEVLINK_ATTR_MAX) < 0) > > - return MNL_CB_ERROR; > > + return MNL_CB_OK; > > > > type = mnl_attr_get_type(attr); > > if (mnl_attr_validate(attr, devlink_policy[type]) < 0) > > > > What's the point of calling mnl_attr_type_valid if you disregard a > failure? you might as well not call mnl_attr_type_valid at all. The way mnl handles attributes, you have to have a callback and it is up to the callback to copy the values it wants. The idea is that old code running against a newer kernel will have a smaller array of attributes it wants, and only copy those.
On 1/19/18 2:02 PM, Stephen Hemminger wrote: > On Thu, 18 Jan 2018 20:42:44 -0800 > David Ahern <dsahern@gmail.com> wrote: > >> On 1/17/18 5:28 AM, Arkadi Sharshevsky wrote: >>> In case of extending the UAPI old packages would break. >>> >>> Signed-off-by: Arkadi Sharshevsky <arkadis@mellanox.com> >>> --- >>> devlink/devlink.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/devlink/devlink.c b/devlink/devlink.c >>> index 39cda06..c9d1838 100644 >>> --- a/devlink/devlink.c >>> +++ b/devlink/devlink.c >>> @@ -343,7 +343,7 @@ static int attr_cb(const struct nlattr *attr, void *data) >>> int type; >>> >>> if (mnl_attr_type_valid(attr, DEVLINK_ATTR_MAX) < 0) >>> - return MNL_CB_ERROR; >>> + return MNL_CB_OK; >>> >>> type = mnl_attr_get_type(attr); >>> if (mnl_attr_validate(attr, devlink_policy[type]) < 0) >>> >> >> What's the point of calling mnl_attr_type_valid if you disregard a >> failure? you might as well not call mnl_attr_type_valid at all. > > The way mnl handles attributes, you have to have a callback and it is up > to the callback to copy the values it wants. The idea is that old code > running against a newer kernel will have a smaller array of attributes > it wants, and only copy those. > mnl_attr_type_valid calls mnl_attr_get_type which does attr->nla_type & NLA_TYPE_MASK. Since you are no longer acknowledging the return code of mnl_attr_type_valid, you don't care about its checks so you might as well not call it. I don't see anything in libmnl that checks that mnl_attr_type_valid is invoked on an attr, so hence my question -- given the change above why call it all?
On Fri, 19 Jan 2018 14:27:06 -0800 David Ahern <dsahern@gmail.com> wrote: > On 1/19/18 2:02 PM, Stephen Hemminger wrote: > > On Thu, 18 Jan 2018 20:42:44 -0800 > > David Ahern <dsahern@gmail.com> wrote: > > > >> On 1/17/18 5:28 AM, Arkadi Sharshevsky wrote: > >>> In case of extending the UAPI old packages would break. > >>> > >>> Signed-off-by: Arkadi Sharshevsky <arkadis@mellanox.com> > >>> --- > >>> devlink/devlink.c | 2 +- > >>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>> > >>> diff --git a/devlink/devlink.c b/devlink/devlink.c > >>> index 39cda06..c9d1838 100644 > >>> --- a/devlink/devlink.c > >>> +++ b/devlink/devlink.c > >>> @@ -343,7 +343,7 @@ static int attr_cb(const struct nlattr *attr, void *data) > >>> int type; > >>> > >>> if (mnl_attr_type_valid(attr, DEVLINK_ATTR_MAX) < 0) > >>> - return MNL_CB_ERROR; > >>> + return MNL_CB_OK; > >>> > >>> type = mnl_attr_get_type(attr); > >>> if (mnl_attr_validate(attr, devlink_policy[type]) < 0) > >>> > >> > >> What's the point of calling mnl_attr_type_valid if you disregard a > >> failure? you might as well not call mnl_attr_type_valid at all. > > > > The way mnl handles attributes, you have to have a callback and it is up > > to the callback to copy the values it wants. The idea is that old code > > running against a newer kernel will have a smaller array of attributes > > it wants, and only copy those. > > > > mnl_attr_type_valid calls mnl_attr_get_type which does attr->nla_type & > NLA_TYPE_MASK. Since you are no longer acknowledging the return code of > mnl_attr_type_valid, you don't care about its checks so you might as > well not call it. I don't see anything in libmnl that checks that > mnl_attr_type_valid is invoked on an attr, so hence my question -- given > the change above why call it all? The part that matters is: static int attr_cb(const struct nlattr *attr, void *data) { const struct nlattr **tb = data; int type; if (mnl_attr_type_valid(attr, DEVLINK_ATTR_MAX) < 0) << makes sure that type < DEVLINK_ATTR_MAX return MNL_CB_OK; type = mnl_attr_get_type(attr); if (mnl_attr_validate(attr, devlink_policy[type]) < 0) << this part doesn't matter really return MNL_CB_ERROR; tb[type] = attr; << necessary so that tb[] is filled in. return MNL_CB_OK; }
On 1/19/18 3:40 PM, Stephen Hemminger wrote: >> mnl_attr_type_valid calls mnl_attr_get_type which does attr->nla_type & >> NLA_TYPE_MASK. Since you are no longer acknowledging the return code of >> mnl_attr_type_valid, you don't care about its checks so you might as >> well not call it. I don't see anything in libmnl that checks that >> mnl_attr_type_valid is invoked on an attr, so hence my question -- given >> the change above why call it all? ok. I see the error in my thinking. > The part that matters is: > > static int attr_cb(const struct nlattr *attr, void *data) > { > const struct nlattr **tb = data; > int type; > > if (mnl_attr_type_valid(attr, DEVLINK_ATTR_MAX) < 0) << makes sure that type < DEVLINK_ATTR_MAX > return MNL_CB_OK; > > type = mnl_attr_get_type(attr); > if (mnl_attr_validate(attr, devlink_policy[type]) < 0) << this part doesn't matter really > return MNL_CB_ERROR; > > tb[type] = attr; << necessary so that tb[] is filled in. > return MNL_CB_OK; > }
diff --git a/devlink/devlink.c b/devlink/devlink.c index 39cda06..c9d1838 100644 --- a/devlink/devlink.c +++ b/devlink/devlink.c @@ -343,7 +343,7 @@ static int attr_cb(const struct nlattr *attr, void *data) int type; if (mnl_attr_type_valid(attr, DEVLINK_ATTR_MAX) < 0) - return MNL_CB_ERROR; + return MNL_CB_OK; type = mnl_attr_get_type(attr); if (mnl_attr_validate(attr, devlink_policy[type]) < 0)
In case of extending the UAPI old packages would break. Signed-off-by: Arkadi Sharshevsky <arkadis@mellanox.com> --- devlink/devlink.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)