On 2017-05-22 03:09, hbelusca(a)svn.reactos.org wrote:
> --- branches/setup_improvements/base/setup/lib/arcname.c (added)
> +++ branches/setup_improvements/base/setup/lib/arcname.c [iso-8859-1] Mon May 22 01:09:35 2017
> +PCSTR
> +ArcGetNextTokenA(
> + IN PCSTR ArcPath,
> + OUT PANSI_STRING TokenSpecifier,
> + OUT PULONG Key)
> +{
> + HRESULT hr;
> + PCSTR p = ArcPath;
> + ULONG SpecifierLength;
> + ULONG KeyValue;
> +
> + /*
> + * We must have a valid "specifier(key)" string, where 'specifier'
> + * cannot be the empty string, and is followed by '('.
> + */
> + p = strchr(p, '(');
> + if (p == NULL)
> + return NULL; /* No '(' found */
> + if (p == ArcPath)
> + return NULL; /* Path starts with '(' and is thus invalid */
> +
> + SpecifierLength = (p - ArcPath) * sizeof(CHAR);
> +
> + /*
> + * The strtoul function skips any leading whitespace.
> + *
> + * Note that if the token is "specifier()" then strtoul won't perform
> + * any conversion and return 0, therefore effectively making the token
> + * equivalent to "specifier(0)", as it should be.
> + */
> + // KeyValue = atoi(p);
> + KeyValue = strtoul(p, (PSTR*)&p, 10);
> +
> + /* Skip any trailing whitespace */
> + while (*p && isspace(*p)) ++p;
isspace(0) is false so you don't need the *p check.
> +PCWSTR
> +ArcGetNextTokenU(
> + IN PCWSTR ArcPath,
> + OUT PUNICODE_STRING TokenSpecifier,
> + OUT PULONG Key)
> +{
> + HRESULT hr;
> + PCWSTR p = ArcPath;
> + ULONG SpecifierLength;
> + ULONG KeyValue;
> +
> + /*
> + * We must have a valid "specifier(key)" string, where 'specifier'
> + * cannot be the empty string, and is followed by '('.
> + */
> + p = wcschr(p, L'(');
> + if (p == NULL)
> + return NULL; /* No '(' found */
> + if (p == ArcPath)
> + return NULL; /* Path starts with '(' and is thus invalid */
> +
> + SpecifierLength = (p - ArcPath) * sizeof(WCHAR);
> +
> + ++p;
> +
> + /*
> + * The strtoul function skips any leading whitespace.
> + *
> + * Note that if the token is "specifier()" then strtoul won't perform
> + * any conversion and return 0, therefore effectively making the token
> + * equivalent to "specifier(0)", as it should be.
> + */
> + // KeyValue = _wtoi(p);
> + KeyValue = wcstoul(p, (PWSTR*)&p, 10);
> + ASSERT(p);
This assert seems superfluous, you just dereferenced the pointer. Also,
you incremented it just above and also (rightly) assumed it was larger
than ArcPath.
> +static NTSTATUS
> +ResolveArcNameManually(
> + OUT PUNICODE_STRING NtName,
> + IN OUT PCWSTR* ArcNamePath,
> + IN PPARTLIST PartList OPTIONAL)
> +{
> [...]
> +Quit:
> + if (FAILED(hr))
> + {
> + /*
> + * We can directly cast the HRESULTs into NTSTATUS since the error codes
> + * returned by StringCbPrintfW:
> + * STRSAFE_E_INVALID_PARAMETER == 0x80070057,
> + * STRSAFE_E_INSUFFICIENT_BUFFER == 0x8007007a,
> + * do not have assigned values in the NTSTATUS space.
> + */
> + return (NTSTATUS)hr;
To me that sounds like an argument of why you _can't_ do that.
(You know the Rtl versions of these functions give you an NTSTATUS btw,
right?)
> --- branches/setup_improvements/base/setup/lib/arcname_tests.c (added)
> +++ branches/setup_improvements/base/setup/lib/arcname_tests.c [iso-8859-1] Mon May 22 01:09:35 2017
> @@ -0,0 +1,142 @@
> +/*
> + * Tests for the arcname.c functions:
> + * - ArcPathNormalize(),
> + * - ArcPathToNtPath().
> + *
> + * You should certainly fix the included headers before being able
> + * to compile this file (as I didn't bother to have it compilable
> + * under our (P/N)DK, but just under VS).
> + */
Wine's test framework works okay for unit tests. You can make this an
apitest by just #include'ing the C source file, then it can run on
Testbot.