From: Jakub Kicinski <kuba@kernel.org>
To: Jiri Pirko <jiri@resnulli.us>
Cc: netdev@vger.kernel.org, davem@davemloft.net,
nhorman@tuxdriver.com, jhs@mojatatu.com,
xiyou.wangcong@gmail.com, idosch@mellanox.com,
mlxsw@mellanox.com
Subject: Re: [patch net-next 09/10] netdevsim: add ACL trap reporting cookie as a metadata
Date: Mon, 24 Feb 2020 20:42:57 -0800 [thread overview]
Message-ID: <20200224204257.07c7456f@cakuba.hsd1.ca.comcast.net> (raw)
In-Reply-To: <20200224210758.18481-10-jiri@resnulli.us>
On Mon, 24 Feb 2020 22:07:57 +0100, Jiri Pirko wrote:
> +static ssize_t nsim_dev_trap_fa_cookie_write(struct file *file,
> + const char __user *data,
> + size_t count, loff_t *ppos)
> +{
> + struct nsim_dev *nsim_dev = file->private_data;
> + struct flow_action_cookie *fa_cookie;
> + size_t cookie_len = count / 2;
> + ssize_t ret;
> + char *buf;
> +
> + if (*ppos != 0)
> + return 0;
return 0? Should this not be an error?
> + cookie_len = (count - 1) / 2;
why was cookie_len initialized when it was declared?
> + if ((count - 1) % 2)
> + return -EINVAL;
> + buf = kmalloc(count, GFP_KERNEL);
Strangely the malloc below has a NOWARN, but this one doesn't?
> + if (!buf)
> + return -ENOMEM;
> +
> + ret = simple_write_to_buffer(buf, count, ppos, data, count);
> + if (ret < 0)
> + goto err_write_to_buffer;
> +
> + fa_cookie = kmalloc(sizeof(*fa_cookie) + cookie_len,
> + GFP_KERNEL | __GFP_NOWARN);
> + if (!fa_cookie) {
> + ret = -ENOMEM;
> + goto err_alloc_cookie;
> + }
> +
> + fa_cookie->cookie_len = cookie_len;
> + ret = hex2bin((u8 *) fa_cookie->cookie, buf, cookie_len);
this u8 cast won't be necessary if type of cookie changes :)
Also I feel like we could just hold onto the ASCII hex buf,
and simplify the reading side. If the hex part is needed in
the first place, hexdump and xxd exist..
> + if (ret)
> + goto err_hex2bin;
> + kfree(buf);
> +
> + spin_lock(&nsim_dev->fa_cookie_lock);
> + kfree(nsim_dev->fa_cookie);
> + nsim_dev->fa_cookie = fa_cookie;
> + spin_unlock(&nsim_dev->fa_cookie_lock);
> +
> + return count;
> +
> +err_hex2bin:
> + kfree(fa_cookie);
> +err_alloc_cookie:
> +err_write_to_buffer:
Error labels should be named after what they undo, not after
destination. That makes both the source and target of the jump
easy to review.
> + kfree(buf);
> + return ret;
> +}
next prev parent reply other threads:[~2020-02-25 4:43 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-02-24 21:07 [patch net-next 00/10] mlxsw: Implement ACL-dropped packets identification Jiri Pirko
2020-02-24 21:07 ` [patch net-next 01/10] flow_offload: pass action cookie through offload structures Jiri Pirko
2020-02-25 4:16 ` Jakub Kicinski
2020-02-24 21:07 ` [patch net-next 02/10] devlink: add trap metadata type for cookie Jiri Pirko
2020-02-25 4:19 ` Jakub Kicinski
2020-02-24 21:07 ` [patch net-next 03/10] drop_monitor: extend by passing cookie from driver Jiri Pirko
2020-02-25 4:19 ` Jakub Kicinski
2020-02-25 7:04 ` Jiri Pirko
2020-02-24 21:07 ` [patch net-next 04/10] devlink: extend devlink_trap_report() to accept cookie and pass Jiri Pirko
2020-02-25 4:20 ` Jakub Kicinski
2020-02-24 21:07 ` [patch net-next 05/10] mlxsw: core_acl_flex_actions: Add trap with userdef action Jiri Pirko
2020-02-24 21:07 ` [patch net-next 06/10] mlxsw: core_acl_flex_actions: Implement flow_offload action cookie offload Jiri Pirko
2020-02-24 21:07 ` [patch net-next 07/10] mlxsw: pci: Extract cookie index for ACL discard trap packets Jiri Pirko
2020-02-24 21:07 ` [patch net-next 08/10] mlxsw: spectrum_trap: Lookup and pass cookie down to devlink_trap_report() Jiri Pirko
2020-02-24 21:07 ` [patch net-next 09/10] netdevsim: add ACL trap reporting cookie as a metadata Jiri Pirko
2020-02-25 4:42 ` Jakub Kicinski [this message]
2020-02-25 7:40 ` Jiri Pirko
2020-02-25 17:53 ` Jakub Kicinski
2020-02-24 21:07 ` [patch net-next 10/10] selftests: netdevsim: Extend devlink trap test to include flow action cookie Jiri Pirko
2020-02-25 4:43 ` Jakub Kicinski
2020-02-25 7:46 ` Jiri Pirko
2020-02-25 17:57 ` Jakub Kicinski
2020-02-26 7:34 ` Jiri Pirko
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200224204257.07c7456f@cakuba.hsd1.ca.comcast.net \
--to=kuba@kernel.org \
--cc=davem@davemloft.net \
--cc=idosch@mellanox.com \
--cc=jhs@mojatatu.com \
--cc=jiri@resnulli.us \
--cc=mlxsw@mellanox.com \
--cc=netdev@vger.kernel.org \
--cc=nhorman@tuxdriver.com \
--cc=xiyou.wangcong@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).