Re: [Xen-devel] [PATCH for-4.6 3/8] tools/libxl: Make libxl__conversion_helper_abort() safe to use

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Andrew Cooper writes ("[PATCH for-4.6 3/8] tools/libxl: Make 
libxl__conversion_helper_abort() safe to use"):
> STATE_AO_GC(chs->ao) uses chs->ao before determining whether the helper
> is active.  In the case that the helper has not been started, its ao
> will not have been set up.

I am not happy with this, because we are further complicating the
rules for the abstract state of the chs.

As I said on irc,

19:07 <Diziet> The problem here is that the rules are not written down
               clearly enough.  In this particular case the set of
               abstract states is written down.
19:07 <Diziet> The Undefined / Idle / Active thing from libxl__ev_*
               doesn't quite apply because libxl__ev_blah_register
               takes things as arguments, rather than expecting the
               caller to fill them in.

So at the very least if you are to do this I would like to see a
comment explicitly and formally setting out the abstract states of a
chs.  For examples of how to write this, look for the doc comments
which show up in searches for:
  - libxl__xs_transaction_start
  - libxl__ev_KIND_register
This comment should explain that libxl__conversion_helper_abort is
a special case.

I think you will find that trying to write this down in this form
results in your proposal requiring an extra abstract state.


Instead, I think it would be preferable for all similar operations to
all work the same.  And then, such a comment about the abstract states
could be written in a general way so that it describes
libxl__datacopier and libxl__openpty too.  In this case IMO while it
would be nice for there to be a comment, it's not essential, because
it is (very nearly) implied by the comment about libxl__ev_KIND_*,
when read together with the comment saying the caller must fill in ao.

So as I say I think it is simpler to require that the foostate->ao be
valid on all entries to libxl__foo_abort().  This is indeed the
current behaviour of:
  - libxl__datacopier_kill
  - libxl__save_helper_abort

I think it is only an accident that libxl__xswait_stop and
libxl__stream_{read,write}_abort don't dereference xswa->ao.


I would also like a comment (which is currently missing) about who is
supposed to fill in the public fields in the chs.  See for example the
comment at the top of libxl__datacopier_state.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel