[libvirt] [PATCH] daemon: Add early libvirtd start verbose errors.
Early errors during start of libvirtd didn't have an error reporting mechanism and caused libvirtd to exit silently (only the return value indicated an error). This patch adds error messages printed to stderr if verbose parameter is specified to the daemon. fixes: https://bugzilla.redhat.com/show_bug.cgi?id=728654 --- daemon/libvirtd.c | 40 ++++++++++++++++++++++++++++++++-------- 1 files changed, 32 insertions(+), 8 deletions(-) diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index 53f1002..c3867af 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -1328,14 +1328,20 @@ int main(int argc, char **argv) { case 'p': VIR_FREE(pid_file); - if (!(pid_file = strdup(optarg))) + if (!(pid_file = strdup(optarg))) { + if (verbose) + fprintf(stderr, _("ERROR: Can't allocate memory\n")); exit(EXIT_FAILURE); + } break; case 'f': VIR_FREE(remote_config_file); - if (!(remote_config_file = strdup(optarg))) + if (!(remote_config_file = strdup(optarg))) { + if (verbose) + fprintf(stderr, _("ERROR: Can't allocate memory\n")); exit(EXIT_FAILURE); + } break; case OPT_VERSION: @@ -1353,21 +1359,30 @@ int main(int argc, char **argv) { } } - if (!(config = daemonConfigNew(privileged))) + if (!(config = daemonConfigNew(privileged))) { + if (verbose) + fprintf(stderr, _("ERROR: Can't create configuration\n")); exit(EXIT_FAILURE); + } /* No explicit config, so try and find a default one */ if (remote_config_file == NULL) { implicit_conf = true; if (daemonConfigFilePath(privileged, - &remote_config_file) < 0) + &remote_config_file) < 0) { + if (verbose) + fprintf(stderr, _("ERROR: Can't determine config path\n")); exit(EXIT_FAILURE); + } } /* Read the config file if it exists*/ if (remote_config_file && - daemonConfigLoad(config, remote_config_file, implicit_conf) < 0) + daemonConfigLoad(config, remote_config_file, implicit_conf) < 0) { + if (verbose) + fprintf(stderr, _("ERROR: Can't load config file '%s'\n"), remote_config_file); exit(EXIT_FAILURE); + } if (config->host_uuid && virSetHostUUIDStr(config->host_uuid) < 0) { @@ -1375,19 +1390,28 @@ int main(int argc, char **argv) { exit(EXIT_FAILURE); } - if (daemonSetupLogging(config, privileged, verbose, godaemon) < 0) + if (daemonSetupLogging(config, privileged, verbose, godaemon) < 0) { + if (verbose) + fprintf(stderr, _("ERROR: Can't initialise logging\n")); exit(EXIT_FAILURE); + } + + /* error logging is up, use libvirt's error logging from now */ if (!pid_file && privileged && daemonPidFilePath(privileged, - &pid_file) < 0) + &pid_file) < 0) { + VIR_ERROR(_("Can't determine pid file path.")); exit(EXIT_FAILURE); + } if (daemonUnixSocketPaths(config, privileged, &sock_file, - &sock_file_ro) < 0) + &sock_file_ro) < 0) { + VIR_ERROR(_("Can't determine socket paths")); exit(EXIT_FAILURE); + } if (godaemon) { char ebuf[1024]; -- 1.7.6
On 08/11/2011 06:18 AM, Peter Krempa wrote:
Early errors during start of libvirtd didn't have an error reporting mechanism and caused libvirtd to exit silently (only the return value indicated an error). This patch adds error messages printed to stderr if verbose parameter is specified to the daemon.
fixes: https://bugzilla.redhat.com/show_bug.cgi?id=728654 --- daemon/libvirtd.c | 40 ++++++++++++++++++++++++++++++++-------- 1 files changed, 32 insertions(+), 8 deletions(-)
diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index 53f1002..c3867af 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -1328,14 +1328,20 @@ int main(int argc, char **argv) {
case 'p': VIR_FREE(pid_file); - if (!(pid_file = strdup(optarg))) + if (!(pid_file = strdup(optarg))) { + if (verbose) + fprintf(stderr, _("ERROR: Can't allocate memory\n")); exit(EXIT_FAILURE); + } break;
As written, if you specify 'libvirtd --pid-file xyz --verbose', you still get no message; you have to specify 'libvirtd --verbose --pid-file xyz' for this to be useful. But I think that in general with daemon programs, it is permissible to write to stderr without needing --verbose in the case of an early exit. That is, if the daemon can't even start, then it should output why; a silent early exit is never nice. I think these messages should be unconditional, and that --verbose should be reserved for conditionally documenting that we are about to attempt steps that normally succeed, rather than for conditionally diagnosing steps that have already failed.
- if (daemonSetupLogging(config, privileged, verbose, godaemon)< 0) + if (daemonSetupLogging(config, privileged, verbose, godaemon)< 0) { + if (verbose) + fprintf(stderr, _("ERROR: Can't initialise logging\n"));
s/initialise/initialize/ - translated messages should be US spelling, and leave en_UK.po as the source of UK spellings visible to the user.
exit(EXIT_FAILURE); + } + + /* error logging is up, use libvirt's error logging from now */
if (!pid_file&& privileged&& daemonPidFilePath(privileged, -&pid_file)< 0) +&pid_file)< 0) { + VIR_ERROR(_("Can't determine pid file path.")); exit(EXIT_FAILURE); + }
This hunk and below are appropriate - once we are far enough along to use logging, then it is better to use logging than stderr before calling an early exit(). I think fixing the first half of the patch to not depend on --verbose probably warrants a v2. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Thu, Aug 11, 2011 at 02:18:15PM +0200, Peter Krempa wrote:
Early errors during start of libvirtd didn't have an error reporting mechanism and caused libvirtd to exit silently (only the return value indicated an error). This patch adds error messages printed to stderr if verbose parameter is specified to the daemon.
fixes: https://bugzilla.redhat.com/show_bug.cgi?id=728654 --- daemon/libvirtd.c | 40 ++++++++++++++++++++++++++++++++-------- 1 files changed, 32 insertions(+), 8 deletions(-)
Part of the problem I see here is that we don't initialized the libvirt logging framework until quite late in startup sequence, due to the need to the read the config file. If we added a call to virLogSetFromEnv() right at the start of libvirtd, then we'd get a basic logging setup activated, and so can just use VIR_ERROR everywhere, instead of a mixture of fprintf(stderr) and VIR_ERROR. The default logging setup for VIR_ERROR will printf to stderr anyway. This will mean you can override the logging destination from environment variables more effectively. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Peter Krempa