[libvirt] [PATCH v2] util: set OOM in virCopyLastError if error is not set
virCopyLastError is intended to be used after last error is set. However due to virLastErrorObject failures (very unlikely through as thread local error is allocated on first use) we can have zero fields in a copy as a result. In particular code field can be set to VIR_ERR_OK. In some places (qemu monitor, qemu agent and qemu migaration code for example) we use copy result as a flag and this leads to bugs. Let's set OOM-like error in copy in case of virLastErrorObject failures. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- Changes from v1: - check @to - set OMM error instead of using virErrorGenericFailure src/util/virerror.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/src/util/virerror.c b/src/util/virerror.c index f198f27..737ea92 100644 --- a/src/util/virerror.c +++ b/src/util/virerror.c @@ -366,19 +366,25 @@ virSetError(virErrorPtr newerr) * * One will need to free the result with virResetError() * - * Returns 0 if no error was found and the error code otherwise and -1 in case - * of parameter error. + * Returns error code or -1 in case of parameter error. */ int virCopyLastError(virErrorPtr to) { virErrorPtr err = virLastErrorObject(); + + if (!to) + return -1; + /* We can't guarantee caller has initialized it to zero */ memset(to, 0, sizeof(*to)); - if (err) + if (err) { virCopyError(err, to); - else - virResetError(to); + } else { + to->code = VIR_ERR_NO_MEMORY; + to->domain = VIR_FROM_NONE; + to->level = VIR_ERR_ERROR; + } return to->code; } -- 1.8.3.1
On 07/02/2018 07:33 AM, Nikolay Shirokovskiy wrote:
virCopyLastError is intended to be used after last error is set. However due to virLastErrorObject failures (very unlikely through as thread local error is allocated on first use) we can have zero fields in a copy as a result. In particular code field can be set to VIR_ERR_OK.
In some places (qemu monitor, qemu agent and qemu migaration code for example) we use copy result as a flag and this leads to bugs.
Let's set OOM-like error in copy in case of virLastErrorObject failures.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> ---
Changes from v1: - check @to - set OMM error instead of using virErrorGenericFailure
src/util/virerror.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-)
diff --git a/src/util/virerror.c b/src/util/virerror.c index f198f27..737ea92 100644 --- a/src/util/virerror.c +++ b/src/util/virerror.c @@ -366,19 +366,25 @@ virSetError(virErrorPtr newerr) * * One will need to free the result with virResetError() * - * Returns 0 if no error was found and the error code otherwise and -1 in case - * of parameter error. + * Returns error code or -1 in case of parameter error. */ int virCopyLastError(virErrorPtr to) { virErrorPtr err = virLastErrorObject(); + + if (!to) + return -1; + /* We can't guarantee caller has initialized it to zero */ memset(to, 0, sizeof(*to)); - if (err) + if (err) { virCopyError(err, to); - else - virResetError(to); + } else { + to->code = VIR_ERR_NO_MEMORY; + to->domain = VIR_FROM_NONE; + to->level = VIR_ERR_ERROR;
Should we do a VIR_FREE(to->message); so that nothing that was there before somehow remains... I don't think either agent or monitor "lastError" is reset until Dispose time. Beyond that, I think this works for this edge/odd/bad case... Reviewed-by: John Ferlan <jferlan@redhat.com> John
+ } return to->code; }
On 17.07.2018 01:38, John Ferlan wrote:
On 07/02/2018 07:33 AM, Nikolay Shirokovskiy wrote:
virCopyLastError is intended to be used after last error is set. However due to virLastErrorObject failures (very unlikely through as thread local error is allocated on first use) we can have zero fields in a copy as a result. In particular code field can be set to VIR_ERR_OK.
In some places (qemu monitor, qemu agent and qemu migaration code for example) we use copy result as a flag and this leads to bugs.
Let's set OOM-like error in copy in case of virLastErrorObject failures.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> ---
Changes from v1: - check @to - set OMM error instead of using virErrorGenericFailure
src/util/virerror.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-)
diff --git a/src/util/virerror.c b/src/util/virerror.c index f198f27..737ea92 100644 --- a/src/util/virerror.c +++ b/src/util/virerror.c @@ -366,19 +366,25 @@ virSetError(virErrorPtr newerr) * * One will need to free the result with virResetError() * - * Returns 0 if no error was found and the error code otherwise and -1 in case - * of parameter error. + * Returns error code or -1 in case of parameter error. */ int virCopyLastError(virErrorPtr to) { virErrorPtr err = virLastErrorObject(); + + if (!to) + return -1; + /* We can't guarantee caller has initialized it to zero */ memset(to, 0, sizeof(*to)); - if (err) + if (err) { virCopyError(err, to); - else - virResetError(to); + } else { + to->code = VIR_ERR_NO_MEMORY; + to->domain = VIR_FROM_NONE; + to->level = VIR_ERR_ERROR;
Should we do a VIR_FREE(to->message); so that nothing that was there before somehow remains... I don't think either agent or monitor> "lastError" is reset until Dispose time.
Won't hurt but probably will not be used by monitor or agent. If thread error is not allocated message is NULL upon return, after error is allocated we never hit this OOM branch anymore. Of course hypotetical client can bring @to with message already set so this a bit future proof. I think then we can leave reset and then set these 3 fields. Nikolay
Beyond that, I think this works for this edge/odd/bad case...
Reviewed-by: John Ferlan <jferlan@redhat.com>
John
+ } return to->code; }
+ } else { + to->code = VIR_ERR_NO_MEMORY; + to->domain = VIR_FROM_NONE; + to->level = VIR_ERR_ERROR;
Should we do a VIR_FREE(to->message); so that nothing that was there before somehow remains... I don't think either agent or monitor> "lastError" is reset until Dispose time.
Won't hurt but probably will not be used by monitor or agent. If thread error is not allocated message is NULL upon return, after error is allocated we never hit this OOM branch anymore. Of course hypotetical client can bring @to with message already set so this a bit future proof.
I think then we can leave reset and then set these 3 fields.
Nikolay
You have commit access and my R-by regardless of whether you add the VIR_FREE or not. I leave the rest to you ;-) John
On 17.07.2018 22:28, John Ferlan wrote:
+ } else { + to->code = VIR_ERR_NO_MEMORY; + to->domain = VIR_FROM_NONE; + to->level = VIR_ERR_ERROR;
Should we do a VIR_FREE(to->message); so that nothing that was there before somehow remains... I don't think either agent or monitor> "lastError" is reset until Dispose time.
Won't hurt but probably will not be used by monitor or agent. If thread error is not allocated message is NULL upon return, after error is allocated we never hit this OOM branch anymore. Of course hypotetical client can bring @to with message already set so this a bit future proof.
I think then we can leave reset and then set these 3 fields.
Nikolay
You have commit access and my R-by regardless of whether you add the VIR_FREE or not. I leave the rest to you ;-)
Thanx! Pushed with reset added. Nikolay
On 07/19/2018 04:15 AM, Nikolay Shirokovskiy wrote:
On 17.07.2018 22:28, John Ferlan wrote:
+ } else { + to->code = VIR_ERR_NO_MEMORY; + to->domain = VIR_FROM_NONE; + to->level = VIR_ERR_ERROR;
Should we do a VIR_FREE(to->message); so that nothing that was there before somehow remains... I don't think either agent or monitor> "lastError" is reset until Dispose time.
Won't hurt but probably will not be used by monitor or agent. If thread error is not allocated message is NULL upon return, after error is allocated we never hit this OOM branch anymore. Of course hypotetical client can bring @to with message already set so this a bit future proof.
I think then we can leave reset and then set these 3 fields.
Nikolay
You have commit access and my R-by regardless of whether you add the VIR_FREE or not. I leave the rest to you ;-)
Thanx! Pushed with reset added.
Nikolay
Oh no, you pushed a local .gnulib too! git log -p ... commit 1bff5bbe25eb7a7e7a4e0067c4ca7cbc1cb34999 Author: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> Date: Mon Jul 2 14:16:52 2018 +0300 ... diff --git a/.gnulib b/.gnulib index cdbf3d385a..d6397dde2e 160000 --- a/.gnulib +++ b/.gnulib @@ -1 +1 @@ -Subproject commit cdbf3d385a32ff904c96f20c26f3470bd8345248 +Subproject commit d6397dde2e127e246e3eeb5254a21f42cac783c8 diff --git a/src/util/virerror.c b/src/util/virerror.c index f198f27957..5f26d59777 100644 --- a/src/util/virerror.c +++ b/src/util/virerror.c ... John
On 19.07.2018 20:30, John Ferlan wrote:
On 07/19/2018 04:15 AM, Nikolay Shirokovskiy wrote:
On 17.07.2018 22:28, John Ferlan wrote:
+ } else { + to->code = VIR_ERR_NO_MEMORY; + to->domain = VIR_FROM_NONE; + to->level = VIR_ERR_ERROR;
Should we do a VIR_FREE(to->message); so that nothing that was there before somehow remains... I don't think either agent or monitor> "lastError" is reset until Dispose time.
Won't hurt but probably will not be used by monitor or agent. If thread error is not allocated message is NULL upon return, after error is allocated we never hit this OOM branch anymore. Of course hypotetical client can bring @to with message already set so this a bit future proof.
I think then we can leave reset and then set these 3 fields.
Nikolay
You have commit access and my R-by regardless of whether you add the VIR_FREE or not. I leave the rest to you ;-)
Thanx! Pushed with reset added.
Nikolay
Oh no, you pushed a local .gnulib too!
git log -p ... commit 1bff5bbe25eb7a7e7a4e0067c4ca7cbc1cb34999 Author: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> Date: Mon Jul 2 14:16:52 2018 +0300
... diff --git a/.gnulib b/.gnulib index cdbf3d385a..d6397dde2e 160000 --- a/.gnulib +++ b/.gnulib @@ -1 +1 @@ -Subproject commit cdbf3d385a32ff904c96f20c26f3470bd8345248 +Subproject commit d6397dde2e127e246e3eeb5254a21f42cac783c8 diff --git a/src/util/virerror.c b/src/util/virerror.c index f198f27957..5f26d59777 100644 --- a/src/util/virerror.c +++ b/src/util/virerror.c
...
Sorry, hope I'll be more careful in the future. Nikolay
participants (2)
-
John Ferlan -
Nikolay Shirokovskiy