Re: [ros-dev] [ros-diffs] [fireball] 50981: [NTOS/FSRTL] - Implement parameters checking i

[Pierre Schweitzer]

Hi,

this commits just shows you didn't understand how that function works. If you had, you wouldn't have said:

"- Implement parameters checking in FsRtlIsNameInExpressionPrivate."
It's already done.

"- Add two shortcuts for common wildcard invocations to make the function faster."
If you look at the algorithm, it will take at maximum as much loop iterations as name length.

"- Second (main part of the function) is still under review."
Thanks for discussing the issue with the original author of the algorithm, especially since you don't understand it. Furthermore, if you're not happy with said algorithm, you were asked to review it more than a year ago, and for at least 6 months. Thing that you NEVER did.

So, now you discovered what my mail address is, contact me before committing anything wrong/useless to current code.
And as you said less than a week ago, focus on fixing things that don't work, instead of rewriting working code.

And as someone else also said once: 1:1 isn't the solution everywhere. Or if you think so, start dropping arwinss.

Of course, don't take that personal, I like everyone ;-).

Regards,
P. Schweitzer

[Aleksey Bragin]

On Mar 7, 2011, at 12:43 AM, Pierre Schweitzer wrote:
> Hi,
>
> this commits just shows you didn't understand how that function  
> works. If you had, you wouldn't have said:
> "- Implement parameters checking in FsRtlIsNameInExpressionPrivate."
> It's already done.

Enlighten me please, where? Is it crypted in the last return  
statement? If so, how many of you would decipher, it should return  
TRUE if both Name and Expression are empty, and false if any of them  
is not empty? And how many of you would break this in an attempt to  
improve the algorithm and thus break that nice 119 chars return  
statement?
> "- Add two shortcuts for common wildcard invocations to make the  
> function faster."
> If you look at the algorithm, it will take at maximum as much loop  
> iterations as name length.

Yeah, but not making those loop iterations is even better for,  
probably, 90% of function invocations. Also I will be honest: I'm not  
sure that loop is correct but I'm not sure it's incorrect either ("a  
joke about Schroedinger cat could be funny and not funny at the same  
moment of time").
Constants updates to the code making it less and less readable only  
add up to my concerns, with latest pool overruns too.
And in contrary, at the very least I am sure those parts I committed  
are perfectly working code.
> "- Second (main part of the function) is still under review."
> Thanks for discussing the issue with the original author of the  
> algorithm, especially since you don't understand it. Furthermore,  
> if you're not happy with said algorithm, you were asked to review  
> it more than a year ago, and for at least 6 months. Thing that you  
> NEVER did.

I review my own code too, what's the matter? Also, if it looked fine  
for me a year ago, it doesn't mean it's perfect and it should never  
be reviewed again. For your information, I review even the most  
perfectly working code in the kernel, and sometimes that's rewarded  
by finding nice typos leading to unpredicted results in a not very  
often executed branch of a code.
> So, now you discovered what my mail address is, contact me before  
> committing anything wrong/useless to current code.
> And as you said less than a week ago, focus on fixing things that  
> don't work, instead of rewriting working code.

Somehow I thought that 7+ years of hacking this kernel (starting from  
revision 4578, and 522 kernel commits over time) give me some idea of  
what is correct, what is useful and what does not work.
> And as someone else also said once: 1:1 isn't the solution  
> everywhere. Or if you think so, start dropping arwinss.

Yeah, I don't disagree with my own words, however it was the case  
when it's a good addition to the code. Maxim Shatskih even outlined  
there is a bug in the original Windows implementation of that  
function and suggested (no, not to us, suggested to someone in some  
forum thread) to use something better "like grep source code".
> Of course, don't take that personal, I like everyone ;-).

If I would take anything personal, I wouldn't survive in this project  
for that long  :D

WBR,
Aleksey.

[Timo Kreuzer]

Aleksey: you have no clue how the kernel works.
Pierre: your code sucks "as usual"(tm).

Please don't take it personally, I hate everyone.

;-)

[James Tabor]

I like everyone here!

On Sun, Mar 6, 2011 at 5:10 PM, Timo Kreuzer [e-mail] wrote:
> Aleksey: you have no clue how the kernel works.
> Pierre: your code sucks "as usual"(tm).
>
> Please don't take it personally, I hate everyone.
>
> ;-)

[Pierre Schweitzer]

Hi,
> > this commits just shows you didn't understand how that function  
> > works. If you had, you wouldn't have said:
> > "- Implement parameters checking in FsRtlIsNameInExpressionPrivate."
> > It's already done.
>
> Enlighten me please, where? Is it crypted in the last return  
> statement? If so, how many of you would decipher, it should return  
> TRUE if both Name and Expression are empty, and false if any of them  
> is not empty? And how many of you would break this in an attempt to  
> improve the algorithm and thus break that nice 119 chars return  
> statement?

This answer confirms you didn't understand the algorithm. So, let's throw some light on it.
The principle of that algorithm is simple: linear search on name using wildcards. This leads to the first and major assertion. This algorithm will determine if a name is matching an expression in at max n iterations[1] (where n is of course name length).
Once you know that, the return just look obvious. When you reach return, you compare how much in both strings you accepted. If you reach end of both string, then name is matching expression. If not, some chars weren't accepted.
Given that, you clearly understand that if you give two null strings, then it will be accepted as both string browsing won't have started, and both string will have 0-length. If you give only one not null, then, the main loop won't even start, you'll just reach the return statement. That will return false as one string won't have been browsed.
The return statement is perhaps 119 chars long, but it's just a stupid logical AND. So, who care about its length?
If you prefer, I can even come back with my "i", "j", "k" vars. Return statement will be shorter.

Regarding the "how many", I'd simply say... If you try to understand the algorithm before committing your stuff on it, then chances to break it are lowered.
And even if you fail to understand the algorithm, it's worth contacting the author to get details about how it works, isn't it?

Finally, about your second shortcut, in worse case, you're browsing expression twice, name once.
Main algorithm, in worse case is browsing expression once, name once.
Still sure about the term "shortcut"?
And we may wonder why you upcase expression in your test, whereas you're not supposed to do so, and you even asserted it was not needed.
And first shortcut is already in current code. Proof: it enters the loop, reads the *. Checks it's end. Accepts name. And leaves. No iteration.
Line 100, line 160, line 163, line 165, line 166, line 204.
> > "- Add two shortcuts for common wildcard invocations to make the  
> > function faster."
> > If you look at the algorithm, it will take at maximum as much loop  
> > iterations as name length.
>
> Yeah, but not making those loop iterations is even better for,  
> probably, 90% of function invocations. Also I will be honest: I'm not  
> sure that loop is correct but I'm not sure it's incorrect either ("a  
> joke about Schroedinger cat could be funny and not funny at the same  
> moment of time").
> Constants updates to the code making it less and less readable only  
> add up to my concerns, with latest pool overruns too.
> And in contrary, at the very least I am sure those parts I committed  
> are perfectly working code.

I'm deeply sorry, I'm not a genius. When first designing the algorithm, I didn't took into account all the possible cases. And when new cases were reported, I just improved the algorithm to make it handle them. So, yes, I somehow proceed by trials-and-errors, but don't forget to look at kmtest, we now have a really complete test case there.

Your remark only raises an error I did, I didn't commented code. Definitely true, huge mistake. Thus, I can be asked to do so.

For pool overruns issue, that's the poorest excuse I've ever seen. In my commit message there were several lines. Surely one reads: "This fixes potential buffer overruns.". Yeah, great, I coded poor algorithm you won!
Actually, you should have read line right upper that reads: "As both UNICODE_STRING & ANSI_STRING might not be NULL-termined, don't attempt to read null char.". Here is the explanation. I did the wrong explanation that strings were always null-terminated, so, if the algorithm was reading one char farther, it was no problem as string were null-terminated. In fact, as they're not always null-terminated (thanks here go to MSDN), I was reading outside of the buffer. My mistake. It's now fixed. You can check bug #5923 to ensure that (http://www.reactos.org/bugzilla/show_bug.cgi?id=5923). A C++ program will help you that way.
> > "- Second (main part of the function) is still under review."
> > Thanks for discussing the issue with the original author of the  
> > algorithm, especially since you don't understand it. Furthermore,  
> > if you're not happy with said algorithm, you were asked to review  
> > it more than a year ago, and for at least 6 months. Thing that you  
> > NEVER did.
>
> I review my own code too, what's the matter? Also, if it looked fine  
> for me a year ago, it doesn't mean it's perfect and it should never  
> be reviewed again. For your information, I review even the most  
> perfectly working code in the kernel, and sometimes that's rewarded  
> by finding nice typos leading to unpredicted results in a not very  
> often executed branch of a code.

Let me correct one broken sentence here: "Also, if it looked fine for me a year ago". It didn't. You didn't ever read it. I finally committed to trunk on some dev advice.
> > So, now you discovered what my mail address is, contact me before  
> > committing anything wrong/useless to current code.
> > And as you said less than a week ago, focus on fixing things that  
> > don't work, instead of rewriting working code.
>
> Somehow I thought that 7+ years of hacking this kernel (starting from  
> revision 4578, and 522 kernel commits over time) give me some idea of  
> what is correct, what is useful and what does not work.

Surely 7+ years of hacking this kernel give you some idea of what is correct. Still to ensure something is correct, don't you need to understand it?
And actually, that algorithm is not really about kernel. As you can see, it's easy to get it in user-mode, so you may be skilled as much as you want about kernel, it's no help here.
And with a so excellent résumé, I would advice you switch to PnP. It's really broken, badly designed, and not that working, and in the kernel. I heard at least two persons working in two different areas complaining about PnP being broken. You would really help them doing that, instead of fixing working code.
> > And as someone else also said once: 1:1 isn't the solution  
> > everywhere. Or if you think so, start dropping arwinss.
>
> Yeah, I don't disagree with my own words, however it was the case  
> when it's a good addition to the code. Maxim Shatskih even outlined  
> there is a bug in the original Windows implementation of that  
> function and suggested (no, not to us, suggested to someone in some  
> forum thread) to use something better "like grep source code".

Which bug? Any link where to read bout that bug? Any serious proof?
And if that guy was that documented, he wouldn't have suggested to use grep source code. Glob isn't regexp. Both are using expressions to match names, but expressions have nothing in common. It would have been more accurate to advice using libc glob source code.
So, to let you know, I already thought about using libc source code. But:
1 - libc source code is even uglier that code we currently have. It eats memory, is recursive, and handles many things we don't need.
2 - On the other side, it doesn't handle enough things we need. Microsoft in that function is using particular wildcards that aren't in "standard" glob. That's why rewriting looked like the best solution. And I still think it is.

Now given all the information, I kindly ask you to revert both r50981 & r50982.
Feel free to review to whole algorithm, I'm sure you'll find issues. In fact, you wouldn't fine some (serious) issues, I would say you badly reviewed it.
Because yes, that algorithm still has issues, but we're not likely to hit them before a while. So, no worries. I'll fix them in time. I'll explain them if you wish. To whom it may interest, your commit doesn't tends to fix those issues at all, it even don't concern them.
Anyway, back to the demand, so, revert, review. If you find something wrong then contact me telling something like: "Timo wouldn't have said better, your code sucks. This looks wrong to me because bla, see this, I suggest that".

Do not forget I wrote all that code, and I think I might knew it a bit better than you believe.

Last part of that mail, is dedicated to Ged.
I'm deeply sorry for previous mail. It wasn't nice, you're definitely true.
I won't say I was right writing that way, but I'll give at least to pieces of information regarding that.
1 - I was really exhausted (yeah, I shouldn't have written, then. But it really pisses me off).
2 - I announced to Aleksey less than a week ago (still...), that I was thinking about retiring from active devs due to some internal issues. I was taking a break (vacations, trains, small coding in an other ReactOS area, just to cool down). And here is the result: starting of rewrite of code I wrote, without even asking whereas before everything was cool. This not-that-fair behaviour just made me quite *angry*.
Especially with the idea of "yet you're almost done with your code, let's review it", whereas I asked for such a review a long while ago without any success! Remember pierre-fsd branch ~

I know I could have write better email, less rude. But all those points cumulated didn't help leading to "let's suggest he may be wrong with his commit" mind-state.
Think twice before pushing send... (either for a commit or an email).

Finally, if one of you felt offended, I beg your pardon.

Regards,
P.
 
[1]: There is one case where the algorithm is a bit worse than n iterations. I let find you where and why. I left that case in the algorithm since it's not linkely to happen and would reveal a coding error on caller side. And in such case, it's just doing n + small constant iterations. It could be optimised in current, but I made choice not to optimise, as it would make code a bit less clear, and wouldn't bring that much improvements.

[Ged Murphy]

Everyone gets angry and frustrated.
Only the guy that acknowledges it with an apology regains his respect :)

Ged.

-----Original Message-----
From: [e-mail] [mailto:[e-mail]] On
Behalf Of Pierre Schweitzer
Sent: 07 March 2011 10:22
To: ReactOS Development List
Subject: Re: [ros-dev] [ros-diffs] [fireball] 50981: [NTOS/FSRTL] -
Implement parameters

Last part of that mail, is dedicated to Ged.
I'm deeply sorry for previous mail. It wasn't nice, you're definitely true.
I won't say I was right writing that way, but I'll give at least to pieces
of information regarding that.
1 - I was really exhausted (yeah, I shouldn't have written, then. But it
really pisses me off).
2 - I announced to Aleksey less than a week ago (still...), that I was
thinking about retiring from active devs due to some internal issues. I was
taking a break (vacations, trains, small coding in an other ReactOS area,
just to cool down). And here is the result: starting of rewrite of code I
wrote, without even asking whereas before everything was cool. This
not-that-fair behaviour just made me quite *angry*.
Especially with the idea of "yet you're almost done with your code, let's
review it", whereas I asked for such a review a long while ago without any
success! Remember pierre-fsd branch ~

I know I could have write better email, less rude. But all those points
cumulated didn't help leading to "let's suggest he may be wrong with his
commit" mind-state.
Think twice before pushing send... (either for a commit or an email).

Finally, if one of you felt offended, I beg your pardon.

[Adam]

In that case, I acknowledge it and I am sorry.
(can I get a free limo ride and a Rolls Royce as a token of respect  
please?)

On Mon, 07 Mar 2011 21:57:52 +1100, Ged Murphy [e-mail] wrote:
> Everyone gets angry and frustrated.
> Only the guy that acknowledges it with an apology regains his respect :)
>
> Ged.
>
> -----Original Message-----
> From: [e-mail] [mailto:[e-mail]] On
> Behalf Of Pierre Schweitzer
> Sent: 07 March 2011 10:22
> To: ReactOS Development List
> Subject: Re: [ros-dev] [ros-diffs] [fireball] 50981: [NTOS/FSRTL] -
> Implement parameters
>
> Last part of that mail, is dedicated to Ged.
> I'm deeply sorry for previous mail. It wasn't nice, you're definitely  
> true.
> I won't say I was right writing that way, but I'll give at least to  
> pieces
> of information regarding that.
> 1 - I was really exhausted (yeah, I shouldn't have written, then. But it
> really pisses me off).
> 2 - I announced to Aleksey less than a week ago (still...), that I was
> thinking about retiring from active devs due to some internal issues. I  
> was
> taking a break (vacations, trains, small coding in an other ReactOS area,
> just to cool down). And here is the result: starting of rewrite of code I
> wrote, without even asking whereas before everything was cool. This
> not-that-fair behaviour just made me quite *angry*.
> Especially with the idea of "yet you're almost done with your code, let's
> review it", whereas I asked for such a review a long while ago without  
> any
> success! Remember pierre-fsd branch ~
>
> I know I could have write better email, less rude. But all those points
> cumulated didn't help leading to "let's suggest he may be wrong with his
> commit" mind-state.
> Think twice before pushing send... (either for a commit or an email).
>
> Finally, if one of you felt offended, I beg your pardon.
>
> Ros-dev mailing list
> [e-mail]
> http://www.reactos.org/mailman/listinfo/ros-dev
-- 
Using Opera's revolutionary email client: http://www.opera.com/mail/

[Aleksey Bragin]

On Mar 7, 2011, at 1:21 PM, Pierre Schweitzer wrote:
> Hi,
> > > this commits just shows you didn't understand how that function
> > > works. If you had, you wouldn't have said:
> > > "- Implement parameters checking in FsRtlIsNameInExpressionPrivate."
> > > It's already done.
> >
> > Enlighten me please, where? Is it crypted in the last return
> > statement? If so, how many of you would decipher, it should return
> > TRUE if both Name and Expression are empty, and false if any of them
> > is not empty? And how many of you would break this in an attempt to
> > improve the algorithm and thus break that nice 119 chars return
> > statement?
>
> And we may wonder why you upcase expression in your test, whereas  
> you're not supposed to do so, and you even asserted it was not needed.

Thanks, that upcasing was really unnecessary, I committed a fix in  
50992.
> This answer confirms you didn't understand the algorithm. So, let's  
> throw some light on it.
> Line 100, line 160, line 163, line 165, line 166, line 204.

Common practice for any (kernel) function really (if you look at all  
other APIs in Ob, Ex, Ke, etc) is to:
1. Check parameters for validity and return if they are invalid or if  
no work of an algorithm is required.
2. Do the work
3. Return result of calculations and status.

Sometimes, step 2. is divided into two substeps:
2.1. Check for simple cases and return right away. This may be seen  
as optimisation or simplifying.
2.2. Factoring out complex parts of an algorithm to subblocks of code  
(prepare some data, process it, compare and calculate results).

Or sometimes, if there is a complex algorithm, a common approach is  
to divide the generic algorithm into two parts:
a. First part deals with simple cases and provides result right away
b. Second part deals with complex cases.

Why is it done so? They should have taught you this in the higher  
school, but I will explain. If you put a+b into one algo, it'll be  
more complex by definition. If you're able to decompose the original  
problem into a set of smaller sub-problems, you win: simplicity,  
maintainability and easier development/testing.
> I'm deeply sorry, I'm not a genius. When first designing the  
> algorithm, I didn't took into account all the possible cases. And  
> when new cases were

That's what I'm talking about. You were so much frustrated that I  
dared to touch your code, that I got an impression that you are  
genius really, and consider all your code as being perfectly perfect.
> For pool overruns issue, that's the poorest excuse I've ever seen.  
> In my commit message there were several lines. Surely one reads:  
> "This fixes potential buffer overruns.". Yeah, great, I coded poor  
> algorithm you won!

I didn't really questioned authorship of the algorithm. If an  
algorithm contains errors, I don't really care who wrote it, I would  
just go and fix it.
> And with a so excellent résumé, I would advice you switch to PnP.

Thanks for the advice, I have a full queue of such advices, but time  
is not infinite, so I have to put some priorities.
> Which bug? Any link where to read bout that bug? Any serious proof?

First hit in google:
http://www.osronline.com/showThread.CFM?link=180514
> Now given all the information, I kindly ask you to revert both  
> r50981 & r50982.

Sorry, request very politely rejected.
> Because yes, that algorithm still has issues, but we're not likely  
> to hit them before a while.

Yeah that's why I wanted to remove complexity from the underlying  
algorithm and factor out two simplest and the most common queries  
into standalone code path. Now it's easier to improve that algorithm  
to deal with more complex scenarios.

P.S. Why do you really sound so much offended by those codelines?  
They have nothing against you, neither about possible problems, it's  
not really your fault. I do mistakes too and what? It's much better  
to learn on mistakes than try to argue about them.

Anyway, it's not a productive way at all. If we start arguing about  
personal offences, then ReactOS would remain a toy project forever.  
It's time to get serious and put away all your personal frustration,  
internal issues and the like.

WBR,
Aleksey.

[Pierre Schweitzer]

> It's time to get serious

Then, rewrite the whole algorithm. Adding things that are already done is just pointless. Mixing styles, algorithms is also pointless, not to say hazardous.

Being serious is also communicating with the code authors before thinking you know better than them.
Being serious is also reviewing code when asked. Should I say how much time I wasted in that function? It could have been used elsewhere.

To end that: rewrite the function. Completely. Get rid of old code, it won't be any use here as it doesn't match proper coding style. I won't maintain this function until your complete rewrite is over.

I'm done here.

[Aleksey Bragin]

I didn't reply to the most important part of an email, I feel I should:

On Mar 7, 2011, at 1:21 PM, Pierre Schweitzer wrote:
> Finally, if one of you felt offended, I beg your pardon.

Apology taken, thanks for understanding.

I would still stand on my points regarding the actual code ;)

WBR,
Aleksey.

[Ged Murphy]

Probably one of the rudest emails I've read in a long while.
You should be ashamed.

-----Original Message-----
From: [e-mail] [mailto:[e-mail]] On
Behalf Of Pierre Schweitzer
Sent: 06 March 2011 21:43
To: [e-mail]
Subject: Re: [ros-dev] [ros-diffs] [fireball] 50981: [NTOS/FSRTL] -
Implement parameters checking i

Hi,

this commits just shows you didn't understand how that function works. If
you had, you wouldn't have said:

"- Implement parameters checking in FsRtlIsNameInExpressionPrivate."
It's already done.

"- Add two shortcuts for common wildcard invocations to make the function
faster."
If you look at the algorithm, it will take at maximum as much loop
iterations as name length.

"- Second (main part of the function) is still under review."
Thanks for discussing the issue with the original author of the algorithm,
especially since you don't understand it. Furthermore, if you're not happy
with said algorithm, you were asked to review it more than a year ago, and
for at least 6 months. Thing that you NEVER did.

So, now you discovered what my mail address is, contact me before committing
anything wrong/useless to current code.
And as you said less than a week ago, focus on fixing things that don't
work, instead of rewriting working code.

And as someone else also said once: 1:1 isn't the solution everywhere. Or if
you think so, start dropping arwinss.

Of course, don't take that personal, I like everyone ;-).

Regards,
P. Schweitzer

_______________________________________________
Ros-dev mailing list
[e-mail]
http://www.reactos.org/mailman/listinfo/ros-dev