Re: [Libguestfs] [PATCH libnbd 7/9] generator: On entry to API functions, check Flags and OFlags parameters.

On 8/10/19 8:02 AM, Richard W.M. Jones wrote: > Generate checks that no unknown (at the time of compilation) flags are > passed to Flags or OFlags parameters. > --- > generator/generator | 20 ++++++++++++++++++++ > 1 file changed, 20 insertions(+) We may still want to introduce a testing mode where you can use libnbd to drive unknown flags to a server that understands them, for experimenting with protocol extensions not yet included in the NBD specification. But if we ever do add such a mode, it shouldn't be hard to make this sanity checking conditional on that mode. > > diff --git a/generator/generator b/generator/generator > index 96d1148..a6aea26 100755 > --- a/generator/generator > +++ b/generator/generator > @@ -3689,6 +3689,19 @@ let generate_lib_api_c () = > ); > > (* Check parameters are valid. *) > + let print_flags_check n { flag_prefix; flags } = > + let value = match errcode with > + | Some value -> value > + | None -> assert false in > + let mask = List.fold_left (lor) 0 (List.map snd flags) in > + pr " if ((%s & ~%d) != 0) {\n" n mask; > + pr " set_error (EINVAL, \"%%s: invalid value for flag: %%d\",\n"; > + pr " \"%s\", %s);\n" n n; > + pr " ret = %s;\n" value; > + pr " goto out;\n"; Some of the checks in lib/rw.c are now unreachable with this in place. Is it worth simplifying that? (But not all of them - there are still checks that depend on runtime values, such as nbd_pread accepting _DF only if the server advertises it after the client requests structured replies). Also, this lets us pass all four existing command flags to all commands that accept an OFlags parameter, even though none of the commands accept all flags at once - the real protection being added here is the check for completely unrecognized flags.

But the changes here look reasonable. ACK. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org