[Libvir] [patch 9/9] Implement better error reporting

[Daniel P. Berrange]

On Fri, Feb 16, 2007 at 02:44:57PM +0000, Mark McLoughlin wrote:
> Add a qemudLog() function which uses syslog() if we're in
> daemon mode, doesn't output INFO/DEBUG messages unless
> the verbose flag is set and doesn't output DEBUG messages
> unless compiled with --enable-debug.

The QEMU daemon can run both as root, or non-root. Using syslog
in the non-root case is not particularly helpful because the user
will have no way to retrieve the log messages. We should have 
the daemon log to a file under $HOME/.libvirt somewhere, and only
use syslog if explicitly turned on via a command line arg to the
daemon.

I think there's scope for making this more generally useful too,
rather than making it just part of the QEMU daemon. The general
libvirtd will definitely need it, and I think once we have remote
managemnet it'll be important to have a way to do logging in the
client library which integrates into apps using libvirt. eg let
the app register a callback to receive debug messages. The same
core functions 

The patch itself looks reasonable, but next time there's a patch
of this scale please post this stuff for review before committing

>      if (sys) {
> -        if (snprintf(sockname, sizeof(sockname), "%s/run/libvirt/qemud-sock", LOCAL_STATE_DIR) >= (int)sizeof(sockname)) {
> -            return -1;
> -        }
> +        if (snprintf(sockname, sizeof(sockname), "%s/run/libvirt/qemud-sock", LOCAL_STATE_DIR) >= (int)sizeof(sockname))
> +            goto snprintf_error;
> +

I've just realized the return value checks I did here are incomplete.
As well as checking for overflow with >=, we also need to check for
general errors which are indicated by a -ve return value.

> +        if (snprintf(sockname, sizeof(sockname), "%s/run/libvirt/qemud-sock-ro", LOCAL_STATE_DIR) >= (int)sizeof(sockname))
> +            goto snprintf_error;
>  
> -        if (snprintf(sockname, sizeof(sockname), "%s/run/libvirt/qemud-sock-ro", LOCAL_STATE_DIR) >= (int)sizeof(sockname)) {
> -            return -1;
> -        }

Likewise.

Dan

>          unlink(sockname);
>          if (qemudListenUnix(server, sockname, 1) < 0)
>              return -1;
> @@ -289,29 +375,38 @@ static int qemudListen(struct qemud_serv
>          int uid;
>  
>          if ((uid = geteuid()) < 0) {
> +            qemudLog(QEMUD_ERR, "You must run the daemon as root to use system mode");
>              return -1;
>          }

This error message looks bogus. This codepath is the per-user unprivileged
session mode, and the error. And actually checking return value at all is
completely unneccesssary because  geteuid() is guarenteed not to fail.

We do, however, need a check

     if (geteuid() != 0) {
           ....
      }

In the first half of the if() block in the qemudListen function to check for
root privs in system mode.

>          if ((uid = geteuid()) < 0) {
> +            qemudLog(QEMUD_ERR, "You must run the daemon as root to use system mode");
>              goto cleanup;
>          }

Again, same as above.

> @@ -1226,6 +1332,7 @@ static int qemudDispatchPoll(struct qemu
>  
>      while (sock) {
>          struct qemud_socket *next = sock->next;
> +        /* FIXME: the daemon shouldn't exit on error here */
>          if (fds[fd].revents)
>              if (qemudDispatchServer(server, sock) < 0)
>                  return -1;

Yes & no. There are two reasons why qemuDispatchServer can fail. Either
it can fail to set  CLOSEXEC/NONBLOCK mode on the client socket, in
which case we could simply drop the client & continue without exiting.
If the accept() call fails for anything other than EAGAIN/EINTR then
we arguably should exit, because something serious has gone wrong.

>          if (fds[fd].revents) {
> -            QEMUD_DEBUG("Poll data normal\n");
> +            qemudDebug("Poll data normal");
>              if (fds[fd].revents == POLLOUT)
>                  qemudDispatchClientWrite(server, client);
>              else if (fds[fd].revents == POLLIN)
> @@ -1281,7 +1388,7 @@ static int qemudDispatchPoll(struct qemu
>          }
>          vm = next;
>          if (failed)
> -            ret = -1;
> +            ret = -1; /* FIXME: the daemon shouldn't exit on failure here */
>      }

The failed flag should only get set if something serious went wrong when
trying to cleanup after a dead VM. If cleanup succeeds we stick around,
but if it fails I'm not entirely convinced we shouldn't exit.

Dan.
-- 
|=- Red Hat, Engineering, Emerging Technologies, Boston.  +1 978 392 2496 -=|
|=-           Perl modules: http://search.cpan.org/~danberr/              -=|
|=-               Projects: http://freshmeat.net/~danielpb/               -=|
|=-  GnuPG: 7D3B9505   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505  -=|