Blob Blame History Raw
From 2d1d83a1d3eeb7cf98972c461842f333f900c1e2 Mon Sep 17 00:00:00 2001
Message-Id: <2d1d83a1d3eeb7cf98972c461842f333f900c1e2@dist-git>
From: Jiri Denemark <jdenemar@redhat.com>
Date: Tue, 5 Apr 2016 09:14:09 +0200
Subject: [PATCH] RHEL: Support virtio disk hotplug in JSON mode

RHEL only, no upstream

For bug
  https://bugzilla.redhat.com/show_bug.cgi?id=1026966
  https://bugzilla.redhat.com/show_bug.cgi?id=573946

The existing drive_add command can hotplug SCSI and VirtIO
disks, but this isn't ported to JSON mode. RHEL6 introduces
a custom __com.redhat_drive_add that only supports VirtIO
disks. Switch the VirtIO hotplug to this command, but leave
the SCSI hotplug using old command so SCSI gets an explicit
error about being unsupported.

* src/libvirt_private.syms: Export virJSONValueObjectRemoveKey
* src/util/json.c, src/util/json.h: Add virJSONValueObjectRemoveKey
  to allow a key to be deleted from an object
* src/qemu/qemu_monitor_json.c: Try __com.redhat_drive_add first and use
  drive_add only if the redhat command is not known to qemu.

Also includes the following fix:

https://bugzilla.redhat.com/show_bug.cgi?id=696596

Upstream added drive_del as a way to ensure that disks are fully
removed before returning control to libvirt.  But RHEL backported
it as __com.redhat_drive_del, prior to upstream adoption of a
QMP counterpart.  Because we weren't trying the RHEL-specific
spelling, we were falling back to the unsafe approach of just
removing the device and hoping for the best, which was racy and
could occasionally result in a rapid hot-plug cycle trying to
plug in a new disk that collides with the old disk not yet gone.

* src/qemu/qemu_monitor_json.c (qemuMonitorJSONDriveDel): Try
rhel-specific drive_del monitor command first.

(cherry picked from commit d1c200dfead14a590a4ddebe20a20ffe441d2b24 in
rhel-6.5 branch)

Signed-off-by: Jiri Denemark <jdenemar@redhat.com>

Conflicts:
	src/libvirt_private.syms - the change is already upstream
        src/util/virjson.[ch] - the change is already upstream
	src/qemu/qemu_monitor_json.c - context; upstream doesn't try to
	    call nonexistent drive_{add,del} QMP commands any more
---
 src/qemu/qemu_monitor.c      |  12 +++--
 src/qemu/qemu_monitor_json.c | 106 +++++++++++++++++++++++++++++++++++++++++++
 src/qemu/qemu_monitor_json.h |   6 +++
 tests/qemuhotplugtest.c      |  92 ++++++++++++++++++++++++++++++++++++-
 4 files changed, 210 insertions(+), 6 deletions(-)

diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index 8bc00bc1e..488d761fb 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -3073,8 +3073,10 @@ qemuMonitorDriveDel(qemuMonitorPtr mon,
 
     QEMU_CHECK_MONITOR(mon);
 
-    /* there won't be a direct replacement for drive_del in QMP */
-    return qemuMonitorTextDriveDel(mon, drivestr);
+    if (mon->json)
+        return qemuMonitorJSONDriveDel(mon, drivestr);
+    else
+        return qemuMonitorTextDriveDel(mon, drivestr);
 }
 
 
@@ -3195,8 +3197,10 @@ qemuMonitorAddDrive(qemuMonitorPtr mon,
 
     QEMU_CHECK_MONITOR(mon);
 
-    /* there won't ever be a direct QMP replacement for this function */
-    return qemuMonitorTextAddDrive(mon, drivestr);
+    if (mon->json)
+        return qemuMonitorJSONAddDrive(mon, drivestr);
+    else
+        return qemuMonitorTextAddDrive(mon, drivestr);
 }
 
 
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 2010657e3..6fcf57cd1 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -3807,6 +3807,112 @@ int qemuMonitorJSONDelObject(qemuMonitorPtr mon,
 }
 
 
+int qemuMonitorJSONAddDrive(qemuMonitorPtr mon,
+                            const char *drivestr)
+{
+    int ret = -1;
+    virJSONValuePtr cmd;
+    virJSONValuePtr reply = NULL;
+    virJSONValuePtr args;
+
+    cmd = qemuMonitorJSONMakeCommand("__com.redhat_drive_add",
+                                     NULL);
+    if (!cmd)
+        return -1;
+
+    args = qemuMonitorJSONKeywordStringToJSON(drivestr, "type");
+    if (!args)
+        goto cleanup;
+
+    /* __com.redhat_drive_add rejects the 'if' key */
+    virJSONValueObjectRemoveKey(args, "if", NULL);
+
+    if (virJSONValueObjectAppend(cmd, "arguments", args) < 0) {
+        virReportOOMError();
+        goto cleanup;
+    }
+    args = NULL; /* cmd owns reference to args now */
+
+    if ((ret = qemuMonitorJSONCommand(mon, cmd, &reply)) < 0)
+        goto cleanup;
+
+    if (qemuMonitorJSONHasError(reply, "CommandNotFound")) {
+        virJSONValueFree(cmd);
+        virJSONValueFree(reply);
+        cmd = reply = NULL;
+
+        VIR_DEBUG("__com.redhat_drive_add command not found,"
+                  " trying upstream way");
+    } else {
+        ret = qemuMonitorJSONCheckError(cmd, reply);
+        goto cleanup;
+    }
+
+    /* Upstream approach */
+    /* there won't be a direct replacement for drive_add in QMP */
+    ret = qemuMonitorTextAddDrive(mon, drivestr);
+
+ cleanup:
+    virJSONValueFree(args);
+    virJSONValueFree(cmd);
+    virJSONValueFree(reply);
+    return ret;
+}
+
+
+int qemuMonitorJSONDriveDel(qemuMonitorPtr mon,
+                            const char *drivestr)
+{
+    int ret;
+    virJSONValuePtr cmd;
+    virJSONValuePtr reply = NULL;
+
+    VIR_DEBUG("drivestr=%s", drivestr);
+    cmd = qemuMonitorJSONMakeCommand("__com.redhat_drive_del",
+                                     "s:id", drivestr,
+                                     NULL);
+    if (!cmd)
+        return -1;
+
+    if ((ret = qemuMonitorJSONCommand(mon, cmd, &reply)) < 0)
+        goto cleanup;
+
+    if (qemuMonitorJSONHasError(reply, "CommandNotFound")) {
+        virJSONValueFree(cmd);
+        virJSONValueFree(reply);
+        cmd = reply = NULL;
+
+        VIR_DEBUG("__com.redhat_drive_del command not found,"
+                  " trying upstream way");
+    } else if (qemuMonitorJSONHasError(reply, "DeviceNotFound")) {
+        /* NB: device not found errors mean the drive was
+         * auto-deleted and we ignore the error */
+        ret = 0;
+        goto cleanup;
+    } else {
+        ret = qemuMonitorJSONCheckError(cmd, reply);
+        goto cleanup;
+    }
+
+    /* Upstream approach */
+    /* there won't be a direct replacement for drive_del in QMP */
+    if ((ret = qemuMonitorTextDriveDel(mon, drivestr)) < 0) {
+        virErrorPtr err = virGetLastError();
+        if (err && err->code == VIR_ERR_OPERATION_UNSUPPORTED) {
+            VIR_ERROR("%s",
+                      _("deleting disk is not supported.  "
+                        "This may leak data if disk is reassigned"));
+            ret = 1;
+            virResetLastError();
+        }
+    }
+
+ 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 fb697033c..696aaeffc 100644
--- a/src/qemu/qemu_monitor_json.h
+++ b/src/qemu/qemu_monitor_json.h
@@ -233,6 +233,12 @@ int qemuMonitorJSONAddObject(qemuMonitorPtr mon,
 int qemuMonitorJSONDelObject(qemuMonitorPtr mon,
                              const char *objalias);
 
+int qemuMonitorJSONAddDrive(qemuMonitorPtr mon,
+                            const char *drivestr);
+
+int qemuMonitorJSONDriveDel(qemuMonitorPtr mon,
+                            const char *drivestr);
+
 int qemuMonitorJSONSetDrivePassphrase(qemuMonitorPtr mon,
                                       const char *alias,
                                       const char *passphrase);
diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c
index cdeb3f1bf..2e1afeea9 100644
--- a/tests/qemuhotplugtest.c
+++ b/tests/qemuhotplugtest.c
@@ -658,6 +658,14 @@ mymain(void)
     "    }"                                                 \
     "}\r\n"
 
+#define QMP_NOT_FOUND \
+    "{" \
+    "    \"error\": {" \
+    "        \"class\": \"CommandNotFound\"," \
+    "        \"desc\": \"The command has not been found\"" \
+    "    }" \
+    "}"
+
     DO_TEST_UPDATE("graphics-spice", "graphics-spice-nochange", false, false, NULL);
     DO_TEST_UPDATE("graphics-spice-timeout", "graphics-spice-timeout-nochange", false, false,
                    "set_password", QMP_OK, "expire_password", QMP_OK);
@@ -678,67 +686,135 @@ mymain(void)
                    "chardev-remove", QMP_OK);
 
     DO_TEST_ATTACH("base-live", "disk-virtio", false, true,
+                   "__com.redhat_drive_add", QMP_NOT_FOUND,
                    "human-monitor-command", HMP("OK\\r\\n"),
                    "device_add", QMP_OK);
     DO_TEST_DETACH("base-live", "disk-virtio", false, false,
                    "device_del", QMP_OK,
+                   "__com.redhat_drive_del", QMP_NOT_FOUND,
                    "human-monitor-command", HMP(""));
 
+    DO_TEST_ATTACH("base-live", "disk-virtio", false, true,
+                   "__com.redhat_drive_add", QMP_OK,
+                   "device_add", QMP_OK);
+    DO_TEST_DETACH("base-live", "disk-virtio", false, false,
+                   "device_del", QMP_OK,
+                   "__com.redhat_drive_del", QMP_OK);
+
     DO_TEST_ATTACH_EVENT("base-live", "disk-virtio", false, true,
+                         "__com.redhat_drive_add", QMP_NOT_FOUND,
                          "human-monitor-command", HMP("OK\\r\\n"),
                          "device_add", QMP_OK);
     DO_TEST_DETACH("base-live", "disk-virtio", true, true,
                    "device_del", QMP_OK,
+                   "__com.redhat_drive_del", QMP_NOT_FOUND,
                    "human-monitor-command", HMP(""));
     DO_TEST_DETACH("base-live", "disk-virtio", false, false,
                    "device_del", QMP_DEVICE_DELETED("virtio-disk4") QMP_OK,
+                   "__com.redhat_drive_del", QMP_NOT_FOUND,
                    "human-monitor-command", HMP(""));
 
+    DO_TEST_ATTACH_EVENT("base-live", "disk-virtio", false, true,
+                         "__com.redhat_drive_add", QMP_OK,
+                         "device_add", QMP_OK);
+    DO_TEST_DETACH("base-live", "disk-virtio", true, true,
+                   "device_del", QMP_OK,
+                   "__com.redhat_drive_del", QMP_OK);
+    DO_TEST_DETACH("base-live", "disk-virtio", false, false,
+                   "device_del", QMP_DEVICE_DELETED("virtio-disk4") QMP_OK,
+                   "__com.redhat_drive_del", QMP_OK);
+
     DO_TEST_ATTACH("base-live", "disk-usb", false, true,
+                   "__com.redhat_drive_add", QMP_NOT_FOUND,
                    "human-monitor-command", HMP("OK\\r\\n"),
                    "device_add", QMP_OK);
     DO_TEST_DETACH("base-live", "disk-usb", false, false,
                    "device_del", QMP_OK,
+                   "__com.redhat_drive_del", QMP_NOT_FOUND,
                    "human-monitor-command", HMP(""));
 
+    DO_TEST_ATTACH("base-live", "disk-usb", false, true,
+                   "__com.redhat_drive_add", QMP_OK,
+                   "device_add", QMP_OK);
+    DO_TEST_DETACH("base-live", "disk-usb", false, false,
+                   "device_del", QMP_OK,
+                   "__com.redhat_drive_del", QMP_OK);
+
     DO_TEST_ATTACH_EVENT("base-live", "disk-usb", false, true,
+                         "__com.redhat_drive_add", QMP_NOT_FOUND,
                          "human-monitor-command", HMP("OK\\r\\n"),
                          "device_add", QMP_OK);
     DO_TEST_DETACH("base-live", "disk-usb", true, true,
                    "device_del", QMP_OK,
+                   "__com.redhat_drive_del", QMP_NOT_FOUND,
                    "human-monitor-command", HMP(""));
     DO_TEST_DETACH("base-live", "disk-usb", false, false,
                    "device_del", QMP_DEVICE_DELETED("usb-disk16") QMP_OK,
+                   "__com.redhat_drive_del", QMP_NOT_FOUND,
                    "human-monitor-command", HMP(""));
 
+    DO_TEST_ATTACH_EVENT("base-live", "disk-usb", false, true,
+                         "__com.redhat_drive_add", QMP_OK,
+                         "device_add", QMP_OK);
+    DO_TEST_DETACH("base-live", "disk-usb", true, true,
+                   "device_del", QMP_OK,
+                   "__com.redhat_drive_del", QMP_OK);
+    DO_TEST_DETACH("base-live", "disk-usb", false, false,
+                   "device_del", QMP_DEVICE_DELETED("usb-disk16") QMP_OK,
+                   "__com.redhat_drive_del", QMP_OK);
+
     DO_TEST_ATTACH("base-live", "disk-scsi", false, true,
+                   "__com.redhat_drive_add", QMP_NOT_FOUND,
                    "human-monitor-command", HMP("OK\\r\\n"),
                    "device_add", QMP_OK);
     DO_TEST_DETACH("base-live", "disk-scsi", false, false,
                    "device_del", QMP_OK,
+                   "__com.redhat_drive_del", QMP_NOT_FOUND,
                    "human-monitor-command", HMP(""));
 
+    DO_TEST_ATTACH("base-live", "disk-scsi", false, true,
+                   "__com.redhat_drive_add", QMP_OK,
+                   "device_add", QMP_OK);
+    DO_TEST_DETACH("base-live", "disk-scsi", false, false,
+                   "device_del", QMP_OK,
+                   "__com.redhat_drive_del", QMP_OK);
+
     DO_TEST_ATTACH_EVENT("base-live", "disk-scsi", false, true,
+                         "__com.redhat_drive_add", QMP_NOT_FOUND,
                          "human-monitor-command", HMP("OK\\r\\n"),
                          "device_add", QMP_OK);
     DO_TEST_DETACH("base-live", "disk-scsi", true, true,
                    "device_del", QMP_OK,
+                   "__com.redhat_drive_del", QMP_NOT_FOUND,
                    "human-monitor-command", HMP(""));
     DO_TEST_DETACH("base-live", "disk-scsi", false, false,
                    "device_del", QMP_DEVICE_DELETED("scsi0-0-0-5") QMP_OK,
+                   "__com.redhat_drive_del", QMP_NOT_FOUND,
                    "human-monitor-command", HMP(""));
 
+    DO_TEST_ATTACH_EVENT("base-live", "disk-scsi", false, true,
+                         "__com.redhat_drive_add", QMP_OK,
+                         "device_add", QMP_OK);
+    DO_TEST_DETACH("base-live", "disk-scsi", true, true,
+                   "device_del", QMP_OK,
+                   "__com.redhat_drive_del", QMP_OK);
+    DO_TEST_DETACH("base-live", "disk-scsi", false, false,
+                   "device_del", QMP_DEVICE_DELETED("scsi0-0-0-5") QMP_OK,
+                   "__com.redhat_drive_del", QMP_OK);
+
     DO_TEST_ATTACH("base-without-scsi-controller-live", "disk-scsi-2", false, true,
                    /* Four controllers added */
                    "device_add", QMP_OK,
                    "device_add", QMP_OK,
                    "device_add", QMP_OK,
                    "device_add", QMP_OK,
-                   "human-monitor-command", HMP("OK\\r\\n"),
                    /* Disk added */
+                   "__com.redhat_drive_add", QMP_NOT_FOUND,
+                   "human-monitor-command", HMP("OK\\r\\n"),
                    "device_add", QMP_OK);
     DO_TEST_DETACH("base-with-scsi-controller-live", "disk-scsi-2", false, false,
                    "device_del", QMP_OK,
+                   "__com.redhat_drive_del", QMP_NOT_FOUND,
                    "human-monitor-command", HMP(""));
 
     DO_TEST_ATTACH_EVENT("base-without-scsi-controller-live", "disk-scsi-2", false, true,
@@ -747,14 +823,17 @@ mymain(void)
                          "device_add", QMP_OK,
                          "device_add", QMP_OK,
                          "device_add", QMP_OK,
-                         "human-monitor-command", HMP("OK\\r\\n"),
                          /* Disk added */
+                         "__com.redhat_drive_add", QMP_NOT_FOUND,
+                         "human-monitor-command", HMP("OK\\r\\n"),
                          "device_add", QMP_OK);
     DO_TEST_DETACH("base-with-scsi-controller-live", "disk-scsi-2", true, true,
                    "device_del", QMP_OK,
+                   "__com.redhat_drive_del", QMP_NOT_FOUND,
                    "human-monitor-command", HMP(""));
     DO_TEST_DETACH("base-with-scsi-controller-live", "disk-scsi-2", false, false,
                    "device_del", QMP_DEVICE_DELETED("scsi3-0-5-7") QMP_OK,
+                   "__com.redhat_drive_del", QMP_NOT_FOUND,
                    "human-monitor-command", HMP(""));
 
     DO_TEST_ATTACH("base-live", "qemu-agent", false, true,
@@ -765,38 +844,47 @@ mymain(void)
                    "chardev-remove", QMP_OK);
 
     DO_TEST_ATTACH("base-ccw-live", "ccw-virtio", false, true,
+                   "__com.redhat_drive_add", QMP_NOT_FOUND,
                    "human-monitor-command", HMP("OK\\r\\n"),
                    "device_add", QMP_OK);
     DO_TEST_DETACH("base-ccw-live", "ccw-virtio", false, false,
                    "device_del", QMP_OK,
+                   "__com.redhat_drive_del", QMP_NOT_FOUND,
                    "human-monitor-command", HMP(""));
 
     DO_TEST_ATTACH("base-ccw-live-with-ccw-virtio", "ccw-virtio-2", false, true,
+                   "__com.redhat_drive_add", QMP_NOT_FOUND,
                    "human-monitor-command", HMP("OK\\r\\n"),
                    "device_add", QMP_OK);
 
     DO_TEST_DETACH("base-ccw-live-with-ccw-virtio", "ccw-virtio-2", false, false,
                    "device_del", QMP_OK,
+                   "__com.redhat_drive_del", QMP_NOT_FOUND,
                    "human-monitor-command", HMP(""));
 
     DO_TEST_ATTACH("base-ccw-live-with-ccw-virtio", "ccw-virtio-2-explicit", false, true,
+                   "__com.redhat_drive_add", QMP_NOT_FOUND,
                    "human-monitor-command", HMP("OK\\r\\n"),
                    "device_add", QMP_OK);
 
     DO_TEST_DETACH("base-ccw-live-with-ccw-virtio", "ccw-virtio-2-explicit", false, false,
                    "device_del", QMP_OK,
+                   "__com.redhat_drive_del", QMP_NOT_FOUND,
                    "human-monitor-command", HMP(""));
 
     /* Attach a second device, then detach the first one. Then attach the first one again. */
     DO_TEST_ATTACH("base-ccw-live-with-ccw-virtio", "ccw-virtio-2-explicit", false, true,
+                   "__com.redhat_drive_add", QMP_NOT_FOUND,
                    "human-monitor-command", HMP("OK\\r\\n"),
                    "device_add", QMP_OK);
 
     DO_TEST_DETACH("base-ccw-live-with-2-ccw-virtio", "ccw-virtio-1-explicit", false, true,
                    "device_del", QMP_OK,
+                   "__com.redhat_drive_del", QMP_NOT_FOUND,
                    "human-monitor-command", HMP(""));
 
     DO_TEST_ATTACH("base-ccw-live-with-2-ccw-virtio", "ccw-virtio-1-reverse", false, false,
+                   "__com.redhat_drive_add", QMP_NOT_FOUND,
                    "human-monitor-command", HMP("OK\\r\\n"),
                    "device_add", QMP_OK);
 
-- 
2.12.2