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

[Wen Congyang]

At 01/25/2011 08:37 AM, Eric Blake Write:
> On 01/23/2011 07:20 PM, Wen Congyang wrote:
>> From: Hu Tao <hutao at cn.fujitsu.com>
>> 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.
>