9:21 p.m.
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
--
Thanks,
Hu Tao