Richard W.M. Jones
2022-Jun-28 14:47 UTC
[Libguestfs] [libguestfs-common PATCH 10/12] mltools/tools_utils: generalize "--key" selector parsing for OCaml utils
On Tue, Jun 28, 2022 at 01:49:13PM +0200, Laszlo Ersek wrote:> In another patch in this series, we generalize the "--key" selector > parsing for C-language utilities. Adapt the OCaml-langauge "--key" parser:s/langauge/language/> > - Incorporate the new (more informative) error messages for consistency. > > - Prepare for selector types that do not take any type-specific > parameters. These will be represented with constant constructors of the > "key_store_key" type, and such values are not blocks, but unboxed > integers: > <https://v2.ocaml.org/manual/intfc.html#ss:c-concrete-datatypes>. > > (This patch is best shown with "git show -b" for review.) > > Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1809453 > Signed-off-by: Laszlo Ersek <lersek at redhat.com> > --- > mltools/tools_utils.ml | 14 ++++++- > mltools/tools_utils-c.c | 44 ++++++++++++-------- > 2 files changed, 38 insertions(+), 20 deletions(-) > > diff --git a/mltools/tools_utils.ml b/mltools/tools_utils.ml > index 6006ab7e4f6c..e534cbead47a 100644 > --- a/mltools/tools_utils.ml > +++ b/mltools/tools_utils.ml > @@ -390,18 +390,28 @@ let create_standard_options argspec ?anon_fun ?(key_opts = false) > L"colour"; L"colours" ], Getopt.Unit set_colours, s_"Use ANSI colour sequences even if not tty"); > add_argspec ([ L"wrap" ], Getopt.Unit set_wrap, s_"Wrap log messages even if not tty"); > > if key_opts then ( > let parse_key_selector arg > - let parts = String.nsplit ~max:3 ":" arg in > + let parts = String.nsplit ":" arg inDoesn't this change what is parsed, ie if you need to include a ":" in the third string?> match parts with > + | [] -> > + error (f_"selector '%s': missing ID") arg > + | [ _ ] -> > + error (f_"selector '%s': missing TYPE") arg > + | [ _; "key" ] > + | _ :: "key" :: _ :: _ :: _ -> > + error (f_"selector '%s': missing KEY_STRING, or too many fields") arg > | [ device; "key"; key ] -> > List.push_back ks.keys (device, KeyString key) > + | [ _; "file" ] > + | _ :: "file" :: _ :: _ :: _ -> > + error (f_"selector '%s': missing FILENAME, or too many fields") arg > | [ device; "file"; file ] -> > List.push_back ks.keys (device, KeyFileName file) > | _ -> > - error (f_"invalid selector string for --key: %s") arg > + error (f_"selector '%s': invalid TYPE") arg > in > > add_argspec ([ L"echo-keys" ], Getopt.Unit c_set_echo_keys, s_"Don?t turn off echo for passphrases"); > add_argspec ([ L"keys-from-stdin" ], Getopt.Unit c_set_keys_from_stdin, s_"Read passphrases from stdin"); > add_argspec ([ L"key" ], Getopt.String (s_"SELECTOR", parse_key_selector), s_"Specify a LUKS key"); > diff --git a/mltools/tools_utils-c.c b/mltools/tools_utils-c.c > index 081466776666..e9f273ec857f 100644 > --- a/mltools/tools_utils-c.c > +++ b/mltools/tools_utils-c.c > @@ -60,28 +60,36 @@ guestfs_int_mllib_inspect_decrypt (value gv, value gpv, value keysv) > key.id = strdup (String_val (Field (elemv, 0))); > if (!key.id) > caml_raise_out_of_memory (); > > v = Field (elemv, 1); > - switch (Tag_val (v)) { > - case 0: /* KeyString of string */ > - key.type = key_string; > - key.string.s = strdup (String_val (Field (v, 0))); > - if (!key.string.s) > - caml_raise_out_of_memory (); > - break; > - case 1: /* KeyFileName of string */ > - key.type = key_file; > - key.file.name = strdup (String_val (Field (v, 0))); > - if (!key.file.name) > - caml_raise_out_of_memory (); > - break; > - default: > - error (EXIT_FAILURE, 0, > - "internal error: unhandled Tag_val (v) = %d", > - Tag_val (v)); > - }The original code here is really bogus. OCaml ensures that the tag can only be 0 or 1. The default case can never be reached (as long as the OCaml key_store type is not changed).> + if (Is_block (v)) > + switch (Tag_val (v)) { > + case 0: /* KeyString of string */ > + key.type = key_string; > + key.string.s = strdup (String_val (Field (v, 0))); > + if (!key.string.s) > + caml_raise_out_of_memory (); > + break; > + case 1: /* KeyFileName of string */ > + key.type = key_file; > + key.file.name = strdup (String_val (Field (v, 0))); > + if (!key.file.name) > + caml_raise_out_of_memory (); > + break; > + default: > + error (EXIT_FAILURE, 0, > + "internal error: unhandled Tag_val (v) = %d", > + Tag_val (v)); > + } > + else > + switch (Int_val (v)) { > + default: > + error (EXIT_FAILURE, 0, > + "internal error: unhandled Int_val (v) = %d", > + Int_val (v)); > + }I'm going to guess this depends on a future patch where more cases are added and the cases don't have parameters (hence are not Is_block) ... Checks forward in the patches series ... OK, that's right, KeyClevis (no parameter) will be added. But the default labels are not reachable and you don't need to check them. (Or call abort() if you really wanted to. Internal errors are usually a good place to call abort().) Rich.> ks = key_store_import_key (ks, &key); > > keysv = Field (keysv, 1); > } > -- > 2.19.1.3.g30247aa5d201 > > > _______________________________________________ > Libguestfs mailing list > Libguestfs at redhat.com > https://listman.redhat.com/mailman/listinfo/libguestfs-- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. http://libguestfs.org
Laszlo Ersek
2022-Jun-29 12:29 UTC
[Libguestfs] [libguestfs-common PATCH 10/12] mltools/tools_utils: generalize "--key" selector parsing for OCaml utils
On 06/28/22 16:47, Richard W.M. Jones wrote:> On Tue, Jun 28, 2022 at 01:49:13PM +0200, Laszlo Ersek wrote: >> In another patch in this series, we generalize the "--key" selector >> parsing for C-language utilities. Adapt the OCaml-langauge "--key" parser: > > s/langauge/language/Argh, yes. I make this typo too frequently.> >> >> - Incorporate the new (more informative) error messages for consistency. >> >> - Prepare for selector types that do not take any type-specific >> parameters. These will be represented with constant constructors of the >> "key_store_key" type, and such values are not blocks, but unboxed >> integers: >> <https://v2.ocaml.org/manual/intfc.html#ss:c-concrete-datatypes>. >> >> (This patch is best shown with "git show -b" for review.) >> >> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1809453 >> Signed-off-by: Laszlo Ersek <lersek at redhat.com> >> --- >> mltools/tools_utils.ml | 14 ++++++- >> mltools/tools_utils-c.c | 44 ++++++++++++-------- >> 2 files changed, 38 insertions(+), 20 deletions(-) >> >> diff --git a/mltools/tools_utils.ml b/mltools/tools_utils.ml >> index 6006ab7e4f6c..e534cbead47a 100644 >> --- a/mltools/tools_utils.ml >> +++ b/mltools/tools_utils.ml >> @@ -390,18 +390,28 @@ let create_standard_options argspec ?anon_fun ?(key_opts = false) >> L"colour"; L"colours" ], Getopt.Unit set_colours, s_"Use ANSI colour sequences even if not tty"); >> add_argspec ([ L"wrap" ], Getopt.Unit set_wrap, s_"Wrap log messages even if not tty"); >> >> if key_opts then ( >> let parse_key_selector arg >> - let parts = String.nsplit ~max:3 ":" arg in >> + let parts = String.nsplit ":" arg in > > Doesn't this change what is parsed, ie if you need to include > a ":" in the third string?Ugh, yes it does -- but, regarding the C option parser, don't we have the same issue there already (pre-patch)? For example, "--key ID:key:foo:bar" causes guestfs_int_split_string() to return "foo" and "bar" separately, doesn't it? I think if a password contains a ":", the user just has to use a file selector.> >> match parts with >> + | [] -> >> + error (f_"selector '%s': missing ID") arg >> + | [ _ ] -> >> + error (f_"selector '%s': missing TYPE") arg >> + | [ _; "key" ] >> + | _ :: "key" :: _ :: _ :: _ -> >> + error (f_"selector '%s': missing KEY_STRING, or too many fields") arg >> | [ device; "key"; key ] -> >> List.push_back ks.keys (device, KeyString key) >> + | [ _; "file" ] >> + | _ :: "file" :: _ :: _ :: _ -> >> + error (f_"selector '%s': missing FILENAME, or too many fields") arg >> | [ device; "file"; file ] -> >> List.push_back ks.keys (device, KeyFileName file) >> | _ -> >> - error (f_"invalid selector string for --key: %s") arg >> + error (f_"selector '%s': invalid TYPE") arg >> in >> >> add_argspec ([ L"echo-keys" ], Getopt.Unit c_set_echo_keys, s_"Don?t turn off echo for passphrases"); >> add_argspec ([ L"keys-from-stdin" ], Getopt.Unit c_set_keys_from_stdin, s_"Read passphrases from stdin"); >> add_argspec ([ L"key" ], Getopt.String (s_"SELECTOR", parse_key_selector), s_"Specify a LUKS key"); >> diff --git a/mltools/tools_utils-c.c b/mltools/tools_utils-c.c >> index 081466776666..e9f273ec857f 100644 >> --- a/mltools/tools_utils-c.c >> +++ b/mltools/tools_utils-c.c >> @@ -60,28 +60,36 @@ guestfs_int_mllib_inspect_decrypt (value gv, value gpv, value keysv) >> key.id = strdup (String_val (Field (elemv, 0))); >> if (!key.id) >> caml_raise_out_of_memory (); >> >> v = Field (elemv, 1); >> - switch (Tag_val (v)) { >> - case 0: /* KeyString of string */ >> - key.type = key_string; >> - key.string.s = strdup (String_val (Field (v, 0))); >> - if (!key.string.s) >> - caml_raise_out_of_memory (); >> - break; >> - case 1: /* KeyFileName of string */ >> - key.type = key_file; >> - key.file.name = strdup (String_val (Field (v, 0))); >> - if (!key.file.name) >> - caml_raise_out_of_memory (); >> - break; >> - default: >> - error (EXIT_FAILURE, 0, >> - "internal error: unhandled Tag_val (v) = %d", >> - Tag_val (v)); >> - } > > The original code here is really bogus. > > OCaml ensures that the tag can only be 0 or 1. The default case can > never be reached (as long as the OCaml key_store type is not changed). > >> + if (Is_block (v)) >> + switch (Tag_val (v)) { >> + case 0: /* KeyString of string */ >> + key.type = key_string; >> + key.string.s = strdup (String_val (Field (v, 0))); >> + if (!key.string.s) >> + caml_raise_out_of_memory (); >> + break; >> + case 1: /* KeyFileName of string */ >> + key.type = key_file; >> + key.file.name = strdup (String_val (Field (v, 0))); >> + if (!key.file.name) >> + caml_raise_out_of_memory (); >> + break; >> + default: >> + error (EXIT_FAILURE, 0, >> + "internal error: unhandled Tag_val (v) = %d", >> + Tag_val (v)); >> + } >> + else >> + switch (Int_val (v)) { >> + default: >> + error (EXIT_FAILURE, 0, >> + "internal error: unhandled Int_val (v) = %d", >> + Int_val (v)); >> + } > > I'm going to guess this depends on a future patch where more cases are > added and the cases don't have parameters (hence are not Is_block) ... > > Checks forward in the patches series ... > > OK, that's right, KeyClevis (no parameter) will be added. > > But the default labels are not reachable and you don't need to check > them. (Or call abort() if you really wanted to. Internal errors are > usually a good place to call abort().)OK, I'll change the pre-patch default to abort() separately, and then stick with abort() in the new switch too. Thanks Laszlo> > Rich. > >> ks = key_store_import_key (ks, &key); >> >> keysv = Field (keysv, 1); >> } >> -- >> 2.19.1.3.g30247aa5d201 >> >> >> _______________________________________________ >> Libguestfs mailing list >> Libguestfs at redhat.com >> https://listman.redhat.com/mailman/listinfo/libguestfs >