On Wed, Aug 7, 2013 at 11:38 AM, Ian Jackson <Ian.Jackson@eu.citrix.com>wrote:

> >
> > +#include <netlink/route/qdisc/plug.h>
>
> We need a configure option to disable this, in case the host doesn't
> have it.
>
>
>  #include <xc_dom.h>
> >  #include <xen/hvm/hvm_info_table.h>
> >  #include <xen/hvm/hvm_xs_strings.h>
> > @@ -1212,12 +1213,36 @@ int libxl__toolstack_save(uint32_t domid
> >
> >  /*----- remus callbacks -----*/
> >
> > +/* REMUS TODO: Issue disk checkpoint reqs. */
> >  static int libxl__remus_domain_suspend_callback(void *data)
> ...
> > +    if (!remus_ctx->num_netbufs) return is_suspended;
> > +
> > +    if (is_suspended) {
> > +        for (i = 0; i < remus_ctx->num_netbufs; ++i) {
> > +            ret =
> rtnl_qdisc_plug_buffer(remus_ctx->netbuf_qdisc_list[i]);
> > +            if (!ret)
> > +                ret = rtnl_qdisc_add(remus_ctx->nlsock,
> remus_ctx->netbuf_qdisc_list[i],
>
> Needs the line length reducing to 70-75 characters, maximum.
> (Multiple occcurrences of this problem in the patch.)
>
> > @@ -1255,13 +1279,104 @@ static void libxl__remus_domain_checkpoi
> >  static void remus_checkpoint_dm_saved(libxl__egc *egc,
> >                                        libxl__domain_suspend_state *dss,
> int rc)
> >  {
> > -    /* REMUS TODO: Wait for disk and memory ack, release network buffer
> */
> > -    /* REMUS TODO: make this asynchronous */
> > -    assert(!rc); /* REMUS TODO handle this error properly */
> ...
> > +    assert(!rc);
>
> Does this TODO not need to remain ?
>
> > +            if (!ret)
> > +                ret = rtnl_qdisc_add(remus_ctx->nlsock,
> remus_ctx->netbuf_qdisc_list[i],
> > +                                     NLM_F_REQUEST);
> > +            if (ret) {
> > +                LOG(ERROR, "Cannot release buffer from %s:%s",
> > +                    dss->remus->netbuf_iflist[i], nl_geterror(ret));
> > +                ret= 0;
>
> Missing space after "=".
>
> ...
> > +    usleep(dss->remus_ctx->interval * 1000);
>
> This is still pretty bad, surely ?
>
>
Was thinking of updating this in a separate patch.. But I guess since this
line
is changing, might as well do it in this patch.


> > +    rtnl_link_put(ifb);
> > +    return remus_ctx;
> > +
> > + end:
> > +    if (ifb) rtnl_link_put(ifb);
> > +    if (qdisc_cache) nl_cache_free(qdisc_cache);
> > +    if (nlsock) nl_close(nlsock);
>
> I think this can perhaps leak remus_ctx and qdisc.
>
>
There is no issue of leaking qdisc. Because it simply obtains references
from
qdisc_cache, which is freed if things fail.

WRT remus_ctx, I had intentionally skipped the free call.
 Its allocated in the GC context. [the same one used by domain_suspend,
etc].
And if this call fails, then the parent call libxl_domain_remus_start will
fail.

I assumed that when libxl_free_all was called at this point, remus_ctx
would also be
freed. Am I missing something in the overall control flow, that would cause
this leak ?

Thanks,
> Ian.
>
>