From b32e593db092c9c4f7c85028fe318f6b401451cb Mon Sep 17 00:00:00 2001 Message-Id: From: "Daniel P. Berrange" Date: Fri, 17 Dec 2010 14:55:38 +0100 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_json.c | 81 +++++++++++++++++++++++++++++++++++++++++++- tests/qemuhotplugtest.c | 74 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 154 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index edfa67f..fd567e4 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -3474,9 +3474,54 @@ 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 */ /* XXX Update to use QMP, if QMP ever adds support for drive_add */ VIR_DEBUG("drive_add command not found, trying HMP"); - return qemuMonitorTextAddDrive(mon, drivestr); + ret = qemuMonitorTextAddDrive(mon, drivestr); + +cleanup: + virJSONValueFree(args); + virJSONValueFree(cmd); + virJSONValueFree(reply); + return ret; } @@ -3484,7 +3529,37 @@ 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 */ /* XXX Update to use QMP, if QMP ever adds support for drive_del */ VIR_DEBUG("drive_del command not found, trying HMP"); if ((ret = qemuMonitorTextDriveDel(mon, drivestr)) < 0) { @@ -3497,6 +3572,10 @@ int qemuMonitorJSONDriveDel(qemuMonitorPtr mon, virResetLastError(); } } + +cleanup: + virJSONValueFree(cmd); + virJSONValueFree(reply); return ret; } diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c index 368a5e7..3b547f2 100644 --- a/tests/qemuhotplugtest.c +++ b/tests/qemuhotplugtest.c @@ -416,6 +416,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); @@ -436,56 +444,122 @@ mymain(void) "chardev-remove", QMP_OK); DO_TEST_ATTACH("hotplug-base", "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("hotplug-base", "disk-virtio", false, false, "device_del", QMP_OK, + "__com.redhat_drive_del", QMP_NOT_FOUND, "human-monitor-command", HMP("")); + DO_TEST_ATTACH("hotplug-base", "disk-virtio", false, true, + "__com.redhat_drive_add", QMP_OK, + "device_add", QMP_OK); + DO_TEST_DETACH("hotplug-base", "disk-virtio", false, false, + "device_del", QMP_OK, + "__com.redhat_drive_del", QMP_OK); + DO_TEST_ATTACH_EVENT("hotplug-base", "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("hotplug-base", "disk-virtio", true, true, "device_del", QMP_OK, + "__com.redhat_drive_del", QMP_NOT_FOUND, "human-monitor-command", HMP("")); DO_TEST_DETACH("hotplug-base", "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("hotplug-base", "disk-virtio", false, true, + "__com.redhat_drive_add", QMP_OK, + "device_add", QMP_OK); + DO_TEST_DETACH("hotplug-base", "disk-virtio", true, true, + "device_del", QMP_OK, + "__com.redhat_drive_del", QMP_OK); + DO_TEST_DETACH("hotplug-base", "disk-virtio", false, false, + "device_del", QMP_DEVICE_DELETED("virtio-disk4") QMP_OK, + "__com.redhat_drive_del", QMP_OK); + DO_TEST_ATTACH("hotplug-base", "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("hotplug-base", "disk-usb", false, false, "device_del", QMP_OK, + "__com.redhat_drive_del", QMP_NOT_FOUND, "human-monitor-command", HMP("")); + DO_TEST_ATTACH("hotplug-base", "disk-usb", false, true, + "__com.redhat_drive_add", QMP_OK, + "device_add", QMP_OK); + DO_TEST_DETACH("hotplug-base", "disk-usb", false, false, + "device_del", QMP_OK, + "__com.redhat_drive_del", QMP_OK); + DO_TEST_ATTACH_EVENT("hotplug-base", "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("hotplug-base", "disk-usb", true, true, "device_del", QMP_OK, + "__com.redhat_drive_del", QMP_NOT_FOUND, "human-monitor-command", HMP("")); DO_TEST_DETACH("hotplug-base", "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("hotplug-base", "disk-usb", false, true, + "__com.redhat_drive_add", QMP_OK, + "device_add", QMP_OK); + DO_TEST_DETACH("hotplug-base", "disk-usb", true, true, + "device_del", QMP_OK, + "__com.redhat_drive_del", QMP_OK); + DO_TEST_DETACH("hotplug-base", "disk-usb", false, false, + "device_del", QMP_DEVICE_DELETED("usb-disk16") QMP_OK, + "__com.redhat_drive_del", QMP_OK); + DO_TEST_ATTACH("hotplug-base", "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("hotplug-base", "disk-scsi", false, false, "device_del", QMP_OK, + "__com.redhat_drive_del", QMP_NOT_FOUND, "human-monitor-command", HMP("")); + DO_TEST_ATTACH("hotplug-base", "disk-scsi", false, true, + "__com.redhat_drive_add", QMP_OK, + "device_add", QMP_OK); + DO_TEST_DETACH("hotplug-base", "disk-scsi", false, false, + "device_del", QMP_OK, + "__com.redhat_drive_del", QMP_OK); + DO_TEST_ATTACH_EVENT("hotplug-base", "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("hotplug-base", "disk-scsi", true, true, "device_del", QMP_OK, + "__com.redhat_drive_del", QMP_NOT_FOUND, "human-monitor-command", HMP("")); DO_TEST_DETACH("hotplug-base", "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("hotplug-base", "disk-scsi", false, true, + "__com.redhat_drive_add", QMP_OK, + "device_add", QMP_OK); + DO_TEST_DETACH("hotplug-base", "disk-scsi", true, true, + "device_del", QMP_OK, + "__com.redhat_drive_del", QMP_OK); + DO_TEST_DETACH("hotplug-base", "disk-scsi", false, false, + "device_del", QMP_DEVICE_DELETED("scsi0-0-0-5") QMP_OK, + "__com.redhat_drive_del", QMP_OK); + virObjectUnref(driver.caps); virObjectUnref(driver.xmlopt); virObjectUnref(driver.config); -- 2.4.5