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

Re: [libvirt] [PATCH] daemon: Add early libvirtd start verbose errors.

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

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':
-            if (!(pid_file = strdup(optarg)))
+            if (!(pid_file = strdup(optarg))) {
+                if (verbose)
+                    fprintf(stderr, _("ERROR: Can't allocate memory\n"));
+            }

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.

+    }
+    /* error logging is up, use libvirt's error logging from now */

      if (!pid_file&&  privileged&&
-&pid_file)<  0)
+&pid_file)<  0) {
+        VIR_ERROR(_("Can't determine pid file path."));
+    }

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

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