On Fri, Feb 12, 2010 at 05:17:00PM +0000, Matthew Booth wrote:
On 12/02/10 14:43, Richard W.M. Jones wrote:
From: Richard Jones <rjones@redhat.com> Date: Fri, 12 Feb 2010 14:06:25 +0000 Subject: [PATCH 2/2] daemon: Don't need to prefix error messages with the command name.
The RPC stubs already prefix the command name to error messages. The daemon doesn't have to do this. As a (small) benefit this also makes the daemon slightly smaller.
Code in the daemon such as:
if (argv[0] == NULL) { reply_with_error ("passed an empty list"); return NULL; }
now results in error messages like this:
<fs> command "" libguestfs: error: command: passed an empty list
Can you please check:
All prefix __func__: NEED_AUG in augeas.c NEED_ROOT ABS_PATH RESOLVE_DEVICE XXX_NOT_IMPL NOT_AVAILABLE in daemon.h NEED_INOTIFY in inotify.c RUN_PARTED in parted.c
These ones I left in. On a first pass it's hard to tell what __func__ will expand to, but in any case it doesn't really matter if a function name is left in there.
What on earth is this (linkc:146): reply_with_error ("ln%s%s: %s: %s: %s", flag ? " " : "", flag ? : "", target, linkname, err);
This seems OK to me: It's basically echoing the actual command that was run (eg. "ln -sf ..."). This seems like possibly useful information to keep around.
lvm.c:300: reply_with_error ("lvremove: %s: %s", xs[i], err); lvm.c:317: reply_with_error ("vgremove: %s: %s", xs[i], err); lvm.c:334: reply_with_error ("pvremove: %s: %s", xs[i], err); lvm.c:453: reply_with_error ("vgchange: %s", err);
I left these in deliberately because what is being echoed back here is the actual LVM command that was run, not the libguestfs command. eg: the first 3 are different stages of the lvm_remove_all command, and if one stage fails it'd be useful to know which.
mount.c:155: reply_with_error ("mount: %s", err); mount.c:254: reply_with_error ("mount: %s", err); mount.c:296: reply_with_error ("umount: %s: %s", mounts[i], err);
Similarly, the command that was run, rather than the libguestfs command.
parted.c:236: reply_with_error ("parted print: %s: %s", device,
The command that is run is "parted [options] print device", which again is different from the libguestfs command (eg. "part_disk").
wc.c:47: reply_with_error ("wc %s: %s", flag, err);
It's echoing "wc -l" etc. This is arguable.
I think that's the lot. If we missed some we'll pick them up over time. Incidentally, I used cscope to look for callers of reply_with_error in deaemon/.
Thanks for checking this in detail. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. http://et.redhat.com/~rjones/libguestfs/ See what it can do: http://et.redhat.com/~rjones/libguestfs/recipes.html