Re: [patch net-next 09/10] netdevsim: add ACL trap reporting cookie as a metadata

[Jiri Pirko <jiri@resnulli.us>]

Tue, Feb 25, 2020 at 05:42:57AM CET, kuba@kernel.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;
>> +}