[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 26/30] Remove virConnectPtr from error reporting in libvirt.c



On 04/04/2010 11:36 AM, Matthias Bolte wrote:
> ---
>  src/libvirt.c | 1577 +++++++++++++++++++++++++--------------------------------
>  1 files changed, 697 insertions(+), 880 deletions(-)
> 
> diff --git a/src/libvirt.c b/src/libvirt.c
> index fb683c0..788b943 100644
> --- a/src/libvirt.c
> +++ b/src/libvirt.c
> @@ -428,38 +428,19 @@ DllMain (HINSTANCE instance ATTRIBUTE_UNUSED,
>  }
>  #endif
>  
> -
> -/**
> - * virLibConnError:
> - * @conn: the connection if available
> - * @error: the error number
> - * @info: extra information string
> - *
> - * Handle an error at the connection level
> - */
> -static void
> -virLibConnError(virConnectPtr conn, virErrorNumber error, const char *info)
> -{
> -    const char *errmsg;
> -
> -    if (error == VIR_ERR_OK)
> -        return;
> -
> -    errmsg = virErrorMsg(error, info);
> -    virRaiseError(conn, NULL, NULL, VIR_FROM_NONE, error, VIR_ERR_ERROR,
> -                  errmsg, info, NULL, 0, 0, errmsg, info);
> -}
> +#define virLibConnError(code, ...)                                            \
> +    virReportErrorHelper(NULL, VIR_FROM_NONE, code, __FILE__,                 \
> +                         __FUNCTION__, __LINE__, __VA_ARGS__)

Any reason you are deleting, rather than updating, the documentation?
It can still be useful to document macros.

> @@ -697,12 +499,12 @@ virRegisterNetworkDriver(virNetworkDriverPtr driver)
>        return -1;
>  
>      if (driver == NULL) {
> -        virLibConnError(NULL, VIR_ERR_INVALID_ARG, __FUNCTION__);
> +        virLibConnError(VIR_ERR_INVALID_ARG, "%s", __FUNCTION__);

Did gcc seriously warn on this?  __FUNCTION__ cannot contain %, and is
not translated.  Maybe it's worth a QoI bug report to gcc, because I see
no reason why __FUNCTION__ should not be directly usable as a format string.

Then again, why are we passing __FUNCTION__ ourselves?  That seems
awkward, especially given that you just defined virLibConnError to pass
__FUNCTION__ to virReportErrorHelper, so now we've ended up using
__FUNCTION__ twice.  Isn't the whole point of going through a macro to
make it easier to apply __FUNCTION__ automatically without having to
think about it at every client site?

> @@ -1217,8 +1018,8 @@ do_open (const char *name,
>                 (res == VIR_DRV_OPEN_ERROR ? "ERROR" : "unknown status")));
>          if (res == VIR_DRV_OPEN_ERROR) {
>              if (STREQ(virNetworkDriverTab[i]->name, "remote")) {
> -                virLibConnWarning (NULL, VIR_WAR_NO_NETWORK,
> -                                   "Is the daemon running ?");
> +                virLibConnWarning(VIR_WAR_NO_NETWORK,
> +                                  "Is the daemon running ?");

Is virLibConnWarning something that deserves translation?  And the space
before ? looks weird, as long as we are touching this line.

-- 
Eric Blake   eblake redhat com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]