Re: [LTP] [PATCH v2] lib: introduce safe_write() retry

[Petr Vorel <pvorel@suse.cz>]

Hi Jan,

> Turn safe_write() len_strict parameter into 3-way switch, introducing
> one additional mode of operation "retry". On short writes, this
> resumes write() with remainder of the buffer.

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

> ---
>  include/safe_macros_fn.h    | 11 ++++++++--
>  lib/safe_macros.c           | 42 ++++++++++++++++++++++++++++---------
>  lib/tests/tst_safe_macros.c |  6 +++---
>  3 files changed, 44 insertions(+), 15 deletions(-)

LGTM. I just wonder if we need to add it to lib/safe_macros.c,
which implements it for the old API. Would it work to add it only to
tst_safe_macros.c and tst_safe_macros.h (instead of safe_macros_fn.h)?
Not a requirement, just if it makes sense to you.

...
> +++ b/include/safe_macros_fn.h
> @@ -24,6 +24,13 @@
>  #include <unistd.h>
>  #include <dirent.h>

> +/* supported values for safe_write() len_strict parameter */
> +enum safe_write_opts {
> +        SAFE_WRITE_ANY = 0,	// no length strictness, short writes are ok
> +        SAFE_WRITE_ALL = 1,	// strict length, short writes raise TBROK
> +        SAFE_WRITE_RETRY = 2,	// retry/resume after short write
> +};

Maybe use /* */ and for readability, maybe put into it's own line?

enum safe_write_opts {
	/* no length strictness, short writes are ok */
	SAFE_WRITE_ANY = 0,

	/* strict length, short writes raise TBROK */
	SAFE_WRITE_ALL = 1,

	/* retry/resume after short write */
	SAFE_WRITE_RETRY = 2,	// 
};

Also checkpatch.pl complains:

safe_macros_fn.h:29: ERROR: code indent should use tabs where possible
safe_macros_fn.h:29: WARNING: please, no spaces at the start of a line
safe_macros_fn.h:30: ERROR: code indent should use tabs where possible
safe_macros_fn.h:30: WARNING: please, no spaces at the start of a line
safe_macros_fn.h:31: ERROR: code indent should use tabs where possible
safe_macros_fn.h:31: WARNING: please, no spaces at the start of a line

Kind regards,
Petr

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