Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.35 as permitted sender) client-ip=23.128.96.35;
Date:   Mon, 2 Oct 2023 23:22:24 +0530
From:   swarup <swarupkotikalapudi@gmail.com>
To:     Alexey Dobriyan <adobriyan@gmail.com>
Cc:     Andrew Morton <akpm@linux-foundation.org>,
        linux-kernel@vger.kernel.org, shuah@kernel.org, hughd@google.com
Subject: Re: + selftests-proc-add-proc-pid-statm-output-validation.patch
 added to mm-nonmm-unstable branch
Message-ID: <ZRsDWFxy3rcILfxg@swarup-virtual-machine>
References: <20231001193740.B716AC433C7@smtp.kernel.org>
 <c1c55bdb-78cd-4b74-b9e5-2667dfdf9b2a@p183>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <c1c55bdb-78cd-4b74-b9e5-2667dfdf9b2a@p183>
Precedence: bulk

On Mon, Oct 02, 2023 at 03:38:25PM +0300, Alexey Dobriyan wrote:
> On Sun, Oct 01, 2023 at 12:37:40PM -0700, Andrew Morton wrote:
> >      selftests-proc-add-proc-pid-statm-output-validation.patch
> 
> > Add /proc/${pid}/statm validation
> > 
> > /proc/$(pid)/statm output is expected to be:
> >  "0 0 0 * 0 0 0\n"
> > Here * can be any value
> > 
> > Read output of /proc/$(pid)/statm
> > and compare length of output is
> > equal or greater than expected output
> 
> > --- a/tools/testing/selftests/proc/proc-empty-vm.c~selftests-proc-add-proc-pid-statm-output-validation
> > +++ a/tools/testing/selftests/proc/proc-empty-vm.c
> > @@ -303,6 +303,37 @@ static int test_proc_pid_smaps_rollup(pi
> >  	}
> >  }
> >  
> > +static const char g_statm[] = "0 0 0 * 0 0 0\n";
> 
> This is both unreliable and incorrect.
> 
> 4th value is "end_code - start_code" when exec is done which could be
> anything not 1-digit number (although unlikely).
> 
> Testing for strlen is simply too weak of a test.
> 
> > +static int test_proc_pid_statm(pid_t pid)
> > +{
> > +	char buf[4096];
> > +
> > +	snprintf(buf, sizeof(buf), "/proc/%u/statm", pid);
> > +
> > +	int fd = open(buf, O_RDONLY);
> > +
> > +	if (fd == -1) {
> > +		if (errno == ENOENT) {
> > +			/*
> > +			 * /proc/${pid}/statm is under CONFIG_PROC_PAGE_MONITOR,
> > +			 * it doesn't necessarily exist.
> > +			 */
> > +			return EXIT_SUCCESS;
> > +		}
> > +		perror("open /proc/${pid}/statm");
> > +		return EXIT_FAILURE;
> > +	} else {
> > +		ssize_t rv = read(fd, buf, sizeof(buf));
> > +
> > +		close(fd);
> > +		size_t len = strlen(g_statm);
> > +
> > +		assert(rv >= len);
> > +		return EXIT_SUCCESS;
> > +	}
> > +}
> > +
> >  int main(void)
> >  {
> >  	int rv = EXIT_SUCCESS;
> > @@ -389,11 +420,8 @@ int main(void)
> >  		if (rv == EXIT_SUCCESS) {
> >  			rv = test_proc_pid_smaps_rollup(pid);
> >  		}
> > -		/*
> > -		 * TODO test /proc/${pid}/statm, task_statm()
> > -		 * ->start_code, ->end_code aren't updated by munmap().
> > -		 * Output can be "0 0 0 2 0 0 0\n" where "2" can be anything.
> > -		 */
> > +		if (rv == EXIT_SUCCESS)
> > +			rv = test_proc_pid_statm(pid);
> >  
> >  		/* Cut the rope. */

Hi Alexey,
Thanks for reviewing the changes.

I assume below output of /proc/${procid}/statm
can be assumed as mentioned below:

static const char g_statm[] = "0 0 0 * 0 0 0\n"

If 0 is correct at their places, only issue is *,
whose value will be single digit or could change?

If this assumption is correct, i can change the
validation to handle 4th postion, and remaining
place will validate if it has zero or not,
and will send another patch?

Thanks,
Swarup