lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date:   Fri, 28 Oct 2022 11:35:58 +0200
From:   "Wilczynski, Michal" <michal.wilczynski@...el.com>
To:     Przemek Kitszel <przemyslaw.kitszel@...el.com>,
        <netdev@...r.kernel.org>
CC:     <alexandr.lobakin@...el.com>, <jacob.e.keller@...el.com>,
        <jesse.brandeburg@...el.com>, <anthony.l.nguyen@...el.com>,
        <kuba@...nel.org>, <ecree.xilinx@...il.com>, <jiri@...nulli.us>
Subject: Re: [PATCH net-next v7 4/9] devlink: Allow for devlink-rate nodes
 parent reassignment



On 10/28/2022 12:34 AM, Przemek Kitszel wrote:
> From:   Michal Wilczynski <michal.wilczynski@...el.com>
> Date:   Thu, 27 Oct 2022 15:00:44 +0200
>
> [...]
>
>> diff --git a/net/core/devlink.c b/net/core/devlink.c
>> index 864fa0967b7a..1e0c1b0376bf 100644
>> --- a/net/core/devlink.c
>> +++ b/net/core/devlink.c
>> @@ -1875,10 +1875,9 @@ devlink_nl_rate_parent_node_set(struct devlink_rate *devlink_rate,
>>   	int err = -EOPNOTSUPP;
>>   
>>   	parent = devlink_rate->parent;
>> -	if (parent && len) {
>> -		NL_SET_ERR_MSG_MOD(info->extack, "Rate object already has parent.");
>> -		return -EBUSY;
>> -	} else if (parent && !len) {
>> +
>> +	/* if a parent is already set, just reassign the parent */
>> +	if (parent && !len) {
> Comment that you have added should be placed way below, here it is misleading.

In my mind it made sense, cause we're handling an additional case here that
wasn't handled before. I think I'll just remove the comment then if you
find it misleading.

>
>>   		if (devlink_rate_is_leaf(devlink_rate))
>>   			err = ops->rate_leaf_parent_set(devlink_rate, NULL,
>>   							devlink_rate->priv, NULL,
>> @@ -1892,7 +1891,7 @@ devlink_nl_rate_parent_node_set(struct devlink_rate *devlink_rate,
>>   
>>   		refcount_dec(&parent->refcnt);
>>   		devlink_rate->parent = NULL;
>> -	} else if (!parent && len) {
>> +	} else if (len) {
>>   		parent = devlink_rate_node_get_by_name(devlink, parent_name);
>>   		if (IS_ERR(parent))
>>   			return -ENODEV;
>> @@ -1919,6 +1918,10 @@ devlink_nl_rate_parent_node_set(struct devlink_rate *devlink_rate,
>>   		if (err)
>>   			return err;
>>   
> Comment above makes more sense here, likely combined with the one just below.

They would duplicate in my mind, so I'll just remove the upper one.

>
>> +		if (devlink_rate->parent)
>> +			/* we're reassigning to other parent in this case */
>> +			refcount_dec(&devlink_rate->parent->refcnt);
>> +
>>   		refcount_inc(&parent->refcnt);
>>   		devlink_rate->parent = parent;
>>   	}
>> --
> Thanks for splitting this patch out of the other, change itself is easier
> to follow now. Code (modulo comments) is correct, you could add my Reviewed-by
> after comment fix.
>
> Side note: ops (rate_{leaf|node}_parent_set) lack documentation. There is also
> not much usage of them as of now, so maybe we could extend them to actually do
> refcount_inc + refcount_dec (if applicable) + set pointers.

I'm not sure I follow you there, those are function pointers, and are 
supposed
to be implemented in the driver.

> OTOH: As of now those are more of "on-event" callbacks, not "do-something",
> what is further confirmed in name (word "set" on the end, not begining).
>
> --PK

Powered by blists - more mailing lists