* [Qemu-devel] Several sheepdog functions mix up -1 and -errno in return values
@ 2015-02-12 14:01 Markus Armbruster
2015-02-13 3:45 ` [Qemu-devel] [PATCH] sheepdog: fix confused " Liu Yuan
0 siblings, 1 reply; 10+ messages in thread
From: Markus Armbruster @ 2015-02-12 14:01 UTC (permalink / raw)
To: mitake.hitoshi, namei.unix, sheepdog; +Cc: kwolf, qemu-devel
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* [Qemu-devel] [PATCH] sheepdog: fix confused return values
2015-02-12 14:01 [Qemu-devel] Several sheepdog functions mix up -1 and -errno in return values Markus Armbruster
@ 2015-02-13 3:45 ` Liu Yuan
2015-02-13 3:49 ` Liu Yuan
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Liu Yuan @ 2015-02-13 3:45 UTC (permalink / raw)
To: qemu-devel, armbru; +Cc: sheepdog
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH] sheepdog: fix confused return values
2015-02-13 3:45 ` [Qemu-devel] [PATCH] sheepdog: fix confused " Liu Yuan
@ 2015-02-13 3:49 ` Liu Yuan
2015-02-16 11:56 ` Kevin Wolf
2015-02-18 8:35 ` Markus Armbruster
2 siblings, 0 replies; 10+ messages in thread
From: Liu Yuan @ 2015-02-13 3:49 UTC (permalink / raw)
To: qemu-devel, armbru; +Cc: kwolf, sheepdog, Stefan Hajnoczi
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH] sheepdog: fix confused return values
2015-02-13 3:45 ` [Qemu-devel] [PATCH] sheepdog: fix confused " Liu Yuan
2015-02-13 3:49 ` Liu Yuan
@ 2015-02-16 11:56 ` Kevin Wolf
2015-02-18 3:52 ` Liu Yuan
2015-02-18 8:35 ` Markus Armbruster
2 siblings, 1 reply; 10+ messages in thread
From: Kevin Wolf @ 2015-02-16 11:56 UTC (permalink / raw)
To: Liu Yuan; +Cc: sheepdog, qemu-devel, armbru
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH] sheepdog: fix confused return values
2015-02-16 11:56 ` Kevin Wolf
@ 2015-02-18 3:52 ` Liu Yuan
0 siblings, 0 replies; 10+ messages in thread
From: Liu Yuan @ 2015-02-18 3:52 UTC (permalink / raw)
To: Kevin Wolf; +Cc: sheepdog, qemu-devel, armbru
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH] sheepdog: fix confused return values
2015-02-13 3:45 ` [Qemu-devel] [PATCH] sheepdog: fix confused " Liu Yuan
2015-02-13 3:49 ` Liu Yuan
2015-02-16 11:56 ` Kevin Wolf
@ 2015-02-18 8:35 ` Markus Armbruster
2015-02-25 1:39 ` Liu Yuan
2 siblings, 1 reply; 10+ messages in thread
From: Markus Armbruster @ 2015-02-18 8:35 UTC (permalink / raw)
To: Liu Yuan; +Cc: sheepdog, qemu-devel
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?
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH] sheepdog: fix confused return values
2015-02-18 8:35 ` Markus Armbruster
@ 2015-02-25 1:39 ` Liu Yuan
2015-02-25 9:35 ` Markus Armbruster
0 siblings, 1 reply; 10+ messages in thread
From: Liu Yuan @ 2015-02-25 1:39 UTC (permalink / raw)
To: Markus Armbruster; +Cc: sheepdog, qemu-devel
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH] sheepdog: fix confused return values
2015-02-25 1:39 ` Liu Yuan
@ 2015-02-25 9:35 ` Markus Armbruster
2015-02-26 3:54 ` Liu Yuan
0 siblings, 1 reply; 10+ messages in thread
From: Markus Armbruster @ 2015-02-25 9:35 UTC (permalink / raw)
To: Liu Yuan; +Cc: sheepdog, qemu-devel
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH] sheepdog: fix confused return values
2015-02-25 9:35 ` Markus Armbruster
@ 2015-02-26 3:54 ` Liu Yuan
2015-02-26 7:50 ` Markus Armbruster
0 siblings, 1 reply; 10+ messages in thread
From: Liu Yuan @ 2015-02-26 3:54 UTC (permalink / raw)
To: Markus Armbruster; +Cc: sheepdog, qemu-devel
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH] sheepdog: fix confused return values
2015-02-26 3:54 ` Liu Yuan
@ 2015-02-26 7:50 ` Markus Armbruster
0 siblings, 0 replies; 10+ messages in thread
From: Markus Armbruster @ 2015-02-26 7:50 UTC (permalink / raw)
To: Liu Yuan; +Cc: sheepdog, qemu-devel
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!
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2015-02-26 8:32 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-02-12 14:01 [Qemu-devel] Several sheepdog functions mix up -1 and -errno in return values Markus Armbruster
2015-02-13 3:45 ` [Qemu-devel] [PATCH] sheepdog: fix confused " Liu Yuan
2015-02-13 3:49 ` Liu Yuan
2015-02-16 11:56 ` Kevin Wolf
2015-02-18 3:52 ` Liu Yuan
2015-02-18 8:35 ` Markus Armbruster
2015-02-25 1:39 ` Liu Yuan
2015-02-25 9:35 ` Markus Armbruster
2015-02-26 3:54 ` Liu Yuan
2015-02-26 7:50 ` Markus Armbruster
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.