[libvirt] attach/detach device in json mode
Hi, all The qemu does not support monitor_command drive_add and drive_del in json mode. These two commands are important because attaching/detaching commands need them. AFAIK, if we use libvirt provided by RHEL6(not upstream), we connect to qemu in json mode. So, it is important to make qemu to support these monitor commands in json mode. Markus Armbruster(armbru@redhat.com) said these commands are designed badly, and he has started to work on saner commands some time ago: http://lists.nongnu.org/archive/html/qemu-devel/2011-02/msg02042.html He suggested us to use human monitor passthrough in the meantime. Should we modify libvirt to use human monitor passthrough command to support attaching/detaching device before the saner commands are ready?
On Tue, Mar 08, 2011 at 10:30:45AM +0800, Wen Congyang wrote:
Hi, all
The qemu does not support monitor_command drive_add and drive_del in json mode. These two commands are important because attaching/detaching commands need them.
AFAIK, if we use libvirt provided by RHEL6(not upstream), we connect to qemu in json mode. So, it is important to make qemu to support these monitor commands in json mode.
Markus Armbruster(armbru@redhat.com) said these commands are designed badly, and he has started to work on saner commands some time ago: http://lists.nongnu.org/archive/html/qemu-devel/2011-02/msg02042.html
He suggested us to use human monitor passthrough in the meantime. Should we modify libvirt to use human monitor passthrough command to support attaching/detaching device before the saner commands are ready?
Now that we have got Jiri's patches to allow use of loadvm, savevm, delvm via human monitor passthrough, we can do the same with drive_add/drive_del very easily Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
Here comes the patch:)
From c836379e8ad585a718496e3a93638aa999e917d3 Mon Sep 17 00:00:00 2001 From: Hu Tao <hutao@cn.fujitsu.com> Date: Fri, 11 Mar 2011 09:20:06 +0800 Subject: [PATCH] qemu: fallback to HMP drive_add/drive_del
fallback to HMP drive_add/drive_del commands if not found in QMP --- src/qemu/qemu_monitor_json.c | 39 +++++++++++++++++++++------------------ 1 files changed, 21 insertions(+), 18 deletions(-) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index d97dc68..c7e94c2 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -2359,11 +2359,18 @@ int qemuMonitorJSONAddDrive(qemuMonitorPtr mon, if (!cmd) return -1; - ret = qemuMonitorJSONCommand(mon, cmd, &reply); + if ((ret = qemuMonitorJSONCommand(mon, cmd, &reply) < 0)) + goto cleanup; - if (ret == 0) - ret = qemuMonitorJSONCheckError(cmd, reply); + if (qemuMonitorJSONHasError(reply, "CommandNotFound")) { + VIR_DEBUG0(_("drive_add command not found, trying HMP")); + ret = qemuMonitorTextAddDrive(mon, drivestr); + goto cleanup; + } + + ret = qemuMonitorJSONCheckError(cmd, reply); +cleanup: virJSONValueFree(cmd); virJSONValueFree(reply); return ret; @@ -2384,22 +2391,18 @@ int qemuMonitorJSONDriveDel(qemuMonitorPtr mon, if (!cmd) return -1; - ret = qemuMonitorJSONCommand(mon, cmd, &reply); + if ((ret = qemuMonitorJSONCommand(mon, cmd, &reply)) < 0) + goto cleanup; - if (ret == 0) { - /* See if drive_del isn't supported */ - if (qemuMonitorJSONHasError(reply, "CommandNotFound")) { - 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); - } + if (qemuMonitorJSONHasError(reply, "CommandNotFound")) { + VIR_DEBUG0(_("drive_del command not found, trying HMP")); + ret = qemuMonitorTextDriveDel(mon, drivestr); + } 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); } cleanup: -- 1.7.3.1
At 03/11/2011 09:30 AM, Hu Tao Write:
Here comes the patch:)
From c836379e8ad585a718496e3a93638aa999e917d3 Mon Sep 17 00:00:00 2001 From: Hu Tao <hutao@cn.fujitsu.com> Date: Fri, 11 Mar 2011 09:20:06 +0800 Subject: [PATCH] qemu: fallback to HMP drive_add/drive_del
fallback to HMP drive_add/drive_del commands if not found in QMP
--- src/qemu/qemu_monitor_json.c | 39 +++++++++++++++++++++------------------ 1 files changed, 21 insertions(+), 18 deletions(-)
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index d97dc68..c7e94c2 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -2359,11 +2359,18 @@ int qemuMonitorJSONAddDrive(qemuMonitorPtr mon, if (!cmd) return -1;
- ret = qemuMonitorJSONCommand(mon, cmd, &reply); + if ((ret = qemuMonitorJSONCommand(mon, cmd, &reply) < 0)) + goto cleanup;
- if (ret == 0) - ret = qemuMonitorJSONCheckError(cmd, reply); + if (qemuMonitorJSONHasError(reply, "CommandNotFound")) { + VIR_DEBUG0(_("drive_add command not found, trying HMP"));
The debug infomation does not need to be translated.
+ ret = qemuMonitorTextAddDrive(mon, drivestr); + goto cleanup; + } + + ret = qemuMonitorJSONCheckError(cmd, reply);
+cleanup: virJSONValueFree(cmd); virJSONValueFree(reply); return ret; @@ -2384,22 +2391,18 @@ int qemuMonitorJSONDriveDel(qemuMonitorPtr mon, if (!cmd) return -1;
- ret = qemuMonitorJSONCommand(mon, cmd, &reply); + if ((ret = qemuMonitorJSONCommand(mon, cmd, &reply)) < 0) + goto cleanup;
- if (ret == 0) { - /* See if drive_del isn't supported */ - if (qemuMonitorJSONHasError(reply, "CommandNotFound")) { - 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); - } + if (qemuMonitorJSONHasError(reply, "CommandNotFound")) { + VIR_DEBUG0(_("drive_del command not found, trying HMP"));
the same as the above.
+ ret = qemuMonitorTextDriveDel(mon, drivestr); + } 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); }
cleanup:
+ if (qemuMonitorJSONHasError(reply, "CommandNotFound")) { + VIR_DEBUG0(_("drive_add command not found, trying HMP"));
The debug infomation does not need to be translated.
<...>
+ if (qemuMonitorJSONHasError(reply, "CommandNotFound")) { + VIR_DEBUG0(_("drive_del command not found, trying HMP"));
the same as the above.
You're right. trivial to fix so I won't send a v2. -- Thanks, Hu Tao
On Fri, Mar 11, 2011 at 09:49:02 +0800, Wen Congyang wrote:
- if (ret == 0) - ret = qemuMonitorJSONCheckError(cmd, reply); + if (qemuMonitorJSONHasError(reply, "CommandNotFound")) { + VIR_DEBUG0(_("drive_add command not found, trying HMP"));
The debug infomation does not need to be translated.
It's no like it doesn't need to be translated. It must not be translated and make syntax-check fails on this. Jirka
- /* See if drive_del isn't supported */ - if (qemuMonitorJSONHasError(reply, "CommandNotFound")) { - 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); - } + if (qemuMonitorJSONHasError(reply, "CommandNotFound")) { + VIR_DEBUG0(_("drive_del command not found, trying HMP")); + ret = qemuMonitorTextDriveDel(mon, drivestr); + } 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); }
Looks good, although I think we should issue the "deleting disk is not supported. This may leak data if disk is reassigned" error also in case human-monitor-command is not supported. But that will need some more work on the HMP infrastructure since it's not possible to get this from qemuMonitorText* function that we call. Jirka
On 03/11/2011 08:05 AM, Jiri Denemark wrote:
+ if (qemuMonitorJSONHasError(reply, "CommandNotFound")) { + VIR_DEBUG0(_("drive_del command not found, trying HMP")); + ret = qemuMonitorTextDriveDel(mon, drivestr); + } 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); }
Looks good, although I think we should issue the "deleting disk is not supported. This may leak data if disk is reassigned" error also in case human-monitor-command is not supported. But that will need some more work on the HMP infrastructure since it's not possible to get this from qemuMonitorText* function that we call.
Do we want to push this now, or get the infrastructure improvements in first? -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Fri, Mar 11, 2011 at 09:56:42AM -0700, Eric Blake wrote:
On 03/11/2011 08:05 AM, Jiri Denemark wrote:
+ if (qemuMonitorJSONHasError(reply, "CommandNotFound")) { + VIR_DEBUG0(_("drive_del command not found, trying HMP")); + ret = qemuMonitorTextDriveDel(mon, drivestr); + } 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); }
Looks good, although I think we should issue the "deleting disk is not supported. This may leak data if disk is reassigned" error also in case human-monitor-command is not supported. But that will need some more work on the HMP infrastructure since it's not possible to get this from qemuMonitorText* function that we call.
Do we want to push this now, or get the infrastructure improvements in first?
What about get it in first based on the fact that human-monitor-command is supported? (with removing the two _() in VIR_DEBUG0) -- Thanks, Hu Tao
On Fri, Mar 11, 2011 at 04:05:16PM +0100, Jiri Denemark wrote:
- /* See if drive_del isn't supported */ - if (qemuMonitorJSONHasError(reply, "CommandNotFound")) { - 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); - } + if (qemuMonitorJSONHasError(reply, "CommandNotFound")) { + VIR_DEBUG0(_("drive_del command not found, trying HMP")); + ret = qemuMonitorTextDriveDel(mon, drivestr); + } 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); }
Looks good, although I think we should issue the "deleting disk is not supported. This may leak data if disk is reassigned" error also in case human-monitor-command is not supported. But that will need some more work on the HMP infrastructure since it's not possible to get this from qemuMonitorText* function that we call.
Is an "unknown command" reply the information we want? If yes this patch v2 should do the work. -- Thanks, Hu Tao
At 03/14/2011 10:50 AM, Hu Tao Write:
On Fri, Mar 11, 2011 at 04:05:16PM +0100, Jiri Denemark wrote:
- /* See if drive_del isn't supported */ - if (qemuMonitorJSONHasError(reply, "CommandNotFound")) { - 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); - } + if (qemuMonitorJSONHasError(reply, "CommandNotFound")) { + VIR_DEBUG0(_("drive_del command not found, trying HMP")); + ret = qemuMonitorTextDriveDel(mon, drivestr); + } 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); }
Looks good, although I think we should issue the "deleting disk is not supported. This may leak data if disk is reassigned" error also in case human-monitor-command is not supported. But that will need some more work on the HMP infrastructure since it's not possible to get this from qemuMonitorText* function that we call.
Is an "unknown command" reply the information we want? If yes this patch v2 should do the work.
Jirka said we should issue it when *human-monitor-command* is not supported, not *drive_del* is not supported in text mode.
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, Mar 14, 2011 at 02:20:39PM +0800, Wen Congyang wrote:
At 03/14/2011 10:50 AM, Hu Tao Write:
On Fri, Mar 11, 2011 at 04:05:16PM +0100, Jiri Denemark wrote:
- /* See if drive_del isn't supported */ - if (qemuMonitorJSONHasError(reply, "CommandNotFound")) { - 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); - } + if (qemuMonitorJSONHasError(reply, "CommandNotFound")) { + VIR_DEBUG0(_("drive_del command not found, trying HMP")); + ret = qemuMonitorTextDriveDel(mon, drivestr); + } 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); }
Looks good, although I think we should issue the "deleting disk is not supported. This may leak data if disk is reassigned" error also in case human-monitor-command is not supported. But that will need some more work on the HMP infrastructure since it's not possible to get this from qemuMonitorText* function that we call.
Is an "unknown command" reply the information we want? If yes this patch v2 should do the work.
Jirka said we should issue it when *human-monitor-command* is not supported, not *drive_del* is not supported in text mode.
got it. Thanks for explaination. How can we tell if a command is not supported? A null reply? Or an "unknown command" reply? -- Thanks, Hu Tao
participants (5)
-
Daniel P. Berrange -
Eric Blake -
Hu Tao -
Jiri Denemark -
Wen Congyang