From: Erik Skultety <eskultet@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@redhat.com>
Signed-off-by: Jonathon Jongsma <jjongsma@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

On Tue, Apr 13, 2021 at 03:39:42PM -0500, Jonathon Jongsma wrote:
> From: Erik Skultety <eskultet@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

On Wed, 14 Apr 2021 12:41:02 +0200
Erik Skultety <eskultet@redhat.com> wrote:

> On Tue, Apr 13, 2021 at 03:39:42PM -0500, Jonathon Jongsma wrote:
> > From: Erik Skultety <eskultet@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.