Re: [libvirt] [PATCH] fix errors in virReportSystemErrorFull

["Daniel P. Berrange" <berrange@xxxxxxxxxx>]

On Mon, Jan 26, 2009 at 03:02:48PM +0100, Jim Meyering wrote:
> 
> So, since we'd rather avoid having to allocate at all in the
> error-reporting path (esp for OOM errors), I've made it automatic.

That doesn't really matter/help the OOM reporting scenario, because
the convention for VIR_ERR_NO_MEMORY is to have a NULL fmt arg, and
the virRaiseError() method will itself fail the allocation later if
we were in a scenario where it mattered. 

>  #ifdef HAVE_STRERROR_R
>  #ifdef __USE_GNU
> @@ -1047,10 +1047,15 @@ void virReportSystemErrorFull(virConnectPtr conn,
>          errorMessage[0] = '\0';
>      }
> 
> -    if (virAsprintf(&combined, "%s: %s", errorMessage, theerrnostr) < 0)
> -        combined = theerrnostr; /* OOM, so lets just pass the strerror info as best effort */
> +    char *err_str;
> +    int n = snprintf(combined, sizeof combined, "%s: %s",
> +                     errorMessage, theerrnostr);
> +    err_str = (0 < n && n < sizeof(combined)
> +               ? combined
> +               /* use the strerror info as best effort */
> +               : theerrnostr);
> 
> -    virerr = virErrorMsg(VIR_ERR_SYSTEM_ERROR, (errorMessage[0] ? errorMessage : NULL));
> +    virerr = virErrorMsg(VIR_ERR_SYSTEM_ERROR, (err_str[0] ? err_str : NULL));

It is actually the next line that really need to the change to use err_str - the
virErrorMsg doesn't actually care about the data in that string, only checks whether
it is NULL or not, so the virErrorMsg call was technically still OK, but this
next one was no good:

>      virRaiseError(conn, NULL, NULL, domcode, VIR_ERR_SYSTEM_ERROR, VIR_ERR_ERROR,
>                    virerr, errorMessage, NULL, -1, -1, virerr, errorMessage);

Looking at the whole method again, I think it needs to be re-written to
something closer to this:

const char *virSystemErrorFormat(int theerrno, char *errBuf, size_t errBufLen)
{
#ifdef HAVE_STRERROR_R
#ifdef __USE_GNU
    /* Annoying linux specific API contract */
    return strerror_r(theerrno, errBuf, errBufLen);
#else
    strerror_r(theerrno, errBuf, errBufLen);
    return errBuf;
#endif
#else
    /* Mingw lacks strerror_r() and its strerror() is definitely not
     * threadsafe, so safest option is to just print the raw errno
     * value - we can at least reliably & safely look it up in the
     * header files for debug purposes
     */
    snprintf(errBuf, errBufLen, "errno=%d", theerrno);
    return errBuf;
#endif
}


void virReportSystemErrorFull(virConnectPtr conn,
                              int domcode,
                              int theerrno,
                              const char *filename ATTRIBUTE_UNUSED,
                              const char *funcname ATTRIBUTE_UNUSED,
                              size_t linenr ATTRIBUTE_UNUSED,
                              const char *fmt, ...)
{
    char systemError[1024];
    char msgDetailBuf[1024];

    const char *errnoDetail = virSystemErrorFormat(theerrno, systemError, sizeof(systemError);
    const char *msg = virErrorMsg(VIR_ERR_SYSTEM_ERROR, fmt);
    const char *msgDetail;

    if (fmt) {
        va_list args;

        va_start(args, fmt);
        vsnprintf(msgDetailBuf, sizeof(msgDetailBuf)-1, fmt, args);
        va_end(args);

        strncat(msgDetailBuf, errnoDetail, (sizeof(msgDetailBuf)-1)-strlen(msgDetail));
        msgDetail = msgDetailBuf;
    } else {
        msgDetail = errnoDetail;
    }

    virRaiseError(conn, NULL, NULL, domcode, VIR_ERR_SYSTEM_ERROR, VIR_ERR_ERROR,
                  msg, msgDetail, NULL, -1, -1, msg, msgDetail);
}

Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

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