Re: [libvirt] [PATCH 2/4] conf: introduce virDomainDefCheckBootOrder

[Peter Krempa]

On Mon, May 28, 2018 at 15:54:03 +0200, Ján Tomko wrote:
> Move the check for boot elements into a separate function
> and remove its dependency on the parser-supplied bootHash table.
> 
> Reconstructing the hash table from the domain definition
> effectively duplicates the check for duplicate boot order
> values, also present in virDomainDeviceBootParseXML.

So the semantical difference is that places that call into the
post-parse infrastructure which construct the domain definition object
by other means that parsing XML will get the same treatment as when XML
is parsed.

> 
> Signed-off-by: Ján Tomko 
> ---
>  src/conf/domain_conf.c   | 89 
> +++-
>  tests/qemuargv2xmldata/nomachine-aarch64.xml |  1 +
>  tests/qemuargv2xmldata/nomachine-ppc64.xml   |  1 +
>  tests/qemuargv2xmldata/nomachine-x86_64.xml  |  1 +
>  tests/sexpr2xmldata/sexpr2xml-fv-kernel.xml  |  1 +
>  5 files changed, 78 insertions(+), 15 deletions(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index d6ac47c629..f087a3680f 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -4939,10 +4939,79 @@ virDomainDefPostParseCPU(virDomainDefPtr def)

[...]

> +
> +
> +static int
> +virDomainDefCheckBootOrder(virDomainDefPtr def)

[1]

> +{
> +virHashTablePtr bootHash = NULL;
> +int ret = -1;
> +
> +if (def->os.type != VIR_DOMAIN_OSTYPE_HVM)
> +return 0;

Please do this check outside of this function. If this function is
invoked it should o it's job, especially since you also have other
conditions when to invoke it outside.

> +
> +if (!(bootHash = virHashCreate(5, NULL)))
> +goto cleanup;
> +
> +if (virDomainDeviceInfoIterate(def, virDomainDefCollectBootOrder, 
> bootHash) < 0)
> +goto cleanup;
> +
> +if (def->os.nBootDevs > 0 && virHashSize(bootHash) > 0) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +   _("per-device boot elements cannot be used"
> + " together with os/boot elements"));
> +goto cleanup;
> +}
> +
> +if (def->os.nBootDevs == 0 && virHashSize(bootHash) == 0) {
> +def->os.nBootDevs = 1;
> +def->os.bootDevs[0] = VIR_DOMAIN_BOOT_DISK;

[1] this in contrast to the name of the function modifies stuff ...

> +}
> +
> +ret = 0;
> +
> + cleanup:
> +virHashFree(bootHash);
> +return ret;
> +}
> +
> +

[...]

> @@ -4979,6 +5034,10 @@ virDomainDefPostParseCommon(virDomainDefPtr def,
>  if (virDomainDefRejectDuplicatePanics(def) < 0)
>  return -1;
>  
> +if (!(data->xmlopt->config.features & 
> VIR_DOMAIN_DEF_FEATURE_NO_BOOT_ORDER) &&
> +virDomainDefCheckBootOrder(def) < 0)

Add the condition for checking HVM here.

> +return -1;
> +
>  if (virDomainDefPostParseTimer(def) < 0)
>  return -1;
>  

[...]

> diff --git a/tests/sexpr2xmldata/sexpr2xml-fv-kernel.xml 
> b/tests/sexpr2xmldata/sexpr2xml-fv-kernel.xml
> index afb9030681..a3d54ae3c1 100644
> --- a/tests/sexpr2xmldata/sexpr2xml-fv-kernel.xml
> +++ b/tests/sexpr2xmldata/sexpr2xml-fv-kernel.xml
> @@ -10,6 +10,7 @@
>  /var/lib/xen/vmlinuz.2Dn2YT
>  /var/lib/xen/initrd.img.0u-Vhq
>   
> method=http://download.fedora.devel.redhat.com/pub/fedora/linux/core/test/5.91/x86_64/os
>   
> +

This looks wrong here since we do have the 'kernel' and 'initrd'
elements. Given that it's caused by the semantic difference I think it's
okay.

>
>
>destroy

ACK if you move out the condition and note the semantic impact in the
commit message.


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

Re: [libvirt] [PATCH 3/4] conf: remove 'bootHash' from the post-parse infrastructure

[Peter Krempa]

On Mon, May 28, 2018 at 15:54:04 +0200, Ján Tomko wrote:
> From: Peter Krempa 
> 
> As the function signature of virDomainDefPostParseInternal does not
> differ from virDomainDefPostParse now, the wrapper can be dropped.
> 
> Signed-off-by: Peter Krempa 
> Signed-off-by: Ján Tomko 
> ---
>  src/conf/domain_conf.c | 33 +
>  1 file changed, 9 insertions(+), 24 deletions(-)

Implicit ACK. Also please push the rest of the patches from my series
which depend on this when you'll be pushing this.


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

Re: [libvirt] [PATCH 4/4] conf: remove 'bootHash' completely

[Peter Krempa]

On Mon, May 28, 2018 at 15:54:05 +0200, Ján Tomko wrote:
> Its only use is now to check for duplicate boot order values,
> which is now also done in virDomainDefPostParseCommon.
> 
> Remove it completely.
> 
> Signed-off-by: Ján Tomko 
> ---
>  src/conf/domain_conf.c | 89 
> ++
>  1 file changed, 31 insertions(+), 58 deletions(-)

ACK


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

Re: [libvirt] [PATCH 1/4] vmx: add VIR_DOMAIN_DEF_FEATURE_NO_BOOT_ORDER

[Peter Krempa]

On Mon, May 28, 2018 at 15:54:02 +0200, Ján Tomko wrote:
> Further patches will introduce validation and a default setting
> of def->os.bootDevs in postParse.
> 
> Introduce a feature flag to opt out of this and set it in the vmx
> driver.

This does not clarify in any way why it is required.

> 
> Signed-off-by: Ján Tomko 
> ---
>  src/conf/domain_conf.h | 1 +
>  src/vmx/vmx.c  | 3 ++-
>  2 files changed, 3 insertions(+), 1 deletion(-)


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

Re: [libvirt] [PATCHv2 1/7] Introduce virDomainVsockDef

[Peter Krempa]

On Thu, May 24, 2018 at 12:39:09 +0200, Ján Tomko wrote:
> A type to represent the new vsock device.
> Also implement an allocation function to allow future addition
> of private data.
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1291851
> 
> Signed-off-by: Ján Tomko 
> ---
>  src/conf/domain_conf.c   | 21 +
>  src/conf/domain_conf.h   |  8 
>  src/libvirt_private.syms |  1 +
>  3 files changed, 30 insertions(+)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 3689ac0a82..5b4b182fd2 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -2027,6 +2027,27 @@ virDomainActualNetDefFree(virDomainActualNetDefPtr def)
>  VIR_FREE(def);
>  }
>  
> +
> +virDomainVsockDefPtr
> +virDomainVsockDefNew(virDomainXMLOptionPtr xmlopt ATTRIBUTE_UNUSED)
> +{
> +virDomainVsockDefPtr ret = NULL;
> +virDomainVsockDefPtr vsock;
> +
> +if (VIR_ALLOC(vsock) < 0)
> +return NULL;
> +
> +VIR_STEAL_PTR(ret, vsock);
> +return ret;
> +}
> +
> +
> +void
> +virDomainVsockDefFree(virDomainVsockDefPtr vsock ATTRIBUTE_UNUSED)
> +{

You should 'VIR_FREE(vsock)' here, as otherwise this does not do what
the name suggests ...

> +}
> +
> +
>  void
>  virDomainNetDefClear(virDomainNetDefPtr def)

ACK with the modification above


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

Re: [libvirt] [PATCHv2 2/7] Add privateData to virDomainVsockDef

[Peter Krempa]

Add 'conf:' prefix.

On Thu, May 24, 2018 at 12:39:10 +0200, Ján Tomko wrote:
> An object for storing driver-specific data in the vsock definition.
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1291851
> Signed-off-by: Ján Tomko 
> ---
>  src/conf/domain_conf.c | 16 ++--
>  src/conf/domain_conf.h |  2 ++
>  2 files changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 5b4b182fd2..b2982fc3d4 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -2029,7 +2029,7 @@ virDomainActualNetDefFree(virDomainActualNetDefPtr def)
>  
>  
>  virDomainVsockDefPtr
> -virDomainVsockDefNew(virDomainXMLOptionPtr xmlopt ATTRIBUTE_UNUSED)
> +virDomainVsockDefNew(virDomainXMLOptionPtr xmlopt)
>  {
>  virDomainVsockDefPtr ret = NULL;
>  virDomainVsockDefPtr vsock;
> @@ -2037,14 +2037,26 @@ virDomainVsockDefNew(virDomainXMLOptionPtr xmlopt 
> ATTRIBUTE_UNUSED)
>  if (VIR_ALLOC(vsock) < 0)
>  return NULL;
>  
> +if (xmlopt &&
> +xmlopt->privateData.vsockNew &&
> +!(vsock->privateData = xmlopt->privateData.vsockNew()))
> +goto cleanup;
> +
>  VIR_STEAL_PTR(ret, vsock);
> + cleanup:
> +virDomainVsockDefFree(vsock);

This will leak the struct if you don't do what I've suggested before.

ACK



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

Re: [libvirt] [PATCH 1/4] vmx: add VIR_DOMAIN_DEF_FEATURE_NO_BOOT_ORDER

[Ján Tomko]

On Tue, May 29, 2018 at 09:30:05AM +0200, Peter Krempa wrote:

On Mon, May 28, 2018 at 15:54:02 +0200, Ján Tomko wrote:

Further patches will introduce validation and a default setting
of def->os.bootDevs in postParse.

Introduce a feature flag to opt out of this and set it in the vmx
driver.


This does not clarify in any way why it is required.




Introduce a feature flag to opt out of this and set it in the vmx
driver, otherwise we would be adding it  into every
vmx config despite having no way to change it.

(Alternatively, if booting from hard-drive is the default, we can just
leave it in because none of the vmx code even touches bootDevs, so
it will be safely ignored)

Jano



Signed-off-by: Ján Tomko 
---
 src/conf/domain_conf.h | 1 +
 src/vmx/vmx.c  | 3 ++-
 2 files changed, 3 insertions(+), 1 deletion(-)





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

Re: [libvirt] [PATCH 1/2] bhyve: add CPU topology support

[Peter Krempa]

On Mon, May 28, 2018 at 20:27:50 +0400, Roman Bogorodskiy wrote:
> Recently, bhyve started supporting specifying guest CPU topology.
> It looks this way:
> 
>   bhyve -c cpus=C,sockets=S,cores=C,threads=T ...
> 
> The old behaviour with bhyve -c C, where C is a number of vCPUs, is
> still supported.
> 
> So if we have CPU topology in the domain XML, use the new syntax,
> otherwise keeps the old behaviour.
> 
> Signed-off-by: Roman Bogorodskiy 
> ---
>  src/bhyve/bhyve_capabilities.c|  7 +++--
>  src/bhyve/bhyve_capabilities.h|  1 +
>  src/bhyve/bhyve_command.c | 17 +++-
>  .../bhyvexml2argv-cputopology.args|  9 +++
>  .../bhyvexml2argv-cputopology.ldargs  |  3 +++
>  .../bhyvexml2argv-cputopology.xml | 26 +++
>  tests/bhyvexml2argvtest.c |  7 -
>  7 files changed, 66 insertions(+), 4 deletions(-)
>  create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-cputopology.args
>  create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-cputopology.ldargs
>  create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-cputopology.xml

[...]

> diff --git a/src/bhyve/bhyve_command.c b/src/bhyve/bhyve_command.c
> index e3f7ded7db..b319518520 100644
> --- a/src/bhyve/bhyve_command.c
> +++ b/src/bhyve/bhyve_command.c
> @@ -467,7 +467,22 @@ virBhyveProcessBuildBhyveCmd(virConnectPtr conn,
>  
>  /* CPUs */
>  virCommandAddArg(cmd, "-c");
> -virCommandAddArgFormat(cmd, "%d", virDomainDefGetVcpus(def));
> +if (def->cpu && def->cpu->sockets) {
> +if ((bhyveDriverGetCaps(conn) & BHYVE_CAP_CPUTOPOLOGY) != 0) {
> +virCommandAddArgFormat(cmd, 
> "cpus=%d,sockets=%d,cores=%d,threads=%d",
> +   virDomainDefGetVcpus(def),
> +   def->cpu->sockets,
> +   def->cpu->cores,
> +   def->cpu->threads);

Note that libvirt XML->def conversion does not validate that def->nvcpus
equals to def->cpu->sockets * def->cpu->cores * def->cpu->threads.

This is a historic artefact since qemu did not do it either. They
started doing it just recently. It might be worth adding that check to
be sure in the future.

> +} else {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +   _("Installed bhyve binary does not support "
> + "defining CPU topology"));
> +goto error;
> +}

The rest looks good to me, so ACK if you don't think the check for the
topology<->vcpu count is important enough.


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

Re: [libvirt] [PATCH 1/4] vmx: add VIR_DOMAIN_DEF_FEATURE_NO_BOOT_ORDER

[Peter Krempa]

On Tue, May 29, 2018 at 09:48:51 +0200, Ján Tomko wrote:
> On Tue, May 29, 2018 at 09:30:05AM +0200, Peter Krempa wrote:
> > On Mon, May 28, 2018 at 15:54:02 +0200, Ján Tomko wrote:
> > > Further patches will introduce validation and a default setting
> > > of def->os.bootDevs in postParse.
> > > 
> > > Introduce a feature flag to opt out of this and set it in the vmx
> > > driver.
> > 
> > This does not clarify in any way why it is required.
> > 
> 
> 
> Introduce a feature flag to opt out of this and set it in the vmx
> driver, otherwise we would be adding it  into every
> vmx config despite having no way to change it.

ACK to the patch if you add this wording.

> 
> (Alternatively, if booting from hard-drive is the default, we can just
> leave it in because none of the vmx code even touches bootDevs, so
> it will be safely ignored)

It very well might be the default. The capability can easily be deleted
later.

> 
> Jano
> 
> > > 
> > > Signed-off-by: Ján Tomko 
> > > ---
> > >  src/conf/domain_conf.h | 1 +
> > >  src/vmx/vmx.c  | 3 ++-
> > >  2 files changed, 3 insertions(+), 1 deletion(-)
> 
> 



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



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

Re: [libvirt] [PATCH 2/4] conf: introduce virDomainDefCheckBootOrder

[Ján Tomko]

On Tue, May 29, 2018 at 09:26:51AM +0200, Peter Krempa wrote:

On Mon, May 28, 2018 at 15:54:03 +0200, Ján Tomko wrote:

Move the check for boot elements into a separate function
and remove its dependency on the parser-supplied bootHash table.

Reconstructing the hash table from the domain definition
effectively duplicates the check for duplicate boot order
values, also present in virDomainDeviceBootParseXML.




How about:
Now it will also be run on domains created by other means than XML
parsing, since it will be run even for code paths that did not supply
the bootHash table before.


So the semantical difference is that places that call into the
post-parse infrastructure which construct the domain definition object
by other means that parsing XML will get the same treatment as when XML
is parsed.



Signed-off-by: Ján Tomko 
---
 src/conf/domain_conf.c   | 89 +++-
 tests/qemuargv2xmldata/nomachine-aarch64.xml |  1 +
 tests/qemuargv2xmldata/nomachine-ppc64.xml   |  1 +
 tests/qemuargv2xmldata/nomachine-x86_64.xml  |  1 +
 tests/sexpr2xmldata/sexpr2xml-fv-kernel.xml  |  1 +
 5 files changed, 78 insertions(+), 15 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index d6ac47c629..f087a3680f 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -4939,10 +4939,79 @@ virDomainDefPostParseCPU(virDomainDefPtr def)


[...]


+
+
+static int
+virDomainDefCheckBootOrder(virDomainDefPtr def)


[1]


+{
+virHashTablePtr bootHash = NULL;
+int ret = -1;
+
+if (def->os.type != VIR_DOMAIN_OSTYPE_HVM)
+return 0;


Please do this check outside of this function. If this function is
invoked it should o it's job, especially since you also have other
conditions when to invoke it outside.


+
+if (!(bootHash = virHashCreate(5, NULL)))
+goto cleanup;
+
+if (virDomainDeviceInfoIterate(def, virDomainDefCollectBootOrder, bootHash) 
< 0)
+goto cleanup;
+
+if (def->os.nBootDevs > 0 && virHashSize(bootHash) > 0) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+   _("per-device boot elements cannot be used"
+ " together with os/boot elements"));
+goto cleanup;
+}
+
+if (def->os.nBootDevs == 0 && virHashSize(bootHash) == 0) {
+def->os.nBootDevs = 1;
+def->os.bootDevs[0] = VIR_DOMAIN_BOOT_DISK;


[1] this in contrast to the name of the function modifies stuff ...


I can rename it to virDomainDefBootOrderPostParse

Jano


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

[libvirt] [PATCH 00/10] Use better PRNG

[Michal Privoznik]

This is inspired by bug reported here [1]. Even though Eric suggested
calling this Linux syscall when building without gnutls [2] I've decided
to not implement it. Firstly, we build with gnuls everywhere (even
Windows), secondly I see no appealing reason to special case Linux -
/dev/urandom is good for both Linux and FreeBSD.

Once these are merged I'm probably going to send patch set that makes
gnutls mandatory. I'm tired of all those WITH_GNUTLS if-defs (esp. in
function arguments). But that is orthogonal to what I'm solving here.

Also, I'm not quite sure this is a release material, so I'm fine with
merging this after the release.

1: https://www.redhat.com/archives/libvirt-users/2018-May/msg00097.html
2: https://www.redhat.com/archives/libvirt-users/2018-May/msg00100.html

Michal Privoznik (10):
  virRandomBytes: Fix return value
  virCryptoGenerateRandom: rename ret
  virCryptoGenerateRandom: Explain gnults error
  virCryptoGenerateRandom: Don't allocate return buffer
  virRandomBytes: Prefer saferead over plain read
  virRandomBytes: Report error
  virRandomBytes: Use gnutls_rnd whenever possible
  virrandom: Make virRandomBits better
  virUUIDGenerate don't fall back to virRandomBits
  vircrypto: Drop virCryptoGenerateRandom

 src/libvirt_private.syms |   1 -
 src/qemu/qemu_domain.c   |  13 --
 src/util/vircrypto.c |  41 ---
 src/util/vircrypto.h |   2 -
 src/util/virrandom.c | 103 ---
 src/util/viruuid.c   |  25 ++--
 tests/qemuxml2argvmock.c |  13 --
 tests/vircryptotest.c|   4 +-
 8 files changed, 48 insertions(+), 154 deletions(-)

-- 
2.16.1

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

[libvirt] [PATCH 02/10] virCryptoGenerateRandom: rename ret

[Michal Privoznik]

This function allocates a buffer, fills it in with random bytes
and then returns it. However, the buffer is held in @buf
variable, therefore having @ret variable which does not hold
return value of the function is misleading.

Signed-off-by: Michal Privoznik 
---
 src/util/vircrypto.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/src/util/vircrypto.c b/src/util/vircrypto.c
index 4079013d6d..930fa3b215 100644
--- a/src/util/vircrypto.c
+++ b/src/util/vircrypto.c
@@ -329,16 +329,16 @@ uint8_t *
 virCryptoGenerateRandom(size_t nbytes)
 {
 uint8_t *buf;
-int ret;
+int rv;
 
 if (VIR_ALLOC_N(buf, nbytes) < 0)
 return NULL;
 
 #if WITH_GNUTLS
 /* Generate the byte stream using gnutls_rnd() if possible */
-if ((ret = gnutls_rnd(GNUTLS_RND_RANDOM, buf, nbytes)) < 0) {
+if ((rv = gnutls_rnd(GNUTLS_RND_RANDOM, buf, nbytes)) < 0) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
-   _("failed to generate byte stream, ret=%d"), ret);
+   _("failed to generate byte stream, rv=%d"), rv);
 VIR_FREE(buf);
 return NULL;
 }
@@ -346,8 +346,8 @@ virCryptoGenerateRandom(size_t nbytes)
 /* If we don't have gnutls_rnd(), we will generate a less cryptographically
  * strong master buf from /dev/urandom.
  */
-if ((ret = virRandomBytes(buf, nbytes)) < 0) {
-virReportSystemError(-ret, "%s", _("failed to generate byte stream"));
+if ((rv = virRandomBytes(buf, nbytes)) < 0) {
+virReportSystemError(-rv, "%s", _("failed to generate byte stream"));
 VIR_FREE(buf);
 return NULL;
 }
-- 
2.16.1

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

[libvirt] [PATCH 01/10] virRandomBytes: Fix return value

[Michal Privoznik]

In libvirt when a function wants to return an error code it
should be a negative value. Returning a positive value (or zero)
means success. But virRandomBytes() does not follow this rule.

Signed-off-by: Michal Privoznik 
---
 src/util/vircrypto.c  | 4 ++--
 src/util/virrandom.c  | 6 +++---
 src/util/viruuid.c| 4 ++--
 tests/vircryptotest.c | 4 ++--
 4 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/src/util/vircrypto.c b/src/util/vircrypto.c
index bbc2a01f22..4079013d6d 100644
--- a/src/util/vircrypto.c
+++ b/src/util/vircrypto.c
@@ -346,8 +346,8 @@ virCryptoGenerateRandom(size_t nbytes)
 /* If we don't have gnutls_rnd(), we will generate a less cryptographically
  * strong master buf from /dev/urandom.
  */
-if ((ret = virRandomBytes(buf, nbytes))) {
-virReportSystemError(ret, "%s", _("failed to generate byte stream"));
+if ((ret = virRandomBytes(buf, nbytes)) < 0) {
+virReportSystemError(-ret, "%s", _("failed to generate byte stream"));
 VIR_FREE(buf);
 return NULL;
 }
diff --git a/src/util/virrandom.c b/src/util/virrandom.c
index 41daa404b2..9597640840 100644
--- a/src/util/virrandom.c
+++ b/src/util/virrandom.c
@@ -168,7 +168,7 @@ uint32_t virRandomInt(uint32_t max)
  * Generate a stream of random bytes from /dev/urandom
  * into @buf of size @buflen
  *
- * Returns 0 on success or an errno on failure
+ * Returns 0 on success or an -errno on failure
  */
 int
 virRandomBytes(unsigned char *buf,
@@ -177,7 +177,7 @@ virRandomBytes(unsigned char *buf,
 int fd;
 
 if ((fd = open("/dev/urandom", O_RDONLY)) < 0)
-return errno;
+return -errno;
 
 while (buflen > 0) {
 ssize_t n;
@@ -186,7 +186,7 @@ virRandomBytes(unsigned char *buf,
 if (errno == EINTR)
 continue;
 VIR_FORCE_CLOSE(fd);
-return n < 0 ? errno : ENODATA;
+return n < 0 ? -errno : -ENODATA;
 }
 
 buf += n;
diff --git a/src/util/viruuid.c b/src/util/viruuid.c
index 3cbaae0b85..61877aeba4 100644
--- a/src/util/viruuid.c
+++ b/src/util/viruuid.c
@@ -76,11 +76,11 @@ virUUIDGenerate(unsigned char *uuid)
 if (uuid == NULL)
 return -1;
 
-if ((err = virRandomBytes(uuid, VIR_UUID_BUFLEN))) {
+if ((err = virRandomBytes(uuid, VIR_UUID_BUFLEN)) < 0) {
 char ebuf[1024];
 VIR_WARN("Falling back to pseudorandom UUID,"
  " failed to generate random bytes: %s",
- virStrerror(err, ebuf, sizeof(ebuf)));
+ virStrerror(-err, ebuf, sizeof(ebuf)));
 err = virUUIDGeneratePseudoRandomBytes(uuid, VIR_UUID_BUFLEN);
 }
 
diff --git a/tests/vircryptotest.c b/tests/vircryptotest.c
index d9ffc6f34c..b6313e73ad 100644
--- a/tests/vircryptotest.c
+++ b/tests/vircryptotest.c
@@ -88,8 +88,8 @@ testCryptoEncrypt(const void *opaque)
 VIR_ALLOC_N(iv, ivlen) < 0)
 goto cleanup;
 
-if (virRandomBytes(enckey, enckeylen) ||
-virRandomBytes(iv, ivlen)) {
+if (virRandomBytes(enckey, enckeylen) < 0 ||
+virRandomBytes(iv, ivlen) < 0) {
 fprintf(stderr, "Failed to generate random bytes\n");
 goto cleanup;
 }
-- 
2.16.1

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

[libvirt] [PATCH 10/10] vircrypto: Drop virCryptoGenerateRandom

[Michal Privoznik]

Now that virCryptoGenerateRandom() is plain wrapper over
virRandomBytes() we can drop it in favour of the latter.

Signed-off-by: Michal Privoznik 
---
 src/libvirt_private.syms |  1 -
 src/qemu/qemu_domain.c   |  5 +++--
 src/util/vircrypto.c | 18 --
 src/util/vircrypto.h |  3 ---
 tests/qemuxml2argvmock.c |  7 ---
 5 files changed, 3 insertions(+), 31 deletions(-)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 8d381ee11b..18c0c3e954 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1649,7 +1649,6 @@ virConfWriteMem;
 
 # util/vircrypto.h
 virCryptoEncryptData;
-virCryptoGenerateRandom;
 virCryptoHashBuf;
 virCryptoHashString;
 virCryptoHaveCipher;
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 2d13a03344..e49398432f 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -51,6 +51,7 @@
 #include "viratomic.h"
 #include "virprocess.h"
 #include "vircrypto.h"
+#include "virrandom.h"
 #include "virsystemd.h"
 #include "secret_util.h"
 #include "logging/log_manager.h"
@@ -934,7 +935,7 @@ qemuDomainMasterKeyCreate(virDomainObjPtr vm)
 return -1;
 priv->masterKeyLen = QEMU_DOMAIN_MASTER_KEY_LEN;
 
-if (virCryptoGenerateRandom(priv->masterKey, QEMU_DOMAIN_MASTER_KEY_LEN) < 
0)
+if (virRandomBytes(priv->masterKey, QEMU_DOMAIN_MASTER_KEY_LEN) < 0)
 return -1;
 
 return 0;
@@ -1219,7 +1220,7 @@ qemuDomainSecretAESSetup(qemuDomainObjPrivatePtr priv,
 goto cleanup;
 
 /* Create a random initialization vector */
-if (virCryptoGenerateRandom(raw_iv, ivlen) < 0)
+if (virRandomBytes(raw_iv, ivlen) < 0)
 goto cleanup;
 
 /* Encode the IV and save that since qemu will need it */
diff --git a/src/util/vircrypto.c b/src/util/vircrypto.c
index 3f3ba0267a..d734ce6ad7 100644
--- a/src/util/vircrypto.c
+++ b/src/util/vircrypto.c
@@ -314,21 +314,3 @@ virCryptoEncryptData(virCryptoCipher algorithm,
 return -1;
 }
 #endif
-
-/* virCryptoGenerateRandom:
- * @buf: Pointer to location to store bytes
- * @buflen: Number of bytes to store
- *
- * Generate a random stream of @buflen length and store it into @buf.
- *
- * Since the gnutls_rnd could be missing, provide an alternate less
- * secure mechanism to at least have something.
- *
- * Returns 0 on success or -1 on failure (with error reported)
- */
-int
-virCryptoGenerateRandom(unsigned char *buf,
-size_t buflen)
-{
-return virRandomBytes(buf, buflen);
-}
diff --git a/src/util/vircrypto.h b/src/util/vircrypto.h
index 649ceff1a1..e3c70d7d9a 100644
--- a/src/util/vircrypto.h
+++ b/src/util/vircrypto.h
@@ -65,7 +65,4 @@ int virCryptoEncryptData(virCryptoCipher algorithm,
 ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(6)
 ATTRIBUTE_NONNULL(8) ATTRIBUTE_NONNULL(9) ATTRIBUTE_RETURN_CHECK;
 
-int virCryptoGenerateRandom(unsigned char *buf,
-size_t buflen) ATTRIBUTE_NOINLINE;
-
 #endif /* __VIR_CRYPTO_H__ */
diff --git a/tests/qemuxml2argvmock.c b/tests/qemuxml2argvmock.c
index 44b6504de9..a4de7f0c46 100644
--- a/tests/qemuxml2argvmock.c
+++ b/tests/qemuxml2argvmock.c
@@ -190,13 +190,6 @@ virCommandPassFD(virCommandPtr cmd ATTRIBUTE_UNUSED,
 /* nada */
 }
 
-int
-virCryptoGenerateRandom(unsigned char *buf,
-   size_t buflen)
-{
-return virRandomBytes(buf, buflen);
-}
-
 int
 virNetDevOpenvswitchGetVhostuserIfname(const char *path ATTRIBUTE_UNUSED,
char **ifname)
-- 
2.16.1

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

[libvirt] [PATCH 03/10] virCryptoGenerateRandom: Explain gnults error

[Michal Privoznik]

When generating random stream using gnults fails an error is
reported. However, the error is not helpful as it contains only
an integer error code (a negative number). Use gnutls_strerror()
to turn the error code into a string explaining what went wrong.

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

diff --git a/src/util/vircrypto.c b/src/util/vircrypto.c
index 930fa3b215..9879c31555 100644
--- a/src/util/vircrypto.c
+++ b/src/util/vircrypto.c
@@ -323,7 +323,8 @@ virCryptoEncryptData(virCryptoCipher algorithm,
  * Since the gnutls_rnd could be missing, provide an alternate less
  * secure mechanism to at least have something.
  *
- * Returns pointer memory containing byte stream on success, NULL on failure
+ * Returns pointer memory containing byte stream on success,
+ * NULL on failure (with error reported)
  */
 uint8_t *
 virCryptoGenerateRandom(size_t nbytes)
@@ -338,7 +339,8 @@ virCryptoGenerateRandom(size_t nbytes)
 /* Generate the byte stream using gnutls_rnd() if possible */
 if ((rv = gnutls_rnd(GNUTLS_RND_RANDOM, buf, nbytes)) < 0) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
-   _("failed to generate byte stream, rv=%d"), rv);
+   _("failed to generate byte stream: %s"),
+   gnutls_strerror(rv));
 VIR_FREE(buf);
 return NULL;
 }
-- 
2.16.1

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

[libvirt] [PATCH 09/10] virUUIDGenerate don't fall back to virRandomBits

[Michal Privoznik]

If virRandomBytes() fails there is no point calling
virRandomBits() because it uses virRandomBytes() internally
again.

Signed-off-by: Michal Privoznik 
---
 src/util/viruuid.c | 25 +++--
 1 file changed, 3 insertions(+), 22 deletions(-)

diff --git a/src/util/viruuid.c b/src/util/viruuid.c
index 61877aeba4..f588a62ec6 100644
--- a/src/util/viruuid.c
+++ b/src/util/viruuid.c
@@ -48,18 +48,6 @@ VIR_LOG_INIT("util.uuid");
 
 static unsigned char host_uuid[VIR_UUID_BUFLEN];
 
-static int
-virUUIDGeneratePseudoRandomBytes(unsigned char *buf,
- int buflen)
-{
-while (buflen > 0) {
-*buf++ = virRandomBits(8);
-buflen--;
-}
-
-return 0;
-}
-
 /**
  * virUUIDGenerate:
  * @uuid: array of VIR_UUID_BUFLEN bytes to store the new UUID
@@ -71,18 +59,11 @@ virUUIDGeneratePseudoRandomBytes(unsigned char *buf,
 int
 virUUIDGenerate(unsigned char *uuid)
 {
-int err;
-
 if (uuid == NULL)
 return -1;
 
-if ((err = virRandomBytes(uuid, VIR_UUID_BUFLEN)) < 0) {
-char ebuf[1024];
-VIR_WARN("Falling back to pseudorandom UUID,"
- " failed to generate random bytes: %s",
- virStrerror(-err, ebuf, sizeof(ebuf)));
-err = virUUIDGeneratePseudoRandomBytes(uuid, VIR_UUID_BUFLEN);
-}
+if (virRandomBytes(uuid, VIR_UUID_BUFLEN) < 0)
+return -1;
 
 /*
  * Make UUID RFC 4122 compliant. Following form will be used:
@@ -103,7 +84,7 @@ virUUIDGenerate(unsigned char *uuid)
 uuid[6] = (uuid[6] & 0x0F) | (4 << 4);
 uuid[8] = (uuid[8] & 0x3F) | (2 << 6);
 
-return err;
+return 0;
 }
 
 /**
-- 
2.16.1

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

[libvirt] [PATCH 07/10] virRandomBytes: Use gnutls_rnd whenever possible

[Michal Privoznik]

While /dev/urandom is not terrible source of random data
gnutls_rnd is better. Prefer that one.

Also, since nearly every platform we build on already has gnutls
(if not all of them) this is going to be used by default.

Signed-off-by: Michal Privoznik 
---
 src/util/vircrypto.c | 20 +---
 src/util/virrandom.c | 18 ++
 2 files changed, 19 insertions(+), 19 deletions(-)

diff --git a/src/util/vircrypto.c b/src/util/vircrypto.c
index e5f2319720..3f3ba0267a 100644
--- a/src/util/vircrypto.c
+++ b/src/util/vircrypto.c
@@ -330,23 +330,5 @@ int
 virCryptoGenerateRandom(unsigned char *buf,
 size_t buflen)
 {
-#if WITH_GNUTLS
-int rv;
-
-/* Generate the byte stream using gnutls_rnd() if possible */
-if ((rv = gnutls_rnd(GNUTLS_RND_RANDOM, buf, buflen)) < 0) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
-   _("failed to generate byte stream: %s"),
-   gnutls_strerror(rv));
-return -1;
-}
-#else
-/* If we don't have gnutls_rnd(), we will generate a less cryptographically
- * strong master buf from /dev/urandom.
- */
-if (virRandomBytes(buf, buflen) < 0)
-return -1;
-#endif
-
-return 0;
+return virRandomBytes(buf, buflen);
 }
diff --git a/src/util/virrandom.c b/src/util/virrandom.c
index 230745d311..444b0f9802 100644
--- a/src/util/virrandom.c
+++ b/src/util/virrandom.c
@@ -29,6 +29,10 @@
 #include 
 #include 
 #include 
+#ifdef WITH_GNUTLS
+# include 
+# include 
+#endif
 
 #include "virrandom.h"
 #include "virthread.h"
@@ -175,6 +179,19 @@ int
 virRandomBytes(unsigned char *buf,
size_t buflen)
 {
+#if WITH_GNUTLS
+int rv;
+
+/* Generate the byte stream using gnutls_rnd() if possible */
+if ((rv = gnutls_rnd(GNUTLS_RND_RANDOM, buf, buflen)) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("failed to generate byte stream: %s"),
+   gnutls_strerror(rv));
+return -1;
+}
+
+#else /* !WITH_GNUTLS */
+
 int fd;
 
 if ((fd = open(RANDOM_SOURCE, O_RDONLY)) < 0) {
@@ -200,6 +217,7 @@ virRandomBytes(unsigned char *buf,
 }
 
 VIR_FORCE_CLOSE(fd);
+#endif /* !WITH_GNUTLS */
 
 return 0;
 }
-- 
2.16.1

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

[libvirt] [PATCH 05/10] virRandomBytes: Prefer saferead over plain read

[Michal Privoznik]

Signed-off-by: Michal Privoznik 
---
 src/util/virrandom.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/src/util/virrandom.c b/src/util/virrandom.c
index 9597640840..ea55fe654d 100644
--- a/src/util/virrandom.c
+++ b/src/util/virrandom.c
@@ -182,9 +182,7 @@ virRandomBytes(unsigned char *buf,
 while (buflen > 0) {
 ssize_t n;
 
-if ((n = read(fd, buf, buflen)) <= 0) {
-if (errno == EINTR)
-continue;
+if ((n = saferead(fd, buf, buflen)) <= 0) {
 VIR_FORCE_CLOSE(fd);
 return n < 0 ? -errno : -ENODATA;
 }
-- 
2.16.1

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

[libvirt] [PATCH 08/10] virrandom: Make virRandomBits better

[Michal Privoznik]

Now that we have strong PRNG generator implemented in
virRandomBytes() let's use that instead of gnulib's random_r.

Problem with the latter is in way we seed it: current UNIX time
and libvirtd's PID are not that random as one might think.
Imagine two hosts booting at the same time. There's a fair chance
that those hosts spawn libvirtds at the same time and with the
same PID. This will result in both daemons generating the same
sequence of say MAC addresses [1].

1: https://www.redhat.com/archives/libvirt-users/2018-May/msg00097.html

Signed-off-by: Michal Privoznik 
---
 src/util/virrandom.c | 63 ++--
 1 file changed, 2 insertions(+), 61 deletions(-)

diff --git a/src/util/virrandom.c b/src/util/virrandom.c
index 444b0f9802..01cc82a052 100644
--- a/src/util/virrandom.c
+++ b/src/util/virrandom.c
@@ -49,53 +49,6 @@ VIR_LOG_INIT("util.random");
 
 #define RANDOM_SOURCE "/dev/urandom"
 
-/* The algorithm of virRandomBits relies on gnulib's guarantee that
- * 'random_r' matches the POSIX requirements on 'random' of being
- * evenly distributed among exactly [0, 2**31) (that is, we always get
- * exactly 31 bits).  While this happens to be the value of RAND_MAX
- * on glibc, note that POSIX only requires RAND_MAX to be tied to the
- * weaker 'rand', so there are platforms where RAND_MAX is smaller
- * than the range of 'random_r'.  For the results to be evenly
- * distributed among up to 64 bits, we also rely on the period of
- * 'random_r' to be at least 2**64, which POSIX only guarantees for
- * 'random' if you use 256 bytes of state.  */
-enum {
-RANDOM_BITS_PER_ITER = 31,
-RANDOM_BITS_MASK = (1U << RANDOM_BITS_PER_ITER) - 1,
-RANDOM_STATE_SIZE = 256,
-};
-
-static char randomState[RANDOM_STATE_SIZE];
-static struct random_data randomData;
-static virMutex randomLock = VIR_MUTEX_INITIALIZER;
-
-
-static int
-virRandomOnceInit(void)
-{
-unsigned int seed = time(NULL) ^ getpid();
-
-#if 0
-/* Normally we want a decent seed.  But if reproducible debugging
- * of a fixed pseudo-random sequence is ever required, uncomment
- * this block to let an environment variable force the seed.  */
-const char *debug = virGetEnvBlockSUID("VIR_DEBUG_RANDOM_SEED");
-
-if (debug && virStrToLong_ui(debug, NULL, 0, &seed) < 0)
-return -1;
-#endif
-
-if (initstate_r(seed,
-randomState,
-sizeof(randomState),
-&randomData) < 0)
-return -1;
-
-return 0;
-}
-
-VIR_ONCE_GLOBAL_INIT(virRandom)
-
 /**
  * virRandomBits:
  * @nbits: Number of bits of randommess required
@@ -108,26 +61,14 @@ VIR_ONCE_GLOBAL_INIT(virRandom)
 uint64_t virRandomBits(int nbits)
 {
 uint64_t ret = 0;
-int32_t bits;
 
-if (virRandomInitialize() < 0) {
+if (virRandomBytes((unsigned char *) &ret, sizeof(ret)) < 0) {
 /* You're already hosed, so this particular non-random value
  * isn't any worse.  */
 return 0;
 }
 
-virMutexLock(&randomLock);
-
-while (nbits > RANDOM_BITS_PER_ITER) {
-random_r(&randomData, &bits);
-ret = (ret << RANDOM_BITS_PER_ITER) | (bits & RANDOM_BITS_MASK);
-nbits -= RANDOM_BITS_PER_ITER;
-}
-
-random_r(&randomData, &bits);
-ret = (ret << nbits) | (bits & ((1 << nbits) - 1));
-
-virMutexUnlock(&randomLock);
+ret &= (1U << nbits) - 1;
 return ret;
 }
 
-- 
2.16.1

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

[libvirt] [PATCH 04/10] virCryptoGenerateRandom: Don't allocate return buffer

[Michal Privoznik]

To unify our vir*Random() functions we need to make
virCryptoGenerateRandom NOT allocate return buffer. It should
just fill given buffer with random data.

Signed-off-by: Michal Privoznik 
---
 src/qemu/qemu_domain.c   | 12 
 src/util/vircrypto.c | 29 -
 src/util/vircrypto.h |  3 ++-
 tests/qemuxml2argvmock.c | 14 --
 4 files changed, 26 insertions(+), 32 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 47910acb83..2d13a03344 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -930,12 +930,13 @@ qemuDomainMasterKeyCreate(virDomainObjPtr vm)
 if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_OBJECT_SECRET))
 return 0;
 
-if (!(priv->masterKey =
-  virCryptoGenerateRandom(QEMU_DOMAIN_MASTER_KEY_LEN)))
+if (VIR_ALLOC_N(priv->masterKey, QEMU_DOMAIN_MASTER_KEY_LEN) < 0)
 return -1;
-
 priv->masterKeyLen = QEMU_DOMAIN_MASTER_KEY_LEN;
 
+if (virCryptoGenerateRandom(priv->masterKey, QEMU_DOMAIN_MASTER_KEY_LEN) < 
0)
+return -1;
+
 return 0;
 }
 
@@ -1214,8 +1215,11 @@ qemuDomainSecretAESSetup(qemuDomainObjPrivatePtr priv,
 if (!(secinfo->s.aes.alias = qemuDomainGetSecretAESAlias(srcalias, 
isLuks)))
 goto cleanup;
 
+if (VIR_ALLOC_N(raw_iv, ivlen) < 0)
+goto cleanup;
+
 /* Create a random initialization vector */
-if (!(raw_iv = virCryptoGenerateRandom(ivlen)))
+if (virCryptoGenerateRandom(raw_iv, ivlen) < 0)
 goto cleanup;
 
 /* Encode the IV and save that since qemu will need it */
diff --git a/src/util/vircrypto.c b/src/util/vircrypto.c
index 9879c31555..673e1648e8 100644
--- a/src/util/vircrypto.c
+++ b/src/util/vircrypto.c
@@ -316,44 +316,39 @@ virCryptoEncryptData(virCryptoCipher algorithm,
 #endif
 
 /* virCryptoGenerateRandom:
- * @nbytes: Size in bytes of random byte stream to generate
+ * @buf: Pointer to location to store bytes
+ * @buflen: Number of bytes to store
  *
- * Generate a random stream of nbytes length and return it.
+ * Generate a random stream of @buflen length and store it into @buf.
  *
  * Since the gnutls_rnd could be missing, provide an alternate less
  * secure mechanism to at least have something.
  *
- * Returns pointer memory containing byte stream on success,
- * NULL on failure (with error reported)
+ * Returns 0 on success or -1 on failure (with error reported)
  */
-uint8_t *
-virCryptoGenerateRandom(size_t nbytes)
+int
+virCryptoGenerateRandom(unsigned char *buf,
+size_t buflen)
 {
-uint8_t *buf;
 int rv;
 
-if (VIR_ALLOC_N(buf, nbytes) < 0)
-return NULL;
-
 #if WITH_GNUTLS
 /* Generate the byte stream using gnutls_rnd() if possible */
-if ((rv = gnutls_rnd(GNUTLS_RND_RANDOM, buf, nbytes)) < 0) {
+if ((rv = gnutls_rnd(GNUTLS_RND_RANDOM, buf, buflen)) < 0) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
_("failed to generate byte stream: %s"),
gnutls_strerror(rv));
-VIR_FREE(buf);
-return NULL;
+return -1;
 }
 #else
 /* If we don't have gnutls_rnd(), we will generate a less cryptographically
  * strong master buf from /dev/urandom.
  */
-if ((rv = virRandomBytes(buf, nbytes)) < 0) {
+if ((rv = virRandomBytes(buf, buflen)) < 0) {
 virReportSystemError(-rv, "%s", _("failed to generate byte stream"));
-VIR_FREE(buf);
-return NULL;
+return -1;
 }
 #endif
 
-return buf;
+return 0;
 }
diff --git a/src/util/vircrypto.h b/src/util/vircrypto.h
index 9b5dada53d..649ceff1a1 100644
--- a/src/util/vircrypto.h
+++ b/src/util/vircrypto.h
@@ -65,6 +65,7 @@ int virCryptoEncryptData(virCryptoCipher algorithm,
 ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(6)
 ATTRIBUTE_NONNULL(8) ATTRIBUTE_NONNULL(9) ATTRIBUTE_RETURN_CHECK;
 
-uint8_t *virCryptoGenerateRandom(size_t nbytes) ATTRIBUTE_NOINLINE;
+int virCryptoGenerateRandom(unsigned char *buf,
+size_t buflen) ATTRIBUTE_NOINLINE;
 
 #endif /* __VIR_CRYPTO_H__ */
diff --git a/tests/qemuxml2argvmock.c b/tests/qemuxml2argvmock.c
index 6d78063f00..44b6504de9 100644
--- a/tests/qemuxml2argvmock.c
+++ b/tests/qemuxml2argvmock.c
@@ -190,17 +190,11 @@ virCommandPassFD(virCommandPtr cmd ATTRIBUTE_UNUSED,
 /* nada */
 }
 
-uint8_t *
-virCryptoGenerateRandom(size_t nbytes)
+int
+virCryptoGenerateRandom(unsigned char *buf,
+   size_t buflen)
 {
-uint8_t *buf;
-
-if (VIR_ALLOC_N(buf, nbytes) < 0)
-return NULL;
-
-ignore_value(virRandomBytes(buf, nbytes));
-
-return buf;
+return virRandomBytes(buf, buflen);
 }
 
 int
-- 
2.16.1

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

[libvirt] [PATCH 06/10] virRandomBytes: Report error

[Michal Privoznik]

Instead of having each caller report error move it into the
function. This way we can produce more accurate error messages
too.

Signed-off-by: Michal Privoznik 
---
 src/util/vircrypto.c |  6 ++
 src/util/virrandom.c | 18 +-
 2 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/src/util/vircrypto.c b/src/util/vircrypto.c
index 673e1648e8..e5f2319720 100644
--- a/src/util/vircrypto.c
+++ b/src/util/vircrypto.c
@@ -330,9 +330,9 @@ int
 virCryptoGenerateRandom(unsigned char *buf,
 size_t buflen)
 {
+#if WITH_GNUTLS
 int rv;
 
-#if WITH_GNUTLS
 /* Generate the byte stream using gnutls_rnd() if possible */
 if ((rv = gnutls_rnd(GNUTLS_RND_RANDOM, buf, buflen)) < 0) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -344,10 +344,8 @@ virCryptoGenerateRandom(unsigned char *buf,
 /* If we don't have gnutls_rnd(), we will generate a less cryptographically
  * strong master buf from /dev/urandom.
  */
-if ((rv = virRandomBytes(buf, buflen)) < 0) {
-virReportSystemError(-rv, "%s", _("failed to generate byte stream"));
+if (virRandomBytes(buf, buflen) < 0)
 return -1;
-}
 #endif
 
 return 0;
diff --git a/src/util/virrandom.c b/src/util/virrandom.c
index ea55fe654d..230745d311 100644
--- a/src/util/virrandom.c
+++ b/src/util/virrandom.c
@@ -43,6 +43,8 @@
 
 VIR_LOG_INIT("util.random");
 
+#define RANDOM_SOURCE "/dev/urandom"
+
 /* The algorithm of virRandomBits relies on gnulib's guarantee that
  * 'random_r' matches the POSIX requirements on 'random' of being
  * evenly distributed among exactly [0, 2**31) (that is, we always get
@@ -107,7 +109,6 @@ uint64_t virRandomBits(int nbits)
 if (virRandomInitialize() < 0) {
 /* You're already hosed, so this particular non-random value
  * isn't any worse.  */
-VIR_WARN("random number generation is broken");
 return 0;
 }
 
@@ -165,10 +166,10 @@ uint32_t virRandomInt(uint32_t max)
  * @buf: Pointer to location to store bytes
  * @buflen: Number of bytes to store
  *
- * Generate a stream of random bytes from /dev/urandom
+ * Generate a stream of random bytes from RANDOM_SOURCE
  * into @buf of size @buflen
  *
- * Returns 0 on success or an -errno on failure
+ * Returns 0 on success or -1 (with error reported)
  */
 int
 virRandomBytes(unsigned char *buf,
@@ -176,13 +177,20 @@ virRandomBytes(unsigned char *buf,
 {
 int fd;
 
-if ((fd = open("/dev/urandom", O_RDONLY)) < 0)
-return -errno;
+if ((fd = open(RANDOM_SOURCE, O_RDONLY)) < 0) {
+virReportSystemError(errno,
+ _("unable to open %s"),
+ RANDOM_SOURCE);
+return -1;
+}
 
 while (buflen > 0) {
 ssize_t n;
 
 if ((n = saferead(fd, buf, buflen)) <= 0) {
+virReportSystemError(errno,
+ _("unable to read from %s"),
+ RANDOM_SOURCE);
 VIR_FORCE_CLOSE(fd);
 return n < 0 ? -errno : -ENODATA;
 }
-- 
2.16.1

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

Re: [libvirt] [PATCHv2 3/7] conf: introduce element

[Peter Krempa]

On Thu, May 24, 2018 at 12:39:11 +0200, Ján Tomko wrote:
> Add a new 'vsock' element for the vsock device.
> The 'model' attribute is optional.
> A  subelement should be used to specify the guest cid,
> or  should be used.
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1291851
> ---
>  docs/formatdomain.html.in |  20 +++
>  docs/schemas/domaincommon.rng |  29 
>  src/conf/domain_conf.c| 195 
> +-
>  src/conf/domain_conf.h|  17 +++
>  src/qemu/qemu_domain.c|   1 +
>  src/qemu/qemu_domain_address.c|  11 ++
>  src/qemu/qemu_driver.c|   6 +
>  src/qemu/qemu_hotplug.c   |   1 +
>  tests/qemuxml2argvdata/vhost-vsock-auto.xml   |  35 +
>  tests/qemuxml2argvdata/vhost-vsock.xml|  36 +
>  tests/qemuxml2xmloutdata/vhost-vsock-auto.xml |  36 +
>  tests/qemuxml2xmloutdata/vhost-vsock.xml  |   1 +
>  tests/qemuxml2xmltest.c   |   3 +
>  13 files changed, 390 insertions(+), 1 deletion(-)
>  create mode 100644 tests/qemuxml2argvdata/vhost-vsock-auto.xml
>  create mode 100644 tests/qemuxml2argvdata/vhost-vsock.xml
>  create mode 100644 tests/qemuxml2xmloutdata/vhost-vsock-auto.xml
>  create mode 12 tests/qemuxml2xmloutdata/vhost-vsock.xml

I've recently added a new swithc case which breaks with the following
message:

qemu/qemu_domain.c: In function 'qemuDomainDeviceDefPostParse':
qemu/qemu_domain.c:5802:5: error: enumeration value 'VIR_DOMAIN_DEVICE_VSOCK' 
not handled in switch [-Werror=switch-enum]
 switch ((virDomainDeviceType) dev->type) {
 ^~

Apply the following patch:

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 527781c0fa..3e8e6358de 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -5845,6 +5845,7 @@ qemuDomainDeviceDefPostParse(virDomainDeviceDefPtr dev,
 case VIR_DOMAIN_DEVICE_TPM:
 case VIR_DOMAIN_DEVICE_MEMORY:
 case VIR_DOMAIN_DEVICE_IOMMU:
+case VIR_DOMAIN_DEVICE_VSOCK:
 ret = 0;
 break;



> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 0d0fd3b9f3..bfe7f757b8 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -8133,6 +8133,26 @@ qemu-kvm -net nic,model=? /dev/null
>
>  
>  
> +Vsock
> +
> +A vsock host/guest interface. The model attribute
> +defaults to virtio.
> +The optional attribute cid of the source element
> +specifies the CID assigned to the guest. If the attribute
> +auto is set to yes, libvirt
> +will assign a free CID automatically on domain startup.
> +Since 4.4.0

Should we perhaps mention some docs where users can figure out how to
use it?

Also please mention what the default value of 'auto' is. e.g. what the
following definition actually configures:



> +
> +
> +...
> +
> +  
> +
> +  
> +
> +...
> +
> +
>  Security label
>  
>  
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 71ac3d079c..3ea5c91773 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -4140,6 +4140,32 @@
>  
>
>  
> +  
> +
> +  
> +
> +  virtio
> +
> +  
> +  
> +

So, source is mandatory? It should be noted in the docs ...

> +  
> +
> +  
> +
> +  
> +  
> +
> +  
> +
> +  

Also both attributes are optional which is kind of weird if  is
mandatory.

> +
> +
> +  
> +
> +  
> +
> +  

[...]

> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index b2982fc3d4..1a3dbf884b 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c

[...]

> @@ -15878,6 +15911,68 @@ virDomainIOMMUDefParseXML(xmlNodePtr node,
>  }
>  
>  
> +static virDomainVsockDefPtr
> +virDomainVsockDefParseXML(virDomainXMLOptionPtr xmlopt,
> +  xmlNodePtr node,
> +  xmlXPathContextPtr ctxt,
> +  unsigned int flags)
> +{
> +virDomainVsockDefPtr vsock = NULL, ret = NULL;
> +xmlNodePtr save = ctxt->node;
> +xmlNodePtr source;
> +char *tmp = NULL;
> +int val;
> +
> +ctxt->node = node;
> +
> +if (!(vsock = virDomainVsockDefNew(xmlopt)))
> +goto cleanup;
> +
> +if ((tmp = virXMLPropString(node, "model"))) {
> +if ((val = virDomainVsockModelTypeFromString(tmp)) < 0) {

Model 'default' should not be allowed.

> +virReportError(VIR_ERR_XML_ERROR, _("unknown vsock model: %s"), 
> tmp);
> +goto cleanup;
> +}
> +vsock->model = val;
> +}
> +
> +source = virXPathNode("./source", ctxt);
> +
> +VIR_FR

Re: [libvirt] [PATCH 2/4] conf: introduce virDomainDefCheckBootOrder

[Peter Krempa]

On Tue, May 29, 2018 at 09:55:18 +0200, Ján Tomko wrote:
> On Tue, May 29, 2018 at 09:26:51AM +0200, Peter Krempa wrote:
> > On Mon, May 28, 2018 at 15:54:03 +0200, Ján Tomko wrote:
> > > Move the check for boot elements into a separate function
> > > and remove its dependency on the parser-supplied bootHash table.
> > > 
> > > Reconstructing the hash table from the domain definition
> > > effectively duplicates the check for duplicate boot order
> > > values, also present in virDomainDeviceBootParseXML.
> > 
> 
> How about:
> Now it will also be run on domains created by other means than XML
> parsing, since it will be run even for code paths that did not supply
> the bootHash table before.

Sounds goog to me.

> 
> > So the semantical difference is that places that call into the
> > post-parse infrastructure which construct the domain definition object
> > by other means that parsing XML will get the same treatment as when XML
> > is parsed.
> > 
> > > 
> > > Signed-off-by: Ján Tomko 
> > > ---
> > >  src/conf/domain_conf.c   | 89 
> > > +++-
> > >  tests/qemuargv2xmldata/nomachine-aarch64.xml |  1 +
> > >  tests/qemuargv2xmldata/nomachine-ppc64.xml   |  1 +
> > >  tests/qemuargv2xmldata/nomachine-x86_64.xml  |  1 +
> > >  tests/sexpr2xmldata/sexpr2xml-fv-kernel.xml  |  1 +
> > >  5 files changed, 78 insertions(+), 15 deletions(-)
> > > 
> > > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> > > index d6ac47c629..f087a3680f 100644
> > > --- a/src/conf/domain_conf.c
> > > +++ b/src/conf/domain_conf.c
> > > @@ -4939,10 +4939,79 @@ virDomainDefPostParseCPU(virDomainDefPtr def)
> > 
> > [...]
> > 
> > > +
> > > +
> > > +static int
> > > +virDomainDefCheckBootOrder(virDomainDefPtr def)
> > 
> > [1]
> > 
> > > +{
> > > +virHashTablePtr bootHash = NULL;
> > > +int ret = -1;
> > > +
> > > +if (def->os.type != VIR_DOMAIN_OSTYPE_HVM)
> > > +return 0;
> > 
> > Please do this check outside of this function. If this function is
> > invoked it should o it's job, especially since you also have other
> > conditions when to invoke it outside.
> > 
> > > +
> > > +if (!(bootHash = virHashCreate(5, NULL)))
> > > +goto cleanup;
> > > +
> > > +if (virDomainDeviceInfoIterate(def, virDomainDefCollectBootOrder, 
> > > bootHash) < 0)
> > > +goto cleanup;
> > > +
> > > +if (def->os.nBootDevs > 0 && virHashSize(bootHash) > 0) {
> > > +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> > > +   _("per-device boot elements cannot be used"
> > > + " together with os/boot elements"));
> > > +goto cleanup;
> > > +}
> > > +
> > > +if (def->os.nBootDevs == 0 && virHashSize(bootHash) == 0) {
> > > +def->os.nBootDevs = 1;
> > > +def->os.bootDevs[0] = VIR_DOMAIN_BOOT_DISK;
> > 
> > [1] this in contrast to the name of the function modifies stuff ...
> 
> I can rename it to virDomainDefBootOrderPostParse

ACK to that

> 
> Jano




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

Re: [libvirt] [PATCHv2 4/7] qemu: add private data for vsock

[Peter Krempa]

On Thu, May 24, 2018 at 12:39:12 +0200, Ján Tomko wrote:
> Introduce a structure and a class that will be used to store
> the private data.
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1291851
> 
> Signed-off-by: Ján Tomko 
> ---
>  src/qemu/qemu_domain.c | 36 
>  src/qemu/qemu_domain.h |  9 +
>  2 files changed, 45 insertions(+)

ACK


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

Re: [libvirt] [PATCHv2 5/7] Introduce QEMU_CAPS_DEVICE_VHOST_VSOCK

[Peter Krempa]

On Thu, May 24, 2018 at 12:39:13 +0200, Ján Tomko wrote:
> Add a new capability flag for vhost-vsock-device
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1291851
> 
> Signed-off-by: Ján Tomko 
> ---

ACK


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

Re: [libvirt] [PATCH 3/4] conf: remove 'bootHash' from the post-parse infrastructure

[Ján Tomko]

On Tue, May 29, 2018 at 09:27:53AM +0200, Peter Krempa wrote:

On Mon, May 28, 2018 at 15:54:04 +0200, Ján Tomko wrote:

From: Peter Krempa 

As the function signature of virDomainDefPostParseInternal does not
differ from virDomainDefPostParse now, the wrapper can be dropped.

Signed-off-by: Peter Krempa 
Signed-off-by: Ján Tomko 
---
 src/conf/domain_conf.c | 33 +
 1 file changed, 9 insertions(+), 24 deletions(-)


Implicit ACK. Also please push the rest of the patches from my series
which depend on this when you'll be pushing this.


Done.

Jano


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

Re: [libvirt] [PATCHv2 6/7] util: create virvsock.c

[Peter Krempa]

On Thu, May 24, 2018 at 12:39:14 +0200, Ján Tomko wrote:
> A file for vsock-related helper functions.
> virVsockSetGuestCid to set an already-known CID,
> virVsockAcquireGuestCid that will use the first available CID
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1291851
> 
> Signed-off-by: Ján Tomko 
> ---
>  configure.ac |  8 +
>  src/libvirt_private.syms |  5 +++
>  src/util/Makefile.inc.am |  2 ++
>  src/util/virvsock.c  | 89 
> 
>  src/util/virvsock.h  | 29 
>  5 files changed, 133 insertions(+)
>  create mode 100644 src/util/virvsock.c
>  create mode 100644 src/util/virvsock.h
> 

[...]

> diff --git a/src/util/virvsock.c b/src/util/virvsock.c
> new file mode 100644
> index 00..8a5c88700b
> --- /dev/null
> +++ b/src/util/virvsock.c
> @@ -0,0 +1,89 @@
> +/*
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library.  If not, see
> + * .
> + *
> + */
> +
> +#include 
> +
> +#include 
> +/* #include  */

Leftover historic includes?

> +
> +#if HAVE_DECL_VHOST_VSOCK_SET_GUEST_CID
> +# include 
> +#endif
> +
> +#include "virvsock.h"
> +
> +#include "virerror.h"
> +#include "virlog.h"
> +
> +
> +#define VIR_FROM_THIS VIR_FROM_NONE
> +
> +VIR_LOG_INIT("util.vsock");
> +
> +#if HAVE_DECL_VHOST_VSOCK_SET_GUEST_CID
> +static int
> +virVsockSetGuestCidQuiet(int fd,
> + unsigned int guest_cid)
> +{
> +uint64_t val = guest_cid;
> +
> +return ioctl(fd, VHOST_VSOCK_SET_GUEST_CID, &val);
> +}
> +
> +#else
> +static int
> +virVsockSetGuestCidQuiet(int fd ATTRIBUTE_UNUSED,
> + unsigned int guest_cid ATTRIBUTE_UNUSED)
> +{
> +errno = ENOSYS;
> +return -1;
> +}
> +#endif
> +
> +
> +int
> +virVsockSetGuestCid(int fd,
> +unsigned int guest_cid)

Missing docs for the internal public API.

> +{
> +if (virVsockSetGuestCidQuiet(fd, guest_cid) < 0) {
> +virReportSystemError(errno, "%s",
> + _("failed to set guest cid"));
> +return -1;
> +}
> +
> +return 0;
> +}
> +
> +#define VIR_VSOCK_GUEST_CID_MIN 3
> +
> +int
> +virVsockAcquireGuestCid(int fd,
> +unsigned int *guest_cid)

Missing docs for the internal public API.


> +{
> +unsigned int cid = VIR_VSOCK_GUEST_CID_MIN;
> +
> +for (; virVsockSetGuestCidQuiet(fd, cid) < 0; cid++) {

It might be worth using an internal static atomic variable as a start of
the loop so that we don't iterate cids which may still be in use.

> +if (errno != EADDRINUSE) {
> +virReportSystemError(errno, "%s",
> + _("failed to acquire guest cid"));
> +return -1;
> +}
> +}
> +*guest_cid = cid;
> +
> +return 0;
> +}

ACK if you add the docs and remove the commented out include.


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

Re: [libvirt] [PATCH 07/13] qemu: Add hotpluging support for PCI devices on S390 guests

[Cornelia Huck]

On Mon, 28 May 2018 16:26:57 +0800
Xiao Feng Ren  wrote:

> On 5/25/2018 6:22 PM, Bjoern Walk wrote:
> > Cornelia Huck  [2018-05-24, 06:25PM +0200]:  
> >> On Thu, 24 May 2018 14:24:32 +0200
> >> Xiao Feng Ren  wrote:
> >>  
> >>> From: Yi Min Zhao 
> >>>
> >>> This commit adds hotplug support for PCI devices on S390 guests.
> >>> There's no need to implement hot unplug for zPCI as QEMU implements
> >>> an unplug callback which will unplug both PCI and zPCI device in a
> >>> cascaded way.
> >>> Currently, the following PCI devices are supported:
> >>>virtio-blk-pci
> >>>virtio-net-pci
> >>>virtio-rng-pci
> >>>virtio-input-host-pci
> >>>virtio-keyboard-pci
> >>>virtio-mouse-pci
> >>>virtio-tablet-pci
> >>>vfio-pci
> >>>Shmem device
> >>>SCSIVhost device  
> >> Hm, how did you arrive at this list? Is it 'anything that uses msi-x'?  
> > I guess it's just any device that libvirt actually supports hotplug for,
> > with a few exceptions (cf. patch 3 and 6). Not familiar wuth msi-x.  
> 
> The list should be the devices that support MSI-X and have realized the 
> hotplug in qemu,
> we support the hotplug for them in the libvirt.  So the list will be 
> updated to:
> 
>virtio-blk-pci
>virtio-net-pci
>virtio-input-host-pci
>virtio-keyboard-pci
>virtio-mouse-pci
>virtio-tablet-pci
>vfio-pci
>SCSIVhost device

Ok, that makes sense.

I also checked that libvirt only allows setting the vectors property
for virtio-serial and shmem (vectors == 0 turns msi-x off), so that
should be fine.

> 
> As the model of Shmem is not support in qemu, the rng device doesn't 
> support MSI-X.  So remove them.

So, should qemu add support for msi-x with virtio-rng-pci in the
future, we'd want to update libvirt as well, correct?

> 
> 
> In addition, the bug found need to be fixed for the hotplug of tablet, 
> in the virDomainDeviceIsUSB()
> if ((t == VIR_DOMAIN_DEVICE_DISK &&
>   dev->data.disk->bus == VIR_DOMAIN_DISK_BUS_USB) ||
>      (t == VIR_DOMAIN_DEVICE_INPUT &&
>   dev->data.input->type == VIR_DOMAIN_INPUT_BUS_USB) ||  
> -->dev->data.input->bus == VIR_DOMAIN_INPUT_BUS_USB  
>      (t == VIR_DOMAIN_DEVICE_HOSTDEV &&
> I will send the fix if this bug is agreed.
> 


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

Re: [libvirt] [PATCHv2 7/7] qemu: add support for vhost-vsock-pci

[Peter Krempa]

On Thu, May 24, 2018 at 12:39:15 +0200, Ján Tomko wrote:
> Create a new vsock endpoint by opening /dev/vhost-vsock,
> set the requested CID via ioctl (or assign a free one if auto='yes'),
> pass the file descriptor to QEMU and build the command line.
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1291851
> Signed-off-by: Ján Tomko 
> ---
>  src/qemu/qemu_alias.c  | 16 
>  src/qemu/qemu_command.c| 45 
> ++
>  src/qemu/qemu_domain.c |  5 +++
>  src/qemu/qemu_domain.h |  2 +-
>  src/qemu/qemu_process.c| 35 +
>  .../vhost-vsock-auto.x86_64-latest.args| 32 +++
>  .../vhost-vsock.x86_64-latest.args | 32 +++
>  tests/qemuxml2argvtest.c   | 14 +++
>  8 files changed, 180 insertions(+), 1 deletion(-)
>  create mode 100644 tests/qemuxml2argvdata/vhost-vsock-auto.x86_64-latest.args
>  create mode 100644 tests/qemuxml2argvdata/vhost-vsock.x86_64-latest.args

[...]

> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 9da2d609e8..ef0716d683 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -9865,6 +9865,47 @@ qemuBuildSeccompSandboxCommandLine(virCommandPtr cmd,
>  }
>  
>  
> +static int
> +qemuBuildVsockCommandLine(virCommandPtr cmd,
> +  virDomainDefPtr def,
> +  virDomainVsockDefPtr vsock,
> +  const char *fdprefix,

fdprefix is always empty string, so why is it necessary?

[1]

> +  virQEMUCapsPtr qemuCaps)
> +{
> +qemuDomainVsockPrivatePtr priv = 
> (qemuDomainVsockPrivatePtr)vsock->privateData;
> +const char *device = "vhost-vsock-pci";
> +virBuffer buf = VIR_BUFFER_INITIALIZER;
> +char *devstr = NULL;
> +int ret = -1;
> +
> +virBufferAsprintf(&buf, "%s", device);
> +virBufferAsprintf(&buf, ",id=%s", vsock->info.alias);
> +virBufferAsprintf(&buf, ",guest-cid=%u", vsock->guest_cid);
> +virBufferAsprintf(&buf, ",vhostfd=%s%u", fdprefix, priv->vhostfd);
> +if (qemuBuildDeviceAddressStr(&buf, def, &vsock->info, qemuCaps) < 0)
> +goto cleanup;
> +#if 0
> +if (qemuBuildVirtioOptionsStr(&buf, net->virtio, qemuCaps) < 0)
> +goto error;
> +#endif

Leftover stuff from previous version?

> +
> +if (virBufferCheckError(&buf) < 0)
> +goto cleanup;
> +
> +devstr = virBufferContentAndReset(&buf);
> +
> +virCommandPassFD(cmd, priv->vhostfd, VIR_COMMAND_PASS_FD_CLOSE_PARENT);
> +priv->vhostfd = -1;
> +virCommandAddArgList(cmd, "-device", devstr, NULL);
> +
> +ret = 0;
> + cleanup:
> +virBufferFreeAndReset(&buf);
> +VIR_FREE(devstr);
> +return ret;
> +}
> +
> +
>  /*
>   * Constructs a argv suitable for launching qemu with config defined
>   * for a given virtual machine.
> @@ -10111,6 +10152,10 @@ qemuBuildCommandLine(virQEMUDriverPtr driver,
>  goto error;
>  }
>  
> +if (def->vsock &&
> +qemuBuildVsockCommandLine(cmd, def, def->vsock, "", qemuCaps) < 0)

[1]

> +goto error;
> +
>  /* In some situations, eg. VFIO passthrough, QEMU might need to lock a
>   * significant amount of memory, so we need to set the limit accordingly 
> */
>  virCommandSetMaxMemLock(cmd, qemuDomainGetMemLockLimitBytes(def));

[...]

> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 174d932ae7..e318639963 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c

[...]

> @@ -5975,6 +5976,36 @@ qemuProcessPrepareHostStorage(virQEMUDriverPtr driver,
>  }
>  
>  
> +static int
> +qemuProcessOpenVhostVsock(virDomainVsockDefPtr vsock)
> +{
> +qemuDomainVsockPrivatePtr priv = 
> (qemuDomainVsockPrivatePtr)vsock->privateData;
> +const char *vsock_path = "/dev/vhost-vsock";
> +int fd;
> +
> +if ((fd = open(vsock_path, O_RDWR)) < 0) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +   "%s", _("unable to open vhost-vsock device"));
> +return -1;
> +}
> +
> +if (vsock->guest_cid) {
> +if (virVsockSetGuestCid(fd, vsock->guest_cid) < 0)
> +goto error;
> +} else {
> +if (virVsockAcquireGuestCid(fd, &vsock->guest_cid) < 0)
> +goto error;

This logic is wrong. You should base the decision on auto_cid, rather
than presence of guest_cid. If automatic cid is requested you should
always honour it.

Specifically this might create problem with migration as the cid that
was used on the source was already taken, but the user is okay with
autogenerating it.

Or if the cid actually can't change, you should make it part of the ABI
stability check.

> +}
> +
> +priv->vhostfd = fd;
> +return 0;
> +


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@r

Re: [libvirt] [PATCH v2 1/2] capabilities: Provide info about host IOMMU support

[Erik Skultety]

On Sun, May 27, 2018 at 06:29:14PM +0200, Filip Alac wrote:
> Fixes:
> https://bugzilla.redhat.com/show_bug.cgi?id=967231
>
> Signed-off-by: Filip Alac 
> ---
>  docs/schemas/capability.rng  | 11 +++
>  src/conf/capabilities.c  |  8 
>  src/conf/capabilities.h  |  5 +
>  src/libvirt_private.syms |  1 +
>  src/qemu/qemu_capabilities.c |  3 +++
>  src/util/virpci.c| 19 +++
>  src/util/virpci.h|  2 ++
>  7 files changed, 49 insertions(+)
>
> diff --git a/docs/schemas/capability.rng b/docs/schemas/capability.rng
> index 66c5de62e5..a604cc54d0 100644
> --- a/docs/schemas/capability.rng
> +++ b/docs/schemas/capability.rng
> @@ -39,6 +39,9 @@
>
>  
>
> +  
> +
> +  
>
>  
>
> @@ -155,6 +158,14 @@
>  
>
>
> +  
> +
> +  
> +
> +  
> +
> +  
> +
>
>  
>
> diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c
> index dd2fc77f91..eb387916f2 100644
> --- a/src/conf/capabilities.c
> +++ b/src/conf/capabilities.c
> @@ -1020,6 +1020,8 @@ virCapabilitiesFormatXML(virCapsPtr caps)
>  }
>  virBufferAdjustIndent(&buf, -2);
>  virBufferAddLit(&buf, "\n");
> +virBufferAsprintf(&buf, "\n",
> +  caps->host.iommu  ? "yes" : "no");

I don't think IOMMU depends on the power_management feature, IOW it should be
formatted outside this block.

>  } else {
>  /* The host does not support any PM feature. */
>  virBufferAddLit(&buf, "\n");
> @@ -1743,3 +1745,9 @@ virCapabilitiesInitCaches(virCapsPtr caps)
>  virBitmapFree(cpus);
>  return ret;
>  }
> +
> +int
> +virCapabilitiesHostInitIOMMU(virCapsPtr caps)
> +{
> +return caps->host.iommu = virPCIHasIOMMU();
> +}
> diff --git a/src/conf/capabilities.h b/src/conf/capabilities.h
> index f0a06a24df..4d41363a30 100644
> --- a/src/conf/capabilities.h
> +++ b/src/conf/capabilities.h
> @@ -183,6 +183,7 @@ struct _virCapsHost {
>  int nPagesSize; /* size of pagesSize array */
>  unsigned int *pagesSize;/* page sizes support on the system */
>  unsigned char host_uuid[VIR_UUID_BUFLEN];
> +int iommu;
   ^^^
bool will do just fine.

>  };
>
>  typedef int (*virDomainDefNamespaceParse)(xmlDocPtr, xmlNodePtr,
> @@ -327,4 +328,8 @@ void virCapsHostCacheBankFree(virCapsHostCacheBankPtr 
> ptr);
>
>  int virCapabilitiesInitCaches(virCapsPtr caps);
>
> +int virCapabilitiesInitCaches(virCapsPtr caps);
> +
^^
Some copy paste leftover...

> +int virCapabilitiesHostInitIOMMU(virCapsPtr caps);
> +
>  #endif /* __VIR_CAPABILITIES_H */
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index a97b7fe223..258d02962c 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -58,6 +58,7 @@ virCapabilitiesFreeMachines;
>  virCapabilitiesFreeNUMAInfo;
>  virCapabilitiesGetCpusForNodemask;
>  virCapabilitiesGetNodeInfo;
> +virCapabilitiesHostInitIOMMU;
>  virCapabilitiesHostSecModelAddBaseLabel;
>  virCapabilitiesInitCaches;
>  virCapabilitiesInitNUMA;
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index 40f49a8d9e..d95fa113b6 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -953,6 +953,9 @@ virQEMUCapsInit(virFileCachePtr cache)
>  virCapabilitiesAddHostMigrateTransport(caps, "tcp");
>  virCapabilitiesAddHostMigrateTransport(caps, "rdma");
>
> +/* Add IOMMU info */
> +virCapabilitiesHostInitIOMMU(caps);

Other drivers report capabilities too, so you should have a look at other
server-side drivers too, most notably lxc, libxl, virtuozzo, and test.

> +
>  /* QEMU can support pretty much every arch that exists,
>   * so just probe for them all - we gracefully fail
>   * if a qemu-system-$ARCH binary can't be found
> diff --git a/src/util/virpci.c b/src/util/virpci.c
> index 8d0234..c88b13c97a 100644
> --- a/src/util/virpci.c
> +++ b/src/util/virpci.c
> @@ -3288,3 +3288,22 @@ virPCIEDeviceInfoFree(virPCIEDeviceInfoPtr dev)
>  VIR_FREE(dev->link_sta);
>  VIR_FREE(dev);
>  }

virpci.c is supposed to contain functions to handle and manage PCI devices,
probing IOMMU support on the host is more of a system thing, so let's move this
to virutil.c and name it virHostHasIOMMU

> +
> +bool
> +virPCIHasIOMMU(void)
> +{
> +struct stat sb;
> +
> +/* We can only check on newer kernels with iommu groups & vfio */
> +if (stat("/sys/kernel/iommu_groups", &sb) < 0)
> +return false;
> +
> +if (!S_ISDIR(sb.st_mode))
> +return false;

We already have a helper that combines both of the checks ^above, have a look
at virFileIsDir and use that one instead.

> +
> +/* Check if folder is empty */

Not entirely true per-se, it will only tell you there no sub-directories (which
is what you're after I know)

> +if (sb.st_nlink <= 2)
> +return fa

Re: [libvirt] [PATCH] storage: Remove rwlocks during virStoragePoolObjListForEach

[John Ferlan]

ping?

This resolves a hang

Tks,

John


On 05/24/2018 03:52 PM, John Ferlan wrote:
> Remove the locks since they are unnecessary and would cause
> a hang for a driver reload/restart when a transient pool was
> previously active as a result of the call:
> 
> virStoragePoolUpdateInactive:
> ...
> if (!virStoragePoolObjGetConfigFile(obj)) {
> virStoragePoolObjRemove(driver->pools, obj);
> ...
> 
> stack trace:
> 
> Thread 17 (Thread 0x7fffcc574700 (LWP 12465)):
> ...pthread_rwlock_wrlock
> ...virRWLockWrite
> ...virObjectRWLockWrite
> ...virStoragePoolObjRemove
> ...virStoragePoolUpdateInactive
> ...storagePoolUpdateStateCallback
> ...virStoragePoolObjListForEachCb
> ...virHashForEach
> ...virStoragePoolObjListForEach
> ...storagePoolUpdateAllState
> ...storageStateInitialize
> ...virStateInitialize
> ...daemonRunStateInit
> ...virThreadHelper
> ...start_thread
> ...clone
> 
> Introduced by commit id 4b2e0ed6e3.
> 
> Signed-off-by: John Ferlan 
> ---
>  src/conf/virstorageobj.c | 8 ++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/src/conf/virstorageobj.c b/src/conf/virstorageobj.c
> index 6c937f105c..e66b2ebfb2 100644
> --- a/src/conf/virstorageobj.c
> +++ b/src/conf/virstorageobj.c
> @@ -438,6 +438,12 @@ virStoragePoolObjListForEachCb(void *payload,
>   * not as it's being used for Autostart and UpdateAllState callers
>   * that want to iterate over all the @pools objects not stopping if
>   * one happens to fail.
> + *
> + * NB: We cannot take the Storage Pool lock here because it's possible
> + * that some action as part of Autostart or UpdateAllState will need
> + * to modify/destroy a transient pool. Since these paths only occur
> + * during periods in which the storageDriverLock is held (Initialization,
> + * AutoStart, or Reload) this is OK.
>   */
>  void
>  virStoragePoolObjListForEach(virStoragePoolObjListPtr pools,
> @@ -447,9 +453,7 @@ virStoragePoolObjListForEach(virStoragePoolObjListPtr 
> pools,
>  struct _virStoragePoolObjListForEachData data = { .iter = iter,
>.opaque = opaque };
>  
> -virObjectRWLockRead(pools);
>  virHashForEach(pools->objs, virStoragePoolObjListForEachCb, &data);
> -virObjectRWUnlock(pools);
>  }
>  
>  
> 

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

Re: [libvirt] [PATCH] storage: Add capability to use LUKS encryption for disk backend

[John Ferlan]

ping?

Tks,

John


On 05/24/2018 07:50 PM, John Ferlan wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1560946
> 
> Following the model of the Logical backend, use qemu-img on
> the created device to set up for LUKS encryption.
> 
> Signed-off-by: John Ferlan 
> ---
> 
>  works much better with the settle patch applied from:
> 
>   https://www.redhat.com/archives/libvir-list/2018-May/msg01847.html
> 
> 
>  src/storage/storage_backend_disk.c | 43 
> --
>  1 file changed, 27 insertions(+), 16 deletions(-)
> 
> diff --git a/src/storage/storage_backend_disk.c 
> b/src/storage/storage_backend_disk.c
> index 7b4549c34d..a3003fd0b5 100644
> --- a/src/storage/storage_backend_disk.c
> +++ b/src/storage/storage_backend_disk.c
> @@ -870,19 +870,13 @@ virStorageBackendDiskCreateVol(virStoragePoolObjPtr 
> pool,
>  char *partFormat = NULL;
>  unsigned long long startOffset = 0, endOffset = 0;
>  virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool);
> +virErrorPtr save_err;
>  virCommandPtr cmd = virCommandNewArgList(PARTED,
>   def->source.devices[0].path,
>   "mkpart",
>   "--script",
>   NULL);
>  
> -if (vol->target.encryption != NULL) {
> -virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> -   "%s", _("storage pool does not support encrypted "
> -   "volumes"));
> -goto cleanup;
> -}
> -
>  if (virStorageBackendDiskPartFormat(pool, vol, &partFormat) != 0)
>  goto cleanup;
>  virCommandAddArg(cmd, partFormat);
> @@ -893,6 +887,12 @@ virStorageBackendDiskCreateVol(virStoragePoolObjPtr pool,
>  goto cleanup;
>  }
>  
> +/* If we're going to encrypt using LUKS, then we could need up to
> + * an extra 2MB for the LUKS header - so account for that now */
> +if (vol->target.encryption &&
> +vol->target.encryption->format == VIR_STORAGE_ENCRYPTION_FORMAT_LUKS)
> +endOffset += 2 * 1024 * 1024;
> +
>  virCommandAddArgFormat(cmd, "%lluB", startOffset);
>  virCommandAddArgFormat(cmd, "%lluB", endOffset);
>  
> @@ -910,15 +910,15 @@ virStorageBackendDiskCreateVol(virStoragePoolObjPtr 
> pool,
>  VIR_FREE(vol->target.path);
>  
>  /* Fetch actual extent info, generate key */
> -if (virStorageBackendDiskReadPartitions(pool, vol) < 0) {
> -/* Best effort to remove the partition. Ignore any errors
> - * since we could be calling this with vol->target.path == NULL
> - */
> -virErrorPtr save_err = virSaveLastError();
> -ignore_value(virStorageBackendDiskDeleteVol(pool, vol, 0));
> -virSetError(save_err);
> -virFreeError(save_err);
> -goto cleanup;
> +if (virStorageBackendDiskReadPartitions(pool, vol) < 0)
> +goto error;
> +
> +if (vol->target.encryption) {
> +/* Adjust the sizes to account for the LUKS header */
> +vol->target.capacity -= 2 * 1024 * 1024;
> +vol->target.allocation -= 2 * 1024 * 1024;
> +if (virStorageBackendCreateVolUsingQemuImg(pool, vol, NULL, 0) < 0)
> +goto error;
>  }
>  
>  res = 0;
> @@ -927,8 +927,19 @@ virStorageBackendDiskCreateVol(virStoragePoolObjPtr pool,
>  VIR_FREE(partFormat);
>  virCommandFree(cmd);
>  return res;
> +
> + error:
> +/* Best effort to remove the partition. Ignore any errors
> + * since we could be calling this with vol->target.path == NULL
> + */
> +save_err = virSaveLastError();
> +ignore_value(virStorageBackendDiskDeleteVol(pool, vol, 0));
> +virSetError(save_err);
> +virFreeError(save_err);
> +goto cleanup;
>  }
>  
> +
>  static int
>  virStorageBackendDiskBuildVolFrom(virStoragePoolObjPtr pool,
>virStorageVolDefPtr vol,
> 

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

Re: [libvirt] [PATCH] storage: Add capability to use LUKS encryption for disk backend

[Peter Krempa]

On Thu, May 24, 2018 at 19:50:09 -0400, John Ferlan wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1560946
> 
> Following the model of the Logical backend, use qemu-img on
> the created device to set up for LUKS encryption.
> 
> Signed-off-by: John Ferlan 
> ---
> 
>  works much better with the settle patch applied from:
> 
>   https://www.redhat.com/archives/libvir-list/2018-May/msg01847.html
> 
> 
>  src/storage/storage_backend_disk.c | 43 
> --
>  1 file changed, 27 insertions(+), 16 deletions(-)
> 
> diff --git a/src/storage/storage_backend_disk.c 
> b/src/storage/storage_backend_disk.c
> index 7b4549c34d..a3003fd0b5 100644
> --- a/src/storage/storage_backend_disk.c
> +++ b/src/storage/storage_backend_disk.c

I must say that I'm not a fan of adding features to the 'disk' backend.
Using the disk backend is borderline insane for managing disk
partitions.

[...]

> @@ -893,6 +887,12 @@ virStorageBackendDiskCreateVol(virStoragePoolObjPtr pool,
>  goto cleanup;
>  }
>  
> +/* If we're going to encrypt using LUKS, then we could need up to
> + * an extra 2MB for the LUKS header - so account for that now */
> +if (vol->target.encryption &&
> +vol->target.encryption->format == VIR_STORAGE_ENCRYPTION_FORMAT_LUKS)
> +endOffset += 2 * 1024 * 1024;


I don't think it's a good idea to change 'endOffset' after calling
virStorageBackendDiskPartBoundaries as the function is looking up space
in the existing partition table. With this if the size is just right and
you increase it afterwards the partition will not fit in the place found
by that function.

> +
>  virCommandAddArgFormat(cmd, "%lluB", startOffset);
>  virCommandAddArgFormat(cmd, "%lluB", endOffset);
>  


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

[libvirt] [RFC PATCH 2/2] conf: Extend cputune/cachetune to support memory bandwidth allocation

[bing . niu]

From: Bing Niu 

Extend current cachetune section to support memory bandwidth allocation.
Add a new cachetune element llc for memory allocation. As the example
below:





id--- on which last level cache memory bandwidth to be set
bandwidth --- the memory bandwidth percent to set.

Signed-off-by: Bing Niu 
---
 src/conf/domain_conf.c | 86 ++
 src/util/virresctrl.c  |  7 
 2 files changed, 86 insertions(+), 7 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index d6ac47c..aba998d 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -18578,6 +18578,55 @@ virDomainDefParseBootOptions(virDomainDefPtr def,
 return ret;
 }
 
+static int
+virDomainCachetuneDefParseMemoryBandwidth(xmlXPathContextPtr ctxt,
+  xmlNodePtr node,
+  virResctrlAllocPtr alloc)
+{
+xmlNodePtr oldnode = ctxt->node;
+unsigned int id;
+unsigned int bandwidth;
+char *tmp = NULL;
+int ret = -1;
+
+ctxt->node = node;
+
+tmp = virXMLPropString(node, "id");
+if (!tmp) {
+virReportError(VIR_ERR_XML_ERROR, "%s",
+   _("Missing cachetune attribute 'id'"));
+goto cleanup;
+}
+if (virStrToLong_uip(tmp, NULL, 10, &id) < 0) {
+virReportError(VIR_ERR_XML_ERROR,
+   _("Invalid cachetune attribute 'id' value '%s'"),
+   tmp);
+goto cleanup;
+}
+VIR_FREE(tmp);
+
+tmp = virXMLPropString(node, "bandwidth");
+if (!tmp) {
+virReportError(VIR_ERR_XML_ERROR, "%s",
+   _("Missing cachetune attribute 'bandwidth'"));
+goto cleanup;
+}
+if (virStrToLong_uip(tmp, NULL, 10, &bandwidth) < 0) {
+virReportError(VIR_ERR_XML_ERROR,
+   _("Invalid cachetune attribute 'bandwidth' value '%s'"),
+   tmp);
+goto cleanup;
+}
+VIR_FREE(tmp);
+if (virResctrlSetMemoryBandwidth(alloc, id, bandwidth) < 0)
+goto cleanup;
+
+ret = 0;
+ cleanup:
+ctxt->node = oldnode;
+VIR_FREE(tmp);
+return ret;
+}
 
 static int
 virDomainCachetuneDefParseCache(xmlXPathContextPtr ctxt,
@@ -18670,6 +18719,7 @@ virDomainCachetuneDefParse(virDomainDefPtr def,
 ssize_t i = 0;
 int n;
 int ret = -1;
+bool mba_available = false;
 
 ctxt->node = node;
 
@@ -18701,12 +18751,26 @@ virDomainCachetuneDefParse(virDomainDefPtr def,
 goto cleanup;
 }
 
+if ((n = virXPathNodeSet("./llc", ctxt, &nodes)) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("Cannot extract Memory Bandwidth  nodes under "
+ "cachetune. try cache allocation"));
+}
+
+for (i = 0; i < n; i++) {
+if (virDomainCachetuneDefParseMemoryBandwidth(ctxt, nodes[i], alloc) < 
0)
+goto cleanup;
+}
+
+if (n)
+mba_available = true;
+
 if ((n = virXPathNodeSet("./cache", ctxt, &nodes)) < 0) {
 virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("Cannot extract cache nodes under cachetune"));
-goto cleanup;
+if (!mba_available)
+goto cleanup;
 }
-
 for (i = 0; i < n; i++) {
 if (virDomainCachetuneDefParseCache(ctxt, nodes[i], alloc) < 0)
 goto cleanup;
@@ -26394,11 +26458,19 @@ virDomainCachetuneDefFormatHelper(unsigned int level,
 virBufferPtr buf = opaque;
 unsigned long long short_size = virFormatIntPretty(size, &unit);
 
-virBufferAsprintf(buf,
-  "\n",
-  cache, level, virCacheTypeToString(type),
-  short_size, unit);
+/* If type is VIR_CACHE_TYPE_LAST, this means it's a memory
+ * bandwidth allocation formatting request */
+if (type == VIR_CACHE_TYPE_LAST)
+virBufferAsprintf(buf,
+  "\n",
+  cache, size);
+
+else
+virBufferAsprintf(buf,
+  "\n",
+  cache, level, virCacheTypeToString(type),
+  short_size, unit);
 
 return 0;
 }
diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
index ad050c7..c720074 100644
--- a/src/util/virresctrl.c
+++ b/src/util/virresctrl.c
@@ -954,6 +954,13 @@ virResctrlAllocForeachSize(virResctrlAllocPtr resctrl,
 }
 }
 
+if (resctrl->mba) {
+virResctrlAllocMBPtr mba = resctrl->mba;
+for (cache = 0; cache < mba->nsizes; cache++)
+if (mba->bandwidth[cache])
+cb(0, VIR_CACHE_TYPE_LAST, cache, *mba->bandwidth[cache], 
opaque);
+}
+
 return 0;
 }
 
-- 
2.7.4

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

[libvirt] [RFC PATCH 1/2] util: Add memory bandwidth support to resctrl

[bing . niu]

From: Bing Niu 

Add memory bandwidth allocation support basing on existing
virresctrl implementation. Two new structures virResctrlInfoMB
and virResctrlAllocMB are introduced.

virResctrlInfoMB is used to record host system MBA supporting
information, e.g., minimum bandwidth allowed, bandwidth
granularity, number of bandwidth controller (same as number of
last level cache).

virResctrlAllocMB is used for allocating memory bandwidth.
Following virResctrlAllocPerType, it also employs a nested
sparse array to indicate whether allocation is available for
particular llc.

Overall, the memory bandwidth allocation follows the same sequence
as existing virresctrl cache allocation model. It exposes
interfaces for probing host's memory bandwidth capability on
initialization time and memory bandwidth allocation on runtime.

Signed-off-by: Bing Niu 
---
 src/util/virresctrl.c | 415 +++---
 src/util/virresctrl.h |   5 +
 2 files changed, 401 insertions(+), 19 deletions(-)

diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
index fc11635..ad050c7 100644
--- a/src/util/virresctrl.c
+++ b/src/util/virresctrl.c
@@ -86,6 +86,20 @@ struct _virResctrlInfoPerType {
 virResctrlInfoPerCache control;
 };
 
+/* Information about memory bandwidth allocation */
+typedef struct _virResctrlInfoMB virResctrlInfoMB;
+typedef virResctrlInfoMB *virResctrlInfoMBPtr;
+struct _virResctrlInfoMB {
+/* minimum memory bandwidth allowed*/
+unsigned int min_bandwidth;
+/* bandwidth granularity */
+unsigned int bandwidth_gran;
+/* level number of llc*/
+unsigned int llc;
+/* number of last level cache */
+unsigned int max_id;
+};
+
 typedef struct _virResctrlInfoPerLevel virResctrlInfoPerLevel;
 typedef virResctrlInfoPerLevel *virResctrlInfoPerLevelPtr;
 struct _virResctrlInfoPerLevel {
@@ -97,6 +111,8 @@ struct _virResctrlInfo {
 
 virResctrlInfoPerLevelPtr *levels;
 size_t nlevels;
+
+virResctrlInfoMBPtr mb_info;
 };
 
 static virClassPtr virResctrlInfoClass;
@@ -126,6 +142,7 @@ virResctrlInfoDispose(void *obj)
 VIR_FREE(level);
 }
 
+VIR_FREE(resctrl->mb_info);
 VIR_FREE(resctrl->levels);
 }
 
@@ -201,6 +218,18 @@ struct _virResctrlAllocPerType {
 size_t nmasks;
 };
 
+/*
+ * virResctrlAllocMB represents one memory bandwidth allocation. Since it can 
have
+ * multiple last level caches in a NUMA system, it is also represented as a 
nested
+ * sparse arrays as virRestrlAllocPerLevel
+ */
+typedef struct _virResctrlAllocMB virResctrlAllocMB;
+typedef virResctrlAllocMB *virResctrlAllocMBPtr;
+struct _virResctrlAllocMB {
+unsigned int **bandwidth;
+size_t nsizes;
+};
+
 typedef struct _virResctrlAllocPerLevel virResctrlAllocPerLevel;
 typedef virResctrlAllocPerLevel *virResctrlAllocPerLevelPtr;
 struct _virResctrlAllocPerLevel {
@@ -214,7 +243,7 @@ struct _virResctrlAlloc {
 
 virResctrlAllocPerLevelPtr *levels;
 size_t nlevels;
-
+virResctrlAllocMBPtr mba;
 /* The identifier (any unique string for now) */
 char *id;
 /* libvirt-generated path in /sys/fs/resctrl for this particular
@@ -259,6 +288,13 @@ virResctrlAllocDispose(void *obj)
 VIR_FREE(level);
 }
 
+if (resctrl->mba) {
+virResctrlAllocMBPtr mba = resctrl->mba;
+for (i = 0; i < mba->nsizes; i++)
+VIR_FREE(mba->bandwidth[i]);
+}
+
+VIR_FREE(resctrl->mba);
 VIR_FREE(resctrl->id);
 VIR_FREE(resctrl->path);
 VIR_FREE(resctrl->levels);
@@ -364,6 +400,9 @@ virResctrlInfoIsEmpty(virResctrlInfoPtr resctrl)
 if (!resctrl)
 return true;
 
+if (resctrl->mb_info)
+return false;
+
 for (i = 0; i < resctrl->nlevels; i++) {
 virResctrlInfoPerLevelPtr i_level = resctrl->levels[i];
 
@@ -379,11 +418,62 @@ virResctrlInfoIsEmpty(virResctrlInfoPtr resctrl)
 return true;
 }
 
+static int
+virResctrlGetMemoryBandwidthInfo(virResctrlInfoPtr resctrl)
+{
+DIR *dirp = NULL;
+int ret = -1;
+int rv = -1;
+virResctrlInfoMBPtr i_mb = NULL;
 
-#ifdef __linux__
+rv = virDirOpenIfExists(&dirp, SYSFS_RESCTRL_PATH "/info");
+if (rv <= 0) {
+ret = rv;
+goto cleanup;
+}
+/* query memory bandwidth allocation info */
+if (VIR_ALLOC(i_mb) < 0) {
+ret = -1;
+goto cleanup;
+}
+rv = virFileReadValueUint(&i_mb->bandwidth_gran,
+  SYSFS_RESCTRL_PATH "/info/MB/bandwidth_gran");
+if (rv == -2) {
+/* The file doesn't exist, so it's unusable for us,
+ *  properly mba unsupported */
+VIR_WARN("The path '" SYSFS_RESCTRL_PATH "/info/MB/bandwidth_gran'"
+ "does not exist");
+ret = 0;
+goto cleanup;
+} else if (rv < 0) {
+/* File no existing or other failures are fatal, so just quit */
+goto cleanup;
+}
 
-int
-virResctrlGetInfo(virResctrlInfoPtr resctrl)
+rv = virFileReadValueUint(&i_mb->min_bandwi

[libvirt] [RFC PATCH 0/2] Introduce RDT memory bandwidth allocation support

[bing . niu]

From: Bing Niu 

This series is to introduce RDT memory bandwidth allocation support by extending
current virresctrl implementation.

The Memory Bandwidth Allocation (MBA) feature provides indirect and approximate
control over memory bandwidth available per-core. This feature provides a 
method to
control applications which may be over-utilizing bandwidth relative to their 
priority
in environments such as the data-center. The details can be found in Intel's 
SDM 17.19.7.
Kernel supports MBA through resctrl file system same as CAT. Each resctrl group 
have a
MB parameter to control how much memory bandwidth it can utilize in unit of 
percentage.

In this series, MBA is enabled by enhancing existing virresctrl implementation. 
The
policy employed for MBA is similar with CAT: The sum of each MBA group's 
bandwidth
dose not exceed 100%. The enhancement of virresctrl include two parts:

Patch 1: Add two new structure virResctrlInfoMB and virResctrlAllocMB for 
collecting
 host system MBA capability and domain memory bandwidth allocation.

Patch 2: On frontend XML parsing, add new element "llc" in cachetune section for
 MBA allocation.

Bing Niu (2):
  util: Add memory bandwidth support to resctrl
  conf: Extend cputune/cachetune to support memory bandwidth allocation

 src/conf/domain_conf.c |  86 +-
 src/util/virresctrl.c  | 422 ++---
 src/util/virresctrl.h  |   5 +
 3 files changed, 487 insertions(+), 26 deletions(-)

-- 
2.7.4

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

Re: [libvirt] [PATCHv2 3/7] conf: introduce element

[Ján Tomko]

On Tue, May 29, 2018 at 10:26:20AM +0200, Peter Krempa wrote:

On Thu, May 24, 2018 at 12:39:11 +0200, Ján Tomko wrote:

Add a new 'vsock' element for the vsock device.
The 'model' attribute is optional.
A  subelement should be used to specify the guest cid,
or  should be used.

https://bugzilla.redhat.com/show_bug.cgi?id=1291851
---
 docs/formatdomain.html.in |  20 +++
 docs/schemas/domaincommon.rng |  29 
 src/conf/domain_conf.c| 195 +-
 src/conf/domain_conf.h|  17 +++
 src/qemu/qemu_domain.c|   1 +
 src/qemu/qemu_domain_address.c|  11 ++
 src/qemu/qemu_driver.c|   6 +
 src/qemu/qemu_hotplug.c   |   1 +
 tests/qemuxml2argvdata/vhost-vsock-auto.xml   |  35 +
 tests/qemuxml2argvdata/vhost-vsock.xml|  36 +
 tests/qemuxml2xmloutdata/vhost-vsock-auto.xml |  36 +
 tests/qemuxml2xmloutdata/vhost-vsock.xml  |   1 +
 tests/qemuxml2xmltest.c   |   3 +
 13 files changed, 390 insertions(+), 1 deletion(-)
 create mode 100644 tests/qemuxml2argvdata/vhost-vsock-auto.xml
 create mode 100644 tests/qemuxml2argvdata/vhost-vsock.xml
 create mode 100644 tests/qemuxml2xmloutdata/vhost-vsock-auto.xml
 create mode 12 tests/qemuxml2xmloutdata/vhost-vsock.xml





diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 71ac3d079c..3ea5c91773 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -4140,6 +4140,32 @@
 
   

+  
+
+  
+
+  virtio
+
+  
+  
+


So, source is mandatory? It should be noted in the docs ...


+  
+
+  
+
+  
+  
+
+  
+
+  


Also both attributes are optional which is kind of weird if  is
mandatory.


+
+
+  
+
+  
+
+  


[...]



[...]


diff --git a/tests/qemuxml2argvdata/vhost-vsock-auto.xml 
b/tests/qemuxml2argvdata/vhost-vsock-auto.xml
new file mode 100644
index 00..729d58bea4
--- /dev/null
+++ b/tests/qemuxml2argvdata/vhost-vsock-auto.xml
@@ -0,0 +1,35 @@
+
+  test
+  bba65c0e-c049-934f-b6aa-4e2c0582acdf
+  1048576
+  1048576
+  1
+  
+hvm


Our XMLs are living in the past.


+
+


For an ACK please state how you plan to deal with the schema issues I've
pointed out. All other necessary changes were pointed out.


The idea is to transform  ->  in the QEMU
driver.

 and both of its attributes will be optional in the schema
and in case 'auto' is missing, we'll tune it up in PostParse:

   if (vsock->auto_cid == VIR_TRISTATE_BOOL_ABSENT) {
   if (vsock->guest_cid != 0)
   vsock->auto_cid = VIR_TRISTATE_BOOL_NO;
   else
   vsock->auto_cid = VIR_TRISTATE_BOOL_YES;
   }

Jano





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

Re: [libvirt] [PATCH] storage: Add capability to use LUKS encryption for disk backend

[John Ferlan]

On 05/29/2018 07:02 AM, Peter Krempa wrote:
> On Thu, May 24, 2018 at 19:50:09 -0400, John Ferlan wrote:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1560946
>>
>> Following the model of the Logical backend, use qemu-img on
>> the created device to set up for LUKS encryption.
>>
>> Signed-off-by: John Ferlan 
>> ---
>>
>>  works much better with the settle patch applied from:
>>
>>   https://www.redhat.com/archives/libvir-list/2018-May/msg01847.html
>>
>>
>>  src/storage/storage_backend_disk.c | 43 
>> --
>>  1 file changed, 27 insertions(+), 16 deletions(-)
>>
>> diff --git a/src/storage/storage_backend_disk.c 
>> b/src/storage/storage_backend_disk.c
>> index 7b4549c34d..a3003fd0b5 100644
>> --- a/src/storage/storage_backend_disk.c
>> +++ b/src/storage/storage_backend_disk.c
> 
> I must say that I'm not a fan of adding features to the 'disk' backend.
> Using the disk backend is borderline insane for managing disk
> partitions.
> 
> [...]
> 

So in your opinion, does that mean the above bug should just closed? I
don't want to assume anything about anyone's sanity ;-). Even though to
a degree I agree as I found this particularly odd to create/pass a
device/partition to qemu-img. In a way I was surprised it worked, but
then again since the Logical back-end also allows encryption of a single
volume - doing so for disk volumes would appear to be similar, albeit
strange.

Depending on configuration it may not stand the test of time (e.g.
reboot) either especially if your infrastructure changes - you'd have to
recreate the secret for a different /dev/sdXN.


>> @@ -893,6 +887,12 @@ virStorageBackendDiskCreateVol(virStoragePoolObjPtr 
>> pool,
>>  goto cleanup;
>>  }
>>  
>> +/* If we're going to encrypt using LUKS, then we could need up to
>> + * an extra 2MB for the LUKS header - so account for that now */
>> +if (vol->target.encryption &&
>> +vol->target.encryption->format == 
>> VIR_STORAGE_ENCRYPTION_FORMAT_LUKS)
>> +endOffset += 2 * 1024 * 1024;
> 
> 
> I don't think it's a good idea to change 'endOffset' after calling
> virStorageBackendDiskPartBoundaries as the function is looking up space
> in the existing partition table. With this if the size is just right and
> you increase it afterwards the partition will not fit in the place found
> by that function.
> 

Oh right, sigh. Brain cramp. If this were to be done, then need to
adjust vol->target.capacity before virStorageBackendDiskPartBoundaries

John


>> +
>>  virCommandAddArgFormat(cmd, "%lluB", startOffset);
>>  virCommandAddArgFormat(cmd, "%lluB", endOffset);
>>  

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

Re: [libvirt] [PATCHv2 7/7] qemu: add support for vhost-vsock-pci

[Ján Tomko]

On Tue, May 29, 2018 at 11:16:40AM +0200, Peter Krempa wrote:

On Thu, May 24, 2018 at 12:39:15 +0200, Ján Tomko wrote:

Create a new vsock endpoint by opening /dev/vhost-vsock,
set the requested CID via ioctl (or assign a free one if auto='yes'),
pass the file descriptor to QEMU and build the command line.

https://bugzilla.redhat.com/show_bug.cgi?id=1291851
Signed-off-by: Ján Tomko 
---
 src/qemu/qemu_alias.c  | 16 
 src/qemu/qemu_command.c| 45 ++
 src/qemu/qemu_domain.c |  5 +++
 src/qemu/qemu_domain.h |  2 +-
 src/qemu/qemu_process.c| 35 +
 .../vhost-vsock-auto.x86_64-latest.args| 32 +++
 .../vhost-vsock.x86_64-latest.args | 32 +++
 tests/qemuxml2argvtest.c   | 14 +++
 8 files changed, 180 insertions(+), 1 deletion(-)
 create mode 100644 tests/qemuxml2argvdata/vhost-vsock-auto.x86_64-latest.args
 create mode 100644 tests/qemuxml2argvdata/vhost-vsock.x86_64-latest.args


[...]


diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 9da2d609e8..ef0716d683 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -9865,6 +9865,47 @@ qemuBuildSeccompSandboxCommandLine(virCommandPtr cmd,
 }


+static int
+qemuBuildVsockCommandLine(virCommandPtr cmd,
+  virDomainDefPtr def,
+  virDomainVsockDefPtr vsock,
+  const char *fdprefix,


fdprefix is always empty string, so why is it necessary?

[1]



It will be necessary for hotplug, because AFAIK the name of the file
descriptor passed via monitor cannot start with a digit.

It has no place in this patch.


+  virQEMUCapsPtr qemuCaps)
+{
+qemuDomainVsockPrivatePtr priv = 
(qemuDomainVsockPrivatePtr)vsock->privateData;
+const char *device = "vhost-vsock-pci";
+virBuffer buf = VIR_BUFFER_INITIALIZER;
+char *devstr = NULL;
+int ret = -1;
+
+virBufferAsprintf(&buf, "%s", device);
+virBufferAsprintf(&buf, ",id=%s", vsock->info.alias);
+virBufferAsprintf(&buf, ",guest-cid=%u", vsock->guest_cid);
+virBufferAsprintf(&buf, ",vhostfd=%s%u", fdprefix, priv->vhostfd);
+if (qemuBuildDeviceAddressStr(&buf, def, &vsock->info, qemuCaps) < 0)
+goto cleanup;
+#if 0
+if (qemuBuildVirtioOptionsStr(&buf, net->virtio, qemuCaps) < 0)
+goto error;
+#endif


Leftover stuff from previous version?



While ats= is not that useful for every device, I'm afraid
iommu_platform might be and a followup implementing it will be required.


+
+if (virBufferCheckError(&buf) < 0)
+goto cleanup;
+
+devstr = virBufferContentAndReset(&buf);
+
+virCommandPassFD(cmd, priv->vhostfd, VIR_COMMAND_PASS_FD_CLOSE_PARENT);
+priv->vhostfd = -1;
+virCommandAddArgList(cmd, "-device", devstr, NULL);
+
+ret = 0;
+ cleanup:
+virBufferFreeAndReset(&buf);
+VIR_FREE(devstr);
+return ret;
+}
+
+
 /*
  * Constructs a argv suitable for launching qemu with config defined
  * for a given virtual machine.
@@ -10111,6 +10152,10 @@ qemuBuildCommandLine(virQEMUDriverPtr driver,
 goto error;
 }

+if (def->vsock &&
+qemuBuildVsockCommandLine(cmd, def, def->vsock, "", qemuCaps) < 0)


[1]


+goto error;
+
 /* In some situations, eg. VFIO passthrough, QEMU might need to lock a
  * significant amount of memory, so we need to set the limit accordingly */
 virCommandSetMaxMemLock(cmd, qemuDomainGetMemLockLimitBytes(def));


[...]


diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 174d932ae7..e318639963 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c


[...]


@@ -5975,6 +5976,36 @@ qemuProcessPrepareHostStorage(virQEMUDriverPtr driver,
 }


+static int
+qemuProcessOpenVhostVsock(virDomainVsockDefPtr vsock)
+{
+qemuDomainVsockPrivatePtr priv = 
(qemuDomainVsockPrivatePtr)vsock->privateData;
+const char *vsock_path = "/dev/vhost-vsock";
+int fd;
+
+if ((fd = open(vsock_path, O_RDWR)) < 0) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   "%s", _("unable to open vhost-vsock device"));
+return -1;
+}
+
+if (vsock->guest_cid) {
+if (virVsockSetGuestCid(fd, vsock->guest_cid) < 0)
+goto error;
+} else {
+if (virVsockAcquireGuestCid(fd, &vsock->guest_cid) < 0)
+goto error;


This logic is wrong. You should base the decision on auto_cid, rather
than presence of guest_cid. If automatic cid is requested you should
always honour it.

Specifically this might create problem with migration as the cid that
was used on the source was already taken, but the user is okay with
autogenerating it.

Or if the cid actually can't change, you should make it part of the ABI
stability check.


I

Re: [libvirt] [PATCH] storage: Remove rwlocks during virStoragePoolObjListForEach

[Michal Privoznik]

On 05/24/2018 09:52 PM, John Ferlan wrote:
> Remove the locks since they are unnecessary and would cause
> a hang for a driver reload/restart when a transient pool was
> previously active as a result of the call:
> 
> virStoragePoolUpdateInactive:
> ...
> if (!virStoragePoolObjGetConfigFile(obj)) {
> virStoragePoolObjRemove(driver->pools, obj);
> ...
> 
> stack trace:
> 
> Thread 17 (Thread 0x7fffcc574700 (LWP 12465)):
> ...pthread_rwlock_wrlock
> ...virRWLockWrite
> ...virObjectRWLockWrite
> ...virStoragePoolObjRemove
> ...virStoragePoolUpdateInactive
> ...storagePoolUpdateStateCallback
> ...virStoragePoolObjListForEachCb
> ...virHashForEach
> ...virStoragePoolObjListForEach
> ...storagePoolUpdateAllState
> ...storageStateInitialize
> ...virStateInitialize
> ...daemonRunStateInit
> ...virThreadHelper
> ...start_thread
> ...clone
> 
> Introduced by commit id 4b2e0ed6e3.
> 
> Signed-off-by: John Ferlan 
> ---
>  src/conf/virstorageobj.c | 8 ++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/src/conf/virstorageobj.c b/src/conf/virstorageobj.c
> index 6c937f105c..e66b2ebfb2 100644
> --- a/src/conf/virstorageobj.c
> +++ b/src/conf/virstorageobj.c
> @@ -438,6 +438,12 @@ virStoragePoolObjListForEachCb(void *payload,
>   * not as it's being used for Autostart and UpdateAllState callers
>   * that want to iterate over all the @pools objects not stopping if
>   * one happens to fail.
> + *
> + * NB: We cannot take the Storage Pool lock here because it's possible
> + * that some action as part of Autostart or UpdateAllState will need
> + * to modify/destroy a transient pool. Since these paths only occur
> + * during periods in which the storageDriverLock is held (Initialization,
> + * AutoStart, or Reload) this is OK.
>   */
>  void
>  virStoragePoolObjListForEach(virStoragePoolObjListPtr pools,
> @@ -447,9 +453,7 @@ virStoragePoolObjListForEach(virStoragePoolObjListPtr 
> pools,
>  struct _virStoragePoolObjListForEachData data = { .iter = iter,
>.opaque = opaque };
>  
> -virObjectRWLockRead(pools);
>  virHashForEach(pools->objs, virStoragePoolObjListForEachCb, &data);
> -virObjectRWUnlock(pools);
>  }
>  
>  
> 

ACK

Michal

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

Re: [libvirt] [PATCHv2 3/7] conf: introduce element

[Peter Krempa]

On Tue, May 29, 2018 at 13:36:51 +0200, Ján Tomko wrote:
> On Tue, May 29, 2018 at 10:26:20AM +0200, Peter Krempa wrote:
> > On Thu, May 24, 2018 at 12:39:11 +0200, Ján Tomko wrote:
> > > Add a new 'vsock' element for the vsock device.
> > > The 'model' attribute is optional.
> > > A  subelement should be used to specify the guest cid,
> > > or  should be used.
> > > 
> > > https://bugzilla.redhat.com/show_bug.cgi?id=1291851
> > > ---
> > >  docs/formatdomain.html.in |  20 +++
> > >  docs/schemas/domaincommon.rng |  29 
> > >  src/conf/domain_conf.c| 195 
> > > +-
> > >  src/conf/domain_conf.h|  17 +++
> > >  src/qemu/qemu_domain.c|   1 +
> > >  src/qemu/qemu_domain_address.c|  11 ++
> > >  src/qemu/qemu_driver.c|   6 +
> > >  src/qemu/qemu_hotplug.c   |   1 +
> > >  tests/qemuxml2argvdata/vhost-vsock-auto.xml   |  35 +
> > >  tests/qemuxml2argvdata/vhost-vsock.xml|  36 +
> > >  tests/qemuxml2xmloutdata/vhost-vsock-auto.xml |  36 +
> > >  tests/qemuxml2xmloutdata/vhost-vsock.xml  |   1 +
> > >  tests/qemuxml2xmltest.c   |   3 +
> > >  13 files changed, 390 insertions(+), 1 deletion(-)
> > >  create mode 100644 tests/qemuxml2argvdata/vhost-vsock-auto.xml
> > >  create mode 100644 tests/qemuxml2argvdata/vhost-vsock.xml
> > >  create mode 100644 tests/qemuxml2xmloutdata/vhost-vsock-auto.xml
> > >  create mode 12 tests/qemuxml2xmloutdata/vhost-vsock.xml
> > 
> 
> > > diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> > > index 71ac3d079c..3ea5c91773 100644
> > > --- a/docs/schemas/domaincommon.rng
> > > +++ b/docs/schemas/domaincommon.rng
> > > @@ -4140,6 +4140,32 @@
> > >  
> > >
> > > 
> > > +  
> > > +
> > > +  
> > > +
> > > +  virtio
> > > +
> > > +  
> > > +  
> > > +
> > 
> > So, source is mandatory? It should be noted in the docs ...
> > 
> > > +  
> > > +
> > > +  
> > > +
> > > +  
> > > +  
> > > +
> > > +  
> > > +
> > > +  
> > 
> > Also both attributes are optional which is kind of weird if  is
> > mandatory.
> > 
> > > +
> > > +
> > > +  
> > > +
> > > +  
> > > +
> > > +  
> > 
> > [...]
> > 
> 
> [...]
> 
> > > diff --git a/tests/qemuxml2argvdata/vhost-vsock-auto.xml 
> > > b/tests/qemuxml2argvdata/vhost-vsock-auto.xml
> > > new file mode 100644
> > > index 00..729d58bea4
> > > --- /dev/null
> > > +++ b/tests/qemuxml2argvdata/vhost-vsock-auto.xml
> > > @@ -0,0 +1,35 @@
> > > +
> > > +  test
> > > +  bba65c0e-c049-934f-b6aa-4e2c0582acdf
> > > +  1048576
> > > +  1048576
> > > +  1
> > > +  
> > > +hvm
> > 
> > Our XMLs are living in the past.
> > 
> > > +
> > > +
> > 
> > For an ACK please state how you plan to deal with the schema issues I've
> > pointed out. All other necessary changes were pointed out.
> 
> The idea is to transform  ->  in the QEMU
> driver.
> 
>  and both of its attributes will be optional in the schema
> and in case 'auto' is missing, we'll tune it up in PostParse:
> 
>if (vsock->auto_cid == VIR_TRISTATE_BOOL_ABSENT) {
>if (vsock->guest_cid != 0)
>vsock->auto_cid = VIR_TRISTATE_BOOL_NO;
>else
>vsock->auto_cid = VIR_TRISTATE_BOOL_YES;
>}

Okay. ACK if you make  optional and add this code.


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

Re: [libvirt] [PATCH] storage: Add capability to use LUKS encryption for disk backend

[Peter Krempa]

On Tue, May 29, 2018 at 07:49:50 -0400, John Ferlan wrote:
> 
> 
> On 05/29/2018 07:02 AM, Peter Krempa wrote:
> > On Thu, May 24, 2018 at 19:50:09 -0400, John Ferlan wrote:
> >> https://bugzilla.redhat.com/show_bug.cgi?id=1560946
> >>
> >> Following the model of the Logical backend, use qemu-img on
> >> the created device to set up for LUKS encryption.
> >>
> >> Signed-off-by: John Ferlan 
> >> ---
> >>
> >>  works much better with the settle patch applied from:
> >>
> >>   https://www.redhat.com/archives/libvir-list/2018-May/msg01847.html
> >>
> >>
> >>  src/storage/storage_backend_disk.c | 43 
> >> --
> >>  1 file changed, 27 insertions(+), 16 deletions(-)
> >>
> >> diff --git a/src/storage/storage_backend_disk.c 
> >> b/src/storage/storage_backend_disk.c
> >> index 7b4549c34d..a3003fd0b5 100644
> >> --- a/src/storage/storage_backend_disk.c
> >> +++ b/src/storage/storage_backend_disk.c
> > 
> > I must say that I'm not a fan of adding features to the 'disk' backend.
> > Using the disk backend is borderline insane for managing disk
> > partitions.
> > 
> > [...]
> > 
> 
> So in your opinion, does that mean the above bug should just closed? I

Well, in my opinion this feature will not be used much ... or perhaps at
all.

Whether it can be closed or not depends on the motivation of the
ones filing it and specifically on the motivation of Red Hat keeping it
on their product tracker.

I just wanted to point that out. Obviously contributions to upstream are
welcome so if a feature is desired and contributors implement it, it's
marginal usefulnes for most cases should not interfere with it's
upstream acceptance as it might be useful for somebody.

> don't want to assume anything about anyone's sanity ;-). Even though to

Well, my comment is specifically targetted on using libvirt to manage
partitions. Using libvirt XML to describe partitioning changes is a very
strange concept and users of it must be very brave.

> a degree I agree as I found this particularly odd to create/pass a
> device/partition to qemu-img. In a way I was surprised it worked, but

Passing raw devices to qemu-img or qemu itself is done regularly. Even
qcow2 formatted logical volumes are used e.g. by oVirt

> then again since the Logical back-end also allows encryption of a single
> volume - doing so for disk volumes would appear to be similar, albeit
> strange.
> 
> Depending on configuration it may not stand the test of time (e.g.
> reboot) either especially if your infrastructure changes - you'd have to
> recreate the secret for a different /dev/sdXN.

Both paragraphs above can be applied to logical volumes too, there
really isn't much difference.

> 
> 
> >> @@ -893,6 +887,12 @@ virStorageBackendDiskCreateVol(virStoragePoolObjPtr 
> >> pool,
> >>  goto cleanup;
> >>  }
> >>  
> >> +/* If we're going to encrypt using LUKS, then we could need up to
> >> + * an extra 2MB for the LUKS header - so account for that now */
> >> +if (vol->target.encryption &&
> >> +vol->target.encryption->format == 
> >> VIR_STORAGE_ENCRYPTION_FORMAT_LUKS)
> >> +endOffset += 2 * 1024 * 1024;
> > 
> > 
> > I don't think it's a good idea to change 'endOffset' after calling
> > virStorageBackendDiskPartBoundaries as the function is looking up space
> > in the existing partition table. With this if the size is just right and
> > you increase it afterwards the partition will not fit in the place found
> > by that function.
> > 
> 
> Oh right, sigh. Brain cramp. If this were to be done, then need to
> adjust vol->target.capacity before virStorageBackendDiskPartBoundaries

Yes. Specifically only the value passed to virStorageBackendDiskPartBoundaries
since you want to keep the capacity desired by the user intact.


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

[libvirt] [PATCH v2] storage: Add capability to use LUKS encryption for disk backend

[John Ferlan]

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

Similar to the the Logical backend, use qemu-img on the created
disk partition device to set up for LUKS encryption. Secret mgmt
for the device can be complicated by a reboot possibly changing
the path to the device if the infrastructure changes.

Signed-off-by: John Ferlan 
---
 Changes in v2: Don't alter the result 'endOffset' of the
 DiskPartBoundaries, rather modify the input capacity value.
 Alteration of the value for the error path won't matter
 since the volume would be deleted anyway. The call to
 virStorageBackendDiskPartBoundaries was also slightly
 modified to check the return value vs -1 rather than != 0
 so that the call is similar to other calls. Separating
 that into it's own patch could have been done, but just
 felt like too much "busy work" to be worth the trouble.

 src/storage/storage_backend_disk.c | 49 ++
 1 file changed, 29 insertions(+), 20 deletions(-)

diff --git a/src/storage/storage_backend_disk.c 
b/src/storage/storage_backend_disk.c
index 2e3d1e04a4..e9578d01d6 100644
--- a/src/storage/storage_backend_disk.c
+++ b/src/storage/storage_backend_disk.c
@@ -879,28 +879,26 @@ virStorageBackendDiskCreateVol(virStoragePoolObjPtr pool,
 char *partFormat = NULL;
 unsigned long long startOffset = 0, endOffset = 0;
 virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool);
+virErrorPtr save_err;
 virCommandPtr cmd = virCommandNewArgList(PARTED,
  def->source.devices[0].path,
  "mkpart",
  "--script",
  NULL);
 
-if (vol->target.encryption != NULL) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-   "%s", _("storage pool does not support encrypted "
-   "volumes"));
-goto cleanup;
-}
-
 if (virStorageBackendDiskPartFormat(pool, vol, &partFormat) != 0)
 goto cleanup;
 virCommandAddArg(cmd, partFormat);
 
-if (virStorageBackendDiskPartBoundaries(pool, &startOffset,
-&endOffset,
-vol->target.capacity) != 0) {
+/* If we're going to encrypt using LUKS, then we could need up to
+ * an extra 2MB for the LUKS header - so account for that now */
+if (vol->target.encryption &&
+vol->target.encryption->format == VIR_STORAGE_ENCRYPTION_FORMAT_LUKS)
+vol->target.capacity += 2 * 1024 * 1024;
+
+if (virStorageBackendDiskPartBoundaries(pool, &startOffset, &endOffset,
+vol->target.capacity) < 0)
 goto cleanup;
-}
 
 virCommandAddArgFormat(cmd, "%lluB", startOffset);
 virCommandAddArgFormat(cmd, "%lluB", endOffset);
@@ -919,15 +917,15 @@ virStorageBackendDiskCreateVol(virStoragePoolObjPtr pool,
 VIR_FREE(vol->target.path);
 
 /* Fetch actual extent info, generate key */
-if (virStorageBackendDiskReadPartitions(pool, vol) < 0) {
-/* Best effort to remove the partition. Ignore any errors
- * since we could be calling this with vol->target.path == NULL
- */
-virErrorPtr save_err = virSaveLastError();
-ignore_value(virStorageBackendDiskDeleteVol(pool, vol, 0));
-virSetError(save_err);
-virFreeError(save_err);
-goto cleanup;
+if (virStorageBackendDiskReadPartitions(pool, vol) < 0)
+goto error;
+
+if (vol->target.encryption) {
+/* Adjust the sizes to account for the LUKS header */
+vol->target.capacity -= 2 * 1024 * 1024;
+vol->target.allocation -= 2 * 1024 * 1024;
+if (virStorageBackendCreateVolUsingQemuImg(pool, vol, NULL, 0) < 0)
+goto error;
 }
 
 res = 0;
@@ -936,8 +934,19 @@ virStorageBackendDiskCreateVol(virStoragePoolObjPtr pool,
 VIR_FREE(partFormat);
 virCommandFree(cmd);
 return res;
+
+ error:
+/* Best effort to remove the partition. Ignore any errors
+ * since we could be calling this with vol->target.path == NULL
+ */
+save_err = virSaveLastError();
+ignore_value(virStorageBackendDiskDeleteVol(pool, vol, 0));
+virSetError(save_err);
+virFreeError(save_err);
+goto cleanup;
 }
 
+
 static int
 virStorageBackendDiskBuildVolFrom(virStoragePoolObjPtr pool,
   virStorageVolDefPtr vol,
-- 
2.14.3

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

[libvirt] [PATCH v3] qemu: add support for vhost-vsock-pci

[Ján Tomko]

Create a new vsock endpoint by opening /dev/vhost-vsock,
set the requested CID via ioctl (or assign a free one if auto='yes'),
pass the file descriptor to QEMU and build the command line.

https://bugzilla.redhat.com/show_bug.cgi?id=1291851
Signed-off-by: Ján Tomko 
---
Rebased version with all the ACKed squash-ins for patches 1-6 available at:
git://repo.or.cz/libvirt/jtomko.git vsock-v3
http://repo.or.cz/libvirt/jtomko.git/shortlog/refs/heads/vsock-v3

 src/qemu/qemu_alias.c  | 16 +
 src/qemu/qemu_command.c| 40 ++
 src/qemu/qemu_domain.c |  5 +++
 src/qemu/qemu_domain.h |  2 +-
 src/qemu/qemu_process.c| 35 +++
 .../vhost-vsock-auto.x86_64-latest.args| 32 +
 .../vhost-vsock.x86_64-latest.args | 32 +
 tests/qemuxml2argvtest.c   | 14 
 8 files changed, 175 insertions(+), 1 deletion(-)
 create mode 100644 tests/qemuxml2argvdata/vhost-vsock-auto.x86_64-latest.args
 create mode 100644 tests/qemuxml2argvdata/vhost-vsock.x86_64-latest.args

diff --git a/src/qemu/qemu_alias.c b/src/qemu/qemu_alias.c
index 578a33b284..89dec91ed1 100644
--- a/src/qemu/qemu_alias.c
+++ b/src/qemu/qemu_alias.c
@@ -533,6 +533,18 @@ qemuAssignDeviceInputAlias(virDomainDefPtr def,
 }
 
 
+static int
+qemuAssignDeviceVsockAlias(virDomainVsockDefPtr vsock)
+{
+if (vsock->info.alias)
+return 0;
+if (VIR_STRDUP(vsock->info.alias, "vsock0") < 0)
+return -1;
+
+return 0;
+}
+
+
 int
 qemuAssignDeviceAliases(virDomainDefPtr def, virQEMUCapsPtr qemuCaps)
 {
@@ -629,6 +641,10 @@ qemuAssignDeviceAliases(virDomainDefPtr def, 
virQEMUCapsPtr qemuCaps)
 if (qemuAssignDeviceMemoryAlias(NULL, def->mems[i], false) < 0)
 return -1;
 }
+if (def->vsock) {
+if (qemuAssignDeviceVsockAlias(def->vsock) < 0)
+return -1;
+}
 
 return 0;
 }
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index c7ff074e29..0b5ec4f2ba 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -9912,6 +9912,42 @@ qemuBuildSeccompSandboxCommandLine(virCommandPtr cmd,
 }
 
 
+static int
+qemuBuildVsockCommandLine(virCommandPtr cmd,
+  virDomainDefPtr def,
+  virDomainVsockDefPtr vsock,
+  virQEMUCapsPtr qemuCaps)
+{
+qemuDomainVsockPrivatePtr priv = 
(qemuDomainVsockPrivatePtr)vsock->privateData;
+const char *device = "vhost-vsock-pci";
+virBuffer buf = VIR_BUFFER_INITIALIZER;
+char *devstr = NULL;
+int ret = -1;
+
+virBufferAsprintf(&buf, "%s", device);
+virBufferAsprintf(&buf, ",id=%s", vsock->info.alias);
+virBufferAsprintf(&buf, ",guest-cid=%u", vsock->guest_cid);
+virBufferAsprintf(&buf, ",vhostfd=%u", priv->vhostfd);
+if (qemuBuildDeviceAddressStr(&buf, def, &vsock->info, qemuCaps) < 0)
+goto cleanup;
+
+if (virBufferCheckError(&buf) < 0)
+goto cleanup;
+
+devstr = virBufferContentAndReset(&buf);
+
+virCommandPassFD(cmd, priv->vhostfd, VIR_COMMAND_PASS_FD_CLOSE_PARENT);
+priv->vhostfd = -1;
+virCommandAddArgList(cmd, "-device", devstr, NULL);
+
+ret = 0;
+ cleanup:
+virBufferFreeAndReset(&buf);
+VIR_FREE(devstr);
+return ret;
+}
+
+
 /*
  * Constructs a argv suitable for launching qemu with config defined
  * for a given virtual machine.
@@ -10161,6 +10197,10 @@ qemuBuildCommandLine(virQEMUDriverPtr driver,
 goto error;
 }
 
+if (def->vsock &&
+qemuBuildVsockCommandLine(cmd, def, def->vsock, qemuCaps) < 0)
+goto error;
+
 /* In some situations, eg. VFIO passthrough, QEMU might need to lock a
  * significant amount of memory, so we need to set the limit accordingly */
 virCommandSetMaxMemLock(cmd, qemuDomainGetMemLockLimitBytes(def));
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 10f22b1cc2..2c51e4c0d8 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -1160,6 +1160,8 @@ qemuDomainVsockPrivateNew(void)
 if (!(priv = virObjectNew(qemuDomainVsockPrivateClass)))
 return NULL;
 
+priv->vhostfd = -1;
+
 return (virObjectPtr) priv;
 }
 
@@ -1167,6 +1169,9 @@ qemuDomainVsockPrivateNew(void)
 static void
 qemuDomainVsockPrivateDispose(void *obj ATTRIBUTE_UNUSED)
 {
+qemuDomainVsockPrivatePtr priv = obj;
+
+VIR_FORCE_CLOSE(priv->vhostfd);
 }
 
 
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index f298ebf785..2e0f4df0fb 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -447,7 +447,7 @@ typedef qemuDomainVsockPrivate *qemuDomainVsockPrivatePtr;
 struct _qemuDomainVsockPrivate {
 virObject parent;
 
-virTristateBool maybe;
+int vhostfd;
 };
 
 
diff --git a/src/qemu/qemu_process.c b/src/qemu/qem

Re: [libvirt] [GSoC] Design ideas for implementing cleanup attribute

[Pavel Hrdina]

On Mon, May 28, 2018 at 09:40:41PM +0530, Sukrit Bhatnagar wrote:
> On 28 May 2018 at 13:54, Pavel Hrdina  wrote:
> > On Mon, May 28, 2018 at 01:04:28PM +0530, Sukrit Bhatnagar wrote:
> >> On 25 May 2018 at 16:20, Pavel Hrdina  wrote:
> >> > On Fri, May 25, 2018 at 12:06:50PM +0200, Andrea Bolognani wrote:
> >> >> On Fri, 2018-05-25 at 10:04 +0200, Pavel Hrdina wrote:
> >> >> > On Fri, May 25, 2018 at 09:13:51AM +0200, Andrea Bolognani wrote:
> >> >> > > However, I realize it might not be possible to register free
> >> >> > > functions for a native type without having to introduce something
> >> >> > > like
> >> >> > >
> >> >> > >   typedef char * virString;
> >> >> > >
> >> >> > > thus causing massive churn. How does GLib deal with that?
> >> >> >
> >> >> > If you would look into GLib documentation you would see that this
> >> >> > design basically copies the one in GLib:
> >> >>
> >> >> Sorry, I should have looked up the documentation and implementation
> >> >> before asking silly questions. Guess the morning coffee hadn't quite
> >> >> kicked in yet :/
> >> >>
> >> >> > GLiblibvirt
> >> >> >
> >> >> > g_autofree  VIR_AUTOFREE
> >> >> > g_autoptr   VIR_AUTOPTR
> >> >> > g_auto  VIR_AUTOCLEAR
> >> >>
> >> >> For what it's worth, I think VIR_AUTOCLEAR is a much better name
> >> >> than g_auto :)
> >> >>
> >> >> > In GLib you are using them like this:
> >> >> >
> >> >> > g_autofree char *string = NULL;
> >> >> > g_autoptr(virDomain) dom = NULL;
> >> >> > g_auto(virDomain) dom = { 0 };
> >> >> >
> >> >> > So yes it would require to introduce a lot of typedefs for basic types
> >> >> > and that is not worth it.
> >> >>
> >> >> I'm not sure we would need so many typedefs, but there would
> >> >> certainly be a lot of churn involved.
> >> >>
> >> >> Personally, I'm not so sure it wouldn't be worth the effort,
> >> >> but it's definitely something that we can experiment with it at
> >> >> a later time instead of holding up what's already a pretty
> >> >> significant improvement.
> >> >>
> >> >> > In libvirt we would have:
> >> >> >
> >> >> > VIR_AUTOFREE char *string = NULL;
> >> >> > VIR_AUTOPTR(virDomainPtr) dom = NULL;
> >> >> > VIR_AUTOCLEAR(virDomain) dom = { 0 };
> >> >> >
> >> >> > If you notice the difference, in libvirt we can use virDomainPtr
> >> >> > directly because we have these typedefs, in GLib macro
> >> >> > G_DEFINE_AUTOPTR_CLEANUP_FUNC creates similar typedef.
> >> >>
> >> >> While I'm not a fan of our *Ptr typedefs in general, I guess this
> >> >> time I'm glad we have them because VIR_AUTOPTR() doesn't hide the
> >> >> fact that what you're declaring is a pointer; that is, the macro
> >> >> argument is also exactly the type of the variable.
> >> >
> >> > So let's make a summary of how it could look like:
> >> >
> >> > VIR_AUTOFREE(char *) string = NULL;
> >> > VIR_AUTOPTR(virDomainPtr) vm = NULL;
> >> > VIR_AUTOCLEAR(virDomain) dom = { 0 };
> >> >
> >> > VIR_DEFINE_AUTOFREE_FUNC(virDomainPtr, virDomainFree);
> >> > VIR_DEFINE_AUTOCLEAR_FUNC(virDomain, virDomainClear);
> >>
> >> Do we define new functions for freeing/clearing, because that is what
> >> VIR_DEFINE_AUTOFREE_FUNC seems to do.
> >>
> >>
> >> This is what new macros will look like:
> >>
> >> # define _VIR_TYPE_PTR(type) type##Ptr
> >>
> >> # define _VIR_ATTR_AUTOFREE_PTR(type)  __attribute__((cleanup(type##Free)))
> >> # define _VIR_ATTR_AUTOCLOSE_PTR(type) 
> >> __attribute__((cleanup(type##Close)))
> >> # define _VIR_ATTR_AUTOCLEAN_PTR(type) 
> >> __attribute__((cleanup(type##Clean)))
> >>
> >> # define VIR_AUTOFREE_PTR(type) _VIR_ATTR_AUTOFREE_PTR(type) 
> >> _VIR_TYPE_PTR(type)
> >
> > NACK to this, please look few lines above how the macros should be named
> > and which macros we would like to have implemented.
> >
> > There is no need to have the _VIR* helper macros and you need to
> > implement the VIR_DEFINE_* macros.
> >
> >> The problem is that our vir*Free functions take on vir*Ptr as the
> >> parameter and not
> >> vir*Ptr * (pointer to it).
> >
> > The problem is only with your current implementation, the original
> > design should not have this issue.
> >
> > The part of VIR_DEFINE_* macros is definition of new wrapper function
> > for the cleanup function which means that our existing free functions
> > are not used directly.
> >
> > GLib version of the define macro:
> >
> > #define G_DEFINE_AUTOPTR_CLEANUP_FUNC(TypeName, func) \
> > typedef TypeName *_GLIB_AUTOPTR_TYPENAME(TypeName); \
> > G_GNUC_BEGIN_IGNORE_DEPRECATIONS \
> > static inline void _GLIB_AUTOPTR_FUNC_NAME(TypeName) (TypeName **_ptr) \
> > { \
> > if (*_ptr) \
> > (func) (*_ptr); \
> > } \
> > G_GNUC_END_IGNORE_DEPRECATIONS
> >
> > Obviously we don't need the "typedef" line and the DEPRECATIONS macros.
> 
> 
> So, using the discussed design, I have added the following lines:
> 
> # define VIR_AUTOPTR_FUNC_NAME(type) type##Free
> # define VIR_AUTOCL

Re: [libvirt] [PATCH v3] qemu: add support for vhost-vsock-pci

[Peter Krempa]

On Tue, May 29, 2018 at 14:53:14 +0200, Ján Tomko wrote:
> Create a new vsock endpoint by opening /dev/vhost-vsock,
> set the requested CID via ioctl (or assign a free one if auto='yes'),
> pass the file descriptor to QEMU and build the command line.
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1291851
> Signed-off-by: Ján Tomko 
> ---
> Rebased version with all the ACKed squash-ins for patches 1-6 available at:
> git://repo.or.cz/libvirt/jtomko.git vsock-v3
> http://repo.or.cz/libvirt/jtomko.git/shortlog/refs/heads/vsock-v3

Note that the linked branch is not in pushable state. Patch 3 does not
have a sign-off and the 'squash-in' patch was not actually squashed in.

In patch " util: create virvsock.c" there's a copy-paste error in the
docs for virVsockAcquireGuestCid, where it also mentions what
virVsockSetGuestCid does. Once you'll be editing that commit, please add
the colons after function names too.

ACK series with the two points above addressed.


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

Re: [libvirt] [PATCH v2] storage: Add capability to use LUKS encryption for disk backend

[Peter Krempa]

On Tue, May 29, 2018 at 08:53:06 -0400, John Ferlan wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1560946
> 
> Similar to the the Logical backend, use qemu-img on the created
> disk partition device to set up for LUKS encryption. Secret mgmt
> for the device can be complicated by a reboot possibly changing
> the path to the device if the infrastructure changes.
> 
> Signed-off-by: John Ferlan 
> ---
>  Changes in v2: Don't alter the result 'endOffset' of the
>  DiskPartBoundaries, rather modify the input capacity value.
>  Alteration of the value for the error path won't matter
>  since the volume would be deleted anyway. The call to
>  virStorageBackendDiskPartBoundaries was also slightly
>  modified to check the return value vs -1 rather than != 0
>  so that the call is similar to other calls. Separating
>  that into it's own patch could have been done, but just
>  felt like too much "busy work" to be worth the trouble.
> 
>  src/storage/storage_backend_disk.c | 49 
> ++
>  1 file changed, 29 insertions(+), 20 deletions(-)
> 
> diff --git a/src/storage/storage_backend_disk.c 
> b/src/storage/storage_backend_disk.c
> index 2e3d1e04a4..e9578d01d6 100644
> --- a/src/storage/storage_backend_disk.c
> +++ b/src/storage/storage_backend_disk.c
> @@ -879,28 +879,26 @@ virStorageBackendDiskCreateVol(virStoragePoolObjPtr 
> pool,
>  char *partFormat = NULL;
>  unsigned long long startOffset = 0, endOffset = 0;
>  virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool);
> +virErrorPtr save_err;
>  virCommandPtr cmd = virCommandNewArgList(PARTED,
>   def->source.devices[0].path,
>   "mkpart",
>   "--script",
>   NULL);
>  
> -if (vol->target.encryption != NULL) {
> -virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> -   "%s", _("storage pool does not support encrypted "
> -   "volumes"));
> -goto cleanup;
> -}
> -
>  if (virStorageBackendDiskPartFormat(pool, vol, &partFormat) != 0)
>  goto cleanup;
>  virCommandAddArg(cmd, partFormat);
>  
> -if (virStorageBackendDiskPartBoundaries(pool, &startOffset,
> -&endOffset,
> -vol->target.capacity) != 0) {
> +/* If we're going to encrypt using LUKS, then we could need up to
> + * an extra 2MB for the LUKS header - so account for that now */
> +if (vol->target.encryption &&
> +vol->target.encryption->format == VIR_STORAGE_ENCRYPTION_FORMAT_LUKS)
> +vol->target.capacity += 2 * 1024 * 1024;

So this is done only for LUKS encryption ...

> +
> +if (virStorageBackendDiskPartBoundaries(pool, &startOffset, &endOffset,
> +vol->target.capacity) < 0)
>  goto cleanup;
> -}
>  
>  virCommandAddArgFormat(cmd, "%lluB", startOffset);
>  virCommandAddArgFormat(cmd, "%lluB", endOffset);
> @@ -919,15 +917,15 @@ virStorageBackendDiskCreateVol(virStoragePoolObjPtr 
> pool,
>  VIR_FREE(vol->target.path);
>  
>  /* Fetch actual extent info, generate key */
> -if (virStorageBackendDiskReadPartitions(pool, vol) < 0) {
> -/* Best effort to remove the partition. Ignore any errors
> - * since we could be calling this with vol->target.path == NULL
> - */
> -virErrorPtr save_err = virSaveLastError();
> -ignore_value(virStorageBackendDiskDeleteVol(pool, vol, 0));
> -virSetError(save_err);
> -virFreeError(save_err);
> -goto cleanup;
> +if (virStorageBackendDiskReadPartitions(pool, vol) < 0)
> +goto error;
> +
> +if (vol->target.encryption) {
> +/* Adjust the sizes to account for the LUKS header */
> +vol->target.capacity -= 2 * 1024 * 1024;
> +vol->target.allocation -= 2 * 1024 * 1024;

but this is done for any encryption type, so if 
VIR_STORAGE_ENCRYPTION_FORMAT_QCOW
would be used it would rob off two megabytes of the volume size.

Honestly I think that VIR_STORAGE_ENCRYPTION_FORMAT_QCOW should be
disallowed for creation code right away, so that nobody creates the
broken format any more.

That way we don't have to deal with possibility that somebody with a old
qemu-img would run into problems here without adding much spurious code.

> +if (virStorageBackendCreateVolUsingQemuImg(pool, vol, NULL, 0) < 0)
> +goto error;

ACK if you reject VIR_STORAGE_ENCRYPTION_FORMAT_QCOW right at the
beginning.



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

Re: [libvirt] [libvirt-users] virRandomBits - not very random

[Martin Kletzander]

On Fri, May 25, 2018 at 09:37:44AM -0500, Eric Blake wrote:

On 05/25/2018 09:17 AM, Michal Privoznik wrote:


We should probably seed it with data from /dev/urandom, and/or the new
Linux getrandom() syscall (or BSD equivalent).


I'm not quite sure that right after reboot there's going to be enough
entropy. Every service that's starting wants some random bits. But it's
probably better than what we have now.


Here's where we left things last time it came up:

https://www.redhat.com/archives/libvir-list/2014-December/msg00573.html

If gnutls has an interface that will give us random bits
(gnutls_key_generate() in 3.0, perhaps), we should use THAT for all of
our random bits (and forget about a seed), except when we are mocking
things in our testsuite, and need a deterministic PRNG from a
deterministic seed.

If not (including if we are not linked with gnutls), then we should
prefer the new Linux syscall but fall back to /dev/urandom for JUST
enough bits for a seed; once we're seeded, stick with using our existing
PRNG for all future bits (after all, we aren't trying to generate
cryptographically secure keys using virRandomBits - and the places where
we DO need crypto-strong randomness such as setting up TLS migration is
where we are relying on gnutls to provide it rather than virRandomBits).

So at this point, it's just a matter of someone writing the patches.



Actually, do we need to have a fallback at all?  Can't we just drop all the
gross parts of the code the conditionally compile based on GNUTLS support?  Why
don't we have gnutls required?


--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

--
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 08/10] virrandom: Make virRandomBits better

[Martin Kletzander]

On Tue, May 29, 2018 at 10:24:44AM +0200, Michal Privoznik wrote:

Now that we have strong PRNG generator implemented in
virRandomBytes() let's use that instead of gnulib's random_r.

Problem with the latter is in way we seed it: current UNIX time
and libvirtd's PID are not that random as one might think.
Imagine two hosts booting at the same time. There's a fair chance
that those hosts spawn libvirtds at the same time and with the
same PID. This will result in both daemons generating the same
sequence of say MAC addresses [1].

1: https://www.redhat.com/archives/libvirt-users/2018-May/msg00097.html

Signed-off-by: Michal Privoznik 
---
src/util/virrandom.c | 63 ++--
1 file changed, 2 insertions(+), 61 deletions(-)



ACK to patches 1-7.  But for this one I'm "concerned" about few things.

First of all, just so I don't forget it, random_r can be removed from
bootstrap.conf after this patch, right?

Before this patch, and without gnutls, we wouldn't deplete the entropy of the
kernel, (even though we're just using /dev/urandom and not /dev/random), but now
we'd read everything from /dev/urandom.

Last but not least, if we completely drop the non-gnutls variants of everything,
wouldn't everything be easier anyway?  Like the worrying about entropy pool in
previous point?

And one thing below:


diff --git a/src/util/virrandom.c b/src/util/virrandom.c
index 444b0f9802..01cc82a052 100644
--- a/src/util/virrandom.c
+++ b/src/util/virrandom.c
@@ -108,26 +61,14 @@ VIR_ONCE_GLOBAL_INIT(virRandom)
uint64_t virRandomBits(int nbits)
{
uint64_t ret = 0;
-int32_t bits;

-if (virRandomInitialize() < 0) {
+if (virRandomBytes((unsigned char *) &ret, sizeof(ret)) < 0) {
/* You're already hosed, so this particular non-random value
 * isn't any worse.  */
return 0;


We definitely want to return an error here now IMHO.


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

Re: [libvirt] [libvirt-users] virRandomBits - not very random

[Michal Privoznik]

On 05/29/2018 03:38 PM, Martin Kletzander wrote:
> On Fri, May 25, 2018 at 09:37:44AM -0500, Eric Blake wrote:
>> On 05/25/2018 09:17 AM, Michal Privoznik wrote:
>>
> We should probably seed it with data from /dev/urandom, and/or the new
> Linux getrandom() syscall (or BSD equivalent).
>>>
>>> I'm not quite sure that right after reboot there's going to be enough
>>> entropy. Every service that's starting wants some random bits. But it's
>>> probably better than what we have now.
>>
>> Here's where we left things last time it came up:
>>
>> https://www.redhat.com/archives/libvir-list/2014-December/msg00573.html
>>
>> If gnutls has an interface that will give us random bits
>> (gnutls_key_generate() in 3.0, perhaps), we should use THAT for all of
>> our random bits (and forget about a seed), except when we are mocking
>> things in our testsuite, and need a deterministic PRNG from a
>> deterministic seed.
>>
>> If not (including if we are not linked with gnutls), then we should
>> prefer the new Linux syscall but fall back to /dev/urandom for JUST
>> enough bits for a seed; once we're seeded, stick with using our existing
>> PRNG for all future bits (after all, we aren't trying to generate
>> cryptographically secure keys using virRandomBits - and the places where
>> we DO need crypto-strong randomness such as setting up TLS migration is
>> where we are relying on gnutls to provide it rather than virRandomBits).
>>
>> So at this point, it's just a matter of someone writing the patches.
>>
> 
> Actually, do we need to have a fallback at all?  Can't we just drop all the
> gross parts of the code the conditionally compile based on GNUTLS
> support?  Why
> don't we have gnutls required?

That's exactly what I'm suggesting in my patches [1]. gnutls is widely
available (including Linux, Windows, *BSD, Mac Os X). However, before
doing that we need to fix virRandomBits() to actually call gnutls_rnd().

1: https://www.redhat.com/archives/libvir-list/2018-May/msg02077.html

Michal

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

Re: [libvirt] [libvirt-users] virRandomBits - not very random

[John Ferlan]

On 05/29/2018 09:44 AM, Michal Privoznik wrote:
> On 05/29/2018 03:38 PM, Martin Kletzander wrote:
>> On Fri, May 25, 2018 at 09:37:44AM -0500, Eric Blake wrote:
>>> On 05/25/2018 09:17 AM, Michal Privoznik wrote:
>>>
>> We should probably seed it with data from /dev/urandom, and/or the new
>> Linux getrandom() syscall (or BSD equivalent).

 I'm not quite sure that right after reboot there's going to be enough
 entropy. Every service that's starting wants some random bits. But it's
 probably better than what we have now.
>>>
>>> Here's where we left things last time it came up:
>>>
>>> https://www.redhat.com/archives/libvir-list/2014-December/msg00573.html
>>>
>>> If gnutls has an interface that will give us random bits
>>> (gnutls_key_generate() in 3.0, perhaps), we should use THAT for all of
>>> our random bits (and forget about a seed), except when we are mocking
>>> things in our testsuite, and need a deterministic PRNG from a
>>> deterministic seed.
>>>
>>> If not (including if we are not linked with gnutls), then we should
>>> prefer the new Linux syscall but fall back to /dev/urandom for JUST
>>> enough bits for a seed; once we're seeded, stick with using our existing
>>> PRNG for all future bits (after all, we aren't trying to generate
>>> cryptographically secure keys using virRandomBits - and the places where
>>> we DO need crypto-strong randomness such as setting up TLS migration is
>>> where we are relying on gnutls to provide it rather than virRandomBits).
>>>
>>> So at this point, it's just a matter of someone writing the patches.
>>>
>>
>> Actually, do we need to have a fallback at all?  Can't we just drop all the
>> gross parts of the code the conditionally compile based on GNUTLS
>> support?  Why
>> don't we have gnutls required?
> 
> That's exactly what I'm suggesting in my patches [1]. gnutls is widely
> available (including Linux, Windows, *BSD, Mac Os X). However, before
> doing that we need to fix virRandomBits() to actually call gnutls_rnd().
> 
> 1: https://www.redhat.com/archives/libvir-list/2018-May/msg02077.html
> 

I have this faint recollection of one of the CI platform builds failing
because something in the gnutls* family didn't exist there when I was
making the changes to add the domain master secret code  After a bit
of digging, it seems it was a perhaps a CENTOS6 environment:

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

and since IIUC that's not an issue any more

John

now if I could only figure out why my mail client seems to be dropping
any patches with "crypto" in the subject line (I'm missing patches 2-4
and 10 from the series referenced above)...

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

Re: [libvirt] [PATCH] virtlockd: acquire locks on re-exec

[Matthew Richardson]

> Locks held by virtlockd are dropped on re-exec.
>
> virtlockd   94306 POSIX 5.4G WRITE 0 0   0 /tmp/test.qcow2
> virtlockd   94306 POSIX   5B WRITE 0 0   0 /run/virtlockd.pid
> virtlockd   94306 POSIX   5B WRITE 0 0   0 /run/virtlockd.pid
>
> Acquire locks in PostExecRestart code path.
>
> Signed-off-by: Jim Fehlig 

I've just encountered this same unexpected behaviour (currently on RHEL7
box running libvirt 3.2), and found this thread while searching for a
bug report.

Has this patch been accepted, or the issue fixed in a later release? I
notice there was some confusion in the conversation following as to the
cause of this issue/the fix for it.

If not, is there a bug open against this which I can follow/comment on?

Cheers,

Matthew



signature.asc
Description: OpenPGP digital signature
The University of Edinburgh is a charitable body, registered in
Scotland, with registration number SC005336.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 2/2] docs: news: Add entry about iommu_support

[Michal Privoznik]

On 05/25/2018 12:39 PM, Filip Alac wrote:
> Signed-off-by: Filip Alac 
> ---
>  docs/news.xml | 8 
>  1 file changed, 8 insertions(+)
> 
> diff --git a/docs/news.xml b/docs/news.xml
> index 693d4a373..babf13379 100644
> --- a/docs/news.xml
> +++ b/docs/news.xml
> @@ -93,6 +93,14 @@
>secret-event, pool-event and nodedev-event)
>  
>
> +  
> +
> +  capabilities: Provide info about host IOMMU support
> +
> +
> +  Capabilities XML now provide information about host IOMMU support.
> +
> +  
>  
>  
>  
> 

This looks okay.

Michal

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

Re: [libvirt] [PATCH 1/2] capabilities: Provide info about host IOMMU support

[Michal Privoznik]

On 05/25/2018 12:39 PM, Filip Alac wrote:
> Fixes:
> https://bugzilla.redhat.com/show_bug.cgi?id=967231
> 
> Signed-off-by: Filip Alac 
> ---
>  docs/schemas/capability.rng  | 11 +++
>  src/conf/capabilities.c  |  8 
>  src/conf/capabilities.h  |  5 +
>  src/libvirt_private.syms |  1 +
>  src/qemu/qemu_capabilities.c |  4 
>  src/util/virpci.c| 19 +++
>  src/util/virpci.h|  2 ++
>  7 files changed, 50 insertions(+)
> 
> diff --git a/docs/schemas/capability.rng b/docs/schemas/capability.rng
> index e1ab5c224..7e7d8fbc7 100644
> --- a/docs/schemas/capability.rng
> +++ b/docs/schemas/capability.rng
> @@ -39,6 +39,9 @@
>
>  
>
> +  
> +
> +  
>
>  
>
> @@ -148,6 +151,14 @@
>  
>
>  
> +  
> +
> +  
> +
> +  
> +
> +  

This does not make much sense. This defines an optional attribute
'support' but does not say to which element.

> +
>
>  
>
> diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c
> index dd2fc77f9..eb387916f 100644
> --- a/src/conf/capabilities.c
> +++ b/src/conf/capabilities.c
> @@ -1020,6 +1020,8 @@ virCapabilitiesFormatXML(virCapsPtr caps)
>  }
>  virBufferAdjustIndent(&buf, -2);
>  virBufferAddLit(&buf, "\n");
> +virBufferAsprintf(&buf, "\n",
> +  caps->host.iommu  ? "yes" : "no");

This doesn't make much sense. This block starts with:

if (caps->host.powerMgmt) {

So you only print the  iff power mgmt is available. These are
orthogonal features and have nothing in common.

>  } else {
>  /* The host does not support any PM feature. */
>  virBufferAddLit(&buf, "\n");
> @@ -1743,3 +1745,9 @@ virCapabilitiesInitCaches(virCapsPtr caps)
>  virBitmapFree(cpus);
>  return ret;
>  }
> +
> +int
> +virCapabilitiesHostInitIOMMU(virCapsPtr caps)
> +{
> +return caps->host.iommu = virPCIHasIOMMU();
> +}

Whoa, this is very unsafe. I don't quite see why
virCapabilitiesHostInitIOMMU() should fail when host doesn't have IOMMU.
In fact, it's quite the opposite because false is 0 and true is 1.

> diff --git a/src/conf/capabilities.h b/src/conf/capabilities.h
> index f0a06a24d..4d41363a3 100644
> --- a/src/conf/capabilities.h
> +++ b/src/conf/capabilities.h
> @@ -183,6 +183,7 @@ struct _virCapsHost {
>  int nPagesSize; /* size of pagesSize array */
>  unsigned int *pagesSize;/* page sizes support on the system */
>  unsigned char host_uuid[VIR_UUID_BUFLEN];
> +int iommu;

You declare this int even though you use it only to store a boolean.

>  };
>  
>  typedef int (*virDomainDefNamespaceParse)(xmlDocPtr, xmlNodePtr,
> @@ -327,4 +328,8 @@ void virCapsHostCacheBankFree(virCapsHostCacheBankPtr 
> ptr);
>  
>  int virCapabilitiesInitCaches(virCapsPtr caps);
>  
> +int virCapabilitiesInitCaches(virCapsPtr caps);

No need to copy this line ;-)

> +
> +int virCapabilitiesHostInitIOMMU(virCapsPtr caps);
> +
>  #endif /* __VIR_CAPABILITIES_H */
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index a97b7fe22..258d02962 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -58,6 +58,7 @@ virCapabilitiesFreeMachines;
>  virCapabilitiesFreeNUMAInfo;
>  virCapabilitiesGetCpusForNodemask;
>  virCapabilitiesGetNodeInfo;
> +virCapabilitiesHostInitIOMMU;
>  virCapabilitiesHostSecModelAddBaseLabel;
>  virCapabilitiesInitCaches;
>  virCapabilitiesInitNUMA;

You should expose virPCIHasIOMMU too. After all, you're exposing it in
header file.

> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index 8a63db5f4..552d5452d 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -948,6 +948,10 @@ virQEMUCapsInit(virFileCachePtr cache)
>  if (virCapabilitiesInitPages(caps) < 0)
>  VIR_WARN("Failed to get pages info");
>  
> +/* Add IOMMU info */
> +if (virCapabilitiesHostInitIOMMU(caps) < 0)
> +VIR_WARN("Failed to get iommmu info");
> +

In the XML you placed  between  and
. Might be worth honouring that order here too.

>  /* Add domain migration transport URIs */
>  virCapabilitiesAddHostMigrateTransport(caps, "tcp");
>  virCapabilitiesAddHostMigrateTransport(caps, "rdma");
> diff --git a/src/util/virpci.c b/src/util/virpci.c
> index 8d023..c88b13c97 100644
> --- a/src/util/virpci.c
> +++ b/src/util/virpci.c
> @@ -3288,3 +3288,22 @@ virPCIEDeviceInfoFree(virPCIEDeviceInfoPtr dev)
>  VIR_FREE(dev->link_sta);
>  VIR_FREE(dev);
>  }
> +

See how the rest of functions in the file is separated by two empty
lines? Might be worth preserving that.

> +bool
> +virPCIHasIOMMU(void)
> +{
> +struct stat sb;
> +
> +/* We can only check on newer kernels with iommu groups & vfio */
> +if (stat("/sys/kernel/iommu_groups", &sb) < 0)
> +return false;
> +
> +if (!S_I

Re: [libvirt] [PATCH v6 1/9] qemu: provide support to query the SEV capability

[Brijesh Singh]

On 05/28/2018 02:25 AM, Erik Skultety wrote:

On Wed, May 23, 2018 at 04:18:26PM -0500, Brijesh Singh wrote:

QEMU version >= 2.12 provides support for launching an encrypted VMs on
AMD x86 platform using Secure Encrypted Virtualization (SEV) feature.
This patch adds support to query the SEV capability from the qemu.

Signed-off-by: Brijesh Singh 
---
  src/conf/domain_capabilities.h | 13 
  src/qemu/qemu_capabilities.c   | 47 ++
  src/qemu/qemu_capabilities.h   |  4 ++
  src/qemu/qemu_capspriv.h   |  4 ++
  src/qemu/qemu_monitor.c|  9 +++
  src/qemu/qemu_monitor.h|  3 +
  src/qemu/qemu_monitor_json.c   | 74 ++
  src/qemu/qemu_monitor_json.h   |  3 +
  .../caps_2.12.0.x86_64.replies | 10 +++
  tests/qemucapabilitiesdata/caps_2.12.0.x86_64.xml  |  3 +-
  10 files changed, 169 insertions(+), 1 deletion(-)

diff --git a/src/conf/domain_capabilities.h b/src/conf/domain_capabilities.h
index 9b852e8649bf..c1093234ceb8 100644
--- a/src/conf/domain_capabilities.h
+++ b/src/conf/domain_capabilities.h
@@ -137,6 +137,19 @@ struct _virDomainCapsCPU {
  virDomainCapsCPUModelsPtr custom;
  };

+/*
+ * SEV capabilities
+ */
+typedef struct _virSEVCapability virSEVCapability;
+typedef virSEVCapability *virSEVCapabilityPtr;
+struct _virSEVCapability {
+char *pdh;
+char *cert_chain;
+unsigned int cbitpos;
+unsigned int reduced_phys_bits;
+};
+
+
  struct _virDomainCaps {
  virObjectLockable parent;

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 8a63db5f4f33..49b74f7e12c1 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -489,6 +489,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
"screendump_device",
"hda-output",
"blockdev-del",
+  "sev-guest",
  );


@@ -555,6 +556,8 @@ struct _virQEMUCaps {
  size_t ngicCapabilities;
  virGICCapability *gicCapabilities;

+virSEVCapability *sevCapabilities;
+
  virQEMUCapsHostCPUData kvmCPU;
  virQEMUCapsHostCPUData tcgCPU;
  };
@@ -1121,6 +1124,7 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] = {
  { "virtual-css-bridge", QEMU_CAPS_CCW },
  { "vfio-ccw", QEMU_CAPS_DEVICE_VFIO_CCW },
  { "hda-output", QEMU_CAPS_HDA_OUTPUT },
+{ "sev-guest", QEMU_CAPS_SEV_GUEST },
  };

  static struct virQEMUCapsStringFlags virQEMUCapsDevicePropsVirtioBalloon[] = {
@@ -2050,6 +2054,28 @@ virQEMUCapsSetGICCapabilities(virQEMUCapsPtr qemuCaps,
  }


+void
+virQEMUSevCapabilitiesFree(virSEVCapability *cap)


Since virSEVCapability will be added to virDomainCaps too, you need to move
^this into domain_capabilities.c so it will become virSEVCapabilityFree, I've
got a further comment regarding this in patch 2 as well.

NOTE: notice the SEV in the function name, we should stay consistent in naming
and since SEV is the name of the feature...




Noted, I will make these changes in next rev.



+{
+if (!cap)
+return;
+
+VIR_FREE(cap->pdh);
+VIR_FREE(cap->cert_chain);
+VIR_FREE(cap);
+}
+
+
+void
+virQEMUCapsSetSEVCapabilities(virQEMUCapsPtr qemuCaps,
+  virSEVCapability *capabilities)
+{
+virQEMUSevCapabilitiesFree(qemuCaps->sevCapabilities);


virSEVCapabilityFree(qemuCaps->sevCapabilities)


+
+qemuCaps->sevCapabilities = capabilities;
+}
+
+
  static int
  virQEMUCapsProbeQMPCommands(virQEMUCapsPtr qemuCaps,
  qemuMonitorPtr mon)
@@ -2580,6 +2606,21 @@ virQEMUCapsProbeQMPGICCapabilities(virQEMUCapsPtr 
qemuCaps,
  }


+static int
+virQEMUCapsProbeQMPSEVCapabilities(virQEMUCapsPtr qemuCaps,
+   qemuMonitorPtr mon)
+{
+virSEVCapability *caps = NULL;
+
+if (qemuMonitorGetSEVCapabilities(mon, &caps) < 0)
+return -1;
+
+virQEMUCapsSetSEVCapabilities(qemuCaps, caps);
+
+return 0;
+}
+
+
  bool
  virQEMUCapsCPUFilterFeatures(const char *name,
   void *opaque)
@@ -3965,6 +4006,12 @@ virQEMUCapsInitQMPMonitor(virQEMUCapsPtr qemuCaps,
  virQEMUCapsClear(qemuCaps, QEMU_CAPS_DEVICE_VFIO_CCW);
  }

+/* Probe for SEV capabilities */
+if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_SEV_GUEST)) {
+if (virQEMUCapsProbeQMPSEVCapabilities(qemuCaps, mon) < 0)
+virQEMUCapsClear(qemuCaps, QEMU_CAPS_SEV_GUEST);
+}
+
  ret = 0;
   cleanup:
  return ret;
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index 3e120e64c0b4..8b7eef4359b7 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -473,6 +473,7 @@ typedef enum { /* virQEMUCapsFlags grouping marker for 
syntax-check */
  QEMU_CAPS_SCREENDUMP_DEVICE, /* screendump command accepts devi

Re: [libvirt] [PATCH 00/22] New CPU related APIs

[Jiri Denemark]

Hi Chris,

> The new hypervisor specific compare and baseline commands seem to depend on 
> qemuCaps being pre-populated with model data that is specific to a hypervisor 
> instance.
> 
> How do we make sure the qemuCaps are pre-populated with cpu model data for 
> any 
> arbitrary hypervisor (with a different exec path, arch, etc) that we can 
> issue 
> the hypervisor compare or baseline commands against?

The cache lookup functions automatically generate qemuCaps if they don't
exist (i.e., someone asked for an emulator binary which is not known to
libvirt yet) or if they need to be refreshed (e.g., the QEMU binary
changed in the meantime). It's a bit hidden behind the generic
virFileCache object which uses the following callbacks to handle QEMU
caps cache:

virFileCacheHandlers qemuCapsCacheHandlers = {
.isValid = virQEMUCapsIsValid,
.newData = virQEMUCapsNewData,
.loadFile = virQEMUCapsLoadFile,
.saveFile = virQEMUCapsSaveFile,
.privFree = virQEMUCapsCachePrivFree,
};

Jirka

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

Re: [libvirt] [PATCH 00/22] New CPU related APIs

[Collin Walling]

On 05/28/2018 03:44 PM, Chris Venteicher wrote:
> Quoting Jiri Denemark (2018-05-28 09:19:51)
>> On Wed, May 16, 2018 at 10:39:19 +0200, Jiri Denemark wrote:
>>> The current virConnectCompareCPU and virConnectBaselineCPU APIs are not
>>> very useful because they ignore what a hypervisor can do on the current
>>> host. This series adds two new APIs which are designed to work with
>>> capabilities of a specific hypervisor to provide usable results.
>>>
>>> The third CPU related API virConnectGetCPUModelNames is pretty useless
>>> too, but no new API with similar functionality is needed because domain
>>> capabilities XML already contains the relevant data.
>>
>> I made the suggested changes and pushed the series. Thanks for the
>> reviews.
>>
>> Jirka
> 
> Hi Jirka,
> 
> FYI I reviewed your patches too and everthing I found that was not already 
> identified was nitpick stuff but I do have a something I am wondering about...
> 
> The new hypervisor specific compare and baseline commands seem to depend on 
> qemuCaps being pre-populated with model data that is specific to a hypervisor 
> instance.
> 
> How do we make sure the qemuCaps are pre-populated with cpu model data for 
> any 
> arbitrary hypervisor (with a different exec path, arch, etc) that we can 
> issue 
> the hypervisor compare or baseline commands against?
> 
> Is it a case of executing a virsh command to populate qemuCaps for a 
> non-default 
> hypervisor prior to issuing the hypervisor compare or baseline commands?
> 
> Sorry if a complete noob question or I missed the answer in previous mails.
> 
> Chris
> 

The qemuCaps are generated for each qemu binary upon startup of libvirt and are 
stored 
in /var/cache/libvirt/qemu/capabilities/

If you take a look at virQEMUCapsInitQMPMonitor in file qemu_capabilities, 
you'll see a 
bunch of functions with monitor calls that will probe qemu for its capabilities.

Since there can be a lot of different qemu caps that can exist on one system 
(due to different 
archs, different virttypes, etc), Jiri adds the ability to add parameters to 
specify which 
qemu capabilities file we want to pull data from.


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


-- 
Respectfully,
- Collin Walling

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

Re: [libvirt] [PATCH v2] storage: Add capability to use LUKS encryption for disk backend

[John Ferlan]

On 05/29/2018 09:34 AM, Peter Krempa wrote:
> On Tue, May 29, 2018 at 08:53:06 -0400, John Ferlan wrote:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1560946
>>
>> Similar to the the Logical backend, use qemu-img on the created
>> disk partition device to set up for LUKS encryption. Secret mgmt
>> for the device can be complicated by a reboot possibly changing
>> the path to the device if the infrastructure changes.
>>
>> Signed-off-by: John Ferlan 
>> ---
>>  Changes in v2: Don't alter the result 'endOffset' of the
>>  DiskPartBoundaries, rather modify the input capacity value.
>>  Alteration of the value for the error path won't matter
>>  since the volume would be deleted anyway. The call to
>>  virStorageBackendDiskPartBoundaries was also slightly
>>  modified to check the return value vs -1 rather than != 0
>>  so that the call is similar to other calls. Separating
>>  that into it's own patch could have been done, but just
>>  felt like too much "busy work" to be worth the trouble.
>>
>>  src/storage/storage_backend_disk.c | 49 
>> ++
>>  1 file changed, 29 insertions(+), 20 deletions(-)
>>
>> diff --git a/src/storage/storage_backend_disk.c 
>> b/src/storage/storage_backend_disk.c
>> index 2e3d1e04a4..e9578d01d6 100644
>> --- a/src/storage/storage_backend_disk.c
>> +++ b/src/storage/storage_backend_disk.c
>> @@ -879,28 +879,26 @@ virStorageBackendDiskCreateVol(virStoragePoolObjPtr 
>> pool,
>>  char *partFormat = NULL;
>>  unsigned long long startOffset = 0, endOffset = 0;
>>  virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool);
>> +virErrorPtr save_err;
>>  virCommandPtr cmd = virCommandNewArgList(PARTED,
>>   def->source.devices[0].path,
>>   "mkpart",
>>   "--script",
>>   NULL);
>>  
>> -if (vol->target.encryption != NULL) {
>> -virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>> -   "%s", _("storage pool does not support encrypted "
>> -   "volumes"));
>> -goto cleanup;
>> -}
>> -
>>  if (virStorageBackendDiskPartFormat(pool, vol, &partFormat) != 0)
>>  goto cleanup;
>>  virCommandAddArg(cmd, partFormat);
>>  
>> -if (virStorageBackendDiskPartBoundaries(pool, &startOffset,
>> -&endOffset,
>> -vol->target.capacity) != 0) {
>> +/* If we're going to encrypt using LUKS, then we could need up to
>> + * an extra 2MB for the LUKS header - so account for that now */
>> +if (vol->target.encryption &&
>> +vol->target.encryption->format == 
>> VIR_STORAGE_ENCRYPTION_FORMAT_LUKS)
>> +vol->target.capacity += 2 * 1024 * 1024;
> 
> So this is done only for LUKS encryption ...
> 
>> +
>> +if (virStorageBackendDiskPartBoundaries(pool, &startOffset, &endOffset,
>> +vol->target.capacity) < 0)
>>  goto cleanup;
>> -}
>>  
>>  virCommandAddArgFormat(cmd, "%lluB", startOffset);
>>  virCommandAddArgFormat(cmd, "%lluB", endOffset);
>> @@ -919,15 +917,15 @@ virStorageBackendDiskCreateVol(virStoragePoolObjPtr 
>> pool,
>>  VIR_FREE(vol->target.path);
>>  
>>  /* Fetch actual extent info, generate key */
>> -if (virStorageBackendDiskReadPartitions(pool, vol) < 0) {
>> -/* Best effort to remove the partition. Ignore any errors
>> - * since we could be calling this with vol->target.path == NULL
>> - */
>> -virErrorPtr save_err = virSaveLastError();
>> -ignore_value(virStorageBackendDiskDeleteVol(pool, vol, 0));
>> -virSetError(save_err);
>> -virFreeError(save_err);
>> -goto cleanup;
>> +if (virStorageBackendDiskReadPartitions(pool, vol) < 0)
>> +goto error;
>> +
>> +if (vol->target.encryption) {
>> +/* Adjust the sizes to account for the LUKS header */
>> +vol->target.capacity -= 2 * 1024 * 1024;
>> +vol->target.allocation -= 2 * 1024 * 1024;
> 
> but this is done for any encryption type, so if 
> VIR_STORAGE_ENCRYPTION_FORMAT_QCOW
> would be used it would rob off two megabytes of the volume size.
> 
> Honestly I think that VIR_STORAGE_ENCRYPTION_FORMAT_QCOW should be
> disallowed for creation code right away, so that nobody creates the
> broken format any more.
> 
> That way we don't have to deal with possibility that somebody with a old
> qemu-img would run into problems here without adding much spurious code.
> 
>> +if (virStorageBackendCreateVolUsingQemuImg(pool, vol, NULL, 0) < 0)
>> +goto error;
> 
> ACK if you reject VIR_STORAGE_ENCRYPTION_FORMAT_QCOW right at the
> beginning.
> 

OK... I'll add this to avoid QCOW and/or DEFAULT

if (vol->target.encryption &&
vol->targe

[libvirt] [PATCH] conf: Introduce align for hostmem-file

[Jie Wang]

QEMU has add the 'align' option to 'memory-backend-file'. Expose
this option to users by new element align.

Signed-off-by: Jie Wang 
---
 docs/formatdomain.html.in  | 18 +++
 docs/schemas/domaincommon.rng  |  7 +++
 src/conf/domain_conf.c | 14 +
 src/conf/domain_conf.h |  1 +
 src/qemu/qemu_command.c|  4 ++
 .../memory-hotplug-nvdimm-align.args   | 31 +++
 .../memory-hotplug-nvdimm-align.xml| 63 ++
 tests/qemuxml2argvtest.c   |  3 ++
 .../memory-hotplug-nvdimm-align.xml|  1 +
 tests/qemuxml2xmltest.c|  1 +
 10 files changed, 143 insertions(+)
 create mode 100644 tests/qemuxml2argvdata/memory-hotplug-nvdimm-align.args
 create mode 100644 tests/qemuxml2argvdata/memory-hotplug-nvdimm-align.xml
 create mode 100644 tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-align.xml

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 0d0fd3b..29fe145 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -7932,6 +7932,9 @@ qemu-kvm -net nic,model=? /dev/null
 
   
   
+
+  2048
+
 
   /tmp/nvdimm
 
@@ -7983,6 +7986,21 @@ qemu-kvm -net nic,model=? /dev/null
 
   
 
+  align
+  
+
+  For NVDIMM type devices one can optionally use
+  align and its subelement size
+  to configure the size of alignment within the NVDIMM module.
+  The size element has usual meaning described
+  here.
+  For QEMU domains the following restrictions apply:
+
+
+  the alignment must be multiples of page size 4KiB,
+
+  
+
   source
   
 
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 71ac3d0..9e994b1 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -5145,6 +5145,13 @@
   
 
   
+  
+
+  
+
+  
+
+  
   
 
   
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 3689ac0..bf91167 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -15739,6 +15739,12 @@ virDomainMemoryDefParseXML(virDomainXMLOptionPtr 
xmlopt,
 }
 VIR_FREE(tmp);
 
+if ((node = virXPathNode("./align", ctxt))) {
+if (virDomainParseMemory("./align/size", "./align/size/@unit", ctxt,
+ &def->align, true, false) < 0)
+goto error;
+}
+
 /* source */
 if ((node = virXPathNode("./source", ctxt)) &&
 virDomainMemorySourceDefParseXML(node, ctxt, def) < 0)
@@ -25334,6 +25340,14 @@ virDomainMemoryDefFormat(virBufferPtr buf,
 virBufferAddLit(buf, ">\n");
 virBufferAdjustIndent(buf, 2);
 
+if (def->align) {
+virBufferAddLit(buf, "\n");
+virBufferAdjustIndent(buf, 2);
+virBufferAsprintf(buf, "%llu\n", def->align);
+virBufferAdjustIndent(buf, -2);
+virBufferAddLit(buf, "\n");
+}
+
 if (virDomainMemorySourceDefFormat(buf, def) < 0)
 return -1;
 
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index a78fdee..1155c84 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2107,6 +2107,7 @@ typedef enum {
 struct _virDomainMemoryDef {
 virDomainMemoryAccess access;
 virTristateBool discard;
+unsigned long long align;
 
 /* source */
 virBitmapPtr sourceNodes;
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index c423733..5862457 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -3186,6 +3186,10 @@ qemuBuildMemoryBackendStr(virJSONValuePtr *backendProps,
 if (virJSONValueObjectAdd(props, "U:size", mem->size * 1024, NULL) < 0)
 goto cleanup;
 
+if (mem->align &&
+virJSONValueObjectAdd(props, "u:align", mem->align * 1024, NULL) < 0)
+goto cleanup;
+
 if (mem->sourceNodes) {
 nodemask = mem->sourceNodes;
 } else {
diff --git a/tests/qemuxml2argvdata/memory-hotplug-nvdimm-align.args 
b/tests/qemuxml2argvdata/memory-hotplug-nvdimm-align.args
new file mode 100644
index 000..e6fcf58
--- /dev/null
+++ b/tests/qemuxml2argvdata/memory-hotplug-nvdimm-align.args
@@ -0,0 +1,31 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/home/test \
+USER=test \
+LOGNAME=test \
+QEMU_AUDIO_DRV=none \
+/usr/bin/qemu-system-i686 \
+-name QEMUGuest1 \
+-S \
+-machine pc,accel=tcg,usb=off,dump-guest-core=off,nvdimm=on \
+-m size=219136k,slots=16,maxmem=1099511627776k \
+-smp 2,sockets=2,cores=1,threads=1 \
+-numa node,nodeid=0,cpus=0-1,mem=214 \
+-object memory-backend-file,id=memnvdimm0,prealloc=yes,mem-path=/tmp/nvdimm,\
+sh

Re: [libvirt] [Qemu-devel] [PATCH v7 1/3] qmp: adding 'wakeup-suspend-support' in query-target

[Eduardo Habkost]

On Mon, May 28, 2018 at 09:23:54AM +0200, Markus Armbruster wrote:
> Eduardo Habkost  writes:
[...]
> > [1] Doing a:
> >   $ git grep 'STR.*machine, "'
> > on libvirt source is enough to find some code demonstrating where
> > query-machines is already lacking today:
[...]
> How can we get from this grep to a list of static or dynamic machine
> type capabilties?

Let's look at the code:


$ git grep -W 'STR.*machine, "'
src/libxl/libxl_capabilities.c=libxlMakeDomainOSCaps(const char *machine,
src/libxl/libxl_capabilities.c-  virDomainCapsOSPtr os,
src/libxl/libxl_capabilities.c-  virFirmwarePtr *firmwares,
src/libxl/libxl_capabilities.c-  size_t nfirmwares)
src/libxl/libxl_capabilities.c-{
src/libxl/libxl_capabilities.c-virDomainCapsLoaderPtr capsLoader = 
&os->loader;
src/libxl/libxl_capabilities.c-size_t i;
src/libxl/libxl_capabilities.c-
src/libxl/libxl_capabilities.c-os->supported = true;
src/libxl/libxl_capabilities.c-
src/libxl/libxl_capabilities.c:if (STREQ(machine, "xenpv"))
src/libxl/libxl_capabilities.c-return 0;

I don't understand why this one is here, but we can find out what
we could add to query-machines to make this unnecessary.


[...]
--
src/libxl/libxl_capabilities.c=libxlMakeDomainCapabilities(virDomainCapsPtr 
domCaps,
src/libxl/libxl_capabilities.c-virFirmwarePtr 
*firmwares,
src/libxl/libxl_capabilities.c-size_t nfirmwares)
src/libxl/libxl_capabilities.c-{
src/libxl/libxl_capabilities.c-virDomainCapsOSPtr os = &domCaps->os;
src/libxl/libxl_capabilities.c-virDomainCapsDeviceDiskPtr disk = 
&domCaps->disk;
src/libxl/libxl_capabilities.c-virDomainCapsDeviceGraphicsPtr graphics = 
&domCaps->graphics;
src/libxl/libxl_capabilities.c-virDomainCapsDeviceVideoPtr video = 
&domCaps->video;
src/libxl/libxl_capabilities.c-virDomainCapsDeviceHostdevPtr hostdev = 
&domCaps->hostdev;
src/libxl/libxl_capabilities.c-
src/libxl/libxl_capabilities.c:if (STREQ(domCaps->machine, "xenfv"))
src/libxl/libxl_capabilities.c-domCaps->maxvcpus = HVM_MAX_VCPUS;
src/libxl/libxl_capabilities.c-else
src/libxl/libxl_capabilities.c-domCaps->maxvcpus = PV_MAX_VCPUS;

Looks like libvirt isn't using MachineInfo::cpu-max.  libvirt
bug, or workaround for QEMU limitation?


[...]
--
src/libxl/libxl_driver.c=libxlConnectGetDomainCapabilities(virConnectPtr conn,
src/libxl/libxl_driver.c-  const char 
*emulatorbin,
src/libxl/libxl_driver.c-  const char *arch_str,
src/libxl/libxl_driver.c-  const char *machine,
src/libxl/libxl_driver.c-  const char 
*virttype_str,
src/libxl/libxl_driver.c-  unsigned int flags)
src/libxl/libxl_driver.c-{
[...]
src/libxl/libxl_driver.c-if (machine) {
src/libxl/libxl_driver.c:if (STRNEQ(machine, "xenpv") && 
STRNEQ(machine, "xenfv")) {
src/libxl/libxl_driver.c-virReportError(VIR_ERR_INVALID_ARG, "%s",
src/libxl/libxl_driver.c-   _("Xen only supports 
'xenpv' and 'xenfv' machines"));


Not sure if this should be encoded in QEMU.  accel=xen works with
other PC machines, doesn't it?


[...]
--
src/qemu/qemu_capabilities.c=bool virQEMUCapsHasPCIMultiBus(virQEMUCapsPtr 
qemuCaps,
src/qemu/qemu_capabilities.c-   const virDomainDef 
*def)
src/qemu/qemu_capabilities.c-{
src/qemu/qemu_capabilities.c-/* x86_64 and i686 support PCI-multibus on all 
machine types
src/qemu/qemu_capabilities.c- * since forever */
src/qemu/qemu_capabilities.c-if (ARCH_IS_X86(def->os.arch))
src/qemu/qemu_capabilities.c-return true;
src/qemu/qemu_capabilities.c-
src/qemu/qemu_capabilities.c-if (def->os.arch == VIR_ARCH_PPC ||
src/qemu/qemu_capabilities.c-ARCH_IS_PPC64(def->os.arch)) {
src/qemu/qemu_capabilities.c-/*
src/qemu/qemu_capabilities.c- * Usage of pci.0 naming:
src/qemu/qemu_capabilities.c- *
src/qemu/qemu_capabilities.c- *ref405ep: no pci
src/qemu/qemu_capabilities.c- *   taihu: no pci
src/qemu/qemu_capabilities.c- *  bamboo: 1.1.0
src/qemu/qemu_capabilities.c- *   mac99: 2.0.0
src/qemu/qemu_capabilities.c- * g3beige: 2.0.0
src/qemu/qemu_capabilities.c- *prep: 1.4.0
src/qemu/qemu_capabilities.c- * pseries: 2.0.0
src/qemu/qemu_capabilities.c- *   mpc8544ds: forever
src/qemu/qemu_capabilities.c- * virtex-m507: no pci
src/qemu/qemu_capabilities.c- * ppce500: 1.6.0
src/qemu/qemu_capabilities.c- */
src/qemu/qemu_capabilities.c-
src/qemu/qemu_capabilities.c-/* We do not store the qemu version in 
domain status XML.
src/qemu/qemu_capabilities.c- * Hope the user is using a QEMU new 
enough to use 'pci.0',
src/qemu/qemu_capab

Re: [libvirt] [PATCH 1/2] bhyve: add CPU topology support

[Roman Bogorodskiy]

  Peter Krempa wrote:

> On Mon, May 28, 2018 at 20:27:50 +0400, Roman Bogorodskiy wrote:
> > Recently, bhyve started supporting specifying guest CPU topology.
> > It looks this way:
> > 
> >   bhyve -c cpus=C,sockets=S,cores=C,threads=T ...
> > 
> > The old behaviour with bhyve -c C, where C is a number of vCPUs, is
> > still supported.
> > 
> > So if we have CPU topology in the domain XML, use the new syntax,
> > otherwise keeps the old behaviour.
> > 
> > Signed-off-by: Roman Bogorodskiy 
> > ---
> >  src/bhyve/bhyve_capabilities.c|  7 +++--
> >  src/bhyve/bhyve_capabilities.h|  1 +
> >  src/bhyve/bhyve_command.c | 17 +++-
> >  .../bhyvexml2argv-cputopology.args|  9 +++
> >  .../bhyvexml2argv-cputopology.ldargs  |  3 +++
> >  .../bhyvexml2argv-cputopology.xml | 26 +++
> >  tests/bhyvexml2argvtest.c |  7 -
> >  7 files changed, 66 insertions(+), 4 deletions(-)
> >  create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-cputopology.args
> >  create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-cputopology.ldargs
> >  create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-cputopology.xml
> 
> [...]
> 
> > diff --git a/src/bhyve/bhyve_command.c b/src/bhyve/bhyve_command.c
> > index e3f7ded7db..b319518520 100644
> > --- a/src/bhyve/bhyve_command.c
> > +++ b/src/bhyve/bhyve_command.c
> > @@ -467,7 +467,22 @@ virBhyveProcessBuildBhyveCmd(virConnectPtr conn,
> >  
> >  /* CPUs */
> >  virCommandAddArg(cmd, "-c");
> > -virCommandAddArgFormat(cmd, "%d", virDomainDefGetVcpus(def));
> > +if (def->cpu && def->cpu->sockets) {
> > +if ((bhyveDriverGetCaps(conn) & BHYVE_CAP_CPUTOPOLOGY) != 0) {
> > +virCommandAddArgFormat(cmd, 
> > "cpus=%d,sockets=%d,cores=%d,threads=%d",
> > +   virDomainDefGetVcpus(def),
> > +   def->cpu->sockets,
> > +   def->cpu->cores,
> > +   def->cpu->threads);
> 
> Note that libvirt XML->def conversion does not validate that def->nvcpus
> equals to def->cpu->sockets * def->cpu->cores * def->cpu->threads.
> 
> This is a historic artefact since qemu did not do it either. They
> started doing it just recently. It might be worth adding that check to
> be sure in the future.

Good point!
Indeed, bhyve validates CPU topology in a way you described, so it makes
sense to fail early instead of waiting the command to fail. I'll roll a
v2.

> > +} else {
> > +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> > +   _("Installed bhyve binary does not support "
> > + "defining CPU topology"));
> > +goto error;
> > +}
> 
> The rest looks good to me, so ACK if you don't think the check for the
> topology<->vcpu count is important enough.



Roman Bogorodskiy


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

Re: [libvirt] [PATCH] vfio/pci: Default display option to "off"

[Alex Williamson]

[Cc +Erik,libvirt]

Sorry, should have cc'd libvirt with this initially since display
support is under development.  I think "off" is the better
compatibility option, but perhaps the damage is done since it was the
2.12 default.  Thanks,

Alex

On Tue, 29 May 2018 09:18:10 -0600
Alex Williamson  wrote:

> Commit a9994687cb9b ("vfio/display: core & wireup") added display
> support to vfio-pci with the default being "auto", which breaks
> existing VMs when the vGPU requires GL support but had no previous
> requirement for a GL compatible configuration.  "Off" is the safer
> default as we impose no new requirements to VM configurations.
> 
> Fixes: a9994687cb9b ("vfio/display: core & wireup")
> Cc: qemu-sta...@nongnu.org
> Cc: Gerd Hoffmann 
> Signed-off-by: Alex Williamson 
> ---
>  hw/vfio/pci.c |2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 84e27c7bb2d1..18c493b49ec1 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -3160,7 +3160,7 @@ static Property vfio_pci_dev_properties[] = {
>  DEFINE_PROP_PCI_HOST_DEVADDR("host", VFIOPCIDevice, host),
>  DEFINE_PROP_STRING("sysfsdev", VFIOPCIDevice, vbasedev.sysfsdev),
>  DEFINE_PROP_ON_OFF_AUTO("display", VFIOPCIDevice,
> -display, ON_OFF_AUTO_AUTO),
> +display, ON_OFF_AUTO_OFF),
>  DEFINE_PROP_UINT32("x-intx-mmap-timeout-ms", VFIOPCIDevice,
> intx.mmap_timeout, 1100),
>  DEFINE_PROP_BIT("x-vga", VFIOPCIDevice, features,
> 

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

Re: [libvirt] [PATCH v6 0/9] x86: Secure Encrypted Virtualization (AMD)

[Brijesh Singh]

On 05/28/2018 05:06 AM, Erik Skultety wrote:

On Wed, May 23, 2018 at 04:18:25PM -0500, Brijesh Singh wrote:

This patch series provides support for launching an encrypted guest using
AMD's new Secure Encrypted  Virtualization (SEV) feature.

SEV is an extension to the AMD-V architecture which supports running
multiple VMs under the control of a hypervisor. When enabled, SEV feature
allows the memory contents of a virtual machine (VM) to be transparently
encrypted with a key unique to the guest VM.

At very high level the flow looks this:

1. mgmt tool calls virConnectGetDomainCapabilities. This returns an XML document
that includes the following


...
   
  
  
  
  
o


It's sad that ^this didn't get more attention since Dan suggested a separate
API to get the certificates, because I myself don't feel like having 2k/8k
base64 strings reported in domain capabilities is a good idea, it should only
report that the capability is supported with the given qemu and that the bit
stuff which is hypervisor specific. For the certificates,  which are not QEMU
specific, rather those are platform dependent and static, we should introduce a
virNodeGetSEVInfo (or something like that - I'm not good with names, but I
think we struggle with this in general) getter for that which would work on top
of virTypedParams so that it's extensible. Moreover, it would also be okay IMHO
to feed the return data of this new API not only with the certs but also with
the bit-related stuff, even though that's already obtained through
capabilities.







IIRC, Dan asked other folks to comment their preferences on APIs vs xml. 
I don't remember seeing any strong preferences hence I didn't rework on 
that code. But if majority of folks think that we should do APIs then I 
can certainly rework it in next patch.





If  is provided then we indicate that hypervisor is capable of launching
SEV guest.

2. (optional) mgmt tool can provide the PDH and Cert-chain to guest owner in 
case
if guest owner wish to establish a secure connection with SEV firmware to
negotiate a key used for validating the measurement.

3. mgmt tool requests to start a guest calling virCreateXML(), passing 
VIR_DOMAIN_START_PAUSED.
The xml would include


  /* the value is same as what is obtained via 
virConnectGetDomainCapabilities()
  /* the value is same as what is 
obtained via virConnectGetDomainCapabilities()
..  /* guest owners diffie-hellman key */ (optional)
.. /* guest owners session blob */ (optional)
.. /* guest policy */ (optional)



Now that I'm looking at the design, why do we need to report ,
 in the domain XML if that is platform specific, it's static
and we already report it withing capabilities? If it can't be used for a
per-guest configuration, i.e. it can change, I don't think we should expose the
same information on multiple places.





We had some discussion about this on QEMU mailing list. To make SEV 
launch migration safe, we require user to provide the cbitpos and 
reduced-phys-bit in domain XML as an input to the launch flow. The 
launch flow should not make a platform dependent call to obtain these 
value. If user does not know the cbitpos for its given platform then it 
can obtain it through the domain capabilities.





Still, does anyone have another idea for an improvement?

Erik



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

[libvirt] [PATCH] storage: Add specific check for LUKS encryption support

[John Ferlan]

Modify virStorageBackendLogicalLVCreate to ensure if encryption
is requested that only type LUKS is supported; otherwise, error.

Signed-off-by: John Ferlan 
---
 src/storage/storage_backend_logical.c | 10 --
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/src/storage/storage_backend_logical.c 
b/src/storage/storage_backend_logical.c
index edd4971f1f..67ca7f514d 100644
--- a/src/storage/storage_backend_logical.c
+++ b/src/storage/storage_backend_logical.c
@@ -938,6 +938,13 @@ virStorageBackendLogicalLVCreate(virStorageVolDefPtr vol,
 unsigned long long capacity = vol->target.capacity;
 virCommandPtr cmd = NULL;
 
+if (vol->target.encryption &&
+vol->target.encryption->format != VIR_STORAGE_ENCRYPTION_FORMAT_LUKS) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+   _("storage pool only supports LUKS encrypted volumes"));
+return -1;
+}
+
 cmd = virCommandNewArgList(LVCREATE,
"--name", vol->name,
NULL);
@@ -953,8 +960,7 @@ virStorageBackendLogicalLVCreate(virStorageVolDefPtr vol,
 
 /* If we're going to encrypt using LUKS, then we could need up to
  * an extra 2MB for the LUKS header - so account for that now */
-if (vol->target.encryption &&
-vol->target.encryption->format == VIR_STORAGE_ENCRYPTION_FORMAT_LUKS)
+if (vol->target.encryption)
 capacity += 2 * 1024 * 1024;
 virCommandAddArgFormat(cmd, "%lluK", VIR_DIV_UP(capacity, 1024));
 
-- 
2.14.3

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

Re: [libvirt] [PATCH v6 2/9] qemu: introduce SEV feature in hypervisor capabilities

[Brijesh Singh]

On 05/28/2018 05:28 AM, Erik Skultety wrote:

On Wed, May 23, 2018 at 04:18:27PM -0500, Brijesh Singh wrote:

Extend hypervisor capabilities to include sev feature. When available,
hypervisor supports launching an encrypted VM on AMD platform. The
sev feature tag provides additional details like Platform Diffie-Hellman
(PDH) key and certificate chain which can be used by the guest owner to
establish a cryptographic session with the SEV firmware to negotiate
keys used for attestation or to provide secret during launch.

Signed-off-by: Brijesh Singh 
---


...and this one should be IMHO named
conf: Expose SEV in domain capabilities


  docs/formatdomaincaps.html.in  | 40 
  docs/schemas/domaincaps.rng| 20 
  src/conf/domain_capabilities.c | 20 
  src/conf/domain_capabilities.h |  1 +
  src/qemu/qemu_capabilities.c   |  2 ++
  5 files changed, 83 insertions(+)

diff --git a/docs/formatdomaincaps.html.in b/docs/formatdomaincaps.html.in
index b68ae4b4f1f3..f37b059ba6b1 100644
--- a/docs/formatdomaincaps.html.in
+++ b/docs/formatdomaincaps.html.in
@@ -434,6 +434,12 @@

  
  
+
+  UWxKSlNrVlRTRk5KVGtkSVFVMUU=
+  
VVd4S1NsTnJWbFJUUms1S1ZHdGtTVkZWTVVVPQ==
+  47
+  1
+

  
  
@@ -462,5 +468,39 @@

  Reports whether the vmcoreinfo feature can be enabled

+SEV capabilities
+
+AMD Secure Encrypted Virtualization (SEV) capabilities are exposed under
+the sev element.
+SEV is an extension to the AMD-V architecture which supports running
+virtual machines (VMs) under the control of a hypervisor. When supported,
+guest owner can create a VM whose memory contents will be transparently
+encrypted with a key unique to that VM.
+
+
+For more details on SEV feature see:
+  https://support.amd.com/TechDocs/55766_SEV-KM%20API_Specification.pdf";>
+SEV API spec and http://amd-dev.wpengine.netdna-cdn.com/wordpress/media/2013/12/AMD_Memory_Encryption_Whitepaper_v7-Public.pdf";>
+SEV White Paper
+
+
+
+  pdh
+  A base64 encoded platform Diffie-Hellman public key which can be
+  exported to remote entities that desire to establish a secure transport
+  context with the SEV platform in order to transmit data securely.
+  cert-chain
+   A base64 encoded platform certificate chain that includes the 
platform
+  endorsement key (PEK), owners certificate authority (OCD), and chip
+  endorsement key (CEK).
+  cbitpos
+  When memory encryption is enabled, one of the physical address bits
+  (aka the C-bit) is utilized to mark if a memory page is protected. The
+  C-bit position is Hypervisor dependent.
+  reduced-phys-bits
+  When memory encryption is enabled, we lose certain bits in physical
+  address space. The number of bits we lose is hypervisor dependent.
+
+

  
diff --git a/docs/schemas/domaincaps.rng b/docs/schemas/domaincaps.rng
index 5913d711a3fe..26265645b82a 100644
--- a/docs/schemas/domaincaps.rng
+++ b/docs/schemas/domaincaps.rng
@@ -184,6 +184,9 @@

  
  
+
+


needs 1 more level of indent...


Noted.





+

  

@@ -201,6 +204,23 @@
  


+  
+
+  
+
+  
+  
+
+  
+  
+
+  
+  
+
+  
+
+  
+

  

diff --git a/src/conf/domain_capabilities.c b/src/conf/domain_capabilities.c
index 6e2ab0a28796..3b767c45cbb3 100644
--- a/src/conf/domain_capabilities.c
+++ b/src/conf/domain_capabilities.c
@@ -542,6 +542,25 @@ virDomainCapsFeatureGICFormat(virBufferPtr buf,
  FORMAT_EPILOGUE(gic);
  }

+static void
+virDomainCapsFeatureSEVFormat(virBufferPtr buf,
+  virSEVCapabilityPtr const sev)
+{
+if (!sev)
+return;
+
+virBufferAddLit(buf, "\n");
+virBufferAdjustIndent(buf, 2);
+virBufferAsprintf(buf, "%d\n", sev->cbitpos);
+virBufferAsprintf(buf, "%d\n",
+  sev->reduced_phys_bits);
+virBufferEscapeString(buf, "%s\n", sev->pdh);
+virBufferEscapeString(buf, "%s\n",
+  sev->cert_chain);


As I said, I have to agree with Dan here that reporting the 8k string in
capabilities is a good idea, so we can store it, we just wouldn't format it
^here...




Sure, if decide to go with new APIs then this code need to be reworked.




+virBufferAdjustIndent(buf, -2);
+virBufferAddLit(buf, "\n");
+}
+

  char *
  virDomainCapsFormat(virDomainCapsPtr const caps)
@@ -585,6 +604,7 @@ virDomainCapsFormat(virDomainCapsPtr const caps)
  virDomainCapsFeatureGICFormat(&buf, &caps->gic);
  virBufferAsprintf(&buf, "\n",
ca

Re: [libvirt] [PATCH v6 3/9] conf: introduce launch-security element in domain

[Brijesh Singh]

On 05/28/2018 05:57 AM, Erik Skultety wrote:

On Wed, May 23, 2018 at 04:18:28PM -0500, Brijesh Singh wrote:

The launch-security element can be used to define the security
model to use when launching a domain. Currently we support 'sev'.

When 'sev' is used, the VM will be launched with AMD SEV feature enabled.
SEV feature supports running encrypted VM under the control of KVM.
Encrypted VMs have their pages (code and data) secured such that only the
guest itself has access to the unencrypted version. Each encrypted VM is
associated with a unique encryption key; if its data is accessed to a
different entity using a different key the encrypted guests data will be
incorrectly decrypted, leading to unintelligible data.

Signed-off-by: Brijesh Singh 
---
  docs/formatdomain.html.in  | 115 ++
  docs/schemas/domaincommon.rng  |  39 ++
  src/conf/domain_conf.c | 133 +
  src/conf/domain_conf.h |  27 +
  tests/genericxml2xmlindata/launch-security-sev.xml |  24 
  tests/genericxml2xmltest.c |   2 +
  6 files changed, 340 insertions(+)
  create mode 100644 tests/genericxml2xmlindata/launch-security-sev.xml

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 665d0f25293e..cab08ea52003 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -8310,6 +8310,121 @@ qemu-kvm -net nic,model=? /dev/null

  Note: DEA/TDEA is synonymous with DES/TDES.

+Secure Encrypted Virtualization (SEV)
+
+
+   The contents of the  
element
+   is used to provide the guest owners input used for creating an encrypted
+   VM using the AMD SEV feature.
+
+   SEV is an extension to the AMD-V architecture which supports running
+   encrypted virtual machine (VMs) under the control of KVM. Encrypted
+   VMs have their pages (code and data) secured such that only the guest
+   itself has access to the unencrypted version. Each encrypted VM is
+   associated with a unique encryption key; if its data is accessed to a
+   different entity using a different key the encrypted guests data will
+   be incorrectly decrypted, leading to unintelligible data.
+
+   For more information see various input parameters and its format see 
the SEV API spec
+   https://support.amd.com/TechDocs/55766_SEV-KM%20API_Specification.pdf";> 
https://support.amd.com/TechDocs/55766_SEV-KM%20API_Specification.pdf 
+   Since 4.4.0
+   
+
+
+  ...
+  
+ 0x0001 
+ 47 
+ 1 
+ AAACCCDD=FFFCCCDSDS 
+ RBBBSDDD=FDDCCCDDDG 
+  
+  ...
+
+
+
+
+  cbitpos
+  The required cbitpos element provides the C-bit (aka 
encryption bit)
+  location in guest page table entry. The value of cbitpos is
+  hypervisor dependent and can be obtained through the sev 
element
+  from the domain capabilities.


 From the cover letter it seemed like this is always the same as the value
reported in the domain capabilities and therefore I wrote what I wrote, but is
it always the same value as the one reported in capabilities as it's
hypervisor-dependent or can it in fact be configurable? Because if it can
change, then of course it makes sense to let user config this for a guest and
this is okay.



As I said in cover letter patch response, the value need to be user 
config to make the SEV launch migration safe. Consider the below example


1) user launches a guest on platform A.

On this platform hypervisor reports cbitpos=47 and reduced-phys-bit=1. 
While creating the guest QEMU requires caller to specify the cbitpos and 
reduced-phys-bit parameters. These params gets validate before creating 
the guest. If the user specified values are acceptable then QEMU will 
create the guest otherwise it will fail to create guest.


Typically cbitpos and reduced-phys-bit is platform and hypervisor 
dependent. If user does not know the value then it can query it with 
"virsh domcapabilities" before creating the domain XML file.



2) user tries to migrate the above guest to platform B.

a) If the platform B hypervisor supports the cbitpos and 
reduced-phys-bits value used in #1 then we have no issues.


b) if platform B hypervisor does not support those value then we should 
fail the migration. If we don't make the  and 
 user configurable then we will end up pick the 
different value compare to what was used during the launch flow.







+  
+  reduced-phys-bits
+  The required reduced-phys-bits element provides the 
physical
+  address bit reducation. Similar to cbitpos the value of 

+  reduced-phys-bit is hypervisor dependent and can be obtained
+  through the sev element from the do

[libvirt] [PATCH v2 1/2] bhyve: add CPU topology support

[Roman Bogorodskiy]

Recently, bhyve started supporting specifying guest CPU topology.
It looks this way:

  bhyve -c cpus=C,sockets=S,cores=C,threads=T ...

The old behaviour with bhyve -c C, where C is a number of vCPUs, is
still supported.

So if we have CPU topology in the domain XML, use the new syntax,
otherwise keeps the old behaviour.

Signed-off-by: Roman Bogorodskiy 
---
 src/bhyve/bhyve_capabilities.c|  7 +++--
 src/bhyve/bhyve_capabilities.h|  1 +
 src/bhyve/bhyve_command.c | 26 ++-
 ...yvexml2argv-cputopology-nvcpu-mismatch.xml | 26 +++
 .../bhyvexml2argv-cputopology.args|  9 +++
 .../bhyvexml2argv-cputopology.ldargs  |  3 +++
 .../bhyvexml2argv-cputopology.xml | 26 +++
 tests/bhyvexml2argvtest.c |  8 +-
 8 files changed, 102 insertions(+), 4 deletions(-)
 create mode 100644 
tests/bhyvexml2argvdata/bhyvexml2argv-cputopology-nvcpu-mismatch.xml
 create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-cputopology.args
 create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-cputopology.ldargs
 create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-cputopology.xml

diff --git a/src/bhyve/bhyve_capabilities.c b/src/bhyve/bhyve_capabilities.c
index e13085b1d5..a3229cea75 100644
--- a/src/bhyve/bhyve_capabilities.c
+++ b/src/bhyve/bhyve_capabilities.c
@@ -227,7 +227,7 @@ bhyveProbeCapsDeviceHelper(unsigned int *caps,
 }
 
 static int
-bhyveProbeCapsRTC_UTC(unsigned int *caps, char *binary)
+bhyveProbeCapsFromHelp(unsigned int *caps, char *binary)
 {
 char *help;
 virCommandPtr cmd = NULL;
@@ -244,6 +244,9 @@ bhyveProbeCapsRTC_UTC(unsigned int *caps, char *binary)
 if (strstr(help, "-u:") != NULL)
 *caps |= BHYVE_CAP_RTC_UTC;
 
+if (strstr(help, "sockets=n][,cores=n][,threads=n") != NULL)
+*caps |= BHYVE_CAP_CPUTOPOLOGY;
+
  out:
 VIR_FREE(help);
 virCommandFree(cmd);
@@ -314,7 +317,7 @@ virBhyveProbeCaps(unsigned int *caps)
 if (binary == NULL)
 goto out;
 
-if ((ret = bhyveProbeCapsRTC_UTC(caps, binary)))
+if ((ret = bhyveProbeCapsFromHelp(caps, binary)))
 goto out;
 
 if ((ret = bhyveProbeCapsAHCI32Slot(caps, binary)))
diff --git a/src/bhyve/bhyve_capabilities.h b/src/bhyve/bhyve_capabilities.h
index 0e310e6eda..873bc9c12d 100644
--- a/src/bhyve/bhyve_capabilities.h
+++ b/src/bhyve/bhyve_capabilities.h
@@ -49,6 +49,7 @@ typedef enum {
 BHYVE_CAP_LPC_BOOTROM = 1 << 3,
 BHYVE_CAP_FBUF = 1 << 4,
 BHYVE_CAP_XHCI = 1 << 5,
+BHYVE_CAP_CPUTOPOLOGY = 1 << 6,
 } virBhyveCapsFlags;
 
 int virBhyveProbeGrubCaps(virBhyveGrubCapsFlags *caps);
diff --git a/src/bhyve/bhyve_command.c b/src/bhyve/bhyve_command.c
index e3f7ded7db..802997bd2d 100644
--- a/src/bhyve/bhyve_command.c
+++ b/src/bhyve/bhyve_command.c
@@ -462,12 +462,36 @@ virBhyveProcessBuildBhyveCmd(virConnectPtr conn,
 size_t i;
 bool add_lpc = false;
 int nusbcontrollers = 0;
+unsigned int nvcpus = virDomainDefGetVcpus(def);
 
 virCommandPtr cmd = virCommandNew(BHYVE);
 
 /* CPUs */
 virCommandAddArg(cmd, "-c");
-virCommandAddArgFormat(cmd, "%d", virDomainDefGetVcpus(def));
+if (def->cpu && def->cpu->sockets) {
+if (nvcpus != def->cpu->sockets * def->cpu->cores * def->cpu->threads) 
{
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+   _("Invalid CPU topology: total number of vCPUs "
+ "must equal the product of sockets, cores, "
+ "and threads"));
+goto error;
+}
+
+if ((bhyveDriverGetCaps(conn) & BHYVE_CAP_CPUTOPOLOGY) != 0) {
+virCommandAddArgFormat(cmd, 
"cpus=%d,sockets=%d,cores=%d,threads=%d",
+   nvcpus,
+   def->cpu->sockets,
+   def->cpu->cores,
+   def->cpu->threads);
+} else {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+   _("Installed bhyve binary does not support "
+ "defining CPU topology"));
+goto error;
+}
+} else {
+virCommandAddArgFormat(cmd, "%d", nvcpus);
+}
 
 /* Memory */
 virCommandAddArg(cmd, "-m");
diff --git 
a/tests/bhyvexml2argvdata/bhyvexml2argv-cputopology-nvcpu-mismatch.xml 
b/tests/bhyvexml2argvdata/bhyvexml2argv-cputopology-nvcpu-mismatch.xml
new file mode 100644
index 00..5bd05fb7da
--- /dev/null
+++ b/tests/bhyvexml2argvdata/bhyvexml2argv-cputopology-nvcpu-mismatch.xml
@@ -0,0 +1,26 @@
+
+  bhyve
+  df3be7e7-a104-11e3-aeb0-50e5492bd3dc
+  219136
+  4
+  
+
+  
+  
+hvm
+  
+  
+
+  
+  
+  
+  
+
+
+  
+  
+  
+  
+
+  
+
diff --git a/tests/bhyvexml2argvdata/bhyvexml2argv-cputopology.args 
b/tests/bhyvexml2

[libvirt] [PATCH v2 2/2] docs: bhyve: document guest CPU topology feature

[Roman Bogorodskiy]

Signed-off-by: Roman Bogorodskiy 
---
 docs/drvbhyve.html.in | 16 
 docs/news.xml |  9 +
 2 files changed, 25 insertions(+)

diff --git a/docs/drvbhyve.html.in b/docs/drvbhyve.html.in
index 5b5513d3df..78a291c6bb 100644
--- a/docs/drvbhyve.html.in
+++ b/docs/drvbhyve.html.in
@@ -444,6 +444,22 @@ be wired and cannot be swapped out as follows:
 
 ...
 
+
+
+CPU topology
+
+Since 4.4.0, it's possible to specify guest CPU 
topology, if bhyve
+supports that. Support for specifying guest CPU topology was added to bhyve in
+http://svnweb.freebsd.org/changeset/base/332298";>r332298 for 
-CURRENT.
+Example:
+
+
+...
+
+  
+
+...
+
 
 
   
diff --git a/docs/news.xml b/docs/news.xml
index c45850f625..318bca5de1 100644
--- a/docs/news.xml
+++ b/docs/news.xml
@@ -94,6 +94,15 @@
   both new APIs consider capabilities of a specific hypervisor.
 
   
+  
+
+  bhyve: Support specifying guest CPU topology
+
+
+  Bhyve's guest CPU topology could be specified using the
+   element.
+
+  
 
 
   
-- 
2.17.0

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

[libvirt] [PATCH v2 0/2] bhyve: add CPU topology support

[Roman Bogorodskiy]

Changes since v1:

 - Added a check that nvcpus == sockets * cores * threads and
   a test for that. 

Roman Bogorodskiy (2):
  bhyve: add CPU topology support
  docs: bhyve: document guest CPU topology feature

 docs/drvbhyve.html.in | 16 
 docs/news.xml |  9 +++
 src/bhyve/bhyve_capabilities.c|  7 +++--
 src/bhyve/bhyve_capabilities.h|  1 +
 src/bhyve/bhyve_command.c | 26 ++-
 ...yvexml2argv-cputopology-nvcpu-mismatch.xml | 26 +++
 .../bhyvexml2argv-cputopology.args|  9 +++
 .../bhyvexml2argv-cputopology.ldargs  |  3 +++
 .../bhyvexml2argv-cputopology.xml | 26 +++
 tests/bhyvexml2argvtest.c |  8 +-
 10 files changed, 127 insertions(+), 4 deletions(-)
 create mode 100644 
tests/bhyvexml2argvdata/bhyvexml2argv-cputopology-nvcpu-mismatch.xml
 create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-cputopology.args
 create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-cputopology.ldargs
 create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-cputopology.xml

-- 
2.17.0

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

[libvirt] Entering freeze for libvirt-4.4.0

[Daniel Veillard]

  Done, I tagged RC1 in git and pushed shigned tarball and rpms to the usual
place:

   ftp://libvirt.org/libvirt/

  This seems to work fine with my limited testing, as usual before a release it
would be good to get more testing scope, especially with other OSes and
platforms. 
  ci.centos.org seems to be raising a few issues with bindings, like perl
python and go, so worth a check before the release, RC2 on Thursday and then
a release over the w.e. is the plan if all goes well,

  thanks for giving it a try !

Daniel

-- 
Daniel Veillard  | Red Hat Developers Tools http://developer.redhat.com/
veill...@redhat.com  | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
http://veillard.com/ | virtualization library  http://libvirt.org/

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

Re: [libvirt] [PATCH 00/22] New CPU related APIs

[Chris Venteicher]

Quoting Jiri Denemark (2018-05-29 09:34:02)
> Hi Chris,
> 
> > The new hypervisor specific compare and baseline commands seem to depend on 
> > qemuCaps being pre-populated with model data that is specific to a 
> > hypervisor 
> > instance.
> > 
> > How do we make sure the qemuCaps are pre-populated with cpu model data for 
> > any 
> > arbitrary hypervisor (with a different exec path, arch, etc) that we can 
> > issue 
> > the hypervisor compare or baseline commands against?
> 
> The cache lookup functions automatically generate qemuCaps if they don't
> exist (i.e., someone asked for an emulator binary which is not known to
> libvirt yet) or if they need to be refreshed (e.g., the QEMU binary
> changed in the meantime). It's a bit hidden behind the generic
> virFileCache object which uses the following callbacks to handle QEMU
> caps cache:
> 
> virFileCacheHandlers qemuCapsCacheHandlers = {
> .isValid = virQEMUCapsIsValid,
> .newData = virQEMUCapsNewData,
> .loadFile = virQEMUCapsLoadFile,
> .saveFile = virQEMUCapsSaveFile,
> .privFree = virQEMUCapsCachePrivFree,
> };
> 
> Jirka

Got it.  Read through cache of sorts.  Thanks Jirka and Collin.

Chris.


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

Re: [libvirt] [PATCH 01/10] virRandomBytes: Fix return value

[Eric Blake]

On 05/29/2018 03:24 AM, Michal Privoznik wrote:

In libvirt when a function wants to return an error code it
should be a negative value. Returning a positive value (or zero)
means success. But virRandomBytes() does not follow this rule.

Signed-off-by: Michal Privoznik 
---
  src/util/vircrypto.c  | 4 ++--
  src/util/virrandom.c  | 6 +++---
  src/util/viruuid.c| 4 ++--
  tests/vircryptotest.c | 4 ++--
  4 files changed, 9 insertions(+), 9 deletions(-)




+++ b/src/util/virrandom.c
@@ -168,7 +168,7 @@ uint32_t virRandomInt(uint32_t max)
   * Generate a stream of random bytes from /dev/urandom
   * into @buf of size @buflen
   *
- * Returns 0 on success or an errno on failure
+ * Returns 0 on success or an -errno on failure


"an negative" sounds awkward when pronounced; I'd go with s/an //

With that tweak,
Reviewed-by: Eric Blake 

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

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

Re: [libvirt] [PATCH 02/10] virCryptoGenerateRandom: rename ret

[Eric Blake]

On 05/29/2018 03:24 AM, Michal Privoznik wrote:

This function allocates a buffer, fills it in with random bytes
and then returns it. However, the buffer is held in @buf
variable, therefore having @ret variable which does not hold
return value of the function is misleading.

Signed-off-by: Michal Privoznik 
---
  src/util/vircrypto.c | 10 +-
  1 file changed, 5 insertions(+), 5 deletions(-)


Reviewed-by: Eric Blake 

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

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

Re: [libvirt] [PATCH 00/10] Use better PRNG

[Eric Blake]

On 05/29/2018 03:24 AM, Michal Privoznik wrote:

This is inspired by bug reported here [1]. Even though Eric suggested
calling this Linux syscall when building without gnutls [2] I've decided
to not implement it. Firstly, we build with gnuls everywhere (even
Windows), secondly I see no appealing reason to special case Linux -
/dev/urandom is good for both Linux and FreeBSD.

Once these are merged I'm probably going to send patch set that makes
gnutls mandatory. I'm tired of all those WITH_GNUTLS if-defs (esp. in
function arguments). But that is orthogonal to what I'm solving here.

Also, I'm not quite sure this is a release material, so I'm fine with
merging this after the release.

1: https://www.redhat.com/archives/libvirt-users/2018-May/msg00097.html
2: https://www.redhat.com/archives/libvirt-users/2018-May/msg00100.html


I'm not sure if we're getting a CVE assigned for this (if Red Hat 
security gets back to me on that question, and says a CVE is warranted, 
then maybe it still is a candidate for this release).  But if a CVE is 
assigned, the fact that this issue has been public since 2014 means that 
one more broken release added to years of neglect regarding the issue 
won't hurt much.


--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

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

Re: [libvirt] [PATCH 08/10] virrandom: Make virRandomBits better

[Eric Blake]

On 05/29/2018 03:24 AM, Michal Privoznik wrote:

Now that we have strong PRNG generator implemented in
virRandomBytes() let's use that instead of gnulib's random_r.

Problem with the latter is in way we seed it: current UNIX time
and libvirtd's PID are not that random as one might think.
Imagine two hosts booting at the same time. There's a fair chance
that those hosts spawn libvirtds at the same time and with the
same PID. This will result in both daemons generating the same
sequence of say MAC addresses [1].

1: https://www.redhat.com/archives/libvirt-users/2018-May/msg00097.html

Signed-off-by: Michal Privoznik 
---
  src/util/virrandom.c | 63 ++--
  1 file changed, 2 insertions(+), 61 deletions(-)




-static int
-virRandomOnceInit(void)
-{
-unsigned int seed = time(NULL) ^ getpid();
-
-#if 0
-/* Normally we want a decent seed.  But if reproducible debugging
- * of a fixed pseudo-random sequence is ever required, uncomment
- * this block to let an environment variable force the seed.  */
-const char *debug = virGetEnvBlockSUID("VIR_DEBUG_RANDOM_SEED");
-
-if (debug && virStrToLong_ui(debug, NULL, 0, &seed) < 0)
-return -1;


Are we sure we don't need the ability to quickly compile in a 
deterministic replacement for debug in the future?  I suppose there's 
always git history, but this particular #if 0 was left in place for a 
reason, where completely removing it makes it harder to realize that 
such debugging is even possible.


--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

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

[libvirt] [RFC 1/4] add macros for implementing automatic cleanup functionality

[Sukrit Bhatnagar]

New macros are added to src/util/viralloc.h which help in
adding cleanup attribute to variable declarations.

Signed-off-by: Sukrit Bhatnagar 
---
 src/util/viralloc.h | 69 +
 1 file changed, 69 insertions(+)

diff --git a/src/util/viralloc.h b/src/util/viralloc.h
index 69d0f90..e1c31c3 100644
--- a/src/util/viralloc.h
+++ b/src/util/viralloc.h
@@ -596,4 +596,73 @@ void virAllocTestInit(void);
 int virAllocTestCount(void);
 void virAllocTestOOM(int n, int m);
 void virAllocTestHook(void (*func)(int, void*), void *data);
+
+# define VIR_AUTOPTR_FUNC_NAME(type) virAutoPtr##type
+# define VIR_AUTOCLEAR_FUNC_NAME(type) virAutoClear##type
+
+/**
+ * VIR_DEFINE_AUTOPTR_FUNC:
+ * @type: type of the variables(s) to free automatically
+ * @func: cleanup function to be automatically called
+ *
+ * Thos macro defines a function for automatically freeing
+ * resources allocated to a variable of type @type. The newly
+ * defined function calls the corresponding pre-defined
+ * function @func.
+ */
+# define VIR_DEFINE_AUTOPTR_FUNC(type, func) \
+static inline void VIR_AUTOPTR_FUNC_NAME(type)(type *_ptr) \
+{ \
+(func)(*_ptr); \
+} \
+
+/**
+ * VIR_DEFINE_AUTOCLEAR_FUNC:
+ * @type: type of the variables(s) to free automatically
+ * @func: cleanup function to be automatically called
+ *
+ * This macro defines a function for automatically clearing
+ * a variable of type @type. The newly deifined function calls
+ * the corresponding pre-defined function @func.
+ */
+# define VIR_DEFINE_AUTOCLEAR_FUNC(type, func) \
+static inline void VIR_AUTOCLEAR_FUNC_NAME(type)(type *_ptr) \
+{ \
+(func)(_ptr); \
+} \
+
+/**
+ * VIR_AUTOFREE:
+ * @type: type of the variables(s) to free automatically
+ *
+ * Macro to automatically free the memory allocated to
+ * the variable(s) declared with it by calling virFree
+ * when the variable goes out of scope.
+ */
+# define VIR_AUTOFREE(type) __attribute__((cleanup(virFree))) type
+
+/**
+ * VIR_AUTOPTR:
+ * @type: type of the variables(s) to free automatically
+ *
+ * Macro to automatically free the memory allocated to
+ * the variable(s) declared with it by calling the function
+ * defined by VIR_DEFINE_AUTOPTR_FUNC when the variable
+ * goes out of scope.
+ */
+# define VIR_AUTOPTR(type) \
+__attribute__((cleanup(VIR_AUTOPTR_FUNC_NAME(type type
+
+/**
+ * VIR_AUTOCLEAR:
+ * @type: type of the variables(s) to free automatically
+ *
+ * Macro to automatically clear the variable(s) declared
+ * with it by calling the function defined by
+ * VIR_DEFINE_AUTOCLEAR_FUNC when the variabl* goes out
+ * of scope.
+ */
+# define VIR_AUTOCLEAR(type) \
+__attribute__((cleanup(VIR_AUTOCLEAR_FUNC_NAME(type type
+
 #endif /* __VIR_MEMORY_H_ */
-- 
1.8.3.1

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

[libvirt] [RFC 0/4] add automatic cleanup functionality in some files

[Sukrit Bhatnagar]

This series of patches aim at augmenting our discussion about the
design for implementing the cleanup attribute.

A set of macros have been added at the end of viralloc.h
A few files have been modified to use the newly created macros.

Sukrit Bhatnagar (4):
  add macros for implementing automatic cleanup functionality
  add automatic cleanup support in src/util/virarptable.c
  add automatic cleanup support in src/util/virauthconfig.c
  add automatic cleanup support in src/util/virauth.c

 src/util/viralloc.h  | 69 
 src/util/virarptable.c   |  9 ++-
 src/util/virauth.c   | 66 +
 src/util/virauthconfig.c | 34 +---
 src/util/virauthconfig.h |  3 +++
 5 files changed, 110 insertions(+), 71 deletions(-)

-- 
1.8.3.1

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

[libvirt] [RFC 3/4] add automatic cleanup support in src/util/virauthconfig.c

[Sukrit Bhatnagar]

Modifiy code to use cleanup macros where required.

Signed-off-by: Sukrit Bhatnagar 
---
 src/util/virauthconfig.c | 34 --
 1 file changed, 12 insertions(+), 22 deletions(-)

diff --git a/src/util/virauthconfig.c b/src/util/virauthconfig.c
index 91c9c0c..66f7f7e 100644
--- a/src/util/virauthconfig.c
+++ b/src/util/virauthconfig.c
@@ -106,10 +106,9 @@ int virAuthConfigLookup(virAuthConfigPtr auth,
 const char *credname,
 const char **value)
 {
-char *authgroup = NULL;
-char *credgroup = NULL;
+VIR_AUTOFREE(char *) authgroup = NULL;
+VIR_AUTOFREE(char *) credgroup = NULL;
 const char *authcred;
-int ret = -1;
 
 *value = NULL;
 
@@ -119,47 +118,38 @@ int virAuthConfigLookup(virAuthConfigPtr auth,
 hostname = "localhost";
 
 if (virAsprintf(&authgroup, "auth-%s-%s", service, hostname) < 0)
-goto cleanup;
+return -1;
 
 if (!virKeyFileHasGroup(auth->keyfile, authgroup)) {
VIR_FREE(authgroup);
if (virAsprintf(&authgroup, "auth-%s-%s", service, "default") < 0)
-   goto cleanup;
+   return -1;
 }
 
-if (!virKeyFileHasGroup(auth->keyfile, authgroup)) {
-ret = 0;
-goto cleanup;
-}
+if (!virKeyFileHasGroup(auth->keyfile, authgroup))
+return 0;
 
 if (!(authcred = virKeyFileGetValueString(auth->keyfile, authgroup, 
"credentials"))) {
 virReportError(VIR_ERR_CONF_SYNTAX,
_("Missing item 'credentials' in group '%s' in '%s'"),
authgroup, auth->path);
-goto cleanup;
+return -1;
 }
 
 if (virAsprintf(&credgroup, "credentials-%s", authcred) < 0)
-goto cleanup;
+return -1;
 
 if (!virKeyFileHasGroup(auth->keyfile, credgroup)) {
 virReportError(VIR_ERR_CONF_SYNTAX,
_("Missing group 'credentials-%s' referenced from group 
'%s' in '%s'"),
authcred, authgroup, auth->path);
-goto cleanup;
+return -1;
 }
 
-if (!virKeyFileHasValue(auth->keyfile, credgroup, credname)) {
-ret = 0;
-goto cleanup;
-}
+if (!virKeyFileHasValue(auth->keyfile, credgroup, credname))
+return 0;
 
 *value = virKeyFileGetValueString(auth->keyfile, credgroup, credname);
 
-ret = 0;
-
- cleanup:
-VIR_FREE(authgroup);
-VIR_FREE(credgroup);
-return ret;
+return 0;
 }
-- 
1.8.3.1

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

[libvirt] [RFC 4/4] add automatic cleanup support in src/util/virauth.c

[Sukrit Bhatnagar]

Define a new cleanup function for virAuthConfigPtr in
src/util/virauthconfig.h.
Modifiy code to use cleanup macros where required.

Signed-off-by: Sukrit Bhatnagar 
---
 src/util/virauth.c   | 66 ++--
 src/util/virauthconfig.h |  3 +++
 2 files changed, 27 insertions(+), 42 deletions(-)

diff --git a/src/util/virauth.c b/src/util/virauth.c
index adb093e..aa7593c 100644
--- a/src/util/virauth.c
+++ b/src/util/virauth.c
@@ -26,7 +26,6 @@
 
 #include "virauth.h"
 #include "virutil.h"
-#include "viralloc.h"
 #include "virlog.h"
 #include "datatypes.h"
 #include "virerror.h"
@@ -42,10 +41,9 @@ int
 virAuthGetConfigFilePathURI(virURIPtr uri,
 char **path)
 {
-int ret = -1;
 size_t i;
 const char *authenv = virGetEnvBlockSUID("LIBVIRT_AUTH_FILE");
-char *userdir = NULL;
+VIR_AUTOFREE(char *) userdir = NULL;
 
 *path = NULL;
 
@@ -54,7 +52,7 @@ virAuthGetConfigFilePathURI(virURIPtr uri,
 if (authenv) {
 VIR_DEBUG("Using path from env '%s'", authenv);
 if (VIR_STRDUP(*path, authenv) < 0)
-goto cleanup;
+return -1;
 return 0;
 }
 
@@ -64,41 +62,38 @@ virAuthGetConfigFilePathURI(virURIPtr uri,
 uri->params[i].value) {
 VIR_DEBUG("Using path from URI '%s'", uri->params[i].value);
 if (VIR_STRDUP(*path, uri->params[i].value) < 0)
-goto cleanup;
+return -1;
 return 0;
 }
 }
 }
 
 if (!(userdir = virGetUserConfigDirectory()))
-goto cleanup;
+return -1;
 
 if (virAsprintf(path, "%s/auth.conf", userdir) < 0)
-goto cleanup;
+return -1;
 
 VIR_DEBUG("Checking for readability of '%s'", *path);
-if (access(*path, R_OK) == 0)
-goto done;
+if (access(*path, R_OK) == 0) {
+VIR_DEBUG("Using auth file '%s'", NULLSTR(*path));
+return 0;
+}
 
 VIR_FREE(*path);
 
 if (VIR_STRDUP(*path, SYSCONFDIR "/libvirt/auth.conf") < 0)
-goto cleanup;
+return -1;
 
 VIR_DEBUG("Checking for readability of '%s'", *path);
-if (access(*path, R_OK) == 0)
-goto done;
+if (access(*path, R_OK) == 0) {
+VIR_DEBUG("Using auth file '%s'", NULLSTR(*path));
+return 0;
+}
 
 VIR_FREE(*path);
 
- done:
-ret = 0;
-
-VIR_DEBUG("Using auth file '%s'", NULLSTR(*path));
- cleanup:
-VIR_FREE(userdir);
-
-return ret;
+return 0;
 }
 
 
@@ -117,8 +112,7 @@ virAuthGetCredential(const char *servicename,
  const char *path,
  char **value)
 {
-int ret = -1;
-virAuthConfigPtr config = NULL;
+VIR_AUTOPTR(virAuthConfigPtr) config = NULL;
 const char *tmp;
 
 *value = NULL;
@@ -127,23 +121,19 @@ virAuthGetCredential(const char *servicename,
 return 0;
 
 if (!(config = virAuthConfigNew(path)))
-goto cleanup;
+return -1;
 
 if (virAuthConfigLookup(config,
 servicename,
 hostname,
 credname,
 &tmp) < 0)
-goto cleanup;
+return -1;
 
 if (VIR_STRDUP(*value, tmp) < 0)
-goto cleanup;
+return -1;
 
-ret = 0;
-
- cleanup:
-virAuthConfigFree(config);
-return ret;
+return 0;
 }
 
 
@@ -156,7 +146,7 @@ virAuthGetUsernamePath(const char *path,
 {
 unsigned int ncred;
 virConnectCredential cred;
-char *prompt;
+VIR_AUTOFREE(char *) prompt = NULL;
 char *ret = NULL;
 
 if (virAuthGetCredential(servicename, hostname, "username", path, &ret) < 
0)
@@ -193,8 +183,6 @@ virAuthGetUsernamePath(const char *path,
 break;
 }
 
-VIR_FREE(prompt);
-
 return cred.result;
 }
 
@@ -207,7 +195,7 @@ virAuthGetUsername(virConnectPtr conn,
const char *hostname)
 {
 char *ret;
-char *path;
+VIR_AUTOFREE(char *) path = NULL;
 
 if (virAuthGetConfigFilePath(conn, &path) < 0)
 return NULL;
@@ -215,8 +203,6 @@ virAuthGetUsername(virConnectPtr conn,
 ret = virAuthGetUsernamePath(path, auth, servicename,
  defaultUsername, hostname);
 
-VIR_FREE(path);
-
 return ret;
 }
 
@@ -230,7 +216,7 @@ virAuthGetPasswordPath(const char *path,
 {
 unsigned int ncred;
 virConnectCredential cred;
-char *prompt;
+VIR_AUTOFREE(char *) prompt = NULL;
 char *ret = NULL;
 
 if (virAuthGetCredential(servicename, hostname, "password", path, &ret) < 
0)
@@ -264,8 +250,6 @@ virAuthGetPasswordPath(const char *path,
 break;
 }
 
-VIR_FREE(prompt);
-
 return cred.result;
 }
 
@@ -278,14 +262,12 @@ virAuthGetPassword(virConnectPtr conn,
const char *hostname)
 {
 char *ret;
-char *path;
+VIR_AUTOFREE(char *) path = NULL;
 
 if (virAuthGetConf

[libvirt] [RFC 2/4] add automatic cleanup support in src/util/virarptable.c

[Sukrit Bhatnagar]

Modifiy code to use cleanup macros where required.

Signed-off-by: Sukrit Bhatnagar 
---
 src/util/virarptable.c | 9 ++---
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/src/util/virarptable.c b/src/util/virarptable.c
index c0e90dc..f53a479 100644
--- a/src/util/virarptable.c
+++ b/src/util/virarptable.c
@@ -71,9 +71,8 @@ virArpTableGet(void)
 {
 int num = 0;
 int msglen;
-void *nlData = NULL;
+VIR_AUTOFREE(void *) nlData = NULL;
 virArpTablePtr table = NULL;
-char *ipstr = NULL;
 struct nlmsghdr* nh;
 struct rtattr * tb[NDA_MAX+1];
 
@@ -89,6 +88,7 @@ virArpTableGet(void)
 VIR_WARNINGS_NO_CAST_ALIGN
 for (; NLMSG_OK(nh, msglen); nh = NLMSG_NEXT(nh, msglen)) {
 VIR_WARNINGS_RESET
+VIR_AUTOFREE(char *) ipstr = NULL;
 struct ndmsg *r = NLMSG_DATA(nh);
 int len = nh->nlmsg_len;
 void *addr;
@@ -134,8 +134,6 @@ virArpTableGet(void)
 
 if (VIR_STRDUP(table->t[num].ipaddr, ipstr) < 0)
 goto cleanup;
-
-VIR_FREE(ipstr);
 }
 
 if (tb[NDA_LLADDR]) {
@@ -155,13 +153,10 @@ virArpTableGet(void)
 }
 
  end_of_netlink_messages:
-VIR_FREE(nlData);
 return table;
 
  cleanup:
 virArpTableFree(table);
-VIR_FREE(ipstr);
-VIR_FREE(nlData);
 return NULL;
 }
 
-- 
1.8.3.1

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

[libvirt] [PATCH] spec: Fix permissions of nwfilter XMLs

[Jiri Denemark]

The nwfilter XMLs in /etc are defined as %ghost in the spec file, which
means rpm will not install them, but it will record its existence and
permissions in the database. During installation the files are copied in
a %post scriptlet from /usr/share/libvirt/nwfilter, but once libvirtd is
restarted, it will rewrite the files to add generated UUIDs.

While RPM recorded 644 mode for the XMLs, libvirt saves them with 600
and thus any future attempt to verify the libvirt-daemon-config-nwfilter
package would fail. We need to tell RPM the ghost files are supposed to
have 600 permissions.

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

Signed-off-by: Jiri Denemark 
---
 libvirt.spec.in | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/libvirt.spec.in b/libvirt.spec.in
index 5e1e1df3f0..17d5d4dff7 100644
--- a/libvirt.spec.in
+++ b/libvirt.spec.in
@@ -1369,6 +1369,8 @@ rm -f 
$RPM_BUILD_ROOT%{_sysconfdir}/libvirt/qemu/networks/autostart/default.xml
 install -d -m 0755 $RPM_BUILD_ROOT%{_datadir}/libvirt/nwfilter/
 cp -a $RPM_BUILD_ROOT%{_sysconfdir}/libvirt/nwfilter/*.xml \
 $RPM_BUILD_ROOT%{_datadir}/libvirt/nwfilter/
+# libvirt saves these files with mode 600
+chmod 600 $RPM_BUILD_ROOT%{_sysconfdir}/libvirt/nwfilter/*.xml
 
 # Strip auto-generated UUID - we need it generated per-install
 sed -i -e "//d" $RPM_BUILD_ROOT%{_datadir}/libvirt/networks/default.xml
-- 
2.17.0

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

Re: [libvirt] [PATCH] virtlockd: acquire locks on re-exec

[Jim Fehlig]

On 05/28/2018 04:34 AM, Matthew Richardson wrote:

Locks held by virtlockd are dropped on re-exec.

virtlockd   94306 POSIX 5.4G WRITE 0 0   0 /tmp/test.qcow2
virtlockd   94306 POSIX   5B WRITE 0 0   0 /run/virtlockd.pid
virtlockd   94306 POSIX   5B WRITE 0 0   0 /run/virtlockd.pid

Acquire locks in PostExecRestart code path.

Signed-off-by: Jim Fehlig 


I've just encountered this same unexpected behaviour (currently on RHEL7
box running libvirt 3.2), and found this thread while searching for a
bug report.

Has this patch been accepted, or the issue fixed in a later release? I
notice there was some confusion in the conversation following as to the
cause of this issue/the fix for it.


The problem with losing fcntl locks on execve was worked around in libvirt with 
this patch series (included in libvirt 4.2.0)


https://www.redhat.com/archives/libvir-list/2018-March/msg00298.html


If not, is there a bug open against this which I can follow/comment on?


There's an open bug to track the lost locks

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

Regards,
Jim

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

Re: [libvirt] [PATCH 04/10] virCryptoGenerateRandom: Don't allocate return buffer

[Eric Blake]

On 05/29/2018 03:24 AM, Michal Privoznik wrote:

To unify our vir*Random() functions we need to make
virCryptoGenerateRandom NOT allocate return buffer. It should
just fill given buffer with random data.

Signed-off-by: Michal Privoznik 
---
  src/qemu/qemu_domain.c   | 12 
  src/util/vircrypto.c | 29 -
  src/util/vircrypto.h |  3 ++-
  tests/qemuxml2argvmock.c | 14 --
  4 files changed, 26 insertions(+), 32 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 47910acb83..2d13a03344 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -930,12 +930,13 @@ qemuDomainMasterKeyCreate(virDomainObjPtr vm)
  if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_OBJECT_SECRET))
  return 0;
  
-if (!(priv->masterKey =

-  virCryptoGenerateRandom(QEMU_DOMAIN_MASTER_KEY_LEN)))
+if (VIR_ALLOC_N(priv->masterKey, QEMU_DOMAIN_MASTER_KEY_LEN) < 0)
  return -1;
-
  priv->masterKeyLen = QEMU_DOMAIN_MASTER_KEY_LEN;
  
+if (virCryptoGenerateRandom(priv->masterKey, QEMU_DOMAIN_MASTER_KEY_LEN) < 0)

+return -1;


Should this free priv->masterKey and set it back to NULL, so that no 
other client is tempted to use a half-baked buffer as a key prior to the 
object being destroyed?




+++ b/tests/qemuxml2argvmock.c
@@ -190,17 +190,11 @@ virCommandPassFD(virCommandPtr cmd ATTRIBUTE_UNUSED,
  /* nada */
  }
  
-uint8_t *

-virCryptoGenerateRandom(size_t nbytes)
+int
+virCryptoGenerateRandom(unsigned char *buf,
+   size_t buflen)


Indentation looks off.


  {
-uint8_t *buf;
-
-if (VIR_ALLOC_N(buf, nbytes) < 0)
-return NULL;
-
-ignore_value(virRandomBytes(buf, nbytes));
-
-return buf;
+return virRandomBytes(buf, buflen);


Hmm, my earlier comment about the #if 0 for debugging might be more 
relevant here - if we are going to mock the random numbers to be 
reproducible during the testsuite, THIS would be a nice place to fall 
back to rand() and friends with a reliable sequence when given a fixed 
seed (rather than directly in src/util/virrandom.c).



--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

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