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

[Richard W.M. Jones]

+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


smime.p7s
Description: S/MIME Cryptographic Signature
--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

[Richard W.M. Jones]

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


smime.p7s
Description: S/MIME Cryptographic Signature
--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

[Jim Paris]

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

--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [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

--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list