Re: dlls/msi/streams.c -- simplify and constify

[Gerald Pfeifer]

On Sat, 3 Nov 2007, James Hawkins wrote:
> This change is wrong.  If you'd actually read what the code intended to 
> do instead of just fixing warnings, you'd see that add_streams_to_table 
> returns -1 on error.

That's what I am actually doing -- trying to read the code, including all
invocations of functions that I modify.  And doing so I have encountered
(and fixed) more than one real bug.  It does mean I need to dig into many
different corner of Wine that I have never seen before and, well, errare
humanum est, so I really appreciate review and feedback like yours. ;-)
> While the check for < 0 is not correct, removing the check entirely
> is wrong.  The check should be if (sv->num_rows == -1) return 
> ERROR_FUNCTION_FAILED.

Good observation, though comparing an unsigned type against -1 is not
exactly good practice.  Delving more into msi/streams.c, I came up with 
the patch below which tries to address this (and some other issues).

If Alexandre and you prefer, I can change the check and really make it
just read sv->num_rows == -1.  I just wanted to point out there also is
an alternative way.

Thanks again for your feedback.  It was very helpful, and I'll watch
out for cases like this.

Gerald

Index: streams.c
===================================================================
RCS file: /home/wine/wine/dlls/msi/streams.c,v
retrieving revision 1.7
diff -u -3 -p -r1.7 streams.c
--- streams.c	18 Oct 2007 13:00:57 -0000	1.7
+++ streams.c	3 Nov 2007 22:01:13 -0000
@@ -39,7 +39,7 @@ WINE_DEFAULT_DEBUG_CHANNEL(msidb);

typedef struct tabSTREAM
 {
-    int str_index;
+    UINT str_index;
     LPWSTR name;
     IStream *stream;
 } STREAM;
@@ -54,7 +54,7 @@ typedef struct tagMSISTREAMSVIEW
     UINT row_size;
 } MSISTREAMSVIEW;

-static BOOL add_stream_to_table(MSISTREAMSVIEW *sv, STREAM *stream, int index)
+static BOOL add_stream_to_table(MSISTREAMSVIEW *sv, STREAM *stream, UINT index)
 {
     if (index >= sv->max_streams)
     {
@@ -315,7 +315,7 @@ static UINT STREAMS_modify(struct tagMSI
 static UINT STREAMS_delete(struct tagMSIVIEW *view)
 {
     MSISTREAMSVIEW *sv = (MSISTREAMSVIEW *)view;
-    int i;
+    UINT i;

TRACE("(%p)\n", view);

@@ -381,7 +381,8 @@ static const MSIVIEWOPS streams_ops =
     NULL,
 };

-static UINT add_streams_to_table(MSISTREAMSVIEW *sv)
+static int add_streams_to_table(MSISTREAMSVIEW *sv)
+    /* Return -1 in case of error. */
 {
     IEnumSTATSTG *stgenum = NULL;
     STATSTG stat;
@@ -433,6 +434,7 @@ static UINT add_streams_to_table(MSISTRE
 UINT STREAMS_CreateView(MSIDATABASE *db, MSIVIEW **view)
 {
     MSISTREAMSVIEW *sv;
+    INT i;

TRACE("(%p, %p)\n", db, view);

@@ -442,10 +444,12 @@ UINT STREAMS_CreateView(MSIDATABASE *db,

sv->view.ops = &streams_ops;
     sv->db = db;
-    sv->num_rows = add_streams_to_table(sv);

-    if (sv->num_rows < 0)
-        return ERROR_FUNCTION_FAILED;
+    i = add_streams_to_table(sv);
+    if (i < 0)
+       return ERROR_FUNCTION_FAILED;
+    else
+       sv->num_rows = i;

*view = (MSIVIEW *)sv;