linux-kernel - Re: [PATCH] mm/damon/dbgfs: remove unnecessary variables

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]

Message-ID: <b503abb8-90dc-fe38-909b-5a4156c170d9@linux.alibaba.com>
Date:   Thu, 14 Oct 2021 00:27:08 +0800
From:   Rongwei Wang <rongwei.wang@...ux.alibaba.com>
To:     SeongJae Park <sj@...nel.org>
Cc:     linux-kernel@...r.kernel.org, linux-mm@...ck.org,
        akpm@...ux-foundation.org
Subject: Re: [PATCH] mm/damon/dbgfs: remove unnecessary variables



On 10/13/21 11:04 PM, SeongJae Park wrote:
> Hi Rongwei,
> 
> 
> Thank you for this patch!  Looks good to me overall.  I left a couple of
> nitpicks below, though.
> 
> On Wed, 13 Oct 2021 21:09:01 +0800 Rongwei Wang <rongwei.wang@...ux.alibaba.com> wrote:
> 
> [...]
>> @@ -352,7 +350,7 @@ static ssize_t dbgfs_target_ids_write(struct file *file,
>>   
>>   	nrs = kbuf;
>>   
>> -	targets = str_to_target_ids(nrs, ret, &nr_targets);
>> +	targets = str_to_target_ids(nrs, count, &nr_targets);
>>   	if (!targets) {
>>   		ret = -ENOMEM;
>>   		goto out;
>> @@ -378,12 +376,12 @@ static ssize_t dbgfs_target_ids_write(struct file *file,
>>   		goto unlock_out;
>>   	}
>>   
>> -	err = damon_set_targets(ctx, targets, nr_targets);
>> -	if (err) {
>> +	ret = damon_set_targets(ctx, targets, nr_targets);
>> +	if (ret < 0) {
> 
> I'd prefer 'if (ret) {', to be consistent with other part.
Hi

In fact, explicit style, such as 'if (ret < 0)', may be more 
understandable. But I agree with your advice that to be consistent
with other part. And I will update it like your said.
> 
>>   		if (targetid_is_pid(ctx))
>>   			dbgfs_put_pids(targets, nr_targets);
>> -		ret = err;
>> -	}
>> +	} else
>> +		ret = count;
> 
> I'd prefer this to have braces:
> https://docs.kernel.org/process/coding-style.html#placing-braces-and-spaces
Agree! I will update here.

Thanks!
> 
>>   
>>   unlock_out:
>>   	mutex_unlock(&ctx->kdamond_lock);
>> @@ -548,8 +546,7 @@ static ssize_t dbgfs_mk_context_write(struct file *file,
> [...]
> 
> 
> Thanks,
> SJ
>