Libguestfs - Jan 2019 - Re: [PATCH 2/5] mltools: create a new external_command_code

On Wed, Jan 16, 2019 at 03:17:32PM +0100, Pino Toscano
wrote:
> Split most of the code from external_command to a new
> external_command_code, so it is possible to get the exit code of the
> process without considering it fatal.
> ---
>  common/mltools/tools_utils.ml  | 22 ++++++++++------------
>  common/mltools/tools_utils.mli |  8 ++++++++
>  2 files changed, 18 insertions(+), 12 deletions(-)
> 
> diff --git a/common/mltools/tools_utils.ml b/common/mltools/tools_utils.ml
> index 3b1554c5a..e6b2b5713 100644
> --- a/common/mltools/tools_utils.ml
> +++ b/common/mltools/tools_utils.ml
> @@ -338,7 +338,13 @@ let create_standard_options argspec ?anon_fun
?(key_opts = false) ?(machine_read
>    }
>  
>  (* Run an external command, slurp up the output as a list of lines. *)
> -let external_command ?(echo_cmd = true) cmd > +let rec external_command
?(echo_cmd = true) cmd > +  let lines, exitcode = external_command_code
~echo_cmd cmd in
> +  if exitcode <> 0 then
> +    error (f_"external command ‘%s’ exited with error %d") cmd
exitcode;
> +  lines
> +
> +and external_command_code ?(echo_cmd = true) cmd >    if echo_cmd then
>      debug "%s" cmd;
>    let chan = Unix.open_process_in cmd in
> @@ -347,18 +353,10 @@ let external_command ?(echo_cmd = true) cmd >    
with End_of_file -> ());
>    let lines = List.rev !lines in
>    let stat = Unix.close_process_in chan in
> -  (match stat with
> -  | Unix.WEXITED 0 -> ()
> -  | Unix.WEXITED i ->
> -    error (f_"external command ‘%s’ exited with error %d") cmd i
> -  | Unix.WSIGNALED i ->
> -    error (f_"external command ‘%s’ killed by signal %d") cmd i
> -  | Unix.WSTOPPED i ->
> -    error (f_"external command ‘%s’ stopped by signal %d") cmd i
> -  );
> -  lines
> +  let exitcode = do_check_exitcode cmd stat in
> +  lines, exitcode
>  
> -let rec run_commands ?(echo_cmd = true) cmds > +and run_commands
?(echo_cmd = true) cmds >    let res = Array.make (List.length cmds) 0 in
>    let pids >      List.mapi (
> diff --git a/common/mltools/tools_utils.mli
b/common/mltools/tools_utils.mli
> index ab70f583e..fb998697c 100644
> --- a/common/mltools/tools_utils.mli
> +++ b/common/mltools/tools_utils.mli
> @@ -100,6 +100,14 @@ val create_standard_options : Getopt.speclist ->
?anon_fun:Getopt.anon_fun -> ?k
>  
>  val external_command : ?echo_cmd:bool -> string -> string list
>  (** Run an external command, slurp up the output as a list of lines.
> +    A non-zero exit code of the command is considered a fatal error.
> +
> +    [echo_cmd] specifies whether to output the full command on verbose
> +    mode, and it's on by default. *)
> +
> +val external_command_code : ?echo_cmd:bool -> string -> string list
* int
> +(** Run an external command, slurp up the output as a list of lines,
> +    and return it together with the exit code of the command.
Can you amend the documentation to say something on the lines of:

  Run an external command, slurp up the output as a list of lines.
  If the command exits normally with any exit code then the exit
  code is returned.  If the command is signalled or stopped then
  that is a fatal error.

ACK with this or similar change.

BTW we use debug + Sys.command all over the place and it might be
worth considering replacing those instances with this new function
where appropriate.

Rich.
>      [echo_cmd] specifies whether to output the full command on verbose
>      mode, and it's on by default. *)
> -- 
> 2.20.1
> 
> _______________________________________________
> Libguestfs mailing list
> Libguestfs@redhat.com
> https://www.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
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

On Wednesday, 16 January 2019 15:31:43 CET Richard W.M. Jones
wrote:
> BTW we use debug + Sys.command all over the place and it might be
> worth considering replacing those instances with this new function
> where appropriate.
Like Tools_utils.shell_command?

-- 
Pino Toscano

On Thu, Jan 17, 2019 at 11:24:35AM +0100, Pino Toscano
wrote:
> On Wednesday, 16 January 2019 15:31:43 CET Richard W.M. Jones wrote:
> > BTW we use debug + Sys.command all over the place and it might be
> > worth considering replacing those instances with this new function
> > where appropriate.
> 
> Like Tools_utils.shell_command?
Indeed, we don't use that as much as we should either :-)

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
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