Re: [PATCH] Updated: add a receive_timeout option for sockets
On Mon, Nov 28, 2022 at 06:48:38PM -0800, Kevin J. McCarthy wrote:
> 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.
Fair enough! Thanks for clarifying for me. Matthew -- feel free to
ignore my advice ;-)
>
> > > 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 *), 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?
>
> 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.
Ah, right you are. I missed that it was saving the value as save_errno.
Seems kind of odd to continue on like that and then do e.g. sigaction(),
but that's neither here nor there. It sounds like we're aligned on the
right / lightest touch approach here being to just dprint() and move on.
Thanks,
David
Re: [PATCH] Updated: add a receive_timeout option for sockets
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 *), 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?
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.
--
Kevin J. McCarthy
GPG Fingerprint: 8975 A9B3 3AA3 7910 385C 5308 ADEF 7684 8031 6BDA
signature.asc
Description: PGP signature
Re: [PATCH] Updated: add a receive_timeout option for sockets
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
> ---
> 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=}, {.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=}, {.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 *), 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
>
[PATCH] Updated: add a receive_timeout option for sockets
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.
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
anything.
Signed-off-by: Matthew Sotoudeh
---
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=}, {.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=}, {.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 *), sizeof(tv));
+ }
+
/* Batch mode does not call mutt_signal_init(), so ensure the alarm
* interrupts the connect call */
if (ConnectTimeout > 0)
--
2.34.1
