from:"Stefan Bader"

Re: [libvirt] [PATCH] apparmor: support finer-grained ptrace checks

2017-09-22 Thread Stefan Bader

On 22.09.2017 14:52, Guido Günther wrote:
> Hi Jim,
> On Wed, Sep 20, 2017 at 11:17:06AM -0600, Jim Fehlig wrote:
>> On 09/20/2017 08:57 AM, Jim Fehlig wrote:
>>> On 09/20/2017 12:51 AM, Guido Günther wrote:
 Hi Jim,
 On Mon, Sep 18, 2017 at 02:06:13PM -0600, Jim Fehlig wrote:
> Kernel 4.13 introduced finer-grained ptrace checks
>
> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git/commit/?h=v4.13.2=290f458a4f16f9cf6cb6562b249e69fe1c3c3a07
>
>
> When Apparmor is enabled and libvirtd is confined, attempting to start
> a domain fails
>
> virsh start test
> error: Failed to start domain test
> error: internal error: child reported: Kernel does not provide mount
>     namespace: Permission denied
>
> The audit log contains
>
> type=AVC msg=audit(1505466699.828:534): apparmor="DENIED"
> operation="ptrace" profile="/usr/sbin/libvirtd" pid=6621
> comm="libvirtd" requested_mask="trace" denied_mask="trace"
> peer="/usr/sbin/libvirtd"

 It seems access to /proc//tasks already requires trace permissions.

>
> It was also noticed that simply connecting to libvirtd (e.g. virsh list)
> resulted in the following entries in the audit log
>
> type=AVC msg=audit(1505755799.975:65): apparmor="DENIED"
> operation="ptrace" profile="/usr/sbin/libvirtd" pid=1418
> comm="libvirtd" requested_mask="trace" denied_mask="trace"
> peer="unconfined"
> type=AVC msg=audit(1505755799.976:66): apparmor="DENIED"
> operation="ptrace" profile="/usr/sbin/libvirtd" pid=1418
> comm="libvirtd" requested_mask="trace" denied_mask="trace"
> peer="unconfined"
>
> Both Apparmor denials can be fixed by adding ptrace rules to the
> libvirtd profile. The new rules only grant trace permission.

 I'm seeing the same denials with 4.13 (4.13.1-1~exp1 (2017-09-11) in
 Debian) but the proposed profile change does not fix the vm start issue
 for me. I can't tell why atm, will have to look into this in more detail
 at the WE.
>>>
>>> I have other problems when running with 'security_default_confined = 1'
>>> in qemu.conf, but the changes allow starting unconfined domains.
>>>
>>> Cedric remembered this old thread
>>>
>>> https://www.redhat.com/archives/libvir-list/2014-October/msg00011.html
>>>
>>> Some of those changes have been merged, but the ptrace, dbus, signal,
>>> etc. have not. I used Stefan's changes to the libvirtd profile but still
>>> see the same issue with confined domains
>>
>> I dug a bit further in that thread to find Stefan's most recent version of
>> the patches
>>
>> https://www.redhat.com/archives/libvir-list/2014-October/msg00556.html
>>
>> I took the ptrace, dbus, signal, etc. changes out of patch 2 and used the
>> attached patch to successfully start confined domains.
>>
>> Since a few years have passed, I'm not sure if patch 1 is still relevant.
>> IIUC, it allows to conditionalize profile content based on apparmor version,
>> which patch 2 uses to add some stuff if version >= 2.9. 2.9 has been out for
>> a while...
> 
> Great, this helps a lot!


Sorry I missed the updates (and also sorry/warning that I have not read the
thread very closely... brain entangled in various mental corners). Christian and
I had started to finally get rid of things we carry by properly upstreaming
things. Sadly about maybe halfway through and then other things popped up.
But for the special parsing, yes we both also thought that nowadays there is no
reason anymore to add the whole parsing stuff as everybody should be on versions
off apparmor supporting those things.

I cannot promise anything but maybe there will be a little bit of time next week
to give some better answers.

-Stefan
> 
>> From e3bb609812776b30acfc0349b25b2e4d539c45c2 Mon Sep 17 00:00:00 2001
>> From: Jim Fehlig 
>> Date: Mon, 18 Sep 2017 13:41:26 -0600
>> Subject: [PATCH] apparmor: support ptrace checks
>>
>> Kernel 4.13 introduced finer-grained ptrace checks
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git/commit/?h=v4.13.2=290f458a4f16f9cf6cb6562b249e69fe1c3c3a07
>>
>> When Apparmor is enabled and libvirtd is confined, attempting to start
>> a domain fails
>>
>> virsh start test
>> error: Failed to start domain test
>> error: internal error: child reported: Kernel does not provide mount
>>namespace: Permission denied
>>
>> The audit log contains
>>
>> type=AVC msg=audit(1505466699.828:534): apparmor="DENIED"
>> operation="ptrace" profile="/usr/sbin/libvirtd" pid=6621
>> comm="libvirtd" requested_mask="trace" denied_mask="trace"
>> peer="/usr/sbin/libvirtd"
>>
>> It was also noticed that simply connecting to libvirtd (e.g. virsh list)
>> resulted in the following entries in the audit log
>>
>> type=AVC msg=audit(1505755799.975:65): apparmor="DENIED"
>> operation="ptrace" profile="/usr/sbin/libvirtd" pid=1418
>> comm="libvirtd"

[libvirt] Various apparmor related changes (part 2)

2017-05-23 Thread Stefan Bader

> Over the years there have been a bunch of changes to the
> apparmor profiles and/or virt-aa-helper which have been
> carried in Debian/Ubuntu but never made it upstream.
> 
> In an attempt to clean this up and generally improve the
> apparmor based environments, we (Christian and I) went
> over the changes, cleaned out cruft as much as possible 
> and would be sending out hunks of changes to this list
> for upstream inclusion.
> 

This second batch consists partially of some reworked
patches from the previous round and some more things
which hopefully are simple enough and improve the upstream
profiles.

5+6: Although these are Debian/Ubuntu specific, there are
 other paths which are specific for SuSE. So I wondered
 why not have both upstream.
9:   Jamie, I know it has been a long time but do you remember
 what this resolved?

Thanks,
Stefan

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

[libvirt] [PATCH 01/10] virt-aa-helper, apparmor: allow /usr/share/OVMF/ too

2017-05-23 Thread Stefan Bader

From: Simon McVittie <s...@debian.org>

The split firmware and variables files introduced by
https://bugs.debian.org/764918 are in a different directory for
some reason. Let the virtual machine read both.

Signed-off-by: Christian Ehrhardt <christian.ehrha...@canonical.com>
Signed-off-by: Stefan Bader <stefan.ba...@canonical.com>
---
 examples/apparmor/libvirt-qemu | 1 +
 src/security/virt-aa-helper.c  | 1 +
 tests/virt-aa-helper-test  | 7 ++-
 3 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/examples/apparmor/libvirt-qemu b/examples/apparmor/libvirt-qemu
index a9020aa..e0988bb 100644
--- a/examples/apparmor/libvirt-qemu
+++ b/examples/apparmor/libvirt-qemu
@@ -70,6 +70,7 @@
   /usr/share/vgabios/** r,
   /usr/share/seabios/** r,
   /usr/share/ovmf/** r,
+  /usr/share/OVMF/** r,
 
   # access PKI infrastructure
   /etc/pki/libvirt-vnc/** r,
diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
index 5f5d1cd..6c5fc28 100644
--- a/src/security/virt-aa-helper.c
+++ b/src/security/virt-aa-helper.c
@@ -512,6 +512,7 @@ valid_path(const char *path, const bool readonly)
 "/vmlinuz",
 "/initrd",
 "/initrd.img",
+"/usr/share/OVMF/",  /* for OVMF images */
 "/usr/share/ovmf/"   /* for OVMF images */
 };
 /* override the above with these */
diff --git a/tests/virt-aa-helper-test b/tests/virt-aa-helper-test
index 68e9399..c05afc1 100755
--- a/tests/virt-aa-helper-test
+++ b/tests/virt-aa-helper-test
@@ -296,8 +296,13 @@ if [ -f /usr/share/ovmf/OVMF.fd ]; then
 -e "s,###DISK###,$disk1,g" \
 -e "s,,/usr/share/ovmf/OVMF.fd,g" "$template_xml" > 
"$test_xml"
 testme "0" "ovmf" "-r -u $valid_uuid" "$test_xml"
+elif [ -f /usr/share/OVMF/OVMF.fd ]; then
+sed -e "s,###UUID###,$uuid,g"  \
+-e "s,###DISK###,$disk1,g" \
+-e "s,,/usr/share/OVMF/OVMF.fd,g" "$template_xml" > 
"$test_xml"
+testme "0" "ovmf" "-r -u $valid_uuid" "$test_xml"
 else
-echo "Skipping OVMF test. Could not find /usr/share/ovmf/OVMF.fd"
+echo "Skipping OVMF test. Could not find /usr/share/ovmf/OVMF.fd or 
/usr/share/OVMF/OVMF.fd"
 fi
 
 sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e 
"s,,$tmpdir/initrd,g" "$template_xml" > "$test_xml"
-- 
2.7.4

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

[libvirt] [PATCH 02/10] virt-aa-helper: Generalize test for firmware paths

2017-05-23 Thread Stefan Bader

From: Christian Ehrhardt <christian.ehrha...@canonical.com>

This replaces individual tests for firmware locations by
a generic function which will simplify having additional
locations in the future.

Signed-off-by: Christian Ehrhardt <christian.ehrha...@canonical.com>
Signed-off-by: Stefan Bader <stefan.ba...@canonical.com>
---
 tests/virt-aa-helper-test | 29 -
 1 file changed, 16 insertions(+), 13 deletions(-)

diff --git a/tests/virt-aa-helper-test b/tests/virt-aa-helper-test
index c05afc1..73f3080 100755
--- a/tests/virt-aa-helper-test
+++ b/tests/virt-aa-helper-test
@@ -145,6 +145,20 @@ testme() {
 fi
 }
 
+testfw() {
+title="$1"
+fwpath="$2"
+
+if [ -f "$fwpath" ]; then
+sed -e "s,###UUID###,$uuid,g"  \
+-e "s,###DISK###,$disk1,g" \
+-e "s,,$fwpath,g" "$template_xml" > "$test_xml"
+testme "0" "$title" "-r -u $valid_uuid" "$test_xml"
+else
+echo "Skipping FW $title test. Could not find $fwpath"
+fi
+}
+
 # Expected failures
 echo "Expected failures:" >$output
 testme "1" "invalid arg" "-z"
@@ -291,19 +305,8 @@ sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" 
-e "s,,$tm
 touch "$tmpdir/kernel"
 testme "0" "kernel" "-r -u $valid_uuid" "$test_xml"
 
-if [ -f /usr/share/ovmf/OVMF.fd ]; then
-sed -e "s,###UUID###,$uuid,g"  \
--e "s,###DISK###,$disk1,g" \
--e "s,,/usr/share/ovmf/OVMF.fd,g" "$template_xml" > 
"$test_xml"
-testme "0" "ovmf" "-r -u $valid_uuid" "$test_xml"
-elif [ -f /usr/share/OVMF/OVMF.fd ]; then
-sed -e "s,###UUID###,$uuid,g"  \
--e "s,###DISK###,$disk1,g" \
--e "s,,/usr/share/OVMF/OVMF.fd,g" "$template_xml" > 
"$test_xml"
-testme "0" "ovmf" "-r -u $valid_uuid" "$test_xml"
-else
-echo "Skipping OVMF test. Could not find /usr/share/ovmf/OVMF.fd or 
/usr/share/OVMF/OVMF.fd"
-fi
+testfw "ovmf (old path)" "/usr/share/ovmf/OVMF.fd"
+testfw "OVMF (new path)" "/usr/share/OVMF/OVMF_CODE.fd"
 
 sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e 
"s,,$tmpdir/initrd,g" "$template_xml" > "$test_xml"
 touch "$tmpdir/initrd"
-- 
2.7.4

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

[libvirt] [PATCH 09/10] apparmor, libvirt-qemu: Allow read access to overcommit_memory

2017-05-23 Thread Stefan Bader

From: Jamie Strandboge <ja...@ubuntu.com>

Allow qemu to read @{PROC}/sys/vm/overcommit_memory.

Signed-off-by: Christian Ehrhardt <christian.ehrha...@canonical.com>
Signed-off-by: Stefan Bader <stefan.ba...@canonical.com>
---
 examples/apparmor/libvirt-qemu | 1 +
 1 file changed, 1 insertion(+)

diff --git a/examples/apparmor/libvirt-qemu b/examples/apparmor/libvirt-qemu
index e2b0dfd..89525b3 100644
--- a/examples/apparmor/libvirt-qemu
+++ b/examples/apparmor/libvirt-qemu
@@ -26,6 +26,7 @@
   # only modify its comm value or those in its thread group.
   owner @{PROC}/@{pid}/task/@{tid}/comm rw,
   @{PROC}/sys/kernel/cap_last_cap r,
+  @{PROC}/sys/vm/overcommit_memory r,
 
   # For hostdev access. The actual devices will be added dynamically
   /sys/bus/usb/devices/ r,
-- 
2.7.4

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

[libvirt] [PATCH 10/10] apparmor, libvirt-qemu: Allow access to certificates used by libvirt-vnc

2017-05-23 Thread Stefan Bader

From: Serge Hallyn <serge.hal...@ubuntu.com>

When setting up VncTLS according to the official Libvirt documentation,
only one certificate for libvirt/libvirt-vnc is used. The document
indicates to use the following directories :

 /etc/pki/CA
 /etc/pki/libvirt
 /etc/pki/libvirt/private

in order to manage the certificates used by libvirt-vnc.

Bug-Ubuntu: https://bugs.launchpad.net/bugs/901272

Signed-off-by: Christian Ehrhardt <christian.ehrha...@canonical.com>
Signed-off-by: Stefan Bader <stefan.ba...@canonical.com>
---
 examples/apparmor/libvirt-qemu | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/examples/apparmor/libvirt-qemu b/examples/apparmor/libvirt-qemu
index 89525b3..e990ab4 100644
--- a/examples/apparmor/libvirt-qemu
+++ b/examples/apparmor/libvirt-qemu
@@ -144,6 +144,12 @@
   /usr/{lib,lib64}/qemu/block-curl.so mr,
   /usr/{lib,lib64}/qemu/block-rbd.so mr,
 
+  # for use by libvirt-vnc (LP: #901272)
+  /etc/pki/CA/ r,
+  /etc/pki/CA/* r,
+  /etc/pki/libvirt/ r,
+  /etc/pki/libvirt/** r,
+
   # for save and resume
   /{usr/,}bin/dash rmix,
   /{usr/,}bin/dd rmix,
-- 
2.7.4

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

[libvirt] [PATCH 03/10] apparmor, virt-aa-helper: Allow aarch64 UEFI.

2017-05-23 Thread Stefan Bader

From: William Grant <wgr...@ubuntu.com>

Allow access to aarch64 UEFI images.

Signed-off-by: Christian Ehrhardt <christian.ehrha...@canonical.com>
Signed-off-by: Stefan Bader <stefan.ba...@canonical.com>
Acked-by: Guido Günther <a...@sigxcpu.org>
---
 examples/apparmor/libvirt-qemu | 2 ++
 src/security/virt-aa-helper.c  | 4 +++-
 tests/virt-aa-helper-test  | 2 ++
 3 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/examples/apparmor/libvirt-qemu b/examples/apparmor/libvirt-qemu
index e0988bb..89466c9 100644
--- a/examples/apparmor/libvirt-qemu
+++ b/examples/apparmor/libvirt-qemu
@@ -71,6 +71,8 @@
   /usr/share/seabios/** r,
   /usr/share/ovmf/** r,
   /usr/share/OVMF/** r,
+  /usr/share/AAVMF/** r,
+  /usr/share/qemu-efi/** r,
 
   # access PKI infrastructure
   /etc/pki/libvirt-vnc/** r,
diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
index 6c5fc28..69e797c 100644
--- a/src/security/virt-aa-helper.c
+++ b/src/security/virt-aa-helper.c
@@ -513,7 +513,9 @@ valid_path(const char *path, const bool readonly)
 "/initrd",
 "/initrd.img",
 "/usr/share/OVMF/",  /* for OVMF images */
-"/usr/share/ovmf/"   /* for OVMF images */
+"/usr/share/ovmf/",  /* for OVMF images */
+"/usr/share/AAVMF/", /* for AAVMF images */
+"/usr/share/qemu-efi/"   /* for AAVMF images */
 };
 /* override the above with these */
 const char * const override[] = {
diff --git a/tests/virt-aa-helper-test b/tests/virt-aa-helper-test
index 73f3080..51072f6 100755
--- a/tests/virt-aa-helper-test
+++ b/tests/virt-aa-helper-test
@@ -307,6 +307,8 @@ testme "0" "kernel" "-r -u $valid_uuid" "$test_xml"
 
 testfw "ovmf (old path)" "/usr/share/ovmf/OVMF.fd"
 testfw "OVMF (new path)" "/usr/share/OVMF/OVMF_CODE.fd"
+testfw "AAVMF" "/usr/share/AAVMF/AAVMF_CODE.fd"
+testfw "qemu-efi" "/usr/share/qemu-efi/QEMU_EFI.fd"
 
 sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e 
"s,,$tmpdir/initrd,g" "$template_xml" > "$test_xml"
 touch "$tmpdir/initrd"
-- 
2.7.4

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

[libvirt] [PATCH 04/10] apparmor, libvirt-qemu: Add ppc64el related changes

2017-05-23 Thread Stefan Bader

From: Serge Hallyn <serge.hal...@ubuntu.com>

Updates profile to allow running on ppc64el.

Bug-Ubuntu: https://bugs.launchpad.net/bugs/1374554

Signed-off-by: Christian Ehrhardt <christian.ehrha...@canonical.com>
Signed-off-by: Stefan Bader <stefan.ba...@canonical.com>
---
 examples/apparmor/libvirt-qemu | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/examples/apparmor/libvirt-qemu b/examples/apparmor/libvirt-qemu
index 89466c9..7fa512f 100644
--- a/examples/apparmor/libvirt-qemu
+++ b/examples/apparmor/libvirt-qemu
@@ -73,6 +73,7 @@
   /usr/share/OVMF/** r,
   /usr/share/AAVMF/** r,
   /usr/share/qemu-efi/** r,
+  /usr/share/slof/** r,
 
   # access PKI infrastructure
   /etc/pki/libvirt-vnc/** r,
@@ -154,3 +155,8 @@
   /etc/udev/udev.conf r,
   /sys/bus/ r,
   /sys/class/ r,
+
+  # for ppc device-tree access
+  @{PROC}/device-tree/ r,
+  @{PROC}/device-tree/** r,
+  /sys/firmware/devicetree/** r,
-- 
2.7.4

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

[libvirt] [PATCH 06/10] apparmor, libvirtd: Allow libxl-save-helper to run on Debian/Ubuntu

2017-05-23 Thread Stefan Bader

On Debian/Ubuntu the libxl-save-helper (used when saving/restoring
a domain through libxl) is located under /usr/lib/xen-/bin.

Bug-Ubuntu: https://bugs.launchpad.net/bugs/1334195

Signed-off-by: Christian Ehrhardt <christian.ehrha...@canonical.com>
Signed-off-by: Stefan Bader <stefan.ba...@canonical.com>
---
 examples/apparmor/usr.sbin.libvirtd | 1 +
 1 file changed, 1 insertion(+)

diff --git a/examples/apparmor/usr.sbin.libvirtd 
b/examples/apparmor/usr.sbin.libvirtd
index f43bfd5..64f6d2c 100644
--- a/examples/apparmor/usr.sbin.libvirtd
+++ b/examples/apparmor/usr.sbin.libvirtd
@@ -51,6 +51,7 @@
   /usr/{lib,lib64}/xen-common/bin/xen-toolstack PUx,
   /usr/{lib,lib64}/xen/bin/* Ux,
   /usr/lib/xen-*/bin/pygrub PUx,
+  /usr/lib/xen-*/bin/libxl-save-helper PUx,
 
   # force the use of virt-aa-helper
   audit deny /{usr/,}sbin/apparmor_parser rwxl,
-- 
2.7.4

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

[libvirt] [PATCH 07/10] apparmor, libvirt-qemu: Allow access to ceph config

2017-05-23 Thread Stefan Bader

From: Serge Hallyn <serge.hal...@ubuntu.com>

Signed-off-by: Christian Ehrhardt <christian.ehrha...@canonical.com>
Signed-off-by: Stefan Bader <stefan.ba...@canonical.com>
---
 examples/apparmor/libvirt-qemu | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/examples/apparmor/libvirt-qemu b/examples/apparmor/libvirt-qemu
index 7fa512f..fddc93a 100644
--- a/examples/apparmor/libvirt-qemu
+++ b/examples/apparmor/libvirt-qemu
@@ -156,6 +156,9 @@
   /sys/bus/ r,
   /sys/class/ r,
 
+  # for rbd
+  /etc/ceph/ceph.conf r,
+
   # for ppc device-tree access
   @{PROC}/device-tree/ r,
   @{PROC}/device-tree/** r,
-- 
2.7.4

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

[libvirt] [PATCH 05/10] apparmor: Allow pygrub to run on Debian/Ubuntu

2017-05-23 Thread Stefan Bader

In Debian/Ubuntu the pygrub command is located under
/usr/lib/xen-/bin/pygrub.

Bug-Ubuntu: https://bugs.launchpad.net/bugs/1326003

Signed-off-by: Christian Ehrhardt <christian.ehrha...@canonical.com>
Signed-off-by: Stefan Bader <stefan.ba...@canonical.com>
---
 examples/apparmor/usr.sbin.libvirtd | 1 +
 1 file changed, 1 insertion(+)

diff --git a/examples/apparmor/usr.sbin.libvirtd 
b/examples/apparmor/usr.sbin.libvirtd
index 353b039..f43bfd5 100644
--- a/examples/apparmor/usr.sbin.libvirtd
+++ b/examples/apparmor/usr.sbin.libvirtd
@@ -50,6 +50,7 @@
   /{usr/,}lib/udev/scsi_id PUx,
   /usr/{lib,lib64}/xen-common/bin/xen-toolstack PUx,
   /usr/{lib,lib64}/xen/bin/* Ux,
+  /usr/lib/xen-*/bin/pygrub PUx,
 
   # force the use of virt-aa-helper
   audit deny /{usr/,}sbin/apparmor_parser rwxl,
-- 
2.7.4

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

[libvirt] [PATCH 08/10] apparmor, libvirt-qemu: Allow macvtap access

2017-05-23 Thread Stefan Bader

From: Guilhem Lettron <guilhem+ubu...@lettron.fr>

Add rule to allow access to /dev/tap* used by macvtap.

Bug-Ubuntu: https://bugs.launchpad.net/bugs/921870

Signed-off-by: Christian Ehrhardt <christian.ehrha...@canonical.com>
Signed-off-by: Stefan Bader <stefan.ba...@canonical.com>
---
 examples/apparmor/libvirt-qemu | 1 +
 1 file changed, 1 insertion(+)

diff --git a/examples/apparmor/libvirt-qemu b/examples/apparmor/libvirt-qemu
index fddc93a..e2b0dfd 100644
--- a/examples/apparmor/libvirt-qemu
+++ b/examples/apparmor/libvirt-qemu
@@ -17,6 +17,7 @@
   network inet6 stream,
 
   /dev/net/tun rw,
+  /dev/tap* rw,
   /dev/kvm rw,
   /dev/ptmx rw,
   /dev/kqemu rw,
-- 
2.7.4

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

Re: [libvirt] [PATCH 8/8] apparmor, libvirt-qemu: Add ppc64el related changes

2017-05-22 Thread Stefan Bader

On 22.05.2017 15:12, Andrea Bolognani wrote:
> On Thu, 2017-05-18 at 10:53 +0200, Stefan Bader wrote:
>> @@ -102,6 +103,7 @@
>> /usr/bin/qemu-system-or32 rmix,
>> /usr/bin/qemu-system-ppc rmix,
>> /usr/bin/qemu-system-ppc64 rmix,
>> +  /usr/bin/qemu-system-ppc64le rmix,
> 
> Why does Ubuntu have qemu-system-ppc64le at all?

Hm, apparently it is not really there. It is a soft-link to qemu-system-ppc64.
So that hunk really makes no sense because apparmor is following the link before
checking permissions.

I will refresh that one to drop the ppc64le path.

-Stefan
> 
> qemu-system-ppc64 can run both ppc64 and ppc64le guests just
> fine, it's emulating the same hardware and the endianness is
> decided at runtime AFAIK.
> 
> Upstream QEMU doesn't seem to know anything about this:
> 
>   $ ./configure --target-list=ppc64le-softmmu
>   ERROR: Unknown target name 'ppc64le-softmmu'
> 
> and it's not in the Debian package, so it must have been
> added specifically for Ubuntu.
> 
> -- 
> Andrea Bolognani / Red Hat / Virtualization
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
> 




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

Re: [libvirt] [PATCH 8/8] apparmor, libvirt-qemu: Add ppc64el related changes

2017-05-22 Thread Stefan Bader

On 19.05.2017 11:03, Christian Ehrhardt wrote:
> 
> On Fri, May 19, 2017 at 9:55 AM, Guido Günther  > wrote:
> 
> LGTM but I don't know much about PPC64, it's SLOF and where the device
> tree should be located.
> 
> 
> Hi those paths for SLOF are the default one for Debian/Ubuntu at least.
> $ dpkg -L qemu-slof
> /.
> /usr
> /usr/share
> /usr/share/doc
> /usr/share/doc/qemu-slof
> /usr/share/doc/qemu-slof/copyright
> /usr/share/doc/qemu-slof/changelog.Debian.gz
> /usr/share/slof
> /usr/share/slof/spapr-rtas.bin
> /usr/share/slof/slof.bin
> 
> The other paths are kernel owned and match the current state:
> (the proc path is a symlink today, not sure if it was a real path back in 
> history)
> $ ll /proc/device-tree
> lrwxrwxrwx 1 root root 29 Mai 19 08:59 /proc/device-tree ->
> /sys/firmware/devicetree/base/
> $ ll  /sys/firmware/devicetree/
> total 0
> drwxr-xr-x  3 root root 0 Mai 19 08:57 ./
> drwxr-xr-x  4 root root 0 Mai 19 08:57 ../
> drwxr-xr-x 51 root root 0 Mai 19 08:57 base/
> (there is much more under this path, but the rule is ** for a reason)
> 
> Hope that helps,

Was that explanation sufficient?

-Stefan

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




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

Re: [libvirt] [PATCH 2/8] apparmor, virt-aa-helper: allow /usr/share/OVMF/ too

2017-05-22 Thread Stefan Bader

On 19.05.2017 09:46, Guido Günther wrote:
> Hi Stefan,
> On Thu, May 18, 2017 at 10:53:40AM +0200, Stefan Bader wrote:
>> From: Simon McVittie <s...@debian.org>
>>
>> The split firmware and variables files introduced by
>> https://bugs.debian.org/764918 are in a different directory for some reason.
>> Let the virtual machine read both.
>>
>> Extended by Christian Ehrhardt to generalize FW test (simplifies
>> additional testing on firmware files in future).
> 
> If you want to credit this separately I suggest to split the ode that
> itroduces testfw into one commit (attributed to Christian) and the code
> that adds read access to OVMF into another one (attributed to Simon).

Though Simon already added some testing (just limited to the one addition made
then). I guess I could re-submit Simon's patch as it was and create one
additionally which only changes the testing (for future use). Which then the
next (3/8) uses.

> 
> Cheers,
>  -- Guido
> 
>>
>> Signed-off-by: Christian Ehrhardt <christian.ehrha...@canonical.com>
>> Signed-off-by: Stefan Bader <stefan.ba...@canonical.com>
>> Acked-by: Guido Günther <a...@sigxcpu.org>
>> ---
>>  examples/apparmor/libvirt-qemu |  1 +
>>  src/security/virt-aa-helper.c  |  1 +
>>  tests/virt-aa-helper-test  | 24 
>>  3 files changed, 18 insertions(+), 8 deletions(-)
>>
>> diff --git a/examples/apparmor/libvirt-qemu b/examples/apparmor/libvirt-qemu
>> index a9020aa..e0988bb 100644
>> --- a/examples/apparmor/libvirt-qemu
>> +++ b/examples/apparmor/libvirt-qemu
>> @@ -70,6 +70,7 @@
>>/usr/share/vgabios/** r,
>>/usr/share/seabios/** r,
>>/usr/share/ovmf/** r,
>> +  /usr/share/OVMF/** r,
>>  
>># access PKI infrastructure
>>/etc/pki/libvirt-vnc/** r,
>> diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
>> index d976a00..dd166c2 100644
>> --- a/src/security/virt-aa-helper.c
>> +++ b/src/security/virt-aa-helper.c
>> @@ -512,6 +512,7 @@ valid_path(const char *path, const bool readonly)
>>  "/vmlinuz",
>>  "/initrd",
>>  "/initrd.img",
>> +"/usr/share/OVMF/",  /* for OVMF images */
>>  "/usr/share/ovmf/"   /* for OVMF images */
>>  };
>>  /* override the above with these */
>> diff --git a/tests/virt-aa-helper-test b/tests/virt-aa-helper-test
>> index 68e9399..73f3080 100755
>> --- a/tests/virt-aa-helper-test
>> +++ b/tests/virt-aa-helper-test
>> @@ -145,6 +145,20 @@ testme() {
>>  fi
>>  }
>>  
>> +testfw() {
>> +title="$1"
>> +fwpath="$2"
>> +
>> +if [ -f "$fwpath" ]; then
>> +sed -e "s,###UUID###,$uuid,g"  \
>> +-e "s,###DISK###,$disk1,g" \
>> +-e "s,,> type='pflash'>$fwpath,g" "$template_xml" > "$test_xml"
>> +testme "0" "$title" "-r -u $valid_uuid" "$test_xml"
>> +else
>> +echo "Skipping FW $title test. Could not find $fwpath"
>> +fi
>> +}
>> +
>>  # Expected failures
>>  echo "Expected failures:" >$output
>>  testme "1" "invalid arg" "-z"
>> @@ -291,14 +305,8 @@ sed -e "s,###UUID###,$uuid,g" -e 
>> "s,###DISK###,$disk1,g" -e "s,,$tm
>>  touch "$tmpdir/kernel"
>>  testme "0" "kernel" "-r -u $valid_uuid" "$test_xml"
>>  
>> -if [ -f /usr/share/ovmf/OVMF.fd ]; then
>> -sed -e "s,###UUID###,$uuid,g"  \
>> --e "s,###DISK###,$disk1,g" \
>> --e "s,,> type='pflash'>/usr/share/ovmf/OVMF.fd,g" "$template_xml" > 
>> "$test_xml"
>> -testme "0" "ovmf" "-r -u $valid_uuid" "$test_xml"
>> -else
>> -echo "Skipping OVMF test. Could not find /usr/share/ovmf/OVMF.fd"
>> -fi
>> +testfw "ovmf (old path)" "/usr/share/ovmf/OVMF.fd"
>> +testfw "OVMF (new path)" "/usr/share/OVMF/OVMF_CODE.fd"
>>  
>>  sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e 
>> "s,,$tmpdir/initrd,g" "$template_xml" > 
>> "$test_xml"
>>  touch "$tmpdir/initrd"
>> -- 
>> 2.7.4
>>
>> --
>> libvir-list mailing list
>> libvir-list@redhat.com
>> https://www.redhat.com/mailman/listinfo/libvir-list
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
> 




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

Re: [libvirt] [PATCH 01/10] virt-aa-helper: Ask for no deny rule for readonly disk elements

2017-05-22 Thread Stefan Bader

On 19.05.2017 13:13, Guido Günther wrote:
> On Fri, May 19, 2017 at 11:18:18AM +0200, Christian Ehrhardt wrote:
>> On Fri, May 19, 2017 at 10:03 AM, Guido Günther  wrote:
>>
>>> But if we aim for a profile replace on blockcommit [1] the would't matter
>>> since the whole profile would get replaced, wouldn't it?
>>>
>>
>> Since this is based on [1][2] looping in Cédric here to share some old
>> explaiantions.
>> See especially [1] for some reasoning for 'R' in general.
>>
>> [1]:
>> http://libvirt.org/git/?p=libvirt.git;a=commit;h=c726af2d5a2248f0dad01201b2fc5231fbd4c20f
>> [2]:
>> http://libvirt.org/git/?p=libvirt.git;a=commit;h=cedd2ab28262db62976b351dbf2a0f8d9f88ca9e
> 
> Let me try to explain why I don't consider this to be a proper fix:
> 
> virsh blockcommit is invoked this leads to:
> 
> 1.) qemuDomainBlockCommit ->
> 2.) qemuDomainDiskChainElementPrepare ->
> 3.) qemuSecuritySetImageLabel ->
> 4.) AppArmorSetSecurityImageLabel (triggers profile reload only) ->
> 5.) virt-aa-helper does the profile reload ->
> 6.) failure since the image has an explicit deny rule
> 
> The path in question tries to fix this at 5.) by not adding a deny write
> rule at all but the place to fix this is 4.) since
> AppArmorSetSecurityImageLabel does not take the virStorageSourcePtr src
> element into account to create a virDomainDefPtr based on def that marks
> the image in question as 'rw' but "only" reloads the profile.


Ok, I think we got the drift but need more time to ponder about that. Christian
created a tracking bug for us and I move it to the needs-more-thinking section.

-Stefan


> 
> Cheers,
>  -- Guido
> 




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

Re: [libvirt] [PATCH 7/8] appmor, libvirt-qemu: Add 9p support

2017-05-19 Thread Stefan Bader

On 18.05.2017 21:40, Serge E. Hallyn wrote:
> Quoting Guido Günther (a...@sigxcpu.org):
>> On Thu, May 18, 2017 at 11:21:54AM -0500, Serge E. Hallyn wrote:
>>> Mind you I'm not crazy about this.  If this could be toggled with a
>>> default-off config option that would seem better than always giving
>>> these caps to libvirt-qemu.
>>
>> virt-aa-helper could add these if it detects a 9pfs file system. That
>> would be better than always adding it.
> 
> Agreed

Ok, so at least for now, actually all 9p related changes should not be
considered. Does the rest look ok (in particular 1/8 with the additional
explanation)?

-Stefan

> 
>> Cheers,
>>  -- Guido
>>
>>>
>>> Quoting Stefan Bader (stefan.ba...@canonical.com):
>>>> From: Serge Hallyn <serge.hal...@ubuntu.com>
>>>>
>>>> Add fowner and fsetid to libvirt-qemu profile.
>>>>
>>>> Bug-Ubuntu: https://bugs.launchpad.net/bugs/1378434
>>>>
>>>> Signed-off-by: Christian Ehrhardt <christian.ehrha...@canonical.com>
>>>> Signed-off-by: Stefan Bader <stefan.ba...@canonical.com>
>>>> ---
>>>>  examples/apparmor/libvirt-qemu | 4 
>>>>  1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/examples/apparmor/libvirt-qemu 
>>>> b/examples/apparmor/libvirt-qemu
>>>> index 89466c9..f04ce04 100644
>>>> --- a/examples/apparmor/libvirt-qemu
>>>> +++ b/examples/apparmor/libvirt-qemu
>>>> @@ -13,6 +13,10 @@
>>>>capability setgid,
>>>>capability setuid,
>>>>  
>>>> +  # for 9p
>>>> +  capability fsetid,
>>>> +  capability fowner,
>>>> +
>>>>network inet stream,
>>>>network inet6 stream,
>>>>  
>>>> -- 
>>>> 2.7.4
>>>
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
> 




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

[libvirt] [PATCH 6/8] apparmor: include local apparmor profiles

2017-05-18 Thread Stefan Bader

From: Felix Geyer <fge...@debian.org>

Local overrides is a feature Debian/Ubuntu libvirt provided for a while.
This allows the user to have a non-conffile that he can use to extend the
package delivered rules with extra content matching his special case.

This change adds the include directives to the apparmor profiles
for virt-aa-helper and libvirtd.

Additionally extended the build environment to carry template local
profiles and install them into the correct places. Without that the
include directives would prevent the profile from loading.

Signed-off-by: Christian Ehrhardt <christian.ehrha...@canonical.com>
Signed-off-by: Stefan Bader <stefan.ba...@canonical.com>
Acked-by: Jamie Strandboge <ja...@canonical.com>
---
 examples/Makefile.am   | 14 ++
 examples/apparmor/local-usr.lib.libvirt.virt-aa-helper |  2 ++
 examples/apparmor/local-usr.sbin.libvirtd  |  2 ++
 examples/apparmor/usr.lib.libvirt.virt-aa-helper   |  3 +++
 examples/apparmor/usr.sbin.libvirtd|  3 +++
 5 files changed, 24 insertions(+)
 create mode 100644 examples/apparmor/local-usr.lib.libvirt.virt-aa-helper
 create mode 100644 examples/apparmor/local-usr.sbin.libvirtd

diff --git a/examples/Makefile.am b/examples/Makefile.am
index 2956e14..16c7bf6 100644
--- a/examples/Makefile.am
+++ b/examples/Makefile.am
@@ -25,6 +25,8 @@ EXTRA_DIST = \
apparmor/libvirt-lxc \
apparmor/usr.lib.libvirt.virt-aa-helper \
apparmor/usr.sbin.libvirtd \
+   apparmor/local-usr.sbin.libvirtd \
+   apparmor/local-usr.lib.libvirt.virt-aa-helper \
lxcconvert/virt-lxc-convert \
polkit/libvirt-acl.rules \
$(wildcard $(srcdir)/systemtap/*.stp) \
@@ -74,6 +76,18 @@ apparmor_DATA = \
apparmor/usr.sbin.libvirtd \
$(NULL)
 
+localdir = $(apparmordir)/local
+local_DATA = \
+   apparmor/local-usr.sbin.libvirtd \
+   apparmor/local-usr.lib.libvirt.virt-aa-helper \
+   $(NULL)
+
+install-data-hook:
+   mv $(DESTDIR)$(localdir)/local-usr.sbin.libvirtd \
+  $(DESTDIR)$(localdir)/usr.sbin.libvirtd
+   mv $(DESTDIR)$(localdir)/local-usr.lib.libvirt.virt-aa-helper \
+  $(DESTDIR)$(localdir)/usr.lib.libvirt.virt-aa-helper
+
 abstractionsdir = $(apparmordir)/abstractions
 abstractions_DATA = \
apparmor/libvirt-qemu \
diff --git a/examples/apparmor/local-usr.lib.libvirt.virt-aa-helper 
b/examples/apparmor/local-usr.lib.libvirt.virt-aa-helper
new file mode 100644
index 000..82c9c39
--- /dev/null
+++ b/examples/apparmor/local-usr.lib.libvirt.virt-aa-helper
@@ -0,0 +1,2 @@
+# Site-specific additions and overrides for usr.lib.libvirt.virt-aa-helper.
+# For more details, please see /etc/apparmor.d/local/README.
diff --git a/examples/apparmor/local-usr.sbin.libvirtd 
b/examples/apparmor/local-usr.sbin.libvirtd
new file mode 100644
index 000..6e19f20
--- /dev/null
+++ b/examples/apparmor/local-usr.sbin.libvirtd
@@ -0,0 +1,2 @@
+# Site-specific additions and overrides for usr.sbin.libvirtd.
+# For more details, please see /etc/apparmor.d/local/README.
diff --git a/examples/apparmor/usr.lib.libvirt.virt-aa-helper 
b/examples/apparmor/usr.lib.libvirt.virt-aa-helper
index 012080c..93ba74e 100644
--- a/examples/apparmor/usr.lib.libvirt.virt-aa-helper
+++ b/examples/apparmor/usr.lib.libvirt.virt-aa-helper
@@ -56,4 +56,7 @@ profile virt-aa-helper 
/usr/{lib,lib64}/libvirt/virt-aa-helper {
   /**.vmdk r,
   /**.[iI][sS][oO] r,
   /**/disk{,.*} r,
+
+  # Site-specific additions and overrides. See local/README for details.
+  #include 
 }
diff --git a/examples/apparmor/usr.sbin.libvirtd 
b/examples/apparmor/usr.sbin.libvirtd
index 353b039..c37d5ee 100644
--- a/examples/apparmor/usr.sbin.libvirtd
+++ b/examples/apparmor/usr.sbin.libvirtd
@@ -85,4 +85,7 @@
 
/usr/{lib,lib64,lib/qemu,libexec}/qemu-bridge-helper rmix,
   }
+
+  # Site-specific additions and overrides. See local/README for details.
+  #include 
 }
-- 
2.7.4

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

[libvirt] [PATCH 7/8] appmor, libvirt-qemu: Add 9p support

2017-05-18 Thread Stefan Bader

From: Serge Hallyn <serge.hal...@ubuntu.com>

Add fowner and fsetid to libvirt-qemu profile.

Bug-Ubuntu: https://bugs.launchpad.net/bugs/1378434

Signed-off-by: Christian Ehrhardt <christian.ehrha...@canonical.com>
Signed-off-by: Stefan Bader <stefan.ba...@canonical.com>
---
 examples/apparmor/libvirt-qemu | 4 
 1 file changed, 4 insertions(+)

diff --git a/examples/apparmor/libvirt-qemu b/examples/apparmor/libvirt-qemu
index 89466c9..f04ce04 100644
--- a/examples/apparmor/libvirt-qemu
+++ b/examples/apparmor/libvirt-qemu
@@ -13,6 +13,10 @@
   capability setgid,
   capability setuid,
 
+  # for 9p
+  capability fsetid,
+  capability fowner,
+
   network inet stream,
   network inet6 stream,
 
-- 
2.7.4

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

[libvirt] [PATCH 1/8] virt-aa-helper: Ask for no deny rule for readonly disk elements

2017-05-18 Thread Stefan Bader

From: Serge Hallyn <serge.hal...@ubuntu.com>

Just because a disk element only requests read access doesn't mean
there may not be another readwrite request.

Using 'R' when creating the apparmor rule will prevent an implicit
write-deny rule to be created alongside. This does not mean write
is allowed but it would cause a denial message and probably more
relevant, allows to add write access later.

Bug-Ubuntu: https://bugs.launchpad.net/bugs/1554031

Signed-off-by: Christian Ehrhardt <christian.ehrha...@canonical.com>
Signed-off-by: Stefan Bader <stefan.ba...@canonical.com>
---
 src/security/virt-aa-helper.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
index 5f5d1cd..d976a00 100644
--- a/src/security/virt-aa-helper.c
+++ b/src/security/virt-aa-helper.c
@@ -888,11 +888,11 @@ add_file_path(virDomainDiskDefPtr disk,
 
 if (depth == 0) {
 if (disk->src->readonly)
-ret = vah_add_file(buf, path, "r");
+ret = vah_add_file(buf, path, "R");
 else
 ret = vah_add_file(buf, path, "rw");
 } else {
-ret = vah_add_file(buf, path, "r");
+ret = vah_add_file(buf, path, "R");
 }
 
 if (ret != 0)
-- 
2.7.4

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

[libvirt] [PATCH 3/8] apparmor, virt-aa-helper: Allow aarch64 UEFI.

2017-05-18 Thread Stefan Bader

From: William Grant <wgr...@ubuntu.com>

Allow access to aarch64 UEFI images.

Signed-off-by: Christian Ehrhardt <christian.ehrha...@canonical.com>
Signed-off-by: Stefan Bader <stefan.ba...@canonical.com>
Acked-by: Guido Günther <a...@sigxcpu.org>
---
 examples/apparmor/libvirt-qemu | 2 ++
 src/security/virt-aa-helper.c  | 4 +++-
 tests/virt-aa-helper-test  | 2 ++
 3 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/examples/apparmor/libvirt-qemu b/examples/apparmor/libvirt-qemu
index e0988bb..89466c9 100644
--- a/examples/apparmor/libvirt-qemu
+++ b/examples/apparmor/libvirt-qemu
@@ -71,6 +71,8 @@
   /usr/share/seabios/** r,
   /usr/share/ovmf/** r,
   /usr/share/OVMF/** r,
+  /usr/share/AAVMF/** r,
+  /usr/share/qemu-efi/** r,
 
   # access PKI infrastructure
   /etc/pki/libvirt-vnc/** r,
diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
index dd166c2..a2d5c21 100644
--- a/src/security/virt-aa-helper.c
+++ b/src/security/virt-aa-helper.c
@@ -513,7 +513,9 @@ valid_path(const char *path, const bool readonly)
 "/initrd",
 "/initrd.img",
 "/usr/share/OVMF/",  /* for OVMF images */
-"/usr/share/ovmf/"   /* for OVMF images */
+"/usr/share/ovmf/",  /* for OVMF images */
+"/usr/share/AAVMF/", /* for AAVMF images */
+"/usr/share/qemu-efi/"   /* for AAVMF images */
 };
 /* override the above with these */
 const char * const override[] = {
diff --git a/tests/virt-aa-helper-test b/tests/virt-aa-helper-test
index 73f3080..51072f6 100755
--- a/tests/virt-aa-helper-test
+++ b/tests/virt-aa-helper-test
@@ -307,6 +307,8 @@ testme "0" "kernel" "-r -u $valid_uuid" "$test_xml"
 
 testfw "ovmf (old path)" "/usr/share/ovmf/OVMF.fd"
 testfw "OVMF (new path)" "/usr/share/OVMF/OVMF_CODE.fd"
+testfw "AAVMF" "/usr/share/AAVMF/AAVMF_CODE.fd"
+testfw "qemu-efi" "/usr/share/qemu-efi/QEMU_EFI.fd"
 
 sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e 
"s,,$tmpdir/initrd,g" "$template_xml" > "$test_xml"
 touch "$tmpdir/initrd"
-- 
2.7.4

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

[libvirt] [PATCH 8/8] apparmor, libvirt-qemu: Add ppc64el related changes

2017-05-18 Thread Stefan Bader

From: Serge Hallyn <serge.hal...@ubuntu.com>

Updates profile to allow running on ppc64el.

Bug-Ubuntu: https://bugs.launchpad.net/bugs/1374554

Signed-off-by: Christian Ehrhardt <christian.ehrha...@canonical.com>
Signed-off-by: Stefan Bader <stefan.ba...@canonical.com>
---
 examples/apparmor/libvirt-qemu | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/examples/apparmor/libvirt-qemu b/examples/apparmor/libvirt-qemu
index f04ce04..2791dfc 100644
--- a/examples/apparmor/libvirt-qemu
+++ b/examples/apparmor/libvirt-qemu
@@ -77,6 +77,7 @@
   /usr/share/OVMF/** r,
   /usr/share/AAVMF/** r,
   /usr/share/qemu-efi/** r,
+  /usr/share/slof/** r,
 
   # access PKI infrastructure
   /etc/pki/libvirt-vnc/** r,
@@ -102,6 +103,7 @@
   /usr/bin/qemu-system-or32 rmix,
   /usr/bin/qemu-system-ppc rmix,
   /usr/bin/qemu-system-ppc64 rmix,
+  /usr/bin/qemu-system-ppc64le rmix,
   /usr/bin/qemu-system-ppcemb rmix,
   /usr/bin/qemu-system-s390x rmix,
   /usr/bin/qemu-system-sh4 rmix,
@@ -158,3 +160,8 @@
   /etc/udev/udev.conf r,
   /sys/bus/ r,
   /sys/class/ r,
+
+  # for ppc device-tree access
+  @{PROC}/device-tree/ r,
+  @{PROC}/device-tree/** r,
+  /sys/firmware/devicetree/** r,
-- 
2.7.4

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

[libvirt] [PATCH 4/8] apparmor, virt-aa-helper: Allow access to libnl-3 config files

2017-05-18 Thread Stefan Bader

From: Felix Geyer <fge...@debian.org>

Allow access to libnl-3 config files

Signed-off-by: Christian Ehrhardt <christian.ehrha...@canonical.com>
Signed-off-by: Stefan Bader <stefan.ba...@canonical.com>
Acked-by: Guido Günther <a...@sigxcpu.org>
---
 examples/apparmor/usr.lib.libvirt.virt-aa-helper | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/examples/apparmor/usr.lib.libvirt.virt-aa-helper 
b/examples/apparmor/usr.lib.libvirt.virt-aa-helper
index 4a8f197..ee53c2c 100644
--- a/examples/apparmor/usr.lib.libvirt.virt-aa-helper
+++ b/examples/apparmor/usr.lib.libvirt.virt-aa-helper
@@ -16,6 +16,8 @@ profile virt-aa-helper 
/usr/{lib,lib64}/libvirt/virt-aa-helper {
   owner @{PROC}/[0-9]*/status r,
   @{PROC}/filesystems r,
 
+  /etc/libnl-3/classid r,
+
   # for hostdev
   /sys/devices/ r,
   /sys/devices/** r,
-- 
2.7.4

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

[libvirt] [PATCH 2/8] apparmor, virt-aa-helper: allow /usr/share/OVMF/ too

2017-05-18 Thread Stefan Bader

From: Simon McVittie <s...@debian.org>

The split firmware and variables files introduced by
https://bugs.debian.org/764918 are in a different directory for some reason.
Let the virtual machine read both.

Extended by Christian Ehrhardt to generalize FW test (simplifies
additional testing on firmware files in future).

Signed-off-by: Christian Ehrhardt <christian.ehrha...@canonical.com>
Signed-off-by: Stefan Bader <stefan.ba...@canonical.com>
Acked-by: Guido Günther <a...@sigxcpu.org>
---
 examples/apparmor/libvirt-qemu |  1 +
 src/security/virt-aa-helper.c  |  1 +
 tests/virt-aa-helper-test  | 24 
 3 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/examples/apparmor/libvirt-qemu b/examples/apparmor/libvirt-qemu
index a9020aa..e0988bb 100644
--- a/examples/apparmor/libvirt-qemu
+++ b/examples/apparmor/libvirt-qemu
@@ -70,6 +70,7 @@
   /usr/share/vgabios/** r,
   /usr/share/seabios/** r,
   /usr/share/ovmf/** r,
+  /usr/share/OVMF/** r,
 
   # access PKI infrastructure
   /etc/pki/libvirt-vnc/** r,
diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
index d976a00..dd166c2 100644
--- a/src/security/virt-aa-helper.c
+++ b/src/security/virt-aa-helper.c
@@ -512,6 +512,7 @@ valid_path(const char *path, const bool readonly)
 "/vmlinuz",
 "/initrd",
 "/initrd.img",
+"/usr/share/OVMF/",  /* for OVMF images */
 "/usr/share/ovmf/"   /* for OVMF images */
 };
 /* override the above with these */
diff --git a/tests/virt-aa-helper-test b/tests/virt-aa-helper-test
index 68e9399..73f3080 100755
--- a/tests/virt-aa-helper-test
+++ b/tests/virt-aa-helper-test
@@ -145,6 +145,20 @@ testme() {
 fi
 }
 
+testfw() {
+title="$1"
+fwpath="$2"
+
+if [ -f "$fwpath" ]; then
+sed -e "s,###UUID###,$uuid,g"  \
+-e "s,###DISK###,$disk1,g" \
+-e "s,,$fwpath,g" "$template_xml" > "$test_xml"
+testme "0" "$title" "-r -u $valid_uuid" "$test_xml"
+else
+echo "Skipping FW $title test. Could not find $fwpath"
+fi
+}
+
 # Expected failures
 echo "Expected failures:" >$output
 testme "1" "invalid arg" "-z"
@@ -291,14 +305,8 @@ sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" 
-e "s,,$tm
 touch "$tmpdir/kernel"
 testme "0" "kernel" "-r -u $valid_uuid" "$test_xml"
 
-if [ -f /usr/share/ovmf/OVMF.fd ]; then
-sed -e "s,###UUID###,$uuid,g"  \
--e "s,###DISK###,$disk1,g" \
--e "s,,/usr/share/ovmf/OVMF.fd,g" "$template_xml" > 
"$test_xml"
-testme "0" "ovmf" "-r -u $valid_uuid" "$test_xml"
-else
-echo "Skipping OVMF test. Could not find /usr/share/ovmf/OVMF.fd"
-fi
+testfw "ovmf (old path)" "/usr/share/ovmf/OVMF.fd"
+testfw "OVMF (new path)" "/usr/share/OVMF/OVMF_CODE.fd"
 
 sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e 
"s,,$tmpdir/initrd,g" "$template_xml" > "$test_xml"
 touch "$tmpdir/initrd"
-- 
2.7.4

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

[libvirt] [PATCH 5/8] apparmor, virt-aa-helper: Explicit denies for host devices

2017-05-18 Thread Stefan Bader

From: Felix Geyer <fge...@debian.org>

Add explicit denies for disk devices to avoid cluttering dmesg with
(acceptable) denials (merged with a second patch which added more
disk device names).

Signed-off-by: Christian Ehrhardt <christian.ehrha...@canonical.com>
Signed-off-by: Stefan Bader <stefan.ba...@canonical.com>
Acked-by: Guido Günther <a...@sigxcpu.org>
---
 examples/apparmor/usr.lib.libvirt.virt-aa-helper | 9 +
 1 file changed, 9 insertions(+)

diff --git a/examples/apparmor/usr.lib.libvirt.virt-aa-helper 
b/examples/apparmor/usr.lib.libvirt.virt-aa-helper
index ee53c2c..012080c 100644
--- a/examples/apparmor/usr.lib.libvirt.virt-aa-helper
+++ b/examples/apparmor/usr.lib.libvirt.virt-aa-helper
@@ -21,6 +21,15 @@ profile virt-aa-helper 
/usr/{lib,lib64}/libvirt/virt-aa-helper {
   # for hostdev
   /sys/devices/ r,
   /sys/devices/** r,
+  deny /dev/sd* r,
+  deny /dev/vd* r,
+  deny /dev/dm-* r,
+  deny /dev/drbd[0-9]* r,
+  deny /dev/dasd* r,
+  deny /dev/nvme* r,
+  deny /dev/zd[0-9]* r,
+  deny /dev/mapper/ r,
+  deny /dev/mapper/* r,
 
   /usr/{lib,lib64}/libvirt/virt-aa-helper mr,
   /{usr/,}sbin/apparmor_parser Ux,
-- 
2.7.4

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

[libvirt] Various apparmor related changes (part 1), version 2

2017-05-18 Thread Stefan Bader

> Over the years there have been a bunch of changes to the
> apparmor profiles and/or virt-aa-helper which have been
> carried in Debian/Ubuntu but never made it upstream.
> 
> In an attempt to clean this up and generally improve the
> apparmor based environments, we (Christian and I) went
> over the changes, cleaned out cruft as much as possible 
> and would be sending out hunks of changes to this list
> for upstream inclusion.
> 
> I hope doing multiple but smaller rounds of submissions
> will make it simpler to get those reviewed and hopefully
> accepted.

For the second version I added acks, merged the patches
related to explicit device denials and local apparmor
profiles, and split the 9p support one (holding back the
part allowing link access for later or to be replaced by
a safer solution).
I also tried to improve the explanation in the description
of patch #1 (virt-aa-helper: Ask for no deny rule for readonly
disk elements).

Thanks,
Stefan

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

Re: [libvirt] [PATCH 09/10] appmor, virt-aa-helper: Add 9p support

2017-05-17 Thread Stefan Bader

On 15.05.2017 18:13, Guido Günther wrote:
> On Mon, May 15, 2017 at 03:23:18PM +0200, Stefan Bader wrote:
>> From: Serge Hallyn <serge.hal...@ubuntu.com>
>>
>> Add fowner and fsetid to libvirt-qemu profile and add link
>> to 9p file options in virt-aa-helper.
>>
>> Bug-Ubuntu: https://bugs.launchpad.net/bugs/1378434
>>
>> Signed-off-by: Christian Ehrhardt <christian.ehrha...@canonical.com>
>> Signed-off-by: Stefan Bader <stefan.ba...@canonical.com>
>> ---
>>  examples/apparmor/libvirt-qemu | 4 
>>  src/security/virt-aa-helper.c  | 2 +-
>>  2 files changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/examples/apparmor/libvirt-qemu b/examples/apparmor/libvirt-qemu
>> index 89466c9..f04ce04 100644
>> --- a/examples/apparmor/libvirt-qemu
>> +++ b/examples/apparmor/libvirt-qemu
>> @@ -13,6 +13,10 @@
>>capability setgid,
>>capability setuid,
>>  
>> +  # for 9p
>> +  capability fsetid,
>> +  capability fowner,
>> +
>>network inet stream,
>>network inet6 stream,
> 
> I would put this into a separate patch.
> 
>>  
>> diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
>> index a2d5c21..667241b 100644
>> --- a/src/security/virt-aa-helper.c
>> +++ b/src/security/virt-aa-helper.c
>> @@ -1108,7 +1108,7 @@ get_files(vahControl * ctl)
>>  /* We don't need to add deny rw rules for readonly mounts,
>>   * this can only lead to troubles when mounting / readonly.
>>   */
>> -if (vah_add_path(, fs->src->path, fs->readonly ? "R" : 
>> "rw", true) != 0)
>> +if (vah_add_path(, fs->src->path, fs->readonly ? "R" : 
>> "rwl", true) != 0)
> 
> Given the recent QEMU 9pfs CVS that allowed to access paths outside src.path
> I would feel better if the rule produces s.th. like
> 
>  link subset src.path/** -> src.path/**,
> 
> instead of allowing links to /**.

I had hoped to gain additional feedback from other people. But will start an
updated submission tomorrow. Splitting this one back into the two halves as
suggested and merging the other (5+6 and 7+8) together.

-Stefan

> Cheers,
>  -- Guido
> 
> 
>>  goto cleanup;
>>  }
>>  }
>> -- 
>> 2.7.4
>>
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
> 




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

Re: [libvirt] [PATCH 01/10] virt-aa-helper: Ask for no deny rule for readonly disk elements

2017-05-15 Thread Stefan Bader

On 15.05.2017 17:48, Guido Günther wrote:
> On Mon, May 15, 2017 at 03:23:10PM +0200, Stefan Bader wrote:
>> From: Serge Hallyn <serge.hal...@ubuntu.com>
>>
>> Just because a disk element only requests read access doesn't mean
>> there may not be another readwrite request.
>>
>> Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/bugs/1554031
> 
> The URL is wrong (drop the "ubuntu" part. From the bug report this looks
> like a workaround:

Darn, thanks for catching this.

> 
>  https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1554031/comments/8
> 
> or am I misreading this? Shouldn't we only change to 'w' on blockcommit?

As I understand it, the 'r' would implicitly add a write deny rule, so it is not
possible to override that. With the 'R' notation only a rule allowing read is
added. Which allows to change to change to 'w' on blockcommit.

-Stefan

> Cheers
>  -- Guido
> 
>>
>> Signed-off-by: Christian Ehrhardt <christian.ehrha...@canonical.com>
>> Signed-off-by: Stefan Bader <stefan.ba...@canonical.com>
>> ---
>>  src/security/virt-aa-helper.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
>> index 5f5d1cd..d976a00 100644
>> --- a/src/security/virt-aa-helper.c
>> +++ b/src/security/virt-aa-helper.c
>> @@ -888,11 +888,11 @@ add_file_path(virDomainDiskDefPtr disk,
>>  
>>  if (depth == 0) {
>>  if (disk->src->readonly)
>> -ret = vah_add_file(buf, path, "r");
>> +ret = vah_add_file(buf, path, "R");
>>  else
>>  ret = vah_add_file(buf, path, "rw");
>>  } else {
>> -ret = vah_add_file(buf, path, "r");
>> +ret = vah_add_file(buf, path, "R");
>>  }
>>  
>>  if (ret != 0)
>> -- 
>> 2.7.4
>>
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
> 




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

Re: [libvirt] [PATCH 06/10] apparmor, virt-aa-helper: Additional explicit denies for host devices

2017-05-15 Thread Stefan Bader

On 15.05.2017 17:56, Guido Günther wrote:
> On Mon, May 15, 2017 at 03:23:15PM +0200, Stefan Bader wrote:
>> From: Christian Ehrhardt <christian.ehrha...@canonical.com>
>>
>> This adds further explicit denies for host devices to silence
>> (acceptable) denial warnings.
>>
>> Signed-off-by: Christian Ehrhardt <christian.ehrha...@canonical.com>
>> Signed-off-by: Stefan Bader <stefan.ba...@canonical.com>
>> ---
>>  examples/apparmor/usr.lib.libvirt.virt-aa-helper | 4 
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/examples/apparmor/usr.lib.libvirt.virt-aa-helper 
>> b/examples/apparmor/usr.lib.libvirt.virt-aa-helper
>> index 7804b72..012080c 100644
>> --- a/examples/apparmor/usr.lib.libvirt.virt-aa-helper
>> +++ b/examples/apparmor/usr.lib.libvirt.virt-aa-helper
>> @@ -24,6 +24,10 @@ profile virt-aa-helper 
>> /usr/{lib,lib64}/libvirt/virt-aa-helper {
>>deny /dev/sd* r,
>>deny /dev/vd* r,
>>deny /dev/dm-* r,
>> +  deny /dev/drbd[0-9]* r,
>> +  deny /dev/dasd* r,
>> +  deny /dev/nvme* r,
>> +  deny /dev/zd[0-9]* r,
>>deny /dev/mapper/ r,
>>deny /dev/mapper/* r,
> 
> This could IMHO be squashed into the previous patch since it just
> extends the list.

I agree. another case of being not certain how to proceed with changes from
different origins.

-Stefan

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




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

Re: [libvirt] [PATCH 07/10] apparmor: include local apparmor profiles

2017-05-15 Thread Stefan Bader

On 15.05.2017 16:30, Jamie Strandboge wrote:
> On Mon, 2017-05-15 at 09:28 -0500, Jamie Strandboge wrote:
>> On Mon, 2017-05-15 at 15:23 +0200, Stefan Bader wrote:
>>> From: Felix Geyer <fge...@debian.org>
>>>
>>> Local overrides is a feature Debian/Ubuntu libvirt provided for a while.
>>> This allows the user to have a non-conffile that he can use to extend the
>>> package delivered rules with extra content matching his special case.
>>>
>>> This change adds the include directives to the apparmor profiles
>>> for virt-aa-helper and libvirtd.
>>>
>>
>> I'm fine with this change but it is important to understand that
>> /etc/apparmor.d/local/usr.sbin.libvirtd must exist otherwise the profile will
>> fail to load. In Debian/Ubuntu we use dh_apparmor which takes care of this 
>> for
>> us. If this is upstreamed, then wherever install of the profile happens or is
>> documented, then the local changes file needs to also be 
>> installed/documented.
>> Other non-deb distributions might not like this extra file, so it is possible
>> this may be a Debian and its derivatives thing
>>
> 
> Oh heh, I see you adjusted the Makefile.am for this in 08. Thanks!

Yeah, I guess it could make sense to merge those two changes into one. I was
just hesitating initially as the first part came via Debian and the latter is
and extension I did. Admittedly it is not completely consistent as I did merge
for other things.

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




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

[libvirt] [PATCH 10/10] apparmor, libvirt-qemu: Add ppc related changes

2017-05-15 Thread Stefan Bader

From: Serge Hallyn <serge.hal...@ubuntu.com>

Updates profile to allow running on ppc64el.

Bug-Ubuntu: https://bugs.launchpad.net/bugs/1374554

Signed-off-by: Christian Ehrhardt <christian.ehrha...@canonical.com>
Signed-off-by: Stefan Bader <stefan.ba...@canonical.com>
---
 examples/apparmor/libvirt-qemu | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/examples/apparmor/libvirt-qemu b/examples/apparmor/libvirt-qemu
index f04ce04..2791dfc 100644
--- a/examples/apparmor/libvirt-qemu
+++ b/examples/apparmor/libvirt-qemu
@@ -77,6 +77,7 @@
   /usr/share/OVMF/** r,
   /usr/share/AAVMF/** r,
   /usr/share/qemu-efi/** r,
+  /usr/share/slof/** r,
 
   # access PKI infrastructure
   /etc/pki/libvirt-vnc/** r,
@@ -102,6 +103,7 @@
   /usr/bin/qemu-system-or32 rmix,
   /usr/bin/qemu-system-ppc rmix,
   /usr/bin/qemu-system-ppc64 rmix,
+  /usr/bin/qemu-system-ppc64le rmix,
   /usr/bin/qemu-system-ppcemb rmix,
   /usr/bin/qemu-system-s390x rmix,
   /usr/bin/qemu-system-sh4 rmix,
@@ -158,3 +160,8 @@
   /etc/udev/udev.conf r,
   /sys/bus/ r,
   /sys/class/ r,
+
+  # for ppc device-tree access
+  @{PROC}/device-tree/ r,
+  @{PROC}/device-tree/** r,
+  /sys/firmware/devicetree/** r,
-- 
2.7.4

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

[libvirt] [PATCH 02/10] apparmor, virt-aa-helper: allow /usr/share/OVMF/ too

2017-05-15 Thread Stefan Bader

From: Simon McVittie <s...@debian.org>

The split firmware and variables files introduced by
https://bugs.debian.org/764918 are in a different directory for some reason.
Let the virtual machine read both.

Extended by Christian Ehrhardt to generalize FW test (simplifies
additional testing on firmware files in future).

Signed-off-by: Christian Ehrhardt <christian.ehrha...@canonical.com>
Signed-off-by: Stefan Bader <stefan.ba...@canonical.com>
---
 examples/apparmor/libvirt-qemu |  1 +
 src/security/virt-aa-helper.c  |  1 +
 tests/virt-aa-helper-test  | 24 
 3 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/examples/apparmor/libvirt-qemu b/examples/apparmor/libvirt-qemu
index a9020aa..e0988bb 100644
--- a/examples/apparmor/libvirt-qemu
+++ b/examples/apparmor/libvirt-qemu
@@ -70,6 +70,7 @@
   /usr/share/vgabios/** r,
   /usr/share/seabios/** r,
   /usr/share/ovmf/** r,
+  /usr/share/OVMF/** r,
 
   # access PKI infrastructure
   /etc/pki/libvirt-vnc/** r,
diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
index d976a00..dd166c2 100644
--- a/src/security/virt-aa-helper.c
+++ b/src/security/virt-aa-helper.c
@@ -512,6 +512,7 @@ valid_path(const char *path, const bool readonly)
 "/vmlinuz",
 "/initrd",
 "/initrd.img",
+"/usr/share/OVMF/",  /* for OVMF images */
 "/usr/share/ovmf/"   /* for OVMF images */
 };
 /* override the above with these */
diff --git a/tests/virt-aa-helper-test b/tests/virt-aa-helper-test
index 68e9399..73f3080 100755
--- a/tests/virt-aa-helper-test
+++ b/tests/virt-aa-helper-test
@@ -145,6 +145,20 @@ testme() {
 fi
 }
 
+testfw() {
+title="$1"
+fwpath="$2"
+
+if [ -f "$fwpath" ]; then
+sed -e "s,###UUID###,$uuid,g"  \
+-e "s,###DISK###,$disk1,g" \
+-e "s,,$fwpath,g" "$template_xml" > "$test_xml"
+testme "0" "$title" "-r -u $valid_uuid" "$test_xml"
+else
+echo "Skipping FW $title test. Could not find $fwpath"
+fi
+}
+
 # Expected failures
 echo "Expected failures:" >$output
 testme "1" "invalid arg" "-z"
@@ -291,14 +305,8 @@ sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" 
-e "s,,$tm
 touch "$tmpdir/kernel"
 testme "0" "kernel" "-r -u $valid_uuid" "$test_xml"
 
-if [ -f /usr/share/ovmf/OVMF.fd ]; then
-sed -e "s,###UUID###,$uuid,g"  \
--e "s,###DISK###,$disk1,g" \
--e "s,,/usr/share/ovmf/OVMF.fd,g" "$template_xml" > 
"$test_xml"
-testme "0" "ovmf" "-r -u $valid_uuid" "$test_xml"
-else
-echo "Skipping OVMF test. Could not find /usr/share/ovmf/OVMF.fd"
-fi
+testfw "ovmf (old path)" "/usr/share/ovmf/OVMF.fd"
+testfw "OVMF (new path)" "/usr/share/OVMF/OVMF_CODE.fd"
 
 sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e 
"s,,$tmpdir/initrd,g" "$template_xml" > "$test_xml"
 touch "$tmpdir/initrd"
-- 
2.7.4

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

[libvirt] [PATCH 08/10] apparmor: provide local override templates

2017-05-15 Thread Stefan Bader

Local overrides is a feature Debian/Ubuntu libvirt provided for a while.
This allows the user to have a non-conffile that he can use to extend the
package delivered rules with extra content matching his special case.

This change provides override templates which the user can extend
and modifies the makefile template to include those when installing
the apparmor profiles.

Signed-off-by: Christian Ehrhardt <christian.ehrha...@canonical.com>
Signed-off-by: Stefan Bader <stefan.ba...@canonical.com>
---
 examples/Makefile.am   | 14 ++
 examples/apparmor/local-usr.lib.libvirt.virt-aa-helper |  2 ++
 examples/apparmor/local-usr.sbin.libvirtd  |  2 ++
 3 files changed, 18 insertions(+)
 create mode 100644 examples/apparmor/local-usr.lib.libvirt.virt-aa-helper
 create mode 100644 examples/apparmor/local-usr.sbin.libvirtd

diff --git a/examples/Makefile.am b/examples/Makefile.am
index 2956e14..16c7bf6 100644
--- a/examples/Makefile.am
+++ b/examples/Makefile.am
@@ -25,6 +25,8 @@ EXTRA_DIST = \
apparmor/libvirt-lxc \
apparmor/usr.lib.libvirt.virt-aa-helper \
apparmor/usr.sbin.libvirtd \
+   apparmor/local-usr.sbin.libvirtd \
+   apparmor/local-usr.lib.libvirt.virt-aa-helper \
lxcconvert/virt-lxc-convert \
polkit/libvirt-acl.rules \
$(wildcard $(srcdir)/systemtap/*.stp) \
@@ -74,6 +76,18 @@ apparmor_DATA = \
apparmor/usr.sbin.libvirtd \
$(NULL)
 
+localdir = $(apparmordir)/local
+local_DATA = \
+   apparmor/local-usr.sbin.libvirtd \
+   apparmor/local-usr.lib.libvirt.virt-aa-helper \
+   $(NULL)
+
+install-data-hook:
+   mv $(DESTDIR)$(localdir)/local-usr.sbin.libvirtd \
+  $(DESTDIR)$(localdir)/usr.sbin.libvirtd
+   mv $(DESTDIR)$(localdir)/local-usr.lib.libvirt.virt-aa-helper \
+  $(DESTDIR)$(localdir)/usr.lib.libvirt.virt-aa-helper
+
 abstractionsdir = $(apparmordir)/abstractions
 abstractions_DATA = \
apparmor/libvirt-qemu \
diff --git a/examples/apparmor/local-usr.lib.libvirt.virt-aa-helper 
b/examples/apparmor/local-usr.lib.libvirt.virt-aa-helper
new file mode 100644
index 000..82c9c39
--- /dev/null
+++ b/examples/apparmor/local-usr.lib.libvirt.virt-aa-helper
@@ -0,0 +1,2 @@
+# Site-specific additions and overrides for usr.lib.libvirt.virt-aa-helper.
+# For more details, please see /etc/apparmor.d/local/README.
diff --git a/examples/apparmor/local-usr.sbin.libvirtd 
b/examples/apparmor/local-usr.sbin.libvirtd
new file mode 100644
index 000..6e19f20
--- /dev/null
+++ b/examples/apparmor/local-usr.sbin.libvirtd
@@ -0,0 +1,2 @@
+# Site-specific additions and overrides for usr.sbin.libvirtd.
+# For more details, please see /etc/apparmor.d/local/README.
-- 
2.7.4

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

[libvirt] [PATCH 03/10] apparmor, virt-aa-helper: Allow aarch64 UEFI.

2017-05-15 Thread Stefan Bader

From: William Grant <wgr...@ubuntu.com>

Allow access to aarch64 UEFI images.

Signed-off-by: Christian Ehrhardt <christian.ehrha...@canonical.com>
Signed-off-by: Stefan Bader <stefan.ba...@canonical.com>
---
 examples/apparmor/libvirt-qemu | 2 ++
 src/security/virt-aa-helper.c  | 4 +++-
 tests/virt-aa-helper-test  | 2 ++
 3 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/examples/apparmor/libvirt-qemu b/examples/apparmor/libvirt-qemu
index e0988bb..89466c9 100644
--- a/examples/apparmor/libvirt-qemu
+++ b/examples/apparmor/libvirt-qemu
@@ -71,6 +71,8 @@
   /usr/share/seabios/** r,
   /usr/share/ovmf/** r,
   /usr/share/OVMF/** r,
+  /usr/share/AAVMF/** r,
+  /usr/share/qemu-efi/** r,
 
   # access PKI infrastructure
   /etc/pki/libvirt-vnc/** r,
diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
index dd166c2..a2d5c21 100644
--- a/src/security/virt-aa-helper.c
+++ b/src/security/virt-aa-helper.c
@@ -513,7 +513,9 @@ valid_path(const char *path, const bool readonly)
 "/initrd",
 "/initrd.img",
 "/usr/share/OVMF/",  /* for OVMF images */
-"/usr/share/ovmf/"   /* for OVMF images */
+"/usr/share/ovmf/",  /* for OVMF images */
+"/usr/share/AAVMF/", /* for AAVMF images */
+"/usr/share/qemu-efi/"   /* for AAVMF images */
 };
 /* override the above with these */
 const char * const override[] = {
diff --git a/tests/virt-aa-helper-test b/tests/virt-aa-helper-test
index 73f3080..51072f6 100755
--- a/tests/virt-aa-helper-test
+++ b/tests/virt-aa-helper-test
@@ -307,6 +307,8 @@ testme "0" "kernel" "-r -u $valid_uuid" "$test_xml"
 
 testfw "ovmf (old path)" "/usr/share/ovmf/OVMF.fd"
 testfw "OVMF (new path)" "/usr/share/OVMF/OVMF_CODE.fd"
+testfw "AAVMF" "/usr/share/AAVMF/AAVMF_CODE.fd"
+testfw "qemu-efi" "/usr/share/qemu-efi/QEMU_EFI.fd"
 
 sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e 
"s,,$tmpdir/initrd,g" "$template_xml" > "$test_xml"
 touch "$tmpdir/initrd"
-- 
2.7.4

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

[libvirt] [PATCH 09/10] appmor, virt-aa-helper: Add 9p support

2017-05-15 Thread Stefan Bader

From: Serge Hallyn <serge.hal...@ubuntu.com>

Add fowner and fsetid to libvirt-qemu profile and add link
to 9p file options in virt-aa-helper.

Bug-Ubuntu: https://bugs.launchpad.net/bugs/1378434

Signed-off-by: Christian Ehrhardt <christian.ehrha...@canonical.com>
Signed-off-by: Stefan Bader <stefan.ba...@canonical.com>
---
 examples/apparmor/libvirt-qemu | 4 
 src/security/virt-aa-helper.c  | 2 +-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/examples/apparmor/libvirt-qemu b/examples/apparmor/libvirt-qemu
index 89466c9..f04ce04 100644
--- a/examples/apparmor/libvirt-qemu
+++ b/examples/apparmor/libvirt-qemu
@@ -13,6 +13,10 @@
   capability setgid,
   capability setuid,
 
+  # for 9p
+  capability fsetid,
+  capability fowner,
+
   network inet stream,
   network inet6 stream,
 
diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
index a2d5c21..667241b 100644
--- a/src/security/virt-aa-helper.c
+++ b/src/security/virt-aa-helper.c
@@ -1108,7 +1108,7 @@ get_files(vahControl * ctl)
 /* We don't need to add deny rw rules for readonly mounts,
  * this can only lead to troubles when mounting / readonly.
  */
-if (vah_add_path(, fs->src->path, fs->readonly ? "R" : "rw", 
true) != 0)
+if (vah_add_path(, fs->src->path, fs->readonly ? "R" : "rwl", 
true) != 0)
 goto cleanup;
 }
 }
-- 
2.7.4

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

[libvirt] [PATCH 04/10] apparmor, virt-aa-helper: Allow access to libnl-3 config files

2017-05-15 Thread Stefan Bader

From: Felix Geyer <fge...@debian.org>

Allow access to libnl-3 config files

Signed-off-by: Christian Ehrhardt <christian.ehrha...@canonical.com>
Signed-off-by: Stefan Bader <stefan.ba...@canonical.com>
---
 examples/apparmor/usr.lib.libvirt.virt-aa-helper | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/examples/apparmor/usr.lib.libvirt.virt-aa-helper 
b/examples/apparmor/usr.lib.libvirt.virt-aa-helper
index 4a8f197..ee53c2c 100644
--- a/examples/apparmor/usr.lib.libvirt.virt-aa-helper
+++ b/examples/apparmor/usr.lib.libvirt.virt-aa-helper
@@ -16,6 +16,8 @@ profile virt-aa-helper 
/usr/{lib,lib64}/libvirt/virt-aa-helper {
   owner @{PROC}/[0-9]*/status r,
   @{PROC}/filesystems r,
 
+  /etc/libnl-3/classid r,
+
   # for hostdev
   /sys/devices/ r,
   /sys/devices/** r,
-- 
2.7.4

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

[libvirt] [PATCH 06/10] apparmor, virt-aa-helper: Additional explicit denies for host devices

2017-05-15 Thread Stefan Bader

From: Christian Ehrhardt <christian.ehrha...@canonical.com>

This adds further explicit denies for host devices to silence
(acceptable) denial warnings.

Signed-off-by: Christian Ehrhardt <christian.ehrha...@canonical.com>
Signed-off-by: Stefan Bader <stefan.ba...@canonical.com>
---
 examples/apparmor/usr.lib.libvirt.virt-aa-helper | 4 
 1 file changed, 4 insertions(+)

diff --git a/examples/apparmor/usr.lib.libvirt.virt-aa-helper 
b/examples/apparmor/usr.lib.libvirt.virt-aa-helper
index 7804b72..012080c 100644
--- a/examples/apparmor/usr.lib.libvirt.virt-aa-helper
+++ b/examples/apparmor/usr.lib.libvirt.virt-aa-helper
@@ -24,6 +24,10 @@ profile virt-aa-helper 
/usr/{lib,lib64}/libvirt/virt-aa-helper {
   deny /dev/sd* r,
   deny /dev/vd* r,
   deny /dev/dm-* r,
+  deny /dev/drbd[0-9]* r,
+  deny /dev/dasd* r,
+  deny /dev/nvme* r,
+  deny /dev/zd[0-9]* r,
   deny /dev/mapper/ r,
   deny /dev/mapper/* r,
 
-- 
2.7.4

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

[libvirt] [PATCH 07/10] apparmor: include local apparmor profiles

2017-05-15 Thread Stefan Bader

From: Felix Geyer <fge...@debian.org>

Local overrides is a feature Debian/Ubuntu libvirt provided for a while.
This allows the user to have a non-conffile that he can use to extend the
package delivered rules with extra content matching his special case.

This change adds the include directives to the apparmor profiles
for virt-aa-helper and libvirtd.

Signed-off-by: Christian Ehrhardt <christian.ehrha...@canonical.com>
Signed-off-by: Stefan Bader <stefan.ba...@canonical.com>
---
 examples/apparmor/usr.lib.libvirt.virt-aa-helper | 3 +++
 examples/apparmor/usr.sbin.libvirtd  | 3 +++
 2 files changed, 6 insertions(+)

diff --git a/examples/apparmor/usr.lib.libvirt.virt-aa-helper 
b/examples/apparmor/usr.lib.libvirt.virt-aa-helper
index 012080c..93ba74e 100644
--- a/examples/apparmor/usr.lib.libvirt.virt-aa-helper
+++ b/examples/apparmor/usr.lib.libvirt.virt-aa-helper
@@ -56,4 +56,7 @@ profile virt-aa-helper 
/usr/{lib,lib64}/libvirt/virt-aa-helper {
   /**.vmdk r,
   /**.[iI][sS][oO] r,
   /**/disk{,.*} r,
+
+  # Site-specific additions and overrides. See local/README for details.
+  #include 
 }
diff --git a/examples/apparmor/usr.sbin.libvirtd 
b/examples/apparmor/usr.sbin.libvirtd
index 353b039..c37d5ee 100644
--- a/examples/apparmor/usr.sbin.libvirtd
+++ b/examples/apparmor/usr.sbin.libvirtd
@@ -85,4 +85,7 @@
 
/usr/{lib,lib64,lib/qemu,libexec}/qemu-bridge-helper rmix,
   }
+
+  # Site-specific additions and overrides. See local/README for details.
+  #include 
 }
-- 
2.7.4

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

[libvirt] [PATCH 01/10] virt-aa-helper: Ask for no deny rule for readonly disk elements

2017-05-15 Thread Stefan Bader

From: Serge Hallyn <serge.hal...@ubuntu.com>

Just because a disk element only requests read access doesn't mean
there may not be another readwrite request.

Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/bugs/1554031

Signed-off-by: Christian Ehrhardt <christian.ehrha...@canonical.com>
Signed-off-by: Stefan Bader <stefan.ba...@canonical.com>
---
 src/security/virt-aa-helper.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
index 5f5d1cd..d976a00 100644
--- a/src/security/virt-aa-helper.c
+++ b/src/security/virt-aa-helper.c
@@ -888,11 +888,11 @@ add_file_path(virDomainDiskDefPtr disk,
 
 if (depth == 0) {
 if (disk->src->readonly)
-ret = vah_add_file(buf, path, "r");
+ret = vah_add_file(buf, path, "R");
 else
 ret = vah_add_file(buf, path, "rw");
 } else {
-ret = vah_add_file(buf, path, "r");
+ret = vah_add_file(buf, path, "R");
 }
 
 if (ret != 0)
-- 
2.7.4

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

[libvirt] [PATCH 05/10] apparmor, virt-aa-helper: Explicit denies for host devices

2017-05-15 Thread Stefan Bader

From: Felix Geyer <fge...@debian.org>

Add explicit denies for disk devices to avoid cluttering dmesg with
(acceptable) denials.

Signed-off-by: Christian Ehrhardt <christian.ehrha...@canonical.com>
Signed-off-by: Stefan Bader <stefan.ba...@canonical.com>
---
 examples/apparmor/usr.lib.libvirt.virt-aa-helper | 5 +
 1 file changed, 5 insertions(+)

diff --git a/examples/apparmor/usr.lib.libvirt.virt-aa-helper 
b/examples/apparmor/usr.lib.libvirt.virt-aa-helper
index ee53c2c..7804b72 100644
--- a/examples/apparmor/usr.lib.libvirt.virt-aa-helper
+++ b/examples/apparmor/usr.lib.libvirt.virt-aa-helper
@@ -21,6 +21,11 @@ profile virt-aa-helper 
/usr/{lib,lib64}/libvirt/virt-aa-helper {
   # for hostdev
   /sys/devices/ r,
   /sys/devices/** r,
+  deny /dev/sd* r,
+  deny /dev/vd* r,
+  deny /dev/dm-* r,
+  deny /dev/mapper/ r,
+  deny /dev/mapper/* r,
 
   /usr/{lib,lib64}/libvirt/virt-aa-helper mr,
   /{usr/,}sbin/apparmor_parser Ux,
-- 
2.7.4

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

[libvirt] Various apparmor related changes (part 1)

2017-05-15 Thread Stefan Bader

Over the years there have been a bunch of changes to the
apparmor profiles and/or virt-aa-helper which have been
carried in Debian/Ubuntu but never made it upstream.

In an attempt to clean this up and generally improve the
apparmor based environments, we (Christian and I) went
over the changes, cleaned out cruft as much as possible 
and would be sending out hunks of changes to this list
for upstream inclusion.

I hope doing multiple but smaller rounds of submissions
will make it simpler to get those reviewed and hopefully
accepted.

This first batch contains a mix of changes from Debian
and Ubuntu.

Thanks,
Stefan

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

Re: [libvirt] libvirt-guest.sh bug fixes

2016-10-11 Thread Stefan Bader

On 10.10.2016 20:43, Eric Blake wrote:
> On 10/10/2016 11:48 AM, Stefan Bader wrote:
> 
>>> I did not hear about that before. But revisiting things again I think what
>>> happened is that the Xen patch which I had done before (but at that time 
>>> forgot
>>> to submit upstream) was adding quotes there because without, the grep does 
>>> not
>>> work. Back then I did not realize this broke the shutdown as that also had
>>> quotes around the list. All the other users of the list are ok because they 
>>> wrap
>>> the processing into for ... loops and for those newline or space does not 
>>> matter.
>>
>> I think the surprising part is that the calls to virsh like:
>>
>>   list=$(virsh ... list)
>>
>> seem to retain the newline seperator despite having no quotes. Only when 
>> using
>> echo with the unquoted variable seems to do that. That is with dash as sh at 
>> least.
> 
> That's true for ALL shells.  Word splitting does NOT happen during
> variable assignment.  list=$() is _strictly_ equivalent to list="$()",
> no matter the shell.

Funny that with all those years working with shells I never memorized that as a
fact. :) Guess it was enough to adhere to the good habit and just use "$()" all
the time.

So just as a wrap up, the first patch ends up not being a fix in the strict
sense as before the second patch word splitting would happen in the list_guest
function. Luckily it neither breaks anything on its own as the result is the
same whether list was already split or not. Somehow I subconsciously reordered
it to come before the dom0 patch which I had done here before (without noticing
that the list file is then broken for more than one guest to shutdown). Somehow
the big part of adding the grep in the dom0 patch made me miss the additional
quotes around the variable. In hindsight maybe the better way would have been to
add a new line which filters the dom0 uid and reassigns the result to list and
keep the echo line unmodified. But then, the end result now should be working
and robust enough...

-Stefan
> 
> It's just that there are so many other places where $() and "$()" behave
> differently that it's (usually) a good habit to always use "", rather
> than learning the rules on when you NEED them, vs. telling the special
> cases when you DON'T want them apart from the cases where they are optional.
> 

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

Re: [libvirt] libvirt-guest.sh bug fixes

2016-10-10 Thread Stefan Bader

On 10.10.2016 18:32, Stefan Bader wrote:
> On 10.10.2016 17:06, Cole Robinson wrote:
>> On 10/07/2016 03:56 AM, Stefan Bader wrote:
>>> Two small changes, before I forget about submitting them...
>>>
>>> First one affects all environments the same. The list of UIDs which
>>> is generated has each element on a separate line. And using quotes
>>> in the echo preserves those newlines. However the processing assumes
>>> one line per URI and all UIDs separated by spaces. So without dropping
>>> the quotes only one guest will get shutdown/suspended.
>>>
>>> The second change is for Xen environments only. Domain-0 appears in
>>> the list of guests and it is a persistent one. So on shutdown, the
>>> script tries to stop Domain-0 (which is not working) and then waits
>>> the whole timeout for it to stop.
>>>
>>
>> Note these patches were flagged as problematic a few months back:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1335585
>>
>> Stefan, have you heard about that issue? Was it resolved with these versions?
> 
> I did not hear about that before. But revisiting things again I think what
> happened is that the Xen patch which I had done before (but at that time 
> forgot
> to submit upstream) was adding quotes there because without, the grep does not
> work. Back then I did not realize this broke the shutdown as that also had
> quotes around the list. All the other users of the list are ok because they 
> wrap
> the processing into for ... loops and for those newline or space does not 
> matter.

I think the surprising part is that the calls to virsh like:

  list=$(virsh ... list)

seem to retain the newline seperator despite having no quotes. Only when using
echo with the unquoted variable seems to do that. That is with dash as sh at 
least.

> 
> So together with the other patch wich drops the quotes when writing the list
> file this should be resolved. list_guests returns a list of uids (separated by
> newline), the output of the info message is correctly showing names in one 
> line
> separated by komma, and the produced list file had only one line per uri with
> uids separated by spaces.
> 
> -Stefan
>>
>> Thanks,
>> Cole
>>
> 
> 




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

Re: [libvirt] libvirt-guest.sh bug fixes

2016-10-10 Thread Stefan Bader

On 10.10.2016 17:06, Cole Robinson wrote:
> On 10/07/2016 03:56 AM, Stefan Bader wrote:
>> Two small changes, before I forget about submitting them...
>>
>> First one affects all environments the same. The list of UIDs which
>> is generated has each element on a separate line. And using quotes
>> in the echo preserves those newlines. However the processing assumes
>> one line per URI and all UIDs separated by spaces. So without dropping
>> the quotes only one guest will get shutdown/suspended.
>>
>> The second change is for Xen environments only. Domain-0 appears in
>> the list of guests and it is a persistent one. So on shutdown, the
>> script tries to stop Domain-0 (which is not working) and then waits
>> the whole timeout for it to stop.
>>
> 
> Note these patches were flagged as problematic a few months back:
> https://bugzilla.redhat.com/show_bug.cgi?id=1335585
> 
> Stefan, have you heard about that issue? Was it resolved with these versions?

I did not hear about that before. But revisiting things again I think what
happened is that the Xen patch which I had done before (but at that time forgot
to submit upstream) was adding quotes there because without, the grep does not
work. Back then I did not realize this broke the shutdown as that also had
quotes around the list. All the other users of the list are ok because they wrap
the processing into for ... loops and for those newline or space does not 
matter.

So together with the other patch wich drops the quotes when writing the list
file this should be resolved. list_guests returns a list of uids (separated by
newline), the output of the info message is correctly showing names in one line
separated by komma, and the produced list file had only one line per uri with
uids separated by spaces.

-Stefan
> 
> Thanks,
> Cole
> 

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

[libvirt] [PATCH 2/2] tools: Exclude Xen dom0 from libvirt-guests.sh list

2016-10-07 Thread Stefan Bader

With newer versions of libvirt Domain-0 is again visible in the list of
running guests but it should not be considered as a guest for shutdown
or suspend.

Signed-off-by Stefan Bader <stefan.ba...@canonical.com>
---
 tools/libvirt-guests.sh.in | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/libvirt-guests.sh.in b/tools/libvirt-guests.sh.in
index 7380b4b..791d927 100644
--- a/tools/libvirt-guests.sh.in
+++ b/tools/libvirt-guests.sh.in
@@ -121,7 +121,7 @@ list_guests() {
 return 1
 fi
 
-echo $list
+echo "$list" | grep -v ----
 }
 
 # guest_name URI UUID
@@ -539,7 +539,7 @@ gueststatus() {
 for uri in $URIS; do
 set +f
 echo "* $uri URI:"
-retval run_virsh "$uri" list || echo
+retval run_virsh "$uri" list | grep -v "Domain-0" || echo
 done
 set +f
 }
-- 
1.9.1

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

[libvirt] [PATCH 1/2] tools: Ignore newlines in libvirt-guests.sh guest list

2016-10-07 Thread Stefan Bader

The list file expects all guest UUIDs on the same line as the URI
which the guests run on. This does not happen when the list is
echo'ed in quotes. When stripping the quotes, newlines get transformed
into spaces. Without this, only the first guest on the list is actually
handled.

Based on a fix by Omar Siam <si...@gmx.net>

Bug-Ubuntu: http://bugs.launchpad.net/bugs/1591695

Signed-off-by: Stefan Bader <stefan.ba...@canonical.com>
---
 tools/libvirt-guests.sh.in | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/libvirt-guests.sh.in b/tools/libvirt-guests.sh.in
index 7f74b85..7380b4b 100644
--- a/tools/libvirt-guests.sh.in
+++ b/tools/libvirt-guests.sh.in
@@ -499,7 +499,7 @@ stop() {
 fi
 
 if [ -n "$list" ]; then
-echo "$uri" "$list" >>"$LISTFILE"
+echo "$uri" $list >>"$LISTFILE"
 fi
 done
 set +f
-- 
1.9.1

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

[libvirt] libvirt-guest.sh bug fixes

2016-10-07 Thread Stefan Bader

Two small changes, before I forget about submitting them...

First one affects all environments the same. The list of UIDs which
is generated has each element on a separate line. And using quotes
in the echo preserves those newlines. However the processing assumes
one line per URI and all UIDs separated by spaces. So without dropping
the quotes only one guest will get shutdown/suspended.

The second change is for Xen environments only. Domain-0 appears in
the list of guests and it is a persistent one. So on shutdown, the
script tries to stop Domain-0 (which is not working) and then waits
the whole timeout for it to stop.

-Stefan


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

Re: [libvirt] [PATCH/RFC] Add missing delta from Ubuntu to apparmor profiles

2016-05-25 Thread Stefan Bader

On 09.05.2016 21:00, intrigeri wrote:
> Hi,
> 
>> Stefan Bader wrote (20 May 2015 10:11:45 GMT) :
>> intrigeri wrote (15 Jun 2015 15:09:11 GMT) :
>> My (possibly incomplete) records say that I've tested the latest
>> proposed patch set back in February (<85iof8v6j5@boum.org>).
> 
>>> Since I lost most context by now, I will try to find my most recent proposal
>>> again and try to get it moved into present state of packages.
> 
> Ping?

*sigh* yeah (/me look guilty). I am on that now. This time approaching things
from the bottom. So I actually have something that seems somehow working with
the Ubuntu packaging. I just want to get some internal feedback first. But I
should be sending out something around next Monday.
The good thing would be that this is against libvirt 1.3.4 which Serge got
closer to Debian than ever before. So there is hope that it can be tried in
Debian without much problem.

-Stefan
> 
> Cheers,
> 

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

[libvirt] Should external snapshots be possible with type volume image files?

2015-08-28 Thread Stefan Bader

At least up to libvirt version 1.2.16 an external snapshot fails when the image
file (supported type like QCOW2) is  not specified as type='file' by as
type='volume' to a pool that consists of image files (type directory).
The reason there is that the source element of the disk definition does not
contain the full path but only references to pool and volume name. Which would
require an additional indirection in order to find that it actually is a file
and what the path is. And that would make things more complicated.
So my question is whether this should work and needs fixing in code or was never
meant to be used that way. If the latter, then maybe needs a clarification in
the documentation. At least the wiki did read for me as only requiring to be a
image file of a supported type. And we got at least one bug report about it.

Thanks,
Stefan



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

Re: [libvirt] [PATCH/RFC] Add missing delta from Ubuntu to apparmor profiles

2015-05-20 Thread Stefan Bader

On 19.05.2015 11:54, intrigeri wrote:
 Hi Stefan,
 
 any news on what follows? Now that Ubuntu 15.04 has been released,
 perhaps you'll be able to allocate some cycles to it? :)

Hm was there not something which I was waiting for feedback from you? Though I
forgot what exactly that was. And after release is before release, the treadmill
never stops... ;-P
Since I lost most context by now, I will try to find my most recent proposal
again and try to get it moved into present state of packages.

-Stefan

 
 intrigeri wrote (11 Feb 2015 14:58:54 GMT) :
 Hi Stefan and others,
 
 Stefan Bader wrote (21 Oct 2014 11:50:24 GMT) :
 On 20.10.2014 12:48, Stefan Bader wrote:
 On 19.10.2014 17:07, intrigeri wrote:
 Cool, I've tested this. I've imported these two patches in Debian's
 1.2.9-3 quilt series, made the build system use dh-autoreconf (the
 build system in the tarball wants aclocal 1.13, while Debian sid has
 1.14), and added a build-dep on libapparmor-dev to get the needed
 pkg-config file.
 
 I've given a try to your last set of patches. Sorry for the delay.
 Here's what I did:
 
 1. Checkout the Vcs-Git libvirt packaging repo for Debian unstable,
currently at 1.2.9-9
 2. Make the build system use dh-autoreconf as previously
 3. Added the build-dep on libapparmor-dev as previously
 4. Hacked debian/rules to make examples/apparmor/profile-preprocess
(created by your patches) executable before it's executed.
This won't be needed anymore once the patches are upstreamed.
 5. Build in a clean Debian unstable chroot, which now works.
Progress :)
 6. Install the resulting binary packages on a sid system with
a working libvirt setup.
 7. In /etc/libvirt/qemu.conf, set security_driver = apparmor
 8. Restart libvirtd.
 9. Start a VM with virsh or virt-manager
 
 = here's what I see:
 
   error: Failed to start domain tails-dev
   error: internal error: cannot load AppArmor profile 
 'libvirt-14dcf3fa-a4d5-4c5a-82ea-3f624b44c7ef'
 
 And the Journal says:
 
   libvirtd[20351]: internal error: Child process 
 (/usr/lib/libvirt/virt-aa-helper -p 0 -c -u 
 libvirt-14dcf3fa-a4d5-4c5a-82ea-3f624b44c7ef) unexpected exit status 1: 
 virt-aa-helper: error: template does not exist
virt-aa-helper: error: could not create profile
   libvirtd[20351]: internal error: cannot load AppArmor profile 
 'libvirt-14dcf3fa-a4d5-4c5a-82ea-3f624b44c7ef'
 
 So I naively tried to do it by hand:
 
   $ virsh dumpxml tails-dev | sudo /usr/lib/libvirt/virt-aa-helper -p 0 -c 
 -u libvirt-14dcf3fa-a4d5-4c5a-82ea-3f624b44c7ef 
   virt-aa-helper: error: template does not exist
   virt-aa-helper: error: could not create profile
 
 I do have a template in place:
 
   $ cat /etc/apparmor.d/libvirt/TEMPLATE.qemu
   #
   # This profile is for the domain whose UUID matches this file.
   #
 
   #include tunables/global
 
   profile LIBVIRT_TEMPLATE {
 #include abstractions/libvirt-qemu
   }
 
 What other information can I provide, or what else should I test?
 
 Also note that I had to add the following line to
 usr.lib.libvirt.virt-aa-helper, in order to silence an AppArmor denial
 log:
 
   /etc/libnl-3/classid r,
 
 Should this be added to the upstream profile, as is or prefixed by
 deny?
 
 Cheers,
 




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

Re: [libvirt] libxl and non-absolute paths

2015-02-18 Thread Stefan Bader

On 18.02.2015 04:15, Jim Fehlig wrote:
 Stefan Bader wrote:
 Just recently we moved to libvirt 1.2.12 for the next release. Which brought 
 up
 a few problems when working with configs which we and Debian used to have.

 A mild complaint towards the xml validation: it would be really nice of that
 would be a bit more specific about what exactly it complains. It took me a 
 while
 to realize that Extra element os in interleave was trying to tell me
 that the string of the loader element within the os section was not an 
 absolute
 path.

 The issue here is that with libxl, I think the goal was to rather allow the
 library to select the path prefix (like for pygrub where the full path got
 removed recently). But now the xml validation disagrees.

 This would go for bootloader for xenpv and loader (within os) for xenfv. And 
 for
 emulator in the device section. Though for that things are a bit more
 complicated. The libxl driver now calls that with the help option and decides
 from the output whether this is the traditional xen forked qemu or the
 upstream qemu binary. Then it selects the device model depending on that 
 outcome.
   
 
 It only sets the device_model_version
 (LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN vs
 LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL).  device_model is
 directly VIR_STRDUP'ed from def-emulator.

Ok, I did not word that correctly. The main caveat here is that to figure out
the device_model_version it calls out to what is set in def-emulator. So it
needs a full path. Up to libvirt 1.2.8 at least the contents of def-emulator
were actually rather ignored since we ended up with qemu-dm which was not true
at all.

 
 Not sure whether the libxl driver could query libxl for the path prefix.
 
 I asked about that too, but the Xen community preferred a build-time
 approach.  Wei Liu added a pkgconfig module to Xen, enabling build-time
 detection of the paths
 
 commit babeca328413baebfdca366a5b17c06acf4295e8
 Author: Wei Liu wei.l...@citrix.com
 Date:   Fri Jan 9 14:32:18 2015 +
 
 libxl: provide xenlight.pc

 A pkg-config file for libxl. It also contains two variables
 (xenfirmwaredir and libexec_bin) so that tools that are very keen on
 knowing the locations of Xen binaries (say, libvirt) can use them to
 determine the location of the binaries.
 
 We are not yet making use of xenlight.pc in libvirt.  But this work aims
 to improve reporting the correct paths in *capabilities*. It could also
 be used in the xl.cfg to domXML conversion code.

Hm, ok. I guess there is upstream and upstream who might also be doing Debian
(and derivatives of that). Since libxl is part of a version package the
pathnames change between Xen versions. Maybe something to discuss about but not
really here. Reality now causes the full path requirement to become a major pain
in the butt.

 
 Right
 now the most straight forward way seems to move back to a full path for the
 emulator.
 
 Full paths are required as per the documentation.  From
 http://libvirt.org/formatdomain.html#elementsOSBootloader
 
 The content of the |bootloader| element provides a fully qualified path
 to the bootloader executable in the host OS.
 
 Same for emulator.
 
  At least now, by using the standard qemu binary for everything, we got
 a predictable path that does not change with Xen versions. So its possible to
 force migrate over to put /usr/bin/qemu-system-i386 there.

 But for loader and bootloader, do you think it reasonable to change the
 templates from absFilePath to filePath?
   
 
 There's probably a fair bit of existing config with e.g.
 bootlaoderpygrub/bootloader that no longer validates.  ATM, I don't
 have a good answer :-/.  Is correct capabilities information and xl.cfg
 - libvirt.xml conversion, along with better error reporting enough? 
 Should users change their non-validating domXML?

Potentially this has to be magically converged in postparse of libxl. This is
where I currently move from the completely lying qemu-dm to
/usr/lib/qemu-system-i386.

Though this will depend on using xenlight.pc and that depends on it being there.
The change currently only is on the devel master branch. So presumably Xen 4.6
may have it. For the reality of now I guess the best way around the issues is
to either disable validation completely or at least make that
absFilePath-filePath conversion. None of that though is worthy of being 
upstream.

-Stefan
 
From libvirt's perspective, I think full paths, discovered from
 capabilities, should be required.  I'd like to hear Daniel's opinion. 
 He may have considered such cases when enabling the validation.
 
 Regards,
 Jim
 
 -Stefan

 --- libvirt-1.2.12.orig/docs/schemas/domaincommon.rng   2015-01-23 
 12:46:24.
 +++ libvirt-1.2.12/docs/schemas/domaincommon.rng2015-02-13 
 10:00:43.1616
 @@ -258,7 +258,7 @@
  /choice
/attribute
  /optional
 -ref name=absFilePath/
 +ref name=filePath

Re: [libvirt] libxl and non-absolute paths

2015-02-16 Thread Stefan Bader

On 16.02.2015 10:18, Martin Kletzander wrote:
 On Fri, Feb 13, 2015 at 03:20:07PM +0100, Stefan Bader wrote:
 Just recently we moved to libvirt 1.2.12 for the next release. Which brought 
 up
 a few problems when working with configs which we and Debian used to have.

 A mild complaint towards the xml validation: it would be really nice of that
 would be a bit more specific about what exactly it complains. It took me a 
 while
 to realize that Extra element os in interleave was trying to tell me
 that the string of the loader element within the os section was not an 
 absolute
 path.

 The issue here is that with libxl, I think the goal was to rather allow the
 library to select the path prefix (like for pygrub where the full path got
 removed recently). But now the xml validation disagrees.

 This would go for bootloader for xenpv and loader (within os) for xenfv. And 
 for
 emulator in the device section. Though for that things are a bit more
 complicated. The libxl driver now calls that with the help option and decides
 from the output whether this is the traditional xen forked qemu or the
 upstream qemu binary. Then it selects the device model depending on that 
 outcome.
 Not sure whether the libxl driver could query libxl for the path prefix. 
 Right
 now the most straight forward way seems to move back to a full path for the
 emulator. At least now, by using the standard qemu binary for everything, we 
 got
 a predictable path that does not change with Xen versions. So its possible to
 force migrate over to put /usr/bin/qemu-system-i386 there.

 But for loader and bootloader, do you think it reasonable to change the
 templates from absFilePath to filePath?

 
 Maybe stupid question here... How does the string with the prefix look
 like then?  Is it something like bootloaderpygrub:/path/to/loader ?

No, sorry I should probably have added that: in both cases there is only the
binary name in the config and libxl extends things internally.

So bootloaderpygrub/bootloader and loaderhvmloader/loader.

-Stefan

 
 -Stefan

 --- libvirt-1.2.12.orig/docs/schemas/domaincommon.rng   2015-01-23 
 12:46:24.
 +++ libvirt-1.2.12/docs/schemas/domaincommon.rng2015-02-13 
 10:00:43.1616
 @@ -258,7 +258,7 @@
 /choice
   /attribute
 /optional
 -ref name=absFilePath/
 +ref name=filePath/
   /element
 /optional
 optional
 @@ -1060,7 +1060,7 @@
   optional
 element name=bootloader
   choice
 -ref name=absFilePath/
 +ref name=filePath/
 empty/
   /choice
 /element

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




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

[libvirt] libxl and non-absolute paths

2015-02-13 Thread Stefan Bader

Just recently we moved to libvirt 1.2.12 for the next release. Which brought up
a few problems when working with configs which we and Debian used to have.

A mild complaint towards the xml validation: it would be really nice of that
would be a bit more specific about what exactly it complains. It took me a while
to realize that Extra element os in interleave was trying to tell me
that the string of the loader element within the os section was not an absolute
path.

The issue here is that with libxl, I think the goal was to rather allow the
library to select the path prefix (like for pygrub where the full path got
removed recently). But now the xml validation disagrees.

This would go for bootloader for xenpv and loader (within os) for xenfv. And for
emulator in the device section. Though for that things are a bit more
complicated. The libxl driver now calls that with the help option and decides
from the output whether this is the traditional xen forked qemu or the
upstream qemu binary. Then it selects the device model depending on that 
outcome.
Not sure whether the libxl driver could query libxl for the path prefix. Right
now the most straight forward way seems to move back to a full path for the
emulator. At least now, by using the standard qemu binary for everything, we got
a predictable path that does not change with Xen versions. So its possible to
force migrate over to put /usr/bin/qemu-system-i386 there.

But for loader and bootloader, do you think it reasonable to change the
templates from absFilePath to filePath?

-Stefan

--- libvirt-1.2.12.orig/docs/schemas/domaincommon.rng   2015-01-23 12:46:24.
+++ libvirt-1.2.12/docs/schemas/domaincommon.rng2015-02-13 10:00:43.1616
@@ -258,7 +258,7 @@
 /choice
   /attribute
 /optional
-ref name=absFilePath/
+ref name=filePath/
   /element
 /optional
 optional
@@ -1060,7 +1060,7 @@
   optional
 element name=bootloader
   choice
-ref name=absFilePath/
+ref name=filePath/
 empty/
   /choice
 /element



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

Re: [libvirt] [PATCH/RFC] Add missing delta from Ubuntu to apparmor profiles

2014-10-21 Thread Stefan Bader

On 20.10.2014 12:48, Stefan Bader wrote:
 On 19.10.2014 17:07, intrigeri wrote:
 Hi Stefan,

 Stefan Bader wrote (19 Oct 2014 11:07:40 GMT) :
 Yeah, I actually did but it felt a bit hackish but then I am told anything 
 looks
 a bit hackish when it involves autoconf. These are again against upstream
 libvirt mostly because the last touch timestamps always clash otherwise.

 Cool, I've tested this. I've imported these two patches in Debian's
 1.2.9-3 quilt series, made the build system use dh-autoreconf (the
 build system in the tarball wants aclocal 1.13, while Debian sid has
 1.14), and added a build-dep on libapparmor-dev to get the needed
 pkg-config file.

 Attempting to build the resulting source package in a clean sid chroot
 fails here:

   Making all in examples/apparmor
   make[3]: Entering directory 
 '/tmp/buildd/libvirt-1.2.9/debian/build/examples/apparmor'
   make[3]: Circular ../../config.h - ../../config.h dependency dropped.
   ./profile-preprocess ../../../../examples/apparmor/libvirt-qemu.in 
 libvirt-qemu
   ./profile-preprocess ../../../../examples/apparmor/libvirt-lxc.in 
 libvirt-lxc
   ./profile-preprocess 
 ../../../../examples/apparmor/usr.lib.libvirt.virt-aa-helper.in 
 usr.lib.libvirt.virt-aa-helper
   ./profile-preprocess ../../../../examples/apparmor/usr.sbin.libvirtd.in 
 usr.sbin.libvirtd
   make[3]: *** No rule to make target 'local-usr.sbin.libvirtd', needed by 
 'all-am'.  Stop.
   make[3]: *** Waiting for unfinished jobs
   /bin/bash: ./profile-preprocess: No such file or directory
   /bin/bash: ./profile-preprocess: No such file or directory
   Makefile:2068: recipe for target 'libvirt-qemu' failed
   make[3]: *** [libvirt-qemu] Error 127
   Makefile:2068: recipe for target 'libvirt-lxc' failed
   make[3]: *** [libvirt-lxc] Error 127
   /bin/bash: ./profile-preprocess: No such file or directory
   /bin/bash: ./profile-preprocess: No such file or directory
   Makefile:2068: recipe for target 'usr.lib.libvirt.virt-aa-helper' failed
   make[3]: *** [usr.lib.libvirt.virt-aa-helper] Error 127
   Makefile:2068: recipe for target 'usr.sbin.libvirtd' failed
   make[3]: *** [usr.sbin.libvirtd] Error 127
   make[3]: Leaving directory 
 '/tmp/buildd/libvirt-1.2.9/debian/build/examples/apparmor'
   Makefile:1979: recipe for target 'all-recursive' failed
   make[2]: *** [all-recursive] Error 1
   make[2]: Leaving directory '/tmp/buildd/libvirt-1.2.9/debian/build'
   Makefile:1877: recipe for target 'all' failed
   make[1]: *** [all] Error 2
   make[1]: Leaving directory '/tmp/buildd/libvirt-1.2.9/debian/build'
   dh_auto_build: make -j5 returned exit code 2
   debian/rules:126: recipe for target 'build' failed
   make: *** [build] Error 2

 Any hint?
 
 Hm, partially this sounds like the preprocess script is not where it should be
 and the other part looks like not finding any local-usr-sbin. Could likely be
 that I need to do something better to make things work in place (as the 
 upstream
 libvirt instructions suggest) as well as with separate object tree (as it is 
 in
 Debian). I also saw something about circular dependency on config.h which
 probably slipped my attention. For most of the problems I guess adding 
 something
 like $(srcdir) (need to look what this would be actually called) to the
 pre-process scripts path as well as to the .in files..

Turns out that this first attempt was not too good at all. First it does not
help to mis-name the new local .in file. Then, using the wildcard form actually
causes many more files to be touched than intended (the circular reference
hinted that). Lastly I found it might be good to also do something about 
cleanup.
Hope this version works better in general.

-Stefan

From 3715e3a3aa29543e38afc6ec97296866b2977e11 Mon Sep 17 00:00:00 2001
From: Stefan Bader stefan.ba...@canonical.com
Date: Mon, 13 Oct 2014 11:31:59 +0200
Subject: [PATCH 1/2] examples/apparmor: Add ability to add versioned features

Adds APPARMOR_VERSION_NUMBER to config.h which by default is set to the
apparmor library version (major*1000+minor). It can be overriden by
the distro by supplyig --with-apparmor-profiles-version=version.

Signed-off-by: Stefan Bader stefan.ba...@canonical.com
---
 configure.ac   |  22 
 examples/apparmor/Makefile.am  |  18 +++
 examples/apparmor/libvirt-lxc  | 116 -
 examples/apparmor/libvirt-lxc.in   | 116 +
 examples/apparmor/libvirt-qemu | 144 -
 examples/apparmor/libvirt-qemu.in  | 144 +
 examples/apparmor/profile-preprocess   |  21 +++
 examples/apparmor/usr.lib.libvirt.virt-aa-helper   |  48 ---
 .../apparmor/usr.lib.libvirt.virt-aa-helper.in |  48 +++
 examples/apparmor/usr.sbin.libvirtd|  63 -
 examples/apparmor/usr.sbin.libvirtd.in |  63 +
 11 files changed

Re: [libvirt] [PATCH/RFC] Add missing delta from Ubuntu to apparmor profiles

2014-10-20 Thread Stefan Bader

On 19.10.2014 17:07, intrigeri wrote:
 Hi Stefan,
 
 Stefan Bader wrote (19 Oct 2014 11:07:40 GMT) :
 Yeah, I actually did but it felt a bit hackish but then I am told anything 
 looks
 a bit hackish when it involves autoconf. These are again against upstream
 libvirt mostly because the last touch timestamps always clash otherwise.
 
 Cool, I've tested this. I've imported these two patches in Debian's
 1.2.9-3 quilt series, made the build system use dh-autoreconf (the
 build system in the tarball wants aclocal 1.13, while Debian sid has
 1.14), and added a build-dep on libapparmor-dev to get the needed
 pkg-config file.
 
 Attempting to build the resulting source package in a clean sid chroot
 fails here:
 
   Making all in examples/apparmor
   make[3]: Entering directory 
 '/tmp/buildd/libvirt-1.2.9/debian/build/examples/apparmor'
   make[3]: Circular ../../config.h - ../../config.h dependency dropped.
   ./profile-preprocess ../../../../examples/apparmor/libvirt-qemu.in 
 libvirt-qemu
   ./profile-preprocess ../../../../examples/apparmor/libvirt-lxc.in 
 libvirt-lxc
   ./profile-preprocess 
 ../../../../examples/apparmor/usr.lib.libvirt.virt-aa-helper.in 
 usr.lib.libvirt.virt-aa-helper
   ./profile-preprocess ../../../../examples/apparmor/usr.sbin.libvirtd.in 
 usr.sbin.libvirtd
   make[3]: *** No rule to make target 'local-usr.sbin.libvirtd', needed by 
 'all-am'.  Stop.
   make[3]: *** Waiting for unfinished jobs
   /bin/bash: ./profile-preprocess: No such file or directory
   /bin/bash: ./profile-preprocess: No such file or directory
   Makefile:2068: recipe for target 'libvirt-qemu' failed
   make[3]: *** [libvirt-qemu] Error 127
   Makefile:2068: recipe for target 'libvirt-lxc' failed
   make[3]: *** [libvirt-lxc] Error 127
   /bin/bash: ./profile-preprocess: No such file or directory
   /bin/bash: ./profile-preprocess: No such file or directory
   Makefile:2068: recipe for target 'usr.lib.libvirt.virt-aa-helper' failed
   make[3]: *** [usr.lib.libvirt.virt-aa-helper] Error 127
   Makefile:2068: recipe for target 'usr.sbin.libvirtd' failed
   make[3]: *** [usr.sbin.libvirtd] Error 127
   make[3]: Leaving directory 
 '/tmp/buildd/libvirt-1.2.9/debian/build/examples/apparmor'
   Makefile:1979: recipe for target 'all-recursive' failed
   make[2]: *** [all-recursive] Error 1
   make[2]: Leaving directory '/tmp/buildd/libvirt-1.2.9/debian/build'
   Makefile:1877: recipe for target 'all' failed
   make[1]: *** [all] Error 2
   make[1]: Leaving directory '/tmp/buildd/libvirt-1.2.9/debian/build'
   dh_auto_build: make -j5 returned exit code 2
   debian/rules:126: recipe for target 'build' failed
   make: *** [build] Error 2
 
 Any hint?

Hm, partially this sounds like the preprocess script is not where it should be
and the other part looks like not finding any local-usr-sbin. Could likely be
that I need to do something better to make things work in place (as the upstream
libvirt instructions suggest) as well as with separate object tree (as it is in
Debian). I also saw something about circular dependency on config.h which
probably slipped my attention. For most of the problems I guess adding something
like $(srcdir) (need to look what this would be actually called) to the
pre-process scripts path as well as to the .in files..

-Stefan
 
 Cheers,
 




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

Re: [libvirt] [PATCH/RFC] Add missing delta from Ubuntu to apparmor profiles

2014-10-19 Thread Stefan Bader

On 18.10.2014 23:12, intrigeri wrote:
 Hi,
 
 Daniel P. Berrange wrote (01 Oct 2014 14:54:43 GMT) :
 Agreed, the libvirt upstream distributed file should do version checks
 based on official apparmor releases, and distros can tweak versions if
 they have backported features.
 
 So, it seems that we've reached a consensus that adding version
 checking machinery makes sense. Great :)
 
 Stefan, do you plan to implement it? One way to go could be to
 implement and upload it first in Ubuntu: the rest of the delta is
 already there anyway, so it's not as if it changed the current
 situation much; and then, it also makes it easy to test the version
 checks on Debian, for added confidence, before submitting the
 patch upstream.
 
 Note: once this machinery is in place, ideally distros should rebuild
 their libvirt binary packages when they introduce new AppArmor parser
 features -- which is effectively a transition, in Debian-speak.
 
 Cheers,
 
Yeah, I actually did but it felt a bit hackish but then I am told anything looks
a bit hackish when it involves autoconf. These are again against upstream
libvirt mostly because the last touch timestamps always clash otherwise. I tried
to do two steps, one introducing the machinery and the second to add the
changes. That way the vast looking delta of the first patch boils down to mostly
renames.

-Stefan


From 5d0c61d3e9df6a4f58ac933d1fadc9b36eff2dce Mon Sep 17 00:00:00 2001
From: Stefan Bader stefan.ba...@canonical.com
Date: Mon, 13 Oct 2014 11:31:59 +0200
Subject: [PATCH 1/2] examples/apparmor: Add ability to add versioned features

Adds APPARMOR_VERSION_NUMBER to config.h which by default is set to the
apparmor library version (major*1000+minor). It can be overriden by
the distro by supplyig --with-apparmor-profiles-version=version.

Signed-off-by: Stefan Bader stefan.ba...@canonical.com
---
 configure.ac   |  22 
 examples/apparmor/Makefile.am  |   3 +
 examples/apparmor/libvirt-lxc  | 116 -
 examples/apparmor/libvirt-lxc.in   | 116 +
 examples/apparmor/libvirt-qemu | 144 -
 examples/apparmor/libvirt-qemu.in  | 144 +
 examples/apparmor/profile-preprocess   |  21 +++
 examples/apparmor/usr.lib.libvirt.virt-aa-helper   |  48 ---
 .../apparmor/usr.lib.libvirt.virt-aa-helper.in |  48 +++
 examples/apparmor/usr.sbin.libvirtd|  63 -
 examples/apparmor/usr.sbin.libvirtd.in |  63 +
 11 files changed, 417 insertions(+), 371 deletions(-)
 delete mode 100644 examples/apparmor/libvirt-lxc
 create mode 100644 examples/apparmor/libvirt-lxc.in
 delete mode 100644 examples/apparmor/libvirt-qemu
 create mode 100644 examples/apparmor/libvirt-qemu.in
 create mode 100755 examples/apparmor/profile-preprocess
 delete mode 100644 examples/apparmor/usr.lib.libvirt.virt-aa-helper
 create mode 100644 examples/apparmor/usr.lib.libvirt.virt-aa-helper.in
 delete mode 100644 examples/apparmor/usr.sbin.libvirtd
 create mode 100644 examples/apparmor/usr.sbin.libvirtd.in

diff --git a/configure.ac b/configure.ac
index f7b02ff..42cf073 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1490,6 +1490,28 @@ if test $with_apparmor = no; then
 fi
 AM_CONDITIONAL([WITH_APPARMOR_PROFILES], [test $with_apparmor_profiles != no])
 
+AC_ARG_WITH([apparmor-profiles-version],
+  [AS_HELP_STRING([--with-apparmor-profiles-version],
+[install apparmor profiles for apparmor version @:@default=check@:@])],
+  [],
+  [with_apparmor_profiles_version=check])
+if test $with_apparmor_profiles = no; then
+  with_apparmor_profiles_version=no
+fi
+if test $with_apparmor_profiles_version = check; then
+  APPARMOR_VERSION=`pkg-config --modversion libapparmor|cut -d. -f1-2`
+elif test $with_apparmor_profiles_version != no; then
+  APPARMOR_VERSION=$withval
+fi
+if test $with_apparmor_profiles_version != no; then
+  APPARMOR_MAJOR_VERSION=`echo $APPARMOR_VERSION|cut -d. -f1`
+  APPARMOR_MINOR_VERSION=`echo $APPARMOR_VERSION|cut -d. -f2`
+  APPARMOR_VERSION_NUMBER=`expr $APPARMOR_MAJOR_VERSION \* 1000 + $APPARMOR_MINOR_VERSION`
+  AC_DEFINE_UNQUOTED([APPARMOR_VERSION_NUMBER],
+$APPARMOR_VERSION_NUMBER,
+[Version number of apparmor library (for profile features)])
+fi
+
 dnl DTrace static probes
 AC_ARG_WITH([dtrace],
   [AS_HELP_STRING([--with-dtrace],
diff --git a/examples/apparmor/Makefile.am b/examples/apparmor/Makefile.am
index 7a20e16..4712b8d 100644
--- a/examples/apparmor/Makefile.am
+++ b/examples/apparmor/Makefile.am
@@ -40,4 +40,7 @@ templates_DATA = \
 	TEMPLATE.qemu \
 	TEMPLATE.lxc \
 	$(NULL)
+
+%:	%.in profile-preprocess ../../config.h
+	./profile-preprocess $ $@
 endif WITH_APPARMOR_PROFILES
diff --git a/examples/apparmor/libvirt-lxc b/examples/apparmor/libvirt-lxc
deleted file mode 100644
index 4bfb503..000
--- a/examples/apparmor

[libvirt] [PATCH/RFC] Add missing delta from Ubuntu to apparmor profiles

2014-10-01 Thread Stefan Bader

This had been on the Debian package list before but its time to take
this onwards. So the goal would be to have one set to rule them all
(when using apparmor) and drop the seperate set of definitions which
exist at least in the Ubuntu packaging.

Right now the patch would be at a state which adds all missing files
and rules to the current examples in libvirt and installs them when
using --with-apparmor-profiles.

One problem seems to be that some of the definitions might cause
parse failures on certain versions of apparmor. I checked this morning
and this looks a bit hairy. So some apparmor 2.8 versions potentially
have issues, but not all apparmor 2.8 are the same (gah).

I could imagine (but John, we really could use some guidance here ;))
that at least some changes could be related to version 2.8.95~2430:

+ debian/patches/mediate-signals.patch,
  debian/patches/change-signal-syntax.patch: Parse signal rules with
  apparmor_parser. See the apparmor.d(5) man page for syntax details.
+ debian/patches/change-ptrace-syntax.patch,
  debian/patches/mediate-ptrace.patch: Parse ptrace rules with
  apparmor_parser. See the apparmor.d(5) man page for syntax details.

But, regardless of the when, the apparmor rules maybe need a way to handle
versioned features of the parser. One proposal was to comment out problematic
rules and allow the packager to re-enable things. Maybe going one step
further and have some pre-processing that handles version based sections
(like #if (APPARMOR_VERSION = xxx)).

So that is where we stand. Ideas are very welcome.

-Stefan

---

From aec5cf8cc30c80492a37856626264c3d4c27a31f Mon Sep 17 00:00:00 2001
From: Stefan Bader stefan.ba...@canonical.com
Date: Thu, 18 Sep 2014 14:15:17 +0200
Subject: [PATCH] Add missing delta from Ubuntu to apparmor profiles

This fixes up the upstream profiles and would allow to drop apparmor
related delta from the Ubuntu package.
Thanks to Serge Hallyn for the Makefile.am install hook that allows
to rename the local file.

Signed-off-by: Stefan Bader stefan.ba...@canonical.com
---
 examples/apparmor/Makefile.am| 10 
 examples/apparmor/libvirt-lxc| 15 +++-
 examples/apparmor/libvirt-qemu   | 31 +++-
 examples/apparmor/local-usr.sbin.libvirtd|  2 ++
 examples/apparmor/usr.lib.libvirt.virt-aa-helper | 25 ---
 examples/apparmor/usr.sbin.libvirtd  | 17 -
 6 files changed, 94 insertions(+), 6 deletions(-)
 create mode 100644 examples/apparmor/local-usr.sbin.libvirtd

diff --git a/examples/apparmor/Makefile.am b/examples/apparmor/Makefile.am
index 7a20e16..aa46cb9 100644
--- a/examples/apparmor/Makefile.am
+++ b/examples/apparmor/Makefile.am
@@ -20,6 +20,7 @@ EXTRA_DIST=   \
libvirt-qemu\
libvirt-lxc \
usr.lib.libvirt.virt-aa-helper  \
+   local-usr.sbin.libvirtd \
usr.sbin.libvirtd
 
 if WITH_APPARMOR_PROFILES
@@ -29,6 +30,15 @@ apparmor_DATA = \
usr.sbin.libvirtd \
$(NULL)
 
+localdir = $(apparmordir)/local
+local_DATA = \
+   local-usr.sbin.libvirtd \
+   $(NULL)
+
+install-data-hook:
+   mv $(DESTDIR)$(localdir)/local-usr.sbin.libvirtd \
+  $(DESTDIR)$(localdir)/usr.sbin.libvirtd
+
 abstractionsdir = $(apparmordir)/abstractions
 abstractions_DATA = \
libvirt-qemu \
diff --git a/examples/apparmor/libvirt-lxc b/examples/apparmor/libvirt-lxc
index 4bfb503..4705e0a 100644
--- a/examples/apparmor/libvirt-lxc
+++ b/examples/apparmor/libvirt-lxc
@@ -1,12 +1,18 @@
-# Last Modified: Fri Feb  7 13:01:36 2014
+# Last Modified: Thu, 18 Sep 2014 13:56:49 +0200
 
   #include abstractions/base
 
   umount,
+  dbus,
+  signal,
+  ptrace,
 
   # ignore DENIED message on / remount
   deny mount options=(ro, remount) - /,
 
+  # support use of cgmanager proxy
+  mount options=(move) /sys/fs/cgroup/cgmanager/ - 
/sys/fs/cgroup/cgmanager.lower/,
+
   # allow tmpfs mounts everywhere
   mount fstype=tmpfs,
 
@@ -33,8 +39,15 @@
   mount fstype=fusectl - /sys/fs/fuse/connections/,
   mount fstype=securityfs - /sys/kernel/security/,
   mount fstype=debugfs - /sys/kernel/debug/,
+  deny mount fstype=debugfs - /var/lib/ureadahead/debugfs/,
   mount fstype=proc - /proc/,
   mount fstype=sysfs - /sys/,
+
+  mount options=(rw nosuid nodev noexec remount) - /sys/,
+  mount options=(rw remount) - /sys/kernel/security/,
+  mount options=(rw remount) - /sys/fs/pstore/,
+  mount options=(ro remount) - /sys/fs/pstore/,
+
   deny /sys/firmware/efi/efivars/** rwklx,
   deny /sys/kernel/security/** rwklx,
 
diff --git a/examples/apparmor/libvirt-qemu b/examples/apparmor/libvirt-qemu
index c6de6dd..b69e64c 100644
--- a/examples/apparmor/libvirt-qemu
+++ b/examples/apparmor/libvirt-qemu
@@ -1,4 +1,4 @@
-# Last Modified: Wed Sep 3 21:52:03 2014
+# Last Modified: Thu, 18 Sep 2014 16:41:21

Re: [libvirt] [PATCH/RFC] Add missing delta from Ubuntu to apparmor profiles

2014-10-01 Thread Stefan Bader

On 01.10.2014 11:04, Daniel P. Berrange wrote:
 On Wed, Oct 01, 2014 at 10:30:58AM +0200, Stefan Bader wrote:
 This had been on the Debian package list before but its time to take
 this onwards. So the goal would be to have one set to rule them all
 (when using apparmor) and drop the seperate set of definitions which
 exist at least in the Ubuntu packaging.

 Right now the patch would be at a state which adds all missing files
 and rules to the current examples in libvirt and installs them when
 using --with-apparmor-profiles.

 One problem seems to be that some of the definitions might cause
 parse failures on certain versions of apparmor. I checked this morning
 and this looks a bit hairy. So some apparmor 2.8 versions potentially
 have issues, but not all apparmor 2.8 are the same (gah).
 
 What versions of apparmour are present in the currently supported
 versions of Debian  Ubuntu ?

The way release are handled in Ubuntu (once released there is usually no
backporting) we would have to worry less about supported releases. For the
Debian side I would think this is similar (correct me if I am wrong, please). So
it looks to me that right now this would be down to Debian having 2.8.0 in
unstable/testing and Ubuntu having 2.8.96~2652 in Utopic (with the same version
in Debian experimental).

Right now I would expect it to boil down to those two. But I suppose the parser
can change again and so there might be a similar situation in the future.

-Stefan

 
 I could imagine (but John, we really could use some guidance here ;))
 that at least some changes could be related to version 2.8.95~2430:

 + debian/patches/mediate-signals.patch,
   debian/patches/change-signal-syntax.patch: Parse signal rules with
   apparmor_parser. See the apparmor.d(5) man page for syntax details.
 + debian/patches/change-ptrace-syntax.patch,
   debian/patches/mediate-ptrace.patch: Parse ptrace rules with
   apparmor_parser. See the apparmor.d(5) man page for syntax details.

 But, regardless of the when, the apparmor rules maybe need a way to handle
 versioned features of the parser. One proposal was to comment out problematic
 rules and allow the packager to re-enable things. Maybe going one step
 further and have some pre-processing that handles version based sections
 (like #if (APPARMOR_VERSION = xxx)).
 
 I think it would be pretty reasonable to rename the files in have '.in'
 suffixes, and then have a build script that expands 'if APPARMOR_VERSION'
 conditionals to generate the final file.
 
 Regards,
 Daniel
 




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

Re: [libvirt] [PATCH] libxl: Implement basic video device selection

2014-09-19 Thread Stefan Bader

On 19.09.2014 05:01, Jim Fehlig wrote:
 Stefan Bader wrote:
 Re-pushing this as the old thread got rather stale.
 
 Thanks.
 
  Some of the
 VFB setup went in a bug fix. Not sure I missed a detail in rebasing
 bug the keyboard setting may be the only thing missing...
   
 
 Yes, agreed.
 
 -Stefan

 [v2: Check return code of VIR_STRDUP and fix indentation]
 [v3: Split out VRAM fixup and return error for unsupported video type]
 [v4: Re-arrange code and move VFB setup into libxlMakeVfbList]
 [v5: Rebased against head which already had some VFB setup code]

 From b3ff8f4c658d29f15e673af88b9ae2fdfa3c1317 Mon Sep 17 00:00:00 2001
 From: Stefan Bader stefan.ba...@canonical.com
 Date: Thu, 27 Mar 2014 16:01:18 +0100
 Subject: [PATCH] libxl: Implement basic video device selection

 This started as an investigation into an issue where libvirt (using the
 libxl driver) and the Xen host, like an old couple, could not agree on
 who is responsible for selecting the VNC port to use.

 Things usually (and a bit surprisingly) did work because, just like that
 old couple, they had the same idea on what to do by default. However it
 was possible that this ended up in a big argument.

 The problem is that display information exists in two different places:
 in the vfbs list and in the build info. And for launching the device model,
 only the latter is used. But that never gets initialized from libvirt. So
 Xen allows the device model to select a default port while libvirt thinks
 it has told Xen that this is done by libvirt (though the vfbs config).

 While fixing that, I made a stab at actually evaluating the configuration
 of the video device. So that it is now possible to at least decide between
 a Cirrus or standard VGA emulation and to modify the VRAM within certain
 limits using libvirt.

 Signed-off-by: Stefan Bader stefan.ba...@canonical.com
 ---
  src/libxl/libxl_conf.c | 50 
 ++
  1 file changed, 50 insertions(+)
   
 
 This patch suffers the same issues as the last version.  And when
 commenting on that version, I promised to work on a followup to address
 my concerns
 
 https://www.redhat.com/archives/libvir-list/2014-July/msg00931.html

Oh my, I have to admit I completely forgot about that.

 
 Your repost poked me into reworking my first attempt, the result of
 which is below.  I should probably look at a sensible split-up of these
 patches that would be easier to review, but in the meantime comments on
 my followup would be appreciated.
 
 With both patches, my tests are passing and my concerns are subdued :-).

There seem to be some parts of code suggesting support for anything beside Xen
(assumed to be alias to VGA) and Cirrus. As much as I know Xen handles only the
two (not qxl or anything else).

I believe the xen specific qemu variant was the only one called qemu-dm (and the
upstream variant always being qemu-system-i386). But checking the help message
probably is safer and not called that often so I should not worry about
performance).

I tried the variant you proposed and it looks to be ok. Still have to carry a
piece which is not going upstream if I wan't to paper over the virt-manager
issue. But at least now the failure is clearly showing what is wrong. So I am
happy enough. :)

-Stefan
 
 Regards,
 Jim
  
 




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

[libvirt] [PATCH] libxl: Implement basic video device selection

2014-09-18 Thread Stefan Bader

Re-pushing this as the old thread got rather stale. Some of the
VFB setup went in a bug fix. Not sure I missed a detail in rebasing
bug the keyboard setting may be the only thing missing...

-Stefan

[v2: Check return code of VIR_STRDUP and fix indentation]
[v3: Split out VRAM fixup and return error for unsupported video type]
[v4: Re-arrange code and move VFB setup into libxlMakeVfbList]
[v5: Rebased against head which already had some VFB setup code]

From b3ff8f4c658d29f15e673af88b9ae2fdfa3c1317 Mon Sep 17 00:00:00 2001
From: Stefan Bader stefan.ba...@canonical.com
Date: Thu, 27 Mar 2014 16:01:18 +0100
Subject: [PATCH] libxl: Implement basic video device selection

This started as an investigation into an issue where libvirt (using the
libxl driver) and the Xen host, like an old couple, could not agree on
who is responsible for selecting the VNC port to use.

Things usually (and a bit surprisingly) did work because, just like that
old couple, they had the same idea on what to do by default. However it
was possible that this ended up in a big argument.

The problem is that display information exists in two different places:
in the vfbs list and in the build info. And for launching the device model,
only the latter is used. But that never gets initialized from libvirt. So
Xen allows the device model to select a default port while libvirt thinks
it has told Xen that this is done by libvirt (though the vfbs config).

While fixing that, I made a stab at actually evaluating the configuration
of the video device. So that it is now possible to at least decide between
a Cirrus or standard VGA emulation and to modify the VRAM within certain
limits using libvirt.

Signed-off-by: Stefan Bader stefan.ba...@canonical.com
---
 src/libxl/libxl_conf.c | 50 ++
 1 file changed, 50 insertions(+)

diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index acba69c..2727230 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -1200,6 +1200,8 @@ libxlMakeVfbList(virPortAllocatorPtr graphicsports,
 if (VIR_STRDUP(b_info-u.hvm.sdl.xauthority, vfb.sdl.xauthority)  
0)
 goto error;
 }
+if (VIR_STRDUP(b_info-u.hvm.keymap, vfb.keymap)  0)
+goto error;
 }
 
 return 0;
@@ -1462,6 +1464,45 @@ libxlMakeCapabilities(libxl_ctx *ctx)
 return NULL;
 }
 
+static int
+libxlMakeVideo(virDomainDefPtr def, libxl_domain_config *d_config)
+{
+libxl_domain_build_info *b_info = d_config-b_info;
+
+if (d_config-c_info.type != LIBXL_DOMAIN_TYPE_HVM)
+return 0;
+
+/*
+ * Take the first defined video device (graphics card) to display
+ * on the first graphics device (display).
+ * Right now only type and vram info is used and anything beside
+ * type xen and vga is mapped to cirrus.
+ */
+if (def-nvideos) {
+switch (def-videos[0]-type) {
+case VIR_DOMAIN_VIDEO_TYPE_VGA:
+case VIR_DOMAIN_VIDEO_TYPE_XEN:
+b_info-u.hvm.vga.kind = LIBXL_VGA_INTERFACE_TYPE_STD;
+break;
+case VIR_DOMAIN_VIDEO_TYPE_CIRRUS:
+b_info-u.hvm.vga.kind = LIBXL_VGA_INTERFACE_TYPE_CIRRUS;
+break;
+default:
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   %s,
+   _(video type not supported by libxl));
+return -1;
+}
+b_info-video_memkb = def-videos[0]-vram ?
+  def-videos[0]-vram :
+  LIBXL_MEMKB_DEFAULT;
+} else {
+libxl_defbool_set(b_info-u.hvm.nographic, 1);
+}
+
+return 0;
+}
+
 int
 libxlBuildDomainConfig(virPortAllocatorPtr graphicsports,
virDomainDefPtr def,
@@ -1488,6 +1529,15 @@ libxlBuildDomainConfig(virPortAllocatorPtr graphicsports,
 if (libxlMakePCIList(def, d_config)  0)
 return -1;
 
+/*
+ * Now that any potential VFBs are defined, it is time to update the
+ * build info with the data of the primary display. Some day libxl
+ * might implicitely do so but as it does not right now, better be
+ * explicit.
+ */
+if (libxlMakeVideo(def, d_config)  0)
+return -1;
+
 d_config-on_reboot = libxlActionFromVirLifecycle(def-onReboot);
 d_config-on_poweroff = libxlActionFromVirLifecycle(def-onPoweroff);
 d_config-on_crash = libxlActionFromVirLifecycleCrash(def-onCrash);
-- 
1.9.1

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

Re: [libvirt] [PATCH] libxl: Implement basic video device selection

2014-07-17 Thread Stefan Bader

On 16.07.2014 23:05, Jim Fehlig wrote:
 Stefan Bader wrote:
 being as bad with timely responses. Ok, so how about the following?

 One note: it could be the STRDUP's are not strictly needed. But
 to me it felt wrong to have two places refer to the same strings
 (as MakeVFB copies the struct containing the pointers).
 
 Agreed.  Without the STRDUP's, seems there is a potential for double
 free when libxl_device_vfb and libxl_domain_config objects are disposed.
 
  If this
 is not needed, then all changes now in MakeVFB probably can be
 dropped (except setting the keyboard layout, maybe; which I
 might miss ;)).

 -Stefan


 From a95db265fa4c1a231e7c2d70baa360c6a0500e3b Mon Sep 17 00:00:00 2001
 From: Stefan Bader stefan.ba...@canonical.com
 Date: Thu, 27 Mar 2014 16:01:18 +0100
 Subject: [PATCH] libxl: Implement basic video device selection

 This started as an investigation into an issue where libvirt (using the
 libxl driver) and the Xen host, like an old couple, could not agree on
 who is responsible for selecting the VNC port to use.

 Things usually (and a bit surprisingly) did work because, just like that
 old couple, they had the same idea on what to do by default. However it
 was possible that this ended up in a big argument.

 The problem is that display information exists in two different places:
 in the vfbs list and in the build info. And for launching the device model,
 only the latter is used. But that never gets initialized from libvirt. So
 Xen allows the device model to select a default port while libvirt thinks
 it has told Xen that this is done by libvirt (though the vfbs config).

 While fixing that, I made a stab at actually evaluating the configuration
 of the video device. So that it is now possible to at least decide between
 a Cirrus or standard VGA emulation and to modify the VRAM within certain
 limits using libvirt.

 [v2: Check return code of VIR_STRDUP and fix indentation]
 [v3: Split out VRAM fixup and return error for unsupported video type]
 [v4: Re-arrange code and move VFB setup into libxlMakeVfbList]
   
 
 [meta-comment]
 libvirt prefers patch version history like this to be below  the '---'
 following your Signed-off-by, so as to not pollute the commit message.

Ah yeah, makes sense. I try to keep it in mind.

 
 Signed-off-by: Stefan Bader stefan.ba...@canonical.com
 ---
  src/libxl/libxl_conf.c |   63 
 ++--
  1 file changed, 61 insertions(+), 2 deletions(-)

 diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
 index 8eeaf82..43cabcf 100644
 --- a/src/libxl/libxl_conf.c
 +++ b/src/libxl/libxl_conf.c
 @@ -1098,10 +1098,21 @@ libxlMakeVfbList(virPortAllocatorPtr graphicsports,
  libxl_domain_build_info *b_info = d_config-b_info;
  libxl_device_vfb vfb = d_config-vfbs[0];
  
 -if (libxl_defbool_val(vfb.vnc.enable))
 +if (libxl_defbool_val(vfb.vnc.enable)) {
  memcpy(b_info-u.hvm.vnc, vfb.vnc, sizeof(libxl_vnc_info));
 -else if (libxl_defbool_val(vfb.sdl.enable))
 +if (VIR_STRDUP(b_info-u.hvm.vnc.listen, vfb.vnc.listen)  0)
 +goto error;
 +if (VIR_STRDUP(b_info-u.hvm.vnc.passwd, vfb.vnc.passwd)  0)
 +goto error;
 +} else if (libxl_defbool_val(vfb.sdl.enable)) {
  memcpy(b_info-u.hvm.sdl, vfb.sdl, sizeof(libxl_sdl_info));
 +if (VIR_STRDUP(b_info-u.hvm.sdl.display, vfb.sdl.display)  0)
 +goto error;
 +if (VIR_STRDUP(b_info-u.hvm.sdl.xauthority, 
 vfb.sdl.xauthority)  0)
 +goto error;
 +}
 +if (VIR_STRDUP(b_info-u.hvm.keymap, vfb.keymap)  0)
 +goto error;
  }
  
  return 0;
 @@ -1363,6 +1374,45 @@ libxlMakeCapabilities(libxl_ctx *ctx)
  return NULL;
  }
  
 +static int
 +libxlMakeVideo(virDomainDefPtr def, libxl_domain_config *d_config)
 +{
 +libxl_domain_build_info *b_info = d_config-b_info;
 +
 +if (d_config-c_info.type != LIBXL_DOMAIN_TYPE_HVM)
 +return 0;
 +
 +/*
 + * Take the first defined video device (graphics card) to display
 + * on the first graphics device (display).
 + * Right now only type and vram info is used and anything beside
 + * type xen and vga is mapped to cirrus.
 + */
 +if (def-nvideos) {
 +switch (def-videos[0]-type) {
 +case VIR_DOMAIN_VIDEO_TYPE_VGA:
 +case VIR_DOMAIN_VIDEO_TYPE_XEN:
 +b_info-u.hvm.vga.kind = LIBXL_VGA_INTERFACE_TYPE_STD;
 +break;
 +case VIR_DOMAIN_VIDEO_TYPE_CIRRUS:
 +b_info-u.hvm.vga.kind = LIBXL_VGA_INTERFACE_TYPE_CIRRUS;
 +break;
 +default:
 +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
 +   %s,
 +   _(video type not supported by libxl));
 +return -1;
 +}
 +b_info-video_memkb = def-videos[0

[libvirt] [PATCH] libxl: Implement basic video device selection

2014-07-01 Thread Stefan Bader

being as bad with timely responses. Ok, so how about the following?

One note: it could be the STRDUP's are not strictly needed. But
to me it felt wrong to have two places refer to the same strings
(as MakeVFB copies the struct containing the pointers). If this
is not needed, then all changes now in MakeVFB probably can be
dropped (except setting the keyboard layout, maybe; which I
might miss ;)).

-Stefan


From a95db265fa4c1a231e7c2d70baa360c6a0500e3b Mon Sep 17 00:00:00 2001
From: Stefan Bader stefan.ba...@canonical.com
Date: Thu, 27 Mar 2014 16:01:18 +0100
Subject: [PATCH] libxl: Implement basic video device selection

This started as an investigation into an issue where libvirt (using the
libxl driver) and the Xen host, like an old couple, could not agree on
who is responsible for selecting the VNC port to use.

Things usually (and a bit surprisingly) did work because, just like that
old couple, they had the same idea on what to do by default. However it
was possible that this ended up in a big argument.

The problem is that display information exists in two different places:
in the vfbs list and in the build info. And for launching the device model,
only the latter is used. But that never gets initialized from libvirt. So
Xen allows the device model to select a default port while libvirt thinks
it has told Xen that this is done by libvirt (though the vfbs config).

While fixing that, I made a stab at actually evaluating the configuration
of the video device. So that it is now possible to at least decide between
a Cirrus or standard VGA emulation and to modify the VRAM within certain
limits using libvirt.

[v2: Check return code of VIR_STRDUP and fix indentation]
[v3: Split out VRAM fixup and return error for unsupported video type]
[v4: Re-arrange code and move VFB setup into libxlMakeVfbList]

Signed-off-by: Stefan Bader stefan.ba...@canonical.com
---
 src/libxl/libxl_conf.c |   63 ++--
 1 file changed, 61 insertions(+), 2 deletions(-)

diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index 8eeaf82..43cabcf 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -1098,10 +1098,21 @@ libxlMakeVfbList(virPortAllocatorPtr graphicsports,
 libxl_domain_build_info *b_info = d_config-b_info;
 libxl_device_vfb vfb = d_config-vfbs[0];
 
-if (libxl_defbool_val(vfb.vnc.enable))
+if (libxl_defbool_val(vfb.vnc.enable)) {
 memcpy(b_info-u.hvm.vnc, vfb.vnc, sizeof(libxl_vnc_info));
-else if (libxl_defbool_val(vfb.sdl.enable))
+if (VIR_STRDUP(b_info-u.hvm.vnc.listen, vfb.vnc.listen)  0)
+goto error;
+if (VIR_STRDUP(b_info-u.hvm.vnc.passwd, vfb.vnc.passwd)  0)
+goto error;
+} else if (libxl_defbool_val(vfb.sdl.enable)) {
 memcpy(b_info-u.hvm.sdl, vfb.sdl, sizeof(libxl_sdl_info));
+if (VIR_STRDUP(b_info-u.hvm.sdl.display, vfb.sdl.display)  0)
+goto error;
+if (VIR_STRDUP(b_info-u.hvm.sdl.xauthority, vfb.sdl.xauthority)  
0)
+goto error;
+}
+if (VIR_STRDUP(b_info-u.hvm.keymap, vfb.keymap)  0)
+goto error;
 }
 
 return 0;
@@ -1363,6 +1374,45 @@ libxlMakeCapabilities(libxl_ctx *ctx)
 return NULL;
 }
 
+static int
+libxlMakeVideo(virDomainDefPtr def, libxl_domain_config *d_config)
+{
+libxl_domain_build_info *b_info = d_config-b_info;
+
+if (d_config-c_info.type != LIBXL_DOMAIN_TYPE_HVM)
+return 0;
+
+/*
+ * Take the first defined video device (graphics card) to display
+ * on the first graphics device (display).
+ * Right now only type and vram info is used and anything beside
+ * type xen and vga is mapped to cirrus.
+ */
+if (def-nvideos) {
+switch (def-videos[0]-type) {
+case VIR_DOMAIN_VIDEO_TYPE_VGA:
+case VIR_DOMAIN_VIDEO_TYPE_XEN:
+b_info-u.hvm.vga.kind = LIBXL_VGA_INTERFACE_TYPE_STD;
+break;
+case VIR_DOMAIN_VIDEO_TYPE_CIRRUS:
+b_info-u.hvm.vga.kind = LIBXL_VGA_INTERFACE_TYPE_CIRRUS;
+break;
+default:
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   %s,
+   _(video type not supported by libxl));
+return -1;
+}
+b_info-video_memkb = def-videos[0]-vram ?
+  def-videos[0]-vram :
+  LIBXL_MEMKB_DEFAULT;
+} else {
+libxl_defbool_set(b_info-u.hvm.nographic, 1);
+}
+
+return 0;
+}
+
 int
 libxlBuildDomainConfig(virPortAllocatorPtr graphicsports,
virDomainDefPtr def,
@@ -1389,6 +1439,15 @@ libxlBuildDomainConfig(virPortAllocatorPtr graphicsports,
 if (libxlMakePCIList(def, d_config)  0)
 return -1;
 
+/*
+ * Now that any potential VFBs are defined

[libvirt] [PATCH 2/2] libxl: Fix up VRAM to minimum requirements

2014-05-19 Thread Stefan Bader

This is a bit debatable. On one side it hides configuration errors
in a way that makes them hard to spot. On the other side there is
at least one issue with (maybe some older versions) virt-manager.
Virt-manager sets VRAM directly, not using the default memory setting
but uses too small values for libxl. Worse, those versions do not seem
to allow to change VRAM from the GUI. So switching the video type to
VGA makes the guest fail to start until one manually adapts the VRAM
size in the XML definition.
With this change this would not happen but VRAM will be bigger than
the GUI says. This would not be that different from current Cirrus
behaviour. Only that in that case qemu seems to ignore the provided
size.

Signed-off-by: Stefan Bader stefan.ba...@canonical.com
---
 src/libxl/libxl_conf.c |   27 ++-
 1 file changed, 26 insertions(+), 1 deletion(-)

diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index 2b5c469..9af8abe 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -1316,13 +1316,38 @@ libxlSetBuildGraphics(virDomainDefPtr def, 
libxl_domain_config *d_config)
  * type xen and vga is mapped to cirrus.
  */
 if (def-nvideos) {
+unsigned int min_vram = 8 * 1024;
+
 switch (def-videos[0]-type) {
 case VIR_DOMAIN_VIDEO_TYPE_VGA:
 case VIR_DOMAIN_VIDEO_TYPE_XEN:
 b_info-u.hvm.vga.kind = LIBXL_VGA_INTERFACE_TYPE_STD;
+/*
+ * Libxl enforces a minimal VRAM size of 8M when using
+ * LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL or
+ * 16M for LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN.
+ * Avoid build failures and go with the minimum if less
+ * is specified.
+ */
+switch (b_info-device_model_version) {
+case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL:
+min_vram = 8 * 1024;
+break;
+case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
+default:
+min_vram = 16 * 1024;
+}
 break;
 case VIR_DOMAIN_VIDEO_TYPE_CIRRUS:
 b_info-u.hvm.vga.kind = LIBXL_VGA_INTERFACE_TYPE_CIRRUS;
+switch (b_info-device_model_version) {
+case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL:
+min_vram = 4 * 1024; /* Actually the max, too */
+break;
+case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
+default:
+min_vram = 8 * 1024;
+}
 break;
 default:
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
@@ -1330,7 +1355,7 @@ libxlSetBuildGraphics(virDomainDefPtr def, 
libxl_domain_config *d_config)
_(video type not supported by libxl));
 return -1;
 }
-b_info-video_memkb = def-videos[0]-vram ?
+b_info-video_memkb = (def-videos[0]-vram = min_vram) ?
   def-videos[0]-vram :
   LIBXL_MEMKB_DEFAULT;
 } else {
-- 
1.7.9.5

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

[libvirt] [PATCH 1/2] libxl: Implement basic video device selection

2014-05-19 Thread Stefan Bader

This started as an investigation into an issue where libvirt (using the
libxl driver) and the Xen host, like an old couple, could not agree on
who is responsible for selecting the VNC port to use.

Things usually (and a bit surprisingly) did work because, just like that
old couple, they had the same idea on what to do by default. However it
was possible that this ended up in a big argument.

The problem is that display information exists in two different places:
in the vfbs list and in the build info. And for launching the device model,
only the latter is used. But that never gets initialized from libvirt. So
Xen allows the device model to select a default port while libvirt thinks
it has told Xen that this is done by libvirt (though the vfbs config).

While fixing that, I made a stab at actually evaluating the configuration
of the video device. So that it is now possible to at least decide between
a Cirrus or standard VGA emulation and to modify the VRAM within certain
limits using libvirt.

[v2: Check return code of VIR_STRDUP and fix indentation]
[v3: Split out VRAM fixup and return error for unsupported video type]

Signed-off-by: Stefan Bader stefan.ba...@canonical.com
---
 src/libxl/libxl_conf.c |   68 
 1 file changed, 68 insertions(+)

diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index b7fed7f..2b5c469 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -1304,6 +1304,64 @@ libxlMakeCapabilities(libxl_ctx *ctx)
 return NULL;
 }
 
+static int
+libxlSetBuildGraphics(virDomainDefPtr def, libxl_domain_config *d_config)
+{
+libxl_domain_build_info *b_info = d_config-b_info;
+
+/*
+ * Take the first defined video device (graphics card) to display
+ * on the first graphics device (display).
+ * Right now only type and vram info is used and anything beside
+ * type xen and vga is mapped to cirrus.
+ */
+if (def-nvideos) {
+switch (def-videos[0]-type) {
+case VIR_DOMAIN_VIDEO_TYPE_VGA:
+case VIR_DOMAIN_VIDEO_TYPE_XEN:
+b_info-u.hvm.vga.kind = LIBXL_VGA_INTERFACE_TYPE_STD;
+break;
+case VIR_DOMAIN_VIDEO_TYPE_CIRRUS:
+b_info-u.hvm.vga.kind = LIBXL_VGA_INTERFACE_TYPE_CIRRUS;
+break;
+default:
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   %s,
+   _(video type not supported by libxl));
+return -1;
+}
+b_info-video_memkb = def-videos[0]-vram ?
+  def-videos[0]-vram :
+  LIBXL_MEMKB_DEFAULT;
+} else {
+libxl_defbool_set(b_info-u.hvm.nographic, 1);
+}
+
+/*
+ * When making the list of displays, only VNC and SDL types were
+ * taken into account. So it seems sensible to connect the default
+ * video device to the first in the vfb list.
+ */
+if (d_config-num_vfbs) {
+libxl_device_vfb *vfb0 = d_config-vfbs[0];
+
+b_info-u.hvm.vnc = vfb0-vnc;
+if (VIR_STRDUP(b_info-u.hvm.vnc.listen, vfb0-vnc.listen)  0)
+return -1;
+if (VIR_STRDUP(b_info-u.hvm.vnc.passwd, vfb0-vnc.passwd)  0)
+return -1;
+b_info-u.hvm.sdl = vfb0-sdl;
+if (VIR_STRDUP(b_info-u.hvm.sdl.display, vfb0-sdl.display)  0)
+return -1;
+if (VIR_STRDUP(b_info-u.hvm.sdl.xauthority, vfb0-sdl.xauthority)  0)
+return -1;
+if (VIR_STRDUP(b_info-u.hvm.keymap, vfb0-keymap)  0)
+return -1;
+}
+
+return 0;
+}
+
 int
 libxlBuildDomainConfig(libxlDriverPrivatePtr driver,
virDomainObjPtr vm, libxl_domain_config *d_config)
@@ -1331,6 +1389,16 @@ libxlBuildDomainConfig(libxlDriverPrivatePtr driver,
 if (libxlMakePCIList(def, d_config)  0)
 return -1;
 
+/*
+ * Now that any potential VFBs are defined, it is time to update the
+ * build info with the data of the primary display. Some day libxl
+ * might implicitely do so but as it does not right now, better be
+ * explicit.
+ */
+if (d_config-c_info.type == LIBXL_DOMAIN_TYPE_HVM)
+if (libxlSetBuildGraphics(def, d_config)  0)
+return -1;
+
 d_config-on_reboot = def-onReboot;
 d_config-on_poweroff = def-onPoweroff;
 d_config-on_crash = def-onCrash;
-- 
1.7.9.5

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

[libvirt] libxl: Enable video device selection for Xen

2014-05-19 Thread Stefan Bader

Sorry, this fell complete off my todos for a while. So I split off
the fixup of VRAM into a separate patch which may or may not be used
and only accept vga, xen and cirrus as supported types in the main
patch.

I believe I saw some discussions about how to fix some of the VRAM
values as they are passed into qemu. At least for the Cirrus type
I saw that the command line looked ok but the guest was getting a
much larger VRAM size than it was told.

-Stefan

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

Re: [libvirt] libvirt-libxl driver defaulting to tap disk and not working (ubuntu 12.10 + xen 4.5 + libvirt 1.2.3 + openstack)

2014-04-22 Thread Stefan Bader

On 21.04.2014 23:53, Jim Fehlig wrote:
 Tian, Shuangtai wrote:
 Hi, Jim
 The blktap seems not a module in xen 4.5, when I tried the load it , can not 
 find the module, is there something wrong I did?
   
 
 It would be provided by your dom0 kernel, not Xen.  The Ubuntu Xen
 kernel doesn't provide a blktap module?

There is a blktap source package which can be installed separately. Though I
must admit I never tried to use it. There is some code for blktap(2) in the Xen
source but I had to disable that because it would otherwise clash with the
external package.
Tian, you may try to sudo apt-get install blktap-utils which should pull in
the other pieces (libraryies/dkms module).

-Stefan

 
 Regards,
 Jim
 
 BTW ,I compiled the xen4.5 by myself.
 Thanks ! 

 -Original Message-
 From: Jim Fehlig [mailto:jfeh...@suse.com] 
 Sent: Friday, April 18, 2014 11:52 PM
 To: Tian, Shuangtai
 Cc: libvir-list@redhat.com
 Subject: Re: [libvirt] libvirt-libxl driver defaulting to tap disk and not 
 working (ubuntu 12.10 + xen 4.5 + libvirt 1.2.3 + openstack)

 Tian, Shuangtai wrote:
   
 HI

 I am an openstacker, when I used the latest libvirt and xen code to 
 run the openstack. Can not create the vm.

 there is an error in libxl log, you can see the log:

 Os : Ubuntu 12.10

  

 Compiled against library: libvirt 1.2.3

 Using library: libvirt 1.2.3

 Using API: Xen 1.2.3

 Running hypervisor: Xen 4.5.0

  

 libxl: debug: libxl_create.c:1356:do_domain_create: ao 0x7f7894002810:
 create: how=(nil) callback=(nil) poller=0x7f7894001bb0

 libxl: debug: libxl_device.c:251:libxl__device_disk_set_backend: Disk 
 vdev=xvda spec.backend=tap

 libxl: debug: libxl_device.c:210:disk_try_backend: Disk vdev=xvda, 
 backend tap unsuitable because blktap not available

 

 In addition to John's suggestions, try loading a blktap module in dom0. 
 It should work if a blktap driver is available.

 Regards,
 Jim

   
 libxl: error: libxl_device.c:289:libxl__device_disk_set_backend: no 
 suitable backend for disk xvda

 libxl: debug: libxl_event.c:1739:libxl__ao_complete: ao
 0x7f7894002810: complete, rc=-3

 libxl: debug: libxl_create.c:1370:do_domain_create: ao 0x7f7894002810:
 inprogress: poller=0x7f7894001bb0, flags=ic

 libxl: debug: libxl_event.c:1711:libxl__ao__destroy: ao
 0x7f7894002810: destroy

  

 The blktap does work, and I also find the same error someone has 
 posted,
 (http://www.redhat.com/archives/libvir-list/2013-February/msg01124.htm
 l)

 When I change the type to “phy”, it also doesnot work. And try to 
 change the type to other options, also does not work.

 Can someone give me some suggestions?

 Thanks !

  

 The XML from the openstack is :

 domain type=xen

   uuid9e9ed86c-8892-40da-acd1-31ec6303abfe/uuid

   nameinstance-0001/name

   memory524288/memory

   vcpu1/vcpu

   os

 typexen/type


 kernel/opt/stack/data/nova/instances/9e9ed86c-8892-40da-acd1-31ec630
 3abfe/kernel/kernel


 initrd/opt/stack/data/nova/instances/9e9ed86c-8892-40da-acd1-31ec630
 3abfe/ramdisk/initrd

 cmdlinero root=/dev/xvda/cmdline

   /os

   features

acpi/

 apic/

   /features

   clock offset=utc/

   devices

 disk type=file device=disk

   driver name=tap2 type=raw cache=none/

   source
 file=/opt/stack/data/nova/instances/9e9ed86c-8892-40da-acd1-31ec6303a
 bfe/disk/

   target bus=xen dev=xvda/

 /disk

 disk type=file device=cdrom

   driver name=tap2 type=raw cache=none/

   source
 file=/opt/stack/data/nova/instances/9e9ed86c-8892-40da-acd1-31ec6303a
 bfe/disk.config/

   target bus=ide dev=xvdd/

 /disk

 interface type=bridge

   mac address=fa:16:3e:d8:c3:c0/

   source bridge=br100/

   filterref 
 filter=nova-instance-instance-0001-fa163ed8c3c0/

 /interface

 console type=pty/

 graphics type=vnc autoport=yes keymap=en-us
 listen=127.0.0.1/

 video

   model type=xen/

 /video

   /devices

 /domain

  

  

  

 Best regards,

 Tian, Shuangtai

  

 --
 --

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




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

[libvirt] [PATCH v2] libxl: Implement basic video device selection

2014-04-04 Thread Stefan Bader

Ok, I think indentation should be ok like that and I added error
handling for VIR_STRDUP. I think just returning -1 like the other
places should be ok as the only caller looks to handle the disposal
of the config structure.

-Stefan

---

From dfe579003e91137ecd824d2a08bcdc8f18725857 Mon Sep 17 00:00:00 2001
From: Stefan Bader stefan.ba...@canonical.com
Date: Thu, 27 Mar 2014 16:01:18 +0100
Subject: [PATCH] libxl: Implement basic video device selection

This started as an investigation into an issue where libvirt (using the
libxl driver) and the Xen host, like an old couple, could not agree on
who is responsible for selecting the VNC port to use.

Things usually (and a bit surprisingly) did work because, just like that
old couple, they had the same idea on what to do by default. However it
was possible that this ended up in a big argument.

The problem is that display information exists in two different places:
in the vfbs list and in the build info. And for launching the device model,
only the latter is used. But that never gets initialized from libvirt. So
Xen allows the device model to select a default port while libvirt thinks
it has told Xen that this is done by libvirt (though the vfbs config).

While fixing that, I made a stab at actually evaluating the configuration
of the video device. So that it is now possible to at least decide between
a Cirrus or standard VGA emulation and to modify the VRAM within certain
limits using libvirt.

Signed-off-by: Stefan Bader stefan.ba...@canonical.com
---
 src/libxl/libxl_conf.c |   93 
 1 file changed, 93 insertions(+)

diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index b8de72a..042f4da 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -1298,6 +1298,89 @@ libxlMakeCapabilities(libxl_ctx *ctx)
 return NULL;
 }
 
+static int
+libxlSetBuildGraphics(virDomainDefPtr def, libxl_domain_config *d_config)
+{
+libxl_domain_build_info *b_info = d_config-b_info;
+
+/*
+ * Take the first defined video device (graphics card) to display
+ * on the first graphics device (display).
+ * Right now only type and vram info is used and anything beside
+ * type xen and vga is mapped to cirrus.
+ */
+if (def-nvideos) {
+unsigned int min_vram = 8 * 1024;
+
+switch (def-videos[0]-type) {
+case VIR_DOMAIN_VIDEO_TYPE_VGA:
+case VIR_DOMAIN_VIDEO_TYPE_XEN:
+b_info-u.hvm.vga.kind = LIBXL_VGA_INTERFACE_TYPE_STD;
+/*
+ * Libxl enforces a minimal VRAM size of 8M when using
+ * LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL or
+ * 16M for LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN.
+ * Avoid build failures and go with the minimum if less
+ * is specified.
+ */
+switch (b_info-device_model_version) {
+case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL:
+min_vram = 8 * 1024;
+break;
+case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
+default:
+min_vram = 16 * 1024;
+}
+break;
+default:
+/*
+ * Ignore any other device type and use Cirrus. Again fix
+ * up the minimal VRAM to what libxl expects.
+ */
+b_info-u.hvm.vga.kind = LIBXL_VGA_INTERFACE_TYPE_CIRRUS;
+switch (b_info-device_model_version) {
+case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL:
+min_vram = 4 * 1024; /* Actually the max, too */
+break;
+case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
+default:
+min_vram = 8 * 1024;
+}
+}
+b_info-video_memkb = (def-videos[0]-vram  min_vram) ?
+   def-videos[0]-vram :
+   LIBXL_MEMKB_DEFAULT;
+} else {
+libxl_defbool_set(b_info-u.hvm.nographic, 1);
+}
+
+/*
+ * When making the list of displays, only VNC and SDL types were
+ * taken into account. So it seems sensible to connect the default
+ * video device to the first in the vfb list.
+ *
+ * FIXME: Copy the structures and fixing the strings feels a bit dirty.
+ */
+if (d_config-num_vfbs) {
+libxl_device_vfb *vfb0 = d_config-vfbs[0];
+
+b_info-u.hvm.vnc = vfb0-vnc;
+if (VIR_STRDUP(b_info-u.hvm.vnc.listen, vfb0-vnc.listen)  0)
+return -1;
+if (VIR_STRDUP(b_info-u.hvm.vnc.passwd, vfb0-vnc.passwd)  0)
+return -1;
+b_info-u.hvm.sdl = vfb0-sdl;
+if (VIR_STRDUP(b_info-u.hvm.sdl.display, vfb0-sdl.display)  0)
+return -1;
+if (VIR_STRDUP(b_info

Re: [libvirt] [Xen-devel] [PATCH v2] libxl: Implement basic video device selection

2014-04-04 Thread Stefan Bader

On 04.04.2014 12:31, Stefan Bader wrote:
 On 04.04.2014 11:48, Ian Campbell wrote:
 On Fri, 2014-04-04 at 11:36 +0200, Stefan Bader wrote:
 +/*
 + * Take the first defined video device (graphics card) to display
 + * on the first graphics device (display).
 + * Right now only type and vram info is used and anything beside
 + * type xen and vga is mapped to cirrus.
 + */
 +if (def-nvideos) {
 +unsigned int min_vram = 8 * 1024;
 +
 +switch (def-videos[0]-type) {
 +case VIR_DOMAIN_VIDEO_TYPE_VGA:
 +case VIR_DOMAIN_VIDEO_TYPE_XEN:
 +b_info-u.hvm.vga.kind = LIBXL_VGA_INTERFACE_TYPE_STD;
 +/*
 + * Libxl enforces a minimal VRAM size of 8M when using
 + * LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL or
 + * 16M for LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN.
 + * Avoid build failures and go with the minimum if less
 + * is specified.

 Build failures? Do you mean domain build rather than libvirt build?

 I'm not sure about this fixing up the GIGO from the user side, but
 that's a libvirt policy decision I suppose? If it were me I would just
 let libxl provide the error and expect people to fix their domain config
 rather than silently giving them something other than what they asked
 for. What if increasing the VRAM causes a cascading failure due e.g. to
 lack of memory? That's going to be tricky to debug I think!
 
 In the end its a start a domain with such a config. Which seems to be what I

*sigh* That wasn't much better English. Yes, I meant a domain build or a failure
to start a domain...

 would end up with in my testing with an admittedly older version of 
 virt-manager
 connecting to a libvirtd running an initial version of this patch without 
 that part.
 The error seen on the front-end was something along the lines of failed to 
 get
 enough memory to start the guest (the libxl log on the other side had the
 better error message). And the gui always reduces the memory below the minimum
 for both the options (VGA and Xen).
 That is the reason I went for meh, go for the minimum anyways.
 
 And btw, it is already confusing enough as with cirrus, I get a 9M by default
 which is passed on to qemu on the command line and then ignored by it and one
 gets 32M in any way...
 
 -Stefan
 


 
 
 
 
 ___
 Xen-devel mailing list
 xen-de...@lists.xen.org
 http://lists.xen.org/xen-devel
 




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

Re: [libvirt] [PATCH v2] libxl: Implement basic video device selection

2014-04-04 Thread Stefan Bader

On 04.04.2014 12:34, Ian Campbell wrote:
 On Fri, 2014-04-04 at 12:31 +0200, Stefan Bader wrote:
 On 04.04.2014 11:48, Ian Campbell wrote:
 On Fri, 2014-04-04 at 11:36 +0200, Stefan Bader wrote:
 +/*
 + * Take the first defined video device (graphics card) to display
 + * on the first graphics device (display).
 + * Right now only type and vram info is used and anything beside
 + * type xen and vga is mapped to cirrus.
 + */
 +if (def-nvideos) {
 +unsigned int min_vram = 8 * 1024;
 +
 +switch (def-videos[0]-type) {
 +case VIR_DOMAIN_VIDEO_TYPE_VGA:
 +case VIR_DOMAIN_VIDEO_TYPE_XEN:
 +b_info-u.hvm.vga.kind = LIBXL_VGA_INTERFACE_TYPE_STD;
 +/*
 + * Libxl enforces a minimal VRAM size of 8M when using
 + * LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL or
 + * 16M for LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN.
 + * Avoid build failures and go with the minimum if less
 + * is specified.

 Build failures? Do you mean domain build rather than libvirt build?

 I'm not sure about this fixing up the GIGO from the user side, but
 that's a libvirt policy decision I suppose? If it were me I would just
 let libxl provide the error and expect people to fix their domain config
 rather than silently giving them something other than what they asked
 for. What if increasing the VRAM causes a cascading failure due e.g. to
 lack of memory? That's going to be tricky to debug I think!

 In the end its a start a domain with such a config. Which seems to be what I
 would end up with in my testing with an admittedly older version of 
 virt-manager
 connecting to a libvirtd running an initial version of this patch without 
 that part.
 The error seen on the front-end was something along the lines of failed to 
 get
 enough memory to start the guest (the libxl log on the other side had the
 better error message). And the gui always reduces the memory below the 
 minimum
 for both the options (VGA and Xen).
 That is the reason I went for meh, go for the minimum anyways.
 
 Does the libvirt protocol require the client to provide all the sizes
 etc with no provision for asking the server side to pick a sane default?

I think libvirt itself would be ok with a config that does not enforce a certain
amount of VRAM (libvirt devs correct me if I am talking non-sense here).
But together with the virt-manager front-end that tries to be helpful halfway.
I get a incorrect setting which I cannot change from that version of the gui.
Again, never versions might be better.
Just practically I expect some crazy people (like me *cough*) to try this from
an older Desktop release...

 
 And btw, it is already confusing enough as with cirrus, I get a 9M by default
 which is passed on to qemu on the command line and then ignored by it and one
 gets 32M in any way...
 
 Lovely!
 
 Ian.
 




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

Re: [libvirt] [PATCH v2] libxl: Implement basic video device selection

2014-04-04 Thread Stefan Bader

On 04.04.2014 14:56, Ian Campbell wrote:
 On Fri, 2014-04-04 at 14:51 +0200, Daniel P. Berrange wrote:
 On Fri, Apr 04, 2014 at 11:34:17AM +0100, Ian Campbell wrote:
 On Fri, 2014-04-04 at 12:31 +0200, Stefan Bader wrote:
 On 04.04.2014 11:48, Ian Campbell wrote:
 On Fri, 2014-04-04 at 11:36 +0200, Stefan Bader wrote:
 +/*
 + * Take the first defined video device (graphics card) to display
 + * on the first graphics device (display).
 + * Right now only type and vram info is used and anything beside
 + * type xen and vga is mapped to cirrus.
 + */
 +if (def-nvideos) {
 +unsigned int min_vram = 8 * 1024;
 +
 +switch (def-videos[0]-type) {
 +case VIR_DOMAIN_VIDEO_TYPE_VGA:
 +case VIR_DOMAIN_VIDEO_TYPE_XEN:
 +b_info-u.hvm.vga.kind = LIBXL_VGA_INTERFACE_TYPE_STD;
 +/*
 + * Libxl enforces a minimal VRAM size of 8M when using
 + * LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL or
 + * 16M for LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN.
 + * Avoid build failures and go with the minimum if less
 + * is specified.

 Build failures? Do you mean domain build rather than libvirt build?

 I'm not sure about this fixing up the GIGO from the user side, but
 that's a libvirt policy decision I suppose? If it were me I would just
 let libxl provide the error and expect people to fix their domain config
 rather than silently giving them something other than what they asked
 for. What if increasing the VRAM causes a cascading failure due e.g. to
 lack of memory? That's going to be tricky to debug I think!

 In the end its a start a domain with such a config. Which seems to be what 
 I
 would end up with in my testing with an admittedly older version of 
 virt-manager
 connecting to a libvirtd running an initial version of this patch without 
 that part.
 The error seen on the front-end was something along the lines of failed 
 to get
 enough memory to start the guest (the libxl log on the other side had the
 better error message). And the gui always reduces the memory below the 
 minimum
 for both the options (VGA and Xen).
 That is the reason I went for meh, go for the minimum anyways.

 Does the libvirt protocol require the client to provide all the sizes
 etc with no provision for asking the server side to pick a sane default?

 The XML does not have to include the VGA ram size. If it is omitted the
 we fill in a default value after initial parsing is done.
 
 I guess the issue is that whatever client Stefan is using is including
 the VGA ram size, with a value which it turns out is not allowed.

Right and the current fixup code is in there because I am too lazy to be
bothered to use virsh to fix up the vram size all the times. And in some way I
expected other users to be of the same mind. Or even not to find out what went
wrong at all.
I am open to let this drop on the upstream side and only carry the delta
locally. Whichever sounds more suitable for the upstream maintainers.

-Stefan
 
 That defualt can be hypervisor specific
 
 Great!
 
 Ian.
 And btw, it is already confusing enough as with cirrus, I get a 9M by 
 default
 which is passed on to qemu on the command line and then ignored by it and 
 one
 gets 32M in any way...

 Yeah, we were silently ignoring ram size for cirrus since QEMU doesn't let
 it be configured. Only QXL and I think stdvga is configurable.


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




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

Re: [libvirt] [PATCH v2] libxl: Implement basic video device selection

2014-04-04 Thread Stefan Bader

On 04.04.2014 15:17, Daniel P. Berrange wrote:
 On Fri, Apr 04, 2014 at 11:36:39AM +0200, Stefan Bader wrote:
 +static int
 +libxlSetBuildGraphics(virDomainDefPtr def, libxl_domain_config *d_config)
 +{
 +libxl_domain_build_info *b_info = d_config-b_info;
 +
 +/*
 + * Take the first defined video device (graphics card) to display
 + * on the first graphics device (display).
 + * Right now only type and vram info is used and anything beside
 + * type xen and vga is mapped to cirrus.
 + */
 +if (def-nvideos) {
 +unsigned int min_vram = 8 * 1024;
 +
 +switch (def-videos[0]-type) {
 +case VIR_DOMAIN_VIDEO_TYPE_VGA:
 +case VIR_DOMAIN_VIDEO_TYPE_XEN:
 +b_info-u.hvm.vga.kind = LIBXL_VGA_INTERFACE_TYPE_STD;
 +/*
 + * Libxl enforces a minimal VRAM size of 8M when using
 + * LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL or
 + * 16M for LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN.
 + * Avoid build failures and go with the minimum if less
 + * is specified.
 + */
 +switch (b_info-device_model_version) {
 +case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL:
 +min_vram = 8 * 1024;
 +break;
 +case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
 +default:
 +min_vram = 16 * 1024;
 +}
 +break;
 +default:
 +/*
 + * Ignore any other device type and use Cirrus. Again fix
 + * up the minimal VRAM to what libxl expects.
 + */
 
 We shouldn't do that 'default'. For any device type that Xen can't support
 we should report  VIR_ERR_CONFIG_UNSUPPORTED.

Ok, I could remove that. At some point it might be possible to enhance that
again. But that requires more careful thinking. At least with
device_model_version QEMU_XEN (not QEMU_XEN_TRADITIONAL and not to confuse with
device_model which just sets the path ;)) at least in theory there should be the
same support as for kvm... Anyway, just loud thinking.

Would you also rather want the VRAM fixup to be removed as well? I probably wait
a bit more for other feedback and then would do a v3...

-Stefan
 
 Regards,
 Daniel
 




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

Re: [libvirt] libxl fixes/improvements for libvirt

2014-04-03 Thread Stefan Bader

On 03.04.2014 17:45, Michal Privoznik wrote:
 On 27.03.2014 17:55, Stefan Bader wrote:
 Here several changes which improve the handling of Xen for me:

 * 0001-libxl-Use-id-from-virDomainObj-inside-the-driver.patch
This is a re-send as I initially submitted that as a reply to some
discussion. Starting from the visibly broken libxlDomainGetInfo when
creating or rebooting a guest with virt-manager, I replaced all usage
of dom-id with the more stable vm-def-id.
 * 0002-libxl-Set-disk-format-for-empty-cdrom-device.patch
Fixes start of a guest after ejecting the iso image from a virtual
CDROM drive (again using virt-manager).
 * 0003-libxl-Implement-basic-video-device-selection.patch
Should fix the occasional disagreement about the used VNC port and
adds the ability to select the standard VGA graphics from Xen.
VRAM size seemed to work with that. Only for Cirrus, while the qemu
command-line looks good, ones seems to end up with 32M.

 Note to people on the Xen list: I wonder whether libxenlight internally
 should somehow should fix up the situation where a caller sets up the
 video devices in the vfbs list but does not sync that with the information
 in the build info. Question probably is what the semantics should be.

 -Stefan

 
 Just for the record, I've pushed the first two patches. The last one has some
 issues, so I rather see it's second version.
 
 Michal
 
Ack, thanks. I will send out an updated version as soon as I get it done. Next
on my list though I cannot exactly tell when next happens. :)

-Stefan



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

[libvirt] [PATCH 3/3] libxl: Implement basic video device selection

2014-03-27 Thread Stefan Bader

This started as an investigation into an issue where libvirt (using the
libxl driver) and the Xen host, like an old couple, could not agree on
who is responsible for selecting the VNC port to use.

Things usually (and a bit surprisingly) did work because, just like that
old couple, they had the same idea on what to do by default. However it
was possible that this ended up in a big argument.

The problem is that display information exists in two different places:
in the vfbs list and in the build info. And for launching the device model,
only the latter is used. But that never gets initialized from libvirt. So
Xen allows the device model to select a default port while libvirt thinks
it has told Xen that this is done by libvirt (though the vfbs config).

While fixing that, I made a stab at actually evaluating the configuration
of the video device. So that it is now possible to at least decide between
a Cirrus or standard VGA emulation and to modify the VRAM within certain
limits using libvirt.

Signed-off-by: Stefan Bader stefan.ba...@canonical.com
---
 src/libxl/libxl_conf.c |   85 
 1 file changed, 85 insertions(+)

diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index b8de72a..fdd2726 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -1298,6 +1298,82 @@ libxlMakeCapabilities(libxl_ctx *ctx)
 return NULL;
 }
 
+static void
+libxlSetBuildGraphics(virDomainDefPtr def, libxl_domain_config *d_config)
+{
+libxl_domain_build_info *b_info = d_config-b_info;
+
+/*
+ * Take the first defined video device (graphics card) to display
+ * on the first graphics device (display).
+ * Right now only type and vram info is used and anything beside
+ * type xen and vga is mapped to cirrus.
+ */
+if (def-nvideos) {
+unsigned int min_vram = 8 * 1024;
+
+switch (def-videos[0]-type) {
+case VIR_DOMAIN_VIDEO_TYPE_VGA:
+case VIR_DOMAIN_VIDEO_TYPE_XEN:
+b_info-u.hvm.vga.kind = LIBXL_VGA_INTERFACE_TYPE_STD;
+/*
+ * Libxl enforces a minimal VRAM size of 8M when using
+ * LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL or
+ * 16M for LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN.
+ * Avoid build failures and go with the minimum if less
+ * is specified.
+ */
+switch (b_info-device_model_version) {
+case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL:
+min_vram = 8 * 1024;
+break;
+case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
+default:
+min_vram = 16 * 1024;
+}
+break;
+default:
+/*
+ * Ignore any other device type and use Cirrus. Again fix
+ * up the minimal VRAM to what libxl expects.
+ */
+b_info-u.hvm.vga.kind = LIBXL_VGA_INTERFACE_TYPE_CIRRUS;
+switch (b_info-device_model_version) {
+case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL:
+min_vram = 4 * 1024; /* Actually the max, too */
+break;
+case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
+default:
+min_vram = 8 * 1024;
+}
+}
+b_info-video_memkb = (def-videos[0]-vram  min_vram) ?
+   def-videos[0]-vram :
+   LIBXL_MEMKB_DEFAULT;
+} else {
+libxl_defbool_set(b_info-u.hvm.nographic, 1);
+}
+
+/*
+ * When making the list of displays, only VNC and SDL types were
+ * taken into account. So it seems sensible to connect the default
+ * video device to the first in the vfb list.
+ *
+ * FIXME: Copy the structures and fixing the strings feels a bit dirty.
+ */
+if (d_config-num_vfbs) {
+libxl_device_vfb *vfb0 = d_config-vfbs[0];
+
+   b_info-u.hvm.vnc = vfb0-vnc;
+VIR_STRDUP(b_info-u.hvm.vnc.listen, vfb0-vnc.listen);
+VIR_STRDUP(b_info-u.hvm.vnc.passwd, vfb0-vnc.passwd);
+b_info-u.hvm.sdl = vfb0-sdl;
+VIR_STRDUP(b_info-u.hvm.sdl.display, vfb0-sdl.display);
+VIR_STRDUP(b_info-u.hvm.sdl.xauthority, vfb0-sdl.xauthority);
+VIR_STRDUP(b_info-u.hvm.keymap, vfb0-keymap);
+}
+}
+
 int
 libxlBuildDomainConfig(libxlDriverPrivatePtr driver,
virDomainObjPtr vm, libxl_domain_config *d_config)
@@ -1325,6 +1401,15 @@ libxlBuildDomainConfig(libxlDriverPrivatePtr driver,
 if (libxlMakePciList(def, d_config)  0)
 return -1;
 
+/*
+ * Now that any potential VFBs are defined, it is time to update the
+ * build info with the data of the primary display. Some day libxl
+ * might

[libvirt] libxl fixes/improvements for libvirt

2014-03-27 Thread Stefan Bader

Here several changes which improve the handling of Xen for me:

* 0001-libxl-Use-id-from-virDomainObj-inside-the-driver.patch
  This is a re-send as I initially submitted that as a reply to some
  discussion. Starting from the visibly broken libxlDomainGetInfo when
  creating or rebooting a guest with virt-manager, I replaced all usage
  of dom-id with the more stable vm-def-id.
* 0002-libxl-Set-disk-format-for-empty-cdrom-device.patch
  Fixes start of a guest after ejecting the iso image from a virtual
  CDROM drive (again using virt-manager).
* 0003-libxl-Implement-basic-video-device-selection.patch
  Should fix the occasional disagreement about the used VNC port and
  adds the ability to select the standard VGA graphics from Xen.
  VRAM size seemed to work with that. Only for Cirrus, while the qemu
  command-line looks good, ones seems to end up with 32M.

Note to people on the Xen list: I wonder whether libxenlight internally
should somehow should fix up the situation where a caller sets up the
video devices in the vfbs list but does not sync that with the information
in the build info. Question probably is what the semantics should be.

-Stefan

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

[libvirt] [PATCH 2/3] libxl: Set disk format for empty cdrom device

2014-03-27 Thread Stefan Bader

The XML config for a CDROM device can be without a source path,
indicating that there is no media present. Without this change
the libxl driver fails to start a guest in that case because
the libxl library checks for the LIBXL_DISK_FORMAT_EMPTY format
type and tries to stat the NULL pointer that gets passed on.

 libxl: error: libxl_device.c:265:libxl__device_disk_set_backend:
 Disk vdev=hdc failed to stat: (null): Bad address

Signed-off-by: Stefan Bader stefan.ba...@canonical.com
---
 src/libxl/libxl_conf.c |3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index de6f7ce..b8de72a 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -827,6 +827,9 @@ libxlMakeDisk(virDomainDiskDefPtr l_disk, libxl_device_disk 
*x_disk)
 x_disk-removable = 1;
 x_disk-readwrite = !l_disk-readonly;
 x_disk-is_cdrom = l_disk-device == VIR_DOMAIN_DISK_DEVICE_CDROM ? 1 : 0;
+/* An empty CDROM must have the empty format, otherwise libxl fails. */
+if (x_disk-is_cdrom  !x_disk-pdev_path)
+x_disk-format = LIBXL_DISK_FORMAT_EMPTY;
 if (l_disk-transient) {
 virReportError(VIR_ERR_INTERNAL_ERROR, %s,
_(libxenlight does not support transient disks));
-- 
1.7.9.5

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

[libvirt] [PATCH 1/3] libxl: Use id from virDomainObj inside the driver

2014-03-27 Thread Stefan Bader

There is a domain id in the virDomain structure as well as in the
virDomainObj structure. While the former can become stale the latter
is kept up to date. So it is safer to always (virDomainObjPtr)-def-id
internally.

This will fix issues seen when managing Xen guests through libvirt from
virt-manager (not being able to get domain info after define or reboot).
This was caused both though libxlDomainGetInfo() only but there were
a lot of places that might potentially cause issues, too.

Signed-off-by: Stefan Bader stefan.ba...@canonical.com
---
 src/libxl/libxl_driver.c |   75 +++---
 1 file changed, 38 insertions(+), 37 deletions(-)

diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index fc97db4..b5061df 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -770,10 +770,10 @@ libxlDomainSuspend(virDomainPtr dom)
 priv = vm-privateData;
 
 if (virDomainObjGetState(vm, NULL) != VIR_DOMAIN_PAUSED) {
-if (libxl_domain_pause(priv-ctx, dom-id) != 0) {
+if (libxl_domain_pause(priv-ctx, vm-def-id) != 0) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
_(Failed to suspend domain '%d' with libxenlight),
-   dom-id);
+   vm-def-id);
 goto endjob;
 }
 
@@ -829,10 +829,10 @@ libxlDomainResume(virDomainPtr dom)
 priv = vm-privateData;
 
 if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_PAUSED) {
-if (libxl_domain_unpause(priv-ctx, dom-id) != 0) {
+if (libxl_domain_unpause(priv-ctx, vm-def-id) != 0) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
_(Failed to resume domain '%d' with libxenlight),
-   dom-id);
+   vm-def-id);
 goto endjob;
 }
 
@@ -883,10 +883,10 @@ libxlDomainShutdownFlags(virDomainPtr dom, unsigned int 
flags)
 }
 
 priv = vm-privateData;
-if (libxl_domain_shutdown(priv-ctx, dom-id) != 0) {
+if (libxl_domain_shutdown(priv-ctx, vm-def-id) != 0) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
_(Failed to shutdown domain '%d' with libxenlight),
-   dom-id);
+   vm-def-id);
 goto cleanup;
 }
 
@@ -930,10 +930,10 @@ libxlDomainReboot(virDomainPtr dom, unsigned int flags)
 }
 
 priv = vm-privateData;
-if (libxl_domain_reboot(priv-ctx, dom-id) != 0) {
+if (libxl_domain_reboot(priv-ctx, vm-def-id) != 0) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
_(Failed to reboot domain '%d' with libxenlight),
-   dom-id);
+   vm-def-id);
 goto cleanup;
 }
 ret = 0;
@@ -974,7 +974,7 @@ libxlDomainDestroyFlags(virDomainPtr dom,
 priv = vm-privateData;
 if (libxl_domain_destroy(priv-ctx, vm-def-id, NULL)  0) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
-   _(Failed to destroy domain '%d'), dom-id);
+   _(Failed to destroy domain '%d'), vm-def-id);
 goto cleanup;
 }
 
@@ -1105,10 +1105,10 @@ libxlDomainSetMemoryFlags(virDomainPtr dom, unsigned 
long newmem,
 
 if (flags  VIR_DOMAIN_MEM_LIVE) {
 priv = vm-privateData;
-if (libxl_domain_setmaxmem(priv-ctx, dom-id, newmem)  0) {
+if (libxl_domain_setmaxmem(priv-ctx, vm-def-id, newmem)  0) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
_(Failed to set maximum memory for domain '%d'
-  with libxenlight), dom-id);
+  with libxenlight), vm-def-id);
 goto endjob;
 }
 }
@@ -1138,13 +1138,13 @@ libxlDomainSetMemoryFlags(virDomainPtr dom, unsigned 
long newmem,
 priv = vm-privateData;
 /* Unlock virDomainObj while ballooning memory */
 virObjectUnlock(vm);
-res = libxl_set_memory_target(priv-ctx, dom-id, newmem, 0,
+res = libxl_set_memory_target(priv-ctx, vm-def-id, newmem, 0,
   /* force */ 1);
 virObjectLock(vm);
 if (res  0) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
_(Failed to set memory for domain '%d'
-  with libxenlight), dom-id);
+  with libxenlight), vm-def-id);
 goto endjob;
 }
 }
@@ -1202,9 +1202,10 @@ libxlDomainGetInfo(virDomainPtr dom, virDomainInfoPtr 
info)
 info-memory = vm-def-mem.cur_balloon;
 info-maxMem = vm-def-mem.max_balloon;
 } else {
-if (libxl_domain_info(priv-ctx, d_info, dom-id) != 0) {
+if (libxl_domain_info(priv-ctx, d_info, vm-def-id) != 0) {
 virReportError(VIR_ERR_INTERNAL_ERROR

[libvirt] libxl: Issues with virt-manager when used to manager Xen domains

2014-03-25 Thread Stefan Bader

This started off with some regression testing after going forward to Xen-4.4. We
currently would pair that with a libvirt version 1.2.2 and right now operations
through virsh seem to be working (mostly) well. But when using virt-manager (not
the most up-to-date versions but some combinations that quite likely will occur
(0.9.1 and 0.9.5)) there are some issues.

Two symptoms seem to be caused by the same underlying problem which is caused by
the way virt-manager interacts with libvirt. That seems to be that virt-manager
acquires a reference to virDomainPtr only once and then uses that for subsequent
call. This is a problem because both virDomainPtr object and virDomainObjPtr
objects contains a domid but only the latter gets updated.

So both, creating a new guest (which must be a define step and then use the
handle to start the guest) as well as rebooting a guest cause problems for
virt-manager. Mainly because of libxlDomainGetInfo() which retrieves the
virDomainObjPtr for a given virDommainPtr. Then checks the domain object to be
active or not but uses the domid from the domain to call into libxl:

if (!(vm = libxlDomObjFromDomain(dom)))
goto cleanup;
...
if (!virDomainObjIsActive(vm)) {
info-cpuTime = 0;
info-memory = vm-def-mem.cur_balloon;
info-maxMem = vm-def-mem.max_balloon;
} else {
if (libxl_domain_info(priv-ctx, d_info, dom-id) != 0) {

This fails either with dom-id being -1 (still) or using the old id from before
the reboot. A dirty hack seems to avoid this:

static virDomainObjPtr
libxlDomObjFromDomain(virDomainPtr dom)
{
virDomainObjPtr vm;
libxlDriverPrivatePtr driver = dom-conn-privateData;
char uuidstr[VIR_UUID_STRING_BUFLEN];

vm = virDomainObjListFindByUUID(driver-domains, dom-uuid);
if (!vm) {
virUUIDFormat(dom-uuid, uuidstr);
virReportError(VIR_ERR_NO_DOMAIN,
   _(no domain with matching uuid '%s' (%s)),
   uuidstr, dom-name);
return NULL;
-   }
+   } else {
+   if (dom-id != vm-def-id) {
+   VIR_WARN(libxlDomObjFromDomain: domid changed (%d-%d),
+dom-id, vm-def-id);
+   dom-id = vm-def-id;
+   }
+   }

return vm;
}

Not really sure this is the right way to go. Also because that warning above
happens a lot more (repeatedly) than I would have expected. This may or may not
be related to the next question.

Not a problem but more a confusion on my side: virGetDomain() has commentary
that says it will return either a pointer to an existing object or one to a new
one. Its just, I cannot see that done in the code. Is that a future plan or was
that way but was removed?

I must admit I have not looked deeper into that but one explanation of the
repeated domid mismatch would be if the virDomainPtr that virt-manager uses to
communicate with libvirt would be cloned before doing things inside libvirt. And
holding that handle for so long and not obtaining a fresh one through the
various lookup interfaces each time could be a mistake of virt-manager.
Unfortunately one that needs to be handled in some way.

-Stefan



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

[libvirt] [PATCH] libxl: Create log directory earlier

2014-03-25 Thread Stefan Bader

Commit d9f19c30d054c86b15a304f4118baa4fa75af9d2 moved a lot of the
configuration setup into libxlDriverConfigNew().
However that tries to create the libxl/libxl-driver.log before the
libxl directory gets created in libxlStateInitialize().

This causes the daemon to fail on systems that have not had the directory
created before.

Move the code to create the libxl directory into libxlDriverConfigNew().

Signed-off-by: Stefan Bader stefan.ba...@canonical.com
---
 src/libxl/libxl_conf.c   |8 
 src/libxl/libxl_driver.c |7 ---
 2 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index a6bf1cf..56bb84a 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -1108,6 +1108,14 @@ libxlDriverConfigNew(void)
 if (virAsprintf(log_file, %s/libxl-driver.log, cfg-logDir)  0)
 goto error;
 
+if (virFileMakePath(cfg-logDir)  0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _(failed to create log dir '%s': %s),
+   cfg-logDir,
+   virStrerror(errno, ebuf, sizeof(ebuf)));
+goto error;
+}
+
 if ((cfg-logger_file = fopen(log_file, a)) == NULL)  {
 VIR_ERROR(_(Failed to create log file '%s': %s),
   log_file, virStrerror(errno, ebuf, sizeof(ebuf)));
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index 0c6672b..e6316a7 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -306,13 +306,6 @@ libxlStateInitialize(bool privileged,
 goto error;
 
 libxl_driver-config = cfg;
-if (virFileMakePath(cfg-logDir)  0) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
-   _(failed to create log dir '%s': %s),
-   cfg-logDir,
-   virStrerror(errno, ebuf, sizeof(ebuf)));
-goto error;
-}
 if (virFileMakePath(cfg-stateDir)  0) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
_(failed to create state dir '%s': %s),
-- 
1.7.9.5

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

Re: [libvirt] libxl: Issues with virt-manager when used to manager Xen domains

2014-03-25 Thread Stefan Bader

On 25.03.2014 16:36, Daniel P. Berrange wrote:
 On Tue, Mar 25, 2014 at 04:22:54PM +0100, Stefan Bader wrote:
 This started off with some regression testing after going forward to 
 Xen-4.4. We
 currently would pair that with a libvirt version 1.2.2 and right now 
 operations
 through virsh seem to be working (mostly) well. But when using virt-manager 
 (not
 the most up-to-date versions but some combinations that quite likely will 
 occur
 (0.9.1 and 0.9.5)) there are some issues.

 Two symptoms seem to be caused by the same underlying problem which is 
 caused by
 the way virt-manager interacts with libvirt. That seems to be that 
 virt-manager
 acquires a reference to virDomainPtr only once and then uses that for 
 subsequent
 call. This is a problem because both virDomainPtr object and virDomainObjPtr
 objects contains a domid but only the latter gets updated.

 So both, creating a new guest (which must be a define step and then use the
 handle to start the guest) as well as rebooting a guest cause problems for
 virt-manager. Mainly because of libxlDomainGetInfo() which retrieves the
 virDomainObjPtr for a given virDommainPtr. Then checks the domain object to 
 be
 active or not but uses the domid from the domain to call into libxl:

 if (!(vm = libxlDomObjFromDomain(dom)))
 goto cleanup;
 ...
 if (!virDomainObjIsActive(vm)) {
 info-cpuTime = 0;
 info-memory = vm-def-mem.cur_balloon;
 info-maxMem = vm-def-mem.max_balloon;
 } else {
 if (libxl_domain_info(priv-ctx, d_info, dom-id) != 0) {

 This fails either with dom-id being -1 (still) or using the old id from 
 before
 the reboot. A dirty hack seems to avoid this:
 
 Yep, this is a pretty commonly hit problem. The solution is to *never*
 access dom-id in the driver code at all. eg In this code snippet above
 you could use  vm-def-id instead which should be up to date.

OK, it just seems for something that should not be done, it is done plenty of
times in the libxl_driver codebase right now. At least pause and unpause seem to
do the same...

-Stefan
 
 Regards,
 Daniel
 




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

Re: [libvirt] libxl: Issues with virt-manager when used to manager Xen domains

2014-03-25 Thread Stefan Bader

On 25.03.2014 16:46, Daniel P. Berrange wrote:
 On Tue, Mar 25, 2014 at 04:42:25PM +0100, Stefan Bader wrote:
 On 25.03.2014 16:36, Daniel P. Berrange wrote:
 On Tue, Mar 25, 2014 at 04:22:54PM +0100, Stefan Bader wrote:
 This started off with some regression testing after going forward to 
 Xen-4.4. We
 currently would pair that with a libvirt version 1.2.2 and right now 
 operations
 through virsh seem to be working (mostly) well. But when using 
 virt-manager (not
 the most up-to-date versions but some combinations that quite likely will 
 occur
 (0.9.1 and 0.9.5)) there are some issues.

 Two symptoms seem to be caused by the same underlying problem which is 
 caused by
 the way virt-manager interacts with libvirt. That seems to be that 
 virt-manager
 acquires a reference to virDomainPtr only once and then uses that for 
 subsequent
 call. This is a problem because both virDomainPtr object and 
 virDomainObjPtr
 objects contains a domid but only the latter gets updated.

 So both, creating a new guest (which must be a define step and then use the
 handle to start the guest) as well as rebooting a guest cause problems for
 virt-manager. Mainly because of libxlDomainGetInfo() which retrieves the
 virDomainObjPtr for a given virDommainPtr. Then checks the domain object 
 to be
 active or not but uses the domid from the domain to call into libxl:

 if (!(vm = libxlDomObjFromDomain(dom)))
 goto cleanup;
 ...
 if (!virDomainObjIsActive(vm)) {
 info-cpuTime = 0;
 info-memory = vm-def-mem.cur_balloon;
 info-maxMem = vm-def-mem.max_balloon;
 } else {
 if (libxl_domain_info(priv-ctx, d_info, dom-id) != 0) {

 This fails either with dom-id being -1 (still) or using the old id from 
 before
 the reboot. A dirty hack seems to avoid this:

 Yep, this is a pretty commonly hit problem. The solution is to *never*
 access dom-id in the driver code at all. eg In this code snippet above
 you could use  vm-def-id instead which should be up to date.

 OK, it just seems for something that should not be done, it is done plenty of
 times in the libxl_driver codebase right now. At least pause and unpause 
 seem to
 do the same...
 
 I expect those pretty much all need to be fixed

All-right, thanks for confirming. :) Will work on that.

-Stefan
 
 Regards,
 Daniel
 




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

[libvirt] [PATCH] libxl: Use id from virDomainObj inside the driver

2014-03-25 Thread Stefan Bader

There is a domain id in the virDomain structure as well as in the
virDomainObj structure. While the former can become stale the latter
is kept up to date. So it is safer to always (virDomainObjPtr)-def-id
internally.

This will fix issues seen when managing Xen guests through libvirt from
virt-manager (not being able to get domain info after define or reboot).
This was caused both though libxlDomainGetInfo() only but there were
a lot of places that might potentially cause issues, too.

Signed-off-by: Stefan Bader stefan.ba...@canonical.com
---
 src/libxl/libxl_driver.c |   75 +++---
 1 file changed, 38 insertions(+), 37 deletions(-)

diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index fc97db4..b5061df 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -770,10 +770,10 @@ libxlDomainSuspend(virDomainPtr dom)
 priv = vm-privateData;
 
 if (virDomainObjGetState(vm, NULL) != VIR_DOMAIN_PAUSED) {
-if (libxl_domain_pause(priv-ctx, dom-id) != 0) {
+if (libxl_domain_pause(priv-ctx, vm-def-id) != 0) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
_(Failed to suspend domain '%d' with libxenlight),
-   dom-id);
+   vm-def-id);
 goto endjob;
 }
 
@@ -829,10 +829,10 @@ libxlDomainResume(virDomainPtr dom)
 priv = vm-privateData;
 
 if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_PAUSED) {
-if (libxl_domain_unpause(priv-ctx, dom-id) != 0) {
+if (libxl_domain_unpause(priv-ctx, vm-def-id) != 0) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
_(Failed to resume domain '%d' with libxenlight),
-   dom-id);
+   vm-def-id);
 goto endjob;
 }
 
@@ -883,10 +883,10 @@ libxlDomainShutdownFlags(virDomainPtr dom, unsigned int 
flags)
 }
 
 priv = vm-privateData;
-if (libxl_domain_shutdown(priv-ctx, dom-id) != 0) {
+if (libxl_domain_shutdown(priv-ctx, vm-def-id) != 0) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
_(Failed to shutdown domain '%d' with libxenlight),
-   dom-id);
+   vm-def-id);
 goto cleanup;
 }
 
@@ -930,10 +930,10 @@ libxlDomainReboot(virDomainPtr dom, unsigned int flags)
 }
 
 priv = vm-privateData;
-if (libxl_domain_reboot(priv-ctx, dom-id) != 0) {
+if (libxl_domain_reboot(priv-ctx, vm-def-id) != 0) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
_(Failed to reboot domain '%d' with libxenlight),
-   dom-id);
+   vm-def-id);
 goto cleanup;
 }
 ret = 0;
@@ -974,7 +974,7 @@ libxlDomainDestroyFlags(virDomainPtr dom,
 priv = vm-privateData;
 if (libxl_domain_destroy(priv-ctx, vm-def-id, NULL)  0) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
-   _(Failed to destroy domain '%d'), dom-id);
+   _(Failed to destroy domain '%d'), vm-def-id);
 goto cleanup;
 }
 
@@ -1105,10 +1105,10 @@ libxlDomainSetMemoryFlags(virDomainPtr dom, unsigned 
long newmem,
 
 if (flags  VIR_DOMAIN_MEM_LIVE) {
 priv = vm-privateData;
-if (libxl_domain_setmaxmem(priv-ctx, dom-id, newmem)  0) {
+if (libxl_domain_setmaxmem(priv-ctx, vm-def-id, newmem)  0) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
_(Failed to set maximum memory for domain '%d'
-  with libxenlight), dom-id);
+  with libxenlight), vm-def-id);
 goto endjob;
 }
 }
@@ -1138,13 +1138,13 @@ libxlDomainSetMemoryFlags(virDomainPtr dom, unsigned 
long newmem,
 priv = vm-privateData;
 /* Unlock virDomainObj while ballooning memory */
 virObjectUnlock(vm);
-res = libxl_set_memory_target(priv-ctx, dom-id, newmem, 0,
+res = libxl_set_memory_target(priv-ctx, vm-def-id, newmem, 0,
   /* force */ 1);
 virObjectLock(vm);
 if (res  0) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
_(Failed to set memory for domain '%d'
-  with libxenlight), dom-id);
+  with libxenlight), vm-def-id);
 goto endjob;
 }
 }
@@ -1202,9 +1202,10 @@ libxlDomainGetInfo(virDomainPtr dom, virDomainInfoPtr 
info)
 info-memory = vm-def-mem.cur_balloon;
 info-maxMem = vm-def-mem.max_balloon;
 } else {
-if (libxl_domain_info(priv-ctx, d_info, dom-id) != 0) {
+if (libxl_domain_info(priv-ctx, d_info, vm-def-id) != 0) {
 virReportError(VIR_ERR_INTERNAL_ERROR

Re: [libvirt] [PATCH] Avoid warning message from libxl driver on non-Xen kernels

2014-03-17 Thread Stefan Bader

On 17.03.2014 13:15, Daniel P. Berrange wrote:
 +if (!virFileExists(/proc/xen/capabilities)) {
 +VIR_INFO(Disabling driver as /proc/xen/capabilities does not 
 exist);
 +return false;
 +}

Oh right, I should have checked the log more carefully in all cases. It would
work in all cases but adding unnecessary error messages is not good.



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

[libvirt] libvirt: [PATCH] libxl: Check for control_d string to decide about dom0

2014-03-12 Thread Stefan Bader

I have been looking into a bug report (see BugLink) which reported
libvirt to fail starting inside a Xen guest. Upon further investigation
I found that some tools that help monitoring Xen guests will mount
xenfs to /proc/xen. This will create a capabilities files there even
if the guest is not dom0. However it will return nothing when reading
from it.

Ian, just to sanity check myself. I looked at the xenfs code and to
me there only seem to be those two outcomes (either control_d for
running in dom0 or notrhing if not).

With the following patch applied, libvirt starts up correctly in
the normal guests (with xenfs mounted) without initializing libxl.
And also in dom0 where it still enables the libxl driver (if the
xl toolstack is selected).

-Stefan

From f11949caca6dfe1a802472a2a6d4fe760115ccc6 Mon Sep 17 00:00:00 2001
From: Stefan Bader stefan.ba...@canonical.com
Date: Wed, 12 Mar 2014 11:37:16 +0100
Subject: [PATCH] libxl: Check for control_d string to decide about dom0

As soon as any guest mounts xenfs to /proc/xen, there is a capabilities
file in that directory. However it returns nothing when reading from it.
Change the test to actually check the contents of the file.

BugLink: http://bugs.launchpad.net/bugs/1248025

Signed-off-by: Stefan Bader stefan.ba...@canonical.com
---
 src/libxl/libxl_driver.c |   14 --
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index 65d80a2..844e828 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -944,6 +944,7 @@ libxlDriverShouldLoad(bool privileged)
 bool ret = false;
 virCommandPtr cmd;
 int status;
+char *output = NULL;
 
 /* Don't load if non-root */
 if (!privileged) {
@@ -951,8 +952,17 @@ libxlDriverShouldLoad(bool privileged)
 return ret;
 }
 
-/* Don't load if not running on a Xen control domain (dom0) */
-if (!virFileExists(/proc/xen/capabilities)) {
+/*
+ * Don't load if not running on a Xen control domain (dom0). It is not
+ * sufficient to check for the file to exist as any guest can mount
+ * xenfs to /proc/xen.
+ */
+status = virFileReadAll(/proc/xen/capabilities, 10. output);
+if (status = 0) {
+status = strncmp(output, control_d, 9);
+}
+VIR_FREE(output);
+if (status) {
 VIR_INFO(No Xen capabilities detected, probably not running 
  in a Xen Dom0.  Disabling libxenlight driver);
 
-- 
1.7.9.5

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

Re: [libvirt] libvirt: [PATCH] libxl: Check for control_d string to decide about dom0

2014-03-12 Thread Stefan Bader

On 12.03.2014 13:08, Ian Campbell wrote:
 On Wed, 2014-03-12 at 13:03 +0100, Stefan Bader wrote:
 I have been looking into a bug report (see BugLink) which reported
 libvirt to fail starting inside a Xen guest. Upon further investigation
 I found that some tools that help monitoring Xen guests will mount
 xenfs to /proc/xen. This will create a capabilities files there even
 if the guest is not dom0. However it will return nothing when reading
 from it.
 
 This seems consistent with the xencommons initscript which does:
 # run this script only in dom0:
 # no capabilities file in xenlinux domU kernel
 # empty capabilities file in pv_ops domU kernel
 if test -f /proc/xen/capabilities  \
! grep -q control_d /proc/xen/capabilities ; then
 exit 0
 fi
 

 Ian, just to sanity check myself. I looked at the xenfs code and to
 me there only seem to be those two outcomes (either control_d for
 running in dom0 or notrhing if not).

 With the following patch applied, libvirt starts up correctly in
 the normal guests (with xenfs mounted) without initializing libxl.
 And also in dom0 where it still enables the libxl driver (if the
 xl toolstack is selected).

 -Stefan

 From f11949caca6dfe1a802472a2a6d4fe760115ccc6 Mon Sep 17 00:00:00 2001
 From: Stefan Bader stefan.ba...@canonical.com
 Date: Wed, 12 Mar 2014 11:37:16 +0100
 Subject: [PATCH] libxl: Check for control_d string to decide about dom0

 As soon as any guest mounts xenfs to /proc/xen, there is a capabilities
 file in that directory. However it returns nothing when reading from it.
 Change the test to actually check the contents of the file.

 BugLink: http://bugs.launchpad.net/bugs/1248025

 Signed-off-by: Stefan Bader stefan.ba...@canonical.com
 ---
  src/libxl/libxl_driver.c |   14 --
  1 file changed, 12 insertions(+), 2 deletions(-)

 diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
 index 65d80a2..844e828 100644
 --- a/src/libxl/libxl_driver.c
 +++ b/src/libxl/libxl_driver.c
 @@ -944,6 +944,7 @@ libxlDriverShouldLoad(bool privileged)
  bool ret = false;
  virCommandPtr cmd;
  int status;
 +char *output = NULL;
  
  /* Don't load if non-root */
  if (!privileged) {
 @@ -951,8 +952,17 @@ libxlDriverShouldLoad(bool privileged)
  return ret;
  }
  
 -/* Don't load if not running on a Xen control domain (dom0) */
 -if (!virFileExists(/proc/xen/capabilities)) {
 +/*
 + * Don't load if not running on a Xen control domain (dom0). It is not
 + * sufficient to check for the file to exist as any guest can mount
 + * xenfs to /proc/xen.
 + */
 +status = virFileReadAll(/proc/xen/capabilities, 10. output);
 
 Is this . supposed to be a ,?

Darn, I thought I had fixed that. Unfortunately in the wrong place. Yes, that
should be a ,. Sorry

-Stefan
 
 +if (status = 0) {
 +status = strncmp(output, control_d, 9);
 +}
 +VIR_FREE(output);
 +if (status) {
  VIR_INFO(No Xen capabilities detected, probably not running 
   in a Xen Dom0.  Disabling libxenlight driver);
  
 
 




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

[libvirt] [PATCH] libxl: Fix devid init in libxlMakeNicList

2014-01-08 Thread Stefan Bader

This basically reverts commit ba64b97134a6129a48684f22f31be92c3b6eef96
libxl: Allow libxl to set NIC devid. However assigning devid's
before calling libxlMakeNic does not work as that is calling
libxl_device_nic_init which sets it back to -1.
Right now auto-assignment only works in the hotplug case. But even if
that would be fixed at some point (if that is possible at all), this
would add a weird dependency between Xen and libvirt versions.
The change here should accept any auto-assignment that makes it into
libxl_device_nic_init. My understanding is that a caller always is
allowed to make the devid choice itself. And assuming libxlMakeNicList
is only used on domain creation, a sequential numbering should be ok.

Signed-off-by: Stefan Bader stefan.ba...@canonical.com
---
 src/libxl/libxl_conf.c |7 +++
 1 file changed, 7 insertions(+)

diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index 04d01af..4cefadf 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -918,6 +918,13 @@ libxlMakeNicList(virDomainDefPtr def,  libxl_domain_config 
*d_config)
 for (i = 0; i  nnics; i++) {
 if (libxlMakeNic(def, l_nics[i], x_nics[i]))
 goto error;
+/*
+ * The devid (at least right now) will not get initialized by
+ * libxl in the setup case but is required for starting the
+ * device-model.
+ */
+if (x_nics[i].devid  0)
+x_nics[i].devid = i;
 }
 
 d_config-nics = x_nics;
-- 
1.7.9.5

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

Re: [libvirt] [PATCH] libxl: Fix devid init in libxlMakeNicList

2014-01-08 Thread Stefan Bader

On 08.01.2014 11:42, Ian Campbell wrote:
 On Wed, 2014-01-08 at 11:39 +0100, Stefan Bader wrote:
 This basically reverts commit ba64b97134a6129a48684f22f31be92c3b6eef96
 libxl: Allow libxl to set NIC devid. However assigning devid's
 before calling libxlMakeNic does not work as that is calling
 libxl_device_nic_init which sets it back to -1.
 Right now auto-assignment only works in the hotplug case. But even if
 that would be fixed at some point (if that is possible at all),
 
 I think it should be -- any chance you could prepare a patch to libxl as
 well?

Not sure that is that simple. At least not in libxl_device_nic_init. The way I
understand this is done does not seem to allow a good solution. Creation looks
to allow initializing NIC definitions in some array that is not connected to a
specific domain, yet.
I suppose what I could come up with is the kind of modification I was proposing
in one of those emails last year (sounds like an awful long time :)). So at the
point where the device-model is about to get started and the code already loops
over the NIC definitions. That won't give the libvirt path default devid's but
any other user that has not done yet.

-Stefan



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

Re: [libvirt] [Xen-devel] Setting devid for emulated NICs (Xen 4.3.1 / libvirt 1.2.0) using libxl driver

2013-12-20 Thread Stefan Bader

On 19.12.2013 18:57, Ian Campbell wrote:
 On Thu, 2013-12-19 at 18:06 +0100, Stefan Bader wrote:
 How about we:
   * move the init to setdefault to catch the single NIC added via
 hotplug case

 Init of devid?
 
 Yes, sorry for not being clear.
 
  Hm, would that work as I am not sure there is a simple way of
 differentiating between a NIC config for a single hotplug and one that is 
 part
 of a create-time array...
 
 The create time array would be initialised with devid's != -1 as part of
 the steps outlined in the following bullets, so setdefault woudln't
 touch it again.
 

   * we add somewhere early in the domain create path a call to a
 function which assigns devids to an entire array of devices (and
 do it for all the different device types). Perhaps in
 initiate_domain_create() after the calls to
 libxl__domain_create_info_setdefault and
 libxl__domain_build_info_setdefault but before the loop calling
 libxl__device_disk_setdefault for the disks.
   * perhaps that same function should call setdefault too, after
 having assigned the device, rather than it being done later in
 an adhoc way?

 Does that sound at all plausible?

 I wonder, well this won't help for any other device types (maybe not really
 needed), what about just adding the following to the existing loop in
 domcreate_launch_dm (just a brain dump, not even tried to compile):

 for (i = 0; i  d_config-num_nics; i++) {
 /* We have to init the nic here, because we still haven't
  * called libxl_device_nic_add at this point, but qemu needs
  * the nic information to be complete.
  */
 ret = libxl__device_nic_setdefault(gc, d_config-nics[i], domid);
 if (ret)
 goto error_out;
 +if (d_config-nics[i].devid  0)
 +d_config-nics[i].devid = i;
 }

 Of course this a gain won't work well if the caller had assigned some devids 
 but
 not other.
 
 Indeed.
 
 Ok, maybe do the loop twice, first round sets default and picks the
 highest pre-assigned devid and second round makes sure any still unassigned 
 ones
 are set to ++that.
 
 That would potentially leave holes, I don't know if that matters. 

Yeah, probably rather rare. This just sounded like a less intrusive change which
would only address the problem that no devid is no option when starting the dm.

 
 Oh, just while talking about setdefault. Jim, this is one of the odd things 
 when
 moving from xm to xl stack from libvirt: libvirt defaults to the netfront NIC
 when no model is specified and sets the type. The libxl setdefault function 
 sets
 the model to rtl8139 but leaves the type untouched.
 
 This sounds like a bug in libxl to me -- it should do something
 consistent I think.
 
  So setting no model in the
 xml config creates a domain with no emulated NIC (this does not matter after
 Linux is up because the emulated devices get unplugged). Just that PXE boot 
 will
 not work. This gets odd because with the old xen (xm) driver, no model meant
 rtl8139.

 Sigh, and to hijack this thread even further I noticed a quite unexpected
 behaviour when starting a domain trhough libvirt and then try to use xl list
 -l to get config details. xl list shows all running domains but xl list 
 -l
 produces something like you have to specify a domain name. I found the 
 origin
 of this to be libxl_userdata_retrieve which takes a userdate_userid as an
 argument. Libvirt uses libvirt-xml for that, while xl uses xl. This 
 might be
 intentional and the bug is just that we need a better check for not finding 
 the
 userdata and then skipping those domains. On the other hand ... its after 
 all in
 both cases a domain created and started through libxl...
 
 I think this was discussed a few weeks ago on the list, and there were
 one or two separate bugs and short comings. I'm not sure which subset
 were actually fixed.
 
 One issue is that xl stored the guest config and then retrieves it for
 use in xl list -l, but libvirt != xl and therefore has no config file to
 save.

Right, that kind of was what I tried to say in many words. :) Oh, hm probably
with the addition that xl would save configs with one key and libvirt with
another. So relying on that at least results in xl not being able to show
details of libvirt created domains (and the other way round).
But as you said, I seem to have missed some discussion about it (which I saw Jim
mentioning a link but have not followed, yet, this morning.

 
 The solution is probably for the list stuff to be based on dynamically
 gathering the domain info, instead of reparsing the config.

Yes, that sounds good. It would feel like a more intuitive behavior to me (not
that I think I could call myself an expert on intuitivity).


 
 Ian.,
 
 




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

Re: [libvirt] [Xen-devel] Setting devid for emulated NICs (Xen 4.3.1 / libvirt 1.2.0) using libxl driver

2013-12-19 Thread Stefan Bader

On 19.12.2013 11:19, Ian Campbell wrote:
On Wed, 2013-12-18 at 17:44 -0700, Jim Fehlig wrote:
Stefan Bader wrote:
On 18.12.2013 14:28, Ian Campbell wrote:

On Wed, 2013-12-18 at 14:12 +0100, Stefan Bader wrote:

On 18.12.2013 13:27, Ian Campbell wrote:

On Tue, 2013-12-17 at 18:32 +0100, Stefan Bader wrote:

Might this libxl fix be relevant:
commit 5420f26507fc5c9853eb1076401a8658d72669da
Author: Jim Fehlig jfeh...@suse.com
Date: Fri Jan 11 12:22:26 2013 +

libxl: Set vfb and vkb devid if not done so by the caller

Other devices set a sensible devid if the caller has not
done so.
Do the same for vfb and vkb. While at it, factor out the
common code
used to determine a sensible devid, so it can be used by
other
libxl__device_*_add functions.

Signed-off-by: Jim Fehlig jfeh...@suse.com
Acked-by: Ian Campbell ian.campb...@citrix.com
Committed-by: Ian Campbell ian.campb...@citrix.com

and a follow up in dfeccbeaa. Although the comment implies that nic's
were already correctly assigning a devid if the caller specified -1, so
I don't know why it doesn't work for you :-(

Ok, yes, the commit above indeed changes libxl__device_nic_add to call
libxl__device_nextid for the devid... Just how is this actually called.
Maybe not sufficient but git grep libxl__device_nic_add in the xen
code only
shows the definition and a declaration in libxl_internal.h to me...

I have a feeling a macro might be involved...

Here we go, look for DEFINE_DEVICE_REMOVE in libxl.c. We should really
add the eventual function names in comments to provide grep fodder

Oh duh, yeah. So in DEFINE_DEVICE_ADD a libxl_device_nic_add is created
which
calls to libxl__device_nic_add. When I look for the single _ version I
find a
call from xl_cmdimpl.c and its public declaration in libxl.h.
So I guess the bug is that libvirt in the libxl driver never seems to do
so

The macro creates libxl__add_nics which adds the nics from the
libxl_domain_config-nics array. I don't think libvirt needs to call
libxl_device_nic_add manually unless it is hotplugging a new nic at
runtime.

Hm, so I think this is the path:

libxl_domain_create_new
- do_domain_create
- initiate_domain_create
- libxl__bootloader_run (HVM domain, skipping bootloader)
- domcreate_bootloader_done
- domcreate_rebuild_done
- domcreate_launch_dm
- libxl__spawn_local_dm
- domcreate_devmodel_started

In libxl__spawn_local_dm, there is the following loop:

for (i = 0; i d_config-num_nics; i++) {
/* We have to init the nic here, because we still haven't
* called libxl_device_nic_add at this point, but qemu needs
* the nic information to be complete.
*/
ret = libxl__device_nic_setdefault(gc, d_config-nics[i], domid);
if (ret)
goto error_out;
}

So I think when starting the dm, the devid just is not set as setdefault
does
not seem to do so. I would be done in the later domcreate_devmodel_started
callback but that is too late for the generated qemu arguments.

Sorry for jumping in late...

I stumbled across this problem just before openSUSE13.1 released and did
a quick fix in libvirt

https://build.opensuse.org/package/view_file/Virtualization:openSUSE13.1/libvirt/libxl-hvm-nic.patch?expand=1

I removed setting the NIC devid in the libxl driver a while back to be
consistent with other devices

http://libvirt.org/git/?p=libvirt.git;a=commit;h=ba64b97134a6129a48684f22f31be92c3b6eef96

The quick fix was to essentially revert the above commit until I could
investigate further. Thank you for now having done that investigation
:). Can the devid assignment logic be moved from
libxl__device_nic_add() to libxl__device_nic_setdefault()?

It certainly seems like it would be more natural to do it there.

I suspect it might be done this way because at setdefault time you might
be walking a list of nics none of which have been created yet -- so
looking in xenstore would return devid zero is free for every one of
them?

How about we:
* move the init to setdefault to catch the single NIC added via
hotplug case

Init of devid? Hm, would that work as I am not sure there is a simple way of
differentiating between a NIC config for a single hotplug and one that is part
of a create-time array...

* we add somewhere early in the domain create path a call to a
function which assigns devids to an entire array of devices (and
do it for all the different device types). Perhaps in
initiate_domain_create() after the calls to
libxl__domain_create_info_setdefault

Re: [libvirt] [Xen-devel] Setting devid for emulated NICs (Xen 4.3.1 / libvirt 1.2.0) using libxl driver

2013-12-18 Thread Stefan Bader

On 18.12.2013 13:27, Ian Campbell wrote:
 On Tue, 2013-12-17 at 18:32 +0100, Stefan Bader wrote:

 Might this libxl fix be relevant:
 commit 5420f26507fc5c9853eb1076401a8658d72669da
 Author: Jim Fehlig jfeh...@suse.com
 Date:   Fri Jan 11 12:22:26 2013 +
 
 libxl: Set vfb and vkb devid if not done so by the caller
 
 Other devices set a sensible devid if the caller has not done 
 so.
 Do the same for vfb and vkb.  While at it, factor out the 
 common code
 used to determine a sensible devid, so it can be used by other
 libxl__device_*_add functions.
 
 Signed-off-by: Jim Fehlig jfeh...@suse.com
 Acked-by: Ian Campbell ian.campb...@citrix.com
 Committed-by: Ian Campbell ian.campb...@citrix.com
 
 and a follow up in dfeccbeaa. Although the comment implies that nic's
 were already correctly assigning a devid if the caller specified -1, so
 I don't know why it doesn't work for you :-(

 Ok, yes, the commit above indeed changes libxl__device_nic_add to call
 libxl__device_nextid for the devid... Just how is this actually called.
 Maybe not sufficient but git grep libxl__device_nic_add in the xen code 
 only
 shows the definition and a declaration in libxl_internal.h to me...
 
 I have a feeling a macro might be involved...
 
 Here we go, look for DEFINE_DEVICE_REMOVE in libxl.c. We should really
 add the eventual function names in comments to provide grep fodder

Oh duh, yeah. So in DEFINE_DEVICE_ADD a libxl_device_nic_add is created which
calls to libxl__device_nic_add. When I look for the single _ version I find a
call from xl_cmdimpl.c and its public declaration in libxl.h.
So I guess the bug is that libvirt in the libxl driver never seems to do so. Ok,
thanks a lot for digging the out the DEFINE. As nice those are to create similar
functions from template, grep and tags fail to be helpful with them.

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




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

Re: [libvirt] [Xen-devel] Setting devid for emulated NICs (Xen 4.3.1 / libvirt 1.2.0) using libxl driver

2013-12-18 Thread Stefan Bader

On 18.12.2013 14:28, Ian Campbell wrote:
 On Wed, 2013-12-18 at 14:12 +0100, Stefan Bader wrote:
 On 18.12.2013 13:27, Ian Campbell wrote:
 On Tue, 2013-12-17 at 18:32 +0100, Stefan Bader wrote:

 Might this libxl fix be relevant:
 commit 5420f26507fc5c9853eb1076401a8658d72669da
 Author: Jim Fehlig jfeh...@suse.com
 Date:   Fri Jan 11 12:22:26 2013 +
 
 libxl: Set vfb and vkb devid if not done so by the caller
 
 Other devices set a sensible devid if the caller has not done 
 so.
 Do the same for vfb and vkb.  While at it, factor out the 
 common code
 used to determine a sensible devid, so it can be used by other
 libxl__device_*_add functions.
 
 Signed-off-by: Jim Fehlig jfeh...@suse.com
 Acked-by: Ian Campbell ian.campb...@citrix.com
 Committed-by: Ian Campbell ian.campb...@citrix.com
 
 and a follow up in dfeccbeaa. Although the comment implies that nic's
 were already correctly assigning a devid if the caller specified -1, so
 I don't know why it doesn't work for you :-(

 Ok, yes, the commit above indeed changes libxl__device_nic_add to call
 libxl__device_nextid for the devid... Just how is this actually called.
 Maybe not sufficient but git grep libxl__device_nic_add in the xen code 
 only
 shows the definition and a declaration in libxl_internal.h to me...

 I have a feeling a macro might be involved...

 Here we go, look for DEFINE_DEVICE_REMOVE in libxl.c. We should really
 add the eventual function names in comments to provide grep fodder

 Oh duh, yeah. So in DEFINE_DEVICE_ADD a libxl_device_nic_add is created which
 calls to libxl__device_nic_add. When I look for the single _ version I find a
 call from xl_cmdimpl.c and its public declaration in libxl.h.
 So I guess the bug is that libvirt in the libxl driver never seems to do so
 
 The macro creates libxl__add_nics which adds the nics from the
 libxl_domain_config-nics array. I don't think libvirt needs to call
 libxl_device_nic_add manually unless it is hotplugging a new nic at
 runtime.

Bah, this is another one, DEFINE_DEVICES_ADD in libxl_devices.c, that does that.
That seems to be used from a few places. That needs more research to understand
the call paths. Probably simpler starting from the entry which looks to be
libxl_domain_create_new from libvirt's side.

 
 . Ok,
 thanks a lot for digging the out the DEFINE. As nice those are to create 
 similar
 functions from template, grep and tags fail to be helpful with them.

 -Stefan

 Ian.

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



 
 




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

Re: [libvirt] [Xen-devel] Setting devid for emulated NICs (Xen 4.3.1 / libvirt 1.2.0) using libxl driver

2013-12-18 Thread Stefan Bader

On 18.12.2013 14:28, Ian Campbell wrote:
 On Wed, 2013-12-18 at 14:12 +0100, Stefan Bader wrote:
 On 18.12.2013 13:27, Ian Campbell wrote:
 On Tue, 2013-12-17 at 18:32 +0100, Stefan Bader wrote:

 Might this libxl fix be relevant:
 commit 5420f26507fc5c9853eb1076401a8658d72669da
 Author: Jim Fehlig jfeh...@suse.com
 Date:   Fri Jan 11 12:22:26 2013 +
 
 libxl: Set vfb and vkb devid if not done so by the caller
 
 Other devices set a sensible devid if the caller has not done 
 so.
 Do the same for vfb and vkb.  While at it, factor out the 
 common code
 used to determine a sensible devid, so it can be used by other
 libxl__device_*_add functions.
 
 Signed-off-by: Jim Fehlig jfeh...@suse.com
 Acked-by: Ian Campbell ian.campb...@citrix.com
 Committed-by: Ian Campbell ian.campb...@citrix.com
 
 and a follow up in dfeccbeaa. Although the comment implies that nic's
 were already correctly assigning a devid if the caller specified -1, so
 I don't know why it doesn't work for you :-(

 Ok, yes, the commit above indeed changes libxl__device_nic_add to call
 libxl__device_nextid for the devid... Just how is this actually called.
 Maybe not sufficient but git grep libxl__device_nic_add in the xen code 
 only
 shows the definition and a declaration in libxl_internal.h to me...

 I have a feeling a macro might be involved...

 Here we go, look for DEFINE_DEVICE_REMOVE in libxl.c. We should really
 add the eventual function names in comments to provide grep fodder

 Oh duh, yeah. So in DEFINE_DEVICE_ADD a libxl_device_nic_add is created which
 calls to libxl__device_nic_add. When I look for the single _ version I find a
 call from xl_cmdimpl.c and its public declaration in libxl.h.
 So I guess the bug is that libvirt in the libxl driver never seems to do so
 
 The macro creates libxl__add_nics which adds the nics from the
 libxl_domain_config-nics array. I don't think libvirt needs to call
 libxl_device_nic_add manually unless it is hotplugging a new nic at
 runtime.
 

Hm, so I think this is the path:

libxl_domain_create_new
- do_domain_create
   - initiate_domain_create
  - libxl__bootloader_run (HVM domain, skipping bootloader)
  - domcreate_bootloader_done
 - domcreate_rebuild_done
- domcreate_launch_dm
   - libxl__spawn_local_dm
 - domcreate_devmodel_started

In libxl__spawn_local_dm, there is the following loop:

for (i = 0; i  d_config-num_nics; i++) {
/* We have to init the nic here, because we still haven't
 * called libxl_device_nic_add at this point, but qemu needs
 * the nic information to be complete.
 */
ret = libxl__device_nic_setdefault(gc, d_config-nics[i], domid);
if (ret)
goto error_out;
}

So I think when starting the dm, the devid just is not set as setdefault does
not seem to do so. I would be done in the later domcreate_devmodel_started
callback but that is too late for the generated qemu arguments.

-Stefan



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

[libvirt] Setting devid for emulated NICs (Xen 4.3.1 / libvirt 1.2.0) using libxl driver

2013-12-17 Thread Stefan Bader


Using virt-manager hypervisor default type:

interface type='bridge'
  mac address='00:16:3e:5e:09:9d'/
  source bridge='br0'/
  script path='vif-bridge'/
/interface

This causes the qemu call to have -net none which removes PXE boot
abilities. A linux kernel has network through the xen pv-driver.

Changing in virt-manager to e1000 type:

interface type='bridge'
  mac address='00:16:3e:5e:09:9d'/
  source bridge='br0'/
  script path='vif-bridge'/
  model type='e1000'/
/interface

This currently (Xen 4.3.1 + libvirt 1.2.0) fails and the qemu arguments
in the log look suspicious:
  -device e1000,id=nic-1,netdev=net-1,mac=00:16:3e:5e:09:9d
  -netdev type=tap,id=net-1,ifname=vif1.-1-emu,script=no,downscript=no

Looking through git I found ba64b97134a6129a48684f22f31be92c3b6eef96
  libxl: Allow libxl to set NIC devid

which removes a line that sets the devid of new NICs. I assume the comment
says (sorry I always seem to get confused reading it) that devid should
be auto-set by the libxl library provided by Xen. But I could not find
where that would be done. Even the xl command implementation sets it
explicitly. libxl_device_nic_init sets it to -1 which would explain the
wrong qemu arguments above.
For testing I re-added the following after the libxlMakeNic call:

--- libvirt-1.2.0.orig/src/libxl/libxl_conf.c   2013-12-11 17:04:17.0 +0
+++ libvirt-1.2.0/src/libxl/libxl_conf.c2013-12-16 19:08:46.830016646 +0
@@ -907,6 +907,8 @@ libxlMakeNicList(virDomainDefPtr def,  l
 for (i = 0; i  nnics; i++) {
 if (libxlMakeNic(l_nics[i], x_nics[i]))
 goto error;
+   if (x_nics[i].devid  0)
+   x_nics[i].devid = i;
 }

 d_config-nics = x_nics;

And with that I get a working PXE boot when I set the device type
in the config. (Side note that I think not using a type defaults to
rtl8139 in the old xend driver. Maybe this should be the same for the
xl driver).
I am not sure how the right fix for it should look like as I am unsure
which part was supposed to set the devid. Just right now it does not
seem to be done.

-Stefan



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

Re: [libvirt] [Xen-devel] Setting devid for emulated NICs (Xen 4.3.1 / libvirt 1.2.0) using libxl driver

2013-12-17 Thread Stefan Bader

On 17.12.2013 17:58, Ian Campbell wrote:
 On Tue, 2013-12-17 at 17:34 +0100, Stefan Bader wrote:
 Using virt-manager hypervisor default type:

 interface type='bridge'
   mac address='00:16:3e:5e:09:9d'/
   source bridge='br0'/
   script path='vif-bridge'/
 /interface

 This causes the qemu call to have -net none which removes PXE boot
 abilities. A linux kernel has network through the xen pv-driver.

 Changing in virt-manager to e1000 type:

 interface type='bridge'
   mac address='00:16:3e:5e:09:9d'/
   source bridge='br0'/
   script path='vif-bridge'/
   model type='e1000'/
 /interface

 This currently (Xen 4.3.1 + libvirt 1.2.0) fails and the qemu arguments
 in the log look suspicious:
   -device e1000,id=nic-1,netdev=net-1,mac=00:16:3e:5e:09:9d
   -netdev type=tap,id=net-1,ifname=vif1.-1-emu,script=no,downscript=no

 Looking through git I found ba64b97134a6129a48684f22f31be92c3b6eef96
   libxl: Allow libxl to set NIC devid
 
 Is that a libvirt commit? Not seeing it in xen.git.

Oh, sorry, yes it is.
 
 Might this libxl fix be relevant:
 commit 5420f26507fc5c9853eb1076401a8658d72669da
 Author: Jim Fehlig jfeh...@suse.com
 Date:   Fri Jan 11 12:22:26 2013 +
 
 libxl: Set vfb and vkb devid if not done so by the caller
 
 Other devices set a sensible devid if the caller has not done so.
 Do the same for vfb and vkb.  While at it, factor out the common 
 code
 used to determine a sensible devid, so it can be used by other
 libxl__device_*_add functions.
 
 Signed-off-by: Jim Fehlig jfeh...@suse.com
 Acked-by: Ian Campbell ian.campb...@citrix.com
 Committed-by: Ian Campbell ian.campb...@citrix.com
 
 and a follow up in dfeccbeaa. Although the comment implies that nic's
 were already correctly assigning a devid if the caller specified -1, so
 I don't know why it doesn't work for you :-(

Ok, yes, the commit above indeed changes libxl__device_nic_add to call
libxl__device_nextid for the devid... Just how is this actually called.
Maybe not sufficient but git grep libxl__device_nic_add in the xen code only
shows the definition and a declaration in libxl_internal.h to me...


 

 which removes a line that sets the devid of new NICs. I assume the comment
 says (sorry I always seem to get confused reading it) that devid should
 be auto-set by the libxl library provided by Xen.
 
 Correct.
 
 But I could not find where that would be done.
 
 I expect the above commits have pointed you to the right line, but the
 code should be quite near the top of libxl__device_nic_add in libxl.c
 
  Even the xl command implementation sets it
 explicitly.
 
 The caller is also allowed to do this if it cares what number is used.
 
  libxl_device_nic_init sets it to -1 which would explain the
 wrong qemu arguments above.
 
 Right, libxl_device_nic_init sets everything to the defaults please
 and __device_nic_add will instantiate any defaults which the application
 didn't supply.
 
 For testing I re-added the following after the libxlMakeNic call:

 --- libvirt-1.2.0.orig/src/libxl/libxl_conf.c   2013-12-11 
 17:04:17.0 +0
 +++ libvirt-1.2.0/src/libxl/libxl_conf.c2013-12-16 
 19:08:46.830016646 +0
 @@ -907,6 +907,8 @@ libxlMakeNicList(virDomainDefPtr def,  l
  for (i = 0; i  nnics; i++) {
  if (libxlMakeNic(l_nics[i], x_nics[i]))
  goto error;
 +   if (x_nics[i].devid  0)
 +   x_nics[i].devid = i;
  }

  d_config-nics = x_nics;

 And with that I get a working PXE boot when I set the device type
 in the config. (Side note that I think not using a type defaults to
 rtl8139 in the old xend driver. Maybe this should be the same for the
 xl driver).
 I am not sure how the right fix for it should look like as I am unsure
 which part was supposed to set the devid. Just right now it does not
 seem to be done.

 -Stefan

 
 
 
 ___
 Xen-devel mailing list
 xen-de...@lists.xen.org
 http://lists.xen.org/xen-devel
 




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

Re: [libvirt] [PATCH] xen: Use internal interfaces in xenDomainUsedCpus

2013-08-06 Thread Stefan Bader

On 05.08.2013 19:52, Jim Fehlig wrote:
 
 libvirt typically uses a '*Internal' naming pattern for these types of
 internal functions, e.g. xenUnifiedDomainGetVcpusFlagsInternal.  Also as
 we touch this code we should strive to use the libvirt pattern of
 putting each parameter after the first on a new line when the list of
 parameters exceeds 80 columns.  Finally, since you added a line after
 the xenUnifiedNodeGetInfo declaration, we should add one here too.
 
Ok, changed.

 
 I don't think this comment is necessary.  Instead, just send a follow-up
 patch :).

Oh well, it was a kind of reminder, but beside of the doing it correct part,
the current usage is ok as there is no special checking between public and
private usage which could lock up... :)

 @@ -1501,7 +1533,7 @@ xenUnifiedDomainGetXMLDesc(virDomainPtr dom, unsigned 
 int flags)
  } else {
  char *cpus;
  xenUnifiedLock(priv);
 -cpus = xenDomainUsedCpus(dom);
 +cpus = xenDomainUsedCpus(dom, def);
   
 
 This should be minidef right?  Otherwise def is NULL, resulting in a
 segfault further down the call chain.


Absolutely right. I missed to do that in the version I forward ported to HEAD
since I did the fix and testing in an older version. :/
Good you spotted that.

Ok, I updated the patch as suggested (attached).

-Stefan

From 47ce666f6a4d91832883341c56f0a55181698e76 Mon Sep 17 00:00:00 2001
From: Stefan Bader stefan.ba...@canonical.com
Date: Mon, 15 Jul 2013 16:03:58 +0200
Subject: [PATCH] xen: Use internal interfaces in xenDomainUsedCpus

Since commit 95e18efd most public interfaces (xenUnified...) obtain
a virDomainDefPtr via xenGetDomainDefFor...() which take the unified
lock.
This is already taken before calling xenDomainUsedCpus(), so we get
a deadlock for active guests. Avoid this by splitting up
xenUnifiedDomainGetVcpusFlags() and xenUnifiedDomainGetVcpus() into
public and private function calls (which get the virDomainDefPtr passed)
and use those in xenDomainUsedCpus().

xenDomainUsedCpus
  ...
  nb_vcpu = xenUnifiedDomainGetMaxVcpus(dom);
return xenUnifiedDomainGetVcpusFlags(...)
  ...
  if (!(def = xenGetDomainDefForDom(dom)))
return xenGetDomainDefForUUID(dom-conn, dom-uuid);
  ...
  ret = xenHypervisorLookupDomainByUUID(conn, uuid);
...
xenUnifiedLock(priv);
name = xenStoreDomainGetName(conn, id);
xenUnifiedUnlock(priv);
  ...
  if ((ncpus = xenUnifiedDomainGetVcpus(dom, cpuinfo, nb_vcpu,
...
if (!(def = xenGetDomainDefForDom(dom)))
  [again like above]

Signed-off-by: Stefan Bader stefan.ba...@canonical.com
---
 src/xen/xen_driver.c | 100 +++
 src/xen/xen_driver.h |   2 +-
 2 files changed, 70 insertions(+), 32 deletions(-)

diff --git a/src/xen/xen_driver.c b/src/xen/xen_driver.c
index 4ae38d3..e1fcbcc 100644
--- a/src/xen/xen_driver.c
+++ b/src/xen/xen_driver.c
@@ -73,12 +73,18 @@
 
 static int
 xenUnifiedNodeGetInfo(virConnectPtr conn, virNodeInfoPtr info);
+
 static int
-xenUnifiedDomainGetMaxVcpus(virDomainPtr dom);
+xenUnifiedDomainGetVcpusFlagsInternal(virDomainPtr dom,
+  virDomainDefPtr def,
+  unsigned int flags);
 static int
-xenUnifiedDomainGetVcpus(virDomainPtr dom,
- virVcpuInfoPtr info, int maxinfo,
- unsigned char *cpumaps, int maplen);
+xenUnifiedDomainGetVcpusInternal(virDomainPtr dom,
+ virDomainDefPtr def,
+ virVcpuInfoPtr info,
+ int maxinfo,
+ unsigned char *cpumaps,
+ int maplen);
 
 
 static bool is_privileged = false;
@@ -173,6 +179,7 @@ xenNumaInit(virConnectPtr conn) {
 /**
  * xenDomainUsedCpus:
  * @dom: the domain
+ * @def: the domain definition
  *
  * Analyze which set of CPUs are used by the domain and
  * return a string providing the ranges.
@@ -181,7 +188,7 @@ xenNumaInit(virConnectPtr conn) {
  * NULL if the domain uses all CPU or in case of error.
  */
 char *
-xenDomainUsedCpus(virDomainPtr dom)
+xenDomainUsedCpus(virDomainPtr dom, virDomainDefPtr def)
 {
 char *res = NULL;
 int ncpus;
@@ -202,7 +209,9 @@ xenDomainUsedCpus(virDomainPtr dom)
 
 if (priv-nbNodeCpus = 0)
 return NULL;
-nb_vcpu = xenUnifiedDomainGetMaxVcpus(dom);
+nb_vcpu = xenUnifiedDomainGetVcpusFlagsInternal(dom, def,
+(VIR_DOMAIN_VCPU_LIVE |
+ VIR_DOMAIN_VCPU_MAXIMUM));
 if (nb_vcpu = 0)
 return NULL;
 if (xenUnifiedNodeGetInfo(dom-conn, nodeinfo)  0)
@@ -217,8 +226,8 @@ xenDomainUsedCpus(virDomainPtr dom)
 VIR_ALLOC_N(cpumap, nb_vcpu

[libvirt] [PATCH] xen: Avoid double free of virDomainDef in xenDaemonCreateXML

2013-07-31 Thread Stefan Bader

Beside dumpxml still being broken with the xen driver (hint [1] ;) )
there is also a quite annoying problem of creating new definitions
now failing. Using virt-manager this even disrupts the connection
to the host and the guest is not created persistent although
it does come up.

-Stefan

[1] https://www.redhat.com/archives/libvir-list/2013-July/msg01183.html

From 0e90fac9004996a6517ce1bd4d7b9c6ebef6c45c Mon Sep 17 00:00:00 2001
From: Stefan Bader stefan.ba...@canonical.com
Date: Tue, 30 Jul 2013 20:48:33 +0200
Subject: [PATCH] xen: Avoid double free of virDomainDef in xenDaemonCreateXML

The virDomainDef is allocated by the caller and also used after
calling to xenDaemonCreateXML. So it must not get freed by the
callee.

Signed-off-by: Stefan Bader stefan.ba...@canonical.com
---
 src/xen/xend_internal.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c
index 9d61fca..1ce36e6 100644
--- a/src/xen/xend_internal.c
+++ b/src/xen/xend_internal.c
@@ -2171,7 +2171,6 @@ xenDaemonCreateXML(virConnectPtr conn, virDomainDefPtr 
def)
 if (xenDaemonDomainResume(conn, def)  0)
 goto error;
 
-virDomainDefFree(def);
 return 0;
 
   error:
-- 
1.8.1.2

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

Re: [libvirt] [Xen-devel] [PATCH] libxl: Correctly initialize vcpu bitmap

2013-07-24 Thread Stefan Bader

On 23.07.2013 23:20, Jim Fehlig wrote:
 One comment below in addition to Konrad's...
 
 Konrad Rzeszutek Wilk wrote:
 On Mon, Jul 22, 2013 at 12:51:05PM +0200, Stefan Bader wrote:
   
 This fixes the basic setup but there is likely more to do if things
 like manual CPU hirarchy (nodes, cores, threads) to be working.

 Cross-posting to xen-devel to make sure I am doing things correctly.

 -Stefan


 From 1ec5e7ea0d3498b9f61b83e8aed87cc3cae106de Mon Sep 17 00:00:00 2001
 From: Stefan Bader stefan.ba...@canonical.com
 Date: Fri, 19 Jul 2013 15:20:00 +0200
 Subject: [PATCH] libxl: Correctly initialize vcpu bitmap

 The avai_vcpu bitmap has to be allocated before it can be used (using
 

 avail_vcpu ?

   
 the maximum allowed value for that). Then for each available VCPU the
 bit in the mask has to be set (libxl_bitmap_set takes a bit position
 as an argument, not the number of bits to set).

 Without this, I would always only get one VCPU for guests created
 through libvirt/libxl.

 Signed-off-by: Stefan Bader stefan.ba...@canonical.com
 

 The libxl calling logic looks Ok to me. So from the libxl perspective
 you can tack on Reviewed-by: Konrad Rzeszutek Wilk konrad.w...@oracle.com

   
 ---
  src/libxl/libxl_conf.c |   14 +++---
  1 file changed, 11 insertions(+), 3 deletions(-)

 diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
 index 4a0fba9..7592dd2 100644
 --- a/src/libxl/libxl_conf.c
 +++ b/src/libxl/libxl_conf.c
 @@ -331,7 +331,8 @@ error:
  }
  
  static int
 -libxlMakeDomBuildInfo(virDomainDefPtr def, libxl_domain_config *d_config)
 +libxlMakeDomBuildInfo(libxlDriverPrivatePtr driver, virDomainDefPtr def,
 +  libxl_domain_config *d_config)
  {
  libxl_domain_build_info *b_info = d_config-b_info;
  int hvm = STREQ(def-os.type, hvm);
 @@ -343,8 +344,15 @@ libxlMakeDomBuildInfo(virDomainDefPtr def, 
 libxl_domain_config *d_config)
  libxl_domain_build_info_init_type(b_info, LIBXL_DOMAIN_TYPE_HVM);
  else
  libxl_domain_build_info_init_type(b_info, LIBXL_DOMAIN_TYPE_PV);
 +
  b_info-max_vcpus = def-maxvcpus;
 -libxl_bitmap_set((b_info-avail_vcpus), def-vcpus);
 +if (libxl_cpu_bitmap_alloc(driver-ctx, b_info-avail_vcpus,
 +   def-maxvcpus))
 
 
 You are using the driver-wide libxl_ctx in libxlDriverPrivate here, but
 should be using the per-domain libxl_ctx in libxlDomainObjPrivate
 structure IMO.
 
 It looks like libxlBuildDomainConfig, which calls libxlMakeDomBuildInfo,
 should have just taken virDomainObjPtr instead of virDomainDefPtr. 
 libxlBuildDomainConfig would then have access to the per-domain
 libxl_ctx, and no longer need the libxlDriverPrivatePtr parameter as well.
 

So more like attached v2. I am not sure libxlDriverPrivatePtr can really be
dropped (and maybe should not as part of a fix to this issue). Maybe calling
libxl_flask_context_to_sid also should use the per domain context. But at least
libxlMakeVfbList-libxlMakeVfb-virPortAllocatorAcquire sounds a bit like it
might need the driver context.

-Stefan

From 2d4b7e5ae2644e6f7a2ea930d0899ea426cb9c84 Mon Sep 17 00:00:00 2001
From: Stefan Bader stefan.ba...@canonical.com
Date: Fri, 19 Jul 2013 15:20:00 +0200
Subject: [PATCH] libxl: Correctly initialize vcpu bitmap

The avail_vcpu bitmap has to be allocated before it can be used (using
the maximum allowed value for that). Then for each available VCPU the
bit in the mask has to be set (libxl_bitmap_set takes a bit position
as an argument, not the number of bits to set).

Without this, I would always only get one VCPU for guests created
through libvirt/libxl.

[v2] Use private ctx from virDomainObjPtr-privateData

Signed-off-by: Stefan Bader stefan.ba...@canonical.com
---
 src/libxl/libxl_conf.c   |   19 +++
 src/libxl/libxl_conf.h   |2 +-
 src/libxl/libxl_driver.c |2 +-
 3 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index 4a0fba9..f732135 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -331,8 +331,10 @@ error:
 }
 
 static int
-libxlMakeDomBuildInfo(virDomainDefPtr def, libxl_domain_config *d_config)
+libxlMakeDomBuildInfo(virDomainObjPtr vm, libxl_domain_config *d_config)
 {
+virDomainDefPtr def = vm-def;
+libxlDomainObjPrivatePtr priv = vm-privateData;
 libxl_domain_build_info *b_info = d_config-b_info;
 int hvm = STREQ(def-os.type, hvm);
 size_t i;
@@ -343,8 +345,15 @@ libxlMakeDomBuildInfo(virDomainDefPtr def, libxl_domain_config *d_config)
 libxl_domain_build_info_init_type(b_info, LIBXL_DOMAIN_TYPE_HVM);
 else
 libxl_domain_build_info_init_type(b_info, LIBXL_DOMAIN_TYPE_PV);
+
 b_info-max_vcpus = def-maxvcpus;
-libxl_bitmap_set((b_info-avail_vcpus), def-vcpus);
+if (libxl_cpu_bitmap_alloc(priv-ctx, b_info-avail_vcpus,
+   def-maxvcpus))
+goto error

Re: [libvirt] [Xen-devel] [PATCH] libxl: Correctly initialize vcpu bitmap

2013-07-23 Thread Stefan Bader

On 22.07.2013 21:39, Konrad Rzeszutek Wilk wrote:
 On Mon, Jul 22, 2013 at 12:51:05PM +0200, Stefan Bader wrote:
 This fixes the basic setup but there is likely more to do if things
 like manual CPU hirarchy (nodes, cores, threads) to be working.

 Cross-posting to xen-devel to make sure I am doing things correctly.

 -Stefan


 From 1ec5e7ea0d3498b9f61b83e8aed87cc3cae106de Mon Sep 17 00:00:00 2001
 From: Stefan Bader stefan.ba...@canonical.com
 Date: Fri, 19 Jul 2013 15:20:00 +0200
 Subject: [PATCH] libxl: Correctly initialize vcpu bitmap

 The avai_vcpu bitmap has to be allocated before it can be used (using
 
 avail_vcpu ?

*sigh* Yeah, of course I cannot spell. :-P

 
 the maximum allowed value for that). Then for each available VCPU the
 bit in the mask has to be set (libxl_bitmap_set takes a bit position
 as an argument, not the number of bits to set).

 Without this, I would always only get one VCPU for guests created
 through libvirt/libxl.

 Signed-off-by: Stefan Bader stefan.ba...@canonical.com
 
 The libxl calling logic looks Ok to me. So from the libxl perspective
 you can tack on Reviewed-by: Konrad Rzeszutek Wilk konrad.w...@oracle.com

Thanks, I will leave that (as well as the complimentary fixing of missing
lletters to the libvirt maintainers. :)

-Stefan

 
 ---
  src/libxl/libxl_conf.c |   14 +++---
  1 file changed, 11 insertions(+), 3 deletions(-)

 diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
 index 4a0fba9..7592dd2 100644
 --- a/src/libxl/libxl_conf.c
 +++ b/src/libxl/libxl_conf.c
 @@ -331,7 +331,8 @@ error:
  }
  
  static int
 -libxlMakeDomBuildInfo(virDomainDefPtr def, libxl_domain_config *d_config)
 +libxlMakeDomBuildInfo(libxlDriverPrivatePtr driver, virDomainDefPtr def,
 +  libxl_domain_config *d_config)
  {
  libxl_domain_build_info *b_info = d_config-b_info;
  int hvm = STREQ(def-os.type, hvm);
 @@ -343,8 +344,15 @@ libxlMakeDomBuildInfo(virDomainDefPtr def, 
 libxl_domain_config *d_config)
  libxl_domain_build_info_init_type(b_info, LIBXL_DOMAIN_TYPE_HVM);
  else
  libxl_domain_build_info_init_type(b_info, LIBXL_DOMAIN_TYPE_PV);
 +
  b_info-max_vcpus = def-maxvcpus;
 -libxl_bitmap_set((b_info-avail_vcpus), def-vcpus);
 +if (libxl_cpu_bitmap_alloc(driver-ctx, b_info-avail_vcpus,
 +   def-maxvcpus))
 +goto error;
 +libxl_bitmap_set_none(b_info-avail_vcpus);
 +for (i = 0; i  def-vcpus; i++)
 +libxl_bitmap_set((b_info-avail_vcpus), i);
 +
  if (def-clock.ntimers  0 
  def-clock.timers[0]-name == VIR_DOMAIN_TIMER_NAME_TSC) {
  switch (def-clock.timers[0]-mode) {
 @@ -802,7 +810,7 @@ libxlBuildDomainConfig(libxlDriverPrivatePtr driver,
  if (libxlMakeDomCreateInfo(driver, def, d_config-c_info)  0)
  return -1;
  
 -if (libxlMakeDomBuildInfo(def, d_config)  0) {
 +if (libxlMakeDomBuildInfo(driver, def, d_config)  0) {
  return -1;
  }
  
 -- 
 1.7.9.5


 ___
 Xen-devel mailing list
 xen-de...@lists.xen.org
 http://lists.xen.org/xen-devel




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

[libvirt] [PATCH] libxl: Correctly initialize vcpu bitmap

2013-07-22 Thread Stefan Bader

This fixes the basic setup but there is likely more to do if things
like manual CPU hirarchy (nodes, cores, threads) to be working.

Cross-posting to xen-devel to make sure I am doing things correctly.

-Stefan


From 1ec5e7ea0d3498b9f61b83e8aed87cc3cae106de Mon Sep 17 00:00:00 2001
From: Stefan Bader stefan.ba...@canonical.com
Date: Fri, 19 Jul 2013 15:20:00 +0200
Subject: [PATCH] libxl: Correctly initialize vcpu bitmap

The avai_vcpu bitmap has to be allocated before it can be used (using
the maximum allowed value for that). Then for each available VCPU the
bit in the mask has to be set (libxl_bitmap_set takes a bit position
as an argument, not the number of bits to set).

Without this, I would always only get one VCPU for guests created
through libvirt/libxl.

Signed-off-by: Stefan Bader stefan.ba...@canonical.com
---
 src/libxl/libxl_conf.c |   14 +++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index 4a0fba9..7592dd2 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -331,7 +331,8 @@ error:
 }
 
 static int
-libxlMakeDomBuildInfo(virDomainDefPtr def, libxl_domain_config *d_config)
+libxlMakeDomBuildInfo(libxlDriverPrivatePtr driver, virDomainDefPtr def,
+  libxl_domain_config *d_config)
 {
 libxl_domain_build_info *b_info = d_config-b_info;
 int hvm = STREQ(def-os.type, hvm);
@@ -343,8 +344,15 @@ libxlMakeDomBuildInfo(virDomainDefPtr def, 
libxl_domain_config *d_config)
 libxl_domain_build_info_init_type(b_info, LIBXL_DOMAIN_TYPE_HVM);
 else
 libxl_domain_build_info_init_type(b_info, LIBXL_DOMAIN_TYPE_PV);
+
 b_info-max_vcpus = def-maxvcpus;
-libxl_bitmap_set((b_info-avail_vcpus), def-vcpus);
+if (libxl_cpu_bitmap_alloc(driver-ctx, b_info-avail_vcpus,
+   def-maxvcpus))
+goto error;
+libxl_bitmap_set_none(b_info-avail_vcpus);
+for (i = 0; i  def-vcpus; i++)
+libxl_bitmap_set((b_info-avail_vcpus), i);
+
 if (def-clock.ntimers  0 
 def-clock.timers[0]-name == VIR_DOMAIN_TIMER_NAME_TSC) {
 switch (def-clock.timers[0]-mode) {
@@ -802,7 +810,7 @@ libxlBuildDomainConfig(libxlDriverPrivatePtr driver,
 if (libxlMakeDomCreateInfo(driver, def, d_config-c_info)  0)
 return -1;
 
-if (libxlMakeDomBuildInfo(def, d_config)  0) {
+if (libxlMakeDomBuildInfo(driver, def, d_config)  0) {
 return -1;
 }
 
-- 
1.7.9.5

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

Re: [libvirt] xen: Add interface versions for Xen 4.3

2013-07-17 Thread Stefan Bader

On 17.07.2013 18:42, Jim Fehlig wrote:
 Stefan Bader wrote:
 I tried to follow previous changes. This worked for me using Xen-4.3 
 and the xm stack (I know rather deprecated but as long as one can get
 it working, still).
   
 
 Agreed, we should keep the legacy driver working with the latest Xen
 release until its dependencies (xm/xend) are completely removed.
 
 -Stefan

 ---

 From 4c36fb22f01689472f6e170a8df6e08fd8c27a42 Mon Sep 17 00:00:00 2001
 From: Stefan Bader stefan.ba...@canonical.com
 Date: Mon, 15 Jul 2013 19:25:23 +0200
 Subject: [PATCH] xen: Add interface versions for Xen 4.3

 Xen 4.3 changes sysctl version to 0xA and domctl version to 0x9. Update
 the hypervisor driver to work with those.

 Signed-off-by: Stefan Bader stefan.ba...@canonical.com
 ---
  src/xen/xen_hypervisor.c |  129 
 ++
  1 file changed, 97 insertions(+), 32 deletions(-)

 diff --git a/src/xen/xen_hypervisor.c b/src/xen/xen_hypervisor.c
 index 39e641e..ff92556 100644
 --- a/src/xen/xen_hypervisor.c
 +++ b/src/xen/xen_hypervisor.c
 @@ -271,6 +271,24 @@ struct xen_v2d8_getdomaininfo {
  };
  typedef struct xen_v2d8_getdomaininfo xen_v2d8_getdomaininfo;
  
 +struct xen_v2d9_getdomaininfo {
 +domid_t  domain;/* the domain number */
 +uint32_t flags; /* flags, see before */
 +uint64_t tot_pages ALIGN_64;/* total number of pages used */
 +uint64_t max_pages ALIGN_64;/* maximum number of pages allowed */
 +uint64_t outstanding_pages ALIGN_64;
 +uint64_t shr_pages ALIGN_64;/* number of shared pages */
 +uint64_t paged_pages ALIGN_64;/* number of paged pages */
 +uint64_t shared_info_frame ALIGN_64; /* MFN of shared_info struct */
 +uint64_t cpu_time ALIGN_64;  /* CPU time used */
 +uint32_t nr_online_vcpus;  /* Number of VCPUs currently online. */
 +uint32_t max_vcpu_id; /* Maximum VCPUID in use by this domain. */
 +uint32_t ssidref;
 +xen_domain_handle_t handle;
 +uint32_t cpupool;
 +};
 +typedef struct xen_v2d9_getdomaininfo xen_v2d9_getdomaininfo;
 +
  union xen_getdomaininfo {
  struct xen_v0_getdomaininfo v0;
  struct xen_v2_getdomaininfo v2;
 @@ -278,6 +296,7 @@ union xen_getdomaininfo {
  struct xen_v2d6_getdomaininfo v2d6;
  struct xen_v2d7_getdomaininfo v2d7;
  struct xen_v2d8_getdomaininfo v2d8;
 +struct xen_v2d9_getdomaininfo v2d9;
  };
  typedef union xen_getdomaininfo xen_getdomaininfo;
  
 @@ -288,6 +307,7 @@ union xen_getdomaininfolist {
  struct xen_v2d6_getdomaininfo *v2d6;
  struct xen_v2d7_getdomaininfo *v2d7;
  struct xen_v2d8_getdomaininfo *v2d8;
 +struct xen_v2d8_getdomaininfo *v2d9;
  };
  typedef union xen_getdomaininfolist xen_getdomaininfolist;
  
 @@ -325,7 +345,9 @@ typedef struct xen_v2s5_availheap  xen_v2s5_availheap;
  #define XEN_GETDOMAININFOLIST_ALLOC(domlist, size)  \
  (hv_versions.hypervisor  2 ?   \
   (VIR_ALLOC_N(domlist.v0, (size)) == 0) :   \
 - (hv_versions.dom_interface = 8 ?  \
 + (hv_versions.dom_interface = 9 ?  \
 +  (VIR_ALLOC_N(domlist.v2d9, (size)) == 0) :\
 + (hv_versions.dom_interface == 8 ?  \
(VIR_ALLOC_N(domlist.v2d8, (size)) == 0) :\
   (hv_versions.dom_interface == 7 ?  \
(VIR_ALLOC_N(domlist.v2d7, (size)) == 0) :\
 @@ -333,12 +355,14 @@ typedef struct xen_v2s5_availheap  xen_v2s5_availheap;
(VIR_ALLOC_N(domlist.v2d6, (size)) == 0) :\
   (hv_versions.dom_interface == 5 ?  \
(VIR_ALLOC_N(domlist.v2d5, (size)) == 0) :\
 -  (VIR_ALLOC_N(domlist.v2, (size)) == 0))
 +  (VIR_ALLOC_N(domlist.v2, (size)) == 0)))
   
 
 I'll be happy when we can ignore changes to sysctl and domctl interface
 in this driver so this madness can stop :).
 
 [...]
  
  
 @@ -1916,6 +1968,19 @@ xenHypervisorInit(struct xenHypervisorVersions 
 *override_versions)
  }
  }
  
 +/* Xen 4.3
 + * sysctl version A - xen-4.3 release
 + * domctl version 9 - xen-4.3 release
   
 
 I like to reference the commits that bump the versions, making it easy
 to look at the interface changes.
 
 + */
 +hv_versions.sys_interface = 0xA; /* XEN_SYSCTL_INTERFACE_VERSION */
 +if (virXen_getdomaininfo(fd, 0, info) == 1) {
 +hv_versions.dom_interface = 0x9; /* XEN_DOMCTL_INTERFACE_VERSION */
 +if (virXen_getvcpusinfo(fd, 0, 0, ipt, NULL, 0) == 0) {
 +VIR_DEBUG(Using hypervisor call v2, sys verA dom ver9);
 +goto done;
 +}
   
 
 Tabs here instead of spaces, caught by 'make syntax-check'.

Ah good to know/remember that. A bit of a problem jumping between projects

[libvirt] [PATCH] xen: Use internal interfaces in xenDomainUsedCpus

2013-07-16 Thread Stefan Bader

Based on Daniel's feedback I did a split for public/private functions
for those that cause the lockup when getting XML. Maybe not complete
but at least seems to allow basic usage again (through virt-manager).

-Stefan

---

From f406c6891fb92a45dc5d5a4d794c5d667965d096 Mon Sep 17 00:00:00 2001
From: Stefan Bader stefan.ba...@canonical.com
Date: Mon, 15 Jul 2013 16:03:58 +0200
Subject: [PATCH] xen: Use internal interfaces in xenDomainUsedCpus

Since commit 95e18efd most public interfaces (xenUnified...) obtain
a virDomainDefPtr via xenGetDomainDefFor...() which take the unified
lock.
This is already taken before calling xenDomainUsedCpus(), so we get
a deadlock for active guests. Avoid this by splitting up
xenUnifiedDomainGetVcpusFlags() and xenUnifiedDomainGetVcpus() into
public and private function calls (which get the virDomainDefPtr passed)
and use those in xenDomainUsedCpus().

xenDomainUsedCpus
  ...
  nb_vcpu = xenUnifiedDomainGetMaxVcpus(dom);
return xenUnifiedDomainGetVcpusFlags(...)
  ...
  if (!(def = xenGetDomainDefForDom(dom)))
return xenGetDomainDefForUUID(dom-conn, dom-uuid);
  ...
  ret = xenHypervisorLookupDomainByUUID(conn, uuid);
...
xenUnifiedLock(priv);
name = xenStoreDomainGetName(conn, id);
xenUnifiedUnlock(priv);
  ...
  if ((ncpus = xenUnifiedDomainGetVcpus(dom, cpuinfo, nb_vcpu,
...
if (!(def = xenGetDomainDefForDom(dom)))
  [again like above]

Signed-off-by: Stefan Bader stefan.ba...@canonical.com
---
 src/xen/xen_driver.c |   94 +-
 src/xen/xen_driver.h |2 +-
 2 files changed, 64 insertions(+), 32 deletions(-)

diff --git a/src/xen/xen_driver.c b/src/xen/xen_driver.c
index 39334b7..9ae728e 100644
--- a/src/xen/xen_driver.c
+++ b/src/xen/xen_driver.c
@@ -73,12 +73,14 @@
 
 static int
 xenUnifiedNodeGetInfo(virConnectPtr conn, virNodeInfoPtr info);
+
 static int
-xenUnifiedDomainGetMaxVcpus(virDomainPtr dom);
+__xenUnifiedDomainGetVcpusFlags(virDomainPtr dom, virDomainDefPtr def,
+   unsigned int flags);
 static int
-xenUnifiedDomainGetVcpus(virDomainPtr dom,
- virVcpuInfoPtr info, int maxinfo,
- unsigned char *cpumaps, int maplen);
+__xenUnifiedDomainGetVcpus(virDomainPtr dom, virDomainDefPtr def,
+  virVcpuInfoPtr info, int maxinfo,
+  unsigned char *cpumaps, int maplen);
 
 
 static bool is_privileged = false;
@@ -173,6 +175,7 @@ xenNumaInit(virConnectPtr conn) {
 /**
  * xenDomainUsedCpus:
  * @dom: the domain
+ * @def: the domain definition
  *
  * Analyze which set of CPUs are used by the domain and
  * return a string providing the ranges.
@@ -181,7 +184,7 @@ xenNumaInit(virConnectPtr conn) {
  * NULL if the domain uses all CPU or in case of error.
  */
 char *
-xenDomainUsedCpus(virDomainPtr dom)
+xenDomainUsedCpus(virDomainPtr dom, virDomainDefPtr def)
 {
 char *res = NULL;
 int ncpus;
@@ -202,9 +205,14 @@ xenDomainUsedCpus(virDomainPtr dom)
 
 if (priv-nbNodeCpus = 0)
 return NULL;
-nb_vcpu = xenUnifiedDomainGetMaxVcpus(dom);
+nb_vcpu = __xenUnifiedDomainGetVcpusFlags(dom, def,
+ (VIR_DOMAIN_VCPU_LIVE |
+  VIR_DOMAIN_VCPU_MAXIMUM));
 if (nb_vcpu = 0)
 return NULL;
+/* FIXME: To be consistent this should map to an internal interface, too.
+ * Currently it actually does map straight to xenDaemonNodeGetInfo().
+ */
 if (xenUnifiedNodeGetInfo(dom-conn, nodeinfo)  0)
 return NULL;
 
@@ -217,8 +225,8 @@ xenDomainUsedCpus(virDomainPtr dom)
 VIR_ALLOC_N(cpumap, nb_vcpu * cpumaplen)  0)
 goto done;
 
-if ((ncpus = xenUnifiedDomainGetVcpus(dom, cpuinfo, nb_vcpu,
-  cpumap, cpumaplen)) = 0) {
+if ((ncpus = __xenUnifiedDomainGetVcpus(dom, def, cpuinfo, nb_vcpu,
+   cpumap, cpumaplen)) = 0) {
 for (n = 0; n  ncpus; n++) {
 for (m = 0; m  priv-nbNodeCpus; m++) {
 bool used;
@@ -1410,54 +1418,57 @@ cleanup:
 }
 
 static int
-xenUnifiedDomainGetVcpus(virDomainPtr dom,
- virVcpuInfoPtr info, int maxinfo,
- unsigned char *cpumaps, int maplen)
+__xenUnifiedDomainGetVcpus(virDomainPtr dom, virDomainDefPtr def,
+  virVcpuInfoPtr info, int maxinfo,
+  unsigned char *cpumaps, int maplen)
 {
 xenUnifiedPrivatePtr priv = dom-conn-privateData;
-virDomainDefPtr def = NULL;
 int ret = -1;
 
-if (!(def = xenGetDomainDefForDom(dom)))
-goto cleanup;
-
-if (virDomainGetVcpusEnsureACL(dom-conn, def)  0)
-goto cleanup;
-
 if (dom-id  0

1 2 >

1 - 100 of 126 matches

Mail list logo