Re: [Libguestfs] [PATCH libnbd 2/9] generator: Generalize OFlags.

On 8/10/19 8:02 AM, Richard W.M. Jones wrote: > In a future commit we want to add (non-optional) Flags arg. As a step > to doing this, generalize OFlags so it's not tied to just > NBD_CMD_FLAG_*. > > This does not change the C API. > > It does introduce a small change to the OCaml API -- putting related > flags into a submodule, basically renaming them. Note we don't > provide guarantees for non-C APIs. And even if we decide to provide guarantees for non-C, we're not at 1.0 yet ;) > @@ -1387,7 +1403,7 @@ Returns the size in bytes of the NBD export." > "pread", { > default_call with > args = [ BytesOut ("buf", "count"); UInt64 "offset" ]; > - optargs = [ OFlags "flags" ]; > + optargs = [ OFlags ("flags", cmd_flags) ]; Do we want to use this as a chance to document which flags a given API supports? For example, pread supports DF but not FUA, REQ_ONE, or MAY_TRIM. Then again, there's still a dynamic element - the API supports the DF flag for compilation, but the server must also support it (nbd_can_df) before you can use it. So any further restrictions we decide to encode in the generator rather (or in addition) to restrictions in lib/rw.c can be a later patch.

> +++ b/ocaml/tests/test_405_pread_structured.ml > @@ -54,11 +54,13 @@ let () = > NBD.pread_structured nbd buf 0_L (f 42); > assert (buf = expected); > > - NBD.pread_structured nbd buf 0_L (f 42) ~flags:[NBD.cmd_flag_df]; > + let flags = let open NBD.CMD_FLAG in [DF] in > + > + NBD.pread_structured nbd buf 0_L (f 42) ~flags; blank line here...

> +++ b/ocaml/tests/test_410_pwrite.ml > @@ -33,7 +33,8 @@ let () = > let nbd = NBD.create () in > NBD.connect_command nbd ["nbdkit"; "-s"; "--exit-with-parent"; "-v"; > "file"; datafile]; > - NBD.pwrite nbd buf1 0_L ~flags:[NBD.cmd_flag_fua]; > + let flags = let open NBD.CMD_FLAG in [FUA] in > + NBD.pwrite nbd buf1 0_L ~flags; ...but not here. It doesn't affect the code, but the consistency may be worth it. This and patch 1 look fine to me.