Re: [LTP] lib: Add tst_sysfs_assert

[Cyril Hrubis <chrubis@suse.cz>]

Hi!
> > [PATCH 01/31] lib: Add tst_sysfs_assert
> 
> Most commit bodies in patches 1-20 and 25-31 only restate what the subject
> already says. Could they explain why the coverage or API is needed?

I do not think that this is really needed. This information is in the
top level comment of each test. I do not see a reason to copy&paste it
into the commit description.

> Patch 1 also has the typo "testscases", and the subjects of patches 14
> and 15 do not say that a test is being added.

Will fix.

> --- [PATCH 1/31] ---
> 
> > 	size = read_file(path, buf, sizeof(buf));
> > 	...
> > 	map = SAFE_CALLOC(max_id / 8 + 1, 1);
> > 	size = read_file(path, buf, sizeof(buf));
> > 	...
> > 	return parse_list(path, buf, size, count, max_id, map);
> 
> Could the list be read once, or could parse_list() receive and enforce the
> allocated bitmap size? A dynamic list such as CPU online can gain a higher
> ID between these reads, after which parse_list() writes beyond map.

CPU hotplug during the testrun would break the assertions we have either
way. It must not happen when the test is running.

> --- [PATCH 6/31] ---
> 
> > 	TST_SYSFS_ASSERT_PARSE_LIST(&count, &max_id, SYS_CPU "/online");
> > 	...
> > 	while (fgets(line, sizeof(line), f)) {
> > 		if (sscanf(line, "cpu%u ", &cpu) == 1)
> > 			cpu_count++;
> > 	}
> 
> Could this retry unless the online list is stable around the /proc/stat
> read? CPU hotplug between these independent snapshots produces different
> counts even when both interfaces are correct.

Again, CPU hotplug must not happen during the testrun.

> --- [PATCH 7/31] ---
> 
> > 	snprintf(sub, sizeof(sub),
> > 		 SYS_CPU "/cpu%d/topology/package_cpus_list", cpu);
> > 	TST_SYSFS_ASSERT_LIST_SUBSET(sub, SYS_CPU "/online");
> 
> Could package_cpus_list be compared with possible/present instead? Topology
> sibling masks can contain offline CPUs, so normal CPU hotplug makes this
> assertion fail.

Right, "present" should be better here.

> > 	for (cpu = 0; cpu <= max_id; cpu++)
> > 		check_cpu_topology(cpu, poss_max_id);
> 
> Could this iterate the parsed online mask rather than every ID through its
> maximum? CPU IDs can have offline holes.

I do not see a reason, we properly skip the checks in
check_cpu_topology() if the .../cpu%d/toplogy/ does not exist.

> > 	TST_SYSFS_ASSERT_RANGELL(0, poss_max_id,
> > 			       SYS_CPU "/cpu%d/topology/physical_package_id",
> > 			       cpu);
> 
> Could this accept -1? The generic topology implementation exports that
> sentinel when an architecture does not provide a physical package ID.

Sure, will fix.

> --- [PATCH 9/31] ---
> 
> > static const char *const control_allowed[] = {
> > 	"on", "off", "forceoff", "notsupported", "notimplemented", NULL
> > };
> 
> Could this also parse the numeric control form? Linux 7.2 returns the active
> thread count as a decimal value when partial SMT is enabled.

Fixed as well.

> --- [PATCH 11/31] ---
> 
> > 	while ((ent = SAFE_READDIR(d))) {
> > 		if (strncmp(ent->d_name, "clockevent", 10))
> > 			continue;
> >
> > 		nclockevents++;
> > 		check_clockevent(ent->d_name);
> > 	}
> > 	...
> > 	if (nclockevents <= online_count) {
> 
> Could this validate only clockevent entries for online CPUs? Linux 7.2
> registers clockeventN for every possible CPU, and current_device can be empty
> for an offline CPU. Counting all entries against online CPUs therefore fails
> on systems with offline CPUs.

Fixed.

> --- [PATCH 12/31] ---
> 
> > 	d = SAFE_OPENDIR(ATA);
> 
> Could the class directory be checked before SAFE_OPENDIR()? Without libata,
> its absence currently produces TBROK rather than the documented TCONF.
> 
> The same ordering appears for /sys/class/hwmon in patch 14,
> /sys/class/leds in patch 15, /sys/class/wakeup in patch 16,
> /sys/class/rtc in patch 17, and /sys/class/thermal in patch 18. Could those
> also return TCONF when the corresponding class is unavailable?

Fixed.

> --- [PATCH 13/31] ---
> 
> > 	min_bytes = TST_SYSFS_READ_LI(BDI "/%s/min_bytes", name);
> > 	max_bytes = TST_SYSFS_READ_LI(BDI "/%s/max_bytes", name);
> 
> Could these use an unsigned 64-bit parser? Linux 7.2 exports both attributes
> as u64, so valid values above LONG_MAX are rejected or misparsed, especially
> on 32-bit systems.

I'm looking at path 13/31 and this code is not even there.

There is only:

>       max_bytes = TST_SYSFS_READ_LI(BDI_CLASS "/%s/max_bytes", id);

I guess that this is a hallucination. I can make the max_bytes 64bit
though.

> --- [PATCH 14/31] ---
> 
> > 	TST_SYSFS_ASSERT_RANGELL(0, 1000000, HWMON "/%s/temp%d_input",
> > 			       hwmon, nr);
> > 	...
> > 	if (min > max)
> > 		tst_res(TFAIL, "temp%d_min (%ld) > temp%d_max (%ld)",
> > 			nr, min, nr, max);
> 
> What kernel ABI guarantees these plausibility ranges and threshold
> orderings? Hwmon values and writable thresholds are device-specific, and
> the kernel does not enforce these policies. Valid hardware or configuration
> can therefore fail the test.

Fair enough, I've dropped these checks.

> --- [PATCH 16/31] ---
> 
> > 		TST_SYSFS_ASSERT_RANGELL(0, LONG_MAX, WAKEUP "/%s/%s",
> > 				       name, counters[i]);
> 
> Could these counters use an unsigned-long parser? Linux 7.2 exports them with
> "%lu", so a valid counter above LONG_MAX false-fails on 32-bit systems.

I've switched the helpers to long long, that allows us to pass ULONG_MAX
limits where needed.

And also added TST_SYSFS_ASSERT_RANGEUU() that works with unsigned long
long.

> --- [PATCH 17/31] ---
> 
> > 	if (hctosys)
> > 		check_system_time(rtc);
> > 	...
> > 	if (!hctosys_found)
> > 		check_system_time("rtc0");
> 
> Could this comparison be removed or made informational? hctosys only records
> that an RTC initialized system time at boot. NTP can subsequently correct
> system time without updating the RTC, and rtc0 is not implicitly
> synchronized when no hctosys attribute is set.

This is all described in the test comment. I've decided to keep the
assertion even though NTP may adjust the time.

> > 	TST_SYSFS_READ_STR(date, sizeof(date), RTC "/%s/date", rtc);
> > 	TST_SYSFS_READ_STR(time, sizeof(time), RTC "/%s/time", rtc);
> 
> Could this use RTC_RD_TIME or verify matching date reads around the time
> read? A midnight rollover between these files combines the previous date
> with the next day's time and creates a false failure of about 24 hours.

I've added a retry if the rollover is detected.

> --- [PATCH 19/31] ---
> 
> > /*
> >  * Change the link-layer (MAC) address of an existing network device. Most
> >  * drivers require the device to be administratively down for this to
> >  * succeed.
> >  */
> > int tst_netdev_set_hwaddr(const char *file, const int lineno, int strict,
> > 	const char *ifname, const void *addr, size_t addrlen);
> 
> Could the two new public APIs and their macros use kernel-doc, including
> parameter documentation, so they are included in the generated C API
> reference?

The file is not included in documentation and there are no doc comments,
so I decided not to add them.

> --- [PATCH 20/31] ---
> 
> >  * - carrier is a boolean (0 or 1) when readable
> >  *
> >  * carrier returns an error (EINVAL) when the interface is administratively
> >  * down, which the test tolerates.
> 
> Could the promised carrier check be implemented, or could this claim be
> removed? check_iface() currently validates only type, MTU, addr_len, address,
> and operstate.

I've removed the stale comment.

> --- [PATCH 23/31] ---
> 
> > 	SAFE_CLOSE(fd);
> > 	read_state(state);
> > 	assert_state("down", 0, 0, state);
> > 	...
> > 	fd = open_tun();
> > 	read_state(state);
> > 	assert_state("up", 1, 1, state);
> 
> Could this poll for the expected operstate with a timeout? TUN updates
> carrier synchronously, but netdev_state_change() schedules operstate updates
> through linkwatch. These immediate reads can still observe the previous
> operstate.

This was reported by Li as well, fixed.

> --- [PATCH 24/31] ---
> 
> > 	if (attached) {
> > 		tst_res(TINFO, "Autoclear did not detach the loop device");
> > 		tst_detach_device(loopdev);
> > 		attached = 0;
> > 	}
> 
> Could attached be cleared only when the fallback detach succeeds? If
> tst_detach_device() fails, cleanup() skips the device and the system-wide
> loop attachment is leaked.

This snippet is not present in the patchset. Looks like another
halucination.

> --- [PATCH 25/31] ---
> 
> > 	TST_SYSFS_ASSERT_RANGELL(0, LONG_MAX,
> > 			       QUEUE "/%s/discard_max_bytes", dev);
> 
> Could discard_max_bytes use an unsigned 64-bit parser and range? The block
> queue ABI exports an unsigned 64-bit byte count, which can exceed LONG_MAX
> on 32-bit systems.

Fixed.

> > static const char *const schedulers[] = {
> > 	"none", "mq-deadline", "kyber", "bfq", NULL
> > };
> 
> Could the scheduler check validate only the bracketed single-selection
> format? Elevators are registered dynamically, so vendor or future scheduler
> names outside this fixed list are valid.

It is expected that we will have to add new elevators into the list as
they are added into the kernel. If that proves to be a big hassle, we
will remove the array.

> --- [PATCH 26/31] ---
> 
> > 	if (sscanf(line, "%u %u %*u %*s", &proc_major, &proc_minor) != 4)
> > 		continue;
> 
> Could this compare the return value with 2? Assignment-suppressed conversions
> do not count, so every valid /proc/partitions line is currently skipped and
> the size comparison never runs.

Fixed.

> --- [PATCH 27/31] ---
> 
> > 	nr = TST_SYSFS_READ_LI(HUGEPAGES "/%s/nr_hugepages", name);
> > 	free = TST_SYSFS_READ_LI(HUGEPAGES "/%s/free_hugepages", name);
> > 	resv = TST_SYSFS_READ_LI(HUGEPAGES "/%s/resv_hugepages", name);
> > 	surp = TST_SYSFS_READ_LI(HUGEPAGES "/%s/surplus_hugepages", name);
> 
> Could these checks retry unless surrounding pool counters are stable?
> Allocation, reservation, or pool resizing between the separate reads can
> violate the asserted relationships even though every individual kernel
> snapshot is consistent.

The rest of LTP tests expect that hugepage pools are not manipulated
during their run.

> --- [PATCH 28/31] ---
> 
> > 	proc_val = tst_read_meminfo(proc_name);
> > 	sys_val = TST_SYSFS_READ_LI(HUGEPAGES "/%s", sys_name);
> > 	TST_EXP_EQ_LI(proc_val, sys_val);
> 
> Could this compare stable snapshots or retry on a concurrent change?
> Hugepage allocation, reservation, and pool resizing can occur between the
> /proc and sysfs reads, producing a false mismatch.

Here as well.

> --- [PATCH 29/31] ---
> 
> > 	for (i = 0; i < ARRAY_SIZE(nonneg_counters); i++) {
> > 		TST_SYSFS_ASSERT_RANGELL(0, LONG_MAX, KSM "/%s",
> > 				       nonneg_counters[i]);
> > 	}
> 
> Could pages_to_scan and the page counters use parsers matching their unsigned
> kernel types? Linux 7.2 exports pages_to_scan as unsigned int and the counters
> as unsigned long, so valid values above LONG_MAX false-fail on 32-bit systems.

Fixed.

> --- [PATCH 30/31] ---
> 
> > +sys_swap01 sys_swap01
> 
> > +++ b/testcases/kernel/sysfs/kernel/mm/swap/sys_mm_swap01.c
> 
> Could the source/binary and runtest names be made consistent? This builds
> sys_mm_swap01, while runtest invokes the nonexistent sys_swap01.

Fixed.

-- 
Cyril Hrubis
chrubis@suse.cz

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