[libvirt] [PATCH v3 4/4] rpc: replacing ssh_write_knownhost() by ssh_session_update_known_hosts().

[Julio Faracco]

After version 0.8.0, libssh deprecated the function scope
ssh_write_knownhost() and moved to ssh_session_update_known_hosts().
So, libvirt is failing to compile using this new function name.

Signed-off-by: Julio Faracco 
---
 src/rpc/virnetlibsshsession.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/rpc/virnetlibsshsession.c b/src/rpc/virnetlibsshsession.c
index eb6d6843a2..5a7ca04dec 100644
--- a/src/rpc/virnetlibsshsession.c
+++ b/src/rpc/virnetlibsshsession.c
@@ -397,7 +397,7 @@ virNetLibsshCheckHostKey(virNetLibsshSessionPtr sess)
 
 /* write the host key file, if specified */
 if (sess->knownHostsFile) {
-if (ssh_write_knownhost(sess->session) < 0) {
+if (ssh_session_update_known_hosts(sess->session) < 0) {
 errmsg = ssh_get_error(sess->session);
 virReportError(VIR_ERR_LIBSSH,
_("failed to write known_host file '%s': %s"),
-- 
2.19.1

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

[libvirt] [PATCH v3 2/4] rpc: replacing ssh_is_server_known() by ssh_session_is_known_server().

[Julio Faracco]

After version 0.8.0, libssh deprecated the function scope
ssh_is_server_known() and moved to ssh_session_is_known_server().
So, libvirt is failing to compile using this new function name.

Signed-off-by: Julio Faracco 
---
 src/rpc/virnetlibsshsession.c | 16 
 1 file changed, 16 insertions(+)

diff --git a/src/rpc/virnetlibsshsession.c b/src/rpc/virnetlibsshsession.c
index 7c5f158f4d..eb6d6843a2 100644
--- a/src/rpc/virnetlibsshsession.c
+++ b/src/rpc/virnetlibsshsession.c
@@ -80,6 +80,22 @@ struct _virNetLibsshAuthMethod {
 int tries;
 };
 
+#ifndef HAVE_SSH_KNOWN_HOSTS_E
+/* This is an auxiliar enum to help libssh migration to version 0.8.0
+ * or higher. This enum associates the enumerator ssh_server_known_e
+ * with new ssh_known_hosts_e enum. In other words, it can be removed
+ * in the future. ERROR was moved from -1 to -2 and NOT_FOUND from 4
+ * to -1. */
+enum _vir_ssh_known_hosts_e {
+SSH_KNOWN_HOSTS_ERROR = SSH_SERVER_ERROR,
+SSH_KNOWN_HOSTS_UNKNOWN = SSH_SERVER_NOT_KNOWN,
+SSH_KNOWN_HOSTS_OK,
+SSH_KNOWN_HOSTS_CHANGED,
+SSH_KNOWN_HOSTS_OTHER,
+SSH_KNOWN_HOSTS_NOT_FOUND,
+};
+#endif
+
 struct _virNetLibsshSession {
 virObjectLockable parent;
 virNetLibsshSessionState state;
-- 
2.19.1

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

[libvirt] [PATCH v3 3/4] m4: checking if ssh_session_update_known_hosts() exists.

[Julio Faracco]

This commit adds some checks inside libssh m4 checking to verify if
ssh_session_update_known_hosts function is available. This new function
scope replaces the old ssh_write_knownhost() from libssh 0.8.0 and
below versions.

Signed-off-by: Julio Faracco 
---
 m4/virt-libssh.m4 | 4 
 1 file changed, 4 insertions(+)

diff --git a/m4/virt-libssh.m4 b/m4/virt-libssh.m4
index 52bd4d3639..f6d307f8d1 100644
--- a/m4/virt-libssh.m4
+++ b/m4/virt-libssh.m4
@@ -41,6 +41,10 @@ AC_DEFUN([LIBVIRT_CHECK_LIBSSH],[
 [AC_DEFINE([HAVE_SSH_KNOWN_HOSTS_E], [1],
   [Defined if enum ssh_known_hosts_e exists in libssh.h])],
 [], [[#include ]])
+AC_CHECK_FUNC([ssh_session_update_known_hosts],
+  [],
+  [AC_DEFINE_UNQUOTED([ssh_session_update_known_hosts], 
[ssh_write_knownhost],
+[ssh_write_knownhost is deprecated and replaced by 
ssh_session_update_known_hosts.])])
 CFLAGS="$old_CFLAGS"
 LIBS="$old_LIBS"
   fi
-- 
2.19.1

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

[libvirt] [PATCH v3 0/4] rpc: fixing compilation error due to deprecated functions.

[Julio Faracco]

After 0.8.0 release, libssh deprecated some functions like:
ssh_is_server_known() and ssh_write_knownhost(). They were replaced by
ssh_session_is_known_server() and ssh_session_update_known_hosts()
respectively. This serie creates the alias to keep the compatibility and
create an auxiliar enum to help it because
ssh_session_update_known_hosts() introduced new state returns.

v1-v2: Rebasing ssh_session_is_known_server() return states.
v2-v3: Only code syntax fixes.

Julio Faracco (4):
  m4: checking if ssh_session_is_known_server() exists.
  rpc: replacing ssh_is_server_known() by ssh_session_is_known_server().
  m4: checking if ssh_session_update_known_hosts() exists.
  rpc: replacing ssh_write_knownhost() by
ssh_session_update_known_hosts().

 m4/virt-libssh.m4 | 12 
 src/rpc/virnetlibsshsession.c | 18 +-
 2 files changed, 29 insertions(+), 1 deletion(-)

-- 
2.19.1

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

[libvirt] [PATCH v3 1/4] m4: checking if ssh_session_is_known_server() exists.

[Julio Faracco]

This commit adds some checks inside libssh m4 checking to verify if
ssh_session_is_known_server function is available. This new function
scope replaces the old ssh_is_server_known() from libssh 0.8.0 and
below versions.

Another auxiliar enumerator was added to keep the compatibility with the
old standard used by ssh_is_server_known() function.

Signed-off-by: Julio Faracco 
---
 m4/virt-libssh.m4 | 8 
 1 file changed, 8 insertions(+)

diff --git a/m4/virt-libssh.m4 b/m4/virt-libssh.m4
index 01c3b75c72..52bd4d3639 100644
--- a/m4/virt-libssh.m4
+++ b/m4/virt-libssh.m4
@@ -33,6 +33,14 @@ AC_DEFUN([LIBVIRT_CHECK_LIBSSH],[
   [],
   [AC_DEFINE_UNQUOTED([ssh_get_server_publickey], [ssh_get_publickey],
 [ssh_get_publickey is deprecated and replaced by 
ssh_get_server_publickey.])])
+AC_CHECK_FUNC([ssh_session_is_known_server],
+  [],
+  [AC_DEFINE_UNQUOTED([ssh_session_is_known_server], [ssh_is_server_known],
+[ssh_is_server_known is deprecated and replaced by 
ssh_session_is_known_server.])])
+AC_CHECK_TYPES([enum ssh_known_hosts_e],
+[AC_DEFINE([HAVE_SSH_KNOWN_HOSTS_E], [1],
+  [Defined if enum ssh_known_hosts_e exists in libssh.h])],
+[], [[#include ]])
 CFLAGS="$old_CFLAGS"
 LIBS="$old_LIBS"
   fi
-- 
2.19.1

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

Re: [libvirt] [PATCHv2 01/16] qemu: Add KVM CPUs into cache only if KVM is present

[Roman Bolshakov]

On Fri, Nov 23, 2018 at 06:16:46PM +0100, Jiri Denemark wrote:
> On Fri, Nov 23, 2018 at 18:55:00 +0300, Roman Bolshakov wrote:
> > On Fri, Nov 23, 2018 at 04:30:13PM +0100, Jiri Denemark wrote:
> > > On Fri, Nov 23, 2018 at 17:16:12 +0300, Roman Bolshakov wrote:
> > > > On Wed, Nov 21, 2018 at 07:43:43PM +0100, Jiri Denemark wrote:
> > > > > virQEMUCapsInitHostCPUModel always fills in something and your check
> > > > > should probably remain in place for it
> > > > > 
> > > > > virQEMUCapsFormatHostCPUModelInfo does
> > > > > virQEMUCapsHostCPUDataPtr cpuData = >kvmCPU;
> > > > > qemuMonitorCPUModelInfoPtr model = cpuData->info;
> > > > > if (!model)
> > > > > return;
> > > > > 
> > > > > virQEMUCapsFormatCPUModels
> > > > > cpus = qemuCaps->kvmCPUModels;
> > > > > if (!cpus)
> > > > > return;
> > > > > 
> > > > > So to me it looks like all functions are ready to see NULL pointers 
> > > > > and
> > > > > just do nothing if that's the case. Thus the only thing this patch
> > > > > should need to do is to make sure virQEMUCapsInitHostCPUModel does not
> > > > > set something non-NULL there.
> > > > 
> > > > Unfortunately, that won't work for the patch series. kvmCPUModels is 
> > > > renamed to
> > > > accelCPUModels and kvmCPU is renamed to accelCPU in PATCH 6.
> > > 
> > > And how does different name change the behavior?
> > > 
> > > > So, virQEMUCapsFormatHostCPUModelInfo looks like:
> > > > if (virQEMUCapsTypeIsAccelerated(type))
> > > > cpuData = qemuCaps->accelCPU;
> > > > else
> > > > cpuData = qemuCaps->tcgCPU;
> > > > 
> > > > and virQEMUCapsFormatCPUModels looks like:
> > > > if (virQEMUCapsTypeIsAccelerated(type))
> > > > cpus = qemuCaps->accelCPUModels;
> > > > else
> > > > cpus = qemuCaps->tcgCPUModels;
> > > > 
> > > > Without the check we'd return CPUs for KVM domain on the platform that 
> > > > doesn't
> > > > support it.
> > > 
> > > It won't return anything because the code will make sure accelCPUModels
> > > and accelCPU will be NULL when no accel method is supported.
> > 
> > But accelCPU is not NULL on macOS with QEMU_CAPS_HVF and on Linux with
> > QEMU_CAPS_KVM. That's where the problem arises.
> 
> Right, and that's what I think should be changed. Rather then adding
> checks to the formatting and loading code to ignore something which
> shouldn't be present in the first place.
> 
> > We're going to get additional kvm CPUs on mac and hvf CPUs on Linux
> > and that will break qemucapabilitiestest.
> 
> I think I'm missing something here. There's only one CPU definition
> describing the host CPU. There are hosts which have several different
> CPUs, but libvirt is not really prepared to see that and I believe this
> is not what you're addressing with this series, is it? Or are you
> talking about some other CPUs?
> 
> > In fact they will be the same accelCPUs of the supported accelerator
> > but with hostCPU's type attribute of the other accelerator.
> 
> How would this happen? We have a single accelerator enabled on a host
> and we generate a host CPU model for it (and just for it, there's no
> reason to generate a CPU model for something that is not supported on
> the host).
> 

accelCPU will be present on a host where an accelerator is avaialable.
You said can't have host CPU definitions present twice. I agree with
that. But if we call virQEMUCapsFormatCPUModels twice for
VIR_DOMAIN_VIRT_KVM and VIR_DOMAIN_VIRT_HVF without the checks, host cpu
definitions will be present twice for each accelerator because accelCPU
is not NULL.

So we need to call it only once for the supported accelerator.
The checks help in that. Alternative approach to do only one call is:

  virDomainVirtType acceleratedDomain = VIR_DOMAIN_VIRT_KVM;
  if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_HVF))
acceleratedDomain = VIR_DOMAIN_VIRT_HVF;

  virQEMUCapsFormatHostCPUModelInfo(qemuCaps, , acceleratedDomain);
  virQEMUCapsFormatHostCPUModelInfo(qemuCaps, , VIR_DOMAIN_VIRT_QEMU);

  virQEMUCapsFormatCPUModels(qemuCaps, , acceleratedDomain);
  virQEMUCapsFormatCPUModels(qemuCaps, , VIR_DOMAIN_VIRT_QEMU);

Would that work for you?

--
Thank you,
Roman

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

Re: [libvirt] [PATCHv9 1/2] qemu: Report cache occupancy (CMT) with domstats

[John Ferlan]

On 11/20/18 8:56 AM, Wang Huaqiang wrote:
> Adding the interface in qemu to report CMT statistic information
> through command 'virsh domstats --cpu-total'.
> 
> Below is a typical output:
> 
>  # virsh domstats 1 --cpu-total
>  Domain: 'ubuntu16.04-base'
>...
>cpu.cache.monitor.count=2
>cpu.cache.monitor.0.name=vcpus_1
>cpu.cache.monitor.0.vcpus=1
>cpu.cache.monitor.0.bank.count=2
>cpu.cache.monitor.0.bank.0.id=0
>cpu.cache.monitor.0.bank.0.bytes=4505600
>cpu.cache.monitor.0.bank.1.id=1
>cpu.cache.monitor.0.bank.1.bytes=5586944
>cpu.cache.monitor.1.name=vcpus_4-6
>cpu.cache.monitor.1.vcpus=4,5,6
>cpu.cache.monitor.1.bank.count=2
>cpu.cache.monitor.1.bank.0.id=0
>cpu.cache.monitor.1.bank.0.bytes=17571840
>cpu.cache.monitor.1.bank.1.id=1
>cpu.cache.monitor.1.bank.1.bytes=29106176
> 
> Signed-off-by: Wang Huaqiang 
> ---
>  src/libvirt-domain.c   |  12 
>  src/qemu/qemu_driver.c | 160 
> -
>  tools/virsh.pod|  14 +
>  3 files changed, 185 insertions(+), 1 deletion(-)
> 
> diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
> index 5b76458..73d602e 100644
> --- a/src/libvirt-domain.c
> +++ b/src/libvirt-domain.c
> @@ -11415,6 +11415,18 @@ virConnectGetDomainCapabilities(virConnectPtr conn,
>   * "cpu.user" - user cpu time spent in nanoseconds as unsigned long long.
>   * "cpu.system" - system cpu time spent in nanoseconds as unsigned long
>   *long.
> + * "cpu.cache.monitor.count" - the number of cache monitors for this 
> domain
> + * "cpu.cache.monitor..name" - the name of cache monitor 
> + * "cpu.cache.monitor..vcpus" - vcpu list of cache monitor 
> + * "cpu.cache.monitor..bank.count" - the number of cache banks in
> + *cache monitor 
> + * "cpu.cache.monitor..bank..id" - host allocated cache id 
> for
> + * bank  in cache
> + * monitor 
> + * "cpu.cache.monitor..bank..bytes" - the number of bytes of
> + *last level cache that 
> the
> + *domain is using on 
> cache
> + *bank 
>   *
>   * VIR_DOMAIN_STATS_BALLOON:
>   * Return memory balloon device information.
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 7fb9102..d9e216c 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -19929,6 +19929,158 @@ typedef enum {
>  #define HAVE_JOB(flags) ((flags) & QEMU_DOMAIN_STATS_HAVE_JOB)
>  
>  
> +typedef struct _virQEMUResctrlMonData virQEMUResctrlMonData;
> +typedef virQEMUResctrlMonData *virQEMUResctrlMonDataPtr;
> +struct _virQEMUResctrlMonData {
> +const char *name;
> +char *vcpus;
> +virResctrlMonitorStatsPtr stats;
> +size_t nstats;
> +};
> +
> +
> +static int
> +qemuDomainGetResctrlMonData(virDomainObjPtr dom,
> +virQEMUResctrlMonDataPtr resdata)
> +{
> +virDomainResctrlDefPtr resctrl = NULL;
> +size_t i = 0;
> +size_t j = 0;
> +size_t k = 0;
> +
> +for (i = 0; i < dom->def->nresctrls; i++) {
> +resctrl = dom->def->resctrls[i];
> +
> +for (j = 0; j < resctrl->nmonitors; j++) {
> +virDomainResctrlMonDefPtr domresmon = NULL;
> +virResctrlMonitorPtr monitor = NULL;
> +
> +domresmon = resctrl->monitors[j];
> +monitor = domresmon->instance;
> +
> +if (domresmon->tag != VIR_RESCTRL_MONITOR_TYPE_CACHE)
> +continue;

If you want to make this generic, then you could pass this tag from
qemuDomainGetStatsCpuCache as the rest would seemingly be useful for
VIR_RESCTRL_MONITOR_TYPE_MEMBW eventually, just different results.

> +
> +/* If virBitmapFormat successfully returns an vcpu string, then
> + * resdata[k].vcpus is assigned with an memory space holding it,
> + * let this newly allocated memory buffer to be freed along with
> + * the free of 'resdata' */
> +if (!(resdata[k].vcpus = virBitmapFormat(domresmon->vcpus)))
> +return -1;
> +
> +if (!(resdata[k].name = virResctrlMonitorGetID(monitor))) {

Could this ever be NULL?  Perhaps we just assign directly and assume
we're good. Alternatively it's a VIR_STRDUP() w/ the corresponding
VIR_FREE(*->name).

> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +   _("Could not get monitor ID"));
> +return -1;
> +}
> +
> +if (virResctrlMonitorGetCacheOccupancy(monitor,
> +  

Re: [libvirt] [PATCHv2 01/16] qemu: Add KVM CPUs into cache only if KVM is present

[Jiri Denemark]

On Fri, Nov 23, 2018 at 18:55:00 +0300, Roman Bolshakov wrote:
> On Fri, Nov 23, 2018 at 04:30:13PM +0100, Jiri Denemark wrote:
> > On Fri, Nov 23, 2018 at 17:16:12 +0300, Roman Bolshakov wrote:
> > > On Wed, Nov 21, 2018 at 07:43:43PM +0100, Jiri Denemark wrote:
> > > > virQEMUCapsInitHostCPUModel always fills in something and your check
> > > > should probably remain in place for it
> > > > 
> > > > virQEMUCapsFormatHostCPUModelInfo does
> > > > virQEMUCapsHostCPUDataPtr cpuData = >kvmCPU;
> > > > qemuMonitorCPUModelInfoPtr model = cpuData->info;
> > > > if (!model)
> > > > return;
> > > > 
> > > > virQEMUCapsFormatCPUModels
> > > > cpus = qemuCaps->kvmCPUModels;
> > > > if (!cpus)
> > > > return;
> > > > 
> > > > So to me it looks like all functions are ready to see NULL pointers and
> > > > just do nothing if that's the case. Thus the only thing this patch
> > > > should need to do is to make sure virQEMUCapsInitHostCPUModel does not
> > > > set something non-NULL there.
> > > 
> > > Unfortunately, that won't work for the patch series. kvmCPUModels is 
> > > renamed to
> > > accelCPUModels and kvmCPU is renamed to accelCPU in PATCH 6.
> > 
> > And how does different name change the behavior?
> > 
> > > So, virQEMUCapsFormatHostCPUModelInfo looks like:
> > > if (virQEMUCapsTypeIsAccelerated(type))
> > > cpuData = qemuCaps->accelCPU;
> > > else
> > > cpuData = qemuCaps->tcgCPU;
> > > 
> > > and virQEMUCapsFormatCPUModels looks like:
> > > if (virQEMUCapsTypeIsAccelerated(type))
> > > cpus = qemuCaps->accelCPUModels;
> > > else
> > > cpus = qemuCaps->tcgCPUModels;
> > > 
> > > Without the check we'd return CPUs for KVM domain on the platform that 
> > > doesn't
> > > support it.
> > 
> > It won't return anything because the code will make sure accelCPUModels
> > and accelCPU will be NULL when no accel method is supported.
> 
> But accelCPU is not NULL on macOS with QEMU_CAPS_HVF and on Linux with
> QEMU_CAPS_KVM. That's where the problem arises.

Right, and that's what I think should be changed. Rather then adding
checks to the formatting and loading code to ignore something which
shouldn't be present in the first place.

> We're going to get additional kvm CPUs on mac and hvf CPUs on Linux
> and that will break qemucapabilitiestest.

I think I'm missing something here. There's only one CPU definition
describing the host CPU. There are hosts which have several different
CPUs, but libvirt is not really prepared to see that and I believe this
is not what you're addressing with this series, is it? Or are you
talking about some other CPUs?

> In fact they will be the same accelCPUs of the supported accelerator
> but with hostCPU's type attribute of the other accelerator.

How would this happen? We have a single accelerator enabled on a host
and we generate a host CPU model for it (and just for it, there's no
reason to generate a CPU model for something that is not supported on
the host).

> If you wish I can try to rework the patchset. Instead of generalizing
> kvmCPU, I'd just add hvfCPU to qemuCaps. It might have a good side
> effect that libvirt will be able to support multiple accelerators on the
> same platform.

I think we can leave this for the future :-)

Jirka

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

Re: [libvirt] [PATCHv2 01/16] qemu: Add KVM CPUs into cache only if KVM is present

[Roman Bolshakov]

On Fri, Nov 23, 2018 at 04:30:13PM +0100, Jiri Denemark wrote:
> On Fri, Nov 23, 2018 at 17:16:12 +0300, Roman Bolshakov wrote:
> > On Wed, Nov 21, 2018 at 07:43:43PM +0100, Jiri Denemark wrote:
> > > virQEMUCapsInitHostCPUModel always fills in something and your check
> > > should probably remain in place for it
> > > 
> > > virQEMUCapsFormatHostCPUModelInfo does
> > > virQEMUCapsHostCPUDataPtr cpuData = >kvmCPU;
> > > qemuMonitorCPUModelInfoPtr model = cpuData->info;
> > > if (!model)
> > > return;
> > > 
> > > virQEMUCapsFormatCPUModels
> > > cpus = qemuCaps->kvmCPUModels;
> > > if (!cpus)
> > > return;
> > > 
> > > So to me it looks like all functions are ready to see NULL pointers and
> > > just do nothing if that's the case. Thus the only thing this patch
> > > should need to do is to make sure virQEMUCapsInitHostCPUModel does not
> > > set something non-NULL there.
> > 
> > Unfortunately, that won't work for the patch series. kvmCPUModels is 
> > renamed to
> > accelCPUModels and kvmCPU is renamed to accelCPU in PATCH 6.
> 
> And how does different name change the behavior?
> 
> > So, virQEMUCapsFormatHostCPUModelInfo looks like:
> > if (virQEMUCapsTypeIsAccelerated(type))
> > cpuData = qemuCaps->accelCPU;
> > else
> > cpuData = qemuCaps->tcgCPU;
> > 
> > and virQEMUCapsFormatCPUModels looks like:
> > if (virQEMUCapsTypeIsAccelerated(type))
> > cpus = qemuCaps->accelCPUModels;
> > else
> > cpus = qemuCaps->tcgCPUModels;
> > 
> > Without the check we'd return CPUs for KVM domain on the platform that 
> > doesn't
> > support it.
> 
> It won't return anything because the code will make sure accelCPUModels
> and accelCPU will be NULL when no accel method is supported.

But accelCPU is not NULL on macOS with QEMU_CAPS_HVF and on Linux with
QEMU_CAPS_KVM. That's where the problem arises. We're going to get
additional kvm CPUs on mac and hvf CPUs on Linux and that will break
qemucapabilitiestest.

In fact they will be the same accelCPUs of the supported accelerator but
with hostCPU's type attribute of the other accelerator.

If you wish I can try to rework the patchset. Instead of generalizing
kvmCPU, I'd just add hvfCPU to qemuCaps. It might have a good side
effect that libvirt will be able to support multiple accelerators on the
same platform.

--
Roman

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

Re: [libvirt] [PATCHv2 01/16] qemu: Add KVM CPUs into cache only if KVM is present

[Jiri Denemark]

On Fri, Nov 23, 2018 at 17:16:12 +0300, Roman Bolshakov wrote:
> On Wed, Nov 21, 2018 at 07:43:43PM +0100, Jiri Denemark wrote:
> > On Wed, Nov 21, 2018 at 20:50:50 +0300, Roman Bolshakov wrote:
> > > On Wed, Nov 21, 2018 at 05:04:07PM +0100, Jiri Denemark wrote:
> > > > On Wed, Nov 21, 2018 at 17:01:44 +0300, Roman Bolshakov wrote:
> > > > > diff --git a/src/qemu/qemu_capabilities.c 
> > > > > b/src/qemu/qemu_capabilities.c
> > > > > index fde27010e4..4ba8369e3a 100644
> > > > > --- a/src/qemu/qemu_capabilities.c
> > > > > +++ b/src/qemu/qemu_capabilities.c
> > > > > @@ -3467,11 +3467,13 @@ virQEMUCapsLoadCache(virArch hostArch,
> > > > >  }
> > > > >  VIR_FREE(str);
> > > > >  
> > > > > -if (virQEMUCapsLoadHostCPUModelInfo(qemuCaps, ctxt, 
> > > > > VIR_DOMAIN_VIRT_KVM) < 0 ||
> > > > > +if ((virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM) &&
> > > > > + virQEMUCapsLoadHostCPUModelInfo(qemuCaps, ctxt, 
> > > > > VIR_DOMAIN_VIRT_KVM) < 0) ||
> > > > >  virQEMUCapsLoadHostCPUModelInfo(qemuCaps, ctxt, 
> > > > > VIR_DOMAIN_VIRT_QEMU) < 0)
> > > > >  goto cleanup;
> > > > 
> > > > I don't think we should introduce these guards in all the places. All
> > > > the loading and formatting functions should return success if the
> > > > appropriate info is not available, so you should just make sure the
> > > > relevant info is NULL in qemuCaps.
> > > 
> > > Do you mean the capabilities checks should be moved inside the
> > > functions?
> > 
> > virQEMUCapsLoadHostCPUModelInfo does (not literally, but effectively)
> > 
> > hostCPUNode = virXPathNode("./hostCPU[@type='kvm']", ctxt);
> > if (!hostCPUNode)
> > return 0;
> > 
> > virQEMUCapsLoadCPUModels does
> > n = virXPathNodeSet("./cpu[@type='kvm']", ctxt, );
> > if (n == 0)
> > return 0;
> > 
> 
> I agree, virQEMUCapsLoadHostCPUModelInfo and virQEMUCapsLoadCPUModels
> don't need the check.
> 
> > virQEMUCapsInitHostCPUModel always fills in something and your check
> > should probably remain in place for it
> > 
> > virQEMUCapsFormatHostCPUModelInfo does
> > virQEMUCapsHostCPUDataPtr cpuData = >kvmCPU;
> > qemuMonitorCPUModelInfoPtr model = cpuData->info;
> > if (!model)
> > return;
> > 
> > virQEMUCapsFormatCPUModels
> > cpus = qemuCaps->kvmCPUModels;
> > if (!cpus)
> > return;
> > 
> > So to me it looks like all functions are ready to see NULL pointers and
> > just do nothing if that's the case. Thus the only thing this patch
> > should need to do is to make sure virQEMUCapsInitHostCPUModel does not
> > set something non-NULL there.
> 
> Unfortunately, that won't work for the patch series. kvmCPUModels is renamed 
> to
> accelCPUModels and kvmCPU is renamed to accelCPU in PATCH 6.

And how does different name change the behavior?

> So, virQEMUCapsFormatHostCPUModelInfo looks like:
> if (virQEMUCapsTypeIsAccelerated(type))
> cpuData = qemuCaps->accelCPU;
> else
> cpuData = qemuCaps->tcgCPU;
> 
> and virQEMUCapsFormatCPUModels looks like:
> if (virQEMUCapsTypeIsAccelerated(type))
> cpus = qemuCaps->accelCPUModels;
> else
> cpus = qemuCaps->tcgCPUModels;
> 
> Without the check we'd return CPUs for KVM domain on the platform that doesn't
> support it.

It won't return anything because the code will make sure accelCPUModels
and accelCPU will be NULL when no accel method is supported.

Jirka

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

Re: [libvirt] [PATCH v2 0/4] rpc: fixing compilation error due to deprecated functions.

[no-reply]

Hi,

This series was run against 'syntax-check' test by patchew.org, which failed, 
please find the details below:

Type: series
Subject: [libvirt] [PATCH v2 0/4] rpc: fixing compilation error due to 
deprecated functions.
Message-id: 20181123150154.28335-1-jcfara...@gmail.com

=== TEST SCRIPT BEGIN ===
#!/bin/bash
# Testing script will be invoked under the git checkout with
# HEAD pointing to a commit that has the patches applied on top of "base"
# branch
time bash -c './autogen.sh && make syntax-check'
=== TEST SCRIPT END ===

Updating bcb55ab053bc79561b55d0394490f4b64e0f2d01
>From https://github.com/patchew-project/libvirt
 * [new tag] patchew/20181123150154.28335-1-jcfara...@gmail.com -> 
patchew/20181123150154.28335-1-jcfara...@gmail.com
Switched to a new branch 'test'
347ab84 rpc: replacing ssh_write_knownhost() by 
ssh_session_update_known_hosts().
1253772 m4: checking if ssh_session_update_known_hosts() exists.
ba4973c rpc: replacing ssh_is_server_known() by ssh_session_is_known_server().
f4718f2 m4: checking if ssh_session_is_known_server() exists.

=== OUTPUT BEGIN ===
Updating submodules...
Submodule 'gnulib' (https://git.savannah.gnu.org/git/gnulib.git/) registered 
for path '.gnulib'
Submodule 'keycodemapdb' (https://gitlab.com/keycodemap/keycodemapdb.git) 
registered for path 'src/keycodemapdb'
Cloning into '.gnulib'...
Submodule path '.gnulib': checked out '68df637b5f1b5c10370f6981d2a43a5cf74368df'
Cloning into 'src/keycodemapdb'...
Submodule path 'src/keycodemapdb': checked out 
'16e5b0787687d8904dad2c026107409eb9bfcb95'
Running bootstrap...
./bootstrap: Bootstrapping from checked-out libvirt sources...
./bootstrap: consider installing git-merge-changelog from gnulib
./bootstrap: getting gnulib files...
running: libtoolize --install --copy
libtoolize: putting auxiliary files in AC_CONFIG_AUX_DIR, `build-aux'.
libtoolize: copying file `build-aux/config.guess'
libtoolize: copying file `build-aux/config.sub'
libtoolize: copying file `build-aux/install-sh'
libtoolize: copying file `build-aux/ltmain.sh'
libtoolize: putting macros in AC_CONFIG_MACRO_DIR, `m4'.
libtoolize: copying file `m4/libtool.m4'
libtoolize: copying file `m4/ltoptions.m4'
libtoolize: copying file `m4/ltsugar.m4'
libtoolize: copying file `m4/ltversion.m4'
libtoolize: copying file `m4/lt~obsolete.m4'
./bootstrap: .gnulib/gnulib-tool--no-changelog   --aux-dir=build-aux   
--doc-base=doc   --lib=libgnu   --m4-base=m4/   --source-base=gnulib/lib/   
--tests-base=gnulib/tests   --local-dir=gnulib/local--lgpl=2 --with-tests 
--makefile-name=gnulib.mk --avoid=pt_chown --avoid=lock-tests   --libtool 
--import ...
Module list with included dependencies (indented):
absolute-header
  accept
accept-tests
alloca
alloca-opt
alloca-opt-tests
allocator
  areadlink
areadlink-tests
arpa_inet
arpa_inet-tests
assure
  autobuild
  base64
base64-tests
binary-io
binary-io-tests
  bind
bind-tests
  bitrotate
bitrotate-tests
btowc
btowc-tests
builtin-expect
  byteswap
byteswap-tests
  c-ctype
c-ctype-tests
  c-strcase
c-strcase-tests
  c-strcasestr
c-strcasestr-tests
  calloc-posix
  canonicalize-lgpl
canonicalize-lgpl-tests
careadlinkat
  chown
chown-tests
  clock-time
cloexec
cloexec-tests
  close
close-tests
  configmake
  connect
connect-tests
  count-leading-zeros
count-leading-zeros-tests
  count-one-bits
count-one-bits-tests
ctype
ctype-tests
  dirname-lgpl
dosname
double-slash-root
dup
dup-tests
dup2
dup2-tests
  environ
environ-tests
errno
errno-tests
error
  execinfo
exitfail
extensions
extern-inline
fatal-signal
  fclose
fclose-tests
  fcntl
  fcntl-h
fcntl-h-tests
fcntl-tests
fd-hook
  fdatasync
fdatasync-tests
fdopen
fdopen-tests
fflush
fflush-tests
  ffs
ffs-tests
  ffsl
ffsl-tests
fgetc-tests
filename
flexmember
float
float-tests
  fnmatch
fnmatch-tests
fpieee
fpucw
fpurge
fpurge-tests
fputc-tests
fread-tests
freading
freading-tests
fseek
fseek-tests
fseeko
fseeko-tests
fstat
fstat-tests
  fsync
fsync-tests
ftell
ftell-tests
ftello
ftello-tests
ftruncate
ftruncate-tests
  func
func-tests
fwrite-tests
  getaddrinfo
getaddrinfo-tests
  getcwd-lgpl
getcwd-lgpl-tests
getdelim
getdelim-tests
getdtablesize
getdtablesize-tests
getgroups
getgroups-tests
  gethostname
gethostname-tests
getline
getline-tests
  getopt-posix
getopt-posix-tests
getpagesize
  getpass
  getpeername
getpeername-tests
getprogname
getprogname-tests
  getsockname
getsockname-tests
getsockopt
getsockopt-tests
gettext-h
  gettimeofday
gettimeofday-tests
getugroups
  gitlog-to-changelog
  gnumakefile
grantpt
grantpt-tests
 

Re: [libvirt] [PATCHv2 07/16] qemu: Introduce virQEMUCapsTypeIsAccelerated

[Roman Bolshakov]

On Fri, Nov 23, 2018 at 03:27:50PM +0100, Pino Toscano wrote:
> On Wednesday, 21 November 2018 15:01:50 CET Roman Bolshakov wrote:
> > +static bool
> > +virQEMUCapsTypeIsAccelerated(virDomainVirtType type)
> > +{
> > +return type == VIR_DOMAIN_VIRT_KVM;
> > +}
> > [...]
> > @@ -4966,7 +4971,8 @@ virQEMUCapsCacheLookupDefault(virFileCachePtr cache,
> >  if (virttype == VIR_DOMAIN_VIRT_NONE)
> >  virttype = capsType;
> >  
> > -if (virttype == VIR_DOMAIN_VIRT_KVM && capsType == 
> > VIR_DOMAIN_VIRT_QEMU) {
> > +if (virQEMUCapsTypeIsAccelerated(virttype) &&
> > +!virQEMUCapsTypeIsAccelerated(capsType)) {
> >  virReportError(VIR_ERR_INVALID_ARG,
> > _("KVM is not supported by '%s' on this host"),
> > binary);
> 
> From what I see, this check is now different:
> - "capsType == VIR_DOMAIN_VIRT_QEMU" will be true only when capsType is
>   VIR_DOMAIN_VIRT_QEMU
> - !virQEMUCapsTypeIsAccelerated(capsType) will be true when capsType is
>   not VIR_DOMAIN_VIRT_KVM
> 

Hi Pino,

Yep, sure I can leave the 'capsType == VIR_DOMAIN_VIRT_QEMU' as is.

Thank you,
Roman

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

Re: [libvirt] [PATCH v2 3/4] qemuMigrationDstPrepareAny: Parse cookie before adding domain onto list

[Jiri Denemark]

On Fri, Nov 23, 2018 at 14:23:50 +0100, Michal Privoznik wrote:
> There are some checks done when parsing a migration cookie. For
> instance, one of the checks ensures that the domain is not being
> migrated onto the same host. If that is the case, then we are in
> big trouble because the @vm is the same domain object used by
> source and it has some jobs sets and everything so recovering
> from failed cookie parsing would be needlessly hard.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/qemu/qemu_migration.c | 25 ++---
>  1 file changed, 14 insertions(+), 11 deletions(-)

Reviewed-by: Jiri Denemark 

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

Re: [libvirt] [PATCH v2 2/4] qemuMigrationEatCookie: Pass virDomainDef instead of virDomainObj

[Jiri Denemark]

On Fri, Nov 23, 2018 at 14:23:49 +0100, Michal Privoznik wrote:
> The function currently takes virDomainObjPtr because it's using
> both: the domain definition and domain private data.
> Unfortunately, this means that in prepare phase we can't parse
> migration cookie before putting incoming domain def onto domain
> objects list (addressed in the very next commit). Change the
> arguments so that virDomainDef and private data are passed
> instead of virDomainObjPtr.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/qemu/qemu_migration.c| 16 ++--
>  src/qemu/qemu_migration_cookie.c | 23 ---
>  src/qemu/qemu_migration_cookie.h |  4 +++-
>  3 files changed, 25 insertions(+), 18 deletions(-)

Reviewed-by: Jiri Denemark 

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

[libvirt] [PATCH v2 4/4] rpc: replacing ssh_write_knownhost() by ssh_session_update_known_hosts().

[Julio Faracco]

After version 0.8.0, libssh deprecated the function scope
ssh_write_knownhost() and moved to ssh_session_update_known_hosts().
So, libvirt is failing to compile using this new function name.

Signed-off-by: Julio Faracco 
---
 src/rpc/virnetlibsshsession.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/rpc/virnetlibsshsession.c b/src/rpc/virnetlibsshsession.c
index 2252bf2975..07fe284ce0 100644
--- a/src/rpc/virnetlibsshsession.c
+++ b/src/rpc/virnetlibsshsession.c
@@ -396,7 +396,7 @@ virNetLibsshCheckHostKey(virNetLibsshSessionPtr sess)
 
 /* write the host key file, if specified */
 if (sess->knownHostsFile) {
-if (ssh_write_knownhost(sess->session) < 0) {
+if (ssh_session_update_known_hosts(sess->session) < 0) {
 errmsg = ssh_get_error(sess->session);
 virReportError(VIR_ERR_LIBSSH,
_("failed to write known_host file '%s': %s"),
-- 
2.19.1

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

[libvirt] [PATCH v2 0/4] rpc: fixing compilation error due to deprecated functions.

[Julio Faracco]

After 0.8.0 release, libssh deprecated some functions like:
ssh_is_server_known() and ssh_write_knownhost(). They were replaced by
ssh_session_is_known_server() and ssh_session_update_known_hosts()
respectively. This serie creates the alias to keep the compatibility and
create an auxiliar enum to help it because
ssh_session_update_known_hosts() introduced new state returns.

Julio Faracco (4):
  m4: checking if ssh_session_is_known_server() exists.
  rpc: replacing ssh_is_server_known() by ssh_session_is_known_server().
  m4: checking if ssh_session_update_known_hosts() exists.
  rpc: replacing ssh_write_knownhost() by
ssh_session_update_known_hosts().

 m4/virt-libssh.m4 | 12 
 src/rpc/virnetlibsshsession.c | 31 +++
 2 files changed, 35 insertions(+), 8 deletions(-)

-- 
2.19.1

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

[libvirt] [PATCH v2 1/4] m4: checking if ssh_session_is_known_server() exists.

[Julio Faracco]

This commit adds some checks inside libssh m4 checking to verify if
ssh_session_is_known_server function is available. This new function
scope replaces the old ssh_is_server_known() from libssh 0.8.0 and
below versions.

Another auxiliar enumerator was added to keep the compatibility with the
old standard used by ssh_is_server_known() function.

Signed-off-by: Julio Faracco 
---
 m4/virt-libssh.m4 | 8 
 1 file changed, 8 insertions(+)

diff --git a/m4/virt-libssh.m4 b/m4/virt-libssh.m4
index 01c3b75c72..6eac84cfe7 100644
--- a/m4/virt-libssh.m4
+++ b/m4/virt-libssh.m4
@@ -33,6 +33,14 @@ AC_DEFUN([LIBVIRT_CHECK_LIBSSH],[
   [],
   [AC_DEFINE_UNQUOTED([ssh_get_server_publickey], [ssh_get_publickey],
 [ssh_get_publickey is deprecated and replaced by 
ssh_get_server_publickey.])])
+AC_CHECK_FUNC([ssh_session_is_known_server],
+  [],
+  [AC_DEFINE_UNQUOTED([ssh_session_is_known_server], [ssh_is_server_known],
+[ssh_is_server_known is deprecated and replaced by 
ssh_session_is_known_server.])])
+AC_CHECK_TYPE([enum ssh_known_hosts_e],
+[AC_DEFINE([HAVE_SSH_KNOWN_HOSTS_E], [1],
+  [Defined if enum ssh_known_hosts_e exists in libssh.h])],
+[], [[#include ]])
 CFLAGS="$old_CFLAGS"
 LIBS="$old_LIBS"
   fi
-- 
2.19.1

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

[libvirt] [PATCH v2 2/4] rpc: replacing ssh_is_server_known() by ssh_session_is_known_server().

[Julio Faracco]

After version 0.8.0, libssh deprecated the function scope
ssh_is_server_known() and moved to ssh_session_is_known_server().
So, libvirt is failing to compile using this new function name.

Signed-off-by: Julio Faracco 
---
 src/rpc/virnetlibsshsession.c | 29 ++---
 1 file changed, 22 insertions(+), 7 deletions(-)

diff --git a/src/rpc/virnetlibsshsession.c b/src/rpc/virnetlibsshsession.c
index 7c5f158f4d..2252bf2975 100644
--- a/src/rpc/virnetlibsshsession.c
+++ b/src/rpc/virnetlibsshsession.c
@@ -80,6 +80,21 @@ struct _virNetLibsshAuthMethod {
 int tries;
 };
 
+#ifndef HAVE_SSH_KNOWN_HOSTS_E
+/* This is an auxiliar enum to help libssh migration to version 0.8.0
+ * or higher. This enum associates the enumerator ssh_server_known_e
+ * with new ssh_known_hosts_e enum. In other words, it can be removed
+ * in the future. */
+enum _vir_ssh_known_hosts_e {
+SSH_KNOWN_HOSTS_ERROR=SSH_SERVER_ERROR,
+SSH_KNOWN_HOSTS_UNKNOWN=SSH_SERVER_NOT_KNOWN,
+SSH_KNOWN_HOSTS_OK,
+SSH_KNOWN_HOSTS_CHANGED,
+SSH_KNOWN_HOSTS_OTHER,
+SSH_KNOWN_HOSTS_NOT_FOUND,
+};
+#endif
+
 struct _virNetLibsshSession {
 virObjectLockable parent;
 virNetLibsshSessionState state;
@@ -287,15 +302,15 @@ virNetLibsshCheckHostKey(virNetLibsshSessionPtr sess)
 if (sess->hostKeyVerify == VIR_NET_LIBSSH_HOSTKEY_VERIFY_IGNORE)
 return 0;
 
-state = ssh_is_server_known(sess->session);
+state = ssh_session_is_known_server(sess->session);
 
 switch (state) {
-case SSH_SERVER_KNOWN_OK:
+case SSH_KNOWN_HOSTS_OK:
 /* host key matches */
 return 0;
 
-case SSH_SERVER_FOUND_OTHER:
-case SSH_SERVER_KNOWN_CHANGED:
+case SSH_KNOWN_HOSTS_OTHER:
+case SSH_KNOWN_HOSTS_CHANGED:
 keyhashstr = virLibsshServerKeyAsString(sess);
 if (!keyhashstr)
 return -1;
@@ -312,8 +327,8 @@ virNetLibsshCheckHostKey(virNetLibsshSessionPtr sess)
 ssh_string_free_char(keyhashstr);
 return -1;
 
-case SSH_SERVER_FILE_NOT_FOUND:
-case SSH_SERVER_NOT_KNOWN:
+case SSH_KNOWN_HOSTS_NOT_FOUND:
+case SSH_KNOWN_HOSTS_UNKNOWN:
 /* key was not found, query to add it to database */
 if (sess->hostKeyVerify == VIR_NET_LIBSSH_HOSTKEY_VERIFY_NORMAL) {
 virConnectCredential askKey;
@@ -393,7 +408,7 @@ virNetLibsshCheckHostKey(virNetLibsshSessionPtr sess)
 /* key was accepted and added */
 return 0;
 
-case SSH_SERVER_ERROR:
+case SSH_KNOWN_HOSTS_ERROR:
 errmsg = ssh_get_error(sess->session);
 virReportError(VIR_ERR_LIBSSH,
_("failed to validate SSH host key: %s"),
-- 
2.19.1

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

[libvirt] [PATCH v2 3/4] m4: checking if ssh_session_update_known_hosts() exists.

[Julio Faracco]

This commit adds some checks inside libssh m4 checking to verify if
ssh_session_update_known_hosts function is available. This new function
scope replaces the old ssh_write_knownhost() from libssh 0.8.0 and
below versions.

Signed-off-by: Julio Faracco 
---
 m4/virt-libssh.m4 | 4 
 1 file changed, 4 insertions(+)

diff --git a/m4/virt-libssh.m4 b/m4/virt-libssh.m4
index 6eac84cfe7..f2a5aed3cc 100644
--- a/m4/virt-libssh.m4
+++ b/m4/virt-libssh.m4
@@ -41,6 +41,10 @@ AC_DEFUN([LIBVIRT_CHECK_LIBSSH],[
 [AC_DEFINE([HAVE_SSH_KNOWN_HOSTS_E], [1],
   [Defined if enum ssh_known_hosts_e exists in libssh.h])],
 [], [[#include ]])
+AC_CHECK_FUNC([ssh_session_update_known_hosts],
+  [],
+  [AC_DEFINE_UNQUOTED([ssh_session_update_known_hosts], 
[ssh_write_knownhost],
+[ssh_write_knownhost is deprecated and replaced by 
ssh_session_update_known_hosts.])])
 CFLAGS="$old_CFLAGS"
 LIBS="$old_LIBS"
   fi
-- 
2.19.1

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

Re: [libvirt] [PATCHv2 07/16] qemu: Introduce virQEMUCapsTypeIsAccelerated

[Pino Toscano]

On Wednesday, 21 November 2018 15:01:50 CET Roman Bolshakov wrote:
> +static bool
> +virQEMUCapsTypeIsAccelerated(virDomainVirtType type)
> +{
> +return type == VIR_DOMAIN_VIRT_KVM;
> +}
> [...]
> @@ -4966,7 +4971,8 @@ virQEMUCapsCacheLookupDefault(virFileCachePtr cache,
>  if (virttype == VIR_DOMAIN_VIRT_NONE)
>  virttype = capsType;
>  
> -if (virttype == VIR_DOMAIN_VIRT_KVM && capsType == VIR_DOMAIN_VIRT_QEMU) 
> {
> +if (virQEMUCapsTypeIsAccelerated(virttype) &&
> +!virQEMUCapsTypeIsAccelerated(capsType)) {
>  virReportError(VIR_ERR_INVALID_ARG,
> _("KVM is not supported by '%s' on this host"),
> binary);

>From what I see, this check is now different:
- "capsType == VIR_DOMAIN_VIRT_QEMU" will be true only when capsType is
  VIR_DOMAIN_VIRT_QEMU
- !virQEMUCapsTypeIsAccelerated(capsType) will be true when capsType is
  not VIR_DOMAIN_VIRT_KVM

-- 
Pino Toscano

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

Re: [libvirt] [PATCHv2 01/16] qemu: Add KVM CPUs into cache only if KVM is present

[Roman Bolshakov]

On Wed, Nov 21, 2018 at 07:43:43PM +0100, Jiri Denemark wrote:
> On Wed, Nov 21, 2018 at 20:50:50 +0300, Roman Bolshakov wrote:
> > On Wed, Nov 21, 2018 at 05:04:07PM +0100, Jiri Denemark wrote:
> > > On Wed, Nov 21, 2018 at 17:01:44 +0300, Roman Bolshakov wrote:
> > > > diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> > > > index fde27010e4..4ba8369e3a 100644
> > > > --- a/src/qemu/qemu_capabilities.c
> > > > +++ b/src/qemu/qemu_capabilities.c
> > > > @@ -3467,11 +3467,13 @@ virQEMUCapsLoadCache(virArch hostArch,
> > > >  }
> > > >  VIR_FREE(str);
> > > >  
> > > > -if (virQEMUCapsLoadHostCPUModelInfo(qemuCaps, ctxt, 
> > > > VIR_DOMAIN_VIRT_KVM) < 0 ||
> > > > +if ((virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM) &&
> > > > + virQEMUCapsLoadHostCPUModelInfo(qemuCaps, ctxt, 
> > > > VIR_DOMAIN_VIRT_KVM) < 0) ||
> > > >  virQEMUCapsLoadHostCPUModelInfo(qemuCaps, ctxt, 
> > > > VIR_DOMAIN_VIRT_QEMU) < 0)
> > > >  goto cleanup;
> > > 
> > > I don't think we should introduce these guards in all the places. All
> > > the loading and formatting functions should return success if the
> > > appropriate info is not available, so you should just make sure the
> > > relevant info is NULL in qemuCaps.
> > 
> > Do you mean the capabilities checks should be moved inside the
> > functions?
> 
> virQEMUCapsLoadHostCPUModelInfo does (not literally, but effectively)
> 
> hostCPUNode = virXPathNode("./hostCPU[@type='kvm']", ctxt);
> if (!hostCPUNode)
> return 0;
> 
> virQEMUCapsLoadCPUModels does
> n = virXPathNodeSet("./cpu[@type='kvm']", ctxt, );
> if (n == 0)
> return 0;
> 

I agree, virQEMUCapsLoadHostCPUModelInfo and virQEMUCapsLoadCPUModels
don't need the check.

> virQEMUCapsInitHostCPUModel always fills in something and your check
> should probably remain in place for it
> 
> virQEMUCapsFormatHostCPUModelInfo does
> virQEMUCapsHostCPUDataPtr cpuData = >kvmCPU;
> qemuMonitorCPUModelInfoPtr model = cpuData->info;
> if (!model)
> return;
> 
> virQEMUCapsFormatCPUModels
> cpus = qemuCaps->kvmCPUModels;
> if (!cpus)
> return;
> 
> So to me it looks like all functions are ready to see NULL pointers and
> just do nothing if that's the case. Thus the only thing this patch
> should need to do is to make sure virQEMUCapsInitHostCPUModel does not
> set something non-NULL there.

Unfortunately, that won't work for the patch series. kvmCPUModels is renamed to
accelCPUModels and kvmCPU is renamed to accelCPU in PATCH 6.

So, virQEMUCapsFormatHostCPUModelInfo looks like:
if (virQEMUCapsTypeIsAccelerated(type))
cpuData = qemuCaps->accelCPU;
else
cpuData = qemuCaps->tcgCPU;

and virQEMUCapsFormatCPUModels looks like:
if (virQEMUCapsTypeIsAccelerated(type))
cpus = qemuCaps->accelCPUModels;
else
cpus = qemuCaps->tcgCPUModels;

Without the check we'd return CPUs for KVM domain on the platform that doesn't
support it.

Thank you,
Roman

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

[libvirt] [PATCH v2 4/4] qemuMigrationSrcConfirm: Don't remove domain config if confirm phase fails

[Michal Privoznik]

If migration is cancelled or confirm phase fails the domain
should be kept on the source even if VIR_MIGRATE_UNDEFINE_SOURCE
was requested.

Signed-off-by: Michal Privoznik 
Reviewed-by: Jiri Denemark 
---
 src/qemu/qemu_migration.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 28ec1f4d50..4c3abbb1e6 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -3044,7 +3044,7 @@ qemuMigrationSrcConfirm(virQEMUDriverPtr driver,
 
 qemuMigrationJobFinish(driver, vm);
 if (!virDomainObjIsActive(vm)) {
-if (flags & VIR_MIGRATE_UNDEFINE_SOURCE) {
+if (!cancelled && ret == 0 && flags & VIR_MIGRATE_UNDEFINE_SOURCE) {
 virDomainDeleteConfig(cfg->configDir, cfg->autostartDir, vm);
 vm->persistent = 0;
 }
-- 
2.18.1

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

[libvirt] [PATCH v2 1/4] qemuMigrationDstPrepareAny: Don't overwrite error in cleanup path

[Michal Privoznik]

There are several functions called in the cleanup path. Some of
them do report error (e.g. qemuDomainRemoveInactiveJob()) which
may result in overwriting an error reported earlier with some
less useful message.

Signed-off-by: Michal Privoznik 
Reviewed-by: Jiri Denemark 
---
 src/qemu/qemu_migration.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 67940330aa..317df4bed5 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -2282,6 +2282,7 @@ qemuMigrationDstPrepareAny(virQEMUDriverPtr driver,
 {
 virDomainObjPtr vm = NULL;
 virObjectEventPtr event = NULL;
+virErrorPtr origErr;
 int ret = -1;
 int dataFD[2] = { -1, -1 };
 qemuDomainObjPrivatePtr priv = NULL;
@@ -2595,6 +2596,7 @@ qemuMigrationDstPrepareAny(virQEMUDriverPtr driver,
 ret = 0;
 
  cleanup:
+virErrorPreserveLast();
 VIR_FREE(tlsAlias);
 qemuProcessIncomingDefFree(incoming);
 VIR_FREE(xmlout);
@@ -2618,6 +2620,7 @@ qemuMigrationDstPrepareAny(virQEMUDriverPtr driver,
 qemuMigrationCookieFree(mig);
 virObjectUnref(caps);
 virNWFilterUnlockFilterUpdates();
+virErrorRestore();
 return ret;
 
  stopjob:
-- 
2.18.1

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

[libvirt] [PATCH v2 0/4] qemu: Fix offline migration onto the same host

[Michal Privoznik]

v2 of:

https://www.redhat.com/archives/libvir-list/2018-November/msg00832.html

diff to v1:
- in 2/4 I'm passing @priv whenever possible
- only doing s/priv/NULL/ in 3/4 as suggested in review

Patches 1/4 and 4/4 are reviewed already (not pushed yet though).

Michal Prívozník (4):
  qemuMigrationDstPrepareAny: Don't overwrite error in cleanup path
  qemuMigrationEatCookie: Pass virDomainDef instead of virDomainObj
  qemuMigrationDstPrepareAny: Parse cookie before adding domain onto
list
  qemuMigrationSrcConfirm: Don't remove domain config if confirm phase
fails

 src/qemu/qemu_migration.c| 42 
 src/qemu/qemu_migration_cookie.c | 23 -
 src/qemu/qemu_migration_cookie.h |  4 ++-
 3 files changed, 41 insertions(+), 28 deletions(-)

-- 
2.18.1

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

[libvirt] [PATCH v2 3/4] qemuMigrationDstPrepareAny: Parse cookie before adding domain onto list

[Michal Privoznik]

There are some checks done when parsing a migration cookie. For
instance, one of the checks ensures that the domain is not being
migrated onto the same host. If that is the case, then we are in
big trouble because the @vm is the same domain object used by
source and it has some jobs sets and everything so recovering
from failed cookie parsing would be needlessly hard.

Signed-off-by: Michal Privoznik 
---
 src/qemu/qemu_migration.c | 25 ++---
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 0317a35246..28ec1f4d50 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -2395,6 +2395,20 @@ qemuMigrationDstPrepareAny(virQEMUDriverPtr driver,
 }
 }
 
+/* Parse cookie earlier than adding the domain onto the
+ * domain list. Parsing/validation may fail and there's no
+ * point in having the domain in the list at that point. */
+if (!(mig = qemuMigrationEatCookie(driver, *def, origname, NULL,
+   cookiein, cookieinlen,
+   QEMU_MIGRATION_COOKIE_LOCKSTATE |
+   QEMU_MIGRATION_COOKIE_NBD |
+   QEMU_MIGRATION_COOKIE_MEMORY_HOTPLUG |
+   QEMU_MIGRATION_COOKIE_CPU_HOTPLUG |
+   QEMU_MIGRATION_COOKIE_CPU |
+   QEMU_MIGRATION_COOKIE_ALLOW_REBOOT |
+   QEMU_MIGRATION_COOKIE_CAPS)))
+goto cleanup;
+
 if (!(vm = virDomainObjListAdd(driver->domains, *def,
driver->xmlopt,
VIR_DOMAIN_OBJ_LIST_ADD_LIVE |
@@ -2412,17 +2426,6 @@ qemuMigrationDstPrepareAny(virQEMUDriverPtr driver,
 priv->hookRun = true;
 }
 
-if (!(mig = qemuMigrationEatCookie(driver, vm->def, origname, priv,
-   cookiein, cookieinlen,
-   QEMU_MIGRATION_COOKIE_LOCKSTATE |
-   QEMU_MIGRATION_COOKIE_NBD |
-   QEMU_MIGRATION_COOKIE_MEMORY_HOTPLUG |
-   QEMU_MIGRATION_COOKIE_CPU_HOTPLUG |
-   QEMU_MIGRATION_COOKIE_CPU |
-   QEMU_MIGRATION_COOKIE_ALLOW_REBOOT |
-   QEMU_MIGRATION_COOKIE_CAPS)))
-goto cleanup;
-
 if (STREQ_NULLABLE(protocol, "rdma") &&
 !virMemoryLimitIsSet(vm->def->mem.hard_limit)) {
 virReportError(VIR_ERR_OPERATION_INVALID, "%s",
-- 
2.18.1

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

[libvirt] [PATCH v2 2/4] qemuMigrationEatCookie: Pass virDomainDef instead of virDomainObj

[Michal Privoznik]

The function currently takes virDomainObjPtr because it's using
both: the domain definition and domain private data.
Unfortunately, this means that in prepare phase we can't parse
migration cookie before putting incoming domain def onto domain
objects list (addressed in the very next commit). Change the
arguments so that virDomainDef and private data are passed
instead of virDomainObjPtr.

Signed-off-by: Michal Privoznik 
---
 src/qemu/qemu_migration.c| 16 ++--
 src/qemu/qemu_migration_cookie.c | 23 ---
 src/qemu/qemu_migration_cookie.h |  4 +++-
 3 files changed, 25 insertions(+), 18 deletions(-)

diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 317df4bed5..0317a35246 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -2041,7 +2041,8 @@ qemuMigrationSrcBeginPhase(virQEMUDriverPtr driver,
 if (!(flags & VIR_MIGRATE_OFFLINE))
 cookieFlags |= QEMU_MIGRATION_COOKIE_CAPS;
 
-if (!(mig = qemuMigrationEatCookie(driver, vm, NULL, 0, 0)))
+if (!(mig = qemuMigrationEatCookie(driver, vm->def,
+   priv->origname, priv, NULL, 0, 0)))
 goto cleanup;
 
 if (qemuMigrationBakeCookie(mig, driver, vm,
@@ -2411,7 +2412,8 @@ qemuMigrationDstPrepareAny(virQEMUDriverPtr driver,
 priv->hookRun = true;
 }
 
-if (!(mig = qemuMigrationEatCookie(driver, vm, cookiein, cookieinlen,
+if (!(mig = qemuMigrationEatCookie(driver, vm->def, origname, priv,
+   cookiein, cookieinlen,
QEMU_MIGRATION_COOKIE_LOCKSTATE |
QEMU_MIGRATION_COOKIE_NBD |
QEMU_MIGRATION_COOKIE_MEMORY_HOTPLUG |
@@ -2922,7 +2924,8 @@ qemuMigrationSrcConfirmPhase(virQEMUDriverPtr driver,
  ? QEMU_MIGRATION_PHASE_CONFIRM3
  : QEMU_MIGRATION_PHASE_CONFIRM3_CANCELLED);
 
-if (!(mig = qemuMigrationEatCookie(driver, vm, cookiein, cookieinlen,
+if (!(mig = qemuMigrationEatCookie(driver, vm->def, priv->origname, priv,
+   cookiein, cookieinlen,
QEMU_MIGRATION_COOKIE_STATS)))
 goto cleanup;
 
@@ -3427,7 +3430,8 @@ qemuMigrationSrcRun(virQEMUDriverPtr driver,
 }
 }
 
-mig = qemuMigrationEatCookie(driver, vm, cookiein, cookieinlen,
+mig = qemuMigrationEatCookie(driver, vm->def, priv->origname, priv,
+ cookiein, cookieinlen,
  cookieFlags |
  QEMU_MIGRATION_COOKIE_GRAPHICS |
  QEMU_MIGRATION_COOKIE_CAPS);
@@ -4948,8 +4952,8 @@ qemuMigrationDstFinish(virQEMUDriverPtr driver,
  * even though VIR_MIGRATE_PERSIST_DEST was not used. */
 cookie_flags |= QEMU_MIGRATION_COOKIE_PERSISTENT;
 
-if (!(mig = qemuMigrationEatCookie(driver, vm, cookiein,
-   cookieinlen, cookie_flags)))
+if (!(mig = qemuMigrationEatCookie(driver, vm->def, priv->origname, priv,
+   cookiein, cookieinlen, cookie_flags)))
 goto endjob;
 
 if (flags & VIR_MIGRATE_OFFLINE) {
diff --git a/src/qemu/qemu_migration_cookie.c b/src/qemu/qemu_migration_cookie.c
index 60df449d53..295e28ae30 100644
--- a/src/qemu/qemu_migration_cookie.c
+++ b/src/qemu/qemu_migration_cookie.c
@@ -279,22 +279,22 @@ qemuMigrationCookieNetworkAlloc(virQEMUDriverPtr driver 
ATTRIBUTE_UNUSED,
 
 
 static qemuMigrationCookiePtr
-qemuMigrationCookieNew(virDomainObjPtr dom)
+qemuMigrationCookieNew(const virDomainDef *def,
+   const char *origname)
 {
-qemuDomainObjPrivatePtr priv = dom->privateData;
 qemuMigrationCookiePtr mig = NULL;
 const char *name;
 
 if (VIR_ALLOC(mig) < 0)
 goto error;
 
-if (priv->origname)
-name = priv->origname;
+if (origname)
+name = origname;
 else
-name = dom->def->name;
+name = def->name;
 if (VIR_STRDUP(mig->name, name) < 0)
 goto error;
-memcpy(mig->uuid, dom->def->uuid, VIR_UUID_BUFLEN);
+memcpy(mig->uuid, def->uuid, VIR_UUID_BUFLEN);
 
 if (!(mig->localHostname = virGetHostname()))
 goto error;
@@ -1472,12 +1472,13 @@ qemuMigrationBakeCookie(qemuMigrationCookiePtr mig,
 
 qemuMigrationCookiePtr
 qemuMigrationEatCookie(virQEMUDriverPtr driver,
-   virDomainObjPtr dom,
+   const virDomainDef *def,
+   const char *origname,
+   qemuDomainObjPrivatePtr priv,
const char *cookiein,
int cookieinlen,
unsigned int flags)
 {
-qemuDomainObjPrivatePtr priv = dom->privateData;
 qemuMigrationCookiePtr mig = NULL;
 
 /* Parse & validate 

Re: [libvirt] [PATCH 3/4] qemuMigrationDstPrepareAny: Parse cookie before adding domain onto list

[Jiri Denemark]

On Thu, Nov 22, 2018 at 14:16:17 +0100, Michal Privoznik wrote:
> There are some checks done when parsing a migration cookie. For
> instance, one of the checks ensures that the domain is not being
> migrated onto the same host. If that is the case, then we are in
> big trouble because the @vm is the same domain object used by
> source and it has some jobs sets and everything so recovering
> from failed cookie parsing would be needlessly hard.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/qemu/qemu_migration.c | 25 ++---
>  1 file changed, 14 insertions(+), 11 deletions(-)
> 
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index 28d3a72e32..3875ea828f 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
> @@ -2395,6 +2395,20 @@ qemuMigrationDstPrepareAny(virQEMUDriverPtr driver,
>  }
>  }
>  

The change from priv to NULL should be done in this patch.

> +/* Parse cookie earlier than adding the domain onto the
> + * domain list. Parsing/validation may fail and there's no
> + * point in having the domain in the list at that point. */
> +if (!(mig = qemuMigrationEatCookie(driver, *def, origname, NULL,
> +   cookiein, cookieinlen,
> +   QEMU_MIGRATION_COOKIE_LOCKSTATE |
> +   QEMU_MIGRATION_COOKIE_NBD |
> +   QEMU_MIGRATION_COOKIE_MEMORY_HOTPLUG |
> +   QEMU_MIGRATION_COOKIE_CPU_HOTPLUG |
> +   QEMU_MIGRATION_COOKIE_CPU |
> +   QEMU_MIGRATION_COOKIE_ALLOW_REBOOT |
> +   QEMU_MIGRATION_COOKIE_CAPS)))
> +goto cleanup;
> +
>  if (!(vm = virDomainObjListAdd(driver->domains, *def,
> driver->xmlopt,
> VIR_DOMAIN_OBJ_LIST_ADD_LIVE |
> @@ -2412,17 +2426,6 @@ qemuMigrationDstPrepareAny(virQEMUDriverPtr driver,
>  priv->hookRun = true;
>  }
>  
> -if (!(mig = qemuMigrationEatCookie(driver, vm->def, origname, NULL,
> -   cookiein, cookieinlen,
> -   QEMU_MIGRATION_COOKIE_LOCKSTATE |
> -   QEMU_MIGRATION_COOKIE_NBD |
> -   QEMU_MIGRATION_COOKIE_MEMORY_HOTPLUG |
> -   QEMU_MIGRATION_COOKIE_CPU_HOTPLUG |
> -   QEMU_MIGRATION_COOKIE_CPU |
> -   QEMU_MIGRATION_COOKIE_ALLOW_REBOOT |
> -   QEMU_MIGRATION_COOKIE_CAPS)))
> -goto cleanup;
> -
>  if (STREQ_NULLABLE(protocol, "rdma") &&
>  !virMemoryLimitIsSet(vm->def->mem.hard_limit)) {
>  virReportError(VIR_ERR_OPERATION_INVALID, "%s",

Jirka

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

Re: [libvirt] [PATCH 2/4] qemuMigrationEatCookie: Pass virDomainDef instead of virDomainObj

[Jiri Denemark]

On Thu, Nov 22, 2018 at 14:16:16 +0100, Michal Privoznik wrote:
> The function currently takes virDomainObjPtr because it's using
> both: the domain definition and domain private data.
> Unfortunately, this means that in prepare phase we can't parse
> migration cookie before putting incoming domain def onto domain
> objects list (addressed in the very next commit). Change the
> arguments so that virDomainDef and private data are passed
> instead of virDomainObjPtr.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/qemu/qemu_migration.c| 16 ++--
>  src/qemu/qemu_migration_cookie.c | 23 ---
>  src/qemu/qemu_migration_cookie.h |  4 +++-
>  3 files changed, 25 insertions(+), 18 deletions(-)
> 
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index 317df4bed5..28d3a72e32 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
> @@ -2041,7 +2041,8 @@ qemuMigrationSrcBeginPhase(virQEMUDriverPtr driver,
>  if (!(flags & VIR_MIGRATE_OFFLINE))
>  cookieFlags |= QEMU_MIGRATION_COOKIE_CAPS;
>  
> -if (!(mig = qemuMigrationEatCookie(driver, vm, NULL, 0, 0)))
> +if (!(mig = qemuMigrationEatCookie(driver, vm->def,
> +   priv->origname, NULL, NULL, 0, 0)))

I think this patch should always pass priv instead of NULL. In the
following patch you can replace priv with NULL in the only place where
qemuMigrationEatCookie will be called when there's no priv pointer yet.

I know we only use priv inside qemuMigrationEatCookie when
QEMU_MIGRATION_COOKIE_STATS flag is set. But I think it would be safer
to pass priv everywhere anyway.

>  goto cleanup;
>  
>  if (qemuMigrationBakeCookie(mig, driver, vm,
...
> @@ -4948,8 +4952,8 @@ qemuMigrationDstFinish(virQEMUDriverPtr driver,

A little bit more lines of context:

   cookie_flags = QEMU_MIGRATION_COOKIE_NETWORK |
  QEMU_MIGRATION_COOKIE_STATS |
  QEMU_MIGRATION_COOKIE_NBD;
   /* Some older versions of libvirt always send persistent XML in the 
cookie
>   * even though VIR_MIGRATE_PERSIST_DEST was not used. */
>  cookie_flags |= QEMU_MIGRATION_COOKIE_PERSISTENT;
>  
> -if (!(mig = qemuMigrationEatCookie(driver, vm, cookiein,
> -   cookieinlen, cookie_flags)))
> +if (!(mig = qemuMigrationEatCookie(driver, vm->def, priv->origname, NULL,
> +   cookiein, cookieinlen, cookie_flags)))
>  goto endjob;

cookie_flags contains QEMU_MIGRATION_COOKIE_STATS at this point so
passing NULL would just crash libvirtd.

Jirka

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

Re: [libvirt] [PATCH 1/4] qemuMigrationDstPrepareAny: Don't overwrite error in cleanup path

[Jiri Denemark]

On Thu, Nov 22, 2018 at 14:16:15 +0100, Michal Privoznik wrote:
> There are several functions called in the cleanup path. Some of
> them do report error (e.g. qemuDomainRemoveInactiveJob()) which
> may result in overwriting an error reported earlier with some
> less useful message.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/qemu/qemu_migration.c | 3 +++
>  1 file changed, 3 insertions(+)

Reviewed-by: Jiri Denemark 

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

Re: [libvirt] [PATCH 4/4] qemuMigrationSrcConfirm: Don't remove domain config if confirm phase fails

[Jiri Denemark]

On Thu, Nov 22, 2018 at 14:16:18 +0100, Michal Privoznik wrote:
> If migration is cancelled or confirm phase fails the domain
> should be kept on the source even if VIR_MIGRATE_UNDEFINE_SOURCE
> was requested.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/qemu/qemu_migration.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index 3875ea828f..04bc55944f 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
> @@ -3044,7 +3044,7 @@ qemuMigrationSrcConfirm(virQEMUDriverPtr driver,
>  
>  qemuMigrationJobFinish(driver, vm);
>  if (!virDomainObjIsActive(vm)) {
> -if (flags & VIR_MIGRATE_UNDEFINE_SOURCE) {
> +if (!cancelled && ret >= 0 && flags & VIR_MIGRATE_UNDEFINE_SOURCE) {

The success is normally indicated by ret == 0. I don't think
qemuMigrationSrcConfirmPhase would ever return anything > 0.

>  virDomainDeleteConfig(cfg->configDir, cfg->autostartDir, vm);
>  vm->persistent = 0;
>  }

Reviewed-by: Jiri Denemark 

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

Re: [libvirt] [PATCH] virsh: Strip XML declaration when extracting CPU XMLs

[Jiri Denemark]

On Thu, Nov 22, 2018 at 17:39:16 +0100, Ján Tomko wrote:
> On Thu, Nov 22, 2018 at 04:46:33PM +0100, Jiri Denemark wrote:
> >Since commit v4.3.0-336-gc84726fbdd all
> >{hypervisor-,}cpu-{baseline,compare} commands use a generic
> >vshExtractCPUDefXMLs helper for extracting individual CPU definitions
> >from the provided input file. The helper wraps the input file in a
> > element so that several independent elements can be easily
> >parsed from the file. This works fine except when the file starts with
> >XML declaration () because the XML declaration
> >cannot be put inside any element. In fact it has to be at the very
> >beginning of the XML document without any preceding white space
> >characters. We can just simply skip the XML declaration.
> 
> What if someone specifies a doctype? O:)
> Also, does libvirt produce such files? I don't think we should bother
> doing extra work to undo the extra work done by the user.

Of course, we don't generate XML declarations, but I can imagine tools
formatting DOM into a file could just automatically output the XML
declaration unless you explicitly tell them not to do so. On the other
hand, nothing would output a doctype or  without an
explicit action.

Moreover, libvirt itself doesn't mind XML declarations and virsh before
4.4.0 didn't mind either.

I agree, it's probably a corner case and I was thinking about not fixing
it, but it turned out to be really easy thanks to the strict XML
specification.

> >https://bugzilla.redhat.com/show_bug.cgi?id=1595993
> 
> I only see a relation between the bug summary and this patch.
> There's no mention of the XML declaration there and no mention of the
> other issues mentioned there here in the patch.

Oops, the patch is supposed to fix another bug
https://bugzilla.redhat.com/show_bug.cgi?id=1592737

Jirka

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

Re: [libvirt] [jenkins-ci PATCH] Revert "Enable {media, tree}uri tests for libosinfo"

[Fabiano Fidêncio]

On Fri, 2018-11-23 at 10:14 +0100, Andrea Bolognani wrote:
> While a very good idea in theory, it turns out that running
> these tests on CI results in a lot of false positives due to
> issues on the remote side, which are of course entirely out
> of our control.
> 
> This reverts commit ad8cdcf7a1e0e293e3cea19f36187b943bb881dd.
> 
> Signed-off-by: Andrea Bolognani 

Reviewed-by: Fabiano Fidêncio 

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

[libvirt] [jenkins-ci PATCH] Revert "Enable {media, tree}uri tests for libosinfo"

[Andrea Bolognani]

While a very good idea in theory, it turns out that running
these tests on CI results in a lot of false positives due to
issues on the remote side, which are of course entirely out
of our control.

This reverts commit ad8cdcf7a1e0e293e3cea19f36187b943bb881dd.

Signed-off-by: Andrea Bolognani 
---
 guests/playbooks/build/projects/libosinfo.yml | 4 
 projects/libosinfo.yaml   | 3 ---
 2 files changed, 7 deletions(-)

diff --git a/guests/playbooks/build/projects/libosinfo.yml 
b/guests/playbooks/build/projects/libosinfo.yml
index dce1333..311a52b 100644
--- a/guests/playbooks/build/projects/libosinfo.yml
+++ b/guests/playbooks/build/projects/libosinfo.yml
@@ -9,10 +9,6 @@
 - include: '{{ playbook_base }}/jobs/autotools-build-job.yml'
 - include: '{{ playbook_base }}/jobs/autotools-syntax-check-job.yml'
 - include: '{{ playbook_base }}/jobs/autotools-check-job.yml'
-  vars:
-local_env: |
-  # Run tests that require network connectivity
-  export LIBOSINFO_NETWORK_TESTS=1
 - include: '{{ playbook_base }}/jobs/autotools-rpm-job.yml'
   vars:
 machines: '{{ rpm_machines }}'
diff --git a/projects/libosinfo.yaml b/projects/libosinfo.yaml
index bf9ea6b..0376d0c 100644
--- a/projects/libosinfo.yaml
+++ b/projects/libosinfo.yaml
@@ -12,9 +12,6 @@
   parent_jobs: 'libosinfo-build'
   - autotools-check-job:
   parent_jobs: 'libosinfo-syntax-check'
-  local_env: |
-# Run tests that require network connectivity
-export LIBOSINFO_NETWORK_TESTS=1
   - autotools-rpm-job:
   parent_jobs: 'libosinfo-check'
   machines: '{rpm_machines}'
-- 
2.19.1

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

[libvirt] [PATCH 07/18] virSecurityDACRestoreAllLabel: Restore more labels

[Michal Privoznik]

We are setting label on kernel, initrd, dtb and slic_table files.
But we never restored it.

Signed-off-by: Michal Privoznik 
---
 src/security/security_dac.c | 16 
 1 file changed, 16 insertions(+)

diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index 9b3069e60c..de12a1e351 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -1720,6 +1720,22 @@ virSecurityDACRestoreAllLabel(virSecurityManagerPtr mgr,
 virSecurityDACRestoreFileLabel(mgr, def->os.loader->nvram) < 0)
 rc = -1;
 
+if (def->os.kernel &&
+virSecurityDACRestoreFileLabel(mgr, def->os.kernel) < 0)
+rc = -1;
+
+if (def->os.initrd &&
+virSecurityDACRestoreFileLabel(mgr, def->os.initrd) < 0)
+rc = -1;
+
+if (def->os.dtb &&
+virSecurityDACRestoreFileLabel(mgr, def->os.dtb) < 0)
+rc = -1;
+
+if (def->os.slic_table &&
+virSecurityDACRestoreFileLabel(mgr, def->os.slic_table) < 0)
+rc = -1;
+
 return rc;
 }
 
-- 
2.18.1

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

[libvirt] [PATCH 15/18] virSecuritySELinuxRestoreAllLabel: Reorder device relabeling

[Michal Privoznik]

It helps whe trying to match calls with virSecuritySELinuxSetAllLabel
if the order in which devices are set/restored is the same in
both functions.

Signed-off-by: Michal Privoznik 
---
 src/security/security_selinux.c | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index 0cf8164265..553fc852db 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -2604,8 +2604,11 @@ virSecuritySELinuxRestoreAllLabel(virSecurityManagerPtr 
mgr,
 if (!secdef || !secdef->relabel || data->skipAllLabel)
 return 0;
 
-if (def->tpm) {
-if (virSecuritySELinuxRestoreTPMFileLabelInt(mgr, def, def->tpm) < 0)
+for (i = 0; i < def->ndisks; i++) {
+virDomainDiskDefPtr disk = def->disks[i];
+
+if (virSecuritySELinuxRestoreImageLabelInt(mgr, def, disk->src,
+   migrated) < 0)
 rc = -1;
 }
 
@@ -2627,11 +2630,8 @@ virSecuritySELinuxRestoreAllLabel(virSecurityManagerPtr 
mgr,
 return -1;
 }
 
-for (i = 0; i < def->ndisks; i++) {
-virDomainDiskDefPtr disk = def->disks[i];
-
-if (virSecuritySELinuxRestoreImageLabelInt(mgr, def, disk->src,
-   migrated) < 0)
+if (def->tpm) {
+if (virSecuritySELinuxRestoreTPMFileLabelInt(mgr, def, def->tpm) < 0)
 rc = -1;
 }
 
-- 
2.18.1

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

[libvirt] [PATCH 14/18] virSecuritySELinuxTransactionRun: Implement rollback

[Michal Privoznik]

When iterating over list of paths/disk sources to relabel it may
happen that the process fails at some point. In that case, for
the sake of keeping seclabel refcount (stored in XATTRs) in sync
with reality we have to perform rollback. However, if that fails
too the only thing we can do is warn user.

Signed-off-by: Michal Privoznik 
---
 src/security/security_selinux.c | 13 -
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index 290faba9d6..0cf8164265 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -276,7 +276,6 @@ virSecuritySELinuxTransactionRun(pid_t pid ATTRIBUTE_UNUSED,
 for (i = 0; i < list->nItems; i++) {
 virSecuritySELinuxContextItemPtr item = list->items[i];
 
-/* TODO Implement rollback */
 if (!item->restore) {
 rv = virSecuritySELinuxSetFileconHelper(list->manager,
 item->path,
@@ -293,6 +292,18 @@ virSecuritySELinuxTransactionRun(pid_t pid 
ATTRIBUTE_UNUSED,
 break;
 }
 
+for (; rv < 0 && i > 0; i--) {
+virSecuritySELinuxContextItemPtr item = list->items[i - 1];
+
+if (!item->restore) {
+virSecuritySELinuxRestoreFileLabel(list->manager,
+   item->path,
+   list->lock);
+} else {
+VIR_WARN("Ignoring failed restore attempt on %s", item->path);
+}
+}
+
 if (list->lock)
 virSecurityManagerMetadataUnlock(list->manager, );
 
-- 
2.18.1

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

[libvirt] [PATCH 11/18] security_selinux: Track if transaction is restore

[Michal Privoznik]

It is going to be important to know if the current transaction we
are running is a restore operation or set label operation.

Signed-off-by: Michal Privoznik 
---
 src/security/security_selinux.c | 36 +++--
 1 file changed, 25 insertions(+), 11 deletions(-)

diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index 95e9a1b0c7..715d9a428b 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -85,6 +85,7 @@ struct _virSecuritySELinuxContextItem {
 char *path;
 char *tcon;
 bool optional;
+bool restore;
 };
 
 typedef struct _virSecuritySELinuxContextList virSecuritySELinuxContextList;
@@ -123,7 +124,8 @@ static int
 virSecuritySELinuxContextListAppend(virSecuritySELinuxContextListPtr list,
 const char *path,
 const char *tcon,
-bool optional)
+bool optional,
+bool restore)
 {
 int ret = -1;
 virSecuritySELinuxContextItemPtr item = NULL;
@@ -135,6 +137,7 @@ 
virSecuritySELinuxContextListAppend(virSecuritySELinuxContextListPtr list,
 goto cleanup;
 
 item->optional = optional;
+item->restore = restore;
 
 if (VIR_APPEND_ELEMENT(list->items, list->nItems, item) < 0)
 goto cleanup;
@@ -178,7 +181,8 @@ virSecuritySELinuxContextListFree(void *opaque)
 static int
 virSecuritySELinuxTransactionAppend(const char *path,
 const char *tcon,
-bool optional)
+bool optional,
+bool restore)
 {
 virSecuritySELinuxContextListPtr list;
 
@@ -186,7 +190,7 @@ virSecuritySELinuxTransactionAppend(const char *path,
 if (!list)
 return 0;
 
-if (virSecuritySELinuxContextListAppend(list, path, tcon, optional) < 0)
+if (virSecuritySELinuxContextListAppend(list, path, tcon, optional, 
restore) < 0)
 return -1;
 
 return 1;
@@ -198,6 +202,11 @@ static int virSecuritySELinuxSetFileconHelper(const char 
*path,
   bool optional,
   bool privileged);
 
+
+static int virSecuritySELinuxRestoreFileLabel(virSecurityManagerPtr mgr,
+  const char *path);
+
+
 /**
  * virSecuritySELinuxTransactionRun:
  * @pid: process pid
@@ -242,13 +251,18 @@ virSecuritySELinuxTransactionRun(pid_t pid 
ATTRIBUTE_UNUSED,
 virSecuritySELinuxContextItemPtr item = list->items[i];
 
 /* TODO Implement rollback */
-if (virSecuritySELinuxSetFileconHelper(item->path,
-   item->tcon,
-   item->optional,
-   privileged) < 0) {
-rv = -1;
-break;
+if (!item->restore) {
+rv = virSecuritySELinuxSetFileconHelper(item->path,
+item->tcon,
+item->optional,
+privileged);
+} else {
+rv = virSecuritySELinuxRestoreFileLabel(list->manager,
+item->path);
 }
+
+if (rv < 0)
+break;
 }
 
 if (list->lock)
@@ -1265,7 +1279,7 @@ virSecuritySELinuxSetFileconHelper(const char *path, 
const char *tcon,
 {
 int rc;
 
-if ((rc = virSecuritySELinuxTransactionAppend(path, tcon, optional)) < 0)
+if ((rc = virSecuritySELinuxTransactionAppend(path, tcon, optional, 
false)) < 0)
 return -1;
 else if (rc > 0)
 return 0;
@@ -1387,7 +1401,7 @@ virSecuritySELinuxRestoreFileLabel(virSecurityManagerPtr 
mgr,
 goto cleanup;
 }
 
-if ((rc = virSecuritySELinuxTransactionAppend(path, fcon, false)) < 0)
+if ((rc = virSecuritySELinuxTransactionAppend(path, fcon, false, true)) < 
0)
 return -1;
 else if (rc > 0)
 return 0;
-- 
2.18.1

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

[libvirt] [PATCH 10/18] virSecurityDACRestoreImageLabelInt: Restore even shared/RO disks

[Michal Privoznik]

Now that we have seclabel remembering we can safely restore
labels for shared and RO disks. In fact we need to do that to
keep seclabel refcount stored in XATTRs in sync with reality.

Signed-off-by: Michal Privoznik 
---
 src/security/security_dac.c | 8 
 1 file changed, 8 deletions(-)

diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index 9d31faa9d4..60adfaf526 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -921,14 +921,6 @@ virSecurityDACRestoreImageLabelInt(virSecurityManagerPtr 
mgr,
 if (!priv->dynamicOwnership)
 return 0;
 
-/* Don't restore labels on readoly/shared disks, because other VMs may
- * still be accessing these. Alternatively we could iterate over all
- * running domains and try to figure out if it is in use, but this would
- * not work for clustered filesystems, since we can't see running VMs using
- * the file on other nodes. Safest bet is thus to skip the restore step. */
-if (src->readonly || src->shared)
-return 0;
-
 secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME);
 if (secdef && !secdef->relabel)
 return 0;
-- 
2.18.1

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

[libvirt] [PATCH 08/18] security_dac: Allow callers to enable/disable label remembering/recall

[Michal Privoznik]

Because the implementation that will be used for label
remembering/recall is not atomic we have to give callers a chance
to enable or disable it. That is, enable it if and only if
metadata locking is enabled. Otherwise the feature MUST be turned
off.

Signed-off-by: Michal Privoznik 
---
 src/security/security_dac.c | 74 ++---
 1 file changed, 45 insertions(+), 29 deletions(-)

diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index de12a1e351..cdbe07543c 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -182,11 +182,13 @@ static int 
virSecurityDACSetOwnership(virSecurityManagerPtr mgr,
   const virStorageSource *src,
   const char *path,
   uid_t uid,
-  gid_t gid);
+  gid_t gid,
+  bool remember);
 
 static int virSecurityDACRestoreFileLabelInternal(virSecurityManagerPtr mgr,
   const virStorageSource *src,
-  const char *path);
+  const char *path,
+  bool recall);
 /**
  * virSecurityDACTransactionRun:
  * @pid: process pid
@@ -234,11 +236,13 @@ virSecurityDACTransactionRun(pid_t pid ATTRIBUTE_UNUSED,
 item->src,
 item->path,
 item->uid,
-item->gid);
+item->gid,
+list->lock);
 } else {
 rv = virSecurityDACRestoreFileLabelInternal(list->manager,
 item->src,
-item->path);
+item->path,
+list->lock);
 }
 
 if (rv < 0)
@@ -251,7 +255,8 @@ virSecurityDACTransactionRun(pid_t pid ATTRIBUTE_UNUSED,
 if (!item->restore) {
 virSecurityDACRestoreFileLabelInternal(list->manager,
item->src,
-   item->path);
+   item->path,
+   list->lock);
 } else {
 VIR_WARN("Ignoring failed restore attempt on %s",
  NULLSTR(item->src ? item->src->path : item->path));
@@ -699,7 +704,8 @@ virSecurityDACSetOwnership(virSecurityManagerPtr mgr,
const virStorageSource *src,
const char *path,
uid_t uid,
-   gid_t gid)
+   gid_t gid,
+   bool remember)
 {
 virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr);
 struct stat sb;
@@ -717,7 +723,7 @@ virSecurityDACSetOwnership(virSecurityManagerPtr mgr,
 else if (rc > 0)
 return 0;
 
-if (path) {
+if (remember && path) {
 if (stat(path, ) < 0) {
 virReportSystemError(errno, _("unable to stat: %s"), path);
 return -1;
@@ -739,7 +745,7 @@ virSecurityDACSetOwnership(virSecurityManagerPtr mgr,
  * this function. However, if our attempt fails, there's
  * not much we can do. XATTRs refcounting is fubar'ed and
  * the only option we have is warn users. */
-if (virSecurityDACRestoreFileLabelInternal(mgr, src, path) < 0)
+if (virSecurityDACRestoreFileLabelInternal(mgr, src, path, remember) < 
0)
 VIR_WARN("Unable to restore label on '%s'. "
  "XATTRs might have been left in inconsistent state.",
  NULLSTR(src ? src->path : path));
@@ -755,7 +761,8 @@ virSecurityDACSetOwnership(virSecurityManagerPtr mgr,
 static int
 virSecurityDACRestoreFileLabelInternal(virSecurityManagerPtr mgr,
const virStorageSource *src,
-   const char *path)
+   const char *path,
+   bool recall)
 {
 virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr);
 int rv;
@@ -774,7 +781,7 @@ 
virSecurityDACRestoreFileLabelInternal(virSecurityManagerPtr mgr,
 else if (rv > 0)
 return 0;
 
-if (path) {
+if (recall && path) {
 rv = virSecurityDACRecallLabel(priv, path, , );
 if (rv < 0)
 return -1;
@@ -793,7 +800,7 @@ static int
 

[libvirt] [PATCH 16/18] virSecuritySELinuxRestoreAllLabel: Restore more labels

[Michal Privoznik]

We are setting label on kernel, initrd, dtb and slic_table files.
But we never restored it.

Signed-off-by: Michal Privoznik 
---
 src/security/security_selinux.c | 16 
 1 file changed, 16 insertions(+)

diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index 553fc852db..5f2fab73bc 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -2656,6 +2656,22 @@ virSecuritySELinuxRestoreAllLabel(virSecurityManagerPtr 
mgr,
 virSecuritySELinuxRestoreFileLabel(mgr, def->os.loader->nvram, false) 
< 0)
 rc = -1;
 
+if (def->os.kernel &&
+virSecuritySELinuxRestoreFileLabel(mgr, def->os.kernel, false) < 0)
+rc = -1;
+
+if (def->os.initrd &&
+virSecuritySELinuxRestoreFileLabel(mgr, def->os.initrd, false) < 0)
+rc = -1;
+
+if (def->os.dtb &&
+virSecuritySELinuxRestoreFileLabel(mgr, def->os.dtb, false) < 0)
+rc = -1;
+
+if (def->os.slic_table &&
+virSecuritySELinuxRestoreFileLabel(mgr, def->os.slic_table, false) < 0)
+rc = -1;
+
 return rc;
 }
 
-- 
2.18.1

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

[libvirt] [PATCH 03/18] security: Include security_util

[Michal Privoznik]

Signed-off-by: Michal Privoznik 
---
 src/security/Makefile.inc.am |   2 +
 src/security/security_util.c | 198 +++
 src/security/security_util.h |  32 ++
 3 files changed, 232 insertions(+)
 create mode 100644 src/security/security_util.c
 create mode 100644 src/security/security_util.h

diff --git a/src/security/Makefile.inc.am b/src/security/Makefile.inc.am
index f88b82df7b..0ade97d355 100644
--- a/src/security/Makefile.inc.am
+++ b/src/security/Makefile.inc.am
@@ -14,6 +14,8 @@ SECURITY_DRIVER_SOURCES = \
security/security_dac.c \
security/security_manager.h \
security/security_manager.c \
+   security/security_util.h \
+   security/security_util.c \
$(NULL)
 
 SECURITY_DRIVER_SELINUX_SOURCES = \
diff --git a/src/security/security_util.c b/src/security/security_util.c
new file mode 100644
index 00..4178fdff81
--- /dev/null
+++ b/src/security/security_util.c
@@ -0,0 +1,198 @@
+/*
+ * Copyright (C) 2018 Red Hat, Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library.  If not, see
+ * .
+ */
+
+#include 
+
+#include "viralloc.h"
+#include "virfile.h"
+#include "virstring.h"
+
+#include "security_util.h"
+
+#define VIR_FROM_THIS VIR_FROM_SECURITY
+
+/* There are four namespaces available (xattr(7)):
+ *
+ *  user - can be modified by anybody,
+ *  system - used by ACLs
+ *  security - used by SELinux
+ *  trusted - accessibly by CAP_SYS_ADMIN processes only
+ *
+ * Looks like the last one is way to go.
+ */
+#define XATTR_NAMESPACE "trusted"
+
+static char *
+virSecurityGetAttrName(const char *name)
+{
+char *ret;
+ignore_value(virAsprintf(, XATTR_NAMESPACE".libvirt.security.%s", 
name));
+return ret;
+}
+
+
+static char *
+virSecurityGetRefCountAttrName(const char *name)
+{
+char *ret;
+ignore_value(virAsprintf(, XATTR_NAMESPACE".libvirt.security.ref_%s", 
name));
+return ret;
+}
+
+
+/**
+ * virSecurityGetRememberedLabel:
+ * @name: security driver name
+ * @path: file name
+ * @label: label
+ *
+ * For given @path and security driver (@name) fetch remembered
+ * @label. The caller must not restore label if an error is
+ * indicated or if @label is NULL upon return.
+ *
+ * Returns: 0 on success,
+ * -1 otherwise (with error reported)
+ */
+int
+virSecurityGetRememberedLabel(const char *name,
+  const char *path,
+  char **label)
+{
+char *ref_name = NULL;
+char *attr_name = NULL;
+char *value = NULL;
+unsigned int refcount = 0;
+int ret = -1;
+
+*label = NULL;
+
+if (!(ref_name = virSecurityGetRefCountAttrName(name)))
+goto cleanup;
+
+if (virFileGetXAtrr(path, ref_name, ) < 0) {
+if (errno == ENOSYS || errno == ENODATA || errno == ENOTSUP) {
+ret = 0;
+} else {
+virReportSystemError(errno,
+ _("Unable to get XATTR %s on %s"),
+ ref_name,
+ path);
+}
+goto cleanup;
+}
+
+if (virStrToLong_ui(value, NULL, 10, ) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("malformed refcount %s on %s"),
+   value, path);
+goto cleanup;
+}
+
+VIR_FREE(value);
+
+refcount--;
+
+if (refcount > 0) {
+if (virAsprintf(, "%u", refcount) < 0)
+goto cleanup;
+
+if (virFileSetXAtrr(path, ref_name, value) < 0)
+goto cleanup;
+} else {
+if (virFileRemoveXAttr(path, ref_name) < 0)
+goto cleanup;
+
+if (!(attr_name = virSecurityGetAttrName(name)))
+goto cleanup;
+
+if (virFileGetXAtrr(path, attr_name, label) < 0)
+goto cleanup;
+
+if (virFileRemoveXAttr(path, attr_name) < 0)
+goto cleanup;
+}
+
+ret = 0;
+ cleanup:
+VIR_FREE(value);
+VIR_FREE(attr_name);
+VIR_FREE(ref_name);
+return ret;
+}
+
+
+int
+virSecuritySetRememberedLabel(const char *name,
+  const char *path,
+  const char *label)
+{
+char *ref_name = NULL;
+char *attr_name = NULL;
+char *value = NULL;
+unsigned int refcount = 0;
+int ret = -1;
+
+if (!(ref_name = virSecurityGetRefCountAttrName(name)))

[libvirt] [PATCH 04/18] security_dac: Restore label on failed chown() attempt

[Michal Privoznik]

It's important to keep XATTRs untouched (well, in the same state
they were in when entering the function). Otherwise our
refcounting would be messed up.

Signed-off-by: Michal Privoznik 
---
 src/security/security_dac.c | 20 +++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index 6b64d2c07a..8155c6d58a 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -718,7 +718,25 @@ virSecurityDACSetOwnership(virSecurityManagerPtr mgr,
 VIR_INFO("Setting DAC user and group on '%s' to '%ld:%ld'",
  NULLSTR(src ? src->path : path), (long)uid, (long)gid);
 
-return virSecurityDACSetOwnershipInternal(priv, src, path, uid, gid);
+if (virSecurityDACSetOwnershipInternal(priv, src, path, uid, gid) < 0) {
+virErrorPtr origerr;
+
+virErrorPreserveLast();
+/* Try to restore the label. This is done so that XATTRs
+ * are left in the same state as when the control entered
+ * this function. However, if our attempt fails, there's
+ * not much we can do. XATTRs refcounting is fubar'ed and
+ * the only option we have is warn users. */
+if (virSecurityDACRestoreFileLabelInternal(mgr, src, path) < 0)
+VIR_WARN("Unable to restore label on '%s'. "
+ "XATTRs might have been left in inconsistent state.",
+ NULLSTR(src ? src->path : path));
+
+virErrorRestore();
+return -1;
+}
+
+return 0;
 }
 
 
-- 
2.18.1

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

[libvirt] [PATCH 12/18] security_selinux: Remember old labels

[Michal Privoznik]

Signed-off-by: Michal Privoznik 
---
 src/security/security_selinux.c | 161 ++--
 1 file changed, 114 insertions(+), 47 deletions(-)

diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index 715d9a428b..4990d94b5f 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -33,6 +33,7 @@
 
 #include "security_driver.h"
 #include "security_selinux.h"
+#include "security_util.h"
 #include "virerror.h"
 #include "viralloc.h"
 #include "virlog.h"
@@ -197,14 +198,40 @@ virSecuritySELinuxTransactionAppend(const char *path,
 }
 
 
+static int
+virSecuritySELinuxRememberLabel(const char *path,
+const security_context_t con)
+{
+return virSecuritySetRememberedLabel(SECURITY_SELINUX_NAME,
+ path, con);
+}
+
+
+static int
+virSecuritySELinuxRecallLabel(const char *path,
+  security_context_t *con)
+{
+if (virSecurityGetRememberedLabel(SECURITY_SELINUX_NAME,
+  path, con) < 0)
+return -1;
+
+if (!con)
+return 1;
+
+return 0;
+}
+
+
 static int virSecuritySELinuxSetFileconHelper(const char *path,
   const char *tcon,
   bool optional,
-  bool privileged);
+  bool privileged,
+  bool remember);
 
 
 static int virSecuritySELinuxRestoreFileLabel(virSecurityManagerPtr mgr,
-  const char *path);
+  const char *path,
+  bool recall);
 
 
 /**
@@ -255,10 +282,12 @@ virSecuritySELinuxTransactionRun(pid_t pid 
ATTRIBUTE_UNUSED,
 rv = virSecuritySELinuxSetFileconHelper(item->path,
 item->tcon,
 item->optional,
-privileged);
+privileged,
+list->lock);
 } else {
 rv = virSecuritySELinuxRestoreFileLabel(list->manager,
-item->path);
+item->path,
+list->lock);
 }
 
 if (rv < 0)
@@ -1275,16 +1304,38 @@ virSecuritySELinuxSetFileconImpl(const char *path, 
const char *tcon,
 
 static int
 virSecuritySELinuxSetFileconHelper(const char *path, const char *tcon,
-   bool optional, bool privileged)
+   bool optional, bool privileged, bool 
remember)
 {
+security_context_t econ = NULL;
 int rc;
+int ret = -1;
 
 if ((rc = virSecuritySELinuxTransactionAppend(path, tcon, optional, 
false)) < 0)
 return -1;
 else if (rc > 0)
 return 0;
 
-return virSecuritySELinuxSetFileconImpl(path, tcon, optional, privileged);
+if (remember) {
+if (getfilecon_raw(path, ) < 0 &&
+errno != ENOTSUP && errno != ENODATA) {
+virReportSystemError(errno,
+ _("unable to get SELinux context of %s"),
+ path);
+goto cleanup;
+}
+
+if (econ &&
+virSecuritySELinuxRememberLabel(path, econ) < 0)
+goto cleanup;
+}
+
+if (virSecuritySELinuxSetFileconImpl(path, tcon, optional, privileged) < 0)
+goto cleanup;
+
+ret = 0;
+ cleanup:
+freecon(econ);
+return ret;
 }
 
 
@@ -1293,7 +1344,7 @@ 
virSecuritySELinuxSetFileconOptional(virSecurityManagerPtr mgr,
  const char *path, const char *tcon)
 {
 bool privileged = virSecurityManagerGetPrivileged(mgr);
-return virSecuritySELinuxSetFileconHelper(path, tcon, true, privileged);
+return virSecuritySELinuxSetFileconHelper(path, tcon, true, privileged, 
false);
 }
 
 static int
@@ -1301,7 +1352,7 @@ virSecuritySELinuxSetFilecon(virSecurityManagerPtr mgr,
  const char *path, const char *tcon)
 {
 bool privileged = virSecurityManagerGetPrivileged(mgr);
-return virSecuritySELinuxSetFileconHelper(path, tcon, false, privileged);
+return virSecuritySELinuxSetFileconHelper(path, tcon, false, privileged, 
false);
 }
 
 static int
@@ -1362,7 +1413,8 @@ getContext(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED,
  * errors that the caller(s) are already dealing with */
 static int
 virSecuritySELinuxRestoreFileLabel(virSecurityManagerPtr mgr,
-   const char *path)
+   const char 

[libvirt] [PATCH 17/18] tools: Provide a script to recover fubar'ed XATTRs setup

[Michal Privoznik]

Our code is not bug free. The refcounting I introduced will
almost certainly not work in some use cases. Provide a script
that will remove all the XATTRs set by libvirt so that it can
start cleanly.

Signed-off-by: Michal Privoznik 
---
 tools/Makefile.am   |  1 +
 tools/libvirt_recover_xattrs.sh | 89 +
 2 files changed, 90 insertions(+)
 create mode 100755 tools/libvirt_recover_xattrs.sh

diff --git a/tools/Makefile.am b/tools/Makefile.am
index f069167acc..1dc009c4fb 100644
--- a/tools/Makefile.am
+++ b/tools/Makefile.am
@@ -75,6 +75,7 @@ EXTRA_DIST = \
virt-login-shell.conf \
virsh-edit.c \
bash-completion/vsh \
+   libvirt_recover_xattrs.sh \
$(PODFILES) \
$(MANINFILES) \
$(NULL)
diff --git a/tools/libvirt_recover_xattrs.sh b/tools/libvirt_recover_xattrs.sh
new file mode 100755
index 00..c4a8b27cbc
--- /dev/null
+++ b/tools/libvirt_recover_xattrs.sh
@@ -0,0 +1,89 @@
+#!/bin/bash
+
+function die {
+echo $@ >&2
+exit 1
+}
+
+function show_help {
+cat << EOF
+Usage: ${0##*/} -[hqn] [PATH]
+
+Clear out any XATTRs set by libvirt on all files that have them.
+The idea is to reset refcounting, should it break.
+
+  -hdisplay this help and exit
+  -qquiet (don't print which files are being fixed)
+  -ndry run; don't remove any XATTR just report the file name
+
+PATH can be specified to refine search to only to given path
+instead of whole root ('/'), which is the default.
+EOF
+}
+
+QUIET=0
+DRY_RUN=0
+P="/"
+
+# So far only qemu and lxc drivers use security driver.
+URI=("qemu:///system"
+ "qemu:///session"
+ "lxc:///system")
+
+LIBVIRT_XATTR_PREFIX="trusted.libvirt.security"
+
+if [ `whoami` != "root" ]; then
+die "Must be run as root"
+fi
+
+while getopts hqn opt; do
+case $opt in
+h)
+show_help
+exit 0
+;;
+q)
+QUIET=1
+;;
+n)
+DRY_RUN=1
+;;
+*)
+show_help >&2
+exit 1
+;;
+esac
+done
+
+shift $((OPTIND - 1))
+if [ $# -gt 0 ]; then
+P=$1
+fi
+
+if [ ${DRY_RUN} -eq 0 ]; then
+for u in ${URI[*]} ; do
+if [ -n "`virsh -q -c $u list 2>/dev/null`" ]; then
+die "There are still some domains running for $u"
+fi
+done
+fi
+
+XATTRS=("trusted.libvirt.security.dac"
+"trusted.libvirt.security.ref_dac"
+"trusted.libvirt.security.selinux"
+"trusted.libvirt.security.ref_selinux")
+
+for i in $(getfattr -R -d -m ${LIBVIRT_XATTR_PREFIX} --absolute-names ${P} 
2>/dev/null | grep "^# file:" | cut -d':' -f 2); do
+if [ ${DRY_RUN} -ne 0 ]; then
+echo $i
+getfattr -d -m ${LIBVIRT_XATTR_PREFIX} $i
+continue
+fi
+
+if [ ${QUIET} -eq 0 ]; then
+echo "Fixing $i";
+fi
+for x in ${XATTRS[*]}; do
+setfattr -x $x $i
+done
+done
-- 
2.18.1

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

[libvirt] [PATCH 05/18] virSecurityDACTransactionRun: Implement rollback

[Michal Privoznik]

When iterating over list of paths/disk sources to relabel it may
happen that the process fails at some point. In that case, for
the sake of keeping seclabel refcount (stored in XATTRs) in sync
with reality we have to perform rollback. However, if that fails
too the only thing we can do is warn user.

Signed-off-by: Michal Privoznik 
---
 src/security/security_dac.c | 14 +-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index 8155c6d58a..82b16f96ee 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -229,7 +229,6 @@ virSecurityDACTransactionRun(pid_t pid ATTRIBUTE_UNUSED,
 for (i = 0; i < list->nItems; i++) {
 virSecurityDACChownItemPtr item = list->items[i];
 
-/* TODO Implement rollback */
 if (!item->restore) {
 rv = virSecurityDACSetOwnership(list->manager,
 item->src,
@@ -246,6 +245,19 @@ virSecurityDACTransactionRun(pid_t pid ATTRIBUTE_UNUSED,
 break;
 }
 
+for (; rv < 0 && i > 0; i--) {
+virSecurityDACChownItemPtr item = list->items[i - 1];
+
+if (!item->restore) {
+virSecurityDACRestoreFileLabelInternal(list->manager,
+   item->src,
+   item->path);
+} else {
+VIR_WARN("Ignoring failed restore attempt on %s",
+ NULLSTR(item->src ? item->src->path : item->path));
+}
+}
+
 if (list->lock)
 virSecurityManagerMetadataUnlock(list->manager, );
 
-- 
2.18.1

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

[libvirt] [PATCH 18/18] qemu.conf: Allow users to enable/disable label remembering

[Michal Privoznik]

Signed-off-by: Michal Privoznik 
---
 src/qemu/libvirtd_qemu.aug | 1 +
 src/qemu/qemu.conf | 6 ++
 src/qemu/qemu_conf.c   | 4 
 src/qemu/test_libvirtd_qemu.aug.in | 1 +
 4 files changed, 12 insertions(+)

diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug
index ddc4bbfd1d..8a5b39e568 100644
--- a/src/qemu/libvirtd_qemu.aug
+++ b/src/qemu/libvirtd_qemu.aug
@@ -71,6 +71,7 @@ module Libvirtd_qemu =
  | str_entry "user"
  | str_entry "group"
  | bool_entry "dynamic_ownership"
+ | bool_entry "remember_owner"
  | str_array_entry "cgroup_controllers"
  | str_array_entry "cgroup_device_acl"
  | int_entry "seccomp_sandbox"
diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf
index 8391332cb4..31e8d8476b 100644
--- a/src/qemu/qemu.conf
+++ b/src/qemu/qemu.conf
@@ -450,6 +450,12 @@
 # Set to 0 to disable file ownership changes.
 #dynamic_ownership = 1
 
+# Whether libvirt should remember and restore the original
+# ownership over files it is relabeling. Be aware that with the
+# current implementation this requires exclusive access to the
+# files which might hurt performance a bit in some cases.
+# Defaults to 1, set to 0 to disable the feature.
+#remember_owner = 1
 
 # What cgroup controllers to make use of with QEMU guests
 #
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index a946b05d5d..89491a37b7 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -147,6 +147,7 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool 
privileged)
 cfg->group = (gid_t)-1;
 }
 cfg->dynamicOwnership = privileged;
+cfg->rememberOwner = true;
 
 cfg->cgroupControllers = -1; /* -1 == auto-detect */
 
@@ -730,6 +731,9 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg,
 if (virConfGetValueBool(conf, "dynamic_ownership", >dynamicOwnership) 
< 0)
 goto cleanup;
 
+if (virConfGetValueBool(conf, "remember_owner", >rememberOwner) < 0)
+goto cleanup;
+
 if (virConfGetValueStringList(conf,  "cgroup_controllers", false,
   ) < 0)
 goto cleanup;
diff --git a/src/qemu/test_libvirtd_qemu.aug.in 
b/src/qemu/test_libvirtd_qemu.aug.in
index f1e8806ad2..92a8ae1192 100644
--- a/src/qemu/test_libvirtd_qemu.aug.in
+++ b/src/qemu/test_libvirtd_qemu.aug.in
@@ -43,6 +43,7 @@ module Test_libvirtd_qemu =
 { "user" = "root" }
 { "group" = "root" }
 { "dynamic_ownership" = "1" }
+{ "remember_owner" = "1" }
 { "cgroup_controllers"
 { "1" = "cpu" }
 { "2" = "devices" }
-- 
2.18.1

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

[libvirt] [PATCH 13/18] security_selinux: Restore label on failed setfilecon() attempt

[Michal Privoznik]

It's important to keep XATTRs untouched (well, in the same state
they were in when entering the function). Otherwise our
refcounting would be messed up.

Signed-off-by: Michal Privoznik 
---
 src/security/security_selinux.c | 40 +++--
 1 file changed, 28 insertions(+), 12 deletions(-)

diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index 4990d94b5f..290faba9d6 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -222,10 +222,10 @@ virSecuritySELinuxRecallLabel(const char *path,
 }
 
 
-static int virSecuritySELinuxSetFileconHelper(const char *path,
+static int virSecuritySELinuxSetFileconHelper(virSecurityManagerPtr mgr,
+  const char *path,
   const char *tcon,
   bool optional,
-  bool privileged,
   bool remember);
 
 
@@ -252,7 +252,6 @@ virSecuritySELinuxTransactionRun(pid_t pid ATTRIBUTE_UNUSED,
 {
 virSecuritySELinuxContextListPtr list = opaque;
 virSecurityManagerMetadataLockStatePtr state;
-bool privileged = virSecurityManagerGetPrivileged(list->manager);
 const char **paths = NULL;
 size_t npaths = 0;
 size_t i;
@@ -279,10 +278,10 @@ virSecuritySELinuxTransactionRun(pid_t pid 
ATTRIBUTE_UNUSED,
 
 /* TODO Implement rollback */
 if (!item->restore) {
-rv = virSecuritySELinuxSetFileconHelper(item->path,
+rv = virSecuritySELinuxSetFileconHelper(list->manager,
+item->path,
 item->tcon,
 item->optional,
-privileged,
 list->lock);
 } else {
 rv = virSecuritySELinuxRestoreFileLabel(list->manager,
@@ -1303,9 +1302,13 @@ virSecuritySELinuxSetFileconImpl(const char *path, const 
char *tcon,
 
 
 static int
-virSecuritySELinuxSetFileconHelper(const char *path, const char *tcon,
-   bool optional, bool privileged, bool 
remember)
+virSecuritySELinuxSetFileconHelper(virSecurityManagerPtr mgr,
+   const char *path,
+   const char *tcon,
+   bool optional,
+   bool remember)
 {
+bool privileged = virSecurityManagerGetPrivileged(mgr);
 security_context_t econ = NULL;
 int rc;
 int ret = -1;
@@ -1329,8 +1332,23 @@ virSecuritySELinuxSetFileconHelper(const char *path, 
const char *tcon,
 goto cleanup;
 }
 
-if (virSecuritySELinuxSetFileconImpl(path, tcon, optional, privileged) < 0)
+if (virSecuritySELinuxSetFileconImpl(path, tcon, optional, privileged) < 
0) {
+virErrorPtr origerr;
+
+virErrorPreserveLast();
+/* Try to restore the label. This is done so that XATTRs
+ * are left in the same state as when the control entered
+ * this function. However, if our attempt fails, there's
+ * not much we can do. XATTRs refcounting is fubar'ed and
+ * the only option we have is warn users. */
+if (virSecuritySELinuxRestoreFileLabel(mgr, path, remember) < 0)
+VIR_WARN("Unable to restore label on '%s'. "
+ "XATTRs might have been left in inconsistent state.",
+ path);
+
+virErrorRestore();
 goto cleanup;
+}
 
 ret = 0;
  cleanup:
@@ -1343,16 +1361,14 @@ static int
 virSecuritySELinuxSetFileconOptional(virSecurityManagerPtr mgr,
  const char *path, const char *tcon)
 {
-bool privileged = virSecurityManagerGetPrivileged(mgr);
-return virSecuritySELinuxSetFileconHelper(path, tcon, true, privileged, 
false);
+return virSecuritySELinuxSetFileconHelper(mgr, path, tcon, true, false);
 }
 
 static int
 virSecuritySELinuxSetFilecon(virSecurityManagerPtr mgr,
  const char *path, const char *tcon)
 {
-bool privileged = virSecurityManagerGetPrivileged(mgr);
-return virSecuritySELinuxSetFileconHelper(path, tcon, false, privileged, 
false);
+return virSecuritySELinuxSetFileconHelper(mgr, path, tcon, false, false);
 }
 
 static int
-- 
2.18.1

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

[libvirt] [PATCH 06/18] virSecurityDACRestoreAllLabel: Reorder device relabeling

[Michal Privoznik]

It helps whe trying to match calls with virSecurityDACSetAllLabel
if the order in which devices are set/restored is the same in
both functions.

Signed-off-by: Michal Privoznik 
---
 src/security/security_dac.c | 36 ++--
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index 82b16f96ee..9b3069e60c 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -1665,24 +1665,6 @@ virSecurityDACRestoreAllLabel(virSecurityManagerPtr mgr,
 VIR_DEBUG("Restoring security label on %s migrated=%d",
   def->name, migrated);
 
-for (i = 0; i < def->nhostdevs; i++) {
-if (virSecurityDACRestoreHostdevLabel(mgr,
-  def,
-  def->hostdevs[i],
-  NULL) < 0)
-rc = -1;
-}
-
-for (i = 0; i < def->ngraphics; i++) {
-if (virSecurityDACRestoreGraphicsLabel(mgr, def, def->graphics[i]) < 0)
-return -1;
-}
-
-for (i = 0; i < def->ninputs; i++) {
-if (virSecurityDACRestoreInputLabel(mgr, def, def->inputs[i]) < 0)
-rc = -1;
-}
-
 for (i = 0; i < def->ndisks; i++) {
 if (virSecurityDACRestoreImageLabelInt(mgr,
def,
@@ -1691,6 +1673,24 @@ virSecurityDACRestoreAllLabel(virSecurityManagerPtr mgr,
 rc = -1;
 }
 
+for (i = 0; i < def->ngraphics; i++) {
+if (virSecurityDACRestoreGraphicsLabel(mgr, def, def->graphics[i]) < 0)
+return -1;
+}
+
+for (i = 0; i < def->ninputs; i++) {
+if (virSecurityDACRestoreInputLabel(mgr, def, def->inputs[i]) < 0)
+rc = -1;
+}
+
+for (i = 0; i < def->nhostdevs; i++) {
+if (virSecurityDACRestoreHostdevLabel(mgr,
+  def,
+  def->hostdevs[i],
+  NULL) < 0)
+rc = -1;
+}
+
 for (i = 0; i < def->nmems; i++) {
 if (virSecurityDACRestoreMemoryLabel(mgr,
  def,
-- 
2.18.1

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

[libvirt] [PATCH 09/18] security_dac: Remember old labels

[Michal Privoznik]

Signed-off-by: Michal Privoznik 
---
 src/security/security_dac.c | 48 ++---
 1 file changed, 40 insertions(+), 8 deletions(-)

diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index cdbe07543c..9d31faa9d4 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -29,6 +29,7 @@
 #endif
 
 #include "security_dac.h"
+#include "security_util.h"
 #include "virerror.h"
 #include "virfile.h"
 #include "viralloc.h"
@@ -415,11 +416,26 @@ virSecurityDACGetImageIds(virSecurityLabelDefPtr seclabel,
  */
 static int
 virSecurityDACRememberLabel(virSecurityDACDataPtr priv ATTRIBUTE_UNUSED,
-const char *path ATTRIBUTE_UNUSED,
-uid_t uid ATTRIBUTE_UNUSED,
-gid_t gid ATTRIBUTE_UNUSED)
+const char *path,
+uid_t uid,
+gid_t gid)
 {
-return 0;
+char *label = NULL;
+int ret = -1;
+
+if (virAsprintf(, "+%u:+%u",
+(unsigned int)uid,
+(unsigned int)gid) < 0)
+goto cleanup;
+
+if (virSecuritySetRememberedLabel(SECURITY_DAC_NAME,
+  path, label) < 0)
+goto cleanup;
+
+ret = 0;
+ cleanup:
+VIR_FREE(label);
+return ret;
 }
 
 /**
@@ -439,11 +455,27 @@ virSecurityDACRememberLabel(virSecurityDACDataPtr priv 
ATTRIBUTE_UNUSED,
  */
 static int
 virSecurityDACRecallLabel(virSecurityDACDataPtr priv ATTRIBUTE_UNUSED,
-  const char *path ATTRIBUTE_UNUSED,
-  uid_t *uid ATTRIBUTE_UNUSED,
-  gid_t *gid ATTRIBUTE_UNUSED)
+  const char *path,
+  uid_t *uid,
+  gid_t *gid)
 {
-return 0;
+char *label;
+int ret = -1;
+
+if (virSecurityGetRememberedLabel(SECURITY_DAC_NAME,
+  path, ) < 0)
+goto cleanup;
+
+if (!label)
+return 1;
+
+if (virParseOwnershipIds(label, uid, gid) < 0)
+goto cleanup;
+
+ret = 0;
+ cleanup:
+VIR_FREE(label);
+return ret;
 }
 
 static virSecurityDriverStatus
-- 
2.18.1

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

[libvirt] [PATCH 01/18] security: Unify header conditionals

[Michal Privoznik]

To avoid including a header file twice the following pattern is
used:

 #ifndef __SOMETHING__
 # define __SOMETHING__

where __SOMETHING__ should correspond to the header file name.
However, some of our header files break that pattern.

Signed-off-by: Michal Privoznik 
---
 src/security/security_apparmor.h | 6 +++---
 src/security/security_dac.h  | 6 +++---
 src/security/security_driver.h   | 6 +++---
 src/security/security_manager.h  | 6 +++---
 src/security/security_nop.h  | 6 +++---
 src/security/security_selinux.h  | 6 +++---
 src/security/security_stack.h| 6 +++---
 7 files changed, 21 insertions(+), 21 deletions(-)

diff --git a/src/security/security_apparmor.h b/src/security/security_apparmor.h
index 7872588f64..6b454d1b5c 100644
--- a/src/security/security_apparmor.h
+++ b/src/security/security_apparmor.h
@@ -19,8 +19,8 @@
  *   Jamie Strandboge 
  *
  */
-#ifndef __VIR_SECURITY_APPARMOR_H__
-# define __VIR_SECURITY_APPARMOR_H__
+#ifndef __SECURITY_APPARMOR_H__
+# define __SECURITY_APPARMOR_H__
 
 # include "security_driver.h"
 
@@ -30,4 +30,4 @@ extern virSecurityDriver virAppArmorSecurityDriver;
 # define PROFILE_NAME_SIZE  8 + VIR_UUID_STRING_BUFLEN /* AA_PREFIX + uuid */
 # define MAX_FILE_LEN   (1024*1024*10)  /* 10MB limit for sanity check */
 
-#endif /* __VIR_SECURITY_APPARMOR_H__ */
+#endif /* __SECURITY_APPARMOR_H__ */
diff --git a/src/security/security_dac.h b/src/security/security_dac.h
index 97681c9610..8007bde000 100644
--- a/src/security/security_dac.h
+++ b/src/security/security_dac.h
@@ -20,8 +20,8 @@
 
 #include "security_driver.h"
 
-#ifndef __VIR_SECURITY_DAC
-# define __VIR_SECURITY_DAC
+#ifndef __SECURITY_DAC__
+# define __SECURITY_DAC__
 
 extern virSecurityDriver virSecurityDriverDAC;
 
@@ -38,4 +38,4 @@ void virSecurityDACSetMountNamespace(virSecurityManagerPtr 
mgr,
 void virSecurityDACSetChownCallback(virSecurityManagerPtr mgr,
 virSecurityManagerDACChownCallback 
chownCallback);
 
-#endif /* __VIR_SECURITY_DAC */
+#endif /* __SECURITY_DAC__ */
diff --git a/src/security/security_driver.h b/src/security/security_driver.h
index cd221f1c78..25d49bb0f4 100644
--- a/src/security/security_driver.h
+++ b/src/security/security_driver.h
@@ -19,8 +19,8 @@
  * James Morris 
  *
  */
-#ifndef __VIR_SECURITY_H__
-# define __VIR_SECURITY_H__
+#ifndef __SECURITY_DRIVER_H__
+# define __SECURITY_DRIVER_H__
 
 # include "internal.h"
 # include "domain_conf.h"
@@ -226,4 +226,4 @@ struct _virSecurityDriver {
 virSecurityDriverPtr virSecurityDriverLookup(const char *name,
  const char *virtDriver);
 
-#endif /* __VIR_SECURITY_H__ */
+#endif /* __SECURITY_DRIVER_H__ */
diff --git a/src/security/security_manager.h b/src/security/security_manager.h
index 7e82304689..139b70ec10 100644
--- a/src/security/security_manager.h
+++ b/src/security/security_manager.h
@@ -20,8 +20,8 @@
  * Author: Daniel P. Berrange 
  */
 
-#ifndef VIR_SECURITY_MANAGER_H__
-# define VIR_SECURITY_MANAGER_H__
+#ifndef __SECURITY_MANAGER_H__
+# define __SECURITY_MANAGER_H__
 
 # include "domain_conf.h"
 # include "vircommand.h"
@@ -210,4 +210,4 @@ void
 virSecurityManagerMetadataUnlock(virSecurityManagerPtr mgr,
  virSecurityManagerMetadataLockStatePtr 
*state);
 
-#endif /* VIR_SECURITY_MANAGER_H__ */
+#endif /* __SECURITY_MANAGER_H__ */
diff --git a/src/security/security_nop.h b/src/security/security_nop.h
index 514b339467..7b2ded2292 100644
--- a/src/security/security_nop.h
+++ b/src/security/security_nop.h
@@ -17,11 +17,11 @@
  *
  */
 
-#ifndef __VIR_SECURITY_NOP_H__
-# define __VIR_SECURITY_NOP_H__
+#ifndef __SECURITY_NOP_H__
+# define __SECURITY_NOP_H__
 
 # include "security_driver.h"
 
 extern virSecurityDriver virSecurityDriverNop;
 
-#endif /* __VIR_SECURITY_NOP_H__ */
+#endif /* __SECURITY_NOP_H__ */
diff --git a/src/security/security_selinux.h b/src/security/security_selinux.h
index 1700d8c661..11b62acb52 100644
--- a/src/security/security_selinux.h
+++ b/src/security/security_selinux.h
@@ -19,9 +19,9 @@
  * James Morris 
  *
  */
-#ifndef __VIR_SECURITY_SELINUX_H__
-# define __VIR_SECURITY_SELINUX_H__
+#ifndef __SECURITY_SELINUX_H__
+# define __SECURITY_SELINUX_H__
 
 extern virSecurityDriver virSecurityDriverSELinux;
 
-#endif /* __VIR_SECURITY_SELINUX_H__ */
+#endif /* __SECURITY_SELINUX_H__ */
diff --git a/src/security/security_stack.h b/src/security/security_stack.h
index b38f9a9481..7e6ab3d93e 100644
--- a/src/security/security_stack.h
+++ b/src/security/security_stack.h
@@ -20,8 +20,8 @@
 
 #include "security_driver.h"
 
-#ifndef __VIR_SECURITY_STACK
-# define __VIR_SECURITY_STACK
+#ifndef __SECURITY_STACK__
+# define __SECURITY_STACK__
 
 extern virSecurityDriver virSecurityDriverStack;
 
@@ -35,4 +35,4 @@ virSecurityStackGetPrimary(virSecurityManagerPtr mgr);
 virSecurityManagerPtr*
 virSecurityStackGetNested(virSecurityManagerPtr mgr);
 
-#endif /* 

[libvirt] [PATCH 02/18] util: Introduce xattr getter/setter/remover

[Michal Privoznik]

Signed-off-by: Michal Privoznik 
---
 src/libvirt_private.syms |   3 +
 src/util/virfile.c   | 121 +++
 src/util/virfile.h   |  11 
 3 files changed, 135 insertions(+)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 8889aaa379..85580beb58 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1827,6 +1827,7 @@ virFileGetACLs;
 virFileGetHugepageSize;
 virFileGetMountReverseSubtree;
 virFileGetMountSubtree;
+virFileGetXAtrr;
 virFileHasSuffix;
 virFileInData;
 virFileIsAbsPath;
@@ -1866,6 +1867,7 @@ virFileReadValueUint;
 virFileRelLinkPointsTo;
 virFileRemove;
 virFileRemoveLastComponent;
+virFileRemoveXAttr;
 virFileResolveAllLinks;
 virFileResolveLink;
 virFileRewrite;
@@ -1873,6 +1875,7 @@ virFileRewriteStr;
 virFileSanitizePath;
 virFileSetACLs;
 virFileSetupDev;
+virFileSetXAtrr;
 virFileSkipRoot;
 virFileStripSuffix;
 virFileTouch;
diff --git a/src/util/virfile.c b/src/util/virfile.c
index f6f9e4ceda..9df5f70c60 100644
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
@@ -64,6 +64,10 @@
 # include 
 #endif
 
+#if HAVE_LIBATTR
+# include 
+#endif
+
 #include "configmake.h"
 #include "intprops.h"
 #include "vircommand.h"
@@ -4354,3 +4358,120 @@ virFileWaitForExists(const char *path,
 
 return 0;
 }
+
+
+#if HAVE_LIBATTR
+/**
+ * virFileGetXAtrr;
+ * @path: a filename
+ * @name: name of xattr
+ * @value: read value
+ *
+ * Reads xattr with @name for given @path and stores it into
+ * @value. Caller is responsible for freeing @value.
+ *
+ * Returns: 0 on success,
+ * -1 otherwise (with errno set).
+ */
+int
+virFileGetXAtrr(const char *path,
+const char *name,
+char **value)
+{
+char *buf = NULL;
+int ret = -1;
+
+/* We might be racing with somebody who sets the same attribute. */
+do {
+ssize_t need;
+ssize_t got;
+
+/* The first call determines how many bytes we need to allocate. */
+if ((need = getxattr(path, name, NULL, 0)) < 0)
+goto cleanup;
+
+if (VIR_REALLOC_N_QUIET(buf, need + 1) < 0)
+goto cleanup;
+
+if ((got = getxattr(path, name, buf, need)) < 0) {
+if (errno == ERANGE)
+continue;
+goto cleanup;
+}
+
+buf[got] = '\0';
+break;
+} while (1);
+
+VIR_STEAL_PTR(*value, buf);
+ret = 0;
+ cleanup:
+VIR_FREE(buf);
+return ret;
+}
+
+/**
+ * virFileSetXAtrr:
+ * @path: a filename
+ * @name: name of xattr
+ * @value: value to set
+ *
+ * Sets xattr of @name and @value on @path.
+ *
+ * Returns: 0 on success,
+ * -1 otherwise (with errno set).
+ */
+int
+virFileSetXAtrr(const char *path,
+const char *name,
+const char *value)
+{
+return setxattr(path, name, value, strlen(value), 0);
+}
+
+/**
+ * virFileRemoveXAttr:
+ * @path: a filename
+ * @name: name of xattr
+ *
+ * Remove xattr of @name on @path.
+ *
+ * Returns: 0 on success,
+ * -1 otherwise (with errno set).
+ */
+int
+virFileRemoveXAttr(const char *path,
+   const char *name)
+{
+return removexattr(path, name);
+}
+
+#else /* !HAVE_LIBATTR */
+
+int
+virFileGetXAtrr(const char *path ATTRIBUTE_UNUSED,
+const char *name ATTRIBUTE_UNUSED,
+char **value ATTRIBUTE_UNUSED)
+{
+errno = ENOSYS;
+return -1;
+}
+
+int
+virFileSetXAtrr(const char *path ATTRIBUTE_UNUSED,
+const char *name ATTRIBUTE_UNUSED,
+const char *value ATTRIBUTE_UNUSED)
+{
+errno = ENOSYS;
+return -1;
+}
+
+int
+virFileRemoveXAttr(const char *path ATTRIBUTE_UNUSED,
+   const char *name ATTRIBUTE_UNUSED)
+{
+errno = ENOSYS;
+return -1;
+}
+
+#endif /* HAVE_LIBATTR */
diff --git a/src/util/virfile.h b/src/util/virfile.h
index 0f7dece958..9cd1bc3a5f 100644
--- a/src/util/virfile.h
+++ b/src/util/virfile.h
@@ -383,4 +383,15 @@ int virFileInData(int fd,
 
 VIR_DEFINE_AUTOPTR_FUNC(virFileWrapperFd, virFileWrapperFdFree)
 
+int virFileGetXAtrr(const char *path,
+const char *name,
+char **value);
+
+int virFileSetXAtrr(const char *path,
+const char *name,
+const char *value);
+
+int virFileRemoveXAttr(const char *path,
+   const char *name);
+
 #endif /* __VIR_FILE_H */
-- 
2.18.1

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

[libvirt] [PATCH 00/18] Implement original label remembering

[Michal Privoznik]

Dear list,

there were several attempts in the past to implement this feature, but
none of them was successful. The problem is that we change security
labels when starting a domain but never record the original labels
therefore when restoring the labels back in domain shutdown phase we
have to go with root:root or restorecon. This is not user friendly.

Now that we have metadata locking implemented we have exclusive access
to the files we are touching and therefore can call functions to record
the original owner. Since this database needs to be distributed
(consider multiple daemons and an network file system) it can't be
stored inside a daemon (libvirtd knows nothing about other daemons
running on distant hosts). Therefore the next option is to store it with
the files themselves - in XATTRs.

There is one caveat though. A file can be passed to multiple domains at
the same time (for instance an installation ISO), therefore we need a
reference counter so that the only the last label restore call actually
restores the original owner. A picture is worth more than a thousand
words:

# chown 5:6 /var/lib/libvirt/images/fd.img

# ls -ln /var/lib/libvirt/images/fd.img
-rw-r--r-- 1 5 6 2097152 Mar 17  2018 /var/lib/libvirt/images/fd.img

# getfattr -d -m - /var/lib/libvirt/images/fd.img
(no output)

# virsh domblklist fedora
 Target   Source

 sda  /var/lib/libvirt/images/fedora.qcow2
 sdb  /var/lib/libvirt/images/fd.img

# virsh domblklist gentoo
 Target   Source
--
 fda  /var/lib/libvirt/images/fd.img
 sda  /var/lib/libvirt/images/gentoo.qcow2

# virsh start fedora
Domain fedora started

# getfattr -d -m - /var/lib/libvirt/images/fd.img
trusted.libvirt.security.dac="+5:+6"
trusted.libvirt.security.ref_dac="1"

# virsh start gentoo
Domain gentoo started

# getfattr -d -m - /var/lib/libvirt/images/fd.img
trusted.libvirt.security.dac="+5:+6"
trusted.libvirt.security.ref_dac="2"

# virsh shutdown --domain fedora
Domain fedora is being shutdown

# ls -ln /var/lib/libvirt/images/fd.img
-rw-r--r-- 1 0 0 2097152 Mar 17  2018 /var/lib/libvirt/images/fd.img

# getfattr -d -m - /var/lib/libvirt/images/fd.img
trusted.libvirt.security.dac="+5:+6"
trusted.libvirt.security.ref_dac="1"

# virsh shutdown --domain gentoo
Domain gentoo is being shutdown

# getfattr -d -m - /var/lib/libvirt/images/fd.img
(no output)

# ls -ln /var/lib/libvirt/images/fd.img
-rw-r--r-- 1 5 6 2097152 Mar 17  2018 /var/lib/libvirt/images/fd.img


Even though I'm showing DAC only in my example, it's the same story with
SELinux.

Of course, this plays nicely with filesystems that don't support XATTRs,
which there are not that much, but unfortunately NFS is one of them :(


Michal Prívozník (18):
  security: Unify header conditionals
  util: Introduce xattr getter/setter/remover
  security: Include security_util
  security_dac: Restore label on failed chown() attempt
  virSecurityDACTransactionRun: Implement rollback
  virSecurityDACRestoreAllLabel: Reorder device relabeling
  virSecurityDACRestoreAllLabel: Restore more labels
  security_dac: Allow callers to enable/disable label remembering/recall
  security_dac: Remember old labels
  virSecurityDACRestoreImageLabelInt: Restore even shared/RO disks
  security_selinux: Track if transaction is restore
  security_selinux: Remember old labels
  security_selinux: Restore label on failed setfilecon() attempt
  virSecuritySELinuxTransactionRun: Implement rollback
  virSecuritySELinuxRestoreAllLabel: Reorder device relabeling
  virSecuritySELinuxRestoreAllLabel: Restore more labels
  tools: Provide a script to recover fubar'ed XATTRs setup
  qemu.conf: Allow users to enable/disable label remembering

 src/libvirt_private.syms   |   3 +
 src/qemu/libvirtd_qemu.aug |   1 +
 src/qemu/qemu.conf |   6 +
 src/qemu/qemu_conf.c   |   4 +
 src/qemu/test_libvirtd_qemu.aug.in |   1 +
 src/security/Makefile.inc.am   |   2 +
 src/security/security_apparmor.h   |   6 +-
 src/security/security_dac.c| 212 +---
 src/security/security_dac.h|   6 +-
 src/security/security_driver.h |   6 +-
 src/security/security_manager.h|   6 +-
 src/security/security_nop.h|   6 +-
 src/security/security_selinux.c| 256 +
 src/security/security_selinux.h|   6 +-
 src/security/security_stack.h  |   6 +-
 src/security/security_util.c   | 198 ++
 src/security/security_util.h   |  32 
 src/util/virfile.c | 121 ++
 src/util/virfile.h |  11 ++
 tools/Makefile.am  |   1 +
 tools/libvirt_recover_xattrs.sh|  89 ++
 21 files changed, 829 insertions(+), 150 deletions(-)
 create mode 100644 src/security/security_util.c
 create mode 100644 src/security/security_util.h
 create mode 100755