[libvirt] [PATCH v2] vz: get additional error information from job correctly
Function logPrlEventErrorHelper is made void and now it only prints an information (if any) from event. If there is no addition information (i.e. an error returned when asked), we don't rewrite original error code with secondary one. Signed-off-by: Maxim Nestratov <mnestratov@virtuozzo.com> --- src/vz/vz_sdk.c | 33 ++++++++++++++++----------------- 1 file changed, 16 insertions(+), 17 deletions(-) diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c index 99c5d4a..54c56e9 100644 --- a/src/vz/vz_sdk.c +++ b/src/vz/vz_sdk.c @@ -99,19 +99,13 @@ logPrlErrorHelper(PRL_RESULT err, const char *filename, } \ } while (0) -static PRL_RESULT +static void logPrlEventErrorHelper(PRL_HANDLE event, const char *filename, const char *funcname, size_t linenr) { - PRL_RESULT ret, retCode; char *msg1 = NULL, *msg2 = NULL; PRL_UINT32 len = 0; - int err = -1; - if ((ret = PrlEvent_GetErrCode(event, &retCode))) { - logPrlError(ret); - return ret; - } PrlEvent_GetErrString(event, PRL_TRUE, PRL_FALSE, NULL, &len); @@ -130,13 +124,9 @@ logPrlEventErrorHelper(PRL_HANDLE event, const char *filename, virReportErrorHelper(VIR_FROM_THIS, VIR_ERR_INTERNAL_ERROR, filename, funcname, linenr, _("%s %s"), msg1, msg2); - err = 0; - cleanup: VIR_FREE(msg1); VIR_FREE(msg2); - - return err; } static PRL_RESULT @@ -146,12 +136,12 @@ getJobResultHelper(PRL_HANDLE job, unsigned int timeout, PRL_HANDLE *result, { PRL_RESULT ret, retCode; - if ((ret = PrlJob_Wait(job, timeout))) { + if (PRL_FAILED(ret = PrlJob_Wait(job, timeout))) { logPrlErrorHelper(ret, filename, funcname, linenr); goto cleanup; } - if ((ret = PrlJob_GetRetCode(job, &retCode))) { + if (PRL_FAILED(ret = PrlJob_GetRetCode(job, &retCode))) { logPrlErrorHelper(ret, filename, funcname, linenr); goto cleanup; } @@ -159,17 +149,26 @@ getJobResultHelper(PRL_HANDLE job, unsigned int timeout, PRL_HANDLE *result, if (retCode) { PRL_HANDLE err_handle; + ret = retCode; + /* Sometimes it's possible to get additional error info. */ - if ((ret = PrlJob_GetError(job, &err_handle))) { + if (PRL_FAILED(retCode = PrlJob_GetError(job, &err_handle))) { logPrlErrorHelper(ret, filename, funcname, linenr); + if (PRL_ERR_NO_DATA != retCode) + logPrlError(retCode); goto cleanup; } - if (logPrlEventErrorHelper(err_handle, filename, funcname, linenr)) - logPrlErrorHelper(retCode, filename, funcname, linenr); + if (PRL_FAILED(retCode = PrlEvent_GetErrCode(err_handle, &retCode))) { + logPrlErrorHelper(ret, filename, funcname, linenr); + if (PRL_ERR_NO_DATA != retCode) + logPrlError(retCode); + goto cleanup; + } + + logPrlEventErrorHelper(err_handle, filename, funcname, linenr); PrlHandle_Free(err_handle); - ret = retCode; } else { ret = PrlJob_GetResult(job, result); if (PRL_FAILED(ret)) { -- 2.4.3
On 11.06.2016 20:16, Maxim Nestratov wrote:
Function logPrlEventErrorHelper is made void and now it only prints an information (if any) from event. If there is no addition information (i.e. an error returned when asked), we don't rewrite original error code with secondary one.
I would mention the original motivation of the patch, that is returning error code of PrlEvent_GetErrCode. And motivation of checking for PRL_ERR_NO_DATA too.
Signed-off-by: Maxim Nestratov <mnestratov@virtuozzo.com> --- src/vz/vz_sdk.c | 33 ++++++++++++++++----------------- 1 file changed, 16 insertions(+), 17 deletions(-)
diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c index 99c5d4a..54c56e9 100644 --- a/src/vz/vz_sdk.c +++ b/src/vz/vz_sdk.c @@ -99,19 +99,13 @@ logPrlErrorHelper(PRL_RESULT err, const char *filename, } \ } while (0)
-static PRL_RESULT +static void logPrlEventErrorHelper(PRL_HANDLE event, const char *filename, const char *funcname, size_t linenr) { - PRL_RESULT ret, retCode; char *msg1 = NULL, *msg2 = NULL; PRL_UINT32 len = 0; - int err = -1;
- if ((ret = PrlEvent_GetErrCode(event, &retCode))) { - logPrlError(ret); - return ret; - }
PrlEvent_GetErrString(event, PRL_TRUE, PRL_FALSE, NULL, &len);
@@ -130,13 +124,9 @@ logPrlEventErrorHelper(PRL_HANDLE event, const char *filename, virReportErrorHelper(VIR_FROM_THIS, VIR_ERR_INTERNAL_ERROR, filename, funcname, linenr, _("%s %s"), msg1, msg2); - err = 0; - cleanup: VIR_FREE(msg1); VIR_FREE(msg2); - - return err; }
static PRL_RESULT @@ -146,12 +136,12 @@ getJobResultHelper(PRL_HANDLE job, unsigned int timeout, PRL_HANDLE *result, { PRL_RESULT ret, retCode;
- if ((ret = PrlJob_Wait(job, timeout))) { + if (PRL_FAILED(ret = PrlJob_Wait(job, timeout))) { logPrlErrorHelper(ret, filename, funcname, linenr); goto cleanup; }
- if ((ret = PrlJob_GetRetCode(job, &retCode))) { + if (PRL_FAILED(ret = PrlJob_GetRetCode(job, &retCode))) { logPrlErrorHelper(ret, filename, funcname, linenr); goto cleanup; } @@ -159,17 +149,26 @@ getJobResultHelper(PRL_HANDLE job, unsigned int timeout, PRL_HANDLE *result, if (retCode) { PRL_HANDLE err_handle;
+ ret = retCode; + /* Sometimes it's possible to get additional error info. */ - if ((ret = PrlJob_GetError(job, &err_handle))) { + if (PRL_FAILED(retCode = PrlJob_GetError(job, &err_handle))) { logPrlErrorHelper(ret, filename, funcname, linenr); + if (PRL_ERR_NO_DATA != retCode) + logPrlError(retCode); goto cleanup; }
- if (logPrlEventErrorHelper(err_handle, filename, funcname, linenr)) - logPrlErrorHelper(retCode, filename, funcname, linenr); + if (PRL_FAILED(retCode = PrlEvent_GetErrCode(err_handle, &retCode))) { + logPrlErrorHelper(ret, filename, funcname, linenr); + if (PRL_ERR_NO_DATA != retCode)
looks like this error code does not matter in case of PrlEvent_GetErrCode
+ logPrlError(retCode); + goto cleanup;
err_handle leak
+ } + + logPrlEventErrorHelper(err_handle, filename, funcname, linenr);
PrlHandle_Free(err_handle); - ret = retCode;
retCode of successful PrlEvent_GetErrCode is lost
} else { ret = PrlJob_GetResult(job, result); if (PRL_FAILED(ret)) {
Even after this patch getJobResultHelper will not be good. If some error occurs we have no simple way to find what function fails: PrlJob_Wait PrlJob_GetRetCode PrlJob_GetError PrlEvent_GetErrCode PrlJob_GetResult as they all will print: logPrlErrorHelper(ret, filename, funcname, linenr) with funcname and linenr of the caller of getJobResultHelper.
14.06.2016 10:43, Nikolay Shirokovskiy пишет:
On 11.06.2016 20:16, Maxim Nestratov wrote:
Function logPrlEventErrorHelper is made void and now it only prints an information (if any) from event. If there is no addition information (i.e. an error returned when asked), we don't rewrite original error code with secondary one. I would mention the original motivation of the patch, that is returning error code of PrlEvent_GetErrCode. And motivation of checking for PRL_ERR_NO_DATA too.
Ok.
Signed-off-by: Maxim Nestratov <mnestratov@virtuozzo.com> --- src/vz/vz_sdk.c | 33 ++++++++++++++++----------------- 1 file changed, 16 insertions(+), 17 deletions(-)
diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c index 99c5d4a..54c56e9 100644 --- a/src/vz/vz_sdk.c +++ b/src/vz/vz_sdk.c @@ -99,19 +99,13 @@ logPrlErrorHelper(PRL_RESULT err, const char *filename, } \ } while (0)
-static PRL_RESULT +static void logPrlEventErrorHelper(PRL_HANDLE event, const char *filename, const char *funcname, size_t linenr) { - PRL_RESULT ret, retCode; char *msg1 = NULL, *msg2 = NULL; PRL_UINT32 len = 0; - int err = -1;
- if ((ret = PrlEvent_GetErrCode(event, &retCode))) { - logPrlError(ret); - return ret; - }
PrlEvent_GetErrString(event, PRL_TRUE, PRL_FALSE, NULL, &len);
@@ -130,13 +124,9 @@ logPrlEventErrorHelper(PRL_HANDLE event, const char *filename, virReportErrorHelper(VIR_FROM_THIS, VIR_ERR_INTERNAL_ERROR, filename, funcname, linenr, _("%s %s"), msg1, msg2); - err = 0; - cleanup: VIR_FREE(msg1); VIR_FREE(msg2); - - return err; }
static PRL_RESULT @@ -146,12 +136,12 @@ getJobResultHelper(PRL_HANDLE job, unsigned int timeout, PRL_HANDLE *result, { PRL_RESULT ret, retCode;
- if ((ret = PrlJob_Wait(job, timeout))) { + if (PRL_FAILED(ret = PrlJob_Wait(job, timeout))) { logPrlErrorHelper(ret, filename, funcname, linenr); goto cleanup; }
- if ((ret = PrlJob_GetRetCode(job, &retCode))) { + if (PRL_FAILED(ret = PrlJob_GetRetCode(job, &retCode))) { logPrlErrorHelper(ret, filename, funcname, linenr); goto cleanup; } @@ -159,17 +149,26 @@ getJobResultHelper(PRL_HANDLE job, unsigned int timeout, PRL_HANDLE *result, if (retCode) { PRL_HANDLE err_handle;
+ ret = retCode; + /* Sometimes it's possible to get additional error info. */ - if ((ret = PrlJob_GetError(job, &err_handle))) { + if (PRL_FAILED(retCode = PrlJob_GetError(job, &err_handle))) { logPrlErrorHelper(ret, filename, funcname, linenr); + if (PRL_ERR_NO_DATA != retCode) + logPrlError(retCode); goto cleanup; }
- if (logPrlEventErrorHelper(err_handle, filename, funcname, linenr)) - logPrlErrorHelper(retCode, filename, funcname, linenr); + if (PRL_FAILED(retCode = PrlEvent_GetErrCode(err_handle, &retCode))) { + logPrlErrorHelper(ret, filename, funcname, linenr); + if (PRL_ERR_NO_DATA != retCode) looks like this error code does not matter in case of PrlEvent_GetErrCode
why? something failed while we were trying to get additional information and we honestly log what and where exactly
+ logPrlError(retCode); + goto cleanup; err_handle leak
thanks, will fix it.
+ } + + logPrlEventErrorHelper(err_handle, filename, funcname, linenr);
PrlHandle_Free(err_handle); - ret = retCode; retCode of successful PrlEvent_GetErrCode is lost
Actually we do it intentionally and did it before the patch. The first version stated incorrectly that we have to return this additional error code.
} else { ret = PrlJob_GetResult(job, result); if (PRL_FAILED(ret)) {
Even after this patch getJobResultHelper will not be good. If some error occurs we have no simple way to find what function fails:
PrlJob_Wait PrlJob_GetRetCode PrlJob_GetError PrlEvent_GetErrCode PrlJob_GetResult
as they all will print: logPrlErrorHelper(ret, filename, funcname, linenr) with funcname and linenr of the caller of getJobResultHelper.
This is what this function for, as far as I understand its author, as the intention of this helper is to get a track of original failure, which is perfectly done. It logs some additional stuff now, when another error code is returned.
participants (2)
-
Maxim Nestratov -
Nikolay Shirokovskiy