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 :| -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list