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.
> 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.
> anything.
>
> Signed-off-by: Matthew Sotoudeh <matt...@masot.net>
> ---
> globals.h | 1 +
> init.h | 7 +++++++
> mutt_socket.c | 6 ++++++
> 3 files changed, 14 insertions(+)
>
> diff --git a/globals.h b/globals.h
> index 06ce410e..b782114e 100644
> --- a/globals.h
> +++ b/globals.h
> @@ -236,6 +236,7 @@ WHERE short PagerContext;
> WHERE short PagerIndexLines;
> WHERE short PagerSkipQuotedContext;
> WHERE short ReadInc;
> +WHERE short ReceiveTimeout;
> WHERE short ReflowWrap;
> WHERE short SaveHist;
> WHERE short SendmailWait;
> diff --git a/init.h b/init.h
> index e160d3d3..e268d796 100644
> --- a/init.h
> +++ b/init.h
> @@ -3221,6 +3221,13 @@ struct option_t MuttVars[] = {
> ** .pp
> ** Also see $$postponed variable.
> */
> + { "receive_timeout", DT_NUM, R_NONE, {.p=&ReceiveTimeout}, {.l=0} },
> + /*
> + ** .pp
> + ** Causes Mutt to timeout any socket read operation (e.g. SSL_read) after
> + ** this many seconds. A zero (default) or negative value causes Mutt to
> wait
> + ** indefinitely for the read to complete.
> + */
> { "record", DT_PATH, R_NONE, {.p=&Outbox}, {.p="~/sent"} },
> /*
> ** .pp
> 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?
Thanks,
David
> +
> /* Batch mode does not call mutt_signal_init(), so ensure the alarm
> * interrupts the connect call */
> if (ConnectTimeout > 0)
> --
> 2.34.1
>
