Re: [Xen-devel] [PATCH] Switch to poll() in cxenstored's IO loop

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Tue, 2013-01-22 at 09:47 +0000, Ian Campbell wrote:
> On Mon, 2013-01-21 at 19:23 +0000, Wei Liu wrote:
> 
> > -           if (reopen_log_pipe[0] != -1 && FD_ISSET(reopen_log_pipe[0], 
> > &inset)) {
> > +           if (reopen_log_pipe[0] != -1 && reopen_log_pipe0_pollfd &&
> 
> Is it the case that reopen_log_pip0_pollfd != NULL iff
> reopen_log_pipe[0] != -1 ? Would simplify things a bit?
> 

Not really. Pollfd is valid iff set_fd() returns a valid pointer.
set_fd() invokes realloc() which can fail.

> > +               !(reopen_log_pipe0_pollfd->revents & 
> > ~(POLLIN|POLLOUT|POLLPRI)) &&
> 
> This represents an error case I think? Is there anything we can do about
> it? If this sort of error occurs and we do nothing will we end up just
> spinning because every subsequent poll will just return straight away?
> 
> 

Yes, this represents an error. This indicates the pipe used to trigger
reopen_log is broken. What's the motivation of reopening log file? I
have no idea about the use case. But simply ignoring this error instead
of crashing the process is better choice IMHO.

The whole pollfd set is reinitialized for every loop. So it is also
possible that for the next loop it will success even previous poll
fails?

> > +               (reopen_log_pipe0_pollfd->revents & POLLIN)) {
> 
> You only handle POLLIN, not POLLOUT or POLLPRI -- so should they not be
> part of the error case above? I don't think you ask for them other than
> on the conn->poll_fd?
> 
> (those three comments apply to the other instances of this pattern too)
> [...]
> > @@ -1939,21 +1981,27 @@ int main(int argc, char *argv[])
> >                             if (talloc_free(conn) == 0)
> >                                     continue;
> >                     } else {
> > -                           if (FD_ISSET(conn->fd, &inset))
> > +                           if (conn->pollfd &&
> > +                               !(conn->pollfd->revents & 
> > ~(POLLIN|POLLOUT|POLLPRI)) &&
> > +                               (conn->pollfd->revents & POLLIN))
> >                                     handle_input(conn);
> >                             if (talloc_free(conn) == 0)
> >                                     continue;
> 
> Do you know what this construct is all about? Some sort of reference
> count on conn? Anyway, I wonder if this means you will frequently miss
> setting pollfd = NULL below?
> 
> talloc_free can apparently only return non-zero if a destructor fails,
> which suggests that more often than not you will not make it as far as
> checking POLLOUT.
> 
> This is all pre-existing though, so maybe it just works and there is
> some magic I don't understand ;-)
> 

The document of talloc_free() says that if the pointer is freed 0 is
returned otherwise -1 is returned.

So if conn is successfully freed, than there is no need to reset pollfd
in conn to NULL.


Wei.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel