Re: [Xen-devel] [PATCH 08/19] libxl: wait for qemu to acknowledge logdirty command

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Thu, 2012-06-14 at 16:47 +0100, Ian Jackson wrote:
> Ian Campbell writes ("Re: [Xen-devel] [PATCH 08/19] libxl: wait for qemu to 
> acknowledge logdirty command"):
> > On Fri, 2012-06-08 at 18:34 +0100, Ian Jackson wrote:
> > > The current migration code in libxl instructs qemu to start or stop
> > > logdirty, but it does not wait for an acknowledgement from qemu before
> > > continuing.  This might lead to memory corruption (!)
> ...
> > > +static void logdirty_init(libxl__logdirty_switch *lds)
> > > +{
> > > +    lds->cmd_path = 0;
> > > +    libxl__ev_xswatch_init(&lds->watch);
> > > +    libxl__ev_time_init(&lds->timeout);
> > 
> > I meant to mention this once before when reviewing some patch or other
> > but I'm not sure I actually did, so at the risk of repeating myself:
> > 
> > This watch with timeout pattern seems to be reasonably common (in fact,
> > I'm not sure we have any watches without a timeout). It might be a
> > candidate for a specific helper routine in the future?
> 
> Perhaps.  We should think about this.

Yes, I should have said the phrase "4.3" in there somewhere.

>   I'm not sure it's necessary
> now.  The benefit might be relatively small, as the callback function
> would more complicated.

Could still have two callbacks, and just helpers for the setup &
teardown phases?

> Perhaps we should integrate the single xs read which most of these callers 
> also have.

You mean pass in the value of the watched key? That's also a
possibility.

> 
> > > +static void switch_logdirty_xswatch(libxl__egc *egc, libxl__ev_xswatch 
> > > *watch,
> > > +                            const char *watch_path, const char 
> > > *event_path)
> > > +{
> ...
> > > + out:
> > > +    /* rc < 0: error
> > > +     * rc == 0: ok, we are done
> > > +     * rc == +1: need to keep waiting
> > > +     */
> > > +    libxl__xs_transaction_abort(gc, &t);
> > > +
> > > +    if (!rc) {
> > > +        switch_logdirty_done(egc,dss,1);
> > > +    } else if (rc < 0) {
> > > +        LOG(ERROR,"logdirty switch: response read failed (rc=%d)",rc);
> > 
> > Is it only "response read failed" which can cause us to end up here,
> > looks like the read, rm etc could do it?
> 
> Oh yes.  I should fix that message.  (All of these paths have already
> logged something already, but another message probably doesn't hurt.)
> 
> Thanks,
> Ian.



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