Re: [LTP] [PATCH v2] sys_hwmon01: always report a result

[Li Wang <li.wang@linux.dev>]

> Hi!
> >  #include "tst_test.h"
> >  #include "tst_sysfs_assert.h"
> > @@ -56,39 +58,67 @@
> >  
> >  #define MAX_CHANNELS 32
> >  
> > +static unsigned int checked;
> > +
> > +static int sysfs_attr_exists(const char *fmt, ...)
> > +{
> > +	char path[PATH_MAX];
> > +	va_list va;
> > +
> > +	va_start(va, fmt);
> > +	vsnprintf(path, sizeof(path), fmt, va);
> > +	va_end(va);
> > +
> > +	return tst_sysfs_exists(path);
> > +}
> 
> Uff, the tst_sysfs_attr_exists() does work with printf-like parameters.
> 
> What do we need the this wrapper for?

Indeed, I missed that it already takes printf-like parameters.
 
> > +#define CHECK_RANGELL(lo, hi, ...) do { \
> > +	if (sysfs_attr_exists(__VA_ARGS__)) { \
> > +		checked++; \
> > +		TST_SYSFS_ASSERT_RANGELL_SILENT(lo, hi, __VA_ARGS__); \
> > +	} \
> > +} while (0)
> > +
> > +#define CHECK_BOOL(...) do { \
> > +	if (sysfs_attr_exists(__VA_ARGS__)) { \
> > +		checked++; \
> > +		TST_SYSFS_ASSERT_BOOL_SILENT(__VA_ARGS__); \
> > +	} \
> > +} while (0)
> 
> What about we add the counter into the sysfs library instead?
> 
> If we add:
> 
> void tst_sysfs_cnt_reset(void);
> 
> unsigned int tst_sysfs_cnt_get(void);
> 
> We can call the tst_sysfs_cnt_rest() at the start of the test function,
> and get the counter at the end.

Good idea, that's cleaner on all fronts: the CHECK_* wrappers and the
extra existence probe go away entirely (the current version stats each
attribute twice), and any other SILENT-only sysfs test gets the same
fix for free.

I'll split v3 into two patches: one adding
tst_sysfs_cnt_reset()/tst_sysfs_cnt_get() with the counter bumped in
the SILENT assertions when the attribute exists, and the hwmon test on
top of it.

One question: should the counter count attributes that exist and get
checked regardless of pass/fail, or only passing ones? I'd go with the
former, since failures already produce TFAIL results on their own.

-- 
Regards,
Li Wang

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