[RESEND]MSI:ACTION_AppSearchReg() should return backslash terminated paths - Solves bug 13838

James Hawkins truiken at gmail.com
Mon Jul 7 14:20:54 CDT 2008


On Mon, Jul 7, 2008 at 1:56 PM, Massimo Del Fedele <max at veneto.com> wrote:
>
> Because AppSearchReg does search in reg, where paths are un-slashed.
> I could have fixed more low-level, of course, but then you'd have big
> probability to break unrelated stuffs.

Listen, I don't want to come off as mean, but like you say yourself,
you are not knowledgeable about msi or the Win32 API for that matter.
AppSearchReg queries the specified registry entry for a directory to
check for its existence.  You say, "in reg, where paths are
un-slashed."  There is no such definition of paths in the registry.  A
registry value contains whatever value the user set it to.

> Again, AppSearchReg MUST return path with backslash. About the other
> AppSearch, I don't know anything.

Ok, you've just verified that you're not thinking about this
correctly.  You should care about the other parts of AppSearch.

>> Those will remain unfixed.  Also, why are you making it the
>> responsibility of AppSearchReg to add the backslash?
> Just look at Package.c and you'll know why...
>     SHGetFolderPathW(NULL,CSIDL_STARTMENU,NULL,0,pth);
>     strcatW(pth,cszbs);
>     MSI_SetPropertyW(package, SMF, pth);
> as an example.

Again, what does that have to do with anything?  This is the special
MSI properties code...not the AppSearch action.  You are trying to
relate all these disparate parts that are completely unrelated.

>   You're assuming
>> that ACTION_SearchDirectory never changes the value sent in and only
>> copies it straight back if it finds the path.
>   That's a bad
>> assumption.
> Yep, right, I checked it on my case and it didn't rip off the backslash,
> but that could change... I should have done after it.
> Plus, if you fix this at the lowest level, you get a fix
>> for all the AppSearch functionalities for free.
>>
> The "lower level", looking at code, would be change RegQueryValueExW()
> behaviour :
>     rc = RegQueryValueExW(key, valueName, NULL, &regType, value, &sz);
> which indeed IS correct like ii is, and for that one I DID a testcase.
> That was my first "hack" on this, it worked but WAS WRONG.
>

Now knowing what I just told you about AppSearchReg above, you cannot
possibly think that I was suggesting you should change
RegQueryValueEx...

> Going back one step...
>
>  > Ok, we've had tons of patches submitted, some committed, that fix one
>  > app and break tons of other apps.  This is exactly why this has to be
>  > backed up by a test.
>
> My patch solves just one case that should indeed be solved. I don't say
> it's perfect solution, but that's the best I've found and I know it's
> correct.

By definition you can't know if it's correct unless you add tests for
it, and even then you can never be 100% sure (but it's pretty close).

> Maybe it doesn't fix the universe, but it does fix a bug, and
> just THAT bug, without fiddling with other stuffs.

...and maybe it breaks some other apps.  You act like I don't think
your fix is correct.  It probably is correct, but that's not the
point.  It's not tested, so you can't be certain that it's correct.  I
already said I'd add a test, so I don't know why you keep going on
about this.

> Making it "more low level" as you told, i.e. changing RegQueryValueExW()
> would be wrong and would probably break tons of stuffs that expect it
> (correctly) to return a path WITHOUT a trailing backslash.
>

See above.

> But maybe you wanted to say that is ACTION_SearchDirectory() that must
> be patched ?

Yes.  I thought that was obvious, but I'll try to not make those
assumptions again.

> Maybe that's right. BUT then, as it's not an exported
> function, you should test ALL usages of it and see if that's a correct
> behaviour. Well, It's used just in AppSearchDr, besides of
> AppSearchReg(). Maybe, knowing what is for AppSearchDr() I could know if
> it's correct to have its result backslashed too... But I don't. Again,
> my skills on MSI are *very* limited. I've chosen to fix just something I
> know must be fixed instead of try to fix the world without knowing
> anything about it. Wrong ?
>

Yes.  You're verifying everything I've told you several times.

> BTW, thanx for the review.
>

No problem.

-- 
James Hawkins



More information about the wine-devel mailing list