Message ID | 1423799142-21893-1-git-send-email-namei.unix@gmail.com |
---|---|
State | New |
Headers | show |
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
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
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
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?
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
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.
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
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!
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) {