Re: [libvirt] [PATCH v2] vz: get additional error information from job correctly

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(a)virtuozzo.com&gt; > --- > 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.