[libvirt] [PATCH] maint: update to latest gnulib

2016-01-04 Thread Eric Blake 
Required for the copyright year bump to keep 'make syntax-check'
happy, and also pulls in several portability fixes.

* .gnulib: Update to latest.
* bootstrap: Resync from upstream.
* gnulib/local/m4/ssize_t.m4.diff: Regenerate.

Signed-off-by: Eric Blake 
---

Pushing under the build-breaker rule.

 .gnulib | 2 +-
 bootstrap   | 2 +-
 gnulib/local/m4/ssize_t.m4.diff | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/.gnulib b/.gnulib
index f39477d..6cc32c6 16
--- a/.gnulib
+++ b/.gnulib
@@ -1 +1 @@
-Subproject commit f39477dba778e99392948dd3dd19ec0d46aee932
+Subproject commit 6cc32c63e80bc1a30c521b2f07f2b54909b59892
diff --git a/bootstrap b/bootstrap
index e72894c..294c0bc 100755
--- a/bootstrap
+++ b/bootstrap
@@ -4,7 +4,7 @@ scriptversion=2014-12-08.12; # UTC

 # Bootstrap this package from checked-out sources.

-# Copyright (C) 2003-2015 Free Software Foundation, Inc.
+# Copyright (C) 2003-2016 Free Software Foundation, Inc.

 # This program is free software: you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
diff --git a/gnulib/local/m4/ssize_t.m4.diff b/gnulib/local/m4/ssize_t.m4.diff
index 12cff12..c4863b9 100644
--- a/gnulib/local/m4/ssize_t.m4.diff
+++ b/gnulib/local/m4/ssize_t.m4.diff
@@ -5,7 +5,7 @@ index 209d64c..5ea72a1 100644
 @@ -1,4 +1,4 @@
 -# ssize_t.m4 serial 5 (gettext-0.18.2)
 +# ssize_t.m4 serial 6 (gettext-0.18.2)
- dnl Copyright (C) 2001-2003, 2006, 2010-2015 Free Software Foundation, Inc.
+ dnl Copyright (C) 2001-2003, 2006, 2010-2016 Free Software Foundation, Inc.
  dnl This file is free software; the Free Software Foundation
  dnl gives unlimited permission to copy and/or distribute it,
 @@ -17,7 +17,21 @@ AC_DEFUN([gt_TYPE_SSIZE_T],
-- 
2.4.3

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

Re: [libvirt] [PATCH] maint: update to latest gnulib

2016-01-04 Thread Eric Blake 
On 01/04/2016 01:47 PM, Eric Blake wrote:
> Required for the copyright year bump to keep 'make syntax-check'
> happy, and also pulls in several portability fixes.
> 
> * .gnulib: Update to latest.
> * bootstrap: Resync from upstream.
> * gnulib/local/m4/ssize_t.m4.diff: Regenerate.
> 
> Signed-off-by: Eric Blake 
> ---
> 
> Pushing under the build-breaker rule.
> 
>  .gnulib | 2 +-

For reference, this includes:

* .gnulib f39477d...6cc32c6 (193):
  > msvc-inval: fix problem with unset shell var
  > autoupdate
  > tests: for compare_(), use cmp -s where available
  > version-etc: new year
  > human: fix output buffer overrun by 1
  > autoupdate
  > maint: add missing ChangeLog entry for previous commit
  > maint: fix operator precedence in mbrtowc test
  > autoupdate
  > regexprops-generic: update from regex.h
  > strftime-tests: avoid false failure on OS X
  > update from texinfo
  > fts: ensure leaf optimization is used for NFS
  > fts: enable leaf optimization for XFS
  > non-recursive-gnulib-prefix-hack: preserve V_GPERF lines
  > intprops: comment fix
  > update from texinfo
  > intprops-test: work around GCC bug 68971
  > autoupdate
  > gnulib-tool: allow multiple --local-dir usage
  > fix freadptr to work with ungetc on all uClibc configs
  > update from texinfo
  > autoupdate
  > parse-datetime: relax license to LGPLv2+, for OSTree
  > update from texinfo
  > autoupdate
  > autoupdate
  > update from texinfo
  > xalloc-oversized: improve performance with GCC 5
  > intprops: new public macro EXPR_SIGNED
  > intprops: fix typo in clang port
  > test-timespec: fix typo in previous change
  > timespec-sub: fix overflow bug; add tests
  > intprops-test: suppress -Woverlength-strings
  > maint: add missing ChangeLog entry for previous commit
  > quotearg: add quotearg_n_style_colon()
  > intprops: revise _WRAPV macros, revert _OVERFLOW
  > intprops: add parentheses for when OP has precedence lower than "-"
  > quotearg: constify get_quoting_style parameters
  > quotearg: add support for $'' shell escaping
  > maint: use a more standard return from mbrtowc test
  > intprops: add WRAPV and const flavors for GCC 5
  > doc: use extended timezone format in iso-8601 example
  > update from texinfo
  > update from texinfo
  > update from texinfo
  > stdalign: port to Sun C 5.9
  > autoupdate
  > update from texinfo
  > autoupdate
  > time_rz: fix comment about tzalloc
  > update from texinfo
  > stdalign: work around pre-4.9 GCC x86 bug
  > maint.mk: sc_tight_scope: remove extraneous expressions
  > time_rz: return NULL if localtime_r fails
  > fts: port to C11 alignof
  > time_rz: avoid warning from bleeding-edge gcc's -Wnonnull
  > maint.mk: _gl_TS_function_match: fix "extern" name extracting regexp
  > maint.mk: sc_tight_scope: factor and support OS X
  > ChangeLog: fix typo: s/cound/count/
  > safe-alloc-tests: fix typo in license header
  > copy-file: fix mem leak in error case
  > localename: control langinfo.h inclusion
  > update from texinfo
  > binary-io, math, pthread, sys_socket, u64, unistd: port to strict C
  > accept4-tests: fix to avoid non portable flags
  > update from texinfo
  > update from texinfo
  > gnulib-tool: fix tests of 'extensions' module
  > unicase/locale-language: fix typo in utf-8 cookie
  > autoupdate
  > xalloc: do not worry about GCC 5 warning on 32 bit
  > xalloc: avoid GCC 5.1 warning on 32 bit
  > uniname/uniname-tests: avoid compiler warnings
  > autoupdate
  > mountlist: clean up of variable duplication
  > c-ctype: do not worry about EBCDIC + char signed
  > c-ctype: port better to z/OS EBCDIC
  > gnulib-common.m4: fix gl_PROG_AR_RANLIB/AM_PROG_AR clash
  > sockets: MS Windows initalization fixes
  > gc: fix detection of installed libgcrypt version
  > c-ctype: rewrite to use inline functions
  > fnmatch: add one more coding cookie
  > maint: add coding cookies to non-ASCII sources
  > gitlog-to-changelog: trim only trailing whitespaces
  > Test that c_iscntrl agrees with iscntrl, etc.
  > c-ctype: improve c_isascii testing
  > Fix ChangeLog typo
  > savewd: remove SAVEWD_CHDIR_READABLE
  > Update ChangeLog to match previous patch.
  > c-ctype: support EBCDIC-style c_isascii
  > c-ctype: assume EBCDIC 1047 for c_iscntrl
  > * modules/c-ctype (Depends-on): Add verify.
  > c-ctype: port better to EBCDIC
  > nanosleep: fix return code for interrupted replacement
  > autoupdate
  > Diagnose ERE '()|\1'
  > Revert previous patch, as it did not fix the bug after all.
  > regex: fix dangling-backreference bug
  > regex: merge patches from libc
  > autoupdate
  > autoupdate
  > autoupdate
  > autoupdate
  > ceill: detect buggy OpenBSD implementation
  > mountlist: add me_mntroot field on Linux machines
  > doc: Describe to use multiple instances of gnulib
  > autoupdate
  > autoupdate
  > base32: mark function as __attribute__ const
  > autoupdate
  > autoupdate
  > gnulib-tool: don't transform binary files with sed
  > autoupdate
  >

[libvirt] [PATCH] tests: Add newlines with VIR_TEST_REGENERATE_OUTPUT

2016-01-04 Thread Cole Robinson 
Since test files are formatted predictably nowadays, we can make
VIR_TEST_REGENERATE_OUTPUT handle most cases for us with a simple
replacement. test-wrap-argv.pl is still canon, but this bit makes
it easier to confirm test output changes during active development.
---
 tests/testutils.c | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/tests/testutils.c b/tests/testutils.c
index 857e819..6487a62 100644
--- a/tests/testutils.c
+++ b/tests/testutils.c
@@ -598,6 +598,7 @@ virtTestCompareToFile(const char *strcontent,
 int ret = -1;
 char *filecontent = NULL;
 char *fixedcontent = NULL;
+char *regencontent = NULL;
 bool regenerate = !!virTestGetFlag("VIR_TEST_REGENERATE_OUTPUT");
 
 if (virtTestLoadFile(filename, ) < 0 && !regenerate)
@@ -613,7 +614,10 @@ virtTestCompareToFile(const char *strcontent,
 if (STRNEQ_NULLABLE(fixedcontent ? fixedcontent : strcontent,
 filecontent)) {
 if (regenerate) {
-if (virFileWriteStr(filename, strcontent, 0666) < 0)
+if (!(regencontent = virStringReplace(strcontent, " -", " \\\n-")))
+goto failure;
+
+if (virFileWriteStr(filename, regencontent, 0666) < 0)
 goto failure;
 goto out;
 }
@@ -626,6 +630,7 @@ virtTestCompareToFile(const char *strcontent,
  failure:
 VIR_FREE(fixedcontent);
 VIR_FREE(filecontent);
+VIR_FREE(regencontent);
 return ret;
 }
 
-- 
2.5.0

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

Re: [libvirt] Reboot with SR-IOV devices confuses IOMMU (bug #1294677)

2016-01-04 Thread Jakub Kicinski 
On Tue, 29 Dec 2015 15:58:48 +, Jakub Kicinski wrote:
> Hi!
> 
> I hit an issue with PCI passthrough - after I reboot the VM IOMMU
> mappings are incorrect and devices will access invalid memory.
> [...]

Reproduced easily on fully up-to-date CentOS 7 with ixgbe (Intel's
10gig cards) on Ivy Bridge CPU:

CentOS Linux release 7.2.1511 (Core)

# rpm -q libvirt qemu-kvm kernel
libvirt-1.2.17-13.el7_2.2.x86_64
qemu-kvm-1.5.3-105.el7_2.1.x86_64
kernel-3.10.0-327.3.1.el7.x86_64

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

Re: [libvirt] Non-contiguous NUMA cell numbers

2016-01-04 Thread Andrea Bolognani 
n Mon, 2016-01-04 at 07:57 +0100, Michal Privoznik wrote:
> On 23.12.2015 17:14, Andrea Bolognani wrote:
> > Hi all,
> > 
> > libvirt currently doesn't allow you to configure a guest with something
> > like
> > 
> >   
> > 
> > 
> >   
> >   
> > 
> 
> This says you are configuring GUEST NUMA nodes, and last time I checked
> qemu did not support non-continuous NUMA nodes. So, whatever your HOST
> topology is, in GUEST you want NUMA nodes to be continuous.

I checked again and, as you and Daniel suggested, QEMU does not support
non-contiguous NUMA nodes, eg.

  qemu-kvm   \
-m 2048  \
-smp 80,sockets=2,cores=5,threads=8  \
-numa node,nodeid=0,cpus=0-39,mem=1024   \
-numa node,nodeid=16,cpus=40-79,mem=1024

outputs

  qemu-kvm: numa: Node ID missing: 15

so indeed there's a very good reason for libvirt to reject that NUMA
configuration.

Cheers.

-- 
Andrea Bolognani
Software Engineer - Virtualization Team

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

Re: [libvirt] [libvirt-glib][PATCH] libvirt-glib-1.0.pc.in: Correctly mark variable

2016-01-04 Thread Martin Kletzander 

On Mon, Jan 04, 2016 at 10:58:15AM +0100, Christophe Fergeau wrote:

Hey,

On Sun, Jan 03, 2016 at 12:49:30PM +0100, Martin Kletzander wrote:

On Sat, Jan 02, 2016 at 12:46:30PM +0100, Michal Privoznik wrote:
>In the pkg-config file for libvirt-glib we have a typo:
>
> Libs.private: @LIBVIRT_LIBS @GLIB2_LIBS@
>
>Noticed the missing '@' after LIBVIRT_LIBS? Well, I just did.
>
>Signed-off-by: Michal Privoznik 
>---
>libvirt-glib-1.0.pc.in | 2 +-
>1 file changed, 1 insertion(+), 1 deletion(-)
>

ACK, although it doesn't look like it hurt anyone, ever.  I'm not sure
when Libs.private is used, but I haven't managed to convince pkg-config
to use it or print it anywhere.


It's used when asking for flags for static linking:

$ pkg-config --static --libs libvirt-glib-1.0
-lvirt-glib-1.0 @LIBVIRT_LIBS -lglib-2.0 -pthread



Oh, thanks, good to know.


Christophe


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

[libvirt] [PATCH 2/5] wireshark: s/ep_alloc/wmem_alloc/

2016-01-04 Thread Michal Privoznik 
In wireshark, they have introduced their own memory allocator
wmem. This means that we need to adapt our code to that change
too. Notably 0ad15f88ccf434e8210ca is the wireshark commit you
want to look at. It's the one where they dropped the old API. The
new allocator has been introduced in 84cc3daa (v1.10.0), however,
was not exposed until 5c05c9e0 (v1.10.0). Since we already are
requiring 1.11.3 or higher no other change is needed.

Signed-off-by: Michal Privoznik 
---
 tools/wireshark/src/packet-libvirt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/wireshark/src/packet-libvirt.c 
b/tools/wireshark/src/packet-libvirt.c
index 3103562..f7c8e0c 100644
--- a/tools/wireshark/src/packet-libvirt.c
+++ b/tools/wireshark/src/packet-libvirt.c
@@ -113,7 +113,7 @@ format_xdr_bytes(guint8 *bytes, guint32 length)
 
 if (length == 0)
 return "";
-buf = ep_alloc(length*2 + 1);
+buf = wmem_alloc(wmem_packet_scope(), length*2 + 1);
 for (i = 0; i < length; i++) {
 /* We know that buf has enough size to contain
2 * length + '\0' characters. */
-- 
2.4.10

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

[libvirt] [PATCH 5/5] wireshark: Fix header of get_message_len()

2016-01-04 Thread Michal Privoznik 
In wireshark commit ceb8d954 (v1.99.2) they have changed the
signature of a function that determines how long a libvirt packet
is. Now it accepts a void pointer for passing data into the
function. Well, this is nice, but we don't need it right now.
Anyway, we have to change our code.

Signed-off-by: Michal Privoznik 
---
 tools/wireshark/src/packet-libvirt.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/tools/wireshark/src/packet-libvirt.c 
b/tools/wireshark/src/packet-libvirt.c
index ac120b5..aa1c323 100644
--- a/tools/wireshark/src/packet-libvirt.c
+++ b/tools/wireshark/src/packet-libvirt.c
@@ -431,8 +431,13 @@ dissect_libvirt_message(tvbuff_t *tvb, packet_info *pinfo, 
proto_tree *tree,
 #endif
 }
 
+#if WIRESHARK_VERSION >= 1099002
+static guint
+get_message_len(packet_info *pinfo ATTRIBUTE_UNUSED, tvbuff_t *tvb, int 
offset, void *data ATTRIBUTE_UNUSED)
+#else
 static guint32
 get_message_len(packet_info *pinfo ATTRIBUTE_UNUSED, tvbuff_t *tvb, int offset)
+#endif
 {
 return tvb_get_ntohl(tvb, offset);
 }
-- 
2.4.10

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

[libvirt] [PATCH 3/5] wireshark: s/tvb_length/tvb_captured_length/

2016-01-04 Thread Michal Privoznik 
In wireshak commit 22149c55 (v.1.11.3) the API was renamed.
Follow the change in our code too. Since the wireshark change was
made in the very same version that we require at least we are
good to go.

Signed-off-by: Michal Privoznik 
---
 tools/wireshark/src/packet-libvirt.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/wireshark/src/packet-libvirt.c 
b/tools/wireshark/src/packet-libvirt.c
index f7c8e0c..d0b0852 100644
--- a/tools/wireshark/src/packet-libvirt.c
+++ b/tools/wireshark/src/packet-libvirt.c
@@ -338,7 +338,7 @@ dissect_libvirt_payload(tvbuff_t *tvb, proto_tree *tree,
 {
 gssize payload_length;
 
-payload_length = tvb_length(tvb) - VIR_HEADER_LEN;
+payload_length = tvb_captured_length(tvb) - VIR_HEADER_LEN;
 if (payload_length <= 0)
 return; /* No payload */
 
@@ -405,7 +405,7 @@ dissect_libvirt_message(tvbuff_t *tvb, packet_info *pinfo, 
proto_tree *tree,
 proto_item *ti;
 proto_tree *libvirt_tree;
 
-ti = proto_tree_add_item(tree, proto_libvirt, tvb, 0, tvb_length(tvb), 
ENC_NA);
+ti = proto_tree_add_item(tree, proto_libvirt, tvb, 0, 
tvb_captured_length(tvb), ENC_NA);
 libvirt_tree = proto_item_add_subtree(ti, ett_libvirt);
 
 offset = 0;
-- 
2.4.10

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

[libvirt] [PATCH 4/5] wireshark: Replace WIRESHARK_COMPAT with actual version comparison

2016-01-04 Thread Michal Privoznik 
In the upcoming patch we will need yet another #ifdef code block
depending on wireshark version. Instead of defining
WIRESHARK_COMPAT2 or something lets just compare the version
right at the place so that we can clearly see what version broke
API.

Signed-off-by: Michal Privoznik 
---
 tools/wireshark/src/packet-libvirt.c | 14 +-
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/tools/wireshark/src/packet-libvirt.c 
b/tools/wireshark/src/packet-libvirt.c
index d0b0852..ac120b5 100644
--- a/tools/wireshark/src/packet-libvirt.c
+++ b/tools/wireshark/src/packet-libvirt.c
@@ -36,16 +36,12 @@
 #include "packet-libvirt.h"
 #include "internal.h"
 
+/* Wireshark 1.12 brings API change */
 #define WIRESHARK_VERSION   \
 ((VERSION_MAJOR * 1000 * 1000) +\
  (VERSION_MINOR * 1000) +   \
  (VERSION_MICRO))
 
-/* Wireshark 1.12 brings API change */
-#if WIRESHARK_VERSION < 1012000
-# define WIRESHARK_COMPAT
-#endif
-
 static int proto_libvirt = -1;
 static int hf_libvirt_length = -1;
 static int hf_libvirt_program = -1;
@@ -316,7 +312,7 @@ dissect_libvirt_payload_xdr_data(tvbuff_t *tvb, proto_tree 
*tree, gint payload_l
 }
 
 payload_tvb = tvb_new_subset(tvb, start, -1, payload_length);
-#ifdef WIRESHARK_COMPAT
+#if WIRESHARK_VERSION < 1012000
 payload_data = (caddr_t)tvb_memdup(payload_tvb, 0, payload_length);
 #else
 payload_data = (caddr_t)tvb_memdup(NULL, payload_tvb, 0, payload_length);
@@ -362,7 +358,7 @@ dissect_libvirt_payload(tvbuff_t *tvb, proto_tree *tree,
 proto_tree_add_item(tree, hf_libvirt_unknown, tvb, VIR_HEADER_LEN, -1, 
ENC_NA);
 }
 
-#ifdef WIRESHARK_COMPAT
+#if WIRESHARK_VERSION < 1012000
 static void
 dissect_libvirt_message(tvbuff_t *tvb, packet_info *pinfo, proto_tree *tree)
 #else
@@ -430,7 +426,7 @@ dissect_libvirt_message(tvbuff_t *tvb, packet_info *pinfo, 
proto_tree *tree,
 dissect_libvirt_payload(tvb, libvirt_tree, prog, proc, type, status);
 }
 
-#ifndef WIRESHARK_COMPAT
+#if WIRESHARK_VERSION >= 1012000
 return 0;
 #endif
 }
@@ -446,7 +442,7 @@ dissect_libvirt(tvbuff_t *tvb, packet_info *pinfo, 
proto_tree *tree)
 {
 /* Another magic const - 4; simply, how much bytes
  * is needed to tell the length of libvirt packet. */
-#ifdef WIRESHARK_COMPAT
+#if WIRESHARK_VERSION < 1012000
 tcp_dissect_pdus(tvb, pinfo, tree, TRUE, 4,
  get_message_len, dissect_libvirt_message);
 #else
-- 
2.4.10

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

[libvirt] [PATCH 1/5] wireshark: s/proto_tree_add_text/proto_tree_add_item/

2016-01-04 Thread Michal Privoznik 
In the wireshark commit e2735ecfdd7a96c they dropped
proto_tree_add_text in favor of proto_tree_add_item. Adapt to
this change.

Moreover, the proto_tree_add_item API is around for ages and we
are already using it anyway. Therefore we don't need to change
required version of wireshark.

Signed-off-by: Michal Privoznik 
---
 tools/wireshark/src/packet-libvirt.c | 2 +-
 tools/wireshark/util/genxdrstub.pl   | 7 +--
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/tools/wireshark/src/packet-libvirt.c 
b/tools/wireshark/src/packet-libvirt.c
index f7aa7ed..3103562 100644
--- a/tools/wireshark/src/packet-libvirt.c
+++ b/tools/wireshark/src/packet-libvirt.c
@@ -56,7 +56,7 @@ static int hf_libvirt_serial = -1;
 static int hf_libvirt_status = -1;
 static int hf_libvirt_stream = -1;
 static int hf_libvirt_num_of_fds = -1;
-static int hf_libvirt_unknown = -1;
+int hf_libvirt_unknown = -1;
 static gint ett_libvirt = -1;
 
 #define XDR_PRIMITIVE_DISSECTOR(xtype, ctype, ftype)\
diff --git a/tools/wireshark/util/genxdrstub.pl 
b/tools/wireshark/util/genxdrstub.pl
index 76ccda9..07f0ff7 100755
--- a/tools/wireshark/util/genxdrstub.pl
+++ b/tools/wireshark/util/genxdrstub.pl
@@ -57,6 +57,9 @@ for my $proto (@ARGV) {
 
 $c->add_header_file($name, sub {
 dbg "*** Start parsing $proto\n";
+
+$c->print("extern int hf_libvirt_unknown;\n");
+
 my @lexs = Lexicalizer->parse($source);
 for my $lex (@lexs) {
 next if $lex->ident eq "enum $name\_procedure";
@@ -903,7 +906,7 @@ static gboolean dissect_xdr_<%= $ident %>(tvbuff_t *tvb, 
proto_tree *tree, XDR *
 <% } %>
 }
 } else {
-proto_tree_add_text(tree, tvb, start, -1, "(unknown)");
+proto_tree_add_item(tree, hf_libvirt_unknown, tvb, start, -1, ENC_NA);
 }
 return FALSE;
 }
@@ -933,7 +936,7 @@ static gboolean dissect_xdr_<%= $ident %>(tvbuff_t *tvb, 
proto_tree *tree, XDR *
 <% } %>
 }
 if (!rc) {
-proto_tree_add_text(tree, tvb, start, -1, "(unknown)");
+proto_tree_add_item(tree, hf_libvirt_unknown, tvb, start, -1, ENC_NA);
 }
 return rc;
 }
-- 
2.4.10

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

[libvirt] [PATCH 0/5] Couple of wireshark fixes

2016-01-04 Thread Michal Privoznik 
Well, I've just updated wireshark on my system and encountered couple of
compile errors while building libvirt. Here are the fixes. Fortunately, none of
them requires us to increase the version number of wireshark that's required.

However, I'd like to discuss how are we going to handle this. I mean, at
wireshark they don't seem so committed to API stability as we are. Therefore
this patch set. In the long term I don't see us adapting to every single API
change, or do I? Although I am not sure what are our options here.

Michal Privoznik (5):
  wireshark: s/proto_tree_add_text/proto_tree_add_item/
  wireshark: s/ep_alloc/wmem_alloc/
  wireshark: s/tvb_length/tvb_captured_length/
  wireshark: Replace WIRESHARK_COMPAT with actual version comparison
  wireshark: Fix header of get_message_len()

 tools/wireshark/src/packet-libvirt.c | 27 ++-
 tools/wireshark/util/genxdrstub.pl   |  7 +--
 2 files changed, 19 insertions(+), 15 deletions(-)

-- 
2.4.10

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

Re: [libvirt] [libvirt-glib][PATCH] libvirt-glib-1.0.pc.in: Correctly mark variable

2016-01-04 Thread Christophe Fergeau 
Hey,

On Sun, Jan 03, 2016 at 12:49:30PM +0100, Martin Kletzander wrote:
> On Sat, Jan 02, 2016 at 12:46:30PM +0100, Michal Privoznik wrote:
> >In the pkg-config file for libvirt-glib we have a typo:
> >
> > Libs.private: @LIBVIRT_LIBS @GLIB2_LIBS@
> >
> >Noticed the missing '@' after LIBVIRT_LIBS? Well, I just did.
> >
> >Signed-off-by: Michal Privoznik 
> >---
> >libvirt-glib-1.0.pc.in | 2 +-
> >1 file changed, 1 insertion(+), 1 deletion(-)
> >
> 
> ACK, although it doesn't look like it hurt anyone, ever.  I'm not sure
> when Libs.private is used, but I haven't managed to convince pkg-config
> to use it or print it anywhere.

It's used when asking for flags for static linking:

$ pkg-config --static --libs libvirt-glib-1.0
-lvirt-glib-1.0 @LIBVIRT_LIBS -lglib-2.0 -pthread

Christophe


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

[libvirt] Reboot with SR-IOV devices confuses IOMMU (bug #1294677)

2016-01-04 Thread Jakub Kicinski 
Hi!

I hit an issue with PCI passthrough - after I reboot the VM IOMMU
mappings are incorrect and devices will access invalid memory.

The issue is quite easy to reproduce, I'm using NICs (e.g. Intel 40
Gig Ethernet NICs hit the issue reliably, I wasn't able to get
Mellanox NICs to work reliably due to other bugs) but you could
probably just pass through any PCI device which does a bit of DMA.

With NICs I do the following to reproduce:
 - configure the NIC for SR-IOV passthrough [1];
 - create two standard VMs;
 - configure VMs with 4GB current allocation and 15GB maximum
   allocation of memory (my machines have 32 or 64GB total);
 - pass a VF to each machine;

Note1: the current/maximum allocation of memory seem to play a role
   here.  I'm not 100% sure, however, if it causes the bug or just
   makes it more likely to be triggered.
Note2: I leave restart so that VMs can reboot.

I was able to reproduce easily on 3 distinct machines: dual CPU
Haswell E, single CPU Haswell E, single Sandy Bridge EP.

With the VMs created above do the following:
 (1) boot;
 (2) configure VF interfaces;
 (3) run ping -c30 to confirm they can communicate;
 (4) run iperf -P4 -t30 between the machines;
 (5) reboot;
 (6) goto 2;

First time (fresh after boot) ping and iperf should work fine.  After
first reboot, there should already be communication problems.  From
traffic inspection with tcpdump it appears that VFs receive zeroed
packets.  Only some of the packets are zeroed so depending on your luck
the communication may work for a while or just have limited throughput.
Usually it breaks down when ARP or important TCP segment gets placed in
area that device reads as zero.  Reboot will not fix this condition,
shutdown/start will.

I reproduced this with fully up-to-date Ubuntu 14.04 (both host and VM).
I also tried kernel from linux-next.git (4ef7675344d68) and qemu from
tip of their git (38a762f) and bug is still there.  libvirt in Ubuntu
14.04 is at 1.2.2.

As already said - I reproduce this easily within a minute on a range of
machines.  If someone looks into this and have problems hitting the bug
- please let me know.

Kuba


[1] For Intel NICs supported by i40e:
http://www.intel.es/content/www/es/es/embedded/products/networking/xl710-sr-iov-config-guide-gbe-linux-brief.html

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

Re: [libvirt] Non-contiguous NUMA cell numbers

2016-01-04 Thread Roy Golan 
On Mon, Jan 4, 2016 at 12:18 PM, Andrea Bolognani 
wrote:

> n Mon, 2016-01-04 at 07:57 +0100, Michal Privoznik wrote:
> > On 23.12.2015 17:14, Andrea Bolognani wrote:
> > > Hi all,
> > >
> > > libvirt currently doesn't allow you to configure a guest with something
> > > like
> > >
> > >   
> > > 
> > > 
> > >   
> > >   
> > > 
> >
> > This says you are configuring GUEST NUMA nodes, and last time I checked
> > qemu did not support non-continuous NUMA nodes. So, whatever your HOST
> > topology is, in GUEST you want NUMA nodes to be continuous.
>
> I checked again and, as you and Daniel suggested, QEMU does not support
> non-contiguous NUMA nodes, eg.
>
>   qemu-kvm   \
> -m 2048  \
> -smp 80,sockets=2,cores=5,threads=8  \
> -numa node,nodeid=0,cpus=0-39,mem=1024   \
> -numa node,nodeid=16,cpus=40-79,mem=1024
>
> outputs
>
>   qemu-kvm: numa: Node ID missing: 15
>
> so indeed there's a very good reason for libvirt to reject that NUMA
> configuration.
>
>

Thanks you all for all the input. oVirt will follow this configuration
limitation and enforce it then.


Cheers.
>
> --
> Andrea Bolognani
> Software Engineer - Virtualization Team
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 2/2] qemu: domain: Prevent overflows in memory alignment code

2016-01-04 Thread Peter Krempa 
On Sun, Jan 03, 2016 at 18:26:56 +0100, Guido Günther wrote:
> Hi,
> On Tue, Dec 01, 2015 at 03:11:05PM +0100, Peter Krempa wrote:
> > Since libvirt for dubious historical reasons stores memory size as
> > kibibytes, it's possible that the alignments done in the qemu code
> > overflow the the maximum representable size in bytes. The XML parser
> > code handles them in bytes in some stages. Prevent this by doing
> > overflow checks when alinging the size and add a test case.
> 
> It seems this broke the build on i386:
> 
> 
> https://buildd.debian.org/status/fetch.php?pkg=libvirt=i386=1.3.0-1=1450436203
> (search for memory-align-fail)
> 
> I did not investigate further yet though.

This should be already fixed ...

commit ace1ee225f5cd87fb095054a6a19bdcd0fa57518
Author: Peter Krempa 
Date:   Thu Dec 10 14:36:51 2015 +0100

test: qemuxml2argv: Mock virMemoryMaxValue to remove 32/64 bit difference

Always return LLONG_MAX even on 32 bit systems. The limitation
originates from our use of "unsigned long" in several APIs. The internal
data type is unsigned long long. Make the test suite deterministic by
removing the architecture difference.

Flaw was introduced in 645881139b3d2c86acf9d644c3a1471520bc9e57 where
I've added a test that uses too large numbers.


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

Re: [libvirt] [PATCH v8 1/5] virstoragefile: Add helper to get storage source backingStore

2016-01-04 Thread Matthias Gatto 
On Wed, Dec 16, 2015 at 12:14 AM, John Ferlan  wrote:
> From: Matthias Gatto 
>
> Add a new helper - virStorageSourceGetBackingStore - to fetch the storage
> source backingStore pointer in order to make it easier to change the
> future format of the data.
>
> A future patch will adjust the backingStore pointer to become a table or
> array of backingStorePtr's accessible by the argument 'pos'.
>
> For now, if 'pos' > 0, the code will return NULL as if the backingStore
> pointer didn't exist. All callers in subsequent patches will start by
> passing a 0 as the parameter.
>
> Signed-off-by: Matthias Gatto 
> Signed-off-by: John Ferlan 
> ---
>  src/libvirt_private.syms  |  1 +
>  src/util/virstoragefile.c | 23 +++
>  src/util/virstoragefile.h |  3 +++
>  3 files changed, 27 insertions(+)
>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 55822ae..1c55370 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -2196,6 +2196,7 @@ virStorageSourceClear;
>  virStorageSourceCopy;
>  virStorageSourceFree;
>  virStorageSourceGetActualType;
> +virStorageSourceGetBackingStore;
>  virStorageSourceGetSecurityLabelDef;
>  virStorageSourceInitChainElement;
>  virStorageSourceIsEmpty;
> diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
> index 2aa1d90..2771c95 100644
> --- a/src/util/virstoragefile.c
> +++ b/src/util/virstoragefile.c
> @@ -1810,6 +1810,29 @@ virStorageSourcePoolDefCopy(const 
> virStorageSourcePoolDef *src)
>
>
>  /**
> + * virStorageSourceGetBackingStore:
> + * @src: virStorageSourcePtr containing the backing stores
> + * @pos: presently unused
> + *
> + * Return the @src backingStore pointer at @pos. For now, @pos is
> + * expected to be 0. A future patch will use @pos index into an array
> + * of storage backingStore pointers
> + *
> + * Returns:
> + * A pointer to the storage source backingStore @pos or
> + * NULL if the backingStore pointer cannot be found
> + */
> +virStorageSourcePtr
> +virStorageSourceGetBackingStore(const virStorageSource *src,
> +size_t pos)
> +{
> +if (!src || pos > 0)
> +return NULL;
> +return src->backingStore;
> +}
> +
> +
> +/**
>   * virStorageSourcePtr:
>   *
>   * Deep-copies a virStorageSource structure. If @backing chain is true
> diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
> index b98fe25..8cd4854 100644
> --- a/src/util/virstoragefile.h
> +++ b/src/util/virstoragefile.h
> @@ -289,6 +289,9 @@ struct _virStorageSource {
>  #  define DEV_BSIZE 512
>  # endif
>
> +virStorageSourcePtr virStorageSourceGetBackingStore(const virStorageSource 
> *src,
> +size_t pos);
> +
>  int virStorageFileProbeFormat(const char *path, uid_t uid, gid_t gid);
>  int virStorageFileProbeFormatFromBuf(const char *path,
>   char *buf,
> --
> 2.5.0
>

ping

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

Re: [libvirt] [PATCH] virsh: improve waiting for block job readiness

2016-01-04 Thread Michael Chapman 

On Mon, 4 Jan 2016, Andrea Bolognani wrote:

On Thu, 2015-12-31 at 16:34 +1100, Michael Chapman wrote:

Wait for a block job event after the job has reached 100% only if
exactly one of the BLOCK_JOB and BLOCK_JOB_2 callbacks were successfully
registered.
 
If neither callback was registered, then no event will ever be received.
If both were registered, then any user-supplied path is guaranteed to
match one of the events.
 
Signed-off-by: Michael Chapman 
---
 
I have found that even a 2.5 second timeout isn't always sufficiently
long for QEMU to flush a disk at the end of a block job.
 
I hope I've understood the code properly here, because as far as I can
tell the comment I'm removing in this commit isn't right. The path the
user supplies *has* to be either the  or  exactly in order for the disk to be matched, and these are
precisely the two strings used by the two events.
 
  tools/virsh-domain.c | 24 +---
  1 file changed, 13 insertions(+), 11 deletions(-)
 
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index edbbc34..60de9ba 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -1942,18 +1942,20 @@ virshBlockJobWait(virshBlockJobWaitDataPtr data)
  goto cleanup;
  }
  
-/* since virsh can't guarantee that the path provided by the user will
- * later be matched in the event we will need to keep the fallback
- * approach and claim success if the block job finishes or vanishes. */
-if (result == 0)
-break;
+/* if either event could not be registered we can't guarantee that the
+ * path provided by the user will be matched, so keep the fallback
+ * approach and claim success if the block job finishes or vanishes */
+if (data->cb_id2 < 0 || data->cb_id2 < 0) {


I'm not going to comment on the rest of the patch, but the condition
above doesn't look like it would make any sense written that way...


Oops, good catch. I had meant to use cb_id and cb_id2 respectively in the 
two conditions (and in the following test as well).



+if (result == 0)
+break;
  
-/* for two-phase jobs we will try to wait in the synchronized phase
- * for event arrival since 100% completion doesn't necessarily mean 
that
- * the block job has finished and can be terminated with success */
-if (info.end == info.cur && --retries == 0) {
-ret = VIR_DOMAIN_BLOCK_JOB_READY;
-goto cleanup;
+/* only wait in the synchronized phase for event arrival if
+ * either callback was registered */
+if (info.end == info.cur &&
+((data->cb_id2 < 0 && data->cb_id2 < 0) || --retries == 0)) {


... and neither does this one.

Am I missing something?

Cheers.


+ret = VIR_DOMAIN_BLOCK_JOB_READY;
+goto cleanup;
+}
  }
  
  if (data->verbose)

-- 
Andrea Bolognani
Software Engineer - Virtualization Team


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

Re: [libvirt] [PATCH] virsh: improve waiting for block job readiness

2016-01-04 Thread Michael Chapman 

On Mon, 4 Jan 2016, Peter Krempa wrote:

On Thu, Dec 31, 2015 at 16:34:49 +1100, Michael Chapman wrote:

Wait for a block job event after the job has reached 100% only if
exactly one of the BLOCK_JOB and BLOCK_JOB_2 callbacks were successfully
registered.

If neither callback was registered, then no event will ever be received.
If both were registered, then any user-supplied path is guaranteed to
match one of the events.

Signed-off-by: Michael Chapman 
---

I have found that even a 2.5 second timeout isn't always sufficiently
long for QEMU to flush a disk at the end of a block job.

I hope I've understood the code properly here, because as far as I can
tell the comment I'm removing in this commit isn't right. The path the
user supplies *has* to be either the  or  exactly in order for the disk to be matched, and these are
precisely the two strings used by the two events.

 tools/virsh-domain.c | 24 +---
 1 file changed, 13 insertions(+), 11 deletions(-)


In addition to Andrea's review:



diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index edbbc34..60de9ba 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -1942,18 +1942,20 @@ virshBlockJobWait(virshBlockJobWaitDataPtr data)
 goto cleanup;
 }

-/* since virsh can't guarantee that the path provided by the user will
- * later be matched in the event we will need to keep the fallback
- * approach and claim success if the block job finishes or vanishes. */
-if (result == 0)
-break;
+/* if either event could not be registered we can't guarantee that the
+ * path provided by the user will be matched, so keep the fallback
+ * approach and claim success if the block job finishes or vanishes */


The new statement is not true. If the user provides a filesystem path
different from what is in the definition but matching the same volume [1]
the callback will still return the path present in the configuration and
thus might not ever match and the job would hang forever.

[1] e.g. /path/to/image and /path/to/../to/image are equivalent but
won't be equal using strcmp. The problem is even more prominent if you
mix in some symlinks.


As far I can tell the block job won't even be able to be identified if the 
user specifies a different path than the one in the domain XML.


At present, the only implementation of the virGetBlockJobInfo API 
is qemuDomainGetBlockJobInfo. The disk definition is found in the call 
chain:


  qemuDomainGetBlockJobInfo
-> virDomainDiskByName
  -> virDomainDiskIndexByName

and virDomainDiskIndexByName only does STREQ tests to match the supplied 
path against the  or  elements.


So at present, the disk path supplied by the user of virGetBlockJobInfo 
has to be *exactly* the source path or the target name, and these are 
precisely the two strings used in the two events.


The only safe way to allow different-but-equivalent paths to match the one 
disk definition would be record the device+inode numbers in the disk 
definition. We can't simply follow symlinks to resolve the path, since the 
symlinks could have changed since the domain was started -- e.g. 
/path/to/../to/image may now be equivalent to /path/to/image, but 
/path/to/image may not be the same as what it was when the domain was 
started.


So on that basis I think we have to settle for requiring the 
virGetBlockJobInfo path to match the disk definition exactly.



+if (data->cb_id2 < 0 || data->cb_id2 < 0) {
+if (result == 0)
+break;

-/* for two-phase jobs we will try to wait in the synchronized phase
- * for event arrival since 100% completion doesn't necessarily mean 
that
- * the block job has finished and can be terminated with success */
-if (info.end == info.cur && --retries == 0) {
-ret = VIR_DOMAIN_BLOCK_JOB_READY;
-goto cleanup;
+/* only wait in the synchronized phase for event arrival if
+ * either callback was registered */
+if (info.end == info.cur &&
+((data->cb_id2 < 0 && data->cb_id2 < 0) || --retries == 0)) {
+ret = VIR_DOMAIN_BLOCK_JOB_READY;
+goto cleanup;
+}
 }


NACK to this approach, if there was a way how this ugly stuff could be
avoided or deleted I'd already do so.

Peter


OK. At any rate, I do think 2.5 seconds is not enough. On a hypervisor 
with a large amount of memory I was able to generate block jobs that would 
take 10-15 seconds to fully flush out to disk.


- Michael

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

[libvirt] [PATCH V2 3/3] libxl: support vif outgoing bandwidth QoS

2016-01-04 Thread Jim Fehlig 
The libxl_device_nic structure supports specifying an outgoing rate
limit based on a time interval and bytes allowed per interval. In xl
config a rate limit is specified as "/s@". INTERVAL
is optional and defaults to 50ms.

libvirt expresses outgoing limits by average (required), peak, burst,
and floor attributes in units of KB/s. This patch supports the outgoing
bandwidth limit by converting the average KB/s to bytes per interval
based on the same default interval (50ms) used by xl.

Signed-off-by: Jim Fehlig 
---
 src/libxl/libxl_conf.c | 39 +++
 1 file changed, 39 insertions(+)

diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index 23c74e7..6320421 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -1093,6 +1093,7 @@ libxlMakeNic(virDomainDefPtr def,
 {
 bool ioemu_nic = def->os.type == VIR_DOMAIN_OSTYPE_HVM;
 virDomainNetType actual_type = virDomainNetGetActualType(l_nic);
+virNetDevBandwidthPtr actual_bw;
 
 /* TODO: Where is mtu stored?
  *
@@ -1206,6 +1207,44 @@ libxlMakeNic(virDomainDefPtr def,
 #endif
 }
 
+/*
+ * Set bandwidth.
+ * From $xen-sources/docs/misc/xl-network-configuration.markdown:
+ *
+ *
+ * Specifies the rate at which the outgoing traffic will be limited to.
+ * The default if this keyword is not specified is unlimited.
+ *
+ * The rate may be specified as "/s" or optionally 
"/s@".
+ *
+ * `RATE` is in bytes and can accept suffixes:
+ * GB, MB, KB, B for bytes.
+ * Gb, Mb, Kb, b for bits.
+ * `INTERVAL` is in microseconds and can accept suffixes: ms, us, s.
+ * It determines the frequency at which the vif transmission credit
+ * is replenished. The default is 50ms.
+
+ * Vif rate limiting is credit-based. It means that for "1MB/s@20ms",
+ * the available credit will be equivalent of the traffic you would have
+ * done at "1MB/s" during 20ms. This will results in a credit of 20,000
+ * bytes replenished every 20,000 us.
+ *
+ *
+ * libvirt doesn't support the notion of rate limiting over an interval.
+ * Similar to xl's behavior when interval is not specified, set a default
+ * interval of 50ms and calculate the number of bytes per interval based
+ * on the specified average bandwidth.
+ */
+actual_bw = virDomainNetGetActualBandwidth(l_nic);
+if (actual_bw && actual_bw->out && actual_bw->out->average) {
+uint64_t bytes_per_sec = actual_bw->out->average * 1024;
+uint64_t bytes_per_interval =
+(((uint64_t) bytes_per_sec * 5UL) / 100UL);
+
+x_nic->rate_bytes_per_interval = bytes_per_interval;
+x_nic->rate_interval_usecs =  5UL;
+}
+
 return 0;
 }
 
-- 
2.1.4

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

[libvirt] [PATCH V2 2/3] xenconfig: support vif bandwidth in xm and xl parser and formatter

2016-01-04 Thread Jim Fehlig 
Both xm and xl config have long supported specifying vif rate
limiting, e.g.

vif = [ 'mac=00:16:3E:74:3d:76,bridge=br0,rate=10MB/s' ]

Add support for mapping rate to and from  in the xenconfig
parser and formatter. rate is mapped to the required 'average' attribute
of the  element, e.g.

  
...

  

  

Also add a unit test to check the conversion logic.

Signed-off-by: Jim Fehlig 
---
 src/xenconfig/xen_common.c   | 30 +++
 tests/xlconfigdata/test-vif-rate.cfg | 26 
 tests/xlconfigdata/test-vif-rate.xml | 57 
 tests/xlconfigtest.c |  1 +
 4 files changed, 114 insertions(+)

diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c
index 54f5791..7cc8639 100644
--- a/src/xenconfig/xen_common.c
+++ b/src/xenconfig/xen_common.c
@@ -819,6 +819,7 @@ xenParseVif(virConfPtr conf, virDomainDefPtr def)
 char mac[18];
 char bridge[50];
 char vifname[50];
+char rate[50];
 char *key;
 
 bridge[0] = '\0';
@@ -827,6 +828,7 @@ xenParseVif(virConfPtr conf, virDomainDefPtr def)
 model[0] = '\0';
 type[0] = '\0';
 vifname[0] = '\0';
+rate[0] = '\0';
 
 if ((list->type != VIR_CONF_STRING) || (list->str == NULL))
 goto skipnic;
@@ -892,6 +894,13 @@ xenParseVif(virConfPtr conf, virDomainDefPtr def)
_("IP %s too big for destination"), 
data);
 goto skipnic;
 }
+} else if (STRPREFIX(key, "rate=")) {
+int len = nextkey ? (nextkey - data) : sizeof(rate) - 1;
+if (virStrncpy(rate, data, len, sizeof(rate)) == NULL) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("rate %s too big for destination"), 
data);
+goto skipnic;
+}
 }
 
 while (nextkey && (nextkey[0] == ',' ||
@@ -942,6 +951,24 @@ xenParseVif(virConfPtr conf, virDomainDefPtr def)
 VIR_STRDUP(net->ifname, vifname) < 0)
 goto cleanup;
 
+if (rate[0]) {
+virNetDevBandwidthPtr bandwidth;
+unsigned long long kbytes_per_sec;
+
+if (xenParseSxprVifRate(rate, _per_sec) < 0)
+goto cleanup;
+
+if (VIR_ALLOC(bandwidth) < 0)
+goto cleanup;
+if (VIR_ALLOC(bandwidth->out) < 0) {
+VIR_FREE(bandwidth);
+goto cleanup;
+}
+
+bandwidth->out->average = kbytes_per_sec;
+net->bandwidth = bandwidth;
+}
+
 if (VIR_APPEND_ELEMENT(def->nets, def->nnets, net) < 0)
 goto cleanup;
 
@@ -1184,6 +1211,9 @@ xenFormatNet(virConnectPtr conn,
 virBufferAsprintf(, ",vifname=%s",
   net->ifname);
 
+if (net->bandwidth && net->bandwidth->out && net->bandwidth->out->average)
+virBufferAsprintf(, ",rate=%lluKB/s", 
net->bandwidth->out->average);
+
 if (virBufferCheckError() < 0)
 goto cleanup;
 
diff --git a/tests/xlconfigdata/test-vif-rate.cfg 
b/tests/xlconfigdata/test-vif-rate.cfg
new file mode 100644
index 000..c5484dc
--- /dev/null
+++ b/tests/xlconfigdata/test-vif-rate.cfg
@@ -0,0 +1,26 @@
+name = "XenGuest2"
+uuid = "c7a5fdb2-cdaf-9455-926a-d65c16db1809"
+maxmem = 579
+memory = 394
+vcpus = 1
+pae = 1
+acpi = 1
+apic = 1
+hap = 0
+viridian = 0
+rtc_timeoffset = 0
+localtime = 0
+on_poweroff = "destroy"
+on_reboot = "restart"
+on_crash = "restart"
+device_model = "/usr/lib/xen/bin/qemu-dm"
+sdl = 0
+vnc = 1
+vncunused = 1
+vnclisten = "127.0.0.1"
+vif = [ 
"mac=00:16:3e:66:92:9c,bridge=xenbr1,script=vif-bridge,model=e1000,rate=10240KB/s"
 ]
+parallel = "none"
+serial = "none"
+builder = "hvm"
+boot = "d"
+disk = [ "/dev/HostVG/XenGuest2,raw,hda,w,backendtype=phy", 
"/var/lib/libvirt/images/XenGuest2-home,qcow2,hdb,w,backendtype=qdisk", 
"/root/boot.iso,raw,hdc,r,backendtype=qdisk,devtype=cdrom" ]
diff --git a/tests/xlconfigdata/test-vif-rate.xml 
b/tests/xlconfigdata/test-vif-rate.xml
new file mode 100644
index 000..29f0f79
--- /dev/null
+++ b/tests/xlconfigdata/test-vif-rate.xml
@@ -0,0 +1,57 @@
+
+  XenGuest2
+  c7a5fdb2-cdaf-9455-926a-d65c16db1809
+  592896
+  403456
+  1
+  
+hvm
+/usr/lib/xen/boot/hvmloader
+
+  
+  
+
+
+
+  
+  
+  destroy
+  restart
+  restart
+  
+/usr/lib/xen/bin/qemu-dm
+
+  
+  
+  
+  
+
+
+  
+  
+  
+  
+
+
+  
+  
+  
+  
+  
+
+
+  
+  
+  
+
+  
+  
+  
+
+
+
+
+  
+
+  
+
diff --git a/tests/xlconfigtest.c

[libvirt] [PATCH V2 0/3] Xen: Support vif outging bandwidth QoS

2016-01-04 Thread Jim Fehlig 
Happy New Year!

This small series adds support for specifying vif outgoing rate limits
in Xen. The first patch adds support for converting rate limits between
sexpr config and domXML. The second patch does the same for xl/xm config.
The third patch adds outgoing rate limiting to the libxl driver.

V1 here

https://www.redhat.com/archives/libvir-list/2015-December/msg00899.html

In V2 I've extended support to include the sexpr config format

Jim Fehlig (3):
  xenconfig: support vif bandwidth in sexpr parser and formatter
  xenconfig: support vif bandwidth in xm and xl parser and formatter
  libxl: support vif outgoing bandwidth QoS

 src/libvirt_xenconfig.syms  |  1 +
 src/libxl/libxl_conf.c  | 39 +
 src/xenconfig/xen_common.c  | 30 ++
 src/xenconfig/xen_sxpr.c| 74 +
 src/xenconfig/xen_sxpr.h|  2 +
 tests/sexpr2xmldata/sexpr2xml-vif-rate.sexpr| 11 
 tests/sexpr2xmldata/sexpr2xml-vif-rate.xml  | 51 +
 tests/sexpr2xmltest.c   |  2 +
 tests/xlconfigdata/test-vif-rate.cfg| 26 +
 tests/xlconfigdata/test-vif-rate.xml| 57 +++
 tests/xlconfigtest.c|  1 +
 tests/xml2sexprdata/xml2sexpr-fv-net-rate.sexpr | 10 
 tests/xml2sexprdata/xml2sexpr-fv-net-rate.xml   | 34 
 tests/xml2sexprtest.c   |  1 +
 14 files changed, 339 insertions(+)
 create mode 100644 tests/sexpr2xmldata/sexpr2xml-vif-rate.sexpr
 create mode 100644 tests/sexpr2xmldata/sexpr2xml-vif-rate.xml
 create mode 100644 tests/xlconfigdata/test-vif-rate.cfg
 create mode 100644 tests/xlconfigdata/test-vif-rate.xml
 create mode 100644 tests/xml2sexprdata/xml2sexpr-fv-net-rate.sexpr
 create mode 100644 tests/xml2sexprdata/xml2sexpr-fv-net-rate.xml

-- 
2.1.4

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

[libvirt] [PATCH V2 1/3] xenconfig: support vif bandwidth in sexpr parser and formatter

2016-01-04 Thread Jim Fehlig 
The xen sexpr config format has long supported specifying vif rate
limiting, e.g.

  (device
(vif
  (mac '00:16:3e:1b:b1:47')
  (rate '10240KB/s')
  ...
)
  )

Add support for mapping rate to and from  in the xenconfig
sexpr parser and formatter. rate is mapped to the required 'average'
attribute of the  element, e.g.

  
...

  

  

Also add unit tests to check the conversion logic.

This patch benefits both the old xen driver and the libxl driver.
Both drivers gain support for vif bandwidth when converting to/from
domXML and xen-sxpr. In addition, the old xen driver will now be
able to handle vif 'rate' setting when communicating with xend.

Signed-off-by: Jim Fehlig 
---

I used a bit of code from libxlu_vif.c to implement xenParseSxprVifRate()
instead of using the libxlutil lib directly, since rate limiting applies
to the old xen driver (no libxl) as well.

 src/libvirt_xenconfig.syms  |  1 +
 src/xenconfig/xen_sxpr.c| 74 +
 src/xenconfig/xen_sxpr.h|  2 +
 tests/sexpr2xmldata/sexpr2xml-vif-rate.sexpr| 11 
 tests/sexpr2xmldata/sexpr2xml-vif-rate.xml  | 51 +
 tests/sexpr2xmltest.c   |  2 +
 tests/xml2sexprdata/xml2sexpr-fv-net-rate.sexpr | 10 
 tests/xml2sexprdata/xml2sexpr-fv-net-rate.xml   | 34 
 tests/xml2sexprtest.c   |  1 +
 9 files changed, 186 insertions(+)

diff --git a/src/libvirt_xenconfig.syms b/src/libvirt_xenconfig.syms
index 6541685..b69f2ab 100644
--- a/src/libvirt_xenconfig.syms
+++ b/src/libvirt_xenconfig.syms
@@ -15,6 +15,7 @@ xenParseSxpr;
 xenParseSxprChar;
 xenParseSxprSound;
 xenParseSxprString;
+xenParseSxprVifRate;
 
 # xenconfig/xen_xm.h
 xenFormatXM;
diff --git a/src/xenconfig/xen_sxpr.c b/src/xenconfig/xen_sxpr.c
index d99bac0..9ae50b0 100644
--- a/src/xenconfig/xen_sxpr.c
+++ b/src/xenconfig/xen_sxpr.c
@@ -26,6 +26,8 @@
 
 #include 
 
+#include 
+
 #include "internal.h"
 #include "virerror.h"
 #include "virconf.h"
@@ -315,6 +317,56 @@ xenParseSxprChar(const char *value,
 }
 
 
+static const char *vif_bytes_per_sec_re = "^[0-9]+[GMK]?[Bb]/s$";
+
+int
+xenParseSxprVifRate(const char *rate, unsigned long long *kbytes_per_sec)
+{
+char *trate = NULL;
+char *p;
+regex_t rec;
+char *suffix;
+unsigned long long tmp;
+int ret = -1;
+
+if (VIR_STRDUP(trate, rate) < 0)
+return -1;
+
+p = strchr(trate, '@');
+if (p != NULL)
+*p = 0;
+
+regcomp(, vif_bytes_per_sec_re, REG_EXTENDED|REG_NOSUB);
+if (regexec(, trate, 0, NULL, 0)) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("Invalid rate '%s' specified"), rate);
+goto cleanup;
+}
+
+if (virStrToLong_ull(rate, , 10, )) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("Failed to parse rate '%s'"), rate);
+goto cleanup;
+}
+
+if (*suffix == 'G')
+   tmp *= 1024 * 1024;
+else if (*suffix == 'M')
+   tmp *= 1024;
+
+if (*suffix == 'b' || *(suffix + 1) == 'b')
+   tmp /= 8;
+
+*kbytes_per_sec = tmp;
+ret = 0;
+
+ cleanup:
+regfree();
+VIR_FREE(trate);
+return ret;
+}
+
+
 /**
  * xenParseSxprDisks:
  * @def: the domain config
@@ -594,6 +646,25 @@ xenParseSxprNets(virDomainDefPtr def,
 VIR_STRDUP(net->model, "netfront") < 0)
 goto cleanup;
 
+tmp = sexpr_node(node, "device/vif/rate");
+if (tmp) {
+virNetDevBandwidthPtr bandwidth;
+unsigned long long kbytes_per_sec;
+
+if (xenParseSxprVifRate(tmp, _per_sec) < 0)
+goto cleanup;
+
+if (VIR_ALLOC(bandwidth) < 0)
+goto cleanup;
+if (VIR_ALLOC(bandwidth->out) < 0) {
+VIR_FREE(bandwidth);
+goto cleanup;
+}
+
+bandwidth->out->average = kbytes_per_sec;
+net->bandwidth = bandwidth;
+}
+
 if (VIR_APPEND_ELEMENT(def->nets, def->nnets, net) < 0)
 goto cleanup;
 
@@ -1784,6 +1855,9 @@ xenFormatSxprNet(virConnectPtr conn,
 
 virBufferAsprintf(buf, "(mac '%s')", virMacAddrFormat(>mac, macaddr));
 
+if (def->bandwidth && def->bandwidth->out && def->bandwidth->out->average)
+virBufferAsprintf(buf, "(rate '%lluKB/s')", 
def->bandwidth->out->average);
+
 switch (def->type) {
 case VIR_DOMAIN_NET_TYPE_BRIDGE:
 virBufferEscapeSexpr(buf, "(bridge '%s')", def->data.bridge.brname);
diff --git a/src/xenconfig/xen_sxpr.h b/src/xenconfig/xen_sxpr.h
index a4f4c44..76ff943 100644
--- a/src/xenconfig/xen_sxpr.h
+++ b/src/xenconfig/xen_sxpr.h
@@ -49,6 +49,8 @@ int xenParseSxprSound(virDomainDefPtr def, const char *str);
 
 virDomainChrDefPtr xenParseSxprChar(const char *value,

Re: [libvirt] [PATCH] qemu: save config after pivot only if domain is persistent

2016-01-04 Thread Michael Chapman 

On Mon, 4 Jan 2016, Peter Krempa wrote:

On Thu, Dec 31, 2015 at 16:42:52 +1100, Michael Chapman wrote:

When pivoting after a completed block job, only save the domain's
persistent configuration if the domain is actually marked persistent.

This commit also refactors the logic surrounding the copying of the new
disk definition into vm->newDef to avoid a NULL pointer dereference if
virStorageSourceCopy were to fail to allocate memory.


This commit is fixing two separate things in one commit, which is not
really good practice. Please repost this as two patches.


No problem, I can do that.



Signed-off-by: Michael Chapman 
---
 src/qemu/qemu_blockjob.c | 34 ++
 1 file changed, 14 insertions(+), 20 deletions(-)

diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c
index 1d5b7ce..ae936a2 100644
--- a/src/qemu/qemu_blockjob.c
+++ b/src/qemu/qemu_blockjob.c
@@ -116,26 +116,20 @@ qemuBlockJobEventProcess(virQEMUDriverPtr driver,
 switch ((virConnectDomainEventBlockJobStatus) status) {
 case VIR_DOMAIN_BLOCK_JOB_COMPLETED:
 if (disk->mirrorState == VIR_DOMAIN_DISK_MIRROR_STATE_PIVOT) {
-if (vm->newDef) {
-virStorageSourcePtr copy = NULL;
-
-if ((persistDisk = virDomainDiskByName(vm->newDef,
-   disk->dst, false))) {
-copy = virStorageSourceCopy(disk->mirror, false);
-if (virStorageSourceInitChainElement(copy,
- persistDisk->src,
- true) < 0) {
-VIR_WARN("Unable to update persistent definition "
- "on vm %s after block job",
- vm->def->name);
-virStorageSourceFree(copy);
-copy = NULL;
-persistDisk = NULL;
-}
-}
-if (copy) {
+if (vm->newDef &&
+(persistDisk = virDomainDiskByName(vm->newDef, disk->dst, 
false))) {
+virStorageSourcePtr copy = virStorageSourceCopy(disk->mirror, 
false);
+if (copy && virStorageSourceInitChainElement(copy,
+ persistDisk->src,
+ true) == 0) {a


The new code will result in a line exceeding 80 cols. Since it's not
very long, it's acceptable though.


 virStorageSourceFree(persistDisk->src);
 persistDisk->src = copy;
+} else {
+VIR_WARN("Unable to update persistent definition "
+ "on vm %s after block job",
+ vm->def->name);
+virStorageSourceFree(copy);
+persistDisk = NULL;
 }
 }

@@ -188,8 +182,8 @@ qemuBlockJobEventProcess(virQEMUDriverPtr driver,
 if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0)
 VIR_WARN("Unable to save status on vm %s after block job",
  vm->def->name);
-if (persistDisk && virDomainSaveConfig(cfg->configDir,
-   vm->newDef) < 0)
+if (vm->persistent && persistDisk &&


I'm not quite sure how this would happen. If there isn't a persistent
configuration, how did we get to having the persistDisk object?

If this happened in some way, the problem then is that vm->newDef was
not freed or is present in any way so we should fix the origin and not
this symptom.


I spent a lot of time trying to work this out myself, and although I can 
see what the code is doing I don't really understand the rationale or 
history behind it all.


vm->newDef is supposed to be the "new domain definition to activate on 
shutdown", according to domain_conf.h. What's confusing though is that it 
is possible for this to exist even on transient domains.


qemuProcessInit calls virDomainObjSetDefTransient ("so any part of the 
startup process can add runtime state to vm->def that won't be 
persisted"), and virDomainObjSetDefTransient actually *sets* vm->newDef. 
If the domain is subsequently undefined, vm->persistent is set to 0 but 
vm->newDef is left alone.


So it's possible for vm->newDef to be non-NULL even though 
vm->persistent is 0.


I had initially thought about putting the vm->persistent test at the top 
of this code block, so persistDisk was never set if the domain was 
transient. However the problem with that approach is that it means 
vm->newDef never gets updated at the completion of the block job, yet it's 
still possible to get this "inactive" domain XML via virDomainGetXMLDesc.


I thought it would be simplest and safest to keep updating vm->newDef as 
before,

Re: [libvirt] [PATCH 0/3] Various Valgrind fixes

2016-01-04 Thread Andrea Bolognani 
On Mon, 2016-01-04 at 15:03 +0100, Michal Privoznik wrote:
> 
> > Michael Chapman (3):
> >   qemu: do not copy out non-existent block job info
> >   qemu: do not leak NBD disk data in migration cookie
> >   storage: do not leak storage pool XML filename
> > 
> >  src/qemu/qemu_driver.c   |  4 ++--
> >  src/qemu/qemu_migration.c|  5 +++--
> >  src/storage/storage_driver.c | 10 +-
> >  3 files changed, 10 insertions(+), 9 deletions(-)
> 
> ACKed and pushed.

I just sent out a follow-up to patch 1/3:

  https://www.redhat.com/archives/libvir-list/2016-January/msg00026.html

Cheers.

-- 
Andrea Bolognani
Software Engineer - Virtualization Team

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

[libvirt] [PATCH] qemu: Fix all callers of qemuMonitorGetBlockJobInfo()

2016-01-04 Thread Andrea Bolognani 
Commit 1b43885 modified one of the callers of this function to take
into account the possible return value of 0 when the block job can't be
found.

This commit finishes the job by updating the remaining caller.
---
 src/qemu/qemu_driver.c | 13 ++---
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 304165c..c4573d9 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -16150,14 +16150,13 @@ qemuDomainBlockPivot(virQEMUDriverPtr driver,
 rc = qemuMonitorGetBlockJobInfo(priv->mon, disk->info.alias, );
 if (qemuDomainObjExitMonitor(driver, vm) < 0)
 goto cleanup;
-if (rc < 0)
+if (rc <= 0)
 goto cleanup;
-if (rc == 1 &&
-(info.ready == 1 ||
- (info.ready == -1 &&
-  info.end == info.cur &&
-  (info.type == VIR_DOMAIN_BLOCK_JOB_TYPE_COPY ||
-   info.type == VIR_DOMAIN_BLOCK_JOB_TYPE_COMMIT
+if (info.ready == 1 ||
+(info.ready == -1 &&
+ info.end == info.cur &&
+ (info.type == VIR_DOMAIN_BLOCK_JOB_TYPE_COPY ||
+  info.type == VIR_DOMAIN_BLOCK_JOB_TYPE_COMMIT)))
 disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_READY;
 }
 
-- 
2.5.0

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

Re: [libvirt] [PATCH] virsh: improve waiting for block job readiness

2016-01-04 Thread Andrea Bolognani 
On Thu, 2015-12-31 at 16:34 +1100, Michael Chapman wrote:
> Wait for a block job event after the job has reached 100% only if
> exactly one of the BLOCK_JOB and BLOCK_JOB_2 callbacks were successfully
> registered.
> 
> If neither callback was registered, then no event will ever be received.
> If both were registered, then any user-supplied path is guaranteed to
> match one of the events.
> 
> Signed-off-by: Michael Chapman 
> ---
> 
> I have found that even a 2.5 second timeout isn't always sufficiently
> long for QEMU to flush a disk at the end of a block job.
> 
> I hope I've understood the code properly here, because as far as I can
> tell the comment I'm removing in this commit isn't right. The path the
> user supplies *has* to be either the  or  dev='name'/> exactly in order for the disk to be matched, and these are
> precisely the two strings used by the two events.
> 
>  tools/virsh-domain.c | 24 +---
>  1 file changed, 13 insertions(+), 11 deletions(-)
> 
> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> index edbbc34..60de9ba 100644
> --- a/tools/virsh-domain.c
> +++ b/tools/virsh-domain.c
> @@ -1942,18 +1942,20 @@ virshBlockJobWait(virshBlockJobWaitDataPtr data)
>  goto cleanup;
>  }
>  
> -/* since virsh can't guarantee that the path provided by the user 
> will
> - * later be matched in the event we will need to keep the fallback
> - * approach and claim success if the block job finishes or vanishes. 
> */
> -if (result == 0)
> -break;
> +/* if either event could not be registered we can't guarantee that 
> the
> + * path provided by the user will be matched, so keep the fallback
> + * approach and claim success if the block job finishes or vanishes 
> */
> +if (data->cb_id2 < 0 || data->cb_id2 < 0) {

I'm not going to comment on the rest of the patch, but the condition
above doesn't look like it would make any sense written that way...

> +if (result == 0)
> +break;
>  
> -/* for two-phase jobs we will try to wait in the synchronized phase
> - * for event arrival since 100% completion doesn't necessarily mean 
> that
> - * the block job has finished and can be terminated with success */
> -if (info.end == info.cur && --retries == 0) {
> -ret = VIR_DOMAIN_BLOCK_JOB_READY;
> -goto cleanup;
> +/* only wait in the synchronized phase for event arrival if
> + * either callback was registered */
> +if (info.end == info.cur &&
> +((data->cb_id2 < 0 && data->cb_id2 < 0) || --retries == 0)) {

... and neither does this one.

Am I missing something?

Cheers.

> +ret = VIR_DOMAIN_BLOCK_JOB_READY;
> +goto cleanup;
> +}
>  }
>  
>  if (data->verbose)
-- 
Andrea Bolognani
Software Engineer - Virtualization Team

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

Re: [libvirt] [PATCH 0/3] Various Valgrind fixes

2016-01-04 Thread Michal Privoznik 
On 31.12.2015 07:04, Michael Chapman wrote:
> Valgrind reported a couple of memory leaks and jumps conditional on
> uninitialized values.
> 
> Happy New Year!

To you too!

> 
> Michael Chapman (3):
>   qemu: do not copy out non-existent block job info
>   qemu: do not leak NBD disk data in migration cookie
>   storage: do not leak storage pool XML filename
> 
>  src/qemu/qemu_driver.c   |  4 ++--
>  src/qemu/qemu_migration.c|  5 +++--
>  src/storage/storage_driver.c | 10 +-
>  3 files changed, 10 insertions(+), 9 deletions(-)
> 


ACKed and pushed.

Michal

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

Re: [libvirt] [PATCH 00/10] various test fixes and input devices handling

2016-01-04 Thread Pavel Hrdina 
ping

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

Re: [libvirt] [PATCH] qemu: Fix all callers of qemuMonitorGetBlockJobInfo()

2016-01-04 Thread Andrea Bolognani 
On Mon, 2016-01-04 at 15:46 +0100, Michal Privoznik wrote:
> On 04.01.2016 15:20, Andrea Bolognani wrote:
> > Commit 1b43885 modified one of the callers of this function to take
> > into account the possible return value of 0 when the block job can't be
> > found.
> > 
> > This commit finishes the job by updating the remaining caller.
> > ---
> >  src/qemu/qemu_driver.c | 13 ++---
> >  1 file changed, 6 insertions(+), 7 deletions(-)
> > 
> > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> > index 304165c..c4573d9 100644
> > --- a/src/qemu/qemu_driver.c
> > +++ b/src/qemu/qemu_driver.c
> > @@ -16150,14 +16150,13 @@ qemuDomainBlockPivot(virQEMUDriverPtr driver,
> >  rc = qemuMonitorGetBlockJobInfo(priv->mon, disk->info.alias, 
> >);
> >  if (qemuDomainObjExitMonitor(driver, vm) < 0)
> >  goto cleanup;
> > -if (rc < 0)
> > +if (rc <= 0)
> >  goto cleanup;
> > -if (rc == 1 &&
> > -(info.ready == 1 ||
> > - (info.ready == -1 &&
> > -  info.end == info.cur &&
> > -  (info.type == VIR_DOMAIN_BLOCK_JOB_TYPE_COPY ||
> > -   info.type == VIR_DOMAIN_BLOCK_JOB_TYPE_COMMIT
> > +if (info.ready == 1 ||
> > +(info.ready == -1 &&
> > + info.end == info.cur &&
> > + (info.type == VIR_DOMAIN_BLOCK_JOB_TYPE_COPY ||
> > +  info.type == VIR_DOMAIN_BLOCK_JOB_TYPE_COMMIT)))
> >  disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_READY;
> >  }
> 
> Interesting. So previously this code worked just right with no block
> operation running on @disk. Now it will fail. I guess that's correct
> approach since this function's job is to abort a block job.
> 
> ACK

I'm actually having second thoughts about this.

We only call qemuMonitorGetBlockJobInfo() if !disk->mirrorState.
However, having it return 0 before would skip reading from info (my
main concern, as it would not have been filled in) and cause the
check for

  disk->mirrorState != VIR_DOMAIN_DISK_MIRROR_STATE_READY

immediately afterwards to fail and an error to be raised before
jumping to cleanup.

After my patch, the function will jump to cleanup earlier, still
returning a negative error code, but not raising any error in the
process.

I guess what I'm trying to say is that, unless you can convince
otherwise, I'm going to have to SNACK this one :)

Cheers.

-- 
Andrea Bolognani
Software Engineer - Virtualization Team

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

Re: [libvirt] [PATCH] rbd: Return VIR_STORAGE_FILE_RAW as format for RBD volumes

2016-01-04 Thread Michal Privoznik 
On 24.12.2015 13:34, Wido den Hollander wrote:
> This used to return 'unkown' and that was not correct.
> 
> A vol-dumpxml now returns:
> 
> 
>   image3
>   libvirt/image3
>   
>   
>   10737418240
>   10737418240
>   
> libvirt/image3
> 
>   
> 
> 
> The RBD driver will now error out if a different format than RAW
> is provided when creating a volume.
> 
> Signed-off-by: Wido den Hollander 
> ---
>  src/conf/storage_conf.c   |  3 ++-
>  src/storage/storage_backend_rbd.c | 13 +
>  2 files changed, 15 insertions(+), 1 deletion(-)
> 


ACKed and pushed.

Michal

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

Re: [libvirt] [PATCH] qemu: Fix all callers of qemuMonitorGetBlockJobInfo()

2016-01-04 Thread Michal Privoznik 
On 04.01.2016 15:20, Andrea Bolognani wrote:
> Commit 1b43885 modified one of the callers of this function to take
> into account the possible return value of 0 when the block job can't be
> found.
> 
> This commit finishes the job by updating the remaining caller.
> ---
>  src/qemu/qemu_driver.c | 13 ++---
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 304165c..c4573d9 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -16150,14 +16150,13 @@ qemuDomainBlockPivot(virQEMUDriverPtr driver,
>  rc = qemuMonitorGetBlockJobInfo(priv->mon, disk->info.alias, );
>  if (qemuDomainObjExitMonitor(driver, vm) < 0)
>  goto cleanup;
> -if (rc < 0)
> +if (rc <= 0)
>  goto cleanup;
> -if (rc == 1 &&
> -(info.ready == 1 ||
> - (info.ready == -1 &&
> -  info.end == info.cur &&
> -  (info.type == VIR_DOMAIN_BLOCK_JOB_TYPE_COPY ||
> -   info.type == VIR_DOMAIN_BLOCK_JOB_TYPE_COMMIT
> +if (info.ready == 1 ||
> +(info.ready == -1 &&
> + info.end == info.cur &&
> + (info.type == VIR_DOMAIN_BLOCK_JOB_TYPE_COPY ||
> +  info.type == VIR_DOMAIN_BLOCK_JOB_TYPE_COMMIT)))
>  disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_READY;
>  }
>  
> 

Interesting. So previously this code worked just right with no block
operation running on @disk. Now it will fail. I guess that's correct
approach since this function's job is to abort a block job.

ACK

Michal

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

[libvirt] [PATCH] qemu: Fix return value of qemuDomainGetBlockJobInfo

2016-01-04 Thread Michal Privoznik 
While reviewing 1b43885d1784640 I've noticed a virReportError()
followed by a goto endjob; without setting the correct return
value. Problem is, if block job is so fast that it's bandwidth
does not fit into ulong, an error is reported. However, by that
time @ret is already set to 1 which means success. Since the
scenario can be hardly considered successful, we should return a
value meaning error.

Signed-off-by: Michal Privoznik 
---
 src/qemu/qemu_driver.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 304165c..1161aa0 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -16546,6 +16546,7 @@ qemuDomainGetBlockJobInfo(virDomainPtr dom,
 virReportError(VIR_ERR_OVERFLOW,
_("bandwidth %llu cannot be represented in result"),
rawInfo.bandwidth);
+ret = -1;
 goto endjob;
 }
 
-- 
2.4.10

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

Re: [libvirt] [PATCH] qemu: Fix all callers of qemuMonitorGetBlockJobInfo()

2016-01-04 Thread Peter Krempa 
On Mon, Jan 04, 2016 at 16:10:56 +0100, Andrea Bolognani wrote:
> On Mon, 2016-01-04 at 15:46 +0100, Michal Privoznik wrote:
> > On 04.01.2016 15:20, Andrea Bolognani wrote:
> > > Commit 1b43885 modified one of the callers of this function to take
> > > into account the possible return value of 0 when the block job can't be
> > > found.
> > > 
> > > This commit finishes the job by updating the remaining caller.
> > > ---
> > >  src/qemu/qemu_driver.c | 13 ++---
> > >  1 file changed, 6 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> > > index 304165c..c4573d9 100644
> > > --- a/src/qemu/qemu_driver.c
> > > +++ b/src/qemu/qemu_driver.c
> > > @@ -16150,14 +16150,13 @@ qemuDomainBlockPivot(virQEMUDriverPtr driver,
> > >  rc = qemuMonitorGetBlockJobInfo(priv->mon, disk->info.alias, 
> > >);
> > >  if (qemuDomainObjExitMonitor(driver, vm) < 0)
> > >  goto cleanup;
> > > -if (rc < 0)
> > > +if (rc <= 0)
> > >  goto cleanup;
> > > -if (rc == 1 &&
> > > -(info.ready == 1 ||
> > > - (info.ready == -1 &&
> > > -  info.end == info.cur &&
> > > -  (info.type == VIR_DOMAIN_BLOCK_JOB_TYPE_COPY ||
> > > -   info.type == VIR_DOMAIN_BLOCK_JOB_TYPE_COMMIT
> > > +if (info.ready == 1 ||
> > > +(info.ready == -1 &&
> > > + info.end == info.cur &&
> > > + (info.type == VIR_DOMAIN_BLOCK_JOB_TYPE_COPY ||
> > > +  info.type == VIR_DOMAIN_BLOCK_JOB_TYPE_COMMIT)))
> > >  disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_READY;
> > >  }
> > 
> > Interesting. So previously this code worked just right with no block
> > operation running on @disk. Now it will fail. I guess that's correct
> > approach since this function's job is to abort a block job.
> > 
> > ACK
> 
> I'm actually having second thoughts about this.
> 
> We only call qemuMonitorGetBlockJobInfo() if !disk->mirrorState.
> However, having it return 0 before would skip reading from info (my
> main concern, as it would not have been filled in) and cause the
> check for
> 
>   disk->mirrorState != VIR_DOMAIN_DISK_MIRROR_STATE_READY
> 
> immediately afterwards to fail and an error to be raised before
> jumping to cleanup.
> 
> After my patch, the function will jump to cleanup earlier, still
> returning a negative error code, but not raising any error in the
> process.
> 
> I guess what I'm trying to say is that, unless you can convince
> otherwise, I'm going to have to SNACK this one :)

Wise choice, this would indeed break the error reporting in case when
the mirror dies for some reason. The timing for that to happen would
need to be really unfortunate though since otherwise the mirror job
would be removed by the block job event callback after receiving the
failure event.

Thanks for not breaking it.

Peter


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

Re: [libvirt] [PATCH] qemu: Fix return value of qemuDomainGetBlockJobInfo

2016-01-04 Thread Peter Krempa 
On Mon, Jan 04, 2016 at 15:53:32 +0100, Michal Privoznik wrote:
> While reviewing 1b43885d1784640 I've noticed a virReportError()
> followed by a goto endjob; without setting the correct return
> value. Problem is, if block job is so fast that it's bandwidth
> does not fit into ulong, an error is reported. However, by that
> time @ret is already set to 1 which means success. Since the
> scenario can be hardly considered successful, we should return a
> value meaning error.

I'd prefer if you dropped the beginning of the commit message describing
how you located this bug, just describing the bug itself.

> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/qemu/qemu_driver.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 304165c..1161aa0 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -16546,6 +16546,7 @@ qemuDomainGetBlockJobInfo(virDomainPtr dom,
>  virReportError(VIR_ERR_OVERFLOW,
> _("bandwidth %llu cannot be represented in result"),
> rawInfo.bandwidth);
> +ret = -1;
>  goto endjob;

ACK,

Peter


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

Re: [libvirt] [PATCH] virsh: improve waiting for block job readiness

2016-01-04 Thread Peter Krempa 
On Thu, Dec 31, 2015 at 16:34:49 +1100, Michael Chapman wrote:
> Wait for a block job event after the job has reached 100% only if
> exactly one of the BLOCK_JOB and BLOCK_JOB_2 callbacks were successfully
> registered.
> 
> If neither callback was registered, then no event will ever be received.
> If both were registered, then any user-supplied path is guaranteed to
> match one of the events.
> 
> Signed-off-by: Michael Chapman 
> ---
> 
> I have found that even a 2.5 second timeout isn't always sufficiently
> long for QEMU to flush a disk at the end of a block job.
> 
> I hope I've understood the code properly here, because as far as I can
> tell the comment I'm removing in this commit isn't right. The path the
> user supplies *has* to be either the  or  dev='name'/> exactly in order for the disk to be matched, and these are
> precisely the two strings used by the two events.
> 
>  tools/virsh-domain.c | 24 +---
>  1 file changed, 13 insertions(+), 11 deletions(-)

In addition to Andrea's review:

> 
> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> index edbbc34..60de9ba 100644
> --- a/tools/virsh-domain.c
> +++ b/tools/virsh-domain.c
> @@ -1942,18 +1942,20 @@ virshBlockJobWait(virshBlockJobWaitDataPtr data)
>  goto cleanup;
>  }
>  
> -/* since virsh can't guarantee that the path provided by the user 
> will
> - * later be matched in the event we will need to keep the fallback
> - * approach and claim success if the block job finishes or vanishes. 
> */
> -if (result == 0)
> -break;
> +/* if either event could not be registered we can't guarantee that 
> the
> + * path provided by the user will be matched, so keep the fallback
> + * approach and claim success if the block job finishes or vanishes 
> */

The new statement is not true. If the user provides a filesystem path
different from what is in the definition but matching the same volume [1]
the callback will still return the path present in the configuration and
thus might not ever match and the job would hang forever.

[1] e.g. /path/to/image and /path/to/../to/image are equivalent but
won't be equal using strcmp. The problem is even more prominent if you
mix in some symlinks.

> +if (data->cb_id2 < 0 || data->cb_id2 < 0) {
> +if (result == 0)
> +break;
>  
> -/* for two-phase jobs we will try to wait in the synchronized phase
> - * for event arrival since 100% completion doesn't necessarily mean 
> that
> - * the block job has finished and can be terminated with success */
> -if (info.end == info.cur && --retries == 0) {
> -ret = VIR_DOMAIN_BLOCK_JOB_READY;
> -goto cleanup;
> +/* only wait in the synchronized phase for event arrival if
> + * either callback was registered */
> +if (info.end == info.cur &&
> +((data->cb_id2 < 0 && data->cb_id2 < 0) || --retries == 0)) {
> +ret = VIR_DOMAIN_BLOCK_JOB_READY;
> +goto cleanup;
> +}
>  }

NACK to this approach, if there was a way how this ugly stuff could be
avoided or deleted I'd already do so.

Peter


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

Re: [libvirt] [PATCH] qemu: save config after pivot only if domain is persistent

2016-01-04 Thread Peter Krempa 
On Thu, Dec 31, 2015 at 16:42:52 +1100, Michael Chapman wrote:
> When pivoting after a completed block job, only save the domain's
> persistent configuration if the domain is actually marked persistent.
> 
> This commit also refactors the logic surrounding the copying of the new
> disk definition into vm->newDef to avoid a NULL pointer dereference if
> virStorageSourceCopy were to fail to allocate memory.

This commit is fixing two separate things in one commit, which is not
really good practice. Please repost this as two patches.



> 
> Signed-off-by: Michael Chapman 
> ---
>  src/qemu/qemu_blockjob.c | 34 ++
>  1 file changed, 14 insertions(+), 20 deletions(-)
> 
> diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c
> index 1d5b7ce..ae936a2 100644
> --- a/src/qemu/qemu_blockjob.c
> +++ b/src/qemu/qemu_blockjob.c
> @@ -116,26 +116,20 @@ qemuBlockJobEventProcess(virQEMUDriverPtr driver,
>  switch ((virConnectDomainEventBlockJobStatus) status) {
>  case VIR_DOMAIN_BLOCK_JOB_COMPLETED:
>  if (disk->mirrorState == VIR_DOMAIN_DISK_MIRROR_STATE_PIVOT) {
> -if (vm->newDef) {
> -virStorageSourcePtr copy = NULL;
> -
> -if ((persistDisk = virDomainDiskByName(vm->newDef,
> -   disk->dst, false))) {
> -copy = virStorageSourceCopy(disk->mirror, false);
> -if (virStorageSourceInitChainElement(copy,
> - persistDisk->src,
> - true) < 0) {
> -VIR_WARN("Unable to update persistent definition "
> - "on vm %s after block job",
> - vm->def->name);
> -virStorageSourceFree(copy);
> -copy = NULL;
> -persistDisk = NULL;
> -}
> -}
> -if (copy) {
> +if (vm->newDef &&
> +(persistDisk = virDomainDiskByName(vm->newDef, disk->dst, 
> false))) {
> +virStorageSourcePtr copy = 
> virStorageSourceCopy(disk->mirror, false);
> +if (copy && virStorageSourceInitChainElement(copy,
> + 
> persistDisk->src,
> + true) == 0) {a

The new code will result in a line exceeding 80 cols. Since it's not
very long, it's acceptable though.

>  virStorageSourceFree(persistDisk->src);
>  persistDisk->src = copy;
> +} else {
> +VIR_WARN("Unable to update persistent definition "
> + "on vm %s after block job",
> + vm->def->name);
> +virStorageSourceFree(copy);
> +persistDisk = NULL;
>  }
>  }
>  
> @@ -188,8 +182,8 @@ qemuBlockJobEventProcess(virQEMUDriverPtr driver,
>  if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0)
>  VIR_WARN("Unable to save status on vm %s after block job",
>   vm->def->name);
> -if (persistDisk && virDomainSaveConfig(cfg->configDir,
> -   vm->newDef) < 0)
> +if (vm->persistent && persistDisk &&

I'm not quite sure how this would happen. If there isn't a persistent
configuration, how did we get to having the persistDisk object?

If this happened in some way, the problem then is that vm->newDef was
not freed or is present in any way so we should fix the origin and not
this symptom.

> +virDomainSaveConfig(cfg->configDir, vm->newDef) < 0)
>  VIR_WARN("Unable to update persistent definition on vm %s "
>   "after block job", vm->def->name);
>  }

Peter


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

Re: [libvirt] [PATCH v2] qemu: Connect to guest agent iff needed

2016-01-04 Thread Peter Krempa 
On Tue, Dec 22, 2015 at 15:41:16 +0100, Michal Privoznik wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1293351
> 
> I've came across a bit of a silly scenario, but long story short:
> after domain was resumed, the virDomainSetTime() API hung for 5
> seconds after which it failed with an error. Problem was, that
> there is no guest agent installed in the domain. But this got me
> thinking and digging into the code. So even though we do listen
> to VSERPORT_CHANGE events and connect and disconnect ourselves to
> guest agent, we still do connect to the guest agent at both
> domain startup and resume. This is a bit silly as it produces the
> delay - basically, because we connect to the guest agent,
> priv->agent is not NULL. Therefore qemuDomainAgentAvailable()
> will return true. And the place where we hang? Well,
> historically, when there was no VSERPORT_CHANGE event, we used a
> dummy ping command to the guest which has 5 seconds timeout. And
> it's still there and effective. So there's where the delay comes
> from.
> What's the resolution? Well, I'm introducing new capability
> QEMU_CAPS_VSERPORT_CHANGE that is enabled iff qemu is capable of
> the event. And, during domain startup, reconnect after resume and
> attach we do connect to the agent socket iff the capability is
> NOT set.

I'd welcome a less verbose commit message containig the technical
details and omitting the story around.

> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/qemu/qemu_capabilities.c |  2 ++
>  src/qemu/qemu_capabilities.h |  1 +
>  src/qemu/qemu_domain.c   | 11 ---
>  src/qemu/qemu_domain.h   |  2 +-
>  src/qemu/qemu_driver.c   |  2 +-
>  src/qemu/qemu_process.c  | 25 +++--
>  src/qemu/qemu_process.h  |  4 +++-
>  tests/qemucapabilitiesdata/caps_2.1.1-1.caps |  1 +
>  tests/qemucapabilitiesdata/caps_2.4.0-1.caps |  1 +
>  tests/qemucapabilitiesdata/caps_2.5.0-1.caps |  1 +
>  10 files changed, 34 insertions(+), 16 deletions(-)
> 

[...]

> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index f274068..3aca227 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -198,16 +198,29 @@ static qemuAgentCallbacks agentCallbacks = {
>  
>  
>  int
> -qemuConnectAgent(virQEMUDriverPtr driver, virDomainObjPtr vm)
> +qemuConnectAgent(virQEMUDriverPtr driver,
> + virDomainObjPtr vm,
> + bool force)
>  {
>  qemuDomainObjPrivatePtr priv = vm->privateData;
>  int ret = -1;
>  qemuAgentPtr agent = NULL;
> -virDomainChrSourceDefPtr config = qemuFindAgentConfig(vm->def);
> +virDomainChrDefPtr config = qemuFindAgentConfig(vm->def);
>  
>  if (!config)
>  return 0;
>  
> +if (!force &&
> +virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_VSERPORT_CHANGE) &&

AFAIK the capability bit check won't be necessary here, since the
event was introduced precisely at the same time as the ability to query
the state via monitor and thus config->state will be _DISCONNECTED only
if that exists.

> +config->state == VIR_DOMAIN_CHR_DEVICE_STATE_DISCONNECTED) {
> +/* With newer qemus capable of VSERPORT_CHANGE event, we are 
> connecting and
> + * disconnecting to the guest agent as it connects or disconnects to 
> the
> + * channel within the guest. So, it's important to connect here iff 
> we are
> + * running qemu not capable of the event or we are called from the 
> event
> + * callback (@force == true). */

So basically this gets called with @force true just from the callback.
But in the callback you call this function if and only if the serial
port state is _CONNECTED. All other callers call this with unknown port
state. The @force argument thus doesn't make much sense.

> +return 0;
> +}
> +
>  if (virSecurityManagerSetDaemonSocketLabel(driver->securityManager,
> vm->def) < 0) {
>  VIR_ERROR(_("Failed to set security context for agent for %s"),

Peter


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