On Thu, 2009-09-24 at 16:00 +0100, Daniel P. Berrange wrote:
> * src/qemu/qemu_monitor.c, src/qemu/qemu_monitor.h: Add new APis
> qemuMonitorChangeMedia and qemuMonitorEjectMedia. Pull in code
> for qemudEscape
> * src/qemu/qemu_driver.c: Remove code that directly issues 'eject'
> and 'change' commands in favour of API calls.
> ---
> src/qemu/qemu_driver.c | 52 +++-----------
> src/qemu/qemu_monitor_text.c | 159 ++++++++++++++++++++++++++++++++++++++++++
> src/qemu/qemu_monitor_text.h | 10 +++
> 3 files changed, 178 insertions(+), 43 deletions(-)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index a0b5e49..b15dc03 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -4573,9 +4573,9 @@ static int qemudDomainChangeEjectableMedia(virConnectPtr conn,
> unsigned int qemuCmdFlags)
> {
> virDomainDiskDefPtr origdisk = NULL, newdisk;
> - char *cmd, *reply, *safe_path;
> char *devname = NULL;
> int i;
> + int ret;
>
> origdisk = NULL;
> newdisk = dev->data.disk;
> @@ -4621,52 +4621,18 @@ static int qemudDomainChangeEjectableMedia(virConnectPtr conn,
> }
>
> if (newdisk->src) {
> - safe_path = qemudEscapeMonitorArg(newdisk->src);
> - if (!safe_path) {
> - virReportOOMError(conn);
> - VIR_FREE(devname);
> - return -1;
> - }
> - if (virAsprintf(&cmd, "change %s \"%s\"", devname, safe_path) == -1) {
> - virReportOOMError(conn);
> - VIR_FREE(safe_path);
> - VIR_FREE(devname);
> - return -1;
> - }
> - VIR_FREE(safe_path);
> -
> - } else if (virAsprintf(&cmd, "eject %s", devname) == -1) {
> - virReportOOMError(conn);
> - VIR_FREE(devname);
> - return -1;
> - }
> - VIR_FREE(devname);
> -
> - if (qemudMonitorCommand(vm, cmd, &reply) < 0) {
> - qemudReportError(conn, dom, NULL, VIR_ERR_OPERATION_FAILED,
> - "%s", _("could not change cdrom media"));
> - VIR_FREE(cmd);
> - return -1;
> + ret = qemuMonitorChangeMedia(vm, devname, newdisk->src);
> + } else {
> + ret = qemuMonitorEjectMedia(vm, devname);
> }
>
> - /* If the command failed qemu prints:
> - * device not found, device is locked ...
> - * No message is printed on success it seems */
> - DEBUG ("%s: ejectable media change reply: %s", vm->def->name, reply);
> - if (strstr(reply, "\ndevice ")) {
> - qemudReportError (conn, dom, NULL, VIR_ERR_OPERATION_FAILED,
> - _("changing cdrom media failed: %s"), reply);
> - VIR_FREE(reply);
> - VIR_FREE(cmd);
> - return -1;
> + if (ret == 0) {
> + VIR_FREE(origdisk->src);
> + origdisk->src = newdisk->src;
> + newdisk->src = NULL;
> + origdisk->type = newdisk->type;
> }
> - VIR_FREE(reply);
> - VIR_FREE(cmd);
>
> - VIR_FREE(origdisk->src);
> - origdisk->src = newdisk->src;
> - newdisk->src = NULL;
> - origdisk->type = newdisk->type;
> return 0;
Should return ret here
> }
>
> diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c
> index be13dce..8be8047 100644
> --- a/src/qemu/qemu_monitor_text.c
> +++ b/src/qemu/qemu_monitor_text.c
> @@ -40,6 +40,84 @@
>
> #define VIR_FROM_THIS VIR_FROM_QEMU
>
> +static char *qemudEscape(const char *in, int shell)
> +{
Personally, rather than creating a copy of the code, I'd have moved it
and exported it back to qemu_driver.c - end result is the same, but the
way you've done it, I couldn't just look at the patch to confirm that
the copy of the code was the same as the original
...
> @@ -651,3 +729,84 @@ int qemuMonitorSetBalloon(const virDomainObjPtr vm,
> return ret;
> }
>
> +int qemuMonitorEjectMedia(const virDomainObjPtr vm,
> + const char *devname)
> +{
> + char *cmd = NULL;
> + char *reply = NULL;
> + int ret = -1;
> +
> + if (virAsprintf(&cmd, "eject %s", devname) < 0) {
> + virReportOOMError(NULL);
> + goto cleanup;
> + }
> +
> + if (qemudMonitorCommand(vm, cmd, &reply) < 0) {
> + qemudReportError(NULL, NULL, NULL, VIR_ERR_OPERATION_FAILED,
> + _("could not eject media on %s"), devname);
> + goto cleanup;
> + }
> +
> + /* If the command failed qemu prints:
> + * device not found, device is locked ...
> + * No message is printed on success it seems */
> + DEBUG ("%s: ejectable media change reply: %s", vm->def->name, reply);
> + if (strstr(reply, "\ndevice ")) {
> + qemudReportError(NULL, NULL, NULL, VIR_ERR_OPERATION_FAILED,
> + _("could not eject media on %s: %s"), devname, reply);
> + goto cleanup;
> + }
> +
> + ret = 0;
> +
> +cleanup:
> + VIR_FREE(reply);
> + VIR_FREE(cmd);
> + return ret;
> +}
> +
> +
> +int qemuMonitorChangeMedia(const virDomainObjPtr vm,
> + const char *devname,
> + const char *newmedia)
> +{
> + char *cmd = NULL;
> + char *reply = NULL;
> + char *safepath = NULL;
> + int ret = -1;
> +
> + if (!(safepath = qemudEscapeMonitorArg(newmedia))) {
> + virReportOOMError(NULL);
> + goto cleanup;
> + }
> +
> + if (virAsprintf(&cmd, "change %s \"%s\"", devname, safepath) < 0) {
> + virReportOOMError(NULL);
> + goto cleanup;
> + }
> +
> + if (qemudMonitorCommand(vm, cmd, &reply) < 0) {
> + qemudReportError(NULL, NULL, NULL, VIR_ERR_OPERATION_FAILED,
> + _("could not eject media on %s"), devname);
> + goto cleanup;
> + }
> +
> + /* If the command failed qemu prints:
> + * device not found, device is locked ...
> + * No message is printed on success it seems */
> + DEBUG ("%s: ejectable media change reply: %s", vm->def->name, reply);
> + if (strstr(reply, "\ndevice ")) {
> + qemudReportError(NULL, NULL, NULL, VIR_ERR_OPERATION_FAILED,
> + _("could not eject media on %s: %s"), devname, reply);
> + goto cleanup;
> + }
Pity this code is duplicated
Should be 'could not eject' here
Otherwise, ACK
Cheers,
Mark.
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list