Rejected patches needing review.
Hello. I have a problem loading a specific proprietary driver. I traced the problem to two bugs in: - services - winedevice The two bugs caused the loading to fail. I created and tested two fixes to solve the problem, but both patches were silently rejected. Can some review them before i re-send them? I have looked them over several time, but i do not see a problem with them. The only "problem" is that there are no test cases for these programs, as there is no such place for such. Thanks, /pedro
On Mon, Sep 15, 2008 at 11:27:34PM +0200, Peter Dons Tychsen wrote:
Hello.
I have a problem loading a specific proprietary driver. I traced the problem to two bugs in:
- services - winedevice
The two bugs caused the loading to fail. I created and tested two fixes to solve the problem, but both patches were silently rejected.
Can some review them before i re-send them? I have looked them over several time, but i do not see a problem with them. The only "problem" is that there are no test cases for these programs, as there is no such place for such.
I was looking at adding some support for testing of the wine programs for another bug. Anyway I added initial support for building tests for programs, and I hadn't sent it in as I have yet to add the test cases. Current structure I have pretty much copies the rules Makefile used for dll's with some minor changes. Would it be worth sending this in, or should I be looking to try and keep it to one rules file and keep that generic enough for test changes to both dlls and exes. Not sure if it's possible but I can try. -- Darragh "Nothing is foolproof to a sufficiently talented fool."
Hi Peter, diff --git a/programs/services/services.c b/programs/services/services.c index 36ed117..cd71a1f 100644 --- a/programs/services/services.c +++ b/programs/services/services.c @@ -106,9 +106,9 @@ static DWORD load_service_config(HKEY hKey, struct service_entry *entry) return err; if ((err = load_reg_string(hKey, SZ_DESCRIPTION, 0, &entry->description)) != 0) return err; - if ((err = load_reg_multisz(hKey, SZ_DEPEND_ON_SERVICE, &entry->dependOnServices)) != 0) + if ((err = load_reg_multisz(hKey, SZ_DEPEND_ON_SERVICE, TRUE, &entry->dependOnServices)) != 0) return err; - if ((err = load_reg_multisz(hKey, SZ_DEPEND_ON_GROUP, &entry->dependOnGroups)) != 0) + if ((err = load_reg_multisz(hKey, SZ_DEPEND_ON_GROUP, FALSE, &entry->dependOnGroups)) != 0) return err; if ((err = load_reg_dword(hKey, SZ_TYPE, &entry->config.dwServiceType)) != 0) diff --git a/programs/services/services.h b/programs/services/services.h index fd99bf9..cd7de02 100644 --- a/programs/services/services.h +++ b/programs/services/services.h @@ -87,7 +87,7 @@ LPWSTR strdupW(LPCWSTR str); BOOL check_multisz(LPCWSTR lpMultiSz, DWORD cbSize); DWORD load_reg_string(HKEY hKey, LPCWSTR szValue, BOOL bExpand, LPWSTR *output); -DWORD load_reg_multisz(HKEY hKey, LPCWSTR szValue, LPWSTR *output); +DWORD load_reg_multisz(HKEY hKey, LPCWSTR szValue, BOOL bAllowSingle, LPWSTR *output); DWORD load_reg_dword(HKEY hKey, LPCWSTR szValue, DWORD *output); static inline LPCWSTR get_display_name(struct service_entry *service) diff --git a/programs/services/utils.c b/programs/services/utils.c index 89eb500..191cc5f 100644 --- a/programs/services/utils.c +++ b/programs/services/utils.c @@ -102,7 +102,7 @@ failed: return err; } -DWORD load_reg_multisz(HKEY hKey, LPCWSTR szValue, LPWSTR *output) +DWORD load_reg_multisz(HKEY hKey, LPCWSTR szValue, BOOL bAllowSingle, LPWSTR *output) { DWORD size, type; LPWSTR buf = NULL; @@ -118,7 +118,7 @@ DWORD load_reg_multisz(HKEY hKey, LPCWSTR szValue, LPWSTR *output) } goto failed; } - if (type != REG_MULTI_SZ) + if (!((type == REG_MULTI_SZ) || ((type == REG_SZ) && bAllowSingle))) { err = ERROR_INVALID_DATATYPE; goto failed; It seems as though the bAllowSingle variable is useless, as the only caller of it tries first with TRUE and again with FALSE. If the registry value is of type REG_SZ, the first call (when bAllowSingle is TRUE) will succeed. Similarly, if it's of type REG_MULTI_SZ, the first call will succeed. Thus, the second call, with bAllowSingle is FALSE, is always unnecessary. Furthermore, since there's only one place that calls load_reg_multisz, and it accepts both REG_SZ and REG_MULTI_SZ, why not just change load_reg_multisz always to accept either REG_SZ or REG_MULTI_SZ? Complicating the interface unnecessarily isn't a good change. I have a minor quibble with your second patch: LPWSTR path = NULL, str; @@ -174,17 +175,31 @@ static BOOL load_driver(void) /* read the executable path from memory */ size = 0; - if (RegQueryValueExW( driver_hkey, ImagePathW, NULL, &type, NULL, &size )) return FALSE; - - str = HeapAlloc( GetProcessHeap(), 0, size ); - if (!RegQueryValueExW( driver_hkey, ImagePathW, NULL, &type, (LPBYTE)str, &size )) + if (!RegQueryValueExW( driver_hkey, ImagePathW, NULL, &type, NULL, &size )) + { + str = HeapAlloc( GetProcessHeap(), 0, size ); You only use str in the then branch of this if statement, so its scope should be changed: it should be declared where, where it's allocated and later freed, instead. --Juan
Am Dienstag, den 16.09.2008, 07:58 -0700 schrieb Juan Lang:
Hi Peter,
diff --git a/programs/services/services.c b/programs/services/services.c index 36ed117..cd71a1f 100644 --- a/programs/services/services.c +++ b/programs/services/services.c @@ -106,9 +106,9 @@ static DWORD load_service_config(HKEY hKey, struct service_entry *entry) return err; if ((err = load_reg_string(hKey, SZ_DESCRIPTION, 0, &entry->description)) != 0) return err; - if ((err = load_reg_multisz(hKey, SZ_DEPEND_ON_SERVICE, &entry->dependOnServices)) != 0) + if ((err = load_reg_multisz(hKey, SZ_DEPEND_ON_SERVICE, TRUE, &entry->dependOnServices)) != 0) return err; - if ((err = load_reg_multisz(hKey, SZ_DEPEND_ON_GROUP, &entry->dependOnGroups)) != 0) + if ((err = load_reg_multisz(hKey, SZ_DEPEND_ON_GROUP, FALSE, &entry->dependOnGroups)) != 0) return err;
It seems as though the bAllowSingle variable is useless, as the only caller of it tries first with TRUE and again with FALSE.
No, you are reading the patch wrong. It tries to read DependOnService with TRUE, and aborts if it *fails*. If it succeeds, it loads *another* setting (DependOnGroup) with FALSE, so definitely not a retry. Peter, are you sure that windows will handle REG_SZ *only* in the DependOnService case and fail if you have a REG_SZ in DependOnGroup? Regards, Michael Karcher
Hello Michael. Thanks for your input (and Juan and Austin).
Peter, are you sure that windows will handle REG_SZ *only* in the DependOnService case and fail if you have a REG_SZ in DependOnGroup?
No. But i did not want to change the behavior for anything else than the scenario that i had been investigating and testing, to avoid regressions. But i am confident that the similar behavior can be found for DependOnGroup if you look into it. That is another patch however, which i do not want to mix into this one. I have re-submitted the patch, Thanks, /pedro
Am Mittwoch, den 17.09.2008, 01:17 +0200 schrieb Peter Dons Tychsen:
Peter, are you sure that windows will handle REG_SZ *only* in the DependOnService case and fail if you have a REG_SZ in DependOnGroup? No. But i did not want to change the behavior for anything else than the scenario that i had been investigating and testing, to avoid regressions.
I can definitely understand your motivation. The problem is that this patch seems to add unneeded complexity to wine which might cause you problems getting it committed. Experimenting with native windows showed strange results. If I replace the DependOnService entry in my RemoteAccess service entry (it has a REG_MULTI_SZ hat contains just one entry) in the registry by an REG_SZ, services.msc still shows the dependency in the properties dialog. If I replace the DependOnGroup (also a REG_MULTI_SZ with one entry) by a REG_SZ entry, it gets ignored completely by services.msc in the properties dialog. So, it might be correct to handle the two cases different, as I see different behaviour on native. But this kind of thing should be tested (at least manually as I did) and documented (perhaps in your commit message) that the different loading behaviour for DependOnService and DependOnGroup is verified. Just for reference: WinXP SP3 in KVM here. Regards, Michael Karcher
participants (4)
-
Darragh Bailey -
Juan Lang -
Michael Karcher -
Peter Dons Tychsen