10:54 a.m.
On 6/11/20 4:32 PM, Erik Skultety wrote:
On Thu, Jun 11, 2020 at 04:15:56PM +0200, Michal Privoznik wrote:
> On 6/11/20 3:40 PM, Erik Skultety wrote:
>> On Wed, Jun 10, 2020 at 08:01:47PM +0200, Michal Privoznik wrote:
>>> On 6/9/20 11:43 PM, Jonathon Jongsma wrote:
>>>> Test that we run 'mdevctl' with the proper arguments when
creating new
>>>> mediated devices with virNodeDeviceCreateXML().
>>>>
>>>> Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com>
>>>> ---
>>>> build-aux/syntax-check.mk | 2 +-
>>>> tests/Makefile.am | 14 +
>>>> ...019_36ea_4111_8f0a_8c9a70e21366-start.argv | 1 +
>>>> ...019_36ea_4111_8f0a_8c9a70e21366-start.json | 1 +
>>>> ...d39_495e_4243_ad9f_beb3f14c23d9-start.argv | 1 +
>>>> ...d39_495e_4243_ad9f_beb3f14c23d9-start.json | 1 +
>>>> ...916_1ca8_49ac_b176_871d16c13076-start.argv | 1 +
>>>> ...916_1ca8_49ac_b176_871d16c13076-start.json | 1 +
>>>> tests/nodedevmdevctltest.c | 257
++++++++++++++++++
>>>> ...v_d069d019_36ea_4111_8f0a_8c9a70e21366.xml | 8 +
>>>> ...v_d2441d39_495e_4243_ad9f_beb3f14c23d9.xml | 10 +
>>>> ...v_fedc4916_1ca8_49ac_b176_871d16c13076.xml | 9 +
>>>> 12 files changed, 305 insertions(+), 1 deletion(-)
>>>> create mode 100644
tests/nodedevmdevctldata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366-start.argv
>>>> create mode 100644
tests/nodedevmdevctldata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366-start.json
>>>> create mode 100644
tests/nodedevmdevctldata/mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9-start.argv
>>>> create mode 100644
tests/nodedevmdevctldata/mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9-start.json
>>>> create mode 100644
tests/nodedevmdevctldata/mdev_fedc4916_1ca8_49ac_b176_871d16c13076-start.argv
>>>> create mode 100644
tests/nodedevmdevctldata/mdev_fedc4916_1ca8_49ac_b176_871d16c13076-start.json
>>>> create mode 100644 tests/nodedevmdevctltest.c
>>>> create mode 100644
tests/nodedevschemadata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366.xml
>>>> create mode 100644
tests/nodedevschemadata/mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9.xml
>>>> create mode 100644
tests/nodedevschemadata/mdev_fedc4916_1ca8_49ac_b176_871d16c13076.xml
>>>>
>>>> diff --git a/build-aux/syntax-check.mk b/build-aux/syntax-check.mk
>>>> index bf8832a2a5..d47a92b530 100644
>>>> --- a/build-aux/syntax-check.mk
>>>> +++ b/build-aux/syntax-check.mk
>>>> @@ -2015,7 +2015,7 @@ exclude_file_name_regexp--sc_prohibit_close = \
>>>>
(\.p[yl]$$|\.spec\.in$$|^docs/|^(src/util/vir(file|event)\.c|src/libvirt-stream\.c|tests/(vir.+mock\.c|commandhelper\.c|qemusecuritymock\.c)|tools/nss/libvirt_nss_(leases|macs)\.c)$$)
>>>> exclude_file_name_regexp--sc_prohibit_empty_lines_at_EOF = \
>>>> -
(^tests/(virhostcpu|virpcitest)data/|docs/js/.*\.js|docs/fonts/.*\.woff|\.diff|tests/virconfdata/no-newline\.conf$$)
>>>> +
(^tests/(nodedevmdevctl|virhostcpu|virpcitest)data/|docs/js/.*\.js|docs/fonts/.*\.woff|\.diff|tests/virconfdata/no-newline\.conf$$)
>>>> exclude_file_name_regexp--sc_prohibit_fork_wrappers = \
>>>>
(^(src/(util/(vircommand|virdaemon)|lxc/lxc_controller)|tests/testutils)\.c$$)
>>>> diff --git a/tests/Makefile.am b/tests/Makefile.am
>>>> index f5766a7790..13cbdbb31e 100644
>>>> --- a/tests/Makefile.am
>>>> +++ b/tests/Makefile.am
>>>> @@ -388,6 +388,10 @@ test_programs += storagevolxml2xmltest
>>>> test_programs += nodedevxml2xmltest
>>>> +if WITH_NODE_DEVICES
>>>> +test_programs += nodedevmdevctltest
>>>> +endif WITH_NODE_DEVICES
>>>> +
>>>> test_programs += interfacexml2xmltest
>>>> test_programs += cputest
>>>> @@ -970,6 +974,16 @@ nodedevxml2xmltest_SOURCES = \
>>>> testutils.c testutils.h
>>>> nodedevxml2xmltest_LDADD = $(LDADDS)
>>>> +if WITH_NODE_DEVICES
>>>> +nodedevmdevctltest_SOURCES = \
>>>> + nodedevmdevctltest.c \
>>>> + testutils.c testutils.h
>>>> +
>>>> +nodedevmdevctltest_LDADD = \
>>>> + ../src/libvirt_driver_nodedev_impl.la \
>>>> + $(LDADDS)
>>>> +endif WITH_NODE_DEVICES
>>>> +
>>>> interfacexml2xmltest_SOURCES = \
>>>> interfacexml2xmltest.c \
>>>> testutils.c testutils.h
>>>> diff --git
a/tests/nodedevmdevctldata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366-start.argv
b/tests/nodedevmdevctldata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366-start.argv
>>>> new file mode 100644
>>>> index 0000000000..dae6dedf7f
>>>> --- /dev/null
>>>> +++
b/tests/nodedevmdevctldata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366-start.argv
>>>> @@ -0,0 +1 @@
>>>> +/usr/sbin/mdevctl start -p 0000:00:02.0 --jsonfile /dev/stdin
>>>
>>> This is a problem. If there is no mdevctl found during configure (my case),
>>> then MDEVCTL is going to be plain "mdevctl". Which is okay for the
nodedev
>>> driver, because it uses virCommandRun() which uses virFindFileInPath() to
>>> get the real path at runtime. But for tests it won't fly. Alternativelly,
if
>>> the mdevctl lived under say /bin or any other location than /usr/sbin the
>>> expected output would be different.
>>>
>>> In virnetdevbandwidthtest.c (which uses TC) I've worked around this by
using
>>> TC directly to construct expected output. However, I guess we can safely
>>> assume that either mdevctl is present at build time so that its location is
>>
>> Why can we assume it safely? config.log doesn't complain and since we
didn't
>> put this in a BuildRequires stanza, even if you do dnf builddep, you won't
get
>> the program. Note that the situation with TC is different because (if I
>> overlook gentoo), it is provided by iproute-tc on Fedora and iproute2 on Debian
>> which are installed by default, so TC will be always present. I don't
>> think we should silently assume anything here, I believe configure should
>> complain because otherwise the tests will be failing and the developer would
>> not know why and it would also be confusing since our gitlab pipelines would be
>> green.
>
> Well, is iproute installed even for minimal installation? Because fedora
> builders certainly don't have it:
>
>
https://kojipkgs.fedoraproject.org//packages/libvirt/6.1.0/4.fc32/data/lo...
>
> checking for radvd... /usr/sbin/radvd
> checking for tc... tc
Hmm, good point, I had minimal installed, but I also installed the @core
package group which apparently has iproute-tc installed.
>
> (if tc was found then the full path would be printed, like it is for radvd)
>
> and we certainly don't want to fail configure if mdevctl wasn't found. What
> we can do then, is to generate the expected cmd line on the fly - just like
I guess we could go with ^this, but why do we have to "mock" it this way just
to let configure pass if this is a build requirement? To me it looks like we're
making it unnecessarily complex.
Configure and spec file are two different things. If the spec file
declares something as build dependency it doesn't have to be that way on
other distributions. Not to mention other OSes like FreeBSD where I bet
is no notion of mediated devices, or when somebody wants to build just
the client (e.g. mingw). Requiring mdevctl during configure wouldn't be
step in the right direction then.
Speaking of build requirements, what is your opinion on adding (well
goes the
same for TC apparently) BuildRequires with mdevctl to the SPEC file?
That might be a good step, but if we discard my suggestion to change
m4/virt-external-programs.m4 and keep what Jonathon proposed, then I
don't see a need for that. Configure will not hardcode any path to
mdevctl and virCommandRun() will then use virFindFileInPath() to
determine its location at runtime (in virExec()).
In the end, I guess we should do what virnetdevbandwidthtest does and
use MDEVCTL macro to construct expected output. Then, whatever value the
macro has (whatever path mdevctl is found under), the expected output
will have the same right one.
Michal