Re: [LTP] [PATCH 01/13] lib: tst_sys_conf: Add two functions

[Petr Vorel <pvorel@suse.cz>]

Hi Cyril,

> Add two functions to read/write integer values from/to sysfs or procfs
> files.

> With that we replace the get_sys_tune() and set_sys_tune() from
> testcases/kernel/mem/lib/mem.c with a better implementation.

> We also remove the inclusion of the mem library from tunables, since
> it's no longer needed there.

+1

Reviewed-by: Petr Vorel <pvorel@suse.cz>

Minor formatting notes below.

> +/**
> + * TST_SYS_CONF_LONG_SET()

I wonder how to silence warning when there is no short description.

include/tst_sys_conf.h:32: [kernel-doc WARN] : missing initial short description of 'TST_SYS_CONF_LONG_SET'

> + *
> + * Sets a sysfs or procfs file and optionally checks that it was set correctly.
> + *
> + * @param path A path to a sysfs or a procfs file.
> + * @param val A long int value to be written to the file.
> + * @param check If non-zero the library reads the file back and checks that the

nit: please fix these before merge (doxygen => sphinx syntax):

include/tst_sys_conf.h:32: [kernel-doc WARN] : no description found for parameter 'path'
include/tst_sys_conf.h:32: [kernel-doc WARN] : no description found for parameter 'val'
include/tst_sys_conf.h:32: [kernel-doc WARN] : no description found for parameter 'check'

> + *              value is the one we have written there. If not the library calls
> + *              tst_brk(TBROK, ...).
> + */


> +#define TST_SYS_CONF_LONG_SET(path, val, check) \
> +	tst_sys_conf_long_set_(__FILE__, __LINE__, path, val, check)
> +
> +void tst_sys_conf_long_set_(const char *file, const int lineno,
> +                            const char *path, long val, int check);
> +
> +
> +/**
> + * TST_SYS_CONF_LONG_GET()
> + *
> + * Gets a sysfs or procfs file value and converts it to long.
> + *
> + * @param path A path to a sysfs or a procfs file.
And here:

include/tst_sys_conf.h:50: [kernel-doc WARN] : no description found for parameter 'path'

> + */
> +#define TST_SYS_CONF_LONG_GET(path) \
> +	tst_sys_conf_long_get_(__FILE__, __LINE__, path)
> +
> +long tst_sys_conf_long_get_(const char *file, const int lineno,
> +                            const char *path);
> +
>  #endif
> diff --git a/lib/tst_sys_conf.c b/lib/tst_sys_conf.c
> index c0981dcb1..f962fc124 100644
> --- a/lib/tst_sys_conf.c
> +++ b/lib/tst_sys_conf.c
> @@ -145,3 +145,32 @@ void tst_sys_conf_restore(int verbose)
>  	}
>  }

> +long tst_sys_conf_long_get_(const char *file, const int lineno,
> +                            const char *path)
nit: could you please use tabs instead of spaces?
Because it triggers warnings:

$ make check-tst_sys_conf
CHECK lib/tst_sys_conf.c
tst_sys_conf.c:149: ERROR: code indent should use tabs where possible
tst_sys_conf.c:149: WARNING: please, no spaces at the start of a line
tst_sys_conf.c:159: ERROR: code indent should use tabs where possible
tst_sys_conf.c:159: WARNING: please, no spaces at the start of a line
tst_sys_conf.c:161: ERROR: code indent should use tabs where possible
tst_sys_conf.c:161: WARNING: please, no spaces at the start of a line
tst_sys_conf.c:163: ERROR: code indent should use tabs where possible
tst_sys_conf.c:163: WARNING: please, no spaces at the start of a line
tst_sys_conf.c:165: ERROR: code indent should use tabs where possible
tst_sys_conf.c:165: WARNING: please, no spaces at the start of a line
tst_sys_conf.c:170: ERROR: code indent should use tabs where possible
tst_sys_conf.c:170: WARNING: please, no spaces at the start of a line
tst_sys_conf.c:171: ERROR: code indent should use tabs where possible
tst_sys_conf.c:171: WARNING: please, no spaces at the start of a line
tst_sys_conf.c:172: ERROR: code indent should use tabs where possible
tst_sys_conf.c:173: ERROR: code indent should use tabs where possible
tst_sys_conf.c:173: WARNING: please, no spaces at the start of a line
tst_sys_conf.c:174: ERROR: code indent should use tabs where possible
tst_sys_conf.c:174: WARNING: please, no spaces at the start of a line


> +{
> +	long ret;
> +
> +	safe_file_scanf(file, lineno, NULL, path, "%ld", &ret);
> +
> +	return ret;
> +}
> +
> +void tst_sys_conf_long_set_(const char *file, const int lineno,
> +                            const char *path, long val, int check)
> +{
> +        tst_res_(file, lineno, TINFO, "Setting %s to %ld", path, val);
> +
> +        safe_file_printf(file, lineno, NULL, path, "%ld", val);
> +
> +        if (check) {
> +		long read_val;
nit: obviously also here are tabs and spaces mixed. Minor, but worth to fix
before merge.

Maybe we would benefit to add .editorconfig [1] (see examples [2] [3]).

Also maybe to save indent?

if (!check)
	return;

Kind regards,
Petr


> +
> +		safe_file_scanf(file, lineno, NULL, path, "%ld", &read_val);
> +
> +                if (val != read_val)
> +                        tst_brk_(file, lineno, TBROK,
> +			         "Wrote %ld to %s but read back %ld",
> +                                 val, path, read_val);
> +        }
> +
> +}

[1] https://editorconfig.org/
[2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/.editorconfig
[3] https://git.busybox.net/buildroot/tree/.editorconfig

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