[ros-dev] [ros-diffs] [hbelusca] 74621: [USETUP]: SetupLib: Add an ARC path to (and from) NT path resolver. It allows mapping between an ARC path as specified in freeldr.ini / boot.ini , to its corresponding NT path, if...

[Thomas Faber]

On 2017-05-22 03:09, hbelusca at 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.