[libvirt] [PATCH] storage_backend_fs: avoid NULL dereference on opendir failure

2009-09-02 Thread Jim Meyering 
I've just begun using clang's static analyzer,

http://clang-analyzer.llvm.org/

It has uncovered a few problems in libvirt.
Here are the first few fixes.
I'll send more details later today.

From b6bb9d82effa56733fbee9013e66fed384d9ff63 Mon Sep 17 00:00:00 2001
From: Jim Meyering meyer...@redhat.com
Date: Wed, 2 Sep 2009 09:42:32 +0200
Subject: [PATCH 1/4] storage_backend_fs: avoid NULL dereference on opendir 
failure

* src/storage_backend_fs.c (virStorageBackendFileSystemRefresh):
Don't call closedir on a NULL pointer.
---
 src/storage_backend_fs.c |3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/src/storage_backend_fs.c b/src/storage_backend_fs.c
index 65b656d..8241504 100644
--- a/src/storage_backend_fs.c
+++ b/src/storage_backend_fs.c
@@ -983,7 +983,8 @@ no_memory:
 /* fallthrough */

  cleanup:
-closedir(dir);
+if (dir)
+closedir(dir);
 virStorageVolDefFree(vol);
 virStoragePoolObjClearVols(pool);
 return -1;
--
1.6.4.2.395.ge3d52


From eaae148291680a72d19aa9d5320f90b98f123746 Mon Sep 17 00:00:00 2001
From: Jim Meyering meyer...@redhat.com
Date: Wed, 2 Sep 2009 09:58:28 +0200
Subject: [PATCH 2/4] storage_conf.c: avoid overflow upon use of z or Z 
(zebi) suffix

* src/storage_conf.c (virStorageSize): Don't try to compute 1024^7,
since it's too large for a 64-bit type.
---
 src/storage_conf.c |6 --
 1 files changed, 0 insertions(+), 6 deletions(-)

diff --git a/src/storage_conf.c b/src/storage_conf.c
index c446069..110f0ad 100644
--- a/src/storage_conf.c
+++ b/src/storage_conf.c
@@ -919,12 +919,6 @@ virStorageSize(virConnectPtr conn,
 1024ull;
 break;

-case 'z':
-case 'Z':
-mult = 1024ull * 1024ull * 1024ull * 1024ull * 1024ull *
-1024ull * 1024ull;
-break;
-
 default:
 virStorageReportError(conn, VIR_ERR_XML_ERROR,
   _(unknown size units '%s'), unit);
--
1.6.4.2.395.ge3d52


From 7f453c68bc709d542e4c40a388c92c7969ad0a3a Mon Sep 17 00:00:00 2001
From: Jim Meyering meyer...@redhat.com
Date: Wed, 2 Sep 2009 09:58:50 +0200
Subject: [PATCH 3/4] lxc: avoid NULL dereference when we find no mount point

* src/lxc_container.c (lxcContainerUnmountOldFS): Don't pass
a NULL pointer to qsort.
---
 src/lxc_container.c |5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/src/lxc_container.c b/src/lxc_container.c
index 950dd50..2073864 100644
--- a/src/lxc_container.c
+++ b/src/lxc_container.c
@@ -546,8 +546,9 @@ static int lxcContainerUnmountOldFS(void)
 }
 endmntent(procmnt);

-qsort(mounts, nmounts, sizeof(mounts[0]),
-  lxcContainerChildMountSort);
+if (mounts)
+qsort(mounts, nmounts, sizeof(mounts[0]),
+  lxcContainerChildMountSort);

 for (i = 0 ; i  nmounts ; i++) {
 VIR_DEBUG(Umount %s, mounts[i]);
--
1.6.4.2.395.ge3d52


From 4e97befca175af427ed3b75f59e67cd620ee3ce2 Mon Sep 17 00:00:00 2001
From: Jim Meyering meyer...@redhat.com
Date: Wed, 2 Sep 2009 10:02:49 +0200
Subject: [PATCH 4/4] lxc: don't unlink(NULL) in main

* src/lxc_controller.c (main): Unlink sockpath only if it's non-NULL.
---
 src/lxc_controller.c |3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/src/lxc_controller.c b/src/lxc_controller.c
index 8d11238..914c10a 100644
--- a/src/lxc_controller.c
+++ b/src/lxc_controller.c
@@ -803,7 +803,8 @@ cleanup:
 if (def)
 virFileDeletePid(LXC_STATE_DIR, def-name);
 lxcControllerCleanupInterfaces(nveths, veths);
-unlink(sockpath);
+if (sockpath):
+unlink(sockpath);
 VIR_FREE(sockpath);

 return rc;
--
1.6.4.2.395.ge3d52

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

Re: [libvirt] [PATCH] storage_backend_fs: avoid NULL dereference on opendir failure

2009-09-02 Thread Daniel Veillard 
On Wed, Sep 02, 2009 at 10:05:03AM +0200, Jim Meyering wrote:
 I've just begun using clang's static analyzer,
 
 http://clang-analyzer.llvm.org/
 
 It has uncovered a few problems in libvirt.
 Here are the first few fixes.
 I'll send more details later today.

  All patches look fine, it's just too bad we have to drop the 'z'
storage size suffix, but it's not realistic on current platforms.

   ACK

Daniel

-- 
Daniel Veillard  | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
dan...@veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/

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

Re: [libvirt] [PATCH 1/6] Add helper module for dealing with USB host devices

2009-09-02 Thread Daniel Veillard 
On Tue, Sep 01, 2009 at 04:28:54PM +0100, Daniel P. Berrange wrote:
 * src/Makefile.am: Add usb.h and usb.h to libvirt_util.la
 * src/libvirt_private.syms: Export symbols
 * src/usb.c, src/usb.h: Helper APIs for USB host devices
 ---
  src/Makefile.am  |1 +
  src/hostusb.c|  103 
 ++
  src/hostusb.h|   45 
  src/libvirt_private.syms |4 ++
  4 files changed, 153 insertions(+), 0 deletions(-)
  create mode 100644 src/hostusb.c
  create mode 100644 src/hostusb.h

  Sounds good

[...]
 +/* For virReportOOMError()  and virReportSystemError() */
 +#define VIR_FROM_THIS VIR_FROM_NONE

   VIR_FROM_STORAGE might be a bit more precise

 +usbDevice *
 +usbGetDevice(virConnectPtr conn,
 + unsigned bus,
 + unsigned devno)
 +{

 The function code should probably be #ifdef linux

 +usbDevice *dev;
 +
 +if (VIR_ALLOC(dev)  0) {
 +virReportOOMError(conn);
 +return NULL;
 +}
 +
 +dev-bus = bus;
 +dev-dev = devno;
 +
 +snprintf(dev-name, sizeof(dev-name), %.3o:%.3o,
 + dev-bus, dev-dev);
 +snprintf(dev-path, sizeof(dev-path),
 + USB_DEVFS %03o/%03o, dev-bus, dev-dev);
 +
 +/* XXX fixme. this should be product/vendor */
 +snprintf(dev-id, sizeof(dev-id), %d %d, dev-bus, dev-dev);
 +
 +VIR_DEBUG(%s %s: initialized, dev-id, dev-name);
 +
 +return dev;

  with a #else emitting an error

   Those tiny things apart, ACK, looks fine !

Daniel

-- 
Daniel Veillard  | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
dan...@veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/

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

Re: [libvirt] [PATCH 2/6] Add helper APIs for iterating over PCI device resource files

2009-09-02 Thread Daniel Veillard 
On Tue, Sep 01, 2009 at 04:28:55PM +0100, Daniel P. Berrange wrote:
[...]
 @@ -1022,3 +1022,55 @@ pciDeviceListFind(pciDeviceList *list, pciDevice *dev)
  return list-devs[i];
  return NULL;
  }
 +
 +
 +int pciDeviceFileIterate(virConnectPtr conn,
 + pciDevice *dev,
 + pciDeviceFileActor actor,
 + void *opaque)
 +{
 +char *pcidir = NULL;
 +char *file = NULL;
 +DIR *dir = NULL;
 +int ret = -1;
 +struct dirent *ent;
 +
 +if (virAsprintf(pcidir, /sys/bus/pci/devices/%04x:%02x:%02x.%x,
 +dev-domain, dev-bus, dev-slot, dev-function)  0) {

  hum %s/devices/%04x:%02x:%02x.%x , PCI_SYSFS, ...
would be a bit better I guess

   Fine, ACK,

Daniel

-- 
Daniel Veillard  | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
dan...@veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/

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

Re: [libvirt] [PATCH 3/6] Port QEMU driver to use USB/PCI device helpers

2009-09-02 Thread Daniel Veillard 
On Tue, Sep 01, 2009 at 04:28:56PM +0100, Daniel P. Berrange wrote:
 * src/qemu_driver.c: Remove usbfs/sysfs iterator code and call
   into generic helper APIs instead when setting device permissions

  Yup, nice cleanup, ACK,

  just one thing, it would be good in 1/6 and 2/6 to detail as a comment
on the iterators in the headers the expected return value and the fact
that non-zero will break iteration and be the returned value from top
level. Usually when having to program those callbacks it's the classic
pitfall :)

Daniel

-- 
Daniel Veillard  | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
dan...@veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/

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

[libvirt] [PATCH] remote_internal.c: don't dereference a NULL conn

2009-09-02 Thread Jim Meyering 
Here's the test just before the else-if in the patch below:

if (conn 
conn-driver 
STREQ (conn-driver-name, remote)) {

So, in the else-branch, conn is guaranteed to be NULL.
And dereferenced.

This may be only a theoretical risk, but if so,
the test of conn above should be changed to an assertion,
and/or the parameter should get the nonnull attribute.

From a1b1d36d96f6b50ddf514539af85da20ca671bf5 Mon Sep 17 00:00:00 2001
From: Jim Meyering meyer...@redhat.com
Date: Wed, 2 Sep 2009 11:54:38 +0200
Subject: [PATCH] remote_internal.c: don't dereference a NULL conn

* src/remote_internal.c (remoteDevMonOpen): Avoid NULL-dereference.
---
 src/remote_internal.c |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/src/remote_internal.c b/src/remote_internal.c
index ea50c11..141fef9 100644
--- a/src/remote_internal.c
+++ b/src/remote_internal.c
@@ -5148,7 +5148,7 @@ remoteDevMonOpen(virConnectPtr conn,
 conn-devMonPrivateData = priv;
 remoteDriverUnlock(priv);
 return VIR_DRV_OPEN_SUCCESS;
-} else if (conn-networkDriver 
+} else if (conn  conn-networkDriver 
STREQ (conn-networkDriver-name, remote)) {
 struct private_data *priv = conn-networkPrivateData;
 remoteDriverLock(priv);
--
1.6.4.2.395.ge3d52

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

Re: [libvirt] [PATCH 4/6] Support relabelling of USB and PCI devices

2009-09-02 Thread Daniel Veillard 
On Tue, Sep 01, 2009 at 04:28:57PM +0100, Daniel P. Berrange wrote:
 * src/security.h: Driver API for relabelling host devices
 * src/security_selinux.c: Implement relabelling of PCI and USB
   devices
 * src/qemu_driver.c: Relabel USB/PCI devices before hotplug
 ---
  src/qemu_driver.c  |   12 ++-
  src/security.h |7 ++
  src/security_selinux.c |  175 
 +++-
  3 files changed, 174 insertions(+), 20 deletions(-)
 
 diff --git a/src/qemu_driver.c b/src/qemu_driver.c
 index e9a09df..d75e28e 100644
 --- a/src/qemu_driver.c
 +++ b/src/qemu_driver.c
 @@ -5498,6 +5498,9 @@ static int qemudDomainAttachHostDevice(virConnectPtr 
 conn,
  
  if (qemuDomainSetDeviceOwnership(conn, driver, dev, 0)  0)
  return -1;
 +if (driver-securityDriver 
 +driver-securityDriver-domainSetSecurityHostdevLabel(conn, vm, 
 dev-data.hostdev)  0)
 +return -1;

  shouldn't we test against the entry point being null ?
   if (driver-securityDriver 
   driver-securityDriver-domainSetSecurityHostdevLabel 
   driver-securityDriver-domainSetSecurityHostdevLabel(conn,
vm, dev-data.hostdev)  0)

Also the long line should probably be split.

[...]
 +if (driver-securityDriver 
 +driver-securityDriver-domainSetSecurityHostdevLabel(conn, vm, 
 dev-data.hostdev)  0)
 +VIR_WARN0(Failed to restore device labelling);
 +

  idem

[...]
  static int
 +SELinuxRestoreSecurityImageLabel(virConnectPtr conn,
 + virDomainDiskDefPtr disk)
 +{
 +/* Don't restore labels on readoly/shared disks, because

  typo readonly

 + * other VMs may still be accessing these
 + * Alternatively we could iterate over all running
 + * domains and try to figure out if it is in use, but
 + * this would not work for clustered filesystems, since
 + * we can't see running VMs using the file on other nodes
 + * Safest bet is thus to skip the restore step.
 + */
 +if (disk-readonly || disk-shared)
 +return 0;
 +
 +if (!disk-src)
 +return 0;
 +
 +return SELinuxRestoreSecurityFileLabel(conn, disk-src);
 +}
 +
 +static int
  SELinuxSetSecurityImageLabel(virConnectPtr conn,
   virDomainObjPtr vm,
   virDomainDiskDefPtr disk)
 @@ -414,6 +427,126 @@ SELinuxSetSecurityImageLabel(virConnectPtr conn,
  return 0;
  }
  
 +
 +static int
 +SELinuxSetSecurityPCILabel(virConnectPtr conn,
 +   pciDevice *dev ATTRIBUTE_UNUSED,
 +   const char *file, void *opaque)
 +{
 +virDomainObjPtr vm = opaque;
 +const virSecurityLabelDefPtr secdef = vm-def-seclabel;
 +
 +return SELinuxSetFilecon(conn, file, secdef-imagelabel);
 +}

  it would be nice if we could check the type of the opaque passed
  in some ways, even if we know it was called a few line below.

 +static int
 +SELinuxSetSecurityHostdevLabel(virConnectPtr conn,
 +   virDomainObjPtr vm,
 +   virDomainHostdevDefPtr dev)
 +
 +{
 +int ret = -1;
 +
 +if (dev-mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS)
 +return 0;
 +
 +switch (dev-source.subsys.type) {
 +case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB:
 +break;
 +
 +case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: {
 +pciDevice *pci = pciGetDevice(conn,
 +  dev-source.subsys.u.pci.domain,
 +  dev-source.subsys.u.pci.bus,
 +  dev-source.subsys.u.pci.slot,
 +  dev-source.subsys.u.pci.function);
 +
 +if (!pci)
 +goto done;
 +
 +ret = pciDeviceFileIterate(conn, pci, SELinuxSetSecurityPCILabel, 
 vm);
 +pciFreeDevice(conn, pci);
 +
 +break;
 +}
 +
 +default:
 +ret = 0;
 +break;
 +}
 +
 +done:
 +return ret;
 +}

  Looks fine, ACK, but I won't pretend I understand all the implications
  of the security calls though,

Daniel

-- 
Daniel Veillard  | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
dan...@veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/

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

Re: [libvirt] [PATCH] remote_internal.c: don't dereference a NULL conn

2009-09-02 Thread Daniel Veillard 
On Wed, Sep 02, 2009 at 11:58:07AM +0200, Jim Meyering wrote:
 Here's the test just before the else-if in the patch below:
 
 if (conn 
 conn-driver 
 STREQ (conn-driver-name, remote)) {
 
 So, in the else-branch, conn is guaranteed to be NULL.
 And dereferenced.
 
 This may be only a theoretical risk, but if so,
 the test of conn above should be changed to an assertion,
 and/or the parameter should get the nonnull attribute.

  I'm fine with your patch, so that even if code around changes
having a gard is IMHO a good thing

  ACK

   thanks !

Daniel

-- 
Daniel Veillard  | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
dan...@veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/

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

[libvirt] [RFC] Support for CPUID masking

2009-09-02 Thread Jiri Denemark 
Hi,

We need to provide support for CPU ID masking. Xen and VMware ESX are examples
of current hypervisors which support such masking.

My proposal is to define new 'cpuid' feature advertised in guest
capabilities:

guest
...
features
cpuid/
/feature
/guest

When a driver supports cpuid feature, one can use it to mask/check for
specific bits returned by CPU ID as follows:

domain
...
features
cpuid
mask level='hex' register='eax|ebx|ecx|edx'MASK/mask
/cpuid
/features
...
/domain

Where
- level is a hexadecimal number used as an input to CPU ID (i.e. eax
  register),
- register is one of eax, ebx, ecx or edx,
- and MASK is a string with the following format:
  ::::::: with m being 1-bit mask for the
  corresponding bit in the register.

There are three possibilities of specifying what values can be used for 'm':
- let it be driver-specific,
- define all possible values,
- define a common set of values and allow drivers to specify their own
  additional values.

I think the third is the way to go as it lowers the confusion of different
values used by different drivers for the same purpose while maintaining the
flexibility to support driver-specific masks.

The following could be a good set of predefined common values:
- 1 force the bit to be 1
- 0 force the bit to be 0
- x don't care, i.e., use driver's default value
- T require the bit to be 1
- F require the bit to be 0


Some examples of what it could look like follow:

capabilities
...
guest
os_typexen/os_type
...
features
pae/
cpuid/
/features
/guest
...
/capabilities


domain type='xen' id='42'
...
features
pae/
acpi/
apic/
cpuid
mask level='1' register='ebx'
:::1010::::
/mask
mask level='1' register='ecx'
::::::xx1x:
/mask
mask level='1' register='edx'
xxx1:::::::
/mask
mask level='8001' register='ecx'
:::::::xx1x
/mask
mask level='8008' register='ecx'
::::::xx00:1001
/mask
/cpuid
/features
...
/domain


What are your opinions about this?

Thank for all comments.

Jirka

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

Re: [libvirt] [PATCH] remote_internal.c: don't dereference a NULL conn

2009-09-02 Thread Jim Meyering 
Daniel Veillard wrote:
 On Wed, Sep 02, 2009 at 11:58:07AM +0200, Jim Meyering wrote:
 Here's the test just before the else-if in the patch below:

 if (conn 
 conn-driver 
 STREQ (conn-driver-name, remote)) {

 So, in the else-branch, conn is guaranteed to be NULL.
 And dereferenced.

 This may be only a theoretical risk, but if so,
 the test of conn above should be changed to an assertion,
 and/or the parameter should get the nonnull attribute.

   I'm fine with your patch, so that even if code around changes
 having a gard is IMHO a good thing

Thanks for the quick review.
Daniel Berrange said he'd prefer the nonnull approach I mentioned above,
so I've just adjusted (caveat, still not tested or even compiled)

From 375bbb2d85fdf708858d726b6c7682e40dc07c2c Mon Sep 17 00:00:00 2001
From: Jim Meyering meyer...@redhat.com
Date: Wed, 2 Sep 2009 12:20:32 +0200
Subject: [PATCH 1/2] infra: define ATTRIBUTE_NONNULL to mark non-NULL parameters

* src/internal.h (ATTRIBUTE_NONNULL): Define.
---
 src/internal.h |8 
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/src/internal.h b/src/internal.h
index 936cd03..8fa579c 100644
--- a/src/internal.h
+++ b/src/internal.h
@@ -116,6 +116,14 @@
 #endif
 #endif

+#ifndef ATTRIBUTE_NONNULL
+# if __GNUC_PREREQ (3, 3)
+#  define ATTRIBUTE_NONNULL(m) __attribute__((__nonnull__(m)))
+# else
+#  define ATTRIBUTE_NONNULL(m)
+# endif
+#endif
+
 #else
 #ifndef ATTRIBUTE_UNUSED
 #define ATTRIBUTE_UNUSED
--
1.6.4.2.395.ge3d52


From 61dc45fcae8b17d9f619563d427f1bc74f44d553 Mon Sep 17 00:00:00 2001
From: Jim Meyering meyer...@redhat.com
Date: Wed, 2 Sep 2009 12:22:14 +0200
Subject: [PATCH 2/2] remote_internal.c: appease clang

* src/remote_internal.c (remoteNetworkOpen): Mark conn parameter
as non-NULL.  Remove now-unnecessary conn == NULL test.
(remoteDevMonOpen): Likewise.
---
 src/remote_internal.c |   10 --
 1 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/src/remote_internal.c b/src/remote_internal.c
index ea50c11..fe36ddd 100644
--- a/src/remote_internal.c
+++ b/src/remote_internal.c
@@ -3281,13 +3281,12 @@ done:
 static virDrvOpenStatus
 remoteNetworkOpen (virConnectPtr conn,
virConnectAuthPtr auth,
-   int flags)
+   int flags) ATTRIBUTE_NONNULL (1)
 {
 if (inside_daemon)
 return VIR_DRV_OPEN_DECLINED;

-if (conn 
-conn-driver 
+if (conn-driver 
 STREQ (conn-driver-name, remote)) {
 struct private_data *priv;

@@ -5130,13 +5129,12 @@ done:
 static virDrvOpenStatus
 remoteDevMonOpen(virConnectPtr conn,
  virConnectAuthPtr auth ATTRIBUTE_UNUSED,
- int flags ATTRIBUTE_UNUSED)
+ int flags ATTRIBUTE_UNUSED) ATTRIBUTE_NONNULL (1)
 {
 if (inside_daemon)
 return VIR_DRV_OPEN_DECLINED;

-if (conn 
-conn-driver 
+if (conn-driver 
 STREQ (conn-driver-name, remote)) {
 struct private_data *priv = conn-privateData;
 /* If we're here, the remote driver is already
--
1.6.4.2.395.ge3d52

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

Re: [libvirt] [PATCH] remote_internal.c: don't dereference a NULL conn

2009-09-02 Thread Jim Meyering 
Jim Meyering wrote:
 Daniel Veillard wrote:
 On Wed, Sep 02, 2009 at 11:58:07AM +0200, Jim Meyering wrote:
 Here's the test just before the else-if in the patch below:

 if (conn 
 conn-driver 
 STREQ (conn-driver-name, remote)) {

 So, in the else-branch, conn is guaranteed to be NULL.
 And dereferenced.

 This may be only a theoretical risk, but if so,
 the test of conn above should be changed to an assertion,
 and/or the parameter should get the nonnull attribute.

   I'm fine with your patch, so that even if code around changes
 having a gard is IMHO a good thing

 Thanks for the quick review.
 Daniel Berrange said he'd prefer the nonnull approach I mentioned above,
 so I've just adjusted (caveat, still not tested or even compiled)

From 375bbb2d85fdf708858d726b6c7682e40dc07c2c Mon Sep 17 00:00:00 2001
 From: Jim Meyering meyer...@redhat.com
 Date: Wed, 2 Sep 2009 12:20:32 +0200
 Subject: [PATCH 1/2] infra: define ATTRIBUTE_NONNULL to mark non-NULL 
 parameters

 * src/internal.h (ATTRIBUTE_NONNULL): Define.
...

From 61dc45fcae8b17d9f619563d427f1bc74f44d553 Mon Sep 17 00:00:00 2001
 From: Jim Meyering meyer...@redhat.com
 Date: Wed, 2 Sep 2009 12:22:14 +0200
 Subject: [PATCH 2/2] remote_internal.c: appease clang

 * src/remote_internal.c (remoteNetworkOpen): Mark conn parameter
 as non-NULL.  Remove now-unnecessary conn == NULL test.
 (remoteDevMonOpen): Likewise.

Note that this patch now address two reported NULL-deref problems,
not just one, like the original.

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

Re: [libvirt] [PATCH] remote_internal.c: don't dereference a NULL conn

2009-09-02 Thread Daniel P. Berrange 
On Wed, Sep 02, 2009 at 12:26:43PM +0200, Jim Meyering wrote:
 Daniel Veillard wrote:
  On Wed, Sep 02, 2009 at 11:58:07AM +0200, Jim Meyering wrote:
  Here's the test just before the else-if in the patch below:
 
  if (conn 
  conn-driver 
  STREQ (conn-driver-name, remote)) {
 
  So, in the else-branch, conn is guaranteed to be NULL.
  And dereferenced.
 
  This may be only a theoretical risk, but if so,
  the test of conn above should be changed to an assertion,
  and/or the parameter should get the nonnull attribute.
 
I'm fine with your patch, so that even if code around changes
  having a gard is IMHO a good thing
 
 Thanks for the quick review.
 Daniel Berrange said he'd prefer the nonnull approach I mentioned above,
 so I've just adjusted (caveat, still not tested or even compiled)

Yeah, for functions where it is expected that the passed in param
be non-NULL, then annotations are definitely the way togo. This 
lets the compiler/checkers validate the callers instead, avoiding
the need to clutter the methods with irrelevant NULL checks.

 
 From 375bbb2d85fdf708858d726b6c7682e40dc07c2c Mon Sep 17 00:00:00 2001
 From: Jim Meyering meyer...@redhat.com
 Date: Wed, 2 Sep 2009 12:20:32 +0200
 Subject: [PATCH 1/2] infra: define ATTRIBUTE_NONNULL to mark non-NULL 
 parameters
 
 * src/internal.h (ATTRIBUTE_NONNULL): Define.
 ---
  src/internal.h |8 
  1 files changed, 8 insertions(+), 0 deletions(-)
 
 diff --git a/src/internal.h b/src/internal.h
 index 936cd03..8fa579c 100644
 --- a/src/internal.h
 +++ b/src/internal.h
 @@ -116,6 +116,14 @@
  #endif
  #endif
 
 +#ifndef ATTRIBUTE_NONNULL
 +# if __GNUC_PREREQ (3, 3)
 +#  define ATTRIBUTE_NONNULL(m) __attribute__((__nonnull__(m)))
 +# else
 +#  define ATTRIBUTE_NONNULL(m)
 +# endif
 +#endif
 +
  #else
  #ifndef ATTRIBUTE_UNUSED
  #define ATTRIBUTE_UNUSED
 --
 1.6.4.2.395.ge3d52
 
 
 From 61dc45fcae8b17d9f619563d427f1bc74f44d553 Mon Sep 17 00:00:00 2001
 From: Jim Meyering meyer...@redhat.com
 Date: Wed, 2 Sep 2009 12:22:14 +0200
 Subject: [PATCH 2/2] remote_internal.c: appease clang
 
 * src/remote_internal.c (remoteNetworkOpen): Mark conn parameter
 as non-NULL.  Remove now-unnecessary conn == NULL test.
 (remoteDevMonOpen): Likewise.
 ---
  src/remote_internal.c |   10 --
  1 files changed, 4 insertions(+), 6 deletions(-)
 
 diff --git a/src/remote_internal.c b/src/remote_internal.c
 index ea50c11..fe36ddd 100644
 --- a/src/remote_internal.c
 +++ b/src/remote_internal.c
 @@ -3281,13 +3281,12 @@ done:
  static virDrvOpenStatus
  remoteNetworkOpen (virConnectPtr conn,
 virConnectAuthPtr auth,
 -   int flags)
 +   int flags) ATTRIBUTE_NONNULL (1)
  {
  if (inside_daemon)
  return VIR_DRV_OPEN_DECLINED;
 
 -if (conn 
 -conn-driver 
 +if (conn-driver 
  STREQ (conn-driver-name, remote)) {
  struct private_data *priv;
 
 @@ -5130,13 +5129,12 @@ done:
  static virDrvOpenStatus
  remoteDevMonOpen(virConnectPtr conn,
   virConnectAuthPtr auth ATTRIBUTE_UNUSED,
 - int flags ATTRIBUTE_UNUSED)
 + int flags ATTRIBUTE_UNUSED) ATTRIBUTE_NONNULL (1)
  {
  if (inside_daemon)
  return VIR_DRV_OPEN_DECLINED;
 
 -if (conn 
 -conn-driver 
 +if (conn-driver 
  STREQ (conn-driver-name, remote)) {
  struct private_data *priv = conn-privateData;
  /* If we're here, the remote driver is already

ACK


Daniel
-- 
|: Red Hat, Engineering, London   -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: [libvirt] [PATCH] storage_backend_fs: avoid NULL dereference on opendir failure

2009-09-02 Thread Daniel P. Berrange 
On Wed, Sep 02, 2009 at 10:05:03AM +0200, Jim Meyering wrote:
 From 7f453c68bc709d542e4c40a388c92c7969ad0a3a Mon Sep 17 00:00:00 2001
 From: Jim Meyering meyer...@redhat.com
 Date: Wed, 2 Sep 2009 09:58:50 +0200
 Subject: [PATCH 3/4] lxc: avoid NULL dereference when we find no mount point
 
 * src/lxc_container.c (lxcContainerUnmountOldFS): Don't pass
 a NULL pointer to qsort.
 ---
  src/lxc_container.c |5 +++--
  1 files changed, 3 insertions(+), 2 deletions(-)
 
 diff --git a/src/lxc_container.c b/src/lxc_container.c
 index 950dd50..2073864 100644
 --- a/src/lxc_container.c
 +++ b/src/lxc_container.c
 @@ -546,8 +546,9 @@ static int lxcContainerUnmountOldFS(void)
  }
  endmntent(procmnt);
 
 -qsort(mounts, nmounts, sizeof(mounts[0]),
 -  lxcContainerChildMountSort);
 +if (mounts)
 +qsort(mounts, nmounts, sizeof(mounts[0]),
 +  lxcContainerChildMountSort);
 
  for (i = 0 ; i  nmounts ; i++) {
  VIR_DEBUG(Umount %s, mounts[i]);

This would is impossible to hit, since you must at least have a /proc
filesystem if we've got this far, but doesn't hurt to check anyway :-)

ACK

Daniel
-- 
|: Red Hat, Engineering, London   -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: [libvirt] [PATCH] storage_backend_fs: avoid NULL dereference on opendir failure

2009-09-02 Thread Jim Meyering 
Daniel P. Berrange wrote:
 On Wed, Sep 02, 2009 at 10:05:03AM +0200, Jim Meyering wrote:
 From 7f453c68bc709d542e4c40a388c92c7969ad0a3a Mon Sep 17 00:00:00 2001
 From: Jim Meyering meyer...@redhat.com
 Date: Wed, 2 Sep 2009 09:58:50 +0200
 Subject: [PATCH 3/4] lxc: avoid NULL dereference when we find no mount point

 * src/lxc_container.c (lxcContainerUnmountOldFS): Don't pass
 a NULL pointer to qsort.
 ---
  src/lxc_container.c |5 +++--
  1 files changed, 3 insertions(+), 2 deletions(-)

 diff --git a/src/lxc_container.c b/src/lxc_container.c
 index 950dd50..2073864 100644
 --- a/src/lxc_container.c
 +++ b/src/lxc_container.c
 @@ -546,8 +546,9 @@ static int lxcContainerUnmountOldFS(void)
  }
  endmntent(procmnt);

 -qsort(mounts, nmounts, sizeof(mounts[0]),
 -  lxcContainerChildMountSort);
 +if (mounts)
 +qsort(mounts, nmounts, sizeof(mounts[0]),
 +  lxcContainerChildMountSort);

  for (i = 0 ; i  nmounts ; i++) {
  VIR_DEBUG(Umount %s, mounts[i]);

 This would is impossible to hit, since you must at least have a /proc
 filesystem if we've got this far, but doesn't hurt to check anyway :-)

It can be triggered when the first getmntent call fails.
I'll revise the log to mention that.

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

Re: [libvirt] PATCH: Don't blindly reorder disk drives

2009-09-02 Thread Daniel Veillard 
On Mon, Aug 17, 2009 at 03:34:38PM +0100, Daniel P. Berrange wrote:
 Calling qsort() on the disks array causes disk to be
 unneccessarily re-ordered, potentially breaking the
 ability to boot if the boot disk gets moved later in
 the list. The new algorithm will insert a new disk as
 far to the end of the list as possible, while being
 ordered correctly wrt other disks on the same bus.
 
 * src/domain_conf.c, src/domain_conf.h: Remove disk sorting
   routines. Add API to insert a disk into existing list at
   the optimal position, without resorting disks
 * src/libvirt_private.syms: Export virDomainDiskInsert
 * src/xend_internal.c, src/xm_internal.c: Remove calls to
   qsort, use virDomainDiskInsert instead.
 * src/qemu_driver.c: Remove calls to qsort, use virDoaminDiskInsert
   instead. Fix reordering bugs when hotunplugging disks and
   networks. Fix memory leak in disk/net unplug

  ACK, an important patch, we really should not drop this !

Daniel

-- 
Daniel Veillard  | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
dan...@veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/

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

Re: [libvirt] [RFC] Support for CPUID masking

2009-09-02 Thread Matthias Bolte 
2009/9/2 Jiri Denemark jdene...@redhat.com:
 Hi,

 We need to provide support for CPU ID masking. Xen and VMware ESX are examples
 of current hypervisors which support such masking.

[...]

 domain type='xen' id='42'
    ...
    features
        pae/
        acpi/
        apic/
        cpuid
            mask level='1' register='ebx'
                :::1010::::
            /mask
            mask level='1' register='ecx'
                ::::::xx1x:
            /mask
            mask level='1' register='edx'
                xxx1:::::::
            /mask
            mask level='8001' register='ecx'
                :::::::xx1x
            /mask
            mask level='8008' register='ecx'
                ::::::xx00:1001
            /mask
        /cpuid
    /features
    ...
 /domain


I like the proposed mapping for the domain XML, because it's an 1:1
mapping of what VMware uses in the VI API [1] and the VMX config.
Beside that VMware has two more possible values for the CPUID bits: H
and R. Both are used to define how to handle/interpret those bits in
the context of VMotion (migration).

For example the domain XML snippet above maps to this VMX snippet:

cpuid.1.ebx = 1010
cpuid.1.ecx = XX1X
cpuid.1.edx = XXX1
cpuid.8001.ecx = XX1X
cpuid.8008.ecx = XX001001

Matthias

[1] 
http://www.vmware.com/support/developer/vc-sdk/visdk400pubs/ReferenceGuide/vim.host.CpuIdInfo.html

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

Re: [libvirt] Re: OpenVZ : The restriction of domain name should be addressed

2009-09-02 Thread Daniel Veillard 
On Fri, Aug 21, 2009 at 11:08:05AM +0900, Yuji NISHIDA wrote:

 2009/7/24 Daniel P. Berrange berra...@redhat.com

 We should make use of this --name parameter then - I guess it didn't
 exist when we first wrote the driver. It is useful to users to have
 separate ID vs Name parameters - and in fact the current reusing of
 'ID' for the name, causes a little confusion in virsh's lookup  
 routines
 because it can't tell whether the parameter its given is a name or  
 an
 ID, since they are identical.

 There is still a question of how to specify both a name and CTID in  
 XML
 description.
 By default, CTID can be obtained as openvzGimmeFirstUnusedCTID(), but
 actually
 I think that there exists a number of persons interested in giving  
 CTIDs
 manually.

 Well, can domain id='' be used for CTID remaining name for
 alphabetical domain name?


 I worte a small patch and tried with following XML setting.

 ### Patch ###
 --- a/src/openvz_driver.c
 +++ b/src/openvz_driver.c
 @@ -130,9 +130,6 @@
  ADD_ARG_LIT(VZCTL);
  ADD_ARG_LIT(--quiet);
  ADD_ARG_LIT(create);
 -ADD_ARG_LIT(vmdef-id);
 -
 -ADD_ARG_LIT(--name);
  ADD_ARG_LIT(vmdef-name);

 ### XML ###
 domain id='100'
 nameabc/name

 I found the type of id was identified as number( obj-type ==  
 XPATH_NUMBER ) in virXPathLongBase,
 but it is clearly string before converted.
 I think correct path is to go in the first if context( obj-type ==  
 XPATH_STRING ) and run strtol.

  the id of domain must be a number, that's a requirement of libvirt
so when parsing we expect a number value.

 Then I tried with following XML.

 ### XML ###
 domain id=100
 nameabc/name

 I got following error.

 AttValue:  or ' expected

  yes, that's not XML ! you need ' or  around attributes, normal too.

 I'm not sure from which function should return this result.

  I'm not sure what you're trying to do but as is the behaviour of
  libvirt looks fine from your report.

Daniel

-- 
Daniel Veillard  | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
dan...@veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/

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

Re: [libvirt] UML driver struct refers to public API functions

2009-09-02 Thread Daniel Veillard 
On Mon, Aug 24, 2009 at 02:13:07PM +0100, Daniel P. Berrange wrote:
 On Sat, Aug 22, 2009 at 10:04:30PM +0200, Matthias Bolte wrote:
  Hi,
  
  The commit Generic shared impls of all NUMA apis
  (b0b968efd56f6c66bfa23eebbecd491ea993f99b) changed the UML driver
  struct to use the shared NUMA API. But now the UML driver struct
  refers to the public API functions:
  
  virNodeGetCellsFreeMemory
  virNodeGetFreeMemory
  
  instead of the shared NUMA API functions
  
  nodeGetCellsFreeMemory
  nodeGetFreeMemory
  
  This results in an infinite recursion, if someone's going to call
  virNodeGetCellsFreeMemory with an UML connection.
 
 Opps, that's a bit of a nasty bug. Clearly need to add these APis to
 the libvirt-TCK  tests

  Apparently the bug seems fixed in git:

static virDriver umlDriver = {
...
NULL, /* domainMemoryPeek */
nodeGetCellsFreeMemory, /* nodeGetCellsFreeMemory */
nodeGetFreeMemory,  /* getFreeMemory */
NULL, /* domainEventRegister */

 so issue seems solved now.

Daniel

-- 
Daniel Veillard  | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
dan...@veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/

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

Re: [libvirt] UML driver struct refers to public API functions

2009-09-02 Thread Matthias Bolte 
2009/9/2 Daniel Veillard veill...@redhat.com:
 On Mon, Aug 24, 2009 at 02:13:07PM +0100, Daniel P. Berrange wrote:
 On Sat, Aug 22, 2009 at 10:04:30PM +0200, Matthias Bolte wrote:
  Hi,
 
  The commit Generic shared impls of all NUMA apis
  (b0b968efd56f6c66bfa23eebbecd491ea993f99b) changed the UML driver
  struct to use the shared NUMA API. But now the UML driver struct
  refers to the public API functions:
 
  virNodeGetCellsFreeMemory
  virNodeGetFreeMemory
 
  instead of the shared NUMA API functions
 
  nodeGetCellsFreeMemory
  nodeGetFreeMemory
 
  This results in an infinite recursion, if someone's going to call
  virNodeGetCellsFreeMemory with an UML connection.

 Opps, that's a bit of a nasty bug. Clearly need to add these APis to
 the libvirt-TCK  tests

  Apparently the bug seems fixed in git:

 static virDriver umlDriver = {
 ...
    NULL, /* domainMemoryPeek */
    nodeGetCellsFreeMemory, /* nodeGetCellsFreeMemory */
    nodeGetFreeMemory,  /* getFreeMemory */
    NULL, /* domainEventRegister */

  so issue seems solved now.

 Daniel


Yes, Daniel P. Berrange fixed it 6 days ago in commit
83af0508007d787e74a57a1fca290f632a58dda7, but he didn't mention it on
the mailing list.

Matthias

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

Re: [libvirt] [PATCH 2/2] RPM spec file updated with glusterfs dependency

2009-09-02 Thread Daniel Veillard 
On Tue, Sep 01, 2009 at 11:51:22AM +0100, Daniel P. Berrange wrote:
 On Sat, Aug 29, 2009 at 12:20:11AM +0200, Gerrit Slomma wrote:
  On Tue, Jul 07, 2009 at 05:47:56AM -0700, Harshavardhana wrote:
   Add new dependency for glusterfs rpm.
  [...]
   +# For glusterfs 
   +Requires: glusterfs-client = 2.0.2
%endif
  
why 2.0.2 ? is taht a hard requirement ? In Fedora/Rawhide we have
  only 2.0.1 at the moment,
 
  Tested libvirt-0.7.0 today.
  And this requirement really breaks installation of builds on 
  RHEL/CentOS/ScientificLinux.
  The newest available built for glusterfs is those systems is 
  glusterfs-client-1.3.8 from freshrpms.
  Even --without-storage-fs does not help a bit, must be pulled in implicitly?
 
 No, its an explicitly RPM dependancy
 
  rpm -pq --requires libvirt-0.7.0-1.x86_64.rpm
  (...)
  glusterfs-client = 2.0.1
  (...)
  
  Is this patched in upcoming 0.7.1 or how could i go around this issue?
 
 We should hjust make this dependancy conditional onn fedora = 11, since
 no earlier Fedora or RHEL has this available.

  I see this was fixed in git, cool,

Daniel

-- 
Daniel Veillard  | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
dan...@veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/

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

Re: [libvirt] [PATCH] Support for getting/setting number of cpus in VirtualBox

2009-09-02 Thread Daniel Veillard 
On Fri, Aug 07, 2009 at 04:08:13PM +0200, Pritesh Kothari wrote:
 Hi All,
 
 I have just added support for getting/setting number of cpus in VirtualBox.
 The patch for the same is include below.

  Okay, applied and commited,

thanks !

Daniel

-- 
Daniel Veillard  | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
dan...@veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/

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

Re: [libvirt] VMware ESX: Allow ethernet address type 'vpx'

2009-09-02 Thread Daniel Veillard 
On Sat, Aug 08, 2009 at 11:56:10PM +0200, Matthias Bolte wrote:
 The VMX entry ethernet0.addressType may be set to 'vpx' beside
 'static' and 'generated'. 'vpx' indicates that the MAC address was
 generated by a vCenter.
 
 The attached patch adds 'vpx' to the valid values for ethernet0.addressType.

  Okay, I finally commited this patch :-)

thanks !

Daniel

-- 
Daniel Veillard  | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
dan...@veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/

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

Re: [libvirt] VMware ESX: Don't warn if a known query parameter should be ignored

2009-09-02 Thread Daniel Veillard 
On Sat, Aug 08, 2009 at 11:57:25PM +0200, Matthias Bolte wrote:
 esxUtil_ParseQuery() warns if a known query parameter should be
 ignored due to the corresponding char/int pointer being NULL, instead
 of silently ignoring it.
 
 The attached patch changes the if/else structure to fix this.

  Okay, makes sense, applied too :-)

   thanks !

Daniel

-- 
Daniel Veillard  | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
dan...@veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/

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

[libvirt] rebased multipath patch

2009-09-02 Thread Dave Allan 

Here's the multipath patch, rebased against the current head.

Dave
From 52cb0ff30dd5a52c5a246f3a2022f933cd2a0eab Mon Sep 17 00:00:00 2001
From: David Allan dal...@redhat.com
Date: Tue, 21 Jul 2009 12:43:01 -0400
Subject: [PATCH] Add a simple pool type for multipath devices

This pool type contains volumes for the multipath devices that are present on 
the host.  It does not (yet) support any sort of multipath configuration, so 
that aspect of system administration must still be done at host build time.
---
 configure.in|   24 +++
 po/POTFILES.in  |1 +
 src/Makefile.am |9 +
 src/storage_backend.c   |   82 ++
 src/storage_backend.h   |5 +-
 src/storage_backend_mpath.c |  352 +++
 src/storage_backend_mpath.h |   31 
 src/storage_conf.c  |7 +-
 src/storage_conf.h  |1 +
 9 files changed, 510 insertions(+), 2 deletions(-)
 create mode 100644 src/storage_backend_mpath.c
 create mode 100644 src/storage_backend_mpath.h

diff --git a/configure.in b/configure.in
index d28c44a..7c93ab7 100644
--- a/configure.in
+++ b/configure.in
@@ -1046,6 +1046,8 @@ AC_ARG_WITH([storage-iscsi],
 [  --with-storage-iscsiwith iSCSI backend for the storage driver 
(on)],[],[with_storage_iscsi=check])
 AC_ARG_WITH([storage-scsi],
 [  --with-storage-scsi with SCSI backend for the storage driver 
(on)],[],[with_storage_scsi=check])
+AC_ARG_WITH([storage-mpath],
+[  --with-storage-mpathwith mpath backend for the storage driver 
(on)],[],[with_storage_mpath=check])
 AC_ARG_WITH([storage-disk],
 [  --with-storage-disk with GPartd Disk backend for the storage driver 
(on)],[],[with_storage_disk=check])
 
@@ -1056,6 +1058,7 @@ if test $with_libvirtd = no; then
   with_storage_lvm=no
   with_storage_iscsi=no
   with_storage_scsi=no
+  with_storage_mpath=no
   with_storage_disk=no
 fi
 if test $with_storage_dir = yes ; then
@@ -1177,6 +1180,26 @@ if test $with_storage_scsi = check; then
 fi
 AM_CONDITIONAL([WITH_STORAGE_SCSI], [test $with_storage_scsi = yes])
 
+if test $with_storage_mpath = check; then
+   with_storage_mpath=yes
+
+   AC_DEFINE_UNQUOTED([WITH_STORAGE_MPATH], 1,
+ [whether mpath backend for storage driver is enabled])
+fi
+AM_CONDITIONAL([WITH_STORAGE_MPATH], [test $with_storage_mpath = yes])
+
+if test $with_storage_mpath = yes; then
+   DEVMAPPER_REQUIRED=0.0
+   DEVMAPPER_CFLAGS=
+   DEVMAPPER_LIBS=
+   PKG_CHECK_MODULES(DEVMAPPER, devmapper = $DEVMAPPER_REQUIRED,
+[], [
+AC_MSG_ERROR(
+[You must install device-mapper-devel = $DEVMAPPER_REQUIRED to compile 
libvirt])
+])
+fi
+AC_SUBST([DEVMAPPER_CFLAGS])
+AC_SUBST([DEVMAPPER_LIBS])
 
 LIBPARTED_CFLAGS=
 LIBPARTED_LIBS=
@@ -1677,6 +1700,7 @@ AC_MSG_NOTICE([   NetFS: $with_storage_fs])
 AC_MSG_NOTICE([ LVM: $with_storage_lvm])
 AC_MSG_NOTICE([   iSCSI: $with_storage_iscsi])
 AC_MSG_NOTICE([SCSI: $with_storage_scsi])
+AC_MSG_NOTICE([   mpath: $with_storage_mpath])
 AC_MSG_NOTICE([Disk: $with_storage_disk])
 AC_MSG_NOTICE([])
 AC_MSG_NOTICE([Security Drivers])
diff --git a/po/POTFILES.in b/po/POTFILES.in
index 0a53dab..1586368 100644
--- a/po/POTFILES.in
+++ b/po/POTFILES.in
@@ -38,6 +38,7 @@ src/storage_backend_fs.c
 src/storage_backend_iscsi.c
 src/storage_backend_logical.c
 src/storage_backend_scsi.c
+src/storage_backend_mpath.c
 src/storage_conf.c
 src/storage_driver.c
 src/storage_encryption_conf.c
diff --git a/src/Makefile.am b/src/Makefile.am
index bedeb84..cf3420b 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -199,6 +199,9 @@ STORAGE_DRIVER_ISCSI_SOURCES =  
\
 STORAGE_DRIVER_SCSI_SOURCES =  \
storage_backend_scsi.h storage_backend_scsi.c
 
+STORAGE_DRIVER_MPATH_SOURCES = \
+   storage_backend_mpath.h storage_backend_mpath.c
+
 STORAGE_DRIVER_DISK_SOURCES =  \
storage_backend_disk.h storage_backend_disk.c
 
@@ -478,6 +481,10 @@ if WITH_STORAGE_SCSI
 libvirt_driver_storage_la_SOURCES += $(STORAGE_DRIVER_SCSI_SOURCES)
 endif
 
+if WITH_STORAGE_MPATH
+libvirt_driver_storage_la_SOURCES += $(STORAGE_DRIVER_MPATH_SOURCES)
+endif
+
 if WITH_STORAGE_DISK
 libvirt_driver_storage_la_SOURCES += $(STORAGE_DRIVER_DISK_SOURCES)
 endif
@@ -539,6 +546,7 @@ EXTRA_DIST +=   
\
$(STORAGE_DRIVER_LVM_SOURCES)   \
$(STORAGE_DRIVER_ISCSI_SOURCES) \
$(STORAGE_DRIVER_SCSI_SOURCES)  \
+   $(STORAGE_DRIVER_MPATH_SOURCES) \
$(STORAGE_DRIVER_DISK_SOURCES)  \
$(NODE_DEVICE_DRIVER_SOURCES)   \
$(NODE_DEVICE_DRIVER_HAL_SOURCES)   \
@@ -607,6 +615,7 @@

Re: [libvirt] [RFC] Support for CPUID masking

2009-09-02 Thread Jim Paris 
Jiri Denemark wrote:
 Hi,
 
 We need to provide support for CPU ID masking. Xen and VMware ESX are examples
 of current hypervisors which support such masking.
 
 My proposal is to define new 'cpuid' feature advertised in guest
 capabilities:
...
 domain type='xen' id='42'
 ...
 features
 pae/
 acpi/
 apic/
 cpuid
 mask level='1' register='ebx'
 :::1010::::
 /mask
...
 What are your opinions about this?

I think it's too low-level, and the structure is x86-specific.  QEMU
and KVM compute their CPUID response based on arguments to the -cpu
argument, e.g.:

   -cpu core2duo,model=23,+ssse3,+lahf_lm

I think a similar structure makes more sense for libvirt, where the
configuration generally avoids big blocks of binary data, and the 
XML format should suit other architectures as well.

-jim

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

Re: [libvirt] [RFC] Support for CPUID masking

2009-09-02 Thread Daniel P. Berrange 
On Wed, Sep 02, 2009 at 11:59:39AM -0400, Jim Paris wrote:
 Jiri Denemark wrote:
  Hi,
  
  We need to provide support for CPU ID masking. Xen and VMware ESX are 
  examples
  of current hypervisors which support such masking.
  
  My proposal is to define new 'cpuid' feature advertised in guest
  capabilities:
 ...
  domain type='xen' id='42'
  ...
  features
  pae/
  acpi/
  apic/
  cpuid
  mask level='1' register='ebx'
  :::1010::::
  /mask
 ...
  What are your opinions about this?
 
 I think it's too low-level, and the structure is x86-specific.  QEMU
 and KVM compute their CPUID response based on arguments to the -cpu
 argument, e.g.:
 
-cpu core2duo,model=23,+ssse3,+lahf_lm
 
 I think a similar structure makes more sense for libvirt, where the
 configuration generally avoids big blocks of binary data, and the 
 XML format should suit other architectures as well.

I'm going back  forth on this too. We essentially have 3 options

 - Named CPU + flags/features
 - CPUID masks
 - Allow either


If we do either of the first two, we have to translate between the
two formats for one or more of the hypervisors. For the last one we
are just punting the problem off to applications.


If we choose CPUID, and made QEMU driver convert to named CPU + flags
we'd be stuck for non-x86 as you say.

If we chose named CPU + flags,  and made VMWare/Xen convert to raw
CPUID we'd potentially loose information if user had defined a config
with a raw CPUID mask outside context of libvirt. 

The other thing to remember is that CPUID also encodes sockets/cores/
threads  topology data, and it'd be very desirable to expose that in
a sensible fashion (ie not a bitmask).

On balance i'm currently leaning to named CPU + flags + expliciti
topology data because although its harder to implement for Xen/VMWare
I think its much nicer to applications  users. We might loose a tiny
bit of data in the CPU - named/flags conversion for Xen/VMWare but
I reckon we can get it good enough that most people won't really care
about that.

Daniel
-- 
|: Red Hat, Engineering, London   -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

[libvirt] virsh vcpuinfo always returns CPU=0

2009-09-02 Thread Shi Jin 
Hi there,

I wonder what does the CPU entry stands for in the output of vcpuinfo. I guess 
it is the CPU utilization rate, right?

For both Ubuntu 9.04 (virsh version 0.6.1) and RHEL 5.4 (version 0.6.3), I 
always get the returned CPU value to be 0, no matter how busy the CPU can be 
and how many VCPU is used.
For example,
onnecting to uri: qemu:///system
VCPU:   0
CPU:0
State:  running
CPU Affinity:   

VCPU:   1
CPU:0
State:  running
CPU Affinity:   


Is this a problem and how do I fix it?

Thank you very much.
--
Shi Jin, PhD


  

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

Re: [libvirt] virsh vcpuinfo always returns CPU=0

2009-09-02 Thread Daniel P. Berrange 
On Wed, Sep 02, 2009 at 09:37:15AM -0700, Shi Jin wrote:
 Hi there,
 
 I wonder what does the CPU entry stands for in the output of
 vcpuinfo. I guess it is the CPU utilization rate, right?

It is the most recent physical CPU in which the vCPU ran.

 For both Ubuntu 9.04 (virsh version 0.6.1) and RHEL 5.4 (version 
 0.6.3), I always get the returned CPU value to be 0, no matter 
 how busy the CPU can be and how many VCPU is used.
 For example,
 onnecting to uri: qemu:///system
 VCPU:   0
 CPU:0
 State:  running
 CPU Affinity:   
 
 VCPU:   1
 CPU:0
 State:  running
 CPU Affinity:   
 
 
 Is this a problem and how do I fix it?

Upgrade to newer libvirt with this patch in

http://libvirt.org/git/?p=libvirt.git;a=commit;h=c4a04dc0240589031ba1042f446095fb69222040

Daniel
-- 
|: Red Hat, Engineering, London   -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: [libvirt] [PATCH] Fix several memory leaks

2009-09-02 Thread Ryota Ozaki 
Hi Chris,

On Tue, Sep 1, 2009 at 4:45 AM, Chris Lalancetteclala...@redhat.com wrote:
 Ryota Ozaki wrote:
 diff --git a/qemud/qemud.c b/qemud/qemud.c
 index df275e6..17ba44a 100644
 --- a/qemud/qemud.c
 +++ b/qemud/qemud.c
 @@ -1733,7 +1733,7 @@ readmore:

          /* Possibly need to create another receive buffer */
          if ((client-nrequests  max_client_requests 
 -             VIR_ALLOC(client-rx)  0)) {
 +             !client-rx  VIR_ALLOC(client-rx)  0)) {
              qemudDispatchClientFailure(client);
          } else {
              if (client-rx)

 Hm, I'm not super familiar with this section of code, but this doesn't seem to
 be right.  How can client-rx ever be NULL here?  We dereference
 client-rx-bufferOffset very early on in the function, so we would have 
 crashed
 before we got here.  What am I missing?

No, you are probably right. As Daniel also says, the fix seems wrong.

I actually used valgrind to find memory leaks and valgrind pointed out here.
However, now the leak cannot be reproduced anymore, so probably I missed
something...

I'll drop this fix in the next patch.


 diff --git a/src/domain_conf.c b/src/domain_conf.c
 index 1d2cc7c..4b64219 100644
 --- a/src/domain_conf.c
 +++ b/src/domain_conf.c
 @@ -4361,6 +4361,7 @@ int virDomainSaveXML(virConnectPtr conn,
   cleanup:
      if (fd != -1)
          close(fd);
 +    VIR_FREE(configFile);
      return ret;
  }

 This one looks good.


 diff --git a/src/network_conf.c b/src/network_conf.c
 index bb649a4..58a4f32 100644
 --- a/src/network_conf.c
 +++ b/src/network_conf.c
 @@ -820,6 +820,7 @@ int virNetworkDeleteConfig(virConnectPtr conn,
  {
      char *configFile = NULL;
      char *autostartLink = NULL;
 +    int ret = -1;

      if ((configFile = virNetworkConfigFile(conn, configDir,
 net-def-name)) == NULL)
          goto error;
 @@ -836,12 +837,12 @@ int virNetworkDeleteConfig(virConnectPtr conn,
          goto error;
      }

 -    return 0;
 +    ret = 0;

  error:
      VIR_FREE(configFile);
      VIR_FREE(autostartLink);
 -    return -1;
 +    return ret;
  }

 This one looks good.


  char *virNetworkConfigFile(virConnectPtr conn,
 diff --git a/src/qemu_conf.c b/src/qemu_conf.c
 index 22f5edd..32d6a48 100644
 --- a/src/qemu_conf.c
 +++ b/src/qemu_conf.c
 @@ -1034,7 +1034,7 @@ qemudNetworkIfaceConnect(virConnectPtr conn,
                           virDomainNetDefPtr net,
                           int qemuCmdFlags)
  {
 -    char *brname;
 +    char *brname = NULL;
      int err;
      int tapfd = -1;
      int vnet_hdr = 0;
 @@ -1053,7 +1053,7 @@ qemudNetworkIfaceConnect(virConnectPtr conn,
          if (brname == NULL)
              return -1;
      } else if (net-type == VIR_DOMAIN_NET_TYPE_BRIDGE) {
 -        brname = net-data.bridge.brname;
 +        brname = strdup(net-data.bridge.brname);
      } else {
          qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
                           _(Network type %d is not supported), net-type);
 @@ -1063,7 +1063,7 @@ qemudNetworkIfaceConnect(virConnectPtr conn,
      if (!driver-brctl  (err = brInit(driver-brctl))) {
          virReportSystemError(conn, err, %s,
                               _(cannot initialize bridge support));
 -        return -1;
 +        goto cleanup;
      }

      if (!net-ifname ||
 @@ -1072,7 +1072,7 @@ qemudNetworkIfaceConnect(virConnectPtr conn,
          VIR_FREE(net-ifname);
          if (!(net-ifname = strdup(vnet%d))) {
              virReportOOMError(conn);
 -            return -1;
 +            goto cleanup;
          }
          /* avoid exposing vnet%d in dumpxml or error outputs */
          template_ifname = 1;
 @@ -1100,9 +1100,12 @@ qemudNetworkIfaceConnect(virConnectPtr conn,
          }
          if (template_ifname)
              VIR_FREE(net-ifname);
 -        return -1;
 +        tapfd = -1;
      }

 +cleanup:
 +    VIR_FREE(brname);
 +
      return tapfd;
  }

 This generally looks OK.


 diff --git a/src/storage_backend_fs.c b/src/storage_backend_fs.c
 index ca6d329..222e591 100644
 --- a/src/storage_backend_fs.c
 +++ b/src/storage_backend_fs.c
 @@ -856,6 +856,8 @@ virStorageBackendFileSystemRefresh(virConnectPtr conn,

          vol-type = VIR_STORAGE_VOL_FILE;
          vol-target.format = VIR_STORAGE_VOL_FILE_RAW; /* Real value
 is filled in during probe */
 +        if (vol-target.path)
 +            VIR_FREE(vol-target.path);

 Again, how can this ever be the case?  The first time through the loop vol is
 definitely NULL, so there's no possibility vol-target.path contains any
 allocated memory.  At the end of this loop, we assign the whole vol object to 
 a
 slot in the pool-volumes array, set vol to NULL, and then subsequent 
 iterations
 get a new vol object allocated.  So how can this ever be true?

I'm wrong again... I was too easy to add the fix along with the below fix.
I'll drop it as well.


          if (virAsprintf(vol-target.path, %s/%s,
                          pool-def-target.path,
            

Re: [libvirt] [PATCH] remote_internal.c: don't dereference a NULL conn

2009-09-02 Thread Paolo Bonzini 

Does __attribute__((__nonnull__())) really cover the case we're
concerned about here?


No, not at all.  Good catch.

Paolo

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

Re: [libvirt] [PATCH] Support configuration of huge pages in guests

2009-09-02 Thread john cooper 
Daniel P. Berrange wrote:
 * configure.in: Add check for mntent.h
 * qemud/libvirtd_qemu.aug, qemud/test_libvirtd_qemu.aug, src/qemu.conf
   Add 'hugetlbfs_mount' config parameter
 * src/qemu_conf.c, src/qemu_conf.h: Check for -mem-path flag in QEMU,
   and pass it when hugepages are requested.
   Load hugetlbfs_mount config parameter, search for mount if not given.
 * src/qemu_driver.c: Free hugetlbfs_mount/path parameter in driver shutdown.
   Create directory for QEMU hugepage usage, chowning if required.
 * docs/formatdomain.html.in: Document memoryBacking/hugepages elements
 * docs/schemas/domain.rng: Add memoryBacking/hugepages elements to schema
 * src/util.c, src/util.h, src/libvirt_private.syms: Add virFileFindMountPoint
   helper API
 * tests/qemuhelptest.c: Add -mem-path constants
 * tests/qemuxml2argvtest.c, tests/qemuxml2xmltest.c: Add tests for hugepage
   handling
 * tests/qemuxml2argvdata/qemuxml2argv-hugepages.xml,
   tests/qemuxml2argvdata/qemuxml2argv-hugepages.args: Data files for
   hugepage tests
 ---
  configure.in   |2 +-
  docs/formatdomain.html |8 +++-
  docs/formatdomain.html.in  |8 +++
  docs/schemas/domain.rng|9 
  qemud/libvirtd_qemu.aug|1 +
  qemud/test_libvirtd_qemu.aug   |4 ++
  src/domain_conf.c  |   10 -
  src/domain_conf.h  |1 +
  src/libvirt_private.syms   |1 +
  src/qemu.conf  |   13 +
  src/qemu_conf.c|   49 
 
  src/qemu_conf.h|3 +
  src/qemu_driver.c  |   34 ++
  src/util.c |   37 ++-
  src/util.h |4 ++
  tests/qemuhelptest.c   |6 ++-
  tests/qemuxml2argvdata/qemuxml2argv-hugepages.args |1 +
  tests/qemuxml2argvdata/qemuxml2argv-hugepages.xml  |   25 ++
  tests/qemuxml2argvtest.c   |7 ++-
  tests/qemuxml2xmltest.c|1 +
  20 files changed, 217 insertions(+), 7 deletions(-)
  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hugepages.args
  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hugepages.xml

I did a fetch on libvirt.git before reviewing and
it appears there is some code motion relative
to the version this patch was against.  Although
AFAICT nothing which appears to result in more than
patch bounce.

Looks good to me.  ACK.

-john

-- 
john.coo...@redhat.com

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