Re: [Xen-devel] [PATCH 01/27] tools/libxl: Fix libxl__ev_child_inuse() check for not-yet-initialised children
Andrew Cooper writes (Re: [PATCH 01/27] tools/libxl: Fix libxl__ev_child_inuse() check for not-yet-initialised children): It is possible that one bit fails before it can be calculated whether the second bit needs to start or not. At the moment, all bits in libxl in this area do initialisation immediately before use; most bits are even initialised in the function which starts their actions. Some bits are initialised differently depending on the path taken to get to the initialisation site. As a rule of thumb a function libxl__initiate_foo_ which takes a libxl__foo_state* should do this initialisation for the whole libxl__foo_state. I don't see why you can't do that. Ian. ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 01/27] tools/libxl: Fix libxl__ev_child_inuse() check for not-yet-initialised children
Ian Campbell writes (Re: [PATCH 01/27] tools/libxl: Fix libxl__ev_child_inuse() check for not-yet-initialised children): On Mon, 2015-06-15 at 14:44 +0100, Andrew Cooper wrote: Shortly, libxl will be juggling multiple parallel operations, and will possibly have to take error decisions before some tasks have been set up. It would be preferable, I think, to arrange to call libxl__ev_child_init on all such libxl__ev_child structs either up front or certainly before there is any possibility of needing to unwind them. Yes. Such an init would at worst correspond to exactly the place where the zeroed structure you refer to is zeroed. I would welcome a patch which caused an assertion failure if -pid==0. Ian. ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 01/27] tools/libxl: Fix libxl__ev_child_inuse() check for not-yet-initialised children
On 16/06/15 14:21, Ian Campbell wrote: On Mon, 2015-06-15 at 14:44 +0100, Andrew Cooper wrote: Shortly, libxl will be juggling multiple parallel operations, and will possibly have to take error decisions before some tasks have been set up. It would be preferable, I think, to arrange to call libxl__ev_child_init on all such libxl__ev_child structs either up front or certainly before there is any possibility of needing to unwind them. Such an init would at worst correspond to exactly the place where the zeroed structure you refer to is zeroed. It is possible that one bit fails before it can be calculated whether the second bit needs to start or not. At the moment, all bits in libxl in this area do initialisation immediately before use; most bits are even initialised in the function which starts their actions. Some bits are initialised differently depending on the path taken to get to the initialisation site. It would be non-trivial to initialise everything appropriately at the very start. ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 01/27] tools/libxl: Fix libxl__ev_child_inuse() check for not-yet-initialised children
On Mon, 2015-06-15 at 14:44 +0100, Andrew Cooper wrote: Shortly, libxl will be juggling multiple parallel operations, and will possibly have to take error decisions before some tasks have been set up. It would be preferable, I think, to arrange to call libxl__ev_child_init on all such libxl__ev_child structs either up front or certainly before there is any possibility of needing to unwind them. Such an init would at worst correspond to exactly the place where the zeroed structure you refer to is zeroed. No child process of libxl will ever have a pid of 0, so gate libxl__ev_child_inuse() on a pid strictly greater than 0. This makes it safe to use on a zeroed structure of a task which has not yet been set up. Signed-off-by: Andrew Cooper andrew.coop...@citrix.com CC: Ian Campbell ian.campb...@citrix.com CC: Ian Jackson ian.jack...@eu.citrix.com CC: Wei Liu wei.l...@citrix.com --- This change does make libxl__ev_child_init() functionally useless. I am undecided between leaving it in place in case it is useful in the future, or to remove it completely. --- tools/libxl/libxl_internal.h |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h index e96d6b5..6226c18 100644 --- a/tools/libxl/libxl_internal.h +++ b/tools/libxl/libxl_internal.h @@ -880,7 +880,7 @@ _hidden pid_t libxl__ev_child_fork(libxl__gc *gc, libxl__ev_child *childw_out, static inline void libxl__ev_child_init(libxl__ev_child *childw_out) { childw_out-pid = -1; } static inline int libxl__ev_child_inuse(const libxl__ev_child *childw_out) -{ return childw_out-pid = 0; } +{ return childw_out-pid 0; } /* Useable (only) in the child to once more make the ctx useable for * xenstore operations. logs failure in the form what: error ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 01/27] tools/libxl: Fix libxl__ev_child_inuse() check for not-yet-initialised children
On 16/06/15 14:47, Ian Jackson wrote: Andrew Cooper writes (Re: [PATCH 01/27] tools/libxl: Fix libxl__ev_child_inuse() check for not-yet-initialised children): It is possible that one bit fails before it can be calculated whether the second bit needs to start or not. At the moment, all bits in libxl in this area do initialisation immediately before use; most bits are even initialised in the function which starts their actions. Some bits are initialised differently depending on the path taken to get to the initialisation site. As a rule of thumb a function libxl__initiate_foo_ which takes a libxl__foo_state* should do this initialisation for the whole libxl__foo_state. I don't see why you can't do that. The only example of libxl__initiate_foo_ is libxl__initiate_device_remove() which starts the first action involved with removing a device. I will see what I can do, but there are areas of this code which can't have their initialisation brought any further forward. ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 01/27] tools/libxl: Fix libxl__ev_child_inuse() check for not-yet-initialised children
On Tue, 2015-06-16 at 14:36 +0100, Andrew Cooper wrote: On 16/06/15 14:21, Ian Campbell wrote: On Mon, 2015-06-15 at 14:44 +0100, Andrew Cooper wrote: Shortly, libxl will be juggling multiple parallel operations, and will possibly have to take error decisions before some tasks have been set up. It would be preferable, I think, to arrange to call libxl__ev_child_init on all such libxl__ev_child structs either up front or certainly before there is any possibility of needing to unwind them. Such an init would at worst correspond to exactly the place where the zeroed structure you refer to is zeroed. It is possible that one bit fails before it can be calculated whether the second bit needs to start or not. You can call libxl__ev_child_init without needing to know that, i.e. you can do them all at the point where you allocate/initialise the containing struct. At the moment, all bits in libxl in this area do initialisation immediately before use; most bits are even initialised in the function which starts their actions. Some bits are initialised differently depending on the path taken to get to the initialisation site. It would be non-trivial to initialise everything appropriately at the very start. You don't need to fully init, just call libxl__ev_child_init in order to arrange for correct behaviour from libxl__ev_child_inuse and friends. Actually turning it into a useful child can stay where it is if it is tricky to arrange for those things to happen at the same time. Ian. ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 01/27] tools/libxl: Fix libxl__ev_child_inuse() check for not-yet-initialised children
On Tue, 2015-06-16 at 15:05 +0100, Andrew Cooper wrote: On 16/06/15 14:47, Ian Jackson wrote: Andrew Cooper writes (Re: [PATCH 01/27] tools/libxl: Fix libxl__ev_child_inuse() check for not-yet-initialised children): It is possible that one bit fails before it can be calculated whether the second bit needs to start or not. At the moment, all bits in libxl in this area do initialisation immediately before use; most bits are even initialised in the function which starts their actions. Some bits are initialised differently depending on the path taken to get to the initialisation site. As a rule of thumb a function libxl__initiate_foo_ which takes a libxl__foo_state* should do this initialisation for the whole libxl__foo_state. I don't see why you can't do that. The only example of libxl__initiate_foo_ is libxl__initiate_device_remove() which starts the first action involved with removing a device. I will see what I can do, but there are areas of this code which can't have their initialisation brought any further forward. I think you need to consider make the struct into some known good initial/default state as something separate from turn the struct into a useful thing to achieve its goal. Only the first bit needs to move IMHO (although if they both can that is nice too) Ian. ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
[Xen-devel] [PATCH 01/27] tools/libxl: Fix libxl__ev_child_inuse() check for not-yet-initialised children
Shortly, libxl will be juggling multiple parallel operations, and will possibly have to take error decisions before some tasks have been set up. No child process of libxl will ever have a pid of 0, so gate libxl__ev_child_inuse() on a pid strictly greater than 0. This makes it safe to use on a zeroed structure of a task which has not yet been set up. Signed-off-by: Andrew Cooper andrew.coop...@citrix.com CC: Ian Campbell ian.campb...@citrix.com CC: Ian Jackson ian.jack...@eu.citrix.com CC: Wei Liu wei.l...@citrix.com --- This change does make libxl__ev_child_init() functionally useless. I am undecided between leaving it in place in case it is useful in the future, or to remove it completely. --- tools/libxl/libxl_internal.h |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h index e96d6b5..6226c18 100644 --- a/tools/libxl/libxl_internal.h +++ b/tools/libxl/libxl_internal.h @@ -880,7 +880,7 @@ _hidden pid_t libxl__ev_child_fork(libxl__gc *gc, libxl__ev_child *childw_out, static inline void libxl__ev_child_init(libxl__ev_child *childw_out) { childw_out-pid = -1; } static inline int libxl__ev_child_inuse(const libxl__ev_child *childw_out) -{ return childw_out-pid = 0; } +{ return childw_out-pid 0; } /* Useable (only) in the child to once more make the ctx useable for * xenstore operations. logs failure in the form what: error -- 1.7.10.4 ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel