Re: [libvirt] [PATCH v5 4/5] storage: Support chap authentication for iscsi pool

2013-07-25 Thread Ján Tomko 
On 07/24/2013 10:06 PM, John Ferlan wrote:
 On 07/24/2013 05:57 AM, Daniel P. Berrange wrote:
 On Wed, Jul 24, 2013 at 10:25:06AM +0200, Ján Tomko wrote:
 ...snip...

 Both secret and qemu drivers are registered after the storage driver on
 libvirtd startup, so autostarting these pools will only work on storage 
 driver
 reload. On libvirtd startup it fails with:
 qemuConnectOpen:1033 : internal error qemu state driver is not active

 (And it seems nwfilter only opens the qemu:// connection on reload)

 Oh damn, yes, that pretty much dooms us.  We can't change the order of
 the drivers either, because autostarting of QEMU guests, requires that
 the storage pools be autostarted already.

 To fix this would require that we split virStateInitialize into two
 parts, virStateInitialize() and virStateAutoStart(). That's too big
 a change todo for this release, but we could do it for next release
 without too much trouble.


 Daniel

 
 
 Could we just do it for storage driver?  It seems you are indicating
 that the following would suffice, right?  Or does this problem go deeper?
 

Doing it only for the storage driver would move the pool autostart after QEMU
guests autostart and the guests wouldn't be able to use the pools.

Jan

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

Re: [libvirt] [PATCH] virdbustest: Don't pass number of arguments as long long

2013-07-25 Thread Daniel Veillard 
On Wed, Jul 24, 2013 at 03:41:17PM -0600, Eric Blake wrote:
 On 07/24/2013 03:29 PM, Guido Günther wrote:
  since sizeof(size_t) != sizeof(long long) on 32bit archs.
  
  This unbreaks virdbustest which otherwise fails like:
  
 
  +++ b/tests/virdbustest.c
  @@ -195,7 +195,7 @@ static int testMessageArray(const void *args 
  ATTRIBUTE_UNUSED)
   if (virDBusMessageEncode(msg,
sais,
in_str1,
  - (long long)3, in_int32a, in_int32b, in_int32c,
  + (size_t)3, in_int32a, in_int32b, in_int32c,
 
 This fix looks correct, but it's annoying that we have to cast the 'a'
 length argument in every caller.  I'm wondering if a better fix would be
 to virDBusMessageEncode to take an 'int' instead of a 'size_t' arg for
 a length; even though that is technically an arbitrary limitation on
 64-bit platforms, I seriously doubt anyone plans to use dBus to send
 more than 2G of objects.

  I agree with your point, but for the sake of fixing this in 1.1.1
maybe that small patch is the right approach, then fix the internal APIs
after the freeze. Makes sense ?

Daniel

-- 
Daniel Veillard  | Open Source and Standards, Red Hat
veill...@redhat.com  | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
http://veillard.com/ | virtualization library  http://libvirt.org/

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

Re: [libvirt] [PATCH] virdbustest: Don't pass number of arguments as long long

2013-07-25 Thread Guido Günther 
Hi Eric,
On Wed, Jul 24, 2013 at 03:41:17PM -0600, Eric Blake wrote:
 On 07/24/2013 03:29 PM, Guido Günther wrote:
  since sizeof(size_t) != sizeof(long long) on 32bit archs.
  
  This unbreaks virdbustest which otherwise fails like:
  
 
  +++ b/tests/virdbustest.c
  @@ -195,7 +195,7 @@ static int testMessageArray(const void *args 
  ATTRIBUTE_UNUSED)
   if (virDBusMessageEncode(msg,
sais,
in_str1,
  - (long long)3, in_int32a, in_int32b, in_int32c,
  + (size_t)3, in_int32a, in_int32b, in_int32c,
 
 This fix looks correct, but it's annoying that we have to cast the 'a'
 length argument in every caller.  I'm wondering if a better fix would be
 to virDBusMessageEncode to take an 'int' instead of a 'size_t' arg for
 a length; even though that is technically an arbitrary limitation on
 64-bit platforms, I seriously doubt anyone plans to use dBus to send
 more than 2G of objects.

I'm not sure we need to change anything. Removing the (long long) is
enough to make the tests pass on 32bit as well as 64bit x86. It's just
that we must not pass in an 8byte type on i386. I just left left the
cast to size_t in since I suspected some (yet to me unclear) intention
behind an explicit cast.
Cheers,
 -- Guido

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


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

[libvirt] Use flock() instead of fcntl()

2013-07-25 Thread David Weber 
Hi,

we are interested in using virtlockd on an OCFS2 shared filesystem.
We are now facing the problem that virtlockd uses fcntl() locks which 
aren't supported by OCFS2 with the o2cb cluster stack and we want 
to avoid using indirect leases.

OCFS2 instead supports flock() which is quite similar to fcntl(). I 
attached a patch which makes libvirt use flock() *instead* of fcntl() 
and it seems to work.

NFS on the contrast only supports fcntl() so it should be configurable 
which lock type to use.

I'm not very experienced with locking, so would such a patch be
acceptable or do you see possible problems with it?

Cheers,
David



From b823a9a9bd60a870d64341c4273c42d4eeba8d9b Mon Sep 17 00:00:00 2001
From: David Weber w...@munzinger.de
Date: Thu, 25 Jul 2013 08:20:20 +
Subject: [PATCH] Use flock() instead of fcntl()

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

diff --git a/src/util/virfile.c b/src/util/virfile.c
index 8f0eec3..e243c26 100644
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
int virFileLock(int fd, bool shared, off_t start, off_t len)
 .l_len = len,
 };
 
-if (fcntl(fd, F_SETLK, fl)  0)
+if (flock(fd, LOCK_EX | LOCK_NB)  0)
 return -errno;
 
 return 0;
int virFileUnlock(int fd, off_t start, off_t len)
 .l_len = len,
 };
 
-if (fcntl(fd, F_SETLK, fl)  0)
+if (flock(fd, LOCK_UN)  0)
 return -errno;
 
 return 0;
-- 
1.8.1.5



0001-Use-flock-instead-of-fcntl.patch
Description: Binary data
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [Qemu-devel] [PATCH 4/7] qemu: Add monitor APIs to fetch CPUID data from QEMU

2013-07-25 Thread Andreas Färber 
Am 24.07.2013 20:25, schrieb Eduardo Habkost:
 In addition to the -cpu host KVM initialization problem, this is an
 additional problem with the current interfaces provided by QEMU:
 
 1) libvirt needs to query data that depend on chosen machine-type and
CPU model
 2) Some machine-type behavior is code and not introspectable data
* Luckily most of the data we need in this case should/will be
  encoded in the compat_props tables.
* In either case, we don't have an API to query for machine-type
  compat_props information yet.

... and I don't think we should add such a thing. It is an internal
implementation detail, whose results should be inspected instead.

 3) CPU model behavior will be modelled as CPU class behavior. Like
on the machine-type case, some of the CPU-model-specific behavior may
be modelled as code, and not introspectable data.
* However, e may be able to eventually encode most or all of
  CPU-model-specific behavior simply as different per-CPU-class
  property defaults.
* In either case, we don't have an API for QOM class introspection,
  yet.

If I understood Anthony correctly on the previous call then that is by
design. We have a query-cpu-definitions QMP API to obtain CPU models
though. And qom-list-types to discover QOM types. The CPU can then be
instantiated via -cpu (the type via -device/-object on other targets),
inspected with qom-list/qom-get API and modified with qom-set.

The problem with the latter is that devices/CPUs get realized in code
rather than shortly before emulation/virtualization starts - that's what
my recursive QOM realization series prepared to address, but Paolo
veto'ed that and hasn't provided sufficient feedback on what exactly his
concerns are founded on and what he proposes instead. In particular:
Would walking the qdev bus tree instead of the QOM composition tree
address the concerns?

Depending on what libvirt is actually trying to do, the above combined
with Igor's feature properties and -S might do the job. For x86 the CPUs
are easily locatable in the QOM tree via ICC.

Regards,
Andreas

 But there's something important in this case: the resulting CPUID data
 for a specific machine-type + CPU-model combination must be always the
 same, forever. This means libvirt may even use a static table, or cache
 this information indefinitely.
 
 (Note that I am not talking about -cpu host, here, but about all the
 other CPU models)

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

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

[libvirt] RFC: Introduce API to return configuration/state paths of the network driver

2013-07-25 Thread Nehal J. Wani 
Currently, there is no API which returns configuration/state paths of the
network driver.
Although it is a private implementation of the network driver, I don't see
any harm in
making the locations public because although the locations might change,
there will always
be a location for these files. There is a need for this API to implement
method 2 of the
API to query ip addresses of a given domain, refer:
http://www.mail-archive.com/libvir-list@redhat.com/msg79793.html . It is
required to parse
the leases file generated by dnsmasq. So, this API will be used by the qemu
driver, but it
can also be made public, so that, if a user wants to know get some
information from a
configuration file, he can get the location from libvirt and analyze it on
his own. Right now,
there is an alternate way to get the info: by using
networkDnsmasqLeaseFileNameDefault,
defined in /src/network/bridge_driver.c Since this function is static, it
is part of the private
implementation and not visible outside. To make it public, the following
hack is possible:

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index a7ff602..7274861 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -124,7 +124,7 @@ static int networkUnplugBandwidth(virNetworkObjPtr net,

 static struct network_driver *driverState = NULL;

-static char *
+char *
 networkDnsmasqLeaseFileNameDefault(const char *netname)
 {
 char *leasefile;
diff --git a/src/network/bridge_driver.h b/src/network/bridge_driver.h
index 50258b5..40e3990 100644
--- a/src/network/bridge_driver.h
+++ b/src/network/bridge_driver.h
@@ -49,6 +49,8 @@ int networkDnsmasqConfContents(virNetworkObjPtr network,
 char **configstr,
 dnsmasqContext *dctx,
 dnsmasqCapsPtr caps);
+char * networkDnsmasqLeaseFileNameDefault(const char *netname)
+ATTRIBUTE_NONNULL(1);
 # else
 /* Define no-op replacements that don't drag in any link dependencies.  */
 #  define networkAllocateActualDevice(iface) 0
@@ -57,6 +59,7 @@ int networkDnsmasqConfContents(virNetworkObjPtr network,
 #  define networkGetNetworkAddress(netname, netaddr) (-2)
 #  define networkDnsmasqConfContents(network, pidfile, configstr, \
 dctx, caps) 0
+#  define networkDnsmasqLeaseFileNameDefault(netname) 0
 # endif

 typedef char *(*networkDnsmasqLeaseFileNameFunc)(const char *netname);

Similar hack has been used so that networkAllocateActualDevice() can be
called
from qemu_command.c. Although the above method works, we want to have a
formal API and not leave things like a hack.

/*
 * @conn: connection object
 * @params: array to populate on output
 * @nparams: number of parameters that will be filled
 * @flags: not supported, user should pass 0 for now
 * return 0 on success -1 otherwise
 * Valid parameter field names:
 * VIR_NETWORK_CONFIG_DIR, VIR_NETWORK_AUTOSTART_DIR, VIR_STATE_DIR,
 * VIR_PID_DIR, VIR_DNSMASQ_STATE_DIR, VIR_RADVD_STATE_DIR
 * All the above will of the type VIR_TYPED_PARAM_STRING
 */
int virNetworkGetConfigFileName(virConnectPtr conn,
virTypedParameterPtr params,
int nparams,
unsigned int flags)


Nehal J. Wani
UG3, BTech CS+MS(CL)
IIIT-Hyderabad
http://commandlinewani.blogspot.com
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] tests: PCI controller checks

2013-07-25 Thread Daniel P. Berrange 
On Wed, Jul 24, 2013 at 03:29:03PM -0600, Eric Blake wrote:
 On 07/22/2013 08:48 AM, Ján Tomko wrote:
  Check if PCI bridges with duplicate indexes are rejected.
  PCI root controllers with non-zero indexes or addresses should
  also be rejected.
  ---
   .../qemuxml2argv-pci-bridge-duplicate-index.xml  | 16 
  
   tests/qemuxml2argvdata/qemuxml2argv-pci-root-address.xml | 16 
  
   .../qemuxml2argv-pci-root-nonzero-index.xml  | 14 
  ++
   tests/qemuxml2argvtest.c |  6 ++
   4 files changed, 52 insertions(+)
   create mode 100644 
  tests/qemuxml2argvdata/qemuxml2argv-pci-bridge-duplicate-index.xml
   create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pci-root-address.xml
   create mode 100644 
  tests/qemuxml2argvdata/qemuxml2argv-pci-root-nonzero-index.xml
 
 ACK.  Adding test cases is always safe, even in freeze.

Famous last words :-)

If maintaining libvirt has taught me anything, it is that nothing is
completely safe :-P

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 v5 4/5] storage: Support chap authentication for iscsi pool

2013-07-25 Thread Daniel P. Berrange 
On Thu, Jul 25, 2013 at 08:10:46AM +0200, Ján Tomko wrote:
 On 07/24/2013 10:06 PM, John Ferlan wrote:
  On 07/24/2013 05:57 AM, Daniel P. Berrange wrote:
  On Wed, Jul 24, 2013 at 10:25:06AM +0200, Ján Tomko wrote:
  ...snip...
 
  Both secret and qemu drivers are registered after the storage driver on
  libvirtd startup, so autostarting these pools will only work on storage 
  driver
  reload. On libvirtd startup it fails with:
  qemuConnectOpen:1033 : internal error qemu state driver is not active
 
  (And it seems nwfilter only opens the qemu:// connection on reload)
 
  Oh damn, yes, that pretty much dooms us.  We can't change the order of
  the drivers either, because autostarting of QEMU guests, requires that
  the storage pools be autostarted already.
 
  To fix this would require that we split virStateInitialize into two
  parts, virStateInitialize() and virStateAutoStart(). That's too big
  a change todo for this release, but we could do it for next release
  without too much trouble.
 
 
  Daniel
 
  
  
  Could we just do it for storage driver?  It seems you are indicating
  that the following would suffice, right?  Or does this problem go deeper?
  
 
 Doing it only for the storage driver would move the pool autostart after QEMU
 guests autostart and the guests wouldn't be able to use the pools.

Yep, we'd need todo it for all the drivers at once to preseve the ordering.

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] virdbustest: Don't pass number of arguments as long long

2013-07-25 Thread Daniel P. Berrange 
On Wed, Jul 24, 2013 at 03:41:17PM -0600, Eric Blake wrote:
 On 07/24/2013 03:29 PM, Guido Günther wrote:
  since sizeof(size_t) != sizeof(long long) on 32bit archs.
  
  This unbreaks virdbustest which otherwise fails like:
  
 
  +++ b/tests/virdbustest.c
  @@ -195,7 +195,7 @@ static int testMessageArray(const void *args 
  ATTRIBUTE_UNUSED)
   if (virDBusMessageEncode(msg,
sais,
in_str1,
  - (long long)3, in_int32a, in_int32b, in_int32c,
  + (size_t)3, in_int32a, in_int32b, in_int32c,
 
 This fix looks correct, but it's annoying that we have to cast the 'a'
 length argument in every caller.  I'm wondering if a better fix would be
 to virDBusMessageEncode to take an 'int' instead of a 'size_t' arg for
 a length; even though that is technically an arbitrary limitation on
 64-bit platforms, I seriously doubt anyone plans to use dBus to send
 more than 2G of objects.

The code requires an int already

narray = va_arg(args, int);

so both the (long long) and (size_t) casts are bogus. We can just
remove the cast completely.

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 00/13] Support use of systemd-machined for cgroups

2013-07-25 Thread Gao feng 
On 07/23/2013 11:21 PM, Daniel P. Berrange wrote:
 From: Daniel P. Berrange berra...@redhat.com
 
 This is a patch series which adds support for using systemd-machined
 for creating cgroups. The first 12 patches are all really just cleanups
 and refactoring. The actual systemd code is the last patch, but at time
 of posting it doesn't quite work properly. Given the freeze, I'd like
 to get the first 12 patches merged, while I iron out the remaining
 problems with the last patch.
 

Right now cgroup is created and set by libvirt itself, in future
libvirt will create and setup the cgroup for domain through systemd?

Thanks

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

Re: [libvirt] Use flock() instead of fcntl()

2013-07-25 Thread Daniel P. Berrange 
On Thu, Jul 25, 2013 at 08:23:24AM +, David Weber wrote:
 Hi,
 
 we are interested in using virtlockd on an OCFS2 shared filesystem.
 We are now facing the problem that virtlockd uses fcntl() locks which 
 aren't supported by OCFS2 with the o2cb cluster stack and we want 
 to avoid using indirect leases.
 
 OCFS2 instead supports flock() which is quite similar to fcntl(). I 
 attached a patch which makes libvirt use flock() *instead* of fcntl() 
 and it seems to work.
 
 NFS on the contrast only supports fcntl() so it should be configurable 
 which lock type to use.
 
 I'm not very experienced with locking, so would such a patch be
 acceptable or do you see possible problems with it?

We definitely can't use flock() unconditionally like that, as it is
less widely supported than fcntl() locking and is known broken on
most network filesystems (flock() will only lock wrt the local node,
not network-wide).

I'm pretty surprised that you can fcntl() is not supported in ocfs2.
Are you really sure about that ?

This mail message suggests it is supported

  https://oss.oracle.com/pipermail/ocfs2-users/2009-June/003572.html

Support for fcntl locking aka file-range locking aka posix locking is
 provided by vfs for all file systems. However, that support is appropriate
 only for local file systems.

 In ocfs2, we have added support for cluster-aware fcntl locking via
 the userspace clustering framework that allows one to use ocfs2 with
 different cluster-stacks.

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] RFC: Introduce API to return configuration/state paths of the network driver

2013-07-25 Thread Daniel P. Berrange 
On Thu, Jul 25, 2013 at 02:02:36PM +0530, Nehal J. Wani wrote:
 Currently, there is no API which returns configuration/state paths of the
 network driver.
 Although it is a private implementation of the network driver, I don't see
 any harm in
 making the locations public because although the locations might change,
 there will always
 be a location for these files. There is a need for this API to implement
 method 2 of the
 API to query ip addresses of a given domain, refer:
 http://www.mail-archive.com/libvir-list@redhat.com/msg79793.html . It is
 required to parse
 the leases file generated by dnsmasq. So, this API will be used by the qemu
 driver, but it
 can also be made public, so that, if a user wants to know get some
 information from a
 configuration file, he can get the location from libvirt and analyze it on
 his own. Right now,
 there is an alternate way to get the info: by using
 networkDnsmasqLeaseFileNameDefault,
 defined in /src/network/bridge_driver.c Since this function is static, it
 is part of the private
 implementation and not visible outside. To make it public, the following
 hack is possible:

NACK,

As I explained on IRC, the hypervisor drivers have no business accessing
the dnsmasq lease files from the bridge driver. This is considered to be
a private implementation detail.

At a conceptual level, what you're after here is a list of all the IP,
mac address mappings of the virtual network. This information is useful
even outside the context of the hypervisor driver method you're working
on. So we should create formal APIs for exposing this, something like:

   virNetworkGetDHCPLeases(virNetworkPtr network,
   virNetworkDHCPLeasePtr *leases,
   unsigned int nleases);

And/or this

   virNetworkGetDHCPLeaseForMAC(virNetworkPtr network,
unsigned char *macaddr,
virNetworkDHCPLeasePtr lease);

and a corresponding  'virsh net-dhcp-leases netname' command

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 00/13] Support use of systemd-machined for cgroups

2013-07-25 Thread Daniel P. Berrange 
On Thu, Jul 25, 2013 at 05:30:20PM +0800, Gao feng wrote:
 On 07/23/2013 11:21 PM, Daniel P. Berrange wrote:
  From: Daniel P. Berrange berra...@redhat.com
  
  This is a patch series which adds support for using systemd-machined
  for creating cgroups. The first 12 patches are all really just cleanups
  and refactoring. The actual systemd code is the last patch, but at time
  of posting it doesn't quite work properly. Given the freeze, I'd like
  to get the first 12 patches merged, while I iron out the remaining
  problems with the last patch.
  
 
 Right now cgroup is created and set by libvirt itself, in future
 libvirt will create and setup the cgroup for domain through systemd?

Yes, systemd is starting to take over all management for cgroups. Initially
we'll defer to systemd for creation of the cgroups. Eventually though, all
writes to the cgroups will have to go via systemd.

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

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

Re: [libvirt] [PATCH 4/7] qemu: Add monitor APIs to fetch CPUID data from QEMU

2013-07-25 Thread Daniel P. Berrange 
On Wed, Jul 24, 2013 at 03:25:19PM -0300, Eduardo Habkost wrote:
 On Tue, Jul 23, 2013 at 07:32:46PM +0200, Jiri Denemark wrote:
  On Tue, Jul 23, 2013 at 19:28:38 +0200, Jiri Denemark wrote:
   On Tue, Jul 23, 2013 at 17:32:42 +0100, Daniel Berrange wrote:
On Tue, Jul 23, 2013 at 06:11:33PM +0200, Jiri Denemark wrote:
 ---
  src/qemu/qemu_monitor.c|  21 +++
  src/qemu/qemu_monitor.h|   3 +
  src/qemu/qemu_monitor_json.c   | 162 
 +
  src/qemu/qemu_monitor_json.h   |   6 +
  tests/Makefile.am  |   1 +
  .../qemumonitorjson-getcpu-empty.data  |   2 +
  .../qemumonitorjson-getcpu-empty.json  |  46 ++
  .../qemumonitorjson-getcpu-filtered.data   |   4 +
  .../qemumonitorjson-getcpu-filtered.json   |  46 ++
  .../qemumonitorjson-getcpu-full.data   |   4 +
  .../qemumonitorjson-getcpu-full.json   |  46 ++
  .../qemumonitorjson-getcpu-host.data   |   5 +
  .../qemumonitorjson-getcpu-host.json   |  45 ++
  tests/qemumonitorjsontest.c|  74 ++
  14 files changed, 465 insertions(+)
  create mode 100644 
 tests/qemumonitorjsondata/qemumonitorjson-getcpu-empty.data
  create mode 100644 
 tests/qemumonitorjsondata/qemumonitorjson-getcpu-empty.json
  create mode 100644 
 tests/qemumonitorjsondata/qemumonitorjson-getcpu-filtered.data
  create mode 100644 
 tests/qemumonitorjsondata/qemumonitorjson-getcpu-filtered.json
  create mode 100644 
 tests/qemumonitorjsondata/qemumonitorjson-getcpu-full.data
  create mode 100644 
 tests/qemumonitorjsondata/qemumonitorjson-getcpu-full.json
  create mode 100644 
 tests/qemumonitorjsondata/qemumonitorjson-getcpu-host.data
  create mode 100644 
 tests/qemumonitorjsondata/qemumonitorjson-getcpu-host.json

ACK, though I believe the design of this monitor API is flawed
because it requires you to re-launch QEMU with different accel
args
   
   Not really, this can be used in tcg too. It's just when we want to get
   the data for host CPU, we need to enable kvm as tcg knows nothing
   about that CPU. Which makes sense as kvm (the kernel module) influences
   how the host CPU will look like.
  
  However, you need to have a CPU to be able to ask for his properties
  (which kinda makes sense too) and for that you also need a machine with
  type != none. Which makes sense too, as the CPU may differ depending on
  machine type (which, however, does not happen for host CPU).
 
 In addition to the -cpu host KVM initialization problem, this is an
 additional problem with the current interfaces provided by QEMU:
 
 1) libvirt needs to query data that depend on chosen machine-type and
CPU model
 2) Some machine-type behavior is code and not introspectable data
* Luckily most of the data we need in this case should/will be
  encoded in the compat_props tables.
* In either case, we don't have an API to query for machine-type
  compat_props information yet.
 3) CPU model behavior will be modelled as CPU class behavior. Like
on the machine-type case, some of the CPU-model-specific behavior may
be modelled as code, and not introspectable data.
* However, e may be able to eventually encode most or all of
  CPU-model-specific behavior simply as different per-CPU-class
  property defaults.
* In either case, we don't have an API for QOM class introspection,
  yet.
 
 But there's something important in this case: the resulting CPUID data
 for a specific machine-type + CPU-model combination must be always the
 same, forever. This means libvirt may even use a static table, or cache
 this information indefinitely.
 
 (Note that I am not talking about -cpu host, here, but about all the
 other CPU models)

Hmm, so if the CPU filtering can vary per every single individual
machine type, then the approach Jiri started here, of invoking QEMU
with machine type set to query the CPU after it was created, is
definitely not something we can follow. It is just far too inefficient.

I understand that the QEMU code isn't currently structured in a way
that lets it easily expose information that varies per machine type,
but I don't think we need to solve the entire problem space in a
perfectly generic fashion here. Perfect is the enemy of good.

If we can get all the CPU feature flag filtering information to be
in statically defined data structures, then it seems that it would
be pretty straightforward to add a monitor API that takes a CPU
model name and machine type name, and returns the list of feature
flags, without actually having to initialize the machine type or
CPU. It can even just open /dev/kvm  issue the neccessary ioctl,
without having to

Re: [libvirt] [PATCH 00/13] Support use of systemd-machined for cgroups

2013-07-25 Thread Daniel P. Berrange 
On Thu, Jul 25, 2013 at 05:48:54PM +0800, Gao feng wrote:
 On 07/25/2013 05:36 PM, Daniel P. Berrange wrote:
  On Thu, Jul 25, 2013 at 05:30:20PM +0800, Gao feng wrote:
  On 07/23/2013 11:21 PM, Daniel P. Berrange wrote:
  From: Daniel P. Berrange berra...@redhat.com
 
  This is a patch series which adds support for using systemd-machined
  for creating cgroups. The first 12 patches are all really just cleanups
  and refactoring. The actual systemd code is the last patch, but at time
  of posting it doesn't quite work properly. Given the freeze, I'd like
  to get the first 12 patches merged, while I iron out the remaining
  problems with the last patch.
 
 
  Right now cgroup is created and set by libvirt itself, in future
  libvirt will create and setup the cgroup for domain through systemd?
  
  Yes, systemd is starting to take over all management for cgroups. Initially
  we'll defer to systemd for creation of the cgroups. Eventually though, all
  writes to the cgroups will have to go via systemd.
 
 Get it, hope we can remove some redundance codes.

It is pretty unlikely that we'll be able to delete any of our existing
cgroups code. We absolutely need to continue to support non-systemd
based deployments of libvirt for Debian/Ubuntu/*BSD and older RHEL-6
etc


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 00/13] Support use of systemd-machined for cgroups

2013-07-25 Thread Gao feng 
On 07/25/2013 05:36 PM, Daniel P. Berrange wrote:
 On Thu, Jul 25, 2013 at 05:30:20PM +0800, Gao feng wrote:
 On 07/23/2013 11:21 PM, Daniel P. Berrange wrote:
 From: Daniel P. Berrange berra...@redhat.com

 This is a patch series which adds support for using systemd-machined
 for creating cgroups. The first 12 patches are all really just cleanups
 and refactoring. The actual systemd code is the last patch, but at time
 of posting it doesn't quite work properly. Given the freeze, I'd like
 to get the first 12 patches merged, while I iron out the remaining
 problems with the last patch.


 Right now cgroup is created and set by libvirt itself, in future
 libvirt will create and setup the cgroup for domain through systemd?
 
 Yes, systemd is starting to take over all management for cgroups. Initially
 we'll defer to systemd for creation of the cgroups. Eventually though, all
 writes to the cgroups will have to go via systemd.

Get it, hope we can remove some redundance codes.

Thanks!

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

Re: [libvirt] Use flock() instead of fcntl()

2013-07-25 Thread David Weber 
Thank you for your quick response!

Am Donnerstag, 25. Juli 2013, 10:31:40 schrieb Daniel P. Berrange:
 On Thu, Jul 25, 2013 at 08:23:24AM +, David Weber wrote:
  Hi,
  
  we are interested in using virtlockd on an OCFS2 shared filesystem.
  We are now facing the problem that virtlockd uses fcntl() locks which
  aren't supported by OCFS2 with the o2cb cluster stack and we want
  to avoid using indirect leases.
  
  OCFS2 instead supports flock() which is quite similar to fcntl(). I
  attached a patch which makes libvirt use flock() *instead* of fcntl()
  and it seems to work.
  
  NFS on the contrast only supports fcntl() so it should be configurable
  which lock type to use.
  
  I'm not very experienced with locking, so would such a patch be
  acceptable or do you see possible problems with it?
 
 We definitely can't use flock() unconditionally like that, as it is
 less widely supported than fcntl() locking and is known broken on
 most network filesystems (flock() will only lock wrt the local node,
 not network-wide).

flock() locks are cluster aware in recent versions of OCFS2 and I would try to 
make it configurable which lock type to use.

 
 I'm pretty surprised that you can fcntl() is not supported in ocfs2.
 Are you really sure about that ?
 
 This mail message suggests it is supported
 
   https://oss.oracle.com/pipermail/ocfs2-users/2009-June/003572.html
 
 Support for fcntl locking aka file-range locking aka posix locking is
  provided by vfs for all file systems. However, that support is appropriate
  only for local file systems.
 
  In ocfs2, we have added support for cluster-aware fcntl locking via
  the userspace clustering framework that allows one to use ocfs2 with
  different cluster-stacks.

OCFS2 supports fcntl() only with the userspace cluster stacks (pacemaker and 
cman) which we want to avoid.

SUSE advises to use indirect leases which we also want to avoid:
https://www.suse.com/documentation//sled11/book_kvm/?page=/documentation//sled11/book_kvm/data/sec_libvirt_storage_locking.html

virtlockd's default configuration does not allow you to lock disk files placed 
on a shared file system (for example NFS or OCFS2). Locking on these file 
systems requires you to specify a lockspace directory on the VM Host Server by 
setting

Although that's not completely correct because NFS supports fcntl()

Cheers,
David
 
 Regards,
 Daniel

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

Re: [libvirt] Use flock() instead of fcntl()

2013-07-25 Thread Daniel P. Berrange 
On Thu, Jul 25, 2013 at 12:07:44PM +0200, David Weber wrote:
 Thank you for your quick response!
 
 Am Donnerstag, 25. Juli 2013, 10:31:40 schrieb Daniel P. Berrange:
  On Thu, Jul 25, 2013 at 08:23:24AM +, David Weber wrote:
   Hi,
   
   we are interested in using virtlockd on an OCFS2 shared filesystem.
   We are now facing the problem that virtlockd uses fcntl() locks which
   aren't supported by OCFS2 with the o2cb cluster stack and we want
   to avoid using indirect leases.
   
   OCFS2 instead supports flock() which is quite similar to fcntl(). I
   attached a patch which makes libvirt use flock() *instead* of fcntl()
   and it seems to work.
   
   NFS on the contrast only supports fcntl() so it should be configurable
   which lock type to use.
   
   I'm not very experienced with locking, so would such a patch be
   acceptable or do you see possible problems with it?
  
  We definitely can't use flock() unconditionally like that, as it is
  less widely supported than fcntl() locking and is known broken on
  most network filesystems (flock() will only lock wrt the local node,
  not network-wide).
 
 flock() locks are cluster aware in recent versions of OCFS2 and I would try 
 to 
 make it configurable which lock type to use.

  I'm pretty surprised that you can fcntl() is not supported in ocfs2.
  Are you really sure about that ?
  
  This mail message suggests it is supported
  
https://oss.oracle.com/pipermail/ocfs2-users/2009-June/003572.html
  
  Support for fcntl locking aka file-range locking aka posix locking is
   provided by vfs for all file systems. However, that support is appropriate
   only for local file systems.
  
   In ocfs2, we have added support for cluster-aware fcntl locking via
   the userspace clustering framework that allows one to use ocfs2 with
   different cluster-stacks.
 
 OCFS2 supports fcntl() only with the userspace cluster stacks (pacemaker and 
 cman) which we want to avoid.

Looking again at flock() I see it cannot support locking of ranges, only
the entire file. This makes it unsuitable as a replacement for libvirt's
use of fcntl() I'm afraid. I can only sugggest you configure OCFS2 so
that it supports fcntl(), or setup virtlockd to use separate indirect
leases on a diffrent shared filesystem, or perhaps try sanlock instead
which doesn't require any special filesystem support.

 SUSE advises to use indirect leases which we also want to avoid:
 https://www.suse.com/documentation//sled11/book_kvm/?page=/documentation//sled11/book_kvm/data/sec_libvirt_storage_locking.html
 
 virtlockd's default configuration does not allow you to lock disk files 
 placed 
 on a shared file system (for example NFS or OCFS2). Locking on these file 
 systems requires you to specify a lockspace directory on the VM Host Server 
 by 
 setting
 
 Although that's not completely correct because NFS supports fcntl()

That's just badly written explanation for that config setting. It should
really be saying that the default configuration does not provide protection
across multiple hosts for file paths which are not visible via a shared
filesystem. eg a SAN LUN in /dev/sdNNN won't be protected in the default
config.

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] RFC: Introduce API to return configuration/state paths of the network driver

2013-07-25 Thread Osier Yang 

On 25/07/13 17:35, Daniel P. Berrange wrote:

On Thu, Jul 25, 2013 at 02:02:36PM +0530, Nehal J. Wani wrote:

Currently, there is no API which returns configuration/state paths of the
network driver.
Although it is a private implementation of the network driver, I don't see
any harm in
making the locations public because although the locations might change,
there will always
be a location for these files. There is a need for this API to implement
method 2 of the
API to query ip addresses of a given domain, refer:
http://www.mail-archive.com/libvir-list@redhat.com/msg79793.html . It is
required to parse
the leases file generated by dnsmasq. So, this API will be used by the qemu
driver, but it
can also be made public, so that, if a user wants to know get some
information from a
configuration file, he can get the location from libvirt and analyze it on
his own. Right now,
there is an alternate way to get the info: by using
networkDnsmasqLeaseFileNameDefault,
defined in /src/network/bridge_driver.c Since this function is static, it
is part of the private
implementation and not visible outside. To make it public, the following
hack is possible:

NACK,

As I explained on IRC, the hypervisor drivers have no business accessing
the dnsmasq lease files from the bridge driver. This is considered to be
a private implementation detail.

At a conceptual level, what you're after here is a list of all the IP,
mac address mappings of the virtual network. This information is useful
even outside the context of the hypervisor driver method you're working
on. So we should create formal APIs for exposing this, something like:

virNetworkGetDHCPLeases(virNetworkPtr network,
virNetworkDHCPLeasePtr *leases,
unsigned int nleases);


i'm wondering if it should be more than just the lease file path, e.g.
also the $net.conf, $net-radvd.conf, etc, though they are useless
now, but may be useful in future, i.e. to have a more general api
than this one.  and in that case, it should return an array of typed
parameter instead.



And/or this

virNetworkGetDHCPLeaseForMAC(virNetworkPtr network,
 unsigned char *macaddr,
 virNetworkDHCPLeasePtr lease);

and a corresponding  'virsh net-dhcp-leases netname' command

Daniel


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

Re: [libvirt] RFC: Introduce API to return configuration/state paths of the network driver

2013-07-25 Thread Daniel P. Berrange 
On Thu, Jul 25, 2013 at 06:43:00PM +0800, Osier Yang wrote:
 On 25/07/13 17:35, Daniel P. Berrange wrote:
 On Thu, Jul 25, 2013 at 02:02:36PM +0530, Nehal J. Wani wrote:
 Currently, there is no API which returns configuration/state paths of the
 network driver.
 Although it is a private implementation of the network driver, I don't see
 any harm in
 making the locations public because although the locations might change,
 there will always
 be a location for these files. There is a need for this API to implement
 method 2 of the
 API to query ip addresses of a given domain, refer:
 http://www.mail-archive.com/libvir-list@redhat.com/msg79793.html . It is
 required to parse
 the leases file generated by dnsmasq. So, this API will be used by the qemu
 driver, but it
 can also be made public, so that, if a user wants to know get some
 information from a
 configuration file, he can get the location from libvirt and analyze it on
 his own. Right now,
 there is an alternate way to get the info: by using
 networkDnsmasqLeaseFileNameDefault,
 defined in /src/network/bridge_driver.c Since this function is static, it
 is part of the private
 implementation and not visible outside. To make it public, the following
 hack is possible:
 NACK,
 
 As I explained on IRC, the hypervisor drivers have no business accessing
 the dnsmasq lease files from the bridge driver. This is considered to be
 a private implementation detail.
 
 At a conceptual level, what you're after here is a list of all the IP,
 mac address mappings of the virtual network. This information is useful
 even outside the context of the hypervisor driver method you're working
 on. So we should create formal APIs for exposing this, something like:
 
 virNetworkGetDHCPLeases(virNetworkPtr network,
 virNetworkDHCPLeasePtr *leases,
 unsigned int nleases);
 
 i'm wondering if it should be more than just the lease file path, e.g.
 also the $net.conf, $net-radvd.conf, etc, though they are useless
 now, but may be useful in future, i.e. to have a more general api
 than this one.  and in that case, it should return an array of typed
 parameter instead.

We've already discussed this in the context of the virDomain API for
getting IP addresses  decided that virTypedParameter was not appropriate
there  we'd use a struct. The same arguments apply here IMHO.

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] RFC: Introduce API to return configuration/state paths of the network driver

2013-07-25 Thread Osier Yang 

On 25/07/13 18:53, Daniel P. Berrange wrote:

On Thu, Jul 25, 2013 at 06:43:00PM +0800, Osier Yang wrote:

On 25/07/13 17:35, Daniel P. Berrange wrote:

On Thu, Jul 25, 2013 at 02:02:36PM +0530, Nehal J. Wani wrote:

Currently, there is no API which returns configuration/state paths of the
network driver.
Although it is a private implementation of the network driver, I don't see
any harm in
making the locations public because although the locations might change,
there will always
be a location for these files. There is a need for this API to implement
method 2 of the
API to query ip addresses of a given domain, refer:
http://www.mail-archive.com/libvir-list@redhat.com/msg79793.html . It is
required to parse
the leases file generated by dnsmasq. So, this API will be used by the qemu
driver, but it
can also be made public, so that, if a user wants to know get some
information from a
configuration file, he can get the location from libvirt and analyze it on
his own. Right now,
there is an alternate way to get the info: by using
networkDnsmasqLeaseFileNameDefault,
defined in /src/network/bridge_driver.c Since this function is static, it
is part of the private
implementation and not visible outside. To make it public, the following
hack is possible:

NACK,

As I explained on IRC, the hypervisor drivers have no business accessing
the dnsmasq lease files from the bridge driver. This is considered to be
a private implementation detail.

At a conceptual level, what you're after here is a list of all the IP,
mac address mappings of the virtual network. This information is useful
even outside the context of the hypervisor driver method you're working
on. So we should create formal APIs for exposing this, something like:

virNetworkGetDHCPLeases(virNetworkPtr network,
virNetworkDHCPLeasePtr *leases,
unsigned int nleases);

i'm wondering if it should be more than just the lease file path, e.g.
also the $net.conf, $net-radvd.conf, etc, though they are useless
now, but may be useful in future, i.e. to have a more general api
than this one.  and in that case, it should return an array of typed
parameter instead.

We've already discussed this in the context of the virDomain API for
getting IP addresses  decided that virTypedParameter was not appropriate
there  we'd use a struct. The same arguments apply here IMHO.


the api to get the ip addresses is more complicate than this, and we
finally chose the struct is because of the multiple level information
is hard to constuct with typed parameter, but for this api, it's different,
as it just needs to return the file paths.

osier

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

Re: [libvirt] RFC: Introduce API to return configuration/state paths of the network driver

2013-07-25 Thread Daniel P. Berrange 
On Thu, Jul 25, 2013 at 07:01:05PM +0800, Osier Yang wrote:
 On 25/07/13 18:53, Daniel P. Berrange wrote:
 On Thu, Jul 25, 2013 at 06:43:00PM +0800, Osier Yang wrote:
 On 25/07/13 17:35, Daniel P. Berrange wrote:
 On Thu, Jul 25, 2013 at 02:02:36PM +0530, Nehal J. Wani wrote:
 Currently, there is no API which returns configuration/state paths of the
 network driver.
 Although it is a private implementation of the network driver, I don't see
 any harm in
 making the locations public because although the locations might change,
 there will always
 be a location for these files. There is a need for this API to implement
 method 2 of the
 API to query ip addresses of a given domain, refer:
 http://www.mail-archive.com/libvir-list@redhat.com/msg79793.html . It is
 required to parse
 the leases file generated by dnsmasq. So, this API will be used by the 
 qemu
 driver, but it
 can also be made public, so that, if a user wants to know get some
 information from a
 configuration file, he can get the location from libvirt and analyze it on
 his own. Right now,
 there is an alternate way to get the info: by using
 networkDnsmasqLeaseFileNameDefault,
 defined in /src/network/bridge_driver.c Since this function is static, it
 is part of the private
 implementation and not visible outside. To make it public, the following
 hack is possible:
 NACK,
 
 As I explained on IRC, the hypervisor drivers have no business accessing
 the dnsmasq lease files from the bridge driver. This is considered to be
 a private implementation detail.
 
 At a conceptual level, what you're after here is a list of all the IP,
 mac address mappings of the virtual network. This information is useful
 even outside the context of the hypervisor driver method you're working
 on. So we should create formal APIs for exposing this, something like:
 
 virNetworkGetDHCPLeases(virNetworkPtr network,
 virNetworkDHCPLeasePtr *leases,
 unsigned int nleases);
 i'm wondering if it should be more than just the lease file path, e.g.
 also the $net.conf, $net-radvd.conf, etc, though they are useless
 now, but may be useful in future, i.e. to have a more general api
 than this one.  and in that case, it should return an array of typed
 parameter instead.
 We've already discussed this in the context of the virDomain API for
 getting IP addresses  decided that virTypedParameter was not appropriate
 there  we'd use a struct. The same arguments apply here IMHO.
 
 the api to get the ip addresses is more complicate than this, and we
 finally chose the struct is because of the multiple level information
 is hard to constuct with typed parameter, but for this api, it's different,
 as it just needs to return the file paths.

No, file paths will absolutely never be exposed outside of the bridge
driver. The API I suggest above are about exposing the IP address + MAC
address of current leases. ie the actual data the user needs, *not* the
file path containing the data which is a private impl detail.


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] RFC: Introduce API to return configuration/state paths of the network driver

2013-07-25 Thread Osier Yang 

On 25/07/13 19:13, Daniel P. Berrange wrote:

On Thu, Jul 25, 2013 at 07:01:05PM +0800, Osier Yang wrote:

On 25/07/13 18:53, Daniel P. Berrange wrote:

On Thu, Jul 25, 2013 at 06:43:00PM +0800, Osier Yang wrote:

On 25/07/13 17:35, Daniel P. Berrange wrote:

On Thu, Jul 25, 2013 at 02:02:36PM +0530, Nehal J. Wani wrote:

Currently, there is no API which returns configuration/state paths of the
network driver.
Although it is a private implementation of the network driver, I don't see
any harm in
making the locations public because although the locations might change,
there will always
be a location for these files. There is a need for this API to implement
method 2 of the
API to query ip addresses of a given domain, refer:
http://www.mail-archive.com/libvir-list@redhat.com/msg79793.html . It is
required to parse
the leases file generated by dnsmasq. So, this API will be used by the qemu
driver, but it
can also be made public, so that, if a user wants to know get some
information from a
configuration file, he can get the location from libvirt and analyze it on
his own. Right now,
there is an alternate way to get the info: by using
networkDnsmasqLeaseFileNameDefault,
defined in /src/network/bridge_driver.c Since this function is static, it
is part of the private
implementation and not visible outside. To make it public, the following
hack is possible:

NACK,

As I explained on IRC, the hypervisor drivers have no business accessing
the dnsmasq lease files from the bridge driver. This is considered to be
a private implementation detail.

At a conceptual level, what you're after here is a list of all the IP,
mac address mappings of the virtual network. This information is useful
even outside the context of the hypervisor driver method you're working
on. So we should create formal APIs for exposing this, something like:

virNetworkGetDHCPLeases(virNetworkPtr network,
virNetworkDHCPLeasePtr *leases,
unsigned int nleases);

i'm wondering if it should be more than just the lease file path, e.g.
also the $net.conf, $net-radvd.conf, etc, though they are useless
now, but may be useful in future, i.e. to have a more general api
than this one.  and in that case, it should return an array of typed
parameter instead.

We've already discussed this in the context of the virDomain API for
getting IP addresses  decided that virTypedParameter was not appropriate
there  we'd use a struct. The same arguments apply here IMHO.


the api to get the ip addresses is more complicate than this, and we
finally chose the struct is because of the multiple level information
is hard to constuct with typed parameter, but for this api, it's different,
as it just needs to return the file paths.

No, file paths will absolutely never be exposed outside of the bridge
driver. The API I suggest above are about exposing the IP address + MAC
address of current leases. ie the actual data the user needs, *not* the
file path containing the data which is a private impl detail.


oh, i see, agreed with the idea then.

osier

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

[libvirt] [PATCH v2] virdbustest: Don't pass number of arguments as long long

2013-07-25 Thread Guido Günther 
since sizeof(int) != sizeof(long long) on 32bit archs.

This unbreaks virdbustest which otherwise fails like:

 (gdb) bt
 #0  __strlen_sse2_bsf () at ../sysdeps/i386/i686/multiarch/strlen-sse2-bsf.S:50
 #1  0x405907d2 in ?? () from /lib/i386-linux-gnu/libdbus-1.so.3
 #2  0x4057c140 in ?? () from /lib/i386-linux-gnu/libdbus-1.so.3
 #3  0x4057e7ec in dbus_message_iter_append_basic () from 
/lib/i386-linux-gnu/libdbus-1.so.3
 #4  0x400742ec in virDBusMessageIterEncode (args=0xbfd4b8f0 k\321\004\b., 
types=0x804d260 ,
 rootiter=0xbfd4b844) at util/virdbus.c:560
 #5  virDBusMessageEncodeArgs (msg=msg@entry=0x893c278, 
types=types@entry=0x804d25c sais,
 args=args@entry=0xbfd4b8d8 r\320\004\b\003) at util/virdbus.c:921
 #6  0x40075917 in virDBusMessageEncode (msg=0x893c278, types=0x804d25c sais) 
at util/virdbus.c:959
 #7  0x0804a4a1 in testMessageArray (args=0x0) at virdbustest.c:195
 #8  0x0804c404 in virtTestRun (title=title@entry=0x804cfcb Test message array 
,
 nloops=nloops@entry=1, body=body@entry=0x804a3f0 testMessageArray, 
data=data@entry=0x0)
 at testutils.c:168
 #9  0x08049346 in mymain () at virdbustest.c:384
 #10 0x0804cb2e in virtTestMain (argc=argc@entry=1, argv=argv@entry=0xbfd4bb24,
 func=func@entry=0x80492c0 mymain) at testutils.c:764
 #11 0x080491af in main (argc=1, argv=0xbfd4bb24) at virdbustest.c:393
---
 tests/virdbustest.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/virdbustest.c b/tests/virdbustest.c
index e054716..fb241ee 100644
--- a/tests/virdbustest.c
+++ b/tests/virdbustest.c
@@ -195,7 +195,7 @@ static int testMessageArray(const void *args 
ATTRIBUTE_UNUSED)
 if (virDBusMessageEncode(msg,
  sais,
  in_str1,
- (long long)3, in_int32a, in_int32b, in_int32c,
+ 3, in_int32a, in_int32b, in_int32c,
  in_str2)  0) {
 VIR_DEBUG(Failed to encode arguments);
 goto cleanup;
-- 
1.8.3.2

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

Re: [libvirt] [PATCH v2] virdbustest: Don't pass number of arguments as long long

2013-07-25 Thread Daniel P. Berrange 
On Thu, Jul 25, 2013 at 01:25:32PM +0200, Guido Günther wrote:
 since sizeof(int) != sizeof(long long) on 32bit archs.
 
 This unbreaks virdbustest which otherwise fails like:
 
  (gdb) bt
  #0  __strlen_sse2_bsf () at 
 ../sysdeps/i386/i686/multiarch/strlen-sse2-bsf.S:50
  #1  0x405907d2 in ?? () from /lib/i386-linux-gnu/libdbus-1.so.3
  #2  0x4057c140 in ?? () from /lib/i386-linux-gnu/libdbus-1.so.3
  #3  0x4057e7ec in dbus_message_iter_append_basic () from 
 /lib/i386-linux-gnu/libdbus-1.so.3
  #4  0x400742ec in virDBusMessageIterEncode (args=0xbfd4b8f0 k\321\004\b., 
 types=0x804d260 ,
  rootiter=0xbfd4b844) at util/virdbus.c:560
  #5  virDBusMessageEncodeArgs (msg=msg@entry=0x893c278, 
 types=types@entry=0x804d25c sais,
  args=args@entry=0xbfd4b8d8 r\320\004\b\003) at util/virdbus.c:921
  #6  0x40075917 in virDBusMessageEncode (msg=0x893c278, types=0x804d25c 
 sais) at util/virdbus.c:959
  #7  0x0804a4a1 in testMessageArray (args=0x0) at virdbustest.c:195
  #8  0x0804c404 in virtTestRun (title=title@entry=0x804cfcb Test message 
 array ,
  nloops=nloops@entry=1, body=body@entry=0x804a3f0 testMessageArray, 
 data=data@entry=0x0)
  at testutils.c:168
  #9  0x08049346 in mymain () at virdbustest.c:384
  #10 0x0804cb2e in virtTestMain (argc=argc@entry=1, 
 argv=argv@entry=0xbfd4bb24,
  func=func@entry=0x80492c0 mymain) at testutils.c:764
  #11 0x080491af in main (argc=1, argv=0xbfd4bb24) at virdbustest.c:393
 ---
  tests/virdbustest.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
 
 diff --git a/tests/virdbustest.c b/tests/virdbustest.c
 index e054716..fb241ee 100644
 --- a/tests/virdbustest.c
 +++ b/tests/virdbustest.c
 @@ -195,7 +195,7 @@ static int testMessageArray(const void *args 
 ATTRIBUTE_UNUSED)
  if (virDBusMessageEncode(msg,
   sais,
   in_str1,
 - (long long)3, in_int32a, in_int32b, in_int32c,
 + 3, in_int32a, in_int32b, in_int32c,
   in_str2)  0) {
  VIR_DEBUG(Failed to encode arguments);
  goto cleanup;

ACK


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

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

Re: [libvirt] RFC: Introduce API to return configuration/state paths of the network driver

2013-07-25 Thread Osier Yang 

On 25/07/13 19:21, Osier Yang wrote:

On 25/07/13 19:13, Daniel P. Berrange wrote:

On Thu, Jul 25, 2013 at 07:01:05PM +0800, Osier Yang wrote:

On 25/07/13 18:53, Daniel P. Berrange wrote:

On Thu, Jul 25, 2013 at 06:43:00PM +0800, Osier Yang wrote:

On 25/07/13 17:35, Daniel P. Berrange wrote:

On Thu, Jul 25, 2013 at 02:02:36PM +0530, Nehal J. Wani wrote:
Currently, there is no API which returns configuration/state 
paths of the

network driver.
Although it is a private implementation of the network driver, I 
don't see

any harm in
making the locations public because although the locations might 
change,

there will always
be a location for these files. There is a need for this API to 
implement

method 2 of the
API to query ip addresses of a given domain, refer:
http://www.mail-archive.com/libvir-list@redhat.com/msg79793.html 
. It is

required to parse
the leases file generated by dnsmasq. So, this API will be used 
by the qemu

driver, but it
can also be made public, so that, if a user wants to know get some
information from a
configuration file, he can get the location from libvirt and 
analyze it on

his own. Right now,
there is an alternate way to get the info: by using
networkDnsmasqLeaseFileNameDefault,
defined in /src/network/bridge_driver.c Since this function is 
static, it

is part of the private
implementation and not visible outside. To make it public, the 
following

hack is possible:

NACK,

As I explained on IRC, the hypervisor drivers have no business 
accessing
the dnsmasq lease files from the bridge driver. This is 
considered to be

a private implementation detail.

At a conceptual level, what you're after here is a list of all 
the IP,
mac address mappings of the virtual network. This information is 
useful
even outside the context of the hypervisor driver method you're 
working
on. So we should create formal APIs for exposing this, something 
like:


virNetworkGetDHCPLeases(virNetworkPtr network,
virNetworkDHCPLeasePtr *leases,
unsigned int nleases);
i'm wondering if it should be more than just the lease file path, 
e.g.

also the $net.conf, $net-radvd.conf, etc, though they are useless
now, but may be useful in future, i.e. to have a more general api
than this one.  and in that case, it should return an array of typed
parameter instead.

We've already discussed this in the context of the virDomain API for
getting IP addresses  decided that virTypedParameter was not 
appropriate

there  we'd use a struct. The same arguments apply here IMHO.


the api to get the ip addresses is more complicate than this, and we
finally chose the struct is because of the multiple level information
is hard to constuct with typed parameter, but for this api, it's 
different,

as it just needs to return the file paths.

No, file paths will absolutely never be exposed outside of the bridge
driver. The API I suggest above are about exposing the IP address + MAC
address of current leases. ie the actual data the user needs, *not* the
file path containing the data which is a private impl detail.


oh, i see, agreed with the idea then.


for the api interface:

int
virNetworkGetDHCPLeases(virNetworkPtr network,
unsigned char *macaddr,
virNetworkDHCPLeasePtr *leases,
unsigned int nleases);

i think this is better. which returns all of the leases if no mac is 
specified.

otherwise just returns the lease of the network matches the mac.

osier

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

Re: [libvirt] RFC: Introduce API to return configuration/state paths of the network driver

2013-07-25 Thread Daniel P. Berrange 
On Thu, Jul 25, 2013 at 07:37:22PM +0800, Osier Yang wrote:
 On 25/07/13 19:21, Osier Yang wrote:
 On 25/07/13 19:13, Daniel P. Berrange wrote:
 On Thu, Jul 25, 2013 at 07:01:05PM +0800, Osier Yang wrote:
 On 25/07/13 18:53, Daniel P. Berrange wrote:
 On Thu, Jul 25, 2013 at 06:43:00PM +0800, Osier Yang wrote:
 On 25/07/13 17:35, Daniel P. Berrange wrote:
 On Thu, Jul 25, 2013 at 02:02:36PM +0530, Nehal J. Wani wrote:
 Currently, there is no API which returns
 configuration/state paths of the
 network driver.
 Although it is a private implementation of the network
 driver, I don't see
 any harm in
 making the locations public because although the
 locations might change,
 there will always
 be a location for these files. There is a need for
 this API to implement
 method 2 of the
 API to query ip addresses of a given domain, refer:
 http://www.mail-archive.com/libvir-list@redhat.com/msg79793.html
 . It is
 required to parse
 the leases file generated by dnsmasq. So, this API
 will be used by the qemu
 driver, but it
 can also be made public, so that, if a user wants to know get some
 information from a
 configuration file, he can get the location from
 libvirt and analyze it on
 his own. Right now,
 there is an alternate way to get the info: by using
 networkDnsmasqLeaseFileNameDefault,
 defined in /src/network/bridge_driver.c Since this
 function is static, it
 is part of the private
 implementation and not visible outside. To make it
 public, the following
 hack is possible:
 NACK,
 
 As I explained on IRC, the hypervisor drivers have no
 business accessing
 the dnsmasq lease files from the bridge driver. This is
 considered to be
 a private implementation detail.
 
 At a conceptual level, what you're after here is a list
 of all the IP,
 mac address mappings of the virtual network. This
 information is useful
 even outside the context of the hypervisor driver method
 you're working
 on. So we should create formal APIs for exposing this,
 something like:
 
 virNetworkGetDHCPLeases(virNetworkPtr network,
 virNetworkDHCPLeasePtr *leases,
 unsigned int nleases);
 i'm wondering if it should be more than just the lease
 file path, e.g.
 also the $net.conf, $net-radvd.conf, etc, though they are useless
 now, but may be useful in future, i.e. to have a more general api
 than this one.  and in that case, it should return an array of typed
 parameter instead.
 We've already discussed this in the context of the virDomain API for
 getting IP addresses  decided that virTypedParameter was
 not appropriate
 there  we'd use a struct. The same arguments apply here IMHO.
 
 the api to get the ip addresses is more complicate than this, and we
 finally chose the struct is because of the multiple level information
 is hard to constuct with typed parameter, but for this api,
 it's different,
 as it just needs to return the file paths.
 No, file paths will absolutely never be exposed outside of the bridge
 driver. The API I suggest above are about exposing the IP address + MAC
 address of current leases. ie the actual data the user needs, *not* the
 file path containing the data which is a private impl detail.
 
 oh, i see, agreed with the idea then.
 
 for the api interface:
 
 int
 virNetworkGetDHCPLeases(virNetworkPtr network,
 unsigned char *macaddr,
 virNetworkDHCPLeasePtr *leases,
 unsigned int nleases);
 
 i think this is better. which returns all of the leases if no mac is
 specified.
 otherwise just returns the lease of the network matches the mac.

I rather prefer to see separate APIs for this job as I described. Sure
you could have an optional macaddr parameter, but I think it is nicer
to just have clear APIs for the list many vs get one tasks.

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 v2] Don't overwrite errors in qemuTranslateDiskSourcePool

2013-07-25 Thread Ján Tomko 
On 07/24/2013 10:44 PM, Eric Blake wrote:
 On 07/24/2013 03:47 AM, Ján Tomko wrote:
  src/qemu/qemu_conf.c | 14 --
  1 file changed, 12 insertions(+), 2 deletions(-)
 
 ACK.
 

Thanks, pushed.

Jan

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

Re: [libvirt] [PATCH 1/4] Use separate macros for failure/success in vol-to-argv test

2013-07-25 Thread Ján Tomko 
On 07/25/2013 12:29 AM, Eric Blake wrote:
 On 07/22/2013 08:52 AM, Ján Tomko wrote:
 Reindent them to put the input volume on a separate line.
 ---
  tests/storagevolxml2argvtest.c | 64 
 +-
  1 file changed, 44 insertions(+), 20 deletions(-)

 diff --git a/tests/storagevolxml2argvtest.c b/tests/storagevolxml2argvtest.c
 index 89c233f..4f4bf7d 100644
 --- a/tests/storagevolxml2argvtest.c
 +++ b/tests/storagevolxml2argvtest.c
 @@ -161,7 +161,8 @@ mymain(void)
  int ret = 0;
  unsigned int flags = VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA;
  
 -#define DO_TEST(shouldFail, pool, vol, inputvol, cmdline, flags, imgformat) 
 \
 +#define DO_TEST_FULL(shouldFail, pool, vol, inputvol, cmdline, flags, \
 + imgformat) \
 
 Is it worth lining up the \ to a single column?  But that's cosmetic, I
 don't care if you do that or not.
 
 ACK.
 

I've lined them up and pushed the series.

Jan

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

Re: [libvirt] [PATCH] tests: PCI controller checks

2013-07-25 Thread Ján Tomko 
On 07/24/2013 11:29 PM, Eric Blake wrote:
 On 07/22/2013 08:48 AM, Ján Tomko wrote:
 Check if PCI bridges with duplicate indexes are rejected.
 PCI root controllers with non-zero indexes or addresses should
 also be rejected.
 ---
  .../qemuxml2argv-pci-bridge-duplicate-index.xml  | 16 
 
  tests/qemuxml2argvdata/qemuxml2argv-pci-root-address.xml | 16 
 
  .../qemuxml2argv-pci-root-nonzero-index.xml  | 14 ++
  tests/qemuxml2argvtest.c |  6 ++
  4 files changed, 52 insertions(+)
  create mode 100644 
 tests/qemuxml2argvdata/qemuxml2argv-pci-bridge-duplicate-index.xml
  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pci-root-address.xml
  create mode 100644 
 tests/qemuxml2argvdata/qemuxml2argv-pci-root-nonzero-index.xml
 
 ACK.  Adding test cases is always safe, even in freeze.
 

Thanks, pushed now.

Jan

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

[libvirt] [PATCH 4/5] Add 'controllers' arg to virCgroupNewDetect

2013-07-25 Thread Daniel P. Berrange 
From: Daniel P. Berrange berra...@redhat.com

When detecting cgroups we must honour any controllers
whitelist the driver may have.

Signed-off-by: Daniel P. Berrange berra...@redhat.com
---
 src/lxc/lxc_process.c  | 4 ++--
 src/qemu/qemu_cgroup.c | 1 +
 src/util/vircgroup.c   | 9 ++---
 src/util/vircgroup.h   | 2 ++
 4 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c
index e632e13..1a5686f 100644
--- a/src/lxc/lxc_process.c
+++ b/src/lxc/lxc_process.c
@@ -1190,7 +1190,7 @@ int virLXCProcessStart(virConnectPtr conn,
 }
 
 if (virCgroupNewDetectMachine(vm-def-name, lxc,
-  vm-pid, priv-cgroup)  0)
+  vm-pid, -1, priv-cgroup)  0)
 goto error;
 
 if (!priv-cgroup) {
@@ -1398,7 +1398,7 @@ virLXCProcessReconnectDomain(virDomainObjPtr vm,
 goto error;
 
 if (virCgroupNewDetectMachine(vm-def-name, lxc,
-  vm-pid, priv-cgroup)  0)
+  vm-pid, -1, priv-cgroup)  0)
 goto error;
 
 if (!priv-cgroup) {
diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
index 07e901c..9f6b251 100644
--- a/src/qemu/qemu_cgroup.c
+++ b/src/qemu/qemu_cgroup.c
@@ -707,6 +707,7 @@ qemuConnectCgroup(virQEMUDriverPtr driver,
 if (virCgroupNewDetectMachine(vm-def-name,
   qemu,
   vm-pid,
+  cfg-cgroupControllers,
   priv-cgroup)  0)
 goto cleanup;
 
diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
index 9065675..3818172 100644
--- a/src/util/vircgroup.c
+++ b/src/util/vircgroup.c
@@ -1406,7 +1406,7 @@ int virCgroupNewPartition(const char *path 
ATTRIBUTE_UNUSED,
 */
 int virCgroupNewSelf(virCgroupPtr *group)
 {
-return virCgroupNewDetect(-1, group);
+return virCgroupNewDetect(-1, -1, group);
 }
 
 
@@ -1577,12 +1577,14 @@ int virCgroupNewEmulator(virCgroupPtr domain 
ATTRIBUTE_UNUSED,
 
 #if defined HAVE_MNTENT_H  defined HAVE_GETMNTENT_R
 int virCgroupNewDetect(pid_t pid,
+   int controllers,
virCgroupPtr *group)
 {
-return virCgroupNew(pid, , NULL, -1, group);
+return virCgroupNew(pid, , NULL, controllers, group);
 }
 #else
 int virCgroupNewDetect(pid_t pid ATTRIBUTE_UNUSED,
+   int controllers ATTRIBUTE_UNUSED,
virCgroupPtr *group ATTRIBUTE_UNUSED)
 {
 virReportSystemError(ENXIO, %s,
@@ -1597,9 +1599,10 @@ int virCgroupNewDetect(pid_t pid ATTRIBUTE_UNUSED,
 int virCgroupNewDetectMachine(const char *name,
   const char *drivername,
   pid_t pid,
+  int controllers,
   virCgroupPtr *group)
 {
-if (virCgroupNewDetect(pid, group)  0) {
+if (virCgroupNewDetect(pid, controllers, group)  0) {
 if (virCgroupNewIgnoreError())
 return 0;
 return -1;
diff --git a/src/util/vircgroup.h b/src/util/vircgroup.h
index d6222d7..3aaf081 100644
--- a/src/util/vircgroup.h
+++ b/src/util/vircgroup.h
@@ -76,11 +76,13 @@ int virCgroupNewEmulator(virCgroupPtr domain,
 ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3);
 
 int virCgroupNewDetect(pid_t pid,
+   int controllers,
virCgroupPtr *group);
 
 int virCgroupNewDetectMachine(const char *name,
   const char *drivername,
   pid_t pid,
+  int controllers,
   virCgroupPtr *group);
 
 int virCgroupNewMachine(const char *name,
-- 
1.8.1.4

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

[libvirt] [PATCH 2/5] Make virCgroupIsValidMachine static

2013-07-25 Thread Daniel P. Berrange 
From: Daniel P. Berrange berra...@redhat.com

The virCgroupIsValidMachine does not need to be called from
outside the cgroups file now, so make it static.

Signed-off-by: Daniel P. Berrange berra...@redhat.com
---
 src/libvirt_private.syms | 1 -
 src/util/vircgroup.c | 7 ---
 src/util/vircgroup.h | 5 -
 3 files changed, 4 insertions(+), 9 deletions(-)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index b076e60..d9615ea 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1186,7 +1186,6 @@ virCgroupGetMemSwapHardLimit;
 virCgroupGetMemSwapUsage;
 virCgroupHasController;
 virCgroupIsolateMount;
-virCgroupIsValidMachineGroup;
 virCgroupKill;
 virCgroupKillPainfully;
 virCgroupKillRecursive;
diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
index fe6c314..308f1a1 100644
--- a/src/util/vircgroup.c
+++ b/src/util/vircgroup.c
@@ -95,9 +95,10 @@ bool virCgroupAvailable(void)
 return ret;
 }
 
-bool virCgroupIsValidMachineGroup(virCgroupPtr group,
-  const char *name,
-  const char *drivername)
+static bool 
+virCgroupIsValidMachineGroup(virCgroupPtr group,
+ const char *name,
+ const char *drivername)
 {
 size_t i;
 bool valid = false;
diff --git a/src/util/vircgroup.h b/src/util/vircgroup.h
index 4f72aa8..d6222d7 100644
--- a/src/util/vircgroup.h
+++ b/src/util/vircgroup.h
@@ -48,11 +48,6 @@ VIR_ENUM_DECL(virCgroupController);
 
 bool virCgroupAvailable(void);
 
-bool virCgroupIsValidMachineGroup(virCgroupPtr group,
-  const char *machinename,
-  const char *drivername);
-
-
 int virCgroupNewPartition(const char *path,
   bool create,
   int controllers,
-- 
1.8.1.4

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

[libvirt] [PATCH 1/5] Introduce a more convenient virCgroupNewDetectMachine

2013-07-25 Thread Daniel P. Berrange 
From: Daniel P. Berrange berra...@redhat.com

Instead of requiring drivers to use a combination of calls
to virCgroupNewDetect and virCgroupIsValidMachine, combine
the two into virCgroupNewDetectMachine

Signed-off-by: Daniel P. Berrange berra...@redhat.com
---
 src/libvirt_private.syms |  1 +
 src/lxc/lxc_process.c| 20 
 src/qemu/qemu_cgroup.c   | 16 
 src/util/vircgroup.c | 22 ++
 src/util/vircgroup.h |  5 +
 5 files changed, 40 insertions(+), 24 deletions(-)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index d5ec146..b076e60 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1192,6 +1192,7 @@ virCgroupKillPainfully;
 virCgroupKillRecursive;
 virCgroupMoveTask;
 virCgroupNewDetect;
+virCgroupNewDetectMachine;
 virCgroupNewDomainPartition;
 virCgroupNewEmulator;
 virCgroupNewIgnoreError;
diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c
index 06ead9f..e632e13 100644
--- a/src/lxc/lxc_process.c
+++ b/src/lxc/lxc_process.c
@@ -1189,16 +1189,14 @@ int virLXCProcessStart(virConnectPtr conn,
 goto cleanup;
 }
 
-if (virCgroupNewDetect(vm-pid, priv-cgroup)  0)
+if (virCgroupNewDetectMachine(vm-def-name, lxc,
+  vm-pid, priv-cgroup)  0)
 goto error;
 
-if (!virCgroupIsValidMachineGroup(priv-cgroup,
-  vm-def-name,
-  lxc)) {
+if (!priv-cgroup) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
-   _(Cgroup name is not valid for machine %s),
+   _(No valid cgroup for machine %s),
vm-def-name);
-virCgroupFree(priv-cgroup);
 goto error;
 }
 
@@ -1399,16 +1397,14 @@ virLXCProcessReconnectDomain(virDomainObjPtr vm,
 if (!(priv-monitor = virLXCProcessConnectMonitor(driver, vm)))
 goto error;
 
-if (virCgroupNewDetect(vm-pid, priv-cgroup)  0)
+if (virCgroupNewDetectMachine(vm-def-name, lxc,
+  vm-pid, priv-cgroup)  0)
 goto error;
 
-if (!virCgroupIsValidMachineGroup(priv-cgroup,
-  vm-def-name,
-  lxc)) {
+if (!priv-cgroup) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
-   _(Cgroup name is not valid for machine %s),
+   _(No valid cgroup for machine %s),
vm-def-name);
-virCgroupFree(priv-cgroup);
 goto error;
 }
 
diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
index bca8630..07e901c 100644
--- a/src/qemu/qemu_cgroup.c
+++ b/src/qemu/qemu_cgroup.c
@@ -704,19 +704,11 @@ qemuConnectCgroup(virQEMUDriverPtr driver,
 
 virCgroupFree(priv-cgroup);
 
-if (virCgroupNewDetect(vm-pid, priv-cgroup)  0) {
-if (virCgroupNewIgnoreError())
-goto done;
+if (virCgroupNewDetectMachine(vm-def-name,
+  qemu,
+  vm-pid,
+  priv-cgroup)  0)
 goto cleanup;
-}
-
-if (!virCgroupIsValidMachineGroup(priv-cgroup,
-  vm-def-name,
-  qemu)) {
-VIR_DEBUG(Cgroup name is not valid for machine);
-virCgroupFree(priv-cgroup);
-goto done;
-}
 
 done:
 ret = 0;
diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
index 87325c0..fe6c314 100644
--- a/src/util/vircgroup.c
+++ b/src/util/vircgroup.c
@@ -1575,6 +1575,28 @@ int virCgroupNewDetect(pid_t pid ATTRIBUTE_UNUSED,
 }
 #endif
 
+/*
+ * Returns 0 on success, -1 on fatal error, -2 on no valid cgroup
+ */
+int virCgroupNewDetectMachine(const char *name,
+  const char *drivername,
+  pid_t pid,
+  virCgroupPtr *group)
+{
+if (virCgroupNewDetect(pid, group)  0) {
+if (virCgroupNewIgnoreError())
+return 0;
+return -1;
+}
+
+if (!virCgroupIsValidMachineGroup(*group, name, drivername)) {
+virCgroupFree(group);
+return 0;
+}
+
+return 0;
+}
+
 int virCgroupNewMachine(const char *name,
 const char *drivername,
 bool privileged ATTRIBUTE_UNUSED,
diff --git a/src/util/vircgroup.h b/src/util/vircgroup.h
index e47367c..4f72aa8 100644
--- a/src/util/vircgroup.h
+++ b/src/util/vircgroup.h
@@ -83,6 +83,11 @@ int virCgroupNewEmulator(virCgroupPtr domain,
 int virCgroupNewDetect(pid_t pid,
virCgroupPtr *group);
 
+int virCgroupNewDetectMachine(const char *name,
+  const char *drivername,
+  pid_t pid,
+  virCgroupPtr *group);
+

[libvirt] [PATCH 0/5] Fix detecting cgroups at libvirtd restart with QEMU

2013-07-25 Thread Daniel P. Berrange 
From: Daniel P. Berrange berra...@redhat.com

The recent refactoring of cgroups broke the ability to detect
cgroups for running guests in the QEMU driver during libvirtd
startup. This was due to it not considering the existance of
the 'emulator' child group, as well as not honouring the
'cgroups_controllers' setting when it was present

Daniel P. Berrange (5):
  Introduce a more convenient virCgroupNewDetectMachine
  Make virCgroupIsValidMachine static
  Fix detection of 'emulator' cgroup
  Add 'controllers' arg to virCgroupNewDetect
  Skip detecting placement if controller is disabled

 src/libvirt_private.syms |  2 +-
 src/lxc/lxc_process.c| 20 +++-
 src/qemu/qemu_cgroup.c   | 17 --
 src/util/vircgroup.c | 60 +---
 src/util/vircgroup.h | 12 ++
 5 files changed, 73 insertions(+), 38 deletions(-)

-- 
1.8.1.4

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

[libvirt] [PATCH 5/5] Skip detecting placement if controller is disabled

2013-07-25 Thread Daniel P. Berrange 
From: Daniel P. Berrange berra...@redhat.com

If the app has provided a whitelist of controllers to be used,
we skip detecting its mount point. We still, however, fill in
the placement info which later confuses the machine name
validation code. Skip detecting placement if the controller
mount point is not set

Signed-off-by: Daniel P. Berrange berra...@redhat.com
---
 src/util/vircgroup.c | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
index 3818172..827d894 100644
--- a/src/util/vircgroup.c
+++ b/src/util/vircgroup.c
@@ -137,8 +137,8 @@ virCgroupValidateMachineGroup(virCgroupPtr group,
 
 if (STRNEQ(tmp, name) 
 STRNEQ(tmp, partname)) {
-VIR_DEBUG(Name '%s' does not match '%s' or '%s',
-  tmp, name, partname);
+VIR_DEBUG(Name '%s' for controller '%s' does not match '%s' or 
'%s',
+  tmp, virCgroupControllerTypeToString(i), name, partname);
 goto cleanup;
 }
 }
@@ -426,7 +426,8 @@ static int virCgroupDetectPlacement(virCgroupPtr group,
  * selfpath==/libvirt.service + path= - /libvirt.service
  * selfpath==/libvirt.service + path=foo - 
/libvirt.service/foo
  */
-if (typelen == len  STREQLEN(typestr, tmp, len)) {
+if (typelen == len  STREQLEN(typestr, tmp, len) 
+group-controllers[i].mountPoint != NULL) {
 if (virAsprintf(group-controllers[i].placement,
 %s%s%s, selfpath,
 (STREQ(selfpath, /) ||
-- 
1.8.1.4

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

[libvirt] [PATCH 3/5] Fix detection of 'emulator' cgroup

2013-07-25 Thread Daniel P. Berrange 
From: Daniel P. Berrange berra...@redhat.com

When a VM has an 'emulator' child cgroup present, we must
strip off that suffix when detecting the cgroup for a
machine

Rename the virCgroupIsValidMachineGroup method to
virCgroupValidateMachineGroup to make a bit clearer
that this isn't simply a boolean check, it will make
changes to the object.

Signed-off-by: Daniel P. Berrange berra...@redhat.com
---
 src/util/vircgroup.c | 29 +++--
 1 file changed, 23 insertions(+), 6 deletions(-)

diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
index 308f1a1..9065675 100644
--- a/src/util/vircgroup.c
+++ b/src/util/vircgroup.c
@@ -96,9 +96,10 @@ bool virCgroupAvailable(void)
 }
 
 static bool 
-virCgroupIsValidMachineGroup(virCgroupPtr group,
- const char *name,
- const char *drivername)
+virCgroupValidateMachineGroup(virCgroupPtr group,
+  const char *name,
+  const char *drivername,
+  bool stripEmulatorSuffix)
 {
 size_t i;
 bool valid = false;
@@ -120,12 +121,26 @@ virCgroupIsValidMachineGroup(virCgroupPtr group,
 tmp = strrchr(group-controllers[i].placement, '/');
 if (!tmp)
 goto cleanup;
+
+if (stripEmulatorSuffix 
+(i == VIR_CGROUP_CONTROLLER_CPU ||
+ i == VIR_CGROUP_CONTROLLER_CPUACCT ||
+ i == VIR_CGROUP_CONTROLLER_CPUSET)) {
+if (STREQ(tmp, /emulator))
+*tmp = '\0';
+tmp = strrchr(group-controllers[i].placement, '/');
+if (!tmp)
+goto cleanup;
+}
+
 tmp++;
 
 if (STRNEQ(tmp, name) 
-STRNEQ(tmp, partname))
+STRNEQ(tmp, partname)) {
+VIR_DEBUG(Name '%s' does not match '%s' or '%s',
+  tmp, name, partname);
 goto cleanup;
-
+}
 }
 
 valid = true;
@@ -1590,7 +1605,9 @@ int virCgroupNewDetectMachine(const char *name,
 return -1;
 }
 
-if (!virCgroupIsValidMachineGroup(*group, name, drivername)) {
+if (!virCgroupValidateMachineGroup(*group, name, drivername, true)) {
+VIR_DEBUG(Failed to validate machine name for '%s' driver '%s',
+  name, drivername);
 virCgroupFree(group);
 return 0;
 }
-- 
1.8.1.4

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

Re: [libvirt] [PATCH 4/7] qemu: Add monitor APIs to fetch CPUID data from QEMU

2013-07-25 Thread Eduardo Habkost 
On Thu, Jul 25, 2013 at 10:45:10AM +0100, Daniel P. Berrange wrote:
 On Wed, Jul 24, 2013 at 03:25:19PM -0300, Eduardo Habkost wrote:
  On Tue, Jul 23, 2013 at 07:32:46PM +0200, Jiri Denemark wrote:
   On Tue, Jul 23, 2013 at 19:28:38 +0200, Jiri Denemark wrote:
On Tue, Jul 23, 2013 at 17:32:42 +0100, Daniel Berrange wrote:
 On Tue, Jul 23, 2013 at 06:11:33PM +0200, Jiri Denemark wrote:
  ---
   src/qemu/qemu_monitor.c|  21 +++
   src/qemu/qemu_monitor.h|   3 +
   src/qemu/qemu_monitor_json.c   | 162 
  +
   src/qemu/qemu_monitor_json.h   |   6 +
   tests/Makefile.am  |   1 +
   .../qemumonitorjson-getcpu-empty.data  |   2 +
   .../qemumonitorjson-getcpu-empty.json  |  46 ++
   .../qemumonitorjson-getcpu-filtered.data   |   4 +
   .../qemumonitorjson-getcpu-filtered.json   |  46 ++
   .../qemumonitorjson-getcpu-full.data   |   4 +
   .../qemumonitorjson-getcpu-full.json   |  46 ++
   .../qemumonitorjson-getcpu-host.data   |   5 +
   .../qemumonitorjson-getcpu-host.json   |  45 ++
   tests/qemumonitorjsontest.c|  74 ++
   14 files changed, 465 insertions(+)
   create mode 100644 
  tests/qemumonitorjsondata/qemumonitorjson-getcpu-empty.data
   create mode 100644 
  tests/qemumonitorjsondata/qemumonitorjson-getcpu-empty.json
   create mode 100644 
  tests/qemumonitorjsondata/qemumonitorjson-getcpu-filtered.data
   create mode 100644 
  tests/qemumonitorjsondata/qemumonitorjson-getcpu-filtered.json
   create mode 100644 
  tests/qemumonitorjsondata/qemumonitorjson-getcpu-full.data
   create mode 100644 
  tests/qemumonitorjsondata/qemumonitorjson-getcpu-full.json
   create mode 100644 
  tests/qemumonitorjsondata/qemumonitorjson-getcpu-host.data
   create mode 100644 
  tests/qemumonitorjsondata/qemumonitorjson-getcpu-host.json
 
 ACK, though I believe the design of this monitor API is flawed
 because it requires you to re-launch QEMU with different accel
 args

Not really, this can be used in tcg too. It's just when we want to get
the data for host CPU, we need to enable kvm as tcg knows nothing
about that CPU. Which makes sense as kvm (the kernel module) influences
how the host CPU will look like.
   
   However, you need to have a CPU to be able to ask for his properties
   (which kinda makes sense too) and for that you also need a machine with
   type != none. Which makes sense too, as the CPU may differ depending on
   machine type (which, however, does not happen for host CPU).
  
  In addition to the -cpu host KVM initialization problem, this is an
  additional problem with the current interfaces provided by QEMU:
  
  1) libvirt needs to query data that depend on chosen machine-type and
 CPU model
  2) Some machine-type behavior is code and not introspectable data
 * Luckily most of the data we need in this case should/will be
   encoded in the compat_props tables.
 * In either case, we don't have an API to query for machine-type
   compat_props information yet.
  3) CPU model behavior will be modelled as CPU class behavior. Like
 on the machine-type case, some of the CPU-model-specific behavior may
 be modelled as code, and not introspectable data.
 * However, e may be able to eventually encode most or all of
   CPU-model-specific behavior simply as different per-CPU-class
   property defaults.
 * In either case, we don't have an API for QOM class introspection,
   yet.
  
  But there's something important in this case: the resulting CPUID data
  for a specific machine-type + CPU-model combination must be always the
  same, forever. This means libvirt may even use a static table, or cache
  this information indefinitely.
  
  (Note that I am not talking about -cpu host, here, but about all the
  other CPU models)
 
 Hmm, so if the CPU filtering can vary per every single individual
 machine type, then the approach Jiri started here, of invoking QEMU
 with machine type set to query the CPU after it was created, is
 definitely not something we can follow. It is just far too inefficient.

I believe there's some confusion here: we are trying to solve two
problems:

1) CPU feature filtering (checking which features are available in a
   given host)
2) CPU model probing (checking what exactly is going to be available
   when a given CPU model is used, in case nothing is filtered out)


Item (1) depends on: host CPU capabilities, host kernel capabilities,
QEMU capabilities, presence of some few QEMU command-line options (e.g.
kernel irqchip), but shouldn't depend on the machine-type. It depends on
/dev/kvm

[libvirt] [PATCH 1/2] qemu: Add capability flag for usb-storage

2013-07-25 Thread Fred A. Kemp 
From: Fred A. Kemp ano...@lavabit.com

Allow use of the usb-storage device only if the new capability flag
QEMU_CAPS_DEVICE_USB_STORAGE is set, which it is for qemu(-kvm)
versions = 0.12.1.2-rhel62-beta.
---
 src/qemu/qemu_capabilities.c |2 ++
 src/qemu/qemu_capabilities.h |1 +
 src/qemu/qemu_command.c  |6 +++---
 tests/qemuhelptest.c |   18 --
 tests/qemuxml2argvtest.c |3 ++-
 5 files changed, 20 insertions(+), 10 deletions(-)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 5dc3c9e..20deda0 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -234,6 +234,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
 
   vnc-share-policy, /* 150 */
   device-del-event,
+  usb-storage,
 );
 
 struct _virQEMUCaps {
@@ -1383,6 +1384,7 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] = {
 { pci-bridge, QEMU_CAPS_DEVICE_PCI_BRIDGE },
 { vfio-pci, QEMU_CAPS_DEVICE_VFIO_PCI },
 { scsi-generic, QEMU_CAPS_DEVICE_SCSI_GENERIC },
+{ usb-storage, QEMU_CAPS_DEVICE_USB_STORAGE },
 };
 
 static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsVirtioBlk[] = {
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index f5f685d..245b90b 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -190,6 +190,7 @@ enum virQEMUCapsFlags {
 QEMU_CAPS_MLOCK  = 149, /* -realtime mlock=on|off */
 QEMU_CAPS_VNC_SHARE_POLICY   = 150, /* set display sharing policy */
 QEMU_CAPS_DEVICE_DEL_EVENT   = 151, /* DEVICE_DELETED event */
+QEMU_CAPS_DEVICE_USB_STORAGE = 152, /* -device usb-storage */
 
 QEMU_CAPS_LAST,   /* this must always be the last item */
 };
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index d0aed7c..b658d22 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -7433,10 +7433,10 @@ qemuBuildCommandLine(virConnectPtr conn,
 bool withDeviceArg = false;
 bool deviceFlagMasked = false;
 
-/* Unless we have -device, then USB disks need special
-   handling */
+/* Unless we have `-device usb-storage`, then USB disks
+   need special handling */
 if ((disk-bus == VIR_DOMAIN_DISK_BUS_USB) 
-!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) {
+!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_USB_STORAGE)) {
 if (disk-device == VIR_DOMAIN_DISK_DEVICE_DISK) {
 virCommandAddArg(cmd, -usbdevice);
 virCommandAddArgFormat(cmd, disk:%s, disk-src);
diff --git a/tests/qemuhelptest.c b/tests/qemuhelptest.c
index d2fd794..cfb2c94 100644
--- a/tests/qemuhelptest.c
+++ b/tests/qemuhelptest.c
@@ -514,7 +514,8 @@ mymain(void)
 QEMU_CAPS_DEVICE_USB_SERIAL,
 QEMU_CAPS_DEVICE_USB_NET,
 QEMU_CAPS_DEVICE_PCI_BRIDGE,
-QEMU_CAPS_DEVICE_SCSI_GENERIC);
+QEMU_CAPS_DEVICE_SCSI_GENERIC,
+QEMU_CAPS_DEVICE_USB_STORAGE);
 DO_TEST(qemu-kvm-0.12.1.2-rhel61, 12001, 1, 0,
 QEMU_CAPS_VNC_COLON,
 QEMU_CAPS_NO_REBOOT,
@@ -653,7 +654,8 @@ mymain(void)
 QEMU_CAPS_DEVICE_QXL,
 QEMU_CAPS_DEVICE_VGA,
 QEMU_CAPS_DEVICE_CIRRUS_VGA,
-QEMU_CAPS_DEVICE_PCI_BRIDGE);
+QEMU_CAPS_DEVICE_PCI_BRIDGE,
+QEMU_CAPS_DEVICE_USB_STORAGE);
 DO_TEST(qemu-1.0, 100, 0, 0,
 QEMU_CAPS_VNC_COLON,
 QEMU_CAPS_NO_REBOOT,
@@ -736,7 +738,8 @@ mymain(void)
 QEMU_CAPS_DEVICE_USB_SERIAL,
 QEMU_CAPS_DEVICE_USB_NET,
 QEMU_CAPS_DEVICE_SCSI_GENERIC,
-QEMU_CAPS_DEVICE_SCSI_GENERIC_BOOTINDEX);
+QEMU_CAPS_DEVICE_SCSI_GENERIC_BOOTINDEX,
+QEMU_CAPS_DEVICE_USB_STORAGE);
 DO_TEST(qemu-1.1.0, 1001000, 0, 0,
 QEMU_CAPS_VNC_COLON,
 QEMU_CAPS_NO_REBOOT,
@@ -831,7 +834,8 @@ mymain(void)
 QEMU_CAPS_DEVICE_PCI_BRIDGE,
 QEMU_CAPS_DEVICE_SCSI_GENERIC,
 QEMU_CAPS_DEVICE_SCSI_GENERIC_BOOTINDEX,
-QEMU_CAPS_VNC_SHARE_POLICY);
+QEMU_CAPS_VNC_SHARE_POLICY,
+QEMU_CAPS_DEVICE_USB_STORAGE);
 DO_TEST(qemu-1.2.0, 1002000, 0, 0,
 QEMU_CAPS_VNC_COLON,
 QEMU_CAPS_NO_REBOOT,
@@ -938,7 +942,8 @@ mymain(void)
 QEMU_CAPS_DEVICE_PCI_BRIDGE,
 QEMU_CAPS_DEVICE_SCSI_GENERIC,
 QEMU_CAPS_DEVICE_SCSI_GENERIC_BOOTINDEX,
-QEMU_CAPS_VNC_SHARE_POLICY);
+QEMU_CAPS_VNC_SHARE_POLICY,
+QEMU_CAPS_DEVICE_USB_STORAGE);
 DO_TEST(qemu-kvm-1.2.0, 1002000, 1, 0,
 QEMU_CAPS_VNC_COLON,
 QEMU_CAPS_NO_REBOOT,
@@ -1050,7 +1055,8 @@ mymain(void)
 QEMU_CAPS_DEVICE_PCI_BRIDGE,
 QEMU_CAPS_DEVICE_SCSI_GENERIC,

[libvirt] [PATCH 0/2] Support settings the 'removable' flag for USB disks

2013-07-25 Thread Fred A. Kemp 
From: Fred A. Kemp ano...@lavabit.com

The commit message of patch #2 explains the purpose of this patch set.
A review would be greatly appreciated!

Note that I've only added the new capability for usb-storage.removable
to the qemu help tests of qemu(-kvm) version 1.2.0, since that's what I
had easily available to get the output of `-device usb-storage,?` from.
I hope that's not an issue, otherwise, is there a way to obtain these
outputs without having to hunt down and install all supported versions?

Previous submissions of this patch set to this list:
http://www.redhat.com/archives/libvir-list/2013-March/msg01051.html
http://www.redhat.com/archives/libvir-list/2013-May/msg02039.html

Cheers!

Fred A. Kemp (2):
  qemu: Add capability flag for usb-storage
  qemu: Support setting the 'removable' flag for USB disks

 docs/formatdomain.html.in  |8 +++--
 docs/schemas/domaincommon.rng  |8 +
 src/conf/domain_conf.c |   31 ++--
 src/conf/domain_conf.h |1 +
 src/qemu/qemu_capabilities.c   |   10 +++
 src/qemu/qemu_capabilities.h   |2 ++
 src/qemu/qemu_command.c|   23 +--
 tests/qemuhelpdata/qemu-1.2.0-device   |   11 +++
 tests/qemuhelpdata/qemu-kvm-1.2.0-device   |   11 +++
 tests/qemuhelptest.c   |   20 +
 .../qemuxml2argv-disk-usb-device-removable.args|8 +
 .../qemuxml2argv-disk-usb-device-removable.xml |   27 +
 tests/qemuxml2argvtest.c   |6 +++-
 13 files changed, 151 insertions(+), 15 deletions(-)
 create mode 100644 
tests/qemuxml2argvdata/qemuxml2argv-disk-usb-device-removable.args
 create mode 100644 
tests/qemuxml2argvdata/qemuxml2argv-disk-usb-device-removable.xml

-- 
1.7.10.4


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

Re: [libvirt] [PATCH] virdbustest: Don't pass number of arguments as long long

2013-07-25 Thread Eric Blake 
On 07/25/2013 03:27 AM, Daniel P. Berrange wrote:

 This fix looks correct, but it's annoying that we have to cast the 'a'
 length argument in every caller.  I'm wondering if a better fix would be
 to virDBusMessageEncode to take an 'int' instead of a 'size_t' arg for
 a length; even though that is technically an arbitrary limitation on
 64-bit platforms, I seriously doubt anyone plans to use dBus to send
 more than 2G of objects.
 
 The code requires an int already
 
 narray = va_arg(args, int);

Yay - the underlying implementation is correct.

 
 so both the (long long) and (size_t) casts are bogus. We can just
 remove the cast completely.

Serves me right for not checking whether the va_arg was using the right
type in the first place.  Guido's second version that drops the cast
completely is correct.

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



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

[libvirt] [PATCH 2/2] qemu: Support setting the 'removable' flag for USB disks

2013-07-25 Thread Fred A. Kemp 
From: Fred A. Kemp ano...@lavabit.com

Add an attribute named 'removable' to the 'target' element of disks,
which controls the removable flag. For instance, on a Linux guest it
controls the value of /sys/block/$dev/removable. This option is only
valid for USB disks (i.e. bus='usb'), and its default value is 'off',
which is the same behaviour as before.

To achieve this, 'removable=on' (or 'off') is appended to the '-device
usb-storage' parameter sent to qemu when adding a USB disk via
'-disk'. A capability flag QEMU_CAPS_USB_STORAGE_REMOVABLE was added
to keep track if this option is supported by the qemu version used.

Bug: https://bugzilla.redhat.com/show_bug.cgi?id=922495
---
 docs/formatdomain.html.in  |8 +++--
 docs/schemas/domaincommon.rng  |8 +
 src/conf/domain_conf.c |   31 ++--
 src/conf/domain_conf.h |1 +
 src/qemu/qemu_capabilities.c   |8 +
 src/qemu/qemu_capabilities.h   |1 +
 src/qemu/qemu_command.c|   17 +++
 tests/qemuhelpdata/qemu-1.2.0-device   |   11 +++
 tests/qemuhelpdata/qemu-kvm-1.2.0-device   |   11 +++
 tests/qemuhelptest.c   |6 ++--
 .../qemuxml2argv-disk-usb-device-removable.args|8 +
 .../qemuxml2argv-disk-usb-device-removable.xml |   27 +
 tests/qemuxml2argvtest.c   |3 ++
 13 files changed, 133 insertions(+), 7 deletions(-)
 create mode 100644 
tests/qemuxml2argvdata/qemuxml2argv-disk-usb-device-removable.args
 create mode 100644 
tests/qemuxml2argvdata/qemuxml2argv-disk-usb-device-removable.xml

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 7601aaa..259ecd0 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -1667,9 +1667,13 @@
 removable disks (i.e. CDROM or Floppy disk), the value can be either
 open or closed, defaults to closed. NB, the value of
 codetray/code could be updated while the domain is running.
-span class=sinceSince 0.0.3; codebus/code attribute since 
0.4.3;
+The optional attribute coderemovable/code sets the
+removable flag for USB disks, and its value can be either on
+or off, defaulting to off. span class=sinceSince
+0.0.3; codebus/code attribute since 0.4.3;
 codetray/code attribute since 0.9.11; usb attribute value since
-after 0.4.4; sata attribute value since 0.9.7/span
+after 0.4.4; sata attribute value since 0.9.7; removable attribute
+value since X.Y.Z/span
   /dd
   dtcodeiotune/code/dt
   ddThe optional codeiotune/code element provides the
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 745b959..65a52d0 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -1278,6 +1278,14 @@
   /choice
 /attribute
   /optional
+  optional
+attribute name=removable
+  choice
+valueon/value
+valueoff/value
+  /choice
+/attribute
+  /optional
 /element
   /define
   define name=geometry
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 10cb7f6..b298ac5 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -4747,6 +4747,7 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
 char *authUUID = NULL;
 char *usageType = NULL;
 char *tray = NULL;
+char *removable = NULL;
 char *logical_block_size = NULL;
 char *physical_block_size = NULL;
 char *wwn = NULL;
@@ -4903,6 +4904,7 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
 target = virXMLPropString(cur, dev);
 bus = virXMLPropString(cur, bus);
 tray = virXMLPropString(cur, tray);
+removable = virXMLPropString(cur, removable);
 
 /* HACK: Work around for compat with Xen
  * driver in previous libvirt releases */
@@ -5336,6 +5338,24 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
 def-tray_status = VIR_DOMAIN_DISK_TRAY_CLOSED;
 }
 
+if (removable) {
+if ((def-removable = virDomainFeatureStateTypeFromString(removable)) 
 0) {
+virReportError(VIR_ERR_XML_ERROR,
+   _(unknown disk removable status '%s'), removable);
+goto error;
+}
+
+if (def-bus != VIR_DOMAIN_DISK_BUS_USB) {
+virReportError(VIR_ERR_XML_ERROR, %s,
+   _(removable is only valid for usb disks));
+goto error;
+}
+} else {
+if (def-bus == VIR_DOMAIN_DISK_BUS_USB) {
+def-removable = VIR_DOMAIN_FEATURE_STATE_DEFAULT;
+}
+}
+
 if (def-device == VIR_DOMAIN_DISK_DEVICE_FLOPPY

Re: [libvirt] [Qemu-devel] [PATCH 4/7] qemu: Add monitor APIs to fetch CPUID data from QEMU

2013-07-25 Thread Eduardo Habkost 
On Thu, Jul 25, 2013 at 11:14:16AM +0200, Andreas Färber wrote:
 Am 24.07.2013 20:25, schrieb Eduardo Habkost:
  In addition to the -cpu host KVM initialization problem, this is an
  additional problem with the current interfaces provided by QEMU:
  
  1) libvirt needs to query data that depend on chosen machine-type and
 CPU model
  2) Some machine-type behavior is code and not introspectable data
 * Luckily most of the data we need in this case should/will be
   encoded in the compat_props tables.
 * In either case, we don't have an API to query for machine-type
   compat_props information yet.
 
 ... and I don't think we should add such a thing. It is an internal
 implementation detail, whose results should be inspected instead.
 
  3) CPU model behavior will be modelled as CPU class behavior. Like
 on the machine-type case, some of the CPU-model-specific behavior may
 be modelled as code, and not introspectable data.
 * However, e may be able to eventually encode most or all of
   CPU-model-specific behavior simply as different per-CPU-class
   property defaults.
 * In either case, we don't have an API for QOM class introspection,
   yet.
 
 If I understood Anthony correctly on the previous call then that is by
 design. We have a query-cpu-definitions QMP API to obtain CPU models
 though. And qom-list-types to discover QOM types. The CPU can then be
 instantiated via -cpu (the type via -device/-object on other targets),
 inspected with qom-list/qom-get API and modified with qom-set.
 
 The problem with the latter is that devices/CPUs get realized in code
 rather than shortly before emulation/virtualization starts - that's what
 my recursive QOM realization series prepared to address, but Paolo
 veto'ed that and hasn't provided sufficient feedback on what exactly his
 concerns are founded on and what he proposes instead. In particular:
 Would walking the qdev bus tree instead of the QOM composition tree
 address the concerns?
 
 Depending on what libvirt is actually trying to do, the above combined
 with Igor's feature properties and -S might do the job. For x86 the CPUs
 are easily locatable in the QOM tree via ICC.

I still don't see how the above would solve the bigger problem: libvirt
needs a way to find out how exactly -machine foo-1.0 -cpu bar looks
different from -machine foo-1.1 -cpu bar, but don't want to execute
QEMU multiple times just to find that out.

-- 
Eduardo

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

Re: [libvirt] [Qemu-devel] [PATCH 4/7] qemu: Add monitor APIs to fetch CPUID data from QEMU

2013-07-25 Thread Andreas Färber 
Am 25.07.2013 16:00, schrieb Eduardo Habkost:
 libvirt
 needs a way to find out how exactly -machine foo-1.0 -cpu bar looks
 different from -machine foo-1.1 -cpu bar,

Why? (What's the actual use case?)

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

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

Re: [libvirt] [Qemu-devel] [PATCH 4/7] qemu: Add monitor APIs to fetch CPUID data from QEMU

2013-07-25 Thread Daniel P. Berrange 
On Thu, Jul 25, 2013 at 04:09:18PM +0200, Andreas Färber wrote:
 Am 25.07.2013 16:00, schrieb Eduardo Habkost:
  libvirt
  needs a way to find out how exactly -machine foo-1.0 -cpu bar looks
  different from -machine foo-1.1 -cpu bar,
 
 Why? (What's the actual use case?)

It already takes a long time to just probe each QEMU binary, without
expanding that to probe each binary once for each machine type they
include. The idea of 'qemu -M none' was that we'd have a machine type
which did not do any hardware set, to query any aspect of the QEMU
binary capabilities.  If we have to also invoke qemu again with every
other machine type that is a failed design IMHO. It should not be beyond
the realm of possibility to make 'qemu -M none' provide information
about the CPU features for machines/cpus without needing to actually
creating instances of those machines.


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

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

Re: [libvirt] [PATCH 1/4] qemu: refactor qemuDomainCheckDiskPresence for only disk presence check

2013-07-25 Thread Martin Kletzander 
On 07/18/2013 01:32 PM, Guannan Ren wrote:
 Refactor this function to make it focus on disk presence checking,
 including diskchain checking, and not only for CDROM and Floppy.
 This change is good for the following patches.
 ---
  src/qemu/qemu_domain.c | 99 
 ++
  1 file changed, 59 insertions(+), 40 deletions(-)
 
 diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
 index 06efe14..1fa1a5f 100644
 --- a/src/qemu/qemu_domain.c
 +++ b/src/qemu/qemu_domain.c
 @@ -1989,6 +1989,61 @@ cleanup:
  virObjectUnref(cfg);
  }
  
 +static int
 +qemuDomainCheckDiskStartupPolicy(virQEMUDriverPtr driver,
 + virDomainObjPtr vm,
 + virDomainDiskDefPtr disk,
 + int startupPolicy,

No need for this parameter as you have it in the disk already.

ACK with that fixed, clean code movement.

Martin

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

Re: [libvirt] [PATCH 2/4] qemu: report error if disk backing files doesn't exist

2013-07-25 Thread Martin Kletzander 
On 07/18/2013 01:32 PM, Guannan Ren wrote:

s/doesn't/don't/ in $SUBJ

 Adding virFileAccessibleAs() to check if the backing file described in
 disk meta exist in real path. If not, report error. The uid and gid
 arguments don't take effect on F_OK mode for access, so use gid and gid
 of current process.
 

This patch doesn't break anything new, but thanks to the
getuid()/getgid(), it leaves the previous problem in the code.  Even
though F_OK doesn't need uid/gid to check whether the file exists, root
may not have permissions for upper directories on NFS storage for
example, so ...

 ---
  src/util/virstoragefile.c | 23 +++
  tests/virstoragetest.c| 16 
  2 files changed, 23 insertions(+), 16 deletions(-)
 
 diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
 index cb6df91..b678eb8 100644
 --- a/src/util/virstoragefile.c
 +++ b/src/util/virstoragefile.c
 @@ -572,6 +572,13 @@ virFindBackingFile(const char *start, bool start_is_dir, 
 const char *path,
  goto cleanup;
  }
  
 +if (virFileAccessibleAs(combined, F_OK, getuid(), getgid())  0) {
 +virReportSystemError(errno,
 + _(Backing file '%s' does not exist),
 + combined);
 +goto cleanup;
 +}
 +
  if (!(*canonical = canonicalize_file_name(combined))) {
  virReportSystemError(errno,
   _(Can't canonicalize path '%s'), path);

... this hunk does make the code report better errors, but in the future
it should canonicalize the filename using root/qemu/domain users.

ACK to this hunk, with the error reworded (e.g. Cannot access backing
file %s) and, of course, commit message changed appropriately, but ...

 @@ -857,14 +864,10 @@ virStorageFileGetMetadataInternal(const char *path,
 !!directory, backing,
 meta-directory,
 meta-backingStore)  0) {
 -/* the backing file is (currently) unavailable, treat 
 this
 - * file as standalone:
 - * backingStoreRaw is kept to mark broken image chains */
 -meta-backingStoreIsFile = false;
 -backingFormat = VIR_STORAGE_FILE_NONE;
 -VIR_WARN(Backing file '%s' of image '%s' is missing.,
 - meta-backingStoreRaw, path);
 -
 +VIR_FREE(backing);
 +VIR_ERROR(_(Backing file '%s' of image '%s' is 
 missing.),
 +  meta-backingStoreRaw, path);
 +goto cleanup;

To fix a pre-existing error, we should (instead of this change) just add
virResetLastError() here as the error is used somewhere else in the code
and should be kept in virFindBackingFile().  Having it in the logs seems
OK to me.

  }
  }
  VIR_FREE(backing);

... NACK to the rest.  As written in the comment you are removing,
backingStoreRaw is kept to mark broken chains.  And that is the thing we
should use to check whether the backing chain is broken, not throw away
all the data we've got.

 @@ -1047,6 +1050,10 @@ virStorageFileGetMetadataRecurse(const char *path, 
 const char *directory,
  uid, gid,
  allow_probe,
  cycle);
 +if (!ret-backingMeta) {
 +virStorageFileFreeMetadata(ret);
 +ret = NULL;
 +}
  }
  
  return ret;

That means you can drop this hunk too, the tests as well, and the
detection of broken chains could be added in a separate function (just a
cycle checking for that) and called when we want to know whether the
chain is broken.

Martin

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

Re: [libvirt] [PATCH 3/4] qemu: check presence of each disk in chain

2013-07-25 Thread Martin Kletzander 
On 07/18/2013 01:32 PM, Guannan Ren wrote:
 For disk with startupPolicy support, such as cdrom and floppy
 when its chain is broken, the startup policy  will apply,
 otherwise, report error on chain issue.
 
 Force to collect diskchain metadata when qemu process start
 everytime.
 ---
  src/qemu/qemu_domain.c  | 19 ++-
  src/qemu/qemu_process.c |  7 ---
  2 files changed, 10 insertions(+), 16 deletions(-)
 
 diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
 index 1fa1a5f..c4adbaa 100644
 --- a/src/qemu/qemu_domain.c
 +++ b/src/qemu/qemu_domain.c
 @@ -2057,20 +2057,21 @@ qemuDomainCheckDiskPresence(virQEMUDriverPtr driver,
  for (i = 0; i  vm-def-ndisks; i++) {
  disk = vm-def-disks[i];
  
 -if (!disk-startupPolicy || !disk-src)
 +if (!disk-src)
  continue;
  
 -if (virFileAccessibleAs(disk-src, F_OK,
 -cfg-user,
 -cfg-group) = 0) {
 -/* disk accessible */
 +if (qemuDomainDetermineDiskChain(driver,
 + disk, true) = 0)
  continue;

As mentioned in [PATCH 2/4], this should be changed to check whether the
backing chain is broken or not.

 +
 +if (disk-startupPolicy) {
 +if (qemuDomainCheckDiskStartupPolicy(driver, vm, disk,
 + disk-startupPolicy,
 + cold_boot) = 0)
 +continue;
  }
  
 -if (qemuDomainCheckDiskStartupPolicy(driver, vm, disk,
 - disk-startupPolicy,
 - cold_boot)  0)
 -goto cleanup;
 +goto cleanup;
  }
  
  ret = 0;
 diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
 index 3d5e8f6..f495267 100644
 --- a/src/qemu/qemu_process.c
 +++ b/src/qemu/qemu_process.c
 @@ -3562,17 +3562,10 @@ int qemuProcessStart(virConnectPtr conn,
  if (qemuAssignDeviceAliases(vm-def, priv-qemuCaps)  0)
  goto cleanup;
  
 -VIR_DEBUG(Checking for CDROM and floppy presence);

Changed debug message could be used in qemuDomainCheckDiskPresence(),
even for every disk, so it is easier to find why we were browsing
through files on the host.

Other than that the patch looks fine, we'll see how it looks after 2/4
is fixed.

Martin

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

Re: [libvirt] [PATCH 4/7] qemu: Add monitor APIs to fetch CPUID data from QEMU

2013-07-25 Thread Daniel P. Berrange 
On Thu, Jul 25, 2013 at 10:15:56AM -0300, Eduardo Habkost wrote:
 On Thu, Jul 25, 2013 at 10:45:10AM +0100, Daniel P. Berrange wrote:
  On Wed, Jul 24, 2013 at 03:25:19PM -0300, Eduardo Habkost wrote:
   In addition to the -cpu host KVM initialization problem, this is an
   additional problem with the current interfaces provided by QEMU:
   
   1) libvirt needs to query data that depend on chosen machine-type and
  CPU model
   2) Some machine-type behavior is code and not introspectable data
  * Luckily most of the data we need in this case should/will be
encoded in the compat_props tables.
  * In either case, we don't have an API to query for machine-type
compat_props information yet.
   3) CPU model behavior will be modelled as CPU class behavior. Like
  on the machine-type case, some of the CPU-model-specific behavior may
  be modelled as code, and not introspectable data.
  * However, e may be able to eventually encode most or all of
CPU-model-specific behavior simply as different per-CPU-class
property defaults.
  * In either case, we don't have an API for QOM class introspection,
yet.
   
   But there's something important in this case: the resulting CPUID data
   for a specific machine-type + CPU-model combination must be always the
   same, forever. This means libvirt may even use a static table, or cache
   this information indefinitely.
   
   (Note that I am not talking about -cpu host, here, but about all the
   other CPU models)
  
  Hmm, so if the CPU filtering can vary per every single individual
  machine type, then the approach Jiri started here, of invoking QEMU
  with machine type set to query the CPU after it was created, is
  definitely not something we can follow. It is just far too inefficient.
 
 I believe there's some confusion here: we are trying to solve two
 problems:
 
 1) CPU feature filtering (checking which features are available in a
given host)
 2) CPU model probing (checking what exactly is going to be available
when a given CPU model is used, in case nothing is filtered out)

Yep, what Jiri proposed in the original libvirt thread was just a
solution to 1). In seeing that though, I was concerned about how it
scales up once we have to deal with 2) as well, which I believe is
planned future work.

 Item (1) depends on: host CPU capabilities, host kernel capabilities,
 QEMU capabilities, presence of some few QEMU command-line options (e.g.
 kernel irqchip), but shouldn't depend on the machine-type. It depends on
 /dev/kvm being open.
 
 Item (2) depends on the machine-type, but is static and must never
 change on future QEMU versions (if it changes, it is a QEMU bug). It
 doesn't depend on opening /dev/kvm.


 Item (1) can be solved if libvirt does the work itself, by opening
 /dev/kvm and checking for GET_SUPPORTED_CPUID and checking for QEMU
 options/capabilities (as long as we document that very carefully). But
 adding a more specific QMP command that won't require accel=kvm to work
 may be simpler and better for everybody.
 
 Item (2) may be solved today using a static table and/or caching (so
 libvirt just need to query this information once in a lifetime). It can
 also be solved partially (without machine-type support) in theory if
 QEMU let libvirt repeatedly create and destroy CPU objects just to query
 the resulting feature properties.

We really don't want to have static tables, since that creates pain
in the case where distro vendors create their own custom machine types
or CPU models. It would mean libvirt had to record info not only about
upstream QEMU, but about every vendor's QEMU builds. Probing the actual
binary is the only sensible way here.

 ...but both problems could be solved very easily using current QEMU
 interfaces, if libvirt simply executed the QEMU binary more than once.
 Is must not run QEMU more than once a hard requirement? Perfect is the
 enemy of good.  :)

Yes, it is a hard requirement.

  I understand that the QEMU code isn't currently structured in a way
  that lets it easily expose information that varies per machine type,
  but I don't think we need to solve the entire problem space in a
  perfectly generic fashion here. Perfect is the enemy of good.
 
 Right. Also, the more important item (item 1) is not affected by
 machine-types. Host features change every time you run on a new
 host/kernel, so probing it precisely is very useful, to detect problems
 earlier (not just at the last moment before starting a VM).
 
 On the other hand, per-machine-type CPU model changes are more rare, and
 libvirt can still detect unexpected results immediately before the VM is
 started. (I don't know what libvirt would do in case it detects it,
 though. Abort? Log a warning?)

We don't want to be running QEMU multiple times during the startup
process for a VM, because that adds delays to the startup process.
It might not sound like much but adding a few 100ms to probe CPUs
by

[libvirt] [PATCH 0/2] Rework client connection handling

2013-07-25 Thread Michal Privoznik 
There are few cases where users don't want to raise 'max_client', but are doing
many concurrent connection and don't want them to fail too. However, we are
currently accept()-ing the incoming request even though we have reached the
limit. If that's the case, error is reported and connection is thrown away.
Gross. What about leaving the requests we know we can't handle yet in the
listen() queue and accepting only those we know we can handle?

For more info see:

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

Michal Privoznik (2):
  RPC: Don't accept client if it would overcommit max_clients
  Introduce max_queued_clients

 daemon/libvirtd-config.c  |  1 +
 daemon/libvirtd-config.h  |  1 +
 daemon/libvirtd.aug   |  1 +
 daemon/libvirtd.c |  4 
 daemon/libvirtd.conf  |  6 ++
 src/locking/lock_daemon.c |  2 +-
 src/lxc/lxc_controller.c  |  1 +
 src/rpc/virnetserver.c| 40 
 src/rpc/virnetserverservice.c | 15 +--
 src/rpc/virnetserverservice.h |  6 ++
 10 files changed, 74 insertions(+), 3 deletions(-)

-- 
1.8.1.5

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

[libvirt] [PATCH 2/2] Introduce max_queued_clients

2013-07-25 Thread Michal Privoznik 
This configuration knob lets user to set the length of queue of
connection requests waiting to be accept()-ed by the daemon. IOW, it
just controls the @backlog passed to listen:

  int listen(int sockfd, int backlog);
---
 daemon/libvirtd-config.c  | 1 +
 daemon/libvirtd-config.h  | 1 +
 daemon/libvirtd.aug   | 1 +
 daemon/libvirtd.c | 4 
 daemon/libvirtd.conf  | 6 ++
 src/locking/lock_daemon.c | 2 +-
 src/lxc/lxc_controller.c  | 1 +
 src/rpc/virnetserverservice.c | 6 --
 src/rpc/virnetserverservice.h | 2 ++
 9 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/daemon/libvirtd-config.c b/daemon/libvirtd-config.c
index 107a9cf..c816fda 100644
--- a/daemon/libvirtd-config.c
+++ b/daemon/libvirtd-config.c
@@ -414,6 +414,7 @@ daemonConfigLoadOptions(struct daemonConfig *data,
 GET_CONF_INT(conf, filename, min_workers);
 GET_CONF_INT(conf, filename, max_workers);
 GET_CONF_INT(conf, filename, max_clients);
+GET_CONF_INT(conf, filename, max_queued_clients);
 
 GET_CONF_INT(conf, filename, prio_workers);
 
diff --git a/daemon/libvirtd-config.h b/daemon/libvirtd-config.h
index 973e0ea..a24d5d2 100644
--- a/daemon/libvirtd-config.h
+++ b/daemon/libvirtd-config.h
@@ -63,6 +63,7 @@ struct daemonConfig {
 int min_workers;
 int max_workers;
 int max_clients;
+int max_queued_clients;
 
 int prio_workers;
 
diff --git a/daemon/libvirtd.aug b/daemon/libvirtd.aug
index 7c56a41..70fce5c 100644
--- a/daemon/libvirtd.aug
+++ b/daemon/libvirtd.aug
@@ -56,6 +56,7 @@ module Libvirtd =
let processing_entry = int_entry min_workers
 | int_entry max_workers
 | int_entry max_clients
+| int_entry max_queued_clients
 | int_entry max_requests
 | int_entry max_client_requests
 | int_entry prio_workers
diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
index 9f7fd8a..402b494 100644
--- a/daemon/libvirtd.c
+++ b/daemon/libvirtd.c
@@ -485,6 +485,7 @@ static int daemonSetupNetworking(virNetServerPtr srv,
NULL,
 #endif
false,
+   config-max_queued_clients,
config-max_client_requests)))
 goto error;
 if (sock_path_ro) {
@@ -497,6 +498,7 @@ static int daemonSetupNetworking(virNetServerPtr srv,
  NULL,
 #endif
  true,
+ config-max_queued_clients,
  config-max_client_requests)))
 goto error;
 }
@@ -522,6 +524,7 @@ static int daemonSetupNetworking(virNetServerPtr srv,
  NULL,
 #endif
  false,
+ 
config-max_queued_clients,
  
config-max_client_requests)))
 goto error;
 
@@ -562,6 +565,7 @@ static int daemonSetupNetworking(virNetServerPtr srv,
 config-auth_tls,
 ctxt,
 false,
+config-max_queued_clients,
 config-max_client_requests))) {
 virObjectUnref(ctxt);
 goto error;
diff --git a/daemon/libvirtd.conf b/daemon/libvirtd.conf
index af4493e..5353927 100644
--- a/daemon/libvirtd.conf
+++ b/daemon/libvirtd.conf
@@ -257,6 +257,12 @@
 # over all sockets combined.
 #max_clients = 20
 
+# The maximum length of queue of connections waiting to be
+# accepted by the daemon. Note, that some protocols supporting
+# retransmission may obey this so that a later reattempt at
+# connection succeeds.
+#max_queued_clients = 1000
+
 
 # The minimum limit sets the number of workers to start up
 # initially. If the number of active clients exceeds this,
diff --git a/src/locking/lock_daemon.c b/src/locking/lock_daemon.c
index c4c1727..c45f45c 100644
--- a/src/locking/lock_daemon.c
+++ b/src/locking/lock_daemon.c
@@ -671,7 +671,7 @@ virLockDaemonSetupNetworkingNative(virNetServerPtr srv, 
const char *sock_path)
 #if WITH_GNUTLS
NULL,
 #endif
-   false, 1)))
+   false, 0, 1)))
 return -1;
 
 if (virNetServerAddService(srv, svc, NULL)  0) {
diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c
index 124ab19..ed73ab0 100644
--- a/src/lxc/lxc_controller.c
+++ b/src/lxc/lxc_controller.c
@@ -745,6 +745,7 @@ static int

Re: [libvirt] [PATCH] virdbustest: Don't pass number of arguments as long long

2013-07-25 Thread Guido Günther 
On Thu, Jul 25, 2013 at 07:37:48AM -0600, Eric Blake wrote:
 On 07/25/2013 03:27 AM, Daniel P. Berrange wrote:
 
  This fix looks correct, but it's annoying that we have to cast the 'a'
  length argument in every caller.  I'm wondering if a better fix would be
  to virDBusMessageEncode to take an 'int' instead of a 'size_t' arg for
  a length; even though that is technically an arbitrary limitation on
  64-bit platforms, I seriously doubt anyone plans to use dBus to send
  more than 2G of objects.
  
  The code requires an int already
  
  narray = va_arg(args, int);
 
 Yay - the underlying implementation is correct.
 
  
  so both the (long long) and (size_t) casts are bogus. We can just
  remove the cast completely.
 
 Serves me right for not checking whether the va_arg was using the right
 type in the first place.  Guido's second version that drops the cast
 completely is correct.

Same here. I assumed size_t but should have double checked.
Pushed now, thanks,
 -- Guido

 
 -- 
 Eric Blake   eblake redhat com+1-919-301-3266
 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] Introduce max_queued_clients

2013-07-25 Thread Daniel P. Berrange 
On Thu, Jul 25, 2013 at 04:23:33PM +0200, Michal Privoznik wrote:
 This configuration knob lets user to set the length of queue of
 connection requests waiting to be accept()-ed by the daemon. IOW, it
 just controls the @backlog passed to listen:
 
   int listen(int sockfd, int backlog);
 ---
  daemon/libvirtd-config.c  | 1 +
  daemon/libvirtd-config.h  | 1 +
  daemon/libvirtd.aug   | 1 +
  daemon/libvirtd.c | 4 
  daemon/libvirtd.conf  | 6 ++
  src/locking/lock_daemon.c | 2 +-
  src/lxc/lxc_controller.c  | 1 +
  src/rpc/virnetserverservice.c | 6 --
  src/rpc/virnetserverservice.h | 2 ++
  9 files changed, 21 insertions(+), 3 deletions(-)
 
 diff --git a/daemon/libvirtd-config.c b/daemon/libvirtd-config.c
 index 107a9cf..c816fda 100644
 --- a/daemon/libvirtd-config.c
 +++ b/daemon/libvirtd-config.c
 @@ -414,6 +414,7 @@ daemonConfigLoadOptions(struct daemonConfig *data,
  GET_CONF_INT(conf, filename, min_workers);
  GET_CONF_INT(conf, filename, max_workers);
  GET_CONF_INT(conf, filename, max_clients);
 +GET_CONF_INT(conf, filename, max_queued_clients);
  
  GET_CONF_INT(conf, filename, prio_workers);
  
 diff --git a/daemon/libvirtd-config.h b/daemon/libvirtd-config.h
 index 973e0ea..a24d5d2 100644
 --- a/daemon/libvirtd-config.h
 +++ b/daemon/libvirtd-config.h
 @@ -63,6 +63,7 @@ struct daemonConfig {
  int min_workers;
  int max_workers;
  int max_clients;
 +int max_queued_clients;
  
  int prio_workers;
  
 diff --git a/daemon/libvirtd.aug b/daemon/libvirtd.aug
 index 7c56a41..70fce5c 100644
 --- a/daemon/libvirtd.aug
 +++ b/daemon/libvirtd.aug
 @@ -56,6 +56,7 @@ module Libvirtd =
 let processing_entry = int_entry min_workers
  | int_entry max_workers
  | int_entry max_clients
 +| int_entry max_queued_clients
  | int_entry max_requests
  | int_entry max_client_requests
  | int_entry prio_workers
 diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
 index 9f7fd8a..402b494 100644
 --- a/daemon/libvirtd.c
 +++ b/daemon/libvirtd.c
 @@ -485,6 +485,7 @@ static int daemonSetupNetworking(virNetServerPtr srv,
 NULL,
  #endif
 false,
 +   config-max_queued_clients,
 config-max_client_requests)))
  goto error;
  if (sock_path_ro) {
 @@ -497,6 +498,7 @@ static int daemonSetupNetworking(virNetServerPtr srv,
   NULL,
  #endif
   true,
 + config-max_queued_clients,
   
 config-max_client_requests)))
  goto error;
  }
 @@ -522,6 +524,7 @@ static int daemonSetupNetworking(virNetServerPtr srv,
   NULL,
  #endif
   false,
 + 
 config-max_queued_clients,
   
 config-max_client_requests)))
  goto error;
  
 @@ -562,6 +565,7 @@ static int daemonSetupNetworking(virNetServerPtr srv,
  config-auth_tls,
  ctxt,
  false,
 +config-max_queued_clients,
  config-max_client_requests))) {
  virObjectUnref(ctxt);
  goto error;
 diff --git a/daemon/libvirtd.conf b/daemon/libvirtd.conf
 index af4493e..5353927 100644
 --- a/daemon/libvirtd.conf
 +++ b/daemon/libvirtd.conf
 @@ -257,6 +257,12 @@
  # over all sockets combined.
  #max_clients = 20
  
 +# The maximum length of queue of connections waiting to be
 +# accepted by the daemon. Note, that some protocols supporting
 +# retransmission may obey this so that a later reattempt at
 +# connection succeeds.
 +#max_queued_clients = 1000
 +
  
  # The minimum limit sets the number of workers to start up
  # initially. If the number of active clients exceeds this,
 diff --git a/src/locking/lock_daemon.c b/src/locking/lock_daemon.c
 index c4c1727..c45f45c 100644
 --- a/src/locking/lock_daemon.c
 +++ b/src/locking/lock_daemon.c
 @@ -671,7 +671,7 @@ virLockDaemonSetupNetworkingNative(virNetServerPtr srv, 
 const char *sock_path)
  #if WITH_GNUTLS
 NULL,
  #endif
 -   false, 1)))
 +   false, 0, 1)))
  return -1;
  
  if (virNetServerAddService(srv, svc, NULL)  0) {

I think

[libvirt] [PATCH 1/2] RPC: Don't accept client if it would overcommit max_clients

2013-07-25 Thread Michal Privoznik 
Currently, even if max_client limit is hit, we accept() incoming
connection request, but close it immediately. This has disadvantage of
not using listen() queue. We should accept() only those clients we
know we can serve and let all other wait in the (limited) queue.
---
 src/rpc/virnetserver.c| 40 
 src/rpc/virnetserverservice.c |  9 +
 src/rpc/virnetserverservice.h |  4 
 3 files changed, 53 insertions(+)

diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
index cb770c3..7ea1707 100644
--- a/src/rpc/virnetserver.c
+++ b/src/rpc/virnetserver.c
@@ -315,6 +315,34 @@ static int 
virNetServerDispatchNewClient(virNetServerServicePtr svc,
 }
 
 
+static int
+virNetServerDispatchCheck(virNetServerServicePtr svc,
+  virNetSocketPtr socket,
+  void *opaque)
+{
+virNetServerPtr srv = opaque;
+int ret = -1;
+
+VIR_DEBUG(svc=%p socket=%p opaque=%p, svc, socket, opaque);
+
+if (srv-nclients = srv-nclients_max) {
+VIR_DEBUG(Not accepting client now due to max_clients setting (%zu),
+  srv-nclients_max);
+
+/* We need to temporarily disable the server services in order to
+ * prevent event loop calling us over and over again. */
+
+virNetServerUpdateServices(srv, false);
+
+goto cleanup;
+}
+
+ret = 0;
+cleanup:
+return ret;
+}
+
+
 static void
 virNetServerFatalSignal(int sig, siginfo_t *siginfo ATTRIBUTE_UNUSED,
 void *context ATTRIBUTE_UNUSED)
@@ -981,6 +1009,7 @@ int virNetServerAddService(virNetServerPtr srv,
 
 virNetServerServiceSetDispatcher(svc,
  virNetServerDispatchNewClient,
+ virNetServerDispatchCheck,
  srv);
 
 virObjectUnlock(srv);
@@ -1110,6 +1139,8 @@ void virNetServerRun(virNetServerPtr srv)
 virNetServerClientClose(srv-clients[i]);
 if (virNetServerClientIsClosed(srv-clients[i])) {
 virNetServerClientPtr client = srv-clients[i];
+bool update_services;
+
 if (srv-nclients  1) {
 memmove(srv-clients + i,
 srv-clients + i + 1,
@@ -1120,7 +1151,16 @@ void virNetServerRun(virNetServerPtr srv)
 srv-nclients = 0;
 }
 
+/* Enable services if we can accept a new client.
+ * The new client can be accepted if we are at the limit. */
+update_services = srv-nclients == srv-nclients_max - 1;
+
 virObjectUnlock(srv);
+if (update_services) {
+/* Now it makes sense to accept() a new client. */
+VIR_DEBUG(Re-enabling services);
+virNetServerUpdateServices(srv, true);
+}
 virObjectUnref(client);
 virObjectLock(srv);
 
diff --git a/src/rpc/virnetserverservice.c b/src/rpc/virnetserverservice.c
index 632f03d..a47c8f8 100644
--- a/src/rpc/virnetserverservice.c
+++ b/src/rpc/virnetserverservice.c
@@ -46,6 +46,7 @@ struct _virNetServerService {
 #endif
 
 virNetServerServiceDispatchFunc dispatchFunc;
+virNetServerServiceDispatchCheckFunc dispatchCheckFunc;
 void *dispatchOpaque;
 };
 
@@ -74,6 +75,12 @@ static void virNetServerServiceAccept(virNetSocketPtr sock,
 virNetServerServicePtr svc = opaque;
 virNetSocketPtr clientsock = NULL;
 
+if (svc-dispatchCheckFunc 
+svc-dispatchCheckFunc(svc, sock, svc-dispatchOpaque)  0) {
+/* Accept declined */
+goto cleanup;
+}
+
 if (virNetSocketAccept(sock, clientsock)  0)
 goto cleanup;
 
@@ -419,9 +426,11 @@ virNetTLSContextPtr 
virNetServerServiceGetTLSContext(virNetServerServicePtr svc)
 
 void virNetServerServiceSetDispatcher(virNetServerServicePtr svc,
   virNetServerServiceDispatchFunc func,
+  virNetServerServiceDispatchCheckFunc 
checkFunc,
   void *opaque)
 {
 svc-dispatchFunc = func;
+svc-dispatchCheckFunc = checkFunc;
 svc-dispatchOpaque = opaque;
 }
 
diff --git a/src/rpc/virnetserverservice.h b/src/rpc/virnetserverservice.h
index 1ece503..e9e5389 100644
--- a/src/rpc/virnetserverservice.h
+++ b/src/rpc/virnetserverservice.h
@@ -36,6 +36,9 @@ enum {
 typedef int (*virNetServerServiceDispatchFunc)(virNetServerServicePtr svc,
virNetSocketPtr sock,
void *opaque);
+typedef int (*virNetServerServiceDispatchCheckFunc)(virNetServerServicePtr svc,
+virNetSocketPtr sock,
+void *opaque);
 
 virNetServerServicePtr

Re: [libvirt] [PATCH 1/2] RPC: Don't accept client if it would overcommit max_clients

2013-07-25 Thread Daniel P. Berrange 
On Thu, Jul 25, 2013 at 04:23:32PM +0200, Michal Privoznik wrote:
 Currently, even if max_client limit is hit, we accept() incoming
 connection request, but close it immediately. This has disadvantage of
 not using listen() queue. We should accept() only those clients we
 know we can serve and let all other wait in the (limited) queue.
 ---
  src/rpc/virnetserver.c| 40 
  src/rpc/virnetserverservice.c |  9 +
  src/rpc/virnetserverservice.h |  4 
  3 files changed, 53 insertions(+)
 
 diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
 index cb770c3..7ea1707 100644
 --- a/src/rpc/virnetserver.c
 +++ b/src/rpc/virnetserver.c
 @@ -315,6 +315,34 @@ static int 
 virNetServerDispatchNewClient(virNetServerServicePtr svc,
  }
  
  
 +static int
 +virNetServerDispatchCheck(virNetServerServicePtr svc,
 +  virNetSocketPtr socket,
 +  void *opaque)
 +{
 +virNetServerPtr srv = opaque;
 +int ret = -1;
 +
 +VIR_DEBUG(svc=%p socket=%p opaque=%p, svc, socket, opaque);
 +
 +if (srv-nclients = srv-nclients_max) {
 +VIR_DEBUG(Not accepting client now due to max_clients setting 
 (%zu),
 +  srv-nclients_max);
 +
 +/* We need to temporarily disable the server services in order to
 + * prevent event loop calling us over and over again. */
 +
 +virNetServerUpdateServices(srv, false);
 +
 +goto cleanup;
 +}
 +
 +ret = 0;
 +cleanup:
 +return ret;
 +}
 +
 +
  static void
  virNetServerFatalSignal(int sig, siginfo_t *siginfo ATTRIBUTE_UNUSED,
  void *context ATTRIBUTE_UNUSED)
 @@ -981,6 +1009,7 @@ int virNetServerAddService(virNetServerPtr srv,
  
  virNetServerServiceSetDispatcher(svc,
   virNetServerDispatchNewClient,
 + virNetServerDispatchCheck,
   srv);
  
  virObjectUnlock(srv);
 @@ -1110,6 +1139,8 @@ void virNetServerRun(virNetServerPtr srv)
  virNetServerClientClose(srv-clients[i]);
  if (virNetServerClientIsClosed(srv-clients[i])) {
  virNetServerClientPtr client = srv-clients[i];
 +bool update_services;
 +
  if (srv-nclients  1) {
  memmove(srv-clients + i,
  srv-clients + i + 1,
 @@ -1120,7 +1151,16 @@ void virNetServerRun(virNetServerPtr srv)
  srv-nclients = 0;
  }
  
 +/* Enable services if we can accept a new client.
 + * The new client can be accepted if we are at the limit. */
 +update_services = srv-nclients == srv-nclients_max - 1;
 +
  virObjectUnlock(srv);
 +if (update_services) {
 +/* Now it makes sense to accept() a new client. */
 +VIR_DEBUG(Re-enabling services);
 +virNetServerUpdateServices(srv, true);
 +}
  virObjectUnref(client);
  virObjectLock(srv);
  
 diff --git a/src/rpc/virnetserverservice.c b/src/rpc/virnetserverservice.c
 index 632f03d..a47c8f8 100644
 --- a/src/rpc/virnetserverservice.c
 +++ b/src/rpc/virnetserverservice.c
 @@ -46,6 +46,7 @@ struct _virNetServerService {
  #endif
  
  virNetServerServiceDispatchFunc dispatchFunc;
 +virNetServerServiceDispatchCheckFunc dispatchCheckFunc;
  void *dispatchOpaque;
  };
  
 @@ -74,6 +75,12 @@ static void virNetServerServiceAccept(virNetSocketPtr sock,
  virNetServerServicePtr svc = opaque;
  virNetSocketPtr clientsock = NULL;
  
 +if (svc-dispatchCheckFunc 
 +svc-dispatchCheckFunc(svc, sock, svc-dispatchOpaque)  0) {
 +/* Accept declined */
 +goto cleanup;
 +}
 +

Rather than having this callback, can we not simply change 
virNetServerAddClient()
to call virNetServerUpdateServices(srv, false); when a new client causes us to 
hit
the max limit ?

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] RPC: Don't accept client if it would overcommit max_clients

2013-07-25 Thread Michal Privoznik 
On 25.07.2013 16:37, Daniel P. Berrange wrote:
 On Thu, Jul 25, 2013 at 04:23:32PM +0200, Michal Privoznik wrote:
 Currently, even if max_client limit is hit, we accept() incoming
 connection request, but close it immediately. This has disadvantage of
 not using listen() queue. We should accept() only those clients we
 know we can serve and let all other wait in the (limited) queue.
 ---
  src/rpc/virnetserver.c| 40 
  src/rpc/virnetserverservice.c |  9 +
  src/rpc/virnetserverservice.h |  4 
  3 files changed, 53 insertions(+)


  
 +if (svc-dispatchCheckFunc 
 +svc-dispatchCheckFunc(svc, sock, svc-dispatchOpaque)  0) {
 +/* Accept declined */
 +goto cleanup;
 +}
 +
 
 Rather than having this callback, can we not simply change 
 virNetServerAddClient()
 to call virNetServerUpdateServices(srv, false); when a new client causes us 
 to hit
 the max limit ?
 

No, because that callback is called *after* accept() which I am trying
to avoid.

Michal

 Daniel
 

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

Re: [libvirt] [PATCH 2/2] Introduce max_queued_clients

2013-07-25 Thread Michal Privoznik 
On 25.07.2013 16:34, Daniel P. Berrange wrote:
 On Thu, Jul 25, 2013 at 04:23:33PM +0200, Michal Privoznik wrote:
 This configuration knob lets user to set the length of queue of
 connection requests waiting to be accept()-ed by the daemon. IOW, it
 just controls the @backlog passed to listen:

   int listen(int sockfd, int backlog);
 ---
  daemon/libvirtd-config.c  | 1 +
  daemon/libvirtd-config.h  | 1 +
  daemon/libvirtd.aug   | 1 +
  daemon/libvirtd.c | 4 
  daemon/libvirtd.conf  | 6 ++
  src/locking/lock_daemon.c | 2 +-
  src/lxc/lxc_controller.c  | 1 +
  src/rpc/virnetserverservice.c | 6 --
  src/rpc/virnetserverservice.h | 2 ++
  9 files changed, 21 insertions(+), 3 deletions(-)

 diff --git a/daemon/libvirtd-config.c b/daemon/libvirtd-config.c
 index 107a9cf..c816fda 100644
 --- a/daemon/libvirtd-config.c
 +++ b/daemon/libvirtd-config.c
 @@ -414,6 +414,7 @@ daemonConfigLoadOptions(struct daemonConfig *data,
  GET_CONF_INT(conf, filename, min_workers);
  GET_CONF_INT(conf, filename, max_workers);
  GET_CONF_INT(conf, filename, max_clients);
 +GET_CONF_INT(conf, filename, max_queued_clients);
  
  GET_CONF_INT(conf, filename, prio_workers);
  
 diff --git a/daemon/libvirtd-config.h b/daemon/libvirtd-config.h
 index 973e0ea..a24d5d2 100644
 --- a/daemon/libvirtd-config.h
 +++ b/daemon/libvirtd-config.h
 @@ -63,6 +63,7 @@ struct daemonConfig {
  int min_workers;
  int max_workers;
  int max_clients;
 +int max_queued_clients;
  
  int prio_workers;
  
 diff --git a/daemon/libvirtd.aug b/daemon/libvirtd.aug
 index 7c56a41..70fce5c 100644
 --- a/daemon/libvirtd.aug
 +++ b/daemon/libvirtd.aug
 @@ -56,6 +56,7 @@ module Libvirtd =
 let processing_entry = int_entry min_workers
  | int_entry max_workers
  | int_entry max_clients
 +| int_entry max_queued_clients
  | int_entry max_requests
  | int_entry max_client_requests
  | int_entry prio_workers
 diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
 index 9f7fd8a..402b494 100644
 --- a/daemon/libvirtd.c
 +++ b/daemon/libvirtd.c
 @@ -485,6 +485,7 @@ static int daemonSetupNetworking(virNetServerPtr srv,
 NULL,
  #endif
 false,
 +   config-max_queued_clients,
 config-max_client_requests)))
  goto error;
  if (sock_path_ro) {
 @@ -497,6 +498,7 @@ static int daemonSetupNetworking(virNetServerPtr srv,
   NULL,
  #endif
   true,
 + config-max_queued_clients,
   
 config-max_client_requests)))
  goto error;
  }
 @@ -522,6 +524,7 @@ static int daemonSetupNetworking(virNetServerPtr srv,
   NULL,
  #endif
   false,
 + 
 config-max_queued_clients,
   
 config-max_client_requests)))
  goto error;
  
 @@ -562,6 +565,7 @@ static int daemonSetupNetworking(virNetServerPtr srv,
  config-auth_tls,
  ctxt,
  false,
 +config-max_queued_clients,
  config-max_client_requests))) {
  virObjectUnref(ctxt);
  goto error;
 diff --git a/daemon/libvirtd.conf b/daemon/libvirtd.conf
 index af4493e..5353927 100644
 --- a/daemon/libvirtd.conf
 +++ b/daemon/libvirtd.conf
 @@ -257,6 +257,12 @@
  # over all sockets combined.
  #max_clients = 20
  
 +# The maximum length of queue of connections waiting to be
 +# accepted by the daemon. Note, that some protocols supporting
 +# retransmission may obey this so that a later reattempt at
 +# connection succeeds.
 +#max_queued_clients = 1000
 +
  
  # The minimum limit sets the number of workers to start up
  # initially. If the number of active clients exceeds this,
 diff --git a/src/locking/lock_daemon.c b/src/locking/lock_daemon.c
 index c4c1727..c45f45c 100644
 --- a/src/locking/lock_daemon.c
 +++ b/src/locking/lock_daemon.c
 @@ -671,7 +671,7 @@ virLockDaemonSetupNetworkingNative(virNetServerPtr srv, 
 const char *sock_path)
  #if WITH_GNUTLS
 NULL,
  #endif
 -   false, 1)))
 +   false, 0, 1)))
  return -1;
  
  if

Re: [libvirt] [PATCH 2/2] Introduce max_queued_clients

2013-07-25 Thread Daniel P. Berrange 
On Thu, Jul 25, 2013 at 04:44:57PM +0200, Michal Privoznik wrote:
 On 25.07.2013 16:34, Daniel P. Berrange wrote:
  On Thu, Jul 25, 2013 at 04:23:33PM +0200, Michal Privoznik wrote:
  This configuration knob lets user to set the length of queue of
  connection requests waiting to be accept()-ed by the daemon. IOW, it
  just controls the @backlog passed to listen:
 
int listen(int sockfd, int backlog);
  ---
   daemon/libvirtd-config.c  | 1 +
   daemon/libvirtd-config.h  | 1 +
   daemon/libvirtd.aug   | 1 +
   daemon/libvirtd.c | 4 
   daemon/libvirtd.conf  | 6 ++
   src/locking/lock_daemon.c | 2 +-
   src/lxc/lxc_controller.c  | 1 +
   src/rpc/virnetserverservice.c | 6 --
   src/rpc/virnetserverservice.h | 2 ++
   9 files changed, 21 insertions(+), 3 deletions(-)
 
  diff --git a/daemon/libvirtd-config.c b/daemon/libvirtd-config.c
  index 107a9cf..c816fda 100644
  --- a/daemon/libvirtd-config.c
  +++ b/daemon/libvirtd-config.c
  @@ -414,6 +414,7 @@ daemonConfigLoadOptions(struct daemonConfig *data,
   GET_CONF_INT(conf, filename, min_workers);
   GET_CONF_INT(conf, filename, max_workers);
   GET_CONF_INT(conf, filename, max_clients);
  +GET_CONF_INT(conf, filename, max_queued_clients);
   
   GET_CONF_INT(conf, filename, prio_workers);
   
  diff --git a/daemon/libvirtd-config.h b/daemon/libvirtd-config.h
  index 973e0ea..a24d5d2 100644
  --- a/daemon/libvirtd-config.h
  +++ b/daemon/libvirtd-config.h
  @@ -63,6 +63,7 @@ struct daemonConfig {
   int min_workers;
   int max_workers;
   int max_clients;
  +int max_queued_clients;
   
   int prio_workers;
   
  diff --git a/daemon/libvirtd.aug b/daemon/libvirtd.aug
  index 7c56a41..70fce5c 100644
  --- a/daemon/libvirtd.aug
  +++ b/daemon/libvirtd.aug
  @@ -56,6 +56,7 @@ module Libvirtd =
  let processing_entry = int_entry min_workers
   | int_entry max_workers
   | int_entry max_clients
  +| int_entry max_queued_clients
   | int_entry max_requests
   | int_entry max_client_requests
   | int_entry prio_workers
  diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
  index 9f7fd8a..402b494 100644
  --- a/daemon/libvirtd.c
  +++ b/daemon/libvirtd.c
  @@ -485,6 +485,7 @@ static int daemonSetupNetworking(virNetServerPtr srv,
  NULL,
   #endif
  false,
  +   config-max_queued_clients,
  config-max_client_requests)))
   goto error;
   if (sock_path_ro) {
  @@ -497,6 +498,7 @@ static int daemonSetupNetworking(virNetServerPtr srv,
NULL,
   #endif
true,
  + 
  config-max_queued_clients,

  config-max_client_requests)))
   goto error;
   }
  @@ -522,6 +524,7 @@ static int daemonSetupNetworking(virNetServerPtr srv,
NULL,
   #endif
false,
  + 
  config-max_queued_clients,

  config-max_client_requests)))
   goto error;
   
  @@ -562,6 +565,7 @@ static int daemonSetupNetworking(virNetServerPtr srv,
   config-auth_tls,
   ctxt,
   false,
  +config-max_queued_clients,
   
  config-max_client_requests))) {
   virObjectUnref(ctxt);
   goto error;
  diff --git a/daemon/libvirtd.conf b/daemon/libvirtd.conf
  index af4493e..5353927 100644
  --- a/daemon/libvirtd.conf
  +++ b/daemon/libvirtd.conf
  @@ -257,6 +257,12 @@
   # over all sockets combined.
   #max_clients = 20
   
  +# The maximum length of queue of connections waiting to be
  +# accepted by the daemon. Note, that some protocols supporting
  +# retransmission may obey this so that a later reattempt at
  +# connection succeeds.
  +#max_queued_clients = 1000
  +
   
   # The minimum limit sets the number of workers to start up
   # initially. If the number of active clients exceeds this,
  diff --git a/src/locking/lock_daemon.c b/src/locking/lock_daemon.c
  index c4c1727..c45f45c 100644
  --- a/src/locking/lock_daemon.c
  +++ b/src/locking/lock_daemon.c
  @@ -671,7 +671,7 @@ virLockDaemonSetupNetworkingNative(virNetServerPtr 
  srv, const char *sock_path)
   #if WITH_GNUTLS

Re: [libvirt] [PATCH 1/2] RPC: Don't accept client if it would overcommit max_clients

2013-07-25 Thread Daniel P. Berrange 
On Thu, Jul 25, 2013 at 04:43:52PM +0200, Michal Privoznik wrote:
 On 25.07.2013 16:37, Daniel P. Berrange wrote:
  On Thu, Jul 25, 2013 at 04:23:32PM +0200, Michal Privoznik wrote:
  Currently, even if max_client limit is hit, we accept() incoming
  connection request, but close it immediately. This has disadvantage of
  not using listen() queue. We should accept() only those clients we
  know we can serve and let all other wait in the (limited) queue.
  ---
   src/rpc/virnetserver.c| 40 
  
   src/rpc/virnetserverservice.c |  9 +
   src/rpc/virnetserverservice.h |  4 
   3 files changed, 53 insertions(+)
 
 
   
  +if (svc-dispatchCheckFunc 
  +svc-dispatchCheckFunc(svc, sock, svc-dispatchOpaque)  0) {
  +/* Accept declined */
  +goto cleanup;
  +}
  +
  
  Rather than having this callback, can we not simply change 
  virNetServerAddClientb()
  to call virNetServerUpdateServices(srv, false); when a new client causes us 
  to hit
  the max limit ?
  
 
 No, because that callback is called *after* accept() which I am trying
 to avoid.

I'm not sure I see wha the problem with doing that is.

IIUC, with your patch, if we have max clients == 30, and have 30 current
active clients, and a 31st comes in, virNetServerServiceAccept is
invoked. We then call dispatchCheckFunc see that we're at the limit,
don't accept() the connection and disable the event polling from then
on.

If we gave responsibility to the virNetServerAddClient callback, then
if we have max clients == 30, and then 30th client comes in, we accept
that, call virNetServerAddClient(), which sees we're at the limti and
so disables event polling.

So its the difference between disabling event polling at the time the
max client is accepted, or disabling it when the max + 1 client arrives
and is about to be accepted. IMHO the former makes more sense.

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] Entering freeze for libvirt-1.1.1

2013-07-25 Thread Jason Helfman 
On Tue, Jul 23, 2013 at 9:11 PM, Daniel Veillard veill...@redhat.comwrote:

  As planned I tagged the release candidate 1 for libvirt 1.1.1 in git,
 I also made tarball and rpms available on the ftp:

   ftp://libvirt.org/libvirt/

  There is still some patches not completely ready after initial review
 it is probably safe to push the simplest ones, but not big sets now.
 Bug fixes are okay until final release obviously !

  The release seems to work fine in my local testing (my fedora 19
 crashed when shutdown a window guest, but that sounds like a kernel
 problem). Goal is to release next tuesday,

   please give it a try, and report problems :-)

thanks !

 Daniel


Configure failure on BSD when detecting libssh2. This is the first time I
have run into this. Has anything changed?

configure: error: You must install the libssh2 = 1.3pkg-config module to
compile libvirt
===  Script configure failed unexpectedly.
Please report the problem to j...@freebsd.org [maintainer] and attach the
/usr/home/jgh/workspace/ports/devel/libvirt/work/libvirt-1.1.1/config.log
including the output of the failure of your make command. Also, it might be
a good idea to provide an overview of all packages installed on your system
(e.g. a /usr/local/sbin/pkg-static info -g -Ea).
*** [do-configure] Error code 1

Stop in /usr/home/jgh/workspace/ports/devel/libvirt.
*** [build] Error code 1

Stop in /usr/home/jgh/workspace/ports/devel/libvirt.
[jgh@hatter ~/workspace/ports/devel/libvirt]$ pkg info -l libssh2 |grep pc
/usr/local/libdata/pkgconfig/libssh2.pc

-jgh

-- 
Jason Helfman  | FreeBSD Committer
j...@freebsd.org | http://people.freebsd.org/~jgh  | The Power to Serve
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] Cannot compile libvirt

2013-07-25 Thread Abhishek Sharma 
Thanks Jan,

Yes it worked when I removed --without-remote.
Thanks for your help.

Cheers,
Abhishek


On Thu, Jul 25, 2013 at 12:37 AM, Ján Tomko jto...@redhat.com wrote:

 On 07/24/2013 11:24 PM, Abhishek Sharma wrote:
  Hi,
 
  I am new to libvirt and I am trying to compile libvirt from the git
  repository. I mainly plan to use it for managing ESX server. However I
 get the
  following error during make.
 
  /libvirt undefined reference to virNetTLSInit/
 
  Here are the steps which I took.
 
  1. Get the code. git clone git://libvirt.org/libvirt.git
  http://libvirt.org/libvirt.git
  2. Run autogen
  ./autogen.sh --without-xen --with-esx --without-numactl --without-remote
  --without-libvirtd --without-one --without-openvz --without-qemu
 --without-uml
  --without-phyp --without-vbox --without-lxc --without-one --without-test
  --without-network --without-selinux --with-python
 
  3. make
 
 
  Looks like the following commit somehow separated the gnutls symbols.
 
  /commit 83d7e4e4607a96b6959a6b0afd44a10a783fd928
  Author: Daniel P. Berrange berra...@redhat.com mailto:
 berra...@redhat.com
  Date:   Wed Mar 13 13:37:29 2013 +
 
  Use separate symbol file for GNUTLS symbols/
 
  How can I successfully compile libvirt ?

 Try building it --with-remote, it seems we assume that in quite a few
 places.

 Jan

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

Re: [libvirt] Cannot compile libvirt

2013-07-25 Thread Daniel P. Berrange 
On Thu, Jul 25, 2013 at 09:38:57AM -0700, Abhishek Sharma wrote:
 Since I am new to this mailing list, I am not sure what is the procedure of
 tracking bugs/enhancements like this. Should I go ahead and file a bug at
 https://bugzilla.redhat.com/ ?

Take a look at this page

  http://libvirt.org/bugs.html

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] Cannot compile libvirt

2013-07-25 Thread Eric Blake 
On 07/25/2013 10:38 AM, Abhishek Sharma wrote:
 Since I am new to this mailing list, I am not sure what is the procedure of

[you top-posted again]

 tracking bugs/enhancements like this. Should I go ahead and file a bug at
 https://bugzilla.redhat.com/ ?

Only if you think it isn't getting resolved fast enough. Most likely,
since we are in code freeze for 1.1.1, and have identified this as a bug
worth fixing, it is fresh on our minds and will get fixed without
needing the extra paperwork of a BZ to close.  But if we don't get it
fixed before 1.1.1, then opening a BZ is wise, so that we don't forget
to do it 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] Cannot compile libvirt

2013-07-25 Thread Abhishek Sharma 
Since I am new to this mailing list, I am not sure what is the procedure of
tracking bugs/enhancements like this. Should I go ahead and file a bug at
https://bugzilla.redhat.com/ ?



On Thu, Jul 25, 2013 at 9:22 AM, Eric Blake ebl...@redhat.com wrote:

 On 07/25/2013 10:17 AM, Abhishek Sharma wrote:
  Thanks Jan,

 [please don't top-post on technical lists]

 
  Yes it worked when I removed --without-remote.

 Good to know.  That said, we should still fix things before 1.1.1 is
 released to fix this bug, so that future users don't have to figure out
 the magic configure option to use.


  2. Run autogen
  ./autogen.sh --without-xen --with-esx --without-numactl
 --without-remote
  --without-libvirtd --without-one --without-openvz --without-qemu
  --without-uml
  --without-phyp --without-vbox --without-lxc --without-one
 --without-test
  --without-network --without-selinux --with-python
 

  How can I successfully compile libvirt ?
 
  Try building it --with-remote, it seems we assume that in quite a few
  places.


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


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

Re: [libvirt] Entering freeze for libvirt-1.1.1

2013-07-25 Thread Roman Bogorodskiy 
  Jason Helfman wrote:

 Configure failure on BSD when detecting libssh2. This is the first time I
 have run into this. Has anything changed?
 
 configure: error: You must install the libssh2 = 1.3pkg-config module to
 compile libvirt
 ===  Script configure failed unexpectedly.
 Please report the problem to j...@freebsd.org [maintainer] and attach the
 /usr/home/jgh/workspace/ports/devel/libvirt/work/libvirt-1.1.1/config.log
 including the output of the failure of your make command. Also, it might be
 a good idea to provide an overview of all packages installed on your system
 (e.g. a /usr/local/sbin/pkg-static info -g -Ea).
 *** [do-configure] Error code 1
 
 Stop in /usr/home/jgh/workspace/ports/devel/libvirt.
 *** [build] Error code 1
 
 Stop in /usr/home/jgh/workspace/ports/devel/libvirt.
 [jgh@hatter ~/workspace/ports/devel/libvirt]$ pkg info -l libssh2 |grep pc
 /usr/local/libdata/pkgconfig/libssh2.pc
 
 -jgh

Never tried to build with libssh2. Just downloaded rc1 now and did:

./configure --with-ssh2

and it worked fine for me:

configure:   ssh2: yes (CFLAGS='-I/usr/local/include  '
LIBS='-L/usr/local/lib -lssh2  -Wl,-rpath=/usr/lib:/usr/local/lib  ')

Could you provide config.log?

Roman Bogorodskiy


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

Re: [libvirt] Cannot compile libvirt

2013-07-25 Thread Eric Blake 
On 07/25/2013 10:17 AM, Abhishek Sharma wrote:
 Thanks Jan,

[please don't top-post on technical lists]

 
 Yes it worked when I removed --without-remote.

Good to know.  That said, we should still fix things before 1.1.1 is
released to fix this bug, so that future users don't have to figure out
the magic configure option to use.


 2. Run autogen
 ./autogen.sh --without-xen --with-esx --without-numactl --without-remote
 --without-libvirtd --without-one --without-openvz --without-qemu
 --without-uml
 --without-phyp --without-vbox --without-lxc --without-one --without-test
 --without-network --without-selinux --with-python


 How can I successfully compile libvirt ?

 Try building it --with-remote, it seems we assume that in quite a few
 places.


-- 
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] virt-login-shell joins users into lxc container.

2013-07-25 Thread Daniel P. Berrange 
On Sat, Jul 20, 2013 at 07:46:33AM -0400, dwa...@redhat.com wrote:
 From: Dan Walsh dwa...@redhat.com
 
 Openshift wants to have their gears stuck into a container when they login
 to the system.  virt-login-shell will join a running gear with the username of
 the person running it, or attempt to start the container if it is not running.
 (Currently containers do not exist if they are not running, so I can not test
 this feature. But the code is there).
 
 This tool needs to be setuid since joining a container (nsjoin) requires 
 privs.
 The root user is not allowed to execute this command. When this tool is
 run by a normal user it will only join the users container.
 
 Only users who are listed as valid_users in /etc/libvirt/virt-login-shell.conf
 are allowed to join containers using this tool. By default no users are 
 allowed.
 ---
  .gitignore  |   1 +
  libvirt.spec.in |   3 +
  mingw-libvirt.spec.in   |   5 +
  po/POTFILES.in  |   1 +
  src/libvirt_private.syms|   1 +
  src/util/virutil.c  |   7 +
  src/util/virutil.h  |   1 +
  tools/Makefile.am   |  30 -
  tools/virt-login-shell.c| 312 
 
  tools/virt-login-shell.conf |  24 
  tools/virt-login-shell.pod  |  62 +
  11 files changed, 446 insertions(+), 1 deletion(-)
  create mode 100644 tools/virt-login-shell.c
  create mode 100755 tools/virt-login-shell.conf
  create mode 100644 tools/virt-login-shell.pod

ACK to this patch.

Technically since we're post freeze we shouldn't commit this until
1.1.2, but since this is an entirely new program perhaps we could
make an exception here ? Thoughts ?

It doesn't hugely matter either way, it'd just make life a little
easier to have it in 1.1.1

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] virt-login-shell joins users into lxc container.

2013-07-25 Thread Eric Blake 
On 07/25/2013 11:06 AM, Daniel P. Berrange wrote:
 On Sat, Jul 20, 2013 at 07:46:33AM -0400, dwa...@redhat.com wrote:
 From: Dan Walsh dwa...@redhat.com

 Openshift wants to have their gears stuck into a container when they login
 to the system.  virt-login-shell will join a running gear with the username 
 of
 the person running it, or attempt to start the container if it is not 
 running.
 (Currently containers do not exist if they are not running, so I can not test
 this feature. But the code is there).

 This tool needs to be setuid since joining a container (nsjoin) requires 
 privs.
 The root user is not allowed to execute this command. When this tool is
 run by a normal user it will only join the users container.

 Only users who are listed as valid_users in 
 /etc/libvirt/virt-login-shell.conf
 are allowed to join containers using this tool. By default no users are 
 allowed.
 ---

 
 ACK to this patch.
 
 Technically since we're post freeze we shouldn't commit this until
 1.1.2, but since this is an entirely new program perhaps we could
 make an exception here ? Thoughts ?

It was posted pre-freeze; the only reason it didn't make freeze was lack
of timely review.

 
 It doesn't hugely matter either way, it'd just make life a little
 easier to have it in 1.1.1

I see little risk in including it in 1.1.1, since it really is a new
feature without touching existing 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] [PATCH] virt-login-shell joins users into lxc container.

2013-07-25 Thread Daniel J Walsh 
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

On 07/25/2013 01:23 PM, Eric Blake wrote:
 On 07/25/2013 11:09 AM, Eric Blake wrote:
 
 ACK to this patch.
 
 Technically since we're post freeze we shouldn't commit this until 
 1.1.2, but since this is an entirely new program perhaps we could make
 an exception here ? Thoughts ?
 
 It was posted pre-freeze; the only reason it didn't make freeze was lack 
 of timely review.
 
 That said, since the patch adds a setuid binary, I would like to review it
 as a second set of eyes before we push it.
 
The code was sent to a  few reviewers, but I have not heard back,  I will
resend the latest code.
-BEGIN PGP SIGNATURE-
Version: GnuPG v1.4.13 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iEYEARECAAYFAlHxX4oACgkQrlYvE4MpobMKhgCguv0ydgBjcEkC5Kxf/rt3OAwv
u4gAn0bA6VM83MJm91dc5gOSC9gJpdcu
=lA4e
-END PGP SIGNATURE-

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

Re: [libvirt] [PATCH] virt-login-shell joins users into lxc container.

2013-07-25 Thread Eric Blake 
On 07/25/2013 11:09 AM, Eric Blake wrote:

 ACK to this patch.

 Technically since we're post freeze we shouldn't commit this until
 1.1.2, but since this is an entirely new program perhaps we could
 make an exception here ? Thoughts ?
 
 It was posted pre-freeze; the only reason it didn't make freeze was lack
 of timely review.

That said, since the patch adds a setuid binary, I would like to review
it as a second set of eyes before we push 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] [PATCH 1/5] Introduce a more convenient virCgroupNewDetectMachine

2013-07-25 Thread Eric Blake 
On 07/25/2013 06:20 AM, Daniel P. Berrange wrote:
 From: Daniel P. Berrange berra...@redhat.com
 
 Instead of requiring drivers to use a combination of calls
 to virCgroupNewDetect and virCgroupIsValidMachine, combine
 the two into virCgroupNewDetectMachine
 
 Signed-off-by: Daniel P. Berrange berra...@redhat.com
 ---
  src/libvirt_private.syms |  1 +
  src/lxc/lxc_process.c| 20 
  src/qemu/qemu_cgroup.c   | 16 
  src/util/vircgroup.c | 22 ++
  src/util/vircgroup.h |  5 +
  5 files changed, 40 insertions(+), 24 deletions(-)

 @@ -1575,6 +1575,28 @@ int virCgroupNewDetect(pid_t pid ATTRIBUTE_UNUSED,
  }
  #endif
  
 +/*
 + * Returns 0 on success, -1 on fatal error, -2 on no valid cgroup
 + */
 +int virCgroupNewDetectMachine(const char *name,
 +  const char *drivername,
 +  pid_t pid,
 +  virCgroupPtr *group)
 +{
 +if (virCgroupNewDetect(pid, group)  0) {
 +if (virCgroupNewIgnoreError())
 +return 0;
 +return -1;
 +}
 +
 +if (!virCgroupIsValidMachineGroup(*group, name, drivername)) {
 +virCgroupFree(group);
 +return 0;

Huh? This says you are returning success.  Also, none of the lxc or qemu
callers checked for a -2 return; do you really need the differentiated
return type?

-- 
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/5] Make virCgroupIsValidMachine static

2013-07-25 Thread Eric Blake 
On 07/25/2013 06:20 AM, Daniel P. Berrange wrote:
 From: Daniel P. Berrange berra...@redhat.com
 
 The virCgroupIsValidMachine does not need to be called from
 outside the cgroups file now, so make it static.
 
 Signed-off-by: Daniel P. Berrange berra...@redhat.com
 ---
  src/libvirt_private.syms | 1 -
  src/util/vircgroup.c | 7 ---
  src/util/vircgroup.h | 5 -
  3 files changed, 4 insertions(+), 9 deletions(-)

ACK.

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



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

Re: [libvirt] [PATCH 3/5] Fix detection of 'emulator' cgroup

2013-07-25 Thread Eric Blake 
On 07/25/2013 06:20 AM, Daniel P. Berrange wrote:
 From: Daniel P. Berrange berra...@redhat.com
 
 When a VM has an 'emulator' child cgroup present, we must
 strip off that suffix when detecting the cgroup for a
 machine
 
 Rename the virCgroupIsValidMachineGroup method to
 virCgroupValidateMachineGroup to make a bit clearer
 that this isn't simply a boolean check, it will make
 changes to the object.
 
 Signed-off-by: Daniel P. Berrange berra...@redhat.com
 ---
  src/util/vircgroup.c | 29 +++--
  1 file changed, 23 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] [PATCH 4/5] Add 'controllers' arg to virCgroupNewDetect

2013-07-25 Thread Eric Blake 
On 07/25/2013 06:20 AM, Daniel P. Berrange wrote:
 From: Daniel P. Berrange berra...@redhat.com
 
 When detecting cgroups we must honour any controllers
 whitelist the driver may have.
 
 Signed-off-by: Daniel P. Berrange berra...@redhat.com
 ---
  src/lxc/lxc_process.c  | 4 ++--
  src/qemu/qemu_cgroup.c | 1 +
  src/util/vircgroup.c   | 9 ++---
  src/util/vircgroup.h   | 2 ++
  4 files changed, 11 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] [PATCH 5/5] Skip detecting placement if controller is disabled

2013-07-25 Thread Eric Blake 
On 07/25/2013 06:20 AM, Daniel P. Berrange wrote:
 From: Daniel P. Berrange berra...@redhat.com
 
 If the app has provided a whitelist of controllers to be used,
 we skip detecting its mount point. We still, however, fill in
 the placement info which later confuses the machine name
 validation code. Skip detecting placement if the controller
 mount point is not set
 
 Signed-off-by: Daniel P. Berrange berra...@redhat.com
 ---
  src/util/vircgroup.c | 7 ---
  1 file changed, 4 insertions(+), 3 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] [Qemu-devel] [PATCH 4/7] qemu: Add monitor APIs to fetch CPUID data from QEMU

2013-07-25 Thread Eduardo Habkost 
On Thu, Jul 25, 2013 at 04:09:18PM +0200, Andreas Färber wrote:
 Am 25.07.2013 16:00, schrieb Eduardo Habkost:
  libvirt
  needs a way to find out how exactly -machine foo-1.0 -cpu bar looks
  different from -machine foo-1.1 -cpu bar,
 
 Why? (What's the actual use case?)

libvirt API allows individual CPU features to be configured, so libvirt
needs to know what exactly will be the result of using a
machine-type/CPU-model combination to make sure it will be exactly what
was requested:
http://libvirt.org/formatdomain.html#elementsCPU

Also, libvirt needs to be able to check if migration to a host is
possible (i.e. if all features enabled by a machine-type/CPU-model
combination are supported by the host) before actually starting the
migration process.

-- 
Eduardo

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

Re: [libvirt] [PATCH 1/5] Introduce a more convenient virCgroupNewDetectMachine

2013-07-25 Thread Daniel P. Berrange 
On Thu, Jul 25, 2013 at 11:37:12AM -0600, Eric Blake wrote:
 On 07/25/2013 06:20 AM, Daniel P. Berrange wrote:
  From: Daniel P. Berrange berra...@redhat.com
  
  Instead of requiring drivers to use a combination of calls
  to virCgroupNewDetect and virCgroupIsValidMachine, combine
  the two into virCgroupNewDetectMachine
  
  Signed-off-by: Daniel P. Berrange berra...@redhat.com
  ---
   src/libvirt_private.syms |  1 +
   src/lxc/lxc_process.c| 20 
   src/qemu/qemu_cgroup.c   | 16 
   src/util/vircgroup.c | 22 ++
   src/util/vircgroup.h |  5 +
   5 files changed, 40 insertions(+), 24 deletions(-)
 
  @@ -1575,6 +1575,28 @@ int virCgroupNewDetect(pid_t pid ATTRIBUTE_UNUSED,
   }
   #endif
   
  +/*
  + * Returns 0 on success, -1 on fatal error, -2 on no valid cgroup
  + */
  +int virCgroupNewDetectMachine(const char *name,
  +  const char *drivername,
  +  pid_t pid,
  +  virCgroupPtr *group)
  +{
  +if (virCgroupNewDetect(pid, group)  0) {
  +if (virCgroupNewIgnoreError())
  +return 0;
  +return -1;
  +}
  +
  +if (!virCgroupIsValidMachineGroup(*group, name, drivername)) {
  +virCgroupFree(group);
  +return 0;
 
 Huh? This says you are returning success.  Also, none of the lxc or qemu
 callers checked for a -2 return; do you really need the differentiated
 return type?

Opps the comment is wrong. I originally had it returning -2, but I
removed that and just useed '0' and let the caller check if 'group != NULL'
instead.


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 1/2] Separate out StateAutoStart from StateInitialize

2013-07-25 Thread John Ferlan 
Separation allows for dependent drivers to be make a connection during
the AutoStart phase of state initialization.
---
 src/driver.h  |  4 
 src/libvirt.c | 23 ++-
 2 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/src/driver.h b/src/driver.h
index cc03e9f..b416902 100644
--- a/src/driver.h
+++ b/src/driver.h
@@ -1801,6 +1801,9 @@ typedef int
  void *opaque);
 
 typedef int
+(*virDrvStateAutoStart)(void);
+
+typedef int
 (*virDrvStateCleanup)(void);
 
 typedef int
@@ -1815,6 +1818,7 @@ typedef virStateDriver *virStateDriverPtr;
 struct _virStateDriver {
 const char *name;
 virDrvStateInitialize stateInitialize;
+virDrvStateAutoStart stateAutoStart;
 virDrvStateCleanup stateCleanup;
 virDrvStateReload stateReload;
 virDrvStateStop stateStop;
diff --git a/src/libvirt.c b/src/libvirt.c
index 444c1c3..864321f 100644
--- a/src/libvirt.c
+++ b/src/libvirt.c
@@ -808,7 +808,11 @@ virRegisterStateDriver(virStateDriverPtr driver)
  * @callback: callback to invoke to inhibit shutdown of the daemon
  * @opaque: data to pass to @callback
  *
- * Initialize all virtualization drivers.
+ * Initialize all virtualization drivers. Accomplished in two phases,
+ * the first being state and structure initialization followed by any
+ * auto start supported by the driver.  This is done to ensure dependencies
+ * that some drivers may have on another driver having been initialized
+ * will exist, such as the storage driver's need to use the secret driver.
  *
  * Returns 0 if all succeed, -1 upon any failure.
  */
@@ -836,6 +840,23 @@ int virStateInitialize(bool privileged,
 }
 }
 }
+
+for (i = 0; i  virStateDriverTabCount; i++) {
+if (virStateDriverTab[i]-stateAutoStart) {
+VIR_DEBUG(Running global auto start for %s state driver,
+  virStateDriverTab[i]-name);
+if (virStateDriverTab[i]-stateAutoStart()  0) {
+virErrorPtr err = virGetLastError();
+VIR_ERROR(_(Auto start for %s driver failed: %s),
+  virStateDriverTab[i]-name,
+  err  err-message ? err-message :
+_(Unknown problem));
+if (virStateDriverTab[i]-stateCleanup)
+ignore_value(virStateDriverTab[i]-stateCleanup());
+return -1;
+}
+}
+}
 return 0;
 }
 
-- 
1.8.1.4

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

[libvirt] [PATCH 0/2] Split driver StateAutoStart from StateInitialization

2013-07-25 Thread John Ferlan 
The post push review/comments for the chap authentication determined that
trying to connect to qemu driver from within the storage auto start would
not be successful, see the following and followups

https://www.redhat.com/archives/libvir-list/2013-July/msg01409.html

These patches will split the virStateInitialize() into two phases - the
first being the bulk of the existing code and the second being just running
the auto start functionality for each of the drivers that needs/support it.

I realize it's probably too late for 1.1.1, but figured it'd be good to be
ready when the barn door opens again.

John Ferlan (2):
  Separate out StateAutoStart from StateInitialize
  virStateDriver - Separate AutoStart from Initialize

 src/driver.h |  4 
 src/libvirt.c| 23 ++-
 src/libxl/libxl_driver.c | 18 +++---
 src/lxc/lxc_driver.c | 18 --
 src/network/bridge_driver.c  | 20 +++-
 src/qemu/qemu_driver.c   | 18 --
 src/storage/storage_driver.c | 19 ++-
 src/uml/uml_driver.c | 18 --
 8 files changed, 126 insertions(+), 12 deletions(-)

-- 
1.8.1.4

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

[libvirt] [PATCH 2/2] virStateDriver - Separate AutoStart from Initialize

2013-07-25 Thread John Ferlan 
Adjust these drivers to handle their Autostart functionality after each
of the drivers has gone through their Initialization functions

merge
---
 src/libxl/libxl_driver.c | 18 +++---
 src/lxc/lxc_driver.c | 18 --
 src/network/bridge_driver.c  | 20 +++-
 src/qemu/qemu_driver.c   | 18 --
 src/storage/storage_driver.c | 19 ++-
 src/uml/uml_driver.c | 18 --
 6 files changed, 100 insertions(+), 11 deletions(-)

diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index 98b1985..3dc8b32 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -1348,9 +1348,6 @@ libxlStateInitialize(bool privileged,
NULL, NULL)  0)
 goto error;
 
-virDomainObjListForEach(libxl_driver-domains, libxlAutostartDomain,
-libxl_driver);
-
 virDomainObjListForEach(libxl_driver-domains, libxlDomainManagedSaveLoad,
 libxl_driver);
 
@@ -1369,6 +1366,20 @@ fail:
 }
 
 static int
+libxlStateAutoStart(void)
+{
+if (!libxl_driver)
+return 0;
+
+libxlDriverLock(libxl_driver);
+virDomainObjListForEach(libxl_driver-domains, libxlAutostartDomain,
+libxl_driver);
+libxlDriverUnlock(libxl_driver);
+
+return 0;
+}
+
+static int
 libxlStateReload(void)
 {
 if (!libxl_driver)
@@ -4887,6 +4898,7 @@ static virDriver libxlDriver = {
 static virStateDriver libxlStateDriver = {
 .name = LIBXL,
 .stateInitialize = libxlStateInitialize,
+.stateAutoStart = libxlStateAutoStart,
 .stateCleanup = libxlStateCleanup,
 .stateReload = libxlStateReload,
 };
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
index 21cf2e3..75c03e0 100644
--- a/src/lxc/lxc_driver.c
+++ b/src/lxc/lxc_driver.c
@@ -1455,8 +1455,6 @@ static int lxcStateInitialize(bool privileged,
NULL, NULL)  0)
 goto cleanup;
 
-virLXCProcessAutostartAll(lxc_driver);
-
 virNWFilterRegisterCallbackDriver(lxcCallbackDriver);
 return 0;
 
@@ -1466,6 +1464,21 @@ cleanup:
 return -1;
 }
 
+/**
+ * lxcStateAutoStart:
+ *
+ * Function to autostart the LXC daemons
+ */
+static int lxcStateAutoStart(void)
+{
+if (!lxc_driver)
+return 0;
+
+virLXCProcessAutostartAll(lxc_driver);
+
+return 0;
+}
+
 static void lxcNotifyLoadDomain(virDomainObjPtr vm, int newVM, void *opaque)
 {
 virLXCDriverPtr driver = opaque;
@@ -4665,6 +4678,7 @@ static virDriver lxcDriver = {
 static virStateDriver lxcStateDriver = {
 .name = LXC_DRIVER_NAME,
 .stateInitialize = lxcStateInitialize,
+.stateAutoStart = lxcStateAutoStart,
 .stateCleanup = lxcStateCleanup,
 .stateReload = lxcStateReload,
 };
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 0bb57ea..98dea9e 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -430,7 +430,6 @@ networkStateInitialize(bool privileged,
 networkFindActiveConfigs(driverState);
 networkReloadFirewallRules(driverState);
 networkRefreshDaemons(driverState);
-networkAutostartConfigs(driverState);
 
 networkDriverUnlock(driverState);
 
@@ -474,6 +473,24 @@ error:
 }
 
 /**
+ * networkStateAutoStart:
+ *
+ * Function to AutoStart the bridge configs
+ */
+static int
+networkStateAutoStart(void)
+{
+if (!driverState)
+return 0;
+
+networkDriverLock(driverState);
+networkAutostartConfigs(driverState);
+networkDriverUnlock(driverState);
+
+return 0;
+}
+
+/**
  * networkStateReload:
  *
  * Function to restart the QEmu daemon, it will recheck the configuration
@@ -3693,6 +3710,7 @@ static virNetworkDriver networkDriver = {
 static virStateDriver networkStateDriver = {
 .name = Network,
 .stateInitialize  = networkStateInitialize,
+.stateAutoStart  = networkStateAutoStart,
 .stateCleanup = networkStateCleanup,
 .stateReload = networkStateReload,
 };
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 2adf6e2..ec909b3 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -837,8 +837,6 @@ qemuStateInitialize(bool privileged,
 if (!qemu_driver-workerPool)
 goto error;
 
-qemuAutostartDomains(qemu_driver);
-
 if (conn)
 virConnectClose(conn);
 
@@ -855,6 +853,21 @@ error:
 return -1;
 }
 
+/**
+ * qemuStateAutoStart:
+ *
+ * Function to auto start the QEmu daemons
+ */
+static int
+qemuStateAutoStart(void)
+{
+if (!qemu_driver)
+return 0;
+
+qemuAutostartDomains(qemu_driver);
+return 0;
+}
+
 static void qemuNotifyLoadDomain(virDomainObjPtr vm, int newVM, void *opaque)
 {
 virQEMUDriverPtr driver = opaque;
@@ -16276,6 +16289,7 @@ static virDriver qemuDriver = {
 static virStateDriver qemuStateDriver = {
 .name = QEMU,
 .stateInitialize = qemuStateInitialize,
+

Re: [libvirt] [PATCH 0/2] Split driver StateAutoStart from StateInitialization

2013-07-25 Thread Daniel P. Berrange 
On Thu, Jul 25, 2013 at 02:32:55PM -0400, John Ferlan wrote:
 The post push review/comments for the chap authentication determined that
 trying to connect to qemu driver from within the storage auto start would
 not be successful, see the following and followups
 
 https://www.redhat.com/archives/libvir-list/2013-July/msg01409.html
 
 These patches will split the virStateInitialize() into two phases - the
 first being the bulk of the existing code and the second being just running
 the auto start functionality for each of the drivers that needs/support it.
 
 I realize it's probably too late for 1.1.1, but figured it'd be good to be
 ready when the barn door opens again.

I don't know about others, but I feel the change is simple enough
that I wouldn't be against including it in 1.1.1, given that it
solves a real world bug we have in startup ordering.

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] Separate out StateAutoStart from StateInitialize

2013-07-25 Thread Daniel P. Berrange 
On Thu, Jul 25, 2013 at 02:32:56PM -0400, John Ferlan wrote:
 Separation allows for dependent drivers to be make a connection during
 the AutoStart phase of state initialization.
 ---
  src/driver.h  |  4 
  src/libvirt.c | 23 ++-
  2 files changed, 26 insertions(+), 1 deletion(-)
 
 diff --git a/src/driver.h b/src/driver.h
 index cc03e9f..b416902 100644
 --- a/src/driver.h
 +++ b/src/driver.h
 @@ -1801,6 +1801,9 @@ typedef int
   void *opaque);
  
  typedef int
 +(*virDrvStateAutoStart)(void);
 +
 +typedef int
  (*virDrvStateCleanup)(void);
  
  typedef int
 @@ -1815,6 +1818,7 @@ typedef virStateDriver *virStateDriverPtr;
  struct _virStateDriver {
  const char *name;
  virDrvStateInitialize stateInitialize;
 +virDrvStateAutoStart stateAutoStart;
  virDrvStateCleanup stateCleanup;
  virDrvStateReload stateReload;
  virDrvStateStop stateStop;
 diff --git a/src/libvirt.c b/src/libvirt.c
 index 444c1c3..864321f 100644
 --- a/src/libvirt.c
 +++ b/src/libvirt.c
 @@ -808,7 +808,11 @@ virRegisterStateDriver(virStateDriverPtr driver)
   * @callback: callback to invoke to inhibit shutdown of the daemon
   * @opaque: data to pass to @callback
   *
 - * Initialize all virtualization drivers.
 + * Initialize all virtualization drivers. Accomplished in two phases,
 + * the first being state and structure initialization followed by any
 + * auto start supported by the driver.  This is done to ensure dependencies
 + * that some drivers may have on another driver having been initialized
 + * will exist, such as the storage driver's need to use the secret driver.
   *
   * Returns 0 if all succeed, -1 upon any failure.
   */
 @@ -836,6 +840,23 @@ int virStateInitialize(bool privileged,
  }
  }
  }
 +
 +for (i = 0; i  virStateDriverTabCount; i++) {
 +if (virStateDriverTab[i]-stateAutoStart) {
 +VIR_DEBUG(Running global auto start for %s state driver,
 +  virStateDriverTab[i]-name);
 +if (virStateDriverTab[i]-stateAutoStart()  0) {
 +virErrorPtr err = virGetLastError();
 +VIR_ERROR(_(Auto start for %s driver failed: %s),
 +  virStateDriverTab[i]-name,
 +  err  err-message ? err-message :
 +_(Unknown problem));
 +if (virStateDriverTab[i]-stateCleanup)
 +ignore_value(virStateDriverTab[i]-stateCleanup());
 +return -1;
 +}
 +}
 +}
  return 0;
  }

ACK

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

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

Re: [libvirt] [PATCH 1/2] Separate out StateAutoStart from StateInitialize

2013-07-25 Thread Daniel P. Berrange 
On Thu, Jul 25, 2013 at 07:39:35PM +0100, Daniel P. Berrange wrote:
 On Thu, Jul 25, 2013 at 02:32:56PM -0400, John Ferlan wrote:
  Separation allows for dependent drivers to be make a connection during
  the AutoStart phase of state initialization.
  ---
   src/driver.h  |  4 
   src/libvirt.c | 23 ++-
   2 files changed, 26 insertions(+), 1 deletion(-)
  
  diff --git a/src/driver.h b/src/driver.h
  index cc03e9f..b416902 100644
  --- a/src/driver.h
  +++ b/src/driver.h
  @@ -1801,6 +1801,9 @@ typedef int
void *opaque);
   
   typedef int
  +(*virDrvStateAutoStart)(void);
  +

Actually, why not just make this 'void' return value. None of the
impls return any error code


  @@ -836,6 +840,23 @@ int virStateInitialize(bool privileged,
   }
   }
   }
  +
  +for (i = 0; i  virStateDriverTabCount; i++) {
  +if (virStateDriverTab[i]-stateAutoStart) {
  +VIR_DEBUG(Running global auto start for %s state driver,
  +  virStateDriverTab[i]-name);
  +if (virStateDriverTab[i]-stateAutoStart()  0) {
  +virErrorPtr err = virGetLastError();
  +VIR_ERROR(_(Auto start for %s driver failed: %s),
  +  virStateDriverTab[i]-name,
  +  err  err-message ? err-message :
  +_(Unknown problem));
  +if (virStateDriverTab[i]-stateCleanup)
  +ignore_value(virStateDriverTab[i]-stateCleanup());
  +return -1;
  +}

...then all this error handling goes away.

 
 ACK
 

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

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

Re: [libvirt] [PATCH 2/2] virStateDriver - Separate AutoStart from Initialize

2013-07-25 Thread Daniel P. Berrange 
On Thu, Jul 25, 2013 at 02:32:57PM -0400, John Ferlan wrote:
 Adjust these drivers to handle their Autostart functionality after each
 of the drivers has gone through their Initialization functions
 
 merge
 ---
  src/libxl/libxl_driver.c | 18 +++---
  src/lxc/lxc_driver.c | 18 --
  src/network/bridge_driver.c  | 20 +++-
  src/qemu/qemu_driver.c   | 18 --
  src/storage/storage_driver.c | 19 ++-
  src/uml/uml_driver.c | 18 --
  6 files changed, 100 insertions(+), 11 deletions(-)
 
 diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
 index 98b1985..3dc8b32 100644
 --- a/src/libxl/libxl_driver.c
 +++ b/src/libxl/libxl_driver.c
 @@ -1348,9 +1348,6 @@ libxlStateInitialize(bool privileged,
 NULL, NULL)  0)
  goto error;
  
 -virDomainObjListForEach(libxl_driver-domains, libxlAutostartDomain,
 -libxl_driver);
 -
  virDomainObjListForEach(libxl_driver-domains, 
 libxlDomainManagedSaveLoad,
  libxl_driver);
  
 @@ -1369,6 +1366,20 @@ fail:
  }
  
  static int
 +libxlStateAutoStart(void)
 +{
 +if (!libxl_driver)
 +return 0;
 +
 +libxlDriverLock(libxl_driver);
 +virDomainObjListForEach(libxl_driver-domains, libxlAutostartDomain,
 +libxl_driver);
 +libxlDriverUnlock(libxl_driver);
 +
 +return 0;
 +}
 +
 +static int
  libxlStateReload(void)
  {
  if (!libxl_driver)
 @@ -4887,6 +4898,7 @@ static virDriver libxlDriver = {
  static virStateDriver libxlStateDriver = {
  .name = LIBXL,
  .stateInitialize = libxlStateInitialize,
 +.stateAutoStart = libxlStateAutoStart,
  .stateCleanup = libxlStateCleanup,
  .stateReload = libxlStateReload,
  };
 diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
 index 21cf2e3..75c03e0 100644
 --- a/src/lxc/lxc_driver.c
 +++ b/src/lxc/lxc_driver.c
 @@ -1455,8 +1455,6 @@ static int lxcStateInitialize(bool privileged,
 NULL, NULL)  0)
  goto cleanup;
  
 -virLXCProcessAutostartAll(lxc_driver);
 -
  virNWFilterRegisterCallbackDriver(lxcCallbackDriver);
  return 0;
  
 @@ -1466,6 +1464,21 @@ cleanup:
  return -1;
  }
  
 +/**
 + * lxcStateAutoStart:
 + *
 + * Function to autostart the LXC daemons
 + */
 +static int lxcStateAutoStart(void)
 +{
 +if (!lxc_driver)
 +return 0;
 +
 +virLXCProcessAutostartAll(lxc_driver);
 +
 +return 0;
 +}
 +
  static void lxcNotifyLoadDomain(virDomainObjPtr vm, int newVM, void *opaque)
  {
  virLXCDriverPtr driver = opaque;
 @@ -4665,6 +4678,7 @@ static virDriver lxcDriver = {
  static virStateDriver lxcStateDriver = {
  .name = LXC_DRIVER_NAME,
  .stateInitialize = lxcStateInitialize,
 +.stateAutoStart = lxcStateAutoStart,
  .stateCleanup = lxcStateCleanup,
  .stateReload = lxcStateReload,
  };
 diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
 index 0bb57ea..98dea9e 100644
 --- a/src/network/bridge_driver.c
 +++ b/src/network/bridge_driver.c
 @@ -430,7 +430,6 @@ networkStateInitialize(bool privileged,
  networkFindActiveConfigs(driverState);
  networkReloadFirewallRules(driverState);
  networkRefreshDaemons(driverState);
 -networkAutostartConfigs(driverState);
  
  networkDriverUnlock(driverState);
  
 @@ -474,6 +473,24 @@ error:
  }
  
  /**
 + * networkStateAutoStart:
 + *
 + * Function to AutoStart the bridge configs
 + */
 +static int
 +networkStateAutoStart(void)
 +{
 +if (!driverState)
 +return 0;
 +
 +networkDriverLock(driverState);
 +networkAutostartConfigs(driverState);
 +networkDriverUnlock(driverState);
 +
 +return 0;
 +}
 +
 +/**
   * networkStateReload:
   *
   * Function to restart the QEmu daemon, it will recheck the configuration
 @@ -3693,6 +3710,7 @@ static virNetworkDriver networkDriver = {
  static virStateDriver networkStateDriver = {
  .name = Network,
  .stateInitialize  = networkStateInitialize,
 +.stateAutoStart  = networkStateAutoStart,
  .stateCleanup = networkStateCleanup,
  .stateReload = networkStateReload,
  };
 diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
 index 2adf6e2..ec909b3 100644
 --- a/src/qemu/qemu_driver.c
 +++ b/src/qemu/qemu_driver.c
 @@ -837,8 +837,6 @@ qemuStateInitialize(bool privileged,
  if (!qemu_driver-workerPool)
  goto error;
  
 -qemuAutostartDomains(qemu_driver);
 -
  if (conn)
  virConnectClose(conn);
  
 @@ -855,6 +853,21 @@ error:
  return -1;
  }
  
 +/**
 + * qemuStateAutoStart:
 + *
 + * Function to auto start the QEmu daemons
 + */
 +static int
 +qemuStateAutoStart(void)
 +{
 +if (!qemu_driver)
 +return 0;
 +
 +qemuAutostartDomains(qemu_driver);
 +return 0;
 +}
 +
  static void qemuNotifyLoadDomain(virDomainObjPtr vm,

Re: [libvirt] [PATCH 1/5] Introduce a more convenient virCgroupNewDetectMachine

2013-07-25 Thread Eric Blake 
On 07/25/2013 12:11 PM, Daniel P. Berrange wrote:

  
 +/*
 + * Returns 0 on success, -1 on fatal error, -2 on no valid cgroup
 + */
 +int virCgroupNewDetectMachine(const char *name,

 +
 +if (!virCgroupIsValidMachineGroup(*group, name, drivername)) {
 +virCgroupFree(group);
 +return 0;

 Huh? This says you are returning success.  Also, none of the lxc or qemu
 callers checked for a -2 return; do you really need the differentiated
 return type?
 
 Opps the comment is wrong. I originally had it returning -2, but I
 removed that and just useed '0' and let the caller check if 'group != NULL'
 instead.

Ah, then ACK with a fixed doc comment that describes the real convention.

-- 
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] Entering freeze for libvirt-1.1.1

2013-07-25 Thread Jason Helfman 
On Thu, Jul 25, 2013 at 9:50 AM, Roman Bogorodskiy bogorods...@gmail.comwrote:

   Jason Helfman wrote:

  Configure failure on BSD when detecting libssh2. This is the first time I
  have run into this. Has anything changed?
 
  configure: error: You must install the libssh2 = 1.3pkg-config module to
  compile libvirt
  ===  Script configure failed unexpectedly.
  Please report the problem to j...@freebsd.org [maintainer] and attach the
 
 /usr/home/jgh/workspace/ports/devel/libvirt/work/libvirt-1.1.1/config.log
  including the output of the failure of your make command. Also, it might
 be
  a good idea to provide an overview of all packages installed on your
 system
  (e.g. a /usr/local/sbin/pkg-static info -g -Ea).
  *** [do-configure] Error code 1
 
  Stop in /usr/home/jgh/workspace/ports/devel/libvirt.
  *** [build] Error code 1
 
  Stop in /usr/home/jgh/workspace/ports/devel/libvirt.
  [jgh@hatter ~/workspace/ports/devel/libvirt]$ pkg info -l libssh2 |grep
 pc
  /usr/local/libdata/pkgconfig/libssh2.pc
 
  -jgh

 Never tried to build with libssh2. Just downloaded rc1 now and did:

 ./configure --with-ssh2

 and it worked fine for me:

 configure:   ssh2: yes (CFLAGS='-I/usr/local/include  '
 LIBS='-L/usr/local/lib -lssh2  -Wl,-rpath=/usr/lib:/usr/local/lib  ')

 Could you provide config.log?

 Roman Bogorodskiy


The port builds by default with libssh2, and this is this first issue there
has been with it.

config.log = http://people.freebsd.org/~jgh/files/config.log
libvirt.shar port = http://people.freebsd.org/~jgh/files/libvirt.shar

I added PKG_CONFIG_PATH to CONFIGURE_ARGS to attempt to solve the build
issue, as this was noted in ./configure help and was also noted in
config.log

Thanks!
-jgh

-- 
Jason Helfman  | FreeBSD Committer
j...@freebsd.org | http://people.freebsd.org/~jgh  | The Power to Serve
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 2/2] virStateDriver - Separate AutoStart from Initialize

2013-07-25 Thread John Ferlan 
On 07/25/2013 02:41 PM, Daniel P. Berrange wrote:
 On Thu, Jul 25, 2013 at 02:32:57PM -0400, John Ferlan wrote:
 Adjust these drivers to handle their Autostart functionality after each
 of the drivers has gone through their Initialization functions

 merge
 ---
  src/libxl/libxl_driver.c | 18 +++---
  src/lxc/lxc_driver.c | 18 --
  src/network/bridge_driver.c  | 20 +++-
  src/qemu/qemu_driver.c   | 18 --
  src/storage/storage_driver.c | 19 ++-
  src/uml/uml_driver.c | 18 --
  6 files changed, 100 insertions(+), 11 deletions(-)

 diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
 index 98b1985..3dc8b32 100644
 --- a/src/libxl/libxl_driver.c
 +++ b/src/libxl/libxl_driver.c
 @@ -1348,9 +1348,6 @@ libxlStateInitialize(bool privileged,
 NULL, NULL)  0)
  goto error;
  
 -virDomainObjListForEach(libxl_driver-domains, libxlAutostartDomain,
 -libxl_driver);
 -
  virDomainObjListForEach(libxl_driver-domains, 
 libxlDomainManagedSaveLoad,
  libxl_driver);
  
 @@ -1369,6 +1366,20 @@ fail:
  }
  
  static int
 +libxlStateAutoStart(void)
 +{
 +if (!libxl_driver)
 +return 0;
 +
 +libxlDriverLock(libxl_driver);
 +virDomainObjListForEach(libxl_driver-domains, libxlAutostartDomain,
 +libxl_driver);
 +libxlDriverUnlock(libxl_driver);
 +
 +return 0;
 +}
 +
 +static int
  libxlStateReload(void)
  {
  if (!libxl_driver)
 @@ -4887,6 +4898,7 @@ static virDriver libxlDriver = {
  static virStateDriver libxlStateDriver = {
  .name = LIBXL,
  .stateInitialize = libxlStateInitialize,
 +.stateAutoStart = libxlStateAutoStart,
  .stateCleanup = libxlStateCleanup,
  .stateReload = libxlStateReload,
  };
 diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
 index 21cf2e3..75c03e0 100644
 --- a/src/lxc/lxc_driver.c
 +++ b/src/lxc/lxc_driver.c
 @@ -1455,8 +1455,6 @@ static int lxcStateInitialize(bool privileged,
 NULL, NULL)  0)
  goto cleanup;
  
 -virLXCProcessAutostartAll(lxc_driver);
 -
  virNWFilterRegisterCallbackDriver(lxcCallbackDriver);
  return 0;
  
 @@ -1466,6 +1464,21 @@ cleanup:
  return -1;
  }
  
 +/**
 + * lxcStateAutoStart:
 + *
 + * Function to autostart the LXC daemons
 + */
 +static int lxcStateAutoStart(void)
 +{
 +if (!lxc_driver)
 +return 0;
 +
 +virLXCProcessAutostartAll(lxc_driver);
 +
 +return 0;
 +}
 +
  static void lxcNotifyLoadDomain(virDomainObjPtr vm, int newVM, void *opaque)
  {
  virLXCDriverPtr driver = opaque;
 @@ -4665,6 +4678,7 @@ static virDriver lxcDriver = {
  static virStateDriver lxcStateDriver = {
  .name = LXC_DRIVER_NAME,
  .stateInitialize = lxcStateInitialize,
 +.stateAutoStart = lxcStateAutoStart,
  .stateCleanup = lxcStateCleanup,
  .stateReload = lxcStateReload,
  };
 diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
 index 0bb57ea..98dea9e 100644
 --- a/src/network/bridge_driver.c
 +++ b/src/network/bridge_driver.c
 @@ -430,7 +430,6 @@ networkStateInitialize(bool privileged,
  networkFindActiveConfigs(driverState);
  networkReloadFirewallRules(driverState);
  networkRefreshDaemons(driverState);
 -networkAutostartConfigs(driverState);
  
  networkDriverUnlock(driverState);
  
 @@ -474,6 +473,24 @@ error:
  }
  
  /**
 + * networkStateAutoStart:
 + *
 + * Function to AutoStart the bridge configs
 + */
 +static int
 +networkStateAutoStart(void)
 +{
 +if (!driverState)
 +return 0;
 +
 +networkDriverLock(driverState);
 +networkAutostartConfigs(driverState);
 +networkDriverUnlock(driverState);
 +
 +return 0;
 +}
 +
 +/**
   * networkStateReload:
   *
   * Function to restart the QEmu daemon, it will recheck the configuration
 @@ -3693,6 +3710,7 @@ static virNetworkDriver networkDriver = {
  static virStateDriver networkStateDriver = {
  .name = Network,
  .stateInitialize  = networkStateInitialize,
 +.stateAutoStart  = networkStateAutoStart,
  .stateCleanup = networkStateCleanup,
  .stateReload = networkStateReload,
  };
 diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
 index 2adf6e2..ec909b3 100644
 --- a/src/qemu/qemu_driver.c
 +++ b/src/qemu/qemu_driver.c
 @@ -837,8 +837,6 @@ qemuStateInitialize(bool privileged,
  if (!qemu_driver-workerPool)
  goto error;
  
 -qemuAutostartDomains(qemu_driver);
 -
  if (conn)
  virConnectClose(conn);
  
 @@ -855,6 +853,21 @@ error:
  return -1;
  }
  
 +/**
 + * qemuStateAutoStart:
 + *
 + * Function to auto start the QEmu daemons
 + */
 +static int
 +qemuStateAutoStart(void)
 +{
 +if (!qemu_driver)
 +return 0;
 +
 +qemuAutostartDomains(qemu_driver);
 +return 0;
 +}
 +
  static

Re: [libvirt] [PATCH] virt-login-shell joins users into lxc container.

2013-07-25 Thread Eric Blake 
On 07/20/2013 05:46 AM, dwa...@redhat.com wrote:
 From: Dan Walsh dwa...@redhat.com
 
 Openshift wants to have their gears stuck into a container when they login
 to the system.  virt-login-shell will join a running gear with the username of
 the person running it, or attempt to start the container if it is not running.
 (Currently containers do not exist if they are not running, so I can not test
 this feature. But the code is there).
 
 This tool needs to be setuid since joining a container (nsjoin) requires 
 privs.
 The root user is not allowed to execute this command. When this tool is
 run by a normal user it will only join the users container.
 
 Only users who are listed as valid_users in /etc/libvirt/virt-login-shell.conf
 are allowed to join containers using this tool. By default no users are 
 allowed.
 ---

I'm afraid this needs another version, and thus I'm reluctant to take it
into 1.1.1 if we can't get it fixed quickly.

 +++ b/mingw-libvirt.spec.in

 @@ -246,9 +249,11 @@ rm -rf 
 $RPM_BUILD_ROOT%{mingw64_libexecdir}/libvirt-guests.sh
  %files -n mingw64-libvirt
  %dir %{mingw64_sysconfdir}/libvirt/
  %config(noreplace) %{mingw64_sysconfdir}/libvirt/libvirt.conf
 +%config(noreplace) %{_sysconfdir}/libvirt/virt-login-shell.conf

This probably needs to be %{mingw64_sysconfdir}.  For that matter, does
virt-login-shell even make sense on mingw, or is it a Linux-only concept
where the mingw .spec file should be ensuring that it is not compiled
nor installed?

  
  %{mingw64_bindir}/libvirt-0.dll
  %{mingw64_bindir}/virsh.exe
 +%{mingw64_bindir}/virt-login-shell.exe

In other words, is virt-login-shell.exe even capable of doing anything
useful on mingw?

 +++ b/src/libvirt_private.syms
 @@ -2026,6 +2026,7 @@ virGetUnprivSGIOSysfsPath;
  virGetUserCacheDirectory;
  virGetUserConfigDirectory;
  virGetUserDirectory;
 +virGetUserDirectoryByUID;

This looks like an independently useful function; it would be worth
splitting this patch into a series where patch 1 adds the new function,
and patch 2 takes advantage of it, rather than mixing it all into one patch.

 +char *virGetUserDirectoryByUID(uid_t uid)
 +{
 +char *ret;
 +virGetUserEnt(uid, NULL, NULL, ret);
 +return ret;
 +}

Is it worth rewriting the existing:

char *virGetUserDirectory(void)
{
return virGetUserDirectoryByUID(geteuid());
}

Doing so would give us instant code coverage of the new function.


  bin_SCRIPTS = virt-xml-validate virt-pki-validate
 -bin_PROGRAMS = virsh virt-host-validate
 +bin_PROGRAMS = virsh virt-host-validate virt-login-shell

Since the .spec file is installing this file as setuid, shouldn't 'make
install' attempt to do so likewise (that is, do more than just the
normal reliance on automake's bin_PROGRAMS, which doesn't set special
mode bits)?  Or is setuid something rare enough that only spec files
need to be requesting it?  I'm thinking about the case of someone that
does './configure --prefix=$HOME' with the intent of installing local
tools - will virt-login-shell be usable by such a user?

 +++ b/tools/virt-login-shell.c
 @@ -0,0 +1,312 @@
 +/*
 + * virt-login-shell.c: a shell to connect to a container
 + *

 +#include config.h
 +#include virconf.h
 +#include virutil.h
 +#include virfile.h
 +#include virprocess.h
 +#include configmake.h
 +#include virstring.h
 +#include viralloc.h
 +#include vircommand.h
 +
 +#include stdarg.h
 +#include stdio.h
 +#include errno.h
 +#include stdlib.h
 +#include fnmatch.h

We tend to list system headers before  headers; but it shouldn't be a
problem that you did it the other way around.

 +#define VIR_FROM_THIS VIR_FROM_NONE
 +
 +static ssize_t nfdlist = 0;
 +static int *fdlist = NULL;

static variables are already 0-initialized without needing an explicit
initializer; while gcc is smart enough to optimize explicit zero
optimization into using the bss, not all compilers are that good.

 +
 +static void virLoginShellFini(virConnectPtr conn, virDomainPtr  dom) {

unnecessary doubled space

Style nit: we prefer:

static void
virLoginShellFini(virConnectPtr conn, virDomainPtr dom)
{

 +size_t i;

Style nit: we prefer blank line between local variable declarations and
expressions.

 +for (i=0; i  nfdlist; i++)

Style nit: we prefer spaces around '='.

 +VIR_FORCE_CLOSE(fdlist[i]);
 +VIR_FREE(fdlist);

This looks like leak prevention (good so that tools like valgrind don't
complain)...

 +nfdlist = 0;

...whereas this may be dead code - if we are not supposed to use
anything after virLoginShellFini() has been called (because we are about
to exit or exec), then cleaning things back to a zero-initialized state
is wasted cycles.  On the other hand, being defensive against reuse
can't hurt, and as this will be setuid, paranoia is okay.

 +if (dom)
 +virDomainFree(dom);
 +if (conn)
 +virConnectClose(conn);
 +}
 +
 +static int virLoginShellAllowedUser(virConfPtr conf,
 +uid_t uid,
 +

[libvirt] [PATCH] Fix virsh snapshot-list error reporting

2013-07-25 Thread Jim Fehlig 
Noticed that the expected not supported error is dropped when
invoking 'virsh snapshot-list dom' on a Xen installation running
the libxl driver

 virsh snapshot-list test
 error: Invalid snapshot: virDomainSnapshotFree

The error is overwritten by a call to virDomainSnapshotFree
in cleanup code within cmdSnapshotList.  Prevent overwritting
the real error by not calling virDomainSnapshotFree with a NULL
virDomainSnapshotPtr.
---

This is one possible fix for the bug.  The other would be to
silently return in virDomainSnapshotFree on a NULL
virDomainSnapshotPtr.

 tools/virsh-snapshot.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/virsh-snapshot.c b/tools/virsh-snapshot.c
index cfe8ee9..db9715b 100644
--- a/tools/virsh-snapshot.c
+++ b/tools/virsh-snapshot.c
@@ -1678,7 +1678,8 @@ cleanup:
 vshSnapshotListFree(snaplist);
 VIR_FREE(parent_snap);
 VIR_FREE(state);
-virDomainSnapshotFree(start);
+if (start)
+virDomainSnapshotFree(start);
 xmlXPathFreeContext(ctxt);
 xmlFreeDoc(xml);
 VIR_FREE(doc);
-- 
1.8.1.4

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

Re: [libvirt] [PATCH] Fix virsh snapshot-list error reporting

2013-07-25 Thread Eric Blake 
On 07/25/2013 02:38 PM, Jim Fehlig wrote:
 Noticed that the expected not supported error is dropped when
 invoking 'virsh snapshot-list dom' on a Xen installation running
 the libxl driver
 
  virsh snapshot-list test
  error: Invalid snapshot: virDomainSnapshotFree
 
 The error is overwritten by a call to virDomainSnapshotFree
 in cleanup code within cmdSnapshotList.  Prevent overwritting
 the real error by not calling virDomainSnapshotFree with a NULL
 virDomainSnapshotPtr.

Indeed.

 ---
 
 This is one possible fix for the bug.  The other would be to
 silently return in virDomainSnapshotFree on a NULL
 virDomainSnapshotPtr.

No, that would be an incompatible behavior change - our API promises a
failure on attempting to free a null object, and people have come to
rely on that.

 +++ b/tools/virsh-snapshot.c
 @@ -1678,7 +1678,8 @@ cleanup:
  vshSnapshotListFree(snaplist);
  VIR_FREE(parent_snap);
  VIR_FREE(state);
 -virDomainSnapshotFree(start);
 +if (start)
 +virDomainSnapshotFree(start);

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] [PATCH] build: avoid -lgcrypt with newer gnutls

2013-07-25 Thread Eric Blake 
https://bugzilla.redhat.com/show_bug.cgi?id=951637

Newer gnutls uses nettle, rather than gcrypt, which is a lot nicer
regarding initialization.  Yet we were unconditionally initializing
gcrypt even when gnutls wouldn't be using it, and having two crypto
libraries linked into libvirt.so is pointless.

The ldd probe in configure borrows from our libnl-1 vs. libnl-3 code.

* configure.ac (WITH_GNUTLS): Probe whether to add -lgcrypt, and
define a witness WITH_GNUTLS_GCRYPT.
* src/libvirt.c (virTLSMutexInit, virTLSMutexDestroy)
(virTLSMutexLock, virTLSMutexUnlock, virTLSThreadImpl)
(virGlobalInit): Honor the witness.
* libvirt.spec.in (BuildRequires): Make gcrypt usage conditional,
no longer needed in Fedora 19.

Signed-off-by: Eric Blake ebl...@redhat.com
---

Tested with 'ldd src/.libs/libvirt.so | grep -E (gcry|net|tls)':
- on RHEL 6.4 and Fedora 18, pre- and post-patch remain unchanged
(use of just libgnutls/libgcrypt)
- on Fedora 19, pre-patch linked against libgnutls, libgcrypt, and
libnettle, post-patch linked against just libgnutls and libnettle

This should probably go in for 1.1.1, but it's not a build-breaker
so it needs review.

 configure.ac| 27 +--
 libvirt.spec.in |  2 ++
 src/libvirt.c   | 10 ++
 3 files changed, 29 insertions(+), 10 deletions(-)

diff --git a/configure.ac b/configure.ac
index cc9942a..de209e2 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1098,13 +1098,28 @@ if test x$with_gnutls != xno; then
   AC_MSG_ERROR([You must install the GnuTLS library in order to compile 
and run libvirt])
 fi
   else
-dnl Not all versions of gnutls include -lgcrypt, and so we add
-dnl it explicitly for the calls to gcry_control/check_version
-GNUTLS_LIBS=$GNUTLS_LIBS -lgcrypt

-dnl We're not using gcrypt deprecated features so define
-dnl GCRYPT_NO_DEPRECATED to avoid deprecated warnings
-GNUTLS_CFLAGS=$GNUTLS_CFLAGS -DGCRYPT_NO_DEPRECATED
+dnl If gnutls linked against -lgcrypt, then we must initialize gcrypt
+dnl prior to using gnutls.  Newer versions of gnutls use -lnettle, in
+dnl which case we don't want to drag in gcrypt ourselves.
+gnutls_ldd=
+for dir in /usr/lib64 /usr/lib /usr/lib/*-linux-gnu*; do
+if test -f $dir/libgnutls.so; then
+gnutls_ldd=`(ldd $dir/libgnutls.so) 21`
+break
+fi
+done
+case $gnutls_ldd in
+'' | *libgcrypt.so*)
+GNUTLS_LIBS=$GNUTLS_LIBS -lgcrypt
+   dnl We're not using gcrypt deprecated features so define
+dnl GCRYPT_NO_DEPRECATED to avoid deprecated warnings
+GNUTLS_CFLAGS=$GNUTLS_CFLAGS -DGCRYPT_NO_DEPRECATED
+   AC_DEFINE_UNQUOTED([WITH_GNUTLS_GCRYPT], 1,
+  [whether GNUTLS uses gcrypt])
+   ;;
+*) ;; # Assume no gcrypt usage
+esac

 dnl gnutls 3.x moved some declarations to a new header
 AC_CHECK_HEADERS([gnutls/crypto.h], [], [], [[
diff --git a/libvirt.spec.in b/libvirt.spec.in
index e0e0004..4320281 100644
--- a/libvirt.spec.in
+++ b/libvirt.spec.in
@@ -422,7 +422,9 @@ BuildRequires: readline-devel
 BuildRequires: ncurses-devel
 BuildRequires: gettext
 BuildRequires: libtasn1-devel
+%if (0%{?rhel}  0%{?rhel}  7) || (0%{?fedora}  0%{?fedora}  19)
 BuildRequires: libgcrypt-devel
+%endif
 BuildRequires: gnutls-devel
 BuildRequires: libattr-devel
 %if %{with_libvirtd}
diff --git a/src/libvirt.c b/src/libvirt.c
index 444c1c3..9775b97 100644
--- a/src/libvirt.c
+++ b/src/libvirt.c
@@ -55,7 +55,9 @@
 #include intprops.h
 #include virconf.h
 #if WITH_GNUTLS
-# include gcrypt.h
+# if WITH_GNUTLS_GCRYPT
+#  include gcrypt.h
+# endif
 # include rpc/virnettlscontext.h
 #endif
 #include vircommand.h
@@ -270,7 +272,7 @@ winsock_init(void)
 #endif


-#ifdef WITH_GNUTLS
+#ifdef WITH_GNUTLS_GCRYPT
 static int virTLSMutexInit(void **priv)
 {
 virMutexPtr lock = NULL;
@@ -323,7 +325,7 @@ static struct gcry_thread_cbs virTLSThreadImpl = {
 virTLSMutexUnlock,
 NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL
 };
-#endif
+#endif /* WITH_GNUTLS_GCRYPT */

 /* Helper macros to implement VIR_DOMAIN_DEBUG using just C99.  This
  * assumes you pass fewer than 15 arguments to VIR_DOMAIN_DEBUG, but
@@ -407,7 +409,7 @@ virGlobalInit(void)
 virErrorInitialize()  0)
 goto error;

-#ifdef WITH_GNUTLS
+#ifdef WITH_GNUTLS_GCRYPT
 /*
  * This sequence of API calls it copied exactly from
  * gnutls 2.12.23 source lib/gcrypt/init.c, with
-- 
1.8.3.1

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

Re: [libvirt] [PATCH 1/7] util: improve user lookup helper

2013-07-25 Thread Cole Robinson 
On 07/23/2013 11:03 AM, Eric Blake wrote:
 https://bugzilla.redhat.com/show_bug.cgi?id=964358
 
 A future patch needs to look up pw_gid; but it is wasteful
 to crawl through getpwuid_r twice for two separate pieces
 of information, and annoying to copy that much boilerplate
 code for doing the crawl.  The current internal-only
 virGetUserEnt is also a rather awkward interface; it's easier
 to just design it to let callers request multiple pieces of
 data as needed from one traversal.
 
 And while at it, I noticed that virGetXDGDirectory could deref
 NULL if the getpwuid_r lookup fails.
 
 * src/util/virutil.c (virGetUserEnt): Alter signature.
 (virGetUserDirectory, virGetXDGDirectory, virGetUserName): Adjust
 callers.
 
 Signed-off-by: Eric Blake ebl...@redhat.com
 (cherry picked from commit c1983ba4e3902308054e961fcae75cece73ef4ba)
 
 Conflicts:
   src/util/virutil.c - oom reporting/strdup changes not backported
 ---
  src/util/util.c | 60 
 ++---
  1 file changed, 36 insertions(+), 24 deletions(-)
 

ACK

- Cole

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

Re: [libvirt] [PATCH 2/7] util: add virGetGroupList

2013-07-25 Thread Cole Robinson 
On 07/23/2013 11:03 AM, Eric Blake wrote:
 https://bugzilla.redhat.com/show_bug.cgi?id=964358
 
 Since neither getpwuid_r() nor initgroups() are safe to call in
 between fork and exec (they obtain a mutex, but if some other
 thread in the parent also held the mutex at the time of the fork,
 the child will deadlock), we have to split out the functionality
 that is unsafe.  At least glibc's initgroups() uses getgrouplist
 under the hood, so the ideal split is to expose getgrouplist for
 use before a fork.  Gnulib already gives us a nice wrapper via
 mgetgroups; we wrap it once more to look up by uid instead of name.
 
 * bootstrap.conf (gnulib_modules): Add mgetgroups.
 * src/util/virutil.h (virGetGroupList): New declaration.
 * src/util/virutil.c (virGetGroupList): New function.
 * src/libvirt_private.syms (virutil.h): Export it.
 
 Signed-off-by: Eric Blake ebl...@redhat.com
 (cherry picked from commit 75c125641ac73473ba4b0542524d67a184769c8e)
 
 Conflicts:
   bootstrap.conf - not updating gnulib submodule...
   configure.ac - ...so checking for getgrouplist by hand...
   src/util/virutil.c - ...and copying only the getgrouplist 
 implementation rather than calling the gnulib function; also, file still 
 named util.c
   src/libvirt_private.syms - context

ACK

- Cole

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

Re: [libvirt] [PATCH 3/7] util: make virSetUIDGID async-signal-safe

2013-07-25 Thread Cole Robinson 
On 07/23/2013 11:03 AM, Eric Blake wrote:
 https://bugzilla.redhat.com/show_bug.cgi?id=964358
 
 POSIX states that multi-threaded apps should not use functions
 that are not async-signal-safe between fork and exec, yet we
 were using getpwuid_r and initgroups.  Although rare, it is
 possible to hit deadlock in the child, when it tries to grab
 a mutex that was already held by another thread in the parent.
 I actually hit this deadlock when testing multiple domains
 being started in parallel with a command hook, with the following
 backtrace in the child:
 
  Thread 1 (Thread 0x7fd56bbf2700 (LWP 3212)):
  #0  __lll_lock_wait ()
  at ../nptl/sysdeps/unix/sysv/linux/x86_64/lowlevellock.S:136
  #1  0x7fd5761e7388 in _L_lock_854 () from /lib64/libpthread.so.0
  #2  0x7fd5761e7257 in __pthread_mutex_lock (mutex=0x7fd56be00360)
  at pthread_mutex_lock.c:61
  #3  0x7fd56bbf9fc5 in _nss_files_getpwuid_r (uid=0, 
 result=0x7fd56bbf0c70,
  buffer=0x7fd55c2a65f0 , buflen=1024, errnop=0x7fd56bbf25b8)
  at nss_files/files-pwd.c:40
  #4  0x7fd575aeff1d in __getpwuid_r (uid=0, resbuf=0x7fd56bbf0c70,
  buffer=0x7fd55c2a65f0 , buflen=1024, result=0x7fd56bbf0cb0)
  at ../nss/getXXbyYY_r.c:253
  #5  0x7fd578aebafc in virSetUIDGID (uid=0, gid=0) at util/virutil.c:1031
  #6  0x7fd578aebf43 in virSetUIDGIDWithCaps (uid=0, gid=0, capBits=0,
  clearExistingCaps=true) at util/virutil.c:1388
  #7  0x7fd578a9a20b in virExec (cmd=0x7fd55c231f10) at 
 util/vircommand.c:654
  #8  0x7fd578a9dfa2 in virCommandRunAsync (cmd=0x7fd55c231f10, pid=0x0)
  at util/vircommand.c:2247
  #9  0x7fd578a9d74e in virCommandRun (cmd=0x7fd55c231f10, exitstatus=0x0)
  at util/vircommand.c:2100
  #10 0x7fd56326fde5 in qemuProcessStart (conn=0x7fd53c000df0,
  driver=0x7fd55c0dc4f0, vm=0x7fd54800b100, migrateFrom=0x0, stdin_fd=-1,
  stdin_path=0x0, snapshot=0x0, vmop=VIR_NETDEV_VPORT_PROFILE_OP_CREATE,
  flags=1) at qemu/qemu_process.c:3694
  ...
 
 The solution is to split the work of getpwuid_r/initgroups into the
 unsafe portions (getgrouplist, called pre-fork) and safe portions
 (setgroups, called post-fork).
 
 * src/util/virutil.h (virSetUIDGID, virSetUIDGIDWithCaps): Adjust
 signature.
 * src/util/virutil.c (virSetUIDGID): Add parameters.
 (virSetUIDGIDWithCaps): Adjust clients.
 * src/util/vircommand.c (virExec): Likewise.
 * src/util/virfile.c (virFileAccessibleAs, virFileOpenForked)
 (virDirCreate): Likewise.
 * src/security/security_dac.c (virSecurityDACSetProcessLabel):
 Likewise.
 * src/lxc/lxc_container.c (lxcContainerSetID): Likewise.
 * configure.ac (AC_CHECK_FUNCS_ONCE): Check for setgroups, not
 initgroups.
 
 Signed-off-by: Eric Blake ebl...@redhat.com
 (cherry picked from commit ee777e994927ed5f2d427fbc5a53cbe8b5969bda)
 
 Conflicts:
   src/lxc/lxc_container.c - did not use setUIDGID before 1.1.0
   src/util/virutil.c - oom handling changes not backported; no 
 virSetUIDGIDWithCaps
   src/util/virfile.c - functions still lived in virutil.c this far back
   configure.ac - context with previous commit
   src/util/command.c - no UID/GID handling in vircommand.c...
   src/storage/storage_backend.c - ...so do it in the one hook user instead

ACK

- Cole

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

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

2013-07-25 Thread Cole Robinson 
On 07/23/2013 11:03 AM, Eric Blake wrote:
 From: Daniel P. Berrange berra...@redhat.com
 
 https://bugzilla.redhat.com/show_bug.cgi?id=964358
 
 The hook scripts used by virCommand must be careful wrt
 accessing any mutexes that may have been held by other
 threads in the parent process. With the recent refactoring
 there are 2 potential flaws lurking, which will become real
 deadlock bugs once the global QEMU driver lock is removed.
 
 Remove use of the QEMU driver lock from the hook function
 by passing in the 'virQEMUDriverConfigPtr' instance directly.
 
 Add functions to the virSecurityManager to be invoked before
 and after fork, to ensure the mutex is held by the current
 thread. This allows it to be safely used in the hook script
 in the child process.
 
 Signed-off-by: Daniel P. Berrange berra...@redhat.com
 (cherry picked from commit 61b52d2e3813cc8c9ff3ab67f232bd0c65f7318d)
 
 Conflicts:
   src/libvirt_private.syms - context
   src/qemu/qemu_process.c - no backport of qemud_driver struct rename
   src/security/security_manager.c - no backport of making the security 
 driver self-locking; just expose the interface

ACK

- Cole

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

Re: [libvirt] [PATCH 6/7] security_dac: compute supplemental groups before fork

2013-07-25 Thread Cole Robinson 
On 07/23/2013 11:03 AM, Eric Blake wrote:
 https://bugzilla.redhat.com/show_bug.cgi?id=964358
 
 Commit 75c1256 states that virGetGroupList must not be called
 between fork and exec, then commit ee777e99 promptly violated
 that for lxc's use of virSecurityManagerSetProcessLabel.  Hoist
 the supplemental group detection to the time that the security
 manager needs to fork.  Qemu is safe, as it uses
 virSecurityManagerSetChildProcessLabel which in turn uses
 virCommand to determine supplemental groups.
 
 This does not fix the fact that virSecurityManagerSetProcessLabel
 calls virSecurityDACParseIds calls parseIds which eventually
 calls getpwnam_r, which also violates fork/exec async-signal-safe
 safety rules, but so far no one has complained of hitting
 deadlock in that case.
 
 * src/security/security_dac.c (_virSecurityDACData): Track groups
 in private data.
 (virSecurityDACPreFork): New function, to set them.
 (virSecurityDACClose): Clean up new fields.
 (virSecurityDACGetIds): Alter signature.
 (virSecurityDACSetSecurityHostdevLabelHelper)
 (virSecurityDACSetChardevLabel, virSecurityDACSetProcessLabel)
 (virSecurityDACSetChildProcessLabel): Update callers.
 
 Signed-off-by: Eric Blake ebl...@redhat.com
 (cherry picked from commit 29fe5d745fbe207ec2415441d4807ae76be05974)
 
 Conflicts:
   src/security/security_dac.c - virSecurityDACSetSecurityUSBLabel needed 
 similar treatment; no virSecurityDACSetChildPrcessLabel

ACK

- Cole

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

Re: [libvirt] [PATCH 7/7] security: fix deadlock with prefork

2013-07-25 Thread Cole Robinson 
On 07/23/2013 11:04 AM, Eric Blake wrote:
 https://bugzilla.redhat.com/show_bug.cgi?id=964358
 
 Attempts to start a domain with both SELinux and DAC security
 modules loaded will deadlock; latent problem introduced in commit
 fdb3bde and exposed in commit 29fe5d7.  Basically, when recursing
 into the security manager for other driver's prefork, we have to
 undo the asymmetric lock taken at the manager level.
 
 Reported by Jiri Denemark, with diagnosis help from Dan Berrange.
 
 * src/security/security_stack.c (virSecurityStackPreFork): Undo
 extra lock grabbed during recursion.
 
 Signed-off-by: Eric Blake ebl...@redhat.com
 (cherry picked from commit bfc183c1e377b24cebf5cede4c00f3dc0d1b3486)
 ---
  src/security/security_stack.c | 5 +
  1 file changed, 5 insertions(+)
 
 diff --git a/src/security/security_stack.c b/src/security/security_stack.c
 index e8133c4..38fe8b5 100644
 --- a/src/security/security_stack.c
 +++ b/src/security/security_stack.c
 @@ -129,6 +129,11 @@ virSecurityStackPreFork(virSecurityManagerPtr mgr)
  rc = -1;
  break;
  }
 +/* Undo the unbalanced locking left behind after recursion; if
 + * PostFork ever delegates to driver callbacks, we'd instead
 + * need to recurse to an internal method that does not regrab
 + * a lock. */
 +virSecurityManagerPostFork(item-securityManager);
  }
 
  return rc;
 

ACK

- Cole

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

Re: [libvirt] [PATCH 0/7] backport of getGroupList to v0.10.2-maint

2013-07-25 Thread Cole Robinson 
On 07/23/2013 11:03 AM, Eric Blake wrote:
 https://bugzilla.redhat.com/show_bug.cgi?id=964358
 
 Since it was on Fedora 18 that I first noticed the deadlock possible
 when a child process calls getpwuid_r while the parent owned the
 lock in a different thread, I'm interested in backporting my recent
 work on virGetGroupList to v0.10.2-maint.  However, I hit enough
 conflict resolution that I'd like a review of my backport decisions
 before pushing this to the stable branch.
 

Thanks for doing this Eric! I've ACK'd each patch.

- Cole

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

Re: [libvirt] [PATCH 5/7] security: framework for driver PreFork handler

2013-07-25 Thread Cole Robinson 
On 07/23/2013 11:03 AM, Eric Blake wrote:
 https://bugzilla.redhat.com/show_bug.cgi?id=964358
 
 A future patch wants the DAC security manager to be able to safely
 get the supplemental group list for a given uid, but at the time
 of a fork rather than during initialization so as to pick up on
 live changes to the system's group database.  This patch adds the
 framework, including the possibility of a pre-fork callback
 failing.
 
 For now, any driver that implements a prefork callback must be
 robust against the possibility of being part of a security stack
 where a later element in the chain fails prefork.  This means
 that drivers cannot do any action that requires a call to postfork
 for proper cleanup (no grabbing a mutex, for example).  If this
 is too prohibitive in the future, we would have to switch to a
 transactioning sequence, where each driver has (up to) 3 callbacks:
 PreForkPrepare, PreForkCommit, and PreForkAbort, to either clean
 up or commit changes made during prepare.
 
 * src/security/security_driver.h (virSecurityDriverPreFork): New
 callback.
 * src/security/security_manager.h (virSecurityManagerPreFork):
 Change signature.
 * src/security/security_manager.c (virSecurityManagerPreFork):
 Optionally call into driver, and allow returning failure.
 * src/security/security_stack.c (virSecurityDriverStack):
 Wrap the handler for the stack driver.
 * src/qemu/qemu_process.c (qemuProcessStart): Adjust caller.
 
 Signed-off-by: Eric Blake ebl...@redhat.com
 (cherry picked from commit fdb3bde31ccf8ff172abf00ef5aa974b87af2794)
 
 Conflicts:
   src/security/security_manager.c - context from previous backport 
 differences

ACK

- Cole

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

Re: [libvirt] [PATCH 0/7] backport of getGroupList to v0.10.2-maint

2013-07-25 Thread Eric Blake 
On 07/25/2013 04:36 PM, Cole Robinson wrote:
 On 07/23/2013 11:03 AM, Eric Blake wrote:
 https://bugzilla.redhat.com/show_bug.cgi?id=964358

 Since it was on Fedora 18 that I first noticed the deadlock possible
 when a child process calls getpwuid_r while the parent owned the
 lock in a different thread, I'm interested in backporting my recent
 work on virGetGroupList to v0.10.2-maint.  However, I hit enough
 conflict resolution that I'd like a review of my backport decisions
 before pushing this to the stable branch.

 
 Thanks for doing this Eric! I've ACK'd each patch.

I've now pushed to v0.10.2-maint and v1.0.5-maint, if you'd like to cut
new releases for Fedora 18 and 19.

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



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

Re: [libvirt] [PATCH] Fix virsh snapshot-list error reporting

2013-07-25 Thread Jim Fehlig 
Eric Blake wrote:
 On 07/25/2013 02:38 PM, Jim Fehlig wrote:
   
 Noticed that the expected not supported error is dropped when
 invoking 'virsh snapshot-list dom' on a Xen installation running
 the libxl driver

  virsh snapshot-list test
  error: Invalid snapshot: virDomainSnapshotFree

 The error is overwritten by a call to virDomainSnapshotFree
 in cleanup code within cmdSnapshotList.  Prevent overwritting
 the real error by not calling virDomainSnapshotFree with a NULL
 virDomainSnapshotPtr.
 

 Indeed.

   
 ---

 This is one possible fix for the bug.  The other would be to
 silently return in virDomainSnapshotFree on a NULL
 virDomainSnapshotPtr.
 

 No, that would be an incompatible behavior change - our API promises a
 failure on attempting to free a null object, and people have come to
 rely on that.
   

Ah, yes, good point.

   
 +++ b/tools/virsh-snapshot.c
 @@ -1678,7 +1678,8 @@ cleanup:
  vshSnapshotListFree(snaplist);
  VIR_FREE(parent_snap);
  VIR_FREE(state);
 -virDomainSnapshotFree(start);
 +if (start)
 +virDomainSnapshotFree(start);
 

 ACK.
   

Thanks, pushed now.

Regards,
Jim

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

  1   2   >