From 8bac3f7591f7df980a631deeb67a2f20eab6fc62 Mon Sep 17 00:00:00 2001 Message-Id: <8bac3f7591f7df980a631deeb67a2f20eab6fc62@dist-git> From: Jiri Denemark 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 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 | 107 +++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 6 ++ tests/qemuhotplugtest.c | 93 +++++++++++++++++++++++++++++- 4 files changed, 212 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 6fc038a8d9..5e0e95cc51 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -2947,8 +2947,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); } @@ -3137,8 +3139,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 af754e870e..ec8469476e 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -4055,6 +4055,113 @@ 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 qemuMonitorJSONDiskSnapshot(qemuMonitorPtr mon, virJSONValuePtr actions, const char *device, const char *file, diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index b92fc3762b..9c8fab7cc0 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -238,6 +238,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 qemuMonitorJSONDiskSnapshot(qemuMonitorPtr mon, virJSONValuePtr actions, const char *device, diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c index 663e33ed00..cddedf768e 100644 --- a/tests/qemuhotplugtest.c +++ b/tests/qemuhotplugtest.c @@ -665,6 +665,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); @@ -685,67 +693,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, @@ -754,14 +830,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, @@ -772,38 +851,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); @@ -821,6 +909,7 @@ mymain(void) "object-del", QMP_OK); DO_TEST_ATTACH("base-live+disk-scsi-wwn", "disk-scsi-duplicate-wwn", false, false, + "__com.redhat_drive_add", QMP_NOT_FOUND, "human-monitor-command", HMP("OK\\r\\n"), "device_add", QMP_OK); -- 2.18.0