[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20200225074019.GB17869@nanopsycho>
Date: Tue, 25 Feb 2020 08:40:19 +0100
From: Jiri Pirko <jiri@...nulli.us>
To: Jakub Kicinski <kuba@...nel.org>
Cc: netdev@...r.kernel.org, davem@...emloft.net, nhorman@...driver.com,
jhs@...atatu.com, xiyou.wangcong@...il.com, idosch@...lanox.com,
mlxsw@...lanox.com
Subject: Re: [patch net-next 09/10] netdevsim: add ACL trap reporting cookie
as a metadata
Tue, Feb 25, 2020 at 05:42:57AM CET, kuba@...nel.org wrote:
>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?
Correct. Changed to return -EINVAL;
>
>> + cookie_len = (count - 1) / 2;
>
>why was cookie_len initialized when it was declared?
Forgotten init. Fixed.
>
>> + if ((count - 1) % 2)
>> + return -EINVAL;
>> + buf = kmalloc(count, GFP_KERNEL);
>
>Strangely the malloc below has a NOWARN, but this one doesn't?
Added nowarn flag here too.
>
>> + 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 :)
Removed.
>
>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..
I don't understand. Do you suggest to keep the write hex "buf" as well
and just print it out in "read()" function? I don't like to store one
info in 2 places. We need to have the cookie in fa_cookie anyway. Easy
to bin2hex from it and send to userspace.
>
>> + 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.
Well, it's a matter of a code you look at. I actually like it better
with err_*. Anyway, netdevsim uses the convention you want, changed.
Thanks for review!
>
>> + kfree(buf);
>> + return ret;
>> +}
Powered by blists - more mailing lists