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

On 01/23/2011 07:20 PM, Wen Congyang wrote: > From: Hu Tao <hutao(a)cn.fujitsu.com&gt; > Date: Tue, 11 Jan 2011 15:01:24 +0800 > Subject: [PATCH 1/3] Cancel migration if user presses Ctrl-C when migration is in progress > > While migration is in progress and virsh is waiting for its > completion, user may want to terminate the progress by pressing > Ctrl-C. But virsh just exits on user's Ctrl-C leaving migration > in background that user isn't even aware of. It's not reasonable. > > This patch changes the behaviour for migration. For other > commands Ctrl-C still terminates virsh itself. > > > +static bool intCatched = FALSE; s/Catched/Caught/g Don't mix FALSE and bool (either int and FALSE, or bool and false; with the caveat that int and TRUE/FALSE are historical baggage in this file and we are gradually trying to convert them over to bool and true/false). > + > +static void vshCatchInt(int sig ATTRIBUTE_UNUSED, > + siginfo_t *siginfo ATTRIBUTE_UNUSED, > + void *context ATTRIBUTE_UNUSED) > +{ > + intCatched = TRUE; Furthermore, POSIX states that variables used to communicate between signal handlers and other threads should be [volatile] sig_atomic_t, rather than bool (why? because on some platforms where bool is a byte and smaller than a machine word, it may result in a read-modify-write cycle that could interfere with neighboring bytes, when contrasted with a machine word write of using a sig_atomic_t). So use of bool for this one variable is non-portable to begin with. > > -static int > -cmdMigrate (vshControl *ctl, const vshCmd *cmd) > +static void > +doMigrate (void *opaque) > { > + char ret = '1'; > virDomainPtr dom = NULL; > const char *desturi; > const char *migrateuri; > const char *dname; > - int flags = 0, found, ret = FALSE; > + int flags = 0, found; > + sigset_t sigmask, oldsigmask; > + struct { > + vshControl *ctl; > + vshCmd *cmd; > + int writefd; > + } *data = opaque; Rather than declaring the struct inline, let's typedef it in advance, so that both caller and consumer share the same struct declaration (that way, if we ever have to add to the struct, we only have to do it in one place). > + vshControl *ctl = data->ctl; > + vshCmd *cmd = data->cmd; > + > + sigemptyset(&sigmask); > + sigaddset(&sigmask, SIGINT); > + if (pthread_sigmask(SIG_BLOCK, &sigmask, &oldsigmask) < 0) > + goto out_sig; Mingw lacks pthread_sigmask, and gnulib doesn't have it (yet). This may cause some compilation portability problems to mingw, but I can help with that (and it can be a followup patch - I won't make it a condition for getting this much-needed improvement in). > +static int > +cmdMigrate (vshControl *ctl, const vshCmd *cmd) > +{ > + > + if (!(dom = vshCommandOptDomain (ctl, cmd, NULL))) Style - new code should use vshCommandOptDomain(ctl, without a space. > + > + intCatched = FALSE; > + sig_action.sa_sigaction = vshCatchInt; > + sig_action.sa_flags = SA_SIGINFO; > + sigemptyset(&sig_action.sa_mask); > + sigaction(SIGINT, &sig_action, &old_sig_action); > + > + pollfd.fd = p[0]; > + pollfd.events = POLLIN; > + pollfd.revents = 0; > + > + ret = poll(&pollfd, 1, -1); > + if (ret > 0) { > + if (saferead(p[0], &retchar, sizeof(retchar)) > 0) { > + if (retchar == '0') > + ret = TRUE; > + else > + ret = FALSE; > + } else > + ret = FALSE; > + } else if (ret < 0) { > + if (errno == EINTR && intCatched) { > + virDomainAbortJob(dom); > + ret = FALSE; > + intCatched = FALSE; > + } Don't you need a retry loop here, if you get EINTR but an interrupt was not marked as caught? > + } else { > + /* timed out */ > + ret = FALSE; > + } > + > + sigaction(SIGINT, &old_sig_action, NULL); > + > + virThreadJoin(&workerThread); > + > +cleanup: > + if (dom) > + virDomainFree(dom); > + if (p[0] != -1) { > + close(p[0]); > + close(p[1]); 'make syntax-check' should have caught these violations (if it didn't, then that's something that we should fix). This should be:

cleanup: virDomainFree(dom); VIR_FORCE_CLOSE(p[0]); VIR_FORCE_CLOSE(p[1]); which avoids issues with double-close, and which avoids useless if statements on free-like functions.