Re: [libvirt] [PATCH 1/5] virt-aa-helper: Ignore open errors again

2010/7/27 Daniel P. Berrange <berrange(a)redhat.com&gt;: > On Tue, Jul 27, 2010 at 11:46:34AM +0200, Matthias Bolte wrote: >> 2010/7/26 Daniel P. Berrange <berrange(a)redhat.com&gt;: >> > On Sat, Jul 24, 2010 at 12:28:11AM +0200, Jamie Strandboge wrote: >> >> On Fri, 2010-07-23 at 19:24 +0200, Matthias Bolte wrote: >> >> > virt-aa-helper used to ignore errors when opening files. >> >> > Commit a8853344994a7c6aaca882a5e949ab5536821ab5 refactored >> >> > the related code and changed this behavior. virt-aa-helper >> >> > didn't ignore open errors anymore and virt-aa-helper-test >> >> > fails. >> >> > >> >> > Make sure that virt-aa-helper ignores open errors again. >> >> > --- >> >> >  src/security/virt-aa-helper.c |    2 +- >> >> >  1 files changed, 1 insertions(+), 1 deletions(-) >> >> > >> >> > diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c >> >> > index 521545d..16b1920 100644 >> >> > --- a/src/security/virt-aa-helper.c >> >> > +++ b/src/security/virt-aa-helper.c >> >> > @@ -846,7 +846,7 @@ get_files(vahControl * ctl) >> >> >      for (i = 0; i < ctl->def->ndisks; i++) { >> >> >          int ret = virDomainDiskDefForeachPath(ctl->def->disks[i], >> >> >                                                ctl->allowDiskFormatProbing, >> >> > -                                              false, >> >> > +                                              true, >> >> >                                                add_file_path, >> >> >                                                &buf); >> >> >          if (ret != 0) >> >> >> >> I'm not 100% sure on this one. I have been developing patches to adjust >> >> for the new behavior on older releases and I did some shuffling to get >> >> this to work with 'false'. I'm not ready to submit at this time, and >> >> won't be able to get to it until the week after next. If this blocks >> >> Matthias' work, then feel free to commit and I'll post with a different >> >> patch if needed. Otherwise, we can wait. >> > >> > What is the scenario in which 'false' breaks things ? We use 'false' for >> > the selinux driver already. The problem with 'true' is that it means the >> > user will never see potentially important errors. >> > >> > Regards, >> > Daniel >> >> Maybe it's a problem that's triggered by the test only. >> >> Passing 'false' makes virt-aa-helper-test fail for tests that involve >> non-existing files: >> >> $ ./virt-aa-helper-test >> FAIL: exited with '1' >>   create (non-existent disk):  '--dryrun -c -u >> libvirt-00000000-0000-0000-0000-0123456789ab': >> FAIL: exited with '1' >>   replace (non-existent disk):  '--dryrun -r -u >> libvirt-00000000-0000-0000-0000-0123456789ab': >> >> Here's the command that virt-aa-helper-test executes for the first failing test: >> >> $ LD_LIBRARY_PATH="../src/.libs/" ../src/virt-aa-helper --dryrun -c -u >> libvirt-00000000-0000-0000-0000-0123456789ab < >> /tmp/tmp.IFQ5dqTvp6/test.xml >> virt-aa-helper: warning: path does not exist, skipping file type checks >> virt-aa-helper: error: invalid VM definition > > Hmm, this does look like a test script bug. It should at least touch > the files so they exist before running. If a real deployment you would > actually want these kind of error messages about non-existant files. > I don't think that it's a bug in the test script in the way that it should touch the files. The test are named "create (non-existent disk)" and "replace (non-existent disk)". I think the are meant to work with non-existent files. As I think of it, I'm not sure whether or not this two tests cases should be there at all. Is there a real use case with non-existent files that this two test cover? If not then we could back to 'true' and remove this two test cases.