[libvirt] [PATCH] qemu: call drive_unplug in DetachPciDiskDevice
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). 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. Signed-off-by: Ryan Harper <ryanh@us.ibm.com> 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); -- Ryan Harper Software Engineer; Linux Technology Center IBM Corp., Austin, Tx ryanh@us.ibm.com
* Ryan Harper <ryanh@us.ibm.com> [2010-10-21 09:01]:
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).
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.
Signed-off-by: Ryan Harper <ryanh@us.ibm.com>
1. http://lists.gnu.org/archive/html/qemu-devel/2010-10/msg01327.html
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);
-- Ryan Harper Software Engineer; Linux Technology Center IBM Corp., Austin, Tx ryanh@us.ibm.com
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- Ryan Harper Software Engineer; Linux Technology Center IBM Corp., Austin, Tx ryanh@us.ibm.com
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 :|
On 10/21/2010 11:45 AM, Daniel P. Berrange wrote:
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.
I think that's reasonable but if sVirt is disabled, libvirt at least should log something somewhere to indicate that something may be wrong. Regards, Anthony Liguori
On Thu, Oct 21, 2010 at 11:48:33AM -0500, Anthony Liguori wrote:
On 10/21/2010 11:45 AM, Daniel P. Berrange wrote:
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.
I think that's reasonable but if sVirt is disabled, libvirt at least should log something somewhere to indicate that something may be wrong.
We can syslog a warning anytime we try to run drive_unplug and it is not present. 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 :|
* Daniel P. Berrange <berrange@redhat.com> [2010-10-21 11:56]:
On Thu, Oct 21, 2010 at 11:48:33AM -0500, Anthony Liguori wrote:
On 10/21/2010 11:45 AM, Daniel P. Berrange wrote:
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.
I think that's reasonable but if sVirt is disabled, libvirt at least should log something somewhere to indicate that something may be wrong.
We can syslog a warning anytime we try to run drive_unplug and it is not present.
Let me know your preference here and I'll respin the patch.
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 :|
-- Ryan Harper Software Engineer; Linux Technology Center IBM Corp., Austin, Tx ryanh@us.ibm.com
* Daniel P. Berrange <berrange@redhat.com> [2010-10-21 11:46]:
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.
Yeah, this sorta already works without explictly catching the error (unless you want to display a different message at caller level than command not found).
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.
Yeah, I was thinking we could re-work the logic to only completely fail in the case where the command wasn't found/successful *and* we don't have sVirt enabled.
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 :|
-- Ryan Harper Software Engineer; Linux Technology Center IBM Corp., Austin, Tx ryanh@us.ibm.com
On Thu, Oct 21, 2010 at 11:53:29AM -0500, Ryan Harper wrote:
* Daniel P. Berrange <berrange@redhat.com> [2010-10-21 11:46]:
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.
Yeah, this sorta already works without explictly catching the error (unless you want to display a different message at caller level than command not found).
This would cause the hot-unplug operation to completely terminate though, whereas we want it to carry on, if the command is not present. If we get a command-not-found, then we should syslog a warning, and then return '0' (ie success) to the caller of qemuMonitorDriveUnplug Alternatively, return '+1' to the caller, and make the callers check for '+1', syslog it and treat it as non-fatal. This is closer to the approach we use for handling 'balloon device not present' error when code runs the 'info balloon' method.
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.
Yeah, I was thinking we could re-work the logic to only completely fail in the case where the command wasn't found/successful *and* we don't have sVirt enabled.
I'm still wary of doing that because of the significant negative impact on existing deployments out there, even in the case where hotunplug would complete just fine. I think we'll just need to syslog the potential issue and people can upgrade their QMEU if they need the extra security 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 :|
* Daniel P. Berrange <berrange@redhat.com> [2010-10-21 12:10]:
On Thu, Oct 21, 2010 at 11:53:29AM -0500, Ryan Harper wrote:
* Daniel P. Berrange <berrange@redhat.com> [2010-10-21 11:46]:
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.
Yeah, this sorta already works without explictly catching the error (unless you want to display a different message at caller level than command not found).
This would cause the hot-unplug operation to completely terminate though, whereas we want it to carry on, if the command is not present. If we get a command-not-found, then we should syslog a warning, and then return '0' (ie success) to the caller of qemuMonitorDriveUnplug
Alternatively, return '+1' to the caller, and make the callers check for '+1', syslog it and treat it as non-fatal. This is closer to the approach we use for handling 'balloon device not present' error when code runs the 'info balloon' method.
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.
Yeah, I was thinking we could re-work the logic to only completely fail in the case where the command wasn't found/successful *and* we don't have sVirt enabled.
I'm still wary of doing that because of the significant negative impact on existing deployments out there, even in the case where hotunplug would complete just fine. I think we'll just need to syslog the potential issue and people can upgrade their QMEU if they need the extra security
OK, I'll respin.
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 :|
-- Ryan Harper Software Engineer; Linux Technology Center IBM Corp., Austin, Tx ryanh@us.ibm.com
* Daniel P. Berrange <berrange@redhat.com> [2010-10-21 12:10]:
On Thu, Oct 21, 2010 at 11:53:29AM -0500, Ryan Harper wrote:
* Daniel P. Berrange <berrange@redhat.com> [2010-10-21 11:46]:
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.
Yeah, this sorta already works without explictly catching the error (unless you want to display a different message at caller level than command not found).
This would cause the hot-unplug operation to completely terminate though, whereas we want it to carry on, if the command is not present. If we get a command-not-found, then we should syslog a warning, and then return '0' (ie success) to the caller of qemuMonitorDriveUnplug
Alternatively, return '+1' to the caller, and make the callers check for '+1', syslog it and treat it as non-fatal. This is closer to the approach we use for handling 'balloon device not present' error when code runs the 'info balloon' method.
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.
Yeah, I was thinking we could re-work the logic to only completely fail in the case where the command wasn't found/successful *and* we don't have sVirt enabled.
I'm still wary of doing that because of the significant negative impact on existing deployments out there, even in the case where hotunplug would complete just fine. I think we'll just need to syslog the potential issue and people can upgrade their QMEU if they need the extra security
Thinking on this again; if we don't have a confirmation that the guest has removed the pci device (independent of whether we've revoked access to the underlying block device); then I really think we should fail the device removal; at the very least we cannot update libvirt state to indicate that the slot is free for re-assignment. -- Ryan Harper Software Engineer; Linux Technology Center IBM Corp., Austin, Tx ryanh@us.ibm.com
On 10/21/2010 12:43 PM, Ryan Harper wrote:
Thinking on this again; if we don't have a confirmation that the guest has removed the pci device (independent of whether we've revoked access to the underlying block device); then I really think we should fail the device removal; at the very least we cannot update libvirt state to indicate that the slot is free for re-assignment.
For the time being, I think we have to take the approach Dan suggested, and then revisit the entire API. The impact here is actually minimal because the chances of unplug failing when plug has succeeded is very low. If it can fail, it changes the nature of the API in a fundamental way that probably requires a deep thought about what the API should look like in the future. Regards, Anthony Liguori
* Ryan Harper <ryanh@us.ibm.com> [2010-10-21 12:43]:
* Daniel P. Berrange <berrange@redhat.com> [2010-10-21 12:10]:
On Thu, Oct 21, 2010 at 11:53:29AM -0500, Ryan Harper wrote:
* Daniel P. Berrange <berrange@redhat.com> [2010-10-21 11:46]:
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.
Yeah, this sorta already works without explictly catching the error (unless you want to display a different message at caller level than command not found).
This would cause the hot-unplug operation to completely terminate though, whereas we want it to carry on, if the command is not present. If we get a command-not-found, then we should syslog a warning, and then return '0' (ie success) to the caller of qemuMonitorDriveUnplug
Alternatively, return '+1' to the caller, and make the callers check for '+1', syslog it and treat it as non-fatal. This is closer to the approach we use for handling 'balloon device not present' error when code runs the 'info balloon' method.
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.
Yeah, I was thinking we could re-work the logic to only completely fail in the case where the command wasn't found/successful *and* we don't have sVirt enabled.
I'm still wary of doing that because of the significant negative impact on existing deployments out there, even in the case where hotunplug would complete just fine. I think we'll just need to syslog the potential issue and people can upgrade their QMEU if they need the extra security
Thinking on this again; if we don't have a confirmation that the guest has removed the pci device (independent of whether we've revoked access to the underlying block device); then I really think we should fail the device removal; at the very least we cannot update libvirt state to indicate that the slot is free for re-assignment.
This is why I wanted to do a failure of the PciDiskRemove() as it address this though it does block users in the case where the guest may respond and running on a qemu without unplug support. I wonder if we've unplugged the disk, the guest hasn't responded, but we update the XML that it isn't present; we should fail subsequent adds to the same slot in the guest in which case, we'd never find the case where we'd be out-of-sync with the guest w.r.t different disks in the same slot. -- Ryan Harper Software Engineer; Linux Technology Center IBM Corp., Austin, Tx ryanh@us.ibm.com
On Thu, Oct 21, 2010 at 12:43:14PM -0500, Ryan Harper wrote:
* Daniel P. Berrange <berrange@redhat.com> [2010-10-21 12:10]:
On Thu, Oct 21, 2010 at 11:53:29AM -0500, Ryan Harper wrote:
* Daniel P. Berrange <berrange@redhat.com> [2010-10-21 11:46]:
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.
Yeah, this sorta already works without explictly catching the error (unless you want to display a different message at caller level than command not found).
This would cause the hot-unplug operation to completely terminate though, whereas we want it to carry on, if the command is not present. If we get a command-not-found, then we should syslog a warning, and then return '0' (ie success) to the caller of qemuMonitorDriveUnplug
Alternatively, return '+1' to the caller, and make the callers check for '+1', syslog it and treat it as non-fatal. This is closer to the approach we use for handling 'balloon device not present' error when code runs the 'info balloon' method.
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.
Yeah, I was thinking we could re-work the logic to only completely fail in the case where the command wasn't found/successful *and* we don't have sVirt enabled.
I'm still wary of doing that because of the significant negative impact on existing deployments out there, even in the case where hotunplug would complete just fine. I think we'll just need to syslog the potential issue and people can upgrade their QMEU if they need the extra security
Thinking on this again; if we don't have a confirmation that the guest has removed the pci device (independent of whether we've revoked access to the underlying block device); then I really think we should fail the device removal; at the very least we cannot update libvirt state to indicate that the slot is free for re-assignment.
The trouble with this is that we'd be breaking 100% of hotunplug cases for old QEMU, even though only 1% of cases actually exhibit any problem in the real world. Accidentally trying to re-use a slot that isn't free is a fairly minor issue. I agree we need to deal with this properly long term, but I don't want to gratuitously break existing deployments yet. Once we have a plan for supporting this properly across libvirt+qemu, then we can re-evaluate whether we should be stricter for old qemu. I think we should focus the first patches on the task of supporting the new drive unplug command which has clear immediate benefit for everyone, and consider the slot re-use problem in future dev work. 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 :|
* Daniel P. Berrange <berrange@redhat.com> [2010-10-21 12:54]:
On Thu, Oct 21, 2010 at 12:43:14PM -0500, Ryan Harper wrote:
* Daniel P. Berrange <berrange@redhat.com> [2010-10-21 12:10]:
On Thu, Oct 21, 2010 at 11:53:29AM -0500, Ryan Harper wrote:
* Daniel P. Berrange <berrange@redhat.com> [2010-10-21 11:46]:
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.
Yeah, this sorta already works without explictly catching the error (unless you want to display a different message at caller level than command not found).
This would cause the hot-unplug operation to completely terminate though, whereas we want it to carry on, if the command is not present. If we get a command-not-found, then we should syslog a warning, and then return '0' (ie success) to the caller of qemuMonitorDriveUnplug
Alternatively, return '+1' to the caller, and make the callers check for '+1', syslog it and treat it as non-fatal. This is closer to the approach we use for handling 'balloon device not present' error when code runs the 'info balloon' method.
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.
Yeah, I was thinking we could re-work the logic to only completely fail in the case where the command wasn't found/successful *and* we don't have sVirt enabled.
I'm still wary of doing that because of the significant negative impact on existing deployments out there, even in the case where hotunplug would complete just fine. I think we'll just need to syslog the potential issue and people can upgrade their QMEU if they need the extra security
Thinking on this again; if we don't have a confirmation that the guest has removed the pci device (independent of whether we've revoked access to the underlying block device); then I really think we should fail the device removal; at the very least we cannot update libvirt state to indicate that the slot is free for re-assignment.
The trouble with this is that we'd be breaking 100% of hotunplug cases for old QEMU, even though only 1% of cases actually exhibit any problem in the real world. Accidentally trying to re-use a slot that isn't free is a fairly minor issue. I agree we need to deal with this properly long term, but I don't want to gratuitously break existing deployments yet. Once we have a plan for supporting this properly across libvirt+qemu, then we can re-evaluate whether we should be stricter for old qemu. I think we should focus the first patches on the task of supporting the new drive unplug command which has clear immediate benefit for everyone, and consider the slot re-use problem in future dev work.
Yep, sounds like a plan. -- Ryan Harper Software Engineer; Linux Technology Center IBM Corp., Austin, Tx ryanh@us.ibm.com
participants (3)
-
Anthony Liguori -
Daniel P. Berrange -
Ryan Harper