Re: [libvirt] [libvirt-php PATCH 29/29] Bump version to 0.5.2

[Neal Gompa]

On Thu, Apr 14, 2016 at 12:30 PM, Michal Privoznik  wrote:
>
> I will push this one only after some testing of the patches I've just
> pushed. It has been a big change so I rather give our users chance to
> test it before calling it release.
>
> Michal

If it helps any, since this has all made it up to the libvirt-php git,
I've tested it on PHP 5 and PHP 7 with some applications with good
success, so it looks good to me. I'd definitely appreciate a libvirt
0.5.2 release. I'm also the php-libvirt package maintainer in Fedora
and EPEL, and I'd like to update the package to the latest stuff once
a release is made. And Remi can add it to his repository to provide
SCL versions of libvirt-php across the different PHP versions he
supports.

Ubuntu is already shipping the snapshot[1] for the 16.04 LTS release
coming out on Wednesday.

A formal release would just make it easier for everyone else to update, as well.

[1]: http://packages.ubuntu.com/xenial/php-libvirt-php

-- 
真実はいつも一つ！/ Always, there's only one truth!

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

Re: [libvirt] [PATCH] network: Add support for dhcp-range lease time in the network XML configuration format and dnsmasq

[Laine Stump]

On 04/18/2016 06:52 PM, Cole Robinson wrote:

On 04/15/2016 08:18 PM, Alberto Ruiz wrote:

 From 112f61ec5cfdc39f7a157825c4209f7bae34c483 Mon Sep 17 00:00:00 2001
From: Alberto Ruiz 
Date: Wed, 13 Apr 2016 17:00:45 +0100
Subject: [PATCH] network: Add support for dhcp-range lease time in the network
  XML configuration format and dnsmasq


Also mention the bug in the commit message, just link it like

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

Needs documentation but that will be dependent on what the final patch looks
like, so fine to skip for now.

The main questions are:

1) is the XML format fine? . lease sounds kinda
non-specific to me, maybe leasetime or leaseTime.

2) what to use for the input format? right now it's just string passthrough to
dnsmasq, which takes a format like XX[s|m|h|d|w], or 'infinite'. Accepting
that format kind of sticks us with that for all time, which probably isn't a
good precedent. the easy way would probably be to just say the value needs to
be in minutes, and maybe -1 == infinite. But that will take a bit more code to
adapt that value to the dnsmasq format.


Yeah, you should never just read an opaque string and pass it directly 
through to dnsmasq. Instead, parse an integer (and whatever scaling info 
- hours / minutes / seconds - I know we do that for bytes vs kbytes vs 
KiB etc, and if we don't already have the same thing for times 
somewhere, we should), scale it, check the range for some amount of 
sanity, and convert that scaled integer into whatever dnsmasq wants when 
building the commandline.




CC laining for his thoughts


Aside from the missing documentation that you pointed out (and that is 
just a pain to put in until the exact placement in the XML is figured 
out anyway), my main sticking point is having the lease time put as an 
attribute to each range. That just seems odd. I know that dnsmasq 
allows for specifying a lease time per range, but is that the case for 
other dhcp server implementations? (yeah, I know we don't support any 
other now, but someday we might :-). And even if dnsmasq *allows* it, 
unless you're using their tagging to put certain clients into certain IP 
ranges, there's no practical value in having a different lease time for 
each range. Maybe it should be an attribute of the  element (or, 
to allow for different scaling, a subelement); every range on the 
dnsmasq commandline would just get the same lease time. Something like this:


   
 3600
 
 
   

If the need for per-range leasetime arose later, that could be added as 
a sub-element to  that would override the leasetime directly 
under .


(It's been at least 15 years since I used ISC's dhcpd, but I glanced at 
the config file manpage just now and it looks like it's possible to have 
a single "max-lease" that applies to all "pools" (their name for ranges) 
or to specify a separate max-lease for each pool. I admit I skipped 98% 
of the contents though :-)).


In practice, I doubt there will be much difference between what you 
proposed and what I've suggested - probably 100% of the libvirt virtual 
networks in existence have only a single range anyway. I *think* 
splitting it out from the range could prevent us from being painted into 
a corner though.


Aside from all that, thanks for taking the time to code this up!




And one tiny comment below:


diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
index 4fb2e2a..449c9ed 100644
--- a/src/conf/network_conf.c
+++ b/src/conf/network_conf.c
@@ -313,6 +313,10 @@ static void
  virNetworkIpDefClear(virNetworkIpDefPtr def)
  {
  VIR_FREE(def->family);
+
+while (def->nranges)
+VIR_FREE(def->ranges[--def->nranges].lease);
+
  VIR_FREE(def->ranges);
  
  while (def->nhosts)

@@ -855,7 +859,6 @@ int virNetworkIpDefNetmask(const virNetworkIpDef *def,
  
VIR_SOCKET_ADDR_FAMILY(>address));
  }
  
-

stray whitespace change here

- Cole



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

[libvirt] [PATCH] qemu: process: comment on min_guarantee validation

[Cole Robinson]

Explain why we check it at process startup time, and not parse time
where most other XML validation checks are performed
---
 src/qemu/qemu_process.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index c087300..628b4b6 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -4550,6 +4550,8 @@ qemuProcessStartValidate(virQEMUDriverPtr driver,
 virDomainDefCheckDuplicateDiskInfo(vm->def) < 0)
 return -1;
 
+/* Previously we silently accepted this parameter; we can't reject
+   it at parse time without breaking those configs, so check it here */
 if (vm->def->mem.min_guarantee) {
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("Parameter 'min_guarantee' "
-- 
2.7.3

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

[libvirt] [PATCH] qemu: command: drop redundant min_guarantee check

[Cole Robinson]

We already reject a VM with min_guarantee early in the VM startup
in qemuProcessStartValidate
---
 src/qemu/qemu_command.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 26c19ff..2fb967a 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -9143,7 +9143,6 @@ qemuBuildCommandLineValidate(virQEMUDriverPtr driver,
 
 if (virMemoryLimitIsSet(def->mem.hard_limit) ||
 virMemoryLimitIsSet(def->mem.soft_limit) ||
-def->mem.min_guarantee ||
 virMemoryLimitIsSet(def->mem.swap_hard_limit)) {
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("Memory tuning is not available in session 
mode"));
-- 
2.7.3

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

[libvirt] [PATCH] Explicitly error on uri=qemu://system

[Cole Robinson]

It's a fairly common error that a user tries to connect to a URI
like qemu://system or qemu://session (missing a slash). This errors
like:

$ virsh --connect qemu://session
error: failed to connect to the hypervisor
error: Unable to resolve address 'session' service '16514': No address 
associated with hostname

If you already know that the standard qemu URI has 3 slashes, that
error will make it obvious enough. But new user's may not get it.
There's even a RHEL support page explicitly mentioning it!:

https://access.redhat.com/documentation/en-US/Red_Hat_Enterprise_Linux/7/html/Virtualization_Deployment_and_Administration_Guide/sect-Troubleshooting-Common_libvirt_errors_and_troubleshooting.html

Catch this error early in libvirt.c virConnectOpen for qemu (and vbox
which has similar rules

https://bugzilla.redhat.com/show_bug.cgi?id=1038304
---
 src/libvirt.c | 32 
 1 file changed, 32 insertions(+)

diff --git a/src/libvirt.c b/src/libvirt.c
index a21d00e..7607ae3 100644
--- a/src/libvirt.c
+++ b/src/libvirt.c
@@ -928,6 +928,33 @@ virConnectGetDefaultURI(virConfPtr conf,
 }
 
 
+/*
+ * Check to see if an invalid URI like qemu://system (missing /) was passed,
+ * offer the suggested fix.
+ */
+static int
+check_uri_missing_slash(const char *uristr, virURIPtr uri)
+{
+/* These drivers _only_ accepts URIs with a 'path' element */
+if (STRNEQ(uri->scheme, "qemu") &&
+STRNEQ(uri->scheme, "vbox"))
+return 0;
+
+if (uri->path != NULL)
+return 0;
+
+if (STREQ(uri->server, "session") ||
+STREQ(uri->server, "system")) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("invalid URI %s (maybe you want %s:///%s)"),
+   uristr, uri->scheme, uri->server);
+return -1;
+}
+
+return 0;
+}
+
+
 static virConnectPtr
 do_open(const char *name,
 virConnectAuthPtr auth,
@@ -995,6 +1022,11 @@ do_open(const char *name,
   NULLSTR(ret->uri->user), ret->uri->port,
   NULLSTR(ret->uri->path));
 
+if (check_uri_missing_slash(alias ? alias : name, ret->uri) < 0) {
+VIR_FREE(alias);
+goto failed;
+}
+
 VIR_FREE(alias);
 } else {
 VIR_DEBUG("no name, allowing driver auto-select");
-- 
2.7.3

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

Re: [libvirt] [PATCH 0/2] prevent duplicate entries in network device pools

[Cole Robinson]

On 04/18/2016 03:12 PM, Laine Stump wrote:
> This is a simple bugfix, but proper testing required the ability to
> test for parse failures in networkxml2xmltest, which wasn't yet an
> option, so I added a separate prerequisite patch to do that (in case
> someone wants to backport the new testing option without the bugfix).
> 
> Laine Stump (2):
>   test: enable testing for expected parse errors in network XML
>   network: prevent duplicate entries in network device pools
> 
>  src/conf/network_conf.c  | 33 --
>  tests/networkxml2xmlin/hostdev-duplicate.xml | 11 
>  tests/networkxml2xmlin/passthrough-duplicate.xml | 10 +++
>  tests/networkxml2xmltest.c   | 77 
> ++--
>  4 files changed, 108 insertions(+), 23 deletions(-)
>  create mode 100644 tests/networkxml2xmlin/hostdev-duplicate.xml
>  create mode 100644 tests/networkxml2xmlin/passthrough-duplicate.xml
> 

ACK

- Cole

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

Re: [libvirt] [PATCH] network: Add support for dhcp-range lease time in the network XML configuration format and dnsmasq

[Cole Robinson]

On 04/15/2016 08:18 PM, Alberto Ruiz wrote:
> From 112f61ec5cfdc39f7a157825c4209f7bae34c483 Mon Sep 17 00:00:00 2001
> From: Alberto Ruiz 
> Date: Wed, 13 Apr 2016 17:00:45 +0100
> Subject: [PATCH] network: Add support for dhcp-range lease time in the network
>  XML configuration format and dnsmasq
> 

Also mention the bug in the commit message, just link it like

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

Needs documentation but that will be dependent on what the final patch looks
like, so fine to skip for now.

The main questions are:

1) is the XML format fine? . lease sounds kinda
non-specific to me, maybe leasetime or leaseTime.

2) what to use for the input format? right now it's just string passthrough to
dnsmasq, which takes a format like XX[s|m|h|d|w], or 'infinite'. Accepting
that format kind of sticks us with that for all time, which probably isn't a
good precedent. the easy way would probably be to just say the value needs to
be in minutes, and maybe -1 == infinite. But that will take a bit more code to
adapt that value to the dnsmasq format.

CC laining for his thoughts

And one tiny comment below:

> diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
> index 4fb2e2a..449c9ed 100644
> --- a/src/conf/network_conf.c
> +++ b/src/conf/network_conf.c
> @@ -313,6 +313,10 @@ static void
>  virNetworkIpDefClear(virNetworkIpDefPtr def)
>  {
>  VIR_FREE(def->family);
> +
> +while (def->nranges)
> +VIR_FREE(def->ranges[--def->nranges].lease);
> +
>  VIR_FREE(def->ranges);
>  
>  while (def->nhosts)
> @@ -855,7 +859,6 @@ int virNetworkIpDefNetmask(const virNetworkIpDef *def,
>  
> VIR_SOCKET_ADDR_FAMILY(>address));
>  }
>  
> -

stray whitespace change here

- Cole

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

Re: [libvirt] [PATCH 15/10] secret: Change virSecretDef variable names

[Cole Robinson]

On 03/08/2016 12:39 PM, John Ferlan wrote:
> Change 'ephemeral' to 'isephemeral' and 'private' to 'isprivate' since
> both are bools.
> 
> Signed-off-by: John Ferlan 
> ---
>  src/conf/secret_conf.c| 26 +-
>  src/conf/secret_conf.h|  4 ++--
>  src/secret/secret_driver.c| 10 +-
>  src/storage/storage_backend.c |  4 ++--
>  4 files changed, 22 insertions(+), 22 deletions(-)

ACK

- Cole

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

Re: [libvirt] [PATCH 14/10] secret: Introduce virSecretObjGetValue and virSecretObjGetValueSize

[Cole Robinson]

On 03/08/2016 12:39 PM, John Ferlan wrote:
> Introduce the final accessor's to _virSecretObject data and move the
> structure from secret_conf.h to secret_conf.c
> 
> The virSecretObjSetValue logic will handle setting both the secret
> value and the value_size. Some slight adjustments to the error path
> over what was in secretSetValue were made.
> 
> Additionally, a slight logic change in secretGetValue where we'll
> check for the internalFlags and error out before checking for
> and erroring out for a NULL secret->value. That way, it won't be
> obvious to anyone that the secret value wasn't set rather they'll
> just know they cannot get the secret value since it's private.
> 
> Signed-off-by: John Ferlan 
> ---
>  src/conf/secret_conf.c | 84 
> ++
>  src/conf/secret_conf.h | 17 +-
>  src/libvirt_private.syms   |  4 +++
>  src/secret/secret_driver.c | 50 ---
>  4 files changed, 104 insertions(+), 51 deletions(-)
> 
> diff --git a/src/conf/secret_conf.c b/src/conf/secret_conf.c
> index 0410328..4d0cb9c 100644
> --- a/src/conf/secret_conf.c
> +++ b/src/conf/secret_conf.c

...

> +
> +
> +int
> +virSecretObjSetValue(virSecretObjPtr secret,
> + const unsigned char *value,
> + size_t value_size)
> +{

...

> +
> +return  0;

Weird spacing here

ACK otherwise

- Cole

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

Re: [libvirt] [PATCH 13/10] secret: Introduce virSecretObjGetDef and virSecretObjSetDef

[Cole Robinson]

On 03/08/2016 12:39 PM, John Ferlan wrote:
> Introduce fetch and set accessor to the secretObj->def field for usage
> by the driver to avoid the driver needing to know the format of virSecretObj
> 
> Signed-off-by: John Ferlan 
> ---
>  src/conf/secret_conf.c | 15 +
>  src/conf/secret_conf.h |  4 
>  src/libvirt_private.syms   |  2 ++
>  src/secret/secret_driver.c | 54 
> --
>  4 files changed, 54 insertions(+), 21 deletions(-)

ACK

- Cole

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

Re: [libvirt] [PATCH 12/10] secret: Introduce virSecretObjSaveConfig and virSecretObjSaveData

[Cole Robinson]

On 03/08/2016 12:36 PM, John Ferlan wrote:
> Move and rename the secretRewriteFile, secretSaveDef, and secretSaveValue
> from secret_driver to secret_conf
> 
> Need to make some slight adjustments since the secretSave* functions
> called secretEnsureDirectory, but otherwise mostly just a move of code.
> 
> Signed-off-by: John Ferlan 
> ---
>  src/conf/secret_conf.c | 69 +++
>  src/conf/secret_conf.h |  4 +++
>  src/libvirt_private.syms   |  2 ++
>  src/secret/secret_driver.c | 90 
> +++---
>  4 files changed, 87 insertions(+), 78 deletions(-)

ACK

Though there should probably be explicit virfile.c support for a generic
'rewrite file with this passed string', rather than requiring a callback.
src/network/leaseshelper.c already has something similar

- Cole

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

Re: [libvirt] [PATCH 11/10] secret: Introduce virSecretObjDeleteConfig and virSecretObjDeleteData

[Cole Robinson]

On 03/08/2016 12:35 PM, John Ferlan wrote:
> Move and rename secretDeleteSaved from secret_driver into secret_conf and
> split it up into two parts since there is error path code that looks to
> just delete the secret data file
> 
> Signed-off-by: John Ferlan 
> ---
>  src/conf/secret_conf.c | 21 +
>  src/conf/secret_conf.h |  4 
>  src/libvirt_private.syms   |  2 ++
>  src/secret/secret_driver.c | 22 ++
>  4 files changed, 33 insertions(+), 16 deletions(-)
> 
> diff --git a/src/conf/secret_conf.c b/src/conf/secret_conf.c
> index f6eee6f..52f78bd 100644
> --- a/src/conf/secret_conf.c
> +++ b/src/conf/secret_conf.c
> @@ -685,6 +685,27 @@ virSecretObjListGetUUIDs(virSecretObjListPtr secrets,
>  }
>  
>  
> +int
> +virSecretObjDeleteConfig(virSecretObjPtr secret)
> +{
> +/* When the XML is missing, we'll still allow the attempt to
> + * delete the secret data. Without a configFile, the secret
> +   won't be loaded again, so we have succeeded already. */

This comment seems weirdly placed now. If it's kept at all it should be placed
at the ObjDeleteData call sites. Or rework it as a comment in ObjDeleteData to
explain why we don't care about failure in this case.

> +if (!secret->def->ephemeral &&
> +unlink(secret->configFile) < 0 && errno != ENOENT)
> +return -1;
> +

This should report have a virReportSystemError call. The original code doesn't
have one, but that's a bug

Minor stuff though so ACK in general, I don't care if you fix before pushing
but not sure if there's a formal policy on that

- Cole

> +return 0;
> +}
> +
> +
> +void
> +virSecretObjDeleteData(virSecretObjPtr secret)
> +{
> +(void)unlink(secret->base64File);
> +}
> +
> +
>  void
>  virSecretDefFree(virSecretDefPtr def)
>  {
> diff --git a/src/conf/secret_conf.h b/src/conf/secret_conf.h
> index d3bd10c..e2f69b5 100644
> --- a/src/conf/secret_conf.h
> +++ b/src/conf/secret_conf.h
> @@ -114,6 +114,10 @@ int virSecretObjListGetUUIDs(virSecretObjListPtr secrets,
>   virSecretObjListACLFilter filter,
>   virConnectPtr conn);
>  
> +int virSecretObjDeleteConfig(virSecretObjPtr secret);
> +
> +void virSecretObjDeleteData(virSecretObjPtr secret);
> +
>  void virSecretDefFree(virSecretDefPtr def);
>  virSecretDefPtr virSecretDefParseString(const char *xml);
>  virSecretDefPtr virSecretDefParseFile(const char *filename);
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index cbc36de..2437b0b 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -787,6 +787,8 @@ virSecretDefFree;
>  virSecretDefParseFile;
>  virSecretDefParseString;
>  virSecretLoadAllConfigs;
> +virSecretObjDeleteConfig;
> +virSecretObjDeleteData;
>  virSecretObjEndAPI;
>  virSecretObjListAdd;
>  virSecretObjListExport;
> diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c
> index b8d9ecc..e4315f3 100644
> --- a/src/secret/secret_driver.c
> +++ b/src/secret/secret_driver.c
> @@ -175,19 +175,6 @@ secretSaveValue(const virSecretObj *secret)
>  return ret;
>  }
>  
> -static int
> -secretDeleteSaved(const virSecretObj *secret)
> -{
> -if (unlink(secret->configFile) < 0 && errno != ENOENT)
> -return -1;
> -
> -/* When the XML is missing, the rest may waste disk space, but the secret
> -   won't be loaded again, so we have succeeded already. */
> -(void)unlink(secret->base64File);
> -
> -return 0;
> -}
> -
>  /* Driver functions */
>  
>  static int
> @@ -325,8 +312,10 @@ secretDefineXML(virConnectPtr conn,
>  goto restore_backup;
>  }
>  } else if (backup && !backup->ephemeral) {
> -if (secretDeleteSaved(secret) < 0)
> +if (virSecretObjDeleteConfig(secret) < 0)
>  goto restore_backup;
> +
> +virSecretObjDeleteData(secret);
>  }
>  /* Saved successfully - drop old values */
>  new_attrs = NULL;
> @@ -489,10 +478,11 @@ secretUndefine(virSecretPtr obj)
>  if (virSecretUndefineEnsureACL(obj->conn, secret->def) < 0)
>  goto cleanup;
>  
> -if (!secret->def->ephemeral &&
> -secretDeleteSaved(secret) < 0)
> +if (virSecretObjDeleteConfig(secret) < 0)
>  goto cleanup;
>  
> +virSecretObjDeleteData(secret);
> +
>  virSecretObjListRemove(driver->secrets, secret);
>  
>  ret = 0;
> 

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

Re: [libvirt] [PATCH 10/10] secret: Move and rename secretLoadAllConfigs

[Cole Robinson]

On 03/02/2016 01:55 PM, John Ferlan wrote:
> Move to secret_conf.c and rename to virSecretLoadAllConfigs. Also includes
> moving/renaming the supporting virSecretLoad, virSecretLoadValue, and
> virSecretLoadValidateUUID.
> 
> Signed-off-by: John Ferlan 
> ---
>  src/conf/secret_conf.c | 175 
> +
>  src/conf/secret_conf.h |   3 +
>  src/libvirt_private.syms   |   1 +
>  src/secret/secret_driver.c | 174 +---
>  4 files changed, 181 insertions(+), 172 deletions(-)

ACK, mirrors network_conf.c layout.

(though honestly I'd rather we have separate files for XML handling and object
handling... the existing conf.c files are too large anyways. I put an entry on
the LibvirtFirstBugs wiki page about that type of code reorg though it's
probably over an initial contributors head)

- Cole

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

Re: [libvirt] [PATCH 09/10] secret: Use the hashed virSecretObjList

[Cole Robinson]

On 03/02/2016 01:55 PM, John Ferlan wrote:
> This patch replaces most of the guts of secret_driver.c with recently
> added secret_conf.c APIs in order manage secret lists and objects
> using the hashed virSecretObjList* lookup API's.
> 
> Signed-off-by: John Ferlan 
> ---
>  src/conf/secret_conf.h |   3 +-
>  src/libvirt_private.syms   |   9 +
>  src/secret/secret_driver.c | 437 
> ++---
>  3 files changed, 61 insertions(+), 388 deletions(-)
> 

ACK, mirrors the network driver usage (where similar), and all the custom
logic beats seem to be handled in the previous patches

- Cole

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

Re: [libvirt] RFC: virtio-rng and /dev/urandom

[H. Peter Anvin]

On April 18, 2016 4:26:24 AM PDT, "Daniel P. Berrange"  
wrote:
>On Mon, Apr 18, 2016 at 01:07:40PM +0200, Hubert Kario wrote:
>> On Monday 18 April 2016 02:46:19 H. Peter Anvin wrote:
>> > Another thing that really needs to be addressed, but is a separate
>> > issue: invalidating and reseeding the entropy pool after a snapshot
>> > event.
>> 
>> definitely agreed
>> 
>> though just reseeding would be sufficient - the goal is to make the 
>> output unpredictable and unique between multiple machines starting
>from 
>> the same snapshot, feeding enough random data to make the entropy
>pool 
>> unique again is sufficient to achieve that
>
>If you're spawning multiple machines from the same base snapshot,
>the seeding of RNG is just one of many many things that need
>dealing with. eg new /etc/machine-id, new ssh host keys, changing
>MAC address of NICs with corresponding guest config file changes,
>many other application specific identifiers / keys intended to
>be unique per machine.  As such, libvirt explicitly tries to
>prevent you spawning multiple machines from the same snapshot.
>
>That all said, Microsoft HyperV has defined a concept of a
>"Virtual Machine Generation ID" and specified various hypervisor
>operations which should result in this value changing[1]. For example
>restoring from a snapshot should always change the genid, as would
>restoring from backup, or cloned from another image, or failed over
>during disaster recovery.
>
>This vm genid is exposed to the guest via ACPI and there's an
>notification whenever it changes.
>
>There are patches for QEMU[2] to support this feature in a manner that
>is compatible with the hyperv spec, but they are sadly still not
>merged :-(
>
>So it would be possible for the Linux kernel to re-initialize its
>RNG after snapshot by hooking into the vm-genid ACPI notification.
>
>
>Regards,
>Daniel
>
>[1]
>https://lists.nongnu.org/archive/html/qemu-devel/2014-10/msg00489.html
>[2] https://lists.gnu.org/archive/html/qemu-devel/2016-01/msg05599.html

There are multiple machines, and there are snapshots restored.
-- 
Sent from my Android device with K-9 Mail. Please excuse brevity and formatting.

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

Re: [libvirt] [PATCH 08/10] secret: Introduce virSecretObjListGetUUIDs

[Cole Robinson]

On 03/02/2016 01:55 PM, John Ferlan wrote:
> Add function to return counted listed of uuids to from the hashed secrets
> object list. This will replace the guts of secretConnectListSecrets.
> 
> Signed-off-by: John Ferlan 
> ---
>  src/conf/secret_conf.c | 53 
> +-
>  src/conf/secret_conf.h |  6 ++
>  2 files changed, 58 insertions(+), 1 deletion(-)
> 

ACK, mirrors virNetworkObjListGetNames and looks sensible on its own.

- Cole

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

[libvirt] [PATCH 0/2] prevent duplicate entries in network device pools

[Laine Stump]

This is a simple bugfix, but proper testing required the ability to
test for parse failures in networkxml2xmltest, which wasn't yet an
option, so I added a separate prerequisite patch to do that (in case
someone wants to backport the new testing option without the bugfix).

Laine Stump (2):
  test: enable testing for expected parse errors in network XML
  network: prevent duplicate entries in network device pools

 src/conf/network_conf.c  | 33 --
 tests/networkxml2xmlin/hostdev-duplicate.xml | 11 
 tests/networkxml2xmlin/passthrough-duplicate.xml | 10 +++
 tests/networkxml2xmltest.c   | 77 ++--
 4 files changed, 108 insertions(+), 23 deletions(-)
 create mode 100644 tests/networkxml2xmlin/hostdev-duplicate.xml
 create mode 100644 tests/networkxml2xmlin/passthrough-duplicate.xml

-- 
2.5.5

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

[libvirt] [PATCH 2/2] network: prevent duplicate entries in network device pools

[Laine Stump]

Prior to this patch we didn't make any attempt to prevent two entries
in the array of interfaces/PCI devices from pointing to the same
device.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1002423
---
 src/conf/network_conf.c  | 33 
 tests/networkxml2xmlin/hostdev-duplicate.xml | 11 
 tests/networkxml2xmlin/passthrough-duplicate.xml | 10 +++
 tests/networkxml2xmltest.c   |  2 ++
 4 files changed, 51 insertions(+), 5 deletions(-)
 create mode 100644 tests/networkxml2xmlin/hostdev-duplicate.xml
 create mode 100644 tests/networkxml2xmlin/passthrough-duplicate.xml

diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
index 4fb2e2a..043c79b 100644
--- a/src/conf/network_conf.c
+++ b/src/conf/network_conf.c
@@ -1,7 +1,7 @@
 /*
  * network_conf.c: network XML handling
  *
- * Copyright (C) 2006-2015 Red Hat, Inc.
+ * Copyright (C) 2006-2016 Red Hat, Inc.
  * Copyright (C) 2006-2008 Daniel P. Berrange
  *
  * This library is free software; you can redistribute it and/or
@@ -1799,7 +1799,7 @@ virNetworkForwardDefParseXML(const char *networkName,
  xmlXPathContextPtr ctxt,
  virNetworkForwardDefPtr def)
 {
-size_t i;
+size_t i, j;
 int ret = -1;
 xmlNodePtr *forwardIfNodes = NULL;
 xmlNodePtr *forwardPfNodes = NULL;
@@ -1936,6 +1936,16 @@ virNetworkForwardDefParseXML(const char *networkName,
 continue;
 }
 
+for (j = 0; j < i; j++) {
+if (STREQ_NULLABLE(def->ifs[j].device.dev, forwardDev)) {
+virReportError(VIR_ERR_XML_ERROR,
+   _("interface '%s' can only be "
+ "listed once in network %s"),
+   forwardDev, networkName);
+goto cleanup;
+}
+}
+
 def->ifs[i].device.dev = forwardDev;
 forwardDev = NULL;
 def->ifs[i].type = VIR_NETWORK_FORWARD_HOSTDEV_DEVICE_NETDEV;
@@ -1963,12 +1973,25 @@ virNetworkForwardDefParseXML(const char *networkName,
 
 switch (def->ifs[i].type) {
 case VIR_NETWORK_FORWARD_HOSTDEV_DEVICE_PCI:
-if (virDevicePCIAddressParseXML(forwardAddrNodes[i],
->ifs[i].device.pci) < 0) {
+{
+virDevicePCIAddressPtr addr = >ifs[i].device.pci;
+
+if (virDevicePCIAddressParseXML(forwardAddrNodes[i], addr) < 
0) {
 goto cleanup;
 }
+for (j = 0; j < i; j++) {
+if (virDevicePCIAddressEqual(addr, 
>ifs[j].device.pci)) {
+virReportError(VIR_ERR_XML_ERROR,
+   _("PCI device '%04x:%02x:%02x.%x' can "
+ "only be listed once in network %s"),
+   addr->domain, addr->bus,
+   addr->slot, addr->function,
+   networkName);
+goto cleanup;
+}
+}
 break;
-
+}
 /* Add USB case here if we ever find a reason to support it */
 
 default:
diff --git a/tests/networkxml2xmlin/hostdev-duplicate.xml 
b/tests/networkxml2xmlin/hostdev-duplicate.xml
new file mode 100644
index 000..79e55aa
--- /dev/null
+++ b/tests/networkxml2xmlin/hostdev-duplicate.xml
@@ -0,0 +1,11 @@
+
+  hostdev
+  81ff0d90-c91e-6742-64da-4a736edb9a9b
+  
+
+
+
+
+
+  
+
diff --git a/tests/networkxml2xmlin/passthrough-duplicate.xml 
b/tests/networkxml2xmlin/passthrough-duplicate.xml
new file mode 100644
index 000..8f645f7
--- /dev/null
+++ b/tests/networkxml2xmlin/passthrough-duplicate.xml
@@ -0,0 +1,10 @@
+
+  passthrough-duplicate
+  81ff0d90-c91e-6742-64da-4a736edb9a8b
+  
+
+
+
+
+  
+
diff --git a/tests/networkxml2xmltest.c b/tests/networkxml2xmltest.c
index b83396b..eafd473 100644
--- a/tests/networkxml2xmltest.c
+++ b/tests/networkxml2xmltest.c
@@ -151,6 +151,8 @@ mymain(void)
 DO_TEST("passthrough-address-crash");
 DO_TEST("nat-network-explicit-flood");
 DO_TEST("host-bridge-no-flood");
+DO_TEST_PARSE_ERROR("hostdev-duplicate");
+DO_TEST_PARSE_ERROR("passthrough-duplicate");
 
 return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE;
 }
-- 
2.5.5

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

[libvirt] [PATCH 1/2] test: enable testing for expected parse errors in network XML

[Laine Stump]

This is patterned after similar functionality for domain XML tests,
but tries harder to avoid reading non-existent networkxml2xmlout data
file when parse fails.
---
 tests/networkxml2xmltest.c | 75 +++---
 1 file changed, 57 insertions(+), 18 deletions(-)

diff --git a/tests/networkxml2xmltest.c b/tests/networkxml2xmltest.c
index 8d60aa8..b83396b 100644
--- a/tests/networkxml2xmltest.c
+++ b/tests/networkxml2xmltest.c
@@ -16,26 +16,58 @@
 
 #define VIR_FROM_THIS VIR_FROM_NONE
 
+typedef enum {
+TEST_COMPARE_NET_XML2XML_RESULT_SUCCESS,
+TEST_COMPARE_NET_XML2XML_RESULT_FAIL_PARSE,
+TEST_COMPARE_NET_XML2XML_RESULT_FAIL_FORMAT,
+TEST_COMPARE_NET_XML2XML_RESULT_FAIL_COMPARE,
+} testCompareNetXML2XMLResult;
+
 static int
 testCompareXMLToXMLFiles(const char *inxml, const char *outxml,
- unsigned int flags)
+ unsigned int flags,
+ testCompareNetXML2XMLResult expectResult)
 {
 char *actual = NULL;
-int ret = -1;
+int ret;
+testCompareNetXML2XMLResult result = 
TEST_COMPARE_DOM_XML2XML_RESULT_SUCCESS;
 virNetworkDefPtr dev = NULL;
 
-if (!(dev = virNetworkDefParseFile(inxml)))
-goto fail;
-
-if (!(actual = virNetworkDefFormat(dev, flags)))
-goto fail;
+if (!(dev = virNetworkDefParseFile(inxml))) {
+result = TEST_COMPARE_NET_XML2XML_RESULT_FAIL_PARSE;
+goto cleanup;
+}
+if (expectResult == TEST_COMPARE_NET_XML2XML_RESULT_FAIL_PARSE)
+goto cleanup;
 
-if (virtTestCompareToFile(actual, outxml) < 0)
-goto fail;
+if (!(actual = virNetworkDefFormat(dev, flags))) {
+result = TEST_COMPARE_NET_XML2XML_RESULT_FAIL_FORMAT;
+goto cleanup;
+}
+if (expectResult == TEST_COMPARE_NET_XML2XML_RESULT_FAIL_FORMAT)
+goto cleanup;
+
+if (virtTestCompareToFile(actual, outxml) < 0) {
+result = TEST_COMPARE_NET_XML2XML_RESULT_FAIL_COMPARE;
+goto cleanup;
+}
+if (expectResult == TEST_COMPARE_NET_XML2XML_RESULT_FAIL_COMPARE)
+goto cleanup;
 
-ret = 0;
+ cleanup:
+if (result == expectResult) {
+ret = 0;
+if (expectResult != TEST_COMPARE_NET_XML2XML_RESULT_SUCCESS) {
+VIR_TEST_DEBUG("Got expected failure code=%d msg=%s",
+   result, virGetLastErrorMessage());
+}
+} else {
+ret = -1;
+VIR_TEST_DEBUG("Expected result code=%d but received code=%d",
+   expectResult, result);
+}
+virResetLastError();
 
- fail:
 VIR_FREE(actual);
 virNetworkDefFree(dev);
 return ret;
@@ -44,6 +76,7 @@ testCompareXMLToXMLFiles(const char *inxml, const char 
*outxml,
 struct testInfo {
 const char *name;
 unsigned int flags;
+testCompareNetXML2XMLResult expectResult;
 };
 
 static int
@@ -61,7 +94,8 @@ testCompareXMLToXMLHelper(const void *data)
 goto cleanup;
 }
 
-result = testCompareXMLToXMLFiles(inxml, outxml, info->flags);
+result = testCompareXMLToXMLFiles(inxml, outxml, info->flags,
+  info->expectResult);
 
  cleanup:
 VIR_FREE(inxml);
@@ -75,14 +109,19 @@ mymain(void)
 {
 int ret = 0;
 
-#define DO_TEST_FULL(name, flags)   \
+#define DO_TEST_FULL(name, flags, expectResult) \
 do {\
-const struct testInfo info = {name, flags}; \
+const struct testInfo info = {name, flags, expectResult};   \
 if (virtTestRun("Network XML-2-XML " name,  \
-testCompareXMLToXMLHelper, ) < 0)  \
+testCompareXMLToXMLHelper, ) < 0) \
 ret = -1;   \
 } while (0)
-#define DO_TEST(name) DO_TEST_FULL(name, 0)
+#define DO_TEST(name) \
+DO_TEST_FULL(name, 0, TEST_COMPARE_NET_XML2XML_RESULT_SUCCESS)
+#define DO_TEST_FLAGS(name, flags) \
+DO_TEST_FULL(name, flags, TEST_COMPARE_NET_XML2XML_RESULT_SUCCESS)
+#define DO_TEST_PARSE_ERROR(name) \
+DO_TEST_FULL(name, 0, TEST_COMPARE_NET_XML2XML_RESULT_FAIL_PARSE)
 
 DO_TEST("dhcp6host-routed-network");
 DO_TEST("empty-allow-ipv6");
@@ -106,9 +145,9 @@ mymain(void)
 DO_TEST("vepa-net");
 DO_TEST("bandwidth-network");
 DO_TEST("openvswitch-net");
-DO_TEST_FULL("passthrough-pf", VIR_NETWORK_XML_INACTIVE);
+DO_TEST_FLAGS("passthrough-pf", VIR_NETWORK_XML_INACTIVE);
 DO_TEST("hostdev");
-DO_TEST_FULL("hostdev-pf", VIR_NETWORK_XML_INACTIVE);
+DO_TEST_FLAGS("hostdev-pf", VIR_NETWORK_XML_INACTIVE);
 DO_TEST("passthrough-address-crash");
 DO_TEST("nat-network-explicit-flood");
 DO_TEST("host-bridge-no-flood");
-- 
2.5.5

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

Re: [libvirt] [PATCH v2] Include sysmacros.h where needed

[Martin Kletzander]

On Fri, Apr 15, 2016 at 05:04:20PM +0200, Martin Kletzander wrote:

On Fri, Apr 15, 2016 at 02:26:09PM +0200, Peter Krempa wrote:

On Fri, Apr 15, 2016 at 09:21:49 +0200, Martin Kletzander wrote:

So in glibc-2.23 sys/sysmacros.h is no longer included from sys/types.h
and we don't build because of the usage of major/minor/makedev macros.
Autoconf already has AC_HEADER_MAJOR macro that check where exactly
these functions/macros are defined, so let's use that.

Signed-off-by: Martin Kletzander 
---
v2:
 - Don't include the file unconditionally, but rather use
   AC_HEADER_MAJOR that exists for exactly this purpose.

v1:
 - https://www.redhat.com/archives/libvir-list/2016-April/msg00851.html

 configure.ac | 2 ++
 src/conf/domain_audit.c  | 6 ++
 src/lxc/lxc_controller.c | 7 +++
 src/lxc/lxc_driver.c | 7 +++
 src/util/vircgroup.c | 7 +++
 src/util/virutil.c   | 7 +++
 tests/vircgroupmock.c| 7 +++
 7 files changed, 43 insertions(+)


I've just found out about the breakage by upgrading glibc and almost
blamed you as I've misunderstood this patch.

This fixes the build for me and the usage of AC_HEADER_MAJOR looks on
the spot to me.

ACK


Thanks but you're not the only one, so let's wait for the consensus to
be reached.



Since nobody objected, I'm pushing this.


Martin





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


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

Re: [libvirt] [PATCH] qemu: Label master key file

[Martin Kletzander]

On Mon, Apr 18, 2016 at 08:55:02AM -0400, Cole Robinson wrote:

On 04/18/2016 02:34 AM, Martin Kletzander wrote:

On Fri, Apr 15, 2016 at 12:16:18PM -0400, Cole Robinson wrote:

On 04/14/2016 03:04 AM, Martin Kletzander wrote:

On Wed, Apr 13, 2016 at 07:58:06PM -0400, Cole Robinson wrote:

On 04/13/2016 11:56 AM, Cole Robinson wrote:

On 04/13/2016 11:17 AM, Martin Kletzander wrote:

When creating the master key, we used mode 0600 (which we should) but
because we were creating it as root, the file is not readable by any
qemu running as non-root.  Fortunately, it's just a matter of labelling
the file.  We are generating the file path few times already, so let's
label it in the same function that has access to the path already.

Signed-off-by: Martin Kletzander 
---
 src/qemu/qemu_domain.c  | 15 ---
 src/qemu/qemu_domain.h  |  3 ++-
 src/qemu/qemu_process.c |  2 +-
 3 files changed, 15 insertions(+), 5 deletions(-)



ACK, makes sense and fixes things for me. One comment below


diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 5d54fffcfb98..83e765ef6868 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -504,11 +504,13 @@ qemuDomainGetMasterKeyFilePath(const char *libDir)
  * Returns 0 on success, -1 on failure with error message indicating
failure
  */
 static int
-qemuDomainWriteMasterKeyFile(qemuDomainObjPrivatePtr priv)
+qemuDomainWriteMasterKeyFile(virQEMUDriverPtr driver,
+ virDomainObjPtr vm)
 {
 char *path;
 int fd = -1;
 int ret = -1;
+qemuDomainObjPrivatePtr priv = vm->privateData;

 if (!(path = qemuDomainGetMasterKeyFilePath(priv->libDir)))
 return -1;
@@ -525,6 +527,10 @@ qemuDomainWriteMasterKeyFile(qemuDomainObjPrivatePtr
priv)
 goto cleanup;
 }

+if (virSecurityManagerDomainSetDirLabel(driver->securityManager,
+vm->def, path) < 0)
+goto cleanup;
+
 ret = 0;



I looked briefly at fixing this but know if there was a function to ask the
security driver 'just set a on this arbitrary path'. I saw DirLabel but was
thrown off by the 'Dir' name. Maybe change it to something more generic?



Yes, it's just a naming; I had to add it for similar purpose when
labelling directories, but it "Just Works" for arbitrary path.  I'll
rename that.



Also adding some CC, I'm guessing virt-aa-helper.c needs to be extended to to
allow access to $libDir/master-key.aes



Actually, it shouldn't.  It's in the per-domain directory and
everything under that should be available.

Anyway, it's a pity that we're not very likely to have a test case for
that.  But couldn't the paths in virt-aa-helper be created from a
security driver?  It knows all the paths, doesn't it?

I'll send a v2 with the rename.


Laine was hitting issues with this too, so I pushed this patch. The API rename
isn't blocking anyone



v2 was alrady on the list, but it can be done the other way around.
I'll send the rename rebased now so it's easier to review.



Oh sorry, I must have missed it? Though looking through the archives I don't
see anything, only the original patch and the v3. Maybe it didn't hit the 
list...



Then I must've missed it somehow.  Anyway, at least it's done now :)


- Cole



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

[libvirt] [PATCH] conf: Skip generating random MAC in DetachDevice xml

[Kothapally Madhu Pavan]

When we try to detach a network device without specifying
the mac address, random mac address is generated. As the
generated mac address will not be available in the running
vm, detaching device will fail erroring out "error:
operation failed: no device matching mac address
xx:xx:xx:xx:xx:xx found".

Signed-off-by: Kothapally Madhu Pavan 
---
 src/conf/domain_conf.c   |4 
 src/conf/domain_conf.h   |3 +++
 src/libxl/libxl_driver.c |   12 ++--
 src/lxc/lxc_driver.c |7 ---
 src/qemu/qemu_driver.c   |2 ++
 src/uml/uml_driver.c |6 --
 src/vbox/vbox_common.c   |6 --
 src/vz/vz_driver.c   |4 +++-
 src/xen/xend_internal.c  |6 --
 src/xen/xm_internal.c|8 
 10 files changed, 38 insertions(+), 20 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 28248c8..512d877 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -8784,6 +8784,10 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
(const char *)macaddr);
 goto error;
 }
+} else if (flags & VIR_DOMAIN_DEF_PARSE_SKIP_GENERATE_MACADDR) {
+virReportError(VIR_ERR_XML_ERROR, "%s",
+   _("mac address not specified in the device xml"));
+goto error;
 } else {
 virDomainNetGenerateMAC(xmlopt, >mac);
 }
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 1986f53..74692f1 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2679,6 +2679,9 @@ typedef enum {
 VIR_DOMAIN_DEF_PARSE_SKIP_OSTYPE_CHECKS = 1 << 8,
 /* allow updates in post parse callback that would break ABI otherwise */
 VIR_DOMAIN_DEF_PARSE_ABI_UPDATE = 1 << 9,
+/* don't generate random mac address when a network device without mac 
address
+ * is detached */
+VIR_DOMAIN_DEF_PARSE_SKIP_GENERATE_MACADDR = 1 << 10,
 } virDomainDefParseFlags;
 
 typedef enum {
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index bf97c9c..507edcf 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -3726,6 +3726,8 @@ libxlDomainDetachDeviceFlags(virDomainPtr dom, const char 
*xml,
 virDomainDefPtr vmdef = NULL;
 virDomainDeviceDefPtr dev = NULL;
 int ret = -1;
+unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE |
+   VIR_DOMAIN_DEF_PARSE_SKIP_GENERATE_MACADDR;
 
 virCheckFlags(VIR_DOMAIN_DEVICE_MODIFY_LIVE |
   VIR_DOMAIN_DEVICE_MODIFY_CONFIG, -1);
@@ -3743,9 +3745,8 @@ libxlDomainDetachDeviceFlags(virDomainPtr dom, const char 
*xml,
 goto endjob;
 
 if (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) {
-if (!(dev = virDomainDeviceDefParse(xml, vm->def,
-cfg->caps, driver->xmlopt,
-VIR_DOMAIN_DEF_PARSE_INACTIVE)))
+if (!(dev = virDomainDeviceDefParse(xml, vm->def, cfg->caps,
+driver->xmlopt, parse_flags)))
 goto endjob;
 
 /* Make a copy for updated domain. */
@@ -3760,9 +3761,8 @@ libxlDomainDetachDeviceFlags(virDomainPtr dom, const char 
*xml,
 if (flags & VIR_DOMAIN_DEVICE_MODIFY_LIVE) {
 /* If dev exists it was created to modify the domain config. Free it. 
*/
 virDomainDeviceDefFree(dev);
-if (!(dev = virDomainDeviceDefParse(xml, vm->def,
-cfg->caps, driver->xmlopt,
-VIR_DOMAIN_DEF_PARSE_INACTIVE)))
+if (!(dev = virDomainDeviceDefParse(xml, vm->def, cfg->caps,
+driver->xmlopt, parse_flags)))
 goto endjob;
 
 if (libxlDomainDetachDeviceLive(driver, vm, dev) < 0)
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
index ef48812..23f0d80 100644
--- a/src/lxc/lxc_driver.c
+++ b/src/lxc/lxc_driver.c
@@ -5196,6 +5196,8 @@ static int lxcDomainDetachDeviceFlags(virDomainPtr dom,
 virDomainDefPtr vmdef = NULL;
 virDomainDeviceDefPtr dev = NULL, dev_copy = NULL;
 int ret = -1;
+unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE |
+   VIR_DOMAIN_DEF_PARSE_SKIP_GENERATE_MACADDR;
 virLXCDriverConfigPtr cfg = virLXCDriverGetConfig(driver);
 
 virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
@@ -5213,9 +5215,8 @@ static int lxcDomainDetachDeviceFlags(virDomainPtr dom,
 if (!(caps = virLXCDriverGetCapabilities(driver, false)))
 goto cleanup;
 
-dev = dev_copy = virDomainDeviceDefParse(xml, vm->def,
- caps, driver->xmlopt,
- VIR_DOMAIN_DEF_PARSE_INACTIVE);
+dev = dev_copy = virDomainDeviceDefParse(xml, vm->def, caps,
+ driver->xmlopt, parse_flags);
 if (dev == NULL)
 

Re: [libvirt] [RFC 6/6] qemu: Cache GIC capabilities

[Andrea Bolognani]

On Thu, 2016-04-14 at 18:14 -0400, Cole Robinson wrote:
> On 04/06/2016 11:29 AM, Andrea Bolognani wrote:
> > On Wed, 2016-03-30 at 16:29 -0400, John Ferlan wrote:
> > 
> > I believe all of these were addressed in my previous comments.
> > 
> > Let me know if that was actually not the case :)
> 
> I tried going through the patches and the comments but it was a bit hard to
> follow, there was quite a few points/counterpoints :)
> 
> Andrea can you send a v2 addressing the obvious bits that John pointed out,
> and anything you're still unsure of/disagreed with just highlight in v2 
> comments?

I've just posted a non-RFC version[1] that does exactly what
you're asking.

Cheers.


[1] https://www.redhat.com/archives/libvir-list/2016-April/msg01143.html
-- 
Andrea Bolognani
Software Engineer - Virtualization Team

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

[libvirt] [PATCH 2/7] qemu: Probe GIC capabilities

[Andrea Bolognani]

QEMU introduced the query-gic-capabilities QMP command
with commit 4468d4e0f383: use the command, if available,
to probe available GIC capabilities.

The information obtained is stored in a virQEMUCaps
instance, and will be later used to fill in a
virDomainCaps instance.
---
Changes from RFC:

  * Free qemuCaps->gicCapabilities when needed

  * Always return NULL when no capabilities have been found

  * Don't allocate (n+1) elements to store (n) capabilities

  * Check for ARM consistently with the rest of the code

  * Reference QEMU commit

  * Leave two empty lines between functions

  * Document all functions

 src/qemu/qemu_capabilities.c |  42 
 src/qemu/qemu_monitor.c  |  17 +++
 src/qemu/qemu_monitor.h  |   4 ++
 src/qemu/qemu_monitor_json.c | 115 +++
 src/qemu/qemu_monitor_json.h |   4 ++
 src/util/virgic.h|  13 +
 6 files changed, 195 insertions(+)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 9ae7b27..4afc6b6 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -358,6 +358,9 @@ struct _virQEMUCaps {
 char **machineTypes;
 char **machineAliases;
 unsigned int *machineMaxCpus;
+
+size_t ngicCapabilities;
+virGICCapability *gicCapabilities;
 };
 
 struct virQEMUCapsSearchData {
@@ -2082,6 +2085,8 @@ void virQEMUCapsDispose(void *obj)
 
 VIR_FREE(qemuCaps->package);
 VIR_FREE(qemuCaps->binary);
+
+VIR_FREE(qemuCaps->gicCapabilities);
 }
 
 void
@@ -2696,6 +2701,34 @@ virQEMUCapsProbeQMPMigrationCapabilities(virQEMUCapsPtr 
qemuCaps,
 return 0;
 }
 
+/**
+ * virQEMUCapsProbeQMPGICCapabilities:
+ * @qemuCaps: QEMU binary capabilities
+ * @mon: QEMU monitor
+ *
+ * Use @mon to obtain information about the GIC capabilities for the
+ * corresponding QEMU binary, and store them in @qemuCaps.
+ *
+ * Returns: 0 on success, <0 on failure
+ */
+static int
+virQEMUCapsProbeQMPGICCapabilities(virQEMUCapsPtr qemuCaps,
+   qemuMonitorPtr mon)
+{
+virGICCapability *caps = NULL;
+int ncaps;
+
+if ((ncaps = qemuMonitorGetGICCapabilities(mon, )) < 0)
+return -1;
+
+VIR_FREE(qemuCaps->gicCapabilities);
+
+qemuCaps->gicCapabilities = caps;
+qemuCaps->ngicCapabilities = ncaps;
+
+return 0;
+}
+
 int virQEMUCapsProbeQMP(virQEMUCapsPtr qemuCaps,
 qemuMonitorPtr mon)
 {
@@ -3047,6 +3080,9 @@ virQEMUCapsReset(virQEMUCapsPtr qemuCaps)
 VIR_FREE(qemuCaps->machineAliases);
 VIR_FREE(qemuCaps->machineMaxCpus);
 qemuCaps->nmachineTypes = 0;
+
+VIR_FREE(qemuCaps->gicCapabilities);
+qemuCaps->ngicCapabilities = 0;
 }
 
 
@@ -3411,6 +3447,12 @@ virQEMUCapsInitQMPMonitor(virQEMUCapsPtr qemuCaps,
 if (virQEMUCapsProbeQMPMigrationCapabilities(qemuCaps, mon) < 0)
 goto cleanup;
 
+/* GIC capabilities, eg. available GIC versions */
+if ((qemuCaps->arch == VIR_ARCH_AARCH64 ||
+ qemuCaps->arch == VIR_ARCH_ARMV7L) &&
+virQEMUCapsProbeQMPGICCapabilities(qemuCaps, mon) < 0)
+goto cleanup;
+
 ret = 0;
  cleanup:
 return ret;
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index 83551a8..7c9ea71 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -3580,6 +3580,23 @@ qemuMonitorSetMigrationCapability(qemuMonitorPtr mon,
 }
 
 
+/**
+ * qemuMonitorGetGICCapabilities:
+ * @mon: QEMU monitor
+ * @capabilities: where to store the GIC capabilities
+ *
+ * See qemuMonitorJSONGetGICCapabilities().
+ */
+int
+qemuMonitorGetGICCapabilities(qemuMonitorPtr mon,
+  virGICCapability **capabilities)
+{
+QEMU_CHECK_MONITOR_JSON(mon);
+
+return qemuMonitorJSONGetGICCapabilities(mon, capabilities);
+}
+
+
 int
 qemuMonitorNBDServerStart(qemuMonitorPtr mon,
   const char *host,
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
index bd5d006..470c729 100644
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -34,6 +34,7 @@
 # include "virnetdev.h"
 # include "device_conf.h"
 # include "cpu/cpu.h"
+# include "util/virgic.h"
 
 typedef struct _qemuMonitor qemuMonitor;
 typedef qemuMonitor *qemuMonitorPtr;
@@ -583,6 +584,9 @@ int qemuMonitorSetMigrationCapability(qemuMonitorPtr mon,
   qemuMonitorMigrationCaps capability,
   bool state);
 
+int qemuMonitorGetGICCapabilities(qemuMonitorPtr mon,
+  virGICCapability **capabilities);
+
 typedef enum {
   QEMU_MONITOR_MIGRATE_BACKGROUND  = 1 << 0,
   QEMU_MONITOR_MIGRATE_NON_SHARED_DISK  = 1 << 1, /* migration with non-shared 
storage with full disk copy */
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 29d6c8c..7bb9976 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -5855,6 +5855,121 @@ 

[libvirt] [PATCH 3/7] schema: Validate GIC capabilities

[Andrea Bolognani]

We need to expose GIC capabilities in the domain capabilities
XML: update the schema to validate documents that contain the
new information.
---
 docs/schemas/domaincaps.rng | 18 ++
 1 file changed, 18 insertions(+)

diff --git a/docs/schemas/domaincaps.rng b/docs/schemas/domaincaps.rng
index 35d3745..0d2777b 100644
--- a/docs/schemas/domaincaps.rng
+++ b/docs/schemas/domaincaps.rng
@@ -31,6 +31,9 @@
 
   
 
+
+  
+
   
 
   
@@ -88,6 +91,21 @@
 
   
 
+  
+
+  
+
+  
+
+  
+
+  
+
+  
+  
+
+  
+
   
 
   
-- 
2.5.5

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

[libvirt] [PATCH 6/7] qemu: Cache GIC capabilities

[Andrea Bolognani]

Implement support for saving GIC capabilities in the cache and
read them back.
---
 src/qemu/qemu_capabilities.c | 87 
 1 file changed, 87 insertions(+)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index ee80be2..be3e420 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -2926,6 +2926,77 @@ virQEMUCapsLoadCache(virQEMUCapsPtr qemuCaps, const char 
*filename,
 }
 VIR_FREE(nodes);
 
+if ((n = virXPathNodeSet("./gic", ctxt, )) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("failed to parse qemu capabilities gic"));
+goto cleanup;
+}
+if (n > 0) {
+unsigned int uintValue;
+bool boolValue;
+
+qemuCaps->ngicCapabilities = n;
+if (VIR_ALLOC_N(qemuCaps->gicCapabilities, n) < 0)
+goto cleanup;
+
+for (i = 0; i < n; i++) {
+virGICCapabilityPtr cap = >gicCapabilities[i];
+
+if (!(str = virXMLPropString(nodes[i], "version"))) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("missing GIC version "
+ "in QEMU capabilities cache"));
+goto cleanup;
+}
+if (str &&
+virStrToLong_ui(str, NULL, 10, ) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("malformed GIC version "
+ "in QEMU capabilities cache"));
+goto cleanup;
+}
+cap->version = uintValue;
+VIR_FREE(str);
+
+if (!(str = virXMLPropString(nodes[i], "kernel"))) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("missing in-kernel GIC information "
+ "in QEMU capabilities cache"));
+goto cleanup;
+}
+if (str &&
+!(boolValue = STREQ(str, "true")) &&
+STRNEQ(str, "false")) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("malformed in-kernel GIC information "
+ "in QEMU capabilities cache"));
+goto cleanup;
+}
+if (boolValue)
+cap->implementation |= VIR_GIC_IMPLEMENTATION_KERNEL;
+VIR_FREE(str);
+
+if (!(str = virXMLPropString(nodes[i], "emulated"))) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("missing emulated GIC information "
+ "in QEMU capabilities cache"));
+goto cleanup;
+}
+if (str &&
+!(boolValue = STREQ(str, "true")) &&
+STRNEQ(str, "false")) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("malformed emulated GIC information "
+ "in QEMU capabilities cache"));
+goto cleanup;
+}
+if (boolValue)
+cap->implementation |= VIR_GIC_IMPLEMENTATION_EMULATED;
+VIR_FREE(str);
+}
+}
+VIR_FREE(nodes);
+
 ret = 0;
  cleanup:
 VIR_FREE(str);
@@ -2992,6 +3063,22 @@ virQEMUCapsSaveCache(virQEMUCapsPtr qemuCaps, const char 
*filename)
   qemuCaps->machineMaxCpus[i]);
 }
 
+for (i = 0; i < qemuCaps->ngicCapabilities; i++) {
+virGICCapabilityPtr cap;
+bool kernel;
+bool emulated;
+
+cap = >gicCapabilities[i];
+kernel = (cap->implementation & VIR_GIC_IMPLEMENTATION_KERNEL);
+emulated = (cap->implementation & VIR_GIC_IMPLEMENTATION_EMULATED);
+
+virBufferAsprintf(,
+  "\n",
+  cap->version,
+  kernel ? "true" : "false",
+  emulated ? "true" : "false");
+}
+
 virBufferAdjustIndent(, -2);
 virBufferAddLit(, "\n");
 
-- 
2.5.5

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

[libvirt] [PATCH 7/7] docs: Document the new XML elements

[Andrea Bolognani]

---
 docs/formatdomain.html.in |  3 ++-
 docs/formatdomaincaps.html.in | 43 ++-
 2 files changed, 44 insertions(+), 2 deletions(-)

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 9bcef6a..54ad99b 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -1497,7 +1497,8 @@
   All features are listed within the features
   element, omitting a togglable feature tag turns it off.
   The available features can be found by asking
-  for the capabilities XML,
+  for the capabilities XML and
+  domain capabilities XML,
   but a common set for fully virtualized domains are:
 
 
diff --git a/docs/formatdomaincaps.html.in b/docs/formatdomaincaps.html.in
index 850109f..0c40321 100644
--- a/docs/formatdomaincaps.html.in
+++ b/docs/formatdomaincaps.html.in
@@ -145,7 +145,7 @@
 Devices
 
 
-  The final set of XML elements describe the supported devices and their
+  Another set of XML elements describe the supported devices and their
   capabilities. All devices occur as children of the main
   devices element.
 
@@ -277,5 +277,46 @@
   Options for the name attribute of the driver/
   element.
 
+
+Features
+
+One more set of XML elements describe the supported features and
+their capabilities. All features occur as children of the main
+features element.
+
+
+domainCapabilities
+  ...
+  features
+gic supported='yes'
+  enum name='version'
+value2/value
+value3/value
+  /enum
+/gic
+  /features
+/domainCapabilities
+
+
+Reported capabilities are expressed as an enumerated list of
+possible values for each of the elements or attributes. For example, the
+gic element has an attribute version which can
+support the values 2 or 3.
+
+For information about the purpose of each feature, see the
+relevant section in
+the domain XML documentation.
+
+
+GIC capabilities
+
+GIC capabilities are exposed under the gic element.
+
+
+  version
+  Options for the version attribute of the
+  gic element.
+
+
   
 
-- 
2.5.5

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

[libvirt] [PATCH 4/7] conf: Expose GIC capabilities

[Andrea Bolognani]

Add information about GIC capabilities to virDomainCaps and update
the formatter to include them in the XML output.
---
 src/conf/domain_capabilities.c | 36 ++
 src/conf/domain_capabilities.h | 10 ++
 tests/domaincapsschemadata/domaincaps-basic.xml|  3 ++
 tests/domaincapsschemadata/domaincaps-full.xml |  3 ++
 .../domaincaps-qemu_1.6.50-1.xml   |  3 ++
 5 files changed, 55 insertions(+)

diff --git a/src/conf/domain_capabilities.c b/src/conf/domain_capabilities.c
index 466c0c6..eb880ae 100644
--- a/src/conf/domain_capabilities.c
+++ b/src/conf/domain_capabilities.c
@@ -262,6 +262,34 @@ virDomainCapsDeviceHostdevFormat(virBufferPtr buf,
 }
 
 
+/**
+ * virDomainCapsFeatureGICFormat:
+ * @buf: target buffer
+ * @gic: GIC features
+ *
+ * Format GIC features for inclusion in the domcapabilities XML.
+ *
+ * The resulting XML will look like
+ *
+ *   
+ * 
 
   
+  
+
+  
 
diff --git a/tests/domaincapsschemadata/domaincaps-full.xml 
b/tests/domaincapsschemadata/domaincaps-full.xml
index 96202bc..d4f91fa 100644
--- a/tests/domaincapsschemadata/domaincaps-full.xml
+++ b/tests/domaincapsschemadata/domaincaps-full.xml
@@ -68,4 +68,7 @@
   
 
   
+  
+
+  
 
diff --git a/tests/domaincapsschemadata/domaincaps-qemu_1.6.50-1.xml 
b/tests/domaincapsschemadata/domaincaps-qemu_1.6.50-1.xml
index 37d2102..990661b 100644
--- a/tests/domaincapsschemadata/domaincaps-qemu_1.6.50-1.xml
+++ b/tests/domaincapsschemadata/domaincaps-qemu_1.6.50-1.xml
@@ -56,4 +56,7 @@
   
 
   
+  
+
+  
 
-- 
2.5.5

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

[libvirt] [PATCH 1/7] conf: Get rid of virDomainCapsDevice

[Andrea Bolognani]

The struct contains a single boolean field, 'supported':
the meaning of this field is too generic to be limited to
devices only, and in fact it's already being used for
other things like loaders and OSs.

Instead of trying to come up with a more generic name just
get rid of the struct altogether.
---
Changes from RFC:

  * Improve commit message

 src/conf/domain_capabilities.c |  6 +++---
 src/conf/domain_capabilities.h | 14 --
 src/qemu/qemu_capabilities.c   |  8 
 tests/domaincapstest.c |  8 
 4 files changed, 15 insertions(+), 21 deletions(-)

diff --git a/src/conf/domain_capabilities.c b/src/conf/domain_capabilities.c
index 0e32f52..466c0c6 100644
--- a/src/conf/domain_capabilities.c
+++ b/src/conf/domain_capabilities.c
@@ -187,9 +187,9 @@ virDomainCapsStringValuesFormat(virBufferPtr buf,
 #define FORMAT_PROLOGUE(item)   \
 do {\
 virBufferAsprintf(buf, "<" #item " supported='%s'%s\n", \
-  item->device.supported ? "yes" : "no",\
-  item->device.supported ? ">" : "/>"); \
-if (!item->device.supported)\
+  item->supported ? "yes" : "no",   \
+  item->supported ? ">" : "/>");\
+if (!item->supported)   \
 return; \
 virBufferAdjustIndent(buf, 2);  \
 } while (0)
diff --git a/src/conf/domain_capabilities.h b/src/conf/domain_capabilities.h
index 597ac75..3eacb35 100644
--- a/src/conf/domain_capabilities.h
+++ b/src/conf/domain_capabilities.h
@@ -44,16 +44,10 @@ struct _virDomainCapsStringValues {
 size_t nvalues; /* number of strings */
 };
 
-typedef struct _virDomainCapsDevice virDomainCapsDevice;
-typedef virDomainCapsDevice *virDomainCapsDevicePtr;
-struct _virDomainCapsDevice {
-bool supported; /* true if  is supported by hypervisor */
-};
-
 typedef struct _virDomainCapsLoader virDomainCapsLoader;
 typedef virDomainCapsLoader *virDomainCapsLoaderPtr;
 struct _virDomainCapsLoader {
-virDomainCapsDevice device;
+bool supported;
 virDomainCapsStringValues values;   /* Info about values for the element */
 virDomainCapsEnum type; /* Info about virDomainLoader */
 virDomainCapsEnum readonly; /* Info about readonly:virTristateBool */
@@ -62,14 +56,14 @@ struct _virDomainCapsLoader {
 typedef struct _virDomainCapsOS virDomainCapsOS;
 typedef virDomainCapsOS *virDomainCapsOSPtr;
 struct _virDomainCapsOS {
-virDomainCapsDevice device;
+bool supported;
 virDomainCapsLoader loader; /* Info about virDomainLoaderDef */
 };
 
 typedef struct _virDomainCapsDeviceDisk virDomainCapsDeviceDisk;
 typedef virDomainCapsDeviceDisk *virDomainCapsDeviceDiskPtr;
 struct _virDomainCapsDeviceDisk {
-virDomainCapsDevice device;
+bool supported;
 virDomainCapsEnum diskDevice;   /* Info about virDomainDiskDevice enum 
values */
 virDomainCapsEnum bus;  /* Info about virDomainDiskBus enum values 
*/
 /* add new fields here */
@@ -78,7 +72,7 @@ struct _virDomainCapsDeviceDisk {
 typedef struct _virDomainCapsDeviceHostdev virDomainCapsDeviceHostdev;
 typedef virDomainCapsDeviceHostdev *virDomainCapsDeviceHostdevPtr;
 struct _virDomainCapsDeviceHostdev {
-virDomainCapsDevice device;
+bool supported;
 virDomainCapsEnum mode; /* Info about virDomainHostdevMode */
 virDomainCapsEnum startupPolicy;/* Info about virDomainStartupPolicy */
 virDomainCapsEnum subsysType;   /* Info about 
virDomainHostdevSubsysType */
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index f46dcad..9ae7b27 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -3908,7 +3908,7 @@ virQEMUCapsFillDomainLoaderCaps(virQEMUCapsPtr qemuCaps,
 {
 size_t i;
 
-capsLoader->device.supported = true;
+capsLoader->supported = true;
 
 if (VIR_ALLOC_N(capsLoader->values.values, nloader) < 0)
 return -1;
@@ -3950,7 +3950,7 @@ virQEMUCapsFillDomainOSCaps(virQEMUCapsPtr qemuCaps,
 {
 virDomainCapsLoaderPtr capsLoader = >loader;
 
-os->device.supported = true;
+os->supported = true;
 if (virQEMUCapsFillDomainLoaderCaps(qemuCaps, capsLoader,
 loader, nloader) < 0)
 return -1;
@@ -3963,7 +3963,7 @@ virQEMUCapsFillDomainDeviceDiskCaps(virQEMUCapsPtr 
qemuCaps,
 const char *machine,
 virDomainCapsDeviceDiskPtr disk)
 {
-disk->device.supported = true;
+disk->supported = true;
 /* QEMU supports all of these */
 VIR_DOMAIN_CAPS_ENUM_SET(disk->diskDevice,
  

[libvirt] [PATCH 5/7] qemu: Fill in GIC capabilities

[Andrea Bolognani]

Take the GIC capabilities stored in a virQEMUCaps instance and
update a virDomainCaps instance appropriately.
---
 src/qemu/qemu_capabilities.c | 57 +++-
 1 file changed, 56 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 4afc6b6..ee80be2 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -4082,6 +4082,60 @@ virQEMUCapsFillDomainDeviceHostdevCaps(virQEMUCapsPtr 
qemuCaps,
 }
 
 
+/**
+ * virQEMUCapsFillDomainFeatureGICCaps:
+ * @qemuCaps: QEMU capabilities
+ * @domCaps: domain capabilities
+ *
+ * Take the information about GIC capabilities that has been obtained
+ * using the 'query-gic-capabilities' QMP command and stored in @qemuCaps
+ * and convert it to a form suitable for @domCaps.
+ *
+ * @qemuCaps contains complete information about the GIC capabilities for
+ * the corresponding QEMU binary, stored as custom objects; @domCaps, on
+ * the other hand, should only contain information about the GIC versions
+ * available for the specific combination of architecture, machine type
+ * and virtualization type. Moreover, a common format is used to store
+ * information about enumerations in @domCaps, so further processing is
+ * required.
+ *
+ * Returns: 0 on success, <0 on failure
+ */
+static int
+virQEMUCapsFillDomainFeatureGICCaps(virQEMUCapsPtr qemuCaps,
+virDomainCapsPtr domCaps)
+{
+virDomainCapsFeatureGICPtr gic = >gic;
+size_t i;
+
+if (domCaps->arch != VIR_ARCH_ARMV7L &&
+domCaps->arch != VIR_ARCH_AARCH64)
+return 0;
+
+if (STRNEQ(domCaps->machine, "virt") &&
+!STRPREFIX(domCaps->machine, "virt-"))
+return 0;
+
+for (i = 0; i < qemuCaps->ngicCapabilities; i++) {
+virGICCapabilityPtr cap = >gicCapabilities[i];
+
+if (domCaps->virttype == VIR_DOMAIN_VIRT_KVM &&
+!(cap->implementation & VIR_GIC_IMPLEMENTATION_KERNEL))
+continue;
+
+if (domCaps->virttype == VIR_DOMAIN_VIRT_QEMU &&
+!(cap->implementation & VIR_GIC_IMPLEMENTATION_EMULATED))
+continue;
+
+gic->supported = true;
+VIR_DOMAIN_CAPS_ENUM_SET(gic->version,
+ cap->version);
+}
+
+return 0;
+}
+
+
 int
 virQEMUCapsFillDomainCaps(virDomainCapsPtr domCaps,
   virQEMUCapsPtr qemuCaps,
@@ -4098,7 +4152,8 @@ virQEMUCapsFillDomainCaps(virDomainCapsPtr domCaps,
 if (virQEMUCapsFillDomainOSCaps(qemuCaps, os,
 loader, nloader) < 0 ||
 virQEMUCapsFillDomainDeviceDiskCaps(qemuCaps, domCaps->machine, disk) 
< 0 ||
-virQEMUCapsFillDomainDeviceHostdevCaps(qemuCaps, hostdev) < 0)
+virQEMUCapsFillDomainDeviceHostdevCaps(qemuCaps, hostdev) < 0 ||
+virQEMUCapsFillDomainFeatureGICCaps(qemuCaps, domCaps) < 0)
 return -1;
 return 0;
 }
-- 
2.5.5

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

[libvirt] [PATCH 0/7] Probe and expose GIC capabilities

[Andrea Bolognani]

Changes from [RFC]:

  * Fix issues pointed out during review (see patches
1 and 2 for details)

  * Add documentation

The only thing missing AFAIK is some test cases... I'm not sure
whether it's possible to include QMP replies for QEMU 2.6 even
though it hasn't been released yet, and I wouldn't know how to
generate a .replies file anyway. Any pointers?

Cheers.


[RFC] https://www.redhat.com/archives/libvir-list/2016-March/msg00956.html

Andrea Bolognani (7):
  conf: Get rid of virDomainCapsDevice
  qemu: Probe GIC capabilities
  schema: Validate GIC capabilities
  conf: Expose GIC capabilities
  qemu: Fill in GIC capabilities
  qemu: Cache GIC capabilities
  docs: Document the new XML elements

 docs/formatdomain.html.in  |   3 +-
 docs/formatdomaincaps.html.in  |  43 -
 docs/schemas/domaincaps.rng|  18 ++
 src/conf/domain_capabilities.c |  42 -
 src/conf/domain_capabilities.h |  24 +--
 src/qemu/qemu_capabilities.c   | 194 -
 src/qemu/qemu_monitor.c|  17 ++
 src/qemu/qemu_monitor.h|   4 +
 src/qemu/qemu_monitor_json.c   | 115 
 src/qemu/qemu_monitor_json.h   |   4 +
 src/util/virgic.h  |  13 ++
 tests/domaincapsschemadata/domaincaps-basic.xml|   3 +
 tests/domaincapsschemadata/domaincaps-full.xml |   3 +
 .../domaincaps-qemu_1.6.50-1.xml   |   3 +
 tests/domaincapstest.c |   8 +-
 15 files changed, 470 insertions(+), 24 deletions(-)

-- 
2.5.5

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

Re: [libvirt] [PATCH REBASE 1/4] vz: introduce vzsdk disk id function

[Maxim Nestratov]

14.04.2016 13:45, Nikolay Shirokovskiy пишет:


Our intention is to use disk bus and disk target name pair
as disk id instead of name returned by PrlVmDev_GetFriendlyName.
We already have the code that extracts this pair from vzsdk
data. Let's factor it out into a function.

Signed-off-by: Nikolay Shirokovskiy 
---
  src/vz/vz_sdk.c | 89 ++---
  1 file changed, 47 insertions(+), 42 deletions(-)


ACK

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

Re: [libvirt] RFC: Drop unmaintained hv drivers? (phyp, xenapi, hyperv)

[Eduardo Otubo]

2016-04-15 21:54 GMT+02:00 Cole Robinson :
> Hi all,
>
> There's a few old hypervisor drivers in the tree that haven't been actively
> maintained for a long time. I'm curious if anyone knows of these drivers being
> actively used. If not I think we should consider dropping them
>
>
> src/phyp/ : for power VM hypervisor. Added in July 2009. The last commit that
> looks like it wasn't either internal API conversion, or caught by code
> analysis, is:
>
> commit 41461ff7f7d6ee6679aae2a8004e305c5830c9e8
> Author: Eduardo Otubo 
> Date:   Tue Apr 19 12:34:08 2011 -0300
>
> PHYP: Adding reboot domain function
>
> Adding reboot  function for pHyp driver.
>
> Nearly 5 years ago. Eduardo is the primary driver author too (CCd at his email
> from github).
>
> Searching the upstream bug tracker for all bugs with 'phyp', the only one
> that's actually about the phyp driver is a report from 2 years ago that it
> crashes trying to open a connection:
>
> https://bugzilla.redhat.com/show_bug.cgi?id=1093094

I developed this driver as per internal IBM project's request. I left
IBM in 2014 and I really don't know if anyone uses this feature
(internally or not) anymore. I'll try to check it out and get back to
this thread as soon as I have some info.

Regards,

>
>
> src/xenapi/: Connecting to a xen api server. Added in March 2010. Largely
> appears to be a code drop, the original author/committer has never had another
> commit. The last xenapi specific commit seems to be:
>
> commit 484460ec4678a264c5e7355495c2f0da72cb42bd
> Author: Matthias Bolte 
> Date:   Thu Jul 21 15:16:11 2011 +0200
>
> xenapi: Improve error reporting in xenapiOpen once again
>
> Nearly 5 years ago. The only upstream bug that was filed about xenapi is:
>
> https://bugzilla.redhat.com/show_bug.cgi?id=711372
>
> Which was about a connection failure that was eventually fixed upstream, and
> dovetailed into the above referenced commit. Current xen guys, you know of
> anyone using this?
>
>
> src/hyperv/: Added in July 2011. This was largely a code drop as well;
> committed and patched a few times by Matthias but it was a university project
> by someone else. Last hyperv targeted patch was:
>
> commit 9e9ea3ead9825bd1dc2c17cea4abc8c4165591d0
> Author: Matthias Bolte 
> Date:   Sun Sep 9 17:39:40 2012 +0200
>
> hyperv: Fix and improve hypervListAllDomains
>
> The driver is fairly minimal as well: it can only list existing VMs and
> perform lifecycle operations. It can't create new VMs, and doesn't list VM
> device config AFAICT.
>
>
> Also, in general, I've never heard about anyone _actually_ using any of those
> drivers in the wild. There's reports here and there but it mostly sounds like
> people trying them out. Just an anecdote so take it with a grain of salt
>
> - Cole



-- 
otubo

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

Re: [libvirt] [PATCH] virDomain{Get, Set}PerfEvents: Tweak documentation

[Erik Skultety]

On 18/04/16 17:13, Michal Privoznik wrote:
> These API already support VIR_DOMAIN_AFFECT_* flags. But the
> documentation does not mention it. Eww.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/libvirt-domain.c | 18 --
>  1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
> index 3e144b6..4f473c9 100644
> --- a/src/libvirt-domain.c
> +++ b/src/libvirt-domain.c
> @@ -9695,11 +9695,13 @@ virDomainOpenChannel(virDomainPtr dom,
>   * @domain: a domain object
>   * @params: where to store perf events setting
>   * @nparams: number of items in @params
> - * @flags: extra flags; not used yet, so callers should always pass 0
> + * @flags: bitwise-OR of virDomainModificationImpact
>   *
> - * Get all perf events setting. Possible fields returned in @params are
> - * defined by VIR_DOMAIN_PERF_* macros and new fields will likely be
> - * introduced in the future.
> + * Get all Linux perf events setting. Possible fields returned in
> + * @params are defined by VIR_PERF_EVENT_* macros and new fields
> + * will likely be introduced in the future.
> + *
> + * Linux perf events are performance analyzing tool in Linux.
>   *
>   * Returns -1 in case of failure, 0 in case of success.
>   */
> @@ -9743,9 +9745,13 @@ int virDomainGetPerfEvents(virDomainPtr domain,
>   * @params: pointer to perf events parameter object
>   * @nparams: number of perf event parameters (this value can be the same
>   *   less than the number of parameters supported)
> - * @flags: extra flags; not used yet, so callers should always pass 0
> + * @flags: bitwise-OR of virDomainModificationImpact
>   *
> - * Enable or disable the particular list of perf events you care about.
> + * Enable or disable the particular list of Linux perf events you
> + * care about. The @params argument should contain any subset of
> + * VIR_PERF_EVENT_ macros.
> + *
> + * Linux perf events are performance analyzing tool in Linux.
>   *
>   * Returns -1 in case of error, 0 in case of success.
>   */
> 

ACK

Erik

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

Re: [libvirt] [PATCH v4 00/11] Introduce worker tuning APIs

[Erik Skultety]

On 18/04/16 17:04, Michal Privoznik wrote:
> On 18.04.2016 16:48, Erik Skultety wrote:
>>> You forgot to update man page for virt-admin. ACK series if you write
>>> some and squash it into the last patch.
>>>
>>> Michal
>>>
>>
>> Would squashing in the patch below work for you? Another thing is that
>> we don't have almost any command documented yet, we could do that later,
>> but since I decided to do that I didn't want to throw it away...
>>
> 
> Yeah, I realized that. Unfortunately after I hit 'Send' button.
> 
>> https://github.com/eskultety/libvirt/commit/ee566cecd82d2d9be0b3c56dfbf58b021489987f
> 
> This is definitely better. Please push.
> 
> Michal
>

Pushed, thank you for review.

Erik

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

[libvirt] [PATCH] virDomain{Get, Set}PerfEvents: Tweak documentation

[Michal Privoznik]

These API already support VIR_DOMAIN_AFFECT_* flags. But the
documentation does not mention it. Eww.

Signed-off-by: Michal Privoznik 
---
 src/libvirt-domain.c | 18 --
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
index 3e144b6..4f473c9 100644
--- a/src/libvirt-domain.c
+++ b/src/libvirt-domain.c
@@ -9695,11 +9695,13 @@ virDomainOpenChannel(virDomainPtr dom,
  * @domain: a domain object
  * @params: where to store perf events setting
  * @nparams: number of items in @params
- * @flags: extra flags; not used yet, so callers should always pass 0
+ * @flags: bitwise-OR of virDomainModificationImpact
  *
- * Get all perf events setting. Possible fields returned in @params are
- * defined by VIR_DOMAIN_PERF_* macros and new fields will likely be
- * introduced in the future.
+ * Get all Linux perf events setting. Possible fields returned in
+ * @params are defined by VIR_PERF_EVENT_* macros and new fields
+ * will likely be introduced in the future.
+ *
+ * Linux perf events are performance analyzing tool in Linux.
  *
  * Returns -1 in case of failure, 0 in case of success.
  */
@@ -9743,9 +9745,13 @@ int virDomainGetPerfEvents(virDomainPtr domain,
  * @params: pointer to perf events parameter object
  * @nparams: number of perf event parameters (this value can be the same
  *   less than the number of parameters supported)
- * @flags: extra flags; not used yet, so callers should always pass 0
+ * @flags: bitwise-OR of virDomainModificationImpact
  *
- * Enable or disable the particular list of perf events you care about.
+ * Enable or disable the particular list of Linux perf events you
+ * care about. The @params argument should contain any subset of
+ * VIR_PERF_EVENT_ macros.
+ *
+ * Linux perf events are performance analyzing tool in Linux.
  *
  * Returns -1 in case of error, 0 in case of success.
  */
-- 
2.7.3

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

[libvirt] [python PATCH] fix crash in getAllDomainStats

[Pavel Hrdina]

Commits 1d39dbaf and 827ed9b4 broke the libvirt-python API by removing
virDomainRef() and virDomainFree().  virDomainStatsRecordListFree() will
free that domain pointer and later when virDomain (python object) call
its destructor and tries to free that same pointer again.

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

Signed-off-by: Pavel Hrdina 
---
 libvirt-override.c | 8 +++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/libvirt-override.c b/libvirt-override.c
index 4640ed5..2de95ce 100644
--- a/libvirt-override.c
+++ b/libvirt-override.c
@@ -8381,6 +8381,7 @@ convertDomainStatsRecord(virDomainStatsRecordPtr *records,
 PyObject *py_retval;
 PyObject *py_record;
 PyObject *py_record_stats = NULL;
+virDomainPtr dom = NULL;
 size_t i;
 
 if (!(py_retval = PyList_New(nrecords)))
@@ -8392,9 +8393,12 @@ convertDomainStatsRecord(virDomainStatsRecordPtr 
*records,
 
 VIR_PY_LIST_SET_GOTO(py_retval, i, py_record, error);
 
+dom = records[i]->dom;
+virDomainRef(dom);
 VIR_PY_TUPLE_SET_GOTO(py_record, 0,
-  libvirt_virDomainPtrWrap(records[i]->dom),
+  libvirt_virDomainPtrWrap(dom),
   error);
+dom = NULL;
 
 if (!(py_record_stats = getPyVirTypedParameter(records[i]->params,
records[i]->nparams)))
@@ -8406,6 +8410,8 @@ convertDomainStatsRecord(virDomainStatsRecordPtr *records,
 return py_retval;
 
  error:
+if (dom)
+virDomainFree(dom);
 Py_XDECREF(py_retval);
 return NULL;
 }
-- 
2.8.1

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

Re: [libvirt] [PATCH v4 00/11] Introduce worker tuning APIs

[Michal Privoznik]

On 18.04.2016 16:48, Erik Skultety wrote:
>> You forgot to update man page for virt-admin. ACK series if you write
>> some and squash it into the last patch.
>>
>> Michal
>>
> 
> Would squashing in the patch below work for you? Another thing is that
> we don't have almost any command documented yet, we could do that later,
> but since I decided to do that I didn't want to throw it away...
> 

Yeah, I realized that. Unfortunately after I hit 'Send' button.

> https://github.com/eskultety/libvirt/commit/ee566cecd82d2d9be0b3c56dfbf58b021489987f

This is definitely better. Please push.

Michal

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

Re: [libvirt] [PATCH v4 00/11] Introduce worker tuning APIs

[Erik Skultety]

> You forgot to update man page for virt-admin. ACK series if you write
> some and squash it into the last patch.
> 
> Michal
>

Would squashing in the patch below work for you? Another thing is that
we don't have almost any command documented yet, we could do that later,
but since I decided to do that I didn't want to throw it away...

https://github.com/eskultety/libvirt/commit/ee566cecd82d2d9be0b3c56dfbf58b021489987f

Regards,
Erik

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

Re: [libvirt] [PATCH] Libvirt: virTypedParamsValidate: Fix detection of multiple parameters

[Peter Krempa]

On Tue, Apr 12, 2016 at 10:13:27 -0400, Jason J. Herne wrote:
> virTypedParamsValidate currently uses an index based check to find
> duplicate parameters. This check does not work. Consider the following
> simple example:
> 
> We have only 2 keys
> A  (multiples allowed)
> B  (multiples NOT allowed)
> 
> We are given the following list of parameters to check:
> A
> A
> B
> 
> If you work through the validation loop you will see that our last iteration
> through the loop has i=2 and j=1. In this case, i > j and keys[j].value.i will
> indicate that multiples are not allowed. Both conditionals are satisfied so
> an incorrect error will be given: "parameter '%s' occurs multiple times"
> 
> This patch replaces the index based check with code that remembers
> the name of the last parameter seen and only triggers the error case if
> the current parameter name equals the last one. This works because the
> list is sorted and duplicate parameters will be grouped together.
> 
> In reality, we hit this bug while using selective block migration to migrate
> a guest with 5 disks. 5 was apparently just the right number to push i > j
> and hit this bug.
> 
> virsh migrate --live guestname --copy-storage-all
>   --migrate-disks vdb,vdc,vdd,vde,vdf
>   qemu+ssh://dsthost/system
> 
> Signed-off-by: Jason J. Herne 
> Reviewed-by: Eric Farman 
> ---
>  src/util/virtypedparam.c | 6 --
>  1 file changed, 4 insertions(+), 2 deletions(-)

I've added a test case to cover the problem to
tests/virtypedparamtest.c and pushed this patch. Thanks for taking time
to debug the issue.

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 18/18] event-test: Enforce domain event sync

[Ján Tomko]

On Thu, Apr 14, 2016 at 05:52:43PM +0200, Peter Krempa wrote:
> Use verify to force adding new events by means of static assertions.
> ---
>  examples/object-events/event-test.c | 5 +
>  1 file changed, 5 insertions(+)
> 

ACK

Jan

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

Re: [libvirt] [PATCH 17/18] event-test: Add VIR_DOMAIN_EVENT_ID_IO_ERROR_REASON

[Ján Tomko]

On Thu, Apr 14, 2016 at 05:52:42PM +0200, Peter Krempa wrote:
> When adding the static check I've noticed that one other event is
> missing.
> ---
>  examples/object-events/event-test.c | 19 +++
>  1 file changed, 19 insertions(+)
> 

ACK

Jan

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

Re: [libvirt] [PATCH 16/18] event-test: Add VIR_DOMAIN_EVENT_ID_DEVICE_REMOVAL_FAILED

[Ján Tomko]

On Thu, Apr 14, 2016 at 05:52:41PM +0200, Peter Krempa wrote:
> ---
>  examples/object-events/event-test.c | 15 ++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 

ACK

Jan

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

Re: [libvirt] [PATCH 15/18] event-test: Add VIR_DOMAIN_EVENT_ID_JOB_COMPLETED

[Ján Tomko]

On Thu, Apr 14, 2016 at 05:52:40PM +0200, Peter Krempa wrote:
> ---
>  examples/object-events/event-test.c | 45 
> -
>  1 file changed, 34 insertions(+), 11 deletions(-)
> 

ACK

Jan

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

Re: [libvirt] [PATCH 14/18] event-test: Add VIR_DOMAIN_EVENT_ID_MIGRATION_ITERATION callback

[Ján Tomko]

On Thu, Apr 14, 2016 at 05:52:39PM +0200, Peter Krempa wrote:
> ---
>  examples/object-events/event-test.c | 14 +-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 

ACK

Jan

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

Re: [libvirt] [PATCH 13/18] event-test: Add VIR_DOMAIN_EVENT_ID_BLOCK_JOB and VIR_DOMAIN_EVENT_ID_BLOCK_JOB_2

[Ján Tomko]

On Thu, Apr 14, 2016 at 05:52:38PM +0200, Peter Krempa wrote:
> ---
>  examples/object-events/event-test.c | 71 
> +++--
>  1 file changed, 69 insertions(+), 2 deletions(-)
> 

ACK

Jan

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

Re: [libvirt] [PATCH 12/18] event-test: make domain event registration declarative

[Ján Tomko]

On Thu, Apr 14, 2016 at 05:52:37PM +0200, Peter Krempa wrote:
> Rather than copying loads of ugly code, let's help out by a few C
> tricks.
> ---
>  examples/object-events/event-test.c | 200 
> +++-
>  1 file changed, 62 insertions(+), 138 deletions(-)
> 

ACK

Jan

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

Re: [libvirt] RFC: Drop unmaintained hv drivers? (phyp, xenapi, hyperv)

[Cole Robinson]

On 04/18/2016 08:58 AM, Daniel P. Berrange wrote:
> On Mon, Apr 18, 2016 at 08:54:15AM -0400, Cole Robinson wrote:
>> On 04/18/2016 05:21 AM, Daniel P. Berrange wrote:
>>> On Fri, Apr 15, 2016 at 03:54:00PM -0400, Cole Robinson wrote:
>>>
>>>
>>> In general I'm not really in favour of dropping virt drivers for hypervisor
>>> platforms that still exist and have potential users, even if we don't hear
>>> much about them. I don't think any of these drivers is really giving us a
>>> significant maintenance headache to justify deletion.
>>>
>>
>> It's just kind of a weird situation. We advertise support for these 
>> platforms,
>> but if someone tries them and finds them lacking and reports an issue here,
>> the only response they are going to get is 'no one has worked on it for 
>> years,
>> no one here has a setup to test, sorry we cannot help you'. That's if they 
>> get
>> a response at all. Seems bizarre to me...
> 
> This is implying that because we don't have an active maintainer who will
> answer bug reports, then the driver is entirely useless to everybody. I
> find that really dubious, because it ignores the possibility that people
> are happily using the other parts of it that actually do work. Sure it
> would be nice if we have active maintainers, but I don't think the lack
> of an active maintainer means the driver is useless to all users.

My paragraph didn't fully describe all the assumptions; Those three drivers
may have zero active users in the wild. Certainly there is no obvious evidence
to the contrary. The most damning bit seems to be that there hasn't been a
single targeted bug fix for those drivers in an average of 4 years; if someone
was actively using those drivers I'm pretty sure we would have received a
drive by patch at some point

Maybe someone will chime in to prove me wrong, but what if the situation is
that there's no active maintainer, and no active users? Then it truly _is_
serving no purpose, and IMO even advertising it is actively harmful since it
may lead users down a dead end path

Removing a driver doesn't need to be the end either... if someone motivated
shows up they can always revive the old code

...now that I do some more targeted searches looking for patches, looks like
someone from bull.net did try to massively expand the hyperv driver a few
years back, but it was an awkward code dump:

https://www.redhat.com/archives/libvir-list/2014-October/msg00257.html

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

Re: [libvirt] RFC: Drop unmaintained hv drivers? (phyp, xenapi, hyperv)

[Daniel P. Berrange]

On Mon, Apr 18, 2016 at 08:54:15AM -0400, Cole Robinson wrote:
> On 04/18/2016 05:21 AM, Daniel P. Berrange wrote:
> > On Fri, Apr 15, 2016 at 03:54:00PM -0400, Cole Robinson wrote:
> > 
> > 
> > In general I'm not really in favour of dropping virt drivers for hypervisor
> > platforms that still exist and have potential users, even if we don't hear
> > much about them. I don't think any of these drivers is really giving us a
> > significant maintenance headache to justify deletion.
> > 
> 
> It's just kind of a weird situation. We advertise support for these platforms,
> but if someone tries them and finds them lacking and reports an issue here,
> the only response they are going to get is 'no one has worked on it for years,
> no one here has a setup to test, sorry we cannot help you'. That's if they get
> a response at all. Seems bizarre to me...

This is implying that because we don't have an active maintainer who will
answer bug reports, then the driver is entirely useless to everybody. I
find that really dubious, because it ignores the possibility that people
are happily using the other parts of it that actually do work. Sure it
would be nice if we have active maintainers, but I don't think the lack
of an active maintainer means the driver is useless to all users.

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 v3] security: Rename DomainSetDirLabel to DomainSetPathLabel

[Cole Robinson]

On 04/18/2016 02:41 AM, Martin Kletzander wrote:
> It already labels abritrary paths, so it's just the naming that was
> wrong.
> 
> Signed-off-by: Martin Kletzander 

ACK

Thanks,
Cole

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

Re: [libvirt] [PATCH] qemu: Label master key file

[Cole Robinson]

On 04/18/2016 02:34 AM, Martin Kletzander wrote:
> On Fri, Apr 15, 2016 at 12:16:18PM -0400, Cole Robinson wrote:
>> On 04/14/2016 03:04 AM, Martin Kletzander wrote:
>>> On Wed, Apr 13, 2016 at 07:58:06PM -0400, Cole Robinson wrote:
 On 04/13/2016 11:56 AM, Cole Robinson wrote:
> On 04/13/2016 11:17 AM, Martin Kletzander wrote:
>> When creating the master key, we used mode 0600 (which we should) but
>> because we were creating it as root, the file is not readable by any
>> qemu running as non-root.  Fortunately, it's just a matter of labelling
>> the file.  We are generating the file path few times already, so let's
>> label it in the same function that has access to the path already.
>>
>> Signed-off-by: Martin Kletzander 
>> ---
>>  src/qemu/qemu_domain.c  | 15 ---
>>  src/qemu/qemu_domain.h  |  3 ++-
>>  src/qemu/qemu_process.c |  2 +-
>>  3 files changed, 15 insertions(+), 5 deletions(-)
>>
>
> ACK, makes sense and fixes things for me. One comment below
>
>> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>> index 5d54fffcfb98..83e765ef6868 100644
>> --- a/src/qemu/qemu_domain.c
>> +++ b/src/qemu/qemu_domain.c
>> @@ -504,11 +504,13 @@ qemuDomainGetMasterKeyFilePath(const char *libDir)
>>   * Returns 0 on success, -1 on failure with error message indicating
>> failure
>>   */
>>  static int
>> -qemuDomainWriteMasterKeyFile(qemuDomainObjPrivatePtr priv)
>> +qemuDomainWriteMasterKeyFile(virQEMUDriverPtr driver,
>> + virDomainObjPtr vm)
>>  {
>>  char *path;
>>  int fd = -1;
>>  int ret = -1;
>> +qemuDomainObjPrivatePtr priv = vm->privateData;
>>
>>  if (!(path = qemuDomainGetMasterKeyFilePath(priv->libDir)))
>>  return -1;
>> @@ -525,6 +527,10 @@ qemuDomainWriteMasterKeyFile(qemuDomainObjPrivatePtr
>> priv)
>>  goto cleanup;
>>  }
>>
>> +if (virSecurityManagerDomainSetDirLabel(driver->securityManager,
>> +vm->def, path) < 0)
>> +goto cleanup;
>> +
>>  ret = 0;
>>
>
> I looked briefly at fixing this but know if there was a function to ask 
> the
> security driver 'just set a on this arbitrary path'. I saw DirLabel but 
> was
> thrown off by the 'Dir' name. Maybe change it to something more generic?
>
>>>
>>> Yes, it's just a naming; I had to add it for similar purpose when
>>> labelling directories, but it "Just Works" for arbitrary path.  I'll
>>> rename that.
>>>

 Also adding some CC, I'm guessing virt-aa-helper.c needs to be extended to 
 to
 allow access to $libDir/master-key.aes

>>>
>>> Actually, it shouldn't.  It's in the per-domain directory and
>>> everything under that should be available.
>>>
>>> Anyway, it's a pity that we're not very likely to have a test case for
>>> that.  But couldn't the paths in virt-aa-helper be created from a
>>> security driver?  It knows all the paths, doesn't it?
>>>
>>> I'll send a v2 with the rename.
>>
>> Laine was hitting issues with this too, so I pushed this patch. The API 
>> rename
>> isn't blocking anyone
>>
> 
> v2 was alrady on the list, but it can be done the other way around.
> I'll send the rename rebased now so it's easier to review.
> 

Oh sorry, I must have missed it? Though looking through the archives I don't
see anything, only the original patch and the v3. Maybe it didn't hit the 
list...

- Cole

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

Re: [libvirt] RFC: Drop unmaintained hv drivers? (phyp, xenapi, hyperv)

[Cole Robinson]

On 04/18/2016 05:21 AM, Daniel P. Berrange wrote:
> On Fri, Apr 15, 2016 at 03:54:00PM -0400, Cole Robinson wrote:
>> Hi all,
>>
>> There's a few old hypervisor drivers in the tree that haven't been actively
>> maintained for a long time. I'm curious if anyone knows of these drivers 
>> being
>> actively used. If not I think we should consider dropping them
>>
>>
>> src/phyp/ : for power VM hypervisor. Added in July 2009. The last commit that
>> looks like it wasn't either internal API conversion, or caught by code
>> analysis, is:
>>
>> commit 41461ff7f7d6ee6679aae2a8004e305c5830c9e8
>> Author: Eduardo Otubo 
>> Date:   Tue Apr 19 12:34:08 2011 -0300
>>
>> PHYP: Adding reboot domain function
>>
>> Adding reboot  function for pHyp driver.
>>
>> Nearly 5 years ago. Eduardo is the primary driver author too (CCd at his 
>> email
>> from github).
>>
>> Searching the upstream bug tracker for all bugs with 'phyp', the only one
>> that's actually about the phyp driver is a report from 2 years ago that it
>> crashes trying to open a connection:
>>
>> https://bugzilla.redhat.com/show_bug.cgi?id=1093094
> 
> The phyp driver would have a fairly niche end user audience, so it would
> not be entirely surprising to not hear much about it.
> 
> 
>> src/xenapi/: Connecting to a xen api server. Added in March 2010. Largely
>> appears to be a code drop, the original author/committer has never had 
>> another
>> commit. The last xenapi specific commit seems to be:
>>
>> commit 484460ec4678a264c5e7355495c2f0da72cb42bd
>> Author: Matthias Bolte 
>> Date:   Thu Jul 21 15:16:11 2011 +0200
>>
>> xenapi: Improve error reporting in xenapiOpen once again
>>
>> Nearly 5 years ago. The only upstream bug that was filed about xenapi is:
>>
>> https://bugzilla.redhat.com/show_bug.cgi?id=711372
>>
>> Which was about a connection failure that was eventually fixed upstream, and
>> dovetailed into the above referenced commit. Current xen guys, you know of
>> anyone using this?
> 
> The XenAPI driver was never "finished" as IIRC the person who started work
> on it, left XenSource before he was able to make it functional enough to
> be useful. The XenAPI was targetting the closed source Xen products, as
> opposed to the open source platform, which is now libxl based. I did vaguely
> remember talk of the Xen products eventualy moving over to be libxl based
> instead of XenAPi, but I don't know if that's actually accurate or not.
> 
>> src/hyperv/: Added in July 2011. This was largely a code drop as well;
>> committed and patched a few times by Matthias but it was a university project
>> by someone else. Last hyperv targeted patch was:
>>
>> commit 9e9ea3ead9825bd1dc2c17cea4abc8c4165591d0
>> Author: Matthias Bolte 
>> Date:   Sun Sep 9 17:39:40 2012 +0200
>>
>> hyperv: Fix and improve hypervListAllDomains
>>
>> The driver is fairly minimal as well: it can only list existing VMs and
>> perform lifecycle operations. It can't create new VMs, and doesn't list VM
>> device config AFAICT.
> 
> This is a fairly similar level of functionality to the XenAPI driver, but
> I'm pretty loathe for us to loose even the limited hyperv support, as it
> is very  much an active product that's not going away.
> 
> 
> In general I'm not really in favour of dropping virt drivers for hypervisor
> platforms that still exist and have potential users, even if we don't hear
> much about them. I don't think any of these drivers is really giving us a
> significant maintenance headache to justify deletion.
> 

It's just kind of a weird situation. We advertise support for these platforms,
but if someone tries them and finds them lacking and reports an issue here,
the only response they are going to get is 'no one has worked on it for years,
no one here has a setup to test, sorry we cannot help you'. That's if they get
a response at all. Seems bizarre to me...

- Cole

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

Re: [libvirt] [PATCH v4 0/2] persistent live migration with specified XML

[Jiri Denemark]

On Thu, Mar 17, 2016 at 19:31:43 +0300, Dmitry Andreev wrote:
> v4: wrong param name in commit msg
> 
> v3:
>  - use shorter name for param and rename args
>  - move qemuMigrationCookieAddPersistent out from
>qemuMigrationBakeCookie
>  - rebase to master
> 
> v2: reimplemented with new migration param

Pushed now with the suggested changes for 2/2.

Jirka

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

Re: [libvirt] [PATCH 11/18] event-test: Enable enum sentinels and warn on not fully populated enums

[Ján Tomko]

The summary does not make sense. How about:

event-test: warn on unhandled enum values

On Thu, Apr 14, 2016 at 05:52:36PM +0200, Peter Krempa wrote:
> Avoid forgetting to add the correct fiels to the event-test.

*fields

or even:

to the switches in event-test.

> ---
>  examples/Makefile.am|  5 
>  examples/object-events/event-test.c | 54 
> +
>  2 files changed, 59 insertions(+)
> 

ACK

Jan

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

Re: [libvirt] [PATCH 10/18] event-test: make few switch statements future proof

[Ján Tomko]

On Thu, Apr 14, 2016 at 05:52:35PM +0200, Peter Krempa wrote:
> Make them return "uknown" for invalid values without breaking compiler
> checks to add new values.
> ---
>  examples/object-events/event-test.c | 226 
> +---
>  1 file changed, 108 insertions(+), 118 deletions(-)
> 

ACK

Jan

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

Re: [libvirt] [PATCH] qemu: reject min_guarantee at parse time

[Cole Robinson]

On 04/16/2016 08:54 AM, Martin Kletzander wrote:
> On Fri, Apr 15, 2016 at 05:37:17PM -0400, Cole Robinson wrote:
>> min_guarantee isn't implemented for qemu, and an explicit check was
>> added in june 2014 to reject the VM at qemu startup time. It's a weird
>> place to do XML validation, so move it to the post parse area where
>> we have similar checks.
>>
> 
> NACK, it's done precisely there because if there is any domain that has
> that parameter already in, it will disappear after upgrade to libvirt
> with this patch.  That's what we have that validation function for and
> the reason why it is called when the domain is being started.
> 
> We have bunch of similar issues that I wanted to address globally, but
> it doesn't look like it will work in near future.
> 

I see what you mean. In this case it's a bit fuzzy since that particular check
has been there since Aug 2014, so I strongly doubt there's any configs in
practice that have that param set on anything recent. But it's a fairly
pedantic point, I'll change the patch to add a comment about the deliberate
location choice.

Thanks,
Cole

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

Re: [libvirt] [PATCH 09/18] event-test: Use switch instead of if/else if chains for lifecycle event translation

[Ján Tomko]

On Thu, Apr 14, 2016 at 05:52:34PM +0200, Peter Krempa wrote:
> ---
>  examples/object-events/event-test.c | 22 --
>  1 file changed, 16 insertions(+), 6 deletions(-)
> 

ACK

Jan

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

Re: [libvirt] [PATCH 08/18] event-test: Use typecasted enum to convert graphics event phase

[Ján Tomko]

On Thu, Apr 14, 2016 at 05:52:33PM +0200, Peter Krempa wrote:
> ---
>  examples/object-events/event-test.c | 31 ---
>  1 file changed, 20 insertions(+), 11 deletions(-)
> 

ACK

> diff --git a/examples/object-events/event-test.c 
> b/examples/object-events/event-test.c
> index 22bd706..e1ad990 100644
> --- a/examples/object-events/event-test.c
> +++ b/examples/object-events/event-test.c
> @@ -386,6 +386,24 @@ myDomainEventIOErrorCallback(virConnectPtr conn 
> ATTRIBUTE_UNUSED,
>  }
> 
> 
> +static const char *
> +graphicsPhaseToStr(int phase)
> +{
> +switch ((virDomainEventGraphicsPhase) phase) {
> +case VIR_DOMAIN_EVENT_GRAPHICS_CONNECT:
> +return "connected ";

Adding the space in the caller would be neater.

> +
> +case VIR_DOMAIN_EVENT_GRAPHICS_INITIALIZE:
> +return "initialized ";
> +
> +case VIR_DOMAIN_EVENT_GRAPHICS_DISCONNECT:
> +return "disconnected ";
> +}
> +
> +return "unknown";

This string does not have a trailing space.

> +}
> +
> +
>  static int
>  myDomainEventGraphicsCallback(virConnectPtr conn ATTRIBUTE_UNUSED,
>virDomainPtr dom,
> @@ -400,17 +418,8 @@ myDomainEventGraphicsCallback(virConnectPtr conn 
> ATTRIBUTE_UNUSED,
>  printf("%s EVENT: Domain %s(%d) graphics ", __func__, 
> virDomainGetName(dom),
> virDomainGetID(dom));
> 
> -switch (phase) {
> -case VIR_DOMAIN_EVENT_GRAPHICS_CONNECT:
> -printf("connected ");
> -break;
> -case VIR_DOMAIN_EVENT_GRAPHICS_INITIALIZE:
> -printf("initialized ");
> -break;
> -case VIR_DOMAIN_EVENT_GRAPHICS_DISCONNECT:
> -printf("disconnected ");
> -break;
> -}
> +printf("%s", graphicsPhaseToStr(phase));
> +

Why the extra newline?

Jan

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

Re: [libvirt] [PATCH 07/18] event-test: Force compiler check in switch for connectClose callback

[Ján Tomko]

On Thu, Apr 14, 2016 at 05:52:32PM +0200, Peter Krempa wrote:
> ---
>  examples/object-events/event-test.c | 21 -
>  1 file changed, 12 insertions(+), 9 deletions(-)
> 

ACK

Jan

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

Re: [libvirt] [PATCH 06/18] lib: document fields virConnectDomainEventDiskChangeReason

[Ján Tomko]

On Thu, Apr 14, 2016 at 05:52:31PM +0200, Peter Krempa wrote:
> ---
>  include/libvirt/libvirt-domain.h | 6 +-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 

ACK

Jan

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

Re: [libvirt] RFC: virtio-rng and /dev/urandom

[Daniel P. Berrange]

On Mon, Apr 18, 2016 at 01:07:40PM +0200, Hubert Kario wrote:
> On Monday 18 April 2016 02:46:19 H. Peter Anvin wrote:
> > Another thing that really needs to be addressed, but is a separate
> > issue: invalidating and reseeding the entropy pool after a snapshot
> > event.
> 
> definitely agreed
> 
> though just reseeding would be sufficient - the goal is to make the 
> output unpredictable and unique between multiple machines starting from 
> the same snapshot, feeding enough random data to make the entropy pool 
> unique again is sufficient to achieve that

If you're spawning multiple machines from the same base snapshot,
the seeding of RNG is just one of many many things that need
dealing with. eg new /etc/machine-id, new ssh host keys, changing
MAC address of NICs with corresponding guest config file changes,
many other application specific identifiers / keys intended to
be unique per machine.  As such, libvirt explicitly tries to
prevent you spawning multiple machines from the same snapshot.

That all said, Microsoft HyperV has defined a concept of a
"Virtual Machine Generation ID" and specified various hypervisor
operations which should result in this value changing[1]. For example
restoring from a snapshot should always change the genid, as would
restoring from backup, or cloned from another image, or failed over
during disaster recovery.

This vm genid is exposed to the guest via ACPI and there's an
notification whenever it changes.

There are patches for QEMU[2] to support this feature in a manner that
is compatible with the hyperv spec, but they are sadly still not
merged :-(

So it would be possible for the Linux kernel to re-initialize its
RNG after snapshot by hooking into the vm-genid ACPI notification.


Regards,
Daniel

[1] https://lists.nongnu.org/archive/html/qemu-devel/2014-10/msg00489.html
[2] https://lists.gnu.org/archive/html/qemu-devel/2016-01/msg05599.html
-- 
|: 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: virtio-rng and /dev/urandom

[Hubert Kario]

On Sunday 17 April 2016 17:27:05 H. Peter Anvin wrote:
> On 04/16/16 01:31, Paolo Bonzini wrote:
> > Right, but there's always the point about people that use
> > heterogeneous hosts and cannot pass rdrand/rdseed to the guest. 
> > For these, we should add a QEMU driver that uses rdrand/rdseed, and
> > thus decouples virtio-rng from the host /dev/* completely.
> > 
> > From the libvirt POV there are various possibilities:
> > 
> > - Libvirt can have a libvirt.conf parameter that says "ignore
> > whatever is specified in the guest XML if rdrand/rdseed is
> > available, and instead use rdrand/rdseed".
> > 
> > - Libvirt can allow specifying rdrand/rdseed _and_ an additional
> > backend,> 
> > like this:
> > 
> > /dev/random
> > 
> > and fallback to the second if rdrand/rdseed are not available.
> 
> The other thing, and this is one area where there is some legitimacy
> to the /dev/urandom argument: on a fresh boot, it would be highly
> desirable to get a seed value from virtio-rng even if that is
> "entropyless".  The backwards-compatible way would be to provide,
> say, 64 bytes of /dev/urandom before switching to /dev/random, but it
> might be desirable to give the guest OS some way to cause that to
> reset, explicitly requesting a new seed after an in-VM guest reboot,
> kexec et al.

it's unnecessary complex, which means it is more likely to have bugs in 
it

besides, it's still feeding CSPRNG output to CSPRNG, no matter if it 
reads the bits from /dev/random or /dev/urandom

kernel will not provide you with raw random values it gathered

so again, why block users from setting the randomness source to value 
they think is sufficient for their use case?

-- 
Regards,
Hubert Kario
Senior Quality Engineer, QE BaseOS Security team
Web: www.cz.redhat.com
Red Hat Czech s.r.o., Purkyňova 99/71, 612 45, Brno, Czech Republic

signature.asc
Description: This is a digitally signed message part.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 05/18] event-test: Use functions with typecasted switch to convert enums

[Ján Tomko]

On Thu, Apr 14, 2016 at 05:52:30PM +0200, Peter Krempa wrote:
> Arrays would induce crash if a new value was introduced without adding
> it here. This could happen for
> VIR_DOMAIN_EVENT_DISK_DROP_MISSING_ON_START
> ---
>  examples/object-events/event-test.c | 37 
> -
>  1 file changed, 28 insertions(+), 9 deletions(-)
> 

> +static const char *
> +diskChangeReasonToStr(int reason)

diskChangeReasonToString would looks nicer, but Str is readable enough,
thanks to all the std funcs in libc.

ACK

Jan

> +{
> +switch ((virConnectDomainEventDiskChangeReason) reason) {
> +case VIR_DOMAIN_EVENT_DISK_CHANGE_MISSING_ON_START:
> +return "disk empty due to startupPolicy";
> +
> +case VIR_DOMAIN_EVENT_DISK_DROP_MISSING_ON_START:
> +return "disk dropped due to startupPolicy";
> +}
> +
> +return "unknown";
> +}
> +

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

Re: [libvirt] [PATCH 04/18] event-test: touch up coding style

[Ján Tomko]

On Thu, Apr 14, 2016 at 05:52:29PM +0200, Peter Krempa wrote:
> Break long lines and format headers correctly.
> ---
>  examples/object-events/event-test.c | 227 
> ++--
>  1 file changed, 137 insertions(+), 90 deletions(-)
> 

ACK

Jan

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

Re: [libvirt] [PATCH 03/18] event-test: Remove unnecessary 'usage' function

[Ján Tomko]

On Thu, Apr 14, 2016 at 05:52:28PM +0200, Peter Krempa wrote:
> ---
>  examples/object-events/event-test.c | 8 +---
>  1 file changed, 1 insertion(+), 7 deletions(-)
> 

ACK

Jan

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

Re: [libvirt] [PATCH 02/18] event-test: Get rid of useless and ambiguous VIR_DEBUG macro

[Ján Tomko]

On Thu, Apr 14, 2016 at 05:52:27PM +0200, Peter Krempa wrote:
> The event test does not try to include libvirt internals. Using a macro
> named VIR_DEBUG might hint to such usage. Additionally it's useless
> since it's used only in the main() function.
> 
> Modernize the message strings while touching them.
> ---
>  examples/object-events/event-test.c | 9 -
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 

ACK

Jan

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

Re: [libvirt] RFC: virtio-rng and /dev/urandom

[Hubert Kario]

On Monday 18 April 2016 02:46:19 H. Peter Anvin wrote:
> Another thing that really needs to be addressed, but is a separate
> issue: invalidating and reseeding the entropy pool after a snapshot
> event.

definitely agreed

though just reseeding would be sufficient - the goal is to make the 
output unpredictable and unique between multiple machines starting from 
the same snapshot, feeding enough random data to make the entropy pool 
unique again is sufficient to achieve that
-- 
Regards,
Hubert Kario
Senior Quality Engineer, QE BaseOS Security team
Web: www.cz.redhat.com
Red Hat Czech s.r.o., Purkyňova 99/71, 612 45, Brno, Czech Republic

signature.asc
Description: This is a digitally signed message part.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [python PATCH] event: Add support VIR_DOMAIN_EVENT_ID_DEVICE_REMOVAL_FAILED

[Pavel Hrdina]

On Mon, Apr 18, 2016 at 12:54:39PM +0200, Peter Krempa wrote:
> ---
>  examples/event-test.py |  4 +++
>  libvirt-override-virConnect.py |  9 +++
>  libvirt-override.c | 60 
> ++
>  3 files changed, 73 insertions(+)

ACK, Pavel

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

Re: [libvirt] [PATCH 01/18] event-test: Remove forward declarations

[Ján Tomko]

On Thu, Apr 14, 2016 at 05:52:26PM +0200, Peter Krempa wrote:
> Most of the functions are no longer in this file. 'usage' does not need
> a declaration.
> ---
>  examples/object-events/event-test.c | 21 -
>  1 file changed, 21 deletions(-)
> 

ACK

Jan

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

Re: [libvirt] RFC: virtio-rng and /dev/urandom

[Hubert Kario]

On Friday 15 April 2016 17:51:36 H. Peter Anvin wrote:
> On April 15, 2016 9:10:44 AM PDT, Hubert Kario  
wrote:
> >On Friday 15 April 2016 09:47:51 Eric Blake wrote:
> >> On 04/15/2016 04:41 AM, Cole Robinson wrote:
> >> > Libvirt currently rejects using host /dev/urandom as an input
> >
> >source
> >
> >> > for a virtio-rng device. The only accepted sources are
> >> > /dev/random
> >> > and /dev/hwrng. This is the result of discussions on qemu-devel
> >> > around when the feature was first added (2013). Examples:
> >> > 
> >> > http://lists.gnu.org/archive/html/qemu-devel/2012-09/msg02387.htm
> >> > l
> >
> >https://lists.gnu.org/archive/html/qemu-devel/2013-03/threads.html#0
> >
> >> > 0023
> >> > 
> >> > libvirt's rejection of /dev/urandom has generated some complaints
> >> > from users:
> >> > 
> >> > https://bugzilla.redhat.com/show_bug.cgi?id=1074464
> >> > * cited: http://www.2uo.de/myths-about-urandom/
> >> > http://www.redhat.com/archives/libvir-list/2016-March/msg01062.ht
> >> > ml
> >> > http://www.redhat.com/archives/libvir-list/2016-April/msg00186.ht
> >> > ml
> >> > 
> >> > I think it's worth having another discussion about this, at least
> >> > with a recent argument in one place so we can put it to bed. I'm
> >> > CCing a bunch of people. I think the questions are:
> >> > 
> >> > 1) is the original recommendation to never use
> >> > virtio-rng+/dev/urandom correct?
> >> 
> >> That I'm not sure about - and the answer may be context-dependent
> >
> >(for
> >
> >> example a FIPS user may care more than an ordinary user)
> >
> >/dev/urandom use is FIPS compliant, no FIPS-validated protocol or
> >cryptographic primitive requires the "fresh" entropy provided by
> >/dev/random. All primitives are designed to work with weaker entropy
> >guarantees than what /dev/urandom provides.
> 
> So: using urandom for a seed makes sense, but "unplugging the drain"
> is a huge waste of resources and provides absolutely zero value.

Since when "wasting resources" is worse than performing Denial of 
Service on your own infrastructure?

Besides, what's the difference between spinning a CSPRNG in host rather 
that guest? If anything, spinning CSPRNG in host is less of a waste as 
the virtualisation overhead (however small) isn't there. If you need X 
number of random bytes, you need to provide X number of random bytes. 
The software simply won't work otherwise.

> Also, I do not believe /dev/urandom is FIPS compliant.  Finally, the
> refill policy is different, so it is not really true the algorithm is
> the same.

We did discuss it with NIST, have you?

The refill policy doesn't matter, after the pool is seeded, it will 
continue generating unpredictable random numbers for years (if not 
decades or centuries) without any additional entropy. And you certainly 
will gather enough entropy to reseed /dev/urandom multiple times an 
hour, even if the host does not do anything but generate random numbers.

> All in all, other than a seed value it really doesn't make any sense. 
> Of course, none of this matters on newer Intel hardware ;)

Not everybody is running on newer Intel, not everybody is even running 
on x86_64 architecture. Not everybody trusts the RNG in Intel hardware 
(e.g. rdrand is a not-Approved algorithm for FIPS certified software).
-- 
Regards,
Hubert Kario
Senior Quality Engineer, QE BaseOS Security team
Web: www.cz.redhat.com
Red Hat Czech s.r.o., Purkyňova 99/71, 612 45, Brno, Czech Republic

signature.asc
Description: This is a digitally signed message part.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [libvirt-glib v2] spec: Add verification of the tarball GPG signature

[Christophe Fergeau]

This at least allows to make sure that all tarballs are signed with the
same GPG key, and that the tarball was not corrupted between the time it
was uploaded upstream, and the time the RPM is built.

danpb-BE86EBB415104FDF.gpg is generated with:
gpg2 -v --armor --export 15104FDF | gpg2 --no-default-keyring --keyring 
./danpb-BE86EBB415104FDF.gpg --import

We cannot unconditionally enable gpg signature checks as when
building from tarballs with rpmbuild -ta (for example), the needed
keyring file will no be available, so this commit checks that
BE86EBB415104FDF.gpg exists before attempting to do the check.
---
 libvirt-glib.spec.in | 17 +
 1 file changed, 17 insertions(+)

Hey, here is my attempt at addressing the issue raised with v1 (rpmbuild -ta
being broken by the change). I cannot rely on the usual test on %{fedora} and 
%{rhel} as
they are set when running rpmbuild on a fedora. Instead I added a test for the
existence of the keyring file. Maybe there are better ways of writing this file 
existence
check..

Christophe

diff --git a/libvirt-glib.spec.in b/libvirt-glib.spec.in
index 32ce4f0..02a27d5 100644
--- a/libvirt-glib.spec.in
+++ b/libvirt-glib.spec.in
@@ -1,5 +1,12 @@
 # -*- rpm-spec -*-
 
+# We cannot unconditionally enable gpg signature checks as when
+# building from tarballs with rpmbuild -ta (for example), the needed
+# keyring file will no be available
+%define gpg_keyring danpb-BE86EBB415104FDF.gpg
+%define has_gpg_keyring %(if [ -f %{gpg_keyring} ]; then echo 1; else echo 0; 
fi)
+%define with_gpg_check %{has_gpg_keyring}
+
 %define with_introspection 0
 %define with_python 0
 %define with_vala 0
@@ -28,6 +35,10 @@ Group: Development/Libraries
 License: LGPLv2+
 URL: http://libvirt.org/
 Source0: ftp://libvirt.org/libvirt/glib/%{name}-%{version}.tar.gz
+%if %{with_gpg_check}
+Source1: ftp://libvirt.org/libvirt/glib/%{name}-%{version}.tar.gz.asc
+Source2: %{gpg_keyring}
+%endif
 BuildRoot: %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
 
 BuildRequires: glib2-devel >= @GLIB2_REQUIRED@
@@ -45,6 +56,9 @@ BuildRequires: libtool
 %if %{with_vala}
 BuildRequires: vala-tools
 %endif
+%if %{with_gpg_check}
+BuildRequires: gnupg2
+%endif
 
 %package devel
 Group: Development/Libraries
@@ -109,6 +123,9 @@ libvirt and the glib event loop
 %endif
 
 %prep
+%if %{with_gpg_check}
+gpgv2 --quiet --keyring %{SOURCE2} %{SOURCE1} %{SOURCE0}
+%endif
 %setup -q
 
 %build
-- 
2.5.5

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

[libvirt] [python PATCH] event: Add support VIR_DOMAIN_EVENT_ID_DEVICE_REMOVAL_FAILED

[Peter Krempa]

---
 examples/event-test.py |  4 +++
 libvirt-override-virConnect.py |  9 +++
 libvirt-override.c | 60 ++
 3 files changed, 73 insertions(+)

diff --git a/examples/event-test.py b/examples/event-test.py
index 5be4978..f96c917 100755
--- a/examples/event-test.py
+++ b/examples/event-test.py
@@ -535,6 +535,9 @@ def myDomainEventMigrationIteration(conn, dom, iteration, 
opaque):
 dom.name(), dom.ID(), iteration))
 def myDomainEventJobCompletedCallback(conn, dom, params, opaque):
 print("myDomainEventJobCompletedCallback: Domain %s(%s) %s" % (dom.name(), 
dom.ID(), params))
+def myDomainEventDeviceRemovalFailedCallback(conn, dom, dev, opaque):
+print("myDomainEventDeviceRemovalFailedCallback: Domain %s(%s) failed to 
remove device: %s" % (
+dom.name(), dom.ID(), dev))

 ##
 # Network events
@@ -649,6 +652,7 @@ def main():
 vc.domainEventRegisterAny(None, libvirt.VIR_DOMAIN_EVENT_ID_DEVICE_ADDED, 
myDomainEventDeviceAddedCallback, None)
 vc.domainEventRegisterAny(None, 
libvirt.VIR_DOMAIN_EVENT_ID_MIGRATION_ITERATION, 
myDomainEventMigrationIteration, None)
 vc.domainEventRegisterAny(None, libvirt.VIR_DOMAIN_EVENT_ID_JOB_COMPLETED, 
myDomainEventJobCompletedCallback, None)
+vc.domainEventRegisterAny(None, 
libvirt.VIR_DOMAIN_EVENT_ID_DEVICE_REMOVAL_FAILED, 
myDomainEventDeviceRemovalFailedCallback, None)

 vc.networkEventRegisterAny(None, libvirt.VIR_NETWORK_EVENT_ID_LIFECYCLE, 
myNetworkEventLifecycleCallback, None)

diff --git a/libvirt-override-virConnect.py b/libvirt-override-virConnect.py
index 396a6ed..0f76c59 100644
--- a/libvirt-override-virConnect.py
+++ b/libvirt-override-virConnect.py
@@ -234,6 +234,15 @@
 cb(self, virDomain(self, _obj=dom), params, opaque)
 return 0

+def _dispatchDomainEventDeviceRemovalFailedCallback(self, dom, devAlias, 
cbData):
+"""Dispatches event to python user domain device removal failed event 
callbacks
+"""
+cb = cbData["cb"]
+opaque = cbData["opaque"]
+
+cb(self, virDomain(self, _obj=dom), devAlias, opaque)
+return 0
+
 def domainEventDeregisterAny(self, callbackID):
 """Removes a Domain Event Callback. De-registering for a
domain callback will disable delivery of this event type """
diff --git a/libvirt-override.c b/libvirt-override.c
index ee355da..4640ed5 100644
--- a/libvirt-override.c
+++ b/libvirt-override.c
@@ -6894,6 +6894,61 @@ 
libvirt_virConnectDomainEventJobCompletedCallback(virConnectPtr conn ATTRIBUTE_U
 }
 #endif /* VIR_DOMAIN_EVENT_ID_JOB_COMPLETED */

+
+#ifdef VIR_DOMAIN_EVENT_ID_DEVICE_REMOVAL_FAILED
+static int
+libvirt_virConnectDomainEventDeviceRemovalFailedCallback(virConnectPtr conn 
ATTRIBUTE_UNUSED,
+ virDomainPtr dom,
+ const char *devAlias,
+ void *opaque)
+{
+PyObject *pyobj_cbData = (PyObject*)opaque;
+PyObject *pyobj_dom;
+PyObject *pyobj_ret = NULL;
+PyObject *pyobj_conn;
+PyObject *dictKey;
+int ret = -1;
+
+LIBVIRT_ENSURE_THREAD_STATE;
+
+if (!(dictKey = libvirt_constcharPtrWrap("conn")))
+goto cleanup;
+pyobj_conn = PyDict_GetItem(pyobj_cbData, dictKey);
+Py_DECREF(dictKey);
+
+/* Create a python instance of this virDomainPtr */
+virDomainRef(dom);
+if (!(pyobj_dom = libvirt_virDomainPtrWrap(dom))) {
+virDomainFree(dom);
+goto cleanup;
+}
+Py_INCREF(pyobj_cbData);
+
+/* Call the Callback Dispatcher */
+pyobj_ret = PyObject_CallMethod(pyobj_conn,
+
(char*)"_dispatchDomainEventDeviceRemovalFailedCallback",
+(char*)"OsO",
+pyobj_dom, devAlias, pyobj_cbData);
+
+Py_DECREF(pyobj_cbData);
+Py_DECREF(pyobj_dom);
+
+ cleanup:
+if (!pyobj_ret) {
+DEBUG("%s - ret:%p\n", __FUNCTION__, pyobj_ret);
+PyErr_Print();
+} else {
+Py_DECREF(pyobj_ret);
+ret = 0;
+}
+
+LIBVIRT_RELEASE_THREAD_STATE;
+return ret;
+
+}
+#endif /* VIR_DOMAIN_EVENT_ID_DEVICE_REMOVAL_FAILED */
+
+
 static PyObject *
 libvirt_virConnectDomainEventRegisterAny(PyObject *self ATTRIBUTE_UNUSED,
  PyObject *args)
@@ -7004,6 +7059,11 @@ libvirt_virConnectDomainEventRegisterAny(PyObject *self 
ATTRIBUTE_UNUSED,
 cb = 
VIR_DOMAIN_EVENT_CALLBACK(libvirt_virConnectDomainEventJobCompletedCallback);
 break;
 #endif /* VIR_DOMAIN_EVENT_ID_JOB_COMPLETED */
+#ifdef VIR_DOMAIN_EVENT_ID_DEVICE_REMOVAL_FAILED
+case VIR_DOMAIN_EVENT_ID_DEVICE_REMOVAL_FAILED:
+cb = 

Re: [libvirt] [PATCH] tests: Fix syntax in iSCSI auth/secret tests

[Ján Tomko]

On Sat, Apr 16, 2016 at 08:30:44AM -0400, John Ferlan wrote:
> While working on the tests for the secret initialization vector, I found
> that the existing iSCSI tests were lacking in how they defined the IQN.
> Many had IQN's of just 'iqn.1992-01.com.example' for one disk while using
> 'iqn.1992-01.com.example/1' for the second disk (same for hostdevs - guess
> how they were copied/generated).
> 
> Typically (and documented this way), IQN's would include be of the form
> 'iqn.1992-01.com.example:storage/1' indicating an IQN using "storage" for
> naming authority specific string and "/1" for the iSCSI LUN.
> 
> So modify the input XML's to use the more proper format - this of course
> has a ripple effect on the output XML and the args.
> 
> Also note that the "%3A" is generated by the virURIFormat/xmlSaveUri
> to represent the colon.
> 
> Signed-off-by: John Ferlan 
> ---
>  .../qemuargv2xml-disk-drive-network-iscsi-auth.args   | 6 +++---
>  .../qemuargv2xml-disk-drive-network-iscsi-auth.xml| 4 ++--
>  .../qemuxml2argv-disk-drive-network-iscsi-auth.args   | 8 
> +---
>  .../qemuxml2argv-disk-drive-network-iscsi-auth.xml| 7 +--
>  .../qemuxml2argv-hostdev-scsi-lsi-iscsi-auth.args | 4 ++--
>  .../qemuxml2argvdata/qemuxml2argv-hostdev-scsi-lsi-iscsi-auth.xml | 4 ++--
>  .../qemuxml2argv-hostdev-scsi-virtio-iscsi-auth.args  | 4 ++--
>  .../qemuxml2argv-hostdev-scsi-virtio-iscsi-auth.xml   | 4 ++--
>  .../qemuxml2xmlout-disk-drive-network-iscsi-auth.xml  | 7 +--
>  .../qemuxml2xmlout-hostdev-scsi-lsi-iscsi-auth.xml| 4 ++--
>  .../qemuxml2xmlout-hostdev-scsi-virtio-iscsi-auth.xml | 4 ++--
>  11 files changed, 32 insertions(+), 24 deletions(-)
> 

ACK

Jan

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

Re: [libvirt] [PATCH v4 2/2] qemu: migration: new migration param for persistent destination XML

[Dmitry Andreev]

On 15.04.2016 21:12, Jiri Denemark wrote:

On Thu, Mar 17, 2016 at 19:31:45 +0300, Dmitry Andreev wrote:

Migration API allows to specify a destination domain configuration.
Offline domain has only inactive XML and it is replaced by configuration
specified using VIR_MIGRATE_PARAM_DEST_XML param. In case of live
migration VIR_MIGRATE_PARAM_DEST_XML param is applied for active XML.

This commit introduces the new VIR_MIGRATE_PARAM_PERSIST_XML param
that can be used within live migration to replace persistent/inactive
configuration.

Required for: https://bugzilla.redhat.com/show_bug.cgi?id=835300
---
  include/libvirt/libvirt-domain.h | 15 +
  src/qemu/qemu_driver.c   | 12 ++
  src/qemu/qemu_migration.c| 47 ++--
  src/qemu/qemu_migration.h|  2 ++
  4 files changed, 56 insertions(+), 20 deletions(-)

diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
index 4ac29cd..f9dae22 100644
--- a/include/libvirt/libvirt-domain.h
+++ b/include/libvirt/libvirt-domain.h
@@ -723,6 +723,21 @@ typedef enum {
  # define VIR_MIGRATE_PARAM_DEST_XML  "destination_xml"

  /**
+ * VIR_MIGRATE_PARAM_PERSIST_XML:
+ *
+ * virDomainMigrate* params field: the new persistant configuration to be used


s/persistant/persistent/


+ * for the domain on the destination host as VIR_TYPED_PARAM_STRING.
+ * This field cannot be used to rename the domain during migration (use
+ * VIR_MIGRATE_PARAM_DEST_NAME field for that purpose). Domain name in the
+ * destination XML must match the original domain name.
+ *
+ * Omitting this parameter keeps the original domain persistent configuration.
+ * Using this field with hypervisors that do not support changing domain
+ * configuration during migration will result in a failure.
+ */
+# define VIR_MIGRATE_PARAM_PERSIST_XML  "persistent_xml"
+
+/**
   * VIR_MIGRATE_PARAM_BANDWIDTH:
   *
   * virDomainMigrate* params field: the maximum bandwidth (in MiB/s) that will

...

diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index f723a52..5624633 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c

...

@@ -4566,14 +4568,20 @@ qemuMigrationRun(virQEMUDriverPtr driver,
  cookieFlags |= QEMU_MIGRATION_COOKIE_NETWORK |
 QEMU_MIGRATION_COOKIE_STATS;

+if (flags & VIR_MIGRATE_PERSIST_DEST && persist_xml &&
+!(def = qemuMigrationPrepareDef(driver, persist_xml, NULL, NULL)))
+ret = -1;
+


I think this should be done before we start migration.


  if (ret == 0 &&
  (((flags & VIR_MIGRATE_PERSIST_DEST &&
-   qemuMigrationCookieAddPersistent(mig, vm->newDef) < 0)) ||
+   qemuMigrationCookieAddPersistent(mig,
+def ? def : vm->newDef) < 0)) ||


And we can use a single variable for both vm->newDef and def depending
on persist_xml.


qemuMigrationBakeCookie(mig, driver, vm, cookieout,
cookieoutlen, cookieFlags) < 0)) {
  VIR_WARN("Unable to encode migration cookie");
  }

+virDomainDefFree(def);
  qemuMigrationCookieFree(mig);

  if (events)

...

@@ -4683,6 +4692,7 @@ static int doNativeMigrate(virQEMUDriverPtr driver,
  static int doTunnelMigrate(virQEMUDriverPtr driver,
 virDomainObjPtr vm,
 virStreamPtr st,
+   const char *xml_persist,


s/xml_persist/persist_xml/ to keep the name consistent accros the file.


 const char *cookiein,
 int cookieinlen,
 char **cookieout,

...

To avoid sending another version of this patch for review, which already
took too long (this is my fault, sorry for that), I suggest squashing
the following patch in and pushing the result.


I'm OK with the following patch. Thanks!



Jirka

diff --git i/include/libvirt/libvirt-domain.h w/include/libvirt/libvirt-domain.h
index 697670f..9936cb2 100644
--- i/include/libvirt/libvirt-domain.h
+++ w/include/libvirt/libvirt-domain.h
@@ -729,7 +729,7 @@ typedef enum {
  /**
   * VIR_MIGRATE_PARAM_PERSIST_XML:
   *
- * virDomainMigrate* params field: the new persistant configuration to be used
+ * virDomainMigrate* params field: the new persistent configuration to be used
   * for the domain on the destination host as VIR_TYPED_PARAM_STRING.
   * This field cannot be used to rename the domain during migration (use
   * VIR_MIGRATE_PARAM_DEST_NAME field for that purpose). Domain name in the
diff --git i/src/qemu/qemu_migration.c w/src/qemu/qemu_migration.c
index eee8ec2..680c9ba 100644
--- i/src/qemu/qemu_migration.c
+++ w/src/qemu/qemu_migration.c
@@ -4504,7 +4504,6 @@ qemuMigrationRun(virQEMUDriverPtr driver,
  {
  int ret = -1;
  unsigned int migrate_flags = QEMU_MONITOR_MIGRATE_BACKGROUND;
-virDomainDefPtr def = NULL;
  

Re: [libvirt] RFC: virtio-rng and /dev/urandom

[H. Peter Anvin]

On April 18, 2016 2:28:42 AM PDT, "Daniel P. Berrange"  
wrote:
>On Fri, Apr 15, 2016 at 08:56:59AM -0700, H. Peter Anvin wrote:
>> On April 15, 2016 3:41:34 AM PDT, Cole Robinson 
>wrote:
>> >Libvirt currently rejects using host /dev/urandom as an input source
>> >for a
>> >virtio-rng device. The only accepted sources are /dev/random and
>> >/dev/hwrng.
>> >This is the result of discussions on qemu-devel around when the
>feature
>> >was
>> >first added (2013). Examples:
>> >
>> >http://lists.gnu.org/archive/html/qemu-devel/2012-09/msg02387.html
>>
>>https://lists.gnu.org/archive/html/qemu-devel/2013-03/threads.html#00023
>> >
>> >libvirt's rejection of /dev/urandom has generated some complaints
>from
>> >users:
>> >
>> >https://bugzilla.redhat.com/show_bug.cgi?id=1074464
>> >* cited: http://www.2uo.de/myths-about-urandom/
>> >http://www.redhat.com/archives/libvir-list/2016-March/msg01062.html
>> >http://www.redhat.com/archives/libvir-list/2016-April/msg00186.html
>> >
>> >I think it's worth having another discussion about this, at least
>with
>> >a
>> >recent argument in one place so we can put it to bed. I'm CCing a
>bunch
>> >of
>> >people. I think the questions are:
>> >
>> >1) is the original recommendation to never use
>virtio-rng+/dev/urandom
>> >correct?
>> >
>> >2) regardless of #1, should we continue to reject that config in
>> >libvirt?
>> >
>> >Thanks,
>> >Cole
>> 
>> Using /dev/urandom for virtio-rng, *except* perhaps for a small seed,
>> it a complete waste of cycles.  There is absolutely no reason to have
>> one prng feed another.
>
>Regardless of the performance aspect, the key question we need the
>answer to is whether it *cryptographically safe* to use /dev/urandom
>on the host to feed virtio-rng. The original discussion said it was
>/unsafe/ to use /dev/urandom, hence why we do not allow it.  If the
>only downside is wasted performance, then it is reasonable to allow
>the user to use /dev/urandom if they so wish.
>
>Regards,
>Daniel

Perhaps.  What we do know is that it at least used to be a fairly common 
misconfiguration; up there with people who would feed /dev/urandom to rngd!

Probably there ought to be a backend which knows to use urandom for a seed and 
then fall back to random, rather than simply relying on a file name.

Another thing that really needs to be addressed, but is a separate issue: 
invalidating and reseeding the entropy pool after a snapshot event.

-- 
Sent from my Android device with K-9 Mail. Please excuse brevity and formatting.

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

Re: [libvirt] RFC: virtio-rng and /dev/urandom

[Daniel P. Berrange]

On Fri, Apr 15, 2016 at 08:56:59AM -0700, H. Peter Anvin wrote:
> On April 15, 2016 3:41:34 AM PDT, Cole Robinson  wrote:
> >Libvirt currently rejects using host /dev/urandom as an input source
> >for a
> >virtio-rng device. The only accepted sources are /dev/random and
> >/dev/hwrng.
> >This is the result of discussions on qemu-devel around when the feature
> >was
> >first added (2013). Examples:
> >
> >http://lists.gnu.org/archive/html/qemu-devel/2012-09/msg02387.html
> >https://lists.gnu.org/archive/html/qemu-devel/2013-03/threads.html#00023
> >
> >libvirt's rejection of /dev/urandom has generated some complaints from
> >users:
> >
> >https://bugzilla.redhat.com/show_bug.cgi?id=1074464
> >* cited: http://www.2uo.de/myths-about-urandom/
> >http://www.redhat.com/archives/libvir-list/2016-March/msg01062.html
> >http://www.redhat.com/archives/libvir-list/2016-April/msg00186.html
> >
> >I think it's worth having another discussion about this, at least with
> >a
> >recent argument in one place so we can put it to bed. I'm CCing a bunch
> >of
> >people. I think the questions are:
> >
> >1) is the original recommendation to never use virtio-rng+/dev/urandom
> >correct?
> >
> >2) regardless of #1, should we continue to reject that config in
> >libvirt?
> >
> >Thanks,
> >Cole
> 
> Using /dev/urandom for virtio-rng, *except* perhaps for a small seed,
> it a complete waste of cycles.  There is absolutely no reason to have
> one prng feed another.

Regardless of the performance aspect, the key question we need the
answer to is whether it *cryptographically safe* to use /dev/urandom
on the host to feed virtio-rng. The original discussion said it was
/unsafe/ to use /dev/urandom, hence why we do not allow it.  If the
only downside is wasted performance, then it is reasonable to allow
the user to use /dev/urandom if they so wish.

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: Drop unmaintained hv drivers? (phyp, xenapi, hyperv)

[Daniel P. Berrange]

On Fri, Apr 15, 2016 at 03:54:00PM -0400, Cole Robinson wrote:
> Hi all,
> 
> There's a few old hypervisor drivers in the tree that haven't been actively
> maintained for a long time. I'm curious if anyone knows of these drivers being
> actively used. If not I think we should consider dropping them
> 
> 
> src/phyp/ : for power VM hypervisor. Added in July 2009. The last commit that
> looks like it wasn't either internal API conversion, or caught by code
> analysis, is:
> 
> commit 41461ff7f7d6ee6679aae2a8004e305c5830c9e8
> Author: Eduardo Otubo 
> Date:   Tue Apr 19 12:34:08 2011 -0300
> 
> PHYP: Adding reboot domain function
> 
> Adding reboot  function for pHyp driver.
> 
> Nearly 5 years ago. Eduardo is the primary driver author too (CCd at his email
> from github).
> 
> Searching the upstream bug tracker for all bugs with 'phyp', the only one
> that's actually about the phyp driver is a report from 2 years ago that it
> crashes trying to open a connection:
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1093094

The phyp driver would have a fairly niche end user audience, so it would
not be entirely surprising to not hear much about it.


> src/xenapi/: Connecting to a xen api server. Added in March 2010. Largely
> appears to be a code drop, the original author/committer has never had another
> commit. The last xenapi specific commit seems to be:
> 
> commit 484460ec4678a264c5e7355495c2f0da72cb42bd
> Author: Matthias Bolte 
> Date:   Thu Jul 21 15:16:11 2011 +0200
> 
> xenapi: Improve error reporting in xenapiOpen once again
> 
> Nearly 5 years ago. The only upstream bug that was filed about xenapi is:
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=711372
> 
> Which was about a connection failure that was eventually fixed upstream, and
> dovetailed into the above referenced commit. Current xen guys, you know of
> anyone using this?

The XenAPI driver was never "finished" as IIRC the person who started work
on it, left XenSource before he was able to make it functional enough to
be useful. The XenAPI was targetting the closed source Xen products, as
opposed to the open source platform, which is now libxl based. I did vaguely
remember talk of the Xen products eventualy moving over to be libxl based
instead of XenAPi, but I don't know if that's actually accurate or not.

> src/hyperv/: Added in July 2011. This was largely a code drop as well;
> committed and patched a few times by Matthias but it was a university project
> by someone else. Last hyperv targeted patch was:
> 
> commit 9e9ea3ead9825bd1dc2c17cea4abc8c4165591d0
> Author: Matthias Bolte 
> Date:   Sun Sep 9 17:39:40 2012 +0200
> 
> hyperv: Fix and improve hypervListAllDomains
> 
> The driver is fairly minimal as well: it can only list existing VMs and
> perform lifecycle operations. It can't create new VMs, and doesn't list VM
> device config AFAICT.

This is a fairly similar level of functionality to the XenAPI driver, but
I'm pretty loathe for us to loose even the limited hyperv support, as it
is very  much an active product that's not going away.


In general I'm not really in favour of dropping virt drivers for hypervisor
platforms that still exist and have potential users, even if we don't hear
much about them. I don't think any of these drivers is really giving us a
significant maintenance headache to justify deletion.

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 v3] security: Rename DomainSetDirLabel to DomainSetPathLabel

[Martin Kletzander]

It already labels abritrary paths, so it's just the naming that was
wrong.

Signed-off-by: Martin Kletzander 
---
v3: The other patch from the series was pushed, so this is the only
one missing right now, so it needed rebasing.

 src/libvirt_private.syms|  2 +-
 src/qemu/qemu_domain.c  |  4 ++--
 src/qemu/qemu_process.c |  4 ++--
 src/security/security_dac.c |  8 
 src/security/security_driver.h  |  8 
 src/security/security_manager.c | 10 +-
 src/security/security_manager.h |  6 +++---
 src/security/security_selinux.c |  8 
 src/security/security_stack.c   | 12 ++--
 9 files changed, 31 insertions(+), 31 deletions(-)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 61fc5004bbb7..1118fdcfa3bc 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1055,7 +1055,7 @@ virSecurityDriverLookup;
 # security/security_manager.h
 virSecurityManagerCheckAllLabel;
 virSecurityManagerClearSocketLabel;
-virSecurityManagerDomainSetDirLabel;
+virSecurityManagerDomainSetPathLabel;
 virSecurityManagerGenLabel;
 virSecurityManagerGetBaseLabel;
 virSecurityManagerGetDOI;
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index ed1e0e502925..e031e0fc42c8 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -527,8 +527,8 @@ qemuDomainWriteMasterKeyFile(virQEMUDriverPtr driver,
 goto cleanup;
 }

-if (virSecurityManagerDomainSetDirLabel(driver->securityManager,
-vm->def, path) < 0)
+if (virSecurityManagerDomainSetPathLabel(driver->securityManager,
+ vm->def, path) < 0)
 goto cleanup;

 ret = 0;
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 81d86c2d1aa2..c0873007d397 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -4486,8 +4486,8 @@ qemuProcessMakeDir(virQEMUDriverPtr driver,
 goto cleanup;
 }

-if (virSecurityManagerDomainSetDirLabel(driver->securityManager,
-vm->def, path) < 0)
+if (virSecurityManagerDomainSetPathLabel(driver->securityManager,
+ vm->def, path) < 0)
 goto cleanup;

 ret = 0;
diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index a09aba5f62c6..df3ed4793be8 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -1542,9 +1542,9 @@ virSecurityDACGetBaseLabel(virSecurityManagerPtr mgr,
 }

 static int
-virSecurityDACDomainSetDirLabel(virSecurityManagerPtr mgr,
-virDomainDefPtr def,
-const char *path)
+virSecurityDACDomainSetPathLabel(virSecurityManagerPtr mgr,
+ virDomainDefPtr def,
+ const char *path)
 {
 virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr);
 virSecurityLabelDefPtr seclabel;
@@ -1607,5 +1607,5 @@ virSecurityDriver virSecurityDriverDAC = {

 .getBaseLabel   = virSecurityDACGetBaseLabel,

-.domainSetDirLabel  = virSecurityDACDomainSetDirLabel,
+.domainSetPathLabel = virSecurityDACDomainSetPathLabel,
 };
diff --git a/src/security/security_driver.h b/src/security/security_driver.h
index 784b0dee65ea..7cb62f029343 100644
--- a/src/security/security_driver.h
+++ b/src/security/security_driver.h
@@ -118,9 +118,9 @@ typedef int (*virSecurityDomainSetImageLabel) 
(virSecurityManagerPtr mgr,
 typedef int (*virSecurityDomainRestoreImageLabel) (virSecurityManagerPtr mgr,
virDomainDefPtr def,
virStorageSourcePtr src);
-typedef int (*virSecurityDomainSetDirLabel) (virSecurityManagerPtr mgr,
- virDomainDefPtr def,
- const char *path);
+typedef int (*virSecurityDomainSetPathLabel) (virSecurityManagerPtr mgr,
+  virDomainDefPtr def,
+  const char *path);


 struct _virSecurityDriver {
@@ -172,7 +172,7 @@ struct _virSecurityDriver {

 virSecurityDriverGetBaseLabel getBaseLabel;

-virSecurityDomainSetDirLabel domainSetDirLabel;
+virSecurityDomainSetPathLabel domainSetPathLabel;
 };

 virSecurityDriverPtr virSecurityDriverLookup(const char *name,
diff --git a/src/security/security_manager.c b/src/security/security_manager.c
index 07a05224e0be..ecb4a40f05c8 100644
--- a/src/security/security_manager.c
+++ b/src/security/security_manager.c
@@ -987,14 +987,14 @@ virSecurityManagerSetHugepages(virSecurityManagerPtr mgr,


 int
-virSecurityManagerDomainSetDirLabel(virSecurityManagerPtr mgr,
-virDomainDefPtr vm,
-

Re: [libvirt] [PATCH] qemu: Label master key file

[Martin Kletzander]

On Fri, Apr 15, 2016 at 12:16:18PM -0400, Cole Robinson wrote:

On 04/14/2016 03:04 AM, Martin Kletzander wrote:

On Wed, Apr 13, 2016 at 07:58:06PM -0400, Cole Robinson wrote:

On 04/13/2016 11:56 AM, Cole Robinson wrote:

On 04/13/2016 11:17 AM, Martin Kletzander wrote:

When creating the master key, we used mode 0600 (which we should) but
because we were creating it as root, the file is not readable by any
qemu running as non-root.  Fortunately, it's just a matter of labelling
the file.  We are generating the file path few times already, so let's
label it in the same function that has access to the path already.

Signed-off-by: Martin Kletzander 
---
 src/qemu/qemu_domain.c  | 15 ---
 src/qemu/qemu_domain.h  |  3 ++-
 src/qemu/qemu_process.c |  2 +-
 3 files changed, 15 insertions(+), 5 deletions(-)



ACK, makes sense and fixes things for me. One comment below


diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 5d54fffcfb98..83e765ef6868 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -504,11 +504,13 @@ qemuDomainGetMasterKeyFilePath(const char *libDir)
  * Returns 0 on success, -1 on failure with error message indicating failure
  */
 static int
-qemuDomainWriteMasterKeyFile(qemuDomainObjPrivatePtr priv)
+qemuDomainWriteMasterKeyFile(virQEMUDriverPtr driver,
+ virDomainObjPtr vm)
 {
 char *path;
 int fd = -1;
 int ret = -1;
+qemuDomainObjPrivatePtr priv = vm->privateData;

 if (!(path = qemuDomainGetMasterKeyFilePath(priv->libDir)))
 return -1;
@@ -525,6 +527,10 @@ qemuDomainWriteMasterKeyFile(qemuDomainObjPrivatePtr
priv)
 goto cleanup;
 }

+if (virSecurityManagerDomainSetDirLabel(driver->securityManager,
+vm->def, path) < 0)
+goto cleanup;
+
 ret = 0;



I looked briefly at fixing this but know if there was a function to ask the
security driver 'just set a on this arbitrary path'. I saw DirLabel but was
thrown off by the 'Dir' name. Maybe change it to something more generic?



Yes, it's just a naming; I had to add it for similar purpose when
labelling directories, but it "Just Works" for arbitrary path.  I'll
rename that.



Also adding some CC, I'm guessing virt-aa-helper.c needs to be extended to to
allow access to $libDir/master-key.aes



Actually, it shouldn't.  It's in the per-domain directory and
everything under that should be available.

Anyway, it's a pity that we're not very likely to have a test case for
that.  But couldn't the paths in virt-aa-helper be created from a
security driver?  It knows all the paths, doesn't it?

I'll send a v2 with the rename.


Laine was hitting issues with this too, so I pushed this patch. The API rename
isn't blocking anyone



v2 was alrady on the list, but it can be done the other way around.
I'll send the rename rebased now so it's easier to review.


Thanks,
Cole


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