Re: [PATCH] tools/libxl: Fixes to stream v2 task joining logic

[Ian Jackson <Ian.Jackson@eu.citrix.com>]

Andrew Cooper writes ("[PATCH] tools/libxl: Fixes to stream v2 task joining logic"):
> During review of the libxl migration v2 series, I changes the task
> joining logic, but clearly didn't think the result through
> properly. This patch fixes several errors.

This would have been much easier to review if it had been split into 3
patches.  I have gone mostly by the commit message because it was hard
to see hunk belonged to what.

> 3) Avoid stacking of check_all_finished() via synchronous teardown of
> tasks.  If the _abort() functions call back synchronously,
> stream->completion_callback() ends up getting called twice, as first and
> last check_all_finished() frames observe each task being finished.

I think this part of the patch is fine.


> 1) Do not call check_all_finished() in the success cases of
> libxl__xc_domain_{save,restore}_done().  It serves no specific purpose
> as the save helper state will report itself as inactive by this point,
> and avoids triggering a second stream->completion_callback() in the case
> that write_toolstack_record()/stream_continue() record errors
> synchronously themselves.

"Serves no specific purpose" other than having a single exit path,
which makes matters much less confusing.

I think the problem may be that libxl__xc_domain_{save,restore}_done
fail to "return" after "write_toolstack_record" and "stream_continue".
That seems like simply a bug.  I'm sorry that I didn't notice it in
review.

In general each callback function should set up exactly one other
callback.  If it does anything else then reentrancy hazards arise.

Also, it is confusing and perhaps wrong that write_toolstack_record
calls stream_complete.  What if there are other threads of control
outstanding ?


> 2) Only ever set stream->rc in stream_complete().  The first version of
> the migration v2 series had separate rc and joined_rc parameters, where
> this logic worked.  However when combining the two, the teardown path
> fails to trigger if stream_done() records stream->rc itself.  A side
> effect of this is that stream_done() needs to take an rc parameter.

"the teardown path fails to trigger if stream_done() records
stream->rc itself" but in the code I am looking at neither of the
functions stream_done assign to rc.

Maybe I would understand this if I were looking at only this set of
diff hunks.


Ian.