In some functions, it's unnecessary to declare 'err'
and 'ret' variables at the same time. This patch mainly
to simplify the issue of such declarations by reusing
one variable.
Signed-off-by: Rongwei Wang <[email protected]>
---
mm/damon/dbgfs.c | 67 ++++++++++++++++++++++--------------------------
1 file changed, 31 insertions(+), 36 deletions(-)
diff --git a/mm/damon/dbgfs.c b/mm/damon/dbgfs.c
index 28d6abf27763..d8dd68f72827 100644
--- a/mm/damon/dbgfs.c
+++ b/mm/damon/dbgfs.c
@@ -69,8 +69,7 @@ static ssize_t dbgfs_attrs_write(struct file *file,
struct damon_ctx *ctx = file->private_data;
unsigned long s, a, r, minr, maxr;
char *kbuf;
- ssize_t ret = count;
- int err;
+ ssize_t ret;
kbuf = user_input_str(buf, count, ppos);
if (IS_ERR(kbuf))
@@ -88,9 +87,9 @@ static ssize_t dbgfs_attrs_write(struct file *file,
goto unlock_out;
}
- err = damon_set_attrs(ctx, s, a, r, minr, maxr);
- if (err)
- ret = err;
+ ret = damon_set_attrs(ctx, s, a, r, minr, maxr);
+ if (!ret)
+ ret = count;
unlock_out:
mutex_unlock(&ctx->kdamond_lock);
out:
@@ -220,14 +219,13 @@ static ssize_t dbgfs_schemes_write(struct file *file, const char __user *buf,
struct damon_ctx *ctx = file->private_data;
char *kbuf;
struct damos **schemes;
- ssize_t nr_schemes = 0, ret = count;
- int err;
+ ssize_t nr_schemes = 0, ret;
kbuf = user_input_str(buf, count, ppos);
if (IS_ERR(kbuf))
return PTR_ERR(kbuf);
- schemes = str_to_schemes(kbuf, ret, &nr_schemes);
+ schemes = str_to_schemes(kbuf, count, &nr_schemes);
if (!schemes) {
ret = -EINVAL;
goto out;
@@ -239,11 +237,12 @@ static ssize_t dbgfs_schemes_write(struct file *file, const char __user *buf,
goto unlock_out;
}
- err = damon_set_schemes(ctx, schemes, nr_schemes);
- if (err)
- ret = err;
- else
+ ret = damon_set_schemes(ctx, schemes, nr_schemes);
+ if (!ret) {
+ ret = count;
nr_schemes = 0;
+ }
+
unlock_out:
mutex_unlock(&ctx->kdamond_lock);
free_schemes_arr(schemes, nr_schemes);
@@ -342,9 +341,8 @@ static ssize_t dbgfs_target_ids_write(struct file *file,
char *kbuf, *nrs;
unsigned long *targets;
ssize_t nr_targets;
- ssize_t ret = count;
+ ssize_t ret;
int i;
- int err;
kbuf = user_input_str(buf, count, ppos);
if (IS_ERR(kbuf))
@@ -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) {
if (targetid_is_pid(ctx))
dbgfs_put_pids(targets, nr_targets);
- ret = err;
- }
+ } else
+ ret = count;
unlock_out:
mutex_unlock(&ctx->kdamond_lock);
@@ -548,8 +546,7 @@ static ssize_t dbgfs_mk_context_write(struct file *file,
{
char *kbuf;
char *ctx_name;
- ssize_t ret = count;
- int err;
+ ssize_t ret;
kbuf = user_input_str(buf, count, ppos);
if (IS_ERR(kbuf))
@@ -567,9 +564,9 @@ static ssize_t dbgfs_mk_context_write(struct file *file,
}
mutex_lock(&damon_dbgfs_lock);
- err = dbgfs_mk_context(ctx_name);
- if (err)
- ret = err;
+ ret = dbgfs_mk_context(ctx_name);
+ if (!ret)
+ ret = count;
mutex_unlock(&damon_dbgfs_lock);
out:
@@ -638,8 +635,7 @@ static ssize_t dbgfs_rm_context_write(struct file *file,
const char __user *buf, size_t count, loff_t *ppos)
{
char *kbuf;
- ssize_t ret = count;
- int err;
+ ssize_t ret;
char *ctx_name;
kbuf = user_input_str(buf, count, ppos);
@@ -658,9 +654,9 @@ static ssize_t dbgfs_rm_context_write(struct file *file,
}
mutex_lock(&damon_dbgfs_lock);
- err = dbgfs_rm_context(ctx_name);
- if (err)
- ret = err;
+ ret = dbgfs_rm_context(ctx_name);
+ if (!ret)
+ ret = count;
mutex_unlock(&damon_dbgfs_lock);
out:
@@ -684,9 +680,8 @@ static ssize_t dbgfs_monitor_on_read(struct file *file,
static ssize_t dbgfs_monitor_on_write(struct file *file,
const char __user *buf, size_t count, loff_t *ppos)
{
- ssize_t ret = count;
+ ssize_t ret;
char *kbuf;
- int err;
kbuf = user_input_str(buf, count, ppos);
if (IS_ERR(kbuf))
@@ -699,14 +694,14 @@ static ssize_t dbgfs_monitor_on_write(struct file *file,
}
if (!strncmp(kbuf, "on", count))
- err = damon_start(dbgfs_ctxs, dbgfs_nr_ctxs);
+ ret = damon_start(dbgfs_ctxs, dbgfs_nr_ctxs);
else if (!strncmp(kbuf, "off", count))
- err = damon_stop(dbgfs_ctxs, dbgfs_nr_ctxs);
+ ret = damon_stop(dbgfs_ctxs, dbgfs_nr_ctxs);
else
- err = -EINVAL;
+ ret = -EINVAL;
- if (err)
- ret = err;
+ if (!ret)
+ ret = count;
kfree(kbuf);
return ret;
}
--
2.27.0
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 <[email protected]> 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.
> 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
>
> unlock_out:
> mutex_unlock(&ctx->kdamond_lock);
> @@ -548,8 +546,7 @@ static ssize_t dbgfs_mk_context_write(struct file *file,
[...]
Thanks,
SJ
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 <[email protected]> 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
>