[Qemu-devel] Several sheepdog functions mix up -1 and -errno in return values

[Qemu-devel] Several sheepdog functions mix up -1 and -errno in return values

[Markus Armbruster]

Two common conventions for functions returning int that may fail:

1. Return non-negative value on success, -1 value on failure.

2. Return non-negative value on success, a negative errno error code on
   failure.

Both work.  But mixing them in the same function is not a good idea.

Suspicious functions in block/sheepdog.c include:

* read_write_object()

  May return 0, -EIO or the value of do_req().  do_req() returns
  srco.ret.  do_co_req() may set it to the value of send_co_req().  I
  don't think that one returns -errno.

* do_sd_create()

  May return 0, -EIO or the value of connect_to_sdog().  I don't think
  the latter returns -errno.

I suspect there are more.  Please audit the file for this kind of
mistake.  Good opportunity to document for each function what it's
supposed to return.

[Qemu-devel] [PATCH] sheepdog: fix confused return values

[Liu Yuan]

From: Liu Yuan <liuyuan@cmss.chinamobile.com>

These functions mix up -1 and -errno in return values and would might cause
trouble error handling in the call chain.

This patch let them return -errno and add some comments.

Reported-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Liu Yuan <liuyuan@cmss.chinamobile.com>
---
 block/sheepdog.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index be3176f..c28658c 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -527,6 +527,7 @@ static SheepdogAIOCB *sd_aio_setup(BlockDriverState *bs, QEMUIOVector *qiov,
     return acb;
 }
 
+/* Return -EIO in case of error, file descriptor on success */
 static int connect_to_sdog(BDRVSheepdogState *s, Error **errp)
 {
     int fd;
@@ -546,11 +547,14 @@ static int connect_to_sdog(BDRVSheepdogState *s, Error **errp)
 
     if (fd >= 0) {
         qemu_set_nonblock(fd);
+    } else {
+        fd = -EIO;
     }
 
     return fd;
 }
 
+/* Return 0 on success and -errno in case of error */
 static coroutine_fn int send_co_req(int sockfd, SheepdogReq *hdr, void *data,
                                     unsigned int *wlen)
 {
@@ -559,11 +563,13 @@ static coroutine_fn int send_co_req(int sockfd, SheepdogReq *hdr, void *data,
     ret = qemu_co_send(sockfd, hdr, sizeof(*hdr));
     if (ret != sizeof(*hdr)) {
         error_report("failed to send a req, %s", strerror(errno));
+        ret = -errno;
         return ret;
     }
 
     ret = qemu_co_send(sockfd, data, *wlen);
     if (ret != *wlen) {
+        ret = -errno;
         error_report("failed to send a req, %s", strerror(errno));
     }
 
@@ -638,6 +644,11 @@ out:
     srco->finished = true;
 }
 
+/*
+ * Send the request to the sheep in a synchronous manner.
+ *
+ * Return 0 on success, -errno in case of error.
+ */
 static int do_req(int sockfd, AioContext *aio_context, SheepdogReq *hdr,
                   void *data, unsigned int *wlen, unsigned int *rlen)
 {
-- 
1.9.1

Re: [Qemu-devel] [PATCH] sheepdog: fix confused return values

[Liu Yuan]

On Fri, Feb 13, 2015 at 11:45:42AM +0800, Liu Yuan wrote:
> From: Liu Yuan <liuyuan@cmss.chinamobile.com>
> 
> These functions mix up -1 and -errno in return values and would might cause
> trouble error handling in the call chain.
> 
> This patch let them return -errno and add some comments.
> 
> Reported-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Liu Yuan <liuyuan@cmss.chinamobile.com>

Cc Kevin and Stefan

Re: [Qemu-devel] [PATCH] sheepdog: fix confused return values

[Kevin Wolf]

Am 13.02.2015 um 04:45 hat Liu Yuan geschrieben:
> From: Liu Yuan <liuyuan@cmss.chinamobile.com>
> 
> These functions mix up -1 and -errno in return values and would might cause
> trouble error handling in the call chain.
> 
> This patch let them return -errno and add some comments.
> 
> Reported-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Liu Yuan <liuyuan@cmss.chinamobile.com>
> ---
>  block/sheepdog.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/block/sheepdog.c b/block/sheepdog.c
> index be3176f..c28658c 100644
> --- a/block/sheepdog.c
> +++ b/block/sheepdog.c
> @@ -527,6 +527,7 @@ static SheepdogAIOCB *sd_aio_setup(BlockDriverState *bs, QEMUIOVector *qiov,
>      return acb;
>  }
>  
> +/* Return -EIO in case of error, file descriptor on success */
>  static int connect_to_sdog(BDRVSheepdogState *s, Error **errp)
>  {
>      int fd;
> @@ -546,11 +547,14 @@ static int connect_to_sdog(BDRVSheepdogState *s, Error **errp)
>  
>      if (fd >= 0) {
>          qemu_set_nonblock(fd);
> +    } else {
> +        fd = -EIO;
>      }
>  
>      return fd;
>  }
>  
> +/* Return 0 on success and -errno in case of error */
>  static coroutine_fn int send_co_req(int sockfd, SheepdogReq *hdr, void *data,
>                                      unsigned int *wlen)
>  {
> @@ -559,11 +563,13 @@ static coroutine_fn int send_co_req(int sockfd, SheepdogReq *hdr, void *data,
>      ret = qemu_co_send(sockfd, hdr, sizeof(*hdr));
>      if (ret != sizeof(*hdr)) {
>          error_report("failed to send a req, %s", strerror(errno));
> +        ret = -errno;

qemu_co_sendv_recvv() uses socket_error() internally to access the
return code. This is defined as errno on POSIX, but as WSAGetLastError()
on Windows.

You should probably either use socket_error() here, or change
qemu_co_sendv_recvv() to return a negative error code instead of -1.

>          return ret;
>      }
>  
>      ret = qemu_co_send(sockfd, data, *wlen);
>      if (ret != *wlen) {
> +        ret = -errno;
>          error_report("failed to send a req, %s", strerror(errno));
>      }

The same here.

Kevin

Re: [Qemu-devel] [PATCH] sheepdog: fix confused return values

[Liu Yuan]

On Mon, Feb 16, 2015 at 12:56:04PM +0100, Kevin Wolf wrote:
> Am 13.02.2015 um 04:45 hat Liu Yuan geschrieben:
> > From: Liu Yuan <liuyuan@cmss.chinamobile.com>
> > 
> > These functions mix up -1 and -errno in return values and would might cause
> > trouble error handling in the call chain.
> > 
> > This patch let them return -errno and add some comments.
> > 
> > Reported-by: Markus Armbruster <armbru@redhat.com>
> > Signed-off-by: Liu Yuan <liuyuan@cmss.chinamobile.com>
> > ---
> >  block/sheepdog.c | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> > 
> > diff --git a/block/sheepdog.c b/block/sheepdog.c
> > index be3176f..c28658c 100644
> > --- a/block/sheepdog.c
> > +++ b/block/sheepdog.c
> > @@ -527,6 +527,7 @@ static SheepdogAIOCB *sd_aio_setup(BlockDriverState *bs, QEMUIOVector *qiov,
> >      return acb;
> >  }
> >  
> > +/* Return -EIO in case of error, file descriptor on success */
> >  static int connect_to_sdog(BDRVSheepdogState *s, Error **errp)
> >  {
> >      int fd;
> > @@ -546,11 +547,14 @@ static int connect_to_sdog(BDRVSheepdogState *s, Error **errp)
> >  
> >      if (fd >= 0) {
> >          qemu_set_nonblock(fd);
> > +    } else {
> > +        fd = -EIO;
> >      }
> >  
> >      return fd;
> >  }
> >  
> > +/* Return 0 on success and -errno in case of error */
> >  static coroutine_fn int send_co_req(int sockfd, SheepdogReq *hdr, void *data,
> >                                      unsigned int *wlen)
> >  {
> > @@ -559,11 +563,13 @@ static coroutine_fn int send_co_req(int sockfd, SheepdogReq *hdr, void *data,
> >      ret = qemu_co_send(sockfd, hdr, sizeof(*hdr));
> >      if (ret != sizeof(*hdr)) {
> >          error_report("failed to send a req, %s", strerror(errno));
> > +        ret = -errno;
> 
> qemu_co_sendv_recvv() uses socket_error() internally to access the
> return code. This is defined as errno on POSIX, but as WSAGetLastError()
> on Windows.
> 
> You should probably either use socket_error() here, or change
> qemu_co_sendv_recvv() to return a negative error code instead of -1.
> 
> >          return ret;
> >      }
> >  
> >      ret = qemu_co_send(sockfd, data, *wlen);
> >      if (ret != *wlen) {
> > +        ret = -errno;
> >          error_report("failed to send a req, %s", strerror(errno));
> >      }
> 
> The same here.
> 

Hi, Kevin, thanks for your tips. I'll post v2 against your comments.

Yuan

Re: [Qemu-devel] [PATCH] sheepdog: fix confused return values

[Markus Armbruster]

Liu Yuan <namei.unix@gmail.com> writes:

> From: Liu Yuan <liuyuan@cmss.chinamobile.com>
>
> These functions mix up -1 and -errno in return values and would might cause
> trouble error handling in the call chain.
>
> This patch let them return -errno and add some comments.
>
> Reported-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Liu Yuan <liuyuan@cmss.chinamobile.com>

Did you review all functions returning negative errno, or just the two
that caught my eye?

Re: [Qemu-devel] [PATCH] sheepdog: fix confused return values

[Liu Yuan]

On Wed, Feb 18, 2015 at 09:35:07AM +0100, Markus Armbruster wrote:
> Liu Yuan <namei.unix@gmail.com> writes:
> 
> > From: Liu Yuan <liuyuan@cmss.chinamobile.com>
> >
> > These functions mix up -1 and -errno in return values and would might cause
> > trouble error handling in the call chain.
> >
> > This patch let them return -errno and add some comments.
> >
> > Reported-by: Markus Armbruster <armbru@redhat.com>
> > Signed-off-by: Liu Yuan <liuyuan@cmss.chinamobile.com>
> 
> Did you review all functions returning negative errno, or just the two
> that caught my eye?

Umm, mostly the two you mentioned.

Yuan

Re: [Qemu-devel] [PATCH] sheepdog: fix confused return values

[Markus Armbruster]

Liu Yuan <namei.unix@gmail.com> writes:

> On Wed, Feb 18, 2015 at 09:35:07AM +0100, Markus Armbruster wrote:
>> Liu Yuan <namei.unix@gmail.com> writes:
>> 
>> > From: Liu Yuan <liuyuan@cmss.chinamobile.com>
>> >
>> > These functions mix up -1 and -errno in return values and would might cause
>> > trouble error handling in the call chain.
>> >
>> > This patch let them return -errno and add some comments.
>> >
>> > Reported-by: Markus Armbruster <armbru@redhat.com>
>> > Signed-off-by: Liu Yuan <liuyuan@cmss.chinamobile.com>
>> 
>> Did you review all functions returning negative errno, or just the two
>> that caught my eye?
>
> Umm, mostly the two you mentioned.

I encourage you to review the whole file for this error pattern!  In my
experience, bugs occur in clusters.  When you find one, there's a high
chance similar ones exist, and looking for them is good practice.

Re: [Qemu-devel] [PATCH] sheepdog: fix confused return values

[Liu Yuan]

On Wed, Feb 25, 2015 at 10:35:17AM +0100, Markus Armbruster wrote:
> Liu Yuan <namei.unix@gmail.com> writes:
> 
> > On Wed, Feb 18, 2015 at 09:35:07AM +0100, Markus Armbruster wrote:
> >> Liu Yuan <namei.unix@gmail.com> writes:
> >> 
> >> > From: Liu Yuan <liuyuan@cmss.chinamobile.com>
> >> >
> >> > These functions mix up -1 and -errno in return values and would might cause
> >> > trouble error handling in the call chain.
> >> >
> >> > This patch let them return -errno and add some comments.
> >> >
> >> > Reported-by: Markus Armbruster <armbru@redhat.com>
> >> > Signed-off-by: Liu Yuan <liuyuan@cmss.chinamobile.com>
> >> 
> >> Did you review all functions returning negative errno, or just the two
> >> that caught my eye?
> >
> > Umm, mostly the two you mentioned.
> 
> I encourage you to review the whole file for this error pattern!  In my
> experience, bugs occur in clusters.  When you find one, there's a high
> chance similar ones exist, and looking for them is good practice.

Hi Markus, I've checked the whole file as possible as I can. I can't find more
ret mixing bugs.

Yuan

Re: [Qemu-devel] [PATCH] sheepdog: fix confused return values

[Markus Armbruster]

Liu Yuan <namei.unix@gmail.com> writes:

> On Wed, Feb 25, 2015 at 10:35:17AM +0100, Markus Armbruster wrote:
>> Liu Yuan <namei.unix@gmail.com> writes:
>> 
>> > On Wed, Feb 18, 2015 at 09:35:07AM +0100, Markus Armbruster wrote:
>> >> Liu Yuan <namei.unix@gmail.com> writes:
>> >> 
>> >> > From: Liu Yuan <liuyuan@cmss.chinamobile.com>
>> >> >
>> >> > These functions mix up -1 and -errno in return values and would
>> >> > might cause
>> >> > trouble error handling in the call chain.
>> >> >
>> >> > This patch let them return -errno and add some comments.
>> >> >
>> >> > Reported-by: Markus Armbruster <armbru@redhat.com>
>> >> > Signed-off-by: Liu Yuan <liuyuan@cmss.chinamobile.com>
>> >> 
>> >> Did you review all functions returning negative errno, or just the two
>> >> that caught my eye?
>> >
>> > Umm, mostly the two you mentioned.
>> 
>> I encourage you to review the whole file for this error pattern!  In my
>> experience, bugs occur in clusters.  When you find one, there's a high
>> chance similar ones exist, and looking for them is good practice.
>
> Hi Markus, I've checked the whole file as possible as I can. I can't find more
> ret mixing bugs.

Thanks!