3:39 p.m.
Erik recommended several changes to the mdev series that was just merged and
suggested to address them in a follow-up patch series. They're mostly related
to simplifying the testing, and shouldn't actually change any behavior (aside
from switching to using the long commandline options when executing mdevctl).
Changes in v2:
- rebase to lastest git master
Erik Skultety (4):
nodedev: driver: Swap virMdevctlStart and virMdevctlCreate
nodedev: driver: Introduce internal mdevctl commands enum
nodedev: driver: Create a generic mdevctl command translator
tests: nodedev: Make the mdevctl test function and helper generic
Jonathon Jongsma (8):
nodedev: don't log error in nodeDeviceFindAddressByName()
nodedev: avoid use of VIR_ERR_NO_* errors internally
tests: nodedev: switch all test macros to accept a filename
nodedev: Switch to using long options for mdevctl
nodedev: Remove GetMdevctl*Command() wrappers
tests: nodedev: simplify test macros
tests: nodedev: remove unused variable
tests: nodedev: remove unnecessary cleanup label
src/node_device/node_device_driver.c | 181 +++++++--------
src/node_device/node_device_driver.h | 43 ++--
...19_36ea_4111_8f0a_8c9a70e21366-create.argv | 5 +
...9_36ea_4111_8f0a_8c9a70e21366-create.json} | 0
...19_36ea_4111_8f0a_8c9a70e21366-define.argv | 4 +-
...019_36ea_4111_8f0a_8c9a70e21366-start.argv | 2 -
...d019_36ea_4111_8f0a_8c9a70e21366-stop.argv | 3 +
..._36ea_4111_8f0a_8c9a70e21366-undefine.argv | 3 +
...39_495e_4243_ad9f_beb3f14c23d9-create.argv | 4 +
...9_495e_4243_ad9f_beb3f14c23d9-create.json} | 0
...39_495e_4243_ad9f_beb3f14c23d9-define.argv | 4 +-
...d39_495e_4243_ad9f_beb3f14c23d9-start.argv | 4 -
...16_1ca8_49ac_b176_871d16c13076-create.argv | 4 +
...6_1ca8_49ac_b176_871d16c13076-create.json} | 0
...16_1ca8_49ac_b176_871d16c13076-define.argv | 4 +-
...916_1ca8_49ac_b176_871d16c13076-start.argv | 4 -
tests/nodedevmdevctldata/mdevctl-create.argv | 3 -
tests/nodedevmdevctldata/mdevctl-stop.argv | 3 -
.../nodedevmdevctldata/mdevctl-undefine.argv | 3 -
tests/nodedevmdevctltest.c | 211 ++++++------------
20 files changed, 197 insertions(+), 288 deletions(-)
create mode 100644 tests/nodedevmdevctldata/mdev_d069d019_36ea_4111_8f0a_8c9=
a70e21366-create.argv
rename tests/nodedevmdevctldata/{mdev_d069d019_36ea_4111_8f0a_8c9a70e21366-s=
tart.json =3D> mdev_d069d019_36ea_4111_8f0a_8c9a70e21366-create.json} (100%)
create mode 100644 tests/nodedevmdevctldata/mdev_d069d019_36ea_4111_8f0a_8c9=
a70e21366-stop.argv
create mode 100644 tests/nodedevmdevctldata/mdev_d069d019_36ea_4111_8f0a_8c9=
a70e21366-undefine.argv
create mode 100644 tests/nodedevmdevctldata/mdev_d2441d39_495e_4243_ad9f_beb=
3f14c23d9-create.argv
rename tests/nodedevmdevctldata/{mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9-s=
tart.json =3D> mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9-create.json} (100%)
delete mode 100644 tests/nodedevmdevctldata/mdev_d2441d39_495e_4243_ad9f_beb=
3f14c23d9-start.argv
create mode 100644 tests/nodedevmdevctldata/mdev_fedc4916_1ca8_49ac_b176_871=
d16c13076-create.argv
rename tests/nodedevmdevctldata/{mdev_fedc4916_1ca8_49ac_b176_871d16c13076-s=
tart.json =3D> mdev_fedc4916_1ca8_49ac_b176_871d16c13076-create.json} (100%)
delete mode 100644 tests/nodedevmdevctldata/mdev_fedc4916_1ca8_49ac_b176_871=
d16c13076-start.argv
delete mode 100644 tests/nodedevmdevctldata/mdevctl-create.argv
delete mode 100644 tests/nodedevmdevctldata/mdevctl-stop.argv
delete mode 100644 tests/nodedevmdevctldata/mdevctl-undefine.argv
--=20
2.26.3
3:39 p.m.
New subject: [libvirt PATCH v2 01/12] nodedev: don't log error in nodeDeviceFindAddressByName()
The calling function will log the error. Just return NULL if a device
cannot be found.
Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com>
---
src/node_device/node_device_driver.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c
index 5f8995172d..4cf4e4214f 100644
--- a/src/node_device/node_device_driver.c
+++ b/src/node_device/node_device_driver.c
@@ -630,11 +630,8 @@ nodeDeviceFindAddressByName(const char *name)
char *addr = NULL;
virNodeDeviceObj *dev = virNodeDeviceObjListFindByName(driver->devs, name);
- if (!dev) {
- virReportError(VIR_ERR_NO_NODE_DEVICE,
- _("could not find device '%s'"), name);
+ if (!dev)
return NULL;
- }
def = virNodeDeviceObjGetDef(dev);
for (caps = def->caps; caps != NULL; caps = caps->next) {
--
2.26.3
5:10 p.m.
New subject: [libvirt PATCH v2 01/12] nodedev: don't log error in nodeDeviceFindAddressByName()
On 4/13/21 4:39 PM, Jonathon Jongsma wrote:
The calling function will log the error. Just return NULL if a
device
cannot be found.
Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com>
Reviewed-by: Laine Stump <laine(a)redhat.com>
---
src/node_device/node_device_driver.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c
index 5f8995172d..4cf4e4214f 100644
--- a/src/node_device/node_device_driver.c
+++ b/src/node_device/node_device_driver.c
@@ -630,11 +630,8 @@ nodeDeviceFindAddressByName(const char *name)
char *addr = NULL;
virNodeDeviceObj *dev = virNodeDeviceObjListFindByName(driver->devs, name);
- if (!dev) {
- virReportError(VIR_ERR_NO_NODE_DEVICE,
- _("could not find device '%s'"), name);
+ if (!dev)
return NULL;
- }
def = virNodeDeviceObjGetDef(dev);
for (caps = def->caps; caps != NULL; caps = caps->next) {
4:12 a.m.
New subject: [libvirt PATCH v2 01/12] nodedev: don't log error in nodeDeviceFindAddressByName()
On Tue, Apr 13, 2021 at 03:39:37PM -0500, Jonathon Jongsma wrote:
The calling function will log the error. Just return NULL if a
device
cannot be found.
Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com>
---
Reviewed-by: Erik Skultety <eskultet(a)redhat.com>
3:39 p.m.
New subject: [libvirt PATCH v2 02/12] nodedev: avoid use of VIR_ERR_NO_* errors internally
These errors are demoted to debug statements[1] since they're only
intended to be used as return values for public APIs. This makes it
difficult to debug the problem when something goes wrong since no error
message is logged. Switch instead to VIR_ERR_INTERNAL_ERROR so that the
error is logged as expected.
[1] See the implementation of daemonErrorLogFilter() for details:
https://gitlab.com/libvirt/libvirt/-/blob/e2f82a3704f680fbb37a733476d870c...
Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com>
---
src/node_device/node_device_driver.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c
index 4cf4e4214f..1d1eaa9561 100644
--- a/src/node_device/node_device_driver.c
+++ b/src/node_device/node_device_driver.c
@@ -709,7 +709,7 @@ nodeDeviceGetMdevctlDefineStartCommand(virNodeDeviceDef *def,
g_autofree char *parent_addr = nodeDeviceFindAddressByName(def->parent);
if (!parent_addr) {
- virReportError(VIR_ERR_NO_NODE_DEVICE,
+ virReportError(VIR_ERR_INTERNAL_ERROR,
_("unable to find parent device '%s'"),
def->parent);
return NULL;
}
--
2.26.3
5:14 p.m.
New subject: [libvirt PATCH v2 02/12] nodedev: avoid use of VIR_ERR_NO_* errors internally
On 4/13/21 4:39 PM, Jonathon Jongsma wrote:
These errors are demoted to debug statements[1] since they're
only
intended to be used as return values for public APIs. This makes it
difficult to debug the problem when something goes wrong since no error
message is logged. Switch instead to VIR_ERR_INTERNAL_ERROR so that the
error is logged as expected.
[1] See the implementation of daemonErrorLogFilter() for details:
https://gitlab.com/libvirt/libvirt/-/blob/e2f82a3704f680fbb37a733476d870c...
Huh. TIL.
Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com>
Reviewed-by: Laine Stump <laine(a)redhat.com>
---
src/node_device/node_device_driver.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c
index 4cf4e4214f..1d1eaa9561 100644
--- a/src/node_device/node_device_driver.c
+++ b/src/node_device/node_device_driver.c
@@ -709,7 +709,7 @@ nodeDeviceGetMdevctlDefineStartCommand(virNodeDeviceDef *def,
g_autofree char *parent_addr = nodeDeviceFindAddressByName(def->parent);
if (!parent_addr) {
- virReportError(VIR_ERR_NO_NODE_DEVICE,
+ virReportError(VIR_ERR_INTERNAL_ERROR,
_("unable to find parent device '%s'"),
def->parent);
return NULL;
}
4:29 a.m.
New subject: [libvirt PATCH v2 02/12] nodedev: avoid use of VIR_ERR_NO_* errors internally
On Tue, Apr 13, 2021 at 03:39:38PM -0500, Jonathon Jongsma wrote:
These errors are demoted to debug statements[1] since they're
only
intended to be used as return values for public APIs. This makes it
difficult to debug the problem when something goes wrong since no error
message is logged. Switch instead to VIR_ERR_INTERNAL_ERROR so that the
error is logged as expected.
[1] See the implementation of daemonErrorLogFilter() for details:
https://gitlab.com/libvirt/libvirt/-/blob/e2f82a3704f680fbb37a733476d870c...
Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com>
---
Reviewed-by: Erik Skultety <eskultet(a)redhat.com>
3:39 p.m.
New subject: [libvirt PATCH v2 03/12] tests: nodedev: switch all test macros to accept a filename
Rather than specifying a UUID string to some test macros, just pass a
filename to an xml definition. This helps work toward unifying the test
macros and making it more maintainable.
Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com>
---
tests/nodedevmdevctldata/mdevctl-create.argv | 2 +-
tests/nodedevmdevctldata/mdevctl-stop.argv | 2 +-
.../nodedevmdevctldata/mdevctl-undefine.argv | 2 +-
tests/nodedevmdevctltest.c | 38 +++++++++++--------
4 files changed, 26 insertions(+), 18 deletions(-)
diff --git a/tests/nodedevmdevctldata/mdevctl-create.argv
b/tests/nodedevmdevctldata/mdevctl-create.argv
index f19c9780dc..ccb8e2992c 100644
--- a/tests/nodedevmdevctldata/mdevctl-create.argv
+++ b/tests/nodedevmdevctldata/mdevctl-create.argv
@@ -1,3 +1,3 @@
mdevctl \
start \
--u 8a05ad83-3472-497d-8631-8142f31460e8
+-u d069d019-36ea-4111-8f0a-8c9a70e21366
diff --git a/tests/nodedevmdevctldata/mdevctl-stop.argv
b/tests/nodedevmdevctldata/mdevctl-stop.argv
index cc621191d6..a07d339e47 100644
--- a/tests/nodedevmdevctldata/mdevctl-stop.argv
+++ b/tests/nodedevmdevctldata/mdevctl-stop.argv
@@ -1,3 +1,3 @@
mdevctl \
stop \
--u e2451f73-c95b-4124-b900-e008af37c576
+-u d069d019-36ea-4111-8f0a-8c9a70e21366
diff --git a/tests/nodedevmdevctldata/mdevctl-undefine.argv
b/tests/nodedevmdevctldata/mdevctl-undefine.argv
index f1a59c5242..dbde32e2a8 100644
--- a/tests/nodedevmdevctldata/mdevctl-undefine.argv
+++ b/tests/nodedevmdevctldata/mdevctl-undefine.argv
@@ -1,3 +1,3 @@
mdevctl \
undefine \
--u d76a6b78-45ed-4149-a325-005f9abc5281
+-u d069d019-36ea-4111-8f0a-8c9a70e21366
diff --git a/tests/nodedevmdevctltest.c b/tests/nodedevmdevctltest.c
index 99e79c18a5..df4185b5f8 100644
--- a/tests/nodedevmdevctltest.c
+++ b/tests/nodedevmdevctltest.c
@@ -125,13 +125,15 @@ testMdevctlStartOrDefineHelper(const void *data)
typedef virCommand* (*GetStopUndefineCmdFunc)(const char *uuid, char **errbuf);
struct UuidCommandTestInfo {
- const char *uuid;
+ const char *filename;
MdevctlCmd command;
};
static int
-testMdevctlUuidCommand(const char *uuid, GetStopUndefineCmdFunc func, const char
*outfile)
+testMdevctlUuidCommand(GetStopUndefineCmdFunc func,
+ const char *mdevxml, const char *outfile)
{
+ g_autoptr(virNodeDeviceDef) def = NULL;
g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;
const char *actualCmdline = NULL;
int ret = -1;
@@ -139,7 +141,10 @@ testMdevctlUuidCommand(const char *uuid, GetStopUndefineCmdFunc func,
const char
g_autofree char *errmsg = NULL;
g_autoptr(virCommandDryRunToken) dryRunToken = virCommandDryRunTokenNew();
- cmd = func(uuid, &errmsg);
+ if (!(def = virNodeDeviceDefParseFile(mdevxml, EXISTING_DEVICE, "QEMU")))
+ goto cleanup;
+
+ cmd = func(def->caps->data.mdev.uuid, &errmsg);
if (!cmd)
goto cleanup;
@@ -167,6 +172,7 @@ testMdevctlUuidCommandHelper(const void *data)
GetStopUndefineCmdFunc func;
const char *cmd;
g_autofree char *cmdlinefile = NULL;
+ g_autofree char *mdevxml = NULL;
if (info->command == MDEVCTL_CMD_STOP) {
cmd = "stop";
@@ -181,10 +187,12 @@ testMdevctlUuidCommandHelper(const void *data)
return -1;
}
+ mdevxml = g_strdup_printf("%s/nodedevschemadata/%s.xml", abs_srcdir,
+ info->filename);
cmdlinefile = g_strdup_printf("%s/nodedevmdevctldata/mdevctl-%s.argv",
abs_srcdir, cmd);
- return testMdevctlUuidCommand(info->uuid, func, cmdlinefile);
+ return testMdevctlUuidCommand(func, mdevxml, cmdlinefile);
}
static int
@@ -405,21 +413,21 @@ mymain(void)
#define DO_TEST_DEFINE(filename) \
DO_TEST_CMD("mdevctl define " filename, "QEMU", CREATE_DEVICE,
filename, MDEVCTL_CMD_DEFINE)
-#define DO_TEST_UUID_COMMAND_FULL(desc, uuid, command) \
+#define DO_TEST_UUID_COMMAND_FULL(desc, filename, command) \
do { \
- struct UuidCommandTestInfo info = { uuid, command }; \
+ struct UuidCommandTestInfo info = { filename, command }; \
DO_TEST_FULL(desc, testMdevctlUuidCommandHelper, &info); \
} \
while (0)
-#define DO_TEST_STOP(uuid) \
- DO_TEST_UUID_COMMAND_FULL("mdevctl stop " uuid, uuid, MDEVCTL_CMD_STOP)
+#define DO_TEST_STOP(filename) \
+ DO_TEST_UUID_COMMAND_FULL("mdevctl stop " filename, filename,
MDEVCTL_CMD_STOP)
-#define DO_TEST_UNDEFINE(uuid) \
- DO_TEST_UUID_COMMAND_FULL("mdevctl undefine " uuid, uuid,
MDEVCTL_CMD_UNDEFINE)
+#define DO_TEST_UNDEFINE(filename) \
+ DO_TEST_UUID_COMMAND_FULL("mdevctl undefine " filename, filename,
MDEVCTL_CMD_UNDEFINE)
-#define DO_TEST_CREATE(uuid) \
- DO_TEST_UUID_COMMAND_FULL("mdevctl create " uuid, uuid,
MDEVCTL_CMD_CREATE)
+#define DO_TEST_CREATE(filename) \
+ DO_TEST_UUID_COMMAND_FULL("mdevctl create " filename, filename,
MDEVCTL_CMD_CREATE)
#define DO_TEST_LIST_DEFINED() \
DO_TEST_FULL("mdevctl list --defined", testMdevctlListDefined, NULL)
@@ -433,7 +441,7 @@ mymain(void)
DO_TEST_START("mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9");
/* Test mdevctl stop command, pass an arbitrary uuid */
- DO_TEST_STOP("e2451f73-c95b-4124-b900-e008af37c576");
+ DO_TEST_STOP("mdev_d069d019_36ea_4111_8f0a_8c9a70e21366");
DO_TEST_LIST_DEFINED();
@@ -443,9 +451,9 @@ mymain(void)
DO_TEST_DEFINE("mdev_fedc4916_1ca8_49ac_b176_871d16c13076");
DO_TEST_DEFINE("mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9");
- DO_TEST_UNDEFINE("d76a6b78-45ed-4149-a325-005f9abc5281");
+ DO_TEST_UNDEFINE("mdev_d069d019_36ea_4111_8f0a_8c9a70e21366");
- DO_TEST_CREATE("8a05ad83-3472-497d-8631-8142f31460e8");
+ DO_TEST_CREATE("mdev_d069d019_36ea_4111_8f0a_8c9a70e21366");
done:
nodedevTestDriverFree(driver);
--
2.26.3
11 a.m.
New subject: [libvirt PATCH v2 03/12] tests: nodedev: switch all test macros to accept a filename
On 4/13/21 4:39 PM, Jonathon Jongsma wrote:
Rather than specifying a UUID string to some test macros, just pass
a
filename to an xml definition. This helps work toward unifying the test
macros and making it more maintainable.
Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com>
Reviewed-by: Laine Stump <laine(a)redhat.com>
3:39 p.m.
New subject: [libvirt PATCH v2 04/12] nodedev: driver: Swap virMdevctlStart and virMdevctlCreate
From: Erik Skultety <eskultet(a)redhat.com>
"start" in libvirt means - "take this object and create an
instance out of it"
"create" in libvirt most of the time means - "take and XML description,
make an object out of it and use it to create an instance"
This gets confusing with mdevctl which uses "start" for both. So, this
patch proposes to use virMdevctlStart in cases where from libvirt's POV
we're starting a defined device (unlike mdevctl). Analogically, use
virMdevctlCreate in scenarios where XML description is passed to
libvirt and a transient device is supposed to be created.
Signed-off-by: Erik Skultety <eskultet(a)redhat.com>
Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com>
---
src/node_device/node_device_driver.c | 34 ++++++-------
src/node_device/node_device_driver.h | 10 ++--
...9_36ea_4111_8f0a_8c9a70e21366-create.argv} | 0
...9_36ea_4111_8f0a_8c9a70e21366-create.json} | 0
...9_495e_4243_ad9f_beb3f14c23d9-create.argv} | 0
...9_495e_4243_ad9f_beb3f14c23d9-create.json} | 0
...6_1ca8_49ac_b176_871d16c13076-create.argv} | 0
...6_1ca8_49ac_b176_871d16c13076-create.json} | 0
...mdevctl-create.argv => mdevctl-start.argv} | 0
tests/nodedevmdevctltest.c | 49 +++++++++----------
10 files changed, 46 insertions(+), 47 deletions(-)
rename tests/nodedevmdevctldata/{mdev_d069d019_36ea_4111_8f0a_8c9a70e21366-start.argv
=> mdev_d069d019_36ea_4111_8f0a_8c9a70e21366-create.argv} (100%)
rename tests/nodedevmdevctldata/{mdev_d069d019_36ea_4111_8f0a_8c9a70e21366-start.json
=> mdev_d069d019_36ea_4111_8f0a_8c9a70e21366-create.json} (100%)
rename tests/nodedevmdevctldata/{mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9-start.argv
=> mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9-create.argv} (100%)
rename tests/nodedevmdevctldata/{mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9-start.json
=> mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9-create.json} (100%)
rename tests/nodedevmdevctldata/{mdev_fedc4916_1ca8_49ac_b176_871d16c13076-start.argv
=> mdev_fedc4916_1ca8_49ac_b176_871d16c13076-create.argv} (100%)
rename tests/nodedevmdevctldata/{mdev_fedc4916_1ca8_49ac_b176_871d16c13076-start.json
=> mdev_fedc4916_1ca8_49ac_b176_871d16c13076-create.json} (100%)
rename tests/nodedevmdevctldata/{mdevctl-create.argv => mdevctl-start.argv} (100%)
diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c
index 1d1eaa9561..dda118b0e5 100644
--- a/src/node_device/node_device_driver.c
+++ b/src/node_device/node_device_driver.c
@@ -699,10 +699,10 @@ nodeDeviceFindAddressByName(const char *name)
* arguments, so provide a common implementation that can be wrapped by a more
* specific function */
static virCommand*
-nodeDeviceGetMdevctlDefineStartCommand(virNodeDeviceDef *def,
- const char *subcommand,
- char **uuid_out,
- char **errmsg)
+nodeDeviceGetMdevctlDefineCreateCommand(virNodeDeviceDef *def,
+ const char *subcommand,
+ char **uuid_out,
+ char **errmsg)
{
virCommand *cmd;
g_autofree char *json = NULL;
@@ -737,12 +737,12 @@ nodeDeviceGetMdevctlDefineStartCommand(virNodeDeviceDef *def,
}
virCommand*
-nodeDeviceGetMdevctlStartCommand(virNodeDeviceDef *def,
+nodeDeviceGetMdevctlCreateCommand(virNodeDeviceDef *def,
char **uuid_out,
char **errmsg)
{
- return nodeDeviceGetMdevctlDefineStartCommand(def, "start", uuid_out,
- errmsg);
+ return nodeDeviceGetMdevctlDefineCreateCommand(def, "start", uuid_out,
+ errmsg);
}
virCommand*
@@ -750,18 +750,18 @@ nodeDeviceGetMdevctlDefineCommand(virNodeDeviceDef *def,
char **uuid_out,
char **errmsg)
{
- return nodeDeviceGetMdevctlDefineStartCommand(def, "define", uuid_out,
- errmsg);
+ return nodeDeviceGetMdevctlDefineCreateCommand(def, "define", uuid_out,
+ errmsg);
}
static int
-virMdevctlStart(virNodeDeviceDef *def, char **uuid, char **errmsg)
+virMdevctlCreate(virNodeDeviceDef *def, char **uuid, char **errmsg)
{
int status;
- g_autoptr(virCommand) cmd = nodeDeviceGetMdevctlStartCommand(def, uuid,
- errmsg);
+ g_autoptr(virCommand) cmd = nodeDeviceGetMdevctlCreateCommand(def, uuid,
+ errmsg);
if (!cmd)
return -1;
@@ -811,7 +811,7 @@ nodeDeviceCreateXMLMdev(virConnectPtr conn,
return NULL;
}
- if (virMdevctlStart(def, &uuid, &errmsg) < 0) {
+ if (virMdevctlCreate(def, &uuid, &errmsg) < 0) {
if (errmsg)
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Unable to start mediated device '%s':
%s"),
@@ -910,7 +910,7 @@ nodeDeviceGetMdevctlUndefineCommand(const char *uuid, char **errmsg)
}
virCommand *
-nodeDeviceGetMdevctlCreateCommand(const char *uuid, char **errmsg)
+nodeDeviceGetMdevctlStartCommand(const char *uuid, char **errmsg)
{
virCommand *cmd = virCommandNewArgList(MDEVCTL,
"start",
@@ -953,12 +953,12 @@ virMdevctlUndefine(virNodeDeviceDef *def, char **errmsg)
static int
-virMdevctlCreate(virNodeDeviceDef *def, char **errmsg)
+virMdevctlStart(virNodeDeviceDef *def, char **errmsg)
{
int status;
g_autoptr(virCommand) cmd = NULL;
- cmd = nodeDeviceGetMdevctlCreateCommand(def->caps->data.mdev.uuid, errmsg);
+ cmd = nodeDeviceGetMdevctlStartCommand(def->caps->data.mdev.uuid, errmsg);
if (virCommandRun(cmd, &status) < 0 || status != 0)
return -1;
@@ -1412,7 +1412,7 @@ nodeDeviceCreate(virNodeDevice *device,
if (nodeDeviceHasCapability(def, VIR_NODE_DEV_CAP_MDEV)) {
g_autofree char *errmsg = NULL;
- if (virMdevctlCreate(def, &errmsg) < 0) {
+ if (virMdevctlStart(def, &errmsg) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Unable to create mediated device: %s"),
errmsg && errmsg[0] ? errmsg : "Unknown
Error");
diff --git a/src/node_device/node_device_driver.h b/src/node_device/node_device_driver.h
index 5a64558f81..4101e34a8f 100644
--- a/src/node_device/node_device_driver.h
+++ b/src/node_device/node_device_driver.h
@@ -123,9 +123,9 @@ nodeConnectNodeDeviceEventDeregisterAny(virConnectPtr conn,
int callbackID);
virCommand *
-nodeDeviceGetMdevctlStartCommand(virNodeDeviceDef *def,
- char **uuid_out,
- char **errmsg);
+nodeDeviceGetMdevctlCreateCommand(virNodeDeviceDef *def,
+ char **uuid_out,
+ char **errmsg);
virCommand*
nodeDeviceGetMdevctlDefineCommand(virNodeDeviceDef *def,
@@ -162,8 +162,8 @@ bool nodeDeviceDefCopyFromMdevctl(virNodeDeviceDef *dst,
virNodeDeviceDef *src);
virCommand*
-nodeDeviceGetMdevctlCreateCommand(const char *uuid,
- char **errmsg);
+nodeDeviceGetMdevctlStartCommand(const char *uuid,
+ char **errmsg);
int
nodeDeviceCreate(virNodeDevice *dev,
diff --git a/tests/nodedevmdevctldata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366-start.argv
b/tests/nodedevmdevctldata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366-create.argv
similarity index 100%
rename from tests/nodedevmdevctldata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366-start.argv
rename to tests/nodedevmdevctldata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366-create.argv
diff --git a/tests/nodedevmdevctldata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366-start.json
b/tests/nodedevmdevctldata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366-create.json
similarity index 100%
rename from tests/nodedevmdevctldata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366-start.json
rename to tests/nodedevmdevctldata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366-create.json
diff --git a/tests/nodedevmdevctldata/mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9-start.argv
b/tests/nodedevmdevctldata/mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9-create.argv
similarity index 100%
rename from tests/nodedevmdevctldata/mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9-start.argv
rename to tests/nodedevmdevctldata/mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9-create.argv
diff --git a/tests/nodedevmdevctldata/mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9-start.json
b/tests/nodedevmdevctldata/mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9-create.json
similarity index 100%
rename from tests/nodedevmdevctldata/mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9-start.json
rename to tests/nodedevmdevctldata/mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9-create.json
diff --git a/tests/nodedevmdevctldata/mdev_fedc4916_1ca8_49ac_b176_871d16c13076-start.argv
b/tests/nodedevmdevctldata/mdev_fedc4916_1ca8_49ac_b176_871d16c13076-create.argv
similarity index 100%
rename from tests/nodedevmdevctldata/mdev_fedc4916_1ca8_49ac_b176_871d16c13076-start.argv
rename to tests/nodedevmdevctldata/mdev_fedc4916_1ca8_49ac_b176_871d16c13076-create.argv
diff --git a/tests/nodedevmdevctldata/mdev_fedc4916_1ca8_49ac_b176_871d16c13076-start.json
b/tests/nodedevmdevctldata/mdev_fedc4916_1ca8_49ac_b176_871d16c13076-create.json
similarity index 100%
rename from tests/nodedevmdevctldata/mdev_fedc4916_1ca8_49ac_b176_871d16c13076-start.json
rename to tests/nodedevmdevctldata/mdev_fedc4916_1ca8_49ac_b176_871d16c13076-create.json
diff --git a/tests/nodedevmdevctldata/mdevctl-create.argv
b/tests/nodedevmdevctldata/mdevctl-start.argv
similarity index 100%
rename from tests/nodedevmdevctldata/mdevctl-create.argv
rename to tests/nodedevmdevctldata/mdevctl-start.argv
diff --git a/tests/nodedevmdevctltest.c b/tests/nodedevmdevctltest.c
index df4185b5f8..188e521f59 100644
--- a/tests/nodedevmdevctltest.c
+++ b/tests/nodedevmdevctltest.c
@@ -44,12 +44,12 @@ typedef virCommand* (*MdevctlCmdFunc)(virNodeDeviceDef *, char **,
char **);
static int
-testMdevctlStartOrDefine(const char *virt_type,
- int create,
- MdevctlCmdFunc mdevctl_cmd_func,
- const char *mdevxml,
- const char *cmdfile,
- const char *jsonfile)
+testMdevctlCreateOrDefine(const char *virt_type,
+ int create,
+ MdevctlCmdFunc mdevctl_cmd_func,
+ const char *mdevxml,
+ const char *cmdfile,
+ const char *jsonfile)
{
g_autoptr(virNodeDeviceDef) def = NULL;
virNodeDeviceObj *obj = NULL;
@@ -93,7 +93,7 @@ testMdevctlStartOrDefine(const char *virt_type,
}
static int
-testMdevctlStartOrDefineHelper(const void *data)
+testMdevctlCreateOrDefineHelper(const void *data)
{
const struct startTestInfo *info = data;
const char *cmd;
@@ -102,9 +102,9 @@ testMdevctlStartOrDefineHelper(const void *data)
g_autofree char *cmdlinefile = NULL;
g_autofree char *jsonfile = NULL;
- if (info->command == MDEVCTL_CMD_START) {
- cmd = "start";
- func = nodeDeviceGetMdevctlStartCommand;
+ if (info->command == MDEVCTL_CMD_CREATE) {
+ cmd = "create";
+ func = nodeDeviceGetMdevctlCreateCommand;
} else if (info->command == MDEVCTL_CMD_DEFINE) {
cmd = "define";
func = nodeDeviceGetMdevctlDefineCommand;
@@ -119,8 +119,8 @@ testMdevctlStartOrDefineHelper(const void *data)
jsonfile = g_strdup_printf("%s/nodedevmdevctldata/%s-%s.json", abs_srcdir,
info->filename, cmd);
- return testMdevctlStartOrDefine(info->virt_type, info->create, func,
- mdevxml, cmdlinefile, jsonfile);
+ return testMdevctlCreateOrDefine(info->virt_type, info->create, func,
+ mdevxml, cmdlinefile, jsonfile);
}
typedef virCommand* (*GetStopUndefineCmdFunc)(const char *uuid, char **errbuf);
@@ -180,9 +180,9 @@ testMdevctlUuidCommandHelper(const void *data)
} else if (info->command == MDEVCTL_CMD_UNDEFINE) {
cmd = "undefine";
func = nodeDeviceGetMdevctlUndefineCommand;
- }else if (info->command == MDEVCTL_CMD_CREATE) {
- cmd = "create";
- func = nodeDeviceGetMdevctlCreateCommand;
+ }else if (info->command == MDEVCTL_CMD_START) {
+ cmd = "start";
+ func = nodeDeviceGetMdevctlStartCommand;
} else {
return -1;
}
@@ -403,12 +403,12 @@ mymain(void)
#define DO_TEST_CMD(desc, virt_type, create, filename, command) \
do { \
struct startTestInfo info = { virt_type, create, filename, command }; \
- DO_TEST_FULL(desc, testMdevctlStartOrDefineHelper, &info); \
+ DO_TEST_FULL(desc, testMdevctlCreateOrDefineHelper, &info); \
} \
while (0)
-#define DO_TEST_START(filename) \
- DO_TEST_CMD("mdevctl start " filename, "QEMU", CREATE_DEVICE,
filename, MDEVCTL_CMD_START)
+#define DO_TEST_CREATE(filename) \
+ DO_TEST_CMD("mdevctl create " filename, "QEMU", CREATE_DEVICE,
filename, MDEVCTL_CMD_CREATE)
#define DO_TEST_DEFINE(filename) \
DO_TEST_CMD("mdevctl define " filename, "QEMU", CREATE_DEVICE,
filename, MDEVCTL_CMD_DEFINE)
@@ -426,8 +426,8 @@ mymain(void)
#define DO_TEST_UNDEFINE(filename) \
DO_TEST_UUID_COMMAND_FULL("mdevctl undefine " filename, filename,
MDEVCTL_CMD_UNDEFINE)
-#define DO_TEST_CREATE(filename) \
- DO_TEST_UUID_COMMAND_FULL("mdevctl create " filename, filename,
MDEVCTL_CMD_CREATE)
+#define DO_TEST_START(filename) \
+ DO_TEST_UUID_COMMAND_FULL("mdevctl start " filename, filename,
MDEVCTL_CMD_START)
#define DO_TEST_LIST_DEFINED() \
DO_TEST_FULL("mdevctl list --defined", testMdevctlListDefined, NULL)
@@ -435,10 +435,9 @@ mymain(void)
#define DO_TEST_PARSE_JSON(filename) \
DO_TEST_FULL("parse mdevctl json " filename, testMdevctlParse, filename)
- /* Test mdevctl start commands */
- DO_TEST_START("mdev_d069d019_36ea_4111_8f0a_8c9a70e21366");
- DO_TEST_START("mdev_fedc4916_1ca8_49ac_b176_871d16c13076");
- DO_TEST_START("mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9");
+ DO_TEST_CREATE("mdev_d069d019_36ea_4111_8f0a_8c9a70e21366");
+ DO_TEST_CREATE("mdev_fedc4916_1ca8_49ac_b176_871d16c13076");
+ DO_TEST_CREATE("mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9");
/* Test mdevctl stop command, pass an arbitrary uuid */
DO_TEST_STOP("mdev_d069d019_36ea_4111_8f0a_8c9a70e21366");
@@ -453,7 +452,7 @@ mymain(void)
DO_TEST_UNDEFINE("mdev_d069d019_36ea_4111_8f0a_8c9a70e21366");
- DO_TEST_CREATE("mdev_d069d019_36ea_4111_8f0a_8c9a70e21366");
+ DO_TEST_START("mdev_d069d019_36ea_4111_8f0a_8c9a70e21366");
done:
nodedevTestDriverFree(driver);
--
2.26.3
11:12 a.m.
New subject: [libvirt PATCH v2 04/12] nodedev: driver: Swap virMdevctlStart and virMdevctlCreate
On 4/13/21 4:39 PM, Jonathon Jongsma wrote:
From: Erik Skultety <eskultet(a)redhat.com>
"start" in libvirt means - "take this object and create an
instance out of it"
"create" in libvirt most of the time means - "take and XML description,
make an object out of it and use it to create an instance"
This gets confusing with mdevctl which uses "start" for both. So, this
patch proposes to use virMdevctlStart in cases where from libvirt's POV
we're starting a defined device (unlike mdevctl). Analogically,
"similarly" sounds less awkward to me, but this is also a word, so... :-)
use
virMdevctlCreate in scenarios where XML description is passed to
libvirt and a transient device is supposed to be created.
Signed-off-by: Erik Skultety <eskultet(a)redhat.com>
Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com>
Reviewed-by: Laine Stump <laine(a)redhat.com>
3:39 p.m.
New subject: [libvirt PATCH v2 05/12] nodedev: Switch to using long options for mdevctl
rather than using short opentions (e.g. "-p 0000:00:02.0"), use long
options everywhere (e.g. "--parent=0000:00:02.0")
Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com>
---
src/node_device/node_device_driver.c | 28 ++++++-------------
...19_36ea_4111_8f0a_8c9a70e21366-create.argv | 4 +--
...19_36ea_4111_8f0a_8c9a70e21366-define.argv | 4 +--
...39_495e_4243_ad9f_beb3f14c23d9-create.argv | 4 +--
...39_495e_4243_ad9f_beb3f14c23d9-define.argv | 4 +--
...16_1ca8_49ac_b176_871d16c13076-create.argv | 4 +--
...16_1ca8_49ac_b176_871d16c13076-define.argv | 4 +--
tests/nodedevmdevctldata/mdevctl-start.argv | 2 +-
tests/nodedevmdevctldata/mdevctl-stop.argv | 2 +-
.../nodedevmdevctldata/mdevctl-undefine.argv | 2 +-
10 files changed, 24 insertions(+), 34 deletions(-)
diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c
index dda118b0e5..56cbae0037 100644
--- a/src/node_device/node_device_driver.c
+++ b/src/node_device/node_device_driver.c
@@ -720,10 +720,9 @@ nodeDeviceGetMdevctlDefineCreateCommand(virNodeDeviceDef *def,
return NULL;
}
- cmd = virCommandNewArgList(MDEVCTL, subcommand,
- "-p", parent_addr,
- "--jsonfile", "/dev/stdin",
- NULL);
+ cmd = virCommandNewArgList(MDEVCTL, subcommand, NULL);
+ virCommandAddArgPair(cmd, "--parent", parent_addr);
+ virCommandAddArgPair(cmd, "--jsonfile", "/dev/stdin");
virCommandSetInputBuffer(cmd, json);
@@ -887,11 +886,8 @@ nodeDeviceCreateXML(virConnectPtr conn,
virCommand *
nodeDeviceGetMdevctlStopCommand(const char *uuid, char **errmsg)
{
- virCommand *cmd = virCommandNewArgList(MDEVCTL,
- "stop",
- "-u",
- uuid,
- NULL);
+ virCommand *cmd = virCommandNewArgList(MDEVCTL, "stop", NULL);
+ virCommandAddArgPair(cmd, "--uuid", uuid);
virCommandSetErrorBuffer(cmd, errmsg);
return cmd;
@@ -900,11 +896,8 @@ nodeDeviceGetMdevctlStopCommand(const char *uuid, char **errmsg)
virCommand *
nodeDeviceGetMdevctlUndefineCommand(const char *uuid, char **errmsg)
{
- virCommand *cmd = virCommandNewArgList(MDEVCTL,
- "undefine",
- "-u",
- uuid,
- NULL);
+ virCommand *cmd = virCommandNewArgList(MDEVCTL, "undefine", NULL);
+ virCommandAddArgPair(cmd, "--uuid", uuid);
virCommandSetErrorBuffer(cmd, errmsg);
return cmd;
}
@@ -912,11 +905,8 @@ nodeDeviceGetMdevctlUndefineCommand(const char *uuid, char **errmsg)
virCommand *
nodeDeviceGetMdevctlStartCommand(const char *uuid, char **errmsg)
{
- virCommand *cmd = virCommandNewArgList(MDEVCTL,
- "start",
- "-u",
- uuid,
- NULL);
+ virCommand *cmd = virCommandNewArgList(MDEVCTL, "start", NULL);
+ virCommandAddArgPair(cmd, "--uuid", uuid);
virCommandSetErrorBuffer(cmd, errmsg);
return cmd;
}
diff --git
a/tests/nodedevmdevctldata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366-create.argv
b/tests/nodedevmdevctldata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366-create.argv
index 73a8046d84..3cf66f8a91 100644
--- a/tests/nodedevmdevctldata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366-create.argv
+++ b/tests/nodedevmdevctldata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366-create.argv
@@ -1,5 +1,5 @@
mdevctl \
start \
--p 0000:00:02.0 \
---jsonfile /dev/stdin \
+--parent=0000:00:02.0 \
+--jsonfile=/dev/stdin \
--uuid=d069d019-36ea-4111-8f0a-8c9a70e21366
diff --git
a/tests/nodedevmdevctldata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366-define.argv
b/tests/nodedevmdevctldata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366-define.argv
index b8bb276f9f..a8c2feaed4 100644
--- a/tests/nodedevmdevctldata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366-define.argv
+++ b/tests/nodedevmdevctldata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366-define.argv
@@ -1,5 +1,5 @@
mdevctl \
define \
--p 0000:00:02.0 \
---jsonfile /dev/stdin \
+--parent=0000:00:02.0 \
+--jsonfile=/dev/stdin \
--uuid=d069d019-36ea-4111-8f0a-8c9a70e21366
diff --git
a/tests/nodedevmdevctldata/mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9-create.argv
b/tests/nodedevmdevctldata/mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9-create.argv
index eae63f7a7f..010e562026 100644
--- a/tests/nodedevmdevctldata/mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9-create.argv
+++ b/tests/nodedevmdevctldata/mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9-create.argv
@@ -1,4 +1,4 @@
mdevctl \
start \
--p 0000:00:02.0 \
---jsonfile /dev/stdin
+--parent=0000:00:02.0 \
+--jsonfile=/dev/stdin
diff --git
a/tests/nodedevmdevctldata/mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9-define.argv
b/tests/nodedevmdevctldata/mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9-define.argv
index bf8637ed57..46d1e95f15 100644
--- a/tests/nodedevmdevctldata/mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9-define.argv
+++ b/tests/nodedevmdevctldata/mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9-define.argv
@@ -1,4 +1,4 @@
mdevctl \
define \
--p 0000:00:02.0 \
---jsonfile /dev/stdin
+--parent=0000:00:02.0 \
+--jsonfile=/dev/stdin
diff --git
a/tests/nodedevmdevctldata/mdev_fedc4916_1ca8_49ac_b176_871d16c13076-create.argv
b/tests/nodedevmdevctldata/mdev_fedc4916_1ca8_49ac_b176_871d16c13076-create.argv
index eae63f7a7f..010e562026 100644
--- a/tests/nodedevmdevctldata/mdev_fedc4916_1ca8_49ac_b176_871d16c13076-create.argv
+++ b/tests/nodedevmdevctldata/mdev_fedc4916_1ca8_49ac_b176_871d16c13076-create.argv
@@ -1,4 +1,4 @@
mdevctl \
start \
--p 0000:00:02.0 \
---jsonfile /dev/stdin
+--parent=0000:00:02.0 \
+--jsonfile=/dev/stdin
diff --git
a/tests/nodedevmdevctldata/mdev_fedc4916_1ca8_49ac_b176_871d16c13076-define.argv
b/tests/nodedevmdevctldata/mdev_fedc4916_1ca8_49ac_b176_871d16c13076-define.argv
index bf8637ed57..46d1e95f15 100644
--- a/tests/nodedevmdevctldata/mdev_fedc4916_1ca8_49ac_b176_871d16c13076-define.argv
+++ b/tests/nodedevmdevctldata/mdev_fedc4916_1ca8_49ac_b176_871d16c13076-define.argv
@@ -1,4 +1,4 @@
mdevctl \
define \
--p 0000:00:02.0 \
---jsonfile /dev/stdin
+--parent=0000:00:02.0 \
+--jsonfile=/dev/stdin
diff --git a/tests/nodedevmdevctldata/mdevctl-start.argv
b/tests/nodedevmdevctldata/mdevctl-start.argv
index ccb8e2992c..e876d08ad3 100644
--- a/tests/nodedevmdevctldata/mdevctl-start.argv
+++ b/tests/nodedevmdevctldata/mdevctl-start.argv
@@ -1,3 +1,3 @@
mdevctl \
start \
--u d069d019-36ea-4111-8f0a-8c9a70e21366
+--uuid=d069d019-36ea-4111-8f0a-8c9a70e21366
diff --git a/tests/nodedevmdevctldata/mdevctl-stop.argv
b/tests/nodedevmdevctldata/mdevctl-stop.argv
index a07d339e47..e422b2c3ea 100644
--- a/tests/nodedevmdevctldata/mdevctl-stop.argv
+++ b/tests/nodedevmdevctldata/mdevctl-stop.argv
@@ -1,3 +1,3 @@
mdevctl \
stop \
--u d069d019-36ea-4111-8f0a-8c9a70e21366
+--uuid=d069d019-36ea-4111-8f0a-8c9a70e21366
diff --git a/tests/nodedevmdevctldata/mdevctl-undefine.argv
b/tests/nodedevmdevctldata/mdevctl-undefine.argv
index dbde32e2a8..fa35a646be 100644
--- a/tests/nodedevmdevctldata/mdevctl-undefine.argv
+++ b/tests/nodedevmdevctldata/mdevctl-undefine.argv
@@ -1,3 +1,3 @@
mdevctl \
undefine \
--u d069d019-36ea-4111-8f0a-8c9a70e21366
+--uuid=d069d019-36ea-4111-8f0a-8c9a70e21366
--
2.26.3
5:20 p.m.
New subject: [libvirt PATCH v2 05/12] nodedev: Switch to using long options for mdevctl
On 4/13/21 4:39 PM, Jonathon Jongsma wrote:
rather than using short opentions (e.g. "-p 0000:00:02.0"),
use long
options everywhere (e.g. "--parent=0000:00:02.0")
Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com>
Reviewed-by: Laine Stump <laine(a)redhat.com>
3:39 p.m.
New subject: [libvirt PATCH v2 06/12] nodedev: driver: Introduce internal mdevctl commands enum
From: Erik Skultety <eskultet(a)redhat.com>
This is not a 1:1 mapping to mdevctl because mdevctl doesn't support a
'create' command. It uses 'start' for both starting a defined device as
well as creating a transient one. To make our code more readable in
that regard and not second-guess what does a specific "start" instance
mean, use "create" internally instead only to translate it to "start"
just before executing mdevctl.
Signed-off-by: Erik Skultety <eskultet(a)redhat.com>
Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com>
---
src/node_device/node_device_driver.c | 7 +++++++
src/node_device/node_device_driver.h | 19 +++++++++++++++++++
tests/nodedevmdevctltest.c | 12 ++----------
3 files changed, 28 insertions(+), 10 deletions(-)
diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c
index 56cbae0037..da55e386f0 100644
--- a/src/node_device/node_device_driver.c
+++ b/src/node_device/node_device_driver.c
@@ -47,6 +47,13 @@
virNodeDeviceDriverState *driver;
+
+VIR_ENUM_IMPL(virMdevctlCommand,
+ MDEVCTL_CMD_LAST,
+ "start", "stop", "define",
"undefine", "create"
+);
+
+
virDrvOpenStatus
nodeConnectOpen(virConnectPtr conn,
virConnectAuthPtr auth G_GNUC_UNUSED,
diff --git a/src/node_device/node_device_driver.h b/src/node_device/node_device_driver.h
index 4101e34a8f..d06efbf354 100644
--- a/src/node_device/node_device_driver.h
+++ b/src/node_device/node_device_driver.h
@@ -33,6 +33,25 @@ int
udevNodeRegister(void);
#endif
+
+typedef enum {
+ MDEVCTL_CMD_START,
+ MDEVCTL_CMD_STOP,
+ MDEVCTL_CMD_DEFINE,
+ MDEVCTL_CMD_UNDEFINE,
+
+ /* mdevctl actually doesn't have a 'create' command, it will be replaced
+ * with 'start' eventually in nodeDeviceGetMdevctlCommand, but this clear
+ * separation makes our code more readable in terms of knowing when we're
+ * starting a defined device and when we're creating a transient one */
+ MDEVCTL_CMD_CREATE,
+
+ MDEVCTL_CMD_LAST,
+} virMdevctlCommand;
+
+VIR_ENUM_DECL(virMdevctlCommand);
+
+
void
nodeDeviceLock(void);
diff --git a/tests/nodedevmdevctltest.c b/tests/nodedevmdevctltest.c
index 188e521f59..cf8de852a8 100644
--- a/tests/nodedevmdevctltest.c
+++ b/tests/nodedevmdevctltest.c
@@ -10,19 +10,11 @@
#define VIR_FROM_THIS VIR_FROM_NODEDEV
-typedef enum {
- MDEVCTL_CMD_START,
- MDEVCTL_CMD_STOP,
- MDEVCTL_CMD_DEFINE,
- MDEVCTL_CMD_UNDEFINE,
- MDEVCTL_CMD_CREATE,
-} MdevctlCmd;
-
struct startTestInfo {
const char *virt_type;
int create;
const char *filename;
- MdevctlCmd command;
+ virMdevctlCommand command;
};
/* capture stdin passed to command */
@@ -126,7 +118,7 @@ testMdevctlCreateOrDefineHelper(const void *data)
typedef virCommand* (*GetStopUndefineCmdFunc)(const char *uuid, char **errbuf);
struct UuidCommandTestInfo {
const char *filename;
- MdevctlCmd command;
+ virMdevctlCommand command;
};
static int
--
2.26.3
5:41 a.m.
New subject: [libvirt PATCH v2 06/12] nodedev: driver: Introduce internal mdevctl commands enum
On Tue, Apr 13, 2021 at 03:39:42PM -0500, Jonathon Jongsma wrote:
From: Erik Skultety <eskultet(a)redhat.com>
This is not a 1:1 mapping to mdevctl because mdevctl doesn't support a
'create' command. It uses 'start' for both starting a defined device as
well as creating a transient one. To make our code more readable in
that regard and not second-guess what does a specific "start" instance
mean, use "create" internally instead only to translate it to
"start"
just before executing mdevctl.
Reading ^this for the second time, I think we need to rephrase the second
sentence, as it's difficult to read and that's something since I wrote it :D.
Erik
3:28 p.m.
New subject: [libvirt PATCH v2 06/12] nodedev: driver: Introduce internal mdevctl commands enum
On Wed, 14 Apr 2021 12:41:02 +0200
Erik Skultety <eskultet(a)redhat.com> wrote:
On Tue, Apr 13, 2021 at 03:39:42PM -0500, Jonathon Jongsma wrote:
> From: Erik Skultety <eskultet(a)redhat.com>
>
> This is not a 1:1 mapping to mdevctl because mdevctl doesn't
> support a 'create' command. It uses 'start' for both starting a
> defined device as well as creating a transient one. To make our
> code more readable in that regard and not second-guess what does a
> specific "start" instance mean, use "create" internally instead
> only to translate it to "start" just before executing mdevctl.
Reading ^this for the second time, I think we need to rephrase the
second sentence, as it's difficult to read and that's something since
I wrote it :D.
heh, true.
How about something like:
This is not a 1:1 mapping to mdevctl commands because mdevctl doesn't support a
separate 'create' command. mdevctl uses 'start' for both starting a
pre-defined
device as well and for creating and starting a new transient device. The
libvirt code will be more readable if we treat these as separate commands. When
we need to actually execute mdevctl, the 'create' command will be translated
into the appropriate 'mdevctl start' command.
12:54 a.m.
New subject: [libvirt PATCH v2 06/12] nodedev: driver: Introduce internal mdevctl commands enum
On Wed, Apr 14, 2021 at 03:28:23PM -0500, Jonathon Jongsma wrote:
On Wed, 14 Apr 2021 12:41:02 +0200
Erik Skultety <eskultet(a)redhat.com> wrote:
> On Tue, Apr 13, 2021 at 03:39:42PM -0500, Jonathon Jongsma wrote:
> > From: Erik Skultety <eskultet(a)redhat.com>
> >
> > This is not a 1:1 mapping to mdevctl because mdevctl doesn't
> > support a 'create' command. It uses 'start' for both starting
a
> > defined device as well as creating a transient one. To make our
> > code more readable in that regard and not second-guess what does a
> > specific "start" instance mean, use "create" internally
instead
> > only to translate it to "start" just before executing mdevctl.
>
> Reading ^this for the second time, I think we need to rephrase the
> second sentence, as it's difficult to read and that's something since
> I wrote it :D.
>
heh, true.
How about something like:
This is not a 1:1 mapping to mdevctl commands because mdevctl doesn't support a
separate 'create' command. mdevctl uses 'start' for both starting a
pre-defined
device as well and for creating and starting a new transient device. The
libvirt code will be more readable if we treat these as separate commands. When
we need to actually execute mdevctl, the 'create' command will be translated
into the appropriate 'mdevctl start' command.
Sounds good, please go with that.
Erik
3:39 p.m.
New subject: [libvirt PATCH v2 07/12] nodedev: driver: Create a generic mdevctl command translator
From: Erik Skultety <eskultet(a)redhat.com>
Currently there are dedicated wrappers to construct mdevctl command.
These are mostly fine except for the one that translates both "start"
and "define" commands, only because mdevctl takes the same set of
arguments. Instead, keep the wrappers, but let them call a single
global translator that handles all the mdevctl command differences and
commonalities.
Signed-off-by: Erik Skultety <eskultet(a)redhat.com>
Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com>
---
src/node_device/node_device_driver.c | 125 ++++++++++++++++-----------
src/node_device/node_device_driver.h | 6 +-
tests/nodedevmdevctltest.c | 4 +-
3 files changed, 79 insertions(+), 56 deletions(-)
diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c
index da55e386f0..bbb01c3967 100644
--- a/src/node_device/node_device_driver.c
+++ b/src/node_device/node_device_driver.c
@@ -702,42 +702,75 @@ nodeDeviceFindAddressByName(const char *name)
}
-/* the mdevctl 'start' and 'define' commands accept almost the exact
same
- * arguments, so provide a common implementation that can be wrapped by a more
- * specific function */
-static virCommand*
-nodeDeviceGetMdevctlDefineCreateCommand(virNodeDeviceDef *def,
- const char *subcommand,
- char **uuid_out,
- char **errmsg)
+static virCommand *
+nodeDeviceGetMdevctlCommand(virNodeDeviceDef *def,
+ virMdevctlCommand cmd_type,
+ char **outbuf,
+ char **errbuf)
{
- virCommand *cmd;
- g_autofree char *json = NULL;
- g_autofree char *parent_addr = nodeDeviceFindAddressByName(def->parent);
-
- if (!parent_addr) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("unable to find parent device '%s'"),
def->parent);
- return NULL;
+ g_autofree char *parent_addr = NULL;
+ virCommand *cmd = NULL;
+ const char *subcommand = virMdevctlCommandTypeToString(cmd_type);
+ g_autofree char *inbuf = NULL;
+
+ switch (cmd_type) {
+ case MDEVCTL_CMD_CREATE:
+ /* now is the time to make sure "create" is replaced with
"start" on
+ * mdevctl cmdline */
+ cmd = virCommandNewArgList(MDEVCTL, "start", NULL);
+ break;
+ case MDEVCTL_CMD_STOP:
+ case MDEVCTL_CMD_START:
+ case MDEVCTL_CMD_DEFINE:
+ case MDEVCTL_CMD_UNDEFINE:
+ cmd = virCommandNewArgList(MDEVCTL, subcommand, NULL);
+ break;
+ case MDEVCTL_CMD_LAST:
+ default:
+ /* SHOULD NEVER HAPPEN */
+ break;
}
- if (nodeDeviceDefToMdevctlConfig(def, &json) < 0) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("couldn't convert node device def to mdevctl
JSON"));
- return NULL;
- }
+ switch (cmd_type) {
+ case MDEVCTL_CMD_CREATE:
+ case MDEVCTL_CMD_DEFINE:
+ parent_addr = nodeDeviceFindAddressByName(def->parent);
- cmd = virCommandNewArgList(MDEVCTL, subcommand, NULL);
- virCommandAddArgPair(cmd, "--parent", parent_addr);
- virCommandAddArgPair(cmd, "--jsonfile", "/dev/stdin");
+ if (!parent_addr) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("unable to find parent device '%s'"),
def->parent);
+ return NULL;
+ }
+
+ if (nodeDeviceDefToMdevctlConfig(def, &inbuf) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("couldn't convert node device def to mdevctl
JSON"));
+ return NULL;
+ }
- virCommandSetInputBuffer(cmd, json);
+ virCommandAddArgPair(cmd, "--parent", parent_addr);
+ virCommandAddArgPair(cmd, "--jsonfile", "/dev/stdin");
+
+ virCommandSetInputBuffer(cmd, inbuf);
+ virCommandSetOutputBuffer(cmd, outbuf);
+ break;
+
+ case MDEVCTL_CMD_UNDEFINE:
+ case MDEVCTL_CMD_STOP:
+ case MDEVCTL_CMD_START:
+ /* No special handling here, we only need to pass UUID with these */
+ break;
+ case MDEVCTL_CMD_LAST:
+ default:
+ /* SHOULD NEVER HAPPEN */
+ break;
+ }
+ /* Fill in UUID for commands that need it */
if (def->caps->data.mdev.uuid)
virCommandAddArgPair(cmd, "--uuid", def->caps->data.mdev.uuid);
- virCommandSetOutputBuffer(cmd, uuid_out);
- virCommandSetErrorBuffer(cmd, errmsg);
+ virCommandSetErrorBuffer(cmd, errbuf);
return cmd;
}
@@ -747,8 +780,7 @@ nodeDeviceGetMdevctlCreateCommand(virNodeDeviceDef *def,
char **uuid_out,
char **errmsg)
{
- return nodeDeviceGetMdevctlDefineCreateCommand(def, "start", uuid_out,
- errmsg);
+ return nodeDeviceGetMdevctlCommand(def, MDEVCTL_CMD_CREATE, uuid_out, errmsg);
}
virCommand*
@@ -756,8 +788,7 @@ nodeDeviceGetMdevctlDefineCommand(virNodeDeviceDef *def,
char **uuid_out,
char **errmsg)
{
- return nodeDeviceGetMdevctlDefineCreateCommand(def, "define", uuid_out,
- errmsg);
+ return nodeDeviceGetMdevctlCommand(def, MDEVCTL_CMD_DEFINE, uuid_out, errmsg);
}
@@ -891,31 +922,24 @@ nodeDeviceCreateXML(virConnectPtr conn,
virCommand *
-nodeDeviceGetMdevctlStopCommand(const char *uuid, char **errmsg)
+nodeDeviceGetMdevctlStopCommand(virNodeDeviceDef *def,
+ char **errmsg)
{
- virCommand *cmd = virCommandNewArgList(MDEVCTL, "stop", NULL);
- virCommandAddArgPair(cmd, "--uuid", uuid);
- virCommandSetErrorBuffer(cmd, errmsg);
- return cmd;
-
+ return nodeDeviceGetMdevctlCommand(def, MDEVCTL_CMD_STOP, NULL, errmsg);
}
virCommand *
-nodeDeviceGetMdevctlUndefineCommand(const char *uuid, char **errmsg)
+nodeDeviceGetMdevctlUndefineCommand(virNodeDeviceDef *def,
+ char **errmsg)
{
- virCommand *cmd = virCommandNewArgList(MDEVCTL, "undefine", NULL);
- virCommandAddArgPair(cmd, "--uuid", uuid);
- virCommandSetErrorBuffer(cmd, errmsg);
- return cmd;
+ return nodeDeviceGetMdevctlCommand(def, MDEVCTL_CMD_UNDEFINE, NULL, errmsg);
}
virCommand *
-nodeDeviceGetMdevctlStartCommand(const char *uuid, char **errmsg)
+nodeDeviceGetMdevctlStartCommand(virNodeDeviceDef *def,
+ char **errmsg)
{
- virCommand *cmd = virCommandNewArgList(MDEVCTL, "start", NULL);
- virCommandAddArgPair(cmd, "--uuid", uuid);
- virCommandSetErrorBuffer(cmd, errmsg);
- return cmd;
+ return nodeDeviceGetMdevctlCommand(def, MDEVCTL_CMD_START, NULL, errmsg);
}
static int
@@ -924,7 +948,7 @@ virMdevctlStop(virNodeDeviceDef *def, char **errmsg)
int status;
g_autoptr(virCommand) cmd = NULL;
- cmd = nodeDeviceGetMdevctlStopCommand(def->caps->data.mdev.uuid, errmsg);
+ cmd = nodeDeviceGetMdevctlStopCommand(def, errmsg);
if (virCommandRun(cmd, &status) < 0 || status != 0)
return -1;
@@ -939,8 +963,7 @@ virMdevctlUndefine(virNodeDeviceDef *def, char **errmsg)
int status;
g_autoptr(virCommand) cmd = NULL;
- cmd = nodeDeviceGetMdevctlUndefineCommand(def->caps->data.mdev.uuid,
- errmsg);
+ cmd = nodeDeviceGetMdevctlUndefineCommand(def, errmsg);
if (virCommandRun(cmd, &status) < 0 || status != 0)
return -1;
@@ -955,7 +978,7 @@ virMdevctlStart(virNodeDeviceDef *def, char **errmsg)
int status;
g_autoptr(virCommand) cmd = NULL;
- cmd = nodeDeviceGetMdevctlStartCommand(def->caps->data.mdev.uuid, errmsg);
+ cmd = nodeDeviceGetMdevctlStartCommand(def, errmsg);
if (virCommandRun(cmd, &status) < 0 || status != 0)
return -1;
diff --git a/src/node_device/node_device_driver.h b/src/node_device/node_device_driver.h
index d06efbf354..105a71dd93 100644
--- a/src/node_device/node_device_driver.h
+++ b/src/node_device/node_device_driver.h
@@ -152,11 +152,11 @@ nodeDeviceGetMdevctlDefineCommand(virNodeDeviceDef *def,
char **errmsg);
virCommand *
-nodeDeviceGetMdevctlStopCommand(const char *uuid,
+nodeDeviceGetMdevctlStopCommand(virNodeDeviceDef *def,
char **errmsg);
virCommand *
-nodeDeviceGetMdevctlUndefineCommand(const char *uuid,
+nodeDeviceGetMdevctlUndefineCommand(virNodeDeviceDef *def,
char **errmsg);
virCommand *
@@ -181,7 +181,7 @@ bool nodeDeviceDefCopyFromMdevctl(virNodeDeviceDef *dst,
virNodeDeviceDef *src);
virCommand*
-nodeDeviceGetMdevctlStartCommand(const char *uuid,
+nodeDeviceGetMdevctlStartCommand(virNodeDeviceDef *def,
char **errmsg);
int
diff --git a/tests/nodedevmdevctltest.c b/tests/nodedevmdevctltest.c
index cf8de852a8..188bad6e53 100644
--- a/tests/nodedevmdevctltest.c
+++ b/tests/nodedevmdevctltest.c
@@ -115,7 +115,7 @@ testMdevctlCreateOrDefineHelper(const void *data)
mdevxml, cmdlinefile, jsonfile);
}
-typedef virCommand* (*GetStopUndefineCmdFunc)(const char *uuid, char **errbuf);
+typedef virCommand* (*GetStopUndefineCmdFunc)(virNodeDeviceDef *def, char **errbuf);
struct UuidCommandTestInfo {
const char *filename;
virMdevctlCommand command;
@@ -136,7 +136,7 @@ testMdevctlUuidCommand(GetStopUndefineCmdFunc func,
if (!(def = virNodeDeviceDefParseFile(mdevxml, EXISTING_DEVICE, "QEMU")))
goto cleanup;
- cmd = func(def->caps->data.mdev.uuid, &errmsg);
+ cmd = func(def, &errmsg);
if (!cmd)
goto cleanup;
--
2.26.3
11:18 a.m.
New subject: [libvirt PATCH v2 07/12] nodedev: driver: Create a generic mdevctl command translator
On 4/13/21 4:39 PM, Jonathon Jongsma wrote:
From: Erik Skultety <eskultet(a)redhat.com>
Currently there are dedicated wrappers to construct mdevctl command.
These are mostly fine except for the one that translates both "start"
and "define" commands, only because mdevctl takes the same set of
arguments. Instead, keep the wrappers, but let them call a single
global translator that handles all the mdevctl command differences and
commonalities.
Signed-off-by: Erik Skultety <eskultet(a)redhat.com>
Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com>
Reviewed-by: Laine Stump <laine(a)redhat.com>
6:34 a.m.
New subject: [libvirt PATCH v2 07/12] nodedev: driver: Create a generic mdevctl command translator
On 4/13/21 4:39 PM, Jonathon Jongsma wrote:
From: Erik Skultety <eskultet(a)redhat.com>
Currently there are dedicated wrappers to construct mdevctl command.
These are mostly fine except for the one that translates both "start"
and "define" commands, only because mdevctl takes the same set of
arguments. Instead, keep the wrappers, but let them call a single
global translator that handles all the mdevctl command differences and
commonalities.
Signed-off-by: Erik Skultety <eskultet(a)redhat.com>
Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com>
---
src/node_device/node_device_driver.c | 125 ++++++++++++++++-----------
src/node_device/node_device_driver.h | 6 +-
tests/nodedevmdevctltest.c | 4 +-
3 files changed, 79 insertions(+), 56 deletions(-)
Coverity found some issues...
diff --git a/src/node_device/node_device_driver.c
b/src/node_device/node_device_driver.c
index da55e386f0..bbb01c3967 100644
--- a/src/node_device/node_device_driver.c
+++ b/src/node_device/node_device_driver.c
@@ -702,42 +702,75 @@ nodeDeviceFindAddressByName(const char *name)
}
-/* the mdevctl 'start' and 'define' commands accept almost the exact
same
- * arguments, so provide a common implementation that can be wrapped by a more
- * specific function */
-static virCommand*
-nodeDeviceGetMdevctlDefineCreateCommand(virNodeDeviceDef *def,
- const char *subcommand,
- char **uuid_out,
- char **errmsg)
+static virCommand *
+nodeDeviceGetMdevctlCommand(virNodeDeviceDef *def,
+ virMdevctlCommand cmd_type,
+ char **outbuf,
+ char **errbuf)
{
- virCommand *cmd;
- g_autofree char *json = NULL;
- g_autofree char *parent_addr = nodeDeviceFindAddressByName(def->parent);
-
- if (!parent_addr) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("unable to find parent device '%s'"),
def->parent);
- return NULL;
+ g_autofree char *parent_addr = NULL;
+ virCommand *cmd = NULL;
+ const char *subcommand = virMdevctlCommandTypeToString(cmd_type);
+ g_autofree char *inbuf = NULL;
+
+ switch (cmd_type) {
+ case MDEVCTL_CMD_CREATE:
+ /* now is the time to make sure "create" is replaced with
"start" on
+ * mdevctl cmdline */
+ cmd = virCommandNewArgList(MDEVCTL, "start", NULL);
+ break;
+ case MDEVCTL_CMD_STOP:
+ case MDEVCTL_CMD_START:
+ case MDEVCTL_CMD_DEFINE:
+ case MDEVCTL_CMD_UNDEFINE:
+ cmd = virCommandNewArgList(MDEVCTL, subcommand, NULL);
+ break;
+ case MDEVCTL_CMD_LAST:
+ default:
+ /* SHOULD NEVER HAPPEN */
Shouldn't happen, but allows code to fall thru w/ later access to @cmd
which will fail. It's a false positive technically...
+ break;
}
- if (nodeDeviceDefToMdevctlConfig(def, &json) < 0) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("couldn't convert node device def to mdevctl
JSON"));
- return NULL;
- }
+ switch (cmd_type) {
+ case MDEVCTL_CMD_CREATE:
+ case MDEVCTL_CMD_DEFINE:
+ parent_addr = nodeDeviceFindAddressByName(def->parent);
- cmd = virCommandNewArgList(MDEVCTL, subcommand, NULL);
- virCommandAddArgPair(cmd, "--parent", parent_addr);
- virCommandAddArgPair(cmd, "--jsonfile", "/dev/stdin");
+ if (!parent_addr) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("unable to find parent device '%s'"),
def->parent);
Leaks @cmd
+ return NULL;
+ }
+
+ if (nodeDeviceDefToMdevctlConfig(def, &inbuf) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("couldn't convert node device def to mdevctl
JSON"));
Leaks @cmd
+ return NULL;
+ }
- virCommandSetInputBuffer(cmd, json);
+ virCommandAddArgPair(cmd, "--parent", parent_addr);
+ virCommandAddArgPair(cmd, "--jsonfile", "/dev/stdin");
+
+ virCommandSetInputBuffer(cmd, inbuf);
+ virCommandSetOutputBuffer(cmd, outbuf);
+ break;
+
+ case MDEVCTL_CMD_UNDEFINE:
+ case MDEVCTL_CMD_STOP:
+ case MDEVCTL_CMD_START:
+ /* No special handling here, we only need to pass UUID with these */
+ break;
+ case MDEVCTL_CMD_LAST:
+ default:
+ /* SHOULD NEVER HAPPEN */
+ break;
+ }
[...]
Hopefully I haven't missed some other patch along the way as I don't
keep up to date as much any more. Coverity just keeps running though...
John
3:39 p.m.
New subject: [libvirt PATCH v2 08/12] nodedev: Remove GetMdevctl*Command() wrappers
These per-command generator functions were only exposed in the header to
allow the commandline generation to be tested. Now that we have a
generic mdevctl command generator, we can get rid of the per-command
wrappers and reduce the noise in the header.
Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com>
---
src/node_device/node_device_driver.c | 56 ++++++----------------------
src/node_device/node_device_driver.h | 24 ++----------
tests/nodedevmdevctltest.c | 41 ++++----------------
3 files changed, 23 insertions(+), 98 deletions(-)
diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c
index bbb01c3967..b8e68d3b27 100644
--- a/src/node_device/node_device_driver.c
+++ b/src/node_device/node_device_driver.c
@@ -702,7 +702,7 @@ nodeDeviceFindAddressByName(const char *name)
}
-static virCommand *
+virCommand *
nodeDeviceGetMdevctlCommand(virNodeDeviceDef *def,
virMdevctlCommand cmd_type,
char **outbuf,
@@ -775,30 +775,15 @@ nodeDeviceGetMdevctlCommand(virNodeDeviceDef *def,
return cmd;
}
-virCommand*
-nodeDeviceGetMdevctlCreateCommand(virNodeDeviceDef *def,
- char **uuid_out,
- char **errmsg)
-{
- return nodeDeviceGetMdevctlCommand(def, MDEVCTL_CMD_CREATE, uuid_out, errmsg);
-}
-
-virCommand*
-nodeDeviceGetMdevctlDefineCommand(virNodeDeviceDef *def,
- char **uuid_out,
- char **errmsg)
-{
- return nodeDeviceGetMdevctlCommand(def, MDEVCTL_CMD_DEFINE, uuid_out, errmsg);
-}
-
-
static int
virMdevctlCreate(virNodeDeviceDef *def, char **uuid, char **errmsg)
{
int status;
- g_autoptr(virCommand) cmd = nodeDeviceGetMdevctlCreateCommand(def, uuid,
- errmsg);
+ g_autoptr(virCommand) cmd = nodeDeviceGetMdevctlCommand(def,
+ MDEVCTL_CMD_CREATE,
+ uuid,
+ errmsg);
if (!cmd)
return -1;
@@ -818,7 +803,9 @@ static int
virMdevctlDefine(virNodeDeviceDef *def, char **uuid, char **errmsg)
{
int status;
- g_autoptr(virCommand) cmd = nodeDeviceGetMdevctlDefineCommand(def, uuid, errmsg);
+ g_autoptr(virCommand) cmd = nodeDeviceGetMdevctlCommand(def,
+ MDEVCTL_CMD_DEFINE,
+ uuid, errmsg);
if (!cmd)
return -1;
@@ -921,34 +908,13 @@ nodeDeviceCreateXML(virConnectPtr conn,
}
-virCommand *
-nodeDeviceGetMdevctlStopCommand(virNodeDeviceDef *def,
- char **errmsg)
-{
- return nodeDeviceGetMdevctlCommand(def, MDEVCTL_CMD_STOP, NULL, errmsg);
-}
-
-virCommand *
-nodeDeviceGetMdevctlUndefineCommand(virNodeDeviceDef *def,
- char **errmsg)
-{
- return nodeDeviceGetMdevctlCommand(def, MDEVCTL_CMD_UNDEFINE, NULL, errmsg);
-}
-
-virCommand *
-nodeDeviceGetMdevctlStartCommand(virNodeDeviceDef *def,
- char **errmsg)
-{
- return nodeDeviceGetMdevctlCommand(def, MDEVCTL_CMD_START, NULL, errmsg);
-}
-
static int
virMdevctlStop(virNodeDeviceDef *def, char **errmsg)
{
int status;
g_autoptr(virCommand) cmd = NULL;
- cmd = nodeDeviceGetMdevctlStopCommand(def, errmsg);
+ cmd = nodeDeviceGetMdevctlCommand(def, MDEVCTL_CMD_STOP, NULL, errmsg);
if (virCommandRun(cmd, &status) < 0 || status != 0)
return -1;
@@ -963,7 +929,7 @@ virMdevctlUndefine(virNodeDeviceDef *def, char **errmsg)
int status;
g_autoptr(virCommand) cmd = NULL;
- cmd = nodeDeviceGetMdevctlUndefineCommand(def, errmsg);
+ cmd = nodeDeviceGetMdevctlCommand(def, MDEVCTL_CMD_UNDEFINE, NULL, errmsg);
if (virCommandRun(cmd, &status) < 0 || status != 0)
return -1;
@@ -978,7 +944,7 @@ virMdevctlStart(virNodeDeviceDef *def, char **errmsg)
int status;
g_autoptr(virCommand) cmd = NULL;
- cmd = nodeDeviceGetMdevctlStartCommand(def, errmsg);
+ cmd = nodeDeviceGetMdevctlCommand(def, MDEVCTL_CMD_START, NULL, errmsg);
if (virCommandRun(cmd, &status) < 0 || status != 0)
return -1;
diff --git a/src/node_device/node_device_driver.h b/src/node_device/node_device_driver.h
index 105a71dd93..edd763f0e4 100644
--- a/src/node_device/node_device_driver.h
+++ b/src/node_device/node_device_driver.h
@@ -142,22 +142,10 @@ nodeConnectNodeDeviceEventDeregisterAny(virConnectPtr conn,
int callbackID);
virCommand *
-nodeDeviceGetMdevctlCreateCommand(virNodeDeviceDef *def,
- char **uuid_out,
- char **errmsg);
-
-virCommand*
-nodeDeviceGetMdevctlDefineCommand(virNodeDeviceDef *def,
- char **uuid_out,
- char **errmsg);
-
-virCommand *
-nodeDeviceGetMdevctlStopCommand(virNodeDeviceDef *def,
- char **errmsg);
-
-virCommand *
-nodeDeviceGetMdevctlUndefineCommand(virNodeDeviceDef *def,
- char **errmsg);
+nodeDeviceGetMdevctlCommand(virNodeDeviceDef *def,
+ virMdevctlCommand cmd_type,
+ char **outbuf,
+ char **errbuf);
virCommand *
nodeDeviceGetMdevctlListCommand(bool defined,
@@ -180,10 +168,6 @@ nodeDeviceGenerateName(virNodeDeviceDef *def,
bool nodeDeviceDefCopyFromMdevctl(virNodeDeviceDef *dst,
virNodeDeviceDef *src);
-virCommand*
-nodeDeviceGetMdevctlStartCommand(virNodeDeviceDef *def,
- char **errmsg);
-
int
nodeDeviceCreate(virNodeDevice *dev,
unsigned int flags);
diff --git a/tests/nodedevmdevctltest.c b/tests/nodedevmdevctltest.c
index 188bad6e53..64ce7fec46 100644
--- a/tests/nodedevmdevctltest.c
+++ b/tests/nodedevmdevctltest.c
@@ -38,7 +38,7 @@ typedef virCommand* (*MdevctlCmdFunc)(virNodeDeviceDef *, char **, char
**);
static int
testMdevctlCreateOrDefine(const char *virt_type,
int create,
- MdevctlCmdFunc mdevctl_cmd_func,
+ virMdevctlCommand cmd_type,
const char *mdevxml,
const char *cmdfile,
const char *jsonfile)
@@ -59,7 +59,7 @@ testMdevctlCreateOrDefine(const char *virt_type,
/* this function will set a stdin buffer containing the json configuration
* of the device. The json value is captured in the callback above */
- cmd = mdevctl_cmd_func(def, &uuid, &errmsg);
+ cmd = nodeDeviceGetMdevctlCommand(def, cmd_type, &uuid, &errmsg);
if (!cmd)
goto cleanup;
@@ -88,22 +88,11 @@ static int
testMdevctlCreateOrDefineHelper(const void *data)
{
const struct startTestInfo *info = data;
- const char *cmd;
- MdevctlCmdFunc func;
+ const char *cmd = virMdevctlCommandTypeToString(info->command);
g_autofree char *mdevxml = NULL;
g_autofree char *cmdlinefile = NULL;
g_autofree char *jsonfile = NULL;
- if (info->command == MDEVCTL_CMD_CREATE) {
- cmd = "create";
- func = nodeDeviceGetMdevctlCreateCommand;
- } else if (info->command == MDEVCTL_CMD_DEFINE) {
- cmd = "define";
- func = nodeDeviceGetMdevctlDefineCommand;
- } else {
- return -1;
- }
-
mdevxml = g_strdup_printf("%s/nodedevschemadata/%s.xml", abs_srcdir,
info->filename);
cmdlinefile = g_strdup_printf("%s/nodedevmdevctldata/%s-%s.argv",
@@ -111,7 +100,7 @@ testMdevctlCreateOrDefineHelper(const void *data)
jsonfile = g_strdup_printf("%s/nodedevmdevctldata/%s-%s.json", abs_srcdir,
info->filename, cmd);
- return testMdevctlCreateOrDefine(info->virt_type, info->create, func,
+ return testMdevctlCreateOrDefine(info->virt_type, info->create,
info->command,
mdevxml, cmdlinefile, jsonfile);
}
@@ -122,7 +111,7 @@ struct UuidCommandTestInfo {
};
static int
-testMdevctlUuidCommand(GetStopUndefineCmdFunc func,
+testMdevctlUuidCommand(virMdevctlCommand command,
const char *mdevxml, const char *outfile)
{
g_autoptr(virNodeDeviceDef) def = NULL;
@@ -136,7 +125,7 @@ testMdevctlUuidCommand(GetStopUndefineCmdFunc func,
if (!(def = virNodeDeviceDefParseFile(mdevxml, EXISTING_DEVICE, "QEMU")))
goto cleanup;
- cmd = func(def, &errmsg);
+ cmd = nodeDeviceGetMdevctlCommand(def, command, NULL, &errmsg);
if (!cmd)
goto cleanup;
@@ -161,30 +150,16 @@ static int
testMdevctlUuidCommandHelper(const void *data)
{
const struct UuidCommandTestInfo *info = data;
- GetStopUndefineCmdFunc func;
- const char *cmd;
+ const char *cmd = virMdevctlCommandTypeToString(info->command);
g_autofree char *cmdlinefile = NULL;
g_autofree char *mdevxml = NULL;
- if (info->command == MDEVCTL_CMD_STOP) {
- cmd = "stop";
- func = nodeDeviceGetMdevctlStopCommand;
- } else if (info->command == MDEVCTL_CMD_UNDEFINE) {
- cmd = "undefine";
- func = nodeDeviceGetMdevctlUndefineCommand;
- }else if (info->command == MDEVCTL_CMD_START) {
- cmd = "start";
- func = nodeDeviceGetMdevctlStartCommand;
- } else {
- return -1;
- }
-
mdevxml = g_strdup_printf("%s/nodedevschemadata/%s.xml", abs_srcdir,
info->filename);
cmdlinefile = g_strdup_printf("%s/nodedevmdevctldata/mdevctl-%s.argv",
abs_srcdir, cmd);
- return testMdevctlUuidCommand(func, mdevxml, cmdlinefile);
+ return testMdevctlUuidCommand(info->command, mdevxml, cmdlinefile);
}
static int
--
2.26.3
11:19 a.m.
New subject: [libvirt PATCH v2 08/12] nodedev: Remove GetMdevctl*Command() wrappers
On 4/13/21 4:39 PM, Jonathon Jongsma wrote:
These per-command generator functions were only exposed in the header
to
allow the commandline generation to be tested. Now that we have a
generic mdevctl command generator, we can get rid of the per-command
wrappers and reduce the noise in the header.
Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com>
Reviewed-by: Laine Stump <laine(a)redhat.com>
3:39 p.m.
New subject: [libvirt PATCH v2 09/12] tests: nodedev: Make the mdevctl test function and helper generic
From: Erik Skultety <eskultet(a)redhat.com>
Now that we have a generic mdevctl command generator, we can unify the
test infrastructure as well.
Signed-off-by: Erik Skultety <eskultet(a)redhat.com>
---
...19_36ea_4111_8f0a_8c9a70e21366-start.argv} | 0
...019_36ea_4111_8f0a_8c9a70e21366-stop.argv} | 0
...36ea_4111_8f0a_8c9a70e21366-undefine.argv} | 0
tests/nodedevmdevctltest.c | 120 +++++-------------
4 files changed, 31 insertions(+), 89 deletions(-)
rename tests/nodedevmdevctldata/{mdevctl-start.argv =>
mdev_d069d019_36ea_4111_8f0a_8c9a70e21366-start.argv} (100%)
rename tests/nodedevmdevctldata/{mdevctl-stop.argv =>
mdev_d069d019_36ea_4111_8f0a_8c9a70e21366-stop.argv} (100%)
rename tests/nodedevmdevctldata/{mdevctl-undefine.argv =>
mdev_d069d019_36ea_4111_8f0a_8c9a70e21366-undefine.argv} (100%)
diff --git a/tests/nodedevmdevctldata/mdevctl-start.argv
b/tests/nodedevmdevctldata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366-start.argv
similarity index 100%
rename from tests/nodedevmdevctldata/mdevctl-start.argv
rename to tests/nodedevmdevctldata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366-start.argv
diff --git a/tests/nodedevmdevctldata/mdevctl-stop.argv
b/tests/nodedevmdevctldata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366-stop.argv
similarity index 100%
rename from tests/nodedevmdevctldata/mdevctl-stop.argv
rename to tests/nodedevmdevctldata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366-stop.argv
diff --git a/tests/nodedevmdevctldata/mdevctl-undefine.argv
b/tests/nodedevmdevctldata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366-undefine.argv
similarity index 100%
rename from tests/nodedevmdevctldata/mdevctl-undefine.argv
rename to
tests/nodedevmdevctldata/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366-undefine.argv
diff --git a/tests/nodedevmdevctltest.c b/tests/nodedevmdevctltest.c
index 64ce7fec46..bba521e563 100644
--- a/tests/nodedevmdevctltest.c
+++ b/tests/nodedevmdevctltest.c
@@ -10,7 +10,7 @@
#define VIR_FROM_THIS VIR_FROM_NODEDEV
-struct startTestInfo {
+struct TestInfo {
const char *virt_type;
int create;
const char *filename;
@@ -32,24 +32,24 @@ testCommandDryRunCallback(const char *const*args G_GNUC_UNUSED,
*stdinbuf = g_strdup(input);
}
-typedef virCommand* (*MdevctlCmdFunc)(virNodeDeviceDef *, char **, char **);
+typedef virCommand * (*MdevctlCmdFunc)(virNodeDeviceDef *, char **, char **);
static int
-testMdevctlCreateOrDefine(const char *virt_type,
- int create,
- virMdevctlCommand cmd_type,
- const char *mdevxml,
- const char *cmdfile,
- const char *jsonfile)
+testMdevctlCmd(const char *virt_type,
+ int create,
+ virMdevctlCommand cmd_type,
+ const char *mdevxml,
+ const char *cmdfile,
+ const char *jsonfile)
{
g_autoptr(virNodeDeviceDef) def = NULL;
virNodeDeviceObj *obj = NULL;
g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;
const char *actualCmdline = NULL;
int ret = -1;
- g_autofree char *uuid = NULL;
- g_autofree char *errmsg = NULL;
+ g_autofree char *outbuf = NULL;
+ g_autofree char *errbuf = NULL;
g_autofree char *stdinbuf = NULL;
g_autoptr(virCommand) cmd = NULL;
g_autoptr(virCommandDryRunToken) dryRunToken = virCommandDryRunTokenNew();
@@ -59,12 +59,17 @@ testMdevctlCreateOrDefine(const char *virt_type,
/* this function will set a stdin buffer containing the json configuration
* of the device. The json value is captured in the callback above */
- cmd = nodeDeviceGetMdevctlCommand(def, cmd_type, &uuid, &errmsg);
+ cmd = nodeDeviceGetMdevctlCommand(def, cmd_type, &outbuf, &errbuf);
if (!cmd)
goto cleanup;
- virCommandSetDryRun(dryRunToken, &buf, true, true, testCommandDryRunCallback,
&stdinbuf);
+ if (create)
+ virCommandSetDryRun(dryRunToken, &buf, true, true,
+ testCommandDryRunCallback, &stdinbuf);
+ else
+ virCommandSetDryRun(dryRunToken, &buf, true, true, NULL, NULL);
+
if (virCommandRun(cmd, NULL) < 0)
goto cleanup;
@@ -74,7 +79,7 @@ testMdevctlCreateOrDefine(const char *virt_type,
if (virTestCompareToFileFull(actualCmdline, cmdfile, false) < 0)
goto cleanup;
- if (virTestCompareToFile(stdinbuf, jsonfile) < 0)
+ if (create && virTestCompareToFile(stdinbuf, jsonfile) < 0)
goto cleanup;
ret = 0;
@@ -84,10 +89,11 @@ testMdevctlCreateOrDefine(const char *virt_type,
return ret;
}
+
static int
-testMdevctlCreateOrDefineHelper(const void *data)
+testMdevctlHelper(const void *data)
{
- const struct startTestInfo *info = data;
+ const struct TestInfo *info = data;
const char *cmd = virMdevctlCommandTypeToString(info->command);
g_autofree char *mdevxml = NULL;
g_autofree char *cmdlinefile = NULL;
@@ -100,67 +106,10 @@ testMdevctlCreateOrDefineHelper(const void *data)
jsonfile = g_strdup_printf("%s/nodedevmdevctldata/%s-%s.json", abs_srcdir,
info->filename, cmd);
- return testMdevctlCreateOrDefine(info->virt_type, info->create,
info->command,
- mdevxml, cmdlinefile, jsonfile);
-}
-
-typedef virCommand* (*GetStopUndefineCmdFunc)(virNodeDeviceDef *def, char **errbuf);
-struct UuidCommandTestInfo {
- const char *filename;
- virMdevctlCommand command;
-};
-
-static int
-testMdevctlUuidCommand(virMdevctlCommand command,
- const char *mdevxml, const char *outfile)
-{
- g_autoptr(virNodeDeviceDef) def = NULL;
- g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;
- const char *actualCmdline = NULL;
- int ret = -1;
- g_autoptr(virCommand) cmd = NULL;
- g_autofree char *errmsg = NULL;
- g_autoptr(virCommandDryRunToken) dryRunToken = virCommandDryRunTokenNew();
-
- if (!(def = virNodeDeviceDefParseFile(mdevxml, EXISTING_DEVICE, "QEMU")))
- goto cleanup;
-
- cmd = nodeDeviceGetMdevctlCommand(def, command, NULL, &errmsg);
-
- if (!cmd)
- goto cleanup;
-
- virCommandSetDryRun(dryRunToken, &buf, true, true, NULL, NULL);
- if (virCommandRun(cmd, NULL) < 0)
- goto cleanup;
-
- if (!(actualCmdline = virBufferCurrentContent(&buf)))
- goto cleanup;
-
- if (virTestCompareToFileFull(actualCmdline, outfile, false) < 0)
- goto cleanup;
-
- ret = 0;
-
- cleanup:
- return ret;
+ return testMdevctlCmd(info->virt_type, info->create, info->command,
+ mdevxml, cmdlinefile, jsonfile);
}
-static int
-testMdevctlUuidCommandHelper(const void *data)
-{
- const struct UuidCommandTestInfo *info = data;
- const char *cmd = virMdevctlCommandTypeToString(info->command);
- g_autofree char *cmdlinefile = NULL;
- g_autofree char *mdevxml = NULL;
-
- mdevxml = g_strdup_printf("%s/nodedevschemadata/%s.xml", abs_srcdir,
- info->filename);
- cmdlinefile = g_strdup_printf("%s/nodedevmdevctldata/mdevctl-%s.argv",
- abs_srcdir, cmd);
-
- return testMdevctlUuidCommand(info->command, mdevxml, cmdlinefile);
-}
static int
testMdevctlListDefined(const void *data G_GNUC_UNUSED)
@@ -369,35 +318,28 @@ mymain(void)
#define DO_TEST_CMD(desc, virt_type, create, filename, command) \
do { \
- struct startTestInfo info = { virt_type, create, filename, command }; \
- DO_TEST_FULL(desc, testMdevctlCreateOrDefineHelper, &info); \
+ struct TestInfo info = { virt_type, create, filename, command }; \
+ DO_TEST_FULL(desc, testMdevctlHelper, &info); \
} \
while (0)
#define DO_TEST_CREATE(filename) \
- DO_TEST_CMD("mdevctl create " filename, "QEMU", CREATE_DEVICE,
filename, MDEVCTL_CMD_CREATE)
+ DO_TEST_CMD("create mdev " filename, "QEMU", CREATE_DEVICE,
filename, MDEVCTL_CMD_CREATE)
#define DO_TEST_DEFINE(filename) \
- DO_TEST_CMD("mdevctl define " filename, "QEMU", CREATE_DEVICE,
filename, MDEVCTL_CMD_DEFINE)
-
-#define DO_TEST_UUID_COMMAND_FULL(desc, filename, command) \
- do { \
- struct UuidCommandTestInfo info = { filename, command }; \
- DO_TEST_FULL(desc, testMdevctlUuidCommandHelper, &info); \
- } \
- while (0)
+ DO_TEST_CMD("define mdev " filename, "QEMU", CREATE_DEVICE,
filename, MDEVCTL_CMD_DEFINE)
#define DO_TEST_STOP(filename) \
- DO_TEST_UUID_COMMAND_FULL("mdevctl stop " filename, filename,
MDEVCTL_CMD_STOP)
+ DO_TEST_CMD("stop mdev " filename, "QEMU", EXISTING_DEVICE,
filename, MDEVCTL_CMD_STOP)
#define DO_TEST_UNDEFINE(filename) \
- DO_TEST_UUID_COMMAND_FULL("mdevctl undefine " filename, filename,
MDEVCTL_CMD_UNDEFINE)
+ DO_TEST_CMD("undefine mdev" filename, "QEMU", EXISTING_DEVICE,
filename, MDEVCTL_CMD_UNDEFINE)
#define DO_TEST_START(filename) \
- DO_TEST_UUID_COMMAND_FULL("mdevctl start " filename, filename,
MDEVCTL_CMD_START)
+ DO_TEST_CMD("start mdev " filename, "QEMU", EXISTING_DEVICE,
filename, MDEVCTL_CMD_START)
#define DO_TEST_LIST_DEFINED() \
- DO_TEST_FULL("mdevctl list --defined", testMdevctlListDefined, NULL)
+ DO_TEST_FULL("list defined mdevs", testMdevctlListDefined, NULL)
#define DO_TEST_PARSE_JSON(filename) \
DO_TEST_FULL("parse mdevctl json " filename, testMdevctlParse, filename)
--
2.26.3
11:21 a.m.
New subject: [libvirt PATCH v2 09/12] tests: nodedev: Make the mdevctl test function and helper generic
On 4/13/21 4:39 PM, Jonathon Jongsma wrote:
From: Erik Skultety <eskultet(a)redhat.com>
Now that we have a generic mdevctl command generator, we can unify the
test infrastructure as well.
Signed-off-by: Erik Skultety <eskultet(a)redhat.com>
Reviewed-by: Laine Stump <laine(a)redhat.com>
3:39 p.m.
New subject: [libvirt PATCH v2 10/12] tests: nodedev: simplify test macros
We only use the virt_type "QEMU" in this tests, so simply hard-code it
in the test function rather than specifying it in the test macro. In
addition, we can figure out the appropriate value for 'create' from the
command type, so push that into the test function rather than specifying
it in the test macro.
Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com>
---
tests/nodedevmdevctltest.c | 43 +++++++++++++++++++++++++-------------
1 file changed, 28 insertions(+), 15 deletions(-)
diff --git a/tests/nodedevmdevctltest.c b/tests/nodedevmdevctltest.c
index bba521e563..4c316e3ed0 100644
--- a/tests/nodedevmdevctltest.c
+++ b/tests/nodedevmdevctltest.c
@@ -10,9 +10,9 @@
#define VIR_FROM_THIS VIR_FROM_NODEDEV
+#define VIRT_TYPE "QEMU"
+
struct TestInfo {
- const char *virt_type;
- int create;
const char *filename;
virMdevctlCommand command;
};
@@ -36,9 +36,7 @@ typedef virCommand * (*MdevctlCmdFunc)(virNodeDeviceDef *, char **, char
**);
static int
-testMdevctlCmd(const char *virt_type,
- int create,
- virMdevctlCommand cmd_type,
+testMdevctlCmd(virMdevctlCommand cmd_type,
const char *mdevxml,
const char *cmdfile,
const char *jsonfile)
@@ -53,8 +51,24 @@ testMdevctlCmd(const char *virt_type,
g_autofree char *stdinbuf = NULL;
g_autoptr(virCommand) cmd = NULL;
g_autoptr(virCommandDryRunToken) dryRunToken = virCommandDryRunTokenNew();
+ int create;
+
+ switch (cmd_type) {
+ case MDEVCTL_CMD_CREATE:
+ case MDEVCTL_CMD_DEFINE:
+ create = CREATE_DEVICE;
+ break;
+ case MDEVCTL_CMD_START:
+ case MDEVCTL_CMD_STOP:
+ case MDEVCTL_CMD_UNDEFINE:
+ create = EXISTING_DEVICE;
+ break;
+ case MDEVCTL_CMD_LAST:
+ default:
+ goto cleanup;
+ }
- if (!(def = virNodeDeviceDefParseFile(mdevxml, create, virt_type)))
+ if (!(def = virNodeDeviceDefParseFile(mdevxml, create, VIRT_TYPE)))
goto cleanup;
/* this function will set a stdin buffer containing the json configuration
@@ -106,8 +120,7 @@ testMdevctlHelper(const void *data)
jsonfile = g_strdup_printf("%s/nodedevmdevctldata/%s-%s.json", abs_srcdir,
info->filename, cmd);
- return testMdevctlCmd(info->virt_type, info->create, info->command,
- mdevxml, cmdlinefile, jsonfile);
+ return testMdevctlCmd(info->command, mdevxml, cmdlinefile, jsonfile);
}
@@ -316,27 +329,27 @@ mymain(void)
if (virTestRun(desc, func, info) < 0) \
ret = -1;
-#define DO_TEST_CMD(desc, virt_type, create, filename, command) \
+#define DO_TEST_CMD(desc, filename, command) \
do { \
- struct TestInfo info = { virt_type, create, filename, command }; \
+ struct TestInfo info = { filename, command }; \
DO_TEST_FULL(desc, testMdevctlHelper, &info); \
} \
while (0)
#define DO_TEST_CREATE(filename) \
- DO_TEST_CMD("create mdev " filename, "QEMU", CREATE_DEVICE,
filename, MDEVCTL_CMD_CREATE)
+ DO_TEST_CMD("create mdev " filename, filename, MDEVCTL_CMD_CREATE)
#define DO_TEST_DEFINE(filename) \
- DO_TEST_CMD("define mdev " filename, "QEMU", CREATE_DEVICE,
filename, MDEVCTL_CMD_DEFINE)
+ DO_TEST_CMD("define mdev " filename, filename, MDEVCTL_CMD_DEFINE)
#define DO_TEST_STOP(filename) \
- DO_TEST_CMD("stop mdev " filename, "QEMU", EXISTING_DEVICE,
filename, MDEVCTL_CMD_STOP)
+ DO_TEST_CMD("stop mdev " filename, filename, MDEVCTL_CMD_STOP)
#define DO_TEST_UNDEFINE(filename) \
- DO_TEST_CMD("undefine mdev" filename, "QEMU", EXISTING_DEVICE,
filename, MDEVCTL_CMD_UNDEFINE)
+ DO_TEST_CMD("undefine mdev" filename, filename, MDEVCTL_CMD_UNDEFINE)
#define DO_TEST_START(filename) \
- DO_TEST_CMD("start mdev " filename, "QEMU", EXISTING_DEVICE,
filename, MDEVCTL_CMD_START)
+ DO_TEST_CMD("start mdev " filename, filename, MDEVCTL_CMD_START)
#define DO_TEST_LIST_DEFINED() \
DO_TEST_FULL("list defined mdevs", testMdevctlListDefined, NULL)
--
2.26.3
5:42 a.m.
New subject: [libvirt PATCH v2 10/12] tests: nodedev: simplify test macros
On Tue, Apr 13, 2021 at 03:39:46PM -0500, Jonathon Jongsma wrote:
We only use the virt_type "QEMU" in this tests, so simply
hard-code it
in the test function rather than specifying it in the test macro. In
addition, we can figure out the appropriate value for 'create' from the
command type, so push that into the test function rather than specifying
it in the test macro.
Sounds like this should be split in 2 commits.
With that:
Reviewed-by: Erik Skultety <eskultet(a)redhat.com>
3:39 p.m.
New subject: [libvirt PATCH v2 11/12] tests: nodedev: remove unused variable
This variable was leftover from previous changes but is no longer used.
Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com>
---
tests/nodedevmdevctltest.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/tests/nodedevmdevctltest.c b/tests/nodedevmdevctltest.c
index 4c316e3ed0..65286761c1 100644
--- a/tests/nodedevmdevctltest.c
+++ b/tests/nodedevmdevctltest.c
@@ -42,7 +42,6 @@ testMdevctlCmd(virMdevctlCommand cmd_type,
const char *jsonfile)
{
g_autoptr(virNodeDeviceDef) def = NULL;
- virNodeDeviceObj *obj = NULL;
g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;
const char *actualCmdline = NULL;
int ret = -1;
@@ -99,7 +98,6 @@ testMdevctlCmd(virMdevctlCommand cmd_type,
ret = 0;
cleanup:
- virNodeDeviceObjEndAPI(&obj);
return ret;
}
--
2.26.3
5:43 a.m.
New subject: [libvirt PATCH v2 11/12] tests: nodedev: remove unused variable
On Tue, Apr 13, 2021 at 03:39:47PM -0500, Jonathon Jongsma wrote:
This variable was leftover from previous changes but is no longer
used.
Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com>
---
Reviewed-by: Erik Skultety <eskultet(a)redhat.com>
3:39 p.m.
New subject: [libvirt PATCH v2 12/12] tests: nodedev: remove unnecessary cleanup label
Now that the last cleanup task was removed in the previous commit, just
remove the label and return early on error rather than goto cleanup.
Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com>
---
tests/nodedevmdevctltest.c | 20 ++++++++------------
1 file changed, 8 insertions(+), 12 deletions(-)
diff --git a/tests/nodedevmdevctltest.c b/tests/nodedevmdevctltest.c
index 65286761c1..8ba1d2da70 100644
--- a/tests/nodedevmdevctltest.c
+++ b/tests/nodedevmdevctltest.c
@@ -44,7 +44,6 @@ testMdevctlCmd(virMdevctlCommand cmd_type,
g_autoptr(virNodeDeviceDef) def = NULL;
g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;
const char *actualCmdline = NULL;
- int ret = -1;
g_autofree char *outbuf = NULL;
g_autofree char *errbuf = NULL;
g_autofree char *stdinbuf = NULL;
@@ -64,18 +63,18 @@ testMdevctlCmd(virMdevctlCommand cmd_type,
break;
case MDEVCTL_CMD_LAST:
default:
- goto cleanup;
+ return -1;
}
if (!(def = virNodeDeviceDefParseFile(mdevxml, create, VIRT_TYPE)))
- goto cleanup;
+ return -1;
/* this function will set a stdin buffer containing the json configuration
* of the device. The json value is captured in the callback above */
cmd = nodeDeviceGetMdevctlCommand(def, cmd_type, &outbuf, &errbuf);
if (!cmd)
- goto cleanup;
+ return -1;
if (create)
virCommandSetDryRun(dryRunToken, &buf, true, true,
@@ -84,21 +83,18 @@ testMdevctlCmd(virMdevctlCommand cmd_type,
virCommandSetDryRun(dryRunToken, &buf, true, true, NULL, NULL);
if (virCommandRun(cmd, NULL) < 0)
- goto cleanup;
+ return -1;
if (!(actualCmdline = virBufferCurrentContent(&buf)))
- goto cleanup;
+ return -1;
if (virTestCompareToFileFull(actualCmdline, cmdfile, false) < 0)
- goto cleanup;
+ return -1;
if (create && virTestCompareToFile(stdinbuf, jsonfile) < 0)
- goto cleanup;
-
- ret = 0;
+ return -1;
- cleanup:
- return ret;
+ return 0;
}
--
2.26.3
5:43 a.m.
New subject: [libvirt PATCH v2 12/12] tests: nodedev: remove unnecessary cleanup label
On Tue, Apr 13, 2021 at 03:39:48PM -0500, Jonathon Jongsma wrote:
Now that the last cleanup task was removed in the previous commit,
just
remove the label and return early on error rather than goto cleanup.
Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com>
---
Reviewed-by: Erik Skultety <eskultet(a)redhat.com>
5:49 a.m.
On Tue, Apr 13, 2021 at 03:39:36PM -0500, Jonathon Jongsma wrote:
Erik recommended several changes to the mdev series that was just
merged and
suggested to address them in a follow-up patch series. They're mostly related
to simplifying the testing, and shouldn't actually change any behavior (aside
from switching to using the long commandline options when executing mdevctl).
Changes in v2:
- rebase to lastest git master
Erik Skultety (4):
nodedev: driver: Swap virMdevctlStart and virMdevctlCreate
nodedev: driver: Introduce internal mdevctl commands enum
nodedev: driver: Create a generic mdevctl command translator
tests: nodedev: Make the mdevctl test function and helper generic
Jonathon Jongsma (8):
nodedev: don't log error in nodeDeviceFindAddressByName()
nodedev: avoid use of VIR_ERR_NO_* errors internally
tests: nodedev: switch all test macros to accept a filename
nodedev: Switch to using long options for mdevctl
nodedev: Remove GetMdevctl*Command() wrappers
tests: nodedev: simplify test macros
tests: nodedev: remove unused variable
tests: nodedev: remove unnecessary cleanup label
The changes look good to me and I'm tempted to put my R-b even on the rest of
the patches, but since I have a fair share on the patches that I didn't comment
on, it suddenly feels like circumventing the rules (I know I suggested you
go ahead and submit the patches). Let's give it a few days and hopefully
someone else gives you a second opinion especially on the patches that bear my
signoff.
Regards,
Erik
11:01 a.m.
On 4/14/21 6:49 AM, Erik Skultety wrote:
On Tue, Apr 13, 2021 at 03:39:36PM -0500, Jonathon Jongsma wrote:
> Erik recommended several changes to the mdev series that was just merged and
> suggested to address them in a follow-up patch series. They're mostly related
> to simplifying the testing, and shouldn't actually change any behavior (aside
> from switching to using the long commandline options when executing mdevctl).
>
> Changes in v2:
> - rebase to lastest git master
>
> Erik Skultety (4):
> nodedev: driver: Swap virMdevctlStart and virMdevctlCreate
> nodedev: driver: Introduce internal mdevctl commands enum
> nodedev: driver: Create a generic mdevctl command translator
> tests: nodedev: Make the mdevctl test function and helper generic
>
> Jonathon Jongsma (8):
> nodedev: don't log error in nodeDeviceFindAddressByName()
> nodedev: avoid use of VIR_ERR_NO_* errors internally
> tests: nodedev: switch all test macros to accept a filename
> nodedev: Switch to using long options for mdevctl
> nodedev: Remove GetMdevctl*Command() wrappers
> tests: nodedev: simplify test macros
> tests: nodedev: remove unused variable
> tests: nodedev: remove unnecessary cleanup label
The changes look good to me and I'm tempted to put my R-b even on the rest of
the patches, but since I have a fair share on the patches that I didn't comment
on, it suddenly feels like circumventing the rules (I know I suggested you
go ahead and submit the patches). Let's give it a few days and hopefully
someone else gives you a second opinion especially on the patches that bear my
signoff.
Yeah, sorry, I went through and picked out the easy ones at the end of
the day, meaning to go back later and get the rest (unless someone else
did it), but then forgot to do that. I'll try to concentrate long enough
to get through them, or if anyone else feels inspired they can jump in
in front of me.
1312
days inactive
1319
days old
31 comments
4 participants
participants (4)
-
Erik Skultety -
John Ferlan -
Jonathon Jongsma -
Laine Stump