4:58 p.m.
Attached is 0001-apparmor-dont-ignore-open.patch
--
Jamie Strandboge | http://www.canonical.com
11:12 a.m.
On Fri, Aug 13, 2010 at 04:58:57PM -0500, Jamie Strandboge wrote:
Attached is 0001-apparmor-dont-ignore-open.patch
--
Jamie Strandboge | http://www.canonical.com
Index: libvirt-0.8.3/src/security/virt-aa-helper.c
===================================================================
--- libvirt-0.8.3.orig/src/security/virt-aa-helper.c 2010-08-12 09:51:42.000000000 -0500
+++ libvirt-0.8.3/src/security/virt-aa-helper.c 2010-08-12 09:58:42.000000000 -0500
@@ -853,11 +853,25 @@
* careful than just ignoring them */
int ret = virDomainDiskDefForeachPath(ctl->def->disks[i],
ctl->allowDiskFormatProbing,
- true,
+ false,
add_file_path,
&buf);
- if (ret != 0)
+ /*
+ * If virDomainDiskDefForeachPath() fails, then exit with error,
+ * unless the disk doesn't exist, in which case we just skip it
+ * without error in order to preserve previous behavior.
+ */
+ if (ret != 0) {
+ if (ctl->def->disks[i] && ctl->def->disks[i]->src) {
+ if (!virFileExists(ctl->def->disks[i]->src)) {
+ continue;
+ } else {
+ vah_warning(ctl->def->disks[i]->src);
+ vah_warning(" skipped (bad disk format)");
+ }
+ }
goto clean;
+ }
Why do we need special behaviour to skip files which don't exist ?
If this is solely to keep the test suite happy, then IMHO the
fix should be in the test suite. Runtime production code shouldn't
have workarounds for test code.
Regards,
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
1:45 p.m.
Why do we need special behaviour to skip files which don't exist
?
If this is solely to keep the test suite happy, then IMHO the
fix should be in the test suite. Runtime production code shouldn't
have workarounds for test code.
This is not to work around the test suite.
The intent is to help people migrating from < 0.8.3 to 0.8.3. When
considering the following binary states: file exists, disk format is
specified, ignoreOpenFailure and allowProbing, what is in trunk (ie
ignoreOpenFailure=true) and this patch work the same in all cases except
when the file does not exist and the format is not specified and
allowProbing=false (the new default). With what is in trunk, this is an
error condition, with my patch, the domain will start but the disk is
not added to the profile. Throwing an error because the format isn't
correct on a disk that doesn't exist seemed an odd side-effect when
migrating. I can see arguments against this, and would like to commit
the changes to tests/virt-aa-helper-test regardless.
--
Jamie Strandboge | http://www.canonical.com
2:11 p.m.
On Mon, Aug 16, 2010 at 01:45:45PM -0500, Jamie Strandboge wrote:
> Why do we need special behaviour to skip files which don't
exist ?
> If this is solely to keep the test suite happy, then IMHO the
> fix should be in the test suite. Runtime production code shouldn't
> have workarounds for test code.
This is not to work around the test suite.
The intent is to help people migrating from < 0.8.3 to 0.8.3. When
considering the following binary states: file exists, disk format is
specified, ignoreOpenFailure and allowProbing, what is in trunk (ie
ignoreOpenFailure=true) and this patch work the same in all cases except
when the file does not exist and the format is not specified and
allowProbing=false (the new default). With what is in trunk, this is an
error condition, with my patch, the domain will start but the disk is
not added to the profile. Throwing an error because the format isn't
correct on a disk that doesn't exist seemed an odd side-effect when
migrating. I can see arguments against this, and would like to commit
the changes to tests/virt-aa-helper-test regardless.
How can the domain start if the configured disk file doesn't exist on
the host filesystem ? QEMU will try to open a non-existant file, fail,
and abort. Failing on non-existant files when setting up the security
profile doesn't change that, it just makes us report the problem to the
user soon in the startup process.
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
3:59 p.m.
On Mon, 2010-08-16 at 20:11 +0100, Daniel P. Berrange wrote:
How can the domain start if the configured disk file doesn't
exist on
the host filesystem ? QEMU will try to open a non-existant file, fail,
and abort. Failing on non-existant files when setting up the security
profile doesn't change that, it just makes us report the problem to the
user soon in the startup process.
I got mixed up thinking of the case when the disk does exist but the
format was not originally specified (and therefore now defaults to raw)
but the disk is non-raw. In that case, the domain starts and POSTs, but
there is no disk to boot off of.
I tested this quite a bit more and you are correct that virt-aa-helper
does not have to be adjusted. I've attached an updated patch which only
adds the new test cases for the -p option.
Thanks again for your review.
--
Jamie Strandboge | http://www.canonical.com
9:59 a.m.
On Mon, 2010-08-16 at 15:59 -0500, Jamie Strandboge wrote:
On Mon, 2010-08-16 at 20:11 +0100, Daniel P. Berrange wrote:
> How can the domain start if the configured disk file doesn't exist on
> the host filesystem ? QEMU will try to open a non-existant file, fail,
> and abort. Failing on non-existant files when setting up the security
> profile doesn't change that, it just makes us report the problem to the
> user soon in the startup process.
I got mixed up thinking of the case when the disk does exist but the
format was not originally specified (and therefore now defaults to raw)
but the disk is non-raw. In that case, the domain starts and POSTs, but
there is no disk to boot off of.
I tested this quite a bit more and you are correct that virt-aa-helper
does not have to be adjusted. I've attached an updated patch which only
adds the new test cases for the -p option.
Thanks again for your review.
Any word on this, and the 2/4 and 3/4 updated patches from the same
date? (4/4 should have already been committed by Eric Blake).
Thanks!
--
Jamie Strandboge | http://www.canonical.com
10:08 a.m.
On Mon, Aug 16, 2010 at 03:59:17PM -0500, Jamie Strandboge wrote:
On Mon, 2010-08-16 at 20:11 +0100, Daniel P. Berrange wrote:
> How can the domain start if the configured disk file doesn't exist on
> the host filesystem ? QEMU will try to open a non-existant file, fail,
> and abort. Failing on non-existant files when setting up the security
> profile doesn't change that, it just makes us report the problem to the
> user soon in the startup process.
I got mixed up thinking of the case when the disk does exist but the
format was not originally specified (and therefore now defaults to raw)
but the disk is non-raw. In that case, the domain starts and POSTs, but
there is no disk to boot off of.
I tested this quite a bit more and you are correct that virt-aa-helper
does not have to be adjusted. I've attached an updated patch which only
adds the new test cases for the -p option.
Thanks again for your review.
--
Jamie Strandboge | http://www.canonical.com
Author: Jamie Strandboge <jamie(a)canonical.com>
Description: add extra tests to virt-aa-helper-test for new '-p' option
Index: libvirt-0.8.3/tests/virt-aa-helper-test
===================================================================
--- libvirt-0.8.3.orig/tests/virt-aa-helper-test 2010-08-12 09:51:53.000000000 -0500
+++ libvirt-0.8.3/tests/virt-aa-helper-test 2010-08-12 10:03:11.000000000 -0500
@@ -144,6 +144,7 @@
testme "1" "invalid arg" "-z"
testme "1" "invalid case" "-A"
testme "1" "not enough args" "-c"
+testme "1" "not enough args" "-p"
cat "$template_xml" | sed "s,###UUID###,$uuid,g" | sed
"s,###DISK###,$disk1,g" > "$test_xml"
testme "1" "no -u with -c" "-c" "$test_xml"
@@ -160,17 +161,25 @@
cat "$template_xml" | sed "s,###UUID###,$uuid,g" | sed
"s,###DISK###,$bad_disk,g" > "$test_xml"
testme "1" "bad disk" "-c -u $valid_uuid"
"$test_xml"
-cat "$template_xml" | sed "s,###UUID###,$uuid,g" | sed
"s,###DISK###,$bad_disk,g" | sed "s,</devices>,<disk
type='file' device='disk'><source
file='$disk2'/><target dev='hda'
bus='ide'/></disk></devices>,g" > "$test_xml"
+cat "$template_xml" | sed "s,###UUID###,$uuid,g" | sed
"s,###DISK###,$bad_disk,g" | sed "s,</devices>,<disk
type='file' device='disk'><driver name='qemu'
type='raw'/><source file='$disk2'/><target dev='hda'
bus='ide'/></disk></devices>,g" > "$test_xml"
+
testme "1" "bad disk2" "-c -u $valid_uuid"
"$test_xml"
cat "$template_xml" | sed "s,###UUID###,$uuid,g" | sed
"s,###DISK###,$disk1,g" | sed "s,</devices>,<devices>,g"
> "$test_xml"
testme "1" "malformed xml" "-c -u $valid_uuid"
"$test_xml"
-cat "$template_xml" | sed "s,###UUID###,$uuid,g" | sed
"s,###DISK###,/boot/initrd,g" > "$test_xml"
-testme "1" "disk in /boot" "-r -u $valid_uuid"
"$test_xml"
-
-cat "$template_xml" | sed "s,###UUID###,$uuid,g" | sed
"s,###DISK###,/boot/initrd,g" > "$test_xml"
-testme "1" "-r with invalid -f" "-r -u $valid_uuid -f
$bad_disk" "$test_xml"
+initrd=`ls -1 /boot/initrd* | head -1`
+if [ -z "$initrd" ]; then
+ echo "Skipping /boot/initrd* tests. Could not find /boot/initrd*"
+else
+ cat "$template_xml" | sed "s,###UUID###,$uuid,g" | sed
"s,###DISK###,$initrd,g" > "$test_xml"
+ testme "1" "disk in /boot without probing" "-p 0 -r -u
$valid_uuid" "$test_xml"
+ testme "1" "disk in /boot with probing" "-p 1 -r -u
$valid_uuid" "$test_xml"
+
+ cat "$template_xml" | sed "s,###UUID###,$uuid,g" | sed
"s,###DISK###,/boot/initrd,g" > "$test_xml"
+ testme "1" "-r with invalid -f with probing" "-p 1 -r -u
$valid_uuid -f $bad_disk" "$test_xml"
+ testme "1" "-r with invalid -f without probing" "-p 0 -r -u
$valid_uuid -f $bad_disk" "$test_xml"
+fi
cat "$template_xml" | sed "s,###UUID###,$uuid,g" | sed
"s,###DISK###,$disk1</disk>,g" > "$test_xml"
testme "1" "-c with malformed xml" "-c -u $valid_uuid"
"$test_xml"
@@ -195,8 +204,8 @@
cat "$template_xml" | sed "s,###UUID###,$uuid,g" | sed
"s,###DISK###,$disk1,g" | sed
"s,arch='x86_64',arch='ppc',g" > "$test_xml"
testme "0" "create (ppc)" "-c -u $valid_uuid"
"$test_xml"
-cat "$template_xml" | sed "s,###UUID###,$uuid,g" | sed
"s,###DISK###,$disk1,g" | sed "s,</disk>,</disk><disk
type='file' device='disk'><source
file='$disk2'/><target dev='hdb'
bus='ide'/></disk>,g" > "$test_xml"
-testme "0" "create multiple disks" "-c -u $valid_uuid -p
1" "$test_xml"
+cat "$template_xml" | sed "s,###UUID###,$uuid,g" | sed
"s,###DISK###,$disk1,g" | sed "s,</disk>,</disk><disk
type='file' device='disk'><driver name='qemu'
type='raw'/><source file='$disk2'/><target dev='hdb'
bus='ide'/></disk>,g" > "$test_xml"
+testme "0" "create multiple disks" "-c -u $valid_uuid"
"$test_xml"
cat "$template_xml" | sed "s,###UUID###,$uuid,g" | sed
"s,###DISK###',${disk1}'/><readonly,g" > "$test_xml"
testme "0" "create (readonly)" "-c -u $valid_uuid"
"$test_xml"
ACK
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
12:30 p.m.
On 09/23/2010 09:08 AM, Daniel P. Berrange wrote:
>
> I tested this quite a bit more and you are correct that virt-aa-helper
> does not have to be adjusted. I've attached an updated patch which only
> adds the new test cases for the -p option.
>
> Thanks again for your review.
>
ACK
Pushed.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
5176
days inactive
5217
days old
7 comments
3 participants
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Jamie Strandboge