Re: [Libvir] PATCH: Avoid buffer out of bounds access in Xen capabilities

[Daniel Veillard]

On Mon, Apr 28, 2008 at 11:42:47PM +0100, Daniel P. Berrange wrote:
 The Xen driver uses a regex to process the hypervisor capabilities data
 
   
 (xen|hvm)-[[:digit:]]+\\.[[:digit:]]+-(x86_32|x86_64|ia64|powerpc64)(p|be)?;
 
 notice how the last match group, however, is optional due to the '?'. The
 code processing matches does not check to see if the match is present or
 not, and just indexes the string on match 3
 
  if (strncmp (token[subs[3].rm_so], p, 1) == 0)
 
 Unfortunately,  subs[3].rm_so is -1 if the match was not present, so we're
 doing an out of bounds array access here. This is fairly harmless, but it
 is still good to fix it. So this patch adds a check for -1 before accessing
 the match. I also replace the strncmp() calls with a call to the brand new
 STRPREFIX() convenience macro

  Okidoc, i assume valgrind spotted that, that's fairly well hidden ...

+1

Daniel

-- 
Red Hat Virtualization group http://redhat.com/virtualization/
Daniel Veillard  | virtualization library  http://libvirt.org/
[EMAIL PROTECTED]  | libxml GNOME XML XSLT toolkit  http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine  http://rpmfind.net/

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

Re: [Libvir] PATCH: Add docs on the network XML format

[Daniel Veillard]

On Tue, Apr 29, 2008 at 03:27:07AM +0100, Daniel P. Berrange wrote:
 This patch fills in the network driver XML format page on
 the website which has been requested many times...

  Yay ! Thanks, +1

Daniel

-- 
Red Hat Virtualization group http://redhat.com/virtualization/
Daniel Veillard  | virtualization library  http://libvirt.org/
[EMAIL PROTECTED]  | libxml GNOME XML XSLT toolkit  http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine  http://rpmfind.net/

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

[Libvir] [PATCH] Ancient libparted

[Soren Hansen]

Some of us are stuck with an ancient libparted, which doesn't know about
PED_PARTITION_PROTECTED. This patch allows us to compile libvirt.


=== modified file 'src/parthelper.c'
--- src/parthelper.c2008-04-10 16:53:29 +
+++ src/parthelper.c2008-04-29 07:47:08 +
@@ -67,8 +67,10 @@
 content = free;
 else if (part-type  PED_PARTITION_METADATA)
 content = metadata;
+#ifdef PED_PARTITION_PROTECTED
 else if (part-type  PED_PARTITION_PROTECTED)
 content = protected;
+#endif
 else
 content = data;
 } else if (part-type == PED_PARTITION_EXTENDED) {
@@ -80,8 +82,10 @@
 content = free;
 else if (part-type  PED_PARTITION_METADATA)
 content = metadata;
+#ifdef PED_PARTITION_PROTECTED
 else if (part-type  PED_PARTITION_PROTECTED)
 content = protected;
+#endif
 else
 content = data;
 }


-- 
Soren Hansen   | 
Virtualisation specialist  | Ubuntu Server Team
Canonical Ltd. | http://www.ubuntu.com/


signature.asc
Description: Digital signature
--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[Libvir] [PATCH] Add bus attribute to disk target definition

[Soren Hansen]

Hi!

I'd like to propose that the following patch gets applied against
libvirt. It adds the option of putting a bus attribute on a disk target.
To acommodate this, it also changes the way drives are defined for kvm
from the old -hda /path/to/file -boot c style to the new -drive
file=/path/to/file,if=ide,boot=on. This makes it possible to specify
virtio, scsi, and ide disks.


=== modified file 'src/qemu_conf.c'
--- src/qemu_conf.c 2008-04-28 15:14:59 +
+++ src/qemu_conf.c 2008-04-29 07:43:11 +
@@ -568,6 +568,7 @@
 xmlChar *source = NULL;
 xmlChar *target = NULL;
 xmlChar *type = NULL;
+xmlChar *bus = NULL;
 int typ = 0;
 
 type = xmlGetProp(node, BAD_CAST type);
@@ -598,6 +599,7 @@
 } else if ((target == NULL) 
(xmlStrEqual(cur-name, BAD_CAST target))) {
 target = xmlGetProp(cur, BAD_CAST dev);
+bus = xmlGetProp(cur, BAD_CAST bus);
 } else if (xmlStrEqual(cur-name, BAD_CAST readonly)) {
 disk-readonly = 1;
 }
@@ -646,7 +648,9 @@
 strcmp((const char *)target, hda) 
 strcmp((const char *)target, hdb) 
 strcmp((const char *)target, hdc) 
-strcmp((const char *)target, hdd)) {
+strcmp((const char *)target, hdd) 
+strcmp((const char *)target, hdd) 
+strncmp((const char *)target, vd, 2)) {
 qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
  _(Invalid harddisk device name: %s), target);
 goto error;
@@ -673,6 +677,20 @@
 goto error;
 }
 
+if (!bus)
+disk-bus = QEMUD_DISK_BUS_IDE;
+else if (!strcmp((const char *)bus, ide))
+disk-bus = QEMUD_DISK_BUS_IDE;
+else if (!strcmp((const char *)bus, scsi))
+disk-bus = QEMUD_DISK_BUS_SCSI;
+else if (!strcmp((const char *)bus, virtio))
+disk-bus = QEMUD_DISK_BUS_VIRTIO;
+else {
+qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, Invalid 
bus type: %s, bus);
+goto error;
+}
+
+xmlFree(bus);
 xmlFree(device);
 xmlFree(target);
 xmlFree(source);
@@ -688,6 +706,8 @@
 xmlFree(source);
 if (device)
 xmlFree(device);
+if (bus)
+xmlFree(bus);
 return -1;
 }
 
@@ -1350,6 +1370,68 @@
 return -1;
 }
 
+static int qemudDiskCompare(const void *aptr, const void *bptr) {
+struct qemud_vm_disk_def *a = (struct qemud_vm_disk_def *) aptr;
+struct qemud_vm_disk_def *b = (struct qemud_vm_disk_def *) bptr;
+if (a-device == b-device)
+return virDiskNameToIndex(a-dst) - virDiskNameToIndex(b-dst);
+else
+return a-device - b-device;
+}
+
+static const char *qemudBusIdToName(int busId) {
+const char *busnames[] = { ide,
+scsi,
+virtio };
+
+   if (busId = 0  busId  3)
+   return busnames[busId];
+   else
+   return 0;
+}
+
+static char *qemudDriveOpt(struct qemud_vm_disk_def *disk, int boot)
+{
+char opt[PATH_MAX];
+
+switch (disk-device) {
+case QEMUD_DISK_CDROM:
+snprintf(opt, PATH_MAX, file=%s,if=ide,media=cdrom%s,
+  disk-src, boot ? ,boot=on : );
+break;
+case QEMUD_DISK_FLOPPY:
+snprintf(opt, PATH_MAX, file=%s,if=floppy%s,
+  disk-src, boot ? ,boot=on : );
+break;
+case QEMUD_DISK_DISK:
+snprintf(opt, PATH_MAX, file=%s,if=%s%s,
+  disk-src, qemudBusIdToName(disk-bus), boot ? 
,boot=on : );
+break;
+default:
+return 0;
+}
+return strdup(opt);
+}
+
+static char *qemudAddBootDrive(virConnectPtr conn,
+struct qemud_vm_def *def,
+   char *handledDisks,
+   int type) {
+int j = 0;
+struct qemud_vm_disk_def *disk = def-disks;
+
+while (disk) {
+if (!handledDisks[j]  disk-device == type) {
+handledDisks[j] = 1;
+return qemudDriveOpt(disk, 1);
+}
+j++;
+disk = disk-next;
+}
+qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
+ Requested boot device type %d, but no such device 
defined., type);
+return 0;
+}
 
 /*
  * Parses a libvirt XML definition of a guest, and populates the
@@ -1739,7 +1821,6 @@
 obj = xmlXPathEval(BAD_CAST /domain/devices/disk, ctxt);
 if ((obj != NULL)  (obj-type == XPATH_NODESET) 
 (obj-nodesetval != NULL)  (obj-nodesetval-nodeNr = 0)) {
-struct qemud_vm_disk_def *prev = NULL;
 for (i = 0; i  obj-nodesetval-nodeNr; i++) {
 struct qemud_vm_disk_def *disk = calloc(1, sizeof(*disk));
 if (!disk) {
@@ -1752,13 +1833,20 @@
 goto error;
 }
 def-ndisks++;
-disk-next = NULL;
   

Re: [Libvir] [PATCH] Allow selection of the NIC model in QEMU/KVM

[Soren Hansen]

On Tue, Apr 08, 2008 at 02:57:15PM +0100, Daniel P. Berrange wrote:
 This patch seems incomplete - there's no code to include the model
 tag when dumping the XML.

FWIW, The patch I applied in Ubuntu to do this, looks like this. It
obviously still lack the test case updates and such, though.


Index: docs/libvir.html
===
--- docs/libvir.html.orig   2008-04-11 12:55:19.672613742 -0500
+++ docs/libvir.html2008-04-11 12:55:57.223628370 -0500
@@ -1041,6 +1041,14 @@
   lt;mac address=11:22:33:44:55:66/gt; 
 lt;/interfacegt;
 /pre
+pre
+lt;interface type='user'gt;
+  lt;mac address=11:22:33:44:55:66/gt;
+  lt;model type=ne2k_pci/gt;
+lt;/interfacegt;
+/pre
+p(where the network card model is one of those supported by
+  QEMU or KVM - see the relevant manual pages)./p
   /li
   liVirtual network
 pProvides a virtual network using a bridge device in the host.
Index: src/qemu_conf.c
===
--- src/qemu_conf.c.orig2008-04-11 12:55:19.692607157 -0500
+++ src/qemu_conf.c 2008-04-11 12:56:13.688628360 -0500
@@ -605,6 +605,7 @@
 xmlChar *script = NULL;
 xmlChar *address = NULL;
 xmlChar *port = NULL;
+xmlChar *model = NULL;
 
 net-type = QEMUD_NET_USER;
 
@@ -666,6 +667,8 @@
(net-type == QEMUD_NET_ETHERNET) 
xmlStrEqual(cur-name, BAD_CAST script)) {
 script = xmlGetProp(cur, BAD_CAST path);
+} else if (xmlStrEqual (cur-name, BAD_CAST model)) {
+model = xmlGetProp (cur, BAD_CAST type);
 }
 }
 cur = cur-next;
@@ -823,6 +826,38 @@
 xmlFree(address);
 }
 
+/* NIC model (see -net nic,model=?).  We only check that it looks
+ * reasonable, not that it is a supported NIC type.  FWIW kvm
+ * supports these types as of April 2008:
+ * i82551 i82557b i82559er ne2k_pci pcnet rtl8139 e1000 virtio
+ */
+if (model != NULL) {
+int i, len, char_ok;
+
+len = xmlStrlen (model);
+if (len = QEMUD_MODEL_MAX_LEN) {
+qemudReportError (conn, NULL, NULL, VIR_ERR_INVALID_ARG,
+  _(Model name '%s' is too long), model);
+goto error;
+}
+for (i = 0; i  len; ++i) {
+char_ok =
+(model[i] = '0'  model[i] = '9') ||
+(model[i] = 'a'  model[i] = 'z') ||
+(model[i] = 'A'  model[i] = 'Z') || model[i] == '_';
+if (!char_ok) {
+qemudReportError (conn, NULL, NULL, VIR_ERR_INVALID_ARG,
+  _(Model name contains invalid characters));
+goto error;
+}
+}
+strncpy (net-model, BAD_CAST model, len);
+net-model[len] = '\0';
+
+xmlFree (model);
+} else
+net-model[0] = '\0';
+
 return 0;
 
  error:
@@ -838,6 +873,8 @@
 xmlFree(script);
 if (bridge)
 xmlFree(bridge);
+if (model)
+xmlFree(model);
 return -1;
 }
 
@@ -1678,13 +1715,22 @@
 } else {
 int vlan = 0;
 while (net) {
+char model[100];
 char nic[100];
 
-if (snprintf(nic, sizeof(nic), 
nic,macaddr=%02x:%02x:%02x:%02x:%02x:%02x,vlan=%d,
+if (net-model[0] != '\0') {
+if (snprintf (model, sizeof (model), ,model=%s, net-model)
+= sizeof (model))
+goto error;
+} else
+model[0] = '\0';
+
+if (snprintf(nic, sizeof(nic),
+ nic,macaddr=%02x:%02x:%02x:%02x:%02x:%02x,vlan=%d%s,
  net-mac[0], net-mac[1],
  net-mac[2], net-mac[3],
  net-mac[4], net-mac[5],
- vlan) = sizeof(nic))
+ vlan, model) = sizeof(nic))
 goto error;
 
 if (!((*argv)[++n] = strdup(-net)))
@@ -2881,6 +2927,11 @@
 goto no_memory;
 }
 }
+if (net-model  net-model[0] != '\0') {
+if (virBufferVSprintf(buf,   model type='%s'/\n,
+  net-model)  0)
+goto no_memory;
+}
 
 if (virBufferVSprintf(buf, /interface\n)  0)
 goto no_memory;
Index: src/qemu_conf.h
===
--- src/qemu_conf.h.orig2008-04-11 12:55:19.716629596 -0500
+++ src/qemu_conf.h 2008-04-11 12:55:57.351638875 -0500
@@ -67,6 +67,7 @@
 };
 
 #define QEMUD_MAC_ADDRESS_LEN 6
+#define QEMUD_MODEL_MAX_LEN 10
 #define QEMUD_OS_TYPE_MAX_LEN 10
 #define QEMUD_OS_ARCH_MAX_LEN 10
 #define QEMUD_OS_MACHINE_MAX_LEN 10
@@ -90,6 +91,7 @@
 struct qemud_vm_net_def {
 int type;
 

Re: [Libvir] [PATCH] Implement SetVcpus and DomainGetMaxVcpus for qemu

[Richard W.M. Jones]

On Mon, Apr 28, 2008 at 01:46:28PM -0400, Cole Robinson wrote:
[...]

Fine except:

 -if (!strcmp(type, qemu))
 +if (!strcasecmp(type, qemu))
  return 16;
  
  /* XXX future KVM will support SMP. Need to probe
 kernel to figure out KVM module version i guess */
 -if (!strcmp(type, kvm))
 +if (!strcasecmp(type, kvm))
  return 1;
  
 -if (!strcmp(type, kqemu))
 +if (!strcasecmp(type, kqemu))
  return 1;

Can you change these equality tests to use the STR* macros from
src/internal.h before committing.

Thanks,

Rich.

-- 
Richard Jones, Emerging Technologies, Red Hat  http://et.redhat.com/~rjones
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into Xen guests.
http://et.redhat.com/~rjones/virt-p2v

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

Re: [Libvir] [PATCH] Ancient libparted

[Richard W.M. Jones]

On Tue, Apr 29, 2008 at 09:54:04AM +0200, Soren Hansen wrote:
 Some of us are stuck with an ancient libparted, which doesn't know about
 PED_PARTITION_PROTECTED. This patch allows us to compile libvirt.
 
 
 === modified file 'src/parthelper.c'
 --- src/parthelper.c  2008-04-10 16:53:29 +
 +++ src/parthelper.c  2008-04-29 07:47:08 +
 @@ -67,8 +67,10 @@
  content = free;
  else if (part-type  PED_PARTITION_METADATA)
  content = metadata;
 +#ifdef PED_PARTITION_PROTECTED
  else if (part-type  PED_PARTITION_PROTECTED)
  content = protected;
 +#endif
  else
  content = data;
  } else if (part-type == PED_PARTITION_EXTENDED) {
 @@ -80,8 +82,10 @@
  content = free;
  else if (part-type  PED_PARTITION_METADATA)
  content = metadata;
 +#ifdef PED_PARTITION_PROTECTED
  else if (part-type  PED_PARTITION_PROTECTED)
  content = protected;
 +#endif
  else
  content = data;
  }

Yup, I'll apply this.

Rich.

-- 
Richard Jones, Emerging Technologies, Red Hat  http://et.redhat.com/~rjones
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into Xen guests.
http://et.redhat.com/~rjones/virt-p2v

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

Re: [Libvir] [PATCH] Implement SetVcpus and DomainGetMaxVcpus for qemu

[Jim Meyering]

Cole Robinson [EMAIL PROTECTED] wrote:
 The attached patch fills in two of the vcpu functions for the qemu driver:

 virDomainSetVcpus : set the number of vcpus the domain can use
 virDomainGetMaxVcpus : max number of vcpus that can be assigned to the domain.

 Code change is only in qemu_driver, as the backend stuff was already in place.
 I also edited qemudGetMaxVcpus to ignore case when checking the passed OS
 type, since it wasn't matching the returned results of qemudDomainGetOSType.

Hi Cole,
This looks fine, modulo a couple nits:
...
 diff --git a/src/qemu_driver.c b/src/qemu_driver.c
...
 +static int qemudDomainSetVcpus(virDomainPtr dom, unsigned int nvcpus) {
 +struct qemud_driver *driver = (struct qemud_driver 
 *)dom-conn-privateData;

Since it doesn't affect an interface, it's less important,
but I think driver can be a const pointer.

 +struct qemud_vm *vm = qemudFindVMByUUID(driver, dom-uuid);
 +int max;
 +
 +if (!vm) {
 +qemudReportError(dom-conn, dom, NULL, VIR_ERR_INVALID_DOMAIN,
 + _(no domain with matching uuid '%s'), dom-uuid);
 +return -1;
 +}
 +
 +if (qemudIsActiveVM(vm)) {
 +qemudReportError(dom-conn, dom, NULL, VIR_ERR_INTERNAL_ERROR, %s,
 + _(cannot change vcpu count of an active domain));
 +return -1;
 +}
 +
 +if ((max = qemudDomainGetMaxVcpus(dom))  0) {
 +qemudReportError(dom-conn, dom, NULL, VIR_ERR_INTERNAL_ERROR,

Please insert a %s argument here, like above, since
the message contains no %-directive.  This will avoid a
compile-time warning.

 + _(could not determine max vcpus for the domain));
 +return -1;
 +}
 +
 +if (nvcpus  max) {
 +qemudReportError(dom-conn, dom, NULL, VIR_ERR_INVALID_ARG,
 + _(requested vcpus is greater than max allowable
 +vcpus for the domain: %d  %d), nvcpus, max);
 +return -1;
 +}
 +
 +vm-def-vcpus = nvcpus;
 +return 0;
 +}

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

Re: [Libvir] [PATCH] Ancient libparted

[Jim Meyering]

Richard W.M. Jones [EMAIL PROTECTED] wrote:
 On Tue, Apr 29, 2008 at 09:54:04AM +0200, Soren Hansen wrote:
 Some of us are stuck with an ancient libparted, which doesn't know about
 PED_PARTITION_PROTECTED. This patch allows us to compile libvirt.
 === modified file 'src/parthelper.c'
 --- src/parthelper.c 2008-04-10 16:53:29 +
 +++ src/parthelper.c 2008-04-29 07:47:08 +
 @@ -67,8 +67,10 @@
  content = free;
  else if (part-type  PED_PARTITION_METADATA)
  content = metadata;
 +#ifdef PED_PARTITION_PROTECTED
  else if (part-type  PED_PARTITION_PROTECTED)
  content = protected;
 +#endif
  else
  content = data;
  } else if (part-type == PED_PARTITION_EXTENDED) {
 @@ -80,8 +82,10 @@
  content = free;
  else if (part-type  PED_PARTITION_METADATA)
  content = metadata;
 +#ifdef PED_PARTITION_PROTECTED
  else if (part-type  PED_PARTITION_PROTECTED)
  content = protected;
 +#endif
  else
  content = data;
  }

 Yup, I'll apply this.

Hi guys,

We prefer to avoid in-function #if directives, when possible,
so I'll apply this on top of what's now in cvs:

Avoid in-function #if directives.
* src/parthelper.c [!PED_PARTITION_PROTECTED]: Define to 0.
Remove in-function #ifdefs.

diff --git a/src/parthelper.c b/src/parthelper.c
index a0fe241..1cd7240 100644
--- a/src/parthelper.c
+++ b/src/parthelper.c
@@ -35,6 +35,12 @@
 #include parted/parted.h
 #include stdio.h

+/* Make the comparisons below fail if your parted headers
+   are so old that they lack the definition.  */
+#ifndef PED_PARTITION_PROTECTED
+# define PED_PARTITION_PROTECTED 0
+#endif
+
 int main(int argc, char **argv)
 {
 PedDevice *dev;
@@ -67,10 +73,8 @@ int main(int argc, char **argv)
 content = free;
 else if (part-type  PED_PARTITION_METADATA)
 content = metadata;
-#ifdef PED_PARTITION_PROTECTED
 else if (part-type  PED_PARTITION_PROTECTED)
 content = protected;
-#endif
 else
 content = data;
 } else if (part-type == PED_PARTITION_EXTENDED) {
@@ -82,10 +86,8 @@ int main(int argc, char **argv)
 content = free;
 else if (part-type  PED_PARTITION_METADATA)
 content = metadata;
-#ifdef PED_PARTITION_PROTECTED
 else if (part-type  PED_PARTITION_PROTECTED)
 content = protected;
-#endif
 else
 content = data;
 }
--
1.5.5.1.68.gbdcd8

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

Re: [Libvir] [PATCH] Ancient libparted

[Daniel P. Berrange]

On Tue, Apr 29, 2008 at 02:32:08PM +0200, Jim Meyering wrote:
 Richard W.M. Jones [EMAIL PROTECTED] wrote:
  On Tue, Apr 29, 2008 at 09:54:04AM +0200, Soren Hansen wrote:
  Some of us are stuck with an ancient libparted, which doesn't know about
  PED_PARTITION_PROTECTED. This patch allows us to compile libvirt.
  === modified file 'src/parthelper.c'
  --- src/parthelper.c   2008-04-10 16:53:29 +
  +++ src/parthelper.c   2008-04-29 07:47:08 +
  @@ -67,8 +67,10 @@
   content = free;
   else if (part-type  PED_PARTITION_METADATA)
   content = metadata;
  +#ifdef PED_PARTITION_PROTECTED
   else if (part-type  PED_PARTITION_PROTECTED)
   content = protected;
  +#endif
   else
   content = data;
   } else if (part-type == PED_PARTITION_EXTENDED) {
  @@ -80,8 +82,10 @@
   content = free;
   else if (part-type  PED_PARTITION_METADATA)
   content = metadata;
  +#ifdef PED_PARTITION_PROTECTED
   else if (part-type  PED_PARTITION_PROTECTED)
   content = protected;
  +#endif
   else
   content = data;
   }
 
  Yup, I'll apply this.
 
 Hi guys,
 
 We prefer to avoid in-function #if directives, when possible,
 so I'll apply this on top of what's now in cvs:
 
   Avoid in-function #if directives.
   * src/parthelper.c [!PED_PARTITION_PROTECTED]: Define to 0.
   Remove in-function #ifdefs.

Seems reasonable to me

Dan.
-- 
|: Red Hat, Engineering, Boston   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

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

Re: [Libvir] [PATCH] Add bus attribute to disk target definition

[Daniel P. Berrange]

On Tue, Apr 29, 2008 at 09:56:13AM +0200, Soren Hansen wrote:
 Hi!
 
 I'd like to propose that the following patch gets applied against
 libvirt. It adds the option of putting a bus attribute on a disk target.
 To acommodate this, it also changes the way drives are defined for kvm
 from the old -hda /path/to/file -boot c style to the new -drive
 file=/path/to/file,if=ide,boot=on. This makes it possible to specify
 virtio, scsi, and ide disks.

 @@ -646,7 +648,9 @@
  strcmp((const char *)target, hda) 
  strcmp((const char *)target, hdb) 
  strcmp((const char *)target, hdc) 
 -strcmp((const char *)target, hdd)) {
 +strcmp((const char *)target, hdd) 
 +strcmp((const char *)target, hdd) 

These two lines test the same thing !

 +strncmp((const char *)target, vd, 2)) {

Its probably a better idea to just compress the explicit hda - hdd 
comparisons down into a single 'hd' prefix comparison, as you did
with 'vd'.


 +if (!bus)
 +disk-bus = QEMUD_DISK_BUS_IDE;
 +else if (!strcmp((const char *)bus, ide))
 +disk-bus = QEMUD_DISK_BUS_IDE;
 +else if (!strcmp((const char *)bus, scsi))
 +disk-bus = QEMUD_DISK_BUS_SCSI;
 +else if (!strcmp((const char *)bus, virtio))
 +disk-bus = QEMUD_DISK_BUS_VIRTIO;

Can you use the   STREQ   macro here instead of strcmp.

 +else {
 +qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, Invalid 
 bus type: %s, bus);
 +goto error;
 +}

This line needs to be marked for translation, with _(...). You can
run 'make syntax-check' for check for such issues.

 +static char *qemudAddBootDrive(virConnectPtr conn,
 +struct qemud_vm_def *def,
 + char *handledDisks,
 + int type) {
 +int j = 0;
 +struct qemud_vm_disk_def *disk = def-disks;
 +
 +while (disk) {
 +if (!handledDisks[j]  disk-device == type) {
 +handledDisks[j] = 1;
 +return qemudDriveOpt(disk, 1);
 +}
 +j++;
 +disk = disk-next;
 +}
 +qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
 + Requested boot device type %d, but no such device 
 defined., type);
 +return 0;
 +}
  

 @@ -2207,30 +2295,32 @@
  goto no_memory;
  }
  
 -for (i = 0 ; i  vm-def-os.nBootDevs ; i++) {
 -switch (vm-def-os.bootDevs[i]) {
 -case QEMUD_BOOT_CDROM:
 -boot[i] = 'd';
 -break;
 -case QEMUD_BOOT_FLOPPY:
 -boot[i] = 'a';
 -break;
 -case QEMUD_BOOT_DISK:
 -boot[i] = 'c';
 -break;
 -case QEMUD_BOOT_NET:
 -boot[i] = 'n';
 -break;
 -default:
 -boot[i] = 'c';
 -break;
 +if (vm-def-virtType != QEMUD_VIRT_KVM) {

This is wrong - both QEMU and KVM can support the -drive parameter,
and not all KVM support it. You need to add a check in the method
qemudExtractVersionInfo() to probe for availability of the -drive
option, as we do with other opts - see QEMUD_CMD_FLAG_NO_REBOOT
for example.

Even if the -drive parameter is supported, it should still pass the
-boot a/c/d/n  parameter in.

 +if (vm-def-virtType == QEMUD_VIRT_KVM) {
 +char *handledDisks = NULL;
 +int j;
 +
 +handledDisks = calloc(sizeof(*handledDisks), vm-def-ndisks);
 +
 +if (!handledDisks)
 +goto no_memory;
 +
 +/* When using -drive notation, we need to provide the devices in boot
 + * preference order. */
 +for (i = 0 ; i  vm-def-os.nBootDevs ; i++) {
 +if (!((*argv)[++n] = strdup(-drive)))
 +goto no_memory;
 +
 +switch (vm-def-os.bootDevs[i]) {
 +case QEMUD_BOOT_CDROM:
 +if (!((*argv)[++n] = qemudAddBootDrive(conn, vm-def, 
 handledDisks, QEMUD_DISK_CDROM)))
 +goto error;
 +break;
 + case QEMUD_BOOT_FLOPPY:
 +if (!((*argv)[++n] = qemudAddBootDrive(conn, vm-def, 
 handledDisks, QEMUD_DISK_FLOPPY)))
 +goto error;
 +break;
 +case QEMUD_BOOT_DISK:
 +if (!((*argv)[++n] = qemudAddBootDrive(conn, vm-def, 
 handledDisks, QEMUD_DISK_DISK)))
 +goto error;
 +break;
 +}
 +}

There is nothing in the -drive parameter handling, AFAICT, that requires
the boot drive to be listed first on the command line. So this first loop
is not needed, and this second loop is sufficient, simply turn on the
boot= flag on the first match drive type when iterating over the list.

 +
 +/* Pick up the rest of the devices */
 +j=0;
 +while (disk) {
 +if (!handledDisks[j]) {
 +handledDisks[j] = 1;
 +if 

Re: [Libvir] [PATCH] Avoid make syntax-check failures.

[Daniel P. Berrange]

On Tue, Apr 29, 2008 at 01:04:19PM +0200, Jim Meyering wrote:
 Including config.h from memory.c is probably required only on
 um, ... unusual systems, but technically, it is required for the
 definition of a possibly missing size_t or ptrdiff_t type.
 
   Avoid make syntax-check failures.
   * src/memory.c: Include config.h.
   Remove trailing blanks.

ACK, sorry for missing this.

Dan.
-- 
|: Red Hat, Engineering, Boston   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

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

Re: [Libvir] PATCH: Avoid buffer out of bounds access in Xen capabilities

[Daniel P. Berrange]

On Tue, Apr 29, 2008 at 02:46:14AM -0400, Daniel Veillard wrote:
 On Mon, Apr 28, 2008 at 11:42:47PM +0100, Daniel P. Berrange wrote:
  The Xen driver uses a regex to process the hypervisor capabilities data
  

  (xen|hvm)-[[:digit:]]+\\.[[:digit:]]+-(x86_32|x86_64|ia64|powerpc64)(p|be)?;
  
  notice how the last match group, however, is optional due to the '?'. The
  code processing matches does not check to see if the match is present or
  not, and just indexes the string on match 3
  
   if (strncmp (token[subs[3].rm_so], p, 1) == 0)
  
  Unfortunately,  subs[3].rm_so is -1 if the match was not present, so we're
  doing an out of bounds array access here. This is fairly harmless, but it
  is still good to fix it. So this patch adds a check for -1 before accessing
  the match. I also replace the strncmp() calls with a call to the brand new
  STRPREFIX() convenience macro
 
   Okidoc, i assume valgrind spotted that, that's fairly well hidden ...

Yeah, and valgrind only finds it on i386 - not x86_64, which is why I didn't
spot it for sooo long !

Dan.
-- 
|: Red Hat, Engineering, Boston   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

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

[Libvir] also check for useless test-before-xmlFree

[Jim Meyering]

Some time ago, Daniel Veillard assured me that xmlFree(NULL) is
valid in regular use, and with upstream code since January it's
ok even in a debug mode that's not normally available because
it's ifdef'd out:

  http://mail.gnome.org/archives/svn-commits-list/2008-January/msg02233.html

Since the potential NULL-deref in debug mode is now gone,
we can now use this to prevent any new useless tests:

also check for useless test-before-xmlFree
* Makefile.cfg (useless_free_options): Add --name=xmlFree.

diff --git a/Makefile.cfg b/Makefile.cfg
index e0d5528..cbf97c2 100644
--- a/Makefile.cfg
+++ b/Makefile.cfg
@@ -53,5 +53,6 @@ local-checks-to-skip =\

 useless_free_options = \
   --name=sexpr_free\
+  --name=xmlFree   \
   --name=xmlXPathFreeContext   \
   --name=xmlXPathFreeObject
--
1.5.5.1.68.gbdcd8

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

Re: [Libvir] [PATCH] Add bus attribute to disk target definition

[Soren Hansen]

On Tue, Apr 29, 2008 at 02:27:00PM +0100, Daniel P. Berrange wrote:
  +strcmp((const char *)target, hdd) 
  +strcmp((const char *)target, hdd) 
 
 These two lines test the same thing !

Quite so. My bad.

  +strncmp((const char *)target, vd, 2)) {
 
 Its probably a better idea to just compress the explicit hda - hdd 
 comparisons down into a single 'hd' prefix comparison, as you did
 with 'vd'.

Hm.. Yes, I suppose that if you're using -drive notation, hd[e-z]
starts making sense. Fixed.

 +if (!bus)
 +disk-bus = QEMUD_DISK_BUS_IDE;
 +else if (!strcmp((const char *)bus, ide))
 +disk-bus = QEMUD_DISK_BUS_IDE;
 +else if (!strcmp((const char *)bus, scsi))
 +disk-bus = QEMUD_DISK_BUS_SCSI;
 +else if (!strcmp((const char *)bus, virtio))
 +disk-bus = QEMUD_DISK_BUS_VIRTIO;
 Can you use the   STREQ   macro here instead of strcmp.

Erm... I *could*.. I'm curious, though, why e.g. the similar code right
above it doesn't use STREQ if that's the preferred way to do it?

IMO, it's better to change this sort of things all in one go, and keep
everything consistent until then.

 +else {
 +qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, Invalid 
 bus type: %s, bus);
 +goto error;
 +}
 
 This line needs to be marked for translation, with _(...). 

Fixed.

 You can run 'make syntax-check' for check for such issues.

Yes, in theory :)  In the real world, however, make syntax-check fails
horribly here. I'll be fixing that up next.

  +static char *qemudAddBootDrive(virConnectPtr conn,
  +struct qemud_vm_def *def,
  +   char *handledDisks,
  +   int type) {
  +int j = 0;
  +struct qemud_vm_disk_def *disk = def-disks;
  +
  +while (disk) {
  +if (!handledDisks[j]  disk-device == type) {
  +handledDisks[j] = 1;
  +return qemudDriveOpt(disk, 1);
  +}
  +j++;
  +disk = disk-next;
  +}
  +qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
  + Requested boot device type %d, but no such 
  device defined., type);
  +return 0;
  +}
   
 
  @@ -2207,30 +2295,32 @@
   goto no_memory;
   }
   
  -for (i = 0 ; i  vm-def-os.nBootDevs ; i++) {
  -switch (vm-def-os.bootDevs[i]) {
  -case QEMUD_BOOT_CDROM:
  -boot[i] = 'd';
  -break;
  -case QEMUD_BOOT_FLOPPY:
  -boot[i] = 'a';
  -break;
  -case QEMUD_BOOT_DISK:
  -boot[i] = 'c';
  -break;
  -case QEMUD_BOOT_NET:
  -boot[i] = 'n';
  -break;
  -default:
  -boot[i] = 'c';
  -break;
  +if (vm-def-virtType != QEMUD_VIRT_KVM) {
 
 This is wrong - both QEMU and KVM can support the -drive parameter,

Oh, I wasn't aware. Hmm..

 and not all KVM support it. You need to add a check in the method
 qemudExtractVersionInfo() to probe for availability of the -drive
 option, as we do with other opts - see QEMUD_CMD_FLAG_NO_REBOOT for
 example.

Interesting. I'll add that.

 Even if the -drive parameter is supported, it should still pass the
 -boot a/c/d/n  parameter in.

Why? And how would you boot from a virtio device this way?

 There is nothing in the -drive parameter handling, AFAICT, that
 requires the boot drive to be listed first on the command line. So
 this first loop is not needed, and this second loop is sufficient,
 simply turn on the boot= flag on the first match drive type when
 iterating over the list.

If you want to specify more than one boot device, the only way to
determine the priority is by specifying them ordered by priority. At
least, that's how it *ought* to be, but now I see that extboot only
actually supports one boot device. :/ I could have sworn I had seen it
work with both hard drive and cdrom. 

 +int virDiskNameToIndex(const char *name) {
 +const char *ptr = NULL;
 +int idx = 0;
 +
 +if (strlen(name)  3)
 +return 0;
 +
 +switch (*name) {
 +case 'f':
 +case 'h':
 +case 'v':
 You need a case 's' there to allow for SCSI...

Good point. I suppose I do so in qemudParseDiskXml as well (when
checking the target).

-- 
Soren Hansen   | 
Virtualisation specialist  | Ubuntu Server Team
Canonical Ltd. | http://www.ubuntu.com/


signature.asc
Description: Digital signature
--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [Libvir] [PATCH] Add bus attribute to disk target definition

[Daniel P. Berrange]

On Tue, Apr 29, 2008 at 04:10:40PM +0200, Soren Hansen wrote:
 On Tue, Apr 29, 2008 at 02:27:00PM +0100, Daniel P. Berrange wrote:
  +if (!bus)
  +disk-bus = QEMUD_DISK_BUS_IDE;
  +else if (!strcmp((const char *)bus, ide))
  +disk-bus = QEMUD_DISK_BUS_IDE;
  +else if (!strcmp((const char *)bus, scsi))
  +disk-bus = QEMUD_DISK_BUS_SCSI;
  +else if (!strcmp((const char *)bus, virtio))
  +disk-bus = QEMUD_DISK_BUS_VIRTIO;
  Can you use the   STREQ   macro here instead of strcmp.
 
 Erm... I *could*.. I'm curious, though, why e.g. the similar code right
 above it doesn't use STREQ if that's the preferred way to do it?

We've been slowly updating code to match these new standards when doing
patches.

  You can run 'make syntax-check' for check for such issues.
 
 Yes, in theory :)  In the real world, however, make syntax-check fails
 horribly here. I'll be fixing that up next.

If you send details to the list,  Jim will no doubt be able to point you
in the right direction on this...

  Even if the -drive parameter is supported, it should still pass the
  -boot a/c/d/n  parameter in.
 
 Why? And how would you boot from a virtio device this way?

It is needed for PXE boot at least, and IMHO, QEMU should treat 'boot c'
as if  'boot=on' were set for the first -drive parameter for back compat.


  There is nothing in the -drive parameter handling, AFAICT, that
  requires the boot drive to be listed first on the command line. So
  this first loop is not needed, and this second loop is sufficient,
  simply turn on the boot= flag on the first match drive type when
  iterating over the list.
 
 If you want to specify more than one boot device, the only way to
 determine the priority is by specifying them ordered by priority. At
 least, that's how it *ought* to be, but now I see that extboot only
 actually supports one boot device. :/ I could have sworn I had seen it
 work with both hard drive and cdrom. 

The QEMU code only allows a single boot device  will abort if  1 has it

if (extboot_drive != -1) {
fprintf(stderr, qemu: two bootable drives specified\n);
return -1;
}

Regards,
Dan.
-- 
|: Red Hat, Engineering, Boston   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

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

Re: [Libvir] [PATCH] Add bus attribute to disk target definition

[Jim Meyering]

Daniel P. Berrange [EMAIL PROTECTED] wrote:

 On Tue, Apr 29, 2008 at 04:10:40PM +0200, Soren Hansen wrote:
 On Tue, Apr 29, 2008 at 02:27:00PM +0100, Daniel P. Berrange wrote:
  +if (!bus)
  +disk-bus = QEMUD_DISK_BUS_IDE;
  +else if (!strcmp((const char *)bus, ide))
  +disk-bus = QEMUD_DISK_BUS_IDE;
  +else if (!strcmp((const char *)bus, scsi))
  +disk-bus = QEMUD_DISK_BUS_SCSI;
  +else if (!strcmp((const char *)bus, virtio))
  +disk-bus = QEMUD_DISK_BUS_VIRTIO;
  Can you use the   STREQ   macro here instead of strcmp.

 Erm... I *could*.. I'm curious, though, why e.g. the similar code right
 above it doesn't use STREQ if that's the preferred way to do it?

 We've been slowly updating code to match these new standards when doing
 patches.

FYI, there's already a test for this, but it's currently disabled.
If someone wants to turn it on (remove the sc_prohibit_strcmp
line from Makefile.cfg) and fix all the new violations exposed
by running make syntax-check, it sounds like it would be welcome, now.

  You can run 'make syntax-check' for check for such issues.

 Yes, in theory :)  In the real world, however, make syntax-check fails
 horribly here. I'll be fixing that up next.

 If you send details to the list,  Jim will no doubt be able to point you
 in the right direction on this...

Yep.  Though most of it is intended to be self explanatory.
If not, please let me know and I'll fix the rules in Makefile.maint.

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

Re: [Libvir] [PATCH] Add bus attribute to disk target definition

[Jim Meyering]

Jim Meyering [EMAIL PROTECTED] wrote:

 Daniel P. Berrange [EMAIL PROTECTED] wrote:

 On Tue, Apr 29, 2008 at 04:10:40PM +0200, Soren Hansen wrote:
 On Tue, Apr 29, 2008 at 02:27:00PM +0100, Daniel P. Berrange wrote:
  +if (!bus)
  +disk-bus = QEMUD_DISK_BUS_IDE;
  +else if (!strcmp((const char *)bus, ide))
  +disk-bus = QEMUD_DISK_BUS_IDE;
  +else if (!strcmp((const char *)bus, scsi))
  +disk-bus = QEMUD_DISK_BUS_SCSI;
  +else if (!strcmp((const char *)bus, virtio))
  +disk-bus = QEMUD_DISK_BUS_VIRTIO;
  Can you use the   STREQ   macro here instead of strcmp.

 Erm... I *could*.. I'm curious, though, why e.g. the similar code right
 above it doesn't use STREQ if that's the preferred way to do it?

 We've been slowly updating code to match these new standards when doing
 patches.

 FYI, there's already a test for this, but it's currently disabled.
 If someone wants to turn it on (remove the sc_prohibit_strcmp
 line from Makefile.cfg) and fix all the new violations exposed
 by running make syntax-check, it sounds like it would be welcome, now.

In preparation for this, I've just relaxed the regexp in the check
to allow for libvirt's varying coding styles (space or not between
function name and opening parenthesis):

tests: recognize more uses of strcmp.
* Makefile.maint (sc_prohibit_strcmp): Relax regexp.

=
Index: Makefile.maint
===
RCS file: /data/cvs/libvirt/Makefile.maint,v
retrieving revision 1.21
retrieving revision 1.22
diff -u -p -u -r1.21 -r1.22
--- Makefile.maint  21 Apr 2008 10:09:07 -  1.21
+++ Makefile.maint  29 Apr 2008 14:25:19 -  1.22
@@ -91,7 +91,7 @@ sc_prohibit_atoi_atof:

 # Use STREQ rather than comparing strcmp == 0, or != 0.
 sc_prohibit_strcmp:
-   @grep -nE '! *str''cmp \(|\str''cmp \([^)]+\) *==' \
+   @grep -nE '! *str''cmp *\(|\str''cmp *\([^)]+\) *=='   \
$$($(VC_LIST_EXCEPT)) \
  { echo '$(ME): use STREQ in place of the above uses of str''cmp' \
12; exit 1; } || :

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

Re: [Libvir] [PATCH] Add bus attribute to disk target definition

[Soren Hansen]

On Tue, Apr 29, 2008 at 03:17:14PM +0100, Daniel P. Berrange wrote:
 +if (!bus)
 +disk-bus = QEMUD_DISK_BUS_IDE;
 +else if (!strcmp((const char *)bus, ide))
 +disk-bus = QEMUD_DISK_BUS_IDE;
 +else if (!strcmp((const char *)bus, scsi))
 +disk-bus = QEMUD_DISK_BUS_SCSI;
 +else if (!strcmp((const char *)bus, virtio))
 +disk-bus = QEMUD_DISK_BUS_VIRTIO;
 Can you use the   STREQ   macro here instead of strcmp.
 Erm... I *could*.. I'm curious, though, why e.g. the similar code right
 above it doesn't use STREQ if that's the preferred way to do it?
 We've been slowly updating code to match these new standards when doing
 patches.

Well, if that's the way you do it, I'll follow suit..  However, I have
to say that I pity the person that reads the code and finds these two
sections of code that seem to do rather similar things, but use
different functions to do it, and then has to work out what on earth the
difference between the two might be.

 You can run 'make syntax-check' for check for such issues.
 Yes, in theory :)  In the real world, however, make syntax-check
 fails horribly here. I'll be fixing that up next.
 If you send details to the list,  Jim will no doubt be able to point
 you in the right direction on this...

I'll do that in a minute. Thanks.

 Even if the -drive parameter is supported, it should still pass the
 -boot a/c/d/n  parameter in.
 Why? And how would you boot from a virtio device this way?
 It is needed for PXE boot at least, and IMHO,

Good point.

 QEMU should treat 'boot c' as if  'boot=on' were set for the first
 -drive parameter for back compat.

Yes, indeed it should. Sadly, though, it doesn't.

  kvm -drive file=root.qcow,if=virtio -boot c

fails miserably, while 

  kvm -drive file=root.qcow,if=virtio,boot=on

works beautifully.

This logic is going to look horrible :( Something like:

if boot == hd  (one of the disks is a virtio disk)
then
use new style -drive foo,boot=on notation
else
use old style -boot [acdn] notation

?

 There is nothing in the -drive parameter handling, AFAICT, that
 requires the boot drive to be listed first on the command line. So
 this first loop is not needed, and this second loop is sufficient,
 simply turn on the boot= flag on the first match drive type when
 iterating over the list.
 If you want to specify more than one boot device, the only way to
 determine the priority is by specifying them ordered by priority. At
 least, that's how it *ought* to be, but now I see that extboot only
 actually supports one boot device. :/ I could have sworn I had seen
 it work with both hard drive and cdrom. 
 The QEMU code only allows a single boot device  will abort if  1 has
 it
 
 if (extboot_drive != -1) {
 fprintf(stderr, qemu: two bootable drives specified\n);
 return -1;
 }

Yes, that's what I noticed earlier today, although I geniunely hope this
is something that will be fixed at some point, and when that happy day
comes, I've either guessed correctly as to how it will derive boot
priorities and we won't have to fix anything, or I've guessed wrong, and
then we'll be no worse off than if we didn't adopt this approach right
now, AFAICS.

-- 
Soren Hansen   | 
Virtualisation specialist  | Ubuntu Server Team
Canonical Ltd. | http://www.ubuntu.com/


signature.asc
Description: Digital signature
--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[Libvir] Basic VMWare support

[Marek 'marx' Grac]

Hi,

I know that there is no support for VMWare ESX yet but I would like to 
know if someone is working on it. I need just three basic operations: 
status/power on/power off but I never saw the source code of libvirt :) 
Do you think it is possible to write this support (using VMWare API) in 
acceptable time? week? month?


   Thanks, for any suggestions

m,

--
Marek Grac
Red Hat Czech s.r.o.

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

Re: [Libvir] [PATCH] Add bus attribute to disk target definition

[Daniel P. Berrange]

On Tue, Apr 29, 2008 at 04:42:54PM +0200, Soren Hansen wrote:
 On Tue, Apr 29, 2008 at 03:17:14PM +0100, Daniel P. Berrange wrote:
  +if (!bus)
  +disk-bus = QEMUD_DISK_BUS_IDE;
  +else if (!strcmp((const char *)bus, ide))
  +disk-bus = QEMUD_DISK_BUS_IDE;
  +else if (!strcmp((const char *)bus, scsi))
  +disk-bus = QEMUD_DISK_BUS_SCSI;
  +else if (!strcmp((const char *)bus, virtio))
  +disk-bus = QEMUD_DISK_BUS_VIRTIO;
  Can you use the   STREQ   macro here instead of strcmp.
  Erm... I *could*.. I'm curious, though, why e.g. the similar code right
  above it doesn't use STREQ if that's the preferred way to do it?
  We've been slowly updating code to match these new standards when doing
  patches.
 
 Well, if that's the way you do it, I'll follow suit..  However, I have
 to say that I pity the person that reads the code and finds these two
 sections of code that seem to do rather similar things, but use
 different functions to do it, and then has to work out what on earth the
 difference between the two might be.


Here is an update to the HACKING file intended to describe some of the 
conventions in use...



Dan

Index: HACKING
===
RCS file: /data/cvs/libvirt/HACKING,v
retrieving revision 1.1
diff -r1.1 HACKING
45a46,160
 
 
 Low level memory management
 ===
 
 Use of the malloc/free/realloc/calloc APIs is deprecated in the libvirt
 codebase, because they encourage a number of serious coding bugs and do
 not enable compile time verification of checks for NULL. Instead of these
 routines, use the macros from memory.h
 
   - eg to allocate  a single object:
 
   virDomainPtr domain;
 
   if (VIR_ALLOC(domain)  0) {
  __virRaiseError(VIR_ERROR_NO_MEMORY)
  return NULL;
   }
 
 
   - eg to allocate an array of objects
 
virDomainPtr domains;
int ndomains = 10;
 
if (VIR_ALLOC_N(domains, ndomains)  0) {
  __virRaiseError(VIR_ERROR_NO_MEMORY)
  return NULL;
}
 
   - eg to allocate an array of object pointers
 
virDomainPtr *domains;
int ndomains = 10;
 
if (VIR_ALLOC_N(domains, ndomains)  0) {
  __virRaiseError(VIR_ERROR_NO_MEMORY)
  return NULL;
}
 
- eg to re-allocate the array of domains to be longer
 
ndomains = 20
 
if (VIR_REALLOC_N(domains, ndomains)  0) {
  __virRaiseError(VIR_ERROR_NO_MEMORY)
  return NULL;
}
 
- eg to free the domain
 
VIR_FREE(domain);
 
 
 
 String comparisons
 ==
 
 Do not use the strcmp, strncmp, etc functions directly. Instead use
 one of the following semantically named macros
 
   - For strict equality:
 
  STREQ(a,b)
  STRNEQ(a,b)
 
   - For case sensitive equality:
  STRCASEEQ(a,b)
  STRCASENEQ(a,b)
 
   - For strict equality of a substring:
 
  STREQLEN(a,b,n)
  STRNEQLEN(a,b,n)
 
   - For case sensitive equality of a substring:
 
  STRCASEEQLEN(a,b,n)
  STRCASENEQLEN(a,b,n)
 
   - For strict equality of a prefix:
 
  STRPREFIX(a,b)
 
 
 
 Variable length string buffer
 =
 
 If there is a need for complex string concatenations, avoid using
 the usual sequence of malloc/strcpy/strcat/snprintf functions and
 make use of the virBuffer API described in buf.h
 
 eg typical usage is as follows:
 
   char *
   somefunction(...) {
  virBuffer buf = VIR_BUFFER_INITIALIZER;
 
  ...
 
  virBufferAdd(buf, domain\n);
  virBufferVSprint(buf,   memory%d/memory\n, memory);
  ...
  virBufferAdd(buf, /domain\n);
 
  
 
  if (virBufferError(buf)) {
  __virRaiseError(...);
  return NULL;
  }
 
  return virBufferContentAndReset(buf);
   }


-- 
|: Red Hat, Engineering, Boston   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

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

Re: [Libvir] also check for useless test-before-xmlFree

[Daniel Veillard]

On Tue, Apr 29, 2008 at 04:07:06PM +0200, Jim Meyering wrote:
 Some time ago, Daniel Veillard assured me that xmlFree(NULL) is
 valid in regular use, and with upstream code since January it's

  Yup it points to free() from libc, unless redefined by the
user, but I don't expect that in an application using libvirt.

  +1

Daniel

-- 
Red Hat Virtualization group http://redhat.com/virtualization/
Daniel Veillard  | virtualization library  http://libvirt.org/
[EMAIL PROTECTED]  | libxml GNOME XML XSLT toolkit  http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine  http://rpmfind.net/

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

[Libvir] make syntax-check fails with bzr checkouts

[Soren Hansen]

I seem to be completely unable to get make syntax-checks to function
properly with my bzr checkout of libvirt[1]. I've attached the output as
as-is.txt.  I tried adding hacking bzr support into vc-list-files (see
vc-list-files-bzr.patch), but that didn't quite seem to do the trick, as
you can see in in vc-list-files-maybe-fixed.log, which is the output
after I patched vc-list-files.

I tried CVS, too, and that also fails (see cvs-syntax-check.log).

Is it only meant to work with git?  
  
[1]: http://bazaar.launchpad.net/~vcs-imports/libvirt/trunk

-- 
Soren Hansen   | 
Virtualisation specialist  | Ubuntu Server Team
Canonical Ltd. | http://www.ubuntu.com/
if test -f po/POTFILES.in; then \
  grep -E -v '^(#|$)' po/POTFILES.in\
| grep -v '^src/false\.c$' | sort  po-check-1; 
\
  files=;   \
  for file in $(build-aux/vc-list-files | if test -f .x-po-check; then 
grep -vEf .x-po-check; else grep -v ChangeLog; fi); do  
 \
  echo $file; \
case $file in   \
djgpp/* | man/*) continue;; \
*/c99-to-c89.diff) continue;;   \
esac;   \
case $file in   \
*.[ch]) \
  base=`expr  $file : ' \(.*\)\..'`;  \
  { test -f $base.l || test -f $base.y; }  continue;; \
*) continue;;   \
esac;   \
files=$files $file;   \
  done; \
  grep -E -l '\b(N?_|gettext *)\([^)]*(|$)' $files\
| sort -u  po-check-2; 
\
  diff -u po-check-1 po-check-2 || exit 1;  
\
  rm -f po-check-1 po-check-2;  
\
fi
build-aux/vc-list-files: Failed to determine type of version control used in 
/home/soren/src/projects/Virtualisation/libvirt/disk-bus
make: *** [po-check] Interrupt
=== modified file 'build-aux/vc-list-files'
--- build-aux/vc-list-files	2008-02-01 19:47:07 +
+++ build-aux/vc-list-files	2008-04-29 14:59:20 +
@@ -39,6 +39,13 @@
   exec git ls-files $dir
 elif test -d .hg; then
   exec hg locate $dir/*
+elif test -d .bzr; then
+  if [ $dir = . ]
+  then
+exec bzr ls
+  else
+exec bzr ls $dir
+  fi
 elif test -d CVS; then
   if test -x build-aux/cvsu; then
 build-aux/cvsu --find --types=AFGM $dir

if test -f po/POTFILES.in; then \
  grep -E -v '^(#|$)' po/POTFILES.in\
| grep -v '^src/false\.c$' | sort  po-check-1; 
\
  files=;   \
  for file in $(build-aux/vc-list-files | if test -f .x-po-check; then 
grep -vEf .x-po-check; else grep -v ChangeLog; fi); do  
 \
  echo $file; \
case $file in   \
djgpp/* | man/*) continue;; \
*/c99-to-c89.diff) continue;;   \
esac;   \
case $file in   \
*.[ch]) \
  base=`expr  $file : ' \(.*\)\..'`;  \
  { test -f $base.l || test -f $base.y; }  continue;; \
*) continue;;   \
esac;   \
files=$files $file;   \
  done; \
  grep -E -l '\b(N?_|gettext *)\([^)]*(|$)' $files\
| sort -u  po-check-2; 
\
  diff -u po-check-1 po-check-2 || exit 1;  
\
  rm -f po-check-1 po-check-2;  
\
fi
.cvsignore
.shelf
.x-sc_avoid_if_before_free
.x-sc_avoid_write
.x-sc_no_have_config_h
.x-sc_require_config_h
.x-sc_trailing_blank
ABOUT-NLS
AUTHORS
COPYING
COPYING.LIB
GNUmakefile
HACKING
INSTALL
Makefile
Makefile.am
Makefile.cfg
Makefile.in
Makefile.maint
NEWS
README
RENAMES
TODO
acinclude.m4
aclocal.m4

Re: [Libvir] Basic VMWare support

[Daniel Veillard]

On Mon, Apr 28, 2008 at 03:24:00PM +0200, Marek 'marx' Grac wrote:
 Hi,
 
 I know that there is no support for VMWare ESX yet but I would like to 
 know if someone is working on it. I need just three basic operations: 
 status/power on/power off but I never saw the source code of libvirt :) 
 Do you think it is possible to write this support (using VMWare API) in 
 acceptable time? week? month?

  I have been looking at VMWare APIs a bit in the last weeks.
There is no support right now. I am tempted to access VMWare by
using the WEB service API (not the VIX one, I'm not sure it
would be able to connect remotely to a standard ESX). 
  The API is a bit complex (I'm certainly biased though)
and I'm not yet able to list to resources from C code. But
once you have a handle to the right VM it looks relatively easy
to get status and power on/off assuming the rights to do so.

Daniel

-- 
Red Hat Virtualization group http://redhat.com/virtualization/
Daniel Veillard  | virtualization library  http://libvirt.org/
[EMAIL PROTECTED]  | libxml GNOME XML XSLT toolkit  http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine  http://rpmfind.net/

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

Re: [Libvir] make syntax-check fails with bzr checkouts

[Daniel Veillard]

On Tue, Apr 29, 2008 at 05:11:59PM +0200, Soren Hansen wrote:
 I tried CVS, too, and that also fails (see cvs-syntax-check.log).

  weird

 Is it only meant to work with git?  

  Really no, I only use CVS checkouts...

Daniel

-- 
Red Hat Virtualization group http://redhat.com/virtualization/
Daniel Veillard  | virtualization library  http://libvirt.org/
[EMAIL PROTECTED]  | libxml GNOME XML XSLT toolkit  http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine  http://rpmfind.net/

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

Re: [Libvir] make syntax-check fails with bzr checkouts

[Daniel P. Berrange]

On Tue, Apr 29, 2008 at 05:11:59PM +0200, Soren Hansen wrote:
 I seem to be completely unable to get make syntax-checks to function
 properly with my bzr checkout of libvirt[1]. I've attached the output as
 as-is.txt.  I tried adding hacking bzr support into vc-list-files (see
 vc-list-files-bzr.patch), but that didn't quite seem to do the trick, as
 you can see in in vc-list-files-maybe-fixed.log, which is the output
 after I patched vc-list-files.
 
 I tried CVS, too, and that also fails (see cvs-syntax-check.log).

 Is it only meant to work with git?  

It works with CVS - I use it with CVS most of the time.


This

 --- po-check-12008-04-29 17:03:10.452343086 +0200
 +++ po-check-22008-04-29 17:03:10.756338852 +0200
 @@ -1,36 +1,36 @@
 -gnulib/lib/gai_strerror.c
 -qemud/qemud.c
 -qemud/remote.c
 -src/conf.c
 -src/console.c
 -src/hash.c
 -src/iptables.c
 -src/libvirt.c
 -src/lxc_conf.c
 -src/lxc_container.c
 -src/lxc_driver.c
 -src/openvz_conf.c
 -src/openvz_driver.c
 -src/proxy_internal.c
 -src/qemu_conf.c
 -src/qemu_driver.c
 -src/remote_internal.c
 -src/sexpr.c
 -src/storage_backend.c
 -src/storage_backend_disk.c
 -src/storage_backend_fs.c
 -src/storage_backend_iscsi.c
 -src/storage_backend_logical.c
 -src/storage_conf.c
 -src/storage_driver.c
 -src/test.c
 -src/util.c
 -src/uuid.c
 -src/virsh.c
 -src/virterror.c
 -src/xen_internal.c
 -src/xend_internal.c
 -src/xm_internal.c
 -src/xml.c
 -src/xmlrpc.c
 -src/xs_internal.c
 +./gnulib/lib/gai_strerror.c
 +./qemud/qemud.c
 +./qemud/remote.c
 +./src/conf.c
 +./src/console.c
 +./src/hash.c
 +./src/iptables.c
 +./src/libvirt.c
 +./src/lxc_conf.c
 +./src/lxc_container.c
 +./src/lxc_driver.c
 +./src/openvz_conf.c
 +./src/openvz_driver.c
 +./src/proxy_internal.c
 +./src/qemu_conf.c
 +./src/qemu_driver.c
 +./src/remote_internal.c
 +./src/sexpr.c
 +./src/storage_backend.c
 +./src/storage_backend_disk.c
 +./src/storage_backend_fs.c
 +./src/storage_backend_iscsi.c
 +./src/storage_backend_logical.c
 +./src/storage_conf.c
 +./src/storage_driver.c
 +./src/test.c
 +./src/util.c
 +./src/uuid.c
 +./src/virsh.c
 +./src/virterror.c
 +./src/xen_internal.c
 +./src/xend_internal.c
 +./src/xm_internal.c
 +./src/xml.c
 +./src/xmlrpc.c
 +./src/xs_internal.c

...says that the vc-list-files script is getting a bogus leading ./ on all
your filenames. So something must be different about Ubuntu versions of
some program that causes this:

awk -F/ '{  \
if (!$1  $3 !~ /^-/) {\
  f=FILENAME;   \
  sub(/CVS\/Entries/, , f);   \
  print f $2;   \
}}' \
  $(find ${*-*} -name Entries -print) /dev/null;


to prepend a ./ - it could be the content of CVS/Entries, or perhaps
the find command - if you can figure out which, it ought to be easy
enough to strip off the ./

Regards,
Dan.
-- 
|: Red Hat, Engineering, Boston   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

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

[Libvir] PATCH: Support network interface model in Xen and QEMU driver

[Daniel P. Berrange]

This patch finishes off the work from Rich / Soren to support network
interface model in both Xen and QEMU drivers, and adds test cases for
the new syntax

 src/qemu_conf.c |   57 ++--
 src/qemu_conf.h |2 
 src/xend_internal.c |7 ++
 src/xm_internal.c   |   22 +++
 src/xml.c   |8 ++
 tests/qemuxml2argvdata/qemuxml2argv-net-virtio.args |1 
 tests/qemuxml2argvdata/qemuxml2argv-net-virtio.xml  |   26 +
 tests/qemuxml2argvtest.c|1 
 tests/qemuxml2xmltest.c |1 
 tests/sexpr2xmldata/sexpr2xml-net-e1000.sexpr   |2 
 tests/sexpr2xmldata/sexpr2xml-net-e1000.xml |   32 +++
 tests/sexpr2xmltest.c   |1 
 tests/testutils.c   |   31 ++
 tests/xmconfigdata/test-paravirt-net-e1000.cfg  |   12 
 tests/xmconfigdata/test-paravirt-net-e1000.xml  |   28 +
 tests/xmconfigtest.c|1 
 tests/xml2sexprdata/xml2sexpr-net-e1000.sexpr   |1 
 tests/xml2sexprdata/xml2sexpr-net-e1000.xml |   30 ++
 tests/xml2sexprtest.c   |1 
 19 files changed, 247 insertions(+), 17 deletions(-)


Dan.

Index: src/qemu_conf.c
===
RCS file: /data/cvs/libvirt/src/qemu_conf.c,v
retrieving revision 1.50
diff -u -p -r1.50 qemu_conf.c
--- src/qemu_conf.c 28 Apr 2008 15:14:59 -  1.50
+++ src/qemu_conf.c 29 Apr 2008 17:15:31 -
@@ -718,6 +718,7 @@ static int qemudParseInterfaceXML(virCon
 xmlChar *script = NULL;
 xmlChar *address = NULL;
 xmlChar *port = NULL;
+xmlChar *model = NULL;
 
 net-type = QEMUD_NET_USER;
 
@@ -779,6 +780,8 @@ static int qemudParseInterfaceXML(virCon
(net-type == QEMUD_NET_ETHERNET) 
xmlStrEqual(cur-name, BAD_CAST script)) {
 script = xmlGetProp(cur, BAD_CAST path);
+} else if (xmlStrEqual (cur-name, BAD_CAST model)) {
+model = xmlGetProp (cur, BAD_CAST type);
 }
 }
 cur = cur-next;
@@ -938,6 +941,39 @@ static int qemudParseInterfaceXML(virCon
 xmlFree(address);
 }
 
+/* NIC model (see -net nic,model=?).  We only check that it looks
+ * reasonable, not that it is a supported NIC type.  FWIW kvm
+ * supports these types as of April 2008:
+ * i82551 i82557b i82559er ne2k_pci pcnet rtl8139 e1000 virtio
+ */
+if (model != NULL) {
+int i, len, char_ok;
+
+len = xmlStrlen (model);
+if (len = QEMUD_MODEL_MAX_LEN) {
+qemudReportError (conn, NULL, NULL, VIR_ERR_INVALID_ARG,
+  _(Model name '%s' is too long), model);
+goto error;
+}
+for (i = 0; i  len; ++i) {
+char_ok =
+(model[i] = '0'  model[i] = '9') ||
+(model[i] = 'a'  model[i] = 'z') ||
+(model[i] = 'A'  model[i] = 'Z') || model[i] == '_';
+if (!char_ok) {
+qemudReportError (conn, NULL, NULL, VIR_ERR_INVALID_ARG,
+  _(Model name contains invalid characters));
+goto error;
+}
+}
+strncpy (net-model, (const char*) model, len);
+net-model[len] = '\0';
+
+xmlFree (model);
+model = NULL;
+} else
+net-model[0] = '\0';
+
 return 0;
 
  error:
@@ -953,6 +989,8 @@ static int qemudParseInterfaceXML(virCon
 xmlFree(script);
 if (bridge)
 xmlFree(bridge);
+if (model)
+xmlFree(model);
 return -1;
 }
 
@@ -2283,13 +2321,22 @@ int qemudBuildCommandLine(virConnectPtr 
 } else {
 int vlan = 0;
 while (net) {
+char model[100];
 char nic[100];
 
-if (snprintf(nic, sizeof(nic), 
nic,macaddr=%02x:%02x:%02x:%02x:%02x:%02x,vlan=%d,
+if (net-model[0] != '\0') {
+if (snprintf (model, sizeof (model), ,model=%s, net-model)
+= sizeof (model))
+goto error;
+} else
+model[0] = '\0';
+
+if (snprintf(nic, sizeof(nic),
+ nic,macaddr=%02x:%02x:%02x:%02x:%02x:%02x,vlan=%d%s,
  net-mac[0], net-mac[1],
  net-mac[2], net-mac[3],
  net-mac[4], net-mac[5],
- vlan) = sizeof(nic))
+ vlan, model) = sizeof(nic))
 goto error;
 
 if (!((*argv)[++n] = strdup(-net)))
@@ -3411,7 +3458,6 @@ static int qemudGenerateXMLChar(virBuffe
 

[Libvir] [PATCH] remove useless tests before xmlFree

[Jim Meyering]

I've just fixed a bug in my useless-if-detecting script,
committed in gnulib.  Using that new script with today's
change adding xmlFree to the list exposed a bunch of useless tests.
This first change set removes those tests.

Below it is a separate patch that updates gnulib-related
files, including that script and vc-list-files.
I verified that with these changes make distcheck passes.

If no one objects, I'll push these in about 12 hours.

remove useless tests before xmlFree
* src/qemu_conf.c (qemudParseDiskXML, qemudParseInterfaceXML):
(qemudParseInputXML, qemudParseDhcpRangesXML):
* src/remote_internal.c (doRemoteOpen):
* src/storage_conf.c (virStoragePoolDefParseDoc):
* src/xm_internal.c (xenXMParseXMLDisk, xenXMParseXMLVif):
(xenXMParseXMLToConfig, xenXMAttachInterface):
* src/xml.c (virDomainParseXMLDiskDesc, virDomainParseXMLIfDesc):
(virDomainXMLDevID):

Signed-off-by: Jim Meyering [EMAIL PROTECTED]
---
 src/qemu_conf.c   |   48 
 src/remote_internal.c |8 
 src/storage_conf.c|3 +--
 src/xm_internal.c |   42 ++
 src/xml.c |   30 ++
 5 files changed, 45 insertions(+), 86 deletions(-)

diff --git a/src/qemu_conf.c b/src/qemu_conf.c
index 07dfe47..a0c4a8f 100644
--- a/src/qemu_conf.c
+++ b/src/qemu_conf.c
@@ -680,14 +680,10 @@ static int qemudParseDiskXML(virConnectPtr conn,
 return 0;

  error:
-if (type)
-xmlFree(type);
-if (target)
-xmlFree(target);
-if (source)
-xmlFree(source);
-if (device)
-xmlFree(device);
+xmlFree(type);
+xmlFree(target);
+xmlFree(source);
+xmlFree(device);
 return -1;
 }

@@ -941,18 +937,12 @@ static int qemudParseInterfaceXML(virConnectPtr conn,
 return 0;

  error:
-if (network)
-xmlFree(network);
-if (address)
-xmlFree(address);
-if (port)
-xmlFree(port);
-if (ifname)
-xmlFree(ifname);
-if (script)
-xmlFree(script);
-if (bridge)
-xmlFree(bridge);
+xmlFree(network);
+xmlFree(address);
+xmlFree(port);
+xmlFree(ifname);
+xmlFree(script);
+xmlFree(bridge);
 return -1;
 }

@@ -1334,18 +1324,14 @@ static int qemudParseInputXML(virConnectPtr conn,
 input-bus = QEMU_INPUT_BUS_USB;
 }

-if (type)
-xmlFree(type);
-if (bus)
-xmlFree(bus);
+xmlFree(type);
+xmlFree(bus);

 return 0;

  error:
-if (type)
-xmlFree(type);
-if (bus)
-xmlFree(bus);
+xmlFree(type);
+xmlFree(bus);

 return -1;
 }
@@ -2860,10 +2846,8 @@ static int qemudParseDhcpRangesXML(virConnectPtr conn,
 free(range);
 }

-if (start)
-xmlFree(start);
-if (end)
-xmlFree(end);
+xmlFree(start);
+xmlFree(end);

 cur = cur-next;
 }
diff --git a/src/remote_internal.c b/src/remote_internal.c
index ef34a3a..70aa5e9 100644
--- a/src/remote_internal.c
+++ b/src/remote_internal.c
@@ -434,9 +434,9 @@ doRemoteOpen (virConnectPtr conn,
 }

 #ifdef HAVE_XMLURI_QUERY_RAW
-if (uri-query_raw) xmlFree (uri-query_raw);
+xmlFree (uri-query_raw);
 #else
-if (uri-query) xmlFree (uri-query);
+xmlFree (uri-query);
 #endif

 if ((
@@ -464,10 +464,10 @@ doRemoteOpen (virConnectPtr conn,
 transport_str[-1] = '\0';
 }
 /* Remove the username, server name and port number. */
-if (uri-user) xmlFree (uri-user);
+xmlFree (uri-user);
 uri-user = 0;

-if (uri-server) xmlFree (uri-server);
+xmlFree (uri-server);
 uri-server = 0;

 uri-port = 0;
diff --git a/src/storage_conf.c b/src/storage_conf.c
index 4499ae2..be21d3b 100644
--- a/src/storage_conf.c
+++ b/src/storage_conf.c
@@ -357,8 +357,7 @@ virStoragePoolDefParseDoc(virConnectPtr conn,

  cleanup:
 free(uuid);
-if (type)
-xmlFree(type);
+xmlFree(type);
 virStoragePoolDefFree(ret);
 return NULL;
 }
diff --git a/src/xm_internal.c b/src/xm_internal.c
index a70436d..08e3e8e 100644
--- a/src/xm_internal.c
+++ b/src/xm_internal.c
@@ -1657,10 +1657,8 @@ static int xenXMParseXMLDisk(xmlNodePtr node, int hvm, 
int xendConfigVersion, ch
 }

 if (target == NULL) {
-if (source != NULL)
-xmlFree(source);
-if (device != NULL)
-xmlFree(device);
+xmlFree(source);
+xmlFree(device);
 return (-1);
 }

@@ -1687,10 +1685,8 @@ static int xenXMParseXMLDisk(xmlNodePtr node, int hvm, 
int xendConfigVersion, ch
 }

 if (source == NULL  !cdrom) {
-if (target != NULL)
-xmlFree(target);
-if (device != NULL)
-xmlFree(device);
+xmlFree(target);
+xmlFree(device);
 return (-1);
 }

@@ 

Re: [Libvir] [PATCH] remove useless tests before xmlFree

[Daniel P. Berrange]

On Tue, Apr 29, 2008 at 09:30:39PM +0200, Jim Meyering wrote:
 I've just fixed a bug in my useless-if-detecting script,
 committed in gnulib.  Using that new script with today's
 change adding xmlFree to the list exposed a bunch of useless tests.
 This first change set removes those tests.
 
 Below it is a separate patch that updates gnulib-related
 files, including that script and vc-list-files.
 I verified that with these changes make distcheck passes.
 
 If no one objects, I'll push these in about 12 hours.
 
   remove useless tests before xmlFree
   * src/qemu_conf.c (qemudParseDiskXML, qemudParseInterfaceXML):
   (qemudParseInputXML, qemudParseDhcpRangesXML):
   * src/remote_internal.c (doRemoteOpen):
   * src/storage_conf.c (virStoragePoolDefParseDoc):
   * src/xm_internal.c (xenXMParseXMLDisk, xenXMParseXMLVif):
   (xenXMParseXMLToConfig, xenXMAttachInterface):
   * src/xml.c (virDomainParseXMLDiskDesc, virDomainParseXMLIfDesc):
   (virDomainXMLDevID):
 
 Signed-off-by: Jim Meyering [EMAIL PROTECTED]

ACK, please commit .

Dan.
-- 
|: Red Hat, Engineering, Boston   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

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

Re: [Libvir] [PATCH] remove useless tests before xmlFree

[Jim Meyering]

Daniel P. Berrange [EMAIL PROTECTED] wrote:
 On Tue, Apr 29, 2008 at 09:30:39PM +0200, Jim Meyering wrote:
 I've just fixed a bug in my useless-if-detecting script,
 committed in gnulib.  Using that new script with today's
 change adding xmlFree to the list exposed a bunch of useless tests.
 This first change set removes those tests.

 Below it is a separate patch that updates gnulib-related
 files, including that script and vc-list-files.
 I verified that with these changes make distcheck passes.

 If no one objects, I'll push these in about 12 hours.

  remove useless tests before xmlFree
...
 ACK, please commit .

Ok.  I've just done that one.
The gnulib one will take a couple more minutes.

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

[Libvir] Basic VMWare support

[Marek 'marx' Grac]

Hi,

I know that there is no support for VMWare ESX yet but I would like to 
know if someone is working on it. I need just three basic operations: 
status/power on/power off but I never saw the source code of libvirt :) 
Do you think it is possible to write this support (using VMWare API) in 
acceptable time? week? month?


  Thanks, for any suggestions
m,

--
Marek Grac
Red Hat Czech s.r.o.


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

Re: [Libvir] Basic VMWare support

[Daniel P. Berrange]

On Tue, Apr 29, 2008 at 10:22:33PM +0200, Marek 'marx' Grac wrote:
 Hi,
 
 I know that there is no support for VMWare ESX yet but I would like to 
 know if someone is working on it. I need just three basic operations: 
 status/power on/power off but I never saw the source code of libvirt :) 
 Do you think it is possible to write this support (using VMWare API) in 
 acceptable time? week? month?

DV already replied to your mail from yesterday - no need to re-post
exactly the same mail every day.

Dan.
-- 
|: Red Hat, Engineering, Boston   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

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

Re: [Libvir] PATCH: Support network interface model in Xen and QEMU driver

[Daniel P. Berrange]

On Tue, Apr 29, 2008 at 06:20:01PM +0100, Daniel P. Berrange wrote:
 This patch finishes off the work from Rich / Soren to support network
 interface model in both Xen and QEMU drivers, and adds test cases for
 the new syntax

Here is a re-diff following Jim's   xmlFree cleanups.

Dan.

Index: tests/qemuxml2argvtest.c
===
RCS file: /data/cvs/libvirt/tests/qemuxml2argvtest.c,v
retrieving revision 1.15
diff -u -p -r1.15 qemuxml2argvtest.c
--- tests/qemuxml2argvtest.c25 Apr 2008 20:46:13 -  1.15
+++ tests/qemuxml2argvtest.c29 Apr 2008 20:46:07 -
@@ -146,6 +146,7 @@ main(int argc, char **argv)
 DO_TEST(misc-acpi);
 DO_TEST(misc-no-reboot);
 DO_TEST(net-user);
+DO_TEST(net-virtio);
 
 DO_TEST(serial-vc);
 DO_TEST(serial-pty);
Index: tests/qemuxml2xmltest.c
===
RCS file: /data/cvs/libvirt/tests/qemuxml2xmltest.c,v
retrieving revision 1.13
diff -u -p -r1.13 qemuxml2xmltest.c
--- tests/qemuxml2xmltest.c 25 Apr 2008 20:46:13 -  1.13
+++ tests/qemuxml2xmltest.c 29 Apr 2008 20:46:07 -
@@ -109,6 +109,7 @@ main(int argc, char **argv)
 DO_TEST(misc-acpi);
 DO_TEST(misc-no-reboot);
 DO_TEST(net-user);
+DO_TEST(net-virtio);
 
 DO_TEST(serial-vc);
 DO_TEST(serial-pty);
Index: tests/sexpr2xmltest.c
===
RCS file: /data/cvs/libvirt/tests/sexpr2xmltest.c,v
retrieving revision 1.26
diff -u -p -r1.26 sexpr2xmltest.c
--- tests/sexpr2xmltest.c   26 Apr 2008 14:22:02 -  1.26
+++ tests/sexpr2xmltest.c   29 Apr 2008 20:46:07 -
@@ -116,6 +116,7 @@ main(int argc, char **argv)
 DO_TEST(curmem, curmem, 1);
 DO_TEST(net-routed, net-routed, 2);
 DO_TEST(net-bridged, net-bridged, 2);
+DO_TEST(net-e1000, net-e1000, 2);
 DO_TEST(no-source-cdrom, no-source-cdrom, 1);
 
 DO_TEST(fv-utc, fv-utc, 1);
Index: tests/testutils.c
===
RCS file: /data/cvs/libvirt/tests/testutils.c,v
retrieving revision 1.12
diff -u -p -r1.12 testutils.c
--- tests/testutils.c   18 Apr 2008 15:05:29 -  1.12
+++ tests/testutils.c   29 Apr 2008 20:46:07 -
@@ -23,6 +23,7 @@
 #include fcntl.h
 #include limits.h
 #include testutils.h
+#include internal.h
 
 #ifdef HAVE_PATHS_H
 #include paths.h
@@ -231,23 +232,27 @@ int virtTestDifference(FILE *stream,
 const char *expectEnd = expect + (strlen(expect)-1);
 const char *actualStart = actual;
 const char *actualEnd = actual + (strlen(actual)-1);
+const char *debug;
 
-if (getenv(DEBUG_TESTS) == NULL)
+if ((debug = getenv(DEBUG_TESTS)) == NULL)
 return 0;
 
-/* Skip to first character where they differ */
-while (*expectStart  *actualStart 
-   *actualStart == *expectStart) {
-actualStart++;
-expectStart++;
-}
+if (STREQ(debug, ) ||
+STREQ(debug, 1)) {
+/* Skip to first character where they differ */
+while (*expectStart  *actualStart 
+   *actualStart == *expectStart) {
+actualStart++;
+expectStart++;
+}
 
-/* Work backwards to last character where they differ */
-while (actualEnd  actualStart 
-   expectEnd  expectStart 
-   *actualEnd == *expectEnd) {
-actualEnd--;
-expectEnd--;
+/* Work backwards to last character where they differ */
+while (actualEnd  actualStart 
+   expectEnd  expectStart 
+   *actualEnd == *expectEnd) {
+actualEnd--;
+expectEnd--;
+}
 }
 
 /* Show the trimmed differences */
Index: tests/xmconfigtest.c
===
RCS file: /data/cvs/libvirt/tests/xmconfigtest.c,v
retrieving revision 1.15
diff -u -p -r1.15 xmconfigtest.c
--- tests/xmconfigtest.c26 Apr 2008 14:22:02 -  1.15
+++ tests/xmconfigtest.c29 Apr 2008 20:46:07 -
@@ -202,6 +202,7 @@ main(int argc, char **argv)
 
 DO_TEST(paravirt-old-pvfb, 2);
 DO_TEST(paravirt-new-pvfb, 3);
+DO_TEST(paravirt-net-e1000, 3);
 DO_TEST(fullvirt-old-cdrom, 1);
 DO_TEST(fullvirt-new-cdrom, 2);
 DO_TEST(fullvirt-utc, 2);
Index: tests/xml2sexprtest.c
===
RCS file: /data/cvs/libvirt/tests/xml2sexprtest.c,v
retrieving revision 1.25
diff -u -p -r1.25 xml2sexprtest.c
--- tests/xml2sexprtest.c   26 Apr 2008 14:22:02 -  1.25
+++ tests/xml2sexprtest.c   29 Apr 2008 20:46:07 -
@@ -123,6 +123,7 @@ main(int argc, char **argv)
 DO_TEST(curmem, curmem, rhel5, 2);
 DO_TEST(net-routed, net-routed, pvtest, 2);
 DO_TEST(net-bridged, net-bridged, pvtest, 2);
+DO_TEST(net-e1000, net-e1000, pvtest, 2);
 DO_TEST(no-source-cdrom, 

Re: [Libvir] [PATCH] remove useless tests before xmlFree

[Daniel P. Berrange]

On Tue, Apr 29, 2008 at 09:44:50PM +0200, Jim Meyering wrote:
 Daniel P. Berrange [EMAIL PROTECTED] wrote:
  On Tue, Apr 29, 2008 at 09:30:39PM +0200, Jim Meyering wrote:
  I've just fixed a bug in my useless-if-detecting script,
  committed in gnulib.  Using that new script with today's
  change adding xmlFree to the list exposed a bunch of useless tests.
  This first change set removes those tests.
 
  Below it is a separate patch that updates gnulib-related
  files, including that script and vc-list-files.
  I verified that with these changes make distcheck passes.
 
  If no one objects, I'll push these in about 12 hours.
 
 remove useless tests before xmlFree
 ...
  ACK, please commit .
 
 Ok.  I've just done that one.
 The gnulib one will take a couple more minutes.

I think one of the files from the gnulib refresh got missed from the
commit

[EMAIL PROTECTED] libvirt-new]$ make
make  all-recursive
make[1]: Entering directory `/home/berrange/src/xen/libvirt-new'
Making all in gnulib/lib
make[2]: Entering directory `/home/berrange/src/xen/libvirt-new/gnulib/lib'
{ echo '/* DO NOT EDIT! GENERATED AUTOMATICALLY! */'; \
  cat ./alloca.in.h; \
}  alloca.h-t
mv -f alloca.h-t alloca.h
/bin/mkdir -p arpa
rm -f arpa/inet.h-t arpa/inet.h
{ echo '/* DO NOT EDIT! GENERATED AUTOMATICALLY! */'; \
  sed -e 's/@''INCLUDE_NEXT''@/include_next/g' \
  -e 's|@''NEXT_ARPA_INET_H''@|arpa/inet.h|g' \
  -e 's|@''HAVE_ARPA_INET_H''@|1|g' \
  -e 's|@''GNULIB_INET_NTOP''@|1|g' \
  -e 's|@''GNULIB_INET_PTON''@|0|g' \
  -e 's|@''HAVE_DECL_INET_NTOP''@|1|g' \
  -e 's|@''HAVE_DECL_INET_PTON''@|1|g' \
   ./arpa_inet.in.h; \
}  arpa/inet.h-t
/bin/sh: line 1: ./arpa_inet.in.h: No such file or directory
make[2]: *** [arpa/inet.h] Error 1
make[2]: Leaving directory `/home/berrange/src/xen/libvirt-new/gnulib/lib'
make[1]: *** [all-recursive] Error 1
make[1]: Leaving directory `/home/berrange/src/xen/libvirt-new'
make: *** [all] Error 2


Regards,
Dan.
-- 
|: Red Hat, Engineering, Boston   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

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

Re: [Libvir] PATCH: Support network interface model in Xen and QEMU driver

[Jim Meyering]

Daniel P. Berrange [EMAIL PROTECTED] wrote:
 On Tue, Apr 29, 2008 at 06:20:01PM +0100, Daniel P. Berrange wrote:
 This patch finishes off the work from Rich / Soren to support network
 interface model in both Xen and QEMU drivers, and adds test cases for
 the new syntax

 Here is a re-diff following Jim's   xmlFree cleanups.

Looks fine to me.

 +/usr/bin/qemu -M pc -m 214 -smp 1 -nographic -monitor pty -no-acpi -boot c 
 -hda /dev/HostVG/QEMUGuest1 -net 
 nic,macaddr=00:11:22:33:44:55,vlan=0,model=virtio -net user,vlan=0 -serial 
 none -parallel none -usb

It'd be nice to change the line above to e.g.,

  qemu -M pc -m 214 -smp 1 -nographic -monitor pty  \
-no-acpi -boot c -hda /dev/HostVG/QEMUGuest1\
-net nic,macaddr=00:11:22:33:44:55,vlan=0,model=virtio  \
-net user,vlan=0 -serial none -parallel none -usb

When possible, I prefer not to hard-code program names like /usr/bin/qemu
in tests, because that lets the test work even when the tool in question
is installed somewhere else.

What if someone is using qemu they built themselves...

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

Re: [Libvir] PATCH: Support network interface model in Xen and QEMU driver

[Jim Paris]

Daniel P. Berrange wrote:
 This patch finishes off the work from Rich / Soren to support network
 interface model in both Xen and QEMU drivers, and adds test cases for
 the new syntax

I still think our consensus from when I posted this patch last year
(nic model=...) makes more sense ... but getting any form of this
patch upstream sounds good to me.

 -if (snprintf(nic, sizeof(nic), 
 nic,macaddr=%02x:%02x:%02x:%02x:%02x:%02x,vlan=%d,
 +if (net-model[0] != '\0') {
 +if (snprintf (model, sizeof (model), ,model=%s, net-model)
 += sizeof (model))
 +goto error;
 +} else
 +model[0] = '\0';
 +
 +if (snprintf(nic, sizeof(nic),
 + 
 nic,macaddr=%02x:%02x:%02x:%02x:%02x:%02x,vlan=%d%s,
   net-mac[0], net-mac[1],
   net-mac[2], net-mac[3],
   net-mac[4], net-mac[5],
 - vlan) = sizeof(nic))
 + vlan, model) = sizeof(nic))

You could simplify this and not require the temporary buffer if you do
it this way:
  http://www.mail-archive.com/libvir-list@redhat.com/msg03557.html

-jim

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