Re: [MSI 1/4] add streams table

[Mike McCormack]

Hi,

Thanks for submitting the patch, and keeping the coding style consistent!

Andrey Turkin wrote:
> This patch adds virtual _Streams table to MSI because native MSI
> maintains such table
>
> ChangeLog:
> virtual _Streams table added
> +static UINT STREAMS_fetch_stream( struct tagMSIVIEW *view, UINT row, UINT col, IStream **stm )
> +{

....
> -
> - /*
> -
> * The column marked with the type stream data seems to have a single number
> -
> * which references the column containing the name of the stream data
> -
> *
> -
> * Fetch the column to reference first.
> -
> */
> - r = view->ops->fetch_int( view, row, col, &ival );

I'm not so keen on the way this is done.

You seem to have duplicated alot of code from TABLE_fetch_stream here.
> - if( r != ERROR_SUCCESS )
> - {
> -
>    ERR("1: %d\n", r);
> -
>    return r;
> - }
> -
> - ERR("fetched %d\n", ival);

And forgotten to remove your debug code.
> +MSIVIEWOPS streams_ops =
> +{
> - TABLE_fetch_int,
> - STREAMS_fetch_stream,
> - TABLE_set_int,
> - TABLE_insert_row,
> - STREAMS_execute,
> - TABLE_close,
> - TABLE_get_dimensions,
> - TABLE_get_column_info,
> - TABLE_modify,
> - TABLE_delete,
> - TABLE_find_matching_rows
>
> +};

The streams table is just another table.  How about generating an 
MSITABLE structure containing the data rather than treat it as a special 
case in here?

Mike

[MSI 1/4] add streams table

[Andrey Turkin]

Mike McCormack wrote:
> Hi,
>
> Thanks for submitting the patch, and keeping the coding style consistent!

Well, i tried :)
> Andrey Turkin wrote:
> > This patch adds virtual _Streams table to MSI because native MSI
> > maintains such table
> >
> > ChangeLog:
> > virtual _Streams table added
> > +static UINT STREAMS_fetch_stream( struct tagMSIVIEW *view, UINT row,
> > UINT col, IStream **stm )
> > +{
>
> ....
> > -
> > - /*
> > -
> > * The column marked with the type stream data seems to have a
> >
> > single number
> > -
> > * which references the column containing the name of the stream
> >
> > data
> > -
> > *
> > -
> > * Fetch the column to reference first.
> > -
> > */
> > - r = view->ops->fetch_int( view, row, col, &ival );
>
> I'm not so keen on the way this is done.
>
> You seem to have duplicated alot of code from TABLE_fetch_stream here.
> > - if( r != ERROR_SUCCESS )
> > - {
> > -
> >    ERR("1: %d\n", r);
> > -
> >    return r;
> > - }
> > -
> > - ERR("fetched %d\n", ival);
>
> And forgotten to remove your debug code.

Oops. It happens.
> > +MSIVIEWOPS streams_ops =
> > +{
> > - TABLE_fetch_int,
> > - STREAMS_fetch_stream,
> > - TABLE_set_int,
> > - TABLE_insert_row,
> > - STREAMS_execute,
> > - TABLE_close,
> > - TABLE_get_dimensions,
> > - TABLE_get_column_info,
> > - TABLE_modify,
> > - TABLE_delete,
> > - TABLE_find_matching_rows
> >
> > +};
>
> The streams table is just another table.  How about generating an
> MSITABLE structure containing the data rather than treat it as a special
> case in here?

I found about ten errors in various DLLs, and fixed them, but, frankly
speaking, all of my patches are a bit hacky (sometimes it is shameless
quick'n'dirty workaround).
I've decided to go forth and find all the errors blocking .NET Framework
installer (I'll submit tests shortly for error found so far). After
that, when all the errors are known, I will try to write good
"submittable" patches to fix the problems.
The main problem with writing actual fixes for me is lack of
knowledge/experience. I would have to get myself acquainted with target
code in order to be able to answer key questions ("Could streams be
added on fly? Should URL_ParseUrl parse c:\blah as Url or not? etc)

/me personally dislikes idea of adding internal tables to the database
structures. I thought that it is better to split real and virtual
tables, that's why I did things in such way. Well, as I said, I would
have to think before rewriting this patch :)

Regards,
  Andrey.