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

[Maxim Nestratov <mnestratov@xxxxxxxxxxxxx>]

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@xxxxxxxxxxxxx>
> > ---
> >   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.

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list