Re: [RESEND]MSI:ACTION_AppSearchReg() should return backslash terminated paths - Solves bug 13838
On Mon, Jul 7, 2008 at 10:04 AM, Massimo Del Fedele <max(a)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. + 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). Second, you can't use L in Wine. + *appValue = (LPWSTR)value; + value = msi_alloc(sz+1); You forgot to check if you're out of memory. + 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; case msidbLocatorTypeFileName: 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. 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 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? Those will remain unfixed. Also, why are you making it the responsibility of AppSearchReg to add the backslash? 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. Plus, if you fix this at the lowest level, you get a fix for all the AppSearch functionalities for free. -- James Hawkins
James Hawkins ha scritto:
On Mon, Jul 7, 2008 at 10:04 AM, Massimo Del Fedele <max(a)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; case msidbLocatorTypeFileName:
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
On Mon, Jul 7, 2008 at 1:56 PM, Massimo Del Fedele <max(a)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, ®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.
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
James Hawkins ha scritto:
On Mon, Jul 7, 2008 at 1:56 PM, Massimo Del Fedele <max(a)veneto.com> wrote:
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. I obviously meant "values contained in registry that are paths", but it seemed to me clear enough how I said. We are talking about values in registry which REPRESENTS paths, and are REPRESENTED in registry as not backslash terminated paths, as all registry contained paths that I'm aware of.
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.
That HAS to do with the other case, because AppSearchReg is used, among others, to retrieve paths from registry and set corresponding local properties. If you look deeply enough inside code (no time now to copy/paste it), you'll see that LOCAL path properties (as LOCALAPPFOLDER, for example) are get from registry using AppSearch and set up with its returning value. EXACTLY as my example reported before about the hard-coded global properties, where paths are read from registry, a backslash is appended and are stored as global properties. The ONLY difference is the missing "append-the-backslash" stuff when setting up local path properties. I can see a STRONG relation between them, don't you ? .................
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).
You can't even with 100 tests. What you can have is a "reasonable knowledge" that it "should" be right. What I've done in my (buggy indeed) patch was to limit intervention to just ONE problem.
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. I do because I'm convinced that the way I solved is ONE of possible RIGHT ways, and because I've got tons of people that would like to run the autocad application without having to build wine from source...That's all. I still think it's better a partial fix that solves a problem for many people than wait for a "maybe-perfect-fix-in-another-year-or-so". I've seen tons of patches that caused regressions, you're right on it, but mine is so simple and straightforward that simply can't do harm, but solves a problem. Nobody says that a "better" patch can't be applied in future. I located the problem since... let's say 3 months ? as nothing was moving, I found an hack that solved the problem, at first, and, as nothing was moving on again I added a patch that do the same without being an hack. Autocad is one of most requested apps for wine, and nothing was moving since years... I'd say about 5-6 years, when I found some solution for R14 and 2000 versions.
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.
Me too. As I said, my patch solves ONE problem, not all related ones. We're speaking about it since 3 weeks, now, and I still haven't seen a better solution nor a demonstration that my way is bad.
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.
I already verified that the patch does no harm AND solves a problem. For me that's more than enough. Ciao Max
Massimo Del Fedele wrote:
Yes. You're verifying everything I've told you several times.
I already verified that the patch does no harm AND solves a problem. For me that's more than enough.
Please make a test and submit it along with your patch. Otherwise your changes won't get in. Vitaliy.
I already verified that the patch does no harm AND solves a problem. For me that's more than enough.
That's fine for a patch for your local tree, but it's not enough for Wine. Submit with a test case that proves you're right and we'll be more likely to accept your patch. --Juan
Massimo Del Fedele wrote:
I already verified that the patch does no harm AND solves a problem. For me that's more than enough.
Ciao
Max
Can you provide proof that beyond the shadow of a doubt it will always be correct? One way to provide that proof is to write a test. -Zac
Zac Brown ha scritto:
Massimo Del Fedele wrote:
I already verified that the patch does no harm AND solves a problem. For me that's more than enough.
Ciao
Max
Can you provide proof that beyond the shadow of a doubt it will always be correct? Nobody can :-)
One way to provide that proof is to write a test.
Even a test is not 100% proof. OTOH, the proof that as it is now is wrong, is autocad installer itself. The patch is so simple and not invasive that IMHO the time I'd need to learn how to build a testcase and to do it is not worth the effort. BTW, for me is no problem at all. I'm happy with the result on my three, I'm using autocad on my ubuntu desktop and it works perfectly, and for me programming is just an hobby, autocad and civil engineering software belongs to my real job. The patch was for other people wishing the same on an unpatched wine; as many people asked me I posted it, but the spare time I can spend on it is finished. If somebody else wants to take care of testcase-patch and so on, he's wellcome :-) Ciao and thanx to all for answers. Max
participants (5)
-
James Hawkins -
Juan Lang -
Massimo Del Fedele -
Vitaliy Margolen -
Zac Brown