| Message ID | 20191001155319.8066-2-vsementsov@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
| Series | error: auto propagated local_err | expand |
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes: > Error **errp is almost always OUT-argument: it's assumed to be NULL, or > pointer to NULL-initialized pointer, or pointer to error_abort or > error_fatal, for callee to report error. Yes. > But very few functions instead get Error **errp as IN-argument: > it's assumed to be set, and callee should clean it. What do you mean by "callee should clean"? Let's see below. > In such cases, rename errp to errp_in. I acknowledge that errp arguments that don't have the usual meaning can be confusing. Naming can help, comments can help, but perhaps we can tweak the code to avoid the problem instead. Let's see: > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > Reviewed-by: Eric Blake <eblake@redhat.com> > --- > include/monitor/hmp.h | 2 +- > include/qapi/error.h | 2 +- > ui/vnc.h | 2 +- > monitor/hmp-cmds.c | 8 ++++---- > ui/vnc.c | 10 +++++----- > util/error.c | 8 ++++---- > 6 files changed, 16 insertions(+), 16 deletions(-) > > diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h > index a0e9511440..f929814f1a 100644 > --- a/include/monitor/hmp.h > +++ b/include/monitor/hmp.h > @@ -16,7 +16,7 @@ > > #include "qemu/readline.h" > > -void hmp_handle_error(Monitor *mon, Error **errp); > +void hmp_handle_error(Monitor *mon, Error **errp_in); > > void hmp_info_name(Monitor *mon, const QDict *qdict); > void hmp_info_version(Monitor *mon, const QDict *qdict); > diff --git a/include/qapi/error.h b/include/qapi/error.h > index 3f95141a01..9376f59c35 100644 > --- a/include/qapi/error.h > +++ b/include/qapi/error.h > @@ -283,7 +283,7 @@ void error_free(Error *err); > /* > * Convenience function to assert that *@errp is set, then silently free it. > */ > -void error_free_or_abort(Error **errp); > +void error_free_or_abort(Error **errp_in); > > /* > * Convenience function to warn_report() and free @err. > diff --git a/ui/vnc.h b/ui/vnc.h > index fea79c2fc9..00e0b48f2f 100644 > --- a/ui/vnc.h > +++ b/ui/vnc.h > @@ -547,7 +547,7 @@ uint32_t read_u32(uint8_t *data, size_t offset); > > /* Protocol stage functions */ > void vnc_client_error(VncState *vs); > -size_t vnc_client_io_error(VncState *vs, ssize_t ret, Error **errp); > +size_t vnc_client_io_error(VncState *vs, ssize_t ret, Error **errp_in); > > void start_client_init(VncState *vs); > void start_auth_vnc(VncState *vs); > diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c > index b2551c16d1..941d5d0a45 100644 > --- a/monitor/hmp-cmds.c > +++ b/monitor/hmp-cmds.c > @@ -60,11 +60,11 @@ > #include <spice/enums.h> > #endif > > -void hmp_handle_error(Monitor *mon, Error **errp) > +void hmp_handle_error(Monitor *mon, Error **errp_in) > { > - assert(errp); > - if (*errp) { > - error_reportf_err(*errp, "Error: "); > + assert(errp_in); > + if (*errp_in) { > + error_reportf_err(*errp_in, "Error: "); > } > } This functions frees the error. It leaves nothing for the caller to clean up. All callers pass &ERR, where ERR is a local variable. Perhaps a more robust way to signal "@errp is not the usual out-argument" would be peeling off an indirection: pass ERR, drop the assertion. > > diff --git a/ui/vnc.c b/ui/vnc.c > index 87b8045afe..9d6384d9b1 100644 > --- a/ui/vnc.c > +++ b/ui/vnc.c > @@ -1312,7 +1312,7 @@ void vnc_disconnect_finish(VncState *vs) > g_free(vs); > } > > -size_t vnc_client_io_error(VncState *vs, ssize_t ret, Error **errp) > +size_t vnc_client_io_error(VncState *vs, ssize_t ret, Error **errp_in) > { > if (ret <= 0) { > if (ret == 0) { > @@ -1320,14 +1320,14 @@ size_t vnc_client_io_error(VncState *vs, ssize_t ret, Error **errp) > vnc_disconnect_start(vs); > } else if (ret != QIO_CHANNEL_ERR_BLOCK) { > trace_vnc_client_io_error(vs, vs->ioc, > - errp ? error_get_pretty(*errp) : > + errp_in ? error_get_pretty(*errp_in) : > "Unknown"); > vnc_disconnect_start(vs); > } > > - if (errp) { > - error_free(*errp); > - *errp = NULL; > + if (errp_in) { > + error_free(*errp_in); > + *errp_in = NULL; > } > return 0; > } This function isn't trivial, and lacks a contract, so let's figure out what it does and how it's used. @ret can be: * Zero Trace EOF, call vnc_disconnect_start(), free the error, return zero. Aside: freeing the error without looking at it feels odd. Can this happen? * Negative other than QIO_CHANNEL_ERR_BLOCK Trace the error if any, else "Unknown" error, call vnc_disconnect_start(), free the error if any, return zero. Note that we can't have errp && !*errp here, or else tracing crashes in error_get_pretty(). * QIO_CHANNEL_ERR_BLOCK Free the error, return zero * Positive Do nothing, return @ret Callers pass one of the following: * ret = -1 and errp = NULL This uses case "Negative other than QIO_CHANNEL_ERR_BLOCK". Since error is null, it traces an "Unknown" error. * ret and &err, where ret = FUN(..., &err), and FUN is qio_channel_read() or qio_channel_write(). qio_channel_read(), _write() are documented to return non-negative on success, QIO_CHANNEL_ERR_BLOCK on "would block", and -1 on other error. By convention, they set an error exactly when they fail, i.e. when they return a negative value. When qio_channel_read() / _write() succeed, we use case "Positive" or "Zero". We don't free the error, which is fine, as none was returned. Aside: I *guess* the channel is non-blocking, and "zero" can happen only when read hits EOF. When qio_channel_read() / _write() fail, we use one of the error cases. Looks like vnc_client_io_error() takes an error code @ret and an optional error object in @errp with additional details. If @ret is non-negative, @errp must be null or point to null. If @ret is negative, @errp must be null or point to non-null. vnc_client_io_error() frees the error. It leaves nothing for the caller to clean up. I think we can again peel off an indirection. The two kinds of calls become: * ret = -1 and err = NULL No textual change, but the NULL gets converted to Error * instead of Error **. * ret and err Pass the (possibly null) error object instead of a pointer to the local variable. > diff --git a/util/error.c b/util/error.c > index d4532ce318..b3ff3832d6 100644 > --- a/util/error.c > +++ b/util/error.c > @@ -271,11 +271,11 @@ void error_free(Error *err) > } > } > > -void error_free_or_abort(Error **errp) > +void error_free_or_abort(Error **errp_in) > { > - assert(errp && *errp); > - error_free(*errp); > - *errp = NULL; > + assert(errp_in && *errp_in); > + error_free(*errp_in); > + *errp_in = NULL; > } > > void error_propagate(Error **dst_errp, Error *local_err) This functions frees the error. It leaves nothing for the caller to clean up. All callers pass &ERR, where ERR is a local variable. We can peel off an indirection. I figure your commit message's "But very few functions instead get Error **errp as IN-argument: it's assumed to be set, and callee should clean it" is to be read as "a few functions take Error **errp as IN-argument, and free it". You found three instances of confusing Error **errp. How? I'm asking because I wonder whether there are more. We can avoid the confusing Error **errp in all three cases by peeling off an indirection. What do you think?
08.10.2019 12:08, Markus Armbruster wrote: > Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes: > >> Error **errp is almost always OUT-argument: it's assumed to be NULL, or >> pointer to NULL-initialized pointer, or pointer to error_abort or >> error_fatal, for callee to report error. > > Yes. > >> But very few functions instead get Error **errp as IN-argument: >> it's assumed to be set, and callee should clean it. > > What do you mean by "callee should clean"? Let's see below. > >> In such cases, rename errp to errp_in. > > I acknowledge that errp arguments that don't have the usual meaning can > be confusing. > > Naming can help, comments can help, but perhaps we can tweak the code to > avoid the problem instead. Let's see: > >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >> Reviewed-by: Eric Blake <eblake@redhat.com> >> --- >> include/monitor/hmp.h | 2 +- >> include/qapi/error.h | 2 +- >> ui/vnc.h | 2 +- >> monitor/hmp-cmds.c | 8 ++++---- >> ui/vnc.c | 10 +++++----- >> util/error.c | 8 ++++---- >> 6 files changed, 16 insertions(+), 16 deletions(-) >> >> diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h >> index a0e9511440..f929814f1a 100644 >> --- a/include/monitor/hmp.h >> +++ b/include/monitor/hmp.h >> @@ -16,7 +16,7 @@ >> >> #include "qemu/readline.h" >> >> -void hmp_handle_error(Monitor *mon, Error **errp); >> +void hmp_handle_error(Monitor *mon, Error **errp_in); >> >> void hmp_info_name(Monitor *mon, const QDict *qdict); >> void hmp_info_version(Monitor *mon, const QDict *qdict); >> diff --git a/include/qapi/error.h b/include/qapi/error.h >> index 3f95141a01..9376f59c35 100644 >> --- a/include/qapi/error.h >> +++ b/include/qapi/error.h >> @@ -283,7 +283,7 @@ void error_free(Error *err); >> /* >> * Convenience function to assert that *@errp is set, then silently free it. >> */ >> -void error_free_or_abort(Error **errp); >> +void error_free_or_abort(Error **errp_in); >> >> /* >> * Convenience function to warn_report() and free @err. >> diff --git a/ui/vnc.h b/ui/vnc.h >> index fea79c2fc9..00e0b48f2f 100644 >> --- a/ui/vnc.h >> +++ b/ui/vnc.h >> @@ -547,7 +547,7 @@ uint32_t read_u32(uint8_t *data, size_t offset); >> >> /* Protocol stage functions */ >> void vnc_client_error(VncState *vs); >> -size_t vnc_client_io_error(VncState *vs, ssize_t ret, Error **errp); >> +size_t vnc_client_io_error(VncState *vs, ssize_t ret, Error **errp_in); >> >> void start_client_init(VncState *vs); >> void start_auth_vnc(VncState *vs); >> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c >> index b2551c16d1..941d5d0a45 100644 >> --- a/monitor/hmp-cmds.c >> +++ b/monitor/hmp-cmds.c >> @@ -60,11 +60,11 @@ >> #include <spice/enums.h> >> #endif >> >> -void hmp_handle_error(Monitor *mon, Error **errp) >> +void hmp_handle_error(Monitor *mon, Error **errp_in) >> { >> - assert(errp); >> - if (*errp) { >> - error_reportf_err(*errp, "Error: "); >> + assert(errp_in); >> + if (*errp_in) { >> + error_reportf_err(*errp_in, "Error: "); >> } >> } > > This functions frees the error. It leaves nothing for the caller to > clean up. > > All callers pass &ERR, where ERR is a local variable. Perhaps a more > robust way to signal "@errp is not the usual out-argument" would be > peeling off an indirection: pass ERR, drop the assertion. > >> >> diff --git a/ui/vnc.c b/ui/vnc.c >> index 87b8045afe..9d6384d9b1 100644 >> --- a/ui/vnc.c >> +++ b/ui/vnc.c >> @@ -1312,7 +1312,7 @@ void vnc_disconnect_finish(VncState *vs) >> g_free(vs); >> } >> >> -size_t vnc_client_io_error(VncState *vs, ssize_t ret, Error **errp) >> +size_t vnc_client_io_error(VncState *vs, ssize_t ret, Error **errp_in) >> { >> if (ret <= 0) { >> if (ret == 0) { >> @@ -1320,14 +1320,14 @@ size_t vnc_client_io_error(VncState *vs, ssize_t ret, Error **errp) >> vnc_disconnect_start(vs); >> } else if (ret != QIO_CHANNEL_ERR_BLOCK) { >> trace_vnc_client_io_error(vs, vs->ioc, >> - errp ? error_get_pretty(*errp) : >> + errp_in ? error_get_pretty(*errp_in) : >> "Unknown"); >> vnc_disconnect_start(vs); >> } >> >> - if (errp) { >> - error_free(*errp); >> - *errp = NULL; >> + if (errp_in) { >> + error_free(*errp_in); >> + *errp_in = NULL; >> } >> return 0; >> } > > This function isn't trivial, and lacks a contract, so let's figure out > what it does and how it's used. > > @ret can be: > > * Zero > > Trace EOF, call vnc_disconnect_start(), free the error, return zero. > > Aside: freeing the error without looking at it feels odd. Can this > happen? > > * Negative other than QIO_CHANNEL_ERR_BLOCK > > Trace the error if any, else "Unknown" error, call > vnc_disconnect_start(), free the error if any, return zero. > > Note that we can't have errp && !*errp here, or else tracing crashes > in error_get_pretty(). > > * QIO_CHANNEL_ERR_BLOCK > > Free the error, return zero > > * Positive > > Do nothing, return @ret > > Callers pass one of the following: > > * ret = -1 and errp = NULL > > This uses case "Negative other than QIO_CHANNEL_ERR_BLOCK". Since > error is null, it traces an "Unknown" error. > > * ret and &err, where ret = FUN(..., &err), and FUN is > qio_channel_read() or qio_channel_write(). > > qio_channel_read(), _write() are documented to return non-negative on > success, QIO_CHANNEL_ERR_BLOCK on "would block", and -1 on other > error. By convention, they set an error exactly when they fail, > i.e. when they return a negative value. > > When qio_channel_read() / _write() succeed, we use case "Positive" or > "Zero". We don't free the error, which is fine, as none was returned. > Aside: I *guess* the channel is non-blocking, and "zero" can happen > only when read hits EOF. > > When qio_channel_read() / _write() fail, we use one of the error > cases. > > Looks like vnc_client_io_error() takes an error code @ret and an > optional error object in @errp with additional details. If @ret is > non-negative, @errp must be null or point to null. If @ret is negative, > @errp must be null or point to non-null. > > vnc_client_io_error() frees the error. It leaves nothing for the caller > to clean up. > > I think we can again peel off an indirection. The two kinds of calls > become: > > * ret = -1 and err = NULL > > No textual change, but the NULL gets converted to Error * instead of > Error **. > > * ret and err > > Pass the (possibly null) error object instead of a pointer to the > local variable. > >> diff --git a/util/error.c b/util/error.c >> index d4532ce318..b3ff3832d6 100644 >> --- a/util/error.c >> +++ b/util/error.c >> @@ -271,11 +271,11 @@ void error_free(Error *err) >> } >> } >> >> -void error_free_or_abort(Error **errp) >> +void error_free_or_abort(Error **errp_in) >> { >> - assert(errp && *errp); >> - error_free(*errp); >> - *errp = NULL; >> + assert(errp_in && *errp_in); >> + error_free(*errp_in); >> + *errp_in = NULL; >> } >> >> void error_propagate(Error **dst_errp, Error *local_err) > > This functions frees the error. It leaves nothing for the caller to > clean up. > > All callers pass &ERR, where ERR is a local variable. We can peel off > an indirection. > > > I figure your commit message's "But very few functions instead get Error > **errp as IN-argument: it's assumed to be set, and callee should clean > it" is to be read as "a few functions take Error **errp as IN-argument, > and free it". > > You found three instances of confusing Error **errp. How? I'm asking > because I wonder whether there are more. I don' remember exactly, but assume that I found them by compilation errors after automatic conversion. > > We can avoid the confusing Error **errp in all three cases by peeling > off an indirection. What do you think? > I like the idea, thanks! I think, I'll try to make a patch. But you are right, unfortunately there more cases, at least, pointed by Greg error_append_socket_sockfd_hint and error_append_security_model_hint Which don't free error.. But if they append hint, they should always be called on wrapped errp, accordingly to the problem about fatal_error, so they may be converted to Error *err too.. Hmm, I should think about the script to find such functions.
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes: > 08.10.2019 12:08, Markus Armbruster wrote: >> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes: >> >>> Error **errp is almost always OUT-argument: it's assumed to be NULL, or >>> pointer to NULL-initialized pointer, or pointer to error_abort or >>> error_fatal, for callee to report error. >> >> Yes. >> >>> But very few functions instead get Error **errp as IN-argument: >>> it's assumed to be set, and callee should clean it. >> >> What do you mean by "callee should clean"? Let's see below. >> >>> In such cases, rename errp to errp_in. >> >> I acknowledge that errp arguments that don't have the usual meaning can >> be confusing. >> >> Naming can help, comments can help, but perhaps we can tweak the code to >> avoid the problem instead. Let's see: >> >>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >>> Reviewed-by: Eric Blake <eblake@redhat.com> [...] >> We can avoid the confusing Error **errp in all three cases by peeling >> off an indirection. What do you think? >> > > I like the idea, thanks! I think, I'll try to make a patch. > > But you are right, unfortunately there more cases, at least, pointed by > Greg > > error_append_socket_sockfd_hint and > error_append_security_model_hint > > Which don't free error.. Neither do error_append_hint() and error_prepend(). For anything named error_append_FOO_hint() or error_prepend_FOO(), confusion seems unlikely. > But if they append hint, they should always be called > on wrapped errp, accordingly to the problem about fatal_error, so they may > be converted to Error *err too.. Hmm, I should think about the script to find > such functions. I figure I better read more of your series before I comment on this thought.
08.10.2019 12:08, Markus Armbruster wrote: > Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes: > >> Error **errp is almost always OUT-argument: it's assumed to be NULL, or >> pointer to NULL-initialized pointer, or pointer to error_abort or >> error_fatal, for callee to report error. > > Yes. > >> But very few functions instead get Error **errp as IN-argument: >> it's assumed to be set, and callee should clean it. > > What do you mean by "callee should clean"? Let's see below. > >> In such cases, rename errp to errp_in. > > I acknowledge that errp arguments that don't have the usual meaning can > be confusing. > > Naming can help, comments can help, but perhaps we can tweak the code to > avoid the problem instead. Let's see: > >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >> Reviewed-by: Eric Blake <eblake@redhat.com> >> --- >> include/monitor/hmp.h | 2 +- >> include/qapi/error.h | 2 +- >> ui/vnc.h | 2 +- >> monitor/hmp-cmds.c | 8 ++++---- >> ui/vnc.c | 10 +++++----- >> util/error.c | 8 ++++---- >> 6 files changed, 16 insertions(+), 16 deletions(-) >> >> diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h >> index a0e9511440..f929814f1a 100644 >> --- a/include/monitor/hmp.h >> +++ b/include/monitor/hmp.h >> @@ -16,7 +16,7 @@ >> >> #include "qemu/readline.h" >> >> -void hmp_handle_error(Monitor *mon, Error **errp); >> +void hmp_handle_error(Monitor *mon, Error **errp_in); >> >> void hmp_info_name(Monitor *mon, const QDict *qdict); >> void hmp_info_version(Monitor *mon, const QDict *qdict); >> diff --git a/include/qapi/error.h b/include/qapi/error.h >> index 3f95141a01..9376f59c35 100644 >> --- a/include/qapi/error.h >> +++ b/include/qapi/error.h >> @@ -283,7 +283,7 @@ void error_free(Error *err); >> /* >> * Convenience function to assert that *@errp is set, then silently free it. >> */ >> -void error_free_or_abort(Error **errp); >> +void error_free_or_abort(Error **errp_in); >> >> /* >> * Convenience function to warn_report() and free @err. >> diff --git a/ui/vnc.h b/ui/vnc.h >> index fea79c2fc9..00e0b48f2f 100644 >> --- a/ui/vnc.h >> +++ b/ui/vnc.h >> @@ -547,7 +547,7 @@ uint32_t read_u32(uint8_t *data, size_t offset); >> >> /* Protocol stage functions */ >> void vnc_client_error(VncState *vs); >> -size_t vnc_client_io_error(VncState *vs, ssize_t ret, Error **errp); >> +size_t vnc_client_io_error(VncState *vs, ssize_t ret, Error **errp_in); >> >> void start_client_init(VncState *vs); >> void start_auth_vnc(VncState *vs); >> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c >> index b2551c16d1..941d5d0a45 100644 >> --- a/monitor/hmp-cmds.c >> +++ b/monitor/hmp-cmds.c >> @@ -60,11 +60,11 @@ >> #include <spice/enums.h> >> #endif >> >> -void hmp_handle_error(Monitor *mon, Error **errp) >> +void hmp_handle_error(Monitor *mon, Error **errp_in) >> { >> - assert(errp); >> - if (*errp) { >> - error_reportf_err(*errp, "Error: "); >> + assert(errp_in); >> + if (*errp_in) { >> + error_reportf_err(*errp_in, "Error: "); >> } >> } > > This functions frees the error. It leaves nothing for the caller to > clean up. > > All callers pass &ERR, where ERR is a local variable. Perhaps a more > robust way to signal "@errp is not the usual out-argument" would be > peeling off an indirection: pass ERR, drop the assertion. > >> >> diff --git a/ui/vnc.c b/ui/vnc.c >> index 87b8045afe..9d6384d9b1 100644 >> --- a/ui/vnc.c >> +++ b/ui/vnc.c >> @@ -1312,7 +1312,7 @@ void vnc_disconnect_finish(VncState *vs) >> g_free(vs); >> } >> >> -size_t vnc_client_io_error(VncState *vs, ssize_t ret, Error **errp) >> +size_t vnc_client_io_error(VncState *vs, ssize_t ret, Error **errp_in) >> { >> if (ret <= 0) { >> if (ret == 0) { >> @@ -1320,14 +1320,14 @@ size_t vnc_client_io_error(VncState *vs, ssize_t ret, Error **errp) >> vnc_disconnect_start(vs); >> } else if (ret != QIO_CHANNEL_ERR_BLOCK) { >> trace_vnc_client_io_error(vs, vs->ioc, >> - errp ? error_get_pretty(*errp) : >> + errp_in ? error_get_pretty(*errp_in) : >> "Unknown"); >> vnc_disconnect_start(vs); >> } >> >> - if (errp) { >> - error_free(*errp); >> - *errp = NULL; >> + if (errp_in) { >> + error_free(*errp_in); >> + *errp_in = NULL; >> } >> return 0; >> } > > This function isn't trivial, and lacks a contract, so let's figure out > what it does and how it's used. > > @ret can be: > > * Zero > > Trace EOF, call vnc_disconnect_start(), free the error, return zero. > > Aside: freeing the error without looking at it feels odd. Can this > happen? > > * Negative other than QIO_CHANNEL_ERR_BLOCK > > Trace the error if any, else "Unknown" error, call > vnc_disconnect_start(), free the error if any, return zero. > > Note that we can't have errp && !*errp here, or else tracing crashes > in error_get_pretty(). > > * QIO_CHANNEL_ERR_BLOCK > > Free the error, return zero > > * Positive > > Do nothing, return @ret > > Callers pass one of the following: > > * ret = -1 and errp = NULL > > This uses case "Negative other than QIO_CHANNEL_ERR_BLOCK". Since > error is null, it traces an "Unknown" error. > > * ret and &err, where ret = FUN(..., &err), and FUN is > qio_channel_read() or qio_channel_write(). > > qio_channel_read(), _write() are documented to return non-negative on > success, QIO_CHANNEL_ERR_BLOCK on "would block", and -1 on other > error. By convention, they set an error exactly when they fail, > i.e. when they return a negative value. > > When qio_channel_read() / _write() succeed, we use case "Positive" or > "Zero". We don't free the error, which is fine, as none was returned. > Aside: I *guess* the channel is non-blocking, and "zero" can happen > only when read hits EOF. > > When qio_channel_read() / _write() fail, we use one of the error > cases. > > Looks like vnc_client_io_error() takes an error code @ret and an > optional error object in @errp with additional details. If @ret is > non-negative, @errp must be null or point to null. If @ret is negative, > @errp must be null or point to non-null. > > vnc_client_io_error() frees the error. It leaves nothing for the caller > to clean up. > > I think we can again peel off an indirection. The two kinds of calls > become: > > * ret = -1 and err = NULL > > No textual change, but the NULL gets converted to Error * instead of > Error **. > > * ret and err > > Pass the (possibly null) error object instead of a pointer to the > local variable. > >> diff --git a/util/error.c b/util/error.c >> index d4532ce318..b3ff3832d6 100644 >> --- a/util/error.c >> +++ b/util/error.c >> @@ -271,11 +271,11 @@ void error_free(Error *err) >> } >> } >> >> -void error_free_or_abort(Error **errp) >> +void error_free_or_abort(Error **errp_in) >> { >> - assert(errp && *errp); >> - error_free(*errp); >> - *errp = NULL; >> + assert(errp_in && *errp_in); >> + error_free(*errp_in); >> + *errp_in = NULL; >> } >> >> void error_propagate(Error **dst_errp, Error *local_err) > > This functions frees the error. It leaves nothing for the caller to > clean up. > > All callers pass &ERR, where ERR is a local variable. We can peel off > an indirection. But if we drop indirection, we'll have to set local variable to NULL by hand. Is it good? Look at test_keyval_parse_list() for example: it uses local err object several times, so it depends on the fact that error_free_or_abort sets pointer to NULL.
08.10.2019 15:05, Markus Armbruster wrote: > Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes: > >> 08.10.2019 12:08, Markus Armbruster wrote: >>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes: >>> >>>> Error **errp is almost always OUT-argument: it's assumed to be NULL, or >>>> pointer to NULL-initialized pointer, or pointer to error_abort or >>>> error_fatal, for callee to report error. >>> >>> Yes. >>> >>>> But very few functions instead get Error **errp as IN-argument: >>>> it's assumed to be set, and callee should clean it. >>> >>> What do you mean by "callee should clean"? Let's see below. >>> >>>> In such cases, rename errp to errp_in. >>> >>> I acknowledge that errp arguments that don't have the usual meaning can >>> be confusing. >>> >>> Naming can help, comments can help, but perhaps we can tweak the code to >>> avoid the problem instead. Let's see: >>> >>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >>>> Reviewed-by: Eric Blake <eblake@redhat.com> > [...] >>> We can avoid the confusing Error **errp in all three cases by peeling >>> off an indirection. What do you think? >>> >> >> I like the idea, thanks! I think, I'll try to make a patch. >> >> But you are right, unfortunately there more cases, at least, pointed by >> Greg >> >> error_append_socket_sockfd_hint and >> error_append_security_model_hint >> >> Which don't free error.. > > Neither do error_append_hint() and error_prepend(). > > For anything named error_append_FOO_hint() or error_prepend_FOO(), > confusion seems unlikely. > >> But if they append hint, they should always be called >> on wrapped errp, accordingly to the problem about fatal_error, so they may >> be converted to Error *err too.. Hmm, I should think about the script to find >> such functions. > > I figure I better read more of your series before I comment on this > thought. > Me trying to find more such functions: script: # cat ../up-fix-error_append_hint/find.py #!/usr/bin/env python import re import sys ret_type = r'^[^{};#]+( |\*|\*\*)' name = r'(?P<name>\w+)' args = r'\([^{};#]*Error \*\*errp[^{};#]*\)' body_before_errp = r'((?<!errp)[^}]|(?<!^)})*' caller = '(if ?|assert|' \ 'error_(v?prepend|get_pretty|append_hint|free|free_or_abort)|' \ '(warn|error)_reportf?_err)' # Match 'caller(errp', 'caller(*errp', 'errp ?' access_errp = '(' + caller + r'\(\*?errp|errp \?)' r = re.compile(ret_type + name + args + '\s*^\{' + body_before_errp + access_errp, re.M) with open(sys.argv[1]) as f: text = f.read() for m in r.finditer(text): print(m.groupdict()['name']) search: # git ls-files | grep '\.\(h\|c\)$' | while read f; do ../up-fix-error_append_hint/find.py $f; done vmdk_co_create_opts_cb error_append_security_model_hint error_append_socket_sockfd_hint qemu_file_get_error_obj hmp_handle_error qbus_list_bus qbus_list_dev kvmppc_hint_smt_possible vnc_client_io_error error_handle_fatal error_setv error_prepend error_setg_win32_internal error_free_or_abort vmdk_co_create_opts_cb and qemu_file_get_error_obj are false positives I think
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes: > 08.10.2019 15:05, Markus Armbruster wrote: >> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes: >> >>> 08.10.2019 12:08, Markus Armbruster wrote: >>>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes: >>>> >>>>> Error **errp is almost always OUT-argument: it's assumed to be NULL, or >>>>> pointer to NULL-initialized pointer, or pointer to error_abort or >>>>> error_fatal, for callee to report error. >>>> >>>> Yes. >>>> >>>>> But very few functions instead get Error **errp as IN-argument: >>>>> it's assumed to be set, and callee should clean it. >>>> >>>> What do you mean by "callee should clean"? Let's see below. >>>> >>>>> In such cases, rename errp to errp_in. >>>> >>>> I acknowledge that errp arguments that don't have the usual meaning can >>>> be confusing. >>>> >>>> Naming can help, comments can help, but perhaps we can tweak the code to >>>> avoid the problem instead. Let's see: >>>> >>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >>>>> Reviewed-by: Eric Blake <eblake@redhat.com> >> [...] >>>> We can avoid the confusing Error **errp in all three cases by peeling >>>> off an indirection. What do you think? >>>> >>> >>> I like the idea, thanks! I think, I'll try to make a patch. >>> >>> But you are right, unfortunately there more cases, at least, pointed by >>> Greg >>> >>> error_append_socket_sockfd_hint and >>> error_append_security_model_hint >>> >>> Which don't free error.. >> >> Neither do error_append_hint() and error_prepend(). >> >> For anything named error_append_FOO_hint() or error_prepend_FOO(), >> confusion seems unlikely. >> >>> But if they append hint, they should always be called >>> on wrapped errp, accordingly to the problem about fatal_error, so they may >>> be converted to Error *err too.. Hmm, I should think about the script to find >>> such functions. >> >> I figure I better read more of your series before I comment on this >> thought. >> > > Me trying to find more such functions: > > script: > # cat ../up-fix-error_append_hint/find.py > #!/usr/bin/env python > import re > import sys > > ret_type = r'^[^{};#]+( |\*|\*\*)' > name = r'(?P<name>\w+)' > args = r'\([^{};#]*Error \*\*errp[^{};#]*\)' > body_before_errp = r'((?<!errp)[^}]|(?<!^)})*' > > caller = '(if ?|assert|' \ > 'error_(v?prepend|get_pretty|append_hint|free|free_or_abort)|' \ > '(warn|error)_reportf?_err)' > > # Match 'caller(errp', 'caller(*errp', 'errp ?' > access_errp = '(' + caller + r'\(\*?errp|errp \?)' > > r = re.compile(ret_type + name + args + '\s*^\{' + body_before_errp + access_errp, re.M) > > with open(sys.argv[1]) as f: > text = f.read() > > for m in r.finditer(text): > print(m.groupdict()['name']) > > > search: > # git ls-files | grep '\.\(h\|c\)$' | while read f; do ../up-fix-error_append_hint/find.py $f; done > vmdk_co_create_opts_cb Forwards errp to vmdk_create_extent(). Also asserts errp == NULL, which looks suspicious. Not your problem. > error_append_security_model_hint > error_append_socket_sockfd_hint Convenience functions to append a canned hint with error_append_hint(). Their name makes confusion unlikely. > qemu_file_get_error_obj Returns an error object in an unusual way: error_copy() instead of error_setg(). Suspicious-looking qemu_file_set_error_obj() nearby: it either stores @err in @f, or reports it to stderr / current monitor. Not your problem. > hmp_handle_error Covered by your patch, already discussed. > qbus_list_bus > qbus_list_dev Convenience functions to append hints with error_append_hint(). Function names do not hint at that (pardon the pun). > kvmppc_hint_smt_possible Convenience function to append hints with error_append_hint(). Function name hints weakly. > vnc_client_io_error Covered by your patch, already discussed. > error_handle_fatal > error_setv > error_prepend > error_setg_win32_internal > error_free_or_abort Let's not worry about error.c itself. > vmdk_co_create_opts_cb and qemu_file_get_error_obj are false positives I think Agree.
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes: > 08.10.2019 12:08, Markus Armbruster wrote: >> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes: [...] >>> diff --git a/util/error.c b/util/error.c >>> index d4532ce318..b3ff3832d6 100644 >>> --- a/util/error.c >>> +++ b/util/error.c >>> @@ -271,11 +271,11 @@ void error_free(Error *err) >>> } >>> } >>> >>> -void error_free_or_abort(Error **errp) >>> +void error_free_or_abort(Error **errp_in) >>> { >>> - assert(errp && *errp); >>> - error_free(*errp); >>> - *errp = NULL; >>> + assert(errp_in && *errp_in); >>> + error_free(*errp_in); >>> + *errp_in = NULL; >>> } >>> >>> void error_propagate(Error **dst_errp, Error *local_err) >> >> This functions frees the error. It leaves nothing for the caller to >> clean up. >> >> All callers pass &ERR, where ERR is a local variable. We can peel off >> an indirection. > > > But if we drop indirection, we'll have to set local variable to NULL by > hand. Is it good? > > Look at test_keyval_parse_list() for example: it uses local err object > several times, so it depends on the fact that error_free_or_abort > sets pointer to NULL. You're right, peeling off the indirection would make error_free_or_abort() worse. It's a convenience function for tests. Confusion seems unlikely to me. Let's not worry about it.
diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h index a0e9511440..f929814f1a 100644 --- a/include/monitor/hmp.h +++ b/include/monitor/hmp.h @@ -16,7 +16,7 @@ #include "qemu/readline.h" -void hmp_handle_error(Monitor *mon, Error **errp); +void hmp_handle_error(Monitor *mon, Error **errp_in); void hmp_info_name(Monitor *mon, const QDict *qdict); void hmp_info_version(Monitor *mon, const QDict *qdict); diff --git a/include/qapi/error.h b/include/qapi/error.h index 3f95141a01..9376f59c35 100644 --- a/include/qapi/error.h +++ b/include/qapi/error.h @@ -283,7 +283,7 @@ void error_free(Error *err); /* * Convenience function to assert that *@errp is set, then silently free it. */ -void error_free_or_abort(Error **errp); +void error_free_or_abort(Error **errp_in); /* * Convenience function to warn_report() and free @err. diff --git a/ui/vnc.h b/ui/vnc.h index fea79c2fc9..00e0b48f2f 100644 --- a/ui/vnc.h +++ b/ui/vnc.h @@ -547,7 +547,7 @@ uint32_t read_u32(uint8_t *data, size_t offset); /* Protocol stage functions */ void vnc_client_error(VncState *vs); -size_t vnc_client_io_error(VncState *vs, ssize_t ret, Error **errp); +size_t vnc_client_io_error(VncState *vs, ssize_t ret, Error **errp_in); void start_client_init(VncState *vs); void start_auth_vnc(VncState *vs); diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c index b2551c16d1..941d5d0a45 100644 --- a/monitor/hmp-cmds.c +++ b/monitor/hmp-cmds.c @@ -60,11 +60,11 @@ #include <spice/enums.h> #endif -void hmp_handle_error(Monitor *mon, Error **errp) +void hmp_handle_error(Monitor *mon, Error **errp_in) { - assert(errp); - if (*errp) { - error_reportf_err(*errp, "Error: "); + assert(errp_in); + if (*errp_in) { + error_reportf_err(*errp_in, "Error: "); } } diff --git a/ui/vnc.c b/ui/vnc.c index 87b8045afe..9d6384d9b1 100644 --- a/ui/vnc.c +++ b/ui/vnc.c @@ -1312,7 +1312,7 @@ void vnc_disconnect_finish(VncState *vs) g_free(vs); } -size_t vnc_client_io_error(VncState *vs, ssize_t ret, Error **errp) +size_t vnc_client_io_error(VncState *vs, ssize_t ret, Error **errp_in) { if (ret <= 0) { if (ret == 0) { @@ -1320,14 +1320,14 @@ size_t vnc_client_io_error(VncState *vs, ssize_t ret, Error **errp) vnc_disconnect_start(vs); } else if (ret != QIO_CHANNEL_ERR_BLOCK) { trace_vnc_client_io_error(vs, vs->ioc, - errp ? error_get_pretty(*errp) : + errp_in ? error_get_pretty(*errp_in) : "Unknown"); vnc_disconnect_start(vs); } - if (errp) { - error_free(*errp); - *errp = NULL; + if (errp_in) { + error_free(*errp_in); + *errp_in = NULL; } return 0; } diff --git a/util/error.c b/util/error.c index d4532ce318..b3ff3832d6 100644 --- a/util/error.c +++ b/util/error.c @@ -271,11 +271,11 @@ void error_free(Error *err) } } -void error_free_or_abort(Error **errp) +void error_free_or_abort(Error **errp_in) { - assert(errp && *errp); - error_free(*errp); - *errp = NULL; + assert(errp_in && *errp_in); + error_free(*errp_in); + *errp_in = NULL; } void error_propagate(Error **dst_errp, Error *local_err)