[libvirt] [PATCH] command: properly diagnose process exit via signal

[Daniel P. Berrange]

On Tue, Mar 22, 2011 at 12:00:02PM -0600, Eric Blake wrote:
> Child processes don't always reach _exit(); if they die from a
> signal, then any messages should still be accurate.  Most users
> either expect a 0 status (thankfully, if status==0, then
> WIFEXITED(status) is true and WEXITSTATUS(status)==0 for all
> known platforms) or were filtering on WIFEXITED before printing
> a status, but a few were missing this check.  Additionally,
> nwfilter_ebiptables_driver was making an assumption that works
> on Linux (where WEXITSTATUS shifts and WTERMSIG just masks)
> but fails on other platforms (where WEXITSTATUS just masks and
> WTERMSIG shifts).
> 
> * src/util/command.h (virCommandTranslateStatus): New helper.
> * src/libvirt_private.syms (command.h): Export it.
> * src/util/command.c (virCommandTranslateStatus): New function.
> (virCommandWait): Use it to also diagnose status from signals.
> * src/security/security_apparmor.c (load_profile): Likewise.
> * src/storage/storage_backend.c
> (virStorageBackendQEMUImgBackingFormat): Likewise.
> * src/util/util.c (virExecDaemonize, virRunWithHook)
> (virFileOperation, virDirCreate): Likewise.
> * daemon/remote.c (remoteDispatchAuthPolkit): Likewise.
> * src/nwfilter/nwfilter_ebiptables_driver.c (ebiptablesExecCLI):
> Likewise.
> ---
> 
> In response to my finding here:
> https://www.redhat.com/archives/libvir-list/2011-March/msg01029.html
> 
> > status includes normal exit and signals.  This should probably use
> > WIFEXITED and WEXITSTATUS to avoid printing values shifted by 8.  For
> > that matter, I just noticed that virCommandWait should probably be more
> > careful in how it interprets status.
> 
>  daemon/remote.c                           |   11 +++++++-
>  src/libvirt_private.syms                  |    1 +
>  src/nwfilter/nwfilter_ebiptables_driver.c |   16 ++++++++++---
>  src/security/security_apparmor.c          |   22 ++++++++++++------
>  src/storage/storage_backend.c             |   18 ++++++---------
>  src/util/command.c                        |   34 ++++++++++++++++++++++++++--
>  src/util/command.h                        |    7 +++++-
>  src/util/util.c                           |   27 ++++++++++------------
>  8 files changed, 92 insertions(+), 44 deletions(-)

>      if (status != 0) {
> -        VIR_ERROR(_("Policy kit denied action %s from pid %d, uid %d, result: %d"),
> -                  action, callerPid, callerUid, status);
> +        char *tmp = virCommandTranslateStatus(status);
> +        if (!tmp) {
> +            virReportOOMError();
> +            goto authfail;
> +        }
> +        VIR_ERROR(_("Policy kit denied action %s from pid %d, uid %d: %s"),
> +                  action, callerPid, callerUid, tmp);
> +        VIR_FREE(tmp);
>          goto authdeny;

Hmm, I rather think we ought to keep the VIR_ERROR log message, even
if virCommandTranslateStatus does OOM. eg just use  NULLSTR(tmp)

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