[PATCH 0/2] Fix doubling of command names in some error messages
Depending on which commands you run and from which language bindings, the command name in error messages can be doubled or even tripled. You could see errors like: foo: foo: /path: File not found These two patches address the problem. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming blog: http://rwmj.wordpress.com Fedora now supports 80 OCaml packages (the OPEN alternative to F#) http://cocan.org/getting_started_with_ocaml_on_red_hat_and_fedora
-- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into Xen guests. http://et.redhat.com/~rjones/virt-p2v
On 12/02/10 14:43, Richard W.M. Jones wrote:
From 26a2aa2a2b4440e170c51c7ee82cba31d65b78ea Mon Sep 17 00:00:00 2001 From: Richard Jones <rjones@redhat.com> Date: Fri, 12 Feb 2010 13:42:57 +0000 Subject: [PATCH 1/2] perl: Don't prefix error messages with command name.
The command name is already being added by the RPC stubs, so adding it again in Perl just results in doubled error messages like: foo: foo: the error --- src/generator.ml | 14 +++++++------- 1 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/src/generator.ml b/src/generator.ml index 607b6d1..b0ffc58 100755 --- a/src/generator.ml +++ b/src/generator.ml @@ -8149,7 +8149,7 @@ DESTROY (g) pr ";\n"; do_cleanups (); pr " if (r == -1)\n"; - pr " croak (\"%s: %%s\", guestfs_last_error (g));\n" name; + pr " croak (\"%%s\", guestfs_last_error (g));\n";
If you search for guestfs_last_error in generator.ml, it looks like there are more of these. Some examples: 8260: do_cleanups (); pr " if (%s == NULL)\n" n; pr " croak (\"%s: %%s\", guestfs_last_error (g));\n" name; 8304: do_cleanups (); pr " if (%s == NULL)\n" n; pr " croak (\"%s: %%s\", guestfs_last_error (g));\n" name; 10314: generate_call (); pr " if (r %s)\n" (c_error_comparison ()); pr " throw new Error (\"%s: \" + guestfs_last_error (_handle));\n" name; Matt -- Matthew Booth, RHCA, RHCSS Red Hat Engineering, Virtualisation Team M: +44 (0)7977 267231 GPG ID: D33C3490 GPG FPR: 3733 612D 2D05 5458 8A8A 1600 3441 EA19 D33C 3490
On Fri, Feb 12, 2010 at 04:45:33PM +0000, Matthew Booth wrote:
On 12/02/10 14:43, Richard W.M. Jones wrote:
From 26a2aa2a2b4440e170c51c7ee82cba31d65b78ea Mon Sep 17 00:00:00 2001 From: Richard Jones <rjones@redhat.com> Date: Fri, 12 Feb 2010 13:42:57 +0000 Subject: [PATCH 1/2] perl: Don't prefix error messages with command name.
The command name is already being added by the RPC stubs, so adding it again in Perl just results in doubled error messages like: foo: foo: the error --- src/generator.ml | 14 +++++++------- 1 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/src/generator.ml b/src/generator.ml index 607b6d1..b0ffc58 100755 --- a/src/generator.ml +++ b/src/generator.ml @@ -8149,7 +8149,7 @@ DESTROY (g) pr ";\n"; do_cleanups (); pr " if (r == -1)\n"; - pr " croak (\"%s: %%s\", guestfs_last_error (g));\n" name; + pr " croak (\"%%s\", guestfs_last_error (g));\n";
If you search for guestfs_last_error in generator.ml, it looks like there are more of these. Some examples:
8260: do_cleanups (); pr " if (%s == NULL)\n" n; pr " croak (\"%s: %%s\", guestfs_last_error (g));\n" name;
8304: do_cleanups (); pr " if (%s == NULL)\n" n; pr " croak (\"%s: %%s\", guestfs_last_error (g));\n" name;
10314: generate_call (); pr " if (r %s)\n" (c_error_comparison ()); pr " throw new Error (\"%s: \" + guestfs_last_error (_handle));\n" name;
Oh dear, indeed there are. Updated patch attached. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones virt-df lists disk usage of guests without needing to install any software inside the virtual machine. Supports Linux and Windows. http://et.redhat.com/~rjones/virt-df/
On 12/02/10 16:49, Richard W.M. Jones wrote:
On Fri, Feb 12, 2010 at 04:45:33PM +0000, Matthew Booth wrote:
On 12/02/10 14:43, Richard W.M. Jones wrote:
From 26a2aa2a2b4440e170c51c7ee82cba31d65b78ea Mon Sep 17 00:00:00 2001 From: Richard Jones <rjones@redhat.com> Date: Fri, 12 Feb 2010 13:42:57 +0000 Subject: [PATCH 1/2] perl: Don't prefix error messages with command name.
The command name is already being added by the RPC stubs, so adding it again in Perl just results in doubled error messages like: foo: foo: the error --- src/generator.ml | 14 +++++++------- 1 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/src/generator.ml b/src/generator.ml index 607b6d1..b0ffc58 100755 --- a/src/generator.ml +++ b/src/generator.ml @@ -8149,7 +8149,7 @@ DESTROY (g) pr ";\n"; do_cleanups (); pr " if (r == -1)\n"; - pr " croak (\"%s: %%s\", guestfs_last_error (g));\n" name; + pr " croak (\"%%s\", guestfs_last_error (g));\n";
If you search for guestfs_last_error in generator.ml, it looks like there are more of these. Some examples:
8260: do_cleanups (); pr " if (%s == NULL)\n" n; pr " croak (\"%s: %%s\", guestfs_last_error (g));\n" name;
8304: do_cleanups (); pr " if (%s == NULL)\n" n; pr " croak (\"%s: %%s\", guestfs_last_error (g));\n" name;
10314: generate_call (); pr " if (r %s)\n" (c_error_comparison ()); pr " throw new Error (\"%s: \" + guestfs_last_error (_handle));\n" name;
Oh dear, indeed there are.
Updated patch attached.
ACK. Matt -- Matthew Booth, RHCA, RHCSS Red Hat Engineering, Virtualisation Team M: +44 (0)7977 267231 GPG ID: D33C3490 GPG FPR: 3733 612D 2D05 5458 8A8A 1600 3441 EA19 D33C 3490
-- 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
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 What on earth is this (linkc:146): reply_with_error ("ln%s%s: %s: %s: %s", flag ? " " : "", flag ? : "", target, linkname, err); 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); 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); parted.c:236: reply_with_error ("parted print: %s: %s", device, wc.c:47: reply_with_error ("wc %s: %s", flag, err); 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/. ACK. Matt -- Matthew Booth, RHCA, RHCSS Red Hat Engineering, Virtualisation Team M: +44 (0)7977 267231 GPG ID: D33C3490 GPG FPR: 3733 612D 2D05 5458 8A8A 1600 3441 EA19 D33C 3490
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
participants (2)
-
Matthew Booth -
Richard W.M. Jones