[libvirt] [PATCH 09/27] Add APIs for issuing 'eject' and 'change dev' monitor commands

[Mark McLoughlin]

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.