Re: [Xen-devel] [PATCH v4 14/32] libxl_qmp: Implement fd callback and read data

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Thu, Aug 02, 2018 at 11:56:11AM +0200, Roger Pau Monné wrote:
> On Fri, Jul 27, 2018 at 03:05:56PM +0100, Anthony PERARD wrote:
> > +        /* The -1 is because there is always space for a NUL character */
> > +        r = read(fd, ev->rx_buf + ev->buf_used,
> > +                 ev->buf_size - ev->buf_used - 1);
> > +        if (r < 0) {
> > +            if (errno == EINTR) continue;
> > +            assert(errno);
> > +            if (errno == EWOULDBLOCK) {
> > +                return 0;
> > +            }
> > +            LOGED(ERROR, ev->domid, "error reading QMP socket");
> > +            return ERROR_FAIL;
> 
> I think it would be clearer to use:
> 
> assert(errno);
> switch (errno) {
> case EINTR:
>     continue;
> case EWOULDBLOCK
>     return 0;
> default:
>     LOGED(...)

That's not the same. In the patch, the only errno allowed is EINTR,
anything else is consider a programmer's mistake. For users (build with
NDEBUG), EWOULDBLOCK is also allowed and not considered an error and
other errno will attempt to let libxl generate a usefull error, instead
of an abort().

> > +        }
> > +        break;
> > +    }
> > +
> > +    if (r == 0) {
> > +        LOGD(ERROR, ev->domid, "No data read on QMP socket");
> > +        return 0;
> > +    }
> > +
> > +    LOG_QMP("received %ldB: '%.*s'", r, (int)r, ev->rx_buf + ev->buf_used);
> > +
> > +    ev->buf_used += r;
> 
> Hm, don't you need to do this inside of a loop together with the
> realloc, in case the data on the fd is bigger than the current space
> on the buffer, and you need to keep growing it until you hit
> EWOULDBLOCK?
> 
> I think with the current approach you can leave data pending in the fd
> if the buffer happens to be smaller than the pending data in the fd?

You would think that, but the read loop isn't in this file. The loop is
part of the libxl__ev_fd. As long as there is data to read from the fd,
the callback (initialized by libxl__ev_fd_register) is going to be
called. This callback, which ultimatly calls qmp_ev_callback_readable,
will attempt to read only once (unless interrupted, EINTR). EWOULDBLOCK
should never be seen.

There is a poll() somewhere in libxl which takes care of checking if
there's data left in the fd, and will call the callback associated with
that fd.

> >  static void qmp_ev_fd_callback(libxl__egc *egc, libxl__ev_fd *ev_fd,
> >                                 int fd, short events, short revents)
> >  {
> > +    EGC_GC;
> > +    int rc;
> > +
> > +    libxl__ev_qmp *ev = CONTAINER_OF(ev_fd, *ev, qmp_efd);
> > +
> > +    if (revents & (POLLHUP)) {
> > +        LOGD(DEBUG, ev->domid, "received POLLHUP from QMP socket");
> > +        qmp_ev_callback_error(egc, ev);
> > +        return;
> > +    }
> > +    if (revents & ~(POLLIN|POLLOUT)) {
> > +        LOGD(ERROR, ev->domid,
> > +             "unexpected poll event 0x%x on QMP socket (expected POLLIN "
> > +             "and/or POLLOUT)",
> > +            revents);
> > +        qmp_ev_callback_error(egc, ev);
> > +        return;
> > +    }
> > +
> > +    if (revents & POLLIN) {
> > +        rc = qmp_ev_callback_readable(egc, ev, fd);
> > +        if (rc)
> > +            goto out;
> > +    }
> > +out:
> 
> Since you already have an out label that calls _error, why not use it
> for the failure paths above by manually setting rc?

Right, I'll make better use of this out label.

> > +    if (rc) {
> > +        qmp_ev_callback_error(egc, ev);
> > +    }
> >  }
> >  
> >  static int qmp_ev_connect(libxl__gc *gc, libxl__ev_qmp *ev)
> > @@ -1346,6 +1442,8 @@ void libxl__ev_qmp_init(libxl__ev_qmp *ev)
> >      ev->qmp_cfd = NULL;
> >      libxl__ev_fd_init(&ev->qmp_efd);
> >      ev->qmp_state = qmp_state_disconnected;
> > +
> > +    ev->rx_buf = NULL;
> 
> Aren't you missing a
> 
> ev->buf_size = ev->buf_used = ev->buf_consumed = 0;

Not really, if there is no rx_buf, those variables can be undefined.

> >  }
> >  
> >  int libxl__ev_qmp_send(libxl__gc *gc, libxl__ev_qmp *ev,
> > @@ -1365,6 +1463,9 @@ void libxl__ev_qmp_dispose(libxl__gc *gc, 
> > libxl__ev_qmp *ev)
> >  {
> >      LOGD(DEBUG, ev->domid, " ev %p", ev);
> >  
> > +    free(ev->rx_buf);
> > +    ev->rx_buf = NULL;
> 
> I don't think you need to set rx_buf to NULL if you call _init
> afterwards.

Will remove it.

Thanks,

-- 
Anthony PERARD

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel