[Libguestfs] [PATCH 2/2] daemon: Don't need to prefix error messages with the command name.

[Richard W.M. Jones]

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