[libvirt] [PATCH] Use sys/uio.h for writev()

[Daniel P. Berrange]

With glibc >= 2.25.90 writev() is only available if you explicitly
include sys/uio.h

Signed-off-by: Daniel P. Berrange 
---

A build breaker, but not yet pushed in case anyone has opinions...

 configure.ac  | 2 +-
 src/util/virlog.c | 3 +++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/configure.ac b/configure.ac
index 1a73b34..1a48bb0 100644
--- a/configure.ac
+++ b/configure.ac
@@ -328,7 +328,7 @@ dnl Availability of various common headers (non-fatal if 
missing).
 AC_CHECK_HEADERS([pwd.h regex.h sys/un.h \
   sys/poll.h syslog.h mntent.h net/ethernet.h linux/magic.h \
   sys/un.h sys/syscall.h sys/sysctl.h netinet/tcp.h ifaddrs.h \
-  libtasn1.h sys/ucred.h sys/mount.h stdarg.h])
+  libtasn1.h sys/ucred.h sys/mount.h stdarg.h sys/uio.h])
 dnl Check whether endian provides handy macros.
 AC_CHECK_DECLS([htole64], [], [], [[#include ]])
 AC_CHECK_FUNCS([stat stat64 __xstat __xstat64 lstat lstat64 __lxstat 
__lxstat64])
diff --git a/src/util/virlog.c b/src/util/virlog.c
index 7933e1a..3a28e5e 100644
--- a/src/util/virlog.c
+++ b/src/util/virlog.c
@@ -32,6 +32,9 @@
 #include 
 #include 
 #include 
+#if HAVE_SYS_UIO_H
+# include 
+#endif
 #if HAVE_SYSLOG_H
 # include 
 #endif
-- 
2.9.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

[Daniel P. Berrange]

On Wed, Jun 14, 2017 at 03:27:25PM +0200, Martin Kletzander wrote:
> On Wed, Jun 14, 2017 at 11:20:44AM +0100, Daniel P. Berrange wrote:
> > This fixes an incompatibility with glibc 2.25.90
> > 
> > Signed-off-by: Daniel P. Berrange 
> > ---
> > 
> > Pushed as a broken build fix to get CI back online
> > 
> 
> After this update the build fails for me with gcc-7.1.0 with the
> following error:
> 
> In file included from util/virobject.c:28:0:
> util/virobject.c: In function 'virClassNew':
> util/viratomic.h:176:46: error: this condition has identical branches 
> [-Werror=duplicated-branches]
> (void)(0 ? *(atomic) ^ *(atomic) : 0);  \
>  ^
> util/virobject.c:144:20: note: in expansion of macro 'virAtomicIntInc'
> klass->magic = virAtomicIntInc(&magicCounter);
>^~~
> 
> Does that mean that gcc does optimize our prefetch trick away
> (considering I understood what that line is trying to do)?  Or should we
> just turn the warning off for that header file?

Yep, "-Wduplicated-branches" appears to be a new warning flag in gcc 7.1
which gnulib turns on. There's a similar hit with mingw

../../src/util/vircommand.c: In function 'virCommandWait':
../../src/util/vircommand.c:2562:51: error: this condition has identical 
branches [-Werror=duplicated-branches]
 *exitstatus = cmd->rawStatus ? status : WEXITSTATUS(status);
   ^
cc1: all warnings being treated as errors


because WEXITSTATUS(x) expands to 'x' on Win32.

We could use a pragma to turn off selectively, but I'm more
inclined to just disable this new warning flag. 




Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

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

[Daniel P. Berrange]

This fixes an incompatibility with glibc 2.25.90

Signed-off-by: Daniel P. Berrange 
---

Pushed as a broken build fix to get CI back online

 .gnulib | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/.gnulib b/.gnulib
index da830b5..ce4ee4c 16
--- a/.gnulib
+++ b/.gnulib
@@ -1 +1 @@
-Subproject commit da830b5146cb553ac2a4bcfe76caeb57bda24cc3
+Subproject commit ce4ee4cbb596a9d7de2786cf8c48cf62a4edede7
-- 
2.9.3

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

Re: [libvirt] [PATCH go-xml] 1,rename DomainInterfaceBoot to DomainDeviceBoot; 2,support disk boot order

[Daniel P. Berrange]

On Tue, Jun 06, 2017 at 01:51:30PM +0800, zhenwei.pi wrote:
> ---
>  domain.go  | 5 +++--
>  domain_test.go | 6 +-
>  2 files changed, 8 insertions(+), 3 deletions(-)

ACK and pushed with some changes to the commit message text.

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

Re: [libvirt] libvirt binding for Rust

[Daniel P. Berrange]

On Tue, Jun 06, 2017 at 10:31:33PM +0100, Daniel P. Berrange wrote:
> On Tue, Jun 06, 2017 at 12:19:21PM +0200, Sahid Orentino Ferdjaoui wrote:
> > Hello,
> > 
> > I started a work on a libvirt binding for Rust [0]. Not all of the API
> > is implemeted but I think it's now in a usable state.
> > 
> >   https://docs.rs/crate/virt
> >   https://github.com/sahid/libvirt-rs
> > 
> > The code is licensed under LGPL-2.1, it's tested by a CI running
> > libvirt 1.2.0, 2.5.0, 3.3.0. There are unit tests, integration tests
> > and some examples to exercise the code (some parts are still not
> > covered) I also have checked the code with valgrind to avoid any
> > memory leaks.
> > 
> >   https://travis-ci.org/sahid/libvirt-rs
> > 
> > Do you think that we can consider it ready to be hosted by libvirt.org
> > GIT server ? That could help to get new contributors also interested
> > by Rust and improve the code and coverage of the API.
> 
> Given the wide range of other language bindings we are already hosting,
> I'd be happy to see the Rust binding added there too.

I forgot to say, please send me your ssh public key off list, and assuming
no one objects, I'll set up the git hosting for this.

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

Re: [libvirt] Remotable Libvirt

[Daniel P. Berrange]

On Wed, Jun 07, 2017 at 10:47:25AM +0200, Stef Walter wrote:
> On 07.06.2017 07:49, Martin Pitt wrote:
> > Hello Richard,
> > 
> > Richard W.M. Jones [2017-05-31 18:00 +0100]:
> >> I agree with others that as things stand you will need a REST or DBus
> >> or similar API added to libvirt.
> >>
> >> However have you considered using gobject-introspection to generate
> >> new "Payload" types automatically?
> > 
> > This doesn't fundamentally change the picture here. Our JavaScript runs in 
> > the
> > browser, so on that side GI doesn't help. What we could do is to dynamically
> > create some code in the Cockpit module, send it over to the remote machine, 
> > and
> > have it be executed. This could be in Python, which then assumes that
> > python-libvirt is installed there, or in JS which then assumes that
> > libvirt-glib and gjs are installed. The latter seems like a stronger
> > assumption on a server even, but structurally these are pretty similiar.
> > 
> > I. e. GI is not a magic thing to make an API remotable, it just makes it
> > bindable to different programming languages.
> > 
> > C/GI interfaces also don't map well to D-Bus, i. e. it's not practical to
> > autogenerate a D-Bus interface for a given GI API. This still works for the
> > most simple methods that only accept primitive data types and strings, but 
> > as
> > soon as you pass any structs, pointers, function pointers etc. around this
> > can't be exposed though D-Bus any more without further interpretation on the
> > server side.
> 
> Perhaps not an runtime. But it seems like such a thing could be done at
> compile time for the libvirt C API. The libvirt folks do it for their
> python bindings, using code generation.
> 
> The libvirt Python API behaves a lot like the C API. It's very flat and
> not very "pythonic". Which is actually a nice target for us. We could do
> something similar with a simple "flat" DBus wrapper of the API ... using
> code generation.

I see no problem with mapping almost all of the libvirt API into DBus, as
the DBus method call + signal concepts are very similar to the libvirt RPC
call and event concepts.  The place where you would have trouble mapping
to DBus is the stream support as that has no equivalent in DBus, and there's
no efficient workaround to deal with it that I can imagine. 

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

Re: [libvirt] More CI options

[Daniel P. Berrange]

On Wed, Jun 07, 2017 at 10:55:21AM +0200, Martin Kletzander wrote:
> On Tue, Jun 06, 2017 at 10:30:16PM +0100, Daniel P. Berrange wrote:
> > On Tue, Jun 06, 2017 at 10:09:00PM +0200, Martin Kletzander wrote:
> > > Since the addition of Travis CI builds, there is some more progress
> > > towards more testing.  I was just wondering if anyone was thinking about
> > > (or is already working on) some settings for other CI environments as
> > > well.  For example if configured AppVeyor, we could have MSVC builds as
> > > well.  And there are more of those, I didn't compare all of them, but
> > > the more environments we have, the more build failures we might catch.
> > > Or even test failures, maybe.
> > 
> > FWIW, I would consider building with MSVC a non-goal unless its feature
> > set has massively improved to support the various GCC extensions we rely
> > on. I've always assumed that only target compilers which support GCC C
> > dialect, which basically means GCC or CLang or Intel CC and not MSVC.
> > 
> 
> Apparently, the Appveyor also supports building with MinGW, MSYS2 and
> others.  I mentioned MSVC because I've seen some configure code that
> handles the option.  But what would be nicer is 'any' Windows
> compilation, but we can still do it in Fedora, too.

We already have MinGW builds done in ci.centos.org.  IIUC, MSYS2 is based
on cygwin. At one point someone did some work to make libvirt build on
cygwin, but it was never touched again thereafter, so currently I'd put
that in the unsupported bucket. Thus I'd ignore MSYS2 too unless someone
actually wants to use it, and is also willing to make sure it works on an
ongoing basis

> > Aside from that, I'd be happy to see addition of more CI environments if
> > they cover platforms / combinations we don't already touch. In particular
> > I think it would be useful to see some non-x86 architectures covered.
> > 
> 
> Claudio mentioned the same, but it looks like the hardware is the issue.
> For a simple start, I can run something at home on a aarch64, but that's
> a cheap board, so until it gets proper support in mainline kernel, it's
> not very helpful.  I don't have much spare time on my hands, so I won't
> promise anything, but I might try giving that a spin, just for the fun
> of it.

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

Re: [libvirt] libvirt binding for Rust

[Daniel P. Berrange]

On Tue, Jun 06, 2017 at 12:19:21PM +0200, Sahid Orentino Ferdjaoui wrote:
> Hello,
> 
> I started a work on a libvirt binding for Rust [0]. Not all of the API
> is implemeted but I think it's now in a usable state.
> 
>   https://docs.rs/crate/virt
>   https://github.com/sahid/libvirt-rs
> 
> The code is licensed under LGPL-2.1, it's tested by a CI running
> libvirt 1.2.0, 2.5.0, 3.3.0. There are unit tests, integration tests
> and some examples to exercise the code (some parts are still not
> covered) I also have checked the code with valgrind to avoid any
> memory leaks.
> 
>   https://travis-ci.org/sahid/libvirt-rs
> 
> Do you think that we can consider it ready to be hosted by libvirt.org
> GIT server ? That could help to get new contributors also interested
> by Rust and improve the code and coverage of the API.

Given the wide range of other language bindings we are already hosting,
I'd be happy to see the Rust binding added there too.

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

Re: [libvirt] More CI options

[Daniel P. Berrange]

On Tue, Jun 06, 2017 at 10:09:00PM +0200, Martin Kletzander wrote:
> Since the addition of Travis CI builds, there is some more progress
> towards more testing.  I was just wondering if anyone was thinking about
> (or is already working on) some settings for other CI environments as
> well.  For example if configured AppVeyor, we could have MSVC builds as
> well.  And there are more of those, I didn't compare all of them, but
> the more environments we have, the more build failures we might catch.
> Or even test failures, maybe.

FWIW, I would consider building with MSVC a non-goal unless its feature
set has massively improved to support the various GCC extensions we rely
on. I've always assumed that only target compilers which support GCC C
dialect, which basically means GCC or CLang or Intel CC and not MSVC.

Aside from that, I'd be happy to see addition of more CI environments if
they cover platforms / combinations we don't already touch. In particular
I think it would be useful to see some non-x86 architectures covered.

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

Re: [libvirt] Remotable Libvirt

[Daniel P. Berrange]

On Tue, Jun 06, 2017 at 01:25:08PM -0400, Stef Walter wrote:
> On 06.06.2017 12:13, Daniel P. Berrange wrote:
> > On Tue, Jun 06, 2017 at 06:07:21PM +0200, Stef Walter wrote:
> >> So to summarize what I'm hearing here:
> >>
> >>  * There is no libvirt remotable API today.
> > 
> > Libvirt drivers that are implemented in libvirtd are accessible remotely
> > using the libvirt client library. We just consider the wire protocol a
> > private impl detail between them.  Drivers implemented outside libvirtd
> > are often accessible remotely too, via the hypervisor's native remoting
> > system. 
> 
> I believe those are implementation details, not and remoteable API.
> Libvirt and its API must always be used in process today? Even though it
> RPC may be in play, it is an implementation detail, not API. Correct?

I don't really know what distinction your trying to make by saying
"used in process". Libvirt provides an ELF library and apps link to
it to use it, or use it indirectly via a higher level language specific
wrapper.

> >>  * All libvirt APIs are only able to be used in-process.
> >>  * There is presently no ability for a libvirt API to be used from a
> >>browser, or non-Linux process or non-Linux system.
> > 
> > Actually libvirt can be built on OS-X, Windows and FreeBSD too.
> 
> Ah good to know. But the API is always used in process, correct?

Again I don't know what you're trying to say here. It is an ELF
library and you link to it and call APIs as you would for any
other library.

> >> In order for Cockpit to interact with libvirt, it would need to grow a
> >> remoteable API. The Cockpit project could likely jumpstart such an
> >> effort, by lightly wrapping the C API in DBus ... and contribute that
> >> remoteable API code to the libvirt project.
> > 
> > FWIW, if someone does want to build a DBus wrapper around libvirt, that
> > would be something todo as a separate code module above the libvirt
> > API. We would be happy to host such an effort on libvirt.org git though,
> > alongside other higher level API wrappers. Likewise for a REST API wrapper.
> 
> This would be a useful division of labor between Cockpit consuming such
> a remotable API  ... and the remoteable API being hosted and maintained
> by the libvirt folks ... although the Cockpit project could help get it
> started.

FWIW, in our RPC system, we auto-generate some of the marshalling code
from the API description XML file in /usr/share/libvirt/libvirt-api.xml
Some APIs are not friendly for auto-generating though, so probably get
about 75% auto-generated and the rest need to be done by hand.

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

Re: [libvirt] [PATCH go-xml] add emulator supported

[Daniel P. Berrange]

On Sat, Jun 03, 2017 at 09:30:00AM +0800, zhenwei.pi wrote:
> ---
>  domain.go  | 1 +
>  domain_test.go | 6 ++
>  2 files changed, 7 insertions(+)
> 
> diff --git a/domain.go b/domain.go
> index dcb8f65..723e761 100644
> --- a/domain.go
> +++ b/domain.go
> @@ -307,6 +307,7 @@ type DomainDeviceList struct {
>   Channels[]DomainChardev`xml:"channel"`
>   MemBalloon  *DomainMemBalloon  `xml:"memballoon"`
>   Sounds  []DomainSound  `xml:"sound"`
> + Emulatorstring `xml:"emulator,omitempty"`
>  }
>  
>  type DomainMemory struct {
> diff --git a/domain_test.go b/domain_test.go
> index 632b714..9efbd6b 100644
> --- a/domain_test.go
> +++ b/domain_test.go
> @@ -572,6 +572,9 @@ var domainTestData = []struct {
>   DomainCPUFeature{Policy: "disable", 
> Name: "lahf_lm"},
>   },
>   },
> + Devices: &DomainDeviceList{
> + Emulator: "/bin/qemu-kvm",
> + },
>   },
>   Expected: []string{
>   ``,
> @@ -582,6 +585,9 @@ var domainTestData = []struct {
>   ` threads="1">`,
>   ` name="lahf_lm">`,
>   `  `,
> + `  `,
> + `/bin/qemu-kvm`,
> + `  `,
>   ``,
>   },
>   },

Thanks, I've pushed this to git with just one small change - i listed
'Emulator' as the first thing in the DomainDeviceList struct to match
ordering libvirt uses.

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

Re: [libvirt] Remotable Libvirt

[Daniel P. Berrange]

On Tue, Jun 06, 2017 at 06:07:21PM +0200, Stef Walter wrote:
> On 06.06.2017 17:17, Daniel P. Berrange wrote:
> > On Tue, Jun 06, 2017 at 05:11:55PM +0200, Peter Krempa wrote:
> >> On Tue, Jun 06, 2017 at 07:45:10 -0700, Peter wrote:
> >>> On 05/26/2017 02:11 AM, Martin Kletzander wrote:
> >>>> On Thu, May 25, 2017 at 10:16:26AM -0700, Peter Volpe wrote:
> >>
> >> [...]
> >>
> >>>> If we standardize even the smallest part of the RPC, then it might screw
> >>>> us immediately.  We are keeping it private just so we can enhance the
> >>>> APIs we already have.  We don't know when we will need to change some
> >>>> part of it.
> >>>>
> >>>
> >>> How does the current ssh implementation work?
> >>> (https://libvirt.org/remote.html) If clients are able to talk to a remote
> >>> libvirtd via ssh then there must be some sort of compatibility guarantee.
> >>> Otherwise unless the versions are exactly the same clients wouldn't be 
> >>> able
> >>> to talk to remote daemons.
> >>
> >> They use the internal RPC protocol transported over SSH. The client
> >> library initializes the ssh connection to the server, and then starts to
> >> talk the RPC tunelled over the SSH session.
> >>
> >> The compatibility is guaranteed only if you use the client library. As
> >> said, the RPC protocol is considered an internal detail and the client
> >> library shields you from a possible incompatibility if we'd ever make
> >> incompatible change.
> > 
> > Ignoring wire compatibility questions, it is important to note that not all
> > functionality in libvirt is done in libvirtd. There are libvirt drivers that
> > are entirely implemented in the library, not libvirtd. For some of the
> > libvirt drivers that do use libvirtd, there is also still some functionality
> > that is client side.
> 
> So to summarize what I'm hearing here:
> 
>  * There is no libvirt remotable API today.

Libvirt drivers that are implemented in libvirtd are accessible remotely
using the libvirt client library. We just consider the wire protocol a
private impl detail between them.  Drivers implemented outside libvirtd
are often accessible remotely too, via the hypervisor's native remoting
system. 

>  * All libvirt APIs are only able to be used in-process.
>  * There is presently no ability for a libvirt API to be used from a
>browser, or non-Linux process or non-Linux system.

Actually libvirt can be built on OS-X, Windows and FreeBSD too.

> In order for Cockpit to interact with libvirt, it would need to grow a
> remoteable API. The Cockpit project could likely jumpstart such an
> effort, by lightly wrapping the C API in DBus ... and contribute that
> remoteable API code to the libvirt project.

FWIW, if someone does want to build a DBus wrapper around libvirt, that
would be something todo as a separate code module above the libvirt
API. We would be happy to host such an effort on libvirt.org git though,
alongside other higher level API wrappers. Likewise for a REST API wrapper.

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

Re: [libvirt] Remotable Libvirt

[Daniel P. Berrange]

On Tue, Jun 06, 2017 at 05:11:55PM +0200, Peter Krempa wrote:
> On Tue, Jun 06, 2017 at 07:45:10 -0700, Peter wrote:
> > On 05/26/2017 02:11 AM, Martin Kletzander wrote:
> > > On Thu, May 25, 2017 at 10:16:26AM -0700, Peter Volpe wrote:
> 
> [...]
> 
> > > If we standardize even the smallest part of the RPC, then it might screw
> > > us immediately.  We are keeping it private just so we can enhance the
> > > APIs we already have.  We don't know when we will need to change some
> > > part of it.
> > > 
> > 
> > How does the current ssh implementation work?
> > (https://libvirt.org/remote.html) If clients are able to talk to a remote
> > libvirtd via ssh then there must be some sort of compatibility guarantee.
> > Otherwise unless the versions are exactly the same clients wouldn't be able
> > to talk to remote daemons.
> 
> They use the internal RPC protocol transported over SSH. The client
> library initializes the ssh connection to the server, and then starts to
> talk the RPC tunelled over the SSH session.
> 
> The compatibility is guaranteed only if you use the client library. As
> said, the RPC protocol is considered an internal detail and the client
> library shields you from a possible incompatibility if we'd ever make
> incompatible change.

Ignoring wire compatibility questions, it is important to note that not all
functionality in libvirt is done in libvirtd. There are libvirt drivers that
are entirely implemented in the library, not libvirtd. For some of the
libvirt drivers that do use libvirtd, there is also still some functionality
that is client side.

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

Re: [libvirt] libvirt builds failing on fedora-rawhide

[Daniel P. Berrange]

On Mon, Jun 05, 2017 at 04:22:41PM -0400, Yash Mankad wrote:
> Hi,
> 
> The libvirt-master-build on ci.c.o seems to be failing on
> the fedora-rawhide slave since Friday.
> 
> This seems to be the commit/build that is causing it to fail.

Nope, it is a gnulib/glibc incompatibility

https://www.redhat.com/archives/libvir-list/2017-June/msg00217.html

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

Re: [libvirt] [PATCH 1/2] util: share code between virExec and virCommandExec

[Daniel P. Berrange]

On Thu, Jun 01, 2017 at 02:26:16PM +0200, Cédric Bosdonnat wrote:
> virCommand is a version of virExec that doesn't fork, however it is
> just calling execve and doesn't honors setting uid/gid and pwd.
> 
> This commit moves those pieces from virExec to virCommandExec and
> makes virExec use virCommandExec to avoid code duplication.
> ---
>  src/util/vircommand.c | 92 
> ---
>  1 file changed, 43 insertions(+), 49 deletions(-)
> 
> diff --git a/src/util/vircommand.c b/src/util/vircommand.c
> index e1bbc0526..aa97a5a10 100644
> --- a/src/util/vircommand.c
> +++ b/src/util/vircommand.c
> @@ -481,21 +481,18 @@ virExec(virCommandPtr cmd)
>  int childerr = -1;
>  int tmpfd;
>  char *binarystr = NULL;
> -const char *binary = NULL;
>  int ret;
>  struct sigaction waxon, waxoff;
> -gid_t *groups = NULL;
> -int ngroups;
>  
>  if (cmd->args[0][0] != '/') {
> -if (!(binary = binarystr = virFindFileInPath(cmd->args[0]))) {
> +if (!(binarystr = virFindFileInPath(cmd->args[0]))) {
>  virReportSystemError(ENOENT,
>   _("Cannot find '%s' in path"),
>   cmd->args[0]);
>  return -1;
>  }
> -} else {
> -binary = cmd->args[0];
> +VIR_FREE(cmd->args[0]);
> +cmd->args[0] = binarystr;
>  }
>  
>  if (childin < 0) {
> @@ -556,9 +553,6 @@ virExec(virCommandPtr cmd)
>  childerr = null;
>  }
>  
> -if ((ngroups = virGetGroupList(cmd->uid, cmd->gid, &groups)) < 0)
> -goto cleanup;
> -
>  pid = virFork();
>  
>  if (pid < 0)
> @@ -577,9 +571,6 @@ virExec(virCommandPtr cmd)
>  
>  cmd->pid = pid;
>  
> -VIR_FREE(binarystr);
> -VIR_FREE(groups);
> -
>  return 0;
>  }
>  
> @@ -727,29 +718,6 @@ virExec(virCommandPtr cmd)
>  }
>  # endif
>  
> -/* The steps above may need to do something privileged, so we delay
> - * setuid and clearing capabilities until the last minute.
> - */
> -if (cmd->uid != (uid_t)-1 || cmd->gid != (gid_t)-1 ||
> -cmd->capabilities || (cmd->flags & VIR_EXEC_CLEAR_CAPS)) {
> -VIR_DEBUG("Setting child uid:gid to %d:%d with caps %llx",
> -  (int)cmd->uid, (int)cmd->gid, cmd->capabilities);
> -if (virSetUIDGIDWithCaps(cmd->uid, cmd->gid, groups, ngroups,
> - cmd->capabilities,
> - !!(cmd->flags & VIR_EXEC_CLEAR_CAPS)) < 0) {
> -goto fork_error;
> -}
> -}
> -
> -if (cmd->pwd) {
> -VIR_DEBUG("Running child in %s", cmd->pwd);
> -if (chdir(cmd->pwd) < 0) {
> -virReportSystemError(errno,
> - _("Unable to change to %s"), cmd->pwd);
> -goto fork_error;
> -}
> -}
> -
>  if (virCommandHandshakeChild(cmd) < 0)
> goto fork_error;

With this change, the setuid + chdir will now take place after we have
done the handshake with the parent. I think this will cause potential
problems. First the child will still be runing privileged at the point
where the parent synchronizes, which may violate some assumptions.
Second, it means we loose any useful error reoprting when setuid or
chdir fail, which is pretty bad.

>  
> @@ -771,15 +739,10 @@ virExec(virCommandPtr cmd)
>  /* Close logging again to ensure no FDs leak to child */
>  virLogReset();

This means that the reset logging before we've done the setuid/chdir,
so we loose any logging of these methods too

>  
> -if (cmd->env)
> -execve(binary, cmd->args, cmd->env);
> -else
> -execv(binary, cmd->args);
> +if (virCommandExec(cmd) == -2)
> +goto fork_error;
>  
>  ret = errno == ENOENT ? EXIT_ENOENT : EXIT_CANNOT_INVOKE;
> -virReportSystemError(errno,
> - _("cannot execute binary %s"),
> - cmd->args[0]);
>  
>   fork_error:
>  virDispatchError(NULL);
> @@ -789,9 +752,6 @@ virExec(virCommandPtr cmd)
>  /* This is cleanup of parent process only - child
> should never jump here on error */
>  
> -VIR_FREE(groups);
> -VIR_FREE(binarystr);
> -
>  /* NB we don't virReportError() on any failures here
> because the code which jumped here already raised
> an error condition which we must not overwrite */
> @@ -2150,23 +2110,57 @@ virCommandProcessIO(virCommandPtr cmd)
>   * in the hook after already forking / cloning, so does not attempt to
>   * daemonize or preserve any FDs.
>   *
> - * Returns -1 on any error executing the command.
> + * Returns -1 on any error executing the command, -2 if the error happen
> + * before running the command.
> + *
>   * Will not return on success.
>   */
>  #ifndef WIN32
>  int virCommandExec(virCommandPtr cmd)
>  {
> +gid_t *groups = NULL;
> +int ngroups;
> +
>  if (!cmd

Re: [libvirt] [PATCH 2/2] lxc: allow user to specify command working directory

[Daniel P. Berrange]

On Thu, Jun 01, 2017 at 02:26:17PM +0200, Cédric Bosdonnat wrote:
> Some containers may want the application to run in a special directory.
> Add  element in the domain configuration to handle this case
> and use it in the lxc driver.
> ---
>  docs/formatdomain.html.in|  5 +
>  docs/schemas/domaincommon.rng|  5 +
>  src/conf/domain_conf.c   |  5 +
>  src/conf/domain_conf.h   |  1 +
>  src/lxc/lxc_container.c  |  4 +++-
>  tests/lxcxml2xmldata/lxc-initdir.xml | 30 ++
>  tests/lxcxml2xmltest.c   |  1 +
>  7 files changed, 50 insertions(+), 1 deletion(-)
>  create mode 100644 tests/lxcxml2xmldata/lxc-initdir.xml

Any need for the native-to-xml conversion here too ?

> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 03153b972..105f0b7a6 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -1841,6 +1841,7 @@ struct _virDomainOSDef {
>  char *init;
>  char **initargv;
>  virDomainOSEnvPtr *initenv;
> +char *initdir;
>  char *kernel;
>  char *initrd;
>  char *cmdline;
> diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c
> index ffafc39d7..c122a588e 100644
> --- a/src/lxc/lxc_container.c
> +++ b/src/lxc/lxc_container.c
> @@ -237,7 +237,7 @@ static virCommandPtr 
> lxcContainerBuildInitCmd(virDomainDefPtr vmDef,
>  virCommandAddEnvString(cmd, "PATH=/bin:/sbin");
>  virCommandAddEnvString(cmd, "TERM=linux");
>  virCommandAddEnvString(cmd, "container=lxc-libvirt");
> -virCommandAddEnvString(cmd, "HOME=/");
> +/*virCommandAddEnvString(cmd, "HOME=/"); */
>  virCommandAddEnvPair(cmd, "container_uuid", uuidstr);
>  if (nttyPaths > 1)
>  virCommandAddEnvPair(cmd, "container_ttys", 
> virBufferCurrentContent(&buf));

This doesn't work obviously.


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

Re: [libvirt] [PATCH] lxc: allow defining environment variables

[Daniel P. Berrange]

On Tue, May 30, 2017 at 09:32:43PM +0200, Cédric Bosdonnat wrote:
> When running an application container, setting environment variables
> could be important.
> 
> The newly introduced  tag in domain configuration will allow
> setting environment variables to the init program.
> ---
>  docs/formatdomain.html.in|  5 +
>  docs/schemas/domaincommon.rng| 10 ++
>  src/conf/domain_conf.c   | 38 
> 
>  src/conf/domain_conf.h   |  8 
>  src/lxc/lxc_container.c  |  5 +
>  tests/lxcxml2xmldata/lxc-initenv.xml | 30 
>  tests/lxcxml2xmltest.c   |  1 +
>  7 files changed, 97 insertions(+)
>  create mode 100644 tests/lxcxml2xmldata/lxc-initenv.xml

Generally looks fine, but do we need to wire this up for  native-to-xml
conversion too ?


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

Re: [libvirt] [ISSUE go-xml] go-xml does NOT support DomainDisk marshal

[Daniel P. Berrange]

On Sat, Jun 03, 2017 at 11:27:19AM +0800, ZhenweiPi wrote:
> Hi,
> 
> 
> In the case of disk hot-plug, we need a xml like this :
> 
> ///
> //  //
> //  //
> //  //
> //  //
> ///
> 
> 
> For now, go-xml does not support DomainDisk marshal.
> 
> Any good idea ?

All we needed todo was add XMLName to the device structs, and define the
Marhsal/Unmarhsal methods. The only complication was that we used the
same DomainChardev struct for console/serial/channel devices, so I had
to define new structs for those.  This is all pushed to git now, with
tests.

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

[libvirt] FYI build currently broken w/ glibc >= 2.25.90

[Daniel P. Berrange]

FYI, there is currently a bug in gnulib that is breaking build for anything
with glibc >= 2.25.90 (aka current Fedora 27 rawhide)

  http://lists.gnu.org/archive/html/bug-gnulib/2017-06/msg3.html

Hopefully gnulib will accept my proposed fix soon...

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

Re: [libvirt] [PATCH] Reset the whole stack in testutils

[Daniel P. Berrange]

On Mon, Jun 05, 2017 at 10:01:10AM +0200, Martin Kletzander wrote:
> The memset() was resetting only 30 bytes in the array (size of the
> array), but it is array of pointers.  Since it is a static array,
> let's just reset it by its size.
> 
> Found by gcc-7.1:
> 
>   testutils.c: In function 'virTestRun':
>   testutils.c:243:13: error: 'memset' used with length equal to number
>   of elements without multiplication by element size [-Werror=memset-elt-size]
> memset(testAllocStack, 0, ARRAY_CARDINALITY(testAllocStack));
> ^~
> 
> Signed-off-by: Martin Kletzander 
> ---
> Pushed as trivial an also under the build-breaker rule.
> 
> I'm still getting one more error in config.h that probably needs
> fixing in autoconf:
> 
> ../config.h:2994:48: warning: this use of "defined" may not be portable 
> [-Wexpansion-to-defined]
>  || (defined _FORTIFY_SOURCE && 0 < _FORTIFY_SOURCE \
> 
> Let me know if you know how I could help with that.

Looks like that comes from gnulib actually, so something to reoprt there
I guess

$ git grep FORTIFY_SOURCE | grep defined
m4/extern-inline.m4:|| (defined _FORTIFY_SOURCE && 0 < 
_FORTIFY_SOURCE \



Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

Re: [libvirt] [PATCH go-xml] Add support for Node Device with basic testing.

[Daniel P. Berrange]

On Wed, May 31, 2017 at 11:35:14AM -0400, Vladik Romanovsky wrote:
> ---
>  node_device.go  | 168 
> 
>  node_device_test.go | 128 +++
>  2 files changed, 296 insertions(+)
>  create mode 100644 node_device.go
>  create mode 100644 node_device_test.go
> 
> diff --git a/node_device.go b/node_device.go
> new file mode 100644
> index 000..d7850e9
> --- /dev/null
> +++ b/node_device.go
> @@ -0,0 +1,168 @@
> +/*
> + * This file is part of the libvirt-go-xml project
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a 
> copy
> + * of this software and associated documentation files (the "Software"), to 
> deal
> + * in the Software without restriction, including without limitation the 
> rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL 
> THE
> + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
> FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + *
> + * Copyright (C) 2017 Red Hat, Inc.
> + *
> + */
> +
> +package libvirtxml
> +
> +import (
> + "encoding/xml"
> +)
> +
> +type NodeDevice struct {
> + Name   string `xml:"name"`
> + Path   string `xml:"path,omitempty"`
> + Parent string `xml:"parent,omitempty"`
> + Driver string `xml:"driver>name,omitempty"`
> + Capability []NodeDeviceCapability `xml:"capability"`
> +}
> +
> +type NodeDeviceCapability struct {
> + Domain  int`xml:"domain,omitempty"`
> + Bus int`xml:"bus,omitempty"`
> + Slotint`xml:"slot,omitempty"`
> + Functionint`xml:"function,omitempty"`
> + IommuGroup  *NodeDeviceIOMMUGroup  `xml:"iommuGroup,omitempty"`
> + Numa*NodeDeviceNUMA`xml:"numa,omitempty"`
> + PciExpress  *NodeDevicePciExpress  `xml:"pci-express,omitempty"`

You don't need omitempty on any field that is a pointer.

> + Hardware*NodeDeviceSystemHardware  `xml:"hardware"`
> + Firmware*NodeDeviceSystemFirmware  `xml:"firmware"`
> + Device  int`xml:"device"`
> + Number  int`xml:"number"`
> + Class   int`xml:"class"`
> + Subclassint`xml:"subclass"`
> + Protocolint`xml:"protocol"`
> + Description string `xml:"description,omitempty"`
> + Interface   string `xml:"interface"`
> + Address string `xml:"address"`
> + Link*NodeDeviceNetLink `xml:"link"`
> + Features[]NodeDeviceNetOffloadFeatures `xml:"feature,omitempty"`
> + UniqueIDint`xml:"unique_id"`
> + Target  int`xml:"target"`
> + Lun int`xml:"lun"`
> + Block   string `xml:"block"`
> + DriverType  string `xml:"drive_type"`
> + Model   string `xml:"model"`
> + Serial  string `xml:"serial"`
> + Sizeint`xml:"size"`
> + Hostint`xml:"host"`
> + Typestring `xml:"type"`

I'm surprised we don't ahve omitempty on almost all of these above

> + Product *NodeDeviceProduct `xml:"product,omitempty"`
> + Vendor  *NodeDeviceVendor  `xml:"vendor,omitempty"`
> + Capability  []NodeDeviceNestedCapabilities `xml:"capability,omitempty"`
> +}

Wow, we have a lot of capabilities here. Makes be wonder if your previous
patch was in fact better... will have to think about that more.

> +
> +type NodeDeviceVendor struct {
> + ID   string `xml:"id,attr,omitempty"`
> + Name string `xml:",chardata"`
> +}
> +type NodeDeviceProduct struct {
> + ID   string `xml:"id,attr,omitempty"`
> + Name string `xml:",chardat

Re: [libvirt] [PATCH go-xml] Add support for domain input address

[Daniel P. Berrange]

On Thu, Jun 01, 2017 at 05:49:59PM +0800, zhenwei.pi wrote:
> ---
>  domain.go  |  5 +++--
>  domain_test.go | 11 ++-
>  2 files changed, 13 insertions(+), 3 deletions(-)

Thanks, I've pushed this


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

Re: [libvirt] [PATCH v2] daemon: Don't initialize SASL context if not necessary

[Daniel P. Berrange]

On Fri, Jun 02, 2017 at 02:53:28PM +0200, Peter Krempa wrote:
> SASL context would be initialized even if the corresponding TCP or TLS
> sockets are not enabled.
> 
> fe772f24a68 attempted to fix the symptom by commenting out the settings,
> but that did not fix the root cause. 3c647ee4bbb later reverted those
> changes so that the more secure algorithm is used.
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1450095
> ---
> v2:
> Fix the message also if SASL authentication and the TCP/TLS sockets are
> explicitly enabled in config bug --listen is not specified.
> 
>  daemon/libvirtd.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
> index 891238bcb..bac4bc1b6 100644
> --- a/daemon/libvirtd.c
> +++ b/daemon/libvirtd.c
> @@ -613,11 +613,11 @@ daemonSetupNetworking(virNetServerPtr srv,
> 
>  #if WITH_SASL
>  if (config->auth_unix_rw == REMOTE_AUTH_SASL ||
> -config->auth_unix_ro == REMOTE_AUTH_SASL ||
> +(sock_path_ro && config->auth_unix_ro == REMOTE_AUTH_SASL) ||
>  # if WITH_GNUTLS
> -config->auth_tls == REMOTE_AUTH_SASL ||
> +(ipsock && config->listen_tls && config->auth_tls == 
> REMOTE_AUTH_SASL) ||
>  # endif
> -config->auth_tcp == REMOTE_AUTH_SASL) {
> +(ipsock && config->listen_tcp && config->auth_tcp == 
> REMOTE_AUTH_SASL)) {
>  saslCtxt = virNetSASLContextNewServer(
>  (const char *const*)config->sasl_allowed_username_list);
>  if (!saslCtxt)


Reviewed-by: Daniel P. Berrange 

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

Re: [libvirt] [PATCH] daemon: Don't initialize SASL context if not necessary

[Daniel P. Berrange]

On Fri, Jun 02, 2017 at 02:10:25PM +0200, Peter Krempa wrote:
> SASL context would be initialized even if the corresponding TCP or TLS
> sockets are not enabled.
> 
> fe772f24a68 attempted to fix the symptom by commenting out the settings,
> but that did not fix the root cause. 3c647ee4bbb later reverted those
> changes so that the more secure algorithm is used.
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1450095
> ---
>  daemon/libvirtd.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
> index 891238bcb..4a242e3e5 100644
> --- a/daemon/libvirtd.c
> +++ b/daemon/libvirtd.c
> @@ -613,11 +613,11 @@ daemonSetupNetworking(virNetServerPtr srv,
> 
>  #if WITH_SASL
>  if (config->auth_unix_rw == REMOTE_AUTH_SASL ||
> -config->auth_unix_ro == REMOTE_AUTH_SASL ||
> +(sock_path_ro && config->auth_unix_ro == REMOTE_AUTH_SASL) ||
>  # if WITH_GNUTLS
> -config->auth_tls == REMOTE_AUTH_SASL ||
> +(config->listen_tls && config->auth_tls == REMOTE_AUTH_SASL) ||
>  # endif
> -config->auth_tcp == REMOTE_AUTH_SASL) {
> +(config->listen_tcp && config->auth_tcp == REMOTE_AUTH_SASL)) {
>  saslCtxt = virNetSASLContextNewServer(
>  (const char *const*)config->sasl_allowed_username_list);
>  if (!saslCtxt)

I think you need to check 'ipsock' too, since  listen_tls defaults
to 1, but is not used unless --listen is set.

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

Re: [libvirt] [PATCH v2 5/6] virsh: Report errors from stream callbacks

[Daniel P. Berrange]

On Thu, Jun 01, 2017 at 03:43:06PM +0200, Michal Privoznik wrote:
> On 06/01/2017 03:39 PM, Daniel P. Berrange wrote:
> > On Thu, Jun 01, 2017 at 02:02:13PM +0200, Michal Privoznik wrote:
> >> There are couple of callbacks we pass to virStreamSendAll(),
> >> virStreamRecvAll() or its sparse variants. However, none of these
> >> callbacks reports error if one occurs and neither do the
> >> virStream* functions leaving user with very unhelpful error
> >> message:
> >>
> >>   error: cannot receive data from volume fedora.img
> >>   error: An error occurred, but the cause is unknown
> > 
> > If we change the code to honour errno set in the callbacks,
> > we only need to deal with ensuring  virFileInData sets
> > the errno correctly.
> 
> Okay, makes sense. Will post v3.
> 
> Meanwhile, do you have any clever idea on this?
> 
> https://www.redhat.com/archives/libvir-list/2017-June/msg00015.html

Hmm, no bright ideas, without spending some time peering into the
code.

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

Re: [libvirt] [PATCH v2 5/6] virsh: Report errors from stream callbacks

[Daniel P. Berrange]

On Thu, Jun 01, 2017 at 02:02:13PM +0200, Michal Privoznik wrote:
> There are couple of callbacks we pass to virStreamSendAll(),
> virStreamRecvAll() or its sparse variants. However, none of these
> callbacks reports error if one occurs and neither do the
> virStream* functions leaving user with very unhelpful error
> message:
> 
>   error: cannot receive data from volume fedora.img
>   error: An error occurred, but the cause is unknown

If we change the code to honour errno set in the callbacks,
we only need to deal with ensuring  virFileInData sets
the errno correctly.


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

Re: [libvirt] [PATCH v2 6/6] streams: Report errors if sendAll/recvAll callbacks fail

[Daniel P. Berrange]

On Thu, Jun 01, 2017 at 02:23:30PM +0200, Martin Kletzander wrote:
> On Thu, Jun 01, 2017 at 01:10:45PM +0100, Daniel P. Berrange wrote:
> > On Thu, Jun 01, 2017 at 02:02:14PM +0200, Michal Privoznik wrote:
> > > We have couple of wrappers over our low level stream APIs:
> > > virSreamRecvAll(), virStreamSendAll() and their sparse stream
> > > variants. All of them take some callbacks and call them at
> > > appropriate times. If a callback fails it aborts the whole
> > > operation. However, if it so happens that the callback fails
> > > without setting any error users are left with very unhelpful
> > > error message:
> > > 
> > >   error: cannot receive data from volume fedora.img
> > >   error: An error occurred, but the cause is unknown
> > > 
> > > One way to avoid it is to report error if callback hasn't.
> > 
> > The callbacks should never be expected to set a libvirt
> > error, as they are implemented by code that is outside
> > libvirt. The intention was really that the callbacks set
> > an errno value on error, since that's all you have access
> > to from application level impl
> > 
> > > 
> > > Signed-off-by: Michal Privoznik 
> > > ---
> > >  src/libvirt-stream.c | 41 +
> > >  1 file changed, 33 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/src/libvirt-stream.c b/src/libvirt-stream.c
> > > index 1594ed212..efdbc9e44 100644
> > > --- a/src/libvirt-stream.c
> > > +++ b/src/libvirt-stream.c
> > > @@ -596,8 +596,12 @@ virStreamSendAll(virStreamPtr stream,
> > >  for (;;) {
> > >  int got, offset = 0;
> > 
> > For sanity we should set 'errno = 0' here.
> > 
> > >  got = (handler)(stream, bytes, want, opaque);
> > > -if (got < 0)
> > > +if (got < 0) {
> > > +if (!virGetLastError())
> > > +virReportError(VIR_ERR_OPERATION_FAILED, "%s",
> > > +   _("send handler failed"));
> > >  goto cleanup;
> > > +}
> > 
> > ...then this should do
> > 
> > 
> >  if (errno == 0) {
> >  errno = EIO;
> >  }
> >  virReportSystemError(errno, )
> > 
> 
> Oh, that's a good idea.  But can we check for both?  The callbacks can
> still call libvirt (to receive from the stream for example) and that
> error could be kept as well.  Not all errors will set errno.  On the
> other hand the application will already know about the problem and can
> deal with it how it wants, but we should also aim at intuitive usage.

Libvirtd code never uses the SendAll/RecvAll functions AFAIK.

The only user of the callbacks which might call other libvirt
(internal) functions is really virsh, but even that is just
calling plain posix functions which set errno (at least until
patch 5 in this series). So I'm not seeing a compelling scenario
where you need to preseve libvirt errors here.

> If we are to expect errno, we need to say that in the documentation,
> really.

Agreed, in fact I thought we already did, but I guess not.

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

Re: [libvirt] [PATCH 3/3] virStream: Remove callbacks on Abort/Finish

[Daniel P. Berrange]

On Thu, Jun 01, 2017 at 03:20:50PM +0200, Martin Kletzander wrote:
> On Thu, Jun 01, 2017 at 01:29:45PM +0100, Daniel P. Berrange wrote:
> > On Thu, Jun 01, 2017 at 02:24:19PM +0200, Martin Kletzander wrote:
> > > On Thu, Jun 01, 2017 at 01:19:05PM +0100, Daniel P. Berrange wrote:
> > > > On Thu, Jun 01, 2017 at 02:08:34PM +0200, Martin Kletzander wrote:
> > > > > Users need to remove their callbacks before calling virStreamAbort()
> > > > > or virStreamFinish() even though that's not documented anywhere.
> > > > > Since it makes no sense to keep those callbacks, we can remove them
> > > > > when the stream is being aborted or finished.  That way it is also
> > > > > more intuitive for developers as that removes some confusing errors
> > > > > being reported.
> > > >
> > > > This changes the semantics of a public API though, so even though
> > > > the suggested behavious would be useful, we mustn't do this as it
> > > > creates an API incompatability across versions.
> > > >
> > > 
> > > I couldn't find any case that could be broken by this change.  Do you
> > > have any in mind?
> > 
> > Someone writes an app that relies on this behaviour and it runs on a 
> > different
> > libvirt version it'll never unregister the callbacks. This means any 
> > freefunc
> > associated with the callback won't be triggered and thus opaque memory will
> > leak. So apps will end up having todo "if libvirt version == x then .. 
> > else.."
> > to deal with the semantic change of the API, or just never rely on this new
> > behaviour at all.
> > 
> 
> OK.  That would most likely happen on downgrade, but this would not be
> very different to some other leak that we fix at some point.  We could
> document this, but I have a feeling that would not help making my case,
> would it?

I don't really think those are the same scenario. Those are apps using APIs
in the correct way, but it just happens that some libvirtd code paths have
some leaks.

In this case, the applications are failing to use the existing APIs in the
correct way, and we're proposing changing semantics of the public API to
cover up application bugs.


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

Re: [libvirt] [PATCH 3/3] virStream: Remove callbacks on Abort/Finish

[Daniel P. Berrange]

On Thu, Jun 01, 2017 at 02:24:19PM +0200, Martin Kletzander wrote:
> On Thu, Jun 01, 2017 at 01:19:05PM +0100, Daniel P. Berrange wrote:
> > On Thu, Jun 01, 2017 at 02:08:34PM +0200, Martin Kletzander wrote:
> > > Users need to remove their callbacks before calling virStreamAbort()
> > > or virStreamFinish() even though that's not documented anywhere.
> > > Since it makes no sense to keep those callbacks, we can remove them
> > > when the stream is being aborted or finished.  That way it is also
> > > more intuitive for developers as that removes some confusing errors
> > > being reported.
> > 
> > This changes the semantics of a public API though, so even though
> > the suggested behavious would be useful, we mustn't do this as it
> > creates an API incompatability across versions.
> > 
> 
> I couldn't find any case that could be broken by this change.  Do you
> have any in mind?

Someone writes an app that relies on this behaviour and it runs on a different
libvirt version it'll never unregister the callbacks. This means any freefunc
associated with the callback won't be triggered and thus opaque memory will
leak. So apps will end up having todo "if libvirt version == x then .. else.."
to deal with the semantic change of the API, or just never rely on this new
behaviour at all.

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

Re: [libvirt] [PATCH 3/3] virStream: Remove callbacks on Abort/Finish

[Daniel P. Berrange]

On Thu, Jun 01, 2017 at 02:08:34PM +0200, Martin Kletzander wrote:
> Users need to remove their callbacks before calling virStreamAbort()
> or virStreamFinish() even though that's not documented anywhere.
> Since it makes no sense to keep those callbacks, we can remove them
> when the stream is being aborted or finished.  That way it is also
> more intuitive for developers as that removes some confusing errors
> being reported.

This changes the semantics of a public API though, so even though
the suggested behavious would be useful, we mustn't do this as it
creates an API incompatability across versions.


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

Re: [libvirt] [PATCH v2 6/6] streams: Report errors if sendAll/recvAll callbacks fail

[Daniel P. Berrange]

On Thu, Jun 01, 2017 at 02:02:14PM +0200, Michal Privoznik wrote:
> We have couple of wrappers over our low level stream APIs:
> virSreamRecvAll(), virStreamSendAll() and their sparse stream
> variants. All of them take some callbacks and call them at
> appropriate times. If a callback fails it aborts the whole
> operation. However, if it so happens that the callback fails
> without setting any error users are left with very unhelpful
> error message:
> 
>   error: cannot receive data from volume fedora.img
>   error: An error occurred, but the cause is unknown
> 
> One way to avoid it is to report error if callback hasn't.

The callbacks should never be expected to set a libvirt
error, as they are implemented by code that is outside
libvirt. The intention was really that the callbacks set
an errno value on error, since that's all you have access
to from application level impl

> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/libvirt-stream.c | 41 +
>  1 file changed, 33 insertions(+), 8 deletions(-)
> 
> diff --git a/src/libvirt-stream.c b/src/libvirt-stream.c
> index 1594ed212..efdbc9e44 100644
> --- a/src/libvirt-stream.c
> +++ b/src/libvirt-stream.c
> @@ -596,8 +596,12 @@ virStreamSendAll(virStreamPtr stream,
>  for (;;) {
>  int got, offset = 0;

For sanity we should set 'errno = 0' here.

>  got = (handler)(stream, bytes, want, opaque);
> -if (got < 0)
> +if (got < 0) {
> +if (!virGetLastError())
> +virReportError(VIR_ERR_OPERATION_FAILED, "%s",
> +   _("send handler failed"));
>  goto cleanup;
> +}

...then this should do


  if (errno == 0) {
  errno = EIO;
  }
  virReportSystemError(errno, )


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

Re: [libvirt] [RFC] Version numbers outside of releases

[Daniel P. Berrange]

On Thu, Jun 01, 2017 at 02:05:04PM +0200, Andrea Bolognani wrote:
> On Thu, 2017-06-01 at 12:21 +0100, Daniel P. Berrange wrote:
> > This is not that attractive from POV the language bindings.
> > 
> > eg in the Go code, I need to make usage of the new APIs
> > conditional at compile time. For example for the new APIs
> > added in 3.4.0 I have code like
> > 
> >   func (v *Stream) RecvFlags(p []byte, flags StreamRecvFlagsValues) (int, 
> >error) {
> > if C.LIBVIR_VERSION_NUMBER < 3004000 {
> > return 0, GetNotImplementedError("virStreamRecvFlags")
> > }
> > 
> > n := C.virStreamRecvFlagsCompat(v.ptr, 
> >(*C.char)(unsafe.Pointer(&p[0])), C.size_t(len(p)), C.uint(flags))
> > 
> > ...snip...
> >   }
> > 
> > and 
> > 
> >   int virStreamRecvFlagsCompat(virStreamPtr st,
> >char *data,
> >size_t nbytes,
> >unsigned int flags)
> >   {
> >   #if LIBVIR_VERSION_NUMBER < 3004000
> >   assert(0); // Caller should have checked version
> >   #else
> >   return virStreamRecvFlags(st, data, nbytes, flags);
> >   #endif
> >   }
> > 
> > The suggested versioning scheme would require changing all these version
> > numbers checks to 3003090 is rather unpleasant IMHO.
> 
> Well, that shouldn't be needed for released versions of the
> bindings as they would of course depend on a released version
> of libvirt, but I see how it would make it more difficult
> than necessary to develop the bindings in parallel to the new
> libvirt version.
> 
> So I guess disregard my proposal :)

We should none the less fix 'make rpm' to use the best practice release
numbering scheme for pre-releases

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

Re: [libvirt] [RFC] Version numbers outside of releases

[Daniel P. Berrange]

On Thu, Jun 01, 2017 at 01:13:05PM +0200, Andrea Bolognani wrote:
> Our current practice when it comes to bumping the version
> number for libvirt is to do so immediately after a release,
> eg. right after 3.4.0 is released later today someone will
> push a commit that changes configure.ac to use 3.5.0 instead.
> 
> As a consequence, builds made from master will always carry
> the release number of the *upcoming* release, so from a
> versioning point of view there is no way to tell eg. 3.4.0-rc2
> or even the final 3.4.0 release apart from a random build made
> at a random time from master during 3.4.0's development cycle.
> 
> In particular, when creating RPMs from a git clone, you'll end
> up having to use either 'dnf install' or 'dnf reinstall'
> depending on whether or not you already installed any build
> during the current development cycle.

This is really a bug in the RPM spec file used to build from
pre-released code.

The recommended Fedora RPM practice for version numbers in
packages from GIT snapshots, is that the release field
start with a "0", and have a date stamp (and possibly git
short hash) appended.  The first RPM following the formal
release would have a release starting with a "1". This
way, every build from git you do is always newer, and the
final release RPM is newer still.

> I suggest we change our habits slightly:
> 
>   * right after X.Y.0 has been released, bump the version
> number to X.Y.90;
> 
>   * bump the version number to X.Y.91 for rc1, X.Y.92 for
> rc2 and so on;
> 
>   * only bump the version number to X.Y+1.0 as the release is
> being prepared, then go back to the first step and move
> on with development.
> 
> This would make sure each step in the development cycle gets
> its own version number, and as a pleasant side effect you'll
> always be able to install newer builds using 'dnf install'.
> 
> Comments welcome! :)

This is not that attractive from POV the language bindings.

eg in the Go code, I need to make usage of the new APIs
conditional at compile time. For example for the new APIs
added in 3.4.0 I have code like

  func (v *Stream) RecvFlags(p []byte, flags StreamRecvFlagsValues) (int, 
error) {
if C.LIBVIR_VERSION_NUMBER < 3004000 {
return 0, GetNotImplementedError("virStreamRecvFlags")
}

n := C.virStreamRecvFlagsCompat(v.ptr, 
(*C.char)(unsafe.Pointer(&p[0])), C.size_t(len(p)), C.uint(flags))

...snip...
  }

and 

  int virStreamRecvFlagsCompat(virStreamPtr st,
   char *data,
   size_t nbytes,
   unsigned int flags)
  {
  #if LIBVIR_VERSION_NUMBER < 3004000
  assert(0); // Caller should have checked version
  #else
  return virStreamRecvFlags(st, data, nbytes, flags);
  #endif
  }

The suggested versioning scheme would require changing all these version
numbers checks to 3003090 is rather unpleasant IMHO.

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

[libvirt] [PATCH] Fix closing XML element in news file

[Daniel P. Berrange]

Signed-off-by: Daniel P. Berrange 
---

Pushed as a trivial build fix

 docs/news.xml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/docs/news.xml b/docs/news.xml
index 6ff01ca..e95fe72 100644
--- a/docs/news.xml
+++ b/docs/news.xml
@@ -108,7 +108,7 @@
 
   Endpoint devices support new mdev capability
   and their parents now report the supported types in new
-  mdev_types capability.
+  mdev_types capability.
 
   
   
-- 
2.9.3

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

Re: [libvirt] [PATCH go-xml] Add support for device sound

[Daniel P. Berrange]

On Thu, Jun 01, 2017 at 10:58:06AM +0800, zhenwei.pi wrote:
> ---
>  domain.go  | 11 +++
>  domain_test.go | 20 
>  2 files changed, 31 insertions(+)

Thanks for your contribution, pushed to git


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

Re: [libvirt] [PATCH V2] CI: also run tests using updated distro(s)

[Daniel P. Berrange]

On Wed, May 31, 2017 at 12:21:05PM -0300, claudioandre...@gmail.com wrote:
> From: Claudio André 
> 
> It is possible to test libvirt using other distros in Travis via Docker;
> including (but not limited to) Fedora and Ubuntu.
> ---
> See it in action at 
> https://travis-ci.org/claudioandre/libvirt/builds/237687907 
> 
>  .travis.yml| 23 ++
>  tests/travis-ci.sh | 70 
> ++
>  2 files changed, 83 insertions(+), 10 deletions(-)
>  create mode 100755 tests/travis-ci.sh
> 
> diff --git a/.travis.yml b/.travis.yml
> index 5a3e765..3ed2093 100644
> --- a/.travis.yml
> +++ b/.travis.yml
> @@ -1,8 +1,6 @@
>  sudo: false
>  language: c
>  dist: precise
> -compiler:
> -  - gcc
>  cache: ccache
>  addons:
>apt:
> @@ -62,15 +60,14 @@ git:
>  before_install:
>- if [ "$TRAVIS_OS_NAME" == "osx" ]; then brew update && brew install 
> gnutls libgcrypt yajl gettext rpcgen ; fi
>  
> -# the custom PATH is just to pick up OS-X homebrew & its harmless on Linux
> -before_script:
> -  - PATH="/usr/local/opt/gettext/bin:/usr/local/opt/rpcgen/bin:$PATH" 
> ./autogen.sh
>  script:
> -  - VIR_TEST_DEBUG=1 make -j3 && make -j3 syntax-check && make -j3 check
> +  - tests/travis-ci.sh
>  
>  # Environments here are run in addition to the main environment defined above
>  matrix:
>include:
> +- compiler: gcc
> +  dist: precise
>  - compiler: clang
>dist: precise
>  - compiler: clang
> @@ -79,10 +76,16 @@ matrix:
>dist: trusty
>  - compiler: clang
>os: osx
> -  script:
> -# many unit tests fail & so does syntax-check, so skip for now
> -# one day we must fix it though
> -- make -j3
> +- services: docker
> +  env: IMAGE=ubuntu:17.04 CCO=gcc
> +  dist: trusty

Why is this labelled 'trusty' if we're running 17.04 which is not 'trusty'

Also just call the env var 'CC' rather than 'CCO' as that is
the more normal convention.

> +- services: docker
> +  env: IMAGE=ubuntu:17.04 CCO=clang
> +  dist: trusty

Again ?

> +
> +  allow_failures:
> +- env: IMAGE=ubuntu:17.04 CCO=gcc
> +- env: IMAGE=ubuntu:17.04 CCO=clang

Can you just fix the tests instead please. 

>  
>  after_failure:
>- echo 
> ''
> diff --git a/tests/travis-ci.sh b/tests/travis-ci.sh
> new file mode 100755
> index 000..07ec85d
> --- /dev/null
> +++ b/tests/travis-ci.sh
> @@ -0,0 +1,70 @@
> +#!/bin/bash -e
> +
> +function do_Install_Dependencies(){
> +echo
> +echo '-- Installing Dependencies --'
> +
> +apt-get update -qq
> +apt-get -y -qq install \
> +build-essential git clang autoconf libtool libcmpicppimpl0 
> gettext \
> +xsltproc autopoint libxml2-dev libncurses5-dev libreadline-dev \
> +zlib1g-dev libgnutls28-dev libgcrypt11-dev libavahi-client-dev 
> libsasl2-dev \
> +libxen-dev lvm2 libgcrypt11-dev libparted0-dev libdevmapper-dev 
> uuid-dev \
> +libudev-dev libpciaccess-dev libcap-ng-dev libnl-3-dev 
> libnl-route-3-dev \
> +libyajl-dev libpcap0.8-dev libnuma-dev libnetcf-dev libaudit-dev 
> \
> +libxml2-utils libapparmor-dev dnsmasq-base librbd-dev 
> w3c-markup-validator kmod > /dev/null
> +}

This isn't portable so we should at least check that we have the apt-get
binary, and print out a message about porting it to other distros...

> +
> +
> +function do_Show_Info(){
> +echo
> +echo '-- Environment --'
> +echo "Running on Docker: $DISTRO"
> +id
> +uname -a
> +}
> +
> +
> +function do_Show_Compiler(){
> +
> +if [[ -n $CC ]]; then
> +echo
> +echo '-- Compiler in use --'
> +"$CC" --version
> +fi
> +}

I don't think we need separate functions for these two - just put
it all in one place.

In fact I think it is best to just run 'printenv' instead of
special casing the 'CC' env variable.

> +
> +
> +# --- Build and Test libvirt ---
> +
> +if [[ -n $IMAGE ]]; then
> +# Run docker using the selected image; then build and test
> +docker run --privileged --cap-add=ALL -v /lib/modules:/lib/modules \
> +  -v "$(pwd)":/cwd -e CC=$CCO -e DISTRO=$IMAGE "$IMAGE" sh -e -c " \
> +cd /cwd; \
> +tests/travis-ci.sh"
> +exit $?
> +fi
> +
> +if [[ -n $DISTRO ]]; then
> +do_Show_Info
> +do_Install_Dependencies
> +do_Show_Compiler
> +fi
> +
> +echo -en 'travis_fold:start:autogen\r'
> +echo '-- Running ./autogen.sh --'
> +# The custom PATH is just to pick up OS-X homebrew & its harmless on 
> Linux
> +PATH="/usr/local/opt/gettext/bin:/usr/local/opt/rpcgen/bin:$PATH" 
> ./autogen.sh
> +echo -en 'travis_fold:end:autogen\r'

Per the comment, this is only needed for OS-X, so should be put inside the
the conditional for OS-X below

> +
> +# Build and test
> +if [[ "$TRAVIS_OS_NAME" = "osx" ]]; then
> + 

Re: [libvirt] Remotable Libvirt

[Daniel P. Berrange]

On Wed, May 31, 2017 at 03:59:10PM +0100, Richard W.M. Jones wrote:
> On Thu, May 25, 2017 at 10:26:47AM -0700, Peter wrote:
> > The majority of cockpit is implemented in
> > javascript.
> 
> How about using the gobject libvirt bindings?
> 
>   https://libvirt.org/git/?p=libvirt-glib.git;a=summary
> 
> They should be usable from Javascript directly, as in the .js example
> here:
> 
>   
> https://libvirt.org/git/?p=libvirt-glib.git;a=tree;f=examples;h=d63d5964be2299b62140f9fd183b5cc744837d8a;hb=HEAD

They're usable from a standalone javascript interpretor, but there's no
way to use them from a client side javascript interpretor in the user's
web browser.

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

Re: [libvirt] [PATCH go-xml] Add support for Node Device with basic testing.

[Daniel P. Berrange]

On Tue, May 30, 2017 at 10:36:22AM -0400, Vladik Romanovsky wrote:
> Thanks for the review.
> 
> On Tue, May 30, 2017 at 8:46 AM, Daniel P. Berrange  
> wrote:
> >> +type NodeDevice struct {
> >> + Name   string  `xml:"name"`
> >> + Path   string  `xml:"path,omitempty"`
> >> + Parent string  `xml:"parent,omitempty"`
> >> + Driver string  `xml:"driver>name,omitempty"`
> >> + Capability *CapabilityType `xml:"capability"`
> >> +}
> >
> > A single device can have multiple capabilities.
> Do you mean it should be:
>  Capability []CapabilityType `xml:"capability"` ?

Yes exactly.


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

libvir-list@redhat.com

[Daniel P. Berrange]

On Wed, May 31, 2017 at 09:26:01AM +0100, Daniel P. Berrange wrote:
> On Wed, May 31, 2017 at 01:32:47PM +0800, ZhenweiPi wrote:
> > ---
> > 
> >  domain.go  | 13 +++--
> >  domain_test.go | 56 
> > +---
> >  2 files changed, 64 insertions(+), 5 deletions(-)
> > 
> > diff --git a/domain.go b/domain.go
> > index 848835a..cbb22e5 100644
> > --- a/domain.go
> > +++ b/domain.go
> > @@ -30,8 +30,10 @@ import (
> >  )
> >  type DomainController struct {
> > -   Type  string `xml:"type,attr"`
> > -   Index string `xml:"index,attr"`
> > +   Typestring `xml:"type,attr"`
> > +   Index   *uint  `xml:"index,attr"`
> > +   Model   string `xml:"model,attr,omitempty"`
> > +   Address *DomainAddress `xml:"address"`
> >  }
> >  type DomainDiskSecret struct {
> > @@ -77,6 +79,8 @@ type DomainDisk struct {
> > Type string`xml:"type,attr"`
> > Device   string`xml:"device,attr"`
> > Snapshot string`xml:"snapshot,attr,omitempty"`
> > +   Cachestring`xml:"cache,attr,omitempty"`
> > +   Io   string`xml:"io,attr,omitempty"`
> > Driver   *DomainDiskDriver `xml:"driver"`
> > Auth *DomainDiskAuth   `xml:"auth"`
> > Source   *DomainDiskSource `xml:"source"`
> > @@ -196,8 +200,13 @@ type DomainAlias struct {
> >  type DomainAddress struct {
> > Type   string `xml:"type,attr"`
> > Controller *uint  `xml:"controller,attr"`
> > +   Domain *uint  `xml:"domain,attr"`
> > Bus*uint  `xml:"bus,attr"`
> > Port   *uint  `xml:"port,attr"`
> > +   Slot   *uint  `xml:"slot,attr"`
> > +   Function   *uint  `xml:"function,attr"`
> > +   Target *uint  `xml:"target,attr"`
> > +   Unit   *uint  `xml:"unit,attr"`
> >  }
> >  type DomainChardev struct {
> > diff --git a/domain_test.go b/domain_test.go
> > index 265cf80..22da947 100644
> > --- a/domain_test.go
> > +++ b/domain_test.go
> > @@ -30,6 +30,16 @@ import (
> > "testing"
> >  )
> > +type PciAddress struct {
> > +   Domain   uint
> > +   Bus  uint
> > +   Slot uint
> > +   Function uint
> > +}
> > +
> > +var uhciIndex uint = 0
> > +var uhciAddr = PciAddress{0, 0, 1, 2}
> 
> This struct and variables are rather pointless - just put the values
> inline in the one place where they are needed

Oh actually I see they are needed - you can't take the address of a
scalar in go.

I'll apply this patch to git.

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

Re: [libvirt] [PATCH] udev: Fix build on older platforms

[Daniel P. Berrange]

On Wed, May 31, 2017 at 12:22:01PM +0200, Erik Skultety wrote:
> Caused by commit @d1eea6c1 due to the missing symbol on older platforms.
> 
> Signed-off-by: Erik Skultety 
> ---
> Despite falling under build-breaker category, I'd like to get a proper review,
> since I'm not really familiar with autoconf and there might be a better fix.
> 
> Erik
> 
>  configure.ac   | 2 +-
>  src/node_device/node_device_udev.c | 2 ++
>  2 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/configure.ac b/configure.ac
> index 246f4e077..b78c8b790 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -322,7 +322,7 @@ AC_CHECK_FUNCS_ONCE([cfmakeraw fallocate geteuid getgid 
> getgrnam_r \
>getmntent_r getpwuid_r getrlimit getuid if_indextoname kill mmap \
>newlocale posix_fallocate posix_memalign prlimit regexec \
>sched_getaffinity setgroups setns setrlimit symlink sysctlbyname \
> -  getifaddrs sched_setscheduler unshare])
> +  getifaddrs sched_setscheduler unshare 
> udev_monitor_set_receive_buffer_size])

That is always going to fail - this AC_CHECK_FUNCS_ONCE macro is only
able to detect functions that are part of glibc.


Take a look at m4/virt-dbus.m4 for an example of how to check for a
function in another library, after detecting the library with pkgconfig


>  dnl Availability of various common headers (non-fatal if missing).
>  AC_CHECK_HEADERS([pwd.h regex.h sys/un.h \
> diff --git a/src/node_device/node_device_udev.c 
> b/src/node_device/node_device_udev.c
> index a69dc1175..01438ea17 100644
> --- a/src/node_device/node_device_udev.c
> +++ b/src/node_device/node_device_udev.c
> @@ -1764,12 +1764,14 @@ static int nodeStateInitialize(bool privileged,
> 
>  udev_monitor_enable_receiving(priv->udev_monitor);
> 
> +#if HAVE_UDEV_MONITOR_SET_RECEIVE_BUFFER_SIZE
>  /* mimic udevd's behaviour and override the systems rmem_max limit in 
> case
>   * there's a significant number of device 'add' events
>   */
>  if (geteuid() == 0)
>  udev_monitor_set_receive_buffer_size(priv->udev_monitor,
>   128 * 1024 * 1024);
> +#endif
> 
>  /* We register the monitor with the event callback so we are
>   * notified by udev of device changes before we enumerate existing
> --
> 2.13.0
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

libvir-list@redhat.com

[Daniel P. Berrange]

On Wed, May 31, 2017 at 01:32:47PM +0800, ZhenweiPi wrote:
> ---
> 
>  domain.go  | 13 +++--
>  domain_test.go | 56 +---
>  2 files changed, 64 insertions(+), 5 deletions(-)
> 
> diff --git a/domain.go b/domain.go
> index 848835a..cbb22e5 100644
> --- a/domain.go
> +++ b/domain.go
> @@ -30,8 +30,10 @@ import (
>  )
>  type DomainController struct {
> - Type  string `xml:"type,attr"`
> - Index string `xml:"index,attr"`
> + Typestring `xml:"type,attr"`
> + Index   *uint  `xml:"index,attr"`
> + Model   string `xml:"model,attr,omitempty"`
> + Address *DomainAddress `xml:"address"`
>  }
>  type DomainDiskSecret struct {
> @@ -77,6 +79,8 @@ type DomainDisk struct {
>   Type string`xml:"type,attr"`
>   Device   string`xml:"device,attr"`
>   Snapshot string`xml:"snapshot,attr,omitempty"`
> + Cachestring`xml:"cache,attr,omitempty"`
> + Io   string`xml:"io,attr,omitempty"`
>   Driver   *DomainDiskDriver `xml:"driver"`
>   Auth *DomainDiskAuth   `xml:"auth"`
>   Source   *DomainDiskSource `xml:"source"`
> @@ -196,8 +200,13 @@ type DomainAlias struct {
>  type DomainAddress struct {
>   Type   string `xml:"type,attr"`
>   Controller *uint  `xml:"controller,attr"`
> + Domain *uint  `xml:"domain,attr"`
>   Bus*uint  `xml:"bus,attr"`
>   Port   *uint  `xml:"port,attr"`
> + Slot   *uint  `xml:"slot,attr"`
> + Function   *uint  `xml:"function,attr"`
> + Target *uint  `xml:"target,attr"`
> + Unit   *uint  `xml:"unit,attr"`
>  }
>  type DomainChardev struct {
> diff --git a/domain_test.go b/domain_test.go
> index 265cf80..22da947 100644
> --- a/domain_test.go
> +++ b/domain_test.go
> @@ -30,6 +30,16 @@ import (
>   "testing"
>  )
> +type PciAddress struct {
> + Domain   uint
> + Bus  uint
> + Slot uint
> + Function uint
> +}
> +
> +var uhciIndex uint = 0
> +var uhciAddr = PciAddress{0, 0, 1, 2}

This struct and variables are rather pointless - just put the values
inline in the one place where they are needed

> +
>  var domainTestData = []struct {
>   Object   *Domain
>   Expected []string
> @@ -130,10 +140,12 @@ var domainTestData = []struct {
>   },
>   },
>   DomainDisk{
> - Type: "volume",
> + Type:   "volume",
>   Device: "cdrom",
> + Cache:  "none",
> + Io: "native",
>   Source: &DomainDiskSource{
> - Pool: "default",
> + Pool:   "default",
>   Volume: "myvolume",
>   },
>   Target: &DomainDiskTarget{
> @@ -174,7 +186,7 @@ var domainTestData = []struct {
>   `  `,
>   `  `,
>   ``,
> - ``,
> + ` io="native">`,
>   `   volume="myvolume">`,
>   `  `,
>   ``,
> @@ -820,6 +832,44 @@ var domainTestData = []struct {
>   ``,
>   },
>   },
> + {
> + Object: &Domain{
> + Type: "kvm",
> + Name: "test",
> + Devices: &DomainDeviceList{
> + Controllers: []DomainController{
> + DomainController{
> + Type:  "usb",
> + Index: &uhciIndex,
> + Model: "piix3-uhci",
> + Address: &DomainAddress{
> + Type: "pci",
> + Domain:   
> &uhciAddr.Domain,
> + Bus:  &uhciAddr.Bus,
> + Slot: 
> &uhciAddr.Slot,
> + Function: 
> &uhciAddr.Function,
> + },
> + },
> + DomainController{
> + Type:  "usb",
> + Index: ni

Re: [libvirt] [PATCH] maint: add sanitizers to the build process

[Daniel P. Berrange]

On Tue, May 30, 2017 at 12:28:57PM -0300, claudioandre...@gmail.com wrote:
> From: Claudio André 
> 
> Sanitizers are based on compile-time instrumentation. They are available in 
> gcc and clang for a range of supported operation systems and platforms. More 
> info at: https://github.com/google/sanitizers

Please line wrap your commit messages at 80 chars or less.


> diff --git a/m4/virt-compile-sanitizer.m4 b/m4/virt-compile-sanitizer.m4
> new file mode 100644
> index 000..a7cac31
> --- /dev/null
> +++ b/m4/virt-compile-sanitizer.m4
> @@ -0,0 +1,51 @@
> +dnl
> +dnl Check for support for Sanitizers
> +dnl Check for -fsanitize=address and -fsanitize=undefined support
> +dnl
> +dnl This library is free software; you can redistribute it and/or
> +dnl modify it under the terms of the GNU Lesser General Public
> +dnl License as published by the Free Software Foundation; either
> +dnl version 2.1 of the License, or (at your option) any later version.
> +dnl
> +dnl This library is distributed in the hope that it will be useful,
> +dnl but WITHOUT ANY WARRANTY; without even the implied warranty of
> +dnl MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +dnl Lesser General Public License for more details.
> +dnl
> +dnl You should have received a copy of the GNU Lesser General Public
> +dnl License along with this library.  If not, see
> +dnl .
> +dnl
> +
> +AC_DEFUN([LIBVIRT_COMPILE_SANITIZER],[
> +LIBVIRT_ARG_ENABLE([ASAN], [Build with address sanitizer support], [no])
> +LIBVIRT_ARG_ENABLE([UBSAN], [Build with undefined behavior sanitizer 
> support], [no])
> +
> +SAN_CFLAGS=
> +SAN_LDFLAGS=
> +
> +AS_IF([test "x$enable_asan" = "xyes"], [
> +gl_COMPILER_OPTION_IF([-fsanitize=address -fno-omit-frame-pointer], [
> +SAN_CFLAGS="-fsanitize=address -fno-omit-frame-pointer"
> +SAN_LDFLAGS="-fsanitize=address"
> +])
> +
> +AC_SUBST([SAN_CFLAGS])
> +AC_SUBST([SAN_LDFLAGS])
> +])
> +
> +AS_IF([test "x$enable_ubsan" = "xyes"], [
> +gl_COMPILER_OPTION_IF([-fsanitize=undefined 
> -fno-omit-frame-pointer], [
> +SAN_CFLAGS="$SAN_CFLAGS -fsanitize=undefined 
> -fno-omit-frame-pointer"
> +SAN_LDFLAGS="$SAN_LDFLAGS -fsanitize=undefined"
> +])
> +
> +AC_SUBST([SAN_CFLAGS])
> +AC_SUBST([SAN_LDFLAGS])
> +])
> +])
> +
> +AC_DEFUN([LIBVIRT_RESULT_SANITIZER], [
> +  AC_MSG_NOTICE([  ASan: $enable_asan])
> +  AC_MSG_NOTICE([ UBSan: $enable_ubsan])
> +])

IMHO this could just be added to m4/virt-compile-warnings.m4, rather than
needing to define new CFLAGS/LDFLAGFS variables.

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

Re: [libvirt] [PATCH 4/9] util: Add magic_marker for all virObjects

[Daniel P. Berrange]

On Tue, May 30, 2017 at 10:43:35AM -0400, John Ferlan wrote:
> 
> 
> On 05/30/2017 09:00 AM, Daniel P. Berrange wrote:
> > On Tue, May 30, 2017 at 07:38:16AM -0400, John Ferlan wrote:
> >> The virObject logic "assumes" that whatever is passed to its API's
> >> would be some sort of virObjectPtr; however, if it is not then some
> >> really bad things can happen.
> >>
> >> So far there's been only virObject{Ref|Unref}, virObject{Lock|Unlock},
> >> and virObjectIsClass and the virObject and virObjectLockable class
> >> consumers have been well behaved and code well tested. Soon there will
> >> be more consumers and one such consumer tripped over this during testing
> >> by passing a virHashTablePtr to virObjectIsClass which ends up calling
> >> virClassIsDerivedFrom using "obj->klass", which wasn't really a klass
> >> object causing one of those bad things to happen.
> >>
> >> To avoid the future possibility that a non virObject class memory was
> >> passed to some virObject* API, let's add a "magic_marker" to the base
> >> virObject class that contains a value "0xFEEDBEEF", then any place in
> >> the code which would accept or process the base opaque virObjectPtr,
> >> compare the magic_marker against 0xFEEDBEEF to make sure this is an
> >> object allocated by this code.
> >>
> >> It is still left up to the caller to handle the failed API calls just
> >> as it would be if it passed a NULL opaque pointer anyobj.
> >>
> >> Signed-off-by: John Ferlan 
> >> ---
> >>  src/util/virobject.c | 12 
> >>  src/util/virobject.h |  1 +
> >>  2 files changed, 9 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/src/util/virobject.c b/src/util/virobject.c
> >> index 9f5f187..a1934941 100644
> >> --- a/src/util/virobject.c
> >> +++ b/src/util/virobject.c
> >> @@ -47,10 +47,12 @@ struct _virClass {
> >>  virObjectDisposeCallback dispose;
> >>  };
> >>  
> >> +#define VIR_OBJECT_NOTVALID(obj) (!obj || obj->magic_marker != 0xFEEDBEEF)
> >> +
> >>  #define VIR_OBJECT_USAGE_PRINT_WARNING(anyobj, objclass)  
> >>   \
> >>  do {  
> >>   \
> >>  virObjectPtr obj = anyobj;
> >>   \
> >> -if (!obj) 
> >>   \
> >> +if (VIR_OBJECT_NOTVALID(obj)) 
> >>   \
> >>  VIR_WARN("Object %p is not a virObject class instance", 
> >> anyobj);\
> >>  else  
> >>   \
> >>  VIR_WARN("Object %p (%s) is not a %s instance",   
> >>   \
> >> @@ -212,6 +214,7 @@ virObjectNew(virClassPtr klass)
> >>  return NULL;
> >>  
> >>  obj->u.s.magic = klass->magic;
> >> +obj->magic_marker = 0xFEEDBEEF;
> >>  obj->klass = klass;
> >>  virAtomicIntSet(&obj->u.s.refs, 1);
> >>  
> >> @@ -272,7 +275,7 @@ virObjectUnref(void *anyobj)
> >>  {
> >>  virObjectPtr obj = anyobj;
> >>  
> >> -if (!obj)
> >> +if (VIR_OBJECT_NOTVALID(obj))
> >>  return false;
> >>  
> >>  bool lastRef = virAtomicIntDecAndTest(&obj->u.s.refs);
> >> @@ -289,6 +292,7 @@ virObjectUnref(void *anyobj)
> >>  /* Clear & poison object */
> >>  memset(obj, 0, obj->klass->objectSize);
> >>  obj->u.s.magic = 0xDEADBEEF;
> >> +obj->magic_marker = 0xDEADBEEF;
> >>  obj->klass = (void*)0xDEADBEEF;
> >>  VIR_FREE(obj);
> >>  }
> >> @@ -311,7 +315,7 @@ virObjectRef(void *anyobj)
> >>  {
> >>  virObjectPtr obj = anyobj;
> >>  
> >> -if (!obj)
> >> +if (VIR_OBJECT_NOTVALID(obj))
> >>  return NULL;
> >>  virAtomicIntInc(&obj->u.s.refs);
> >>  PROBE(OBJECT_REF, "obj=%p", obj);
> >> @@ -389,7 +393,7 @@ virObjectIsClass(void *anyobj,
> >>   virClassPtr klass)
> >>  {
> >>  virObjectPtr obj = anyobj;
> >> -if (!obj)
> &g

Re: [libvirt] [PATCH 5/9] util: Introduce virObjectLockableRecursiveNew

[Daniel P. Berrange]

On Tue, May 30, 2017 at 03:03:14PM +0200, Pavel Hrdina wrote:
> On Tue, May 30, 2017 at 07:38:17AM -0400, John Ferlan wrote:
> > Introduce a recursive option for the lockable object which calls
> > virMutexInitRecursive instead of virMutexInit.
> 
> We should avoid using recursive lock because they are dangerous.  I
> would rather cleanup the nwfilter code than introducing more stuff for
> recursive locks.

Last time I looked I came to the conclusion that it wasn't possible to
remove the recursive locking from nwfilter.

I agree about /not/ introducing new usage of recursive locking though.

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

Re: [libvirt] [PATCH 09/18] Add virtio-related options to interfaces

[Daniel P. Berrange]

On Tue, May 30, 2017 at 02:50:32PM +0200, Ján Tomko wrote:
> 
>   
>   
>   
> 

Feels like we could just use  'iommu=on' as the name.

Also, I'm unclear whether we actually need the 'ats' attribute.

>From the description it sounds like if we have device_iotlb=on
for the IOMMU device, then we should have ats=on too. Is there
a  reason to use ats=off when device_iotlb=on, or vica-verca.
If not, then we should just do the right thing and set ats=on
whenever we see device_iotlb=on.
 

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

Re: [libvirt] [PATCH 4/9] util: Add magic_marker for all virObjects

[Daniel P. Berrange]

On Tue, May 30, 2017 at 07:38:16AM -0400, John Ferlan wrote:
> The virObject logic "assumes" that whatever is passed to its API's
> would be some sort of virObjectPtr; however, if it is not then some
> really bad things can happen.
> 
> So far there's been only virObject{Ref|Unref}, virObject{Lock|Unlock},
> and virObjectIsClass and the virObject and virObjectLockable class
> consumers have been well behaved and code well tested. Soon there will
> be more consumers and one such consumer tripped over this during testing
> by passing a virHashTablePtr to virObjectIsClass which ends up calling
> virClassIsDerivedFrom using "obj->klass", which wasn't really a klass
> object causing one of those bad things to happen.
> 
> To avoid the future possibility that a non virObject class memory was
> passed to some virObject* API, let's add a "magic_marker" to the base
> virObject class that contains a value "0xFEEDBEEF", then any place in
> the code which would accept or process the base opaque virObjectPtr,
> compare the magic_marker against 0xFEEDBEEF to make sure this is an
> object allocated by this code.
> 
> It is still left up to the caller to handle the failed API calls just
> as it would be if it passed a NULL opaque pointer anyobj.
> 
> Signed-off-by: John Ferlan 
> ---
>  src/util/virobject.c | 12 
>  src/util/virobject.h |  1 +
>  2 files changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/src/util/virobject.c b/src/util/virobject.c
> index 9f5f187..a1934941 100644
> --- a/src/util/virobject.c
> +++ b/src/util/virobject.c
> @@ -47,10 +47,12 @@ struct _virClass {
>  virObjectDisposeCallback dispose;
>  };
>  
> +#define VIR_OBJECT_NOTVALID(obj) (!obj || obj->magic_marker != 0xFEEDBEEF)
> +
>  #define VIR_OBJECT_USAGE_PRINT_WARNING(anyobj, objclass)\
>  do {\
>  virObjectPtr obj = anyobj;  \
> -if (!obj)   \
> +if (VIR_OBJECT_NOTVALID(obj))   \
>  VIR_WARN("Object %p is not a virObject class instance", anyobj);\
>  else\
>  VIR_WARN("Object %p (%s) is not a %s instance", \
> @@ -212,6 +214,7 @@ virObjectNew(virClassPtr klass)
>  return NULL;
>  
>  obj->u.s.magic = klass->magic;
> +obj->magic_marker = 0xFEEDBEEF;
>  obj->klass = klass;
>  virAtomicIntSet(&obj->u.s.refs, 1);
>  
> @@ -272,7 +275,7 @@ virObjectUnref(void *anyobj)
>  {
>  virObjectPtr obj = anyobj;
>  
> -if (!obj)
> +if (VIR_OBJECT_NOTVALID(obj))
>  return false;
>  
>  bool lastRef = virAtomicIntDecAndTest(&obj->u.s.refs);
> @@ -289,6 +292,7 @@ virObjectUnref(void *anyobj)
>  /* Clear & poison object */
>  memset(obj, 0, obj->klass->objectSize);
>  obj->u.s.magic = 0xDEADBEEF;
> +obj->magic_marker = 0xDEADBEEF;
>  obj->klass = (void*)0xDEADBEEF;
>  VIR_FREE(obj);
>  }
> @@ -311,7 +315,7 @@ virObjectRef(void *anyobj)
>  {
>  virObjectPtr obj = anyobj;
>  
> -if (!obj)
> +if (VIR_OBJECT_NOTVALID(obj))
>  return NULL;
>  virAtomicIntInc(&obj->u.s.refs);
>  PROBE(OBJECT_REF, "obj=%p", obj);
> @@ -389,7 +393,7 @@ virObjectIsClass(void *anyobj,
>   virClassPtr klass)
>  {
>  virObjectPtr obj = anyobj;
> -if (!obj)
> +if (VIR_OBJECT_NOTVALID(obj))
>  return false;
>  
>  return virClassIsDerivedFrom(obj->klass, klass);
> diff --git a/src/util/virobject.h b/src/util/virobject.h
> index f4c292b..89f8050 100644
> --- a/src/util/virobject.h
> +++ b/src/util/virobject.h
> @@ -51,6 +51,7 @@ struct _virObject {
>  int refs;
>  } s;
>  } u;
> +unsigned int magic_marker;
>  virClassPtr klass;
>  };

I'm wondering whether this will risk re-introducing the bug fixed in

  commit fca4f2334072d87f7faeb2948e6f83201309e1b9
  Author: Eric Blake 
  Date:   Thu Dec 12 16:01:15 2013 -0700

object: require maximal alignment in base class

I'm also thinking we don't really need to have 2 magic fields in the
same struct - we already have a 'magic' field that is initialized from
the class object magic value.

Now, this existing magic is different for each object subclass - we allocate
class magic starting with

  static unsigned int magicCounter = 0xCAFE;

I'm thinking though, that we're never going to have > 65556 different
sub-classes (well at least not for a long time).

So instead of adding this new field you could just check

 ((object->u.s.magic & 0xCAFE) == 0xCAFE)


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org   

Re: [libvirt] [PATCH go-xml] Add support for Node Device with basic testing.

[Daniel P. Berrange]

On Mon, May 29, 2017 at 03:58:52AM -0400, Vladik Romanovsky wrote:
> ---
>  node_device.go  | 277 
> 
>  node_device_test.go | 111 +
>  2 files changed, 388 insertions(+)
>  create mode 100644 node_device.go
>  create mode 100644 node_device_test.go
> 
> diff --git a/node_device.go b/node_device.go
> new file mode 100644
> index 000..5375d32
> --- /dev/null
> +++ b/node_device.go
> @@ -0,0 +1,277 @@
> +/*
> + * This file is part of the libvirt-go-xml project
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a 
> copy
> + * of this software and associated documentation files (the "Software"), to 
> deal
> + * in the Software without restriction, including without limitation the 
> rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL 
> THE
> + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
> FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + *
> + * Copyright (C) 2017 Red Hat, Inc.
> + *
> + */
> +
> +package libvirtxml
> +
> +import (
> + "encoding/xml"
> + "fmt"
> +)
> +
> +type NodeDevice struct {
> + Name   string  `xml:"name"`
> + Path   string  `xml:"path,omitempty"`
> + Parent string  `xml:"parent,omitempty"`
> + Driver string  `xml:"driver>name,omitempty"`
> + Capability *CapabilityType `xml:"capability"`
> +}

A single device can have multiple capabilities.

> +
> +type CapabilityType struct {
> + Type interface{} `xml:"type,attr"`
> +}

I'm not convinced about this approach - it differs from what we've
done in other schemas, where we just have a single struct containing
the union of data from all types. The user has no idea what they
should be providing for 'Type' here since its a generic interface
type.

>
> +type IDName struct {
> + ID   string `xml:"id,attr"`
> + Name string `xml:",chardata"`
> +}
> +
> +type PciExpress struct {
> + Links []PciExpressLink `xml:"link"`
> +}
> +
> +type PciExpressLink struct {
> + Validity string  `xml:"validity,attr,omitempty"`
> + Speedfloat64 `xml:"speed,attr,omitempty"`
> + Port int `xml:"port,attr,omitempty"`
> + Widthint `xml:"width,attr,omitempty"`
> +}
> +
> +type IOMMUGroupType struct {
> + Number int `xml:"number,attr"`
> +}
> +
> +type NUMA struct {
> + Node int `xml:"node,attr"`
> +}
> +
> +type PciCapabilityType struct {
> + Domain   int `xml:"domain,omitempty"`
> + Bus  int `xml:"bus,omitempty"`
> + Slot int `xml:"slot,omitempty"`
> + Function int `xml:"function,omitempty"`
> + Product  IDName  `xml:"product,omitempty"`
> + Vendor   IDName  `xml:"vendor,omitempty"`
> + IommuGroup   IOMMUGroupType  `xml:"iommuGroup,omitempty"`
> + Numa NUMA`xml:"numa,omitempty"`
> + PciExpress   PciExpress  `xml:"pci-express,omitempty"`
> + Capabilities []PciCapability `xml:"capability,omitempty"`
> +}
> +
> +type PCIAddress struct {
> + Domain   string `xml:"domain,attr"`
> + Bus  string `xml:"bus,attr"`
> + Slot string `xml:"slot,attr"`
> + Function string `xml:"function,attr"`
> +}
> +
> +type PciCapability struct {

There's alot of inconsistency in capitalization of abbreviations
in this file.  PCI vs Pci,  Numa vs NUMA, IOMMU vs Iiommu. They
should generally always be capitalized.

> + Type string`xml:"type,attr"`
> + Address  []PCIAddress `xml:"address,omitempty"`
> + MaxCount int  `xml:"maxCount,attr,omitempty"`
> +}
> +
> +type SystemHardware struct {
> + Vendor  string `xml:"vendor"`
> + Version string `xml:"version"`
> + Serial  string `xml:"serial"`
> + UUIDstring `xml:"uuid"`
> +}
> +
> +type SystemFirmware struct {
> + Vendor  string `xml:"vendor"`
> + Version string `xml:"version"`
> + ReleaseData string `xml:"release_date"`
> +}
> +
> +type SystemCapabilityType struct {
> + Product  string `xml:"product"`
> + Hardware SystemHardware `xml:"hardware"`
> + Firmware SystemFirmware `xml:"firmware"`
> +}
> +
> +type USBDeviceCapabilityType 

Re: [libvirt] [libvirt-sandbox PATCH] Drop library/ from template name and image path

[Daniel P. Berrange]

On Sat, May 27, 2017 at 06:29:32PM +0200, Guido Günther wrote:
> If one pastes from the output of virt-sansbox-image
> 
>   $ virt-sandbox-image list
>   docker:/library/ubuntu?tag=17.04
>   docker:/library/debian?tag=latest
> 
> verbatim
> 
>   $ virt-sandbox-image run -c qemu:///session 
> docker:/library/debian?tag=latest
> 
> This fails like
> 
>   /home//.local/share/libvirt/images/library/debian:qbeilwxard.qcow2: 
> Could not create file: No such file or directory
>   Unable to start sandbox: Failed to create domain: XML error: name 
> library/debian:qbeilwxard cannot contain '/'
>   /usr/bin/virt-sandbox-image: [Errno 2] No such file or directory: 
> '/home//.local/share/libvirt/images/library/debian:qbeilwxard.qcow2'
> 
> so strip of any leading components.
> ---
> This might not be the correct fix but I hope you get the idea.

We need to replace '/' with "_" - in fact I'd suggest we replace any char
exception a-Z, 0-9 and -.


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

Re: [libvirt] [PATCH v2] nodedev: Increase the netlink socket buffer size to the one used by udev

[Daniel P. Berrange]

On Fri, May 19, 2017 at 03:16:26PM +0200, Erik Skultety wrote:
> From: "ning.bo" 
> 
> When a number of SRIOV VFs (up to 128 on Intel XL710) is created:
> for i in `seq 0 1`; do
>   echo 63 > /sys/class/net//device/sriov_numvfs
> done
> 
> libvirtd will then report "udev_monitor_receive_device returned NULL"
> error because the netlink socket buffer is not big enough (using GDB on
> libudev confirmed this with ENOBUFFS) and thus some udev events were
> dropped. This results in some devices being missing in the nodedev-list
> output. This patch overrides the system's rmem_max limit but for that,
> we need to make sure we've got root privileges.
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1450960
> 
> Signed-off-by: ning.bo 
> Signed-off-by: Erik Skultety 
> ---
> Additionally, we might want to check for the errno, providing a specific
> message if such issue occurs again in a further non-specified point in time 
> and
> return the generic, yet cryptic one for all other cases.
> 
>  src/node_device/node_device_udev.c | 7 +++
>  1 file changed, 7 insertions(+)

This has broken the build on older systems  as udev doesn't have this
method.


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

Re: [libvirt] [PATCH 0/7] extend node_device for CCW and fc_remote_port

[Daniel P. Berrange]

On Fri, May 26, 2017 at 07:11:53AM -0400, John Ferlan wrote:
> 
> 
> On 05/26/2017 03:15 AM, Bjoern Walk wrote:
> > Thanks a lot for your review John.
> > 
> > If you don't mind, you can make the adjustments you mentioned and push,
> > as I can only prepare a v2 on Monday and I wanted to get this in v3.4.0.
> > 
> > Best,
> > Bjoern
> > 
> 
> I've made the adjustments and will push this sometime today...

FYI this broke the build on OS-X


util/virfcp.c:85:5: error: implicit declaration of function 
'virReportSystemError' is invalid in C99 
[-Werror,-Wimplicit-function-declaration]

virReportSystemError(ENOSYS, "%s", _("Not supported on this platform"));

util/virfcp.c:83:1: error: no previous prototype for function 
'virSysfsIsCapableFCRport' [-Werror,-Wmissing-prototypes]

virSysfsIsCapableFCRport(const char *rport ATTRIBUTE_UNUSED)

util/virfcp.c:90:1: error: no previous prototype for function 
'virSysfsReadFCRport' [-Werror,-Wmissing-prototypes]

virSysfsReadFCRport(const char *rport ATTRIBUTE_UNUSED,


https://travis-ci.org/libvirt/libvirt/jobs/236407391

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

libvir-list@redhat.com

[Daniel P. Berrange]

On Fri, May 26, 2017 at 06:50:58PM +0800, zhenwei.pi wrote:
> ---
>  domain.go  | 13 +++--
>  domain_test.go | 31 ---
>  2 files changed, 39 insertions(+), 5 deletions(-)
> 
> diff --git a/domain.go b/domain.go
> index 848835a..1382cd0 100644
> --- a/domain.go
> +++ b/domain.go
> @@ -30,8 +30,10 @@ import (
>  )
>  
>  type DomainController struct {
> - Type  string `xml:"type,attr"`
> - Index string `xml:"index,attr"`
> + Typestring `xml:"type,attr"`
> + Index   string `xml:"index,attr"`

This should be a *uint rather than string

> + Model   string `xml:"model,attr,omitempty"`
> + Address *DomainAddress `xml:"address"`
>  }
>  
>  type DomainDiskSecret struct {
> @@ -77,6 +79,8 @@ type DomainDisk struct {
>   Type string`xml:"type,attr"`
>   Device   string`xml:"device,attr"`
>   Snapshot string`xml:"snapshot,attr,omitempty"`
> + Cachestring`xml:"cache,attr,omitempty"`
> + Io   string`xml:"io,attr,omitempty"`
>   Driver   *DomainDiskDriver `xml:"driver"`
>   Auth *DomainDiskAuth   `xml:"auth"`
>   Source   *DomainDiskSource `xml:"source"`


These look fine and you've added test coverage

> @@ -196,8 +200,13 @@ type DomainAlias struct {
>  type DomainAddress struct {
>   Type   string `xml:"type,attr"`
>   Controller *uint  `xml:"controller,attr"`
> + Domain *uint  `xml:"domain,attr"`
>   Bus*uint  `xml:"bus,attr"`
>   Port   *uint  `xml:"port,attr"`
> + Slot   *uint  `xml:"slot,attr"`
> + Function   *uint  `xml:"function,attr"`
> + Target *uint  `xml:"target,attr"`
> + Unit   *uint  `xml:"unit,attr"`
>  }

These also look fine, but need test coverage added still

>  
>  type DomainChardev struct {
> diff --git a/domain_test.go b/domain_test.go
> index 265cf80..fd6914e 100644
> --- a/domain_test.go
> +++ b/domain_test.go
> @@ -130,10 +130,12 @@ var domainTestData = []struct {
>   },
>   },
>   DomainDisk{
> - Type: "volume",
> + Type:   "volume",
>   Device: "cdrom",
> + Cache:  "none",
> + Io: "native",
>   Source: &DomainDiskSource{
> - Pool: "default",
> + Pool:   "default",
>   Volume: "myvolume",
>   },
>   Target: &DomainDiskTarget{
> @@ -174,7 +176,7 @@ var domainTestData = []struct {
>   `  `,
>   `  `,
>   ``,
> - ``,
> + ` io="native">`,
>   `   volume="myvolume">`,
>   `  `,
>   ``,
> @@ -820,6 +822,29 @@ var domainTestData = []struct {
>   ``,
>   },
>   },
> + {
> + Object: &Domain{
> + Type: "kvm",
> + Name: "test",
> + Devices: &DomainDeviceList{
> + Controllers: []DomainController{
> + DomainController{
> + Type:  "usb",
> + Index: "0",

This shoudl be an integer not string.

> + Model: "piix3-uhci",
> + },
> + },
> + },
> + },
> + Expected: []string{
> + ``,
> + `  test`,
> + `  `,
> + ` model="piix3-uhci">`,
> + `  `,
> + ``,
> + },
> + },
>  }

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

Re: [libvirt] Double deference error in libvirt_virConnectDomainEventRegisterAny

[Daniel P. Berrange]

On Fri, May 26, 2017 at 04:39:17PM +0800, fangying wrote:
> Hi,
> We'd like to report a double dereference error of 'pyobj_cbData' in 
> libvirt_virConnectDomainEventRegisterAny.
> The bug can be triggered in the situation where 'domainEventRegisterAny' 
> (the python interface of libvirt_virConnectDomainEventRegisterAny)
> is invoked and network connection is coincidently lost (likely libvirtd 
> restarted) at same time.
> 
> We get the following stacktrace when the bug is hit.
> Program terminated with signal 6, Aborted.
> #0  0x7fc45cba15d7 in raise () from /usr/lib64/libc.so.6
> #1  0x7fc45cba2cc8 in abort () from /usr/lib64/libc.so.6
> #2  0x7fc45cbe12f7 in __libc_message () from /usr/lib64/libc.so.6
> #3  0x7fc45cbe86d3 in _int_free () from /usr/lib64/libc.so.6
> #4  0x7fc45d8d292c in PyDict_Fini () from 
> /usr/lib64/libpython2.7.so.1.0
> #5  0x7fc45d94f46a in Py_Finalize () from 
> /usr/lib64/libpython2.7.so.1.0
> #6  0x7fc45d960735 in Py_Main () from /usr/lib64/libpython2.7.so.1.0
> #7  0x7fc45cb8daf5 in __libc_start_main () from /usr/lib64/libc.so.6
> #8  0x00400721 in _start ()
> 
> The double dereference of 'pyobj_cbData' is triggered in the following 
> way:
> (1) libvirt_virConnectDomainEventRegisterAny is invoked.
> (2) the event is successfully added to the event callback list 
> (virDomainEventStateRegisterClient in
> remoteConnectDomainEventRegisterAny returns 1 which means ok).
> (3) when function remoteConnectDomainEventRegisterAny is hit, network 
> connection disconnected coincidently
> (or libvirtd is restarted) in the context of function 'call' then the 
> connection is lost and the
> function 'call' failed, the branch virObjectEventStateDeregisterID is 
> therefore taken.
> (4) 'pyobj_conn' is dereferenced the 1st time in 
> libvirt_virConnectDomainEventFreeFunc.
> (5) 'pyobj_cbData' (refered to pyobj_conn) is dereferenced the 2nd time 
> in libvirt_virConnectDomainEventRegisterAny.
> (6) the double free error is triggered.

IOW,  even when virConnectDomainEventRegisterAny returns -1 there is still
a scenario in which it will trigger the free callback.

This is a bug in the libvirt C impl of virConnectDomainEventRegisterAny,
not the python bindings. If this API returns -1, it *must* guarantee
that the callbacks are never called.


> static void
> libvirt_virConnectDomainEventFreeFunc(void *opaque)
> {
> PyObject *pyobj_conn = (PyObject*)opaque;
> LIBVIRT_ENSURE_THREAD_STATE;
> Py_DECREF(pyobj_conn);  /* 1st dereference comes here */
> LIBVIRT_RELEASE_THREAD_STATE;
> }
> 
> static PyObject *
> libvirt_virConnectDomainEventRegisterAny(PyObject *self ATTRIBUTE_UNUSED,
>  PyObject *args) {
> ...
> Py_INCREF(pyobj_cbData);
> 
> LIBVIRT_BEGIN_ALLOW_THREADS;
> ret = virConnectDomainEventRegisterAny(conn, dom, eventID,
>cb, pyobj_cbData,
>
> libvirt_virConnectDomainEventFreeFunc);
> 
> if (ret < 0) {
> Py_DECREF(pyobj_cbData);   /* 2nd dereference comes here */
> }
> }
> 
> Currently we cannot find a good solution to fix this problem,
> could anyone guide us to fix it ?

You need to look at the  libvirt code to fix it, rather than python
code. The python code expectations are correct in assuming that
when ret == -1, it must free the data.

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

Re: [libvirt] [PATCH] rpc: Allow up to 256K records to be returned from virConnectGetAllDomainStats.

[Daniel P. Berrange]

On Fri, May 26, 2017 at 01:29:20PM +0100, Richard W.M. Jones wrote:
> In commit 89a706681cb3a4aa003d920db3163b809cfbc9ca, the returned
> array of stats is limited to REMOTE_DOMAIN_LIST_MAX entries (4096).
> 
> As well as being far too low -- this breaks if a single guest is added
> with 320 disks -- it also seems as if use of REMOTE_DOMAIN_LIST_MAX
> was a mistake, and it should be using
> REMOTE_CONNECT_GET_ALL_DOMAIN_STATS_MAX instead.
> 
> However REMOTE_CONNECT_GET_ALL_DOMAIN_STATS_MAX is also too low (also
> 4096), so this patch also increases the limit to something more
> sensible, allowing us to cope with lots of stats from lots of domains
> in future.  (This limit could be increased further quite easily since
> each stats record takes about 32 bytes, and the maximum message size
> is currently 32 MB, so we could increase the limit by another factor
> of 4 without touching the maximum message size).

262144 stats per guest, 16384 guests per host, 32 bytes per stat
gives 128 GBs of memory actually :-)

Regardless though, raising the max stats per guest is still fine
IMHO - it just means if you have lots of guests, all with lots
of stats you'll still need to batch them long before you hit
this new max stats limit.

> 
> I tested this using a guest with 320, 500 and 1000 disks with no issues.
> 
> Signed-off-by: Richard W.M. Jones 
> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1440683
> ---
>  src/remote/remote_protocol.x | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x
> index 25e62a181..142508713 100644
> --- a/src/remote/remote_protocol.x
> +++ b/src/remote/remote_protocol.x
> @@ -233,7 +233,7 @@ const REMOTE_DOMAIN_FSFREEZE_MOUNTPOINTS_MAX = 256;
>  const REMOTE_NETWORK_DHCP_LEASES_MAX = 65536;
>  
>  /* Upper limit on count of parameters returned via bulk stats API */
> -const REMOTE_CONNECT_GET_ALL_DOMAIN_STATS_MAX = 4096;
> +const REMOTE_CONNECT_GET_ALL_DOMAIN_STATS_MAX = 262144;
>  
>  /* Upper limit of message size for tunable event. */
>  const REMOTE_DOMAIN_EVENT_TUNABLE_MAX = 2048;
> @@ -3264,7 +3264,7 @@ struct remote_domain_event_callback_agent_lifecycle_msg 
> {
>  };
>  
>  struct remote_connect_get_all_domain_stats_ret {
> -remote_domain_stats_record retStats;
> +remote_domain_stats_record 
> retStats;
>  };

There is one retStats entry per guest domain, so using DOMAIN_LIST_MAX
is right here.

The remote_domain_stats_record entry then contains N statistics
per guest. So we should only need the first hunk of this patch

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

Re: [libvirt] Remotable Libvirt

[Daniel P. Berrange]

On Thu, May 25, 2017 at 10:26:47AM -0700, Peter wrote:
> As far as I know libvirt doesn't currently have a remoteable API. It does
> have a daemon that communicates with clients via a XDR RPC.
> (https://libvirt.org/internals/rpc.html) However from what I'm hearing the
> RPC is considered an internal implementation and shouldn't be used by
> external applications. Is that still the case? Is there any chance of
> getting talking the daemon directly using the XDR standard for a subset of
> methods blessed as part of the externally supported API?

That's correct - the XDR protocol is a private implementation detail
between the libvirtd daemon and libvirt client library. The /only/
supported way of using libvirt is via the client library API, or one
of the many language bindings written on top of it. A number of the
libvirt virtualization drivers are implemented entirely in the client
library and don't involve libvirtd at all.

> An alternative is to implement a standards based remotable API, using
> something like dbus or REST, that can be used by external applications.
> I imagine that this would be at a bit of a higher level than the current RPC
> and contain at least some of the logic around the actions it performs rather
> than being a direct passthrough to the daemon.
> 
> Of course that is a pretty big undertaking and would, in my opinion, only be
> worth it if there is broader interest in the community and use cases beyond
> what cockpit would like to.

I imagine any DBus API would be a pretty straightforward 1-1 mapping from
the C api to DBus methods & signals. For a REST API I think you would quite
possibly want to build something higher level which will involve more design
thought.

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

Re: [libvirt] [PATCH 1/1] CI: add code coverage analysis

[Daniel P. Berrange]

On Thu, May 25, 2017 at 01:14:36AM -0300, claudioandre...@gmail.com wrote:
> From: Claudio André 
> 
> It builds the code coverage report and uploads the coverage data to a web 
> service in order to allow to track libvirt's code coverage over time.
> ---
>  .travis.yml | 11 ++-
>  README.md   |  1 +
>  2 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/.travis.yml b/.travis.yml
> index 266..24b8d6c 100644
> --- a/.travis.yml
> +++ b/.travis.yml
> @@ -39,6 +39,7 @@ addons:
>- dnsmasq-base
>- librbd-dev
>- w3c-dtd-xhtml
> +  - lcov
>  
>  notifications:
>irc:
> @@ -64,7 +65,8 @@ before_install:
>  
>  # the custom PATH is just to pick up OS-X homebrew & its harmless on Linux
>  before_script:
> -  - PATH="/usr/local/opt/gettext/bin:/usr/local/opt/rpcgen/bin:$PATH" 
> ./autogen.sh
> +  - PATH="/usr/local/opt/gettext/bin:/usr/local/opt/rpcgen/bin:$PATH" 
> ./autogen.sh "$COVERAGE"
> +  - gem install coveralls-lcov

Does this actually work on OS-X ? The fact that you only enabled
testing on Trusty suggests not, and if so you shouldn't run this
in the common path.


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

Re: [libvirt] [PATCH v2 3/3] virQEMUDriverDomainABIStability: Check for memoryBacking

[Daniel P. Berrange]

On Wed, May 24, 2017 at 04:45:57PM +0200, Michal Privoznik wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1450349
> 
> Problem is, qemu fails to load guest memory image if these
> attribute change on migration/restore from an image.

[snip]

> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 35fd79de8..c1ff8ca8a 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -5797,6 +5797,46 @@ qemuDomainUpdateMemoryDeviceInfo(virQEMUDriverPtr 
> driver,
>  }
>  
>  
> +static bool
> +qemuDomainABIStabilityCheck(const virDomainDef *src,
> +const virDomainDef *dst)
> +{
> +if (src->mem.source != dst->mem.source) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +   _("Target memoryBacking source '%s' doesn't "
> + "match source memoryBacking source'%s'"),
> +   virDomainMemorySourceTypeToString(dst->mem.source),
> +   virDomainMemorySourceTypeToString(src->mem.source));
> +return false;
> +}
> +
> +if (src->mem.access != dst->mem.access) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +   _("Target memoryBacking access '%s' doesn't "
> + "match access memoryBacking access'%s'"),
> +   virDomainMemoryAccessTypeToString(dst->mem.access),
> +   virDomainMemoryAccessTypeToString(src->mem.access));
> +return false;
> +}
> +
> +if (src->mem.allocation != dst->mem.allocation) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +   _("Target memoryBacking allocation '%s' doesn't "
> + "match allocation memoryBacking allocation'%s'"),
> +   
> virDomainMemoryAllocationTypeToString(dst->mem.allocation),
> +   
> virDomainMemoryAllocationTypeToString(src->mem.allocation));
> +return false;
> +}


Do we really need to blacklist all of these changes. I can understand that
changing the memory source would affect migration ABI, as it causes us to
use the memory backend command line config differently.

Assuming that matches though, I'm sceptical that changing 'access' or
'allocation' affects ABI.


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

Re: [libvirt] [PATCH 04/13] nodedev: Move node_device_linux_sysfs from node_driver to conf

[Daniel P. Berrange]

On Wed, May 24, 2017 at 09:45:09AM -0400, John Ferlan wrote:
> 
> 
> On 05/23/2017 04:13 AM, Bjoern Walk wrote:
> > John Ferlan  [2017-05-19, 09:08AM -0400]:
> >> Move the whole file from src/node_device into src/conf and rename the
> >> API's to have the "virNodeDevice" prefix.
> >>
> >> Signed-off-by: John Ferlan 
> >> ---
> >> src/Makefile.am|  8 
> >> .../node_device_linux_sysfs.c  | 24
> >> ++
> >> .../node_device_linux_sysfs.h  |  9 +---
> >> src/libvirt_private.syms   |  5 +
> >> src/node_device/node_device_driver.c   |  8 
> >> src/node_device/node_device_hal.c  |  4 ++--
> >> src/node_device/node_device_udev.c |  4 ++--
> >> 7 files changed, 34 insertions(+), 28 deletions(-)
> >> rename src/{node_device => conf}/node_device_linux_sysfs.c (89%)
> >> rename src/{node_device => conf}/node_device_linux_sysfs.h (82%)
> >>
> > 
> > What's the reasoning for this move? It somehow doesn't feel right and it
> > will make backports harder.
> 
> "Eventually" code that is currently in node_device_driver to peruse the
> node device object list is going to move to virnodedevobj.c and there
> was some "boundary" thing about calling back into the driver that I
> don't quite recall the exact compile/link error which caused me to move
> the code. Although the commit date is relatively recent - it's more or
> less a copy of work done in Dec 2016.

This still doesn't make sense really.  The src/conf directory should
only contain code that's related to the XML parsing/formatting, and
generic object management.  It should never contain any functional
driver code, and certainly none that pokes in sysfs.


> One could also make the argument that there's nothing "driver specific"
> in the code, so why should it have ever been put in the src/node_device
> directory?  My other option would be to essentially move the guts of the
> code into virnodedevobj and call it from src/node_device/...  IDC which
> way this happens, but I just found this easier. Using gitk I've never
> had issues in the past following history of the code when a module moves
> from one directory to another.
> 
> FWIW: My first instinct was move the code to src/util like other code
> that has linux (or arch specific) pieces, but I couldn't do that because
> node_device_linux_sysfs.h references "node_device_conf.h". Inclusion of
> conf/*.h has been disallowed from src/util.

To get around that simply change the APIs so that instead of taking
the virNodeDevXXX structs as a single parameter, you just pass in
an exploded list of parameters, populated from the individual struct
fields that are needed for the methods in question.

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

[libvirt] [PATCH python] Fix error check for virDomainGetTime method

[Daniel P. Berrange]

The virDomainGetTime returns either a dict or None, but the python
glue layer for checking for '-1'. Thus it failed to raise an
exception on error.

Signed-off-by: Daniel P. Berrange 
---
 libvirt-override-virDomain.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libvirt-override-virDomain.py b/libvirt-override-virDomain.py
index fa5f75f..7c417b8 100644
--- a/libvirt-override-virDomain.py
+++ b/libvirt-override-virDomain.py
@@ -63,7 +63,7 @@
 def getTime(self, flags=0):
 """Extract information about guest time """
 ret = libvirtmod.virDomainGetTime(self._o, flags)
-if ret == -1: raise libvirtError ('virDomainGetTime() failed', 
dom=self)
+if ret == None: raise libvirtError ('virDomainGetTime() failed', 
dom=self)
 return ret
 
 def setTime(self, time=None, flags=0):
-- 
2.9.3

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

Re: [libvirt] [PATCH 2/2] qemu: don't relabel chardev source file if virtlogd is used

[Daniel P. Berrange]

On Tue, May 23, 2017 at 05:53:46PM +0200, Pavel Hrdina wrote:
> On Tue, May 23, 2017 at 04:24:13PM +0100, Daniel P. Berrange wrote:
> > On Tue, May 23, 2017 at 04:25:01PM +0200, Martin Kletzander wrote:
> > > On Mon, May 15, 2017 at 04:28:35PM +0200, Pavel Hrdina wrote:
> > > > If libvirt uses virtlogd instead of passing the file path directly
> > > > to QEMU we shouldn't relabel the chardev source file, otherwise
> > > > virtlogd will get a permission denied while reloading.
> > > > 
> > > > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=143098
> > > > 
> > > 
> > > You missed a digit and I'm too lazy to check 10 bugs for a reproducer ;)
> > > 
> > > > Signed-off-by: Pavel Hrdina 
> > > > ---
> > > > src/conf/domain_conf.c  | 20 
> > > > src/conf/domain_conf.h  |  1 +
> > > > src/qemu/qemu_command.c | 12 
> > > > src/security/security_dac.c |  6 ++
> > > > src/security/security_selinux.c |  6 ++
> > > > 5 files changed, 41 insertions(+), 4 deletions(-)
> > > > 
> > > 
> > > I see you are changing the parser code, but you are not changing the
> > > Relax-NG schema, neither any test.
> > > 
> > > > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> > > > index aa441fae3c..92f011d3a4 100644
> > > > --- a/src/conf/domain_conf.c
> > > > +++ b/src/conf/domain_conf.c
> > > > @@ -2064,6 +2064,7 @@ 
> > > > virDomainChrSourceDefCopy(virDomainChrSourceDefPtr dest,
> > > > }
> > > > 
> > > > dest->type = src->type;
> > > > +dest->skipRelabel = src->skipRelabel;
> > > > 
> > > > return 0;
> > > > }
> > > > @@ -10608,6 +10609,7 @@ 
> > > > virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def,
> > > > char *append = NULL;
> > > > char *haveTLS = NULL;
> > > > char *tlsFromConfig = NULL;
> > > > +char *skipRelabel = NULL;
> > > > int remaining = 0;
> > > > 
> > > > while (cur != NULL) {
> > > > @@ -10628,6 +10630,8 @@ 
> > > > virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def,
> > > > case VIR_DOMAIN_CHR_TYPE_UNIX:
> > > > if (!append && def->type == 
> > > > VIR_DOMAIN_CHR_TYPE_FILE)
> > > > append = virXMLPropString(cur, "append");
> > > > +if (!skipRelabel && def->type == 
> > > > VIR_DOMAIN_CHR_TYPE_FILE)
> > > > +skipRelabel = virXMLPropString(cur, 
> > > > "skipRelabel");
> > > 
> > > I'm guessing you want to keep this away from users, right?  You should
> > > not be parsing it from the XML then.  Or you should add a thing there
> > > that the XML supports.  Not just a random attribute.
> > > 
> > > Either keep this data in private structure or even better, just add the
> > > same thing as you would do with:
> > > 
> > >  
> > > 
> > > with all the details of course.  The user can see it, can supply it, old
> > > releases support it, all the stuff is there already.
> > > 
> > > I'm open to suggestions, but NACK to random "wannabe private" attributes.
> > 
> > The use of virtlogd affects many devices in guests. So we should just
> > record at the top level of the QEMU domain status, whether virtlogd
> > was used or not.
> 
> That would be one possibility, however we need to decide what to do in
> one specific case:
> 
> 1. stdio_handler is set to logd and QEMU_CAPS_CHARDEV_FILE_APPEND is
> supported
> 
> 2. start a guest with one serial chardev
> 
> 3. change the stdio_handler to file and restart libvirtd
> 
> 4. hotplug another serial chardev (this is currently broken [1])
> 
> There are to possible solution, we will store the usage of virtlogd
> domain-wide and virtlogd will be used for that domain even if the config
> option would change for the rest of that domain's life.  Or we need to
> store the usage of virtlogd per device and it will always depend on the
> config option.

Having some devices use virtlogd and some devices not use virtlogd
is just a recipe for maximising confusion of developers, users, and
support people alike.

We should record whether we used virtlogd when we first start the
guest, and honour that until QEMU is killed.

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

Re: [libvirt] [libvirt-perl][PATCH 0/7] Implement sparse streams

[Daniel P. Berrange]

On Tue, May 23, 2017 at 05:05:34PM +0200, Michal Privoznik wrote:
> *** BLURB HERE ***
> 
> Michal Privoznik (7):
>   Fix send_all() callback helper
>   Introduce flags to Stream::recv()
>   Introduce Stream::recvHole() and Stream::sendHole()
>   Introduce Stream::sparse_recv_all()
>   Introduce Stream::sparse_send_all()
>   Register VOL_DOWNLOAD_SPARSE_STREAM & VOL_UPLOAD_SPARSE_STREAM
> constants
>   examples: Introduce vol-sparse.pl
> 
>  Changes|   9 ++
>  Virt.xs| 241 
> -
>  examples/vol-sparse.pl | 142 ++
>  lib/Sys/Virt/StorageVol.pm |  30 +-
>  lib/Sys/Virt/Stream.pm |  70 -
>  t/030-api-coverage.t   |   3 +
>  6 files changed, 483 insertions(+), 12 deletions(-)
>  create mode 100755 examples/vol-sparse.pl

ACK with the couple of renames suggested


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

Re: [libvirt] [libvirt-perl][PATCH 5/7] Introduce Stream::sparse_send_all()

[Daniel P. Berrange]

On Tue, May 23, 2017 at 05:05:39PM +0200, Michal Privoznik wrote:
> Signed-off-by: Michal Privoznik 
> ---
>  Changes|   1 +
>  Virt.xs| 124 
> +
>  lib/Sys/Virt/Stream.pm |  19 
>  t/030-api-coverage.t   |   2 +
>  4 files changed, 146 insertions(+)
> 
> diff --git a/Changes b/Changes
> index 4c1e071..989b795 100644
> --- a/Changes
> +++ b/Changes
> @@ -8,6 +8,7 @@ Revision history for perl module Sys::Virt
> register RECV_STOP_AT_HOLE constant
>   - Introduce Stream::recvHole() and Stream::sendHole()
>   - Introduce Stream::sparse_recv_all()
> + - Introduce Stream::sparse_send_all()
>  
>  3.3.0 2017-05-08
>  
> diff --git a/Virt.xs b/Virt.xs
> index 002bd6a..fc282b8 100644
> --- a/Virt.xs
> +++ b/Virt.xs
> @@ -1960,6 +1960,100 @@ _stream_send_all_source(virStreamPtr st,
>  }
>  
>  
> +static int
> +_stream_sparse_send_all_holeHandler(virStreamPtr st,
> +int *inData,
> +long long *length,
> +void *opaque)

Use _handler, rather than Handler - and the same elsewhere in this
patch.

> +{
> +AV *av = opaque;
> +SV **self;
> +SV **holeHandler;
> +SV *inDataSV;
> +SV *lengthSV;
> +int count;
> +int ret;
> +dSP;
> +
> +self = av_fetch(av, 0, 0);
> +holeHandler = av_fetch(av, 2, 0);
> +
> +SvREFCNT_inc(*self);
> +
> +ENTER;
> +SAVETMPS;
> +
> +PUSHMARK(SP);
> +XPUSHs(*self);
> +PUTBACK;
> +
> +count = call_sv((SV*)*holeHandler, G_ARRAY);
> +
> +SPAGAIN;
> +
> +if (count == 2) {
> +/* @holeHandler returns (inData, length), but on a stack.
> + * Therefore the order is reversed. */
> +lengthSV = POPs;
> +inDataSV = POPs;
> +*inData = virt_SvIVll(inDataSV);
> +*length = virt_SvIVll(lengthSV);
> +ret = 0;
> +} else {
> +ret = -1;
> +}
> +
> +PUTBACK;
> +FREETMPS;
> +LEAVE;
> +
> +return ret;
> +}
> +
> +
> +static int
> +_stream_sparse_send_all_skipHandler(virStreamPtr st,
> +long long length,
> +void *opaque)
> +{
> +AV *av = opaque;
> +SV **self;
> +SV **skipHandler;
> +int rv;
> +int ret;
> +dSP;
> +
> +self = av_fetch(av, 0, 0);
> +skipHandler = av_fetch(av, 3, 0);
> +
> +SvREFCNT_inc(*self);
> +
> +ENTER;
> +SAVETMPS;
> +
> +PUSHMARK(SP);
> +XPUSHs(*self);
> +XPUSHs(sv_2mortal(virt_newSVll(length)));
> +PUTBACK;
> +
> +rv = call_sv((SV*)*skipHandler, G_SCALAR);
> +
> +SPAGAIN;
> +
> +if (rv == 1) {
> +ret = POPi;
> +} else {
> +ret = -1;
> +}
> +
> +PUTBACK;
> +FREETMPS;
> +LEAVE;
> +
> +return ret;
> +}
> +
> +
>  static int
>  _stream_recv_all_sink(virStreamPtr st,
>const char *data,
> @@ -8041,6 +8135,36 @@ sparse_recv_all(stref, handler, holeHandler)
>  
>SvREFCNT_dec(opaque);
>  
> +void
> +sparse_send_all(stref, handler, holeHandler, skipHandler)
> +  SV *stref;
> +  SV *handler;
> +  SV *holeHandler;
> +  SV *skipHandler;
> + PREINIT:
> +  AV *opaque;
> +  virStreamPtr st;
> +CODE:
> +  st = (virStreamPtr)SvIV((SV*)SvRV(stref));
> +
> +  opaque = newAV();
> +  SvREFCNT_inc(stref);
> +  SvREFCNT_inc(handler);
> +  SvREFCNT_inc(holeHandler);
> +  SvREFCNT_inc(skipHandler);
> +  av_push(opaque, stref);
> +  av_push(opaque, handler);
> +  av_push(opaque, holeHandler);
> +  av_push(opaque, skipHandler);
> +
> +  if (virStreamSparseSendAll(st,
> + _stream_send_all_source,
> + _stream_sparse_send_all_holeHandler,
> + _stream_sparse_send_all_skipHandler,
> + opaque) < 0)
> +  _croak_error();
> +
> +  SvREFCNT_dec(opaque);
>  
>  void
>  add_callback(stref, events, cb)
> diff --git a/lib/Sys/Virt/Stream.pm b/lib/Sys/Virt/Stream.pm
> index c32b957..e3b25a5 100644
> --- a/lib/Sys/Virt/Stream.pm
> +++ b/lib/Sys/Virt/Stream.pm
> @@ -144,6 +144,25 @@ bytes). The C<$holeHandler> is expected to return a 
> non-negative
>  number on success (usually 0) and a negative number (usually -1)
>  otherwise.
>  
> +=item $st->sparse_send_all($handler, $holeHandler, $skipHandler)
> +
> +Send all data produced by C<$handler> to the stream.  The
> +C<$handler> parameter must be a function which expects three
> +arguments, the C<$st> stream object, a scalar which must be
> +filled with data and a maximum data byte count desired.  The
> +function should return the number of bytes filled, 0 on end of
> +file, or -1 upon error. The second argument C<$holeHandler> is a
> +function expecting just one argument C<$st> and returning an
> +array of two elements (C<

Re: [libvirt] [libvirt-perl][PATCH 3/7] Introduce Stream::recvHole() and Stream::sendHole()

[Daniel P. Berrange]

On Tue, May 23, 2017 at 05:05:37PM +0200, Michal Privoznik wrote:
> Signed-off-by: Michal Privoznik 
> ---
>  Changes|  1 +
>  Virt.xs| 28 
>  lib/Sys/Virt/Stream.pm | 17 +
>  3 files changed, 46 insertions(+)
> 
> diff --git a/Changes b/Changes
> index b4a493c..c92c271 100644
> --- a/Changes
> +++ b/Changes
> @@ -6,6 +6,7 @@ Revision history for perl module Sys::Virt
>   - Fix send_all() callback helper
>   - Introduce flags to Stream::recv() and
> register RECV_STOP_AT_HOLE constant
> + - Introduce Stream::recvHole() and Stream::sendHole()
>  
>  3.3.0 2017-05-08
>  
> diff --git a/Virt.xs b/Virt.xs
> index 498e711..d112708 100644
> --- a/Virt.xs
> +++ b/Virt.xs
> @@ -7900,6 +7900,34 @@ recv(st, data, nbytes, flags=0)
>RETVAL
>  
>  
> +SV *
> +recvHole(st, flags=0)
> +  virStreamPtr st;
> +  unsigned int flags;
> +  PREINIT:
> +  long long length;
> +CODE:
> +  if (virStreamRecvHole(st, &length, flags) < 0)
> +  _croak_error();
> +
> +  RETVAL = virt_newSVll(length);
> +  OUTPUT:
> +  RETVAL
> +
> +
> +void
> +sendHole(st, lengthSV, flags=0)
> +  virStreamPtr st;
> +  SV *lengthSV;
> +  unsigned int flags;
> + PREINIT:
> +  long long length;
> +  PPCODE:
> +  length = virt_SvIVll(lengthSV);
> +  if (virStreamSendHole(st, length, flags) < 0)
> +  _croak_error();
> +
> +
>  void
>  send_all(stref, handler)
>SV *stref;
> diff --git a/lib/Sys/Virt/Stream.pm b/lib/Sys/Virt/Stream.pm
> index 069895e..5984fb6 100644
> --- a/lib/Sys/Virt/Stream.pm
> +++ b/lib/Sys/Virt/Stream.pm
> @@ -93,6 +93,23 @@ Send upto C<$nbytes> worth of data, copying from C<$data>.
>  Returns the number of bytes sent, or -2 if I/O would block,
>  or -1 on error.
>  
> +=item $rv = $st->recvHole($flags=0)
> +
> +Determine the amount of the empty space (in bytes) to be created
> +in a stream's target file when uploading or downloading sparsely
> +populated files. This is the counterpart to C. The
> +optional C<$flags> parameter is currently unused and defaults to
> +zero if omitted.
> +
> +=item $st->sendHole($length, $flags=0)
> +
> +Rather than transmitting empty file space, this method directs
> +the stream target to create C<$length> bytes of empty space.
> +This method would be used when uploading or downloading sparsely
> +populated files to avoid the needless copy of empty file space.
> +The optional C<$flags> parameter is currently unused and defaults
> +to zero if omitted.

These methods should use '_' rather than camelCase in naming.


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

Re: [libvirt] [PATCH 2/2] qemu: don't relabel chardev source file if virtlogd is used

[Daniel P. Berrange]

On Tue, May 23, 2017 at 04:25:01PM +0200, Martin Kletzander wrote:
> On Mon, May 15, 2017 at 04:28:35PM +0200, Pavel Hrdina wrote:
> > If libvirt uses virtlogd instead of passing the file path directly
> > to QEMU we shouldn't relabel the chardev source file, otherwise
> > virtlogd will get a permission denied while reloading.
> > 
> > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=143098
> > 
> 
> You missed a digit and I'm too lazy to check 10 bugs for a reproducer ;)
> 
> > Signed-off-by: Pavel Hrdina 
> > ---
> > src/conf/domain_conf.c  | 20 
> > src/conf/domain_conf.h  |  1 +
> > src/qemu/qemu_command.c | 12 
> > src/security/security_dac.c |  6 ++
> > src/security/security_selinux.c |  6 ++
> > 5 files changed, 41 insertions(+), 4 deletions(-)
> > 
> 
> I see you are changing the parser code, but you are not changing the
> Relax-NG schema, neither any test.
> 
> > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> > index aa441fae3c..92f011d3a4 100644
> > --- a/src/conf/domain_conf.c
> > +++ b/src/conf/domain_conf.c
> > @@ -2064,6 +2064,7 @@ virDomainChrSourceDefCopy(virDomainChrSourceDefPtr 
> > dest,
> > }
> > 
> > dest->type = src->type;
> > +dest->skipRelabel = src->skipRelabel;
> > 
> > return 0;
> > }
> > @@ -10608,6 +10609,7 @@ 
> > virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def,
> > char *append = NULL;
> > char *haveTLS = NULL;
> > char *tlsFromConfig = NULL;
> > +char *skipRelabel = NULL;
> > int remaining = 0;
> > 
> > while (cur != NULL) {
> > @@ -10628,6 +10630,8 @@ 
> > virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def,
> > case VIR_DOMAIN_CHR_TYPE_UNIX:
> > if (!append && def->type == VIR_DOMAIN_CHR_TYPE_FILE)
> > append = virXMLPropString(cur, "append");
> > +if (!skipRelabel && def->type == 
> > VIR_DOMAIN_CHR_TYPE_FILE)
> > +skipRelabel = virXMLPropString(cur, "skipRelabel");
> 
> I'm guessing you want to keep this away from users, right?  You should
> not be parsing it from the XML then.  Or you should add a thing there
> that the XML supports.  Not just a random attribute.
> 
> Either keep this data in private structure or even better, just add the
> same thing as you would do with:
> 
>  
> 
> with all the details of course.  The user can see it, can supply it, old
> releases support it, all the stuff is there already.
> 
> I'm open to suggestions, but NACK to random "wannabe private" attributes.

The use of virtlogd affects many devices in guests. So we should just
record at the top level of the QEMU domain status, whether virtlogd
was used or not.


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

Re: [libvirt] [RFC PATCH 2/2] lib: Add note that bulk stats API queries may overrun RPC buffers

[Daniel P. Berrange]

On Tue, May 23, 2017 at 10:51:47AM +0200, Martin Kletzander wrote:
> On Tue, May 23, 2017 at 09:32:03AM +0100, Daniel P. Berrange wrote:
> > On Mon, May 22, 2017 at 06:00:13PM +0200, Peter Krempa wrote:
> > > Hint that the users should limit the number of VMs queried in the bulk
> > > stats API.
> > > ---
> > >  src/libvirt-domain.c | 7 +++
> > >  1 file changed, 7 insertions(+)
> > > 
> > > diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
> > > index 310b91b37..b01f2705c 100644
> > > --- a/src/libvirt-domain.c
> > > +++ b/src/libvirt-domain.c
> > > @@ -11315,6 +11315,10 @@ virConnectGetDomainCapabilities(virConnectPtr 
> > > conn,
> > >   * VIR_CONNECT_GET_ALL_DOMAINS_STATS_SHUTOFF and/or
> > >   * VIR_CONNECT_GET_ALL_DOMAINS_STATS_OTHER for all other states.
> > >   *
> > > + * Note that this API is prone to exceeding maximum RPC message size on 
> > > hosts
> > > + * with lots of VMs so it's suggested to use virDomainListGetStats with a
> > > + * reasonable list of VMs as the argument.
> > > + *
> > >   * Returns the count of returned statistics structures on success, -1 on 
> > > error.
> > >   * The requested data are returned in the @retStats parameter. The 
> > > returned
> > >   * array should be freed by the caller. See virDomainStatsRecordListFree.
> > > @@ -11386,6 +11390,9 @@ virConnectGetAllDomainStats(virConnectPtr conn,
> > >   * Note that any of the domain list filtering flags in @flags may be 
> > > rejected
> > >   * by this function.
> > >   *
> > > + * Note that this API is prone to exceeding maximum RPC message size on 
> > > hosts
> > > + * with lots of VMs so it's suggested to limit the number of VMs queried.
> > 
> > With the way this is worded, applications have no guidance on what is a
> > sensible max number of VMs they can safely use, so I don't think it is
> > particularly useful, except as a way to say "we told you so" when apps
> > report a bug.
> > 
> > I'd be inclined to explicitly put a limit of 100 VMs[1] in the API, and
> > document that hard limit, so apps immediately know to write their code
> > to chunk in 100 VM blocks.
> > 
> 
> Or we can give a hint on what the limit is and let users figure out the
> sensible number.  It is not only based on the number of VMs.  I believe
> you can hit the limit with one or two VMs if you have lot of devices
> that we report statistics for.  Limiting the number of VMs to a
> particular number would not help as much in this case.  But we can
> combine both approaches.

Urgh, that's even worse. Apps can't simply split queries into blocks of
N vms, because any single VM in that list might have huge number of disks.
So to use this at all reliably you have to query the XML config of every
guest to see what devices are present and then split up the queries into
variable number of VMs :-(

This just adds to my feeling that we should consider this API a failed
experiment

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

Re: [libvirt] [RFC PATCH 2/2] lib: Add note that bulk stats API queries may overrun RPC buffers

[Daniel P. Berrange]

On Mon, May 22, 2017 at 06:00:13PM +0200, Peter Krempa wrote:
> Hint that the users should limit the number of VMs queried in the bulk
> stats API.
> ---
>  src/libvirt-domain.c | 7 +++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
> index 310b91b37..b01f2705c 100644
> --- a/src/libvirt-domain.c
> +++ b/src/libvirt-domain.c
> @@ -11315,6 +11315,10 @@ virConnectGetDomainCapabilities(virConnectPtr conn,
>   * VIR_CONNECT_GET_ALL_DOMAINS_STATS_SHUTOFF and/or
>   * VIR_CONNECT_GET_ALL_DOMAINS_STATS_OTHER for all other states.
>   *
> + * Note that this API is prone to exceeding maximum RPC message size on hosts
> + * with lots of VMs so it's suggested to use virDomainListGetStats with a
> + * reasonable list of VMs as the argument.
> + *
>   * Returns the count of returned statistics structures on success, -1 on 
> error.
>   * The requested data are returned in the @retStats parameter. The returned
>   * array should be freed by the caller. See virDomainStatsRecordListFree.
> @@ -11386,6 +11390,9 @@ virConnectGetAllDomainStats(virConnectPtr conn,
>   * Note that any of the domain list filtering flags in @flags may be rejected
>   * by this function.
>   *
> + * Note that this API is prone to exceeding maximum RPC message size on hosts
> + * with lots of VMs so it's suggested to limit the number of VMs queried.

With the way this is worded, applications have no guidance on what is a
sensible max number of VMs they can safely use, so I don't think it is
particularly useful, except as a way to say "we told you so" when apps
report a bug.

I'd be inclined to explicitly put a limit of 100 VMs[1] in the API, and
document that hard limit, so apps immediately know to write their code
to chunk in 100 VM blocks.

Regards,
Daniel

[1] 100 is entirely arbitrary for this email - we'd actually pick a number
based on what we reasonably expect to fit in the RPC message size.
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

Re: [libvirt] [RFC PATCH 2/2] lib: Add note that bulk stats API queries may overrun RPC buffers

[Daniel P. Berrange]

On Tue, May 23, 2017 at 10:12:06AM +0200, Martin Kletzander wrote:
> On Mon, May 22, 2017 at 06:00:13PM +0200, Peter Krempa wrote:
> > Hint that the users should limit the number of VMs queried in the bulk
> > stats API.
> > ---
> > src/libvirt-domain.c | 7 +++
> > 1 file changed, 7 insertions(+)
> > 
> > diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
> > index 310b91b37..b01f2705c 100644
> > --- a/src/libvirt-domain.c
> > +++ b/src/libvirt-domain.c
> > @@ -11315,6 +11315,10 @@ virConnectGetDomainCapabilities(virConnectPtr conn,
> >  * VIR_CONNECT_GET_ALL_DOMAINS_STATS_SHUTOFF and/or
> >  * VIR_CONNECT_GET_ALL_DOMAINS_STATS_OTHER for all other states.
> >  *
> > + * Note that this API is prone to exceeding maximum RPC message size on 
> > hosts
> > + * with lots of VMs so it's suggested to use virDomainListGetStats with a
> > + * reasonable list of VMs as the argument.
> > + *
> >  * Returns the count of returned statistics structures on success, -1 on 
> > error.
> >  * The requested data are returned in the @retStats parameter. The returned
> >  * array should be freed by the caller. See virDomainStatsRecordListFree.
> > @@ -11386,6 +11390,9 @@ virConnectGetAllDomainStats(virConnectPtr conn,
> >  * Note that any of the domain list filtering flags in @flags may be 
> > rejected
> >  * by this function.
> >  *
> > + * Note that this API is prone to exceeding maximum RPC message size on 
> > hosts
> > + * with lots of VMs so it's suggested to limit the number of VMs queried.
> > + *
> 
> Makes sense to me, now we can at least point people somewhere when this
> happens again.  ACK from me, but feel free to wait if you want to really
> make this an RFC and you want more opinions.
> 
> Is there a discussion about how to continue for the future?  I remember
> Michal starting some discussion, but I'm not sure whether that was about
> the same thing.  Anyway, that's just a rhetorical question so that I can
> say the two ideas I have:
> 
> 1) Just increase the limit over time.  Computers and networks are
>getting faster, there's more storage space and memory, and so on.
>It only makes sense to do scale other things respectively.
> 
> 2) Have a new API that streams the data back over virStream.  We can
>then do it continuously (like every X seconds), that might help
>management apps as well because I suspect that's how they use this
>API anyway.
> 
> Thanks for listening ;)

Before we do either of those we should consider just changing the RPC
message format for the APIs in question.

Essentially have, say, 10 statistics we are grabbing for every VM.
On the wire we are encoding


  : 
  : 
  ...
  : 

  : 
  : 
  ...
  : 

  : 
  : 
  ...
  : 

where  is a long string that is basically the same for
every VM, while  is just an int64.

We could change this so that we have a table of names at the
start

  : 
  : 
  ...
  : 

and then for each VM we have

  : 
  : 
  ...
  : 

Soo the name string is no longer repeated, and in its place is an
integer index. If every name is say 12 characters long, and the
index is a 4 byte int, we've reduced a 20 byte packet per stat
to 12 bytes.

That's just one idea, possibly/probably not even the best - there's
various other ways we could encode the data to make it more efficient.

We could perhaps even consider changing the public API too, because
the typed parameters are a really inefficient way to exporting the
data in the API too


If you want to get more radical with a push based solution, then I
would suggest we consider setting up shared memory segment between
libvirt & the client app where we just continuously update the data,
avoiding RPC entirely.

This would only work for apps running locally of course, but at a
large scale, it seems the major apps using libvirt have all taken
the approach of having a local libvirtd connection, rather than
trying to use our remote RPC at data center scale levels from
a central host.

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

Re: [libvirt] [RFC] Fixing a regression caused by recent CPU driver changes

[Daniel P. Berrange]

On Thu, May 18, 2017 at 10:22:59AM +0200, Jiri Denemark wrote:
> The big question is how to fix the regression in a backward compatible
> way and still keep the ability to properly check guest CPU ABI with new
> enough libvirt and QEMU. Clearly, we need to keep both the original and
> the updated CPU definition (or one of them and a diff against the
> other).
> 
> I came up with the following options:
> 
> 1. When migrating a domain, the domain XML would contain the original
>CPU def while the updated one would be transferred in a migration
>cookie.
>- doesn't fix save/restore or snapshot revert
>  - could be fixed by not updating the CPU with QEMU < 2.9.0, but it
>won't work when restore is done on a different host with older
>QEMU (and yes, this happens in real life, e.g., with oVirt)
>- doesn't even fix migration after save/restore or snapshot revert

Yep, not an option.

> 
> 2. Use migration cookie and change the save image format to contain
>additional data (something like migration cookie) which a new libvirt
>could make use of while old libvirt would ignore any additional data
>it doesn't know about
>- snapshot XML would need to be updated too, but that's trivial
>- cleanly changing save image format requires its version to be
>  increased and old libvirt will just refuse to read such image
>- this would fix save/restore on the same host or on a host with
>  older QEMU
>- doesn't fix restore on a different host running older libvirt
>  - even this is done by oVirt

The only way we can change save image format, is if we ensure we use
the old format by default, and require a manual VIR_DOMAIN_SAVE_CPU_CHECK
flag to turn on the "new" format.

> 3. Use migration cookie and change the save image format without
>increasing its version (and update snapshot XML)
>- this fixes all cases
>- technically possible by using one of the unused 32b ints and
>  adding \0 at the end of the domain XML followed by anothe XML with
>  the additional data (pointed to by the formerly unused uint32_t)
>- very ugly hack

While it seems ugly, this is kind of what the 'unused' ints are
there for in the first place.

Basically for required incompatible changes we'd bump version, but
for safe opt-in changes we can just use an unused field. So I think
this is ok


> 4. Format both CPUs in domain XML by adding a new subelement in 
>which would list all extra features disabled or enabled by QEMU
>- fixes all cases without the need to use migration cookie, change
>  save image format, or snapshot XML
>- but it may be very confusing for users and it would need to be
>  documented as "output only, don't touch staff"

Yeah this just looks too ugly.

> So my preferred solution is 2. It breaks restore on a host with older
> libvirt, but it was never guaranteed to work, even though it usually
> just worked. And we could just tell oVirt to fix there usage :-) Also
> additional data in save image header may be very useful in the future; I
> think I remember someone sighing about the inability to store more than
> just domain XML in a saved image when they were trying to fix some
> compatibility issues.

I think 2 is ok, if we use the VIR_DOMAIN_SAVE_CPU_CHECK to opt-in to
writing the new format with expanded CPU.

It means OpenStack/oVirt would *not* use this flag by default - it would
need to be an opt-in once the admin knows they don't have any nodes with
older version in use.


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

Re: [libvirt] [libvirt-perl][PATCH] Add LIST_CAP_MDEV & LIST_CAP_MDEV_TYPES constants

[Daniel P. Berrange]

On Mon, May 22, 2017 at 05:23:31PM +0200, Michal Privoznik wrote:
> Signed-off-by: Michal Privoznik 
> ---
> 
> The sparse streams are still missing. Working on that.
> 
>  Changes| 2 +-
>  Virt.xs| 2 ++
>  lib/Sys/Virt/NodeDevice.pm | 8 
>  3 files changed, 11 insertions(+), 1 deletion(-)

Reviewed-by: Daniel P. Berrange 


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

[libvirt] [PATCH v3] Provide a useful README file

[Daniel P. Berrange]

The current README file contents has almost no useful info, and that
which does exist is very outdated.

Signed-off-by: Daniel P. Berrange 
---
 Makefile.am |  1 +
 README  | 14 +--
 README.md   | 80 +
 3 files changed, 82 insertions(+), 13 deletions(-)
 mode change 100644 => 12 README
 create mode 100644 README.md

diff --git a/Makefile.am b/Makefile.am
index 333ec5a..db991ba 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -40,6 +40,7 @@ EXTRA_DIST = \
   autogen.sh \
   cfg.mk \
   run.in \
+  README.md \
   AUTHORS.in
 
 pkgconfigdir = $(libdir)/pkgconfig
diff --git a/README b/README
deleted file mode 100644
index 3d5167d..000
--- a/README
+++ /dev/null
@@ -1,13 +0,0 @@
-
- LibVirt : simple API for virtualization
-
-  Libvirt is a C toolkit to interact with the virtualization capabilities
-of recent versions of Linux (and other OSes). It is free software
-available under the GNU Lesser General Public License. Virtualization of
-the Linux Operating System means the ability to run multiple instances of
-Operating Systems concurrently on a single hardware system where the basic
-resources are driven by a Linux instance. The library aim at providing
-long term stable C API initially for the Xen paravirtualization but
-should be able to integrate other virtualization mechanisms if needed.
-
-Daniel Veillard 
diff --git a/README b/README
new file mode 12
index 000..42061c0
--- /dev/null
+++ b/README
@@ -0,0 +1 @@
+README.md
\ No newline at end of file
diff --git a/README.md b/README.md
new file mode 100644
index 000..f5d4cd2
--- /dev/null
+++ b/README.md
@@ -0,0 +1,80 @@
+[![Build 
Status](https://travis-ci.org/libvirt/libvirt.svg)](https://travis-ci.org/libvirt/libvirt)
+
+Libvirt API for virtualization
+==
+
+Libvirt provides a portable, long term stable C API for managing the
+virtualization technologies provided by many operating systems. It
+includes support for QEMU, KVM, Xen, LXC, bhyve, Virtuozzo, VMware
+vCenter and ESX, VMware Desktop, Hyper-V, VirtualBox and the POWER
+Hypervisor.
+
+For some of these hypervisors, it provides a stateful management
+daemon which runs on the virtualization host allowing access to the
+API both by non-privileged local users and remote users.
+
+Layered packages provide bindings of the libvirt C API into other
+languages including Python, Perl, PHP, Go, Java, OCaml, as well as
+mappings into object systems such as GObject, CIM and SNMP.
+
+Further information about the libvirt project can be found on the
+website:
+
+*  <https://libvirt.org>
+
+License
+---
+
+The libvirt C API is distributed under the terms of GNU Lesser General
+Public License, version 2.1 (or later). Some parts of the code that are
+not part of the C library may have the more restrictive GNU General
+Public License, version 2.1 (or later). See the files COPYING.LESSER
+and COPYING for full license terms & conditions.
+
+Installation
+
+
+Libvirt uses the GNU Autotools build system, so in general can be built
+and installed with the usual commands. For example, to build in a manner
+that is suitable for installing as root, use:
+
+```
+$ ./configure --prefix=/usr --sysconfdir=/etc --localstatedir=/var
+$ make
+$ sudo make install
+```
+
+While to build & install as an unprivileged user
+
+```
+$ ./configure --prefix=$HOME/usr
+$  make
+$  make install
+```
+
+
+The libvirt code relies on a large number of 3rd party libraries. These will
+be detected during execution of the configure script and a summary printed
+which lists any missing (optional) dependencies.
+
+Contributing
+
+
+The libvirt project welcomes contributions in many ways. For most components
+the best way to contribute is to send patches to the primary development
+mailing list, using the `git send-email` command. Further guidance on this
+can be found in the `HACKING` file, or the project website
+
+* <https://libvirt.org/contribute.html>
+
+Contact
+---
+
+The libvirt project has two primary mailing lists:
+
+ * libvirt-us...@redhat.com (**for user discussions**)
+ * libvir-list@redhat.com (**for development only**)
+
+Further details on contacting the project are available on the website
+
+* <https://libvirt.org/contact.html>
-- 
2.9.3

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

Re: [libvirt] [PATCH v2] Provide a useful README file

[Daniel P. Berrange]

On Mon, May 22, 2017 at 02:00:01PM +0200, Andrea Bolognani wrote:
> On Tue, 2017-05-16 at 13:29 +0100, Daniel P. Berrange wrote:
> > The current README file contents has almost no useful info, and that
> > which does exist is very outdated.
> > 
> > Signed-off-by: Daniel P. Berrange 
> > ---
> > 
> > In v2:
> > 
> >  - Use markdown syntax
> >  - Use README.md file
> 
> My preference would be to call this README.markdown instead.

While it appears github supports both names, README.md appears
more commonly used to me - indeed github's own markup repor
uses that name

  https://github.com/github/markup/blob/master/README.md


> [...]
> > +Libvirt API for virtualization
> > +==
> > +
> > +Libvirt provides a portable, long term stable C API for managing the
> 
> I like using "libvirt" with a capital L consistently, even
> when it's at the beginning of a sentence. I think there might
> be style rules agains it, though.

To me it looks pretty odd to not captialize a word at the start
of the sentence.


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

Re: [libvirt] [PATCH] Revert "qemu: propagate bridge MTU into qemu "host_mtu" option"

[Daniel P. Berrange]

On Mon, May 22, 2017 at 03:30:48AM -0400, Laine Stump wrote:
> This reverts commit 2841e675.
> 
> It turns out that adding the host_mtu field to the PCI capabilities in
> the guest bumps the length of PCI capabilities beyond the 32 byte
> boundary, so the virtio-net device gets 64 bytes of ioport space
> instead of 32, which offsets the address of all the other following
> devices. Migration doesn't work very well when the location and length
> of PCI capabilities of devices is changed between source and
> destination.
> 
> This means that we need to make sure that the absence/presence of
> host_mtu on the qemu commandline always matches between source and
> destination, which means that we need to make setting of host_mtu an
> opt-in thing (it can't happen automatically when the bridge being used
> has a non-default MTU, which is what commit 2841e675 implemented).
> 
> I do want to re-implement this feature with an 
> setting, but probably won't backport that to any stable branches, so
> I'm first reverting the original commit, and that revert can be pushed
> to the few releases that have been made since the original (3.1.0 -
> 3.3.0)
> 
> Resolves: https://bugzilla.redhat.com/1449346
> ---
>  src/qemu/qemu_command.c   | 32 ++--
>  src/qemu/qemu_command.h   |  3 +--
>  src/qemu/qemu_hotplug.c   |  5 ++---
>  src/qemu/qemu_interface.c |  5 ++---
>  src/qemu/qemu_interface.h |  3 +--
>  5 files changed, 16 insertions(+), 32 deletions(-)

ACK, please push to all relevant branches too


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

Re: [libvirt] [PATCH v1 0/3] Loadparm support

[Daniel P. Berrange]

On Fri, May 19, 2017 at 12:56:44PM -0400, Farhan Ali wrote:
> This patch series introduces the support for new s390x 'loadparm'
> feature. The 'loadparm' can be used to select the boot entry to
> boot from, for a boot device.

Why do we need / want any of this when we already have bootindex=NN
support. Inventing a S390-only way to select the boot device order
is pretty horrible IMHO


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

Re: [libvirt] [PATCH 1/3] virfile: Provide stub for virFileInData

[Daniel P. Berrange]

On Fri, May 19, 2017 at 01:31:32PM +0200, Michal Privoznik wrote:
> On 05/19/2017 12:28 PM, Peter Krempa wrote:
> > On Thu, May 18, 2017 at 15:46:44 +0200, Michal Privoznik wrote:
> >> Some older systems (such as RHEL6) lack SEEK_HOLE and SEEK_DATA
> >> which virFileInData relies on. Provide a stub for these systems.
> >>
> >> Signed-off-by: Michal Privoznik 
> >> ---
> >>  configure.ac   |  5 +
> >>  src/util/virfile.c | 15 +++
> >>  2 files changed, 20 insertions(+)
> > 
> > [...]
> > 
> >> @@ -3904,6 +3905,20 @@ virFileInData(int fd,
> >>  return ret;
> >>  }
> >>  
> >> +#else /* !HAVE_DECL_SEEK_HOLE */
> >> +
> >> +int
> >> +virFileInData(int fd ATTRIBUTE_UNUSED,
> >> +  int *inData ATTRIBUTE_UNUSED,
> >> +  long long *length ATTRIBUTE_UNUSED)
> >> +{
> >> +virReportSystemError(ENOSYS, "%s",
> >> + _("sparse files not supported"));
> >> +return -1;
> > 
> > Wouldn't it be better as a fallback always return that data is present
> > rather than failing?
> > 
> 
> I don't think so. What I can do actually is to not only report error,
> but also set errno = ENOSYS, so that caller can distinguish this case
> from other cases where virFileInData fails. I think we should let it up
> to the caller to decide then whether they want to ignore the error (and
> pretend there are no holes), or fail too. More generally, helpers in
> src/util/ should really be one purpose functions without trying to be
> clever and leave the logic for callers (drivers).

In particular by reporting an error we give options to the mgmt application.
They may well *not* want to do the upload/download if there is no sparse
support. By reporting an error they can detect that and make a decision
as to whether to retry without sparse flag enabled or not.


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

Re: [libvirt] [PATCH v2] qemu: Adding 'downscript' feature for QEMU network interfaces.

[Daniel P. Berrange]

On Tue, May 16, 2017 at 06:16:19PM -0400, Laine Stump wrote:
> On 05/05/2017 10:22 AM, Julio Faracco wrote:
> > This commit adds the support for 'downscript' feature:
> > - For QEMU command line with the option:
> >   '-net downscript=/etc/qemu-ifdown,...'.
> > 
> > - For Domains with a network interface description:
> >   '
> >  ...
> >  
> >  ...
> >   '
> > 
> > The options 'script' and 'downscript' accept the argument 'no' to disable
> > the script executions. The way that the code was implemented, the XML file
> > accepts '<[down]script path='no'>' to solve this problem.
> > 
> > This commit updates the tests and documentation too.
> > 
> > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=825939
> 
> I have a couple issues with this implementation:
> 
> 1) the script in an interface's 

Re: [libvirt] [PATCH v2] qemu: Report shutdown event details

[Daniel P. Berrange]

On Tue, May 16, 2017 at 12:10:23PM -0500, Eric Blake wrote:
> On 05/16/2017 08:49 AM, Martin Kletzander wrote:
> > QEMU will likely report the details of it shutting down, particularly
> > whether the shutdown was initiated by the guest or host.  We should
> > forward that information along, at least for shutdown events.  Reset
> > has that as well, however that is not a lifecycle event and would add
> > extra constants that might not be used.  It can be added later on.
> > 
> > Since the only way we can extend information provided to the user is
> > adding event details, we might as well emit multiple events (one with
> > the reason for the shutdown and keep the one for the shutdown being
> > finished for clarity and compatibility).
> > 
> > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1384007
> > 
> > Signed-off-by: Martin Kletzander 
> > ---
> > v2:
> >  - Adapt to new message format
> > 
> > Patch in QEMU:
> > https://lists.gnu.org/archive/html/qemu-devel/2017-05/msg03624.html
> > Applied to qapi-next: 
> > https://lists.gnu.org/archive/html/qemu-devel/2017-05/msg03742.html
> 
> Not in qemu master yet, but should land there prior to the next libvirt
> release.
> 
> 
> > +++ b/include/libvirt/libvirt-domain.h
> > @@ -2983,7 +2983,16 @@ typedef enum {
> >   * Details on the cause of a 'shutdown' lifecycle event
> >   */
> >  typedef enum {
> > -VIR_DOMAIN_EVENT_SHUTDOWN_FINISHED = 0, /* Guest finished shutdown 
> > sequence */
> > +/* Guest finished shutdown sequence */
> > +VIR_DOMAIN_EVENT_SHUTDOWN_FINISHED = 0,
> > +
> > +/* Guest is shutting down due to request from guest (e.g. 
> > hardware-specific
> > + * action) */
> > +VIR_DOMAIN_EVENT_SHUTDOWN_GUEST = 1,
> > +
> > +/* Guest is shutting down due to request from host (e.g. killed by a
> > + * signal) */
> > +VIR_DOMAIN_EVENT_SHUTDOWN_HOST = 2,
> > 
> 
> Looks reasonable.
> 
> > +++ b/src/qemu/qemu_monitor_json.c
> > @@ -523,9 +523,15 @@ qemuMonitorJSONKeywordStringToJSON(const char *str, 
> > const char *firstkeyword)
> >  }
> > 
> > 
> > -static void qemuMonitorJSONHandleShutdown(qemuMonitorPtr mon, 
> > virJSONValuePtr data ATTRIBUTE_UNUSED)
> > +static void qemuMonitorJSONHandleShutdown(qemuMonitorPtr mon, 
> > virJSONValuePtr data)
> >  {
> > -qemuMonitorEmitShutdown(mon);
> > +bool guest = false;
> > +virTristateBool guest_initiated = VIR_TRISTATE_BOOL_ABSENT;
> > +
> > +if (virJSONValueObjectGetBoolean(data, "guest", &guest) == 0)
> > +guest_initiated = guest ? VIR_TRISTATE_BOOL_YES : 
> > VIR_TRISTATE_BOOL_NO;
> > +
> > +qemuMonitorEmitShutdown(mon, guest_initiated);
> 
> Yes, that uses the new qemu interface correctly.
> 
> > @@ -678,6 +699,7 @@ qemuProcessHandleShutdown(qemuMonitorPtr mon 
> > ATTRIBUTE_UNUSED,
> > 
> >   unlock:
> >  virObjectUnlock(vm);
> > +qemuDomainEventQueue(driver, pre_event);
> >  qemuDomainEventQueue(driver, event);
> >  virObjectUnref(cfg);
> 
> Nice - you send the same event as always so old clients don't break, but
> new clients can now look for the new cause.

I don't think that's right actually. IMHO, we should onyl be sending the
new event, not the original event. These are intended to indicate state
changes, and having two VIR_DOMAIN_EVENT_SHUTDOWN events sent with
different details is not really representing a state change.

WRT to compatibility, clients should always expect that 'detail' may
change or new 'detail' enum values may be added - indeed we've done
that many many times int he past. So I don't think we need to duplicate
the existing event


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

[libvirt] [PATCH v2] Provide a useful README file

[Daniel P. Berrange]

The current README file contents has almost no useful info, and that
which does exist is very outdated.

Signed-off-by: Daniel P. Berrange 
---

In v2:

 - Use markdown syntax
 - Use README.md file
 - Symlink README to README.md
 - Include travis build status

 README| 14 +--
 README.md | 79 +++
 2 files changed, 80 insertions(+), 13 deletions(-)
 mode change 100644 => 12 README
 create mode 100644 README.md

diff --git a/README b/README
deleted file mode 100644
index 3d5167d..000
--- a/README
+++ /dev/null
@@ -1,13 +0,0 @@
-
- LibVirt : simple API for virtualization
-
-  Libvirt is a C toolkit to interact with the virtualization capabilities
-of recent versions of Linux (and other OSes). It is free software
-available under the GNU Lesser General Public License. Virtualization of
-the Linux Operating System means the ability to run multiple instances of
-Operating Systems concurrently on a single hardware system where the basic
-resources are driven by a Linux instance. The library aim at providing
-long term stable C API initially for the Xen paravirtualization but
-should be able to integrate other virtualization mechanisms if needed.
-
-Daniel Veillard 
diff --git a/README b/README
new file mode 12
index 000..42061c0
--- /dev/null
+++ b/README
@@ -0,0 +1 @@
+README.md
\ No newline at end of file
diff --git a/README.md b/README.md
new file mode 100644
index 000..c2bd2f8
--- /dev/null
+++ b/README.md
@@ -0,0 +1,79 @@
+[![Build 
Status](https://travis-ci.org/libvirt/libvirt.svg)](https://travis-ci.org/libvirt/libvirt)
+
+Libvirt API for virtualization
+==
+
+Libvirt provides a portable, long term stable C API for managing the
+virtualization technologies provided by many operating systems. It
+includes support for QEMU, KVM, Xen, LXC, BHyve, Virtuozzo, VMWare
+vCenter and ESX, VMWare Desktop, Hyper-V, VirtualBox and PowerHyp.
+
+For some of these hypervisors, it provides a stateful management
+daemon runs on the virtualization host allowing access to the API
+both by non-privileged local users and remote users.
+
+Layered packages provide bindings of the Libvirt C API into other
+languages including Python, Perl, Php, Go, Java, OCaml, as well as
+mappings into object systems such as GObject, CIM and SNMP.
+
+Further information about the libvirt project can be found on the
+website:
+
+*  <https://libvirt.org>
+
+License
+---
+
+The libvirt C API is distributed under the terms of GNU Lesser General
+Public License, version 2.1 (or later). Some parts of the code that are
+not part of the C library, may have the more restricted GNU General
+Public License, version 2.1 (or later). See the files COPYING.LESSER
+and COPYING for full license terms & conditions.
+
+Installation
+
+
+Libvirt uses the GNU Autotools build system, so in general can be built
+and installed with the normal commands. For example, to build in a manner
+that is suitable for installing as root, use:
+
+```
+# ./configure --prefix=/usr --sysconfdir=/etc --localstatedir=/var
+# make
+# sudo make install
+```
+
+While to build & install as an unprivileged user
+
+```
+# ./configure --prefix=$HOME/usr
+#  make
+#  make install
+```
+
+
+The libvirt code relies on a large number of 3rd party libraries. These will
+be detected during execution of the configure script and a summary printed
+which lists any missing (optional) dependancies.
+
+Contributing
+
+
+The libvirt project welcomes contributors from all. For most components
+the best way to contributor is to send patches to the primary development
+mailing list, using the 'git send-email' command. Further guidance on this
+can be found in the HACKING file, or the project website
+
+* <https://libvirt.org/contribute.html>
+
+Contact
+---
+
+The libvirt project has two primary mailing lists:
+
+ * libvir-list@redhat.com (**for development**)
+ * libvirt-us...@redhat.com (**for users**)
+
+Further details on contacting the project are available on the website
+
+* <https://libvirt.org/contact.html>
-- 
2.9.3

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

Re: [libvirt] [PATCH] Provide a useful README file

[Daniel P. Berrange]

On Tue, May 16, 2017 at 01:54:30PM +0200, Martin Kletzander wrote:
> On Tue, May 16, 2017 at 11:50:27AM +0100, Daniel P. Berrange wrote:
> > The current README file contents has almost no useful info, and that
> > which does exist is very outdated.
> > 
> > Signed-off-by: Daniel P. Berrange 
> > ---
> > README | 81 
> > +-
> > 1 file changed, 71 insertions(+), 10 deletions(-)
> > 
> 
> This is great and all, but if we are making this better, why not take
> the opportunity to use some plaintext format that has few features?  I,
> personally, don't care whether that's org, markdown or rst.  Just
> something that is still usable and readable as a plaintext file while it
> can be nicely formatted (with headers, links and images) on pages that
> support it (e.g. github).  I know this seems like stupid wannabe modern
> tiny thing, but there are many nuances that can potentially influence
> future contributors.  And I think this is one of the positive ones.

Sure, I can do that.


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

[libvirt] [PATCH] Provide a useful README file

[Daniel P. Berrange]

The current README file contents has almost no useful info, and that
which does exist is very outdated.

Signed-off-by: Daniel P. Berrange 
---
 README | 81 +-
 1 file changed, 71 insertions(+), 10 deletions(-)

diff --git a/README b/README
index 3d5167d..7103ae9 100644
--- a/README
+++ b/README
@@ -1,13 +1,74 @@
+ Libvirt API for virtualization
+==
 
- LibVirt : simple API for virtualization
+Libvirt provides a portable, long term stable C API for managing the
+virtualization technologies provided by many operating systems. It
+includes support for QEMU, KVM, Xen, LXC, BHyve, Virtuozzo, VMWare
+vCenter and ESX, VMWare Desktop, Hyper-V, VirtualBox and PowerHyp.
 
-  Libvirt is a C toolkit to interact with the virtualization capabilities
-of recent versions of Linux (and other OSes). It is free software
-available under the GNU Lesser General Public License. Virtualization of
-the Linux Operating System means the ability to run multiple instances of
-Operating Systems concurrently on a single hardware system where the basic
-resources are driven by a Linux instance. The library aim at providing
-long term stable C API initially for the Xen paravirtualization but
-should be able to integrate other virtualization mechanisms if needed.
+For some of these hypervisors, it provides a stateful management
+daemon runs on the virtualization host allowing access to the API
+both by non-privileged local users and remote users.
 
-Daniel Veillard 
+Layered packages provide bindings of the Libvirt C API into other
+languages including Python, Perl, Php, Go, Java, OCaml, as well as
+mappings into object systems such as GObject, CIM and SNMP.
+
+Further information about the libvirt project can be found on the
+website:
+
+  https://libvirt.org
+
+License
+===
+
+The libvirt C API is distributed under the terms of GNU Lesser General
+Public License, version 2.1 (or later). Some parts of the code that are
+not part of the C library, may have the more restricted GNU General
+Public License, version 2.1 (or later). See the files COPYING.LESSER
+and COPYING for full license terms & conditions.
+
+Installation
+
+
+Libvirt uses the GNU Autotools build system, so in general can be built
+and installed with the normal commands. For example, to build in a manner
+that is suitable for installing as root, use:
+
+  ./configure --prefix=/usr --sysconfdir=/etc --localstatedir=/var
+  make
+  sudo make install
+
+While to build & install as an unprivileged user
+
+  ./configure --prefix=$HOME/usr
+  make
+  make install
+
+The libvirt code relies on a large number of 3rd party libraries. These will
+be detected during execution of the configure script and a summary printed
+which lists any missing (optional) dependancies.
+
+Contributing
+
+
+The libvirt project welcomes contributors from all. For most components
+the best way to contributor is to send patches to the primary development
+mailing list, using the 'git send-email' command. Further guidance on this
+can be found in the HACKING file, or the project website
+
+  https://libvirt.org/contribute.html
+
+Contact
+===
+
+The libvirt project has two primary mailing lists:
+
+  * libvir-list@redhat.com (for development)
+  * libvirt-us...@redhat.com (for users)
+
+Further details on contacting the project are available on the website
+
+  https://libvirt.org/contact.html
+
+-- End
-- 
2.9.3

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

Re: [libvirt] [PATCH] qemu: allow to control host side link status of network device

[Daniel P. Berrange]

On Tue, May 16, 2017 at 10:28:27AM +0300, Vasiliy Tolstov wrote:
> 2017-05-16 9:32 GMT+03:00 Peter Krempa :
> > 3.3.2? We dropped micro versions some time ago.
> >
> 
> This is copy/paste from link element, sorry.
> 
> >> +
> >> +
> >>  MTU configuration
> >>  
> >
> >
> > There already is an element called  with attribute "state" which
> > modifies the link state in qemu. So that maps to guest link state. So
> > this patch has a naming collision going on.
> 
> Yes, but some times ago Laine Stump says that for host side link
> status we can use source element with link state.
> 
> >
> > I also don't really see how modifying the host side link state would be
> > useful. Could you elaborate why you think it would be useful?
> 
> OSPF - tap device have addresses assigned to it and bird/quagga create
> routing based on interface link status and addresses on device.
> So if i have host side device in up state no matter that have guest -
> host system forward traffic to it. So i need to modify host side link
> status too.

Shouldn't we just tie the host & guest link state together then. This
doesn't seem like enough of a reason to introduce new XML elements.


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

Re: [libvirt] [libvirt-python PATCH v2] spec: Install egg-info with rpm package

[Daniel P. Berrange]

On Mon, May 15, 2017 at 05:58:47PM +0200, Martin Kletzander wrote:
> This was being done due to now deprecated policy and that file should
> be installed so that pip can recognize that the packages is already
> installed in the system.
> 
> Signed-off-by: Martin Kletzander 
> ---
> v2:
>  - Put each egg-info ito its respective RPM package
> 
>  libvirt-python.spec.in | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

ACK


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

[libvirt] [PATCH] remove hack for debian etch limits.h

[Daniel P. Berrange]

The debian etch distro was end-of-life a long time ago so we no
longer need the ULLONG_MAX hack. In any case gnulib now provides
an equivalent fix by default, and so our definition now triggers
syntax-check rule failure

src/internal.h:#define ULLONG_MAX   ULONG_LONG_MAX
maint.mk: define the above via some gnulib .h file
maint.mk:843: recipe for target 'sc_prohibit_always-defined_macros' failed

Signed-off-by: Daniel P. Berrange 
---

Pushed as a build break fix

 src/internal.h | 5 -
 1 file changed, 5 deletions(-)

diff --git a/src/internal.h b/src/internal.h
index 713734c..5a5a430 100644
--- a/src/internal.h
+++ b/src/internal.h
@@ -114,11 +114,6 @@
 #define __GNUC_PREREQ(maj, min) 0
 #   endif
 
-/* Work around broken limits.h on debian etch */
-#   if defined _GCC_LIMITS_H_ && ! defined ULLONG_MAX
-#define ULLONG_MAX   ULONG_LONG_MAX
-#   endif
-
 #  endif /* __GNUC__ */
 
 /**
-- 
2.9.3

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

Re: [libvirt] [libvirt-python PATCH] spec: Install egg-info with rpm package

[Daniel P. Berrange]

On Mon, May 15, 2017 at 05:26:39PM +0200, Martin Kletzander wrote:
> This was being done due to now deprecated policy and that file should
> be installed so that pip can recognize that the packages is already
> installed in the system.
> 
> Signed-off-by: Martin Kletzander 
> ---
>  libvirt-python.spec.in | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/libvirt-python.spec.in b/libvirt-python.spec.in
> index 5ad029281c52..132183c93c02 100644
> --- a/libvirt-python.spec.in
> +++ b/libvirt-python.spec.in
> @@ -65,7 +65,6 @@ CFLAGS="$RPM_OPT_FLAGS" %{__python3} setup.py build
>  %if %{with_python3}
>  %{__python3} setup.py install --skip-build --root=%{buildroot}
>  %endif
> -rm -f %{buildroot}%{_libdir}/python*/site-packages/*egg-info
> 
>  %check
>  %{__python} setup.py test
> @@ -81,6 +80,8 @@ rm -f %{buildroot}%{_libdir}/python*/site-packages/*egg-info
>  %{_libdir}/python2*/site-packages/libvirt_lxc.py*
>  %{_libdir}/python2*/site-packages/libvirtmod*
> 
> +%{_libdir}/python*/site-packages/*egg-info

That wildcard looks like it'll put the python3 egg info in the python2 RPM

> +
>  %if %{with_python3}
>  %files -n libvirt-python3
>  %defattr(-,root,root)

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

Re: [libvirt] libvirt-python RPM installation not recognized by pip

[Daniel P. Berrange]

On Mon, May 15, 2017 at 02:52:07PM +0200, Martin Kletzander wrote:
> On Mon, May 15, 2017 at 07:35:30AM -0400, Cleber Rosa wrote:
> > Hello,
> > 
> > When using the standard "requirements.txt" files for installation
> > package dependencies, I noticed that "libvirt-python" would attempt to
> > be installed by "pip" even when the equivalent RPM package is already
> > installed.
> > 
> > For instance, on a Fedora 25 system:
> > 
> >  $ rpm -q libvirt-python
> >  libvirt-python-2.2.0-1.fc25.x86_64
> >  $ python -e 'import pkg_resources;
> > pkg_resources.get_distribution("libvirt-python")'
> >  ...
> > pkg_resources.DistributionNotFound: The 'libvirt-python' distribution
> > was not found and is required by the application
> > 
> > The provider (the actual module) is actually present, but (rightfully
> > so) under the name "libvirt":
> > 
> >  $ python -c 'import pkg_resources; print
> > pkg_resources.get_provider("libvirt")'
> > 
> > 
> > At first sight, this seems to be caused by the lack of an "egg-info"
> > file, such as:
> > 
> > $PYTHON_SITE_PACKAGES/libvirt_python-2.2.0-py2.7.egg-info
> > 
> > I'm reporting a supposedly packaging issue here since the libvirt-python
> > setup.py file itself includes support for a custom rpm command.
> > 
> > Please advise if any of this is intentional, and/or whether this should
> > indeed be reported here or on downstream only.
> > 
> 
> On non-rpm system, I have that file and it works properly:
> 
>  $ ls /usr/lib64/python*/site-packages/libvirt_python-3.2.0-py*.egg-info
>  /usr/lib64/python2.7/site-packages/libvirt_python-3.2.0-py2.7.egg-info
>  /usr/lib64/python3.4/site-packages/libvirt_python-3.2.0-py3.4.egg-info
> 
>  $ pip install --user libvirt-python
>  Requirement already satisfied: libvirt-python in 
> /usr/lib64/python3.4/site-packages
> 
> Looks like this _might_ be related to the %install script in 
> libvirt-python.spec.in:
> 
>  rm -f %{buildroot}%{_libdir}/python*/site-packages/*egg-info
> 
> So it should be included instead, I guess.  Cc'ing the original author
> for confirmation that it was not intentional.

Fedora policy for a long time was that python packages *should* delete
the *egg-info metadata files, but this has apparently changed so that it
should be kept installed.

https://lists.fedoraproject.org/archives/list/de...@lists.fedoraproject.org/thread/FOG5APDRQVNXR5ZOZZSVDNZVN4WURKG4/

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

Re: [libvirt] [PATCH v2] storage: use 0711 as the default perms for dirs

[Daniel P. Berrange]

On Mon, May 15, 2017 at 01:05:31PM +0200, Christian Ehrhardt wrote:
> From: Serge Hallyn 
> 
> There should be no need to make dir based pools world/group readable.
> So use 0711, not 0755, as the default perms for storage dirs.
> 
> Updates in v2:
>  - adapt commit wording to mention dropping group readable as well
> 
> Signed-off-by: Christian Ehrhardt 
> ---
>  docs/formatstorage.html.in | 2 +-
>  src/storage/storage_util.h | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Daniel P. Berrange 

Will push to git shortly.

BTW, for libvir-list we recommend to send v2/v3/etc followup patches as
top level threads, not in-reply-to the previous versions.


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

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

[Daniel P. Berrange]

This pulls in the fixes for poll() on Win32 which finally
makes the remote driver work again.

Signed-off-by: Daniel P. Berrange 
---
 .gnulib | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/.gnulib b/.gnulib
index 94386a1..da830b5 16
--- a/.gnulib
+++ b/.gnulib
@@ -1 +1 @@
-Subproject commit 94386a13667c645fd42544a7fd302c39fcdf
+Subproject commit da830b5146cb553ac2a4bcfe76caeb57bda24cc3
-- 
2.9.3

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

Re: [libvirt] [PATCH v2 11/38] Introduce virStreamSkip

[Daniel P. Berrange]

On Mon, May 15, 2017 at 10:54:03AM +0200, Michal Privoznik wrote:
> On 05/15/2017 10:25 AM, Daniel P. Berrange wrote:
> > On Fri, May 12, 2017 at 09:29:27AM +0200, Michal Privoznik wrote:
> >> On 05/05/2017 04:48 PM, Daniel P. Berrange wrote:
> >>> On Fri, May 05, 2017 at 01:25:34PM +0200, Michal Privoznik wrote:
> >>>> On 05/04/2017 11:29 PM, John Ferlan wrote:
> >>>>>
> >>>>>
> >>>>> On 04/20/2017 06:01 AM, Michal Privoznik wrote:
> >>>>>> This API can be used to tell the other side of the stream to skip
> >>>>>
> >>>>> s/can be/is  (unless it can be used for something else ;-))
> >>>>>
> >>>>>> some bytes in the stream. This can be used to create a sparse
> >>>>>> file on the receiving side of a stream.
> >>>>>>
> >>>>>> It takes just one argument @length, which says how big the hole
> >>>>>> is. Since our streams are not rewindable like regular files, we
> >>>>>> don't need @whence argument like seek(2) has.
> >>>>>
> >>>>> lseek is an implementation detail...  However, it could be stated that
> >>>>> the skipping would be from the current point in the file forward by some
> >>>>> number of bytes.  It's expected to be used in conjunction with code that
> >>>>> is copying over the real (or non-zero) data and should be considered an
> >>>>> optimization over sending zere data segments.
> >>>>>
> >>>>>>
> >>>>>> Signed-off-by: Michal Privoznik 
> >>>>>> ---
> >>>>>>  include/libvirt/libvirt-stream.h |  3 +++
> >>>>>>  src/driver-stream.h  |  5 
> >>>>>>  src/libvirt-stream.c | 57 
> >>>>>> 
> >>>>>>  src/libvirt_public.syms  |  1 +
> >>>>>>  4 files changed, 66 insertions(+)
> >>>>>>
> >>>>>
> >>>>> While it would be unused for now, should @flags be added.  Who knows
> >>>>> what use it could have, but avoids a new Flags API, but does cause a few
> >>>>> other wording changes here.
> >>>>
> >>>> Ah sure. We should have @flags there. Good point.
> >>>>
> >>>>>
> >>>>> Perhaps it's just me - but "Skip" and "HoleSize" just seem awkward.
> >>>>> Would "virStreamSetSkip" and "virStreamGetSkip" be more apropos? (or
> >>>>> s/Skip/HoleSize/ - ewww). Names would then follow our more recent
> >>>>> function naming guidelines. I think I dislike the HoleSize much more
> >>>>> than the Skip.
> >>>>
> >>>> SetSkip and GetSkip sound wrong to me instead :D
> >>>>
> >>>>>
> >>>>>> diff --git a/include/libvirt/libvirt-stream.h 
> >>>>>> b/include/libvirt/libvirt-stream.h
> >>>>>> index bee2516..4e0a599 100644
> >>>>>> --- a/include/libvirt/libvirt-stream.h
> >>>>>> +++ b/include/libvirt/libvirt-stream.h
> >>>>>> @@ -50,6 +50,9 @@ int virStreamRecvFlags(virStreamPtr st,
> >>>>>> size_t nbytes,
> >>>>>> unsigned int flags);
> >>>>>>  
> >>>>>> +int virStreamSkip(virStreamPtr st,
> >>>>>> +  unsigned long long length);
> >>>>>
> >>>>> Was there consideration for using 'off_t' instead of ULL? I know it's an
> >>>>> implementation detail of virFDStreamData and lseek() usage, but it does
> >>>>> hide things... IDC either way.
> >>>>
> >>>> The problem with off_t is that it is signed type, while ULL is unsigned.
> >>>> There's not much point in sending a negative offset, is there?
> >>>> Moreover, we use ULL for arguments like offset (not sure really why).
> >>>> Frankly, I don't really know why. Perhaps some types don't exist 
> >>>> everywhere?
> >>>
> >>> If anything, we would use  size_t, for consistency with the Send/Recv
> >>> methods. 
> >>
> >> So I&#

Re: [libvirt] [PATCH] storage: use 0711 as the default perms for dirs

[Daniel P. Berrange]

On Thu, May 11, 2017 at 06:36:22PM -0400, John Ferlan wrote:
> 
> 
> On 05/11/2017 04:31 AM, Christian Ehrhardt wrote:
> > From: Serge Hallyn 
> > 
> > There should be no need to make dir based pools world readable.
> > So use 0711, not 0755, as the default perms for storage dirs.
> > 
> > Signed-off-by: Christian Ehrhardt 
> > ---
> >  docs/formatstorage.html.in | 2 +-
> >  src/storage/storage_util.h | 2 +-
> >  2 files changed, 2 insertions(+), 2 deletions(-)
> > 
> 
> Kinda surprised this didn't generate some immediate discussion...  I
> would also think that if you had a desire to change defaults you'd also
> have a libvirt.spec.in adjustment...

Actually no it doesn't - the spec file is already marking
/var/lib/libvirt/images as 0711.

> Still 0755 or umask(022) seem to be fairly prevalent setting and having
> the  for the XML to be able to override a default certainly gives
> credence to arguments in either direction whether or not to change the
> defaults.
> 
> It's been a long while since I considered system/directory/file security
> things, but I have this faint recollection of some strange issue when
> not having world or group "executable" as a default.

The fact that RPM spec ships with 0711 show that it works ok. So I
think this change is reasonable.


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

Re: [libvirt] [PATCH v2 11/38] Introduce virStreamSkip

[Daniel P. Berrange]

On Fri, May 12, 2017 at 09:29:27AM +0200, Michal Privoznik wrote:
> On 05/05/2017 04:48 PM, Daniel P. Berrange wrote:
> > On Fri, May 05, 2017 at 01:25:34PM +0200, Michal Privoznik wrote:
> >> On 05/04/2017 11:29 PM, John Ferlan wrote:
> >>>
> >>>
> >>> On 04/20/2017 06:01 AM, Michal Privoznik wrote:
> >>>> This API can be used to tell the other side of the stream to skip
> >>>
> >>> s/can be/is  (unless it can be used for something else ;-))
> >>>
> >>>> some bytes in the stream. This can be used to create a sparse
> >>>> file on the receiving side of a stream.
> >>>>
> >>>> It takes just one argument @length, which says how big the hole
> >>>> is. Since our streams are not rewindable like regular files, we
> >>>> don't need @whence argument like seek(2) has.
> >>>
> >>> lseek is an implementation detail...  However, it could be stated that
> >>> the skipping would be from the current point in the file forward by some
> >>> number of bytes.  It's expected to be used in conjunction with code that
> >>> is copying over the real (or non-zero) data and should be considered an
> >>> optimization over sending zere data segments.
> >>>
> >>>>
> >>>> Signed-off-by: Michal Privoznik 
> >>>> ---
> >>>>  include/libvirt/libvirt-stream.h |  3 +++
> >>>>  src/driver-stream.h  |  5 
> >>>>  src/libvirt-stream.c | 57 
> >>>> 
> >>>>  src/libvirt_public.syms  |  1 +
> >>>>  4 files changed, 66 insertions(+)
> >>>>
> >>>
> >>> While it would be unused for now, should @flags be added.  Who knows
> >>> what use it could have, but avoids a new Flags API, but does cause a few
> >>> other wording changes here.
> >>
> >> Ah sure. We should have @flags there. Good point.
> >>
> >>>
> >>> Perhaps it's just me - but "Skip" and "HoleSize" just seem awkward.
> >>> Would "virStreamSetSkip" and "virStreamGetSkip" be more apropos? (or
> >>> s/Skip/HoleSize/ - ewww). Names would then follow our more recent
> >>> function naming guidelines. I think I dislike the HoleSize much more
> >>> than the Skip.
> >>
> >> SetSkip and GetSkip sound wrong to me instead :D
> >>
> >>>
> >>>> diff --git a/include/libvirt/libvirt-stream.h 
> >>>> b/include/libvirt/libvirt-stream.h
> >>>> index bee2516..4e0a599 100644
> >>>> --- a/include/libvirt/libvirt-stream.h
> >>>> +++ b/include/libvirt/libvirt-stream.h
> >>>> @@ -50,6 +50,9 @@ int virStreamRecvFlags(virStreamPtr st,
> >>>> size_t nbytes,
> >>>> unsigned int flags);
> >>>>  
> >>>> +int virStreamSkip(virStreamPtr st,
> >>>> +  unsigned long long length);
> >>>
> >>> Was there consideration for using 'off_t' instead of ULL? I know it's an
> >>> implementation detail of virFDStreamData and lseek() usage, but it does
> >>> hide things... IDC either way.
> >>
> >> The problem with off_t is that it is signed type, while ULL is unsigned.
> >> There's not much point in sending a negative offset, is there?
> >> Moreover, we use ULL for arguments like offset (not sure really why).
> >> Frankly, I don't really know why. Perhaps some types don't exist 
> >> everywhere?
> > 
> > If anything, we would use  size_t, for consistency with the Send/Recv
> > methods. 
> 
> So I've given this some though and ran some experiments. On a 32bit arch
> I've found this:
> 
> long long 8 signed
> size_t 4 unsigned
> ssize_t 4 signed
> off_t 4 signed
> 
> So size_t is 4 bytes long and long long is 8 bytes. This got me
> thinking, size_t type makes sense for those APIs where we need to
> address individual bytes. But what would happen if I have the following
> file on a 32 bit arch:
> 
> [2MB data] -> [5GB hole] -> [2M data]
> 
> The hole does not fit into size_t, but it would fit into long long. On
> the other hand, we are very likely to hit lseek() error as off_t is 4
> bytes also (WTF?!). On a 64 bit arch everything is as expected:

Were you testing libvirt, or testing a standalone demo program ?

Libvirt should be defining the macro that enables large file support,
which turns off_t into a 64-bit type.  So in fact, we would actually
be wrong to use size_t - off_t or long long is actually what we need
here.

I was wrong about consistency with Send/Recv, since the arg there is
about the length of the data array which is size_t and this is a
different boundary than max file size which is off_t.


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

Re: [libvirt] [PATCH 1/2] spec: Support maintenance releases on mingw

[Daniel P. Berrange]

On Thu, May 11, 2017 at 06:41:07PM +0200, Andrea Bolognani wrote:
> The regular spec file contains code to deal with the fact
> that maintenance releases are uploaded to their own
> directory: copy it over to the mingw spec file so that it's
> possible to build maintenance releases there as well.
> 
> This also switches the source URL from FTP to HTTP for
> consistency with the main spec file.
> 
> Signed-off-by: Andrea Bolognani 
> ---
>  mingw-libvirt.spec.in | 6 +-
>  1 file changed, 5 insertions(+), 1 deletion(-)

Reviewed-by: Daniel P. Berrange 

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

Re: [libvirt] [PATCH 1/1] Enable Travis CI build status icon

[Daniel P. Berrange]

On Thu, May 11, 2017 at 04:24:41PM +0200, Martin Kletzander wrote:
> On Thu, May 11, 2017 at 02:35:12PM +0100, Daniel P. Berrange wrote:
> > On Thu, May 11, 2017 at 03:30:00PM +0200, Martin Kletzander wrote:
> > > On Tue, Apr 18, 2017 at 02:39:28PM -0300, Claudio André wrote:
> > > > Using GitHub libvirt site, it is possible to show Travis's fancy icon 
> > > > of the current build status. It highlights the QA process.
> > > 
> > > I like seeing the icon there.  It's very quick reference that serves the
> > > purpose.
> > > 
> > > There's no need for a cover letter when sending one patch.
> > > 
> > > > ---
> > > > README.md | 12 
> > > > 1 file changed, 12 insertions(+)
> > > > create mode 100644 README.md
> > > >
> > > > diff --git a/README.md b/README.md
> > > > new file mode 100644
> > > > index 000..a609286
> > > > --- /dev/null
> > > > +++ b/README.md
> > > > @@ -0,0 +1,12 @@
> > > > +## *LibVirt: the virtualization API* [![Build 
> > > > Status](https://travis-ci.org/libvirt/libvirt.svg)](https://travis-ci.org/libvirt/libvirt)
> > > > +
> > > 
> > > I don't care about the number of (sub-)s in 'sub-heading', but why
> > > making it italic as well?  ACK without the italic.  Will push this in a
> > > while.  Thanks.
> > > 
> > > > +  Libvirt is a C toolkit to interact with the virtualization 
> > > > capabilities
> > > > +of recent versions of Linux (and other OSes). It is free software
> > > > +available under the GNU Lesser General Public License. Virtualization 
> > > > of
> > > > +the Linux Operating System means the ability to run multiple instances 
> > > > of
> > > > +Operating Systems concurrently on a single hardware system where the 
> > > > basic
> > > > +resources are driven by a Linux instance. The library aim at providing
> > > > +long term stable C API initially for the Xen paravirtualization but
> > > > +should be able to integrate other virtualization mechanisms if needed.
> > > > +
> > > > +Daniel Veillard 
> > 
> > This just duplicates the existing README file content. If we're going todo
> > this we should just make README a symlink to README.md or vica-verca.
> > 
> 
> What space would we save by that?  I don't think people would be
> confused by the icon text in the readme, so I don't really care.  So I
> can add one more patch after this or squash it in:

I wasn't meaning for sake of saving space, but rather ensuring the content
is always in sync. For that matter our README file is awful and we should
put some useful content in it:-)



Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

Re: [libvirt] [PATCH] tests: stub out virfilewrapper.c on Win32

[Daniel P. Berrange]

On Thu, May 11, 2017 at 03:40:37PM +0200, Martin Kletzander wrote:
> On Thu, May 11, 2017 at 02:16:11PM +0100, Daniel P. Berrange wrote:
> > On Thu, May 11, 2017 at 03:13:03PM +0200, Martin Kletzander wrote:
> > > On Thu, May 11, 2017 at 11:46:01AM +0100, Daniel P. Berrange wrote:
> > > > The Win32 platform can not do link time overrides in the same way
> > > > that we can on POSIX / ELF based platforms, so we cannot build
> > > > the virfilewrapper.c code reliably. Just stub it out on Win32
> > > > so it is a no-op. Tests that use this file are already written
> > > > to skip on Win32.
> > > >
> > > 
> > > I understood we wanted to see that some tests were skipped on a
> > > particular platform.  But we already have so many tests build
> > > conditionally, why don't we just exclude all of them in a Makefile?
> > > That way we wouldn't need to make whole file inside a condition.  I can
> > > walk through them and clean it up, I just want to know if it makes
> > > sense.
> > 
> > The benefit of compiling the tests, but having them exit status 77
> > is that it means automake test harness then clearly reports which
> > tests are being skipped on each platform.  If we do conditionals
> > in the makefile, them skipped tests are invisible - they just
> > disappear entirely from output. So I prefer that we use the exit
> > status 77
> > 
> 
> Yes.  As I said, I understand that.  But we already have bunch of
> test_programs appended only conditionally.  And I felt like mixing the
> approach is not really intuitive for readers.

My vote would be to get rid of the makefile conditionals because they
often lead to bugs in 'make dist' missing out files.


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

Re: [libvirt] [PATCH 1/1] Enable Travis CI build status icon

[Daniel P. Berrange]

On Thu, May 11, 2017 at 03:30:00PM +0200, Martin Kletzander wrote:
> On Tue, Apr 18, 2017 at 02:39:28PM -0300, Claudio André wrote:
> > Using GitHub libvirt site, it is possible to show Travis's fancy icon of 
> > the current build status. It highlights the QA process.
> 
> I like seeing the icon there.  It's very quick reference that serves the
> purpose.
> 
> There's no need for a cover letter when sending one patch.
> 
> > ---
> > README.md | 12 
> > 1 file changed, 12 insertions(+)
> > create mode 100644 README.md
> > 
> > diff --git a/README.md b/README.md
> > new file mode 100644
> > index 000..a609286
> > --- /dev/null
> > +++ b/README.md
> > @@ -0,0 +1,12 @@
> > +## *LibVirt: the virtualization API* [![Build 
> > Status](https://travis-ci.org/libvirt/libvirt.svg)](https://travis-ci.org/libvirt/libvirt)
> > +
> 
> I don't care about the number of (sub-)s in 'sub-heading', but why
> making it italic as well?  ACK without the italic.  Will push this in a
> while.  Thanks.
> 
> > +  Libvirt is a C toolkit to interact with the virtualization capabilities
> > +of recent versions of Linux (and other OSes). It is free software
> > +available under the GNU Lesser General Public License. Virtualization of
> > +the Linux Operating System means the ability to run multiple instances of
> > +Operating Systems concurrently on a single hardware system where the basic
> > +resources are driven by a Linux instance. The library aim at providing
> > +long term stable C API initially for the Xen paravirtualization but
> > +should be able to integrate other virtualization mechanisms if needed.
> > +
> > +Daniel Veillard 

This just duplicates the existing README file content. If we're going todo
this we should just make README a symlink to README.md or vica-verca.

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

Re: [libvirt] [PATCH] tests: stub out virfilewrapper.c on Win32

[Daniel P. Berrange]

On Thu, May 11, 2017 at 03:13:03PM +0200, Martin Kletzander wrote:
> On Thu, May 11, 2017 at 11:46:01AM +0100, Daniel P. Berrange wrote:
> > The Win32 platform can not do link time overrides in the same way
> > that we can on POSIX / ELF based platforms, so we cannot build
> > the virfilewrapper.c code reliably. Just stub it out on Win32
> > so it is a no-op. Tests that use this file are already written
> > to skip on Win32.
> > 
> 
> I understood we wanted to see that some tests were skipped on a
> particular platform.  But we already have so many tests build
> conditionally, why don't we just exclude all of them in a Makefile?
> That way we wouldn't need to make whole file inside a condition.  I can
> walk through them and clean it up, I just want to know if it makes
> sense.

The benefit of compiling the tests, but having them exit status 77
is that it means automake test harness then clearly reports which
tests are being skipped on each platform.  If we do conditionals
in the makefile, them skipped tests are invisible - they just
disappear entirely from output. So I prefer that we use the exit
status 77

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

[libvirt] [PATCH] tests: stub out virfilewrapper.c on Win32

[Daniel P. Berrange]

The Win32 platform can not do link time overrides in the same way
that we can on POSIX / ELF based platforms, so we cannot build
the virfilewrapper.c code reliably. Just stub it out on Win32
so it is a no-op. Tests that use this file are already written
to skip on Win32.

Signed-off-by: Daniel P. Berrange 
---

Pushed as a mingw build fix

 tests/virfilewrapper.c | 29 -
 1 file changed, 16 insertions(+), 13 deletions(-)

diff --git a/tests/virfilewrapper.c b/tests/virfilewrapper.c
index 5ea70b3..bf2fa69 100644
--- a/tests/virfilewrapper.c
+++ b/tests/virfilewrapper.c
@@ -18,15 +18,17 @@
 
 #include 
 
-#include 
-#include 
-#include 
+#ifndef WIN32
 
-#include "viralloc.h"
-#include "virfile.h"
-#include "virfilewrapper.h"
-#include "virmock.h"
-#include "virstring.h"
+# include 
+# include 
+# include 
+
+# include "viralloc.h"
+# include "virfile.h"
+# include "virfilewrapper.h"
+# include "virmock.h"
+# include "virstring.h"
 
 
 /* Mapping for prefix overrides */
@@ -139,7 +141,7 @@ virFileWrapperOverridePrefix(const char *path)
 }
 
 
-#define PATH_OVERRIDE(newpath, path)\
+# define PATH_OVERRIDE(newpath, path)   \
 do {\
 init_syms();\
 \
@@ -177,7 +179,7 @@ int access(const char *path, int mode)
 return ret;
 }
 
-#ifdef HAVE___LXSTAT
+# ifdef HAVE___LXSTAT
 int __lxstat(int ver, const char *path, struct stat *sb)
 {
 int ret = -1;
@@ -191,7 +193,7 @@ int __lxstat(int ver, const char *path, struct stat *sb)
 
 return ret;
 }
-#endif /* HAVE___LXSTAT */
+# endif /* HAVE___LXSTAT */
 
 int lstat(const char *path, struct stat *sb)
 {
@@ -207,7 +209,7 @@ int lstat(const char *path, struct stat *sb)
 return ret;
 }
 
-#ifdef HAVE___XSTAT
+# ifdef HAVE___XSTAT
 int __xstat(int ver, const char *path, struct stat *sb)
 {
 int ret = -1;
@@ -221,7 +223,7 @@ int __xstat(int ver, const char *path, struct stat *sb)
 
 return ret;
 }
-#endif /* HAVE___XSTAT */
+# endif /* HAVE___XSTAT */
 
 int stat(const char *path, struct stat *sb)
 {
@@ -278,3 +280,4 @@ DIR *opendir(const char *path)
 
 return ret;
 }
+#endif
-- 
2.9.3

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

[libvirt] [PATCH] remote: increase max storage pools, nwfilters & snapshots to 16384

[Daniel P. Berrange]

Most other top level objects have already had their limits increased
to 16384. Increase the storage pool, nwfilter & snapshot object
limits to match. For snapshots at least, we have seen hosts which
exceeded the current limit

Signed-off-by: Daniel P. Berrange 
---
 src/remote/remote_protocol.x | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x
index 87b2bd3..4e3fa60 100644
--- a/src/remote/remote_protocol.x
+++ b/src/remote/remote_protocol.x
@@ -80,7 +80,7 @@ const REMOTE_NETWORK_LIST_MAX = 16384;
 const REMOTE_INTERFACE_LIST_MAX = 16384;
 
 /* Upper limit on lists of storage pools. */
-const REMOTE_STORAGE_POOL_LIST_MAX = 4096;
+const REMOTE_STORAGE_POOL_LIST_MAX = 16384;
 
 /* Upper limit on lists of storage vols. */
 const REMOTE_STORAGE_VOL_LIST_MAX = 16384;
@@ -92,7 +92,7 @@ const REMOTE_NODE_DEVICE_LIST_MAX = 65536;
 const REMOTE_NODE_DEVICE_CAPS_LIST_MAX = 65536;
 
 /* Upper limit on lists of network filters. */
-const REMOTE_NWFILTER_LIST_MAX = 1024;
+const REMOTE_NWFILTER_LIST_MAX = 16384;
 
 /* Upper limit on list of scheduler parameters. */
 const REMOTE_DOMAIN_SCHEDULER_PARAMETERS_MAX = 16;
@@ -137,7 +137,7 @@ const REMOTE_AUTH_TYPE_LIST_MAX = 20;
 const REMOTE_DOMAIN_MEMORY_STATS_MAX = 1024;
 
 /* Upper limit on lists of domain snapshots. */
-const REMOTE_DOMAIN_SNAPSHOT_LIST_MAX = 1024;
+const REMOTE_DOMAIN_SNAPSHOT_LIST_MAX = 16384;
 
 /* Maximum length of a block peek buffer message.
  * Note applications need to be aware of this limit and issue multiple
-- 
2.9.3

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

[libvirt] [PATCH] rpc: improve error message for bounds check

[Daniel P. Berrange]

If we exceed a fixed limit in RPC code we get a horrible message
like this, if the parameter type is a 'string', because we forgot
to initialize the error message type field:

  $ virsh snapshot-list ostack1
  error: too many remote undefineds: 1329 > 1024

It would also be useful to know which RPC call and field was
exceeded. So this patch makes us report:

  $ virsh snapshot-list ostack1
  error: too many remote undefineds: 1329 > 1024,
  in parameter 'names' for 'virDomainSnapshotListNames'

Signed-off-by: Daniel P. Berrange 
---
 src/rpc/gendispatch.pl | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/src/rpc/gendispatch.pl b/src/rpc/gendispatch.pl
index 173189c..0c5e4ba 100755
--- a/src/rpc/gendispatch.pl
+++ b/src/rpc/gendispatch.pl
@@ -1501,6 +1501,7 @@ elsif ($mode eq "client") {
 $single_ret_list_name = $1;
 $single_ret_list_max_var = "max$1";
 $single_ret_list_max_define = $2;
+$single_ret_list_error_msg_type = "string";
 } elsif ($ret_member =~ m/^(admin|remote)_nonnull_string 
(\S+)<\S+>;/) {
 # error out on unannotated arrays
 die "$1_nonnull_string array without insert@ 
annotation: $ret_member";
@@ -1773,7 +1774,8 @@ elsif ($mode eq "client") {
 print "\n";
 print "if ($single_ret_list_max_var > 
$single_ret_list_max_define) {\n";
 print "virReportError(VIR_ERR_RPC,\n";
-print "   _(\"too many remote 
${single_ret_list_error_msg_type}s: %d > %d\"),\n";
+print "   _(\"too many remote 
${single_ret_list_error_msg_type}s: %d > %d,\"\n";
+print " \"in parameter 
'$single_ret_list_name' for 'vir$call->{ProcName}'\"),\n";
 print "   $single_ret_list_max_var, 
$single_ret_list_max_define);\n";
 print "goto done;\n";
 print "}\n";
@@ -1839,7 +1841,8 @@ elsif ($mode eq "client") {
 $modern_ret_as_list) {
 print "if 
(ret.$single_ret_list_name.${single_ret_list_name}_len > 
$single_ret_list_max_var) {\n";
 print "virReportError(VIR_ERR_RPC,\n";
-print "   _(\"too many remote 
${single_ret_list_error_msg_type}s: %d > %d\"),\n";
+print "   _(\"too many remote 
${single_ret_list_error_msg_type}s: %d > %d,\"\n";
+print " \"in parameter 
'$single_ret_list_name' for 'vir$call->{ProcName}'\"),\n";
 print "   
ret.$single_ret_list_name.${single_ret_list_name}_len, 
$single_ret_list_max_var);\n";
 print "goto cleanup;\n";
 print "}\n";
-- 
2.9.3

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

Re: [libvirt] CLI management tool

[Daniel P. Berrange]

On Thu, May 11, 2017 at 09:17:39AM +0200, Michal Privoznik wrote:
> Dear list,
> 
> you might have seen a discussion about virsh, and adding some new
> features to it [1]. While the feature was rejected, it got me thinking.
> What options do we offer for sysadmins that:
> 
> a) want to stay in command line
> b) want higher level mgmt of their domains
> c) yet want to manage a single host
> 
> Basically, virsh is just too low level for some operations (and using it
> in non-interactive mode from a script can mean hundreds of connections).
> Then we have virt-manager, which suits b) and c), but it's not a CLI
> tool. Therefore I was thinking whether we should start a new project on
> the top of libvirt that would fit all three points.
> 
> Personally, I've never been a sysadmin, so perhaps I am not the best one
> to write the tool. But I'm open for suggestions.
> 
> What do you think?

A long long long time ago, I did some work on creating an extensible
replacement for virsh.

The idea was to provide a minimal, pluggable framework for a CLI
tool using the GObject framework. Commands would emit structured
objects, and we'd have plugins to translate it to plain text,
JSON, XML, etc.  The commands themselves would also all be plugins,
likely written in a non-C language like Python/JavaScript/blah.

A key idea is that this tool would *not* simply target libvirt
APIs, it would be a more general virt shell that would include
other tasks. For example, 'virt-install' would just be a command
plugin, as would various libguestfs commands, and virt-top, etc
etc

By having commands written in a high level language, it was
inteded that sysadmins would be able to provide custom stuff
to automate jobs that they particularly cared about

I never got it developed far enough though to be worth publishing
it. Not even sure what happened to the code I did right...

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

Re: [libvirt] [PATCH] docs: install html fonts and related

[Daniel P. Berrange]

On Thu, May 11, 2017 at 12:01:27PM +0300, Nikolay Shirokovskiy wrote:
> ---
>  docs/Makefile.am | 22 --
>  1 file changed, 20 insertions(+), 2 deletions(-)

ACK


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

[libvirt] [PATCH] Add missing deps on virfilewrapper.h

[Daniel P. Berrange]

The test programs depend on virfilewrapper.h as well as the
virfilewrapper.c. Adding the dep ensures that virfilewrapper.h
gets included in the dist tarball.

Signed-off-by: Daniel P. Berrange 
---

Pushed as a build fix, since RPM build / distcheck is broken without this

 tests/Makefile.am | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/Makefile.am b/tests/Makefile.am
index 5c77b55..7673329 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -930,7 +930,7 @@ virconftest_SOURCES = \
 virconftest_LDADD = $(LDADDS)
 
 virhostcputest_SOURCES = \
-   virhostcputest.c testutils.h testutils.c virfilewrapper.c
+   virhostcputest.c testutils.h testutils.c virfilewrapper.h 
virfilewrapper.c
 virhostcputest_LDADD = $(LDADDS)
 
 commandtest_SOURCES = \
@@ -1147,7 +1147,7 @@ virhostcpumock_la_LIBADD = $(MOCKLIBS_LIBS)
 
 if WITH_LINUX
 vircaps2xmltest_SOURCES = \
-   vircaps2xmltest.c testutils.h testutils.c virfilewrapper.c
+   vircaps2xmltest.c testutils.h testutils.c virfilewrapper.h 
virfilewrapper.c
 vircaps2xmltest_LDADD = $(LDADDS)
 
 virnumamock_la_SOURCES = \
-- 
2.9.3

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

Re: [libvirt] [PATCH] nodedev_udev: Fix missing events when kernel report lots of udev events within a short time

[Daniel P. Berrange]

On Wed, May 10, 2017 at 01:38:00PM +0200, Erik Skultety wrote:
> On Tue, May 09, 2017 at 10:09:07AM +0800, ZhiPeng Lu wrote:
> > From: "ning.bo" 
> >
> > When create Virtual Function for Inter XL710 use below commands:
> > for i in `seq 0 1`; do
> > echo 63 > 
> > /sys/devices/pci:00/:00:03.2/:07:00.$i/sriov_numvfs
> > done
> > for i in `seq 0 3`; do
> > echo 31 > 
> > /sys/devices/pci:80/:80:02.2/:82:00.$i/sriov_numvfs
> > done
> >
> 
> Hi, I think this is worth a BZ, so we can track and test the issue. Now, I'm
> working on a similar issue which is very hard to reproduce and this looks like
> it could be reproduced easily with 100% chance, but I don't have any HW to try
> it on (it might take a while to get my hands on some). So, would you mind
> opening a BZ for this, attaching not only libvirtd's debug logs, but 
> preferably
> udev debug logs as well (libudev API is kinda poorly designed in this aspect
> and the only way to check for the real error is to enable the debugging and
> look into the logs).
> 
> > The libvirtd will missing some udev events, the result of libvirt-python API
> > listDevices('pci') will not list all pci devices.
> > The reason is that the buffer of udev monitor default size cann't save all 
> > udev
> > events reported by kernel.
> > So we need change buffer size so that we can receive as much events as 
> > possible
> > whitin a short time.
> >
> > Signed-off-by: ning.bo 
> > ---
> >  src/node_device/node_device_udev.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/src/node_device/node_device_udev.c 
> > b/src/node_device/node_device_udev.c
> > index 6e706a1..d813206 100644
> > --- a/src/node_device/node_device_udev.c
> > +++ b/src/node_device/node_device_udev.c
> > @@ -1564,6 +1564,7 @@ static int nodeStateInitialize(bool privileged,
> >  }
> >
> >  udev_monitor_enable_receiving(priv->udev_monitor);
> > +udev_monitor_set_receive_buffer_size(priv->udev_monitor, 128 * 1024 * 
> > 1024);
> >
> 
> The reason why I want to investigate the logs (ideally try it myself) is
> rather than blindly increasing the buffer size, maybe we receive just too many
> events, out of which some we might filter out and the problem would disappear.
> Anyhow, isn't 129 MiB bit of an overkill for a buffer size? I mean, you
> didn't provide any commentary on why you chose 128MiB specifically, I can only
> guess it somehow relates to the fact, that XL710 is capable of 128 VFs??

This matches what udevd sets for the buffer.

This method ends up setting the SO_RCVRBUFFORCE socket options. So we're
not allocating 128 MB in userspace - IIUC it just lets the kernel expand
it upto 128 MB if needed.  This is a privileged operation however, so
we better make sure we don't call this when running non-root.

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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