[Libvir] PATCH: Implement CDROM media change for QEMU/KVM driver
Hugh recently added support for CDROM media change to the Xen driver, so I thought it was time I did the same for the QEMU/KVM driver. The attached patch implements the virDomainAttachDevice API to support changing the media of the CDROM device. To re-use the existing QEMU code for parsing I had to re-factor a couple of the internal QEMU device parsing apis so that they get supplied with a pre-allocated struct for the device info, rather than allocating it themselves. Although this isn't strictly needed for CDROM media change, I have prepared the virDomainAttachDevice method so that I can easily add USB device hotplut in the near future. It will also be useful when KVM gets paravirt driver support sooon... The actual implementation just runs the 'change cdrom path' command over the QEMU monitor console. diffstat libvirt-qemu-cdrom-change.patch qemu_conf.c | 135 +++++++++++++++++++++++++++++++++++++++------------------- qemu_conf.h | 21 +++++++++ qemu_driver.c | 132 ++++++++++++++++++++++++++++++++++++++++++++++++++------ 3 files changed, 230 insertions(+), 58 deletions(-) Dan. -- |=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=| |=- Perl modules: http://search.cpan.org/~danberr/ -=| |=- Projects: http://freshmeat.net/~danielpb/ -=| |=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
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
+ 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"); Much as it irritates me to say it, a fixed-size buffer and snprintf might be preferable here ... Rich. -- Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/ Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in England and Wales under Company Registration No. 03798903
Richard W.M. Jones wrote:
+ strcat(cmd, newdisk->src);
Also, is quoting/escaping required? In a naive libvirt-based app, it's plausible that the string is provided by the user and could contain \n to send arbitrary commands to the qemu console. Rich. -- Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/ Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in England and Wales under Company Registration No. 03798903
Richard W.M. Jones wrote:
Richard W.M. Jones wrote:
+ strcat(cmd, newdisk->src);
Also, is quoting/escaping required? In a naive libvirt-based app, it's plausible that the string is provided by the user and could contain \n to send arbitrary commands to the qemu console.
Agreed. We can use something like qemudEscapeShellArg for that. This (untested) patch adds qemudEscapeArg for non-shell arguments. -jim
Jim Paris wrote:
Richard W.M. Jones wrote:
Richard W.M. Jones wrote:
+ strcat(cmd, newdisk->src);
Also, is quoting/escaping required? In a naive libvirt-based app, it's plausible that the string is provided by the user and could contain \n to send arbitrary commands to the qemu console.
Agreed. We can use something like qemudEscapeShellArg for that. This (untested) patch adds qemudEscapeArg for non-shell arguments.
Sorry, I think my mailer did something funny there. Here's the patch. --- src/qemu_driver.c | 30 +++++++++++++++++++++++------- 1 files changed, 23 insertions(+), 7 deletions(-) diff --git a/src/qemu_driver.c b/src/qemu_driver.c index c9cecc0..671b334 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -1905,7 +1905,7 @@ static int qemudDomainGetInfo(virDomainPtr dom, } -static char *qemudEscapeShellArg(const char *in) +static char *qemudEscape(const char *in, int shell) { int len = 0; int i, j; @@ -1927,7 +1927,10 @@ static char *qemudEscapeShellArg(const char *in) len += 2; break; case '\'': - len += 5; + if (shell) + len += 5; + else + len += 1; break; default: len += 1; @@ -1954,11 +1957,15 @@ static char *qemudEscapeShellArg(const char *in) out[j++] = in[i]; break; case '\'': - out[j++] = '\''; - out[j++] = '\\'; - out[j++] = '\\'; - out[j++] = '\''; - out[j++] = '\''; + if (shell) { + out[j++] = '\''; + out[j++] = '\\'; + out[j++] = '\\'; + out[j++] = '\''; + out[j++] = '\''; + } else { + out[j++] = in[i]; + } break; default: out[j++] = in[i]; @@ -1970,6 +1977,15 @@ static char *qemudEscapeShellArg(const char *in) return out; } +static char *qemudEscapeArg(const char *in) +{ + return qemudEscape(in, 0); +} + +static char *qemudEscapeShellArg(const char *in) +{ + return qemudEscape(in, 1); +} #define QEMUD_SAVE_MAGIC "LibvirtQemudSave" #define QEMUD_SAVE_VERSION 1 -- 1.5.3.rc4
On Mon, Oct 15, 2007 at 11:25:44AM -0400, Jim Paris wrote:
Jim Paris wrote:
Richard W.M. Jones wrote:
Richard W.M. Jones wrote:
+ strcat(cmd, newdisk->src);
Also, is quoting/escaping required? In a naive libvirt-based app, it's plausible that the string is provided by the user and could contain \n to send arbitrary commands to the qemu console.
Agreed. We can use something like qemudEscapeShellArg for that. This (untested) patch adds qemudEscapeArg for non-shell arguments.
Sorry, I think my mailer did something funny there. Here's the patch.
I want to test how QEMU handles quoting of filenames, but this looks like it'll do the trick. I'll cook up a revised patch incorporating all the feedback from this thread. Dan. -- |=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=| |=- Perl modules: http://search.cpan.org/~danberr/ -=| |=- Projects: http://freshmeat.net/~danielpb/ -=| |=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
Richard W.M. Jones wrote:
+ 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");
Much as it irritates me to say it, a fixed-size buffer and snprintf might be preferable here ...
Or asprintf. -jim
participants (3)
-
Daniel P. Berrange -
Jim Paris -
Richard W.M. Jones