qemu: call drive_del 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_del() command and introduces it
in the disk removal paths.  If the guest is running in a QEMU without this
command we currently explicitly check for unknown command/CommandNotFound
and log the issue.

If QEMU supports the command we issue the drive_del command after we attempt
to remove the device.  The guest may respond and remove the block device
before we get to attempt to call drive_del.  In that case, we explicitly check
for 'Device not found' from the monitor indicating that the target drive
was auto-deleted upon guest responds to the device removal notification.

1. http://thread.gmane.org/gmane.comp.emulators.qemu/84745

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

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 75f4563b639f272aa7725adc07c534ce2d25405d..4e65f3b399d44ef903b89a716d50d0e5893ac6d3 100644 (file)
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -9033,7 +9033,7 @@ static int qemudDomainDetachPciDiskDevice(struct qemud_driver *driver,
     virDomainDiskDefPtr detach = NULL;
     qemuDomainObjPrivatePtr priv = vm->privateData;
     virCgroupPtr cgroup = NULL;
-    char drivestr[PATH_MAX];
+    char *drivestr = NULL;
 
     i = qemudFindDisk(vm->def, dev->data.disk->dst);
 
@@ -9063,40 +9063,29 @@ static int qemudDomainDetachPciDiskDevice(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)) {
+    if (virAsprintf(&drivestr, "%s%s",
+                    QEMU_DRIVE_HOST_PREFIX, detach->info.alias) < 0) {
         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);
             goto cleanup;
         }
     }
+
+    /* disconnect guest from host device */
+    qemuMonitorDriveDel(priv->mon, drivestr);
+
     qemuDomainObjExitMonitorWithDriver(driver, vm);
 
     qemuDomainDiskAudit(vm, detach, NULL, "detach", ret >= 0);
@@ -9124,6 +9113,7 @@ static int qemudDomainDetachPciDiskDevice(struct qemud_driver *driver,
     ret = 0;
 
 cleanup:
+    VIR_FREE(drivestr);
     return ret;
 }
 
@@ -9136,7 +9126,7 @@ static int qemudDomainDetachSCSIDiskDevice(struct qemud_driver *driver,
     virDomainDiskDefPtr detach = NULL;
     qemuDomainObjPrivatePtr priv = vm->privateData;
     virCgroupPtr cgroup = NULL;
-    char drivestr[PATH_MAX];
+    char *drivestr = NULL;
 
     i = qemudFindDisk(vm->def, dev->data.disk->dst);
 
@@ -9165,24 +9155,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)) {
+    if (virAsprintf(&drivestr, "%s%s",
+                    QEMU_DRIVE_HOST_PREFIX, detach->info.alias) < 0) {
         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;
     }
+
+    /* disconnect guest from host device */
+    qemuMonitorDriveDel(priv->mon, drivestr);
+
     qemuDomainObjExitMonitorWithDriver(driver, vm);
 
     qemuDomainDiskAudit(vm, detach, NULL, "detach", ret >= 0);
@@ -9206,6 +9193,7 @@ static int qemudDomainDetachSCSIDiskDevice(struct qemud_driver *driver,
     ret = 0;
 
 cleanup:
+    VIR_FREE(drivestr);
     virCgroupFree(&cgroup);
     return ret;
 }

diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index 8330b9cf22fd79399fb00dc02c74358960f0508b..6ad894d6fd78abcb85df2c7b2367c8a7267362cd 100644 (file)
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -1774,8 +1774,8 @@ int qemuMonitorGetAllPCIAddresses(qemuMonitorPtr mon,
     return ret;
 }
 
-int qemuMonitorDriveUnplug(qemuMonitorPtr mon,
-                         const char *drivestr)
+int qemuMonitorDriveDel(qemuMonitorPtr mon,
+                        const char *drivestr)
 {
     DEBUG("mon=%p drivestr=%s", mon, drivestr);
     int ret;
@@ -1787,9 +1787,9 @@ int qemuMonitorDriveUnplug(qemuMonitorPtr mon,
     }
 
     if (mon->json)
-        ret = qemuMonitorJSONDriveUnplug(mon, drivestr);
+        ret = qemuMonitorJSONDriveDel(mon, drivestr);
     else
-        ret = qemuMonitorTextDriveUnplug(mon, drivestr);
+        ret = qemuMonitorTextDriveDel(mon, drivestr);
     return ret;
 }
 

diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
index cf5b402eec8296f650a54a108e5318105cb10efa..4d7aaf7e03997e2ea6eea58a3c751a91b7ac1716 100644 (file)
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -379,7 +379,7 @@ int qemuMonitorDelDevice(qemuMonitorPtr mon,
 int qemuMonitorAddDrive(qemuMonitorPtr mon,
                         const char *drivestr);
 
-int qemuMonitorDriveUnplug(qemuMonitorPtr mon,
+int qemuMonitorDriveDel(qemuMonitorPtr mon,
                         const char *drivestr);
 
 int qemuMonitorSetDrivePassphrase(qemuMonitorPtr mon,

diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index bcf6377f45ffc44369fc368d4de0b3265f75ebb6..409f6bd25b94db72c994b3f963d6e6acf7fe002d 100644 (file)
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -2239,15 +2239,15 @@ int qemuMonitorJSONAddDrive(qemuMonitorPtr mon,
 }
 
 
-int qemuMonitorJSONDriveUnplug(qemuMonitorPtr mon,
-                             const char *drivestr)
+int qemuMonitorJSONDriveDel(qemuMonitorPtr mon,
+                            const char *drivestr)
 {
     int ret;
     virJSONValuePtr cmd;
     virJSONValuePtr reply = NULL;
 
-    DEBUG("JSONDriveUnplug drivestr=%s", drivestr);
-    cmd = qemuMonitorJSONMakeCommand("drive_unplug",
+    DEBUG("JSONDriveDel drivestr=%s", drivestr);
+    cmd = qemuMonitorJSONMakeCommand("drive_del",
                                      "s:id", drivestr,
                                      NULL);
     if (!cmd)
@@ -2256,14 +2256,19 @@ int qemuMonitorJSONDriveUnplug(qemuMonitorPtr mon,
     ret = qemuMonitorJSONCommand(mon, cmd, &reply);
 
     if (ret == 0) {
-        /* See if drive_unplug isn't supported */
+        /* See if drive_del isn't supported */
         if (qemuMonitorJSONHasError(reply, "CommandNotFound")) {
-            VIR_ERROR0(_("unplugging disk is not supported.  "
+            VIR_ERROR0(_("deleting disk is not supported.  "
                         "This may leak data if disk is reassigned"));
             ret = 1;
             goto cleanup;
+        } else if (qemuMonitorJSONHasError(reply, "DeviceNotFound")) {
+            /* NB: device not found errors mean the drive was
+             * auto-deleted and we ignore the error */
+            ret = 0;
+        } else {
+            ret = qemuMonitorJSONCheckError(cmd, reply);
         }
-        ret = qemuMonitorJSONCheckError(cmd, reply);
     }
 
 cleanup:

diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h
index 4882d4e443fc777b12aa890a406672b5263a464e..8d9614632179c68eb8022009b94f4fdcb7bbfb26 100644 (file)
--- a/src/qemu/qemu_monitor_json.h
+++ b/src/qemu/qemu_monitor_json.h
@@ -189,7 +189,7 @@ int qemuMonitorJSONDelDevice(qemuMonitorPtr mon,
 int qemuMonitorJSONAddDrive(qemuMonitorPtr mon,
                             const char *drivestr);
 
-int qemuMonitorJSONDriveUnplug(qemuMonitorPtr mon,
+int qemuMonitorJSONDriveDel(qemuMonitorPtr mon,
                             const char *drivestr);
 
 int qemuMonitorJSONSetDrivePassphrase(qemuMonitorPtr mon,

diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c
index 1a8d0ec529a16b6d6f65523998535d1ccf8075ca..29e21ce2a77fbb31fd65e0ae7927b58308adcd87 100644 (file)
--- a/src/qemu/qemu_monitor_text.c
+++ b/src/qemu/qemu_monitor_text.c
@@ -2286,6 +2286,7 @@ int qemuMonitorTextDelDevice(qemuMonitorPtr mon,
         goto cleanup;
     }
 
+    DEBUG("TextDelDevice devalias=%s", devalias);
     if (qemuMonitorCommand(mon, cmd, &reply) < 0) {
         qemuReportError(VIR_ERR_OPERATION_FAILED,
                         _("cannot detach %s device"), devalias);
@@ -2392,41 +2393,48 @@ 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)
+/* Attempts to remove a host drive.
+ * Returns 1 if unsupported, 0 if ok, and -1 on other failure */
+int qemuMonitorTextDriveDel(qemuMonitorPtr mon,
+                            const char *drivestr)
 {
     char *cmd = NULL;
     char *reply = NULL;
     char *safedev;
     int ret = -1;
-    DEBUG("TextDriveUnplug drivestr=%s", drivestr);
+    DEBUG("TextDriveDel drivestr=%s", drivestr);
 
     if (!(safedev = qemuMonitorEscapeArg(drivestr))) {
         virReportOOMError();
         goto cleanup;
     }
 
-    if (virAsprintf(&cmd, "drive_unplug %s", safedev) < 0) {
+    if (virAsprintf(&cmd, "drive_del %s", safedev) < 0) {
         virReportOOMError();
         goto cleanup;
     }
 
     if (qemuMonitorCommand(mon, cmd, &reply) < 0) {
         qemuReportError(VIR_ERR_OPERATION_FAILED,
-                        _("cannot unplug %s drive"), drivestr);
+                        _("cannot delete %s drive"), drivestr);
         goto cleanup;
     }
 
     if (strstr(reply, "unknown command:")) {
-        VIR_ERROR0(_("unplugging disk is not supported.  "
+        VIR_ERROR0(_("deleting drive is not supported.  "
                     "This may leak data if disk is reassigned"));
         ret = 1;
         goto cleanup;
+
+    /* (qemu) drive_del wark
+     * Device 'wark' not found */
+    } else if (STRPREFIX(reply, "Device '") && (strstr(reply, "not found"))) {
+        /* NB: device not found errors mean the drive was auto-deleted and we
+         * ignore the error */
+        ret = 0;
     } else if (STRNEQ(reply, "")) {
         qemuReportError(VIR_ERR_OPERATION_FAILED,
-                        _("unplugging %s drive failed: %s"), drivestr, reply);
+                        _("deleting %s drive failed: %s"), drivestr, reply);
         goto cleanup;
     }
 

diff --git a/src/qemu/qemu_monitor_text.h b/src/qemu/qemu_monitor_text.h
index 6ef858c13aecfeb73f999cc483c77e31c9dce258..57d6e9b24a57ff6a872e47d35ea1786342f83033 100644 (file)
--- a/src/qemu/qemu_monitor_text.h
+++ b/src/qemu/qemu_monitor_text.h
@@ -187,7 +187,7 @@ int qemuMonitorTextDelDevice(qemuMonitorPtr mon,
 int qemuMonitorTextAddDrive(qemuMonitorPtr mon,
                              const char *drivestr);
 
-int qemuMonitorTextDriveUnplug(qemuMonitorPtr mon,
+int qemuMonitorTextDriveDel(qemuMonitorPtr mon,
                              const char *drivestr);
 
 int qemuMonitorTextSetDrivePassphrase(qemuMonitorPtr mon,