[ovs-dev] [PATCH 1/4] datapath-windows: Data structures and functions for dump state (Nithin Raju)

[Samuel Ghinet]

Hello Nithin,

I have a few notes regarding the patch:

>     struct {
>         POVS_MESSAGE ovsMsg;    /* OVS message passed during dump start. */
>         UINT32 index[8];        /* markers to continue dump from. One or more
>                                  * of them may be used. */
>     } dumpState;                /* data to support dump commands. */

1. It might be useful if you would add a comment to index[8] to express how this index field is going to be used in dump operations.
I am sure the way the indexes are used in the dump operations would be quite the same in all the project.

2. Do we need 8 markers? If we may need only index[0], perhaps we should keep only one index.
If there will be a part in code where two indexes will be used, perhaps we should add then, a new variable, with a more specific name.
Specific names for how the index(es) is / are actually used might be clearer than using index[0], index[1], etc.

btw, pershaps one day we should rename the OVS_OPEN_INSTANCE to something more specific, such as an OVS_FILE_HANDLE_CONTEXT.

function InitUserDumpState:
> instance->dumpState.ovsMsg = (POVS_MESSAGE) OvsAllocateMemory(
>        sizeof *instance->dumpState.ovsMsg);

I think it would be cleaner (and shorter) if you could do instead:
instance->dumpState.ovsMsg = (POVS_MESSAGE) OvsAllocateMemory(
        sizeof OVS_MESSAGE);

"OVS_MESSAGE" is much shorter to read than "*instance->dumpState.ovsMsg"

Regards,
Sam
--------------------------------------------------------------
Message: 2
Date: Thu, 28 Aug 2014 10:59:10 -0700
From: Nithin Raju <nithin at vmware.com>
To: dev at openvswitch.org
Subject: [ovs-dev] [PATCH 1/4] datapath-windows: Data structures and
        functions for dump state
Message-ID: <1409248753-12733-1-git-send-email-nithin at vmware.com>

Signed-off-by: Nithin Raju <nithin at vmware.com>
---
 datapath-windows/ovsext/Datapath.h |   54 ++++++++++++++++++++++++++++++------
 1 files changed, 45 insertions(+), 9 deletions(-)

diff --git a/datapath-windows/ovsext/Datapath.h b/datapath-windows/ovsext/Datapath.h
index 6d8a6db..0b303a2 100644
--- a/datapath-windows/ovsext/Datapath.h
+++ b/datapath-windows/ovsext/Datapath.h
@@ -28,6 +28,16 @@
 #ifndef __OVS_DATAPATH_H_
 #define __OVS_DATAPATH_H_ 1

+/*
+ * Structure of any message passed between userspace and kernel.
+ */
+typedef struct _OVS_MESSAGE {
+    NL_MSG_HDR nlMsg;
+    GENL_MSG_HDR genlMsg;
+    OVS_HDR ovsHdr;
+    /* Variable length nl_attrs follow. */
+} OVS_MESSAGE, *POVS_MESSAGE;
+
 typedef struct _OVS_DEVICE_EXTENSION {
     INT numberOpenInstance;
     INT pidCount;
@@ -57,6 +67,12 @@ typedef struct _OVS_OPEN_INSTANCE {
      * restriction might go away as the userspace code gets implemented.
      */
     INT inUse;
+
+    struct {
+        POVS_MESSAGE ovsMsg;    /* OVS message passed during dump start. */
+        UINT32 index[8];        /* markers to continue dump from. One or more
+                                 * of them may be used. */
+    } dumpState;                /* data to support dump commands. */
 } OVS_OPEN_INSTANCE, *POVS_OPEN_INSTANCE;

 NDIS_STATUS OvsCreateDeviceObject(NDIS_HANDLE ovsExtDriverHandle);
@@ -67,15 +83,35 @@ POVS_OPEN_INSTANCE OvsGetOpenInstance(PFILE_OBJECT fileObject,

 NTSTATUS OvsCompleteIrpRequest(PIRP irp, ULONG_PTR infoPtr, NTSTATUS status);

-/*
- * Structure of any message passed between userspace and kernel.
- */
-typedef struct _OVS_MESSAGE {
-    NL_MSG_HDR nlMsg;
-    GENL_MSG_HDR genlMsg;
-    struct ovs_header ovsHdr;
-    /* Variable length nl_attrs follow. */
-} OVS_MESSAGE, *POVS_MESSAGE;
+static __inline NTSTATUS
+InitUserDumpState(POVS_OPEN_INSTANCE instance,
+                  POVS_MESSAGE ovsMsg)
+{
+    /* Clear the dumpState from a previous dump sequence. */
+    ASSERT(instance->dumpState.ovsMsg == NULL);
+    ASSERT(ovsMsg);
+
+    instance->dumpState.ovsMsg = (POVS_MESSAGE) OvsAllocateMemory(
+        sizeof *instance->dumpState.ovsMsg);
+    if (instance->dumpState.ovsMsg == NULL) {
+        return STATUS_NO_MEMORY;
+    }
+    RtlCopyMemory(instance->dumpState.ovsMsg, ovsMsg,
+                  sizeof *instance->dumpState.ovsMsg);
+    RtlZeroMemory(instance->dumpState.index,
+                  sizeof instance->dumpState.index);
+
+    return STATUS_SUCCESS;
+}
+
+static __inline VOID
+FreeUserDumpState(POVS_OPEN_INSTANCE instance)
+{
+    if (instance->dumpState.ovsMsg != NULL) {
+        OvsFreeMemory(instance->dumpState.ovsMsg);
+        RtlZeroMemory(&instance->dumpState, sizeof instance->dumpState);
+    }
+}

 #endif /* __OVS_DATAPATH_H_ */

--
1.7.4.1