Re: [LTP] power_management: rewrite runpwtests04.sh in C

[Andrea Cervesato via ltp <ltp@lists.linux.it>]

Hi Jinseok,

> 
> open() is a tested subject here -- its return code drives the
> TCONF/TFAIL decision. Bare syscalls that are the subject of the
> test must be wrapped in TEST(), not called directly. The same
> applies to read() and write() below.

This is a false positive.

> 
> > +	ret = read(fd, buf, sizeof(buf));
> > +
> > +	if (ret < 0)
> > +		tst_res(TFAIL | TERRNO, "%s read() failed", path);
> > +	else if (ret == 0)
> > +		tst_res(TFAIL, "%s read() returned 0", path);
> > +	else
> > +		tst_res(TPASS, "%s read() successfully", path);
> 
> read() is the primary subject of this test and must be wrapped in
> TEST() so that TST_RET and TST_ERR are populated.

This is false positive.

> 
> > +	if (tcases[i].is_writable) {
> > +		if (write(fd, "invalid_governor", 16) < 0) {
> 
> Same for write().

This is false poitive.

> 
> > +static struct tst_test test = {
> > +	.cleanup = cleanup,
> > +	.needs_root = 1,
> 
> The doc comment only mentions readability. It does not explain why
> root is required. Ground Rule 4 requires the reason to appear in
> the /*\ ... */ block. Root is needed here for writing to
> current_governor and for the save_restore of that path; please
> document that.
> 
> > + * Verify that selected cpuidle sysfs files are readable.
> 
> The test also verifies that writing an invalid governor name to
> current_governor is rejected with EINVAL. The description should
> cover both aspects.
> 
> Verdict - Needs revision

The rest is correct.

--
Andrea Cervesato
SUSE QE Automation Engineer Linux
andrea.cervesato@suse.com

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp