Laszlo Ersek
2022-Jul-15 07:17 UTC
[Libguestfs] [PATCH common 3/4] mltools: Introduce priority for ordering actions in On_exit
On 07/14/22 14:36, Richard W.M. Jones wrote:> Introduce a new, optional ?prio parameter which can be used to control > the order that actions run on exit. By default actions have priority > 1000. Higher numbered actions run first. Lower numbered actions run > last. So to have an action run at the very end before exit you might > use ~prio:0 > > Note that even with this change, some actions (eg kill) are still > asynchronous. > --- > mltools/on_exit.ml | 24 +++++++++++++----------- > mltools/on_exit.mli | 18 +++++++++++++----- > 2 files changed, 26 insertions(+), 16 deletions(-) > > diff --git a/mltools/on_exit.ml b/mltools/on_exit.ml > index 4fa2c3b..e8353df 100644 > --- a/mltools/on_exit.ml > +++ b/mltools/on_exit.ml > @@ -29,7 +29,7 @@ type action > | Kill of int * int (* signal, pid *) > | Fn of (unit -> unit) (* generic function *) > > -(* List of actions. *) > +(* List of (priority, action). *) > let actions = ref [] > > (* Perform a single exit action, printing any exception but > @@ -50,10 +50,12 @@ let do_action action > (* Make sure the actions are performed only once. *) > let done_actions = ref false > > -(* Perform the exit actions. *) > +(* Perform the exit actions in reverse priority order (highest first). *) > let do_actions () > if not !done_actions then ( > - List.iter do_action !actions > + let actions = List.sort (fun (a, _) (b, _) -> compare b a) !actions in > + let actions = List.map snd actions in > + List.iter do_action actions > ); > done_actions := true > > @@ -92,18 +94,18 @@ let register () > ); > registered := true > > -let f fn > +let f ?(prio = 1000) fn > register (); > - List.push_front (Fn fn) actions > + List.push_front (prio, Fn fn) actions > > -let unlink filename > +let unlink ?(prio = 1000) filename > register (); > - List.push_front (Unlink filename) actions > + List.push_front (prio, Unlink filename) actions > > -let rm_rf dir > +let rm_rf ?(prio = 1000) dir > register (); > - List.push_front (Rm_rf dir) actions > + List.push_front (prio, Rm_rf dir) actions > > -let kill ?(signal = Sys.sigterm) pid > +let kill ?(prio = 1000) ?(signal = Sys.sigterm) pid > register (); > - List.push_front (Kill (signal, pid)) actions > + List.push_front (prio, Kill (signal, pid)) actions > diff --git a/mltools/on_exit.mli b/mltools/on_exit.mli > index 9bcf104..910783e 100644 > --- a/mltools/on_exit.mli > +++ b/mltools/on_exit.mli > @@ -28,6 +28,12 @@ > killing another process, so we provide simple > wrappers for those common actions here. > > + Actions can be ordered by setting the optional [?prio] > + parameter. By default all actions have priority 1000. > + Higher numbered actions run first. Lower numbered > + actions run last. So to have an action run at the > + very end before exit you might use [~prio:0] > + > Note this module registers signal handlers for > SIGINT, SIGQUIT, SIGTERM and SIGHUP. This means > that any program that links with mltools.cmxa > @@ -39,18 +45,20 @@ > Your cleanup action might no longer run unless the > program calls {!Stdlib.exit}. *) > > -val f : (unit -> unit) -> unit > +val f : ?prio:int -> (unit -> unit) -> unit > (** Register a function [f] which runs when the program exits. > Similar to [Stdlib.at_exit] but also runs if the program is > - killed with a signal that we can catch. *) > + killed with a signal that we can catch. > > -val unlink : string -> unit > + [?prio] is the priority, default 1000. See the description above. *) > + > +val unlink : ?prio:int -> string -> unit > (** Unlink a single temporary file on exit. *) > > -val rm_rf : string -> unit > +val rm_rf : ?prio:int -> string -> unit > (** Recursively remove a temporary directory on exit (using [rm -rf]). *) > > -val kill : ?signal:int -> int -> unit > +val kill : ?prio:int -> ?signal:int -> int -> unit > (** Kill [PID] on exit. The signal sent defaults to [Sys.sigterm]. > > Use this with care since you can end up unintentionally killing >I think this patch is good, but I'm surprised that ~prio:0 is for placing stuff at the *end* of the list, where 1000 is the default. This ordering differs from both the firstboot script priorities we did recently (lower prio number -> runs earlier) and also from the operation ordering in virt-sysprep (default is 0, 99 puts stuff at the end). Swapping around the arguments to "compare" wouldn't be difficult, the more laborious update would be for the documentation (commit message, comments). Up to you... Reviewed-by: Laszlo Ersek <lersek at redhat.com>
Richard W.M. Jones
2022-Jul-15 08:13 UTC
[Libguestfs] [PATCH common 3/4] mltools: Introduce priority for ordering actions in On_exit
On Fri, Jul 15, 2022 at 09:17:26AM +0200, Laszlo Ersek wrote:> On 07/14/22 14:36, Richard W.M. Jones wrote: > > Introduce a new, optional ?prio parameter which can be used to control > > the order that actions run on exit. By default actions have priority > > 1000. Higher numbered actions run first. Lower numbered actions run > > last. So to have an action run at the very end before exit you might > > use ~prio:0 > > > > Note that even with this change, some actions (eg kill) are still > > asynchronous. > > --- > > mltools/on_exit.ml | 24 +++++++++++++----------- > > mltools/on_exit.mli | 18 +++++++++++++----- > > 2 files changed, 26 insertions(+), 16 deletions(-) > > > > diff --git a/mltools/on_exit.ml b/mltools/on_exit.ml > > index 4fa2c3b..e8353df 100644 > > --- a/mltools/on_exit.ml > > +++ b/mltools/on_exit.ml > > @@ -29,7 +29,7 @@ type action > > | Kill of int * int (* signal, pid *) > > | Fn of (unit -> unit) (* generic function *) > > > > -(* List of actions. *) > > +(* List of (priority, action). *) > > let actions = ref [] > > > > (* Perform a single exit action, printing any exception but > > @@ -50,10 +50,12 @@ let do_action action > > (* Make sure the actions are performed only once. *) > > let done_actions = ref false > > > > -(* Perform the exit actions. *) > > +(* Perform the exit actions in reverse priority order (highest first). *) > > let do_actions () > > if not !done_actions then ( > > - List.iter do_action !actions > > + let actions = List.sort (fun (a, _) (b, _) -> compare b a) !actions in > > + let actions = List.map snd actions in > > + List.iter do_action actions > > ); > > done_actions := true > > > > @@ -92,18 +94,18 @@ let register () > > ); > > registered := true > > > > -let f fn > > +let f ?(prio = 1000) fn > > register (); > > - List.push_front (Fn fn) actions > > + List.push_front (prio, Fn fn) actions > > > > -let unlink filename > > +let unlink ?(prio = 1000) filename > > register (); > > - List.push_front (Unlink filename) actions > > + List.push_front (prio, Unlink filename) actions > > > > -let rm_rf dir > > +let rm_rf ?(prio = 1000) dir > > register (); > > - List.push_front (Rm_rf dir) actions > > + List.push_front (prio, Rm_rf dir) actions > > > > -let kill ?(signal = Sys.sigterm) pid > > +let kill ?(prio = 1000) ?(signal = Sys.sigterm) pid > > register (); > > - List.push_front (Kill (signal, pid)) actions > > + List.push_front (prio, Kill (signal, pid)) actions > > diff --git a/mltools/on_exit.mli b/mltools/on_exit.mli > > index 9bcf104..910783e 100644 > > --- a/mltools/on_exit.mli > > +++ b/mltools/on_exit.mli > > @@ -28,6 +28,12 @@ > > killing another process, so we provide simple > > wrappers for those common actions here. > > > > + Actions can be ordered by setting the optional [?prio] > > + parameter. By default all actions have priority 1000. > > + Higher numbered actions run first. Lower numbered > > + actions run last. So to have an action run at the > > + very end before exit you might use [~prio:0] > > + > > Note this module registers signal handlers for > > SIGINT, SIGQUIT, SIGTERM and SIGHUP. This means > > that any program that links with mltools.cmxa > > @@ -39,18 +45,20 @@ > > Your cleanup action might no longer run unless the > > program calls {!Stdlib.exit}. *) > > > > -val f : (unit -> unit) -> unit > > +val f : ?prio:int -> (unit -> unit) -> unit > > (** Register a function [f] which runs when the program exits. > > Similar to [Stdlib.at_exit] but also runs if the program is > > - killed with a signal that we can catch. *) > > + killed with a signal that we can catch. > > > > -val unlink : string -> unit > > + [?prio] is the priority, default 1000. See the description above. *) > > + > > +val unlink : ?prio:int -> string -> unit > > (** Unlink a single temporary file on exit. *) > > > > -val rm_rf : string -> unit > > +val rm_rf : ?prio:int -> string -> unit > > (** Recursively remove a temporary directory on exit (using [rm -rf]). *) > > > > -val kill : ?signal:int -> int -> unit > > +val kill : ?prio:int -> ?signal:int -> int -> unit > > (** Kill [PID] on exit. The signal sent defaults to [Sys.sigterm]. > > > > Use this with care since you can end up unintentionally killing > > > > I think this patch is good, but I'm surprised that ~prio:0 is for > placing stuff at the *end* of the list, where 1000 is the default. This > ordering differs from both the firstboot script priorities we did > recently (lower prio number -> runs earlier) and also from the operation > ordering in virt-sysprep (default is 0, 99 puts stuff at the end). > > Swapping around the arguments to "compare" wouldn't be difficult, the > more laborious update would be for the documentation (commit message, > comments). Up to you...I thought (without checking of course) that this way was consistent with firstboot. I'll change it to use the same way as firstboot.> Reviewed-by: Laszlo Ersek <lersek at redhat.com>Thanks, Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com nbdkit - Flexible, fast NBD server with plugins https://gitlab.com/nbdkit/nbdkit
Richard W.M. Jones
2022-Jul-15 09:26 UTC
[Libguestfs] [PATCH common 3/4] mltools: Introduce priority for ordering actions in On_exit
On Fri, Jul 15, 2022 at 09:17:26AM +0200, Laszlo Ersek wrote:> I think this patch is good, but I'm surprised that ~prio:0 is for > placing stuff at the *end* of the list, where 1000 is the default. This > ordering differs from both the firstboot script priorities we did > recently (lower prio number -> runs earlier) and also from the operation > ordering in virt-sysprep (default is 0, 99 puts stuff at the end). > > Swapping around the arguments to "compare" wouldn't be difficult, the > more laborious update would be for the documentation (commit message, > comments). Up to you... > > Reviewed-by: Laszlo Ersek <lersek at redhat.com>So, here's the change to libguestfs-common: https://github.com/libguestfs/libguestfs-common/commit/290a2cecbe679660debd1b2c4fe5912c79c9d84e https://github.com/libguestfs/libguestfs-common/commit/1000604ff391e49f0b645c1932c50c9e7a70f649 Priorities are 0..9999 (limit is not actually enforced) with higher numbers running later and 5000 being the default. The change to virt-v2v: https://github.com/libguestfs/virt-v2v/commit/e96357fc3b26aaf96eaa21afa36c894a27af6261 I don't think there are other places where we umount host devices, but I'm sure there are other places that could use On_exit priorities (in guestfs-tools as well). Let's keep an eye out for those. Thanks, Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-builder quickly builds VMs from scratch http://libguestfs.org/virt-builder.1.html