Re: [PATCH v3 1/2] mspatcha: Implement ApplyPatchToFile and related functions.

[Hans Leidekker]

Hi Conor,

Thanks for your contribution. The patches look good overall but I do
have some comments, mostly involving Wine conventions.
> create mode 100755 dlls/mspatcha/lzxd_dec.c
>  create mode 100755 dlls/mspatcha/lzxd_dec.h
>  create mode 100755 dlls/mspatcha/pa19.c
>  create mode 100755 dlls/mspatcha/pa19.h

I assume that Alexandre's scripts will fix this up, but notice that
these files have the executable bit set.
> diff --git a/dlls/mspatcha/mspatcha.spec b/dlls/mspatcha/mspatcha.spec
> index cc8ca94..8c014b9 100644
> --- a/dlls/mspatcha/mspatcha.spec
> +++ b/dlls/mspatcha/mspatcha.spec
> @@ -1,12 +1,16 @@
> -1 stdcall ApplyPatchToFileA(str str str long)
> -2 stub ApplyPatchToFileByHandles
> -3 stub ApplyPatchToFileByHandlesEx
> -4 stub ApplyPatchToFileExA
> -5 stub ApplyPatchToFileExW
> -6 stdcall ApplyPatchToFileW(wstr wstr wstr long)
> -7 stdcall GetFilePatchSignatureA(str long ptr long ptr long ptr long ptr)
> -8 stub GetFilePatchSignatureByHandle
> -9 stdcall GetFilePatchSignatureW(wstr long ptr long ptr long ptr long ptr)
> -10 stub TestApplyPatchToFileA
> -11 stub TestApplyPatchToFileByHandles
> -12 stub TestApplyPatchToFileW
> +@ stdcall ApplyPatchToFileA(str str str long)
> +@ stdcall ApplyPatchToFileByBuffers(ptr long ptr long ptr long ptr ptr long ptr ptr)
> +@ stdcall ApplyPatchToFileByHandles(ptr ptr ptr long)
> +@ stdcall ApplyPatchToFileByHandlesEx(ptr ptr ptr long ptr ptr)
> +@ stdcall ApplyPatchToFileExA(str str str long ptr ptr)
> +@ stdcall ApplyPatchToFileExW(wstr wstr wstr long ptr ptr)
> +@ stdcall ApplyPatchToFileW(wstr wstr wstr long)
> +@ stdcall GetFilePatchSignatureA(str long ptr long ptr long ptr long str)
> +@ stdcall GetFilePatchSignatureByBuffer(ptr long long ptr long ptr long ptr long str)
> +@ stdcall GetFilePatchSignatureByHandle(ptr long ptr long ptr long ptr long str)
> +@ stdcall GetFilePatchSignatureW(wstr long ptr long ptr long ptr long wstr)

The last parameter to the signature functions is an output buffer,
which should be marked as 'ptr'.
> +@ stdcall NormalizeFileForPatchSignature(ptr long long ptr long long long ptr long ptr)
> +@ stdcall TestApplyPatchToFileA(str str long)
> +@ stdcall TestApplyPatchToFileByBuffers(ptr long ptr long ptr long)
> +@ stdcall TestApplyPatchToFileByHandles(ptr ptr long)
> +@ stdcall TestApplyPatchToFileW(wstr wstr long)
> diff --git a/dlls/mspatcha/mspatcha_main.c b/dlls/mspatcha/mspatcha_main.c
> index f78b8ab..3a311bc 100644
> --- a/dlls/mspatcha/mspatcha_main.c
> +++ b/dlls/mspatcha/mspatcha_main.c
> @@ -2,6 +2,7 @@
> - PatchAPI
> -
> - Copyright 2011 David Hedberg for CodeWeavers
> -
> - Copyright 2019 Conor McCarthy (implementations)
> -
> - This library is free software; you can redistribute it and/or
> - modify it under the terms of the GNU Lesser General Public
>
> @@ -16,6 +17,31 @@
> - You should have received a copy of the GNU Lesser General Public
> - License along with this library; if not, write to the Free Software
> - Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA
> -
> -
> -
> -
> -
> - This library is free software; you can redistribute it and/or
> -
> - modify it under the terms of the GNU Lesser General Public
> -
> - License as published by the Free Software Foundation; either
> -
> - version 2.1 of the License, or (at your option) any later version.
> -
> -
> -
> - This library is distributed in the hope that it will be useful,
> -
> - but WITHOUT ANY WARRANTY; without even the implied warranty of
> -
> - MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> -
> - Lesser General Public License for more details.
> -
> -
> -
> - You should have received a copy of the GNU Lesser General Public
> -
> - License along with this library; if not, write to the Free Software
> -
> - Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA

Duplicate copyright text.

...
> /*****************************************************
> -
> - ApplyPatchToFileA (MSPATCHA.1)
> -
> - TestApplyPatchToFileA (MSPATCHA.@)
> */
>
> -BOOL WINAPI ApplyPatchToFileA(LPCSTR patch_file, LPCSTR old_file, LPCSTR new_file, ULONG apply_flags)
> +BOOL WINAPI TestApplyPatchToFileA(LPCSTR patch_file, LPCSTR old_file, ULONG apply_flags)
>  {
>      BOOL ret;
> - WCHAR *patch_fileW, *new_fileW, *old_fileW = NULL;
> - WCHAR *patch_fileW, *old_fileW = NULL;
> - if (!(patch_fileW = strdupAW( patch_file ))) return FALSE;
> - if (old_file && !(old_fileW = strdupAW( old_file )))
> - if (!(patch_fileW = strdupAW(patch_file))) return FALSE;
> - if (old_file && !(old_fileW = strdupAW(old_file)))
> {
> -
>    HeapFree( GetProcessHeap(), 0, patch_fileW );
> -
>    HeapFree(GetProcessHeap(), 0, patch_fileW);
>    return FALSE;
> }
> - if (!(new_fileW = strdupAW( new_file )))
> - ret = apply_patch_to_file(patch_fileW, old_fileW, NULL, apply_flags, NULL, NULL, TRUE);
> - HeapFree(GetProcessHeap(), 0, patch_fileW);
> - if(old_fileW)
> -
>    HeapFree(GetProcessHeap(), 0, old_fileW);

There's no need to check for NULL, HeapFree will do that for you.
> +BOOL WINAPI TestApplyPatchToFileByBuffers(BYTE *patch_file_buf, ULONG patch_file_size,
> - BYTE *old_file_buf, ULONG old_file_size,
> - ULONG* new_file_size,
> - ULONG  apply_option_flags)
>
> +{
> - /* preserve last error on success as per windows */
> - DWORD last = GetLastError();
> -
> - DWORD err = apply_patch_to_file_by_buffers(patch_file_buf, patch_file_size,
> -
>    old_file_buf, old_file_size,
> -
>    NULL, 0, new_file_size, NULL,
> -
>    apply_option_flags,
> -
>    NULL, NULL,
> -
>    TRUE);
> -
> - SetLastError(err == ERROR_SUCCESS ? last : err);

You can ignore last error on success until we find an application that
depends on it, which is unlikely.
> +/*****************************************************
> -
> - ApplyPatchToFileExA (MSPATCHA.@)
> - */
>
> +BOOL WINAPI ApplyPatchToFileExA(LPCSTR patch_file, LPCSTR old_file, LPCSTR new_file, ULONG apply_flags,
> - PPATCH_PROGRESS_CALLBACK progress_fn, PVOID progress_ctx)
>
> +{
> - BOOL ret = FALSE;
> - WCHAR *patch_fileW, *new_fileW, *old_fileW = NULL;
> -
> - if (!(patch_fileW = strdupAW(patch_file))) return FALSE;
> - if (old_file && !(old_fileW = strdupAW(old_file)))
> {
> -
>    HeapFree( GetProcessHeap(), 0, patch_fileW );
> -
>    HeapFree( GetProcessHeap(), 0, old_fileW );
> -
>    HeapFree(GetProcessHeap(), 0, patch_fileW);
>    return FALSE;
> }
> - ret = ApplyPatchToFileW( patch_fileW, old_fileW, new_fileW, apply_flags );
> - HeapFree( GetProcessHeap(), 0, patch_fileW );
> - HeapFree( GetProcessHeap(), 0, old_fileW );
> - HeapFree( GetProcessHeap(), 0, new_fileW );
> - if (!(new_fileW = strdupAW(new_file)))
> - {
> -
>    goto free_wstrs;
> - }
> - ret = apply_patch_to_file(patch_fileW, old_fileW, new_fileW, apply_flags, progress_fn, progress_ctx, FALSE);
> - HeapFree(GetProcessHeap(), 0, new_fileW);
>
> +free_wstrs:
> - HeapFree(GetProcessHeap(), 0, patch_fileW);
> - if(old_fileW)
> -
>    HeapFree(GetProcessHeap(), 0, old_fileW);

Redundant NULL check.

...
> +/*****************************************************
> -
> - GetFilePatchSignatureA (MSPATCHA.@)
> */
>
> BOOL WINAPI GetFilePatchSignatureA(LPCSTR filename, ULONG flags, PVOID data, ULONG ignore_range_count,
> -
>                               PPATCH_IGNORE_RANGE ignore_range, ULONG retain_range_count,
> -
>                               PPATCH_RETAIN_RANGE retain_range, ULONG bufsize, LPSTR buffer)
> - PPATCH_IGNORE_RANGE ignore_range, ULONG retain_range_count,
> - PPATCH_RETAIN_RANGE retain_range, ULONG bufsize, LPSTR buffer)
>
> {
>      FIXME("stub - %s, %x, %p, %u, %p, %u, %p, %u, %p\n", debugstr_a(filename), flags, data,
> -
>      ignore_range_count, ignore_range, retain_range_count, retain_range, bufsize, buffer);
> -
>    ignore_range_count, ignore_range, retain_range_count, retain_range, bufsize, buffer);
> SetLastError(ERROR_CALL_NOT_IMPLEMENTED);
>    return FALSE;
>
> }
>
> /*****************************************************
> -
> - GetFilePatchSignatureW (MSPATCHA.9)
> -
> - GetFilePatchSignatureW (MSPATCHA.@)
> */
>
> BOOL WINAPI GetFilePatchSignatureW(LPCWSTR filename, ULONG flags, PVOID data, ULONG ignore_range_count,
> -
>                               PPATCH_IGNORE_RANGE ignore_range, ULONG retain_range_count,
> -
>                               PPATCH_RETAIN_RANGE retain_range, ULONG bufsize, LPWSTR buffer)
> - PPATCH_IGNORE_RANGE ignore_range, ULONG retain_range_count,
> - PPATCH_RETAIN_RANGE retain_range, ULONG bufsize, LPWSTR buffer)
>
> {
>      FIXME("stub - %s, %x, %p, %u, %p, %u, %p, %u, %p\n", debugstr_w(filename), flags, data,
> -
>      ignore_range_count, ignore_range, retain_range_count, retain_range, bufsize, buffer);
> -
>    ignore_range_count, ignore_range, retain_range_count, retain_range, bufsize, buffer);
> - SetLastError(ERROR_CALL_NOT_IMPLEMENTED);
> - return FALSE;
>
> +}
> -
>
> +/*****************************************************
> -
> - GetFilePatchSignatureByHandle (MSPATCHA.@)
> - */
>
> +BOOL WINAPI GetFilePatchSignatureByHandle(HANDLE handle, ULONG flags, PVOID options, ULONG ignore_range_count,
> - PPATCH_IGNORE_RANGE ignore_range, ULONG retain_range_count,
> - PPATCH_RETAIN_RANGE retain_range, ULONG bufsize, LPSTR buffer)
>
> +{
> - FIXME("stub - %p, %x, %p, %u, %p, %u, %p, %u, %p\n", handle, flags, options,
> -
>    ignore_range_count, ignore_range, retain_range_count, retain_range, bufsize, buffer);
> - SetLastError(ERROR_CALL_NOT_IMPLEMENTED);
> - return FALSE;
>
> +}
> -
>
> +/*****************************************************
> -
> - GetFilePatchSignatureByBuffer (MSPATCHA.@)
> - */
>
> +BOOL WINAPI GetFilePatchSignatureByBuffer(PBYTE file_buf, ULONG file_size, ULONG flags, PVOID options,
> - ULONG ignore_range_count, PPATCH_IGNORE_RANGE ignore_range,
> - ULONG retain_range_count, PPATCH_RETAIN_RANGE retain_range,
> - ULONG bufsize, LPSTR buffer)
>
> +{
> - FIXME("stub - %p, %u, %x, %p, %u, %p, %u, %p, %u, %p\n", file_buf, file_size, flags, options,
> -
>    ignore_range_count, ignore_range, retain_range_count, retain_range, bufsize, buffer);
> SetLastError(ERROR_CALL_NOT_IMPLEMENTED);
>    return FALSE;
>
> }
> -
>
> +/*****************************************************
> -
> - NormalizeFileForPatchSignature (MSPATCHA.@)
> - */
>
> +INT WINAPI NormalizeFileForPatchSignature(PVOID file_buffer, ULONG file_size, ULONG flags, PATCH_OPTION_DATA *options,
> - ULONG new_coff_base, ULONG new_coff_time, ULONG ignore_range_count, PPATCH_IGNORE_RANGE ignore_range,
> - ULONG retain_range_count, PPATCH_RETAIN_RANGE retain_range)
>
> +{
> - FIXME("stub - %p, %u, %x, %p, %u, %u, %u, %p, %u, %p\n", file_buffer, file_size, flags, options, new_coff_base,
> -
>    new_coff_time, ignore_range_count, ignore_range, retain_range_count, retain_range);
> - SetLastError(ERROR_CALL_NOT_IMPLEMENTED);
> - return 0;
>
> +}

Please split this patch where possible to reduce its size. You could
send the patchapi.h changes first as a separate patch and these stubs
could also go into a separate patch.