Re: (sashiko review) [RFC PATCH v5 07/10] selftests/damon/_damon_sysfs: support pause file staging

[SeongJae Park]

On Mon, 23 Mar 2026 18:28:24 -0700 SeongJae Park <sj@xxxxxxxxxx> wrote:

> Forwarding Sashiko review for doing discussions via mails.
> 
> # review url: https://sashiko.dev/#/patchset/20260323231538.84452-8-sj@xxxxxxxxxx
> # start of sashiko.dev inline review
> commit d9ca6c4a33fc4d613982730edda6f4b67ae30a67
> Author: SeongJae Park <sj@xxxxxxxxxx>
> selftests/damon/_damon_sysfs: support pause file staging
> 
> This commit adds support for the newly added pause file to the DAMON
> test-purpose sysfs interface control Python module.
> 
> > diff --git a/tools/testing/selftests/damon/_damon_sysfs.py b/tools/testing/selftests/damon/_damon_sysfs.py
> > --- a/tools/testing/selftests/damon/_damon_sysfs.py
> > +++ b/tools/testing/selftests/damon/_damon_sysfs.py
> [ ... ]
> >  class DamonCtx:
> >      targets = None
> >      schemes = None
> >      kdamond = None
> > +    pause = None
> >      idx = None
> >  
> >      def __init__(self, ops='paddr', monitoring_attrs=DamonAttrs(), targets=[],
> > -            schemes=[]):
> > +            schemes=[], pause=False):
> 
> This isn't a bug introduced by this patch, but since the function signature
> is being modified here, is it a good time to address the mutable default
> arguments?
> 
> In Python, default arguments are evaluated exactly once at function
> definition time. This means any instances of DamonCtx created without
> explicitly providing targets, schemes, or monitoring_attrs will share
> the same list structures and DamonAttrs instance in memory.
> 
> For example, self.monitoring_attrs.context = self overwrites the context
> attribute of the shared DamonAttrs object for all default contexts.
> 
> Would it be better to change these defaults to None and instantiate them
> within the method body to avoid unexpected state sharing across instances?

I agree changing the defaults to None is a good idea.  But that would better to
be another patch, not this one.


Thanks,
SJ

[...]