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

[James Hawkins]

On Nov 3, 2007 1:10 PM, Gerald Pfeifer <gerald(a)pfeifer.com> wrote:
> While removing the dead code I noticed that we can actually also
> strengthen the const-ness of this function.
>
> Gerald
>
> ChangeLog:
> Remove a check which never could trigger (due to the domain of the
> variable in question) and increase const-ness of STREAMS_CreateView().
>
> Index: dlls/msi/streams.c
> ===================================================================
> RCS file: /home/wine/wine/dlls/msi/streams.c,v
> retrieving revision 1.7
> diff -u -3 -p -r1.7 streams.c
> --- dlls/msi/streams.c  18 Oct 2007 13:00:57 -0000      1.7
> +++ dlls/msi/streams.c  3 Nov 2007 18:06:27 -0000
> @@ -430,7 +430,7 @@ static UINT add_streams_to_table(MSISTRE
>      return count;
>  }
>
> -UINT STREAMS_CreateView(MSIDATABASE *db, MSIVIEW **view)
> +UINT STREAMS_CreateView(const MSIDATABASE *db, MSIVIEW **view)
>  {
>      MSISTREAMSVIEW *sv;
>
> @@ -444,9 +444,6 @@ UINT STREAMS_CreateView(MSIDATABASE *db,
>      sv->db = db;
>      sv->num_rows = add_streams_to_table(sv);
>
> -    if (sv->num_rows < 0)
> -        return ERROR_FUNCTION_FAILED;
> -
>      *view = (MSIVIEW *)sv;
>
> return ERROR_SUCCESS;

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.  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.

-- 
James Hawkins

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;

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

[Gerald Pfeifer]

For the record, while I don't think I saw a response to this, the
issue itself was now fixed with basically the same patch submitted
by Marcusso we can remove this from our radars.

Thanks for trying again, Marcus! :-)

Gerald

On Sat, 3 Nov 2007, Gerald Pfeifer wrote:
> 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;