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
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list