On Mon, Aug 26, 2013 at 6:28 PM, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
On 26/08/2013 00:45, rshriram@cs.ubc.ca wrote:Why does this comment need to move?
> # HG changeset patch
> # User Shriram Rajagopalan <rshriram@cs.ubc.ca>
> # Date 1377473611 25200
> # Node ID c6804ccfe660cb9c373c5f53a8996d0443316684
> # Parent 4b23104828c09218aa7f9fbde1578bb6706e13d6
> tools/libxl: Control network buffering in remus callbacks
>
> This patch constitutes the core network buffering logic.
> and does the following:
> a) create a new network buffer when the domain is suspended
> (remus_domain_suspend_callback)
> b) release the previous network buffer pertaining to the
> committed checkpoint (remus_domain_checkpoint_dm_saved)
>
> Signed-off-by: Shriram Rajagopalan <rshriram@cs.ubc.ca>
>
> diff -r 4b23104828c0 -r c6804ccfe660 tools/libxl/libxl_dom.c
> --- a/tools/libxl/libxl_dom.c Sun Aug 25 16:33:29 2013 -0700
> +++ b/tools/libxl/libxl_dom.c Sun Aug 25 16:33:31 2013 -0700
> @@ -1213,12 +1213,28 @@ int libxl__toolstack_save(uint32_t domid
>
> /*----- remus callbacks -----*/
>
> +/* REMUS TODO: Issue disk checkpoint reqs. */
split to new line please.
> static int libxl__remus_domain_suspend_callback(void *data)
> {
> - /* REMUS TODO: Issue disk and network checkpoint reqs. */
> - return libxl__domain_suspend_common_callback(data);
> + libxl__save_helper_state *shs = data;
> + libxl__domain_suspend_state *dss = CONTAINER_OF(shs, *dss, shs);
> + libxl__remus_ctx *remus_ctx = dss->remus_ctx;
> + int is_suspended = 0;
> + STATE_AO_GC(dss->ao);
> +
> + is_suspended = libxl__domain_suspend_common_callback(data);
> + if (!remus_ctx->netbuf_ctx)
is_suspended is logically a boolean, so should be bool_t. Unhelpfully,
> return is_suspended;
> +
> + if (is_suspended) {
> + if (libxl__remus_netbuf_start_new_epoch(gc, dss->domid,
> + remus_ctx))
> + return !is_suspended;
> + }
> +
> + return is_suspended;
libxl__domain_suspend_common_callback() returns an int, although its
implementation strictly returns 0 on failure and 1 on success.
IMO, It would probably be best to have "bool_t is_suspended" set to
"!!libxl__domain_suspend_common_callback()", at which the subsequent
return !is_suspended doesn't look as suspect, although that is just a
matter of style.
Did you mean bool (not bool_t) ? Because I didnt see a typedef in libxl folder.