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: NRyan Harper <ryanh@us.ibm.com>

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;

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

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

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)

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

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,

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