Re: [PATCH 4 of 5] tools/libxl: Control network buffering in remus callbacks

[Andrew Cooper <andrew.cooper3@citrix.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3131 bytes --]

On 29/08/13 22:49, Shriram Rajagopalan wrote:
> On Mon, Aug 26, 2013 at 6:28 PM, Andrew Cooper
> <andrew.cooper3@citrix.com <mailto:andrew.cooper3@citrix.com>> wrote:
>
>     On 26/08/2013 00:45, rshriram@cs.ubc.ca
>     <mailto:rshriram@cs.ubc.ca> wrote:
>     > # HG changeset patch
>     > # User Shriram Rajagopalan <rshriram@cs.ubc.ca
>     <mailto: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
>     <mailto: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. */
>
>     Why does this comment need to move?
>
>     >  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)
>
>     split to new line please.
>
>     > return is_suspended;
>     > +
>     > +    if (is_suspended) {
>     > +        if (libxl__remus_netbuf_start_new_epoch(gc, dss->domid,
>     > +                                                remus_ctx))
>     > +            return !is_suspended;
>     > +    }
>     > +
>     > +    return is_suspended;
>
>     is_suspended is logically a boolean, so should be bool_t.
>      Unhelpfully,
>     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.
>  

Indeed.  Sorry.  (I had Xen on the brain when reviewing, which does use
bool_t rather than bool).

~Andrew

[-- Attachment #1.2: Type: text/html, Size: 6238 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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