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

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Fri, Aug 03, 2018 at 05:24:52PM +0200, Roger Pau Monné wrote:
> 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.

I thought I was following a pattern found elsewhere in libxl
(datacopier_readable), but apparently not. I'm not sure what would be
better to do. I think I'll just remove this loop and simply return on
EINTR.

-- 
Anthony PERARD

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