* [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.