[libvirt] [PATCH] cleanup of direct stderr logging

[Daniel P. Berrange]

On Tue, May 19, 2009 at 05:40:15PM +0200, Daniel Veillard wrote:
>   In a number of places we still had direct output of logs, warnings or
> errors, instead of using the existing error and logging infrastructure.
> This patches tries to clean this up,

It all looks good side from  a couple bits which I think can be
simplified still further...

> Index: src/network_driver.c
> ===================================================================
> RCS file: /data/cvs/libxen/src/network_driver.c,v
> retrieving revision 1.25
> diff -u -r1.25 network_driver.c
> --- src/network_driver.c	19 May 2009 11:06:25 -0000	1.25
> +++ src/network_driver.c	19 May 2009 15:25:45 -0000
> @@ -56,6 +56,7 @@
>  #include "uuid.h"
>  #include "iptables.h"
>  #include "bridge.h"
> +#include "logging.h"
>  
>  #define NETWORK_PID_DIR LOCAL_STATE_DIR "/run/libvirt/network"
>  #define NETWORK_STATE_DIR LOCAL_STATE_DIR "/lib/libvirt/network"
> @@ -87,10 +88,6 @@
>  
>  static int networkShutdown(void);
>  
> -/* networkDebug statements should be changed to use this macro instead. */
> -
> -#define networkLog(level, msg...) fprintf(stderr, msg)
> -
>  static int networkStartNetworkDaemon(virConnectPtr conn,
>                                     struct network_driver *driver,
>                                     virNetworkObjPtr network);
> @@ -174,7 +171,8 @@
>              !virNetworkIsActive(driver->networks.objs[i]) &&
>              networkStartNetworkDaemon(NULL, driver, driver->networks.objs[i]) < 0) {
>              virErrorPtr err = virGetLastError();
> -            networkLog(NETWORK_ERR, _("Failed to autostart network '%s': %s\n"),
> +            networkReportError(NULL, NULL, NULL, VIR_WAR_NO_NETWORK,
> +                              _("Failed to autostart network '%s': %s\n"),
>                         driver->networks.objs[i]->def->name,
>                         err ? err->message : NULL);
>          }

This code is a left-over from the old livirtd specific logging code.

Originally if a virError were raised by the networkStartNetworkDaemon()
it would disappear into  a black hole, because this is an internal
libvirtd API. So I added the networkLog() call here to record the
error details.

When you added the new logging APIs, you made it so that all virError
which are raised are always logged using VIR_ERROR - see the call to
'virLogMessage' in virRaiseError.

Thus, we don't need to call networkReportError() here. We can simply
delete the existing networkLog() line - the error is already logged.



> Index: src/storage_conf.c
> ===================================================================
> RCS file: /data/cvs/libxen/src/storage_conf.c,v
> retrieving revision 1.46
> diff -u -r1.46 storage_conf.c
> --- src/storage_conf.c	1 Apr 2009 16:03:22 -0000	1.46
> +++ src/storage_conf.c	19 May 2009 15:25:45 -0000
> @@ -50,7 +50,9 @@
>  # define ULLONG_MAX   ULONG_LONG_MAX
>  #endif
>  
> -#define virStorageLog(msg...) fprintf(stderr, msg)
> +#define virStorageError(conn, code, fmt...)                             \
> +            virReportErrorHelper(conn, VIR_FROM_STORAGE, code, __FILE__,\
> +                                  __FUNCTION__, __LINE__, fmt)
>  
>  VIR_ENUM_IMPL(virStoragePool,
>                VIR_STORAGE_POOL_LAST,
> @@ -1333,33 +1335,38 @@
>  
>      if (!(def = virStoragePoolDefParse(NULL, xml, file))) {
>          virErrorPtr err = virGetLastError();
> -        virStorageLog("Error parsing storage pool config '%s' : %s",
> -                      path, err ? err->message : NULL);
> +        virStorageError(conn, VIR_ERR_PARSE_FAILED,
> +                        "Error parsing storage pool config '%s' : %s",
> +                        path, err ? err->message : NULL);
>          return NULL;
>      }
>  

>      if (!(pool = virStoragePoolObjAssignDef(conn, pools, def))) {
> -        virStorageLog("Failed to load storage pool config '%s': out of memory", path);
> +        virStorageError(conn, VIR_ERR_INTERNAL_ERROR,
> +            "Failed to load storage pool config '%s': out of memory", path);
>          virStoragePoolDefFree(def);
>          return NULL;
>      }


These two virStorageLog() lines can also be removed for same reason - the
function being called has already reported the rror and will be logged.

>  
>      pool->configFile = strdup(path);
>      if (pool->configFile == NULL) {
> -        virStorageLog("Failed to load storage pool config '%s': out of memory", path);
> +        virStorageError(conn, VIR_ERR_INTERNAL_ERROR,
> +            "Failed to load storage pool config '%s': out of memory", path);
>          virStoragePoolDefFree(def);
>          return NULL;
>      }
>      pool->autostartLink = strdup(autostartLink);
>      if (pool->autostartLink == NULL) {
> -        virStorageLog("Failed to load storage pool config '%s': out of memory", path);
> +        virStorageError(conn, VIR_ERR_INTERNAL_ERROR,
> +            "Failed to load storage pool config '%s': out of memory", path);
>          virStoragePoolDefFree(def);
>          return NULL;
>      }

These two should just be virReportOOM()  calls.

> @@ -1383,7 +1390,7 @@
>          char ebuf[1024];
>          if (errno == ENOENT)
>              return 0;
> -        virStorageLog("Failed to open dir '%s': %s",
> +        virReportSystemError(conn, errno, _("Failed to open dir '%s': %s"),
>                        configDir, virStrerror(errno, ebuf, sizeof ebuf));
>          return -1;
>      }

No need to add  virStrerror() here, because virReportSystemError does
this automatically



Regards,
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 :|