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

[Rongwei Wang <rongwei.wang@xxxxxxxxxxxxxxxxx>]

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@xxxxxxxxxxxxxxxxx> 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