Re: [advpack/setupapi] LaunchINFSection

[Robert Shearman]

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