[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [Libguestfs] [PATCH 3/7] generator: Add new return types to ABI: RLenNode and RLenValue



Hi Rich,

Sorry for letting this one sit for a while.

I decided the appropriate way to make sure everything was correct was to just write some unit tests and include them for runs with 'make check' - a bit more than you said to do, but I think it makes sense.  Unfortunately, an OCaml type error is biting me, and I'm hung on that for verifying the OCaml bindings.

In the attached patch, the test ocaml/t/hivex_120_rlenvalue.ml has an error arising from the type abbreviation (is that the right term here? Or is it "synonym"?) of "value" and "int".  From what I can do at the OCaml prompt (pwd=.../hivex/ocaml), this is the confusion:
# (2:Hivex.value);;
Error: This expression has type int but an expression was expected of type
         Hivex.value
#
(The 2 is underlined to specify the error.)

I get similar confusion later from the format string and trying to coerce the returned values to vanilla ints.  (I semi-understand "coerce" is also the wrong term to use here, since that's apparently a term for strictly-ordered subclass abstraction.)

May I please have your help with this type confusion?

Aside:  The attached patch is not intended to be the final correction of this patch 3/7.  It's all the remaining unapplied of the work from the 7-patch series, which I will break up as previously planned.

--Alex

Attachment: almost.patch
Description: Binary data



On Sep 6, 2011, at 10:00 , Richard W.M. Jones wrote:

> There are still a few mistakes.  See my comments inline.
> 
>> From 02ce84fd959bf67afc6999eaeecef78ccdf57c71 Mon Sep 17 00:00:00 2001
>> From: Alex Nelson <ajnelson cs ucsc edu>
>> Date: Tue, 6 Sep 2011 09:27:51 -0700
>> Subject: [PATCH] generator: Add new return type to ABI: RLenValue
>> 
>> RLenValue is are similar to RLenType, though with one less argument.
>> This required adding one processing function for OCaml and Python.
>> 
>> Signed-off-by: Alex Nelson <ajnelson cs ucsc edu>
>> ---
>> generator/generator.ml |   69 +++++++++++++++++++++++++++++++++++++++++++++++
>> 1 files changed, 69 insertions(+), 0 deletions(-)
>> 
>> diff --git a/generator/generator.ml b/generator/generator.ml
>> index b6fb8b3..2703023 100755
>> --- a/generator/generator.ml
>> +++ b/generator/generator.ml
>> @@ -51,6 +51,7 @@ and ret =
>>   | RNodeList                           (* Returns hive_node_h* or NULL. *)
>>   | RValue                              (* Returns hive_value_h or 0. *)
>>   | RValueList                          (* Returns hive_value_h* or NULL. *)
>> +  | RLenValue                           (* See value_struct_length. *)
> 
> I think the comment should read "see value_data_cell_offset", but I'd
> prefer it if the comment was "Returns offset and length of value"
> since that describes what it actually does.
> 
>>   | RString                             (* Returns char* or NULL. *)
>>   | RStringList                         (* Returns char** or NULL. *)
>>   | RLenType                            (* See hivex_value_type. *)
>> @@ -890,6 +891,7 @@ and generate_c_prototype ?(extern = false) name style =
>>    | RValueList -> pr "hive_value_h *"
>>    | RString -> pr "char *"
>>    | RStringList -> pr "char **"
>> +   | RLenValue -> pr "hive_value_h "
>>    | RLenType -> pr "int "
>>    | RLenTypeVal -> pr "char *"
>>    | RInt32 -> pr "int32_t "
>> @@ -911,6 +913,7 @@ and generate_c_prototype ?(extern = false) name style =
>>   ) (snd style);
>>   (match fst style with
>>    | RLenType | RLenTypeVal -> pr ", hive_type *t, size_t *len"
>> +   | RLenValue -> pr ", size_t *len"
>>    | _ -> ()
>>   );
>>   pr ");\n"
>> @@ -1113,6 +1116,10 @@ On error this returns NULL and sets errno.\n\n"
>>            pr "\
>> Returns 0 on success.
>> On error this returns -1 and sets errno.\n\n"
>> +       | RLenValue ->
>> +           pr "\
>> +Returns a positive number on success.
>> +On error this returns 0 and sets errno.\n\n"
> 
> This should be:
> 
>  Returns a value handle.
>  On error this returns 0 and sets errno.\n\n"
> 
>> @@ -1687,6 +1695,7 @@ static hive_type HiveType_val (value);
>> static value Val_hive_type (hive_type);
>> static value copy_int_array (size_t *);
>> static value copy_type_len (size_t, hive_type);
>> +static value copy_len (size_t);
>> static value copy_type_value (const char *, size_t, hive_type);
>> static void raise_error (const char *) Noreturn;
>> static void raise_closed (const char *) Noreturn;
>> @@ -1709,6 +1718,7 @@ static void raise_closed (const char *) Noreturn;
>>       let c_params =
>>         match fst style with
>>         | RLenType | RLenTypeVal -> c_params @ [["&t"; "&len"]]
>> +        | RLenValue -> c_params @ [["&len"]]
>>         | _ -> c_params in
>>       let c_params = List.concat c_params in
>> 
>> @@ -1781,6 +1791,10 @@ static void raise_closed (const char *) Noreturn;
>>             pr "  size_t len;\n";
>>             pr "  hive_type t;\n";
>>             "-1"
>> +        | RLenValue ->
>> +            pr "  int r;\n";
>> +            pr "  size_t len;\n";
>> +            "0"
> 
> 'r' is not an int.  It's a hive_value_h.
> 
>>         | RLenTypeVal ->
>>             pr "  char *r;\n";
>>             pr "  size_t len;\n";
>> @@ -1861,6 +1875,7 @@ static void raise_closed (const char *) Noreturn;
>>            pr "  for (int i = 0; r[i] != NULL; ++i) free (r[i]);\n";
>>            pr "  free (r);\n"
>>        | RLenType -> pr "  rv = copy_type_len (len, t);\n"
>> +       | RLenValue -> pr "  rv = copy_len (len);\n"
> 
> This call is wrong, and so is copy_len below.  What happens to 'r'?
> 
>>        | RLenTypeVal ->
>>            pr "  rv = copy_type_value (r, len, t);\n";
>>            pr "  free (r);\n"
>> @@ -1983,6 +1998,18 @@ copy_type_len (size_t len, hive_type t)
>> }
>> 
>> static value
>> +copy_len (size_t len)
>> +{
>> +  CAMLparam0 ();
>> +  CAMLlocal2 (v, rv);
>> +
>> +  rv = caml_alloc (1, 0);
>> +  v = Val_int (len);
>> +  Store_field (rv, 0, v);
>> +  CAMLreturn (rv);
>> +}
> 
> This creates a tuple with a single element, which OCaml can't actually
> process.  In any case you need to be passing two parameters here: the
> value and the len, and you need to make a 2-element tuple containing
> both of them.
> 
>> +static value
>> copy_type_value (const char *r, size_t len, hive_type t)
>> {
>>   CAMLparam0 ();
>> @@ -2172,6 +2199,7 @@ sub open {
>> 	 | RString
>> 	 | RStringList
>> 	 | RLenType
>> +	 | RLenValue
>> 	 | RLenTypeVal
>> 	 | RInt32
>> 	 | RInt64 -> ()
>> @@ -2246,6 +2274,7 @@ and generate_perl_prototype name style =
>>    | RString -> pr "$string = "
>>    | RStringList -> pr "@strings = "
>>    | RLenType -> pr "($type, $len) = "
>> +   | RLenValue -> pr "($len) = "
> 
> Just a single length?  What about the value?
> 
>>    | RLenTypeVal -> pr "($type, $data) = "
>>    | RInt32 -> pr "$int32 = "
>>    | RInt64 -> pr "$int64 = "
>> @@ -2469,6 +2498,7 @@ DESTROY (h)
>> 	 | RValueList
>> 	 | RStringList
>> 	 | RLenType
>> +	 | RLenValue
>> 	 | RLenTypeVal -> pr "void\n"
>> 	 | RInt32 -> pr "SV *\n"
>> 	 | RInt64 -> pr "SV *\n"
>> @@ -2641,6 +2671,20 @@ DESTROY (h)
>> 	     pr "      PUSHs (sv_2mortal (newSViv (type)));\n";
>> 	     pr "      PUSHs (sv_2mortal (newSViv (len)));\n";
>> 
>> +	 | RLenValue ->
>> +	     pr "PREINIT:\n";
>> +	     pr "      int r;\n";
> 
> int r?  The return from the function is a hive_value_h.
> 
>> +	     pr "      size_t len;\n";
>> +	     pr " PPCODE:\n";
>> +             pr "      r = hivex_%s (%s, &len);\n"
>> +	       name (String.concat ", " c_params);
>> +	     free_args ();
>> +             pr "      if (r == 0)\n";
>> +             pr "        croak (\"%%s: \", \"%s\", strerror (errno));\n"
>> +	       name;
>> +	     pr "      EXTEND (SP, 2);\n";
>> +	     pr "      PUSHs (sv_2mortal (newSViv (len)));\n";
> 
> You extend the stack by two elements, then push one element.  This
> would cause Perl to crash.  In any case you need to push both the
> value (r) and the length.  I can't remember in which order you need to
> push them -- take a look at other examples.
> 
>> 	 | RLenTypeVal ->
>> 	     pr "PREINIT:\n";
>> 	     pr "      char *r;\n";
>> @@ -2879,6 +2923,14 @@ put_len_type (size_t len, hive_type t)
>> }
>> 
>> static PyObject *
>> +put_len (size_t len)
>> +{
>> +  PyObject *r = PyTuple_New (1);
>> +  PyTuple_SetItem (r, 0, PyLong_FromLongLong ((long) len));
>> +  return r;
>> +}
> 
> Same problem in Python as in OCaml and Perl.
> 
> It's probably going to help if you make some small test programs in
> all languages, so you can check the returned values are marshalled
> properly.
> 
>> +static PyObject *
>> put_val_type (char *val, size_t len, hive_type t)
>> {
>>   PyObject *r = PyTuple_New (2);
>> @@ -2918,6 +2970,10 @@ put_val_type (char *val, size_t len, hive_type t)
>>             pr "  size_t len;\n";
>>             pr "  hive_type t;\n";
>>             "-1"
>> +        | RLenValue ->
>> +            pr "  int r;\n";
> 
> As above.
> 
>> +            pr "  size_t len;\n";
>> +            "0"
>>         | RLenTypeVal ->
>>             pr "  char *r;\n";
>>             pr "  size_t len;\n";
>> @@ -2942,6 +2998,7 @@ put_val_type (char *val, size_t len, hive_type t)
>>       let c_params =
>>         match fst style with
>>         | RLenType | RLenTypeVal -> c_params @ ["&t"; "&len"]
>> +        | RLenValue -> c_params @ ["&len"]
>>         | _ -> c_params in
>> 
>>       List.iter (
>> @@ -3086,6 +3143,8 @@ put_val_type (char *val, size_t len, hive_type t)
>>            pr "  free_strings (r);\n"
>>        | RLenType ->
>>            pr "  py_r = put_len_type (len, t);\n"
>> +       | RLenValue ->
>> +           pr "  py_r = put_len (len);\n"
> 
> As above.
> 
>>        | RLenTypeVal ->
>>            pr "  py_r = put_val_type (r, len, t);\n";
>>            pr "  free (r);\n"
>> @@ -3296,6 +3355,7 @@ get_values (VALUE valuesv, size_t *nr_values)
>>           | RString -> "string"
>>           | RStringList -> "list"
>>           | RLenType -> "hash"
>> +          | RLenValue -> "integer"
>>           | RLenTypeVal -> "hash"
>>           | RInt32 -> "integer"
>>           | RInt64 -> "integer" in
>> @@ -3394,6 +3454,10 @@ get_values (VALUE valuesv, size_t *nr_values)
>>             pr "  size_t len;\n";
>>             pr "  hive_type t;\n";
>>             "-1"
>> +        | RLenValue ->
>> +            pr "  int r;\n";
> 
> As above (Ruby).
> 
>> +            pr "  size_t len;\n";
>> +            "0"
>>         | RLenTypeVal ->
>>             pr "  char *r;\n";
>>             pr "  size_t len;\n";
>> @@ -3418,6 +3482,7 @@ get_values (VALUE valuesv, size_t *nr_values)
>>       let c_params =
>>         match ret with
>>         | RLenType | RLenTypeVal -> c_params @ [["&t"; "&len"]]
>> +        | RLenValue -> c_params @ [["&len"]]
>>         | _ -> c_params in
>>       let c_params = List.concat c_params in
>> 
>> @@ -3499,6 +3564,10 @@ get_values (VALUE valuesv, size_t *nr_values)
>>         pr "  rb_hash_aset (rv, ID2SYM (rb_intern (\"len\")), INT2NUM (len));\n";
>>         pr "  rb_hash_aset (rv, ID2SYM (rb_intern (\"type\")), INT2NUM (t));\n";
>>         pr "  return rv;\n"
>> +      | RLenValue ->
>> +        pr "  VALUE rv = rb_hash_new ();\n";
>> +        pr "  rb_hash_aset (rv, ID2SYM (rb_intern (\"len\")), INT2NUM (len));\n";
>> +        pr "  return rv;\n"
> 
> As above.
> 
>>       | RLenTypeVal ->
>>         pr "  VALUE rv = rb_hash_new ();\n";
>>         pr "  rb_hash_aset (rv, ID2SYM (rb_intern (\"len\")), INT2NUM (len));\n";
> 
> 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/


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]