Re: [libvirt] libvirt and Parallels Cloud Storage

2013-02-11 Thread Daniel P. Berrange 
On Sun, Feb 10, 2013 at 02:20:02AM +0400, Alexander Gordeev wrote:
 В Thu, 7 Feb 2013 16:09:31 +
 Daniel P. Berrange berra...@redhat.com пишет:
 
   It's not a POSIX FS but there is a FUSE client for it that can be
   used to access and manipulate images. It's quite high speed but only
   when used with O_DIRECT + aio. I tried to setup several KVMs on top of
   a Pstorage mount using virt-manager. It worked good, but I had to:
   1. tune cache and IO settings for every disk
   2. disable space allocation by libvirt because it is using sync IO and
   is therefore slow
   
   I tried to find ways to solve the first issue and IMHO this can be
   done by adding a way to specify per-pool defaults for cache and IO
   settings. I didn't find any way for this in the current code and UI.
   I'd like to add a new storage backend also that will be a 'dir' backend
   with extra ability to manage Pstorage mount points and UI integration.
   I'd like to merge my work to the main tree when it's finished if
   possible.
  
  I don't think that putting cache/IO defaults in the XML is really
  appropriate. There are too many different scenarios which want
  different settings to be able to clearly identify one set of
  perfect settings. I see this as primarily a documentation problem.
  Someone needs to write something describing the diferrent settings
  for each storage pool  what the pros/cons are for each. Downstream
  app developers can then make decisions about suitable defaults for
  their use cases
 
 If you are against changes in the XML maybe you are ok with a simpler
 option: I create a storage backend for Pstorage, add two fields for
 cache and io (or maybe just flags) to virStoragePoolTypeInfo and set
 the right values for the new pool type. Then add some code to check
 them to qemuBuildDriveStr (and to other drivers if possible, of course).
 So no changes to XML and API. Is this better?

No, the hypervisor drivers can only access data that is available via
from the storage drivers via the XML or APIs. As I said before this is
a policy decision for application authors to take, not for libvirt
itself.

Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

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

Re: [libvirt] [PATCH 1/5] Public API to allow defining new domain using OVA file

2013-02-11 Thread Daniel P. Berrange 
On Fri, Feb 08, 2013 at 05:26:24PM -0800, Ata Bohra wrote:
 
 
 
  NACK,  as I said with previous postings, this does not belong in
  libvirt APIs, it should be built as a layer above. 
 [AB]: Thanks for reviewing this Daniel. I completly understand the concern of 
 not making it part of libvirt API, but as discussed in other thread 
 (https://www.redhat.com/archives/libvir-list/2012-December/msg00385.html) I 
 was unable to find API supported way to 
 upload OVA disk to the ESX server (which is an integral part of OVA install). 

See the virStorageVolUpload API.

 Further this API design fundamentally flawed as it is assuming the hypervisor
  driver can access files that are on the libvirt application
  machine which is not the case in general.
 [AB]: If possible please suggest if there can be any other way to overcome
   this design flaw.  
 Further, there are two unrealted reviews that I posted fixing issues with
 exisitng ESX driver, it would be useful if someone can review them.

Again see the design of the virStorageVolUpload API

Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

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

Re: [libvirt] libvirt and Parallels Cloud Storage

2013-02-11 Thread Alexander Gordeev 
В Mon, 11 Feb 2013 09:57:05 +
Daniel P. Berrange berra...@redhat.com пишет:

 On Sun, Feb 10, 2013 at 02:20:02AM +0400, Alexander Gordeev wrote:
  В Thu, 7 Feb 2013 16:09:31 +
  Daniel P. Berrange berra...@redhat.com пишет:
  
It's not a POSIX FS but there is a FUSE client for it that can be
used to access and manipulate images. It's quite high speed but only
when used with O_DIRECT + aio. I tried to setup several KVMs on top of
a Pstorage mount using virt-manager. It worked good, but I had to:
1. tune cache and IO settings for every disk
2. disable space allocation by libvirt because it is using sync IO and
is therefore slow

I tried to find ways to solve the first issue and IMHO this can be
done by adding a way to specify per-pool defaults for cache and IO
settings. I didn't find any way for this in the current code and UI.
I'd like to add a new storage backend also that will be a 'dir' backend
with extra ability to manage Pstorage mount points and UI integration.
I'd like to merge my work to the main tree when it's finished if
possible.
   
   I don't think that putting cache/IO defaults in the XML is really
   appropriate. There are too many different scenarios which want
   different settings to be able to clearly identify one set of
   perfect settings. I see this as primarily a documentation problem.
   Someone needs to write something describing the diferrent settings
   for each storage pool  what the pros/cons are for each. Downstream
   app developers can then make decisions about suitable defaults for
   their use cases
  
  If you are against changes in the XML maybe you are ok with a simpler
  option: I create a storage backend for Pstorage, add two fields for
  cache and io (or maybe just flags) to virStoragePoolTypeInfo and set
  the right values for the new pool type. Then add some code to check
  them to qemuBuildDriveStr (and to other drivers if possible, of course).
  So no changes to XML and API. Is this better?
 
 No, the hypervisor drivers can only access data that is available via
 from the storage drivers via the XML or APIs. As I said before this is
 a policy decision for application authors to take, not for libvirt
 itself.

Ok, I certainly see the point about policy decisions but shouldn't
libvirt store the selected policy somehow? :)

-- 
  Alexander

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

Re: [libvirt] [PATCH 1/4] qemu: Add checking in helpers for sgio setting

2013-02-11 Thread Osier Yang 

On 2013年02月09日 04:21, John Ferlan wrote:

On 02/08/2013 08:07 AM, Osier Yang wrote:

This moves the checking into the helpers, to avoid the
callers missing the checking.
---
  src/qemu/qemu_conf.c|   20 
  src/qemu/qemu_conf.h|4 ++--
  src/qemu/qemu_driver.c  |   18 +++---
  src/qemu/qemu_process.c |   21 -
  4 files changed, 37 insertions(+), 26 deletions(-)

diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 17f7d45..69ba710 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -726,12 +726,20 @@ qemuGetSharedDiskKey(const char *disk_path)
   */
  int
  qemuAddSharedDisk(virHashTablePtr sharedDisks,
-  const char *disk_path)
+  virDomainDiskDefPtr disk)
  {
  size_t *ref = NULL;
  char *key = NULL;

-if (!(key = qemuGetSharedDiskKey(disk_path)))
+/* Currently the only conflicts we have to care about
+ * for the shared disk is sgio setting, which is only
+ * valid for block disk.
+ */
+if (disk-type != VIR_DOMAIN_DISK_TYPE_BLOCK ||
+!disk-shared || !disk-src)
+return 0;
+
+if (!(key = qemuGetSharedDiskKey(disk-src)))
  return -1;

  if ((ref = virHashLookup(sharedDisks, key))) {
@@ -755,12 +763,16 @@ qemuAddSharedDisk(virHashTablePtr sharedDisks,
   */
  int
  qemuRemoveSharedDisk(virHashTablePtr sharedDisks,
- const char *disk_path)
+ virDomainDiskDefPtr disk)
  {
  size_t *ref = NULL;
  char *key = NULL;

-if (!(key = qemuGetSharedDiskKey(disk_path)))
+if (disk-type != VIR_DOMAIN_DISK_TYPE_BLOCK ||
+!disk-shared || !disk-src)
+return 0;


[2]


+
+if (!(key = qemuGetSharedDiskKey(disk-src)))
  return -1;

  if (!(ref = virHashLookup(sharedDisks, key))) {
diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
index 60c4109..8e84bcf 100644
--- a/src/qemu/qemu_conf.h
+++ b/src/qemu/qemu_conf.h
@@ -270,11 +270,11 @@ void qemuDriverCloseCallbackRunAll(virQEMUDriverPtr 
driver,
 virConnectPtr conn);

  int qemuAddSharedDisk(virHashTablePtr sharedDisks,
-  const char *disk_path)
+  virDomainDiskDefPtr disk)
  ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);

  int qemuRemoveSharedDisk(virHashTablePtr sharedDisks,
- const char *disk_path)
+ virDomainDiskDefPtr disk)
  ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
  char * qemuGetSharedDiskKey(const char *disk_path)
  ATTRIBUTE_NONNULL(1);
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 979a027..043efd3 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -5855,11 +5855,9 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn,
  }

  if (ret == 0) {
-if (disk-type == VIR_DOMAIN_DISK_TYPE_BLOCK  disk-shared) {
-if (qemuAddSharedDisk(driver-sharedDisks, disk-src)  0)
-VIR_WARN(Failed to add disk '%s' to shared disk table,
- disk-src);
-}
+if (qemuAddSharedDisk(driver-sharedDisks, disk)  0)
+VIR_WARN(Failed to add disk '%s' to shared disk table,
+ disk-src);

  if (qemuSetUnprivSGIO(disk)  0)
  VIR_WARN(Failed to set unpriv_sgio of disk '%s', disk-src);


Does there need to be a NULLSTR(disk-src)?  Doesn't seem so, but just
checking.  I only note this because the qemuAddSharedDisk() has a
!disk-src check prior to returning zero.


qemuSetUnprivSGIO returns 0 as long as disk-src is NULL too. See [1].




@@ -5980,12 +5978,10 @@ qemuDomainDetachDeviceDiskLive(virQEMUDriverPtr driver,
  break;
  }

-if (ret == 0
-disk-type == VIR_DOMAIN_DISK_TYPE_BLOCK
-disk-shared) {
-if (qemuRemoveSharedDisk(driver-sharedDisks, disk-src)  0)
- VIR_WARN(Failed to remove disk '%s' from shared disk table,
-  disk-src);
+if (ret == 0) {
+if (qemuRemoveSharedDisk(driver-sharedDisks, disk)  0)
+VIR_WARN(Failed to remove disk '%s' from shared disk table,
+ disk-src);


Similar comment here w/r/t NULLSTR and checks in Remove code


Likewise. See [2].




  }

  return ret;
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 30f923a..bc171a4 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -3453,6 +3453,13 @@ qemuSetUnprivSGIO(virDomainDiskDefPtr disk)
  {
  int val = -1;

+/* sgio is only valid for block disk; cdrom
+ * and floopy disk can have empty source.
+ */
+if (disk-type != VIR_DOMAIN_DISK_TYPE_BLOCK ||
+!disk-src)
+return 0;


[1]


+
  if (disk-sgio)
  val = (disk-sgio == VIR_DOMAIN_DISK_SGIO_UNFILTERED);

@@ -3875,13 +3882,11 @@ int qemuProcessStart(virConnectPtr conn,
 _(Raw I/O is not

Re: [libvirt] [PATCH 1/4] qemu: Add checking in helpers for sgio setting

2013-02-11 Thread Daniel P. Berrange 
On Mon, Feb 11, 2013 at 06:35:42PM +0800, Osier Yang wrote:
 On 2013年02月09日 04:21, John Ferlan wrote:
 On 02/08/2013 08:07 AM, Osier Yang wrote:
 This moves the checking into the helpers, to avoid the
 callers missing the checking.
 ---
   src/qemu/qemu_conf.c|   20 
   src/qemu/qemu_conf.h|4 ++--
   src/qemu/qemu_driver.c  |   18 +++---
   src/qemu/qemu_process.c |   21 -
   4 files changed, 37 insertions(+), 26 deletions(-)
 
 diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
 index 17f7d45..69ba710 100644
 --- a/src/qemu/qemu_conf.c
 +++ b/src/qemu/qemu_conf.c
 @@ -726,12 +726,20 @@ qemuGetSharedDiskKey(const char *disk_path)
*/
   int
   qemuAddSharedDisk(virHashTablePtr sharedDisks,
 -  const char *disk_path)
 +  virDomainDiskDefPtr disk)
   {
   size_t *ref = NULL;
   char *key = NULL;
 
 -if (!(key = qemuGetSharedDiskKey(disk_path)))
 +/* Currently the only conflicts we have to care about
 + * for the shared disk is sgio setting, which is only
 + * valid for block disk.
 + */
 +if (disk-type != VIR_DOMAIN_DISK_TYPE_BLOCK ||
 +!disk-shared || !disk-src)
 +return 0;
 +
 +if (!(key = qemuGetSharedDiskKey(disk-src)))
   return -1;
 
   if ((ref = virHashLookup(sharedDisks, key))) {
 @@ -755,12 +763,16 @@ qemuAddSharedDisk(virHashTablePtr sharedDisks,
*/
   int
   qemuRemoveSharedDisk(virHashTablePtr sharedDisks,
 - const char *disk_path)
 + virDomainDiskDefPtr disk)
   {
   size_t *ref = NULL;
   char *key = NULL;
 
 -if (!(key = qemuGetSharedDiskKey(disk_path)))
 +if (disk-type != VIR_DOMAIN_DISK_TYPE_BLOCK ||
 +!disk-shared || !disk-src)
 +return 0;
 
 [2]
 
 +
 +if (!(key = qemuGetSharedDiskKey(disk-src)))
   return -1;
 
   if (!(ref = virHashLookup(sharedDisks, key))) {
 diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
 index 60c4109..8e84bcf 100644
 --- a/src/qemu/qemu_conf.h
 +++ b/src/qemu/qemu_conf.h
 @@ -270,11 +270,11 @@ void qemuDriverCloseCallbackRunAll(virQEMUDriverPtr 
 driver,
  virConnectPtr conn);
 
   int qemuAddSharedDisk(virHashTablePtr sharedDisks,
 -  const char *disk_path)
 +  virDomainDiskDefPtr disk)
   ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
 
   int qemuRemoveSharedDisk(virHashTablePtr sharedDisks,
 - const char *disk_path)
 + virDomainDiskDefPtr disk)
   ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
   char * qemuGetSharedDiskKey(const char *disk_path)
   ATTRIBUTE_NONNULL(1);
 diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
 index 979a027..043efd3 100644
 --- a/src/qemu/qemu_driver.c
 +++ b/src/qemu/qemu_driver.c
 @@ -5855,11 +5855,9 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn,
   }
 
   if (ret == 0) {
 -if (disk-type == VIR_DOMAIN_DISK_TYPE_BLOCK  disk-shared) {
 -if (qemuAddSharedDisk(driver-sharedDisks, disk-src)  0)
 -VIR_WARN(Failed to add disk '%s' to shared disk table,
 - disk-src);
 -}
 +if (qemuAddSharedDisk(driver-sharedDisks, disk)  0)
 +VIR_WARN(Failed to add disk '%s' to shared disk table,
 + disk-src);
 
   if (qemuSetUnprivSGIO(disk)  0)
   VIR_WARN(Failed to set unpriv_sgio of disk '%s', disk-src);
 
 Does there need to be a NULLSTR(disk-src)?  Doesn't seem so, but just
 checking.  I only note this because the qemuAddSharedDisk() has a
 !disk-src check prior to returning zero.
 
 qemuSetUnprivSGIO returns 0 as long as disk-src is NULL too. See [1].

If disk-type == NETWORK, then de-referencing disk-src has potential to
SEGV the entire process, since that field is not valid.

 
 
 @@ -5980,12 +5978,10 @@ qemuDomainDetachDeviceDiskLive(virQEMUDriverPtr 
 driver,
   break;
   }
 
 -if (ret == 0
 -disk-type == VIR_DOMAIN_DISK_TYPE_BLOCK
 -disk-shared) {
 -if (qemuRemoveSharedDisk(driver-sharedDisks, disk-src)  0)
 - VIR_WARN(Failed to remove disk '%s' from shared disk table,
 -  disk-src);
 +if (ret == 0) {
 +if (qemuRemoveSharedDisk(driver-sharedDisks, disk)  0)
 +VIR_WARN(Failed to remove disk '%s' from shared disk table,
 + disk-src);
 
 Similar comment here w/r/t NULLSTR and checks in Remove code
 
 Likewise. See [2].

Again you must *not* de-reference disk-src without first validating
the disk-type value.


Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-

Re: [libvirt] [PATCH 2/4] qemu: Merge qemuCheckSharedDisk into qemuAddSharedDisk

2013-02-11 Thread Osier Yang 

On 2013年02月09日 04:46, John Ferlan wrote:

On 02/08/2013 08:08 AM, Osier Yang wrote:

Based on moving various checking into qemuAddSharedDisk, this
avoids the caller using it in wrong ways.
---
  src/qemu/qemu_conf.c|   50 
  src/qemu/qemu_driver.c  |5 
  src/qemu/qemu_process.c |   53 ---
  src/qemu/qemu_process.h |3 --
  4 files changed, 50 insertions(+), 61 deletions(-)

diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 69ba710..3eeea4b 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -721,6 +721,53 @@ qemuGetSharedDiskKey(const char *disk_path)
  return key;
  }

+/* Check if a shared disk's setting conflicts with the conf
+ * used by other domain(s). Currently only checks the sgio
+ * setting. Note that this should only be called for disk with
+ * block source.
+ *
+ * Returns 0 if no conflicts, otherwise returns -1.
+ */
+static int
+qemuCheckSharedDisk(virHashTablePtr sharedDisks,
+virDomainDiskDefPtr disk)
+{
+int val;
+size_t *ref = NULL;
+char *key = NULL;
+int ret = 0;
+
+if (!(key = qemuGetSharedDiskKey(disk-src)))
+return -1;
+
+/* It can't be conflict if no other domain is
+ * is sharing it.
+ */
+if (!(ref = virHashLookup(sharedDisks, key)))
+goto cleanup;
+
+if (virGetDeviceUnprivSGIO(disk-src, NULL,val)  0) {
+ret = -1;
+goto cleanup;
+}
+
+if ((val == 0
+ (disk-sgio == VIR_DOMAIN_DISK_SGIO_FILTERED ||
+  disk-sgio == VIR_DOMAIN_DISK_SGIO_DEFAULT)) ||
+(val == 1
+ disk-sgio == VIR_DOMAIN_DISK_SGIO_UNFILTERED))
+goto cleanup;
+
+virReportError(VIR_ERR_OPERATION_INVALID,
+   _(sgio of shared disk '%s' conflicts with other 
+ active domains), disk-src);
+ret = -1;
+
+cleanup:
+VIR_FREE(key);
+return ret;
+}
+
  /* Increase ref count if the entry already exists, otherwise
   * add a new entry.
   */
@@ -739,6 +786,9 @@ qemuAddSharedDisk(virHashTablePtr sharedDisks,
  !disk-shared || !disk-src)
  return 0;

+if (qemuCheckSharedDisk(sharedDisks, disk)  0)
+return -1;
+


I take the order previously in qemuProcessStart() was wrong then - it
was adding it first, then checking if it was shared.  The DiskLive code
checked shared first, then added, which does seem more correct.


See PATCH 3/4, the reason for adding it first, and then checking, in
qemuProcessStart, is to avoid removing the hash entry which belongs to
other domain(s) when doing cleanup for failing on starting.

But it has flaws, because it still could removes the hash entry belongs
to other domain(s) if it fails before the block to add the hash entry
 set unpriv_sgio.

The reason for checking first, and then adding in DiskLive is that
there is no risk to remove hash entry of other domains. But it also
has flaws for [1], which works for qemuProcessStart, but not for
DiskLive. I have ever added a new argument to the checking method
so that it knowns whether should return 0 when the ref count is
1 in a previous version. But it's not that nice, and I changed it
by using a bitmap for qemuProcessStop PATCH 3/4 of this version.



However, the change is subtle enough to make me wonder about the reason
for the ordering of calls in the DiskLive code that would check shared
before calls to qemuDomainDetermineDiskChain() and qemuSetupDiskCgroup()


There is no special reason, we can also put the checking after these
calls. Just I think checking first is more effective in case of
there is error when checking.



Seems as though now the code in DiskLive may need some extra backout
code if the add now fails because of the check


It's done in PATCH 4/4.





  if (!(key = qemuGetSharedDiskKey(disk-src)))
  return -1;

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 043efd3..1dc9789 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -5798,11 +5798,6 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn,
  goto end;
  }

-if (disk-type == VIR_DOMAIN_DISK_TYPE_BLOCK
-disk-shared
-(qemuCheckSharedDisk(driver-sharedDisks, disk)  0))
-goto end;
-
  if (qemuDomainDetermineDiskChain(driver, disk, false)  0)
  goto end;

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index bc171a4..1e0876c 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -3483,56 +3483,6 @@ qemuSetUnprivSGIO(virDomainDiskDefPtr disk)
  return 0;
  }

-/* Check if a shared disk's setting conflicts with the conf
- * used by other domain(s). Currently only checks the sgio
- * setting. Note that this should only be called for disk with
- * block source.
- *
- * Returns 0 if no conflicts, otherwise returns -1.
- */
-int
-qemuCheckSharedDisk(virHashTablePtr sharedDisks,
-

Re: [libvirt] [PATCH 4/4] qemu: Move shared disk entry adding and unpriv_sgio seting

2013-02-11 Thread Daniel P. Berrange 
On Fri, Feb 08, 2013 at 09:08:02PM +0800, Osier Yang wrote:
 The disk def could be free'ed by qemuDomainChangeEjectableMedia
 for cdrom or floppy disk. This moves the adding and setting before
 the attaching takes place. And for cdrom floppy disk, if the
 change is ejecting, removing the existed hash entry for it.
 ---
  src/qemu/qemu_driver.c  |   23 +--
  src/qemu/qemu_hotplug.c |6 +-
  src/qemu/qemu_hotplug.h |3 ++-
  3 files changed, 20 insertions(+), 12 deletions(-)
 
 diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
 index 03fe526..4aad42f 100644
 --- a/src/qemu/qemu_driver.c
 +++ b/src/qemu/qemu_driver.c
 @@ -5789,6 +5789,7 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn,
  {
  virDomainDiskDefPtr disk = dev-data.disk;
  virCgroupPtr cgroup = NULL;
 +int eject, added;
  int ret = -1;
  
  if (disk-driverName != NULL  !STREQ(disk-driverName, qemu)) {
 @@ -5798,6 +5799,12 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn,
  goto end;
  }
  
 +if (qemuAddSharedDisk(driver-sharedDisks, disk, added)  0)
 +goto end;
 +
 +if (qemuSetUnprivSGIO(disk)  0)
 +goto end;
 +
  if (qemuDomainDetermineDiskChain(driver, disk, false)  0)
  goto end;
  
 @@ -5814,7 +5821,7 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn,
  switch (disk-device)  {
  case VIR_DOMAIN_DISK_DEVICE_CDROM:
  case VIR_DOMAIN_DISK_DEVICE_FLOPPY:
 -ret = qemuDomainChangeEjectableMedia(driver, vm, disk, false);
 +ret = qemuDomainChangeEjectableMedia(driver, vm, disk, false, 
 eject);
  break;
  case VIR_DOMAIN_DISK_DEVICE_DISK:
  case VIR_DOMAIN_DISK_DEVICE_LUN:
 @@ -5843,22 +5850,18 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn,
  break;
  }
  
 +if (ret == 0  eject)
 +ignore_value(qemuRemoveSharedDisk(driver-sharedDisks, disk));

This doesn't make sense - you're removing the disk we just added.
You need to remove the *old* disk-src surely ? In addition it is
*not* valid to reference 'disk' at all at this point, since the
functions we just called may have free'd it.

Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

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

Re: [libvirt] [PATCH 3/4] qemu: Don't remove hash entry of other domains

2013-02-11 Thread Daniel P. Berrange 
On Fri, Feb 08, 2013 at 09:08:01PM +0800, Osier Yang wrote:
 qemuProcessStart invokes qemuProcessStop when fails, to avoid
 removing hash entry which belongs to other domain(s), this introduces
 a new virBitmapPtr argument for qemuProcessStop. And qemuProcessStart
 sets the bit for the disk only if it's successfully added into the
 hash table. Thus if the argument is provided for qemuProcessStop, it
 can't remove the hash entry belongs to other domain(s).

I find this approach rather dubious - IMHO it is a sign that you're
not recording enough information in the shared disk hash. eg perhaps
the hash ought to be recording the UUID of each VM that is holding
a reference. That way you're protected from qemuProcessStop() trying
todo something wrong.

Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

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

Re: [libvirt] [PATCH 1/4] qemu: Add checking in helpers for sgio setting

2013-02-11 Thread Osier Yang 

On 2013年02月11日 18:48, Daniel P. Berrange wrote:

On Mon, Feb 11, 2013 at 06:35:42PM +0800, Osier Yang wrote:

On 2013年02月09日 04:21, John Ferlan wrote:

On 02/08/2013 08:07 AM, Osier Yang wrote:

This moves the checking into the helpers, to avoid the
callers missing the checking.
---
  src/qemu/qemu_conf.c|   20 
  src/qemu/qemu_conf.h|4 ++--
  src/qemu/qemu_driver.c  |   18 +++---
  src/qemu/qemu_process.c |   21 -
  4 files changed, 37 insertions(+), 26 deletions(-)

diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 17f7d45..69ba710 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -726,12 +726,20 @@ qemuGetSharedDiskKey(const char *disk_path)
   */
  int
  qemuAddSharedDisk(virHashTablePtr sharedDisks,
-  const char *disk_path)
+  virDomainDiskDefPtr disk)
  {
  size_t *ref = NULL;
  char *key = NULL;

-if (!(key = qemuGetSharedDiskKey(disk_path)))
+/* Currently the only conflicts we have to care about
+ * for the shared disk is sgio setting, which is only
+ * valid for block disk.
+ */
+if (disk-type != VIR_DOMAIN_DISK_TYPE_BLOCK ||
+!disk-shared || !disk-src)
+return 0;
+
+if (!(key = qemuGetSharedDiskKey(disk-src)))
  return -1;

  if ((ref = virHashLookup(sharedDisks, key))) {
@@ -755,12 +763,16 @@ qemuAddSharedDisk(virHashTablePtr sharedDisks,
   */
  int
  qemuRemoveSharedDisk(virHashTablePtr sharedDisks,
- const char *disk_path)
+ virDomainDiskDefPtr disk)
  {
  size_t *ref = NULL;
  char *key = NULL;

-if (!(key = qemuGetSharedDiskKey(disk_path)))
+if (disk-type != VIR_DOMAIN_DISK_TYPE_BLOCK ||
+!disk-shared || !disk-src)
+return 0;


[2]


+
+if (!(key = qemuGetSharedDiskKey(disk-src)))
  return -1;

  if (!(ref = virHashLookup(sharedDisks, key))) {
diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
index 60c4109..8e84bcf 100644
--- a/src/qemu/qemu_conf.h
+++ b/src/qemu/qemu_conf.h
@@ -270,11 +270,11 @@ void qemuDriverCloseCallbackRunAll(virQEMUDriverPtr 
driver,
 virConnectPtr conn);

  int qemuAddSharedDisk(virHashTablePtr sharedDisks,
-  const char *disk_path)
+  virDomainDiskDefPtr disk)
  ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);

  int qemuRemoveSharedDisk(virHashTablePtr sharedDisks,
- const char *disk_path)
+ virDomainDiskDefPtr disk)
  ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
  char * qemuGetSharedDiskKey(const char *disk_path)
  ATTRIBUTE_NONNULL(1);
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 979a027..043efd3 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -5855,11 +5855,9 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn,
  }

  if (ret == 0) {
-if (disk-type == VIR_DOMAIN_DISK_TYPE_BLOCK   disk-shared) {
-if (qemuAddSharedDisk(driver-sharedDisks, disk-src)   0)
-VIR_WARN(Failed to add disk '%s' to shared disk table,
- disk-src);
-}
+if (qemuAddSharedDisk(driver-sharedDisks, disk)   0)
+VIR_WARN(Failed to add disk '%s' to shared disk table,
+ disk-src);

  if (qemuSetUnprivSGIO(disk)   0)
  VIR_WARN(Failed to set unpriv_sgio of disk '%s', disk-src);


Does there need to be a NULLSTR(disk-src)?  Doesn't seem so, but just
checking.  I only note this because the qemuAddSharedDisk() has a
!disk-src check prior to returning zero.


qemuSetUnprivSGIO returns 0 as long as disk-src is NULL too. See [1].


If disk-type == NETWORK, then de-referencing disk-src has potential to
SEGV the entire process, since that field is not valid.


There is checking in this version:

/* sgio is only valid for block disk; cdrom
 * and floopy disk can have empty source.
 */
if (disk-type != VIR_DOMAIN_DISK_TYPE_BLOCK ||
!disk-src)
return 0;

So for a network disk, it has no chance to de-reference disk-src
if the return value  0.






@@ -5980,12 +5978,10 @@ qemuDomainDetachDeviceDiskLive(virQEMUDriverPtr driver,
  break;
  }

-if (ret == 0
-disk-type == VIR_DOMAIN_DISK_TYPE_BLOCK
-disk-shared) {
-if (qemuRemoveSharedDisk(driver-sharedDisks, disk-src)   0)
- VIR_WARN(Failed to remove disk '%s' from shared disk table,
-  disk-src);
+if (ret == 0) {
+if (qemuRemoveSharedDisk(driver-sharedDisks, disk)   0)
+VIR_WARN(Failed to remove disk '%s' from shared disk table,
+ disk-src);


Similar comment here w/r/t NULLSTR and checks in Remove code


Likewise. See [2].


Again you must *not* de-reference disk-src without first validating
the disk-type value.



Likewise.



Daniel

Re: [libvirt] [PATCH 1/4] qemu: Add checking in helpers for sgio setting

2013-02-11 Thread Daniel P. Berrange 
On Mon, Feb 11, 2013 at 07:09:29PM +0800, Osier Yang wrote:
 On 2013年02月11日 18:48, Daniel P. Berrange wrote:
 On Mon, Feb 11, 2013 at 06:35:42PM +0800, Osier Yang wrote:
 On 2013年02月09日 04:21, John Ferlan wrote:
 On 02/08/2013 08:07 AM, Osier Yang wrote:
 This moves the checking into the helpers, to avoid the
 callers missing the checking.
 ---
   src/qemu/qemu_conf.c|   20 
   src/qemu/qemu_conf.h|4 ++--
   src/qemu/qemu_driver.c  |   18 +++---
   src/qemu/qemu_process.c |   21 -
   4 files changed, 37 insertions(+), 26 deletions(-)
 
 diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
 index 17f7d45..69ba710 100644
 --- a/src/qemu/qemu_conf.c
 +++ b/src/qemu/qemu_conf.c
 @@ -726,12 +726,20 @@ qemuGetSharedDiskKey(const char *disk_path)
*/
   int
   qemuAddSharedDisk(virHashTablePtr sharedDisks,
 -  const char *disk_path)
 +  virDomainDiskDefPtr disk)
   {
   size_t *ref = NULL;
   char *key = NULL;
 
 -if (!(key = qemuGetSharedDiskKey(disk_path)))
 +/* Currently the only conflicts we have to care about
 + * for the shared disk is sgio setting, which is only
 + * valid for block disk.
 + */
 +if (disk-type != VIR_DOMAIN_DISK_TYPE_BLOCK ||
 +!disk-shared || !disk-src)
 +return 0;
 +
 +if (!(key = qemuGetSharedDiskKey(disk-src)))
   return -1;
 
   if ((ref = virHashLookup(sharedDisks, key))) {
 @@ -755,12 +763,16 @@ qemuAddSharedDisk(virHashTablePtr sharedDisks,
*/
   int
   qemuRemoveSharedDisk(virHashTablePtr sharedDisks,
 - const char *disk_path)
 + virDomainDiskDefPtr disk)
   {
   size_t *ref = NULL;
   char *key = NULL;
 
 -if (!(key = qemuGetSharedDiskKey(disk_path)))
 +if (disk-type != VIR_DOMAIN_DISK_TYPE_BLOCK ||
 +!disk-shared || !disk-src)
 +return 0;
 
 [2]
 
 +
 +if (!(key = qemuGetSharedDiskKey(disk-src)))
   return -1;
 
   if (!(ref = virHashLookup(sharedDisks, key))) {
 diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
 index 60c4109..8e84bcf 100644
 --- a/src/qemu/qemu_conf.h
 +++ b/src/qemu/qemu_conf.h
 @@ -270,11 +270,11 @@ void qemuDriverCloseCallbackRunAll(virQEMUDriverPtr 
 driver,
  virConnectPtr conn);
 
   int qemuAddSharedDisk(virHashTablePtr sharedDisks,
 -  const char *disk_path)
 +  virDomainDiskDefPtr disk)
   ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
 
   int qemuRemoveSharedDisk(virHashTablePtr sharedDisks,
 - const char *disk_path)
 + virDomainDiskDefPtr disk)
   ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
   char * qemuGetSharedDiskKey(const char *disk_path)
   ATTRIBUTE_NONNULL(1);
 diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
 index 979a027..043efd3 100644
 --- a/src/qemu/qemu_driver.c
 +++ b/src/qemu/qemu_driver.c
 @@ -5855,11 +5855,9 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn,
   }
 
   if (ret == 0) {
 -if (disk-type == VIR_DOMAIN_DISK_TYPE_BLOCK   disk-shared) {
 -if (qemuAddSharedDisk(driver-sharedDisks, disk-src)   0)
 -VIR_WARN(Failed to add disk '%s' to shared disk table,
 - disk-src);
 -}
 +if (qemuAddSharedDisk(driver-sharedDisks, disk)   0)
 +VIR_WARN(Failed to add disk '%s' to shared disk table,
 + disk-src);
 
   if (qemuSetUnprivSGIO(disk)   0)
   VIR_WARN(Failed to set unpriv_sgio of disk '%s', 
  disk-src);
 
 Does there need to be a NULLSTR(disk-src)?  Doesn't seem so, but just
 checking.  I only note this because the qemuAddSharedDisk() has a
 !disk-src check prior to returning zero.
 
 qemuSetUnprivSGIO returns 0 as long as disk-src is NULL too. See [1].
 
 If disk-type == NETWORK, then de-referencing disk-src has potential to
 SEGV the entire process, since that field is not valid.
 
 There is checking in this version:
 
 /* sgio is only valid for block disk; cdrom
  * and floopy disk can have empty source.
  */
 if (disk-type != VIR_DOMAIN_DISK_TYPE_BLOCK ||
 !disk-src)
 return 0;
 
 So for a network disk, it has no chance to de-reference disk-src
 if the return value  0.

Hmm, that is rather unclear, and looking at the code is also just
something we don't need. These methods are doing virRaiseError,
so we shouldn't also be doing VIR_WARN - just remove these VIR_WARN
lines from all this code.

Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

--
libvir-list mailing list

Re: [libvirt] [PATCH 1/4] qemu: Add checking in helpers for sgio setting

2013-02-11 Thread Osier Yang 

On 2013年02月11日 19:14, Daniel P. Berrange wrote:

On Mon, Feb 11, 2013 at 07:09:29PM +0800, Osier Yang wrote:

On 2013年02月11日 18:48, Daniel P. Berrange wrote:

On Mon, Feb 11, 2013 at 06:35:42PM +0800, Osier Yang wrote:

On 2013年02月09日 04:21, John Ferlan wrote:

On 02/08/2013 08:07 AM, Osier Yang wrote:

This moves the checking into the helpers, to avoid the
callers missing the checking.
---
  src/qemu/qemu_conf.c|   20 
  src/qemu/qemu_conf.h|4 ++--
  src/qemu/qemu_driver.c  |   18 +++---
  src/qemu/qemu_process.c |   21 -
  4 files changed, 37 insertions(+), 26 deletions(-)

diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 17f7d45..69ba710 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -726,12 +726,20 @@ qemuGetSharedDiskKey(const char *disk_path)
   */
  int
  qemuAddSharedDisk(virHashTablePtr sharedDisks,
-  const char *disk_path)
+  virDomainDiskDefPtr disk)
  {
  size_t *ref = NULL;
  char *key = NULL;

-if (!(key = qemuGetSharedDiskKey(disk_path)))
+/* Currently the only conflicts we have to care about
+ * for the shared disk is sgio setting, which is only
+ * valid for block disk.
+ */
+if (disk-type != VIR_DOMAIN_DISK_TYPE_BLOCK ||
+!disk-shared || !disk-src)
+return 0;
+
+if (!(key = qemuGetSharedDiskKey(disk-src)))
  return -1;

  if ((ref = virHashLookup(sharedDisks, key))) {
@@ -755,12 +763,16 @@ qemuAddSharedDisk(virHashTablePtr sharedDisks,
   */
  int
  qemuRemoveSharedDisk(virHashTablePtr sharedDisks,
- const char *disk_path)
+ virDomainDiskDefPtr disk)
  {
  size_t *ref = NULL;
  char *key = NULL;

-if (!(key = qemuGetSharedDiskKey(disk_path)))
+if (disk-type != VIR_DOMAIN_DISK_TYPE_BLOCK ||
+!disk-shared || !disk-src)
+return 0;


[2]


+
+if (!(key = qemuGetSharedDiskKey(disk-src)))
  return -1;

  if (!(ref = virHashLookup(sharedDisks, key))) {
diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
index 60c4109..8e84bcf 100644
--- a/src/qemu/qemu_conf.h
+++ b/src/qemu/qemu_conf.h
@@ -270,11 +270,11 @@ void qemuDriverCloseCallbackRunAll(virQEMUDriverPtr 
driver,
 virConnectPtr conn);

  int qemuAddSharedDisk(virHashTablePtr sharedDisks,
-  const char *disk_path)
+  virDomainDiskDefPtr disk)
  ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);

  int qemuRemoveSharedDisk(virHashTablePtr sharedDisks,
- const char *disk_path)
+ virDomainDiskDefPtr disk)
  ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
  char * qemuGetSharedDiskKey(const char *disk_path)
  ATTRIBUTE_NONNULL(1);
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 979a027..043efd3 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -5855,11 +5855,9 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn,
  }

  if (ret == 0) {
-if (disk-type == VIR_DOMAIN_DISK_TYPE_BLOCKdisk-shared) {
-if (qemuAddSharedDisk(driver-sharedDisks, disk-src)0)
-VIR_WARN(Failed to add disk '%s' to shared disk table,
- disk-src);
-}
+if (qemuAddSharedDisk(driver-sharedDisks, disk)0)
+VIR_WARN(Failed to add disk '%s' to shared disk table,
+ disk-src);

  if (qemuSetUnprivSGIO(disk)0)
  VIR_WARN(Failed to set unpriv_sgio of disk '%s', disk-src);


Does there need to be a NULLSTR(disk-src)?  Doesn't seem so, but just
checking.  I only note this because the qemuAddSharedDisk() has a
!disk-src check prior to returning zero.


qemuSetUnprivSGIO returns 0 as long as disk-src is NULL too. See [1].


If disk-type == NETWORK, then de-referencing disk-src has potential to
SEGV the entire process, since that field is not valid.


There is checking in this version:

 /* sgio is only valid for block disk; cdrom
  * and floopy disk can have empty source.
  */
 if (disk-type != VIR_DOMAIN_DISK_TYPE_BLOCK ||
 !disk-src)
 return 0;

So for a network disk, it has no chance to de-reference disk-src
if the return value  0.


Hmm, that is rather unclear, and looking at the code is also just
something we don't need. These methods are doing virRaiseError,
so we shouldn't also be doing VIR_WARN - just remove these VIR_WARN
lines from all this code.



They are removed in 4/4. :)

Osier

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

Re: [libvirt] [PATCH 3/4] qemu: Don't remove hash entry of other domains

2013-02-11 Thread Osier Yang 

On 2013年02月09日 05:09, John Ferlan wrote:

On 02/08/2013 08:08 AM, Osier Yang wrote:

qemuProcessStart invokes qemuProcessStop when fails, to avoid
removing hash entry which belongs to other domain(s), this introduces
a new virBitmapPtr argument for qemuProcessStop. And qemuProcessStart
sets the bit for the disk only if it's successfully added into the
hash table. Thus if the argument is provided for qemuProcessStop, it
can't remove the hash entry belongs to other domain(s).
---
  src/qemu/qemu_conf.c  |5 -
  src/qemu/qemu_conf.h  |3 ++-
  src/qemu/qemu_driver.c|   21 +++--
  src/qemu/qemu_migration.c |   14 +++---
  src/qemu/qemu_process.c   |   37 +
  src/qemu/qemu_process.h   |3 ++-
  6 files changed, 55 insertions(+), 28 deletions(-)

diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 3eeea4b..a6162b6 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -773,7 +773,8 @@ cleanup:
   */
  int
  qemuAddSharedDisk(virHashTablePtr sharedDisks,
-  virDomainDiskDefPtr disk)
+  virDomainDiskDefPtr disk,
+  int *added)
  {
  size_t *ref = NULL;
  char *key = NULL;
@@ -804,6 +805,8 @@ qemuAddSharedDisk(virHashTablePtr sharedDisks,
  }
  }

+if (added)
+*added = 1;
  VIR_FREE(key);
  return 0;
  }
diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
index 8e84bcf..b8b5275 100644
--- a/src/qemu/qemu_conf.h
+++ b/src/qemu/qemu_conf.h
@@ -270,7 +270,8 @@ void qemuDriverCloseCallbackRunAll(virQEMUDriverPtr driver,
 virConnectPtr conn);

  int qemuAddSharedDisk(virHashTablePtr sharedDisks,
-  virDomainDiskDefPtr disk)
+  virDomainDiskDefPtr disk,
+  int *added)
  ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);

  int qemuRemoveSharedDisk(virHashTablePtr sharedDisks,
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 1dc9789..03fe526 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -2098,7 +2098,7 @@ qemuDomainDestroyFlags(virDomainPtr dom,
  goto endjob;
  }

-qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_DESTROYED, 0);
+qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_DESTROYED, 0, NULL);
  event = virDomainEventNewFromObj(vm,
   VIR_DOMAIN_EVENT_STOPPED,
   VIR_DOMAIN_EVENT_STOPPED_DESTROYED);
@@ -2944,7 +2944,7 @@ qemuDomainSaveInternal(virQEMUDriverPtr driver, 
virDomainPtr dom,
  goto endjob;

  /* Shut it down */
-qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_SAVED, 0);
+qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_SAVED, 0, NULL);
  virDomainAuditStop(vm, saved);
  event = virDomainEventNewFromObj(vm,
   VIR_DOMAIN_EVENT_STOPPED,
@@ -3393,7 +3393,7 @@ static int qemuDomainCoreDump(virDomainPtr dom,

  endjob:
  if ((ret == 0)  (flags  VIR_DUMP_CRASH)) {
-qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_CRASHED, 0);
+qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_CRASHED, 0, NULL);
  virDomainAuditStop(vm, crashed);
  event = virDomainEventNewFromObj(vm,
   VIR_DOMAIN_EVENT_STOPPED,
@@ -4902,7 +4902,7 @@ qemuDomainSaveImageStartVM(virConnectPtr conn,
  }

  if (virCommandWait(cmd, NULL)  0) {
-qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED, 0);
+qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED, 0, NULL);
  ret = -1;
  }
  VIR_DEBUG(Decompression binary stderr: %s, NULLSTR(errbuf));
@@ -5850,7 +5850,7 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn,
  }

  if (ret == 0) {
-if (qemuAddSharedDisk(driver-sharedDisks, disk)  0)
+if (qemuAddSharedDisk(driver-sharedDisks, disk, NULL)  0)
  VIR_WARN(Failed to add disk '%s' to shared disk table,
   disk-src);

@@ -10677,7 +10677,7 @@ qemuDomainSnapshotCreateActiveInternal(virConnectPtr 
conn,
  if (flags  VIR_DOMAIN_SNAPSHOT_CREATE_HALT) {
  event = virDomainEventNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED,
   
VIR_DOMAIN_EVENT_STOPPED_FROM_SNAPSHOT);
-qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT, 0);
+qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT, 0, NULL);
  virDomainAuditStop(vm, from-snapshot);
  /* We already filtered the _HALT flag for persistent domains
   * only, so this end job never drops the last reference.  */
@@ -11232,7 +11232,7 @@ qemuDomainSnapshotCreateActiveExternal(virConnectPtr 
conn,

  event = virDomainEventNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED,

Re: [libvirt] [PATCH 4/4] qemu: Move shared disk entry adding and unpriv_sgio seting

2013-02-11 Thread Osier Yang 

On 2013年02月11日 18:55, Daniel P. Berrange wrote:

On Fri, Feb 08, 2013 at 09:08:02PM +0800, Osier Yang wrote:

The disk def could be free'ed by qemuDomainChangeEjectableMedia
for cdrom or floppy disk. This moves the adding and setting before
the attaching takes place. And for cdrom floppy disk, if the
change is ejecting, removing the existed hash entry for it.
---
  src/qemu/qemu_driver.c  |   23 +--
  src/qemu/qemu_hotplug.c |6 +-
  src/qemu/qemu_hotplug.h |3 ++-
  3 files changed, 20 insertions(+), 12 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 03fe526..4aad42f 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -5789,6 +5789,7 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn,
  {
  virDomainDiskDefPtr disk = dev-data.disk;
  virCgroupPtr cgroup = NULL;
+int eject, added;
  int ret = -1;

  if (disk-driverName != NULL  !STREQ(disk-driverName, qemu)) {
@@ -5798,6 +5799,12 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn,
  goto end;
  }

+if (qemuAddSharedDisk(driver-sharedDisks, disk,added)  0)
+goto end;
+
+if (qemuSetUnprivSGIO(disk)  0)
+goto end;
+
  if (qemuDomainDetermineDiskChain(driver, disk, false)  0)
  goto end;

@@ -5814,7 +5821,7 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn,
  switch (disk-device)  {
  case VIR_DOMAIN_DISK_DEVICE_CDROM:
  case VIR_DOMAIN_DISK_DEVICE_FLOPPY:
-ret = qemuDomainChangeEjectableMedia(driver, vm, disk, false);
+ret = qemuDomainChangeEjectableMedia(driver, vm, disk, false,eject);
  break;
  case VIR_DOMAIN_DISK_DEVICE_DISK:
  case VIR_DOMAIN_DISK_DEVICE_LUN:
@@ -5843,22 +5850,18 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn,
  break;
  }

+if (ret == 0  eject)
+ignore_value(qemuRemoveSharedDisk(driver-sharedDisks, disk));


This doesn't make sense - you're removing the disk we just added.


No, the removing only happens when the operation is to eject the
media of CD-ROM or Floppy. It's determined by eject.


You need to remove the *old* disk-src surely ?


Yes, if the operation is ejecting.


In addition it is
*not* valid to reference 'disk' at all at this point, since the
functions we just called may have free'd it.


Oh, yes, I should copy the disk def before
qemuDomainChangeEjectableMedia takes place.

Osier

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

Re: [libvirt] [PATCH 3/4] qemu: Don't remove hash entry of other domains

2013-02-11 Thread Osier Yang 

On 2013年02月11日 18:59, Daniel P. Berrange wrote:

On Fri, Feb 08, 2013 at 09:08:01PM +0800, Osier Yang wrote:

qemuProcessStart invokes qemuProcessStop when fails, to avoid
removing hash entry which belongs to other domain(s), this introduces
a new virBitmapPtr argument for qemuProcessStop. And qemuProcessStart
sets the bit for the disk only if it's successfully added into the
hash table. Thus if the argument is provided for qemuProcessStop, it
can't remove the hash entry belongs to other domain(s).


I find this approach rather dubious - IMHO it is a sign that you're
not recording enough information in the shared disk hash. eg perhaps
the hash ought to be recording the UUID of each VM that is holding
a reference. That way you're protected from qemuProcessStop() trying
todo something wrong.



I'm doubting about if it's really deserved to maintain a bunch of
arrays in the hash entry. As we only need the recording for
qemuProcessStart, it's much simpler to only use a bitmap to record
the added disks for a VM in qemuProcessStart from my p.o.v.

Osier

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

Re: [libvirt] [PATCH 4/4] Remove qemuDriverLock from almost everywhere

2013-02-11 Thread Daniel P. Berrange 
On Thu, Feb 07, 2013 at 05:46:38PM +, Daniel P. Berrange wrote:
 From: Daniel P. Berrange berra...@redhat.com
 
 With the majority of fields in the virQEMUDriverPtr struct
 now immutable or self-locking, there is no need for practically
 any methods to be using the QEMU driver lock. Only a handful
 of helper APIs in qemu_conf.c now need it
 ---
  src/qemu/THREADS.txt| 194 +++-
  src/qemu/qemu_conf.c|  50 --
  src/qemu/qemu_domain.c  | 213 +-
  src/qemu/qemu_domain.h  |  40 +
  src/qemu/qemu_driver.c  | 386 
 +---
  src/qemu/qemu_hotplug.c | 118 ++--
  src/qemu/qemu_migration.c   |  66 ---
  src/qemu/qemu_process.c | 223 ---
  src/qemu/qemu_process.h |   3 +-
  src/security/security_selinux.c |  28 +--
  10 files changed, 374 insertions(+), 947 deletions(-)

Don't bother reviewing this one yet, it is flawed.
I will be sending a new version

Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

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

Re: [libvirt] Adding new standalone API

2013-02-11 Thread Osier Yang 

On 2013年02月11日 14:27, Bilal Ahmad wrote:

Hi all,

I am learning to hack libvirt and so far for basic things, I am able to
get things done. For the advanced things like adding a completely new
API, I will need some help from you people. For starters, I have decided
to add a custom network API with only name and UUID and get my
custom network API to work. I would really appreciate if you could guide
me. I have gone through the “*Implementing a new API in Libvirt*” but
its an example which introduces new APIs to an existing API type. To
learn better, I plan to implement my own standalone sample API and get
it to work.


You can pick examples in the git log:

Such as virNetworkUpdate (commit 574b9bc66b, and several
commits on top of it).

And after that, if you are not confident about how to
implement it, please post RFC about your thoughts first
before implementation, including what you are try to
do, and how you plan to do it.


I would really appreciate help from this open-source community.



Hope it helps.


Thanks,

Bilal



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


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

Re: [libvirt] [PATCH V4 1/3] Add a class for file descriptor sets

2013-02-11 Thread Daniel P. Berrange 
On Thu, Feb 07, 2013 at 10:35:14PM -0500, Stefan Berger wrote:
 Rather than passing the next-to-use file descriptor set Id
 and the hash table for rembering the mappings of aliases to
 file descriptor sets around, encapsulate the two in a class.
 
 Signed-off-by: Stefan Berger stef...@linux.vnet.ibm.com

Nit-picking, I'd rename  Fdset to FdSet since we usually
capitalize each new word, and Fd and Set are really separate
words here.

 Index: libvirt/src/util/virfdset.h
 ===
 --- /dev/null
 +++ libvirt/src/util/virfdset.h
 +#ifndef __FDSET_H__
 +# define __FDSET_H__
 +
 +# include internal.h
 +# include virbuffer.h
 +# include virxml.h
 +# include virhash.h
 +
 +typedef struct _virFdset virFdset;
 +typedef virFdset *virFdsetPtr;
 +
 +struct _virFdset {
 +virHashTablePtr aliasToFdset;
 +unsigned int nextfdset;
 +};

It'd be preferrable for the struct to be in the .c file to make
the class representation completely opaque to callers.

I'd also suggest making it a virObject

 +/**
 + * virFdsetInit
 + * @fdset: fdset
 + *
 + * Initialize the @fdset.
 + * Returns 0 on success, -1 on failure.
 + */
 +int virFdsetInit(virFdsetPtr fdset);

I'd prefer this to use our more normal paradigm of

   virFdsetPtr virFdsetNew(void);



 +
 +/**
 + * virFdsetReset
 + * @fdset: fdset
 + *
 + * Reset the @fdset and forget about all mappings
 + * of aliases to file descriptor set data
 + */
 +void virFdsetReset(virFdsetPtr fdset);
 +
 +/**
 + * virFdsetClear
 + * @fdset: the fdset
 + *
 + * Free all memory associated with the @fdset but do not free
 + * the fdset structure itself. This is the counter part to
 + * virFdsetInit.
 + */
 +void virFdsetClear(virFdsetPtr fdset);

And just rely on virObjectUnref()

 +
 +
 +/**
 + * virFdsetRemoveAlias
 + * @fdset: the fdset
 + * @alias: the alias to remove
 + *
 + * Remove the given alias' mapping from @fdset
 + */
 +void virFdsetRemoveAlias(virFdsetPtr fdset, const char *alias);

  ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);

 +
 +
 +/**
 + * virFdsetNextSet
 + * @fdset: fdset
 + * @alias: device alias
 + * @fdset: pointer to unsigned int for storing the file descriptor set id
 + *
 + * Get the next file descriptor set number and store it with the given
 + * @alias. If successful, return the file descriptor set id in @fdsetnum.
 + *
 + * Returns 0 on success, -1 on failure.
 + */
 +int virFdsetNextSet(virFdsetPtr fdset, const char *alias,
 +unsigned int *fdsetnum);

  ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3);


 +/**
 + * virFdsetFormatXML
 + * @fdset: fdset
 + * @buf: virBufferPtr for writing into
 + *
 + * Write XML representation of the @fdset into the buffer @buf.
 + */
 +void virFdsetFormatXML(virFdsetPtr fdset, virBufferPtr buf);

  ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);

 +
 +/**
 + * virFdsetParseXML
 + * @fdset: fdset
 + * @xPath: xpath expression to find the @fdset's XML nodes
 + * @ctxt: xpath context
 + *
 + * Parse the fdset XML representation and collect the data into @fdset.
 + *
 + * Returns 0 on success, -1 on failure.
 + */
 +int virFdsetParseXML(virFdsetPtr fdset, const char *xPath,
 + xmlXPathContextPtr ctxt);

  ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3);


Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

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

Re: [libvirt] [PATCH V4 2/3] Introduce file descriptor set for QEMU domains

2013-02-11 Thread Daniel P. Berrange 
On Thu, Feb 07, 2013 at 10:35:15PM -0500, Stefan Berger wrote:
 Extend the QEMU private domain structure with virFdset.
 Persist the virFdset using XML and parse its XML.
 Reset the fdset upon domain stop.
 
 Stefan Berger stef...@linux.vnet.ibm.com
 
 ---
  src/qemu/qemu_domain.c  |   10 ++
  src/qemu/qemu_domain.h  |3 +++
  src/qemu/qemu_process.c |2 ++
  3 files changed, 15 insertions(+)
 

 Index: libvirt/src/qemu/qemu_domain.h
 ===
 --- libvirt.orig/src/qemu/qemu_domain.h
 +++ libvirt/src/qemu/qemu_domain.h
 @@ -32,6 +32,7 @@
  # include qemu_conf.h
  # include qemu_capabilities.h
  # include virchrdev.h
 +# include virfdset.h
  
  # define QEMU_EXPECTED_VIRT_TYPES  \
  ((1  VIR_DOMAIN_VIRT_QEMU) | \
 @@ -160,6 +161,8 @@ struct _qemuDomainObjPrivate {
  qemuDomainCleanupCallback *cleanupCallbacks;
  size_t ncleanupCallbacks;
  size_t ncleanupCallbacks_max;
 +
 +virFdset fdset;

s/virFdset/virFdsetPtr/ so we can just use the normal alloc/free
pattern for this.


Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

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

Re: [libvirt] [PATCH V4 3/3] Add support for file descriptor sets

2013-02-11 Thread Daniel P. Berrange 
On Thu, Feb 07, 2013 at 10:35:16PM -0500, Stefan Berger wrote:
 Add support for file descriptor sets by converting some of the 
 command line parameters to use /dev/fdset/%d if -add-fd is found
 to be supported by QEMU. For those devices libvirt now open()s
 the device to obtain the file descriptor and 'transfers' the 
 fd using virCommand.
 
 For the following fragments of domain XML
 
 
 disk type='file' device='disk'
   driver name='qemu' type='raw'/
   source file='/var/lib/libvirt/images/fc14-x86_64.img'/
   target dev='hda' bus='ide'/
   address type='drive' controller='0' bus='0' target='0' unit='0'/
 /disk
 
serial type='dev'
   source path='/dev/ttyS0'/
   target port='0'/
 /serial
 serial type='pipe'
   source path='/tmp/testpipe'/
   target port='1'/
 /serial
 
 libvirt now creates the following parts for the QEMU command line:
 
 old: -drive 
 file=/var/lib/libvirt/images/fc14-x86_64.img,if=none,id=drive-ide0-0-0,format=raw
 new: -add-fd set=1,fd=23,opaque=RDONLY:/var/lib/libvirt/images/fc14-x86_64.img
  -add-fd set=1,fd=24,opaque=RDWR:/var/lib/libvirt/images/fc14-x86_64.img
  -drive file=/dev/fdset/1,if=none,id=drive-ide0-0-0,format=raw
 
 old: -chardev tty,id=charserial0,path=/dev/ttyS0
 new: -add-fd set=1,fd=30,opaque=/dev/ttyS0
  -chardev tty,id=charserial0,path=/dev/fdset/1
 
 old: -chardev pipe,id=charserial1,path=/tmp/testpipe
 new: -add-fd set=2,fd=32,opaque=/tmp/testpipe
  -chardev pipe,id=charserial1,path=/dev/fdset/2
 
 Test cases are part of this patch now.
 
 Signed-off-by: Stefan Berger stef...@linux.vnet.ibm.com
 

 @@ -201,6 +206,7 @@ out:
  virCommandFree(cmd);
  virDomainDefFree(vmdef);
  virObjectUnref(conn);
 +virFdsetClear(fdset);
  return ret;
  }
  
 @@ -875,6 +881,11 @@ mymain(void)
  QEMU_CAPS_DEVICE, QEMU_CAPS_DEVICE_VIDEO_PRIMARY,
  QEMU_CAPS_DEVICE_QXL, QEMU_CAPS_DEVICE_QXL_VGA);
  
 +DO_TEST(no-add-fd,
 +QEMU_CAPS_CHARDEV, QEMU_CAPS_DEVICE, QEMU_CAPS_DRIVE);

Do we really need this test case - don't all existing test cases
already validate correct CLI args in the non-fdset case ?

 +DO_TEST(add-fd, QEMU_CAPS_ADD_FD,
 +QEMU_CAPS_CHARDEV, QEMU_CAPS_DEVICE, QEMU_CAPS_DRIVE);

Thanks for adding this.


Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

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

Re: [libvirt] [PATCH] qemu: Fix potential crash when attaching/detaching cdrom or floppy

2013-02-11 Thread Osier Yang 

On 2013年02月09日 06:23, Eric Blake wrote:

On 02/07/2013 06:21 AM, Osier Yang wrote:

The crash could happen if the disk source is empty for cdrom or
floppy disk.
---
  src/qemu/qemu_driver.c  |7 +--
  src/qemu/qemu_process.c |3 +++
  2 files changed, 8 insertions(+), 2 deletions(-)


I'm not sure if this was subsumed by a later version, but if it still
needs a review:


Sorry, I should tell this is subsumed by later patch.

Osier

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

[libvirt] [PATCH 1/2] virsh-snapshot: Refactor some details in virsh snapshot-create-as

2013-02-11 Thread Peter Krempa 
This patch simplifies the creation of XML, some error paths and adds
correct approach to check for virBuffer errors.
---
 tools/virsh-snapshot.c | 19 ---
 1 file changed, 8 insertions(+), 11 deletions(-)

diff --git a/tools/virsh-snapshot.c b/tools/virsh-snapshot.c
index 66776e2..fe1cee9 100644
--- a/tools/virsh-snapshot.c
+++ b/tools/virsh-snapshot.c
@@ -427,19 +427,16 @@ cmdSnapshotCreateAs(vshControl *ctl, const vshCmd *cmd)
 if (vshCommandOptBool(cmd, live))
 flags |= VIR_DOMAIN_SNAPSHOT_CREATE_LIVE;

-dom = vshCommandOptDomain(ctl, cmd, NULL);
-if (dom == NULL)
-goto cleanup;
+if (!(dom = vshCommandOptDomain(ctl, cmd, NULL)))
+return false;

 if (vshCommandOptStringReq(ctl, cmd, name, name)  0 ||
 vshCommandOptStringReq(ctl, cmd, description, desc)  0)
 goto cleanup;

 virBufferAddLit(buf, domainsnapshot\n);
-if (name)
-virBufferEscapeString(buf,   name%s/name\n, name);
-if (desc)
-virBufferEscapeString(buf,   description%s/description\n, desc);
+virBufferEscapeString(buf,   name%s/name\n, name);
+virBufferEscapeString(buf,   description%s/description\n, desc);

 if (vshCommandOptStringReq(ctl, cmd, memspec, memspec)  0)
 goto cleanup;
@@ -457,12 +454,13 @@ cmdSnapshotCreateAs(vshControl *ctl, const vshCmd *cmd)
 }
 virBufferAddLit(buf, /domainsnapshot\n);

-buffer = virBufferContentAndReset(buf);
-if (buffer == NULL) {
+if (virBufferError(buf)) {
 vshError(ctl, %s, _(Out of memory));
 goto cleanup;
 }

+buffer = virBufferContentAndReset(buf);
+
 if (vshCommandOptBool(cmd, print-xml)) {
 vshPrint(ctl, %s\n,  buffer);
 ret = true;
@@ -474,8 +472,7 @@ cmdSnapshotCreateAs(vshControl *ctl, const vshCmd *cmd)
 cleanup:
 virBufferFreeAndReset(buf);
 VIR_FREE(buffer);
-if (dom)
-virDomainFree(dom);
+virDomainFree(dom);

 return ret;
 }
-- 
1.8.1.1

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

[libvirt] [PATCH 0/2] virsh: Two small fixes for snapshot-create-as

2013-02-11 Thread Peter Krempa 
Peter Krempa (2):
  virsh-snapshot: Refactor some details in virsh snapshot-create-as
  virsh-snapshot: Reject --no-metadata together with --print-xml

 tools/virsh-snapshot.c | 27 +++
 1 file changed, 15 insertions(+), 12 deletions(-)

-- 
1.8.1.1

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

[libvirt] [PATCH 2/2] virsh-snapshot: Reject --no-metadata together with --print-xml

2013-02-11 Thread Peter Krempa 
Manual for virsh snapshot-create-as states that --no-metadata and
--print-xml are incompatible. Honor this detail in the code.
---
 tools/virsh-snapshot.c | 8 +++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/tools/virsh-snapshot.c b/tools/virsh-snapshot.c
index fe1cee9..d9659d4 100644
--- a/tools/virsh-snapshot.c
+++ b/tools/virsh-snapshot.c
@@ -412,8 +412,14 @@ cmdSnapshotCreateAs(vshControl *ctl, const vshCmd *cmd)
 unsigned int flags = 0;
 const vshCmdOpt *opt = NULL;

-if (vshCommandOptBool(cmd, no-metadata))
+if (vshCommandOptBool(cmd, no-metadata)) {
+if (vshCommandOptBool(cmd, print-xml)) {
+vshError(ctl, %s,
+ _(--print-xml is incompatible with --no-metadata));
+return false;
+}
 flags |= VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA;
+}
 if (vshCommandOptBool(cmd, halt))
 flags |= VIR_DOMAIN_SNAPSHOT_CREATE_HALT;
 if (vshCommandOptBool(cmd, disk-only))
-- 
1.8.1.1

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

Re: [libvirt] [PATCH 4/4 v5] virsh: Use virNodeDeviceLookupSCSIHostByWWN

2013-02-11 Thread Osier Yang 

On 2013年02月07日 23:09, Daniel P. Berrange wrote:

On Mon, Feb 04, 2013 at 10:16:44PM +0800, Osier Yang wrote:

Only nodedev-destroy and nodedev-dumpxml can benifit from the
new API, other commands like nodedev-detach only works for
PCI devices, WWN makes no sense for them.


Is that really correct ? I thought we could use 'nodedev-detach' to
delete an NPIV SCSI adapter, which would benefit from WWN wouldn't
it ?


nodedev-destroy can be used to delete the NPIV scsi adapter. Do we
also want to delete it using nodedev-detach? personally I think no
need, it just can introduce confusion.

Osier

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

Re: [libvirt] [PATCH 4/4 v5] virsh: Use virNodeDeviceLookupSCSIHostByWWN

2013-02-11 Thread Daniel P. Berrange 
On Mon, Feb 11, 2013 at 09:16:32PM +0800, Osier Yang wrote:
 On 2013年02月07日 23:09, Daniel P. Berrange wrote:
 On Mon, Feb 04, 2013 at 10:16:44PM +0800, Osier Yang wrote:
 Only nodedev-destroy and nodedev-dumpxml can benifit from the
 new API, other commands like nodedev-detach only works for
 PCI devices, WWN makes no sense for them.
 
 Is that really correct ? I thought we could use 'nodedev-detach' to
 delete an NPIV SCSI adapter, which would benefit from WWN wouldn't
 it ?
 
 nodedev-destroy can be used to delete the NPIV scsi adapter. Do we
 also want to delete it using nodedev-detach? personally I think no
 need, it just can introduce confusion.

Oh yes, ignore my comment. I was mixing up the two commands

Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

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

Re: [libvirt] [PATCH 4/4 v5] virsh: Use virNodeDeviceLookupSCSIHostByWWN

2013-02-11 Thread Osier Yang 

On 2013年02月11日 21:18, Daniel P. Berrange wrote:

On Mon, Feb 11, 2013 at 09:16:32PM +0800, Osier Yang wrote:

On 2013年02月07日 23:09, Daniel P. Berrange wrote:

On Mon, Feb 04, 2013 at 10:16:44PM +0800, Osier Yang wrote:

Only nodedev-destroy and nodedev-dumpxml can benifit from the
new API, other commands like nodedev-detach only works for
PCI devices, WWN makes no sense for them.


Is that really correct ? I thought we could use 'nodedev-detach' to
delete an NPIV SCSI adapter, which would benefit from WWN wouldn't
it ?


nodedev-destroy can be used to delete the NPIV scsi adapter. Do we
also want to delete it using nodedev-detach? personally I think no
need, it just can introduce confusion.


Oh yes, ignore my comment. I was mixing up the two commands



So, can you review this too? :-)

Osier

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

Re: [libvirt] [PATCH 4/4 v5] virsh: Use virNodeDeviceLookupSCSIHostByWWN

2013-02-11 Thread Daniel P. Berrange 
On Mon, Feb 04, 2013 at 10:16:44PM +0800, Osier Yang wrote:
 Only nodedev-destroy and nodedev-dumpxml can benifit from the
 new API, other commands like nodedev-detach only works for
 PCI devices, WWN makes no sense for them.
 
 ---
 Rebased on Peter's virsh cleanup patches.
 ---
  tools/virsh-nodedev.c |   84 +++-
  tools/virsh.pod   |   15 +---
  2 files changed, 77 insertions(+), 22 deletions(-)
 
 diff --git a/tools/virsh-nodedev.c b/tools/virsh-nodedev.c
 index f85bded..7c51a56 100644
 --- a/tools/virsh-nodedev.c
 +++ b/tools/virsh-nodedev.c
 @@ -101,9 +101,14 @@ static const vshCmdInfo info_node_device_destroy[] = {
  
  static const vshCmdOptDef opts_node_device_destroy[] = {
  {.name = name,
 + .type = VSH_OT_ALIAS,
 + .flags = 0,
 + .help = device
 +},
 +{.name = device,
   .type = VSH_OT_DATA,
   .flags = VSH_OFLAG_REQ,
 - .help = N_(name of the device to be destroyed)
 + .help = N_(device name or wwn pair in 'wwnn,wwpn' format)
  },
  {.name = NULL}
  };
 @@ -112,21 +117,47 @@ static bool
  cmdNodeDeviceDestroy(vshControl *ctl, const vshCmd *cmd)
  {
  virNodeDevicePtr dev = NULL;
 -bool ret = true;
 -const char *name = NULL;
 +bool ret = false;
 +const char *device_value = NULL;
 +char **arr = NULL;
 +int narr;
  
 -if (vshCommandOptStringReq(ctl, cmd, name, name)  0)
 +if (vshCommandOptStringReq(ctl, cmd, device, device_value)  0)
  return false;
  
 -dev = virNodeDeviceLookupByName(ctl-conn, name);
 +if (strchr(device_value, ',')) {
 +narr = vshStringToArray(device_value, arr);
 +if (narr != 2) {
 +vshError(ctl, _(Malformed device value '%s'), device_value);
 +goto cleanup;
 +}
 +
 +if (!virValidateWWN(arr[0]) || !virValidateWWN(arr[1]))
 +goto cleanup;
 +
 +dev = virNodeDeviceLookupSCSIHostByWWN(ctl-conn, arr[0], arr[1], 0);
 +} else {
 +dev = virNodeDeviceLookupByName(ctl-conn, device_value);
 +}
 +
 +if (!dev) {
 +vshError(ctl, %s '%s', _(Could not find matching device), 
 device_value);
 +goto cleanup;
 +}
  
  if (virNodeDeviceDestroy(dev) == 0) {
 -vshPrint(ctl, _(Destroyed node device '%s'\n), name);
 +vshPrint(ctl, _(Destroyed node device '%s'\n), device_value);
  } else {
 -vshError(ctl, _(Failed to destroy node device '%s'), name);
 -ret = false;
 +vshError(ctl, _(Failed to destroy node device '%s'), device_value);
 +goto cleanup;
  }
  
 +ret = true;
 +cleanup:
 +if (arr) {
 +VIR_FREE(*arr);

Hmm, IIUC, that is equiv to VIR_FREE(arr[0]), so what is
free'ing arr[1] ?

 +VIR_FREE(arr);
 +}


ACK if that leak is fixed, or otherwise explained.

Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

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

Re: [libvirt] [PATCH 1/2] virsh-snapshot: Refactor some details in virsh snapshot-create-as

2013-02-11 Thread Osier Yang 

On 2013年02月11日 21:10, Peter Krempa wrote:

This patch simplifies the creation of XML, some error paths and adds
correct approach to check for virBuffer errors.
---
  tools/virsh-snapshot.c | 19 ---
  1 file changed, 8 insertions(+), 11 deletions(-)

diff --git a/tools/virsh-snapshot.c b/tools/virsh-snapshot.c
index 66776e2..fe1cee9 100644
--- a/tools/virsh-snapshot.c
+++ b/tools/virsh-snapshot.c
@@ -427,19 +427,16 @@ cmdSnapshotCreateAs(vshControl *ctl, const vshCmd *cmd)
  if (vshCommandOptBool(cmd, live))
  flags |= VIR_DOMAIN_SNAPSHOT_CREATE_LIVE;

-dom = vshCommandOptDomain(ctl, cmd, NULL);
-if (dom == NULL)
-goto cleanup;
+if (!(dom = vshCommandOptDomain(ctl, cmd, NULL)))
+return false;

  if (vshCommandOptStringReq(ctl, cmd, name,name)  0 ||
  vshCommandOptStringReq(ctl, cmd, description,desc)  0)
  goto cleanup;

  virBufferAddLit(buf, domainsnapshot\n);
-if (name)
-virBufferEscapeString(buf, name%s/name\n, name);
-if (desc)
-virBufferEscapeString(buf, description%s/description\n, desc);
+virBufferEscapeString(buf, name%s/name\n, name);
+virBufferEscapeString(buf, description%s/description\n, desc);

  if (vshCommandOptStringReq(ctl, cmd, memspec,memspec)  0)
  goto cleanup;
@@ -457,12 +454,13 @@ cmdSnapshotCreateAs(vshControl *ctl, const vshCmd *cmd)
  }
  virBufferAddLit(buf, /domainsnapshot\n);

-buffer = virBufferContentAndReset(buf);
-if (buffer == NULL) {
+if (virBufferError(buf)) {
  vshError(ctl, %s, _(Out of memory));
  goto cleanup;
  }

+buffer = virBufferContentAndReset(buf);
+
  if (vshCommandOptBool(cmd, print-xml)) {
  vshPrint(ctl, %s\n,  buffer);
  ret = true;
@@ -474,8 +472,7 @@ cmdSnapshotCreateAs(vshControl *ctl, const vshCmd *cmd)
  cleanup:
  virBufferFreeAndReset(buf);
  VIR_FREE(buffer);
-if (dom)
-virDomainFree(dom);
+virDomainFree(dom);

  return ret;
  }


ACK

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

Re: [libvirt] libvirt and Parallels Cloud Storage

2013-02-11 Thread Alexander Gordeev 
В Mon, 11 Feb 2013 09:57:05 +
Daniel P. Berrange berra...@redhat.com пишет:

 On Sun, Feb 10, 2013 at 02:20:02AM +0400, Alexander Gordeev wrote:
  В Thu, 7 Feb 2013 16:09:31 +
  Daniel P. Berrange berra...@redhat.com пишет:
  
It's not a POSIX FS but there is a FUSE client for it that can be
used to access and manipulate images. It's quite high speed but only
when used with O_DIRECT + aio. I tried to setup several KVMs on top of
a Pstorage mount using virt-manager. It worked good, but I had to:
1. tune cache and IO settings for every disk
2. disable space allocation by libvirt because it is using sync IO and
is therefore slow

I tried to find ways to solve the first issue and IMHO this can be
done by adding a way to specify per-pool defaults for cache and IO
settings. I didn't find any way for this in the current code and UI.
I'd like to add a new storage backend also that will be a 'dir' backend
with extra ability to manage Pstorage mount points and UI integration.
I'd like to merge my work to the main tree when it's finished if
possible.
   
   I don't think that putting cache/IO defaults in the XML is really
   appropriate. There are too many different scenarios which want
   different settings to be able to clearly identify one set of
   perfect settings. I see this as primarily a documentation problem.
   Someone needs to write something describing the diferrent settings
   for each storage pool  what the pros/cons are for each. Downstream
   app developers can then make decisions about suitable defaults for
   their use cases
  
  If you are against changes in the XML maybe you are ok with a simpler
  option: I create a storage backend for Pstorage, add two fields for
  cache and io (or maybe just flags) to virStoragePoolTypeInfo and set
  the right values for the new pool type. Then add some code to check
  them to qemuBuildDriveStr (and to other drivers if possible, of course).
  So no changes to XML and API. Is this better?
 
 No, the hypervisor drivers can only access data that is available via
 from the storage drivers via the XML or APIs. As I said before this is
 a policy decision for application authors to take, not for libvirt
 itself.

Ok, I'll do the storage backend then. If you accept it virt-manager (and
other clients) probably will be able to distinguish a local directory
from a Pstorage mount point.

-- 
  Alexander

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

Re: [libvirt] [PATCH 2/2] virsh-snapshot: Reject --no-metadata together with --print-xml

2013-02-11 Thread Osier Yang 

On 2013年02月11日 21:10, Peter Krempa wrote:

Manual for virsh snapshot-create-as states that --no-metadata and
--print-xml are incompatible. Honor this detail in the code.
---
  tools/virsh-snapshot.c | 8 +++-
  1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/tools/virsh-snapshot.c b/tools/virsh-snapshot.c
index fe1cee9..d9659d4 100644
--- a/tools/virsh-snapshot.c
+++ b/tools/virsh-snapshot.c
@@ -412,8 +412,14 @@ cmdSnapshotCreateAs(vshControl *ctl, const vshCmd *cmd)
  unsigned int flags = 0;
  const vshCmdOpt *opt = NULL;

-if (vshCommandOptBool(cmd, no-metadata))
+if (vshCommandOptBool(cmd, no-metadata)) {
+if (vshCommandOptBool(cmd, print-xml)) {


Considering using a variable to store the return value instead of
calling the function twice.


+vshError(ctl, %s,
+ _(--print-xml is incompatible with --no-metadata));
+return false;
+}
  flags |= VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA;
+}
  if (vshCommandOptBool(cmd, halt))
  flags |= VIR_DOMAIN_SNAPSHOT_CREATE_HALT;
  if (vshCommandOptBool(cmd, disk-only))


But I'm fine if you keep the twice calling, as it's trivial. ACK.

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

[libvirt] libvirtd (from git) no longer responds to 'kill'

2013-02-11 Thread Richard W.M. Jones 

It used to be that you could kill a session libvirtd using eg:

  killall libvirtd lt-libvirtd

However with upstream libvirt from git today, this no longer appears
to work:

$ ps ax | grep libvirtd
 4240 ?Ssl0:05 /usr/sbin/libvirtd
18492 ?Sl 0:00 /home/rjones/d/libvirt/daemon/.libs/lt-libvirtd 
--timeout=30
18775 pts/10   S+ 0:00 grep --color=auto libvirtd
$ killall lt-libvirtd
$ ps ax | grep libvirtd
 4240 ?Ssl0:05 /usr/sbin/libvirtd
18492 ?Sl 0:00 /home/rjones/d/libvirt/daemon/.libs/lt-libvirtd 
--timeout=30
18785 pts/10   S+ 0:00 grep --color=auto libvirtd
$ killall lt-libvirtd
$ ps ax | grep libvirtd
 4240 ?Ssl0:05 /usr/sbin/libvirtd
18492 ?Sl 0:00 /home/rjones/d/libvirt/daemon/.libs/lt-libvirtd 
--timeout=30
18799 pts/10   S+ 0:00 grep --color=auto libvirtd

Is this new brokenness, or was this never meant to work in the first
place?

BTW this libvirtd process is pretty persistent.  I sent it a whole
variety of signals, and only 'kill -9' worked.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org

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

[libvirt] [PATCH v3 0/2] net: support set public ip for forward mode nat

2013-02-11 Thread Natanael Copa 
Rebased patch[1].

Changes v3:
 - remove support for nat address='1.2.3.4'/ format, the 2/4 patch[2].

[1] http://www.redhat.com/archives/libvir-list/2013-February/msg00088.html
[2] http://www.redhat.com/archives/libvir-list/2013-February/msg00090.html

Natanael Copa (2):
  net: support set public ip range for forward mode nat
  net: add support for specifying port range for forward mode nat

 docs/formatnetwork.html.in  |  32 
 src/conf/network_conf.c | 189 ++--
 src/conf/network_conf.h |   4 +
 src/network/bridge_driver.c |  32 
 src/util/viriptables.c  |  77 +++---
 src/util/viriptables.h  |   8 ++
 6 files changed, 328 insertions(+), 14 deletions(-)

-- 
1.8.1.2

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

[libvirt] [PATCH v3 1/2] net: support set public ip range for forward mode nat

2013-02-11 Thread Natanael Copa 
Support setting which public ip to use for NAT via attribute
address in subelement nat in forward:

...
  forward mode='nat'
  address start='1.2.3.4' end='1.2.3.10'/
  /forward
...

This will construct an iptables line using:

  '-j SNAT --to-source start-end'

instead of:

  '-j MASQUERADE'

Signed-off-by: Natanael Copa nc...@alpinelinux.org
---
 docs/formatnetwork.html.in  |  17 ++
 src/conf/network_conf.c | 146 ++--
 src/conf/network_conf.h |   3 +
 src/network/bridge_driver.c |  16 +
 src/util/viriptables.c  |  56 ++---
 src/util/viriptables.h  |   4 ++
 6 files changed, 228 insertions(+), 14 deletions(-)

diff --git a/docs/formatnetwork.html.in b/docs/formatnetwork.html.in
index 7b42529..fc064dc 100644
--- a/docs/formatnetwork.html.in
+++ b/docs/formatnetwork.html.in
@@ -136,6 +136,23 @@
 network, and to/from the host to the guests, are
 unrestricted and not NATed.span class=sinceSince
 0.4.2/span
+
+pspan class=sinceSince 1.0.3/span it is possible to
+specify a public IPv4 address range to be used for the NAT by
+using the codelt;natgt;/code and
+codelt;addressgt;/code subelements.
+pre
+...
+  lt;forward mode='nat'gt;
+lt;natgt;
+  lt;address start='1.2.3.4' end='1.2.3.10'/gt;
+lt;/natgt;
+  lt;/forwardgt;
+...
+/pre
+An singe IPv4 address can be set by omitting the
+codeend/code attribute.
+/p
   /dd
 
   dtcoderoute/code/dt
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
index 3604ff7..939b9f2 100644
--- a/src/conf/network_conf.c
+++ b/src/conf/network_conf.c
@@ -1325,6 +1325,74 @@ cleanup:
 }
 
 static int
+virNetworkForwardNatDefParseXML(const char *networkName,
+xmlNodePtr node,
+xmlXPathContextPtr ctxt,
+virNetworkForwardDefPtr def)
+{
+int ret = -1;
+xmlNodePtr *natAddrNodes = NULL;
+int nNatAddrs;
+char *addr_start = NULL;
+char *addr_end = NULL;
+xmlNodePtr save = ctxt-node;
+
+ctxt-node = node;
+
+if (def-type != VIR_NETWORK_FORWARD_NAT) {
+virReportError(VIR_ERR_XML_ERROR,
+   _(The nat element can only be used when forward 
'mode' is 'nat' in network %s),
+   networkName);
+goto cleanup;
+}
+
+/* addresses for SNAT */
+nNatAddrs = virXPathNodeSet(./address, ctxt, natAddrNodes);
+if (nNatAddrs  0) {
+virReportError(VIR_ERR_XML_ERROR,
+   _(invalid address element found in forward of 
+ network %s), networkName);
+goto cleanup;
+} else if (nNatAddrs  1) {
+virReportError(VIR_ERR_XML_ERROR,
+   _(Only one address element is allowed in nat in 
+ forward in network %s), networkName);
+goto cleanup;
+} else if (nNatAddrs == 1) {
+addr_start = virXMLPropString(*natAddrNodes, start);
+if (addr_start == NULL) {
+virReportError(VIR_ERR_XML_ERROR,
+   _(missing 'start' attribute in address element 
in nat in 
+ forward in network %s), networkName);
+goto cleanup;
+}
+addr_end = virXMLPropString(*natAddrNodes, end);
+}
+
+if (addr_start  virSocketAddrParse(def-addr_start, addr_start, 
AF_INET)  0) {
+virReportError(VIR_ERR_XML_ERROR,
+   _(Bad ipv4 address '%s' in nat in forward in 
+ network '%s'), addr_start, networkName);
+goto cleanup;
+}
+
+if (addr_end  virSocketAddrParse(def-addr_end, addr_end, AF_INET)  0) 
{
+virReportError(VIR_ERR_XML_ERROR,
+   _(Bad ipv4 address '%s' in nat in forward in 
+ network '%s'), addr_end, networkName);
+goto cleanup;
+}
+
+ret = 0;
+
+cleanup:
+VIR_FREE(addr_start);
+VIR_FREE(addr_end);
+ctxt-node = save;
+return ret;
+}
+
+static int
 virNetworkForwardDefParseXML(const char *networkName,
  xmlNodePtr node,
  xmlXPathContextPtr ctxt,
@@ -1334,7 +1402,8 @@ virNetworkForwardDefParseXML(const char *networkName,
 xmlNodePtr *forwardIfNodes = NULL;
 xmlNodePtr *forwardPfNodes = NULL;
 xmlNodePtr *forwardAddrNodes = NULL;
-int nForwardIfs, nForwardAddrs, nForwardPfs;
+xmlNodePtr *forwardNatNodes = NULL;
+int nForwardIfs, nForwardAddrs, nForwardPfs, nForwardNats;
 char *forwardDev = NULL;
 char *forwardManaged = NULL;
 char *type = NULL;
@@ -1384,6 +1453,24 @@ virNetworkForwardDefParseXML(const char *networkName,
 goto cleanup;
 }
 
+nForwardNats = virXPathNodeSet(./nat, ctxt, forwardNatNodes);
+if

Re: [libvirt] libvirtd (from git) no longer responds to 'kill'

2013-02-11 Thread Richard W.M. Jones 

This seems to be some sort of deadlock, easily reproduced by running
the libguestfs test suite, or even just 'libguestfs-test-tool'.

Here is a stack trace:

(gdb) t a a bt

Thread 11 (Thread 0x7fe6505d7700 (LWP 20021)):
#0  pthread_cond_wait@@GLIBC_2.3.2 ()
at ../nptl/sysdeps/unix/sysv/linux/x86_64/pthread_cond_wait.S:165
#1  0x7fe650ad155a in virCondWait (c=c@entry=0x1943c80, 
m=m@entry=0x1943c58) at util/virthreadpthread.c:117
#2  0x7fe650ad1bdb in virThreadPoolWorker (opaque=opaque@entry=0x1934930)
at util/virthreadpool.c:103
#3  0x7fe650ad11f6 in virThreadHelper (data=optimized out)
at util/virthreadpthread.c:161
#4  0x00328ca07d15 in start_thread (arg=0x7fe6505d7700)
at pthread_create.c:308
#5  0x00328c6f246d in clone ()
at ../sysdeps/unix/sysv/linux/x86_64/clone.S:114

Thread 10 (Thread 0x7fe64fdd6700 (LWP 20022)):
#0  pthread_cond_wait@@GLIBC_2.3.2 ()
at ../nptl/sysdeps/unix/sysv/linux/x86_64/pthread_cond_wait.S:165
#1  0x7fe650ad155a in virCondWait (c=c@entry=0x1943c80, 
m=m@entry=0x1943c58) at util/virthreadpthread.c:117
#2  0x7fe650ad1bdb in virThreadPoolWorker (opaque=opaque@entry=0x1934b50)
at util/virthreadpool.c:103
#3  0x7fe650ad11f6 in virThreadHelper (data=optimized out)
at util/virthreadpthread.c:161
#4  0x00328ca07d15 in start_thread (arg=0x7fe64fdd6700)
at pthread_create.c:308
#5  0x00328c6f246d in clone ()
at ../sysdeps/unix/sysv/linux/x86_64/clone.S:114

Thread 9 (Thread 0x7fe64f5d5700 (LWP 20023)):
#0  pthread_cond_wait@@GLIBC_2.3.2 ()
at ../nptl/sysdeps/unix/sysv/linux/x86_64/pthread_cond_wait.S:165
#1  0x7fe650ad155a in virCondWait (c=c@entry=0x1943c80, 
m=m@entry=0x1943c58) at util/virthreadpthread.c:117
#2  0x7fe650ad1bdb in virThreadPoolWorker (opaque=opaque@entry=0x1934c50)
at util/virthreadpool.c:103
#3  0x7fe650ad11f6 in virThreadHelper (data=optimized out)
at util/virthreadpthread.c:161
#4  0x00328ca07d15 in start_thread (arg=0x7fe64f5d5700)
at pthread_create.c:308
#5  0x00328c6f246d in clone ()
at ../sysdeps/unix/sysv/linux/x86_64/clone.S:114

Thread 8 (Thread 0x7fe64edd4700 (LWP 20024)):
#0  __lll_lock_wait ()
at ../nptl/sysdeps/unix/sysv/linux/x86_64/lowlevellock.S:135
---Type return to continue, or q return to quit---
#1  0x00328ca09ca6 in _L_lock_836 () from /lib64/libpthread.so.0
#2  0x00328ca09ba8 in __GI___pthread_mutex_lock (
mutex=mutex@entry=0x7fe64006eb30) at pthread_mutex_lock.c:64
#3  0x7fe650ad142d in virMutexLock (m=m@entry=0x7fe64006eb30)
at util/virthreadpthread.c:85
#4  0x7fe650ac38de in virObjectLock (anyobj=anyobj@entry=0x7fe64006eb20)
at util/virobject.c:322
#5  0x7fe650ce65b1 in virSecurityManagerGetModel (
mgr=mgr@entry=0x7fe64006eb20) at security/security_manager.c:236
#6  0x7fe650ce994c in virSecuritySELinuxSecurityVerify (
mgr=0x7fe64006eb20, def=optimized out)
at security/security_selinux.c:1806
#7  0x7fe650ce7251 in virSecurityManagerVerify (mgr=0x7fe64006eb20, 
def=def@entry=0x7fe63400ac20) at security/security_manager.c:573
#8  0x7fe650ce3cd4 in virSecurityStackVerify (mgr=optimized out, 
def=0x7fe63400ac20) at security/security_stack.c:125
#9  0x7fe650ce7251 in virSecurityManagerVerify (mgr=0x7fe64001cc50, 
def=def@entry=0x7fe63400ac20) at security/security_manager.c:573
#10 0x7fe64893e63d in qemuDomainCreate (conn=0x7fe638000bd0, 
xml=0x7fe6340009a0 ?xml version=\1.0\?\ndomain type=\kvm\ 
xmlns:qemu=\http://libvirt.org/schemas/domain/qemu/1.0\;\n  
nameguestfs-492qa31a2ntfmk0j/name\n  memory unit=\MiB\500/memory\n  
currentMemory unit=\MiB\..., flags=optimized out) at 
qemu/qemu_driver.c:1538
#11 0x7fe650b4fa39 in virDomainCreateXML (conn=0x7fe638000bd0, 
xmlDesc=0x7fe6340009a0 ?xml version=\1.0\?\ndomain type=\kvm\ 
xmlns:qemu=\http://libvirt.org/schemas/domain/qemu/1.0\;\n  
nameguestfs-492qa31a2ntfmk0j/name\n  memory unit=\MiB\500/memory\n  
currentMemory unit=\MiB\..., flags=2) at libvirt.c:1988
#12 0x0042c915 in remoteDispatchDomainCreateXML (
server=optimized out, msg=optimized out, ret=0x7fe6340008c0, 
args=0x7fe634000970, rerr=0x7fe64edd3c50, client=0x196eb60)
at remote_dispatch.h:1172
#13 remoteDispatchDomainCreateXMLHelper (server=optimized out, 
client=0x196eb60, msg=optimized out, rerr=0x7fe64edd3c50, 
args=0x7fe634000970, ret=0x7fe6340008c0) at remote_dispatch.h:1152
#14 0x7fe650bbe602 in virNetServerProgramDispatchCall (msg=0x196cd40, 
client=0x196eb60, server=0x1943b10, prog=0x1968460)
at rpc/virnetserverprogram.c:432
#15 virNetServerProgramDispatch (prog=0x1968460, 
server=server@entry=0x1943b10, client=0x196eb60, msg=0x196cd40)
at rpc/virnetserverprogram.c:305
#16 0x7fe650bb8838 in virNetServerProcessMsg (msg=optimized out, 
prog=optimized out, client=optimized out, srv=0x1943b10)
at rpc/virnetserver.c:162
#17 virNetServerHandleJob

[libvirt] [PATCH v3 2/2] net: add support for specifying port range for forward mode nat

2013-02-11 Thread Natanael Copa 
Let users set the port range to be used for forward mode NAT:

...
  forward mode='nat'
nat
  port start='1024' end='65535'/
/nat
  /forward
...

Signed-off-by: Natanael Copa nc...@alpinelinux.org
---
 docs/formatnetwork.html.in  | 21 ++---
 src/conf/network_conf.c | 57 +++--
 src/conf/network_conf.h |  3 ++-
 src/network/bridge_driver.c | 16 +
 src/util/viriptables.c  | 39 ---
 src/util/viriptables.h  |  4 
 6 files changed, 120 insertions(+), 20 deletions(-)

diff --git a/docs/formatnetwork.html.in b/docs/formatnetwork.html.in
index fc064dc..bcdb240 100644
--- a/docs/formatnetwork.html.in
+++ b/docs/formatnetwork.html.in
@@ -138,9 +138,11 @@
 0.4.2/span
 
 pspan class=sinceSince 1.0.3/span it is possible to
-specify a public IPv4 address range to be used for the NAT by
-using the codelt;natgt;/code and
-codelt;addressgt;/code subelements.
+specify a public IPv4 address and port range to be used for
+the NAT by using the codelt;natgt;/code subelement.
+The address range is set with the codelt;addressgt;/code
+subelements and codestart/code and codestop/code
+attributes:
 pre
 ...
   lt;forward mode='nat'gt;
@@ -153,6 +155,19 @@
 An singe IPv4 address can be set by omitting the
 codeend/code attribute.
 /p
+p
+The port range to be used for the codelt;natgt;/code can
+be set via the subelement codelt;portgt;/code:
+pre
+...
+  lt;forward mode='nat'gt;
+lt;natgt;
+  lt;port start='500' end='1000'/gt;
+lt;/natgt;
+  lt;/forwardgt;
+...
+/pre
+/p
   /dd
 
   dtcoderoute/code/dt
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
index 939b9f2..15713dc 100644
--- a/src/conf/network_conf.c
+++ b/src/conf/network_conf.c
@@ -1332,7 +1332,8 @@ virNetworkForwardNatDefParseXML(const char *networkName,
 {
 int ret = -1;
 xmlNodePtr *natAddrNodes = NULL;
-int nNatAddrs;
+xmlNodePtr *natPortNodes = NULL;
+int nNatAddrs, nNatPorts;
 char *addr_start = NULL;
 char *addr_end = NULL;
 xmlNodePtr save = ctxt-node;
@@ -1383,6 +1384,36 @@ virNetworkForwardNatDefParseXML(const char *networkName,
 goto cleanup;
 }
 
+/* ports for SNAT and MASQUERADE */
+nNatPorts = virXPathNodeSet(./port, ctxt, natPortNodes);
+if (nNatPorts  0) {
+virReportError(VIR_ERR_XML_ERROR,
+   _(invalid port element found in forward of 
+ network %s), networkName);
+goto cleanup;
+} else if (nNatPorts  1) {
+virReportError(VIR_ERR_XML_ERROR,
+   _(Only one port element is allowed in nat in 
+ forward in network %s), networkName);
+goto cleanup;
+} else if (nNatPorts == 1) {
+if (virXPathUInt(string(./port[1]/@start), ctxt, def-port_start)  0
+|| def-port_start  65535) {
+
+virReportError(VIR_ERR_XML_DETAIL,
+   _(Missing or invalid 'start' attribute in port 
+ in nat in forward in network %s),
+ networkName);
+goto cleanup;
+}
+if (virXPathUInt(string(./port[1]/@end), ctxt, def-port_end)  0
+|| def-port_end  65535 || def-port_end  def-port_start) {
+virReportError(VIR_ERR_XML_DETAIL,
+   _(Missing or invalid 'end' attribute in port in 
+ nat in forward in network %s), networkName);
+goto cleanup;
+}
+}
 ret = 0;
 
 cleanup:
@@ -2173,6 +2204,7 @@ virNatDefFormat(virBufferPtr buf,
 char *addr_start = NULL;
 char *addr_end = NULL;
 int ret = -1;
+int longdef;
 
 if (VIR_SOCKET_ADDR_VALID(fwd-addr_start)) {
 addr_start = virSocketAddrFormat(fwd-addr_start);
@@ -2186,16 +2218,25 @@ virNatDefFormat(virBufferPtr buf,
 goto cleanup;
 }
 
-if (!addr_end  !addr_start)
+if (!addr_start  !addr_end  !fwd-port_start  !fwd-port_end)
 return 0;
 
 virBufferAddLit(buf, nat\n);
 virBufferAdjustIndent(buf, 2);
 
-virBufferAsprintf(buf, address start='%s', addr_start);
-if (addr_end)
-virBufferAsprintf(buf,  end='%s', addr_end);
-virBufferAsprintf(buf, /\n);
+if (addr_start) {
+virBufferAsprintf(buf, address start='%s', addr_start);
+if (addr_end)
+virBufferAsprintf(buf,  end='%s', addr_end);
+virBufferAsprintf(buf, /\n);
+}
+
+if (fwd-port_start || fwd-port_end) {
+virBufferAsprintf(buf, port start='%d', fwd-port_start);
+if (fwd-port_end)
+virBufferAsprintf(buf,  end='%d', fwd-port_end);
+

Re: [libvirt] [PATCH v2 2/4] net: support set public ip for forward mode nat

2013-02-11 Thread Natanael Copa 
On Fri, 8 Feb 2013 17:25:11 +
Daniel P. Berrange berra...@redhat.com wrote:

 On Mon, Feb 04, 2013 at 10:45:24AM +0100, Natanael Copa wrote:
  Support setting which public ip to use for NAT via attribute
  address in subelement nat in forward:
  
  ...
forward mode='nat'
nat address='1.2.3.4'/
/forward
 
 Unless I'm mis-understanding, this is just identical to
 using a range, with the start + end addresses equal eg
 
   forward mode='nat'
 nat
   address start='1.2.3.4' end='1.2.3.4'/
 /nat
   /forward

Almost, its redundant with no 'end' attribute.
   forward mode='nat'
 nat
   address start='1.2.3.4'/
 /nat
   /forward
 
 if so, then this is redundant - we should just use the
 address start='1.2.3.4' end='1.2.3.4'/ syntax
 for everything, and not special case the scenario
 where start+end are equal.

I sent a rebase without 2/4 which is still somewhat redundant.

If you specify 'start', but not 'end', it will generate different iptables 
lines:
  iptables ... --to-source 1.2.3.4
vs
  iptables ... --to-source 1.2.3.4-1.2.3.4

It appears that iptables currently accepts both forms and generate the
same thing. I cannot guarantee that iptables maintainers will remove
duplicate ways of specifying same rules in future or treat them
different, so it might be an idea to be able to specify both variants.

-nc

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

[libvirt] [PATCH] Remove re-entrant API call in SELinux/AppArmor security managers

2013-02-11 Thread Daniel P. Berrange 
From: Daniel P. Berrange berra...@redhat.com

The security manager drivers are not allowed to call back
out to top level security manager APIs, since that results
in recursive mutex acquisition and thus deadlock. Remove
calls to virSecurityManagerGetModel from SELinux / AppArmor
drivers

Signed-off-by: Daniel P. Berrange berra...@redhat.com
---
 src/security/security_apparmor.c |  4 ++--
 src/security/security_selinux.c  | 20 ++--
 2 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c
index bf795b0..f281555 100644
--- a/src/security/security_apparmor.c
+++ b/src/security/security_apparmor.c
@@ -603,12 +603,12 @@ AppArmorSetSecurityProcessLabel(virSecurityManagerPtr 
mgr, virDomainDefPtr def)
 if ((profile_name = get_profile_name(def)) == NULL)
 return rc;
 
-if (STRNEQ(virSecurityManagerGetModel(mgr), secdef-model)) {
+if (STRNEQ(SECURITY_APPARMOR_NAME, secdef-model)) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
_(security label driver mismatch: 
  \'%s\' model configured for domain, but 
  hypervisor driver is \'%s\'.),
-   secdef-model, virSecurityManagerGetModel(mgr));
+   secdef-model, SECURITY_APPARMOR_NAME);
 if (use_apparmor()  0)
 goto clean;
 }
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index 2f5012d..cfb99a3 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -1803,12 +1803,12 @@ virSecuritySELinuxSecurityVerify(virSecurityManagerPtr 
mgr ATTRIBUTE_UNUSED,
 if (secdef == NULL)
 return -1;
 
-if (!STREQ(virSecurityManagerGetModel(mgr), secdef-model)) {
+if (!STREQ(SECURITY_SELINUX_NAME, secdef-model)) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
_(security label driver mismatch: 
  '%s' model configured for domain, but 
  hypervisor driver is '%s'.),
-   secdef-model, virSecurityManagerGetModel(mgr));
+   secdef-model, SECURITY_SELINUX_NAME);
 return -1;
 }
 
@@ -1837,12 +1837,12 @@ 
virSecuritySELinuxSetSecurityProcessLabel(virSecurityManagerPtr mgr,
 return 0;
 
 VIR_DEBUG(label=%s, secdef-label);
-if (!STREQ(virSecurityManagerGetModel(mgr), secdef-model)) {
+if (!STREQ(SECURITY_SELINUX_NAME, secdef-model)) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
_(security label driver mismatch: 
  '%s' model configured for domain, but 
  hypervisor driver is '%s'.),
-   secdef-model, virSecurityManagerGetModel(mgr));
+   secdef-model, SECURITY_SELINUX_NAME);
 if (security_getenforce() == 1)
 return -1;
 }
@@ -1875,12 +1875,12 @@ 
virSecuritySELinuxSetSecurityDaemonSocketLabel(virSecurityManagerPtr mgr,
 if (secdef-label == NULL)
 return 0;
 
-if (!STREQ(virSecurityManagerGetModel(mgr), secdef-model)) {
+if (!STREQ(SECURITY_SELINUX_NAME, secdef-model)) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
_(security label driver mismatch: 
  '%s' model configured for domain, but 
  hypervisor driver is '%s'.),
-   secdef-model, virSecurityManagerGetModel(mgr));
+   secdef-model, SECURITY_SELINUX_NAME);
 goto done;
 }
 
@@ -1925,12 +1925,12 @@ 
virSecuritySELinuxSetSecuritySocketLabel(virSecurityManagerPtr mgr,
 if (secdef-label == NULL)
 return 0;
 
-if (!STREQ(virSecurityManagerGetModel(mgr), secdef-model)) {
+if (!STREQ(SECURITY_SELINUX_NAME, secdef-model)) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
_(security label driver mismatch: 
  '%s' model configured for domain, but 
  hypervisor driver is '%s'.),
-   secdef-model, virSecurityManagerGetModel(mgr));
+   secdef-model, SECURITY_SELINUX_NAME);
 goto done;
 }
 
@@ -1966,12 +1966,12 @@ 
virSecuritySELinuxClearSecuritySocketLabel(virSecurityManagerPtr mgr,
 if (secdef-label == NULL)
 return 0;
 
-if (!STREQ(virSecurityManagerGetModel(mgr), secdef-model)) {
+if (!STREQ(SECURITY_SELINUX_NAME, secdef-model)) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
_(security label driver mismatch: 
  '%s' model configured for domain, but 
  hypervisor driver is '%s'.),
-   secdef-model, virSecurityManagerGetModel(mgr));
+   secdef-model, SECURITY_SELINUX_NAME);
 if (security_getenforce() == 1)
 return -1;

Re: [libvirt] libvirtd (from git) no longer responds to 'kill'

2013-02-11 Thread Daniel P. Berrange 
On Mon, Feb 11, 2013 at 02:02:21PM +, Richard W.M. Jones wrote:
 
 This seems to be some sort of deadlock, easily reproduced by running
 the libguestfs test suite, or even just 'libguestfs-test-tool'.
 
 Here is a stack trace:
 
 (gdb) t a a bt

 Thread 8 (Thread 0x7fe64edd4700 (LWP 20024)):
 #0  __lll_lock_wait ()
 at ../nptl/sysdeps/unix/sysv/linux/x86_64/lowlevellock.S:135
 ---Type return to continue, or q return to quit---
 #1  0x00328ca09ca6 in _L_lock_836 () from /lib64/libpthread.so.0
 #2  0x00328ca09ba8 in __GI___pthread_mutex_lock (
 mutex=mutex@entry=0x7fe64006eb30) at pthread_mutex_lock.c:64
 #3  0x7fe650ad142d in virMutexLock (m=m@entry=0x7fe64006eb30)
 at util/virthreadpthread.c:85
 #4  0x7fe650ac38de in virObjectLock (anyobj=anyobj@entry=0x7fe64006eb20)
 at util/virobject.c:322
 #5  0x7fe650ce65b1 in virSecurityManagerGetModel (
 mgr=mgr@entry=0x7fe64006eb20) at security/security_manager.c:236
 #6  0x7fe650ce994c in virSecuritySELinuxSecurityVerify (
 mgr=0x7fe64006eb20, def=optimized out)
 at security/security_selinux.c:1806
 #7  0x7fe650ce7251 in virSecurityManagerVerify (mgr=0x7fe64006eb20, 
 def=def@entry=0x7fe63400ac20) at security/security_manager.c:573
 #8  0x7fe650ce3cd4 in virSecurityStackVerify (mgr=optimized out, 
 def=0x7fe63400ac20) at security/security_stack.c:125
 #9  0x7fe650ce7251 in virSecurityManagerVerify (mgr=0x7fe64001cc50, 
 def=def@entry=0x7fe63400ac20) at security/security_manager.c:573

This shows the problem - the security driver implementations are not
allowed to call back out to other virSecurityManagerXXX APis like
virSecurityManagerGetModel, since that causes recursive mutex acquisition.
I've cc'd you on a fix

Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

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

Re: [libvirt] [PATCH] Remove re-entrant API call in SELinux/AppArmor security managers

2013-02-11 Thread Richard W.M. Jones 
On Mon, Feb 11, 2013 at 02:26:15PM +, Daniel P. Berrange wrote:
 From: Daniel P. Berrange berra...@redhat.com
 
 The security manager drivers are not allowed to call back
 out to top level security manager APIs, since that results
 in recursive mutex acquisition and thus deadlock. Remove
 calls to virSecurityManagerGetModel from SELinux / AppArmor
 drivers
 
 Signed-off-by: Daniel P. Berrange berra...@redhat.com
 ---
  src/security/security_apparmor.c |  4 ++--
  src/security/security_selinux.c  | 20 ++--
  2 files changed, 12 insertions(+), 12 deletions(-)
 
 diff --git a/src/security/security_apparmor.c 
 b/src/security/security_apparmor.c
 index bf795b0..f281555 100644
 --- a/src/security/security_apparmor.c
 +++ b/src/security/security_apparmor.c
 @@ -603,12 +603,12 @@ AppArmorSetSecurityProcessLabel(virSecurityManagerPtr 
 mgr, virDomainDefPtr def)
  if ((profile_name = get_profile_name(def)) == NULL)
  return rc;
  
 -if (STRNEQ(virSecurityManagerGetModel(mgr), secdef-model)) {
 +if (STRNEQ(SECURITY_APPARMOR_NAME, secdef-model)) {
  virReportError(VIR_ERR_INTERNAL_ERROR,
 _(security label driver mismatch: 
   \'%s\' model configured for domain, but 
   hypervisor driver is \'%s\'.),
 -   secdef-model, virSecurityManagerGetModel(mgr));
 +   secdef-model, SECURITY_APPARMOR_NAME);
  if (use_apparmor()  0)
  goto clean;
  }
 diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
 index 2f5012d..cfb99a3 100644
 --- a/src/security/security_selinux.c
 +++ b/src/security/security_selinux.c
 @@ -1803,12 +1803,12 @@ 
 virSecuritySELinuxSecurityVerify(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED,
  if (secdef == NULL)
  return -1;
  
 -if (!STREQ(virSecurityManagerGetModel(mgr), secdef-model)) {
 +if (!STREQ(SECURITY_SELINUX_NAME, secdef-model)) {
  virReportError(VIR_ERR_INTERNAL_ERROR,
 _(security label driver mismatch: 
   '%s' model configured for domain, but 
   hypervisor driver is '%s'.),
 -   secdef-model, virSecurityManagerGetModel(mgr));
 +   secdef-model, SECURITY_SELINUX_NAME);
  return -1;
  }
  
 @@ -1837,12 +1837,12 @@ 
 virSecuritySELinuxSetSecurityProcessLabel(virSecurityManagerPtr mgr,
  return 0;
  
  VIR_DEBUG(label=%s, secdef-label);
 -if (!STREQ(virSecurityManagerGetModel(mgr), secdef-model)) {
 +if (!STREQ(SECURITY_SELINUX_NAME, secdef-model)) {
  virReportError(VIR_ERR_INTERNAL_ERROR,
 _(security label driver mismatch: 
   '%s' model configured for domain, but 
   hypervisor driver is '%s'.),
 -   secdef-model, virSecurityManagerGetModel(mgr));
 +   secdef-model, SECURITY_SELINUX_NAME);
  if (security_getenforce() == 1)
  return -1;
  }
 @@ -1875,12 +1875,12 @@ 
 virSecuritySELinuxSetSecurityDaemonSocketLabel(virSecurityManagerPtr mgr,
  if (secdef-label == NULL)
  return 0;
  
 -if (!STREQ(virSecurityManagerGetModel(mgr), secdef-model)) {
 +if (!STREQ(SECURITY_SELINUX_NAME, secdef-model)) {
  virReportError(VIR_ERR_INTERNAL_ERROR,
 _(security label driver mismatch: 
   '%s' model configured for domain, but 
   hypervisor driver is '%s'.),
 -   secdef-model, virSecurityManagerGetModel(mgr));
 +   secdef-model, SECURITY_SELINUX_NAME);
  goto done;
  }
  
 @@ -1925,12 +1925,12 @@ 
 virSecuritySELinuxSetSecuritySocketLabel(virSecurityManagerPtr mgr,
  if (secdef-label == NULL)
  return 0;
  
 -if (!STREQ(virSecurityManagerGetModel(mgr), secdef-model)) {
 +if (!STREQ(SECURITY_SELINUX_NAME, secdef-model)) {
  virReportError(VIR_ERR_INTERNAL_ERROR,
 _(security label driver mismatch: 
   '%s' model configured for domain, but 
   hypervisor driver is '%s'.),
 -   secdef-model, virSecurityManagerGetModel(mgr));
 +   secdef-model, SECURITY_SELINUX_NAME);
  goto done;
  }
  
 @@ -1966,12 +1966,12 @@ 
 virSecuritySELinuxClearSecuritySocketLabel(virSecurityManagerPtr mgr,
  if (secdef-label == NULL)
  return 0;
  
 -if (!STREQ(virSecurityManagerGetModel(mgr), secdef-model)) {
 +if (!STREQ(SECURITY_SELINUX_NAME, secdef-model)) {
  virReportError(VIR_ERR_INTERNAL_ERROR,
 _(security label driver mismatch: 
   '%s' model configured for domain, but 
   hypervisor driver is '%s'.),
 -

Re: [libvirt] [PATCH 4/4 v5] virsh: Use virNodeDeviceLookupSCSIHostByWWN

2013-02-11 Thread Osier Yang 

On 2013年02月11日 21:33, Daniel P. Berrange wrote:

On Mon, Feb 04, 2013 at 10:16:44PM +0800, Osier Yang wrote:

Only nodedev-destroy and nodedev-dumpxml can benifit from the
new API, other commands like nodedev-detach only works for
PCI devices, WWN makes no sense for them.

---
Rebased on Peter's virsh cleanup patches.
---
  tools/virsh-nodedev.c |   84 +++-
  tools/virsh.pod   |   15 +---
  2 files changed, 77 insertions(+), 22 deletions(-)

diff --git a/tools/virsh-nodedev.c b/tools/virsh-nodedev.c
index f85bded..7c51a56 100644
--- a/tools/virsh-nodedev.c
+++ b/tools/virsh-nodedev.c
@@ -101,9 +101,14 @@ static const vshCmdInfo info_node_device_destroy[] = {

  static const vshCmdOptDef opts_node_device_destroy[] = {
  {.name = name,
+ .type = VSH_OT_ALIAS,
+ .flags = 0,
+ .help = device
+},
+{.name = device,
   .type = VSH_OT_DATA,
   .flags = VSH_OFLAG_REQ,
- .help = N_(name of the device to be destroyed)
+ .help = N_(device name or wwn pair in 'wwnn,wwpn' format)
  },
  {.name = NULL}
  };
@@ -112,21 +117,47 @@ static bool
  cmdNodeDeviceDestroy(vshControl *ctl, const vshCmd *cmd)
  {
  virNodeDevicePtr dev = NULL;
-bool ret = true;
-const char *name = NULL;
+bool ret = false;
+const char *device_value = NULL;
+char **arr = NULL;
+int narr;

-if (vshCommandOptStringReq(ctl, cmd, name,name)  0)
+if (vshCommandOptStringReq(ctl, cmd, device,device_value)  0)
  return false;

-dev = virNodeDeviceLookupByName(ctl-conn, name);
+if (strchr(device_value, ',')) {
+narr = vshStringToArray(device_value,arr);
+if (narr != 2) {
+vshError(ctl, _(Malformed device value '%s'), device_value);
+goto cleanup;
+}
+
+if (!virValidateWWN(arr[0]) || !virValidateWWN(arr[1]))
+goto cleanup;
+
+dev = virNodeDeviceLookupSCSIHostByWWN(ctl-conn, arr[0], arr[1], 0);
+} else {
+dev = virNodeDeviceLookupByName(ctl-conn, device_value);
+}
+
+if (!dev) {
+vshError(ctl, %s '%s', _(Could not find matching device), 
device_value);
+goto cleanup;
+}

  if (virNodeDeviceDestroy(dev) == 0) {
-vshPrint(ctl, _(Destroyed node device '%s'\n), name);
+vshPrint(ctl, _(Destroyed node device '%s'\n), device_value);
  } else {
-vshError(ctl, _(Failed to destroy node device '%s'), name);
-ret = false;
+vshError(ctl, _(Failed to destroy node device '%s'), device_value);
+goto cleanup;
  }

+ret = true;
+cleanup:
+if (arr) {
+VIR_FREE(*arr);


Hmm, IIUC, that is equiv to VIR_FREE(arr[0]), so what is
free'ing arr[1] ?


vshStringToArray substitutes the delimiter ',' with '\0',
and the elements simply point to the pieces. So freeing
the first element frees all the memory of the array elements.




+VIR_FREE(arr);
+}



ACK if that leak is fixed, or otherwise explained.



Thanks for the reviewing, I pushed this set.

Osier

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

[libvirt] [PATCH v4 0/2] net: support set source address(es) and ports for NAT

2013-02-11 Thread Natanael Copa 
Changes v4:
 - barf if 'end' attribute is missing in address
 - update doc to tell how to properly set single address

Natanael Copa (2):
  net: support set public ip range for forward mode nat
  net: add support for specifying port range for forward mode nat

 docs/formatnetwork.html.in  |  33 
 src/conf/network_conf.c | 195 ++--
 src/conf/network_conf.h |   4 +
 src/network/bridge_driver.c |  32 
 src/util/viriptables.c  |  77 +++--
 src/util/viriptables.h  |   8 ++
 6 files changed, 335 insertions(+), 14 deletions(-)

-- 
1.8.1.2

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

[libvirt] [PATCH v4 1/2] net: support set public ip range for forward mode nat

2013-02-11 Thread Natanael Copa 
Support setting which public ip to use for NAT via attribute
address in subelement nat in forward:

...
  forward mode='nat'
  address start='1.2.3.4' end='1.2.3.10'/
  /forward
...

This will construct an iptables line using:

  '-j SNAT --to-source start-end'

instead of:

  '-j MASQUERADE'

Signed-off-by: Natanael Copa nc...@alpinelinux.org
---
 docs/formatnetwork.html.in  |  18 ++
 src/conf/network_conf.c | 152 ++--
 src/conf/network_conf.h |   3 +
 src/network/bridge_driver.c |  16 +
 src/util/viriptables.c  |  56 +---
 src/util/viriptables.h  |   4 ++
 6 files changed, 235 insertions(+), 14 deletions(-)

diff --git a/docs/formatnetwork.html.in b/docs/formatnetwork.html.in
index 7b42529..5fbd0a9 100644
--- a/docs/formatnetwork.html.in
+++ b/docs/formatnetwork.html.in
@@ -136,6 +136,24 @@
 network, and to/from the host to the guests, are
 unrestricted and not NATed.span class=sinceSince
 0.4.2/span
+
+pspan class=sinceSince 1.0.3/span it is possible to
+specify a public IPv4 address range to be used for the NAT by
+using the codelt;natgt;/code and
+codelt;addressgt;/code subelements.
+pre
+...
+  lt;forward mode='nat'gt;
+lt;natgt;
+  lt;address start='1.2.3.4' end='1.2.3.10'/gt;
+lt;/natgt;
+  lt;/forwardgt;
+...
+/pre
+An singe IPv4 address can be set by setting
+codestart/code and codeend/code attributes to
+the same value.
+/p
   /dd
 
   dtcoderoute/code/dt
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
index 3604ff7..61d086a 100644
--- a/src/conf/network_conf.c
+++ b/src/conf/network_conf.c
@@ -1325,6 +1325,80 @@ cleanup:
 }
 
 static int
+virNetworkForwardNatDefParseXML(const char *networkName,
+xmlNodePtr node,
+xmlXPathContextPtr ctxt,
+virNetworkForwardDefPtr def)
+{
+int ret = -1;
+xmlNodePtr *natAddrNodes = NULL;
+int nNatAddrs;
+char *addr_start = NULL;
+char *addr_end = NULL;
+xmlNodePtr save = ctxt-node;
+
+ctxt-node = node;
+
+if (def-type != VIR_NETWORK_FORWARD_NAT) {
+virReportError(VIR_ERR_XML_ERROR,
+   _(The nat element can only be used when forward 
'mode' is 'nat' in network %s),
+   networkName);
+goto cleanup;
+}
+
+/* addresses for SNAT */
+nNatAddrs = virXPathNodeSet(./address, ctxt, natAddrNodes);
+if (nNatAddrs  0) {
+virReportError(VIR_ERR_XML_ERROR,
+   _(invalid address element found in forward of 
+ network %s), networkName);
+goto cleanup;
+} else if (nNatAddrs  1) {
+virReportError(VIR_ERR_XML_ERROR,
+   _(Only one address element is allowed in nat in 
+ forward in network %s), networkName);
+goto cleanup;
+} else if (nNatAddrs == 1) {
+addr_start = virXMLPropString(*natAddrNodes, start);
+if (addr_start == NULL) {
+virReportError(VIR_ERR_XML_ERROR,
+   _(missing 'start' attribute in address element 
in nat in 
+ forward in network %s), networkName);
+goto cleanup;
+}
+addr_end = virXMLPropString(*natAddrNodes, end);
+if (addr_end == NULL) {
+virReportError(VIR_ERR_XML_ERROR,
+   _(missing 'end' attribute in address element in 
nat in 
+ forward in network %s), networkName);
+goto cleanup;
+}
+}
+
+if (addr_start  virSocketAddrParse(def-addr_start, addr_start, 
AF_INET)  0) {
+virReportError(VIR_ERR_XML_ERROR,
+   _(Bad ipv4 start address '%s' in nat in forward in 

+ network '%s'), addr_start, networkName);
+goto cleanup;
+}
+
+if (addr_end  virSocketAddrParse(def-addr_end, addr_end, AF_INET)  0) 
{
+virReportError(VIR_ERR_XML_ERROR,
+   _(Bad ipv4 end address '%s' in nat in forward in 
+ network '%s'), addr_end, networkName);
+goto cleanup;
+}
+
+ret = 0;
+
+cleanup:
+VIR_FREE(addr_start);
+VIR_FREE(addr_end);
+ctxt-node = save;
+return ret;
+}
+
+static int
 virNetworkForwardDefParseXML(const char *networkName,
  xmlNodePtr node,
  xmlXPathContextPtr ctxt,
@@ -1334,7 +1408,8 @@ virNetworkForwardDefParseXML(const char *networkName,
 xmlNodePtr *forwardIfNodes = NULL;
 xmlNodePtr *forwardPfNodes = NULL;
 xmlNodePtr *forwardAddrNodes = NULL;
-int nForwardIfs, nForwardAddrs, nForwardPfs;
+xmlNodePtr *forwardNatNodes = NULL;
+int

[libvirt] [PATCH v4 2/2] net: add support for specifying port range for forward mode nat

2013-02-11 Thread Natanael Copa 
Let users set the port range to be used for forward mode NAT:

...
  forward mode='nat'
nat
  port start='1024' end='65535'/
/nat
  /forward
...

Signed-off-by: Natanael Copa nc...@alpinelinux.org
---
 docs/formatnetwork.html.in  | 21 ++---
 src/conf/network_conf.c | 57 +++--
 src/conf/network_conf.h |  3 ++-
 src/network/bridge_driver.c | 16 +
 src/util/viriptables.c  | 39 ---
 src/util/viriptables.h  |  4 
 6 files changed, 120 insertions(+), 20 deletions(-)

diff --git a/docs/formatnetwork.html.in b/docs/formatnetwork.html.in
index 5fbd0a9..adb5bb9 100644
--- a/docs/formatnetwork.html.in
+++ b/docs/formatnetwork.html.in
@@ -138,9 +138,11 @@
 0.4.2/span
 
 pspan class=sinceSince 1.0.3/span it is possible to
-specify a public IPv4 address range to be used for the NAT by
-using the codelt;natgt;/code and
-codelt;addressgt;/code subelements.
+specify a public IPv4 address and port range to be used for
+the NAT by using the codelt;natgt;/code subelement.
+The address range is set with the codelt;addressgt;/code
+subelements and codestart/code and codestop/code
+attributes:
 pre
 ...
   lt;forward mode='nat'gt;
@@ -154,6 +156,19 @@
 codestart/code and codeend/code attributes to
 the same value.
 /p
+p
+The port range to be used for the codelt;natgt;/code can
+be set via the subelement codelt;portgt;/code:
+pre
+...
+  lt;forward mode='nat'gt;
+lt;natgt;
+  lt;port start='500' end='1000'/gt;
+lt;/natgt;
+  lt;/forwardgt;
+...
+/pre
+/p
   /dd
 
   dtcoderoute/code/dt
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
index 61d086a..5725800 100644
--- a/src/conf/network_conf.c
+++ b/src/conf/network_conf.c
@@ -1332,7 +1332,8 @@ virNetworkForwardNatDefParseXML(const char *networkName,
 {
 int ret = -1;
 xmlNodePtr *natAddrNodes = NULL;
-int nNatAddrs;
+xmlNodePtr *natPortNodes = NULL;
+int nNatAddrs, nNatPorts;
 char *addr_start = NULL;
 char *addr_end = NULL;
 xmlNodePtr save = ctxt-node;
@@ -1389,6 +1390,36 @@ virNetworkForwardNatDefParseXML(const char *networkName,
 goto cleanup;
 }
 
+/* ports for SNAT and MASQUERADE */
+nNatPorts = virXPathNodeSet(./port, ctxt, natPortNodes);
+if (nNatPorts  0) {
+virReportError(VIR_ERR_XML_ERROR,
+   _(invalid port element found in forward of 
+ network %s), networkName);
+goto cleanup;
+} else if (nNatPorts  1) {
+virReportError(VIR_ERR_XML_ERROR,
+   _(Only one port element is allowed in nat in 
+ forward in network %s), networkName);
+goto cleanup;
+} else if (nNatPorts == 1) {
+if (virXPathUInt(string(./port[1]/@start), ctxt, def-port_start)  0
+|| def-port_start  65535) {
+
+virReportError(VIR_ERR_XML_DETAIL,
+   _(Missing or invalid 'start' attribute in port 
+ in nat in forward in network %s),
+ networkName);
+goto cleanup;
+}
+if (virXPathUInt(string(./port[1]/@end), ctxt, def-port_end)  0
+|| def-port_end  65535 || def-port_end  def-port_start) {
+virReportError(VIR_ERR_XML_DETAIL,
+   _(Missing or invalid 'end' attribute in port in 
+ nat in forward in network %s), networkName);
+goto cleanup;
+}
+}
 ret = 0;
 
 cleanup:
@@ -2179,6 +2210,7 @@ virNatDefFormat(virBufferPtr buf,
 char *addr_start = NULL;
 char *addr_end = NULL;
 int ret = -1;
+int longdef;
 
 if (VIR_SOCKET_ADDR_VALID(fwd-addr_start)) {
 addr_start = virSocketAddrFormat(fwd-addr_start);
@@ -2192,16 +2224,25 @@ virNatDefFormat(virBufferPtr buf,
 goto cleanup;
 }
 
-if (!addr_end  !addr_start)
+if (!addr_start  !addr_end  !fwd-port_start  !fwd-port_end)
 return 0;
 
 virBufferAddLit(buf, nat\n);
 virBufferAdjustIndent(buf, 2);
 
-virBufferAsprintf(buf, address start='%s', addr_start);
-if (addr_end)
-virBufferAsprintf(buf,  end='%s', addr_end);
-virBufferAsprintf(buf, /\n);
+if (addr_start) {
+virBufferAsprintf(buf, address start='%s', addr_start);
+if (addr_end)
+virBufferAsprintf(buf,  end='%s', addr_end);
+virBufferAsprintf(buf, /\n);
+}
+
+if (fwd-port_start || fwd-port_end) {
+virBufferAsprintf(buf, port start='%d', fwd-port_start);
+if (fwd-port_end)
+virBufferAsprintf(buf,  end='%d', fwd-port_end);
+

Re: [libvirt] [PATCH 4/4 v5] virsh: Use virNodeDeviceLookupSCSIHostByWWN

2013-02-11 Thread Osier Yang 

On 2013年02月11日 22:51, Osier Yang wrote:

On 2013年02月11日 21:33, Daniel P. Berrange wrote:

On Mon, Feb 04, 2013 at 10:16:44PM +0800, Osier Yang wrote:

Only nodedev-destroy and nodedev-dumpxml can benifit from the
new API, other commands like nodedev-detach only works for
PCI devices, WWN makes no sense for them.

---
Rebased on Peter's virsh cleanup patches.
---
tools/virsh-nodedev.c | 84
+++-
tools/virsh.pod | 15 +---
2 files changed, 77 insertions(+), 22 deletions(-)

diff --git a/tools/virsh-nodedev.c b/tools/virsh-nodedev.c
index f85bded..7c51a56 100644
--- a/tools/virsh-nodedev.c
+++ b/tools/virsh-nodedev.c
@@ -101,9 +101,14 @@ static const vshCmdInfo
info_node_device_destroy[] = {

static const vshCmdOptDef opts_node_device_destroy[] = {
{.name = name,
+ .type = VSH_OT_ALIAS,
+ .flags = 0,
+ .help = device
+ },
+ {.name = device,
.type = VSH_OT_DATA,
.flags = VSH_OFLAG_REQ,
- .help = N_(name of the device to be destroyed)
+ .help = N_(device name or wwn pair in 'wwnn,wwpn' format)
},
{.name = NULL}
};
@@ -112,21 +117,47 @@ static bool
cmdNodeDeviceDestroy(vshControl *ctl, const vshCmd *cmd)
{
virNodeDevicePtr dev = NULL;
- bool ret = true;
- const char *name = NULL;
+ bool ret = false;
+ const char *device_value = NULL;
+ char **arr = NULL;
+ int narr;

- if (vshCommandOptStringReq(ctl, cmd, name,name) 0)
+ if (vshCommandOptStringReq(ctl, cmd, device,device_value) 0)
return false;

- dev = virNodeDeviceLookupByName(ctl-conn, name);
+ if (strchr(device_value, ',')) {
+ narr = vshStringToArray(device_value,arr);
+ if (narr != 2) {
+ vshError(ctl, _(Malformed device value '%s'), device_value);
+ goto cleanup;
+ }
+
+ if (!virValidateWWN(arr[0]) || !virValidateWWN(arr[1]))
+ goto cleanup;
+
+ dev = virNodeDeviceLookupSCSIHostByWWN(ctl-conn, arr[0], arr[1], 0);
+ } else {
+ dev = virNodeDeviceLookupByName(ctl-conn, device_value);
+ }
+
+ if (!dev) {
+ vshError(ctl, %s '%s', _(Could not find matching device),
device_value);
+ goto cleanup;
+ }

if (virNodeDeviceDestroy(dev) == 0) {
- vshPrint(ctl, _(Destroyed node device '%s'\n), name);
+ vshPrint(ctl, _(Destroyed node device '%s'\n), device_value);
} else {
- vshError(ctl, _(Failed to destroy node device '%s'), name);
- ret = false;
+ vshError(ctl, _(Failed to destroy node device '%s'), device_value);
+ goto cleanup;
}

+ ret = true;
+cleanup:
+ if (arr) {
+ VIR_FREE(*arr);


Hmm, IIUC, that is equiv to VIR_FREE(arr[0]), so what is
free'ing arr[1] ?


vshStringToArray substitutes the delimiter ',' with '\0',
and the elements simply point to the pieces. So freeing
the first element frees all the memory of the array elements.



Btw, I think it's time to destroy the use of vshStringToArray
, and use the more general virStringSplit now, (which not only
supports the delimiter ','). It will be later patch.




+ VIR_FREE(arr);
+ }



ACK if that leak is fixed, or otherwise explained.



Thanks for the reviewing, I pushed this set.

Osier

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


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

Re: [libvirt] [PATCH 00/11] Revisit xen driver Coverity cleanup changes

2013-02-11 Thread John Ferlan 

Thanks for the reviews - this is now pushed

John

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

Re: [libvirt] [PATCHv2 1/5] S390: Documentation for CCW address type

2013-02-11 Thread Daniel P. Berrange 
On Fri, Feb 08, 2013 at 06:32:20PM +0100, Viktor Mihajlovski wrote:
 The native bus for s390 I/O is called CCW (channel command word).
 As QEMU has added basic support for the CCW bus, i.e. the
 ability to assign CCW devnos (bus addresses) to devices.
 Domains with the new machine type s390-ccw-virtio can use the
 CCW bus. Currently QEMU will only allow to define virtio
 devices on the CCW bus.
 Here we add the new machine type and the new device address to the
 schema definition and add a new paragraph to the domain XML
 documentation.
 
 Signed-off-by: Viktor Mihajlovski mihaj...@linux.vnet.ibm.com
 ---
 V2 Changes
  - replace single devno attribute with cssid, ssid, schid triple
in documentation section
  - add new data ranges for cssid, ssid and schid to domain schema
 
  docs/formatdomain.html.in |   14 +++
  docs/schemas/domaincommon.rng |   52 
 +

ACK


Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

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

Re: [libvirt] [PATCHv2 2/5] S390: domain_conf support for CCW

2013-02-11 Thread Daniel P. Berrange 
On Fri, Feb 08, 2013 at 06:32:21PM +0100, Viktor Mihajlovski wrote:
 Add necessary handling code for the new s390 CCW address type to
 virDomainDeviceInfo. Further, introduce  memory management, XML
 parsing, output formatting and range validation for the new
 virDomainDeviceCCWAddress type.
 
 Signed-off-by: Viktor Mihajlovski mihaj...@linux.vnet.ibm.com
 ---
 V2 Changes
  - adapted virDomainDeviceCCWAddressParseXML to handle the new
set of CCW address attributes
 
  src/conf/domain_conf.c   |  107 
 +-
  src/conf/domain_conf.h   |   16 +++
  src/libvirt_private.syms |1 +
  3 files changed, 123 insertions(+), 1 deletion(-)

ACK

Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

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

Re: [libvirt] [PATCHv2 3/5] S390: QEMU driver support for CCW addresses

2013-02-11 Thread Daniel P. Berrange 
On Fri, Feb 08, 2013 at 06:32:22PM +0100, Viktor Mihajlovski wrote:
 This commit adds the QEMU driver support for CCW addresses. The
 current QEMU only allows virtio devices to be attached to the
 CCW bus. We named the new capability indicating that support
 QEMU_CAPS_VIRTIO_CCW accordingly.
 
 The fact that CCW devices can only be assigned to domains with a
 machine type of s390-ccw-virtio requires a modification in the
 capability handling approach.
 
 First, the QEMU binary name alone will not suffice for capability
 lookup, we need the machine type as well. For that purpose we
 mangle the machine type into the cache lookup key.
 
 The other thing is that the device support probing will
 unfortunately always return both the old virtio-*-s390
 and the new virtio-*-ccw devices. This makes it impossible to
 choose the correct default device address type if the domain
 definition XML doesn't contain explict addresses.
 Therefore we apply a fix up in the cache lookup: depending
 on the machine type we clear either the VIRTIO_S390 or the
 VIRTIO_CCW capability.

I'm not a fan of this approach. The capabilities data is reflecting
what the QEMU binary is capable of supporting, *regardless* of what
guest config is chosen.

The decision about whether to use S390 or CCW based on the machine
type value, should be made by the internal method at the point when
it actually assigns addresses, not when we create the capabilities
initially.

Your approach here means we're going to be storing many many more
capabilities instances, and we're going to be re-probing the QEMU
binaries for each machine type. This is absolutely not something
we want todo.


Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

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

Re: [libvirt] [PATCH] Remove re-entrant API call in SELinux/AppArmor security managers

2013-02-11 Thread Eric Blake 
On 02/11/2013 07:50 AM, Richard W.M. Jones wrote:
 On Mon, Feb 11, 2013 at 02:26:15PM +, Daniel P. Berrange wrote:
 From: Daniel P. Berrange berra...@redhat.com

 The security manager drivers are not allowed to call back
 out to top level security manager APIs, since that results
 in recursive mutex acquisition and thus deadlock. Remove
 calls to virSecurityManagerGetModel from SELinux / AppArmor
 drivers


 
 The patch causes the following warning:
 
 security/security_selinux.c: In function 
 'virSecuritySELinuxSetSecurityProcessLabel':
 security/security_selinux.c:1826:65: error: unused parameter 'mgr' 
 [-Werror=unused-parameter]

These can be fixed by adding ATTRIBUTE_UNUSED in the function signature.

 I switched off warnings and compiled libvirt with the patch anyway,
 and it fixed the problem for me (both libvirtd not being able to be
 killed, and libvirtd not starting up the libguestfs appliance).

I concur with the patch; ACK with the warnings addressed.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



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

[libvirt] [PATCH] hypervisor: Restore pm initialization

2013-02-11 Thread John Ferlan 
Adjustment for 'c059cdeaf' due to older compiler complaint about pm
not being initialized even though the j7 == 0 does the trick.
---
 src/xen/xen_hypervisor.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/xen/xen_hypervisor.c b/src/xen/xen_hypervisor.c
index 767fc0c..9b7dd2e 100644
--- a/src/xen/xen_hypervisor.c
+++ b/src/xen/xen_hypervisor.c
@@ -1773,7 +1773,7 @@ virXen_setvcpumap(int handle,
 ret = -1;
 } else {
 cpumap_t xen_cpumap; /* limited to 64 CPUs in old hypervisors */
-uint64_t *pm;
+uint64_t *pm = xen_cpumap;
 int j;
 
 if ((maplen  (int)sizeof(cpumap_t)) || (sizeof(cpumap_t)  7))
-- 
1.7.11.7

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

[libvirt] [PATCH] Fix potential deadlock across fork() in QEMU driver

2013-02-11 Thread Daniel P. Berrange 
From: Daniel P. Berrange berra...@redhat.com

The hook scripts used by virCommand must be careful wrt
accessing any mutexes that may have been held by other
threads in the parent process. With the recent refactorigng
there are 2 potential flaws lurking, which will become real
deadlock bugs once the global QEMU driver lock is removed.

Remove use of the QEMU driver lock from the hook function
by passing in the 'virQEMUDriverConfigPtr' instance directly.

Add functions to the virSecurityManager to be invoked before
and after fork, to ensure the mutex is held by the current
thread. This allows it to be safely used in the hook script
in the child procss.

Signed-off-by: Daniel P. Berrange berra...@redhat.com
---
 src/libvirt_private.syms|  2 ++
 src/qemu/qemu_process.c | 16 
 src/security/security_manager.c | 20 
 src/security/security_manager.h |  3 +++
 4 files changed, 37 insertions(+), 4 deletions(-)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index cb81497..5f19269 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1054,6 +1054,8 @@ virSecurityManagerGetProcessLabel;
 virSecurityManagerNew;
 virSecurityManagerNewDAC;
 virSecurityManagerNewStack;
+virSecurityManagerPostFork;
+virSecurityManagerPreFork;
 virSecurityManagerReleaseLabel;
 virSecurityManagerReserveLabel;
 virSecurityManagerRestoreAllLabel;
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 9759332..12e3544 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -2773,6 +2773,7 @@ struct qemuProcessHookData {
 virDomainObjPtr vm;
 virQEMUDriverPtr driver;
 virBitmapPtr nodemask;
+virQEMUDriverConfigPtr cfg;
 };
 
 static int qemuProcessHook(void *data)
@@ -2780,7 +2781,11 @@ static int qemuProcessHook(void *data)
 struct qemuProcessHookData *h = data;
 int ret = -1;
 int fd;
-virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(h-driver);
+/* This method cannot use any mutexes, which are not
+ * protected across fork()
+ */
+
+virSecurityManagerPostFork(h-driver-securityManager);
 
 /* Some later calls want pid present */
 h-vm-pid = getpid();
@@ -2796,7 +2801,7 @@ static int qemuProcessHook(void *data)
 if (virSecurityManagerSetSocketLabel(h-driver-securityManager, 
h-vm-def)  0)
 goto cleanup;
 if (virDomainLockProcessStart(h-driver-lockManager,
-  cfg-uri,
+  h-cfg-uri,
   h-vm,
   /* QEMU is always paused initially */
   true,
@@ -2805,7 +2810,7 @@ static int qemuProcessHook(void *data)
 if (virSecurityManagerClearSocketLabel(h-driver-securityManager, 
h-vm-def)  0)
 goto cleanup;
 
-if (qemuProcessLimits(cfg)  0)
+if (qemuProcessLimits(h-cfg)  0)
 goto cleanup;
 
 /* This must take place before exec(), so that all QEMU
@@ -2831,7 +2836,7 @@ static int qemuProcessHook(void *data)
 ret = 0;
 
 cleanup:
-virObjectUnref(cfg);
+virObjectUnref(h-cfg);
 VIR_DEBUG(Hook complete ret=%d, ret);
 return ret;
 }
@@ -3642,6 +3647,7 @@ int qemuProcessStart(virConnectPtr conn,
 hookData.conn = conn;
 hookData.vm = vm;
 hookData.driver = driver;
+hookData.cfg = virObjectRef(cfg);
 
 VIR_DEBUG(Beginning VM startup process);
 
@@ -3973,7 +3979,9 @@ int qemuProcessStart(virConnectPtr conn,
 virCommandDaemonize(cmd);
 virCommandRequireHandshake(cmd);
 
+virSecurityManagerPreFork(driver-securityManager);
 ret = virCommandRun(cmd, NULL);
+virSecurityManagerPostFork(driver-securityManager);
 
 /* wait for qemu process to show up */
 if (ret == 0) {
diff --git a/src/security/security_manager.c b/src/security/security_manager.c
index 6f8ddbf..50962ba 100644
--- a/src/security/security_manager.c
+++ b/src/security/security_manager.c
@@ -192,6 +192,26 @@ virSecurityManagerPtr virSecurityManagerNew(const char 
*name,
requireConfined);
 }
 
+
+/*
+ * Must be called before fork()'ing to ensure mutex state
+ * is sane for the child to use
+ */
+void virSecurityManagerPreFork(virSecurityManagerPtr mgr)
+{
+virObjectLock(mgr);
+}
+
+
+/*
+ * Must be called after fork()'ing in both parent and child
+ * to ensure mutex state is sane for the child to use
+ */
+void virSecurityManagerPostFork(virSecurityManagerPtr mgr)
+{
+virObjectUnlock(mgr);
+}
+
 void *virSecurityManagerGetPrivateData(virSecurityManagerPtr mgr)
 {
 return mgr-privateData;
diff --git a/src/security/security_manager.h b/src/security/security_manager.h
index 4d4dc73..8e8accf 100644
--- a/src/security/security_manager.h
+++ b/src/security/security_manager.h
@@ -46,6 +46,9 @@ virSecurityManagerPtr virSecurityManagerNewDAC(const char 
*virtDriver,
bool

[libvirt] [PATCH V5 3/3] Add support for file descriptor sets

2013-02-11 Thread Stefan Berger 
Add support for file descriptor sets by converting some of the 
command line parameters to use /dev/fdset/%d if -add-fd is found
to be supported by QEMU. For those devices libvirt now open()s
the device to obtain the file descriptor and 'transfers' the 
fd using virCommand.

For the following fragments of domain XML


disk type='file' device='disk'
  driver name='qemu' type='raw'/
  source file='/var/lib/libvirt/images/fc14-x86_64.img'/
  target dev='hda' bus='ide'/
  address type='drive' controller='0' bus='0' target='0' unit='0'/
/disk

   serial type='dev'
  source path='/dev/ttyS0'/
  target port='0'/
/serial
serial type='pipe'
  source path='/tmp/testpipe'/
  target port='1'/
/serial

libvirt now creates the following parts for the QEMU command line:

old: -drive 
file=/var/lib/libvirt/images/fc14-x86_64.img,if=none,id=drive-ide0-0-0,format=raw
new: -add-fd set=1,fd=23,opaque=RDONLY:/var/lib/libvirt/images/fc14-x86_64.img
 -add-fd set=1,fd=24,opaque=RDWR:/var/lib/libvirt/images/fc14-x86_64.img
 -drive file=/dev/fdset/1,if=none,id=drive-ide0-0-0,format=raw

old: -chardev tty,id=charserial0,path=/dev/ttyS0
new: -add-fd set=1,fd=30,opaque=/dev/ttyS0
 -chardev tty,id=charserial0,path=/dev/fdset/1

old: -chardev pipe,id=charserial1,path=/tmp/testpipe
new: -add-fd set=2,fd=32,opaque=/tmp/testpipe
 -chardev pipe,id=charserial1,path=/dev/fdset/2

Test cases are part of this patch now.

Signed-off-by: Stefan Berger stef...@linux.vnet.ibm.com

---
v4-v5:
   - removed one test case testing for 'old' command line

v3-v4:
   - Adapt to changes in patch 1
   - better handling of error case on the hotplug code

v2-v3:
   - Use an unsigned int for remembering the next-to-use fdset

v1-v2:
   - Addressed many of Eric's comment; many changes, though
   - virBitmap holds used file descriptor sets; it's attached to
 QEMU private domain structure
   - persisting and parsing the fdset in the virDomainDeviceInfo XML
   - rebuilding the fdset bitmap upon libvirt start and after parsing
 the virDomainDeviceInfo XML

---
 src/qemu/qemu_command.c |  244 +++-
 src/qemu/qemu_command.h |   14 +
 src/qemu/qemu_driver.c  |5 
 src/qemu/qemu_driver.h  |4 
 src/qemu/qemu_hotplug.c |   15 +
 src/qemu/qemu_process.c |6 
 tests/qemuxml2argvdata/qemuxml2argv-add-fd.args |   23 ++
 tests/qemuxml2argvdata/qemuxml2argv-add-fd.xml  |   36 +++
 tests/qemuxml2argvtest.c|   11 -
 tests/qemuxmlnstest.c   |8 
 10 files changed, 309 insertions(+), 57 deletions(-)

Index: libvirt/src/qemu/qemu_command.c
===
--- libvirt.orig/src/qemu/qemu_command.c
+++ libvirt/src/qemu/qemu_command.c
@@ -46,6 +46,7 @@
 #include base64.h
 #include device_conf.h
 #include virstoragefile.h
+#include qemu_driver.h
 
 #include sys/stat.h
 #include fcntl.h
@@ -133,6 +134,70 @@ VIR_ENUM_IMPL(qemuDomainFSDriver, VIR_DO
 
 
 /**
+ * qemuCommandPrintFDSet:
+ * @fdset: the number of the file descriptor set to which @fd belongs
+ * @fd: fd that will be assigned to QEMU
+ * @open_flags: the flags used for opening the fd; of interest are only
+ *   O_RDONLY, O_WRONLY, O_RDWR
+ * @name: optional name of the file
+ *
+ * Write the parameters for the QEMU -add-fd command line option
+ * for the given file descriptor and return the string.
+ * This function for example returns set=10,fd=20,opaque=RDWR:/foo/bar.
+ */
+static char *
+qemuCommandPrintFDSet(int fdset, int fd, int open_flags, const char *name)
+{
+const char *mode = ;
+virBuffer buf = VIR_BUFFER_INITIALIZER;
+
+if (name) {
+switch ((open_flags  O_ACCMODE)) {
+case O_RDONLY:
+mode = RDONLY:;
+break;
+case O_WRONLY:
+mode = WRONLY:;
+break;
+case O_RDWR:
+mode = RDWR:;
+break;
+}
+}
+
+virBufferAsprintf(buf, set=%d,fd=%d%s%s, fdset, fd,
+  (name) ? ,opaque= : ,
+  mode);
+if (name)
+virBufferEscape(buf, ',', ,, %s, name);
+
+if (virBufferError(buf)) {
+virReportOOMError();
+virBufferFreeAndReset(buf);
+return NULL;
+}
+
+return virBufferContentAndReset(buf);
+}
+
+/**
+ * qemuCommandPrintDevSet:
+ * @buf: buffer to write the file descriptor set string
+ * @fdset: the file descriptor set
+ *
+ * Get the parameters for the QEMU path= parameter where a file
+ * descriptor is accessed via a file descriptor set, for example
+ * /dev/fdset/10. The file descriptor must previously have been
+ * 'transferred' in a virCommandTransfer() call.
+ */
+static void
+qemuCommandPrintDevSet(virBufferPtr buf, int fdset)
+{
+virBufferAsprintf(buf,

[libvirt] [PATCH] Check if classes are derived from object

2013-02-11 Thread Guido Günther 
This makes sure we don't regress to old style classes
---
Just a minor addition that came up while verifying if the corresponding
Debian bug is fixed.

 python/sanitytest.py |   27 ---
 1 file changed, 16 insertions(+), 11 deletions(-)

diff --git a/python/sanitytest.py b/python/sanitytest.py
index 047450b..ace6792 100644
--- a/python/sanitytest.py
+++ b/python/sanitytest.py
@@ -7,17 +7,22 @@ globals = dir(libvirt)
 # Sanity test that the generator hasn't gone wrong
 
 # Look for core classes
-assert(virConnect in globals)
-assert(virDomain in globals)
-assert(virDomainSnapshot in globals)
-assert(virInterface in globals)
-assert(virNWFilter in globals)
-assert(virNodeDevice in globals)
-assert(virNetwork in globals)
-assert(virSecret in globals)
-assert(virStoragePool in globals)
-assert(virStorageVol in globals)
-assert(virStream in globals)
+for clsname in [virConnect,
+virDomain,
+virDomainSnapshot,
+virInterface,
+virNWFilter,
+virNodeDevice,
+virNetwork,
+virSecret,
+virStoragePool,
+virStorageVol,
+virStream,
+]:
+assert(clsname in globals)
+assert(object in getattr(libvirt, clsname).__bases__)
+
+# Constants
 assert(VIR_CONNECT_RO in globals)
 
 # Error related bits
-- 
1.7.10.4

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

[libvirt] [PATCH V5 1/3] Add a class for file descriptor sets

2013-02-11 Thread Stefan Berger 
Rather than passing the next-to-use file descriptor set Id
and the hash table for rembering the mappings of aliases to
file descriptor sets around, encapsulate the two in a class.

Signed-off-by: Stefan Berger stef...@linux.vnet.ibm.com

---
 v4-v5:
  - followed Daniel Berrange's comments
  - converted to virObject
  - provide virFdSetNew

---
 src/Makefile.am  |1 
 src/libvirt_private.syms |   10 ++
 src/util/virfdset.c  |  205 +++
 src/util/virfdset.h  |  112 +
 4 files changed, 328 insertions(+)

Index: libvirt/src/util/virfdset.c
===
--- /dev/null
+++ libvirt/src/util/virfdset.c
@@ -0,0 +1,205 @@
+/*
+ * virfdset.c: File descriptor set support
+ *
+ * Copyright (C) 2013 IBM Corporation
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library.  If not, see
+ * http://www.gnu.org/licenses/.
+ *
+ * Author: Stefan Berger stef...@linux.vnet.ibm.com
+ */
+
+#include config.h
+
+#include virfdset.h
+#include virobject.h
+#include viralloc.h
+#include virutil.h
+#include virerror.h
+#include virbuffer.h
+
+#define VIR_FROM_THIS VIR_FROM_NONE
+
+
+struct _virFdSet {
+virObject object;
+virHashTablePtr aliasToFdSet;
+unsigned int nextfdset;
+};
+
+static virClassPtr virFdSetClass;
+static void virFdSetDispose(void *obj);
+
+static int virFdSetOnceInit(void)
+{
+if (!(virFdSetClass = virClassNew(virClassForObject(),
+  virFdSet,
+  sizeof(virFdSet),
+  virFdSetDispose)))
+return -1;
+
+return 0;
+}
+
+VIR_ONCE_GLOBAL_INIT(virFdSet)
+
+static void virFdSetDispose(void *obj)
+{
+virFdSetPtr fdset = obj;
+
+virHashFree(fdset-aliasToFdSet);
+}
+
+static void virFdSetFreeIntPtr(void *payload,
+   const void *name ATTRIBUTE_UNUSED)
+{
+VIR_FREE(payload);
+}
+
+virFdSetPtr virFdSetNew(void)
+{
+virFdSetPtr fdset;
+
+if (virFdSetInitialize()  0)
+return NULL;
+
+if (!(fdset = virObjectNew(virFdSetClass)))
+return NULL;
+
+if (!(fdset-aliasToFdSet = virHashCreate(10, virFdSetFreeIntPtr))) {
+virObjectUnref(fdset);
+return NULL;
+}
+fdset-nextfdset = 1;
+
+return fdset;
+}
+
+void virFdSetFree(virFdSetPtr fdset)
+{
+virObjectUnref(fdset);
+}
+
+void virFdSetReset(virFdSetPtr fdset)
+{
+virHashRemoveAll(fdset-aliasToFdSet);
+fdset-nextfdset = 1;
+}
+
+void virFdSetRemoveAlias(virFdSetPtr fdset, const char *alias)
+{
+   virHashRemoveEntry(fdset-aliasToFdSet, alias);
+}
+
+int virFdSetNextSet(virFdSetPtr fdset, const char *alias,
+unsigned int *fdsetnum)
+{
+int *num;
+
+if (VIR_ALLOC(num)  0) {
+virReportOOMError();
+return -1;
+}
+
+*num = fdset-nextfdset;
+
+if (virHashAddEntry(fdset-aliasToFdSet, alias, num)  0) {
+VIR_FREE(num);
+return -1;
+}
+
+*fdsetnum = fdset-nextfdset++;
+
+return 0;
+}
+
+static void virFdSetPrintAliasToFdSet(void *payload,
+  const void *name,
+  void *data)
+{
+virBufferPtr buf = data;
+
+virBufferAsprintf(buf,   entry alias='%s' fdset='%u'/\n,
+  (char *)name,
+  *(unsigned int *)payload);
+}
+
+void virFdSetFormatXML(virFdSetPtr fdset, virBufferPtr buf)
+{
+virBufferAsprintf(buf, fdsets\n);
+virHashForEach(fdset-aliasToFdSet, virFdSetPrintAliasToFdSet, buf);
+virBufferAsprintf(buf, /fdsets\n);
+}
+
+int virFdSetParseXML(virFdSetPtr fdset, const char *xPath,
+ xmlXPathContextPtr ctxt)
+{
+xmlNodePtr *nodes = NULL;
+int n, i;
+char *key = NULL;
+char *val = NULL;
+unsigned int *fdsetnum = NULL;
+int ret = 0;
+
+virFdSetReset(fdset);
+
+if ((n = virXPathNodeSet(xPath, ctxt, nodes))  0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   %s, _(failed to parse qemu file descriptor sets));
+goto error;
+}
+if (n  0) {
+for (i = 0 ; i  n ; i++) {
+key = virXMLPropString(nodes[i], alias);
+val = virXMLPropString(nodes[i], fdset);
+if (key  val) {
+if (VIR_ALLOC(fdsetnum)  0) {

[libvirt] [PATCH V5 0/3] Add support for QEMU file descriptor sets

2013-02-11 Thread Stefan Berger 
The following patch series adds initial support for QEMU file
descriptor sets implementing support for creating the proper
command line. Some devices are using the sets now.

Regards,
Stefan

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

Re: [libvirt] [PATCH] Check if classes are derived from object

2013-02-11 Thread Daniel P. Berrange 
On Mon, Feb 11, 2013 at 05:20:31PM +0100, Guido Günther wrote:
 This makes sure we don't regress to old style classes
 ---
 Just a minor addition that came up while verifying if the corresponding
 Debian bug is fixed.
 
  python/sanitytest.py |   27 ---
  1 file changed, 16 insertions(+), 11 deletions(-)
 
 diff --git a/python/sanitytest.py b/python/sanitytest.py
 index 047450b..ace6792 100644
 --- a/python/sanitytest.py
 +++ b/python/sanitytest.py
 @@ -7,17 +7,22 @@ globals = dir(libvirt)
  # Sanity test that the generator hasn't gone wrong
  
  # Look for core classes
 -assert(virConnect in globals)
 -assert(virDomain in globals)
 -assert(virDomainSnapshot in globals)
 -assert(virInterface in globals)
 -assert(virNWFilter in globals)
 -assert(virNodeDevice in globals)
 -assert(virNetwork in globals)
 -assert(virSecret in globals)
 -assert(virStoragePool in globals)
 -assert(virStorageVol in globals)
 -assert(virStream in globals)
 +for clsname in [virConnect,
 +virDomain,
 +virDomainSnapshot,
 +virInterface,
 +virNWFilter,
 +virNodeDevice,
 +virNetwork,
 +virSecret,
 +virStoragePool,
 +virStorageVol,
 +virStream,
 +]:
 +assert(clsname in globals)
 +assert(object in getattr(libvirt, clsname).__bases__)
 +
 +# Constants
  assert(VIR_CONNECT_RO in globals)

ACK, good idea.


Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

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

[libvirt] [PATCH V5 2/3] Introduce file descriptor set for QEMU domains

2013-02-11 Thread Stefan Berger 
Extend the QEMU private domain structure with virFdSet.
Persist the virFdSet using XML and parse its XML.
Reset the FdSet upon domain stop.

Stefan Berger stef...@linux.vnet.ibm.com

---
 src/qemu/qemu_domain.c  |   13 +
 src/qemu/qemu_domain.h  |3 +++
 src/qemu/qemu_process.c |2 ++
 3 files changed, 18 insertions(+)

Index: libvirt/src/qemu/qemu_domain.c
===
--- libvirt.orig/src/qemu/qemu_domain.c
+++ libvirt/src/qemu/qemu_domain.c
@@ -212,6 +212,9 @@ static void *qemuDomainObjPrivateAlloc(v
 if (VIR_ALLOC(priv)  0)
 return NULL;
 
+if (!(priv-fdset = virFdSetNew()))
+goto error;
+
 if (qemuDomainObjInitJob(priv)  0)
 goto error;
 
@@ -223,6 +226,7 @@ static void *qemuDomainObjPrivateAlloc(v
 return priv;
 
 error:
+virFdSetFree(priv-fdset);
 VIR_FREE(priv);
 return NULL;
 }
@@ -252,6 +256,7 @@ static void qemuDomainObjPrivateFree(voi
 qemuAgentClose(priv-agent);
 }
 VIR_FREE(priv-cleanupCallbacks);
+virFdSetFree(priv-fdset);
 VIR_FREE(priv);
 }
 
@@ -326,9 +331,14 @@ static int qemuDomainObjPrivateXMLFormat
 if (priv-fakeReboot)
 virBufferAsprintf(buf,   fakereboot/\n);
 
+virBufferAdjustIndent(buf, 2);
+virFdSetFormatXML(priv-fdset, buf);
+virBufferAdjustIndent(buf, -2);
+
 return 0;
 }
 
+
 static int qemuDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt, void *data)
 {
 qemuDomainObjPrivatePtr priv = data;
@@ -471,6 +481,9 @@ static int qemuDomainObjPrivateXMLParse(
 
 priv-fakeReboot = virXPathBoolean(boolean(./fakereboot), ctxt) == 1;
 
+if (virFdSetParseXML(priv-fdset, ./fdset/entry, ctxt)  0)
+goto error;
+
 return 0;
 
 error:
Index: libvirt/src/qemu/qemu_domain.h
===
--- libvirt.orig/src/qemu/qemu_domain.h
+++ libvirt/src/qemu/qemu_domain.h
@@ -32,6 +32,7 @@
 # include qemu_conf.h
 # include qemu_capabilities.h
 # include virchrdev.h
+# include virfdset.h
 
 # define QEMU_EXPECTED_VIRT_TYPES  \
 ((1  VIR_DOMAIN_VIRT_QEMU) | \
@@ -160,6 +161,8 @@ struct _qemuDomainObjPrivate {
 qemuDomainCleanupCallback *cleanupCallbacks;
 size_t ncleanupCallbacks;
 size_t ncleanupCallbacks_max;
+
+virFdSetPtr fdset;
 };
 
 struct qemuDomainWatchdogEvent
Index: libvirt/src/qemu/qemu_process.c
===
--- libvirt.orig/src/qemu/qemu_process.c
+++ libvirt/src/qemu/qemu_process.c
@@ -4337,6 +4337,8 @@ void qemuProcessStop(virQEMUDriverPtr dr
 priv-monConfig = NULL;
 }
 
+virFdsetReset(priv-fdset);
+
 /* shut it off for sure */
 ignore_value(qemuProcessKill(driver, vm, VIR_QEMU_PROCESS_KILL_FORCE|
  VIR_QEMU_PROCESS_KILL_NOCHECK));

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

Re: [libvirt] [PATCH] hypervisor: Restore pm initialization

2013-02-11 Thread Osier Yang 

On 2013年02月12日 00:03, John Ferlan wrote:

Adjustment for 'c059cdeaf' due to older compiler complaint about pm
not being initialized even though the j7 == 0 does the trick.
---
  src/xen/xen_hypervisor.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/xen/xen_hypervisor.c b/src/xen/xen_hypervisor.c
index 767fc0c..9b7dd2e 100644
--- a/src/xen/xen_hypervisor.c
+++ b/src/xen/xen_hypervisor.c
@@ -1773,7 +1773,7 @@ virXen_setvcpumap(int handle,
  ret = -1;
  } else {
  cpumap_t xen_cpumap; /* limited to 64 CPUs in old hypervisors */
-uint64_t *pm;
+uint64_t *pm =xen_cpumap;
  int j;

  if ((maplen  (int)sizeof(cpumap_t)) || (sizeof(cpumap_t)  7))


ACK, I got the failure too.

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

Re: [libvirt] [PATCH] hypervisor: Restore pm initialization

2013-02-11 Thread Osier Yang 

On 2013年02月12日 00:27, Osier Yang wrote:

On 2013年02月12日 00:03, John Ferlan wrote:

Adjustment for 'c059cdeaf' due to older compiler complaint about pm
not being initialized even though the j7 == 0 does the trick.
---
src/xen/xen_hypervisor.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/xen/xen_hypervisor.c b/src/xen/xen_hypervisor.c
index 767fc0c..9b7dd2e 100644
--- a/src/xen/xen_hypervisor.c
+++ b/src/xen/xen_hypervisor.c
@@ -1773,7 +1773,7 @@ virXen_setvcpumap(int handle,
ret = -1;
} else {
cpumap_t xen_cpumap; /* limited to 64 CPUs in old hypervisors */
- uint64_t *pm;
+ uint64_t *pm =xen_cpumap;
int j;

if ((maplen (int)sizeof(cpumap_t)) || (sizeof(cpumap_t) 7))


ACK, I got the failure too.


Pushed this as I'm compling.

Osier

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

[libvirt] [PATCH] Release VM lock before acquiring virDomainObjListPtr lock

2013-02-11 Thread Daniel P. Berrange 
From: Daniel P. Berrange berra...@redhat.com

When removing a VM from the virDomainObjListPtr, we must not
be holding the VM lock while acquiring the list lock. Re-order
code to ensure that we can release the VM lock early.
---
 src/conf/domain_conf.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 5e16ddf..d92e54a 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -2115,11 +2115,10 @@ void virDomainObjListRemove(virDomainObjListPtr doms,
 {
 char uuidstr[VIR_UUID_STRING_BUFLEN];
 
-virObjectLock(doms);
 virUUIDFormat(dom-def-uuid, uuidstr);
-
 virObjectUnlock(dom);
 
+virObjectLock(doms);
 virHashRemoveEntry(doms-objs, uuidstr);
 virObjectUnlock(doms);
 }
-- 
1.8.1

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

Re: [libvirt] [PATCH] Release VM lock before acquiring virDomainObjListPtr lock

2013-02-11 Thread Osier Yang 

On 2013年02月12日 00:46, Daniel P. Berrange wrote:

From: Daniel P. Berrangeberra...@redhat.com

When removing a VM from the virDomainObjListPtr, we must not
be holding the VM lock while acquiring the list lock. Re-order
code to ensure that we can release the VM lock early.
---
  src/conf/domain_conf.c | 3 +--
  1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 5e16ddf..d92e54a 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -2115,11 +2115,10 @@ void virDomainObjListRemove(virDomainObjListPtr doms,
  {
  char uuidstr[VIR_UUID_STRING_BUFLEN];

-virObjectLock(doms);
  virUUIDFormat(dom-def-uuid, uuidstr);
-
  virObjectUnlock(dom);

+virObjectLock(doms);
  virHashRemoveEntry(doms-objs, uuidstr);
  virObjectUnlock(doms);
  }


ACK

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

Re: [libvirt] [PATCH v2] Remove qemuDriverLock from almost everywhere

2013-02-11 Thread Daniel P. Berrange 
On Mon, Feb 11, 2013 at 04:47:29PM +, Daniel P. Berrange wrote:
 From: Daniel P. Berrange berra...@redhat.com
 
 With the majority of fields in the virQEMUDriverPtr struct
 now immutable or self-locking, there is no need for practically
 any methods to be using the QEMU driver lock. Only a handful
 of helper APIs in qemu_conf.c now need it

BTW, example of the performance improvements this brings

With the 0.10.2 release of libvirt, running 4 threads in
parallel, each starting + stopping 50 VMs, takes 2 mins 11
seconds.

This is on a 2 CPU machine. I'd expect the win to be even
greater on a machine with more CPUs.


In terms of testing, I am using a torture test which runs
multiple threads, each creating+destroying VMs in parallel.
I've successfully run this for over 5000 VM create+delete
pairs so far, without any issues (it has previously detected
a few crashes and deadlocks during my earlier testing of
previous versions of this patch).

Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

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

Re: [libvirt] [PATCH v2] Remove qemuDriverLock from almost everywhere

2013-02-11 Thread Daniel P. Berrange 
On Mon, Feb 11, 2013 at 04:59:24PM +, Daniel P. Berrange wrote:
 On Mon, Feb 11, 2013 at 04:47:29PM +, Daniel P. Berrange wrote:
  From: Daniel P. Berrange berra...@redhat.com
  
  With the majority of fields in the virQEMUDriverPtr struct
  now immutable or self-locking, there is no need for practically
  any methods to be using the QEMU driver lock. Only a handful
  of helper APIs in qemu_conf.c now need it
 
 BTW, example of the performance improvements this brings
 
 With the 0.10.2 release of libvirt, running 4 threads in
 parallel, each starting + stopping 50 VMs, takes 2 mins 11
 seconds.
 

Opps, somehow lost the middle paragraph

With this patch applied to GIT, the same test takes only
40 seconds.

 This is on a 2 CPU machine. I'd expect the win to be even
 greater on a machine with more CPUs.
 
 
 In terms of testing, I am using a torture test which runs
 multiple threads, each creating+destroying VMs in parallel.
 I've successfully run this for over 5000 VM create+delete
 pairs so far, without any issues (it has previously detected
 a few crashes and deadlocks during my earlier testing of
 previous versions of this patch).


Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

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

Re: [libvirt] [PATCH 11/11] hypervisor: Revisit Coverity issues regarding cpumap

2013-02-11 Thread Eric Blake 
On 01/30/2013 11:51 AM, John Ferlan wrote:
 Turns out the issue regarding ptr_arith and sign_exension weren't false
 positives. When shifting an 'unsigned char' as a target, it gets promoted
 to an 'int'; however, that 'int' cannot be shifted 32 bits which was how
 the algorithm was written. For the ptr_arith rather than index into the
 cpumap, change the to address as necessary and assign directly.
 ---
  src/xen/xen_hypervisor.c | 10 +-

  for (j = 0; j  maplen; j++) {
 -/* coverity[ptr_arith] */
 -/* coverity[sign_extension] */
 -*(pm + (j / 8)) |= cpumap[j]  (8 * (j  7));
 +if ((j  7) == 0)
 +pm = (uint64_t *)((uint64_t)xen_cpumap + (j  ~0x7UL));

I'm not happy with how this turned out.  We are turning an address into
a pointer but not via intptr_t (probably warnings on mingw); then doing
math on that pointer, then turning the result back into a uint64_t
pointer.  It looks like we are risking alignment errors.

 +*pm |= (uint64_t)cpumap[j]  (8 * (j  7));

This part looks better, although I can see why you had to push a
followup to silence a false negative compiler warning about pm
potentially being used uninitialized.

I still think we can do a better job; the whole goal of this code is to
write an endian-specific ordering of a multiple of 8 bytes; I can't help
but wonder if my concurrent work on a new virendian.h header would be a
nicer-looking solution here.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



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

Re: [libvirt] [PATCHv2 3/5] S390: QEMU driver support for CCW addresses

2013-02-11 Thread Viktor Mihajlovski 

On 02/11/2013 04:40 PM, Daniel P. Berrange wrote:


I'm not a fan of this approach. The capabilities data is reflecting
what the QEMU binary is capable of supporting, *regardless* of what
guest config is chosen.


not convinced this is the best possible definition ... we would
have seen more issues with it if the machine type would really have
had a meaning on other architectures, e.g., if -M isapc would not
allow to instantiate PCI devices. But since...

The decision about whether to use S390 or CCW based on the machine
type value, should be made by the internal method at the point when
it actually assigns addresses, not when we create the capabilities
initially.


...I can certainly use the vm definition to figure out which address
type to choose that should work. Let me run a few tests to prove that 
and I will come back with a V3 for 3-5/5 in the next couple of days.


--

Mit freundlichen Grüßen/Kind Regards
   Viktor Mihajlovski

IBM Deutschland Research  Development GmbH
Vorsitzender des Aufsichtsrats: Martin Jetter
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

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

Re: [libvirt] [PATCHv2 3/5] S390: QEMU driver support for CCW addresses

2013-02-11 Thread Daniel P. Berrange 

To clarify my earlier reply - my point is that in the following
method:

On Fri, Feb 08, 2013 at 06:32:22PM +0100, Viktor Mihajlovski wrote:
 +/*
 + * Three steps populating CCW devnos
 + * 1. Allocate empty address set
 + * 2. Gather addresses with explicit devno
 + * 3. Assign defaults to the rest
 + */
 +static int
 +qemuDomainAssignS390Addresses(virDomainDefPtr def,
 +  virQEMUCapsPtr qemuCaps,
 +  virDomainObjPtr obj)
 +{
 +int ret = -1;
 +qemuDomainCCWAddressSetPtr addrs = NULL;
 +qemuDomainObjPrivatePtr priv = NULL;
 +
 +if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_CCW)) {

change this to

   if (STREQ(def-os.machine), virtio-s390-ccw) {
   if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_CCW)) {
  virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
 _(This QEMU does not support CCW addressing));
   }

  ... assign CCW based addresses ...
   }

 +} else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_S390)) {
 +/* deal with legacy virtio-s390 */
  qemuDomainPrimeS390VirtioDevices(
  def, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390);
 +}
 +

This avoids need to pass machine type into the capabilities APIs at all.

Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

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

Re: [libvirt] [PATCH 3/4] qemu: Don't remove hash entry of other domains

2013-02-11 Thread Daniel P. Berrange 
On Mon, Feb 11, 2013 at 07:37:14PM +0800, Osier Yang wrote:
 On 2013年02月11日 18:59, Daniel P. Berrange wrote:
 On Fri, Feb 08, 2013 at 09:08:01PM +0800, Osier Yang wrote:
 qemuProcessStart invokes qemuProcessStop when fails, to avoid
 removing hash entry which belongs to other domain(s), this introduces
 a new virBitmapPtr argument for qemuProcessStop. And qemuProcessStart
 sets the bit for the disk only if it's successfully added into the
 hash table. Thus if the argument is provided for qemuProcessStop, it
 can't remove the hash entry belongs to other domain(s).
 
 I find this approach rather dubious - IMHO it is a sign that you're
 not recording enough information in the shared disk hash. eg perhaps
 the hash ought to be recording the UUID of each VM that is holding
 a reference. That way you're protected from qemuProcessStop() trying
 todo something wrong.
 
 
 I'm doubting about if it's really deserved to maintain a bunch of
 arrays in the hash entry. As we only need the recording for
 qemuProcessStart, it's much simpler to only use a bitmap to record
 the added disks for a VM in qemuProcessStart from my p.o.v.

IMHO it is a more robust design to record which VM is owning the
disk, since it prevents us introducing errors elsewhere in the
code which can lead to the same kind of problem later.


Incidentally, how does the shared disks hash get populated when
libvirtd starts up  reconnects to existing running VMs ? AFAICT,
nothing is being done in that codepath.

Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

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

Re: [libvirt] [PATCHv2 3/5] S390: QEMU driver support for CCW addresses

2013-02-11 Thread Viktor Mihajlovski 

On 02/11/2013 06:41 PM, Daniel P. Berrange wrote:


To clarify my earlier reply - my point is that in the following
method:


Thanks Daniel ... no clarification needed here. :-)

--

Mit freundlichen Grüßen/Kind Regards
   Viktor Mihajlovski

IBM Deutschland Research  Development GmbH
Vorsitzender des Aufsichtsrats: Martin Jetter
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

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

Re: [libvirt] [PATCH] Fix potential deadlock across fork() in QEMU driver

2013-02-11 Thread Eric Blake 
On 02/11/2013 09:12 AM, Daniel P. Berrange wrote:
 From: Daniel P. Berrange berra...@redhat.com
 
 The hook scripts used by virCommand must be careful wrt
 accessing any mutexes that may have been held by other
 threads in the parent process. With the recent refactorigng

s/refactorigng/refactoring/

 there are 2 potential flaws lurking, which will become real
 deadlock bugs once the global QEMU driver lock is removed.
 
 Remove use of the QEMU driver lock from the hook function
 by passing in the 'virQEMUDriverConfigPtr' instance directly.
 
 Add functions to the virSecurityManager to be invoked before
 and after fork, to ensure the mutex is held by the current
 thread. This allows it to be safely used in the hook script
 in the child procss.

s/procss/process/

 
 Signed-off-by: Daniel P. Berrange berra...@redhat.com
 ---
  src/libvirt_private.syms|  2 ++
  src/qemu/qemu_process.c | 16 
  src/security/security_manager.c | 20 
  src/security/security_manager.h |  3 +++
  4 files changed, 37 insertions(+), 4 deletions(-)
 

ACK.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



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

Re: [libvirt] [PATCH] Fix potential deadlock across fork() in QEMU driver

2013-02-11 Thread Osier Yang 

On 2013年02月12日 00:12, Daniel P. Berrange wrote:

From: Daniel P. Berrangeberra...@redhat.com

The hook scripts used by virCommand must be careful wrt
accessing any mutexes that may have been held by other
threads in the parent process. With the recent refactorigng


s/refactorigng/refactoring/,


there are 2 potential flaws lurking, which will become real
deadlock bugs once the global QEMU driver lock is removed.

Remove use of the QEMU driver lock from the hook function
by passing in the 'virQEMUDriverConfigPtr' instance directly.

Add functions to the virSecurityManager to be invoked before
and after fork, to ensure the mutex is held by the current
thread. This allows it to be safely used in the hook script
in the child procss.


s/procss/process/,



Signed-off-by: Daniel P. Berrangeberra...@redhat.com
---
  src/libvirt_private.syms|  2 ++
  src/qemu/qemu_process.c | 16 
  src/security/security_manager.c | 20 
  src/security/security_manager.h |  3 +++
  4 files changed, 37 insertions(+), 4 deletions(-)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index cb81497..5f19269 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1054,6 +1054,8 @@ virSecurityManagerGetProcessLabel;
  virSecurityManagerNew;
  virSecurityManagerNewDAC;
  virSecurityManagerNewStack;
+virSecurityManagerPostFork;
+virSecurityManagerPreFork;
  virSecurityManagerReleaseLabel;
  virSecurityManagerReserveLabel;
  virSecurityManagerRestoreAllLabel;
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 9759332..12e3544 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -2773,6 +2773,7 @@ struct qemuProcessHookData {
  virDomainObjPtr vm;
  virQEMUDriverPtr driver;
  virBitmapPtr nodemask;
+virQEMUDriverConfigPtr cfg;
  };

  static int qemuProcessHook(void *data)
@@ -2780,7 +2781,11 @@ static int qemuProcessHook(void *data)
  struct qemuProcessHookData *h = data;
  int ret = -1;
  int fd;
-virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(h-driver);
+/* This method cannot use any mutexes, which are not
+ * protected across fork()
+ */
+


Useless blank line.


+virSecurityManagerPostFork(h-driver-securityManager);




  /* Some later calls want pid present */
  h-vm-pid = getpid();
@@ -2796,7 +2801,7 @@ static int qemuProcessHook(void *data)
  if (virSecurityManagerSetSocketLabel(h-driver-securityManager, 
h-vm-def)  0)
  goto cleanup;
  if (virDomainLockProcessStart(h-driver-lockManager,
-  cfg-uri,
+  h-cfg-uri,
h-vm,
/* QEMU is always paused initially */
true,
@@ -2805,7 +2810,7 @@ static int qemuProcessHook(void *data)
  if (virSecurityManagerClearSocketLabel(h-driver-securityManager, 
h-vm-def)  0)
  goto cleanup;

-if (qemuProcessLimits(cfg)  0)
+if (qemuProcessLimits(h-cfg)  0)
  goto cleanup;

  /* This must take place before exec(), so that all QEMU
@@ -2831,7 +2836,7 @@ static int qemuProcessHook(void *data)
  ret = 0;

  cleanup:
-virObjectUnref(cfg);
+virObjectUnref(h-cfg);
  VIR_DEBUG(Hook complete ret=%d, ret);
  return ret;
  }
@@ -3642,6 +3647,7 @@ int qemuProcessStart(virConnectPtr conn,
  hookData.conn = conn;
  hookData.vm = vm;
  hookData.driver = driver;
+hookData.cfg = virObjectRef(cfg);

  VIR_DEBUG(Beginning VM startup process);

@@ -3973,7 +3979,9 @@ int qemuProcessStart(virConnectPtr conn,
  virCommandDaemonize(cmd);
  virCommandRequireHandshake(cmd);

+virSecurityManagerPreFork(driver-securityManager);
  ret = virCommandRun(cmd, NULL);
+virSecurityManagerPostFork(driver-securityManager);

  /* wait for qemu process to show up */
  if (ret == 0) {
diff --git a/src/security/security_manager.c b/src/security/security_manager.c
index 6f8ddbf..50962ba 100644
--- a/src/security/security_manager.c
+++ b/src/security/security_manager.c
@@ -192,6 +192,26 @@ virSecurityManagerPtr virSecurityManagerNew(const char 
*name,
 requireConfined);
  }

+
+/*
+ * Must be called before fork()'ing to ensure mutex state
+ * is sane for the child to use
+ */
+void virSecurityManagerPreFork(virSecurityManagerPtr mgr)
+{
+virObjectLock(mgr);
+}
+
+
+/*
+ * Must be called after fork()'ing in both parent and child
+ * to ensure mutex state is sane for the child to use
+ */
+void virSecurityManagerPostFork(virSecurityManagerPtr mgr)
+{
+virObjectUnlock(mgr);
+}
+
  void *virSecurityManagerGetPrivateData(virSecurityManagerPtr mgr)
  {
  return mgr-privateData;
diff --git a/src/security/security_manager.h b/src/security/security_manager.h
index 4d4dc73..8e8accf 100644
--- a/src/security/security_manager.h
+++

Re: [libvirt] [PATCH 3/4] qemu: Don't remove hash entry of other domains

2013-02-11 Thread Osier Yang 

On 2013年02月12日 01:44, Daniel P. Berrange wrote:

On Mon, Feb 11, 2013 at 07:37:14PM +0800, Osier Yang wrote:

On 2013年02月11日 18:59, Daniel P. Berrange wrote:

On Fri, Feb 08, 2013 at 09:08:01PM +0800, Osier Yang wrote:

qemuProcessStart invokes qemuProcessStop when fails, to avoid
removing hash entry which belongs to other domain(s), this introduces
a new virBitmapPtr argument for qemuProcessStop. And qemuProcessStart
sets the bit for the disk only if it's successfully added into the
hash table. Thus if the argument is provided for qemuProcessStop, it
can't remove the hash entry belongs to other domain(s).


I find this approach rather dubious - IMHO it is a sign that you're
not recording enough information in the shared disk hash. eg perhaps
the hash ought to be recording the UUID of each VM that is holding
a reference. That way you're protected from qemuProcessStop() trying
todo something wrong.



I'm doubting about if it's really deserved to maintain a bunch of
arrays in the hash entry. As we only need the recording for
qemuProcessStart, it's much simpler to only use a bitmap to record
the added disks for a VM in qemuProcessStart from my p.o.v.


IMHO it is a more robust design to record which VM is owning the
disk, since it prevents us introducing errors elsewhere in the
code which can lead to the same kind of problem later.



Okay, I have no more reason to not go this way.



Incidentally, how does the shared disks hash get populated when
libvirtd starts up  reconnects to existing running VMs ? AFAICT,
nothing is being done in that codepath.


Indeed, will add.



Daniel


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

Re: [libvirt] [PATCH] Check if classes are derived from object

2013-02-11 Thread Guido Günther 
On Mon, Feb 11, 2013 at 04:24:15PM +, Daniel P. Berrange wrote:
 On Mon, Feb 11, 2013 at 05:20:31PM +0100, Guido Günther wrote:
  This makes sure we don't regress to old style classes
  ---
  Just a minor addition that came up while verifying if the corresponding
  Debian bug is fixed.
  
   python/sanitytest.py |   27 ---
   1 file changed, 16 insertions(+), 11 deletions(-)
  
  diff --git a/python/sanitytest.py b/python/sanitytest.py
  index 047450b..ace6792 100644
  --- a/python/sanitytest.py
  +++ b/python/sanitytest.py
  @@ -7,17 +7,22 @@ globals = dir(libvirt)
   # Sanity test that the generator hasn't gone wrong
   
   # Look for core classes
  -assert(virConnect in globals)
  -assert(virDomain in globals)
  -assert(virDomainSnapshot in globals)
  -assert(virInterface in globals)
  -assert(virNWFilter in globals)
  -assert(virNodeDevice in globals)
  -assert(virNetwork in globals)
  -assert(virSecret in globals)
  -assert(virStoragePool in globals)
  -assert(virStorageVol in globals)
  -assert(virStream in globals)
  +for clsname in [virConnect,
  +virDomain,
  +virDomainSnapshot,
  +virInterface,
  +virNWFilter,
  +virNodeDevice,
  +virNetwork,
  +virSecret,
  +virStoragePool,
  +virStorageVol,
  +virStream,
  +]:
  +assert(clsname in globals)
  +assert(object in getattr(libvirt, clsname).__bases__)
  +
  +# Constants
   assert(VIR_CONNECT_RO in globals)
 
 ACK, good idea.
Pushed. Thanks,
 -- Guido

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

[libvirt] [PATCH] Avoid cast to unit64_t on 32bit platform

2013-02-11 Thread Guido Günther 
Fixes compilation on 32bit platforms:

xen/xen_hypervisor.c: In function 'virXen_setvcpumap':
xen/xen_hypervisor.c:1785:35: error: cast from pointer to integer of different 
size [-Werror=pointer-to-int-cast]
xen/xen_hypervisor.c:1785:22: error: cast to pointer from integer of different 
size [-Werror=int-to-pointer-cast]
cc1: all warnings being treated as errors
---
 src/xen/xen_hypervisor.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/xen/xen_hypervisor.c b/src/xen/xen_hypervisor.c
index 9b7dd2e..e3de0b2 100644
--- a/src/xen/xen_hypervisor.c
+++ b/src/xen/xen_hypervisor.c
@@ -1782,7 +1782,7 @@ virXen_setvcpumap(int handle,
 memset(xen_cpumap, 0, sizeof(cpumap_t));
 for (j = 0; j  maplen; j++) {
 if ((j  7) == 0)
-pm = (uint64_t *)((uint64_t)xen_cpumap + (j  ~0x7UL));
+pm = (uint64_t *)((intptr_t)xen_cpumap + (j  ~0x7UL));
 *pm |= (uint64_t)cpumap[j]  (8 * (j  7));
 }
 
-- 
1.7.10.4

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

Re: [libvirt] [PATCH] Avoid cast to unit64_t on 32bit platform

2013-02-11 Thread Eric Blake 
On 02/11/2013 12:40 PM, Guido Günther wrote:
 Fixes compilation on 32bit platforms:
 
 xen/xen_hypervisor.c: In function 'virXen_setvcpumap':
 xen/xen_hypervisor.c:1785:35: error: cast from pointer to integer of 
 different size [-Werror=pointer-to-int-cast]
 xen/xen_hypervisor.c:1785:22: error: cast to pointer from integer of 
 different size [-Werror=int-to-pointer-cast]
 cc1: all warnings being treated as errors
 ---
  src/xen/xen_hypervisor.c |2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
 
 diff --git a/src/xen/xen_hypervisor.c b/src/xen/xen_hypervisor.c
 index 9b7dd2e..e3de0b2 100644
 --- a/src/xen/xen_hypervisor.c
 +++ b/src/xen/xen_hypervisor.c
 @@ -1782,7 +1782,7 @@ virXen_setvcpumap(int handle,
  memset(xen_cpumap, 0, sizeof(cpumap_t));
  for (j = 0; j  maplen; j++) {
  if ((j  7) == 0)
 -pm = (uint64_t *)((uint64_t)xen_cpumap + (j  ~0x7UL));
 +pm = (uint64_t *)((intptr_t)xen_cpumap + (j  ~0x7UL));

I was afraid of this.  :(

I would ask that we hold off on this patch, and instead use a PROPER
patch that quits violating aliasing constraints.  I'll hopefully have
one posted later today (if not, then we can push this to fix builds as
an interim measure, but it feels lousy already having 2, and this would
make 3, patches to clean up what was supposed to be a simple Coverity
cleanup original patch).

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



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

[libvirt] [PATCH] build: fix make check of remote_protocol-structs

2013-02-11 Thread Laine Stump 
Broken by incorrect formatting / spelling of remote_nonnull in commit
39758e7567b766f1df3948ea671d6ccb93daf7a9
---

Pushed under build breaker rule

 src/remote_protocol-structs | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs
index b8ca88b..7f5ff7a 100644
--- a/src/remote_protocol-structs
+++ b/src/remote_protocol-structs
@@ -1401,9 +1401,9 @@ struct remote_node_device_lookup_by_name_ret {
 remote_nonnull_node_device dev;
 };
 struct remote_node_device_lookup_scsi_host_by_wwn_args {
-remote_nonull_string wwnn;
-remote_nonull_string wwpn;
-unsigned int flags;
+remote_nonnull_string  wwnn;
+remote_nonnull_string  wwpn;
+u_int  flags;
 };
 struct remote_node_device_lookup_scsi_host_by_wwn_ret {
 remote_nonnull_node_device dev;
-- 
1.8.1

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

Re: [libvirt] Google Summer of Code 2013 ideas wiki open

2013-02-11 Thread Stefan Hajnoczi 
On Thu, Feb 7, 2013 at 4:19 PM, Stefan Hajnoczi stefa...@gmail.com wrote:
 I believe Google will announce GSoC again this year (there is
 no guarantee though) and I have created the wiki page so we can begin
 organizing project ideas that students can choose from.

Google Summer of Code 2013 has just been announced!

http://google-opensource.blogspot.de/2013/02/flip-bits-not-burgers-google-summer-of.html

Some project ideas have already been discussed on IRC or private
emails.  Please go ahead and put them on the project ideas wiki page:

http://wiki.qemu.org/Google_Summer_of_Code_2013

Stefan

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

[libvirt] [PATCH] regex: gnulib guarantees that we have regex support

2013-02-11 Thread Eric Blake 
No need to use HAVE_REGEX_H - our use of gnulib guarantees that
the header exists and works, regardless of platform.  Similarly,
we can unconditionally assume a compiling sys/wait.h (although
the mingw version of this header is not full-featured).

* src/storage/storage_backend.c: Drop useless conditional.
* tests/testutils.c: Likewise.
---

I noticed this while trying to clean up a VPATH test failure.

 src/storage/storage_backend.c |  6 ++
 tests/testutils.c | 18 +++---
 2 files changed, 5 insertions(+), 19 deletions(-)

diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
index cab72c6..bdf 100644
--- a/src/storage/storage_backend.c
+++ b/src/storage/storage_backend.c
@@ -1,7 +1,7 @@
 /*
  * storage_backend.c: internal storage driver backend contract
  *
- * Copyright (C) 2007-2012 Red Hat, Inc.
+ * Copyright (C) 2007-2013 Red Hat, Inc.
  * Copyright (C) 2007-2008 Daniel P. Berrange
  *
  * This library is free software; you can redistribute it and/or
@@ -25,9 +25,7 @@

 #include string.h
 #include stdio.h
-#if HAVE_REGEX_H
-# include regex.h
-#endif
+#include regex.h
 #include sys/types.h
 #include sys/wait.h
 #include unistd.h
diff --git a/tests/testutils.c b/tests/testutils.c
index 9c8f365..7b2ea51 100644
--- a/tests/testutils.c
+++ b/tests/testutils.c
@@ -1,7 +1,7 @@
 /*
  * testutils.c: basic test utils
  *
- * Copyright (C) 2005-2012 Red Hat, Inc.
+ * Copyright (C) 2005-2013 Red Hat, Inc.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Lesser General Public
@@ -27,12 +27,8 @@
 #include sys/time.h
 #include sys/types.h
 #include sys/stat.h
-#ifndef WIN32
-# include sys/wait.h
-#endif
-#ifdef HAVE_REGEX_H
-# include regex.h
-#endif
+#include sys/wait.h
+#include regex.h
 #include unistd.h
 #include string.h
 #include fcntl.h
@@ -735,7 +731,6 @@ cleanup:
 }


-#ifdef HAVE_REGEX_H
 int virtTestClearLineRegex(const char *pattern,
char *str)
 {
@@ -779,10 +774,3 @@ int virtTestClearLineRegex(const char *pattern,

 return 0;
 }
-#else
-int virtTestClearLineRegex(const char *pattern ATTRIBUTE_UNUSED,
-   char *str ATTRIBUTE_UNUSED)
-{
-return 0;
-}
-#endif
-- 
1.8.1.2

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

[libvirt] [PATCH] build: fix VPATH testsuite

2013-02-11 Thread Eric Blake 
'make check' has been failing on VPATH builds since commit
907a39e7.  The tests already had magic for munging path names,
but were munging to the wrong location, thus working only on
an in-tree build.

* tests/securityselinuxlabeltest.c (testSELinuxMungePath): Munge
to correct path.
---

Pushing under the build-breaker rule.

 tests/securityselinuxlabeltest.c |4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/securityselinuxlabeltest.c b/tests/securityselinuxlabeltest.c
index c0bb09b..7445ab6 100644
--- a/tests/securityselinuxlabeltest.c
+++ b/tests/securityselinuxlabeltest.c
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2011-2012 Red Hat, Inc.
+ * Copyright (C) 2011-2013 Red Hat, Inc.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Lesser General Public
@@ -61,7 +61,7 @@ testSELinuxMungePath(char **path)
 char *tmp;

 if (virAsprintf(tmp, %s/securityselinuxlabeldata%s,
-abs_srcdir, *path)  0) {
+abs_builddir, *path)  0) {
 virReportOOMError();
 return -1;
 }
-- 
1.7.1

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

[libvirt] [PATCHv3 0/3] S390: Finish support for native CCW bus

2013-02-11 Thread Viktor Mihajlovski 
Originally, QEMU did not implement a native I/O bus for s390.
The initial implementation had a machine type 's390-virtio'
featuring a fully paravirtualized I/O system with an artificial
bus type 'virtio-s390'.
This bus had a number of short-comings, like the need for a
non-standard device discovery mechanism, a limited number
of devices, limited hotplugging capabilites and the lack
of persistent device addresses.
To resolve these issues a new machine type 's390-ccw-virtio' 
has been recently added to QEMU which implementa the native
s390 CCW I/O bus.
Guests with arch='s390x' and machine='s390-ccw-virtio' can
now be defined with up to 262144 virtio devices.

This series adds the support for the s390-ccw-virtio
machine type and the CCW bus to libvirt. 

V2 Changes
 - use an attribute triple cssid, ssid, schid instead of devno
   to better represent the libvirt CCW address structure
 - rebase to current upstream, mainly address qemuCapsXXX rename

V3 Changes
 - skip already acked patches 1/5 and 2/5
 - revert machine-based capabilities
 - use machine definition from domain object

J.B. Joret (1):
  S390: Add hotplug support for s390 virtio devices

Viktor Mihajlovski (2):
  S390: QEMU driver support for CCW addresses
  S390: Testcases for virtio-ccw machines

 src/qemu/qemu_capabilities.c   |7 +-
 src/qemu/qemu_capabilities.h   |1 +
 src/qemu/qemu_command.c|  280 ++--
 src/qemu/qemu_command.h|6 +
 src/qemu/qemu_domain.c |1 +
 src/qemu/qemu_domain.h |3 +
 src/qemu/qemu_driver.c |6 +-
 src/qemu/qemu_hotplug.c|  154 +++
 src/qemu/qemu_hotplug.h|   14 +-
 src/qemu/qemu_process.c|3 +
 .../qemuxml2argv-console-virtio-ccw.args   |   10 +
 .../qemuxml2argv-console-virtio-ccw.xml|   27 ++
 .../qemuxml2argv-console-virtio-s390.args  |4 +-
 .../qemuxml2argv-console-virtio-s390.xml   |2 +-
 .../qemuxml2argv-disk-virtio-ccw-many.args |   12 +
 .../qemuxml2argv-disk-virtio-ccw-many.xml  |   40 +++
 .../qemuxml2argv-disk-virtio-ccw.args  |8 +
 .../qemuxml2argv-disk-virtio-ccw.xml   |   30 +++
 .../qemuxml2argv-net-virtio-ccw.args   |8 +
 .../qemuxml2argv-net-virtio-ccw.xml|   30 +++
 tests/qemuxml2argvtest.c   |   12 +-
 tests/testutilsqemu.c  |3 +-
 22 files changed, 578 insertions(+), 83 deletions(-)
 create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-console-virtio-ccw.args
 create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-console-virtio-ccw.xml
 create mode 100644 
tests/qemuxml2argvdata/qemuxml2argv-disk-virtio-ccw-many.args
 create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-virtio-ccw-many.xml
 create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-virtio-ccw.args
 create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-virtio-ccw.xml
 create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-net-virtio-ccw.args
 create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-net-virtio-ccw.xml

-- 
1.7.9.5

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

[libvirt] [PATCHv3 3/3] S390: Testcases for virtio-ccw machines

2013-02-11 Thread Viktor Mihajlovski 
This adds and corrects testcases for virtio devices on s390
guests.

Signed-off-by: Viktor Mihajlovski mihaj...@linux.vnet.ibm.com
---
V2 Changes
 - adapt the testcase XML files to use the new attributes
 - use different variations for the values: hex, decimal, leading zeroes...

V3 Changes
 - add virtio-s390 caps to all ccw tests to reflect reality

 .../qemuxml2argv-console-virtio-ccw.args   |   10 +
 .../qemuxml2argv-console-virtio-ccw.xml|   27 +
 .../qemuxml2argv-console-virtio-s390.args  |4 +-
 .../qemuxml2argv-console-virtio-s390.xml   |2 +-
 .../qemuxml2argv-disk-virtio-ccw-many.args |   12 ++
 .../qemuxml2argv-disk-virtio-ccw-many.xml  |   40 
 .../qemuxml2argv-disk-virtio-ccw.args  |8 
 .../qemuxml2argv-disk-virtio-ccw.xml   |   30 +++
 .../qemuxml2argv-net-virtio-ccw.args   |8 
 .../qemuxml2argv-net-virtio-ccw.xml|   30 +++
 tests/qemuxml2argvtest.c   |   12 +-
 tests/testutilsqemu.c  |3 +-
 12 files changed, 181 insertions(+), 5 deletions(-)
 create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-console-virtio-ccw.args
 create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-console-virtio-ccw.xml
 create mode 100644 
tests/qemuxml2argvdata/qemuxml2argv-disk-virtio-ccw-many.args
 create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-virtio-ccw-many.xml
 create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-virtio-ccw.args
 create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-virtio-ccw.xml
 create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-net-virtio-ccw.args
 create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-net-virtio-ccw.xml

diff --git a/tests/qemuxml2argvdata/qemuxml2argv-console-virtio-ccw.args 
b/tests/qemuxml2argvdata/qemuxml2argv-console-virtio-ccw.args
new file mode 100644
index 000..6660a30
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-console-virtio-ccw.args
@@ -0,0 +1,10 @@
+LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M \
+s390-ccw -m 214 -smp 1 -nographic -nodefconfig -nodefaults -chardev \
+socket,id=charmonitor,path=/tmp/test-monitor,server,nowait -mon \
+chardev=charmonitor,id=monitor,mode=readline -no-acpi \
+-device virtio-serial-ccw,id=virtio-serial0,devno=fe.0.0001 \
+-usb -drive file=/dev/HostVG/QEMUGuest1,if=none,id=drive-virtio-disk0 \
+-device 
virtio-blk-ccw,devno=fe.0.,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1
 \
+-chardev pty,id=charconsole0 \
+-device virtconsole,chardev=charconsole0,id=console0 \
+-device virtio-balloon-ccw,id=balloon0,devno=fe.0.000a
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-console-virtio-ccw.xml 
b/tests/qemuxml2argvdata/qemuxml2argv-console-virtio-ccw.xml
new file mode 100644
index 000..faaeeb8
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-console-virtio-ccw.xml
@@ -0,0 +1,27 @@
+domain type='qemu'
+  nameQEMUGuest1/name
+  uuidc7a5fdbd-edaf-9455-926a-d65c16db1809/uuid
+  memory219100/memory
+  currentMemory219100/currentMemory
+  os
+type arch='s390x' machine='s390-ccw'hvm/type
+  /os
+  clock offset='utc'/
+  on_poweroffdestroy/on_poweroff
+  on_rebootrestart/on_reboot
+  on_crashdestroy/on_crash
+  devices
+emulator/usr/bin/qemu/emulator
+disk type='block' device='disk'
+  source dev='/dev/HostVG/QEMUGuest1'/
+  target dev='hda' bus='virtio'/
+  boot order='1'/
+/disk
+console type='pty'
+  target type='virtio'/
+/console
+memballoon model='virtio'
+  address type='ccw' cssid='0xfe' ssid='0' schid='0x000a'/
+/memballoon
+  /devices
+/domain
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-console-virtio-s390.args 
b/tests/qemuxml2argvdata/qemuxml2argv-console-virtio-s390.args
index 3bd7817..bf7b180 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-console-virtio-s390.args
+++ b/tests/qemuxml2argvdata/qemuxml2argv-console-virtio-s390.args
@@ -2,8 +2,8 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test 
/usr/bin/qemu -S -M \
 s390-virtio -m 214 -smp 1 -nographic -nodefconfig -nodefaults -chardev \
 socket,id=charmonitor,path=/tmp/test-monitor,server,nowait -mon \
 chardev=charmonitor,id=monitor,mode=readline -no-acpi \
--boot c -device virtio-serial-s390,id=virtio-serial0 \
+-device virtio-serial-s390,id=virtio-serial0 \
 -usb -drive file=/dev/HostVG/QEMUGuest1,if=none,id=drive-virtio-disk0 \
--device virtio-blk-s390,drive=drive-virtio-disk0,id=virtio-disk0 \
+-device virtio-blk-s390,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1 \
 -chardev pty,id=charconsole0 \
 -device virtconsole,chardev=charconsole0,id=console0
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-console-virtio-s390.xml 
b/tests/qemuxml2argvdata/qemuxml2argv-console-virtio-s390.xml
index 5a4a9d4..221232d 100644
---

[libvirt] [PATCHv3 2/3] S390: Add hotplug support for s390 virtio devices

2013-02-11 Thread Viktor Mihajlovski 
From: J.B. Joret j...@linux.vnet.ibm.com

We didn't yet expose the virtio device attach and detach functionality
for s390 domains as the device hotplug was very limited with the old
virtio-s390 bus. With the CCW bus there's full hotplug support for
virtio devices in QEMU, so we are adding this to libvirt too.

Since the virtio hotplug isn't limited to PCI anymore, we change the
function names from xxxPCIyyy to xxxVirtioyyy, where we handle all
three virtio bus types.

Signed-off-by: J.B. Joret j...@linux.vnet.ibm.com
Signed-off-by: Viktor Mihajlovski mihaj...@linux.vnet.ibm.com
---
V2 Changes
 - rebase, mainly addressing the rename of qemuCapsXXX to virQEMUCapsXXX 

V3 Changes
 - check whether machine type is s390-ccw

 src/qemu/qemu_driver.c  |6 +-
 src/qemu/qemu_hotplug.c |  154 ---
 src/qemu/qemu_hotplug.h |   14 ++---
 3 files changed, 117 insertions(+), 57 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index a4fc0c9..dbc6fce 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -5859,7 +5859,7 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn,
 ret = qemuDomainAttachUsbMassstorageDevice(conn, driver, vm,
disk);
 } else if (disk-bus == VIR_DOMAIN_DISK_BUS_VIRTIO) {
-ret = qemuDomainAttachPciDiskDevice(conn, driver, vm, disk);
+ret = qemuDomainAttachVirtioDiskDevice(conn, driver, vm, disk);
 } else if (disk-bus == VIR_DOMAIN_DISK_BUS_SCSI) {
 ret = qemuDomainAttachSCSIDisk(conn, driver, vm, disk);
 } else {
@@ -5992,7 +5992,7 @@ qemuDomainDetachDeviceDiskLive(virQEMUDriverPtr driver,
 case VIR_DOMAIN_DISK_DEVICE_DISK:
 case VIR_DOMAIN_DISK_DEVICE_LUN:
 if (disk-bus == VIR_DOMAIN_DISK_BUS_VIRTIO)
-ret = qemuDomainDetachPciDiskDevice(driver, vm, dev);
+ret = qemuDomainDetachVirtioDiskDevice(driver, vm, dev);
 else if (disk-bus == VIR_DOMAIN_DISK_BUS_SCSI ||
  disk-bus == VIR_DOMAIN_DISK_BUS_USB)
 ret = qemuDomainDetachDiskDevice(driver, vm, dev);
@@ -6003,7 +6003,7 @@ qemuDomainDetachDeviceDiskLive(virQEMUDriverPtr driver,
 default:
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_(disk device type '%s' cannot be detached),
-   virDomainDiskDeviceTypeToString(disk-type));
+   virDomainDiskDeviceTypeToString(disk-device));
 break;
 }
 
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 0c28a6a..d09a5fb 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -224,11 +224,10 @@ cleanup:
 return ret;
 }
 
-
-int qemuDomainAttachPciDiskDevice(virConnectPtr conn,
-  virQEMUDriverPtr driver,
-  virDomainObjPtr vm,
-  virDomainDiskDefPtr disk)
+int qemuDomainAttachVirtioDiskDevice(virConnectPtr conn,
+ virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ virDomainDiskDefPtr disk)
 {
 int i, ret = -1;
 const char* type = virDomainDiskBusTypeToString(disk-bus);
@@ -238,6 +237,15 @@ int qemuDomainAttachPciDiskDevice(virConnectPtr conn,
 bool releaseaddr = false;
 virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
 
+if (!disk-info.type) {
+if (STREQLEN(vm-def-os.machine, s390-ccw, 8) 
+virQEMUCapsGet(priv-qemuCaps, QEMU_CAPS_VIRTIO_CCW))
+disk-info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW;
+else if (virQEMUCapsGet(priv-qemuCaps, QEMU_CAPS_VIRTIO_S390))
+disk-info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390;
+else disk-info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI;
+}
+
 for (i = 0 ; i  vm-def-ndisks ; i++) {
 if (STREQ(vm-def-disks[i]-dst, disk-dst)) {
 virReportError(VIR_ERR_OPERATION_FAILED,
@@ -258,8 +266,14 @@ int qemuDomainAttachPciDiskDevice(virConnectPtr conn,
 }
 
 if (virQEMUCapsGet(priv-qemuCaps, QEMU_CAPS_DEVICE)) {
-if (qemuDomainPCIAddressEnsureAddr(priv-pciaddrs, disk-info)  0)
-goto error;
+if (disk-info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) {
+if (qemuDomainCCWAddressAssign(disk-info, priv-ccwaddrs,
+   !disk-info.addr.ccw.assigned)  0)
+goto error;
+} else if (disk-info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) {
+if (qemuDomainPCIAddressEnsureAddr(priv-pciaddrs, disk-info)  
0)
+goto error;
+}
 releaseaddr = true;
 if (qemuAssignDeviceDiskAlias(vm-def, disk, priv-qemuCaps)  0)
 goto error;
@@ -294,16 +308,14 @@ int qemuDomainAttachPciDiskDevice(virConnectPtr conn,
 }

[libvirt] [PATCHv3 1/3] S390: QEMU driver support for CCW addresses

2013-02-11 Thread Viktor Mihajlovski 
This commit adds the QEMU driver support for CCW addresses. The
current QEMU only allows virtio devices to be attached to the
CCW bus. We named the new capability indicating that support
QEMU_CAPS_VIRTIO_CCW accordingly.

The fact that CCW devices can only be assigned to domains with a
machine type of s390-ccw-virtio requires a few extra checks for
machine type in qemu_command.c on top of querying 
QEMU_CAPS_VIRTIO_{CCW|S390}. 

The majority of the new functions deals with CCW address generation
and management.

Signed-off-by: Viktor Mihajlovski mihaj...@linux.vnet.ibm.com
---
V2 Changes
 - rebase, mainly addressing the rename of qemuCapsXXX to virQEMUCapsXXX 

V3 Changes
 - revert machine based capability detection
 - check the machine type in conjunction with s390-specific capability
   tests in qemu_command.c
 - remove useless paranoia check in qemu_command.c

 src/qemu/qemu_capabilities.c |7 +-
 src/qemu/qemu_capabilities.h |1 +
 src/qemu/qemu_command.c  |  280 +++---
 src/qemu/qemu_command.h  |6 +
 src/qemu/qemu_domain.c   |1 +
 src/qemu/qemu_domain.h   |3 +
 src/qemu/qemu_process.c  |3 +
 7 files changed, 280 insertions(+), 21 deletions(-)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 4efe052..cb413fd 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -205,6 +205,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
   usb-serial, /* 125 */
   usb-net,
   add-fd,
+  virtio-ccw,
 
 );
 
@@ -1339,6 +1340,7 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] = {
 { usb-hub, QEMU_CAPS_USB_HUB },
 { ich9-ahci, QEMU_CAPS_ICH9_AHCI },
 { virtio-blk-s390, QEMU_CAPS_VIRTIO_S390 },
+{ virtio-blk-ccw, QEMU_CAPS_VIRTIO_CCW },
 { sclpconsole, QEMU_CAPS_SCLP_S390 },
 { lsi53c895a, QEMU_CAPS_SCSI_LSI },
 { virtio-scsi-pci, QEMU_CAPS_VIRTIO_SCSI_PCI },
@@ -1356,7 +1358,6 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] = {
 { usb-net, QEMU_CAPS_DEVICE_USB_NET},
 };
 
-
 static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsVirtioBlk[] = {
 { multifunction, QEMU_CAPS_PCI_MULTIFUNCTION },
 { bootindex, QEMU_CAPS_BOOTINDEX },
@@ -1411,6 +1412,10 @@ static struct virQEMUCapsObjectTypeProps 
virQEMUCapsObjectProps[] = {
   ARRAY_CARDINALITY(virQEMUCapsObjectPropsVirtioBlk) },
 { virtio-net-pci, virQEMUCapsObjectPropsVirtioNet,
   ARRAY_CARDINALITY(virQEMUCapsObjectPropsVirtioNet) },
+{ virtio-blk-ccw, virQEMUCapsObjectPropsVirtioBlk,
+  ARRAY_CARDINALITY(virQEMUCapsObjectPropsVirtioBlk) },
+{ virtio-net-ccw, virQEMUCapsObjectPropsVirtioNet,
+  ARRAY_CARDINALITY(virQEMUCapsObjectPropsVirtioNet) },
 { virtio-blk-s390, virQEMUCapsObjectPropsVirtioBlk,
   ARRAY_CARDINALITY(virQEMUCapsObjectPropsVirtioBlk) },
 { virtio-net-s390, virQEMUCapsObjectPropsVirtioNet,
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index e69d558..4c68892 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -166,6 +166,7 @@ enum virQEMUCapsFlags {
 QEMU_CAPS_DEVICE_USB_SERIAL  = 125, /* -device usb-serial */
 QEMU_CAPS_DEVICE_USB_NET = 126, /* -device usb-net */
 QEMU_CAPS_ADD_FD = 127, /* -add-fd */
+QEMU_CAPS_VIRTIO_CCW = 128, /* -device virtio-*-ccw */
 
 QEMU_CAPS_LAST,   /* this must always be the last item */
 };
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 6c28123..af82f04 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -795,6 +795,100 @@ no_memory:
 return -1;
 }
 
+/* S390 ccw bus support */
+
+struct _qemuDomainCCWAddressSet {
+virHashTablePtr defined;
+virDomainDeviceCCWAddress next;
+};
+
+static char*
+qemuCCWAddressAsString(virDomainDeviceCCWAddressPtr addr)
+{
+char *addrstr = NULL;
+
+if (virAsprintf(addrstr, %x.%x.%04x,
+addr-cssid,
+addr-ssid,
+addr-schid)  0) {
+virReportOOMError();
+return NULL;
+}
+
+return addrstr;
+}
+
+static int
+qemuCCWAdressIncrement(virDomainDeviceCCWAddressPtr addr)
+{
+virDomainDeviceCCWAddress ccwaddr = *addr;
+
+/* We are not touching subchannel sets and channel subsystems */
+if (++ccwaddr.schid  VIR_DOMAIN_DEVICE_CCW_MAX_SCHID)
+return -1;
+
+*addr = ccwaddr;
+return 0;
+}
+
+static void
+qemuDomainCCWAddressSetFreeEntry(void *payload,
+ const void *name ATTRIBUTE_UNUSED)
+{
+VIR_FREE(payload);
+}
+
+int qemuDomainCCWAddressAssign(virDomainDeviceInfoPtr dev,
+   qemuDomainCCWAddressSetPtr addrs,
+   bool autoassign)
+{
+int ret = -1;
+char *addr = NULL;
+
+if (dev-type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW)
+

Re: [libvirt] [PATCHv2 2/5] S390: domain_conf support for CCW

2013-02-11 Thread Eric Blake 
On 02/11/2013 08:36 AM, Daniel P. Berrange wrote:
 On Fri, Feb 08, 2013 at 06:32:21PM +0100, Viktor Mihajlovski wrote:
 Add necessary handling code for the new s390 CCW address type to
 virDomainDeviceInfo. Further, introduce  memory management, XML
 parsing, output formatting and range validation for the new
 virDomainDeviceCCWAddress type.

 Signed-off-by: Viktor Mihajlovski mihaj...@linux.vnet.ibm.com
 ---
 V2 Changes
  - adapted virDomainDeviceCCWAddressParseXML to handle the new
set of CCW address attributes

  src/conf/domain_conf.c   |  107 
 +-
  src/conf/domain_conf.h   |   16 +++
  src/libvirt_private.syms |1 +
  3 files changed, 123 insertions(+), 1 deletion(-)
 
 ACK

I've gone ahead and pushed patches 1 and 2, since v3 of the series did
not repost them.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



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

Re: [libvirt] [PATCH 11/11] hypervisor: Revisit Coverity issues regarding cpumap

2013-02-11 Thread Eric Blake 
On 02/11/2013 10:24 AM, Eric Blake wrote:
 On 01/30/2013 11:51 AM, John Ferlan wrote:
 Turns out the issue regarding ptr_arith and sign_exension weren't false
 positives. When shifting an 'unsigned char' as a target, it gets promoted
 to an 'int'; however, that 'int' cannot be shifted 32 bits which was how
 the algorithm was written. For the ptr_arith rather than index into the
 cpumap, change the to address as necessary and assign directly.
 ---
  src/xen/xen_hypervisor.c | 10 +-
 
  for (j = 0; j  maplen; j++) {
 -/* coverity[ptr_arith] */
 -/* coverity[sign_extension] */
 -*(pm + (j / 8)) |= cpumap[j]  (8 * (j  7));
 +if ((j  7) == 0)
 +pm = (uint64_t *)((uint64_t)xen_cpumap + (j  ~0x7UL));
 
 I'm not happy with how this turned out.  We are turning an address into
 a pointer but not via intptr_t (probably warnings on mingw); then doing
 math on that pointer, then turning the result back into a uint64_t
 pointer.  It looks like we are risking alignment errors.
 
 +*pm |= (uint64_t)cpumap[j]  (8 * (j  7));

More research - I think this is highly over-engineered, which explains
why we got it so wrong.  I checked a full range of xen-devel, from RHEL
5 (3.0.3, about as old as we support out-of-the-box with libvirt.git) to
Fedora 18 (4.2.1); ALL of those versions had:

/usr/include/xen/dom0_ops.h: typedef uint64_t cpumap_t;

which means this type has (more or less) _always_ been exactly 64 bits;
the code that tries to allow for iterating through 16 bytes instead of 8
is over-engineered.  Really, _ALL_ we are doing is reading a
little-endian 64-bit number in from the user (possibly in unaligned
memory), and turning it into a host-endian cpumap_t to hand to xen.

I should have a patch soon.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



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

[libvirt] [PATCH 1/3] util: add virendian.h macros

2013-02-11 Thread Eric Blake 
We have several cases where we need to read endian-dependent
data regardless of host endianness; rather than open-coding
these call sites, it will be nicer to funnel things through
a macro.

The virendian.h file can be expanded to add writer functions,
and/or 16-bit access patterns, if needed.  Also, if we need
to turn things into a function to avoid multiple evaluations
of buf, that can be done later.  But for now, a macro worked.

* src/util/virendian.h: New file.
* src/Makefile.am (UTIL_SOURCES): Ship it.
* tests/virendiantest.c: New test.
* tests/Makefile.am (test_programs, virendiantest_SOURCES): Run
the test.
* .gitignore: Ignore built file.
---
 .gitignore|   1 +
 src/Makefile.am   |   1 +
 src/util/virendian.h  |  93 +
 tests/Makefile.am |   8 +++-
 tests/virendiantest.c | 102 ++
 5 files changed, 203 insertions(+), 2 deletions(-)
 create mode 100644 src/util/virendian.h
 create mode 100644 tests/virendiantest.c

diff --git a/.gitignore b/.gitignore
index 1670637..8afbf33 100644
--- a/.gitignore
+++ b/.gitignore
@@ -177,6 +177,7 @@
 /tests/virbitmaptest
 /tests/virbuftest
 /tests/virdrivermoduletest
+/tests/virendiantest
 /tests/virhashtest
 /tests/virkeyfiletest
 /tests/virlockspacetest
diff --git a/src/Makefile.am b/src/Makefile.am
index f6162df..d554aa1 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -67,6 +67,7 @@ UTIL_SOURCES =
\
util/virdbus.c util/virdbus.h   \
util/virdnsmasq.c util/virdnsmasq.h \
util/virebtables.c util/virebtables.h   \
+   util/virendian.h\
util/virerror.c util/virerror.h \
util/virevent.c util/virevent.h \
util/vireventpoll.c util/vireventpoll.h \
diff --git a/src/util/virendian.h b/src/util/virendian.h
new file mode 100644
index 000..eefe48c
--- /dev/null
+++ b/src/util/virendian.h
@@ -0,0 +1,93 @@
+/*
+ * virendian.h: aid for reading endian-specific data
+ *
+ * Copyright (C) 2013 Red Hat, Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library.  If not, see
+ * http://www.gnu.org/licenses/.
+ *
+ */
+
+#ifndef __VIR_ENDIAN_H__
+# define __VIR_ENDIAN_H__
+
+# include internal.h
+
+/* The interfaces in this file are provided as macros for speed.  */
+
+/**
+ * virReadBufInt64BE:
+ * @buf: byte to start reading at (can be 'char*' or 'unsigned char*');
+ *   evaluating buf must not have any side effects
+ *
+ * Read 8 bytes at BUF as a big-endian 64-bit number.  Caller is
+ * responsible to avoid reading beyond array bounds.
+ */
+# define virReadBufInt64BE(buf)  \
+(((uint64_t)(uint8_t)((buf)[0])  56) | \
+ ((uint64_t)(uint8_t)((buf)[1])  48) | \
+ ((uint64_t)(uint8_t)((buf)[2])  40) | \
+ ((uint64_t)(uint8_t)((buf)[3])  32) | \
+ ((uint64_t)(uint8_t)((buf)[4])  24) | \
+ ((uint64_t)(uint8_t)((buf)[5])  16) | \
+ ((uint64_t)(uint8_t)((buf)[6])  8) |  \
+ (uint64_t)(uint8_t)((buf)[7]))
+
+/**
+ * virReadBufInt64LE:
+ * @buf: byte to start reading at (can be 'char*' or 'unsigned char*');
+ *   evaluating buf must not have any side effects
+ *
+ * Read 8 bytes at BUF as a little-endian 64-bit number.  Caller is
+ * responsible to avoid reading beyond array bounds.
+ */
+# define virReadBufInt64LE(buf)  \
+((uint64_t)(uint8_t)((buf)[0]) | \
+ ((uint64_t)(uint8_t)((buf)[1])  8) |  \
+ ((uint64_t)(uint8_t)((buf)[2])  16) | \
+ ((uint64_t)(uint8_t)((buf)[3])  24) | \
+ ((uint64_t)(uint8_t)((buf)[4])  32) | \
+ ((uint64_t)(uint8_t)((buf)[5])  40) | \
+ ((uint64_t)(uint8_t)((buf)[6])  48) | \
+ ((uint64_t)(uint8_t)((buf)[7])  56))
+
+/**
+ * virReadBufInt32BE:
+ * @buf: byte to start reading at (can be 'char*' or 'unsigned char*');
+ *   evaluating buf must not have any side effects
+ *
+ * Read 4 bytes at BUF as a big-endian 32-bit number.  Caller is
+ * responsible to avoid reading beyond array bounds.
+ */
+# define

[libvirt] [PATCH 0/3] add virendian.h, and use it to kill xen cpumap issue

2013-02-11 Thread Eric Blake 
Not only does this solve the compiler warning on 32-bit machines,
but it completely gets rid of what looks like bogus pointer aliasing,
all while reducing the number of lines in xen_hypervisor.c.

Eric Blake (3):
  util: add virendian.h macros
  util: use new virendian.h macros
  xen: clean up the mess with cpumap

 .gitignore|   1 +
 src/Makefile.am   |   1 +
 src/cpu/cpu_x86.c |  24 --
 src/util/virendian.h  |  93 +++
 src/util/virstoragefile.c | 109 ++
 src/xen/xen_hypervisor.c  |  16 +++
 tests/Makefile.am |   8 +++-
 tests/virendiantest.c | 102 +++
 8 files changed, 239 insertions(+), 115 deletions(-)
 create mode 100644 src/util/virendian.h
 create mode 100644 tests/virendiantest.c

-- 
1.8.1.2

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

[libvirt] [PATCH 2/3] util: use new virendian.h macros

2013-02-11 Thread Eric Blake 
This makes code easier to read, by avoiding lines longer than
80 columns and removing the repetition from the callers.

* src/util/virstoragefile.c (qedGetHeaderUL, qedGetHeaderULL):
Delete in favor of more generic macros.
(qcow2GetBackingStoreFormat, qcowXGetBackingStore)
(qedGetBackingStore, virStorageFileMatchesVersion)
(virStorageFileGetMetadataInternal): Use new macros.
* src/cpu/cpu_x86.c (x86VendorLoad): Likewise.
---
 src/cpu/cpu_x86.c |  24 --
 src/util/virstoragefile.c | 109 ++
 2 files changed, 30 insertions(+), 103 deletions(-)

diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c
index c9a833a..c750afd 100644
--- a/src/cpu/cpu_x86.c
+++ b/src/cpu/cpu_x86.c
@@ -1,7 +1,7 @@
 /*
  * cpu_x86.c: CPU driver for CPUs with x86 compatible CPUID instruction
  *
- * Copyright (C) 2009-2011 Red Hat, Inc.
+ * Copyright (C) 2009-2011, 2013 Red Hat, Inc.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Lesser General Public
@@ -32,6 +32,7 @@
 #include cpu_map.h
 #include cpu_x86.h
 #include virbuffer.h
+#include virendian.h


 #define VIR_FROM_THIS VIR_FROM_CPU
@@ -548,22 +549,13 @@ x86VendorLoad(xmlXPathContextPtr ctxt,
 }

 vendor-cpuid.function = 0;
-vendor-cpuid.ebx = (string[0]) |
-(string[1]  8) |
-(string[2]  16) |
-(string[3]  24);
-vendor-cpuid.edx = (string[4]) |
-(string[5]  8) |
-(string[6]  16) |
-(string[7]  24);
-vendor-cpuid.ecx = (string[8]) |
-(string[9]  8) |
-(string[10]  16) |
-(string[11]  24);
-
-if (!map-vendors)
+vendor-cpuid.ebx = virReadBufInt32LE(string);
+vendor-cpuid.edx = virReadBufInt32LE(string + 4);
+vendor-cpuid.ecx = virReadBufInt32LE(string + 8);
+
+if (!map-vendors) {
 map-vendors = vendor;
-else {
+} else {
 vendor-next = map-vendors;
 map-vendors = vendor;
 }
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index 6e2d61e..dc28539 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -42,6 +42,7 @@
 #include c-ctype.h
 #include vircommand.h
 #include virhash.h
+#include virendian.h

 #define VIR_FROM_THIS VIR_FROM_STORAGE

@@ -255,16 +256,8 @@ qcow2GetBackingStoreFormat(int *format,
  */
 while (offset  (buf_size-8) 
offset  (extension_end-8)) {
-unsigned int magic =
-(buf[offset]  24) +
-(buf[offset+1]  16) +
-(buf[offset+2]  8) +
-(buf[offset+3]);
-unsigned int len =
-(buf[offset+4]  24) +
-(buf[offset+5]  16) +
-(buf[offset+6]  8) +
-(buf[offset+7]);
+unsigned int magic = virReadBufInt32BE(buf + offset);
+unsigned int len = virReadBufInt32BE(buf + offset + 4);

 offset += 8;

@@ -312,20 +305,10 @@ qcowXGetBackingStore(char **res,

 if (buf_size  QCOWX_HDR_BACKING_FILE_OFFSET+8+4)
 return BACKING_STORE_INVALID;
-offset = (((unsigned long long)buf[QCOWX_HDR_BACKING_FILE_OFFSET]  56)
-  | ((unsigned long long)buf[QCOWX_HDR_BACKING_FILE_OFFSET+1]  
48)
-  | ((unsigned long long)buf[QCOWX_HDR_BACKING_FILE_OFFSET+2]  
40)
-  | ((unsigned long long)buf[QCOWX_HDR_BACKING_FILE_OFFSET+3]  
32)
-  | ((unsigned long long)buf[QCOWX_HDR_BACKING_FILE_OFFSET+4]  
24)
-  | ((unsigned long long)buf[QCOWX_HDR_BACKING_FILE_OFFSET+5]  
16)
-  | ((unsigned long long)buf[QCOWX_HDR_BACKING_FILE_OFFSET+6]  8)
-  | buf[QCOWX_HDR_BACKING_FILE_OFFSET+7]); /* 
QCowHeader.backing_file_offset */
+offset = virReadBufInt64BE(buf + QCOWX_HDR_BACKING_FILE_OFFSET);
 if (offset  buf_size)
 return BACKING_STORE_INVALID;
-size = ((buf[QCOWX_HDR_BACKING_FILE_SIZE]  24)
-| (buf[QCOWX_HDR_BACKING_FILE_SIZE+1]  16)
-| (buf[QCOWX_HDR_BACKING_FILE_SIZE+2]  8)
-| buf[QCOWX_HDR_BACKING_FILE_SIZE+3]); /* 
QCowHeader.backing_file_size */
+size = virReadBufInt32BE(buf + QCOWX_HDR_BACKING_FILE_SIZE);
 if (size == 0) {
 if (format)
 *format = VIR_STORAGE_FILE_NONE;
@@ -468,28 +451,6 @@ cleanup:
 return ret;
 }

-static unsigned long
-qedGetHeaderUL(const unsigned char *loc)
-{
-return (((unsigned long)loc[3]  24) |
-((unsigned long)loc[2]  16) |
-((unsigned long)loc[1]  8) |
-((unsigned long)loc[0]  0));
-}
-
-static unsigned long long
-qedGetHeaderULL(const unsigned char *loc)
-{
-return (((unsigned long long)loc[7]  56) |
-((unsigned long long)loc[6]  48) |
-((unsigned long long)loc[5]  40) |
-((unsigned long long)loc[4]  32) |
-((unsigned long

[libvirt] [PATCH 3/3] xen: clean up the mess with cpumap

2013-02-11 Thread Eric Blake 
Commit 8b55992f added some Coverity comments to silence what was
a real bug in the code.  Since then, we've had a miserable run
of trying to fix the underlying problem (commits c059cde and
ba5193c), and still have a problem on 32-bit machines.

This fixes the problem for once and for all, by realizing that
on older xen, cpumap_t is identical to uint64_t, and using the
new virendian.h to do the transformation from the API (documented
to be little-endian) to the host structure.

* src/xen/xen_hypervisor.c (virXen_setvcpumap): Do the conversion
correctly.  Finally.
---
 src/xen/xen_hypervisor.c | 16 ++--
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/src/xen/xen_hypervisor.c b/src/xen/xen_hypervisor.c
index 9b7dd2e..d803972 100644
--- a/src/xen/xen_hypervisor.c
+++ b/src/xen/xen_hypervisor.c
@@ -80,6 +80,7 @@
 #include virfile.h
 #include virnodesuspend.h
 #include virtypedparam.h
+#include virendian.h

 #define VIR_FROM_THIS VIR_FROM_XEN

@@ -1773,18 +1774,13 @@ virXen_setvcpumap(int handle,
 ret = -1;
 } else {
 cpumap_t xen_cpumap; /* limited to 64 CPUs in old hypervisors */
-uint64_t *pm = xen_cpumap;
-int j;
+char buf[8] = ;

-if ((maplen  (int)sizeof(cpumap_t)) || (sizeof(cpumap_t)  7))
+if (maplen  sizeof(cpumap_t) || sizeof(cpumap_t) != sizeof(uint64_t))
 return -1;
-
-memset(xen_cpumap, 0, sizeof(cpumap_t));
-for (j = 0; j  maplen; j++) {
-if ((j  7) == 0)
-pm = (uint64_t *)((uint64_t)xen_cpumap + (j  ~0x7UL));
-*pm |= (uint64_t)cpumap[j]  (8 * (j  7));
-}
+/* Supply trailing 0s if user's input array was short */
+memcpy(buf, cpumap, maplen);
+xen_cpumap = virReadBufInt64LE(buf);

 if (hv_versions.hypervisor == 1) {
 xen_op_v1 op;
-- 
1.8.1.2

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

[libvirt] [PATCH] build: fix compilation of selinux on RHEL 5

2013-02-11 Thread Eric Blake 
On RHEL 5, I got:

security/security_selinux.c: In function 'getContext':
security/security_selinux.c:971: warning: unused parameter 'mgr' 
[-Wunused-parameter]

* src/security/security_selinux.c (getContext): Mark potentially
unused parameter.
---

Pushing under the build-breaker rule.

 src/security/security_selinux.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index 2a9333c..8173b20 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2008-2012 Red Hat, Inc.
+ * Copyright (C) 2008-2013 Red Hat, Inc.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Lesser General Public
@@ -968,7 +968,7 @@ virSecuritySELinuxFSetFilecon(int fd, char *tcon)

 /* Set fcon to the appropriate label for path and mode, or return -1.  */
 static int
-getContext(virSecurityManagerPtr mgr,
+getContext(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED,
const char *newpath, mode_t mode, security_context_t *fcon)
 {
 #if HAVE_SELINUX_LABEL_H
-- 
1.8.1.2

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

Re: [libvirt] [PATCH 1/3] util: add virendian.h macros

2013-02-11 Thread John Ferlan 
On 02/11/2013 07:07 PM, Eric Blake wrote:
 We have several cases where we need to read endian-dependent
 data regardless of host endianness; rather than open-coding
 these call sites, it will be nicer to funnel things through
 a macro.
 
 The virendian.h file can be expanded to add writer functions,
 and/or 16-bit access patterns, if needed.  Also, if we need
 to turn things into a function to avoid multiple evaluations
 of buf, that can be done later.  But for now, a macro worked.
 
 * src/util/virendian.h: New file.
 * src/Makefile.am (UTIL_SOURCES): Ship it.
 * tests/virendiantest.c: New test.
 * tests/Makefile.am (test_programs, virendiantest_SOURCES): Run
 the test.
 * .gitignore: Ignore built file.
 ---
  .gitignore|   1 +
  src/Makefile.am   |   1 +
  src/util/virendian.h  |  93 +
  tests/Makefile.am |   8 +++-
  tests/virendiantest.c | 102 
 ++
  5 files changed, 203 insertions(+), 2 deletions(-)
  create mode 100644 src/util/virendian.h
  create mode 100644 tests/virendiantest.c
 
 diff --git a/.gitignore b/.gitignore
 index 1670637..8afbf33 100644
 --- a/.gitignore
 +++ b/.gitignore
 @@ -177,6 +177,7 @@
  /tests/virbitmaptest
  /tests/virbuftest
  /tests/virdrivermoduletest
 +/tests/virendiantest
  /tests/virhashtest
  /tests/virkeyfiletest
  /tests/virlockspacetest
 diff --git a/src/Makefile.am b/src/Makefile.am
 index f6162df..d554aa1 100644
 --- a/src/Makefile.am
 +++ b/src/Makefile.am
 @@ -67,6 +67,7 @@ UTIL_SOURCES =  
 \
   util/virdbus.c util/virdbus.h   \
   util/virdnsmasq.c util/virdnsmasq.h \
   util/virebtables.c util/virebtables.h   \
 + util/virendian.h\
   util/virerror.c util/virerror.h \
   util/virevent.c util/virevent.h \
   util/vireventpoll.c util/vireventpoll.h \
 diff --git a/src/util/virendian.h b/src/util/virendian.h
 new file mode 100644
 index 000..eefe48c
 --- /dev/null
 +++ b/src/util/virendian.h
 @@ -0,0 +1,93 @@
 +/*
 + * virendian.h: aid for reading endian-specific data
 + *
 + * Copyright (C) 2013 Red Hat, Inc.
 + *
 + * This library is free software; you can redistribute it and/or
 + * modify it under the terms of the GNU Lesser General Public
 + * License as published by the Free Software Foundation; either
 + * version 2.1 of the License, or (at your option) any later version.
 + *
 + * This library is distributed in the hope that it will be useful,
 + * but WITHOUT ANY WARRANTY; without even the implied warranty of
 + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
 + * Lesser General Public License for more details.
 + *
 + * You should have received a copy of the GNU Lesser General Public
 + * License along with this library.  If not, see
 + * http://www.gnu.org/licenses/.
 + *
 + */
 +
 +#ifndef __VIR_ENDIAN_H__
 +# define __VIR_ENDIAN_H__
 +
 +# include internal.h
 +
 +/* The interfaces in this file are provided as macros for speed.  */
 +
 +/**
 + * virReadBufInt64BE:
 + * @buf: byte to start reading at (can be 'char*' or 'unsigned char*');
 + *   evaluating buf must not have any side effects
 + *
 + * Read 8 bytes at BUF as a big-endian 64-bit number.  Caller is
 + * responsible to avoid reading beyond array bounds.
 + */
 +# define virReadBufInt64BE(buf)  \
 +(((uint64_t)(uint8_t)((buf)[0])  56) | \
 + ((uint64_t)(uint8_t)((buf)[1])  48) | \
 + ((uint64_t)(uint8_t)((buf)[2])  40) | \
 + ((uint64_t)(uint8_t)((buf)[3])  32) | \
 + ((uint64_t)(uint8_t)((buf)[4])  24) | \
 + ((uint64_t)(uint8_t)((buf)[5])  16) | \
 + ((uint64_t)(uint8_t)((buf)[6])  8) |  \
 + (uint64_t)(uint8_t)((buf)[7]))
 +
 +/**
 + * virReadBufInt64LE:
 + * @buf: byte to start reading at (can be 'char*' or 'unsigned char*');
 + *   evaluating buf must not have any side effects
 + *
 + * Read 8 bytes at BUF as a little-endian 64-bit number.  Caller is
 + * responsible to avoid reading beyond array bounds.
 + */
 +# define virReadBufInt64LE(buf)  \
 +((uint64_t)(uint8_t)((buf)[0]) | \
 + ((uint64_t)(uint8_t)((buf)[1])  8) |  \
 + ((uint64_t)(uint8_t)((buf)[2])  16) | \
 + ((uint64_t)(uint8_t)((buf)[3])  24) | \
 + ((uint64_t)(uint8_t)((buf)[4])  32) | \
 + ((uint64_t)(uint8_t)((buf)[5])  40) | \
 + ((uint64_t)(uint8_t)((buf)[6])  48) | \
 + ((uint64_t)(uint8_t)((buf)[7])  56))
 +
 +/**
 + * virReadBufInt32BE:
 + * @buf: byte to start reading at (can be 'char*' or 'unsigned char*');
 + *   evaluating buf must not have any

Re: [libvirt] [PATCH 2/3] util: use new virendian.h macros

2013-02-11 Thread John Ferlan 
On 02/11/2013 07:07 PM, Eric Blake wrote:
 This makes code easier to read, by avoiding lines longer than
 80 columns and removing the repetition from the callers.
 
 * src/util/virstoragefile.c (qedGetHeaderUL, qedGetHeaderULL):
 Delete in favor of more generic macros.
 (qcow2GetBackingStoreFormat, qcowXGetBackingStore)
 (qedGetBackingStore, virStorageFileMatchesVersion)
 (virStorageFileGetMetadataInternal): Use new macros.
 * src/cpu/cpu_x86.c (x86VendorLoad): Likewise.
 ---
  src/cpu/cpu_x86.c |  24 --
  src/util/virstoragefile.c | 109 
 ++
  2 files changed, 30 insertions(+), 103 deletions(-)
 

ACK

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

Re: [libvirt] [PATCH 3/3] xen: clean up the mess with cpumap

2013-02-11 Thread John Ferlan 
On 02/11/2013 07:07 PM, Eric Blake wrote:
 Commit 8b55992f added some Coverity comments to silence what was
 a real bug in the code.  Since then, we've had a miserable run
 of trying to fix the underlying problem (commits c059cde and
 ba5193c), and still have a problem on 32-bit machines.
 
 This fixes the problem for once and for all, by realizing that
 on older xen, cpumap_t is identical to uint64_t, and using the
 new virendian.h to do the transformation from the API (documented
 to be little-endian) to the host structure.
 
 * src/xen/xen_hypervisor.c (virXen_setvcpumap): Do the conversion
 correctly.  Finally.
 ---
  src/xen/xen_hypervisor.c | 16 ++--
  1 file changed, 6 insertions(+), 10 deletions(-)
 

ACK

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