Re: [libvirt] [RFC PATCH v2 3/3] qemu: memory pre-pinning support for RDMA migration

2014-04-03 Thread Michael R. Hines

On 02/04/2014 10:56 PM, Jiri Denemark wrote:

On Mon, Jan 13, 2014 at 14:28:12 +0800, mrhi...@linux.vnet.ibm.com wrote:

From: "Michael R. Hines" 

RDMA Live migration requires registering memory with the hardware,

Hmm, I forgot to ask when I was reviewing the previous patch but does
any of this RDMA migration functionality require special privileges or
is any unprivileged process able to use RDMA?


No, it does not require any special privileges (just like HPC programs),
but it does access a bunch of special device files (unprivleged):

I believe someone recommended that we had the list of those
device files to the default list of allowed devices that libvirt is
already maintaining.

I'll include them in the next patch


+{
+qemuDomainObjPrivatePtr priv = vm->privateData;
+int ret;
+
+if (qemuDomainObjEnterMonitorAsync(driver, vm, job) < 0)
+return -1;
+
+ret = qemuMonitorGetMigrationCapability(
+priv->mon,
+QEMU_MONITOR_MIGRATION_CAPS_RDMA_PIN_ALL);
Is this capability always present when QEMU supports RDMA migration? If
so, it could be used when we check if QEMU supports RDMA migration.


See my comment earlier in the thread...

Yes, it's present, but it still does not guarantee that QEMU supports
it if RDMA was compiled out - only the version number is a
(minimal) guarantee, and even then the hardware can still throw
an error if RDMA itself is not supported.

I'm still not seeing what's wrong with depending on the version
number since other features are also depending on the version
number.

Are you guys suggesting that we stop depending on version altogether
for *all* QEMU features?



+unsigned long long memKB = vm->def->mem.hard_limit ?
+   vm->def->mem.hard_limit :
+vm->def->mem.max_balloon + 1024 * 1024;
+virProcessSetMaxMemLock(vm->pid, memKB * 3);

Apart from weird indentation of the virProcessSetMaxMemLock line, there
are several issues here:

- this code should be moved inside qemuMigrationSetPinAll and done only
   if VIR_MIGRATE_RDMA_PIN_ALL flag was used.

- virProcessSetMaxMemLock wants the value in bytes, thus memKB should
   rather by multiplied by 1024.

- what memory is going to be mlock()ed with rdma-pin-all? Is it going to
   be all memory allocated by QEMU or just the domain's memory? If it's
   all QEMU memory, we already painfully found out it's impossible to
   automatically compute how much memory QEMU consumes in addition to the
   configured domain's memory and I think a better approach would be to
   just migration with rdma-pin-all unless hard_limit is specified.

(Acknowledged for the first two comments).

Regarding your 3rd part: That's why I multiplied the number by 3,
the RDMA code *must* lock or the whole thing falls apart, so
we have to make "some kind" of reasonable assumption on how
much to set the lock limit to.

I'm willing to go even higher than 3 times the limit, if nobody objects,
but we have to pick some kind of limit..somehow.

Comments?

- Michael

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


Re: [libvirt] [RFC PATCH v2 2/3] qemu: RDMA migration support using 'rdma' URI

2014-04-03 Thread Michael R. Hines

On 02/03/2014 11:44 PM, Eric Blake wrote:

On 02/03/2014 08:19 AM, Jiri Denemark wrote:

On Mon, Jan 13, 2014 at 14:28:11 +0800, mrhi...@linux.vnet.ibm.com wrote:

From: "Michael R. Hines" 

The switch from x-rdma => rdma has not yet happened,
but at least we can review the patch until it goes
through on qemu-devel.

The paragraph above would better fit after "---" below so that it
disappears once this patch gets applied as the statement won't be valid
anymore at that time.


USAGE: $ virsh migrate --live --migrateuri x-rdma:hostname domain 
qemu+ssh://hostname/system

s/x-rdma/rdma/ and I believe we should use rdma://hostname as the URI


Full documenation of the feature: 
http://wiki.qemu.org/Features/RDMALiveMigration

s/documenation/documentation/



@@ -2561,6 +2570,10 @@ virQEMUCapsInitQMPMonitor(virQEMUCapsPtr qemuCaps,
  if (qemuCaps->version >= 1006000)
  virQEMUCapsSet(qemuCaps, QEMU_CAPS_DEVICE_VIDEO_PRIMARY);
  
+if (qemuCaps->version >= 200)

+virQEMUCapsSet(qemuCaps, QEMU_CAPS_MIGRATE_QEMU_RDMA);
+
+

And here we need a better check for rdma migration. What if someone
compiles QEMU without RDMA support?

Better than hard-coding it to a version string is to probe the results
of query-migrate-capabilities and only setting the capability if the
resulting list includes rdma-pin-all, as that will serve as a reliable
witness of qemu being new enough to support rdma without an x- prefix.



These comments I don't understand: Why can't we depend on the
version number here? Isn't that what it was designed for?

If someone compiles QEMU without RDMA support - why does
libvirt need to know about that? Shouldn't the admin know what their
hardware is capable of - otherwise, if they try to specify "rdma://hostname"
as a migration option, they will get a failure - which would be the
correct behavior - they tried to do something without verifying
that their hardware was capable of handling it.

Checking the capability list won't help here either: It will still be in 
the list

even if we don't compile QEMU with RDMA support. And if someone
sets the capability anyway, it will just get ignored by QEMU since
RDMA support was not available at compile time.

- Michael

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


Re: [libvirt] [PATCH] virsh: Fix help info of net-event and event command

2014-04-03 Thread Ján Tomko
On 04/04/2014 07:58 AM, liyang wrote:
> From: Li Yang 
> 
> For now the help informatin of net-event and event like this:
> [root@localhost qemu]# virsh help net-event
>   NAME
> net-event - (null)
>   ...
> 
> [root@localhost qemu]# virsh help event
>   NAME
> event - (null)
>   ...
> Now fixed them, make them show correct information
> instead of 'null'.
> 
> Signed-off-by: Li Yang 
> ---
>  tools/virsh-domain.c  |2 +-
>  tools/virsh-network.c |2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)

This has already been fixed:

commit 14d7fcc23a47e9e8911e8bba0adcd8cf51727779
Author: Eric Blake 
CommitDate: 2014-03-31 08:37:39 -0600

virsh: fix 'help event'

Jan



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

Re: [libvirt] [PATCH] Fix Memory Leak in daemon/libvirtd.c

2014-04-03 Thread Ján Tomko
On 04/03/2014 08:13 PM, Nehal J Wani wrote:
> Fixes leak introduced by e562e82f
> 
> ==4937== 64 bytes in 1 blocks are definitely lost in loss record 270 of 405
> ==4937==at 0x4A06BE0: realloc (vg_replace_malloc.c:662)
> ==4937==by 0x6FA41C4: __vasprintf_chk (vasprintf_chk.c:90)
> ==4937==by 0x50C8D29: virVasprintfInternal (stdio2.h:199)
> ==4937==by 0x50C8E3A: virAsprintfInternal (virstring.c:362)
> ==4937==by 0x11D01A: main (libvirtd.c:1170)
> 
> ---
>  daemon/libvirtd.c |3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
> index e247259..bb84c90 100644
> --- a/daemon/libvirtd.c
> +++ b/daemon/libvirtd.c
> @@ -1132,6 +1132,7 @@ int main(int argc, char **argv) {
>  bool privileged = geteuid() == 0 ? true : false;
>  bool implicit_conf = false;
>  char *run_dir = NULL;
> +char *cpumap = NULL;
>  mode_t old_umask;
>  
>  struct option opts[] = {
> @@ -1159,7 +1160,6 @@ int main(int argc, char **argv) {
>  if (strstr(argv[0], "lt-libvirtd") ||
>  strstr(argv[0], "/daemon/.libs/libvirtd")) {
>  char *tmp = strrchr(argv[0], '/');
> -char *cpumap;

There is no need to move the declaration or initialize it to NULL, since it's
always declared and initialized when we get to the VIR_FREE below and we don't
have cleanup paths here.

>  if (!tmp) {
>  fprintf(stderr, _("%s: cannot identify driver directory\n"), 
> argv[0]);
>  exit(EXIT_FAILURE);
> @@ -1182,6 +1182,7 @@ int main(int argc, char **argv) {
>  virDriverModuleInitialize(driverdir);
>  #endif
>  cpuMapOverride(cpumap);
> +VIR_FREE(cpumap);
>  *tmp = '/';
>  /* Must not free 'driverdir' - it is still used */
>  }
> 

ACK and pushed, thank you for the patch!

Jan



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

Re: [libvirt] [PATCH v2] Define CPUINFO_FILE_LEN and fix maxlen of cpuinfo file for all uses

2014-04-03 Thread Ján Tomko
On 04/04/2014 03:21 AM, Olivia Yin wrote:
> For example, the file /proc/cpuinfo for 24 cores PowerPC platform is larger 
> than
> the previous maximum size 2KB.
> It will fail to start libvirtd with the error message as below:
> virFileReadAll: Failed to read file '/proc/cpuinfo': Value too large for 
> defined
> data type
> virSysinfoRead: internal error Failed to open /proc/cpuinfo
> 
> This patch defines CPUINFO_FILE_LEN as 10KB which is enough for most 
> architectures.
> 
> Signed-off-by: Olivia Yin 
> ---
>  src/util/virsysinfo.c | 6 +++---
>  src/util/virsysinfo.h | 2 ++
>  2 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/src/util/virsysinfo.c b/src/util/virsysinfo.c
> index 7b16157..873872c 100644
> --- a/src/util/virsysinfo.c
> +++ b/src/util/virsysinfo.c
> @@ -223,7 +223,7 @@ virSysinfoRead(void)
>  if (VIR_ALLOC(ret) < 0)
>  goto no_memory;
>  
> -if (virFileReadAll(CPUINFO, 2048, &outbuf) < 0) {
> +if (virFileReadAll(CPUINFO, CPUINFO_FILE_LEN, &outbuf) < 0) {
>  virReportError(VIR_ERR_INTERNAL_ERROR,
> _("Failed to open %s"), CPUINFO);
>  return NULL;
> @@ -341,7 +341,7 @@ virSysinfoRead(void)
>  if (VIR_ALLOC(ret) < 0)
>  goto no_memory;
>  
> -if (virFileReadAll(CPUINFO, 2048, &outbuf) < 0) {
> +if (virFileReadAll(CPUINFO, CPUINFO_FILE_LEN, &outbuf) < 0) {
>  virReportError(VIR_ERR_INTERNAL_ERROR,
> _("Failed to open %s"), CPUINFO);
>  return NULL;
> @@ -470,7 +470,7 @@ virSysinfoRead(void)
>  goto no_memory;
>  
>  /* Gather info from /proc/cpuinfo */
> -if (virFileReadAll(CPUINFO, 8192, &outbuf) < 0) {
> +if (virFileReadAll(CPUINFO, CPUINFO_FILE_LEN, &outbuf) < 0) {
>  virReportError(VIR_ERR_INTERNAL_ERROR,
> _("Failed to open %s"), CPUINFO);
>  return NULL;
> diff --git a/src/util/virsysinfo.h b/src/util/virsysinfo.h
> index fbb505b..b5336e2 100644
> --- a/src/util/virsysinfo.h
> +++ b/src/util/virsysinfo.h
> @@ -102,4 +102,6 @@ bool virSysinfoIsEqual(virSysinfoDefPtr src,
>  
>  VIR_ENUM_DECL(virSysinfo)
>  
> +#define CPUINFO_FILE_LEN (10*1024)   /*10KB limit for /proc/cpuinfo file*/
> +

This doesn't need to be in the header file.

I moved it near the definition of CPUINFO and pushed it. Thank you for the 
patch!

Jan




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

Re: [libvirt] [RFC PATCH v2 2/3] qemu: RDMA migration support using 'rdma' URI

2014-04-03 Thread Michael R. Hines

On 02/03/2014 11:19 PM, Jiri Denemark wrote:

USAGE: $ virsh migrate --live --migrateuri x-rdma:hostname domain 
qemu+ssh://hostname/system

s/x-rdma/rdma/ and I believe we should use rdma://hostname as the URI


Acknowledged.





Signed-off-by: Michael R. Hines 
---
  src/qemu/qemu_capabilities.c |  13 +
  src/qemu/qemu_capabilities.h |   1 +
  src/qemu/qemu_command.c  |   8 
  src/qemu/qemu_migration.c| 110 ++-
  src/qemu/qemu_monitor.c  |   3 +-
  src/qemu/qemu_monitor.h  |   1 +
  src/util/viruri.c|   7 ++-
  7 files changed, 119 insertions(+), 24 deletions(-)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 548b988..d82b48c 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -243,6 +243,8 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
"virtio-mmio",
"ich9-intel-hda",
"kvm-pit-lost-tick-policy",
+
+  "migrate-qemu-rdma", /* 160 */

s/migrate-qemu-rdma/migrate-rdma/

"qemu" string in pretty redundant here given that it is a qemu
capability.


Acknowledged.




  );
  
  struct _virQEMUCaps {

@@ -906,6 +908,9 @@ virCapsPtr virQEMUCapsInit(virQEMUCapsCachePtr cache)
  virCapabilitiesAddHostMigrateTransport(caps,
 "tcp");
  
+virCapabilitiesAddHostMigrateTransport(caps,

+   "rdma");
+
  /* QEMU can support pretty much every arch that exists,
   * so just probe for them all - we gracefully fail
   * if a qemu-system-$ARCH binary can't be found
@@ -1110,6 +1115,7 @@ virQEMUCapsComputeCmdFlags(const char *help,
   *  -incoming unix   (qemu >= 0.12.0)
   *  -incoming fd (qemu >= 0.12.0)
   *  -incoming stdio  (all earlier kvm)
+ *  -incoming rdma   (qemu >= 2.0.0)
   *
   * NB, there was a pre-kvm-79 'tcp' support, but it
   * was broken, because it blocked the monitor console
@@ -1130,6 +1136,9 @@ virQEMUCapsComputeCmdFlags(const char *help,
  virQEMUCapsSet(qemuCaps, QEMU_CAPS_MIGRATE_KVM_STDIO);
  }
  
+if (version >= 200)

+virQEMUCapsSet(qemuCaps, QEMU_CAPS_MIGRATE_QEMU_RDMA);
+
  if (version >= 1)
  virQEMUCapsSet(qemuCaps, QEMU_CAPS_0_10);
  

This is not needed, we won't be parsing -help for any QEMU that supports
RDMA.


Acknowledged.




@@ -2561,6 +2570,10 @@ virQEMUCapsInitQMPMonitor(virQEMUCapsPtr qemuCaps,
  if (qemuCaps->version >= 1006000)
  virQEMUCapsSet(qemuCaps, QEMU_CAPS_DEVICE_VIDEO_PRIMARY);
  
+if (qemuCaps->version >= 200)

+virQEMUCapsSet(qemuCaps, QEMU_CAPS_MIGRATE_QEMU_RDMA);
+
+

And here we need a better check for rdma migration. What if someone
compiles QEMU without RDMA support?


  if (virQEMUCapsProbeQMPCommands(qemuCaps, mon) < 0)
  goto cleanup;
  if (virQEMUCapsProbeQMPEvents(qemuCaps, mon) < 0)
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index 02d47c6..3e78961 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -198,6 +198,7 @@ enum virQEMUCapsFlags {
  QEMU_CAPS_DEVICE_VIRTIO_MMIO = 157, /* -device virtio-mmio */
  QEMU_CAPS_DEVICE_ICH9_INTEL_HDA = 158, /* -device ich9-intel-hda */
  QEMU_CAPS_KVM_PIT_TICK_POLICY = 159, /* kvm-pit.lost_tick_policy */
+QEMU_CAPS_MIGRATE_QEMU_RDMA  = 160, /* have qemu rdma migration */

s/_QEMU// as it is redundant.


Acknowledged.



  
  QEMU_CAPS_LAST,   /* this must always be the last item */

  };
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 763417f..0d23d8b 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -9448,6 +9448,14 @@ qemuBuildCommandLine(virConnectPtr conn,
  goto error;
  }
  virCommandAddArg(cmd, migrateFrom);
+} else if (STRPREFIX(migrateFrom, "rdma")) {
+if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MIGRATE_QEMU_RDMA)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   "%s", _("RDMA migration is not supported with "
+   "this QEMU binary"));
+goto error;
+}
+virCommandAddArg(cmd, migrateFrom);
  } else if (STREQ(migrateFrom, "stdio")) {
  if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_MIGRATE_QEMU_FD)) {
  virCommandAddArgFormat(cmd, "fd:%d", migrateFd);
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index ef6f1c5..1e0f538 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -46,6 +46,7 @@
  #include "virerror.h"
  #include "viralloc.h"
  #include "virfile.h"
+#include "virprocess.h"
  #include "datatypes.h"
  #include "fdstream.h"
  #include "viruuid.h"
@@ -2163,6 +2164,7 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver,

Re: [libvirt] [PATCH] Define STATE_XMLFILE_LEN as 8192

2014-04-03 Thread Ján Tomko
On 04/03/2014 08:43 AM, Olivia Yin wrote:
> Save/restore the state of domain and migrate need read state XML file.
> 8192B is not the exactly maximum file length of state file.
> 
> restore command could work successfully in virsh shell.
> virsh # list --all
>  IdName   State
> 
>  9 sdkrunning
> 
> virsh # save sdk pp
> 
> Domain sdk saved to pp
> 
> virsh # list --all
>  IdName   State
> 
>  - sdkshut off
> 
> virsh # restore pp
> Domain restored from pp
> 
> virsh # list --all
>  IdName   State
> 
>  10sdkrunning
> 
> But it will fail with 'virsh restore' command because the state file is 
> oversized.
> ~# virsh restore pp
> error: Failed to read file 'pp': Value too large for defined data type

There is no xml file supplied here, just the save file...

> 
> Signed-off-by: Olivia Yin 
> ---
>  tools/virsh-domain.c | 6 +++---
>  tools/virsh.h| 1 +
>  2 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> index 73414f8..8ade296 100644
> --- a/tools/virsh-domain.c
> +++ b/tools/virsh-domain.c
> @@ -3547,7 +3547,7 @@ doSave(void *opaque)
>  goto out;
>  
>  if (xmlfile &&
> -virFileReadAll(xmlfile, 8192, &xml) < 0) {
> +virFileReadAll(xmlfile, STATE_XMLFILE_LEN, &xml) < 0) {
>  vshReportError(ctl);
>  goto out;
>  }
> @@ -3840,7 +3840,7 @@ cmdSaveImageDefine(vshControl *ctl, const vshCmd *cmd)
>  if (vshCommandOptStringReq(ctl, cmd, "xml", &xmlfile) < 0)
>  return false;
>  
> -if (virFileReadAll(xmlfile, 8192, &xml) < 0)
> +if (virFileReadAll(xmlfile, STATE_XMLFILE_LEN, &xml) < 0)
>  goto cleanup;
>  
>  if (virDomainSaveImageDefineXML(ctl->conn, file, xml, flags) < 0) {
> @@ -4424,7 +4424,7 @@ cmdRestore(vshControl *ctl, const vshCmd *cmd)
>  return false;
>  
>  if (xmlfile &&
> -virFileReadAll(xmlfile, 8192, &xml) < 0)
> +virFileReadAll(xmlfile, STATE_XMLFILE_LEN, &xml) < 0)

... but this changes the limit for the XML file of the updated domain, so it
shouldn't be executed by the above command. Does this actually fix the issue?

Also, VSH_MAX_XML_FILE can be used for all of these.

Jan




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

[libvirt] [PATCH] virsh: Fix help info of net-event and event command

2014-04-03 Thread liyang
From: Li Yang 

For now the help informatin of net-event and event like this:
[root@localhost qemu]# virsh help net-event
  NAME
net-event - (null)
  ...

[root@localhost qemu]# virsh help event
  NAME
event - (null)
  ...
Now fixed them, make them show correct information
instead of 'null'.

Signed-off-by: Li Yang 
---
 tools/virsh-domain.c  |2 +-
 tools/virsh-network.c |2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index f2856a5..310b5dc 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -11030,7 +11030,7 @@ static vshEventCallback vshEventCallbacks[] = {
 verify(VIR_DOMAIN_EVENT_ID_LAST == ARRAY_CARDINALITY(vshEventCallbacks));
 
 static const vshCmdInfo info_event[] = {
-{.name = "event",
+{.name = "help",
  .data = N_("Domain Events")
 },
 {.name = "desc",
diff --git a/tools/virsh-network.c b/tools/virsh-network.c
index 22d21e7..338db28 100644
--- a/tools/virsh-network.c
+++ b/tools/virsh-network.c
@@ -1180,7 +1180,7 @@ vshEventLifecyclePrint(virConnectPtr conn 
ATTRIBUTE_UNUSED,
 }
 
 static const vshCmdInfo info_network_event[] = {
-{.name = "net-event",
+{.name = "help",
  .data = N_("Network Events")
 },
 {.name = "desc",
-- 
1.7.1

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


Re: [libvirt] [RFC PATCH v2 1/3] qemu: Expose additional timing metrics for 'setup' and 'mbps'

2014-04-03 Thread Michael R. Hines

On 02/03/2014 08:52 PM, Daniel P. Berrange wrote:

On Mon, Feb 03, 2014 at 01:32:59PM +0100, Jiri Denemark wrote:

On Mon, Jan 13, 2014 at 14:28:10 +0800, mrhi...@linux.vnet.ibm.com wrote:

From: "Michael R. Hines" 

RDMA migration uses the 'setup' state in QEMU to optionally lock
all memory before the migration starts. The total time spent in
this state is already exposed by QEMU, so expose it in libvirt.

Additionally, QEMU also now exports migration throughput (mbps),
so let's see that one too...

Signed-off-by: Michael R. Hines 
---
  include/libvirt/libvirt.h.in | 15 +++
  src/qemu/qemu_driver.c   | 14 ++
  src/qemu/qemu_monitor.h  | 12 
  src/qemu/qemu_monitor_json.c |  8 
  4 files changed, 49 insertions(+)

diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
index 5aad75c..5ac2694 100644
--- a/include/libvirt/libvirt.h.in
+++ b/include/libvirt/libvirt.h.in
@@ -4153,6 +4153,21 @@ int virDomainAbortJob(virDomainPtr dom);
  #define VIR_DOMAIN_JOB_DOWNTIME "downtime"
  
  /**

+ * VIR_DOMAIN_JOB_MBPS:
+ *
+ * virDomainGetJobStats field: network throughput used while migrating.
+ */
+#define VIR_DOMAIN_JOB_MBPS "mbps"

I think this would be better as

 #define VIR_DOMAIN_JOB_THROUGHPUT "throughput"

And you need to explicitly mention the type of the value returned in
this parameter.

Hmm, in block I/O tuning XML/APIs we use either

  VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC

or

   VIR_DOMAIN_BLKIO_DEVICE_READ_BPS

which would suggest that first we should measure this in bps too,
not mbp, and using either BPS or BYTES_SEC as a constant suffix
is reasonable for consistency.

Daniel


Acknowledged.

- Michael

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


Re: [libvirt] [RFC PATCH v2 1/3] qemu: Expose additional timing metrics for 'setup' and 'mbps'

2014-04-03 Thread Michael R. Hines

On 02/03/2014 08:32 PM, Jiri Denemark wrote:

On Mon, Jan 13, 2014 at 14:28:10 +0800, mrhi...@linux.vnet.ibm.com wrote:

From: "Michael R. Hines" 

RDMA migration uses the 'setup' state in QEMU to optionally lock
all memory before the migration starts. The total time spent in
this state is already exposed by QEMU, so expose it in libvirt.

Additionally, QEMU also now exports migration throughput (mbps),
so let's see that one too...

Signed-off-by: Michael R. Hines 
---
  include/libvirt/libvirt.h.in | 15 +++
  src/qemu/qemu_driver.c   | 14 ++
  src/qemu/qemu_monitor.h  | 12 
  src/qemu/qemu_monitor_json.c |  8 
  4 files changed, 49 insertions(+)

diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
index 5aad75c..5ac2694 100644
--- a/include/libvirt/libvirt.h.in
+++ b/include/libvirt/libvirt.h.in
@@ -4153,6 +4153,21 @@ int virDomainAbortJob(virDomainPtr dom);
  #define VIR_DOMAIN_JOB_DOWNTIME "downtime"
  
  /**

+ * VIR_DOMAIN_JOB_MBPS:
+ *
+ * virDomainGetJobStats field: network throughput used while migrating.
+ */
+#define VIR_DOMAIN_JOB_MBPS "mbps"

I think this would be better as

 #define VIR_DOMAIN_JOB_THROUGHPUT "throughput"

And you need to explicitly mention the type of the value returned in
this parameter.


Acknowledged.




+
+/**
+ * VIR_DOMAIN_JOB_SETUP_TIME:
+ *
+ * virDomainGetJobStats field: total time in milliseconds spent preparing
+ * the migration in the 'setup' phase before the iterations begin.
+ */
+#define VIR_DOMAIN_JOB_SETUP_TIME "setup_time"

Again, the documentation does not mention what type is returned.


Acknowledged.


+
+/**
   * VIR_DOMAIN_JOB_DATA_TOTAL:
   *
   * virDomainGetJobStats field: total number of bytes supposed to be
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 47d8a09..6a7dcc7 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -11078,6 +11078,20 @@ qemuDomainGetJobStats(virDomainPtr dom,
  goto cleanup;
  }
  
+if (priv->job.status.mbps_set) {

+if (virTypedParamsAddDouble(&par, &npar, &maxpar,
+VIR_DOMAIN_JOB_MBPS,
+priv->job.status.mbps) < 0)
+goto cleanup;
+}

I think we should take the Mbps value from qemu and turn it into Bps (or
even bps), which would make this parameter consistent with other
migration statistics parameters that return values in bytes. And then we
can change the type to ULLong.



Acknowledged.


+
+if (priv->job.status.setup_time_set) {
+if (virTypedParamsAddULLong(&par, &npar, &maxpar,
+VIR_DOMAIN_JOB_SETUP_TIME,
+priv->job.status.setup_time) < 0)
+goto cleanup;
+}
+
  if (virTypedParamsAddULLong(&par, &npar, &maxpar,
  VIR_DOMAIN_JOB_DISK_TOTAL,
  priv->job.info.fileTotal) < 0 ||
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
index eabf000..27f9cb4 100644
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -412,6 +412,18 @@ struct _qemuMonitorMigrationStatus {
  /* total or expected depending on status */
  bool downtime_set;
  unsigned long long downtime;
+/*
+ * Duration of the QEMU 'setup' state.
+ * for RDMA, this may be on the order of several seconds
+ * if pinning support is requested before the migration begins.
+ */
+bool setup_time_set;
+unsigned long long setup_time;
+/*
+ * Migration throughput in Mbps.
+ */
+bool mbps_set;
+double mbps;
  
  unsigned long long ram_transferred;

  unsigned long long ram_remaining;
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index ec3b958..214e140 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -2426,6 +2426,14 @@ qemuMonitorJSONGetMigrationStatusReply(virJSONValuePtr 
reply,
  virJSONValueObjectGetNumberUlong(ram, "normal-bytes",
   &status->ram_normal_bytes);
  
+if (virJSONValueObjectGetNumberDouble(ram, "mbps",

+  &status->mbps) == 0)
+status->mbps_set = true;
+
+if (virJSONValueObjectGetNumberUlong(ret, "setup-time",
+  &status->setup_time) == 0)
+status->setup_time_set = true;
+
  virJSONValuePtr disk = virJSONValueObjectGet(ret, "disk");
  if (disk) {
  rc = virJSONValueObjectGetNumberUlong(disk, "transferred",

Overall the patch looks good and could even be pushed separately.

Jirka



Will submit a v3 for everything soon

- Michae

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


[libvirt] [PATCH 5/5] conf: start testing contents of the new backing chain fields

2014-04-03 Thread Eric Blake
The testsuite is absolutely essential to feeling comfortable
about swapping the backing chain structure over to a new format.
This patch tests the path settings, and demonstrates that right
now the code is passing only the canonical name to the child
struct, which means more work is necessary in virstoragefile
to pass the user spelling alongside the canonical name down to
the child.

* tests/virstoragetest.c (testStorageChain): Test path.
(mymain): Update expected data.

Signed-off-by: Eric Blake 
---
 tests/virstoragetest.c | 239 -
 1 file changed, 175 insertions(+), 64 deletions(-)

diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c
index efd920a..9d6e344 100644
--- a/tests/virstoragetest.c
+++ b/tests/virstoragetest.c
@@ -205,6 +205,7 @@ struct _testFileData
 bool expIsFile;
 unsigned long long expCapacity;
 bool expEncrypted;
+const char *expPath;
 };

 enum {
@@ -266,21 +267,25 @@ testStorageChain(const void *args)
 }

 if (virAsprintf(&expect,
-"store:%s\nraw:%s\ndirectory:%s\nother:%d %d %lld %d",
+"store:%s\nraw:%s\ndirectory:%s\nother:%d %d %lld %d\n"
+"path:%s\n",
 NULLSTR(data->files[i]->expBackingStore),
 NULLSTR(data->files[i]->expBackingStoreRaw),
 NULLSTR(data->files[i]->expDirectory),
 data->files[i]->expFormat,
 data->files[i]->expIsFile,
 data->files[i]->expCapacity,
-data->files[i]->expEncrypted) < 0 ||
+data->files[i]->expEncrypted,
+NULLSTR(data->files[i]->expPath)) < 0 ||
 virAsprintf(&actual,
-"store:%s\nraw:%s\ndirectory:%s\nother:%d %d %lld %d",
+"store:%s\nraw:%s\ndirectory:%s\nother:%d %d %lld %d\n"
+"path:%s\n",
 NULLSTR(elt->backingStore),
 NULLSTR(elt->backingStoreRaw),
 NULLSTR(elt->directory),
 elt->backingStoreFormat, elt->backingStoreIsFile,
-elt->capacity, !!elt->encryption) < 0) {
+elt->capacity, !!elt->encryption,
+NULLSTR(elt->path)) < 0) {
 VIR_FREE(expect);
 VIR_FREE(actual);
 goto cleanup;
@@ -351,9 +356,24 @@ mymain(void)
 } while (0)

 /* Expected details about files in chains */
-const testFileData raw = {
+const testFileData rel_raw = {
 .expFormat = VIR_STORAGE_FILE_NONE,
+.expPath = "raw",
 };
+const testFileData abs_raw = {
+.expFormat = VIR_STORAGE_FILE_NONE,
+.expPath = canonraw,
+};
+
+const testFileData rel_qcow2_as_raw = {
+.expFormat = VIR_STORAGE_FILE_NONE,
+.expPath = "qcow2",
+};
+const testFileData abs_qcow2_as_raw = {
+.expFormat = VIR_STORAGE_FILE_NONE,
+.expPath = canonqcow2,
+};
+
 const testFileData qcow2_relback_relstart = {
 .expBackingStore = canonraw,
 .expBackingStoreRaw = "raw",
@@ -361,6 +381,7 @@ mymain(void)
 .expFormat = VIR_STORAGE_FILE_RAW,
 .expIsFile = true,
 .expCapacity = 1024,
+.expPath = "qcow2",
 };
 const testFileData qcow2_relback_absstart = {
 .expBackingStore = canonraw,
@@ -369,15 +390,28 @@ mymain(void)
 .expFormat = VIR_STORAGE_FILE_RAW,
 .expIsFile = true,
 .expCapacity = 1024,
+.expPath = canonqcow2,
 };
-const testFileData qcow2_absback = {
+
+const testFileData rel_qcow2_absback = {
 .expBackingStore = canonraw,
 .expBackingStoreRaw = absraw,
 .expDirectory = datadir,
 .expFormat = VIR_STORAGE_FILE_RAW,
 .expIsFile = true,
 .expCapacity = 1024,
+.expPath = "qcow2",
 };
+const testFileData abs_qcow2_absback = {
+.expBackingStore = canonraw,
+.expBackingStoreRaw = absraw,
+.expDirectory = datadir,
+.expFormat = VIR_STORAGE_FILE_RAW,
+.expIsFile = true,
+.expCapacity = 1024,
+.expPath = canonqcow2,
+};
+
 const testFileData qcow2_as_probe = {
 .expBackingStore = canonraw,
 .expBackingStoreRaw = absraw,
@@ -385,50 +419,122 @@ mymain(void)
 .expFormat = VIR_STORAGE_FILE_AUTO,
 .expIsFile = true,
 .expCapacity = 1024,
+.expPath = canonqcow2,
 };
-const testFileData qcow2_bogus = {
+
+const testFileData rel_qcow2_bogus = {
 .expBackingStoreRaw = datadir "/bogus",
 .expDirectory = datadir,
 .expFormat = VIR_STORAGE_FILE_NONE,
 .expCapacity = 1024,
+.expPath = "qcow2",
 };
-const testFileData qcow2_protocol = {
+const t

[libvirt] [PATCH 4/5] conf: track more fields in backing chain metadata

2014-04-03 Thread Eric Blake
The current use of virStorageFileMetadata is awkward; to learn
some of the information about a child node, you have to read
fields in the parent node.  This does not lend itself well to
modifying backing chains (whether inserting a new node in the
chain, or consolidating existing nodes); better would be to
learn about a child node directly in that node.  This patch
sets up some new fields which contain redundant information,
although not necessarily in the final desired state for the
new fields (see the next patch for actual tests of what is there
now).  Then later patches will do any refactoring necessary to
get the fields to their desired states, and update clients to
get the information from the new fields, so we can finally
delete the fields that are tracking information about the wrong
node.

* src/util/virstoragefile.h (_virStorageFileMetadata): Add
path, canonName, relDir, type, and format fields.  Reorder
existing fields, and add lots of comments.
* src/util/virstoragefile.c (virStorageFileFreeMetadata): Clean
new fields.
(virStorageFileGetMetadataInternal)
(virStorageFileGetMetadataFromFDInternal): Start populating new
fields.

Signed-off-by: Eric Blake 
---
 src/util/virstoragefile.c | 30 --
 src/util/virstoragefile.h | 35 ++-
 2 files changed, 58 insertions(+), 7 deletions(-)

diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index 0e1461d..d741fb7 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -798,6 +798,10 @@ virStorageFileGetMetadataInternal(const char *path,

 if (VIR_ALLOC(meta) < 0)
 return NULL;
+if (VIR_STRDUP(meta->path, path) < 0)
+goto cleanup;
+if (VIR_STRDUP(meta->relDir, directory) < 0)
+goto cleanup;

 if (format == VIR_STORAGE_FILE_AUTO)
 format = virStorageFileProbeFormatFromBuf(path, buf, len);
@@ -808,6 +812,7 @@ virStorageFileGetMetadataInternal(const char *path,
  format);
 goto cleanup;
 }
+meta->format = format;

 /* XXX we should consider moving virStorageBackendUpdateVolInfo
  * code into this method, for non-magic files
@@ -1013,9 +1018,20 @@ virStorageFileGetMetadataFromFDInternal(const char *path,
 return NULL;
 }

-/* No header to probe for directories, but also no backing file */
 if (S_ISDIR(sb.st_mode)) {
-ignore_value(VIR_ALLOC(ret));
+/* No header to probe for directories, but also no backing
+ * file; therefore, no inclusion loop is possible, and we
+ * don't need canonName or relDir.  */
+if (VIR_ALLOC(ret) < 0)
+goto cleanup;
+if (VIR_STRDUP(ret->path, path) < 0) {
+virStorageFileFreeMetadata(ret);
+ret = NULL;
+goto cleanup;
+}
+ret->type = VIR_STORAGE_TYPE_DIR;
+ret->format = format > VIR_STORAGE_FILE_NONE ? format
+: VIR_STORAGE_FILE_DIR;
 goto cleanup;
 }

@@ -1030,6 +1046,12 @@ virStorageFileGetMetadataFromFDInternal(const char *path,
 }

 ret = virStorageFileGetMetadataInternal(path, buf, len, directory, format);
+if (ret) {
+if (S_ISREG(sb.st_mode))
+ret->type = VIR_STORAGE_TYPE_FILE;
+else if (S_ISBLK(sb.st_mode))
+ret->type = VIR_STORAGE_TYPE_BLOCK;
+}
  cleanup:
 VIR_FREE(buf);
 return ret;
@@ -1205,6 +1227,10 @@ virStorageFileFreeMetadata(virStorageFileMetadata *meta)
 if (!meta)
 return;

+VIR_FREE(meta->path);
+VIR_FREE(meta->canonName);
+VIR_FREE(meta->relDir);
+
 virStorageFileFreeMetadata(meta->backingMeta);
 VIR_FREE(meta->backingStore);
 VIR_FREE(meta->backingStoreRaw);
diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
index 3807285..a284e37 100644
--- a/src/util/virstoragefile.h
+++ b/src/util/virstoragefile.h
@@ -112,17 +112,42 @@ struct _virStorageTimestamps {
 typedef struct _virStorageFileMetadata virStorageFileMetadata;
 typedef virStorageFileMetadata *virStorageFileMetadataPtr;
 struct _virStorageFileMetadata {
-char *backingStore; /* Canonical name (absolute file, or protocol) */
-char *backingStoreRaw; /* If file, original name, possibly relative */
-char *directory; /* The directory containing basename of backingStoreRaw */
-int backingStoreFormat; /* enum virStorageFileFormat */
-bool backingStoreIsFile;
+/* Name of this file as spelled by the user (top level) or
+ * metadata of the parent (if this is a backing store).  */
+char *path;
+/* Canonical name of this file, used to detect loops in the
+ * backing store chain.  */
+char *canonName;
+/* Directory to start from if backingStoreRaw is a relative file
+ * name */
+char *relDir;
+/* Name of the backing store recorded in metadata of the parent */
+char *backingStoreRaw;
+
+/* Backing chain.  backingMeta->origName should match
+ * 

[libvirt] [PATCH 3/5] conf: track when storage type is still undetermined

2014-04-03 Thread Eric Blake
Right now, virStorageFileMetadata tracks bool backingStoreIsFile
for whether the backing string specified in metadata can be
resolved as a file (covering both block and regular file
resources) or is treated as a network protocol.  But when
merging this struct with virStorageSource, it will be easier
to just actually track which type of resource it is, as well
as have a reserved value for the case where the resource type
is unknown (or had an error during probing).

* src/util/virstoragefile.h (virStorageType): Add a placeholder
value, swap order to match similar public enum.
* src/util/virstoragefile.c (virStorage): Update string mapping.
* src/conf/domain_conf.c (virDomainDiskSourceParse)
(virDomainDiskDefParseXML, virDomainDiskDefFormat)
(virDomainDiskSourceFormat): Adjust clients.
* src/conf/snapshot_conf.c (virDomainSnapshotDiskDefParseXML):
Likewise.
* src/qemu/qemu_driver.c
(qemuDomainSnapshotPrepareDiskExternalBackingInactive)
(qemuDomainSnapshotPrepareDiskExternalOverlayActive)
(qemuDomainSnapshotPrepareDiskExternalOverlayInactive)
(qemuDomainSnapshotPrepareDiskInternal)
(qemuDomainSnapshotCreateSingleDiskActive): Likewise.
* src/qemu/qemu_command.c (qemuGetDriveSourceString): Likewise.

Signed-off-by: Eric Blake 
---
 src/conf/domain_conf.c| 10 ++
 src/conf/snapshot_conf.c  |  2 +-
 src/qemu/qemu_command.c   |  1 +
 src/qemu/qemu_driver.c| 11 +--
 src/util/virstoragefile.c |  3 ++-
 src/util/virstoragefile.h |  8 ++--
 6 files changed, 25 insertions(+), 10 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 465bf84..0c5c7ab 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -4965,7 +4965,7 @@ virDomainDiskSourceParse(xmlNodePtr node,

 memset(&host, 0, sizeof(host));

-switch (src->type) {
+switch ((enum virStorageType)src->type) {
 case VIR_STORAGE_TYPE_FILE:
 src->path = virXMLPropString(node, "file");
 break;
@@ -5053,7 +5053,8 @@ virDomainDiskSourceParse(xmlNodePtr node,
 if (virDomainDiskSourcePoolDefParse(node, &src->srcpool) < 0)
 goto cleanup;
 break;
-default:
+case VIR_STORAGE_TYPE_NONE:
+case VIR_STORAGE_TYPE_LAST:
 virReportError(VIR_ERR_INTERNAL_ERROR,
_("unexpected disk type %s"),
virStorageTypeToString(src->type));
@@ -5150,7 +5151,7 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,

 type = virXMLPropString(node, "type");
 if (type) {
-if ((def->src.type = virStorageTypeFromString(type)) < 0) {
+if ((def->src.type = virStorageTypeFromString(type)) <= 0) {
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("unknown disk type '%s'"), type);
 goto error;
@@ -14836,6 +14837,7 @@ virDomainDiskSourceFormat(virBufferPtr buf,
  src->seclabels, flags);
 break;

+case VIR_STORAGE_TYPE_NONE:
 case VIR_STORAGE_TYPE_LAST:
 virReportError(VIR_ERR_INTERNAL_ERROR,
_("unexpected disk type %d"), src->type);
@@ -14867,7 +14869,7 @@ virDomainDiskDefFormat(virBufferPtr buf,

 char uuidstr[VIR_UUID_STRING_BUFLEN];

-if (!type) {
+if (!type || !def->src.type) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
_("unexpected disk type %d"), def->src.type);
 return -1;
diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
index 374a104..852a286 100644
--- a/src/conf/snapshot_conf.c
+++ b/src/conf/snapshot_conf.c
@@ -132,7 +132,7 @@ virDomainSnapshotDiskDefParseXML(xmlNodePtr node,
 }

 if ((type = virXMLPropString(node, "type"))) {
-if ((def->src.type = virStorageTypeFromString(type)) < 0 ||
+if ((def->src.type = virStorageTypeFromString(type)) <= 0 ||
 def->src.type == VIR_STORAGE_TYPE_VOLUME ||
 def->src.type == VIR_STORAGE_TYPE_DIR) {
 virReportError(VIR_ERR_XML_ERROR,
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 099a777..37841d1 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -3856,6 +3856,7 @@ qemuGetDriveSourceString(int type,
 break;

 case VIR_STORAGE_TYPE_VOLUME:
+case VIR_STORAGE_TYPE_NONE:
 case VIR_STORAGE_TYPE_LAST:
 break;
 }
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 1d08951..4bb4819 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -12356,6 +12356,7 @@ 
qemuDomainSnapshotPrepareDiskExternalBackingInactive(virDomainDiskDefPtr disk)

 case VIR_STORAGE_TYPE_DIR:
 case VIR_STORAGE_TYPE_VOLUME:
+case VIR_STORAGE_TYPE_NONE:
 case VIR_STORAGE_TYPE_LAST:
 virReportError(VIR_ERR_INTERNAL_ERROR,
_("external inactive snapshots are not supported on "
@@ -12420,6 +12421,7 @@ 
qemuDomainSnapshotPrepareDiskExternalOverlayActive(virDomainSna

[libvirt] [PATCH 2/5] tests: refactor virstoragetest for less stack space

2014-04-03 Thread Eric Blake
I'm about to add fields to virStorageFileMetadata, which means
also adding fields to the testFileData struct in virstoragetest.
Alas, adding even one pointer on an x86_64 machine gave me a
dreaded compiler error:

virstoragetest.c:712:1: error: the frame size of 4208 bytes is larger than 4096 
bytes [-Werror=frame-larger-than=]

After some experimentation, I realized that each test was creating
yet another testChainData (which contains testFileData) on the stack;
forcing the reuse of one of these structures instead of creating a
fresh one each time drastically reduces the size requirements.  While
at it, I also got rid of a lot of intermediate structs, with some
macro magic that lets me directly build up the destination chains
inline.

* tests/virstoragetest.c (mymain): Rewrite TEST_ONE_CHAIN to
reuse the same struct for each test, and to take the data
inline rather than via intermediate variables.
(testChainData): Use bounded array of pointers instead of
unlimited array of struct.
(testStorageChain): Reflect struct change.

Signed-off-by: Eric Blake 
---
 tests/virstoragetest.c | 167 -
 1 file changed, 81 insertions(+), 86 deletions(-)

diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c
index 9a3b3a3..efd920a 100644
--- a/tests/virstoragetest.c
+++ b/tests/virstoragetest.c
@@ -218,7 +218,7 @@ struct testChainData
 {
 const char *start;
 enum virStorageFileFormat format;
-const testFileData *files;
+const testFileData *files[5];
 int nfiles;
 unsigned int flags;
 };
@@ -267,13 +267,13 @@ testStorageChain(const void *args)

 if (virAsprintf(&expect,
 "store:%s\nraw:%s\ndirectory:%s\nother:%d %d %lld %d",
-NULLSTR(data->files[i].expBackingStore),
-NULLSTR(data->files[i].expBackingStoreRaw),
-NULLSTR(data->files[i].expDirectory),
-data->files[i].expFormat,
-data->files[i].expIsFile,
-data->files[i].expCapacity,
-data->files[i].expEncrypted) < 0 ||
+NULLSTR(data->files[i]->expBackingStore),
+NULLSTR(data->files[i]->expBackingStoreRaw),
+NULLSTR(data->files[i]->expDirectory),
+data->files[i]->expFormat,
+data->files[i]->expIsFile,
+data->files[i]->expCapacity,
+data->files[i]->expEncrypted) < 0 ||
 virAsprintf(&actual,
 "store:%s\nraw:%s\ndirectory:%s\nother:%d %d %lld %d",
 NULLSTR(elt->backingStore),
@@ -312,29 +312,42 @@ mymain(void)
 {
 int ret;
 virCommandPtr cmd = NULL;
+struct testChainData data;

 /* Prep some files with qemu-img; if that is not found on PATH, or
  * if it lacks support for qcow2 and qed, skip this test.  */
 if ((ret = testPrepImages()) != 0)
 return ret;

-#define TEST_ONE_CHAIN(id, start, format, chain, flags)  \
+#define TEST_ONE_CHAIN(id, start, format, flags, ...)\
 do { \
-struct testChainData data = {\
-start, format, chain, ARRAY_CARDINALITY(chain), flags,   \
+size_t i;\
+memset(&data, 0, sizeof(data));  \
+data = (struct testChainData){   \
+start, format, { __VA_ARGS__ }, 0, flags,\
 };   \
+for (i = 0; i < ARRAY_CARDINALITY(data.files); i++)  \
+if (data.files[i])   \
+data.nfiles++;   \
 if (virtTestRun("Storage backing chain " id, \
 testStorageChain, &data) < 0)\
 ret = -1;\
 } while (0)

+#define VIR_FLATTEN_2(...) __VA_ARGS__
+#define VIR_FLATTEN_1(_1) VIR_FLATTEN_2 _1
+
 #define TEST_CHAIN(id, relstart, absstart, format, chain1, flags1,   \
chain2, flags2, chain3, flags3, chain4, flags4)   \
 do { \
-TEST_ONE_CHAIN(#id "a", relstart, format, chain1, flags1);   \
-TEST_ONE_CHAIN(#id "b", relstart, format, chain2, flags2);   \
-TEST_ONE_CHAIN(#id "c", absstart, format, chain3, flags3);   \
-TEST_ONE_CHAIN(#id "d", absstart, format, chain4, flags4);   \
+TEST_ONE_CHAIN(#id "a", relstart, format, flags1,\
+   VIR_FLATTEN_1(chain1));   \
+TEST_ONE_CHAIN(#id "b"

[libvirt] [PATCH 0/5] virstoragefile refactoring, part 3

2014-04-03 Thread Eric Blake
Part 2 gave us a common virStorageSource struct, now it's time
to start using that struct when crawling backing file chains.
I posted some RFCs about my full conversion plan, here's the
patches I have working so far.  Still plenty more to come, but
today's batch of patches took me a lot longer than I had
planned due to having to refactor the testsuite to avoid a
compiler error about over-large stack allocation.

Eric Blake (5):
  tests: use C99 initialization for storage test
  tests: refactor virstoragetest for less stack space
  conf: track when storage type is still undetermined
  conf: track more fields in backing chain metadata
  conf: start testing contents of the new backing chain fields

 src/conf/domain_conf.c|  10 +-
 src/conf/snapshot_conf.c  |   2 +-
 src/qemu/qemu_command.c   |   1 +
 src/qemu/qemu_driver.c|  11 +-
 src/util/virstoragefile.c |  33 +++-
 src/util/virstoragefile.h |  43 -
 tests/virstoragetest.c| 405 --
 7 files changed, 366 insertions(+), 139 deletions(-)

-- 
1.9.0

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


[libvirt] [PATCH 1/5] tests: use C99 initialization for storage test

2014-04-03 Thread Eric Blake
Writing this test with C99 initializers will make it easier to test
additions and deletions to struct members as I refactor the code.

* tests/virstoragetest.c (mymain): Rewrite initializers.

Signed-off-by: Eric Blake 
---
 tests/virstoragetest.c | 105 +
 1 file changed, 80 insertions(+), 25 deletions(-)

diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c
index 9ec39c7..9a3b3a3 100644
--- a/tests/virstoragetest.c
+++ b/tests/virstoragetest.c
@@ -339,60 +339,115 @@ mymain(void)

 /* Expected details about files in chains */
 const testFileData raw = {
-NULL, NULL, NULL, VIR_STORAGE_FILE_NONE, false, 0, false,
+.expFormat = VIR_STORAGE_FILE_NONE,
 };
 const testFileData qcow2_relback_relstart = {
-canonraw, "raw", ".", VIR_STORAGE_FILE_RAW, true, 1024, false,
+.expBackingStore = canonraw,
+.expBackingStoreRaw = "raw",
+.expDirectory = ".",
+.expFormat = VIR_STORAGE_FILE_RAW,
+.expIsFile = true,
+.expCapacity = 1024,
 };
 const testFileData qcow2_relback_absstart = {
-canonraw, "raw", datadir, VIR_STORAGE_FILE_RAW, true, 1024, false,
+.expBackingStore = canonraw,
+.expBackingStoreRaw = "raw",
+.expDirectory = datadir,
+.expFormat = VIR_STORAGE_FILE_RAW,
+.expIsFile = true,
+.expCapacity = 1024,
 };
 const testFileData qcow2_absback = {
-canonraw, absraw, datadir, VIR_STORAGE_FILE_RAW, true, 1024, false,
+.expBackingStore = canonraw,
+.expBackingStoreRaw = absraw,
+.expDirectory = datadir,
+.expFormat = VIR_STORAGE_FILE_RAW,
+.expIsFile = true,
+.expCapacity = 1024,
 };
 const testFileData qcow2_as_probe = {
-canonraw, absraw, datadir, VIR_STORAGE_FILE_AUTO, true, 1024, false,
+.expBackingStore = canonraw,
+.expBackingStoreRaw = absraw,
+.expDirectory = datadir,
+.expFormat = VIR_STORAGE_FILE_AUTO,
+.expIsFile = true,
+.expCapacity = 1024,
 };
 const testFileData qcow2_bogus = {
-NULL, datadir "/bogus", datadir, VIR_STORAGE_FILE_NONE,
-false, 1024, false,
+.expBackingStoreRaw = datadir "/bogus",
+.expDirectory = datadir,
+.expFormat = VIR_STORAGE_FILE_NONE,
+.expCapacity = 1024,
 };
 const testFileData qcow2_protocol = {
-"nbd:example.org:6000", NULL, NULL, VIR_STORAGE_FILE_RAW,
-false, 1024, false,
+.expBackingStore = "nbd:example.org:6000",
+.expFormat = VIR_STORAGE_FILE_RAW,
+.expCapacity = 1024,
 };
 const testFileData wrap = {
-canonqcow2, absqcow2, datadir, VIR_STORAGE_FILE_QCOW2,
-true, 1024, false,
+.expBackingStore = canonqcow2,
+.expBackingStoreRaw = absqcow2,
+.expDirectory = datadir,
+.expFormat = VIR_STORAGE_FILE_QCOW2,
+.expIsFile = true,
+.expCapacity = 1024,
 };
 const testFileData wrap_as_raw = {
-canonqcow2, absqcow2, datadir, VIR_STORAGE_FILE_RAW,
-true, 1024, false,
+.expBackingStore = canonqcow2,
+.expBackingStoreRaw = absqcow2,
+.expDirectory = datadir,
+.expFormat = VIR_STORAGE_FILE_RAW,
+.expIsFile = true,
+.expCapacity = 1024,
 };
 const testFileData wrap_as_probe = {
-canonqcow2, absqcow2, datadir, VIR_STORAGE_FILE_AUTO,
-true, 1024, false,
+.expBackingStore = canonqcow2,
+.expBackingStoreRaw = absqcow2,
+.expDirectory = datadir,
+.expFormat = VIR_STORAGE_FILE_AUTO,
+.expIsFile = true,
+.expCapacity = 1024,
 };
 const testFileData qed = {
-canonraw, absraw, datadir, VIR_STORAGE_FILE_RAW,
-true, 1024, false,
+.expBackingStore = canonraw,
+.expBackingStoreRaw = absraw,
+.expDirectory = datadir,
+.expFormat = VIR_STORAGE_FILE_RAW,
+.expIsFile = true,
+.expCapacity = 1024,
 };
 #if HAVE_SYMLINK
 const testFileData link1_rel = {
-canonraw, "../raw", "sub/../sub/..", VIR_STORAGE_FILE_RAW,
-true, 1024, false,
+.expBackingStore = canonraw,
+.expBackingStoreRaw = "../raw",
+.expDirectory = "sub/../sub/..",
+.expFormat = VIR_STORAGE_FILE_RAW,
+.expIsFile = true,
+.expCapacity = 1024,
 };
 const testFileData link1_abs = {
-canonraw, "../raw", datadir "/sub/../sub/..", VIR_STORAGE_FILE_RAW,
-true, 1024, false,
+.expBackingStore = canonraw,
+.expBackingStoreRaw = "../raw",
+.expDirectory = datadir "/sub/../sub/..",
+.expFormat = VIR_STORAGE_FILE_RAW,
+.expIsFile = true,
+.expCapacity = 1024,
 };
 const testFileData link2_rel = {
-canonqcow2, "../sub/link1", "sub/../sub", VIR_STORAGE_FILE_QCOW2,
-true, 1024, false,
+

[libvirt] [PATCH v2] Define CPUINFO_FILE_LEN and fix maxlen of cpuinfo file for all uses

2014-04-03 Thread Olivia Yin
For example, the file /proc/cpuinfo for 24 cores PowerPC platform is larger than
the previous maximum size 2KB.
It will fail to start libvirtd with the error message as below:
virFileReadAll: Failed to read file '/proc/cpuinfo': Value too large for defined
data type
virSysinfoRead: internal error Failed to open /proc/cpuinfo

This patch defines CPUINFO_FILE_LEN as 10KB which is enough for most 
architectures.

Signed-off-by: Olivia Yin 
---
 src/util/virsysinfo.c | 6 +++---
 src/util/virsysinfo.h | 2 ++
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/src/util/virsysinfo.c b/src/util/virsysinfo.c
index 7b16157..873872c 100644
--- a/src/util/virsysinfo.c
+++ b/src/util/virsysinfo.c
@@ -223,7 +223,7 @@ virSysinfoRead(void)
 if (VIR_ALLOC(ret) < 0)
 goto no_memory;
 
-if (virFileReadAll(CPUINFO, 2048, &outbuf) < 0) {
+if (virFileReadAll(CPUINFO, CPUINFO_FILE_LEN, &outbuf) < 0) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
_("Failed to open %s"), CPUINFO);
 return NULL;
@@ -341,7 +341,7 @@ virSysinfoRead(void)
 if (VIR_ALLOC(ret) < 0)
 goto no_memory;
 
-if (virFileReadAll(CPUINFO, 2048, &outbuf) < 0) {
+if (virFileReadAll(CPUINFO, CPUINFO_FILE_LEN, &outbuf) < 0) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
_("Failed to open %s"), CPUINFO);
 return NULL;
@@ -470,7 +470,7 @@ virSysinfoRead(void)
 goto no_memory;
 
 /* Gather info from /proc/cpuinfo */
-if (virFileReadAll(CPUINFO, 8192, &outbuf) < 0) {
+if (virFileReadAll(CPUINFO, CPUINFO_FILE_LEN, &outbuf) < 0) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
_("Failed to open %s"), CPUINFO);
 return NULL;
diff --git a/src/util/virsysinfo.h b/src/util/virsysinfo.h
index fbb505b..b5336e2 100644
--- a/src/util/virsysinfo.h
+++ b/src/util/virsysinfo.h
@@ -102,4 +102,6 @@ bool virSysinfoIsEqual(virSysinfoDefPtr src,
 
 VIR_ENUM_DECL(virSysinfo)
 
+#define CPUINFO_FILE_LEN (10*1024) /*10KB limit for /proc/cpuinfo file*/
+
 #endif /* __VIR_SYSINFOS_H__ */
-- 
1.8.5


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


Re: [libvirt] [PATCH] Define CPUINFO_FILE_LEN and fix maxlen of cpuinfo file for powerpc

2014-04-03 Thread hong-hua....@freescale.com
Hi Eric,

Thanks for comments. I'll submit a new revision of this patch.

Best Regards,
Olivia

> -Original Message-
> From: Eric Blake [mailto:ebl...@redhat.com]
> Sent: Thursday, April 03, 2014 8:25 PM
> To: Yin Olivia-R63875; libvir-list@redhat.com
> Subject: Re: [libvirt] [PATCH] Define CPUINFO_FILE_LEN and fix maxlen of
> cpuinfo file for powerpc
> 
> On 04/03/2014 12:43 AM, Olivia Yin wrote:
> > This patch define CPUINFO_FILE_LEN as 2KB which is enough for most
> architectures.
> >
> > For the 24 cores PowerPC machines, the file of /proc/cpuinfo will be
> larger than 2KB.
> > It will fail to start libvirtd with the error message as below:
> > virFileReadAll: Failed to read file '/proc/cpuinfo': Value too large
> > for defined data type
> > virSysinfoRead: internal error Failed to open /proc/cpuinfo
> >
> > So this patch increases the maxlen of cpuinfo for PowerPC architecture.
> >
> > Signed-off-by: Olivia Yin 
> > ---
> >  src/util/virsysinfo.c | 6 +++---
> >  src/util/virsysinfo.h | 2 ++
> >  2 files changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/src/util/virsysinfo.c b/src/util/virsysinfo.c index
> > 7b16157..33c4bc6 100644
> > --- a/src/util/virsysinfo.c
> > +++ b/src/util/virsysinfo.c
> > @@ -223,7 +223,7 @@ virSysinfoRead(void)
> >  if (VIR_ALLOC(ret) < 0)
> >  goto no_memory;
> >
> > -if (virFileReadAll(CPUINFO, 2048, &outbuf) < 0) {
> > +if (virFileReadAll(CPUINFO, 2 * CPUINFO_FILE_LEN, &outbuf) < 0) {
> 
> Uggh.  Having to multiply the constant means you defined the constant wrong.
> Just make CPUINFO_FILE_LEN big enough.  Even as large as 1M, for ALL uses,
> is not a burden.
> 
> > +++ b/src/util/virsysinfo.h
> > @@ -102,4 +102,6 @@ bool virSysinfoIsEqual(virSysinfoDefPtr src,
> >
> >  VIR_ENUM_DECL(virSysinfo)
> >
> > +#define CPUINFO_FILE_LEN   2048/*2KB limit for normal cpuinfo file*/
> > +
> >  #endif /* __VIR_SYSINFOS_H__ */
> >
> 
> --
> Eric Blake   eblake redhat com+1-919-301-3266
> Libvirt virtualization library http://libvirt.org


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


[libvirt] Enhancing clean-traffic to work with IPv6

2014-04-03 Thread Brian Rak
I'm looking into adding IPv6 support to the nwfilter clean-traffic 
rules, but I'm unsure of the best approach to this.  I'm planning on 
sending patches once I get this correct, so I'm trying to figure out 
what way fits in best.


There's a couple different ways I can think of:

1) Explicitly add v6 rules to the existing clean-traffic rules. This 
would enable IPv6 for guests whenever libvirt was upgraded, which may be 
a problem.
2) Add another filter chain (clean-ipv6-traffic) that would do the same 
thing as clean-traffic, just for IPv6
3) Add another filter chain (clean-ipv6-ipv4-traffic), that would clean 
IPv6 traffic, and include the clean-traffic filter set


The limitation here is that IP learning will not work for IPv6, so 
actually using IPv6 is going to require passing in parameters to filter 
specifying what ranges the guest should be allowed to use.  I think this 
rules out #1.


Any suggestions?

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


[libvirt] [PATCH] Fix Memory Leak in daemon/libvirtd.c

2014-04-03 Thread Nehal J Wani
Fixes leak introduced by e562e82f

==4937== 64 bytes in 1 blocks are definitely lost in loss record 270 of 405
==4937==at 0x4A06BE0: realloc (vg_replace_malloc.c:662)
==4937==by 0x6FA41C4: __vasprintf_chk (vasprintf_chk.c:90)
==4937==by 0x50C8D29: virVasprintfInternal (stdio2.h:199)
==4937==by 0x50C8E3A: virAsprintfInternal (virstring.c:362)
==4937==by 0x11D01A: main (libvirtd.c:1170)

---
 daemon/libvirtd.c |3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
index e247259..bb84c90 100644
--- a/daemon/libvirtd.c
+++ b/daemon/libvirtd.c
@@ -1132,6 +1132,7 @@ int main(int argc, char **argv) {
 bool privileged = geteuid() == 0 ? true : false;
 bool implicit_conf = false;
 char *run_dir = NULL;
+char *cpumap = NULL;
 mode_t old_umask;
 
 struct option opts[] = {
@@ -1159,7 +1160,6 @@ int main(int argc, char **argv) {
 if (strstr(argv[0], "lt-libvirtd") ||
 strstr(argv[0], "/daemon/.libs/libvirtd")) {
 char *tmp = strrchr(argv[0], '/');
-char *cpumap;
 if (!tmp) {
 fprintf(stderr, _("%s: cannot identify driver directory\n"), 
argv[0]);
 exit(EXIT_FAILURE);
@@ -1182,6 +1182,7 @@ int main(int argc, char **argv) {
 virDriverModuleInitialize(driverdir);
 #endif
 cpuMapOverride(cpumap);
+VIR_FREE(cpumap);
 *tmp = '/';
 /* Must not free 'driverdir' - it is still used */
 }
-- 
1.7.1

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


Re: [libvirt] [PATCH v1 0/2] Keep original file label

2014-04-03 Thread Michal Privoznik

On 13.03.2014 10:02, Michal Privoznik wrote:

This creates a new file to store the original file labels.
Or do we want the whole functionality to reside in virtlockd?

Michal Privoznik (2):
   security_dac: Keep original file label
   security_dac: Lock label store file

  src/Makefile.am  |   3 +-
  src/locking/lock_driver.h|   2 +
  src/locking/lock_driver_lockd.c  |  25 +++
  src/lxc/lxc_controller.c |   2 +-
  src/lxc/lxc_driver.c |   3 +-
  src/qemu/qemu_driver.c   |   9 +-
  src/security/security_dac.c  | 412 +++
  src/security/security_manager.c  |  24 ++-
  src/security/security_manager.h  |   7 +-
  tests/qemuhotplugtest.c  |   2 +-
  tests/seclabeltest.c |   2 +-
  tests/securityselinuxlabeltest.c |   3 +-
  tests/securityselinuxtest.c  |   2 +-
  13 files changed, 438 insertions(+), 58 deletions(-)



Ping?

Michal

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


Re: [libvirt] libxl fixes/improvements for libvirt

2014-04-03 Thread Stefan Bader
On 03.04.2014 17:45, Michal Privoznik wrote:
> On 27.03.2014 17:55, Stefan Bader wrote:
>> Here several changes which improve the handling of Xen for me:
>>
>> * 0001-libxl-Use-id-from-virDomainObj-inside-the-driver.patch
>>This is a re-send as I initially submitted that as a reply to some
>>discussion. Starting from the visibly broken libxlDomainGetInfo when
>>creating or rebooting a guest with virt-manager, I replaced all usage
>>of dom->id with the more stable vm->def->id.
>> * 0002-libxl-Set-disk-format-for-empty-cdrom-device.patch
>>Fixes start of a guest after ejecting the iso image from a virtual
>>CDROM drive (again using virt-manager).
>> * 0003-libxl-Implement-basic-video-device-selection.patch
>>Should fix the occasional disagreement about the used VNC port and
>>adds the ability to select the standard VGA graphics from Xen.
>>VRAM size seemed to work with that. Only for Cirrus, while the qemu
>>command-line looks good, ones seems to end up with 32M.
>>
>> Note to people on the Xen list: I wonder whether libxenlight internally
>> should somehow should fix up the situation where a caller sets up the
>> video devices in the vfbs list but does not sync that with the information
>> in the build info. Question probably is what the semantics should be.
>>
>> -Stefan
>>
> 
> Just for the record, I've pushed the first two patches. The last one has some
> issues, so I rather see it's second version.
> 
> Michal
> 
Ack, thanks. I will send out an updated version as soon as I get it done. Next
on my list though I cannot exactly tell when "next" happens. :)

-Stefan



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

Re: [libvirt] libxl fixes/improvements for libvirt

2014-04-03 Thread Michal Privoznik

On 27.03.2014 17:55, Stefan Bader wrote:

Here several changes which improve the handling of Xen for me:

* 0001-libxl-Use-id-from-virDomainObj-inside-the-driver.patch
   This is a re-send as I initially submitted that as a reply to some
   discussion. Starting from the visibly broken libxlDomainGetInfo when
   creating or rebooting a guest with virt-manager, I replaced all usage
   of dom->id with the more stable vm->def->id.
* 0002-libxl-Set-disk-format-for-empty-cdrom-device.patch
   Fixes start of a guest after ejecting the iso image from a virtual
   CDROM drive (again using virt-manager).
* 0003-libxl-Implement-basic-video-device-selection.patch
   Should fix the occasional disagreement about the used VNC port and
   adds the ability to select the standard VGA graphics from Xen.
   VRAM size seemed to work with that. Only for Cirrus, while the qemu
   command-line looks good, ones seems to end up with 32M.

Note to people on the Xen list: I wonder whether libxenlight internally
should somehow should fix up the situation where a caller sets up the
video devices in the vfbs list but does not sync that with the information
in the build info. Question probably is what the semantics should be.

-Stefan



Just for the record, I've pushed the first two patches. The last one has 
some issues, so I rather see it's second version.


Michal

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


[libvirt] [PATCH v5 5/5] virsh: Expose new virDomainFSFreeze and virDomainFSThaw API

2014-04-03 Thread Tomoki Sekiyama
These are exposed under domfsfreeze command and domfsthaw command.

Signed-off-by: Tomoki Sekiyama 
---
 tools/virsh-domain.c |  128 ++
 tools/virsh.pod  |   23 +
 2 files changed, 151 insertions(+)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 73414f8..331ba36 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -11391,6 +11391,122 @@ cmdDomFSTrim(vshControl *ctl, const vshCmd *cmd)
 return ret;
 }
 
+static const vshCmdInfo info_domfsfreeze[] = {
+{.name = "help",
+ .data = N_("Freeze domain's mounted filesystems.")
+},
+{.name = "desc",
+ .data = N_("Freeze domain's mounted filesystems.")
+},
+{.name = NULL}
+};
+
+static const vshCmdOptDef opts_domfsfreeze[] = {
+{.name = "domain",
+ .type = VSH_OT_DATA,
+ .flags = VSH_OFLAG_REQ,
+ .help = N_("domain name, id or uuid")
+},
+{.name = "disks",
+ .type = VSH_OT_DATA,
+ .help = N_("comma separated list of disks to be frozen")
+},
+{.name = NULL}
+};
+static bool
+cmdDomFSFreeze(vshControl *ctl, const vshCmd *cmd)
+{
+virDomainPtr dom = NULL;
+bool ret = false;
+unsigned int flags = 0;
+const char *disk_string = NULL;
+char **disk_list = NULL;  /* tokenized disk_string */
+int ndisks = 0;
+size_t i;
+
+ignore_value(vshCommandOptString(cmd, "disks", &disk_string));
+
+if (!(dom = vshCommandOptDomain(ctl, cmd, NULL)))
+return ret;
+
+if (disk_string) {
+if ((ndisks = vshStringToArray(disk_string, &disk_list)) < 0)
+goto cleanup;
+}
+
+if (virDomainFSFreeze(dom, (const char **)disk_list, ndisks, flags) < 0) {
+vshError(ctl, _("Unable to freeze filesystems"));
+goto cleanup;
+}
+
+ret = true;
+
+ cleanup:
+for (i = 0; i < ndisks; i++)
+VIR_FREE(disk_list[i]);
+VIR_FREE(disk_list);
+virDomainFree(dom);
+return ret;
+}
+
+static const vshCmdInfo info_domfsthaw[] = {
+{.name = "help",
+ .data = N_("Thaw domain's mounted filesystems.")
+},
+{.name = "desc",
+ .data = N_("Thaw domain's mounted filesystems.")
+},
+{.name = NULL}
+};
+
+static const vshCmdOptDef opts_domfsthaw[] = {
+{.name = "domain",
+ .type = VSH_OT_DATA,
+ .flags = VSH_OFLAG_REQ,
+ .help = N_("domain name, id or uuid")
+},
+{.name = "disks",
+ .type = VSH_OT_DATA,
+ .help = N_("comma separated list of disks to be thawed")
+},
+{.name = NULL}
+};
+static bool
+cmdDomFSThaw(vshControl *ctl, const vshCmd *cmd)
+{
+virDomainPtr dom = NULL;
+bool ret = false;
+unsigned int flags = 0;
+const char *disk_string = NULL;
+char **disk_list = NULL;  /* tokenized disk_string */
+int ndisks = 0;
+size_t i;
+
+ignore_value(vshCommandOptString(cmd, "disks", &disk_string));
+
+if (!(dom = vshCommandOptDomain(ctl, cmd, NULL)))
+return ret;
+
+if (disk_string) {
+if ((ndisks = vshStringToArray(disk_string, &disk_list)) < 0)
+goto cleanup;
+}
+
+if (virDomainFSThaw(dom, (const char **)disk_list, ndisks, flags) < 0) {
+vshError(ctl, _("Unable to thaw filesystems"));
+goto cleanup;
+}
+
+ret = true;
+
+ cleanup:
+for (i = 0; i < ndisks; i++)
+VIR_FREE(disk_list[i]);
+VIR_FREE(disk_list);
+virDomainFree(dom);
+return ret;
+}
+
 const vshCmdDef domManagementCmds[] = {
 {.name = "attach-device",
  .handler = cmdAttachDevice,
@@ -11538,6 +11654,18 @@ const vshCmdDef domManagementCmds[] = {
  .info = info_domdisplay,
  .flags = 0
 },
+{.name = "domfsfreeze",
+ .handler = cmdDomFSFreeze,
+ .opts = opts_domfsfreeze,
+ .info = info_domfsfreeze,
+ .flags = 0
+},
+{.name = "domfsthaw",
+ .handler = cmdDomFSThaw,
+ .opts = opts_domfsthaw,
+ .info = info_domfsthaw,
+ .flags = 0
+},
 {.name = "domfstrim",
  .handler = cmdDomFSTrim,
  .opts = opts_domfstrim,
diff --git a/tools/virsh.pod b/tools/virsh.pod
index 98d891a..652aabc 100644
--- a/tools/virsh.pod
+++ b/tools/virsh.pod
@@ -941,6 +941,29 @@ Output a URI which can be used to connect to the graphical 
display of the
 domain via VNC, SPICE or RDP. If I<--include-password> is specified, the
 SPICE channel password will be included in the URI.
 
+=item B I [I<--disks> B]
+
+Freeze all mounted filesystems within a running domain to prepare for
+consistent snapshots.
+
+The I<--disks> flag takes a parameter B, which is a comma separated
+list of names of disks to be frozen. If this is not specified, every mounted
+filesystem on the guest is frozen.
+
+Note that B command has a I<--quiesce> option to freeze
+and thaw the filesystems automatically to keep snapshots consistent.
+B command is only needed when a user wants to utilize the
+native snapshot features of storage devices not supported by libvirt yet.
+
+=item B I [I<--disks> B]
+

[libvirt] [PATCH v5 3/5] qemu: Track domain quiesced status and make fsfreeze/thaw nestable

2014-04-03 Thread Tomoki Sekiyama
Adds 'quiesced' counter into qemuDomainObjPrivate that tracks how many times
fsfreeze is requested in the domain.
When qemuDomainSnapshotFSFreeze is called, the counter is incremented. The
fsfreeze request is sent to the guest agent when the counter changes from 0
to 1. qemuDomainSnapshotFSThaw decrements the counter and requests fsthaw
when it becomes 0.

It also modify error code from qemuDomainSnapshotFSFreeze and
qemuDomainSnapshotFSThaw, so that a caller can know whether the command is
actually sent to the guest agent. If the error is caused before sending a
freeze command, a counterpart thaw command shouldn't be sent either.

Signed-off-by: Tomoki Sekiyama 
---
 src/qemu/qemu_domain.c |6 
 src/qemu/qemu_domain.h |2 +
 src/qemu/qemu_driver.c |   65 ++--
 3 files changed, 59 insertions(+), 14 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index cdd4601..6ed3d67 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -357,6 +357,9 @@ qemuDomainObjPrivateXMLFormat(virBufferPtr buf, void *data)
 virBufferAddLit(buf, "\n");
 }
 
+if (priv->quiesced)
+virBufferAsprintf(buf, "\n", priv->quiesced);
+
 return 0;
 }
 
@@ -518,6 +521,9 @@ qemuDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt, void 
*data)
 }
 VIR_FREE(nodes);
 
+if (virXPathInt("string(./quiesced/@depth)", ctxt, &priv->quiesced) < 0)
+priv->quiesced = 0;
+
 return 0;
 
  error:
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index b2830c4..620a258 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -176,6 +176,8 @@ struct _qemuDomainObjPrivate {
 char **qemuDevices; /* NULL-terminated list of devices aliases known to 
QEMU */
 
 bool hookRun;  /* true if there was a hook run over this domain */
+
+int quiesced; /* count how many times filesystems are quiesced */
 };
 
 typedef enum {
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 9a2de12..2bb5fa9 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -12100,32 +12100,68 @@ qemuDomainPrepareDiskChainElement(virQEMUDriverPtr 
driver,
 }
 
 
+/* Return -1 if request is not sent to agent due to misconfig, -2 if request
+ * is sent but failed, and number of frozen filesystems on success. */
 static int
-qemuDomainSnapshotFSFreeze(virDomainObjPtr vm)
+qemuDomainSnapshotFSFreeze(virQEMUDriverPtr driver, virDomainObjPtr vm)
 {
 qemuDomainObjPrivatePtr priv = vm->privateData;
+virQEMUDriverConfigPtr cfg;
 int freezed;
 
 if (!qemuDomainAgentAvailable(priv, true))
 return -1;
 
+priv->quiesced++;
+cfg = virQEMUDriverGetConfig(driver);
+if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) {
+virObjectUnref(cfg);
+priv->quiesced--;
+return -1;
+}
+virObjectUnref(cfg);
+
+if (priv->quiesced > 1)
+return 0;
+
 qemuDomainObjEnterAgent(vm);
 freezed = qemuAgentFSFreeze(priv->agent);
 qemuDomainObjExitAgent(vm);
-
-return freezed;
+return freezed < 0 ? -2 : freezed;
 }
 
+/* Return -1 if request is not sent to agent due to misconfig, -2 if request
+ * is sent but failed, and number of thawed filesystems on success. */
 static int
-qemuDomainSnapshotFSThaw(virDomainObjPtr vm, bool report)
+qemuDomainSnapshotFSThaw(virQEMUDriverPtr driver,
+ virDomainObjPtr vm, bool report)
 {
 qemuDomainObjPrivatePtr priv = vm->privateData;
+virQEMUDriverConfigPtr cfg;
 int thawed;
 virErrorPtr err = NULL;
 
 if (!qemuDomainAgentAvailable(priv, report))
 return -1;
 
+if (!priv->quiesced && report) {
+virReportError(VIR_ERR_OPERATION_INVALID, "%s",
+   _("domain is not quiesced"));
+return -1;
+}
+
+priv->quiesced--;
+cfg = virQEMUDriverGetConfig(driver);
+if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) {
+virObjectUnref(cfg);
+priv->quiesced++;
+return -1;
+}
+virObjectUnref(cfg);
+
+if (priv->quiesced)
+return 0;
+
 qemuDomainObjEnterAgent(vm);
 if (!report)
 err = virSaveLastError();
@@ -12135,7 +12171,7 @@ qemuDomainSnapshotFSThaw(virDomainObjPtr vm, bool 
report)
 qemuDomainObjExitAgent(vm);
 
 virFreeError(err);
-return thawed;
+return thawed < 0 ? -2 : thawed;
 }
 
 /* The domain is expected to be locked and inactive. */
@@ -13116,17 +13152,18 @@ qemuDomainSnapshotCreateActiveExternal(virConnectPtr 
conn,
 goto cleanup;
 
 /* If quiesce was requested, then issue a freeze command, and a
- * counterpart thaw command, no matter what.  The command will
- * fail if the guest is paused or the guest agent is not
- * running.  */
+ * counterpart thaw command when it is actually sent to agent.
+ * The command will fail if the guest is paused or the guest agent
+ * is not run

[libvirt] [PATCH v5 1/5] Introduce virDomainFSFreeze() and virDomainFSThaw() public API

2014-04-03 Thread Tomoki Sekiyama
These will freeze and thaw filesystems within guest. The APIs take @disks
and @ndisks parameters to specify disks to be frozen or thawed.
The parameters can be NULL and 0, then the all mounted filesystes are
frozen or thawed. If some disks are frozen multiple times, they are not
thawed until requested to be thawed as many times as freeze request.
@flags parameter, which are currently not used, is for future extensions.

Signed-off-by: Tomoki Sekiyama 
---
 include/libvirt/libvirt.h.in |   10 +
 src/driver.h |   14 ++
 src/libvirt.c|   92 ++
 src/libvirt_public.syms  |6 +++
 4 files changed, 122 insertions(+)

diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
index 930b7e8..d408f19 100644
--- a/include/libvirt/libvirt.h.in
+++ b/include/libvirt/libvirt.h.in
@@ -5277,6 +5277,16 @@ int virDomainFSTrim(virDomainPtr dom,
 unsigned long long minimum,
 unsigned int flags);
 
+int virDomainFSFreeze(virDomainPtr dom,
+  const char **disks,
+  unsigned int ndisks,
+  unsigned int flags);
+
+int virDomainFSThaw(virDomainPtr dom,
+const char **disks,
+unsigned int ndisks,
+unsigned int flags);
+
 /**
  * virSchedParameterType:
  *
diff --git a/src/driver.h b/src/driver.h
index e66fc7a..5c81d96 100644
--- a/src/driver.h
+++ b/src/driver.h
@@ -1149,6 +1149,18 @@ typedef int
  unsigned int flags,
  int cancelled);
 
+typedef int
+(*virDrvDomainFSFreeze)(virDomainPtr dom,
+const char **disks,
+unsigned int ndisks,
+unsigned int flags);
+
+typedef int
+(*virDrvDomainFSThaw)(virDomainPtr dom,
+  const char **disks,
+  unsigned int ndisks,
+  unsigned int flags);
+
 typedef struct _virDriver virDriver;
 typedef virDriver *virDriverPtr;
 
@@ -1363,6 +1375,8 @@ struct _virDriver {
 virDrvDomainMigrateFinish3Params domainMigrateFinish3Params;
 virDrvDomainMigrateConfirm3Params domainMigrateConfirm3Params;
 virDrvConnectGetCPUModelNames connectGetCPUModelNames;
+virDrvDomainFSFreeze domainFSFreeze;
+virDrvDomainFSThaw domainFSThaw;
 };
 
 
diff --git a/src/libvirt.c b/src/libvirt.c
index 4454829..43614b5 100644
--- a/src/libvirt.c
+++ b/src/libvirt.c
@@ -20658,3 +20658,95 @@ virDomainFSTrim(virDomainPtr dom,
 virDispatchError(dom->conn);
 return -1;
 }
+
+/**
+ * virDomainFSFreeze:
+ * @dom: a domain object
+ * @disks: list of disk names to be frozen
+ * @ndisks: the number of disks specified in @disks
+ * @flags: extra flags, not used yet, so callers should always pass 0
+ *
+ * Freeze filesystems on the specified disks within the guest (hence guest
+ * agent may be required depending on hypervisor used). If @ndisks is 0,
+ * every mounted filesystem on the guest is frozen.
+ * Freeze can be nested. When it is called on the same disk multiple times,
+ * the disk will not be thawed until virDomainFSThaw is called the same times.
+ *
+ * Returns 0 on success, -1 otherwise.
+ */
+int
+virDomainFSFreeze(virDomainPtr dom,
+  const char **disks,
+  unsigned int ndisks,
+  unsigned int flags)
+{
+VIR_DOMAIN_DEBUG(dom, "disks=%p, ndisks=%d, flags=%x",
+ disks, ndisks, flags);
+
+virResetLastError();
+
+virCheckDomainReturn(dom, -1);
+virCheckReadOnlyGoto(dom->conn->flags, error);
+if (ndisks)
+virCheckNonNullArgGoto(disks, error);
+else
+virCheckNullArgGoto(disks, error);
+
+if (dom->conn->driver->domainFSFreeze) {
+int ret = dom->conn->driver->domainFSFreeze(dom, disks, ndisks, flags);
+if (ret < 0)
+goto error;
+return ret;
+}
+
+virReportUnsupportedError();
+
+ error:
+virDispatchError(dom->conn);
+return -1;
+}
+
+/**
+ * virDomainFSThaw:
+ * @dom: a domain object
+ * @disks: list of disk names to be thawed
+ * @ndisks: the number of disks specified in @disks
+ * @flags: extra flags, not used yet, so callers should always pass 0
+ *
+ * Thaw filesystems on the specified disks within the guest (hence guest
+ * agent may be required depending on hypervisor used). If @ndisks is 0,
+ * every mounted filesystem on the guest is thawed.
+ *
+ * Returns 0 on success, -1 otherwise.
+ */
+int
+virDomainFSThaw(virDomainPtr dom,
+const char **disks,
+unsigned int ndisks,
+unsigned int flags)
+{
+VIR_DOMAIN_DEBUG(dom, "disks=%p, ndisks=%d, flags=%x",
+ disks, ndisks, flags);
+
+virResetLastError();
+
+virCheckDomainReturn(dom, -1);
+virCheckReadOnlyGoto(dom->conn->flags, error);
+if (ndisks)
+vir

[libvirt] [PATCH v5 2/5] remote: Implement virDomainFSFreeze and virDomainFSThaw

2014-04-03 Thread Tomoki Sekiyama
New rules are added in fixup_name in gendispatch.pl to keep the name
FSFreeze and FSThaw. This adds a new ACL permission 'fs_freeze',
which is also applied to VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE flag.

Signed-off-by: Tomoki Sekiyama 
---
 src/access/viraccessperm.c   |2 +-
 src/access/viraccessperm.h   |6 ++
 src/remote/remote_driver.c   |2 ++
 src/remote/remote_protocol.x |   30 --
 src/remote_protocol-structs  |   18 ++
 src/rpc/gendispatch.pl   |2 ++
 6 files changed, 57 insertions(+), 3 deletions(-)

diff --git a/src/access/viraccessperm.c b/src/access/viraccessperm.c
index d517c66..462f46c 100644
--- a/src/access/viraccessperm.c
+++ b/src/access/viraccessperm.c
@@ -39,7 +39,7 @@ VIR_ENUM_IMPL(virAccessPermDomain,
   "start", "stop", "reset",
   "save", "delete",
   "migrate", "snapshot", "suspend", "hibernate", "core_dump", 
"pm_control",
-  "init_control", "inject_nmi", "send_input", "send_signal", 
"fs_trim",
+  "init_control", "inject_nmi", "send_input", "send_signal", 
"fs_trim", "fs_freeze",
   "block_read", "block_write", "mem_read",
   "open_graphics", "open_device", "screenshot",
   "open_namespace");
diff --git a/src/access/viraccessperm.h b/src/access/viraccessperm.h
index 6d14f05..ac48d70 100644
--- a/src/access/viraccessperm.h
+++ b/src/access/viraccessperm.h
@@ -242,6 +242,12 @@ typedef enum {
  */
 VIR_ACCESS_PERM_DOMAIN_FS_TRIM,  /* Issue TRIM to guest filesystems */
 
+/**
+ * @desc: Freeze and thaw domain filesystems
+ * @message: Freezing and thawing domain filesystems require authorization
+ */
+VIR_ACCESS_PERM_DOMAIN_FS_FREEZE,/* Freeze and thaw guest filesystems 
*/
+
 /* Peeking at guest */
 
 /**
diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
index ed7dde6..8b39a90 100644
--- a/src/remote/remote_driver.c
+++ b/src/remote/remote_driver.c
@@ -7800,6 +7800,8 @@ static virDriver remote_driver = {
 .domainMigrateFinish3Params = remoteDomainMigrateFinish3Params, /* 1.1.0 */
 .domainMigrateConfirm3Params = remoteDomainMigrateConfirm3Params, /* 1.1.0 
*/
 .connectGetCPUModelNames = remoteConnectGetCPUModelNames, /* 1.1.3 */
+.domainFSFreeze = remoteDomainFSFreeze, /* 1.2.4 */
+.domainFSThaw = remoteDomainFSThaw, /* 1.2.4 */
 };
 
 static virNetworkDriver network_driver = {
diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x
index 6c445cc..13fa69b 100644
--- a/src/remote/remote_protocol.x
+++ b/src/remote/remote_protocol.x
@@ -235,6 +235,9 @@ const REMOTE_DOMAIN_JOB_STATS_MAX = 64;
 /* Upper limit on number of CPU models */
 const REMOTE_CONNECT_CPU_MODELS_MAX = 8192;
 
+/* Upper limit on number of disks to frozen */
+const REMOTE_DOMAIN_FSFREEZE_DISKS_MAX = 256;
+
 /* UUID.  VIR_UUID_BUFLEN definition comes from libvirt.h */
 typedef opaque remote_uuid[VIR_UUID_BUFLEN];
 
@@ -2959,6 +2962,17 @@ struct remote_network_event_lifecycle_msg {
 int detail;
 };
 
+struct remote_domain_fsfreeze_args {
+remote_nonnull_domain dom;
+remote_nonnull_string disks; /* (const 
char **) */
+unsigned int flags;
+};
+
+struct remote_domain_fsthaw_args {
+remote_nonnull_domain dom;
+remote_nonnull_string disks; /* (const 
char **) */
+unsigned int flags;
+};
 
 
 /*- Protocol. -*/
@@ -4289,7 +4303,7 @@ enum remote_procedure {
 /**
  * @generate: both
  * @acl: domain:snapshot
- * @acl: domain:write:VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE
+ * @acl: domain:fs_freeze:VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE
  */
 REMOTE_PROC_DOMAIN_SNAPSHOT_CREATE_XML = 185,
 
@@ -5275,5 +5289,17 @@ enum remote_procedure {
  * @generate: both
  * @acl: domain:core_dump
  */
-REMOTE_PROC_DOMAIN_CORE_DUMP_WITH_FORMAT = 334
+REMOTE_PROC_DOMAIN_CORE_DUMP_WITH_FORMAT = 334,
+
+/**
+ * @generate: both
+ * @acl: domain:fs_freeze
+ */
+REMOTE_PROC_DOMAIN_FSFREEZE = 335,
+
+/**
+ * @generate: both
+ * @acl: domain:fs_freeze
+ */
+REMOTE_PROC_DOMAIN_FSTHAW = 336
 };
diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs
index 456d0da..439ff87 100644
--- a/src/remote_protocol-structs
+++ b/src/remote_protocol-structs
@@ -2427,6 +2427,22 @@ struct remote_network_event_lifecycle_msg {
 intevent;
 intdetail;
 };
+struct remote_domain_fsfreeze_args {
+remote_nonnull_domain  dom;
+struct {
+u_int  disks_len;
+remote_nonnull_string * disks_val;
+} disks;
+u_int  flags;
+};
+struct remote_domain_fsthaw_args {
+remote_nonnull_domain  dom;
+struct {
+u_int  disks_len;
+remote_nonnull_string * disks_val;
+} disks;
+

[libvirt] [PATCH v5 0/5] Expose FSFreeze/FSThaw within the guest as API

2014-04-03 Thread Tomoki Sekiyama
Hello,

This is patchset v5 to add FSFreeze/FSThaw API for custom disk snapshotting.

Changes to v4:
 * add disks and ndisks parameter to specify disks to be frozen/thawed
 * make fsfreeze requests nestable
 * change api version to 1.2.4
 (v4: https://www.redhat.com/archives/libvir-list/2014-March/msg01674.html )

=== Description ===

Currently FSFreeze and FSThaw are supported by qemu guest agent and they are
used internally in snapshot-create command with --quiesce option.
However, when users want to utilize the native snapshot feature of storage
devices (such as LVM over iSCSI, enterprise storage appliances, etc.),
they need to issue fsfreeze command separately from libvirt-driven snapshots.
(OpenStack cinder provides these storages' snapshot feature, but it cannot
 quiesce the guest filesystems automatically for now.)

Although virDomainQemuGuestAgent() API could be used for this purpose, it
is only for debugging and is not supported officially.

This patchset adds virDomainFSFreeze()/virDomainFSThaw() APIs and virsh
domfsfreeze/domfsthaw commands to enable the users to freeze and thaw
domain's filesystems cleanly.

  
The APIs take disks and ndisks parameters, which is a list of disk names
to be frozen/thawed. If the option is not provided, every mounted
filesystem is frozen/thawed.

The fsfreeze can be nestable. When fsfreeze requests to a disk are issued
multiple times, it is not thawed until the fsthaw requests are issued as
many times as the freeze requests.

Currently, qemu driver doesn't support disks parameter because the guest
agent doesn't have means to specify disks to be frozen/thawed. Hence, it
just counts depth of fsfreeze per domain, not per disk, so far.
  

The APIs have flags option currently unsupported for future extension.
---

Tomoki Sekiyama (5):
  Introduce virDomainFSFreeze() and virDomainFSThaw() public API
  remote: Implement virDomainFSFreeze and virDomainFSThaw
  qemu: Track domain quiesced status and make fsfreeze/thaw nestable
  qemu: Implement virDomainFSFreeze and virDomainFSThaw
  virsh: Expose new virDomainFSFreeze and virDomainFSThaw API


 include/libvirt/libvirt.h.in |   10 +++
 src/access/viraccessperm.c   |2 -
 src/access/viraccessperm.h   |6 ++
 src/driver.h |   14 
 src/libvirt.c|   92 
 src/libvirt_public.syms  |6 ++
 src/qemu/qemu_domain.c   |6 ++
 src/qemu/qemu_domain.h   |2 +
 src/qemu/qemu_driver.c   |  159 ++
 src/remote/remote_driver.c   |2 +
 src/remote/remote_protocol.x |   30 +++-
 src/remote_protocol-structs  |   18 +
 src/rpc/gendispatch.pl   |2 +
 tools/virsh-domain.c |  128 ++
 tools/virsh.pod  |   23 ++
 15 files changed, 483 insertions(+), 17 deletions(-)

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


[libvirt] [PATCH v5 4/5] qemu: Implement virDomainFSFreeze and virDomainFSThaw

2014-04-03 Thread Tomoki Sekiyama
Use qemuDomainSnapshotFSFreeze() and qemuDomainSnapshotFSFThaw() which are
already implemented for snapshot quiescing. So far, disks and ndisks
parameters are unsupported, because the guest agent doesn't have means to
specify disks to be frozen or thawed.

Signed-off-by: Tomoki Sekiyama 
---
 src/qemu/qemu_driver.c |   94 
 1 file changed, 94 insertions(+)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 2bb5fa9..81ff473 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -16700,6 +16700,98 @@ qemuConnectGetCPUModelNames(virConnectPtr conn,
 }
 
 
+static int
+qemuDomainFSFreeze(virDomainPtr dom,
+   const char **disks,
+   unsigned int ndisks,
+   unsigned int flags)
+{
+virQEMUDriverPtr driver = dom->conn->privateData;
+virDomainObjPtr vm;
+int ret = -1;
+
+virCheckFlags(0, -1);
+
+if (disks || ndisks) {
+virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
+   _("Specifying disks is not supported for now"));
+return -1;
+}
+
+if (!(vm = qemuDomObjFromDomain(dom)))
+goto cleanup;
+
+if (virDomainFSFreezeEnsureACL(dom->conn, vm->def) < 0)
+goto cleanup;
+
+if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
+goto cleanup;
+
+if (!virDomainObjIsActive(vm)) {
+virReportError(VIR_ERR_OPERATION_INVALID,
+   "%s", _("domain is not running"));
+goto endjob;
+}
+
+ret = qemuDomainSnapshotFSFreeze(driver, vm);
+
+ endjob:
+if (!qemuDomainObjEndJob(driver, vm))
+vm = NULL;
+
+ cleanup:
+if (vm)
+virObjectUnlock(vm);
+return ret;
+}
+
+
+static int
+qemuDomainFSThaw(virDomainPtr dom,
+ const char **disks,
+ unsigned int ndisks,
+ unsigned int flags)
+{
+virQEMUDriverPtr driver = dom->conn->privateData;
+virDomainObjPtr vm;
+int ret = -1;
+
+virCheckFlags(0, -1);
+
+if (disks || ndisks) {
+virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
+   _("Specifying disks is not supported for now"));
+return -1;
+}
+
+if (!(vm = qemuDomObjFromDomain(dom)))
+goto cleanup;
+
+if (virDomainFSThawEnsureACL(dom->conn, vm->def) < 0)
+goto cleanup;
+
+if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
+goto cleanup;
+
+if (!virDomainObjIsActive(vm)) {
+virReportError(VIR_ERR_OPERATION_INVALID,
+   "%s", _("domain is not running"));
+goto endjob;
+}
+
+ret = qemuDomainSnapshotFSThaw(driver, vm, true);
+
+ endjob:
+if (!qemuDomainObjEndJob(driver, vm))
+vm = NULL;
+
+ cleanup:
+if (vm)
+virObjectUnlock(vm);
+return ret;
+}
+
+
 static virDriver qemuDriver = {
 .no = VIR_DRV_QEMU,
 .name = QEMU_DRIVER_NAME,
@@ -16890,6 +16982,8 @@ static virDriver qemuDriver = {
 .domainMigrateFinish3Params = qemuDomainMigrateFinish3Params, /* 1.1.0 */
 .domainMigrateConfirm3Params = qemuDomainMigrateConfirm3Params, /* 1.1.0 */
 .connectGetCPUModelNames = qemuConnectGetCPUModelNames, /* 1.1.3 */
+.domainFSFreeze = qemuDomainFSFreeze, /* 1.2.4 */
+.domainFSThaw = qemuDomainFSThaw, /* 1.2.4 */
 };
 
 

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


Re: [libvirt] [PATCH v3 0/2] bhyve: add console support through nmdm device

2014-04-03 Thread Michal Privoznik

On 30.03.2014 15:19, Roman Bogorodskiy wrote:

Changes from v2:
  - Add unittest
  - Minor: new labels style and target for 1.2.4

Changes from v1:
  Switch over from implicit slave device path, i.e. guessing
  from  that the slave is '/dev/nmdm0B', to
  explicit master and slave device specification using
  .

Roman Bogorodskiy (2):
   bhyve: add console support through nmdm device
   bhyve: add xml2argv tests for console

  src/bhyve/bhyve_command.c  | 34 +
  src/bhyve/bhyve_driver.c   | 45 +
  src/conf/domain_audit.c|  1 +
  src/conf/domain_conf.c | 59 +-
  src/conf/domain_conf.h |  5 ++
  src/qemu/qemu_command.c|  1 +
  src/qemu/qemu_monitor_json.c   |  1 +
  tests/bhyvexml2argvdata/bhyvexml2argv-console.args |  4 ++
  tests/bhyvexml2argvdata/bhyvexml2argv-console.xml  | 23 +
  tests/bhyvexml2argvdata/bhyvexml2argv-serial.args  |  4 ++
  tests/bhyvexml2argvdata/bhyvexml2argv-serial.xml   | 23 +
  tests/bhyvexml2argvtest.c  |  2 +
  12 files changed, 201 insertions(+), 1 deletion(-)
  create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-console.args
  create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-console.xml
  create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-serial.args
  create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-serial.xml



ACK to both patches.

Michal

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


[libvirt] [PATCH v2] Add V6LOCAL nwfilter parameter.

2014-04-03 Thread Brian Rak
Currently, adding any sort of IPv6 nwfilter rules is rather difficult.
There are no standard rules, and you end up doing a lot of things by
hand.  This patch makes the $V6LOCAL variable available within nwfilter
nules.  This is the generated from the interface's mac address using
the modified EUI-64 format, which matches what the guest should be
using.

This is part of what information is needed to correctly filter guest
IPv6 traffic.  Since this changes with the MAC address, it is
significantly easier if libvirt populates it (rather then requiring the
user to enter it).

As an example, an interface with a MAC address of "12:34:45:46:23:41"
would have a V6LOCAL value of "fe80::1034:45ff:fe46:2341"
---
 docs/formatnwfilter.html.in|   11 ---
 src/conf/nwfilter_params.h |1 +
 src/nwfilter/nwfilter_gentech_driver.c |   25 +
 3 files changed, 34 insertions(+), 3 deletions(-)

diff --git a/docs/formatnwfilter.html.in b/docs/formatnwfilter.html.in
index 45b97f7..4f10884 100644
--- a/docs/formatnwfilter.html.in
+++ b/docs/formatnwfilter.html.in
@@ -239,9 +239,9 @@
 Usage of variables in filters
 
 
-  Two variables names have so far been reserved for usage by the
-  network traffic filtering subsystem: MAC and
-  IP.
+  Three variables names have so far been reserved for usage by the
+  network traffic filtering subsystem: MAC,
+  IP, and V6LOCAL (since 1.2.4).
   
   MAC is the MAC address of the
   network interface. A filtering rule that references this variable
@@ -251,6 +251,11 @@
   parameter similar to the IP parameter above, it is discouraged
   since libvirt knows what MAC address an interface will be using.
   
+  V6LOCAL is the computed IPv6 link-local address.
+  This is based on the MAC address of the interface.  As an example,
+  a MAC address of 12:34:45:46:23:41 would result in a
+  link local address of fe80::1034:45ff:fe46:2341.
+  
   The parameter IP represents the IP address
   that the operating system inside the virtual machine is expected
   to use on the given interface. The IP parameter
diff --git a/src/conf/nwfilter_params.h b/src/conf/nwfilter_params.h
index 5e9777b..f61250f 100644
--- a/src/conf/nwfilter_params.h
+++ b/src/conf/nwfilter_params.h
@@ -98,6 +98,7 @@ bool virNWFilterHashTableEqual(virNWFilterHashTablePtr a,
 
 # define NWFILTER_VARNAME_IP "IP"
 # define NWFILTER_VARNAME_MAC "MAC"
+# define NWFILTER_VARNAME_V6LOCAL "V6LOCAL"
 # define NWFILTER_VARNAME_CTRL_IP_LEARNING "CTRL_IP_LEARNING"
 # define NWFILTER_VARNAME_DHCPSERVER "DHCPSERVER"
 
diff --git a/src/nwfilter/nwfilter_gentech_driver.c 
b/src/nwfilter/nwfilter_gentech_driver.c
index 1ce5e70..c58df1c 100644
--- a/src/nwfilter/nwfilter_gentech_driver.c
+++ b/src/nwfilter/nwfilter_gentech_driver.c
@@ -45,6 +45,7 @@ VIR_LOG_INIT("nwfilter.nwfilter_gentech_driver");
 
 #define NWFILTER_STD_VAR_MAC NWFILTER_VARNAME_MAC
 #define NWFILTER_STD_VAR_IP  NWFILTER_VARNAME_IP
+#define NWFILTER_STD_VAR_V6LOCAL  NWFILTER_VARNAME_V6LOCAL
 
 #define NWFILTER_DFLT_LEARN  "any"
 
@@ -163,6 +164,30 @@ virNWFilterVarHashmapAddStdValues(virNWFilterHashTablePtr 
table,
"%s", _("Could not add variable 'MAC' to hashmap"));
 return -1;
 }
+
+virMacAddr parsedMac;
+if (virMacAddrParse(macaddr, &parsedMac) == 0) {
+parsedMac.addr[0] ^= 2;
+
+char euiMacAddr[26];
+snprintf(euiMacAddr, sizeof(euiMacAddr),
+   "fe80::%02x%02x:%02xff:fe%02x:%02x%02x", parsedMac.addr[0],
+   parsedMac.addr[1], parsedMac.addr[2], parsedMac.addr[3],
+   parsedMac.addr[4], parsedMac.addr[5]);
+
+val = virNWFilterVarValueCreateSimpleCopyValue(euiMacAddr);
+if (!val)
+return -1;
+
+if (virHashAddEntry(table->hashTable,
+NWFILTER_STD_VAR_V6LOCAL,
+val) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   "%s", _("Could not add variable 'V6LOCAL' "
+   "to hashmap"));
+return -1;
+}
+}
 }
 
 if (ipaddr) {
-- 
1.7.1

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


Re: [libvirt] [PATCH 2/3] libxl: Set disk format for empty cdrom device

2014-04-03 Thread Michal Privoznik

On 27.03.2014 17:55, Stefan Bader wrote:

The XML config for a CDROM device can be without a source path,
indicating that there is no media present. Without this change
the libxl driver fails to start a guest in that case because
the libxl library checks for the LIBXL_DISK_FORMAT_EMPTY format
type and tries to stat the NULL pointer that gets passed on.


libxl: error: libxl_device.c:265:libxl__device_disk_set_backend:
Disk vdev=hdc failed to stat: (null): Bad address


Signed-off-by: Stefan Bader 
---
  src/libxl/libxl_conf.c |3 +++
  1 file changed, 3 insertions(+)

diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index de6f7ce..b8de72a 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -827,6 +827,9 @@ libxlMakeDisk(virDomainDiskDefPtr l_disk, libxl_device_disk 
*x_disk)
  x_disk->removable = 1;
  x_disk->readwrite = !l_disk->readonly;
  x_disk->is_cdrom = l_disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM ? 1 : 0;
+/* An empty CDROM must have the empty format, otherwise libxl fails. */
+if (x_disk->is_cdrom && !x_disk->pdev_path)
+x_disk->format = LIBXL_DISK_FORMAT_EMPTY;
  if (l_disk->transient) {
  virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
 _("libxenlight does not support transient disks"));



ACK

Michal

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


Re: [libvirt] [PATCH 3/3] libxl: Implement basic video device selection

2014-04-03 Thread Michal Privoznik

On 27.03.2014 17:55, Stefan Bader wrote:

This started as an investigation into an issue where libvirt (using the
libxl driver) and the Xen host, like an old couple, could not agree on
who is responsible for selecting the VNC port to use.

Things usually (and a bit surprisingly) did work because, just like that
old couple, they had the same idea on what to do by default. However it
was possible that this ended up in a big argument.

The problem is that display information exists in two different places:
in the vfbs list and in the build info. And for launching the device model,
only the latter is used. But that never gets initialized from libvirt. So
Xen allows the device model to select a default port while libvirt thinks
it has told Xen that this is done by libvirt (though the vfbs config).

While fixing that, I made a stab at actually evaluating the configuration
of the video device. So that it is now possible to at least decide between
a Cirrus or standard VGA emulation and to modify the VRAM within certain
limits using libvirt.

Signed-off-by: Stefan Bader 
---
  src/libxl/libxl_conf.c |   85 
  1 file changed, 85 insertions(+)

diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index b8de72a..fdd2726 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -1298,6 +1298,82 @@ libxlMakeCapabilities(libxl_ctx *ctx)
  return NULL;
  }

+static void
+libxlSetBuildGraphics(virDomainDefPtr def, libxl_domain_config *d_config)
+{
+libxl_domain_build_info *b_info = &d_config->b_info;
+
+/*
+ * Take the first defined video device (graphics card) to display
+ * on the first graphics device (display).
+ * Right now only type and vram info is used and anything beside
+ * type xen and vga is mapped to cirrus.
+ */
+if (def->nvideos) {
+unsigned int min_vram = 8 * 1024;
+
+switch (def->videos[0]->type) {
+case VIR_DOMAIN_VIDEO_TYPE_VGA:
+case VIR_DOMAIN_VIDEO_TYPE_XEN:
+b_info->u.hvm.vga.kind = LIBXL_VGA_INTERFACE_TYPE_STD;
+/*
+ * Libxl enforces a minimal VRAM size of 8M when using
+ * LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL or
+ * 16M for LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN.
+ * Avoid build failures and go with the minimum if less
+ * is specified.
+ */
+switch (b_info->device_model_version) {
+case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL:
+min_vram = 8 * 1024;
+break;
+case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
+default:
+min_vram = 16 * 1024;
+}
+break;
+default:
+/*
+ * Ignore any other device type and use Cirrus. Again fix
+ * up the minimal VRAM to what libxl expects.
+ */
+b_info->u.hvm.vga.kind = LIBXL_VGA_INTERFACE_TYPE_CIRRUS;
+switch (b_info->device_model_version) {
+case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL:
+min_vram = 4 * 1024; /* Actually the max, too */
+break;
+case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
+default:
+min_vram = 8 * 1024;
+}
+}
+b_info->video_memkb = (def->videos[0]->vram > min_vram) ?
+   def->videos[0]->vram :
+   LIBXL_MEMKB_DEFAULT;
+} else {
+libxl_defbool_set(&b_info->u.hvm.nographic, 1);
+}
+
+/*
+ * When making the list of displays, only VNC and SDL types were
+ * taken into account. So it seems sensible to connect the default
+ * video device to the first in the vfb list.
+ *
+ * FIXME: Copy the structures and fixing the strings feels a bit dirty.
+ */
+if (d_config->num_vfbs) {
+libxl_device_vfb *vfb0 = &d_config->vfbs[0];
+
+   b_info->u.hvm.vnc = vfb0->vnc;
+VIR_STRDUP(b_info->u.hvm.vnc.listen, vfb0->vnc.listen);
+VIR_STRDUP(b_info->u.hvm.vnc.passwd, vfb0->vnc.passwd);
+b_info->u.hvm.sdl = vfb0->sdl;
+VIR_STRDUP(b_info->u.hvm.sdl.display, vfb0->sdl.display);
+VIR_STRDUP(b_info->u.hvm.sdl.xauthority, vfb0->sdl.xauthority);
+VIR_STRDUP(b_info->u.hvm.keymap, vfb0->keymap);


You need to check for the return value of VIR_STRDUP(). Moreover, the 
indentation seems off. Looking forward to v2.


Michal

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


Re: [libvirt] [PATCH 1/3] libxl: Use id from virDomainObj inside the driver

2014-04-03 Thread Michal Privoznik

On 27.03.2014 17:55, Stefan Bader wrote:

There is a domain id in the virDomain structure as well as in the
virDomainObj structure. While the former can become stale the latter
is kept up to date. So it is safer to always (virDomainObjPtr)->def->id
internally.

This will fix issues seen when managing Xen guests through libvirt from
virt-manager (not being able to get domain info after define or reboot).
This was caused both though libxlDomainGetInfo() only but there were
a lot of places that might potentially cause issues, too.

Signed-off-by: Stefan Bader 
---
  src/libxl/libxl_driver.c |   75 +++---
  1 file changed, 38 insertions(+), 37 deletions(-)


Oh, you've sent the patch again. This time as a part of bigger set. 
Anyway, my ACK stands.


Michal

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


Re: [libvirt] beginner project idea: enum scrubbing [was: Fwd: Start contributing with Libvirt (Finding a mentor or helpful tips)]

2014-04-03 Thread Julio Faracco
It's a good suggestion to start.
I will do that, Eric. =)

--
*Julio Cesar Faracco*
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] cpu: Add documentation for CPU driver APIs

2014-04-03 Thread Michal Privoznik

On 26.03.2014 16:08, Jiri Denemark wrote:

Signed-off-by: Jiri Denemark 
---
  src/cpu/cpu.c | 202 ++
  1 file changed, 202 insertions(+)


ACK

Michal

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


Re: [libvirt] [PATCH] libxl: Use id from virDomainObj inside the driver

2014-04-03 Thread Michal Privoznik

On 25.03.2014 18:39, Stefan Bader wrote:

There is a domain id in the virDomain structure as well as in the
virDomainObj structure. While the former can become stale the latter
is kept up to date. So it is safer to always (virDomainObjPtr)->def->id
internally.

This will fix issues seen when managing Xen guests through libvirt from
virt-manager (not being able to get domain info after define or reboot).
This was caused both though libxlDomainGetInfo() only but there were
a lot of places that might potentially cause issues, too.

Signed-off-by: Stefan Bader 
---
  src/libxl/libxl_driver.c |   75 +++---
  1 file changed, 38 insertions(+), 37 deletions(-)


ACK

Michal

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


Re: [libvirt] [PATCH] phyp: fix logic error on volume creation

2014-04-03 Thread Michal Privoznik

On 02.04.2014 01:39, Eric Blake wrote:

The phyp code claims that it wants a non-zero value, but actually
enforces a capacity of zero.  It has been this way since commit
ebc46fe in June 2010.  Bummer that it has my name as the committer
- I guess I should have been much more stubborn about not blindly
taking someone else's 1600-line patch.

* src/phyp/phyp_driver.c (phypStorageVolCreateXML): Use correct
logic.

Signed-off-by: Eric Blake 
---

The fact that this bug has gone unnoticed for years makes me
wonder if we are better off just removing the phyp driver from
our code base, since it is obvious it is not getting much
testing.  I'm also waiting for a review on this, because although
I _think_ the code wanted a non-zero capacity, I don't know
enough about phyp and the "viosvrcmd -c 'mklv -lv'" command line;
maybe the comments are wrong and it always wanted 0 capacity
instead (which is the only thing that would get past the
pre-patch code check).


C'mon. Who's really creating volumes via libvirt? I mean, the fact that 
users don't create volumes via libvirt doesn't mean they are not using 
phyp driver. I probably should not say this aloud, but I used volume 
creation only when starting with libvirt. I used to play with 
virt-manager to get sense of libvirt's capabilities. I admit that things 
are different now sice we have domains and storage pools tied together, 
but not for me as I'm already used to doing it the other way.


Michal

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


Re: [libvirt] [PATCH] Add redirdevs to ABI stability check

2014-04-03 Thread Michal Privoznik

On 02.04.2014 14:03, Ján Tomko wrote:

Check the bus, type of the source device (tcp vs. spicevmc)
and the device address visible in the guest.

https://bugzilla.redhat.com/show_bug.cgi?id=1035128
---
  src/conf/domain_conf.c | 50 ++
  1 file changed, 50 insertions(+)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 0af5be7..9f1c020 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -13693,6 +13693,42 @@ virDomainHubDefCheckABIStability(virDomainHubDefPtr 
src,
  return true;
  }

+
+static bool
+virDomainRedirdevDefCheckABIStability(virDomainRedirdevDefPtr src,
+  virDomainRedirdevDefPtr dst)
+{
+if (src->bus != dst->bus) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("Target redirected device bus %s does not match "
+ "source %s"),
+   virDomainRedirdevBusTypeToString(dst->bus),
+   virDomainRedirdevBusTypeToString(src->bus));
+return false;
+}
+
+switch ((enum virDomainRedirdevBus) src->bus) {
+case VIR_DOMAIN_REDIRDEV_BUS_USB:
+if (src->source.chr.type != dst->source.chr.type) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("Target redirected device source type %s does "
+ "not match source device source type %s"),
+   virDomainChrTypeToString(dst->source.chr.type),
+   virDomainChrTypeToString(src->source.chr.type));
+return false;
+}
+break;
+case VIR_DOMAIN_REDIRDEV_BUS_LAST:
+break;
+}
+
+if (!virDomainDeviceInfoCheckABIStability(&src->info, &dst->info))
+return false;
+
+return true;
+}
+
+
  static bool
  virDomainRedirFilterDefCheckABIStability(virDomainRedirFilterDefPtr src,
   virDomainRedirFilterDefPtr dst)
@@ -14133,6 +14169,20 @@ virDomainDefCheckABIStability(virDomainDefPtr src,
  if (!virDomainHubDefCheckABIStability(src->hubs[i], dst->hubs[i]))
  goto error;

+if (src->nredirdevs != dst->nredirdevs) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("Target domain redirected devices count %zu "
+ "does not match source %zu"),
+   dst->nconsoles, src->nconsoles);
+goto error;
+}
+
+for (i = 0; i < src->nredirdevs; i++) {
+if (!virDomainRedirdevDefCheckABIStability(src->redirdevs[i],
+   dst->redirdevs[i]))
+goto error;
+}
+
  if ((!src->redirfilter && dst->redirfilter) ||
  (src->redirfilter && !dst->redirfilter)) {
  virReportError(VIR_ERR_CONFIG_UNSUPPORTED,



ACK

Michal

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


Re: [libvirt] [PATCH] Define CPUINFO_FILE_LEN and fix maxlen of cpuinfo file for powerpc

2014-04-03 Thread Eric Blake
On 04/03/2014 12:43 AM, Olivia Yin wrote:
> This patch define CPUINFO_FILE_LEN as 2KB which is enough for most 
> architectures.
> 
> For the 24 cores PowerPC machines, the file of /proc/cpuinfo will be larger 
> than 2KB.
> It will fail to start libvirtd with the error message as below:
> virFileReadAll: Failed to read file '/proc/cpuinfo': Value too large for 
> defined data type
> virSysinfoRead: internal error Failed to open /proc/cpuinfo
> 
> So this patch increases the maxlen of cpuinfo for PowerPC architecture.
> 
> Signed-off-by: Olivia Yin 
> ---
>  src/util/virsysinfo.c | 6 +++---
>  src/util/virsysinfo.h | 2 ++
>  2 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/src/util/virsysinfo.c b/src/util/virsysinfo.c
> index 7b16157..33c4bc6 100644
> --- a/src/util/virsysinfo.c
> +++ b/src/util/virsysinfo.c
> @@ -223,7 +223,7 @@ virSysinfoRead(void)
>  if (VIR_ALLOC(ret) < 0)
>  goto no_memory;
>  
> -if (virFileReadAll(CPUINFO, 2048, &outbuf) < 0) {
> +if (virFileReadAll(CPUINFO, 2 * CPUINFO_FILE_LEN, &outbuf) < 0) {

Uggh.  Having to multiply the constant means you defined the constant
wrong.  Just make CPUINFO_FILE_LEN big enough.  Even as large as 1M, for
ALL uses, is not a burden.

> +++ b/src/util/virsysinfo.h
> @@ -102,4 +102,6 @@ bool virSysinfoIsEqual(virSysinfoDefPtr src,
>  
>  VIR_ENUM_DECL(virSysinfo)
>  
> +#define CPUINFO_FILE_LEN 2048/*2KB limit for normal cpuinfo file*/
> +
>  #endif /* __VIR_SYSINFOS_H__ */
> 

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



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

[libvirt] [PATCH v4] Introduce --without-pm-utils to get rid of pm-is-supported dependency

2014-04-03 Thread Cédric Bosdonnat
This uses the dbus api of systemd to check the power management
capabilities of the node.
---
 Diff with v3:
   * Added unit tests vir virSystemdCan* helpers
   * Make the default for with-pm-utils depending on dbus and systemd detection
   * Changed the implementation of the helpers to return true for
 'yes' and 'challenge' responses, we shouldn't get a 'no' given libvirtd 
runs
 as privileged user... but who knows.

 configure.ac  |  24 +++
 libvirt.spec.in   |   9 +
 src/libvirt_private.syms  |   3 ++
 src/util/virnodesuspend.c |  75 ++
 src/util/virsystemd.c |  59 +++
 src/util/virsystemd.h |   6 +++
 tests/virsystemdmock.c|  22 ++
 tests/virsystemdtest.c| 100 +-
 8 files changed, 281 insertions(+), 17 deletions(-)

diff --git a/configure.ac b/configure.ac
index 73efffa..34e5ec2 100644
--- a/configure.ac
+++ b/configure.ac
@@ -563,6 +563,10 @@ AC_ARG_WITH([chrdev-lock-files],
 [location for UUCP style lock files for character devices
  (use auto for default paths on some platforms) @<:@default=auto@:>@])])
 m4_divert_text([DEFAULTS], [with_chrdev_lock_files=auto])
+AC_ARG_WITH([pm-utils],
+  [AS_HELP_STRING([--with-pm-utils],
+[use pm-utils for power management @<:@default=yes@:>@])])
+m4_divert_text([DEFAULTS], [with_pm_utils=check])
 
 dnl
 dnl in case someone want to build static binaries
@@ -1621,6 +1625,25 @@ fi
 
 AM_CONDITIONAL([WITH_PHYP],[test "$with_phyp" = "yes"])
 
+dnl
+dnl Should we build with pm-utils support?
+dnl
+set -x
+if test "$with_pm_utils" = "check"; then
+with_pm_utils=yes
+if test "$with_dbus" = "yes"; then
+if test "$init_systemd" = "yes"; then
+with_pm_utils=no
+fi
+fi
+fi
+
+if test "$with_pm_utils" = "yes"; then
+AC_DEFINE_UNQUOTED([WITH_PM_UTILS], 1, [whether to use pm-utils])
+fi
+AM_CONDITIONAL([WITH_PM_UTILS], [test "$with_pm_utils" = "yes"])
+set +x
+
 dnl virsh libraries
 VIRSH_LIBS="$VIRSH_LIBS $READLINE_LIBS"
 AC_SUBST([VIRSH_LIBS])
@@ -2845,6 +2868,7 @@ AC_MSG_NOTICE([ rbd: $LIBRBD_LIBS])
 else
 AC_MSG_NOTICE([ rbd: no])
 fi
+AC_MSG_NOTICE([pm-utils: $with_pm_utils])
 
 AC_MSG_NOTICE([])
 AC_MSG_NOTICE([Test suite])
diff --git a/libvirt.spec.in b/libvirt.spec.in
index eab9b23..5c20955 100644
--- a/libvirt.spec.in
+++ b/libvirt.spec.in
@@ -132,6 +132,7 @@
 %define with_libssh2   0%{!?_without_libssh2:0}
 %define with_wireshark 0%{!?_without_wireshark:0}
 %define with_systemd_daemon 0%{!?_without_systemd_daemon:0}
+%define with_pm_utils  1
 
 # Non-server/HV driver defaults which are always enabled
 %define with_sasl  0%{!?_without_sasl:1}
@@ -182,6 +183,7 @@
 %if 0%{?fedora} >= 17 || 0%{?rhel} >= 7
 %define with_systemd 1
 %define with_systemd_daemon 1
+%define with_pm_utils 0
 %endif
 
 # Fedora 18 / RHEL-7 are first where firewalld support is enabled
@@ -1138,8 +1140,10 @@ Requires: nc
 Requires: gettext
 # Needed by virt-pki-validate script.
 Requires: gnutls-utils
+%if %{with_pm_utils}
 # Needed for probing the power management features of the host.
 Requires: pm-utils
+%{endif}
 %if %{with_sasl}
 Requires: cyrus-sasl
 # Not technically required, but makes 'out-of-box' config
@@ -1395,6 +1399,10 @@ driver
 %define _without_systemd_daemon --without-systemd-daemon
 %endif
 
+%if ! %{with_pm_utils}
+%define _without_pm_utils --without-pm-utils
+%endif
+
 %define when  %(date +"%%F-%%T")
 %define where %(hostname)
 %define who   %{?packager}%{!?packager:Unknown}
@@ -1471,6 +1479,7 @@ rm -f po/stamp-po
%{?_with_firewalld} \
%{?_without_wireshark} \
%{?_without_systemd_daemon} \
+   %{?_without_pm_utils} \
%{with_packager} \
%{with_packager_version} \
--with-qemu-user=%{qemu_user} \
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 38fbf63..ce51bdf 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1879,6 +1879,9 @@ virSysinfoSetup;
 
 
 # util/virsystemd.h
+virSystemdCanHibernate;
+virSystemdCanHybridSleep;
+virSystemdCanSuspend;
 virSystemdCreateMachine;
 virSystemdMakeMachineName;
 virSystemdMakeScopeName;
diff --git a/src/util/virnodesuspend.c b/src/util/virnodesuspend.c
index 8088931..b1eddff 100644
--- a/src/util/virnodesuspend.c
+++ b/src/util/virnodesuspend.c
@@ -22,6 +22,7 @@
 #include 
 #include "virnodesuspend.h"
 
+# include "virsystemd.h"
 #include "vircommand.h"
 #include "virthread.h"
 #include "datatypes.h"
@@ -245,23 +246,9 @@ int nodeSuspendForDuration(unsigned int target,
 return ret;
 }
 
-
-/**
- * virNodeSuspendSupportsTarget:
- * @target: The power management target to check whether it is supported
- *   by the host. Values could be:
- *   VIR_NODE_SUSPEND_TARGET_MEM
- *   VIR_NODE_SUSPEND_TARGET_DISK
- *   VIR_NODE_SUSPEND_T

Re: [libvirt] [PATCH] Use the force flag for mkfs -t xfs

2014-04-03 Thread Michal Privoznik

On 03.04.2014 12:23, Ján Tomko wrote:

Without this, building an XFS pool on a formatted partition
fails with --overwrite.

https://bugzilla.redhat.com/show_bug.cgi?id=927172
---
  src/storage/storage_backend_fs.c | 12 +++-
  1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c
index e0244ba..1d85871 100644
--- a/src/storage/storage_backend_fs.c
+++ b/src/storage/storage_backend_fs.c
@@ -665,11 +665,13 @@ virStorageBackendExecuteMKFS(const char *device,
  int ret = 0;
  virCommandPtr cmd = NULL;

-cmd = virCommandNewArgList(MKFS,
-   "-t",
-   format,
-   device,
-   NULL);
+cmd = virCommandNewArgList(MKFS, "-t", format, NULL);
+
+/* use the force, otherwise mkfs.xfs won't overwrite existing fs */
+if (STREQ(format, "xfs"))
+virCommandAddArg(cmd, "-f");
+
+virCommandAddArg(cmd, device);

  if (virCommandRun(cmd, NULL) < 0) {
  virReportSystemError(errno,



ACK

Michal

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


[libvirt] [PATCH] Use the force flag for mkfs -t xfs

2014-04-03 Thread Ján Tomko
Without this, building an XFS pool on a formatted partition
fails with --overwrite.

https://bugzilla.redhat.com/show_bug.cgi?id=927172
---
 src/storage/storage_backend_fs.c | 12 +++-
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c
index e0244ba..1d85871 100644
--- a/src/storage/storage_backend_fs.c
+++ b/src/storage/storage_backend_fs.c
@@ -665,11 +665,13 @@ virStorageBackendExecuteMKFS(const char *device,
 int ret = 0;
 virCommandPtr cmd = NULL;
 
-cmd = virCommandNewArgList(MKFS,
-   "-t",
-   format,
-   device,
-   NULL);
+cmd = virCommandNewArgList(MKFS, "-t", format, NULL);
+
+/* use the force, otherwise mkfs.xfs won't overwrite existing fs */
+if (STREQ(format, "xfs"))
+virCommandAddArg(cmd, "-f");
+
+virCommandAddArg(cmd, device);
 
 if (virCommandRun(cmd, NULL) < 0) {
 virReportSystemError(errno,
-- 
1.8.3.2

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


Re: [libvirt] [PATCH] virsh: Make 'exit' action same as 'quit'

2014-04-03 Thread Michal Privoznik

On 02.04.2014 10:43, liyang wrote:

From: Li Yang 

For now 'virsh quit' action like this:

[root@localhost /]# virsh quit
[root@localhost /]#

And 'virsh exit' action:

[root@localhost /]# virsh exit

[root@localhost /]#

There is a small difference('/n') between them.
According to manual said:
quit, exit
quit this interactive terminal

And in the code they all called cmdQuit func,
They should get same actions.

Signed-off-by: Li Yang 
---
  tools/virsh.c |3 ++-
  1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/tools/virsh.c b/tools/virsh.c
index 02835d9..da7fedd 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -1852,7 +1852,8 @@ vshCommandRun(vshControl *ctl, const vshCmd *cmd)
  if (!ret && disconnected != 0)
  vshReconnect(ctl);

-if (STREQ(cmd->def->name, "quit"))/* hack ... */
+if (STREQ(cmd->def->name, "quit") ||
+STREQ(cmd->def->name, "exit"))/* hack ... */
  return ret;

  if (enable_timing) {



ACKed and pushed. Congratulations on your second libvirt contribution!

Michal

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


Re: [libvirt] [PATCH] qemu: make sure agent returns error when required data are missing

2014-04-03 Thread Martin Kletzander

On Thu, Apr 03, 2014 at 09:32:02AM +0200, Peter Krempa wrote:

On 04/03/14 07:58, Martin Kletzander wrote:

Commit 5b3492fa aimed to fix this and caught one error but exposed
another one.  When agent command is being executed and the thread
waiting for the reply is woken up by an event (e.g. EOF in case of
shutdown), the command finishes with no data (rxObject == NULL), but
no error is reported, since this might be desired by the caller
(e.g. suspend through agent).  However, in other situations, when the
data are required (e.g. getting vCPUs), we proceed to getting desired
data out of the reply, but none of the virJSON*() functions works well
with NULLs.  I chose the way of a new parameter for qemuAgentCommand()
function that specifies whether reply is required and behaves
according to that.

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

Signed-off-by: Martin Kletzander 
---

Notes:
This issue probably exists since qemu-ga is supported in libvirt, so
this (along with 5b3492fa and e9d09fe1) might be worth back-porting to
some maintenance branches, I just haven't gone through them to see
which ones are applicable.

 src/qemu/qemu_agent.c | 19 ++-
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
index 92573bd..4082331 100644
--- a/src/qemu/qemu_agent.c
+++ b/src/qemu/qemu_agent.c
@@ -1080,6 +1080,7 @@ static int
 qemuAgentCommand(qemuAgentPtr mon,
  virJSONValuePtr cmd,
  virJSONValuePtr *reply,
+ bool needReply,
  int seconds)
 {


Seeing that the "needReply" parameter is true in most cases, I'd rename
this function and add one not requiring the new parameter for normal
usage and use the renamed in the few cases that actually expect the VM
to quit.



I don't think it's worth adding this much of unnecessary code just to
eliminate one argument in 4 out of 7 call, especially when all the
functions are private and even static, therefore not much of a cleanup
from the readability POV.


But this is just a cosmetic issue.



Patches are welcome, though ;)


ACK



Thanks, pushed


Peter



Martin


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

Re: [libvirt] [PATCH 2/3] bhyve: create capabilities submodule

2014-04-03 Thread Michal Privoznik

On 02.04.2014 10:30, Wojciech Macek wrote:

Adding one minor change to copyrights header...



- Move all capabilities functions to separate file
- Add initCPU
---
  src/Makefile.am|   2 +
  src/bhyve/bhyve_capabilities.c | 105 ++
+++
  src/bhyve/bhyve_capabilities.h |  30 
  src/bhyve/bhyve_driver.c   |  56 +-
  4 files changed, 171 insertions(+), 22 deletions(-)
  create mode 100644 src/bhyve/bhyve_capabilities.c
  create mode 100644 src/bhyve/bhyve_capabilities.h


This is not the best way to send patch as reply. I see you've used git 
send-email to send the previous patches. If you want to send a v2 just 
to a specific one (say 2/3), you should use git send-email too; just 
specify in-reply-to and adjust subject prefix: PATCHv2: 
Moreover, sending patches other way than git send-email is tricky as you 
need to make sure the lines are not wrapped (yes, it happened in this 
case too).


Can you resend the v2? It's okay to send v2 just to a single patch.

Michal

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


[libvirt] [PATCHv3 3/3] qemu: Implement virDomain{Get,Set}Time

2014-04-03 Thread Michal Privoznik
One caveat though, qemu-ga is expecting time and returning time
in nanoseconds. With all the buffering and propagation delay, the
time is already wrong once it gets to the qemu-ga, but there's
nothing we can do about it.

Signed-off-by: Michal Privoznik 
---
 src/qemu/qemu_agent.c  |  99 
 src/qemu/qemu_agent.h  |   8 
 src/qemu/qemu_driver.c | 109 +
 3 files changed, 216 insertions(+)

diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
index 92573bd..4a8fa87 100644
--- a/src/qemu/qemu_agent.c
+++ b/src/qemu/qemu_agent.c
@@ -1645,3 +1645,102 @@ qemuAgentUpdateCPUInfo(unsigned int nvcpus,
 
 return 0;
 }
+
+
+int
+qemuAgentGetTime(qemuAgentPtr mon,
+ long long *seconds,
+ unsigned int *nseconds)
+{
+int ret = -1;
+unsigned long long json_time;
+virJSONValuePtr cmd;
+virJSONValuePtr reply = NULL;
+
+cmd = qemuAgentMakeCommand("guest-get-time",
+   NULL);
+if (!cmd)
+return ret;
+
+if (qemuAgentCommand(mon, cmd, &reply,
+ VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK) < 0)
+goto cleanup;
+
+if (!reply || qemuAgentCheckError(cmd, reply) < 0)
+goto cleanup;
+
+if (virJSONValueObjectGetNumberUlong(reply, "return", &json_time) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("malformed return value"));
+goto cleanup;
+}
+
+/* guest agent returns time in nanoseconds,
+ * we need it in seconds here */
+*seconds = json_time / 10LL;
+*nseconds = json_time % 10LL;
+ret = 0;
+
+ cleanup:
+virJSONValueFree(cmd);
+virJSONValueFree(reply);
+return ret;
+}
+
+
+/**
+ * qemuAgentSetTime:
+ * @setTime: time to set
+ * @sync: let guest agent to read domain's RTC (@setTime is ignored)
+ */
+int
+qemuAgentSetTime(qemuAgentPtr mon,
+long long seconds,
+unsigned int nseconds,
+bool sync)
+{
+int ret = -1;
+virJSONValuePtr cmd;
+virJSONValuePtr reply = NULL;
+
+if (sync) {
+cmd = qemuAgentMakeCommand("guest-set-time", NULL);
+} else {
+/* guest agent expect time with nanosecond granularity.
+ * Impressing. */
+long long json_time;
+
+/* Check if we overflow. For some reason qemu doesn't handle unsigned
+ * long long on the monitor well as it silently truncates numbers to
+ * signed long long. Therefore we must check overflow against LLONG_MAX
+ * not ULLONG_MAX. */
+if (seconds > LLONG_MAX / 10LL) {
+virReportError(VIR_ERR_INVALID_ARG,
+   _("Time '%lld' is too big for guest agent"),
+   seconds);
+return ret;
+}
+
+json_time = seconds * 10LL;
+json_time += nseconds;
+cmd = qemuAgentMakeCommand("guest-set-time",
+   "I:time", json_time,
+   NULL);
+}
+
+if (!cmd)
+return ret;
+
+if (qemuAgentCommand(mon, cmd, &reply,
+ VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK) < 0)
+goto cleanup;
+
+if (!reply || qemuAgentCheckError(cmd, reply) < 0)
+goto cleanup;
+
+ret = 0;
+ cleanup:
+virJSONValueFree(cmd);
+virJSONValueFree(reply);
+return ret;
+}
diff --git a/src/qemu/qemu_agent.h b/src/qemu/qemu_agent.h
index 5fbacdb..399b586 100644
--- a/src/qemu/qemu_agent.h
+++ b/src/qemu/qemu_agent.h
@@ -97,4 +97,12 @@ int qemuAgentSetVCPUs(qemuAgentPtr mon, qemuAgentCPUInfoPtr 
cpus, size_t ncpus);
 int qemuAgentUpdateCPUInfo(unsigned int nvcpus,
qemuAgentCPUInfoPtr cpuinfo,
int ncpuinfo);
+
+int qemuAgentGetTime(qemuAgentPtr mon,
+ long long *seconds,
+ unsigned int *nseconds);
+int qemuAgentSetTime(qemuAgentPtr mon,
+ long long seconds,
+ unsigned int nseconds,
+ bool sync);
 #endif /* __QEMU_AGENT_H__ */
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 1d08951..e661f28 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -16640,6 +16640,113 @@ qemuConnectGetCPUModelNames(virConnectPtr conn,
 return cpuGetModels(arch, models);
 }
 
+static int
+qemuDomainGetTime(virDomainPtr dom,
+  long long *seconds,
+  unsigned int *nseconds,
+  unsigned int flags)
+{
+virQEMUDriverPtr driver = dom->conn->privateData;
+virDomainObjPtr vm = NULL;
+qemuDomainObjPrivatePtr priv;
+int ret = -1;
+int rv;
+
+virCheckFlags(0, ret);
+
+if (!(vm = qemuDomObjFromDomain(dom)))
+return ret;
+
+if (virDomainGetTimeEnsureACL(dom->conn, vm->def) < 0)
+goto cleanup;
+
+   

[libvirt] [PATCHv3 0/3] Time setting and getting in qemu guests

2014-04-03 Thread Michal Privoznik
Hopefully the last round.

Michal Privoznik (3):
  Introduce virDomain{Get,Set}Time APIs
  virsh: Expose virDomain{Get,Set}Time
  qemu: Implement virDomain{Get,Set}Time

 daemon/remote.c  |  37 
 include/libvirt/libvirt.h.in |  14 +
 src/access/viraccessperm.c   |   2 +-
 src/access/viraccessperm.h   |   7 ++-
 src/driver.h |  14 +
 src/libvirt.c|  94 ++
 src/libvirt_public.syms  |   5 ++
 src/qemu/qemu_agent.c|  99 
 src/qemu/qemu_agent.h|   8 +++
 src/qemu/qemu_driver.c   | 109 +++
 src/remote/remote_driver.c   |  35 
 src/remote/remote_protocol.x |  31 +-
 src/remote_protocol-structs  |  16 ++
 tools/virsh-domain-monitor.c | 132 +++
 tools/virsh.pod  |  16 ++
 15 files changed, 616 insertions(+), 3 deletions(-)

-- 
1.9.0

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


[libvirt] [PATCHv3 2/3] virsh: Expose virDomain{Get,Set}Time

2014-04-03 Thread Michal Privoznik
These APIs are exposed under new virsh command 'domtime' which both gets
and sets (not at the same time of course :)).

Signed-off-by: Michal Privoznik 
---
 tools/virsh-domain-monitor.c | 132 +++
 tools/virsh.pod  |  16 ++
 2 files changed, 148 insertions(+)

diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c
index 18d551a..31fa8de 100644
--- a/tools/virsh-domain-monitor.c
+++ b/tools/virsh-domain-monitor.c
@@ -1356,6 +1356,132 @@ cmdDomstate(vshControl *ctl, const vshCmd *cmd)
 }
 
 /*
+ * "domtime" command
+ */
+static const vshCmdInfo info_domtime[] = {
+{.name = "help",
+ .data = N_("domain time")
+},
+{.name = "desc",
+ .data = N_("Gets or sets a domain time")
+},
+{.name = NULL}
+};
+
+static const vshCmdOptDef opts_domtime[] = {
+{.name = "domain",
+ .type = VSH_OT_DATA,
+ .flags = VSH_OFLAG_REQ,
+ .help = N_("domain name, id or uuid")
+},
+{.name = "now",
+ .type = VSH_OT_BOOL,
+ .help = N_("set current host time")
+},
+{.name = "pretty",
+ .type = VSH_OT_BOOL,
+ .help = N_("print domain's time in human readable form")
+},
+{.name = "sync",
+ .type = VSH_OT_BOOL,
+ .help = N_("instead of setting given time, synchronize from domain's 
RTC"),
+},
+{.name = "time",
+ .type = VSH_OT_INT,
+ .help = N_("time to set")
+},
+{.name = NULL}
+};
+
+static bool
+cmdDomTime(vshControl *ctl, const vshCmd *cmd)
+{
+virDomainPtr dom;
+bool ret = false;
+bool now = vshCommandOptBool(cmd, "now");
+bool pretty = vshCommandOptBool(cmd, "pretty");
+bool sync = vshCommandOptBool(cmd, "sync");
+long long seconds = 0;
+unsigned int nseconds = 0;
+unsigned int flags = 0;
+bool doSet = false;
+int rv;
+
+if (!(dom = vshCommandOptDomain(ctl, cmd, NULL)))
+return false;
+
+rv = vshCommandOptLongLong(cmd, "time", &seconds);
+
+if (rv < 0) {
+/* invalid integer format */
+vshError(ctl, "%s",
+ _("Unable to parse integer parameter to --time."));
+goto cleanup;
+} else if (rv > 0) {
+/* --time is used, so set time instead of get time.
+ * However, --time and --now are mutually exclusive. */
+if (now) {
+vshError(ctl, _("--time and --now are mutually exclusive"));
+goto cleanup;
+}
+
+/* Neither is --time and --sync */
+if (sync) {
+vshError(ctl, _("--time and --sync are mutually exclusive"));
+goto cleanup;
+
+}
+doSet = true;
+}
+
+if (sync && now) {
+vshError(ctl, _("--sync and --now are mutually exclusive"));
+goto cleanup;
+}
+
+/* --now or --sync means setting */
+doSet |= now | sync;
+
+if (doSet) {
+if (now && ((seconds = time(NULL)) == (time_t) -1)) {
+vshError(ctl, _("Unable to get current time"));
+goto cleanup;
+}
+
+if (sync)
+flags |= VIR_DOMAIN_TIME_SYNC;
+
+if (virDomainSetTime(dom, seconds, nseconds, flags) < 0)
+goto cleanup;
+
+} else {
+if (virDomainGetTime(dom, &seconds, &nseconds, flags) < 0)
+goto cleanup;
+
+if (pretty) {
+char timestr[100];
+time_t cur_time = seconds;
+struct tm time_info;
+
+if (!gmtime_r(&cur_time, &time_info)) {
+vshError(ctl, _("Unable to format time"));
+goto cleanup;
+}
+strftime(timestr, sizeof(timestr), "%Y-%m-%d %H:%M:%S", 
&time_info);
+
+vshPrint(ctl, _("Time: %s"), timestr);
+} else {
+vshPrint(ctl, _("Time: %lld"), seconds);
+}
+}
+
+ret = true;
+ cleanup:
+virDomainFree(dom);
+return ret;
+}
+
+/*
  * "list" command
  */
 static const vshCmdInfo info_list[] = {
@@ -1911,6 +2037,12 @@ const vshCmdDef domMonitoringCmds[] = {
  .info = info_domstate,
  .flags = 0
 },
+{.name = "domtime",
+ .handler = cmdDomTime,
+ .opts = opts_domtime,
+ .info = info_domtime,
+ .flags = 0
+},
 {.name = "list",
  .handler = cmdList,
  .opts = opts_list,
diff --git a/tools/virsh.pod b/tools/virsh.pod
index 98d891a..d2de8a6 100644
--- a/tools/virsh.pod
+++ b/tools/virsh.pod
@@ -994,6 +994,22 @@ Returns state of an interface to VMM used to control a 
domain.  For
 states other than "ok" or "error" the command also prints number of
 seconds elapsed since the control interface entered its current state.
 
+=item B I { [I<--now>] [I<--pretty>] [I<--sync>]
+[I<--time> B] }
+
+Gets or sets the domain's system time. When run without any arguments
+(but I), the current domain's system time is printed out. The
+I<--pretty> modifier can be used to print the time in more human
+readable form. When I<--time> B is specified, the domain's time is
+not get 

[libvirt] [PATCHv3 1/3] Introduce virDomain{Get,Set}Time APIs

2014-04-03 Thread Michal Privoznik
These APIs allow users to get or set time in a domain, which may come
handy if the domain has been resumed just recently and NTP is not
configured or hasn't kicked in yet and the guest is running
something time critical. In addition, NTP may refuse to re-set the clock
if the skew is too big.

In addition, new ACL attribute is introduced 'set_time'.

Signed-off-by: Michal Privoznik 
---
 daemon/remote.c  | 37 +
 include/libvirt/libvirt.h.in | 14 +++
 src/access/viraccessperm.c   |  2 +-
 src/access/viraccessperm.h   |  7 +++-
 src/driver.h | 14 +++
 src/libvirt.c| 94 
 src/libvirt_public.syms  |  5 +++
 src/remote/remote_driver.c   | 35 +
 src/remote/remote_protocol.x | 31 ++-
 src/remote_protocol-structs  | 16 
 10 files changed, 252 insertions(+), 3 deletions(-)

diff --git a/daemon/remote.c b/daemon/remote.c
index 8476961..34c96c9 100644
--- a/daemon/remote.c
+++ b/daemon/remote.c
@@ -6077,6 +6077,43 @@ 
qemuDispatchConnectDomainMonitorEventDeregister(virNetServerPtr server ATTRIBUTE
 return rv;
 }
 
+static int
+remoteDispatchDomainGetTime(virNetServerPtr server ATTRIBUTE_UNUSED,
+virNetServerClientPtr client,
+virNetMessagePtr msg ATTRIBUTE_UNUSED,
+virNetMessageErrorPtr rerr,
+remote_domain_get_time_args *args,
+remote_domain_get_time_ret *ret)
+{
+int rv = -1;
+virDomainPtr dom = NULL;
+long long seconds;
+unsigned int nseconds;
+struct daemonClientPrivate *priv =
+virNetServerClientGetPrivateData(client);
+
+if (!priv->conn) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open"));
+goto cleanup;
+}
+
+if (!(dom = get_nonnull_domain(priv->conn, args->dom)))
+goto cleanup;
+
+if (virDomainGetTime(dom, &seconds, &nseconds, args->flags) < 0)
+goto cleanup;
+
+ret->seconds = seconds;
+ret->nseconds = nseconds;
+rv = 0;
+
+ cleanup:
+if (rv < 0)
+virNetMessageSaveError(rerr);
+if (dom)
+virDomainFree(dom);
+return rv;
+}
 
 /*- Helpers. -*/
 
diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
index 930b7e8..988f00d 100644
--- a/include/libvirt/libvirt.h.in
+++ b/include/libvirt/libvirt.h.in
@@ -5277,6 +5277,20 @@ int virDomainFSTrim(virDomainPtr dom,
 unsigned long long minimum,
 unsigned int flags);
 
+int virDomainGetTime(virDomainPtr dom,
+ long long *seconds,
+ unsigned int *nseconds,
+ unsigned int flags);
+
+typedef enum {
+VIR_DOMAIN_TIME_SYNC = (1 << 0), /* Re-sync domain time from domain's RTC 
*/
+} virDomainSetTimeFlags;
+
+int virDomainSetTime(virDomainPtr dom,
+ long long seconds,
+ unsigned int nseconds,
+ unsigned int flags);
+
 /**
  * virSchedParameterType:
  *
diff --git a/src/access/viraccessperm.c b/src/access/viraccessperm.c
index d517c66..bbcb6c1 100644
--- a/src/access/viraccessperm.c
+++ b/src/access/viraccessperm.c
@@ -42,7 +42,7 @@ VIR_ENUM_IMPL(virAccessPermDomain,
   "init_control", "inject_nmi", "send_input", "send_signal", 
"fs_trim",
   "block_read", "block_write", "mem_read",
   "open_graphics", "open_device", "screenshot",
-  "open_namespace");
+  "open_namespace", "set_time");
 
 VIR_ENUM_IMPL(virAccessPermInterface,
   VIR_ACCESS_PERM_INTERFACE_LAST,
diff --git a/src/access/viraccessperm.h b/src/access/viraccessperm.h
index 6d14f05..6fa0f01 100644
--- a/src/access/viraccessperm.h
+++ b/src/access/viraccessperm.h
@@ -282,13 +282,18 @@ typedef enum {
  */
 VIR_ACCESS_PERM_DOMAIN_SCREENSHOT,/* Trigger a screen shot */
 
-
 /**
  * @desc: Open domain namespace
  * @message: Opening domain namespaces requires authorization
  */
 VIR_ACCESS_PERM_DOMAIN_OPEN_NAMESPACE,
 
+/**
+ * @desc: Write domain time
+ * @message: Setting the domain time requires authorization
+ */
+VIR_ACCESS_PERM_DOMAIN_SET_TIME,
+
 VIR_ACCESS_PERM_DOMAIN_LAST,
 } virAccessPermDomain;
 
diff --git a/src/driver.h b/src/driver.h
index e66fc7a..474d045 100644
--- a/src/driver.h
+++ b/src/driver.h
@@ -1084,6 +1084,18 @@ typedef int
   unsigned int flags);
 
 typedef int
+(*virDrvDomainGetTime)(virDomainPtr dom,
+   long long *seconds,
+   unsigned int *nseconds,
+   unsigned int flags);
+
+typedef int
+(*virDrvDomainSetTime)(virDomainPtr dom,
+   long long seconds,
+   unsigned int nseconds,
+   unsigned int flags);
+
+typedef int
 (*

Re: [libvirt] [PATCH] qemu: make sure agent returns error when required data are missing

2014-04-03 Thread Peter Krempa
On 04/03/14 07:58, Martin Kletzander wrote:
> Commit 5b3492fa aimed to fix this and caught one error but exposed
> another one.  When agent command is being executed and the thread
> waiting for the reply is woken up by an event (e.g. EOF in case of
> shutdown), the command finishes with no data (rxObject == NULL), but
> no error is reported, since this might be desired by the caller
> (e.g. suspend through agent).  However, in other situations, when the
> data are required (e.g. getting vCPUs), we proceed to getting desired
> data out of the reply, but none of the virJSON*() functions works well
> with NULLs.  I chose the way of a new parameter for qemuAgentCommand()
> function that specifies whether reply is required and behaves
> according to that.
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1058149
> 
> Signed-off-by: Martin Kletzander 
> ---
> 
> Notes:
> This issue probably exists since qemu-ga is supported in libvirt, so
> this (along with 5b3492fa and e9d09fe1) might be worth back-porting to
> some maintenance branches, I just haven't gone through them to see
> which ones are applicable.
> 
>  src/qemu/qemu_agent.c | 19 ++-
>  1 file changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
> index 92573bd..4082331 100644
> --- a/src/qemu/qemu_agent.c
> +++ b/src/qemu/qemu_agent.c
> @@ -1080,6 +1080,7 @@ static int
>  qemuAgentCommand(qemuAgentPtr mon,
>   virJSONValuePtr cmd,
>   virJSONValuePtr *reply,
> + bool needReply,
>   int seconds)
>  {

Seeing that the "needReply" parameter is true in most cases, I'd rename
this function and add one not requiring the new parameter for normal
usage and use the renamed in the few cases that actually expect the VM
to quit.

But this is just a cosmetic issue.


ACK

Peter




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

Re: [libvirt] [PATCH 2/2] Include PCI address in the error in virDomainNetFindIdx

2014-04-03 Thread Ján Tomko
On 04/02/2014 01:53 PM, Laine Stump wrote:
> On 04/01/2014 06:11 PM, Ján Tomko wrote:
>> When looking up a net device by a MAC and PCI address, it is possible
>> that we've got a match on the MAC address but failed to match the
>> PCI address.
>>
>> In that case, outputting just the MAC address can be confusing.
>>
>> Partially resolves:
>> https://bugzilla.redhat.com/show_bug.cgi?id=872028
>> ---
>>  src/conf/domain_conf.c | 17 ++---
>>  1 file changed, 14 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index 1624c7e..35defaf 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -10456,9 +10456,20 @@ virDomainNetFindIdx(virDomainDefPtr def, 
>> virDomainNetDefPtr net)
>>  }
>>  }
>>  if (matchidx < 0) {
>> -virReportError(VIR_ERR_OPERATION_FAILED,
>> -   _("no device matching mac address %s found"),
>> -   virMacAddrFormat(&net->mac, mac));
>> +if (PCIAddrSpecified) {
>> +virReportError(VIR_ERR_OPERATION_FAILED,
>> +   _("no device matching mac address %s found on "
>> + "%.4x:%.2x:%.2x.%.1x"),
>> +   virMacAddrFormat(&net->mac, mac),
>> +   net->info.addr.pci.domain,
>> +   net->info.addr.pci.bus,
>> +   net->info.addr.pci.slot,
>> +   net->info.addr.pci.function);
> 
> We really should make a virPCIAddrFormat() function and start using it...
> 
> That's a separate issue though. ACK to this patch.
> 

Thanks, I've pushed the series.

Jan




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