[libvirt] [PATCH 02/10] qemu: migration: Refactor code now that we assume support for fd migration

[Ján Tomko]

On Tue, Feb 16, 2016 at 04:29:44PM +0100, Peter Krempa wrote:
> After removing capability check for fd migration the code that was left
> behind didn't make quite sense. The old exec migration would be used in
> case when pipe() failed. Remove the old code and make failure of pipe()
> a hard error.
> 
> This additionally removes usage of virCgroupAllowDevicePath outside of
> qemu_cgroup.c.
> ---
>  src/qemu/qemu_driver.c    |   4 +-
>  src/qemu/qemu_migration.c | 114 ++++++++++++++++------------------------------
>  src/qemu/qemu_migration.h |   2 +-
>  3 files changed, 41 insertions(+), 79 deletions(-)
> 

> @@ -5972,54 +5972,28 @@ qemuMigrationToFile(virQEMUDriverPtr driver, virDomainObjPtr vm,
>          return -1;
>      }
> 
> -    if ((!compressor || pipe(pipeFD) == 0)) {
> -        /* All right! We can use fd migration, which means that qemu
> -         * doesn't have to open() the file, so while we still have to
> -         * grant SELinux access, we can do it on fd and avoid cleanup
> -         * later, as well as skip futzing with cgroup.  */
> -        if (virSecurityManagerSetImageFDLabel(driver->securityManager, vm->def,
> -                                              compressor ? pipeFD[1] : fd) < 0)
> -            goto cleanup;
> -        bypassSecurityDriver = true;
> -    } else {
> -        /* Phooey - we have to fall back on exec migration, where qemu
> -         * has to popen() the file by name, and block devices have to be
> -         * given cgroup ACL permission.  We might also stumble on
> -         * a race present in some qemu versions where it does a wait()
> -         * that botches pclose.  */
> -        if (virCgroupHasController(priv->cgroup,
> -                                   VIR_CGROUP_CONTROLLER_DEVICES)) {
> -            int rv = virCgroupAllowDevicePath(priv->cgroup, path,
> -                                              VIR_CGROUP_DEVICE_RW);
> -            virDomainAuditCgroupPath(vm, priv->cgroup, "allow", path, "rw", rv == 0);
> -            if (rv == 1) {
> -                /* path was not a device, no further need for cgroup */
> -            } else if (rv < 0) {
> -                goto cleanup;
> -            }
> -        }
> -        if ((!bypassSecurityDriver) &&
> -            virSecurityManagerSetSavedStateLabel(driver->securityManager,
> -                                                 vm->def, path) < 0)
> -            goto cleanup;
> -        restoreLabel = true;

This is the only place that set restoreLabel to true.

> +    if (compressor && pipe(pipeFD) < 0) {
> +        virReportSystemError(errno, "%s",
> +                             _("Failed to create pipe for migration"));
> +        return -1;
>      }
> 
> +    /* All right! We can use fd migration, which means that qemu
> +     * doesn't have to open() the file, so while we still have to
> +     * grant SELinux access, we can do it on fd and avoid cleanup
> +     * later, as well as skip futzing with cgroup.  */
> +    if (virSecurityManagerSetImageFDLabel(driver->securityManager, vm->def,
> +                                          compressor ? pipeFD[1] : fd) < 0)
> +        goto cleanup;

Can you really relabel a pipe after it's been created?

> +    bypassSecurityDriver = true;
> +
>      if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
>          goto cleanup;
> 
>      if (!compressor) {
> -        const char *args[] = { "cat", NULL };
> -
> -        if (priv->monConfig->type == VIR_DOMAIN_CHR_TYPE_UNIX) {

While it seems it's (in theory) possible to get a different monType through
qemuProcessReconnect, I think we can safely ignore it.

> -            rc = qemuMonitorMigrateToFd(priv->mon,
> -                                        QEMU_MONITOR_MIGRATE_BACKGROUND,
> -                                        fd);
> -        } else {
> -            rc = qemuMonitorMigrateToFile(priv->mon,
> -                                          QEMU_MONITOR_MIGRATE_BACKGROUND,
> -                                          args, path, offset);
> -        }
> +        rc = qemuMonitorMigrateToFd(priv->mon,
> +                                    QEMU_MONITOR_MIGRATE_BACKGROUND,
> +                                    fd);
>      } else {
>          const char *prog = compressor;
>          const char *args[] = {
> @@ -6027,33 +6001,28 @@ qemuMigrationToFile(virQEMUDriverPtr driver, virDomainObjPtr vm,
>              "-c",
>              NULL
>          };
> -        if (pipeFD[0] != -1) {
> -            cmd = virCommandNewArgs(args);
> -            virCommandSetInputFD(cmd, pipeFD[0]);
> -            virCommandSetOutputFD(cmd, &fd);
> -            virCommandSetErrorBuffer(cmd, &errbuf);
> -            virCommandDoAsyncIO(cmd);
> -            if (virSetCloseExec(pipeFD[1]) < 0) {
> -                virReportSystemError(errno, "%s",
> -                                     _("Unable to set cloexec flag"));
> -                ignore_value(qemuDomainObjExitMonitor(driver, vm));
> -                goto cleanup;
> -            }
> -            if (virCommandRunAsync(cmd, NULL) < 0) {
> -                ignore_value(qemuDomainObjExitMonitor(driver, vm));
> -                goto cleanup;
> -            }
> -            rc = qemuMonitorMigrateToFd(priv->mon,
> -                                        QEMU_MONITOR_MIGRATE_BACKGROUND,
> -                                        pipeFD[1]);
> -            if (VIR_CLOSE(pipeFD[0]) < 0 ||
> -                VIR_CLOSE(pipeFD[1]) < 0)
> -                VIR_WARN("failed to close intermediate pipe");
> -        } else {
> -            rc = qemuMonitorMigrateToFile(priv->mon,
> -                                          QEMU_MONITOR_MIGRATE_BACKGROUND,
> -                                          args, path, offset);

This was the last usage of qemuMonitorMigrateToFile.

> +
> +        cmd = virCommandNewArgs(args);
> +        virCommandSetInputFD(cmd, pipeFD[0]);
> +        virCommandSetOutputFD(cmd, &fd);
> +        virCommandSetErrorBuffer(cmd, &errbuf);
> +        virCommandDoAsyncIO(cmd);
> +        if (virSetCloseExec(pipeFD[1]) < 0) {
> +            virReportSystemError(errno, "%s",
> +                                 _("Unable to set cloexec flag"));
> +            ignore_value(qemuDomainObjExitMonitor(driver, vm));
> +            goto cleanup;
>          }
> +        if (virCommandRunAsync(cmd, NULL) < 0) {
> +            ignore_value(qemuDomainObjExitMonitor(driver, vm));
> +            goto cleanup;
> +        }
> +        rc = qemuMonitorMigrateToFd(priv->mon,
> +                                    QEMU_MONITOR_MIGRATE_BACKGROUND,
> +                                    pipeFD[1]);
> +        if (VIR_CLOSE(pipeFD[0]) < 0 ||
> +            VIR_CLOSE(pipeFD[1]) < 0)
> +            VIR_WARN("failed to close intermediate pipe");
>      }
>      if (qemuDomainObjExitMonitor(driver, vm) < 0)
>          goto cleanup;
> @@ -6105,13 +6074,6 @@ qemuMigrationToFile(virQEMUDriverPtr driver, virDomainObjPtr vm,
>                                                   vm->def, path) < 0)
>          VIR_WARN("failed to restore save state label on %s", path);
> 

The hunk of code not displayed here will disappear after you delete the
unused bool.

ACK with restoreLabel removed.

Your call if you remove qemuMonitorMigrateToFile here or in a separate
patch.

Jan
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20160216/84963bd4/attachment-0001.sig>