Re: [Qemu-devel] [sheepdog] [PATCH v3 03/10] sheepdog: check return values of qemu_co_recv/send correctly

[MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp>]

At Thu, 25 Jul 2013 16:46:36 +0800,
Liu Yuan wrote:
> 
> On Thu, Jul 25, 2013 at 05:31:58PM +0900, MORITA Kazutaka wrote:
> > If qemu_co_recv/send doesn't return the specified length, it means
> > that an error happened.
> > 
> > Signed-off-by: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp>
> > ---
> >  block/sheepdog.c | 16 ++++++++--------
> >  1 file changed, 8 insertions(+), 8 deletions(-)
> > 
> > diff --git a/block/sheepdog.c b/block/sheepdog.c
> > index 6a41ad9..c6e9b89 100644
> > --- a/block/sheepdog.c
> > +++ b/block/sheepdog.c
> > @@ -489,13 +489,13 @@ static coroutine_fn int send_co_req(int sockfd, SheepdogReq *hdr, void *data,
> >      int ret;
> >  
> >      ret = qemu_co_send(sockfd, hdr, sizeof(*hdr));
> > -    if (ret < sizeof(*hdr)) {
> > +    if (ret != sizeof(*hdr)) {
> >          error_report("failed to send a req, %s", strerror(errno));
> >          return ret;
> >      }
> >  
> >      ret = qemu_co_send(sockfd, data, *wlen);
> > -    if (ret < *wlen) {
> > +    if (ret != *wlen) {
> >          error_report("failed to send a req, %s", strerror(errno));
> >      }
> >  
> > @@ -548,7 +548,7 @@ static coroutine_fn void do_co_req(void *opaque)
> >      qemu_aio_set_fd_handler(sockfd, restart_co_req, NULL, have_co_req, co);
> >  
> >      ret = qemu_co_recv(sockfd, hdr, sizeof(*hdr));
> > -    if (ret < sizeof(*hdr)) {
> > +    if (ret != sizeof(*hdr)) {
> >          error_report("failed to get a rsp, %s", strerror(errno));
> >          ret = -errno;
> >          goto out;
> > @@ -560,7 +560,7 @@ static coroutine_fn void do_co_req(void *opaque)
> >  
> >      if (*rlen) {
> >          ret = qemu_co_recv(sockfd, data, *rlen);
> > -        if (ret < *rlen) {
> > +        if (ret != *rlen) {
> >              error_report("failed to get the data, %s", strerror(errno));
> >              ret = -errno;
> >              goto out;
> > @@ -671,7 +671,7 @@ static void coroutine_fn aio_read_response(void *opaque)
> >  
> >      /* read a header */
> >      ret = qemu_co_recv(fd, &rsp, sizeof(rsp));
> > -    if (ret < 0) {
> > +    if (ret != sizeof(rsp)) {
> >          error_report("failed to get the header, %s", strerror(errno));
> >          goto out;
> >      }
> > @@ -722,7 +722,7 @@ static void coroutine_fn aio_read_response(void *opaque)
> >      case AIOCB_READ_UDATA:
> >          ret = qemu_co_recvv(fd, acb->qiov->iov, acb->qiov->niov,
> >                              aio_req->iov_offset, rsp.data_length);
> > -        if (ret < 0) {
> > +        if (ret != rsp.data_length) {
> >              error_report("failed to get the data, %s", strerror(errno));
> >              goto out;
> >          }
> > @@ -1075,7 +1075,7 @@ static int coroutine_fn add_aio_request(BDRVSheepdogState *s, AIOReq *aio_req,
> >  
> >      /* send a header */
> >      ret = qemu_co_send(s->fd, &hdr, sizeof(hdr));
> > -    if (ret < 0) {
> > +    if (ret != sizeof(hdr)) {
> >          qemu_co_mutex_unlock(&s->lock);
> >          error_report("failed to send a req, %s", strerror(errno));
> >          return -errno;
> > @@ -1083,7 +1083,7 @@ static int coroutine_fn add_aio_request(BDRVSheepdogState *s, AIOReq *aio_req,
> >  
> >      if (wlen) {
> >          ret = qemu_co_sendv(s->fd, iov, niov, aio_req->iov_offset, wlen);
> > -        if (ret < 0) {
> > +        if (ret != wlen) {
> >              qemu_co_mutex_unlock(&s->lock);
> >              error_report("failed to send a data, %s", strerror(errno));
> >              return -errno;
> 
> These checks are wrong because signed int will be converted to unsgned int. E.g,
> 
> ret = -1;
> (ret < sizeof(hdr)) will always be false, since -1 is converted to unsigned int.

Yes, that's the reason I replaced (ret < sizeof(hdr)) with (ret != sizeof(hdr)).

ret = -1;
(ret != sizeof(hdr)) will be true.

Thanks,

Kazutaka