Re: [libvirt] [PATCH] virsh: Check wether found volume is member of the specified storage pool

[Ján Tomko]

s/wether/whether/ in the subject



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] virsh: Check wether found volume is member of the specified storage pool

[Peter Krempa]

On 05/30/14 16:28, Martin Kletzander wrote:
 On Fri, May 30, 2014 at 03:39:36PM +0200, Peter Krempa wrote:
 When looking up storage volumes virsh uses multiple lookup steps. Some
 of the steps don't require a pool name specified. This resulted into a
 possibility that a volume would be part of a different pool than the
 user specified:

 Let's have a /var/lib/libvirt/images/test.qcow image in the 'default'
 pool and a second pool 'emptypool':

 Currently we'd return:
  $ virsh vol-info --pool emptypool /var/lib/libvirt/images/test.qcow
  Name:   test.qcow
  Type:   file
  Capacity:   100.00 MiB
  Allocation: 212.00 KiB

 
 I believed that the --pool parameter for vol-info (and *some* others)
 was only a hint in case you had more volumes with the same name
 (specifying absolute path with a pool doesn't make any sense).  That
 would mean the BZ is notabug actually, but let's assume such users
 exist...
 
 After the fix:
 $ tools/virsh vol-info --pool emptypool /var/lib/libvirt/images/test.qcow
 error: Requested volume '/var/lib/libvirt/images/test.qcow' found in a
 different pool (default) than specified

 
 I'd say this is rather noisy.  How about changing it to ...'%s' not
 found in pool '%s'...  or is not in pool '%s'?
 
 ACK after release with or without the change mentioned above.

I went with your shorter wording and fixed the typo in the subject
pointed out by Jan. Pushed now.

Thanks.

Peter



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 v2] qemu: Properly label FDs when restoring domain with static label

[Martin Kletzander]

On Thu, May 29, 2014 at 10:42:37AM -0400, Shivaprasad G Bhat wrote:

The restore of a saved image file fails when the selinux context is static.
The libvirt has to set the conext of save image file handle to that of
the guest before handing off the FD to qemu.

Signed-off-by: Shivaprasad G Bhat shivaprasadb...@gmail.com
---
src/qemu/qemu_process.c |4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 124fe28..47d1f7d 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -4052,14 +4052,14 @@ int qemuProcessStart(virConnectPtr conn,
 */
struct stat stdin_sb;

-VIR_DEBUG(setting security label on pipe used for migration);
+VIR_DEBUG(setting security label on fd used for migration or 
restore);

if (fstat(stdin_fd, stdin_sb)  0) {
virReportSystemError(errno,
 _(cannot stat fd %d), stdin_fd);
goto cleanup;
}
-if (S_ISFIFO(stdin_sb.st_mode) 
+if ((S_ISFIFO(stdin_sb.st_mode) || S_ISREG(stdin_sb.st_mode)) 
virSecurityManagerSetImageFDLabel(driver-securityManager, vm-def, 
stdin_fd)  0)
goto cleanup;
}



Sorry for being so uncertain, but this does not look like what needs to
be done.

Few lines before this code there is virSecurityManagerSetAllLabel()
called.  If the domain is starting with an fd that is not a fifo (thus
already pointing right to the file), the file path is in stdin_path and
that same path should be labeled inside virSecurityManagerSetAllLabel().

I'm not certain this needs fixing as I haven't seen that error with a
scenario that should cause it.

So there are few options what is wrong:

a) some newer selinux keeps the label on the fd pointing to path even
   when path was relabelled (IIRC it does not happen with older
   versions),

b) or we have a bug in our code that the path does not get relabelled,
   but it should not be relabelled here,

c) even if it needs to be relabelled here in the code, the first part
   for the condition you created is effectively always true.  Unless
   resuming from, I don't know, block device or something, in which
   case it would fail as well.

I'd love to make the code fixed, but I'd like to know what is the
scenario that you are trying to fix here.  Maybe the code is exactly as
it needs to be, but I'd like to see an explanation of that in the commit
message if that's the case).  In that case I don't understand why does
it fail with static selinux context only.

Also make sure (with dumpxml) that your machine does not have
relabel=no in the specification.

Martin


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

Re: [libvirt] [PATCH] Don't use AI_ADDRCONFIG when binding to wildcard addresses

[Daniel P. Berrange]

On Fri, May 30, 2014 at 08:21:40AM +0200, Ján Tomko wrote:
 On 05/29/2014 04:47 PM, Eric Blake wrote:
  On 05/29/2014 03:32 AM, Ján Tomko wrote:
  https://bugzilla.redhat.com/show_bug.cgi?id=1098659
 
  With parallel boot, network addresses might not yet be assigned [1],
  but binding to wildcard addresses should work.
 
  For non-wildcard addresses, ADDRCONFIG is still used. Document this
  in libvirtd.conf.
 
  [1] http://www.freedesktop.org/wiki/Software/systemd/NetworkTarget/
  ---
  
  -hints.ai_flags = AI_PASSIVE | AI_ADDRCONFIG;
  +hints.ai_flags = AI_PASSIVE;
   hints.ai_socktype = SOCK_STREAM;
   
  +/* Don't use ADDRCONFIG for binding to the wildcard address.
  + * Just catch the error returned by socket() if the system has
  + * no IPv6 support.
  + *
  + * This allows libvirtd to be started in parallel with the network
  + * startup in most cases.
  + */
  +if (nodename 
  +!(virSocketAddrParse(tmp_addr, nodename, AF_UNSPEC)  0 
  +  virSocketAddrIsWildcard(tmp_addr)))
  +hints.ai_flags = AI_ADDRCONFIG;
  
  Shouldn't this be |= ?
  
 
 Functionally it's the same, AI_PASSIVE is ignored if nodename is non-NULL.
 
 But it should be |= if we were using other flags.

Please make it '|=' regardless. Using '=' is setting a nasty trap-door for
a future change which adds some other flags where use of '|=' will be
needed.

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 3/5] libxl: Move virDomainXMLOptionNew into libxlCreateXMLConf

[Daniel P. Berrange]

On Fri, May 30, 2014 at 04:41:28PM -0600, Jim Fehlig wrote:
 Daniel P. Berrange wrote:
  To allow the test suite to creat the XML option object,
  move the virDomainXMLOptionNew call into a libxlCreateXMLConf
  method.
 
  Signed-off-by: Daniel P. Berrange berra...@redhat.com
  ---
   src/libxl/libxl_conf.c   | 7 +++
   src/libxl/libxl_conf.h   | 2 ++
   src/libxl/libxl_domain.c | 4 ++--
   src/libxl/libxl_driver.c | 4 +---
   4 files changed, 12 insertions(+), 5 deletions(-)
 

  diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
  index e00a3fb..00ff14f 100644
  --- a/src/libxl/libxl_domain.c
  +++ b/src/libxl/libxl_domain.c
  @@ -1100,6 +1100,8 @@ libxlDomainStart(libxlDriverPrivatePtr driver, 
  virDomainObjPtr vm,
   #endif
   virHostdevManagerPtr hostdev_mgr = driver-hostdevMgr;
   
  +libxl_domain_config_init(d_config);
  +
   if (libxlDomainObjPrivateInitCtx(vm)  0)
   return ret;
   
  @@ -1149,8 +1151,6 @@ libxlDomainStart(libxlDriverPrivatePtr driver, 
  virDomainObjPtr vm,
   VIR_FREE(managed_save_path);
   }
   
  -libxl_domain_config_init(d_config);
  -
   if (libxlBuildDomainConfig(driver-reservedVNCPorts, vm-def,
  priv-ctx, d_config)  0)
   goto endjob;

 
 Are these two hunks fixing a bug you found? :-)

Hmm, yes, I should have done those as a separate patch.


The 'd_config' variable is stack allocated so has undefined initial
state. 'libxl_domain_config_init' is basically doing a memset(0,...)
on it to give it predictable value. So if we call that late in the
function, there's a chance that a 'goto endjob' call will jump to
the place where we call libxl_domain_config_dispose(), which will
then access uninitialized memory, will unpredictably bad results.


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 1/2] qemu: Finish device removal in the original thread

[Peter Krempa]

On 05/26/14 17:30, Jiri Denemark wrote:
 If QEMU supports DEVICE_DELETED event, we always call
 qemuDomainRemoveDevice from the event handler. However, we will need to
 push this call away from the main event loop and begin a job for it (see
 the following commit), we need to make sure the device if fully removed
 by the original thread (and within its existing job) in case the
 DEVICE_DELETED event arrives before qemuDomainWaitForDeviceRemoval times
 out.
 
 Without this patch, device removals would be guaranteed to never finish
 before the timeout because the could would be blocked by the original
 job being still active.
 
 Signed-off-by: Jiri Denemark jdene...@redhat.com
 ---
  src/qemu/qemu_hotplug.c | 44 +---
  src/qemu/qemu_hotplug.h |  2 +-
  src/qemu/qemu_process.c |  3 ++-
  3 files changed, 36 insertions(+), 13 deletions(-)
 

ACK,

Peter




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 2/2] qemu: Process DEVICE_DELETED event in a separate thread

[Peter Krempa]

On 05/26/14 17:30, Jiri Denemark wrote:
 Currently, we don not acquire any job when removing a device after
 DEVICE_DELETED event was received from QEMU. This means that if there is
 another API running at the time DEVICE_DELETED is delivered and the API
 acquired a job, we may happily change the definition of the domain the
 API is working with whenever it unlocks the domain object (e.g., to talk
 with its monitor). That said, we have to acquire a job before finishing
 device removal to make things safe. However, doing so in the main event
 loop would cause a deadlock so we need to move most of the event handler
 into a separate thread.
 
 Another good reason for both acquiring a job and handling the event in a
 separate thread is that we currently remove a device backend immediately
 after removing its frontend while we should only remove the backend once
 we already received DEVICE_DELETED event. That is, we will have to talk
 to QEMU monitor from the event handler.
 
 Signed-off-by: Jiri Denemark jdene...@redhat.com
 ---
  src/qemu/qemu_domain.h  |  2 ++
  src/qemu/qemu_driver.c  | 48 ++--
  src/qemu/qemu_process.c | 27 +++
  3 files changed, 67 insertions(+), 10 deletions(-)
 

ACK,

Peter




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 v5] Add helper program to create custom leases

[Daniel P. Berrange]

On Sat, Apr 26, 2014 at 05:29:16AM +0530, Nehal J Wani wrote:

  src/Makefile.am |   22 +++
  src/network/bridge_driver.c |   27 
  src/network/leaseshelper.c  |  360 
 +++

The 'leaseshelper' program also needs to be listed in libvirt.spec.in and
in po/POTFILES.in too. 

 diff --git a/src/network/leaseshelper.c b/src/network/leaseshelper.c
 new file mode 100644
 index 000..d580369
 --- /dev/null
 +++ b/src/network/leaseshelper.c
 @@ -0,0 +1,360 @@
 +/*
 + * leasehelper.c: Helper program to create custom leases file

typo s/leasehelper/leaseshelper/


 +/* Doesn't hurt to check */
 +if (argc  1) {
 +if(STREQ(argv[1], --help))

Missing space after 'if'

 +usage(EXIT_SUCCESS);
 +
 +if (STREQ(argv[1], --version)) {
 +helperVersion(argv[0]);
 +exit(EXIT_SUCCESS);
 +}
 +}
 +rv = EXIT_SUCCESS;
 +
 +cleanup:

Missing leading space before 'cleanup'

ACK with the nits fixed.

In the interests of finally getting this merged, I've corrected the
mistakes and pushed the patch. Please remember to use 'make syntax-check'
before sending patches.

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] [libvirt-glib] [PATCH v2] GVirDomainSnapshot: Add gvir_domain_snapshot_delete

[Christophe Fergeau]

Hey,

On Sun, Jun 01, 2014 at 11:16:47AM +0200, Timm Bäder wrote:
 ---
 
 Whoops, totally forgot about the delete flags in the first version,
 sorry.
 
 As for the underline in domain_snapshot, I took that from
 gvir_domain_snapshot_get_config so I don't really know what's the
 correct version.
 
 Since this is using gvir_domain_snapshot_get_name now, the patch
 removing the #if 0's in libvirt-gobject-domain-snapshot.c is needed.
 
  libvirt-gobject/libvirt-gobject-domain-snapshot.c | 25 
 +++
  libvirt-gobject/libvirt-gobject-domain-snapshot.h | 16 +++
  libvirt-gobject/libvirt-gobject.sym   |  6 ++
  3 files changed, 47 insertions(+)
 
 diff --git a/libvirt-gobject/libvirt-gobject-domain-snapshot.c 
 b/libvirt-gobject/libvirt-gobject-domain-snapshot.c
 index fcf70ed..aa0504d 100644
 --- a/libvirt-gobject/libvirt-gobject-domain-snapshot.c
 +++ b/libvirt-gobject/libvirt-gobject-domain-snapshot.c
 @@ -205,3 +205,28 @@ GVirConfigDomainSnapshot *gvir_domain_snapshot_get_config
  free(xml);
  return conf;
  }
 +
 +/**
 + * gvir_domain_snapshot_delete:
 + * @snapshot: the domain snapshot
 + * @flags: Bitwise or of #GVirDomainSnapshotDeleteFlags
 + * @error: (allow-none): Place-holder for error or NULL
 + */
 +void gvir_domain_snapshot_delete (GVirDomainSnapshot *snapshot,
 +  GVirDomainSnapshotDeleteFlags flags,

This is inconsistent with how it's declared in the .h file (guint flags
over there). I'd favour the 'guint flags' version

 +  GError **error)

Functions taking a GError ** arg generally also return a gboolean
indicating success/failure.

Looks good otherwise.

Christophe


pgpf4maTgPyAo.pgp
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 1/4] qemu: Unref cfg when detaching hostdev interface

[Peter Krempa]

On 05/27/14 16:53, Jiri Denemark wrote:
 Signed-off-by: Jiri Denemark jdene...@redhat.com
 ---
  src/qemu/qemu_hotplug.c | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)

ACK

Peter



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

[libvirt] [PATCH 0/3] Expose distance between host NUMA nodes

[Michal Privoznik]

*** BLURB HERE ***

Michal Privoznik (3):
  virnuma.c: Fix some comments
  virnuma: Introduce virNumaGetDistances
  virCaps: Expose distance between host NUMA nodes

 docs/schemas/capability.rng | 11 
 src/conf/capabilities.c | 13 +-
 src/conf/capabilities.h | 13 +-
 src/libvirt_private.syms|  1 +
 src/libxl/libxl_conf.c  |  4 +--
 src/nodeinfo.c  | 61 ++---
 src/test/test_driver.c  |  2 +-
 src/util/virnuma.c  | 54 +--
 src/util/virnuma.h  |  3 +++
 src/xen/xend_internal.c |  3 ++-
 tests/vircapstest.c |  4 +--
 11 files changed, 156 insertions(+), 13 deletions(-)

-- 
2.0.0

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

[libvirt] [PATCH 1/3] virnuma.c: Fix some comments

[Michal Privoznik]

In 9dd02965 the virNumaGetNodeMemory was introduced, however the
comment describing the function mentions virNumaGetNodeMemorySize.
And there's one typo in virNumaIsAvailable() description.

Signed-off-by: Michal Privoznik mpriv...@redhat.com
---
 src/util/virnuma.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/util/virnuma.c b/src/util/virnuma.c
index bf3b9e6..1e099eb 100644
--- a/src/util/virnuma.c
+++ b/src/util/virnuma.c
@@ -193,7 +193,7 @@ virNumaIsAvailable(void)
  * Get the highest node number available on the current system.
  * (See the node numbers in /sys/devices/system/node/ ).
  *
- * Returns the highes NUMA node id on success, -1 on error.
+ * Returns the highest NUMA node id on success, -1 on error.
  */
 int
 virNumaGetMaxNode(void)
@@ -217,7 +217,7 @@ virNumaGetMaxNode(void)
 
 
 /**
- * virNumaGetNodeMemorySize:
+ * virNumaGetNodeMemory:
  * @node: identifier of the requested NUMA node
  * @memsize: returns the total size of memory in the NUMA node
  * @memfree: returns the total free memory in a NUMA node
-- 
2.0.0

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

[libvirt] [PATCH 3/3] virCaps: Expose distance between host NUMA nodes

[Michal Privoznik]

If user or management application wants to create a guest,
it may be useful to know the cost of internode latencies
before the guest resources are pinned. For example:

capabilities

  host
...
topology
  cells num='2'
cell id='0'
  memory unit='KiB'4004132/memory
  cell id='0' distance='10'/
  cell id='1' distance='20'/
  cpus num='2'
cpu id='0' socket_id='0' core_id='0' siblings='0'/
cpu id='2' socket_id='0' core_id='2' siblings='2'/
  /cpus
/cell
cell id='1'
  memory unit='KiB'4030064/memory
  cell id='0' distance='20'/
  cell id='1' distance='10'/
  cpus num='2'
cpu id='1' socket_id='0' core_id='0' siblings='1'/
cpu id='3' socket_id='0' core_id='2' siblings='3'/
  /cpus
/cell
  /cells
/topology
...
  /host
  ...
/capabilities

we can see the distance from node1 to node0 is 20 and within nodes 10.

Signed-off-by: Michal Privoznik mpriv...@redhat.com
---
 docs/schemas/capability.rng | 11 
 src/conf/capabilities.c | 13 +-
 src/conf/capabilities.h | 13 +-
 src/libxl/libxl_conf.c  |  4 +--
 src/nodeinfo.c  | 61 ++---
 src/test/test_driver.c  |  2 +-
 src/xen/xend_internal.c |  3 ++-
 tests/vircapstest.c |  4 +--
 8 files changed, 100 insertions(+), 11 deletions(-)

diff --git a/docs/schemas/capability.rng b/docs/schemas/capability.rng
index d2d9776..63b53b9 100644
--- a/docs/schemas/capability.rng
+++ b/docs/schemas/capability.rng
@@ -188,6 +188,17 @@
 ref name='memory'/
   /optional
 
+  zeroOrMore
+element name='cell'
+  attribute name='id'
+ref name='unsignedInt'/
+  /attribute
+  attribute name='distance'
+ref name='unsignedInt'/
+  /attribute
+/element
+  /zeroOrMore
+
   optional
 element name='cpus'
   attribute name='num'
diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c
index cf474d7..e66abb2 100644
--- a/src/conf/capabilities.c
+++ b/src/conf/capabilities.c
@@ -107,6 +107,7 @@ virCapabilitiesFreeHostNUMACell(virCapsHostNUMACellPtr cell)
 virCapabilitiesClearHostNUMACellCPUTopology(cell-cpus, cell-ncpus);
 
 VIR_FREE(cell-cpus);
+VIR_FREE(cell-siblings);
 VIR_FREE(cell);
 }
 
@@ -286,8 +287,10 @@ int
 virCapabilitiesAddHostNUMACell(virCapsPtr caps,
int num,
int ncpus,
+   int nsiblings,
unsigned long long mem,
-   virCapsHostNUMACellCPUPtr cpus)
+   virCapsHostNUMACellCPUPtr cpus,
+   virCapsHostNUMACellSiblingInfoPtr siblings)
 {
 virCapsHostNUMACellPtr cell;
 
@@ -302,6 +305,8 @@ virCapabilitiesAddHostNUMACell(virCapsPtr caps,
 cell-num = num;
 cell-mem = mem;
 cell-cpus = cpus;
+cell-siblings = siblings;
+cell-nsiblings = nsiblings;
 
 caps-host.numaCell[caps-host.nnumaCell++] = cell;
 
@@ -766,6 +771,12 @@ virCapabilitiesFormatNUMATopology(virBufferPtr buf,
 virBufferAsprintf(buf, memory unit='KiB'%llu/memory\n,
   cells[i]-mem);
 
+for (j = 0; j  cells[i]-nsiblings; j++) {
+virBufferAsprintf(buf, cell id='%d' distance='%d'/\n,
+  cells[i]-siblings[j].node,
+  cells[i]-siblings[j].distance);
+}
+
 virBufferAsprintf(buf, cpus num='%d'\n, cells[i]-ncpus);
 virBufferAdjustIndent(buf, 2);
 for (j = 0; j  cells[i]-ncpus; j++) {
diff --git a/src/conf/capabilities.h b/src/conf/capabilities.h
index ba99e1a..5da31a8 100644
--- a/src/conf/capabilities.h
+++ b/src/conf/capabilities.h
@@ -95,6 +95,13 @@ struct _virCapsHostNUMACellCPU {
 virBitmapPtr siblings;
 };
 
+typedef struct _virCapsHostNUMACellSiblingInfo virCapsHostNUMACellSiblingInfo;
+typedef virCapsHostNUMACellSiblingInfo *virCapsHostNUMACellSiblingInfoPtr;
+struct _virCapsHostNUMACellSiblingInfo {
+int node;   /* foreign NUMA node */
+unsigned int distance;  /* distance to the node */
+};
+
 typedef struct _virCapsHostNUMACell virCapsHostNUMACell;
 typedef virCapsHostNUMACell *virCapsHostNUMACellPtr;
 struct _virCapsHostNUMACell {
@@ -102,6 +109,8 @@ struct _virCapsHostNUMACell {
 int ncpus;
 unsigned long long mem; /* in kibibytes */
 virCapsHostNUMACellCPUPtr cpus;
+int nsiblings;
+virCapsHostNUMACellSiblingInfoPtr siblings;
 };
 
 typedef struct _virCapsHostSecModelLabel virCapsHostSecModelLabel;
@@ -194,8 +203,10 @@ extern int
 virCapabilitiesAddHostNUMACell(virCapsPtr caps,
int num,
int ncpus,
+   int 

[libvirt] [PATCH 2/3] virnuma: Introduce virNumaGetDistances

[Michal Privoznik]

The API gets a NUMA node and find distances to other nodes.  The
distances are returned in an array. If an item X within the array
equals to value of zero, then there's no such node as X.

Signed-off-by: Michal Privoznik mpriv...@redhat.com
---
 src/libvirt_private.syms |  1 +
 src/util/virnuma.c   | 50 
 src/util/virnuma.h   |  3 +++
 3 files changed, 54 insertions(+)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index cb635cd..6168f76 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1652,6 +1652,7 @@ virNodeSuspendGetTargetMask;
 virDomainNumatuneMemModeTypeFromString;
 virDomainNumatuneMemModeTypeToString;
 virNumaGetAutoPlacementAdvice;
+virNumaGetDistances;
 virNumaGetMaxNode;
 virNumaGetNodeMemory;
 virNumaIsAvailable;
diff --git a/src/util/virnuma.c b/src/util/virnuma.c
index 1e099eb..68b2698 100644
--- a/src/util/virnuma.c
+++ b/src/util/virnuma.c
@@ -217,6 +217,56 @@ virNumaGetMaxNode(void)
 
 
 /**
+ * virNumaGetDistances:
+ * @node: identifier of the requested NUMA node
+ * @distances: array of distances to sibling nodes
+ * @ndistances: size of @distances
+ *
+ * Get array of distances to sibling nodes from @node. If a
+ * distances[x] equals to zero, the node x is not enabled or
+ * doesn't exist. As a special case, if @node itself refers to
+ * disabled or nonexistent NUMA node, then @distances and
+ * @ndistances are set to NULL and zero respectively.
+ *
+ * Returns 0 on success, -1 otherwise.
+ */
+int
+virNumaGetDistances(int node,
+int **distances,
+int *ndistances)
+{
+int ret = -1;
+int max_node;
+size_t i;
+
+if (!numa_bitmask_isbitset(numa_nodes_ptr, node)) {
+VIR_DEBUG(Node %d does not exist, node);
+*distances = NULL;
+*ndistances = 0;
+return 0;
+}
+
+if ((max_node = virNumaGetMaxNode())  0)
+goto cleanup;
+
+if (VIR_ALLOC_N(*distances, max_node)  0)
+goto cleanup;
+
+*ndistances = max_node + 1;
+
+for (i = 0; i= max_node; i++) {
+if (!numa_bitmask_isbitset(numa_nodes_ptr, i))
+continue;
+
+(*distances)[i] = numa_distance(node, i);
+}
+
+ret = 0;
+ cleanup:
+return ret;
+}
+
+/**
  * virNumaGetNodeMemory:
  * @node: identifier of the requested NUMA node
  * @memsize: returns the total size of memory in the NUMA node
diff --git a/src/util/virnuma.h b/src/util/virnuma.h
index 8464b19..fe1e966 100644
--- a/src/util/virnuma.h
+++ b/src/util/virnuma.h
@@ -58,6 +58,9 @@ int virNumaSetupMemoryPolicy(virNumaTuneDef numatune,
 
 bool virNumaIsAvailable(void);
 int virNumaGetMaxNode(void);
+int virNumaGetDistances(int node,
+int **distances,
+int *ndistances);
 int virNumaGetNodeMemory(int node,
  unsigned long long *memsize,
  unsigned long long *memfree);
-- 
2.0.0

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

Re: [libvirt] [PATCH 2/4] qemu: Remove interface backend only after frontend is gone

[Peter Krempa]

On 05/27/14 16:53, Jiri Denemark wrote:
 [1] reported that we are removing network's backend too early. I didn't
 really get the reproducer but libvirt behaves strangely when a guest
 does not confirm the removal, e.g., it does not support PCI hotplug. In
 such case, detaching a network device leaves its frontend in place but
 removes the backend, which makes the device unusable for the guest.
 Moreover attaching the same device again succeeds and both the guest and
 libvirt will see two network interfaces attached but only one of them is
 actually working.
 
 I checked with Paolo Bonzini and he confirmed we should only remove a
 backend after seeing DEVICE_DELETED event for a corresponding frontend.
 
 [1] https://www.redhat.com/archives/libvir-list/2014-March/msg01740.html
 
 Signed-off-by: Jiri Denemark jdene...@redhat.com
 ---
  src/qemu/qemu_hotplug.c | 56 
 +
  1 file changed, 29 insertions(+), 27 deletions(-)
 
 diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
 index b209b04..fd1f002 100644
 --- a/src/qemu/qemu_hotplug.c
 +++ b/src/qemu/qemu_hotplug.c
 @@ -2636,8 +2636,10 @@ qemuDomainRemoveNetDevice(virQEMUDriverPtr driver,
virDomainNetDefPtr net)
  {
  virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
 +qemuDomainObjPrivatePtr priv = vm-privateData;
  virNetDevVPortProfilePtr vport;
  virObjectEventPtr event;
 +char *hostnet_name = NULL;
  size_t i;
  
  if (virDomainNetGetActualType(net) == VIR_DOMAIN_NET_TYPE_HOSTDEV) {
 @@ -2649,6 +2651,32 @@ qemuDomainRemoveNetDevice(virQEMUDriverPtr driver,
  VIR_DEBUG(Removing network interface %s from domain %p %s,
net-info.alias, vm, vm-def-name);
  
 +if (virAsprintf(hostnet_name, host%s, net-info.alias)  0)
 +goto cleanup;
 +
 +qemuDomainObjEnterMonitor(driver, vm);
 +if (virQEMUCapsGet(priv-qemuCaps, QEMU_CAPS_NETDEV) 
 +virQEMUCapsGet(priv-qemuCaps, QEMU_CAPS_DEVICE)) {
 +if (qemuMonitorRemoveNetdev(priv-mon, hostnet_name)  0) {
 +qemuDomainObjExitMonitor(driver, vm);
 +virDomainAuditNet(vm, net, NULL, detach, false);
 +goto cleanup;
 +}
 +} else {
 +int vlan;
 +if ((vlan = qemuDomainNetVLAN(net))  0 ||
 +qemuMonitorRemoveHostNetwork(priv-mon, vlan, hostnet_name)  0) 
 {
 +if (vlan  0) {
 +virReportError(VIR_ERR_OPERATION_FAILED, %s,
 +   _(unable to determine original VLAN));


This function is void, so this error won't get propagated as it was in
the original place. As this function is called from the place the above
code originally was you should probably convert it to return int and
propagate the error.

 +}
 +qemuDomainObjExitMonitor(driver, vm);
 +virDomainAuditNet(vm, net, NULL, detach, false);
 +goto cleanup;
 +}
 +}
 +qemuDomainObjExitMonitor(driver, vm);
 +
  virDomainAuditNet(vm, net, NULL, detach, true);
  
  event = virDomainEventDeviceRemovedNewFromObj(vm, net-info.alias);

Peter




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 4/4] qemu: Remove character device backend only after frontend is gone

[Peter Krempa]

On 05/27/14 16:53, Jiri Denemark wrote:
 In general, we should only remove a backend after seeing DEVICE_DELETED
 event for a corresponding frontend.
 
 Signed-off-by: Jiri Denemark jdene...@redhat.com
 ---
  src/qemu/qemu_hotplug.c | 25 +++--
  1 file changed, 15 insertions(+), 10 deletions(-)
 
 diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
 index 43c52bf..4c2f6e3 100644
 --- a/src/qemu/qemu_hotplug.c
 +++ b/src/qemu/qemu_hotplug.c
 @@ -2743,16 +2743,31 @@ qemuDomainRemoveChrDevice(virQEMUDriverPtr driver,
virDomainChrDefPtr chr)
  {
  virObjectEventPtr event;
 +char *charAlias = NULL;
 +qemuDomainObjPrivatePtr priv = vm-privateData;
  
  VIR_DEBUG(Removing character device %s from domain %p %s,
chr-info.alias, vm, vm-def-name);
  
 +if (virAsprintf(charAlias, char%s, chr-info.alias)  0)
 +return;
 +
 +qemuDomainObjEnterMonitor(driver, vm);
 +if (qemuMonitorDetachCharDev(priv-mon, charAlias)  0) {
 +qemuDomainObjExitMonitor(driver, vm);
 +goto cleanup;
 +}
 +qemuDomainObjExitMonitor(driver, vm);
 +

Same conditions as in 3/4.

Peter




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 3/4] qemu: Remove disk backend only after frontend is gone

[Peter Krempa]

On 05/27/14 16:53, Jiri Denemark wrote:
 In general, we should only remove a backend after seeing DEVICE_DELETED
 event for a corresponding frontend. This doesn't make any difference for
 disks attached using -drive or drive_add since QEMU automatically
 removes their frontends but it's still better to make our code

s/frontends/backends/ ?

 consistent. And it may start making difference in case we switch to
 attaching disks using -blockdev.
 
 Signed-off-by: Jiri Denemark jdene...@redhat.com
 ---
  src/qemu/qemu_hotplug.c | 37 +
  1 file changed, 13 insertions(+), 24 deletions(-)
 
 diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
 index fd1f002..43c52bf 100644
 --- a/src/qemu/qemu_hotplug.c
 +++ b/src/qemu/qemu_hotplug.c
 @@ -2472,10 +2472,23 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver,
  virObjectEventPtr event;
  size_t i;
  const char *src = virDomainDiskGetSource(disk);
 +qemuDomainObjPrivatePtr priv = vm-privateData;
 +char *drivestr;
  
  VIR_DEBUG(Removing disk %s from domain %p %s,
disk-info.alias, vm, vm-def-name);
  
 +/* build the actual drive id string as the disk-info.alias doesn't
 + * contain the QEMU_DRIVE_HOST_PREFIX that is passed to qemu */
 +if (virAsprintf(drivestr, %s%s,
 +QEMU_DRIVE_HOST_PREFIX, disk-info.alias)  0)
 +return;

Should this be treated the same way for propagating errors as in the
previous patch? I'm not going to enforce this here as the allocaion is
unlikely to fail.

 +
 +qemuDomainObjEnterMonitor(driver, vm);
 +qemuMonitorDriveDel(priv-mon, drivestr);
 +qemuDomainObjExitMonitor(driver, vm);
 +VIR_FREE(drivestr);
 +
  virDomainAuditDisk(vm, src, NULL, detach, true);
  
  event = virDomainEventDeviceRemovedNewFromObj(vm, disk-info.alias);

ACK if you go with this patch as-is, v2 if you upgrade the error reporting.

Peter





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

Re: [libvirt] [Xen-devel] [PATCH 0/5] Testing libvirt XML - libxl_domain_config conversion

[Ian Campbell]

On Fri, 2014-05-30 at 18:24 +0100, Daniel P. Berrange wrote:
 At the Xen Hackathon I learnt that libxl provides an API which
 can serialize a libxl_domain_config instance to a JSON document.
 This is exactly what we need for testing the XML - libxl_domain_config
 conversion process, so I spent the afternoon trying to get such a test
 working. The result is that we can now just add pairs of XML, JSON
 files to libvirt to test handling of new config features.
 
 I hit a couple of small issues with libxl, which I worked around, when
 writing this test which I why I'm copying xen-devel
 
  - libxl_ctx_alloc() will call xs_daemon_open and xc_interface_open,
and stat /var/run/xenstored.pid to see if Xen is actually running.
This fails when run on non-Xen hosts (and also possibly if run
unprivileged).
 
I used an evil LD_PRELOAD hack to stub out xs_daemon_open and
xc_interface_open to return (void*)0x1, and also turn
xc_interface_close and xs_daemon_close to no-ops, and make
stat() always return success for xenstored.pid.
 
This works (evidenced by the fact that if something was needing
these xs/xc handles they would have crashed referencing 0x1), 
but at the same time it might be an idea to have an officially
supported  non live mode for libxl_ctx_alloc() turned on by a
flag of some sort.

Yes, we absolutely should have this.

  - The libxl_json.h header file is relying on conditionals that
are only set by Xen's build process
 
 eg HAVE_YAJL_YAJL_VERSION_H
 

It looks like this header has ended up with a mixture of library
internal and user facing stuff, which is wrong. I think splitting the
internal stuff into libxl_json_internal.h or similar would solve this.
I'll take a look.

I hacked around this, but it is a little dirty too. libvirt
already links to libyajl for the QEMU driver, but we  don't
really need the raw YAJL objects. It'd be nice to have a
 
   char * libxl_domain_config_as_json(libxl_domain_config *p)
 
as a higher level wrapper around libxl_domain_config_gen_json
avoiding the pain of dealing with YAJL's different APIs.
 
Ian J mentioned to me that he thought there was already such a
method, but AFAICT, the only such code is in the 'xl' command
line tool itself (xl_cmdimpl.c - printf_info_one_json)

That was me not Ian J I think.

The function you need is libxl_domain_config_to_json (which is
autogenerated, so in _libxl_types.[hc]). I think the general
libxl_*_to_json support has been there since Xen 4.2, but IIRC
libxl_domain_config only got moved into the autogenerated/IDL layer in
4.3.

For compatibility with 4.2 you could probably use libxl_*_to_json on the
various members of libxl_domain_config individually, or just punt on the
unit tests in that case of course...

Ian.

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

Re: [libvirt] [Xen-devel] [PATCH 0/5] Testing libvirt XML - libxl_domain_config conversion

[Daniel P. Berrange]

On Mon, Jun 02, 2014 at 01:41:58PM +0100, Ian Campbell wrote:
 I hacked around this, but it is a little dirty too. libvirt
 already links to libyajl for the QEMU driver, but we  don't
 really need the raw YAJL objects. It'd be nice to have a
  
char * libxl_domain_config_as_json(libxl_domain_config *p)
  
 as a higher level wrapper around libxl_domain_config_gen_json
 avoiding the pain of dealing with YAJL's different APIs.
  
 Ian J mentioned to me that he thought there was already such a
 method, but AFAICT, the only such code is in the 'xl' command
 line tool itself (xl_cmdimpl.c - printf_info_one_json)
 
 That was me not Ian J I think.

Opps, my bad, was talking to too many people to remember things clearly :-)
 
 The function you need is libxl_domain_config_to_json (which is
 autogenerated, so in _libxl_types.[hc]). I think the general
 libxl_*_to_json support has been there since Xen 4.2, but IIRC
 libxl_domain_config only got moved into the autogenerated/IDL layer in
 4.3.

Ahhh, so I was looking in the wrong place - no wonder 'git grep' failed
to find it :-)

 For compatibility with 4.2 you could probably use libxl_*_to_json on the
 various members of libxl_domain_config individually, or just punt on the
 unit tests in that case of course...

Yeah, I think it is reasonable to just disable the test case if
this function is missing.  So I can avoid libxl_json.h entirely
in this case.

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] [Xen-devel] [PATCH 5/5] Add a test suite for libxl option generator

[Ian Campbell]

On Fri, 2014-05-30 at 18:24 +0100, Daniel P. Berrange wrote:
 +if (STRNEQ(expectargv, (char *)actualargv)) {
 +virtTestDifference(stderr, expectargv, (char *)actualargv);
 +goto cleanup;
 +}

Since you are using libxl_domain_config_gen_json you can control the
pretty printing, but if you were to use the libxl_domain_config_to_json
you might have problems if the library was to do something slightly
different e.g. with whitespace.

In 4.5 we will have libxl_*_from_json and (I think) libxl_*_compare, so
you could read in the template and compare it with the generated struct.
That doesn't help you now of course.

Also in 4.5 the json will omit fields which are set to the their
explicit default value. libxl_*_from_json will still do the right thing,
but it'd be another annoyance for you here I think.

Lastly, when we add new fields to the API they will start showing up in
the json (modulo the omission of defaults discussed above).

Other than having a template per libxl version (yuck) I don't have any
particularly smart suggestions except for to aim long
term for:
  libxl_domain_config_init(..., template)
  libxl_domain_config_from_json(..., template, { json json json )

  libxl_domain_config_init(..., xml)
  virtLibxlFromXML(..., xml, domain)

  libxl_domain_config_compare(template, xml)

which will likely handle all of those corner cases, except perhaps the
case where libvirt is using new fields on new versions of libxl etc. I
suppose that would be handled by specific unit tests for = and 
versions of libxl separately?

Ian.


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

Re: [libvirt] [Xen-devel] [PATCH 5/5] Add a test suite for libxl option generator

[Daniel P. Berrange]

On Mon, Jun 02, 2014 at 01:53:46PM +0100, Ian Campbell wrote:
 
 On Fri, 2014-05-30 at 18:24 +0100, Daniel P. Berrange wrote:
  +if (STRNEQ(expectargv, (char *)actualargv)) {
  +virtTestDifference(stderr, expectargv, (char *)actualargv);
  +goto cleanup;
  +}
 
 Since you are using libxl_domain_config_gen_json you can control the
 pretty printing, but if you were to use the libxl_domain_config_to_json
 you might have problems if the library was to do something slightly
 different e.g. with whitespace.
 
 In 4.5 we will have libxl_*_from_json and (I think) libxl_*_compare, so
 you could read in the template and compare it with the generated struct.
 That doesn't help you now of course.
 
 Also in 4.5 the json will omit fields which are set to the their
 explicit default value. libxl_*_from_json will still do the right thing,
 but it'd be another annoyance for you here I think.
 
 Lastly, when we add new fields to the API they will start showing up in
 the json (modulo the omission of defaults discussed above).

Hmm, that's a v good point.

 Other than having a template per libxl version (yuck) I don't have any
 particularly smart suggestions except for to aim long
 term for:
   libxl_domain_config_init(..., template)
   libxl_domain_config_from_json(..., template, { json json json )
 
   libxl_domain_config_init(..., xml)
   virtLibxlFromXML(..., xml, domain)
 
   libxl_domain_config_compare(template, xml)
 
 which will likely handle all of those corner cases, except perhaps the
 case where libvirt is using new fields on new versions of libxl etc. I
 suppose that would be handled by specific unit tests for = and 
 versions of libxl separately?

I think that perhaps I should just add a general JSON DOM comparison
function in libvirt - no need for it to be libxl specific, as it might
be useful for libvirt JSON usage elsewhere.

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] [Xen-devel] [PATCH 0/5] Testing libvirt XML - libxl_domain_config conversion

[Ian Campbell]

create !
title it libxl_ctx_alloc should have dummy mode which does not require a Xen 
host
severity it wishlist
thanks
(just creating a bug for this issue)

On Mon, 2014-06-02 at 13:41 +0100, Ian Campbell wrote:
 On Fri, 2014-05-30 at 18:24 +0100, Daniel P. Berrange wrote:
  I hit a couple of small issues with libxl, which I worked around, when
  writing this test which I why I'm copying xen-devel
  
   - libxl_ctx_alloc() will call xs_daemon_open and xc_interface_open,
 and stat /var/run/xenstored.pid to see if Xen is actually running.
 This fails when run on non-Xen hosts (and also possibly if run
 unprivileged).
  
 I used an evil LD_PRELOAD hack to stub out xs_daemon_open and
 xc_interface_open to return (void*)0x1, and also turn
 xc_interface_close and xs_daemon_close to no-ops, and make
 stat() always return success for xenstored.pid.
  
 This works (evidenced by the fact that if something was needing
 these xs/xc handles they would have crashed referencing 0x1), 
 but at the same time it might be an idea to have an officially
 supported  non live mode for libxl_ctx_alloc() turned on by a
 flag of some sort.
 
 Yes, we absolutely should have this.


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

[libvirt] [PATCH 2/2] Parallels: add connectBaselineCPU. Openstack Nova (starting at icehouse release) requires this function to start VM. Because the information is taken from host capabilities xml,

[Alexander Burluka]

From: A.Burluka aburl...@parallels.com

---
 src/parallels/parallels_driver.c |   34 --
 1 files changed, 32 insertions(+), 2 deletions(-)

diff --git a/src/parallels/parallels_driver.c b/src/parallels/parallels_driver.c
index f1d5ecc..3aa73f0 100644
--- a/src/parallels/parallels_driver.c
+++ b/src/parallels/parallels_driver.c
@@ -51,6 +51,7 @@
 #include nodeinfo.h
 #include c-ctype.h
 #include virstring.h
+#include cpu/cpu.h
 
 #include parallels_driver.h
 #include parallels_utils.h
@@ -117,7 +118,9 @@ parallelsDomObjFreePrivate(void *p)
 static virCapsPtr
 parallelsBuildCapabilities(void)
 {
-virCapsPtr caps;
+virCapsPtr caps = NULL;
+virCPUDefPtr cpu = NULL;
+virCPUDataPtr data = NULL;
 virCapsGuestPtr guest;
 
 if ((caps = virCapabilitiesNew(virArchFromHost(),
@@ -147,11 +150,25 @@ parallelsBuildCapabilities(void)
   parallels, NULL, NULL, 0, NULL) == 
NULL)
 goto error;
 
+if (VIR_ALLOC(cpu)  0)
+goto error;
+
+cpu-arch = caps-host.arch;
+cpu-type = VIR_CPU_TYPE_HOST;
+caps-host.cpu = cpu;
+
+if (!(data = cpuNodeData(cpu-arch))
+|| cpuDecode(cpu, data, NULL, 0, NULL)  0) {
+goto error;
+}
+
+ cleanup:
+cpuDataFree(data);
 return caps;
 
  error:
 virObjectUnref(caps);
-return NULL;
+goto cleanup;
 }
 
 static char *
@@ -2312,6 +2329,18 @@ static int parallelsConnectIsAlive(virConnectPtr conn 
ATTRIBUTE_UNUSED)
 }
 
 
+static char *
+parallelsConnectBaselineCPU(virConnectPtr conn ATTRIBUTE_UNUSED,
+const char **xmlCPUs,
+unsigned int ncpus,
+unsigned int flags)
+{
+virCheckFlags(VIR_CONNECT_BASELINE_CPU_EXPAND_FEATURES, NULL);
+
+return cpuBaselineXML(xmlCPUs, ncpus, NULL, 0, flags);
+}
+
+
 static int
 parallelsNodeGetCpuMask(parallelsDomObjPtr privatedomdata,
 virBitmapPtr *cpumask,
@@ -2471,6 +2500,7 @@ static virDriver parallelsDriver = {
 .connectGetHostname = parallelsConnectGetHostname,  /* 0.10.0 */
 .nodeGetInfo = parallelsNodeGetInfo,  /* 0.10.0 */
 .connectGetCapabilities = parallelsConnectGetCapabilities,  /* 0.10.0 
*/
+.connectBaselineCPU = parallelsConnectBaselineCPU, /* 1.2.6 */
 .connectListDomains = parallelsConnectListDomains,  /* 0.10.0 */
 .connectNumOfDomains = parallelsConnectNumOfDomains,/* 0.10.0 */
 .connectListDefinedDomains = parallelsConnectListDefinedDomains,/* 
0.10.0 */
-- 
1.7.1

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

[libvirt] parallels: add 2 functions needed by OpenStack

[Alexander Burluka]

Hello,

I would like to contribute in Libvirt development. Parallels team continues 
working
on driver to make it compatible with OpenStack. So, this patches are part of our
goal. I would gracefully accept your comments and remarks.

Thank you!

--
Regards,
Alexander Burluka

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

[libvirt] [PATCH 1/2] Parallels: add domainGetVcpus(). OpenStack Nova requires this function to start VM instance. Cpumask info is obtained via prlctl utility. Unlike KVM, Parallels Cloud Server is un

[Alexander Burluka]

From: A.Burluka aburl...@parallels.com

---
 src/parallels/parallels_driver.c |  169 +-
 src/parallels/parallels_utils.h  |1 +
 2 files changed, 167 insertions(+), 3 deletions(-)

diff --git a/src/parallels/parallels_driver.c b/src/parallels/parallels_driver.c
index ab59599..f1d5ecc 100644
--- a/src/parallels/parallels_driver.c
+++ b/src/parallels/parallels_driver.c
@@ -108,6 +108,7 @@ parallelsDomObjFreePrivate(void *p)
 if (!pdom)
 return;
 
+VIR_FREE(pdom-cpumask);
 VIR_FREE(pdom-uuid);
 VIR_FREE(pdom-home);
 VIR_FREE(p);
@@ -654,6 +655,9 @@ parallelsLoadDomain(parallelsConnPtr privconn, 
virJSONValuePtr jobj)
 if (VIR_ALLOC(def)  0)
 goto cleanup;
 
+if (VIR_ALLOC(pdom)  0)
+goto cleanup;
+
 def-virtType = VIR_DOMAIN_VIRT_PARALLELS;
 def-id = -1;
 
@@ -716,6 +720,17 @@ parallelsLoadDomain(parallelsConnPtr privconn, 
virJSONValuePtr jobj)
 goto cleanup;
 }
 
+if (!(tmp = virJSONValueObjectGetString(jobj3, mask))) {
+/* Absence of this field means that all domains cpus are available */
+if (VIR_STRDUP(pdom-cpumask, all)  0) {
+goto cleanup;
+}
+} else {
+if (VIR_STRDUP(pdom-cpumask, tmp)  0) {
+goto cleanup;
+}
+}
+
 if (!(jobj3 = virJSONValueObjectGet(jobj2, memory))) {
 parallelsParseError();
 goto cleanup;
@@ -757,9 +772,6 @@ parallelsLoadDomain(parallelsConnPtr privconn, 
virJSONValuePtr jobj)
 
 def-os.arch = VIR_ARCH_X86_64;
 
-if (VIR_ALLOC(pdom)  0)
-goto cleanup;
-
 if (virJSONValueObjectGetNumberUint(jobj, EnvID, x)  0)
 goto cleanup;
 pdom-id = x;
@@ -2300,6 +2312,156 @@ static int parallelsConnectIsAlive(virConnectPtr conn 
ATTRIBUTE_UNUSED)
 }
 
 
+static int
+parallelsNodeGetCpuMask(parallelsDomObjPtr privatedomdata,
+virBitmapPtr *cpumask,
+int hostcpus)
+{
+int ret = -1;
+int cpunum = -1;
+int prevcpunum = -1;
+int offset = 0;
+const char *it = privatedomdata-cpumask;
+bool isrange = false;
+size_t i;
+int cpunums[512] = { 0 };
+size_t count = 0;
+
+if (STREQ(it, all)) {
+if (!(*cpumask = virBitmapNew(hostcpus)))
+goto cleanup;
+virBitmapSetAll(*cpumask);
+} else {
+while (sscanf(it, %d%n, cpunum, offset)) {
+char delim = 0;
+if (isrange) {
+for (i = prevcpunum + 1; i = cpunum; ++i) {
+cpunums[count++] = i;
+}
+} else {
+cpunums[count++] = cpunum;
+}
+
+it += offset;
+
+if (sscanf(it, %c%n, delim, offset) == EOF) {
+break;
+} else {
+it += offset;
+switch (delim) {
+case ',':
+isrange = false;
+break;
+case '-':
+isrange = true;
+prevcpunum = cpunum;
+break;
+default:
+virReportError(VIR_ERR_INVALID_ARG,
+   _(Invalid cpumask format '%s'),
+   privatedomdata-cpumask);
+goto cleanup;
+break;
+}
+}
+}
+if (!(*cpumask = virBitmapNew(cpunums[count-1] + 1)))
+goto cleanup;
+virBitmapClearAll(*cpumask);
+for (i = 0; i  count; ++i) {
+if (virBitmapSetBit(*cpumask, cpunums[i]) == -1) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _(cannot set %d bit in cpumask),
+   cpunums[i]);
+goto cleanup;
+}
+}
+}
+
+return 0;
+ cleanup:
+virBitmapFree(*cpumask);
+return ret;
+}
+
+static int
+parallelsDomainGetVcpus(virDomainPtr domain,
+   virVcpuInfoPtr info,
+   int maxinfo,
+   unsigned char *cpumaps,
+   int maplen)
+{
+parallelsConnPtr privconn = domain-conn-privateData;
+parallelsDomObjPtr privdomdata = NULL;
+virDomainObjPtr privdom = NULL;
+size_t i;
+int v, maxcpu, hostcpus;
+int ret = -1;
+
+parallelsDriverLock(privconn);
+privdom = virDomainObjListFindByUUID(privconn-domains, domain-uuid);
+parallelsDriverUnlock(privconn);
+
+if (privdom == NULL) {
+parallelsDomNotFoundError(domain);
+goto cleanup;
+}
+
+if (!virDomainObjIsActive(privdom)) {
+virReportError(VIR_ERR_OPERATION_INVALID,
+   %s,
+   _(cannot list vcpu pinning for an inactive domain));
+goto cleanup;
+}
+
+privdomdata = privdom-privateData;
+if ((hostcpus = nodeGetCPUCount())  0)
+goto cleanup;
+
+

[libvirt] [PATCH v7 RFC 0/2] libxl USB prototype and design discussion

[George Dunlap]

Hey all!  

So here I finally have a rebased the HVM USB hotplug series from last
year.  I went through and addressed all of IanC's technical comments
that I could; so it should be in much better shape than it was.

However, one of the unfinished threads of the conversation at that
time was about the interface.  In particular since Simon Bo is now
working on the PVUSB side of things, I thought it would be good to
review the situation so we can get an interface we're happy with.

In order to really answer the questions, I went through and looked at
the underlying capabilities of QEMU and PVUSB, at the existing libxl
and xend interfaces for those two, and also at the libvirt interface.

I summarize what I've found here; but if you want a TLDR version, just
skip to the bottom, where it says Draft Design Proposal.  But before
asking any questions or making any criticisms, please go back and read
the appropriate summary section.

References are at the very bottom of the e-mail.

This can be found at the following address:

git://xenbits.xen.org/people/gdunlap/xen.git out/hvm-usb-hotplug-v7-RFC

== Capabilities of QEMU ==

Qemu has the additional ability to not only pass through host devices,
but other kinds of devices -- tablets, mice, keyboards, drives, hubs,
and so on.  As far as I know, PVUSB can only do host device
pass-through at the moment.  (Although you could certainly imagine
that changing; if qemu was taught how to become a PVUSB back-end, for
example.)

qemu has two ways of specifying usb devices.  The legacy method can
be specified on the qemu command-line or via the qemu monitor.  It is
limited to USB 1.1.  The new qdevice method can be used either on
the command-line or via qmp.

qemu-traditional only supports the legacy method.  qemu-xen still
supports the legacy method via command-line, but we should try to
start using the qdevice method if we can.

qemu-xen has the ability to create virtual USB 2.0 or 3.0 controllers.
These can be named, and it is possible to specify (to a certain level)
which controller to plug a device into.  These can be created either
via the command-line, or hot-plugged via qmp.

qemu also seems to be willing to shift things around behind the scenes
to be able to handle your request; shifting around the USB topology,
for instance.  It will also do helpful things; for instance, if you
specify that you want to plug a device into a USB 2.0 controller, and
the device is only 1.1 capable, it will ignore what you asked and plug
it into the 1.1 controller.


== Capabilities of PVUSB ==

PVUSB also requires you to first create a virtual controller, before
attaching host devices to it.  There is some flexibility in how to
create these, and you can create more than one and specify which
virtual bus to attach the host device.  You seem to be able to specify
the USB version number when you specify the controller as well.  You
can also specify the number of ports for a device; I'm not sure what
the maximum number of ports per controller is, or why you would ever
really want to have less than the maximum number of ports.

It looks like in the xm interface, when you create a virtual
controller it is assigned an index, starting at 0; and this is the
index that is used when specifying where to plug in a device.

(Simon, can you please correct this if I'm wrong, and add anything
important that I missed?)


== libxl/xl interface ==

At the moment, libxl/xl only support USB at domain creation time.

For HVM guests, we have two incompatible sets of options.  usb=1 and
usbdevice=[] use the old-style qemu interface on the qemu
command-line.  The old-style interface can only be used to specify
USB 1.1 devices.  We also have usbversion=[foo], which uses the
new-style device specification commands.  These new specification
commands *can* be specified either on the qemu command-line or via
qmp; but at the moment libxl specifies them on the command-line.

The patch series attached specifes using the new-style interface via
qmp.  (This seems to work properly with the various controllers no
matter how they were created.)  In theory, we should be able to attach
these devices during domain creation, after qemu has been started but
before the guest is running.

Obviously, we would ideally like for the user not to have to worry
about a lot of this complexity, and just say, Can you pass this
device through to the guest?  Thanks.

Another thing to consider in the design space is config file and
start-up behavior.


== Libvirt's interface ==

I've had a brief look at libvirt's USB interface, and learned a bit
about libvirt's general approach to things at the Xen Hackathon last
week.  One of the goals of libvirt is to be able to specify the
virtual hardware in enough detail to keep it from changing when you
upgrade the hypervisor, so that certain proprietary operating systems
which are sensitive to this kind of thing continue to work.

Instead of specifying a controller name, you specify a controller
index 

[libvirt] [PATCH 2/2] Parallels: add connectBaselineCPU().

[Alexander Burluka]

From: A.Burluka aburl...@parallels.com

Openstack Nova (starting at
icehouse release) requires this function to start VM. Because the
information is taken from host capabilities xml, I've expanded it
and add cpu section in it.
---
 src/parallels/parallels_driver.c |   34 --
 1 files changed, 32 insertions(+), 2 deletions(-)

diff --git a/src/parallels/parallels_driver.c b/src/parallels/parallels_driver.c
index f1d5ecc..3aa73f0 100644
--- a/src/parallels/parallels_driver.c
+++ b/src/parallels/parallels_driver.c
@@ -51,6 +51,7 @@
 #include nodeinfo.h
 #include c-ctype.h
 #include virstring.h
+#include cpu/cpu.h
 
 #include parallels_driver.h
 #include parallels_utils.h
@@ -117,7 +118,9 @@ parallelsDomObjFreePrivate(void *p)
 static virCapsPtr
 parallelsBuildCapabilities(void)
 {
-virCapsPtr caps;
+virCapsPtr caps = NULL;
+virCPUDefPtr cpu = NULL;
+virCPUDataPtr data = NULL;
 virCapsGuestPtr guest;
 
 if ((caps = virCapabilitiesNew(virArchFromHost(),
@@ -147,11 +150,25 @@ parallelsBuildCapabilities(void)
   parallels, NULL, NULL, 0, NULL) == 
NULL)
 goto error;
 
+if (VIR_ALLOC(cpu)  0)
+goto error;
+
+cpu-arch = caps-host.arch;
+cpu-type = VIR_CPU_TYPE_HOST;
+caps-host.cpu = cpu;
+
+if (!(data = cpuNodeData(cpu-arch))
+|| cpuDecode(cpu, data, NULL, 0, NULL)  0) {
+goto error;
+}
+
+ cleanup:
+cpuDataFree(data);
 return caps;
 
  error:
 virObjectUnref(caps);
-return NULL;
+goto cleanup;
 }
 
 static char *
@@ -2312,6 +2329,18 @@ static int parallelsConnectIsAlive(virConnectPtr conn 
ATTRIBUTE_UNUSED)
 }
 
 
+static char *
+parallelsConnectBaselineCPU(virConnectPtr conn ATTRIBUTE_UNUSED,
+const char **xmlCPUs,
+unsigned int ncpus,
+unsigned int flags)
+{
+virCheckFlags(VIR_CONNECT_BASELINE_CPU_EXPAND_FEATURES, NULL);
+
+return cpuBaselineXML(xmlCPUs, ncpus, NULL, 0, flags);
+}
+
+
 static int
 parallelsNodeGetCpuMask(parallelsDomObjPtr privatedomdata,
 virBitmapPtr *cpumask,
@@ -2471,6 +2500,7 @@ static virDriver parallelsDriver = {
 .connectGetHostname = parallelsConnectGetHostname,  /* 0.10.0 */
 .nodeGetInfo = parallelsNodeGetInfo,  /* 0.10.0 */
 .connectGetCapabilities = parallelsConnectGetCapabilities,  /* 0.10.0 
*/
+.connectBaselineCPU = parallelsConnectBaselineCPU, /* 1.2.6 */
 .connectListDomains = parallelsConnectListDomains,  /* 0.10.0 */
 .connectNumOfDomains = parallelsConnectNumOfDomains,/* 0.10.0 */
 .connectListDefinedDomains = parallelsConnectListDefinedDomains,/* 
0.10.0 */
-- 
1.7.1

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

[libvirt] parallels: add 2 functions needed by OpenStack

[Alexander Burluka]

Hello,

I would like to apologize my for my inattention, that led to
unformatted subjects. I'll try to prevent this in the future.


--
Regards,
Alexander Burluka

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

[libvirt] [PATCH 1/2] Parallels: add domainGetVcpus().

[Alexander Burluka]

From: A.Burluka aburl...@parallels.com

OpenStack Nova requires this function
to start VM instance. Cpumask info is obtained via prlctl utility.
Unlike KVM, Parallels Cloud Server is unable to set cpu affinity
mask for every VCpu. Mask is unique for all VCpu. You can set it
using 'prlctl set vm_id|vm_name --cpumask {n[,n,n1-n2]|all}'
command. For example, 'prlctl set SomeDomain --cpumask 0,1,5-7'
would set this mask to yy---yyy.
---
 src/parallels/parallels_driver.c |  169 +-
 src/parallels/parallels_utils.h  |1 +
 2 files changed, 167 insertions(+), 3 deletions(-)

diff --git a/src/parallels/parallels_driver.c b/src/parallels/parallels_driver.c
index ab59599..f1d5ecc 100644
--- a/src/parallels/parallels_driver.c
+++ b/src/parallels/parallels_driver.c
@@ -108,6 +108,7 @@ parallelsDomObjFreePrivate(void *p)
 if (!pdom)
 return;
 
+VIR_FREE(pdom-cpumask);
 VIR_FREE(pdom-uuid);
 VIR_FREE(pdom-home);
 VIR_FREE(p);
@@ -654,6 +655,9 @@ parallelsLoadDomain(parallelsConnPtr privconn, 
virJSONValuePtr jobj)
 if (VIR_ALLOC(def)  0)
 goto cleanup;
 
+if (VIR_ALLOC(pdom)  0)
+goto cleanup;
+
 def-virtType = VIR_DOMAIN_VIRT_PARALLELS;
 def-id = -1;
 
@@ -716,6 +720,17 @@ parallelsLoadDomain(parallelsConnPtr privconn, 
virJSONValuePtr jobj)
 goto cleanup;
 }
 
+if (!(tmp = virJSONValueObjectGetString(jobj3, mask))) {
+/* Absence of this field means that all domains cpus are available */
+if (VIR_STRDUP(pdom-cpumask, all)  0) {
+goto cleanup;
+}
+} else {
+if (VIR_STRDUP(pdom-cpumask, tmp)  0) {
+goto cleanup;
+}
+}
+
 if (!(jobj3 = virJSONValueObjectGet(jobj2, memory))) {
 parallelsParseError();
 goto cleanup;
@@ -757,9 +772,6 @@ parallelsLoadDomain(parallelsConnPtr privconn, 
virJSONValuePtr jobj)
 
 def-os.arch = VIR_ARCH_X86_64;
 
-if (VIR_ALLOC(pdom)  0)
-goto cleanup;
-
 if (virJSONValueObjectGetNumberUint(jobj, EnvID, x)  0)
 goto cleanup;
 pdom-id = x;
@@ -2300,6 +2312,156 @@ static int parallelsConnectIsAlive(virConnectPtr conn 
ATTRIBUTE_UNUSED)
 }
 
 
+static int
+parallelsNodeGetCpuMask(parallelsDomObjPtr privatedomdata,
+virBitmapPtr *cpumask,
+int hostcpus)
+{
+int ret = -1;
+int cpunum = -1;
+int prevcpunum = -1;
+int offset = 0;
+const char *it = privatedomdata-cpumask;
+bool isrange = false;
+size_t i;
+int cpunums[512] = { 0 };
+size_t count = 0;
+
+if (STREQ(it, all)) {
+if (!(*cpumask = virBitmapNew(hostcpus)))
+goto cleanup;
+virBitmapSetAll(*cpumask);
+} else {
+while (sscanf(it, %d%n, cpunum, offset)) {
+char delim = 0;
+if (isrange) {
+for (i = prevcpunum + 1; i = cpunum; ++i) {
+cpunums[count++] = i;
+}
+} else {
+cpunums[count++] = cpunum;
+}
+
+it += offset;
+
+if (sscanf(it, %c%n, delim, offset) == EOF) {
+break;
+} else {
+it += offset;
+switch (delim) {
+case ',':
+isrange = false;
+break;
+case '-':
+isrange = true;
+prevcpunum = cpunum;
+break;
+default:
+virReportError(VIR_ERR_INVALID_ARG,
+   _(Invalid cpumask format '%s'),
+   privatedomdata-cpumask);
+goto cleanup;
+break;
+}
+}
+}
+if (!(*cpumask = virBitmapNew(cpunums[count-1] + 1)))
+goto cleanup;
+virBitmapClearAll(*cpumask);
+for (i = 0; i  count; ++i) {
+if (virBitmapSetBit(*cpumask, cpunums[i]) == -1) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _(cannot set %d bit in cpumask),
+   cpunums[i]);
+goto cleanup;
+}
+}
+}
+
+return 0;
+ cleanup:
+virBitmapFree(*cpumask);
+return ret;
+}
+
+static int
+parallelsDomainGetVcpus(virDomainPtr domain,
+   virVcpuInfoPtr info,
+   int maxinfo,
+   unsigned char *cpumaps,
+   int maplen)
+{
+parallelsConnPtr privconn = domain-conn-privateData;
+parallelsDomObjPtr privdomdata = NULL;
+virDomainObjPtr privdom = NULL;
+size_t i;
+int v, maxcpu, hostcpus;
+int ret = -1;
+
+parallelsDriverLock(privconn);
+privdom = virDomainObjListFindByUUID(privconn-domains, domain-uuid);
+parallelsDriverUnlock(privconn);
+
+if (privdom == NULL) {
+

[libvirt] [libvirt-glib] [PATCH v3] GVirDomainSnapshot: Add gvir_domain_snapshot_delete

[Timm Bäder]

---
 libvirt-gobject/libvirt-gobject-domain-snapshot.c | 29 +++
 libvirt-gobject/libvirt-gobject-domain-snapshot.h | 16 +
 libvirt-gobject/libvirt-gobject.sym   |  6 +
 3 files changed, 51 insertions(+)

diff --git a/libvirt-gobject/libvirt-gobject-domain-snapshot.c 
b/libvirt-gobject/libvirt-gobject-domain-snapshot.c
index fcf70ed..f835b58 100644
--- a/libvirt-gobject/libvirt-gobject-domain-snapshot.c
+++ b/libvirt-gobject/libvirt-gobject-domain-snapshot.c
@@ -205,3 +205,32 @@ GVirConfigDomainSnapshot *gvir_domain_snapshot_get_config
 free(xml);
 return conf;
 }
+
+/**
+ * gvir_domain_snapshot_delete:
+ * @snapshot: The domain snapshot
+ * @flags: Bitwise or of #GVirDomainSnapshotDeleteFlags
+ * @error: (allow-none): Place-holder for error or NULL
+ *
+ * Returns: TRUE on success, FALSE otherwise
+ */
+gboolean gvir_domain_snapshot_delete (GVirDomainSnapshot *snapshot,
+  guint flags,
+  GError **error)
+{
+GVirDomainSnapshotPrivate *priv;
+int status;
+
+g_return_if_fail(GVIR_IS_DOMAIN_SNAPSHOT (snapshot));
+g_return_if_fail(error == NULL || *error == NULL);
+
+priv = snapshot-priv;
+status = virDomainSnapshotDelete(priv-handle, flags);
+if (status  0) {
+gvir_set_error(error, GVIR_DOMAIN_SNAPSHOT_ERROR, 0,
+   Unable to delete snapshot `%s',
+   gvir_domain_snapshot_get_name(snapshot));
+return FALSE;
+}
+return TRUE;
+}
diff --git a/libvirt-gobject/libvirt-gobject-domain-snapshot.h 
b/libvirt-gobject/libvirt-gobject-domain-snapshot.h
index 5bd827c..b3ebe7f 100644
--- a/libvirt-gobject/libvirt-gobject-domain-snapshot.h
+++ b/libvirt-gobject/libvirt-gobject-domain-snapshot.h
@@ -58,6 +58,18 @@ struct _GVirDomainSnapshotClass
 gpointer padding[20];
 };
 
+/**
+ * GVirDomainSnapshotDeleteFlags:
+ * @GVIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN: Also delete children
+ * @GVIR_DOMAIN_SNAPSHOT_DELETE_METADATA_ONLY: Delete just metadata
+ * @GVIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY: Delete just children
+ */
+typedef enum {
+  GVIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN = 1,
+  GVIR_DOMAIN_SNAPSHOT_DELETE_METADATA_ONLY = 2,
+  GVIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY = 4
+} GVirDomainSnapshotDeleteFlags;
+
 
 GType gvir_domain_snapshot_get_type(void);
 GType gvir_domain_snapshot_handle_get_type(void);
@@ -69,6 +81,10 @@ GVirConfigDomainSnapshot *gvir_domain_snapshot_get_config
  guint flags,
  GError **err);
 
+gboolean gvir_domain_snapshot_delete (GVirDomainSnapshot *snapshot,
+  guint flags,
+  GError **error);
+
 G_END_DECLS
 
 #endif /* __LIBVIRT_GOBJECT_DOMAIN_SNAPSHOT_H__ */
diff --git a/libvirt-gobject/libvirt-gobject.sym 
b/libvirt-gobject/libvirt-gobject.sym
index f2419ac..232e63b 100644
--- a/libvirt-gobject/libvirt-gobject.sym
+++ b/libvirt-gobject/libvirt-gobject.sym
@@ -234,4 +234,10 @@ LIBVIRT_GOBJECT_0.1.5 {
gvir_connection_open_read_only_finish;
 } LIBVIRT_GOBJECT_0.1.4;
 
+LIBVIRT_GOBJECT_0.1.9 {
+  global:
+  gvir_domain_snapshot_delete_flags_get_type;
+  gvir_domain_snapshot_delete;
+} LIBVIRT_GOBJECT_0.1.5;
+
 #  define new API here using predicted next version number 
-- 
2.0.0

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

Re: [libvirt] Bug: iohelper drops I/O error messages

[Jason J. Herne]

On 05/20/2014 02:08 PM, Jason J. Herne wrote:

During a recent managed save operation I received the following error
message:

 error: operation failed: domain save job: unexpectedly failed.

It turns out that I had run out of disk space. After a brief
investigation I
discovered that libvirt_iohelper is exec'ed and is used to handle all
I/O during
a (Qemu) managed save operation. While iohelper appears to be set up to log
error conditions when they occur, for some reason the logging is getting
lost.
I'm hoping someone can help figure out why these errors are getting
lost. It
would be nice to present a useful error message to the user when a
managed save
fails because of an I/O error.



Ping? Anyone have any input on this? Not sure who the maintainer of 
iohelper is.


--
-- Jason J. Herne (jjhe...@linux.vnet.ibm.com)

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

[libvirt] [PATCH] leaseshelper: Include locale.h

[Michal Privoznik]

The commit baafe668 introduces this new helper utility. As in all
our programs, there's setlocale() called at the beginning of
main().  However, this source file don't include locale.h file
which causes build errors on some systems (e.g. mine).

Signed-off-by: Michal Privoznik mpriv...@redhat.com
---
 src/network/leaseshelper.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/network/leaseshelper.c b/src/network/leaseshelper.c
index e53a73c..eb695e4 100644
--- a/src/network/leaseshelper.c
+++ b/src/network/leaseshelper.c
@@ -28,6 +28,7 @@
 #include stdio.h
 #include stdlib.h
 #include sys/stat.h
+#include locale.h
 
 #include virutil.h
 #include virthread.h
-- 
2.0.0

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

Re: [libvirt] [PATCH 0/2] qemu: Process DEVICE_DELETED event in a separate thread

[Jiri Denemark]

On Mon, May 26, 2014 at 17:30:19 +0200, Jiri Denemark wrote:
 Jiri Denemark (2):
   qemu: Finish device removal in the original thread
   qemu: Process DEVICE_DELETED event in a separate thread
 
  src/qemu/qemu_domain.h  |  2 ++
  src/qemu/qemu_driver.c  | 48 ++--
  src/qemu/qemu_hotplug.c | 44 +---
  src/qemu/qemu_hotplug.h |  2 +-
  src/qemu/qemu_process.c | 30 +-
  5 files changed, 103 insertions(+), 23 deletions(-)

Pushed.

Jirka

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

Re: [libvirt] [PATCH 1/4] qemu: Unref cfg when detaching hostdev interface

[Jiri Denemark]

On Mon, Jun 02, 2014 at 14:00:57 +0200, Peter Krempa wrote:
 On 05/27/14 16:53, Jiri Denemark wrote:
  Signed-off-by: Jiri Denemark jdene...@redhat.com
  ---
   src/qemu/qemu_hotplug.c | 4 +++-
   1 file changed, 3 insertions(+), 1 deletion(-)
 
 ACK

Pushed, thanks.

Jirka

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

[libvirt] [PATCH] Fix build on freebsd

[Pavel Hrdina]

On freebsd there isn't known setlocale so we have to include locale.h

Signed-off-by: Pavel Hrdina phrd...@redhat.com
---

Pushed under trivial rule

 src/network/leaseshelper.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/network/leaseshelper.c b/src/network/leaseshelper.c
index e53a73c..b6f6c32 100644
--- a/src/network/leaseshelper.c
+++ b/src/network/leaseshelper.c
@@ -25,6 +25,7 @@
 
 #include config.h
 
+#include locale.h
 #include stdio.h
 #include stdlib.h
 #include sys/stat.h
-- 
1.8.5.5

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

[libvirt] [PATCH] virsh-nodedev: Avoid spurious errors

[Michal Privoznik]

Our public free functions explicitly don't accept NULL pointers
(sigh). Therefore, callers must do something like this:

if (dev)
virNodeDeviceFree(dev);

And we are not doing that on two places I've found. This leads to
dummy error message thrown by virsh:

virsh # nodedev-dumpxml nonexistent-device
error: Could not find matching device 'nonexistent-device'
error: invalid node device pointer in virNodeDeviceFree

Signed-off-by: Michal Privoznik mpriv...@redhat.com
---
 tools/virsh-nodedev.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/tools/virsh-nodedev.c b/tools/virsh-nodedev.c
index a35387a..46e0045 100644
--- a/tools/virsh-nodedev.c
+++ b/tools/virsh-nodedev.c
@@ -162,7 +162,8 @@ cmdNodeDeviceDestroy(vshControl *ctl, const vshCmd *cmd)
 ret = true;
  cleanup:
 virStringFreeList(arr);
-virNodeDeviceFree(dev);
+if (dev)
+virNodeDeviceFree(dev);
 return ret;
 }
 
@@ -571,7 +572,8 @@ cmdNodeDeviceDumpXML(vshControl *ctl, const vshCmd *cmd)
  cleanup:
 virStringFreeList(arr);
 VIR_FREE(xml);
-virNodeDeviceFree(device);
+if (device)
+virNodeDeviceFree(device);
 return ret;
 }
 
-- 
2.0.0

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

[libvirt] ANNOUNCE: Perl binding Sys-Virt release 1.2.5

[Daniel P. Berrange]

I am pleased to announce that release 1.2.5 of Sys-Virt, the libvirt Perl
API binding, is now available for download:

  http://search.cpan.org/CPAN/authors/id/D/DA/DANBERR/Sys-Virt-1.2.5.tar.gz

Changed in the 1.2.5 release:

 - Add VIR_DOMAIN_{REBOOT,SHUTDOWN}_PARAVIRT constants
 - Add virDomainFSFreeze/virDomainFSThaw APIs
 - Add virDomainSetTime/virDomainGetTime APIs

Further information, including online API documentation  previous release
archives, is available from the CPAN home page

   http://search.cpan.org/dist/Sys-Virt/

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] leaseshelper: Include locale.h

[Daniel P. Berrange]

On Mon, Jun 02, 2014 at 04:46:25PM +0200, Michal Privoznik wrote:
 The commit baafe668 introduces this new helper utility. As in all
 our programs, there's setlocale() called at the beginning of
 main().  However, this source file don't include locale.h file
 which causes build errors on some systems (e.g. mine).
 
 Signed-off-by: Michal Privoznik mpriv...@redhat.com
 ---
  src/network/leaseshelper.c | 1 +
  1 file changed, 1 insertion(+)

ACK as a build-breaker fix

Since we clearly keep making this mistake over  over, do you
fancy adding a syntax-check rule which looks for any file which
calls 'setlocale' but does not include 'locale.h' ?

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] [PATCHv2 0/4] Another attempt at improving virsh autocompletion

[Eric Blake]

On 04/01/2014 07:34 PM, Solly Ross wrote:
 Version 2: Electric Boogaloo
 Version 1: 
 https://www.redhat.com/archives/libvir-list/2014-March/msg01898.html
 
 This version of the patch introduces the following new things:
 
 - Tests (a whole bunch of them, in fact)!
 

Apologies for letting this one fall off my radar; does it still apply to
the latest libvirt.git, or does it need rebasing?

 - A new `complete` command to run get newline-separated
   completion results from the command line
 
 - Support for completing partial quotes
   (e.g. `virsh complete fake-command ab \i `)

I'd love to get this in for 1.2.6, now that 1.2.5 is out the door.

-- 
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] virsh-nodedev: Avoid spurious errors

[Eric Blake]

On 06/02/2014 08:52 AM, Michal Privoznik wrote:
 Our public free functions explicitly don't accept NULL pointers
 (sigh). Therefore, callers must do something like this:
 
 if (dev)
 virNodeDeviceFree(dev);

Back-compat stinks - changing an error into silent success may break
older code that was relying on the error, so I'm not brave enough to
change the public API.

I wonder if it would be appropriate to annotate our public API with
non-NULL markers, so that at least the compiler could warn on calls to
the API with a NULL pointer.

 
 And we are not doing that on two places I've found. This leads to
 dummy error message thrown by virsh:
 
 virsh # nodedev-dumpxml nonexistent-device
 error: Could not find matching device 'nonexistent-device'
 error: invalid node device pointer in virNodeDeviceFree
 
 Signed-off-by: Michal Privoznik mpriv...@redhat.com
 ---
  tools/virsh-nodedev.c | 6 --
  1 file changed, 4 insertions(+), 2 deletions(-)

At any rate, whether or not we choose to make further changes to
libvirt.h, your patch is stand-alone correct. 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

[libvirt] [libvirt-php] libvirt_domain_new Create a slow vm

[Mario Di Ture]

Hi,

I've tested this script on centos 6.5 (libvirt-0.10.2, qemu-kvm-0.12.1.2)
and
ubuntu server 14.04 (libvirt 1.2.2, qemu-kvm  2.0.0) with the same result.
The created vm is very slow compared to the vm created with virt-manager,
with the same features/devices on the same host.
Can you suggest any configuration that can avoid that slowness?

With regards
Mario D.


?php
$conn = libvirt_connect('null', false);
$name=testvm;
$arch=x86_64;
$memMB=1024;
$maxmemMB=1536;
$vcpus=1;
$iso_image=/home/iso/CentOS-6.5-x86_64-minimal.iso;
$disk1 = array(
path   = /home/vm/centos-script.raw,
driver = raw,
bus= virtio,
dev= hda,
size   = 6G,
flags  = VIR_DOMAIN_DISK_FILE |
VIR_DOMAIN_DISK_ACCESS_ALL );

$disks = array( $disk1 );
$network1 = array(
'mac' = '00:11:22:33:44:55',
'network' = 'default',
'model'   = 'e1000'
);

$networks = array( $network1 );
$flags=VIR_DOMAIN_FLAG_FEATURE_ACPI;

$newdom=libvirt_domain_new($conn, $name, $arch, $memMB, $maxmemMB, $vcpus,
$iso_image, $disks, $networks, $flags);
print_r($newdom);
?
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] IPv6 in Libvirt LXC

[Thomas Maddox]

Hey all,

According to a discussion last week in the Nova-Libvirt subgroup meeting, it 
was advised, by danpb, that I bring this issue up on the Libvirt mailing list 
for discussion and resolution. So, here goes -

I'm currently using config drive from Nova to generate network configurations 
for LXC guests that are spun up via Libvirt. Unfortunately, when doing some 
IPv6 testing, I ran into a snag (with a couple work arounds detailed below). 
Due to the read-only mount of /proc/sys 
(http://libvirt.org/drvlxc.html#fsmounts), I am unable to get expected behavior 
from IPv6 static network configurations. I did some poking around and found 
this bug from a couple years ago that pretty well outlines the problem: 
https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/964882.

I wasn't sure how we might go about correcting this, but it seems like 
something we'll need to address in Libvirt. Maybe with the user namespaces 
working, we can begin to provide some read/write mounts instead of read-only 
with clear documentation on the security implications? =] When using static 
IPv6 addressing it was attempting the following command: 'sysctl -q -e -w 
net.ipv6.conf.eth0.autoconf=0'. I tested to see whether the host and the guest 
share this value. I was able to change it in the host without it being 
reflected in the guest.

The work arounds I've tried that seemed to allow IPv6 to get configured 
properly:

1. Use the post-up hook on an IPv4 static configuration to configure IPv6 via 
ifconfig/routes (example: http://paste.openstack.org/show/82446/).
2. Patch Libvirt to include a /proc/sys/net mount as read/write.

Cheers!

-Thomas

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

Re: [libvirt] [PATCH] Don't use AI_ADDRCONFIG when binding to wildcard addresses

[Ján Tomko]

On 05/30/2014 09:36 PM, Eric Blake wrote:
 On 05/30/2014 12:21 AM, Ján Tomko wrote:
 
 +if (nodename 
 +!(virSocketAddrParse(tmp_addr, nodename, AF_UNSPEC)  0 
 +  virSocketAddrIsWildcard(tmp_addr)))
 +hints.ai_flags = AI_ADDRCONFIG;

 Shouldn't this be |= ?


 Functionally it's the same, AI_PASSIVE is ignored if nodename is non-NULL.

 But it should be |= if we were using other flags.
 
 Okay.  ACK to the patch, and safe to include in 1.2.5.
 

Pushed with the |= change, although it's too late for 1.2.5 now.

Thanks for the review.

Jan




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 1/2] Parallels: add domainGetVcpus(). OpenStack Nova requires this function to start VM instance. Cpumask info is obtained via prlctl utility. Unlike KVM, Parallels Cloud Server i

[Eric Blake]

On 06/02/2014 07:40 AM, Alexander Burluka wrote:
 From: A.Burluka aburl...@parallels.com

Subject line is WAAAY too long.  You missed a blank line in between add
domainGetVcpus(). and the rest of your commit message.

 
 ---
  src/parallels/parallels_driver.c |  169 
 +-
  src/parallels/parallels_utils.h  |1 +
  2 files changed, 167 insertions(+), 3 deletions(-)
 
 diff --git a/src/parallels/parallels_driver.c 
 b/src/parallels/parallels_driver.c

  
 +static int
 +parallelsNodeGetCpuMask(parallelsDomObjPtr privatedomdata,
 +virBitmapPtr *cpumask,
 +int hostcpus)
 +{
 +int ret = -1;
 +int cpunum = -1;
 +int prevcpunum = -1;
 +int offset = 0;
 +const char *it = privatedomdata-cpumask;
 +bool isrange = false;
 +size_t i;
 +int cpunums[512] = { 0 };

Why a magic number?

 +size_t count = 0;
 +
 +if (STREQ(it, all)) {
 +if (!(*cpumask = virBitmapNew(hostcpus)))
 +goto cleanup;
 +virBitmapSetAll(*cpumask);
 +} else {
 +while (sscanf(it, %d%n, cpunum, offset)) {

sscanf(%d) is evil. It has undefined behavior on integer overflow, and
we have to assume that the input file that we are parsing could possibly
come from untrusted sources.  Please use virstring.h virStrToLong_i()
instead.


 +case ',':
 +isrange = false;
 +break;
 +case '-':
 +isrange = true;
 +prevcpunum = cpunum;
 +break;

Instead of open-coding your own bitmap parser, can you see if
virBitmapParse() can do the job?  If it can't, can it at least be
refactored into a common helper function with an additional parameter,
so that you can reuse that code?

-- 
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] [libvirt-php] libvirt_domain_new Create a slow vm

[Eric Blake]

On 06/02/2014 09:17 AM, Mario Di Ture wrote:
 Hi,
 
 I've tested this script on centos 6.5 (libvirt-0.10.2, qemu-kvm-0.12.1.2)
 and
 ubuntu server 14.04 (libvirt 1.2.2, qemu-kvm  2.0.0) with the same result.
 The created vm is very slow compared to the vm created with virt-manager,
 with the same features/devices on the same host.
 Can you suggest any configuration that can avoid that slowness?

Can you compare the output of 'virsh dumpxml' for the fast and the slow
domain? My guess is that you forgot to request KVM hardware acceleration
for the slow domain.  Look for 'domain type='kvm' as well as for the
right emulator binary.  'However, I'm not familiar enough with the
libvirt-php bindings to suggest what to add to your code to get the
correct XML.

-- 
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 1/3] virnuma.c: Fix some comments

[Daniel P. Berrange]

On Mon, Jun 02, 2014 at 02:15:58PM +0200, Michal Privoznik wrote:
 In 9dd02965 the virNumaGetNodeMemory was introduced, however the
 comment describing the function mentions virNumaGetNodeMemorySize.
 And there's one typo in virNumaIsAvailable() description.
 
 Signed-off-by: Michal Privoznik mpriv...@redhat.com
 ---
  src/util/virnuma.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)
 
 diff --git a/src/util/virnuma.c b/src/util/virnuma.c
 index bf3b9e6..1e099eb 100644
 --- a/src/util/virnuma.c
 +++ b/src/util/virnuma.c
 @@ -193,7 +193,7 @@ virNumaIsAvailable(void)
   * Get the highest node number available on the current system.
   * (See the node numbers in /sys/devices/system/node/ ).
   *
 - * Returns the highes NUMA node id on success, -1 on error.
 + * Returns the highest NUMA node id on success, -1 on error.
   */
  int
  virNumaGetMaxNode(void)
 @@ -217,7 +217,7 @@ virNumaGetMaxNode(void)
  
  
  /**
 - * virNumaGetNodeMemorySize:
 + * virNumaGetNodeMemory:
   * @node: identifier of the requested NUMA node
   * @memsize: returns the total size of memory in the NUMA node
   * @memfree: returns the total free memory in a NUMA node

ACK

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 3/3] virCaps: Expose distance between host NUMA nodes

[Daniel P. Berrange]

On Mon, Jun 02, 2014 at 02:16:00PM +0200, Michal Privoznik wrote:
 If user or management application wants to create a guest,
 it may be useful to know the cost of internode latencies
 before the guest resources are pinned. For example:
 
 capabilities
 
   host
 ...
 topology
   cells num='2'
 cell id='0'
   memory unit='KiB'4004132/memory
   cell id='0' distance='10'/
   cell id='1' distance='20'/

I'd be a little more comfortable if we didn't use a cell
within a cell. Perhaps lets use 'sibling' as the name
instead and group the elements. eg could we do

  distances
 sibling id=0 value=10/
 sibling id=1 value=20'/
  /distance

 /topology
 ...
   /host
   ...
 /capabilities
 
 we can see the distance from node1 to node0 is 20 and within nodes 10.

One thing with having the data under each cell is that we're
actually reporting twice as much as we need to. ie the distance
between cell N and M is reported under both N and M. A different
option would be todo reporting at the toplevel within topology
eg

 distances
siblings distance=10
   cell id=0/
   cell id=1/
/siblings
 /distance

I'm not sure whether doing this is worth while or not though ?

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 2/3] virnuma: Introduce virNumaGetDistances

[Daniel P. Berrange]

On Mon, Jun 02, 2014 at 02:15:59PM +0200, Michal Privoznik wrote:
 The API gets a NUMA node and find distances to other nodes.  The
 distances are returned in an array. If an item X within the array
 equals to value of zero, then there's no such node as X.
 
 Signed-off-by: Michal Privoznik mpriv...@redhat.com
 ---
  src/libvirt_private.syms |  1 +
  src/util/virnuma.c   | 50 
 
  src/util/virnuma.h   |  3 +++
  3 files changed, 54 insertions(+)
 
 diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
 index cb635cd..6168f76 100644
 --- a/src/libvirt_private.syms
 +++ b/src/libvirt_private.syms
 @@ -1652,6 +1652,7 @@ virNodeSuspendGetTargetMask;
  virDomainNumatuneMemModeTypeFromString;
  virDomainNumatuneMemModeTypeToString;
  virNumaGetAutoPlacementAdvice;
 +virNumaGetDistances;
  virNumaGetMaxNode;
  virNumaGetNodeMemory;
  virNumaIsAvailable;
 diff --git a/src/util/virnuma.c b/src/util/virnuma.c
 index 1e099eb..68b2698 100644
 --- a/src/util/virnuma.c
 +++ b/src/util/virnuma.c
 @@ -217,6 +217,56 @@ virNumaGetMaxNode(void)
  
  
  /**
 + * virNumaGetDistances:
 + * @node: identifier of the requested NUMA node
 + * @distances: array of distances to sibling nodes
 + * @ndistances: size of @distances
 + *
 + * Get array of distances to sibling nodes from @node. If a
 + * distances[x] equals to zero, the node x is not enabled or
 + * doesn't exist. As a special case, if @node itself refers to
 + * disabled or nonexistent NUMA node, then @distances and
 + * @ndistances are set to NULL and zero respectively.

I think it'd be worth stating what the distance is between
a node and itself, since that's another special case. I'd
assumed that the distince between a ndoe and itself would
be zero, but your next patch shows that it would be 10 which
I find a bit bizarre. Presumably that's just what numactl,
or perhaps the kernel, reports ?

If we are relying  on numactl's value ranges, then we should
be clear about this to help people porting to non-Linux
platforms.

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 2/3] virnuma: Introduce virNumaGetDistances

[Daniel P. Berrange]

On Mon, Jun 02, 2014 at 04:57:42PM +0100, Daniel P. Berrange wrote:
 On Mon, Jun 02, 2014 at 02:15:59PM +0200, Michal Privoznik wrote:
  The API gets a NUMA node and find distances to other nodes.  The
  distances are returned in an array. If an item X within the array
  equals to value of zero, then there's no such node as X.
  
  Signed-off-by: Michal Privoznik mpriv...@redhat.com
  ---
   src/libvirt_private.syms |  1 +
   src/util/virnuma.c   | 50 
  
   src/util/virnuma.h   |  3 +++
   3 files changed, 54 insertions(+)
  
  diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
  index cb635cd..6168f76 100644
  --- a/src/libvirt_private.syms
  +++ b/src/libvirt_private.syms
  @@ -1652,6 +1652,7 @@ virNodeSuspendGetTargetMask;
   virDomainNumatuneMemModeTypeFromString;
   virDomainNumatuneMemModeTypeToString;
   virNumaGetAutoPlacementAdvice;
  +virNumaGetDistances;
   virNumaGetMaxNode;
   virNumaGetNodeMemory;
   virNumaIsAvailable;
  diff --git a/src/util/virnuma.c b/src/util/virnuma.c
  index 1e099eb..68b2698 100644
  --- a/src/util/virnuma.c
  +++ b/src/util/virnuma.c
  @@ -217,6 +217,56 @@ virNumaGetMaxNode(void)
   
   
   /**
  + * virNumaGetDistances:
  + * @node: identifier of the requested NUMA node
  + * @distances: array of distances to sibling nodes
  + * @ndistances: size of @distances
  + *
  + * Get array of distances to sibling nodes from @node. If a
  + * distances[x] equals to zero, the node x is not enabled or
  + * doesn't exist. As a special case, if @node itself refers to
  + * disabled or nonexistent NUMA node, then @distances and
  + * @ndistances are set to NULL and zero respectively.
 
 I think it'd be worth stating what the distance is between
 a node and itself, since that's another special case. I'd
 assumed that the distince between a ndoe and itself would
 be zero, but your next patch shows that it would be 10 which
 I find a bit bizarre. Presumably that's just what numactl,
 or perhaps the kernel, reports ?
 
 If we are relying  on numactl's value ranges, then we should
 be clear about this to help people porting to non-Linux
 platforms.

Answering my own question these values come from the kernel
which in turn gets them from the BIOS SLIT tables. '10' is the
magic value for local distance and everything large is a rough
indication of memory access time penalty. So '20' means the
remote node memory access is x2 slower.  Since it is BIOS
defined, we should be fine using these values as-is in the
libvirt XML. Lets just document '10' as being the local node
value and that other values are a scale factor relative to
this.

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] virsh-nodedev: Avoid spurious errors

[Michal Privoznik]

On 02.06.2014 17:12, Eric Blake wrote:

On 06/02/2014 08:52 AM, Michal Privoznik wrote:

Our public free functions explicitly don't accept NULL pointers
(sigh). Therefore, callers must do something like this:

 if (dev)
 virNodeDeviceFree(dev);


Back-compat stinks - changing an error into silent success may break
older code that was relying on the error, so I'm not brave enough to
change the public API.


It's not our action rather than the code written that way that I'd call 
broken :)




I wonder if it would be appropriate to annotate our public API with
non-NULL markers, so that at least the compiler could warn on calls to
the API with a NULL pointer.



And we are not doing that on two places I've found. This leads to
dummy error message thrown by virsh:

 virsh # nodedev-dumpxml nonexistent-device
 error: Could not find matching device 'nonexistent-device'
 error: invalid node device pointer in virNodeDeviceFree

Signed-off-by: Michal Privoznik mpriv...@redhat.com
---
  tools/virsh-nodedev.c | 6 --
  1 file changed, 4 insertions(+), 2 deletions(-)


At any rate, whether or not we choose to make further changes to
libvirt.h, your patch is stand-alone correct. ACK.



Pushed, thanks.

Michal

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

Re: [libvirt] [libvirt-php] libvirt_domain_new Create a slow vm

[Mario Di Ture]

2014-06-02 17:35 GMT+02:00 Eric Blake ebl...@redhat.com:

 On 06/02/2014 09:17 AM, Mario Di Ture wrote:
  Hi,
 
  I've tested this script on centos 6.5 (libvirt-0.10.2, qemu-kvm-0.12.1.2)
  and
  ubuntu server 14.04 (libvirt 1.2.2, qemu-kvm  2.0.0) with the same
 result.
  The created vm is very slow compared to the vm created with virt-manager,
  with the same features/devices on the same host.
  Can you suggest any configuration that can avoid that slowness?

 Can you compare the output of 'virsh dumpxml' for the fast and the slow
 domain? My guess is that you forgot to request KVM hardware acceleration
 for the slow domain.  Look for 'domain type='kvm' as well as for the
 right emulator binary.  'However, I'm not familiar enough with the
 libvirt-php bindings to suggest what to add to your code to get the
 correct XML.



That's right Eric, the setting is domain type='kvm' on the fast, and domain
type='qemu' on the slow domain.
It seems that the parameter cannot be passed with the  libvirt_domain_new
function...looking for an alternative...
Thanks for the tip!
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] IPv6 in Libvirt LXC

[Daniel P. Berrange]

On Mon, Jun 02, 2014 at 03:09:08PM +, Thomas Maddox wrote:
 Hey all,
 
 According to a discussion last week in the Nova-Libvirt subgroup meeting,
 it was advised, by danpb, that I bring this issue up on the Libvirt mailing
 list for discussion and resolution. So, here goes -
 
 I'm currently using config drive from Nova to generate network configurations
 for LXC guests that are spun up via Libvirt. Unfortunately, when doing some
 IPv6 testing, I ran into a snag (with a couple work arounds detailed below).
 Due to the read-only mount of /proc/sys 
 (http://libvirt.org/drvlxc.html#fsmounts),
 I am unable to get expected behavior from IPv6 static network configurations.
 I did some poking around and found this bug from a couple years ago that 
 pretty
 well outlines the problem:

 https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/964882.
 
 I wasn't sure how we might go about correcting this, but it seems like 
 something
 we'll need to address in Libvirt. Maybe with the user namespaces working, we 
 can
 begin to provide some read/write mounts instead of read-only with clear
 documentation on the security implications? =] When using static IPv6 
 addressing
 it was attempting the following command: 'sysctl -q -e -w 
 net.ipv6.conf.eth0.autoconf=0'.
 I tested to see whether the host and the guest share this value. I was able to
 change it in the host without it being reflected in the guest.

That sounds promising. I guess we need to check whether that isolation applies
to every tunable underneath net.* or just the tunables that are tied to specific
network interfaces. eg net.*.eth0.*  Hopefully it would be the former, but I'm
afraid it could well be the latter

 The work arounds I've tried that seemed to allow IPv6 to get configured 
 properly:
 
 1. Use the post-up hook on an IPv4 static configuration to configure IPv6 via
ifconfig/routes (example: http://paste.openstack.org/show/82446/).
 2. Patch Libvirt to include a /proc/sys/net mount as read/write.

This would be reasonable todo, assuming the entire subtree of
/proc/sys/net was actually isolated from the host by the network
namespace.

The remounting of /proc/sys readonly is a psuedo-security measure. In the
absence of user namespaces it does not actually protect against a malicious
user with root in the container, since they can just re-mount it read-write
again. So when user namespaces are *not* active, we could easily just make
/proc/sys/net (or even the entire of /proc/sys) read-write, without lowering
the real security protection.

When user namesplaces *are* active, the remapping of UID/GID==0  to a
non-0 value will prevent the root user in the container from changing
anything in /proc/sys even if we have it entirely read-write. So merely
changing /proc/sys/net to read-write won't fix your problem when user
namespaces are active.

IIUC, we'd need to recursively chown the files under /proc/sys/net to
give them the remapped UID/GID of the root user in the container, in
order that they can be used.

So overall I think we'd have to do

 - Make either /proc/sys/net or /proc/sys read-write

 - If userns is active, recursive chown /proc/sys/net (or a subset of
   files in it that we explicitly want to grant access to)

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] Is there a way to get host uptime in remote libvirt

[Daniel P. Berrange]

On Wed, May 14, 2014 at 08:29:30AM +0800, coffeeball wrote:
 We manage hypervisors (VMWare ESXi/vCenter, KVM, XEN, Hyper-V) by remote
 Libvirt API, in our case we need to get the host uptime via the same 
 libvirt interface. Is there a way get this info now for all the
 aforementioned hypervisor types?
 
 The APIs virConnectGetSysinfo(), virNodeGetInfo() provide host info but
 it doesn't include the system uptime.

Afraid we don't have any reporting of the uptime yet - feel free to
report a bug against libvirt asking for this, or if you are C coder
we'd accept patches too.

 The virNodeGetCPUStats() can returns CPU usage in nanosecond, can we add
 the user + system + idle + iowait to calculate the system uptime? Looks
 like the sum value has a huge gap with the real uptime value returned
 by uptime CLI.

Yeah, I'm not sure that's going to be reliable.

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] [Question] Collect vNode pinning to pNode run-time information

[Daniel P. Berrange]

On Fri, May 16, 2014 at 03:01:14AM +, Shi, Xiao-Lei (Bruce, HP 
Servers-PSC-CQ) wrote:
 Hi all,
 
 I have a question about NUMA.
 User configured vNode(guest virtual numa node), but he didn't
 configure cputune and numatune. Now we want to get the information
 that each vNode run in which pNode(host numa node). It's run-time
 information that may be modified with high frequency.
 In current Libvirt's API, we can get the information that each vCpu
 running on which pCpu through virsh vcpuinfo(there should be a
 corresponding Libvirt API function). But we didn't find any APIs to
 get the information that each vNode uses which pNode's memory, or
 just each vCpu consumes which pNode's memory.

Yes, you are correct - the libvirt APIs only provide a way to figure
out the vCPU-pCPU placement, nothing about memory. 

 We find a command numastat -mcn -p qemu that can get the memory
 consume data of each VM, but it still loses the information that
 we want(vNode memory consume data), as following:
 # numastat -mcn -p qemu
 
 Per-node process memory usage (in MBs)
 PID  Node 0 Node 1 Total
 ---  -- -- -
 8900 (qemu-kvm)2032 50  2083
 17716 (qemu-kvm)   1546663  2209
 22484 (qemu-kvm)621   1524  2146
 29694 (qemu-kvm)892   1350  2242
 ---  -- -- -
 Total  5092   3588  8680
 .
 
 My question is:
 
 1.   In Libvirt, are there any ways that we can get our
  needed data?

Not at this point in time.

 
 2.   If no ways in Libvirt, do you have any other suggestions
  to collect the information?

I don't believe there is any easy way. The 'numastat' command
can only see things at process-level granularity - it has no
way of knowing about the fact that the KVM process has two
virtual guest NUMA nodes.

With current QEMU there's not even any way for libvirt to
know where guest NUMA node memory is allocated from. There
is working taking place in QEMU to make it possible to
associated guest NUMA nodes with host NUMA nodes. This only
helps if the guest / host nodes are specified though.

If you are letting the guest NUMA nodes float across any
host NUMA node, I'm not sure that KVM will provide us enough
info to determine what was allocated from where. You might
want to send this query to the qemu-devel mailing list
instead to see if they have better suggestions

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] [PATCHv3] Add invariant TSC cpu flag

[Daniel P. Berrange]

On Thu, May 15, 2014 at 10:31:05AM +0200, Ján Tomko wrote:
 diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
 index f0df1a6..7504a38 100644
 --- a/src/qemu/qemu_migration.c
 +++ b/src/qemu/qemu_migration.c
 @@ -1513,6 +1513,20 @@ qemuMigrationIsAllowed(virQEMUDriverPtr driver, 
 virDomainObjPtr vm,
  return false;
  }
  
 +for (i = 0; i  def-cpu-nfeatures; i++) {
 +virCPUFeatureDefPtr feature = def-cpu-features[i];
 +
 +if (feature-policy != VIR_CPU_FEATURE_REQUIRE)
 +continue;
 +
 +if (STREQ(feature-name, invtsc)) {
 +virReportError(VIR_ERR_OPERATION_INVALID,
 +   _(domain has CPU feature: %s),
 +   feature-name);
 +return false;
 +}
 +}

Could you add a comment describing why we forbid migration with
this feature set. It probably isn't obvious to some random person
reading this in the future :-)

ACK

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 1/2] Parallels: add domainGetVcpus().

[Daniel P. Berrange]

On Mon, Jun 02, 2014 at 06:06:38PM +0400, Alexander Burluka wrote:
 From: A.Burluka aburl...@parallels.com
 
 OpenStack Nova requires this function
 to start VM instance. Cpumask info is obtained via prlctl utility.

Hmm, what error did you get from openstack ?  The two uses of the
'dom.vcpus' function are both wrapped in try/except so that it is
considered non-fatal if libvirt doesn't provide this.

 Unlike KVM, Parallels Cloud Server is unable to set cpu affinity
 mask for every VCpu. Mask is unique for all VCpu. You can set it
 using 'prlctl set vm_id|vm_name --cpumask {n[,n,n1-n2]|all}'
 command. For example, 'prlctl set SomeDomain --cpumask 0,1,5-7'
 would set this mask to yy---yyy.

Are you talking about container based virt here or full machine
based virt ? IIUC Parallels can do both ?  With container based
virt, does parallels have the concept of 'vcpus' at all ? We
don't have that in LXC at least.

I don't think it makes sense to support the virDomainGetVCPUs
function if this is only about container virt. I'd be more
inclined to fix openstack so it doesn't fail

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

[libvirt] [PATCH] build: fix race in vapi/ subdirectory

[Michael Catanzaro]

libvirt-gobject-1.0.vapi depends on libvirt-gconfig-1.0.vapi
---
 vapi/Makefile.am | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/vapi/Makefile.am b/vapi/Makefile.am
index 0154104..af10e98 100644
--- a/vapi/Makefile.am
+++ b/vapi/Makefile.am
@@ -17,7 +17,7 @@ libvirt-glib-1.0.vapi:
$(top_builddir)/libvirt-glib/LibvirtGLib-1.0.gir
--library libvirt-glib-1.0  \
$
 
-libvirt-gobject-1.0.vapi:
$(top_builddir)/libvirt-gobject/LibvirtGObject-1.0.gir
libvirt-glib-1.0.vapi
+libvirt-gobject-1.0.vapi:
$(top_builddir)/libvirt-gobject/LibvirtGObject-1.0.gir
libvirt-glib-1.0.vapi libvirt-gconfig-1.0.vapi
$(AM_V_GEN)$(VAPIGEN)   
 \
--vapidir=$(builddir)   
 \
--pkg gobject-2.0   
 \
-- 
1.9.3

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

Re: [libvirt] [PATCHv3] Add invariant TSC cpu flag

[Eric Blake]

On 05/15/2014 02:31 AM, Ján Tomko wrote:
 Add suport for invariant TSC flag (CPUID 0x8007, bit 8 of EDX).
 If this flag is enabled, the TSC ticks at a constant rate across
 all ACPI P-, C- and T-states.
 
 This can be enabled by adding:
 feature name='invtsc'/
 to the cpu element.
 
 Migration and saving the domain does not work with this flag.
 
 QEMU support for this is not merged yet:
 https://lists.gnu.org/archive/html/qemu-devel/2014-04/msg05024.html

In spite of Dan's ack, let's wait until it actually lands in qemu.git
before you push this.

-- 
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] [PATCHv3 11/36] storage: Change to new backing store parser

[Eric Blake]

On 05/30/2014 02:37 AM, Peter Krempa wrote:
 Use the new backing store parser in the backing chain crawler. This
 change needs one test change where information about the NBD image are
 now parsed differently.
 ---
  src/storage/storage_driver.c | 90 
 +---
  tests/virstoragetest.c   | 14 ---
  2 files changed, 9 insertions(+), 95 deletions(-)

Nice diffstat!

 
 +++ b/tests/virstoragetest.c
 @@ -730,20 +730,22 @@ mymain(void)
  /* Rewrite qcow2 to use an nbd: protocol as backend */
  virCommandFree(cmd);
  cmd = virCommandNewArgList(qemuimg, rebase, -u, -f, qcow2,
 -   -F, raw, -b, nbd:example.org:6000,
 +   -F, raw, -b, 
 nbd:example.org:6000:exportname=blah,

Nice increased test coverage.  I'm glad I spent the time on the
testsuite, it makes changes like this more reassuring.

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 1/3] cpu: use typedefs for enums in src/cpu/cpu_map.h

[Eric Blake]

On 05/31/2014 06:22 PM, Julio Faracco wrote:
 In src/cpu/ there are some enumerations (enum) declarations.
 Similar to the recent cleanup to src/util, src/conf and other
 directories, it's better to use a typedef for variable types,
 function types and other usages. Other enumeration and folders will
 be changed to typedef's in the future. Specially, in files that are
 in different places of src/util and src/conf. Most of the files
 changed in this commit are related to CPU (cpu_map.h) enums.
 
 Signed-off-by: Julio Faracco jcfara...@gmail.com
 ---
  src/cpu/cpu.c |2 +-
  src/cpu/cpu_map.c |2 +-
  src/cpu/cpu_map.h |6 +++---
  src/cpu/cpu_powerpc.c |2 +-
  src/cpu/cpu_x86.c |2 +-
  5 files changed, 7 insertions(+), 7 deletions(-)

ACK and pushed.

-- 
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 2/3] conf: enum cleanups in src/conf/domain_conf.h

[Eric Blake]

On 05/31/2014 06:22 PM, Julio Faracco wrote:
 In src/conf/domain_conf.h there are many enumerations (enum)
 declarations to be converted as a typedef too. As mentioned before,
 it's better to use a typedef for variable types, function types and
 other usages. I think this file has most of those enum declarations
 at src/conf/. So, me and Eric Blake plan to keep the cleanups all
 over the source code. This time, most of the files changed in this
 commit are related to part of one file: src/conf/domain_conf.h.
 
 Signed-off-by: Julio Faracco jcfara...@gmail.com
 ---
  src/conf/domain_conf.c  |4 +--
  src/conf/domain_conf.h  |   84 
 +--
  src/qemu/qemu_command.c |4 +--
  src/qemu/qemu_domain.c  |2 +-
  src/qemu/qemu_hotplug.c |2 +-
  src/security/security_dac.c |4 +--
  6 files changed, 50 insertions(+), 50 deletions(-)
 

ACK and pushed.

-- 
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 3/3] conf: more enum cleanups in src/conf/domain_conf.h

[Eric Blake]

On 05/31/2014 06:22 PM, Julio Faracco wrote:
 In src/conf/domain_conf.h there are many enum declarations. The
 cleanup in this header filer was started, but it wasn't enough and
 there are many other files that has enum variables declared. So, the
 commit was starting to be big. This commit finish the cleanup in this
 header file and in other files that has enum variables, parameters,
 or functions declared.
 
 Signed-off-by: Julio Faracco jcfara...@gmail.com
 ---
  src/conf/domain_audit.c  |6 +-
  src/conf/domain_conf.c   |   46 +++
  src/conf/domain_conf.h   |  274 
 +-
  src/network/bridge_driver.c  |6 +-
  src/qemu/qemu_command.c  |   24 ++--

Merge conflict here based on changes in the meantime, but trivial enough
to resolve.

  src/qemu/qemu_domain.c   |2 +-
  src/qemu/qemu_domain.h   |2 +-
  src/qemu/qemu_hotplug.c  |2 +-
  src/qemu/qemu_monitor.c  |2 +-
  src/qemu/qemu_monitor.h  |2 +-
  src/qemu/qemu_monitor_json.c |4 +-
  src/qemu/qemu_monitor_json.h |2 +-
  src/qemu/qemu_monitor_text.c |2 +-
  src/qemu/qemu_monitor_text.h |2 +-
  src/security/security_dac.c  |4 +-
  15 files changed, 190 insertions(+), 190 deletions(-)

You missed changes required in libxl/libxl_domain.c.

ACK with fixes, and pushed.

diff --git i/src/libxl/libxl_domain.c w/src/libxl/libxl_domain.c
index 80d5280..9743c85 100644
--- i/src/libxl/libxl_domain.c
+++ w/src/libxl/libxl_domain.c
@@ -526,7 +526,7 @@ libxlDomainShutdownThread(void *opaque)
 dom_event = virDomainEventLifecycleNewFromObj(vm,
VIR_DOMAIN_EVENT_STOPPED,

VIR_DOMAIN_EVENT_STOPPED_SHUTDOWN);
-switch ((enum virDomainLifecycleAction) vm-def-onPoweroff) {
+switch ((virDomainLifecycleAction) vm-def-onPoweroff) {
 case VIR_DOMAIN_LIFECYCLE_DESTROY:
 reason = VIR_DOMAIN_SHUTOFF_SHUTDOWN;
 goto destroy;
@@ -541,7 +541,7 @@ libxlDomainShutdownThread(void *opaque)
 dom_event = virDomainEventLifecycleNewFromObj(vm,
VIR_DOMAIN_EVENT_STOPPED,

VIR_DOMAIN_EVENT_STOPPED_CRASHED);
-switch ((enum virDomainLifecycleCrashAction) vm-def-onCrash) {
+switch ((virDomainLifecycleCrashAction) vm-def-onCrash) {
 case VIR_DOMAIN_LIFECYCLE_CRASH_DESTROY:
 reason = VIR_DOMAIN_SHUTOFF_CRASHED;
 goto destroy;
@@ -562,7 +562,7 @@ libxlDomainShutdownThread(void *opaque)
 dom_event = virDomainEventLifecycleNewFromObj(vm,
VIR_DOMAIN_EVENT_STOPPED,

VIR_DOMAIN_EVENT_STOPPED_SHUTDOWN);
-switch ((enum virDomainLifecycleAction) vm-def-onReboot) {
+switch ((virDomainLifecycleAction) vm-def-onReboot) {
 case VIR_DOMAIN_LIFECYCLE_DESTROY:
 reason = VIR_DOMAIN_SHUTOFF_SHUTDOWN;
 goto destroy;


-- 
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] [PATCHv3 12/36] storage: Traverse backing chains of network disks

[Eric Blake]

On 05/30/2014 02:37 AM, Peter Krempa wrote:
 Now we don't need to skip backing chain detection for remote disks.
 ---
  src/qemu/qemu_domain.c   |  8 +++-
  src/storage/storage_driver.c | 18 ++
  2 files changed, 9 insertions(+), 17 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] [PATCHv3 13/36] util: string: Return element count from virStringSplit

[Eric Blake]

On 05/30/2014 02:37 AM, Peter Krempa wrote:
 To allow using the array manipulation macros on the arrays returned by
 virStringSplit we need to know the count of the elements in the array.
 Modify virStringSplit to return this value, rename it and add a helper
 with the old name so that we don't need to update all the code.
 ---
 
 Notes:
 Version 3:
 - added test coverage

Thanks :)

 
  src/libvirt_private.syms |  1 +
  src/util/virstring.c | 24 
  src/util/virstring.h |  6 ++
  tests/virstringtest.c| 14 +-
  4 files changed, 40 insertions(+), 5 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] [Xen-devel] [PATCH 5/5] Add a test suite for libxl option generator

[Jim Fehlig]

Daniel P. Berrange wrote:
 On Mon, Jun 02, 2014 at 01:53:46PM +0100, Ian Campbell wrote:
   
 On Fri, 2014-05-30 at 18:24 +0100, Daniel P. Berrange wrote:
 
 +if (STRNEQ(expectargv, (char *)actualargv)) {
 +virtTestDifference(stderr, expectargv, (char *)actualargv);
 +goto cleanup;
 +}
   
 Since you are using libxl_domain_config_gen_json you can control the
 pretty printing, but if you were to use the libxl_domain_config_to_json
 you might have problems if the library was to do something slightly
 different e.g. with whitespace.

 In 4.5 we will have libxl_*_from_json and (I think) libxl_*_compare, so
 you could read in the template and compare it with the generated struct.
 That doesn't help you now of course.

 Also in 4.5 the json will omit fields which are set to the their
 explicit default value. libxl_*_from_json will still do the right thing,
 but it'd be another annoyance for you here I think.

 Lastly, when we add new fields to the API they will start showing up in
 the json (modulo the omission of defaults discussed above).
 

 Hmm, that's a v good point.
   

One that can already be seen.  The test works when run on Xen 4.3, but
fails on 4.4.  E.g. c_info gained some new fields

poolid: 0,
run_hotplug_scripts: default,
+pvh: default,
+driver_domain: default
},

And on_watchdog's value changed

on_poweroff: null,
on_reboot: destroy,
-on_watchdog: null,
+on_watchdog: destroy,
on_crash: destroy

Regards,
Jim

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

Re: [libvirt] [PATCHv3 14/36] util: string: Add helper to free non-NULL terminated string arrays

[Eric Blake]

On 05/30/2014 02:37 AM, Peter Krempa wrote:
 Sometimes the length of the string list is known but the array isn't
 NULL terminated. Add helper to free the array in such cases.

Can't this just be done with:

array[count] = NULL;
virStringFreeList(array);

Furthermore, MOST of our allocation guarantees a NULL-terminated array
(VIR_ALLOC_N for example).  Where did you actually run into a situation
where the tail wasn't already NULL?

 ---
  src/libvirt_private.syms |  1 +
  src/util/virstring.c | 20 
  src/util/virstring.h |  1 +
  3 files changed, 22 insertions(+)

I'm not quite sold on whether we need this.  The code looks clean
enough, but without seeing it used in context, I'm reserving judgment
until later in the series.

-- 
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] [PATCHv3 15/36] util: storagefile: Add helper to resolve ../, ./ and //// in paths

[Eric Blake]

On 05/30/2014 02:37 AM, Peter Krempa wrote:
 As gnulib's canonicalize_filename_mode isn't LGPL and relative snapshot
 resolution code will need to clean paths with relative path components

s/components/components,/

 this patch adds a libvirt's own implementation of that functionality and

s/adds a/adds/

 tests to make sure everything works well.
 ---
  src/util/virstoragefile.c | 95 
 +++
  src/util/virstoragefile.h |  2 +
  tests/virstoragetest.c| 83 +
  3 files changed, 180 insertions(+)
 

 +static char *
 +virStorageFileExportPath(char **components,
 + size_t ncomponents,
 + bool beginSlash)
 +{
 +virBuffer buf = VIR_BUFFER_INITIALIZER;
 +size_t i;
 +char *ret = NULL;
 +
 +if (beginSlash)
 +virBufferAddLit(buf, /);
 +
 +for (i = 0; i  ncomponents; i++) {
 +if (i != 0)
 +virBufferAddLit(buf, /);

I find it slightly fewer lines of code to just blindly add a trailing
'/' always...

 +
 +virBufferAdd(buf, components[i], -1);
 +}

...then use virBufferTrim() to remove the last unneeded one.  But that's
aesthetic; what you have works.

 +
 +/* if the output string is empty ... wel just return an empty string */

Did you mean well or we'll? Either way, it still reads okay (and
shorter) as:

s/wel//

 +virStorageFileSimplifyPath(const char *path,
 +   bool allow_relative)
 +{
 +bool beginSlash = false;
 +char **components = NULL;
 +size_t ncomponents = 0;
 +size_t i;
 +char *ret = NULL;
 +
 +/* special cases are  and //, return them as they are */
 +if (STREQ(path, ) || STREQ(path, //)) {
 +ignore_value(VIR_STRDUP(ret, path));

It's more than just // that is special; also special is anything with
a leading double slash (//foo should not be simplified to /foo, even
though /bar//foo can be simplified.  Other corner cases: //foo//bar
should be simplified to //foo/bar, //../blah can be simplified to
//blah).  Maybe this means checking if path[0]=='/'  path[1]=='/' 
path[2]!='/'.

 +++ b/tests/virstoragetest.c
 @@ -512,12 +512,61 @@ testStorageLookup(const void *args)
  return ret;
  }
 
 +
 +struct testPathSimplifyData
 +{
 +const char *path;
 +const char *exp_abs;
 +const char *exp_rel;
 +};
 +

Thanks; adding the test makes it more obvious what the code intends to do :)


  static int
  mymain(void)
  {
  int ret;
  virCommandPtr cmd = NULL;
  struct testChainData data;
 +struct testPathSimplifyData data3;

data followed by data3 looks a bit odd :)


 +
 +/* PATH, absolute simplification, relative simplification */
 +TEST_SIMPLIFY(1, /, /, /);
 +TEST_SIMPLIFY(2, /path, /path, /path);
 +TEST_SIMPLIFY(3, /path/to/blah, /path/to/blah, /path/to/blah);
 +TEST_SIMPLIFY(4, /path/, /path, /path);
 +TEST_SIMPLIFY(5, ///, /, /);
 +TEST_SIMPLIFY(6, //, //, //);
 +TEST_SIMPLIFY(7, , , );
 +TEST_SIMPLIFY(8, ../, , ..);
 +TEST_SIMPLIFY(9, ../../, , ../..);
 +TEST_SIMPLIFY(10, ../../blah, blah, ../../blah);
 +TEST_SIMPLIFY(11, /./././blah, /blah, /blah);
 +TEST_SIMPLIFY(12, .././../././../blah, blah, ../../../blah);
 +TEST_SIMPLIFY(13, /././, /, /);
 +TEST_SIMPLIFY(14, ./././, , );

Turning . into empty looks a bit odd (POSIX requires  to fail while
. is the current directory).  Not sure if that needs tweaking.  Also,
maybe it's worth a test of plain . after test 7 (so that we are
separating test of . behavior from test 14's coverage of multiple /.
behavior).

 +TEST_SIMPLIFY(15, blah/../foo, foo, foo);

This is not always true. It is wrong if blah/ is not a (symlink to a)
directory; and if blah IS a symlink, it can still be wrong if it is a
symlink to somewhere else in the hierarchy.  While I'm in favor of
simplifying /./ and a//b, I'm less certain about the benefits of
reducing '..' without actually stat'ing the underlying filesystem.

 +TEST_SIMPLIFY(16, foo/bar/../blah, foo/blah, foo/blah);
 +TEST_SIMPLIFY(17, foo/bar/.././blah, foo/blah, foo/blah);
 +TEST_SIMPLIFY(18, /path/to/foo/bar/../../../../../../../../baz, 
 /baz, /baz);
 +TEST_SIMPLIFY(19, path/to/foo/bar/../../../../../../../../baz, baz, 
 ../../../../baz);
 +TEST_SIMPLIFY(20, path/to/foo/bar, path/to/foo/bar, 
 path/to/foo/bar);
 +TEST_SIMPLIFY(21, 
 some/path/to/image.qcow/../image2.qcow/../image3.qcow/,
 +  some/path/to/image3.qcow, some/path/to/image3.qcow);

Yep, the more I look at this, the more I worry that simplifying '..' is
wrong. :(

-- 
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] [libvirt-glib] [PATCH] build: fix race in vapi/ subdirectory

[Eric Blake]

[adding glib to subject line to possibly trigger some mail filters...]

On 06/02/2014 10:52 AM, Michael Catanzaro wrote:
 libvirt-gobject-1.0.vapi depends on libvirt-gconfig-1.0.vapi
 ---
  vapi/Makefile.am | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
 
 diff --git a/vapi/Makefile.am b/vapi/Makefile.am
 index 0154104..af10e98 100644
 --- a/vapi/Makefile.am
 +++ b/vapi/Makefile.am
 @@ -17,7 +17,7 @@ libvirt-glib-1.0.vapi:
 $(top_builddir)/libvirt-glib/LibvirtGLib-1.0.gir
   --library libvirt-glib-1.0  \
   $
  
 -libvirt-gobject-1.0.vapi:
 $(top_builddir)/libvirt-gobject/LibvirtGObject-1.0.gir
 libvirt-glib-1.0.vapi
 +libvirt-gobject-1.0.vapi:
 $(top_builddir)/libvirt-gobject/LibvirtGObject-1.0.gir
 libvirt-glib-1.0.vapi libvirt-gconfig-1.0.vapi

This hit the list with line-wrapping , making it a bit harder to tell
what changed.  It's okay to use:

libvirt-gobject-1.0.vapi: \
dep1 dep2 \
dep3

instead of cramming it on one line longer than 80 columns, if that makes
it more legible.  I'm not a regular libvirt-glib contributor, so I won't
push it, but the change (adding libvirt-gconfig-1.0.vapi as an
additional dependency) makes sense on the surface.

-- 
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] [libvirt-glib] [PATCH] build: fix race in vapi/ subdirectory

[Michael Catanzaro]

On Mon, 2014-06-02 at 16:12 -0600, Eric Blake wrote:
 This hit the list with line-wrapping , making it a bit harder to tell
 what changed. 

Sorry, patch attached to avoid line wrapping.

I wonder if the libvirt-gconfig-1.0.vapi dependency should actually
REPLACE the one on libvirt-glib-1.0.vapi, but I haven't checked.
From cca64359dd43a5392609c58b7f8c1fe161e3775e Mon Sep 17 00:00:00 2001
From: Michael Catanzaro mcatanz...@gnome.org
Date: Mon, 2 Jun 2014 11:36:24 -0500
Subject: [PATCH] build: fix race in vapi/ subdirectory

libvirt-gobject-1.0.vapi depends on libvirt-gconfig-1.0.vapi
---
 vapi/Makefile.am | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/vapi/Makefile.am b/vapi/Makefile.am
index 0154104..af10e98 100644
--- a/vapi/Makefile.am
+++ b/vapi/Makefile.am
@@ -17,7 +17,7 @@ libvirt-glib-1.0.vapi: $(top_builddir)/libvirt-glib/LibvirtGLib-1.0.gir
 		--library libvirt-glib-1.0	\
 		$
 
-libvirt-gobject-1.0.vapi: $(top_builddir)/libvirt-gobject/LibvirtGObject-1.0.gir libvirt-glib-1.0.vapi
+libvirt-gobject-1.0.vapi: $(top_builddir)/libvirt-gobject/LibvirtGObject-1.0.gir libvirt-glib-1.0.vapi libvirt-gconfig-1.0.vapi
 	$(AM_V_GEN)$(VAPIGEN)			 \
 		--vapidir=$(builddir)		 \
 		--pkg gobject-2.0		 \
-- 
1.9.3



signature.asc
Description: This is a digitally signed message part
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [Xen-devel] [PATCH 0/5] Testing libvirt XML - libxl_domain_config conversion

[Jim Fehlig]

Daniel P. Berrange wrote:
 On Mon, Jun 02, 2014 at 01:41:58PM +0100, Ian Campbell wrote:
   
I hacked around this, but it is a little dirty too. libvirt
already links to libyajl for the QEMU driver, but we  don't
really need the raw YAJL objects. It'd be nice to have a

   char * libxl_domain_config_as_json(libxl_domain_config *p)

as a higher level wrapper around libxl_domain_config_gen_json
avoiding the pain of dealing with YAJL's different APIs.

Ian J mentioned to me that he thought there was already such a
method, but AFAICT, the only such code is in the 'xl' command
line tool itself (xl_cmdimpl.c - printf_info_one_json)
   
 That was me not Ian J I think.
 

 Opps, my bad, was talking to too many people to remember things clearly :-)
  
   
 The function you need is libxl_domain_config_to_json (which is
 autogenerated, so in _libxl_types.[hc]). I think the general
 libxl_*_to_json support has been there since Xen 4.2, but IIRC
 libxl_domain_config only got moved into the autogenerated/IDL layer in
 4.3.
 

 Ahhh, so I was looking in the wrong place - no wonder 'git grep' failed
 to find it :-)
   

This patch becomes a bit smaller by using libxl_domain_config_to_json()
- see below diff.  It works with Xen 4.2-4.4, although the test fails on
all but 4.4 due to changes in the json noted earlier (e.g. additional
c_info fields added to the returned json).

Thanks for looking at this btw!  Great topic for the hackathon.  I was
groaning about the need for round-trip config testing last week while
reviewing some other patches, just before you posted this series!  Awesome!

Regards,
Jim


diff --git a/tests/libxlxml2jsontest.c b/tests/libxlxml2jsontest.c
index 02808b7..ed7c256 100644
--- a/tests/libxlxml2jsontest.c
+++ b/tests/libxlxml2jsontest.c
@@ -40,24 +40,11 @@
 # include virmock.h
 # include testutilsxen.h
 
-# ifdef WITH_YAJL2
-#  define HAVE_YAJL_V2
-# endif
-
-# include yajl/yajl_gen.h
-# include libxl_json.h
-
 # define VIR_FROM_THIS VIR_FROM_XEN
 
 static const char *abs_top_srcdir;
 static virCapsPtr xencaps;
 
-# ifdef WITH_YAJL2
-#  define yajl_size_t size_t
-# else
-#  define yajl_size_t unsigned int
-# endif
-
 static int testCompareXMLToJSONFiles(const char *xml,
  const char *cmdline)
 {
@@ -69,31 +56,10 @@ static int testCompareXMLToJSONFiles(const char *xml,
 libxl_domain_config config;
 xentoollog_logger *log = NULL;
 virDomainXMLOptionPtr xmlopt = NULL;
-yajl_gen gen;
-# ifndef WITH_YAJL2
-yajl_gen_config conf = { 1,  };
-# endif
-const unsigned char *actualargv;
-yajl_size_t actualargvlen;
+char *actualargv = NULL;
 
 libxl_domain_config_init(config);
 
-# ifdef WITH_YAJL2
-gen = yajl_gen_alloc(NULL);
-if (gen) {
-yajl_gen_config(gen, yajl_gen_beautify, 1);
-yajl_gen_config(gen, yajl_gen_indent_string, );
-yajl_gen_config(gen, yajl_gen_validate_utf8, 1);
-}
-# else
-gen = yajl_gen_alloc(conf, NULL);
-# endif
-if (!gen) {
-virReportError(VIR_ERR_INTERNAL_ERROR, %s,
-   Unable to create JSON formatter);
-goto cleanup;
-}
-
 if (!(log = (xentoollog_logger
*)xtl_createlogger_stdiostream(stderr, XTL_DEBUG, 0)))
 goto cleanup;
 
@@ -114,24 +80,18 @@ static int testCompareXMLToJSONFiles(const char *xml,
 if (libxlBuildDomainConfig(gports, vmdef, ctx, config)  0)
 goto cleanup;
 
-libxl_domain_config_gen_json(gen, config);
-
-if (yajl_gen_get_buf(gen, actualargv, actualargvlen) !=
yajl_gen_status_ok) {
-virReportOOMError();
-goto cleanup;
-}
-
+actualargv = libxl_domain_config_to_json(ctx, config);
 virtTestLoadFile(cmdline, expectargv);
 
-if (STRNEQ(expectargv, (char *)actualargv)) {
-virtTestDifference(stderr, expectargv, (char *)actualargv);
+if (STRNEQ(expectargv, actualargv)) {
+virtTestDifference(stderr, expectargv, actualargv);
 goto cleanup;
 }
 
 ret = 0;
 
  cleanup:
-yajl_gen_free(gen);
+VIR_FREE(actualargv);
 VIR_FREE(expectargv);
 virDomainDefFree(vmdef);
 virObjectUnref(gports);

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

Re: [libvirt] [PATCHv3 16/36] util: storage: Add helper to resolve relative path difference

[Eric Blake]

On 05/30/2014 02:37 AM, Peter Krempa wrote:
 This patch introduces a function that will allow us to resolve a
 relative difference between two elements of a disk backing chain. This
 fucntion will be used to allow relative block commit and block pull

s/fucntion/function/

 where we need to specify the new relative name of the image to qemu.
 
 This patch also adds unit tests for the function to verify that it works
 correctly.
 ---
  src/libvirt_private.syms  |   1 +
  src/util/virstoragefile.c |  45 +
  src/util/virstoragefile.h |   4 ++
  tests/virstoragetest.c| 101 
 ++
  4 files changed, 151 insertions(+)
 

 +int
 +virStorageFileGetRelativeBackingPath(virStorageSourcePtr from,
 + virStorageSourcePtr to,
 + char **relpath)

Missing documentation on what this function is intended to do.

 +{
 +virBuffer buf = VIR_BUFFER_INITIALIZER;
 +virStorageSourcePtr next;
 +char *tmp = NULL;
 +char ret = -1;
 +
 +*relpath = NULL;
 +
 +for (next = from; next; next = next-backingStore) {
 +if (!next-backingRelative || !next-relPath) {
 +ret = 1;
 +goto cleanup;
 +}
 +
 +if (next != from)
 +virBufferAddLit(buf, /../);
 +
 +virBufferAdd(buf, next-relPath, -1);
 +
 +if (next == to)
 +break;
 +}
 +
 +if (next != to)
 +goto cleanup;
 +
 +if (!(tmp = virBufferContentAndReset(buf)))
 +goto cleanup;
 +
 +if (!(*relpath = virStorageFileSimplifyPath(tmp, true)))

Ouch.  Playing fast and loose with 'path/to/file/../otherfile' as a way
to simplify to 'path/to/otherfile' is very risky.  Instead of doing
'path/to/file'+'/../'+'otherfile', I'd feel better doing
mdirname('path/to/file')+'/'+'otherfile' == 'path/to'+'/'+'otherfile'.
Don't we already have a helper function in util/virfile.h that can
concatenate a relative filename in relation to the containing directory
of another filename?

/me re-reads virFileBuildPath...

nope, not quite what we need.


 
 +virStorageSource backingchain[9];
 +
 +static void
 +testPathRelativePrepare(void)
 +{
 +size_t i;
 +
 +for (i = 0; i  ARRAY_CARDINALITY(backingchain) - 1; i++) {
 +backingchain[i].backingStore = backingchain[i+1];
 +}
 +
 +backingchain[0].relPath = (char *) /path/to/some/img;
 +backingchain[0].backingRelative = false;
 +
 +backingchain[1].relPath = (char *) asdf;
 +backingchain[1].backingRelative = true;
 +
 +backingchain[2].relPath = (char *) test;
 +backingchain[2].backingRelative = true;
 +
 +backingchain[3].relPath = (char *) blah;
 +backingchain[3].backingRelative = true;

1 through 3 are relative to the directory containing img in 0, but that
is '/path/to/some/blah' and not a simplification of
'/path/to/some/img/../blah' since 'img/..' doesn't resolve via POSIX
functions.

 +
 +backingchain[4].relPath = (char *) /path/to/some/other/img;
 +backingchain[4].backingRelative = false;

As a non-relative image, the search for relative starts over again.

 +
 +backingchain[5].relPath = (char *) ../relative/in/other/path;
 +backingchain[5].backingRelative = true;

Here, the answer has to be
/path/to/some/other/../relative/in/other/path, unless we did a stat
test to ensure that '/path/to/some/other/..' resolves to the same
location as '/path/to/some'.

 +
 +backingchain[6].relPath = (char *) test;
 +backingchain[6].backingRelative = true;
 +
 +backingchain[7].relPath = (char *) ../../../../../below;
 +backingchain[7].backingRelative = true;

I see that you are trying to test that you can't escape past /...

 +
 +backingchain[8].relPath = (char *) a/little/more/upwards;
 +backingchain[8].backingRelative = true;

but that still doesn't make me feel good about simplifying .. without
actual stat tests.

 +testPathRelativePrepare();
 +
 +TEST_RELATIVE_BACKING(1, backingchain[0], backingchain[1], NULL);

I'm trying to wrap my head around this test and the expected results,
and merely got confused.  I need something that describes what we are
doing visually, something like:

Given the chain { base - intermediate - active }, we plan to
commit intermediate into base and need to rewrite the backing file
stored in active to point to base.  TEST_RELATIVE_BACKING(, active,
intermediate, expected) then gives the expected string to write into active.

Except that my formulation doesn't work with what your code expects, or
I'm getting lost somewhere.  backingchain[0] is the active image (living
at /path/to/some/file, and with backing element currently at the
relative asdf); and we are committing backingchain[1] (file at asdf,
 whose backing is currently test).  Doesn't that mean that we want to
determine a relative name for test that we can store in the metadata
for /path/to/some/file?  If so, isn't the answer asdf, 

Re: [libvirt] [PATCHv3 17/36] util: storagefile: Add canonicalization to virStorageFileSimplifyPath

[Eric Blake]

On 05/30/2014 02:37 AM, Peter Krempa wrote:
 The recently introduced virStorageFileSimplifyPath is good at resolving
 relative portions of a path. To add full path canonicalization
 capability we need to be able to resolve symlinks in the path too.
 
 This patch adds a callback to that function so that arbitrary storage
 systems can use this functionality.
 ---
  src/libvirt_private.syms  |  1 +
  src/util/virstoragefile.c | 82 
 +--
  src/util/virstoragefile.h |  9 ++
  tests/virstoragetest.c| 80 +
  4 files changed, 170 insertions(+), 2 deletions(-)
 

 
 +static int
 +virStorageFileExplodePath(const char *path,
 +  size_t at,
 +  char ***components,
 +  size_t *ncomponents)
 +{
 +char **tmp = NULL;
 +char **next;
 +size_t ntmp = 0;
 +int ret = -1;
 +
 +if (!(tmp = virStringSplitCount(path, /, 0, ntmp)))
 +goto cleanup;
 +
 +/* prepend */
 +for (next = tmp; *next; next++) {
 +if (VIR_INSERT_ELEMENT(*components, at, *ncomponents, *next)  0)

So this is the call that is not necessarily guaranteeing NULL termination...

 +goto cleanup;
 +
 +at++;
 +}
 +
 +ret = 0;
 +
 + cleanup:
 +virStringFreeListCount(tmp, ntmp);

...and therefore why you wanted to add this function from 14/36.  Hmm,
aren't you just taking the split array and reversing its order?  Do you
really need VIR_INSERT_ELEMENT for each array element, or could you just
VIR_ALLOC_N an array already big enough and then do the assignment from
'n' to 'size - n' in a loop through all size entries, at which point
you'd be guaranteed a NULL terminator and fewer intermediate malloc calls?

 +return ret;
 +}
 +
 +
  char *
 -virStorageFileSimplifyPath(const char *path,
 -   bool allow_relative)
 +virStorageFileSimplifyPathInternal(const char *path,
 +   bool allow_relative,
 +   
 virStorageFileSimplifyPathReadlinkCallback cb,
 +   void *cbdata)
  {
  bool beginSlash = false;
  char **components = NULL;
  size_t ncomponents = 0;
 +char *linkpath = NULL;
 +char *currentpath = NULL;
  size_t i;
  char *ret = NULL;
 
 @@ -2012,6 +2046,40 @@ virStorageFileSimplifyPath(const char *path,
  continue;
  }
 
 +/* read link and stuff */
 +if (cb) {
 +int rc;
 +if (!(currentpath = virStorageFileExportPath(components, i + 1,
 + beginSlash)))

My first thought when reading this in isolation was Why are we changing
the environment variable PATH to export it?  I'm not sure how much of
your existing series is impacted, but I'm wondering if using name is
better than path when referring to a file name (GNU Coding Standards
recommends this nomenclature for a reason, after all, even if glibc's
realpath() muddies the water by having a use of path for a non-PATH
entity).


 +goto cleanup;
 +
 +if ((rc = cb(currentpath, linkpath, cbdata))  0)
 +goto cleanup;
 +
 +if (rc == 0) {
 +if (linkpath[0] == '/') {
 +/* start from a clean slate */
 +virStringFreeListCount(components, ncomponents);
 +ncomponents = 0;
 +components = NULL;
 +beginSlash = true;
 +i = 0;
 +} else {
 +VIR_FREE(components[i]);
 +VIR_DELETE_ELEMENT(components, i, ncomponents);
 +}
 +
 +if (virStorageFileExplodePath(linkpath, i, components,
 +  ncomponents)  0)
 +goto cleanup;
 +
 +VIR_FREE(linkpath);
 +VIR_FREE(currentpath);
 +
 +continue;
 +}
 +}
 +

I've run out of review time at the moment, I'll have to pick up here
again later...

-- 
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] [PATCHv3 17/36] util: storagefile: Add canonicalization to virStorageFileSimplifyPath

[Eric Blake]

On 05/30/2014 02:37 AM, Peter Krempa wrote:
 The recently introduced virStorageFileSimplifyPath is good at resolving
 relative portions of a path. To add full path canonicalization
 capability we need to be able to resolve symlinks in the path too.
 
 This patch adds a callback to that function so that arbitrary storage
 systems can use this functionality.
 ---
  src/libvirt_private.syms  |  1 +
  src/util/virstoragefile.c | 82 
 +--
  src/util/virstoragefile.h |  9 ++
  tests/virstoragetest.c| 80 +
  4 files changed, 170 insertions(+), 2 deletions(-)
 

 
 diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c
 index 80d73ca..771df0b 100644
 --- a/tests/virstoragetest.c
 +++ b/tests/virstoragetest.c
 @@ -639,6 +639,72 @@ testPathRelative(const void *args)
  }
 
 
 +struct testPathCanonicalizeData
 +{
 +const char *path;
 +const char *expect;
 +};
 +
 +static const char *testPathCanonicalizeSymlinks[][2] =
 +{
 +{/path/blah, /other/path/huzah},
 +{/path/to/relative/symlink, ../../actual/file},
 +};

Looks like a good way to test both types of symlinks.


 +
 +TEST_PATH_CANONICALIZE(1, /some/full/path, /some/full/path);
 +TEST_PATH_CANONICALIZE(2, /path/blah, /other/path/huzah);
 +TEST_PATH_CANONICALIZE(3, /path/to/relative/symlink, 
 /path/actual/file);
 +

You probably also want to test a symlink to directory in the middle of a
longer name: for example, /path/blah/yippee should resolve to
/other/path/huzah/yippee

   cleanup:
  /* Final cleanup */
  virStorageSourceFree(chain);
 

-- 
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] [PATCHv3 18/36] storage: gluster: Add backend to return unique storage file path

[Eric Blake]

On 05/30/2014 02:37 AM, Peter Krempa wrote:
 Use virStorageFileSimplifyPathInternal to canonicalize gluster paths
 via a callback and use it for the unique volume path retrieval API.
 ---
  src/storage/storage_backend_gluster.c | 81 
 +++
  1 file changed, 81 insertions(+)
 
 diff --git a/src/storage/storage_backend_gluster.c 
 b/src/storage/storage_backend_gluster.c
 index 3db4e66..1e86383 100644
 --- a/src/storage/storage_backend_gluster.c
 +++ b/src/storage/storage_backend_gluster.c
 @@ -533,6 +533,7 @@ typedef virStorageFileBackendGlusterPriv 
 *virStorageFileBackendGlusterPrivPtr;
 
  struct _virStorageFileBackendGlusterPriv {
  glfs_t *vol;
 +char *uid;

Once again, char *uid is misnamed (when I see uid, I think integral uid_t).

Otherwise, seems to make sense.

-- 
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] [PATCHv3 19/36] qemu: json: Add format strings for optional command arguments

[Eric Blake]

On 05/30/2014 02:37 AM, Peter Krempa wrote:
 This patch adds option to specify that a json qemu command argument is
 optional without the need to use if's or ternary operators to pass the
 list. Additionally all the modifier characters are documented to avoid
 user confusion.
 ---
  src/qemu/qemu_monitor_json.c | 122 
 +--
  1 file changed, 84 insertions(+), 38 deletions(-)
 

The diffstat is misleading - the bulk of the addition is documentation,
and the bulk of the deletions are compression taking advantage of the
new feature.  Overall, I like the patch!

 + *
 + * i: signed integer value
 + * z: signed integer value, omitted if zero
 + *
 + * I: signed long integer value
 + * Z: integer value, signed, omitted if zero

Looks more consistent as: signed long integer value, omitted if zero

 + *
 + * u: unsigned integer value
 + * p: unsigned integer value, omitted if zero
 + *
 + * U: unsigned long integer value (see below for quirks)
 + * P: unsigned long integer value, omitted if zero,

drop the trailing comma

 + *
 + * d: double precision floating point number
 + * b: boolean value

Is it worth a B for a boolean value that is omitted if false?

 + * n: json null value
 + * a: json array
 + */
  type = key[0];
  key += 2;
 

 @@ -3557,7 +3603,7 @@ int qemuMonitorJSONSendKey(qemuMonitorPtr mon,
 
  cmd = qemuMonitorJSONMakeCommand(send-key,
   a:keys, keys,
 -  holdtime ? U:hold-time : NULL, 
 holdtime,
 +  P:hold-time, holdtime,
NULL);

Fix the indentation while you are at it.

ACK with nits 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

Re: [libvirt] [PATCHv3 20/36] tests: storagetest: Unify and reformat storage chain format string

[Eric Blake]

On 05/30/2014 02:37 AM, Peter Krempa wrote:
 All the fields crammed into two lines weren't easy to parse by human
 eyes. Split up the format string into lines and put it into a central
 variable so that changes in two places aren't necessary.
 ---
  tests/virstoragetest.c | 24 ++--
  1 file changed, 18 insertions(+), 6 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] [PATCHv3 22/36] tests: virstoragetest: Fix output when hitting errors

[Eric Blake]

On 05/30/2014 02:37 AM, Peter Krempa wrote:
 When the test is failing but the debug output isn't enabled the
 resulting line would look ugly like and would not contain the actual
 difference.
 
 TEST: virstoragetest
   .chain member 1!chain member 1!chain member 1!
 
 Store the member index in the actual checked string to hide this problem
 ---
  tests/virstoragetest.c | 6 +++---
  1 file changed, 3 insertions(+), 3 deletions(-)

ACK.  Thanks for cleaning up after me; I stuck in the chain member as a
debug item a while back, and forgot to remove it (or formalize it).

-- 
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] [PATCHv3 28/36] storage: Don't canonicalize paths unnecessarily

[Eric Blake]

On 05/30/2014 02:37 AM, Peter Krempa wrote:
 Store backing chain paths as non-cannonical. The cannonicalization step

s/cannon/canon/ twice

 will be already taken. This will allow to avoid storing unnecessary
 amounts of data.
 ---
  src/util/virstoragefile.c | 33 ++---
  tests/virstoragetest.c| 10 +-
  2 files changed, 11 insertions(+), 32 deletions(-)
 

[I'll review the technical content tomorrow when it's not quite so late
for me...]

-- 
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] [PATCHv3 30/36] qemu: monitor: Add argument for specifying backing name for block commit

[Eric Blake]

On 05/30/2014 02:37 AM, Peter Krempa wrote:
 To allow changing the name that is recorded in the overlay of the TOP
 image used in a block commit operation, we need to specify the backing
 name to qemu. This is done via the backing-file attribute to the
 block-commit commad.
 ---
  src/qemu/qemu_driver.c   | 1 +
  src/qemu/qemu_monitor.c  | 9 ++---
  src/qemu/qemu_monitor.h  | 1 +
  src/qemu/qemu_monitor_json.c | 2 ++
  src/qemu/qemu_monitor_json.h | 1 +
  tests/qemumonitorjsontest.c  | 2 +-
  6 files changed, 12 insertions(+), 4 deletions(-)

We need a capability in qemu_capabilities.[ch] first; that way we can
give better error messages if qemu is too old to support use of this
feature.

ACK to this patch; it will be the caller qemu_driver that needs to pay
attention to the capability.

-- 
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] [PATCHv3 30/36] qemu: monitor: Add argument for specifying backing name for block commit

[Eric Blake]

On 05/30/2014 02:37 AM, Peter Krempa wrote:
 To allow changing the name that is recorded in the overlay of the TOP
 image used in a block commit operation, we need to specify the backing
 name to qemu. This is done via the backing-file attribute to the
 block-commit commad.

Oops, missed this on first review.  s/commad/command/

 ---
  src/qemu/qemu_driver.c   | 1 +
  src/qemu/qemu_monitor.c  | 9 ++---
  src/qemu/qemu_monitor.h  | 1 +
  src/qemu/qemu_monitor_json.c | 2 ++
  src/qemu/qemu_monitor_json.h | 1 +
  tests/qemumonitorjsontest.c  | 2 +-
  6 files changed, 12 insertions(+), 4 deletions(-)
 
 
 -VIR_DEBUG(mon=%p, device=%s, top=%s, base=%s, bandwidth=%ld,
 -  mon, device, top, base, bandwidth);
 +VIR_DEBUG(mon=%p, device=%s, top=%s, base=%s, backingName=%s, 
 +  bandwidth=%ld,
 +  mon, device, top, base, backingName, bandwidth);

NULLSTR(backingName)

-- 
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] [PATCHv3 31/36] qemu: monitor: Add support for backing name specification for block-stream

[Eric Blake]

On 05/30/2014 02:37 AM, Peter Krempa wrote:
 To allow changing the name that is recorded in the top of the current
 image chain used in a block pull/rebase operation, we need to specify
 the backing name to qemu. This is done via the backing-file attribute
 to the block-stream commad.

s/commad/command/

 ---
  src/qemu/qemu_driver.c   |  8 
  src/qemu/qemu_migration.c|  6 +++---
  src/qemu/qemu_monitor.c  | 12 +++-
  src/qemu/qemu_monitor.h  |  3 ++-
  src/qemu/qemu_monitor_json.c | 15 +++
  src/qemu/qemu_monitor_json.h |  1 +
  6 files changed, 32 insertions(+), 13 deletions(-)
 

Again, I think the qemu_capability.[ch] change is needed as a separate
commit; but you can get away with the same capability for both
block-commit and block-pull since both commands are learning the feature
in the same qemu series.  Not sure if the capability needs to come
first, or if you have it later in the series.

This patch is okay (although this and 30/36 should probably wait until
Jeff's series is actually committed into qemu.git).


 @@ -3759,6 +3759,7 @@ int
  qemuMonitorJSONBlockJob(qemuMonitorPtr mon,

 +
 +if (backingName  mode != BLOCK_JOB_PULL) {
 +virReportError(VIR_ERR_INTERNAL_ERROR, %s,
 +   _(backing name is supported only for block pull));
 +return -1;
 +}

[side note - I honestly don't know why we tried to cram so much into
qemuMonitorJSONBlockJob through the entire stack; it might have been
simpler to have one qemu_monitor.h entry point per QMP command, rather
than trying to multiplex.  But it may not be worth the refactor now]

If these were separate functions instead of multiplexed through a mode
argument, then you wouldn't need a check like this (since the check will
never fail unless we introduce a bug in qemu_driver.c).

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] [PATCHv3 32/36] lib: Introduce flag VIR_DOMAIN_BLOCK_COMMIT_RELATIVE

[Eric Blake]

On 05/30/2014 02:37 AM, Peter Krempa wrote:
 Introduce flag for the block commit API to allow the commit operation to
 leave the chain relatively addressed. Also adds a virsh switch to enable
 this behavior.
 ---
  include/libvirt/libvirt.h.in | 4 
  tools/virsh-domain.c | 7 +++
  tools/virsh.pod  | 6 --
  3 files changed, 15 insertions(+), 2 deletions(-)

  },
 +{.name = keep-relative,
 + .type = VSH_OT_BOOL,
 + .help = N_(keep the backing chain relative if it was relatively 
 +referenced if it was before)

s/if it was before/before/

I'd also like to see the flag mentioned in the doc text of libvirt.c.

-- 
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] [PATCHv3 33/36] lib: Introduce flag VIR_DOMAIN_BLOCK_REBASE_RELATIVE

[Eric Blake]

On 05/30/2014 02:37 AM, Peter Krempa wrote:
 Introduce flag for the block rebase API to allow the rebase operation to
 leave the chain relatively addressed. Also adds a virsh switch to enable
 this behavior.
 ---
  include/libvirt/libvirt.h.in |  2 ++
  tools/virsh-domain.c | 22 +++---
  tools/virsh.pod  |  4 
  3 files changed, 25 insertions(+), 3 deletions(-)
 

Again, missing doc changes in libvirt.c mentioning the flag.


 -else
 +if (base) {
 +  if (vshCommandOptBool(cmd, keep-relative))
 +  flags |= VIR_DOMAIN_BLOCK_REBASE_RELATIVE;

Why not parse this flag unconditionally, to check that the code has a
sane error path if the flag is present but base was not specified?  In
other words, filtering at the virsh level is too high when it comes to
validating lower levels.

 +{.name = keep-relative,
 + .type = VSH_OT_BOOL,
 + .help = N_(keep the backing chain relative if it was relatively 
 +referenced if it was before)

s/if it was before/before/

 @@ -2091,6 +2100,13 @@ cmdBlockPull(vshControl *ctl, const vshCmd *cmd)
  bool quit = false;
  int abort_flags = 0;
 
 +if (vshCommandOptBool(cmd, keep-relative) 
 +!vshCommandOptBool(cmd, base)) {
 +vshError(ctl, %s, _(--keep-relative is supported only with 
 partial 
 +  pull operations with --base specified));

Again, let the lower level flag this, to prove that error message is sane.

-- 
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] [PATCHv3 34/36] qemu: caps: Add capability for change-backing-file command

[Eric Blake]

On 05/30/2014 02:37 AM, Peter Krempa wrote:
 This command allows to change the backing file name recorded in the
 metadata of a qcow (or other) image. The capability also notifies that
 the block-stream and block-commit commands understand the
 backing-file attribute.

It might be better to rebase this ahead of 30 and 31.  Also, where does
the qemu_driver code actually honor the new flags from 32 and 33 in
comparison to whether this capability bit is set?


  QEMU_CAPS_HOST_PCI_MULTIDOMAIN = 166, /* support domain  0 in host pci 
 address */
  QEMU_CAPS_MSG_TIMESTAMP  = 167, /* -msg timestamp */
 +QEMU_CAPS_CHANGE_BACKING_FILE = 168, /* change namen of backing file in 
 metadata */

s/namen/name/

Wait for Jeff's series to actually hit qemu.git before pushing this one.

-- 
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