At 01/25/2011 08:37 AM, Eric Blake Write:
> On 01/23/2011 07:20 PM, Wen Congyang wrote:
>> From: Hu Tao <hutao@xxxxxxxxxxxxxx>
>> 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:
unfortunately, 'make syntax-check' does not catch these violations.
>
> 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.
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list