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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Fri, Aug 03, 2018 at 03:32:09PM +0100, Anthony PERARD wrote:
> 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().

I'm not sure I follow. According to the man page if read returns -1
errno will always be set to indicate the error, so I'm not sure of the
usefulness of the assert(errno), apart from checking that libc behaves
correctly.

> > > +        }
> > > +        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.

Then why do you have a loop here to handle EINTR?

If the loop is somewhere else isn't it capable of handling EINTR like
it handles partial reads? Adding the loop here just to handle EINTR
makes it look like this is the complete read loop.

Thanks, Roger.

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