Re: [libvirt] [PATCH 3/4] qemu: parse and create -cpu ..., -kvmclock

2012-01-24 Thread Jiri Denemark
On Mon, Jan 23, 2012 at 14:11:10 +0100, Paolo Bonzini wrote:
 Creating part of the -cpu command-line from something other than the
 cpu XML element introduces some ugliness.

Well, that's what we get for creating virtual CPUID features :-)

 ---
  src/qemu/qemu_command.c|   72 
 ++--
  tests/qemuargv2xmltest.c   |4 +
  .../qemuxml2argv-cpu-host-kvmclock.args|4 +
  .../qemuxml2argv-cpu-host-kvmclock.xml |   23 ++
  .../qemuxml2argv-cpu-kvmclock.args |4 +
  .../qemuxml2argvdata/qemuxml2argv-cpu-kvmclock.xml |   24 +++
  tests/qemuxml2argvdata/qemuxml2argv-kvmclock.args  |4 +
  tests/qemuxml2argvdata/qemuxml2argv-kvmclock.xml   |   21 ++
  tests/qemuxml2argvtest.c   |3 +
  tests/qemuxml2xmltest.c|3 +
  10 files changed, 157 insertions(+), 5 deletions(-)
  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-cpu-host-kvmclock.args
  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-cpu-host-kvmclock.xml
  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-cpu-kvmclock.args
  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-cpu-kvmclock.xml
  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-kvmclock.args
  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-kvmclock.xml

Thanks for creating tests right away :-)

ACK

Jirka

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


[libvirt] Re: [PATCH v2 4/4] Allow choice of shutdown method via virsh

2012-01-24 Thread Nicolas Sebrecht
The 23/01/12, Daniel P. Berrange wrote:
 On Mon, Jan 23, 2012 at 04:41:43PM +0100, Nicolas Sebrecht wrote:
  The 23/01/12, Michal Privoznik wrote:
  
   +By default the hypervisor will try to pick a suitable shutdown
   +method. To specify an alternative method, the I--mode parameter
   +can specify Cacpi or Cagent.
   +
  
  What's the suitable shutdown method, BTW? If I don't give the --mode
  option, it is not clear which method will be used.
 
 That is intentional, because the choice of what is used as the default
 option is hypervisor depedendant and may change at will

Thank you for the information but I think my point still applies. I
mean, what are the options/conditions which can change this hypervisor
dependent default option which may change at will? A compilation
option? A qemu option? A libvirt option?

The current documentation is a bit cryptic for the user unless we
document _how_ is defined the suitable shutdown method. Exposing what
is the current enabled method (if possible) could be interesting, too.

-- 
Nicolas Sebrecht

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


Re: [libvirt] [libvirt-glib] API to get/set custom metadata from/to domain config

2012-01-24 Thread Christophe Fergeau
On Mon, Jan 23, 2012 at 11:31:57PM +0200, Zeeshan Ali (Khattak) wrote:
 On Mon, Jan 23, 2012 at 7:19 PM, Christophe Fergeau cferg...@redhat.com 
 wrote:
  Based on a patch from Zeeshan Ali (Khattak) zeesha...@gnome.org
 
 Looks good! I'll test it later

I've only compile-tested it, if it doesn't work, don't waste time on it,
just tell me to test my stuff

 
  +    gvir_config_object_set_namespace(custom_xml, ns, ns_uri, TRUE);
 
 You meant to pass 'FALSE' in the last arg?

Dunno, depends on what we decide to do :) I prefer FALSE, your initial
patch uses TRUE so I kept that for now.

 
  +    g_return_val_if_fail(GVIR_CONFIG_IS_OBJECT(object), FALSE);
  +    g_return_val_if_fail(ns != NULL, FALSE);
  +    g_return_val_if_fail(ns_uri != NULL, FALSE);
  +
  +    namespace = xmlNewNs(object-priv-node,
  +                         (xmlChar *)ns_uri, (xmlChar *)ns);
  +    if (namespace == NULL)
  +        return FALSE;
  +    if (recursive) {
  +        set_namespace_on_tree(object-priv-node, namespace);
  +    } else {
  +        xmlSetNs(object-priv-node, namespace);
  +    }
 
 You don't need the curly braces here.

I know, but I like them :) I don't mind dropping them

Christophe


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

Re: [libvirt] libvirt and mingw64 x64, bad idea?

2012-01-24 Thread Daniel P. Berrange
On Mon, Jan 23, 2012 at 05:20:50PM -0700, Eric Blake wrote:
 On 01/23/2012 03:29 PM, Marc-André Lureau wrote:
  Hi,
  
  I tried to update the fedora mingw package to follow the mingw64
  packaging guideline and allow build for x86 and x64. But I got into
  build warning and errors for x86_64. (using Fedora Cross project repo:
  http://build1.openftd.org/fedora-cross/fedora-cross.repo,
  x86_64-w64-mingw32-gcc (GCC) 4.6.2 20110908 and later)
  
 
  The somewhat worrying error is:
  
  util/command.c:54:1: error: static assertion failed: verify
  (sizeof(pid_t) = sizeof(int))
  
  /* We have quite a bit of changes to make if this doesn't hold.  */
  verify(sizeof(pid_t) = sizeof(int));
 
 Oh my.  I guess it makes sense - while Linux pid_t is just a
 (linearly-incrementing) index into an array of kernel structs, other
 OS's use pid_t as the actual kernel pointer to the location in physical
 memory where the process information is stored; so if you have 64-bit
 pointers, you have a 64-bit pid_t.
 
 Alas, this means that any interface where we are operating on pid_t, but
 passed an int, are broken.  Thankfully, I don't see any interface like
 that in libvirt.c, and we are free to change any internal functions
 without breaking the ABI.

Arguably, we could even change the public API, provided that we
hide the change behind an #ifdef  WIN64, since we have never
done a release that is officially working on WIN64. That would
obviously want to be something of a last resort though.

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 2/3] Add test case for virHashEqual function

2012-01-24 Thread Stefan Berger

On 01/24/2012 06:19 AM, Michal Privoznik wrote:


Therefore I am pushing this under trivial and build-breaker rules (yeah,
one thing - and perhaps the only one - i like about 4.6 gcc is enhanced
static analysis as I spotted warning while compiling current HEAD):

Author: Michal Privoznikmpriv...@redhat.com
Date:   Tue Jan 24 12:09:42 2012 +0100

 hashtest: Initialize variable in virHashEqual test

 One of latest patches (b7bcb22ce2) enhanced testing for virHashEqual.
 However, hash2 variable might be used uninitialized.

diff --git a/tests/hashtest.c b/tests/hashtest.c
index 6c45b01..441672c 100644
--- a/tests/hashtest.c
+++ b/tests/hashtest.c
@@ -583,7 +583,7 @@ testHashEqualCompValue(const void *value1, const
void *value2)
  static int
  testHashEqual(const void *data ATTRIBUTE_UNUSED)
  {
-virHashTablePtr hash1, hash2;
+virHashTablePtr hash1, hash2 = NULL;
  int ret = -1;
  char keya[] = a;
  char keyb[] = b;


ACK.


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


Re: [libvirt] [PATCH v2 2/3] Add test case for virHashEqual function

2012-01-24 Thread Michal Privoznik
On 18.01.2012 17:20, Stefan Berger wrote:
 +
 +static int
 +testHashEqual(const void *data ATTRIBUTE_UNUSED)
 +{
 +virHashTablePtr hash1, hash2;
 +int ret = -1;
 +char keya[] = a;
 +char keyb[] = b;
 +char keyc[] = c;
 +char value1[] = 1;
 +char value2[] = 2;
 +char value3[] = 3;
 +char value4[] = 4;
 +
 +if (!(hash1 = virHashCreate(0, NULL)) ||
 +!(hash2 = virHashCreate(0, NULL)) ||

Actually, if the first virHashCreate() returns NULL, hash2 remains
uninitialized and we jump

 +virHashAddEntry(hash1, keya, value1)  0 ||
 +virHashAddEntry(hash1, keyb, value2)  0 ||
 +virHashAddEntry(hash1, keyc, value3)  0 ||
 +virHashAddEntry(hash2, keya, value1)  0 ||
 +virHashAddEntry(hash2, keyb, value2)  0) {
 +if (virTestGetVerbose()) {
 +testError(\nfailed to create hashes);
 +}
 +goto cleanup;
 +}

over here and do free() on uninitialized pointer.
 +
 +cleanup:
 +virHashFree(hash1);
 +virHashFree(hash2);
 +return ret;
 +}

Therefore I am pushing this under trivial and build-breaker rules (yeah,
one thing - and perhaps the only one - i like about 4.6 gcc is enhanced
static analysis as I spotted warning while compiling current HEAD):

Author: Michal Privoznik mpriv...@redhat.com
Date:   Tue Jan 24 12:09:42 2012 +0100

hashtest: Initialize variable in virHashEqual test

One of latest patches (b7bcb22ce2) enhanced testing for virHashEqual.
However, hash2 variable might be used uninitialized.

diff --git a/tests/hashtest.c b/tests/hashtest.c
index 6c45b01..441672c 100644
--- a/tests/hashtest.c
+++ b/tests/hashtest.c
@@ -583,7 +583,7 @@ testHashEqualCompValue(const void *value1, const
void *value2)
 static int
 testHashEqual(const void *data ATTRIBUTE_UNUSED)
 {
-virHashTablePtr hash1, hash2;
+virHashTablePtr hash1, hash2 = NULL;
 int ret = -1;
 char keya[] = a;
 char keyb[] = b;

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


Re: [libvirt] [PATCH v2 0/4] Add support for QEMU guest agent control

2012-01-24 Thread Michal Privoznik
On 23.01.2012 15:48, Michal Privoznik wrote:
 
 These patches are taken from here:
 https://www.redhat.com/archives/libvir-list/2011-October/msg00135.html
 
 I've just rebased and polished them.
 
 
 The QEMU guest agent /usr/bin/qemu-ga has some handy functions
 for controlling the guest, not least, shutdown/reboot and filesystem
 freeze/thaw.
 
 In Fedora 15/16 the semantics of the ACPI power button have been
 changed to suspend-to-RAM which breaks our current shutdown
 implementation.
 
 By adding support for the agent we gain a more predictable way
 to shutdown / reboot guests.
 
 NB: the code currently has the same flaw as the monitor, in
 so much as we wait forever for a guest agent reply. We need to
 add a timeout ability to the agent code
 
 diff to v1:
 - Eric's review taken in
 
 Daniel P. Berrange (4):
   QEMU guest agent support
   Add new virDomainShutdownFlags API
   Wire up QEMU agent to reboot/shutdown APIs
   Allow choice of shutdown method via virsh
 
  include/libvirt/libvirt.h.in |   15 +
  po/POTFILES.in   |1 +
  src/Makefile.am  |1 +
  src/driver.h |5 +
  src/esx/esx_driver.c |   11 +-
  src/libvirt.c|   68 +++-
  src/libvirt_public.syms  |4 +
  src/libxl/libxl_driver.c |   12 +-
  src/openvz/openvz_driver.c   |1 +
  src/qemu/qemu_agent.c| 1112 
 ++
  src/qemu/qemu_agent.h|   69 +++
  src/qemu/qemu_domain.c   |   97 
  src/qemu/qemu_domain.h   |   22 +
  src/qemu/qemu_driver.c   |  131 -
  src/qemu/qemu_monitor_json.c |2 +-
  src/qemu/qemu_process.c  |  188 +++
  src/remote/remote_driver.c   |1 +
  src/remote/remote_protocol.x |8 +-
  src/remote_protocol-structs  |5 +
  src/test/test_driver.c   |   11 +-
  src/uml/uml_driver.c |   11 +-
  src/vbox/vbox_tmpl.c |   11 +-
  src/vmware/vmware_driver.c   |1 +
  src/xen/xen_driver.c |   14 +-
  src/xenapi/xenapi_driver.c   |   12 +-
  tools/virsh.c|   47 ++-
  tools/virsh.pod  |   12 +-
  27 files changed, 1832 insertions(+), 40 deletions(-)
  create mode 100644 src/qemu/qemu_agent.c
  create mode 100644 src/qemu/qemu_agent.h
 

Thanks Eric; I've changed the patches as you've suggested and pushed.

Michal

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


Re: [libvirt] [PATCH v2] Add two NUMA tuning python bindings APIs

2012-01-24 Thread Guan Nan Ren

  Thanks for these comments.
  I will refactor the loop code, rethink about other codes and make v3 patch.

  Guannan Ren

- Original Message -
From: Eric Blake ebl...@redhat.com
To: Guannan Ren g...@redhat.com
Cc: libvir-list@redhat.com
Sent: Saturday, January 21, 2012 11:04:15 PM
Subject: Re: [libvirt] [PATCH v2] Add two NUMA tuning python bindings APIs

On 01/19/2012 02:16 AM, Guannan Ren wrote:
 *virDomainSetNumaParameters
 *virDomainGetNumaParameters
 ---
  python/libvirt-override-api.xml |   13 +++
  python/libvirt-override.c   |  186 
 +++
  2 files changed, 199 insertions(+), 0 deletions(-)
 

  static PyObject *
 +libvirt_virDomainSetNumaParameters(PyObject *self ATTRIBUTE_UNUSED,
 + PyObject *args) {

 +LIBVIRT_BEGIN_ALLOW_THREADS;
 +i_retval = virDomainGetNumaParameters(domain, params, nparams, flags);
 +LIBVIRT_END_ALLOW_THREADS;

Why are we getting the existing parameters, instead of just blindly
constructing params based on args?  I think that's a waste of effort,
considering that libvirt will already reject things if the user passes
in unknown key names.  Besides,

 +
 +if (i_retval  0) {
 +free(params);
 +return VIR_PY_INT_FAIL;
 +}
 +
 +/* convert to a Python tuple of long objects */
 +for (i = 0; i  nparams; i++) {
 +PyObject *key, *val;
 +key = libvirt_constcharPtrWrap(params[i].field);
 +val = PyDict_GetItem(info, key);
 +Py_DECREF(key);
 +
 +if (val == NULL)
 +continue;

this says that you will silently discard any unknown keys passed in from
the python code, whereas libvirt would noisily reject unknown keys.  I
don't think the python code should behave differently from the C code.

 +
 +switch (params[i].type) {
 +case VIR_TYPED_PARAM_INT:
 +params[i].value.i = (int)PyInt_AS_LONG(val);
 +break;

We're starting to repeat this code sequence in several places - we ought
to refactor things to share this conversion to and from python types and
virTypedParameter, so that all of the glue code can share the same
conversions (and fixing bugs in one will fix all of them at the same time).

 +static PyObject *
 +libvirt_virDomainGetNumaParameters(PyObject *self ATTRIBUTE_UNUSED,
 + PyObject *args) {

Indentation; also, we've lately been sticking the { of a function on
column 1:

static PyObject *
libvirt_virDomainGetNumaParameters(PyObject *self ATTRIBUTE_UNUSED,
   PyObject *args)
{

 +
 +if (i_retval  0)
 +return VIR_PY_NONE;

I think this is okay; it tells python that the C call successfully
reported an error, and that the caller can then query for the last
libvirt error.

 +
 +if ((params = malloc(sizeof(*params)*nparams)) == NULL)
 +return VIR_PY_NONE;

Bad - we didn't raise any libvirt error, so the user wouldn't be able to
tell why we returned NONE.  Rather, we should really be calling return
PyErr_NoMemory() in situations where we failed to malloc(), so that
python can correctly raise a memory exception.  (You were just
copying-and-pasting; the problem is pervasive throughout this file).

 +
 +LIBVIRT_BEGIN_ALLOW_THREADS;
 +i_retval = virDomainGetNumaParameters(domain, params, nparams, flags);
 +LIBVIRT_END_ALLOW_THREADS;
 +
 +if (i_retval  0) {
 +free(params);
 +return VIR_PY_NONE;
 +}

Okay.

 +
 +/* convert to a Python tuple of long objects */
 +if ((info = PyDict_New()) == NULL) {
 +free(params);

Memory leak - params might include VIR_TYPED_PARAM_STRING contents, so
we need to call virTypedParameterArrayClear before freeing params.

 +return VIR_PY_NONE;

Bad - PyDict_New already raised the python exception for no memory, but
we silently discarded it by returning NONE instead of NULL.  We should
really be propagating any python errors back up the call chain.

 +}
 +
 +for (i = 0 ; i  nparams ; i++) {
 +PyObject *key, *val;
 +
 +switch (params[i].type) {
 +case VIR_TYPED_PARAM_INT:
 +val = PyInt_FromLong((long)params[i].value.i);
 +break;

Again, worth refactoring code to share this loop.

 +
 +default:
 +free(params);

Another memory leak if params had any string contents.

 +Py_DECREF(info);
 +return VIR_PY_NONE;
 +}
 +
 +key = libvirt_constcharPtrWrap(params[i].field);
 +PyDict_SetItem(info, key, val);

Bad - we should be checking for failure of PyDict_SetItem, and
propagating that failure back to the caller.

 +}
 +free(params);

Another memory leak.

 +return(info);
 +}
 +

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

--
libvir-list mailing list
libvir-list@redhat.com

[libvirt] [PATCH] src/datatypes.h: fix typo

2012-01-24 Thread Alon Levy
Signed-off-by: Alon Levy al...@redhat.com
---
 src/datatypes.h |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/src/datatypes.h b/src/datatypes.h
index 91b1bfd..47058ed 100644
--- a/src/datatypes.h
+++ b/src/datatypes.h
@@ -31,7 +31,7 @@
  * VIR_CONNECT_MAGIC:
  *
  * magic value used to protect the API when pointers to connection structures
- * are passed down by the uers.
+ * are passed down by the users.
  */
 # define VIR_CONNECT_MAGIC 0x4F23DEAD
 # define VIR_IS_CONNECT(obj)   ((obj)  (obj)-magic==VIR_CONNECT_MAGIC)
-- 
1.7.8.4

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


Re: [libvirt] [PATCH v2] Add two NUMA tuning python bindings APIs

2012-01-24 Thread Eric Blake
On 01/24/2012 05:07 AM, Guan Nan Ren wrote:
  static PyObject *
 +libvirt_virDomainSetNumaParameters(PyObject *self ATTRIBUTE_UNUSED,
 + PyObject *args) {
 
 +LIBVIRT_BEGIN_ALLOW_THREADS;
 +i_retval = virDomainGetNumaParameters(domain, params, nparams, flags);
 +LIBVIRT_END_ALLOW_THREADS;
 
 Why are we getting the existing parameters, instead of just blindly
 constructing params based on args?  I think that's a waste of effort,
 considering that libvirt will already reject things if the user passes
 in unknown key names.

I finally realized _why_ we do it - that's because we want to pass the
correct types to libvirt.c, but python is not strongly typed.  That is,
if libvirt is expecting a particular named value to be
VIR_TYPED_PARAM_ULLONG, but the python code passes '1', we should be
able to properly convert that python value to C's 1ULL, rather than
rejecting the python code for using an incorrect type, since there is
not quite a notion of incorrect type in python.

That means that either we hard-code the list of all known parameter
names and their type, or we make things done via a runtime query by
getting all parameters to learn their types before formatting the user's
settings back into something that libvirt will parse.

It may also mean that libvirt itself should be a bit nicer about things
- now that we have virTypedParam.c, maybe that function should be taught
to do some type conversions (if the user passes in VIR_TYPED_PARAM_INT,
but the code wanted VIR_TYPED_PARAM_ULLONG, libvirt could do the type
conversion on the user's behalf); if libvirt is made smarter, then the
python glue code might be simpler.  But there's still the issue of
back-compat - a newer python library talking to an older libvirt that
didn't do automatic type conversion would still be stuck needing to
query for types first.

-- 
Eric Blake   ebl...@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] src/datatypes.h: fix typo

2012-01-24 Thread Michal Privoznik
On 24.01.2012 13:45, Alon Levy wrote:
 Signed-off-by: Alon Levy al...@redhat.com
 ---
  src/datatypes.h |2 +-
  1 files changed, 1 insertions(+), 1 deletions(-)
 
 diff --git a/src/datatypes.h b/src/datatypes.h
 index 91b1bfd..47058ed 100644
 --- a/src/datatypes.h
 +++ b/src/datatypes.h
 @@ -31,7 +31,7 @@
   * VIR_CONNECT_MAGIC:
   *
   * magic value used to protect the API when pointers to connection structures
 - * are passed down by the uers.
 + * are passed down by the users.
   */
  # define VIR_CONNECT_MAGIC   0x4F23DEAD
  # define VIR_IS_CONNECT(obj) ((obj)  (obj)-magic==VIR_CONNECT_MAGIC)

ACK  pushed. Thanks for catching that.

Michal

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


[libvirt] [PATCH v2 1/2] Added RSS information gathering into qemudGetProcessInfo

2012-01-24 Thread Martin Kletzander
One more parameter added into the function parsing /proc/pid/stat
and the call of the function is fixed as well.
---
v2:
 - correction of the format in fscanf in qemudGetProcessInfo

 src/qemu/qemu_driver.c |   25 ++---
 1 files changed, 18 insertions(+), 7 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 608e82a..6600afd 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -1049,12 +1049,13 @@ cleanup:


 static int
-qemudGetProcessInfo(unsigned long long *cpuTime, int *lastCpu, int pid,
-int tid)
+qemudGetProcessInfo(unsigned long long *cpuTime, int *lastCpu, long *vm_rss,
+int pid, int tid)
 {
 char *proc;
 FILE *pidinfo;
 unsigned long long usertime, systime;
+long rss;
 int cpu;
 int ret;

@@ -1071,6 +1072,8 @@ qemudGetProcessInfo(unsigned long long *cpuTime, int 
*lastCpu, int pid,
 *cpuTime = 0;
 if (lastCpu)
 *lastCpu = 0;
+if (vm_rss)
+*vm_rss = 0;
 VIR_FREE(proc);
 return 0;
 }
@@ -1082,10 +1085,10 @@ qemudGetProcessInfo(unsigned long long *cpuTime, int 
*lastCpu, int pid,
/* pid - stime */
%*d %*s %*c %*d %*d %*d %*d %*d %*u %*u %*u %*u %*u %llu %llu
/* cutime - endcode */
-   %*d %*d %*d %*d %*d %*u %*u %*d %*u %*u %*u %*u
+   %*d %*d %*d %*d %*d %*d %*u %*u %ld %*u %*u %*u
/* startstack - processor */
%*u %*u %*u %*u %*u %*u %*u %*u %*u %*u %*d %d,
-   usertime, systime, cpu) != 3) {
+   usertime, systime, rss, cpu) != 4) {
 VIR_FORCE_FCLOSE(pidinfo);
 VIR_WARN(cannot parse process status data);
 errno = -EINVAL;
@@ -1102,9 +1105,16 @@ qemudGetProcessInfo(unsigned long long *cpuTime, int 
*lastCpu, int pid,
 if (lastCpu)
 *lastCpu = cpu;

+/* We got pages
+ * We want kiloBytes
+ * _SC_PAGESIZE is page size in Bytes
+ * So calculate, but first lower the pagesize so we don't get overflow */
+if (vm_rss)
+*vm_rss = rss * (sysconf(_SC_PAGESIZE)  10);

-VIR_DEBUG(Got status for %d/%d user=%llu sys=%llu cpu=%d,
-  pid, tid, usertime, systime, cpu);
+
+VIR_DEBUG(Got status for %d/%d user=%llu sys=%llu cpu=%d rss=%ld,
+  pid, tid, usertime, systime, cpu, rss);

 VIR_FORCE_FCLOSE(pidinfo);

@@ -2066,7 +2076,7 @@ static int qemudDomainGetInfo(virDomainPtr dom,
 if (!virDomainObjIsActive(vm)) {
 info-cpuTime = 0;
 } else {
-if (qemudGetProcessInfo((info-cpuTime), NULL, vm-pid, 0)  0) {
+if (qemudGetProcessInfo((info-cpuTime), NULL, NULL, vm-pid, 0)  0) 
{
 qemuReportError(VIR_ERR_OPERATION_FAILED, %s,
 _(cannot read cputime for domain));
 goto cleanup;
@@ -3648,6 +3658,7 @@ qemudDomainGetVcpus(virDomainPtr dom,
 if (priv-vcpupids != NULL 
 qemudGetProcessInfo((info[i].cpuTime),
 (info[i].cpu),
+NULL,
 vm-pid,
 priv-vcpupids[i])  0) {
 virReportSystemError(errno, %s,
--
1.7.3.4

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


Re: [libvirt] [PATCH 1/4] qemu: get arch name from cpu element

2012-01-24 Thread Jiri Denemark
On Mon, Jan 23, 2012 at 17:21:01 +0100, Paolo Bonzini wrote:
 On 01/23/2012 04:52 PM, Jiri Denemark wrote:
  I think we could just set cpu-model even if the model used in qemu command
  line was {qemu,kvm}{32,64}. That would probably mean we need to add some of
  the models in cpu_map.xml, though.
 
 Actually I was doing that on purpose, not just for laziness. :)
 
 The idea is that, if you were using -cpu just to set -cpu 
 {qemu,kvm}{32,64},-kvmclock, you will not get an irrelevant 
 cpumodelqemu32/model/cpu in domxml-from-native output.

Yeah, I understand why you did so, however, you just discard {qemu,kvm}{32,64}
models even if there were more flags than just +-kvmclock specified, don't
you? Anyway, cpumodelqemu32/model/cpu doesn't seem irrelevant to me,
it just describes the guest visible hardware.

Jirka

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


[libvirt] [PATCH v2 2/2] Added RSS reporting

2012-01-24 Thread Martin Kletzander
Added RSS information gathering into qemuMemoryStats into qemu driver
and the reporting into virsh dommemstat.
---
v2:
 - fixed sign for the ret variable (can be negative)

 include/libvirt/libvirt.h.in |7 ++-
 src/qemu/qemu_driver.c   |   23 ++-
 tools/virsh.c|2 ++
 3 files changed, 26 insertions(+), 6 deletions(-)

diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
index 958e5a6..d5ef061 100644
--- a/include/libvirt/libvirt.h.in
+++ b/include/libvirt/libvirt.h.in
@@ -884,11 +884,16 @@ typedef enum {

 /* Current balloon value (in KB). */
 VIR_DOMAIN_MEMORY_STAT_ACTUAL_BALLOON  = 6,
+
+/* Resident Set Size of the process running the domain. This value
+ * is in kB */
+VIR_DOMAIN_MEMORY_STAT_RSS = 7,
+
 /*
  * The number of statistics supported by this version of the interface.
  * To add new statistics, add them to the enum and increase this value.
  */
-VIR_DOMAIN_MEMORY_STAT_NR  = 7,
+VIR_DOMAIN_MEMORY_STAT_NR  = 8,

 #ifdef VIR_ENUM_SENTINELS
 VIR_DOMAIN_MEMORY_STAT_LAST = VIR_DOMAIN_MEMORY_STAT_NR
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 6600afd..20d3d84 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -7978,7 +7978,7 @@ qemudDomainMemoryStats (virDomainPtr dom,
 {
 struct qemud_driver *driver = dom-conn-privateData;
 virDomainObjPtr vm;
-unsigned int ret = -1;
+int ret = -1;

 virCheckFlags(0, -1);

@@ -7997,14 +7997,27 @@ qemudDomainMemoryStats (virDomainPtr dom,
 if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY)  0)
 goto cleanup;

-if (virDomainObjIsActive(vm)) {
+if (!virDomainObjIsActive(vm)) {
+qemuReportError(VIR_ERR_OPERATION_INVALID,
+%s, _(domain is not running));
+} else {
 qemuDomainObjPrivatePtr priv = vm-privateData;
 qemuDomainObjEnterMonitor(driver, vm);
 ret = qemuMonitorGetMemoryStats(priv-mon, stats, nr_stats);
 qemuDomainObjExitMonitor(driver, vm);
-} else {
-qemuReportError(VIR_ERR_OPERATION_INVALID,
-%s, _(domain is not running));
+
+if (ret = 0  ret  nr_stats) {
+long rss;
+if (qemudGetProcessInfo(NULL, NULL, rss, vm-pid, 0)  0) {
+qemuReportError(VIR_ERR_OPERATION_FAILED, %s,
+_(cannot get RSS for domain));
+} else {
+stats[ret].tag = VIR_DOMAIN_MEMORY_STAT_RSS;
+stats[ret].val = rss;
+ret++;
+}
+
+}
 }

 if (qemuDomainObjEndJob(driver, vm) == 0)
diff --git a/tools/virsh.c b/tools/virsh.c
index d635b56..d5bbabf 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -1871,6 +1871,8 @@ cmdDomMemStat(vshControl *ctl, const vshCmd *cmd)
 vshPrint (ctl, available %llu\n, stats[i].val);
 if (stats[i].tag == VIR_DOMAIN_MEMORY_STAT_ACTUAL_BALLOON)
 vshPrint (ctl, actual %llu\n, stats[i].val);
+if (stats[i].tag == VIR_DOMAIN_MEMORY_STAT_RSS)
+vshPrint (ctl, rss %llu\n, stats[i].val);
 }

 virDomainFree(dom);
--
1.7.3.4

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


Re: [libvirt] [PATCH 1/4] qemu: get arch name from cpu element

2012-01-24 Thread Paolo Bonzini

On 01/24/2012 02:28 PM, Jiri Denemark wrote:

  The idea is that, if you were using -cpu just to set -cpu
  {qemu,kvm}{32,64},-kvmclock, you will not get an irrelevant
  cpumodelqemu32/model/cpu  in domxml-from-native output.

Yeah, I understand why you did so, however, you just discard {qemu,kvm}{32,64}
models even if there were more flags than just +-kvmclock specified, don't
you?


Right, that gives Non-empty feature list specified without CPU model 
when you later start the domain.  I'll fix this.



Anyway,cpumodelqemu32/model/cpu  doesn't seem irrelevant to me,
it just describes the guest visible hardware.


Yeah, but arch=i686 says the same (kvm32/kvm64 shouldn't be considered 
by this code, only qemu32/qemu64).


Paolo

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


[libvirt] [PATCH][RFC] adding a title to the domain description informations

2012-01-24 Thread Daniel Veillard
 The idea is that currently we have only the domain name usable as
a description for the domain. It is not really a good human readable
identifier, as the kind of string allowed is limited (no space for
example). The idea would then be to extend the existing description
field in the domain XML to keep 40 or less character string to describe
a domain and provide that information later for example in an extended
virsh list command or for other interfaces.
  While the idea is simple, see attached patch for this, it becomes more
complex when one tries to make accessors to set/get that title for a
domain, since it's mutable and possibly could be coming from the
hypervisor itself (is there anything like this in VMWare or VirtualBox?)
it should to be implemented down at the driver level. Is that worth the
effort ? If we go that route should we do this for other objects
(network, storage, etc ...) too in the end ?

here is a basic patch for just the XML side to give an idea, but
adding APIs is far more work.

  Opinions ?

Daniel
 
Provide a title attribute on domain descriptions

Allow to provide a title attribute to domain descriptions
useful when presenting the domain in an user interface.

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index de9b480..ad5ffb0 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -58,7 +58,13 @@
   ddThe content of the codedescription/code element provides a
   human readable description of the virtual machine. This data is not
   used by libvirt in any way, it can contain any information the user
-  wants. span class=sinceSince 0.7.2/span/dd
+  wants. span class=sinceSince 0.7.2/span. It is also possible
+  to provide an human readable title for the virtual machine, by
+  providing it as a codetitlecode attribute on the
+  codedescription/code element, this can be useful to provide an
+  human description when listing virtual machines (span class=since
+  Since 0.9.10/span)/dd
+
 /dl
 
 h3a name=elementsOSOperating system booting/a/h3
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 2041dfb..78ba67c 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -10,6 +10,11 @@
 --
   define name=description
 element name=description
+  optional
+attribute name=title
+  ref name=title-value/
+/attribute
+  /optional
   text/
 /element
   /define
@@ -3115,4 +3120,9 @@
 /element
 empty/
   /define
+  define name=title-value
+data type=string
+  param name=pattern.{0,40}/param
+/data
+  /define
 /grammar
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index f97014e..0e1ff44 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -1478,6 +1478,7 @@ void virDomainDefFree(virDomainDefPtr def)
 VIR_FREE(def-cpumask);
 VIR_FREE(def-emulator);
 VIR_FREE(def-description);
+VIR_FREE(def-title);
 
 virBlkioDeviceWeightArrayClear(def-blkio.devices,
def-blkio.ndevices);
@@ -7092,6 +7093,7 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr 
caps,
 
 /* Extract documentation if present */
 def-description = virXPathString(string(./description[1]), ctxt);
+def-title = virXPathString(string(./description[1]/@title), ctxt);
 
 /* analysis of security label, done early even though we format it
  * late, so devices can refer to this for defaults */
@@ -11417,8 +11419,16 @@ virDomainDefFormatInternal(virDomainDefPtr def,
 virUUIDFormat(uuid, uuidstr);
 virBufferAsprintf(buf,   uuid%s/uuid\n, uuidstr);
 
-virBufferEscapeString(buf,   description%s/description\n,
-  def-description);
+if (def-title || def-description) {
+virBufferAddLit(buf,   description);
+if (def-title)
+virBufferEscapeString(buf,  title='%s', def-title);
+if (def-description)
+virBufferEscapeString(buf, %s/description\n,
+  def-description);
+else
+virBufferAddLit(buf, /);
+}
 
 virBufferAsprintf(buf,   memory%lu/memory\n, def-mem.max_balloon);
 virBufferAsprintf(buf,   currentMemory%lu/currentMemory\n,
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index b121f9c..45d70e6 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -1421,6 +1421,7 @@ struct _virDomainDef {
 unsigned char uuid[VIR_UUID_BUFLEN];
 char *name;
 char *description;
+char *title;
 
 struct {
 unsigned int weight;

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

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


Re: [libvirt] [PATCH][RFC] adding a title to the domain description informations

2012-01-24 Thread Eric Blake
On 01/24/2012 07:18 AM, Daniel Veillard wrote:
  The idea is that currently we have only the domain name usable as
 a description for the domain. It is not really a good human readable
 identifier, as the kind of string allowed is limited (no space for
 example). The idea would then be to extend the existing description
 field in the domain XML to keep 40 or less character string to describe
 a domain and provide that information later for example in an extended
 virsh list command or for other interfaces.
   While the idea is simple, see attached patch for this, it becomes more
 complex when one tries to make accessors to set/get that title for a
 domain, since it's mutable and possibly could be coming from the
 hypervisor itself (is there anything like this in VMWare or VirtualBox?)
 it should to be implemented down at the driver level. Is that worth the
 effort ? If we go that route should we do this for other objects
 (network, storage, etc ...) too in the end ?
 
 here is a basic patch for just the XML side to give an idea, but
 adding APIs is far more work.
 
   Opinions ?

Looks like this is more or less identical to Peter's proposal to add a
title element:
https://www.redhat.com/archives/libvir-list/2012-January/msg00710.html

so I'm all for the idea (not sure whether your patch or Peter's is
better for the idea).

As far as I know, the title is _not_ coming from the hypervisor itself,
nor is it guest-visible; it is merely metadata from the host's perspective.

-- 
Eric Blake   ebl...@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][RFC] adding a title to the domain description informations

2012-01-24 Thread Daniel P. Berrange
On Tue, Jan 24, 2012 at 08:06:31AM -0700, Eric Blake wrote:
 On 01/24/2012 07:18 AM, Daniel Veillard wrote:
   The idea is that currently we have only the domain name usable as
  a description for the domain. It is not really a good human readable
  identifier, as the kind of string allowed is limited (no space for
  example). The idea would then be to extend the existing description
  field in the domain XML to keep 40 or less character string to describe
  a domain and provide that information later for example in an extended
  virsh list command or for other interfaces.
While the idea is simple, see attached patch for this, it becomes more
  complex when one tries to make accessors to set/get that title for a
  domain, since it's mutable and possibly could be coming from the
  hypervisor itself (is there anything like this in VMWare or VirtualBox?)
  it should to be implemented down at the driver level. Is that worth the
  effort ? If we go that route should we do this for other objects
  (network, storage, etc ...) too in the end ?
  
  here is a basic patch for just the XML side to give an idea, but
  adding APIs is far more work.
  
Opinions ?
 
 Looks like this is more or less identical to Peter's proposal to add a
 title element:
 https://www.redhat.com/archives/libvir-list/2012-January/msg00710.html
 
 so I'm all for the idea (not sure whether your patch or Peter's is
 better for the idea).

IMHO using a element is preferrable to an attribute, particularly
since we're also adding a metadata attribute.

 As far as I know, the title is _not_ coming from the hypervisor itself,
 nor is it guest-visible; it is merely metadata from the host's perspective.


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] [RFC] virCommandProcessIO(): make poll() usage more robust

2012-01-24 Thread Laszlo Ersek
POLLIN and POLLHUP are not mutually exclusive. Currently the following
seems possible: the child writes 3K to its stdout or stderr pipe, and
immediately closes it. We get POLLIN|POLLHUP (I'm not sure that's possible
on Linux, but SUSv4 seems to allow it). We read 1K and throw away the
rest.

When poll() returns and we're about to check the /revents/ member in a
given array element, let's map all the revents bits to two (independent)
ideas: let's attempt to read(), and let's attempt to write(). This
should cover all errors, EOFs, and normal conditions; the read()/write()
call should report any pending error.

Under this approach, both POLLHUP and POLLERR are mapped to needs read()
if we're otherwise prepared for POLLIN. POLLERR also maps to needs
write() if we're otherwise prepared for POLLOUT. The rest of the mappings
(POLLPRI etc.) would be easy, but probably useless for pipes.

Additionally, SUSv4 doesn't appear to forbid POLLIN|POLLERR (or
POLLOUT|POLLERR) set simultaneously. One could argue that the read() or
write() call would return without blocking in these cases (with an error),
so POLLIN / POLLOUT would be justified beside POLLERR.

The code now penalizes POLLIN|POLLERR differently from plain POLLERR. The
former (ie. read() returning -1) is terminal and we jump to cleanup, while
plain POLLERR masks only the affected file descriptor for the future.
Let's unify those.

Build tested only.

Please keep me CC'd, I'm not subscribed. Thanks.

Signed-off-by: Laszlo Ersek ler...@redhat.com
---
 src/util/command.c |   17 ++---
 1 files changed, 2 insertions(+), 15 deletions(-)

diff --git a/src/util/command.c b/src/util/command.c
index f05493e..dc3cfc5 100644
--- a/src/util/command.c
+++ b/src/util/command.c
@@ -1710,7 +1710,7 @@ virCommandProcessIO(virCommandPtr cmd)
 }
 
 for (i = 0; i  nfds ; i++) {
-if (fds[i].revents  POLLIN 
+if (fds[i].revents  (POLLIN | POLLHUP | POLLERR) 
 (fds[i].fd == errfd || fds[i].fd == outfd)) {
 char data[1024];
 char **buf;
@@ -1751,7 +1751,7 @@ virCommandProcessIO(virCommandPtr cmd)
 }
 }
 
-if (fds[i].revents  POLLOUT 
+if (fds[i].revents  (POLLOUT | POLLERR) 
 fds[i].fd == infd) {
 int done;
 
@@ -1777,19 +1777,6 @@ virCommandProcessIO(virCommandPtr cmd)
 }
 }
 }
-
-if (fds[i].revents  (POLLHUP | POLLERR)) {
-if (fds[i].fd == errfd) {
-VIR_DEBUG(hangup on stderr);
-errfd = -1;
-} else if (fds[i].fd == outfd) {
-VIR_DEBUG(hangup on stdout);
-outfd = -1;
-} else {
-VIR_DEBUG(hangup on stdin);
-infd = -1;
-}
-}
 }
 }
 
-- 
1.7.1

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


Re: [libvirt] [PATCH 1/4] virDomainIOError public API and remote protocol

2012-01-24 Thread Federico Simoncelli
- Original Message -
 From: Jiri Denemark jdene...@redhat.com
 To: libvir-list@redhat.com
 Sent: Monday, January 23, 2012 2:30:54 PM
 Subject: [libvirt] [PATCH 1/4] virDomainIOError public API and remote protocol
 
 We already provide ways to detect when a domain has been paused as a
 result of I/O error, but there was no way of getting the exact error
 or
 even the device that experienced it.  This new API may be used for
 both.
 ---
  include/libvirt/libvirt.h.in |   19 +++
  src/driver.h |6 
  src/libvirt.c|   53
  ++
  src/libvirt_public.syms  |5 
  src/remote/remote_driver.c   |1 +
  src/remote/remote_protocol.x |   13 +-
  src/remote_protocol-structs  |9 +++
  7 files changed, 105 insertions(+), 1 deletions(-)
 
 diff --git a/include/libvirt/libvirt.h.in
 b/include/libvirt/libvirt.h.in
 index 958e5a6..2c3423a 100644
 --- a/include/libvirt/libvirt.h.in
 +++ b/include/libvirt/libvirt.h.in
 @@ -3746,6 +3746,25 @@ int virConnectSetKeepAlive(virConnectPtr conn,
 int interval,
 unsigned int count);
  
 +/**
 + * virDomainIoError:
 + *
 + * Disk I/O error.
 + */
 +typedef enum {
 +VIR_DOMAIN_IOERROR_NONE = 0, /* no error */
 +VIR_DOMAIN_IOERROR_NO_SPACE = 1, /* no space left on the device
 */
 +VIR_DOMAIN_IOERROR_UNSPEC   = 2, /* unspecified I/O error */
 +
 +#ifdef VIR_ENUM_SENTINELS
 +VIR_DOMAIN_IOERROR_LAST
 +#endif
 +} virDomainIoError;
 +


Hi Jiri, how do we detect an EIO error? We should need an additional
error type here?

-- 
Federico

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


[libvirt] [PATCH 1/3] qemu_agent: Create file system freeze and thaw functions

2012-01-24 Thread Michal Privoznik
These functions simply issue command to guest agent which
should freeze or unfreeze all file systems within guest.
---
 src/qemu/qemu_agent.c |   68 +
 src/qemu/qemu_agent.h |3 ++
 2 files changed, 71 insertions(+), 0 deletions(-)

diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
index 6a7c7b3..3a5cc4b 100644
--- a/src/qemu/qemu_agent.c
+++ b/src/qemu/qemu_agent.c
@@ -1110,3 +1110,71 @@ int qemuAgentShutdown(qemuAgentPtr mon,
 virJSONValueFree(reply);
 return ret;
 }
+
+/*
+ * qemuAgentFSFreeze:
+ * @mon: Agent
+ *
+ * Issue guest-fsfreeze-freeze command to guest agent,
+ * which freezes all mounted file systems and returns
+ * number of frozen file systems on success.
+ *
+ * Returns: number of file system frozen on success,
+ *  -1 on error.
+ */
+int qemuAgentFSFreeze(qemuAgentPtr mon)
+{
+int ret = -1;
+virJSONValuePtr cmd;
+virJSONValuePtr reply = NULL;
+
+cmd = qemuAgentMakeCommand(guest-fsfreeze-freeze, NULL);
+
+if (!cmd)
+return -1;
+
+if (qemuAgentCommand(mon, cmd, reply)  0 ||
+qemuAgentCheckError(cmd, reply)  0)
+goto cleanup;
+
+virJSONValueObjectGetNumberInt(reply, return, ret);
+
+cleanup:
+virJSONValueFree(cmd);
+virJSONValueFree(reply);
+return ret;
+}
+
+/*
+ * qemuAgentFSThaw:
+ * @mon: Agent
+ *
+ * Issue guest-fsfreeze-thaw command to guest agent,
+ * which unfreezes all mounted file systems and returns
+ * number of thawed file systems on success.
+ *
+ * Returns: number of file system thawed on success,
+ *  -1 on error.
+ */
+int qemuAgentFSThaw(qemuAgentPtr mon)
+{
+int ret = -1;
+virJSONValuePtr cmd;
+virJSONValuePtr reply = NULL;
+
+cmd = qemuAgentMakeCommand(guest-fsfreeze-thaw, NULL);
+
+if (!cmd)
+return -1;
+
+if (qemuAgentCommand(mon, cmd, reply)  0 ||
+qemuAgentCheckError(cmd, reply)  0)
+goto cleanup;
+
+virJSONValueObjectGetNumberInt(reply, return, ret);
+
+cleanup:
+virJSONValueFree(cmd);
+virJSONValueFree(reply);
+return ret;
+}
diff --git a/src/qemu/qemu_agent.h b/src/qemu/qemu_agent.h
index 93c2ae7..df59ef7 100644
--- a/src/qemu/qemu_agent.h
+++ b/src/qemu/qemu_agent.h
@@ -66,4 +66,7 @@ typedef enum {
 int qemuAgentShutdown(qemuAgentPtr mon,
   qemuAgentShutdownMode mode);
 
+int qemuAgentFSFreeze(qemuAgentPtr mon);
+int qemuAgentFSThaw(qemuAgentPtr mon);
+
 #endif /* __QEMU_AGENT_H__ */
-- 
1.7.3.4

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


[libvirt] [PATCH 2/3] snapshots: Introduce VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE flag

2012-01-24 Thread Michal Privoznik
With this flag, virDomainSnapshotCreate will use fs-freeze and
fs-thaw guest agent commands to quiesce guest's disks.
---
 include/libvirt/libvirt.h.in |4 ++
 src/libvirt.c|6 ++
 src/qemu/qemu_driver.c   |  118 -
 3 files changed, 113 insertions(+), 15 deletions(-)

diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
index 5e6e488..182065d 100644
--- a/include/libvirt/libvirt.h.in
+++ b/include/libvirt/libvirt.h.in
@@ -3125,6 +3125,10 @@ typedef enum {
   system checkpoint */
 VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT   = (1  5), /* reuse any existing
   external files */
+VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE = (1  6), /* use guest agent to
+  quiesce all mounted
+  file systems within
+  the domain */
 } virDomainSnapshotCreateFlags;
 
 /* Take a snapshot of the current VM state */
diff --git a/src/libvirt.c b/src/libvirt.c
index 96ad3d5..d9b8d88 100644
--- a/src/libvirt.c
+++ b/src/libvirt.c
@@ -16602,6 +16602,12 @@ virDomainSnapshotGetConnect(virDomainSnapshotPtr 
snapshot)
  * inconsistent (as if power had been pulled), and specifying this
  * with the VIR_DOMAIN_SNAPSHOT_CREATE_HALT flag risks data loss.
  *
+ * If @flags includes VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE, then the
+ * libvirt will attempt to use guest agent to freeze and thaw all
+ * file systems in use within domain OS. However, if the guest agent
+ * is not present, an error is trowed. Moreover, this flags requires
+ * VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY to be passed as well.
+ *
  * By default, if the snapshot involves external files, and any of the
  * destination files already exist as a regular file, the snapshot is
  * rejected to avoid losing contents of those files.  However, if
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index c81289a..14ad30b 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -9461,6 +9461,56 @@ static int qemuDomainSnapshotIsAllowed(virDomainObjPtr 
vm)
 return 1;
 }
 
+static int
+qemuDomainSnapshotFSFreeze(struct qemud_driver *driver,
+   virDomainObjPtr vm) {
+qemuDomainObjPrivatePtr priv = vm-privateData;
+int freezed;
+
+if (priv-agentError) {
+qemuReportError(VIR_ERR_INTERNAL_ERROR, %s,
+_(QEMU guest agent is not 
+  available due to an error));
+return -1;
+}
+if (!priv-agent) {
+qemuReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, %s,
+_(QEMU guest agent is not configured));
+return -1;
+}
+
+qemuDomainObjEnterAgent(driver, vm);
+freezed = qemuAgentFSFreeze(priv-agent);
+qemuDomainObjExitAgent(driver, vm);
+
+return freezed;
+}
+
+static int
+qemuDomainSnapshotFSThaw(struct qemud_driver *driver,
+ virDomainObjPtr vm) {
+qemuDomainObjPrivatePtr priv = vm-privateData;
+int thawed;
+
+if (priv-agentError) {
+qemuReportError(VIR_ERR_INTERNAL_ERROR, %s,
+_(QEMU guest agent is not 
+  available due to an error));
+return -1;
+}
+if (!priv-agent) {
+qemuReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, %s,
+_(QEMU guest agent is not configured));
+return -1;
+}
+
+qemuDomainObjEnterAgent(driver, vm);
+thawed = qemuAgentFSThaw(priv-agent);
+qemuDomainObjExitAgent(driver, vm);
+
+return thawed;
+}
+
 /* The domain is expected to be locked and inactive. */
 static int
 qemuDomainSnapshotCreateInactive(struct qemud_driver *driver,
@@ -9493,6 +9543,13 @@ qemuDomainSnapshotCreateActive(virConnectPtr conn,
 }
 
 if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_RUNNING) {
+if ((flags  VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE) 
+(qemuDomainSnapshotFSFreeze(driver, vm)  0)) {
+qemuReportError(VIR_ERR_INTERNAL_ERROR, %s,
+_(Unable to freeze domain's file systems));
+goto endjob;
+}
+
 /* savevm monitor command pauses the domain emitting an event which
  * confuses libvirt since it's not notified when qemu resumes the
  * domain. Thus we stop and start CPUs ourselves.
@@ -9532,13 +9589,21 @@ qemuDomainSnapshotCreateActive(virConnectPtr conn,
 }
 
 cleanup:
-if (resume  virDomainObjIsActive(vm) 
-qemuProcessStartCPUs(driver, vm, conn,
- VIR_DOMAIN_RUNNING_UNPAUSED,
- QEMU_ASYNC_JOB_NONE)  0 
-virGetLastError() == NULL) {
-qemuReportError(VIR_ERR_OPERATION_FAILED, %s,
-_(resuming after 

[libvirt] [PATCH 0/3] Use guest agent to quiesce disks

2012-01-24 Thread Michal Privoznik
Since we have qemu guest agent support in libvirt,
we can start wiring up some things that GA already
knows how to do. One of them is file system freeze
and thaw. Domain snapshots can profit from this
functionality.

Michal Privoznik (3):
  qemu_agent: Create file system freeze and thaw functions
  snapshots: Introduce VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE flag
  virsh: Expose new VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE flag

 include/libvirt/libvirt.h.in |4 ++
 src/libvirt.c|6 ++
 src/qemu/qemu_agent.c|   68 
 src/qemu/qemu_agent.h|3 +
 src/qemu/qemu_driver.c   |  118 -
 tools/virsh.c|6 ++
 tools/virsh.pod  |   14 -
 7 files changed, 202 insertions(+), 17 deletions(-)

-- 
1.7.3.4

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


[libvirt] [PATCH 3/3] virsh: Expose new VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE flag

2012-01-24 Thread Michal Privoznik
to cmdSnapshotCreate and cmdSnapshotCreateAs.
---
 tools/virsh.c   |6 ++
 tools/virsh.pod |   14 --
 2 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/tools/virsh.c b/tools/virsh.c
index 5560988..41c 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -14662,6 +14662,7 @@ static const vshCmdOptDef opts_snapshot_create[] = {
 {halt, VSH_OT_BOOL, 0, N_(halt domain after snapshot is created)},
 {disk-only, VSH_OT_BOOL, 0, N_(capture disk state but not vm state)},
 {reuse-external, VSH_OT_BOOL, 0, N_(reuse any existing external 
files)},
+{quiesce, VSH_OT_BOOL, 0, N_(quiesce guest's file systems)},
 {NULL, 0, 0, NULL}
 };
 
@@ -14686,6 +14687,8 @@ cmdSnapshotCreate(vshControl *ctl, const vshCmd *cmd)
 flags |= VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY;
 if (vshCommandOptBool(cmd, reuse-external))
 flags |= VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT;
+if (vshCommandOptBool(cmd, quiesce))
+flags |= VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE;
 
 if (!vshConnectionUsability(ctl, ctl-conn))
 goto cleanup;
@@ -14795,6 +14798,7 @@ static const vshCmdOptDef opts_snapshot_create_as[] = {
 {halt, VSH_OT_BOOL, 0, N_(halt domain after snapshot is created)},
 {disk-only, VSH_OT_BOOL, 0, N_(capture disk state but not vm state)},
 {reuse-external, VSH_OT_BOOL, 0, N_(reuse any existing external 
files)},
+{quiesce, VSH_OT_BOOL, 0, N_(quiesce guest's file systems)},
 {diskspec, VSH_OT_ARGV, 0,
  N_(disk attributes: disk[,snapshot=type][,driver=type][,file=name])},
 {NULL, 0, 0, NULL}
@@ -14820,6 +14824,8 @@ cmdSnapshotCreateAs(vshControl *ctl, const vshCmd *cmd)
 flags |= VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY;
 if (vshCommandOptBool(cmd, reuse-external))
 flags |= VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT;
+if (vshCommandOptBool(cmd, quiesce))
+flags |= VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE;
 
 if (!vshConnectionUsability(ctl, ctl-conn))
 goto cleanup;
diff --git a/tools/virsh.pod b/tools/virsh.pod
index 72c6d8f..b1f75de 100644
--- a/tools/virsh.pod
+++ b/tools/virsh.pod
@@ -2046,7 +2046,8 @@ used to represent properties of snapshots.
 =over 4
 
 =item Bsnapshot-create Idomain [Ixmlfile] {[I--redefine [I--current]]
-| [I--no-metadata] [I--halt] [I--disk-only] [I--reuse-external]}
+| [I--no-metadata] [I--halt] [I--disk-only] [I--reuse-external]
+[I--quiesce]}
 
 Create a snapshot for domain Idomain with the properties specified in
 Ixmlfile.  Normally, the only properties settable for a domain snapshot
@@ -2088,6 +2089,10 @@ external snapshot with a destination of an existing 
file, then the
 existing file is truncated and reused; otherwise, a snapshot is refused
 to avoid losing contents of the existing files.
 
+If I--quiesce is specified, libvirt will try to use guest agent
+to freeze and unfreeze domain's mounted file systems. However,
+currently this requires I--disk-only to be passed as well.
+
 Existence of snapshot metadata will prevent attempts to Bundefine
 a persistent domain.  However, for transient domains, snapshot
 metadata is silently lost when the domain quits running (whether
@@ -2095,7 +2100,8 @@ by command such as Bdestroy or by internal guest 
action).
 
 =item Bsnapshot-create-as Idomain {[I--print-xml]
 | [I--no-metadata] [I--halt] [I--reuse-existing]} [Iname]
-[Idescription] [I--disk-only [[I--diskspec] Bdiskspec]...]
+[Idescription] [I--disk-only [I--quiesce]
+[[I--diskspec] Bdiskspec]...]
 
 Create a snapshot for domain Idomain with the given name and
 description; if either value is omitted, libvirt will choose a
@@ -2123,6 +2129,10 @@ option requests an external snapshot with a destination 
of an existing
 file, then the existing file is truncated and reused; otherwise, a
 snapshot is refused to avoid losing contents of the existing files.
 
+If I--quiesce is specified, libvirt will try to use guest agent
+to freeze and unfreeze domain's mounted file systems. However,
+currently this requires I--disk-only to be passed as well.
+
 If I--no-metadata is specified, then the snapshot data is created,
 but any metadata is immediately discarded (that is, libvirt does not
 treat the snapshot as current, and cannot revert to the snapshot
-- 
1.7.3.4

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


Re: [libvirt] [PATCH 1/4] virDomainIOError public API and remote protocol

2012-01-24 Thread Jiri Denemark
On Tue, Jan 24, 2012 at 11:57:23 -0500, Federico Simoncelli wrote:
  +/**
  + * virDomainIoError:
  + *
  + * Disk I/O error.
  + */
  +typedef enum {
  +VIR_DOMAIN_IOERROR_NONE = 0, /* no error */
  +VIR_DOMAIN_IOERROR_NO_SPACE = 1, /* no space left on the device
  */
  +VIR_DOMAIN_IOERROR_UNSPEC   = 2, /* unspecified I/O error */
  +
  +#ifdef VIR_ENUM_SENTINELS
  +VIR_DOMAIN_IOERROR_LAST
  +#endif
  +} virDomainIoError;
  +
 
 
 Hi Jiri, how do we detect an EIO error? We should need an additional
 error type here?

We could certainly add more error codes but doing so only makes sense when
there is a hypervisor that can report them. Current qemu has three io-status
values: ok, nospace, failed and these values are mapped to
VIR_DOMAIN_IOERROR_NONE, VIR_DOMAIN_IOERROR_NO_SPACE, and
VIR_DOMAIN_IOERROR_UNSPEC. I believe that EIO would be reported as failed by
qemu and thus VIR_DOMAIN_IOERROR_UNSPEC by libvirt. It's possible that
VIR_DOMAIN_IOERROR_UNSPEC is not the best name, though :-)

Jirka

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


Re: [libvirt] [PATCH 1/3] qemu_agent: Create file system freeze and thaw functions

2012-01-24 Thread Jiri Denemark
On Tue, Jan 24, 2012 at 18:37:43 +0100, Michal Privoznik wrote:
 These functions simply issue command to guest agent which
 should freeze or unfreeze all file systems within guest.
 ---
  src/qemu/qemu_agent.c |   68 
 +
  src/qemu/qemu_agent.h |3 ++
  2 files changed, 71 insertions(+), 0 deletions(-)
 
 diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
 index 6a7c7b3..3a5cc4b 100644
 --- a/src/qemu/qemu_agent.c
 +++ b/src/qemu/qemu_agent.c
 @@ -1110,3 +1110,71 @@ int qemuAgentShutdown(qemuAgentPtr mon,
  virJSONValueFree(reply);
  return ret;
  }
 +
 +/*
 + * qemuAgentFSFreeze:
 + * @mon: Agent
 + *
 + * Issue guest-fsfreeze-freeze command to guest agent,
 + * which freezes all mounted file systems and returns
 + * number of frozen file systems on success.
 + *
 + * Returns: number of file system frozen on success,
 + *  -1 on error.
 + */
 +int qemuAgentFSFreeze(qemuAgentPtr mon)
 +{
 +int ret = -1;
 +virJSONValuePtr cmd;
 +virJSONValuePtr reply = NULL;
 +
 +cmd = qemuAgentMakeCommand(guest-fsfreeze-freeze, NULL);
 +
 +if (!cmd)
 +return -1;
 +
 +if (qemuAgentCommand(mon, cmd, reply)  0 ||
 +qemuAgentCheckError(cmd, reply)  0)
 +goto cleanup;
 +
 +virJSONValueObjectGetNumberInt(reply, return, ret);

Doesn't the above function need to be checked for errors? That is, what if
there's no return key in the reply? In json monitor code, we usually have
something like the following code:

if (virJSONValueObjectGetNumberInt(reply, return, ret)  0) {
qemuReportError(VIR_ERR_INTERNAL_ERROR, %s,
_(guest-fsfreeze-freeze reply was missing return
   data));
}

 +
 +cleanup:
 +virJSONValueFree(cmd);
 +virJSONValueFree(reply);
 +return ret;
 +}
 +
 +/*
 + * qemuAgentFSThaw:
 + * @mon: Agent
 + *
 + * Issue guest-fsfreeze-thaw command to guest agent,
 + * which unfreezes all mounted file systems and returns
 + * number of thawed file systems on success.
 + *
 + * Returns: number of file system thawed on success,
 + *  -1 on error.
 + */
 +int qemuAgentFSThaw(qemuAgentPtr mon)
 +{
 +int ret = -1;
 +virJSONValuePtr cmd;
 +virJSONValuePtr reply = NULL;
 +
 +cmd = qemuAgentMakeCommand(guest-fsfreeze-thaw, NULL);
 +
 +if (!cmd)
 +return -1;
 +
 +if (qemuAgentCommand(mon, cmd, reply)  0 ||
 +qemuAgentCheckError(cmd, reply)  0)
 +goto cleanup;
 +
 +virJSONValueObjectGetNumberInt(reply, return, ret);

And similar check for this line as well.

 +
 +cleanup:
 +virJSONValueFree(cmd);
 +virJSONValueFree(reply);
 +return ret;
 +}
 diff --git a/src/qemu/qemu_agent.h b/src/qemu/qemu_agent.h
 index 93c2ae7..df59ef7 100644
 --- a/src/qemu/qemu_agent.h
 +++ b/src/qemu/qemu_agent.h
 @@ -66,4 +66,7 @@ typedef enum {
  int qemuAgentShutdown(qemuAgentPtr mon,
qemuAgentShutdownMode mode);
  
 +int qemuAgentFSFreeze(qemuAgentPtr mon);
 +int qemuAgentFSThaw(qemuAgentPtr mon);
 +
  #endif /* __QEMU_AGENT_H__ */
 -- 
 1.7.3.4
 
 --
 libvir-list mailing list
 libvir-list@redhat.com
 https://www.redhat.com/mailman/listinfo/libvir-list

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


Re: [libvirt] [PATCH 6/7] Add support for sVirt in the LXC driver

2012-01-24 Thread Eric Blake
On 01/11/2012 09:33 AM, Daniel P. Berrange wrote:
 From: Daniel P. Berrange berra...@redhat.com
 
 For the sake of backwards compat, LXC guests are *not*
 confined by default. This is because it is not practical
 to dynamically relabel containers using large filesystem
 trees. Applications can create confined containers though,
 by giving suitable XML configs
 
 * src/Makefile.am: Link libvirt_lxc to security drivers
 * src/lxc/libvirtd_lxc.aug, src/lxc/lxc_conf.h,
   src/lxc/lxc_conf.c, src/lxc/lxc.conf,
   src/lxc/test_libvirtd_lxc.aug: Config file handling for
   security driver
 * src/lxc/lxc_driver.c: Wire up security driver functions
 * src/lxc/lxc_controller.c: Add a '--security' flag to
   specify which security driver to activate
 * src/lxc/lxc_container.c, src/lxc/lxc_container.h: Set
   the process label just before exec'ing init.
 ---

 +++ b/src/lxc/lxc.conf
 @@ -11,3 +11,21 @@
  # This is disabled by default, uncomment below to enable it.
  #
  # log_with_libvirtd = 1
 +
 +
 +# The default security driver is SELinux. If SELinux is disabled
 +# on the host, then the security driver will automatically disable
 +# itself. If you wish to disable QEMU SELinux security driver while
 +# leaving SELinux enabled for the host in general, then set this
 +# to 'none' instead.
 +#
 +# security_driver = selinux
 +
 +# If set to non-zero, then the default security labelling

Same question as 5/7 about whether to prefer US spelling of labeling.

 +# will make guests confined. If set to zero, then guests
 +# will be unconfined by default. Defaults to zero
 +# security_default_confined = 1
 +
 +# If set to non-zero, then attempts to create unconfined
 +# guests will be blocked. Defaults to zero.

Consistency - one description ended with '.', the other did not.  Back
to the 5/7 question of whether this should be spelled out as 'zero' or
listed as '0'.

 +# security_require_confined = 1
 \ No newline at end of file

'make syntax-check' wasn't happy:

prohibit_empty_lines_at_EOF
src/lxc/lxc.conf
maint.mk: empty line(s) or no newline at EOF

 @@ -1598,6 +1625,12 @@ lxcBuildControllerCmd(lxc_driver_t *driver,
  virCommandAddArgFormat(cmd, %d, ttyFDs[i]);
  virCommandPreserveFD(cmd, ttyFDs[i]);
  }
 +
 +if (driver-securityDriverName) {
 +virCommandAddArg(cmd, --security);
 +virCommandAddArg(cmd, driver-securityDriverName);
 +}

Is it worth the shorter:

if (driver-securityDriverName)
virCommandAddArgPair(cmd, --security, driver-securityDriverName);

 +
 +static int lxcNodeGetSecurityModel(virConnectPtr conn,
 +   virSecurityModelPtr secmodel)
 +{

 +
 +p = driver-caps-host.secModel.model;
 +if (strlen(p) = VIR_SECURITY_MODEL_BUFLEN-1) {
 +lxcError(VIR_ERR_INTERNAL_ERROR,
 + _(security model string exceeds max %d bytes),
 + VIR_SECURITY_MODEL_BUFLEN-1);
 +ret = -1;
 +goto cleanup;
 +}
 +strcpy(secmodel-model, p);

Rather than doing length checks and then strcpy, wouldn't it be better
to use virStrncpy?  (Twice in this function).

 @@ -3859,6 +4051,8 @@ static virDriver lxcDriver = {
  .domainGetBlkioParameters = lxcDomainGetBlkioParameters, /* 0.9.8 */
  .domainGetInfo = lxcDomainGetInfo, /* 0.4.2 */
  .domainGetState = lxcDomainGetState, /* 0.9.2 */
 +.domainGetSecurityLabel = lxcDomainGetSecurityLabel, /* 0.9.4 */
 +.nodeGetSecurityModel = lxcNodeGetSecurityModel, /* 0.9.4 */

You've been sitting on this series for a while, now :)

0.9.10, not 0.9.4.

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



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

Re: [libvirt] [PATCH 1/3] qemu_agent: Create file system freeze and thaw functions

2012-01-24 Thread Michal Privoznik
On 24.01.2012 19:21, Jiri Denemark wrote:
 On Tue, Jan 24, 2012 at 18:37:43 +0100, Michal Privoznik wrote:

 +
 +if (qemuAgentCommand(mon, cmd, reply)  0 ||
 +qemuAgentCheckError(cmd, reply)  0)
 +goto cleanup;
 +
 +virJSONValueObjectGetNumberInt(reply, return, ret);
 
 Doesn't the above function need to be checked for errors? That is, what if
 there's no return key in the reply? In json monitor code, we usually have
 something like the following code:
 
 if (virJSONValueObjectGetNumberInt(reply, return, ret)  0) {
 qemuReportError(VIR_ERR_INTERNAL_ERROR, %s,
 _(guest-fsfreeze-freeze reply was missing return
data));
 }
 

In fact this check is done in qemuAgentCheckError(); which reports error
as well. But I agree I should check if returned value is integer and
throw an error. I have squashed this into my local branch:

diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
index 3a5cc4b..9df5546 100644
--- a/src/qemu/qemu_agent.c
+++ b/src/qemu/qemu_agent.c
@@ -1137,7 +1137,10 @@ int qemuAgentFSFreeze(qemuAgentPtr mon)
 qemuAgentCheckError(cmd, reply)  0)
 goto cleanup;

-virJSONValueObjectGetNumberInt(reply, return, ret);
+if (virJSONValueObjectGetNumberInt(reply, return, ret)  0) {
+qemuReportError(VIR_ERR_INTERNAL_ERROR, %s,
+_(malformed return value));
+}

 cleanup:
 virJSONValueFree(cmd);
@@ -1171,7 +1174,10 @@ int qemuAgentFSThaw(qemuAgentPtr mon)
 qemuAgentCheckError(cmd, reply)  0)
 goto cleanup;

-virJSONValueObjectGetNumberInt(reply, return, ret);
+if (virJSONValueObjectGetNumberInt(reply, return, ret)  0) {
+qemuReportError(VIR_ERR_INTERNAL_ERROR, %s,
+_(malformed return value));
+}

 cleanup:
 virJSONValueFree(cmd);

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


Re: [libvirt] [PATCH 2/3] snapshots: Introduce VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE flag

2012-01-24 Thread Jiri Denemark
On Tue, Jan 24, 2012 at 18:37:44 +0100, Michal Privoznik wrote:
 With this flag, virDomainSnapshotCreate will use fs-freeze and
 fs-thaw guest agent commands to quiesce guest's disks.
 ---
  include/libvirt/libvirt.h.in |4 ++
  src/libvirt.c|6 ++
  src/qemu/qemu_driver.c   |  118 -
  3 files changed, 113 insertions(+), 15 deletions(-)
 
 diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
 index 5e6e488..182065d 100644
 --- a/include/libvirt/libvirt.h.in
 +++ b/include/libvirt/libvirt.h.in
 @@ -3125,6 +3125,10 @@ typedef enum {
system checkpoint 
 */
  VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT   = (1  5), /* reuse any existing
external files */
 +VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE = (1  6), /* use guest agent to
 +  quiesce all mounted
 +  file systems within
 +  the domain */

Do we also want to add another flag that would make quiescing optional? That
is, use it if it's available but don't fail if it's not.

  } virDomainSnapshotCreateFlags;
  
  /* Take a snapshot of the current VM state */
 diff --git a/src/libvirt.c b/src/libvirt.c
 index 96ad3d5..d9b8d88 100644
 --- a/src/libvirt.c
 +++ b/src/libvirt.c
 @@ -16602,6 +16602,12 @@ virDomainSnapshotGetConnect(virDomainSnapshotPtr 
 snapshot)
   * inconsistent (as if power had been pulled), and specifying this
   * with the VIR_DOMAIN_SNAPSHOT_CREATE_HALT flag risks data loss.
   *
 + * If @flags includes VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE, then the
 + * libvirt will attempt to use guest agent to freeze and thaw all
 + * file systems in use within domain OS. However, if the guest agent
 + * is not present, an error is trowed. Moreover, this flags requires

s/trowed/thrown/; s/flags/flag/

 + * VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY to be passed as well.
 + *
   * By default, if the snapshot involves external files, and any of the
   * destination files already exist as a regular file, the snapshot is
   * rejected to avoid losing contents of those files.  However, if
 diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
 index c81289a..14ad30b 100644
 --- a/src/qemu/qemu_driver.c
 +++ b/src/qemu/qemu_driver.c
 @@ -9461,6 +9461,56 @@ static int qemuDomainSnapshotIsAllowed(virDomainObjPtr 
 vm)
  return 1;
  }
  
 +static int
 +qemuDomainSnapshotFSFreeze(struct qemud_driver *driver,
 +   virDomainObjPtr vm) {
 +qemuDomainObjPrivatePtr priv = vm-privateData;
 +int freezed;
 +
 +if (priv-agentError) {
 +qemuReportError(VIR_ERR_INTERNAL_ERROR, %s,
 +_(QEMU guest agent is not 
 +  available due to an error));
 +return -1;
 +}
 +if (!priv-agent) {
 +qemuReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, %s,
 +_(QEMU guest agent is not configured));
 +return -1;
 +}
 +
 +qemuDomainObjEnterAgent(driver, vm);
 +freezed = qemuAgentFSFreeze(priv-agent);
 +qemuDomainObjExitAgent(driver, vm);
 +
 +return freezed;
 +}
 +
 +static int
 +qemuDomainSnapshotFSThaw(struct qemud_driver *driver,
 + virDomainObjPtr vm) {
 +qemuDomainObjPrivatePtr priv = vm-privateData;
 +int thawed;
 +
 +if (priv-agentError) {
 +qemuReportError(VIR_ERR_INTERNAL_ERROR, %s,
 +_(QEMU guest agent is not 
 +  available due to an error));
 +return -1;
 +}
 +if (!priv-agent) {
 +qemuReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, %s,
 +_(QEMU guest agent is not configured));
 +return -1;
 +}
 +
 +qemuDomainObjEnterAgent(driver, vm);
 +thawed = qemuAgentFSThaw(priv-agent);
 +qemuDomainObjExitAgent(driver, vm);
 +
 +return thawed;
 +}
 +
  /* The domain is expected to be locked and inactive. */
  static int
  qemuDomainSnapshotCreateInactive(struct qemud_driver *driver,
 @@ -9493,6 +9543,13 @@ qemuDomainSnapshotCreateActive(virConnectPtr conn,
  }
  
  if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_RUNNING) {
 +if ((flags  VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE) 
 +(qemuDomainSnapshotFSFreeze(driver, vm)  0)) {
 +qemuReportError(VIR_ERR_INTERNAL_ERROR, %s,
 +_(Unable to freeze domain's file systems));

This just masks useful error reported by qemuDomainSnapshotFSFreeze().

 +goto endjob;
 +}
 +
  /* savevm monitor command pauses the domain emitting an event which
   * confuses libvirt since it's not notified when qemu resumes the
   * domain. Thus we stop and start CPUs ourselves.
 @@ -9532,13 

[libvirt] [PATCH] lxc: export container=lxc-libvirt for systemd

2012-01-24 Thread Eric Blake
Systemd detects containers based on whether they have
an environment variable starting with 'container=lxc';
using a longer name fits the expectations, while also
allowing detection of who created the container.

Requested by Lennart Poettering, in response to
https://bugs.freedesktop.org/show_bug.cgi?id=45175

* src/lxc/lxc_container.c (lxcContainerBuildInitCmd): Add another
env-var.
---
 src/lxc/lxc_container.c |3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c
index c1ec9c4..70f6908 100644
--- a/src/lxc/lxc_container.c
+++ b/src/lxc/lxc_container.c
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2008-2011 Red Hat, Inc.
+ * Copyright (C) 2008-2012 Red Hat, Inc.
  * Copyright (C) 2008 IBM Corp.
  *
  * lxc_container.c: file description
@@ -124,6 +124,7 @@ static virCommandPtr 
lxcContainerBuildInitCmd(virDomainDefPtr vmDef)

 virCommandAddEnvString(cmd, PATH=/bin:/sbin);
 virCommandAddEnvString(cmd, TERM=linux);
+virCommandAddEnvString(cmd, container=lxc-libvirt);
 virCommandAddEnvPair(cmd, LIBVIRT_LXC_UUID, uuidstr);
 virCommandAddEnvPair(cmd, LIBVIRT_LXC_NAME, vmDef-name);
 if (vmDef-os.cmdline)
-- 
1.7.7.5

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


Re: [libvirt] [PATCH 3/3] virsh: Expose new VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE flag

2012-01-24 Thread Jiri Denemark
On Tue, Jan 24, 2012 at 18:37:45 +0100, Michal Privoznik wrote:
 to cmdSnapshotCreate and cmdSnapshotCreateAs.
 ---
  tools/virsh.c   |6 ++
  tools/virsh.pod |   14 --
  2 files changed, 18 insertions(+), 2 deletions(-)
 
 diff --git a/tools/virsh.c b/tools/virsh.c
 index 5560988..41c 100644
 --- a/tools/virsh.c
 +++ b/tools/virsh.c
 @@ -14662,6 +14662,7 @@ static const vshCmdOptDef opts_snapshot_create[] = {
  {halt, VSH_OT_BOOL, 0, N_(halt domain after snapshot is created)},
  {disk-only, VSH_OT_BOOL, 0, N_(capture disk state but not vm state)},
  {reuse-external, VSH_OT_BOOL, 0, N_(reuse any existing external 
 files)},
 +{quiesce, VSH_OT_BOOL, 0, N_(quiesce guest's file systems)},
  {NULL, 0, 0, NULL}
  };
  
 @@ -14686,6 +14687,8 @@ cmdSnapshotCreate(vshControl *ctl, const vshCmd *cmd)
  flags |= VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY;
  if (vshCommandOptBool(cmd, reuse-external))
  flags |= VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT;
 +if (vshCommandOptBool(cmd, quiesce))
 +flags |= VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE;
  
  if (!vshConnectionUsability(ctl, ctl-conn))
  goto cleanup;
 @@ -14795,6 +14798,7 @@ static const vshCmdOptDef opts_snapshot_create_as[] = 
 {
  {halt, VSH_OT_BOOL, 0, N_(halt domain after snapshot is created)},
  {disk-only, VSH_OT_BOOL, 0, N_(capture disk state but not vm state)},
  {reuse-external, VSH_OT_BOOL, 0, N_(reuse any existing external 
 files)},
 +{quiesce, VSH_OT_BOOL, 0, N_(quiesce guest's file systems)},
  {diskspec, VSH_OT_ARGV, 0,
   N_(disk attributes: disk[,snapshot=type][,driver=type][,file=name])},
  {NULL, 0, 0, NULL}
 @@ -14820,6 +14824,8 @@ cmdSnapshotCreateAs(vshControl *ctl, const vshCmd 
 *cmd)
  flags |= VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY;
  if (vshCommandOptBool(cmd, reuse-external))
  flags |= VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT;
 +if (vshCommandOptBool(cmd, quiesce))
 +flags |= VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE;
  
  if (!vshConnectionUsability(ctl, ctl-conn))
  goto cleanup;
 diff --git a/tools/virsh.pod b/tools/virsh.pod
 index 72c6d8f..b1f75de 100644
 --- a/tools/virsh.pod
 +++ b/tools/virsh.pod
 @@ -2046,7 +2046,8 @@ used to represent properties of snapshots.
  =over 4
  
  =item Bsnapshot-create Idomain [Ixmlfile] {[I--redefine 
 [I--current]]
 -| [I--no-metadata] [I--halt] [I--disk-only] [I--reuse-external]}
 +| [I--no-metadata] [I--halt] [I--disk-only] [I--reuse-external]
 +[I--quiesce]}
  
  Create a snapshot for domain Idomain with the properties specified in
  Ixmlfile.  Normally, the only properties settable for a domain snapshot
 @@ -2088,6 +2089,10 @@ external snapshot with a destination of an existing 
 file, then the
  existing file is truncated and reused; otherwise, a snapshot is refused
  to avoid losing contents of the existing files.
  
 +If I--quiesce is specified, libvirt will try to use guest agent
 +to freeze and unfreeze domain's mounted file systems. However,
 +currently this requires I--disk-only to be passed as well.
 +

I'm not a native speaker but I feel like libvirt will try to use guest agent
suggests that the snapshot will still be created even though guest agent
couldn't be used for freezing guest's file systems. I'd rather be explicit
about the fact that no guest agent implies no snapshot.

  Existence of snapshot metadata will prevent attempts to Bundefine
  a persistent domain.  However, for transient domains, snapshot
  metadata is silently lost when the domain quits running (whether
 @@ -2095,7 +2100,8 @@ by command such as Bdestroy or by internal guest 
 action).
  
  =item Bsnapshot-create-as Idomain {[I--print-xml]
  | [I--no-metadata] [I--halt] [I--reuse-existing]} [Iname]
 -[Idescription] [I--disk-only [[I--diskspec] Bdiskspec]...]
 +[Idescription] [I--disk-only [I--quiesce]
 +[[I--diskspec] Bdiskspec]...]
  
  Create a snapshot for domain Idomain with the given name and
  description; if either value is omitted, libvirt will choose a
 @@ -2123,6 +2129,10 @@ option requests an external snapshot with a 
 destination of an existing
  file, then the existing file is truncated and reused; otherwise, a
  snapshot is refused to avoid losing contents of the existing files.
  
 +If I--quiesce is specified, libvirt will try to use guest agent
 +to freeze and unfreeze domain's mounted file systems. However,
 +currently this requires I--disk-only to be passed as well.
 +

And here as well.

  If I--no-metadata is specified, then the snapshot data is created,
  but any metadata is immediately discarded (that is, libvirt does not
  treat the snapshot as current, and cannot revert to the snapshot

Jirka

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


Re: [libvirt] [PATCH 3/3] virsh: Expose new VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE flag

2012-01-24 Thread Michal Privoznik
On 24.01.2012 19:57, Jiri Denemark wrote:
 On Tue, Jan 24, 2012 at 18:37:45 +0100, Michal Privoznik wrote:
 to cmdSnapshotCreate and cmdSnapshotCreateAs.
 ---
  tools/virsh.c   |6 ++
  tools/virsh.pod |   14 --
  2 files changed, 18 insertions(+), 2 deletions(-)


 diff --git a/tools/virsh.pod b/tools/virsh.pod
 index 72c6d8f..b1f75de 100644
 --- a/tools/virsh.pod
 +++ b/tools/virsh.pod
 @@ -2046,7 +2046,8 @@ used to represent properties of snapshots.
  =over 4
  
  =item Bsnapshot-create Idomain [Ixmlfile] {[I--redefine 
 [I--current]]
 -| [I--no-metadata] [I--halt] [I--disk-only] [I--reuse-external]}
 +| [I--no-metadata] [I--halt] [I--disk-only] [I--reuse-external]
 +[I--quiesce]}
  
  Create a snapshot for domain Idomain with the properties specified in
  Ixmlfile.  Normally, the only properties settable for a domain snapshot
 @@ -2088,6 +2089,10 @@ external snapshot with a destination of an existing 
 file, then the
  existing file is truncated and reused; otherwise, a snapshot is refused
  to avoid losing contents of the existing files.
  
 +If I--quiesce is specified, libvirt will try to use guest agent
 +to freeze and unfreeze domain's mounted file systems. However,
 +currently this requires I--disk-only to be passed as well.
 +
 
 I'm not a native speaker but I feel like libvirt will try to use guest agent
 suggests that the snapshot will still be created even though guest agent
 couldn't be used for freezing guest's file systems. I'd rather be explicit
 about the fact that no guest agent implies no snapshot.
 

Okay, reworded in v2.

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


Re: [libvirt] [PATCH 2/3] snapshots: Introduce VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE flag

2012-01-24 Thread Michal Privoznik
On 24.01.2012 19:49, Jiri Denemark wrote:
 On Tue, Jan 24, 2012 at 18:37:44 +0100, Michal Privoznik wrote:
 With this flag, virDomainSnapshotCreate will use fs-freeze and
 fs-thaw guest agent commands to quiesce guest's disks.
 ---
  include/libvirt/libvirt.h.in |4 ++
  src/libvirt.c|6 ++
  src/qemu/qemu_driver.c   |  118 
 -
  3 files changed, 113 insertions(+), 15 deletions(-)

 diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
 index 5e6e488..182065d 100644
 --- a/include/libvirt/libvirt.h.in
 +++ b/include/libvirt/libvirt.h.in
 @@ -3125,6 +3125,10 @@ typedef enum {
system checkpoint 
 */
  VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT   = (1  5), /* reuse any existing
external files */
 +VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE = (1  6), /* use guest agent to
 +  quiesce all 
 mounted
 +  file systems 
 within
 +  the domain */
 
 Do we also want to add another flag that would make quiescing optional? That
 is, use it if it's available but don't fail if it's not.

If this is ever needed we can add it then.

 @@ -9493,6 +9543,13 @@ qemuDomainSnapshotCreateActive(virConnectPtr conn,
  }
  
  if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_RUNNING) {
 +if ((flags  VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE) 
 +(qemuDomainSnapshotFSFreeze(driver, vm)  0)) {
 +qemuReportError(VIR_ERR_INTERNAL_ERROR, %s,
 +_(Unable to freeze domain's file systems));
 
 This just masks useful error reported by qemuDomainSnapshotFSFreeze().

In fact, I just realized, this is dead code. Since we are checking for
DISK_ONLY flag which implies calling
qemuDomainSnapshotCreateDiskActive() we will never get this condition to
evaluate as true. I'll drop it in v2.

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


[libvirt] [PATCH v2 1/3] qemu_agent: Create file system freeze and thaw functions

2012-01-24 Thread Michal Privoznik
These functions simply issue command to guest agent which
should freeze or unfreeze all file systems within guest.
---
 src/qemu/qemu_agent.c |   74 +
 src/qemu/qemu_agent.h |3 ++
 2 files changed, 77 insertions(+), 0 deletions(-)

diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
index 6a7c7b3..9df5546 100644
--- a/src/qemu/qemu_agent.c
+++ b/src/qemu/qemu_agent.c
@@ -1110,3 +1110,77 @@ int qemuAgentShutdown(qemuAgentPtr mon,
 virJSONValueFree(reply);
 return ret;
 }
+
+/*
+ * qemuAgentFSFreeze:
+ * @mon: Agent
+ *
+ * Issue guest-fsfreeze-freeze command to guest agent,
+ * which freezes all mounted file systems and returns
+ * number of frozen file systems on success.
+ *
+ * Returns: number of file system frozen on success,
+ *  -1 on error.
+ */
+int qemuAgentFSFreeze(qemuAgentPtr mon)
+{
+int ret = -1;
+virJSONValuePtr cmd;
+virJSONValuePtr reply = NULL;
+
+cmd = qemuAgentMakeCommand(guest-fsfreeze-freeze, NULL);
+
+if (!cmd)
+return -1;
+
+if (qemuAgentCommand(mon, cmd, reply)  0 ||
+qemuAgentCheckError(cmd, reply)  0)
+goto cleanup;
+
+if (virJSONValueObjectGetNumberInt(reply, return, ret)  0) {
+qemuReportError(VIR_ERR_INTERNAL_ERROR, %s,
+_(malformed return value));
+}
+
+cleanup:
+virJSONValueFree(cmd);
+virJSONValueFree(reply);
+return ret;
+}
+
+/*
+ * qemuAgentFSThaw:
+ * @mon: Agent
+ *
+ * Issue guest-fsfreeze-thaw command to guest agent,
+ * which unfreezes all mounted file systems and returns
+ * number of thawed file systems on success.
+ *
+ * Returns: number of file system thawed on success,
+ *  -1 on error.
+ */
+int qemuAgentFSThaw(qemuAgentPtr mon)
+{
+int ret = -1;
+virJSONValuePtr cmd;
+virJSONValuePtr reply = NULL;
+
+cmd = qemuAgentMakeCommand(guest-fsfreeze-thaw, NULL);
+
+if (!cmd)
+return -1;
+
+if (qemuAgentCommand(mon, cmd, reply)  0 ||
+qemuAgentCheckError(cmd, reply)  0)
+goto cleanup;
+
+if (virJSONValueObjectGetNumberInt(reply, return, ret)  0) {
+qemuReportError(VIR_ERR_INTERNAL_ERROR, %s,
+_(malformed return value));
+}
+
+cleanup:
+virJSONValueFree(cmd);
+virJSONValueFree(reply);
+return ret;
+}
diff --git a/src/qemu/qemu_agent.h b/src/qemu/qemu_agent.h
index 93c2ae7..df59ef7 100644
--- a/src/qemu/qemu_agent.h
+++ b/src/qemu/qemu_agent.h
@@ -66,4 +66,7 @@ typedef enum {
 int qemuAgentShutdown(qemuAgentPtr mon,
   qemuAgentShutdownMode mode);
 
+int qemuAgentFSFreeze(qemuAgentPtr mon);
+int qemuAgentFSThaw(qemuAgentPtr mon);
+
 #endif /* __QEMU_AGENT_H__ */
-- 
1.7.3.4

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


[libvirt] [PATCH v2 0/3] Use guest agent to quiesce disks

2012-01-24 Thread Michal Privoznik
Since we have qemu guest agent support in libvirt,
we can start wiring up some things that GA already
knows how to do. One of them is file system freeze
and thaw. Domain snapshots can profit from this
functionality.

Michal Privoznik (3):
  qemu_agent: Create file system freeze and thaw functions
  snapshots: Introduce VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE flag
  virsh: Expose new VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE flag

 include/libvirt/libvirt.h.in |4 ++
 src/libvirt.c|6 +++
 src/qemu/qemu_agent.c|   74 +++
 src/qemu/qemu_agent.h|3 +
 src/qemu/qemu_driver.c   |   87 ++
 tools/virsh.c|6 +++
 tools/virsh.pod  |   16 +++-
 7 files changed, 186 insertions(+), 10 deletions(-)

-- 
1.7.3.4

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


[libvirt] [PATCH v2 2/3] snapshots: Introduce VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE flag

2012-01-24 Thread Michal Privoznik
With this flag, virDomainSnapshotCreate will use fs-freeze and
fs-thaw guest agent commands to quiesce guest's disks.
---
 include/libvirt/libvirt.h.in |4 ++
 src/libvirt.c|6 +++
 src/qemu/qemu_driver.c   |   87 ++
 3 files changed, 89 insertions(+), 8 deletions(-)

diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
index 5e6e488..182065d 100644
--- a/include/libvirt/libvirt.h.in
+++ b/include/libvirt/libvirt.h.in
@@ -3125,6 +3125,10 @@ typedef enum {
   system checkpoint */
 VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT   = (1  5), /* reuse any existing
   external files */
+VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE = (1  6), /* use guest agent to
+  quiesce all mounted
+  file systems within
+  the domain */
 } virDomainSnapshotCreateFlags;
 
 /* Take a snapshot of the current VM state */
diff --git a/src/libvirt.c b/src/libvirt.c
index 96ad3d5..722a2e2 100644
--- a/src/libvirt.c
+++ b/src/libvirt.c
@@ -16602,6 +16602,12 @@ virDomainSnapshotGetConnect(virDomainSnapshotPtr 
snapshot)
  * inconsistent (as if power had been pulled), and specifying this
  * with the VIR_DOMAIN_SNAPSHOT_CREATE_HALT flag risks data loss.
  *
+ * If @flags includes VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE, then the
+ * libvirt will attempt to use guest agent to freeze and thaw all
+ * file systems in use within domain OS. However, if the guest agent
+ * is not present, an error is thrown. Moreover, this flag requires
+ * VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY to be passed as well.
+ *
  * By default, if the snapshot involves external files, and any of the
  * destination files already exist as a regular file, the snapshot is
  * rejected to avoid losing contents of those files.  However, if
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index c81289a..ab69dca 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -9461,6 +9461,56 @@ static int qemuDomainSnapshotIsAllowed(virDomainObjPtr 
vm)
 return 1;
 }
 
+static int
+qemuDomainSnapshotFSFreeze(struct qemud_driver *driver,
+   virDomainObjPtr vm) {
+qemuDomainObjPrivatePtr priv = vm-privateData;
+int freezed;
+
+if (priv-agentError) {
+qemuReportError(VIR_ERR_INTERNAL_ERROR, %s,
+_(QEMU guest agent is not 
+  available due to an error));
+return -1;
+}
+if (!priv-agent) {
+qemuReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, %s,
+_(QEMU guest agent is not configured));
+return -1;
+}
+
+qemuDomainObjEnterAgent(driver, vm);
+freezed = qemuAgentFSFreeze(priv-agent);
+qemuDomainObjExitAgent(driver, vm);
+
+return freezed;
+}
+
+static int
+qemuDomainSnapshotFSThaw(struct qemud_driver *driver,
+ virDomainObjPtr vm) {
+qemuDomainObjPrivatePtr priv = vm-privateData;
+int thawed;
+
+if (priv-agentError) {
+qemuReportError(VIR_ERR_INTERNAL_ERROR, %s,
+_(QEMU guest agent is not 
+  available due to an error));
+return -1;
+}
+if (!priv-agent) {
+qemuReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, %s,
+_(QEMU guest agent is not configured));
+return -1;
+}
+
+qemuDomainObjEnterAgent(driver, vm);
+thawed = qemuAgentFSThaw(priv-agent);
+qemuDomainObjExitAgent(driver, vm);
+
+return thawed;
+}
+
 /* The domain is expected to be locked and inactive. */
 static int
 qemuDomainSnapshotCreateInactive(struct qemud_driver *driver,
@@ -9773,6 +9823,12 @@ qemuDomainSnapshotCreateDiskActive(virConnectPtr conn,
 
 
 if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_RUNNING) {
+if ((flags  VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE) 
+(qemuDomainSnapshotFSFreeze(driver, vm)  0)) {
+/* helper reported the error */
+goto endjob;
+}
+
 /* In qemu, snapshot_blkdev on a single disk will pause cpus,
  * but this confuses libvirt since notifications are not given
  * when qemu resumes.  And for multiple disks, libvirt must
@@ -9840,13 +9896,20 @@ qemuDomainSnapshotCreateDiskActive(virConnectPtr conn,
 }
 
 cleanup:
-if (resume  virDomainObjIsActive(vm) 
-qemuProcessStartCPUs(driver, vm, conn,
- VIR_DOMAIN_RUNNING_UNPAUSED,
- QEMU_ASYNC_JOB_NONE)  0 
-virGetLastError() == NULL) {
-qemuReportError(VIR_ERR_OPERATION_FAILED, %s,
-_(resuming after snapshot failed));
+if (resume  virDomainObjIsActive(vm)) {
+if 

[libvirt] [PATCH v2 3/3] virsh: Expose new VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE flag

2012-01-24 Thread Michal Privoznik
to cmdSnapshotCreate and cmdSnapshotCreateAs.
---
 tools/virsh.c   |6 ++
 tools/virsh.pod |   16 ++--
 2 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/tools/virsh.c b/tools/virsh.c
index 5560988..41c 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -14662,6 +14662,7 @@ static const vshCmdOptDef opts_snapshot_create[] = {
 {halt, VSH_OT_BOOL, 0, N_(halt domain after snapshot is created)},
 {disk-only, VSH_OT_BOOL, 0, N_(capture disk state but not vm state)},
 {reuse-external, VSH_OT_BOOL, 0, N_(reuse any existing external 
files)},
+{quiesce, VSH_OT_BOOL, 0, N_(quiesce guest's file systems)},
 {NULL, 0, 0, NULL}
 };
 
@@ -14686,6 +14687,8 @@ cmdSnapshotCreate(vshControl *ctl, const vshCmd *cmd)
 flags |= VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY;
 if (vshCommandOptBool(cmd, reuse-external))
 flags |= VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT;
+if (vshCommandOptBool(cmd, quiesce))
+flags |= VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE;
 
 if (!vshConnectionUsability(ctl, ctl-conn))
 goto cleanup;
@@ -14795,6 +14798,7 @@ static const vshCmdOptDef opts_snapshot_create_as[] = {
 {halt, VSH_OT_BOOL, 0, N_(halt domain after snapshot is created)},
 {disk-only, VSH_OT_BOOL, 0, N_(capture disk state but not vm state)},
 {reuse-external, VSH_OT_BOOL, 0, N_(reuse any existing external 
files)},
+{quiesce, VSH_OT_BOOL, 0, N_(quiesce guest's file systems)},
 {diskspec, VSH_OT_ARGV, 0,
  N_(disk attributes: disk[,snapshot=type][,driver=type][,file=name])},
 {NULL, 0, 0, NULL}
@@ -14820,6 +14824,8 @@ cmdSnapshotCreateAs(vshControl *ctl, const vshCmd *cmd)
 flags |= VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY;
 if (vshCommandOptBool(cmd, reuse-external))
 flags |= VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT;
+if (vshCommandOptBool(cmd, quiesce))
+flags |= VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE;
 
 if (!vshConnectionUsability(ctl, ctl-conn))
 goto cleanup;
diff --git a/tools/virsh.pod b/tools/virsh.pod
index 72c6d8f..b6962cf 100644
--- a/tools/virsh.pod
+++ b/tools/virsh.pod
@@ -2046,7 +2046,8 @@ used to represent properties of snapshots.
 =over 4
 
 =item Bsnapshot-create Idomain [Ixmlfile] {[I--redefine [I--current]]
-| [I--no-metadata] [I--halt] [I--disk-only] [I--reuse-external]}
+| [I--no-metadata] [I--halt] [I--disk-only] [I--reuse-external]
+[I--quiesce]}
 
 Create a snapshot for domain Idomain with the properties specified in
 Ixmlfile.  Normally, the only properties settable for a domain snapshot
@@ -2088,6 +2089,11 @@ external snapshot with a destination of an existing 
file, then the
 existing file is truncated and reused; otherwise, a snapshot is refused
 to avoid losing contents of the existing files.
 
+If I--quiesce is specified, libvirt will try to use guest agent
+to freeze and unfreeze domain's mounted file systems. However,
+if domain has no guest agent, snapshot creation will fail.
+Currently, this requires I--disk-only to be passed as well.
+
 Existence of snapshot metadata will prevent attempts to Bundefine
 a persistent domain.  However, for transient domains, snapshot
 metadata is silently lost when the domain quits running (whether
@@ -2095,7 +2101,8 @@ by command such as Bdestroy or by internal guest 
action).
 
 =item Bsnapshot-create-as Idomain {[I--print-xml]
 | [I--no-metadata] [I--halt] [I--reuse-existing]} [Iname]
-[Idescription] [I--disk-only [[I--diskspec] Bdiskspec]...]
+[Idescription] [I--disk-only [I--quiesce]
+[[I--diskspec] Bdiskspec]...]
 
 Create a snapshot for domain Idomain with the given name and
 description; if either value is omitted, libvirt will choose a
@@ -2123,6 +2130,11 @@ option requests an external snapshot with a destination 
of an existing
 file, then the existing file is truncated and reused; otherwise, a
 snapshot is refused to avoid losing contents of the existing files.
 
+If I--quiesce is specified, libvirt will try to use guest agent
+to freeze and unfreeze domain's mounted file systems. However,
+if domain has no guest agent, snapshot creation will fail.
+Currently, this requires I--disk-only to be passed as well.
+
 If I--no-metadata is specified, then the snapshot data is created,
 but any metadata is immediately discarded (that is, libvirt does not
 treat the snapshot as current, and cannot revert to the snapshot
-- 
1.7.3.4

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


Re: [libvirt] [PATCH 7/7] Set a security context on /dev and /dev/pts mounts

2012-01-24 Thread Eric Blake
On 01/11/2012 09:33 AM, Daniel P. Berrange wrote:
 From: Daniel P. Berrange berra...@redhat.com
 
 To allow the container to access /dev and /dev/pts when under
 sVirt, set an explicit mount option. Also set a max size on
 the /dev mount to prevent DOS on memory usage
 
 * src/lxc/lxc_container.c: Set /dev mount context
 * src/lxc/lxc_controller.c: Set /dev/pts mount context
 ---
  src/lxc/lxc_container.c  |   75 +++--
  src/lxc/lxc_controller.c |   43 +++---
  2 files changed, 96 insertions(+), 22 deletions(-)
 

 +} else {
 +#endif
 +/*
 + * tmpfs is limited to 64kb, since we only have device nodes in 
 there
 + * and don't want to DOS the entire OS RAM usage
 + */
 +if (virAsprintf(opts, mode=755,size=65536%%%s%s%s,

Ouch.  size=65536% is _not_ what you want; you either want size=65536 or
something like size=10%.

 +con ? ,context=\ : ,
 +con ? (const char *)con : ,
 +con ? \ : )  0) {

I would have split this:

if (virAsprintf(opts, mode=755,size=65536)  0 ||
(con  virAsprintf(opts, ,context=\%s\,
(const char *)con)  0)) {

 +virReportOOMError();
 +goto cleanup;
 +}
 +#if HAVE_SELINUX
 +}
 +#endif

You don't need this second #if.  That is, instead of writing:

#if HAVE_SELINUX
if (condition) {
goto cleanup;
} else {
#endif
stuff;
#if HAVE_SELINUX
}
#endif

I would have done:

#if HAVE_SELINUX
if (condition) {
goto cleanup;
}
#endif
stuff;

 @@ -1373,16 +1380,42 @@ lxcControllerRun(virDomainDefPtr def,
  goto cleanup;
  }
  
 -/* XXX should we support gid=X for X!=5 for distros which use
 - * a different gid for tty?  */
 -VIR_DEBUG(Mounting 'devpts' on %s, devpts);
 -if (mount(devpts, devpts, devpts, 0,
 -  newinstance,ptmxmode=0666,mode=0620,gid=5)  0) {
 +#if HAVE_SELINUX
 +if (getfilecon(root-src, con)  0 
 +errno != ENOTSUP) {
 +virReportSystemError(errno,
 + _(Failed to query file context on %s),
 + root-src);
 +goto cleanup;
 +} else {
 +#endif
 +/*
 + * tmpfs is limited to 64kb, since we only have device nodes in 
 there
 + * and don't want to DOS the entire OS RAM usage
 + */

Is this comment really relative to the devpts mount point?

 +/* XXX should we support gid=X for X!=5 for distros which use
 + * a different gid for tty?  */
 +if (virAsprintf(opts, 
 newinstance,ptmxmode=0666,mode=0620,gid=5%s%s%s,
 +con ? ,context=\ : ,
 +con ? (const char *)con : ,
 +con ? \ : )  0) {
 +virReportOOMError();
 +goto cleanup;
 +}
 +#if HAVE_SELINUX
 +}
 +#endif

Same formatting nit about not needing a second #if.

-- 
Eric Blake   ebl...@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 7/7] Set a security context on /dev and /dev/pts mounts

2012-01-24 Thread Eric Blake
On 01/24/2012 01:21 PM, Eric Blake wrote:
 +if (virAsprintf(opts, mode=755,size=65536%%%s%s%s,
 +con ? ,context=\ : ,
 +con ? (const char *)con : ,
 +con ? \ : )  0) {
 
 I would have split this:
 
 if (virAsprintf(opts, mode=755,size=65536)  0 ||
 (con  virAsprintf(opts, ,context=\%s\,
 (const char *)con)  0)) {

Never mind - that doesn't work; likewise, I don't think we have any
guarantees about self-modifying strings such as:

if (virAsprintf(opts, mode=755,size=65536)  0 ||
(con  virAsprintf(opts, %s,context=\%s\,
opts, (const char *)con)  0)) {

I guess I was thinking virBufferAsprintf, where appending is indeed
easier than doing it in one shot.

-- 
Eric Blake   ebl...@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] qemu: Emit bootindex even for direct boot

2012-01-24 Thread Jiri Denemark
Direct boot (using kernel, initrd, and command line) is used by
virt-install/virt-manager for network install. While any bootindex has
no direct effect since -kernel is always first, we need it as a hint for
SeaBIOS to present disks in the same order as they will be presented
during normal boot.
---
 src/qemu/qemu_command.c |3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index aaccf62..9b69ad0 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -4334,8 +4334,7 @@ qemuBuildCommandLine(virConnectPtr conn,
 if (qemuCapsGet(qemuCaps, QEMU_CAPS_DRIVE)) {
 int bootCD = 0, bootFloppy = 0, bootDisk = 0;
 
-if ((qemuCapsGet(qemuCaps, QEMU_CAPS_DRIVE_BOOT) || emitBootindex) 
-!def-os.kernel) {
+if ((qemuCapsGet(qemuCaps, QEMU_CAPS_DRIVE_BOOT) || emitBootindex)) {
 /* bootDevs will get translated into either bootindex=N or boot=on
  * depending on what qemu supports */
 for (i = 0 ; i  def-os.nBootDevs ; i++) {
-- 
1.7.8.4

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


Re: [libvirt] [PATCH] qemu: Emit bootindex even for direct boot

2012-01-24 Thread Eric Blake
On 01/24/2012 03:53 PM, Jiri Denemark wrote:
 Direct boot (using kernel, initrd, and command line) is used by
 virt-install/virt-manager for network install. While any bootindex has
 no direct effect since -kernel is always first, we need it as a hint for
 SeaBIOS to present disks in the same order as they will be presented
 during normal boot.
 ---
  src/qemu/qemu_command.c |3 +--
  1 files changed, 1 insertions(+), 2 deletions(-)
 
 diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
 index aaccf62..9b69ad0 100644
 --- a/src/qemu/qemu_command.c
 +++ b/src/qemu/qemu_command.c
 @@ -4334,8 +4334,7 @@ qemuBuildCommandLine(virConnectPtr conn,
  if (qemuCapsGet(qemuCaps, QEMU_CAPS_DRIVE)) {
  int bootCD = 0, bootFloppy = 0, bootDisk = 0;
  
 -if ((qemuCapsGet(qemuCaps, QEMU_CAPS_DRIVE_BOOT) || emitBootindex) 
 -!def-os.kernel) {
 +if ((qemuCapsGet(qemuCaps, QEMU_CAPS_DRIVE_BOOT) || emitBootindex)) {

ACK.

-- 
Eric Blake   ebl...@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 v2 1/3] qemu_agent: Create file system freeze and thaw functions

2012-01-24 Thread MATSUDA, Daiki

I am happy that you provide the patches.


These functions simply issue command to guest agent which
should freeze or unfreeze all file systems within guest.
---
  src/qemu/qemu_agent.c |   74 +
  src/qemu/qemu_agent.h |3 ++
  2 files changed, 77 insertions(+), 0 deletions(-)

diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
index 6a7c7b3..9df5546 100644
--- a/src/qemu/qemu_agent.c
+++ b/src/qemu/qemu_agent.c
@@ -1110,3 +1110,77 @@ int qemuAgentShutdown(qemuAgentPtr mon,
  virJSONValueFree(reply);
  return ret;
  }
+
+/*
+ * qemuAgentFSFreeze:
+ * @mon: Agent
+ *
+ * Issue guest-fsfreeze-freeze command to guest agent,
+ * which freezes all mounted file systems and returns
+ * number of frozen file systems on success.
+ *
+ * Returns: number of file system frozen on success,
+ *  -1 on error.
+ */
+int qemuAgentFSFreeze(qemuAgentPtr mon)
+{
+int ret = -1;
+virJSONValuePtr cmd;
+virJSONValuePtr reply = NULL;
+
+cmd = qemuAgentMakeCommand(guest-fsfreeze-freeze, NULL);
+
+if (!cmd)
+return -1;
+
+if (qemuAgentCommand(mon, cmd,reply)  0 ||
+qemuAgentCheckError(cmd, reply)  0)
+goto cleanup;
+
+if (virJSONValueObjectGetNumberInt(reply, return,ret)  0) {
+qemuReportError(VIR_ERR_INTERNAL_ERROR, %s,
+_(malformed return value));
+}
+
+cleanup:
+virJSONValueFree(cmd);
+virJSONValueFree(reply);
+return ret;
+}
+
+/*
+ * qemuAgentFSThaw:
+ * @mon: Agent
+ *
+ * Issue guest-fsfreeze-thaw command to guest agent,
+ * which unfreezes all mounted file systems and returns
+ * number of thawed file systems on success.
+ *
+ * Returns: number of file system thawed on success,
+ *  -1 on error.
+ */
+int qemuAgentFSThaw(qemuAgentPtr mon)
+{
+int ret = -1;
+virJSONValuePtr cmd;
+virJSONValuePtr reply = NULL;
+
+cmd = qemuAgentMakeCommand(guest-fsfreeze-thaw, NULL);
+
+if (!cmd)
+return -1;
+
+if (qemuAgentCommand(mon, cmd,reply)  0 ||
+qemuAgentCheckError(cmd, reply)  0)
+goto cleanup;
+
+if (virJSONValueObjectGetNumberInt(reply, return,ret)  0) {
+qemuReportError(VIR_ERR_INTERNAL_ERROR, %s,
+_(malformed return value));
+}
+
+cleanup:
+virJSONValueFree(cmd);
+virJSONValueFree(reply);
+return ret;
+}


But qemuAgentFSFreeze() and qemuAgentFSThaw() are almost same. In 
addition, qemu guest agent provides status option for FS Freezer. So, 
could you change to following?


VIR_ENUM_DECL(qemuAgentFSFreezeCtlMode);

VIR_ENUM_IMPL(qemuAgentFSFreezeCtlMode,
  QEMU_AGENT_FSFREEZE_LAST,
  guest-fsfreeze-freeze,
  guest-fsfreeze-thaw,
  guest-fsfreeze-status);

int qemuAgentFSFreezeCtl(qemuAGentPtr mon, qemuAgentFSFreezeCtlMode mode){
...
cmd = qemuAgentMakeCommand(qemuAgentFSFreezeCtlModeToString(mode), 
NULL);

...


diff --git a/src/qemu/qemu_agent.h b/src/qemu/qemu_agent.h
index 93c2ae7..df59ef7 100644
--- a/src/qemu/qemu_agent.h
+++ b/src/qemu/qemu_agent.h
@@ -66,4 +66,7 @@ typedef enum {
  int qemuAgentShutdown(qemuAgentPtr mon,
qemuAgentShutdownMode mode);

+int qemuAgentFSFreeze(qemuAgentPtr mon);
+int qemuAgentFSThaw(qemuAgentPtr mon);
+
  #endif /* __QEMU_AGENT_H__ */


Similarly
int qemuAgentFSFreezeCtl(qemuAgentPtr mon, qemuAgentFSFreezeCtlMode mode);

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


Re: [libvirt] [PATCH v2 1/3] qemu_agent: Create file system freeze and thaw functions

2012-01-24 Thread Eric Blake
On 01/24/2012 04:17 PM, MATSUDA, Daiki wrote:
 I am happy that you provide the patches.
 
 These functions simply issue command to guest agent which
 should freeze or unfreeze all file systems within guest.
 ---

 
 But qemuAgentFSFreeze() and qemuAgentFSThaw() are almost same. In
 addition, qemu guest agent provides status option for FS Freezer. So,
 could you change to following?
 
 VIR_ENUM_DECL(qemuAgentFSFreezeCtlMode);
 
 VIR_ENUM_IMPL(qemuAgentFSFreezeCtlMode,
   QEMU_AGENT_FSFREEZE_LAST,
   guest-fsfreeze-freeze,
   guest-fsfreeze-thaw,
   guest-fsfreeze-status);

That would make sense if we planned on exposing guest freeze and thaw
directly to the user.  But right now, we are only planning on using
guest freeze and thaw internally as part of our higher-level snapshot
create function, where we do freeze and thaw in a balanced pair, and
where our command is blocking so that there is no need to call status
(that is, if we exposed a libvirt command to query the status, you would
only ever be able to call it when we are not in the middle of a
snapshot, and thus the fs would always report that it is in a thawed
state, unless someone has gone behind libvirt's back, at which point why
are you using libvirt).

Maybe you can convince me otherwise that we need to expose these three
lower-level states to the user, rather than using them internally only
for the implementation of the higher-level commands like
snapshot-create, but unless we have a reason to expose this much detail,
I don't think we need to make your proposed change.

-- 
Eric Blake   ebl...@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] Allow custom metadata in domain configuration XML

2012-01-24 Thread Eric Blake
On 01/23/2012 07:26 PM, Zeeshan Ali (Khattak) wrote:
 From: Zeeshan Ali (Khattak) zeesha...@gnome.org
 
 Applications can now insert custom nodes and hierarchies into domain
 cofiguration XML. Although currently not enforced, application are

s/cofiguration/configuration/
s/application are/applications are/

 required to use their own namespaces on every custom node they insert.

I also mentioned that we request only one element per namespace, in
light of Dan's proposal for a new API that can get and set metadata on a
per-namespace basis, so that applications don't have to filter all
elements for the one they are interested in:

https://www.redhat.com/archives/libvir-list/2012-January/msg00902.html

 ---
  docs/formatdomain.html.in  |   18 +
  docs/schemas/domaincommon.rng  |   26 +
  src/conf/domain_conf.c |   24 
  src/conf/domain_conf.h |3 ++
  tests/domainsnapshotxml2xmlout/metadata.xml|   38 
 
  tests/domainsnapshotxml2xmltest.c  |1 +
  tests/qemuxml2argvdata/qemuxml2argv-metadata.args  |4 ++
  tests/qemuxml2argvdata/qemuxml2argv-metadata.xml   |   29 +++
  .../qemuxml2xmloutdata/qemuxml2xmlout-metadata.xml |   29 +++
  tests/qemuxml2xmltest.c|2 +

Thanks for taking in my suggestions.  'git send-email
--subject-prefix=PATCHv2' makes it easier to state when you are sending
a v2.

I added you to AUTHORS; let me know if I need to update the spelling to
match your preferred listing.

 +pre
 + ...
 +  lt;metadatagt;
 +lt;app1:foo xmlns:app1=http://app1.org/app1/gt;..lt;/app1:foogt;
 +lt;app2:bar xmlns:app2=http://app1.org/app2/gt;..lt;/app2:bargt;
 +  lt;/metadatagt;
 +  .../pre

Someday, we might try to list actual metadata (like danpb's suggestion
of virtman:guest xmlns:virtman=http://virt-manager.org/guets/1.0;;);
but I'm okay waiting for some implementation experience before listing
something that actually appears in typical distro setups.

 +
 +dl
 +  dtcodemetadata/code/dt
 +  ddcodemetadata/code node could be used by applications to
 +  store custom metadata in the form of XML nodes/trees. Applications
 +  must use custom namespaces on their XML nodes/trees.
 +  span class=sinceSince 0.9.9/span/dd

since 0.9.10; and document the further restriction of only one element
per namespace.

I also think that this section belongs up next to description; but
I'll move it as a followup patch.

 +++ b/docs/schemas/domaincommon.rng
 @@ -43,6 +43,9 @@
ref name=seclabel/
  /optional
  optional
 +  ref name=metadata/
 +/optional
 +optional

I'm moving this up next to description now.  That is, both description
and metadata are overall metadata about the domain, and thus belong
next to one another.

ref name='qemucmdline'/
  /optional
/interleave
 @@ -2942,6 +2945,29 @@
  /element
/define
  
 +  define name=metadata
 +element name=metadata
 +  zeroOrMore
 +  ref name=customElement/

Indentation.

 +++ b/src/conf/domain_conf.c
 @@ -1500,6 +1500,9 @@ void virDomainDefFree(virDomainDefPtr def)
  if (def-namespaceData  def-ns.free)
  (def-ns.free)(def-namespaceData);
  
 +if (def-metadata)
 +xmlFreeNode(def-metadata);

Useless use of if before free, since xmlFreeNode is documented as a
no-op on NULL.  I'll update our list of function names to include
xmlFreeNode(), so that 'make syntax-check' will prevent this in the
future, as a followup patch.

 +
  VIR_FREE(def);
  }
  
 @@ -8072,6 +8075,11 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr 
 caps,
  def-os.smbios_mode = VIR_DOMAIN_SMBIOS_NONE; /* not present */
  }
  
 +/* Extract custom metadata */
 +if ((node = virXPathNode(./metadata[1], ctxt)) != NULL) {
 +def-metadata = xmlCopyNode (node, 1);

Style - libvirt doesn't use space before ( in function calls.

 @@ -11833,6 +11841,22 @@ virDomainDefFormatInternal(virDomainDefPtr def,
  goto cleanup;
  }
  
 +/* Custom metadata comes at the end */
 +if (def-metadata) {
 +xmlBufferPtr xmlbuf;

I'll move this earlier in a subsequent patch.

 +
 +xmlbuf = xmlBufferCreate();
 +if (xmlNodeDump(xmlbuf, def-metadata-doc, def-metadata, 4, 1)  
 0) {

I'm still not convinced you got this right.

/me 2 hours later, after reading libxml2 docs

Nope.  The problem is that for indentation on output to work, we have to
explicitly request ignoring extra whitespace on input, via
xmlKeepBlanksDefault() (it defaults to 1, but must be 0 during the
parse).  Furthermore, the indentation generated by libxml defaults to 2
spaces per 1 level, which means that we need virBufferGetIndent / 2, to
properly adjust things.

 +xmlBufferFree(xmlbuf);
 +  

[libvirt] [PATCH] build: simplify xmlFreeNode usage

2012-01-24 Thread Eric Blake
Noticed while reviewing the previous patch; thankfully, there
are no violations.

* cfg.mk (useless_free_options): Add xmlFreeNode.
---

  +if (def-metadata)
  +xmlFreeNode(def-metadata);
 Useless use of if before free, since xmlFreeNode is documented as a
 no-op on NULL.  I'll update our list of function names to include
 xmlFreeNode(), so that 'make syntax-check' will prevent this in the
 future, as a followup patch.

Pushing under the trivial rule.

 cfg.mk |1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/cfg.mk b/cfg.mk
index 20a085e..d853caf 100644
--- a/cfg.mk
+++ b/cfg.mk
@@ -171,6 +171,7 @@ useless_free_options =  \
   --name=xmlBufferFree \
   --name=xmlFree   \
   --name=xmlFreeDoc\
+  --name=xmlFreeNode   \
   --name=xmlXPathFreeContext   \
   --name=xmlXPathFreeObject

-- 
1.7.7.5

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


[libvirt] [PATCH] metadata: group metadata next to description

2012-01-24 Thread Eric Blake
It's better to group all the metadata together.  This is a
cosmetic output change; since the RNG allows interleave, it
doesn't matter where the user stuck it on input, and an XPath
query will find it in the same location when parsing the output.

* src/conf/domain_conf.c (virDomainDefFormatInternal): Output
metadata earlier.
* docs/formatdomain.html.in: Update documentation.
* tests/domainsnapshotxml2xmlout/metadata.xml: Update test.
* tests/qemuxml2xmloutdata/qemuxml2xmlout-metadata.xml: Likewise.
---

As threatened here:

 I also think that this section belongs up next to description; but
 I'll move it as a followup patch.

 docs/formatdomain.html.in  |   39 +++--
 src/conf/domain_conf.c |   47 ++--
 tests/domainsnapshotxml2xmlout/metadata.xml|8 ++--
 .../qemuxml2xmloutdata/qemuxml2xmlout-metadata.xml |8 ++--
 4 files changed, 47 insertions(+), 55 deletions(-)

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 6b025e8..dca9a81 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -33,6 +33,10 @@
   lt;namegt;fv0lt;/namegt;
   lt;uuidgt;4dea22b31d52d8f32516782e98ab3fa0lt;/uuidgt;
   lt;descriptiongt;Some human readable descriptionlt;/descriptiongt;
+  lt;metadatagt;
+lt;app1:foo xmlns:app1=http://app1.org/app1/gt;..lt;/app1:foogt;
+lt;app2:bar xmlns:app2=http://app1.org/app2/gt;..lt;/app2:bargt;
+  lt;/metadatagt;
   .../pre

 dl
@@ -56,9 +60,18 @@

   dtcodedescription/code/dt
   ddThe content of the codedescription/code element provides a
-  human readable description of the virtual machine. This data is not
-  used by libvirt in any way, it can contain any information the user
-  wants. span class=sinceSince 0.7.2/span/dd
+human readable description of the virtual machine. This data is not
+used by libvirt in any way, it can contain any information the user
+wants. span class=sinceSince 0.7.2/span/dd
+
+  dtcodemetadata/code/dt
+  ddThe codemetadata/code node can be used by applications
+to store custom metadata in the form of XML
+nodes/trees. Applications must use custom namespaces on their
+XML nodes/trees, with only one top-level element per namespace
+(if the application needs structure, they should have
+sub-elements to their namespace
+element). span class=sinceSince 0.9.10/span/dd
 /dl

 h3a name=elementsOSOperating system booting/a/h3
@@ -3556,26 +3569,6 @@ qemu-kvm -net nic,model=? /dev/null
   sub-element codelabel/code are supported.
 /p

-h3a name=customMetadataCustom metadata/a/h3
-
-pre
-  ...
-  lt;metadatagt;
-lt;app1:foo xmlns:app1=http://app1.org/app1/gt;..lt;/app1:foogt;
-lt;app2:bar xmlns:app2=http://app1.org/app2/gt;..lt;/app2:bargt;
-  lt;/metadatagt;
-  .../pre
-
-dl
-  dtcodemetadata/code/dt
-  ddThe codemetadata/code node can be used by applications to
-  store custom metadata in the form of XML nodes/trees. Applications
-  must use custom namespaces on their XML nodes/trees, with only
-  one top-level element per namespace (if the application needs
-  structure, they should have sub-elements to their namespace
-  element). span class=sinceSince 0.9.10/span/dd
-/dl
-
 h2a name=examplesExample configs/a/h2

 p
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index f2c8d02..e872396 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -11431,6 +11431,29 @@ virDomainDefFormatInternal(virDomainDefPtr def,
 virBufferEscapeString(buf,   description%s/description\n,
   def-description);

+if (def-metadata) {
+xmlBufferPtr xmlbuf;
+int oldIndentTreeOutput = xmlIndentTreeOutput;
+
+/* Indentation on output requires that we previously set
+ * xmlKeepBlanksDefault to 0 when parsing; also, libxml does 2
+ * spaces per level of indentation of intermediate elements,
+ * but no leading indentation before the starting element.
+ * Thankfully, libxml maps what looks like globals into
+ * thread-local uses, so we are thread-safe.  */
+xmlIndentTreeOutput = 1;
+xmlbuf = xmlBufferCreate();
+if (xmlNodeDump(xmlbuf, def-metadata-doc, def-metadata,
+virBufferGetIndent(buf, false) / 2 + 1, 1)  0) {
+xmlBufferFree(xmlbuf);
+xmlIndentTreeOutput = oldIndentTreeOutput;
+goto cleanup;
+}
+virBufferAsprintf(buf,   %s\n, (char *) xmlBufferContent(xmlbuf));
+xmlBufferFree(xmlbuf);
+xmlIndentTreeOutput = oldIndentTreeOutput;
+}
+
 virBufferAsprintf(buf,   memory%lu/memory\n, def-mem.max_balloon);
 virBufferAsprintf(buf,   currentMemory%lu/currentMemory\n,
   def-mem.cur_balloon);
@@ -11844,30 +11867,6 @@ 

Re: [libvirt] [PATCH] metadata: group metadata next to description

2012-01-24 Thread Zeeshan Ali (Khattak)
On Wed, Jan 25, 2012 at 2:28 AM, Eric Blake ebl...@redhat.com wrote:
 It's better to group all the metadata together.  This is a
 cosmetic output change; since the RNG allows interleave, it
 doesn't matter where the user stuck it on input, and an XPath
 query will find it in the same location when parsing the output.

Looks right. ACK!


-- 
Regards,

Zeeshan Ali (Khattak)
FSF member#5124

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

Re: [libvirt] [PATCH] metadata: group metadata next to description

2012-01-24 Thread Eric Blake
On 01/24/2012 05:49 PM, Zeeshan Ali (Khattak) wrote:
 On Wed, Jan 25, 2012 at 2:28 AM, Eric Blake ebl...@redhat.com wrote:
 It's better to group all the metadata together.  This is a
 cosmetic output change; since the RNG allows interleave, it
 doesn't matter where the user stuck it on input, and an XPath
 query will find it in the same location when parsing the output.
 
 Looks right. ACK!

Thanks; pushed.

-- 
Eric Blake   ebl...@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] docs: fix a few small typos in formatdomain.html.in

2012-01-24 Thread Laine Stump
---
Pushed under the trivial rule.

 docs/formatdomain.html.in |   10 +-
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index dca9a81..dfb010d 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -2275,8 +2275,8 @@
 pre
   ...
   lt;devicesgt;
-lt;mac address='52:54:00:6d:90:01'gt;
 lt;interface type='mcast'gt;
+  lt;mac address='52:54:00:6d:90:01'gt;
   lt;source address='230.0.0.1' port='5558'/gt;
 lt;/interfacegt;
   lt;/devicesgt;
@@ -2297,13 +2297,13 @@
   ...
   lt;devicesgt;
 lt;interface type='server'gt;
-lt;mac address='52:54:00:22:c9:42'gt;
+  lt;mac address='52:54:00:22:c9:42'gt;
   lt;source address='192.168.0.1' port='5558'/gt;
 lt;/interfacegt;
 ...
 lt;interface type='client'gt;
-lt;mac address='52:54:00:8b:c9:51'gt;
-lt;source address='192.168.0.1' port='5558'/gt;
+  lt;mac address='52:54:00:8b:c9:51'gt;
+  lt;source address='192.168.0.1' port='5558'/gt;
 lt;/interfacegt;
   lt;/devicesgt;
   .../pre
@@ -2470,7 +2470,7 @@ qemu-kvm -net nic,model=? /dev/null
   .../pre
 
 p
-  For hypervisors which support this, you can set exact NIC which should
+  For hypervisors which support this, you can set a specific NIC to
   be used for network boot. The codeorder/code attribute determines
   the order in which devices will be tried during boot sequence. The
   per-device codeboot/code elements cannot be used together with
-- 
1.7.7.5

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


Re: [libvirt] [PATCH][RFC] adding a title to the domain description informations

2012-01-24 Thread Daniel Veillard
On Tue, Jan 24, 2012 at 08:06:31AM -0700, Eric Blake wrote:
 On 01/24/2012 07:18 AM, Daniel Veillard wrote:
   The idea is that currently we have only the domain name usable as
  a description for the domain. It is not really a good human readable
  identifier, as the kind of string allowed is limited (no space for
  example). The idea would then be to extend the existing description
  field in the domain XML to keep 40 or less character string to describe
  a domain and provide that information later for example in an extended
  virsh list command or for other interfaces.
While the idea is simple, see attached patch for this, it becomes more
  complex when one tries to make accessors to set/get that title for a
  domain, since it's mutable and possibly could be coming from the
  hypervisor itself (is there anything like this in VMWare or VirtualBox?)
  it should to be implemented down at the driver level. Is that worth the
  effort ? If we go that route should we do this for other objects
  (network, storage, etc ...) too in the end ?
  
  here is a basic patch for just the XML side to give an idea, but
  adding APIs is far more work.
  
Opinions ?
 
 Looks like this is more or less identical to Peter's proposal to add a
 title element:
 https://www.redhat.com/archives/libvir-list/2012-January/msg00710.html
 
 so I'm all for the idea (not sure whether your patch or Peter's is
 better for the idea).
 
 As far as I know, the title is _not_ coming from the hypervisor itself,
 nor is it guest-visible; it is merely metadata from the host's perspective.

  Okay, let's go with Peter's then, the only difference is that he is
  using an element to store the title instead of an attribute on 
  the description element. That may be simpler in the end, so I'm fine
  dropping my patch :-)

Daniel

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

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