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) &params);

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.

[advpack/setupapi] LaunchINFSection

[Raphael]

Hi,

On Thursday 17 November 2005 04:45, Robert Shearman wrote:
> I have a few comments:

You too :)
> 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.

because i used to use assert in many of my codes 
(and i think that winedbg catch assertions).
> 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.

I used it but you are right i forget it in this code
> Is this left over debugging code?

I prefer to speak about currently unsupported pathes
> > -
> >  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.

see 
http://msdn.microsoft.com/workshop/delivery/download/overview/launchinfsecti...
Native function returns E_FAIL
> 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.

you are right
> > -
> > - @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.

No this should be fixed

Regards,
Raphael

[advpack/setupapi] LaunchINFSection

[Robert Shearman]

Raphael wrote:
> > > -
> > >  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.
>
> see 
> http://msdn.microsoft.com/workshop/delivery/download/overview/launchinfsecti...
> Native function returns E_FAIL

Maybe you are being naïve in believing MSDN or maybe MSDN is correct, 
but I'm basing this opinion on other tests for advpack that are 
returning values like HRESULT_FROM_WIN32(ERROR_FILE_NOT_FOUND). The best 
way to prove it either way is to write a test case that exercises this 
code path. Where this is not possible then you should return as 
appropriate an error message as possible and this will typically not be 
E_FAIL.