10:40 a.m.
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,
Daniel
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
daniel(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
6:40 a.m.
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 :|
8:43 a.m.
On Wed, May 20, 2009 at 12:40:36PM +0100, Daniel P. Berrange wrote:
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...
[...]
> 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.
I had kept them because they gave more contextual informations, but
basically if there isn't enough memory, it's not worth bothering
really.
So I made all the changes and commited, thanks !
One thing we noticed is that if someone disable debug with
--disable-debug then all errors/warnings/info which are reported
directly via VIR_ERROR/VIR_WARN/VIR_INFO disapear as the logging
code is not compiled in. We need to change this, logging code should
not be made optional and --disable-debug should just redefine the
VIR_DEBUG* macros to noop, but the rest of the logging infrastructure
should still be activated.
This will have to be addressed in a separate patch,
Daniel
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
daniel(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
5668
days inactive
5669
days old
2 comments
2 participants
participants (2)
-
Daniel P. Berrange -
Daniel Veillard