From: Nikolay Sivov <nsivov@codeweavers.com>
Subject: Re: [PATCH] find: First simple implementation and tests
Message-Id: <58776d26-8f5d-9871-fd50-63826c053cfe@codeweavers.com>
Date: Sat, 15 Sep 2018 21:42:46 +0300
In-Reply-To: <20180915154246.8132-1-dark.shadow4@web.de>
References: <20180915154246.8132-1-dark.shadow4@web.de>

On 09/15/2018 06:42 PM, Fabian Maurer wrote:

> The message text is the same as on windows,
> since the exit code doesn't seem reliable.

Is it unreliable across Windows versions? I only see error message for 
invalid input, not for the case when string was or wasn't found. So why 
does it matter?

> +BOOL read_char_from_handle(HANDLE handle, char *char_out)

Function name does not correspond with what function is doing.

> +{
> +    static char buffer[4096];
> +    static UINT buffer_max = 0;
> +    static UINT buffer_pos = 0;

What's a reason for this to be static?

> +
> +    /* Read next content into buffer */
> +    if (buffer_pos >= buffer_max)
> +    {
> +        BOOL success = ReadFile(handle, buffer, 4096, &buffer_max, NULL);
> +        if (!success)

Separate variable does not seem to be necessary.

> +            return FALSE;
> +        buffer_pos = 0;
> +    }
> +
> +    *char_out = buffer[buffer_pos++];
> +    return TRUE;
> +}

> +/* Reads a line from a handle, returns TRUE if there is more to be read */
> +BOOL read_line_from_handle(HANDLE handle, WCHAR **line_out)
> +{
> +    int buffer_size = 4096;
> +    char c;
> +    int length = 0;
> +    WCHAR *line_converted;
> +    int line_converted_length;
> +    BOOL success;
> +    char *line = heap_alloc(buffer_size);
> +
> +    for (;;)
> +    {
> +        success = read_char_from_handle(handle, &c);
> +
> +        /* Check for EOF */
> +        if (!success)
> +        {
> +            if (length == 0)
> +                return FALSE;
> +            else
> +                break;
> +        }
> +
> +        if (c == '\n')
> +            break;
> +
> +        /* Make sure buffer is large enough */
> +        if (length + 1 >= buffer_size)
> +        {
> +            buffer_size *= 2;
> +            line = heap_realloc(line, buffer_size);
> +        }
> +
> +        line[length++] = c;
> +    }
> +
> +    line[length] = 0;
> +    if (length - 1 >= 0 && line[length - 1] == '\r')
> +        line[length - 1] = 0;
> +
> +    line_converted_length = MultiByteToWideChar(CP_ACP, 0, line, -1, 0, 0);
> +    line_converted = heap_alloc(line_converted_length * sizeof(WCHAR));
> +    MultiByteToWideChar(CP_ACP, 0, line, -1, line_converted, line_converted_length);
> +
> +    heap_free(line);
> +
> +    *line_out = line_converted;
> +    return TRUE;
> +}

CP_ACP is probably not correct in this case.

> +void write_to_handle(HANDLE handle, const WCHAR *str)
> +{
> +    DWORD bytes_written_sum = 0;
> +    char *str_converted;
> +    UINT str_converted_length;
> +    UINT str_length = lstrlenW(str);
> +
> +    str_converted_length = WideCharToMultiByte(CP_ACP, 0, str, str_length, NULL, 0, NULL, NULL);
> +    str_converted = heap_alloc(str_converted_length);
> +    WideCharToMultiByte(CP_ACP, 0, str, str_length, str_converted, str_converted_length, NULL, NULL);
> +
> +    do
> +    {
> +        DWORD bytes_written;
> +        WriteFile(handle, str_converted, str_converted_length, &bytes_written, NULL);
> +        bytes_written_sum += bytes_written;
> +    } while (bytes_written_sum < str_converted_length);
> +
> +    heap_free(str_converted);
> +}
> +
> +void find_printf(const WCHAR *str)
> +{
> +    write_to_handle(GetStdHandle(STD_OUTPUT_HANDLE), str);
> +}
Check how it's done in reg/regedit, probably you should reuse it here. 
Less important, but why two helpers here? And why do you need a loop to 
write this buffer?

> +BOOL run_find_for_line(const WCHAR *line, const WCHAR *tofind)
> +{
> +    void *found;
> +    WCHAR lineending[] = {'\r', '\n', 0};
> +
> +    if (lstrlenW(line) == 0)
> +        return FALSE;
> +
> +    found = StrStrW(line, tofind);
> +
> +    if (found)
> +    {
> +        find_printf(line);
> +        find_printf(lineending);
> +        return TRUE;
> +    }
> +
> +    return FALSE;
> +}
There's strstrW(), importing shlwapi only for that is too much.


> +    WCHAR message_parameter_invalid[64];
> +    WCHAR message_switch_invalid[64];
>       int i;
>
>
> +    LoadStringW(GetModuleHandleW(NULL), IDS_INVALID_PARAMETER, message_parameter_invalid, sizeof(message_parameter_invalid));
> +    LoadStringW(GetModuleHandleW(NULL), IDS_INVALID_SWITCH, message_switch_invalid, sizeof(message_switch_invalid));
You don't need them both at the same time. Also check the arguments.


> +    input = GetStdHandle(STD_INPUT_HANDLE);
> +
> +    for (i = 1; i < argc; i++)
> +    {
> +        if (argv[i][0] == '/')
> +        {
> +            find_printf(message_switch_invalid);
> +            return 2;
> +        }
> +        else if(tofind == NULL)
> +        {
> +            tofind = argv[i];
> +        }
> +    }
> +
> +    if (tofind == NULL)
> +    {
> +        find_printf(message_parameter_invalid);
> +        return 2;
> +    }
> +
> +    exitcode = 1;
> +    while (read_line_from_handle(input, &line))
> +    {
> +        if (run_find_for_line(line, tofind))
> +            exitcode = 0;
> +
> +        heap_free(line);
> +    }
I don't understand why you're reading from stdin uncoditionally. This 
utility takes options, string to find, and a list of file paths. Only 
when paths are not provided it should read from stdin. Will this work if 
you have your string argument starts with '/'?