[Libvir] PATCH: Implement CDROM media change for QEMU/KVM driver

[Jim Paris]

Hi Dan,

That's definitely be a useful feature.  Some comments...

> @@ -453,7 +454,7 @@ static int qemudOpenMonitor(virConnectPt
>      char buf[1024];
>      int ret = -1;
>  
> -    if (!(monfd = open(monitor, O_RDWR))) {
> +    if (!(monfd = open(monitor, O_NOCTTY |O_RDWR))) {

Is this just to ensure portability or does it change the behavior?

> @@ -1365,13 +1360,35 @@ static int qemudMonitorCommand(struct qe
> + retry1:
> +    if (write(vm->logfile, buf, strlen(buf)) < 0) {
> +        /* Log, but ignore failures to write logfile for VM */
> +        if (errno == EINTR)
> +            goto retry1;
> +        qemudLog(QEMUD_WARN, "Unable to log VM console data: %s",
> +                 strerror(errno));
> +    }
> +
>      *reply = buf;
>      return 0;
> +
> + error:
> +    if (buf) {
> +    retry2:
> +        if (write(vm->logfile, buf, strlen(buf)) < 0) {
> +            /* Log, but ignore failures to write logfile for VM */
> +            if (errno == EINTR)
> +                goto retry2;
> +            qemudLog(QEMUD_WARN, "Unable to log VM console data: %s",
> +                     strerror(errno));
> +        }
> +        free(buf);
> +    }
> +    return -1;

I think both of these retry loops could be replaced with safewrite
from util.c:

  if (safewrite(vm->logfile, buf, strlen(buf)) != strlen(buf))
     qemudLog(...)

> +static int qemudDomainChangeCDROM(virDomainPtr dom,
> +                                  struct qemud_vm *vm,
> +                                  struct qemud_vm_disk_def *olddisk,
> +                                  struct qemud_vm_disk_def *newdisk) {
> +    struct qemud_driver *driver = (struct qemud_driver *)dom->conn->privateData;
> +    char *cmd;
> +    char *reply;
> +    /* XXX QEMU only supports a single CDROM for now */
> +    /*cmd = malloc(strlen("change ") + strlen(olddisk->dst) + 1 + strlen(newdisk->src) + 2);*/
> +    cmd = malloc(strlen("change ") + strlen("cdrom") + 1 + strlen(newdisk->src) + 2);
> +    if (!cmd) {
> +        qemudReportError(dom->conn, dom, NULL, VIR_ERR_NO_MEMORY, "monitor command");
> +        return -1;
> +    }
> +    strcpy(cmd, "change ");
> +    /* XXX QEMU only supports a single CDROM for now */
> +    /*strcat(cmd, olddisk->dst);*/
> +    strcat(cmd, "cdrom");
> +    strcat(cmd, " ");
> +    strcat(cmd, newdisk->src);
> +    strcat(cmd, "\n");

Commands should be terminated with "\r", otherwise the terminal layer
replaces "\n" -> "\r\n", and bugs in earlier qemu means it would
execute the command twice.  Recent qemu will still print the "(qemu) "
prompt twice in this case, which might confuse qemudMonitorCommand
into thinking that a subsequent command is finished before it's even
started.

Unless the O_NOCTTY somehow fixes that?

-jim