| Message ID | 20221123203834.738606-3-jacob.e.keller@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | support direct read from region | expand |
Wed, Nov 23, 2022 at 09:38:27PM CET, jacob.e.keller@intel.com wrote: >Report extended error details in the devlink_nl_cmd_region_read_dumpit Nit: It is customary to use "()"s when mentioning the function name in patch description. w >function, by using the extack structure from the netlink_callback. > >Signed-off-by: Jacob Keller <jacob.e.keller@intel.com> Reviewed-by: Jiri Pirko <jiri@nvidia.com>
Wed, Nov 23, 2022 at 09:38:27PM CET, jacob.e.keller@intel.com wrote: [...] >@@ -6525,8 +6525,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(cb->extack, "No region name provided"); >+ err = -EINVAL; >+ goto out_unlock; >+ } >+ >+ if (!attrs[DEVLINK_ATTR_REGION_SNAPSHOT_ID]) { >+ NL_SET_ERR_MSG(cb->extack, "No snapshot id provided"); > err = -EINVAL; > goto out_unlock; > } >@@ -6541,7 +6547,8 @@ static int devlink_nl_cmd_region_read_dumpit(struct sk_buff *skb, > } > } > >- region_name = nla_data(attrs[DEVLINK_ATTR_REGION_NAME]); >+ region_attr = attrs[DEVLINK_ATTR_REGION_NAME]; >+ region_name = nla_data(region_attr); > > if (port) > region = devlink_port_region_get_by_name(port, region_name); >@@ -6549,6 +6556,7 @@ 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_ATTR(cb->extack, region_attr, "requested region does not exist"); Any reason why don't start the message with uppercase? It would be consistent with the other 2 messages you just introduced. Same goes to the message in the next patch (perhaps some others too) > err = -EINVAL; > goto out_unlock; > } >-- >2.38.1.420.g319605f8f00e >
> -----Original Message----- > From: Jiri Pirko <jiri@resnulli.us> > Sent: Thursday, November 24, 2022 12:47 AM > To: Keller, Jacob E <jacob.e.keller@intel.com> > Cc: netdev@vger.kernel.org; Jiri Pirko <jiri@nvidia.com>; Jakub Kicinski > <kuba@kernel.org> > Subject: Re: [PATCH net-next v2 2/9] devlink: report extended error message in > region_read_dumpit > > Wed, Nov 23, 2022 at 09:38:27PM CET, jacob.e.keller@intel.com wrote: > > [...] > > > >@@ -6525,8 +6525,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(cb->extack, "No region name provided"); > >+ err = -EINVAL; > >+ goto out_unlock; > >+ } > >+ > >+ if (!attrs[DEVLINK_ATTR_REGION_SNAPSHOT_ID]) { > >+ NL_SET_ERR_MSG(cb->extack, "No snapshot id provided"); > > err = -EINVAL; > > goto out_unlock; > > } > >@@ -6541,7 +6547,8 @@ static int devlink_nl_cmd_region_read_dumpit(struct > sk_buff *skb, > > } > > } > > > >- region_name = nla_data(attrs[DEVLINK_ATTR_REGION_NAME]); > >+ region_attr = attrs[DEVLINK_ATTR_REGION_NAME]; > >+ region_name = nla_data(region_attr); > > > > if (port) > > region = devlink_port_region_get_by_name(port, region_name); > >@@ -6549,6 +6556,7 @@ 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_ATTR(cb->extack, region_attr, "requested > region does not exist"); > > Any reason why don't start the message with uppercase? It would be > consistent with the other 2 messages you just introduced. > Same goes to the message in the next patch (perhaps some others too) > > No particular reason. I'll fix these. > > err = -EINVAL; > > goto out_unlock; > > } > >-- > >2.38.1.420.g319605f8f00e > >
diff --git a/net/core/devlink.c b/net/core/devlink.c index a490b8850179..b5b317661f9a 100644 --- a/net/core/devlink.c +++ b/net/core/devlink.c @@ -6507,10 +6507,10 @@ static int devlink_nl_cmd_region_read_dumpit(struct sk_buff *skb, { const struct genl_dumpit_info *info = genl_dumpit_info(cb); u64 ret_offset, start_offset, end_offset = U64_MAX; + struct nlattr *chunks_attr, *region_attr; struct nlattr **attrs = info->attrs; struct devlink_port *port = NULL; struct devlink_region *region; - struct nlattr *chunks_attr; const char *region_name; struct devlink *devlink; unsigned int index; @@ -6525,8 +6525,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(cb->extack, "No region name provided"); + err = -EINVAL; + goto out_unlock; + } + + if (!attrs[DEVLINK_ATTR_REGION_SNAPSHOT_ID]) { + NL_SET_ERR_MSG(cb->extack, "No snapshot id provided"); err = -EINVAL; goto out_unlock; } @@ -6541,7 +6547,8 @@ static int devlink_nl_cmd_region_read_dumpit(struct sk_buff *skb, } } - region_name = nla_data(attrs[DEVLINK_ATTR_REGION_NAME]); + region_attr = attrs[DEVLINK_ATTR_REGION_NAME]; + region_name = nla_data(region_attr); if (port) region = devlink_port_region_get_by_name(port, region_name); @@ -6549,6 +6556,7 @@ 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_ATTR(cb->extack, region_attr, "requested region 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> --- Changes since v1: * Moved to 2/9 of the series * No longer includes change to snapshot, as that is now handled in 3/9 * use local region_attr variable and NL_SET_ERR_MSG_ATTR net/core/devlink.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-)