Changeset 40665 in vbox for trunk/src

Timestamp:

Mar 27, 2012 1:52:22 PM (13 years ago)

Author:

vboxsync

Message:

Device/Parallel: incorporation of the review comments

File:

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

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

@@ -47 +47 @@
 
 
-/** @todo r=bird: The following indicator is neccessary to select the right
- *        code. */
 /** @def VBOX_WITH_WIN_PARPORT_SUP *
  * Indicates whether to use the generic direct hardware access or host specific
@@ -76 +74 @@
 # include <cfgmgr32.h>
 # include <iprt/mem.h>
+# define CTRL_REG_OFFSET 2
+# define STATUS_REG_OFFSET  1
 #endif
 
@@ -101 +101 @@
     /** Our host device interface. */
     PDMIHOSTPARALLELCONNECTOR     IHostParallelConnectorR3;
-    /** Ring-3 base interface for the ring-0 context. */
-    PDMIBASER0                    IBaseR0;
     /** Device Path */
     char                         *pszDevicePath;
     /** Device Handle */
     RTFILE                        hFileDevice;
-#ifndef VBOX_WITH_WIN_PARPORT_SUP       /** @todo r=bird: Eliminate thing not needed, it rules out things you might have missed. */
+#ifndef VBOX_WITH_WIN_PARPORT_SUP
     /** Thread waiting for interrupts. */
     PPDMTHREAD                    pMonitorThread;
@@ -114 +112 @@
     /** Wakeup pipe write end. */
     RTPIPE                        hWakeupPipeW;
-#endif
     /** Current mode the parallel port is in. */
     PDMPARALLELPORTMODE           enmModeCur;
+#endif
+
 #ifdef VBOX_WITH_WIN_PARPORT_SUP
     /** Data register. */
@@ -130 +129 @@
     /** Status read buffer. */
     uint8_t                       u8ReadInStatus;
+        /** Parallel port name */
+    uint8_t                       u8ParportName[6];
     /** Whether the parallel port is available or not. */
     bool                          fParportAvail;
-# ifdef IN_RING0 /** @todo r=bird: This isn't needed and will not work as you always need to declare all structure members in all contexts. (Size will differer between ring-3 and ring-0 now, meaning you'll corrupt heap because ring-3 does the alloc.) */
-    typedef struct DEVICE_EXTENSION
-    {
-        PPARALLEL_PORT_INFORMATION  pParallelInfo;
-        PPARALLEL_PNP_INFORMATION   pPnpInfo;
-        UNICODE_STRING              uniName;
-        PFILE_OBJECT                FileObj;
-        PDEVICE_OBJECT              pParallelDeviceObject;
-    } DEVICE_EXTENSION, *PDEVICE_EXTENSION;
-    PDEVICE_EXTENSION             pDevExtn;
-# endif
 #endif /* VBOX_WITH_WIN_PARPORT_SUP */
 } DRVHOSTPARALLEL, *PDRVHOSTPARALLEL;
@@ -182 +172 @@
  *
  */
-static int drvR0HostParallelReqWrite(PPDMDRVINS pDrvIns, uint64_t u64Arg) /** @todo r=bird: PDMBOTHCBDECL is only required for entry points. Everything which doesn't need to be globally visible shall be static! */
+static int drvR0HostParallelReqWrite(PPDMDRVINS pDrvIns, uint64_t u64Arg)
 {
     PDRVHOSTPARALLEL pThis = PDMINS_2_DATA(pDrvIns, PDRVHOSTPARALLEL);
@@ -292 +282 @@
 
     LogFlowFuncEnter();
+    /* I have included break after each case. Need to work on this. */
     switch ((DRVHOSTPARALLELR0OP)uOperation)
     {
         case DRVHOSTPARALLELR0OP_READ:
             rc = drvR0HostParallelReqRead(pDrvIns, u64Arg);
-
+            break;
         case DRVHOSTPARALLELR0OP_READSTATUS:
             rc = drvR0HostParallelReqReadStatus(pDrvIns, u64Arg);
-
+            break;
         case DRVHOSTPARALLELR0OP_READCONTROL:
             rc = drvR0HostParallelReqReadControl(pDrvIns, u64Arg);
-
+            break;
         case DRVHOSTPARALLELR0OP_WRITE:
             rc = drvR0HostParallelReqWrite(pDrvIns, u64Arg);
-
+            break;
         case DRVHOSTPARALLELR0OP_WRITECONTROL:
             rc = drvR0HostParallelReqWriteControl(pDrvIns, u64Arg);
-
+            break;
         case DRVHOSTPARALLELR0OP_SETPORTDIRECTION:
             rc = drvR0HostParallelReqSetPortDir(pDrvIns, u64Arg);
-
+            break;
         default:        /* not supported */
             rc = VERR_NOT_SUPPORTED;
@@ -329 +320 @@
  * @param   DevInst    Device Instance for parallel port.
  */
-static uint32_t drvHostWinFindIORangeResource(const DEVINST DevInst) /** @todo r=bird: Prefix methods like in the rest of the file, since windows specific, throw in a 'Win'. */
+static uint32_t drvHostWinFindIORangeResource(const DEVINST DevInst)
 {
     uint8_t  *pBuf  = NULL;
@@ -389 +380 @@
         if (cmRet != CR_SUCCESS)
            break;
-        /** @todo r=bird: No need to else soemthing when the 'then' clause
-         *        returned or broke ouf of the loop. */
+
         CM_Free_Res_Des_Handle(nextLogConf);
         nextLogConf = rdPrevResDes;
@@ -405 +395 @@
  * @param   pThis    The host parallel port instance data.
  */
-static int drvHostParallelGetParportAddr(PDRVHOSTPARALLEL pThis)
+static int drvWinHostGetparportAddr(PDRVHOSTPARALLEL pThis)
 {
     HDEVINFO hDevInfo;
@@ -433 +423 @@
                      RTMemFree(pBuf);
                 /* Max size will never be more than 2048 bytes */
-                /** @todo r=bird: Too many parentheses and casts: (uint8_t *)((char*)RTMemAlloc(u32BufSize * 2)) */
                 pBuf = (uint8_t *)RTMemAlloc(u32BufSize * 2);
             }
@@ -442 +431 @@
         if (pBuf) /** @todo r=bird: You're not checking errors here. */
         {
-             LogFlowFunc(("device name=%s\n", pBuf));
-             if (strstr((char*)pBuf, "LPT"))        /** @todo Use IPRT equivalent? r=klaus: make check much more precise, must be LPT + number (>= 1), without anything else before or after. */
+             char *pCh = NULL;
+             char* pTmpCh = NULL;
+             if (strstr((char*)pBuf, "LPT"))        /** @todo Use IPRT equivalent? */
              {
-                 LogFlowFunc(("found parallel port\n"));
                  u32ParportAddr = drvHostWinFindIORangeResource(DeviceInfoData.DevInst);
                  if (u32ParportAddr)
                  {
+                     /* Find parallel port name and update the shared data struncture */
+                     pCh = strstr((char*)pBuf, "(");
+                     pTmpCh = strstr((char *)pBuf, ")");
+                     /* check for the confirmation for the availability of parallel port */
+                     if (!(pCh && pTmpCh))
+                     {
+                         LogFlowFunc(("Parallel port Not Found \n"));
+                         return VERR_NOT_FOUND;
+
+                     }
+                     /* check for the confirmation for the availability of parallel port */
+                     if (!strncpy((char *)(pThis->u8ParportName), pCh+1, ((pTmpCh - (char *)pBuf) -
+                                 (pCh - (char *)pBuf)) - 1))
+                     {
+                         LogFlowFunc(("Parallel Port Not Found\n"));
+                         return VERR_NOT_FOUND;
+                     }
+                     *((char *)pThis->u8ParportName + (pTmpCh - (char *)pBuf) - (pCh - (char *)pBuf) + 1 ) = '\0';
+                     /* checking again to make sure that we have got a valid name and in valid format too. */
+                     if (!strstr((char *)pThis->u8ParportName, "LPT") ||
+                          !(pThis->u8ParportName[3] >= '0' && pThis->u8ParportName[3] <= '9')) {
+                         pThis->u8ParportName[0] = '\0';
+                         LogFlowFunc(("Printer Port Name Not Found\n"));
+                         return VERR_NOT_FOUND;
+                     }
                      pThis->fParportAvail = true;
                      pThis->u32LptAddr = u32ParportAddr;
-                     pThis->u32LptAddrControl = pThis->u32LptAddr + DCR_OFFSET;
-                     pThis->u32LptAddrStatus = pThis->u32LptAddr + DSR_OFFSET;
+                     pThis->u32LptAddrControl = pThis->u32LptAddr + CTRL_REG_OFFSET;
+                     pThis->u32LptAddrStatus = pThis->u32LptAddr + STATUS_REG_OFFSET;
                  }
                  if (pThis->fParportAvail)
@@ -618 +632 @@
         int rc = PDMDrvHlpCallR0(pThis->CTX_SUFF(pDrvIns), DRVHOSTPARALLELR0OP_READ, 0);
         AssertRC(rc);
-        *(uint8_t *)pvBuf = (uint8_t)pThis->u8ReadIn; /** r=bird: *((uint8_t *)(pvBuf)) is too many parentheses. Heaping them up make the code harder to read. Please check C++ operator precedence rules. */
+        *(uint8_t *)pvBuf = (uint8_t)pThis->u8ReadIn;
     }
 # endif /* VBOX_WITH_WIN_PARPORT_SUP */
@@ -740 +754 @@
 }
 
-# ifndef VBOX_WITH_WIN_PARPORT_SUP /** @todo r=bird: This whole function is unused in the direct access path. */
+# ifndef VBOX_WITH_WIN_PARPORT_SUP
 
 static DECLCALLBACK(int) drvHostParallelMonitorThread(PPDMDRVINS pDrvIns, PPDMTHREAD pThread)
@@ -958 +972 @@
 
 #else /* VBOX_WITH_WIN_PARPORT_SUP */
+    HANDLE hPort;
     pThis->fParportAvail = false;
     pThis->u32LptAddr = 0L;
     pThis->u32LptAddrControl = 0L;
     pThis->u32LptAddrStatus = 0L;
-    rc = drvHostParallelGetParportAddr(pThis);
-    return rc;
-    /**@todo r=bird: Just remove or #if 0 this code.*/
-    HANDLE hPort;
-    /** @todo r=klaus convert to IPRT */
-    hPort = CreateFile("LPT1", GENERIC_READ | GENERIC_WRITE, FILE_SHARE_READ,
-                       NULL, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL); /** @todo r=bird: Continuation indent under start parentheses like the rest of the file. */
-    if (hPort == INVALID_HANDLE_VALUE) /** @todo r=bird: Please, variable == constant, not the other way around. Just learn to not make accidental assignments in conditionals! */
-    {
-        /** @todo r=klaus probably worth a nicely formatted release log entry,
-         * pointing to the device name etc etc. */
-        LogFlowFunc(("failed to get exclusive access to parallel port\n"));
-        /** @todo r=klaus wrong kind of error code, must be IPRT error code */
-        return GetLastError();
-    }
-#endif /* VBOX_WITH_WIN_PARPORT_SUP  */
+    rc = drvWinHostGetparportAddr(pThis);
+
+    /* If we have the char port availabe use it , else I am not getting exclusive access to parallel port.
+       Read and write will be done only if addresses are availabel
+    */
+    if (pThis->u8ParportName) {
+        LogFlowFunc(("Get the Handle to Printer Port =%s\n", (char *)pThis->u8ParportName));
+        /** @todo r=klaus convert to IPRT */
+        hPort = CreateFile((char *)pThis->u8ParportName, GENERIC_READ | GENERIC_WRITE, FILE_SHARE_READ,
+                           NULL, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);
+    }
+    /** @todo amakkar: handle the case if hPort is NULL */
+# if 0
+    if (hPort == INVALID_HANDLE_VALUE)
+    {
+        LogFlow(("Failed to get exclusive access to parallel port\n"));
+        rc = VERR_INVALID_HANDLE;
+    }*/
+# endif /* VBOX_WITH_WIN_PARPORT_SUP  */
     return VINF_SUCCESS;
 }
@@ -998 +1016 @@
     "Parallel host driver.",
     /* fFlags */
-#if defined(VBOX_WITH_WIN_PARPORT_SUP)
+# if defined(VBOX_WITH_WIN_PARPORT_SUP)
     PDM_DRVREG_FLAGS_HOST_BITS_DEFAULT | PDM_DRVREG_FLAGS_R0,
-#else
+# else
     PDM_DRVREG_FLAGS_HOST_BITS_DEFAULT,
-#endif
+# endif
     /* fClass. */
     PDM_DRVREG_CLASS_CHAR,