| Message ID | 20221117220803.2773887-4-jacob.e.keller@intel.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | devlink: support direct read from region | expand |
On Thu, 17 Nov 2022 14:07:58 -0800 Jacob Keller wrote: > Report extended error details in the devlink_nl_cmd_region_read_dumpit > function, by using the extack structure from the netlink_callback. > > Signed-off-by: Jacob Keller <jacob.e.keller@intel.com> > --- > net/core/devlink.c | 14 ++++++++++++-- > 1 file changed, 12 insertions(+), 2 deletions(-) > > diff --git a/net/core/devlink.c b/net/core/devlink.c > index 932476956d7e..f2ee1da5283c 100644 > --- a/net/core/devlink.c > +++ b/net/core/devlink.c > @@ -6453,8 +6453,14 @@ static int devlink_nl_cmd_region_read_dumpit(struct sk_buff *skb, > > devl_lock(devlink); > > - if (!attrs[DEVLINK_ATTR_REGION_NAME] || > - !attrs[DEVLINK_ATTR_REGION_SNAPSHOT_ID]) { > + if (!attrs[DEVLINK_ATTR_REGION_NAME]) { > + NL_SET_ERR_MSG_MOD(cb->extack, "No region name provided"); > + err = -EINVAL; > + goto out_unlock; > + } > + > + if (!attrs[DEVLINK_ATTR_REGION_SNAPSHOT_ID]) { Please use GENL_REQ_ATTR_CHECK() instead of adding strings. > + NL_SET_ERR_MSG_MOD(cb->extack, "No snapshot id provided"); > err = -EINVAL; > goto out_unlock; > } > @@ -6477,6 +6483,8 @@ static int devlink_nl_cmd_region_read_dumpit(struct sk_buff *skb, > region = devlink_region_get_by_name(devlink, region_name); > > if (!region) { > + NL_SET_ERR_MSG_MOD(cb->extack, > + "The requested region does not exist"); NL_SET_ERR_MSG_ATTR() > err = -EINVAL; > goto out_unlock; > } > @@ -6484,6 +6492,8 @@ static int devlink_nl_cmd_region_read_dumpit(struct sk_buff *skb, > snapshot_id = nla_get_u32(attrs[DEVLINK_ATTR_REGION_SNAPSHOT_ID]); > snapshot = devlink_region_snapshot_get_by_id(region, snapshot_id); > if (!snapshot) { > + NL_SET_ERR_MSG_MOD(cb->extack, > + "The requested snapshot id does not exist"); ditto > err = -EINVAL; > goto out_unlock; > }
On 11/18/2022 5:40 PM, Jakub Kicinski wrote: > On Thu, 17 Nov 2022 14:07:58 -0800 Jacob Keller wrote: >> Report extended error details in the devlink_nl_cmd_region_read_dumpit >> function, by using the extack structure from the netlink_callback. >> >> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com> >> --- >> net/core/devlink.c | 14 ++++++++++++-- >> 1 file changed, 12 insertions(+), 2 deletions(-) >> >> diff --git a/net/core/devlink.c b/net/core/devlink.c >> index 932476956d7e..f2ee1da5283c 100644 >> --- a/net/core/devlink.c >> +++ b/net/core/devlink.c >> @@ -6453,8 +6453,14 @@ static int devlink_nl_cmd_region_read_dumpit(struct sk_buff *skb, >> >> devl_lock(devlink); >> >> - if (!attrs[DEVLINK_ATTR_REGION_NAME] || >> - !attrs[DEVLINK_ATTR_REGION_SNAPSHOT_ID]) { >> + if (!attrs[DEVLINK_ATTR_REGION_NAME]) { >> + NL_SET_ERR_MSG_MOD(cb->extack, "No region name provided"); >> + err = -EINVAL; >> + goto out_unlock; >> + } >> + >> + if (!attrs[DEVLINK_ATTR_REGION_SNAPSHOT_ID]) { > > Please use GENL_REQ_ATTR_CHECK() instead of adding strings. > Wasn't aware of this, nice! >> + NL_SET_ERR_MSG_MOD(cb->extack, "No snapshot id provided"); >> err = -EINVAL; >> goto out_unlock; >> } >> @@ -6477,6 +6483,8 @@ static int devlink_nl_cmd_region_read_dumpit(struct sk_buff *skb, >> region = devlink_region_get_by_name(devlink, region_name); >> >> if (!region) { >> + NL_SET_ERR_MSG_MOD(cb->extack, >> + "The requested region does not exist"); > > NL_SET_ERR_MSG_ATTR() Yep, should have noticed that myself. Thanks. > >> err = -EINVAL; >> goto out_unlock; >> } >> @@ -6484,6 +6492,8 @@ static int devlink_nl_cmd_region_read_dumpit(struct sk_buff *skb, >> snapshot_id = nla_get_u32(attrs[DEVLINK_ATTR_REGION_SNAPSHOT_ID]); >> snapshot = devlink_region_snapshot_get_by_id(region, snapshot_id); >> if (!snapshot) { >> + NL_SET_ERR_MSG_MOD(cb->extack, >> + "The requested snapshot id does not exist"); > > ditto > >> err = -EINVAL; >> goto out_unlock; >> } >
On 11/18/2022 5:40 PM, Jakub Kicinski wrote: > On Thu, 17 Nov 2022 14:07:58 -0800 Jacob Keller wrote: >> Report extended error details in the devlink_nl_cmd_region_read_dumpit >> function, by using the extack structure from the netlink_callback. >> >> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com> >> --- >> net/core/devlink.c | 14 ++++++++++++-- >> 1 file changed, 12 insertions(+), 2 deletions(-) >> >> diff --git a/net/core/devlink.c b/net/core/devlink.c >> index 932476956d7e..f2ee1da5283c 100644 >> --- a/net/core/devlink.c >> +++ b/net/core/devlink.c >> @@ -6453,8 +6453,14 @@ static int devlink_nl_cmd_region_read_dumpit(struct sk_buff *skb, >> >> devl_lock(devlink); >> >> - if (!attrs[DEVLINK_ATTR_REGION_NAME] || >> - !attrs[DEVLINK_ATTR_REGION_SNAPSHOT_ID]) { >> + if (!attrs[DEVLINK_ATTR_REGION_NAME]) { >> + NL_SET_ERR_MSG_MOD(cb->extack, "No region name provided"); >> + err = -EINVAL; >> + goto out_unlock; >> + } >> + >> + if (!attrs[DEVLINK_ATTR_REGION_SNAPSHOT_ID]) { > > Please use GENL_REQ_ATTR_CHECK() instead of adding strings. > Ahhh. Figured out why GENL_REQ_ATTR_CHECK wasn't used here already. It happens because the dumpit functions don't get a genl_info * struct, they get a netlink_cb and a genl_dumpit_info. I can look at improving this. Thanks, Jake
On Mon, 21 Nov 2022 11:10:37 -0800 Jacob Keller wrote: > >> @@ -6453,8 +6453,14 @@ static int devlink_nl_cmd_region_read_dumpit(struct sk_buff *skb, > >> > >> devl_lock(devlink); > >> > >> - if (!attrs[DEVLINK_ATTR_REGION_NAME] || > >> - !attrs[DEVLINK_ATTR_REGION_SNAPSHOT_ID]) { > >> + if (!attrs[DEVLINK_ATTR_REGION_NAME]) { > >> + NL_SET_ERR_MSG_MOD(cb->extack, "No region name provided"); > >> + err = -EINVAL; > >> + goto out_unlock; > >> + } > >> + > >> + if (!attrs[DEVLINK_ATTR_REGION_SNAPSHOT_ID]) { > > > > Please use GENL_REQ_ATTR_CHECK() instead of adding strings. > > > > Ahhh. Figured out why GENL_REQ_ATTR_CHECK wasn't used here already. It > happens because the dumpit functions don't get a genl_info * struct, > they get a netlink_cb and a genl_dumpit_info. > > I can look at improving this. Ah damn, you're right, I thought I just missed it because it wasn't at the top of the function.
On 11/21/2022 11:23 AM, Jakub Kicinski wrote: > On Mon, 21 Nov 2022 11:10:37 -0800 Jacob Keller wrote: >>>> @@ -6453,8 +6453,14 @@ static int devlink_nl_cmd_region_read_dumpit(struct sk_buff *skb, >>>> >>>> devl_lock(devlink); >>>> >>>> - if (!attrs[DEVLINK_ATTR_REGION_NAME] || >>>> - !attrs[DEVLINK_ATTR_REGION_SNAPSHOT_ID]) { >>>> + if (!attrs[DEVLINK_ATTR_REGION_NAME]) { >>>> + NL_SET_ERR_MSG_MOD(cb->extack, "No region name provided"); >>>> + err = -EINVAL; >>>> + goto out_unlock; >>>> + } >>>> + >>>> + if (!attrs[DEVLINK_ATTR_REGION_SNAPSHOT_ID]) { >>> >>> Please use GENL_REQ_ATTR_CHECK() instead of adding strings. >>> >> >> Ahhh. Figured out why GENL_REQ_ATTR_CHECK wasn't used here already. It >> happens because the dumpit functions don't get a genl_info * struct, >> they get a netlink_cb and a genl_dumpit_info. >> >> I can look at improving this. > > Ah damn, you're right, I thought I just missed it because it wasn't > at the top of the function. I also saw a few other cases where it might make sense to use a GENL_CB_REQ_ATTR_CHECK or similar. Unfortunately there's at least one area where we check for attributes inside a function that is used in both flows which would get a bit problematic :( Will see what I can come up with. Thanks, Jake
On Mon, 21 Nov 2022 13:18:37 -0800 Jacob Keller wrote: > > Ah damn, you're right, I thought I just missed it because it wasn't > > at the top of the function. > > I also saw a few other cases where it might make sense to use a > GENL_CB_REQ_ATTR_CHECK or similar. > > Unfortunately there's at least one area where we check for attributes > inside a function that is used in both flows which would get a bit > problematic :( Will see what I can come up with. Perhaps this series is not the right place to worry about the missing attr ext_ack for dumps. Go forward with v2, we can solve that later. I think the info discrepancy falls under a larger problem of message building code between doit and dump.
diff --git a/net/core/devlink.c b/net/core/devlink.c index 932476956d7e..f2ee1da5283c 100644 --- a/net/core/devlink.c +++ b/net/core/devlink.c @@ -6453,8 +6453,14 @@ static int devlink_nl_cmd_region_read_dumpit(struct sk_buff *skb, devl_lock(devlink); - if (!attrs[DEVLINK_ATTR_REGION_NAME] || - !attrs[DEVLINK_ATTR_REGION_SNAPSHOT_ID]) { + if (!attrs[DEVLINK_ATTR_REGION_NAME]) { + NL_SET_ERR_MSG_MOD(cb->extack, "No region name provided"); + err = -EINVAL; + goto out_unlock; + } + + if (!attrs[DEVLINK_ATTR_REGION_SNAPSHOT_ID]) { + NL_SET_ERR_MSG_MOD(cb->extack, "No snapshot id provided"); err = -EINVAL; goto out_unlock; } @@ -6477,6 +6483,8 @@ static int devlink_nl_cmd_region_read_dumpit(struct sk_buff *skb, region = devlink_region_get_by_name(devlink, region_name); if (!region) { + NL_SET_ERR_MSG_MOD(cb->extack, + "The requested region does not exist"); err = -EINVAL; goto out_unlock; } @@ -6484,6 +6492,8 @@ static int devlink_nl_cmd_region_read_dumpit(struct sk_buff *skb, snapshot_id = nla_get_u32(attrs[DEVLINK_ATTR_REGION_SNAPSHOT_ID]); snapshot = devlink_region_snapshot_get_by_id(region, snapshot_id); if (!snapshot) { + NL_SET_ERR_MSG_MOD(cb->extack, + "The requested snapshot id does not exist"); err = -EINVAL; goto out_unlock; }
Report extended error details in the devlink_nl_cmd_region_read_dumpit function, by using the extack structure from the netlink_callback. Signed-off-by: Jacob Keller <jacob.e.keller@intel.com> --- net/core/devlink.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)