[Libguestfs] [PATCH] supermin: dpkg: Handle multiarch setups correctly

[Richard W.M. Jones]

On Tue, Mar 04, 2014 at 06:35:27PM +0100, Hilko Bengen wrote:
> ---
>  src/dpkg.ml | 39 +++++++++++++++++++++++++--------------
>  1 file changed, 25 insertions(+), 14 deletions(-)
> 
> diff --git a/src/dpkg.ml b/src/dpkg.ml
> index c0d3292..2662465 100644
> --- a/src/dpkg.ml
> +++ b/src/dpkg.ml
> @@ -29,6 +29,10 @@ let dpkg_detect () =
>      Config.apt_get <> "no" &&
>      file_exists "/etc/debian_version"
>  
> +let dpkg_primary_arch =
> +  let cmd = sprintf "%s --print-architecture" Config.dpkg in
> +  List.hd (run_command_get_lines cmd)

While this is not wrong, if the command returns no lines for some
reason it will cause a mysterious exception to escape which would be
hard to track down.  Users would have to know to set an environment
variable to enable stack traces.  Thus it is better to write this:

  let cmd = ... in
  let lines = run_command_get_lines cmd in
  match lines with
  | [] ->
    eprintf "supermin: dpkg: expecting %s to return some output\n" cmd;
    exit 1
  | arch :: _ -> arch

> +    (* On multiarch setups, only consider the primary architecture *)
> +    List.find
> +      (fun pkg -> pkg.arch = dpkg_primary_arch || pkg.arch = "all")
> +      pkgs

Again, this would let a mysterious exception escape if no matching
match is found.  Since this is an internal error, I guess we don't
need an error mesage, but using 'assert false' will cause the file &
line number to be printed, so something like this would be better:

  try
    List.find (fun pkg -> .....) pkgs
  with
    Not_found -> assert false

ACK with those changes.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top