James Hawkins ha scritto:
On Mon, Jul 7, 2008 at 10:04 AM, Massimo Del Fedele max@veneto.com wrote:
I'll repost that one... I'd like some feedback if it looks wrong.
This isn't going to be committed without a test.
To maintain consistence between global MSI path properties (as CommonAppDataFolder) which are returned with a trailing backslash, and local MSI path properties (as LOCALAPPDATAFOLDER) which by now where returned without the trailing backslash.
Properties in MSI are just that...properties. Folders loaded from the Folder table just happen to be stored as properties. The AppSearch action looks for a path (in this case) and sets a user-defined property to the path of the directory. You're making a connection between these two completely separated sets of functionality that doesn't exist.
This solves autocad's installer bug 13838.
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.
I'll review your patch for you anyway:
@@ -368,6 +368,17 @@ static UINT ACTION_AppSearchReg(MSIPACKAGE *package, LPWSTR *appValue, MSISIGNAT switch (type & 0x0f) { case msidbLocatorTypeDirectory:
- /* directories should be returned with a trailing backslash
so check for it and append if necessary */Leave the comment out.
Why ? there are tons of comments just in appsearch.c Just to make an example of 'obvious' comment there :
/* check whether parent is set */ parentName = msi_dup_record_field(row,2); if (parentName)
It would have been better a phrase like "please leave comment out because xyzreason". Usually I like to know what and why I should do stuffs.
if((LPWSTR)value[sz-1] != L'\\'){Two things wrong. That first part is casting the result of value[sz-1], which is a BYTE (integer), to LPWSTR (pointer).
Right, my mistake... I forgot one parenthesis.
Second, you can't use L in Wine.
Again... why ? An explanation would bring no harm.
*appValue = (LPWSTR)value;value = msi_alloc(sz+1);You forgot to check if you're out of memory.
Just 3-4 lines above :
/* FIXME: sanity-check sz before allocating (is there an upper-limit * on the value of a property?) */ value = msi_alloc( sz ); (no check on returned value....)
And, just 3-4 lines below....
case msidbLocatorTypeFileName: *appValue = strdupW((LPWSTR)value); break; (no check on returned value)
BTW, I agree, it should be checked.
lstrcpyW((LPWSTR)value, *appValue);lstrcatW(value, L"\\");Again, can't use L in Wine.
.......
msi_free(*appValue);*appValue = NULL;} rc = ACTION_SearchDirectory(package, sig, (LPWSTR)value, 0, appValue); break;Now let's look at the whole thing:
*appValue = (LPWSTR)value; value = msi_alloc(sz+1); lstrcpyW((LPWSTR)value, *appValue); lstrcatW(value, L"\\"); msi_free(*appValue); *appValue = NULL;You're using *appValue as a temporary pointer, which is ugly. msi_realloc is your friend.
Thanx for the hint, I didn't know msi_realloc. BTW, that's why I didn't write a testcase, my skills on msi are too limited.
Check out the warnings you get when you compile this, which is the first sign that something is wrong:
appsearch.c: In function 'ACTION_AppSearchReg': appsearch.c:373: warning: cast to pointer from integer of different size appsearch.c:373: warning: comparison between pointer and integer appsearch.c:378: warning: passing argument 1 of 'lstrcatW' from incompatible pointer type appsearch.c:378: warning: passing argument 2 of 'lstrcatW' from incompatible pointer type
again, right. My mistake.
Taking one more step back:
@@ -368,6 +368,17 @@ static UINT ACTION_AppSearchReg(MSIPACKAGE *package, LPWSTR *appValue, MSISIGNAT switch (type & 0x0f) { case msidbLocatorTypeDirectory:
- /* directories should be returned with a trailing backslash
so check for it and append if necessary */So this 'fix' is only for ACTION_AppSearchReg. What about all the other AppSearch functionalities that also search for a directory?
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. Again, AppSearchReg MUST return path with backslash. About the other AppSearch, I don't know anything.
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. 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, ®Type, 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.
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. Maybe it doesn't fix the universe, but it does fix a bug, and just THAT bug, without fiddling with other stuffs. 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.
But maybe you wanted to say that is ACTION_SearchDirectory() that must be patched ? 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 ?
BTW, thanx for the review.
Max