[Libguestfs] [PATCH v2] sysprep: Add --network to enable the network (RHBZ#1345813).

[Pino Toscano]

On Monday 13 June 2016 15:41:21 Richard W.M. Jones wrote:
> In commit ae6f726ecc3bc1b67fd76e51a7b1e1a33d4dcfc0 we started to use
> the virt-customize code to replace various virt-sysprep operations.
> This had the effect of adding many more possible operations to
> virt-sysprep, but some of them (specifically --install) did not work
> unless the appliance network is enabled.  It was not enabled in
> virt-sysprep, so these operations never worked.
> 
> This change does NOT enable the network by default.  However it adds a
> --network flag which may be used in conjunction with --install etc to
> make those commands work.
> 
> In addition we now emit a warning for certain customize operations if
> they fail and if the network is not enabled.  It will print:
> 
>   [   4.5] Installing packages: tcpdump
>   Error: Cannot retrieve repository metadata (repomd.xml) for repository: base. Please verify its path and try again
>   Could not retrieve mirrorlist http://mirrorlist.centos.org/?release=6&arch=x86_64&repo=os&infra=stock error was
> 
>   virt-sysprep: warning: the command may have failed because the network is
>   disabled.  Try either removing '--no-network' or adding '--network' on the
>   command line.
> 
>   virt-sysprep: error: yum -y install 'tcpdump': command exited with an error
> 
> (If the network is already enabled, or if the command is successful,
> then the warning is not printed.)
> 
> Thanks: Xianghua Chen
> ---

LGTM, just one note below.

>  customize/customize_run.ml | 22 ++++++++++++++--------
>  sysprep/main.ml            |  5 +++++
>  sysprep/virt-sysprep.pod   | 13 +++++++++++++
>  3 files changed, 32 insertions(+), 8 deletions(-)
> 
> diff --git a/customize/customize_run.ml b/customize/customize_run.ml
> index d4950a2..b96e40c 100644
> --- a/customize/customize_run.ml
> +++ b/customize/customize_run.ml
> @@ -50,7 +50,7 @@ let run (g : Guestfs.guestfs) root (ops : ops) =
>        warning (f_"log file %s: %s (ignored)") logfile (Printexc.to_string exn) in
>  
>    (* Useful wrapper for scripts. *)
> -  let do_run ~display cmd =
> +  let do_run ~display ?(warn_failed_no_network = false) cmd =
>      if not guest_arch_compatible then
>        error (f_"host cpu (%s) and guest arch (%s) are not compatible, so you cannot use command line options that involve running commands in the guest.  Use --firstboot scripts instead.")
>              Guestfs_config.host_cpu guest_arch;
> @@ -90,6 +90,11 @@ exec >>%s 2>&1
>      with
>        Guestfs.Error msg ->
>          debug_logfile ();
> +        if warn_failed_no_network && not (g#get_network ()) then (
> +          prerr_newline ();
> +          warning (f_"the command may have failed because the network is disabled.  Try either removing '--no-network' or adding '--network' on the command line.");
> +          prerr_newline ()
> +        );
>          error (f_"%s: command exited with an error") display
>    in
>  
> @@ -263,7 +268,7 @@ exec >>%s 2>&1
>      | `InstallPackages pkgs ->
>        message (f_"Installing packages: %s") (String.concat " " pkgs);
>        let cmd = guest_install_command pkgs in
> -      do_run ~display:cmd cmd
> +      do_run ~display:cmd ~warn_failed_no_network:true cmd

Maybe just after the "let do_run .. in" can be defined something like

  let do_run_network = do_run ~warn_failed_no_network:true in

just to shorten these various do_run, as syntactic sugar.

Just an idea though, thinking out loud.

Thanks,
-- 
Pino Toscano
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <http://listman.redhat.com/archives/libguestfs/attachments/20160613/c64165c6/attachment.sig>