Re: [libvirt] [PATCH] qemu: call drive_unplug in DetachPciDiskDevice

On Thu, Oct 21, 2010 at 08:50:35AM -0500, Ryan Harper wrote: > Currently libvirt doesn't confirm whether the guest has responded to the > disk removal request. In some cases this can leave the guest with > continued access to the device while the mgmt layer believes that it has > been removed. With a recent qemu monitor command[1] we can > deterministically revoke a guests access to the disk (on the QEMU side) > to ensure no futher access is permitted. > > This patch adds support for the drive_unplug() command and introduces it > in the disk removal paths. There is some discussion to be had about how > to handle the case where the guest is running in a QEMU without this > command (and the fact that we currently don't have a way of detecting > what monitor commands are available). Basically we try to run the command and then catch the failure. For QMP, we can check for a error with a class of 'CommandNotFound', For HMP, QEMU will print 'unknown command' in the reply. Neither is ideal, since neither is a guarenteed part of the monitor interface, but it is all we have to go on, and ensure other critical errors will still be treated as fatal by libvirt.

> My current implementation assumes that if you don't have a QEMU with > this capability that we should fail the device removal. This is a > strong statement around hotplug that isn't consistent with previous > releases so I'm open to other approachs, but given the potential data > leakage problem hot-remove can lead to without drive_unplug, I think > it's the right thing to do. I don't think we can do this, since it obviously breaks every single existing deployment out there. Users who have sVirt enabled will have a level of protection from the data leakage, so I don't think it is a severe problem.

> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c > index abd8e9d..c7f4746 100644 > --- a/src/qemu/qemu_driver.c > +++ b/src/qemu/qemu_driver.c > @@ -8646,6 +8646,7 @@ static int qemudDomainDetachPciDiskDevice(struct qemud_driver *driver, > virDomainDiskDefPtr detach = NULL; > qemuDomainObjPrivatePtr priv = vm->privateData; > virCgroupPtr cgroup = NULL; > + char drivestr[PATH_MAX]; > > i = qemudFindDisk(vm->def, dev->data.disk->dst); > > @@ -8673,13 +8674,34 @@ static int qemudDomainDetachPciDiskDevice(struct qemud_driver *driver, > goto cleanup; > } > > + /* build the actual drive id string as the disk->info.alias doesn't > + * contain the QEMU_DRIVE_HOST_PREFIX that is passed to qemu */ > + if ((ret = snprintf(drivestr, sizeof(drivestr), "%s%s", > + QEMU_DRIVE_HOST_PREFIX, > + detach->info.alias)) > + < 0 || ret >= sizeof(drivestr)) { > + virReportOOMError(); > + goto cleanup; > + } > + > qemuDomainObjEnterMonitorWithDriver(driver, vm); > if (qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE) { > + ret = qemuMonitorDriveUnplug(priv->mon, drivestr); > + DEBUG("DriveUnplug ret=%d", ret); > + if (ret != 0) { > + qemuDomainObjExitMonitor(vm); > + goto cleanup; > + } > if (qemuMonitorDelDevice(priv->mon, detach->info.alias) < 0) { > qemuDomainObjExitMonitor(vm); > goto cleanup; > } > } else { > + ret = qemuMonitorDriveUnplug(priv->mon, drivestr); > + if (ret != 0) { > + qemuDomainObjExitMonitor(vm); > + goto cleanup; > + } > if (qemuMonitorRemovePCIDevice(priv->mon, > &detach->info.addr.pci) < 0) { > qemuDomainObjExitMonitor(vm); > @@ -8723,6 +8745,7 @@ static int qemudDomainDetachSCSIDiskDevice(struct qemud_driver *driver, > virDomainDiskDefPtr detach = NULL; > qemuDomainObjPrivatePtr priv = vm->privateData; > virCgroupPtr cgroup = NULL; > + char drivestr[PATH_MAX]; > > i = qemudFindDisk(vm->def, dev->data.disk->dst); > > @@ -8749,7 +8772,21 @@ static int qemudDomainDetachSCSIDiskDevice(struct qemud_driver *driver, > } > } > > + /* build the actual drive id string as the disk->info.alias doesn't > + * contain the QEMU_DRIVE_HOST_PREFIX that is passed to qemu */ > + if ((ret = snprintf(drivestr, sizeof(drivestr), "%s%s", > + QEMU_DRIVE_HOST_PREFIX, > + detach->info.alias)) > + < 0 || ret >= sizeof(drivestr)) { > + virReportOOMError(); > + goto cleanup; > + } > + > qemuDomainObjEnterMonitorWithDriver(driver, vm); > + if (qemuMonitorDriveUnplug(priv->mon, drivestr) < 0) { > + qemuDomainObjExitMonitor(vm); > + goto cleanup; > + } > if (qemuMonitorDelDevice(priv->mon, detach->info.alias) < 0) { > qemuDomainObjExitMonitor(vm); > goto cleanup; > diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c > index 2366fdb..285381d 100644 > --- a/src/qemu/qemu_monitor.c > +++ b/src/qemu/qemu_monitor.c > @@ -1781,6 +1781,25 @@ int qemuMonitorGetAllPCIAddresses(qemuMonitorPtr mon, > return ret; > } > > +int qemuMonitorDriveUnplug(qemuMonitorPtr mon, > + const char *drivestr) > +{ > + DEBUG("mon=%p drivestr=%s", mon, drivestr); > + int ret; > + > + if (!mon) { > + qemuReportError(VIR_ERR_INVALID_ARG, "%s", > + _("monitor must not be NULL")); > + return -1; > + } > + > + if (mon->json) > + ret = qemuMonitorJSONDriveUnplug(mon, drivestr); > + else > + ret = qemuMonitorTextDriveUnplug(mon, drivestr); > + return ret; > +} > + > int qemuMonitorDelDevice(qemuMonitorPtr mon, > const char *devalias) > { > diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h > index 48f4c20..bfe3641 100644 > --- a/src/qemu/qemu_monitor.h > +++ b/src/qemu/qemu_monitor.h > @@ -381,6 +381,9 @@ int qemuMonitorDelDevice(qemuMonitorPtr mon, > int qemuMonitorAddDrive(qemuMonitorPtr mon, > const char *drivestr); > > +int qemuMonitorDriveUnplug(qemuMonitorPtr mon, > + const char *drivestr); > + > int qemuMonitorSetDrivePassphrase(qemuMonitorPtr mon, > const char *alias, > const char *passphrase); > diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c > index d3ab25f..e99adac 100644 > --- a/src/qemu/qemu_monitor_json.c > +++ b/src/qemu/qemu_monitor_json.c > @@ -2243,6 +2243,30 @@ int qemuMonitorJSONAddDrive(qemuMonitorPtr mon, > } > > > +int qemuMonitorJSONDriveUnplug(qemuMonitorPtr mon, > + const char *drivestr) > +{ > + int ret; > + virJSONValuePtr cmd; > + virJSONValuePtr reply = NULL; > + > + DEBUG("JSONDriveUnplug drivestr=%s", drivestr); > + cmd = qemuMonitorJSONMakeCommand("drive_unplug", > + "s:id", drivestr, > + NULL); > + if (!cmd) > + return -1; > + > + ret = qemuMonitorJSONCommand(mon, cmd, &reply); > + > + if (ret == 0) > + ret = qemuMonitorJSONCheckError(cmd, reply); > + > + virJSONValueFree(cmd); > + virJSONValueFree(reply); > + return ret; > +} > + > int qemuMonitorJSONSetDrivePassphrase(qemuMonitorPtr mon, > const char *alias, > const char *passphrase) > diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h > index 94806c1..6a8692e 100644 > --- a/src/qemu/qemu_monitor_json.h > +++ b/src/qemu/qemu_monitor_json.h > @@ -188,6 +188,9 @@ int qemuMonitorJSONDelDevice(qemuMonitorPtr mon, > int qemuMonitorJSONAddDrive(qemuMonitorPtr mon, > const char *drivestr); > > +int qemuMonitorJSONDriveUnplug(qemuMonitorPtr mon, > + const char *drivestr); > + > int qemuMonitorJSONSetDrivePassphrase(qemuMonitorPtr mon, > const char *alias, > const char *passphrase); > diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c > index 69971a6..ded3078 100644 > --- a/src/qemu/qemu_monitor_text.c > +++ b/src/qemu/qemu_monitor_text.c > @@ -2380,6 +2380,45 @@ cleanup: > return ret; > } > > +int qemuMonitorTextDriveUnplug(qemuMonitorPtr mon, > + const char *drivestr) > +{ > + char *cmd = NULL; > + char *reply = NULL; > + char *safedev; > + int ret = -1; > + DEBUG("TextDriveUnplug drivestr=%s", drivestr); > + > + if (!(safedev = qemuMonitorEscapeArg(drivestr))) { > + virReportOOMError(); > + goto cleanup; > + } > + > + if (virAsprintf(&cmd, "drive_unplug %s", safedev) < 0) { > + virReportOOMError(); > + goto cleanup; > + } > + > + if (qemuMonitorCommand(mon, cmd, &reply) < 0) { > + qemuReportError(VIR_ERR_OPERATION_FAILED, > + _("cannot unplug %s drive"), drivestr); > + goto cleanup; > + } > + > + if (STRNEQ(reply, "")) { > + qemuReportError(VIR_ERR_OPERATION_FAILED, > + _("unplugging %s drive failed: %s"), drivestr, reply); > + goto cleanup; > + } > + > + ret = 0; > + > +cleanup: > + VIR_FREE(cmd); > + VIR_FREE(reply); > + VIR_FREE(safedev); > + return ret; > +} > > int qemuMonitorTextSetDrivePassphrase(qemuMonitorPtr mon, > const char *alias, > diff --git a/src/qemu/qemu_monitor_text.h b/src/qemu/qemu_monitor_text.h > index c017509..8355ce8 100644 > --- a/src/qemu/qemu_monitor_text.h > +++ b/src/qemu/qemu_monitor_text.h > @@ -186,6 +186,9 @@ int qemuMonitorTextDelDevice(qemuMonitorPtr mon, > int qemuMonitorTextAddDrive(qemuMonitorPtr mon, > const char *drivestr); > > +int qemuMonitorTextDriveUnplug(qemuMonitorPtr mon, > + const char *drivestr); > + > int qemuMonitorTextSetDrivePassphrase(qemuMonitorPtr mon, > const char *alias, > const char *passphrase); The patch all looks good to me, apart from issues around handling the command-not-found case as non-fatal. Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|