Re: [Xen-devel] [PATCH 01/27] tools/libxl: Fix libxl__ev_child_inuse() check for not-yet-initialised children

2015-06-16 Thread Ian Jackson
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

2015-06-16 Thread Ian Jackson
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

2015-06-16 Thread Andrew Cooper
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

2015-06-16 Thread Ian Campbell
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

2015-06-16 Thread Andrew Cooper
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

2015-06-16 Thread Ian Campbell
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

2015-06-16 Thread Ian Campbell
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

2015-06-15 Thread Andrew Cooper
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