On 01/06/22 16:02, Richard W.M. Jones wrote: > On Thu, Jan 06, 2022 at 03:09:08PM +0100, Laszlo Ersek wrote: >> For debugging purposes, we'll want to print the list of devices returned >> by the previously introduced "osinfo_os#get_devices" method. >> >> Format the device list as a nice table. >> >> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1942325 >> Signed-off-by: Laszlo Ersek <[email protected]> >> --- >> >> Notes: >> Log examples will be shown later in this series, in the Notes section(s) >> of other patch(es). >> >> convert/libosinfo_utils.mli | 3 ++ >> convert/libosinfo_utils.ml | 35 ++++++++++++++++++++ >> 2 files changed, 38 insertions(+) >> >> diff --git a/convert/libosinfo_utils.mli b/convert/libosinfo_utils.mli >> index b3714d224ae4..5a703334d9f0 100644 >> --- a/convert/libosinfo_utils.mli >> +++ b/convert/libosinfo_utils.mli >> @@ -27,3 +27,6 @@ val get_os_by_short_id : string -> Libosinfo.osinfo_os >> >> val string_of_osinfo_device_driver : Libosinfo.osinfo_device_driver -> >> string >> (** Convert a [osinfo_device_driver] to a printable string for debugging. *) >> + >> +val string_of_osinfo_device_list : Libosinfo.osinfo_device list -> string >> +(** Convert an [osinfo_device] list to a printable string for debugging. *) >> diff --git a/convert/libosinfo_utils.ml b/convert/libosinfo_utils.ml >> index 1fc138cce321..d5eb082b458c 100644 >> --- a/convert/libosinfo_utils.ml >> +++ b/convert/libosinfo_utils.ml >> @@ -42,3 +42,38 @@ let string_of_osinfo_device_driver { >> Libosinfo.architecture; location; >> (if signed then "signed" else "unsigned") >> priority >> (String.concat " " files) >> + >> +let string_of_osinfo_device_list dev_list = >> + >> + (* Turn the fields of an "osinfo_device" record into a list. *) >> + let listify { Libosinfo.id; vendor; vendor_id; product; product_id; name; >> + class_; bus_type; subsystem } = >> + [ id; vendor; vendor_id; product; product_id; name; >> + class_; bus_type; subsystem ] >> + >> + (* Given a list of strings, and a list of previously known maximum widths, >> + * "increase" each width, if necessary, to the length of the corresponding >> + * string. >> + *) >> + and grow_widths = List.map2 (fun s -> max (String.length s)) >> + in >> + >> + (* Compute the maximum width for each field in "dev_list". *) >> + let max_widths = >> + List.fold_right grow_widths (List.map listify dev_list) >> + [ 0; 0; 0; 0; 0; 0; 0; 0; 0 ] >> + >> + (* Given a list of strings and a list of field widths, format "string1 | >> + * string2 | ... | stringN" such that each field is right-padded to the >> + * corresponding width. >> + *) >> + and columnate strings widths = >> + String.concat " | " (List.map2 (Printf.sprintf "%-*s") widths strings) >> + in >> + >> + (* Format "dev_list" as a table by (a) printing one "osinfo_device" record >> + * per line, and (b) right-padding each field of each "osinfo_device" >> record >> + * to the maximum width of that field. >> + *) >> + String.concat "\n" >> + (List.map (fun dev -> columnate (listify dev) max_widths) dev_list) > > Seems ... complicated, and I guess something that if we think we'll > need to format a lot of tables we should add a generic mechanism to > Std_utils.
It certainly occurred to me that we'd probably move this later to a more central utils module. > > I would have done this using printf a fixed maximum width like > this code: > > https://gitlab.com/nbdkit/nbdkit/-/blob/d7c69529c453adb1b5b651803881dac2e724f2e9/plugins/vddk/stats.c#L99 I vaguely remember reviewing a patch from you that also used fixed maximum widths... I think it was about printing nbd block statuses or some such in a table format?... and I remember not really liking the fixed widths. Fixed widths almost always do the wrong thing: - they either waste space, or - fail to tabulate correctly. Case in point, before I set out writing this function, I ran: osinfo-query device just to get a taste of what I might expect. If you run that command now, you'll see that the command attempts to tabulate its output -- and that it fails at it *miserably*. The output is utter garbage, so much so that I couldn't read it, before I did: osinfo-query device | column -t -s \| which helped at least a little. So that's why I feel quite strongly about this -- and the resultant log snippets (which you can see in the Notes section of the last patch in the series) confirmed that determining the minimum sufficient field widths was a good idea. > > But it's encapsulated into a function and only used for debugging so > we could revise it later if we wanted, so sure. Thanks! This is actually the most functional-style part of the series, and I was super excited to write it. This is not the initial version: I remembered that you emphasized partial application in OCaml, and I kept trimming it (using partial application and existent List module APIs) until I could take nothing more away. Thanks! Laszlo _______________________________________________ Libguestfs mailing list [email protected] https://listman.redhat.com/mailman/listinfo/libguestfs
