[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 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?

>      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.

> @@ -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
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.

> +        } 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

Attachment: signature.asc
Description: OpenPGP digital signature


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