2:37 a.m.
This is a v2 of:
https://www.redhat.com/archives/libvir-list/2018-July/msg00425.html
diff to v1:
- Patch 1/6 from original series was ACKed and thus pushed,
- Patch 2/6 from original series is replaced with 1/5,
- The rest of the patches remained unreviewed, therefore I'm resending
them.
Michal Prívozník (5):
qemuxml2argvtest: Set more fake drivers
Forget last daemon/ dir artefacts
virtestmock: Track connect() too
check-file-access: Allow specifying action
virtestmock: Track action
Makefile.am | 2 +-
run.in | 2 +-
tests/check-file-access.pl | 32 ++++++++++++++++++----
tests/file_access_whitelist.txt | 15 +++++++----
tests/libvirtd-fail | 4 +--
tests/qemuxml2argvtest.c | 6 +++++
tests/virtestmock.c | 59 ++++++++++++++++++++++++++++++-----------
7 files changed, 90 insertions(+), 30 deletions(-)
--
2.16.4
2:37 a.m.
New subject: [libvirt] [PATCH v2 1/5] qemuxml2argvtest: Set more fake drivers
So far we are setting only fake secret and storage drivers.
Therefore if the code wants to call a public NWFilter API (like
qemuBuildInterfaceCommandLine() and qemuBuildNetCommandLine() are
doing) the virGetConnectNWFilter() function will try to actually
spawn session daemon because there's no connection object set to
handle NWFilter driver.
Even though I haven't experienced the same problem with the rest
of the drivers (interface, network and node dev), the reasoning
above can be applied to them as well.
At the same time, now that connection object is registered for
the drivers, the public APIs will throw
virReportUnsupportedError(). And since we don't provide any error
func the error is printed to stderr. Fix this by setting dummy
error func.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
tests/qemuxml2argvtest.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index a929e4314e..28073e2c40 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -471,6 +471,10 @@ testCompareXMLToArgv(const void *data)
conn->secretDriver = &fakeSecretDriver;
conn->storageDriver = &fakeStorageDriver;
+ virSetConnectInterface(conn);
+ virSetConnectNetwork(conn);
+ virSetConnectNWFilter(conn);
+ virSetConnectNodeDev(conn);
virSetConnectSecret(conn);
virSetConnectStorage(conn);
@@ -652,6 +656,8 @@ mymain(void)
return EXIT_FAILURE;
}
+ virTestQuiesceLibvirtErrors(true);
+
if (qemuTestDriverInit(&driver) < 0)
return EXIT_FAILURE;
--
2.16.4
3:01 a.m.
New subject: [libvirt] [PATCH v2 1/5] qemuxml2argvtest: Set more fake drivers
On Thu, Jul 12, 2018 at 09:37:47 +0200, Michal Privoznik wrote:
So far we are setting only fake secret and storage drivers.
Therefore if the code wants to call a public NWFilter API (like
qemuBuildInterfaceCommandLine() and qemuBuildNetCommandLine() are
doing) the virGetConnectNWFilter() function will try to actually
spawn session daemon because there's no connection object set to
handle NWFilter driver.
Even though I haven't experienced the same problem with the rest
of the drivers (interface, network and node dev), the reasoning
above can be applied to them as well.
At the same time, now that connection object is registered for
the drivers, the public APIs will throw
virReportUnsupportedError(). And since we don't provide any error
func the error is printed to stderr. Fix this by setting dummy
error func.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
tests/qemuxml2argvtest.c | 6 ++++++
1 file changed, 6 insertions(+)
[...]
@@ -652,6 +656,8 @@ mymain(void)
return EXIT_FAILURE;
}
+ virTestQuiesceLibvirtErrors(true);
+
NACK, this suppresses legitimate errors in the testsuite.
I've mangled one of the XML files and ran the qemuxml2argvtest with
VIR_TEST_DEBUG=1 and got:
249) QEMU XML-2-startup-XML disk-drive-cache-directsync ... SKIP
250) QEMU XML-2-ARGV disk-drive-cache-unsafe ... FAILED
251) QEMU XML-2-startup-XML disk-drive-cache-unsafe ... SKIP
Without this patch I'd get:
249) QEMU XML-2-startup-XML disk-drive-cache-directsync ... SKIP
250) QEMU XML-2-ARGV disk-drive-cache-unsafe ... libvirt: Domain
Config error : unsupported configuration: unknown disk cache mode 'unafe'
FAILED
251) QEMU XML-2-startup-XML disk-drive-cache-unsafe ... SKIP
if (qemuTestDriverInit(&driver) < 0)
return EXIT_FAILURE;
--
2.16.4
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
3:51 a.m.
New subject: [libvirt] [PATCH v2 1/5] qemuxml2argvtest: Set more fake drivers
On 07/12/2018 10:01 AM, Peter Krempa wrote:
On Thu, Jul 12, 2018 at 09:37:47 +0200, Michal Privoznik wrote:
> So far we are setting only fake secret and storage drivers.
> Therefore if the code wants to call a public NWFilter API (like
> qemuBuildInterfaceCommandLine() and qemuBuildNetCommandLine() are
> doing) the virGetConnectNWFilter() function will try to actually
> spawn session daemon because there's no connection object set to
> handle NWFilter driver.
>
> Even though I haven't experienced the same problem with the rest
> of the drivers (interface, network and node dev), the reasoning
> above can be applied to them as well.
>
> At the same time, now that connection object is registered for
> the drivers, the public APIs will throw
> virReportUnsupportedError(). And since we don't provide any error
> func the error is printed to stderr. Fix this by setting dummy
> error func.
>
> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
> ---
> tests/qemuxml2argvtest.c | 6 ++++++
> 1 file changed, 6 insertions(+)
[...]
>
> @@ -652,6 +656,8 @@ mymain(void)
> return EXIT_FAILURE;
> }
>
> + virTestQuiesceLibvirtErrors(true);
> +
NACK, this suppresses legitimate errors in the testsuite.
I've mangled one of the XML files and ran the qemuxml2argvtest with
VIR_TEST_DEBUG=1 and got:
249) QEMU XML-2-startup-XML disk-drive-cache-directsync ... SKIP
250) QEMU XML-2-ARGV disk-drive-cache-unsafe ... FAILED
251) QEMU XML-2-startup-XML disk-drive-cache-unsafe ... SKIP
Without this patch I'd get:
249) QEMU XML-2-startup-XML disk-drive-cache-directsync ... SKIP
250) QEMU XML-2-ARGV disk-drive-cache-unsafe ... libvirt:
Domain Config error : unsupported configuration: unknown disk cache mode 'unafe'
FAILED
251) QEMU XML-2-startup-XML disk-drive-cache-unsafe ... SKIP
Well, without it I get:
tests $ ./qemuxml2argvtest
TEST: qemuxml2argvtest
._._._._..._._._._._._._._._._._._._._._ 40
._._._._._._._._._._._._._._._._._._._._ 80
._._._._._._._._._._._._._._._._._._._._ 120
._._._._._._._._._._._._._._._._._._._._ 160
._._._._._._._._._._._._._._._._._._._._ 200
._._._._._._._._._._._._._._._._..._._._ 240
.._._._._._...._._._._._._._._._._._._._ 280
._._._._._._._._._._._._._._._._._._._._ 320
._._._._._._._._._._._._._._._._._._._._ 360
._._._._._._._._._._._._._._._._._._._._ 400
._._._._._._._._._._._._._._._._._._._._ 440
._._._._._._._._libvirt: Network Filter Driver error : internal
error: unexpected nwfilter URI path '/session', try nwfilter:///system
libvirt: Network Filter Driver error : internal error: unexpected
nwfilter URI path '/session', try nwfilter:///system
libvirt: Network Filter Driver error : internal error: unexpected
nwfilter URI path '/session', try nwfilter:///system
libvirt: Network Filter Driver error : internal error: unexpected
nwfilter URI path '/session', try nwfilter:///system
libvirt: Network Filter Driver error : internal error: unexpected
nwfilter URI path '/session', try nwfilter:///system
._._._._._._._._._._._._ 480
So do you have any other idea? I came up with two already and neither of
them got through review. Just to remind everybody, we are possibly
touching live user data here so we need a resolution rather sooner than
later.
Michal
3:56 a.m.
New subject: [libvirt] [PATCH v2 1/5] qemuxml2argvtest: Set more fake drivers
On Thu, Jul 12, 2018 at 10:51:33 +0200, Michal Privoznik wrote:
On 07/12/2018 10:01 AM, Peter Krempa wrote:
> On Thu, Jul 12, 2018 at 09:37:47 +0200, Michal Privoznik wrote:
>> So far we are setting only fake secret and storage drivers.
>> Therefore if the code wants to call a public NWFilter API (like
>> qemuBuildInterfaceCommandLine() and qemuBuildNetCommandLine() are
>> doing) the virGetConnectNWFilter() function will try to actually
>> spawn session daemon because there's no connection object set to
>> handle NWFilter driver.
>>
>> Even though I haven't experienced the same problem with the rest
>> of the drivers (interface, network and node dev), the reasoning
>> above can be applied to them as well.
>>
>> At the same time, now that connection object is registered for
>> the drivers, the public APIs will throw
>> virReportUnsupportedError(). And since we don't provide any error
>> func the error is printed to stderr. Fix this by setting dummy
>> error func.
>>
>> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
>> ---
>> tests/qemuxml2argvtest.c | 6 ++++++
>> 1 file changed, 6 insertions(+)
>
> [...]
>
>>
>> @@ -652,6 +656,8 @@ mymain(void)
>> return EXIT_FAILURE;
>> }
>>
>> + virTestQuiesceLibvirtErrors(true);
>> +
>
> NACK, this suppresses legitimate errors in the testsuite.
>
> I've mangled one of the XML files and ran the qemuxml2argvtest with
> VIR_TEST_DEBUG=1 and got:
>
> 249) QEMU XML-2-startup-XML disk-drive-cache-directsync ... SKIP
> 250) QEMU XML-2-ARGV disk-drive-cache-unsafe ... FAILED
> 251) QEMU XML-2-startup-XML disk-drive-cache-unsafe ... SKIP
>
> Without this patch I'd get:
>
> 249) QEMU XML-2-startup-XML disk-drive-cache-directsync ... SKIP
> 250) QEMU XML-2-ARGV disk-drive-cache-unsafe ... libvirt:
Domain Config error : unsupported configuration: unknown disk cache mode 'unafe'
> FAILED
> 251) QEMU XML-2-startup-XML disk-drive-cache-unsafe ... SKIP
>
Well, without it I get:
tests $ ./qemuxml2argvtest
TEST: qemuxml2argvtest
._._._._..._._._._._._._._._._._._._._._ 40
._._._._._._._._._._._._._._._._._._._._ 80
._._._._._._._._._._._._._._._._._._._._ 120
._._._._._._._._._._._._._._._._._._._._ 160
._._._._._._._._._._._._._._._._._._._._ 200
._._._._._._._._._._._._._._._._..._._._ 240
.._._._._._...._._._._._._._._._._._._._ 280
._._._._._._._._._._._._._._._._._._._._ 320
._._._._._._._._._._._._._._._._._._._._ 360
._._._._._._._._._._._._._._._._._._._._ 400
._._._._._._._._._._._._._._._._._._._._ 440
._._._._._._._._libvirt: Network Filter Driver error : internal
error: unexpected nwfilter URI path '/session', try nwfilter:///system
libvirt: Network Filter Driver error : internal error: unexpected
nwfilter URI path '/session', try nwfilter:///system
libvirt: Network Filter Driver error : internal error: unexpected
nwfilter URI path '/session', try nwfilter:///system
libvirt: Network Filter Driver error : internal error: unexpected
nwfilter URI path '/session', try nwfilter:///system
libvirt: Network Filter Driver error : internal error: unexpected
nwfilter URI path '/session', try nwfilter:///system
._._._._._._._._._._._._ 480
So do you have any other idea? I came up with two already and neither of
them got through review. Just to remind everybody, we are possibly
touching live user data here so we need a resolution rather sooner than
later.
I specifically NACKd the part that installs the callback for suppressing
error messages. The messages here need to be suppressed by some other
way, but we should not decrease the debugability of tests.
The error handler installation does not seem to have to do with live
user data touching.
I did not try to see what the other part of this patch does.
4 a.m.
New subject: [libvirt] [PATCH v2 1/5] qemuxml2argvtest: Set more fake drivers
On 07/12/2018 10:56 AM, Peter Krempa wrote:
On Thu, Jul 12, 2018 at 10:51:33 +0200, Michal Privoznik wrote:
> On 07/12/2018 10:01 AM, Peter Krempa wrote:
>> On Thu, Jul 12, 2018 at 09:37:47 +0200, Michal Privoznik wrote:
>>> So far we are setting only fake secret and storage drivers.
>>> Therefore if the code wants to call a public NWFilter API (like
>>> qemuBuildInterfaceCommandLine() and qemuBuildNetCommandLine() are
>>> doing) the virGetConnectNWFilter() function will try to actually
>>> spawn session daemon because there's no connection object set to
>>> handle NWFilter driver.
>>>
>>> Even though I haven't experienced the same problem with the rest
>>> of the drivers (interface, network and node dev), the reasoning
>>> above can be applied to them as well.
>>>
>>> At the same time, now that connection object is registered for
>>> the drivers, the public APIs will throw
>>> virReportUnsupportedError(). And since we don't provide any error
>>> func the error is printed to stderr. Fix this by setting dummy
>>> error func.
>>>
>>> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
>>> ---
>>> tests/qemuxml2argvtest.c | 6 ++++++
>>> 1 file changed, 6 insertions(+)
>>
>> [...]
>>
>>>
>>> @@ -652,6 +656,8 @@ mymain(void)
>>> return EXIT_FAILURE;
>>> }
>>>
>>> + virTestQuiesceLibvirtErrors(true);
>>> +
>>
>> NACK, this suppresses legitimate errors in the testsuite.
>>
>> I've mangled one of the XML files and ran the qemuxml2argvtest with
>> VIR_TEST_DEBUG=1 and got:
>>
>> 249) QEMU XML-2-startup-XML disk-drive-cache-directsync ... SKIP
>> 250) QEMU XML-2-ARGV disk-drive-cache-unsafe ...
FAILED
>> 251) QEMU XML-2-startup-XML disk-drive-cache-unsafe ... SKIP
>>
>> Without this patch I'd get:
>>
>> 249) QEMU XML-2-startup-XML disk-drive-cache-directsync ... SKIP
>> 250) QEMU XML-2-ARGV disk-drive-cache-unsafe ...
libvirt: Domain Config error : unsupported configuration: unknown disk cache mode
'unafe'
>> FAILED
>> 251) QEMU XML-2-startup-XML disk-drive-cache-unsafe ... SKIP
>>
>
> Well, without it I get:
>
> tests $ ./qemuxml2argvtest
> TEST: qemuxml2argvtest
> ._._._._..._._._._._._._._._._._._._._._ 40
> ._._._._._._._._._._._._._._._._._._._._ 80
> ._._._._._._._._._._._._._._._._._._._._ 120
> ._._._._._._._._._._._._._._._._._._._._ 160
> ._._._._._._._._._._._._._._._._._._._._ 200
> ._._._._._._._._._._._._._._._._..._._._ 240
> .._._._._._...._._._._._._._._._._._._._ 280
> ._._._._._._._._._._._._._._._._._._._._ 320
> ._._._._._._._._._._._._._._._._._._._._ 360
> ._._._._._._._._._._._._._._._._._._._._ 400
> ._._._._._._._._._._._._._._._._._._._._ 440
> ._._._._._._._._libvirt: Network Filter Driver error : internal
> error: unexpected nwfilter URI path '/session', try nwfilter:///system
> libvirt: Network Filter Driver error : internal error: unexpected
> nwfilter URI path '/session', try nwfilter:///system
> libvirt: Network Filter Driver error : internal error: unexpected
> nwfilter URI path '/session', try nwfilter:///system
> libvirt: Network Filter Driver error : internal error: unexpected
> nwfilter URI path '/session', try nwfilter:///system
> libvirt: Network Filter Driver error : internal error: unexpected
> nwfilter URI path '/session', try nwfilter:///system
> ._._._._._._._._._._._._ 480
>
>
> So do you have any other idea? I came up with two already and neither of
> them got through review. Just to remind everybody, we are possibly
> touching live user data here so we need a resolution rather sooner than
> later.
I specifically NACKd the part that installs the callback for suppressing
error messages. The messages here need to be suppressed by some other
way, but we should not decrease the debugability of tests.
The error handler installation does not seem to have to do with live
user data touching.
I did not try to see what the other part of this patch does.
Okay, I will drop it. The output of the test would be ugly then, but I
don't care anymore.
Also, can I get review on the rest of the patches please? I noticed some
people started replying only to a single patch in a series leaving the
rest unreviewed. I don't think that's good.
Michal
4:08 a.m.
New subject: [libvirt] [PATCH v2 1/5] qemuxml2argvtest: Set more fake drivers
On Thu, Jul 12, 2018 at 11:00:42 +0200, Michal Privoznik wrote:
On 07/12/2018 10:56 AM, Peter Krempa wrote:
[...]
Also, can I get review on the rest of the patches please? I noticed
some
people started replying only to a single patch in a series leaving the
rest unreviewed. I don't think that's good.
I did not plan to review this series. I think it's warranted to point
out a problem with a patch even if you would not review that series
otherwise.
4:12 a.m.
New subject: [libvirt] [PATCH v2 1/5] qemuxml2argvtest: Set more fake drivers
On 07/12/2018 11:08 AM, Peter Krempa wrote:
On Thu, Jul 12, 2018 at 11:00:42 +0200, Michal Privoznik wrote:
> On 07/12/2018 10:56 AM, Peter Krempa wrote:
[...]
> Also, can I get review on the rest of the patches please? I noticed some
> people started replying only to a single patch in a series leaving the
> rest unreviewed. I don't think that's good.
I did not plan to review this series. I think it's warranted to point
out a problem with a patch even if you would not review that series
otherwise.
Well, unless reviewers are reading every e-mail in every thread, they
will see a discussion to this thread thinking there is an review going
on and thus skip to next (unreviewed) thread. This is usually my experience.
So while pointing out problems is a good thing to do, leaving the rest
unreviewed isn't IMO.
Michal
7:10 a.m.
New subject: [libvirt] [PATCH v2 1/5] qemuxml2argvtest: Set more fake drivers
On 07/12/2018 05:00 AM, Michal Privoznik wrote:
On 07/12/2018 10:56 AM, Peter Krempa wrote:
> On Thu, Jul 12, 2018 at 10:51:33 +0200, Michal Privoznik wrote:
>> On 07/12/2018 10:01 AM, Peter Krempa wrote:
>>> On Thu, Jul 12, 2018 at 09:37:47 +0200, Michal Privoznik wrote:
>>>> So far we are setting only fake secret and storage drivers.
>>>> Therefore if the code wants to call a public NWFilter API (like
>>>> qemuBuildInterfaceCommandLine() and qemuBuildNetCommandLine() are
>>>> doing) the virGetConnectNWFilter() function will try to actually
>>>> spawn session daemon because there's no connection object set to
>>>> handle NWFilter driver.
>>>>
>>>> Even though I haven't experienced the same problem with the rest
>>>> of the drivers (interface, network and node dev), the reasoning
>>>> above can be applied to them as well.
>>>>
>>>> At the same time, now that connection object is registered for
>>>> the drivers, the public APIs will throw
>>>> virReportUnsupportedError(). And since we don't provide any error
>>>> func the error is printed to stderr. Fix this by setting dummy
>>>> error func.
>>>>
>>>> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
>>>> ---
>>>> tests/qemuxml2argvtest.c | 6 ++++++
>>>> 1 file changed, 6 insertions(+)
>>>
>>> [...]
>>>
>>>>
>>>> @@ -652,6 +656,8 @@ mymain(void)
>>>> return EXIT_FAILURE;
>>>> }
>>>>
>>>> + virTestQuiesceLibvirtErrors(true);
>>>> +
>>>
>>> NACK, this suppresses legitimate errors in the testsuite.
>>>
>>> I've mangled one of the XML files and ran the qemuxml2argvtest with
>>> VIR_TEST_DEBUG=1 and got:
>>>
>>> 249) QEMU XML-2-startup-XML disk-drive-cache-directsync ...
SKIP
>>> 250) QEMU XML-2-ARGV disk-drive-cache-unsafe ...
FAILED
>>> 251) QEMU XML-2-startup-XML disk-drive-cache-unsafe ...
SKIP
>>>
>>> Without this patch I'd get:
>>>
>>> 249) QEMU XML-2-startup-XML disk-drive-cache-directsync ...
SKIP
>>> 250) QEMU XML-2-ARGV disk-drive-cache-unsafe ...
libvirt: Domain Config error : unsupported configuration: unknown disk cache mode
'unafe'
>>> FAILED
>>> 251) QEMU XML-2-startup-XML disk-drive-cache-unsafe ...
SKIP
>>>
>>
>> Well, without it I get:
>>
>> tests $ ./qemuxml2argvtest
>> TEST: qemuxml2argvtest
>> ._._._._..._._._._._._._._._._._._._._._ 40
>> ._._._._._._._._._._._._._._._._._._._._ 80
>> ._._._._._._._._._._._._._._._._._._._._ 120
>> ._._._._._._._._._._._._._._._._._._._._ 160
>> ._._._._._._._._._._._._._._._._._._._._ 200
>> ._._._._._._._._._._._._._._._._..._._._ 240
>> .._._._._._...._._._._._._._._._._._._._ 280
>> ._._._._._._._._._._._._._._._._._._._._ 320
>> ._._._._._._._._._._._._._._._._._._._._ 360
>> ._._._._._._._._._._._._._._._._._._._._ 400
>> ._._._._._._._._._._._._._._._._._._._._ 440
>> ._._._._._._._._libvirt: Network Filter Driver error : internal
>> error: unexpected nwfilter URI path '/session', try nwfilter:///system
>> libvirt: Network Filter Driver error : internal error: unexpected
>> nwfilter URI path '/session', try nwfilter:///system
>> libvirt: Network Filter Driver error : internal error: unexpected
>> nwfilter URI path '/session', try nwfilter:///system
>> libvirt: Network Filter Driver error : internal error: unexpected
>> nwfilter URI path '/session', try nwfilter:///system
>> libvirt: Network Filter Driver error : internal error: unexpected
>> nwfilter URI path '/session', try nwfilter:///system
>> ._._._._._._._._._._._._ 480
>>
FWIW: Using a "clean" build this same message appears for me before any
of these patches...
After this patch without Peter's NACK'd portion I get FOR TEST 469
(net-vhostuser-multiq):
libvirt: Network Filter Driver error : this function is not supported by
the connection driver: virNWFilterBindingLookupByPortDev
libvirt: Network Filter Driver error : this function is not supported by
the connection driver: virNWFilterBindingLookupByPortDev
libvirt: Network Filter Driver error : this function is not supported by
the connection driver: virNWFilterBindingLookupByPortDev
Running just test 469 (e.g. net-vhostuser-multiq w/
QEMU_CAPS_VHOSTUSER_MULTIQUEUE) using VIR_TEST_DEBUG=1 also shows:
2018-07-20 12:52:13.793+0000: 18540: warning :
qemuProcessStartWarnShmem:4923 : Detected vhost-user interface without
any shared memory, the interface might not be operational
2018-07-20 12:52:13.793+0000: 18540: error :
qemuBuildVhostuserCommandLine:8350 : unsupported configuration:
multi-queue is not supported for vhost-user with this QEMU binary
...
Since you're copying the virSetConnect*(conn) calls, I would think you
also need to create fake*Driver constructs as well; otherwise, for any
API called you'd get those function not supported messages. FWIW: I
believe they come from the virDomainConfNWFilterTeardownImpl code - it's
a real patience tester ;-).
FWIW: Investigating this is what sent me down the path of looking at
Jan's patches regarding needing jansson-devel because I was getting the
following results once Erik's patch to add a new capability (commit
7ab7d0ed49d8) were present:
./tests/qemuxml2argvtest
TEST: qemuxml2argvtest
._._._._ 8 FAIL
qemu was no longer being built, old images and libraries still existed,
and eventually failure was bound to occur.
>>
>> So do you have any other idea? I came up with two already and neither of
>> them got through review. Just to remind everybody, we are possibly
>> touching live user data here so we need a resolution rather sooner than
>> later.
>
> I specifically NACKd the part that installs the callback for suppressing
> error messages. The messages here need to be suppressed by some other
> way, but we should not decrease the debugability of tests.
>
> The error handler installation does not seem to have to do with live
> user data touching.
>
> I did not try to see what the other part of this patch does.
>
Okay, I will drop it. The output of the test would be ugly then, but I
don't care anymore.
Also, can I get review on the rest of the patches please? I noticed some
people started replying only to a single patch in a series leaving the
rest unreviewed. I don't think that's good.
This is always the ultimate conundrum - I agree with Michal that once
someone responds to a series that generally no one else looks. Well,
unless of course after a push it affects them negatively in some way
0-). On the other hand, since this patch seems separable from the rest
of the series (at least from my quick look), then perhaps multiple
series could have been posted. Of course that can get burdensome to
manage once reviews are given.
Still I think it'd be "good form" and "the polite thing" to do to
at
least indicate that you looked at the rest of the patches in a series
and they're generally OK or indicate that you don't plan on reviewing
them. IMO, nothing worse than "fixing" the one patch that gets reviewed,
posting a new series, and then having the second patch get dinged when
the first review could have easily done the same thing.
John
2:37 a.m.
New subject: [libvirt] [PATCH v2 2/5] Forget last daemon/ dir artefacts
The most important part is LIBVIRTD_PATH env var fix. It is used
in virFileFindResourceFull() from tests. The libvirtd no longer
lives under daemon/.
Then, libvirtd-fail test was still failing (as expected) but not
because of missing config file but because it was trying to
execute (nonexistent) top_builddir/daemon/libvirtd which
fulfilled expected outcome and thus test did not fail.
Thirdly, lcov was told to generate coverage for daemon/ dir too.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
Makefile.am | 2 +-
run.in | 2 +-
tests/libvirtd-fail | 4 ++--
3 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/Makefile.am b/Makefile.am
index 1926e21b7a..709064c6a6 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -80,7 +80,7 @@ check-access:
cov: clean-cov
$(MKDIR_P) $(top_builddir)/coverage
$(LCOV) -c -o $(top_builddir)/coverage/libvirt.info.tmp \
- -d $(top_builddir)/src -d $(top_builddir)/daemon \
+ -d $(top_builddir)/src \
-d $(top_builddir)/tests
$(LCOV) -r $(top_builddir)/coverage/libvirt.info.tmp \
-o $(top_builddir)/coverage/libvirt.info
diff --git a/run.in b/run.in
index cbef61a674..06ad54b62b 100644
--- a/run.in
+++ b/run.in
@@ -63,7 +63,7 @@ export PKG_CONFIG_PATH
export LIBVIRT_DRIVER_DIR="$b/src/.libs"
export LIBVIRT_LOCK_MANAGER_PLUGIN_DIR="$b/src/.libs"
export VIRTLOCKD_PATH="$b/src"
-export LIBVIRTD_PATH="$b/daemon"
+export LIBVIRTD_PATH="$b/src"
# This is a cheap way to find some use-after-free and uninitialized
# read problems when using glibc.
diff --git a/tests/libvirtd-fail b/tests/libvirtd-fail
index 6c61b892cb..f9e927b61f 100755
--- a/tests/libvirtd-fail
+++ b/tests/libvirtd-fail
@@ -5,12 +5,12 @@
if test "$VERBOSE" = yes; then
set -x
- $abs_top_builddir/daemon/libvirtd --version
+ $abs_top_builddir/src/libvirtd --version
fi
fail=0
-$abs_top_builddir/daemon/libvirtd --config=no-such-conf --timeout=5 2> log
+$abs_top_builddir/src/libvirtd --config=no-such-conf --timeout=5 2> log
RET=$?
test "$RET" != "0" && exit 0 || exit 1
--
2.16.4
7:11 a.m.
New subject: [libvirt] [PATCH v2 2/5] Forget last daemon/ dir artefacts
On 07/12/2018 03:37 AM, Michal Privoznik wrote:
The most important part is LIBVIRTD_PATH env var fix. It is used
in virFileFindResourceFull() from tests. The libvirtd no longer
lives under daemon/.
Then, libvirtd-fail test was still failing (as expected) but not
because of missing config file but because it was trying to
execute (nonexistent) top_builddir/daemon/libvirtd which
fulfilled expected outcome and thus test did not fail.
Thirdly, lcov was told to generate coverage for daemon/ dir too.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
Makefile.am | 2 +-
run.in | 2 +-
tests/libvirtd-fail | 4 ++--
3 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/Makefile.am b/Makefile.am
index 1926e21b7a..709064c6a6 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -80,7 +80,7 @@ check-access:
cov: clean-cov
$(MKDIR_P) $(top_builddir)/coverage
$(LCOV) -c -o $(top_builddir)/coverage/libvirt.info.tmp \
- -d $(top_builddir)/src -d $(top_builddir)/daemon \
+ -d $(top_builddir)/src \
Since daemon is the former name and this appears to be a clean label
target for coverage, perhaps we should keep daemon just to clean up
"old" trees...
The rest looks reasonable to me although Makefiles, run scripts, etc.
aren't my favorite things to look at 0-)
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
[...]
3:44 a.m.
New subject: [libvirt] [PATCH v2 2/5] Forget last daemon/ dir artefacts
On 07/21/2018 02:11 PM, John Ferlan wrote:
On 07/12/2018 03:37 AM, Michal Privoznik wrote:
> The most important part is LIBVIRTD_PATH env var fix. It is used
> in virFileFindResourceFull() from tests. The libvirtd no longer
> lives under daemon/.
>
> Then, libvirtd-fail test was still failing (as expected) but not
> because of missing config file but because it was trying to
> execute (nonexistent) top_builddir/daemon/libvirtd which
> fulfilled expected outcome and thus test did not fail.
>
> Thirdly, lcov was told to generate coverage for daemon/ dir too.
>
> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
> ---
> Makefile.am | 2 +-
> run.in | 2 +-
> tests/libvirtd-fail | 4 ++--
> 3 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/Makefile.am b/Makefile.am
> index 1926e21b7a..709064c6a6 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -80,7 +80,7 @@ check-access:
> cov: clean-cov
> $(MKDIR_P) $(top_builddir)/coverage
> $(LCOV) -c -o $(top_builddir)/coverage/libvirt.info.tmp \
> - -d $(top_builddir)/src -d $(top_builddir)/daemon \
> + -d $(top_builddir)/src \
Since daemon is the former name and this appears to be a clean label
target for coverage, perhaps we should keep daemon just to clean up
"old" trees...
No. This is no a clean label. The rule says: in order to make target
"cov" you need to make target "clean-cov" first as it is dependency.
So
I'm changing the create target not the cleanup. And -d $dir to lcov
means "include directory $dir to search for .da files" (whatever they
are - doesn't matter now).
Michal
9:02 a.m.
New subject: [libvirt] [PATCH v2 2/5] Forget last daemon/ dir artefacts
On 07/23/2018 04:44 AM, Michal Prívozník wrote:
On 07/21/2018 02:11 PM, John Ferlan wrote:
>
>
> On 07/12/2018 03:37 AM, Michal Privoznik wrote:
>> The most important part is LIBVIRTD_PATH env var fix. It is used
>> in virFileFindResourceFull() from tests. The libvirtd no longer
>> lives under daemon/.
>>
>> Then, libvirtd-fail test was still failing (as expected) but not
>> because of missing config file but because it was trying to
>> execute (nonexistent) top_builddir/daemon/libvirtd which
>> fulfilled expected outcome and thus test did not fail.
>>
>> Thirdly, lcov was told to generate coverage for daemon/ dir too.
>>
>> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
>> ---
>> Makefile.am | 2 +-
>> run.in | 2 +-
>> tests/libvirtd-fail | 4 ++--
>> 3 files changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/Makefile.am b/Makefile.am
>> index 1926e21b7a..709064c6a6 100644
>> --- a/Makefile.am
>> +++ b/Makefile.am
>> @@ -80,7 +80,7 @@ check-access:
>> cov: clean-cov
>> $(MKDIR_P) $(top_builddir)/coverage
>> $(LCOV) -c -o $(top_builddir)/coverage/libvirt.info.tmp \
>> - -d $(top_builddir)/src -d $(top_builddir)/daemon \
>> + -d $(top_builddir)/src \
>
> Since daemon is the former name and this appears to be a clean label
> target for coverage, perhaps we should keep daemon just to clean up
> "old" trees...
>
No. This is no a clean label. The rule says: in order to make target
"cov" you need to make target "clean-cov" first as it is dependency.
So
I'm changing the create target not the cleanup. And -d $dir to lcov
means "include directory $dir to search for .da files" (whatever they
are - doesn't matter now).
Michal
Hence the reason I don't like to review Makefile changes ;-)
John
2:37 a.m.
New subject: [libvirt] [PATCH v2 3/5] virtestmock: Track connect() too
The aim of this mock is to track if a test doesn't touch anything
in live system. Well, connect() which definitely falls into that
category isn't tracked yet.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
tests/virtestmock.c | 22 ++++++++++++++++++++++
1 file changed, 22 insertions(+)
diff --git a/tests/virtestmock.c b/tests/virtestmock.c
index 9b91adec77..654af24a10 100644
--- a/tests/virtestmock.c
+++ b/tests/virtestmock.c
@@ -27,6 +27,10 @@
#include <execinfo.h>
#include <sys/file.h>
#include <sys/stat.h>
+#include <sys/socket.h>
+#ifdef HAVE_SYS_UN_H
+# include <sys/un.h>
+#endif
#include "internal.h"
#include "configmake.h"
@@ -61,6 +65,7 @@ static int (*real_lstat)(const char *path, struct stat *sb);
static int (*real_lstat64)(const char *path, void *sb);
static int (*real___lxstat)(int ver, const char *path, struct stat *sb);
static int (*real___lxstat64)(int ver, const char *path, void *sb);
+static int (*real_connect)(int fd, const struct sockaddr *addr, socklen_t addrlen);
static const char *progname;
const char *output;
@@ -79,6 +84,7 @@ static void init_syms(void)
VIR_MOCK_REAL_INIT_ALT(stat64, __xstat64);
VIR_MOCK_REAL_INIT_ALT(lstat, __lxstat);
VIR_MOCK_REAL_INIT_ALT(lstat64, __lxstat64);
+ VIR_MOCK_REAL_INIT(connect);
}
static void
@@ -321,3 +327,19 @@ __lxstat64(int ver, const char *path, struct stat64 *sb)
return real___lxstat64(ver, path, sb);
}
#endif
+
+
+int connect(int sockfd, const struct sockaddr *addr, socklen_t addrlen)
+{
+ init_syms();
+
+#ifdef HAVE_SYS_UN_H
+ if (addrlen == sizeof(struct sockaddr_un)) {
+ struct sockaddr_un *tmp = (struct sockaddr_un *) addr;
+ if (tmp->sun_family == AF_UNIX)
+ checkPath(tmp->sun_path);
+ }
+#endif
+
+ return real_connect(sockfd, addr, addrlen);
+}
--
2.16.4
7:44 a.m.
New subject: [libvirt] [PATCH v2 3/5] virtestmock: Track connect() too
On 07/12/2018 03:37 AM, Michal Privoznik wrote:
The aim of this mock is to track if a test doesn't touch
anything
in live system. Well, connect() which definitely falls into that
category isn't tracked yet.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
tests/virtestmock.c | 22 ++++++++++++++++++++++
1 file changed, 22 insertions(+)
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
(the mock environment still has some mysteries for me ;-))
2:37 a.m.
New subject: [libvirt] [PATCH v2 4/5] check-file-access: Allow specifying action
The check-file-access.pl script is used to match access list
generated by virtestmock against whitelisted rules stored in
file_access_whitelist.txt. So far the rules are in form:
$path: $progname: $testname
This is not sufficient because the rule does not take into
account 'action' that caused $path to appear in the list of
accessed files. After this commit the rule can be in new form:
$path: $action: $progname: $testname
where $action is one from ("open", "fopen", "access",
"stat",
"lstat", "connect"). This way the white list can be fine tuned to
allow say access() but not connect().
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
tests/check-file-access.pl | 32 +++++++++++++++++++++++++++-----
tests/file_access_whitelist.txt | 15 ++++++++++-----
2 files changed, 37 insertions(+), 10 deletions(-)
diff --git a/tests/check-file-access.pl b/tests/check-file-access.pl
index 977a2bc533..ea0b7a18a2 100755
--- a/tests/check-file-access.pl
+++ b/tests/check-file-access.pl
@@ -27,18 +27,21 @@ use warnings;
my $access_file = "test_file_access.txt";
my $whitelist_file = "file_access_whitelist.txt";
+my @known_actions = ("open", "fopen", "access",
"stat", "lstat", "connect");
+
my @files;
my @whitelist;
open FILE, "<", $access_file or die "Unable to open $access_file:
$!";
while (<FILE>) {
chomp;
- if (/^(\S*):\s*(\S*)(\s*:\s*(.*))?$/) {
+ if (/^(\S*):\s*(\S*):\s*(\S*)(\s*:\s*(.*))?$/) {
my %rec;
${rec}{path} = $1;
- ${rec}{progname} = $2;
- if (defined $4) {
- ${rec}{testname} = $4;
+ ${rec}{action} = $2;
+ ${rec}{progname} = $3;
+ if (defined $5) {
+ ${rec}{testname} = $5;
}
push (@files, \%rec);
} else {
@@ -52,7 +55,21 @@ while (<FILE>) {
chomp;
if (/^\s*#.*$/) {
# comment
+ } elsif (/^(\S*):\s*(\S*)(:\s*(\S*)(\s*:\s*(.*))?)?$/ and
+ grep /^$2$/, @known_actions) {
+ # $path: $action: $progname: $testname
+ my %rec;
+ ${rec}{path} = $1;
+ ${rec}{action} = $3;
+ if (defined $4) {
+ ${rec}{progname} = $4;
+ }
+ if (defined $6) {
+ ${rec}{testname} = $6;
+ }
+ push (@whitelist, \%rec);
} elsif (/^(\S*)(:\s*(\S*)(\s*:\s*(.*))?)?$/) {
+ # $path: $progname: $testname
my %rec;
${rec}{path} = $1;
if (defined $3) {
@@ -79,6 +96,11 @@ for my $file (@files) {
next;
}
+ if (defined %${rule}{action} and
+ not %${file}{action} =~ m/^$rule->{action}$/) {
+ next;
+ }
+
if (defined %${rule}{progname} and
not %${file}{progname} =~ m/^$rule->{progname}$/) {
next;
@@ -95,7 +117,7 @@ for my $file (@files) {
if (not $match) {
$error = 1;
- print "$file->{path}: $file->{progname}";
+ print "$file->{path}: $file->{action}: $file->{progname}";
print ": $file->{testname}" if defined %${file}{testname};
print "\n";
}
diff --git a/tests/file_access_whitelist.txt b/tests/file_access_whitelist.txt
index 850b28506e..3fb318cbab 100644
--- a/tests/file_access_whitelist.txt
+++ b/tests/file_access_whitelist.txt
@@ -1,14 +1,17 @@
# This is a whitelist that allows accesses to files not in our
# build directory nor source directory. The records are in the
-# following format:
+# following formats:
#
# $path: $progname: $testname
+# $path: $action: $progname: $testname
#
-# All these three are evaluated as perl RE. So to allow /dev/sda
-# and /dev/sdb, you can just '/dev/sd[a-b]', or to allow
+# All these variables are evaluated as perl RE. So to allow
+# /dev/sda and /dev/sdb, you can just '/dev/sd[a-b]', or to allow
# /proc/$pid/status you can '/proc/\d+/status' and so on.
-# Moreover, $progname and $testname can be empty, in which which
-# case $path is allowed for all tests.
+# Moreover, $action, $progname and $testname can be empty, in which
+# which case $path is allowed for all tests. However, $action (if
+# specified) must be one of "open", "fopen", "access",
"stat",
+# "lstat", "connect".
/bin/cat: sysinfotest
/bin/dirname: sysinfotest: x86 sysinfo
@@ -19,5 +22,7 @@
/etc/hosts
/proc/\d+/status
+/etc/passwd: fopen
+
# This is just a dummy example, DO NOT USE IT LIKE THAT!
.*: nonexistent-test-touching-everything
--
2.16.4
7:57 a.m.
New subject: [libvirt] [PATCH v2 4/5] check-file-access: Allow specifying action
On 07/12/2018 03:37 AM, Michal Privoznik wrote:
The check-file-access.pl script is used to match access list
generated by virtestmock against whitelisted rules stored in
file_access_whitelist.txt. So far the rules are in form:
$path: $progname: $testname
This is not sufficient because the rule does not take into
account 'action' that caused $path to appear in the list of
accessed files. After this commit the rule can be in new form:
$path: $action: $progname: $testname
where $action is one from ("open", "fopen", "access",
"stat",
"lstat", "connect"). This way the white list can be fine tuned to
allow say access() but not connect().
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
tests/check-file-access.pl | 32 +++++++++++++++++++++++++++-----
tests/file_access_whitelist.txt | 15 ++++++++++-----
2 files changed, 37 insertions(+), 10 deletions(-)
Perl scripts, not my area of expertise... Even worse, regexes.
Still I am unclear about this particular one because you stuffed $action
in front of something that already existed and it makes me wonder how
that affects existing files. [1]
diff --git a/tests/check-file-access.pl b/tests/check-file-access.pl
index 977a2bc533..ea0b7a18a2 100755
--- a/tests/check-file-access.pl
+++ b/tests/check-file-access.pl
@@ -27,18 +27,21 @@ use warnings;
my $access_file = "test_file_access.txt";
my $whitelist_file = "file_access_whitelist.txt";
+my @known_actions = ("open", "fopen", "access",
"stat", "lstat", "connect");
+
my @files;
my @whitelist;
open FILE, "<", $access_file or die "Unable to open $access_file:
$!";
while (<FILE>) {
chomp;
- if (/^(\S*):\s*(\S*)(\s*:\s*(.*))?$/) {
+ if (/^(\S*):\s*(\S*):\s*(\S*)(\s*:\s*(.*))?$/) {
my %rec;
${rec}{path} = $1;
- ${rec}{progname} = $2;
- if (defined $4) {
- ${rec}{testname} = $4;
+ ${rec}{action} = $2;
+ ${rec}{progname} = $3;
+ if (defined $5) {
+ ${rec}{testname} = $5;
}
push (@files, \%rec);
} else {
@@ -52,7 +55,21 @@ while (<FILE>) {
chomp;
if (/^\s*#.*$/) {
# comment
+ } elsif (/^(\S*):\s*(\S*)(:\s*(\S*)(\s*:\s*(.*))?)?$/ and
+ grep /^$2$/, @known_actions) {
+ # $path: $action: $progname: $testname
+ my %rec;
+ ${rec}{path} = $1;
+ ${rec}{action} = $3;
+ if (defined $4) {
+ ${rec}{progname} = $4;
+ }
+ if (defined $6) {
+ ${rec}{testname} = $6;
+ }
+ push (@whitelist, \%rec);
} elsif (/^(\S*)(:\s*(\S*)(\s*:\s*(.*))?)?$/) {
+ # $path: $progname: $testname
my %rec;
${rec}{path} = $1;
if (defined $3) {
@@ -79,6 +96,11 @@ for my $file (@files) {
next;
}
+ if (defined %${rule}{action} and
+ not %${file}{action} =~ m/^$rule->{action}$/) {
+ next;
+ }
+
if (defined %${rule}{progname} and
not %${file}{progname} =~ m/^$rule->{progname}$/) {
next;
@@ -95,7 +117,7 @@ for my $file (@files) {
if (not $match) {
$error = 1;
- print "$file->{path}: $file->{progname}";
+ print "$file->{path}: $file->{action}: $file->{progname}";
print ": $file->{testname}" if defined %${file}{testname};
print "\n";
}
diff --git a/tests/file_access_whitelist.txt b/tests/file_access_whitelist.txt
index 850b28506e..3fb318cbab 100644
--- a/tests/file_access_whitelist.txt
+++ b/tests/file_access_whitelist.txt
@@ -1,14 +1,17 @@
# This is a whitelist that allows accesses to files not in our
# build directory nor source directory. The records are in the
-# following format:
+# following formats:
#
# $path: $progname: $testname
+# $path: $action: $progname: $testname
#
-# All these three are evaluated as perl RE. So to allow /dev/sda
-# and /dev/sdb, you can just '/dev/sd[a-b]', or to allow
+# All these variables are evaluated as perl RE. So to allow
+# /dev/sda and /dev/sdb, you can just '/dev/sd[a-b]', or to allow
# /proc/$pid/status you can '/proc/\d+/status' and so on.
-# Moreover, $progname and $testname can be empty, in which which
-# case $path is allowed for all tests.
+# Moreover, $action, $progname and $testname can be empty, in which
+# which case $path is allowed for all tests. However, $action (if
+# specified) must be one of "open", "fopen", "access",
"stat",
+# "lstat", "connect".
/bin/cat: sysinfotest
/bin/dirname: sysinfotest: x86 sysinfo
[1] For example, why wouldn't sysinfotest in this case relate to $action
instead of $progname?
Are things read backwards - I just don't feel comfortable enough about
this code to review in much depth. Since patch 5 is based upon this
change, I'll also stop here.
John
BTW: I have to say the output in patch5 at least explains something else
I saw as a result of this series that I couldn't quite explain. I recall
seeing the message and wondering, WTF it was trying to tell me.
@@ -19,5 +22,7 @@
/etc/hosts
/proc/\d+/status
+/etc/passwd: fopen
+
# This is just a dummy example, DO NOT USE IT LIKE THAT!
.*: nonexistent-test-touching-everything
3:44 a.m.
New subject: [libvirt] [PATCH v2 4/5] check-file-access: Allow specifying action
On 07/21/2018 02:57 PM, John Ferlan wrote:
On 07/12/2018 03:37 AM, Michal Privoznik wrote:
> The check-file-access.pl script is used to match access list
> generated by virtestmock against whitelisted rules stored in
> file_access_whitelist.txt. So far the rules are in form:
>
> $path: $progname: $testname
>
> This is not sufficient because the rule does not take into
> account 'action' that caused $path to appear in the list of
> accessed files. After this commit the rule can be in new form:
>
> $path: $action: $progname: $testname
>
> where $action is one from ("open", "fopen", "access",
"stat",
> "lstat", "connect"). This way the white list can be fine tuned
to
> allow say access() but not connect().
>
> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
> ---
> tests/check-file-access.pl | 32 +++++++++++++++++++++++++++-----
> tests/file_access_whitelist.txt | 15 ++++++++++-----
> 2 files changed, 37 insertions(+), 10 deletions(-)
>
Perl scripts, not my area of expertise... Even worse, regexes.
Still I am unclear about this particular one because you stuffed $action
in front of something that already existed and it makes me wonder how
that affects existing files. [1]
> diff --git a/tests/check-file-access.pl b/tests/check-file-access.pl
> index 977a2bc533..ea0b7a18a2 100755
> --- a/tests/check-file-access.pl
> +++ b/tests/check-file-access.pl
> @@ -27,18 +27,21 @@ use warnings;
> my $access_file = "test_file_access.txt";
> my $whitelist_file = "file_access_whitelist.txt";
>
> +my @known_actions = ("open", "fopen", "access",
"stat", "lstat", "connect");
> +
> my @files;
> my @whitelist;
>
> open FILE, "<", $access_file or die "Unable to open $access_file:
$!";
> while (<FILE>) {
> chomp;
> - if (/^(\S*):\s*(\S*)(\s*:\s*(.*))?$/) {
> + if (/^(\S*):\s*(\S*):\s*(\S*)(\s*:\s*(.*))?$/) {
> my %rec;
> ${rec}{path} = $1;
> - ${rec}{progname} = $2;
> - if (defined $4) {
> - ${rec}{testname} = $4;
> + ${rec}{action} = $2;
> + ${rec}{progname} = $3;
> + if (defined $5) {
> + ${rec}{testname} = $5;
> }
> push (@files, \%rec);
> } else {
> @@ -52,7 +55,21 @@ while (<FILE>) {
> chomp;
> if (/^\s*#.*$/) {
> # comment
> + } elsif (/^(\S*):\s*(\S*)(:\s*(\S*)(\s*:\s*(.*))?)?$/ and
> + grep /^$2$/, @known_actions) {
> + # $path: $action: $progname: $testname
> + my %rec;
> + ${rec}{path} = $1;
> + ${rec}{action} = $3;
> + if (defined $4) {
> + ${rec}{progname} = $4;
> + }
> + if (defined $6) {
> + ${rec}{testname} = $6;
> + }
> + push (@whitelist, \%rec);
> } elsif (/^(\S*)(:\s*(\S*)(\s*:\s*(.*))?)?$/) {
> + # $path: $progname: $testname
> my %rec;
> ${rec}{path} = $1;
> if (defined $3) {
> @@ -79,6 +96,11 @@ for my $file (@files) {
> next;
> }
>
> + if (defined %${rule}{action} and
> + not %${file}{action} =~ m/^$rule->{action}$/) {
> + next;
> + }
> +
> if (defined %${rule}{progname} and
> not %${file}{progname} =~ m/^$rule->{progname}$/) {
> next;
> @@ -95,7 +117,7 @@ for my $file (@files) {
>
> if (not $match) {
> $error = 1;
> - print "$file->{path}: $file->{progname}";
> + print "$file->{path}: $file->{action}:
$file->{progname}";
> print ": $file->{testname}" if defined %${file}{testname};
> print "\n";
> }
> diff --git a/tests/file_access_whitelist.txt b/tests/file_access_whitelist.txt
> index 850b28506e..3fb318cbab 100644
> --- a/tests/file_access_whitelist.txt
> +++ b/tests/file_access_whitelist.txt
> @@ -1,14 +1,17 @@
> # This is a whitelist that allows accesses to files not in our
> # build directory nor source directory. The records are in the
> -# following format:
> +# following formats:
> #
> # $path: $progname: $testname
> +# $path: $action: $progname: $testname
> #
> -# All these three are evaluated as perl RE. So to allow /dev/sda
> -# and /dev/sdb, you can just '/dev/sd[a-b]', or to allow
> +# All these variables are evaluated as perl RE. So to allow
> +# /dev/sda and /dev/sdb, you can just '/dev/sd[a-b]', or to allow
> # /proc/$pid/status you can '/proc/\d+/status' and so on.
> -# Moreover, $progname and $testname can be empty, in which which
> -# case $path is allowed for all tests.
> +# Moreover, $action, $progname and $testname can be empty, in which
> +# which case $path is allowed for all tests. However, $action (if
> +# specified) must be one of "open", "fopen", "access",
"stat",
> +# "lstat", "connect".
>
> /bin/cat: sysinfotest
> /bin/dirname: sysinfotest: x86 sysinfo
[1] For example, why wouldn't sysinfotest in this case relate to $action
instead of $progname?
Because "sysinfotest" is not one of the array ("open",
"fopen",
"access", ...)
The idea is to have two sets of rules:
/some/path: program
/some/path: open: program
The former allows just ANY access to /some/path for program "program",
i.e. it can open, fopen, access, stat, lstat, connect to it.
The latter allows only one action over /some/path. For instance
open("/some/path") is allowed but stat("/some/path") isn't.
To complicate things a bit more, if I wanted to allow /some/path for
every program, instead of having one rule per each program I can only have:
/some/path
/some/path: open
The former allows ANY access to /some/path for ANY program, the latter
allows open("/some/path") for ANY program but disallows other types of
access. Joining types of access is not supported yet. For instance:
/some/path: open stat
has to be written as:
/some/path: open
/some/path: stat
And when I say allowed what I really mean is: reported by 'make
check-access'.
True, we are missing a lot of rules there. But I have an idea how to
fill them out ;-)
Michal
4:01 p.m.
New subject: [libvirt] [PATCH v2 4/5] check-file-access: Allow specifying action
On 07/23/2018 04:44 AM, Michal Prívozník wrote:
On 07/21/2018 02:57 PM, John Ferlan wrote:
>
>
> On 07/12/2018 03:37 AM, Michal Privoznik wrote:
>> The check-file-access.pl script is used to match access list
>> generated by virtestmock against whitelisted rules stored in
>> file_access_whitelist.txt. So far the rules are in form:
>>
>> $path: $progname: $testname
>>
>> This is not sufficient because the rule does not take into
>> account 'action' that caused $path to appear in the list of
>> accessed files. After this commit the rule can be in new form:
>>
>> $path: $action: $progname: $testname
>>
>> where $action is one from ("open", "fopen",
"access", "stat",
>> "lstat", "connect"). This way the white list can be fine
tuned to
>> allow say access() but not connect().
>>
>> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
>> ---
>> tests/check-file-access.pl | 32 +++++++++++++++++++++++++++-----
>> tests/file_access_whitelist.txt | 15 ++++++++++-----
>> 2 files changed, 37 insertions(+), 10 deletions(-)
>>
>
> Perl scripts, not my area of expertise... Even worse, regexes.
>
> Still I am unclear about this particular one because you stuffed $action
> in front of something that already existed and it makes me wonder how
> that affects existing files. [1]
>
>
>> diff --git a/tests/check-file-access.pl b/tests/check-file-access.pl
>> index 977a2bc533..ea0b7a18a2 100755
>> --- a/tests/check-file-access.pl
>> +++ b/tests/check-file-access.pl
>> @@ -27,18 +27,21 @@ use warnings;
>> my $access_file = "test_file_access.txt";
>> my $whitelist_file = "file_access_whitelist.txt";
>>
>> +my @known_actions = ("open", "fopen", "access",
"stat", "lstat", "connect");
>> +
>> my @files;
>> my @whitelist;
>>
>> open FILE, "<", $access_file or die "Unable to open
$access_file: $!";
>> while (<FILE>) {
>> chomp;
>> - if (/^(\S*):\s*(\S*)(\s*:\s*(.*))?$/) {
>> + if (/^(\S*):\s*(\S*):\s*(\S*)(\s*:\s*(.*))?$/) {
>> my %rec;
>> ${rec}{path} = $1;
>> - ${rec}{progname} = $2;
>> - if (defined $4) {
>> - ${rec}{testname} = $4;
>> + ${rec}{action} = $2;
>> + ${rec}{progname} = $3;
>> + if (defined $5) {
>> + ${rec}{testname} = $5;
>> }
>> push (@files, \%rec);
>> } else {
>> @@ -52,7 +55,21 @@ while (<FILE>) {
>> chomp;
>> if (/^\s*#.*$/) {
>> # comment
>> + } elsif (/^(\S*):\s*(\S*)(:\s*(\S*)(\s*:\s*(.*))?)?$/ and
>> + grep /^$2$/, @known_actions) {
>> + # $path: $action: $progname: $testname
>> + my %rec;
>> + ${rec}{path} = $1;
>> + ${rec}{action} = $3;
>> + if (defined $4) {
>> + ${rec}{progname} = $4;
>> + }
>> + if (defined $6) {
>> + ${rec}{testname} = $6;
>> + }
>> + push (@whitelist, \%rec);
>> } elsif (/^(\S*)(:\s*(\S*)(\s*:\s*(.*))?)?$/) {
>> + # $path: $progname: $testname
>> my %rec;
>> ${rec}{path} = $1;
>> if (defined $3) {
>> @@ -79,6 +96,11 @@ for my $file (@files) {
>> next;
>> }
>>
>> + if (defined %${rule}{action} and
>> + not %${file}{action} =~ m/^$rule->{action}$/) {
>> + next;
>> + }
>> +
>> if (defined %${rule}{progname} and
>> not %${file}{progname} =~ m/^$rule->{progname}$/) {
>> next;
>> @@ -95,7 +117,7 @@ for my $file (@files) {
>>
>> if (not $match) {
>> $error = 1;
>> - print "$file->{path}: $file->{progname}";
>> + print "$file->{path}: $file->{action}:
$file->{progname}";
>> print ": $file->{testname}" if defined %${file}{testname};
>> print "\n";
>> }
>> diff --git a/tests/file_access_whitelist.txt b/tests/file_access_whitelist.txt
>> index 850b28506e..3fb318cbab 100644
>> --- a/tests/file_access_whitelist.txt
>> +++ b/tests/file_access_whitelist.txt
>> @@ -1,14 +1,17 @@
>> # This is a whitelist that allows accesses to files not in our
>> # build directory nor source directory. The records are in the
>> -# following format:
>> +# following formats:
>> #
>> # $path: $progname: $testname
>> +# $path: $action: $progname: $testname
>> #
>> -# All these three are evaluated as perl RE. So to allow /dev/sda
>> -# and /dev/sdb, you can just '/dev/sd[a-b]', or to allow
>> +# All these variables are evaluated as perl RE. So to allow
>> +# /dev/sda and /dev/sdb, you can just '/dev/sd[a-b]', or to allow
>> # /proc/$pid/status you can '/proc/\d+/status' and so on.
>> -# Moreover, $progname and $testname can be empty, in which which
>> -# case $path is allowed for all tests.
>> +# Moreover, $action, $progname and $testname can be empty, in which
>> +# which case $path is allowed for all tests. However, $action (if
>> +# specified) must be one of "open", "fopen",
"access", "stat",
>> +# "lstat", "connect".
>>
>> /bin/cat: sysinfotest
>> /bin/dirname: sysinfotest: x86 sysinfo
>
> [1] For example, why wouldn't sysinfotest in this case relate to $action
> instead of $progname?
Because "sysinfotest" is not one of the array ("open",
"fopen",
"access", ...)
Well, thanks for the missing details... Guess I could have read the last
sentence of the comment a lot more closely or perhaps maybe the whole
thing needs to be more obvious as to what the options are.
So maybe rather than:
# following formats:
#
# $path: $progname: $testname
# $path: $action: $progname: $testname
it's:
# following two accepted formats:
#
# $path: $progname: $testname
# or
# $path: $action: $progname: $testname
...
and
Rather than
# Moreover, $action, $progname and $testname can be empty, in which
# which case $path is allowed for all tests. However, $action (if
# specified) must be one of "open", "fopen", "access",
"stat",
# "lstat", "connect".
it's:
# Moreover, $action, $progname and $testname can be empty, in which
# case $path is allowed for all tests.
#
# If $action is specified, then it must be one of "open", "fopen",
# "access", "stat", "lstat", or "connect" followed
by $progname and
# $testname.
#
# The $progname and $testname specify...
[BTW: explained much better than I can even attempt to retry below and I
think perfectly reasonable to put this entirely in the file].
The idea is to have two sets of rules:
/some/path: program
/some/path: open: program
The former allows just ANY access to /some/path for program "program",
i.e. it can open, fopen, access, stat, lstat, connect to it.
The latter allows only one action over /some/path. For instance
open("/some/path") is allowed but stat("/some/path") isn't.
To complicate things a bit more, if I wanted to allow /some/path for
every program, instead of having one rule per each program I can only have:
/some/path
/some/path: open
The former allows ANY access to /some/path for ANY program, the latter
allows open("/some/path") for ANY program but disallows other types of
access. Joining types of access is not supported yet. For instance:
/some/path: open stat
has to be written as:
/some/path: open
/some/path: stat
And when I say allowed what I really mean is: reported by 'make
check-access'.
Huh, hmm, say what? Interesting. I've learned something new I think.
I'm good for the week now, right?
True, we are missing a lot of rules there. But I have an idea how to
fill them out ;-)
And as as noted previously, my scripting review is not really that good
and throw in regex, and well I'm out of my comfort zone. Still using the
"pattern" from the previous code for "$path: $progname: $testname"
where
$1 is $path, $3 is $progname, and $5 is testname, what I don't
understand then is why the above for "$path: $action: $progname:
$testname" has $1 for $path, $3 for $action, $4 for $progname, and $6
for $testname. Speaking only from the "pattern" POV I would have though
$5 for $progname and $7 for $testname.
Similarly for the other file it was $1, $2, $4 and now it's $1, $2, $3,
and $5.
But beyond adding some doc wording, that's about as good as it gets from
me for any sort of perl script... sorry I cannot be more help.
John
2:37 a.m.
New subject: [libvirt] [PATCH v2 5/5] virtestmock: Track action
As advertised in the previous commit, we need the list of
accessed files to also contain action that caused the $path to
appear on the list. Not only this enables us to fine tune our
white list rules it also helps us to see why $path is reported.
For instance:
/run/user/1000/libvirt/libvirt-sock: connect: qemuxml2argvtest: QEMU XML-2-ARGV
net-vhostuser-multiq
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
tests/virtestmock.c | 39 ++++++++++++++++++++++-----------------
1 file changed, 22 insertions(+), 17 deletions(-)
diff --git a/tests/virtestmock.c b/tests/virtestmock.c
index 654af24a10..25aadf8aea 100644
--- a/tests/virtestmock.c
+++ b/tests/virtestmock.c
@@ -88,7 +88,8 @@ static void init_syms(void)
}
static void
-printFile(const char *file)
+printFile(const char *file,
+ const char *func)
{
FILE *fp;
const char *testname = getenv("VIR_TEST_MOCK_TESTNAME");
@@ -116,9 +117,9 @@ printFile(const char *file)
}
/* Now append the following line into the output file:
- * $file: $progname $testname */
+ * $file: $progname: $func: $testname */
- fprintf(fp, "%s: %s", file, progname);
+ fprintf(fp, "%s: %s: %s", file, func, progname);
if (testname)
fprintf(fp, ": %s", testname);
@@ -128,8 +129,12 @@ printFile(const char *file)
fclose(fp);
}
+#define CHECK_PATH(path) \
+ checkPath(path, __FUNCTION__)
+
static void
-checkPath(const char *path)
+checkPath(const char *path,
+ const char *func)
{
char *fullPath = NULL;
char *relPath = NULL;
@@ -160,7 +165,7 @@ checkPath(const char *path)
if (!STRPREFIX(path, abs_topsrcdir) &&
!STRPREFIX(path, abs_topbuilddir)) {
- printFile(path);
+ printFile(path, func);
}
VIR_FREE(crippledPath);
@@ -180,7 +185,7 @@ int open(const char *path, int flags, ...)
init_syms();
- checkPath(path);
+ CHECK_PATH(path);
if (flags & O_CREAT) {
va_list ap;
@@ -199,7 +204,7 @@ FILE *fopen(const char *path, const char *mode)
{
init_syms();
- checkPath(path);
+ CHECK_PATH(path);
return real_fopen(path, mode);
}
@@ -209,7 +214,7 @@ int access(const char *path, int mode)
{
init_syms();
- checkPath(path);
+ CHECK_PATH(path);
return real_access(path, mode);
}
@@ -239,7 +244,7 @@ int stat(const char *path, struct stat *sb)
{
init_syms();
- checkPath(path);
+ checkPath(path, "stat");
return real_stat(path, sb);
}
@@ -250,7 +255,7 @@ int stat64(const char *path, struct stat64 *sb)
{
init_syms();
- checkPath(path);
+ checkPath(path, "stat");
return real_stat64(path, sb);
}
@@ -262,7 +267,7 @@ __xstat(int ver, const char *path, struct stat *sb)
{
init_syms();
- checkPath(path);
+ checkPath(path, "stat");
return real___xstat(ver, path, sb);
}
@@ -274,7 +279,7 @@ __xstat64(int ver, const char *path, struct stat64 *sb)
{
init_syms();
- checkPath(path);
+ checkPath(path, "stat");
return real___xstat64(ver, path, sb);
}
@@ -286,7 +291,7 @@ lstat(const char *path, struct stat *sb)
{
init_syms();
- checkPath(path);
+ checkPath(path, "lstat");
return real_lstat(path, sb);
}
@@ -298,7 +303,7 @@ lstat64(const char *path, struct stat64 *sb)
{
init_syms();
- checkPath(path);
+ checkPath(path, "lstat");
return real_lstat64(path, sb);
}
@@ -310,7 +315,7 @@ __lxstat(int ver, const char *path, struct stat *sb)
{
init_syms();
- checkPath(path);
+ checkPath(path, "lstat");
return real___lxstat(ver, path, sb);
}
@@ -322,7 +327,7 @@ __lxstat64(int ver, const char *path, struct stat64 *sb)
{
init_syms();
- checkPath(path);
+ checkPath(path, "lstat");
return real___lxstat64(ver, path, sb);
}
@@ -337,7 +342,7 @@ int connect(int sockfd, const struct sockaddr *addr, socklen_t
addrlen)
if (addrlen == sizeof(struct sockaddr_un)) {
struct sockaddr_un *tmp = (struct sockaddr_un *) addr;
if (tmp->sun_family == AF_UNIX)
- checkPath(tmp->sun_path);
+ CHECK_PATH(tmp->sun_path);
}
#endif
--
2.16.4
2345
days inactive
2356
days old
19 comments
4 participants
participants (4)
-
John Ferlan -
Michal Privoznik -
Michal Prívozník
-
Peter Krempa