Re: [Libguestfs] [libnbd PATCH v2 2/5] generator: Refactor filtering of accepted OFlags

Rather than having to open-code the list of accepted command flags in the unlocked version of each command, we can store that information in the generator to produce the check directly in the public API.

generator/API.ml | 53 +++++++++++++++++++++++++++++---------------- generator/API.mli | 3 ++- generator/C.ml | 26 +++++++++++++++------- generator/GoLang.ml | 10 ++++----- generator/OCaml.ml | 6 ++--- generator/Python.ml | 10 ++++----- lib/disconnect.c | 10 --------- lib/rw.c | 45 -------------------------------------- 8 files changed, 67 insertions(+), 96 deletions(-) diff --git a/generator/API.ml b/generator/API.ml index 42eeac0..b212e95 100644 --- a/generator/API.ml +++ b/generator/API.ml @@ -55,7 +55,7 @@ and arg = | UInt64 of string and optarg = | OClosure of closure -| OFlags of string * flags +| OFlags of string * flags * string list option and ret = | RBool | RStaticString @@ -1485,7 +1485,11 @@ Future NBD extensions may result in additional C<size_type> values. "pread", { default_call with args = [ BytesOut ("buf", "count"); UInt64 "offset" ]; - optargs = [ OFlags ("flags", cmd_flags) ]; + (* We could silently accept flag DF, but it really only makes sense + * with callbacks, because otherwise there is no observable change + * except that the server may fail where it would otherwise succeed. + *) + optargs = [ OFlags ("flags", cmd_flags, Some []) ]; ret = RErr; permitted_states = [ Connected ]; shortdesc = "read from the NBD server"; @@ -1509,7 +1513,7 @@ protocol extensions)."; default_call with args = [ BytesOut ("buf", "count"); UInt64 "offset"; Closure chunk_closure ]; - optargs = [ OFlags ("flags", cmd_flags) ]; + optargs = [ OFlags ("flags", cmd_flags, Some ["DF"]) ]; ret = RErr; permitted_states = [ Connected ]; shortdesc = "read from the NBD server"; @@ -1585,7 +1589,7 @@ actually obeys the flag."; "pwrite", { default_call with args = [ BytesIn ("buf", "count"); UInt64 "offset" ]; - optargs = [ OFlags ("flags", cmd_flags) ]; + optargs = [ OFlags ("flags", cmd_flags, Some ["FUA"]) ]; ret = RErr; permitted_states = [ Connected ]; shortdesc = "write to the NBD server"; @@ -1608,7 +1612,8 @@ L<nbd_can_fua(3)>)."; "shutdown", { default_call with - args = []; optargs = [ OFlags ("flags", shutdown_flags) ]; ret = RErr; + args = []; optargs = [ OFlags ("flags", shutdown_flags, None) ]; + ret = RErr; permitted_states = [ Connected ]; shortdesc = "disconnect from the NBD server"; longdesc = "\ @@ -1645,7 +1650,7 @@ A future version of the library may add new flags."; "flush", { default_call with - args = []; optargs = [ OFlags ("flags", cmd_flags) ]; ret = RErr; + args = []; optargs = [ OFlags ("flags", cmd_flags, Some []) ]; ret = RErr; permitted_states = [ Connected ]; shortdesc = "send flush command to the NBD server"; longdesc = "\ @@ -1662,7 +1667,7 @@ protocol extensions)."; "trim", { default_call with args = [ UInt64 "count"; UInt64 "offset" ]; - optargs = [ OFlags ("flags", cmd_flags) ]; + optargs = [ OFlags ("flags", cmd_flags, Some ["FUA"]) ]; ret = RErr; permitted_states = [ Connected ]; shortdesc = "send trim command to the NBD server"; @@ -1685,7 +1690,7 @@ L<nbd_can_fua(3)>)."; "cache", { default_call with args = [ UInt64 "count"; UInt64 "offset" ]; - optargs = [ OFlags ("flags", cmd_flags) ]; + optargs = [ OFlags ("flags", cmd_flags, Some []) ]; ret = RErr; permitted_states = [ Connected ]; shortdesc = "send cache (prefetch) command to the NBD server"; @@ -1705,7 +1710,8 @@ protocol extensions)."; "zero", { default_call with args = [ UInt64 "count"; UInt64 "offset" ]; - optargs = [ OFlags ("flags", cmd_flags) ]; + optargs = [ OFlags ("flags", cmd_flags, + Some ["FUA"; "NO_HOLE"; "FAST_ZERO"]) ]; ret = RErr; permitted_states = [ Connected ]; shortdesc = "send write zeroes command to the NBD server"; @@ -1733,7 +1739,7 @@ cannot do this, see L<nbd_can_fast_zero(3)>)."; "block_status", { default_call with args = [ UInt64 "count"; UInt64 "offset"; Closure extent_closure ]; - optargs = [ OFlags ("flags", cmd_flags) ]; + optargs = [ OFlags ("flags", cmd_flags, Some ["REQ_ONE"]) ]; ret = RErr; permitted_states = [ Connected ]; shortdesc = "send block status command to the NBD server"; @@ -2026,7 +2032,8 @@ callback."; "aio_pread", { default_call with args = [ BytesPersistOut ("buf", "count"); UInt64 "offset" ]; - optargs = [ OClosure completion_closure; OFlags ("flags", cmd_flags) ]; + optargs = [ OClosure completion_closure; + OFlags ("flags", cmd_flags, Some []) ]; ret = RCookie; permitted_states = [ Connected ]; shortdesc = "read from the NBD server"; @@ -2048,7 +2055,8 @@ completed. Other parameters behave as documented in L<nbd_pread(3)>."; default_call with args = [ BytesPersistOut ("buf", "count"); UInt64 "offset"; Closure chunk_closure ]; - optargs = [ OClosure completion_closure; OFlags ("flags", cmd_flags) ]; + optargs = [ OClosure completion_closure; + OFlags ("flags", cmd_flags, Some ["DF"]) ]; ret = RCookie; permitted_states = [ Connected ]; shortdesc = "read from the NBD server"; @@ -2067,7 +2075,8 @@ Other parameters behave as documented in L<nbd_pread_structured(3)>."; "aio_pwrite", { default_call with args = [ BytesPersistIn ("buf", "count"); UInt64 "offset" ]; - optargs = [ OClosure completion_closure; OFlags ("flags", cmd_flags) ]; + optargs = [ OClosure completion_closure; + OFlags ("flags", cmd_flags, Some ["FUA"]) ]; ret = RCookie; permitted_states = [ Connected ]; shortdesc = "write to the NBD server"; @@ -2086,7 +2095,7 @@ completed. Other parameters behave as documented in L<nbd_pwrite(3)>."; "aio_disconnect", { default_call with - args = []; optargs = [ OFlags ("flags", cmd_flags) ]; ret = RErr; + args = []; optargs = [ OFlags ("flags", cmd_flags, Some []) ]; ret = RErr; permitted_states = [ Connected ]; shortdesc = "disconnect from the NBD server"; longdesc = "\ @@ -2111,7 +2120,8 @@ however, L<nbd_shutdown(3)> will call this function if appropriate."; "aio_flush", { default_call with args = []; - optargs = [ OClosure completion_closure; OFlags ("flags", cmd_flags) ]; + optargs = [ OClosure completion_closure; + OFlags ("flags", cmd_flags, Some []) ]; ret = RCookie; permitted_states = [ Connected ]; shortdesc = "send flush command to the NBD server"; @@ -2130,7 +2140,8 @@ Other parameters behave as documented in L<nbd_flush(3)>."; "aio_trim", { default_call with args = [ UInt64 "count"; UInt64 "offset" ]; - optargs = [ OClosure completion_closure; OFlags ("flags", cmd_flags) ]; + optargs = [ OClosure completion_closure; + OFlags ("flags", cmd_flags, Some ["FUA"]) ]; ret = RCookie; permitted_states = [ Connected ]; shortdesc = "send trim command to the NBD server"; @@ -2149,7 +2160,8 @@ Other parameters behave as documented in L<nbd_trim(3)>."; "aio_cache", { default_call with args = [ UInt64 "count"; UInt64 "offset" ]; - optargs = [ OClosure completion_closure; OFlags ("flags", cmd_flags) ]; + optargs = [ OClosure completion_closure; + OFlags ("flags", cmd_flags, Some []) ]; ret = RCookie; permitted_states = [ Connected ]; shortdesc = "send cache (prefetch) command to the NBD server"; @@ -2168,7 +2180,9 @@ Other parameters behave as documented in L<nbd_cache(3)>."; "aio_zero", { default_call with args = [ UInt64 "count"; UInt64 "offset" ]; - optargs = [ OClosure completion_closure; OFlags ("flags", cmd_flags) ]; + optargs = [ OClosure completion_closure; + OFlags ("flags", cmd_flags, + Some ["FUA"; "NO_HOLE"; "FAST_ZERO"]) ]; ret = RCookie; permitted_states = [ Connected ]; shortdesc = "send write zeroes command to the NBD server"; @@ -2188,7 +2202,8 @@ Other parameters behave as documented in L<nbd_zero(3)>."; "aio_block_status", { default_call with args = [ UInt64 "count"; UInt64 "offset"; Closure extent_closure ]; - optargs = [ OClosure completion_closure; OFlags ("flags", cmd_flags) ]; + optargs = [ OClosure completion_closure; + OFlags ("flags", cmd_flags, Some ["REQ_ONE"]) ]; ret = RCookie; permitted_states = [ Connected ]; shortdesc = "send block status command to the NBD server"; diff --git a/generator/API.mli b/generator/API.mli index e45b5c0..db978ca 100644 --- a/generator/API.mli +++ b/generator/API.mli @@ -65,7 +65,8 @@ and arg = | UInt64 of string (** 64 bit unsigned int *) and optarg = | OClosure of closure (** optional closure *) -| OFlags of string * flags (** optional flags, uint32_t in C *) +| OFlags of string * flags * string list option (** optional flags, uint32_t + in C, and valid subset *) and ret = | RBool (** return a boolean, or error *) | RStaticString (** return a static string (must be located in diff --git a/generator/C.ml b/generator/C.ml index 4d4958d..86d9c5c 100644 --- a/generator/C.ml +++ b/generator/C.ml @@ -191,7 +191,7 @@ and print_arg_list' ?(handle = false) ?(types = true) ?(closure_style = Direct) | AddressOf -> "&" | Pointer -> "*" in pr "%s%s_callback" mark cbname - | OFlags (n, _) -> + | OFlags (n, _, _) -> if types then pr "uint32_t "; pr "%s" n ) optargs @@ -494,11 +494,21 @@ let generate_lib_api_c () = ); (* Check parameters are valid. *) - let print_flags_check n { flag_prefix } = + let print_flags_check n { flag_prefix; flags } subset = let value = match errcode with | Some value -> value | None -> assert false in - pr " if (unlikely ((%s & ~LIBNBD_%s_MASK) != 0)) {\n" n flag_prefix; + let mask = match subset with + | Some [] -> "0" + | Some subset -> + let v = ref 0 in + List.iter ( + fun (flag, i) -> + if List.mem flag subset then v := !v lor i + ) flags; + sprintf "0x%x" !v + | None -> "LIBNBD_" ^ flag_prefix ^ "_MASK" in + pr " if (unlikely ((%s & ~%s) != 0)) {\n" n mask; pr " set_error (EINVAL, \"%%s: invalid value for flag: 0x%%x\",\n"; pr " \"%s\", %s);\n" n n; pr " ret = %s;\n" value; @@ -536,7 +546,7 @@ let generate_lib_api_c () = pr " }\n"; need_out_label := true | Flags (n, flags) -> - print_flags_check n flags + print_flags_check n flags None | String n -> let value = match errcode with | Some value -> value @@ -552,8 +562,8 @@ let generate_lib_api_c () = List.iter ( function | OClosure _ -> () - | OFlags (n, flags) -> - print_flags_check n flags + | OFlags (n, flags, subset) -> + print_flags_check n flags subset ) optargs; (* Make the call. *) @@ -635,7 +645,7 @@ let generate_lib_api_c () = List.iter ( function | OClosure { cbname } -> pr " %s=%%s" cbname - | OFlags (n, _) -> pr " %s=0x%%x" n + | OFlags (n, _, _) -> pr " %s=0x%%x" n ) optargs; pr "\""; List.iter ( @@ -660,7 +670,7 @@ let generate_lib_api_c () = function | OClosure { cbname } -> pr ", CALLBACK_IS_NULL (%s_callback) ? \"<fun>\" : \"NULL\"" cbname - | OFlags (n, _) -> pr ", %s" n + | OFlags (n, _, _) -> pr ", %s" n ) optargs; pr ");\n"; List.iter ( diff --git a/generator/GoLang.ml b/generator/GoLang.ml index f07b074..81446a6 100644 --- a/generator/GoLang.ml +++ b/generator/GoLang.ml @@ -83,7 +83,7 @@ let go_arg_type = function let go_name_of_optarg = function | OClosure { cbname } -> sprintf "%sCallback" (camel_case cbname) - | OFlags (n, _) -> String.capitalize_ascii n + | OFlags (n, _, _) -> String.capitalize_ascii n let go_ret_type = function (* RErr returns only the error, with no return value. *) @@ -202,7 +202,7 @@ let print_binding (name, { args; optargs; ret; shortdesc }) = pr " %s " fname; (match optarg with | OClosure { cbname } -> pr "%sCallback" (camel_case cbname) - | OFlags (_, {flag_prefix}) -> pr "%s" (camel_case flag_prefix) + | OFlags (_, {flag_prefix}, _) -> pr "%s" (camel_case flag_prefix) ); pr "\n" ) optargs; @@ -298,7 +298,7 @@ let print_binding (name, { args; optargs; ret; shortdesc }) = function | OClosure { cbname } -> pr " var c_%s C.nbd_%s_callback\n" cbname cbname - | OFlags (n, _) -> pr " var c_%s C.uint32_t\n" n + | OFlags (n, _, _) -> pr " var c_%s C.uint32_t\n" n ) optargs; pr " if optargs != nil {\n"; List.iter ( @@ -312,7 +312,7 @@ let print_binding (name, { args; optargs; ret; shortdesc }) = cbname cbname; pr " c_%s.user_data = unsafe.Pointer (C.long_to_vp (C.long (registerCallbackId (optargs.%s))))\n" cbname (go_name_of_optarg optarg) - | OFlags (n, _) -> + | OFlags (n, _, _) -> pr " c_%s = C.uint32_t (optargs.%s)\n" n (go_name_of_optarg optarg); ); @@ -346,7 +346,7 @@ let print_binding (name, { args; optargs; ret; shortdesc }) = List.iter ( function | OClosure { cbname} -> pr ", c_%s" cbname - | OFlags (n, _) -> pr ", c_%s" n + | OFlags (n, _, _) -> pr ", c_%s" n ) optargs; pr ")\n"; diff --git a/generator/OCaml.ml b/generator/OCaml.ml index 43b3679..28acb50 100644 --- a/generator/OCaml.ml +++ b/generator/OCaml.ml @@ -72,7 +72,7 @@ and ocaml_ret_to_string = function and ocaml_optarg_to_string = function | OClosure { cbname; cbargs } -> sprintf "?%s:(%s)" cbname (ocaml_closuredecl_to_string cbargs) - | OFlags (n, { flag_prefix }) -> sprintf "?%s:%s.t list" n flag_prefix + | OFlags (n, { flag_prefix }, _) -> sprintf "?%s:%s.t list" n flag_prefix and ocaml_closuredecl_to_string cbargs = let cbargs = List.map ocaml_cbarg_to_string cbargs in @@ -112,7 +112,7 @@ let ocaml_name_of_arg = function let ocaml_name_of_optarg = function | OClosure { cbname } -> cbname - | OFlags (n, _) -> n + | OFlags (n, _, _) -> n let num_params args optargs = List.length optargs + 1 (* handle *) + List.length args @@ -614,7 +614,7 @@ let print_ocaml_binding (name, { args; optargs; ret }) = pr " }\n"; pr " %s_callback.user_data = %s_user_data;\n" cbname cbname; pr " %s_callback.free = free_user_data;\n" cbname; - | OFlags (n, { flag_prefix }) -> + | OFlags (n, { flag_prefix }, _) -> pr " uint32_t %s;\n" n; pr " if (%sv != Val_int (0)) /* Some [ list of %s.t ] */\n" n flag_prefix; diff --git a/generator/Python.ml b/generator/Python.ml index fd09eae..1705ad9 100644 --- a/generator/Python.ml +++ b/generator/Python.ml @@ -320,7 +320,7 @@ let print_python_binding name { args; optargs; ret; may_set_error } = pr " nbd_%s_callback %s = { .callback = %s_wrapper,\n" cbname cbname cbname; pr " .free = free_user_data };\n" - | OFlags (n, _) -> + | OFlags (n, _, _) -> pr " uint32_t %s_u32;\n" n; pr " unsigned int %s; /* really uint32_t */\n" n ) optargs; @@ -378,7 +378,7 @@ let print_python_binding name { args; optargs; ret; may_set_error } = List.iter ( function | OClosure { cbname } -> pr ", &py_%s_fn" cbname - | OFlags (n, _) -> pr ", &%s" n + | OFlags (n, _, _) -> pr ", &%s" n ) optargs; pr "))\n"; pr " goto out;\n"; @@ -402,7 +402,7 @@ let print_python_binding name { args; optargs; ret; may_set_error } = pr " }\n"; pr " else\n"; pr " %s.callback = NULL; /* we're not going to call it */\n" cbname - | OFlags (n, _) -> pr " %s_u32 = %s;\n" n n + | OFlags (n, _, _) -> pr " %s_u32 = %s;\n" n n ) optargs; List.iter ( function @@ -472,7 +472,7 @@ let print_python_binding name { args; optargs; ret; may_set_error } = List.iter ( function | OClosure { cbname } -> pr ", %s" cbname - | OFlags (n, _) -> pr ", %s_u32" n + | OFlags (n, _, _) -> pr ", %s_u32" n ) optargs; pr ");\n"; List.iter ( @@ -802,7 +802,7 @@ class NBD(object): List.map ( function | OClosure { cbname } -> cbname, Some "None", None - | OFlags (n, _) -> n, Some "0", None + | OFlags (n, _, _) -> n, Some "0", None ) optargs in let args = args @ optargs in pr " def %s(" name; diff --git a/lib/disconnect.c b/lib/disconnect.c index b8356b7..9de1e34 100644 --- a/lib/disconnect.c +++ b/lib/disconnect.c @@ -30,11 +30,6 @@ int nbd_unlocked_shutdown (struct nbd_handle *h, uint32_t flags) { - if ((flags & ~LIBNBD_SHUTDOWN_IMMEDIATE) != 0) { - set_error (EINVAL, "invalid flag: %" PRIu32, flags); - return -1; - } - /* If IMMEDIATE, abort any commands that have not yet had any bytes * sent to the server, so that NBD_CMD_DISC will be first in line. */ @@ -69,11 +64,6 @@ nbd_unlocked_aio_disconnect (struct nbd_handle *h, uint32_t flags) { int64_t id; - if (flags != 0) { - set_error (EINVAL, "invalid flag: %" PRIu32, flags); - return -1; - } - id = nbd_internal_command_common (h, 0, NBD_CMD_DISC, 0, 0, NULL, NULL); if (id == -1) return -1; diff --git a/lib/rw.c b/lib/rw.c index f3adb71..95c002c 100644 --- a/lib/rw.c +++ b/lib/rw.c @@ -280,15 +280,6 @@ nbd_unlocked_aio_pread (struct nbd_handle *h, void *buf, { struct command_cb cb = { .completion = *completion }; - /* We could silently accept flag DF, but it really only makes sense - * with callbacks, because otherwise there is no observable change - * except that the server may fail where it would otherwise succeed. - */ - if (flags != 0) { - set_error (EINVAL, "invalid flag: %" PRIu32, flags); - return -1; - } - SET_CALLBACK_TO_NULL (*completion); return nbd_internal_command_common (h, 0, NBD_CMD_READ, offset, count, buf, &cb); @@ -304,11 +295,6 @@ nbd_unlocked_aio_pread_structured (struct nbd_handle *h, void *buf, struct command_cb cb = { .fn.chunk = *chunk, .completion = *completion }; - if ((flags & ~LIBNBD_CMD_FLAG_DF) != 0) { - set_error (EINVAL, "invalid flag: %" PRIu32, flags); - return -1; - } - if ((flags & LIBNBD_CMD_FLAG_DF) != 0 && nbd_unlocked_can_df (h) != 1) { set_error (EINVAL, "server does not support the DF flag"); @@ -334,11 +320,6 @@ nbd_unlocked_aio_pwrite (struct nbd_handle *h, const void *buf, return -1; } - if ((flags & ~LIBNBD_CMD_FLAG_FUA) != 0) { - set_error (EINVAL, "invalid flag: %" PRIu32, flags); - return -1; - } - if ((flags & LIBNBD_CMD_FLAG_FUA) != 0 && nbd_unlocked_can_fua (h) != 1) { set_error (EINVAL, "server does not support the FUA flag"); @@ -362,11 +343,6 @@ nbd_unlocked_aio_flush (struct nbd_handle *h, return -1; } - if (flags != 0) { - set_error (EINVAL, "invalid flag: %" PRIu32, flags); - return -1; - } - SET_CALLBACK_TO_NULL (*completion); return nbd_internal_command_common (h, 0, NBD_CMD_FLUSH, 0, 0, NULL, &cb); @@ -389,11 +365,6 @@ nbd_unlocked_aio_trim (struct nbd_handle *h, return -1; } - if ((flags & ~LIBNBD_CMD_FLAG_FUA) != 0) { - set_error (EINVAL, "invalid flag: %" PRIu32, flags); - return -1; - } - if ((flags & LIBNBD_CMD_FLAG_FUA) != 0 && nbd_unlocked_can_fua (h) != 1) { set_error (EINVAL, "server does not support the FUA flag"); @@ -427,11 +398,6 @@ nbd_unlocked_aio_cache (struct nbd_handle *h, return -1; } - if (flags != 0) { - set_error (EINVAL, "invalid flag: %" PRIu32, flags); - return -1; - } - SET_CALLBACK_TO_NULL (*completion); return nbd_internal_command_common (h, 0, NBD_CMD_CACHE, offset, count, NULL, &cb); @@ -454,12 +420,6 @@ nbd_unlocked_aio_zero (struct nbd_handle *h, return -1; } - if ((flags & ~(LIBNBD_CMD_FLAG_FUA | LIBNBD_CMD_FLAG_NO_HOLE | - LIBNBD_CMD_FLAG_FAST_ZERO)) != 0) { - set_error (EINVAL, "invalid flag: %" PRIu32, flags); - return -1; - } - if ((flags & LIBNBD_CMD_FLAG_FUA) != 0 && nbd_unlocked_can_fua (h) != 1) { set_error (EINVAL, "server does not support the FUA flag"); @@ -504,11 +464,6 @@ nbd_unlocked_aio_block_status (struct nbd_handle *h, return -1; } - if ((flags & ~LIBNBD_CMD_FLAG_REQ_ONE) != 0) { - set_error (EINVAL, "invalid flag: %" PRIu32, flags); - return -1; - } - if (count == 0) { /* NBD protocol forbids this. */ set_error (EINVAL, "count cannot be 0"); return -1; -- 2.28.0 _______________________________________________ Libguestfs mailing list Libguestfs(a)redhat.com https://www.redhat.com/mailman/listinfo/libguestfs