[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 2/2] Cancel migration/dump if user presses Ctrl-C when migration/dump is in progress



On Thu, Dec 23, 2010 at 09:05:58AM -0700, Eric Blake wrote:
> On 12/23/2010 02:07 AM, Hu Tao wrote:
> > ---
> 
> No commit message beyond the title?
> 
> > @@ -208,6 +211,7 @@ typedef struct __vshCmd {
> >  typedef struct __vshControl {
> >      char *name;                 /* connection name */
> >      virConnectPtr conn;         /* connection to hypervisor (MAY BE NULL) */
> > +    virDomainPtr dom;
> 
> No comment as to it's purpose?

Will update it if I update the patch.

> 
> >      vshCmd *cmd;                /* the current command */
> >      char *cmdstr;               /* string with command */
> >      int imode;                  /* interactive mode? */
> > @@ -221,6 +225,9 @@ typedef struct __vshControl {
> >      int log_fd;                 /* log file descriptor */
> >      char *historydir;           /* readline history directory name */
> >      char *historyfile;          /* readline history file name */
> > +
> > +    virMutex mutex;
> > +    virThreadPoolPtr threadPool;
> 
> Likewise.

Will update it if I update the patch.

> 
> > @@ -509,6 +518,44 @@ static void vshCatchDisconnect(int sig, siginfo_t * siginfo,
> >      if ((sig == SIGPIPE) || (siginfo->si_signo == SIGPIPE))
> >          disconnected++;
> >  }
> > +#endif
> > +
> > +#ifdef SIGINT
> 
> Unnecessary ifdef - SIGINT is available on all platforms (it is required
> by C).
> 
> > +static void vshCatchInt(int sig, siginfo_t *siginfo,
> > +                        void *context ATTRIBUTE_UNUSED) {
> > +    vshControl *ctl = &_ctl;
> > +    vshCmd *cmds, *c;
> > +    char *cmdStr = NULL;
> > +
> > +    virMutexLock(&ctl->mutex);
> > +    if (!ctl->dom)
> > +        goto error;
> > +
> > +    if (virAsprintf(&cmdStr, "domjobabort %s", ctl->dom->name) < 0)
> 
> _BIG_ no-no.  virAsprintf calls malloc(), which is NOT signal-safe (that
> is, if the signal arrived while some other thread was in the middle of a
> malloc(), you've just caused deadlock).  The only safe thing to do in a
> signal handler is to set some state variables that are then checked in
> the main processing loop, when you are back in a safe state to act on a
> (minimally-delayed) basis on the signal.
> 
> See how daemon/libvirtd.c uses qemudDispatchSignalEvent (although that's
> being rewritten by danpb's factorization into a cleaner rpc
> server/client model).
> 
> > +        goto error;
> > +
> > +    if ((sig == SIGINT) || (siginfo->si_signo == SIGINT)) {
> > +        if (!vshConnectionUsability (ctl, ctl->conn))
> > +            goto error;
> > +
> > +        vshCommandStringParse(ctl, cmdStr);
> > +        cmds = ctl->cmd;
> > +        ctl->cmd = NULL;
> > +        virMutexUnlock(&ctl->mutex);
> > +        do {
> > +            c = cmds;
> > +            cmds = cmds->next;
> > +            c->next = NULL;
> > +            ignore_value(virThreadPoolSendJob(ctl->threadPool, c, true));
> 
> Continuing in the vein of bad practice - blocking on another thread to
> complete inside a signal handler is crazy.  But setting up a threadpool

I did a copy&paste and forgot to change true to false :)

> to handle signals in a separate thread means that your signal-handling
> thread is already independently available to cancel the job that is
> underway in the primary thread.

The intention of setting up a threadpool is to submit another command in
a seprate thread while there is already a command being processed. If we
do it in one thread, then we will end up with deaklock eventually. Take
dump as example:

  1. submit command dump 
  2. polling for reply from RPC server
  3. SIGINT(poll is interrupted)
  4. submit command domjobabort form signal handler
  5. (in remoteIO())there is a priv->waitDispatch, so go to
     sleep(vifCondWait). The impl of remote driver is thread-aware, and
     this thread (processing command domjobabort) is supposed to be
     waken up by the thread that is having priv->waitDispatch(processing
     command dump). But we are the same thread, so deadlock.


Well, I have to say that the patch is nasty. Do you (or anyone) have a
good idea to achieve the goal to cancel the active job if user presses
Ctrl-C? Thanks.

> 
> > +        } while (cmds);
> > +    }
> > +    VIR_FREE(cmdStr);
> > +    return;
> > +error:
> > +    virMutexUnlock(&ctl->mutex);
> > +    VIR_FREE(cmdStr);
> > +}
> > +#endif
> >  
> >  /*
> >   * vshSetupSignals:
> > @@ -520,17 +567,20 @@ static void
> >  vshSetupSignals(void) {
> >      struct sigaction sig_action;
> >  
> > -    sig_action.sa_sigaction = vshCatchDisconnect;
> >      sig_action.sa_flags = SA_SIGINFO;
> >      sigemptyset(&sig_action.sa_mask);
> >  
> > +#ifdef SIGPIPE
> 
> Might be unnecessary - gnulib guarantees that a working replacement for
> SIGPIPE is available on mingw, which is the only platform that lacks it
> natively.  Oh, but that module is currently LGPLv3+, I'll have to ask on
> the gnulib list if people are willing to relax it to LGPLv2+ so we can
> use it.
> 
> -- 
> Eric Blake   eblake redhat com    +1-801-349-2682
> Libvirt virtualization library http://libvirt.org
> 



-- 
Thanks,
Hu Tao


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]