Re: [libvirt] [RFC PATCH 7/8] qemu: Prepare basic APIs to handle invalid defs

[Martin Kletzander]

On Tue, Sep 22, 2015 at 02:36:54PM +0100, Daniel P. Berrange wrote:

On Tue, Sep 22, 2015 at 03:26:26PM +0200, Martin Kletzander wrote:

On Tue, Sep 22, 2015 at 02:17:14PM +0100, Daniel P. Berrange wrote:
>On Tue, Sep 22, 2015 at 03:12:14PM +0200, Martin Kletzander wrote:
>>On Tue, Sep 22, 2015 at 01:48:01PM +0100, Daniel P. Berrange wrote:
>>>On Tue, Sep 22, 2015 at 02:15:53PM +0200, Martin Kletzander wrote:
In order for the user to be able to fix broken domains function
qemuDomainGetXMLDesc() needs to be able to lookup invalid domain
definitions and handle them properly.  When redefined, function
qemuDomainDefineXMLFlags() must clear the 'invalid XML' reason.  As a
nice addition, qemuDomainGetState() can lookup such domains without any
other change and that allows virsh not only to get their status, but
also to list them.
>>>
>>>Hmm, that's an interesting approach to the problem. I wonder though
>>>if we could do things slightly differently such that we don't need
>>>to change so many APIs.
>>>
>>>eg, just have a 'bool error' field in virDomainDefPtr. When loading
>>>the XML fails, populate a virDomainObjPtr/DefPtr as normal, but set
>>>the error field. Now we merely need to change the qemuDomainStart
>>>method, so it refuses to launch a VM with the 'error' flag set. All
>>>the other APIs could be essentially unchanged. Sure it would not
>>>be useful to allow things like virDomainAttachDevice, etc on such
>>>broken domains, but for sake of simplicity we can avoid touching
>>>all the methods except start.
>>>
>>
>>To be honest, I'm a afraid that we might forget some API that needs to
>>be blocked as well.  And we would have to go through all the APIs just
>>to see whether it accesses something that might be missing.  Moreover,
>>how would you decide what to skip at each error during parsing?  If,
>>for example, the  has some faulty attribute in one
>>subelement should we skip all the elements or just the one or just
>>skip that one particular attribute?  We would also not format the
>>faulty attribute to the XML being dumped, so the user wouldn't see
>>what's missing, which is even worse when there are two problems and
>>they fix only one, so we skip the other one.
>
>Oh, I wasn't suggesting changing the parser. I just mean that we would
>create a virDomainDefPtr instance which only contains the name and
>UUID, and ID field (if the guest is running from domain status XML).
>
>We'd leave all the rest of the config blank - our code ought to be
>able to deal with that, since essentially all config is optional
>aside from the name/uuid anyway.
>

Then probably I misunderstood because that's exactly what this series
is doing.


Right, but I mean without converting all the drivers to use this new
qemuDomObjFromDomainInvalid() method. I'm suggesting just having a
check in the start method only.



I don't think virDomainDef with only name, id and uuid set would not
cause problems, but to be honest I haven't tried that.  I just
remember how sometimes we rely on the fact that whatever we have in
virDomainDef went through parsing code.  I believe that if we remove
the safeguard and only add a check to *CreateXML*() functions, we'll
find some flaws.  Mainly information gathering functions that work
with enums that have no default values (e.g. virDomainVirtType).
Well, even without those, we will report false information.  No
pinning, no tuning, everything would return success with all
information lost.  So we'd add few checks here and there.  Then we'd
realize that (offline) migration will fail weirdly since
qemuMigrationIsAllowed() will return true and the first thing we will
do after that is formatting the XML.  We'd probably define way
different domain on the destination if that worked, I'm pretty sure
parsing would fail there.  Anyway, I think we'll end up adding more
explicit checks to the code in your case.  Way more than three calls
to a differently named function per driver.

Also, this can lead to a bigger cleanup, if we do some wrapper around
our APIs, we might move ACL checks, domain object gathering, job
creation and common API cleanups (*EndAPI()) and all other stuff in
it, so it won't be visible anyway.  But that's a future that won't
happen until I have way to much free time.


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


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

[libvirt] [PATCH] spec: Fix some warnings with latest rpmbuild

[Cole Robinson]

$ rpmbuild -ba libvirt.spec
warning: Macro expanded in comment on line 5: # If neither fedora nor rhel was 
defined, try to guess them from %{dist}

warning: Macro %enable_autotools defined but not used within scope
warning: Macro %client_only defined but not used within scope
...
---
 libvirt.spec.in | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/libvirt.spec.in b/libvirt.spec.in
index 48461e8..853d2ec 100644
--- a/libvirt.spec.in
+++ b/libvirt.spec.in
@@ -2,7 +2,7 @@
 
 # This spec file assumes you are building for Fedora 13 or newer,
 # or for RHEL 5 or newer. It may need some tweaks for other distros.
-# If neither fedora nor rhel was defined, try to guess them from %{dist}
+# If neither fedora nor rhel was defined, try to guess them from dist
 %if !0%{?rhel} && !0%{?fedora}
 %{expand:%(echo "%{?dist}" | \
   sed -ne 's/^\.el\([0-9]\+\).*/%%define rhel \1/p')}
@@ -13,13 +13,13 @@
 # Default to skipping autoreconf.  Distros can change just this one line
 # (or provide a command-line override) if they backport any patches that
 # touch configure.ac or Makefile.am.
-%{!?enable_autotools:%define enable_autotools 0}
+%{!?enable_autotools:%global enable_autotools 0}
 
 # A client only build will create a libvirt.so only containing
 # the generic RPC driver, and test driver and no libvirtd
 # Default to a full server + client build, but with the possibility
 # of a command-line or ~/.rpmmacros override for client-only.
-%{!?client_only:%define client_only 0}
+%{!?client_only:%global client_only 0}
 
 # Now turn off server build in certain cases
 
-- 
2.5.0

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

Re: [libvirt] [PATCH sandbox] docker: don't assume X-Docker-Token is set

[Cedric Bosdonnat]

On Mon, 2015-09-21 at 22:12 +0200, Cedric Bosdonnat wrote:
> On Mon, 2015-09-21 at 15:12 +0100, Daniel P. Berrange wrote:
> > The Red Hat docker registry (registry.access.redhat.com) does
> > not set any X-Docker-Token HTTP header in its responses. Change
> > the code so it only passes around this header if it is actually
> > present.
> > 
> > Signed-off-by: Daniel P. Berrange 
> > ---
> >  libvirt-sandbox/image/sources/DockerSource.py | 12 
> >  1 file changed, 8 insertions(+), 4 deletions(-)
> > 
> > diff --git a/libvirt-sandbox/image/sources/DockerSource.py 
> > b/libvirt-sandbox/image/sources/DockerSource.py
> > index f367c8f..78b2a53 100644
> > --- a/libvirt-sandbox/image/sources/DockerSource.py
> > +++ b/libvirt-sandbox/image/sources/DockerSource.py
> > @@ -83,10 +83,14 @@ class DockerSource(Source):
> >  checksums = {}
> >  for layer in data:
> >  pass
> > +
> > +headers = {}
> > +if token is not None:
> > +headers["Authorization"] = "Token" + token
> >  (data, res) = self._get_json(template,
> >   registryendpoint,
> >   "/v1/repositories" + template.path + 
> > "/tags",
> > - { "Authorization": "Token " + token })
> > + headers)
> >  
> >  cookie = res.info().getheader('Set-Cookie')
> >  
> > @@ -98,7 +102,7 @@ class DockerSource(Source):
> >  (data, res) = self._get_json(template,
> >   registryendpoint,
> >   "/v1/images/" + imagetagid + 
> > "/ancestry",
> > - { "Authorization": "Token "+ token })
> > + headers)
> >  
> >  if data[0] != imagetagid:
> >  raise ValueError(["Expected first layer id '%s' to match image 
> > id '%s'",
> > @@ -121,7 +125,7 @@ class DockerSource(Source):
> >  res = self._save_data(template,
> >registryendpoint,
> >"/v1/images/" + layerid + 
> > "/json",
> > -  { "Authorization": "Token " + 
> > token },
> > +  headers,
> >jsonfile)
> >  createdFiles.append(jsonfile)
> >  
> > @@ -134,7 +138,7 @@ class DockerSource(Source):
> >  self._save_data(template,
> >  registryendpoint,
> >  "/v1/images/" + layerid + "/layer",
> > -{ "Authorization": "Token "+token },
> > +headers,
> >  datafile, datacsum, layersize)
> >  createdFiles.append(datafile)
> >  
> 
> ACK

Hum, after some real life testing, I'm getting an error from the default
docker registry with this patch:

Fetching
https://registry-1.docker.io/v1/repositories/opensuse/tags...FAIL HTTP
Error 401: UNAUTHORIZED

--
Cedric

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

Re: [libvirt] [PATCH v2 sandbox] virt-sandbox-image: switch to use URI to identify templates

[Cedric Bosdonnat]

On Mon, 2015-09-21 at 22:11 +0200, Cedric Bosdonnat wrote:
> On Mon, 2015-09-21 at 15:45 +0100, Daniel P. Berrange wrote:
> > Currently the CLI syntax is somewhat docker specific requiring
> > inclusion of --registry arg to identify the docker download
> > server. Other app containers have a notion of download server,
> > but don't separate it from the template name.
> > 
> > This patch removes that docker-ism by changing to use a URI
> > for identifying the template image. So instead of
> > 
> >   virt-sandbox-image download \
> >   --source docker --registry index.docker.io
> >   --username dan --password 123456 ubuntu:15.04
> > 
> > You can use
> > 
> >   virt-sandbox-image download 
> > docker://dan:123...@index.docker.io/ubuntu?tag=15.04
> > 
> > The only mandatory part is the source prefix and image name, so
> > that can shorten to just
> > 
> >   virt-sandbox-image download docker:///ubuntu
> > 
> > to pull down the latest ubuntu image, from the default registry
> > using no authentication.
> > ---
> > 
> > Changed in v2:
> > 
> >  - Rebase against master, instead of (unpushed) docker volume patch
> > 
> >  libvirt-sandbox/image/cli.py  |  71 +
> >  libvirt-sandbox/image/sources/DockerSource.py | 142 
> > ++
> >  libvirt-sandbox/image/sources/Source.py   |  29 +++---
> >  libvirt-sandbox/image/template.py | 110 
> 
> Missing change in libvirt-sandbox/image/Makefile.am to add template.py.
> As is that file isn't installed.
> 
> I'm also just realizing that we didn't add Eren't commit for the
> virt-sandbox-image man page. Adding it later is fine, but we need to
> keep that on our radar.
> 
> >  4 files changed, 228 insertions(+), 124 deletions(-)
> >  create mode 100644 libvirt-sandbox/image/template.py
> > 
> > diff --git a/libvirt-sandbox/image/cli.py b/libvirt-sandbox/image/cli.py
> > index 1718cc5..4d02289 100755
> > --- a/libvirt-sandbox/image/cli.py
> > +++ b/libvirt-sandbox/image/cli.py
> > @@ -3,7 +3,7 @@
> >  # Authors: Daniel P. Berrange 
> >  #  Eren Yagdiran 
> >  #
> > -# Copyright (C) 2013 Red Hat, Inc.
> > +# Copyright (C) 2013-2015 Red Hat, Inc.
> >  # Copyright (C) 2015 Universitat Politècnica de Catalunya.
> >  #
> >  # This program is free software; you can redistribute it and/or modify
> > @@ -34,6 +34,8 @@ import subprocess
> >  import random
> >  import string
> >  
> > +from libvirt_sandbox.image import template
> > +
> >  if os.geteuid() == 0:
> >  default_template_dir = "/var/lib/libvirt/templates"
> >  default_image_dir = "/var/lib/libvirt/images"
> > @@ -44,15 +46,6 @@ else:
> >  debug = False
> >  verbose = False
> >  
> > -import importlib
> > -def dynamic_source_loader(name):
> > -name = name[0].upper() + name[1:]
> > -modname = "libvirt_sandbox.image.sources." + name + "Source"
> > -mod = importlib.import_module(modname)
> > -classname = name + "Source"
> > -classimpl = getattr(mod, classname)
> > -return classimpl()
> > -
> >  gettext.bindtextdomain("libvirt-sandbox", "/usr/share/locale")
> >  gettext.textdomain("libvirt-sandbox")
> >  try:
> > @@ -73,11 +66,10 @@ def info(msg):
> >  
> >  def download(args):
> >  try:
> > -
> > dynamic_source_loader(args.source).download_template(templatename=args.template,
> > - 
> > templatedir=args.template_dir,
> > - 
> > registry=args.registry,
> > - 
> > username=args.username,
> > - 
> > password=args.password)
> > +tmpl = template.Template.from_uri(args.template)
> > +source = tmpl.get_source_impl()
> > +source.download_template(template=tmpl,
> > + templatedir=args.template_dir)
> >  except IOError,e:
> >  print "Source %s cannot be found in given path" %args.source

This IOError exception message should be rephrased or handled with the
general Exception.

--
Cedric

> >  except Exception,e:
> > @@ -85,17 +77,21 @@ def download(args):
> >  
> >  def delete(args):
> >  try:
> > -
> > dynamic_source_loader(args.source).delete_template(templatename=args.template,
> > -   
> > templatedir=args.template_dir)
> > +tmpl = template.Template.from_uri(args.template)
> > +source = tmpl.get_source_impl()
> > +source.delete_template(template=tmpl,
> > +   templatedir=args.template_dir)
> >  except Exception,e:
> >  print "Delete Error %s", str(e)
> >  
> >  def create(args):
> >  try:
> > -
> > dynamic_source_loader(args.source).create_template(templatename=args.template,
> > - 

Re: [libvirt] [PATCH 3/4] Use mockup cache

[Ján Tomko]

On Mon, Sep 21, 2015 at 03:03:20PM +0200, Michal Privoznik wrote:
> On 15.09.2015 10:05, Ján Tomko wrote:
> > From: Pavel Fedin 
> > 
> > Use the new API in order to correctly add capability sets to the cache
> > before parsing XML files
> > 
> > Signed-off-by: Pavel Fedin 
> 
> s/^/tests: / in $SUBJ.
> 
> > ---
> >  tests/qemuhotplugtest.c  | 23 +++
> >  tests/qemuxml2argvtest.c |  6 ++
> >  tests/qemuxmlnstest.c|  6 ++
> >  3 files changed, 27 insertions(+), 8 deletions(-)
> > 
> > diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c
> > index 3cf7f36..109d820 100644
> > --- a/tests/qemuhotplugtest.c
> > +++ b/tests/qemuhotplugtest.c
> > @@ -57,7 +57,7 @@ static int
> >  qemuHotplugCreateObjects(virDomainXMLOptionPtr xmlopt,
> >   virDomainObjPtr *vm,
> >   const char *domxml,
> > - bool event)
> > + bool event, const char *testname)
> >  {
> >  int ret = -1;
> >  qemuDomainObjPrivatePtr priv = NULL;
> > @@ -65,12 +65,6 @@ qemuHotplugCreateObjects(virDomainXMLOptionPtr xmlopt,
> >  if (!(*vm = virDomainObjNew(xmlopt)))
> >  goto cleanup;
> >  
> > -if (!((*vm)->def = virDomainDefParseString(domxml,
> > -   driver.caps,
> > -   driver.xmlopt,
> > -   
> > VIR_DOMAIN_DEF_PARSE_INACTIVE)))
> > -goto cleanup;
> > -
> >  priv = (*vm)->privateData;
> >  
> >  if (!(priv->qemuCaps = virQEMUCapsNew()))
> > @@ -85,6 +79,18 @@ qemuHotplugCreateObjects(virDomainXMLOptionPtr xmlopt,
> >  if (event)
> >  virQEMUCapsSet(priv->qemuCaps, QEMU_CAPS_DEVICE_DEL_EVENT);
> >  
> > +qemuTestCapsName = testname;
> > +ret = qemuTestCapsCacheInsert(driver.qemuCapsCache, testname,
> > +  priv->qemuCaps);
> 
> 
> I think that @qemuTestCapsName should be set in
> qemuTestCapsCacheInsert(). On its successful return.
> 

It makes sense since we always overwrite it.

Anything else?

Jan


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

[libvirt] [libvirt-guests] New feature ALWAYS_START

[Marek Lukács]

Hi,

It will be nice feature to have configuration option
ALWAYS_START="$uri:$name $uri:name ..." in libvirt-guests
configuration file.

If ON_BOOT is "start" and if ALWAYS_START is not empty, it iterates
over the ALWAYS_START and starts guests with same conditions (delays
etc.) before it starts guests from LISTFILE.

Benefits:
- guests are started with delays
- guests are started after host failure
- guests are started in specific order (for example complex
environment, when DB should be started before other guest, etc.)

Regards,

Marek Lukács

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

Re: [libvirt] New software based on libvirt

[Gustav Fransson Nyvell]

Feel free to! I don't know where that file is at the moment. :) Thanks!
//Gustav

Den 2015-09-22 kl. 11:12, skrev Michal Privoznik:

On 21.09.2015 21:28, Gustav Fransson Nyvell wrote:

Hello,

I'm introducing to you the decentralized cloud Cherrypop.

Combining libvirt and LizardFS (as of now) it becomes a cloud completely
without masters. Thus, any node is sufficient for the cloud to be up
and therefore no wasted resources and no single point of failure.

It's still pretty crude software but will work with some tinkering. Hope
you try it and like it!

For more information, source and binary:
https://github.com/gustavfranssonnyvell/cherrypop

//Gustav


Hey,

what an awesome news! We keep list of software based on libvirt:

 http://libvirt.org/apps.html

Mind posting a patch against docs/apps.html.in? Or I can do that too, if
you don't want to bother.

Michal


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

Re: [libvirt] [PATCH] spec: Fix some warnings with latest rpmbuild

[Cole Robinson]

On 09/22/2015 04:40 PM, Eric Blake wrote:
> On 09/22/2015 01:57 PM, Cole Robinson wrote:
>> $ rpmbuild -ba libvirt.spec
>> warning: Macro expanded in comment on line 5: # If neither fedora nor rhel 
>> was defined, try to guess them from %{dist}
>>
>> warning: Macro %enable_autotools defined but not used within scope
>> warning: Macro %client_only defined but not used within scope
>> ...
>> ---
>>  libvirt.spec.in | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
>>  # Default to skipping autoreconf.  Distros can change just this one line
>>  # (or provide a command-line override) if they backport any patches that
>>  # touch configure.ac or Makefile.am.
>> -%{!?enable_autotools:%define enable_autotools 0}
>> +%{!?enable_autotools:%global enable_autotools 0}
> 
> I guess %global works. I might have tried:
> 
> %if !0%{?enable_autotools}
> %define enable_autotools 0
> %endif
> 
> but that's longer.
> 
> ACK
> 

Thanks, pushed now

- Cole

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

Re: [libvirt] [PATCH 2/5] qemuDomainCreateXML: Make domain definition transient

[Jim Fehlig]

On 09/22/2015 09:28 AM, Michal Privoznik wrote:

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

So, you want to create a domain from XML. The domain already
exists in libvirt's database of domains. It's okay, because name
and UUID matches. However, on domain startup, internal
representation of the domain is overwritten with your XML even
though we claim that the XML you've provided is a transient one.
Le sigh.

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

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 30d2d98..2a4b026 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -1745,6 +1745,7 @@ static virDomainPtr qemuDomainCreateXML(virConnectPtr 
conn,
  
  if (!(vm = virDomainObjListAdd(driver->domains, def,

 driver->xmlopt,
+   VIR_DOMAIN_OBJ_LIST_ADD_LIVE |
 VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE,
 NULL)))
  goto cleanup;


It looks like the libxl driver should be fixed similarly, right?

Regards,
Jim

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

[libvirt] [PATCH] Makefile: fix build fail when make rpm

[Luyao Huang]

Build fail and error like this:

  CC   qemu/libvirt_driver_qemu_impl_la-qemu_command.lo
qemu/qemu_capabilities.c:46:27: fatal error: qemu_capspriv.h: No such file or 
directory
 #include "qemu_capspriv.h"

Add qemu_capspriv.h to source.

Signed-off-by: Luyao Huang 
---
 src/Makefile.am | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/Makefile.am b/src/Makefile.am
index 060abe8..07d5879 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -763,7 +763,8 @@ QEMU_DRIVER_SOURCES =   
\
qemu/qemu_monitor_json.c\
qemu/qemu_monitor_json.h\
qemu/qemu_driver.c qemu/qemu_driver.h   \
-   qemu/qemu_interface.c qemu/qemu_interface.h
+   qemu/qemu_interface.c qemu/qemu_interface.h \
+   qemu/qemu_capspriv.h
 
 XENAPI_DRIVER_SOURCES =\
xenapi/xenapi_driver.c xenapi/xenapi_driver.h   \
-- 
1.8.3.1

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

Re: [libvirt] [PATCH] spec: Fix some warnings with latest rpmbuild

[Eric Blake]

On 09/22/2015 01:57 PM, Cole Robinson wrote:
> $ rpmbuild -ba libvirt.spec
> warning: Macro expanded in comment on line 5: # If neither fedora nor rhel 
> was defined, try to guess them from %{dist}
> 
> warning: Macro %enable_autotools defined but not used within scope
> warning: Macro %client_only defined but not used within scope
> ...
> ---
>  libvirt.spec.in | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

>  # Default to skipping autoreconf.  Distros can change just this one line
>  # (or provide a command-line override) if they backport any patches that
>  # touch configure.ac or Makefile.am.
> -%{!?enable_autotools:%define enable_autotools 0}
> +%{!?enable_autotools:%global enable_autotools 0}

I guess %global works. I might have tried:

%if !0%{?enable_autotools}
%define enable_autotools 0
%endif

but that's longer.

ACK

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



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

[libvirt] [PATCH] add compress stream support

[Vasiliy Tolstov]

Some libvirt functions use streams, this patch add
compress stream support.
So VolumeDownload/VolumeUpload can greatly speedup by using
compressed streams to save network bandtwidth and don't transfer
zero bytes (in case of raw disk format)

Signed-off-by: Vasiliy Tolstov 
---
 configure.ac |  2 ++
 daemon/Makefile.am   |  3 +++
 include/libvirt/libvirt-stream.h |  3 +++
 m4/virt-archive.m4   | 29 ++
 src/Makefile.am  | 17 +++-
 src/datatypes.h  |  7 +++
 src/fdstream.c   | 44 
 src/libvirt-stream.c | 15 ++
 tools/Makefile.am| 17 ++--
 9 files changed, 130 insertions(+), 7 deletions(-)
 create mode 100644 m4/virt-archive.m4

diff --git a/configure.ac b/configure.ac
index 03463b0..58a15bb 100644
--- a/configure.ac
+++ b/configure.ac
@@ -250,6 +250,7 @@ LIBVIRT_CHECK_SANLOCK
 LIBVIRT_CHECK_SASL
 LIBVIRT_CHECK_SELINUX
 LIBVIRT_CHECK_SSH2
+LIBVIRT_CHECK_LIBARCHIVE
 LIBVIRT_CHECK_SYSTEMD_DAEMON
 LIBVIRT_CHECK_UDEV
 LIBVIRT_CHECK_WIRESHARK
@@ -2892,6 +2893,7 @@ LIBVIRT_RESULT_SANLOCK
 LIBVIRT_RESULT_SASL
 LIBVIRT_RESULT_SELINUX
 LIBVIRT_RESULT_SSH2
+LIBVIRT_RESULT_LIBARCHIVE
 LIBVIRT_RESULT_SYSTEMD_DAEMON
 LIBVIRT_RESULT_UDEV
 LIBVIRT_RESULT_WIRESHARK
diff --git a/daemon/Makefile.am b/daemon/Makefile.am
index be1b5a9..a4d63eb 100644
--- a/daemon/Makefile.am
+++ b/daemon/Makefile.am
@@ -177,17 +177,20 @@ libvirtd_CFLAGS = \
$(XDR_CFLAGS) $(DBUS_CFLAGS) $(LIBNL_CFLAGS) \
$(WARN_CFLAGS) $(PIE_CFLAGS) \
$(COVERAGE_CFLAGS) \
+   $(LIBARCHIVE_CFLAGS) \
-DQEMUD_PID_FILE="\"$(QEMUD_PID_FILE)\""
 
 libvirtd_LDFLAGS = \
$(RELRO_LDFLAGS)\
$(PIE_LDFLAGS)  \
$(COVERAGE_LDFLAGS) \
+   $(LIBARCHIVE_LDFLAGS)   \
$(NO_INDIRECT_LDFLAGS)  \
$(NULL)
 
 libvirtd_LDADD =   \
$(LIBXML_LIBS)  \
+   $(LIBARCHIVE_LIBS)  \
$(GNUTLS_LIBS)  \
$(SASL_LIBS)\
$(DBUS_LIBS)\
diff --git a/include/libvirt/libvirt-stream.h b/include/libvirt/libvirt-stream.h
index 831640d..c75f03e 100644
--- a/include/libvirt/libvirt-stream.h
+++ b/include/libvirt/libvirt-stream.h
@@ -31,10 +31,13 @@
 
 typedef enum {
 VIR_STREAM_NONBLOCK = (1 << 0),
+VIR_STREAM_COMPRESS_GZIP = (1 << 1),
+VIR_STREAM_COMPRESS_XZ = (1 << 2),
 } virStreamFlags;
 
 virStreamPtr virStreamNew(virConnectPtr conn,
   unsigned int flags);
+
 int virStreamRef(virStreamPtr st);
 
 int virStreamSend(virStreamPtr st,
diff --git a/m4/virt-archive.m4 b/m4/virt-archive.m4
new file mode 100644
index 000..9770732
--- /dev/null
+++ b/m4/virt-archive.m4
@@ -0,0 +1,29 @@
+dnl The libarchive.so library
+dnl
+dnl Copyright (C) 2012-2013 Red Hat, Inc.
+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_CHECK_LIBARCHIVE],[
+  LIBVIRT_CHECK_PKG([LIBARCHIVE], [libarchive], [3.1.2])
+  AC_SUBST(LIBARCHIVE_CFLAGS)
+  AC_SUBST(LIBARCHIVE_LIBS)
+  AC_SUBST(LIBARCHIVE_LDFLAGS)
+])
+
+AC_DEFUN([LIBVIRT_RESULT_LIBARCHIVE],[
+  LIBVIRT_RESULT_LIB([LIBARCHIVE])
+])
diff --git a/src/Makefile.am b/src/Makefile.am
index 060abe8..429c2c7 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -25,11 +25,11 @@ abs_topsrcdir = $(shell cd $(top_srcdir) && pwd)
 # No libraries with the exception of LIBXML should be listed
 # here. List them against the individual XXX_la_CFLAGS targets
 # that actually use them. Also keep GETTEXT_CPPFLAGS at the end.
-INCLUDES = -I../gnulib/lib \
+INCLUDES = -I../gnulib/lib \
-I$(top_srcdir)/gnulib/lib  \
-   -I$(top_srcdir) \
+   -I$(top_srcdir) \
-I../include 

Re: [libvirt] [PATCH] add compress stream support

[Vasiliy Tolstov]

2015-09-22 18:29 GMT+03:00 Daniel P. Berrange :
> On Tue, Sep 22, 2015 at 02:10:41PM +, Vasiliy Tolstov wrote:
>> use libarchive for compressed stream support
>
> Can you explain a bit more about how you expect this to be
> used ?


Thanks for all suggestions. I'm send new version.

-- 
Vasiliy Tolstov,
e-mail: v.tols...@selfip.ru

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

Re: [libvirt] [PATCH] add compress stream support

[Vasiliy Tolstov]

2015-09-23 1:26 GMT+03:00 Vasiliy Tolstov :
> Some libvirt functions use streams, this patch add
> compress stream support.
> So VolumeDownload/VolumeUpload can greatly speedup by using
> compressed streams to save network bandtwidth and don't transfer
> zero bytes (in case of raw disk format)


This is new version. Now i'm try to test it. But firstly - i have many
Makefile.am writings, to fix linking errors. Can i add globally
libarchive cflags and libs to all libvirt libraries?

-- 
Vasiliy Tolstov,
e-mail: v.tols...@selfip.ru

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

Re: [libvirt] [PATCH 0/3] Introduce new virtType enum item

[Shivangi Dhir]

Hi,

On Tue, Sep 22, 2015 at 6:53 PM, Daniel P. Berrange 
wrote:
>
> On Tue, Sep 22, 2015 at 07:50:41AM -0400, John Ferlan wrote:
> >
> >
> > On 09/17/2015 04:46 AM, Shivangi Dhir wrote:
> > > These patches solve a BiteSizedTask - Introduce new virtType enum
item[0]
> > >
> > > [0]
http://wiki.libvirt.org/page/BiteSizedTasks#Introduce_new_virtType_enum_item
> > >
> > > Shivangi Dhir (3):
> > >   conf: Add new VIR_DOMAIN_VIRT_NONE enum
> > >   qemu: Make virtType of type virDomainVirtType
> > >   conf: Change the description of virCapabilitiesDomainDataLookup()
> > >
> > >  src/conf/capabilities.c  |  6 +++---
> > >  src/conf/domain_conf.c   |  3 ++-
> > >  src/conf/domain_conf.h   |  3 ++-
> > >  src/qemu/qemu_monitor.c  |  2 +-
> > >  src/qemu/qemu_monitor.h  |  2 +-
> > >  src/qemu/qemu_monitor_json.c |  2 +-
> > >  src/qemu/qemu_monitor_json.h |  2 +-
> > >  src/qemu/qemu_monitor_text.c |  2 +-
> > >  src/qemu/qemu_monitor_text.h |  2 +-
> > >  tests/qemumonitorjsontest.c  |  2 +-
> > >  tests/vircapstest.c  | 32 
> > >  11 files changed, 30 insertions(+), 28 deletions(-)
> > >
> >
> > While yes this is a bite sized task, I do have a concern with changing
> > the meaning of undefined from -1 to 0. Doing so will change (increase by
> > 1) each of the VIR_DOMAIN_VIRT_* values, e.g. VIR_DOMAIN_VIRT_QEMU
> > changes from 0 to 1. If you look through the history of changes to
> > virDomainVirtType you'll note it's been appended to and never had
> > something inserted in the middle.
> >
> > Inserting in the middle is not a problem if we could "guarantee" that
> > nothing exists (saved) or is currently running that references an
> > existing numerical value. Unfortunately, I'm not sure we can do that.
> >
> > As seen in patch 2, there is a 'virtType' value in the domain
> > definition. A running domain VIR_DOMAIN_VIRT_QEMU would have been
> > started with "0" in that field. When a new libvirtd with these changes
> > is in place, the running domain would still have "0" stored away and
> > that would mean something different.
> >
> > Furthermore (or likewise), a migration from an "older" host to a newer
> > host would have a different virtType value and I would think cause
> > virDomainDefCheckABIStability to complain. Similarly, a 'saved' domain
> > would have a numerical anomaly - although for that case it's less clear
> > whether we save the physical name or the numerical value.
> >
> > Of course I could be off-base/wrong, but let's see what others say...
>
> In general we should never be serializing the raw integer enum
> values only the string representation. If we did have somewhere
> that used an integer representation, we'd certainly have to avoid
> this change as you mention. I think we're probably safe though
> unless someone knows of a place we don't use the string rep.
>
> >
> > FWIW: For a "first" patch set - it seems you've followed the guidelines
> > quite well. About the only suggestions I have are in patch1 the
> > comparison "if (domaintype)" in virCapabilitiesDomainDataLookupInternal
> > could be "if (domaintype > VIR_DOMAIN_VIRT_NONE)" as well as patch3
> > should be merged with patch1 and the "int domaintype" in the args list
> > for virCapabilitiesDomainDataLookup (and capabilities.h) should be
> > "virDomainVirtType domaintype".
>

Thanks alot for your feedback.
Should I modify and resend the patches after making the changes suggested
above, if there are no other issues ?


--
Regards,
Shivangi Dhir
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 3/4] Use mockup cache

[Michal Privoznik]

On 22.09.2015 08:27, Ján Tomko wrote:
> On Mon, Sep 21, 2015 at 03:03:20PM +0200, Michal Privoznik wrote:
>> On 15.09.2015 10:05, Ján Tomko wrote:
>>> From: Pavel Fedin 
>>>
>>> Use the new API in order to correctly add capability sets to the cache
>>> before parsing XML files
>>>
>>> Signed-off-by: Pavel Fedin 
>>
>> s/^/tests: / in $SUBJ.
>>
>>> ---
>>>  tests/qemuhotplugtest.c  | 23 +++
>>>  tests/qemuxml2argvtest.c |  6 ++
>>>  tests/qemuxmlnstest.c|  6 ++
>>>  3 files changed, 27 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c
>>> index 3cf7f36..109d820 100644
>>> --- a/tests/qemuhotplugtest.c
>>> +++ b/tests/qemuhotplugtest.c
>>> @@ -57,7 +57,7 @@ static int
>>>  qemuHotplugCreateObjects(virDomainXMLOptionPtr xmlopt,
>>>   virDomainObjPtr *vm,
>>>   const char *domxml,
>>> - bool event)
>>> + bool event, const char *testname)
>>>  {
>>>  int ret = -1;
>>>  qemuDomainObjPrivatePtr priv = NULL;
>>> @@ -65,12 +65,6 @@ qemuHotplugCreateObjects(virDomainXMLOptionPtr xmlopt,
>>>  if (!(*vm = virDomainObjNew(xmlopt)))
>>>  goto cleanup;
>>>  
>>> -if (!((*vm)->def = virDomainDefParseString(domxml,
>>> -   driver.caps,
>>> -   driver.xmlopt,
>>> -   
>>> VIR_DOMAIN_DEF_PARSE_INACTIVE)))
>>> -goto cleanup;
>>> -
>>>  priv = (*vm)->privateData;
>>>  
>>>  if (!(priv->qemuCaps = virQEMUCapsNew()))
>>> @@ -85,6 +79,18 @@ qemuHotplugCreateObjects(virDomainXMLOptionPtr xmlopt,
>>>  if (event)
>>>  virQEMUCapsSet(priv->qemuCaps, QEMU_CAPS_DEVICE_DEL_EVENT);
>>>  
>>> +qemuTestCapsName = testname;
>>> +ret = qemuTestCapsCacheInsert(driver.qemuCapsCache, testname,
>>> +  priv->qemuCaps);
>>
>>
>> I think that @qemuTestCapsName should be set in
>> qemuTestCapsCacheInsert(). On its successful return.
>>
> 
> It makes sense since we always overwrite it.
> 
> Anything else?
> 

No, I think if you fix this locally, you're good to push it.

ACK with my suggestion worked in.

Michal

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

Re: [libvirt] New software based on libvirt

[Michal Privoznik]

On 21.09.2015 21:28, Gustav Fransson Nyvell wrote:
> Hello,
> 
> I'm introducing to you the decentralized cloud Cherrypop.
> 
> Combining libvirt and LizardFS (as of now) it becomes a cloud completely
> without masters. Thus, any node is sufficient for the cloud to be up
> and therefore no wasted resources and no single point of failure.
> 
> It's still pretty crude software but will work with some tinkering. Hope
> you try it and like it!
> 
> For more information, source and binary:
> https://github.com/gustavfranssonnyvell/cherrypop
> 
> //Gustav
> 

Hey,

what an awesome news! We keep list of software based on libvirt:

http://libvirt.org/apps.html

Mind posting a patch against docs/apps.html.in? Or I can do that too, if
you don't want to bother.

Michal

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

Re: [libvirt] [PATCH v2 sandbox] virt-sandbox-image: switch to use URI to identify templates

[Daniel P. Berrange]

On Tue, Sep 22, 2015 at 09:13:53AM +0200, Cedric Bosdonnat wrote:
> On Mon, 2015-09-21 at 22:11 +0200, Cedric Bosdonnat wrote:
> > On Mon, 2015-09-21 at 15:45 +0100, Daniel P. Berrange wrote:
> > > +tmpl = template.Template.from_uri(args.template)
> > > +source = tmpl.get_source_impl()
> > > +source.download_template(template=tmpl,
> > > + templatedir=args.template_dir)
> > >  except IOError,e:
> > >  print "Source %s cannot be found in given path" %args.source
> 
> This IOError exception message should be rephrased or handled with the
> general Exception.

Opps, yes, still accessing the deleted cli arg field.


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

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

Re: [libvirt] [PATCH v2 sandbox] virt-sandbox-image: switch to use URI to identify templates

[Daniel P. Berrange]

On Mon, Sep 21, 2015 at 10:11:48PM +0200, Cedric Bosdonnat wrote:
> On Mon, 2015-09-21 at 15:45 +0100, Daniel P. Berrange wrote:
> > Currently the CLI syntax is somewhat docker specific requiring
> > inclusion of --registry arg to identify the docker download
> > server. Other app containers have a notion of download server,
> > but don't separate it from the template name.
> > 
> > This patch removes that docker-ism by changing to use a URI
> > for identifying the template image. So instead of
> > 
> >   virt-sandbox-image download \
> >   --source docker --registry index.docker.io
> >   --username dan --password 123456 ubuntu:15.04
> > 
> > You can use
> > 
> >   virt-sandbox-image download 
> > docker://dan:123...@index.docker.io/ubuntu?tag=15.04
> > 
> > The only mandatory part is the source prefix and image name, so
> > that can shorten to just
> > 
> >   virt-sandbox-image download docker:///ubuntu
> > 
> > to pull down the latest ubuntu image, from the default registry
> > using no authentication.
> > ---
> > 
> > Changed in v2:
> > 
> >  - Rebase against master, instead of (unpushed) docker volume patch
> > 
> >  libvirt-sandbox/image/cli.py  |  71 +
> >  libvirt-sandbox/image/sources/DockerSource.py | 142 
> > ++
> >  libvirt-sandbox/image/sources/Source.py   |  29 +++---
> >  libvirt-sandbox/image/template.py | 110 
> 
> Missing change in libvirt-sandbox/image/Makefile.am to add template.py.
> As is that file isn't installed.
> 
> I'm also just realizing that we didn't add Eren't commit for the
> virt-sandbox-image man page. Adding it later is fine, but we need to
> keep that on our radar.

Yep, that's in my tree to update & pyush.

> > @@ -151,7 +150,7 @@ def run(args):
> >  
> >  def requires_template(parser):
> >  parser.add_argument("template",
> > -help=_("name of the template"))
> > +help=_("URI of the template"))
> 
> Shouldn't we provide some examples here? As those URIs can't be invented
> we need to give the user some chances to discover them without having to
> read our code ;)

I wasn't sure this was the best place. We'll certainly put examples in
the man pages though.

> ACK, with the help improvement + Makefile.am fix.

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

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

Re: [libvirt] [PATCH V2 0/9] support multi-thread compress migration.

[Feng, Shaohe]

Ping. 

> -Original Message-
> From: Feng, Shaohe
> Sent: Monday, August 10, 2015 5:32 PM
> To: 'libvir-list@redhat.com'
> Cc: 'Jiri Denemark'; 'Eric Blake'; Li, Liang Z; Dugger, Donald D
> Subject: RE: [PATCH V2 0/9] support multi-thread compress migration.
> 
> Hi, Eric and Jiri
> 
> Can you have a look at this patch and give some comments at your leisure?
> 
> 
> 
> > -Original Message-
> > From: Feng, Shaohe
> > Sent: Thursday, July 9, 2015 9:02 PM
> > To: libvir-list@redhat.com
> > Cc: Jiri Denemark; Eric Blake; Li, Liang Z; Feng, Shaohe
> > Subject: [PATCH V2 0/9] support multi-thread compress migration.
> >
> > These series patches support multi-thread compress during live migration.
> >
> > Eli Qiao (4):
> >   Add test cases for qemuMonitorJSONGetMigrationParameter
> >   remote: Add support for set and get multil thread migration parameters
> >   qemu_driver: Add support to set/get migration parameters.
> >   virsh: Add set and get multi-thread migration parameters commands
> >
> > ShaoHe Feng (5):
> >   qemu_migration: Add support for mutil-thread compressed migration
> > enable
> >   qemu: Add monitor API for get/set migration parameters
> >   set multi-thread compress params for Migrate3 during live migration
> >   virsh: add multi-thread migration option for live migrate command
> >   Implement the public APIs for multi-thread compress parameters.
> >
> >  .gnulib  |   2 +-
> >  daemon/remote.c  |  62 +++
> >  include/libvirt/libvirt-domain.h |  31 ++
> >  src/driver-hypervisor.h  |  14 +++
> >  src/libvirt-domain.c | 110 +++
> >  src/libvirt_public.syms  |   5 +
> >  src/qemu/qemu_domain.h   |   3 +
> >  src/qemu/qemu_driver.c   | 186 
> >  src/qemu/qemu_migration.c| 105 ++
> >  src/qemu/qemu_migration.h|  32 --
> >  src/qemu/qemu_monitor.c  |  40 ++-
> >  src/qemu/qemu_monitor.h  |  11 ++
> >  src/qemu/qemu_monitor_json.c |  93 
> >  src/qemu/qemu_monitor_json.h |   9 ++
> >  src/qemu/qemu_monitor_text.c |  95 +
> >  src/qemu/qemu_monitor_text.h |  10 ++
> >  src/remote/remote_driver.c   |  54 ++
> >  src/remote/remote_protocol.x |  30 +-
> >  src/remote_protocol-structs  |  26 +
> >  tests/qemumonitorjsontest.c  |  53 ++
> >  tools/virsh-domain.c | 223 
> > ++-
> >  tools/virsh.pod  |  37 +--
> >  22 files changed, 1212 insertions(+), 19 deletions(-)
> >
> > --
> > 2.1.4


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

Re: [libvirt] [PATCH sandbox] docker: don't assume X-Docker-Token is set

[Daniel P. Berrange]

On Tue, Sep 22, 2015 at 09:10:48AM +0200, Cedric Bosdonnat wrote:
> On Mon, 2015-09-21 at 22:12 +0200, Cedric Bosdonnat wrote:
> > On Mon, 2015-09-21 at 15:12 +0100, Daniel P. Berrange wrote:
> > > The Red Hat docker registry (registry.access.redhat.com) does
> > > not set any X-Docker-Token HTTP header in its responses. Change
> > > the code so it only passes around this header if it is actually
> > > present.
> > > 
> > > Signed-off-by: Daniel P. Berrange 
> > > ---
> > >  libvirt-sandbox/image/sources/DockerSource.py | 12 
> > >  1 file changed, 8 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/libvirt-sandbox/image/sources/DockerSource.py 
> > > b/libvirt-sandbox/image/sources/DockerSource.py
> > > index f367c8f..78b2a53 100644
> > > --- a/libvirt-sandbox/image/sources/DockerSource.py
> > > +++ b/libvirt-sandbox/image/sources/DockerSource.py
> > > @@ -83,10 +83,14 @@ class DockerSource(Source):
> > >  checksums = {}
> > >  for layer in data:
> > >  pass
> > > +
> > > +headers = {}
> > > +if token is not None:
> > > +headers["Authorization"] = "Token" + token
> > >  (data, res) = self._get_json(template,
> > >   registryendpoint,
> > >   "/v1/repositories" + template.path 
> > > + "/tags",
> > > - { "Authorization": "Token " + token 
> > > })
> > > + headers)
> > >  
> > >  cookie = res.info().getheader('Set-Cookie')
> > >  
> > > @@ -98,7 +102,7 @@ class DockerSource(Source):
> > >  (data, res) = self._get_json(template,
> > >   registryendpoint,
> > >   "/v1/images/" + imagetagid + 
> > > "/ancestry",
> > > - { "Authorization": "Token "+ token 
> > > })
> > > + headers)
> > >  
> > >  if data[0] != imagetagid:
> > >  raise ValueError(["Expected first layer id '%s' to match 
> > > image id '%s'",
> > > @@ -121,7 +125,7 @@ class DockerSource(Source):
> > >  res = self._save_data(template,
> > >registryendpoint,
> > >"/v1/images/" + layerid + 
> > > "/json",
> > > -  { "Authorization": "Token " + 
> > > token },
> > > +  headers,
> > >jsonfile)
> > >  createdFiles.append(jsonfile)
> > >  
> > > @@ -134,7 +138,7 @@ class DockerSource(Source):
> > >  self._save_data(template,
> > >  registryendpoint,
> > >  "/v1/images/" + layerid + "/layer",
> > > -{ "Authorization": "Token "+token },
> > > +headers,
> > >  datafile, datacsum, layersize)
> > >  createdFiles.append(datafile)
> > >  
> > 
> > ACK
> 
> Hum, after some real life testing, I'm getting an error from the default
> docker registry with this patch:
> 
> Fetching
> https://registry-1.docker.io/v1/repositories/opensuse/tags...FAIL HTTP
> Error 401: UNAUTHORIZED

Hmm, that's peculiar, I'll investigate, but I don't see what I screwed
up so far

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

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

Re: [libvirt] [PATCH v2 sandbox] virt-sandbox-image: switch to use URI to identify templates

[Daniel P. Berrange]

On Tue, Sep 22, 2015 at 10:40:02AM +0200, Cedric Bosdonnat wrote:
> On Mon, 2015-09-21 at 15:45 +0100, Daniel P. Berrange wrote:
> > -def _get_url(self,server, path, headers):
> > -url = "https://; + server + path
> > +def _get_url(self, template, server, path, headers):
> > +if template.protocol is None:
> > +protocol = "https"
> > +else:
> > +protocol = template.protocol
> > +
> > +if server is None:
> > +if template.hostname is None:
> > +server = "index.docker.io"
> > +else:
> > +if template.port is not None:
> > +server = template.hostname + ":" + template.port
> 
> This doesn't fly, port is an int, we need to have it in this form:
>   server = "%s:%d" % (template.hostname, template.port)

Good catch, I missed that

> > +def __repr__(self):
> > +if self.protocol is not None:
> > +scheme = self.source + "+" + self.protocol
> > +else:
> > +scheme = self.source
> > +if self.hostname:
> > +if self.port:
> > +netloc = self.hostname + ":" + self.port
> 
> This doesn't work, python requires to put it this way:
>  netloc = "%s:%d" % (self.hostname, self.port)

Yep, wil do


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

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

[libvirt] [PATCH] conf: escape string for disk driver name attribute

[Luyao Huang]

Just like e92e5ba1, this attribute was missed.

Signed-off-by: Luyao Huang 
---
 src/conf/domain_conf.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index a3b3ccb..85c2d0d 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -18867,8 +18867,7 @@ virDomainDiskDefFormat(virBufferPtr buf,
 def->ioeventfd || def->event_idx || def->copy_on_read ||
 def->discard || def->iothread) {
 virBufferAddLit(buf, "src->driverName)
-virBufferAsprintf(buf, " name='%s'", def->src->driverName);
+virBufferEscapeString(buf, " name='%s'", def->src->driverName);
 if (def->src->format > 0)
 virBufferAsprintf(buf, " type='%s'",
   
virStorageFileFormatTypeToString(def->src->format));
-- 
1.8.3.1

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

Re: [libvirt] [libvirt-python][PATCH] generator: fix build fail with old xml lib

[lhuang]

On 09/21/2015 06:09 PM, Michal Privoznik wrote:

On 02.09.2015 07:58, Luyao Huang wrote:

https://bugzilla.redhat.com/show_bug.cgi?id=1222795#c6

if build libvirt-python with some old xml lib (python-pyxml),
build will fail and error like this:

   File "generator.py", line 139, in start
 if "string" in attrs:
   File "/usr/local/lib/python2.7/site-packages/_xmlplus/sax/xmlreader.py" \
 , line 316, in __getitem__
 return self._attrs[name]
 KeyError: 0

This is an old issue and have been mentioned in commit 3ae0a76d.
There is no __contains__ in class AttributesImpl, python will use
__getitem__ in this place, so we will get error.
Let's use 'YYY in XXX.keys()' to avoid this issue.

Signed-off-by: Luyao Huang 
---
  generator.py | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/generator.py b/generator.py
index 2fc838c..d9ae17e 100755
--- a/generator.py
+++ b/generator.py
@@ -136,7 +136,7 @@ class docParser(xml.sax.handler.ContentHandler):
  elif attrs['file'] == "libvirt-qemu":
  qemu_enum(attrs['type'],attrs['name'],attrs['value'])
  elif tag == "macro":
-if "string" in attrs:
+if "string" in attrs.keys():
  params.append((attrs['name'], attrs['string']))
  
  def end(self, tag):



ACKed and pushed.


Thanks your review, Michal :)


Michal


Luyao

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

Re: [libvirt] [PATCH v2] virsh: Fix job status indicator for 0 length block jobs

[Erik Skultety]

On 21/09/15 19:46, Peter Krempa wrote:
> Although 0 length block jobs aren't entirely useful, the output of virsh
> blockjob is empty due to the condition that suppresses the output for
> migration jobs that did not start. Since the only place that actually
> uses the condition that suppresses the output is in migration, let's
> move the check there and thus add support for 0 of 0 equaling to 100%.
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1196711
> ---
>  tools/virsh-domain.c | 6 +-
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> index 73c476d..fb138d5 100644
> --- a/tools/virsh-domain.c
> +++ b/tools/virsh-domain.c
> @@ -1700,10 +1700,6 @@ virshPrintJobProgress(const char *label, unsigned long 
> long remaining,
>  {
>  int progress;
> 
> -if (total == 0)
> -/* migration has not been started */
> -return;
> -
>  if (remaining == 0) {
>  /* migration has completed */
>  progress = 100;
> @@ -4401,7 +4397,7 @@ virshWatchJob(vshControl *ctl,
>  ret = virDomainGetJobInfo(dom, );
>  pthread_sigmask(SIG_SETMASK, , NULL);
>  if (ret == 0) {
> -if (verbose)
> +if (verbose && jobinfo.dataTotal > 0)
>  virshPrintJobProgress(label, jobinfo.dataRemaining,
>jobinfo.dataTotal);
> 
Seems reasonable to me, ACK.

Erik

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

Re: [libvirt] [PATCH v2 sandbox] virt-sandbox-image: switch to use URI to identify templates

[Cedric Bosdonnat]

On Mon, 2015-09-21 at 15:45 +0100, Daniel P. Berrange wrote:
> Currently the CLI syntax is somewhat docker specific requiring
> inclusion of --registry arg to identify the docker download
> server. Other app containers have a notion of download server,
> but don't separate it from the template name.
> 
> This patch removes that docker-ism by changing to use a URI
> for identifying the template image. So instead of
> 
>   virt-sandbox-image download \
>   --source docker --registry index.docker.io
>   --username dan --password 123456 ubuntu:15.04
> 
> You can use
> 
>   virt-sandbox-image download 
> docker://dan:123...@index.docker.io/ubuntu?tag=15.04
> 
> The only mandatory part is the source prefix and image name, so
> that can shorten to just
> 
>   virt-sandbox-image download docker:///ubuntu
> 
> to pull down the latest ubuntu image, from the default registry
> using no authentication.
> ---
> 
> Changed in v2:
> 
>  - Rebase against master, instead of (unpushed) docker volume patch
> 
>  libvirt-sandbox/image/cli.py  |  71 +
>  libvirt-sandbox/image/sources/DockerSource.py | 142 
> ++
>  libvirt-sandbox/image/sources/Source.py   |  29 +++---
>  libvirt-sandbox/image/template.py | 110 
>  4 files changed, 228 insertions(+), 124 deletions(-)
>  create mode 100644 libvirt-sandbox/image/template.py
> 
> diff --git a/libvirt-sandbox/image/cli.py b/libvirt-sandbox/image/cli.py
> index 1718cc5..4d02289 100755
> --- a/libvirt-sandbox/image/cli.py
> +++ b/libvirt-sandbox/image/cli.py
> @@ -3,7 +3,7 @@
>  # Authors: Daniel P. Berrange 
>  #  Eren Yagdiran 
>  #
> -# Copyright (C) 2013 Red Hat, Inc.
> +# Copyright (C) 2013-2015 Red Hat, Inc.
>  # Copyright (C) 2015 Universitat Politècnica de Catalunya.
>  #
>  # This program is free software; you can redistribute it and/or modify
> @@ -34,6 +34,8 @@ import subprocess
>  import random
>  import string
>  
> +from libvirt_sandbox.image import template
> +
>  if os.geteuid() == 0:
>  default_template_dir = "/var/lib/libvirt/templates"
>  default_image_dir = "/var/lib/libvirt/images"
> @@ -44,15 +46,6 @@ else:
>  debug = False
>  verbose = False
>  
> -import importlib
> -def dynamic_source_loader(name):
> -name = name[0].upper() + name[1:]
> -modname = "libvirt_sandbox.image.sources." + name + "Source"
> -mod = importlib.import_module(modname)
> -classname = name + "Source"
> -classimpl = getattr(mod, classname)
> -return classimpl()
> -
>  gettext.bindtextdomain("libvirt-sandbox", "/usr/share/locale")
>  gettext.textdomain("libvirt-sandbox")
>  try:
> @@ -73,11 +66,10 @@ def info(msg):
>  
>  def download(args):
>  try:
> -
> dynamic_source_loader(args.source).download_template(templatename=args.template,
> - 
> templatedir=args.template_dir,
> - 
> registry=args.registry,
> - 
> username=args.username,
> - 
> password=args.password)
> +tmpl = template.Template.from_uri(args.template)
> +source = tmpl.get_source_impl()
> +source.download_template(template=tmpl,
> + templatedir=args.template_dir)
>  except IOError,e:
>  print "Source %s cannot be found in given path" %args.source
>  except Exception,e:
> @@ -85,17 +77,21 @@ def download(args):
>  
>  def delete(args):
>  try:
> -
> dynamic_source_loader(args.source).delete_template(templatename=args.template,
> -   
> templatedir=args.template_dir)
> +tmpl = template.Template.from_uri(args.template)
> +source = tmpl.get_source_impl()
> +source.delete_template(template=tmpl,
> +   templatedir=args.template_dir)
>  except Exception,e:
>  print "Delete Error %s", str(e)
>  
>  def create(args):
>  try:
> -
> dynamic_source_loader(args.source).create_template(templatename=args.template,
> -   
> templatedir=args.template_dir,
> -   
> connect=args.connect,
> -   
> format=args.format)
> +tmpl = template.Template.from_uri(args.template)
> +source = tmpl.get_source_impl()
> +source.create_template(template=tmpl,
> +   templatedir=args.template_dir,
> +   connect=args.connect,
> +   format=args.format)
>  except Exception,e:
>  print "Create Error %s" % str(e)
>  
> @@ -103,19 +99,22 @@ def run(args):

[libvirt] virtlockd - manual lock management. Is there a way?

[Piotr Rybicki]

Hello.

Is there a way to manually manage (acquire/release/info) locks in 
virtlockd for running qemu via libvirt?


I haven't found any virsh command for that.

Possibly via virtlockd's unix socket? What to send through socat?

I'm trying to workarund bug
(live external disk-only snapshot deadlocks with virtlockd):
https://bugzilla.redhat.com/show_bug.cgi?id=1191901

Best regards
--
Piotr Rybicki, Prezes Zarządu
InnerVision Sp. z o.o.
http://www.innervision.pl

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

Re: [libvirt] [PATCH 2/3] virutil: Introduce virEnumFromString

[Daniel P. Berrange]

On Thu, Sep 17, 2015 at 05:31:28PM +0200, Michal Privoznik wrote:
> So, now we have VIR_ENUM_DECL() and VIR_ENUM_IMPL() macros. They
> will define a pair of functions for you to convert from and to
> certain enum. Unfortunately, their usage is slightly cumbersome:
> 
>   int tmp;
>   virMyAwesome ma;
> 
>   if ((tmp = virMyAwesomeTypeFromString(str)) < 0) {
>   virReportError();
>   goto cleanup;
>   }
> 
>   ma = tmp;
> 
> Ideally, we could avoid using the dummy @tmp variable:
> 
>   virMyAwesome ma;
> 
>   if (virMyAwesomeEnumFromString(str, ,
>   "Unable to convert '%s'", str) < 0)
>   goto cleanup;
> 
> So, the first @str is string to convert from. @ma should point to
> the variable, where result is stored. Then, the third argument is
> the error message to be printed if conversion fails, followed by
> all the necessary arguments (message is in printf format).

I'm not seeing a hugely compelling reason to pass in the error
message string every time we parse an enum from a string. This
is no better than current code, where every caller has a potentially
different error message reported for the same scenario. I'd like
to see

   virMyAwesome ma;
   if (virMyAwesomeEnumFromString(str, ) < 0)
   goto cleanup;

We could introduce an explicit VIR_ERR_INVALID_ENUM_VALUE
error code, and a fixed error message to go with it.

> -# define VIR_ENUM_DECL(name) \
> -const char *name ## TypeToString(int type); \
> -int name ## TypeFromString(const char*type);
> +# define VIR_ENUM_DECL(name)\
> +const char *name ## TypeToString(int type); \
> +int name ## TypeFromString(const char*type);\
> +int name ## EnumFromString(const char *str, int *result,\
> +   const char *fmt, ...)\
> +ATTRIBUTE_FMT_PRINTF(3, 4);

It'd be nice to avoid generating two different FromString methods
at the same time too. I understand you probably did that to avoid
needing to convert all callers at once. How about we have a 
VIR_ENUM_DECL_ERR macro generate the new format only, and just
convert all callers at a time, for any single enum.


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

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

Re: [libvirt] [RFC PATCH 7/8] qemu: Prepare basic APIs to handle invalid defs

[Daniel P. Berrange]

On Tue, Sep 22, 2015 at 03:26:26PM +0200, Martin Kletzander wrote:
> On Tue, Sep 22, 2015 at 02:17:14PM +0100, Daniel P. Berrange wrote:
> >On Tue, Sep 22, 2015 at 03:12:14PM +0200, Martin Kletzander wrote:
> >>On Tue, Sep 22, 2015 at 01:48:01PM +0100, Daniel P. Berrange wrote:
> >>>On Tue, Sep 22, 2015 at 02:15:53PM +0200, Martin Kletzander wrote:
> In order for the user to be able to fix broken domains function
> qemuDomainGetXMLDesc() needs to be able to lookup invalid domain
> definitions and handle them properly.  When redefined, function
> qemuDomainDefineXMLFlags() must clear the 'invalid XML' reason.  As a
> nice addition, qemuDomainGetState() can lookup such domains without any
> other change and that allows virsh not only to get their status, but
> also to list them.
> >>>
> >>>Hmm, that's an interesting approach to the problem. I wonder though
> >>>if we could do things slightly differently such that we don't need
> >>>to change so many APIs.
> >>>
> >>>eg, just have a 'bool error' field in virDomainDefPtr. When loading
> >>>the XML fails, populate a virDomainObjPtr/DefPtr as normal, but set
> >>>the error field. Now we merely need to change the qemuDomainStart
> >>>method, so it refuses to launch a VM with the 'error' flag set. All
> >>>the other APIs could be essentially unchanged. Sure it would not
> >>>be useful to allow things like virDomainAttachDevice, etc on such
> >>>broken domains, but for sake of simplicity we can avoid touching
> >>>all the methods except start.
> >>>
> >>
> >>To be honest, I'm a afraid that we might forget some API that needs to
> >>be blocked as well.  And we would have to go through all the APIs just
> >>to see whether it accesses something that might be missing.  Moreover,
> >>how would you decide what to skip at each error during parsing?  If,
> >>for example, the  has some faulty attribute in one
> >>subelement should we skip all the elements or just the one or just
> >>skip that one particular attribute?  We would also not format the
> >>faulty attribute to the XML being dumped, so the user wouldn't see
> >>what's missing, which is even worse when there are two problems and
> >>they fix only one, so we skip the other one.
> >
> >Oh, I wasn't suggesting changing the parser. I just mean that we would
> >create a virDomainDefPtr instance which only contains the name and
> >UUID, and ID field (if the guest is running from domain status XML).
> >
> >We'd leave all the rest of the config blank - our code ought to be
> >able to deal with that, since essentially all config is optional
> >aside from the name/uuid anyway.
> >
> 
> Then probably I misunderstood because that's exactly what this series
> is doing.

Right, but I mean without converting all the drivers to use this new
qemuDomObjFromDomainInvalid() method. I'm suggesting just having a
check in the start method only.

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

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

Re: [libvirt] [PATCH 06/13] conf: Add XML parser flag that will allow us to do incompatible updates

[Michal Privoznik]

On 21.09.2015 19:21, Peter Krempa wrote:
> Add a new parser flag that will mark code paths that parse XML files
> wich will not be used with existing VM state so that post parse
> callbacks can possibly do ABI incompatible changes if needed.
> ---
>  src/conf/domain_conf.h|  2 ++
>  src/qemu/qemu_driver.c| 12 
>  src/qemu/qemu_migration.c |  3 ++-
>  3 files changed, 12 insertions(+), 5 deletions(-)
> 

ACK

Michal

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

Re: [libvirt] [libvirt-users] New software based on libvirt

[Cole Robinson]

On 09/22/2015 05:12 AM, Michal Privoznik wrote:
> On 21.09.2015 21:28, Gustav Fransson Nyvell wrote:
>> Hello,
>>
>> I'm introducing to you the decentralized cloud Cherrypop.
>>
>> Combining libvirt and LizardFS (as of now) it becomes a cloud completely
>> without masters. Thus, any node is sufficient for the cloud to be up
>> and therefore no wasted resources and no single point of failure.
>>
>> It's still pretty crude software but will work with some tinkering. Hope
>> you try it and like it!
>>
>> For more information, source and binary:
>> https://github.com/gustavfranssonnyvell/cherrypop
>>
>> //Gustav
>>
> 
> Hey,
> 
> what an awesome news! We keep list of software based on libvirt:
> 
> http://libvirt.org/apps.html
> 
> Mind posting a patch against docs/apps.html.in? Or I can do that too, if
> you don't want to bother.
> 

At this point we should probably just make apps.html a wiki page...

- Cole

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

Re: [libvirt] [PATCH 10/13] conf: Don't always recalculate initial memory size from NUMA size totals

[Peter Krempa]

On Tue, Sep 22, 2015 at 14:29:02 +0200, Michal Privoznik wrote:
> On 21.09.2015 19:21, Peter Krempa wrote:
> > When implementing memory hotplug I've opted to recalculate the initial
> > memory size (contents of the  element) as a sum of the sizes of
> > NUMA nodes when NUMA was enabled. This was based on an assumption that
> > qemu did not allow starting when the NUMA node size total didn't equal
> > to the initial memory size. Unfortunately the check was introduced to
> > qemu just lately.
> > 
> > This patch uses the new XML parser flag to decide whether it's safe to
> > update the memory size total from the NUMA cell sizes or not.
> > 
> > As an additional improvement we now report an error in case when the
> > size of hotplug memory would exceed the total memory size.
> > 
> > The rest of the changes assures that the function is called with correct
> > flags.
> > ---
> >  src/conf/domain_conf.c  | 37 ++---
> >  src/conf/domain_conf.h  |  1 +
> >  src/qemu/qemu_command.c |  3 ++-
> >  3 files changed, 33 insertions(+), 8 deletions(-)
> > 
> > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> > index d2a13ca..64cfd8c 100644
> > --- a/src/conf/domain_conf.c
> > +++ b/src/conf/domain_conf.c
> > @@ -3726,15 +3726,36 @@ virDomainDefRemoveDuplicateMetadata(virDomainDefPtr 
> > def)
> > 
> > 
> >  static int
> > -virDomainDefPostParseMemory(virDomainDefPtr def)
> > +virDomainDefPostParseMemory(virDomainDefPtr def,
> > +unsigned int parseFlags)
> >  {
> >  size_t i;
> > +unsigned long long numaMemory = 0;
> > +unsigned long long hotplugMemory = 0;
> > 
> > -if ((def->mem.initial_memory = virDomainNumaGetMemorySize(def->numa)) 
> > == 0) {
> > +/* Attempt to infer the initial memory size from the sum NUMA memory 
> > sizes
> > + * in case ABI updates are allowed or the  element wasn't 
> > specified */
> > +if (def->mem.total_memory == 0 ||
> > +parseFlags & VIR_DOMAIN_DEF_PARSE_ABI_UPDATE)
> > +numaMemory = virDomainNumaGetMemorySize(def->numa);
> > +
> > +if (numaMemory) {
> > +def->mem.initial_memory = numaMemory;
> 
> Is there a reason to not use the setter function you've introduced in
> previous patch?
> 

Not really, it even saves a few lines of code.

Peter


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

Re: [libvirt] [PATCH 2/3] virutil: Introduce virEnumFromString

[John Ferlan]

On 09/17/2015 11:31 AM, Michal Privoznik wrote:
> So, now we have VIR_ENUM_DECL() and VIR_ENUM_IMPL() macros. They
> will define a pair of functions for you to convert from and to
> certain enum. Unfortunately, their usage is slightly cumbersome:
> 
>   int tmp;
>   virMyAwesome ma;
> 
>   if ((tmp = virMyAwesomeTypeFromString(str)) < 0) {
>   virReportError();
>   goto cleanup;
>   }
> 
>   ma = tmp;
> 
> Ideally, we could avoid using the dummy @tmp variable:
> 
>   virMyAwesome ma;
> 
>   if (virMyAwesomeEnumFromString(str, ,
>   "Unable to convert '%s'", str) < 0)
>   goto cleanup;
> 
> So, the first @str is string to convert from. @ma should point to
> the variable, where result is stored. Then, the third argument is
> the error message to be printed if conversion fails, followed by
> all the necessary arguments (message is in printf format).
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/libvirt_private.syms |  1 +
>  src/util/virutil.c   | 31 +++
>  src/util/virutil.h   | 32 +---
>  3 files changed, 61 insertions(+), 3 deletions(-)
> 

Your example shows a comparison of "< 0"; however, your patch 3 examples
change a "<= 0" to a "< 0", e.g. (from patch 3):

-if ((val = virTristateSwitchTypeFromString(ioeventfd)) <= 0) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-   _("unknown disk ioeventfd mode '%s'"),
-   ioeventfd);
+if (virTristateSwitchEnumFromString(ioeventfd, >ioeventfd,
+_("unknown disk ioeventfd
mode '%s'"),
+ioeventfd) < 0)

where essentially that's converted to:

+
+if ((type = (convertFunc)(str)) < 0) {
+if (fmt) {


For the tristate in particular, "0" is absent which is used as a marker
of sorts. It's not an error.

Similarly, for "many" a 0 would convert to some VIR_*_NONE and would be
deemed 'illegal'. Of course the "virtType" example in patch 3 shows a
case where a VIRT_*_NONE is not present (there's another recent patch
stream that wanted to change that...)

I think with a tweak to account for a min required value would be
necessary and perhaps for extra credit a non-zero max value.

As an aside, it looks strange to have an argument repeated. I would
think a majority of error messages are going to use the first argument
of the function to the first argument of the error message anyway.


John
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index ee7c229..730f2f8 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -2343,6 +2343,7 @@ virUSBDeviceSetUsedBy;
>  
>  # util/virutil.h
>  virDoubleToStr;
> +virEnumFromString;
>  virFindFCHostCapableVport;
>  virFindSCSIHostByPCI;
>  virFormatIntDecimal;
> diff --git a/src/util/virutil.c b/src/util/virutil.c
> index 3343e0d..6bb9e35 100644
> --- a/src/util/virutil.c
> +++ b/src/util/virutil.c
> @@ -444,6 +444,37 @@ const char *virTypeToString(const char *const*types,
>  return types[type];
>  }
>  
> +int
> +virEnumFromString(const char *str,
> +  int *result,
> +  virEnumConvertFunc convertFunc,
> +  const char *enumName,
> +  const char *fmt,
> +  va_list ap)
> +{
> +int ret = -1;
> +int type;
> +char *errMsg = NULL;
> +
> +if ((type = (convertFunc)(str)) < 0) {
> +if (fmt) {
> +if (virVasprintf(, fmt, ap) >= 0)
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", errMsg);
> +} else {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +   _("Unable to convert '%s' to %s type"),
> +   str, enumName);
> +}
> +goto cleanup;
> +}
> +
> +*result = type;
> +ret = 0;
> + cleanup:
> +VIR_FREE(errMsg);
> +return ret;
> +}
> +
>  /* In case thread-safe locales are available */
>  #if HAVE_NEWLOCALE
>  
> diff --git a/src/util/virutil.h b/src/util/virutil.h
> index 648de28..5dd2790 100644
> --- a/src/util/virutil.h
> +++ b/src/util/virutil.h
> @@ -27,6 +27,7 @@
>  
>  # include "internal.h"
>  # include 
> +# include 
>  # include 
>  
>  # ifndef MIN
> @@ -78,6 +79,16 @@ const char *virTypeToString(const char *const*types,
>  unsigned int ntypes,
>  int type);
>  
> +typedef int (*virEnumConvertFunc) (const char *str);
> +
> +int virEnumFromString(const char *str,
> +  int *result,
> +  virEnumConvertFunc convertFunc,
> +  const char *enumName,
> +  const char *fmt,
> +  va_list ap)
> +ATTRIBUTE_FMT_PRINTF(5, 0);
> +
>  # define VIR_ENUM_IMPL(name, lastVal, ...)   \
>  static const char *const name ## 

Re: [libvirt] [PATCH 0/3] Introduce new virtType enum item

[Daniel P. Berrange]

On Tue, Sep 22, 2015 at 07:50:41AM -0400, John Ferlan wrote:
> 
> 
> On 09/17/2015 04:46 AM, Shivangi Dhir wrote:
> > These patches solve a BiteSizedTask - Introduce new virtType enum item[0]
> > 
> > [0] 
> > http://wiki.libvirt.org/page/BiteSizedTasks#Introduce_new_virtType_enum_item
> > 
> > Shivangi Dhir (3):
> >   conf: Add new VIR_DOMAIN_VIRT_NONE enum
> >   qemu: Make virtType of type virDomainVirtType
> >   conf: Change the description of virCapabilitiesDomainDataLookup()
> > 
> >  src/conf/capabilities.c  |  6 +++---
> >  src/conf/domain_conf.c   |  3 ++-
> >  src/conf/domain_conf.h   |  3 ++-
> >  src/qemu/qemu_monitor.c  |  2 +-
> >  src/qemu/qemu_monitor.h  |  2 +-
> >  src/qemu/qemu_monitor_json.c |  2 +-
> >  src/qemu/qemu_monitor_json.h |  2 +-
> >  src/qemu/qemu_monitor_text.c |  2 +-
> >  src/qemu/qemu_monitor_text.h |  2 +-
> >  tests/qemumonitorjsontest.c  |  2 +-
> >  tests/vircapstest.c  | 32 
> >  11 files changed, 30 insertions(+), 28 deletions(-)
> > 
> 
> While yes this is a bite sized task, I do have a concern with changing
> the meaning of undefined from -1 to 0. Doing so will change (increase by
> 1) each of the VIR_DOMAIN_VIRT_* values, e.g. VIR_DOMAIN_VIRT_QEMU
> changes from 0 to 1. If you look through the history of changes to
> virDomainVirtType you'll note it's been appended to and never had
> something inserted in the middle.
> 
> Inserting in the middle is not a problem if we could "guarantee" that
> nothing exists (saved) or is currently running that references an
> existing numerical value. Unfortunately, I'm not sure we can do that.
> 
> As seen in patch 2, there is a 'virtType' value in the domain
> definition. A running domain VIR_DOMAIN_VIRT_QEMU would have been
> started with "0" in that field. When a new libvirtd with these changes
> is in place, the running domain would still have "0" stored away and
> that would mean something different.
> 
> Furthermore (or likewise), a migration from an "older" host to a newer
> host would have a different virtType value and I would think cause
> virDomainDefCheckABIStability to complain. Similarly, a 'saved' domain
> would have a numerical anomaly - although for that case it's less clear
> whether we save the physical name or the numerical value.
> 
> Of course I could be off-base/wrong, but let's see what others say...

In general we should never be serializing the raw integer enum
values only the string representation. If we did have somewhere
that used an integer representation, we'd certainly have to avoid
this change as you mention. I think we're probably safe though
unless someone knows of a place we don't use the string rep.

> 
> FWIW: For a "first" patch set - it seems you've followed the guidelines
> quite well. About the only suggestions I have are in patch1 the
> comparison "if (domaintype)" in virCapabilitiesDomainDataLookupInternal
> could be "if (domaintype > VIR_DOMAIN_VIRT_NONE)" as well as patch3
> should be merged with patch1 and the "int domaintype" in the args list
> for virCapabilitiesDomainDataLookup (and capabilities.h) should be
> "virDomainVirtType domaintype".

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

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

Re: [libvirt] [RFC PATCH 7/8] qemu: Prepare basic APIs to handle invalid defs

[Martin Kletzander]

On Tue, Sep 22, 2015 at 02:17:14PM +0100, Daniel P. Berrange wrote:

On Tue, Sep 22, 2015 at 03:12:14PM +0200, Martin Kletzander wrote:

On Tue, Sep 22, 2015 at 01:48:01PM +0100, Daniel P. Berrange wrote:
>On Tue, Sep 22, 2015 at 02:15:53PM +0200, Martin Kletzander wrote:
>>In order for the user to be able to fix broken domains function
>>qemuDomainGetXMLDesc() needs to be able to lookup invalid domain
>>definitions and handle them properly.  When redefined, function
>>qemuDomainDefineXMLFlags() must clear the 'invalid XML' reason.  As a
>>nice addition, qemuDomainGetState() can lookup such domains without any
>>other change and that allows virsh not only to get their status, but
>>also to list them.
>
>Hmm, that's an interesting approach to the problem. I wonder though
>if we could do things slightly differently such that we don't need
>to change so many APIs.
>
>eg, just have a 'bool error' field in virDomainDefPtr. When loading
>the XML fails, populate a virDomainObjPtr/DefPtr as normal, but set
>the error field. Now we merely need to change the qemuDomainStart
>method, so it refuses to launch a VM with the 'error' flag set. All
>the other APIs could be essentially unchanged. Sure it would not
>be useful to allow things like virDomainAttachDevice, etc on such
>broken domains, but for sake of simplicity we can avoid touching
>all the methods except start.
>

To be honest, I'm a afraid that we might forget some API that needs to
be blocked as well.  And we would have to go through all the APIs just
to see whether it accesses something that might be missing.  Moreover,
how would you decide what to skip at each error during parsing?  If,
for example, the  has some faulty attribute in one
subelement should we skip all the elements or just the one or just
skip that one particular attribute?  We would also not format the
faulty attribute to the XML being dumped, so the user wouldn't see
what's missing, which is even worse when there are two problems and
they fix only one, so we skip the other one.


Oh, I wasn't suggesting changing the parser. I just mean that we would
create a virDomainDefPtr instance which only contains the name and
UUID, and ID field (if the guest is running from domain status XML).

We'd leave all the rest of the config blank - our code ought to be
able to deal with that, since essentially all config is optional
aside from the name/uuid anyway.



Then probably I misunderstood because that's exactly what this series
is doing.


I thought about your approach as well, but that would require complete
rewrite of the whole parsing code (maybe changing it to generated one
based on RNG schema and some minor additional info) and it would touch
way more APIs.  The approach I'm suggesting here keeps almost all APIs
untouched and it doesn't have an effect on anything, unless explicitly
specified.

Check last two patches to see how much qemu driver needs to be
changed, for others it will be similar, probably a little bit less..
The patches are 17+/9- when combined.  I don't think you can do much
better than that.  And that doesn't show the improvements that can be
made in the starting and parsing code after this series is applied.


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


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

Re: [libvirt] virtlockd - manual lock management. Is there a way?

[Daniel P. Berrange]

On Tue, Sep 22, 2015 at 03:26:14PM +0200, Piotr Rybicki wrote:
> Hello.
> 
> Is there a way to manually manage (acquire/release/info) locks in virtlockd
> for running qemu via libvirt?
> 
> I haven't found any virsh command for that.
> 
> Possibly via virtlockd's unix socket? What to send through socat?

No, there's no supportable way to talk to virtlockd directly. It is
considered an internal only service at the current time.


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

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

Re: [libvirt] [PATCH sandbox] docker: don't assume X-Docker-Token is set

[Daniel P. Berrange]

On Tue, Sep 22, 2015 at 09:10:48AM +0200, Cedric Bosdonnat wrote:
> On Mon, 2015-09-21 at 22:12 +0200, Cedric Bosdonnat wrote:
> > On Mon, 2015-09-21 at 15:12 +0100, Daniel P. Berrange wrote:
> > > The Red Hat docker registry (registry.access.redhat.com) does
> > > not set any X-Docker-Token HTTP header in its responses. Change
> > > the code so it only passes around this header if it is actually
> > > present.
> > > 
> > > Signed-off-by: Daniel P. Berrange 
> > > ---
> > >  libvirt-sandbox/image/sources/DockerSource.py | 12 
> > >  1 file changed, 8 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/libvirt-sandbox/image/sources/DockerSource.py 
> > > b/libvirt-sandbox/image/sources/DockerSource.py
> > > index f367c8f..78b2a53 100644
> > > --- a/libvirt-sandbox/image/sources/DockerSource.py
> > > +++ b/libvirt-sandbox/image/sources/DockerSource.py
> > > @@ -83,10 +83,14 @@ class DockerSource(Source):
> > >  checksums = {}
> > >  for layer in data:
> > >  pass
> > > +
> > > +headers = {}
> > > +if token is not None:
> > > +headers["Authorization"] = "Token" + token

I accidentally lost the trailing space after the word "Token"

> > >  (data, res) = self._get_json(template,
> > >   registryendpoint,
> > >   "/v1/repositories" + template.path 
> > > + "/tags",
> > > - { "Authorization": "Token " + token 
> > > })
> > > + headers)

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

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

Re: [libvirt] [PATCH 0/3] Introduce new virtType enum item

[John Ferlan]

On 09/17/2015 04:46 AM, Shivangi Dhir wrote:
> These patches solve a BiteSizedTask - Introduce new virtType enum item[0]
> 
> [0] 
> http://wiki.libvirt.org/page/BiteSizedTasks#Introduce_new_virtType_enum_item
> 
> Shivangi Dhir (3):
>   conf: Add new VIR_DOMAIN_VIRT_NONE enum
>   qemu: Make virtType of type virDomainVirtType
>   conf: Change the description of virCapabilitiesDomainDataLookup()
> 
>  src/conf/capabilities.c  |  6 +++---
>  src/conf/domain_conf.c   |  3 ++-
>  src/conf/domain_conf.h   |  3 ++-
>  src/qemu/qemu_monitor.c  |  2 +-
>  src/qemu/qemu_monitor.h  |  2 +-
>  src/qemu/qemu_monitor_json.c |  2 +-
>  src/qemu/qemu_monitor_json.h |  2 +-
>  src/qemu/qemu_monitor_text.c |  2 +-
>  src/qemu/qemu_monitor_text.h |  2 +-
>  tests/qemumonitorjsontest.c  |  2 +-
>  tests/vircapstest.c  | 32 
>  11 files changed, 30 insertions(+), 28 deletions(-)
> 

While yes this is a bite sized task, I do have a concern with changing
the meaning of undefined from -1 to 0. Doing so will change (increase by
1) each of the VIR_DOMAIN_VIRT_* values, e.g. VIR_DOMAIN_VIRT_QEMU
changes from 0 to 1. If you look through the history of changes to
virDomainVirtType you'll note it's been appended to and never had
something inserted in the middle.

Inserting in the middle is not a problem if we could "guarantee" that
nothing exists (saved) or is currently running that references an
existing numerical value. Unfortunately, I'm not sure we can do that.

As seen in patch 2, there is a 'virtType' value in the domain
definition. A running domain VIR_DOMAIN_VIRT_QEMU would have been
started with "0" in that field. When a new libvirtd with these changes
is in place, the running domain would still have "0" stored away and
that would mean something different.

Furthermore (or likewise), a migration from an "older" host to a newer
host would have a different virtType value and I would think cause
virDomainDefCheckABIStability to complain. Similarly, a 'saved' domain
would have a numerical anomaly - although for that case it's less clear
whether we save the physical name or the numerical value.

Of course I could be off-base/wrong, but let's see what others say...

FWIW: For a "first" patch set - it seems you've followed the guidelines
quite well. About the only suggestions I have are in patch1 the
comparison "if (domaintype)" in virCapabilitiesDomainDataLookupInternal
could be "if (domaintype > VIR_DOMAIN_VIRT_NONE)" as well as patch3
should be merged with patch1 and the "int domaintype" in the args list
for virCapabilitiesDomainDataLookup (and capabilities.h) should be
"virDomainVirtType domaintype".

John

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

[libvirt] [PATCH 0/2] vz: remove unused network and storage drivers

[Maxim Nestratov]

From: Maxim Nestratov 

Maxim Nestratov (2):
  vz: remove network driver as never used
  vz: remove storage driver as never used

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

Re: [libvirt] [PATCH v2 sandbox] virt-sandbox-image: switch to use URI to identify templates

[Daniel P. Berrange]

On Tue, Sep 22, 2015 at 10:19:03AM +0100, Daniel P. Berrange wrote:
> On Mon, Sep 21, 2015 at 10:11:48PM +0200, Cedric Bosdonnat wrote:
> > On Mon, 2015-09-21 at 15:45 +0100, Daniel P. Berrange wrote:
> > > Currently the CLI syntax is somewhat docker specific requiring
> > > inclusion of --registry arg to identify the docker download
> > > server. Other app containers have a notion of download server,
> > > but don't separate it from the template name.
> > > 
> > > This patch removes that docker-ism by changing to use a URI
> > > for identifying the template image. So instead of
> > > 
> > >   virt-sandbox-image download \
> > >   --source docker --registry index.docker.io
> > >   --username dan --password 123456 ubuntu:15.04
> > > 
> > > You can use
> > > 
> > >   virt-sandbox-image download 
> > > docker://dan:123...@index.docker.io/ubuntu?tag=15.04
> > > 
> > > The only mandatory part is the source prefix and image name, so
> > > that can shorten to just
> > > 
> > >   virt-sandbox-image download docker:///ubuntu
> > > 
> > > to pull down the latest ubuntu image, from the default registry
> > > using no authentication.
> > > ---
> > > 
> > > Changed in v2:
> > > 
> > >  - Rebase against master, instead of (unpushed) docker volume patch
> > > 
> > >  libvirt-sandbox/image/cli.py  |  71 +
> > >  libvirt-sandbox/image/sources/DockerSource.py | 142 
> > > ++
> > >  libvirt-sandbox/image/sources/Source.py   |  29 +++---
> > >  libvirt-sandbox/image/template.py | 110 
> > 
> > Missing change in libvirt-sandbox/image/Makefile.am to add template.py.
> > As is that file isn't installed.
> > 
> > I'm also just realizing that we didn't add Eren't commit for the
> > virt-sandbox-image man page. Adding it later is fine, but we need to
> > keep that on our radar.
> 
> Yep, that's in my tree to update & pyush.
> 
> > > @@ -151,7 +150,7 @@ def run(args):
> > >  
> > >  def requires_template(parser):
> > >  parser.add_argument("template",
> > > -help=_("name of the template"))
> > > +help=_("URI of the template"))
> > 
> > Shouldn't we provide some examples here? As those URIs can't be invented
> > we need to give the user some chances to discover them without having to
> > read our code ;)
> 
> I wasn't sure this was the best place. We'll certainly put examples in
> the man pages though.

I found out how to add an epilog to the help output, so we can get text
that looks like this:

$ virt-sandbox-image download --help
usage: virt-sandbox-image download [-h] [-t TEMPLATE_DIR] template

positional arguments:
  template  URI of the template

optional arguments:
  -h, --helpshow this help message and exit
  -t TEMPLATE_DIR, --template-dir TEMPLATE_DIR
Template directory for saving templates

Example supported URI formats:

  docker:///ubuntu?tag=15.04
  docker://username:passw...@index.docker.io/private/image
  docker://registry.access.redhat.com/rhel6


> > ACK, with the help improvement + Makefile.am fix.

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

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

[libvirt] [PATCH 2/2] vz: remove storage driver as never used

[Maxim Nestratov]

From: Maxim Nestratov 

In fact, it was never used as far as vz has no features supporting it.
That is why there will be no harm to anyone if we just remove this code to
prevent further misunderstanding and efforts to support dead code.

Signed-off-by: Maxim Nestratov 
---
 src/Makefile.am |1 -
 src/vz/vz_driver.c  |6 +-
 src/vz/vz_storage.c | 1654 ---
 src/vz/vz_utils.h   |4 -
 4 files changed, 1 insertions(+), 1664 deletions(-)
 delete mode 100644 src/vz/vz_storage.c

diff --git a/src/Makefile.am b/src/Makefile.am
index 9b42022..b9089be 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -838,7 +838,6 @@ VZ_DRIVER_SOURCES = \
vz/vz_driver.c  \
vz/vz_utils.c   \
vz/vz_utils.h   \
-   vz/vz_storage.c \
vz/vz_sdk.h \
vz/vz_sdk.c
 
diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c
index 1a4523c..2fd1104 100644
--- a/src/vz/vz_driver.c
+++ b/src/vz/vz_driver.c
@@ -308,8 +308,7 @@ vzConnectOpen(virConnectPtr conn,
 return VIR_DRV_OPEN_ERROR;
 }
 
-if ((ret = vzOpenDefault(conn)) != VIR_DRV_OPEN_SUCCESS ||
-(ret = vzStorageOpen(conn, flags)) != VIR_DRV_OPEN_SUCCESS) {
+if ((ret = vzOpenDefault(conn)) != VIR_DRV_OPEN_SUCCESS) {
 vzConnectClose(conn);
 return ret;
 }
@@ -325,8 +324,6 @@ vzConnectClose(virConnectPtr conn)
 if (!privconn)
 return 0;
 
-vzStorageClose(conn);
-
 vzDriverLock(privconn);
 prlsdkUnsubscribeFromPCSEvents(privconn);
 virObjectUnref(privconn->caps);
@@ -1398,7 +1395,6 @@ static virHypervisorDriver vzDriver = {
 
 static virConnectDriver vzConnectDriver = {
 .hypervisorDriver = ,
-.storageDriver = ,
 };
 
 /* Parallels domain type backward compatibility*/
diff --git a/src/vz/vz_storage.c b/src/vz/vz_storage.c
deleted file mode 100644
index 1e82195..000
--- a/src/vz/vz_storage.c
+++ /dev/null
@@ -1,1654 +0,0 @@
-/*
- * vz_storage.c: core driver functions for managing
- * Parallels Cloud Server hosts
- *
- * Copyright (C) 2013-2014 Red Hat, Inc.
- * Copyright (C) 2012 Parallels, Inc.
- *
- * This library is free software; you can redistribute it and/or
- * modify it under the terms of the GNU Lesser General Public
- * License as published by the Free Software Foundation; either
- * version 2.1 of the License, or (at your option) any later version.
- *
- * This library is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
- * Lesser General Public License for more details.
- *
- * You should have received a copy of the GNU Lesser General Public
- * License along with this library.  If not, see
- * .
- *
- */
-
-#include 
-
-#include 
-#include 
-#include 
-#include 
-#include 
-#include 
-
-#include "datatypes.h"
-#include "dirname.h"
-#include "viralloc.h"
-#include "configmake.h"
-#include "virstoragefile.h"
-#include "virerror.h"
-#include "virfile.h"
-#include "vz_utils.h"
-#include "virstring.h"
-
-#define VIR_FROM_THIS VIR_FROM_PARALLELS
-
-#define vzPoolNotFoundError(pool_name)\
-virReportError(VIR_ERR_INVALID_ARG,  \
-   _("pool '%s' not found"), pool_name);
-
-static virStorageVolDefPtr
-vzStorageVolDefineXML(virStoragePoolObjPtr pool, const char *xmldesc,
-  const char *xmlfile, bool is_new);
-static virStorageVolPtr
-vzStorageVolLookupByPath(virConnectPtr conn, const char *path);
-
-static int
-vzStoragePoolGetAlloc(virStoragePoolDefPtr def);
-
-static void
-vzStorageLock(virStorageDriverStatePtr driver)
-{
-virMutexLock(>lock);
-}
-
-static void
-vzStorageUnlock(virStorageDriverStatePtr driver)
-{
-virMutexUnlock(>lock);
-}
-
-int
-vzStorageClose(virConnectPtr conn)
-{
-vzConnPtr privconn = conn->privateData;
-
-if (!privconn)
-return 0;
-
-virStorageDriverStatePtr storageState = privconn->storageState;
-privconn->storageState = NULL;
-
-if (!storageState)
-return 0;
-
-vzStorageLock(storageState);
-virStoragePoolObjListFree(>pools);
-VIR_FREE(storageState->configDir);
-VIR_FREE(storageState->autostartDir);
-vzStorageUnlock(storageState);
-virMutexDestroy(>lock);
-VIR_FREE(storageState);
-
-return 0;
-}
-
-static int
-vzFindVolumes(virStoragePoolObjPtr pool)
-{
-DIR *dir;
-struct dirent *ent;
-char *path = NULL;
-int ret = -1;
-int direrr;
-
-if (!(dir = opendir(pool->def->target.path))) {
-virReportSystemError(errno,
- _("cannot open path '%s'"),
- pool->def->target.path);
- 

[libvirt] [PATCH 1/2] vz: remove network driver as never used

[Maxim Nestratov]

From: Maxim Nestratov 

At the time this code was added we had intentions to support libvirt interface
to manage vz networks. In fact, it was never implemented completely to work
correctly that makes me think that there will be no harm to anyone if we just
rip it off. Moreover, in vz7 we started to use libvirt bridge network driver to
manage networks.

Signed-off-by: Maxim Nestratov 
---
 src/Makefile.am |1 -
 src/vz/vz_driver.c  |5 +-
 src/vz/vz_network.c |  556 ---
 src/vz/vz_utils.h   |4 -
 4 files changed, 1 insertions(+), 565 deletions(-)
 delete mode 100644 src/vz/vz_network.c

diff --git a/src/Makefile.am b/src/Makefile.am
index 060abe8..9b42022 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -839,7 +839,6 @@ VZ_DRIVER_SOURCES = \
vz/vz_utils.c   \
vz/vz_utils.h   \
vz/vz_storage.c \
-   vz/vz_network.c \
vz/vz_sdk.h \
vz/vz_sdk.c
 
diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c
index 8fa7957..1a4523c 100644
--- a/src/vz/vz_driver.c
+++ b/src/vz/vz_driver.c
@@ -309,8 +309,7 @@ vzConnectOpen(virConnectPtr conn,
 }
 
 if ((ret = vzOpenDefault(conn)) != VIR_DRV_OPEN_SUCCESS ||
-(ret = vzStorageOpen(conn, flags)) != VIR_DRV_OPEN_SUCCESS ||
-(ret = vzNetworkOpen(conn, flags)) != VIR_DRV_OPEN_SUCCESS) {
+(ret = vzStorageOpen(conn, flags)) != VIR_DRV_OPEN_SUCCESS) {
 vzConnectClose(conn);
 return ret;
 }
@@ -326,7 +325,6 @@ vzConnectClose(virConnectPtr conn)
 if (!privconn)
 return 0;
 
-vzNetworkClose(conn);
 vzStorageClose(conn);
 
 vzDriverLock(privconn);
@@ -1401,7 +1399,6 @@ static virHypervisorDriver vzDriver = {
 static virConnectDriver vzConnectDriver = {
 .hypervisorDriver = ,
 .storageDriver = ,
-.networkDriver = ,
 };
 
 /* Parallels domain type backward compatibility*/
diff --git a/src/vz/vz_network.c b/src/vz/vz_network.c
deleted file mode 100644
index 8c97812..000
--- a/src/vz/vz_network.c
+++ /dev/null
@@ -1,556 +0,0 @@
-/*
- * vz_network.c: core privconn functions for managing
- * Parallels Cloud Server hosts
- *
- * Copyright (C) 2013-2014 Red Hat, Inc.
- * Copyright (C) 2012 Parallels, Inc.
- *
- * This library is free software; you can redistribute it and/or
- * modify it under the terms of the GNU Lesser General Public
- * License as published by the Free Software Foundation; either
- * version 2.1 of the License, or (at your option) any later version.
- *
- * This library is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
- * Lesser General Public License for more details.
- *
- * You should have received a copy of the GNU Lesser General Public
- * License along with this library.  If not, see
- * .
- *
- */
-
-#include 
-
-#include "datatypes.h"
-#include "dirname.h"
-#include "viralloc.h"
-#include "virerror.h"
-#include "virfile.h"
-#include "virnetdev.h"
-#include "md5.h"
-#include "vz_utils.h"
-#include "virstring.h"
-
-#define VIR_FROM_THIS VIR_FROM_PARALLELS
-#define PARALLELS_ROUTED_NETWORK_UUID   "eb593dd1-6846-45b0-84a0-de0729286982"
-
-#define vzParseError() 
 \
-virReportErrorHelper(VIR_FROM_TEST, VIR_ERR_OPERATION_FAILED, __FILE__,
 \
- __FUNCTION__, __LINE__, _("Can't parse prlctl 
output"))
-
-static int vzGetBridgedNetInfo(virNetworkDefPtr def, virJSONValuePtr jobj)
-{
-const char *ifname;
-char *bridgeLink = NULL;
-char *bridgePath = NULL;
-char *bridgeAddressPath = NULL;
-char *bridgeAddress = NULL;
-int len = 0;
-int ret = -1;
-
-if (!(ifname = virJSONValueObjectGetString(jobj, "Bound To"))) {
-vzParseError();
-goto cleanup;
-}
-
-if (virAsprintf(, SYSFS_NET_DIR "%s/brport/bridge", ifname) < 0)
-goto cleanup;
-
-if (virFileResolveLink(bridgeLink, ) < 0) {
-virReportSystemError(errno, _("cannot read link '%s'"), bridgeLink);
-goto cleanup;
-}
-
-if (VIR_STRDUP(def->bridge, last_component(bridgePath)) < 0)
-goto cleanup;
-
-if (virAsprintf(, SYSFS_NET_DIR 
"%s/brport/bridge/address",
-ifname) < 0)
-goto cleanup;
-
-if ((len = virFileReadAll(bridgeAddressPath, 18, )) < 0) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
-   _("Error reading file '%s'"), bridgeAddressPath);
-
-goto cleanup;
-}
-
-if (len < VIR_MAC_STRING_BUFLEN) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
-   _("Error reading MAC from '%s'"), 

Re: [libvirt] [PATCH 3/3] virsh: Notify users about disconnects

[Jiri Denemark]

On Mon, Sep 21, 2015 at 17:32:41 -0400, John Ferlan wrote:
> 
> 
> On 09/17/2015 08:23 AM, Jiri Denemark wrote:
> > After my "client rpc: Report proper error for keepalive disconnections"
> > patch, virsh would no long print a warning when it closes a connection
> > to a daemon after a keepalive timeout. Although the warning
> > 
> > virsh # 2015-09-15 10:59:26.729+: 642080: info :
> > libvirt version: 1.2.19
> > 2015-09-15 10:59:26.729+: 642080: warning :
> > virKeepAliveTimerInternal:143 : No response from client
> > 0x7efdc0a46730 after 1 keepalive messages in 2 seconds
> > 
> > was pretty ugly, it was still useful. This patch brings the useful part
> > back while making it much nicer:
> > 
> > virsh # error: Disconnected from qemu:///system due to keepalive timeout
> > 
> > Signed-off-by: Jiri Denemark 
> > ---
> >  tools/virsh.c | 35 ---
> >  1 file changed, 32 insertions(+), 3 deletions(-)
> > 
> 
> Patch series seems reasonable to me, except for one minor thing
> Found by my coverity checker...  Naturally
> 
> > diff --git a/tools/virsh.c b/tools/virsh.c
> > index bb12dec..23436ea 100644
> > --- a/tools/virsh.c
> > +++ b/tools/virsh.c
> > @@ -95,12 +95,41 @@ static int disconnected; /* we may have been 
> > disconnected */
> >   * handler, just save the fact it was raised.
> >   */
> >  static void
> > -virshCatchDisconnect(virConnectPtr conn ATTRIBUTE_UNUSED,
> > +virshCatchDisconnect(virConnectPtr conn,
> >   int reason,
> > - void *opaque ATTRIBUTE_UNUSED)
> > + void *opaque)
> >  {
> > -if (reason != VIR_CONNECT_CLOSE_REASON_CLIENT)
> > +if (reason != VIR_CONNECT_CLOSE_REASON_CLIENT) {
> 
> [1]
> 
> > +vshControl *ctl = opaque;
> > +const char *str = "unknown reason";
> > +virErrorPtr error;
> > +char *uri;
> > +
> > +error = virSaveLastError();
> > +uri = virConnectGetURI(conn);
> > +
> > +switch ((virConnectCloseReason) reason) {
> > +case VIR_CONNECT_CLOSE_REASON_ERROR:
> > +str = N_("Disconnected from %s due to I/O error");
> > +break;
> > +case VIR_CONNECT_CLOSE_REASON_EOF:
> > +str = N_("Disconnected from %s due to end of file");
> > +break;
> > +case VIR_CONNECT_CLOSE_REASON_KEEPALIVE:
> > +str = N_("Disconnected from %s due to keepalive timeout");
> > +break;
> > +case VIR_CONNECT_CLOSE_REASON_CLIENT:
> 
> [1]^^^ Coverity complains about DEADCODE
> 
> Other than setting str = NULL and testing for it later, I'm not exactly
> sure of the 'best' course of action.  e.g., if (str) { vshError();
> disconnected++; }
> 
> I think if this is handled, then the series is ACK-able.

I think /* coverity[dead_error_begin] */ is the right solution in this
case.

Jirka

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

Re: [libvirt] [PATCH v2] Close the source fd if the destination qemu exits during tunnelled migration

[Shivaprasad bhat]

On Mon, Sep 21, 2015 at 8:04 PM, John Ferlan  wrote:
>
>
> On 09/21/2015 05:09 AM, Shivaprasad bhat wrote:
>> Thanks John for the comments.
>>
>>
>> On Fri, Sep 18, 2015 at 10:34 PM, John Ferlan  wrote:
>>>
>>>
>>> On 09/14/2015 10:44 AM, Shivaprasad G Bhat wrote:
 Tunnelled migration can hang if the destination qemu exits despite all the
 ABI checks. This happens whenever the destination qemu exits before the
 complete transfer is noticed by source qemu. The savevm state checks at
 runtime can fail at destination and cause qemu to error out.
 The source qemu cant notice it as the EPIPE is not propogated to it.
 The qemuMigrationIOFunc() notices the stream being broken from 
 virStreamSend()
 and it cleans up the stream alone. The qemuMigrationWaitForCompletion() 
 would
 never get to 100% transfer completion.
 The qemuMigrationWaitForCompletion() never breaks out as well since
 the ssh connection to destination is healthy, and the source qemu also 
 thinks
 the migration is ongoing as the Fd to which it transfers, is never
 closed or broken. So, the migration will hang forever. Even Ctrl-C on the
 virsh migrate wouldn't be honoured. Close the source side FD when there is
 an error in the stream. That way, the source qemu updates itself and
 qemuMigrationWaitForCompletion() notices the failure.

 Close the FD for all kinds of errors to be sure. The error message is not
 copied for EPIPE so that the destination error is copied instead later.

 Note:
 Reproducible with repeated migrations between Power hosts running in 
 different
 subcores-per-core modes.

 Changes from v1 -> v2:
VIR_FORCE_CLOSE() was called twice for this use case which would log
unneccessary warnings. So, move the fd close to qemuMigrationIOFunc
so that there are no unnecessary duplicate attempts.(Would this trigger
a Coverity error? I don't have a setup to check.)

 Signed-off-by: Shivaprasad G Bhat 
 ---
  src/qemu/qemu_migration.c |8 ++--
  1 file changed, 6 insertions(+), 2 deletions(-)

 diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
 index ff89ab5..9602fb2 100644
 --- a/src/qemu/qemu_migration.c
 +++ b/src/qemu/qemu_migration.c
 @@ -4012,6 +4012,7 @@ static void qemuMigrationIOFunc(void *arg)
  if (virStreamFinish(data->st) < 0)
  goto error;

 +VIR_FORCE_CLOSE(data->sock);
  VIR_FREE(buffer);

  return;
 @@ -4029,7 +4030,11 @@ static void qemuMigrationIOFunc(void *arg)
  }

   error:
 -virCopyLastError(>err);
 +/* Let the source qemu know that the transfer cant continue anymore.
 + * Don't copy the error for EPIPE as destination has the actual 
 error. */
 +VIR_FORCE_CLOSE(data->sock);
 +if (!virLastErrorIsSystemErrno(EPIPE))
 +virCopyLastError(>err);
  virResetLastError();
  VIR_FREE(buffer);
  }
 @@ -4397,7 +4402,6 @@ qemuMigrationRun(virQEMUDriverPtr driver,
  if (iothread && qemuMigrationStopTunnel(iothread, ret < 0) < 0)
  ret = -1;
  }
 -VIR_FORCE_CLOSE(fd);
>>>
>>> ^^^
>>>
>>> This causes Coverity to claim a RESOURCE_LEAK
>>>
>>> Feels like this was a mistake edit...
>>>
>>
>> The VIR_FORCE_CLOSE() inside qemuMigrationIOFunc() alone is sufficient.
>> Having this again here would lead to Warning in the logs. I too thought 
>> coverity
>> would complain. Is there a way to force ignore a coverity warning?
>>
>
> Typically a marker of sorts, such as
>
> /* coverity[leaked_handle]  */
>
> Although I'm not sure that's the best way to handle this either.
>
> The problem I see though is this is an "error path" issue and when
> perhaps it's safe/required to close fd/io->sock/data->sock.
>
> Your commit comment notes that the issue is seen on a fairly specific
> event (virStreamSend failure) for a specific type of migration.

I believe the failure can be seen for all types of migration with savestate
mismatch.

> As I
> read the code, that failure jumps to error (as does virStreamFinish). So
> now you have a fairly specific set of instances which perhaps would
> cause qemuMigrationWaitForCompletion to need to fail. The fix you have
> proposed is to close the data->sock (io->sock, fd). However, your
> proposal is a larger hammer. I assume closure of data->sock causes
> WaitForCompletion to fail (perhaps differently) and that's why you chose it.
>
> Going back to the beginning of qemuMigrationRun, setting the 'fd' and
> using StartTunnel seems to rely on MIGRATION_FWD_DIRECT, but if a tunnel
> is created an (ugh) 'iothread' variable is NON-NULL. What if 'iothread'
> were passed to qemuMigrationWaitForCompletion? Then again passed to
> qemuMigrationCompleted which 

Re: [libvirt] [PATCH v2 sandbox] virt-sandbox-image: switch to use URI to identify templates

[Cedric Bosdonnat]

On Tue, 2015-09-22 at 11:26 +0100, Daniel P. Berrange wrote:
> On Tue, Sep 22, 2015 at 10:19:03AM +0100, Daniel P. Berrange wrote:
> > On Mon, Sep 21, 2015 at 10:11:48PM +0200, Cedric Bosdonnat wrote:
> > > On Mon, 2015-09-21 at 15:45 +0100, Daniel P. Berrange wrote:
> > > > Currently the CLI syntax is somewhat docker specific requiring
> > > > inclusion of --registry arg to identify the docker download
> > > > server. Other app containers have a notion of download server,
> > > > but don't separate it from the template name.
> > > > 
> > > > This patch removes that docker-ism by changing to use a URI
> > > > for identifying the template image. So instead of
> > > > 
> > > >   virt-sandbox-image download \
> > > >   --source docker --registry index.docker.io
> > > >   --username dan --password 123456 ubuntu:15.04
> > > > 
> > > > You can use
> > > > 
> > > >   virt-sandbox-image download 
> > > > docker://dan:123...@index.docker.io/ubuntu?tag=15.04
> > > > 
> > > > The only mandatory part is the source prefix and image name, so
> > > > that can shorten to just
> > > > 
> > > >   virt-sandbox-image download docker:///ubuntu
> > > > 
> > > > to pull down the latest ubuntu image, from the default registry
> > > > using no authentication.
> > > > ---
> > > > 
> > > > Changed in v2:
> > > > 
> > > >  - Rebase against master, instead of (unpushed) docker volume patch
> > > > 
> > > >  libvirt-sandbox/image/cli.py  |  71 +
> > > >  libvirt-sandbox/image/sources/DockerSource.py | 142 
> > > > ++
> > > >  libvirt-sandbox/image/sources/Source.py   |  29 +++---
> > > >  libvirt-sandbox/image/template.py | 110 
> > > > 
> > > 
> > > Missing change in libvirt-sandbox/image/Makefile.am to add template.py.
> > > As is that file isn't installed.
> > > 
> > > I'm also just realizing that we didn't add Eren't commit for the
> > > virt-sandbox-image man page. Adding it later is fine, but we need to
> > > keep that on our radar.
> > 
> > Yep, that's in my tree to update & pyush.
> > 
> > > > @@ -151,7 +150,7 @@ def run(args):
> > > >  
> > > >  def requires_template(parser):
> > > >  parser.add_argument("template",
> > > > -help=_("name of the template"))
> > > > +help=_("URI of the template"))
> > > 
> > > Shouldn't we provide some examples here? As those URIs can't be invented
> > > we need to give the user some chances to discover them without having to
> > > read our code ;)
> > 
> > I wasn't sure this was the best place. We'll certainly put examples in
> > the man pages though.
> 
> I found out how to add an epilog to the help output, so we can get text
> that looks like this:
> 
> $ virt-sandbox-image download --help
> usage: virt-sandbox-image download [-h] [-t TEMPLATE_DIR] template
> 
> positional arguments:
>   template  URI of the template
> 
> optional arguments:
>   -h, --helpshow this help message and exit
>   -t TEMPLATE_DIR, --template-dir TEMPLATE_DIR
> Template directory for saving templates
> 
> Example supported URI formats:
> 
>   docker:///ubuntu?tag=15.04
>   docker://username:passw...@index.docker.io/private/image
>   docker://registry.access.redhat.com/rhel6
> 

Sounds nice.

--
Cedric

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

Re: [libvirt] virtlockd - manual lock management. Is there a way?

[Piotr Rybicki]

W dniu 2015-09-22 o 15:34, Daniel P. Berrange pisze:

Possibly via virtlockd's unix socket? What to send through socat?


No, there's no supportable way to talk to virtlockd directly. It is
considered an internal only service at the current time.


I see. Thanks for information.

I'll try libvirtd hooks then, to implement own locking mechanism.

Best regards
Piotr Rybicki

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

Re: [libvirt] [PATCH 0/2] Couple of fixes after mockup patches

[Ján Tomko]

On Tue, Sep 22, 2015 at 04:39:03PM +0200, Michal Privoznik wrote:
> I've noticed a few tests failing strangely after [1] was merged. Here are the
> fixes.
> 
> 1: https://www.redhat.com/archives/libvir-list/2015-September/msg00460.html
> 
> Michal Privoznik (2):
>   qemuTestDriverInit: init the driver lock too
>   tests: Avoid use of virQEMUDriverCreateXMLConf(NULL)
> 
>  tests/qemucapabilitiestest.c |  9 +
>  tests/qemumonitorjsontest.c  | 19 ++-
>  tests/qemumonitortest.c  |  9 +
>  tests/securityselinuxlabeltest.c |  8 +---
>  tests/testutilsqemu.c|  6 +-
>  5 files changed, 30 insertions(+), 21 deletions(-)
> 

ACK

Jan


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

[libvirt] [PATCH] add compress stream support

[Vasiliy Tolstov]

use libarchive for compressed stream support

Signed-off-by: Vasiliy Tolstov 
---
 configure.ac | 11 ++--
 include/libvirt/libvirt-stream.h |  6 +
 m4/virt-archive.m4   | 26 +++
 src/fdstream.c   | 47 +
 src/libvirt-stream.c | 56 +++-
 src/libvirt_public.syms  |  7 +
 6 files changed, 150 insertions(+), 3 deletions(-)
 create mode 100644 m4/virt-archive.m4

diff --git a/configure.ac b/configure.ac
index 03463b0..4018b49 100644
--- a/configure.ac
+++ b/configure.ac
@@ -250,6 +250,7 @@ LIBVIRT_CHECK_SANLOCK
 LIBVIRT_CHECK_SASL
 LIBVIRT_CHECK_SELINUX
 LIBVIRT_CHECK_SSH2
+LIBVIRT_CHECK_LIBARCHIVE
 LIBVIRT_CHECK_SYSTEMD_DAEMON
 LIBVIRT_CHECK_UDEV
 LIBVIRT_CHECK_WIRESHARK
@@ -1603,8 +1604,6 @@ fi
 AC_SUBST([LIBPCAP_CFLAGS])
 AC_SUBST([LIBPCAP_LIBS])
 
-
-
 dnl
 dnl Checks for the UML driver
 dnl
@@ -2097,6 +2096,13 @@ AM_CONDITIONAL([WITH_STORAGE_DISK], [test 
"$with_storage_disk" = "yes"])
 AC_SUBST([LIBPARTED_CFLAGS])
 AC_SUBST([LIBPARTED_LIBS])
 
+if test "$with_libarchive" = "yes" || test "$with_libarchive" = "check"; then
+   PKG_CHECK_MODULES([LIBARCHIVE], [libarchive >= 3.1.2], [], 
[LIBARCHIVE_FOUND=no])
+fi
+AC_SUBST([LIBARCHIVE_CFLAGS])
+AC_SUBST([LIBARCHIVE_LIBS])
+
+
 if test "$with_storage_mpath" = "yes" ||
test "$with_storage_disk" = "yes"; then
DEVMAPPER_CFLAGS=
@@ -2892,6 +2898,7 @@ LIBVIRT_RESULT_SANLOCK
 LIBVIRT_RESULT_SASL
 LIBVIRT_RESULT_SELINUX
 LIBVIRT_RESULT_SSH2
+LIBVIRT_RESULT_LIBARCHIVE
 LIBVIRT_RESULT_SYSTEMD_DAEMON
 LIBVIRT_RESULT_UDEV
 LIBVIRT_RESULT_WIRESHARK
diff --git a/include/libvirt/libvirt-stream.h b/include/libvirt/libvirt-stream.h
index 831640d..ac48fba 100644
--- a/include/libvirt/libvirt-stream.h
+++ b/include/libvirt/libvirt-stream.h
@@ -35,6 +35,12 @@ typedef enum {
 
 virStreamPtr virStreamNew(virConnectPtr conn,
   unsigned int flags);
+virStreamPtr virStreamNewLz4(virConnectPtr conn,
+  unsigned int flags);
+virStreamPtr virStreamNewGzip(virConnectPtr conn,
+  unsigned int flags);
+virStreamPtr virStreamNewXz(virConnectPtr conn,
+  unsigned int flags);
 int virStreamRef(virStreamPtr st);
 
 int virStreamSend(virStreamPtr st,
diff --git a/m4/virt-archive.m4 b/m4/virt-archive.m4
new file mode 100644
index 000..b550c41
--- /dev/null
+++ b/m4/virt-archive.m4
@@ -0,0 +1,26 @@
+dnl The libarchive.so library
+dnl
+dnl Copyright (C) 2012-2013 Red Hat, Inc.
+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_CHECK_LIBARCHIVE],[
+  LIBVIRT_CHECK_PKG([LIBARCHIVE], [libarchive], [3.1.2])
+])
+
+AC_DEFUN([LIBVIRT_RESULT_LIBARCHIVE],[
+  LIBVIRT_RESULT_LIB([LIBARCHIVE])
+])
diff --git a/src/fdstream.c b/src/fdstream.c
index b8ea86e..82f0e8c 100644
--- a/src/fdstream.c
+++ b/src/fdstream.c
@@ -33,6 +33,10 @@
 #include 
 #include 
 
+#ifdef WITH_LIBARCHIVE
+# include 
+#endif
+
 #include "fdstream.h"
 #include "virerror.h"
 #include "datatypes.h"
@@ -75,6 +79,9 @@ struct virFDStreamData {
 void *icbOpaque;
 
 virMutex lock;
+#ifdef WITH_LIBARCHIVE
+void *archive;
+#endif
 };
 
 
@@ -319,6 +326,12 @@ virFDStreamCloseInt(virStreamPtr st, bool streamAbort)
 if (VIR_CLOSE(fdst->errfd) < 0)
 VIR_DEBUG("ignoring failed close on fd %d", fdst->errfd);
 
+#ifdef WITH_LIBARCHIVE
+if (fdst->archive != NULL) {
+archive_write_free(fdst->archive);
+archive_read_free(fdst->archive);
+}
+#endif
 st->privateData = NULL;
 
 /* call the internal stream closing callback */
@@ -385,7 +398,16 @@ static int virFDStreamWrite(virStreamPtr st, const char 
*bytes, size_t nbytes)
 }
 
  retry:
+#ifdef WITH_LIBARCHIVE
+if (fdst->archive != NULL) {
+archive_write_open_fd(fdst->archive, fdst->fd);
+ret = archive_write_data(fdst->archive, bytes, nbytes);
+} else {
+ret = write(fdst->fd, bytes, nbytes);
+}
+#else
 ret = write(fdst->fd, bytes, nbytes);
+#endif
 if (ret < 0) {
 if (errno == EAGAIN || errno == EWOULDBLOCK) {
 ret = -2;
@@ -397,7 +419,15 @@ static int virFDStreamWrite(virStreamPtr st, const char 
*bytes, size_t nbytes)
   

Re: [libvirt] [PATCH] add compress stream support

[Vasiliy Tolstov]

2015-09-22 17:10 GMT+03:00 Vasiliy Tolstov :
> use libarchive for compressed stream support
>
> Signed-off-by: Vasiliy Tolstov 


this is test patch, because libvirt not build with this - struct
virFDStreamData is internally used by fdstream.c, but i need to acces
private data from libvirt-stream.c via virStreamNewLz4 and so. Can
somebody helps me and say, how the best to integrate compressed stream
support to libvirt?
I plan to use this stream when upload/download volume via libvirt.

-- 
Vasiliy Tolstov,
e-mail: v.tols...@selfip.ru

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

[libvirt] ANNOUNCE: libvirt 1.2.18.1 maintenance release

[Cole Robinson]

libvirt 1.2.18.1 is now available. This is a maintenance release of
libvirt 1.2.18 with additional bugfixes that have accumulated
upstream since the initial release.

This release can be downloaded at:

http://libvirt.org/sources/stable_updates/libvirt-1.2.18.1.tar.gz

Changes in this version:

* test driver: don't unlock pool after freeing it
* libxl: fix AttachDeviceConfig on hostdev type
* security_selinux: Take @privileged into account
* selinux: fix compile errors
* security_selinux: Add SetDirLabel support
* security: Add virSecurityDomainSetDirLabel
* security_selinux: Use proper structure to access socket data
* security_selinux: Replace SELinuxSCSICallbackData with proper struct
* virSecuritySELinuxSetSecurityAllLabel: drop useless
  virFileIsSharedFSType
* virSecurityManager: Track if running as privileged
* qemu: hotplug: Properly clean up drive backend if frontend hotplug
  fails
* xen: fix race in refresh of config cache
* libxl: don't end job for ephemeal domain on start failure
* conf: fix crash when parsing a unordered NUMA 
* qemu: Check virGetLastError return value for migration finish failure
* libxl: don't overwrite error from virNetSocketNewConnectTCP()
* domain-conf: escape string for socket attribute
* src: Check for symbols ordering in ADMIN_SYM_FILES
* src: Cleanup libvirt_admin.syms
* src: Check libvirt_admin.syms for exported symbols
* util: fallback to ioctl(SIOCBRDELBR) if netlink RTM_DELLINK fails
* util: fallback to ioctl(SIOCBRADDBR) if netlink RTM_NEWLINK fails
* libxl: acquire a job when receiving a migrating domain
* libxl: don't attempt to resume domain when suspend fails
* libxl: fix ref counting of libxlMigrationDstArgs
* libvirt_lxc: Claim success for --help
* virt-aa-helper: Improve valid_path
* qemu: Emit correct audit message for memory hot unplug
* qemu: Emit correct audit message for memory hot plug
* hostdev: skip ACS check when using VFIO for device assignment
* Start daemon only after filesystems are mounted
* virt-aa-helper: add NVRAM store file for read/write
* qemu: Update blkio.weight value after successful set
* Eliminate incorrect and unnecessary check for changed IP address
* virt-aa-helper: allow access to /usr/share/ovmf/
* virt-aa-helper: Simplify restriction logic
* virt-aa-helper: document --probing and --dry-run
* Add generated libvirt_admin.syms into .gitignore
* libvirt-admin: Generate symbols file
* daemon: Use $(NULL) for libvird_admin's flags
* qemu: Add check for invalid iothread_id in qemuDomainChgIOThread
* virsh: Reset global error after successfull domain lookup
* build: fix mingw build
* Detect location of qemu-bridge-helper
* Check if qemu-bridge-helper exists and is executable
* qemu: Use numad information when getting pin information
* qemu: Keep numad hint after daemon restart
* conf: Pass private data to Parse function of XML options
* qemu: Fix segfault when parsing private domain data
* domain: Fix crash if trying to live update disk 
* virNetSocketCheckProtocols: handle EAI_NONAME as IPv6 unavailable
* util: don't overwrite stack when getting ethtool gfeatures
* conf: Don't try formating non-existing addresses
* admin: Drop 'internal.h' include from libvirt-admin.h
* qemu: fail on attempts to use  for non-tap network
  connections
* network: validate network NAT range
* virNetDevBandwidthParseRate: Reject negative values
* network: verify proper address family in updates to  and 
* conf: more useful error message when pci function is out of range
* virDomainDefParseXML: Check for malicious cpu ids in 
* numa_conf: Introduce virDomainNumaGetMaxCPUID
* Allow vfio hotplug of a device to the domain which owns the iommu
* qemu: Forbid image pre-creation for non-shared storage migration
* virsh: fix domfsinfo output in quiet mode
* tests: extend workaround for gnutls private key loading failure
* qemu: fix some api cannot work when disable cpuset in conf
* storage: only run safezero if allocation is > 0
* qemu: command: Report stderr from qemu-bridge-helper
* qemu: Fix reporting of physical capacity for block devices
* remoteClientCloseFunc: Don't mangle connection object refcount
* storage: Correct the 'mode' check
* storage: Handle failure from refreshVol
* virfile: Introduce virFileUnlink
* Revert "LXC: show used memory as 0 when domain is not active"

For info about past maintenance releases, see:

http://wiki.libvirt.org/page/Maintenance_Releases

Thanks,
Cole

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

Re: [libvirt] [PATCH 03/13] qemu: Make memory alignment helper more universal

[Michal Privoznik]

On 21.09.2015 19:21, Peter Krempa wrote:
> Extract the size determination into a separate function and reuse it
> across the memory device alignment functions. Since later we will need
> to decide the alignment size according to architecture let's pass def to
> the functions.
> ---
>  src/qemu/qemu_domain.c  | 26 ++
>  src/qemu/qemu_domain.h  |  3 ++-
>  src/qemu/qemu_hotplug.c |  4 ++--
>  3 files changed, 22 insertions(+), 11 deletions(-)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index f840b0d..a5e9b75 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -3367,30 +3367,39 @@ qemuDomainAgentAvailable(virDomainObjPtr vm,
>  }
> 
> 
> +static unsigned long long
> +qemuDomainGetMemorySizeAlignment(virDomainDefPtr def ATTRIBUTE_UNUSED)
> +{
> +/* Align memory size. QEMU requires rounding to next 4KiB block.
> + * We'll take the "traditional" path and round it to 1MiB*/
> +
> +return 1024;
> +}
> +
> +
>  int
>  qemuDomainAlignMemorySizes(virDomainDefPtr def)
>  {
>  unsigned long long mem;
> +unsigned long long align = qemuDomainGetMemorySizeAlignment(def);

How about:

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index a5e9b75..807112c 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -3381,7 +3381,7 @@ int
 qemuDomainAlignMemorySizes(virDomainDefPtr def)
 {
 unsigned long long mem;
-unsigned long long align = qemuDomainGetMemorySizeAlignment(def);
+unsigned long long const align = qemuDomainGetMemorySizeAlignment(def);
 size_t ncells = virDomainNumaGetNodeCount(def->numa);
 size_t i;

>  size_t ncells = virDomainNumaGetNodeCount(def->numa);
>  size_t i;
> 
>  /* align NUMA cell sizes if relevant */
>  for (i = 0; i < ncells; i++) {
>  mem = virDomainNumaGetNodeMemorySize(def->numa, i);
> -virDomainNumaSetNodeMemorySize(def->numa, i, VIR_ROUND_UP(mem, 
> 1024));
> +virDomainNumaSetNodeMemorySize(def->numa, i, VIR_ROUND_UP(mem, 
> align));
>  }
> 
>  /* align initial memory size */
>  mem = virDomainDefGetMemoryInitial(def);
> -virDomainDefSetMemoryInitial(def, VIR_ROUND_UP(mem, 1024));
> +virDomainDefSetMemoryInitial(def, VIR_ROUND_UP(mem, align));
> 
> -/* Align maximum memory size. QEMU requires rounding to next 4KiB block.
> - * We'll take the "traditional" path and round it to 1MiB*/
> -def->mem.max_memory = VIR_ROUND_UP(def->mem.max_memory, 1024);
> +def->mem.max_memory = VIR_ROUND_UP(def->mem.max_memory, align);
> 
>  /* Align memory module sizes */
>  for (i = 0; i < def->nmems; i++)
> -qemuDomainMemoryDeviceAlignSize(def->mems[i]);
> +def->mems[i]->size = VIR_ROUND_UP(def->mems[i]->size, align);
> 
>  return 0;
>  }
> @@ -3405,9 +3414,10 @@ qemuDomainAlignMemorySizes(virDomainDefPtr def)
>   * size so this should be safe).
>   */
>  void
> -qemuDomainMemoryDeviceAlignSize(virDomainMemoryDefPtr mem)
> +qemuDomainMemoryDeviceAlignSize(virDomainDefPtr def,
> +virDomainMemoryDefPtr mem)
>  {
> -mem->size = VIR_ROUND_UP(mem->size, 1024);
> +mem->size = VIR_ROUND_UP(mem->size, 
> qemuDomainGetMemorySizeAlignment(def));
>  }
> 
> 
> diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
> index 8cf535f..64cd7e1 100644
> --- a/src/qemu/qemu_domain.h
> +++ b/src/qemu/qemu_domain.h
> @@ -469,7 +469,8 @@ bool qemuDomainHasBlockjob(virDomainObjPtr vm, bool 
> copy_only)
>  ATTRIBUTE_NONNULL(1);
> 
>  int qemuDomainAlignMemorySizes(virDomainDefPtr def);
> -void qemuDomainMemoryDeviceAlignSize(virDomainMemoryDefPtr mem);
> +void qemuDomainMemoryDeviceAlignSize(virDomainDefPtr def,
> + virDomainMemoryDefPtr mem);
> 
>  virDomainChrSourceDefPtr qemuFindAgentConfig(virDomainDefPtr def);
> 
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 4797836..afc5408 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -1802,7 +1802,7 @@ qemuDomainAttachMemory(virQEMUDriverPtr driver,
>  if (!(devstr = qemuBuildMemoryDeviceStr(mem, vm->def, priv->qemuCaps)))
>  goto cleanup;
> 
> -qemuDomainMemoryDeviceAlignSize(mem);
> +qemuDomainMemoryDeviceAlignSize(vm->def, mem);
> 
>  if (qemuBuildMemoryBackendStr(mem->size, mem->pagesize,
>mem->targetNode, mem->sourceNodes, NULL,
> @@ -4273,7 +4273,7 @@ qemuDomainDetachMemoryDevice(virQEMUDriverPtr driver,
>  return -1;
>  }
> 
> -qemuDomainMemoryDeviceAlignSize(memdef);
> +qemuDomainMemoryDeviceAlignSize(vm->def, memdef);
> 
>  if ((idx = virDomainMemoryFindByDef(vm->def, memdef)) < 0) {
>  virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> 

ACK

Michal

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

Re: [libvirt] [PATCH 05/13] conf: Document all VIR_DOMAIN_DEF_PARSE_* flags

[Michal Privoznik]

On 21.09.2015 19:21, Peter Krempa wrote:
> ---
>  src/conf/domain_conf.h | 10 --
>  1 file changed, 8 insertions(+), 2 deletions(-)

ACK

Michal

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

Re: [libvirt] [PATCH 02/13] conf: Add helper to determine whether memory hotplug is enabled for a vm

[Michal Privoznik]

On 21.09.2015 19:21, Peter Krempa wrote:
> Add a simple helper so that the code doesn't have to rewrite the same
> condition multiple times.
> ---
>  src/conf/domain_conf.c| 9 -
>  src/conf/domain_conf.h| 1 +
>  src/libvirt_private.syms  | 1 +
>  src/qemu/qemu_command.c   | 2 +-
>  src/qemu/qemu_domain.c| 2 +-
>  src/qemu/qemu_migration.c | 5 ++---
>  6 files changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index a3b3ccb..fa2e331 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -1154,7 +1154,7 @@ int
>  virDomainDefCheckUnsupportedMemoryHotplug(virDomainDefPtr def)
>  {
>  /* memory hotplug tunables are not supported by this driver */
> -if (def->mem.max_memory > 0 || def->mem.memory_slots > 0) {
> +if (virDomainDefHasMemoryHotplug(def)) {
>  virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> _("memory hotplug tunables  are not "
>   "supported by this hypervisor driver"));
> @@ -7671,6 +7671,13 @@ virDomainParseMemoryLimit(const char *xpath,
>  }
> 
> 
> +bool
> +virDomainDefHasMemoryHotplug(const virDomainDef *def)
> +{
> +return def->mem.memory_slots > 0 || def->mem.max_memory > 0;
> +}
> +

There are some other occurrences of this pattern too, e.g.:

virDomainDefPostParseInternal
virDomainDefFormatInternal

Probably worth 'fixing' those places too.


> +
>  /**
>   * virDomainDefGetMemoryInitial:
>   * @def: domain definition
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 8be390b..cfd2600 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -2324,6 +2324,7 @@ struct _virDomainDef {
>  unsigned long long virDomainDefGetMemoryInitial(virDomainDefPtr def);
>  void virDomainDefSetMemoryInitial(virDomainDefPtr def, unsigned long long 
> size);
>  unsigned long long virDomainDefGetMemoryActual(virDomainDefPtr def);
> +bool virDomainDefHasMemoryHotplug(const virDomainDef *def);
> 
>  typedef enum {
>  VIR_DOMAIN_KEY_WRAP_CIPHER_NAME_AES,
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 8c1f388..1a92422 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -217,6 +217,7 @@ virDomainDefGetMemoryActual;
>  virDomainDefGetMemoryInitial;
>  virDomainDefGetSecurityLabelDef;
>  virDomainDefHasDeviceAddress;
> +virDomainDefHasMemoryHotplug;
>  virDomainDefMaybeAddController;
>  virDomainDefMaybeAddInput;
>  virDomainDefNeedsPlacementAdvice;
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 25f57f2..e1f199c 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -9323,7 +9323,7 @@ qemuBuildCommandLine(virConnectPtr conn,
> 
>  virCommandAddArg(cmd, "-m");
> 
> -if (def->mem.max_memory) {
> +if (virDomainDefHasMemoryHotplug(def)) {
>  if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PC_DIMM)) {
>  virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> _("memory hotplug isn't supported by this QEMU 
> binary"));
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index e4a88cd..f840b0d 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -1367,7 +1367,7 @@ qemuDomainDeviceDefPostParse(virDomainDeviceDefPtr dev,
>  }
> 
>  if (dev->type == VIR_DOMAIN_DEVICE_MEMORY &&
> -def->mem.max_memory == 0) {
> +!virDomainDefHasMemoryHotplug(def)) {
>  virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> _("maxMemory has to be specified when using memory "
>   "devices "));
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index 903612b..948ad3b 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
> @@ -3000,10 +3000,9 @@ qemuMigrationBeginPhase(virQEMUDriverPtr driver,
>  }
>  }
> 
> -if (vm->def->mem.max_memory ||
> +if (virDomainDefHasMemoryHotplug(vm->def) ||
>  ((flags & VIR_MIGRATE_PERSIST_DEST) &&
> - vm->newDef &&
> - vm->newDef->mem.max_memory))
> + vm->newDef && virDomainDefHasMemoryHotplug(vm->newDef)))
>  cookieFlags |= QEMU_MIGRATION_COOKIE_MEMORY_HOTPLUG;
> 
>  if (!(mig = qemuMigrationEatCookie(driver, vm, NULL, 0, 0)))
> 

ACK

Michal

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

Re: [libvirt] [RFC PATCH 7/8] qemu: Prepare basic APIs to handle invalid defs

[Daniel P. Berrange]

On Tue, Sep 22, 2015 at 02:15:53PM +0200, Martin Kletzander wrote:
> In order for the user to be able to fix broken domains function
> qemuDomainGetXMLDesc() needs to be able to lookup invalid domain
> definitions and handle them properly.  When redefined, function
> qemuDomainDefineXMLFlags() must clear the 'invalid XML' reason.  As a
> nice addition, qemuDomainGetState() can lookup such domains without any
> other change and that allows virsh not only to get their status, but
> also to list them.

Hmm, that's an interesting approach to the problem. I wonder though
if we could do things slightly differently such that we don't need
to change so many APIs.

eg, just have a 'bool error' field in virDomainDefPtr. When loading
the XML fails, populate a virDomainObjPtr/DefPtr as normal, but set
the error field. Now we merely need to change the qemuDomainStart
method, so it refuses to launch a VM with the 'error' flag set. All
the other APIs could be essentially unchanged. Sure it would not
be useful to allow things like virDomainAttachDevice, etc on such
broken domains, but for sake of simplicity we can avoid touching
all the methods except start.


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

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

Re: [libvirt] [PATCH 02/13] conf: Add helper to determine whether memory hotplug is enabled for a vm

[Peter Krempa]

On Tue, Sep 22, 2015 at 14:29:26 +0200, Michal Privoznik wrote:
> On 21.09.2015 19:21, Peter Krempa wrote:
> > Add a simple helper so that the code doesn't have to rewrite the same
> > condition multiple times.
> > ---
> >  src/conf/domain_conf.c| 9 -
> >  src/conf/domain_conf.h| 1 +
> >  src/libvirt_private.syms  | 1 +
> >  src/qemu/qemu_command.c   | 2 +-
> >  src/qemu/qemu_domain.c| 2 +-
> >  src/qemu/qemu_migration.c | 5 ++---
> >  6 files changed, 14 insertions(+), 6 deletions(-)
> > 
> > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> > index a3b3ccb..fa2e331 100644
> > --- a/src/conf/domain_conf.c
> > +++ b/src/conf/domain_conf.c
> > @@ -1154,7 +1154,7 @@ int
> >  virDomainDefCheckUnsupportedMemoryHotplug(virDomainDefPtr def)
> >  {
> >  /* memory hotplug tunables are not supported by this driver */
> > -if (def->mem.max_memory > 0 || def->mem.memory_slots > 0) {
> > +if (virDomainDefHasMemoryHotplug(def)) {
> >  virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> > _("memory hotplug tunables  are not "
> >   "supported by this hypervisor driver"));
> > @@ -7671,6 +7671,13 @@ virDomainParseMemoryLimit(const char *xpath,
> >  }
> > 
> > 
> > +bool
> > +virDomainDefHasMemoryHotplug(const virDomainDef *def)
> > +{
> > +return def->mem.memory_slots > 0 || def->mem.max_memory > 0;
> > +}
> > +
> 
> There are some other occurrences of this pattern too, e.g.:
> 
> virDomainDefPostParseInternal

Well this place makes sure that both the slot count and maximum size
were specified so I think it makes sense to leave the condition to stay
explicit as it's now so that it's more clear what's happening there.

> virDomainDefFormatInternal

I'll change this one.

> 
> Probably worth 'fixing' those places too.

Peter


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

Re: [libvirt] [RFC PATCH 7/8] qemu: Prepare basic APIs to handle invalid defs

[Martin Kletzander]

On Tue, Sep 22, 2015 at 01:48:01PM +0100, Daniel P. Berrange wrote:

On Tue, Sep 22, 2015 at 02:15:53PM +0200, Martin Kletzander wrote:

In order for the user to be able to fix broken domains function
qemuDomainGetXMLDesc() needs to be able to lookup invalid domain
definitions and handle them properly.  When redefined, function
qemuDomainDefineXMLFlags() must clear the 'invalid XML' reason.  As a
nice addition, qemuDomainGetState() can lookup such domains without any
other change and that allows virsh not only to get their status, but
also to list them.


Hmm, that's an interesting approach to the problem. I wonder though
if we could do things slightly differently such that we don't need
to change so many APIs.

eg, just have a 'bool error' field in virDomainDefPtr. When loading
the XML fails, populate a virDomainObjPtr/DefPtr as normal, but set
the error field. Now we merely need to change the qemuDomainStart
method, so it refuses to launch a VM with the 'error' flag set. All
the other APIs could be essentially unchanged. Sure it would not
be useful to allow things like virDomainAttachDevice, etc on such
broken domains, but for sake of simplicity we can avoid touching
all the methods except start.



To be honest, I'm a afraid that we might forget some API that needs to
be blocked as well.  And we would have to go through all the APIs just
to see whether it accesses something that might be missing.  Moreover,
how would you decide what to skip at each error during parsing?  If,
for example, the  has some faulty attribute in one
subelement should we skip all the elements or just the one or just
skip that one particular attribute?  We would also not format the
faulty attribute to the XML being dumped, so the user wouldn't see
what's missing, which is even worse when there are two problems and
they fix only one, so we skip the other one.

I thought about your approach as well, but that would require complete
rewrite of the whole parsing code (maybe changing it to generated one
based on RNG schema and some minor additional info) and it would touch
way more APIs.  The approach I'm suggesting here keeps almost all APIs
untouched and it doesn't have an effect on anything, unless explicitly
specified.

Check last two patches to see how much qemu driver needs to be
changed, for others it will be similar, probably a little bit less..
The patches are 17+/9- when combined.  I don't think you can do much
better than that.  And that doesn't show the improvements that can be
made in the starting and parsing code after this series is applied.



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


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

[libvirt] [RFC PATCH 5/8] conf: Optionally keep domains with invalid XML

[Martin Kletzander]

Add new parameter to virDomainObjListLoadConfig() and
virDomainObjListLoadAllConfigs() that controls whether domains with
invalid XML (which could not be parsed) should be kept in order not to
lose track of them.  For now, the parameter is set to false in all
callers.  Each driver can switch it to true when it is prepared to deal
with such domains.

For the domain object to be created add virDomainDefParseMinimal() that
parses only name and UUID from the XML definition.  UUID must be
present, it will not be generated.  The purpose of this function is to
be used when all else fails, but we still want a domain object to work
with.

Signed-off-by: Martin Kletzander 
---
 src/bhyve/bhyve_driver.c |  2 +
 src/conf/domain_conf.c   | 95 ++--
 src/conf/domain_conf.h   |  5 +++
 src/libxl/libxl_driver.c |  3 ++
 src/lxc/lxc_driver.c |  3 ++
 src/qemu/qemu_driver.c   |  3 ++
 src/uml/uml_driver.c |  2 +
 7 files changed, 109 insertions(+), 4 deletions(-)

diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c
index 7f365b1f2446..abf6789c2f9c 100644
--- a/src/bhyve/bhyve_driver.c
+++ b/src/bhyve/bhyve_driver.c
@@ -1219,6 +1219,7 @@ bhyveStateInitialize(bool privileged,
NULL, 1,
bhyve_driver->caps,
bhyve_driver->xmlopt,
+   false,
NULL, NULL) < 0)
 goto cleanup;

@@ -1227,6 +1228,7 @@ bhyveStateInitialize(bool privileged,
BHYVE_AUTOSTART_DIR, 0,
bhyve_driver->caps,
bhyve_driver->xmlopt,
+   false,
NULL, NULL) < 0)
 goto cleanup;

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 230800542f8c..6850e79396bb 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -22676,6 +22676,39 @@ virDomainSaveStatus(virDomainXMLOptionPtr xmlopt,
 return ret;
 }

+static virDomainDefPtr
+virDomainDefParseMinimal(const char *filename,
+ const char *xmlStr)
+{
+xmlXPathContextPtr ctxt = NULL;
+virDomainDefPtr def = NULL;
+xmlDocPtr xml = NULL;
+
+if (!(xml = virXMLParse(filename, xmlStr, _("(domain_definition)"
+goto cleanup;
+
+ctxt = xmlXPathNewContext(xml);
+if (ctxt == NULL) {
+virReportOOMError();
+goto cleanup;
+}
+
+ctxt->node = xmlDocGetRootElement(xml);
+
+if (!(def = virDomainDefNew()))
+goto cleanup;
+
+def->id = -1;
+
+if (virDomainDefParseName(def, ctxt) < 0 ||
+virDomainDefParseUUID(def, ctxt, true, NULL) < 0)
+virDomainDefFree(def);
+
+ cleanup:
+xmlFreeDoc(xml);
+xmlXPathFreeContext(ctxt);
+return def;
+}

 static virDomainObjPtr
 virDomainObjListLoadConfig(virDomainObjListPtr doms,
@@ -22684,6 +22717,7 @@ virDomainObjListLoadConfig(virDomainObjListPtr doms,
const char *configDir,
const char *autostartDir,
const char *name,
+   bool keep_invalid,
virDomainLoadConfigNotify notify,
void *opaque)
 {
@@ -22692,13 +22726,57 @@ virDomainObjListLoadConfig(virDomainObjListPtr doms,
 virDomainObjPtr dom;
 int autostart;
 virDomainDefPtr oldDef = NULL;
+char *xmlStr = NULL;
+char *xmlErr = NULL;

 if ((configFile = virDomainConfigFile(configDir, name)) == NULL)
 goto error;
-if (!(def = virDomainDefParseFile(configFile, caps, xmlopt,
-  VIR_DOMAIN_DEF_PARSE_INACTIVE |
-  
VIR_DOMAIN_DEF_PARSE_SKIP_OSTYPE_CHECKS)))
-goto error;
+
+def = virDomainDefParseFile(configFile, caps, xmlopt,
+VIR_DOMAIN_DEF_PARSE_INACTIVE |
+VIR_DOMAIN_DEF_PARSE_SKIP_OSTYPE_CHECKS);
+if (!def) {
+char *tmp = NULL;
+
+if (!keep_invalid)
+goto error;
+
+if (VIR_STRDUP(xmlErr, virGetLastErrorMessage()) < 0)
+goto error;
+
+if (virFileReadAll(configFile, 1024*1024*10, ) < 0)
+goto error;
+
+if (!(def = virDomainDefParseMinimal(NULL, xmlStr)))
+goto error;
+
+/*
+ * Remove the comment with a warning from the top.  Don't fail
+ * if we can't copy it or find it.
+ */
+tmp = strstr(xmlStr, "-->");
+
+if (tmp)
+tmp += strlen("-->");
+else
+tmp = xmlStr;
+
+if (virAsprintf(>xmlStr,
+"%s",
+_("WARNING: The following XML failed to load!"
+

[libvirt] [RFC PATCH 7/8] qemu: Prepare basic APIs to handle invalid defs

[Martin Kletzander]

In order for the user to be able to fix broken domains function
qemuDomainGetXMLDesc() needs to be able to lookup invalid domain
definitions and handle them properly.  When redefined, function
qemuDomainDefineXMLFlags() must clear the 'invalid XML' reason.  As a
nice addition, qemuDomainGetState() can lookup such domains without any
other change and that allows virsh not only to get their status, but
also to list them.

Signed-off-by: Martin Kletzander 
---
 src/qemu/qemu_driver.c | 21 ++---
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 0a671500134f..881c5c4c1984 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -2706,7 +2706,7 @@ qemuDomainGetState(virDomainPtr dom,

 virCheckFlags(0, -1);

-if (!(vm = qemuDomObjFromDomain(dom)))
+if (!(vm = qemuDomObjFromDomainInvalid(dom)))
 goto cleanup;

 if (virDomainGetStateEnsureACL(dom->conn, vm->def) < 0)
@@ -7082,19 +7082,23 @@ qemuDomainGetXMLDesc(virDomainPtr dom,

 /* Flags checked by virDomainDefFormat */

-if (!(vm = qemuDomObjFromDomain(dom)))
+if (!(vm = qemuDomObjFromDomainInvalid(dom)))
 goto cleanup;

 if (virDomainGetXMLDescEnsureACL(dom->conn, vm->def, flags) < 0)
 goto cleanup;

-if (qemuDomainUpdateCurrentMemorySize(driver, vm) < 0)
-goto cleanup;
+if (vm->def->parseError) {
+ignore_value(VIR_STRDUP(ret, vm->def->xmlStr));
+} else {
+if (qemuDomainUpdateCurrentMemorySize(driver, vm) < 0)
+goto cleanup;

-if ((flags & VIR_DOMAIN_XML_MIGRATABLE))
-flags |= QEMU_DOMAIN_FORMAT_LIVE_FLAGS;
+if ((flags & VIR_DOMAIN_XML_MIGRATABLE))
+flags |= QEMU_DOMAIN_FORMAT_LIVE_FLAGS;

-ret = qemuDomainFormatXML(driver, vm, flags);
+ret = qemuDomainFormatXML(driver, vm, flags);
+}

  cleanup:
 virDomainObjEndAPI();
@@ -7557,6 +7561,9 @@ qemuDomainDefineXMLFlags(virConnectPtr conn,
 goto cleanup;
 }

+if (oldDef && oldDef->parseError)
+virDomainObjSetState(vm, virDomainObjGetState(vm, NULL), 0);
+
 event = virDomainEventLifecycleNewFromObj(vm,
  VIR_DOMAIN_EVENT_DEFINED,
  !oldDef ?
-- 
2.5.3

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

[libvirt] [RFC PATCH 3/8] conf: Extract name-parsing into its own function

[Martin Kletzander]

Create virDomainDefParseName that parses only the name from XML
definition.  This will be used in future patches.

Signed-off-by: Martin Kletzander 
---
 src/conf/domain_conf.c | 18 ++
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 564f66de25bb..c2aeca2e52a6 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -14579,6 +14579,19 @@ virDomainVcpuParse(virDomainDefPtr def,
 return ret;
 }

+static int
+virDomainDefParseName(virDomainDefPtr def,
+  xmlXPathContextPtr ctxt)
+{
+if (!(def->name = virXPathString("string(./name[1])", ctxt))) {
+virReportError(VIR_ERR_NO_NAME, NULL);
+return -1;
+}
+
+return 0;
+}
+
+
 static virDomainDefPtr
 virDomainDefParseXML(xmlDocPtr xml,
  xmlNodePtr root,
@@ -14703,11 +14716,8 @@ virDomainDefParseXML(xmlDocPtr xml,
 VIR_FREE(capsdata);
 }

-/* Extract domain name */
-if (!(def->name = virXPathString("string(./name[1])", ctxt))) {
-virReportError(VIR_ERR_NO_NAME, NULL);
+if (virDomainDefParseName(def, ctxt) < 0)
 goto error;
-}

 /* Extract domain uuid. If both uuid and sysinfo/system/entry/uuid
  * exist, they must match; and if only the latter exists, it can
-- 
2.5.3

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

[libvirt] [RFC PATCH 0/8] Make loading domains with invalid XML possible

[Martin Kletzander]

We always had to deal with new parsing errors in a weird way.  All of
them needed to go into functions starting the domains.  That messes up
the code, it's confusing to newcomers and so on.

I once had an idea that we can handle this stuff.  We know what
failed, we have the XML that failed parsing and if the problem is not
in the domain name nor UUID, we can create a domain object out of that
as well.  This way we are able to do something we weren't with this
series applied.  Example follows.

Assume "dummy" is a domain with invalid XML (I just modified the
file).  Now, just for the purpose of this silly example, let's say we
allowed domains with archtecture x86_*, but now we've realized that
x86_64 is the only one we want to allow, but there already is a domain
defined that has .  With the current master,
the domain would be lost, so we would need to modify the funstion
starting the domain (e.g. qemuProcessStart for qemu domain).  However,
with this series, it behaves like this:

  # virsh list --all
   IdName   State
  
   - dummy  shut off

  # virsh domstate --reason dummy
  shut off (invalid XML)

  # virsh start dummy
  error: Failed to start domain dummy
  error: XML error: domain 'dummy' was not loaded due to an XML error
  (unsupported configuration: Unknown architecture x86_256), please
  redefine it

  # VISUAL='sed -i s/x86_256/x86_64/' virsh edit dummy
  Domain dummy XML configuration edited.

  # virsh domstate --reason dummy
  shut off (unknown)

  # virsh start dummy
  Domain dummy started

This is a logical next step for us to clean and separate parsing and
starting, getting rid of some old code without sacrifying compatibility
and maybe generating parser code in the future.

Why is this an RFC?
 - I can certainly easily imagine some people who might not like this
 - It doesn't work for all drivers (yet)
 - It does handle status XMLs, but only slightly.  All the handling is
   done in a commit message that says something along the lines of
   "beware, we should still be careful, but not as much as before".
 - The error type used for the message is one that is already used for
   something else and we might want a new type of error for exactly
   this one error message, although it might be enough for some to
   have the possibility of checking this by querying the domain state
   and checking the reason.
 - Just so you know I came up with something new for which there's no
   BZ (or at least yet) =)


Martin Kletzander (8):
  conf, virsh: Add new domain shutoff reason
  qemu: Few whitespace cleanups
  conf: Extract name-parsing into its own function
  conf: Extract UUID parsing into its own function
  conf: Optionally keep domains with invalid XML
  qemu: Don't lookup invalid domains unless specified otherwise
  qemu: Prepare basic APIs to handle invalid defs
  qemu: Load domains with invalid XML on start

 include/libvirt/libvirt-domain.h |   2 +
 src/bhyve/bhyve_driver.c |   2 +
 src/conf/domain_conf.c   | 180 +--
 src/conf/domain_conf.h   |   5 ++
 src/libxl/libxl_driver.c |   3 +
 src/lxc/lxc_driver.c |   3 +
 src/qemu/qemu_driver.c   |  64 +++---
 src/uml/uml_driver.c |   2 +
 tools/virsh-domain-monitor.c |   3 +-
 9 files changed, 223 insertions(+), 41 deletions(-)

--
2.5.3

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

[libvirt] [RFC PATCH 4/8] conf: Extract UUID parsing into its own function

[Martin Kletzander]

Create virDomainDefParseUUID that parses only the UUID from XML
definition.  This will be used in future patch(es).

Signed-off-by: Martin Kletzander 
---
 src/conf/domain_conf.c | 64 +++---
 1 file changed, 45 insertions(+), 19 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index c2aeca2e52a6..230800542f8c 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -14579,6 +14579,49 @@ virDomainVcpuParse(virDomainDefPtr def,
 return ret;
 }

+/*
+ * Parse domain's UUID, optionally generating it if missing.
+ */
+static int
+virDomainDefParseUUID(virDomainDefPtr def,
+  xmlXPathContextPtr ctxt,
+  bool required,
+  bool *generated)
+{
+char *tmp = virXPathString("string(./uuid[1])", ctxt);
+
+if (generated)
+*generated = false;
+
+if (tmp) {
+if (virUUIDParse(tmp, def->uuid) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   "%s", _("malformed uuid element"));
+VIR_FREE(tmp);
+return -1;
+}
+VIR_FREE(tmp);
+return 0;
+}
+
+if (required) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("Missing required UUID"));
+return -1;
+}
+
+if (virUUIDGenerate(def->uuid)) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   "%s", _("Failed to generate UUID"));
+return -1;
+}
+
+if (generated)
+*generated = true;
+
+return 0;
+}
+
 static int
 virDomainDefParseName(virDomainDefPtr def,
   xmlXPathContextPtr ctxt)
@@ -14719,25 +14762,8 @@ virDomainDefParseXML(xmlDocPtr xml,
 if (virDomainDefParseName(def, ctxt) < 0)
 goto error;

-/* Extract domain uuid. If both uuid and sysinfo/system/entry/uuid
- * exist, they must match; and if only the latter exists, it can
- * also serve as the uuid. */
-tmp = virXPathString("string(./uuid[1])", ctxt);
-if (!tmp) {
-if (virUUIDGenerate(def->uuid)) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
-   "%s", _("Failed to generate UUID"));
-goto error;
-}
-uuid_generated = true;
-} else {
-if (virUUIDParse(tmp, def->uuid) < 0) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
-   "%s", _("malformed uuid element"));
-goto error;
-}
-VIR_FREE(tmp);
-}
+if (virDomainDefParseUUID(def, ctxt, false, _generated) < 0)
+goto error;

 /* Extract short description of domain (title) */
 def->title = virXPathString("string(./title[1])", ctxt);
-- 
2.5.3

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

[libvirt] [RFC PATCH 1/8] conf, virsh: Add new domain shutoff reason

[Martin Kletzander]

This new reason means the domain is shutoff because parsing of the XML
failed while daemon was starting and from user's point of view, there's
nothing else to do with it other than re-defining it.

Signed-off-by: Martin Kletzander 
---
 include/libvirt/libvirt-domain.h | 2 ++
 src/conf/domain_conf.c   | 3 ++-
 tools/virsh-domain-monitor.c | 3 ++-
 3 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
index a1ea6a5d0786..37aa6db85cda 100644
--- a/include/libvirt/libvirt-domain.h
+++ b/include/libvirt/libvirt-domain.h
@@ -142,6 +142,8 @@ typedef enum {
 VIR_DOMAIN_SHUTOFF_FAILED = 6,  /* domain failed to start */
 VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT = 7, /* restored from a snapshot which was
* taken while domain was shutoff */
+VIR_DOMAIN_SHUTOFF_INVALID_XML = 8, /* Domain XML failed to load for some
+ * reason, it needs to be re-defined */
 # ifdef VIR_ENUM_SENTINELS
 VIR_DOMAIN_SHUTOFF_LAST
 # endif
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index a3b3ccb0d7cc..564f66de25bb 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -710,7 +710,8 @@ VIR_ENUM_IMPL(virDomainShutoffReason, 
VIR_DOMAIN_SHUTOFF_LAST,
   "migrated",
   "saved",
   "failed",
-  "from snapshot")
+  "from snapshot",
+  "invalid XML")

 VIR_ENUM_IMPL(virDomainCrashedReason, VIR_DOMAIN_CRASHED_LAST,
   "unknown",
diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c
index d4e500b21225..cdcee3cbb267 100644
--- a/tools/virsh-domain-monitor.c
+++ b/tools/virsh-domain-monitor.c
@@ -218,7 +218,8 @@ VIR_ENUM_IMPL(virshDomainShutoffReason,
   N_("migrated"),
   N_("saved"),
   N_("failed"),
-  N_("from snapshot"))
+  N_("from snapshot"),
+  N_("invalid XML"))

 VIR_ENUM_DECL(virshDomainCrashedReason)
 VIR_ENUM_IMPL(virshDomainCrashedReason,
-- 
2.5.3

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

[libvirt] [RFC PATCH 2/8] qemu: Few whitespace cleanups

[Martin Kletzander]

Signed-off-by: Martin Kletzander 
---
 src/qemu/qemu_driver.c | 11 +++
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index fc3b60d01c01..8197f0238fb7 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -7043,9 +7043,9 @@ qemuDomainObjRestore(virConnectPtr conn,
 }


-static char
-*qemuDomainGetXMLDesc(virDomainPtr dom,
-  unsigned int flags)
+static char *
+qemuDomainGetXMLDesc(virDomainPtr dom,
+ unsigned int flags)
 {
 virQEMUDriverPtr driver = dom->conn->privateData;
 virDomainObjPtr vm;
@@ -7449,7 +7449,10 @@ qemuDomainCreate(virDomainPtr dom)
 return qemuDomainCreateWithFlags(dom, 0);
 }

-static virDomainPtr qemuDomainDefineXMLFlags(virConnectPtr conn, const char 
*xml, unsigned int flags)
+static virDomainPtr
+qemuDomainDefineXMLFlags(virConnectPtr conn,
+ const char *xml,
+ unsigned int flags)
 {
 virQEMUDriverPtr driver = conn->privateData;
 virDomainDefPtr def = NULL;
-- 
2.5.3

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

Re: [libvirt] [PATCH 07/13] conf: Split memory related post parse stuff into separate function

[Michal Privoznik]

On 21.09.2015 19:21, Peter Krempa wrote:
> The post parse func is growing rather large. Since later patches will
> introduce more logic in the memory post parse code, split it into a
> separate handler.
> ---
>  src/conf/domain_conf.c | 32 +---
>  1 file changed, 21 insertions(+), 11 deletions(-)

ACK

Michal

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

Re: [libvirt] [PATCH 10/13] conf: Don't always recalculate initial memory size from NUMA size totals

[Michal Privoznik]

On 21.09.2015 19:21, Peter Krempa wrote:
> When implementing memory hotplug I've opted to recalculate the initial
> memory size (contents of the  element) as a sum of the sizes of
> NUMA nodes when NUMA was enabled. This was based on an assumption that
> qemu did not allow starting when the NUMA node size total didn't equal
> to the initial memory size. Unfortunately the check was introduced to
> qemu just lately.
> 
> This patch uses the new XML parser flag to decide whether it's safe to
> update the memory size total from the NUMA cell sizes or not.
> 
> As an additional improvement we now report an error in case when the
> size of hotplug memory would exceed the total memory size.
> 
> The rest of the changes assures that the function is called with correct
> flags.
> ---
>  src/conf/domain_conf.c  | 37 ++---
>  src/conf/domain_conf.h  |  1 +
>  src/qemu/qemu_command.c |  3 ++-
>  3 files changed, 33 insertions(+), 8 deletions(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index d2a13ca..64cfd8c 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -3726,15 +3726,36 @@ virDomainDefRemoveDuplicateMetadata(virDomainDefPtr 
> def)
> 
> 
>  static int
> -virDomainDefPostParseMemory(virDomainDefPtr def)
> +virDomainDefPostParseMemory(virDomainDefPtr def,
> +unsigned int parseFlags)
>  {
>  size_t i;
> +unsigned long long numaMemory = 0;
> +unsigned long long hotplugMemory = 0;
> 
> -if ((def->mem.initial_memory = virDomainNumaGetMemorySize(def->numa)) == 
> 0) {
> +/* Attempt to infer the initial memory size from the sum NUMA memory 
> sizes
> + * in case ABI updates are allowed or the  element wasn't 
> specified */
> +if (def->mem.total_memory == 0 ||
> +parseFlags & VIR_DOMAIN_DEF_PARSE_ABI_UPDATE)
> +numaMemory = virDomainNumaGetMemorySize(def->numa);
> +
> +if (numaMemory) {
> +def->mem.initial_memory = numaMemory;

Is there a reason to not use the setter function you've introduced in
previous patch?

> +} else {
>  def->mem.initial_memory = def->mem.total_memory;
> 
> +/* calculate the sizes of hotplug memory */
>  for (i = 0; i < def->nmems; i++)
> -def->mem.initial_memory -= def->mems[i]->size;
> +hotplugMemory += def->mems[i]->size;
> +
> +if (hotplugMemory > def->mem.initial_memory) {
> +virReportError(VIR_ERR_XML_ERROR, "%s",
> +   _("Total size of memory devices exceeds the total 
> "
> + "memory size"));
> +return -1;
> +}
> +
> +def->mem.initial_memory -= hotplugMemory;
>  }
> 
>  if (virDomainDefGetMemoryInitial(def) == 0) {
> @@ -3770,7 +3791,8 @@ virDomainDefPostParseMemory(virDomainDefPtr def)
> 
>  static int
>  virDomainDefPostParseInternal(virDomainDefPtr def,
> -  virCapsPtr caps ATTRIBUTE_UNUSED)
> +  virCapsPtr caps ATTRIBUTE_UNUSED,
> +  unsigned int parseFlags)
>  {
>  size_t i;
> 
> @@ -3781,7 +3803,7 @@ virDomainDefPostParseInternal(virDomainDefPtr def,
>  return -1;
>  }
> 
> -if (virDomainDefPostParseMemory(def) < 0)
> +if (virDomainDefPostParseMemory(def, parseFlags) < 0)
>  return -1;
> 
>  /*
> @@ -4274,6 +4296,7 @@ virDomainDefPostParseDeviceIterator(virDomainDefPtr def 
> ATTRIBUTE_UNUSED,
>  int
>  virDomainDefPostParse(virDomainDefPtr def,
>virCapsPtr caps,
> +  unsigned int parseFlags,
>virDomainXMLOptionPtr xmlopt)
>  {
>  int ret;
> @@ -4299,7 +4322,7 @@ virDomainDefPostParse(virDomainDefPtr def,
>  return ret;
> 
> 
> -if ((ret = virDomainDefPostParseInternal(def, caps)) < 0)
> +if ((ret = virDomainDefPostParseInternal(def, caps, parseFlags)) < 0)
>  return ret;
> 
>  return 0;
> @@ -16426,7 +16449,7 @@ virDomainDefParseXML(xmlDocPtr xml,
>  goto error;
> 
>  /* callback to fill driver specific domain aspects */
> -if (virDomainDefPostParse(def, caps, xmlopt) < 0)
> +if (virDomainDefPostParse(def, caps, flags, xmlopt) < 0)
>  goto error;
> 
>  /* Auto-add any implied controllers which aren't present */
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index ab250bd..25914b4 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -2459,6 +2459,7 @@ virDomainXMLOptionGetNamespace(virDomainXMLOptionPtr 
> xmlopt)
>  int
>  virDomainDefPostParse(virDomainDefPtr def,
>virCapsPtr caps,
> +  unsigned int parseFlags,
>virDomainXMLOptionPtr xmlopt);
> 
>  static inline bool
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 3044b11..7a1c9fe 100644
> --- a/src/qemu/qemu_command.c
> +++ 

Re: [libvirt] [PATCH 13/13] test: Add test to validate that memory sizes don't get updated on migration

[Michal Privoznik]

On 21.09.2015 19:21, Peter Krempa wrote:
> ---
>  .../qemuxml2argv-migrate-numa-unaligned.args   | 13 +
>  .../qemuxml2argv-migrate-numa-unaligned.xml| 33 
> ++
>  tests/qemuxml2argvtest.c   | 13 +++--
>  3 files changed, 56 insertions(+), 3 deletions(-)
>  create mode 100644 
> tests/qemuxml2argvdata/qemuxml2argv-migrate-numa-unaligned.args
>  create mode 100644 
> tests/qemuxml2argvdata/qemuxml2argv-migrate-numa-unaligned.xml

ACK

Michal

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

Re: [libvirt] [PATCH 12/13] qemu: ppc64: Align memory sizes to 256MiB blocks

[Michal Privoznik]

On 21.09.2015 19:21, Peter Krempa wrote:
> For some machine types ppc64 machines now require that memory sizes are
> aligned to 256MiB increments (due to the dynamically reconfigurable
> memory). As now we treat existing configs reasonably in regards to
> migration, we can round all the sizes unconditionally. The only drawback
> will be that the memory size of a VM can potentially increase by
> (256MiB - 1byte) * number_of_NUMA_nodes.
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1249006
> ---
> CC: David Gibson 
> 
>  src/qemu/qemu_domain.c  | 7 ++-
>  tests/qemuxml2argvdata/qemuxml2argv-pseries-cpu-compat.args | 2 +-
>  2 files changed, 7 insertions(+), 2 deletions(-)

ACK

Michal

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

Re: [libvirt] [RFC PATCH 7/8] qemu: Prepare basic APIs to handle invalid defs

[Daniel P. Berrange]

On Tue, Sep 22, 2015 at 03:12:14PM +0200, Martin Kletzander wrote:
> On Tue, Sep 22, 2015 at 01:48:01PM +0100, Daniel P. Berrange wrote:
> >On Tue, Sep 22, 2015 at 02:15:53PM +0200, Martin Kletzander wrote:
> >>In order for the user to be able to fix broken domains function
> >>qemuDomainGetXMLDesc() needs to be able to lookup invalid domain
> >>definitions and handle them properly.  When redefined, function
> >>qemuDomainDefineXMLFlags() must clear the 'invalid XML' reason.  As a
> >>nice addition, qemuDomainGetState() can lookup such domains without any
> >>other change and that allows virsh not only to get their status, but
> >>also to list them.
> >
> >Hmm, that's an interesting approach to the problem. I wonder though
> >if we could do things slightly differently such that we don't need
> >to change so many APIs.
> >
> >eg, just have a 'bool error' field in virDomainDefPtr. When loading
> >the XML fails, populate a virDomainObjPtr/DefPtr as normal, but set
> >the error field. Now we merely need to change the qemuDomainStart
> >method, so it refuses to launch a VM with the 'error' flag set. All
> >the other APIs could be essentially unchanged. Sure it would not
> >be useful to allow things like virDomainAttachDevice, etc on such
> >broken domains, but for sake of simplicity we can avoid touching
> >all the methods except start.
> >
> 
> To be honest, I'm a afraid that we might forget some API that needs to
> be blocked as well.  And we would have to go through all the APIs just
> to see whether it accesses something that might be missing.  Moreover,
> how would you decide what to skip at each error during parsing?  If,
> for example, the  has some faulty attribute in one
> subelement should we skip all the elements or just the one or just
> skip that one particular attribute?  We would also not format the
> faulty attribute to the XML being dumped, so the user wouldn't see
> what's missing, which is even worse when there are two problems and
> they fix only one, so we skip the other one.

Oh, I wasn't suggesting changing the parser. I just mean that we would
create a virDomainDefPtr instance which only contains the name and
UUID, and ID field (if the guest is running from domain status XML).

We'd leave all the rest of the config blank - our code ought to be
able to deal with that, since essentially all config is optional
aside from the name/uuid anyway.

> I thought about your approach as well, but that would require complete
> rewrite of the whole parsing code (maybe changing it to generated one
> based on RNG schema and some minor additional info) and it would touch
> way more APIs.  The approach I'm suggesting here keeps almost all APIs
> untouched and it doesn't have an effect on anything, unless explicitly
> specified.
> 
> Check last two patches to see how much qemu driver needs to be
> changed, for others it will be similar, probably a little bit less..
> The patches are 17+/9- when combined.  I don't think you can do much
> better than that.  And that doesn't show the improvements that can be
> made in the starting and parsing code after this series is applied.

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

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

[libvirt] [RFC PATCH 8/8] qemu: Load domains with invalid XML on start

[Martin Kletzander]

If we load such domains, we don't need to handle invalid XML checking in
qemuProcessStart, but we can move it to qemuDomainDefPostParse() or even
into the parsing functions if all goes well.  So the only thing we'll
need to worry about after this is XML parsing code that would error out
for running domains or non-qemu drivers.  The latter will be fixed in
following patches, hence making the active XML parsing the only
problematic part.

Signed-off-by: Martin Kletzander 
---
 src/qemu/qemu_driver.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 881c5c4c1984..c4df44c9f71f 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -312,6 +312,7 @@ qemuAutostartDomain(virDomainObjPtr vm,
 virObjectRef(vm);
 virResetLastError();
 if (vm->autostart &&
+!vm->def->parseError &&
 !virDomainObjIsActive(vm)) {
 if (qemuDomainObjBeginJob(data->driver, vm,
   QEMU_JOB_MODIFY) < 0) {
@@ -951,7 +952,7 @@ qemuStateInitialize(bool privileged,
cfg->autostartDir, 0,
qemu_driver->caps,
qemu_driver->xmlopt,
-   false,
+   true,
NULL, NULL) < 0)
 goto error;

@@ -1032,7 +1033,7 @@ qemuStateReload(void)
cfg->configDir,
cfg->autostartDir, 0,
caps, qemu_driver->xmlopt,
-   false,
+   true,
qemuNotifyLoadDomain, qemu_driver);
  cleanup:
 virObjectUnref(cfg);
-- 
2.5.3

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

[libvirt] [RFC PATCH 6/8] qemu: Don't lookup invalid domains unless specified otherwise

[Martin Kletzander]

Change qemuDomObjFromDomain() to qemuDomObjFromDomainInternal() with
additional parameter that controls how to deal with invalid domains.
New qemuDomObjFromDomain() then follows the safe path wo we don't have
to change its callers from all APIs and qemuDomObjFromDomainInvalid() is
added as a new function that can be called from APIs that are prepared
to handle invalid definitions as well.

Signed-off-by: Martin Kletzander 
---
 src/qemu/qemu_driver.c | 28 +++-
 1 file changed, 27 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index e7b47eaa944c..0a671500134f 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -209,15 +209,22 @@ struct qemuAutostartData {
 /**
  * qemuDomObjFromDomain:
  * @domain: Domain pointer that has to be looked up
+ * @invalid: Whether to also return invalid definitions
  *
  * This function looks up @domain and returns the appropriate virDomainObjPtr
  * that has to be released by calling virDomainObjEndAPI().
  *
+ * If @invalid is true, this function can return domain definition with only
+ * name and uuid set, so beware as that can lead to NULL pointer dereference.
+ * This function should be called with @invalid == true only from APIs that are
+ * needed to fix the domain definition (e.g. those needed for looking it up or
+ * providing a new definition.
+ *
  * Returns the domain object with incremented reference counter which is locked
  * on success, NULL otherwise.
  */
 static virDomainObjPtr
-qemuDomObjFromDomain(virDomainPtr domain)
+qemuDomObjFromDomainInternal(virDomainPtr domain, bool invalid)
 {
 virDomainObjPtr vm;
 virQEMUDriverPtr driver = domain->conn->privateData;
@@ -231,10 +238,29 @@ qemuDomObjFromDomain(virDomainPtr domain)
uuidstr, domain->name);
 return NULL;
 }
+if (!invalid && vm->def->parseError) {
+virReportError(VIR_ERR_XML_ERROR,
+   _("domain '%s' was not loaded due to an XML error "
+ "(%s), please redefine it"),
+   vm->def->name, vm->def->parseError);
+virDomainObjEndAPI();
+}

 return vm;
 }

+static inline virDomainObjPtr
+qemuDomObjFromDomain(virDomainPtr domain)
+{
+return qemuDomObjFromDomainInternal(domain, false);
+}
+
+static inline virDomainObjPtr
+qemuDomObjFromDomainInvalid(virDomainPtr domain)
+{
+return qemuDomObjFromDomainInternal(domain, true);
+}
+
 /* Looks up the domain object from snapshot and unlocks the
  * driver. The returned domain object is locked and ref'd and the
  * caller must call virDomainObjEndAPI() on it. */
-- 
2.5.3

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

Re: [libvirt] [PATCH 08/13] conf: Rename max_balloon to total_memory

[Michal Privoznik]

On 21.09.2015 19:21, Peter Krempa wrote:
> The name of the variable was misleading. Rename it and it's setting
> accessor before other fixes.
> ---
>  src/conf/domain_conf.c | 18 +-
>  src/conf/domain_conf.h |  7 ---
>  src/hyperv/hyperv_driver.c |  2 +-
>  src/libvirt_private.syms   |  2 +-
>  src/libxl/libxl_driver.c   |  4 ++--
>  src/lxc/lxc_driver.c   |  2 +-
>  src/lxc/lxc_native.c   |  4 ++--
>  src/phyp/phyp_driver.c |  2 +-
>  src/qemu/qemu_command.c|  4 ++--
>  src/qemu/qemu_domain.c |  2 +-
>  src/qemu/qemu_driver.c |  2 +-
>  src/test/test_driver.c |  2 +-
>  src/uml/uml_driver.c   |  2 +-
>  src/vbox/vbox_common.c |  4 ++--
>  src/vmx/vmx.c  |  2 +-
>  src/vz/vz_sdk.c|  2 +-
>  src/xen/xm_internal.c  |  2 +-
>  src/xenapi/xenapi_driver.c |  2 +-
>  src/xenconfig/xen_common.c |  2 +-
>  src/xenconfig/xen_sxpr.c   |  2 +-
>  20 files changed, 35 insertions(+), 34 deletions(-)

ACK

Michal

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

Re: [libvirt] [PATCH 04/13] conf: Drop VIR_DOMAIN_DEF_PARSE_CLOCK_ADJUST flag

[Michal Privoznik]

On 21.09.2015 19:21, Peter Krempa wrote:
> The flag was used only for formatting the XML and once the parser and
> formatter flags were split in 0ecd6851093945dd5ddc78266c61b577c65394ae
> it doesn't make sense any more to have it.
> ---
>  src/conf/domain_conf.c  | 1 -
>  src/conf/domain_conf.h  | 7 +++
>  tests/qemuxml2xmltest.c | 3 +--
>  3 files changed, 4 insertions(+), 7 deletions(-)

ACK

Michal

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

Re: [libvirt] [PATCH 01/13] libxl: vz: Use accessor instead of direct access for max_balloon

[Michal Privoznik]

On 21.09.2015 19:21, Peter Krempa wrote:
> Commits 45697fe5 and f863ac80 used direct access to the variable instead
> of the preferred accessor method.
> ---
>  src/libxl/libxl_driver.c | 2 +-
>  src/vz/vz_driver.c   | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> index 8087c27..019f04f 100644
> --- a/src/libxl/libxl_driver.c
> +++ b/src/libxl/libxl_driver.c
> @@ -553,7 +553,7 @@ libxlAddDom0(libxlDriverPrivatePtr driver)
>  vm->def->vcpus = d_info.vcpu_online;
>  vm->def->maxvcpus = d_info.vcpu_max_id + 1;
>  vm->def->mem.cur_balloon = d_info.current_memkb;
> -vm->def->mem.max_balloon = d_info.max_memkb;
> +virDomainDefSetMemoryInitial(vm->def, d_info.max_memkb);
> 
>  ret = 0;
> 
> diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c
> index 8fa7957..ba5d7e4 100644
> --- a/src/vz/vz_driver.c
> +++ b/src/vz/vz_driver.c
> @@ -1200,7 +1200,7 @@ vzDomainGetMaxMemory(virDomainPtr domain)
>  if (!(dom = vzDomObjFromDomain(domain)))
>  return -1;
> 
> -ret = dom->def->mem.max_balloon;
> +ret = virDomainDefGetMemoryActual(dom->def->mem);
>  virObjectUnlock(dom);
>  return ret;
>  }
> 

ACK with this squashed in:

diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c
index ba5d7e4..2eb025c 100644
--- a/src/vz/vz_driver.c
+++ b/src/vz/vz_driver.c
@@ -1200,7 +1200,7 @@ vzDomainGetMaxMemory(virDomainPtr domain)
 if (!(dom = vzDomObjFromDomain(domain)))
 return -1;

-ret = virDomainDefGetMemoryActual(dom->def->mem);
+ret = virDomainDefGetMemoryActual(dom->def);
 virObjectUnlock(dom);
 return ret;
 }


Michal

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

Re: [libvirt] [PATCH 09/13] conf: Pre-calculate initial memory size instead of always calculating it

[Michal Privoznik]

On 21.09.2015 19:21, Peter Krempa wrote:
> Add 'initial_memory' member to struct virDomainMemtune so that the
> memory size can be pre-calculated once instead of inferring it always
> again and again.
> 
> Separating of the fields will also allow finer granularity of decisions
> in later patches where it will allow to keep the old initial memory
> value in cases where we are handling incomming migration from older
> versions that did not always update the size from NUMA as the code did
> previously.
> 
> The change also requires modification of the qemu memory alignment
> function since at the point where we are modifying the size of NUMA
> nodes the total size needs to be recalculated too.
> 
> The refactoring done in this patch also fixes a crash in the hyperv
> driver that did not properly initialize def->numa and thus
> virDomainNumaGetMemorySize(def->numa) crashed.
> 
> In summary this patch should have no functional impact at this point.
> ---
>  src/conf/domain_conf.c   | 52 
> +---
>  src/conf/domain_conf.h   |  3 +++
>  src/libvirt_private.syms |  1 +
>  src/qemu/qemu_domain.c   | 15 +-
>  4 files changed, 46 insertions(+), 25 deletions(-)

ACK

Michal

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

Re: [libvirt] [PATCH 11/13] qemu: command: Align memory sizes only on fresh starts

[Michal Privoznik]

On 21.09.2015 19:21, Peter Krempa wrote:
> When we are starting a qemu process for an incomming migration or
> snapshot reloading we should not modify the memory sizes in the domain
> since we could potentially change the guest ABI that was tediously
> checked before. Additionally the function now updates the initial memory
> size according to the NUMA node size, which should not happen if we are
> restoring state.
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1252685
> ---
>  src/qemu/qemu_command.c | 3 ++-
>  tests/qemuxml2argvdata/qemuxml2argv-restore-v1.args | 2 +-
>  2 files changed, 3 insertions(+), 2 deletions(-)

ACK

Michal

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

Re: [libvirt] [PATCH] Revert "qemu: Fix integer/boolean logic in qemuSetUnprivSGIO"

[Ján Tomko]

On Fri, Sep 18, 2015 at 03:56:34PM -0400, John Ferlan wrote:
> This reverts commit 69b850fe2a19d0c32ae2f209e8d8463df6ead665.
> 
> This change broke the ability to "clear" or reset unfiltered back
> to filtered.
> 
> Signed-off-by: John Ferlan 
> ---
> 
> This is a  event - have to wonder what I was thinking
> at the time. The patch pushed removes the ability to actually
> clear the value in the file. Although it's possible to check what's
> in the file first, it's still important to "attempt" the check if
> someone set sgio on a disk and unpriv_sgio isn't supported. So, just
> reverting this restores the former functionality of setting to
> either 0 or 1.
> 
>  src/qemu/qemu_conf.c | 10 +++---
>  1 file changed, 3 insertions(+), 7 deletions(-)
> 

ACK

Jan


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

Re: [libvirt] [PATCH 13/13] test: Add test to validate that memory sizes don't get updated on migration

[Peter Krempa]

On Tue, Sep 22, 2015 at 14:28:50 +0200, Michal Privoznik wrote:
> On 21.09.2015 19:21, Peter Krempa wrote:
> > ---
> >  .../qemuxml2argv-migrate-numa-unaligned.args   | 13 +
> >  .../qemuxml2argv-migrate-numa-unaligned.xml| 33 
> > ++
> >  tests/qemuxml2argvtest.c   | 13 +++--
> >  3 files changed, 56 insertions(+), 3 deletions(-)
> >  create mode 100644 
> > tests/qemuxml2argvdata/qemuxml2argv-migrate-numa-unaligned.args
> >  create mode 100644 
> > tests/qemuxml2argvdata/qemuxml2argv-migrate-numa-unaligned.xml
> 
> ACK

Thanks, I've fixed few of the problems you've pointed out and pushed the
series.

Peter


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

[libvirt] [PATCH 0/2] Couple of fixes after mockup patches

[Michal Privoznik]

I've noticed a few tests failing strangely after [1] was merged. Here are the
fixes.

1: https://www.redhat.com/archives/libvir-list/2015-September/msg00460.html

Michal Privoznik (2):
  qemuTestDriverInit: init the driver lock too
  tests: Avoid use of virQEMUDriverCreateXMLConf(NULL)

 tests/qemucapabilitiestest.c |  9 +
 tests/qemumonitorjsontest.c  | 19 ++-
 tests/qemumonitortest.c  |  9 +
 tests/securityselinuxlabeltest.c |  8 +---
 tests/testutilsqemu.c|  6 +-
 5 files changed, 30 insertions(+), 21 deletions(-)

-- 
2.4.9

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

[libvirt] [PATCH 2/2] tests: Avoid use of virQEMUDriverCreateXMLConf(NULL)

[Michal Privoznik]

We use the function to create a virDomainXMLOption object that is
required for some functions. However, we don't pass the driver
pointer to the object anywhere - rather than pass NULL. This
causes trouble later when parsing a domain XML and calling post
parse callbacks:

  Program received signal SIGSEGV, Segmentation fault.
  0x0043fa3e in qemuDomainDefPostParse (def=0x7d36c0, caps=0x7caf10, 
opaque=0x0) at qemu/qemu_domain.c:1043
  1043qemuCaps = virQEMUCapsCacheLookup(driver->qemuCapsCache, 
def->emulator);
  (gdb) bt
  #0  0x0043fa3e in qemuDomainDefPostParse (def=0x7d36c0, 
caps=0x7caf10, opaque=0x0) at qemu/qemu_domain.c:1043
  #1  0x72928bf9 in virDomainDefPostParse (def=0x7d36c0, caps=0x7caf10, 
xmlopt=0x7c82c0) at conf/domain_conf.c:4269
  #2  0x7294de04 in virDomainDefParseXML (xml=0x7da8c0, root=0x7dab80, 
ctxt=0x7da980, caps=0x7caf10, xmlopt=0x7c82c0, flags=0) at 
conf/domain_conf.c:16400
  #3  0x7294e5b5 in virDomainDefParseNode (xml=0x7da8c0, root=0x7dab80, 
caps=0x7caf10, xmlopt=0x7c82c0, flags=0) at conf/domain_conf.c:16582
  #4  0x7294e424 in virDomainDefParse (xmlStr=0x0, filename=0x7c7ef0 
"/home/zippy/work/libvirt/libvirt.git/tests/securityselinuxlabeldata/disks.xml",
 caps=0x7caf10, xmlopt=0x7c82c0, flags=0) at conf/domain_conf.c:16529
  #5  0x7294e4b2 in virDomainDefParseFile (filename=0x7c7ef0 
"/home/zippy/work/libvirt/libvirt.git/tests/securityselinuxlabeldata/disks.xml",
 caps=0x7caf10, xmlopt=0x7c82c0, flags=0) at conf/domain_conf.c:16553
  #6  0x004303ca in testSELinuxLoadDef (testname=0x53c929 "disks") at 
securityselinuxlabeltest.c:192
  #7  0x004309e8 in testSELinuxLabeling (opaque=0x53c929) at 
securityselinuxlabeltest.c:313
  #8  0x00431207 in virtTestRun (title=0x53c92f "Labelling \"disks\"", 
body=0x430964 , data=0x53c929) at testutils.c:211
  #9  0x00430c5d in mymain () at securityselinuxlabeltest.c:373
  #10 0x004325c2 in virtTestMain (argc=1, argv=0x7fffd7e8, 
func=0x430b4a ) at testutils.c:863
  #11 0x00430deb in main (argc=1, argv=0x7fffd7e8) at 
securityselinuxlabeltest.c:381

Signed-off-by: Michal Privoznik 
---
 tests/qemucapabilitiestest.c |  9 +
 tests/qemumonitorjsontest.c  | 19 ++-
 tests/qemumonitortest.c  |  9 +
 tests/securityselinuxlabeltest.c |  8 +---
 4 files changed, 25 insertions(+), 20 deletions(-)

diff --git a/tests/qemucapabilitiestest.c b/tests/qemucapabilitiestest.c
index 0fbc43c..99abd02 100644
--- a/tests/qemucapabilitiestest.c
+++ b/tests/qemucapabilitiestest.c
@@ -160,7 +160,7 @@ static int
 mymain(void)
 {
 int ret = 0;
-virDomainXMLOptionPtr xmlopt;
+virQEMUDriver driver;
 testQemuData data;
 
 #if !WITH_YAJL
@@ -169,12 +169,12 @@ mymain(void)
 #endif
 
 if (virThreadInitialize() < 0 ||
-!(xmlopt = virQEMUDriverCreateXMLConf(NULL)))
+qemuTestDriverInit() < 0)
 return EXIT_FAILURE;
 
 virEventRegisterDefaultImpl();
 
-data.xmlopt = xmlopt;
+data.xmlopt = driver.xmlopt;
 
 #define DO_TEST(name)   \
 do {\
@@ -191,7 +191,8 @@ mymain(void)
 DO_TEST("caps_1.6.50-1");
 DO_TEST("caps_2.1.1-1");
 
-virObjectUnref(xmlopt);
+qemuTestDriverFree();
+
 return (ret == 0) ? EXIT_SUCCESS : EXIT_FAILURE;
 }
 
diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c
index ded0423..7d4741e 100644
--- a/tests/qemumonitorjsontest.c
+++ b/tests/qemumonitorjsontest.c
@@ -2238,7 +2238,7 @@ static int
 mymain(void)
 {
 int ret = 0;
-virDomainXMLOptionPtr xmlopt;
+virQEMUDriver driver;
 testQemuMonitorJSONSimpleFuncData simpleFunc;
 
 #if !WITH_YAJL
@@ -2247,29 +2247,30 @@ mymain(void)
 #endif
 
 if (virThreadInitialize() < 0 ||
-!(xmlopt = virQEMUDriverCreateXMLConf(NULL)))
+qemuTestDriverInit() < 0)
 return EXIT_FAILURE;
 
 virEventRegisterDefaultImpl();
 
-#define DO_TEST(name)   \
-if (virtTestRun(# name, testQemuMonitorJSON ## name, xmlopt) < 0)   \
+#define DO_TEST(name)  
\
+if (virtTestRun(# name, testQemuMonitorJSON ## name, driver.xmlopt) < 0)   
\
 ret = -1
 
 #define DO_TEST_SIMPLE(CMD, FNC, ...)   \
 simpleFunc = (testQemuMonitorJSONSimpleFuncData) {.cmd = CMD, .func = FNC, 
\
-  .xmlopt = xmlopt, __VA_ARGS__ }; 
\
+   .xmlopt = driver.xmlopt, __VA_ARGS__ }; 
\
 if (virtTestRun(# FNC, testQemuMonitorJSONSimpleFunc, ) < 0)
\
 ret = -1
 
 #define DO_TEST_GEN(name, ...) \
-simpleFunc = (testQemuMonitorJSONSimpleFuncData) {.xmlopt = xmlopt, 
__VA_ARGS__ }; \
-if (virtTestRun(# name, 

[libvirt] [PATCH 1/2] qemuTestDriverInit: init the driver lock too

[Michal Privoznik]

Even though usage of the lock is limited to a very few cases,
it's still needed. Therefore we should initialize it too.
Otherwise we may get some random test failures:

==1204== Conditional jump or move depends on uninitialised value(s)
==1204==at 0xEF7F7CF: pthread_mutex_lock (in /lib64/libpthread-2.20.so)
==1204==by 0x9CA89A5: virMutexLock (virthread.c:89)
==1204==by 0x450B2A: qemuDriverLock (qemu_conf.c:83)
==1204==by 0x45549C: virQEMUDriverGetConfig (qemu_conf.c:869)
==1204==by 0x448E29: qemuDomainDeviceDefPostParse (qemu_domain.c:1240)
==1204==by 0x9CC9B13: virDomainDeviceDefPostParse (domain_conf.c:4224)
==1204==by 0x9CC9B91: virDomainDefPostParseDeviceIterator 
(domain_conf.c:4251)
==1204==by 0x9CC7843: virDomainDeviceInfoIterateInternal 
(domain_conf.c:3440)
==1204==by 0x9CC9C25: virDomainDefPostParse (domain_conf.c:4276)
==1204==by 0x9CEEE03: virDomainDefParseXML (domain_conf.c:16400)
==1204==by 0x9CEF5B4: virDomainDefParseNode (domain_conf.c:16582)
==1204==by 0x9CEF423: virDomainDefParse (domain_conf.c:16529)

Signed-off-by: Michal Privoznik 
---
 tests/testutilsqemu.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/tests/testutilsqemu.c b/tests/testutilsqemu.c
index cec2f6c..2f2be12 100644
--- a/tests/testutilsqemu.c
+++ b/tests/testutilsqemu.c
@@ -531,6 +531,7 @@ qemuTestParseCapabilities(const char *capsFile)
 
 void qemuTestDriverFree(virQEMUDriver *driver)
 {
+virMutexDestroy(>lock);
 virQEMUCapsCacheFree(driver->qemuCapsCache);
 virObjectUnref(driver->xmlopt);
 virObjectUnref(driver->caps);
@@ -567,9 +568,12 @@ int qemuTestCapsCacheInsert(virQEMUCapsCachePtr cache, 
const char *binary,
 
 int qemuTestDriverInit(virQEMUDriver *driver)
 {
+if (virMutexInit(>lock) < 0)
+return -1;
+
 driver->config = virQEMUDriverConfigNew(false);
 if (!driver->config)
-return -1;
+goto error;
 
 driver->caps = testQemuCapsInit();
 if (!driver->caps)
-- 
2.4.9

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

Re: [libvirt] [PATCH] add compress stream support

[Daniel P. Berrange]

On Tue, Sep 22, 2015 at 02:10:41PM +, Vasiliy Tolstov wrote:
> use libarchive for compressed stream support

Can you explain a bit more about how you expect this to be
used ?

> Signed-off-by: Vasiliy Tolstov 
> ---
>  configure.ac | 11 ++--
>  include/libvirt/libvirt-stream.h |  6 +
>  m4/virt-archive.m4   | 26 +++
>  src/fdstream.c   | 47 +
>  src/libvirt-stream.c | 56 
> +++-
>  src/libvirt_public.syms  |  7 +
>  6 files changed, 150 insertions(+), 3 deletions(-)
>  create mode 100644 m4/virt-archive.m4
> 
> diff --git a/configure.ac b/configure.ac
> index 03463b0..4018b49 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -250,6 +250,7 @@ LIBVIRT_CHECK_SANLOCK
>  LIBVIRT_CHECK_SASL
>  LIBVIRT_CHECK_SELINUX
>  LIBVIRT_CHECK_SSH2
> +LIBVIRT_CHECK_LIBARCHIVE

This does all the pkg-config checks for libarchive...

> @@ -1603,8 +1604,6 @@ fi
>  AC_SUBST([LIBPCAP_CFLAGS])
>  AC_SUBST([LIBPCAP_LIBS])
>  
> -
> -

Avoid changing unrelated lines please

>  dnl
>  dnl Checks for the UML driver
>  dnl
> @@ -2097,6 +2096,13 @@ AM_CONDITIONAL([WITH_STORAGE_DISK], [test 
> "$with_storage_disk" = "yes"])
>  AC_SUBST([LIBPARTED_CFLAGS])
>  AC_SUBST([LIBPARTED_LIBS])
>  
> +if test "$with_libarchive" = "yes" || test "$with_libarchive" = "check"; then
> +   PKG_CHECK_MODULES([LIBARCHIVE], [libarchive >= 3.1.2], [], 
> [LIBARCHIVE_FOUND=no])
> +fi
> +AC_SUBST([LIBARCHIVE_CFLAGS])
> +AC_SUBST([LIBARCHIVE_LIBS])

...this is redundant given LIBVIRT_CHECK_LIBARCHIVE earlier.

>  if test "$with_storage_mpath" = "yes" ||
> test "$with_storage_disk" = "yes"; then
> DEVMAPPER_CFLAGS=
> diff --git a/include/libvirt/libvirt-stream.h 
> b/include/libvirt/libvirt-stream.h
> index 831640d..ac48fba 100644
> --- a/include/libvirt/libvirt-stream.h
> +++ b/include/libvirt/libvirt-stream.h
> @@ -35,6 +35,12 @@ typedef enum {
>  
>  virStreamPtr virStreamNew(virConnectPtr conn,
>unsigned int flags);
> +virStreamPtr virStreamNewLz4(virConnectPtr conn,
> +  unsigned int flags);
> +virStreamPtr virStreamNewGzip(virConnectPtr conn,
> +  unsigned int flags);
> +virStreamPtr virStreamNewXz(virConnectPtr conn,
> +  unsigned int flags);

If we want to expose this in the public API, then we'd really
want an enum for each archive format, so we don't need to keep
adding new APIs for each format.

> diff --git a/src/libvirt-stream.c b/src/libvirt-stream.c
> index c16f586..40702dc 100644
> --- a/src/libvirt-stream.c
> +++ b/src/libvirt-stream.c
> @@ -69,6 +73,56 @@ virStreamNew(virConnectPtr conn,
>  return st;
>  }
>  
> +virStreamPtr
> +virStreamNewLz4(virConnectPtr conn,
> +unsigned int flags)
> +{
> +virStreamPtr st;
> +
> +st = virStreamNew(conn, flags);
> +if (st != NULL) {
> +struct virFDStreamData *fdst = st->privateData;

This is not valid. You can't assume anything about what
st->privateData is pointing to - it can be anything that
a virt driver wants to use. For example with the remote
driver it can point to a virNetClientStreamPtr instead
instead of virFDStreamData.

> +fdst->archive = archive_write_new();
> +archive_write_add_filter(fdst->archive, ARCHIVE_FILTER_LZ4);
> +}
> +
> +return st;
> +}

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

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

[libvirt] [PATCH 4/5] lxcDomainCreateXMLWithFiles: Don't remove persistent domains on error

[Michal Privoznik]

Similarly to one of previous commits, don't lose a persistent
domain on some error.

Signed-off-by: Michal Privoznik 
---
 src/lxc/lxc_driver.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
index a9f0005..e5e6c5a 100644
--- a/src/lxc/lxc_driver.c
+++ b/src/lxc/lxc_driver.c
@@ -1239,8 +1239,10 @@ lxcDomainCreateXMLWithFiles(virConnectPtr conn,
(flags & VIR_DOMAIN_START_AUTODESTROY),
VIR_DOMAIN_RUNNING_BOOTED) < 0) {
 virDomainAuditStart(vm, "booted", false);
-virDomainObjListRemove(driver->domains, vm);
-vm = NULL;
+if (!vm->persistent) {
+virDomainObjListRemove(driver->domains, vm);
+vm = NULL;
+}
 goto cleanup;
 }
 
-- 
2.4.9

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

[libvirt] [PATCH 5/5] lxcDomainCreateXMLWithFiles: Make domain definition transient

[Michal Privoznik]

Yet again, the same error that was in qemu driver is in lxc
driver too.

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

diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
index e5e6c5a..b408be0 100644
--- a/src/lxc/lxc_driver.c
+++ b/src/lxc/lxc_driver.c
@@ -1229,6 +1229,7 @@ lxcDomainCreateXMLWithFiles(virConnectPtr conn,
 
 if (!(vm = virDomainObjListAdd(driver->domains, def,
driver->xmlopt,
+   VIR_DOMAIN_OBJ_LIST_ADD_LIVE |
VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE,
NULL)))
 goto cleanup;
-- 
2.4.9

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

[libvirt] [PATCH 1/5] qemuDomainCreateXML: Don't remove persistent domains on error

[Michal Privoznik]

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

Okay, so we allow users to 'virsh create' an already existing
domain, providing completely different XML than the one stored in
Libvirt. Well, as long as name and UUID matches. However, the
code that handles errors unconditionally removes the domain that
failed to start even though the domain might have been
persistent. Fortunately, the domain is removed just from the
internal list of domains and the config file is kept around.

Steps to reproduce:

1) virsh dumpxml $dom > /tmp/dom.xml
2) change XML so that it is still parse-able but won't boot, e.g.
change guest agent path to /foo/bar
3) virsh create /tmp/dom.xml
4) virsh dumpxml $dom
5) Observe "No such domain" error

Signed-off-by: Michal Privoznik 
---
 src/qemu/qemu_driver.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 2387cf3..30d2d98 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -1752,7 +1752,8 @@ static virDomainPtr qemuDomainCreateXML(virConnectPtr 
conn,
 def = NULL;
 
 if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) {
-qemuDomainRemoveInactive(driver, vm);
+if (!vm->persistent)
+qemuDomainRemoveInactive(driver, vm);
 goto cleanup;
 }
 
@@ -1762,7 +1763,8 @@ static virDomainPtr qemuDomainCreateXML(virConnectPtr 
conn,
  start_flags) < 0) {
 virDomainAuditStart(vm, "booted", false);
 qemuDomainObjEndJob(driver, vm);
-qemuDomainRemoveInactive(driver, vm);
+if (!vm->persistent)
+qemuDomainRemoveInactive(driver, vm);
 goto cleanup;
 }
 
-- 
2.4.9

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

[libvirt] [PATCH 0/5] Couple of 'virsh create' fixes

[Michal Privoznik]

*** BLURB HERE ***

Michal Privoznik (5):
  qemuDomainCreateXML: Don't remove persistent domains on error
  qemuDomainCreateXML: Make domain definition transient
  qemu: Move vm->persistent check into qemuDomainRemoveInactive
  lxcDomainCreateXMLWithFiles: Don't remove persistent domains on error
  lxcDomainCreateXMLWithFiles: Make domain definition transient

 src/lxc/lxc_driver.c  |  7 +--
 src/qemu/qemu_domain.c|  9 -
 src/qemu/qemu_driver.c| 37 +++--
 src/qemu/qemu_migration.c | 14 +-
 src/qemu/qemu_process.c   | 12 
 5 files changed, 37 insertions(+), 42 deletions(-)

-- 
2.4.9

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

[libvirt] [PATCH 2/5] qemuDomainCreateXML: Make domain definition transient

[Michal Privoznik]

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

So, you want to create a domain from XML. The domain already
exists in libvirt's database of domains. It's okay, because name
and UUID matches. However, on domain startup, internal
representation of the domain is overwritten with your XML even
though we claim that the XML you've provided is a transient one.
Le sigh.

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

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 30d2d98..2a4b026 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -1745,6 +1745,7 @@ static virDomainPtr qemuDomainCreateXML(virConnectPtr 
conn,
 
 if (!(vm = virDomainObjListAdd(driver->domains, def,
driver->xmlopt,
+   VIR_DOMAIN_OBJ_LIST_ADD_LIVE |
VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE,
NULL)))
 goto cleanup;
-- 
2.4.9

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

[libvirt] [PATCH 3/5] qemu: Move vm->persistent check into qemuDomainRemoveInactive

[Michal Privoznik]

So far we have the following pattern occurring over and over
again:

  if (!vm->persistent)
  qemuDomainRemoveInactive(driver, vm);

It's safe to put the check into the function and save some LoC.

Signed-off-by: Michal Privoznik 
---
 src/qemu/qemu_domain.c|  9 -
 src/qemu/qemu_driver.c| 42 --
 src/qemu/qemu_migration.c | 14 +-
 src/qemu/qemu_process.c   | 12 
 4 files changed, 33 insertions(+), 44 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 7d92f3a..367d662 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -2632,7 +2632,14 @@ qemuDomainRemoveInactive(virQEMUDriverPtr driver,
 {
 bool haveJob = true;
 char *snapDir;
-virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
+virQEMUDriverConfigPtr cfg;
+
+if (vm->persistent) {
+/* Short-circuit, we don't want to remove a persistent domain */
+return;
+}
+
+cfg = virQEMUDriverGetConfig(driver);
 
 if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
 haveJob = false;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 2a4b026..3532973 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -1753,8 +1753,7 @@ static virDomainPtr qemuDomainCreateXML(virConnectPtr 
conn,
 def = NULL;
 
 if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) {
-if (!vm->persistent)
-qemuDomainRemoveInactive(driver, vm);
+qemuDomainRemoveInactive(driver, vm);
 goto cleanup;
 }
 
@@ -1764,8 +1763,7 @@ static virDomainPtr qemuDomainCreateXML(virConnectPtr 
conn,
  start_flags) < 0) {
 virDomainAuditStart(vm, "booted", false);
 qemuDomainObjEndJob(driver, vm);
-if (!vm->persistent)
-qemuDomainRemoveInactive(driver, vm);
+qemuDomainRemoveInactive(driver, vm);
 goto cleanup;
 }
 
@@ -2250,7 +2248,7 @@ qemuDomainDestroyFlags(virDomainPtr dom,
 ret = 0;
  endjob:
 qemuDomainObjEndJob(driver, vm);
-if (ret == 0 && !vm->persistent)
+if (ret == 0)
 qemuDomainRemoveInactive(driver, vm);
 
  cleanup:
@@ -3273,7 +3271,7 @@ qemuDomainSaveInternal(virQEMUDriverPtr driver, 
virDomainPtr dom,
 }
 }
 qemuDomainObjEndAsyncJob(driver, vm);
-if (ret == 0 && !vm->persistent)
+if (ret == 0)
 qemuDomainRemoveInactive(driver, vm);
 
  cleanup:
@@ -3774,7 +3772,7 @@ qemuDomainCoreDumpWithFormat(virDomainPtr dom,
 }
 
 qemuDomainObjEndAsyncJob(driver, vm);
-if (ret == 0 && flags & VIR_DUMP_CRASH && !vm->persistent)
+if (ret == 0 && flags & VIR_DUMP_CRASH)
 qemuDomainRemoveInactive(driver, vm);
 
  cleanup:
@@ -4054,8 +4052,7 @@ processGuestPanicEvent(virQEMUDriverPtr driver,
 
 virDomainAuditStop(vm, "destroyed");
 
-if (!vm->persistent)
-qemuDomainRemoveInactive(driver, vm);
+qemuDomainRemoveInactive(driver, vm);
 break;
 
 case VIR_DOMAIN_LIFECYCLE_CRASH_COREDUMP_RESTART:
@@ -6831,7 +6828,7 @@ qemuDomainRestoreFlags(virConnectPtr conn,
 VIR_WARN("Failed to close %s", path);
 
 qemuDomainObjEndJob(driver, vm);
-if (ret < 0 && !vm->persistent)
+if (ret < 0)
 qemuDomainRemoveInactive(driver, vm);
 
  cleanup:
@@ -7526,6 +7523,7 @@ static virDomainPtr 
qemuDomainDefineXMLFlags(virConnectPtr conn, const char *xml
 } else {
 /* Brand new domain. Remove it */
 VIR_INFO("Deleting domain '%s'", vm->def->name);
+vm->persistent = 0;
 qemuDomainRemoveInactive(driver, vm);
 }
 goto cleanup;
@@ -7651,11 +7649,9 @@ qemuDomainUndefineFlags(virDomainPtr dom,
  * domainDestroy and domainShutdown will take care of removing the
  * domain obj from the hash table.
  */
-if (virDomainObjIsActive(vm)) {
-vm->persistent = 0;
-} else {
+vm->persistent = 0;
+if (!virDomainObjIsActive(vm))
 qemuDomainRemoveInactive(driver, vm);
-}
 
 ret = 0;
 
@@ -15550,12 +15546,9 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr 
snapshot,
 }
 
 if (qemuDomainSnapshotRevertInactive(driver, vm, snap) < 0) {
-if (!vm->persistent) {
-qemuDomainObjEndJob(driver, vm);
-qemuDomainRemoveInactive(driver, vm);
-goto cleanup;
-}
-goto endjob;
+qemuDomainObjEndJob(driver, vm);
+qemuDomainRemoveInactive(driver, vm);
+goto cleanup;
 }
 if (config)
 virDomainObjAssignDef(vm, config, false, NULL);
@@ -15575,12 +15568,9 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr 
snapshot,
   start_flags);
 virDomainAuditStart(vm, "from-snapshot", rc >= 0);
 if (rc < 0) {
-if