[libvirt] [PATCH 0/4] vz: error processing fixes
Maxim Nestratov (4): vz: remove unused macro logPrlEventError vz: return event error code in logPrlEventErrorHelper and use it vz: fix getJobResultHelper vz: return correct result for unimplemented ChangeState actions src/vz/vz_sdk.c | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) -- 1.8.3.1
Signed-off-by: Maxim Nestratov <mnestratov@virtuozzo.com> --- src/vz/vz_sdk.c | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c index 7eb78ca..e5c56a5 100644 --- a/src/vz/vz_sdk.c +++ b/src/vz/vz_sdk.c @@ -138,10 +138,6 @@ logPrlEventErrorHelper(PRL_HANDLE event, const char *filename, return err; } -#define logPrlEventError(event) \ - logPrlEventErrorHelper(event, __FILE__, \ - __FUNCTION__, __LINE__) - static PRL_RESULT getJobResultHelper(PRL_HANDLE job, unsigned int timeout, PRL_HANDLE *result, const char *filename, const char *funcname, -- 1.8.3.1
On 26.05.2016 13:14, Maxim Nestratov wrote:
Signed-off-by: Maxim Nestratov <mnestratov@virtuozzo.com> --- src/vz/vz_sdk.c | 4 ---- 1 file changed, 4 deletions(-)
diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c index 7eb78ca..e5c56a5 100644 --- a/src/vz/vz_sdk.c +++ b/src/vz/vz_sdk.c @@ -138,10 +138,6 @@ logPrlEventErrorHelper(PRL_HANDLE event, const char *filename, return err; }
-#define logPrlEventError(event) \ - logPrlEventErrorHelper(event, __FILE__, \ - __FUNCTION__, __LINE__) - static PRL_RESULT getJobResultHelper(PRL_HANDLE job, unsigned int timeout, PRL_HANDLE *result, const char *filename, const char *funcname,
ACK
27.05.2016 11:39, Nikolay Shirokovskiy пишет:
On 26.05.2016 13:14, Maxim Nestratov wrote:
Signed-off-by: Maxim Nestratov <mnestratov@virtuozzo.com> --- src/vz/vz_sdk.c | 4 ---- 1 file changed, 4 deletions(-)
diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c index 7eb78ca..e5c56a5 100644 --- a/src/vz/vz_sdk.c +++ b/src/vz/vz_sdk.c @@ -138,10 +138,6 @@ logPrlEventErrorHelper(PRL_HANDLE event, const char *filename, return err; }
-#define logPrlEventError(event) \ - logPrlEventErrorHelper(event, __FILE__, \ - __FUNCTION__, __LINE__) - static PRL_RESULT getJobResultHelper(PRL_HANDLE job, unsigned int timeout, PRL_HANDLE *result, const char *filename, const char *funcname,
ACK
Pushed now.
If PrlEvent_GetErrCode returns an error code this is what we should use as an error code for the whole action Signed-off-by: Maxim Nestratov <mnestratov@virtuozzo.com> --- src/vz/vz_sdk.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c index e5c56a5..7fc7d97 100644 --- a/src/vz/vz_sdk.c +++ b/src/vz/vz_sdk.c @@ -100,14 +100,17 @@ logPrlErrorHelper(PRL_RESULT err, const char *filename, static PRL_RESULT logPrlEventErrorHelper(PRL_HANDLE event, const char *filename, - const char *funcname, size_t linenr) + const char *funcname, size_t linenr, PRL_RESULT* retCode) { - PRL_RESULT ret, retCode; + PRL_RESULT ret; char *msg1 = NULL, *msg2 = NULL; PRL_UINT32 len = 0; int err = -1; - if ((ret = PrlEvent_GetErrCode(event, &retCode))) { + if (!retCode) + return -1; + + if ((ret = PrlEvent_GetErrCode(event, retCode))) { logPrlError(ret); return ret; } @@ -164,7 +167,8 @@ getJobResultHelper(PRL_HANDLE job, unsigned int timeout, PRL_HANDLE *result, goto cleanup; } - if (logPrlEventErrorHelper(err_handle, filename, funcname, linenr)) + ret = logPrlEventErrorHelper(err_handle, filename, funcname, linenr, &retCode); + if (ret) logPrlErrorHelper(retCode, filename, funcname, linenr); PrlHandle_Free(err_handle); -- 1.8.3.1
On 26.05.2016 13:14, Maxim Nestratov wrote:
If PrlEvent_GetErrCode returns an error code this is what we should use as an error code for the whole action
Signed-off-by: Maxim Nestratov <mnestratov@virtuozzo.com> --- src/vz/vz_sdk.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c index e5c56a5..7fc7d97 100644 --- a/src/vz/vz_sdk.c +++ b/src/vz/vz_sdk.c @@ -100,14 +100,17 @@ logPrlErrorHelper(PRL_RESULT err, const char *filename,
static PRL_RESULT logPrlEventErrorHelper(PRL_HANDLE event, const char *filename, - const char *funcname, size_t linenr) + const char *funcname, size_t linenr, PRL_RESULT* retCode)
I would put retCode before caller position parameters(filename etc)
{ - PRL_RESULT ret, retCode; + PRL_RESULT ret; char *msg1 = NULL, *msg2 = NULL; PRL_UINT32 len = 0; int err = -1;
- if ((ret = PrlEvent_GetErrCode(event, &retCode))) { + if (!retCode) + return -1;
Supplying NULL retCode is a big usage mistake as you get wrong result code in this case. Thus I would instead don't fail but give warning and continue. This way correct error will be logged at least. Eventually I think simpliest would be move PrlEvent_GetErrCode out of this function. Also I would make it void as it is a logging function.
+ + if ((ret = PrlEvent_GetErrCode(event, retCode))) { logPrlError(ret); return ret; } @@ -164,7 +167,8 @@ getJobResultHelper(PRL_HANDLE job, unsigned int timeout, PRL_HANDLE *result, goto cleanup; }
- if (logPrlEventErrorHelper(err_handle, filename, funcname, linenr)) + ret = logPrlEventErrorHelper(err_handle, filename, funcname, linenr, &retCode); + if (ret) logPrlErrorHelper(retCode, filename, funcname, linenr);
PrlHandle_Free(err_handle);
27.05.2016 12:14, Nikolay Shirokovskiy пишет:
On 26.05.2016 13:14, Maxim Nestratov wrote:
If PrlEvent_GetErrCode returns an error code this is what we should use as an error code for the whole action
Signed-off-by: Maxim Nestratov <mnestratov@virtuozzo.com> --- src/vz/vz_sdk.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c index e5c56a5..7fc7d97 100644 --- a/src/vz/vz_sdk.c +++ b/src/vz/vz_sdk.c @@ -100,14 +100,17 @@ logPrlErrorHelper(PRL_RESULT err, const char *filename,
static PRL_RESULT logPrlEventErrorHelper(PRL_HANDLE event, const char *filename, - const char *funcname, size_t linenr) + const char *funcname, size_t linenr, PRL_RESULT* retCode) I would put retCode before caller position parameters(filename etc)
{ - PRL_RESULT ret, retCode; + PRL_RESULT ret; char *msg1 = NULL, *msg2 = NULL; PRL_UINT32 len = 0; int err = -1;
- if ((ret = PrlEvent_GetErrCode(event, &retCode))) { + if (!retCode) + return -1; Supplying NULL retCode is a big usage mistake as you get wrong result code in this case. Thus I would instead don't fail but give warning and continue. This way correct error will be logged at least.
Eventually I think simpliest would be move PrlEvent_GetErrCode out of this function. Also I would make it void as it is a logging function.
Makes sense. Thanks. I'll resend this patch. Maxim
when additional information is asked PrlJob_GetError an error PRL_ERR_NO_DATA should not be treated as error Signed-off-by: Maxim Nestratov <mnestratov@virtuozzo.com> --- src/vz/vz_sdk.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c index 7fc7d97..bad4ddd 100644 --- a/src/vz/vz_sdk.c +++ b/src/vz/vz_sdk.c @@ -162,7 +162,8 @@ getJobResultHelper(PRL_HANDLE job, unsigned int timeout, PRL_HANDLE *result, PRL_HANDLE err_handle; /* Sometimes it's possible to get additional error info. */ - if ((ret = PrlJob_GetError(job, &err_handle))) { + ret = PrlJob_GetError(job, &err_handle); + if (ret && ret != PRL_ERR_NO_DATA) { logPrlErrorHelper(ret, filename, funcname, linenr); goto cleanup; } -- 1.8.3.1
On 26.05.2016 13:14, Maxim Nestratov wrote:
when additional information is asked PrlJob_GetError an error PRL_ERR_NO_DATA should not be treated as error
Signed-off-by: Maxim Nestratov <mnestratov@virtuozzo.com> --- src/vz/vz_sdk.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c index 7fc7d97..bad4ddd 100644 --- a/src/vz/vz_sdk.c +++ b/src/vz/vz_sdk.c @@ -162,7 +162,8 @@ getJobResultHelper(PRL_HANDLE job, unsigned int timeout, PRL_HANDLE *result, PRL_HANDLE err_handle;
/* Sometimes it's possible to get additional error info. */ - if ((ret = PrlJob_GetError(job, &err_handle))) { + ret = PrlJob_GetError(job, &err_handle); + if (ret && ret != PRL_ERR_NO_DATA) { logPrlErrorHelper(ret, filename, funcname, linenr); goto cleanup; }
I would log and return in case of PRL_ERR_NO_DATA or it is rather compilicated to call logPrlEventErrorHelper only to return immediately.
27.05.2016 12:20, Nikolay Shirokovskiy пишет:
On 26.05.2016 13:14, Maxim Nestratov wrote:
when additional information is asked PrlJob_GetError an error PRL_ERR_NO_DATA should not be treated as error
Signed-off-by: Maxim Nestratov <mnestratov@virtuozzo.com> --- src/vz/vz_sdk.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c index 7fc7d97..bad4ddd 100644 --- a/src/vz/vz_sdk.c +++ b/src/vz/vz_sdk.c @@ -162,7 +162,8 @@ getJobResultHelper(PRL_HANDLE job, unsigned int timeout, PRL_HANDLE *result, PRL_HANDLE err_handle;
/* Sometimes it's possible to get additional error info. */ - if ((ret = PrlJob_GetError(job, &err_handle))) { + ret = PrlJob_GetError(job, &err_handle); + if (ret && ret != PRL_ERR_NO_DATA) { logPrlErrorHelper(ret, filename, funcname, linenr); goto cleanup; }
I would log and return in case of PRL_ERR_NO_DATA or it is rather compilicated to call logPrlEventErrorHelper only to return immediately.
I squashed this patch with the previous and reworked it a bit. Sent a new version. Maxim
Map PRL_ERR_UNIMPLEMENTED to VIR_ERR_OPERATION_INVALID Signed-off-by: Maxim Nestratov <mnestratov@virtuozzo.com> --- src/vz/vz_sdk.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c index bad4ddd..94ed35e 100644 --- a/src/vz/vz_sdk.c +++ b/src/vz/vz_sdk.c @@ -2052,6 +2052,7 @@ prlsdkDomainChangeStateLocked(vzDriverPtr driver, case PRL_ERR_DISP_VM_IS_NOT_STARTED: case PRL_ERR_DISP_VM_IS_NOT_STOPPED: case PRL_ERR_INVALID_ACTION_REQUESTED: + case PRL_ERR_UNIMPLEMENTED: virerr = VIR_ERR_OPERATION_INVALID; break; default: -- 1.8.3.1
On 26.05.2016 13:15, Maxim Nestratov wrote:
Map PRL_ERR_UNIMPLEMENTED to VIR_ERR_OPERATION_INVALID
Signed-off-by: Maxim Nestratov <mnestratov@virtuozzo.com> --- src/vz/vz_sdk.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c index bad4ddd..94ed35e 100644 --- a/src/vz/vz_sdk.c +++ b/src/vz/vz_sdk.c @@ -2052,6 +2052,7 @@ prlsdkDomainChangeStateLocked(vzDriverPtr driver, case PRL_ERR_DISP_VM_IS_NOT_STARTED: case PRL_ERR_DISP_VM_IS_NOT_STOPPED: case PRL_ERR_INVALID_ACTION_REQUESTED: + case PRL_ERR_UNIMPLEMENTED: virerr = VIR_ERR_OPERATION_INVALID; break; default:
ACK
27.05.2016 12:38, Nikolay Shirokovskiy пишет:
On 26.05.2016 13:15, Maxim Nestratov wrote:
Map PRL_ERR_UNIMPLEMENTED to VIR_ERR_OPERATION_INVALID
Signed-off-by: Maxim Nestratov <mnestratov@virtuozzo.com> --- src/vz/vz_sdk.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c index bad4ddd..94ed35e 100644 --- a/src/vz/vz_sdk.c +++ b/src/vz/vz_sdk.c @@ -2052,6 +2052,7 @@ prlsdkDomainChangeStateLocked(vzDriverPtr driver, case PRL_ERR_DISP_VM_IS_NOT_STARTED: case PRL_ERR_DISP_VM_IS_NOT_STOPPED: case PRL_ERR_INVALID_ACTION_REQUESTED: + case PRL_ERR_UNIMPLEMENTED: virerr = VIR_ERR_OPERATION_INVALID; break; default:
ACK
Pushed now. Maxim
participants (2)
-
Maxim Nestratov -
Nikolay Shirokovskiy