Re: [PATCH net-next v2 3/9] devlink: find snapshot in devlink_nl_cmd_region_read_dumpit

[Jiri Pirko <jiri@resnulli.us>]

Wed, Nov 23, 2022 at 09:38:28PM CET, jacob.e.keller@intel.com wrote:
>The snapshot pointer is obtained inside of the function
>devlink_nl_region_read_snapshot_fill. Simplify this function by locating
>the snapshot upfront in devlink_nl_cmd_region_read_dumpit instead. This
>aligns with how other netlink attributes are handled, and allows us to
>exit slightly earlier if an invalid snapshot ID is provided.
>
>It also allows us to pass the snapshot pointer directly to the
>devlink_nl_region_read_snapshot_fill, and remove the now unused attrs
>parameter.
>
>Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>

With the msg uppercase start nit below.
Reviewed-by: Jiri Pirko <jiri@nvidia.com>



>---
>Changes since v1
>* Moved to 3/9 of series
>* Use snapshot_attr and NL_SET_ERR_MSG_ATTR to report extended error
>
> net/core/devlink.c | 25 ++++++++++++++-----------
> 1 file changed, 14 insertions(+), 11 deletions(-)
>
>diff --git a/net/core/devlink.c b/net/core/devlink.c
>index b5b317661f9a..825c52a71df1 100644
>--- a/net/core/devlink.c
>+++ b/net/core/devlink.c
>@@ -6463,24 +6463,16 @@ static int devlink_nl_cmd_region_read_chunk_fill(struct sk_buff *msg,
> 
> static int devlink_nl_region_read_snapshot_fill(struct sk_buff *skb,
> 						struct devlink *devlink,
>-						struct devlink_region *region,
>-						struct nlattr **attrs,
>+						struct devlink_snapshot *snapshot,
> 						u64 start_offset,
> 						u64 end_offset,
> 						u64 *new_offset)
> {
>-	struct devlink_snapshot *snapshot;
> 	u64 curr_offset = start_offset;
>-	u32 snapshot_id;
> 	int err = 0;
> 
> 	*new_offset = start_offset;
> 
>-	snapshot_id = nla_get_u32(attrs[DEVLINK_ATTR_REGION_SNAPSHOT_ID]);
>-	snapshot = devlink_region_snapshot_get_by_id(region, snapshot_id);
>-	if (!snapshot)
>-		return -EINVAL;
>-
> 	while (curr_offset < end_offset) {
> 		u32 data_size;
> 		u8 *data;
>@@ -6506,14 +6498,16 @@ static int devlink_nl_cmd_region_read_dumpit(struct sk_buff *skb,
> 					     struct netlink_callback *cb)
> {
> 	const struct genl_dumpit_info *info = genl_dumpit_info(cb);
>+	struct nlattr *chunks_attr, *region_attr, *snapshot_attr;
> 	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_snapshot *snapshot;
> 	struct devlink_region *region;
> 	const char *region_name;
> 	struct devlink *devlink;
> 	unsigned int index;
>+	u32 snapshot_id;
> 	void *hdr;
> 	int err;
> 
>@@ -6561,6 +6555,15 @@ static int devlink_nl_cmd_region_read_dumpit(struct sk_buff *skb,
> 		goto out_unlock;
> 	}
> 
>+	snapshot_attr = attrs[DEVLINK_ATTR_REGION_SNAPSHOT_ID];
>+	snapshot_id = nla_get_u32(snapshot_attr);
>+	snapshot = devlink_region_snapshot_get_by_id(region, snapshot_id);
>+	if (!snapshot) {
>+		NL_SET_ERR_MSG_ATTR(cb->extack, snapshot_attr, "requested snapshot does not exist");

Why not to start with "R"?


>+		err = -EINVAL;
>+		goto out_unlock;
>+	}
>+
> 	if (attrs[DEVLINK_ATTR_REGION_CHUNK_ADDR] &&
> 	    attrs[DEVLINK_ATTR_REGION_CHUNK_LEN]) {
> 		if (!start_offset)
>@@ -6610,7 +6613,7 @@ static int devlink_nl_cmd_region_read_dumpit(struct sk_buff *skb,
> 	}
> 
> 	err = devlink_nl_region_read_snapshot_fill(skb, devlink,
>-						   region, attrs,
>+						   snapshot,
> 						   start_offset,
> 						   end_offset, &ret_offset);
> 
>-- 
>2.38.1.420.g319605f8f00e
>