[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <Y38vqeX/3WRj7SxJ@nanopsycho>
Date: Thu, 24 Nov 2022 09:47:37 +0100
From: Jiri Pirko <jiri@...nulli.us>
To: Jacob Keller <jacob.e.keller@...el.com>
Cc: netdev@...r.kernel.org, Jiri Pirko <jiri@...dia.com>,
Jakub Kicinski <kuba@...nel.org>
Subject: Re: [PATCH net-next v2 3/9] devlink: find snapshot in
devlink_nl_cmd_region_read_dumpit
Wed, Nov 23, 2022 at 09:38:28PM CET, jacob.e.keller@...el.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@...el.com>
With the msg uppercase start nit below.
Reviewed-by: Jiri Pirko <jiri@...dia.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
>
Powered by blists - more mailing lists