Re: [LTP] [PATCH v4 01/31] lib: Add tst_sysfs_assert

[Petr Vorel <pvorel@suse.cz>]

Hi Cyril,

FYI more minor formatting notes. There is often conflict that doc readable in
opened file has broken formatting and correct formatting for HTML looks slightly
less readable when opening file. I personally prefer better HTML formatting.

Anyway, whole sysfs library code looks really nice.
Reviewed-by: Petr Vorel <pvorel@suse.cz>

...
> diff --git a/include/tst_sysfs_assert.h b/include/tst_sysfs_assert.h
> +
> +/**
> + * TST_SYSFS_ASSERT_CHOICE() - Asserts that file is a list of choices.
> + *
> + * Validates a bracketed-choice file such as:
> + *
> + *   "none mq-deadline kyber [bfq]"
> + *   "[always] madvise never"

nit: this is inline, but who cares.

> + *
> + * Asserts that exactly one token is [selected]. If allowed is non-NULL (a
> + * NULL-terminated array of strings) it also asserts that every token is a
> + * member of the allowed set. If sel is non-NULL the selected token (with the
> + * brackets stripped) is copied into sel, truncated to sel_size.
> + *
> + * Skips with TCONF if the file does not exist.
> + *
> + * @allowed: Optional NULL terminated array of allowed choices.
> + * @sel: A buffer to copy the selected choice into.
> + * @sel_size: A size of the sel buffer.
> + * @fmt: A printf-like format to build a path to the file.
> + * @...: A printf-like parameters for fmt.
> + */
> +#define TST_SYSFS_ASSERT_CHOICE(allowed, sel, sel_size, fmt, ...) \
> +	tst_sysfs_assert_choice(__FILE__, __LINE__, allowed, sel, sel_size, \
> +		fmt, ##__VA_ARGS__)
> +
> +void tst_sysfs_assert_choice(const char *file, const int lineno,
> +	const char *const allowed[], char *sel, size_t sel_size,
> +	const char *fmt, ...);
> +
> +/**
> + * TST_SYSFS_ASSERT_TOKENS() - Asserts that file is a list of tokens.
> + *
> + * Validates a file that lists a whitespace-separated set of tokens without any
> + * bracketed "current" selection, such as /sys/power/state:
> + *
> + *   "freeze mem disk"
> + *
> + * Asserts that the file is non-empty and, if allowed is non-NULL, that every
> + * token is a member of the allowed set (a NULL-terminated array of strings).
> + *
> + * If find is non-NULL it additionally asserts that find itself was seen
> + * among the tokens, which is useful for cross-checking a "current" value
> + * read from a different file against this file's list, e.g.:
> + *
> + *   TST_SYSFS_ASSERT_TOKENS(NULL, current_clocksource,
> + *                           "%s/available_clocksource", name);
But this has completely broken formatting in generated html, looking like:

TST_SYSFS_ASSERT_TOKENS(NULL, current_clocksource,

	“%s/available_clocksource”, name);

where first line is bold due tab shift.

=> I'd just before merge put it without tab and put into single line.

 * TST_SYSFS_ASSERT_TOKENS(NULL, current_clocksource, "%s/available_clocksource", name);

> + *
> + * Skips with TCONF if the file does not exist.
> + *
> + * @allowed: Optional NULL terminated array of allowed tokens.
> + * @find: Optional token that must be present in the file, or NULL to skip
> + *        this check.
> + * @fmt: A printf-like format to build a path to the file.
> + * @...: A printf-like parameters for fmt.
> + */
...

> +void tst_sysfs_assert_list_contains(const char *file, const int lineno,
> +	int id, const char *fmt, ...);
> +
> +/**
> + * enum tst_sysfs_cmp - Comparison operators used by tst_sysfs_assert_cmp().

nit: FYI tst_sysfs_assert_cmp() is visible in HTML doc ...
> + *
> + * @TST_SYSFS_CMP_EQ: val1 == val2
> + * @TST_SYSFS_CMP_LT: val1 < val2
> + * @TST_SYSFS_CMP_LE: val1 <= val2
> + */
> +enum tst_sysfs_cmp {
> +	TST_SYSFS_CMP_EQ,
> +	TST_SYSFS_CMP_LT,
> +	TST_SYSFS_CMP_LE,
> +};
> +

... because this has no /** */ documentation.

But I perfectly understand why you did it this way, maybe note macros as well:

/**
 * enum tst_sysfs_cmp - Comparison operators used by tst_sysfs_assert_cmp()
 * which is used by TST_SYSFS_ASSERT_EQ() and other test macros.
 */

Kind regards,
Petr

> +void tst_sysfs_assert_cmp(const char *file, const int lineno,
> +	const char *path1, enum tst_sysfs_cmp op, const char *path2);
...

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