Re: [Xen-devel] [PATCH v5 14/15] libxl: Change libxl__domain_suspend_device_model() to be async.

[Ian Jackson <ian.jackson@xxxxxxxxxx>]

Anthony PERARD writes ("[PATCH v5 14/15] libxl: Change 
libxl__domain_suspend_device_model() to be async."):
> This create an extra step for the two call sites of the function.
...
> -int libxl__domain_suspend_device_model(libxl__gc *gc,
> +int libxl__domain_suspend_device_model(libxl__egc *egc,
>                                         libxl__domain_suspend_state *dsps)
>  {
> +    STATE_AO_GC(dsps->ao);
>      int ret = 0;
>      uint32_t const domid = dsps->domid;
>      const char *const filename = dsps->dm_savefile;
> @@ -94,6 +95,7 @@ int libxl__domain_suspend_device_model(libxl__gc *gc,
>          return ERROR_INVAL;
>      }
>  
> +    dsps->callback_device_model_done(egc, dsps, ret);
>      return ret;
>  }

This function has broken error handling now, I'm afraid.  The actual
problem is that if it reaches the end there with rc!=0 (not currently
possible but might occur in the future), it will *both* call the
callback and return nonzero.

The root cause of this is the decision to make the function have two
ways of reporting errors: both via callback, and via return value.
Normally it is better when making a function like this async, to make
it return void.  (After making it handle all of its errors with `goto
out', first.)  Then all errors go via the callback.

That does introduce a new reentrancy hazard.  But in general we have
been able to deal with that by putting a doc comment
/* ... perhaps reentrantly */ or some such nearby.

That is sufficient when all the call sites the last thing in their
function.  Which I think is true here ?

If not, then you do need both error paths but then you mustn't call
_done reentrantly at all, as you do.  And that is not compatible with
easily converting this synchronous function into an async one, as you
are doing.

Thanks,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel