Patch to fix GetThreadTimes() to work for all threads - fixes Bug 20230

[Vitaliy Margolen]

First of all you bottom post here.

> -----Original Message-----
> From: Vitaliy Margolen [mailto:wine-devel at kievinfo.com]
> Sent: 19 April 2011 06:17
> To: wine-devel at winehq.org
> Cc: Ray Hinchliffe (RH)
> Subject: Re: Patch to fix GetThreadTimes() to work for all threads - fixes
> Bug 20230
>
> On 04/18/2011 12:45 AM, Ray Hinchliffe (RH) wrote:
>> The patch works by reading from /proc/<pid>/task/<tid>/stat. If this in
> not
>> possible it will report the fix messages as before.
>> Given that SystemProcessorPerformanceInformation uses /proc/stat I expect
> this
>
> Thanks for the patch, however there are several issues with it:
> - Don't attach changes to automatically generated files (server/trace.c,
> server/request.h, etc). Only server/protocol.def should be included, which
> you didn't even modify.
> - Keep formatting of the original file you modifying. No whitespace only
> changes, no funky new formatting styles.
> - Please use Windows types instead of system types for integers (ULONG
> instead of unsigned long)
>
>> +                    sprintf( buf, "/proc/%u/task/%u/stat",
> reply->unix_pid, reply->unix_tid );
> Please don't call any functions in the server call context. Also you not
> using this buffer if server call fails, or if the thread is the same.
>
> Vitaliy.

On 04/19/2011 01:03 AM, Ray Hinchliffe (RH) wrote:
> Thank you for checking out the patch and pointing out the issues. I have
> never made a server change before so I am wondering if there is a document
> that tells me how to correctly make a server change? How do I stop git
> putting them in the patch?
Make 2 separate patches. One with files you want to send another one with 
auto-generated files. And don't send second one. For details see git add --help.

> OK about ULONG, but in struct init_thread_request unix_pid and unix_tid int
> which is why I used unsined long for the others. Also looking in ntdll/nt.c
> it has "unsigned long clk_tck = sysconf(_SC_CLK_TCK);". Should I use const
> or CONST?
If you need to use a variable with system function - it should have 
appropriate type. But if it's internal variable then it should have windows 
type. Especially if it's anything "long".


> I felt I should try and fix the patch anyway, so here is my revised one.  I
You still have auto-generated files attached. Still have stile changes.

> +                    ULONG usr;
> +                    ULONG sys;
> +                    ULONG ctt;
> +                    ULONG got;
All these can be declared in one line.

> +                        fgets(buf, sizeof(buf) - 1, fid);
fgets already subtracts 1 from size for terminating \0.

> +  for( got = 0, pos = buf; (got < 13) && pos; got++, pos = strchr(pos + 1, ' ')) {}
Please don't do this. Put at least something into loop body.

> + if( got = pos && (sscanf(pos, " %u %u", &usr, &sys) == 2))
You sure gcc doesn't complain about "got = pos" here? Also it should be:
"if (foo)" not "if( foo )" as the rest of the function.

Vitaliy.