Rejected patches needing review.

[Peter Dons Tychsen]

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

[Darragh Bailey]

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.

[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;

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

[Michael Karcher]

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

[Peter Dons Tychsen]

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

[Michael Karcher]

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