Re: [libvirt] [RFC PATCH] lxc: don't return error on GetInfo when cgroups not yet set up

2011-10-04 Thread Daniel P. Berrange
On Mon, Oct 03, 2011 at 02:03:18PM -0500, Serge E. Hallyn wrote:
 Quoting Daniel P. Berrange (berra...@redhat.com):
  The LXC controller 'main' method received the handshake FD and invokes
  lxcControllerRun(). This method does various setup tasks, in particular
  the following:
  
  
  
  if (lxcSetContainerResources(def)  0)
  goto cleanup;
  ...
 ...
  if (lxcContainerSendContinue(handshakefd)  0) {
  virReportSystemError(errno, %s,
   _(error sending continue signal to parent));
  goto cleanup;
  }
  VIR_FORCE_CLOSE(handshakefd);
 
 Thanks, Daniel.  You're right!  This is fixed in git, by the patch 'lxc:
 controller: Improve container error reporting' (which does much more
 than it says :).  The following patch is how I had just fixed 0.9.2 this
 morning.  It'll be nicer if I can get the git commit cherrypicked.  I
 can't wait till I can upgrade!

Ah, that explains it :-) Good to know its fixed then.


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] Document that ff callbacks need to be invoked from a clean stack

2011-10-04 Thread Daniel P. Berrange
On Mon, Oct 03, 2011 at 10:41:17PM +0200, Guido Günther wrote:
 On Tue, Aug 16, 2011 at 02:24:53PM +0200, Guido Günther wrote:
  Hi Daniel,
  On Sat, Aug 13, 2011 at 08:57:45PM -0700, Daniel P. Berrange wrote:
   On Fri, Aug 12, 2011 at 11:54:28PM +0200, Guido Günther wrote:
  [..snip..] 
   In the default libvirt event loop, the 'ff' callback is always invoked
   from a clean stack in the event loop, so you never have this problem
   with re-entrancy.
   
Working around this by removing the locks from
virNetSocketRemoveIOCallback leads to another deadlock:
   
   Yeah this is not a viable approach. 
  Sure. This was only to see what else fails.
  

I didn't see a simple way to fix this but would welcome any suggestions.
   
   IMHO we just have to document that event loop implementations
   should make sure that the 'ff' callbacks are always invoked
   from a clean stack. In the case of virt-viewer, this means
   changing it to register a g_idle  callback function to invoke
   the 'ff' callback.
  
  Patch for virt-viewer attached. I'll come up with a doc patch for
  libvirt once I have a bit more time.
 
 As promised here's the patch for libvirt. O.k. to apply?
 Cheers,
  -- Guido

 From c2e2be4e377871ace12bb7adfde130e9b8779ab5 Mon Sep 17 00:00:00 2001
 From: =?UTF-8?q?Guido=20G=C3=BCnther?= a...@sigxcpu.org
 Date: Mon, 3 Oct 2011 22:24:13 +0200
 Subject: [PATCH] Document that ff callbacks need to be invoked from a clean
  stack.
 
 ---
  include/libvirt/libvirt.h.in |6 --
  1 files changed, 4 insertions(+), 2 deletions(-)
 
 diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
 index a3c581d..bd7a0f7 100644
 --- a/include/libvirt/libvirt.h.in
 +++ b/include/libvirt/libvirt.h.in
 @@ -2243,13 +2243,15 @@ typedef void (*virEventHandleCallback)(int watch, int 
 fd, int events, void *opaq
   * @opaque: user data to pass to the callback
   * @ff: the callback invoked to free opaque data blob
   *
 - * Part of the EventImpl, this callback Adds a file handle callback to
 + * Part of the EventImpl, this callback adds a file handle callback to
   * listen for specific events. The same file handle can be registered
   * multiple times provided the requested event sets are non-overlapping
   *
   * If the opaque user data requires free'ing when the handle
   * is unregistered, then a 2nd callback can be supplied for
 - * this purpose.
 + * this purpose. This callback needs to be invoked from a clean stack.
 + * If 'ff' callbacks are invoked directly from the virEventRemoveHandleFunc
 + * they will likely deadlock in libvirt.
   *
   * Returns a handle watch number to be used for updating
   * and unregistering for events


ACK


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

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

[libvirt] [PATCH] qemu: Fix migration with dname

2011-10-04 Thread Jiri Denemark
Destination libvirtd remembers the original name in the prepare phase
and clears it in the finish phase. The original name is used when
comparing domain name in migration cookie.
---
Notes:
Originally, I wanted to transfer the new name in migration cookie but
that appeared to be much more complicated and it would require adding
new Confirm API since the current version does not have 'dname'
parameter.

 src/qemu/qemu_domain.c|1 +
 src/qemu/qemu_domain.h|1 +
 src/qemu/qemu_migration.c |   19 ---
 3 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index d3ad192..65f721a 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -231,6 +231,7 @@ static void qemuDomainObjPrivateFree(void *data)
 qemuDomainObjFreeJob(priv);
 VIR_FREE(priv-vcpupids);
 VIR_FREE(priv-lockState);
+VIR_FREE(priv-origname);
 
 /* This should never be non-NULL if we get here, but just in case... */
 if (priv-mon) {
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index cdf1375..d9f323c 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -126,6 +126,7 @@ struct _qemuDomainObjPrivate {
 int jobs_queued;
 
 unsigned long migMaxBandwidth;
+char *origname;
 };
 
 struct qemuDomainWatchdogEvent
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 1122dab..4516231 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -254,12 +254,18 @@ error:
 static qemuMigrationCookiePtr
 qemuMigrationCookieNew(virDomainObjPtr dom)
 {
+qemuDomainObjPrivatePtr priv = dom-privateData;
 qemuMigrationCookiePtr mig = NULL;
+const char *name;
 
 if (VIR_ALLOC(mig)  0)
 goto no_memory;
 
-if (!(mig-name = strdup(dom-def-name)))
+if (priv-origname)
+name = priv-origname;
+else
+name = dom-def-name;
+if (!(mig-name = strdup(name)))
 goto no_memory;
 memcpy(mig-uuid, dom-def-uuid, VIR_UUID_BUFLEN);
 
@@ -1064,6 +1070,7 @@ qemuMigrationPrepareAny(struct qemud_driver *driver,
 unsigned long long now;
 qemuMigrationCookiePtr mig = NULL;
 bool tunnel = !!st;
+char *origname = NULL;
 
 if (virTimeMs(now)  0)
 return -1;
@@ -1078,7 +1085,7 @@ qemuMigrationPrepareAny(struct qemud_driver *driver,
 
 /* Target domain name, maybe renamed. */
 if (dname) {
-VIR_FREE(def-name);
+origname = def-name;
 def-name = strdup(dname);
 if (def-name == NULL)
 goto cleanup;
@@ -1095,6 +1102,8 @@ qemuMigrationPrepareAny(struct qemud_driver *driver,
 }
 def = NULL;
 priv = vm-privateData;
+priv-origname = origname;
+origname = NULL;
 
 if (!(mig = qemuMigrationEatCookie(driver, vm, cookiein, cookieinlen,
QEMU_MIGRATION_COOKIE_LOCKSTATE)))
@@ -1175,6 +1184,7 @@ qemuMigrationPrepareAny(struct qemud_driver *driver,
 ret = 0;
 
 cleanup:
+VIR_FREE(origname);
 virDomainDefFree(def);
 VIR_FORCE_CLOSE(dataFD[0]);
 VIR_FORCE_CLOSE(dataFD[1]);
@@ -2542,6 +2552,7 @@ qemuMigrationFinish(struct qemud_driver *driver,
 qemuMigrationCookiePtr mig = NULL;
 virErrorPtr orig_err = NULL;
 int cookie_flags = 0;
+qemuDomainObjPrivatePtr priv = vm-privateData;
 
 VIR_DEBUG(driver=%p, dconn=%p, vm=%p, cookiein=%s, cookieinlen=%d, 
   cookieout=%p, cookieoutlen=%p, flags=%lx, retcode=%d,
@@ -2695,8 +2706,10 @@ endjob:
 }
 
 cleanup:
-if (vm)
+if (vm) {
+VIR_FREE(priv-origname);
 virDomainObjUnlock(vm);
+}
 if (event)
 qemuDomainEventQueue(driver, event);
 qemuMigrationCookieFree(mig);
-- 
1.7.7

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


Re: [libvirt] [RFC] Adding new filesystem 'proxy' to 9p

2011-10-04 Thread M. Mohan Kumar
 
 That is the case if the proxy helper code is perfectly written. I am trying
 to think about the scenario where there is a bug (eg heap corruption /
 stack overflow) which allows a malicious non-root QEMU process to exploit
 the proxy helper to run code that it was *not* intended to run.
 
 If the proxy helper is running root with all capabilities, then a bug in
 the proxy helper can easily turn into a full root exploit.
 
 If the proxy helper starts as root, chroots, and then immediately drops to
 a non-root user, keeping only the CAP_CHOWN, CAP_DAC_OVERRIDE, CAP_FOWNER
 and CAP_DAC_READ_SEARCH capabilities, then a bug in the proxy helper can
 only be used to access files within the designated 9pfs export. If the
 exported directory does not contain any important host system files, then
 it is unlikely it can be used to create a full root exploit.
 

Thanks Daniel, I will add 'capabiliies' to proxy helper. CAP_FOWNER capability 
also need.

I am working on the patches. I will post them in few days.

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


Re: [libvirt] [PATCH] Add AHCI support to qemu driver

2011-10-04 Thread Adam Litke
On Mon, Oct 03, 2011 at 10:46:18PM -0600, Jim Fehlig wrote:
 Adam Litke wrote:
  Hi Jim.  I was testing this and found that I could not boot from the sata 
  disks
  I defined.  When I switch them back to ide, they can be booted just fine.
  Perhaps something is missing from the boot order logic?

 
 Hmm, I didn't notice this in my testing.  sda in the below config
 contained /boot.  Can you provide the domain config?

With the following config, I end up booting from the cdrom.  If I take away the
cdrom definition completely, I am unable to boot at all.

domain type='kvm'
  nameblockPull-test/name
  memory262144/memory
  currentMemory262144/currentMemory
  vcpu1/vcpu
  os
type arch='x86_64' machine='pc-0.13'hvm/type
boot dev='hd'/
  /os
  features
acpi/
apic/
pae/
  /features
  clock offset='utc'/
  on_poweroffdestroy/on_poweroff
  on_rebootrestart/on_reboot
  on_crashrestart/on_crash
  devices

emulator/home/aglitke/src/qemu/x86_64-softmmu/qemu-system-x86_64/emulator
disk type='file' device='disk'
  driver name='qemu' type='qed'/
  source file='/home/aglitke/vm/sata-test/disk1.qed' /
  target dev='sda' bus='sata'/
/disk
disk type='file' device='disk'
  driver name='qemu' type='qed'/
  source file='/home/aglitke/vm/sata-test/disk2.qed' /
  target dev='sdb' bus='sata'/
/disk
disk type='file' device='cdrom'
  driver name='qemu' type='raw'/
  source file='/home/aglitke/vm/images/natty-alternate-amd64.iso' /
  target dev='hda' bus='ide'/
/disk
interface type=network
  source network=default /
/interface
graphics type='vnc' port='-1' autoport='yes'/
  /devices
/domain

-- 
Adam Litke a...@us.ibm.com
IBM Linux Technology Center

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


Re: [libvirt] [PATCH] network: fill in bandwidth from portgroup for all forward modes

2011-10-04 Thread Daniel P. Berrange
On Tue, Oct 04, 2011 at 12:17:27AM -0400, Laine Stump wrote:
 This patch is a fix for:
 
   https://bugzilla.redhat.com/show_bug.cgi?id=743176
 
 which was discovered by Dan Berrange while making bandwidth
 configuration work for LXC guests.
 
 Background: Although virtportprofile data from a network portgroup is
 only applicable for direct mode interfaces, the code that copies
 bandwidth data from the portgroup was also only being executed in the
 case of direct mode interfaces. The result was that interfaces using
 traditional virtual networks (forward mode='nat|route|none'), and
 those using a host bridge for forwarding, would not pick up bandwidth
 data from a portgroup defined in the network.
 
 This patch moves that code outside the conditional, so that bandwidth
 information is *alway* copied from the appropriate portgroup (unless
 the interface definition itself already has bandwidth information,
 which would take precedence over what's in the portgroup anyway).

ACK, this works in my test

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] Make LXC work with new network configuration types

2011-10-04 Thread Daniel P. Berrange
From: Daniel P. Berrange berra...@redhat.com

If using one of the new non-NAT/routed virtual network
configurations, the LXC driver would not know how to
setup the VETH devices. Adding in calls to setup the
actual network configuration at VM startup and cleanup
when shutting down fixes this.

* src/lxc/lxc_driver.c: Setup/cleanup actual net devs
---
 src/lxc/lxc_driver.c |   18 --
 1 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
index c8e6119..c475887 100644
--- a/src/lxc/lxc_driver.c
+++ b/src/lxc/lxc_driver.c
@@ -54,6 +54,7 @@
 #include fdstream.h
 #include domain_audit.h
 #include domain_nwfilter.h
+#include network/bridge_driver.h
 
 #define VIR_FROM_THIS VIR_FROM_LXC
 
@@ -1042,6 +1043,8 @@ static void lxcVmCleanup(lxc_driver_t *driver,
 for (i = 0 ; i  vm-def-nnets ; i++) {
 vethInterfaceUpOrDown(vm-def-nets[i]-ifname, 0);
 vethDelete(vm-def-nets[i]-ifname);
+
+networkReleaseActualDevice(vm-def-nets[i]);
 }
 
 virDomainConfVMNWFilterTeardown(vm);
@@ -1093,7 +1096,14 @@ static int lxcSetupInterfaces(virConnectPtr conn,
 char *parentVeth;
 char *containerVeth = NULL;
 
-switch (def-nets[i]-type) {
+/* If appropriate, grab a physical device from the configured
+ * network's pool of devices, or resolve bridge device name
+ * to the one defined in the network definition.
+ */
+if (networkAllocateActualDevice(def-nets[i])  0)
+goto error_exit;
+
+switch (virDomainNetGetActualType(def-nets[i])) {
 case VIR_DOMAIN_NET_TYPE_NETWORK:
 {
 virNetworkPtr network;
@@ -1110,7 +1120,7 @@ static int lxcSetupInterfaces(virConnectPtr conn,
 break;
 }
 case VIR_DOMAIN_NET_TYPE_BRIDGE:
-bridge = def-nets[i]-data.bridge.brname;
+bridge = virDomainNetGetActualBridgeName(def-nets[i]);
 break;
 
 case VIR_DOMAIN_NET_TYPE_USER:
@@ -1183,6 +1193,10 @@ static int lxcSetupInterfaces(virConnectPtr conn,
 
 error_exit:
 brShutdown(brctl);
+if (rc != 0) {
+for (i = 0 ; i  def-nnets ; i++)
+networkReleaseActualDevice(def-nets[i]);
+}
 return rc;
 }
 
-- 
1.7.6.2

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


Re: [libvirt] [PATCH] qemu: Fix migration with dname

2011-10-04 Thread Daniel P. Berrange
On Tue, Oct 04, 2011 at 12:06:10PM +0200, Jiri Denemark wrote:
 Destination libvirtd remembers the original name in the prepare phase
 and clears it in the finish phase. The original name is used when
 comparing domain name in migration cookie.

What is the actual error we get ?  Is it that the 'Confirm' method
raises an error Incoming cookie data had unexpected name ?



 ---
 Notes:
 Originally, I wanted to transfer the new name in migration cookie but
 that appeared to be much more complicated and it would require adding
 new Confirm API since the current version does not have 'dname'
 parameter.

IIUC, you are trying to fix this, by making sure that the 'Finish'
method encodes the original name in the cookie, not the new name ?



ACK, if my two questions here are correct 

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] network: fill in bandwidth from portgroup for all forward modes

2011-10-04 Thread Laine Stump

On 10/04/2011 08:49 AM, Daniel P. Berrange wrote:

On Tue, Oct 04, 2011 at 12:17:27AM -0400, Laine Stump wrote:

This patch is a fix for:

   https://bugzilla.redhat.com/show_bug.cgi?id=743176

which was discovered by Dan Berrange while making bandwidth
configuration work for LXC guests.

Background: Although virtportprofile data from a network portgroup is
only applicable for direct mode interfaces, the code that copies
bandwidth data from the portgroup was also only being executed in the
case of direct mode interfaces. The result was that interfaces using
traditional virtual networks (forward mode='nat|route|none'), and
those using a host bridge for forwarding, would not pick up bandwidth
data from a portgroup defined in the network.

This patch moves that code outside the conditional, so that bandwidth
information is *alway* copied from the appropriate portgroup (unless
theinterface  definition itself already has bandwidth information,
which would take precedence over what's in the portgroup anyway).

ACK, this works in my test


Pushed. Thanks!

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


Re: [libvirt] [PATCH] qemu: Fix migration with dname

2011-10-04 Thread Jiri Denemark
On Tue, Oct 04, 2011 at 14:03:34 +0100, Daniel P. Berrange wrote:
 On Tue, Oct 04, 2011 at 12:06:10PM +0200, Jiri Denemark wrote:
  Destination libvirtd remembers the original name in the prepare phase
  and clears it in the finish phase. The original name is used when
  comparing domain name in migration cookie.
 
 What is the actual error we get ?  Is it that the 'Confirm' method
 raises an error Incoming cookie data had unexpected name ?

It's actually the Prepare method that fails but the error is the same
(obviously since it comes from EatCookie).

  Notes:
  Originally, I wanted to transfer the new name in migration cookie but
  that appeared to be much more complicated and it would require adding
  new Confirm API since the current version does not have 'dname'
  parameter.
 
 IIUC, you are trying to fix this, by making sure that the 'Finish'
 method encodes the original name in the cookie, not the new name ?

Yes, although the complete picture is that incoming (from the POV of
destination libvirtd) cookie is checked against the original name instead of
the new one and cookies generated by destination libvirtd contain the original
name. It applies to Prepare as well as Finish.

 ACK, if my two questions here are correct

Mostly correct so I take it as ACK :-)

Jirka

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


Re: [libvirt] [PATCH] Make LXC work with new network configuration types

2011-10-04 Thread Laine Stump

On 10/04/2011 08:52 AM, Daniel P. Berrange wrote:

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

If using one of the new non-NAT/routed virtual network
configurations, the LXC driver would not know how to
setup the VETH devices. Adding in calls to setup the
actual network configuration at VM startup and cleanup
when shutting down fixes this.

* src/lxc/lxc_driver.c: Setup/cleanup actual net devs


ACK. This looks like proper usage of the functions in the proper place.

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


Re: [libvirt] [PATCH] qemu: Fix migration with dname

2011-10-04 Thread Jiri Denemark
On Tue, Oct 04, 2011 at 15:16:28 +0200, Jiri Denemark wrote:
 On Tue, Oct 04, 2011 at 14:03:34 +0100, Daniel P. Berrange wrote:
  On Tue, Oct 04, 2011 at 12:06:10PM +0200, Jiri Denemark wrote:
   Destination libvirtd remembers the original name in the prepare phase
   and clears it in the finish phase. The original name is used when
   comparing domain name in migration cookie.
  
  What is the actual error we get ?  Is it that the 'Confirm' method
  raises an error Incoming cookie data had unexpected name ?
 
 It's actually the Prepare method that fails but the error is the same
 (obviously since it comes from EatCookie).
 
   Notes:
   Originally, I wanted to transfer the new name in migration cookie but
   that appeared to be much more complicated and it would require adding
   new Confirm API since the current version does not have 'dname'
   parameter.
  
  IIUC, you are trying to fix this, by making sure that the 'Finish'
  method encodes the original name in the cookie, not the new name ?
 
 Yes, although the complete picture is that incoming (from the POV of
 destination libvirtd) cookie is checked against the original name instead of
 the new one and cookies generated by destination libvirtd contain the original
 name. It applies to Prepare as well as Finish.
 
  ACK, if my two questions here are correct
 
 Mostly correct so I take it as ACK :-)

So I pushed this, thanks.

Jirka

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


Re: [libvirt] bug: try to take disk snapshot for LVM2 Volume

2011-10-04 Thread MATSUDA, Daiki
 On 09/29/2011 11:26 PM, MATSUDA, Daiki wrote:
 I tried the new snapshot function implemented by Eric Blake.

 It works very well for QCOW2 disk image system.
 But I often use LVM2 volume for QEMU virtual machines and tried to take
 disk snapshot by virsh command ( snapshot-create DOMNAME --disk-only).
 So, finally qemu monitor command 'snapshot_blkdev' accepts the LVM2
 volume and create QCOW2 snapshot image. In addition, domain's
 configuration file is replaced to use snapshot disk image instead of
 LVM2 volume.
 
 It sounds like virsh did what it was told, but that you told it so
 little information that it had to fill in the gaps and choose its own
 destination file name (hence the .1317357844 suffix after the action).
 Your situation sounds like a case where you may want a bit more control
 over the destination file name.

virsh outputs
virsh # snapshot-create LVM2_dom --disk-only
Domain snapshot 1317357844 created

And I confirmed that the qcow2 image file is created under /dev/VG1
# file /dev/VG1/LVM2_dom.1317357844
/dev/VG1/LVM2_dom.1317686816: Qemu Image, Format: Qcow , Version: 2

 configuration file
 from
 
 disk type='block' device='disk
 driver name='qemu' type='raw' cache='none'/
 source dev='dev/VG1/LVM2_dom'/
 

 to
 disk type='block' device='disk
 driver name='qemu' type='qcow2' cache='none'/
 source dev='dev/VG1/LVM2_dom.1317357844'/
 
 Are you pasting literal chunks, or retyping this?  You're missing the /
 in front of dev/VG1, so I can't help but wonder what else might have
 been mistyped.

I am sorry. They are my mistyping and correct is '/dev/VG1/LVM2_dom' and
'/dev/VG1/LVM2_dom.1317357844'.

 After then, the domain runs well till it is shutdowned.
 
 I'm surprised that it got that far - generally, libvirt can't create
 random regular files under the /dev/VG1/ device-mapper hierarchy, and if
 a file can't be created, then what was open() doing, and what did qemu
 actually do?  It may be worth looking at lsof says that qemu has open,
 if you still have it running.  Or it may be that you've found a bug in
 libvirt and/or qemu for not accurately reporting failure to create the
 snapshot image.

But in reality the file is created by qemu-kvm with snapshot_blkdev in
qemu-monitor command. I use libvirt-0.9.6 and qemu-kvm-0.12.12.1.2-2.160
and August's snapshot.

 I think we need to step back a bit and look at the bigger picture.  Do
 you want your new qcow2 file to be its own LVM volume (in which case,
 I'd suggest that you pre-create the LVM volume, and pass in the file
 name within /dev/VG1/ of the existing block device to use), or are you
 okay with it being a regular file (in which case, I'd suggest that you
 do not pre-create the file, but that you still pass in the desired
 filename to some absolute path that lives outside of /dev/)?

No, I do not want qcow2 file on LVM volume. I found the bug at simply
tesing. I will never create the snapshot with 'snapshot-create ...
--disk-only' for LVM2 volume, but users will try... So, I think that it
is better not to refuse in libvirt.

 Either way, it sounds like you do _not_ want libvirt to be generating
 its own filename, since libvirt only knows how to generate a name in the
 same directory as the snapshot is taking place, but your lvm is in a
 special directory.  To do this, you either need to create an XML file
 yourself, and call 'virsh snapshot-create dom --disk-only file', or you
 need to have virsh create the xml for you with 'virsh snapshot-create-as
 dom --disk-only vda,file=/path/to/file'.  You can see the xml that
 snapshot-create-as would generate (in case you need to further fine-tune
 it for your own use in snapshot-create) via the --print-xml option.
 
 I started the
 domain, but it does not with following error
 virtsh # start LVM2_dom
 error: Failed to start domain LVM2_dom
 error: 内部エラー Process exited while reading console log output: char
 device redirected to /dev/pts/7
 qemu: could not open disk image /dev/VG1/LVM2_dom.1317357844: Invalid
 argument.
 
 That makes sense, if that file doesn't exist (but why qemu didn't reject
 the snapshot in the first place still remains to be investigated).
 

 I think that if the volume but qcow2 is given libvirt should be refuse,
 
 No, qemu does just fine with a non-qcow2 backing file.  The real problem
 here is that the new qcow2 file was not created, not the type of the
 original file.

At least its phenomenon is reproduced easily. So I hope you test.

 e.g. in qemuDomainSnapshotCreateDiskActive() with voulme driver type.
 But currently the structures concerning with snapshot or disk has no
 member to hold such a volume driver information. In addition, as we want
 to add the LVM2 and other volume snapshot function, we hope you add its
 information and fix.
 
 Yes, I have much longer-term plans for refactoring device snapshots to
 move the work into more virStorageVolPtr operations, and teach
 virDomainSnapshotCreateXML to reuse virStorageVol actions rather 

Re: [libvirt] [PATCH] qemu: Fix migration with dname

2011-10-04 Thread Eric Blake

On 10/04/2011 07:48 AM, Jiri Denemark wrote:


IIUC, you are trying to fix this, by making sure that the 'Finish'
method encodes the original name in the cookie, not the new name ?


Yes, although the complete picture is that incoming (from the POV of
destination libvirtd) cookie is checked against the original name instead of
the new one and cookies generated by destination libvirtd contain the original
name. It applies to Prepare as well as Finish.


ACK, if my two questions here are correct


Mostly correct so I take it as ACK :-)


Quick questions (from a latecomer to the thread): what happens if I use 
both the @dname and @dxml arguments?  Are we properly requiring that the 
new name in both arguments match, and rejecting the migration as 
impossible otherwise (since you can't request two different names), or 
are we having one of the two names take priority over the other?


Also, if @dname is NULL but @dxml is provided, I think that we currently 
refuse migration to a server that only understands v2 migration (since 
only v3 can take @dxml).  Can @dxml in isolation be used to change the 
name, without the use of @dname?


--
Eric Blake   ebl...@redhat.com+1-801-349-2682
Libvirt virtualization library http://libvirt.org

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


[libvirt] [PATCH] build: fix 'make distcheck' with pdwtags installed

2011-10-04 Thread Eric Blake
I am getting this failure with 'make distcheck':

  GEN../../src/remote_protocol-structs
/bin/sh: ../../src/remote_protocol-structs-t: Permission denied
make[4]: *** [../../src/remote_protocol-structs] Error 1

since it attempts a sub-run of a VPATH 'make check' where $(srcdir)
is intentionally read-only.  I'm not sure which commit introduced
the problem, although I suspect it was around 62dee6f when I
refactored protocol struct checking to be more powerful.

$(@F) is required by POSIX, and although it is not yet portable
to all make implementations, we already require GNU make.

* src/Makefile.am (PDWTAGS): Generate temp file into current
directory, since $(srcdir) is read-only during distcheck.
---
 src/Makefile.am |4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/Makefile.am b/src/Makefile.am
index 738ee91..9650139 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -255,9 +255,9 @@ PDWTAGS = \
-e 'exit 8;'\
-e '  }'\
-e '}'  \
-$@-t; \
+$(@F)-t;  \
  case $$? in 8) exit 0;; 0) ;; *) exit 1;; esac;   \
- diff -u $@-t $@; st=$$?; rm -f $@-t; exit $$st;   \
+ diff -u $(@F)-t $@; st=$$?; rm -f $(@F)-t; exit $$st; \
else\
  echo 'WARNING: you lack pdwtags; skipping the $@ test' 2;   \
  echo 'WARNING: install the dwarves package to get pdwtags' 2; \
-- 
1.7.4.4

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


[libvirt] [PATCH] init: raise default system aio limits

2011-10-04 Thread Eric Blake
https://bugzilla.redhat.com/show_bug.cgi?id=740899 documents that
if qemu uses aio=native for its disks, then it consumes 128 aio
requests per disk.  On a host with multiple guests, this can quickly
run out of kernel aio requests with the default aio-max-nr of
65536.  Kernel developers have confirmed that there is no up-front
cost to raising this limit (a larger limit merely implies that more
aio requests can be issued in parallel, which in turn will result
in more kernel memory allocation if the system really does use that
many requests).  Since the system default limit prevents 256 disks,
which is well within libvirt's current scalability, this patch
installs a file to raise the limit and document it in case a system
administrator has further cause to tune the limit.  The install
only works on platforms new enough to source /etc/sysctl.d/*
alongside /etc/sysctl.conf (F14 and RHEL 6).

* daemon/libvirtd.sysctl: New file.
* daemon/Makefile.am (EXTRA_DIST): Ship it.
(install-init, uninstall-init): Install it.
* libvirt.spec.in (%files): Include it in rpm.
---

So far, I've confirmed that this passes 'make distcheck' (well, once
https://www.redhat.com/archives/libvir-list/2011-October/msg00074.html
is applied) and 'make rpm', although I didn't actually try installing
the resulting rpm nor rebooting a system to see if it takes effect.

 daemon/Makefile.am |   11 ---
 daemon/libvirtd.sysctl |8 
 libvirt.spec.in|5 +
 3 files changed, 21 insertions(+), 3 deletions(-)
 create mode 100644 daemon/libvirtd.sysctl

diff --git a/daemon/Makefile.am b/daemon/Makefile.am
index 690bf85..1cf2b73 100644
--- a/daemon/Makefile.am
+++ b/daemon/Makefile.am
@@ -38,6 +38,7 @@ EXTRA_DIST =  \
libvirtd.policy-1   \
libvirtd.sasl   \
libvirtd.sysconf\
+   libvirtd.sysctl \
libvirtd.aug\
libvirtd.logrotate.in   \
libvirtd.qemu.logrotate.in  \
@@ -252,16 +253,20 @@ install-logrotate: $(LOGROTATE_CONFS)

 if LIBVIRT_INIT_SCRIPT_RED_HAT
 install-init: libvirtd.init
-   mkdir -p $(DESTDIR)$(sysconfdir)/rc.d/init.d
+   mkdir -p $(DESTDIR)$(sysconfdir)/rc.d/init.d \
+ $(DESTDIR)$(sysconfdir)/sysconfig \
+ $(DESTDIR)$(sysconfdir)/sysctl.d
$(INSTALL_SCRIPT) libvirtd.init \
  $(DESTDIR)$(sysconfdir)/rc.d/init.d/libvirtd
-   mkdir -p $(DESTDIR)$(sysconfdir)/sysconfig
$(INSTALL_DATA) $(srcdir)/libvirtd.sysconf \
  $(DESTDIR)$(sysconfdir)/sysconfig/libvirtd
+   $(INSTALL_DATA) $(srcdir)/libvirtd.sysctl \
+ $(DESTDIR)$(sysconfdir)/sysctl.d/libvirtd

 uninstall-init:
rm -f $(DESTDIR)$(sysconfdir)/rc.d/init.d/libvirtd \
-   $(DESTDIR)$(sysconfdir)/sysconfig/libvirtd
+   $(DESTDIR)$(sysconfdir)/sysconfig/libvirtd \
+   $(DESTDIR)$(sysconfdir)/sysctl.d/libvirtd

 BUILT_SOURCES += libvirtd.init

diff --git a/daemon/libvirtd.sysctl b/daemon/libvirtd.sysctl
new file mode 100644
index 000..3c70884
--- /dev/null
+++ b/daemon/libvirtd.sysctl
@@ -0,0 +1,8 @@
+# The kernel allocates aio memory on demand, and this number limits the
+# number of parallel aio requests; the only drawback of a larger limit is
+# that a malicious guest could issue parallel requests to cause the kernel
+# to set aside memory.  Set this number at least as large as
+#   128 * (number of virtual disks on the host)
+# Libvirt uses a default of 1M requests to allow 8k disks, with at most
+# 64M of kernel memory if all disks hit an aio request at the same time.
+fs.aio-max-size = 1048576
diff --git a/libvirt.spec.in b/libvirt.spec.in
index b87e3f6..9f5797a 100644
--- a/libvirt.spec.in
+++ b/libvirt.spec.in
@@ -962,6 +962,11 @@ fi
 %doc daemon/libvirtd.upstart
 %config(noreplace) %{_sysconfdir}/sysconfig/libvirtd
 %config(noreplace) %{_sysconfdir}/libvirt/libvirtd.conf
+%if 0%{?fedora} = 14 || 0%{?rhel} = 6
+%config(noreplace) %{_sysconfdir}/sysctl.d/libvirtd
+%else
+rm -f $RPM_BUILD_ROOT%{_sysconfdir}/sysctl.d/libvirtd
+%endif
 %if %{with_dtrace}
 %{_datadir}/systemtap/tapset/libvirtd.stp
 %endif
-- 
1.7.4.4

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


Re: [libvirt] [RFC] security_dac: don't chown iso file

2011-10-04 Thread Serge E. Hallyn
Quoting Serge E. Hallyn (serge.hal...@canonical.com):
 isos are read-only, so libvirt doesn't need to chown them.  In one of
 our testing setups, libvirt uses mirrorred isos.  Since libvirt chowns
 the files, (and especially does not chown them back) the mirror refuses
 to update the iso.
 
 This patch prevents libvirt from chowning files.
 
 Does this seem reasonable?

Hi,

any feedback on this?  Does it seem ok?

thanks,
-serge

 Signed-off-by: Serge Hallyn serge.hal...@canonical.com
 ---
  src/security/security_dac.c |2 ++
  1 files changed, 2 insertions(+), 0 deletions(-)
 
 diff --git a/src/security/security_dac.c b/src/security/security_dac.c
 index af02236..e7db324 100644
 --- a/src/security/security_dac.c
 +++ b/src/security/security_dac.c
 @@ -555,6 +555,8 @@ virSecurityDACSetSecurityAllLabel(virSecurityManagerPtr 
 mgr,
  /* XXX fixme - we need to recursively label the entire tree :-( */
  if (vm-def-disks[i]-type == VIR_DOMAIN_DISK_TYPE_DIR)
  continue;
 + if (vm-def-disks[i]-device == VIR_DOMAIN_DISK_DEVICE_CDROM)
 + continue;
  if (virSecurityDACSetSecurityImageLabel(mgr,
  vm,
  vm-def-disks[i])  0)
 -- 
 1.7.5.4
 
 --
 libvir-list mailing list
 libvir-list@redhat.com
 https://www.redhat.com/mailman/listinfo/libvir-list

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


[libvirt] [PATCH 0/2] qemu: fix disk error_policy problems

2011-10-04 Thread Laine Stump
These two patches are related to:

https://bugzilla.redhat.com/show_bug.cgi?id=730909

The first removes the obvious problem of attempting to feed qemu
invalid options. The second makes it possible to specify a different
policy for read errors and write errors. 1/2 is written such that 2/2
will apply with minimal rewriting of existing code (that's why the
qemu_command.c part may seem a bit obtuse at first).

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


[libvirt] [PATCH 2/2] qemu: add separate rerror_policy for disk errors

2011-10-04 Thread Laine Stump
libvirt's XML only had a single attribute, error_policy to control
both read and write error policy, but qemu has separate settings for
these. In one case (enospc) a policy is allowed for write errors but
not read errors.

This patch adds a separate attribute that sets only the read error
policy. If just error_policy is set, it will apply to both read and
write error policy (previous behavior), but if the new rerror_policy
attribute is set, it will override error_policy for read errors only.
---
 docs/formatdomain.html.in |   16 +---
 src/conf/domain_conf.c|   16 
 src/conf/domain_conf.h|3 ++-
 src/qemu/qemu_command.c   |5 -
 4 files changed, 35 insertions(+), 5 deletions(-)

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 593adcb..5265b21 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -1008,9 +1008,19 @@
   /li
   li
 The optional codeerror_policy/code attribute controls
-how the hypervisor will behave on an error, possible
-values are stop, ignore, and enospace.
-span class=sinceSince 0.8.0/span
+how the hypervisor will behave on a disk read or write
+error, possible values are stop, ignore, and
+enospace.span class=sinceSince 0.8.0/span There is
+also an optional codererror_policy/code that controls
+behavior for read errors only.span class=sinceSince
+0.9.7/space. If no rerror_policy is given, error_policy
+is used for both read and write errors. If rerror_policy
+is given, it overrides the codeerror_policy/code for
+read errors. Also note that enospace is not a valid
+policy for read errors, so if codeerror_policy/code is
+set to enospace and no codererror_policy/code is
+given, the read error policy will be automatically set to
+ignore.
   /li
   li
 The optional codeio/code attribute controls specific
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index f9007ce..95e1a9a 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -2297,6 +2297,7 @@ virDomainDiskDefParseXML(virCapsPtr caps,
 char *bus = NULL;
 char *cachetag = NULL;
 char *error_policy = NULL;
+char *rerror_policy = NULL;
 char *iotag = NULL;
 char *ioeventfd = NULL;
 char *event_idx = NULL;
@@ -2416,6 +2417,7 @@ virDomainDiskDefParseXML(virCapsPtr caps,
 driverType = virXMLPropString(cur, type);
 cachetag = virXMLPropString(cur, cache);
 error_policy = virXMLPropString(cur, error_policy);
+rerror_policy = virXMLPropString(cur, rerror_policy);
 iotag = virXMLPropString(cur, io);
 ioeventfd = virXMLPropString(cur, ioeventfd);
 event_idx = virXMLPropString(cur, event_idx);
@@ -2560,6 +2562,16 @@ virDomainDiskDefParseXML(virCapsPtr caps,
 goto error;
 }
 
+if (rerror_policy 
+(((def-rerror_policy
+   = virDomainDiskErrorPolicyTypeFromString(error_policy))  0) ||
+ (def-rerror_policy == VIR_DOMAIN_DISK_ERROR_POLICY_ENOSPACE))) {
+virDomainReportError(VIR_ERR_INTERNAL_ERROR,
+ _(unknown disk read error policy '%s'),
+ rerror_policy);
+goto error;
+}
+
 if (iotag) {
 if ((def-iomode = virDomainDiskIoTypeFromString(iotag))  0 ||
 def-iomode == VIR_DOMAIN_DISK_IO_DEFAULT) {
@@ -2667,6 +2679,7 @@ cleanup:
 VIR_FREE(driverName);
 VIR_FREE(cachetag);
 VIR_FREE(error_policy);
+VIR_FREE(rerror_policy);
 VIR_FREE(iotag);
 VIR_FREE(ioeventfd);
 VIR_FREE(event_idx);
@@ -9127,6 +9140,7 @@ virDomainDiskDefFormat(virBufferPtr buf,
 const char *bus = virDomainDiskBusTypeToString(def-bus);
 const char *cachemode = virDomainDiskCacheTypeToString(def-cachemode);
 const char *error_policy = 
virDomainDiskErrorPolicyTypeToString(def-error_policy);
+const char *rerror_policy = 
virDomainDiskErrorPolicyTypeToString(def-rerror_policy);
 const char *iomode = virDomainDiskIoTypeToString(def-iomode);
 const char *ioeventfd = virDomainIoEventFdTypeToString(def-ioeventfd);
 const char *event_idx = 
virDomainVirtioEventIdxTypeToString(def-event_idx);
@@ -9177,6 +9191,8 @@ virDomainDiskDefFormat(virBufferPtr buf,
 virBufferAsprintf(buf,  cache='%s', cachemode);
 if (def-error_policy)
 virBufferAsprintf(buf,  error_policy='%s', error_policy);
+if (def-rerror_policy)
+virBufferAsprintf(buf,  rerror_policy='%s', rerror_policy);
 if (def-iomode)
 virBufferAsprintf(buf,  io='%s', iomode);
 if (def-ioeventfd)
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index bc41d34..35eacc7 100644
--- 

[libvirt] [PATCH 1/2] qemu: correct misspelled 'enospc' option, and only use for werror

2011-10-04 Thread Laine Stump
This resolves:

 https://bugzilla.redhat.com/show_bug.cgi?id=730909

When support for setting the qemu disk error policy to 'enospc' was
added, it was inadvertantly as enospace. This patch corrects that on
the qemu commandline (while retaining the enospace spelling for
libvirt's XML.

Also, while examining the qemu source, I found that enospc is not
allowed for the read error policy, only for write error policy (makes
sense). Since libvirt currently only has a single error policy
setting, when enospace is selected, the read error policy is set to
ignore.
---
 src/qemu/qemu_command.c |   22 ++
 1 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index ff83e2d..123bcab 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -1692,11 +1692,25 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk,
 }
 
 if (qemuCapsGet(qemuCaps, QEMU_CAPS_MONITOR_JSON)) {
-if (disk-error_policy) {
-virBufferAsprintf(opt, ,werror=%s,rerror=%s,
-  
virDomainDiskErrorPolicyTypeToString(disk-error_policy),
-  
virDomainDiskErrorPolicyTypeToString(disk-error_policy));
+const char *wpolicy = NULL, *rpolicy = NULL;
+
+if (disk-error_policy)
+wpolicy = virDomainDiskErrorPolicyTypeToString(disk-error_policy);
+if (!rpolicy)
+rpolicy = wpolicy;
+
+if (disk-error_policy == VIR_DOMAIN_DISK_ERROR_POLICY_ENOSPACE) {
+/* in the case of enospace, the option is spelled differently in 
qemu,
+ * and it's only valid for werror, not for rerror.
+ */
+wpolicy=enospc;
+rpolicy=ignore;
 }
+
+if (wpolicy)
+virBufferAsprintf(opt, ,werror=%s, wpolicy);
+if (rpolicy)
+virBufferAsprintf(opt, ,rerror=%s, rpolicy);
 }
 
 if (disk-iomode) {
-- 
1.7.3.4

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


Re: [libvirt] [PATCH] init: raise default system aio limits

2011-10-04 Thread Laine Stump

On 10/04/2011 12:59 PM, Eric Blake wrote:

https://bugzilla.redhat.com/show_bug.cgi?id=740899 documents that
if qemu uses aio=native for its disks, then it consumes 128 aio
requests per disk.  On a host with multiple guests, this can quickly
run out of kernel aio requests with the default aio-max-nr of
65536.  Kernel developers have confirmed that there is no up-front
cost to raising this limit (a larger limit merely implies that more
aio requests can be issued in parallel, which in turn will result
in more kernel memory allocation if the system really does use that
many requests).  Since the system default limit prevents 256 disks,
which is well within libvirt's current scalability, this patch
installs a file to raise the limit and document it in case a system
administrator has further cause to tune the limit.  The install
only works on platforms new enough to source /etc/sysctl.d/*
alongside /etc/sysctl.conf (F14 and RHEL 6).


ACK. This all makes sense and seems fairly thoroughly researched. (Would 
be good to actually try it, though)


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


Re: [libvirt] [PATCH] Document that ff callbacks need to be invoked from a clean stack

2011-10-04 Thread Guido Günther
On Tue, Oct 04, 2011 at 09:00:17AM +0100, Daniel P. Berrange wrote:
 On Mon, Oct 03, 2011 at 10:41:17PM +0200, Guido Günther wrote:
  On Tue, Aug 16, 2011 at 02:24:53PM +0200, Guido Günther wrote:
   Hi Daniel,
   On Sat, Aug 13, 2011 at 08:57:45PM -0700, Daniel P. Berrange wrote:
On Fri, Aug 12, 2011 at 11:54:28PM +0200, Guido Günther wrote:
   [..snip..] 
In the default libvirt event loop, the 'ff' callback is always invoked
from a clean stack in the event loop, so you never have this problem
with re-entrancy.

 Working around this by removing the locks from
 virNetSocketRemoveIOCallback leads to another deadlock:

Yeah this is not a viable approach. 
   Sure. This was only to see what else fails.
   
 
 I didn't see a simple way to fix this but would welcome any 
 suggestions.

IMHO we just have to document that event loop implementations
should make sure that the 'ff' callbacks are always invoked
from a clean stack. In the case of virt-viewer, this means
changing it to register a g_idle  callback function to invoke
the 'ff' callback.
   
   Patch for virt-viewer attached. I'll come up with a doc patch for
   libvirt once I have a bit more time.
  
  As promised here's the patch for libvirt. O.k. to apply?
  Cheers,
   -- Guido
 
  From c2e2be4e377871ace12bb7adfde130e9b8779ab5 Mon Sep 17 00:00:00 2001
  From: =?UTF-8?q?Guido=20G=C3=BCnther?= a...@sigxcpu.org
  Date: Mon, 3 Oct 2011 22:24:13 +0200
  Subject: [PATCH] Document that ff callbacks need to be invoked from a clean
   stack.
  
  ---
   include/libvirt/libvirt.h.in |6 --
   1 files changed, 4 insertions(+), 2 deletions(-)
  
  diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
  index a3c581d..bd7a0f7 100644
  --- a/include/libvirt/libvirt.h.in
  +++ b/include/libvirt/libvirt.h.in
  @@ -2243,13 +2243,15 @@ typedef void (*virEventHandleCallback)(int watch, 
  int fd, int events, void *opaq
* @opaque: user data to pass to the callback
* @ff: the callback invoked to free opaque data blob
*
  - * Part of the EventImpl, this callback Adds a file handle callback to
  + * Part of the EventImpl, this callback adds a file handle callback to
* listen for specific events. The same file handle can be registered
* multiple times provided the requested event sets are non-overlapping
*
* If the opaque user data requires free'ing when the handle
* is unregistered, then a 2nd callback can be supplied for
  - * this purpose.
  + * this purpose. This callback needs to be invoked from a clean stack.
  + * If 'ff' callbacks are invoked directly from the virEventRemoveHandleFunc
  + * they will likely deadlock in libvirt.
*
* Returns a handle watch number to be used for updating
* and unregistering for events
 
 
 ACK
Pushed. Thanks.
 -- Guido

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


Re: [libvirt] [RFC PATCH] snapshot: better virsh handling of missing current, parent

2011-10-04 Thread Laine Stump

On 09/30/2011 12:19 PM, Eric Blake wrote:

Previously, virsh 'snapshot-parent' and 'snapshot-current' were
completely silent in the case where the code conclusively proved
there was no parent or current snapshot, but differed in exit
status; this silence caused some confusion on whether the commands
worked.  Furthermore, commit d1be48f introduced a regression where
snapshot-parent would leak output about an unknown function, but
only on the first attempt, when talking to an older server that
lacks virDomainSnapshotGetParent.  This changes things to consistenly
report an error message and exit with status 1 when no snapshot
exists, and to avoid leaking unknown function warnings when using
fallbacks.

* tools/virsh.c (vshGetSnapshotParent): Alter signature, to
distinguish between real error and missing parent.  Don't pollute
last_error on success.
(cmdSnapshotParent): Adjust caller.  Always output message on
failure.
(vshGetSnapshotParent): Adjust caller.
(cmdSnapshotCurrent): Always output message on failure.
---

This patch is an RFC because of backwards-compatibility concerns:

Currently, snapshot-current outputs nothing but has status 0 if
there is no current snapshot; but snapshot-parent outputs nothing
but has status 1 if there is no parent snapshot.  Either way, we
have an inconsistency that needs to be fixed, and gaining consistency
means breaking backwards compatibility with at least one command.

Approach 1 (this patch): change snapshot-parent and snapshot-current
to always output something, whether to stdout on success or to
stderr on failure, with lack of a snapshot being considered a
failure (where snapshot-current used to treat it as success).

Approach 2 (not written) since snapshot-current existed much longer,
makesnapshot-parent consistent by giving status 0 when it is silent.

Preferences?  (I guess mine is approach 1, by evidence of this patch).



The code all looks fine, so ACK to that. Not being very familiar with 
the snapshot stuff, I can't say that I have an valid opinion on whether 
it's better to log an error or exit silently when there is no parent. I 
guess I would defer to your opinion, since you're the person who's spent 
the most time dealing with snapshots lately :-)



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


Re: [libvirt] [PATCH 1/2] qemu: correct misspelled 'enospc' option, and only use for werror

2011-10-04 Thread Eric Blake

On 10/04/2011 12:35 PM, Laine Stump wrote:

This resolves:

  https://bugzilla.redhat.com/show_bug.cgi?id=730909

When support for setting the qemu disk error policy to 'enospc' was
added, it was inadvertantly as enospace. This patch corrects that on


spelling and grammar

s/inadvertantly/inadvertently spelled/


the qemu commandline (while retaining the enospace spelling for
libvirt's XML.

Also, while examining the qemu source, I found that enospc is not
allowed for the read error policy, only for write error policy (makes
sense). Since libvirt currently only has a single error policy
setting, when enospace is selected, the read error policy is set to
ignore.
---
  src/qemu/qemu_command.c |   22 ++
  1 files changed, 18 insertions(+), 4 deletions(-)


ACK.

--
Eric Blake   ebl...@redhat.com+1-801-349-2682
Libvirt virtualization library http://libvirt.org

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


Re: [libvirt] [PATCH 1/2] snapshot: add REVERT_FORCE to API

2011-10-04 Thread Laine Stump

On 09/30/2011 02:52 PM, Eric Blake wrote:

Although reverting to a snapshot is a form of data loss, this is
normally expected.  However, there are two cases where additional
surprises (failure to run the reverted state, or a break in
connectivity to the domain) can come into play.  Requiring extra
acknowledgment in these cases will make it less likely that
someone can get into an unrecoverable state due to a default revert.

Also create a new error code, so users can distinguish when forcing
would make a difference, rather than having to blindly request force.

* include/libvirt/libvirt.h.in (VIR_DOMAIN_SNAPSHOT_REVERT_FORCE):
New flag.
* src/libvirt.c (virDomainRevertToSnapshot): Document it.
* include/libvirt/virterror.h (VIR_ERR_SNAPSHOT_REVERT_RISKY): New
error value.
* src/util/virterror.c (virErrorMsg): Implement it.
* tools/virsh.c (cmdDomainSnapshotRevert): Add --force to virsh.
* tools/virsh.pod (snapshot-revert): Document it.
---
  include/libvirt/libvirt.h.in |1 +
  include/libvirt/virterror.h  |2 ++
  src/libvirt.c|   22 ++
  src/util/virterror.c |6 ++
  tools/virsh.c|   19 ++-
  tools/virsh.pod  |   17 +
  6 files changed, 66 insertions(+), 1 deletions(-)

diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
index 3c7f278..7403a9a 100644
--- a/include/libvirt/libvirt.h.in
+++ b/include/libvirt/libvirt.h.in
@@ -2736,6 +2736,7 @@ virDomainSnapshotPtr 
virDomainSnapshotGetParent(virDomainSnapshotPtr snapshot,
  typedef enum {
  VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING = 1  0, /* Run after revert */
  VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED  = 1  1, /* Pause after revert */
+VIR_DOMAIN_SNAPSHOT_REVERT_FORCE   = 1  2, /* Allow risky reverts */
  } virDomainSnapshotRevertFlags;

  /* Revert the domain to a point-in-time snapshot.  The
diff --git a/include/libvirt/virterror.h b/include/libvirt/virterror.h
index 0aae622..0c98014 100644
--- a/include/libvirt/virterror.h
+++ b/include/libvirt/virterror.h
@@ -237,6 +237,8 @@ typedef enum {
 the given driver */
  VIR_ERR_STORAGE_PROBE_FAILED = 75,  /* storage pool proble failed */
  VIR_ERR_STORAGE_POOL_BUILT = 76,/* storage pool already built */
+VIR_ERR_SNAPSHOT_REVERT_RISKY = 77, /* force was not requested for a
+   risky domain snapshot revert */
  } virErrorNumber;

  /**
diff --git a/src/libvirt.c b/src/libvirt.c
index 2b2f6be..c2aabf7 100644
--- a/src/libvirt.c
+++ b/src/libvirt.c
@@ -16373,6 +16373,28 @@ error:
   * into an inactive state, so transient domains require the use of one
   * of these two flags.
   *
+ * Reverting to any snapshot discards all configuration changes made since
+ * the last snapshot.  Additionally, reverting to a snapshot from a running
+ * domain is a form of data loss, since it discards whatever is in the
+ * guest's RAM at the time.  However, the very nature of keeping snapshots
+ * implies the intent to roll back state, so no additional confirmation is
+ * normally required for these effects.
+ *
+ * However, there are two particular situations where reverting will
+ * be refused by default, and where @flags must include
+ * VIR_DOMAIN_SNAPSHOT_REVERT_FORCE to acknowledge the risks.  1) Any
+ * attempt to revert to a snapshot that lacks the metadata to perform
+ * ABI compatibility checks (generally the case for snapshots that
+ * lack a fulldomain  when listed by virDomainSnapshotGetXMLDesc(),
+ * such as those created prior to libvirt 0.9.5).  2) Any attempt to
+ * revert a running domain to an active state that requires starting a
+ * new hypervisor instance rather than reusing the existing hypervisor
+ * (since this would terminate all connections to the domain, such as
+ * such as VNC or Spice graphics) - this condition arises from active
+ * snapshots that are provably ABI incomaptible, as well as from
+ * inactive snapshots with a request to start the domain after the
+ * revert.
+ *
   * Returns 0 if the creation is successful, -1 on error.
   */
  int
diff --git a/src/util/virterror.c b/src/util/virterror.c
index 26c4981..5006fa2 100644
--- a/src/util/virterror.c
+++ b/src/util/virterror.c
@@ -1204,6 +1204,12 @@ virErrorMsg(virErrorNumber error, const char *info)
  else
  errmsg = _(argument unsupported: %s);
  break;
+case VIR_ERR_SNAPSHOT_REVERT_RISKY:
+if (info == NULL)
+errmsg = _(revert requires force);
+else
+errmsg = _(revert requires force: %s);
+break;
  }
  return (errmsg);
  }
diff --git a/tools/virsh.c b/tools/virsh.c
index 655378c..4e79325 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -13571,6 +13571,7 @@ static const vshCmdOptDef opts_snapshot_revert[] = {
  {snapshotname, VSH_OT_DATA, VSH_OFLAG_REQ, N_(snapshot name)},
  

Re: [libvirt] [PATCH 2/2] snapshot: enforce REVERT_FORCE on qemu

2011-10-04 Thread Laine Stump

On 09/30/2011 02:52 PM, Eric Blake wrote:

Implements the documentation for snapshot revert vs. force.

Part of the patch tightens existing behavior (previously, reverting
to an old snapshot withoutdomain  was blindly attempted, now it
requires force), while part of it relaxes behavior (previously, it
was not possible to revert an active domain to an ABI-incompatible
active snapshot, now force allows this transition).

* src/qemu/qemu_driver.c (qemuDomainRevertToSnapshot): Check for
risky situations, and allow force to get past them.
---
  src/qemu/qemu_driver.c |   47 +--
  1 files changed, 37 insertions(+), 10 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 5110102..efd60a7 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -9753,7 +9753,8 @@ static int 
qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
   * 7. paused   -  inactive: EVENT_STOPPED
   * 8. paused   -  running:  EVENT_RESUMED
   * 9. paused   -  paused:   none
- * Also, several transitions occur even if we fail partway through.
+ * Also, several transitions occur even if we fail partway through,
+ * and use of FORCE can cause multiple transitions.
   */

  qemuDriverLock(driver);
@@ -9789,6 +9790,24 @@ static int 
qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
yet));
  goto cleanup;
  }
+if (!(flags  VIR_DOMAIN_SNAPSHOT_REVERT_FORCE)) {
+if (!snap-def-dom) {
+qemuReportError(VIR_ERR_SNAPSHOT_REVERT_RISKY,
+_(snapshot lacks domain '%s' rollback details),
+snap-def-name);
+goto cleanup;
+}
+if (virDomainObjIsActive(vm)
+!(snap-def-state == VIR_DOMAIN_RUNNING
+  || snap-def-state == VIR_DOMAIN_PAUSED)
+(flags  (VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING |
+  VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED))) {
+qemuReportError(VIR_ERR_SNAPSHOT_REVERT_RISKY,
+_(must respawn qemu to start inactive snapshot));
+goto cleanup;
+}
+}
+

  if (vm-current_snapshot) {
  vm-current_snapshot-def-current = false;
@@ -9818,11 +9837,6 @@ static int 
qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
  VIR_FREE(xml);
  if (!config)
  goto cleanup;
-} else {
-/* XXX Fail if VIR_DOMAIN_REVERT_FORCE is not set, rather than
- * blindly hoping for the best.  */
-VIR_WARN(snapshot is lacking rollback information for domain '%s',
- snap-def-name);
  }

  if (qemuDomainObjBeginJobWithDriver(driver, vm, QEMU_JOB_MODIFY)  0)
@@ -9843,10 +9857,22 @@ static int 
qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
  /* Transitions 5, 6, 8, 9 */
  /* Check for ABI compatibility.  */
  if (config  !virDomainDefCheckABIStability(vm-def, config)) {
-/* XXX Add VIR_DOMAIN_REVERT_FORCE to permit killing
- * and restarting a new qemu, since loadvm monitor
- * command won't work.  */
-goto endjob;
+if (!(flags  VIR_DOMAIN_SNAPSHOT_REVERT_FORCE)) {
+/* Alter existing error to give correct category. */
+virErrorPtr err = virGetLastError();
+err-code = VIR_ERR_SNAPSHOT_REVERT_RISKY;
+goto endjob;
+}
+qemuProcessStop(driver, vm, 0,
+VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT);
+virDomainAuditStop(vm, from-snapshot);
+detail = VIR_DOMAIN_EVENT_STOPPED_FROM_SNAPSHOT;
+event = virDomainEventNewFromObj(vm,
+ VIR_DOMAIN_EVENT_STOPPED,
+ detail);
+if (event)
+qemuDomainEventQueue(driver, event);
+goto load;
  }

  priv = vm-privateData;
@@ -9882,6 +9908,7 @@ static int 
qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
  virDomainObjAssignDef(vm, config, false);
  } else {
  /* Transitions 2, 3 */
+load:
  was_stopped = true;
  if (config)
  virDomainObjAssignDef(vm, config, false);



ACK.

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


Re: [libvirt] [PATCH 1/2] snapshot: add REVERT_FORCE to API

2011-10-04 Thread Eric Blake

On 10/04/2011 01:24 PM, Laine Stump wrote:


- if (virDomainRevertToSnapshot(snapshot, flags) 0)
+ result = virDomainRevertToSnapshot(snapshot, flags);
+ if (result 0 force
+ last_error-code == VIR_ERR_SNAPSHOT_REVERT_RISKY) {
+ flags |= VIR_DOMAIN_SNAPSHOT_REVERT_FORCE;


Are you not adding the FORCE flag the first time to allow for better
interoperability with older libvirtd?


Exactly.  For example, libvirt 0.9.6 does not understand FORCE (and 
using it would error out with unrecognized flag bit), but just happens 
to revert to a snapshot created by 0.9.4 on the first call with flags == 
0.  Libvirt 0.9.7, in the same situation, will error out with the new 
error value when flags == 0, but with the new error, so that you know 
you can safely add FORCE.  One other consideration is that 0.9.6 
understands the new flag VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING, which 0.9.4 
did not understand, so blindly looking for the 'unknown flag bit' error 
is not unique enough to be the indication that we could remove the 
--force bit and retry, because that still won't resolve on an 0.9.4 
server.  My choice of --force handling thus seems to be the one most 
likely to work for 0.9.4, 0.9.6, and 0.9.7, so that the user can blindly 
type 'virsh snapshot-revert dom old-snapshot --force' and attempt the 
revert regardless of server version.


Conversely, I still wanted 'virsh snapshot-revert' _without_ --force 
will work in as many situations as the server supports, and that if a 
server rejects the operation because force is required, the error 
message will be specific enough to mention that.  This is a good thing, 
so that users don't have to blindly add '--force' and expose themselves 
to unnecessary risk; rather, they should only add --force after an 
explicit message says what force would solve, and deciding that the risk 
of forcing for that particular case is worth it.



That's the only question I had. It all looks fine - ACK.


I'll wait for the 2/2 ack before applying this, then.

--
Eric Blake   ebl...@redhat.com+1-801-349-2682
Libvirt virtualization library http://libvirt.org

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


Re: [libvirt] [PATCH 2/2] qemu: add separate rerror_policy for disk errors

2011-10-04 Thread Eric Blake

On 10/04/2011 12:35 PM, Laine Stump wrote:

libvirt's XML only had a single attribute, error_policy to control
both read and write error policy, but qemu has separate settings for
these. In one case (enospc) a policy is allowed for write errors but
not read errors.

This patch adds a separate attribute that sets only the read error
policy. If just error_policy is set, it will apply to both read and
write error policy (previous behavior), but if the new rerror_policy
attribute is set, it will override error_policy for read errors only.
---
  docs/formatdomain.html.in |   16 +---


Missing the patch to docs/schemas/domaincommon.rng

Also, I'd like to see at least one addition to tests/qemuxml2argvdata 
that exposes the new command line.



-span class=sinceSince 0.8.0/span
+how the hypervisor will behave on a disk read or write
+error, possible values are stop, ignore, and
+enospace.span class=sinceSince 0.8.0/span  There is
+also an optionalcodererror_policy/code  that controls
+behavior for read errors only.span class=sinceSince
+0.9.7/space. If no rerror_policy is given, error_policy
+is used for both read and write errors. If rerror_policy
+is given, it overrides thecodeerror_policy/code  for
+read errors. Also note that enospace is not a valid
+policy for read errors, so ifcodeerror_policy/code  is
+set to enospace and nocodererror_policy/code  is
+given, the read error policy will be automatically set to
+ignore.


I think we also need to document that default can be used for either 
(or both) [r]error_policy as a way to rely on the hypervisor defaults. 
More on this below.



@@ -2560,6 +2562,16 @@ virDomainDiskDefParseXML(virCapsPtr caps,
  goto error;
  }

+if (rerror_policy
+(((def-rerror_policy
+   = virDomainDiskErrorPolicyTypeFromString(error_policy))  0) ||


virDomainDiskErrorPolicyTypeFromString converts default to 0, which 
means this is an undocumented value that we parse.  In many existing 
cases, we use vir...TypeFromString() = 0 to reject parsing default. 
But here, you copied-and-pasted from error_policy, which also was using 
 0 (and silently accepting default).  Now, as to _why_ it makes sense 
to allow default, consider the case where I want my qemu command line to 
have one, but not both, policies.  How do I specify that in the XML?  By 
doing:


rerror_policy='stop'

I get my desired:

rerror=stop

But what about the converse, for just werror on the command line?  Using

error_policy='stop'

produces

rerror=stop,werror=stop

which isn't quite what I wanted.  So I need:

rerror_policy='default' error_policy='stop'

to get exactly:

werror=stop

on the command line, which argues that default should be part of the 
documented XML for rerror_policy.  And symmetry argues that we might as 
well then document default for error_policy.


Hence, I think the following rng change covers things:

diff --git i/docs/schemas/domaincommon.rng w/docs/schemas/domaincommon.rng
index cffaac2..fffee9a 100644
--- i/docs/schemas/domaincommon.rng
+++ w/docs/schemas/domaincommon.rng
@@ -853,13 +853,25 @@
 /attribute
   /define
   define name=driverErrorPolicy
-attribute name=error_policy
-  choice
-valuestop/value
-valueignore/value
-valueenospace/value
-  /choice
-/attribute
+optional
+  attribute name=error_policy
+choice
+  valuedefault/value
+  valuestop/value
+  valueignore/value
+  valueenospace/value
+/choice
+  /attribute
+/optional
+optional
+  attribute name=rerror_policy
+choice
+  valuedefault/value
+  valuestop/value
+  valueignore/value
+/choice
+  /attribute
+/optional
   /define
   define name=driverIO
 attribute name=io


@@ -9177,6 +9191,8 @@ virDomainDiskDefFormat(virBufferPtr buf,
  virBufferAsprintf(buf,  cache='%s', cachemode);
  if (def-error_policy)
  virBufferAsprintf(buf,  error_policy='%s', error_policy);
+if (def-rerror_policy)
+virBufferAsprintf(buf,  rerror_policy='%s', rerror_policy);


Hmm, given my earlier arguments that documenting default makes sense, 
we would have to change the logic here to be that we always print both 
attributes, even if one of them is default, if at least one of them is 
something besides default:


if (def-error_policy || def-rerror_policy) {
  virBufferAsprintf(buf,  error_policy='%s' rerror_policy='%s',
error_policy, rerror_policy
}


+++ b/src/qemu/qemu_command.c
@@ -1696,6 +1696,8 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk,

  if (disk-error_policy)
  wpolicy = 
virDomainDiskErrorPolicyTypeToString(disk-error_policy);
+if (disk-rerror_policy)
+rpolicy = 

Re: [libvirt] [RFC PATCH] snapshot: better virsh handling of missing current, parent

2011-10-04 Thread Eric Blake

On 10/04/2011 12:54 PM, Laine Stump wrote:

On 09/30/2011 12:19 PM, Eric Blake wrote:

Previously, virsh 'snapshot-parent' and 'snapshot-current' were
completely silent in the case where the code conclusively proved
there was no parent or current snapshot, but differed in exit
status; this silence caused some confusion on whether the commands
worked. Furthermore, commit d1be48f introduced a regression where
snapshot-parent would leak output about an unknown function, but
only on the first attempt, when talking to an older server that
lacks virDomainSnapshotGetParent. This changes things to consistenly
report an error message and exit with status 1 when no snapshot
exists, and to avoid leaking unknown function warnings when using
fallbacks.

Preferences? (I guess mine is approach 1, by evidence of this patch).



The code all looks fine, so ACK to that. Not being very familiar with
the snapshot stuff, I can't say that I have an valid opinion on whether
it's better to log an error or exit silently when there is no parent. I
guess I would defer to your opinion, since you're the person who's spent
the most time dealing with snapshots lately :-)


I went ahead and pushed this as-is, on the basis that a common usage 
pattern, virsh snapshot-revert dom $(virsh snapshot-current --name), 
will do the right thing when there is no stdout, whether or not stderr 
was supplied, but having stderr available can help understand the 
situation when using the command in isolation.


--
Eric Blake   ebl...@redhat.com+1-801-349-2682
Libvirt virtualization library http://libvirt.org

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


Re: [libvirt] [PATCH 2/2] snapshot: enforce REVERT_FORCE on qemu

2011-10-04 Thread Eric Blake

On 10/04/2011 01:26 PM, Laine Stump wrote:

On 09/30/2011 02:52 PM, Eric Blake wrote:

Implements the documentation for snapshot revert vs. force.

Part of the patch tightens existing behavior (previously, reverting
to an old snapshot withoutdomain was blindly attempted, now it
requires force), while part of it relaxes behavior (previously, it
was not possible to revert an active domain to an ABI-incompatible
active snapshot, now force allows this transition).

* src/qemu/qemu_driver.c (qemuDomainRevertToSnapshot): Check for
risky situations, and allow force to get past them.


ACK.


Before pushing this, I'm running some sanity tests.  So far, this test 
sequence (adjusted to the fixed code) shows where force helps with older 
snapshots (I'll send separate email for showing how force helps active 
ABI-incompatible snapshots):


Test 1:
$ virsh snapshot-create-as dom snap # offline domain with one qcow2 disk
$ virsh edit dom # add a second qcow2 disk
$ virsh snapshot-revert dom snap # offline revert doesn't need force
$ virsh dumpxml dom # sure enough, second disk is gone
$ virsh edit dom # add a second qcow2 disk
$ virsh snapshot-dumpxml dom snap  snap.xml
$ virsh snapshot-delete --metadata dom snap
$ sed -i '\|domain |,\|/domain| d' snap.xml
$ virsh snapshot-create --redefine fedora_15-32 snap.xml
# the delete --metadata/--redefine is necessary, so that libvirt
# won't reuse domain from the prior definition
$ virsh snapshot-revert dom snap # simulates reverting to 0.9.4 snapshot
error: revert requires force: snapshot 'snap' lacks domain 'dom' 
rollback info

$ virsh snapshot-revert dom snap --force
error: internal error Child process (/usr/bin/qemu-img snapshot -a snap 
/path/to/second.qcow2) status unexpected: exit status 1

# See why we shouldn't have allowed blind revert? :)
$ virsh edit dom # remove that second disk
$ virsh snapshot-revert dom snap --force
# now that we match expected state, the revert works as desired

And here's what I have to squash in for test 1 to succeed as planned.

diff --git i/src/qemu/qemu_driver.c w/src/qemu/qemu_driver.c
index 1a171cf..07c4fd4 100644
--- i/src/qemu/qemu_driver.c
+++ w/src/qemu/qemu_driver.c
@@ -9649,7 +9649,8 @@ static int 
qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,

 virDomainDefPtr config = NULL;

 virCheckFlags(VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING |
-  VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED, -1);
+  VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED |
+  VIR_DOMAIN_SNAPSHOT_REVERT_FORCE, -1);

 /* We have the following transitions, which create the following 
events:

  * 1. inactive - inactive: none
@@ -9701,8 +9702,8 @@ static int 
qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,

 if (!(flags  VIR_DOMAIN_SNAPSHOT_REVERT_FORCE)) {
 if (!snap-def-dom) {
 qemuReportError(VIR_ERR_SNAPSHOT_REVERT_RISKY,
-_(snapshot lacks domain '%s' rollback 
details),

-snap-def-name);
+_(snapshot '%s' lacks domain '%s' rollback 
info),

+snap-def-name, vm-def-name);
 goto cleanup;
 }
 if (virDomainObjIsActive(vm) 
diff --git i/src/qemu/qemu_domain.c w/src/qemu/qemu_domain.c
index 65f721a..dda53f3 100644
--- i/src/qemu/qemu_domain.c
+++ w/src/qemu/qemu_domain.c
@@ -1441,7 +1441,7 @@ qemuDomainSnapshotForEachQcow2(struct qemud_driver 
*driver,

 if (virRun(qemuimgarg, NULL)  0) {
 if (try_all) {
 VIR_WARN(skipping snapshot action on %s,
- vm-def-disks[i]-info.alias);
+ vm-def-disks[i]-dst);
 skipped = true;
 continue;
 }


--
Eric Blake   ebl...@redhat.com+1-801-349-2682
Libvirt virtualization library http://libvirt.org

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


Re: [libvirt] [PATCH 2/2] snapshot: enforce REVERT_FORCE on qemu

2011-10-04 Thread Eric Blake

On 10/04/2011 04:02 PM, Eric Blake wrote:

Before pushing this, I'm running some sanity tests. So far, this test
sequence (adjusted to the fixed code) shows where force helps with older
snapshots (I'll send separate email for showing how force helps active
ABI-incompatible snapshots):

Test 1:
$ virsh snapshot-create-as dom snap # offline domain with one qcow2 disk
$ virsh edit dom # add a second qcow2 disk
$ virsh snapshot-revert dom snap # offline revert doesn't need force
$ virsh dumpxml dom # sure enough, second disk is gone


Whoops, this part of the test didn't quite work out, either.  I need to 
revert to the snapshot domain prior to determining the list of disks 
to iterate over, so that we avoid calling qemu-img snapshot -a on the 
disk image that was not part of the snapshot.  Likewise, snapshot-delete 
should call qemu-img snapshot -d only on the disk images involved in the 
snapshot.



And here's what I have to squash in for test 1 to succeed as planned.


Rather than squash in the fixed qemu-img iteration unreviewed into this 
ACK'd patch, I'll submit it as a separate patch.


--
Eric Blake   ebl...@redhat.com+1-801-349-2682
Libvirt virtualization library http://libvirt.org

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


Re: [libvirt] Connection does not support host device enumeration

2011-10-04 Thread Kaushal Shriyan
2011/10/1 Kaushal Shriyan kaushalshri...@gmail.com:
 2011/9/29 Kaushal Shriyan kaushalshri...@gmail.com:
 2011/9/27 Osier Yang jy...@redhat.com:
 于 2011年09月27日 01:13, Kaushal Shriyan 写道:

 Hi, Kaushal

 I don't use CentOS, so don't known it, though I guess it's
 the intention or mistake of the packager.

 you can compile from the source code youself.

 # rpm -qi libvirt

 You will see the version you use currently, and get the same
 source tarball from http://libvirt.org/sources/ to compile.

 Of course, you can download newer source to compile, but
 it may also need newer dependency packages. So compiling
 from the same version is the easiest way. :-)

 If you really don't want to compile yourself, just file a bug
 to CentOS, guess the packger will help you do it.

 Regards,
 Osier


 Hi Osier

 Can you please point me to the rpm version which has hal and udev
 flags enabled by default for CentOS 5.6 ?

 Regards

 Kaushal

 On Thu, Sep 22, 2011 at 10:22 AM, Osier Yang jy...@redhat.com wrote:

 于 2011年09月22日 12:46, Osier Yang 写道:

 于 2011年09月22日 12:41, Kaushal Shriyan 写道:

 On Thu, Sep 22, 2011 at 9:41 AM, Osier Yang jy...@redhat.com wrote:

 于 2011年09月22日 09:11, Kaushal Shriyan 写道:

 does not support host device enumeration

 Seems your libvirt is compiled without --with-hal and
 --with-udev

 Regards
 Osier

 Hi Osier

 Thanks for the reply. Please let me know how do i enable this feature
 since its a binary package installed on CentOS 5.6 x86_64
 architecture.

 You need to re-compile from the source if that's true.

 http://libvirt.org/compiling.html

 To make sure get your wanted, specifying --with-hal/--with-udev explicitly
 with yes will help.


 Regards,
 Osier


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




 Hi Osier,

 Is there libudev-devel or udev-devel package available on CentOS 5.6 ?

 [root@~]# yum search udev-devel
 Loaded plugins: fastestmirror
 Loading mirror speeds from cached hostfile
  * base: centos.aol.in
  * extras: centos.aol.in
  * updates: centos.aol.in
 base

  | 1.1 kB 00:00
 extras

  | 2.1 kB 00:00
 updates

  | 1.9 kB 00:00
 Warning: No matches found for: udev-devel
 No Matches found
 [root@~]# yum search libudev-devel
 Loaded plugins: fastestmirror
 Loading mirror speeds from cached hostfile
  * base: centos.aol.in
  * extras: centos.aol.in
  * updates: centos.aol.in
 Warning: No matches found for: libudev-devel
 No Matches found
 [root@~]#

 Please suggest further and also it seems like this package is not
 available in CentOS 5.6


 Regards

 Kaushal


 Hi,

 Please suggest/comment on my earlier post to this mailing list.

 Regards

 Kaushal


Hi Osier,

Checking in again. Please suggest/comment on my earlier post to this
mailing list.

Regards,

Kaushal

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

[libvirt] Qemu/KVM guest boots 2x slower with vhost_net

2011-10-04 Thread Reeted

Hello all,
for people in qemu-devel list, you might want to have a look at the 
previous thread about this topic, at

http://www.spinics.net/lists/kvm/msg61537.html
but I will try to recap here.

I found that virtual machines in my host booted 2x slower (on average 
it's 2x slower, but probably some parts are at least 3x slower) under 
libvirt compared to manual qemu-kvm launch. With the help of Daniel I 
narrowed it down to the vhost_net presence (default active when launched 
by libvirt) i.e. with vhost_net, boot process is *UNIFORMLY* 2x slower.


The problem is still reproducible on my systems but these are going to 
go to production soon and I am quite busy, I might not have many more 
days for testing left. Might be just next saturday and sunday for 
testing this problem, so if you can write here some of your suggestions 
by saturday that would be most appreciated.



I have performed some benchmarks now, which I hadn't performed in the 
old thread:


openssl speed -multi 2 rsa : (cpu benchmark) show no performance 
difference with or without vhost_net

disk benchmarks : show no performance difference with or without vhost_net
the disk benchmarks were: (both with cache=none and cache=writeback)
dd streaming read
dd streaming write
fio 4k random read in all cases of cache=none, cache=writeback with host 
cache dropped before test, cache=writeback with all fio data in host 
cache (measures context switch)

fio 4k random write

So I couldn't reproduce the problem with any benchmark that came to my mind.

But in the boot process this is very visible.
I'll continue the description below
before that, here are the System Specifications:
---
Host is with kernel 3.0.3 and Qemu-KVM 0.14.1, both vanilla and compiled 
by me.
Libvirt is the version in Ubuntu 11.04 Natty which is 0.8.8-1ubuntu6.5 . 
I didn't recompile this one


VM disks are LVs of LVM on MD raid array.
The problem shows identically on both cache=none and cache=writeback. 
Aio native.


Physical CPUs are: dual westmere 6-core (12 cores total, + hyperthreading)
2 vCPUs per VM.
All VMs are idle or off except the VM being tested.

Guests are:
- multiple Ubuntu 11.04 Natty 64bit with their 2.6.38-8-virtual kernel: 
very-minimal Ubuntu installs with deboostrap (not from the Ubuntu installer)
- one Fedora Core 6 32bit with a 32bit 2.6.38-8-virtual kernel + initrd 
both taken from Ubuntu Natty 32bit (so I could have virtio). Standard 
install (except kernel replaced afterwards).

Always static IP address in all guests
---

All types of guests show this problem, but it is more visible in the FC6 
guest because the boot process is MUCH longer than in the 
debootstrap-installed ubuntus.


Please note that most of boot process, at least from a certain point 
onwards, appears to the eye uniformly 2x or 3x slower under vhost_net, 
and by boot process I mean, roughly, copying by hand from some screenshots:



Loading default keymap
Setting hostname
Setting up LVM - no volume groups found
checking ilesystems... clean ...
remounting root filesystem in read-write mode
mounting local filesystems
enabling local filesystems quotas
enabling /etc/fstab swaps
INIT entering runlevel 3
entering non-interactive startup
Starting sysstat: calling the system activity data collector (sadc)
Starting background readahead

** starting from here it is everything, or almost everything, 
much slower


Checking for hardware changes
Bringing up loopback interface
Bringing up interface eth0
starting system logger
starting kernel logger
starting irqbalance
starting potmap
starting nfs statd
starting rpc idmapd
starting system message bus
mounting other filesystems
starting PC/SC smart card daemon (pcscd)
starint hidd ... can't open HIDP control socket : address familiy not 
supported by protocol (this is an error due to backporting a new ubuntu 
kernel to FC6)

starting autofs: loading autofs4
starting automount
starting acpi daemon
starting hpiod
starting hpssd
starting cups
starting sshd
starting ntpd
starting sendmail
starting sm-client
startingg console mouse services
starting crond
starting xfs
starting anacron
starting atd
starting youm-updatesd
starting Avahi daemon
starting HAL daemon


From the point I marked, onwards, most are services, i.e. daemons 
listening from sockets, so I have thought that maybe the binding to a 
socket could have been slower under vhost_net, but trying to put nc in 
listening with: nc -l 15000 is instantaneous, so I am not sure.


The shutdown of FC6 with basically the same services as above which tear 
down, is *also* much slower on vhost_net.


Thanks for any suggestions
R.

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


[libvirt] [PATCH 1.5/2] snapshot: use qemu-img on disks in use at time of snapshot

2011-10-04 Thread Eric Blake
Once we know which set of disks belong to a snapshot, reverting or
deleting that snapshot should visit just those disks, rather than
also visiting disks that were hot-plugged in the meantime or
skipping disks that were hot-unplugged in the meantime.

* src/qemu/qemu_domain.c (qemuDomainSnapshotForEachQcow2): Use
snapshot domain details when available.  Avoid NULL deref.
---

Detected while actually testing patch 2/2 in the series.  This
fixes the issue, but is worth a separate review before I push
the series.

 src/qemu/qemu_domain.c |   24 
 1 files changed, 16 insertions(+), 8 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 65f721a..85bebd6 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -1402,6 +1402,14 @@ qemuDomainSnapshotForEachQcow2(struct qemud_driver 
*driver,
 const char *qemuimgarg[] = { NULL, snapshot, NULL, NULL, NULL, NULL };
 int i;
 bool skipped = false;
+virDomainDefPtr def;
+
+/* Prefer action on the disks in use at the time the snapshot was
+ * created; but fall back to current definition if dealing with a
+ * snapshot created prior to libvirt 0.9.5.  */
+def = snap-def-dom;
+if (!def)
+def = vm-def;

 qemuimgarg[0] = qemuFindQemuImgBinary(driver);
 if (qemuimgarg[0] == NULL) {
@@ -1412,36 +1420,36 @@ qemuDomainSnapshotForEachQcow2(struct qemud_driver 
*driver,
 qemuimgarg[2] = op;
 qemuimgarg[3] = snap-def-name;

-for (i = 0; i  vm-def-ndisks; i++) {
+for (i = 0; i  def-ndisks; i++) {
 /* FIXME: we also need to handle LVM here */
 /* FIXME: if we fail halfway through this loop, we are in an
  * inconsistent state.  I'm not quite sure what to do about that
  */
-if (vm-def-disks[i]-device == VIR_DOMAIN_DISK_DEVICE_DISK) {
-if (!vm-def-disks[i]-driverType ||
-STRNEQ(vm-def-disks[i]-driverType, qcow2)) {
+if (def-disks[i]-device == VIR_DOMAIN_DISK_DEVICE_DISK) {
+if (!def-disks[i]-driverType ||
+STRNEQ(def-disks[i]-driverType, qcow2)) {
 if (try_all) {
 /* Continue on even in the face of error, since other
  * disks in this VM may have the same snapshot name.
  */
 VIR_WARN(skipping snapshot action on %s,
- vm-def-disks[i]-dst);
+ def-disks[i]-dst);
 skipped = true;
 continue;
 }
 qemuReportError(VIR_ERR_OPERATION_INVALID,
 _(Disk device '%s' does not support
snapshotting),
-vm-def-disks[i]-dst);
+def-disks[i]-dst);
 return -1;
 }

-qemuimgarg[4] = vm-def-disks[i]-src;
+qemuimgarg[4] = def-disks[i]-src;

 if (virRun(qemuimgarg, NULL)  0) {
 if (try_all) {
 VIR_WARN(skipping snapshot action on %s,
- vm-def-disks[i]-info.alias);
+ def-disks[i]-dst);
 skipped = true;
 continue;
 }
-- 
1.7.4.4

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


Re: [libvirt] Connection does not support host device enumeration

2011-10-04 Thread Dave Allan
On Wed, Oct 05, 2011 at 04:10:07AM +0530, Kaushal Shriyan wrote:
 2011/10/1 Kaushal Shriyan kaushalshri...@gmail.com:
  2011/9/29 Kaushal Shriyan kaushalshri...@gmail.com:
  2011/9/27 Osier Yang jy...@redhat.com:
  于 2011年09月27日 01:13, Kaushal Shriyan 写道:
 
  Hi, Kaushal
 
  I don't use CentOS, so don't known it, though I guess it's
  the intention or mistake of the packager.
 
  you can compile from the source code youself.
 
  # rpm -qi libvirt
 
  You will see the version you use currently, and get the same
  source tarball from http://libvirt.org/sources/ to compile.
 
  Of course, you can download newer source to compile, but
  it may also need newer dependency packages. So compiling
  from the same version is the easiest way. :-)
 
  If you really don't want to compile yourself, just file a bug
  to CentOS, guess the packger will help you do it.
 
  Regards,
  Osier
 
 
  Hi Osier
 
  Can you please point me to the rpm version which has hal and udev
  flags enabled by default for CentOS 5.6 ?
 
  Regards
 
  Kaushal
 
  On Thu, Sep 22, 2011 at 10:22 AM, Osier Yang jy...@redhat.com wrote:
 
  于 2011年09月22日 12:46, Osier Yang 写道:
 
  于 2011年09月22日 12:41, Kaushal Shriyan 写道:
 
  On Thu, Sep 22, 2011 at 9:41 AM, Osier Yang jy...@redhat.com wrote:
 
  于 2011年09月22日 09:11, Kaushal Shriyan 写道:
 
  does not support host device enumeration
 
  Seems your libvirt is compiled without --with-hal and
  --with-udev
 
  Regards
  Osier
 
  Hi Osier
 
  Thanks for the reply. Please let me know how do i enable this feature
  since its a binary package installed on CentOS 5.6 x86_64
  architecture.
 
  You need to re-compile from the source if that's true.
 
  http://libvirt.org/compiling.html
 
  To make sure get your wanted, specifying --with-hal/--with-udev explicitly
  with yes will help.
 
 
  Regards,
  Osier
 
 
  --
  libvir-list mailing list
  libvir-list@redhat.com
  https://www.redhat.com/mailman/listinfo/libvir-list
 
 
 
 
  Hi Osier,
 
  Is there libudev-devel or udev-devel package available on CentOS 5.6 ?
 
  [root@~]# yum search udev-devel
  Loaded plugins: fastestmirror
  Loading mirror speeds from cached hostfile
   * base: centos.aol.in
   * extras: centos.aol.in
   * updates: centos.aol.in
  base
 
   | 1.1 kB 00:00
  extras
 
   | 2.1 kB 00:00
  updates
 
   | 1.9 kB 00:00
  Warning: No matches found for: udev-devel
  No Matches found
  [root@~]# yum search libudev-devel
  Loaded plugins: fastestmirror
  Loading mirror speeds from cached hostfile
   * base: centos.aol.in
   * extras: centos.aol.in
   * updates: centos.aol.in
  Warning: No matches found for: libudev-devel
  No Matches found
  [root@~]#
 
  Please suggest further and also it seems like this package is not
  available in CentOS 5.6
 
 
  Regards
 
  Kaushal
 
 
  Hi,
 
  Please suggest/comment on my earlier post to this mailing list.
 
  Regards
 
  Kaushal
 
 
 Hi Osier,
 
 Checking in again. Please suggest/comment on my earlier post to this
 mailing list.

You need to read his advice to you carefully: you need either hal or
udev; you're using Centos 5.x, so you need hal.  

Dave

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

Re: [libvirt] [PATCH 1/2] qemu: correct misspelled 'enospc' option, and only use for werror

2011-10-04 Thread Laine Stump

On 10/04/2011 03:17 PM, Eric Blake wrote:

On 10/04/2011 12:35 PM, Laine Stump wrote:

This resolves:

  https://bugzilla.redhat.com/show_bug.cgi?id=730909

When support for setting the qemu disk error policy to 'enospc' was
added, it was inadvertantly as enospace. This patch corrects that on


spelling and grammar

s/inadvertantly/inadvertently spelled/



You have to appreciate the irony, though, right? :-)





the qemu commandline (while retaining the enospace spelling for
libvirt's XML.

Also, while examining the qemu source, I found that enospc is not
allowed for the read error policy, only for write error policy (makes
sense). Since libvirt currently only has a single error policy
setting, when enospace is selected, the read error policy is set to
ignore.
---
  src/qemu/qemu_command.c |   22 ++
  1 files changed, 18 insertions(+), 4 deletions(-)


ACK.




Okay, I'm pushing this one without waiting for 2/2, though, as I haven't 
decided how to deal with some of the issues you brought up in your review.


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