[libvirt] [PATCH] error: preserve errno when saving last error
It is common to see the sequence: virErrorPtr save_err = virSaveLastError(); // do cleanup virSetError(save_err); virFreeError(save_err); on cleanup paths. But for functions where it is desirable to return the errno that caused failure, this sequence can clobber that errno. virFreeError was already safe; this makes the other two functions in the sequence safe as well, assuming all goes well (on OOM, errno will be clobbered, but then again, save_err won't reflect the real error that happened, so you are no longer preserving the real situation - that's life with OOM). * src/util/virterror.c (virSaveLastError, virSetError): Preserve errno. --- Fixes an errno-clobbering situation pointed out here: https://www.redhat.com/archives/libvir-list/2011-July/msg01382.html src/util/virterror.c | 17 +++++++++++++---- 1 files changed, 13 insertions(+), 4 deletions(-) diff --git a/src/util/virterror.c b/src/util/virterror.c index 0c7698a..07f8b45 100644 --- a/src/util/virterror.c +++ b/src/util/virterror.c @@ -295,18 +295,24 @@ virGetLastError(void) * Can be used to re-set an old error, which may have been squashed by * other functions (like cleanup routines). * - * Returns 0 on success, 1 on failure + * Returns 0 on success, -1 on failure. Leaves errno unchanged. */ int virSetError(virErrorPtr newerr) { virErrorPtr err; + int saved_errno = errno; + int ret = -1; + err = virLastErrorObject(); if (!err) - return -1; + goto cleanup; virResetError(err); - return virCopyError(newerr, err); + ret = virCopyError(newerr, err); +cleanup: + errno = saved_errno; + return ret; } /** @@ -339,7 +345,8 @@ virCopyLastError(virErrorPtr to) /** * virSaveLastError: * - * Save the last error into a new error object. + * Save the last error into a new error object. On success, errno is + * unchanged; on failure, errno is ENOMEM. * * Returns a pointer to the copied error or NULL if allocation failed. * It is the caller's responsibility to free the error with @@ -349,11 +356,13 @@ virErrorPtr virSaveLastError(void) { virErrorPtr to; + int saved_errno = errno; if (VIR_ALLOC(to) < 0) return NULL; virCopyLastError(to); + errno = saved_errno; return to; } -- 1.7.4.4
On 07/20/2011 07:15 PM, Eric Blake wrote:
It is common to see the sequence:
virErrorPtr save_err = virSaveLastError(); // do cleanup virSetError(save_err); virFreeError(save_err);
on cleanup paths. But for functions where it is desirable to return the errno that caused failure, this sequence can clobber that errno. virFreeError was already safe; this makes the other two functions in the sequence safe as well, assuming all goes well (on OOM, errno will be clobbered, but then again, save_err won't reflect the real error that happened, so you are no longer preserving the real situation - that's life with OOM).
* src/util/virterror.c (virSaveLastError, virSetError): Preserve errno. ---
ACK.
On 07/20/2011 07:30 PM, Laine Stump wrote:
On 07/20/2011 07:15 PM, Eric Blake wrote:
It is common to see the sequence:
virErrorPtr save_err = virSaveLastError(); // do cleanup virSetError(save_err); virFreeError(save_err);
on cleanup paths. But for functions where it is desirable to return the errno that caused failure, this sequence can clobber that errno. virFreeError was already safe; this makes the other two functions in the sequence safe as well, assuming all goes well (on OOM, errno will be clobbered, but then again, save_err won't reflect the real error that happened, so you are no longer preserving the real situation - that's life with OOM).
* src/util/virterror.c (virSaveLastError, virSetError): Preserve errno. ---
ACK.
Thanks; pushed. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake -
Laine Stump