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

2007-10-15 Thread 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

2007-10-15 Thread 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

2007-10-15 Thread 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

2007-10-13 Thread 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

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

2007-10-12 Thread Daniel P. Berrange 
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  -=| 
Index: src/qemu_conf.c
===
RCS file: /data/cvs/libvirt/src/qemu_conf.c,v
retrieving revision 1.16
diff -u -p -r1.16 qemu_conf.c
--- src/qemu_conf.c 12 Oct 2007 16:05:44 -  1.16
+++ src/qemu_conf.c 13 Oct 2007 02:46:23 -
@@ -499,11 +499,14 @@ int qemudExtractVersion(virConnectPtr co
 }
 
 
-/* Parse the XML definition for a disk */
-static struct qemud_vm_disk_def *qemudParseDiskXML(virConnectPtr conn,
-   struct qemud_driver *driver 
ATTRIBUTE_UNUSED,
-   xmlNodePtr node) {
-struct qemud_vm_disk_def *disk = calloc(1, sizeof(struct 
qemud_vm_disk_def));
+/* Parse the XML definition for a disk
+ * @param disk pre-allocated  zero'd disk record
+ * @param node XML nodeset to parse for disk definition
+ * @return 0 on success, -1 on failure
+ */
+static int qemudParseDiskXML(virConnectPtr conn,
+ struct qemud_vm_disk_def *disk,
+ xmlNodePtr node) {
 xmlNodePtr cur;
 xmlChar *device = NULL;
 xmlChar *source = NULL;
@@ -511,11 +514,6 @@ static struct qemud_vm_disk_def *qemudPa
 xmlChar *type = NULL;
 int typ = 0;
 
-if (!disk) {
-qemudReportError(conn, NULL, NULL, VIR_ERR_NO_MEMORY, disk);
-return NULL;
-}
-
 type = xmlGetProp(node, BAD_CAST type);
 if (type != NULL) {
 if (xmlStrEqual(type, BAD_CAST file))
@@ -612,7 +610,7 @@ static struct qemud_vm_disk_def *qemudPa
 xmlFree(target);
 xmlFree(source);
 
-return disk;
+return 0;
 
  error:
 if (type)
@@ -623,8 +621,7 @@ static struct qemud_vm_disk_def *qemudPa
 xmlFree(source);
 if (device)
 xmlFree(device);
-free(disk);
-return NULL;
+return -1;
 }
 
 static void qemudRandomMAC(struct qemud_vm_net_def *net) {
@@ -637,11 +634,14 @@ static void qemudRandomMAC(struct qemud_
 }
 
 
-/* Parse the XML definition for a network interface */
-static struct qemud_vm_net_def *qemudParseInterfaceXML(virConnectPtr conn,
-   struct qemud_driver 
*driver ATTRIBUTE_UNUSED,
-   xmlNodePtr node) {
-struct qemud_vm_net_def *net = calloc(1, sizeof(struct qemud_vm_net_def));
+/* Parse the XML definition for a network interface
+ * @param net pre-allocated  zero'd net record
+ * @param node XML nodeset to parse for net definition
+ * @return 0 on success, -1 on failure
+ */
+static int qemudParseInterfaceXML(virConnectPtr conn,
+  struct qemud_vm_net_def *net,
+  xmlNodePtr node) {
 xmlNodePtr cur;
 xmlChar *macaddr = NULL;
 xmlChar *type = NULL;
@@ -652,11 +652,6 @@ static struct qemud_vm_net_def *qemudPar
 xmlChar *address = NULL;
 xmlChar *port = NULL;
 
-if (!net) {
-qemudReportError(conn, NULL, NULL, VIR_ERR_NO_MEMORY, net);
-return NULL;
-}
-
 net-type = QEMUD_NET_USER;
 
 type = xmlGetProp(node, BAD_CAST type);
@@ -869,7 +864,7 @@ static struct qemud_vm_net_def *qemudPar
 xmlFree(address);
 }
 
-return net;
+return 0;
 
  error:
 if (network)
@@ -884,24 +879,17 @@ static struct qemud_vm_net_def *qemudPar
 xmlFree(script);
 if (bridge)
 xmlFree(bridge);
-free(net);
-return NULL;
+return -1;
 }
 
 
 /* Parse the XML definition for a network interface */