Libguestfs - Sep 2009 - [PATCH] Add command trace functionality

-- 
Richard Jones, Emerging Technologies, Red Hat  http://et.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
-------------- next part --------------
>From 759de3f6289967a3c9978b4e947a38a4585f404c Mon Sep 17 00:00:00 2001
From: Richard Jones <rjones at trick.home.annexia.org>
Date: Wed, 9 Sep 2009 17:48:30 +0100
Subject: [PATCH] Add command trace functionality.

Enable this by calling guestfs_trace (handle, 1) or by
setting the LIBGUESTFS_TRACE=1 environment variable.
---
 guestfish.pod    |    4 +++
 guestfs.pod      |    5 +++
 src/generator.ml |   74 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 src/guestfs.c    |   17 ++++++++++++
 4 files changed, 100 insertions(+), 0 deletions(-)

diff --git a/guestfish.pod b/guestfish.pod
index 5427b23..3afc967 100644
--- a/guestfish.pod
+++ b/guestfish.pod
@@ -575,6 +575,10 @@ Set the default qemu binary that libguestfs uses.  If not
set, then
 the qemu which was found at compile time by the configure script is
 used.
 
+=item LIBGUESTFS_TRACE
+
+Set C<LIBGUESTFS_TRACE=1> to enable command traces.
+
 =item PAGER
 
 The C<more> command uses C<$PAGER> as the pager.  If not
diff --git a/guestfs.pod b/guestfs.pod
index d8e4da3..b8379d0 100644
--- a/guestfs.pod
+++ b/guestfs.pod
@@ -983,6 +983,11 @@ used.
 
 See also L</QEMU WRAPPERS> above.
 
+=item LIBGUESTFS_TRACE
+
+Set C<LIBGUESTFS_TRACE=1> to enable command traces.  This
+has the same effect as calling C<guestfs_set_trace (handle, 1)>.
+
 =item TMPDIR
 
 Location of temporary directory, defaults to C</tmp>.
diff --git a/src/generator.ml b/src/generator.ml
index 765cb16..6184890 100755
--- a/src/generator.ml
+++ b/src/generator.ml
@@ -805,6 +805,32 @@ is passed to the appliance at boot time.  See
C<guestfs_set_selinux>.
 For more information on the architecture of libguestfs,
 see L<guestfs(3)>.");
 
+  ("set_trace", (RErr, [Bool "trace"]), -1, [FishAlias
"trace"],
+   [InitNone, Always, TestOutputTrue (
+      [["set_trace"; "true"];
+       ["get_trace"]])],
+   "enable or disable command traces",
+   "\
+If the command trace flag is set to 1, then commands are
+printed on stdout before they are executed in a format
+which is very similar to the one used by guestfish.  In
+other words, you can run a program with this enabled, and
+you will get out a script which you can feed to guestfish
+to perform the same set of actions.
+
+If you want to trace C API calls into libguestfs (and
+other libraries) then possibly a better way is to use
+the external ltrace(1) command.
+
+Command traces are disabled unless the environment variable
+C<LIBGUESTFS_TRACE> is defined and set to C<1>.");
+
+  ("get_trace", (RBool "trace", []), -1, [],
+   [],
+   "get command trace enabled flag",
+   "\
+Return the command trace flag.");
+
 ]
 
 (* daemon_functions are any functions which cause some action
@@ -4630,6 +4656,52 @@ check_state (guestfs_h *g, const char *caller)
 
 ";
 
+  (* Generate code to generate guestfish call traces. *)
+  let trace_call shortname style +    pr "  if (guestfs__get_trace (g))
{\n";
+
+    let needs_i +      List.exists (function
+		   | StringList _ | DeviceList _ -> true
+		   | _ -> false) (snd style) in
+    if needs_i then (
+      pr "    int i;\n";
+      pr "\n"
+    );
+
+    pr "    printf (\"%%s\", \"%s\");\n"
shortname;
+    List.iter (
+      function
+      | String n			(* strings *)
+      | Device n
+      | Pathname n
+      | Dev_or_Path n
+      | FileIn n
+      | FileOut n ->
+	  (* guestfish doesn't support string escaping, so neither do we *)
+	  pr "    printf (\" \\\"%%s\\\"\", %s);\n" n
+      | OptString n ->			(* string option *)
+	  pr "    if (%s) printf (\" \\\"%%s\\\"\",
%s);\n" n n;
+	  pr "    else printf (\" null\");\n"
+      | StringList n
+      | DeviceList n ->			(* string list *)
+	  pr "    printf (\"\\\"\");\n";
+	  pr "    for (i = 0; %s[i]; ++i) {\n" n;
+	  pr "      if (i > 0) putchar (' ');\n";
+	  pr "      printf (\"%%s\", %s[i]);\n" n;
+	  pr "    }\n";
+	  pr "    printf (\"\\\"\");\n";
+      | Bool n ->			(* boolean *)
+	  pr "    if (%s) printf (\" true\");\n" n;
+	  pr "    else printf (\" false\");\n"
+      | Int n ->			(* int *)
+	  pr "    printf (\" %%d\", %s);\n" n
+    ) (snd style);
+    pr "    printf (\"\\n\");\n";
+    pr "  }\n";
+    pr "\n";
+  in
+
   (* For non-daemon functions, generate a wrapper around each function. *)
   List.iter (
     fun (shortname, style, _, _, _, _, _) ->
@@ -4638,6 +4710,7 @@ check_state (guestfs_h *g, const char *caller)
       generate_prototype ~extern:false ~semicolon:false ~newline:true
         ~handle:"g" name style;
       pr "{\n";
+      trace_call shortname style;
       pr "  return guestfs__%s " shortname;
       generate_c_call_args ~handle:"g" style;
       pr ";\n";
@@ -4745,6 +4818,7 @@ check_state (guestfs_h *g, const char *caller)
       pr "  guestfs_main_loop *ml = guestfs_get_main_loop (g);\n";
       pr "  int serial;\n";
       pr "\n";
+      trace_call shortname style;
       pr "  if (check_state (g, \"%s\") == -1) return
%s;\n" name error_code;
       pr "  guestfs_set_busy (g);\n";
       pr "\n";
diff --git a/src/guestfs.c b/src/guestfs.c
index 571205f..98d99b8 100644
--- a/src/guestfs.c
+++ b/src/guestfs.c
@@ -170,6 +170,7 @@ struct guestfs_h
   int cmdline_size;
 
   int verbose;
+  int trace;
   int autosync;
 
   char *path;			/* Path to kernel, initrd. */
@@ -238,6 +239,9 @@ guestfs_create (void)
   str = getenv ("LIBGUESTFS_DEBUG");
   g->verbose = str != NULL && strcmp (str, "1") == 0;
 
+  str = getenv ("LIBGUESTFS_TRACE");
+  g->trace = str != NULL && strcmp (str, "1") == 0;
+
   str = getenv ("LIBGUESTFS_PATH");
   g->path = str != NULL ? strdup (str) : strdup (GUESTFS_DEFAULT_PATH);
   if (!g->path) goto error;
@@ -734,6 +738,19 @@ guestfs__version (guestfs_h *g)
   return r;
 }
 
+int
+guestfs__set_trace (guestfs_h *g, int t)
+{
+  g->trace = !!t;
+  return 0;
+}
+
+int
+guestfs__get_trace (guestfs_h *g)
+{
+  return g->trace;
+}
+
 /* Add a string to the current command line. */
 static void
 incr_cmdline_size (guestfs_h *g)
-- 
1.6.2.5

On 09/09/09 17:49, Richard W.M. Jones wrote:
>> From 759de3f6289967a3c9978b4e947a38a4585f404c Mon Sep 17 00:00:00 2001
> From: Richard Jones<rjones at trick.home.annexia.org>
> Date: Wed, 9 Sep 2009 17:48:30 +0100
> Subject: [PATCH] Add command trace functionality.
>
> Enable this by calling guestfs_trace (handle, 1) or by
> setting the LIBGUESTFS_TRACE=1 environment variable.
> ---
>   guestfish.pod    |    4 +++
>   guestfs.pod      |    5 +++
>   src/generator.ml |   74
++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   src/guestfs.c    |   17 ++++++++++++
>   4 files changed, 100 insertions(+), 0 deletions(-)
>
> diff --git a/guestfish.pod b/guestfish.pod
> index 5427b23..3afc967 100644
> --- a/guestfish.pod
> +++ b/guestfish.pod
> @@ -575,6 +575,10 @@ Set the default qemu binary that libguestfs uses.  If
not set, then
>   the qemu which was found at compile time by the configure script is
>   used.
>
> +=item LIBGUESTFS_TRACE
> +
> +Set C<LIBGUESTFS_TRACE=1>  to enable command traces.
> +
>   =item PAGER
>
>   The C<more>  command uses C<$PAGER>  as the pager.  If not
> diff --git a/guestfs.pod b/guestfs.pod
> index d8e4da3..b8379d0 100644
> --- a/guestfs.pod
> +++ b/guestfs.pod
> @@ -983,6 +983,11 @@ used.
>
>   See also L</QEMU WRAPPERS>  above.
>
> +=item LIBGUESTFS_TRACE
> +
> +Set C<LIBGUESTFS_TRACE=1>  to enable command traces.  This
> +has the same effect as calling C<guestfs_set_trace (handle, 1)>.
> +
>   =item TMPDIR
>
>   Location of temporary directory, defaults to C</tmp>.
> diff --git a/src/generator.ml b/src/generator.ml
> index 765cb16..6184890 100755
> --- a/src/generator.ml
> +++ b/src/generator.ml
> @@ -805,6 +805,32 @@ is passed to the appliance at boot time.  See
C<guestfs_set_selinux>.
>   For more information on the architecture of libguestfs,
>   see L<guestfs(3)>.");
>
> +  ("set_trace", (RErr, [Bool "trace"]), -1, [FishAlias
"trace"],
> +   [InitNone, Always, TestOutputTrue (
> +      [["set_trace"; "true"];
> +       ["get_trace"]])],
> +   "enable or disable command traces",
> +   "\
> +If the command trace flag is set to 1, then commands are
> +printed on stdout before they are executed in a format
> +which is very similar to the one used by guestfish.  In
> +other words, you can run a program with this enabled, and
> +you will get out a script which you can feed to guestfish
> +to perform the same set of actions.
> +
> +If you want to trace C API calls into libguestfs (and
> +other libraries) then possibly a better way is to use
> +the external ltrace(1) command.
> +
> +Command traces are disabled unless the environment variable
> +C<LIBGUESTFS_TRACE>  is defined and set to C<1>.");
> +
> +  ("get_trace", (RBool "trace", []), -1, [],
> +   [],
> +   "get command trace enabled flag",
> +   "\
> +Return the command trace flag.");
> +
>   ]
>
>   (* daemon_functions are any functions which cause some action
> @@ -4630,6 +4656,52 @@ check_state (guestfs_h *g, const char *caller)
>
>   ";
>
> +  (* Generate code to generate guestfish call traces. *)
> +  let trace_call shortname style > +    pr "  if
(guestfs__get_trace (g)) {\n";
> +
> +    let needs_i > +      List.exists (function
> +		   | StringList _ | DeviceList _ ->  true
> +		   | _ ->  false) (snd style) in
> +    if needs_i then (
> +      pr "    int i;\n";
> +      pr "\n"
> +    );
> +
> +    pr "    printf (\"%%s\", \"%s\");\n"
shortname;
> +    List.iter (
> +      function
> +      | String n			(* strings *)
> +      | Device n
> +      | Pathname n
> +      | Dev_or_Path n
> +      | FileIn n
> +      | FileOut n ->
> +	  (* guestfish doesn't support string escaping, so neither do we *)
> +	  pr "    printf (\" \\\"%%s\\\"\", %s);\n"
n
> +      | OptString n ->			(* string option *)
> +	  pr "    if (%s) printf (\" \\\"%%s\\\"\",
%s);\n" n n;
> +	  pr "    else printf (\" null\");\n"
> +      | StringList n
> +      | DeviceList n ->			(* string list *)
> +	  pr "    printf (\"\\\"\");\n";
There's a missing leading space in the above line.
> +	  pr "    for (i = 0; %s[i]; ++i) {\n" n;
> +	  pr "      if (i>  0) putchar (' ');\n";
> +	  pr "      printf (\"%%s\", %s[i]);\n" n;
> +	  pr "    }\n";
> +	  pr "    printf (\"\\\"\");\n";
As discussed on the list, this results in commands which can't be 
executed in guestfish because argument groupings are lost when arguments 
contain spaces. While this is a limitation of guestfish rather than this 
patch, a fix in guestfish will require a modification to this patch. Can 
we hold off on this until guestfish is fixed?
> +      | Bool n ->			(* boolean *)
> +	  pr "    if (%s) printf (\" true\");\n" n;
> +	  pr "    else printf (\" false\");\n"
> +      | Int n ->			(* int *)
> +	  pr "    printf (\" %%d\", %s);\n" n
> +    ) (snd style);
> +    pr "    printf (\"\\n\");\n";
> +    pr "  }\n";
> +    pr "\n";
> +  in
> +
>     (* For non-daemon functions, generate a wrapper around each function.
*)
>     List.iter (
>       fun (shortname, style, _, _, _, _, _) ->
> @@ -4638,6 +4710,7 @@ check_state (guestfs_h *g, const char *caller)
>         generate_prototype ~extern:false ~semicolon:false ~newline:true
>           ~handle:"g" name style;
>         pr "{\n";
> +      trace_call shortname style;
>         pr "  return guestfs__%s " shortname;
>         generate_c_call_args ~handle:"g" style;
>         pr ";\n";
> @@ -4745,6 +4818,7 @@ check_state (guestfs_h *g, const char *caller)
>         pr "  guestfs_main_loop *ml = guestfs_get_main_loop
(g);\n";
>         pr "  int serial;\n";
>         pr "\n";
> +      trace_call shortname style;
>         pr "  if (check_state (g, \"%s\") == -1) return
%s;\n" name error_code;
>         pr "  guestfs_set_busy (g);\n";
>         pr "\n";
> diff --git a/src/guestfs.c b/src/guestfs.c
> index 571205f..98d99b8 100644
> --- a/src/guestfs.c
> +++ b/src/guestfs.c
> @@ -170,6 +170,7 @@ struct guestfs_h
>     int cmdline_size;
>
>     int verbose;
> +  int trace;
>     int autosync;
>
>     char *path;			/* Path to kernel, initrd. */
> @@ -238,6 +239,9 @@ guestfs_create (void)
>     str = getenv ("LIBGUESTFS_DEBUG");
>     g->verbose = str != NULL&&  strcmp (str, "1") ==
0;
>
> +  str = getenv ("LIBGUESTFS_TRACE");
> +  g->trace = str != NULL&&  strcmp (str, "1") == 0;
> +
>     str = getenv ("LIBGUESTFS_PATH");
>     g->path = str != NULL ? strdup (str) : strdup
(GUESTFS_DEFAULT_PATH);
>     if (!g->path) goto error;
> @@ -734,6 +738,19 @@ guestfs__version (guestfs_h *g)
>     return r;
>   }
>
> +int
> +guestfs__set_trace (guestfs_h *g, int t)
> +{
> +  g->trace = !!t;
> +  return 0;
> +}
> +
> +int
> +guestfs__get_trace (guestfs_h *g)
> +{
> +  return g->trace;
> +}
> +
>   /* Add a string to the current command line. */
>   static void
>   incr_cmdline_size (guestfs_h *g)
> -- 1.6.2.5
I really like this. It *almost* works in practise. I've found 2 limitations:

String list splitting doesn't work in guestfish. See note above and 
expect a patch.

It spits out internal state changes. Look at this output:
add_drive
"/var/lib/virt-snapshot/images/RHEL39FV32-hda-1252577522.qcow2"
config "-drive" 
"file=/var/lib/virt-snapshot/images/RHEL39FV32-hda-1252577522.qcow2,cache=off,if=ide"
add_cdrom "/tmp/4BQ6oiKKAJ.iso"
config "-cdrom" "/tmp/4BQ6oiKKAJ.iso"
set_selinux true
launch
wait_ready
list_partitions
is_ready
set_busy
end_busy
pvs
is_ready
set_busy
end_busy
lvs
is_ready
set_busy
end_busy
file "/dev/sda1"
is_ready

We also discussed this on the list. I don't think this is a problem with 
this patch as such, but that state transition functions are part of the 
external API. These need to be removed from *all* bindings, including 
guestfish.

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 W.M. Jones wrote:
...
A few small suggestions on the generated code:
> diff --git a/src/generator.ml b/src/generator.ml
...
> +    pr "    printf (\"%%s\", \"%s\");\n"
shortname;
> +    List.iter (
> +      function
> +      | String n			(* strings *)
> +      | Device n
> +      | Pathname n
> +      | Dev_or_Path n
> +      | FileIn n
> +      | FileOut n ->
> +	  (* guestfish doesn't support string escaping, so neither do we *)
> +	  pr "    printf (\" \\\"%%s\\\"\", %s);\n"
n
> +      | OptString n ->			(* string option *)
> +	  pr "    if (%s) printf (\" \\\"%%s\\\"\",
%s);\n" n n;
> +	  pr "    else printf (\" null\");\n"
> +      | StringList n
> +      | DeviceList n ->			(* string list *)
> +	  pr "    printf (\"\\\"\");\n";
How about this, instead?

    	  pr "    putchar ('\"');\n";
> +	  pr "    for (i = 0; %s[i]; ++i) {\n" n;
> +	  pr "      if (i > 0) putchar (' ');\n";
> +	  pr "      printf (\"%%s\", %s[i]);\n" n;
The minimalist in me prefers to use fputs in cases like this:
    	  pr "      fputs (%s[i], stdout);\n" n;
> +	  pr "    }\n";
> +	  pr "    printf (\"\\\"\");\n";
Same as above.
> +      | Bool n ->			(* boolean *)
> +	  pr "    if (%s) printf (\" true\");\n" n;
> +	  pr "    else printf (\" false\");\n"
you might like to replace the two lines above with this one:
    	  pr "    printf (%s ? \" true\" : \"
false\");\n" n;
or this,
    	  pr "    fputs (%s ? \" true\" : \" false\",
stdout);\n" n;
> +      | Int n ->			(* int *)
> +	  pr "    printf (\" %%d\", %s);\n" n
> +    ) (snd style);
> +    pr "    printf (\"\\n\");\n";
> +    pr "  }\n";
> +    pr "\n";
> +  in