[libvirt PATCH v2 07/10] nodedev: Add testing for 'mdevctl start'

Michal Privoznik mprivozn at redhat.com
Wed Jun 10 18:01:47 UTC 2020


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 at 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 detected properly, or we hardcode its path. IOW, this needs 
to be squashed into 05/10:


diff --git i/m4/virt-external-programs.m4 w/m4/virt-external-programs.m4
index bd3cb1f757..f642dcbf0e 100644
--- i/m4/virt-external-programs.m4
+++ w/m4/virt-external-programs.m4
@@ -65,7 +65,7 @@ AC_DEFUN([LIBVIRT_CHECK_EXTERNAL_PROGRAMS], [
    AC_PATH_PROG([OVSVSCTL], [ovs-vsctl], [ovs-vsctl], [$LIBVIRT_SBIN_PATH])
    AC_PATH_PROG([SCRUB], [scrub], [scrub], [$LIBVIRT_SBIN_PATH])
    AC_PATH_PROG([ADDR2LINE], [addr2line], [addr2line], 
[$LIBVIRT_SBIN_PATH])
-  AC_PATH_PROG([MDEVCTL], [mdevctl], [mdevctl], [$LIBVIRT_SBIN_PATH])
+  AC_PATH_PROG([MDEVCTL], [mdevctl], [/usr/sbin/mdevctl], 
[$LIBVIRT_SBIN_PATH])

    AC_DEFINE_UNQUOTED([DMIDECODE], ["$DMIDECODE"],
                       [Location or name of the dmidecode program])


> diff --git a/tests/nodedevmdevctldata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366-start.json b/tests/nodedevmdevctldata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366-start.json
> new file mode 100644
> index 0000000000..bfc6dcace3
> --- /dev/null
> +++ b/tests/nodedevmdevctldata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366-start.json
> @@ -0,0 +1 @@
> +{"mdev_type":"i915-GVTg_V5_8","start":"manual"}
> \ No newline at end of file
> diff --git a/tests/nodedevmdevctldata/mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9-start.argv b/tests/nodedevmdevctldata/mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9-start.argv
> new file mode 100644
> index 0000000000..dae6dedf7f
> --- /dev/null
> +++ b/tests/nodedevmdevctldata/mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9-start.argv
> @@ -0,0 +1 @@
> +/usr/sbin/mdevctl start -p 0000:00:02.0 --jsonfile /dev/stdin
> diff --git a/tests/nodedevmdevctldata/mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9-start.json b/tests/nodedevmdevctldata/mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9-start.json
> new file mode 100644
> index 0000000000..e5b22b2c44
> --- /dev/null
> +++ b/tests/nodedevmdevctldata/mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9-start.json
> @@ -0,0 +1 @@
> +{"mdev_type":"i915-GVTg_V5_8","start":"manual","attrs":[{"example-attribute-1":"attribute-value-1"},{"example-attribute-2":"attribute-value-2"}]}
> \ No newline at end of file
> diff --git a/tests/nodedevmdevctldata/mdev_fedc4916_1ca8_49ac_b176_871d16c13076-start.argv b/tests/nodedevmdevctldata/mdev_fedc4916_1ca8_49ac_b176_871d16c13076-start.argv
> new file mode 100644
> index 0000000000..dae6dedf7f
> --- /dev/null
> +++ b/tests/nodedevmdevctldata/mdev_fedc4916_1ca8_49ac_b176_871d16c13076-start.argv
> @@ -0,0 +1 @@
> +/usr/sbin/mdevctl start -p 0000:00:02.0 --jsonfile /dev/stdin
> diff --git a/tests/nodedevmdevctldata/mdev_fedc4916_1ca8_49ac_b176_871d16c13076-start.json b/tests/nodedevmdevctldata/mdev_fedc4916_1ca8_49ac_b176_871d16c13076-start.json
> new file mode 100644
> index 0000000000..2e03d0bd8e
> --- /dev/null
> +++ b/tests/nodedevmdevctldata/mdev_fedc4916_1ca8_49ac_b176_871d16c13076-start.json
> @@ -0,0 +1 @@
> +{"mdev_type":"i915-GVTg_V5_8","start":"manual","attrs":[{"example-attribute":"attribute-value"}]}
> \ No newline at end of file
> diff --git a/tests/nodedevmdevctltest.c b/tests/nodedevmdevctltest.c
> new file mode 100644
> index 0000000000..32a22246c2
> --- /dev/null
> +++ b/tests/nodedevmdevctltest.c
> @@ -0,0 +1,257 @@
> +#include <config.h>
> +
> +#include "internal.h"
> +#include "testutils.h"
> +#include "datatypes.h"
> +#include "node_device/node_device_driver.h"
> +#include "vircommand.h"
> +#define LIBVIRT_VIRCOMMANDPRIV_H_ALLOW
> +#include "vircommandpriv.h"
> +
> +#define VIR_FROM_THIS VIR_FROM_NODEDEV
> +
> +struct testInfo {
> +    bool shouldFail;
> +    const char *virt_type;
> +    int create;
> +    const char *filename;
> +};
> +
> +// capture stdin passed to command
> +static void
> +testCommandDryRunCallback(const char *const*args G_GNUC_UNUSED,
> +                          const char *const*env G_GNUC_UNUSED,
> +                          const char *input,
> +                          char **output G_GNUC_UNUSED,
> +                          char **error G_GNUC_UNUSED,
> +                          int *status G_GNUC_UNUSED,
> +                          void *opaque G_GNUC_UNUSED)
> +{
> +    char **stdinbuf = opaque;
> +
> +    *stdinbuf = g_strdup(input);
> +}
> +
> +static int
> +testMdevctlStart(bool shouldFail G_GNUC_UNUSED,
> +                 const char *virt_type,
> +                 int create,
> +                 const char *mdevxml,
> +                 const char *startcmdfile,
> +                 const char *startjsonfile)
> +{
> +    virNodeDeviceDefPtr def = NULL;

@def is never freed.

> +    virNodeDeviceObjPtr obj = NULL;
> +    virBuffer buf = VIR_BUFFER_INITIALIZER;
> +    const char *actualCmdline = NULL;
> +    int ret = -1;
> +    g_autofree char *uuid = NULL;
> +    g_autofree char *stdinbuf = NULL;
> +    g_autoptr(virCommand) cmd = NULL;
> +
> +    if (!(def = virNodeDeviceDefParseFile(mdevxml, create, virt_type)))
> +        goto cleanup;
> +
> +    // this function will set a stdin buffer containing the json configuration
> +    // of the device. This json value is captured in the mock wrapper above
> +    cmd = nodeDeviceGetMdevctlStartCommand(def, false, &uuid);
> +
> +    if (!cmd)
> +        goto cleanup;
> +
> +    virCommandSetDryRun(&buf, testCommandDryRunCallback, &stdinbuf);
> +    if (virCommandRun(cmd, NULL) < 0)
> +        goto cleanup;
> +
> +    if (!(actualCmdline = virBufferCurrentContent(&buf)))
> +        goto cleanup;
> +
> +    if (virTestCompareToFile(actualCmdline, startcmdfile) < 0)
> +        goto cleanup;
> +
> +    if (virTestCompareToFile(stdinbuf, startjsonfile) < 0)
> +        goto cleanup;
> +
> +    ret = 0;
> +
> + cleanup:
> +    virBufferFreeAndReset(&buf);
> +    virCommandSetDryRun(NULL, NULL, NULL);
> +    virNodeDeviceObjEndAPI(&obj);
> +    return ret;
> +}
> +
> +static int
> +testMdevctlStartHelper(const void *data)
> +{
> +    const struct testInfo *info = data;
> +
> +    g_autofree char *mdevxml = g_strdup_printf("%s/nodedevschemadata/%s.xml",
> +                                               abs_srcdir, info->filename);
> +    g_autofree char *cmdlinefile = g_strdup_printf("%s/nodedevmdevctldata/%s-start.argv",
> +                                                   abs_srcdir, info->filename);
> +    g_autofree char *jsonfile = g_strdup_printf("%s/nodedevmdevctldata/%s-start.json",
> +                                                   abs_srcdir, info->filename);
> +
> +    return testMdevctlStart(info->shouldFail, info->virt_type,
> +                            info->create, mdevxml, cmdlinefile,
> +                            jsonfile);
> +}
> +
> +static void
> +nodedevTestDriverFree(virNodeDeviceDriverStatePtr drv)
> +{
> +    if (!drv)
> +        return;
> +

All devices added in nodedevTestDriverAddTestDevices() should be freed here.

> +    virCondDestroy(&drv->initCond);
> +    virMutexDestroy(&drv->lock);
> +    VIR_FREE(drv->stateDir);
> +    VIR_FREE(drv);
> +}
> +
> +/* Add a fake root 'computer' device */
> +static virNodeDeviceDefPtr
> +fakeRootDevice(void)
> +{
> +    virNodeDeviceDefPtr def = NULL;
> +
> +    if (VIR_ALLOC(def) != 0 || VIR_ALLOC(def->caps) != 0) {
> +        virNodeDeviceDefFree(def);
> +        return NULL;
> +    }
> +
> +    def->name = g_strdup("computer");
> +
> +    return def;
> +}
> +
> +/* Add a fake pci device that can be used as a parent device for mediated
> + * devices. For our purposes, it only needs to have a name that matches the
> + * parent of the mdev, and it needs a PCI address
> + */
> +static virNodeDeviceDefPtr
> +fakeParentDevice(void)
> +{
> +    virNodeDeviceDefPtr def = NULL;
> +    virNodeDevCapPCIDevPtr pci_dev;
> +
> +    if (VIR_ALLOC(def) != 0 || VIR_ALLOC(def->caps) != 0) {
> +        virNodeDeviceDefFree(def);
> +        return NULL;
> +    }
> +
> +    def->name = g_strdup("pci_0000_00_02_0");
> +    def->parent = g_strdup("computer");
> +
> +    def->caps->data.type = VIR_NODE_DEV_CAP_PCI_DEV;
> +    pci_dev = &def->caps->data.pci_dev;
> +    pci_dev->domain = 0;
> +    pci_dev->bus = 0;
> +    pci_dev->slot = 2;
> +    pci_dev->function = 0;
> +
> +    return def;
> +}
> +
> +static int
> +addDevice(virNodeDeviceDefPtr def)
> +{
> +    if (!def)
> +        return -1;
> +
> +    virNodeDeviceObjPtr obj = virNodeDeviceObjListAssignDef(driver->devs, def);
> +
> +    if (!obj) {
> +        virNodeDeviceDefFree(def);
> +        return -1;
> +    }
> +
> +    virNodeDeviceObjEndAPI(&obj);
> +    return 0;
> +}
> +
> +static int
> +nodedevTestDriverAddTestDevices(void)
> +{
> +    if (addDevice(fakeRootDevice()) < 0 ||
> +        addDevice(fakeParentDevice()) < 0)
> +        return -1;
> +
> +    return 0;
> +}
> +
> +/* Bare minimum driver init to be able to test nodedev functionality */
> +static int
> +nodedevTestDriverInit(void)
> +{
> +    int ret = -1;
> +    char statedir[] = abs_builddir "/nodedevstatedir-XXXXXX";
> +    if (VIR_ALLOC(driver) < 0)
> +        return -1;
> +
> +    if (!g_mkdtemp(statedir)) {
> +        fprintf(stderr, "Cannot create fake stateDir");
> +        goto error;
> +    }

This creates a temporary dir which is never removed. But it doesn't look 
like the directory is needed at all, is it?

> +
> +    driver->stateDir = g_strdup(statedir);
> +    driver->lockFD = -1;
> +    if (virMutexInit(&driver->lock) < 0 ||
> +        virCondInit(&driver->initCond) < 0) {
> +        VIR_TEST_DEBUG("Unable to initialize test nodedev driver");
> +        goto error;
> +    }
> +
> +    if (!(driver->devs = virNodeDeviceObjListNew()))
> +        goto error;
> +
> +    return 0;
> +
> + error:
> +    nodedevTestDriverFree(driver);
> +    return ret;
> +}


The following should be squashed in:

diff --git i/tests/nodedevmdevctltest.c w/tests/nodedevmdevctltest.c
index 32a22246c2..e57eaa0cf2 100644
--- i/tests/nodedevmdevctltest.c
+++ w/tests/nodedevmdevctltest.c
@@ -40,7 +40,7 @@ testMdevctlStart(bool shouldFail G_GNUC_UNUSED,
                   const char *startcmdfile,
                   const char *startjsonfile)
  {
-    virNodeDeviceDefPtr def = NULL;
+    g_autoptr(virNodeDeviceDef) def = NULL;
      virNodeDeviceObjPtr obj = NULL;
      virBuffer buf = VIR_BUFFER_INITIALIZER;
      const char *actualCmdline = NULL;
@@ -104,6 +104,7 @@ nodedevTestDriverFree(virNodeDeviceDriverStatePtr drv)
      if (!drv)
          return;

+    virNodeDeviceObjListFree(drv->devs);
      virCondDestroy(&drv->initCond);
      virMutexDestroy(&drv->lock);
      VIR_FREE(drv->stateDir);
@@ -186,16 +187,10 @@ static int
  nodedevTestDriverInit(void)
  {
      int ret = -1;
-    char statedir[] = abs_builddir "/nodedevstatedir-XXXXXX";
+
      if (VIR_ALLOC(driver) < 0)
          return -1;

-    if (!g_mkdtemp(statedir)) {
-        fprintf(stderr, "Cannot create fake stateDir");
-        goto error;
-    }
-
-    driver->stateDir = g_strdup(statedir);
      driver->lockFD = -1;
      if (virMutexInit(&driver->lock) < 0 ||
          virCondInit(&driver->initCond) < 0) {


Michal




More information about the libvir-list mailing list