Re: [PATCH v29 07/13] mm/damon: Implement a debugfs-based user space interface

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Thank you for the comments!

On Fri, 11 Jun 2021 20:59:04 +0200 <sieberf@xxxxxxxxxx> wrote:

> + Returns non-empty string on success, negarive error code otherwise.
> 
> Nit: typo, 'negarive'

Good catch!  I will fix this in the next spin.

> 
> 
> +    if (ctx->kdamond) {
> +        if (targetid_is_pid(ctx))
> +            dbgfs_put_pids(targets, nr_targets);
> +        ret = -EBUSY;
> +        goto unlock_out;
> 
> I don't understand this block, what does EBUSY mean in this context?

That means DAMON is busy because someone already requested running the
monitoring context.  So, the user will be able to wait a little bit and try
later, or try to figure out who requested the monitoring.

> 
> +    if (sscanf(kbuf, "%s", kbuf) != 1) {
> +        kfree(kbuf);
> +        return -EINVAL;
> +    }
> 
> Nit, this diverts from other patterns use elsewhere where error 
> conditions that need to free memory use a goto label.

I want to use 'goto' only if it makes the code shorter and easier to read.  In
this case, this is the only one case that could use the 'goto' statement.  And,
this part would still be two lines like below:

    if (sscanf(kbuf, "%s", kbuf) != 1) {
    	ret = -EINVAL;
    	goto out;
    }

Meanwhile, to use the 'goto', we would need to add the label below, so we will
only add one more line.  Therefore, I think using 'goto' here doesn't really
reduce the lines of code or make it easier to read.

So, if you don't care, I'd like to use current code as is.

If I missed something, please let me know.


Thanks,
SeongJae Park



[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux FS]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux