[Libguestfs] [hivex][PATCH 2/8] generator: Add new return type to ABI: RLenValue

[Richard W.M. Jones]

On Wed, Oct 19, 2011 at 04:53:30PM -0700, Alex Nelson wrote:
> @@ -1622,6 +1629,7 @@ and generate_ocaml_prototype ?(is_external = false) name style =
[...]
> +   | RLenValue -> pr "value * int"

The declared return type is a pair (value * int), but ...

> @@ -1981,6 +1997,20 @@ copy_type_len (size_t len, hive_type t)
>  }
>  
>  static value
> +copy_len_value (size_t len, hive_value_h r)
> +{
> +  CAMLparam0 ();
> +  CAMLlocal2 (v, rv);
> +
> +  rv = caml_alloc (2, 0);
> +  v = Val_int (len);
> +  Store_field (rv, 0, v);
> +  v = Val_int (r);
> +  Store_field (rv, 1, v);
> +  CAMLreturn (rv);
> +}

... what you're actually returning is the length (field 0) and the
value (field 1).  So one of these definitions is backwards.

There is a test in a later patch, but it didn't pick up the bug.  Not
surprising: All of the tests check a cell that returns (0,0), so none
of the tests could have picked up this type of bug.  I think the tests
(all of them) need to be changed to be more thorough.

> @@ -2244,6 +2275,7 @@ and generate_perl_prototype name style =
>     | RString -> pr "$string = "
>     | RStringList -> pr "@strings = "
>     | RLenType -> pr "($type, $len) = "
> +   | RLenValue -> pr "($len, $value) = "
>     | RLenTypeVal -> pr "($type, $data) = "
>     | RInt32 -> pr "$int32 = "
>     | RInt64 -> pr "$int64 = "
> @@ -2467,6 +2499,7 @@ DESTROY (h)
>  	 | RValueList
>  	 | RStringList
>  	 | RLenType
> +	 | RLenValue
>  	 | RLenTypeVal -> pr "void\n"
>  	 | RInt32 -> pr "SV *\n"
>  	 | RInt64 -> pr "SV *\n"
> @@ -2639,6 +2672,22 @@ DESTROY (h)
>  	     pr "      PUSHs (sv_2mortal (newSViv (type)));\n";
>  	     pr "      PUSHs (sv_2mortal (newSViv (len)));\n";
>  
> +	 | RLenValue ->
> +	     pr "PREINIT:\n";
> +	     pr "      hive_value_h r;\n";
> +	     pr "      size_t len;\n";
> +	     pr " PPCODE:\n";
> +             pr "      errno = 0;\n";
> +             pr "      r = hivex_%s (%s, &len);\n"
> +	       name (String.concat ", " c_params);
> +	     free_args ();
> +             pr "      if (r == 0 && errno)\n";
> +             pr "        croak (\"%%s: \", \"%s\", strerror (errno));\n"
> +	       name;
> +	     pr "      EXTEND (SP, 2);\n";
> +	     pr "      PUSHs (sv_2mortal (newSViv (len)));\n";
> +	     pr "      PUSHs (sv_2mortal (newSViv (r)));\n";

I can't remember now the details of which order you have to push
return values in Perl, but there may or may not be the same bug here
too.  The test wouldn't pick it up.

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/