Raphael wrote:
> Changelog:
> - semi implementation of advpack APIs LaunchINFSection/LaunchINFSectionEx
> - semi implementation of advpack API ExtractFiles
> - setupapi: correct and check pExtractFiles return code
>
>
I have a few comments:
>------------------------------------------------------------------------
>@@ -91,30 +105,132 @@
> }
>
> /***********************************************************************
>- * LaunchINFSection (ADVPACK.@)
>- */
>-void WINAPI LaunchINFSection( HWND hWnd, HINSTANCE hInst, LPCSTR cmdline, INT show )
>-{
>- FIXME("(%p %p %s %d): stub\n", hWnd, hInst, debugstr_a(cmdline), show );
>-}
>-
>-/***********************************************************************
> * LaunchINFSectionEx (ADVPACK.@)
> */
>-void WINAPI LaunchINFSectionEx( HWND hWnd, HINSTANCE hInst, LPCSTR cmdline, INT show )
>+static UINT LaunchINFSectionEx_CabExtractCallBackA(PVOID Context, UINT Notification, UINT_PTR Param1, UINT_PTR Param2)
> {
>- FIXME("(%p %p %s %d): stub\n", hWnd, hInst, debugstr_a(cmdline), show );
>-}
>+ LPCSTR file = (LPCSTR) Context;
>+ PFILE_IN_CABINET_INFO_A pInfo = (PFILE_IN_CABINET_INFO_A) Param1;
>+ if ( Notification != SPFILENOTIFY_FILEINCABINET ) return 0;
>+ assert( 0 != Param1 );
>
>
Why bother with the assert? You'll crash more nicely by just
dereferencing the NULL pointer. At least then you'll get a backtrace in
the debugger.
>+ if (0 != strcmp(file, pInfo->NameInCabinet)) return FILEOP_SKIP;
>+ lstrcpyA(pInfo->FullTargetName, pInfo->NameInCabinet);
>+ TRACE("Extraction of '%s'\n", pInfo->NameInCabinet);
>+ return FILEOP_DOIT;
>+}
>+
>+HRESULT WINAPI LaunchINFSectionEx( HWND hWnd, HINSTANCE hInst, LPCSTR cmdline, INT show )
>+{
>+ CHAR inf_name[512];
>+ CHAR section_name[512];
>+ CHAR cab_name[512];
>+ CHAR szFlags[48];
>+ CHAR szShow[2];
>+ DWORD dwFlags;
>+ CHAR* cursor = NULL;
>+ SIZE_T sz = 0;
>+ BOOL bTest;
>+ HINF hinf;
>+ void* callback_context;
>
>-/* this structure very closely resembles parameters of RunSetupCommand() */
>-typedef struct
>-{
>- HWND hwnd;
>- LPCSTR title;
>- LPCSTR inf_name;
>- LPCSTR dir;
>- LPCSTR section_name;
>-} SETUPCOMMAND_PARAMS;
>+ FIXME("(%p %p %s %d): semi-stub\n", hWnd, hInst, debugstr_a(cmdline), show );
>+
>+ cursor = strchr(cmdline, ',');
>+ if (NULL == cursor) { return E_INVALIDARG; }
>
>
This is just personal preference, but the extra brackets there aren't
needed and don't add to the readability and I haven't seen this style
used elsewhere in the Wine source code.
>+ sz = (cursor - cmdline) + 1;
>+ lstrcpynA(inf_name, cmdline, sz); inf_name[sz] = 0;
>+ cmdline = cursor + 1;
>+
>+ cursor = strchr(cmdline, ',');
>+ if (NULL == cursor) { return E_INVALIDARG; }
>+ sz = (cursor - cmdline) + 1;
>+ lstrcpynA(section_name, cmdline, sz); section_name[sz] = 0;
>+ cmdline = cursor + 1;
>+
>+ cursor = strchr(cmdline, ',');
>+ if (NULL == cursor) { return E_INVALIDARG; }
>+ sz = (cursor - cmdline) + 1;
>+ lstrcpynA(cab_name, cmdline, sz); cab_name[sz] = 0;
>+ cmdline = cursor + 1;
>+
>+ cursor = strchr(cmdline, ',');
>+ if (NULL == cursor) { return E_INVALIDARG; }
>+ sz = (cursor - cmdline) + 1;
>+ lstrcpynA(szFlags, cmdline, sz); szFlags[sz] = 0;
>+ cmdline = cursor + 1;
>+
>+ lstrcpynA(szShow, cmdline, 2); szShow[1] = 0;
>+
>+ dwFlags = atol(szFlags);
>+
>+ TRACE("cab name('%s')\n", cab_name);
>+ TRACE("inf name('%s')\n", inf_name);
>+ TRACE("inf section name('%s')\n", section_name);
>+ TRACE("flags('%s','%lu')\n", szFlags, dwFlags);
>+ TRACE("nShow('%s')\n", szShow);
>+
>+ if (dwFlags & 0x04) {
>+ FIXME("dwFlags: Quiet Mode\n");
>+ }
>+ if (dwFlags & 0x08) {
>+ FIXME("dwFlags: Don't Run GrpConv\n");
>+ FIXME("Unsupported\n"); return E_NOTIMPL;
>+ }
>+ if (dwFlags & 16) {
>+ FIXME("dwFlags: Force self-updating\n");
>+ FIXME("Unsupported\n"); return E_NOTIMPL;
>+ }
>+ if (dwFlags & 32) {
>+ FIXME("dwFlags: Backup data before install\n");
>+ }
>+ if (dwFlags & 64) {
>+ FIXME("dwFlags: Rollback data to previous state\n");
>+ /*FIXME("Unsupported\n"); return E_NOTIMPL;*/
>+ }
>+ if (dwFlags & 128) {
>+ FIXME("dwFlags: Validate Backup data before install\n");
>+ FIXME("Unsupported\n"); return E_NOTIMPL;
>+ }
>+ if (dwFlags & 256) {
>+ FIXME("dwFlags: Rollback data to previous state\n");
>+ FIXME("Unsupported\n"); return E_NOTIMPL;
>+ }
>+ if (dwFlags & 512) {
>+ FIXME("dwFlags: Force delay of OCX registration\n");
>+ FIXME("Unsupported\n"); return E_NOTIMPL;
>+ }
>+
>+ if (0 != strcmp(cab_name, "")) {
>+ FIXME("Unsupported Cab Extraction\n"); return E_NOTIMPL;
>
>
Is this left over debugging code?
>+ bTest = SetupIterateCabinetA(cab_name, 0,
>+ (PSP_FILE_CALLBACK_A) LaunchINFSectionEx_CabExtractCallBackA,
>+ (LPVOID) inf_name);
>
>
Don't cast LaunchINFSectionEx_CabExtractCallBackA. That's a recipe for
using a function with an incorrect calling convention and trashing the
stack.
>+ if (!bTest) return E_FAIL;
>
>
Don't use E_FAIL. Try to return a proper error code, preferably using
what native returns in this circumstance by adding a test case.
>+ }
>+
>+ hinf = SetupOpenInfFileA(inf_name, NULL, INF_STYLE_WIN4, NULL);
>+ if (hinf == INVALID_HANDLE_VALUE) {
>+ ERR("Failed to SetupOpenInfFileA(%s)\n", inf_name);
>+ return E_FAIL;
>
>
Again, don't use E_FAIL. Also consider adding some more debugging
information to the error message (like, say, the value returned by
GetLastError()).
>+ }
>+
>+ callback_context = SetupInitDefaultQueueCallback(hWnd);
>+ bTest = SetupInstallFromInfSectionA(NULL, hinf,
>+ section_name,
>+ SPINST_ALL,
>+ NULL, NULL, 0,
>+ SetupDefaultQueueCallbackA,
>+ callback_context,
>+ NULL, NULL);
>+ SetupTermDefaultQueueCallback(callback_context);
>+ SetupCloseInfFile(hinf);
>+
>+ if (!bTest) {
>+ ERR("Failed to SetupInstallFromInfSectionA(%s, %s)\n", inf_name, section_name);
>+ return E_FAIL;
>
>
Again, don't use E_FAIL - return a proper error code.
>+ }
>+ return S_OK;
>+}
>
> /***********************************************************************
> * DoInfInstall (ADVPACK.@)
>@@ -144,6 +260,15 @@
> }
>
> /***********************************************************************
>+ * LaunchINFSection (ADVPACK.@)
>+ */
>+HRESULT WINAPI LaunchINFSection( HWND hWnd, HINSTANCE hInst, LPCSTR cmdline, INT show )
>+{
>+ FIXME("(%p %p %s %d): semi-stub\n", hWnd, hInst, debugstr_a(cmdline), show );
>
>
Can you give us a clue as to what the difference between
LaunchINFSection and LaunchINFSectionEx might be by adding a comment? If
you don't know, a comment would still be appreciated saying that it
should be doing something different here.
>+ return LaunchINFSectionEx( hWnd, hInst, cmdline, show );
>+}
>+
>+/***********************************************************************
> * IsNTAdmin (ADVPACK.@)
> */
> BOOL WINAPI IsNTAdmin( DWORD reserved, PDWORD pReserved )
>@@ -395,16 +520,59 @@
> /***********************************************************************
> * ExtractFiles (ADVPACK.@)
> *
>+ * @param CabName: Cabinet file path
>+ * @param ExpandDir: Extraction Output Directory
>+ * @param Flags: UNKNOWN
>+ * @param FileList: if NULL extract all files
>+ * else UNKNOWN
>+ *
>
>
I don't think that is a c2man compatible documentation format.
> * BUGS
> * Unimplemented
> */
>+typedef struct
>+{
>+ DWORD Flags;
>+ LPCSTR ExpandDir;
>+ LPCSTR FileList;
>+} ExtractFiles_CabExtractCallBackA_params_t;
>+
>+static UINT ExtractFiles_CabExtractCallBackA(PVOID Context, UINT Notification, UINT_PTR Param1, UINT_PTR Param2)
>+{
>+ ExtractFiles_CabExtractCallBackA_params_t* pParams = (ExtractFiles_CabExtractCallBackA_params_t*) Context;
>+ PFILE_IN_CABINET_INFO_A pInfo = (PFILE_IN_CABINET_INFO_A) Param1;
>+ if ( Notification != SPFILENOTIFY_FILEINCABINET ) return 0;
>+ assert( 0 != Param1 );
>+ assert( 0 != Param2 );
>+ /** stupid heuristic: waiting for better: ie. understanding what is FileList format */
>+ if (NULL != pParams->FileList)
>+ {
>+ if (NULL == strstr(pParams->FileList, pInfo->NameInCabinet)) return FILEOP_SKIP;
>+ }
>+ lstrcpyA(pInfo->FullTargetName, pParams->ExpandDir);
>+ /** we should test if pParams->ExpandDir finished with "\" but each app examples i found have it */
>+ lstrcatA(pInfo->FullTargetName, pInfo->NameInCabinet);
>+ TRACE("Extraction '%s' to '%s' (wanted Path was '%s')\n", pInfo->NameInCabinet, pInfo->FullTargetName, pParams->ExpandDir);
>+ return FILEOP_DOIT;
>+}
>
> HRESULT WINAPI ExtractFiles ( LPCSTR CabName, LPCSTR ExpandDir, DWORD Flags,
> LPCSTR FileList, LPVOID LReserved, DWORD Reserved)
> {
>- FIXME("(%p %p 0x%08lx %p %p 0x%08lx): stub\n", CabName, ExpandDir, Flags,
>+ ExtractFiles_CabExtractCallBackA_params_t params;
>+ BOOL bTest;
>+
>+ FIXME("(%p %p 0x%08lx %p %p 0x%08lx): semi-stub\n", CabName, ExpandDir, Flags,
> FileList, LReserved, Reserved);
>- return E_FAIL;
>+
>+ params.Flags = Flags;
>+ params.ExpandDir = ExpandDir;
>+ params.FileList = FileList;
>+
>+ bTest = SetupIterateCabinetA(CabName, 0,
>+ (PSP_FILE_CALLBACK_A) ExtractFiles_CabExtractCallBackA,
>+ (LPVOID) ¶ms);
>
>
Again, don't cast ExtractFiles_CabExtractCallBackA. Just fix up the
declaration of that function if it issues a warning here.
>+ if (!bTest) return E_FAIL;
>
>
Again, don't use E_FAIL.
--
Rob Shearman
