Re: [PATCH] Updated: add a receive_timeout option for sockets

[Kevin J. McCarthy]

On Mon, Nov 28, 2022 at 07:45:00PM -0600, David Vernet wrote:

On Mon, Nov 28, 2022 at 04:14:10PM -0800, Matthew Sotoudeh via Mutt-dev wrote:

Per feedback from Kevin & David (thanks!), moved the timeout code into
socket_connect and updated the description to be more descriptive and
more closely match the existing connect_timeout.

I'm not sure how the mutt review process usually works (Kevin can chime
in here to correct me if it's different around here), but in Linux
kernel reviews, the differences between patch revisions is put below the
--- at the sign off. See [0] for more information. If you put it here,
it is like you are proposing to include it in the commit summary.

Also, follow-on iterations of a patch should have the version specified
(e.g. -v 2 for git format-patch), and be sent as a separate email thread
rather than being sent as a reply to an earlier thread. Again, Kevin can
tell me if things are different for mutt reviews.

We're not so formal here, because the volume is quite low, especially as I've eased down development. A lot of our review has also moved to gitlab, although I like to have things reviewed here that might be interesting to a wider audience.

There is some open question about where exactly in socket_connect to put
the timeout code. Currently I put it after the basic input validation
but it may be slightly better to wait until the socket is actually
opened? But being a global option anyways, I don't think it can hurt

FWIW, I would do it before the socket is opened.

Agree.

diff --git a/mutt_socket.c b/mutt_socket.c
index 3e192072..4e1b7889 100644
--- a/mutt_socket.c
+++ b/mutt_socket.c
@@ -407,6 +407,12 @@ static int socket_connect (int fd, struct sockaddr* sa)
     return -1;
   }

+  if (ReceiveTimeout > 0)
+  {
+    struct timeval tv = { ReceiveTimeout, 0 };
+    setsockopt(fd, SOL_SOCKET, SO_RCVTIMEO, (char *)&tv, sizeof(tv));
+  }

socket_connect() doesn't seem super concerned with checking return
values elsewhere (e.g. in sigaction() calls), but IMO there's no reason
for us to not add them for new code. Could you please check the return
value of setsockopt() here?  Not sure if it's preferable to return that
value to the caller, or instead just print a dprint() message if it
fails and continue on. Given that we don't even propagate the error for
connect(), I expect the latter is preferable. Kevin, thoughts?

--
Kevin J. McCarthy
GPG Fingerprint: 8975 A9B3 3AA3 7910 385C  5308 ADEF 7684 8031 6BDA

The connect error looks to be returned and used in raw_socket_open(). But for setsockopt, I agree just adding a dprint and continuing should be okay.

signature.asc
Description: PGP signature