All of lore.kernel.org
 help / color / mirror / Atom feed
* [LTP] [PATCH 1/3] lib: introduce safe_write_fully()
@ 2022-10-03 12:48 Jan Stancek
  2022-10-03 12:48 ` [LTP] [PATCH 2/3] syscalls/preadv203: don't treat short write() as failure Jan Stancek
                   ` (3 more replies)
  0 siblings, 4 replies; 13+ messages in thread
From: Jan Stancek @ 2022-10-03 12:48 UTC (permalink / raw)
  To: ltp

In case there is a short (but otherwise successful) write(),
safe_write_fully() repeats write() and attempts to resume
with the remainder of the buffer.

Signed-off-by: Jan Stancek <jstancek@redhat.com>
---
 include/tst_safe_macros.h |  5 +++++
 lib/tst_safe_macros.c     | 19 +++++++++++++++++++
 2 files changed, 24 insertions(+)

diff --git a/include/tst_safe_macros.h b/include/tst_safe_macros.h
index 81c4b0844267..caee0e9cf842 100644
--- a/include/tst_safe_macros.h
+++ b/include/tst_safe_macros.h
@@ -645,4 +645,9 @@ int safe_sysinfo(const char *file, const int lineno, struct sysinfo *info);
 #define SAFE_SYSINFO(info) \
 	safe_sysinfo(__FILE__, __LINE__, (info))
 
+ssize_t safe_write_fully(const char *file, const int lineno,
+	int fildes, const void *buf, size_t nbyte);
+#define SAFE_WRITE_FULLY(fildes, buf, nbyte) \
+	safe_write_fully(__FILE__, __LINE__, (fildes), (buf), (nbyte))
+
 #endif /* SAFE_MACROS_H__ */
diff --git a/lib/tst_safe_macros.c b/lib/tst_safe_macros.c
index c4cdc87e7346..e4d4ef4269a4 100644
--- a/lib/tst_safe_macros.c
+++ b/lib/tst_safe_macros.c
@@ -591,3 +591,22 @@ void safe_cmd(const char *file, const int lineno, const char *const argv[],
 		tst_brk_(file, lineno, TBROK, "%s failed (%d)", argv[0], rval);
 	}
 }
+
+ssize_t safe_write_fully(const char *file, const int lineno,
+	int fildes, const void *buf, size_t nbyte)
+{
+	ssize_t rval;
+	size_t len = nbyte;
+
+	do {
+		rval = write(fildes, buf, len);
+		if (rval == -1) {
+			tst_brk_(file, lineno, TBROK | TERRNO,
+				"write(%d,%p,%zu) failed", fildes, buf, len);
+		}
+		buf += rval;
+		len -= rval;
+	} while (len > 0);
+
+	return nbyte;
+}
-- 
2.27.0


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

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [LTP] [PATCH 2/3] syscalls/preadv203: don't treat short write() as failure
  2022-10-03 12:48 [LTP] [PATCH 1/3] lib: introduce safe_write_fully() Jan Stancek
@ 2022-10-03 12:48 ` Jan Stancek
  2022-10-03 12:48 ` [LTP] [PATCH 3/3] mmapstress04: use SAFE_WRITE_FULLY from LTP library Jan Stancek
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 13+ messages in thread
From: Jan Stancek @ 2022-10-03 12:48 UTC (permalink / raw)
  To: ltp

Test is sporadically running into TBROK in setup() due to short write:
  tst_test.c:1064: TINFO: Formatting /dev/loop0 with xfs opts='' extra opts=''
  preadv203.c:257: TBROK: write(501,0x3fff4ef7d15,4123) failed: SUCCESS (0)

Switch to SAFE_WRITE_FULLY().

Signed-off-by: Jan Stancek <jstancek@redhat.com>
---
 testcases/kernel/syscalls/preadv2/preadv203.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/testcases/kernel/syscalls/preadv2/preadv203.c b/testcases/kernel/syscalls/preadv2/preadv203.c
index 60dc4a882f16..f3e3ef438de2 100644
--- a/testcases/kernel/syscalls/preadv2/preadv203.c
+++ b/testcases/kernel/syscalls/preadv2/preadv203.c
@@ -254,7 +254,7 @@ static void setup(void)
 
 		for (j = 0; j < CHUNKS; j++) {
 			memset(buf, '0' + j, sizeof(buf));
-			SAFE_WRITE(1, fds[i], buf, sizeof(buf));
+			SAFE_WRITE_FULLY(fds[i], buf, sizeof(buf));
 		}
 	}
 }
-- 
2.27.0


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

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [LTP] [PATCH 3/3] mmapstress04: use SAFE_WRITE_FULLY from LTP library
  2022-10-03 12:48 [LTP] [PATCH 1/3] lib: introduce safe_write_fully() Jan Stancek
  2022-10-03 12:48 ` [LTP] [PATCH 2/3] syscalls/preadv203: don't treat short write() as failure Jan Stancek
@ 2022-10-03 12:48 ` Jan Stancek
  2022-10-03 13:16 ` [LTP] [PATCH 1/3] lib: introduce safe_write_fully() Cyril Hrubis
  2022-10-04  8:31 ` [LTP] [PATCH v2] lib: introduce safe_write() retry Jan Stancek
  3 siblings, 0 replies; 13+ messages in thread
From: Jan Stancek @ 2022-10-03 12:48 UTC (permalink / raw)
  To: ltp

Signed-off-by: Jan Stancek <jstancek@redhat.com>
---
 testcases/kernel/mem/mmapstress/mmapstress04.c | 13 +------------
 1 file changed, 1 insertion(+), 12 deletions(-)

diff --git a/testcases/kernel/mem/mmapstress/mmapstress04.c b/testcases/kernel/mem/mmapstress/mmapstress04.c
index ceede7eaa860..2b421890a461 100644
--- a/testcases/kernel/mem/mmapstress/mmapstress04.c
+++ b/testcases/kernel/mem/mmapstress/mmapstress04.c
@@ -32,17 +32,6 @@ static void setup(void)
 		MAP_PRIVATE|MAP_ANONYMOUS, -1, 0);
 }
 
-static void write_fully(int fd, void *buf, int len)
-{
-	int ret;
-
-	do {
-		ret = SAFE_WRITE(0, fd, buf, len);
-		buf += ret;
-		len -= ret;
-	} while (len > 0);
-}
-
 static void mmapstress04(void)
 {
 	int i, j, rofd, rwfd;
@@ -85,7 +74,7 @@ static void mmapstress04(void)
 	buf = SAFE_MALLOC(page_size);
 	memset(buf, 'a', page_size);
 	for (i = 0; i < 6 * 64; i++)
-		write_fully(rwfd, buf, page_size);
+		SAFE_WRITE_FULLY(rwfd, buf, page_size);
 	free(buf);
 	SAFE_CLOSE(rwfd);
 
-- 
2.27.0


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

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [LTP] [PATCH 1/3] lib: introduce safe_write_fully()
  2022-10-03 12:48 [LTP] [PATCH 1/3] lib: introduce safe_write_fully() Jan Stancek
  2022-10-03 12:48 ` [LTP] [PATCH 2/3] syscalls/preadv203: don't treat short write() as failure Jan Stancek
  2022-10-03 12:48 ` [LTP] [PATCH 3/3] mmapstress04: use SAFE_WRITE_FULLY from LTP library Jan Stancek
@ 2022-10-03 13:16 ` Cyril Hrubis
  2022-10-03 13:56   ` Jan Stancek
  2022-10-04  8:31 ` [LTP] [PATCH v2] lib: introduce safe_write() retry Jan Stancek
  3 siblings, 1 reply; 13+ messages in thread
From: Cyril Hrubis @ 2022-10-03 13:16 UTC (permalink / raw)
  To: Jan Stancek; +Cc: ltp

Hi!
> In case there is a short (but otherwise successful) write(),
> safe_write_fully() repeats write() and attempts to resume
> with the remainder of the buffer.
> 
> Signed-off-by: Jan Stancek <jstancek@redhat.com>
> ---
>  include/tst_safe_macros.h |  5 +++++
>  lib/tst_safe_macros.c     | 19 +++++++++++++++++++
>  2 files changed, 24 insertions(+)
> 
> diff --git a/include/tst_safe_macros.h b/include/tst_safe_macros.h
> index 81c4b0844267..caee0e9cf842 100644
> --- a/include/tst_safe_macros.h
> +++ b/include/tst_safe_macros.h
> @@ -645,4 +645,9 @@ int safe_sysinfo(const char *file, const int lineno, struct sysinfo *info);
>  #define SAFE_SYSINFO(info) \
>  	safe_sysinfo(__FILE__, __LINE__, (info))
>  
> +ssize_t safe_write_fully(const char *file, const int lineno,
> +	int fildes, const void *buf, size_t nbyte);
> +#define SAFE_WRITE_FULLY(fildes, buf, nbyte) \
> +	safe_write_fully(__FILE__, __LINE__, (fildes), (buf), (nbyte))

We already have a flag for SAFE_WRITE() to fail/not-fail on partial
write, what about turning that into three way switch?

Something as:

enum safe_write_opts {
	SAFE_WRITE_PARTIAL = 0,
	SAFE_WRITE_FULL = 1,
	SAFE_WRITE_RETRY = 2,
};

Or maybe just rename the SAFE_WRITE_FULLY() to SAFE_WRITE_RETRY().

>  #endif /* SAFE_MACROS_H__ */
> diff --git a/lib/tst_safe_macros.c b/lib/tst_safe_macros.c
> index c4cdc87e7346..e4d4ef4269a4 100644
> --- a/lib/tst_safe_macros.c
> +++ b/lib/tst_safe_macros.c
> @@ -591,3 +591,22 @@ void safe_cmd(const char *file, const int lineno, const char *const argv[],
>  		tst_brk_(file, lineno, TBROK, "%s failed (%d)", argv[0], rval);
>  	}
>  }
> +
> +ssize_t safe_write_fully(const char *file, const int lineno,
> +	int fildes, const void *buf, size_t nbyte)
> +{
> +	ssize_t rval;
> +	size_t len = nbyte;
> +
> +	do {
> +		rval = write(fildes, buf, len);
> +		if (rval == -1) {
> +			tst_brk_(file, lineno, TBROK | TERRNO,
> +				"write(%d,%p,%zu) failed", fildes, buf, len);

I guess that we may print potentionally confusing output here since we
modify the buf and len in the loop. I guess that we should store the buf
pointer at the start just for a case of this message and use the nbyte
and possibly write how many bytes we have managed to write before the
failure.

> +		}
> +		buf += rval;
> +		len -= rval;
> +	} while (len > 0);
> +
> +	return nbyte;
> +}
> -- 
> 2.27.0
> 
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

-- 
Cyril Hrubis
chrubis@suse.cz

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [LTP] [PATCH 1/3] lib: introduce safe_write_fully()
  2022-10-03 13:16 ` [LTP] [PATCH 1/3] lib: introduce safe_write_fully() Cyril Hrubis
@ 2022-10-03 13:56   ` Jan Stancek
  2022-10-03 14:00     ` Cyril Hrubis
  0 siblings, 1 reply; 13+ messages in thread
From: Jan Stancek @ 2022-10-03 13:56 UTC (permalink / raw)
  To: Cyril Hrubis; +Cc: ltp

On Mon, Oct 3, 2022 at 3:14 PM Cyril Hrubis <chrubis@suse.cz> wrote:
>
> Hi!
> > In case there is a short (but otherwise successful) write(),
> > safe_write_fully() repeats write() and attempts to resume
> > with the remainder of the buffer.
> >
> > Signed-off-by: Jan Stancek <jstancek@redhat.com>
> > ---
> >  include/tst_safe_macros.h |  5 +++++
> >  lib/tst_safe_macros.c     | 19 +++++++++++++++++++
> >  2 files changed, 24 insertions(+)
> >
> > diff --git a/include/tst_safe_macros.h b/include/tst_safe_macros.h
> > index 81c4b0844267..caee0e9cf842 100644
> > --- a/include/tst_safe_macros.h
> > +++ b/include/tst_safe_macros.h
> > @@ -645,4 +645,9 @@ int safe_sysinfo(const char *file, const int lineno, struct sysinfo *info);
> >  #define SAFE_SYSINFO(info) \
> >       safe_sysinfo(__FILE__, __LINE__, (info))
> >
> > +ssize_t safe_write_fully(const char *file, const int lineno,
> > +     int fildes, const void *buf, size_t nbyte);
> > +#define SAFE_WRITE_FULLY(fildes, buf, nbyte) \
> > +     safe_write_fully(__FILE__, __LINE__, (fildes), (buf), (nbyte))
>
> We already have a flag for SAFE_WRITE() to fail/not-fail on partial
> write, what about turning that into three way switch?
>
> Something as:
>
> enum safe_write_opts {
>         SAFE_WRITE_PARTIAL = 0,
>         SAFE_WRITE_FULL = 1,
>         SAFE_WRITE_RETRY = 2,
> };

I do find those names little confusing. What do you think about:

SAFE_WRITE_ANY = 0 // no strictness
SAFE_WRITE_ALL = 1 // all strict
SAFE_WRITE_RETRY = 2 // retry


>
> Or maybe just rename the SAFE_WRITE_FULLY() to SAFE_WRITE_RETRY().
>
> >  #endif /* SAFE_MACROS_H__ */
> > diff --git a/lib/tst_safe_macros.c b/lib/tst_safe_macros.c
> > index c4cdc87e7346..e4d4ef4269a4 100644
> > --- a/lib/tst_safe_macros.c
> > +++ b/lib/tst_safe_macros.c
> > @@ -591,3 +591,22 @@ void safe_cmd(const char *file, const int lineno, const char *const argv[],
> >               tst_brk_(file, lineno, TBROK, "%s failed (%d)", argv[0], rval);
> >       }
> >  }
> > +
> > +ssize_t safe_write_fully(const char *file, const int lineno,
> > +     int fildes, const void *buf, size_t nbyte)
> > +{
> > +     ssize_t rval;
> > +     size_t len = nbyte;
> > +
> > +     do {
> > +             rval = write(fildes, buf, len);
> > +             if (rval == -1) {
> > +                     tst_brk_(file, lineno, TBROK | TERRNO,
> > +                             "write(%d,%p,%zu) failed", fildes, buf, len);
>
> I guess that we may print potentionally confusing output here since we
> modify the buf and len in the loop. I guess that we should store the buf
> pointer at the start just for a case of this message and use the nbyte
> and possibly write how many bytes we have managed to write before the
> failure.

ack


>
> > +             }
> > +             buf += rval;
> > +             len -= rval;
> > +     } while (len > 0);
> > +
> > +     return nbyte;
> > +}
> > --
> > 2.27.0
> >
> >
> > --
> > Mailing list info: https://lists.linux.it/listinfo/ltp
>
> --
> Cyril Hrubis
> chrubis@suse.cz
>


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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [LTP] [PATCH 1/3] lib: introduce safe_write_fully()
  2022-10-03 13:56   ` Jan Stancek
@ 2022-10-03 14:00     ` Cyril Hrubis
  0 siblings, 0 replies; 13+ messages in thread
From: Cyril Hrubis @ 2022-10-03 14:00 UTC (permalink / raw)
  To: Jan Stancek; +Cc: ltp

Hi!
> I do find those names little confusing. What do you think about:
> 
> SAFE_WRITE_ANY = 0 // no strictness
> SAFE_WRITE_ALL = 1 // all strict
> SAFE_WRITE_RETRY = 2 // retry

+1 for making them shorter and more descriptive.

-- 
Cyril Hrubis
chrubis@suse.cz

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [LTP] [PATCH v2] lib: introduce safe_write() retry
  2022-10-03 12:48 [LTP] [PATCH 1/3] lib: introduce safe_write_fully() Jan Stancek
                   ` (2 preceding siblings ...)
  2022-10-03 13:16 ` [LTP] [PATCH 1/3] lib: introduce safe_write_fully() Cyril Hrubis
@ 2022-10-04  8:31 ` Jan Stancek
  2022-10-04 12:20   ` Petr Vorel
  2022-10-05  9:32   ` Cyril Hrubis
  3 siblings, 2 replies; 13+ messages in thread
From: Jan Stancek @ 2022-10-04  8:31 UTC (permalink / raw)
  To: ltp

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.

Signed-off-by: Jan Stancek <jstancek@redhat.com>
---
 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(-)

I can send follow-up to update all call sites too.

diff --git a/include/safe_macros_fn.h b/include/safe_macros_fn.h
index 3df952811b94..8eb03edd81ba 100644
--- a/include/safe_macros_fn.h
+++ 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
+};
+
 char* safe_basename(const char *file, const int lineno,
                     void (*cleanup_fn)(void), char *path);
 
@@ -111,8 +118,8 @@ int safe_symlink(const char *file, const int lineno,
                  const char *newpath);
 
 ssize_t safe_write(const char *file, const int lineno,
-                   void (cleanup_fn)(void), char len_strict, int fildes,
-                   const void *buf, size_t nbyte);
+                   void (cleanup_fn)(void), enum safe_write_opts len_strict,
+                   int fildes, const void *buf, size_t nbyte);
 
 long safe_strtol(const char *file, const int lineno,
                  void (cleanup_fn)(void), char *str, long min, long max);
diff --git a/lib/safe_macros.c b/lib/safe_macros.c
index 16e582bc976b..eac31f4ce3ff 100644
--- a/lib/safe_macros.c
+++ b/lib/safe_macros.c
@@ -524,20 +524,42 @@ int safe_symlink(const char *file, const int lineno,
 }
 
 ssize_t safe_write(const char *file, const int lineno, void (cleanup_fn) (void),
-                   char len_strict, int fildes, const void *buf, size_t nbyte)
+                   enum safe_write_opts len_strict, int fildes, const void *buf,
+                   size_t nbyte)
 {
 	ssize_t rval;
+	const void *wbuf = buf;
+	size_t len = nbyte;
+	int iter = 0;
+
+	do {
+		iter++;
+		rval = write(fildes, wbuf, len);
+		if (rval == -1) {
+			if (len_strict == SAFE_WRITE_RETRY)
+				tst_resm_(file, lineno, TINFO,
+					"write() wrote %zu bytes in %d calls",
+					nbyte-len, iter);
+			tst_brkm_(file, lineno, TBROK | TERRNO,
+				cleanup_fn, "write(%d,%p,%zu) failed",
+				fildes, buf, nbyte);
+		}
 
-	rval = write(fildes, buf, nbyte);
+		if (len_strict == SAFE_WRITE_ANY)
+			return rval;
 
-	if (rval == -1 || (len_strict && (size_t)rval != nbyte)) {
-		tst_brkm_(file, lineno, TBROK | TERRNO, cleanup_fn,
-			"write(%d,%p,%zu) failed", fildes, buf, nbyte);
-	} else if (rval < 0) {
-		tst_brkm_(file, lineno, TBROK | TERRNO, cleanup_fn,
-			"Invalid write(%d,%p,%zu) return value %zd", fildes,
-			buf, nbyte, rval);
-	}
+		if (len_strict == SAFE_WRITE_ALL) {
+			if ((size_t)rval != nbyte)
+				tst_brkm_(file, lineno, TBROK | TERRNO,
+					cleanup_fn, "short write(%d,%p,%zu) "
+					"return value %zd",
+					fildes, buf, nbyte, rval);
+			return rval;
+		}
+
+		wbuf += rval;
+		len -= rval;
+	} while (len > 0);
 
 	return rval;
 }
diff --git a/lib/tests/tst_safe_macros.c b/lib/tests/tst_safe_macros.c
index b5809f40d10e..5c427ee16832 100644
--- a/lib/tests/tst_safe_macros.c
+++ b/lib/tests/tst_safe_macros.c
@@ -31,9 +31,9 @@ int main(int argc LTP_ATTRIBUTE_UNUSED, char **argv)
 	printf("buf: %s\n", buf);
 	SAFE_READ(cleanup, 1, fd, buf, 9);
 	printf("buf: %s\n", buf);
-	SAFE_WRITE(cleanup, 0, -1, buf, 9);
-	SAFE_WRITE(NULL, 0, fd, buf, 9);
-	SAFE_WRITE(NULL, 1, fd, buf, 9);
+	SAFE_WRITE(cleanup, SAFE_WRITE_ANY, -1, buf, 9);
+	SAFE_WRITE(NULL, SAFE_WRITE_ANY, fd, buf, 9);
+	SAFE_WRITE(NULL, SAFE_WRITE_ALL, fd, buf, 9);
 	SAFE_PIPE(NULL, fds);
 
 	return 0;
-- 
2.27.0


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

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [LTP] [PATCH v2] lib: introduce safe_write() retry
  2022-10-04  8:31 ` [LTP] [PATCH v2] lib: introduce safe_write() retry Jan Stancek
@ 2022-10-04 12:20   ` Petr Vorel
  2022-10-04 12:34     ` Jan Stancek
  2022-10-05  9:32   ` Cyril Hrubis
  1 sibling, 1 reply; 13+ messages in thread
From: Petr Vorel @ 2022-10-04 12:20 UTC (permalink / raw)
  To: Jan Stancek; +Cc: ltp

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [LTP] [PATCH v2] lib: introduce safe_write() retry
  2022-10-04 12:20   ` Petr Vorel
@ 2022-10-04 12:34     ` Jan Stancek
  2022-10-04 18:45       ` Petr Vorel
  0 siblings, 1 reply; 13+ messages in thread
From: Jan Stancek @ 2022-10-04 12:34 UTC (permalink / raw)
  To: Petr Vorel; +Cc: ltp

On Tue, Oct 4, 2022 at 2:20 PM Petr Vorel <pvorel@suse.cz> wrote:
>
> 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,

It's currently shared code.

> 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)?

We could have 2 implementations for safe_write, but modifying existing one
seemed better option. There's no harm supporting new option in old API too.

> 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

thanks, I missed that.

>
> Kind regards,
> Petr
>


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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [LTP] [PATCH v2] lib: introduce safe_write() retry
  2022-10-04 12:34     ` Jan Stancek
@ 2022-10-04 18:45       ` Petr Vorel
  0 siblings, 0 replies; 13+ messages in thread
From: Petr Vorel @ 2022-10-04 18:45 UTC (permalink / raw)
  To: Jan Stancek; +Cc: ltp

Hi Jan,

...
> > LGTM. I just wonder if we need to add it to lib/safe_macros.c,

> It's currently shared code.
Ah, I'm sorry.

> > 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)?

> We could have 2 implementations for safe_write, but modifying existing one
> seemed better option. There's no harm supporting new option in old API too.

Sure, np.

Kind regards,
Petr

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [LTP] [PATCH v2] lib: introduce safe_write() retry
  2022-10-04  8:31 ` [LTP] [PATCH v2] lib: introduce safe_write() retry Jan Stancek
  2022-10-04 12:20   ` Petr Vorel
@ 2022-10-05  9:32   ` Cyril Hrubis
  2022-10-05 12:02     ` Jan Stancek
  1 sibling, 1 reply; 13+ messages in thread
From: Cyril Hrubis @ 2022-10-05  9:32 UTC (permalink / raw)
  To: Jan Stancek; +Cc: ltp

Hi!
Reviewed-by: Cyril Hrubis <chrubis@suse.cz>

-- 
Cyril Hrubis
chrubis@suse.cz

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [LTP] [PATCH v2] lib: introduce safe_write() retry
  2022-10-05  9:32   ` Cyril Hrubis
@ 2022-10-05 12:02     ` Jan Stancek
  2022-10-17 14:15       ` Richard Palethorpe
  0 siblings, 1 reply; 13+ messages in thread
From: Jan Stancek @ 2022-10-05 12:02 UTC (permalink / raw)
  To: Cyril Hrubis; +Cc: ltp

On Wed, Oct 5, 2022 at 11:31 AM Cyril Hrubis <chrubis@suse.cz> wrote:
>
> Hi!
> Reviewed-by: Cyril Hrubis <chrubis@suse.cz>

Pushed with Petr notes about comments and whitespace fixed.

>
> --
> Cyril Hrubis
> chrubis@suse.cz
>


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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [LTP] [PATCH v2] lib: introduce safe_write() retry
  2022-10-05 12:02     ` Jan Stancek
@ 2022-10-17 14:15       ` Richard Palethorpe
  0 siblings, 0 replies; 13+ messages in thread
From: Richard Palethorpe @ 2022-10-17 14:15 UTC (permalink / raw)
  To: Jan Stancek; +Cc: ltp

Hello,

Jan Stancek <jstancek@redhat.com> writes:

> On Wed, Oct 5, 2022 at 11:31 AM Cyril Hrubis <chrubis@suse.cz> wrote:
>>
>> Hi!
>> Reviewed-by: Cyril Hrubis <chrubis@suse.cz>
>
> Pushed with Petr notes about comments and whitespace fixed.
>
>>
>> --
>> Cyril Hrubis
>> chrubis@suse.cz
>>

I have set this to accepted in patchwork. Please update patchwork when
merging something.

-- 
Thank you,
Richard.

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2022-10-17 14:16 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-10-03 12:48 [LTP] [PATCH 1/3] lib: introduce safe_write_fully() Jan Stancek
2022-10-03 12:48 ` [LTP] [PATCH 2/3] syscalls/preadv203: don't treat short write() as failure Jan Stancek
2022-10-03 12:48 ` [LTP] [PATCH 3/3] mmapstress04: use SAFE_WRITE_FULLY from LTP library Jan Stancek
2022-10-03 13:16 ` [LTP] [PATCH 1/3] lib: introduce safe_write_fully() Cyril Hrubis
2022-10-03 13:56   ` Jan Stancek
2022-10-03 14:00     ` Cyril Hrubis
2022-10-04  8:31 ` [LTP] [PATCH v2] lib: introduce safe_write() retry Jan Stancek
2022-10-04 12:20   ` Petr Vorel
2022-10-04 12:34     ` Jan Stancek
2022-10-04 18:45       ` Petr Vorel
2022-10-05  9:32   ` Cyril Hrubis
2022-10-05 12:02     ` Jan Stancek
2022-10-17 14:15       ` Richard Palethorpe

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.