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).

Changes since v2:
 - use VIR_ERROR to report when unplug command not found
Changes since v1:
 - return > 0 when command isn't present, < 0 on command failure
 - detect when drive_unplug command isn't present and log error
   instead of failing entire command

Signed-off-by: Ryan Harper <ryanh@us.ibm.com>

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index b640d23312814d03cf8968768e0ba53e9a2d6b3e..75f4563b639f272aa7725adc07c534ce2d25405d 100644 (file)
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -9033,6 +9033,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);
 
@@ -9060,13 +9061,36 @@ 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);
+        /* ret > 0 indicates unplug isn't supported, issue will be logged */
+        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);
+        /* ret > 0 indicates unplug isn't supported, issue will be logged */
+        if (ret < 0) {
+            qemuDomainObjExitMonitor(vm);
+            goto cleanup;
+        }
         if (qemuMonitorRemovePCIDevice(priv->mon,
                                        &detach->info.addr.pci) < 0) {
             qemuDomainObjExitMonitor(vm);
@@ -9112,6 +9136,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);
 
@@ -9138,7 +9163,22 @@ 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);
+    /* ret > 0 indicates unplug isn't supported, issue will be logged */
+    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 85d0d0fb8c53fa8ca96c747649110e1775b1838f..8330b9cf22fd79399fb00dc02c74358960f0508b 100644 (file)
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -1774,6 +1774,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 41b3135d3e04358eba5e9efc0a5d5f25cacf1a2a..cf5b402eec8296f650a54a108e5318105cb10efa 100644 (file)
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -379,6 +379,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 ec6b720fa0339448331b237c9b66c367d88e69d9..bcf6377f45ffc44369fc368d4de0b3265f75ebb6 100644 (file)
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -2239,6 +2239,39 @@ 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) {
+        /* See if drive_unplug isn't supported */
+        if (qemuMonitorJSONHasError(reply, "CommandNotFound")) {
+            VIR_ERROR0(_("unplugging disk is not supported.  "
+                        "This may leak data if disk is reassigned"));
+            ret = 1;
+            goto cleanup;
+        }
+        ret = qemuMonitorJSONCheckError(cmd, reply);
+    }
+
+cleanup:
+    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 c78ee24573af426857513892670e9a372bebb034..4882d4e443fc777b12aa890a406672b5263a464e 100644 (file)
--- a/src/qemu/qemu_monitor_json.h
+++ b/src/qemu/qemu_monitor_json.h
@@ -189,6 +189,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 64ec57bdcde8367c1e6dc8924b3b7a98d80869cc..1a8d0ec529a16b6d6f65523998535d1ccf8075ca 100644 (file)
--- a/src/qemu/qemu_monitor_text.c
+++ b/src/qemu/qemu_monitor_text.c
@@ -2392,6 +2392,52 @@ cleanup:
     return ret;
 }
 
+/* Attempts to unplug a drive.  Returns 1 if unsupported, 0 if ok, and -1 on
+ * other failure */
+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 (strstr(reply, "unknown command:")) {
+        VIR_ERROR0(_("unplugging disk is not supported.  "
+                    "This may leak data if disk is reassigned"));
+        ret = 1;
+        goto cleanup;
+    } else 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 983f402eda133e6b03a9dea0018915e03279fb32..6ef858c13aecfeb73f999cc483c77e31c9dce258 100644 (file)
--- a/src/qemu/qemu_monitor_text.h
+++ b/src/qemu/qemu_monitor_text.h
@@ -187,6 +187,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);