On 6/9/20 11:43 PM, Jonathon Jongsma wrote:

Mediated devices support arbitrary vendor-specific attributes that can be attached to a mediated device. These attributes are ordered, and are written to sysfs in order after a device is created. This patch adds support for these attributes to the mdev data types and XML schema.

Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- docs/formatnode.html.in | 7 +++++ docs/schemas/nodedev.rng | 6 ++++ src/conf/node_device_conf.c | 59 +++++++++++++++++++++++++++++++++++-- src/conf/node_device_conf.h | 2 ++ src/libvirt_private.syms | 2 ++ src/util/virmdev.c | 12 ++++++++ src/util/virmdev.h | 11 +++++++ 7 files changed, 96 insertions(+), 3 deletions(-)

+static int +virNodeDevCapMdevAttributeParseXML(xmlXPathContextPtr ctxt, + xmlNodePtr node, + virNodeDevCapMdevPtr mdev) +{ + xmlNodePtr orig_node = node; + ctxt->node = node;

Alternatively, VIR_XPATH_NODE_AUTORESTORE() can be used.

+ int ret = -1; + g_autoptr(virMediatedDeviceAttr) attr = virMediatedDeviceAttrNew(); + + attr->name = virXPathString("string(./@name)", ctxt); + attr->value = virXPathString("string(./@value)", ctxt); + if (!attr->name || !attr->value) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("mdev attribute missing name or value")); + goto cleanup; + } + + if (VIR_APPEND_ELEMENT(mdev->attributes, + mdev->nattributes, + attr) < 0) + goto cleanup; + + ret = 0; + + cleanup: + ctxt->node = orig_node; + return ret; +}

static int virNodeDevCapMdevParseXML(xmlXPathContextPtr ctxt, @@ -1766,6 +1808,9 @@ virNodeDevCapMdevParseXML(xmlXPathContextPtr ctxt, { VIR_XPATH_NODE_AUTORESTORE(ctxt); int ret = -1; + int nattrs = 0; + xmlNodePtr *attrs;

g_autofree becasue ..

+ size_t i;

ctxt->node = node;

@@ -1783,6 +1828,11 @@ virNodeDevCapMdevParseXML(xmlXPathContextPtr ctxt, "'%s'")) < 0) goto out;

+ if ((nattrs = virXPathNodeSet("./attr", ctxt, &attrs)) < 0) + goto out;

.. this allocates @attrs array.

+ for (i = 0; i < nattrs; i++) + virNodeDevCapMdevAttributeParseXML(ctxt, attrs[i], mdev); + ret = 0; out: return ret;

Just squash this in: diff --git i/src/conf/node_device_conf.c w/src/conf/node_device_conf.c index 045d146433..7b2a8c5545 100644 --- i/src/conf/node_device_conf.c +++ w/src/conf/node_device_conf.c @@ -1775,29 +1775,21 @@ virNodeDevCapMdevAttributeParseXML(xmlXPathContextPtr ctxt, xmlNodePtr node, virNodeDevCapMdevPtr mdev) { - xmlNodePtr orig_node = node; - ctxt->node = node; - int ret = -1; + VIR_XPATH_NODE_AUTORESTORE(ctxt); g_autoptr(virMediatedDeviceAttr) attr = virMediatedDeviceAttrNew(); + ctxt->node = node; attr->name = virXPathString("string(./@name)", ctxt); attr->value = virXPathString("string(./@value)", ctxt); if (!attr->name || !attr->value) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("mdev attribute missing name or value")); - goto cleanup; + return -1; } - if (VIR_APPEND_ELEMENT(mdev->attributes, - mdev->nattributes, - attr) < 0) - goto cleanup; - - ret = 0; - - cleanup: - ctxt->node = orig_node; - return ret; + return VIR_APPEND_ELEMENT(mdev->attributes, + mdev->nattributes, + attr); } static int @@ -1809,7 +1801,7 @@ virNodeDevCapMdevParseXML(xmlXPathContextPtr ctxt, VIR_XPATH_NODE_AUTORESTORE(ctxt); int ret = -1; int nattrs = 0; - xmlNodePtr *attrs; + g_autofree xmlNodePtr *attrs = NULL; size_t i; ctxt->node = node; Michal

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@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

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@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. Regards, Erik

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@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/logs/...

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. Speaking of build requirements, what is your opinion on adding (well goes the same for TC apparently) BuildRequires with mdevctl to the SPEC file? Erik

On Tue, Jun 09, 2020 at 04:43:47PM -0500, 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@redhat.com> --- ...

+ + // this function will set a stdin buffer containing the json configuration + // of the device. This json value is captured in the mock wrapper above

nitpick: /* ... */ instead of //

+ 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; + + 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; + } + + 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; +} + +static int +mymain(void) +{ + int ret = 0; + + if (nodedevTestDriverInit() < 0) + return EXIT_FAILURE; + + if (nodedevTestDriverAddTestDevices() < 0) { + ret = EXIT_FAILURE; + goto done; + } + + // add a mock device to the device list so it can be used as a parent + // reference

..same ^here...

+ +#define DO_TEST_FULL(shouldFail, virt_type, create, filename) \ + do { \ + struct testInfo info = { shouldFail, virt_type, create, filename, }; \ + if (virTestRun("mdevctl start " filename, \ + testMdevctlStartHelper, &info) < 0) \ + ret = -1; \ + } \ + while (0); + +#define DO_TEST(...) \ + DO_TEST_FULL(false, "QEMU", CREATE_DEVICE, __VA_ARGS__) +#define DO_TEST_FAIL(...) \ + DO_TEST_FULL(true, "QEMU", CREATE_DEVICE, __VA_ARGS__) + + // Test mdevctl start commands + DO_TEST("mdev_d069d019_36ea_4111_8f0a_8c9a70e21366"); + DO_TEST("mdev_fedc4916_1ca8_49ac_b176_871d16c13076"); + DO_TEST("mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9"); + + done: + nodedevTestDriverFree(driver); + + return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; +} + +VIR_TEST_MAIN(mymain) diff --git a/tests/nodedevschemadata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366.xml b/tests/nodedevschemadata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366.xml new file mode 100644 index 0000000000..36d50630e4 --- /dev/null +++ b/tests/nodedevschemadata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366.xml @@ -0,0 +1,8 @@ +<device> + <name>mdev_d069d019_36ea_4111_8f0a_8c9a70e21366</name> + <parent>pci_0000_00_02_0</parent> + <capability type='mdev'> + <type id='i915-GVTg_V5_8'/> + <iommuGroup number='0'/>

Like I said in the response to one of the earlier patches, I'd like to see iommuGroup gone from the create XML. Reviewed-by: Erik Skultety <eskultet@redhat.com>

On 6/9/20 11:43 PM, Jonathon Jongsma wrote:

Add the ability to destroy mdev node devices via the mdevctl utility.

Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- src/node_device/node_device_driver.c | 46 ++++++++++++++++++++++++++++ src/node_device/node_device_driver.h | 3 ++ 2 files changed, 49 insertions(+)

diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index dbc7eb4d1e..c956bb55fc 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -790,6 +790,45 @@ nodeDeviceCreateXML(virConnectPtr conn, }

+virCommandPtr +nodeDeviceGetMdevctlStopCommand(const char *uuid, + bool persist) +{ + g_autofree char *mdevctl = virFindFileInPath(MDEVCTL);

Same comment about virFindFileInPath() as in one of previous patches.

+ const char *subcommand; + + if (!mdevctl) + return NULL; + + if (persist) + subcommand = "undefine"; + else + subcommand = "stop"; + + virCommandPtr cmd = virCommandNewArgList(mdevctl, + subcommand, + "-u", + uuid, + NULL);

We don't really like variables being defined in the middle of a block. Fortunately, the variable is not really needed and this can be turned into "return virCommandNewArgList(...)" Squash this in: diff --git i/src/node_device/node_device_driver.c w/src/node_device/node_device_driver.c index 23d18308f7..2c204c7a83 100644 --- i/src/node_device/node_device_driver.c +++ w/src/node_device/node_device_driver.c @@ -790,26 +790,21 @@ virCommandPtr nodeDeviceGetMdevctlStopCommand(const char *uuid, bool persist) { - g_autofree char *mdevctl = virFindFileInPath(MDEVCTL); const char *subcommand; - if (!mdevctl) - return NULL; - if (persist) subcommand = "undefine"; else subcommand = "stop"; - virCommandPtr cmd = virCommandNewArgList(mdevctl, - subcommand, - "-u", - uuid, - NULL); - - return cmd; + return virCommandNewArgList(MDEVCTL, + subcommand, + "-u", + uuid, + NULL); } + static int virMdevctlStop(virNodeDeviceDefPtr def) { Michal

On Thu, 2020-06-11 at 16:00 +0200, Erik Skultety wrote:

On Tue, Jun 09, 2020 at 04:43:48PM -0500, Jonathon Jongsma wrote:

...

Add the ability to destroy mdev node devices via the mdevctl utility.

Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- src/node_device/node_device_driver.c | 46 ++++++++++++++++++++++++++++ src/node_device/node_device_driver.h | 3 ++ 2 files changed, 49 insertions(+)

diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index dbc7eb4d1e..c956bb55fc 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -790,6 +790,45 @@ nodeDeviceCreateXML(virConnectPtr conn, }

+virCommandPtr +nodeDeviceGetMdevctlStopCommand(const char *uuid, + bool persist) +{ + g_autofree char *mdevctl = virFindFileInPath(MDEVCTL); + const char *subcommand; + + if (!mdevctl) + return NULL; + + if (persist) + subcommand = "undefine";

"undefine" is a NOP on active mdevs IIRC, so again the helper name is confusing.

Oh, you're right. This part was meant to plan ahead for persistent mediated devices, but since it's not yet used (and since it doesn't have the effect intended, as you point out), it should probably just be removed from this patch series.

...

+ else + subcommand = "stop"; + + virCommandPtr cmd = virCommandNewArgList(mdevctl, + subcommand, + "-u", + uuid, + NULL); + + return cmd; +}

Like I mentioned already, we could have a generic translator to the mdevctl subcommands.

Regards, Erik

On Tue, Jun 09, 2020 at 04:43:49PM -0500, Jonathon Jongsma wrote:

Test that we run 'mdevctl' with the proper arguments when we destroy mediated devices with virNodeDeviceDestroy()

Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com> --- tests/nodedevmdevctldata/mdevctl-stop.argv | 1 + tests/nodedevmdevctltest.c | 43 ++++++++++++++++++++++ 2 files changed, 44 insertions(+) create mode 100644 tests/nodedevmdevctldata/mdevctl-stop.argv

diff --git a/tests/nodedevmdevctldata/mdevctl-stop.argv b/tests/nodedevmdevctldata/mdevctl-stop.argv new file mode 100644 index 0000000000..25ee7145ce --- /dev/null +++ b/tests/nodedevmdevctldata/mdevctl-stop.argv @@ -0,0 +1 @@ +/usr/sbin/mdevctl stop -u e2451f73-c95b-4124-b900-e008af37c576 diff --git a/tests/nodedevmdevctltest.c b/tests/nodedevmdevctltest.c index 32a22246c2..58ebf976e2 100644 --- a/tests/nodedevmdevctltest.c +++ b/tests/nodedevmdevctltest.c @@ -98,6 +98,42 @@ testMdevctlStartHelper(const void *data) jsonfile); }

+static int +testMdevctlStopHelper(const void *data)

This is not very symmetric to the StartHelper, maybe we can call this one MdevctlStop directly?

+{ + const char *uuid = data; + virBuffer buf = VIR_BUFFER_INITIALIZER; + const char *actualCmdline = NULL; + int ret = -1; + g_autoptr(virCommand) cmd = NULL; + + g_autofree char *cmdlinefile = + g_strdup_printf("%s/nodedevmdevctldata/mdevctl-stop.argv", + abs_srcdir); + + cmd = nodeDeviceGetMdevctlStopCommand(uuid, false); + + if (!cmd) + goto cleanup; + + virCommandSetDryRun(&buf, NULL, NULL); + if (virCommandRun(cmd, NULL) < 0) + goto cleanup; + + if (!(actualCmdline = virBufferCurrentContent(&buf))) + goto cleanup; + + if (virTestCompareToFile(actualCmdline, cmdlinefile) < 0) + goto cleanup; + + ret = 0; + + cleanup: + virBufferFreeAndReset(&buf); + virCommandSetDryRun(NULL, NULL, NULL); + return ret; +} + static void nodedevTestDriverFree(virNodeDeviceDriverStatePtr drv) { @@ -248,6 +284,13 @@ mymain(void) DO_TEST("mdev_fedc4916_1ca8_49ac_b176_871d16c13076"); DO_TEST("mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9");

+ // Test mdevctl stop command, pass an arbitrary uuid

/* ... */

+ if (virTestRun("mdevctl stop", testMdevctlStopHelper, + "e2451f73-c95b-4124-b900-e008af37c576") < 0)

Can we remain consistent with the usage of macros and rework DO_TEST_FULL so that mdevctl stop could be run through the DO_TEST macro? Erik