8:40 a.m.
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
8:43 a.m.
New subject: [PATCH 1/2] perl: Don't prefix error messages with command name.
--
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
10:45 a.m.
New subject: [PATCH 1/2] perl: Don't prefix error messages with command name.
On 12/02/10 14:43, Richard W.M. Jones wrote:
>From 26a2aa2a2b4440e170c51c7ee82cba31d65b78ea Mon Sep 17 00:00:00
2001
From: Richard Jones <rjones(a)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
10:49 a.m.
New subject: [PATCH 1/2] perl: Don't prefix error messages with command name.
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(a)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/
11:23 a.m.
New subject: [PATCH 1/2] perl: Don't prefix error messages with command name.
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(a)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
8:43 a.m.
New subject: [PATCH 2/2] daemon: Don't need to prefix error messages with the command name.
--
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
11:17 a.m.
New subject: [PATCH 2/2] daemon: Don't need to prefix error messages with the command name.
On 12/02/10 14:43, Richard W.M. Jones wrote:
From: Richard Jones <rjones(a)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
12:44 p.m.
New subject: [PATCH 2/2] daemon: Don't need to prefix error messages with the command name.
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(a)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
5431
days inactive
5431
days old
7 comments
2 participants
participants (2)
-
Matthew Booth -
Richard W.M. Jones