[Libguestfs] [PATCH v2 00/23] Reimplement many daemon APIs in OCaml.

[Richard W.M. Jones]

v1 was posted here:

https://www.redhat.com/archives/libguestfs/2017-July/msg00098.html

This series now depends on two small patches which I posted separately:

https://www.redhat.com/archives/libguestfs/2017-July/msg00207.html
https://www.redhat.com/archives/libguestfs/2017-July/msg00209.html

v1 -> v2:

- Previously changes to generator/daemon.ml were made incrementally
  through the patch series.  Now these changes are moved into the
  first patch.  This wasn't quite straightforward because I also had
  to move the static functions into a separate file so that bisection
  wouldn't break.

- Related to previous change, move ocaml_exn_to_reply_with_error to
  daemon/daemon-c.c, and rename it
  guestfs_int_daemon_exn_to_reply_with_error.

- Move separate patches implementing optgroups and
  CommandFlagFoldStdoutOnStderr into the first patch.

- daemon/Makefile.am: Deduplicate BUILT_SOURCES & generator_built vars.

- daemon/chroot.mli: In a comment, fix misspelled parmeter -> parameter.

- daemon/utils.ml: Use stringify_args.

- Add a chomp function to Std_utils [patch posted separately]
  and use it in daemon/utils.ml.

- Add Sysroot.sysroot_path function which works like the C function
  and use it throughout.

- Modify Chroot.create with optional ?chroot parameter, so you don't
  need to call Sysroot followed by Chroot in two steps.

- get_blkid_tag: Add device name to error message as in C equiv.

- get_blkid_tag: Use String.chomp instead of String.trimr, since
  that's closer to what the C code does.

- Replace O_CLOEXEC with Unix.set_close_on_exec.  We'll need to fix
  this properly at some point, especially if we make the daemon
  multithreaded.

- Simplify (fun dev -> "/dev/" ^ dev) -> ((^) "/dev/") as suggested.

- Factored the functions in daemon/is.ml about as far as is possible.

- daemon/parted.ml: Use triml before sscanf.

- daemon/filearch.ml: Remove comment about "caller must free".

- daemon/filearch.ml: Use Unix_utils.Mkdtemp instead of making by hand,
  and clean up afterwards.

- daemon/ldm.ml: Factor the two functions as far as possible.

- daemon/lvm.ml: Readd the comment above lvs_has_S_opt which was
  accidentally dropped during conversion.

- daemon/lvm.ml: Optimize handling of prefix in convert_lvm_output.

- daemon/optgroups.c: Remove unnecessary <stdio.h>, <stdlib.h> additions.

- daemon/optgroups.c: Use optgroup_names so we don't need to check
  for non-retired groups.

- daemon/btrfs.ml: Rewrite ‘with_mounted’ helper to use Mkdtemp.

- daemon/btrfs.ml: Filter empty lines.

- daemon/listfs.ml: Use StringSet to simplify filtering the
  devices.

- daemon/parted.ml: Fix print_partition_table and add a comment
  so it's clear this version only works in -m mode.

- daemon/md.ml: Filter empty lines.

- Reran the tests.


Requested, but not done for various reasons:

- Combining Utils, Sysroot, Chroot modules into Daemon.  Not possible
  because of the order of initialization of the modules (Daemon must
  come after everything else, whereas the others must come before
  everything else).  It would be possible to combine Sysroot and
  Chroot together completely , but since they are separate modules
  with different purposes there didn't seem much point.  However I
  did modify the Chroot.create ?chroot function -- see above.

- Writing C bindings to command* functions: too complex, and the OCaml
  replacements are better anyway.

- Reimplement udev_settle{,_file} as wrappers around C functions.
  I tried to make this change but it is unexpectedly complex, because
  it would mean that daemon/utils.ml contains a C function which can
  only be satisfied by linking to the daemon, but daemon_utils_tests
  is a standalone program which cannot be linked to the daemon.  So
  we'd either need to create another OCaml module, or split up the C
  parts of the daemon into "library bits" and "daemon bits".

- Removing GUESTFSD_EXT_CMD.  This is still being used by OpenSUSE, so
  we'll have to think of a better mechanism for it.  Something like
  ‘guestfsd --dump-commands’ may work better.

- Replace compare with subtraction in daemon/devsparts.ml.  I find the
  compare version easier to understand.

- In parted.ml, didn't replace sscanf " %x", since my reading is that
  OCaml sscanf works the same way as C sscanf with regard to spaces,
  ie. it will eat any amount of leading whitespace.

- Generating:
    external available : unit -> bool = "guestfs_int_daemon_optgroup..."
  etc.  It's hard to generate these functions and have them added to the
  correct modules (since the OCaml modules are hand-written, and it's
  awkward to include generated bits in them).


Other notes:

- daemon must be linked to -ldl -lm because the OCaml runtime
  (libasmrun.so) depends on both.

- Structs must be repeated in *.mli files because you can't use OCaml
  ‘include’ to pick up single structs from another module.  However
  the structs are still type checked by the compiler, so we cannot
  write incorrect / untyped code this way.


Rich.