> -----Original Message-----
> From: Davi Arnaut [mailto:davi.arnaut@stripped]
> Sent: Thursday, November 18, 2010 5:44 PM
> To: Vladislav Vaintroub
> Cc: commits@stripped
> Subject: Re: bzr commit into mysql-5.5-runtime branch (davi:3186) Bug#54790
> I'll add a explanation. Essentially, it allows for a single, common and
> shared implementation. If Windows XP supported WSAPoll(), I would have
> gone for poll even there.
>
Ok..
Looking forward to fixing "kill" command bugs, I hope we'll not repeat libevent
mistakes and introduce monsters like socketpairs
to signal events (just because select() or WSAPoll() would not work with pipes) .
Old-fashioned WaitForSingle/MultipleObjects or
a little more fancy asynchronous IO could be a better tool for the job :) But I digress,
this has nothing to do with this patch.
> >> +int vio_timeout(Vio *vio, uint which, int timeout_sec)
> > [skip]
> >> + /* Deduce the current timeout status mode. */ + old_mode=
> >> vio->write_timeout< 0&& vio->read_timeout< 0;
> >
> > Maybe more obvious variable name than old_mode (was_blocking?), or
> > more explanation what and how was deduced.
>
> It is deducing a change in the mode of the timeout values, whether both
> of them represent infinite timeouts. Blocking mode is on a layer below,
> so it doesn't make much sense. I'll improve the comment.
OK, so the "mode" either some-timeout (== 0, either send or recv or both are timed
operations), or no-timeout (==1, neither send
nor receive are timed).
> >
> >> === modified file 'vio/viosocket.c' --- a/vio/viosocket.c
> >> 2010-08-16 12:50:27 +0000
> >
> >
> >> +/* + Define a stub MSG_DONTWAIT if unavailable. In this case,
> >> fcntl + (or a equivalent) is used to enable non-blocking
> >> operations. + The flag must be supported in both send and recv
> >> operations. +*/ +#if defined(__linux__) +#define VIO_USE_DONTWAIT
> >> 1 +#define VIO_DONTWAIT MSG_DONTWAIT +#else +#define
> >> VIO_DONTWAIT 0 +#endif
> >
> > I have to admit I have not fully understood why it is a hard
> > requirement to support MSG_DONTWAIT in both send() and recv(). In
> > recv() , it is supported by Linux, Solaris and FreeBSD,while in
> > send() it is supported by Linux only. So maybe we can take advantage
> > of the optimization here by defining 2 constants say
> > VIO_MSG_DONTWAIT_SEND ( == MSG_DONTWAIT on Linux, 0 everywhere else)
> > , and VIO_MSG_DONTWAIT_RECV (== MSG_DONTWAIT on Linux, Solaris and
> > BSD).
>
> Currently there is no way to take advantage of the optimization just for
> recv(). This is because there will always be a timeout for sending and
> receiving, as there is no way to just set a timeout just for receive.
> Since in order to implement the send timeout we need to change the
> socket to non-blocking mode, the use of the flag in receive becomes
> useless as the socket is already in non-blocking mode. If this changes
> in the future, we can revisit it. Otherwise, it would just be dead code.
Ok
> >
> >> + /* Deduce what should be the new blocking mode of the socket.
> >> */ + my_bool new_mode= vio->write_timeout< 0&&
> >> vio->read_timeout< 0; + + /* If necessary, update the blocking
> >> mode. */ + if (new_mode != old_mode) + ret=
> >> vio_set_blocking(vio, new_mode);
> >
> > See earlier comment, I think it would be better understandable if
> > old_mode were named was_blocking and new_mode were named
> > is_blocking or something like this..
>
> But the information is dependent upon the timeout value, not the
> blocking mode. You have to consider what we use as the source of the
> information. I'll add a comment to clarify.
Ok
> >
> >> +static int +select_poll(my_socket fd, enum enum_vio_io_event
> >> event, struct timeval *timeout)
> >
> > {
> >> + int ret; + fd_set readfds, writefds, exceptfds;
> >
> > This is minor , but you only need 2 fd_sets, you never use all 3
> > together, only readfds/exceptfds or writefds/exceptfds, so you might
> > just have fds and exceptfds .
>
> But select takes three fds. I would have to either use a pointer or a
> conditional branch to pass the fds as the second or third parameter.
Yeah, but you could also spare some conditionals/switches:
This:
+ case VIO_IO_EVENT_READ:
+ /* Readiness for reading. */
+ FD_SET(fd, &readfds);
+ break;
+ case VIO_IO_EVENT_WRITE:
+ case VIO_IO_EVENT_CONNECT:
+ /* Readiness for writing. */
+ FD_SET(fd, &writefds);
+ break;
}
replacing by
FD_SET(&fds);
and this
+ case VIO_IO_EVENT_READ:
+ ret= test(FD_ISSET(fd, &readfds));
+ break;
+ case VIO_IO_EVENT_WRITE:
+ case VIO_IO_EVENT_CONNECT:
+ ret= test(FD_ISSET(fd, &writefds));
+ break;
replacing by
ret = test(FD_ISSET(fd, &fds));
Anyway, it not so important, just a suggestion you can ignore if you feel like it..
> >> + /* The first argument is ignored on Windows. */ + ret=
> >> select(IF_WIN(0, fd + 1),&readfds,&writefds,&exceptfds,
> timeout);
> >
> > It is already under #ifdef _WIN32, no IF_WIN required.
> >
> >> + IF_WIN(int, socklen_t) optlen= sizeof(error); +
> >> IF_WIN(char, void) *optval= (IF_WIN(char, void) *)&error;
> >
> > Maybe there is a way to reduce 3 IF_WIN s to a single #ifdef _WIN32
> > ?
>
> Personally, the result looked more ugly two me. I would actually prefer
> proper typedefs here. Do we set socklen_t to int on Windows? I could
> introduce another type for the argument to setsockopt.
SOCKET_SIZE_TYPE maybe, does it look close enough?
> Regards,
>
> Davi
>
> --
> MySQL Code Commits Mailing List
> For list archives: http://lists.mysql.com/commits
> To unsubscribe: http://lists.mysql.com/commits?unsub=1
