| Message ID | 20241117225950.138968-1-leocstone@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [net-next] selftests: net/psock_lib: Handle EINTR and EAGAIN | expand |
Leo Stone wrote: > Make pair_udp_send_char handle EAGAIN, EINTR, and partial reads or > writes. > > Signed-off-by: Leo Stone <leocstone@gmail.com> Did you observe actual issues or is this based on the comment in the existing code ("Should really handle EINTR and EAGAIN"). AFAIK all the users of psock_lib use default blocking IO, so should not see EAGAIN. A simpler approach to dealing with EINTR is to ask glibc to restart with sigaction or siginterrupt. > --- > tools/testing/selftests/net/psock_lib.h | 39 +++++++++++++++++++------ > 1 file changed, 30 insertions(+), 9 deletions(-) > > diff --git a/tools/testing/selftests/net/psock_lib.h b/tools/testing/selftests/net/psock_lib.h > index 6e4fef560873..e28bff95c8e2 100644 > --- a/tools/testing/selftests/net/psock_lib.h > +++ b/tools/testing/selftests/net/psock_lib.h > @@ -13,6 +13,7 @@ > #include <string.h> > #include <arpa/inet.h> > #include <unistd.h> > +#include <stdbool.h> > > #include "kselftest.h" > > @@ -110,21 +111,41 @@ static __maybe_unused void pair_udp_open(int fds[], uint16_t port) > } > } > > +static void read_write_checked(int fd, char *buf, size_t sz, bool is_write) > +{ > + int bytes_processed = 0; > + int ret; > + > + do { > + if (is_write) > + ret = write(fd, buf + bytes_processed, > + sz - bytes_processed); > + else > + ret = read(fd, buf + bytes_processed, > + sz - bytes_processed); > + if (ret == -1) { > + if (errno == EAGAIN || errno == EINTR) { > + continue; Instead of busy looping, on return with EAGAIN should probably block. > + } else { > + fprintf(stderr, "ERROR: %s failed, bytes left=%lu\n", > + is_write ? "send" : "recv", > + sz - bytes_processed); > + exit(1); > + } > + } > + bytes_processed += ret; > + } while (bytes_processed < sz); > +} > + > static __maybe_unused void pair_udp_send_char(int fds[], int num, char payload) > { > char buf[DATA_LEN], rbuf[DATA_LEN]; > > memset(buf, payload, sizeof(buf)); > while (num--) { > - /* Should really handle EINTR and EAGAIN */ > - if (write(fds[0], buf, sizeof(buf)) != sizeof(buf)) { > - fprintf(stderr, "ERROR: send failed left=%d\n", num); > - exit(1); > - } > - if (read(fds[1], rbuf, sizeof(rbuf)) != sizeof(rbuf)) { > - fprintf(stderr, "ERROR: recv failed left=%d\n", num); > - exit(1); > - } > + read_write_checked(fds[0], buf, sizeof(buf), true); > + read_write_checked(fds[1], rbuf, sizeof(rbuf), false); > + > if (memcmp(buf, rbuf, sizeof(buf))) { > fprintf(stderr, "ERROR: data failed left=%d\n", num); > exit(1); > -- > 2.39.5 >
Willem de Bruijn wrote: > Did you observe actual issues or is this based on the comment in the > existing code ("Should really handle EINTR and EAGAIN"). No, this patch was based purely off of the comment, and the examples of similar code in other selftests, e.g. tools/testing/selftests/bpf/xsk.c: > static int netlink_recvmsg(int sock, struct msghdr *mhdr, int flags) > { > int len; > > do { > len = recvmsg(sock, mhdr, flags); > } while (len < 0 && (errno == EINTR || errno == EAGAIN)); > > if (len < 0) > return -errno; > return len; > } > A simpler approach to dealing with EINTR is to ask glibc to restart > with sigaction or siginterrupt. > If we want to handle it that way, this patch is probably not necessary, since there is no specific signal I would want to install a handler for. Thanks for your feedback, Leo
diff --git a/tools/testing/selftests/net/psock_lib.h b/tools/testing/selftests/net/psock_lib.h index 6e4fef560873..e28bff95c8e2 100644 --- a/tools/testing/selftests/net/psock_lib.h +++ b/tools/testing/selftests/net/psock_lib.h @@ -13,6 +13,7 @@ #include <string.h> #include <arpa/inet.h> #include <unistd.h> +#include <stdbool.h> #include "kselftest.h" @@ -110,21 +111,41 @@ static __maybe_unused void pair_udp_open(int fds[], uint16_t port) } } +static void read_write_checked(int fd, char *buf, size_t sz, bool is_write) +{ + int bytes_processed = 0; + int ret; + + do { + if (is_write) + ret = write(fd, buf + bytes_processed, + sz - bytes_processed); + else + ret = read(fd, buf + bytes_processed, + sz - bytes_processed); + if (ret == -1) { + if (errno == EAGAIN || errno == EINTR) { + continue; + } else { + fprintf(stderr, "ERROR: %s failed, bytes left=%lu\n", + is_write ? "send" : "recv", + sz - bytes_processed); + exit(1); + } + } + bytes_processed += ret; + } while (bytes_processed < sz); +} + static __maybe_unused void pair_udp_send_char(int fds[], int num, char payload) { char buf[DATA_LEN], rbuf[DATA_LEN]; memset(buf, payload, sizeof(buf)); while (num--) { - /* Should really handle EINTR and EAGAIN */ - if (write(fds[0], buf, sizeof(buf)) != sizeof(buf)) { - fprintf(stderr, "ERROR: send failed left=%d\n", num); - exit(1); - } - if (read(fds[1], rbuf, sizeof(rbuf)) != sizeof(rbuf)) { - fprintf(stderr, "ERROR: recv failed left=%d\n", num); - exit(1); - } + read_write_checked(fds[0], buf, sizeof(buf), true); + read_write_checked(fds[1], rbuf, sizeof(rbuf), false); + if (memcmp(buf, rbuf, sizeof(buf))) { fprintf(stderr, "ERROR: data failed left=%d\n", num); exit(1);
Make pair_udp_send_char handle EAGAIN, EINTR, and partial reads or writes. Signed-off-by: Leo Stone <leocstone@gmail.com> --- tools/testing/selftests/net/psock_lib.h | 39 +++++++++++++++++++------ 1 file changed, 30 insertions(+), 9 deletions(-)