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

[Eric Blake]

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:

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.

-- 
Eric Blake   eblake at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 619 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20110124/2bbc7deb/attachment-0001.sig>