Libguestfs - Sep 2020 - Re: [nbdkit PATCH 2/2] ocaml: Implement .list_exports and friends

On 9/1/20 8:25 AM, Eric Blake wrote:
> Fairly straightforward. I'd love for type export to be a bit more
> flexible to make description optional, but could not figure out how to
> decode that from the C side of things, so for now this just requires
> the caller to supply a description for all exports during
> .list_exports.
> 
Maybe I did figure it out after all, although I'm still not sure this is 
the best interface.  Applying this on top of the original patch lets me 
use 'string option' instead of 'string' as the second member of
the
export record. 
https://caml.inria.fr/pub/docs/manual-ocaml/intfc.html#s%3Ac-ocaml-datatype-repr
didn't directly answer my question, but my understanding is that since 
'string option' is the same as:

type 'a t = a' option   | None          (* Is_block is false, value is
Val_int(0) *)
  | Some of 'a    (* Is_block is true, value is block with tag 0 *)

then checking Is_block tells me whether I have None or Some string, at 
which point another Field() deref gets me to the string contained in 
that block.


diff --git i/plugins/ocaml/NBDKit.mli w/plugins/ocaml/NBDKit.mli
index 0d7e325b..aaec519c 100644
--- i/plugins/ocaml/NBDKit.mli
+++ w/plugins/ocaml/NBDKit.mli
@@ -53,7 +53,7 @@ type extent = {

  type export = {
    name : string;
-  description : string;
+  description : string option;
  }
  (** The type of the export list returned by [.list_exports]. *)

diff --git i/plugins/ocaml/NBDKit.ml w/plugins/ocaml/NBDKit.ml
index 1d014934..1823fc71 100644
--- i/plugins/ocaml/NBDKit.ml
+++ w/plugins/ocaml/NBDKit.ml
@@ -55,7 +55,7 @@ type extent = {

  type export = {
    name : string;
-  description : string;
+  description : string option;
  }

  type 'a plugin = {
diff --git i/plugins/ocaml/example.ml w/plugins/ocaml/example.ml
index 5dc7b374..de493bf2 100644
--- i/plugins/ocaml/example.ml
+++ w/plugins/ocaml/example.ml
@@ -42,8 +42,8 @@ let ocamlexample_config key value        failwith
(Printf.sprintf "unknown parameter: %s" key)

  let ocamlexample_list_exports ro tls : NBDKit.export list -  [ { name =
"name1"; description = "desc1" };
-    { name = "name2"; description = "desc2" } ]
+  [ { name = "name1"; description = Some "desc1" };
+    { name = "name2"; description = None } ]

  let ocamlexample_default_export ro tls     "name1"
diff --git i/plugins/ocaml/ocaml.c w/plugins/ocaml/ocaml.c
index a34f67ca..ea499454 100644
--- i/plugins/ocaml/ocaml.c
+++ w/plugins/ocaml/ocaml.c
@@ -332,11 +332,12 @@ list_exports_wrapper (int readonly, int is_tls, 
struct nbdkit_exports *exports)

    /* Convert exports list into calls to nbdkit_add_export. */
    while (rv != Val_int (0)) {
-    const char *name, *desc;
+    const char *name, *desc = NULL;

      v = Field (rv, 0);          /* export struct */
      name = String_val (Field (v, 0));
-    desc = String_val (Field (v, 1));
+    if (Is_block (Field (v, 1)))
+      desc = String_val (Field (Field (v, 1), 0));
      if (nbdkit_add_export (exports, name, desc) == -1) {
        caml_enter_blocking_section ();
        CAMLreturnT (int, -1);




-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

On Tue, Sep 01, 2020 at 08:41:40AM -0500, Eric Blake
wrote:
> On 9/1/20 8:25 AM, Eric Blake wrote:
> >Fairly straightforward. I'd love for type export to be a bit more
> >flexible to make description optional, but could not figure out how to
> >decode that from the C side of things, so for now this just requires
> >the caller to supply a description for all exports during
> >.list_exports.
> >
> 
> Maybe I did figure it out after all, although I'm still not sure
> this is the best interface.  Applying this on top of the original
> patch lets me use 'string option' instead of 'string' as
the second
> member of the export record.
https://caml.inria.fr/pub/docs/manual-ocaml/intfc.html#s%3Ac-ocaml-datatype-repr
> didn't directly answer my question, but my understanding is that
> since 'string option' is the same as:
> 
> type 'a t = a' option >  | None          (* Is_block is false,
value is Val_int(0) *)
>  | Some of 'a    (* Is_block is true, value is block with tag 0 *)
> 
> then checking Is_block tells me whether I have None or Some string,
> at which point another Field() deref gets me to the string contained
> in that block.
All ‘value’ things in OCaml are just 64 bit integers.  The only
difference is whether the bottom bit is 0, in which case it's a
pointer to an OCaml block, or 1 in which case it's an integer-like
thing shifted left by one bit.  Is_block() simply checks the bottom
bit.

Val_int(0) turns C int 0 into an OCaml int.  Which is done by shifting
it left one place and setting the bottom bit.  IOW it returns C 1.

Also None is represented as OCaml 0 (so C 1):
https://rwmj.wordpress.com/2009/08/04/ocaml-internals/

OCaml blocks are lists of values allocated on the OCaml heap:
https://rwmj.wordpress.com/2009/08/05/ocaml-internals-part-2-strings-and-other-types/
and those values may be ints or pointers.

Therefore the easiest way to tell if something is None from C is
to compare it with Val_int(0), ie:

  value v = /* something which has type ‘string option’ */;

  if (v == Val_int(0)) /* It's None */
    printf ("None\n");
  else {
    /* It's Some string, set sv to the string value */
    value sv = Field (v, 0);
    printf ("Some %s\n", String_val (sv));
  }

The weird casting macros have the form: To_from()

Also that String_val is only valid for a short period of time,
basically until the next OCaml allocation in the current thread.
> +++ w/plugins/ocaml/ocaml.c
> @@ -332,11 +332,12 @@ list_exports_wrapper (int readonly, int
> is_tls, struct nbdkit_exports *exports)
> 
>    /* Convert exports list into calls to nbdkit_add_export. */
>    while (rv != Val_int (0)) {
> -    const char *name, *desc;
> +    const char *name, *desc = NULL;
> 
>      v = Field (rv, 0);          /* export struct */
>      name = String_val (Field (v, 0));
> -    desc = String_val (Field (v, 1));
> +    if (Is_block (Field (v, 1)))
> +      desc = String_val (Field (Field (v, 1), 0));
>      if (nbdkit_add_export (exports, name, desc) == -1) {
>        caml_enter_blocking_section ();
>        CAMLreturnT (int, -1);
Seems OK.  I'm guessing this crashes?

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html

On 9/1/20 9:13 AM, Richard W.M. Jones wrote:
> On Tue, Sep 01, 2020 at 08:41:40AM -0500, Eric Blake wrote:
>> On 9/1/20 8:25 AM, Eric Blake wrote:
>>> Fairly straightforward. I'd love for type export to be a bit
more
>>> flexible to make description optional, but could not figure out how
to
>>> decode that from the C side of things, so for now this just
requires
>>> the caller to supply a description for all exports during
>>> .list_exports.
>>>
>>
> OCaml blocks are lists of values allocated on the OCaml heap:
>
https://rwmj.wordpress.com/2009/08/05/ocaml-internals-part-2-strings-and-other-types/
> and those values may be ints or pointers.
> 
> Therefore the easiest way to tell if something is None from C is
> to compare it with Val_int(0), ie:
> 
>    value v = /* something which has type ‘string option’ */;
> 
>    if (v == Val_int(0)) /* It's None */
>      printf ("None\n");
>    else {
>      /* It's Some string, set sv to the string value */
>      value sv = Field (v, 0);
>      printf ("Some %s\n", String_val (sv));
>    }
> 
> The weird casting macros have the form: To_from()
> 
> Also that String_val is only valid for a short period of time,
> basically until the next OCaml allocation in the current thread.
That's helpful, as well.
> 
>> +++ w/plugins/ocaml/ocaml.c
>> @@ -332,11 +332,12 @@ list_exports_wrapper (int readonly, int
>> is_tls, struct nbdkit_exports *exports)
>>
>>     /* Convert exports list into calls to nbdkit_add_export. */
>>     while (rv != Val_int (0)) {
>> -    const char *name, *desc;
>> +    const char *name, *desc = NULL;
>>
>>       v = Field (rv, 0);          /* export struct */
>>       name = String_val (Field (v, 0));
>> -    desc = String_val (Field (v, 1));
>> +    if (Is_block (Field (v, 1)))
>> +      desc = String_val (Field (Field (v, 1), 0));
>>       if (nbdkit_add_export (exports, name, desc) == -1) {
>>         caml_enter_blocking_section ();
>>         CAMLreturnT (int, -1);
> 
> Seems OK.  I'm guessing this crashes?
Nope, it worked.  So compared to your explanation above, my test for 
Some was Is_block, rather than Int_val(0).  And I repeated Field(v,1) 
for getting to the 'Some "string"' rather than an intermediate
variable,
but matched your code in the Field(x, 0) for accessing the string 
portion of it.

I guess my remaining questions are whether there is a better approach than:

func : export list    [ { "name1"; None }; { "name2"; Some
"desc" } ]

In python, I was able to use:

   return [ "name1", ( "name2", "desc" ) ]

where the alternation was on a string vs. a 2-tuple of strings, rather 
than all list members being a record but where the record had an 
optional member.  But I'm not sure how to represent that in the ocaml 
interface.  Maybe I'm thinking of something like:

type export    | Name of string
   | NameDesc of string * string

but I'm not quite sure how to write a list that initializes that.  On 
the other hand, I think I understand the C binding for such a type: 
either val is Val_int(0) (C 1) for the Name branch (Field(v, 0) is a 
string), or val is Val_int(1) (C 2) for the NameDesc branch (Field(v, 0) 
is name, Field(v, 1) is desc).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

On Tue, Sep 01, 2020 at 03:13:28PM +0100, Richard W.M. Jones
wrote:
> Val_int(0) turns C int 0 into an OCaml int.  Which is done by shifting
> it left one place and setting the bottom bit.  IOW it returns C 1.
That was confusing.

I mean Val_int(0) returns the C representation of the OCaml integer 0
(or integer-like thing: None is also represented as the OCaml integer 0).

This is also the reason why OCaml ints are 63 bits, and you must use
the OCaml int64 type (which is boxed) to store a true 64 bit int.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org