[Libguestfs] [PATCH libguestfs 15/23] generator.ml: new type, "Pathname"

[Richard W.M. Jones]

On Wed, Aug 12, 2009 at 06:52:51PM +0200, Jim Meyering wrote:
> From: Jim Meyering <meyering at redhat.com>
> 
> * src/generator.ml: Emit NEED_ROOT and ABS_PATH into generated
> stubs.c, rather than requiring they be added manually at the start
> of each and every do_* function that operates on a "path" parameter.
> Update grammar: Pathname is just a String, with the above exception.

> @@ -532,7 +533,7 @@ Return the current qemu binary.
>  This is always non-NULL.  If it wasn't set already, then this will
>  return the default qemu binary name.");
> 
> -  ("set_path", (RErr, [String "path"]), -1, [FishAlias "path"],
> +  ("set_path", (RErr, [Pathname "path"]), -1, [FishAlias "path"],
>     [],
>     "set the search path",
>     "\

This is wrong.  The path here is the :-separated path for finding the
appliance:

http://libguestfs.org/guestfs.3.html#path

None of the non-daemon functions should be touched by your patches,
because they are handled on the library side and don't go through to
the daemon.

> @@ -1066,20 +1067,20 @@ On success this returns a pair containing the
>  number of nodes in the nodeset, and a boolean flag
>  if a node was created.");
> 
> -  ("aug_get", (RString "val", [String "path"]), 19, [],
> +  ("aug_get", (RString "val", [Pathname "path"]), 19, [],
>     [], (* XXX Augeas code needs tests. *)
>     "look up the value of an Augeas path",
>     "\
>  Look up the value associated with C<path>.  If C<path>
>  matches exactly one node, the C<value> is returned.");
> 
> -  ("aug_set", (RErr, [String "path"; String "val"]), 20, [],
> +  ("aug_set", (RErr, [Pathname "path"; String "val"]), 20, [],
>     [], (* XXX Augeas code needs tests. *)
>     "set Augeas path to value",
>     "\
>  Set the value associated with C<path> to C<value>.");
> 
> -  ("aug_insert", (RErr, [String "path"; String "label"; Bool "before"]), 21, [],
> +  ("aug_insert", (RErr, [Pathname "path"; String "label"; Bool "before"]), 21, [],
>     [], (* XXX Augeas code needs tests. *)
>     "insert a sibling Augeas node",
>     "\
> @@ -1091,7 +1092,7 @@ C<path> must match exactly one existing node in the tree, and
>  C<label> must be a label, ie. not contain C</>, C<*> or end
>  with a bracketed index C<[N]>.");
> 
> -  ("aug_rm", (RInt "nrnodes", [String "path"]), 22, [],
> +  ("aug_rm", (RInt "nrnodes", [Pathname "path"]), 22, [],
>     [], (* XXX Augeas code needs tests. *)
>     "remove an Augeas path",
>     "\
> @@ -1106,7 +1107,7 @@ On success this returns the number of entries which were removed.");
>  Move the node C<src> to C<dest>.  C<src> must match exactly
>  one node.  C<dest> is overwritten if it exists.");
> 
> -  ("aug_match", (RStringList "matches", [String "path"]), 24, [],
> +  ("aug_match", (RStringList "matches", [Pathname "path"]), 24, [],
>     [], (* XXX Augeas code needs tests. *)
>     "return Augeas nodes which match path",
>     "\
> @@ -1132,14 +1133,14 @@ Load files into the tree.
>  See C<aug_load> in the Augeas documentation for the full gory
>  details.");
> 
> -  ("aug_ls", (RStringList "matches", [String "path"]), 28, [],
> +  ("aug_ls", (RStringList "matches", [Pathname "path"]), 28, [],
>     [], (* XXX Augeas code needs tests. *)
>     "list Augeas nodes under a path",
>     "\
>  This is just a shortcut for listing C<guestfs_aug_match>
>  C<path/*> and sorting the resulting nodes into alphabetical order.");

These Augeas changes are dubious.  Augeas paths != real paths:

http://augeas.net/docs/tree.html

> 
> -  ("mkmountpoint", (RErr, [String "path"]), 148, [],
> +  ("mkmountpoint", (RErr, [Pathname "path"]), 148, [],
>     [],
>     "create a mountpoint",
>     "\
> @@ -3026,7 +3027,7 @@ in guestfish:
> 
>  The inner filesystem is now unpacked under the /ext3 mountpoint.");
> 
> -  ("rmmountpoint", (RErr, [String "path"]), 149, [],
> +  ("rmmountpoint", (RErr, [Pathname "path"]), 149, [],
>     [],
>     "remove a mountpoint",
>     "\

I guess as discussed on IRC a future patch will change these.
Nevertheless checking for root mounted in these two calls is wrong.

> @@ -4703,7 +4704,10 @@ and generate_daemon_actions () =
>             pr "  }\n";
>             List.iter (
>               function
> -             | Device n ->
> +             | Pathname n ->
> +                 pr "  NEED_ROOT (goto done);\n";
> +                 pr "  ABS_PATH (%s, goto done);\n" n;
> +	     | Device n ->
>                   pr "  %s = args.%s;\n" n n;
>                   pr "  RESOLVE_DEVICE (%s, goto done);" n;
>               | String n -> pr "  %s = args.%s;\n" n n

NEED_ROOT once - future patch?

Rich.

-- 
Richard Jones, Emerging Technologies, Red Hat  http://et.redhat.com/~rjones
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://et.redhat.com/~rjones/virt-top