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

[Richard W.M. Jones]

On Sat, Aug 10, 2019 at 12:22:44PM -0500, Eric Blake wrote:
> 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.

It's a bit tricky to do this in the generator and especially as you
say if it is dynamic.

On a similar theme the checks introduced in a later patch in this
series are not a subsitute for more fine-grained checks in the
nbd_unlocked_* function.

> > +++ 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...

OK fixed in my copy.

> 
> > +++ 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.

Thanks,

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW