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 *)&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?
>
> 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
