From: Fabian Maurer Subject: Re: [PATCH] find: First simple implementation and tests Message-Id: <9669248.GM1dufnK2Z@arch> Date: Sat, 15 Sep 2018 22:38:15 +0200 In-Reply-To: <58776d26-8f5d-9871-fd50-63826c053cfe@codeweavers.com> References: <20180915154246.8132-1-dark.shadow4@web.de> <58776d26-8f5d-9871-fd50-63826c053cfe@codeweavers.com> On Samstag, 15. September 2018 20:42:46 CEST Nikolay Sivov wrote: > 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? I might have mixed that up with fc.exe, I'll take a second look. > > +BOOL read_char_from_handle(HANDLE handle, char *char_out) > > Function name does not correspond with what function is doing. You mean because it does buffering, too? > > + static char buffer[4096]; > > + static UINT buffer_max = 0; > > + static UINT buffer_pos = 0; > > What's a reason for this to be static? So it's not lost when the function returns. We read a lot from the handle, but only use one char at a time. This buffering is needed since reading one char at a time with ReadFile would be to inefficient. > > + /* 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. BOOL success you mean? I always pull that out of an if to make it more readable. > > + 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. Maybe, but it works in the tests for now. What would you propose? > > > +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? I'll take a look at it. AFAIK WriteFile doesn't guarantee all bytes to be written at once, that's why we have the "bytes written" parameter. Correct me if I'm wrong. > There's strstrW(), importing shlwapi only for that is too much. Ah, missed that one. I'll change it. > > > + 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. Sure thing, just loaded all of them at once for simplicity. Performance wise it shouldn't matter, but do you want me to load them only when needed? By checking the arguments, do you mean I should call it with buffer length 0 to get a readonly pointer?

On Samstag, 15. September 2018 20:42:46 CEST Nikolay Sivov wrote:

> 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?

 

I might have mixed that up with fc.exe, I'll take a second look.

 

 

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

>

> Function name does not correspond with what function is doing.

 

You mean because it does buffering, too?

 

 

> > + static char buffer[4096];

> > + static UINT buffer_max = 0;

> > + static UINT buffer_pos = 0;

>

> What's a reason for this to be static?

 

So it's not lost when the function returns. We read a lot from the handle, but only use one char at a time. This buffering is needed since reading one char at a time with ReadFile would be to inefficient.

 

 

> > + /* 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.

 

BOOL success you mean? I always pull that out of an if to make it more readable.

 

> > + 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.

 

Maybe, but it works in the tests for now. What would you propose?

 

>

> > +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?

 

I'll take a look at it. AFAIK WriteFile doesn't guarantee all bytes to be written at once, that's why we have the "bytes written" parameter. Correct me if I'm wrong.

 

 

> There's strstrW(), importing shlwapi only for that is too much.

 

Ah, missed that one. I'll change it.

 

 

>

> > + 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.

 

Sure thing, just loaded all of them at once for simplicity. Performance wise it shouldn't matter, but do you want me to load them only when needed?

 

By checking the arguments, do you mean I should call it with buffer length 0 to get a readonly pointer?

 

 

>

> > + 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 '/'?

 

It's only supposed to be a first implementation, I don't want to add everything in one huge commit. It's already gotten too big for my likes, I don't think we should add even more functionality for the first version.

 

Regards,

Fabian Maurer