Changeset 42152 in vbox for trunk

Timestamp:

Jul 13, 2012 7:31:42 PM (13 years ago)

Author:

vboxsync

Message:

DrvHostParallel.cpp: Some cleanups and todos.

File:

• trunk/src/VBox/Devices/Parallel/DrvHostParallel.cpp (modified) (12 diffs)

trunk/src/VBox/Devices/Parallel/DrvHostParallel.cpp

@@ -60 +60 @@
 
 #if defined(VBOX_WITH_WIN_PARPORT_SUP) && defined(IN_RING0)
-# include <Wdm.h>
-# include <parallel.h>
 # include <iprt/asm-amd64-x86.h>
 #endif
 
 #if defined(VBOX_WITH_WIN_PARPORT_SUP) && defined(IN_RING3)
-# include <stdio.h>
-# include <windows.h>
-# include <devguid.h>
+# include <Windows.h>
 # include <setupapi.h>
-# include <regstr.h>
-# include <string.h>
 # include <cfgmgr32.h>
 # include <iprt/mem.h>
-# define CTRL_REG_OFFSET 2
-# define STATUS_REG_OFFSET  1
+# include <iprt/string.h>
 #endif
 
@@ -130 +123 @@
     uint8_t                       u8ReadInStatus;
         /** Parallel port name */
+    /** @todo r=bird: This is an array, they start with 'a'. However, this seems
+     * to be a string, and they are not uint8_t but char and are prefixed with 'sz'
+     * in char array form.
+     *
+     * Also, the use of tabs as indentation in anything but Makefiles is
+     * forbidden.  That is not a guideline, it's the law. :-) (See above line.) */
     uint8_t                       u8ParportName[6];
     /** Whether the parallel port is available or not. */
@@ -163 +162 @@
 #define PDMIHOSTPARALLELCONNECTOR_2_DRVHOSTPARALLEL(pInterface) ( (PDRVHOSTPARALLEL)((uintptr_t)pInterface - RT_OFFSETOF(DRVHOSTPARALLEL, CTX_SUFF(IHostParallelConnector))) )
 
+
+/*******************************************************************************
+*   Defined Constants And Macros                                               *
+*******************************************************************************/
+#define CTRL_REG_OFFSET                 2
+#define STATUS_REG_OFFSET               1
+#define LPT_CONTROL_ENABLE_BIDIRECT     0x20
+
+
+
 #ifdef VBOX_WITH_WIN_PARPORT_SUP
-#ifdef IN_RING0
+# ifdef IN_RING0
+
 /**
  * R0 mode function to write byte value to data port.
@@ -262 +272 @@
     {
        u8ReadControlVal = ASMInU8(pThis->u32LptAddrControl);
-       u8WriteControlVal = u8ReadControlVal | DCR_DIRECTION; /* enable input direction */
+       u8WriteControlVal = u8ReadControlVal | LPT_CONTROL_ENABLE_BIDIRECT; /* enable input direction */
        ASMOutU8(pThis->u32LptAddrControl, u8WriteControlVal);
     }
@@ -268 +278 @@
     {
         u8ReadControlVal = ASMInU8(pThis->u32LptAddrControl);
-        u8WriteControlVal = u8ReadControlVal & ~DCR_DIRECTION; /* disable input direction */
+        u8WriteControlVal = u8ReadControlVal & ~LPT_CONTROL_ENABLE_BIDIRECT; /* disable input direction */
         ASMOutU8(pThis->u32LptAddrControl, u8WriteControlVal);
     }
@@ -309 +319 @@
     return rc;
 }
-#endif /* IN_RING0 */
+
+# endif /* IN_RING0 */
 #endif /* VBOX_WITH_WIN_PARPORT_SUP */
 
 #ifdef IN_RING3
 # ifdef VBOX_WITH_WIN_PARPORT_SUP
+
 /**
  * Find IO port range for the parallel port and return the lower address.
@@ -411 +423 @@
     for (u32Idx = 0; SetupDiEnumDeviceInfo(hDevInfo, u32Idx, &DeviceInfoData); u32Idx++)
     {
-        uint32_t u32DataType;
+        uint32_t u32DataType; /** @todo r=bird: why do you use uint32_t here when the function wants a DWORD? */
         uint8_t *pBuf = NULL;
-        uint32_t u32BufSize = 0;
-
-        while (!SetupDiGetDeviceRegistryProperty(hDevInfo, &DeviceInfoData, SPDRP_FRIENDLYNAME, (PDWORD)&u32DataType, (uint8_t *)pBuf,
-                                                 u32BufSize, (PDWORD)&u32BufSize))
+        uint32_t u32BufSize = 0; /** @todo r=bird: ditto */
+
+        while (!SetupDiGetDeviceRegistryProperty(hDevInfo, &DeviceInfoData, SPDRP_FRIENDLYNAME,
+                                                 (PDWORD)&u32DataType, (uint8_t *)pBuf,
+                                                 u32BufSize, (PDWORD)&u32BufSize)
+               && GetLastError() == ERROR_INSUFFICIENT_BUFFER)
         {
-            if (GetLastError() == ERROR_INSUFFICIENT_BUFFER)
-            {
-                if (pBuf)
-                     RTMemFree(pBuf);
-                /* Max size will never be more than 2048 bytes */
-                pBuf = (uint8_t *)RTMemAlloc(u32BufSize * 2);
-            }
-            else
-                break;
+            if (pBuf)
+                 RTMemFree(pBuf);
+            /* Max size will never be more than 2048 bytes */
+            pBuf = (uint8_t *)RTMemAlloc(u32BufSize * 2);
         }
 
         if (pBuf) /** @todo r=bird: You're not checking errors here. */
         {
+            /** @todo r=bird: The indent of the block is wrong... More importantely, the
+             * scope and purpose of the two variables would be clearer if you moved them
+             * to the RTStrStr calls further down. */
              char *pCh = NULL;
              char* pTmpCh = NULL;
@@ -449 +461 @@
                      }
                      if (((pTmpCh - (char *)pBuf) - (pCh - (char *)pBuf)) < 0) {
-                         LogFlowFunc(("Parallel port string not properly formatted. \n"));
+                         LogFlowFunc(("Parallel port string not properly formatted.\n"));
                          return VERR_NOT_FOUND;
                      }
@@ -456 +468 @@
                                       pCh+1, ((pTmpCh - (char *)pBuf) - (pCh - (char *)pBuf)) - 1))
                      {
-                         LogFlowFunc(("Parallel Port Not Found. \n"));
+                         LogFlowFunc(("Parallel Port Not Found.\n"));
                          return VERR_NOT_FOUND;
                      }
@@ -463 +475 @@
                      /* checking again to make sure that we have got a valid name and in valid format too. */
                      if (RTStrNCmp((char *)pThis->u8ParportName, "LPT", 3)) {
-                         LogFlowFunc(("Parallel Port name \"LPT\" Not Found. \n"));
+                         LogFlowFunc(("Parallel Port name \"LPT\" Not Found.\n"));
                          return VERR_NOT_FOUND;
                      }
 
+                     /** @todo r=bird: Multiline expressions starts the next line with the
+                      * operator, (instead of the previous line ending with an operator like you do
+                      * here).
+                      *
+                      * Also, note that the opening curly brackets have it's own line in this
+                      * file, so you do the same as the original author(s) of the file. */
                      if (!RTStrStr((char *)pThis->u8ParportName, "LPT") ||
                           !(pThis->u8ParportName[3] >= '0' && pThis->u8ParportName[3] <= '9')) {
                          RT_BZERO(pThis->u8ParportName, sizeof(pThis->u8ParportName));
-                         LogFlowFunc(("Printer Port Name Not Found. \n"));
+                         LogFlowFunc(("Printer Port Name Not Found.\n"));
                          return VERR_NOT_FOUND;
                      }
-                     pThis->fParportAvail = true;
-                     pThis->u32LptAddr = u32ParportAddr;
+                     pThis->fParportAvail     = true;
+                     pThis->u32LptAddr        = u32ParportAddr;
                      pThis->u32LptAddrControl = pThis->u32LptAddr + CTRL_REG_OFFSET;
-                     pThis->u32LptAddrStatus = pThis->u32LptAddr + STATUS_REG_OFFSET;
+                     pThis->u32LptAddrStatus  = pThis->u32LptAddr + STATUS_REG_OFFSET;
                  }
                  if (pThis->fParportAvail)
@@ -983 +1001 @@
 #else /* VBOX_WITH_WIN_PARPORT_SUP */
     HANDLE hPort;
-    pThis->fParportAvail = false;
-    pThis->u32LptAddr = 0L;
-    pThis->u32LptAddrControl = 0L;
-    pThis->u32LptAddrStatus = 0L;
+    pThis->fParportAvail     = false;
+    pThis->u32LptAddr        = 0;
+    pThis->u32LptAddrControl = 0;
+    pThis->u32LptAddrStatus  = 0;
     rc = drvWinHostGetparportAddr(pThis);
 
@@ -992 +1010 @@
        Read and write will be done only if addresses are available
     */
-    if (pThis->u8ParportName) {
+    if (pThis->u8ParportName)
+    {
         LogFlowFunc(("Get the Handle to Printer Port =%s\n", (char *)pThis->u8ParportName));
         /** @todo r=klaus convert to IPRT */